* test(#3942): failing-first suite for the emitted-drift ack commit trailer Binds 37 input classes from the phase test matrix to the behavior ADR-3942 specifies, before any of it exists. Stubs return benign empty values rather than throwing, deliberately: several rows assert that something DOES throw (cap overflow, uncomputable commit range), and a throwing stub would turn those green for the wrong reason and destroy the red. The two rows that carry the design's load: - merge-base semantics. The range is $(git merge-base base HEAD)..HEAD, not base..HEAD, because changedPaths comes from `git diff base...HEAD` (three dot). Two-dot would let the ack set and the change set disagree about which commits are this PR's. The fixture forks a topic branch, puts a trailer on each side, and asserts only the topic-side trailer is in range. - fail-closed on an uncomputable range. With fragments a depth-1 checkout passes VACUOUSLY, every fragment reading as brand-new. With trailers the range cannot be computed at all, and returning an empty set would silently disarm the gate, so it must throw. The fixture builds a genuine shallow clone rather than simulating one. Also covers the self-inflicted case: this change's own documentation quotes the trailer syntax, so an example landing at the end of a commit message would arm a live acknowledgment keyed on the literal placeholder text. Keys carrying angle brackets or whitespace are rejected. Authored per the phase artifacts 40-design.md and 50-test-matrix.md. Not yet run on the remote runner — this commit exists to be tested. Refs #3942 * chore(#3942): move the emitted-drift ack to a commit trailer Implements ADR-3942, superseding ADR-2719 section 3 and its #2789 amendment. Sections 1, 2 and 4-7 are retained: the conservation law is unchanged, only the storage of its escape hatch moved off the working tree. An acknowledgment explains one PR's ripple, and the moment that PR merges the ripple is in the base, so it can never clear anything again. It was stored in permanent shared state anyway, and every consequence of that mismatch had to be built and then maintained. The chain is #2789 -> #2914 -> #3078 -> #3842 -> #3823 -> #3875, each fix generating the next defect, ending in a scheduled sweeper whose own first PR could not merge itself. Added parseAckTrailers + renderAckTrailer (pure) and readAckTrailers (IO shell), reading Emitted-Drift-Ack-Hash: / Emitted-Drift-Ack-Growth: trailers over the merge-base range. tests/emitted-ack-trailer.test.cjs, 37 cases, written failing-first and confirmed red before any of this existed. Changed diffEmitted takes two structurally distinct key-space maps instead of one shared paths map. That closes a latent defect: the spaces were separated by convention only, so a growth key satisfied a hash lookup by naming coincidence. staleAcks now reports which space a key was declared in. REMEDIATION teaches the trailer, per space, with its example rendered through renderAckTrailer so the taught grammar cannot drift from what the parser accepts. Removed the sweep workflow, the guard-no-ack-on-next job, the standalone linter and its lint:ci entry, the fragment directory and its three spent fragments, the legacy single-file union, and the baseAck/spentAcks mechanism -- spentness is now structural, not computed. Two range properties carry the design and are pinned by tests rather than asserted: the range is merge-base scoped, matching git diff base...HEAD, so an already-merged trailer is out of range by construction; and an uncomputable range throws instead of reading as zero acknowledgments, which is the inverse of the fragment guard's vacuous pass. Three deliberate observable changes, each disclosed in the changeset: the unread runtime field is gone, the legacy file is no longer read, and cross-space excusal no longer works. Ten open PRs carry fragments and will meet a modify/delete conflict. Measured before landing and accepted deliberately; the one-line migration is in the PR body. Verified: lint:ci exit 0. Remote runner to follow on this exact sha. Refs #3942 * fix(#3942): silent trailer collapse, lost coverage, and an unbounded cap Six findings from the orthogonal review round, all fixed in place. BLOCKER -- two trailers of the same name on one commit collapsed silently. readAckTrailers built `separator=1d` where git needs `separator=%x1d`: the `separator=` value inside a %(trailers:...) placeholder is itself a pretty-format string, so the bare hex was emitted as two literal characters and the split on \x1d never matched. Two same-name trailers therefore joined into one value with errors empty -- the first reason absorbing the second entry's key. Silent truncation, the exact class MAX_ACK_TRAILERS throws to prevent. Confirmed with od -c against real git output before and after. The failing-first matrix did not catch it because its "both spaces coexist" row uses Hash plus Growth -- different trailer NAMES -- so the value separator was never exercised. Two regression tests now cover same-name trailers directly. Coverage recovered: normalizeAckReason and INVISIBLE stayed on the live path via parseAckTrailers but lost every test when the old suite was pruned. Back under test against the current surface -- all six invisible codepoints individually, whitespace collapse, trim, CRLF, and two seeded fast-check properties. Dropping any single codepoint now fails. MAX_ACK_TRAILERS counted raw trailers before de-duplication, so one trailer carried forward across rebased commits counted once per commit and could throw on a legitimate branch. Now counts distinct entries; 100 identical repeats dedupe to one. diffEmitted validated baseline, current and changedPaths but not the new ackHash/ackGrowth, so a bad shape raised an unhandled TypeError instead of an error verdict -- the same defect shape this file documents for #2778. Docs: CONTRIBUTING and TESTING-SUITES were rewritten only in their first sections; the later passages still taught fragments, git rm and the deleted guard, contradicting the new text directly above them. Finished. Also extends lint-removed-but-needed to exempt docs/adr and docs/research. That gate fails on any docs mention of a file deleted in the same diff, which makes it impossible to document a deletion in the PR performing it -- an ADR's whole job is naming what it retired. Exemption is narrow and comes with a test proving the gate still fires for a live consumer elsewhere under docs/. A guard that cannot fail is worse than no guard. Maintainer-approved. CONTEXT.md names the retired machinery by role rather than by filename: its generated projection lands in docs/, which that gate does scan. Adds docs/how-to/acknowledge-emitted-drift.md. The required docs set is Reference and Explanation, so the task quadrant can be empty with every gate green -- and this change has a real multi-step journey, including the fragment migration ten open PRs now need. lint:ci exit 0. Refs #3942 * docs(#3942): correct the duplicate-trailer rule in CONTRIBUTING Both axes of the code review independently flagged the same passage, without seeing each other's output. It claimed two declarations of the same key are always "a hard, loudly-reported error, not a silent last-wins". That is only half true, and the missing half is the one contributors hit: identical declarations -- same key, same reason -- dedupe silently, because a trailer legitimately survives a rebase and reappears on every rebased commit. Failing there would red a branch for doing nothing wrong, which is exactly why the dedup exists. Only a same-key/different-reason pair errors, and that one is a genuine ambiguity about which explanation holds. As written, the paragraph told a contributor that a rebase-carried trailer breaks the gate -- the opposite of the behavior. CONTEXT.md's parallel entry already stated it correctly; this brings CONTRIBUTING into line. Doc-only, root-level markdown. Refs #3942 * chore(#3942): backfill changeset PR number to 3954 --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
123
CONTRIBUTING.md
123
CONTRIBUTING.md
@@ -337,9 +337,7 @@ several PRs in flight everyone picked the same next integer, and two PRs adding
|
||||
165 → 166 → 167 → 168 across successive rebases — the last collision landing
|
||||
*during* a verification run — and because every rebase invalidates the sha-keyed
|
||||
pass marker, each collision also cost a full remote matrix run. This is the same
|
||||
fix `.changeset/` already applies to `CHANGELOG.md` and
|
||||
`tests/emitted-drift-acks/` applies to the ack ledger
|
||||
([#2914](https://github.com/open-gsd/gsd-core/issues/2914)): one file per
|
||||
fix `.changeset/` already applies to `CHANGELOG.md`: one file per
|
||||
contribution, consolidated by a generator. You add a new file and touch no
|
||||
shared file, so there is nothing to collide on.
|
||||
|
||||
@@ -1061,86 +1059,54 @@ 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 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
|
||||
a converter change, for example — go through a **commit trailer on one of your own
|
||||
commits** (ADR-3942; name the key, say why):
|
||||
|
||||
```
|
||||
Emitted-Drift-Ack-Hash: skills/gsd-add-tests/SKILL.md — the converter rewrote every skill header
|
||||
Emitted-Drift-Ack-Growth: explore.md — new dispatch section, reasoning ships with the block
|
||||
```
|
||||
|
||||
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. 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.
|
||||
and still apply.
|
||||
|
||||
**Why a trailer and not a file (ADR-3942).** An acknowledgment explains one PR's ripple.
|
||||
The moment that PR merges the ripple is in the base, so the acknowledgment can never clear
|
||||
anything again — its useful life is exactly your PR's open window. Storing it in the
|
||||
working tree meant storing PR-lifetime data in permanent shared state, and every
|
||||
consequence of that mismatch had to be built and then maintained: a guard to detect spent
|
||||
files on `next`, a scheduled bot to delete them, a hold so the bot did not conflict
|
||||
in-flight PRs, and a shared key namespace that walled off the next PR to touch the same
|
||||
path. A trailer has no file, so it has no merge-conflict surface, never becomes spent, and
|
||||
needs no garbage collector. The trailer is read from `git log $(git merge-base <base>
|
||||
HEAD)..HEAD` — your commits and no others — which is the same merge-base the differential
|
||||
check already uses to compute what your PR changed.
|
||||
|
||||
This is **not** a verdict on `.changeset/` or `tests/qa/smell-acks/`, which use the
|
||||
fragment idiom correctly: a changeset and a smell acknowledgment stay meaningful after
|
||||
merge, so durable state is the right home for them. Only the emitted-drift ack was spent
|
||||
on arrival.
|
||||
|
||||
You do not need to memorize any of this. **The failure output names its own remedy** — it
|
||||
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
|
||||
tells you which key to add and prints a minimal trailer line you can paste onto one of your
|
||||
commits. 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 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.
|
||||
appears under `gsd-core/workflows/` or `agents/` (`explore.md`). The two spaces are
|
||||
structurally distinct — a `Growth` trailer never excuses a `Hash` ripple, even when the key
|
||||
text happens to match.
|
||||
|
||||
**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. 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.
|
||||
|
||||
**When two sources collide (#3078).** The error names both resolutions, because which one
|
||||
applies depends on the fragment that already owns the path. If the owning fragment is
|
||||
already **merged**, its entry is spent — it is at the base, so it gates nothing — and the
|
||||
answer is to delete it (`git rm tests/emitted-drift-acks/<owner>.json`) and keep your own.
|
||||
If it is still **live** on your branch, append your explanation to its existing entry
|
||||
instead; that re-arms it, and re-arming deliberately costs an actual new sentence, because
|
||||
the reason is the whole artifact a reviewer reads. Never rename the path to dodge the
|
||||
error, and never declare it twice.
|
||||
|
||||
**Neither ack source may persist on `next`, and a fragment is not exempt (#3078).**
|
||||
`tests/emitted-drift-ack.json`, the legacy single file, must never survive there at all:
|
||||
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** is judged on a different rule but the
|
||||
same law. #2914 originally exempted the fragment directory on the premise that a persisting
|
||||
fragment is harmless, since fragments are independently named and cannot conflict. That
|
||||
premise was wrong: fragments do not share a *file*, but they do share a *path key space*,
|
||||
and a path claimed by two sources is the hard error above. So a fully-spent fragment left on
|
||||
`next` owns keys it can no longer gate, and the next PR that grows one of those paths can
|
||||
declare it neither there (spent) nor in its own fragment (duplicate) — the exact wall #2914
|
||||
removed for the legacy file, one level down. A fragment is therefore swept once **every**
|
||||
entry in it is spent; a **partially** spent one is left alone, which is what keeps the
|
||||
re-arm-by-appending route above working. Both rules are 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), which means the job alerts **after** the merge and cannot
|
||||
stop the offending PR — that is why the collision error above has to teach the resolution
|
||||
too. If you ever see the legacy file present on `next`, delete it; do not try to make it
|
||||
well-formed. If the job names a spent fragment, run the `git rm` it prints — that is the
|
||||
whole remedy, and there is nothing to regenerate.
|
||||
|
||||
**The sweep is staged around open PRs, not unconditional (#3842).** A fragment being
|
||||
all-spent is necessary but not sufficient to sweep it: deleting a fragment that an OPEN
|
||||
pull request still modifies hands that PR a `modify/delete` conflict on its very next
|
||||
merge attempt — precisely the shared-file conflict fragments were adopted to end, just
|
||||
reintroduced by the sweep itself. This actually happened the first time the sweep ran:
|
||||
#3330, #3774, and #3648 all conflicted simultaneously, each with the swept fragment as its
|
||||
*only* conflicting path, all three outside contributors. So `--guard-next` now also takes
|
||||
`--defer-to-open-prs` (passed by the `guard-no-ack-on-next` job): it runs one `gh pr list
|
||||
--json number,files` call, and any all-spent fragment an open PR's file list still names
|
||||
is *held* rather than swept — reported informationally in the job output, never as a
|
||||
failure — until that PR merges or closes. If the open-PR lookup itself fails (auth,
|
||||
network, rate limit), every otherwise-sweepable fragment is held for that run rather than
|
||||
swept blind; the next push to `next` tries again. A fragment fully spent AND untouched by
|
||||
any open PR sweeps exactly as before — this changes *when* a spent fragment is removed,
|
||||
never what "spent" means.
|
||||
**Declaring the same key twice is fine if you say the same thing twice.** Identical
|
||||
declarations — same key, same reason — are de-duplicated silently, because a trailer
|
||||
legitimately survives a rebase and reappears on every rebased commit; failing there would
|
||||
red a branch for doing nothing wrong. Two declarations of the same key with *different*
|
||||
reasons are a hard, loudly-reported error: that is a genuine ambiguity about which
|
||||
explanation holds, and only you can say which. There is no "which source owns the key"
|
||||
question underneath it, because there is no shared file for two sources to own — to change
|
||||
an acknowledgment, amend the commit carrying it.
|
||||
|
||||
`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,
|
||||
@@ -1389,9 +1355,8 @@ 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 a per-PR fragment in
|
||||
tests/emitted-drift-acks/ (#2914), no committed
|
||||
snapshot to regenerate. Loose tier
|
||||
requires an Emitted-Drift-Ack-Growth commit trailer
|
||||
(ADR-3942), 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 +
|
||||
|
||||
Reference in New Issue
Block a user