From 27aad04984f6e1bbbd9a000515e7a747a52af28f Mon Sep 17 00:00:00 2001 From: alexion Date: Mon, 20 Jul 2026 13:12:40 -0400 Subject: [PATCH] fix(setup): record the session-start hook as a search-path name (task 0043) 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. --- .../0009-setup-command-over-postinstall.md | 4 + .../adr/0019-hook-records-search-path-name.md | 47 ++++ .claude/spec/nix-flake-packaging.md | 18 +- .../0043-hook-records-bare-binary-name.md | 46 ++- CLAUDE.md | 15 +- package.nix | 19 -- src/commands/setup.ts | 71 ++++- src/hooks.ts | 217 +++++++++++++++ test/hooks.test.ts | 263 ++++++++++++++++++ test/setup.test.ts | 99 ++++++- 10 files changed, 750 insertions(+), 49 deletions(-) create mode 100644 .claude/adr/0019-hook-records-search-path-name.md create mode 100644 src/hooks.ts create mode 100644 test/hooks.test.ts diff --git a/.claude/adr/0009-setup-command-over-postinstall.md b/.claude/adr/0009-setup-command-over-postinstall.md index cad8c82..02f7265 100644 --- a/.claude/adr/0009-setup-command-over-postinstall.md +++ b/.claude/adr/0009-setup-command-over-postinstall.md @@ -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. diff --git a/.claude/adr/0019-hook-records-search-path-name.md b/.claude/adr/0019-hook-records-search-path-name.md new file mode 100644 index 0000000..919bbc4 --- /dev/null +++ b/.claude/adr/0019-hook-records-search-path-name.md @@ -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. diff --git a/.claude/spec/nix-flake-packaging.md b/.claude/spec/nix-flake-packaging.md index f764f6d..b3c72f8 100644 --- a/.claude/spec/nix-flake-packaging.md +++ b/.claude/spec/nix-flake-packaging.md @@ -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 diff --git a/.claude/tasks/0043-hook-records-bare-binary-name.md b/.claude/tasks/0043-hook-records-bare-binary-name.md index a97f300..7717356 100644 --- a/.claude/tasks/0043-hook-records-bare-binary-name.md +++ b/.claude/tasks/0043-hook-records-bare-binary-name.md @@ -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. diff --git a/CLAUDE.md b/CLAUDE.md index 14b4ed1..6e5d617 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -72,8 +72,13 @@ 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. +`resolvePortableHookCommand` in `axi-sdk-js` returns the bare binary name only when a `PATH` entry *realpath-matches* the **`execPath` it is handed**, and the absolute path otherwise. +Passing `binaryNames` does not by itself make the hook portable: npm symlinks its `bin` entry straight at `dist/main.js` so the match succeeds there, but a wrapper-based install (Nix, a shim, a generated `.cmd`) never can, because a script that *invokes* a file does not resolve *to* it. +Per [ADR 0019](.claude/adr/0019-hook-records-search-path-name.md), `setup hooks` therefore resolves `gitea-axi` on `PATH` itself and hands *that* location over, which makes the match succeed and the bare name get recorded. +It only accepts a `PATH` entry that resolves to the running entrypoint — a symlink to it, or a wrapper naming it in its text — so a same-named binary that is some other program falls through to the absolute-path fallback, as does a name that is not on `PATH` at all. +It reads `PATH` from `process.env`, not from the injected `deps.env`, because it has to agree with the SDK's own probing — so a test that wants to steer this has to set `process.env.PATH`. +Verify hook behaviour by driving the installed binary and reading `~/.claude/settings.json`, not by reading the SDK's interface; a hook whose recorded path no longer exists does not run and does not warn. + +The SDK's `isManagedHook` recognises its own hook by testing whether the recorded command *contains* the marker, so it appends a duplicate instead of updating in place whenever the recorded command lacks the substring `gitea-axi`. +`setup hooks` compensates by pruning duplicates itself after the SDK writes. +This is why `package.nix` no longer needs to rename its build tree: the fast tier runs from `/build/source`, whose path has no marker in it, and the idempotency test passes there. diff --git a/package.nix b/package.nix index 9c74e1c..fe5fce8 100644 --- a/package.nix +++ b/package.nix @@ -60,25 +60,6 @@ buildNpmPackage { nativeBuildInputs = [ makeWrapper ]; - # The builder would otherwise unpack to a generic `source` directory, which no - # real installation resembles: under npm the tree lives at - # `node_modules/gitea-axi`, under Nix at `…-gitea-axi-/…`. The fast - # tier's `setup hooks` test is sensitive to the difference, because the SDK - # records the entrypoint's absolute path and recognises its own managed hook by - # 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. 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 - ''; - # The fast tier only. The live end-to-end and benchmark smoke tiers need a # live Gitea host. Two of these test files invoke `git` directly and one # resolves it with `which`; `tea` is already stubbed within this tier. diff --git a/src/commands/setup.ts b/src/commands/setup.ts index 3c258dd..61c8a8f 100644 --- a/src/commands/setup.ts +++ b/src/commands/setup.ts @@ -5,6 +5,7 @@ import { fileURLToPath } from "node:url"; import { installSessionStartHooks } from "axi-sdk-js"; import type { CliDeps } from "../deps.js"; import { axiError } from "../errors.js"; +import { pruneDuplicateManagedHooks, resolveEntrypointOnPath } from "../hooks.js"; import { renderDetail } from "../render.js"; export const SETUP_HELP = `usage: gitea-axi setup [hooks] @@ -18,14 +19,6 @@ 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 `; @@ -38,6 +31,11 @@ const SKILL_NAME = "gitea-axi"; const SKILL_SOURCE = new URL("../../skills/gitea-axi/SKILL.md", import.meta.url); const EXEC_PATH = fileURLToPath(new URL("../main.js", import.meta.url)); +// The same string as SKILL_NAME, kept apart because it names a different thing: +// the executable as it is spelled on PATH, which is what the SessionStart hook +// records. They are free to diverge; the SDK's marker follows the skill. +const BINARY_NAME = "gitea-axi"; + const HOOK_INTEGRATIONS = ["Claude Code", "Codex", "OpenCode"]; /** The home directory, from the injected env first so tests can point at a temp HOME. */ @@ -93,13 +91,66 @@ async function setupSkill(deps: CliDeps): Promise { }); } +// The two files the SDK writes the SessionStart hook array into. Its third +// integration, OpenCode, is a whole plugin file it rewrites wholesale behind +// its own managed marker, so that one cannot accumulate duplicates. +const HOOK_SETTINGS_FILES = [ + [".claude", "settings.json"], + [".codex", "hooks.json"], +]; + +/** + * Collapse any duplicate managed entry the SDK's own recognition missed. + * + * It identifies its hook by finding the marker inside the recorded command, so + * an entrypoint path that does not happen to contain "gitea-axi" makes a re-run + * append a second entry rather than update the first. Matching the exact + * command this run records makes idempotency independent of the recorded + * command's shape — and cannot mistake another tool's hook for ours the way a + * substring test can. + */ +function pruneHookSettingsFiles(home: string, command: string, errors: string[]): void { + const isManaged = (recorded: string) => recorded === command; + + for (const segments of HOOK_SETTINGS_FILES) { + const target = join(home, ...segments); + if (!existsSync(target)) { + continue; + } + try { + const current = JSON.parse(readFileSync(target, "utf8")); + const { settings, changed } = pruneDuplicateManagedHooks(current, isManaged); + if (changed) { + writeFileSync(target, `${JSON.stringify(settings, null, 2)}\n`, "utf8"); + } + } catch (error) { + errors.push(`${target}: ${error instanceof Error ? error.message : String(error)}`); + } + } +} + async function setupHooks(deps: CliDeps): Promise { const home = resolveHome(deps); const errors: string[] = []; + + // ADR 0019: record a search-path name, not an install-tree path. Handing the + // SDK where the binary resolves on PATH — rather than the module-relative + // entrypoint — is what lets its realpath test succeed for a wrapper-based + // install, so it records the bare, upgrade-stable name. When the name + // resolves to no wrapper or symlink of ours, the entrypoint stands as the + // fallback and the absolute path is recorded exactly as before. + // + // PATH is read from the process rather than the injected environment on + // purpose: it has to be the same PATH the SDK itself probes, and the SDK + // reads its own. + const onPath = resolveEntrypointOnPath(BINARY_NAME, EXEC_PATH, process.env.PATH); + const execPath = onPath ?? EXEC_PATH; + const command = onPath ? BINARY_NAME : EXEC_PATH; + installSessionStartHooks({ marker: SKILL_NAME, binaryNames: [SKILL_NAME], - execPath: EXEC_PATH, + execPath, homeDir: home, // This is an explicit user command, so install unconditionally rather than // deferring to the SDK's auto-install safety gate (which is tuned for the @@ -108,6 +159,8 @@ async function setupHooks(deps: CliDeps): Promise { onError: (message) => errors.push(message), }); + pruneHookSettingsFiles(home, command, errors); + if (errors.length > 0) { throw axiError(`Failed to install session hooks: ${errors.join("; ")}`, "UNKNOWN"); } diff --git a/src/hooks.ts b/src/hooks.ts new file mode 100644 index 0000000..ebb25a0 --- /dev/null +++ b/src/hooks.ts @@ -0,0 +1,217 @@ +import { readFileSync, realpathSync, statSync } from "node:fs"; +import { delimiter, join } from "node:path"; + +/** Filesystem reads {@link resolveEntrypointOnPath} needs, injectable for tests. */ +export interface PathProbe { + /** The resolved real path of `candidate`, or `undefined` if it is not a file. */ + realPath: (candidate: string) => string | undefined; + /** The contents of `candidate`, or `undefined` if it cannot be read as text. */ + readText: (candidate: string) => string | undefined; +} + +/** + * Where `name` resolves on `pathValue` *to the program running `entrypoint`*, or + * `undefined` when it resolves nowhere, or resolves to some other program. + * + * This exists so `setup hooks` can hand the agent SDK the location the binary + * actually resolves to on `PATH` rather than the module-relative entrypoint. + * The SDK records a bare, upgrade-stable name only when a `PATH` entry + * realpath-matches the path it is given, and from the entrypoint's side that + * can only ever succeed under npm, which symlinks its `bin` entry straight at + * it. A wrapper-based install cannot: a script that *invokes* a file never + * resolves *to* that file. + * + * The two install shapes are therefore recognised on their own terms: + * + * - a **symlink** to the entrypoint, matched by realpath — npm's shape; + * - a **generated wrapper** naming the entrypoint in its text, matched by + * containment — the shape Nix, shims and `.cmd` launchers all produce. + * + * Wrappers chain, so containment follows the references a wrapper makes to + * other files rather than only reading the first one. A Nix install is two + * hops: `bin/gitea-axi` sets `PATH` and execs `bin/.gitea-axi-wrapped`, which + * is what actually names the entrypoint. + * + * Requiring one of these is what keeps the answer honest. Accepting any file + * that merely *bears the name* would hand the SDK a path that trivially + * realpath-matches itself, turning its check into a tautology and recording a + * bare name that resolves to a different program than the one asked. + * + * Windows `PATHEXT` suffixes are deliberately not tried. The package supports + * Linux and macOS, and wherever the lookup misses — an unreadable wrapper, a + * compiled launcher, a chain deeper than {@link MAX_WRAPPER_HOPS} — the + * caller's absolute-path fallback still produces a working hook. + */ +export function resolveEntrypointOnPath( + name: string, + entrypoint: string, + pathValue: string | undefined, + probe: PathProbe = defaultPathProbe, +): string | undefined { + const entrypointReal = probe.realPath(entrypoint); + + for (const dir of (pathValue ?? "").split(delimiter)) { + if (!dir) { + continue; + } + const candidate = join(dir, name); + const candidateReal = probe.realPath(candidate); + if (!candidateReal) { + continue; + } + if (entrypointReal !== undefined && candidateReal === entrypointReal) { + return candidate; + } + if (wrapperLeadsTo(candidate, entrypoint, probe)) { + return candidate; + } + } + return undefined; +} + +/** How many wrapper-to-wrapper hops to follow. Nix needs two; the cap is slack. */ +const MAX_WRAPPER_HOPS = 4; + +/** How many files one lookup may read before giving up, so a dense chain cannot run away. */ +const MAX_WRAPPER_FILES = 32; + +/** Absolute paths appearing in a script's text, stopping at shell quoting and separators. */ +function absolutePathsIn(text: string): string[] { + return text.match(/\/[^\s"';|&()]+/g) ?? []; +} + +/** Whether `start`, followed through the files it names, ends up naming `entrypoint`. */ +function wrapperLeadsTo(start: string, entrypoint: string, probe: PathProbe): boolean { + const seen = new Set(); + let frontier = [start]; + + for (let hop = 0; hop <= MAX_WRAPPER_HOPS && frontier.length > 0; hop++) { + const next: string[] = []; + for (const file of frontier) { + if (seen.has(file) || seen.size >= MAX_WRAPPER_FILES) { + continue; + } + seen.add(file); + + const text = probe.readText(file); + if (text === undefined) { + continue; + } + if (text.includes(entrypoint)) { + return true; + } + for (const referenced of absolutePathsIn(text)) { + if (!seen.has(referenced) && probe.realPath(referenced)) { + next.push(referenced); + } + } + } + frontier = next; + } + return false; +} + +/** + * Anything much larger than a wrapper script is not one. The cap keeps a chain + * that happens to name `node` or `bash` from reading whole binaries back. + */ +const MAX_WRAPPER_BYTES = 64 * 1024; + +const defaultPathProbe: PathProbe = { + realPath: (candidate) => { + try { + return statSync(candidate).isFile() ? realpathSync(candidate) : undefined; + } catch { + return undefined; + } + }, + readText: (candidate) => { + try { + if (statSync(candidate).size > MAX_WRAPPER_BYTES) { + return undefined; + } + return readFileSync(candidate, "utf8"); + } catch { + return undefined; + } + }, +}; + +interface HookEntry { + command?: unknown; +} + +interface HookGroup { + hooks?: unknown; +} + +interface HookSettings { + hooks?: { SessionStart?: unknown }; +} + +/** + * Collapse repeated managed SessionStart entries down to the last one, which is + * the entry the SDK has just written or refreshed. + * + * The SDK recognises its own hook by testing whether the recorded command + * *contains* the marker, so an entrypoint path that happens not to contain + * "gitea-axi" makes a re-run append a second entry instead of updating the + * first — contradicting the idempotency `setup` promises. Recognition here does + * not depend on the command's shape: callers pass an `isManaged` that matches + * the exact command this run records, so a re-run identifies its own previous + * entry by equality rather than by a substring accident — and a hook belonging + * to another tool is never a candidate, whatever its command happens to spell. + * + * The *last* match survives. Every match holds the identical command, so the + * choice can only affect `type` and `timeout`, and the last is the entry the + * SDK has just appended in the case this exists to repair. + * + * Groups left with no hooks are dropped rather than kept as empty objects. + */ +export function pruneDuplicateManagedHooks( + settings: unknown, + isManaged: (command: string) => boolean, +): { settings: unknown; changed: boolean } { + const pruned = structuredClone(settings) as HookSettings | null; + const groups = pruned?.hooks?.SessionStart; + if (!Array.isArray(groups)) { + return { settings, changed: false }; + } + + const isManagedHook = (hook: HookEntry) => + typeof hook?.command === "string" && isManaged(hook.command); + + const managedCount = (groups as HookGroup[]).reduce( + (total, group) => + total + + (Array.isArray(group?.hooks) ? (group.hooks as HookEntry[]).filter(isManagedHook).length : 0), + 0, + ); + if (managedCount < 2) { + return { settings, changed: false }; + } + + // Every managed entry but the last is a leftover from an earlier run. + let remaining = managedCount - 1; + const kept: HookGroup[] = []; + for (const group of groups as HookGroup[]) { + if (!Array.isArray(group?.hooks)) { + kept.push(group); + continue; + } + const survivors = (group.hooks as HookEntry[]).filter((hook) => { + if (remaining > 0 && isManagedHook(hook)) { + remaining--; + return false; + } + return true; + }); + if (survivors.length > 0) { + group.hooks = survivors; + kept.push(group); + } + } + (pruned as HookSettings).hooks = { ...pruned?.hooks, SessionStart: kept }; + + return { settings: pruned, changed: true }; +} diff --git a/test/hooks.test.ts b/test/hooks.test.ts new file mode 100644 index 0000000..3a52e4e --- /dev/null +++ b/test/hooks.test.ts @@ -0,0 +1,263 @@ +import { delimiter } from "node:path"; +import { describe, expect, it } from "vitest"; +import { + type PathProbe, + pruneDuplicateManagedHooks, + resolveEntrypointOnPath, +} from "../src/hooks.js"; + +describe("resolveEntrypointOnPath", () => { + const ENTRYPOINT = "/opt/gitea-axi/dist/main.js"; + + /** A probe over a fake filesystem: real paths, plus text for wrapper scripts. */ + const probeOver = ( + realPaths: Record, + texts: Record = {}, + ): PathProbe => ({ + realPath: (candidate) => realPaths[candidate], + readText: (candidate) => texts[candidate], + }); + + const path = ["/empty", "/usr/local/bin", "/usr/bin"].join(delimiter); + + it("matches a symlink to the entrypoint by real path — npm's install shape", () => { + const probe = probeOver({ + [ENTRYPOINT]: ENTRYPOINT, + "/usr/local/bin/gitea-axi": ENTRYPOINT, + }); + + expect(resolveEntrypointOnPath("gitea-axi", ENTRYPOINT, path, probe)).toBe( + "/usr/local/bin/gitea-axi", + ); + }); + + it("matches a wrapper naming the entrypoint in its text — the Nix install shape", () => { + // The wrapper's own realpath is the wrapper, never the entrypoint, which is + // precisely why the realpath test alone cannot see this install. + const probe = probeOver( + { + [ENTRYPOINT]: ENTRYPOINT, + "/usr/local/bin/gitea-axi": "/usr/local/bin/gitea-axi", + }, + { "/usr/local/bin/gitea-axi": `#!/bin/sh\nexec node ${ENTRYPOINT} "$@"\n` }, + ); + + expect(resolveEntrypointOnPath("gitea-axi", ENTRYPOINT, path, probe)).toBe( + "/usr/local/bin/gitea-axi", + ); + }); + + it("follows a chained wrapper to the entrypoint — the real Nix shape", () => { + // Nix is two hops: bin/gitea-axi sets PATH and execs bin/.gitea-axi-wrapped, + // and only that second script names the entrypoint. + const wrapped = "/usr/local/bin/.gitea-axi-wrapped"; + const probe = probeOver( + { + [ENTRYPOINT]: ENTRYPOINT, + "/usr/local/bin/gitea-axi": "/usr/local/bin/gitea-axi", + [wrapped]: wrapped, + }, + { + "/usr/local/bin/gitea-axi": `#!/bin/bash -e\nexport PATH\nexec -a "$0" "${wrapped}" "$@"\n`, + [wrapped]: `#!/bin/bash -e\nexec "/usr/bin/node" ${ENTRYPOINT} "$@"\n`, + }, + ); + + expect(resolveEntrypointOnPath("gitea-axi", ENTRYPOINT, path, probe)).toBe( + "/usr/local/bin/gitea-axi", + ); + }); + + it("gives up on a wrapper chain that never names the entrypoint", () => { + // A cycle: it must terminate rather than following the loop forever. + const other = "/usr/local/bin/other"; + const probe = probeOver( + { + [ENTRYPOINT]: ENTRYPOINT, + "/usr/local/bin/gitea-axi": "/usr/local/bin/gitea-axi", + [other]: other, + }, + { + "/usr/local/bin/gitea-axi": `exec "${other}"\n`, + [other]: `exec "/usr/local/bin/gitea-axi"\n`, + }, + ); + + expect(resolveEntrypointOnPath("gitea-axi", ENTRYPOINT, path, probe)).toBeUndefined(); + }); + + it("refuses a same-named binary that is some other program", () => { + // Accepting this would hand the SDK a path that realpath-matches itself, + // making its check a tautology and recording a name for a program the + // caller never asked about. + const probe = probeOver( + { + [ENTRYPOINT]: ENTRYPOINT, + "/usr/local/bin/gitea-axi": "/somewhere/else/gitea-axi", + }, + { "/usr/local/bin/gitea-axi": "#!/bin/sh\nexec node /somewhere/else/dist/main.js\n" }, + ); + + expect(resolveEntrypointOnPath("gitea-axi", ENTRYPOINT, path, probe)).toBeUndefined(); + }); + + it("returns undefined when the name resolves nowhere on PATH", () => { + const probe = probeOver({ [ENTRYPOINT]: ENTRYPOINT }); + + expect(resolveEntrypointOnPath("gitea-axi", ENTRYPOINT, path, probe)).toBeUndefined(); + }); + + it("returns undefined for an unset or empty PATH", () => { + const probe = probeOver({ [ENTRYPOINT]: ENTRYPOINT }); + + expect(resolveEntrypointOnPath("gitea-axi", ENTRYPOINT, undefined, probe)).toBeUndefined(); + expect(resolveEntrypointOnPath("gitea-axi", ENTRYPOINT, "", probe)).toBeUndefined(); + }); + + it("skips empty PATH entries rather than probing the working directory", () => { + const probed: string[] = []; + resolveEntrypointOnPath("gitea-axi", ENTRYPOINT, ["", "/usr/bin", ""].join(delimiter), { + realPath: (candidate) => { + probed.push(candidate); + return undefined; + }, + readText: () => undefined, + }); + + expect(probed).toEqual([ENTRYPOINT, "/usr/bin/gitea-axi"]); + }); + + it("takes the first PATH entry that matches, not a later one", () => { + const probe = probeOver({ + [ENTRYPOINT]: ENTRYPOINT, + "/usr/local/bin/gitea-axi": ENTRYPOINT, + "/usr/bin/gitea-axi": ENTRYPOINT, + }); + + expect(resolveEntrypointOnPath("gitea-axi", ENTRYPOINT, path, probe)).toBe( + "/usr/local/bin/gitea-axi", + ); + }); +}); + +describe("pruneDuplicateManagedHooks", () => { + const settingsWith = (...commands: string[]) => ({ + hooks: { + SessionStart: commands.map((command) => ({ + matcher: "", + hooks: [{ type: "command", command, timeout: 10 }], + })), + }, + }); + + interface ReadBack { + hooks: { SessionStart: { hooks: { command: string; timeout?: number }[] }[] }; + } + + const commandsOf = (settings: unknown) => + (settings as ReadBack).hooks.SessionStart.flatMap((group) => + group.hooks.map((hook) => hook.command), + ); + + it("collapses a duplicated entry whose command does not contain the marker", () => { + // The case the SDK cannot handle: it recognises its own hook by finding the + // marker inside the recorded command, so an entrypoint path without + // "gitea-axi" in it makes a re-run append rather than update. + const entrypoint = "/build/source/dist/main.js"; + const result = pruneDuplicateManagedHooks( + settingsWith(entrypoint, entrypoint), + (command) => command === entrypoint, + ); + + expect(result.changed).toBe(true); + expect(commandsOf(result.settings)).toEqual([entrypoint]); + }); + + it("keeps the last managed entry, which is the one the SDK just appended", () => { + // Both entries carry the same command, so the survivor is identified by the + // rest of its shape: the stale one has a timeout the SDK no longer writes. + const result = pruneDuplicateManagedHooks( + { + hooks: { + SessionStart: [ + { matcher: "", hooks: [{ type: "command", command: "gitea-axi", timeout: 99 }] }, + { matcher: "", hooks: [{ type: "command", command: "gitea-axi", timeout: 10 }] }, + ], + }, + }, + (command) => command === "gitea-axi", + ); + + const settings = result.settings as ReadBack; + expect(settings.hooks.SessionStart).toHaveLength(1); + expect(settings.hooks.SessionStart[0]?.hooks[0]).toMatchObject({ timeout: 10 }); + }); + + it("leaves hooks belonging to other tools untouched", () => { + const result = pruneDuplicateManagedHooks( + settingsWith("other-tool", "gitea-axi", "another-tool", "gitea-axi"), + (command) => command === "gitea-axi", + ); + + expect(commandsOf(result.settings)).toEqual(["other-tool", "another-tool", "gitea-axi"]); + }); + + it("prunes duplicates that share a single group", () => { + const result = pruneDuplicateManagedHooks( + { + hooks: { + SessionStart: [ + { + matcher: "", + hooks: [ + { type: "command", command: "gitea-axi", timeout: 10 }, + { type: "command", command: "other-tool", timeout: 10 }, + { type: "command", command: "gitea-axi", timeout: 10 }, + ], + }, + ], + }, + }, + (command) => command === "gitea-axi", + ); + + expect(commandsOf(result.settings)).toEqual(["other-tool", "gitea-axi"]); + }); + + it("reports no change and returns the input when there is nothing to prune", () => { + const single = settingsWith("gitea-axi"); + const result = pruneDuplicateManagedHooks(single, (command) => command === "gitea-axi"); + + expect(result.changed).toBe(false); + expect(result.settings).toBe(single); + }); + + it("preserves sibling settings keys and other hook events", () => { + const result = pruneDuplicateManagedHooks( + { + model: "opus", + hooks: { + PreToolUse: [{ matcher: "Bash", hooks: [{ type: "command", command: "audit" }] }], + SessionStart: settingsWith("gitea-axi", "gitea-axi").hooks.SessionStart, + }, + }, + (command) => command === "gitea-axi", + ); + + const settings = result.settings as ReadBack & { + model: string; + hooks: { PreToolUse: unknown[] }; + }; + expect(settings.model).toBe("opus"); + expect(settings.hooks.PreToolUse).toHaveLength(1); + expect(commandsOf(settings)).toEqual(["gitea-axi"]); + }); + + it("tolerates settings with no SessionStart hooks at all", () => { + for (const input of [{}, { hooks: {} }, { hooks: { SessionStart: "nonsense" } }, null]) { + const result = pruneDuplicateManagedHooks(input, () => true); + expect(result.changed).toBe(false); + expect(result.settings).toBe(input); + } + }); +}); diff --git a/test/setup.test.ts b/test/setup.test.ts index a4c7dad..1edccef 100644 --- a/test/setup.test.ts +++ b/test/setup.test.ts @@ -1,8 +1,9 @@ -import { existsSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { delimiter, isAbsolute, join } from "node:path"; +import { fileURLToPath } from "node:url"; import { afterEach, describe, expect, it } from "vitest"; -import { runCliTest } from "./harness.js"; +import { type CliResult, runCliTest } from "./harness.js"; let tempHome: string; @@ -12,6 +13,46 @@ afterEach(() => { } }); +/** + * Run `body` with `process.env.PATH` replaced. `setup hooks` reads PATH from the + * process rather than the injected environment, because it has to agree with + * the agent SDK's own probing — so this is the seam that decides whether the + * recorded hook command is the bare name or the absolute entrypoint path. + */ +async function withPath(path: string, body: () => Promise): Promise { + const original = process.env.PATH; + process.env.PATH = path; + try { + return await body(); + } finally { + process.env.PATH = original; + } +} + +/** + * The entrypoint `setup hooks` resolves for itself — `src/main.js` here, since + * the dist layout mirrors src/ and setup.ts locates it relative to its own + * module. + */ +function entrypointPath(): string { + return fileURLToPath(new URL("../src/main.js", import.meta.url)); +} + +/** Write an executable `gitea-axi` into a fresh `dir`, and return that dir. */ +function writeFakeBinary(dir: string, contents: string): string { + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, "gitea-axi"), contents, { mode: 0o755 }); + return dir; +} + +/** The single command string recorded in the Claude Code SessionStart hook. */ +function recordedHookCommand(home: string): string { + const settings = JSON.parse(readFileSync(join(home, ".claude", "settings.json"), "utf8")); + expect(settings.hooks.SessionStart).toHaveLength(1); + expect(settings.hooks.SessionStart[0].hooks).toHaveLength(1); + return settings.hooks.SessionStart[0].hooks[0].command; +} + describe("setup", () => { it("installs the skill and is idempotent: installed -> unchanged -> updated", async () => { tempHome = mkdtempSync(join(tmpdir(), "gitea-axi-setup-")); @@ -86,6 +127,58 @@ describe("setup hooks", () => { expect(claudeSettings.hooks.SessionStart).toHaveLength(1); expect(claudeSettings.hooks.SessionStart[0].hooks).toHaveLength(1); }); + + it("records the bare binary name when a wrapper on PATH runs this entrypoint", async () => { + tempHome = mkdtempSync(join(tmpdir(), "gitea-axi-setup-")); + + // A wrapper-based install in miniature — a script that *invokes* the + // entrypoint, so its realpath is itself and the SDK could never match it + // from the entrypoint's side. This is the shape Nix installs. + const binDir = writeFakeBinary( + join(tempHome, "wrapper"), + `#!/bin/sh\nexec node ${entrypointPath()} "$@"\n`, + ); + + const { exitCode } = await withPath(`${binDir}${delimiter}${process.env.PATH ?? ""}`, () => + runCliTest(["setup", "hooks"], { env: { HOME: tempHome } }), + ); + expect(exitCode).toBe(0); + + expect(recordedHookCommand(tempHome)).toBe("gitea-axi"); + }); + + it("falls back to the absolute entrypoint path when gitea-axi is not on PATH", async () => { + tempHome = mkdtempSync(join(tmpdir(), "gitea-axi-setup-")); + const emptyDir = join(tempHome, "empty"); + mkdirSync(emptyDir, { recursive: true }); + + const { exitCode } = await withPath(emptyDir, () => + runCliTest(["setup", "hooks"], { env: { HOME: tempHome } }), + ); + expect(exitCode).toBe(0); + + const command = recordedHookCommand(tempHome); + expect(isAbsolute(command)).toBe(true); + expect(command).toBe(entrypointPath()); + }); + + it("falls back rather than recording a name for some unrelated gitea-axi", async () => { + tempHome = mkdtempSync(join(tmpdir(), "gitea-axi-setup-")); + + // Same name on PATH, different program. Recording the bare name here would + // point the hook at a binary the user never asked to install. + const binDir = writeFakeBinary( + join(tempHome, "impostor"), + "#!/bin/sh\nexec node /somewhere/else/dist/main.js \"$@\"\n", + ); + + const { exitCode } = await withPath(binDir, () => + runCliTest(["setup", "hooks"], { env: { HOME: tempHome } }), + ); + expect(exitCode).toBe(0); + + expect(recordedHookCommand(tempHome)).toBe(entrypointPath()); + }); }); describe("setup dispatch", () => {