Add arti-pack plugin manager scaffold #1

Merged

benvin merged 6 commits from benvin/scaffold into main 2026-09-09 23:19:48 +10:00

Conversation 3 Commits 6 Files Changed 24

+2255 -1

unkin-agent commented 2026-09-08 23:22:36 +10:00

Member

Copy Link

Copy Source

Neovim plugin managers clone from GitHub, putting the editor on the public network and pinning nothing but a moving branch. arti-pack fetches tagged source archives from any HTTP artifact repository instead, so every plugin arrives as an immutable, cached artifact.

• add lua modules for spec parsing, lock state, fs helpers and the installer
• add ArtiPackSync/ArtiPackClean/ArtiPackList commands and help doc
• require an explicit registry, failing with a clear message when it is unset
• add headless nvim test harness and specs
• add make test/lint/fmt/pre-commit and patch/minor/major bump targets
• add woodpecker test, lint and pre-commit pipelines

Neovim plugin managers clone from GitHub, putting the editor on the public network and pinning nothing but a moving branch. arti-pack fetches tagged source archives from any HTTP artifact repository instead, so every plugin arrives as an immutable, cached artifact. - add lua modules for spec parsing, lock state, fs helpers and the installer - add ArtiPackSync/ArtiPackClean/ArtiPackList commands and help doc - require an explicit registry, failing with a clear message when it is unset - add headless nvim test harness and specs - add make test/lint/fmt/pre-commit and patch/minor/major bump targets - add woodpecker test, lint and pre-commit pipelines

unkin-agent added 1 commit 2026-09-08 23:22:36 +10:00

Add arti-pack plugin manager scaffold ... 46517953e5

- add lua modules for spec parsing, lock state, fs helpers and the installer
- add ArtiPackSync/ArtiPackClean/ArtiPackList commands and help doc
- add headless nvim test harness and specs
- add make test/lint/fmt/pre-commit and patch/minor/major bump targets
- add woodpecker test, lint and pre-commit pipelines

unkin-agent commented 2026-09-08 23:27:03 +10:00

Author

Member

Copy Link

Copy Source

Review findings — diff matches the PR body (all five bullets accounted for, nothing unaccounted extra), body follows conventions, and this is a coherent atomic scaffold (fine to keep as one PR for a new repo). Tests for spec/fs/lock/installer are genuinely good (path-traversal cases, corrupt-archive rejection, cache reuse, concurrency, partial-failure isolation).

Must fix

1. lua/arti-pack/init.lua:122 — M.clean() calls local specs = spec_mod.parse_all(M.config.plugins), capturing only the first return value. spec_mod.parse_all returns specs, errors (see correct usage at line 95 and 157). Any spec entry that fails to parse (bad tag, invalid ext, malformed source, etc.) is silently dropped from specs/wanted, so :ArtiPackClean treats that plugin's already-installed directory as orphaned and fs.rm_rfs it, and strips its lock entry — even though it's still listed in the user's config, just with a typo elsewhere. This directly contradicts the documented behavior ("Remove installed plugins no longer in the spec", README.md:70) and is exactly the "deletes the wrong directory" failure mode a plugin manager can't afford. Fix: capture errors too, and refuse to clean (or at least warn loudly) when parse_all returned any errors, rather than treating unparseable-but-configured plugins as orphaned.

2. lua/arti-pack/init.lua (sync/clean/list/setup) has zero test coverage — there's no init_spec.lua among the added specs (fs_spec, installer_spec, lock_spec, spec_spec only). This is the module owning the destructive clean() path and directory-move logic, and a single test asserting "an unparseable spec entry doesn't get its installed directory removed by :ArtiPackClean" would have caught finding #1.

Nice to have

