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 <noreply@anthropic.com>

* chore(#2914): backfill changeset pr number

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-07-31 13:15:29 -04:00
committed by GitHub
parent d2d2f7c088
commit c043f2946c
9 changed files with 1287 additions and 62 deletions

View File

@@ -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}/<a-name-nobody-else-will-use>.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: '<why this ripple is deliberate>',
growthReason: '<why this growth is deliberate>',
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,

View File

@@ -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
* <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.
*
* 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=<file>`, 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,