fix(916): a failed substitute is an error, not a landed claim; head_sha described in both schemas; the issue's box says what the code does
The runner-beside-the-lenses design is now what Done-when box 1 asks for (body amended). A rubric round whose runner and worktree fallback both fail returns an error before the push instead of landing a PR whose body claims a substitute reviewed it. The harness records lens count at the runner's start too (expects 0, so a re-serialised runner reddens), resets its counter per round, and has a case for the double failure. Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV
This commit is contained in:
@@ -38,21 +38,24 @@ const finding = (severity) => ({ severity, file: 'f', summary: `${severity} find
|
||||
// Every stub lens returns the same findings, so a finding counts once per lens (two lenses → 2).
|
||||
// reviewRounds: array indexed by round-1 of {findings, lensesNull?}; landOverrides: extra fields on the finisher report;
|
||||
// postRebase: findings returned by the review after a patch-changing rebase, or 'lensesNull'; fixerNull: the fixer agent dies.
|
||||
function run(script, args, { reviewRounds, landOverrides = {}, postRebase = null, fixerNull = false, fixerDone = true, fixerNoCommit = false }) {
|
||||
function run(script, args, { reviewRounds, landOverrides = {}, postRebase = null, fixerNull = false, fixerDone = true, fixerNoCommit = false, fallbackNull = false }) {
|
||||
const calls = [];
|
||||
let round = 0;
|
||||
let fixCount = 0;
|
||||
let lensesReturned = 0;
|
||||
const lensesReturnedWhenFallbackStarted = [];
|
||||
const lensesReturnedWhenRunnerStarted = [];
|
||||
const agent = async (prompt, opts) => {
|
||||
const label = (opts && opts.label) || '';
|
||||
calls.push({ label, prompt, opts });
|
||||
if (label.startsWith('impl:') || label.startsWith('fix:#')) return { done: true, summary: '', verified: '', left: '', commits: IMPL_COMMITS, head_sha: 'aaaaaaa' };
|
||||
if (label.startsWith('review:codex:')) {
|
||||
lensesReturnedWhenRunnerStarted.push(lensesReturned);
|
||||
return { verdict: 'merge', findings: [], ran: false };
|
||||
}
|
||||
if (label.startsWith('review:fallback:')) {
|
||||
lensesReturnedWhenFallbackStarted.push(lensesReturned);
|
||||
if (fallbackNull) return null;
|
||||
const spec = reviewRounds[round - 1] || { findings: [] };
|
||||
return { verdict: 'merge', findings: spec.findings };
|
||||
}
|
||||
@@ -74,12 +77,13 @@ function run(script, args, { reviewRounds, landOverrides = {}, postRebase = null
|
||||
};
|
||||
const parallel = async (thunks) => {
|
||||
round += 1;
|
||||
lensesReturned = 0;
|
||||
if (postRebase !== null && calls.some((c) => c.label.startsWith('land:'))) {
|
||||
return Promise.all(thunks.map(() => Promise.resolve(postRebase === 'lensesNull' ? null : { verdict: 'merge', findings: postRebase })));
|
||||
}
|
||||
return Promise.all(thunks.map((t) => t().catch(() => null)));
|
||||
};
|
||||
return script(args, agent, parallel, () => {}, () => {}).then((result) => ({ result, calls, fixCount, lensesReturnedWhenFallbackStarted }));
|
||||
return script(args, agent, parallel, () => {}, () => {}).then((result) => ({ result, calls, fixCount, lensesReturnedWhenFallbackStarted, lensesReturnedWhenRunnerStarted }));
|
||||
}
|
||||
|
||||
const cases = [
|
||||
@@ -171,13 +175,21 @@ describe.each(cases)('%s review loop', (name, args) => {
|
||||
expect(result.error).toMatch(/^the post-rebase review round produced no reviews/);
|
||||
});
|
||||
|
||||
it('on a rubric-class change the worktree-isolated fallback reviewer starts only after both lenses have returned', async () => {
|
||||
it('a rubric round whose runner and substitute both fail is an error, never a landed PR', async () => {
|
||||
const rubricArgs = { ...args, risk: 'rubric' };
|
||||
const { result, calls, lensesReturnedWhenFallbackStarted } = await run(script, rubricArgs, { reviewRounds: [{ findings: [] }] });
|
||||
const { result, calls } = await run(script, rubricArgs, { reviewRounds: [{ findings: [] }], fallbackNull: true });
|
||||
expect(result.error).toMatch(/^the cross-family runner and its substitute both failed in round 1/);
|
||||
expect(calls.some((c) => c.label.startsWith('land:'))).toBe(false);
|
||||
});
|
||||
|
||||
it('on a rubric-class change the fallback reviewer starts only after both lenses have returned, and the runner beside them', async () => {
|
||||
const rubricArgs = { ...args, risk: 'rubric' };
|
||||
const { result, calls, lensesReturnedWhenFallbackStarted, lensesReturnedWhenRunnerStarted } = await run(script, rubricArgs, { reviewRounds: [{ findings: [] }] });
|
||||
expect(result.error).toBeUndefined();
|
||||
expect(calls.filter((c) => c.label.startsWith('review:codex:')).length).toBe(1);
|
||||
expect(calls.filter((c) => c.label.startsWith('review:fallback:')).length).toBe(1);
|
||||
expect(lensesReturnedWhenFallbackStarted).toEqual([2]);
|
||||
expect(lensesReturnedWhenRunnerStarted).toEqual([0]);
|
||||
expect(finisherPrompt(calls)).toContain('substituted a cold same-family review-only agent');
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user