3. All three .woodpecker/*.yaml set serviceAccountName: default instead of a dedicated arti-pack SA, with no accompanying argocd-apps PR. Low risk since none of these jobs touch secrets or deploy, but it's a stated org convention.

4. lock.lua:36 Lock:save() writes the JSON file in place (no temp+rename). An interrupted nvim process during save can leave a truncated lock.json; load() already tolerates that by resetting to {}, so the failure mode is "acts like first run" rather than data loss, but a temp+fs.rename write would be more robust and fs.rename already exists in this codebase.

5. Archive extraction (unzip -o, tar -xzf) relies entirely on the extraction tool's own defenses against path traversal / absolute-path entries in a crafted archive. Source is the estate's own artifactapi GitHub proxy (semi-trusted), so severity is low, but worth a one-line note on the trust assumption.

Review findings — diff matches the PR body (all five bullets accounted for, nothing unaccounted extra), body follows conventions, and this is a coherent atomic scaffold (fine to keep as one PR for a new repo). Tests for spec/fs/lock/installer are genuinely good (path-traversal cases, corrupt-archive rejection, cache reuse, concurrency, partial-failure isolation). **Must fix** 1. `lua/arti-pack/init.lua:122` — `M.clean()` calls `local specs = spec_mod.parse_all(M.config.plugins)`, capturing only the first return value. `spec_mod.parse_all` returns `specs, errors` (see correct usage at line 95 and 157). Any spec entry that fails to parse (bad tag, invalid ext, malformed source, etc.) is silently dropped from `specs`/`wanted`, so `:ArtiPackClean` treats that plugin's already-installed directory as orphaned and `fs.rm_rf`s it, and strips its lock entry — even though it's still listed in the user's config, just with a typo elsewhere. This directly contradicts the documented behavior ("Remove installed plugins no longer in the spec", README.md:70) and is exactly the "deletes the wrong directory" failure mode a plugin manager can't afford. Fix: capture `errors` too, and refuse to clean (or at least warn loudly) when `parse_all` returned any errors, rather than treating unparseable-but-configured plugins as orphaned. 2. `lua/arti-pack/init.lua` (sync/clean/list/setup) has zero test coverage — there's no `init_spec.lua` among the added specs (fs_spec, installer_spec, lock_spec, spec_spec only). This is the module owning the destructive `clean()` path and directory-move logic, and a single test asserting "an unparseable spec entry doesn't get its installed directory removed by :ArtiPackClean" would have caught finding #1. **Nice to have** 3. All three `.woodpecker/*.yaml` set `serviceAccountName: default` instead of a dedicated arti-pack SA, with no accompanying argocd-apps PR. Low risk since none of these jobs touch secrets or deploy, but it's a stated org convention. 4. `lock.lua:36` `Lock:save()` writes the JSON file in place (no temp+rename). An interrupted nvim process during save can leave a truncated lock.json; `load()` already tolerates that by resetting to `{}`, so the failure mode is "acts like first run" rather than data loss, but a temp+`fs.rename` write would be more robust and `fs.rename` already exists in this codebase. 5. Archive extraction (`unzip -o`, `tar -xzf`) relies entirely on the extraction tool's own defenses against path traversal / absolute-path entries in a crafted archive. Source is the estate's own artifactapi GitHub proxy (semi-trusted), so severity is low, but worth a one-line note on the trust assumption.

unkin-agent added 2 commits 2026-09-08 23:32:10 +10:00

Abort clean when a plugin spec fails to parse ... 3190b30e97

An unparseable entry dropped out of the wanted set, so clean deleted the
directory and lock entry of a plugin the user still has configured.

Add tests/init_spec.lua covering setup, sync, clean and list.

Save the lockfile through a temp file and a rename ... 11ce1fed9d

An interrupted save could leave a truncated lock.json.

unkin-agent added 1 commit 2026-09-09 22:35:05 +10:00

Describe arti-pack against any artifact repository ... 7def2a28a9

The docs described one specific deployment by hostname, reading as though
arti-pack only works there. It works against any HTTP artifact repository that
serves a tag archive at a stable URL.

- describe the registry as any HTTP artifact repository, with Artifactory as
  the worked example
- use a placeholder host in every configuration and URL example
- state that registry must be set, since the shipped default is site-specific

unkin-agent added 1 commit 2026-09-09 22:44:38 +10:00

Require an explicit registry instead of defaulting to one ... c75a866c02

The shipped default named one private, internet-unreachable host, so an
unconfigured install failed against a server it could never reach rather
than saying what to set.

- drop the registry default and make the option required
- reject a non-string, empty, or non-http(s) registry with the option name
  and how to set it
- ask for a registry only when a run has to resolve one, so clean, list,
  and specs with an explicit url keep working without it

unkin-agent commented 2026-09-09 22:52:01 +10:00

Author

Member

Copy Link

Copy Source

Reviewed the two changes since the last clean pass (de-branding, required registry, commit c75a866). The registry contract's core logic is genuinely solid — verified all entry points (setup, sync, clean, list, and a command invoked before setup()), and confirmed by actually deleting the guard and running make test: 5 tests fail concretely (spec_spec.lua:105/114, init_spec.lua:159/203/215), so these aren't guard-less rubber stamps. M.url's trailing-slash stripping is byte-for-byte unchanged from before c75a866. De-branding is clean: no artifactapi/k8s.syd1.au left in lua/, doc/, README.md, or tests/; the one remaining git.unkin.net hit (README.md:24, the git clone install command) is this repo's own real, public clone URL, not a private-deployment assumption — fine to keep, same rationale as the .woodpecker/ carve-out.

Must fix

1. lua/arti-pack/spec.lua:73-81 (M.check_registry) — the http(s) check only inspects the character immediately after ://; it never trims the value, so a registry with leading/trailing whitespace (a stray newline from a file read, a copy-pasted trailing space) sails through validation and gets baked verbatim into the resolved URL instead of being rejected. Reproduced directly:

   check_registry('https://host ')  -> base = "https://host " , err = nil
   spec.url(s, 'https://host ')     -> "https://host /owner/repo/archive/refs/tags/v1.zip"
   

   That malformed URL goes straight to curl (installer.lua:75), producing a raw curl error instead of the intended clear "registry is not configured" message — exactly the failure mode this commit set out to close. Fix: registry = trim(registry) (the file already defines trim() for owner/tag) before matching in check_registry.

2. PR body doesn't describe the final state. It still only lists the original 5-bullet scaffold; it says nothing about the registry now being a required, validated option (the change this PR revision is centered on) or about the docs being rewritten vendor-neutral, and doesn't mention the two bugfix commits (clean aborts on parse errors, lockfile saved via temp+rename) that also landed on this branch. Please rewrite the body to reflect what's actually in the diff today.

Nice to have (carried over, unaddressed, low risk — not blocking)

3. All three .woodpecker/*.yaml still use serviceAccountName: default with no dedicated SA / argocd-apps PR.
4. Archive extraction still relies solely on unzip/tar's own path-traversal defenses against a crafted archive; worth a one-line trust-assumption note.

Reviewed the two changes since the last clean pass (de-branding, required registry, commit c75a866). The registry contract's core logic is genuinely solid — verified all entry points (`setup`, `sync`, `clean`, `list`, and a command invoked before `setup()`), and confirmed by actually deleting the guard and running `make test`: 5 tests fail concretely (`spec_spec.lua:105/114`, `init_spec.lua:159/203/215`), so these aren't guard-less rubber stamps. `M.url`'s trailing-slash stripping is byte-for-byte unchanged from before c75a866. De-branding is clean: no `artifactapi`/`k8s.syd1.au` left in `lua/`, `doc/`, `README.md`, or `tests/`; the one remaining `git.unkin.net` hit (README.md:24, the `git clone` install command) is this repo's own real, public clone URL, not a private-deployment assumption — fine to keep, same rationale as the `.woodpecker/` carve-out. **Must fix** 1. `lua/arti-pack/spec.lua:73-81` (`M.check_registry`) — the http(s) check only inspects the character immediately after `://`; it never trims the value, so a registry with leading/trailing whitespace (a stray newline from a file read, a copy-pasted trailing space) sails through validation and gets baked verbatim into the resolved URL instead of being rejected. Reproduced directly: ``` check_registry('https://host ') -> base = "https://host " , err = nil spec.url(s, 'https://host ') -> "https://host /owner/repo/archive/refs/tags/v1.zip" ``` That malformed URL goes straight to curl (`installer.lua:75`), producing a raw curl error instead of the intended clear "registry is not configured" message — exactly the failure mode this commit set out to close. Fix: `registry = trim(registry)` (the file already defines `trim()` for owner/tag) before matching in `check_registry`. 2. PR body doesn't describe the final state. It still only lists the original 5-bullet scaffold; it says nothing about the registry now being a required, validated option (the change this PR revision is centered on) or about the docs being rewritten vendor-neutral, and doesn't mention the two bugfix commits (clean aborts on parse errors, lockfile saved via temp+rename) that also landed on this branch. Please rewrite the body to reflect what's actually in the diff today. **Nice to have (carried over, unaddressed, low risk — not blocking)** 3. All three `.woodpecker/*.yaml` still use `serviceAccountName: default` with no dedicated SA / argocd-apps PR. 4. Archive extraction still relies solely on `unzip`/`tar`'s own path-traversal defenses against a crafted archive; worth a one-line trust-assumption note.

unkin-agent added 1 commit 2026-09-09 22:56:23 +10:00

Trim whitespace out of the registry and spec urls ... e61e950d30

A registry with a stray space or newline passed validation and was
concatenated verbatim into the archive URL, so curl saw the malformed
address instead of the user seeing the required-registry error. Trim the
value before matching and reject any whitespace left inside it; apply the
same to a spec's explicit url.

unkin-agent commented 2026-09-09 23:03:12 +10:00

Author

Member

Copy Link

Copy Source

Reviewed e61e950 against the round-2 finding. Verified independently (not from the diff/commit message): built spec.check_registry/spec.url by hand for leading space, trailing space, trailing newline, embedded newline, embedded space, tab (leading and embedded), CR, whitespace-only, and "scheme followed only by whitespace" — every case either trims to a clean URL or is rejected with the "not an http(s) URL" error; none reach curl malformed. Also reverted lua/arti-pack/spec.lua to c75a866 with the new tests in place: make test gives 6 concrete failures, then 0 with the fix restored, confirming they're real regression tests. The whitespace hole from round 2 is genuinely closed, and registry/url/tag/owner+repo now share the same trim-outer-reject-inner rule, documented accurately in README.md and doc/arti-pack.txt.

Must fix

1. PR body still does not describe the branch's final state, despite round 2 explicitly asking for a rewrite. Current body is byte-for-byte the original 5-bullet scaffold list from the first commit — it says nothing about the registry now being required and validated (whitespace-trimmed, rejects embedded whitespace), nothing about the docs being rewritten vendor-neutral, and nothing about the two bugfix commits on this branch (3190b30 clean aborts when a spec fails to parse, 11ce1fe lockfile saved via temp+rename). A reader of the PR page today gets no idea any of that shipped. Please rewrite it to reflect what's actually in the diff now.

Nice to have

2. tests/spec_spec.lua:176-182 ('rejects a whitespace-only registry') is non-load-bearing: whitespace-only strings (' ', '\n', etc.) were already rejected by the pre-fix check_registry too, since they never matched the leading '^https?://' anchor with or without a trim step. Confirmed by reverting spec.lua to c75a866: this specific test still passes while the other 5 new tests fail. It doesn't hurt anything, but it isn't testing the trim fix — worth either removing or replacing with a case that only fails without the trim (e.g. 'https://x ' alone, already covered elsewhere, or noting it's a belt-and-suspenders check).
3. lua/arti-pack/spec.lua:49-52 (as/name via bad_name, line 12-14) doesn't follow the same trim-then-check rule as registry/url/tag: it never trims, so as = 'tokyonight ' (trailing-space typo) is flatly rejected as an "unusable directory name" instead of being trimmed and accepted like the same typo would be on registry or url. Not a security issue — rejecting is the safe direction — but it's a different rule for a field the docs don't call out either way, worth a one-line README/doc note or aligning the behavior for consistency.

Reviewed `e61e950` against the round-2 finding. Verified independently (not from the diff/commit message): built `spec.check_registry`/`spec.url` by hand for leading space, trailing space, trailing newline, embedded newline, embedded space, tab (leading and embedded), CR, whitespace-only, and "scheme followed only by whitespace" — every case either trims to a clean URL or is rejected with the "not an http(s) URL" error; none reach curl malformed. Also reverted `lua/arti-pack/spec.lua` to `c75a866` with the new tests in place: `make test` gives 6 concrete failures, then 0 with the fix restored, confirming they're real regression tests. The whitespace hole from round 2 is genuinely closed, and `registry`/`url`/`tag`/`owner`+`repo` now share the same trim-outer-reject-inner rule, documented accurately in README.md and doc/arti-pack.txt. **Must fix** 1. PR body still does not describe the branch's final state, despite round 2 explicitly asking for a rewrite. Current body is byte-for-byte the original 5-bullet scaffold list from the first commit — it says nothing about the registry now being required *and validated* (whitespace-trimmed, rejects embedded whitespace), nothing about the docs being rewritten vendor-neutral, and nothing about the two bugfix commits on this branch (`3190b30` clean aborts when a spec fails to parse, `11ce1fe` lockfile saved via temp+rename). A reader of the PR page today gets no idea any of that shipped. Please rewrite it to reflect what's actually in the diff now. **Nice to have** 2. `tests/spec_spec.lua:176-182` (`'rejects a whitespace-only registry'`) is non-load-bearing: whitespace-only strings (`' '`, `'\n'`, etc.) were already rejected by the *pre-fix* `check_registry` too, since they never matched the leading `'^https?://'` anchor with or without a trim step. Confirmed by reverting `spec.lua` to `c75a866`: this specific test still passes while the other 5 new tests fail. It doesn't hurt anything, but it isn't testing the trim fix — worth either removing or replacing with a case that only fails without the trim (e.g. `'https://x '` alone, already covered elsewhere, or noting it's a belt-and-suspenders check). 3. `lua/arti-pack/spec.lua:49-52` (`as`/name via `bad_name`, line 12-14) doesn't follow the same trim-then-check rule as `registry`/`url`/`tag`: it never trims, so `as = 'tokyonight '` (trailing-space typo) is flatly rejected as an "unusable directory name" instead of being trimmed and accepted like the same typo would be on `registry` or `url`. Not a security issue — rejecting is the safe direction — but it's a different rule for a field the docs don't call out either way, worth a one-line README/doc note or aligning the behavior for consistency.

benvin merged commit c7da77a588 into main 2026-09-09 23:19:48 +10:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/arti-pack#1