From 06ad81c19ec32e0f7e9c15397cc40028bb86514d Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 02:09:17 -0700 Subject: [PATCH 1/3] fix(ci): the tool-contract guard sees a gitlink rollback, with the cells tested MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 9ac61bdef 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 Co-authored-by: nova Agent-Model: claude/claude-opus-5/high --- .../unit/scripts/moltbotToolContract.test.js | 125 ++++++++++++++++-- scripts/verify-moltbot-tool-contract.js | 67 +++++++++- 2 files changed, 183 insertions(+), 9 deletions(-) diff --git a/backend/__tests__/unit/scripts/moltbotToolContract.test.js b/backend/__tests__/unit/scripts/moltbotToolContract.test.js index eb0b5cbcbe..df089a04db 100644 --- a/backend/__tests__/unit/scripts/moltbotToolContract.test.js +++ b/backend/__tests__/unit/scripts/moltbotToolContract.test.js @@ -291,6 +291,92 @@ describe('moltbot tool contract', () => { expect(checkPinReachable({ exec }).state).toBe('contained'); }); + /** + * DIRECTION, which containment structurally cannot judge. + * + * A rollback is by definition to a commit the declared branch already + * had, so "the pin is an ancestor of the branch" is TRUE of every + * rollback. Measured on main 9ac61bdef before the fix: the scripted + * repo below returned `contained` — the guard passed #1089's exact + * shape. #1089 was caught because ux-lead read the diff by hand. + * + * The five cells are scripted through the injected exec rather than + * with real commits, because the original fix (#1090) said outright + * that it could not exercise the forward-bump cell — "no sha exists + * that is a descendant of the base pin … both are argued rather than + * exercised". Through this seam all five are exercised, and the + * superproject and submodule calls are told apart by cwd, not by call + * order, so a reordering cannot make these vacuous. + */ + const directionRepoWhere = ({ basePin, isAncestor, baseResolves = true }) => { + const calls = []; + return jest.fn((_bin, args, opts = {}) => { + const inSubmodule = String(opts.cwd || '').includes('_external'); + calls.push({ args, inSubmodule }); + if (!inSubmodule && args[0] === 'merge-base') { + if (!baseResolves) throw Object.assign(new Error('no merge base'), { status: 1 }); + return 'basecommitsha\n'; + } + if (!inSubmodule && args[0] === 'ls-tree') { + return basePin ? `160000 commit ${basePin}\t_external/clawdbot\n` : '\n'; + } + if (args[0] === 'rev-parse' && args[1] === '--is-shallow-repository') return 'false\n'; + if (args[0] === 'merge-base' && args[1] === '--is-ancestor') { + // The FIRST submodule ancestry call is the direction probe (pin vs + // base pin); later ones are containment's own walk. + const probes = calls.filter((c) => c.inSubmodule && c.args[1] === '--is-ancestor'); + if (probes.length === 1 && !isAncestor) { + throw Object.assign(new Error('not an ancestor'), { status: 1 }); + } + return ''; + } + return ''; + }); + }; + + // #1089's real relation: the branch pinned 70bd82b8 while the base + // carried 5d88a3f1, two commits ahead. + const ROLLED_BACK_BASE = '5d88a3f1bf8abbbbbbbbbbbbbbbbbbbbbbbbbbbb'; + + it('reports rollback when the pin is a strict ancestor of the base pin', () => { + const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: true }); + const result = checkPinReachable({ exec }); + expect(result.state).toBe('rollback'); + expect(result.detail).toMatch(/moved BACKWARD/); + // The remedy has to be in the message: the author of the next one will + // not have typed a submodule command either. + expect(result.detail).toMatch(/update-index --cacheinfo/); + }); + + // These next two do NOT discriminate against main: main has no direction + // check at all, so it answers `contained` here too, and both pass with + // the fix reverted. Measured, not assumed — the revert mutation reds 2 of + // these 4 cells. They are future-regression guards on the narrowness the + // check promises, which is the same role (and the same honest label) as + // the known-limit pin on #2491. Kept because the obvious way to break + // this check later is to widen it until it second-guesses containment. + it('reports contained, not rollback, when the pin is unchanged from the base', () => { + const exec = directionRepoWhere({ basePin: null, isAncestor: true }); + expect(checkPinReachable({ exec }).state).toBe('contained'); + }); + + // The cell #1090 could not exercise. A forward bump IS an ancestry + // answer of "no" in the probe's direction, which is exactly what a + // diverged pin answers too — so one scripted case covers the claim the + // original PR could only argue: neither is reported as a rollback. + it('stays out of the way for a forward bump or a diverged pin', () => { + const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: false }); + expect(checkPinReachable({ exec }).state).toBe('contained'); + }); + + it('reports contained, not rollback, when the merge base cannot be resolved', () => { + // A shallow superproject clone has no merge base with origin/main. + // Undeterminable must not be a finding: containment still runs. Also + // non-discriminating against main — see the note above. + const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: true, baseResolves: false }); + expect(checkPinReachable({ exec }).state).toBe('contained'); + }); + /** * A SHALLOW checkout answers this question confidently and wrongly. * @@ -314,18 +400,27 @@ describe('moltbot tool contract', () => { * depth-1 is-ancestor → 1 (wrong) .git 35M * after --deepen=64 is-ancestor → 0 (correct) .git 36M */ - const shallowRepoWhere = ({ ancestry, tip = 'a-different-sha' }) => { + const shallowRepoWhere = ({ ancestry, tip = 'a-different-sha', refMissing = false }) => { const calls = []; + // Where a call was made matters now that the guard also probes the + // SUPERPROJECT for the merge base (direction check below). `calls` + // stays argv arrays so the existing rung assertions are untouched; + // `submoduleCalls` is what "did containment walk ancestry" means. + const submoduleCalls = []; let fetched = 0; - const exec = jest.fn((_bin, args) => { + const exec = jest.fn((_bin, args, opts = {}) => { calls.push(args); + if (String(opts.cwd || '').includes('_external')) submoduleCalls.push(args); if (args[0] === 'rev-parse' && args[1] === '--is-shallow-repository') { // Real git keeps reporting `true` after a successful --deepen; only // --unshallow clears it. Modelled exactly, because a probe that // flipped to false would hide a bug in the ladder's exit condition. return calls.some((c) => c.includes('--unshallow')) ? 'false\n' : 'true\n'; } - if (args[0] === 'rev-parse' && args[1] === '--verify') return ''; + if (args[0] === 'rev-parse' && args[1] === '--verify') { + if (refMissing) throw Object.assign(new Error('no such ref'), { status: 1 }); + return ''; + } if (args[0] === 'rev-parse') return `${tip}\n`; if (args[0] === 'fetch') { fetched += 1; return ''; } if (args[0] === 'merge-base') { @@ -340,7 +435,9 @@ describe('moltbot tool contract', () => { const rungs = () => calls .filter((c) => c[0] === 'fetch') .map((c) => c.find((a) => /^--(deepen=\d+|unshallow)$/.test(a))); - return { exec, calls, rungs }; + return { + exec, calls, submoduleCalls, rungs, + }; }; it('climbs the deepen ladder and reports contained once history reaches the pin', () => { @@ -447,7 +544,7 @@ describe('moltbot tool contract', () => { */ it('reports contained without ancestry when the pin is the branch tip', () => { const pin = readGitlinkSha({ full: true }); - const { exec, calls } = shallowRepoWhere({ + const { exec, submoduleCalls } = shallowRepoWhere({ ancestry: () => { throw new Error('ancestry must not be consulted here'); }, tip: pin, }); @@ -455,7 +552,12 @@ describe('moltbot tool contract', () => { expect(result.state).toBe('contained'); expect(result.detail).toMatch(/tip/); // Shallowness is irrelevant on this path — assert we never got that far. - expect(calls.some((c) => c[0] === 'merge-base')).toBe(false); + // Scoped to the SUBMODULE deliberately: this used to read `calls`, and + // the direction check now makes one `merge-base` call in the + // superproject on every path, which would red this test for a reason + // it was not written about. The ancestry walk is what must not happen + // here, and `submoduleCalls` is exactly that. + expect(submoduleCalls.some((c) => c[0] === 'merge-base')).toBe(false); }); it('reports orphaned when --is-ancestor exits 1 on a full clone', () => { @@ -501,8 +603,15 @@ describe('moltbot tool contract', () => { * and both hand back a usable ref — so this test pins the flag itself. */ it('caps the first fetch at depth 1 when the repo is shallow', () => { - const { exec, calls } = shallowRepoWhere({ ancestry: (fetched) => (fetched >= 2 ? 0 : 1) }); - exec.mockImplementationOnce(() => { throw gitError(1); }); // no origin/ ref + // `mockImplementationOnce` used to carry this: the first exec call WAS + // the `rev-parse --verify` ref probe. The direction check now calls + // `merge-base` in the superproject first, so a positional mock lands on + // the wrong call and the ref-absence case silently stops being tested. + // Target it by shape instead. + const { exec, calls } = shallowRepoWhere({ + ancestry: (fetched) => (fetched >= 2 ? 0 : 1), + refMissing: true, + }); checkPinReachable({ exec }); const first = calls.find((c) => c[0] === 'fetch'); expect(first).toContain('--depth=1'); diff --git a/scripts/verify-moltbot-tool-contract.js b/scripts/verify-moltbot-tool-contract.js index 32da19c2c0..29db678914 100644 --- a/scripts/verify-moltbot-tool-contract.js +++ b/scripts/verify-moltbot-tool-contract.js @@ -393,6 +393,63 @@ const checkPinReachable = ({ exec = execFileSync } = {}) => { return { state: 'undetermined', branch, pin, detail: 'could not read the gitlink sha from HEAD' }; } + /** + * Did this pin move FORWARD from what the merge base carries, or backward? + * + * Containment — "the pin is on the declared branch" — cannot catch a + * ROLLBACK, because a rollback is by definition to a commit the branch + * already had. Measured on #1089: `compare/70bd82b8...5d88a3f1` returned + * `status=ahead, behind_by=0`, so the check above passed while the live + * gateway reverted two commits. A human caught it by reading the diff. + * + * How the bad pin gets there is why this must not key on intent: `git add -A` + * in a checkout whose submodule sits at an older commit stages that stale + * pointer as if it were an edit. The author types no submodule command, so a + * guard keyed on a title or a label misses the entire population — the + * regression happens precisely in PRs that believe they have nothing to do + * with the submodule. + * + * Returns 'rollback' ONLY for a strict ancestor of the merge base's pin. + * Unchanged, forward, diverged and undeterminable all fall through, because + * containment already judges those and this must not become a second opinion + * on a check that works. + */ + function checkPinDirection(gitlink) { + const sup = (args) => exec('git', args, { cwd: REPO_ROOT, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); + const sub = (args) => exec('git', args, { cwd: submodule, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); + + let basePin; + try { + const base = String(sup(['merge-base', 'HEAD', 'origin/main'])).trim(); + const line = String(sup(['ls-tree', base, '_external/clawdbot'])).trim(); + basePin = line.split(/\s+/)[2]; + } catch { + return { state: 'undetermined', detail: 'could not resolve the merge base with origin/main' }; + } + if (!basePin || basePin === gitlink) return { state: 'ok' }; + + try { + sub(['merge-base', '--is-ancestor', gitlink, basePin]); + } catch { + // Not an ancestor (status 1) or the object is absent (128). Forward, + // diverged and unknown all land here, and all are containment's problem. + return { state: 'ok' }; + } + return { + state: 'rollback', + detail: `the gitlink moved BACKWARD: ${gitlink.slice(0, 12)} is an ancestor of the base's ${basePin.slice(0, 12)}. ` + + 'If you did not mean to touch the submodule, this is `git add -A` staging a stale checkout. ' + + 'Repair with `git update-index --cacheinfo 160000,,_external/clawdbot` — note that ' + + '`git checkout origin/main -- _external/clawdbot` stages the right sha and ANY later `git add` ' + + 'of that path overwrites it again from the working tree.', + }; + } + + const direction = checkPinDirection(pin); + if (direction.state === 'rollback') { + return { state: 'rollback', branch, pin, detail: direction.detail }; + } + const git = (args) => exec('git', args, { cwd: submodule, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); const isShallow = () => { @@ -851,6 +908,14 @@ const main = () => { // simultaneously tool-complete and unreachable (that is exactly #840), and // exiting on the first failure would hide whichever ran second. const reach = checkPinReachable(); + if (reach.state === 'rollback') { + console.error( + '[moltbot-tool-contract] FAIL — the submodule pin moved BACKWARD.\n\n' + + ` pin ${reach.pin}\n` + + ` declared branch ${reach.branch}\n` + + ` ${reach.detail}\n\n`, + ); + } if (reach.state === 'orphaned') { console.error( `[moltbot-tool-contract] FAIL — the pin is not on the branch .gitmodules declares.\n` @@ -938,7 +1003,7 @@ const main = () => { // Severity order: a proven violation (1) outranks an unrun check (2) only // because a violation is actionable now. Both are non-zero; neither is a pass. - if (missing.length || presetCheck.missing.length || reach.state === 'orphaned') process.exit(1); + if (missing.length || presetCheck.missing.length || reach.state === 'orphaned' || reach.state === 'rollback') process.exit(1); if (presetCheck.uncovered.length || reach.state === 'undetermined') process.exit(2); }; From 31dda881a38ddc5a3b57f9361dee9232ad19a2cc Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 02:30:03 -0700 Subject: [PATCH 2/3] fix(ci): make the pin-direction check actually run in CI, and say so when it cannot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @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 Co-authored-by: nova Agent-Model: claude/claude-opus-5/high --- .github/workflows/tests.yml | 7 + .../unit/scripts/moltbotToolContract.test.js | 182 ++++++++++++++---- scripts/verify-moltbot-tool-contract.js | 180 +++++++++++------ 3 files changed, 271 insertions(+), 98 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 7616250f3c..d7a92996ae 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -130,6 +130,13 @@ jobs: - uses: actions/checkout@v4 with: submodules: recursive + # The tool-contract guard compares the submodule pin against the + # merge base with origin/main to catch a ROLLBACK, which containment + # structurally cannot see. At the default depth 1 there is no + # origin/main and no shared history, so that check could not run at + # all (#2497). The guard now says so loudly instead of passing, and + # this is what lets it answer. + fetch-depth: 0 - uses: actions/setup-node@v4 with: node-version: '22' diff --git a/backend/__tests__/unit/scripts/moltbotToolContract.test.js b/backend/__tests__/unit/scripts/moltbotToolContract.test.js index df089a04db..433a501dd1 100644 --- a/backend/__tests__/unit/scripts/moltbotToolContract.test.js +++ b/backend/__tests__/unit/scripts/moltbotToolContract.test.js @@ -24,6 +24,7 @@ const { loadMentionCues, readDeclaredBranch, checkPinReachable, + checkPinDirection, readGitlinkSha, stripComments, MENTION_CUES, @@ -308,39 +309,61 @@ describe('moltbot tool contract', () => { * superproject and submodule calls are told apart by cwd, not by call * order, so a reordering cannot make these vacuous. */ - const directionRepoWhere = ({ basePin, isAncestor, baseResolves = true }) => { + const directionRepoWhere = ({ basePin, isAncestor, resolvableRefs = ['origin/main'] }) => { const calls = []; - return jest.fn((_bin, args, opts = {}) => { + const supCalls = []; + const mock = jest.fn((_bin, args, opts = {}) => { const inSubmodule = String(opts.cwd || '').includes('_external'); calls.push({ args, inSubmodule }); + if (!inSubmodule) supCalls.push(args); if (!inSubmodule && args[0] === 'merge-base') { - if (!baseResolves) throw Object.assign(new Error('no merge base'), { status: 1 }); + // `merge-base HEAD `. A ref the checkout never fetched is not + // a git failure with a special status — the command simply exits + // non-zero, the same as "no common history", which is why the probe + // cannot tell them apart and must say undetermined for both. + if (!resolvableRefs.includes(args[2])) { + throw Object.assign(new Error('not a valid object name'), { status: 128 }); + } return 'basecommitsha\n'; } if (!inSubmodule && args[0] === 'ls-tree') { return basePin ? `160000 commit ${basePin}\t_external/clawdbot\n` : '\n'; } - if (args[0] === 'rev-parse' && args[1] === '--is-shallow-repository') return 'false\n'; - if (args[0] === 'merge-base' && args[1] === '--is-ancestor') { - // The FIRST submodule ancestry call is the direction probe (pin vs - // base pin); later ones are containment's own walk. - const probes = calls.filter((c) => c.inSubmodule && c.args[1] === '--is-ancestor'); - if (probes.length === 1 && !isAncestor) { - throw Object.assign(new Error('not an ancestor'), { status: 1 }); - } + if (inSubmodule && args[0] === 'merge-base' && args[1] === '--is-ancestor') { + if (!isAncestor) throw Object.assign(new Error('not an ancestor'), { status: 1 }); return ''; } return ''; }); + mock.supCalls = supCalls; + return mock; }; + // The probe reads GITHUB_BASE_REF, which the Actions runner SETS — and + // sets to the PR's base, so on a stacked PR these cells would inherit a + // ref their harness does not resolve and red in CI only. Each cell states + // its own base; nothing is inherited from the process. + // Both halves of the CI repair live outside this process — one in the + // guard's source, one in the workflow — so two cells read repo files. + const repoFile = (rel) => fs.readFileSync(path.join(__dirname, '../../../..', rel), 'utf8'); + + let savedBaseRef; + beforeEach(() => { + savedBaseRef = process.env.GITHUB_BASE_REF; + delete process.env.GITHUB_BASE_REF; + }); + afterEach(() => { + if (savedBaseRef === undefined) delete process.env.GITHUB_BASE_REF; + else process.env.GITHUB_BASE_REF = savedBaseRef; + }); + // #1089's real relation: the branch pinned 70bd82b8 while the base // carried 5d88a3f1, two commits ahead. const ROLLED_BACK_BASE = '5d88a3f1bf8abbbbbbbbbbbbbbbbbbbbbbbbbbbb'; it('reports rollback when the pin is a strict ancestor of the base pin', () => { const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: true }); - const result = checkPinReachable({ exec }); + const result = checkPinDirection({ exec }); expect(result.state).toBe('rollback'); expect(result.detail).toMatch(/moved BACKWARD/); // The remedy has to be in the message: the author of the next one will @@ -349,15 +372,14 @@ describe('moltbot tool contract', () => { }); // These next two do NOT discriminate against main: main has no direction - // check at all, so it answers `contained` here too, and both pass with - // the fix reverted. Measured, not assumed — the revert mutation reds 2 of - // these 4 cells. They are future-regression guards on the narrowness the - // check promises, which is the same role (and the same honest label) as - // the known-limit pin on #2491. Kept because the obvious way to break - // this check later is to widen it until it second-guesses containment. - it('reports contained, not rollback, when the pin is unchanged from the base', () => { + // check at all, so nothing here reds with the fix reverted. Measured, not + // assumed. They are future-regression guards on the narrowness the check + // promises, which is the same role (and the same honest label) as the + // known-limit pin on #2491. Kept because the obvious way to break this + // check later is to widen it until it second-guesses containment. + it('stays silent when the pin is unchanged from the base', () => { const exec = directionRepoWhere({ basePin: null, isAncestor: true }); - expect(checkPinReachable({ exec }).state).toBe('contained'); + expect(checkPinDirection({ exec }).state).toBe('ok'); }); // The cell #1090 could not exercise. A forward bump IS an ancestry @@ -366,15 +388,104 @@ describe('moltbot tool contract', () => { // original PR could only argue: neither is reported as a rollback. it('stays out of the way for a forward bump or a diverged pin', () => { const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: false }); - expect(checkPinReachable({ exec }).state).toBe('contained'); + expect(checkPinDirection({ exec }).state).toBe('ok'); }); - it('reports contained, not rollback, when the merge base cannot be resolved', () => { - // A shallow superproject clone has no merge base with origin/main. - // Undeterminable must not be a finding: containment still runs. Also - // non-discriminating against main — see the note above. - const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: true, baseResolves: false }); - expect(checkPinReachable({ exec }).state).toBe('contained'); + /** + * CI's real topology, and the reason this cell used to assert the wrong + * thing. `tests.yml`'s tool-tests job checked out with no `fetch-depth`, + * so `origin/main` did not exist and there was no shared history to take + * a merge base from. @otto measured that on #2497: the probe returned + * undetermined, the caller dropped it, and the check never fired on a + * single pull request. This cell asserted `contained` and therefore + * PINNED that silence as acceptable. + * + * Two halves to the repair, and this is the half that belongs here: an + * unresolvable base is LOUD. The other half is the checkout depth, which + * belongs in the workflow and is pinned by the last test in this block — + * not by a fetch from inside the guard, which would both hide a + * misconfigured job and pay the cost this file measures at 11.4s/314M. + */ + it('reports undetermined, NOT silence, when no base ref resolves', () => { + const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: true, resolvableRefs: [] }); + const result = checkPinDirection({ exec }); + expect(result.state).toBe('undetermined'); + expect(result.detail).toMatch(/NOT a pass/); + // Name the repair, or the next person reads this as flakiness. + expect(result.detail).toMatch(/fetch-depth: 0/); + // And it must not try to buy the history itself. + expect(exec.supCalls.some((c) => c[0] === 'fetch')).toBe(false); + }); + + /** + * A stacked PR's base is not main. Taking the merge base with main would + * reach behind the real base and read a pin the PR never touched, which + * is a FALSE rollback — the failure mode this check cannot afford, since + * a check that cries wolf gets switched off. + * + * This is about ORDER, not fallback: at `fetch-depth: 0` BOTH refs + * resolve, so a probe that merely fell back to the base branch when main + * was missing would be dead code in CI and would answer against main on + * every stacked PR. + */ + it('takes the base the pull request targets before main', () => { + process.env.GITHUB_BASE_REF = 'nova/stack-parent'; + const exec = directionRepoWhere({ + basePin: ROLLED_BACK_BASE, + isAncestor: true, + resolvableRefs: ['origin/nova/stack-parent', 'origin/main'], + }); + expect(checkPinDirection({ exec }).state).toBe('rollback'); + const asked = exec.supCalls.filter((c) => c[0] === 'merge-base').map((c) => c[2]); + expect(asked[0]).toBe('origin/nova/stack-parent'); + // And it stops there — asking main as well would read a second, older + // pin and make the verdict depend on which answer came last. + expect(asked).not.toContain('origin/main'); + }); + + it('falls back to main when no base branch is named', () => { + const exec = directionRepoWhere({ basePin: ROLLED_BACK_BASE, isAncestor: true }); + expect(checkPinDirection({ exec }).state).toBe('rollback'); + expect(exec.supCalls.filter((c) => c[0] === 'merge-base').map((c) => c[2])).toEqual(['origin/main']); + }); + + /** + * The verdict has to reach the exit code. This is the defect @otto found + * on #2497: the probe computed `undetermined` correctly and the caller + * dropped it on the floor, so the check was structurally incapable of + * failing a run. `main()` cannot be exercised through the injected exec + * — it reads the live repo and exits the process — so this reads the + * wiring out of the source. A source assertion is a weak instrument and + * is named as one: it proves the branch exists, not that it behaves. + * The behaviour of the two states is pinned by the cells above. + */ + it('wires both direction verdicts into the exit code', () => { + const source = repoFile('scripts/verify-moltbot-tool-contract.js'); + const main = source.slice(source.indexOf('const main = ')); + // The last `if (…) process.exit(n)` in main() is the verdict line; + // the earlier bare `process.exit(2)` calls are the can't-run bailouts. + const verdict = (code) => main.match(new RegExp(`^\\s*if \\(.*process\\.exit\\(${code}\\);$`, 'gm')).pop(); + const exitOne = verdict(1); + const exitTwo = verdict(2); + expect(exitOne).toContain("direction.state === 'rollback'"); + expect(exitTwo).toContain("direction.state === 'undetermined'"); + }); + + /** + * The other half of the CI repair, and the one a mocked exec cannot + * reach: the job that runs this guard has to check out deep enough for + * `origin/main` to exist. Without this the probe is correct and silent — + * it reports undetermined on every pull request and the direction check + * is, in practice, off. That was the live state until #2497. + */ + it('runs in a CI job whose checkout can reach origin/main', () => { + const workflow = repoFile('.github/workflows/tests.yml'); + const jobs = workflow.split(/\n {2}(?=[a-z][\w-]*:\n)/); + const job = jobs.find((block) => block.includes('verify:moltbot-tools')); + expect(job).toBeDefined(); + // Scoped to the job that actually runs the guard: a `fetch-depth` on + // some other job is exactly the thing that looks like a pass and isn't. + expect(job).toMatch(/fetch-depth:\s*0/); }); /** @@ -402,10 +513,10 @@ describe('moltbot tool contract', () => { */ const shallowRepoWhere = ({ ancestry, tip = 'a-different-sha', refMissing = false }) => { const calls = []; - // Where a call was made matters now that the guard also probes the - // SUPERPROJECT for the merge base (direction check below). `calls` - // stays argv arrays so the existing rung assertions are untouched; - // `submoduleCalls` is what "did containment walk ancestry" means. + // Containment only ever talks to the submodule; direction is a + // separate function with its own harness above. `submoduleCalls` keeps + // that explicit so a future merge of the two cannot make these rung + // assertions count a superproject call. const submoduleCalls = []; let fetched = 0; const exec = jest.fn((_bin, args, opts = {}) => { @@ -603,11 +714,10 @@ describe('moltbot tool contract', () => { * and both hand back a usable ref — so this test pins the flag itself. */ it('caps the first fetch at depth 1 when the repo is shallow', () => { - // `mockImplementationOnce` used to carry this: the first exec call WAS - // the `rev-parse --verify` ref probe. The direction check now calls - // `merge-base` in the superproject first, so a positional mock lands on - // the wrong call and the ref-absence case silently stops being tested. - // Target it by shape instead. + // `mockImplementationOnce` used to carry this, keyed on the ref probe + // being the FIRST exec call. Targeted by shape instead: a positional + // mock makes this cell silently stop testing ref-absence the moment a + // call is added ahead of it. const { exec, calls } = shallowRepoWhere({ ancestry: (fetched) => (fetched >= 2 ? 0 : 1), refMissing: true, diff --git a/scripts/verify-moltbot-tool-contract.js b/scripts/verify-moltbot-tool-contract.js index 29db678914..e02ec6718b 100644 --- a/scripts/verify-moltbot-tool-contract.js +++ b/scripts/verify-moltbot-tool-contract.js @@ -357,6 +357,107 @@ const readDeclaredBranch = (gitmodulesText) => { return m ? m[1] : null; }; +/** + * Did this pin move FORWARD from what the merge base carries, or backward? + * + * Containment — "the pin is on the declared branch" — cannot catch a + * ROLLBACK, because a rollback is by definition to a commit the branch + * already had. Measured on #1089: `compare/70bd82b8...5d88a3f1` returned + * `status=ahead, behind_by=0`, so the check above passed while the live + * gateway reverted two commits. A human caught it by reading the diff. + * + * How the bad pin gets there is why this must not key on intent: `git add -A` + * in a checkout whose submodule sits at an older commit stages that stale + * pointer as if it were an edit. The author types no submodule command, so a + * guard keyed on a title or a label misses the entire population — the + * regression happens precisely in PRs that believe they have nothing to do + * with the submodule. + * + * Returns 'rollback' ONLY for a strict ancestor of the merge base's pin. + * Unchanged, forward, diverged and undeterminable all fall through, because + * containment already judges those and this must not become a second opinion + * on a check that works. + */ +const checkPinDirection = ({ exec = execFileSync } = {}) => { + const gitlink = readGitlinkSha({ full: true }); + const submodule = path.join(REPO_ROOT, '_external', 'clawdbot'); + if (gitlink === '(unknown)') { + return { state: 'undetermined', pin: gitlink, detail: 'could not read the gitlink sha from HEAD' }; + } + const sup = (args) => exec('git', args, { cwd: REPO_ROOT, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); + const sub = (args) => exec('git', args, { cwd: submodule, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); + + // CI is the case that matters and it is the hostile one. `tests.yml`'s + // tool-tests job used to check out with `submodules: recursive` and NO + // fetch-depth, so actions/checkout took depth 1 and fetched only the PR's + // merge ref: `origin/main` did not exist, no merge base could be computed, + // and this probe returned undetermined on every pull request while the + // caller dropped it — the check never fired. Measured by @otto on #2497. + // Two halves to the fix: that job now checks out with `fetch-depth: 0` + // (the depth belongs in the workflow, not in a git fetch from inside a + // guard, where it would also pollute the fetch assertions this suite makes + // about the submodule ladder), and an unresolvable base is now LOUD. + // + // `GITHUB_BASE_REF` goes FIRST, not last: it is the branch the pull request + // actually targets, and on a stacked PR the merge base with main sits behind + // the real base and carries a pin this PR never touched — a FALSE rollback, + // which is the one failure this check cannot afford. On an ordinary PR the + // two are the same ref. Unset (local runs, push events) falls to main. + const baseCandidates = [ + process.env.GITHUB_BASE_REF ? `origin/${process.env.GITHUB_BASE_REF}` : null, + 'origin/main', + ].filter(Boolean); + + let base; + let resolved; + for (const ref of baseCandidates) { + try { + base = String(sup(['merge-base', 'HEAD', ref])).trim(); + resolved = ref; + break; + } catch { /* try the next candidate */ } + } + if (!resolved) { + return { + state: 'undetermined', + pin: gitlink, + detail: `could not resolve a merge base against ${baseCandidates.join(' or ')}, so the ` + + 'direction of the pin is unknown. This is NOT a pass: a checkout too shallow to reach ' + + 'the base is exactly the state in which a rollback goes unnoticed. If this is CI, the ' + + 'job needs `fetch-depth: 0`.', + }; + } + // An empty success is not a real git answer — it is what a stub returns — + // so treat it as nothing to compare and let containment judge. + if (!base) return { state: 'ok', pin: gitlink }; + + let basePin; + try { + const line = String(sup(['ls-tree', base, '_external/clawdbot'])).trim(); + basePin = line.split(/\s+/)[2]; + } catch { + return { state: 'undetermined', pin: gitlink, detail: `could not read the gitlink at the merge base ${base.slice(0, 12)}` }; + } + if (!basePin || basePin === gitlink) return { state: 'ok', pin: gitlink }; + + try { + sub(['merge-base', '--is-ancestor', gitlink, basePin]); + } catch { + // Not an ancestor (status 1) or the object is absent (128). Forward, + // diverged and unknown all land here, and all are containment's problem. + return { state: 'ok', pin: gitlink }; + } + return { + state: 'rollback', + pin: gitlink, + detail: `the gitlink moved BACKWARD: ${gitlink.slice(0, 12)} is an ancestor of the base's ${basePin.slice(0, 12)}. ` + + 'If you did not mean to touch the submodule, this is `git add -A` staging a stale checkout. ' + + 'Repair with `git update-index --cacheinfo 160000,,_external/clawdbot` — note that ' + + '`git checkout origin/main -- _external/clawdbot` stages the right sha and ANY later `git add` ' + + 'of that path overwrites it again from the working tree.', + }; +}; + /** * Is the pinned commit contained in the branch `.gitmodules` declares? * @@ -393,62 +494,6 @@ const checkPinReachable = ({ exec = execFileSync } = {}) => { return { state: 'undetermined', branch, pin, detail: 'could not read the gitlink sha from HEAD' }; } - /** - * Did this pin move FORWARD from what the merge base carries, or backward? - * - * Containment — "the pin is on the declared branch" — cannot catch a - * ROLLBACK, because a rollback is by definition to a commit the branch - * already had. Measured on #1089: `compare/70bd82b8...5d88a3f1` returned - * `status=ahead, behind_by=0`, so the check above passed while the live - * gateway reverted two commits. A human caught it by reading the diff. - * - * How the bad pin gets there is why this must not key on intent: `git add -A` - * in a checkout whose submodule sits at an older commit stages that stale - * pointer as if it were an edit. The author types no submodule command, so a - * guard keyed on a title or a label misses the entire population — the - * regression happens precisely in PRs that believe they have nothing to do - * with the submodule. - * - * Returns 'rollback' ONLY for a strict ancestor of the merge base's pin. - * Unchanged, forward, diverged and undeterminable all fall through, because - * containment already judges those and this must not become a second opinion - * on a check that works. - */ - function checkPinDirection(gitlink) { - const sup = (args) => exec('git', args, { cwd: REPO_ROOT, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); - const sub = (args) => exec('git', args, { cwd: submodule, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); - - let basePin; - try { - const base = String(sup(['merge-base', 'HEAD', 'origin/main'])).trim(); - const line = String(sup(['ls-tree', base, '_external/clawdbot'])).trim(); - basePin = line.split(/\s+/)[2]; - } catch { - return { state: 'undetermined', detail: 'could not resolve the merge base with origin/main' }; - } - if (!basePin || basePin === gitlink) return { state: 'ok' }; - - try { - sub(['merge-base', '--is-ancestor', gitlink, basePin]); - } catch { - // Not an ancestor (status 1) or the object is absent (128). Forward, - // diverged and unknown all land here, and all are containment's problem. - return { state: 'ok' }; - } - return { - state: 'rollback', - detail: `the gitlink moved BACKWARD: ${gitlink.slice(0, 12)} is an ancestor of the base's ${basePin.slice(0, 12)}. ` - + 'If you did not mean to touch the submodule, this is `git add -A` staging a stale checkout. ' - + 'Repair with `git update-index --cacheinfo 160000,,_external/clawdbot` — note that ' - + '`git checkout origin/main -- _external/clawdbot` stages the right sha and ANY later `git add` ' - + 'of that path overwrites it again from the working tree.', - }; - } - - const direction = checkPinDirection(pin); - if (direction.state === 'rollback') { - return { state: 'rollback', branch, pin, detail: direction.detail }; - } const git = (args) => exec('git', args, { cwd: submodule, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }); @@ -908,12 +953,23 @@ const main = () => { // simultaneously tool-complete and unreachable (that is exactly #840), and // exiting on the first failure would hide whichever ran second. const reach = checkPinReachable(); - if (reach.state === 'rollback') { + // Direction is its own question and its own function: containment asks "is + // the pin on the branch", direction asks "did it move backward", and a + // rollback answers YES to the first. Kept separate so that a containment + // harness cannot answer for direction and vice versa — they were one + // function and the direction verdict was silently dropped (#2497). + const direction = checkPinDirection(); + if (direction.state === 'rollback') { console.error( '[moltbot-tool-contract] FAIL — the submodule pin moved BACKWARD.\n\n' - + ` pin ${reach.pin}\n` + + ` pin ${direction.pin}\n` + ` declared branch ${reach.branch}\n` - + ` ${reach.detail}\n\n`, + + ` ${direction.detail}\n\n`, + ); + } else if (direction.state === 'undetermined') { + console.error( + `[moltbot-tool-contract] CANNOT VERIFY pin direction — ${direction.detail}\n` + + ' Exiting 2, NOT 0.\n', ); } if (reach.state === 'orphaned') { @@ -1003,15 +1059,15 @@ const main = () => { // Severity order: a proven violation (1) outranks an unrun check (2) only // because a violation is actionable now. Both are non-zero; neither is a pass. - if (missing.length || presetCheck.missing.length || reach.state === 'orphaned' || reach.state === 'rollback') process.exit(1); - if (presetCheck.uncovered.length || reach.state === 'undetermined') process.exit(2); + if (missing.length || presetCheck.missing.length || reach.state === 'orphaned' || direction.state === 'rollback') process.exit(1); + if (presetCheck.uncovered.length || reach.state === 'undetermined' || direction.state === 'undetermined') process.exit(2); }; if (require.main === module) main(); module.exports = { parseDeclaredTools, collectRequiredTools, loadCyclesTrailer, loadMentionCues, - readDeclaredBranch, checkPinReachable, readGitlinkSha, + readDeclaredBranch, checkPinReachable, checkPinDirection, readGitlinkSha, parsePresets, checkPresetToolNames, loadNativeTools, readExtensionToolsAtPin, NAMED_AS_REMOVED, PRESETS_FILE, MENTION_CUES, REQUIRED_TOOL_SOURCES, stripComments, From 18b40aa78423cc702b3e5dad559b5dfe19352641 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 23 Oct 2026 02:32:54 -0700 Subject: [PATCH 3/3] fix(ci): say which base the direction check used, on the pass path too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-authored-by: nova Agent-Model: claude/claude-opus-5/high --- scripts/verify-moltbot-tool-contract.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/scripts/verify-moltbot-tool-contract.js b/scripts/verify-moltbot-tool-contract.js index e02ec6718b..6331482f91 100644 --- a/scripts/verify-moltbot-tool-contract.js +++ b/scripts/verify-moltbot-tool-contract.js @@ -429,7 +429,7 @@ const checkPinDirection = ({ exec = execFileSync } = {}) => { } // An empty success is not a real git answer — it is what a stub returns — // so treat it as nothing to compare and let containment judge. - if (!base) return { state: 'ok', pin: gitlink }; + if (!base) return { state: 'ok', pin: gitlink, base: resolved }; let basePin; try { @@ -438,14 +438,14 @@ const checkPinDirection = ({ exec = execFileSync } = {}) => { } catch { return { state: 'undetermined', pin: gitlink, detail: `could not read the gitlink at the merge base ${base.slice(0, 12)}` }; } - if (!basePin || basePin === gitlink) return { state: 'ok', pin: gitlink }; + if (!basePin || basePin === gitlink) return { state: 'ok', pin: gitlink, base: resolved }; try { sub(['merge-base', '--is-ancestor', gitlink, basePin]); } catch { // Not an ancestor (status 1) or the object is absent (128). Forward, // diverged and unknown all land here, and all are containment's problem. - return { state: 'ok', pin: gitlink }; + return { state: 'ok', pin: gitlink, base: resolved }; } return { state: 'rollback', @@ -1056,6 +1056,12 @@ const main = () => { if (reach.state === 'contained') { console.log(`[moltbot-tool-contract] OK — ${reach.detail}, the branch .gitmodules declares.`); } + // 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, and that is how this one stayed dead for two months. + if (direction.state === 'ok') { + console.log(`[moltbot-tool-contract] OK — the pin did not move backward from ${direction.base}.`); + } // Severity order: a proven violation (1) outranks an unrun check (2) only // because a violation is actionable now. Both are non-zero; neither is a pass.