diff --git a/.claude/spec/nix-flake-packaging.md b/.claude/spec/nix-flake-packaging.md index 9832eb7..f764f6d 100644 --- a/.claude/spec/nix-flake-packaging.md +++ b/.claude/spec/nix-flake-packaging.md @@ -200,15 +200,30 @@ The workflow keeps its container-and-npm shape, deliberately preserving the GitH ## Further Notes -### Open verification item +### Resolved verification item: the hook records the absolute path under Nix The `setup` command's hook installation passes the SDK both an absolute path to the running entrypoint and the bare binary name. -Under Nix the absolute path is content-addressed: it changes on every rebuild and is eventually garbage-collected, so a hook recording it would break silently, since a session-start hook that cannot execute simply does not run. -The bare binary name strongly suggests the SDK prefers search-path resolution and treats the absolute path as a fallback, which would make this a non-issue, but this could not be confirmed during design because the dependency was not installed. +The design-time hope was that the SDK prefers search-path resolution and treats the absolute path as a fallback, which would have made this a non-issue. +It was verified against the installed dependency and against a real Nix build, and the answer is the unfavourable one: **under Nix the absolute store path is recorded**, even with the binary on `PATH`. -The decision is to verify before acting. -If the SDK does record the absolute path, the immediate mitigation is documenting that the hook setup should be re-run after an upgrade. -Changing the `setup` command to prefer the bare name would then become a separate task with its own ADR — justified on the grounds that a stable search-path name is more robust for *every* installation method, and explicitly not as a special case that detects Nix store paths in application code. +The SDK's `resolvePortableHookCommand` returns the bare name only when some `PATH` entry *realpath-matches* the entrypoint, and the absolute path otherwise. +That test is what splits the two installation methods, and the split is a property of how each one puts the binary on `PATH`: + +- **npm** symlinks the `bin` entry directly at the entrypoint, so the realpath comparison succeeds and the bare name is recorded. +- **Nix** installs the `bin` entry as a *generated wrapper script* that invokes `node ` — `nodejsInstallExecutables` inside `npmInstallHook`, plus this package's own `makeWrapper` layer for `git` and `tea`. + A wrapper's realpath is the wrapper, never the entrypoint, so the comparison cannot succeed and the absolute path is recorded. + +So the preference for the bare name is real, but it is unreachable through any wrapper-based install. +It is not that Nix was overlooked; it is that the mechanism keys on a filesystem relationship only the symlink shape has. + +The mitigation is therefore documentation, per the decision recorded when this item was opened: the `setup` command's help text states that `setup hooks` must be re-run after an upgrade. +The failure it guards against is silent — a session-start hook that cannot execute simply does not run, so a user gets no error, only the quiet absence of their ambient dashboard. + +Changing the `setup` command to prefer the bare name remains a separate task with its own ADR, justified on the grounds that a stable search-path name is more robust for *every* installation method, and explicitly not as a special case that detects Nix store paths in application code. +Two findings feed that future task. +First, the mitigation above is documentation against a silent failure, which is the weakest kind of fix. +Second, a related defect shares the same line: `isManagedHook` recognises its own hook by testing whether the recorded command string *contains* the marker `gitea-axi`, so an entrypoint path lacking that substring makes `setup hooks` append a duplicate rather than update in place, contradicting the idempotency its help text promises. +That coupling is why `package.nix` renames its build tree in `postUnpack`; the rename can be deleted once the hook no longer depends on the entrypoint path. ### Verified during design diff --git a/.claude/tasks/0042-verify-hook-path-resolution.md b/.claude/tasks/0042-verify-hook-path-resolution.md index b89d8b6..4f60576 100644 --- a/.claude/tasks/0042-verify-hook-path-resolution.md +++ b/.claude/tasks/0042-verify-hook-path-resolution.md @@ -19,10 +19,10 @@ Changing the `setup` command to prefer the bare name is explicitly **not** part ## Acceptance criteria -- [ ] The SDK's actual hook-path behavior is determined by observation against the installed dependency, not inference from its interface. -- [ ] The finding is recorded where a future reader will meet it, so the question is not re-opened from scratch. -- [ ] If the absolute path is recorded, the documentation states that hook setup must be re-run after an upgrade. -- [ ] No change is made to how the `setup` command constructs the hook in this task. +- [x] The SDK's actual hook-path behavior is determined by observation against the installed dependency, not inference from its interface. +- [x] The finding is recorded where a future reader will meet it, so the question is not re-opened from scratch. +- [x] If the absolute path is recorded, the documentation states that hook setup must be re-run after an upgrade. +- [x] No change is made to how the `setup` command constructs the hook in this task. ## Evidence gathered during task 0037 @@ -47,3 +47,46 @@ Consequences for this task: - Both defects trace to the same resolution line, so they should be weighed together. - The stale store path is user-facing breakage on the install method task 0037 added, which argues for not letting this drift far behind it. - `package.nix` carries a `postUnpack` rename of the build tree purely to work around the substring coupling. It is commented as a workaround and should be **deleted as part of this task**, once the hook no longer depends on the entrypoint path. + +## Implementation Notes + +### The observation + +Task 0037's evidence was re-verified independently rather than taken on trust, since acceptance criterion 1 asks for observation and not for citation. +Two observations were made against the installed dependency. + +A probe drove `resolvePortableHookCommand` directly with two synthetic install trees. +Given a `PATH` entry that is a *symlink* to the entrypoint it returned the bare name `gitea-axi`; given a `PATH` entry that is a *wrapper script* invoking `node ` it returned the absolute path. +Then the flake was built and the resulting binary driven for real against a temporary `HOME`. +With `$out/bin` on `PATH`, `~/.claude/settings.json` recorded: + +``` +/nix/store/pqxhyy5cg1rljyn78kxfpxyfpgz7rgzk-gitea-axi-0.1.0/lib/node_modules/gitea-axi/dist/main.js +``` + +This confirms 0037's finding and sharpens it. +The task framed the bare name as a hint that the SDK "prefers search-path resolution"; that preference is real, but it is gated on a `PATH` entry whose realpath equals the entrypoint. +npm satisfies that by symlinking its `bin` entry; Nix cannot, because `nodejsInstallExecutables` generates a wrapper script and this package adds a second `makeWrapper` layer for `git` and `tea`. +So the behaviour is not Nix-specific — it applies to *any* wrapper-based install — which strengthens the case, already recorded, that the eventual fix belongs in the `setup` command for every installation method rather than as a Nix special case. + +### Deviations + +**The `postUnpack` rename in `package.nix` was kept, not deleted.** +The "Evidence gathered during task 0037" section above says it "should be **deleted as part of this task**", which conflicts with acceptance criterion 4 and with the "What to build" section's statement that changing the hook to prefer the bare name is "explicitly **not** part of this task". +The conflict resolves on the Evidence section's own wording: the deletion is conditioned on "once the hook no longer depends on the entrypoint path", and establishing that precondition is exactly the out-of-scope change. +Deleting the rename now would leave the derivation's build tree at a path with no `gitea-axi` segment, which `isManagedHook`'s substring test still requires, and `test/setup.test.ts` would fail inside `checkPhase`. +The rename's comment was rewritten instead: it previously promised that task 0042 would remove the coupling, which would have become a stale forward reference the moment this task landed. + +**The documentation surface is the `setup` help text.** +The repository has no README, so the command's own help is the only place a user meets this. +Criterion 4 is untouched — `binaryNames`, `execPath`, and the `installSessionStartHooks` call are all unchanged; only the `usage` string moved. + +**One sentence beyond the strict ask.** +The help text asserted "Both are idempotent" unqualified, which the finding recorded in this same commit makes false for the duplicate-append case. +Leaving a statement the commit itself documents as untrue seemed worse than a one-line caveat, so the new paragraph notes that a stale entry may survive a re-run. + +### Follow-up + +The successor task is described in the spec's Further Notes but not yet written as a task file. +It should carry its own ADR and cover both defects together, since both trace to the same resolution line: the absolute-path recording, and `isManagedHook` recognising its hook by substring. +Deleting `package.nix`'s `postUnpack` rename belongs to it. diff --git a/CLAUDE.md b/CLAUDE.md index 1edf9fe..14b4ed1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -71,3 +71,9 @@ Its `act` fork declares `RawContinueOnError` on the `Step` struct only — `pkg/ Gitea's own syntax-comparison page does not list the gap. Put `continue-on-error` on each step instead: `act` and GitHub Actions both honour it there, and a job whose every step carries it concludes green on either platform. The same fork historically ignored `jobs..if` (go-gitea#25897), so treat any job-level key as needing a check against the fork's structs rather than against GitHub's documentation. + +The session-start hook installed by `setup hooks` records the entrypoint's **absolute path**, not the bare binary name, on every wrapper-based install. +`resolvePortableHookCommand` in `axi-sdk-js` returns the bare name only when a `PATH` entry *realpath-matches* the entrypoint. +npm symlinks its `bin` entry straight at `dist/main.js`, so that match succeeds; Nix installs a generated wrapper script that invokes `node `, whose realpath is the wrapper, so the match cannot succeed and the store path is recorded. +Passing `binaryNames` therefore does not make the hook portable under Nix — verify by driving the installed binary and reading `~/.claude/settings.json`, not by reading the SDK's interface. +The consequence is silent: a hook whose recorded path no longer exists does not run and does not warn. diff --git a/package.nix b/package.nix index 4537c11..9c74e1c 100644 --- a/package.nix +++ b/package.nix @@ -68,9 +68,12 @@ buildNpmPackage { # finding "gitea-axi" within it. Naming the tree makes the build representative # rather than an environment no operator ever has. # - # This coupling is a defect, not a property worth preserving — see task 0042, - # which removes the hook's dependence on the entrypoint path entirely. Once it - # lands this rename should go with it. + # This coupling is a defect, not a property worth preserving. Task 0042 + # verified the resolution behaviour and documented the mitigation, but left + # the hook's dependence on the entrypoint path in place: removing it needs its + # own ADR, since a stable search-path name is the right answer for every + # installation method and not a Nix special case. This rename goes away with + # that task, not before. postUnpack = '' mv "$sourceRoot" gitea-axi export sourceRoot=gitea-axi diff --git a/src/commands/setup.ts b/src/commands/setup.ts index 81290b9..3c258dd 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -18,6 +18,14 @@ Install gitea-axi's ambient context for agent sessions. Both are idempotent: re-running updates the managed files in place rather than failing. There is no postinstall script — installation is always explicit. +Re-run "setup hooks" after upgrading gitea-axi. The hook records an absolute +path to the entrypoint, which moves when the install location changes, and a +session-start hook that cannot be executed fails silently rather than warning. +This matters most for immutable installs such as Nix, where every rebuild lands +the entrypoint at a fresh path and the old one is eventually collected. When the +path moves, the re-run may leave the stale entry behind instead of replacing it; +remove it by hand if a duplicate appears. + flags: --help Show this help `;