Skip to content

Commit 9ea004c

Browse files
lilyshen0722claudenova
authored
The tool-contract guard sees a gitlink rollback, with the cells tested (refresh of #1090) (#2497)
* fix(ci): the tool-contract guard sees a gitlink rollback, with the cells tested Refresh of #1090, which was re-reviewed clean in August and never merged. The direction check is theirs; the tests are what they said they could not write, and two of their coupled assumptions needed re-scoping. Measured on main 9ac61bd through the suite's own injected-exec seam: a pin that is a strict ancestor of the merge base's pin returns `contained`. Containment cannot catch a rollback — a rollback is by definition to a commit the branch already had — so #1089's exact shape passes today. A human caught that one by reading the diff. #1090's "Not verified" section named two gaps. Both are closed here: the forward-bump cell is exercised rather than argued, and the guard now has unit coverage, because a suite with an injected exec exists now and did not in August. TWO PINNED ASSERTIONS CHANGED, both because the direction probe adds one superproject git call on every path: - "reports contained without ancestry when the pin is the branch tip" asserted no `merge-base` call anywhere. Its own comment says it means the ancestry walk, so it is now scoped to submodule calls. It still fails if containment ever walks ancestry on the tip path. - "caps the first fetch at depth 1" simulated a missing ref with `mockImplementationOnce`, i.e. positionally on the first exec call. The direction probe is now first, so the mock landed on the wrong call and the ref-absence case silently stopped being tested. It targets the ref probe by shape instead. Honest about the new tests: the revert mutation reds 2 of the 4 new cells. The other two assert `contained`, which main also answers, so they are future-regression guards on the check's narrowness rather than evidence about this change — labelled as such in the file. Fixes #2496 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: nova <WyJub3ZhIiwiZGVmYXVsdCJd@agents.commonly.invalid> Agent-Model: claude/claude-opus-5/high * fix(ci): make the pin-direction check actually run in CI, and say so when it cannot @otto's gate on #2497 found the defect: the direction probe was correct and dead. `tests.yml`'s tool-tests job checks out with `submodules: recursive` and no `fetch-depth`, so `origin/main` does not exist on a pull_request, no merge base resolves, the probe returned `undetermined` — and the caller dropped that verdict on the floor. The check never fired on a single PR, and a test cell pinned the fall-through as a pass. Three repairs, one invariant: - `checkPinDirection` is its own exported function, not a closure inside `checkPinReachable`. Containment asks "is the pin on the branch"; direction asks "did it move backward"; a rollback answers YES to the first. Split, a containment harness cannot answer for direction, and the direction verdict reaches `main()` on its own — exit 1 for a rollback, exit 2 for undetermined. - An unresolvable base is LOUD: `undetermined`, naming `fetch-depth: 0` as the repair. The probe does not fetch to paper over it — that would hide a misconfigured job and pay the 11.4s/314M cost this file measures. - The tool-tests job checks out with `fetch-depth: 0`, which is what lets it answer. Pinned by a test that reads the workflow, scoped to the job that runs the guard. `GITHUB_BASE_REF` now takes precedence over `main`, not the other way round: at depth 0 both refs resolve, so a fallback would be dead code and every stacked PR would be judged against main — a base behind the real one, holding a pin the PR never touched, i.e. a false rollback. Measured with real git, both topologies: `origin/main` present → ok; the ref removed → undetermined. Each of the three repairs reverted in turn reds exactly one cell, the one that claims it. 60/60 in the suite, and green again under `GITHUB_BASE_REF=nova/some-parent`, the stacked-PR environment that would otherwise have red this suite in CI only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: nova <WyJub3ZhIiwiZGVmYXVsdCJd@agents.commonly.invalid> Agent-Model: claude/claude-opus-5/high * fix(ci): say which base the direction check used, on the pass path too A check that prints nothing when it is happy is indistinguishable from a check that is not running — which is exactly how this one stayed dead for two months while its probe returned `undetermined` on every pull request. The verdict now names the ref it compared against, so the CI log distinguishes "compared against origin/main, no rollback" from silence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: nova <WyJub3ZhIiwiZGVmYXVsdCJd@agents.commonly.invalid> Agent-Model: claude/claude-opus-5/high --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: nova <WyJub3ZhIiwiZGVmYXVsdCJd@agents.commonly.invalid>
1 parent b023087 commit 9ea004c

3 files changed

Lines changed: 364 additions & 11 deletions

File tree

‎.github/workflows/tests.yml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,13 @@ jobs:
130130
- uses: actions/checkout@v4
131131
with:
132132
submodules: recursive
133+
# The tool-contract guard compares the submodule pin against the
134+
# merge base with origin/main to catch a ROLLBACK, which containment
135+
# structurally cannot see. At the default depth 1 there is no
136+
# origin/main and no shared history, so that check could not run at
137+
# all (#2497). The guard now says so loudly instead of passing, and
138+
# this is what lets it answer.
139+
fetch-depth: 0
133140
- uses: actions/setup-node@v4
134141
with:
135142
node-version: '22'

‎backend/__tests__/unit/scripts/moltbotToolContract.test.js‎

Lines changed: 227 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ const {
2424
loadMentionCues,
2525
readDeclaredBranch,
2626
checkPinReachable,
27+
checkPinDirection,
2728
readGitlinkSha,
2829
stripComments,
2930
MENTION_CUES,
@@ -291,6 +292,202 @@ describe('moltbot tool contract', () => {
291292
expect(checkPinReachable({ exec }).state).toBe('contained');
292293
});
293294

295+
/**
296+
* DIRECTION, which containment structurally cannot judge.
297+
*
298+
* A rollback is by definition to a commit the declared branch already
299+
* had, so "the pin is an ancestor of the branch" is TRUE of every
300+
* rollback. Measured on main 9ac61bdef before the fix: the scripted
301+
* repo below returned `contained` — the guard passed #1089's exact
302+
* shape. #1089 was caught because ux-lead read the diff by hand.
303+
*
304+
* The five cells are scripted through the injected exec rather than
305+
* with real commits, because the original fix (#1090) said outright
306+
* that it could not exercise the forward-bump cell — "no sha exists
307+
* that is a descendant of the base pin … both are argued rather than
308+
* exercised". Through this seam all five are exercised, and the
309+
* superproject and submodule calls are told apart by cwd, not by call
310+
* order, so a reordering cannot make these vacuous.
311+
*/
312+
const directionRepoWhere = ({ basePin, isAncestor, resolvableRefs = ['origin/main'] }) => {
313+
const calls = [];
314+
const supCalls = [];
315+
const mock = jest.fn((_bin, args, opts = {}) => {
316+
const inSubmodule = String(opts.cwd || '').includes('_external');
317+
calls.push({ args, inSubmodule });
318+
if (!inSubmodule) supCalls.push(args);
319+
if (!inSubmodule && args[0] === 'merge-base') {
320+
// `merge-base HEAD <ref>`. A ref the checkout never fetched is not
321+
// a git failure with a special status — the command simply exits
322+
// non-zero, the same as "no common history", which is why the probe
323+
// cannot tell them apart and must say undetermined for both.
324+
if (!resolvableRefs.includes(args[2])) {
325+
throw Object.assign(new Error('not a valid object name'), { status: 128 });
326+
}
327+
return 'basecommitsha\n';
328+
}
329+
if (!inSubmodule && args[0] === 'ls-tree') {
330+
return basePin ? `160000 commit ${basePin}\t_external/clawdbot\n` : '\n';
331+
}
332+
if (inSubmodule && args[0] === 'merge-base' && args[1] === '--is-ancestor') {
333+
if (!isAncestor) throw Object.assign(new Error('not an ancestor'), { status: 1 });
334+
return '';
335+
}
336+
return '';
337+
});
338+
mock.supCalls = supCalls;
339+
return mock;
340+
};
341+
342+
// The probe reads GITHUB_BASE_REF, which the Actions runner SETS — and
343+
// sets to the PR's base, so on a stacked PR these cells would inherit a
344+
// ref their harness does not resolve and red in CI only. Each cell states
345+
// its own base; nothing is inherited from the process.
346+
// Both halves of the CI repair live outside this process — one in the
347+
// guard's source, one in the workflow — so two cells read repo files.
348+
const repoFile = (rel) => fs.readFileSync(path.join(__dirname, '../../../..', rel), 'utf8');
349+
350+
let savedBaseRef;
351+
beforeEach(() => {
352+
savedBaseRef = process.env.GITHUB_BASE_REF;
353+
delete process.env.GITHUB_BASE_REF;
354+
});
355+
afterEach(() => {
356+
if (savedBaseRef === undefined) delete process.env.GITHUB_BASE_REF;
357+
else process.env.GITHUB_BASE_REF = savedBaseRef;
358+
});
359+
360+
// #1089's real relation: the branch pinned 70bd82b8 while the base
361+
// carried 5d88a3f1, two commits ahead.
362+
const ROLLED_BACK_BASE = '5d88a3f1bf8abbbbbbbbbbbbbbbbbbbbbbbbbbbb';
363+
364+
it('reports rollback when the pin is a strict ancestor of the base pin', () => {
365+
const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: true });
366+
const result = checkPinDirection({ exec });
367+
expect(result.state).toBe('rollback');
368+
expect(result.detail).toMatch(/moved BACKWARD/);
369+
// The remedy has to be in the message: the author of the next one will
370+
// not have typed a submodule command either.
371+
expect(result.detail).toMatch(/update-index --cacheinfo/);
372+
});
373+
374+
// These next two do NOT discriminate against main: main has no direction
375+
// check at all, so nothing here reds with the fix reverted. Measured, not
376+
// assumed. They are future-regression guards on the narrowness the check
377+
// promises, which is the same role (and the same honest label) as the
378+
// known-limit pin on #2491. Kept because the obvious way to break this
379+
// check later is to widen it until it second-guesses containment.
380+
it('stays silent when the pin is unchanged from the base', () => {
381+
const exec = directionRepoWhere({ basePin: null, isAncestor: true });
382+
expect(checkPinDirection({ exec }).state).toBe('ok');
383+
});
384+
385+
// The cell #1090 could not exercise. A forward bump IS an ancestry
386+
// answer of "no" in the probe's direction, which is exactly what a
387+
// diverged pin answers too — so one scripted case covers the claim the
388+
// original PR could only argue: neither is reported as a rollback.
389+
it('stays out of the way for a forward bump or a diverged pin', () => {
390+
const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: false });
391+
expect(checkPinDirection({ exec }).state).toBe('ok');
392+
});
393+
394+
/**
395+
* CI's real topology, and the reason this cell used to assert the wrong
396+
* thing. `tests.yml`'s tool-tests job checked out with no `fetch-depth`,
397+
* so `origin/main` did not exist and there was no shared history to take
398+
* a merge base from. @otto measured that on #2497: the probe returned
399+
* undetermined, the caller dropped it, and the check never fired on a
400+
* single pull request. This cell asserted `contained` and therefore
401+
* PINNED that silence as acceptable.
402+
*
403+
* Two halves to the repair, and this is the half that belongs here: an
404+
* unresolvable base is LOUD. The other half is the checkout depth, which
405+
* belongs in the workflow and is pinned by the last test in this block —
406+
* not by a fetch from inside the guard, which would both hide a
407+
* misconfigured job and pay the cost this file measures at 11.4s/314M.
408+
*/
409+
it('reports undetermined, NOT silence, when no base ref resolves', () => {
410+
const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: true, resolvableRefs: [] });
411+
const result = checkPinDirection({ exec });
412+
expect(result.state).toBe('undetermined');
413+
expect(result.detail).toMatch(/NOT a pass/);
414+
// Name the repair, or the next person reads this as flakiness.
415+
expect(result.detail).toMatch(/fetch-depth: 0/);
416+
// And it must not try to buy the history itself.
417+
expect(exec.supCalls.some((c) => c[0] === 'fetch')).toBe(false);
418+
});
419+
420+
/**
421+
* A stacked PR's base is not main. Taking the merge base with main would
422+
* reach behind the real base and read a pin the PR never touched, which
423+
* is a FALSE rollback — the failure mode this check cannot afford, since
424+
* a check that cries wolf gets switched off.
425+
*
426+
* This is about ORDER, not fallback: at `fetch-depth: 0` BOTH refs
427+
* resolve, so a probe that merely fell back to the base branch when main
428+
* was missing would be dead code in CI and would answer against main on
429+
* every stacked PR.
430+
*/
431+
it('takes the base the pull request targets before main', () => {
432+
process.env.GITHUB_BASE_REF = 'nova/stack-parent';
433+
const exec = directionRepoWhere({
434+
basePin: ROLLED_BACK_BASE,
435+
isAncestor: true,
436+
resolvableRefs: ['origin/nova/stack-parent', 'origin/main'],
437+
});
438+
expect(checkPinDirection({ exec }).state).toBe('rollback');
439+
const asked = exec.supCalls.filter((c) => c[0] === 'merge-base').map((c) => c[2]);
440+
expect(asked[0]).toBe('origin/nova/stack-parent');
441+
// And it stops there — asking main as well would read a second, older
442+
// pin and make the verdict depend on which answer came last.
443+
expect(asked).not.toContain('origin/main');
444+
});
445+
446+
it('falls back to main when no base branch is named', () => {
447+
const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: true });
448+
expect(checkPinDirection({ exec }).state).toBe('rollback');
449+
expect(exec.supCalls.filter((c) => c[0] === 'merge-base').map((c) => c[2])).toEqual(['origin/main']);
450+
});
451+
452+
/**
453+
* The verdict has to reach the exit code. This is the defect @otto found
454+
* on #2497: the probe computed `undetermined` correctly and the caller
455+
* dropped it on the floor, so the check was structurally incapable of
456+
* failing a run. `main()` cannot be exercised through the injected exec
457+
* — it reads the live repo and exits the process — so this reads the
458+
* wiring out of the source. A source assertion is a weak instrument and
459+
* is named as one: it proves the branch exists, not that it behaves.
460+
* The behaviour of the two states is pinned by the cells above.
461+
*/
462+
it('wires both direction verdicts into the exit code', () => {
463+
const source = repoFile('scripts/verify-moltbot-tool-contract.js');
464+
const main = source.slice(source.indexOf('const main = '));
465+
// The last `if (…) process.exit(n)` in main() is the verdict line;
466+
// the earlier bare `process.exit(2)` calls are the can't-run bailouts.
467+
const verdict = (code) => main.match(new RegExp(`^\\s*if \\(.*process\\.exit\\(${code}\\);$`, 'gm')).pop();
468+
const exitOne = verdict(1);
469+
const exitTwo = verdict(2);
470+
expect(exitOne).toContain("direction.state === 'rollback'");
471+
expect(exitTwo).toContain("direction.state === 'undetermined'");
472+
});
473+
474+
/**
475+
* The other half of the CI repair, and the one a mocked exec cannot
476+
* reach: the job that runs this guard has to check out deep enough for
477+
* `origin/main` to exist. Without this the probe is correct and silent —
478+
* it reports undetermined on every pull request and the direction check
479+
* is, in practice, off. That was the live state until #2497.
480+
*/
481+
it('runs in a CI job whose checkout can reach origin/main', () => {
482+
const workflow = repoFile('.github/workflows/tests.yml');
483+
const jobs = workflow.split(/\n {2}(?=[a-z][\w-]*:\n)/);
484+
const job = jobs.find((block) => block.includes('verify:moltbot-tools'));
485+
expect(job).toBeDefined();
486+
// Scoped to the job that actually runs the guard: a `fetch-depth` on
487+
// some other job is exactly the thing that looks like a pass and isn't.
488+
expect(job).toMatch(/fetch-depth:\s*0/);
489+
});
490+
294491
/**
295492
* A SHALLOW checkout answers this question confidently and wrongly.
296493
*
@@ -314,18 +511,27 @@ describe('moltbot tool contract', () => {
314511
* depth-1 is-ancestor → 1 (wrong) .git 35M
315512
* after --deepen=64 is-ancestor → 0 (correct) .git 36M
316513
*/
317-
const shallowRepoWhere = ({ ancestry, tip = 'a-different-sha' }) => {
514+
const shallowRepoWhere = ({ ancestry, tip = 'a-different-sha', refMissing = false }) => {
318515
const calls = [];
516+
// Containment only ever talks to the submodule; direction is a
517+
// separate function with its own harness above. `submoduleCalls` keeps
518+
// that explicit so a future merge of the two cannot make these rung
519+
// assertions count a superproject call.
520+
const submoduleCalls = [];
319521
let fetched = 0;
320-
const exec = jest.fn((_bin, args) => {
522+
const exec = jest.fn((_bin, args, opts = {}) => {
321523
calls.push(args);
524+
if (String(opts.cwd || '').includes('_external')) submoduleCalls.push(args);
322525
if (args[0] === 'rev-parse' && args[1] === '--is-shallow-repository') {
323526
// Real git keeps reporting `true` after a successful --deepen; only
324527
// --unshallow clears it. Modelled exactly, because a probe that
325528
// flipped to false would hide a bug in the ladder's exit condition.
326529
return calls.some((c) => c.includes('--unshallow')) ? 'false\n' : 'true\n';
327530
}
328-
if (args[0] === 'rev-parse' && args[1] === '--verify') return '';
531+
if (args[0] === 'rev-parse' && args[1] === '--verify') {
532+
if (refMissing) throw Object.assign(new Error('no such ref'), { status: 1 });
533+
return '';
534+
}
329535
if (args[0] === 'rev-parse') return `${tip}\n`;
330536
if (args[0] === 'fetch') { fetched += 1; return ''; }
331537
if (args[0] === 'merge-base') {
@@ -340,7 +546,9 @@ describe('moltbot tool contract', () => {
340546
const rungs = () => calls
341547
.filter((c) => c[0] === 'fetch')
342548
.map((c) => c.find((a) => /^--(deepen=\d+|unshallow)$/.test(a)));
343-
return { exec, calls, rungs };
549+
return {
550+
exec, calls, submoduleCalls, rungs,
551+
};
344552
};
345553

346554
it('climbs the deepen ladder and reports contained once history reaches the pin', () => {
@@ -447,15 +655,20 @@ describe('moltbot tool contract', () => {
447655
*/
448656
it('reports contained without ancestry when the pin is the branch tip', () => {
449657
const pin = readGitlinkSha({ full: true });
450-
const { exec, calls } = shallowRepoWhere({
658+
const { exec, submoduleCalls } = shallowRepoWhere({
451659
ancestry: () => { throw new Error('ancestry must not be consulted here'); },
452660
tip: pin,
453661
});
454662
const result = checkPinReachable({ exec });
455663
expect(result.state).toBe('contained');
456664
expect(result.detail).toMatch(/tip/);
457665
// Shallowness is irrelevant on this path — assert we never got that far.
458-
expect(calls.some((c) => c[0] === 'merge-base')).toBe(false);
666+
// Scoped to the SUBMODULE deliberately: this used to read `calls`, and
667+
// the direction check now makes one `merge-base` call in the
668+
// superproject on every path, which would red this test for a reason
669+
// it was not written about. The ancestry walk is what must not happen
670+
// here, and `submoduleCalls` is exactly that.
671+
expect(submoduleCalls.some((c) => c[0] === 'merge-base')).toBe(false);
459672
});
460673

461674
it('reports orphaned when --is-ancestor exits 1 on a full clone', () => {
@@ -501,8 +714,14 @@ describe('moltbot tool contract', () => {
501714
* and both hand back a usable ref — so this test pins the flag itself.
502715
*/
503716
it('caps the first fetch at depth 1 when the repo is shallow', () => {
504-
const { exec, calls } = shallowRepoWhere({ ancestry: (fetched) => (fetched >= 2 ? 0 : 1) });
505-
exec.mockImplementationOnce(() => { throw gitError(1); }); // no origin/<branch> ref
717+
// `mockImplementationOnce` used to carry this, keyed on the ref probe
718+
// being the FIRST exec call. Targeted by shape instead: a positional
719+
// mock makes this cell silently stop testing ref-absence the moment a
720+
// call is added ahead of it.
721+
const { exec, calls } = shallowRepoWhere({
722+
ancestry: (fetched) => (fetched >= 2 ? 0 : 1),
723+
refMissing: true,
724+
});
506725
checkPinReachable({ exec });
507726
const first = calls.find((c) => c[0] === 'fetch');
508727
expect(first).toContain('--depth=1');

0 commit comments

Comments
 (0)