fix(setup): record the session-start hook as a search-path name (task 0043)
All checks were successful
CI / test (22) (pull_request) Successful in 47s
CI / test (true, 24) (pull_request) Successful in 1m6s
CI / flake (pull_request) Successful in 3s
CI / test (22) (push) Successful in 49s
CI / test (true, 24) (push) Successful in 1m5s
CI / flake (push) Successful in 2s
All checks were successful
CI / test (22) (pull_request) Successful in 47s
CI / test (true, 24) (pull_request) Successful in 1m6s
CI / flake (pull_request) Successful in 3s
CI / test (22) (push) Successful in 49s
CI / test (true, 24) (push) Successful in 1m5s
CI / flake (push) Successful in 2s
The SDK records a bare, upgrade-stable binary name only when a PATH entry realpath-matches the execPath it is handed. From the module-relative entrypoint that can only succeed under npm, which symlinks its bin entry straight at it; a wrapper-based install never can, because a script that invokes a file does not resolve to that file. So every wrapper install -- Nix, a shim, a generated .cmd -- recorded an absolute path that moves on upgrade, and a session-start hook that cannot execute fails silently. `setup hooks` now resolves gitea-axi on PATH itself and hands that location to the SDK, so the bare name is recorded. A candidate qualifies only if it resolves to the running entrypoint -- by realpath for a symlink, or by naming it in its text for a wrapper, following the chain, since a Nix install is two hops. A same-named binary that is some other program does not qualify, and the absolute entrypoint path stands as the fallback exactly as before. The related defect on the same line goes too: the SDK recognises its hook by finding the marker inside the recorded command, so an entrypoint path without "gitea-axi" in it made a re-run append a duplicate rather than update in place. `setup hooks` now prunes duplicates by matching the exact command it records, which is independent of that command's shape and cannot mistake another tool's hook for its own. With the coupling gone, package.nix no longer renames its build tree; the fast tier runs from /build/source and its idempotency test passes there. The help text's instruction to re-run hooks after an upgrade is deleted, having become false. Verified against the built Nix binary and a globally npm-installed pack: both record the bare name, both fall back to the absolute path when the name is absent, an impostor on PATH is refused, and re-runs leave one entry. Decision recorded as ADR 0019; ADR 0009's addendum is amended.
This commit was merged in pull request #52.
This commit is contained in:
@@ -28,3 +28,7 @@ gitea-axi adds the same opt-in `setup hooks`; the skill remains the default `set
|
||||
|
||||
Hooks are not the default because the hook runs the dashboard in every session in every directory, and outside a Gitea repo the dashboard errors with `REPO_NOT_FOUND` — a graceful exit-0 degradation was considered and rejected in favor of keeping the error explicit, so hook noise in non-Gitea sessions is an accepted consequence for users who opt in.
|
||||
The SDK registers the bare binary as the hook command, so the hook always runs the short dashboard tier (see ADR 0012).
|
||||
|
||||
**Amended by [ADR 0019](0019-hook-records-search-path-name.md):** that last sentence held only for npm installs.
|
||||
The SDK records the bare name only when a `PATH` entry realpath-matches the entrypoint it is handed, which npm's symlinked `bin` satisfies and a wrapper-based install cannot.
|
||||
`setup hooks` now resolves the binary on `PATH` itself and hands that location over, so the bare name is recorded for wrapper-based installs too; the absolute entrypoint path remains the fallback when the name resolves to no install of ours.
|
||||
|
||||
47
.claude/adr/0019-hook-records-search-path-name.md
Normal file
47
.claude/adr/0019-hook-records-search-path-name.md
Normal file
@@ -0,0 +1,47 @@
|
||||
# Record the SessionStart hook as a search-path name, not an install-tree path
|
||||
|
||||
`setup hooks` resolves `gitea-axi` on `PATH` and hands that location to `installSessionStartHooks`, so the recorded hook command is the bare name `gitea-axi`.
|
||||
When the name resolves nowhere on `PATH`, the module-relative entrypoint is handed over instead and the absolute path is recorded, exactly as before.
|
||||
|
||||
A `PATH` entry qualifies only when it resolves to the running entrypoint — a symlink pointing at it, or a generated wrapper naming it — so a same-named binary that is some other program does not count.
|
||||
|
||||
`setup hooks` also collapses duplicate managed entries itself, recognising its own hook by the exact command it records rather than by a substring of that command.
|
||||
|
||||
## Context
|
||||
|
||||
The agent SDK's `resolvePortableHookCommand` returns a bare binary name only when some `PATH` entry *realpath-matches* the entrypoint it is handed, and the absolute path otherwise.
|
||||
Task 0042 verified that this splits the two installation methods: npm symlinks its `bin` entry straight at `dist/main.js` so the match succeeds, while any wrapper-based install cannot match, because a script that *invokes* a file never resolves *to* that file.
|
||||
|
||||
Under Nix the recorded path is content-addressed — it changes on every rebuild and is eventually garbage-collected — and a SessionStart hook that cannot execute does not run and does not warn.
|
||||
|
||||
## Considered Options
|
||||
|
||||
**Document a re-run after upgrade** (rejected; this was the task 0042 mitigation being replaced) — Documentation against a silent failure is the weakest kind of fix.
|
||||
It also does not work reliably: the re-run may leave the stale entry behind rather than replacing it, so the help text had to caveat its own remedy.
|
||||
|
||||
**Detect store paths in application code** (rejected) — Special-casing Nix in the CLI is the wrong shape.
|
||||
The problem is not Nix; it is every wrapper-based install — a shim, a launcher, a generated `.cmd`.
|
||||
|
||||
**Write the hook files directly, bypassing the SDK** (rejected) — Would duplicate the SDK's handling of three integrations and four files to change one string, and would drift from it on every SDK change.
|
||||
|
||||
**Hand the SDK the `PATH` location** (chosen) — The SDK already prefers the search-path name; it was only ever reaching for it from the wrong end.
|
||||
Resolving the name the way a shell does and handing that over makes the SDK's own realpath test succeed, so the preference becomes reachable for every installation method rather than only for the symlink shape npm happens to use.
|
||||
Recording a bare name and letting `PATH` resolve it is also the convention for tools writing into user-owned configuration; absolute paths belong in configuration a package manager regenerates.
|
||||
|
||||
## Consequences
|
||||
|
||||
- The hook survives an upgrade whenever the binary is on `PATH`, so `setup hooks` no longer needs re-running after one, and the help text saying so is gone.
|
||||
- The absolute path remains the documented fallback for a binary that is not on `PATH` — a source checkout run through `node dist/main.js`, say — where it is the only thing that could work.
|
||||
- Which command gets recorded now depends on the invoking environment's `PATH`, not only on how the package was installed.
|
||||
`PATH` is therefore read from the process rather than from the injected environment, since it must agree with the SDK's own probing.
|
||||
- A same-named binary on `PATH` that is *not* this program does not qualify.
|
||||
The name must resolve to the running entrypoint — by realpath for a symlink, or by the wrapper naming it — or the fallback applies.
|
||||
Accepting any file that merely bears the name would make the SDK's realpath test a tautology, since the path handed over would trivially match itself.
|
||||
- The SDK recognises its managed hook by finding the marker inside the recorded command, which made a re-run append a duplicate whenever the entrypoint path lacked the substring `gitea-axi`.
|
||||
`setup hooks` now prunes duplicates by matching the exact command it records, so idempotency no longer depends on the recorded command's shape, and another tool's hook can never be mistaken for ours.
|
||||
- `package.nix` no longer renames its build tree in `postUnpack`.
|
||||
That rename existed only to give the fast tier an entrypoint path containing the marker.
|
||||
With the coupling gone the build runs from `/build/source` and the idempotency test passes there, which is what demonstrates the coupling is actually broken.
|
||||
- ADR 0009's addendum claimed "the SDK registers the bare binary as the hook command".
|
||||
That was true only for npm installs; as of this decision it is true for any install whose `PATH` entry resolves to this entrypoint.
|
||||
ADR 0009 is amended accordingly.
|
||||
@@ -216,14 +216,18 @@ That test is what splits the two installation methods, and the split is a proper
|
||||
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.
|
||||
The first mitigation was documentation: the `setup` command's help text stated that `setup hooks` must be re-run after an upgrade.
|
||||
The failure it guarded 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.
|
||||
**Task 0043 superseded that mitigation and this section's conclusion.**
|
||||
The preference for the bare name is reachable through a wrapper-based install after all; it was only being reached for from the wrong end.
|
||||
`setup hooks` now resolves `gitea-axi` on `PATH` the way a shell does and hands *that* location to the SDK, so the realpath comparison succeeds and the bare name is recorded — under Nix as under npm.
|
||||
The absolute path survives as the fallback for a binary that is not on `PATH` at all.
|
||||
The reasoning is recorded in [ADR 0019](../adr/0019-hook-records-search-path-name.md); the help text's re-run instruction is gone, having become false.
|
||||
|
||||
The related defect on the same line went with it: `isManagedHook` recognises its own hook by testing whether the recorded command string *contains* the marker `gitea-axi`, so an entrypoint path lacking that substring made `setup hooks` append a duplicate rather than update in place.
|
||||
`setup hooks` now prunes duplicates itself, recognising its entry by the exact command it records rather than by a substring of it.
|
||||
That coupling was why `package.nix` renamed its build tree in `postUnpack`; the rename is deleted, and the build running green from `/build/source` is what demonstrates the coupling is gone.
|
||||
|
||||
### Verified during design
|
||||
|
||||
|
||||
@@ -26,9 +26,43 @@ Once the coupling is gone the rename has no remaining purpose and goes with it.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- [ ] The recorded hook command is the bare binary name whenever that name resolves to the running program on `PATH`.
|
||||
- [ ] The recorded hook command remains the absolute entrypoint path when the binary is not resolvable on `PATH`, and that fallback is exercised by a test.
|
||||
- [ ] Re-running `setup hooks` updates the existing entry in place rather than appending a second one, including when the entrypoint path does not contain the marker.
|
||||
- [ ] The `setup` help text no longer instructs the user to re-run hooks after an upgrade, that instruction having become false.
|
||||
- [ ] The derivation no longer renames its build tree, and the build still passes with the tree at a path that does not contain the marker.
|
||||
- [ ] The behaviour is verified against a real wrapper-based install, not only against a source checkout.
|
||||
- [x] The recorded hook command is the bare binary name whenever that name resolves to the running program on `PATH`.
|
||||
- [x] The recorded hook command remains the absolute entrypoint path when the binary is not resolvable on `PATH`, and that fallback is exercised by a test.
|
||||
- [x] Re-running `setup hooks` updates the existing entry in place rather than appending a second one, including when the entrypoint path does not contain the marker.
|
||||
- [x] The `setup` help text no longer instructs the user to re-run hooks after an upgrade, that instruction having become false.
|
||||
- [x] The derivation no longer renames its build tree, and the build still passes with the tree at a path that does not contain the marker.
|
||||
- [x] The behaviour is verified against a real wrapper-based install, not only against a source checkout.
|
||||
|
||||
## Implementation Notes
|
||||
|
||||
The decision is recorded as [ADR 0019](../adr/0019-hook-records-search-path-name.md).
|
||||
ADR 0009's addendum claimed the SDK registers the bare binary as the hook command, which held only for npm; it is amended in place.
|
||||
The spec's "Resolved verification item" section, which concluded the bare name was unreachable through a wrapper, is rewritten to record that task 0043 superseded it.
|
||||
|
||||
### Resolving the name had to be stricter than first written
|
||||
|
||||
The first cut accepted any executable file named `gitea-axi` on `PATH` and handed it to the SDK.
|
||||
That satisfied the letter of the change — the SDK's realpath test passed and the bare name got recorded — but only because the path handed over trivially matched itself, which made the SDK's check a tautology rather than a use of it.
|
||||
Criterion 1 asks for the name to resolve *to the running program*, and that version would have recorded a bare name for a different `gitea-axi` shadowing this one on `PATH`.
|
||||
|
||||
`resolveEntrypointOnPath` therefore requires the candidate to be either a symlink whose realpath is the entrypoint (npm's shape) or a wrapper that names the entrypoint in its text (the generated shape).
|
||||
Driving the real Nix binary showed the wrapper case is two hops, not one: `bin/gitea-axi` sets `PATH` and execs `bin/.gitea-axi-wrapped`, and only that second script names the entrypoint.
|
||||
Containment follows the chain, bounded by hop, file-count and file-size caps so a dense chain cannot run away, and falls back to the absolute path wherever it cannot reach the entrypoint.
|
||||
|
||||
### Recognising the tool's own hook
|
||||
|
||||
The SDK's `isManagedHook` is a substring test against the recorded command and is not ours to change, so `setup hooks` prunes duplicates itself after the SDK writes.
|
||||
An early version's predicate was `recorded === command || recorded.includes("gitea-axi")`, which reintroduced the very coupling this task removes and could have deleted an unrelated tool's hook whose command merely mentioned `gitea-axi`.
|
||||
It is now exact-equality only.
|
||||
That is sufficient: a *re-run* records an identical command, and the upgrade case is handled by the bare name being stable in the first place.
|
||||
|
||||
Duplicates are pruned only from `~/.claude/settings.json` and `~/.codex/hooks.json`.
|
||||
The third integration, OpenCode, is a plugin file the SDK rewrites wholesale behind its own managed marker, so it cannot accumulate duplicates.
|
||||
|
||||
### Verification
|
||||
|
||||
Criterion 6 was met by driving the built Nix binary rather than by a test, since no test tier installs a wrapper.
|
||||
Against `result/bin/gitea-axi`: on `PATH` records `gitea-axi`; off `PATH` records the store entrypoint path; a same-named impostor on `PATH` falls back rather than recording the name; and re-running in both the on-`PATH` and fallback cases leaves exactly one entry.
|
||||
A globally `npm install`-ed pack of the same tree records `gitea-axi` through its symlinked `bin`, confirming the npm shape still resolves.
|
||||
|
||||
Criterion 5 is what `nix build` now demonstrates: with `postUnpack` deleted the fast tier runs from `/build/source`, a path with no marker in it, and the re-run idempotency test passes there — which it could not before the pruning change.
|
||||
|
||||
Reference in New Issue
Block a user