golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests 1 Actions 1 Packages Projects Releases Wiki Activity

fix(#2789): scope the emitted-drift ack to the diff that introduced it (#2803)

Browse Source 
* fix(#2789): scope the emitted-drift ack to the diff that introduced it

Every input to `diffEmitted` is base-relative -- `baseline` vs `current`,
`changedPaths` from `git diff base...HEAD` -- except the ack set, which
was read absolutely, from the working tree only. A differential machine
consulting a non-differential input.

So `staleAcks` asks exactly one question, "did a delta consume you?", and
that cannot distinguish an ack that never explained anything (an
authoring mistake) from one whose ripple is now absorbed into the base
(the ack's SUCCESS condition). After merge an ack is in the second state
but reports as the first.

The trigger is ordinary. Actions sets GITHUB_BASE_REF on pull_request
events only, so a push to `next` falls through to origin/next -- the very
commit under test. Both sides build identical content, no deltas remain,
and every live ack is reported stale. PR #2768 acked a deliberate 40866
-> 42020 byte growth, was green on its own lane, and reddened `next` the
moment it merged. It also reds every PR branching off the poisoned base,
and since publish-emitted-baseline is gated on the test job, it blocked
baseline publication too.

Give the ack the base side it was missing. `diffEmitted` now takes
`baseAck` -- the same document at the base ref, via `readAckFileAtRef`.
An entry already present there is SPENT: it may no longer consume a delta
and is never reported stale, only surfaced as `spentAcks` for tidying. An
entry new or reworded in this diff stays live, and if nothing consumes it
that genuinely fails, with blame on the author who just wrote it.

This closes a hazard the IMPLEMENTATION named but could not prevent -- a
leftover ack silently pre-clearing the next ripple on its path. (ADR-2719
§3 asserted only that TOUCHING the file is the alarm; its residual-risk
list never covered pre-clearing, and §3 now carries an amendment.)
Verified against the two-PR laundering sequence -- land an innocuous ack,
then change the artifact -- which passed silently before and now fails on
both the hash pass and the size ratchet.

Three things the design has to get right, each of which was wrong first:

  - A read failure on the base document THROWS; only absence-at-the-ref
    returns null. Returning null on error LOOKS armed (every entry stays
    live) but a live entry's defining power is that it CONSUMES a delta,
    so null is armed on the staleness axis and DISARMED on consumption --
    silently the whole pre-#2789 gate. `git show` cannot tell absence
    from fault, so absence is established with `ls-tree`.
  - Re-arming a spent ack costs actual PROSE. Internal whitespace and the
    zero-width family collapse, and `runtime` is not compared: a doubled
    space, an invisible character, or a decorative field would otherwise
    re-arm an ack whose justification still describes the previous
    ripple, showing a reviewer nothing.
  - `baseAck` is REQUIRED once an ack declares entries -- omission is an
    error, not a silent "inherit nothing" -- so a dropped argument fails
    loudly instead of quietly restoring this bug with the suite green.

Because a corrupt document ON THE BASE is expensive (the loud base-side
failure reds every ack-carrying PR), scripts/lint-emitted-drift-ack.cjs
blocks one from landing. It is standalone rather than importing parseAck
-- scripts/ ships in the npm package and tests/ does not -- so a parity
test runs both surfaces over one corpus and fails on divergence; it
caught one immediately, a `null` document, now classed as policy rather
than schema. Deadlock is separately foreclosed: a tree carrying no ack
never reads the base, so the PR that DELETES a corrupt file still lands.

`readAckFileAtRef` takes an injected git runner so all four branches are
tested deterministically; it never executes in the remote runner, where
the real-tree test skips for want of a base ref. It also refuses an
option-shaped ref, since execFileSync's array form stops shell
metacharacters but not git's own option parsing.

Rejected: skipping the differential when base == HEAD. It treats the
symptom, costs real coverage on the push-to-next lane, and does nothing
about the downstream PRs the same flaw was reddening.

Deletes the now-spent tests/emitted-drift-ack.json, and updates the
CONTEXT.md canon and ADR-2719 §3: presence is no longer the alarm -- a
LIVE entry is, and a spent one is inert.

Closes #2789

* chore(#2789): backfill changeset PR number
This commit is contained in:
Tom Boucher
2026-07-28 21:28:10 -04:00
committed by GitHub
parent d626dbc6e3
commit 1e3c995e6f
8 changed files with 870 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/curious-newts-swim.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 2803
---
**Merging an emitted-drift acknowledgment no longer turns the mainline red.** An acknowledgment is now scoped to the diff that introduced it, so once its ripple is absorbed into the base it goes inert instead of reporting as stale — which had reddened `next` for five consecutive commits and every pull request branching off it. (#2789)

4
CONTEXT.md
View File

File diff suppressed because one or more lines are too long

4
docs/adr/2719-emitted-artifact-attribution.md
View File

@@ -75,6 +75,10 @@ The design property that matters: the acknowledgment file appears in the changed
This does not make a bad change impossible. It converts a silent regeneration into a conspicuous declaration. That is the intended strength, stated plainly rather than overclaimed.
> **Amendment (#2789):** *touching* the acknowledgment is still the alarm, and is now strictly harder to fake. Mere **presence** is not, and treating it as such was a real defect. The document was read only from the working tree while every other input to the law is base-relative, so `staleAcks` — "acks no delta consumed" — could not tell an ack that never explained anything from one whose ripple had been **absorbed into the base**, which is the ack's success condition. Merging an acknowledgment therefore reddened `next` and every PR branching off it (#2768).
>
> An ack is now scoped to the diff that introduced it: `diffEmitted` also takes the document at the base ref, and an entry already present there is **spent** — it can no longer consume a delta and is never reported stale, only listed for tidying. New or reworded entries stay live, and re-arming costs actual prose (internal whitespace, invisible characters and the unread `runtime` field are all normalized away), so the conspicuous declaration this section asks for cannot be forged with a zero-information edit. That also closes a hazard the *implementation* named but could not prevent — a leftover ack silently pre-clearing the next ripple on its path; note this ADR's own residual-risk list never covered it. `scripts/lint-emitted-drift-ack.cjs` refuses the merge if a malformed document would reach the base, where the base-side reader's deliberate hard failure is expensive.
### 4. The size ratchet folds into the same machine
`workflow-size-baseline.json` conflicts on 7 of 7 — deleting only the golden fixtures would leave every affected PR still blocked. The attribution law does not transfer to it (growth is trivially attributable to the edit that caused it), so instead the same differential machine reports growth with exact byte deltas, and growth requires the same acknowledgment entry.

2
package.json
View File

@@ -104,7 +104,7 @@
"lint": "eslint . --cache --cache-location node_modules/.cache/eslint/",
"lint:fix": "eslint . --fix",
"lint:table-schema-drift": "node scripts/lint-table-schema-drift.cjs",
"lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs",
"lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs",
"lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs",
"lint:regression-names": "node scripts/lint-regression-test-names.cjs",
"lint:descriptions": "node scripts/lint-descriptions.cjs",

149
scripts/lint-emitted-drift-ack.cjs Normal file
View File

@@ -0,0 +1,149 @@
#!/usr/bin/env node
'use strict';
/**
* lint-emitted-drift-ack — refuse to merge a broken emitted-drift acknowledgment (#2789).
*
* The ack document is read from TWO sides: the working tree (`readAckFile`) and the BASE
* REF (`readAckFileAtRef`), because an entry already present at the base is spent and may
* no longer clear a delta. The base-side read fails LOUDLY on a document it cannot parse
* — it has to, since silently inheriting nothing would leave every entry able to consume
* a delta, which is the pre-#2789 gate.
*
* That makes a corrupt document ON THE BASE BRANCH unusually expensive: it reds every PR
* that carries an ack until someone repairs it. The real-tree test avoids an outright
* deadlock (a tree with no ack never consults the base, so the repair PR still lands),
* but the cheaper answer is to never let a broken document reach the base at all. This
* runs in `lint:ci`, so a PR carrying one cannot go green and cannot merge.
*
* This validator is DELIBERATELY STANDALONE. `scripts/` ships in the npm package and
* `tests/` does not (package.json `files`), so requiring the gate's own `parseAck` from
* 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.
*/
const fs = require('node:fs');
const path = require('node:path');
const ACK_VERSION = 1;
const ACK_REPO_PATH = 'tests/emitted-drift-ack.json';
const REPO_ROOT = path.join(__dirname, '..');
const isPlainObject = (v) => v !== null && typeof v === 'object' && !Array.isArray(v);
/**
* Validate an ack document's raw text.
*
* `schemaErrors` are the ones that must agree with the gate's `parseAck` — the shape
* contract. `policyErrors` are lint-only rules that `parseAck` deliberately does NOT
* enforce, because they are about what may be COMMITTED rather than what may be parsed:
* a present-but-entryless document parses fine and signals nothing, so it must be deleted
* rather than left behind.
*
* @param {string|null} raw file contents, or null when the file is absent
* @returns {{ schemaErrors: string[], policyErrors: string[], ok: boolean }}
*/
function validateAckText(raw) {
const schemaErrors = [];
const policyErrors = [];
const done = () => ({ schemaErrors, policyErrors, ok: schemaErrors.length === 0 && policyErrors.length === 0 });
if (raw === null) return done(); // absent is the healthy steady state
if (raw.trim() === '') {
schemaErrors.push(`${ACK_REPO_PATH} is present but empty`);
return done();
}
let doc;
try {
doc = JSON.parse(raw);
} catch (err) {
schemaErrors.push(`${ACK_REPO_PATH} is not valid JSON: ${err.message}`);
return done();
}
// A document that is literally `null` is POLICY, not schema. The gate's `parseAck` uses
// `null` as its "absent == no acks" sentinel, so it reads such a file as legal and
// harmless — and the parity test holds us to that. It is still not something to commit:
// 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 — `
+ 'the healthy steady state is no file at all.',
);
return done();
}
if (!isPlainObject(doc)) {
schemaErrors.push(
`${ACK_REPO_PATH}: 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})`,
);
}
const paths = doc.paths;
if (paths !== undefined && !isPlainObject(paths)) {
schemaErrors.push(`${ACK_REPO_PATH}: "paths" must be an object of <emitted path> -> { reason }`);
return done();
}
const entries = paths === undefined ? [] : Object.entries(paths);
for (const [rel, value] of entries) {
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"`);
}
}
// Lint-only. An entryless document parses cleanly and acknowledges nothing, so it is
// pure confusion on the base branch — and it is exactly what a contributor leaves
// 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 `
+ 'empty one signals nothing, and the healthy steady state is no file at all.',
);
}
return done();
}
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];
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}`);
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.',
);
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)`,
);
}
if (require.main === module) main();
module.exports = { validateAckText, ACK_VERSION, ACK_REPO_PATH };

