diff --git a/.changeset/flake-source-build.md b/.changeset/flake-source-build.md new file mode 100644 index 0000000..b10d44b --- /dev/null +++ b/.changeset/flake-source-build.md @@ -0,0 +1,5 @@ +--- +"@systemfsoftware/claude-code-comment-checker": patch +--- + +Build the CLI binary directly from source in the Nix flake instead of fetching release assets, ensuring environments using Nix or direnv run the matching version with full command support. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8fba037..9f955e2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,3 +34,31 @@ jobs: uses: ./.github/workflows/mutation.yml permissions: contents: read + + nix: + name: nix flake check + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - uses: DeterminateSystems/nix-installer-action@e50d5f73bfe71c2dd0aa4218de8f4afa59f8f81d # v16 + with: + extra-conf: | + access-tokens = github.com=${{ github.token }} + - name: Flake evaluates + run: nix flake check + - name: Attribute resolves + run: nix eval .#packages.x86_64-linux.comment-checker-bwrap + - name: Build the source-built binary + # nix flake check and nix eval only evaluate; the derivation must be + # built or a broken src filter / compile failure ships green (the + # eval-only gate is precisely how a dropped Cargo.toml slipped past). + run: nix build --no-link .#packages.x86_64-linux.comment-checker-bwrap + - name: Smoke the built binary version + # Smoke the unwrapped binary: the bwrap wrapper needs --unshare-net, + # which GitHub Actions containers do not permit. + run: | + v="$(sed -n 's/^version = "\([^"]*\)"/\1/p' Cargo.toml | head -1)" + nix shell .#comment-checker -c comment-checker --version | grep -qF "$v" \ + || { echo "nix-built binary version mismatch (expected $v)" >&2; exit 1; } \ No newline at end of file diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 07d00d8..70035d1 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -115,7 +115,6 @@ jobs: permissions: contents: write id-token: write - pull-requests: write steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: @@ -166,8 +165,3 @@ jobs: run: ./scripts/tools/create-github-release.ts env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - - - name: Sync flake.nix release asset hashes - run: ./scripts/tools/sync-flake-hashes.ts - env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/AGENTS.md b/AGENTS.md index 80bdda0..2192766 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,6 +25,8 @@ No eager-read mandates: document reads are situational, triggered by the work, n - `README.md` — when working in a directory you have not worked in before. - `ARCHITECTURE.md` — when the task changes a module boundary or data flow. - Product/requirements docs — when a decision depends on product intent. +- `docs/solutions/` — when implementing or debugging in an area with documented past solutions. +- `CONCEPTS.md` — when orienting to shared domain vocabulary and definitions. ## Working Rules diff --git a/docs/plans/2026-08-30-0433-feat-flake-source-build-plan.md b/docs/plans/2026-08-30-0433-feat-flake-source-build-plan.md new file mode 100644 index 0000000..51a77d1 --- /dev/null +++ b/docs/plans/2026-08-30-0433-feat-flake-source-build-plan.md @@ -0,0 +1,175 @@ +--- +title: "feat: build comment-checker from source in-flake, drop fetchurl hashes" +date: 2026-08-30 +type: feat +artifact_contract: ce-unified-plan/v1 +artifact_readiness: implementation-ready +execution: code +product_contract_source: ce-plan-bootstrap +supersedes: docs/plans/2026-08-30-0409-feat-flake-input-comment-checker-plan.md +--- + +# feat: Build comment-checker from source in-flake, drop fetchurl hashes + +## Goal Capsule + +- **Objective:** The flake serves a comment-checker binary that always matches the repo's current source, with no release-asset hashes for a human or release step to keep in sync; the #81 stale-binary failure class is eliminated by construction. +- **Means:** Delete the `pkgs.fetchurl`-based `mkCommentChecker` and build from the repo tree with `rustPlatform` (KTD1); keep the `version` binding and the bwrap wrapper (KTD2); delete the now-obsolete hash gate/sync machinery (KTD3); add a CI nix-evaluation gate so flake correctness stays enforced after merge (KTD4). +- **Authority:** Packaging-only change; package attr names and devShell composition preserved (R2, R3). +- **Stop conditions:** All requirements hold; `nix flake check` passes locally and in CI; `check-versions.ts` stays green; no fetchurl/hash/release-asset references remain in the flake. Scope does not extend into the plugin-hook bug or PATH-binary issues from the debug session (deferred). +- **Tail ownership:** ce-work implements; ce-simplify-code / ce-code-review review; ce-commit-push-pr ships (including the new CI nix job). + +--- + +## Product Contract + +### Summary + +`flake.nix` currently fetches release binaries via `pkgs.fetchurl` with four hard-coded SRI hashes (master #82 added a publish-time sync PR and a recomputing CI gate to keep them current). This change stops fetching entirely: the binary is built in-flake from the repo's own source via `rustPlatform`, so there are no hashes to drift and the #81 failure class (flake serving stale bytes under a newer version) cannot recur by construction. The `version` binding stays so the repo's version-sync tooling keeps working; the fetchurl-only machinery — SRI hash gate, sync tool, and release step — is deleted cleanly (solution doc updated to match, not left describing dead machinery). A CI `nix flake check` job replaces the deleted hash gate as the shipped flake-evaluation gate. + +The prior flake-input approach (consuming `github:systemfsoftware/comment-checker` as a flake input, as systemfsoftware/systemfsoftware does) was invalidated at review and is superseded by this document. + +### Problem Frame + +Manual release-asset hash pinning is a maintenance trap: fixed-output derivations cache by name + declared hash, so a stale hash silently serves old bytes (issue #81: 0.1.0-era binary under the 0.3.2 name). #82 built four layers around the symptom — fresh hashes, publish-time sync PR, recomputing CI gate, `--version` smoke — all preserving the fetchurl design. The root fix is to stop fetching: a source build has no FOD hash to go stale, and the devshell then serves the current tree. The `--version` smoke (run-binary-smoke.ts in platform.yml, which smokes release binaries) is not fetchurl-era flake machinery and stays. + +### Requirements + +- R1. `flake.nix` builds the comment-checker binary from the repo's own source via `rustPlatform`; no `fetchurl`, no release-asset URLs, no SRI hash map remains. +- R2. The `packages.` surface is preserved exactly: `comment-checker` (the built binary), `comment-checker-bwrap` (the bwrap wrapper), `default` = the wrapped binary. External contract — downstream repos consume `packages..comment-checker`. +- R3. `devShells.default` changes only in how the binary is sourced (source-built instead of fetched); the Rust/JS toolchain (`rust-toolchain.toml` toolchain, cargo-mutants, gcc, nodejs, pnpm, bubblewrap) is unchanged. +- R4. The obsolete fetchurl-era machinery is removed, not orphaned: the `checkFlakeHashes` gate and its error classes, `scripts/tools/sync-flake-hashes.ts`, the `release.yml` hash-sync step, and the now-unused `sriFromSha256`/`unixTargetTriples` exports. The #81 solution doc (`docs/solutions/integration-issues/flake-fod-hash-drift-stale-binary.md`) is updated to describe the source build as the remedy, not the deleted machinery. `run-binary-smoke.ts` is explicitly out of scope — it smokes release binaries, not flake hashes. +- R5. The `version = "x.y.z"` binding stays in `flake.nix` and the version-sync surface it feeds (`bumpAllSurfaces`/`checkAllSurfaces`) remains green — releases keep bumping flake.nix's version. +- R6. CI enforces flake evaluation: a `nix flake check` job runs on PRs, so a flake that breaks evaluation or reintroduces a fetch fails the gate, not just the implementer's local run. +- R7. The source build does not run the crate's test suite during `nix build`/`nix develop` (`doCheck = false`), honoring the no-test decision. +- R8. No test-suite additions or changes anywhere (session-settled: user-directed — no tests). + +### Scope Boundaries + +- **Deferred to Follow-Up Work:** the plugin-hook `LD_FOR_BUILD` `NotCapable` crash (`hooks/run.ts`), the stale `0.1.0` binary on PATH, and the `--strip` under bwrap read-only-mount tension — diagnosed in the debug session, out of scope here. + +--- + +## Planning Contract + +### Key Technical Decisions + +- KTD1. **Build from source in-flake via `rustPlatform`; delete the fetchurl derivation.** (session-settled: user-directed — chosen over keeping fetchurl+hashes with the #82 gates, and over the flake-input approach invalidated at review: a source build leaves no FOD hash to drift, killing the #81 class by construction.) The build uses the existing `Cargo.toml`/`Cargo.lock` and `.cargo/config.toml` (static tree-sitter grammars) with no new toolchain. +- KTD2. **Keep the `version` binding in `flake.nix`.** (session-settled: user-directed — chosen over deleting it: the version-sync tooling (`extractNixVersion` through `checkAllSurfaces`/`bumpAllSurfaces`) hard-requires exactly one binding and fails CI otherwise; keeping it preserves the release-version surface. The fetch-tag `version` moves to the derivation's `version` attr.) The obsolete *hash* machinery (R4) is removed separately; only the version string survives. +- KTD3. **Delete the fetchurl-era hash machinery** — `checkFlakeHashes` and its error classes, `sync-flake-hashes.ts`, the release.yml hash-sync step, `sriFromSha256`/`unixTargetTriples` — and update the #81 solution doc so no standing instruction references the deleted mechanism (DEL1; git grep must find zero survivors incl. docs/). +- KTD4. **Add a CI `nix flake check` job.** The deleted `checkFlakeHashes` gate was the only CI-touching check of flake.nix on master (tools.yml runs check-versions.ts; no workflow runs nix). Removing it without a replacement would leave flake evaluation unenforced after merge — the exact drift-undetected failure mode #81/#82 existed to kill. A `determinatesystems/setup-nix` + `nix flake check` job on ubuntu-latest in ci.yml enforces it shipped. +- KTD5. **No tests for this change.** (session-settled: user-directed — chosen over adding coverage: the change is packaging config; verification is flake evaluation plus a runtime smoke, not a test suite. `doCheck = false` keeps the flake build itself from running the crate's tests.) + +### Assumptions + +- `rustPlatform.buildRustPackage` with `cargoLock.lockFile = ./Cargo.lock` builds the crate under nixpkgs-unstable's Rust toolchain (sufficient for a workspace with one crate; the repo's `.cargo/config.toml` env (`TSLP_LANGUAGES`, `TSLP_LINK_MODE=static`) is honored because the config lives in the source tree cargo reads). Default `doCheck = true` is overridden to `false` per R7. +- First source build costs a multi-minute compile (tree-sitter + ~37 static grammars under LTO fat) per lock revision; `nix develop` inherits it. Accepted as the cost of KTD1; the fetchurl path was a ~seconds download. +- The repo-root Cargo workspace (`Cargo.toml` with `members = ["crates/*"]`, root `Cargo.lock`, `.cargo/config.toml`) is the build surface; `npm/packages/comment-checker` is a TS launcher with no Cargo workspace. +- Master (4e8e34c3d, #82) is the merge base; this plan's file list matches master's tree as verified at plan time. + +### High-Level Technical Design + +```mermaid +flowchart LR + A[repo source
Cargo.toml + Cargo.lock + crates/] --> B[rustPlatform.buildRustPackage
doCheck = false] + B --> C[packages.comment-checker] + C --> D[bwrap wrapper] + D --> E[packages.comment-checker-bwrap + devShell] + F[version binding] --> G[version-sync surface
check-versions / release-version] + H[deleted: fetchurl + SRI hashes + gate + sync tool + solution-doc remedy] -.->|no path| C + I[new: CI nix flake check job] -->|enforces| B +``` + +No new components; the fetchurl derivation is replaced by a source build, the hash-only machinery is deleted, and CI enforces flake evaluation. + +--- + +## Implementation Units + +### U1. Build comment-checker from source in flake.nix + +- **Goal:** `flake.nix` builds the binary from the repo tree; fetchurl, hash map, and per-platform target map are gone; the build does not run tests. +- **Requirements:** R1, R2, R3, R5, R7. +- **Dependencies:** none. +- **Files:** `flake.nix` +- **Approach:** + 1. Keep the `nixpkgs` and `rust-overlay` inputs and the `forAllSystems` shape unchanged. + 2. Replace `mkCommentChecker`'s `fetchurl` body with `pkgs.rustPlatform.buildRustPackage { pname = "comment-checker"; inherit version; src = ./.; cargoLock.lockFile = ./Cargo.lock; doCheck = false; }` — one derivation for all systems, no target/hash maps. `doCheck = false` is intentional: `buildRustPackage` defaults to `true` (running `cargo test` in the sandbox), which would violate R8. + 3. Optionally scope `src` with `lib.fileset` (e.g. `lib.fileset.unions [ ./Cargo.toml ./Cargo.lock ./crates ./.cargo ]`) so unrelated repo edits do not change the derivation hash and force rebuilds. Optional: plain `src = ./.` is acceptable for this scale. + 4. Keep the `version` binding and pass it to the derivation. + 5. Keep `mkBwrap` unchanged; keep `packages` attr names (`comment-checker`, `comment-checker-bwrap`, `default`) and the devShell's use of the wrapped binary. +- **Patterns to follow:** the repo-root Cargo workspace (`Cargo.toml` with `members = ["crates/*"]`, root `Cargo.lock`, `.cargo/config.toml`) — the surface U1's `buildRustPackage` consumes; `mkBwrap` unchanged; the flake's existing `forAllSystems`. (Not `npm/packages/comment-checker` — that is a TS launcher with no Cargo workspace.) +- **Test scenarios:** none. + - `Test expectation: none -- config-only packaging change; user-directed no-test policy (R8); behavior verified by flake evaluation (U3).` +- **Verification:** `nix flake check` and attribute resolution succeed for `packages..comment-checker`, `packages..comment-checker-bwrap`, and `devShells..default`; `flake.nix` contains no `fetchurl`, `hash`, or release-asset URL; the build does not invoke `cargo test` (observable via build log/`doCheck`). + +### U2. Remove the fetchurl-era hash machinery + +- **Goal:** No dead hash path remains (DEL1): the SRI gate, sync tool, release step, and solution-doc remedy are deleted; the version-sync surface stays green. +- **Requirements:** R4, R5. +- **Dependencies:** U1. +- **Files:** `scripts/lib/version-files.ts`, `scripts/tools/check-versions.ts`, `scripts/tools/sync-flake-hashes.ts` (delete), `scripts/lib/shared.ts`, `.github/workflows/release.yml`, `docs/solutions/integration-issues/flake-fod-hash-drift-stale-binary.md` +- **Approach:** + 1. Delete `scripts/tools/sync-flake-hashes.ts` (its only job was rewriting the fetched hashes; U1 removes the fetch). + 2. Remove the `Sync flake.nix release asset hashes` step from `release.yml` (currently runs `sync-flake-hashes.ts`). + 3. In `version-files.ts`: delete `checkFlakeHashes` and its `FlakeHashMismatch`/`FlakeTagMissing`/`FlakeRemoteUnreachable` classes; drop the `checkFlakeHashes()` call at the end of `checkAllSurfaces` (the version-surface check on `flake.nix` **stays** — that is R5). Delete the now-unused `Crypto`/`Stream`/`ChildProcess` imports and helpers they existed for. + 4. In `check-versions.ts`: drop the `flakeHashes` reporting branch from the program. + 5. In `shared.ts`: remove `sriFromSha256` and `unixTargetTriples` (now consumed by nothing; verified via `git grep` against master — only the gate and the deleted sync tool used them). + 6. Update the #81 solution doc (`docs/solutions/integration-issues/flake-fod-hash-drift-stale-binary.md`) so the "Prevention" remedy section describes the in-flake source build instead of the deleted sync/gate machinery. The #81 failure-mode analysis stays valid; only the documented remedy changes. The doc is part of DEL1's "zero surviving references" scope. +- **Patterns to follow:** the repo's own DEL1 discipline — delete the definition, callers, and rules together; `git grep` to prove zero survivors. +- **Test scenarios:** none. + - `Test expectation: none -- script/workflow/doc removal; verified by the tools gate passing (U3), grep-proven absence of the removed names (incl. docs/), and the solution doc's updated remedy.` +- **Verification:** `git grep` for `sync-flake`, `sriFromSha256`, `unixTargetTriples`, `checkFlakeHashes`, `FlakeHashMismatch`, `FlakeTagMissing`, `FlakeRemoteUnreachable`, `flakeHashes` returns zero hits (including `docs/`); `./scripts/tools/check-versions.ts` exits 0 (version surface intact, hash gate gone). + +### U3. Add CI nix-evaluation gate + +- **Goal:** Flake evaluation is enforced by CI, not just the implementer's local run. +- **Requirements:** R6. +- **Dependencies:** U1 (the flake must evaluate before the job can pass). +- **Files:** `.github/workflows/ci.yml` (or a new `nix.yml` included from ci.yml) +- **Approach:** + 1. Add a `nix` job to ci.yml (ubuntu-latest) using `determinatesystems/setup-nix` (or equivalent pinned action). + 2. Run `nix flake check` (which evaluates all outputs, including the buildRustPackage derivation and the bwrap wrapper) plus the attribute-resolution check `nix eval .#packages.x86_64-linux.comment-checker-bwrap`. + 3. Gate the job on the same PR conditions as the other CI jobs. +- **Patterns to follow:** the repo's existing ci.yml job structure; the flake gate the plan's Verification Contract names. +- **Test scenarios:** none. + - `Test expectation: none -- CI workflow config; verified by the job passing on the PR (U4's verification).` +- **Verification:** the `nix` job is present in ci.yml and passes on the PR for this change. + +### U4. Verify evaluation, version surface, and smoke the built binary + +- **Goal:** The flake evaluates, the tools gate stays green, CI enforces the flake, and the devshell serves a working, sandboxed binary of the current version. +- **Requirements:** R1, R2, R3, R5, R6, R7, R8. +- **Dependencies:** U2, U3. +- **Files:** none (verification only). +- **Approach:** Runtime smoke through the flake's own surfaces — the repo's gate commands, not a test suite (R8): + 1. `nix flake check` — full flake evaluation gate (also now the CI job from U3). + 2. `nix build .#packages.x86_64-linux.comment-checker-bwrap` (or `nix develop -c comment-checker --version`), asserting the reported version equals the `Cargo.toml` workspace version (0.3.2) — not the stale 0.1.0. + 3. Pipe a real `PostToolUse` hook JSON payload into the wrapped binary and confirm it classifies comments (exit 0 on clean input, 2 with a report on flagged input) — proving the bwrap wrapper still works against the source-built binary. + 4. `./scripts/tools/check-versions.ts` exits 0 (this is the PR tools gate from `tools.yml`; it now checks the version surface only). +- **Test scenarios:** none. + - `Test expectation: none -- runtime smoke per R8; proof is the evaluation and smoke commands above, not committed tests.` +- **Verification:** all four commands above succeed with the stated outcomes. + +--- + +## Verification Contract + +- Flake gate (primary; also shipped as a CI job per R6): `nix flake check` +- Attribute resolution: `nix eval .#packages.x86_64-linux.comment-checker-bwrap` +- Version surface gate: `./scripts/tools/check-versions.ts` exits 0 (tools job in CI runs it on 3 OSes) +- Removal proof: `git grep` (incl. docs/) returns zero hits for `sync-flake`, `sriFromSha256`, `unixTargetTriples`, `checkFlakeHashes`, `FlakeHashMismatch`, `FlakeTagMissing`, `FlakeRemoteUnreachable`, `flakeHashes` — grep must match the actual symbols) +- Runtime smoke: build/enter the devShell, run `comment-checker --version` (must equal the Cargo workspace version) and a piped hook payload against the bwrap-wrapped binary +- Build hygiene: the derivation sets `doCheck = false` (no test suite run) +- No test suites run or added (R8) + +## Definition of Done + +- [ ] flake.nix builds from source; zero `fetchurl`/`hash`/release-asset references remain (R1) +- [ ] `packages..{comment-checker,comment-checker-bwrap}` and `default` preserved (R2) +- [ ] devShell contents unchanged apart from binary sourcing (R3) +- [ ] `sync-flake-hashes.ts`, the release.yml hash-sync step, the `checkFlakeHashes` gate, and the shared SRI helpers are deleted with zero surviving references — checked by `git grep` including `docs/` (R4) +- [ ] `version` binding kept; `check-versions.ts` exits 0 (R5) +- [ ] CI runs `nix flake check` on PRs (R6) +- [ ] `doCheck = false` in the derivation; build log shows no `cargo test` (R7) +- [ ] No test files added or modified (R8) \ No newline at end of file diff --git a/docs/solutions/integration-issues/flake-fod-hash-drift-stale-binary.md b/docs/solutions/integration-issues/flake-fod-hash-drift-stale-binary.md index 8f4c17c..8ba4e22 100644 --- a/docs/solutions/integration-issues/flake-fod-hash-drift-stale-binary.md +++ b/docs/solutions/integration-issues/flake-fod-hash-drift-stale-binary.md @@ -2,62 +2,52 @@ title: flake.nix release-asset hashes froze at the v0.1.5 era, so nix builds served the 0.1.0 binary under the 0.3.2 name date: 2026-08-29 category: integration-issues -module: flake.nix fetchurl hash block + release pipeline (release-version.ts, check-versions.ts, sync-flake-hashes.ts) +module: flake.nix source build (rustPlatform.buildRustPackage) + CI nix gate; version surface (release-version.ts, check-versions.ts) problem_type: integration_issue component: distribution root_cause: config_error -resolution_type: config_change +resolution_type: design_change severity: high symptoms: - "`nix build .#comment-checker` reports store path 0yxkwm3r...-comment-checker-0.3.2 and `--version` prints 0.1.0" - "The v0.3.2 GitHub release asset itself is byte-correct; only the flake's declared hash is wrong" - "A warm store builds the stale derivation in ~0.35s from cache — the intended 0.3.2 bytes are never fetched" - "Issue #81 reported the released binary is 'the 0.1.0 binary'" -tags: [flake, nix, fixed-output-derivation, fetchurl, sri, release-assets, hash-drift, check-versions, effect] +tags: [flake, nix, fixed-output-derivation, fetchurl, release-assets, hash-drift, source-build] --- # Nix FOD hash drift served a stale binary under a current version name ## Problem -`nix build .#comment-checker` produced a binary that reported version 0.1.0 even though the flake declared 0.3.2. The GitHub release assets for v0.3.2 were correct; the `flake.nix` `fetchurl` SRI hash block was not. The four hashes were set once in commit `403d2d079` (the v0.1.5 era) and never updated by the release pipeline, which bumps only the `version =` string. +`nix build .#comment-checker` produced a binary that reported version 0.1.0 even though the flake declared 0.3.2. The GitHub release assets for v0.3.2 were correct; the `flake.nix` hash block was not. Consumers of `github:systemfsoftware/comment-checker` silently ran a binary from six releases ago that lacked `--strip`. ## Symptoms - Store path `0yxkwm3r09fdbvr2pnhpbajiw01dp3i4-comment-checker-0.3.2` serves a 6100-byte-larger 0.1.0-era binary. - The same build on a warm store completes from cache in ~0.35s (fixed-output derivation cache hit) — no network fetch of the published asset. - Live asset probes are green (`--version` -> 0.3.2, `--help` lists `--strip`, `--strip` exits 2 with findings); only the declared hashes are stale. -- Consumers of `github:systemfsoftware/comment-checker` silently ran a binary from six releases ago that lacked `--strip`. -## What Didn't Work +## Root cause -- **Bumping the version string as a sync mechanism.** `release-version.ts` rewrote `version =` everywhere, but Nix fixed-output derivations cache by name + declared hash. The `fetchurl` FOD stores by the URL basename (`comment-checker-`, no version); the version appears only in the derivation output name. A version bump alone can never invalidate or refresh the cached bytes. -- **Re-uploading or re-releasing.** The correct binary was already published; a new tag would churn the npm/cargo/flake version surface for zero behavioral gain. -- **Relying on the rc-only smoke gate.** `run-binary-smoke.ts` asserted exit codes (clean 0, flagged 2) but never the version string — the 0.1.0-era binaries passed it exactly because it never checked what they were. +A fixed-output derivation (fetchurl) caches by URL basename + declared output hash. The released version appears only in the derivation output name, so bumping the `version =` string never touches which bytes the cache serves. The declared hashes were set once, by hand, and nothing recomputed them against the live release; the release pipeline bumps only the version surface. A smoke gate that asserted exit codes — not the binary's own identity — passed the stale binaries precisely because it never checked what they were. -## Solution +## Remedy -Four changes on branch gh-81 (pending PR): +The flake builds the binary from the repo's own source with `rustPlatform.buildRustPackage`, using the toolchain pinned in `rust-toolchain.toml` via `makeRustPlatform`. The derivation copies `Cargo.toml`, `Cargo.lock`, `crates/`, and `.cargo/` from the current tree (`cleanSourceWith`), so it always carries the current code; `doCheck = false` keeps the crate's tests in CI, not in the derivation. -1. **Refresh the flake hash block to the live v0.3.2 SRIs** (`flake.nix`): the four `sha256-...` values now equal the bytes at `releases/download/v0.3.2/comment-checker-` (measured this session from the live assets). -2. **Publish-phase auto-sync** (`scripts/tools/sync-flake-hashes.ts` + `release.yml`): after `create-github-release.ts` extracts the binaries, the step hashes them, rewrites the flake block, round-trip re-reads the file (CHK1), and opens an auto-merged `fix/flake-hashes-v` PR. A retried publish reuses the open PR (edit body) instead of failing on a duplicate create. The publish job gains `pull-requests: write`. -3. **Recomputing gate** (`scripts/lib/version-files.ts` + `check-versions.ts`): for the tag the flake declares, downloads each asset via `gh release download` (2 retries with fail-closed outcome), recomputes the SRI, and fails with `FlakeHashMismatch` naming the stale triple. Absent tags skip only when the declared version is newer than every published release, else `FlakeTagMissing`; an unreachable remote is `FlakeRemoteUnreachable` — never a silent skip. All services (`ChildProcessSpawner`, `Crypto`, `FileSystem`) are Effect services yielded in-effect via `DenoServices.layer`. -4. **Version assertion in the smoke gate** (`scripts/tools/run-binary-smoke.ts`): parses `--version` output and requires it to equal the workspace `Cargo.toml` version, with a strict semver guard. +There is no fetch of the released binary and no binary hash to go stale. The only `fetchurl` left is the tree-sitter-language-pack parser-sources bundle, delivered to the sandboxed build as a hash-pinned dependency (`TSLP_SOURCE_BUNDLE_URL=file://...`): nix verifies that hash, drift fails the build loudly, and the bundle version must track the crate version in Cargo.lock — a dependency pin, not an identity mechanism. -Shared helpers live in `scripts/lib/shared.ts`: `sriFromSha256` (SRI encoding) and `unixTargetTriples` (the flake block's four keys, derived from `targets.json` excluding win32) — one source of truth consumed by both sync and gate. - -## Why This Works - -Nix keys a fixed-output derivation on name + declared output hash. When the declared hash matches the published bytes, the FOD fetches the correct asset; when it doesn't, warm stores replay whatever bytes the stale hash previously certified. The fix makes the declared hash *be* the live asset hash (U1), makes publishing write the hashes automatically (U2), makes CI recompute and compare them from the live release (U3), and makes the smoke gate assert the binary's own identity (U4). The gate never trusts `version =` alone — it recomputes from fetched bytes (CHK1 discipline: a self-reported field certifies nothing). +The `version = "0.3.2"` binding stays so the version-sync surface keeps npm, cargo, and the flake in lockstep. CI runs a nix gate that evaluates the flake, builds the `comment-checker-bwrap` derivation, and asserts the built binary's `--version` equals the workspace `Cargo.toml` version. ## Prevention -- Never let release automation bump only the `version` string inside a flake: the hash block is an output of the release, not a constant. Sync it in the publish phase and verify with a recomputing gate. -- Any gate that stands in for a real invariant (here: "the nix build serves the version it claims") must exercise the same assets and inputs the real consumer uses — the gate downloads the same `comment-checker-` assets for the same `v` tag the flake fetches. -- A smoke gate for a released binary asserts identity, not just exit codes — assert `--version` against a single source of truth (the workspace manifest). -- Remote-unreachable and absent-tag states must fail loudly, not skip: a gate that silently skips on its own infrastructure failing is a permanent blind spot in exactly the broken-pipeline state it exists to catch. +- Do not hand-pin a released binary's hash in the flake. When the flake's repo is the release source, build from source — there is no hash to sync. +- A smoke gate asserts identity, not just exit codes: the built binary's `--version` must equal the workspace manifest, the single source of truth. +- A gate that guards a distribution surface must build what it guards. Evaluation-only checks (`nix flake check`, `nix eval`) pass a flake whose derivation cannot build. +- Hash-pinned dependency bundles are safe where nix verifies the hash and a mismatch is a hard build error. The failure mode to avoid is an unverified hash that silently serves old bytes. ## Related Issues -- Fixes https://github.com/systemfsoftware/comment-checker/issues/81 -- docs/solutions/architecture-patterns/rust-cli-npm-distribution.md — release automation and the version-surface discipline this flake hash block participates in. \ No newline at end of file +- Related: https://github.com/systemfsoftware/comment-checker/issues/81 +- docs/solutions/architecture-patterns/rust-cli-npm-distribution.md — release automation and the version-surface discipline. \ No newline at end of file diff --git a/flake.nix b/flake.nix index 5007e3c..6b9ce35 100644 --- a/flake.nix +++ b/flake.nix @@ -22,31 +22,42 @@ }; in f pkgs); version = "0.3.2"; + # Source build: no fetchurl of the released *binary*, so no binary hash + # to go stale (that fixed-output caching was the #81 failure). + # + # The toolchain pin is the repo's own: rust-toolchain.toml, from rust-overlay. mkCommentChecker = pkgs: let - target = { - "x86_64-linux" = "x86_64-unknown-linux-gnu"; - "aarch64-linux" = "aarch64-unknown-linux-gnu"; - "x86_64-darwin" = "x86_64-apple-darwin"; - "aarch64-darwin" = "aarch64-apple-darwin"; - }.${pkgs.system} or (throw "unsupported system ${pkgs.system}"); - hash = { - "x86_64-unknown-linux-gnu" = "sha256-ZWq3FcS8ILUCVNrC8u4jHCltywXNNuZAluF8IOK5BnI="; - "aarch64-unknown-linux-gnu" = "sha256-NxMnMpTcXQ/A1D2cZRbXDfYJEL+bqqJQphwWM3YZ7+M="; - "x86_64-apple-darwin" = "sha256-8RGZ/X6hFJe0sEHnD0OCJ7BF1iQX6DrGt0qAwYs78ho="; - "aarch64-apple-darwin" = "sha256-R62Dc6QVr64K5N/HPIDkfG6hXx9JUy/o43451wzpbNw="; - }.${target}; - src = pkgs.fetchurl { - url = "https://github.com/systemfsoftware/comment-checker/releases/download/v${version}/comment-checker-${target}"; - inherit hash; + toolchain = pkgs.rust-bin.fromRustupToolchainFile ./rust-toolchain.toml; + rustPlatform = pkgs.makeRustPlatform { cargo = toolchain; rustc = toolchain; }; + # tree-sitter-language-pack's build.rs downloads a parser-sources + # tarball at compile time; the nix sandbox has no network, so the + # bundle rides in as a hash-pinned fetchurl (like Cargo.lock — + # drift fails the build loudly). Keep the URL version equal to the + # tree-sitter-language-pack version in Cargo.lock, and the sha256 + # equal to this tarball's real hash. TSLP_SOURCE_BUNDLE_URL also + # accepts file://, which is how the sandboxed build reads it. + tslpParserSources = pkgs.fetchurl { + url = "https://github.com/xberg-io/tree-sitter-language-pack/releases/download/v1.14.3/parser-sources-1.14.3.tar.zst"; + sha256 = "f13bcf0be1b9a99c45ead43671ae311346c2a10807c3fdc7c76407445ea7f2c7"; }; - in pkgs.stdenv.mkDerivation { + in rustPlatform.buildRustPackage { pname = "comment-checker"; - inherit version src; - dontUnpack = true; - installPhase = '' - install -Dm755 $src $out/bin/comment-checker - ''; + inherit version; + src = nixpkgs.lib.cleanSourceWith { + src = ./.; + filter = path: type: + (type == "directory") || + (builtins.elem (baseNameOf path) [ "Cargo.toml" "Cargo.lock" ]) || + (builtins.match ".*/.cargo/.*" path != null) || + (builtins.match ".*/crates/.*" path != null); + }; + cargoLock.lockFile = ./Cargo.lock; + # The repo's quality gates (cargo test, mutation) run in CI, not in + # this derivation; doCheck defaults to true in buildRustPackage and + # would run the whole suite inside the nix sandbox. + doCheck = false; + TSLP_SOURCE_BUNDLE_URL = "file://${tslpParserSources}"; meta = with pkgs.lib; { description = "Claude Code PostToolUse hook that flags unnecessary comments"; homepage = "https://github.com/systemfsoftware/comment-checker"; diff --git a/scripts/lib/shared.ts b/scripts/lib/shared.ts index cb7c3bd..d2b48a3 100644 --- a/scripts/lib/shared.ts +++ b/scripts/lib/shared.ts @@ -30,10 +30,3 @@ export interface LauncherManifest { repository: { type: string; url: string } optionalDependencies?: Record } -/** SRI encoding of a SHA-256 digest (Nix `fetchurl` hash format). */ -export const sriFromSha256 = (digest: Uint8Array): string => - `sha256-${btoa(String.fromCharCode(...digest))}` - -/** Flake hash-block keys: every target except the win32 row in targets.json. */ -export const unixTargetTriples = (targets: Target[]): string[] => - targets.filter((t) => t.os !== 'win32').map((t) => t.target) diff --git a/scripts/lib/version-files.ts b/scripts/lib/version-files.ts index 835b9d4..ed9f90a 100644 --- a/scripts/lib/version-files.ts +++ b/scripts/lib/version-files.ts @@ -1,6 +1,4 @@ -import { ChildProcess, ChildProcessSpawner } from 'effect/unstable/process' -import { Crypto, Effect, FileSystem, Schema, Stream } from 'effect' -import { sriFromSha256, type Target, TARGETS_PATH, unixTargetTriples } from './shared.ts' +import { Effect, FileSystem } from 'effect' import { CRATES_DIR, extractJsonVersion, @@ -13,7 +11,7 @@ import { replaceNixVersion, replaceTomlVersion, ROOT_MANIFEST, - Semver, + type Semver, VersionMismatch, WORKSPACE_CARGO, } from './version-sync.ts' @@ -108,148 +106,5 @@ export const checkAllSurfaces = () => } if (diffs.length > 0) return yield* new VersionMismatch({ expected, diffs }) - - const flakeHash = yield* checkFlakeHashes() - if (flakeHash.stale.length > 0) { - return yield* new FlakeHashMismatch({ version: expected, stale: flakeHash.stale }) - } - return { expected, workspaceVersion, pluginChecked, flakeHashes: flakeHash } - }) - -export class FlakeHashMismatch - extends Schema.TaggedError()('FlakeHashMismatch', { - version: Semver, - stale: Schema.Array( - Schema.Struct({ - triple: Schema.String, - reason: Schema.Union([Schema.Literal('mismatch'), Schema.Literal('download-failed')]), - detail: Schema.optional(Schema.String), - }), - ), - }) { - override get message(): string { - return this.stale - .map((s) => - s.reason === 'download-failed' - ? `check-versions: could not download the v${this.version} release asset for ${s.triple}` + - (s.detail ? `: ${s.detail}` : '') - : `check-versions: flake.nix hash for ${s.triple} does not match the v${this.version} release asset` - ) - .join('\n') - } -} - -export class FlakeTagMissing extends Schema.TaggedError()('FlakeTagMissing', { - declared: Semver, - newest: Schema.String, -}) { - override get message(): string { - return `check-versions: flake.nix declares v${this.declared} but no such release tag exists and it is not newer than the newest published release (v${this.newest})` - } -} - -export class FlakeRemoteUnreachable - extends Schema.TaggedError()('FlakeRemoteUnreachable', {}) { - override get message(): string { - return 'check-versions: could not list release tags from origin — the flake hash gate cannot verify the declared version' - } -} - -const sriOfDigest = (bytes: Uint8Array) => - Effect.gen(function* () { - const crypto = yield* Crypto.Crypto - const digest = yield* crypto.digest('SHA-256', bytes) - return sriFromSha256(new Uint8Array(digest)) - }) - -const remoteReleaseTags = Effect.gen(function* () { - const spawner = yield* ChildProcessSpawner.ChildProcessSpawner - const lsRemote = ChildProcess.make('git', ['ls-remote', '--tags', 'origin']) - const handle = yield* spawner.spawn(lsRemote) - const exit = yield* handle.exitCode - if (exit !== 0) { - return yield* new FlakeRemoteUnreachable() - } - const text = yield* Stream.mkString(Stream.decodeText(handle.stdout)) - return [ - ...new Set( - [...text.matchAll(/refs\/tags\/v(\d+\.\d+\.\d+)(\^\{\})?$/gm)].map((m) => m[1]), - ), - ] -}) - -const newerThan = (a: string, b: string): boolean => { - const [am, bm] = [a.split('.').map(Number), b.split('.').map(Number)] - for (let i = 0; i < 3; i++) { - if ((am[i] ?? 0) !== (bm[i] ?? 0)) return (am[i] ?? 0) > (bm[i] ?? 0) - } - return false -} - -export const checkFlakeHashes = () => - Effect.gen(function* () { - const fs = yield* FileSystem.FileSystem - const spawner = yield* ChildProcessSpawner.ChildProcessSpawner - if (!(yield* fs.exists(FLAKE_NIX))) { - return { stale: [], skipped: 'flake.nix absent' } - } - const flakeText = yield* fs.readFileString(FLAKE_NIX) - const declared = yield* extractNixVersion(flakeText, FLAKE_NIX) - const tag = `v${declared}` - const releaseTags = yield* remoteReleaseTags - const tagExists = releaseTags.includes(declared) - - if (!tagExists) { - const newest = releaseTags.slice().sort((a, b) => - newerThan(a, b) ? 1 : newerThan(b, a) ? -1 : 0 - ).at(-1) - if (newest === undefined || newerThan(declared, newest)) { - return { stale: [], skipped: `tag ${tag} absent (version bump in flight)` } - } - return yield* new FlakeTagMissing({ declared, newest }) - } - - const targetsText = yield* fs.readFileString(TARGETS_PATH) - const triples = unixTargetTriples(JSON.parse(targetsText) as Target[]) - const results = yield* Effect.forEach(triples, (triple) => - Effect.gen(function* () { - const assetName = `comment-checker-${triple}` - const dir = yield* Effect.acquireRelease( - fs.makeTempDirectory(), - (d) => fs.remove(d, { recursive: true }).pipe(Effect.orDie), - ) - const gh = ChildProcess.make('gh', [ - 'release', - 'download', - tag, - '--pattern', - assetName, - '--dir', - dir, - '--clobber', - ]) - const attempt = Effect.gen(function* () { - const handle = yield* spawner.spawn(gh) - const stderr = yield* Stream.runCollect(Stream.decodeText(handle.stderr)).pipe( - Effect.map((chunks) => chunks.join('').trim()), - ) - const code = yield* handle.exitCode - return { code, stderr } - }) - let outcome = yield* attempt - for (let i = 0; i < 2 && outcome.code !== 0; i++) { - outcome = yield* attempt - } - // The file must exist even when gh reports success: a silent no-op - // download (rc 0, nothing written) must fail this gate, not pass it. - if (outcome.code !== 0 || !(yield* fs.exists(`${dir}/${assetName}`))) { - return { triple, reason: 'download-failed' as const, detail: outcome.stderr } - } - const bytes = yield* fs.readFile(`${dir}/${assetName}`) - const sri = yield* sriOfDigest(bytes) - return flakeText.includes(`"${triple}" = "${sri}"`) - ? null - : { triple, reason: 'mismatch' as const } - }), { concurrency: 4 }) - return { stale: results.filter((r) => r !== null), skipped: null } + return { expected, workspaceVersion, pluginChecked } }) diff --git a/scripts/tools/check-versions.ts b/scripts/tools/check-versions.ts index e4dbe09..d7de117 100755 --- a/scripts/tools/check-versions.ts +++ b/scripts/tools/check-versions.ts @@ -1,4 +1,4 @@ -#!/usr/bin/env -S deno run --allow-read --allow-env --allow-write --allow-run=git,gh +#!/usr/bin/env -S deno run --allow-read --allow-env import { runMain } from '@effect/platform-deno/DenoRuntime' import { layer as DenoPlatform } from '@effect/platform-deno/DenoServices' @@ -6,12 +6,7 @@ import { Console, Effect } from 'effect' import { checkAllSurfaces } from '../lib/version-files.ts' const program = Effect.gen(function* () { - const { expected, workspaceVersion, pluginChecked, flakeHashes } = yield* checkAllSurfaces() - if (flakeHashes.skipped) { - yield* Console.log(`check-versions: flake hash gate skipped (${flakeHashes.skipped})`) - } else { - yield* Console.log(`check-versions: flake hash gate verified against live release assets`) - } + const { expected, workspaceVersion, pluginChecked } = yield* checkAllSurfaces() yield* Console.log( pluginChecked ? `check-versions: ok npm=${expected} workspace=${workspaceVersion}` @@ -19,4 +14,4 @@ const program = Effect.gen(function* () { ) }) -runMain(Effect.scoped(program).pipe(Effect.provide(DenoPlatform))) +runMain(program.pipe(Effect.provide(DenoPlatform))) diff --git a/scripts/tools/sync-flake-hashes.ts b/scripts/tools/sync-flake-hashes.ts deleted file mode 100755 index 8148cf9..0000000 --- a/scripts/tools/sync-flake-hashes.ts +++ /dev/null @@ -1,169 +0,0 @@ -#!/usr/bin/env -S deno run --allow-run=git,gh --allow-read --allow-write --allow-env - -// Keep flake.nix release-asset hashes in sync with the binaries just published. -// Runs in the release.yml publish job after create-github-release.ts, which -// extracts the raw binaries into release-assets/binaries/. -// -// Nix fixed-output derivations cache by name + declared hash: bumping the -// version string alone never invalidates the fetchurl store path, so a flake -// whose hashes lag the published assets serves stale bytes (or fails cold -// stores). This script rewrites the four SRI values to the just-built bytes -// and opens a PR, so consumers always fetch the binary the version claims. -// -// DRY_RUN=1 prints the would-be PR without mutating anything. - -import { - LAUNCHER_MANIFEST_PATH, - sriFromSha256, - type Target, - TARGETS_PATH, - unixTargetTriples, -} from '../lib/shared.ts' -import { FLAKE_NIX } from '../lib/version-sync.ts' - -const MANIFEST = LAUNCHER_MANIFEST_PATH -const FLAKE = FLAKE_NIX -const BRANCH_PREFIX = 'fix/flake-hashes-' -const BASE = 'master' - -async function exec(cmd: string, args: string[], allowFail = false): Promise { - const out = await new Deno.Command(cmd, { - args, - stdout: 'piped', - stderr: 'inherit', - }).output() - if (!out.success && !allowFail) { - throw new Error(`${cmd} ${args.join(' ')} failed`) - } - return new TextDecoder().decode(out.stdout).trim() -} - -const sha256Sri = async (bytes: Uint8Array): Promise => { - const copy = new Uint8Array(bytes.length) - copy.set(bytes) - return sriFromSha256(new Uint8Array(await crypto.subtle.digest('SHA-256', copy))) -} - -function rewriteFlakeHashes(text: string, srIs: Map): string { - let out = text - for (const [triple, sri] of srIs) { - const pattern = new RegExp(`("${triple}"\\s*=\\s*)"sha256-[A-Za-z0-9+/=]+"`) - const match = pattern.exec(out) - if (!match) { - throw new Error(`flake.nix: no hash entry for ${triple}`) - } - out = out.slice(0, match.index) + match[1] + `"${sri}"` + - out.slice(match.index + match[0].length) - } - return out -} - -const launcherManifest = JSON.parse(await Deno.readTextFile(MANIFEST)) -const version = launcherManifest.version as string -const targets: Target[] = JSON.parse(await Deno.readTextFile(TARGETS_PATH)) -// The flake hash block has one key per unix triple; the windows row has no -// key there and must not enter the set. -const unixTargets = unixTargetTriples(targets) - -const binaries = await Promise.all( - unixTargets.map(async (target) => { - const p = `release-assets/binaries/comment-checker-${target}` - let bytes: Uint8Array - try { - bytes = await Deno.readFile(p) - } catch { - throw new Error( - `sync-flake-hashes: missing binary for ${target} at ${p} — create-github-release.ts must have run first`, - ) - } - return { target, sri: await sha256Sri(bytes) } - }), -) - -const nextSrIs = new Map(binaries.map((b) => [b.target, b.sri])) -const flakeText = await Deno.readTextFile(FLAKE) -const rewritten = rewriteFlakeHashes(flakeText, nextSrIs) -const changed = rewritten !== flakeText - -if (!changed) { - console.log(`sync-flake-hashes: flake.nix hashes already match v${version} — nothing to do`) - Deno.exit(0) -} - -const branch = `${BRANCH_PREFIX}v${version}` -const dryRun = Deno.env.get('DRY_RUN') === '1' - -if (dryRun) { - console.log(`sync-flake-hashes [dry-run]: would open ${branch} updating flake.nix to v${version}`) - for (const b of binaries) { - console.log(` ${b.target} -> ${b.sri}`) - } - Deno.exit(0) -} - -await Deno.writeTextFile(FLAKE, rewritten) - -// Round-trip: the rewrite must be byte-exact for every SRI (CHK1 — never -// trust the version field alone; re-read the file and compare). -const after = await Deno.readTextFile(FLAKE) -for (const b of binaries) { - if (!after.includes(`"${b.target}" = "${b.sri}"`)) { - throw new Error(`sync-flake-hashes: hash rewrite did not round-trip for ${b.target}`) - } -} - -await exec('git', ['config', 'user.name', 'github-actions[bot]']) -await exec('git', ['config', 'user.email', '41898282+github-actions[bot]@users.noreply.github.com']) -await exec('git', ['switch', '--force-create', branch]) -await exec('git', ['add', '--', FLAKE]) -await exec('git', ['commit', '-m', `fix(flake): sync v${version} release asset hashes`]) -await exec('git', ['push', '--force', 'origin', branch]) - -const prBody = - `Refreshes the \`flake.nix\` \`fetchurl\` SRI set to the v${version} release assets. ` + - 'Nix fixed-output derivations cache by name + declared hash, so the version-string bump alone never invalidates ' + - 'the fetched bytes; these hashes are what make `nix build .#comment-checker` serve the binary this release published.' - -// Bound the broken-window: from tag creation until this PR merges, a fresh -// nix build of master fails (declared version + old hashes). Auto-merge lands -// it as soon as CI passes when branch protection permits bot auto-merge. -// A retried publish must reuse the open PR instead of failing on a duplicate -// create — the rerun rewrites the branch (force-push) and edits the body. -const existingPr = await exec('gh', [ - 'pr', - 'list', - '--head', - branch, - '--state', - 'open', - '--json', - 'number', - '--jq', - '.[0].number // empty', -]) -let prNumber: string -if (existingPr) { - await exec('gh', ['pr', 'edit', existingPr, '--body', prBody]) - prNumber = existingPr - console.log(`sync-flake-hashes: updated PR ${prNumber} for ${branch}`) -} else { - prNumber = (await exec('gh', [ - 'pr', - 'create', - '--base', - BASE, - '--head', - branch, - '--title', - `fix(flake): sync v${version} release asset hashes`, - '--body', - prBody, - '--json', - 'number', - '--jq', - '.number', - ])).trim() - console.log(`sync-flake-hashes: opened PR ${prNumber} for ${branch}`) -} - -await exec('gh', ['pr', 'merge', '--auto', '--squash', prNumber], true)