From c043f2946cd4642227bc52ce1bc9bf6129b90523 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 31 Jul 2026 13:15:29 -0400 Subject: [PATCH] fix(#2914): per-PR ack fragments instead of one shared mutable file (#2923) * fix(#2914): never persist a spent emitted-drift ack on next tests/emitted-drift-ack.json held 34 spent #2834 entries merged via #2900. Every entry is scoped to the diff that introduced it (#2789), so once merged to next it is at the base by definition -- spent and inert. Its presence is still load-bearing though: each PR rewrites the paths map wholesale, making a persistent base copy a shared cell. Five of six conflicting PRs in the open queue collided on this file and nothing else. Deletes the stale document and adds a push-to-next guard asserting it stays absent. The guard is deliberately NOT wired into lint:ci -- a PR-lane check against the base is the #2768 shape #2789 exists to end. Closes #2914 Co-Authored-By: Claude Opus 5 * chore(#2914): backfill changeset pr number Co-Authored-By: Claude Opus 5 * fix(#2914): per-PR ack fragments instead of one shared mutable file The emitted-drift acknowledgment lived in a single tests/emitted-drift-ack.json whose paths map every PR rewrote wholesale. That is a shared mutable cell: any two PRs needing an ack edit the same lines and conflict. Five of six conflicting PRs in the open queue collided on this file and nothing else. Acks now live as per-PR fragments under tests/emitted-drift-acks/, the same shape .changeset/ already uses to solve this exact problem. Two PRs pick different filenames, so they cannot collide, and fragments lingering on next are harmless rather than toxic. The legacy file's 35 entries are MIGRATED into a fragment, not deleted. An earlier delete-only attempt failed verification twice: the ratchet lost the spec-phase.md acknowledgment from #2779 and reported a 10-byte growth with no ack. Relocating preserves every acknowledgment. The legacy single file is still READ (unioned with the fragments) because five open PRs carry it; dropping support would break all of them. A duplicate path key across sources is a hard error, never last-wins. The push-to-next guard is retargeted accordingly: it now asserts only that the legacy SHARED file never reappears on next. Fragments may persist harmlessly. Closes #2914 Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 --- .changeset/sunny-finches-rest.md | 5 + .github/workflows/test.yml | 34 + CONTEXT.md | 8 +- CONTRIBUTING.md | 56 +- scripts/lint-emitted-drift-ack.cjs | 237 +++++- tests/emitted-attribution.test.cjs | 693 +++++++++++++++++- .../0000-legacy-migration.json} | 0 tests/helpers/emitted-diff.cjs | 156 +++- tests/helpers/emitted-runtime.cjs | 160 +++- 9 files changed, 1287 insertions(+), 62 deletions(-) create mode 100644 .changeset/sunny-finches-rest.md rename tests/{emitted-drift-ack.json => emitted-drift-acks/0000-legacy-migration.json} (100%) diff --git a/.changeset/sunny-finches-rest.md b/.changeset/sunny-finches-rest.md new file mode 100644 index 000000000..6cc0526db --- /dev/null +++ b/.changeset/sunny-finches-rest.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2923 +--- +**Contributor PRs stop conflicting on a file they never meaningfully changed** — the emitted-drift acknowledgment moves from one shared `tests/emitted-drift-ack.json` every PR rewrote wholesale to per-PR fragments under `tests/emitted-drift-acks/`, so two PRs needing an acknowledgment can no longer collide with each other; the legacy file's 35 spent entries are migrated (not deleted) into a fragment so nothing is lost, and a next-only push guard now fails if the legacy shared file itself ever reappears, since every entry is scoped to the diff that introduced it and is spent the moment it merges. (#2914) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f3a64bd89..0c8d8ed85 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -583,3 +583,37 @@ jobs: with: path: .gsd-cache/emitted-baseline.json key: emitted-baseline-${{ github.sha }} + + # #2914: tests/emitted-drift-ack.json (the LEGACY single ack file) must never persist + # on `next`. Every entry is scoped to the diff that introduced it (#2789) — once + # merged it is, by definition, already at the base, so it is spent and inert + # regardless of shape. Acks now go in per-PR fragments under + # tests/emitted-drift-acks/ instead — one independently-named file per PR, never a + # single file every PR rewrites wholesale — so a fragment LEFT ON next is harmless and + # is deliberately NOT what this guard checks; only the legacy shared file is a shared + # merge-conflict cell worth guarding against. This is DELIBERATELY NOT a PR-lane check + # comparing a PR's base ack against `next`: that is exactly the #2768 shape #2789 was + # written to end (a spent-but-present base ack would red every open PR the moment one + # landed). It runs only here, on push to `next`, asserting a fact about `next`'s own + # tree; it needs no npm install, since the guard is pure fs.existsSync. + # + # Enforcement scope, stated honestly: this job is push-triggered and runs post-merge, + # and is not wired into `required-tests` — it cannot BLOCK a merge. A red run here + # only turns `next`'s own CI red for manual follow-up, same as publish-emitted-baseline + # above. It fires reliably today because the path that reaches `next` + # (`auto-backmerge.yml`'s admin-merge step) authenticates with `GSD_BOT_PR_TOKEN`, a + # PAT, which DOES trigger this workflow on push. If that secret ever lapses, the + # `|| secrets.GITHUB_TOKEN` fallback there would push with the default token instead, + # which GitHub's anti-recursion rule keeps from triggering new workflow runs — + # silently skipping this job and publish-emitted-baseline alike. That is a known, + # shared limitation of every push-to-next job in this file, not specific to this guard. + guard-no-ack-on-next: + name: Guard no spent ack on next + if: github.event_name == 'push' && github.ref == 'refs/heads/next' + runs-on: ubuntu-latest + timeout-minutes: 1 + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Assert tests/emitted-drift-ack.json is absent + run: node scripts/lint-emitted-drift-ack.cjs --guard-next diff --git a/CONTEXT.md b/CONTEXT.md index 8d762f79d..dc1c0f2cd 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -422,7 +422,7 @@ The producer half of the async external-job contract (#1164, part of #1105). Def The enforced cross-phase defect register operationalizing GSD's no-defer discipline as a tracked artifact (#1950). Markdown file at `.planning/WINDOWS.md` (project-level, cross-phase) with YAML frontmatter carrying scalar counts (`schema_version`, `open_count`, `waived_count`, `fixed_count`, `total_count`, `last_updated`) for the FAST path the gate reads via jq without parsing JSON, plus a JSON code block as the AUTHORITATIVE entries source; the two cross-check and fail closed on drift. Each entry: `{ id, kind, phase, file, line, description, status, reason, recorded_at, resolved_at }`; kinds are closed (`stub | todo | fixme | skipped-test | lint-warning | unmet-truth | unrun-verify | deviation`); statuses are closed (`open | waived | fixed`). The `broken-windows` Capability (`capabilities/broken-windows/capability.json`) registers one `ship:pre` gate with predicate `artifact-frontmatter-equals WINDOWS.md open_count == 0`; federated config key `workflow.windows_enforce` (default `false` — opt-in enforcement, tracking-only by default so a project can adopt the ledger before turning the gate on). Population is best-effort and never blocks execution: `agents/gsd-executor.md` appends stubs/skipped-tests/unrun-verifies via `gsd_run windows append` after writing SUMMARY.md. Source of truth: `src/broken-windows.cts` → `gsd-core/bin/lib/broken-windows.cjs` (pure `parseLedger`/`renderLedger`/`appendWindow`/`markWaived`/`markFixed` + I/O `cmdWindowsStatus`/`Append`/`Waive`/`MarkFixed`); CLI surface `gsd-tools windows status|append|waive|fixed`. Ship gate enforcement is a `capId == "broken-windows"` branch in `gsd-core/workflows/ship.md` preflight (sibling to the `security` branch); it reads `gsd_run windows status --raw` and fails closed on a non-zero/non-numeric `open_count` (an unparseable ledger is itself a broken window). `/gsd:progress` surfaces the open+waived count. The ledger is optional and backward-compatible: a project with no `.planning/WINDOWS.md` reports `open_count: 0` and ships cleanly, and with `workflow.windows_enforce=false` (the default) ship never blocks on it. Frozen `REASON` enum: `WINDOWS_LEDGER_MISSING | WINDOWS_LEDGER_MALFORMED | WINDOWS_ID_NOT_FOUND | WINDOWS_ALREADY_RESOLVED | WINDOWS_WAIVE_REASON_EMPTY | WINDOWS_INVALID_KIND | WINDOWS_INVALID_FILE | WINDOWS_INVALID_ID | WINDOWS_APPEND_MISSING_FIELD | WINDOWS_USAGE | WINDOWS_OK` — surfaced through `--json-errors` for typed test assertions. Test seam: `tests/broken-windows.test.cjs`. Origin: *The Pragmatic Programmer* Topic 3 (Hunt & Thomas — software transplant of Wilson & Kelling's broken-windows metaphor) plus Cunningham's debt metaphor (decay accrues interest ⇒ accounting, not just habit). ### Emitted Artifact Provenance -Cross-seam principle (ADR-2719, epic #2719): a committed artifact that is a pure function of the source tree is not reviewable state — it is derived state wearing a review costume, and it must be *attributable* rather than *pinned*. Concept, not a Module: it ships nothing, so it takes no `Module` suffix (follows the `### Resolution Provenance` precedent). Scope is the emitted-artifact family named by `RULESET.EMITTED_ATTRIBUTION`. The principle: every emitted path whose hash moved between `next` HEAD and PR HEAD must be attributable — through a declarative provenance table — to a path the pull request actually changed; unattributable deltas are a hard failure that *names them* rather than an anomaly a reviewer must notice inside 7,500 lines of hex. Totality is enforced, so an emitted path matching no rule fails loudly instead of passing through unattributed. The escape hatch is a committed acknowledgment (`tests/emitted-drift-ack.json`), deliberately not a flag or env var — the file appears in the changed-files list ONLY when something rippled unexpectedly, so touching it IS the alarm, whereas today 100% of emitted-byte changes touch fixtures and touching them signals nothing. The same differential machine carries the size ratchet: growth is reported with exact byte deltas and needs the same acknowledgment, so anti-creep survives without pinning a number. Distinguish from the absolute check that remains: `tests/fixtures/install-tree/*.json` stays committed and normally-merged (ADR-2719 §7) because "the installer stopped shipping X" must fail with no attribution reasoning involved. Delivery was phased — #2721 naming + interim merge relief, #2722 provenance table + totality guard, #2723 differential check dual-run beside `golden-install-parity.test.cjs`, #2724 cutover (COMPLETE: the dual-run window observed agreement on real PRs after fixing #2750/#2760, and the golden fixtures/test/generator/merge-driver bridge are now deleted; the differential is the sole gate). The table LANDED in #2722 as `tests/helpers/emitted-provenance.cjs` (19 rules, guarded by `tests/emitted-provenance.test.cjs`); it maps emitted path → repo source and is TOTAL over EVERY emitted path in all 19 manifests — exactly one rule per path, with zero-match, two-match, AND dead-rule (a rule matching nothing) all hard failures, so table rot is loud in both directions. Deliberately NO path/family counts are recorded here: those move with every shipped-content edit, and a hand-maintained number in glossary canon is the exact silent-drift failure this whole seam exists to end. The guard recomputes them from the fixtures on every run — read them from a failure message, never from prose. What IS stable is the rule count, which changes only when a new emitted family or host appears. Note the surface is materially wider than #2722 estimated from `claude.json` alone (its "13 families / 15-20 rules" was a single-runtime sample; the 19-manifest surface spans runtime-specific roots — `.agents/`, `.kimi/hooks/`, `command/`, `agents/subagents/`, `.clinerules/`, `plugins/`, `extensions/`, `.gsd/`, the hermes `skills/gsd/` category and the #69 nested `skills//skills//` layout). Two design invariants carry forward to #2723: emitted SHAPES are hard-coded (deriving them from the installer would make the guard tautological — it would follow any installer change silently), while source PATHS may read a first-party descriptor where that descriptor is the sole declaration (`hostBehaviors.nativePlugin.source`); and attribution is keyed on `(rel, runtime)`, never `rel` alone, because one emitted path has different sources per host (`plugins/gsd-core.js` ← `.opencode/` vs `.kilo/`). Emitted skills attribute to `commands/gsd/*.md`, NEVER the repo `skills/` dir — that dir is itself generated from `commands/gsd` by `scripts/gen-plugin-skills.cjs`, so attributing to it is false attribution that still passes totality. Totality does NOT catch a rule pointing at the WRONG source (the recorded residual); the guard against that is the companion assertion that every attributed source EXISTS in the repo, which caught three real cases while the table was built (Copilot's `.agent.md` rename, Kimi's code-literal `agents/gsd.{yaml,md}` root agent, and Copilot's `hooks/gsd-session.json`). A `sources` entry ending in `/` is a PREFIX, not a file — and prefix matching is SEGMENT-AWARE, so a source of `agents/` must not attribute `agentsfoo/x.md`. The differential check LANDED in #2723 as `tests/helpers/emitted-diff.cjs` (the conservation law, a PURE function — no fs/git/installer/clock) + `tests/helpers/emitted-baseline.cjs` (baseline resolution), guarded by `tests/emitted-attribution.test.cjs`. It ran DUAL beside `golden-install-parity.test.cjs` through the #2723 dual-run window with both green and fixtures untouched; #2724 deleted the golden fixtures/test/generator and the check is now the sole gate. Purity is deliberate and load-bearing: the naive one-big-integration-test shape would need ~38 installer spawns per assertion, so the four failing-first criteria would not in practice get written — which is exactly how a phase ships promised-but-not-built. Buckets are CONSERVED: every moved emitted path lands in exactly one of `attributed | unattributable | acked` (property-tested), and a path the provenance table cannot resolve surfaces as an ERROR rather than a silent skip. Four asymmetries worth knowing: an ADDED emitted key is a ripple too (not just modified ones); `synthesized` paths are exempt but `code-derived` ones are NOT (that is why Phase 2 refused to mark them exempt — exempt means permanently blind); SHRINKAGE needs no ack while growth does (gating shrinkage would punish what the ratchet wants); and a STALE ack is a hard failure — but ONLY for an ack THIS diff wrote or reworded (#2789). An ack is SCOPED TO THE DIFF THAT INTRODUCED IT: `diffEmitted` takes the document at the base ref (`baseAck`, read by `readAckFileAtRef`) alongside the working-tree one, and an entry already present at the base is SPENT — its ripple is absorbed into the base, so it can no longer clear a delta and is never reported stale (surfaced as `spentAcks`, informational, gating nothing). Before #2789 the ack set was the one ABSOLUTE input to an otherwise base-relative machine — `baseline` vs `current`, `changedPaths` from `git diff base...HEAD` — and that mismatch made a MERGED ack indistinguishable from one that never explained anything, since `staleAcks` asks only "did a delta consume you?": merging an ack the PR lane had already accepted reddened `next` and every PR branching off it (#2768). Making spent entries inert is also what finally closes the pre-clearing hazard the original design NAMED but could not prevent — a leftover ack used to silently clear the next ripple on its path; now that ripple must be explained on its own terms, and a reworded reason is how a contributor re-arms an ack deliberately. `baseAck` is REQUIRED once an ack DECLARES ENTRIES (omission is an error, never a silent "inherit nothing", same discipline as `changedPaths`; an entry is the only thing that can be misclassified, so an empty-but-legal document needs no base side). Absent AT THE REF returns null — the healthy steady state — but every other read failure THROWS, and that asymmetry is load-bearing in the direction that is easy to invert: returning null looks armed because every entry stays LIVE, yet a live entry's defining power is that it CONSUMES a delta, so null is armed on the staleness axis and DISARMED on the consumption axis — a genuinely new unexplained ripple on a path carrying an already-merged ack would come back `acked` instead of `unattributable`, silently restoring the whole pre-#2789 gate. `git show` cannot tell absence from fault (both say "does not exist in"), so absence is established with `ls-tree`. Re-arming a spent ack is legitimate and deliberate, but it costs ACTUAL PROSE: the comparison collapses internal whitespace and ignores `runtime`, because a doubled space or a decorative field would otherwise re-arm an ack whose recorded justification still describes the PREVIOUS ripple, showing a reviewer nothing new in the diff. Because a corrupt document ON THE BASE is expensive (it reds every PR carrying an ack until repaired), `scripts/lint-emitted-drift-ack.cjs` runs in `lint:ci` and refuses the merge before one can land — invalid JSON, a non-object, a bad version, a reasonless entry, or a present-but-entryless/`null` document. It is deliberately STANDALONE rather than importing `parseAck` (`scripts/` ships in the npm package and `tests/` does not, so the require would be MODULE_NOT_FOUND once published); the duplication is bounded by a parity test that runs both surfaces over one corpus and fails on any disagreement about schema validity. The two are MEANT to differ on exactly one axis: an entryless or `null` document is legal to PARSE (it is the gate's own absent-equals-no-acks sentinel) and still refused for COMMIT. Deadlock is separately foreclosed at the call site — a tree carrying no ack never reads the base at all, so the PR that DELETES a corrupt file still lands. `tests/emitted-drift-ack.json` (absent = no acks; a LIVE entry is the alarm, a spent one is inert cruft) requires a non-empty `reason` per path — "name them and say why" is the contract, and a document that parses but is not an object is rejected rather than read as "no acks", which would silently disarm the gate. Baseline is CACHED not committed, keyed on the `next` sha; a stale key is REFUSED, never used — absence fails loudly and gets fixed, whereas staleness produces a confident wrong answer. An explicitly-pointed-at (`GSD_EMITTED_BASELINE`) stale baseline is a hard stop, while a stale CACHE falls through to the in-job build. No baseline-unavailable path may `return` (in `node:test` that is a PASS, not a skip — ADR-2719 §6). Supersedes ADR-2264 §2–§4 and its Amendment; ADR-2264 Phase 1 (`buildParityManifest` and the exclusion constants in `tests/helpers/install-shared.cjs`) is retained and depended upon. +Cross-seam principle (ADR-2719, epic #2719): a committed artifact that is a pure function of the source tree is not reviewable state — it is derived state wearing a review costume, and it must be *attributable* rather than *pinned*. Concept, not a Module: it ships nothing, so it takes no `Module` suffix (follows the `### Resolution Provenance` precedent). Scope is the emitted-artifact family named by `RULESET.EMITTED_ATTRIBUTION`. The principle: every emitted path whose hash moved between `next` HEAD and PR HEAD must be attributable — through a declarative provenance table — to a path the pull request actually changed; unattributable deltas are a hard failure that *names them* rather than an anomaly a reviewer must notice inside 7,500 lines of hex. Totality is enforced, so an emitted path matching no rule fails loudly instead of passing through unattributed. The escape hatch is a committed acknowledgment — a per-PR fragment under `tests/emitted-drift-acks/` (#2914; the legacy single `tests/emitted-drift-ack.json` is still read and unioned in for pre-#2914 branches, with a duplicate key across two sources a hard, loudly-reported error rather than silent last-wins), deliberately not a flag or env var — a fragment appears in the changed-files list ONLY when something rippled unexpectedly, so adding one IS the alarm, whereas today 100% of emitted-byte changes touch fixtures and touching them signals nothing. Fragments exist because the single legacy file, rewritten wholesale by every PR needing an ack, was a guaranteed merge-conflict cell between any two such PRs (5 of 6 conflicting PRs in one open queue collided on it and nothing else) — the same shape `.changeset/` already solves the same way. The same differential machine carries the size ratchet: growth is reported with exact byte deltas and needs the same acknowledgment, so anti-creep survives without pinning a number. Distinguish from the absolute check that remains: `tests/fixtures/install-tree/*.json` stays committed and normally-merged (ADR-2719 §7) because "the installer stopped shipping X" must fail with no attribution reasoning involved. Delivery was phased — #2721 naming + interim merge relief, #2722 provenance table + totality guard, #2723 differential check dual-run beside `golden-install-parity.test.cjs`, #2724 cutover (COMPLETE: the dual-run window observed agreement on real PRs after fixing #2750/#2760, and the golden fixtures/test/generator/merge-driver bridge are now deleted; the differential is the sole gate). The table LANDED in #2722 as `tests/helpers/emitted-provenance.cjs` (19 rules, guarded by `tests/emitted-provenance.test.cjs`); it maps emitted path → repo source and is TOTAL over EVERY emitted path in all 19 manifests — exactly one rule per path, with zero-match, two-match, AND dead-rule (a rule matching nothing) all hard failures, so table rot is loud in both directions. Deliberately NO path/family counts are recorded here: those move with every shipped-content edit, and a hand-maintained number in glossary canon is the exact silent-drift failure this whole seam exists to end. The guard recomputes them from the fixtures on every run — read them from a failure message, never from prose. What IS stable is the rule count, which changes only when a new emitted family or host appears. Note the surface is materially wider than #2722 estimated from `claude.json` alone (its "13 families / 15-20 rules" was a single-runtime sample; the 19-manifest surface spans runtime-specific roots — `.agents/`, `.kimi/hooks/`, `command/`, `agents/subagents/`, `.clinerules/`, `plugins/`, `extensions/`, `.gsd/`, the hermes `skills/gsd/` category and the #69 nested `skills//skills//` layout). Two design invariants carry forward to #2723: emitted SHAPES are hard-coded (deriving them from the installer would make the guard tautological — it would follow any installer change silently), while source PATHS may read a first-party descriptor where that descriptor is the sole declaration (`hostBehaviors.nativePlugin.source`); and attribution is keyed on `(rel, runtime)`, never `rel` alone, because one emitted path has different sources per host (`plugins/gsd-core.js` ← `.opencode/` vs `.kilo/`). Emitted skills attribute to `commands/gsd/*.md`, NEVER the repo `skills/` dir — that dir is itself generated from `commands/gsd` by `scripts/gen-plugin-skills.cjs`, so attributing to it is false attribution that still passes totality. Totality does NOT catch a rule pointing at the WRONG source (the recorded residual); the guard against that is the companion assertion that every attributed source EXISTS in the repo, which caught three real cases while the table was built (Copilot's `.agent.md` rename, Kimi's code-literal `agents/gsd.{yaml,md}` root agent, and Copilot's `hooks/gsd-session.json`). A `sources` entry ending in `/` is a PREFIX, not a file — and prefix matching is SEGMENT-AWARE, so a source of `agents/` must not attribute `agentsfoo/x.md`. The differential check LANDED in #2723 as `tests/helpers/emitted-diff.cjs` (the conservation law, a PURE function — no fs/git/installer/clock) + `tests/helpers/emitted-baseline.cjs` (baseline resolution), guarded by `tests/emitted-attribution.test.cjs`. It ran DUAL beside `golden-install-parity.test.cjs` through the #2723 dual-run window with both green and fixtures untouched; #2724 deleted the golden fixtures/test/generator and the check is now the sole gate. Purity is deliberate and load-bearing: the naive one-big-integration-test shape would need ~38 installer spawns per assertion, so the four failing-first criteria would not in practice get written — which is exactly how a phase ships promised-but-not-built. Buckets are CONSERVED: every moved emitted path lands in exactly one of `attributed | unattributable | acked` (property-tested), and a path the provenance table cannot resolve surfaces as an ERROR rather than a silent skip. Four asymmetries worth knowing: an ADDED emitted key is a ripple too (not just modified ones); `synthesized` paths are exempt but `code-derived` ones are NOT (that is why Phase 2 refused to mark them exempt — exempt means permanently blind); SHRINKAGE needs no ack while growth does (gating shrinkage would punish what the ratchet wants); and a STALE ack is a hard failure — but ONLY for an ack THIS diff wrote or reworded (#2789). An ack is SCOPED TO THE DIFF THAT INTRODUCED IT: `diffEmitted` takes the document at the base ref (`baseAck`, read by `readAckFileAtRef`) alongside the working-tree one, and an entry already present at the base is SPENT — its ripple is absorbed into the base, so it can no longer clear a delta and is never reported stale (surfaced as `spentAcks`, informational, gating nothing). Before #2789 the ack set was the one ABSOLUTE input to an otherwise base-relative machine — `baseline` vs `current`, `changedPaths` from `git diff base...HEAD` — and that mismatch made a MERGED ack indistinguishable from one that never explained anything, since `staleAcks` asks only "did a delta consume you?": merging an ack the PR lane had already accepted reddened `next` and every PR branching off it (#2768). Making spent entries inert is also what finally closes the pre-clearing hazard the original design NAMED but could not prevent — a leftover ack used to silently clear the next ripple on its path; now that ripple must be explained on its own terms, and a reworded reason is how a contributor re-arms an ack deliberately. `baseAck` is REQUIRED once an ack DECLARES ENTRIES (omission is an error, never a silent "inherit nothing", same discipline as `changedPaths`; an entry is the only thing that can be misclassified, so an empty-but-legal document needs no base side). Absent AT THE REF returns null — the healthy steady state — but every other read failure THROWS, and that asymmetry is load-bearing in the direction that is easy to invert: returning null looks armed because every entry stays LIVE, yet a live entry's defining power is that it CONSUMES a delta, so null is armed on the staleness axis and DISARMED on the consumption axis — a genuinely new unexplained ripple on a path carrying an already-merged ack would come back `acked` instead of `unattributable`, silently restoring the whole pre-#2789 gate. `git show` cannot tell absence from fault (both say "does not exist in"), so absence is established with `ls-tree`. Re-arming a spent ack is legitimate and deliberate, but it costs ACTUAL PROSE: the comparison collapses internal whitespace and ignores `runtime`, because a doubled space or a decorative field would otherwise re-arm an ack whose recorded justification still describes the PREVIOUS ripple, showing a reviewer nothing new in the diff. Because a corrupt document ON THE BASE is expensive (it reds every PR carrying an ack until repaired), `scripts/lint-emitted-drift-ack.cjs` runs in `lint:ci` and refuses the merge before one can land — invalid JSON, a non-object, a bad version, a reasonless entry, or a present-but-entryless/`null` document. It is deliberately STANDALONE rather than importing `parseAck` (`scripts/` ships in the npm package and `tests/` does not, so the require would be MODULE_NOT_FOUND once published); the duplication is bounded by a parity test that runs both surfaces over one corpus and fails on any disagreement about schema validity. The two are MEANT to differ on exactly one axis: an entryless or `null` document is legal to PARSE (it is the gate's own absent-equals-no-acks sentinel) and still refused for COMMIT. Deadlock is separately foreclosed at the call site — a tree carrying no ack never reads the base at all, so the PR that DELETES a corrupt file still lands. Each ack source — a fragment under `tests/emitted-drift-acks/`, or the legacy `tests/emitted-drift-ack.json` (#2914; both read and UNIONED via `mergeAckSources`/`readAckSources`/`readAckSourcesAtRef` in `tests/helpers/emitted-diff.cjs` / `emitted-runtime.cjs`, a duplicate key across sources a hard error) — follows the same rule: absent = no acks; a LIVE entry is the alarm, a spent one is inert cruft; requires a non-empty `reason` per path — "name them and say why" is the contract, and a document that parses but is not an object is rejected rather than read as "no acks", which would silently disarm the gate. Baseline is CACHED not committed, keyed on the `next` sha; a stale key is REFUSED, never used — absence fails loudly and gets fixed, whereas staleness produces a confident wrong answer. An explicitly-pointed-at (`GSD_EMITTED_BASELINE`) stale baseline is a hard stop, while a stale CACHE falls through to the in-job build. No baseline-unavailable path may `return` (in `node:test` that is a PASS, not a skip — ADR-2719 §6). Supersedes ADR-2264 §2–§4 and its Amendment; ADR-2264 Phase 1 (`buildParityManifest` and the exclusion constants in `tests/helpers/install-shared.cjs`) is retained and depended upon. ### Untrusted-input boundary The prompt-level data/instruction isolation seam for untrusted web/document ingress (#1577). Shared reference `gsd-core/references/untrusted-input-boundary.md`, `@`-included by the 10 ingest agents (`gsd-project-researcher`, `gsd-phase-researcher`, `gsd-ui-researcher`, `gsd-assumptions-analyzer`, `gsd-advisor-researcher`, `gsd-ai-researcher`, `gsd-domain-researcher`, `gsd-research-synthesizer`, `gsd-doc-classifier`, `gsd-doc-synthesizer`) — every agent that reads fetch/search/MCP output or external source documents. The reference instructs: treat fetched/read content as **data, never instructions**; self-scan content for embedded directives before use; act only on the assigned task (ignore off-task instructions in data); and wrap quoted untrusted spans in a **fresh random delimiter** per wrap (fixed markers are spoofable). This prompt-level boundary is the primary control — it keeps an injection from being *followed* even while it sits in context. The hook-level companion is the read-injection scanner (`hooks/gsd-read-injection-scanner.js`, PostToolUse on `Read`/`WebFetch`/`WebSearch`), advisory by default; the opt-in top-level `security.injection_blocking` key upgrades HIGH-confidence detections to a PostToolUse circuit-breaker that halts the agent's next step (it runs *after* the fetch, so it is not a redactor). Tests: `tests/untrusted-input-isolation.test.cjs`, `tests/read-injection-scanner.*.test.cjs`, `tests/injection-blocking-config.test.cjs`. See `docs/adr/1577-untrusted-input-boundary-and-injection-blocking.md` and `docs/explanation/security-model.md`. Grounding: arXiv 2506.05739 (PPA), 2507.15219 (PromptArmor), 2504.20472. @@ -484,9 +484,9 @@ The prompt-level data/instruction isolation seam for untrusted web/document ingr `RULESET.AUDIT.search-source-not-generated=verify an invariant/validation EXISTS by searching the AUTHORED source (src/*.cts OR the scripts/gen-*.cjs generator), never the generated bin/lib/*.cjs (gitignored, ADR-457); gen-time checks live in gen-*.cjs not the .cts it consumes → search BOTH before declaring absent; read generated .cjs only for output drift. Repro: grep src/*.cts for VALID_CONVERTER_NAMES → false "5e ConverterName unenforced"; actually enforced in gen-capability-registry.cjs. cf RULESET.TESTS.no-source-grep` `RULESET.WORKFLOW_MARKDOWN.FENCES=preserve opening language fence when editing shell snippets in workflow markdown; malformed fence creates fresh CR threads (MD040)` -`RULESET.WORKFLOW_SIZE_BUDGET=workflow size enforcement (#1074; BYTES not lines per #717; LF-normalized per #683) = differential attribution size ratchet (PRIMARY anti-creep since #2724/ADR-2719 §4: tests/emitted-attribution.test.cjs's real-tree test reports growth in any gsd-core/workflows/*.md with its exact byte delta vs `next`, no committed snapshot, requires a tests/emitted-drift-ack.json entry) + loose tier hard caps (outer red lines, NEVER raised on approach: XL<=98304 / LARGE<=61440 / DEFAULT<=40960) + discuss-phase<32000; a file that grew fails the differential guard — add an ack entry naming the file and reason, justify the growth in the PR (or extract LAZILY-loaded content; eager @-imports don't reduce loaded context); crossing a hard cap means EXTRACT, not bump. The prior per-file baseline (tests/workflow-size-baseline.json, `npm run size:baseline`) is REMOVED by #2724. Its new-file cap (ADR-1610 Decision point 3, un-baselined files <=32768, the Codex anchor) is REVIVED inside the differential's size ratchet itself (`NEW_FILE_CAP` in tests/helpers/emitted-diff.cjs) rather than lost: "not yet baselined" is exactly "present in sizeCurrent, absent from sizeBaseline", a signal the ratchet already computes for its own reasons. NOT ack-able — same as the tier hard caps, the fix is extraction. Narrower than the original: this check cannot see XL/LARGE tiering (tests/workflow-size-budget.test.cjs's classification, invisible to the pure differential module), so a legitimately large NEW file must extract rather than tier in, one release earlier than an existing file would need to — a disclosed, deliberate simplification` -`RULESET.AGENT_SIZE_BUDGET=agent-size-budget (#1074; sibling of WORKFLOW_SIZE_BUDGET; BYTES not lines per #717/#683, rebased from lines in PR 3/3) = differential attribution size ratchet (PRIMARY anti-creep since #2724/ADR-2719 §4, same mechanism and same tests/emitted-drift-ack.json as WORKFLOW_SIZE_BUDGET, scoped to agents/gsd-*.md) + loose tier hard caps (red lines, never raised on approach: XL<=57344 / LARGE<=49152 / DEFAULT<=24576); net-new agents are DEFAULT-tier (no separate new-file cap). Sizes are measured via the shared scripts/workflow-size.cjs measureMdFiles(dir,predicate) counter (tests/helpers/emitted-runtime.cjs's currentSizes() and the guard's own tier-cap checks both import it). A grown agent fails the differential guard — ack + justify, or extract LAZILY to gsd-core/references/. DISTINCT from DEFECT.AGENT-FILE-SIZE-CAP-BREACH (a separate 45K-CHAR extraction-evidence threshold on gsd-planner via planner-decomposition/reachability tests): that guard proves mode-sections were extracted; this one bounds total agent bytes. Two guards, two units (chars vs bytes), two purposes. The prior per-file baseline (tests/agent-size-baseline.json, `npm run size:baseline`) is REMOVED by #2724` -`RULESET.EMITTED_ATTRIBUTION=the emitted-artifact family (ADR-2719, epic #2719) — POST-CUTOVER (#2724, Phase 4). Historically tests/fixtures/golden-install-parity/*.json (19 path→hash manifests) + tests/workflow-size-baseline.json + tests/agent-size-baseline.json were all committed, PURE FUNCTIONS of the source tree whose correct merge was ALWAYS "recompute" — 140 of 143 conflicted-file instances across the open PR queue were these files. #2724 DELETES all three, the golden test (tests/golden-install-parity.test.cjs), the generator (scripts/gen-golden-install-parity-zcode.cjs), `npm run gen:golden`, `UPDATE_GOLDEN`, the merge-driver bridge (scripts/git-merge-regen-driver.cjs, `npm run setup:merge-driver`, the .gitattributes merge=gsd-regen block), and scripts/update-size-baseline.cjs (`npm run size:baseline`). The differential attribution check (tests/emitted-attribution.test.cjs + tests/emitted-provenance.test.cjs) is now the SOLE gate for emitted-artifact propagation AND size growth — no committed artifact, nothing to hand-merge, nothing to regenerate. `npm run regen:derived` still exists for what remains committed and derived: build, registry, ADR index, capability matrix, inventory manifest, manifest versions, and `tests/fixtures/install-tree/*.json` (now `npm run gen:install-tree`, folded into `regen:derived`). tests/fixtures/install-tree/*.json is DELIBERATELY EXCLUDED from the cutover (ADR-2719 §7): it conflicts on 0 of 7, its diffs are readable, and it preserves "the installer stopped shipping X" as a hard absolute failure — capturing it would convert that absolute into an attribution-free auto-resolve. The baseline the differential compares against is now published by `scripts/gen-emitted-baseline.cjs` on every push to `next` (cached, keyed on sha) and restored in PR lanes via `GSD_EMITTED_BASELINE`/`resolveBaseline()` (tests/helpers/emitted-baseline.cjs); a cache miss falls back to an in-job build via a throwaway `git worktree` (tests/helpers/emitted-runtime.cjs's `buildBaselineAtRef`). REMEDIATION IS PART OF THE GATE (#2778): the failure output names its own remedy, because a gate that states a requirement and withholds the means of satisfying it is a maintainer round-trip, not a gate — ADR-2719 §3's "conspicuous declaration" only works if the contributor can discover how to make it. Both failing branches name `tests/emitted-drift-ack.json`, say it may not exist yet (absence is the healthy steady state), print a minimal valid document, and repeat "do NOT regenerate anything" — post-#2724 there is nothing left to regenerate, and hunting for a deleted baseline is the predictable wrong guess. The two branches key on DIFFERENT spaces and each says which: the hash pass keys on the EMITTED PATH (always contains a `/`), the size ratchet keys on the BARE FILENAME (`currentSizes` writes `sizes[entry.name]` from readdirSync over `gsd-core/workflows/` + `agents/`). A stale-ack failure additionally says to delete the FILE when removing its last entry, since an empty-but-present ack parses fine yet signals nothing; post-#2789 it also offers CORRECTING the entry to name the ripple actually made, which is the other honest resolution and the one a contributor usually wants. NOT ack-able and deliberately given no ack text: the `NEW_FILE_CAP` branch, whose remedy is extraction. Text is sourced from one frozen `REMEDIATION` export in tests/helpers/emitted-diff.cjs whose example document is rendered from `ACK_VERSION` via `JSON.stringify`, so the taught schema cannot drift from the accepted one (a round-trip test feeds the printed document back through `parseAck`); the message teaches ONE canonical shape even though `parseAck` also accepts a bare-string reason and a missing `version` — liberal in what it accepts, conservative in what it sends. Note the ADR's Consequences originally called the #2724 migration "terminal"; #2778 corrected that — it is terminal only for a PR that grows no shipped file. cf `RULESET.WORKFLOW_SIZE_BUDGET`, `RULESET.AGENT_SIZE_BUDGET`; see `### Emitted Artifact Provenance`` +`RULESET.WORKFLOW_SIZE_BUDGET=workflow size enforcement (#1074; BYTES not lines per #717; LF-normalized per #683) = differential attribution size ratchet (PRIMARY anti-creep since #2724/ADR-2719 §4: tests/emitted-attribution.test.cjs's real-tree test reports growth in any gsd-core/workflows/*.md with its exact byte delta vs `next`, no committed snapshot, requires an ack entry — a fragment under tests/emitted-drift-acks/, #2914; the legacy tests/emitted-drift-ack.json is still honored and unioned in) + loose tier hard caps (outer red lines, NEVER raised on approach: XL<=98304 / LARGE<=61440 / DEFAULT<=40960) + discuss-phase<32000; a file that grew fails the differential guard — add an ack entry naming the file and reason, justify the growth in the PR (or extract LAZILY-loaded content; eager @-imports don't reduce loaded context); crossing a hard cap means EXTRACT, not bump. The prior per-file baseline (tests/workflow-size-baseline.json, `npm run size:baseline`) is REMOVED by #2724. Its new-file cap (ADR-1610 Decision point 3, un-baselined files <=32768, the Codex anchor) is REVIVED inside the differential's size ratchet itself (`NEW_FILE_CAP` in tests/helpers/emitted-diff.cjs) rather than lost: "not yet baselined" is exactly "present in sizeCurrent, absent from sizeBaseline", a signal the ratchet already computes for its own reasons. NOT ack-able — same as the tier hard caps, the fix is extraction. Narrower than the original: this check cannot see XL/LARGE tiering (tests/workflow-size-budget.test.cjs's classification, invisible to the pure differential module), so a legitimately large NEW file must extract rather than tier in, one release earlier than an existing file would need to — a disclosed, deliberate simplification` +`RULESET.AGENT_SIZE_BUDGET=agent-size-budget (#1074; sibling of WORKFLOW_SIZE_BUDGET; BYTES not lines per #717/#683, rebased from lines in PR 3/3) = differential attribution size ratchet (PRIMARY anti-creep since #2724/ADR-2719 §4, same mechanism and same ack fragments (tests/emitted-drift-acks/, #2914; legacy tests/emitted-drift-ack.json still honored) as WORKFLOW_SIZE_BUDGET, scoped to agents/gsd-*.md) + loose tier hard caps (red lines, never raised on approach: XL<=57344 / LARGE<=49152 / DEFAULT<=24576); net-new agents are DEFAULT-tier (no separate new-file cap). Sizes are measured via the shared scripts/workflow-size.cjs measureMdFiles(dir,predicate) counter (tests/helpers/emitted-runtime.cjs's currentSizes() and the guard's own tier-cap checks both import it). A grown agent fails the differential guard — ack + justify, or extract LAZILY to gsd-core/references/. DISTINCT from DEFECT.AGENT-FILE-SIZE-CAP-BREACH (a separate 45K-CHAR extraction-evidence threshold on gsd-planner via planner-decomposition/reachability tests): that guard proves mode-sections were extracted; this one bounds total agent bytes. Two guards, two units (chars vs bytes), two purposes. The prior per-file baseline (tests/agent-size-baseline.json, `npm run size:baseline`) is REMOVED by #2724` +`RULESET.EMITTED_ATTRIBUTION=the emitted-artifact family (ADR-2719, epic #2719) — POST-CUTOVER (#2724, Phase 4). Historically tests/fixtures/golden-install-parity/*.json (19 path→hash manifests) + tests/workflow-size-baseline.json + tests/agent-size-baseline.json were all committed, PURE FUNCTIONS of the source tree whose correct merge was ALWAYS "recompute" — 140 of 143 conflicted-file instances across the open PR queue were these files. #2724 DELETES all three, the golden test (tests/golden-install-parity.test.cjs), the generator (scripts/gen-golden-install-parity-zcode.cjs), `npm run gen:golden`, `UPDATE_GOLDEN`, the merge-driver bridge (scripts/git-merge-regen-driver.cjs, `npm run setup:merge-driver`, the .gitattributes merge=gsd-regen block), and scripts/update-size-baseline.cjs (`npm run size:baseline`). The differential attribution check (tests/emitted-attribution.test.cjs + tests/emitted-provenance.test.cjs) is now the SOLE gate for emitted-artifact propagation AND size growth — no committed artifact, nothing to hand-merge, nothing to regenerate. `npm run regen:derived` still exists for what remains committed and derived: build, registry, ADR index, capability matrix, inventory manifest, manifest versions, and `tests/fixtures/install-tree/*.json` (now `npm run gen:install-tree`, folded into `regen:derived`). tests/fixtures/install-tree/*.json is DELIBERATELY EXCLUDED from the cutover (ADR-2719 §7): it conflicts on 0 of 7, its diffs are readable, and it preserves "the installer stopped shipping X" as a hard absolute failure — capturing it would convert that absolute into an attribution-free auto-resolve. The baseline the differential compares against is now published by `scripts/gen-emitted-baseline.cjs` on every push to `next` (cached, keyed on sha) and restored in PR lanes via `GSD_EMITTED_BASELINE`/`resolveBaseline()` (tests/helpers/emitted-baseline.cjs); a cache miss falls back to an in-job build via a throwaway `git worktree` (tests/helpers/emitted-runtime.cjs's `buildBaselineAtRef`). REMEDIATION IS PART OF THE GATE (#2778): the failure output names its own remedy, because a gate that states a requirement and withholds the means of satisfying it is a maintainer round-trip, not a gate — ADR-2719 §3's "conspicuous declaration" only works if the contributor can discover how to make it. Both failing branches name a NEW fragment to create under `tests/emitted-drift-acks/` (#2914; pick a name nobody else is using), say it may not exist yet (absence is the healthy steady state), print a minimal valid document, and repeat "do NOT regenerate anything" — post-#2724 there is nothing left to regenerate, and hunting for a deleted baseline is the predictable wrong guess. The two branches key on DIFFERENT spaces and each says which: the hash pass keys on the EMITTED PATH (always contains a `/`), the size ratchet keys on the BARE FILENAME (`currentSizes` writes `sizes[entry.name]` from readdirSync over `gsd-core/workflows/` + `agents/`). A stale-ack failure additionally says to delete the FILE when removing its last entry, since an empty-but-present ack parses fine yet signals nothing; post-#2789 it also offers CORRECTING the entry to name the ripple actually made, which is the other honest resolution and the one a contributor usually wants. NOT ack-able and deliberately given no ack text: the `NEW_FILE_CAP` branch, whose remedy is extraction. Text is sourced from one frozen `REMEDIATION` export in tests/helpers/emitted-diff.cjs whose example document is rendered from `ACK_VERSION` via `JSON.stringify`, so the taught schema cannot drift from the accepted one (a round-trip test feeds the printed document back through `parseAck`); the message teaches ONE canonical shape even though `parseAck` also accepts a bare-string reason and a missing `version` — liberal in what it accepts, conservative in what it sends. Note the ADR's Consequences originally called the #2724 migration "terminal"; #2778 corrected that — it is terminal only for a PR that grows no shipped file. #2914 replaced the single shared ack file with per-PR fragments under `tests/emitted-drift-acks/` — exactly the shape `.changeset/` already uses for the identical "every PR rewrites one shared document" conflict problem — so two PRs needing an ack can no longer collide with each other, and a fragment left on `next` after merge is inert rather than a shared cell; the legacy file is still read and unioned in for branches that predate the split, and a duplicate path key across two sources is a hard, loudly-reported error, never silent last-wins. `tests/emitted-drift-ack.json` (the LEGACY file specifically, NOT the fragment directory) must NEVER persist on `next` (#2914): every entry is scoped to the diff that introduced it, so once merged it is by definition already at the base — spent and inert regardless of shape — and a persistent copy makes that ONE file a shared merge-conflict cell across every open PR that also carries an ack, exactly the "140 of 143" cost this whole cutover exists to remove; a persisting FRAGMENT is harmless by construction and is deliberately not what this guard checks. This is enforced on `next` itself only, never as a PR-lane check: the `guard-no-ack-on-next` job in `.github/workflows/test.yml` (push-to-`next` trigger) runs `scripts/lint-emitted-drift-ack.cjs --guard-next` (`assertAbsentOnNext`), which fails on the LEGACY file's PRESENCE alone, valid or not — a PR-lane "base ack must be absent" check would red every open PR the instant a spent ack merged, which is the #2768 shape #2789 already ended. cf `RULESET.WORKFLOW_SIZE_BUDGET`, `RULESET.AGENT_SIZE_BUDGET`; see `### Emitted Artifact Provenance`` `RULESET.WORKFLOW_FILE_NAMES=workflow files use hyphens; XML attributes must match (extract-learnings not extract_learnings); tests should pin exact hyphenated name` `RULESET.WORKFLOW_EXECUTION_CONTEXT=@-ref in commands/gsd/*.md must resolve to an existing file on disk; regression test in tests/docs-update.test.cjs (folds former \`bug-3135-capture-backlog-workflow\`, consolidation epic #1969); INVENTORY.md row + INVENTORY-MANIFEST.json families.workflows must stay in sync; "Invoked by" attribution must move when a flag absorbs a micro-skill` `RULESET.WORKFLOW_EXECUTE_END_TO_END=standard for single-workflow commands is "Execute end-to-end." (no bolded **Follow the X workflow** fragments); flag-dispatch routing uses "execute the X workflow end-to-end." in routing bullets — convention verified live across ~20 commands/gsd/*.md files; no ADR currently documents this specific phrasing rule (ADR-0002 covers the adjacent but distinct command-contract/@-ref-resolution seam, not this convention)` diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a5313e912..517c1e49e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -773,23 +773,50 @@ what your PR changed against `next` and requires every emitted-artifact hash tha to be attributable to your diff. If it is not, the check fails and names the paths. Legitimate cases where emitted bytes move for a reason your diff cannot show directly — -a converter change, for example — go through `tests/emitted-drift-ack.json` (name the -path, say why); see `CONTEXT.md`'s `### Emitted Artifact Provenance` entry for the full -model. Growth in a `gsd-core/workflows/*.md` or `agents/gsd-*.md` file is reported with -its exact byte delta and needs the same acknowledgment; the outer tier hard caps in +a converter change, for example — go through a **per-PR fragment** under +`tests/emitted-drift-acks/` (#2914; name the path, say why); see `CONTEXT.md`'s +`### Emitted Artifact Provenance` entry for the full model. Growth in a +`gsd-core/workflows/*.md` or `agents/gsd-*.md` file is reported with its exact byte delta +and needs the same acknowledgment; the outer tier hard caps in `tests/workflow-size-budget.test.cjs` / `tests/agent-size-budget.test.cjs` are unaffected -and still apply. +and still apply. The legacy single `tests/emitted-drift-ack.json` is still read and +unioned in for any branch that still carries it, but new acknowledgments go in a NEW +fragment, never that file. You do not need to memorize any of this. **The failure output names its own remedy** — it -tells you the file to create, that it does not exist yet, which key to use, and prints a -minimal valid document you can paste. Note the two key spaces, because the message says -which one applies: an unattributable **hash** ripple is keyed on the emitted path +tells you to create a new fragment under `tests/emitted-drift-acks/` (with a name nobody +else is using — include your issue or PR number), which key to use, and prints a minimal +valid document you can paste. Note the two key spaces, because the message says which one +applies: an unattributable **hash** ripple is keyed on the emitted path (`skills/gsd-add-tests/SKILL.md`), while **growth** is keyed on the bare filename as it appears under `gsd-core/workflows/` or `agents/` (`explore.md`). When you remove the last -entry from `tests/emitted-drift-ack.json`, delete the file too — its presence is the -alarm, so an empty one signals nothing. Nothing here is regenerated: if you find yourself -looking for a baseline file to re-run a generator over, that file was deleted by #2724 and -is not coming back. +entry from your fragment, delete the fragment file too — its presence is the alarm, so an +empty one signals nothing. Nothing here is regenerated: if you find yourself looking for a +baseline file to re-run a generator over, that file was deleted by #2724 and is not coming +back. + +**Why fragments, not one file (#2914):** every PR needing an acknowledgment used to +rewrite `tests/emitted-drift-ack.json`'s `paths` map wholesale — a single shared mutable +file every such PR touches guarantees a merge conflict between any two of them (5 of 6 +conflicting PRs in one open queue collided on this file and nothing else), and it means +spent, already-merged entries pile up on `next`. A fragment per PR — the same shape +`.changeset/` already uses for the identical problem — means two PRs can never conflict on +this seam again, and a fragment left on `next` after merge is inert rather than a shared +cell. Two ack sources (two fragments, or a fragment and the legacy file) may **never** name +the same path; that is a hard, loudly-reported error, not a silent last-wins. + +`tests/emitted-drift-ack.json` (the legacy single file, specifically — NOT the fragment +directory) must never persist on `next` (#2914): every entry is scoped to the diff that +introduced it, so once merged it is, by definition, already at the base — spent and inert, +regardless of shape, and its persistence is what makes it a shared merge-conflict cell. A +fragment persisting on `next` is harmless, since fragments are independently named and +cannot conflict with anything, so this guard is deliberately scoped to the legacy file +alone. This is enforced only on `next` itself, by the `guard-no-ack-on-next` job in +`.github/workflows/test.yml` (push-to-`next` trigger, +`scripts/lint-emitted-drift-ack.cjs --guard-next`), never as a PR-lane check — a PR-lane +"base ack must be absent" check would red every open PR the moment one landed (the #2768 +shape #2789 exists to prevent). If you ever see the legacy file present on `next`, delete +it; do not try to make it well-formed. `npm run regen:derived` still exists for the artifacts that ARE committed and derived — `sync-manifest-versions`, the ADR index, the capability matrix, the inventory manifest, @@ -929,8 +956,9 @@ gsd-core/ Per-file growth is caught by the differential attribution check (tests/emitted-attribution.test.cjs, ADR-2719) — it reports the exact byte delta and - requires an entry in tests/emitted-drift-ack.json, - no committed snapshot to regenerate. Loose tier + requires a per-PR fragment in + tests/emitted-drift-acks/ (#2914), no committed + snapshot to regenerate. Loose tier hard caps remain in tests/workflow-size-budget.test.cjs. The same applies to agent files (agents/gsd-*.md, tests/agent-size-budget.test.cjs). Full how-to + diff --git a/scripts/lint-emitted-drift-ack.cjs b/scripts/lint-emitted-drift-ack.cjs index 56be608ea..6c88aedf0 100644 --- a/scripts/lint-emitted-drift-ack.cjs +++ b/scripts/lint-emitted-drift-ack.cjs @@ -21,6 +21,13 @@ * here would be a MODULE_NOT_FOUND in the published package. The duplication is bounded * by a parity test — `tests/emitted-attribution.test.cjs` runs both surfaces over one * corpus and fails if they ever disagree about what is schema-valid. + * + * #2914: the single shared `tests/emitted-drift-ack.json` is replaced by per-PR + * fragments under `tests/emitted-drift-acks/` (kept alongside the legacy file, which is + * still honored). This validator now checks BOTH: every physical source is run through + * the same schema/policy rules below, and — because two sources are never allowed to + * name the same path (silent last-wins would resurrect exactly the silent-drift class + * the ack seam exists to end) — a cross-source duplicate key is ALSO a hard failure. */ const fs = require('node:fs'); @@ -28,10 +35,36 @@ const path = require('node:path'); const ACK_VERSION = 1; const ACK_REPO_PATH = 'tests/emitted-drift-ack.json'; +const ACK_DIR_REPO_PATH = 'tests/emitted-drift-acks'; const REPO_ROOT = path.join(__dirname, '..'); +/** + * Upper bound on how many fragment files `listFragmentFiles` may return in one + * `readdirSync` pass. Mirrors `MAX_ACK_FRAGMENTS` in `tests/helpers/emitted-diff.cjs` — + * DUPLICATED rather than imported, because `scripts/` ships in the npm package and + * `tests/` does not (requiring across that line would be MODULE_NOT_FOUND once + * published; see this file's top-of-file comment). The two are held to the same value + * by the schema-parity test in `tests/emitted-attribution.test.cjs`. + * + * Exceeding it throws rather than truncating: a truncated listing would silently drop + * acknowledgments from consideration, which is exactly the class of silent failure this + * whole ack seam exists to prevent. + */ +const MAX_ACK_FRAGMENTS = 500; + const isPlainObject = (v) => v !== null && typeof v === 'object' && !Array.isArray(v); +/** + * Key names that can never be a legitimate emitted path or bare workflow/agent filename + * (`__proto__`, `constructor`, `prototype`). Duplicated (not imported) in + * `tests/helpers/emitted-diff.cjs`'s `parseAck` for the same reason every other constant + * here is duplicated rather than required — `scripts/` ships, `tests/` does not. Held to + * the same set by the schema-parity test in `tests/emitted-attribution.test.cjs`, which + * must see BOTH surfaces reject a document naming one of these, never one silently + * accepting what the other errors on (#2914 review). + */ +const RESERVED_ACK_KEYS = new Set(['__proto__', 'constructor', 'prototype']); + /** * Validate an ack document's raw text. * @@ -42,9 +75,13 @@ const isPlainObject = (v) => v !== null && typeof v === 'object' && !Array.isArr * rather than left behind. * * @param {string|null} raw file contents, or null when the file is absent + * @param {object} [opts] + * @param {string} [opts.source] the path used in error messages (default: the legacy + * file). Generalized (#2914) so the same rules apply verbatim to a fragment under + * `ACK_DIR_REPO_PATH` — one definition of "valid", named per the file it is checking. * @returns {{ schemaErrors: string[], policyErrors: string[], ok: boolean }} */ -function validateAckText(raw) { +function validateAckText(raw, { source = ACK_REPO_PATH } = {}) { const schemaErrors = []; const policyErrors = []; const done = () => ({ schemaErrors, policyErrors, ok: schemaErrors.length === 0 && policyErrors.length === 0 }); @@ -52,7 +89,7 @@ function validateAckText(raw) { if (raw === null) return done(); // absent is the healthy steady state if (raw.trim() === '') { - schemaErrors.push(`${ACK_REPO_PATH} is present but empty`); + schemaErrors.push(`${source} is present but empty`); return done(); } @@ -60,7 +97,7 @@ function validateAckText(raw) { try { doc = JSON.parse(raw); } catch (err) { - schemaErrors.push(`${ACK_REPO_PATH} is not valid JSON: ${err.message}`); + schemaErrors.push(`${source} is not valid JSON: ${err.message}`); return done(); } @@ -70,7 +107,7 @@ function validateAckText(raw) { // it declares nothing, so the remedy is the same as an entryless document. if (doc === null) { policyErrors.push( - `${ACK_REPO_PATH} contains "null" and declares no acknowledgments. Delete the file — ` + `${source} contains "null" and declares no acknowledgments. Delete the file — ` + 'the healthy steady state is no file at all.', ); return done(); @@ -78,28 +115,41 @@ function validateAckText(raw) { if (!isPlainObject(doc)) { schemaErrors.push( - `${ACK_REPO_PATH}: must be a JSON object, got ${Array.isArray(doc) ? 'array' : typeof doc}`, + `${source}: must be a JSON object, got ${Array.isArray(doc) ? 'array' : typeof doc}`, ); return done(); } if (doc.version !== undefined && doc.version !== ACK_VERSION) { schemaErrors.push( - `${ACK_REPO_PATH}: unsupported version ${JSON.stringify(doc.version)} (expected ${ACK_VERSION})`, + `${source}: unsupported version ${JSON.stringify(doc.version)} (expected ${ACK_VERSION})`, ); } const paths = doc.paths; if (paths !== undefined && !isPlainObject(paths)) { - schemaErrors.push(`${ACK_REPO_PATH}: "paths" must be an object of -> { reason }`); + schemaErrors.push(`${source}: "paths" must be an object of -> { reason }`); return done(); } const entries = paths === undefined ? [] : Object.entries(paths); for (const [rel, value] of entries) { + if (RESERVED_ACK_KEYS.has(rel)) { + // Reject loudly rather than silently filter. Previously this key was excluded + // only from `declaredKeys`'s duplicate-detection view, so a document naming it + // passed validation here while the gate's `parseAck` (fed the JSON.parse'd + // document, where such a key is a genuine own property) either mishandled it or + // disagreed silently — two surfaces reaching different verdicts on the same + // document (#2914 review). Recognizably the same finding as `parseAck`'s. + schemaErrors.push( + `${source}: ack key "${rel}" is reserved and can never be a valid emitted path ` + + 'or workflow/agent filename — remove it', + ); + continue; + } const reason = isPlainObject(value) ? value.reason : value; if (typeof reason !== 'string' || reason.trim() === '') { - schemaErrors.push(`${ACK_REPO_PATH}: ack for "${rel}" has no non-empty "reason"`); + schemaErrors.push(`${source}: ack for "${rel}" has no non-empty "reason"`); } } @@ -108,7 +158,7 @@ function validateAckText(raw) { // behind after removing the last entry by hand. if (entries.length === 0) { policyErrors.push( - `${ACK_REPO_PATH} is present but declares no acknowledgments. Delete the file — an ` + `${source} is present but declares no acknowledgments. Delete the file — an ` + 'empty one signals nothing, and the healthy steady state is no file at all.', ); } @@ -120,30 +170,175 @@ function readIfPresent(file) { return fs.existsSync(file) ? fs.readFileSync(file, 'utf8') : null; } -function main() { - const file = path.join(REPO_ROOT, ...ACK_REPO_PATH.split('/')); - const result = validateAckText(readIfPresent(file)); - const all = [...result.schemaErrors, ...result.policyErrors]; +/** + * Fragment filenames under `dir`, sorted. Absent directory == zero fragments. + * + * Fails loudly, naming `dir`, the cap, and the actual count, when the directory holds + * more than `MAX_ACK_FRAGMENTS` entries — never silently truncates the listing. + */ +function listFragmentFiles(dir) { + if (!fs.existsSync(dir)) return []; + const names = fs.readdirSync(dir).filter((name) => name.endsWith('.json')).sort(); + if (names.length > MAX_ACK_FRAGMENTS) { + throw new Error( + `lint-emitted-drift-ack: ${dir} contains ${names.length} ack fragments, exceeding ` + + `the cap of ${MAX_ACK_FRAGMENTS}. Refusing to read only some of them — a truncated ` + + 'read would silently drop acknowledgments. Prune spent fragments from this directory.', + ); + } + return names; +} - if (all.length) { - console.error(`lint-emitted-drift-ack: ${all.length} problem(s) in ${ACK_REPO_PATH}\n`); - for (const e of all) console.error(` - ${e}`); +/** + * The path keys a document declares, for cross-source collision detection — but ONLY + * when the document is itself trustworthy. A document that failed its own schema check + * must not also seed a bogus "collision" derived from garbage; its own error already + * blocks the merge, and reporting a fabricated collision on top would confuse rather + * than clarify. `RESERVED_ACK_KEYS` are also excluded here — they can never be a + * legitimate duplicate, since they can never be a legitimate key at all — but this is + * belt-and-suspenders, not the enforcement point: `validateAckText` above now rejects any + * document naming one outright, so `main()` only ever calls this on a document whose + * schema already checked out, making the exclusion below unreachable in practice. + */ +function declaredKeys(raw) { + if (raw === null) return []; + let doc; + try { + doc = JSON.parse(raw); + } catch { + return []; + } + if (!isPlainObject(doc)) return []; + const paths = doc.paths; + if (paths === undefined) return []; + if (!isPlainObject(paths)) return []; + return Object.keys(paths).filter((k) => !RESERVED_ACK_KEYS.has(k)); +} + +/** + * assertAbsentOnNext — the `next`-lane guard (#2914), invoked only by the + * `guard-no-ack-on-next` workflow job on push to `next`, never in `lint:ci`. + * + * `validateAckText` lints SHAPE, because a PR's own working tree may legitimately carry + * a live, well-formed ack — that is the normal case a PR-lane check must allow. This + * function instead rejects PRESENCE outright, valid or not: per the ack-lifecycle law + * (#2789, `RULESET.EMITTED_ATTRIBUTION`), an entry already at the base is spent the + * moment it merges, so a document surviving on `next` is inert cruft by definition, not + * a thing to schema-check. + * + * This MUST NOT run as a PR-lane check comparing a PR against `next` — that is the #2768 + * shape #2789 exists to prevent (a spent-but-present base ack would red every open PR the + * instant one landed). It is safe only because it runs on `next` itself, asserting a fact + * about `next`'s own tree, never about any PR's diff against it. + * + * @param {boolean} present whether ACK_REPO_PATH exists in the tree being checked + * @returns {{ ok: boolean, message: string }} + */ +function assertAbsentOnNext(present) { + if (!present) { + return { ok: true, message: `ok guard-no-ack-on-next: ${ACK_REPO_PATH} is absent (the healthy steady state)` }; + } + return { + ok: false, + message: [ + `guard-no-ack-on-next: ${ACK_REPO_PATH} exists on next.`, + '', + 'Every entry in this file is scoped to the diff that introduced it (#2789). Once merged ' + + 'to next it is, by definition, already at the base -- spent and inert, regardless of ' + + 'whether it is otherwise well-formed.', + '', + '#2914: acks now go in per-PR fragments under tests/emitted-drift-acks/, one file per ' + + 'PR, never this single shared file -- a persistent fragment there is harmless (every ' + + 'fragment is independently named, so it cannot conflict with any other PR), which is ' + + 'why only THIS legacy file is guarded here, never the fragment directory.', + '', + 'CONTRIBUTING.md: "When you remove the last entry from tests/emitted-drift-ack.json, ' + + 'delete the file too -- its presence is the alarm."', + '', + `Remedy: git rm ${ACK_REPO_PATH}`, + ].join('\n'), + }; +} + +function main() { + const legacyFile = path.join(REPO_ROOT, ...ACK_REPO_PATH.split('/')); + + if (process.argv.includes('--guard-next')) { + const result = assertAbsentOnNext(fs.existsSync(legacyFile)); + console.log(result.message); + if (!result.ok) process.exitCode = 1; + return; + } + + const fragmentsDir = path.join(REPO_ROOT, ...ACK_DIR_REPO_PATH.split('/')); + const sources = [ + { label: ACK_REPO_PATH, raw: readIfPresent(legacyFile) }, + ...listFragmentFiles(fragmentsDir).map((name) => ({ + label: `${ACK_DIR_REPO_PATH}/${name}`, + raw: readIfPresent(path.join(fragmentsDir, name)), + })), + ]; + + const problems = []; + const owner = new Map(); // path key -> the source label that already claimed it + let anyPresent = false; + + for (const { label, raw } of sources) { + if (raw !== null) anyPresent = true; + + // `validateAckText` already prefixes every message with `source` (== `label`), so + // these are pushed verbatim rather than re-prefixed — a second prefix would read as + // "tests/emitted-drift-acks/x.json: tests/emitted-drift-acks/x.json is not valid + // JSON", naming the same file twice for no reason. + const result = validateAckText(raw, { source: label }); + problems.push(...result.schemaErrors, ...result.policyErrors); + + // Only chase collisions across documents whose OWN schema already checked out — + // a document we could not trust must not also seed a fabricated collision. + if (result.schemaErrors.length === 0) { + for (const key of declaredKeys(raw)) { + if (owner.has(key)) { + problems.push( + `duplicate ack for "${key}": declared in both ${owner.get(key)} and ${label}. ` + + 'Two ack sources (fragments, or a fragment and the legacy file) may never ' + + 'name the same path — rename or merge them.', + ); + continue; + } + owner.set(key, label); + } + } + } + + if (problems.length) { + console.error(`lint-emitted-drift-ack: ${problems.length} problem(s)\n`); + for (const e of problems) console.error(` - ${e}`); console.error( '\nThis blocks the merge on purpose. The base-side reader fails loudly on a document ' + 'it cannot parse, so a broken one on the base branch reds every PR that carries an ' - + 'acknowledgment. Fix or delete the file here, where it is cheap.', + + 'acknowledgment, and a duplicate across two sources is exactly the silent-drift class ' + + 'the ack seam exists to end. Fix or delete the offending source(s) here, where it is cheap.', ); process.exitCode = 1; return; } console.log( - fs.existsSync(file) - ? `ok lint-emitted-drift-ack: ${ACK_REPO_PATH} is well-formed` - : `ok lint-emitted-drift-ack: ${ACK_REPO_PATH} absent (the healthy steady state)`, + anyPresent + ? 'ok lint-emitted-drift-ack: all acknowledgment sources are well-formed' + : 'ok lint-emitted-drift-ack: no acknowledgment sources present (the healthy steady state)', ); } if (require.main === module) main(); -module.exports = { validateAckText, ACK_VERSION, ACK_REPO_PATH }; +module.exports = { + validateAckText, + assertAbsentOnNext, + declaredKeys, + listFragmentFiles, + ACK_VERSION, + ACK_REPO_PATH, + ACK_DIR_REPO_PATH, + MAX_ACK_FRAGMENTS, +}; diff --git a/tests/emitted-attribution.test.cjs b/tests/emitted-attribution.test.cjs index f05ccff72..5c66cd9b6 100644 --- a/tests/emitted-attribution.test.cjs +++ b/tests/emitted-attribution.test.cjs @@ -33,6 +33,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); +const os = require('node:os'); const path = require('node:path'); const crypto = require('node:crypto'); const { execFileSync } = require('node:child_process'); @@ -49,7 +50,13 @@ const { currentSizes, readAckFile, readAckFileAtRef, + readAckSources, + readAckSourcesAtRef, + listAckFragmentFiles, + listAckFragmentFilesAtRef, ACK_REPO_PATH, + ACK_DIR, + ACK_DIR_REPO_PATH, baselineFamilyNamesAtRef, MANIFEST_FAMILIES, MINIMUM_MANIFEST_FAMILIES, @@ -62,14 +69,21 @@ const { } = require('./helpers/emitted-runtime.cjs'); const { EXPECTED_MANIFEST_COUNT, loadManifests } = require('./helpers/emitted-provenance.cjs'); -const { validateAckText } = require('../scripts/lint-emitted-drift-ack.cjs'); +const { + validateAckText, + assertAbsentOnNext, + MAX_ACK_FRAGMENTS: MAX_ACK_FRAGMENTS_LINT, +} = require('../scripts/lint-emitted-drift-ack.cjs'); const { ACK_VERSION, ACK_FILE, + ACK_DIR: ACK_DIR_PURE, NEW_FILE_CAP, + MAX_ACK_FRAGMENTS, REMEDIATION, sourceSatisfiedBy, parseAck, + mergeAckSources, diffEmitted, buildReport, formatReport, @@ -714,6 +728,13 @@ test('the pre-merge lint and the gate parser agree on what is schema-valid', () ['whitespace reason', '{"version":1,"paths":{"a.md":{"reason":" "}}}', false], ['missing reason', '{"version":1,"paths":{"a.md":{}}}', false], ['numeric reason', '{"version":1,"paths":{"a.md":42}}', false], + // #2914 review: a `__proto__`/`constructor`/`prototype` key is a genuine OWN key on + // the production path (`JSON.parse`, unlike a JS object literal), and both surfaces + // must reject it outright rather than one silently filtering it and the other + // erroring or mishandling it. + ['reserved key __proto__', '{"version":1,"paths":{"__proto__":{"reason":"ok"}}}', false], + ['reserved key constructor', '{"version":1,"paths":{"constructor":{"reason":"ok"}}}', false], + ['reserved key prototype', '{"version":1,"paths":{"prototype":{"reason":"ok"}}}', false], ]; for (const [label, raw, expectedValid] of corpus) { @@ -737,6 +758,48 @@ test('the pre-merge lint and the gate parser agree on what is schema-valid', () } }); +test('a __proto__/constructor/prototype ack key is rejected loudly by both surfaces, never silently dropped (#2914 review)', () => { + // Built via JSON.parse — the production path — so the key is a genuine OWN property, + // never the JS object-literal special case (`{__proto__: v}` sets the prototype and + // yields zero own keys, which is what made the pre-fix regression test vacuous). + for (const key of ['__proto__', 'constructor', 'prototype']) { + const raw = JSON.stringify({ version: ACK_VERSION, paths: { [key]: { reason: 'hostile' } } }); + const doc = JSON.parse(raw); + assert.deepEqual(Object.keys(doc.paths), [key], `JSON.parse must create a genuine own key for ${key}`); + + const gate = parseAck(doc, { source: 'tests/emitted-drift-acks/1000-a.json' }); + assert.equal(gate.entries.size, 0, `${key} must never become a live ack entry`); + assert.equal(gate.errors.length, 1); + assert.match(gate.errors[0], /reserved/); + assert.match(gate.errors[0], new RegExp(key)); + + const lint = validateAckText(raw, { source: 'tests/emitted-drift-acks/1000-a.json' }); + assert.equal(lint.schemaErrors.length, 1); + assert.match(lint.schemaErrors[0], /reserved/); + assert.match(lint.schemaErrors[0], new RegExp(key)); + + // Recognizably the SAME finding on both surfaces, not merely both non-empty. + assert.equal( + gate.errors[0].replace('tests/emitted-drift-acks/1000-a.json', 'SOURCE'), + lint.schemaErrors[0].replace('tests/emitted-drift-acks/1000-a.json', 'SOURCE'), + `${key}: parseAck and validateAckText must report the same finding`, + ); + + assert.equal(({}).reason, undefined, 'Object.prototype must stay untouched throughout'); + } +}); + +test('the pre-merge lint and the gate helpers agree on the fragment-count cap (#2914 review)', () => { + // Duplicated by necessity (scripts/ ships, tests/ does not — see MAX_ACK_FRAGMENTS's + // doc comment in both files), so this parity test is what keeps the two values from + // silently drifting apart the way the schema rules above are held together. + assert.equal( + MAX_ACK_FRAGMENTS_LINT, MAX_ACK_FRAGMENTS, + 'scripts/lint-emitted-drift-ack.cjs and tests/helpers/emitted-diff.cjs must agree on ' + + 'MAX_ACK_FRAGMENTS', + ); +}); + test('the lint additionally rejects a present-but-entryless document the parser accepts', () => { // This is policy, not schema, and the one place the two surfaces are MEANT to differ: // `parseAck` must treat `{}` as "no acks" (legal) so an absent-equivalent document @@ -772,6 +835,500 @@ test('the lint rejects a present-but-empty file rather than reading it as absent } }); +test('validateAckText names the SOURCE it is checking, not a hardcoded literal (#2914)', () => { + // Generalized so the lint can run the SAME rules over a fragment as over the legacy + // file. A message that hardcoded tests/emitted-drift-ack.json would misname every + // fragment's own errors. + const r = validateAckText('{"version":1,"paths":{"a.md":{"reason":""}}}', { + source: 'tests/emitted-drift-acks/1000-a.json', + }); + assert.match(r.schemaErrors[0], /tests\/emitted-drift-acks\/1000-a\.json/); +}); + +test('declaredKeys: only a schema-trustworthy document contributes keys for collision detection', () => { + const { declaredKeys } = require('../scripts/lint-emitted-drift-ack.cjs'); + assert.deepEqual(declaredKeys(null), []); + assert.deepEqual(declaredKeys('{ not json'), [], 'unparseable JSON contributes no keys'); + assert.deepEqual(declaredKeys('[]'), [], 'a non-object document contributes no keys'); + assert.deepEqual(declaredKeys('{"paths":{"a.md":{"reason":"r"}}}'), ['a.md']); + assert.deepEqual( + declaredKeys('{"paths":{"__proto__":{"reason":"r"}}}'), [], + '__proto__ is excluded from collision detection as belt-and-suspenders — in practice ' + + 'main() never reaches this on such a document, because validateAckText already ' + + 'rejects it outright (#2914 review), so there is no schema-valid document left for ' + + 'declaredKeys to see it on', + ); +}); + +test('listFragmentFiles: absent directory is zero fragments, present directory is sorted .json only', () => { + const { listFragmentFiles } = require('../scripts/lint-emitted-drift-ack.cjs'); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-lint-frag-')); + try { + assert.deepEqual(listFragmentFiles(path.join(dir, 'missing')), []); + fs.writeFileSync(path.join(dir, '2000-z.json'), '{}'); + fs.writeFileSync(path.join(dir, '1000-a.json'), '{}'); + fs.writeFileSync(path.join(dir, 'notes.txt'), 'nope'); + assert.deepEqual(listFragmentFiles(dir), ['1000-a.json', '2000-z.json']); + } finally { + cleanup(dir); + } +}); + +test('listFragmentFiles: exactly MAX_ACK_FRAGMENTS entries passes, one over fails loudly (#2914 review)', () => { + const { listFragmentFiles } = require('../scripts/lint-emitted-drift-ack.cjs'); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-lint-frag-cap-')); + try { + for (let i = 0; i < MAX_ACK_FRAGMENTS; i++) { + fs.writeFileSync(path.join(dir, `f-${String(i).padStart(4, '0')}.json`), '{}'); + } + assert.equal(listFragmentFiles(dir).length, MAX_ACK_FRAGMENTS, 'at the cap must still pass'); + + fs.writeFileSync(path.join(dir, `f-${String(MAX_ACK_FRAGMENTS).padStart(4, '0')}.json`), '{}'); + assert.throws( + () => listFragmentFiles(dir), + (err) => { + assert.match(err.message, new RegExp(dir.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))); + assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS + 1))); + assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS))); + return true; + }, + 'one over the cap must throw, naming the directory, the cap, and the actual count', + ); + } finally { + cleanup(dir); + } +}); + +// ─── guard-no-ack-on-next: presence itself is the failure (#2914) ──────────── +// +// Unlike `validateAckText` above (a PR-lane shape lint that must let a live, well-formed +// ack through), `assertAbsentOnNext` runs ONLY against `next` itself — see the +// `guard-no-ack-on-next` job in `.github/workflows/test.yml`, gated on push to `next` — and +// rejects PRESENCE outright, valid or not. Per the ack-lifecycle law (#2789), an entry +// already at the base is spent the moment it merges, so the shape never matters here. +// `assertAbsentOnNext` takes only the boolean `present` — an entryless-vs-populated +// distinction is collapsed to that boolean before this function ever sees it (see +// `main()`'s `fs.existsSync` call in `scripts/lint-emitted-drift-ack.cjs`), so no test +// here can exercise that distinction: there is deliberately no separate "entryless" +// case below, since one would be identical in input and assertion to the populated +// case and would claim coverage the function structurally cannot provide. + +test('assertAbsentOnNext passes when the file is absent — the healthy steady state', () => { + const r = assertAbsentOnNext(false); + assert.ok(r.ok); + assert.match(r.message, /absent \(the healthy steady state\)/); +}); + +test('assertAbsentOnNext fails when the file is present with entries, naming the file and the remedy', () => { + const r = assertAbsentOnNext(true); + assert.ok(!r.ok); + assert.match(r.message, /tests\/emitted-drift-ack\.json exists on next/); + assert.match(r.message, /spent and inert/); + assert.match(r.message, /delete the file too/, 'must cite CONTRIBUTING.md\'s delete-the-file rule'); + assert.match(r.message, /git rm tests\/emitted-drift-ack\.json/, 'the remedy must be named, not just the problem'); + assert.match( + r.message, /tests\/emitted-drift-acks\//, + '#2914: the message must explain that acks now go in per-PR fragments, and that a ' + + 'persisting fragment (unlike this legacy file) is harmless', + ); +}); + +test('assertAbsentOnNext fed from a real next-like tree: absent passes, present fails (regression, #2914)', () => { + // A throwaway directory standing in for `next`'s tree, so the check exercises real + // fs.existsSync semantics on ACK_REPO_PATH rather than a hand-picked boolean. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-guard-next-')); + const ackPath = path.join(dir, ACK_REPO_PATH); + fs.mkdirSync(path.dirname(ackPath), { recursive: true }); + + assert.ok( + assertAbsentOnNext(fs.existsSync(ackPath)).ok, + 'a fresh tree with no ack file must pass', + ); + + // Reproduces the exact #2834/#2900 shape: 34 spent entries surviving on next. + fs.writeFileSync(ackPath, JSON.stringify({ version: 1, paths: { 'a.md': { reason: 'spent' } } })); + const r = assertAbsentOnNext(fs.existsSync(ackPath)); + assert.ok(!r.ok, 'a tree carrying the file, however well-formed, must fail'); + assert.match(r.message, /exists on next/); +}); + +// ─── Per-PR ack fragments: mergeAckSources + readAckSources (#2914) ────────── +// +// #2914 replaces the single shared tests/emitted-drift-ack.json with per-PR fragments +// under tests/emitted-drift-acks/, exactly the shape .changeset/ already uses to solve +// the same "every PR rewrites one file wholesale" conflict problem. The legacy file is +// still read and unioned in — five open PRs (#2818, #2812, #2728, #2566, #2531) carry it +// — so BOTH surfaces must keep working, together and alone. + +// `merged.paths` is deliberately built via `Object.create(null)` (see mergeAckSources's +// doc comment: a fragment/legacy source naming a key `__proto__` must set a PROPERTY, +// never the prototype). `assert.deepEqual`/`deepStrictEqual` compares `[[Prototype]]` +// too, so a direct comparison against an ordinary `{}` literal fails on the prototype +// alone even when every key/value matches. Round-tripping through JSON (exactly what a +// real committed ack document goes through) normalizes it to a plain object for +// assertion purposes without touching the production code under test. +const plain = (o) => JSON.parse(JSON.stringify(o)); + +test('mergeAckSources: a single source with no entries merges to an empty, legal document', () => { + const { merged, errors } = mergeAckSources([]); + assert.deepEqual(errors, []); + assert.deepEqual(plain(merged), { version: ACK_VERSION, paths: {} }); +}); + +test('mergeAckSources: fragments-only union with no overlap', () => { + const { merged, errors } = mergeAckSources([ + { source: 'tests/emitted-drift-acks/1000-a.json', doc: { version: ACK_VERSION, paths: { 'a.md': { reason: 'ra' } } } }, + { source: 'tests/emitted-drift-acks/1001-b.json', doc: { version: ACK_VERSION, paths: { 'b.md': { reason: 'rb' } } } }, + ]); + assert.deepEqual(errors, []); + assert.deepEqual(plain(merged.paths), { 'a.md': { reason: 'ra' }, 'b.md': { reason: 'rb' } }); +}); + +test('mergeAckSources: legacy-only (a single source) merges through unchanged', () => { + const { merged, errors } = mergeAckSources([ + { source: ACK_FILE, doc: { version: ACK_VERSION, paths: { 'a.md': { reason: 'legacy' } } } }, + ]); + assert.deepEqual(errors, []); + assert.deepEqual(plain(merged.paths), { 'a.md': { reason: 'legacy' } }); +}); + +test('mergeAckSources: legacy file and fragments together, no overlap', () => { + const { merged, errors } = mergeAckSources([ + { source: ACK_FILE, doc: { version: ACK_VERSION, paths: { 'legacy.md': { reason: 'from legacy' } } } }, + { source: 'tests/emitted-drift-acks/1000-a.json', doc: { version: ACK_VERSION, paths: { 'a.md': { reason: 'from fragment' } } } }, + ]); + assert.deepEqual(errors, []); + assert.deepEqual(plain(merged.paths), { + 'legacy.md': { reason: 'from legacy' }, + 'a.md': { reason: 'from fragment' }, + }); +}); + +test('mergeAckSources: a duplicate key across two fragments is a loud error, never silent last-wins', () => { + const { merged, errors } = mergeAckSources([ + { source: 'tests/emitted-drift-acks/1000-a.json', doc: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'first' } } } }, + { source: 'tests/emitted-drift-acks/1001-b.json', doc: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'second' } } } }, + ]); + assert.equal(errors.length, 1); + assert.match(errors[0], /duplicate ack for/); + assert.match(errors[0], /1000-a\.json/, 'the error must name the first source'); + assert.match(errors[0], /1001-b\.json/, 'the error must name the second source'); + // First-wins is a deliberate, DOCUMENTED simplification (not silent): the caller is + // told loudly via `errors`, and the merged doc still holds a well-defined value. + assert.equal(merged.paths[WORKFLOW_KEY].reason, 'first'); +}); + +test('mergeAckSources: an empty fragments directory (represented as zero docs) is legal', () => { + const { merged, errors } = mergeAckSources([]); + assert.deepEqual(errors, []); + assert.deepEqual(plain(merged.paths), {}); +}); + +test('mergeAckSources: a malformed source (bad version) surfaces the same error parseAck would', () => { + const { errors } = mergeAckSources([ + { source: 'tests/emitted-drift-acks/1000-bad.json', doc: { version: 99, paths: {} } }, + ]); + assert.match(errors.join('\n'), /unsupported version 99/); +}); + +test('mergeAckSources: a __proto__ key from a fragment is rejected, never merged in or used to pollute', () => { + // MUST be built via JSON.parse, not a JS object literal: `{ '__proto__': v }` is the + // special ObjectLiteral case that SETS THE PROTOTYPE and yields zero own keys, so a + // fixture built that way is empty and never exercises this path at all (the exact + // reason the previous version of this test was vacuous and failed with "Cannot read + // properties of undefined"). `JSON.parse` is the production path and creates a genuine + // own key. + const doc = JSON.parse('{"version":' + ACK_VERSION + ',"paths":{"__proto__":{"reason":"hostile"}}}'); + assert.deepEqual(Object.keys(doc.paths), ['__proto__'], 'JSON.parse must create a genuine own key'); + + const { merged, errors } = mergeAckSources([ + { source: 'tests/emitted-drift-acks/1000-a.json', doc }, + ]); + + assert.equal(errors.length, 1); + assert.match(errors[0], /reserved/); + assert.match(errors[0], /__proto__/); + assert.deepEqual(plain(merged.paths), {}, 'a reserved key must never be merged into the document'); + assert.equal(Object.getPrototypeOf(merged.paths), null, 'merged.paths stays null-prototype'); + assert.equal(({}).reason, undefined, 'Object.prototype must be untouched'); +}); + +test('readAckSources: fragments-only on a real tree', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-ack-frag-')); + try { + const fragDir = path.join(dir, 'acks'); + fs.mkdirSync(fragDir, { recursive: true }); + fs.writeFileSync(path.join(fragDir, '1000-a.json'), JSON.stringify({ version: ACK_VERSION, paths: { 'a.md': { reason: 'ra' } } })); + fs.writeFileSync(path.join(fragDir, '1001-b.json'), JSON.stringify({ version: ACK_VERSION, paths: { 'b.md': { reason: 'rb' } } })); + + const { doc, errors } = readAckSources({ legacyPath: path.join(dir, 'emitted-drift-ack.json'), fragmentsDir: fragDir }); + assert.deepEqual(errors, []); + assert.deepEqual(plain(doc.paths), { 'a.md': { reason: 'ra' }, 'b.md': { reason: 'rb' } }); + } finally { + cleanup(dir); + } +}); + +test('readAckSources: legacy-only on a real tree (no fragments directory at all)', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-ack-legacy-')); + try { + const legacyPath = path.join(dir, 'emitted-drift-ack.json'); + fs.writeFileSync(legacyPath, JSON.stringify({ version: ACK_VERSION, paths: { 'legacy.md': { reason: 'from legacy' } } })); + + const { doc, errors } = readAckSources({ legacyPath, fragmentsDir: path.join(dir, 'nonexistent-acks') }); + assert.deepEqual(errors, []); + assert.deepEqual(plain(doc.paths), { 'legacy.md': { reason: 'from legacy' } }); + } finally { + cleanup(dir); + } +}); + +test('readAckSources: legacy file and fragments together', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-ack-both-')); + try { + const legacyPath = path.join(dir, 'emitted-drift-ack.json'); + const fragDir = path.join(dir, 'acks'); + fs.mkdirSync(fragDir, { recursive: true }); + fs.writeFileSync(legacyPath, JSON.stringify({ version: ACK_VERSION, paths: { 'legacy.md': { reason: 'from legacy' } } })); + fs.writeFileSync(path.join(fragDir, '1000-a.json'), JSON.stringify({ version: ACK_VERSION, paths: { 'a.md': { reason: 'from fragment' } } })); + + const { doc, errors } = readAckSources({ legacyPath, fragmentsDir: fragDir }); + assert.deepEqual(errors, []); + assert.deepEqual(plain(doc.paths), { + 'legacy.md': { reason: 'from legacy' }, + 'a.md': { reason: 'from fragment' }, + }); + } finally { + cleanup(dir); + } +}); + +test('readAckSources: neither the legacy file nor the fragments directory exists — the healthy steady state', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-ack-absent-')); + try { + const { doc, errors } = readAckSources({ + legacyPath: path.join(dir, 'emitted-drift-ack.json'), + fragmentsDir: path.join(dir, 'acks'), + }); + assert.equal(doc, null); + assert.deepEqual(errors, []); + } finally { + cleanup(dir); + } +}); + +test('readAckSources: an empty fragments directory (present, zero files) plus no legacy file', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-ack-emptydir-')); + try { + const fragDir = path.join(dir, 'acks'); + fs.mkdirSync(fragDir, { recursive: true }); + const { doc, errors } = readAckSources({ legacyPath: path.join(dir, 'emitted-drift-ack.json'), fragmentsDir: fragDir }); + assert.equal(doc, null, 'zero fragments and no legacy file is still the healthy steady state'); + assert.deepEqual(errors, []); + } finally { + cleanup(dir); + } +}); + +test('readAckSources: a duplicate key across two fragments fails loudly and would fail the real gate', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-ack-dupe-')); + try { + const fragDir = path.join(dir, 'acks'); + fs.mkdirSync(fragDir, { recursive: true }); + fs.writeFileSync(path.join(fragDir, '1000-a.json'), JSON.stringify({ version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'first' } } })); + fs.writeFileSync(path.join(fragDir, '1001-b.json'), JSON.stringify({ version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'second' } } })); + + const { doc: ack, errors: mergeAckErrors } = readAckSources({ + legacyPath: path.join(dir, 'emitted-drift-ack.json'), + fragmentsDir: fragDir, + }); + assert.equal(mergeAckErrors.length, 1); + assert.match(mergeAckErrors[0], /duplicate ack for/); + + // Wired exactly as the real-tree test wires it: folded into diffEmitted's own + // errors, which must fail the whole gate — never silently pass with one winner. + const r = diffEmitted({ + baseline: mf({ [WORKFLOW_KEY]: 'aaa' }), + current: mf({ [WORKFLOW_KEY]: 'aaa' }), + changedPaths: [], + ack, + baseAck: null, + mergeAckErrors, + }); + assert.ok(!r.ok, 'a duplicate ack across two fragments must fail the gate'); + assert.match(formatReport(r), /duplicate ack for/); + } finally { + cleanup(dir); + } +}); + +test('readAckSources: a malformed fragment (invalid JSON) throws, naming the fragment file', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-ack-malformed-')); + try { + const fragDir = path.join(dir, 'acks'); + fs.mkdirSync(fragDir, { recursive: true }); + fs.writeFileSync(path.join(fragDir, '1000-bad.json'), '{ not json'); + + assert.throws( + () => readAckSources({ legacyPath: path.join(dir, 'emitted-drift-ack.json'), fragmentsDir: fragDir }), + /1000-bad\.json.*not valid JSON/, + ); + } finally { + cleanup(dir); + } +}); + +test('readAckSources: listAckFragmentFiles returns sorted .json names only, absent dir is empty', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-ack-listing-')); + try { + assert.deepEqual(listAckFragmentFiles(path.join(dir, 'missing')), []); + + const fragDir = path.join(dir, 'acks'); + fs.mkdirSync(fragDir, { recursive: true }); + fs.writeFileSync(path.join(fragDir, '2000-z.json'), '{}'); + fs.writeFileSync(path.join(fragDir, '1000-a.json'), '{}'); + fs.writeFileSync(path.join(fragDir, 'README.md'), 'not a fragment'); + assert.deepEqual(listAckFragmentFiles(fragDir), ['1000-a.json', '2000-z.json']); + } finally { + cleanup(dir); + } +}); + +test('listAckFragmentFilesAtRef: lists .json fragment names at a ref directly, sorted, non-.json excluded', () => { + const run = (args) => { + assert.equal(args[0], 'ls-tree'); + assert.equal(args[args.length - 1], `${ACK_DIR_REPO_PATH}/`); + return [ + `${ACK_DIR_REPO_PATH}/2000-z.json`, + `${ACK_DIR_REPO_PATH}/1000-a.json`, + `${ACK_DIR_REPO_PATH}/README.md`, + ].join('\n') + '\n'; + }; + assert.deepEqual(listAckFragmentFilesAtRef(SHA_A, { run }), ['1000-a.json', '2000-z.json']); +}); + +test('listAckFragmentFilesAtRef: an absent fragment directory at the ref is zero names, not a fault', () => { + const run = () => '\n'; + assert.deepEqual(listAckFragmentFilesAtRef(SHA_A, { run }), []); +}); + +test('listAckFragmentFilesAtRef: a git failure listing the directory throws', () => { + const run = () => { throw new Error('injected git failure'); }; + assert.throws(() => listAckFragmentFilesAtRef(SHA_A, { run }), /could not list tests\/emitted-drift-acks\//); +}); + +test('listAckFragmentFiles: exactly MAX_ACK_FRAGMENTS entries passes, one over fails loudly (#2914 review)', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-ack-listing-cap-')); + try { + for (let i = 0; i < MAX_ACK_FRAGMENTS; i++) { + fs.writeFileSync(path.join(dir, `f-${String(i).padStart(4, '0')}.json`), '{}'); + } + assert.equal(listAckFragmentFiles(dir).length, MAX_ACK_FRAGMENTS, 'at the cap must still pass'); + + fs.writeFileSync(path.join(dir, `f-${String(MAX_ACK_FRAGMENTS).padStart(4, '0')}.json`), '{}'); + assert.throws( + () => listAckFragmentFiles(dir), + (err) => { + assert.match(err.message, new RegExp(dir.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))); + assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS + 1))); + assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS))); + return true; + }, + 'one over the cap must throw, naming the directory, the cap, and the actual count', + ); + } finally { + cleanup(dir); + } +}); + +test('listAckFragmentFilesAtRef: exactly MAX_ACK_FRAGMENTS entries passes, one over fails loudly (#2914 review)', () => { + const makeListing = (count) => Array.from( + { length: count }, + (_, i) => `${ACK_DIR_REPO_PATH}/f-${String(i).padStart(4, '0')}.json`, + ).join('\n') + '\n'; + + const atCap = () => makeListing(MAX_ACK_FRAGMENTS); + assert.equal( + listAckFragmentFilesAtRef(SHA_A, { run: atCap }).length, + MAX_ACK_FRAGMENTS, + 'at the cap must still pass', + ); + + const overCap = () => makeListing(MAX_ACK_FRAGMENTS + 1); + assert.throws( + () => listAckFragmentFilesAtRef(SHA_A, { run: overCap }), + (err) => { + assert.match(err.message, new RegExp(`${ACK_DIR_REPO_PATH}/ at "${SHA_A}"`.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))); + assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS + 1))); + assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS))); + return true; + }, + 'one over the cap must throw, naming the directory, the cap, and the actual count', + ); +}); + +test('the migrated fragment physically lives at ACK_DIR/0000-legacy-migration.json on this checkout', () => { + // `includes`, not `deepEqual`, on purpose: other PRs merging their own fragments over + // time must not make this permanent regression test fail — it only pins THIS + // fragment's continued existence at the real, non-injected path. + const fragmentPath = path.join(ACK_DIR, '0000-legacy-migration.json'); + assert.ok(fs.existsSync(fragmentPath), 'the migration must have landed at the real fragment directory path'); + assert.ok(listAckFragmentFiles(ACK_DIR).includes('0000-legacy-migration.json')); +}); + +test('the migrated legacy fragment still clears the real #2733 spec-phase.md growth (#2914 migration)', () => { + // The whole point of MIGRATING rather than deleting: no acknowledgment is lost, only + // relocated. This pins the migrated fragment's spec-phase.md entry to the exact + // growth #2733 introduced (31987 -> 31997 bytes), so a regression here would have + // reproduced the ratchet failure a real verification run already caught once. + const fragmentPath = path.join(REPO_ROOT, 'tests', 'emitted-drift-acks', '0000-legacy-migration.json'); + const doc = JSON.parse(fs.readFileSync(fragmentPath, 'utf8')); + const entry = doc.paths['spec-phase.md']; + assert.ok(entry, 'the migrated fragment must still carry the spec-phase.md entry from #2733'); + assert.match(entry.reason, /31987 -> 31997/, 'the exact byte delta must survive the migration'); + + // Isolated to JUST this one entry, not the whole 35-entry document: the migrated + // fragment carries 34 OTHER already-spent entries for OTHER paths, which this + // minimal repro's baseline/current never touches — including the full document here + // would report those 34 as freshly-stale (nothing in THIS synthetic diff consumes + // them), which is a fact about this test's narrow fixture, not about the migration. + const r = diffEmitted({ + baseline: mf({}), + current: mf({}), + changedPaths: [], + sizeBaseline: { 'spec-phase.md': 31987 }, + sizeCurrent: { 'spec-phase.md': 31997 }, + ack: { version: ACK_VERSION, paths: { 'spec-phase.md': entry } }, + baseAck: null, + }); + assert.equal(r.grown.length, 1); + assert.equal(r.grown[0].acked, true, 'the migrated entry must still clear the growth it was written for'); + assert.deepEqual(r.staleAcks, []); + assert.ok(r.ok); +}); + +test('the full migrated document is safe to land: every entry is SPENT against the legacy file at base, none stale', () => { + // The actual migration PR's real shape: `next` still carries the legacy file with + // these SAME 35 entries (until this PR removes it), so every entry in the new + // fragment is already "spent" (base already explains it) rather than "live" — this + // PR introduces no NEW ripple of its own, it only relocates old acknowledgments. + // Getting this wrong (e.g. losing a reason's exact text in the move) would turn a + // spent entry into a freshly-unexplained "stale" one and red this very migration PR. + const fragmentPath = path.join(REPO_ROOT, 'tests', 'emitted-drift-acks', '0000-legacy-migration.json'); + const doc = JSON.parse(fs.readFileSync(fragmentPath, 'utf8')); + + const r = diffEmitted({ + baseline: mf({}), + current: mf({}), + changedPaths: [], + ack: doc, + baseAck: doc, // the legacy file at `next` HEAD, byte-identical, before this PR removes it + }); + assert.deepEqual(r.staleAcks, [], 'nothing in the migrated document should read as freshly unexplained'); + assert.equal(r.spentAcks.length, Object.keys(doc.paths).length, 'every migrated entry must be recognized as already spent'); + assert.ok(r.ok); +}); + // ─── readAckFileAtRef: the base-side reader (#2789) ────────────────────────── // // This half never runs in the remote runner — the real-tree test skips there, because a @@ -842,6 +1399,116 @@ test('readAckFileAtRef: refuses an option-shaped ref rather than handing it to g } }); +// ─── readAckSourcesAtRef: the base-side UNION reader (#2914) ───────────────── +// +// Mirrors readAckFileAtRef's fakeGit harness above, extended to also answer `ls-tree` +// on the FRAGMENT DIRECTORY and `show` for each fragment name it lists — a base-side +// stand-in for "the legacy file plus every fragment, as they existed at that ref". + +const fakeMultiGit = ({ legacy, fragments = {} } = {}) => (args) => { + if (args[0] === 'ls-tree') { + const target = args[args.length - 1]; + if (target === ACK_REPO_PATH) return legacy !== undefined ? `${ACK_REPO_PATH}\n` : '\n'; + if (target === `${ACK_DIR_REPO_PATH}/`) { + const names = Object.keys(fragments); + return names.length ? names.map((n) => `${ACK_DIR_REPO_PATH}/${n}`).join('\n') + '\n' : '\n'; + } + // readAckFileAtRef's OWN per-file existence check, once per fragment name it was + // told about by the directory listing above — a second, distinct ls-tree call. + const name = target.slice(target.lastIndexOf('/') + 1); + if (`${ACK_DIR_REPO_PATH}/${name}` === target && Object.prototype.hasOwnProperty.call(fragments, name)) { + return `${target}\n`; + } + throw new Error(`fakeMultiGit: unexpected ls-tree target ${target}`); + } + if (args[0] === 'show') { + const spec = args[1]; + const p = spec.slice(spec.indexOf(':') + 1); + if (p === ACK_REPO_PATH) return legacy; + const name = p.slice(p.lastIndexOf('/') + 1); + if (Object.prototype.hasOwnProperty.call(fragments, name)) return fragments[name]; + throw new Error(`fakeMultiGit: unexpected show path ${p}`); + } + throw new Error(`fakeMultiGit: unexpected git call: ${args.join(' ')}`); +}; + +test('readAckSourcesAtRef: fragments-only at a ref', () => { + const { doc } = readAckSourcesAtRef(SHA_A, { + run: fakeMultiGit({ + fragments: { + '1000-a.json': JSON.stringify({ version: ACK_VERSION, paths: { 'a.md': { reason: 'ra' } } }), + '1001-b.json': JSON.stringify({ version: ACK_VERSION, paths: { 'b.md': { reason: 'rb' } } }), + }, + }), + }); + assert.deepEqual(plain(doc.paths), { 'a.md': { reason: 'ra' }, 'b.md': { reason: 'rb' } }); +}); + +test('readAckSourcesAtRef: legacy-only at a ref (no fragments directory)', () => { + const { doc } = readAckSourcesAtRef(SHA_A, { + run: fakeMultiGit({ legacy: JSON.stringify({ version: ACK_VERSION, paths: { 'legacy.md': { reason: 'from legacy' } } }) }), + }); + assert.deepEqual(plain(doc.paths), { 'legacy.md': { reason: 'from legacy' } }); +}); + +test('readAckSourcesAtRef: legacy and fragments together at a ref', () => { + const { doc } = readAckSourcesAtRef(SHA_A, { + run: fakeMultiGit({ + legacy: JSON.stringify({ version: ACK_VERSION, paths: { 'legacy.md': { reason: 'from legacy' } } }), + fragments: { '1000-a.json': JSON.stringify({ version: ACK_VERSION, paths: { 'a.md': { reason: 'from fragment' } } }) }, + }), + }); + assert.deepEqual(plain(doc.paths), { 'legacy.md': { reason: 'from legacy' }, 'a.md': { reason: 'from fragment' } }); +}); + +test('readAckSourcesAtRef: neither legacy nor any fragment exists at the ref', () => { + const { doc } = readAckSourcesAtRef(SHA_A, { run: fakeMultiGit({}) }); + assert.equal(doc, null); +}); + +test('readAckSourcesAtRef: an empty fragments directory at the ref, no legacy file', () => { + const { doc } = readAckSourcesAtRef(SHA_A, { run: fakeMultiGit({ fragments: {} }) }); + assert.equal(doc, null, 'zero fragments and no legacy file at the ref is still the healthy steady state'); +}); + +test('readAckSourcesAtRef: a base-side duplicate across fragments does not throw (discarded like other base schema issues)', () => { + // Matches this module's existing precedent for the base side (see diffEmitted's real + // -tree caller: "Base-side SCHEMA errors are deliberately discarded"). A base-side + // duplicate is `next`'s own health, not this diff's to answer for, and + // mergeAckSources's first-source-wins keeps the STRICT reading even with no error + // surfaced -- an entry can only be spent against the ONE reason actually kept. + const { doc } = readAckSourcesAtRef(SHA_A, { + run: fakeMultiGit({ + fragments: { + '1000-a.json': JSON.stringify({ version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'first' } } }), + '1001-b.json': JSON.stringify({ version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'second' } } }), + }, + }), + }); + assert.equal(doc.paths[WORKFLOW_KEY].reason, 'first'); +}); + +test('readAckSourcesAtRef: a fragment that is unreadable at the ref still throws', () => { + const fragRelPath = `${ACK_DIR_REPO_PATH}/1000-a.json`; + const run = (args) => { + if (args[0] === 'ls-tree') { + const target = args[args.length - 1]; + if (target === ACK_REPO_PATH) return '\n'; + if (target === `${ACK_DIR_REPO_PATH}/`) return `${fragRelPath}\n`; + // readAckFileAtRef's OWN existence check for the individual fragment file, prior + // to `show` — it exists at this ref, so the failure below is a genuine read fault. + if (target === fragRelPath) return `${fragRelPath}\n`; + throw new Error(`unexpected ls-tree target: ${target}`); + } + if (args[0] === 'show') throw new Error('injected git failure'); + throw new Error(`unexpected git call: ${args.join(' ')}`); + }; + assert.throws( + () => readAckSourcesAtRef(SHA_A, { run }), + /exists at .* but could not be read/, + ); +}); + test('OMITTING baseAck while an ack is present is a loud error, never a silent pass', () => { // This is what makes the production seam non-revertible in silence. Drop `baseAck:` // from the real-tree call and the gate fails loudly here, instead of quietly restoring @@ -1102,9 +1769,14 @@ test('the taught document derives its version from ACK_VERSION', () => { assert.equal(JSON.parse(REMEDIATION.ackDocument([{ key: 'x.md', reason: 'r' }])).version, ACK_VERSION); }); -test('the remediation surface is frozen and names the ack file once', () => { +test('the remediation surface is frozen and points at the fragment directory, not the legacy file', () => { + // #2914: the remedy is a NEW fragment under ACK_DIR, never the single legacy file — + // asserting `ackFile === ACK_FILE` here would pin the exact bandaid this design + // replaces (a shared filename every PR is tempted back onto). assert.ok(Object.isFrozen(REMEDIATION), 'the exported surface must not be mutable'); - assert.equal(REMEDIATION.ackFile, ACK_FILE, 'one definition, not a second literal'); + assert.equal(REMEDIATION.ackDir, ACK_DIR_PURE, 'one definition, not a second literal'); + assert.ok(REMEDIATION.ackFile.startsWith(`${ACK_DIR_PURE}/`), 'the taught path must live under the fragment directory'); + assert.notEqual(REMEDIATION.ackFile, ACK_FILE, 'the remedy must not be the legacy shared file'); }); test('a ripple and a growth in one report share ONE document', () => { @@ -2395,17 +3067,21 @@ test('differential attribution over the real tree', { timeout: 900_000 }, async assert.ok(baseline && Object.keys(baseline).length > 0, `resolved baseline via ${resolvedBaseline.via} has no families`); const changedPaths = resolveChangedPaths(base); - const ack = readAckFile(); + // #2914: unions the legacy single file with every per-PR fragment under + // tests/emitted-drift-acks/. `mergeAckErrors` (e.g. two sources naming the same path) + // is folded into `diffEmitted`'s own errors below, exactly like any other ack schema + // problem — never silently resolved. + const { doc: ack, errors: mergeAckErrors } = readAckSources(); const current = currentManifests(); // Consult the base side ONLY when this tree actually has a document to classify. - // `readAckFileAtRef` throws on a base it cannot read, which is right — but reading it - // unconditionally would DEADLOCK the repo if `next` ever carried a corrupt ack (a bad - // merge leaving conflict markers in exactly the file class this epic exists over): + // `readAckSourcesAtRef` throws on a base it cannot read, which is right — but reading + // it unconditionally would DEADLOCK the repo if `next` ever carried a corrupt ack (a + // bad merge leaving conflict markers in exactly the file class this epic exists over): // every PR would go red, INCLUDING the PR that deletes the corrupt file and repairs // base. A tree carrying no ack has nothing to inherit, so it needs no base read — which // is precisely the shape of the repair PR, and it lands and unblocks everyone. - const baseAck = ack === null ? null : readAckFileAtRef(baseSha); + const baseAck = ack === null ? null : readAckSourcesAtRef(baseSha).doc; // Reconcile the family SET across three independent signals, rather than asserting one // count against both sides. The baseline is built at the base ref and the current tree @@ -2443,6 +3119,7 @@ test('differential attribution over the real tree', { timeout: 900_000 }, async baseAck, sizeBaseline: resolvedBaseline.sizeBaseline, sizeCurrent: currentSizes(), + mergeAckErrors, }); assert.ok( diff --git a/tests/emitted-drift-ack.json b/tests/emitted-drift-acks/0000-legacy-migration.json similarity index 100% rename from tests/emitted-drift-ack.json rename to tests/emitted-drift-acks/0000-legacy-migration.json diff --git a/tests/helpers/emitted-diff.cjs b/tests/helpers/emitted-diff.cjs index 792607f61..b3ec11a86 100644 --- a/tests/helpers/emitted-diff.cjs +++ b/tests/helpers/emitted-diff.cjs @@ -7,7 +7,9 @@ * Given the emitted manifests at `next` HEAD and at PR HEAD, plus the repo paths the * PR actually changed, decide which moved emitted paths are EXPLAINED by the diff and * which are not. Unattributable deltas are a hard failure that names them; the only - * way through is a committed acknowledgment (`tests/emitted-drift-ack.json`). + * way through is a committed acknowledgment — a per-PR fragment under + * `tests/emitted-drift-acks/` (#2914), or, for branches predating that split, the + * legacy single `tests/emitted-drift-ack.json` — both are read and unioned (#2914). * * ── Why this module is pure ────────────────────────────────────────────────── * No fs, no git, no installer, no clock. The naive shape — one integration test that @@ -35,16 +37,43 @@ const { attributeEmittedPath } = require('./emitted-provenance.cjs'); const ACK_VERSION = 1; /** - * The acknowledgment file, named ONCE (#2778). + * Key names that can never be a legitimate emitted path or bare workflow/agent filename, + * and that also happen to be the JS-object footguns (`__proto__`, `constructor`, + * `prototype`). Rejected LOUDLY by `parseAck` rather than silently dropped: a document + * naming one of these is always an authoring mistake (never a real path), and dropping it + * quietly would let the SAME document pass `scripts/lint-emitted-drift-ack.cjs`'s + * duplicate-detection (which excludes these keys for a different reason — see + * `declaredKeys`'s doc comment there) while erroring differently here — exactly the + * generative-fix-divergence class this repo's parity test exists to catch (#2914 review). + */ +const RESERVED_ACK_KEYS = new Set(['__proto__', 'constructor', 'prototype']); + +/** + * The LEGACY acknowledgment file, named ONCE (#2778), still honored (#2914). * * This string was previously typed by hand in `formatReport`'s unattributable branch, in * `parseAck`'s default `source`, and again as `ACK_PATH` in emitted-runtime.cjs. Adding a * fourth copy for the growth branch is the *generative fix divergence* class this repo * records: parallel surfaces reading one shared value must not be able to drift. One * definition consumed by every branch is cheaper than a parity test over four literals. + * + * #2914 replaces this SINGLE SHARED FILE with per-PR fragments under `ACK_DIR` — a + * single mutable document whose `paths` map every PR rewrites wholesale is a guaranteed + * merge-conflict cell between any two PRs that both need an ack (5 of 6 conflicting PRs + * in the open queue collided on this file and nothing else). The legacy path is still + * read and unioned with the fragments directory so open PRs authored before the split + * (#2818, #2812, #2728, #2566, #2531) are not broken by this change. */ const ACK_FILE = 'tests/emitted-drift-ack.json'; +/** + * The per-PR fragment directory (#2914) — the `.changeset/`-shaped fix to the same + * shared-mutable-file problem `.changeset/` already solves: every fragment is a + * separately-named file, so two PRs adding an ack can never conflict with each other, + * and a fragment left behind on `next` after merge is inert rather than a shared cell. + */ +const ACK_DIR = 'tests/emitted-drift-acks'; + /** * Distinguishes "caller omitted the base side" from "caller said there is none". * @@ -90,6 +119,24 @@ const INVISIBLE = new RegExp( */ const NEW_FILE_CAP = 32768; +/** + * Upper bound on how many fragment files a `readdirSync` of `ACK_DIR` (or its + * counterpart in `scripts/lint-emitted-drift-ack.cjs`) may return in one pass. + * + * 500 is ample headroom over any real repo's fragment count, which stays in the + * single digits between releases (fragments are deleted once spent). Exceeding it + * throws rather than silently truncating: a truncated listing would silently drop + * acknowledgments from the merged set, which is exactly the class of silent failure + * this whole ack seam exists to prevent — the fix for a directory this large is to + * prune spent fragments, never to read only some of them. + * + * Duplicated (not imported) in `scripts/lint-emitted-drift-ack.cjs`, which cannot + * require anything from `tests/` (it ships in the npm package; `tests/` does not). + * The two are held to the same value by the schema-parity test in + * `tests/emitted-attribution.test.cjs`. + */ +const MAX_ACK_FRAGMENTS = 500; + /** * Render the minimal valid acknowledgment document for a set of entries (#2778). * @@ -136,8 +183,16 @@ function ackDocument(entries) { * help text must not be a breaking change. */ const REMEDIATION = Object.freeze({ - ackFile: ACK_FILE, - createIfAbsent: 'create the file if absent — it exists only when something needs acknowledging', + // Deliberately NOT a fixed filename (#2914): the remedy is a NEW fragment under + // `ACK_DIR`, and the whole point of a fragment directory is that its name is the + // contributor's to pick — a fixed suggestion here would tempt everyone back onto one + // shared filename, resurrecting the exact merge-conflict cell this design removes. + ackFile: `${ACK_DIR}/.json`, + ackDir: ACK_DIR, + createIfAbsent: + `create a NEW file under ${ACK_DIR}/ — pick a name nobody else is using (include ` + + 'this issue or PR number, e.g. `2914-fix.json`), and never reuse an existing ' + + 'fragment\'s name', doNotRegenerate: 'Do NOT regenerate anything to silence this — there is nothing left to regenerate.', /** The size ratchet keys on `entry.name` from readdirSync (emitted-runtime.cjs `currentSizes`). */ @@ -147,13 +202,13 @@ const REMEDIATION = Object.freeze({ rippleReason: '', growthReason: '', staleAckFix: - `Delete those entries from ${ACK_FILE}, or correct them to name the ripple you ` - + 'actually made. If that leaves no entries, delete the file itself — an empty one ' - + 'signals nothing.', + `Delete those entries from your fragment under ${ACK_DIR}/, or correct them to name ` + + 'the ripple you actually made. If that leaves no entries, delete the file itself ' + + '— an empty one signals nothing.', spentAckNote: 'These are inert, NOT a failure: the base already carries them, so their ripple is ' - + `absorbed and they can no longer clear anything. Delete them from ${ACK_FILE} ` - + 'whenever convenient.', + + `absorbed and they can no longer clear anything. Delete them from your fragment ` + + `under ${ACK_DIR}/ whenever convenient.`, ackDocument, }); @@ -209,6 +264,19 @@ function parseAck(doc, { source = 'emitted-drift-ack.json' } = {}) { } for (const [rel, value] of Object.entries(paths)) { + if (RESERVED_ACK_KEYS.has(rel)) { + // Reject loudly rather than silently drop. This is the fix for #2914 review: a + // document naming `__proto__`/`constructor`/`prototype` used to be silently + // accepted here (a genuine own key when the document comes from `JSON.parse`, + // per the production path) while the lint filtered it out of duplicate detection + // — two surfaces disagreeing about the SAME key is exactly the drift the parity + // test below exists to catch. + errors.push( + `${source}: ack key "${rel}" is reserved and can never be a valid emitted path ` + + 'or workflow/agent filename — remove it', + ); + continue; + } const reason = value && typeof value === 'object' ? value.reason : value; if (typeof reason !== 'string' || reason.trim() === '') { // "name them AND say why" is the contract (ADR-2719 §3). An ack with no reason @@ -225,6 +293,63 @@ function parseAck(doc, { source = 'emitted-drift-ack.json' } = {}) { return { entries, errors }; } +/** + * Union multiple ack SOURCES into ONE document (#2914). + * + * `docs` is an ordered list of `{ source, doc }`, where `doc` is a parsed ack document + * (or `null`) and `source` is a human label used only in error messages — a fragment's + * repo-relative path, or the legacy file's path. Each source is parsed with the SAME + * `parseAck` the single-document gate already uses, so the schema can never drift + * between "one file" and "many files" — there is exactly one definition of what a valid + * entry looks like, reused here rather than re-typed. + * + * ── The collision rule (#2914) ──────────────────────────────────────────────── + * Two sources declaring the SAME emitted/growth key is an ERROR, never a silent + * last-wins merge. Two per-PR fragments are never supposed to name the same path — if + * they do, at least one of them is wrong, or the world has already changed under one of + * them since it was written — and silently letting the later source win would let a + * fragment quietly retire an earlier one's acknowledgment with zero signal in the diff. + * That is exactly the class of silent drift the acknowledgment seam (#2789 spent/live + * lifecycle) exists to end: an ack that stops explaining anything must be conspicuous, + * never invisible. Failing loudly, with both source names in the message, is the only + * reading consistent with the rest of this module's "unknown fails toward the strict + * side" law (see `readAckFileAtRef`'s doc comment for the base-side version of the same + * principle). + * + * @param {Array<{source: string, doc: object|null}>} docs + * @returns {{ merged: {version: number, paths: object}, errors: string[] }} + */ +function mergeAckSources(docs) { + const errors = []; + // Null-prototype for the same reason `ackDocument` uses one above: an ordinary `{}` + // would turn an assignment keyed `__proto__` into setting the prototype rather than a + // property. `parseAck` now rejects `RESERVED_ACK_KEYS` outright (#2914 review) so + // `entries` below can never actually carry one — this is belt-and-suspenders against + // the day that stops being true, not the current enforcement point. + const paths = Object.create(null); + const owner = new Map(); // rel -> source that already claimed it, for the error message + + for (const { source, doc } of docs) { + const { entries, errors: parseErrors } = parseAck(doc, { source }); + errors.push(...parseErrors); + for (const [rel, entry] of entries) { + if (owner.has(rel)) { + errors.push( + `duplicate ack for "${rel}": declared in both ${owner.get(rel)} and ${source}. ` + + 'Two ack sources may never name the same path — rename or merge the fragments.', + ); + continue; + } + owner.set(rel, source); + paths[rel] = entry.runtime !== undefined + ? { reason: entry.reason, runtime: entry.runtime } + : { reason: entry.reason }; + } + } + + return { merged: { version: ACK_VERSION, paths }, errors }; +} + /** * The conservation law. * @@ -259,6 +384,13 @@ function parseAck(doc, { source = 'emitted-drift-ack.json' } = {}) { * absent there). Required once `ack` is non-null. * @param {object} [opts.sizeBaseline] { [name]: bytes } workflow/agent sizes at next * @param {object} [opts.sizeCurrent] { [name]: bytes } workflow/agent sizes at PR HEAD + * @param {string[]} [opts.mergeAckErrors] errors already discovered while UNIONING + * `ack` from multiple physical sources (`mergeAckSources`, #2914) — e.g. two fragments + * naming the same path. This module never touches the filesystem, so it cannot + * discover a cross-file collision on its own; the shell layer that reads the legacy + * file plus every fragment computes this and folds it in verbatim so a duplicate + * fails the gate exactly like any other ack schema error, rather than silently + * resolving via last-wins. * * @returns {{ * moved: number, attributed: Array, unattributable: Array, acked: Array, @@ -274,6 +406,7 @@ function diffEmitted({ baseAck = BASE_ACK_OMITTED, sizeBaseline = null, sizeCurrent = null, + mergeAckErrors = [], } = {}) { const errors = []; @@ -306,6 +439,8 @@ function diffEmitted({ const changedSet = new Set(changedPaths); const { entries: declaredAcks, errors: ackErrors } = parseAck(ack); errors.push(...ackErrors); + // Folded in verbatim, not re-derived: see `mergeAckErrors`'s doc comment above. + errors.push(...mergeAckErrors); // Keyed on DECLARED ENTRIES, not on the document being non-null. `{}`, `{version:1}` // and `{paths:{}}` all carry zero acks and `parseAck` calls them legal, so demanding a @@ -702,10 +837,13 @@ function formatReport(result, { sampleLimit = 20 } = {}) { module.exports = { ACK_VERSION, ACK_FILE, + ACK_DIR, NEW_FILE_CAP, + MAX_ACK_FRAGMENTS, REMEDIATION, sourceSatisfiedBy, parseAck, + mergeAckSources, diffEmitted, buildReport, formatReport, diff --git a/tests/helpers/emitted-runtime.cjs b/tests/helpers/emitted-runtime.cjs index be542e698..d7db4b83f 100644 --- a/tests/helpers/emitted-runtime.cjs +++ b/tests/helpers/emitted-runtime.cjs @@ -44,15 +44,45 @@ const { buildParityManifest, PKG_VERSION, } = require('./install-shared.cjs'); +const { mergeAckSources, MAX_ACK_FRAGMENTS } = require('./emitted-diff.cjs'); + +/** + * Fail loudly when a fragment listing exceeds `MAX_ACK_FRAGMENTS`, naming the + * directory, the cap, and the actual count. Never truncate: a silently-truncated + * listing would silently drop acknowledgments, which is exactly the class of silent + * failure the ack seam exists to prevent (see `MAX_ACK_FRAGMENTS`'s doc comment in + * `emitted-diff.cjs`). + */ +function assertFragmentCountWithinCap(dirLabel, names) { + if (names.length > MAX_ACK_FRAGMENTS) { + throw new Error( + `emitted-attribution: ${dirLabel} contains ${names.length} ack fragments, ` + + `exceeding the cap of ${MAX_ACK_FRAGMENTS}. Refusing to read only some of them — a ` + + 'truncated read would silently drop acknowledgments. Prune spent fragments from ' + + 'this directory.', + ); + } + return names; +} const REPO_ROOT = path.join(__dirname, '..', '..'); /** * Repo-relative and POSIX-separated on every platform: this form is what `git show * :` requires, and git speaks only forward slashes regardless of host OS. * `ACK_PATH` derives from it so the two can never name different files. + * + * LEGACY single-file path (#2778), still honored and unioned with `ACK_DIR_REPO_PATH` + * below (#2914) — open PRs authored before the fragment split still carry this file. */ const ACK_REPO_PATH = 'tests/emitted-drift-ack.json'; const ACK_PATH = path.join(REPO_ROOT, ...ACK_REPO_PATH.split('/')); +/** + * Per-PR fragment directory (#2914). Every fragment is independently named, so two PRs + * that each need an ack can never collide on this path the way they always did on the + * single legacy file above. + */ +const ACK_DIR_REPO_PATH = 'tests/emitted-drift-acks'; +const ACK_DIR = path.join(REPO_ROOT, ...ACK_DIR_REPO_PATH.split('/')); const FIXTURE_SUBDIR = 'tests/fixtures/golden-install-parity'; /** @@ -324,8 +354,13 @@ function resolveBaseSha(base = 'origin/next') { * the same "does not exist in" message — so absence is established with `ls-tree`, which * exits 0 with empty output when the path is simply not there and non-zero on a real * fault. + * + * `repoPath` defaults to the legacy single file, but is generalized (#2914) so the same + * read-at-ref logic serves any one fragment under `ACK_DIR_REPO_PATH` too — there is + * exactly one implementation of "read this ack path at that ref", reused per source + * rather than re-typed per fragment. */ -function readAckFileAtRef(base, { cwd = REPO_ROOT, run = git } = {}) { +function readAckFileAtRef(base, { cwd = REPO_ROOT, run = git, repoPath = ACK_REPO_PATH } = {}) { // `execFileSync`'s array form stops SHELL metacharacters but not git's own option // parsing: a ref beginning with `-` is read as an option token, and `git show` honors // diff options including `--output=`, which writes. Today every caller passes a @@ -340,7 +375,7 @@ function readAckFileAtRef(base, { cwd = REPO_ROOT, run = git } = {}) { let listing; try { - listing = run(['ls-tree', '--name-only', base, '--', ACK_REPO_PATH], { cwd }); + listing = run(['ls-tree', '--name-only', base, '--', repoPath], { cwd }); } catch (err) { throw new Error( `emitted-attribution: could not list the ack at "${base}": ${err.message}. This is a ` @@ -352,24 +387,131 @@ function readAckFileAtRef(base, { cwd = REPO_ROOT, run = git } = {}) { let raw; try { - raw = run(['show', `${base}:${ACK_REPO_PATH}`], { cwd }); + raw = run(['show', `${base}:${repoPath}`], { cwd }); } catch (err) { throw new Error( - `emitted-attribution: ${ACK_REPO_PATH} exists at "${base}" but could not be read: ${err.message}`, + `emitted-attribution: ${repoPath} exists at "${base}" but could not be read: ${err.message}`, ); } if (raw.trim() === '') { - throw new Error(`emitted-attribution: ${ACK_REPO_PATH} is present at "${base}" but empty`); + throw new Error(`emitted-attribution: ${repoPath} is present at "${base}" but empty`); } try { return JSON.parse(raw); } catch (err) { throw new Error( - `emitted-attribution: ${ACK_REPO_PATH} at "${base}" is not valid JSON: ${err.message}`, + `emitted-attribution: ${repoPath} at "${base}" is not valid JSON: ${err.message}`, ); } } +/** + * Fragment filenames present under `ACK_DIR_REPO_PATH` AT `base`, sorted. + * + * Mirrors `readAckFileAtRef`'s absence handling: `ls-tree` on a directory that does not + * exist at that ref exits 0 with empty output, which this reads as "no fragments there" + * — the healthy steady state, not a fault. A genuine git failure (bad ref, corrupt + * object) still throws, for the same reason `readAckFileAtRef` throws on one: silently + * reading "could not list" as "nothing there" would leave every fragment ack able to + * consume a delta it should not. + */ +function listAckFragmentFilesAtRef(base, { cwd = REPO_ROOT, run = git } = {}) { + let out; + try { + out = run(['ls-tree', '--name-only', base, '--', `${ACK_DIR_REPO_PATH}/`], { cwd }); + } catch (err) { + throw new Error( + `emitted-attribution: could not list ${ACK_DIR_REPO_PATH}/ at "${base}": ${err.message}.`, + ); + } + const names = out + .split('\n') + .map((line) => line.trim()) + .filter(Boolean) + .filter((p) => p.endsWith('.json')) + .map((p) => p.slice(p.lastIndexOf('/') + 1)) + .sort(); + return assertFragmentCountWithinCap(`${ACK_DIR_REPO_PATH}/ at "${base}"`, names); +} + +/** + * Fragment filenames present under `ACK_DIR` on THIS tree (the working copy), sorted. + * Absent directory == zero fragments, the healthy steady state — not a fault. + */ +function listAckFragmentFiles(dir = ACK_DIR) { + if (!fs.existsSync(dir)) return []; + const names = fs.readdirSync(dir) + .filter((name) => name.endsWith('.json')) + .sort(); + return assertFragmentCountWithinCap(dir, names); +} + +/** + * Read + union every ack source on THIS tree (#2914): the legacy single file (if + * present) plus every fragment under `ACK_DIR`. Reuses `readAckFile` per physical file + * (same absent/empty/unparseable rules for a fragment as for the legacy file — one + * definition, not a second one per source) and `mergeAckSources` (tests/helpers/ + * emitted-diff.cjs) for the union + duplicate-key detection. + * + * Returns `{ doc: null, errors: [] }` only when NEITHER the legacy file nor any + * fragment exists — the healthy steady state matching `readAckFile`'s own `null` + * contract, so callers can keep testing `ack === null` to decide whether the base side + * needs consulting at all (avoiding the deadlock `readAckFileAtRef`'s doc comment + * describes for a corrupt base). + * + * @returns {{ doc: {version: number, paths: object} | null, errors: string[] }} + */ +function readAckSources({ legacyPath = ACK_PATH, fragmentsDir = ACK_DIR } = {}) { + const docs = []; + if (fs.existsSync(legacyPath)) { + docs.push({ source: ACK_REPO_PATH, doc: readAckFile(legacyPath) }); + } + for (const name of listAckFragmentFiles(fragmentsDir)) { + docs.push({ + source: `${ACK_DIR_REPO_PATH}/${name}`, + doc: readAckFile(path.join(fragmentsDir, name)), + }); + } + if (docs.length === 0) return { doc: null, errors: [] }; + const { merged, errors } = mergeAckSources(docs); + return { doc: merged, errors }; +} + +/** + * Read + union every ack source AT `base` (#2914): the legacy single file plus every + * fragment, as they existed at that ref. Mirrors `readAckSources` above, one ref-read + * per physical source via `readAckFileAtRef`'s now-generalized `repoPath` option. + * + * Base-side merge/schema errors are DELIBERATELY DISCARDED, matching this module's + * existing precedent for the base side (see `diffEmitted`'s caller below: "Base-side + * SCHEMA errors are deliberately discarded... a document we cannot read simply inherits + * nothing — which is the ARMED reading"). A cross-fragment collision found only at the + * base is `next`'s own health, not this diff's to answer for; `mergeAckSources`'s + * first-source-wins fallback for a duplicate key is still the STRICT reading here (an + * entry can only be "spent" against the ONE reason kept, never either of two), so + * discarding the error text costs no protection while avoiding a lint-clean PR being + * blocked by a historical duplicate it did not introduce and cannot fix by itself. + * + * A genuine READ failure (corrupt JSON, unreadable object) on any single source still + * throws, exactly as `readAckFileAtRef` already does — only the schema/collision + * bookkeeping is discarded, never a fault. + * + * @returns {{ doc: {version: number, paths: object} | null }} + */ +function readAckSourcesAtRef(base, { cwd = REPO_ROOT, run = git } = {}) { + const docs = []; + const legacyDoc = readAckFileAtRef(base, { cwd, run }); + if (legacyDoc !== null) docs.push({ source: ACK_REPO_PATH, doc: legacyDoc }); + for (const name of listAckFragmentFilesAtRef(base, { cwd, run })) { + const relPath = `${ACK_DIR_REPO_PATH}/${name}`; + const doc = readAckFileAtRef(base, { cwd, run, repoPath: relPath }); + if (doc !== null) docs.push({ source: relPath, doc }); + } + if (docs.length === 0) return { doc: null }; + const { merged } = mergeAckSources(docs); + return { doc: merged }; +} + /** * Base-ref candidates, most-specific first. * @@ -755,7 +897,13 @@ module.exports = { REPO_ROOT, ACK_PATH, ACK_REPO_PATH, + ACK_DIR, + ACK_DIR_REPO_PATH, readAckFileAtRef, + listAckFragmentFiles, + listAckFragmentFilesAtRef, + readAckSources, + readAckSourcesAtRef, FIXTURE_SUBDIR, MANIFEST_FAMILIES, MINIMUM_MANIFEST_FAMILIES,