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

fix(#4255): resolve reviewer-lane effort from the lane, not from gsd-plan-checker (#4275)

Browse Source 
`review-lane plan` resolved every cross-AI reviewer lane's reasoning effort by
spawning `query resolve-execution gsd-plan-checker --host <slug>`. The agent id
was a hardcoded literal, so `--host` chose only the argv RENDERING while the
LEVEL always came from the installed plan-checker's frontmatter — `low` under
every shipped model profile. Every prompt-fed lane therefore ran at a fast
structural verifier's effort, and because the rendered argument is a CLI config
override it silently beat the effort the operator had configured for that CLI.
At `low` a large source-grounded prompt makes a model end its turn with no final
message, so the lane came back empty and its stub read as a crash.

Effort is a property of the review, so the lane declares it. Two new fields on
ReviewerLane — `effortConfigKey` (`review.effort.<slug>`) and `defaultEffort` —
carried through each capability manifest and the generated registry, set on the
three lanes with an argv effort channel and null on the other nine. A new pure
`resolveLaneEffort()` resolves config key -> lane default -> nothing, where
"nothing" emits no effort argument at all and the reviewer CLI's own
configuration decides; `inherit` selects that path explicitly and an
unrecognized level falls back to the lane default rather than being forwarded to
a CLI that would reject it. The host's negotiated effortSurface still gates the
rendering, so ADR-1239/#2481's trust boundary holds on this path too. Resolving
in-process also removes up to twelve subprocess spawns per review.

The empty-output stub now names the effort the lane ran at and distinguishes a
clean exit from a timeout kill, a non-zero exit, and a process that never ran —
`status` is null for both a timeout and a signal, so those were indistinguishable
before. The hint is hedged: a clean empty exit is most often a model stopping
short, but it is also consistent with a CLI writing its output elsewhere.

Also: the capability validator now knows both fields, rejects a malformed key or
an out-of-vocabulary default, and rejects a default declared without a config
key (a level the operator could never override). An existing end-to-end row in
tests/effort-surface-axis.test.cjs asserted the old coupling; it now configures
the lane's own key and pins the decoupling in the same real spawn, with the
agent execution tier set to a level that must not appear.

Emitted-Drift-Ack-Growth: review.md — the effort/model resolution-order table this fix adds. The workflow is where an operator looks to find out which knob set a lane's model and effort; leaving the new key undocumented there is the same invisibility that made the plan-checker coupling survive this long.

Emitted-Drift-Ack-Growth: review.md — the effort/model resolution-order table this fix adds. The workflow is where an operator looks to find out which knob set a lane's model and effort, so leaving the new key undocumented there is the same invisibility that let the plan-checker coupling survive.

Claude-Session: https://claude.ai/code/session_01CRMEuzNMWn3gs5uUW2ghcF

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Behruz Nassre Esfahani
2026-09-05 02:25:44 -07:00
committed by GitHub
parent 925a363879
commit 5ad9a36f35
26 changed files with 759 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
tests/effort-surface-axis.test.cjs
View File

@@ -580,14 +580,20 @@ describe('#2481 — ADR-443 mechanism callers, as they actually exist', () => {
describe('#2481 review workflow resolves effort per reviewer', () => {
test('shipped orchestration: the live claude lane genuinely receives --effort <level> in its spawned argv', () => {
// Phase 5b (#2799) moved the call out of review.md's per-lane bash and into the review-lane
// route's `effortFor()`, which SPAWNS `query resolve-execution … --pick effort_argv_string`
// once per selected lane and folds the result into that lane's argv template. A text grep for
// the string "resolve-execution" in gsd-tools.cjs would pass even if effortFor's result were
// silently dropped before reaching the spawned reviewer, or if the call were dead code. This
// drives the REAL `review-lane invoke` route end-to-end — real cp.spawnSync, a real project
// config, a real claude-shaped shim on PATH — and inspects the argv the shim actually received,
// which is the only way to prove the resolved effort reaches the invocation rather than merely
// that some file mentions the command name.
// route's `effortFor()`, which folds a resolved level into that lane's argv template. A text
// grep in gsd-tools.cjs would pass even if the result were silently dropped before reaching
// the spawned reviewer, or if the call were dead code. This drives the REAL `review-lane
// invoke` route end-to-end — real cp.spawnSync, a real project config, a real claude-shaped
// shim on PATH — and inspects the argv the shim actually received, which is the only way to
// prove the resolved effort reaches the invocation rather than merely that some file mentions
// the command name.
//
// #4255 changed WHERE the level comes from, not whether it must arrive. It used to be read
// from the `gsd-plan-checker` AGENT's execution settings via a hardcoded id, so this row
// configured `effort.routing_tier_defaults` and expected that value in the reviewer's argv.
// A reviewer lane is not that agent, and the coupling is the bug. The row now configures the
// lane's OWN `review.effort.claude` and pins the decoupling in the same spawn: the execution
// routing tier is set to a DIFFERENT level, and leaking it into the reviewer is a failure.
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2481-orchestration-e2e-'));
const projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2481-orchestration-project-'));
try {
@@ -637,9 +643,14 @@ describe('#2481 review workflow resolves effort per reviewer', () => {
fs.mkdirSync(path.join(projectDir, '.planning'), { recursive: true });
fs.writeFileSync(
path.join(projectDir, '.planning', 'config.json'),
// #3531: pin every tier so the expected value is agent-independent —
// the reviewer lane's tier decides, not effort.default.
JSON.stringify({ effort: { routing_tier_defaults: { light: 'xhigh', standard: 'xhigh', heavy: 'xhigh' } } }, null, 2),
// Two levels, deliberately different (#4255). `review.effort.claude` is the reviewer
// lane's own key and is what must reach the shim. `effort.routing_tier_defaults` drives
// the AGENT execution axis and is pinned across every tier to a level that must NOT
// appear — before #4255 it, through gsd-plan-checker, was the only thing that could.
JSON.stringify({
review: { effort: { claude: 'xhigh' } },
effort: { routing_tier_defaults: { light: 'minimal', standard: 'minimal', heavy: 'minimal' } },
}, null, 2),
);
const r = cp.spawnSync(
@@ -663,7 +674,15 @@ describe('#2481 review workflow resolves effort per reviewer', () => {
const argv = fs.readFileSync(seenArgv, 'utf8').trim().split(/\r?\n/);
assert.ok(
argv.includes('--effort') && argv.includes('xhigh'),
`resolved effort ("xhigh") did not reach the spawned claude reviewer's argv: ${JSON.stringify(argv)}`,
`the lane's own review effort ("xhigh") did not reach the spawned claude reviewer's argv: ${JSON.stringify(argv)}`,
);
// The decoupling half (#4255), and the reason this row is worth a real spawn: the agent
// execution tier is pinned to `minimal` above. Seeing it here would mean a reviewer lane is
// still taking its effort from an agent's execution settings.
assert.ok(
!argv.includes('minimal'),
'the AGENT execution routing tier leaked into the reviewer lane\'s argv: '
+ `${JSON.stringify(argv)} — a lane's effort must come from its own review key`,
);
} finally {
cleanup(dir);

46
tests/review-lane-descriptor.test.cjs
View File

@@ -1045,3 +1045,49 @@ describe('review-lane CLI overlay invocation (#2927, rows 9–10)', () => {
});
});
}
describe('#4255 — every lane declares where its review effort comes from', () => {
// The two fields exist so a lane's effort is DATA about the lane, resolvable without asking any
// agent for its execution settings. These rows pin the declaration itself: a new lane that adds
// an argv effort channel without saying what effort to run at would otherwise silently inherit
// whatever the resolver's fallback happens to be — the exact shape of the original bug.
test('a lane with an argv effort channel declares BOTH a config key and a default', () => {
for (const lane of REVIEWER_LANES) {
if (lane.invoke.effortChannel !== 'argv') continue;
assert.equal(typeof lane.effortConfigKey, 'string',
`${lane.slug} renders an effort argument but declares no effortConfigKey`);
assert.equal(lane.effortConfigKey, `review.effort.${lane.slug}`,
`${lane.slug}: the key is read from config by this exact name`);
assert.equal(typeof lane.defaultEffort, 'string',
`${lane.slug} renders an effort argument but declares no review default`);
}
});
test('a lane with no effort channel declares neither', () => {
for (const lane of REVIEWER_LANES) {
if (lane.invoke.effortChannel === 'argv') continue;
assert.equal(lane.effortConfigKey, null, `${lane.slug} has no channel to feed`);
assert.equal(lane.defaultEffort, null, `${lane.slug} has no channel to feed`);
}
});
test('the prompt-fed, source-grounded lanes default no lower than high', () => {
// These lanes read the repository against a plan set. Effort is load-bearing exactly here:
// it is where a large prompt at a low level ends the turn with no final message.
const RANK = ['minimal', 'low', 'medium', 'high', 'xhigh', 'max'];
for (const lane of REVIEWER_LANES) {
if (lane.defaultEffort === null) continue;
assert.equal(lane.evidenceClass, 'source-grounded', `${lane.slug}: unexpected class for a default`);
assert.ok(RANK.indexOf(lane.defaultEffort) >= RANK.indexOf('high'),
`${lane.slug} defaults to ${lane.defaultEffort}, below the review floor`);
}
});
test('no declared default is the `inherit` sentinel — that is what a null key means', () => {
// `inherit` is a legitimate CONFIGURED value (it selects "emit nothing"), but as a declared
// default it would be a second spelling for `null` and split one behaviour across two shapes.
for (const lane of REVIEWER_LANES) {
assert.notEqual(lane.defaultEffort, 'inherit', `${lane.slug}: use null, not 'inherit'`);
}
});
});

140
tests/review-lane-invocation.test.cjs
View File

@@ -26,6 +26,7 @@ const {
} = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
const {
resolveLanePlan,
resolveLaneEffort,
resolveTimeoutMs,
isEmptyReview,
normalizeHost,
@@ -722,3 +723,142 @@ describe('#2358 design principle: run-scoped temp dirs never collide across proj
);
});
});
describe('#4255 — lane effort comes from REVIEW config, never from another agent', () => {
// Before this fix `review-lane plan` resolved every lane's effort by spawning
// `query resolve-execution gsd-plan-checker --host <slug>`. The agent id was a hardcoded
// literal, so `--host` chose only the argv RENDERING while the LEVEL always came from the
// installed plan-checker's frontmatter — `low` under every shipped model profile. A cross-AI
// review is the opposite workload from a fast structural verifier, the rendered argument is a
// CLI config OVERRIDE (so it beat the effort the operator had set for that CLI), and at `low` a
// large source-grounded prompt makes the model end its turn with no final message: the lane
// came back empty and its stub read as a crash.
//
// These rows pin the replacement. `renderArgv` is injected, so they assert the RESOLUTION —
// which level wins, and whether an argument is emitted at all — independently of any host's
// argv syntax. The syntax itself stays pinned by the GOLDEN table above.
/** Stand-in for the host renderer: echoes the level back in a recognisable shape. */
const render = (host, level) => ({ argv: ['-c', `effort=${level}`], value: level });
/** A host that declares no argv effort surface — renders nothing, as ADR-2481 requires. */
const renderNone = () => ({ argv: [], value: null });
const lane = (slug) => REVIEWER_LANES.find((l) => l.slug === slug);
test('the declared default is used when nothing is configured — high on the prompt-fed lanes', () => {
for (const slug of ['codex', 'claude', 'opencode']) {
const r = resolveLaneEffort(lane(slug), () => undefined, render);
assert.equal(r.value, 'high', `${slug} must default to high, not to another agent's level`);
assert.equal(r.source, 'lane-default');
assert.deepEqual(r.argv, ['-c', 'effort=high']);
}
});
test("a lane's own config key wins over the declared default", () => {
const r = resolveLaneEffort(lane('codex'), (k) => (k === 'review.effort.codex' ? 'xhigh' : undefined), render);
assert.equal(r.value, 'xhigh');
assert.equal(r.source, 'config');
});
test('each lane reads ONLY its own key — one lane’s effort never leaks into another', () => {
const configGet = (k) => (k === 'review.effort.codex' ? 'minimal' : undefined);
assert.equal(resolveLaneEffort(lane('codex'), configGet, render).value, 'minimal');
assert.equal(resolveLaneEffort(lane('claude'), configGet, render).value, 'high',
'claude must fall back to its OWN default, not pick up the codex key');
});
test('a configured `inherit` emits NO argument, so the reviewer CLI’s own config decides', () => {
const r = resolveLaneEffort(lane('codex'), () => 'inherit', render);
assert.deepEqual(r.argv, []);
assert.equal(r.value, null);
assert.equal(r.source, 'none');
});
test('a lane declaring no review effort at all emits no argument', () => {
// The non-vacuity half of the row above: `none` must be reachable from the DECLARATION too,
// not only from an explicit `inherit`. Nine shipped lanes have no effort channel to feed.
for (const l of REVIEWER_LANES.filter((x) => x.effortConfigKey === null)) {
const r = resolveLaneEffort(l, () => 'high', render);
assert.deepEqual(r.argv, [], `${l.slug} declares no effort key and must emit nothing`);
assert.equal(r.source, 'none');
}
});
test('an unrecognized level is REFUSED, not forwarded to the CLI', () => {
// Forwarding a typo renders an argument the CLI rejects, which kills the lane outright — a
// strictly worse outcome than the level the operator meant. Fall back to the declared default.
for (const bogus of ['hihg', 'HIGH ', 'very-high', '', ' ', 'null']) {
const r = resolveLaneEffort(lane('codex'), () => bogus, render);
assert.equal(r.value, 'high', `'${bogus}' must fall back to the lane default`);
assert.equal(r.source, 'lane-default');
}
});
test('a non-string configured value is ignored rather than rendered', () => {
for (const bogus of [42, true, null, {}, ['high']]) {
const r = resolveLaneEffort(lane('codex'), () => bogus, render);
assert.equal(r.value, 'high', `${JSON.stringify(bogus)} must not reach the renderer`);
}
});
test('the host’s negotiated surface still decides — a renderer that emits nothing yields none', () => {
// ADR-1239/#2481's trust-boundary invariant: a lane cannot talk a host into accepting an
// argument its negotiated effortSurface does not declare, however the lane is configured.
const r = resolveLaneEffort(lane('codex'), () => 'xhigh', renderNone);
assert.deepEqual(r.argv, []);
assert.equal(r.source, 'none');
});
test('the documented clamp is what the catalog actually does', () => {
// The configuration reference tells the operator which levels survive per host, and a prose
// claim about behaviour is a claim that can rot. Pin it against the real renderer so the two
// cannot drift. (No file is read here — the lane's clamp table IS the thing being asserted.)
const mc = require('../gsd-core/bin/lib/model-catalog.cjs');
const seen = {};
for (const host of ['codex', 'claude', 'opencode']) {
seen[host] = Object.fromEntries(
['minimal', 'low', 'medium', 'high', 'xhigh', 'max']
.map((l) => [l, mc.renderEffortArgv(host, l, 'argv').value]),
);
}
assert.equal(seen.codex.minimal, 'low', 'codex clamps minimal to low');
assert.equal(seen.claude.minimal, 'low', 'claude clamps minimal to low');
assert.equal(seen.opencode.minimal, 'minimal', 'opencode accepts minimal as-is');
for (const host of ['codex', 'claude', 'opencode']) {
for (const l of ['low', 'medium', 'high', 'xhigh', 'max']) {
assert.equal(seen[host][l], l, `${host} must pass ${l} through unchanged`);
}
}
});
test('the renderer’s clamped value is recorded, not the level asked for', () => {
// #2295 records the effort in REVIEWS.md as `model (reasoning=LEVEL)`. When a host clamps
// (`max` -> `xhigh` on codex), the recorded level must be what actually ran.
const clamping = () => ({ argv: ['-c', 'effort=xhigh'], value: 'xhigh' });
const r = resolveLaneEffort(lane('codex'), () => 'max', clamping);
assert.equal(r.value, 'xhigh', 'the clamped level is what ran, so it is what is recorded');
});
test('a malformed lane object degrades to no effort instead of throwing', () => {
// The resolver is called from gsd-tools.cjs (untyped) with a lane looked up by slug, which
// can miss. Losing one lane's effort is recoverable; a throw there takes down every lane.
for (const bad of [null, undefined, {}, { slug: 'x' }]) {
assert.deepEqual(resolveLaneEffort(bad, () => 'high', render).argv, []);
}
});
test('resolved effort reaches argv only for lanes declaring effortChannel argv', () => {
for (const l of REVIEWER_LANES) {
const eff = resolveLaneEffort(l, () => undefined, render);
const r = resolveLanePlan({
lane: l, configGet: () => undefined, runDir: RUN, repoRoot: ROOT,
effortArgs: eff.argv, effortValue: eff.value,
});
if (!r.ok || r.plan.transport !== 'spawn') continue;
const carriesEffort = r.plan.argv.some((a) => String(a).startsWith('effort='));
assert.equal(carriesEffort, l.invoke.effortChannel === 'argv',
`${l.slug}: effort argv must appear exactly when the lane declares the channel`);
assert.equal(r.plan.effort, l.invoke.effortChannel === 'argv' ? 'high' : null,
`${l.slug}: the recorded effort must match what actually expanded`);
}
});
});

104
tests/review-lane-runner.test.cjs
View File

@@ -14,7 +14,7 @@ const path = require('node:path');
const fc = require('fast-check');
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
const { resolveLanePlan, LANE_UNAVAILABLE } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
const { resolveLanePlan, resolveLaneEffort, LANE_UNAVAILABLE } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
const {
checkEgressHost,
probeLane,
@@ -233,6 +233,108 @@ describe('runner — empty-output policy (#2494 / #2605 / #2794)', () => {
}
});
/**
* A plan built through the SAME two steps the CLI runs (#4255): resolve the lane's review
* effort, then expand it into argv. `plan()` above passes no effort at all, which is the right
* default for rows that do not care — but a stub row asserting the effort must not invent it.
*/
function planWithEffort(slug) {
const lane = REVIEWER_LANES.find((l) => l.slug === slug);
const eff = resolveLaneEffort(lane, () => undefined, (host, level) => ({
argv: ['-c', `model_reasoning_effort=${level}`], value: level,
}));
const r = resolveLanePlan({
lane, configGet: () => undefined, runDir: RUN, repoRoot: ROOT,
effortArgs: eff.argv, effortValue: eff.value,
});
assert.equal(r.ok, true, `${slug} failed to resolve`);
return r.plan;
}
test('#4255 — the stub names the effort and says the exit was clean', () => {
// A crash, a timeout kill and a model that ended its turn without writing a final message all
// reach the stub as the same zero bytes. The third is what a too-low reasoning effort produces
// on a large source-grounded prompt, and it was indistinguishable from the first two — the
// operator read "Codex failed" and went looking for a broken CLI. The stub now names the level
// it ran at (the value they would change) and how the process actually ended.
const p = planWithEffort('codex');
const d = deps();
writeReviewOrStub(p, '', d, undefined, { status: 0 });
const out = d.files[p.reviewPath];
assert.ok(out.includes('failed or returned empty output'),
'the header every downstream reader greps for must be untouched');
assert.ok(/ran at effort=high/.test(out), 'the stub must name the effort the lane ran at');
assert.ok(/exited cleanly inside the timeout/.test(out));
assert.ok(/ending its turn without writing a final message/.test(out),
'a clean exit with no output must be named as the likeliest cause, not left reading as a crash');
assert.ok(/most often/.test(out),
'the cause is hedged on purpose: a clean empty exit is consistent with a stopped-short model '
+ 'AND with a CLI writing its output somewhere this lane did not read');
});
test('#4255 — a timeout kill and a crash are NOT reported as stopping short', () => {
// The non-vacuity half: the stopped-short hint must be earned by a clean exit, or it is
// advice that sends the operator to raise effort on a lane that was killed or crashed.
const timedOut = deps();
writeReviewOrStub(planWithEffort('codex'), '', timedOut, undefined, { status: null, errorCode: 'ETIMEDOUT' });
const t = timedOut.files[planWithEffort('codex').reviewPath];
assert.ok(/killed by the outer timeout/.test(t));
assert.ok(!/most often/.test(t), 'a timeout is not a model stopping short');
const crashed = deps();
writeReviewOrStub(planWithEffort('codex'), '', crashed, undefined, { status: 127 });
const c = crashed.files[planWithEffort('codex').reviewPath];
assert.ok(/exited with status 127/.test(c));
assert.ok(!/most often/.test(c), 'a non-zero exit is not a model stopping short');
// `status` is null for a process that never started or died on a signal, exactly as it is for
// a timeout kill. Reporting "status null" named nothing the operator could act on, and the
// two must not collapse into one another (Codex review of #4255).
for (const [label, out, expect] of [
['binary missing', { status: null, errorCode: 'ENOENT' }, /did not exit normally \(ENOENT\)/],
['killed by a signal', { status: null }, /did not exit normally \(killed by a signal\)/],
]) {
const d = deps();
writeReviewOrStub(planWithEffort('codex'), '', d, undefined, out);
const text = d.files[planWithEffort('codex').reviewPath];
assert.match(text, expect, `${label} must be named, not reported as "status null"`);
assert.ok(!/status null/.test(text), `${label}: "status null" tells the operator nothing`);
assert.ok(!/most often/.test(text), `${label} is not a model stopping short`);
}
});
test('#4255 — an HTTP lane is not described as having a reviewer CLI', () => {
// ollama is reached directly over HTTP: there is no CLI, so "the CLI's own configuration
// applied" would be a lie about what ran (Codex review of #4255).
const p = plan('ollama');
const d = deps();
writeReviewOrStub(p, '', d, undefined, { status: 0 });
const out = d.files[p.reviewPath];
assert.ok(/HTTP lane/.test(out));
assert.ok(!/reviewer CLI/.test(out), 'an HTTP lane has no reviewer CLI to attribute anything to');
assert.ok(!/effort=/.test(out), 'no level may be claimed for a transport that carries none');
});
test('#4255 — a lane that sent no effort argument says so, rather than naming a level', () => {
// gemini declares no effort channel, so `plan.effort` is null. Printing a level there would
// be a lie about what reached the CLI; the stub says the CLI's own configuration applied.
const p = plan('gemini');
const d = deps();
writeReviewOrStub(p, '', d, undefined, { status: 0 });
const out = d.files[p.reviewPath];
assert.ok(/no effort argument, so the reviewer CLI's own configuration applied/.test(out));
assert.ok(!/effort=/.test(out), 'no level may be claimed for a lane that sent none');
});
test('#4255 — the diagnosis is added even when the outcome is unknown', () => {
// The HTTP path has no spawn outcome to pass. The effort half still applies, so the line is
// still written — just without the exit clause it cannot honestly make.
const p = planWithEffort('codex');
const d = deps();
writeReviewOrStub(p, '', d);
assert.ok(/ran at effort=high/.test(d.files[p.reviewPath]));
});
test('the stub is distinguishable from a real review', () => {
// The ambiguity between "failed" and "ran cleanly with nothing to report" IS the defect.
const p = plan('gemini');