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

fix(#3078): sweep all-spent ack fragments on next, name the collision remedy (#3823)

Browse Source 
* fix(#3078): sweep all-spent ack fragments on next, name the collision remedy

`guard-no-ack-on-next` only ever watched the legacy tests/emitted-drift-ack.json.
#2914 exempted the fragment directory on the premise that a persisting fragment
"cannot conflict with any other PR". Fragments do not share a FILE, but they do
share a PATH KEY SPACE, and a path claimed by two sources is a hard failure in
the same script -- so a fully-spent fragment on next owns keys it can no longer
gate, and the next PR to grow one of those paths can declare it neither there
(spent) nor in its own fragment (duplicate). Measured at the sweep: 45 fragments
owning 403 paths, up from 13/272 at triage 19 days earlier.

- `assertNoAllSpentFragments` fails a fragment only when EVERY surviving entry is
  spent against the copy at HEAD^, so a partially spent fragment -- and the
  re-arm-by-appending route #2639/#2993 ship on -- keeps working.
- `ackProse` duplicates the gate's zero-width/whitespace stripping across the
  scripts-ship/tests-do-not line, bounded by a prose-parity test.
- The guard job's checkout takes fetch-depth: 2; at depth 1 HEAD^ is absent and
  every fragment reads as brand-new, i.e. the guard passes vacuously.
- The duplicate-ack error now names both resolutions, since the guard is
  post-merge by design and cannot stop the colliding PR.
- All 45 spent fragments deleted, 0000-legacy-migration.json included, and the
  three tests that pinned its permanence corrected.

Verification is the remote runner (gsd-test), not a local suite.

Closes #3078

* fix(#3078): make the prose-parity test two-sided, cover the git seam, base on the pre-push tip

Three review findings, all fixed:

- The parity test was a tautology: it checked ACK_INVISIBLE against a
  hardcoded list matching its own definition, never against the gate. The
  gate's INVISIBLE and its reason normalizer (hoisted out of diffEmitted as
  normalizeAckReason) are now exported for that sole purpose, and the test
  sweeps 0x00-0xFFFF against both surfaces. Mutation-checked: adding a
  codepoint to one side and not the other now fails.
- resolveBaseRef, readFragmentAtRef and assertUsableBaseRef had zero direct
  coverage -- the tests reimplemented the git reads in a local helper, so the
  ls-tree-vs-show discrimination, the root-commit fallback and the
  option-injection guard were never executed. All are exported and tested
  against real temp repositories now, plus an end-to-end --base-ref subprocess.
- HEAD^ is not 'the state of next before this push'. The default branch allows
  REBASE merges, so one push can carry N commits, and a 2-commit rebase-merge
  whose first commit adds a fragment would be told to git rm it on the very
  push that introduced it. CI now passes github.event.before via --base-ref and
  fetches it explicitly; HEAD^ remains only the local fallback.

Also adds the safe.directory guard every other git call in this repo carries
(#2767), and stops naming the deleted migration fragment by filename in
CONTEXT.md, which tripped lint-removed-but-needed.

Refs #3078

* fix(#3078): keep the fragment directory alive after the sweep empties it

Sweeping every fragment leaves the directory untracked, and check-glossary-refs
then fails: CONTEXT.md references tests/emitted-drift-acks, which no longer
exists. The empty directory IS the intended steady state, so it has to survive
its own remedy.

Adds tests/emitted-drift-acks/README.md documenting the create/use/delete
lifecycle where a contributor actually meets it, matching the existing
tests/qa/smell-acks/README.md precedent. Every reader filters on .json, so the
README is invisible to the gate.

Also sweeps #3809's ack fragment, which the rebase onto origin/next brought in
and the new guard immediately reported as all-spent -- its own remedy applied.

Refs #3078

* fix(#3078): guard the added tests' git calls, drop a second fragment-existence pin

Both defects surfaced by the remote runner (linux-node24, 4/37445 failed).

- The new --base-ref E2E test ran `git rev-parse HEAD` against the checkout
  without the #2767 safe.directory guard. The runner mounts the repo at a path
  owned by another uid, so git refused every operation there with 'detected
  dubious ownership'. Every git call the new tests make now names its own
  specific directory as safe, via one local helper, mirroring safeDirArgs in
  helpers/emitted-runtime.cjs.
- tests/agent-tracked-source-rule.test.cjs pinned the existence and contents of
  the 3645 and 3409 ack fragments. That is a merged PR's paperwork, not live
  behavior: once the growth is in next's baseline the acks are spent and this
  PR's guard sweeps them. The third assertion pinned the hand-appended
  workaround for the exact collision #3078 removes. Deleted; #3645's real
  protection is the two behavioral tests above it, untouched.

Also restores #3809's ack fragment, which merged one commit before this branch.
Deleting an ack in the same window as its introducing PR races any consumer
whose baseline predates it -- the runner's container proved it, resolving
origin/next to 8ed105c8a where the file is still 13847. The backlog sweep is
this PR's scope; that fragment is left for the guard's own first run.

Adds the rule to the fragment README so the class stops recurring.

Refs #3078

* test(#3078): derive the E2E guard expectation from the fragment inventory

The --base-ref E2E test asserted exit 0 while passing the checkout's own HEAD
as the base ref. HEAD-as-base makes every present fragment byte-identical to
itself, so all of them are trivially all-spent and the guard correctly exits 1.
The test only ever passed because the directory happened to be empty when it
was written; restoring #3809's fragment made it fail. The script was right and
the test was wrong.

The degenerate base ref is kept deliberately -- it is what makes 'spent'
trivially true and therefore deterministic -- but the expectation is now
derived from listFragmentFiles() at runtime: zero fragments means exit 0 and
the no-survivors line, N fragments means exit 1 with every name and its git rm.
Proven state-independent by running the suite with the fragment present, with
the directory emptied, and with it restored.

The option-shaped --base-ref rejection is split into its own test, unchanged.

Refs #3078

* chore(#3078): backfill PR number into the changeset fragment (pr:0 -> pr:3823)

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-24 15:24:30 -04:00
committed by GitHub
parent 8442d984b9
commit a84f756303
57 changed files with 1598 additions and 1351 deletions
Download Patch File Download Diff File Expand all files Collapse all files

305
scripts/lint-emitted-drift-ack.cjs
View File

@@ -32,12 +32,46 @@
const fs = require('node:fs');
const path = require('node:path');
const { execFileSync } = require('node:child_process');
const ACK_VERSION = 1;
const ACK_REPO_PATH = 'tests/emitted-drift-ack.json';
const ACK_DIR_REPO_PATH = 'tests/emitted-drift-acks';
const REPO_ROOT = path.join(__dirname, '..');
/**
* Upper bound on any one git call made by the `--guard-next` lane (#3078).
*
* Every subprocess this repo spawns is bounded (CLAUDE.md -> KNOWN DEFECTS, "Unbounded
* Subprocesses": 5-30s for git). The guard reads one directory listing plus one blob per
* surviving fragment, all against local objects, so 15s is generous — but an unbounded
* `execFileSync` on a wedged git is an indefinite hang in a job whose whole timeout
* budget is one minute.
*/
const GIT_TIMEOUT_MS = 15_000;
/**
* Characters that render as nothing: soft hyphen, the zero-width family, word joiner,
* BOM. Stripped before ack reasons are compared, so an invisible edit cannot make a spent
* acknowledgment look re-armed.
*
* DUPLICATED from `INVISIBLE` in `tests/helpers/emitted-diff.cjs`, for the same reason
* every other constant here is duplicated rather than required: `scripts/` ships in the
* npm package and `tests/` does not, so the require would be MODULE_NOT_FOUND once
* published (see this file's top-of-file comment). The two are held together by the
* prose-parity test in `tests/emitted-attribution.test.cjs`, which enumerates the gate's
* own codepoints and fails if this list stops covering them.
*
* Spelled as codepoints on purpose — a literal character class here would itself be
* invisible in review, which is the exact failure being defended against.
*/
const ACK_INVISIBLE = new RegExp(
`[${[0x00AD, 0x200B, 0x200C, 0x200D, 0x2060, 0xFEFF]
.map((c) => `\\u${c.toString(16).toUpperCase().padStart(4, '0')}`)
.join('')}]`,
'g',
);
/**
* Upper bound on how many fragment files `listFragmentFiles` may return in one
* `readdirSync` pass. Mirrors `MAX_ACK_FRAGMENTS` in `tests/helpers/emitted-diff.cjs` —
@@ -215,6 +249,132 @@ function declaredKeys(raw) {
return Object.keys(paths).filter((k) => !RESERVED_ACK_KEYS.has(k));
}
/**
* Normalize an ack reason to the prose a reviewer actually reads.
*
* Mirrors the gate's own `prose()` inside `diffEmitted` (`tests/helpers/emitted-diff.cjs`)
* exactly: strip invisibles, collapse internal whitespace, trim. 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, so a doubled space, a CRLF, or a U+200B may never
* make a spent entry look live. Bounded by the parity test named on ACK_INVISIBLE.
*/
function ackProse(reason) {
return reason.replace(ACK_INVISIBLE, '').replace(/\s+/g, ' ').trim();
}
/**
* The declared entries of one ack document as `path key -> normalized prose`, or `null`
* when the document cannot be trusted to answer the question.
*
* `null` is NOT "no entries" — it is "do not draw a conclusion from this file". A document
* that will not parse, is not an object, has a non-object `paths`, carries a reasonless or
* non-string entry, or names a RESERVED_ACK_KEY cannot be shown to be spent, and the
* conservative direction here is to leave it alone: `validateAckText` (run by `lint:ci`,
* pre-merge) owns SHAPE and already blocks such a document from reaching the base, while
* this guard owns LIFECYCLE. Sweeping a file we could not read would delete an
* acknowledgment on the strength of a parse failure.
*
* An ABSENT document (`raw === null`) is a genuine empty entry set — the fragment simply
* did not exist at that ref, so nothing it declares now has a counterpart there.
*/
function ackEntries(raw) {
if (raw === null) return new Map();
let doc;
try {
doc = JSON.parse(raw);
} catch {
return null;
}
if (!isPlainObject(doc)) return null;
const paths = doc.paths;
if (paths === undefined) return new Map();
if (!isPlainObject(paths)) return null;
const entries = new Map();
for (const [rel, value] of Object.entries(paths)) {
if (RESERVED_ACK_KEYS.has(rel)) return null;
const reason = isPlainObject(value) ? value.reason : value;
if (typeof reason !== 'string') return null;
entries.set(rel, ackProse(reason));
}
return entries;
}
/**
* assertNoAllSpentFragments — the fragment half of the `next`-lane guard (#3078).
*
* #2914 split the single shared ack file into per-PR fragments and deliberately exempted
* the fragment directory from `assertAbsentOnNext`, on the premise that a persistent
* fragment "cannot conflict with any other PR". That premise does not hold. Fragments do
* not share a FILE, but they do share a PATH KEY SPACE, and `main()` below treats a path
* claimed by two sources as a hard failure. So a merged fragment is not harmless: every
* entry it leaves on `next` is spent by definition — its prose is already at the base, so
* it gates nothing — while still owning its key, and the next PR that grows the same
* workflow can declare it neither in the owning fragment (spent) nor in its own
* (duplicate). That is the exact failure #2914 fixed for the legacy file, reintroduced one
* level down. Measured on `next` when this landed: 45 fragments owning 403 paths.
*
* Unlike the legacy file, PRESENCE alone is not the failure — a fragment landed by the
* very push being guarded is the healthy case for every ack-carrying PR. The failure is
* INERTNESS: every surviving entry's prose already matches the copy at the base ref, so
* the fragment can no longer clear a delta for anyone. A PARTIALLY spent fragment is left
* alone; only an entirely inert one is cruft. That distinction is why the base side is
* required, and it is what keeps the re-arm-by-appending route working (#2639, #2993).
*
* An ENTRYLESS document is vacuously all-spent and swept for the same reason
* `validateAckText` refuses to let one be committed: it acknowledges nothing.
*
* Pure — no fs, no git, no clock. `main()` does the reading.
*
* @param {Array<{name: string, currentRaw: string|null, baseRaw: string|null}>} fragments
* @returns {{ ok: boolean, message: string, sweepable: string[] }}
*/
function assertNoAllSpentFragments(fragments) {
const sweepable = [];
for (const { name, currentRaw, baseRaw } of fragments) {
const current = ackEntries(currentRaw);
if (current === null) continue; // unreadable — `validateAckText` owns that verdict
const base = ackEntries(baseRaw);
if (base === null) continue;
const allSpent = [...current].every(([rel, prose]) => base.get(rel) === prose);
if (allSpent) sweepable.push({ name, entries: current.size });
}
if (sweepable.length === 0) {
return {
ok: true,
message: `ok guard-no-ack-on-next: no all-spent fragment survives in ${ACK_DIR_REPO_PATH}/`,
sweepable: [],
};
}
const lines = sweepable.map(
({ name, entries }) => ` - ${ACK_DIR_REPO_PATH}/${name} (${entries} entr${entries === 1 ? 'y' : 'ies'}, all spent)`
+ `\n remedy: git rm ${ACK_DIR_REPO_PATH}/${name}`,
);
return {
ok: false,
sweepable: sweepable.map(({ name }) => name),
message: [
`guard-no-ack-on-next: ${sweepable.length} fully-spent ack fragment(s) survive on next.`,
'',
...lines,
'',
'Every entry in these fragments is already at the base, so each is spent and gates '
+ 'nothing (#2789) — but it still OWNS its path keys. The next PR that grows one of '
+ 'those paths can declare it neither here (spent) nor in its own fragment (a '
+ 'duplicate ack is a hard failure), so a spent fragment left behind is a wall, not '
+ 'harmless cruft (#3078).',
'',
'A partially spent fragment is deliberately NOT reported: only an entirely inert one '
+ 'is swept, so appending prose to a live entry to re-arm it keeps working.',
].join('\n'),
};
}
/**
* assertAbsentOnNext — the `next`-lane guard (#2914), invoked only by the
* `guard-no-ack-on-next` workflow job on push to `next`, never in `lint:ci`.
@@ -231,6 +391,12 @@ function declaredKeys(raw) {
* instant one landed). It is safe only because it runs on `next` itself, asserting a fact
* about `next`'s own tree, never about any PR's diff against it.
*
* Scoped to the LEGACY FILE ONLY — the fragment directory is guarded by
* `assertNoAllSpentFragments` above, on a stricter-to-state but weaker-to-apply rule
* (inertness, not presence). #3078 corrected the original premise that a persisting
* fragment "cannot conflict with any other PR": fragments share a path key space even
* though they do not share a file.
*
* @param {boolean} present whether ACK_REPO_PATH exists in the tree being checked
* @returns {{ ok: boolean, message: string }}
*/
@@ -248,9 +414,10 @@ function assertAbsentOnNext(present) {
+ 'whether it is otherwise well-formed.',
'',
'#2914: acks now go in per-PR fragments under tests/emitted-drift-acks/, one file per '
+ 'PR, never this single shared file -- a persistent fragment there is harmless (every '
+ 'fragment is independently named, so it cannot conflict with any other PR), which is '
+ 'why only THIS legacy file is guarded here, never the fragment directory.',
+ 'PR, never this single shared file. A fragment cannot MERGE-CONFLICT with another '
+ "PR's fragment, but it does own its path keys, so a fully-spent one left on next "
+ 'still blocks the next PR that grows the same path (#3078) -- fragments are guarded '
+ 'separately, on inertness rather than on presence.',
'',
'CONTRIBUTING.md: "When you remove the last entry from tests/emitted-drift-ack.json, '
+ 'delete the file too -- its presence is the alarm."',
@@ -260,13 +427,123 @@ function assertAbsentOnNext(present) {
};
}
/**
* Every git call declares the SPECIFIC directory it operates on as safe, mirroring
* `safeDirArgs` in `tests/helpers/emitted-runtime.cjs` (#2767): a checkout mounted at a
* path owned by a different uid makes git refuse EVERY operation there with "detected
* dubious ownership", and this guard's whole value is that it fails LOUDLY on a real
* fault rather than degrading to "no base, nothing spent". Never the `*` wildcard, which
* would mark every repository on the machine safe. Duplicated rather than imported for
* the reason stated at the top of this file: `scripts/` ships in the npm package and
* `tests/` does not.
*/
function git(args, { cwd = REPO_ROOT } = {}) {
return execFileSync('git', ['-c', `safe.directory=${path.resolve(cwd)}`, ...args], {
cwd,
encoding: 'utf8',
timeout: GIT_TIMEOUT_MS,
maxBuffer: 16 * 1024 * 1024,
stdio: ['ignore', 'pipe', 'pipe'],
});
}
/**
* The LOCAL/MANUAL fallback for "the commit `next` was at BEFORE this push" — `HEAD^`,
* or `null` on a root commit. Correct only when the push it is standing in for carries
* exactly one commit.
*
* CI never relies on this: it passes the authoritative pre-push tip explicitly via
* `--base-ref` (`github.event.before`, wired in `.github/workflows/test.yml`), because
* the default branch's ruleset allows REBASE merges
* (`.github/rulesets/main-protection.json`, `allowed_merge_methods`), so a single push
* event can land N commits at once. `HEAD^` steps back exactly one commit — for a
* 2-commit rebase-merge whose first commit adds a fragment and whose second is
* unrelated, `HEAD^` would land on the first commit, read the fragment as already
* present there, and demand `git rm` on the very push that introduced it (#3078). This
* function exists purely as the manual-run / single-commit-push fallback.
*
* Two steps on purpose. `HEAD` is resolved first, which proves git runs and the working
* directory is a readable repository; only then is a failure to resolve `HEAD^` read as
* "this commit has no parent". A single blanket try/catch would collapse "git is broken"
* into "there is no base", and a guard with no base sweeps nothing — it would pass
* vacuously, which is precisely how the legacy-file job spent months guarding a file that
* had not existed since #2914 (#3078).
*/
function resolveBaseRef({ cwd = REPO_ROOT, run = git } = {}) {
run(['rev-parse', '--verify', 'HEAD'], { cwd });
try {
return run(['rev-parse', '--verify', 'HEAD^'], { cwd }).trim();
} catch {
return null; // root commit — nothing can be spent against it
}
}
/**
* Raw text of one ack fragment AT `base`, or `null` when it is simply not there.
*
* Mirrors `readAckFileAtRef` in `tests/helpers/emitted-runtime.cjs` (duplicated across the
* ships/does-not-ship line, as everything else here is): `git show` alone cannot tell a
* bogus ref from an absent path — both say "does not exist in" — so absence is established
* with `ls-tree`, which exits 0 with empty output when the path is not there and non-zero
* on a real fault. A genuine git failure THROWS rather than degrading to `null`, because
* "could not read the base" read as "absent at the base" would make every fragment look
* brand-new and silently disarm the sweep.
*/
function readFragmentAtRef(base, name, { cwd = REPO_ROOT, run = git } = {}) {
const repoPath = `${ACK_DIR_REPO_PATH}/${name}`;
const listing = run(['ls-tree', '--name-only', base, '--', repoPath], { cwd });
if (listing.trim() === '') return null;
return run(['show', `${base}:${repoPath}`], { cwd });
}
/**
* A base ref must not begin with `-`: `execFileSync`'s array form stops shell
* metacharacters but not git's own option parsing, and `git show` honors diff options
* including `--output=<file>`, which WRITES. Same guard, same reason, as
* `readAckFileAtRef`'s.
*/
function assertUsableBaseRef(ref) {
if (typeof ref !== 'string' || ref === '' || ref.startsWith('-')) {
throw new Error(
`lint-emitted-drift-ack: refusing to read fragments at ${JSON.stringify(ref)} — a base `
+ 'ref must be a non-empty string that does not begin with "-", which git would parse '
+ 'as an option.',
);
}
return ref;
}
function main() {
const legacyFile = path.join(REPO_ROOT, ...ACK_REPO_PATH.split('/'));
if (process.argv.includes('--guard-next')) {
const result = assertAbsentOnNext(fs.existsSync(legacyFile));
console.log(result.message);
if (!result.ok) process.exitCode = 1;
const legacy = assertAbsentOnNext(fs.existsSync(legacyFile));
console.log(legacy.message);
// The fragment half (#3078). CI always passes `--base-ref` (the pre-push tip of
// `next`, `github.event.before`), because the default branch allows REBASE merges
// and one push can carry N commits — `HEAD^` alone is not "the state of next before
// this push" in that case. `resolveBaseRef()`'s `HEAD^` is only the fallback for a
// manual run or a single-commit push, where the two agree. NOTE the job's checkout
// must fetch at least depth 2 for the `HEAD^` fallback to resolve at all, and must
// separately fetch the `--base-ref` commit itself, or every fragment reads as
// brand-new.
const baseFlag = process.argv.indexOf('--base-ref');
const baseRef = baseFlag === -1
? resolveBaseRef()
: assertUsableBaseRef(process.argv[baseFlag + 1]);
const dir = path.join(REPO_ROOT, ...ACK_DIR_REPO_PATH.split('/'));
const fragments = listFragmentFiles(dir).map((name) => ({
name,
currentRaw: readIfPresent(path.join(dir, name)),
baseRaw: baseRef === null ? null : readFragmentAtRef(baseRef, name),
}));
const sweep = assertNoAllSpentFragments(fragments);
console.log(sweep.message);
if (!legacy.ok || !sweep.ok) process.exitCode = 1;
return;
}
@@ -301,7 +578,12 @@ function main() {
problems.push(
`duplicate ack for "${key}": declared in both ${owner.get(key)} and ${label}. `
+ 'Two ack sources (fragments, or a fragment and the legacy file) may never '
+ 'name the same path — rename or merge them.',
+ 'name the same path. Resolve it one of two ways, depending on the owner: if '
+ `${owner.get(key)} is already merged, its entry is SPENT and gates nothing — `
+ `delete it (git rm ${owner.get(key)}) and keep your own. If it is still live `
+ 'on this branch, APPEND your explanation to its existing entry instead, which '
+ 're-arms it — re-arming deliberately costs actual new prose. Do not rename '
+ 'the path to dodge this.',
);
continue;
}
@@ -335,10 +617,19 @@ if (require.main === module) main();
module.exports = {
validateAckText,
assertAbsentOnNext,
assertNoAllSpentFragments,
ackProse,
ackEntries,
declaredKeys,
listFragmentFiles,
ACK_VERSION,
ACK_REPO_PATH,
ACK_DIR_REPO_PATH,
ACK_INVISIBLE,
MAX_ACK_FRAGMENTS,
git,
resolveBaseRef,
readFragmentAtRef,
assertUsableBaseRef,
GIT_TIMEOUT_MS,
};