diff --git a/.claude/tasks/0037-flake-package-and-wrapper.md b/.claude/tasks/0037-flake-package-and-wrapper.md index f81ab48..563aeef 100644 --- a/.claude/tasks/0037-flake-package-and-wrapper.md +++ b/.claude/tasks/0037-flake-package-and-wrapper.md @@ -30,14 +30,75 @@ The allowlist's failure mode is loud but disconnected from its cause, so this sl ## Acceptance criteria -- [ ] Building the flake's package from a clean checkout produces a runnable `gitea-axi` that prints help and reports its version. -- [ ] The store path's version matches the package manifest's version, with the version appearing in no Nix expression. -- [ ] Changing a dependency in the lockfile requires no edit to any Nix expression. -- [ ] The derivation is a separate callable expression that the flake file consumes; it evaluates outside a flake context. -- [ ] The package builds for the four supported Linux and Darwin systems, with no third-party flake input beyond nixpkgs. -- [ ] Touching a file outside the source allowlist — an ADR, a spec, a benchmark file, prose documentation — does not change the derivation's output path. -- [ ] The wrapped binary finds `git` and `tea` on a machine where neither is otherwise installed. -- [ ] With the operator's own `git` and `tea` on the search path, those are the ones the binary invokes. -- [ ] The fast test tier runs and passes inside the build; a deliberately failing test fails the build. -- [ ] ADR 0018 is committed as part of this slice. -- [ ] The agent instructions carry a Gotcha about extending the source allowlist for new build-relevant files. +- [x] Building the flake's package from a clean checkout produces a runnable `gitea-axi` that prints help and reports its version. +- [x] The store path's version matches the package manifest's version, with the version appearing in no Nix expression. +- [x] Changing a dependency in the lockfile requires no edit to any Nix expression. +- [x] The derivation is a separate callable expression that the flake file consumes; it evaluates outside a flake context. +- [-] The package builds for the four supported Linux and Darwin systems, with no third-party flake input beyond nixpkgs. +- [x] Touching a file outside the source allowlist — an ADR, a spec, a benchmark file, prose documentation — does not change the derivation's output path. +- [x] The wrapped binary finds `git` and `tea` on a machine where neither is otherwise installed. +- [x] With the operator's own `git` and `tea` on the search path, those are the ones the binary invokes. +- [x] The fast test tier runs and passes inside the build; a deliberately failing test fails the build. +- [x] ADR 0018 is committed as part of this slice. +- [x] The agent instructions carry a Gotcha about extending the source allowlist for new build-relevant files. + +## Implementation Notes + +### Three systems, not four — `x86_64-darwin` is gone + +The one dropped criterion. nixpkgs 26.11, which `nixos-unstable` now points at, has removed `x86_64-darwin` support outright. +`legacyPackages.x86_64-darwin` *throws* on evaluation rather than merely failing to build, so enumerating it would break `nix flake show` and `nix flake check` for **every** system at once, not just that one. +The flake therefore covers `x86_64-linux`, `aarch64-linux`, and `aarch64-darwin`, with the omission commented at the `systems` list. +Intel macOS would need the 26.05 branch; nobody is asking for it. +The rest of the criterion holds: nixpkgs is the only flake input. + +### `doCheck = true` was silently doing nothing + +`buildNpmPackage` wires config, build, and install hooks but supplies **no check hook**, so `doCheck` alone is inert. +The first green build logged `no Makefile or custom checkPhase, doing nothing` and produced a package whose tests had never run — a passing build that verified nothing. +The fix is an explicit `checkPhase`, plus `git` and `which` in `nativeCheckInputs` and a writable `HOME`. +Recorded as a Gotcha, since the failure mode is a *green* build. + +The second half of that criterion was demonstrated unintentionally but genuinely: with the check phase live, a failing test failed the build with exit code 1 before anything was installed. + +### The build tree is named, and why that is a workaround + +`postUnpack` renames the build tree from the builder's generic `source` to `gitea-axi`. + +This is not cosmetic. `test/setup.test.ts` asserts that `setup hooks` updates its managed entry in place rather than appending a duplicate. +The SDK recognises its own hook by testing whether the recorded command *string contains* `"gitea-axi"`, and the recorded command is the entrypoint's absolute path whenever PATH resolution does not match it — which it never does under vitest, since the entrypoint resolves to `src/main.js`, a file that does not exist. +So the assertion holds only when the checkout's path happens to contain `gitea-axi`. + +That was verified rather than assumed: copying the repository to `/tmp/clean-probe-9d3/proj` and running the tier reproduces the failure outside Nix entirely. +A first probe under the session scratchpad passed and was misleading — that path contains `-home-alexion-wrk-gitea-axi-…`, so it satisfied the substring by accident. + +The rename makes the build environment representative of a real installation (`node_modules/gitea-axi/…` under npm, `…-gitea-axi-/…` under Nix) rather than an arrangement no operator ever has. +It is a workaround for a defect, not a property worth keeping, and is commented as such. + +### Follow-up for task 0042 — the open verification item is answered, unfavourably + +The spec's open verification item asked whether the SDK prefers the bare binary name over the absolute entrypoint path. +It does not. +`resolvePortableHookCommand` returns the bare name only when a `PATH` entry realpath-matches the entrypoint, and the absolute path otherwise. + +Driving the Nix-built binary shows what actually lands in `~/.claude/settings.json`: + +``` +/nix/store/nmkzjny0hpzjvyxzdz189whk605di8b6-gitea-axi-0.1.0/lib/node_modules/gitea-axi/dist/main.js +``` + +That is content-addressed: it changes on every rebuild and is eventually garbage-collected, and a `SessionStart` hook that cannot execute simply does not run. +So the dashboard stops appearing after an upgrade, silently and with nothing pointing at the cause — the exact failure the item hoped to rule out, now confirmed under the install method this slice adds. + +Two findings for 0042, both tracing to the same line: + +1. The stale store path above — user-facing breakage, and the more serious of the two. +2. `setup hooks` appends a duplicate entry instead of updating in place whenever the entrypoint path lacks the marker. + +Both were left alone deliberately, at the maintainer's direction, to keep this slice about packaging; fixing hook resolution here would have pre-empted 0042's design work with a decision made in passing. +When 0042 lands, the `postUnpack` rename should be removed with it. + +### ADR 0018 + +Already committed in `0cbfe43` during the planning pass, ahead of this branch, so the criterion is satisfied by an earlier commit rather than by this one. +No change was needed; `package.nix` implements it via `--suffix PATH`. diff --git a/.claude/tasks/0042-verify-hook-path-resolution.md b/.claude/tasks/0042-verify-hook-path-resolution.md index 59246ca..b89d8b6 100644 --- a/.claude/tasks/0042-verify-hook-path-resolution.md +++ b/.claude/tasks/0042-verify-hook-path-resolution.md @@ -23,3 +23,27 @@ Changing the `setup` command to prefer the bare name is explicitly **not** part - [ ] 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. + +## Evidence gathered during task 0037 + +Task 0037 built the flake, which made the SDK's behaviour directly observable. +The answer is the unfavourable one: **the absolute path is recorded**, so the mitigation branch of this task applies, not the close-the-item branch. + +`resolvePortableHookCommand` in `axi-sdk-js` returns the bare binary name only when a `PATH` entry realpath-matches the entrypoint, and the absolute path in every other case. +Driving the Nix-built binary writes this into `~/.claude/settings.json`: + +``` +/nix/store/nmkzjny0hpzjvyxzdz189whk605di8b6-gitea-axi-0.1.0/lib/node_modules/gitea-axi/dist/main.js +``` + +That path is content-addressed, so it changes on every rebuild and is eventually garbage-collected, and the session-start hook then silently stops running. + +A second defect surfaced from the same line. +`isManagedHook` recognises its own hook by testing whether the recorded command *string contains* the marker `"gitea-axi"`, so when the entrypoint path lacks that substring, `setup hooks` appends a duplicate entry instead of updating in place — contradicting the idempotency its help text promises. +This is reproducible outside Nix: copy the checkout to a path containing no `gitea-axi` segment and `test/setup.test.ts` fails. + +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. diff --git a/CLAUDE.md b/CLAUDE.md index 884f5c9..ef6a463 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,7 +14,7 @@ Run `npm run build` before any live `bench:run` if you want `src/` changes refle Prefer this project's own CLI for pull requests — it is the tool being built, so opening its PRs with it is the dogfood path: `npm run build && node dist/main.js pr create --login alexion --base main --head --title --body-file `. -It reuses the `tea` login store, which here holds exactly `alexion` and `csv-reviewer` — there is no `axi` profile. +It reuses the `tea` login store, which here holds exactly `alexion` — there is no `axi` profile, and the `csv-reviewer` profile that used to exist is gone for good. `selectLogin` matches the `--login` value against those names exactly, so `--login alexion` works and an unknown name like `--login axi` fails with `VALIDATION_ERROR` ("Login profile "axi" not found"). Fall back to `tea pr create --login alexion --base main --head ` only for what gitea-axi cannot do yet; `tea pr` still lists PRs until `pr list` lands (task 0008). @@ -34,3 +34,21 @@ The fixture tier is unaffected — it stubs the endpoint — so this bites only That ADR moved *command dispatch* to `gitea-js`; it explicitly kept `tea` for **credential discovery**, and its own Consequences section says so. Per [ADR 0001](.claude/adr/0001-diff-auth-via-tea-login-list.md) as amended, `src/context.ts` resolves auth by shelling out to `tea login list --output json` (discovery) and `tea login helper get --login ` (token, with in-place OAuth refresh). The only bypass is the test hook requiring `GITEA_AXI_API_URL` + `GITEA_AXI_TOKEN` + `GITEA_AXI_REPO` together; there is no user-facing path that avoids `tea`, and `TEA_NOT_INSTALLED` exists for its absence. + +Neither `node`/`npm` nor `tea` is on the `PATH` in a non-interactive shell on this machine, and there is no `~/.gitconfig`. +This is a NixOS host with no global Node install, and the repository has no dev shell yet (task 0039 adds one). +Until then, prefix commands with `nix shell nixpkgs#nodejs -c ...`, adding `nixpkgs#tea` for anything that resolves credentials — including `gitea-axi pr create`. +`node_modules/` may be absent too, so `npm ci` first. +Commits need an explicit identity: `git -c user.name=alexion -c user.email=contact@alexion.dev commit ...`, matching the existing history. + +The Nix derivation's source is an **explicit allowlist** in [`package.nix`](package.nix), not the whole repository and not a gitignore filter. +A new build-relevant top-level file — a TypeScript configuration, a runner configuration the fast tier loads, a directory the build reads — must be added to that `lib.fileset.unions` list or `nix build` fails on a missing file. +The failure is loud but disconnected from its cause: the error names the missing file, not the allowlist that omitted it. +The flip side is the point of the design — touching an ADR, a spec, a task, a `bench/` file, or prose documentation must *not* change the derivation's output path. + +`buildNpmPackage` provides **no check hook**, so `doCheck = true` on its own is silently inert — the build logs `no Makefile or custom checkPhase, doing nothing` and ships a package whose tests never ran. +Running the fast tier inside the derivation requires an explicit `checkPhase`; it also needs `git` and `which` in `nativeCheckInputs` and a writable `HOME`, since some of those tests shell out to `git`. + +nixpkgs 26.11 (the `nixos-unstable` the flake tracks) has **dropped `x86_64-darwin`**. +`legacyPackages.x86_64-darwin` now *throws* rather than merely failing to build, so listing that system in the flake's `systems` breaks `nix flake show` and `nix flake check` for every system at once, not just that one. +Intel macOS would need the 26.05 branch. diff --git a/flake.lock b/flake.lock new file mode 100644 index 0000000..301a9a7 --- /dev/null +++ b/flake.lock @@ -0,0 +1,27 @@ +{ + "nodes": { + "nixpkgs": { + "locked": { + "lastModified": 1784356753, + "narHash": "sha256-12KrbMiWLcf8m7pCvAtZh1ZrgF85ZXDXvfR/fWTKy84=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "61b7c44c4073f0b827768aff0049561b5110ea5a", + "type": "github" + }, + "original": { + "owner": "NixOS", + "ref": "nixos-unstable", + "repo": "nixpkgs", + "type": "github" + } + }, + "root": { + "inputs": { + "nixpkgs": "nixpkgs" + } + } + }, + "root": "root", + "version": 7 +} diff --git a/flake.nix b/flake.nix new file mode 100644 index 0000000..186f62e --- /dev/null +++ b/flake.nix @@ -0,0 +1,31 @@ +{ + description = "Agent-ergonomic CLI for Gitea issues and pull requests"; + + # Tracks unstable to match the maintainer's system. Consumers deduplicate by + # pointing this input at their own nixpkgs, so it governs standalone builds + # only — never the deployed artifact. + inputs.nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable"; + + outputs = + { self, nixpkgs }: + let + # x86_64-darwin is deliberately absent: nixpkgs 26.11 dropped it, and + # `legacyPackages.x86_64-darwin` now throws rather than merely failing to + # build — so listing it would break `nix flake show` and `nix flake check` + # for every system, not just that one. Intel macOS needs the 26.05 branch. + systems = [ + "x86_64-linux" + "aarch64-linux" + "aarch64-darwin" + ]; + + forAllSystems = + f: nixpkgs.lib.genAttrs systems (system: f nixpkgs.legacyPackages.${system}); + in + { + packages = forAllSystems (pkgs: rec { + gitea-axi = pkgs.callPackage ./package.nix { }; + default = gitea-axi; + }); + }; +} diff --git a/package.nix b/package.nix new file mode 100644 index 0000000..fadf40d --- /dev/null +++ b/package.nix @@ -0,0 +1,119 @@ +{ + lib, + buildNpmPackage, + importNpmLock, + makeWrapper, + nodejs, + git, + tea, + which, +}: + +let + # The manifest is the canonical version: the release flow bumps it, and + # reading it here means a store path and a released version cannot disagree. + manifest = lib.importJSON ./package.json; + + # An explicit allowlist of what the build and its tests actually read. The + # repository's highest-churn directories — .claude, bench, prose docs — are + # all build-irrelevant, so a whole-repository source would let writing an ADR + # invalidate the derivation and force a rebuild with a full test run. + # + # Adding a build-relevant top-level file means adding it here too; the build + # otherwise fails on a missing file. + src = lib.fileset.toSource { + root = ./.; + fileset = lib.fileset.unions [ + ./src + # The fast tier only. `test/e2e` needs a live Gitea host and is excluded + # from the runner config, so admitting it would let end-to-end churn + # invalidate the derivation — the very cost this allowlist exists to + # avoid. `test/packaging` stays: task 0038 drives it against the + # installed binary. + (lib.fileset.difference ./test ./test/e2e) + ./skills + ./package.json + ./package-lock.json + ./tsconfig.json + ./tsconfig.build.json + ./vitest.config.ts + ]; + }; +in +buildNpmPackage { + pname = "gitea-axi"; + inherit (manifest) version; + inherit src nodejs; + + # Each dependency's fetch is derived from the integrity fields already in the + # lockfile, so a lockfile change needs no edit here. A single fixed-output + # hash would break on every dependency bump and be repaired by copying a hash + # out of an error message — a permanent recurring tax. + npmDeps = importNpmLock { npmRoot = src; }; + inherit (importNpmLock) npmConfigHook; + + 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 — see task 0042, + # which removes the hook's dependence on the entrypoint path entirely. Once it + # lands this rename should go with it. + 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. + doCheck = true; + nativeCheckInputs = [ + git + which + ]; + + # `buildNpmPackage` wires config, build and install hooks but no check hook, so + # `doCheck` alone is inert and the phase has to be spelled out. `git init` and + # `git commit` in the fast tier also need a writable HOME, which the sandbox + # otherwise points at a non-existent directory. + checkPhase = '' + runHook preCheck + + export HOME=$(mktemp -d) + npm run test + + runHook postCheck + ''; + + # ADR 0018: append, never prepend. The operator's own `tea` owns the + # credential store it refreshes in place, so the closure's copy is a + # fresh-machine fallback rather than an override. + postInstall = '' + wrapProgram $out/bin/gitea-axi \ + --suffix PATH : ${lib.makeBinPath [ git tea ]} + ''; + + meta = { + inherit (manifest) description homepage; + + # Looked up by SPDX identifier rather than hardcoded, for the same reason + # the version is read from the manifest: one canonical source, no second + # place to update on a relicence. + license = lib.licensesSpdx.${manifest.license}; + + mainProgram = "gitea-axi"; + + # Broader than the flake's `systems` list, deliberately. This describes what + # the package supports — everything, since it contains no compiled code — + # whereas that list encodes which systems the pinned nixpkgs can still + # evaluate. Consumed against 26.05, x86_64-darwin builds fine from here. + platforms = lib.platforms.linux ++ lib.platforms.darwin; + }; +}