469
tests/emitted-attribution.test.cjs
View File

@@ -47,6 +47,8 @@ const {
currentManifests,
currentSizes,
readAckFile,
readAckFileAtRef,
ACK_REPO_PATH,
baselineFamilyNamesAtRef,
MANIFEST_FAMILIES,
MINIMUM_MANIFEST_FAMILIES,
@@ -58,6 +60,7 @@ 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 {
ACK_VERSION,
ACK_FILE,
@@ -394,6 +397,7 @@ test('an acked ripple passes and is echoed', () => {
current: mf({ [WORKFLOW_KEY]: 'bbb' }),
changedPaths: ['README.md'],
ack,
baseAck: null,
});
assert.equal(r.unattributable.length, 0);
assert.equal(r.acked.length, 1);
@@ -409,6 +413,7 @@ test('a stale ack entry fails', () => {
current: mf({ [WORKFLOW_KEY]: 'aaa' }),
changedPaths: [],
ack,
baseAck: null,
});
assert.deepEqual(r.staleAcks, [WORKFLOW_KEY]);
assert.ok(!r.ok);
@@ -422,6 +427,7 @@ test('an ack without a reason fails', () => {
current: mf({ [WORKFLOW_KEY]: 'bbb' }),
changedPaths: [],
ack: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: bad } },
baseAck: null,
});
assert.ok(!r.ok, `${JSON.stringify(bad)} must be rejected`);
assert.match(r.errors.join('\n'), /has no non-empty "reason"/);
@@ -452,10 +458,443 @@ test('a live ack and a stale ack together: only the stale one is named', () => {
current: mf({ [WORKFLOW_KEY]: 'bbb', [SKILL_KEY]: 'ccc' }),
changedPaths: [],
ack,
baseAck: null,
});
assert.deepEqual(r.staleAcks, [SKILL_KEY], 'the live one must not be named');
});
// ─── Ack lifecycle: an ack is scoped to the diff that introduced it (#2789) ──
//
// Every other input to the law is base-relative — `baseline` vs `current`, `changedPaths`
// from `git diff base...HEAD`. The ack set was the one absolute input, read only from
// HEAD. That mismatch is what made a MERGED ack look identical to a never-explained one:
// both present as "no delta consumed it", so merging an ack the PR lane had accepted
// reddened `next` and every PR branching off it (#2768).
//
// `baseAck` closes it. An entry already present at the base is SPENT — its ripple is
// absorbed, it is not this diff's to answer for, and it may no longer clear anything.
test('an ack already present at the base is spent — not stale, and it does not fail', () => {
// The #2768 shape exactly: the ack merged, so the base carries it and no delta remains.
const ack = { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'deliberate growth' } } };
const r = diffEmitted({
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'aaa' }),
changedPaths: [],
ack,
baseAck: ack,
});
assert.deepEqual(r.staleAcks, [], 'an absorbed ripple is the ack SUCCEEDING, not failing');
assert.deepEqual(r.spentAcks, [WORKFLOW_KEY], 'still surfaced, so it can be cleaned up');
assert.ok(r.ok);
});
test('a spent ack cannot pre-clear a NEW ripple on its own path', () => {
// ADR-2719's own named hazard. Today a leftover ack silently clears the next ripple;
// scoped to its diff it cannot, so the new ripple must be explained on its own terms.
const ack = { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'last time' } } };
const r = diffEmitted({
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'bbb' }), // a genuinely new, unexplained move
changedPaths: [],
ack,
baseAck: ack,
});
assert.equal(r.acked.length, 0, 'a spent ack must not absorb a new ripple');
assert.equal(r.unattributable.length, 1);
assert.ok(!r.ok);
});
test('re-arming a spent ack costs actual prose — not whitespace, not a decorative field', () => {
// Re-arming is legitimate; it is how a contributor says "this is a NEW ripple, and here
// is why". But the reason is the whole artifact a reviewer reads, so it must cost a
// real explanation. Both of these once re-armed an ack whose justification still
// described the PREVIOUS ripple, showing a reviewer nothing new in the ack file's diff.
const base = { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'the same words' } } };
const newRipple = {
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'bbb' }), // genuinely new and unexplained
changedPaths: [],
baseAck: base,
};
const doubledSpace = diffEmitted({
...newRipple,
ack: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'the same words' } } },
});
assert.equal(doubledSpace.acked.length, 0, 'internal whitespace must not re-arm');
assert.ok(!doubledSpace.ok);
const decoratedField = diffEmitted({
...newRipple,
ack: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'the same words', runtime: 'claude' } } },
});
assert.equal(decoratedField.acked.length, 0, 'an unrelated field must not re-arm');
assert.ok(!decoratedField.ok);
// …while genuinely new prose still does.
const reworded = diffEmitted({
...newRipple,
ack: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'a different, specific explanation' } } },
});
assert.equal(reworded.acked.length, 1);
assert.ok(reworded.ok);
});
test('spent entries are reported sorted, and modelled in buildReport not just rendered', () => {
// Insertion order is deliberately REVERSE-sorted (`skills/…` before `gsd-core/…`), so
// the assertion bites: comparing against a sorted copy of the result would pass even
// with the sort deleted, and asserting on an already-ordered fixture proves nothing.
const both = {
version: ACK_VERSION,
paths: { [SKILL_KEY]: { reason: 'second' }, [WORKFLOW_KEY]: { reason: 'first' } },
};
assert.ok(SKILL_KEY > WORKFLOW_KEY, 'the fixture must be inserted out of order to be a real test');
const r = diffEmitted({
baseline: mf({ 'gsd-core/workflows/zzz.md': 'aaa' }),
current: mf({ 'gsd-core/workflows/zzz.md': 'bbb' }), // an unrelated failure to render under
changedPaths: [],
ack: both,
baseAck: both,
});
assert.deepEqual(r.spentAcks, [WORKFLOW_KEY, SKILL_KEY], 'spent entries must come back sorted');
const block = buildReport(r).blocks.find((b) => b.kind === 'spent-acks');
assert.ok(block, 'spent acks must be modelled in the IR, so tests need no raw text matching');
assert.equal(block.count, 2);
assert.deepEqual(block.items, r.spentAcks);
});
test('buildReport and formatReport agree about spent acks on a PASSING run', () => {
// `formatReport` is documented as a pure rendering of `buildReport`. The spent section
// is the one block whose emit-condition could drift, because a passing run must render
// nothing — so the IR must withhold it there too, or a JSON reporter built on the IR
// would report spent acks for a green run while the text reporter stayed silent.
const spent = { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'absorbed' } } };
const passing = diffEmitted({
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'aaa' }),
changedPaths: [],
ack: spent,
baseAck: spent,
});
assert.ok(passing.ok);
assert.deepEqual(passing.spentAcks, [WORKFLOW_KEY], 'the datum is still on the result object');
assert.equal(formatReport(passing), '');
assert.equal(
buildReport(passing).blocks.find((b) => b.kind === 'spent-acks'),
undefined,
'the IR must not carry a block the renderer suppresses',
);
});
test('ackDocument survives a __proto__ key instead of silently teaching an empty document', () => {
// `key` comes from repo/emitted paths. On a plain object `__proto__` sets the prototype
// rather than a property, so JSON.stringify would emit `"paths":{}` — remediation text
// that teaches the contributor to acknowledge nothing at all.
const doc = JSON.parse(REMEDIATION.ackDocument([
{ key: '__proto__', reason: 'hostile key' },
{ key: 'plan-phase.md', reason: 'ordinary key' },
]));
assert.deepEqual(Object.keys(doc.paths).sort(), ['__proto__', 'plan-phase.md']);
assert.equal(doc.paths.__proto__.reason, 'hostile key');
assert.equal(({}).reason, undefined, 'Object.prototype must be untouched');
});
test('a clean run renders NOTHING, even when spent entries exist', () => {
// `formatReport` returning prose for an ok result reads as "something is wrong".
const spent = { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'absorbed' } } };
const r = diffEmitted({
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'aaa' }),
changedPaths: [],
ack: spent,
baseAck: spent,
});
assert.ok(r.ok);
assert.deepEqual(r.spentAcks, [WORKFLOW_KEY]);
assert.equal(formatReport(r), '', 'a passing run must render an empty report');
});
test('an ack whose reason changed in this diff is live again', () => {
const r = diffEmitted({
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'bbb' }),
changedPaths: [],
ack: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'THIS ripple, freshly explained' } } },
baseAck: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'the previous one' } } },
});
assert.equal(r.acked.length, 1, 'rewriting the reason re-arms the ack for the new ripple');
assert.deepEqual(r.staleAcks, []);
assert.ok(r.ok);
});
test('an ack absent from the base is live and consumes its ripple', () => {
const r = diffEmitted({
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'bbb' }),
changedPaths: [],
ack: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'new in this PR' } } },
baseAck: { version: ACK_VERSION, paths: { [SKILL_KEY]: { reason: 'unrelated, already merged' } } },
});
assert.equal(r.acked.length, 1);
assert.deepEqual(r.staleAcks, []);
assert.ok(r.ok);
});
test('a LIVE ack that nothing consumes is still stale and still fails', () => {
// The softening must not reach the case the rule exists for: an ack written in THIS
// diff that never explained anything is an authoring mistake, and blame lands right.
const r = diffEmitted({
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'aaa' }),
changedPaths: [],
ack: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'explains nothing' } } },
baseAck: { version: ACK_VERSION, paths: { [SKILL_KEY]: { reason: 'unrelated' } } },
});
assert.deepEqual(r.staleAcks, [WORKFLOW_KEY]);
assert.deepEqual(r.spentAcks, []);
assert.ok(!r.ok);
});
test('an absent or unreadable base ack inherits NOTHING — the gate stays armed', () => {
// Omission is not evidence that an entry was already merged. Every unknown here fails
// toward the strict reading, so a base we could not read cannot excuse a stale ack.
// `undefined` is deliberately NOT in this list: a destructuring default fires on it, so
// it takes the OMITTED path and fails with "baseAck was not supplied" — a different
// rule, covered by its own test above. Including it here would look like coverage of
// the staleness path while asserting something else entirely.
for (const baseAck of [null, {}, { version: ACK_VERSION }, 'not-an-object', 42, []]) {
const r = diffEmitted({
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'aaa' }),
changedPaths: [],
ack: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'x' } } },
baseAck,
});
assert.deepEqual(r.staleAcks, [WORKFLOW_KEY], `baseAck ${JSON.stringify(baseAck)} must inherit nothing`);
assert.ok(!r.ok);
}
});
// ─── Pre-merge lint parity (#2789) ───────────────────────────────────────────
//
// `scripts/lint-emitted-drift-ack.cjs` blocks a broken ack document from ever reaching
// the base branch, where the base-side reader's (correct) loud failure would be
// expensive. It cannot reuse `parseAck`: `scripts/` ships in the npm package and `tests/`
// does not, so requiring across that line would be a MODULE_NOT_FOUND in the published
// package. Two validators of one schema is exactly the divergence this repo requires a
// parity assertion for — so the corpus below runs through BOTH and must get the same
// verdict from each.
test('the pre-merge lint and the gate parser agree on what is schema-valid', () => {
const corpus = [
// [label, raw text, expected schema-valid?]
['absent-equivalent empty object', '{}', true],
['versioned, no paths', '{"version":1}', true],
['empty paths', '{"version":1,"paths":{}}', true],
['one good entry', '{"version":1,"paths":{"a.md":{"reason":"why"}}}', true],
['bare-string reason', '{"version":1,"paths":{"a.md":"why"}}', true],
['no version key', '{"paths":{"a.md":{"reason":"why"}}}', true],
['unknown extra field', '{"version":1,"paths":{"a.md":{"reason":"why","note":"x"}}}', true],
['bad JSON', '{ not json', false],
['array document', '[]', false],
['scalar document', '42', false],
['string document', '"nope"', false],
// NOT here: a document of literally `null`. `parseAck` uses null as its
// "absent == no acks" sentinel and reads it as legal, so it is schema-valid on both
// sides; the lint rejects it on POLICY instead. Covered in the entryless test below.
['wrong version', '{"version":9,"paths":{}}', false],
['paths is an array', '{"version":1,"paths":[]}', false],
['paths is a scalar', '{"version":1,"paths":7}', false],
['empty reason', '{"version":1,"paths":{"a.md":{"reason":""}}}', false],
['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],
];
for (const [label, raw, expectedValid] of corpus) {
const lint = validateAckText(raw);
const lintValid = lint.schemaErrors.length === 0;
// The gate's own parser, fed the same document the same way `readAckFile` would.
let gateValid;
try {
gateValid = parseAck(JSON.parse(raw)).errors.length === 0;
} catch {
gateValid = false; // unparseable JSON never reaches parseAck; readAckFile throws first
}
assert.equal(lintValid, expectedValid, `lint verdict for ${label}`);
assert.equal(
gateValid, lintValid,
`DIVERGENCE on ${label}: the pre-merge lint and parseAck disagree, so one of them `
+ 'would let a document through that the other rejects',
);
}
});
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
// never fails the gate mid-run, while the lint refuses to let one be COMMITTED,
// because it acknowledges nothing and only confuses the next reader.
// `null` belongs here rather than in the schema corpus: it is the gate's own
// "absent == no acks" sentinel, so it is legal to PARSE and still wrong to COMMIT.
for (const raw of ['{}', '{"version":1}', '{"version":1,"paths":{}}', 'null']) {
const r = validateAckText(raw);
assert.deepEqual(r.schemaErrors, [], `${raw} must be schema-valid`);
assert.equal(r.policyErrors.length, 1, `${raw} must trip the delete-the-file policy`);
assert.ok(!r.ok);
assert.deepEqual(parseAck(JSON.parse(raw)).errors, [], `${raw} must stay legal for the gate`);
}
});
test('the lint passes on an absent file — the healthy steady state', () => {
const r = validateAckText(null);
assert.deepEqual(r.schemaErrors, []);
assert.deepEqual(r.policyErrors, []);
assert.ok(r.ok);
});
test('the lint rejects a present-but-empty file rather than reading it as absent', () => {
for (const raw of ['', ' ', '\n\t ']) {
const r = validateAckText(raw);
assert.equal(r.schemaErrors.length, 1, `${JSON.stringify(raw)} must be rejected`);
// Asserting the SPECIFIC message, not just the count: deleting the empty-file branch
// leaves `JSON.parse('')` throwing its own single error, so a bare count passes either
// way and the branch can be removed with no test failing.
assert.match(r.schemaErrors[0], /present but empty/, `${JSON.stringify(raw)} must name emptiness`);
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
// shallow clone has no `origin/*` to resolve. Without these, replacing the body with
// `return null` would fail nothing while silently restoring the pre-#2789 gate. The git
// runner is injected rather than monkeypatched: deterministic on every OS, and no
// dependence on the host repo's actual refs.
const fakeGit = (handlers) => (args) => {
if (args[0] === 'ls-tree') return handlers.lsTree ? handlers.lsTree() : `${ACK_REPO_PATH}\n`;
if (args[0] === 'show') return handlers.show ? handlers.show() : '{}';
throw new Error(`unexpected git call: ${args.join(' ')}`);
};
test('readAckFileAtRef: absent at the ref is the healthy steady state and returns null', () => {
// ls-tree exits 0 with EMPTY output when the path simply is not there.
const doc = readAckFileAtRef(SHA_A, { run: fakeGit({ lsTree: () => '\n' }) });
assert.equal(doc, null);
});
test('readAckFileAtRef: present and valid parses through', () => {
const payload = { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'r' } } };
const doc = readAckFileAtRef(SHA_A, {
run: fakeGit({ show: () => JSON.stringify(payload) }),
});
assert.deepEqual(doc, payload);
});
test('readAckFileAtRef: a READ FAILURE throws — it must never degrade to "inherit nothing"', () => {
// The whole point. Returning null here looks armed (every entry stays live) but a LIVE
// entry is precisely the one that CAN CONSUME a delta, so a genuinely new unexplained
// ripple would come back `acked` instead of `unattributable` — silently the entire
// pre-#2789 gate. Same law as resolveChangedPaths: a failed git read is an error.
const boom = () => { throw new Error('injected git failure'); };
assert.throws(
() => readAckFileAtRef(SHA_A, { run: fakeGit({ lsTree: boom }) }),
/could not list the ack/,
);
assert.throws(
() => readAckFileAtRef(SHA_A, { run: fakeGit({ show: boom }) }),
/exists at .* but could not be read/,
);
});
test('readAckFileAtRef: present but empty or unparseable throws, like the head-side reader', () => {
assert.throws(
() => readAckFileAtRef(SHA_A, { run: fakeGit({ show: () => ' \n' }) }),
/present at .* but empty/,
);
assert.throws(
() => readAckFileAtRef(SHA_A, { run: fakeGit({ show: () => '{ not json' }) }),
/is not valid JSON/,
);
});
test('readAckFileAtRef: refuses an option-shaped ref rather than handing it to git', () => {
// execFileSync's array form stops shell metacharacters but NOT git's option parsing:
// `git show` honors --output=<file>, which writes. The guard belongs with the argument,
// since this helper is exported and its callers are not the only possible ones.
const never = () => { throw new Error('git must not be invoked at all'); };
for (const bad of ['--output=/tmp/pwn', '-next', '--upload-pack=x', '', null, undefined, 42]) {
assert.throws(
() => readAckFileAtRef(bad, { run: fakeGit({ lsTree: never, show: never }) }),
/refusing to read the ack/,
`${JSON.stringify(bad)} must be refused`,
);
}
});
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
// #2768 with every other test still green. Same discipline the module already applies
// to `changedPaths`: a missing input is an error, not an empty set.
const r = diffEmitted({
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'aaa' }),
changedPaths: [],
ack: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'x' } } },
});
assert.ok(!r.ok);
assert.match(r.errors.join('\n'), /baseAck was not supplied/);
});
test('with no ack ENTRIES, baseAck is not required — the healthy steady state stays quiet', () => {
// Absence of an ack file is the normal case for almost every PR. It must not be made
// to carry a new required argument it has no use for — and neither must a document
// that is present but declares nothing, which `parseAck` accepts as legal (see
// 'non-object ack JSON is rejected'). Only a real ENTRY can be spent or live, so only
// a real entry needs the base side.
for (const ack of [undefined, null, {}, { version: ACK_VERSION }, { paths: {} }]) {
const r = diffEmitted({
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'bbb' }),
changedPaths: [WORKFLOW_SRC],
ack,
});
assert.deepEqual(r.errors, [], `ack ${JSON.stringify(ack)} must need no baseAck`);
assert.ok(r.ok);
}
});
test('a spent size-growth ack neither fails nor clears a further growth', () => {
const ack = { version: ACK_VERSION, paths: { 'plan-phase.md': { reason: 'grew once, deliberately' } } };
const shared = {
baseline: mf({ [WORKFLOW_KEY]: 'aaa' }),
current: mf({ [WORKFLOW_KEY]: 'aaa' }),
changedPaths: [],
ack,
baseAck: ack,
};
// Absorbed: base and current agree on size, so there is no growth left to explain.
const settled = diffEmitted({ ...shared, sizeBaseline: { 'plan-phase.md': 5000 }, sizeCurrent: { 'plan-phase.md': 5000 } });
assert.deepEqual(settled.staleAcks, []);
assert.ok(settled.ok);
// A further growth is a NEW ripple: the spent ack must not silently absorb it.
const grewAgain = diffEmitted({ ...shared, sizeBaseline: { 'plan-phase.md': 5000 }, sizeCurrent: { 'plan-phase.md': 5400 } });
assert.equal(grewAgain.grown.length, 1);
assert.equal(grewAgain.grown[0].acked, false, 'a spent ack must not clear a further growth');
assert.ok(!grewAgain.ok);
});
test('non-object ack JSON is rejected, not treated as empty', () => {
// Reading these as "no acks" would SILENTLY DISARM the gate — indistinguishable
// from a healthy run, which is the worst failure available here.
@@ -507,6 +946,7 @@ test('a converter change fails without an ack and passes with one', () => {
const withAck = diffEmitted({
baseline: mf(base), current: mf(moved), changedPaths,
ack: { version: ACK_VERSION, paths },
baseAck: null,
});
assert.equal(withAck.unattributable.length, 0);
assert.equal(withAck.acked.length, 25);
@@ -532,6 +972,7 @@ test('growth is reported with its exact byte delta and needs an ack', () => {
const withAck = diffEmitted({
baseline: mf({}), current: mf({}), changedPaths: [], sizeBaseline, sizeCurrent,
ack: { version: ACK_VERSION, paths: { 'verify-work.md': { reason: 'new UAT section' } } },
baseAck: null,
});
assert.equal(withAck.grown[0].acked, true);
assert.ok(withAck.ok);
@@ -548,6 +989,7 @@ test('an ack consumed by size growth alone is not reported as stale', () => {
sizeBaseline: { 'verify-work.md': 10000 },
sizeCurrent: { 'verify-work.md': 11247 },
ack: { version: ACK_VERSION, paths: { 'verify-work.md': { reason: 'new UAT section' } } },
baseAck: null,
});
assert.deepEqual(r.staleAcks, [], 'a growth-consumed ack is live, not stale');
assert.equal(r.grown[0].acked, true);
@@ -587,6 +1029,10 @@ const growthOnly = (extra = {}) => diffEmitted({
changedPaths: [],
sizeBaseline: { 'explore.md': 11127 },
sizeCurrent: { 'explore.md': 13230 },
// Sits BEFORE the spread so a row can still override it, while every row that passes
// an `ack` inline gets the explicit "nothing inherited from the base" reading rather
// than tripping `diffEmitted`'s required-baseAck error (#2789).
baseAck: null,
...extra,
});
@@ -706,6 +1152,7 @@ test('a stale ack names the file it lives in and the delete-the-file case', () =
current: mf({ [WORKFLOW_KEY]: 'aaa' }),
changedPaths: [],
ack: { version: ACK_VERSION, paths: { [WORKFLOW_KEY]: { reason: 'old' } } },
baseAck: null,
});
const stale = blockOf(buildReport(r), 'stale-acks');
assert.deepEqual(stale.items, [WORKFLOW_KEY]);
@@ -777,6 +1224,7 @@ test('an acked growth produces no block and nothing to acknowledge', () => {
// The contributor already did the thing the remediation asks for; repeating it is noise.
const r = growthOnly({
ack: { version: ACK_VERSION, paths: { 'explore.md': { reason: 'new mode section' } } },
baseAck: null,
});
assert.ok(r.ok);
const report = buildReport(r);
@@ -799,6 +1247,7 @@ test('a mixed grown set offers an ack entry only for the unacked files', () => {
sizeBaseline: { 'kept.md': 100, 'loud.md': 100 },
sizeCurrent: { 'kept.md': 200, 'loud.md': 200 },
ack: { version: ACK_VERSION, paths: { 'kept.md': { reason: 'declared' } } },
baseAck: null,
});
const report = buildReport(r);
const growth = blockOf(report, 'unacked-growth');
@@ -892,6 +1341,7 @@ test('the new-file cap is not ack-able (extraction, not acknowledgment, is the f
baseline: mf({}), current: mf({}), changedPaths: [],
sizeBaseline: {}, sizeCurrent: { 'new-workflow.md': NEW_FILE_CAP + 1 },
ack: { version: ACK_VERSION, paths: { 'new-workflow.md': { reason: 'trying to bypass it' } } },
baseAck: null,
});
assert.equal(r.newFileCapExceeded.length, 1, 'an ack entry must not exempt the new-file cap');
assert.ok(!r.ok);
@@ -1247,6 +1697,7 @@ test('property: every moved key lands in exactly one bucket', () => {
current: mf(current),
changedPaths: changedKeys.map((k) => sources[k]),
ack: { version: ACK_VERSION, paths: ackPaths },
baseAck: null,
});
if (r.errors.length) return false;
@@ -1759,6 +2210,15 @@ test('differential attribution over the real tree', { timeout: 900_000 }, async
const ack = readAckFile();
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):
// 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);
// 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
// at PR HEAD, so the two legitimately differ by a family whenever a PR adds or removes
@@ -1784,6 +2244,15 @@ test('differential attribution over the real tree', { timeout: 900_000 }, async
current,
changedPaths,
ack,
// The base side of the ack lifecycle (#2789). Without it a MERGED ack is
// indistinguishable from one that never explained anything, which is what reddened
// `next` for five commits and every PR branching off it (#2768). Entries already
// present here are spent: inert, never stale, and unable to pre-clear a new ripple.
//
// Keyed on `baseSha`, not `base`: the baseline half is already validated against that
// exact sha, so both halves of the base side provably describe the SAME commit, and a
// ref that moved between `resolveBase()` and here cannot split them.
baseAck,
sizeBaseline: resolvedBaseline.sizeBaseline,
sizeCurrent: currentSizes(),
});

167
tests/helpers/emitted-diff.cjs
View File

@@ -45,6 +45,28 @@ const ACK_VERSION = 1;
*/
const ACK_FILE = 'tests/emitted-drift-ack.json';
/**
* Distinguishes "caller omitted the base side" from "caller said there is none".
*
* A plain `null` default cannot tell those apart, and the difference is the whole point:
* omission would silently mean "inherit nothing", which is precisely how a dropped
* argument would restore #2768 with every unit test still green.
*/
const BASE_ACK_OMITTED = Symbol('baseAck omitted');
/**
* Characters that render as nothing: soft hyphen, the zero-width family, word joiner,
* BOM. Stripped before reasons are compared, so an invisible edit cannot re-arm a spent
* acknowledgment. Spelled as codepoints on purpose — a literal character class here
* would be invisible in review, which is the exact failure being defended against.
*/
const INVISIBLE = new RegExp(
`[${[0x00AD, 0x200B, 0x200C, 0x200D, 0x2060, 0xFEFF]
.map((c) => `\\u${c.toString(16).toUpperCase().padStart(4, '0')}`)
.join('')}]`,
'g',
);
/**
* A brand-new workflow/agent file — absent from the baseline, present now — must
* still stay under the Codex `project_doc_max_bytes` anchor (ADR-1610 Decision
@@ -89,7 +111,11 @@ const NEW_FILE_CAP = 32768;
* One document, one file, one paste.
*/
function ackDocument(entries) {
const paths = {};
// Null-prototype: `key` comes from repo/emitted paths, so a file literally named
// `__proto__` would otherwise SET THE PROTOTYPE instead of a property, and
// `JSON.stringify` would then emit `"paths":{}` — a remediation document that silently
// teaches the contributor to acknowledge nothing.
const paths = Object.create(null);
for (const { key, reason } of entries) paths[key] = { reason };
return JSON.stringify({ version: ACK_VERSION, paths });
}
@@ -121,8 +147,13 @@ const REMEDIATION = Object.freeze({
rippleReason: '<why this ripple is deliberate>',
growthReason: '<why this growth is deliberate>',
staleAckFix:
`Delete those entries from ${ACK_FILE}. If that leaves no entries, delete the file `
+ 'itself — its PRESENCE is the alarm, so an empty one signals nothing.',
`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.',
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.',
ackDocument,
});
@@ -197,18 +228,42 @@ function parseAck(doc, { source = 'emitted-drift-ack.json' } = {}) {
/**
* The conservation law.
*
* ── Ack lifecycle: an ack is scoped to the diff that introduced it (#2789) ───
*
* Every other input here is BASE-RELATIVE — `baseline` vs `current`, `changedPaths` from
* `git diff base...HEAD`. `ack` was the one ABSOLUTE input, read only from the working
* tree, and that mismatch was a real defect: `staleAcks` asks only "did a delta consume
* you?", which cannot tell "you never explained anything" (an authoring mistake) from
* "your ripple is now absorbed into the base" (the ack's SUCCESS condition). Merging an
* ack therefore made it look like a mistake, reddening `next` and every PR branching off
* it (#2768).
*
* `baseAck` supplies the missing side. An entry already present at the base is SPENT: it
* is not part of this diff, so it may no longer consume a delta and is never reported
* stale. Making it inert is also what finally closes ADR-2719's own named hazard — a
* leftover ack used to SILENTLY pre-clear the next ripple on its path; now that ripple
* must be explained on its own terms. Spent entries are still reported (`spentAcks`) so
* they can be tidied, but they gate nothing.
*
* `baseAck` is REQUIRED whenever `ack` is present — omitting it is an error, never a
* silent "nothing inherited". Same discipline as `changedPaths` above: a caller that
* cannot supply the base side must say `null` deliberately, so that dropping the
* argument fails loudly instead of quietly restoring #2768.
*
* @param {object} opts
* @param {object} opts.baseline { [runtime]: { [rel]: hash } } at `next` HEAD
* @param {object} opts.current { [runtime]: { [rel]: hash } } at PR HEAD
* @param {string[]} opts.changedPaths repo paths the PR changed (git diff --name-only)
* @param {object} [opts.ack] parsed emitted-drift-ack.json document (or null)
* @param {object} opts.baseAck the same document AT THE BASE REF (or null when
* 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
*
* @returns {{
* moved: number, attributed: Array, unattributable: Array, acked: Array,
* removed: Array, grown: Array, shrunk: Array, newFileCapExceeded: Array,
* staleAcks: string[], errors: string[], ok: boolean
* staleAcks: string[], spentAcks: string[], errors: string[], ok: boolean
* }}
*/
function diffEmitted({
@@ -216,6 +271,7 @@ function diffEmitted({
current,
changedPaths,
ack = null,
baseAck = BASE_ACK_OMITTED,
sizeBaseline = null,
sizeCurrent = null,
} = {}) {
@@ -242,14 +298,77 @@ function diffEmitted({
// manifest came back malformed, so the crash replaced the one message that would
// have explained the infrastructure problem (#2778).
moved: 0, attributed: [], unattributable: [], acked: [], removed: [],
grown: [], shrunk: [], newFileCapExceeded: [], staleAcks: [], errors, ok: false,
grown: [], shrunk: [], newFileCapExceeded: [], staleAcks: [], spentAcks: [],
errors, ok: false,
};
}
const changedSet = new Set(changedPaths);
const { entries: ackEntries, errors: ackErrors } = parseAck(ack);
const { entries: declaredAcks, errors: ackErrors } = parseAck(ack);
errors.push(...ackErrors);
// 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
// base side for them would fail a document the module elsewhere accepts. Protection is
// undiminished: an entry is the only thing that can be misclassified as spent or live,
// so the error still fires in exactly the situation where dropping `baseAck` would
// reintroduce #2768.
if (declaredAcks.size > 0 && baseAck === BASE_ACK_OMITTED) {
errors.push(
`${ACK_FILE}: baseAck was not supplied. The base side is not optional — without it `
+ 'a merged ack is indistinguishable from one that never explained anything, which '
+ 'is exactly the defect this parameter exists to close (#2789). Pass the document '
+ 'read at the base ref, or an explicit null when it is absent there.',
);
}
// Base-side SCHEMA errors are deliberately discarded, not surfaced. The base's validity
// is not this diff's to answer for, and a document we cannot read simply inherits
// nothing — which is the ARMED reading, since every entry then stays live and gated.
const resolvedBaseAck = baseAck === BASE_ACK_OMITTED ? null : baseAck;
const { entries: baseAcks } = parseAck(resolvedBaseAck);
/**
* Spent == the base already carries this entry's REASON, compared on prose alone.
*
* Re-arming a spent ack is legitimate — it is how a contributor says "this is a NEW
* ripple, and here is why" — but it must cost an actual explanation, because the
* reason is the entire artifact a reviewer reads. So the comparison is deliberately
* insensitive to everything that is not prose:
*
* - INTERNAL whitespace collapses. `parseAck` only trims the ends, so without this a
* doubled space re-arms an ack whose justification still describes the PREVIOUS
* ripple, and the ack file's diff shows a reviewer nothing new.
* - `runtime` is NOT compared. Nothing else in this module reads it (lookups key on
* `rel` alone), so including it would make an undocumented, schema-absent field the
* one thing that re-arms an ack — `+ "runtime": "claude"` beside a byte-identical
* reason, carrying no explanation at all.
*
* Both directions were live re-arm paths for a genuinely unattributable ripple.
*/
// `\s` covers NBSP and the ideographic space but NOT the zero-width family, so without
// this a U+200B (or a soft hyphen) re-arms a spent ack while being literally invisible
// in the diff — the purest form of "no new explanation". Stripped before the whitespace
// collapse so a zero-width char cannot glue two words into a different-looking string.
// Zero-information re-arm defence. `\s` covers NBSP and the ideographic space but NOT
// the zero-width family, so without this a U+200B or a soft hyphen re-arms a spent ack
// while being literally INVISIBLE in the diff — the purest form of "no new
// explanation". Built from codepoints rather than literal characters, because a
// literal class would itself be unreviewable in this file.
const prose = (reason) => reason.replace(INVISIBLE, '').replace(/\s+/g, ' ').trim();
const isSpent = (rel, entry) => {
const prior = baseAcks.get(rel);
return prior !== undefined && prose(prior.reason) === prose(entry.reason);
};
const ackEntries = new Map();
const spentAcks = [];
for (const [rel, entry] of declaredAcks) {
if (isSpent(rel, entry)) spentAcks.push(rel);
else ackEntries.set(rel, entry);
}
spentAcks.sort();
const attributed = [];
const unattributable = [];
const acked = [];
@@ -388,6 +507,7 @@ function diffEmitted({
shrunk,
newFileCapExceeded,
staleAcks,
spentAcks,
errors,
ok,
};
@@ -456,6 +576,24 @@ function buildReport(result, { sampleLimit = 20 } = {}) {
});
}
// Modelled here, not only rendered, so it can be asserted on identity like every other
// block — this file's stated contract is that `formatReport` is a pure rendering of
// this IR, and a section that exists only in prose would have to be tested by raw text
// matching, which CONTRIBUTING.md prohibits.
//
// Gated on `!result.ok` to KEEP that contract true. Spent acks are the one block whose
// emit-condition could drift from the renderer's, since a passing run must render
// nothing at all; without this, a JSON reporter built on the IR would announce spent
// acks for a green run while the text reporter stayed silent.
if (!result.ok && result.spentAcks && result.spentAcks.length) {
blocks.push({
kind: 'spent-acks',
count: result.spentAcks.length,
items: result.spentAcks.slice(0, sampleLimit),
fix: REMEDIATION.spentAckNote,
});
}
// ONE ack set for the whole report, not one per branch. A report can trip the hash
// branch and the size branch at once, and two complete documents each reading as "the
// file to create" invites pasting the second over the first — losing an acknowledgment
@@ -526,13 +664,26 @@ function formatReport(result, { sampleLimit = 20 } = {}) {
if (result.staleAcks.length) {
parts.push(
`${result.staleAcks.length} stale acknowledgment(s) — the ripple they explained is gone, ` +
'so they must be deleted (an ack that outlives its ripple pre-clears the next one):\n ' +
`${result.staleAcks.length} stale acknowledgment(s) — written or reworded in THIS diff, ` +
'but nothing here needed them, so they explain nothing:\n ' +
result.staleAcks.slice(0, sampleLimit).join('\n ') +
`\n\n${REMEDIATION.staleAckFix}`,
);
}
// Informational, never gating, and rendered ONLY alongside a real failure. Spent
// entries are inert housekeeping, not a demand — surfacing them when a contributor is
// already in the file is free, but emitting them for an otherwise-clean run would make
// `formatReport` return prose for an `ok` result, which this module's callers read as
// "there is something wrong".
const spent = (result.spentAcks || []).slice(0, sampleLimit);
if (spent.length && !result.ok) {
parts.push(
`${result.spentAcks.length} spent acknowledgment(s):\n ` + spent.join('\n ') +
`\n\n${REMEDIATION.spentAckNote}`,
);
}
// The remedy, once, at the end — one file, one document, one paste.
const { ackable } = buildReport(result, { sampleLimit });
if (ackable.length) {

82
tests/helpers/emitted-runtime.cjs
View File

@@ -45,7 +45,13 @@ const {
} = require('./install-shared.cjs');
const REPO_ROOT = path.join(__dirname, '..', '..');
const ACK_PATH = path.join(REPO_ROOT, 'tests', 'emitted-drift-ack.json');
/**
* Repo-relative and POSIX-separated on every platform: this form is what `git show
* <ref>:<path>` requires, and git speaks only forward slashes regardless of host OS.
* `ACK_PATH` derives from it so the two can never name different files.
*/
const ACK_REPO_PATH = 'tests/emitted-drift-ack.json';
const ACK_PATH = path.join(REPO_ROOT, ...ACK_REPO_PATH.split('/'));
const FIXTURE_SUBDIR = 'tests/fixtures/golden-install-parity';
/**
@@ -291,6 +297,78 @@ function resolveBaseSha(base = 'origin/next') {
return git(['rev-parse', base]).trim();
}
/**
* The acknowledgment document AS IT EXISTS AT `base` — the base side of the ack
* lifecycle (#2789). An entry already present there is SPENT: its ripple is absorbed
* into the base, so it may no longer clear a delta and is never reported stale.
*
* ── Why "inherit nothing" is NOT a safe default ──────────────────────────────
* Absent at that ref is the healthy steady state and returns `null`. Every OTHER failure
* THROWS, and the distinction is load-bearing in the direction that is easy to get
* backwards. Returning `null` on a read error looks armed — nothing is inherited, so
* every entry stays live — but 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,
* which is the axis a gate over shipped artifacts actually cares about: a genuinely new,
* unexplained ripple on a path carrying an already-merged ack would come back `acked`
* instead of `unattributable`. That is silently the whole pre-#2789 behavior, including
* the pre-clearing hazard this change exists to close.
*
* So this follows the same law as `resolveChangedPaths` above — a failed git read is an
* ERROR, not an empty set — and matches the head-side `readAckFile`, which already
* throws on a document that exists but will not parse. Being more forgiving about the
* base copy of the same file would be strictly worse: it is the copy we cannot see in
* the diff.
*
* `git show` alone cannot make the distinction — a bogus ref and an absent path produce
* 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.
*/
function readAckFileAtRef(base, { cwd = REPO_ROOT, run = git } = {}) {
// `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=<file>`, which writes. Today every caller passes a
// resolved 40-hex sha, but this function is exported and validated nothing itself —
// the guard belonged with the argument, not with the one caller that happens to be safe.
if (typeof base !== 'string' || base === '' || base.startsWith('-')) {
throw new Error(
`emitted-attribution: refusing to read the ack at ${JSON.stringify(base)} — a base ref `
+ 'must be a non-empty string that does not begin with "-", which git would parse as an option.',
);
}
let listing;
try {
listing = run(['ls-tree', '--name-only', base, '--', ACK_REPO_PATH], { cwd });
} catch (err) {
throw new Error(
`emitted-attribution: could not list the ack at "${base}": ${err.message}. This is a `
+ 'hard error on purpose — treating an unreadable base as "nothing inherited" would '
+ 'leave every ack able to consume a delta, which is the pre-#2789 gate.',
);
}
if (listing.trim() === '') return null; // genuinely absent at that ref — the steady state
let raw;
try {
raw = run(['show', `${base}:${ACK_REPO_PATH}`], { cwd });
} catch (err) {
throw new Error(
`emitted-attribution: ${ACK_REPO_PATH} 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`);
}
try {
return JSON.parse(raw);
} catch (err) {
throw new Error(
`emitted-attribution: ${ACK_REPO_PATH} at "${base}" is not valid JSON: ${err.message}`,
);
}
}
/**
* Base-ref candidates, most-specific first.
*
@@ -597,6 +675,8 @@ function readAckFile(ackPath = ACK_PATH) {
module.exports = {
REPO_ROOT,
ACK_PATH,
ACK_REPO_PATH,
readAckFileAtRef,
FIXTURE_SUBDIR,
MANIFEST_FAMILIES,
MINIMUM_MANIFEST_FAMILIES,