* chore(#2799): resolve reviewer lanes into executable invocation plans Phase 5b of ADR-2782. Adds the resolver and runner that let invoke_reviewers iterate declared lanes instead of hand-authored per-CLI bash. Five additive descriptor amendments, each forced by a lane that ships today: - LaneHandler gains 'opencode' — the lane rebuilds its review from assistant text parts of a --format json stream; a plain stdout copy re-breaks #1936. - modelConfigKey — antigravity's key is review.models.agy, not .antigravity, so resolving by slug silently dropped a configured model. - defaultHost/fallbackModel — Phase 4 federated every *_host with a default of empty string; the real fallback only existed in the bash. - args becomes an argv template with a closed four-placeholder vocabulary. Positional splicing produced 'codex --model M -o F exec --ephemeral', which is not a valid invocation: codex injects in the middle, twice. - kimi-code lane, with the bounded command-capability probe (needle --output-format) that tells Kimi Code from the legacy python kimi-cli. Parity gate re-pointed: the workflow-text families it scanned are the text this phase deletes, so they are replaced by descriptor-to-registry parity plus an anti-parity check that no bespoke leg returns. jq, curl and external timeout/gtimeout all drop out of the review path. Refs #2782 * chore(#2799): add review-lane query surface and widen the manifest vocabulary Adds the gsd-tools 'review-lane' route (plan/invoke/sections) the workflow loops over, projects all twelve lanes into their capability manifests, and widens capability-validator for the amendments. opencode admitted to VALID_LANE_HANDLERS under the second arm of the enum's own admission rule: one lane, justified by a documented upstream defect data cannot express (#1936 — the agent can end its turn with zero output tokens and --format default then drops the assistant text entirely). Two bugs caught by an end-to-end stub run and fixed here: - loadConfigResolved returns a provenance wrapper, not the config; using it directly resolved every key to undefined, which reads as 'nothing configured' and silently dropped every model override. - hasBinary used shell:true with an args array (Node 26 DEP0190). Replaced with a PATH scan that spawns nothing at all. Refs #2782 * chore(#2799): iterate declared lanes in invoke_reviewers and write_reviews Replaces the eleven hand-authored per-CLI bash legs with a loop over resolved lanes, and renders REVIEWS.md sections from each lane's declared reviewsSection instead of thirteen hardcoded headings. review.md drops from 1104 lines to 507 (61KB to 28.7KB). Parity gate re-pointed, as agreed: the leg-marker and section-heading families scanned exactly the text this phase deletes, so they are replaced by descriptor-to-registry parity in both directions, plus an anti-parity check that fires if a bespoke leg is ever re-added. Enum, emitting sites and the Object.keys lock moved together. The budget-trim helper is hoisted out of the Ollama leg: it was always lane-agnostic, and any lane may now declare a promptBudgetKey. Refs #2782 * feat(#2799): bind the consented egress host and re-verify it at invocation Completes ADR-2782 D5. Rule 1 was recorded in the ADR as delivered by Phase 3 but was not implemented: ConsentRecord had no host field and nothing in the tree bound one, so this phase's rule-4 comparison had no baseline. ConsentRecord gains an OPTIONAL reviewerHost. Optional is the whole design: isValidConsentRecord does not require it, so every record already on disk stays valid and no re-consent storm fires (D4 rule 5). It is deliberately excluded from disclosureSignature — the loader has no config resolver, so folding a config-derived value in would make loader and lifecycle compute different signatures for the same manifest and re-prompt forever. Install resolves hostConfigKey (falling back to the lane's declared defaultHost, which is what the invocation path uses) and records it. Invocation re-resolves and blocks on mismatch rather than silently redirecting. Absence allows: no record, or a record predating the field, means nothing to compare — denying there would break every existing local-model user on upgrade. Refs #2782 * test(#2799): cover the resolver, runner and handlers; retarget the parity suites Adds the golden invocation-plan table (one row per shipped lane, derived from the bash legs rather than the descriptor types) plus runner coverage for the probe, empty-output policy, the three handlers and the egress check. Retargets the existing suites onto the new contract: descriptor-to-registry parity, the anti-parity check, the opencode handler, and the twelfth lane. Two corrections found by running them: - modelConfigKey was required; that breaks D4 rule 2, since a reviewer manifest authored before this phase would fail validation on upgrade. It is optional, read as null when absent. - the antigravity non-zero-exit test pre-seeded the transcript, which asserted that a STALE entry leaks through — the exact bug the watermark prevents. The spawn now appends, as the real tool does. Refs #2782 * fix(#2799): restore agy --add-dir and the self-report prompt in the handler Retargeting the three legacy reviewer suites off the deleted bash surfaced two real regressions in the port, both #2176: - --add-dir was dropped. Without it agy's permission context never receives the cwd repo, so the agent anchors on its own scratch dir and reviews the plan text in isolation — the exact failure the Review Instructions forbid. It is capability-probed, because an older agy rejects the unknown flag outright and a lane that fails to start is worse than one running on the prompt anchor. - the prompt lost the clause mandating a REVIEWED-WITHOUT-REPO-ACCESS self-report, which is what makes a blind review distinguishable from a grounded one. antigravity now builds its own prompt variant. Also ports the #2073 mode-2 cli.log diagnostic, which was dropped: a pinned model that 404s exits 0 with empty stdout AND an empty transcript, so agy's own log is the only evidence that anything failed. The three suites now assert against the plan and the handler instead of matching fence text, so they no longer need allow-test-rule exemptions. Refs #2782 * docs(#2799): document the declared lanes, the new flag, and dropped prerequisites COMMANDS.md gains --kimi-code and replaces the jq-prerequisite paragraph, which is now false: no lane requires jq, curl or an external timeout. Adds the changed-egress-destination behavior, since a blocked lane is something a user can hit. CONFIGURATION.md records that the model config key is declared per lane rather than derived from the flag — antigravity's is review.models.agy — and adds review.models.kimi-code. reviewer-instances.md now routes an instance through its lane's single invocation seam instead of a copied per-adapter bash block, which is what lets a cross-cutting fix reach instances for free. That required implementing the --model/--agent/--as flags it documents; --model re-resolves through the lane's argv template rather than splicing, so the flag lands where the lane declares it rather than ahead of a subcommand. CONTEXT.md glossary gains both new modules. Refs #2782 * chore(#2799): drop the stale emitted-drift acknowledgment The only entry was #2797's, acknowledging COMMENT-ONLY GROWTH in review.md. That file now shrinks by ~32KB and every emitted hash that moved is attributable to this diff, so the ack no longer explains anything. Removing the last entry means removing the file: its presence is the alarm, and an empty one signals nothing. Verified by deleting it and re-running the attribution and provenance gates plus lint:ci — all green without it. Refs #2782 * docs(#2799): record the Phase 5b vocabulary widenings in ADR-2782 Five additive amendments, each forced by a lane that ships today, plus two corrections the phase had to make rather than work around: - D5 rule 1 was recorded as delivered by Phase 3 and was not implemented, so this phase's rule-4 comparison had no baseline. Recorded because an ADR asserting a rule was delivered is exactly what stops a later phase checking. - The DEFECT.GENERATIVE-FIX gate is re-pointed: its workflow-text families scanned the text this phase deletes. Also records that D7's 'skip the probe where no bounding mechanism exists' carve-out is obsolete — in practice it meant the Antigravity lane ran unbounded on every stock macOS host, which ships neither timeout nor gtimeout. Refs #2782 * fix(#2799): close four defects found by adversarial review Two confirmed bugs, both reproduced before fixing: - resolveLanePlan was not total. An openai-http lane with a missing or non-object invoke dereferenced inv.hostConfigKey and threw, contradicting the module's own documented contract; the spawn branch guarded correctly and the http branch did not. The CLI seam resolves every selected lane in one map, so one malformed overlay manifest would have aborted the whole review rather than dropping its own lane. Guarded, plus a per-lane try/catch at the seam so a throw can never take down siblings. - A reviewer-instance model was silently dropped for any lane declaring modelConfigKey null (cursor, qwen, coderabbit). reviewer_instances validates that cli is a known slug but never that the slug accepts a model, so a user could configure one, get a clean run, and never learn a different model reviewed their plan. Now warns explicitly. Two hardening fixes: - The slug is concatenated into artifact paths, so LANE_SLUG_RE is enforced in the resolver rather than inherited from a validator that does not run on this path — the module documents itself as the overlay-manifest trust boundary, so it should not depend on someone else having checked. - normalizeHost mangled a scheme-less value: new URL('localhost:11434') parses with an empty hostname, so it became 'localhost://11434' and was compared and requested as if real. An empty hostname now means not-a-URL. Also documents the one gap that cannot be closed here: the antigravity watermark is keyed by workspace, so two concurrent reviews of the same repo share a transcript. agy exposes no per-invocation id to filter on, so the handler now states which half of its never-stale guarantee actually holds. Refs #2782 * test(#2799): retarget the remaining eight review.md-asserting suites The remote runner found 37 failures the local sweep missed (it hit the shell's two-minute cap before reaching these). All eight extract per-CLI bash from review.md that this phase deletes; each protects a real invariant, so each is retargeted onto the plan, the runner or the handler rather than removed. Three real defects surfaced by doing so: - effort args never reached ANY lane. model-resolver.cjs exports no resolveExecution, so effortFor silently returned [] every time. Restored by calling the same bounded resolve-execution query the bash legs used — and NOT with --raw, which prints the resolved effort rather than the picked field, so claude got 'low' instead of '--effort low'. - the timeout guidance lost 'a silent empty output is a timeout kill, not a crash' — the operator note that exists because of the Codex 0xc0000142 misdiagnosis. Restored. - the opencode handler dropped EMPTY assistant text parts. The shipped jq was , and only substitutes for false/null — an empty string is truthy in jq and contributed a blank line. Found by a property test shrinking to ['', '']. The opencode property suite no longer spawns jq at all, which deletes the #2099 hang mechanism it was architected around rather than mitigating it. Refs #2782 * fix(#2799): register the two new generated modules, and untrack them The remote runner caught build output committed to git. Both new modules compile from src/*.cts into gsd-core/bin/lib/*.cjs, and every sibling generated that way is gitignored and eslint-ignored (ADR-457) - including Phase 1's own review-lane-descriptor.cjs. Mine were neither, so repo-invariants' "each bin/lib/*.cjs is linted xor ignored according to migration state" failed. Registered both in .gitignore and eslint.config.mjs alongside the Phase 1 module, and dropped them from the index. Nothing about the shipped behaviour changes; the artifacts are rebuilt by build:lib. This is the new-.cts-module registration ripple, and it is the one part of it I had not completed - the CONTEXT.md glossary and the inventory manifest were already done. Refs #2782 * chore(#2799): backfill changeset pr number to 2861 * chore(#2799): backfill changeset pr number to 2861 --------- Co-authored-by: Test <test@example.com>
This commit is contained in:
Tom Boucher
@@ -1,192 +1,140 @@
|
||||
// allow-test-rule: source-text-is-the-product (see #2073)
|
||||
// gsd-core/workflows/review.md is a workflow document whose bash blocks ARE
|
||||
// what /gsd-review loads and executes at runtime. Asserting the invocation
|
||||
// shape asserts the deployed contract — this is behavioral coverage of the
|
||||
// workflow, not a source-grep over application code.
|
||||
|
||||
/**
|
||||
* Antigravity reviewer repo-grounding tests (#2176)
|
||||
* Antigravity reviewer repo-grounding (#2176).
|
||||
*
|
||||
* The agy block invoked the CLI without granting it the repo under review:
|
||||
* no --add-dir on either invocation arm, and no absolute repo-root anchor in
|
||||
* _AGY_PROMPT. The agent frequently anchored on its own
|
||||
* ~/.gemini/antigravity-cli/scratch dir, reviewed the plan text in isolation
|
||||
* (the exact failure the block's Review Instructions forbid), and its
|
||||
* ungrounded verdict flowed into the Consensus Summary at full weight,
|
||||
* undetected.
|
||||
* The agy leg once invoked the CLI without granting it the repo under review — no `--add-dir` and
|
||||
* no absolute repo-root anchor in the prompt — so the agent anchored on its own
|
||||
* `~/.gemini/antigravity-cli/scratch` dir and reviewed the plan text in isolation, which is exactly
|
||||
* what the Review Instructions forbid.
|
||||
*
|
||||
* These tests pin the fix: capability-probed --add-dir (mirrors the Codex
|
||||
* bypass-flag probe), absolute-root prompt anchor (agy AND the cursor-agent
|
||||
* block, which shared the anchor gap), a mandated self-report line, a stamped
|
||||
* blind-review marker, and consensus down-weighting of marked reviews.
|
||||
* Each assertion fails against the pre-fix block.
|
||||
* Phase 5b (#2799) deleted that bash. Both fixes now live in the named `antigravity` handler, which
|
||||
* is where they belong: "add this flag only when the binary's --help mentions it" is a conditional,
|
||||
* and conditionals escape to a handler rather than accreting inside the descriptor (ADR-2782 D6).
|
||||
* These tests moved with them, so this file no longer reads any source text and the source-text
|
||||
* exemption it used to carry is gone.
|
||||
*/
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const REVIEW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'review.md');
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const {
|
||||
antigravityArgv,
|
||||
antigravityPrompt,
|
||||
stampBlindReview,
|
||||
} = require('../gsd-core/bin/lib/review-lane-runner.cjs');
|
||||
|
||||
function reviewContent() {
|
||||
return fs.readFileSync(REVIEW_PATH, 'utf-8');
|
||||
const RUN = '/run';
|
||||
const ROOT = '/abs/repo';
|
||||
|
||||
function planFor(slug) {
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === slug);
|
||||
const r = resolveLanePlan({ lane, configGet: () => undefined, runDir: RUN, repoRoot: ROOT });
|
||||
assert.equal(r.ok, true);
|
||||
return r.plan;
|
||||
}
|
||||
|
||||
function agyBashBlock() {
|
||||
const fences = reviewContent().match(/```bash[\s\S]*?```/g) || [];
|
||||
const agy = fences.find((f) => /\bagy\b/.test(f) && /gsd-review-antigravity/.test(f));
|
||||
assert.ok(agy, 'review.md should contain the agy invocation bash block');
|
||||
return agy;
|
||||
}
|
||||
/** A spawn stub whose `--help` either advertises `--add-dir` or does not. */
|
||||
const helpSaying = (text) => ({ spawn: () => ({ status: 0, stdout: text, stderr: '' }) });
|
||||
|
||||
function cursorBashBlock() {
|
||||
const fences = reviewContent().match(/```bash[\s\S]*?```/g) || [];
|
||||
const cursor = fences.find((f) => /cursor-agent -p/.test(f));
|
||||
assert.ok(cursor, 'review.md should contain the cursor-agent invocation bash block');
|
||||
return cursor;
|
||||
}
|
||||
|
||||
describe('Antigravity reviewer repo grounding in /gsd-review (#2176)', () => {
|
||||
test('probes agy for --add-dir support (capability-probe idiom, mirrors the Codex block)', () => {
|
||||
const block = agyBashBlock();
|
||||
assert.ok(
|
||||
/agy --help 2>\/dev\/null \| grep -q -- '--add-dir'/.test(block),
|
||||
'agy block must capability-probe --add-dir via `agy --help | grep -q` so older CLIs still run',
|
||||
);
|
||||
describe('#2176 — agy receives the repo under review', () => {
|
||||
test('--add-dir is passed with the repo root when the binary supports it', () => {
|
||||
const p = planFor('antigravity');
|
||||
const argv = antigravityArgv(p.argv, p.promptPath, ROOT, helpSaying('--add-dir --model'));
|
||||
const i = argv.indexOf('--add-dir');
|
||||
assert.ok(i !== -1, '--add-dir must be passed when supported');
|
||||
assert.equal(argv[i + 1], ROOT);
|
||||
});
|
||||
|
||||
test('passes --add-dir with the repo root when supported', () => {
|
||||
const block = agyBashBlock();
|
||||
assert.ok(
|
||||
/set -- "\$@" --add-dir "\$_AGY_WS"/.test(block),
|
||||
'the probed arm must append --add-dir "$_AGY_WS" so both invocation arms (which expand "$@") receive it',
|
||||
);
|
||||
test('--add-dir is CAPABILITY-PROBED, not assumed', () => {
|
||||
// An older agy rejects an unknown flag outright. A lane that fails to start is worse than one
|
||||
// running on the prompt anchor alone, so support is probed rather than presumed.
|
||||
const p = planFor('antigravity');
|
||||
const argv = antigravityArgv(p.argv, p.promptPath, ROOT, helpSaying('no such flag here'));
|
||||
assert.ok(!argv.includes('--add-dir'));
|
||||
});
|
||||
|
||||
test('_AGY_PROMPT is anchored to the absolute repo root', () => {
|
||||
const block = agyBashBlock();
|
||||
const promptLine = block.split('\n').find((l) => l.startsWith('_AGY_PROMPT='));
|
||||
assert.ok(promptLine, 'agy block must define _AGY_PROMPT');
|
||||
assert.ok(
|
||||
/\$_AGY_WS/.test(promptLine),
|
||||
'_AGY_PROMPT must embed the absolute repo root ($_AGY_WS) so repo-relative references resolve on the no---add-dir fallback',
|
||||
);
|
||||
test('the probe is bounded', () => {
|
||||
const p = planFor('antigravity');
|
||||
const calls = [];
|
||||
antigravityArgv(p.argv, p.promptPath, ROOT, {
|
||||
spawn: (b, a, o) => { calls.push(o); return { status: 0, stdout: '', stderr: '' }; },
|
||||
});
|
||||
assert.ok(calls.every((c) => c.timeoutMs > 0), 'every process-starting probe must be bounded');
|
||||
});
|
||||
|
||||
test('_AGY_PROMPT mandates a REVIEWED-WITHOUT-REPO-ACCESS self-report', () => {
|
||||
const block = agyBashBlock();
|
||||
const promptLine = block.split('\n').find((l) => l.startsWith('_AGY_PROMPT='));
|
||||
assert.ok(
|
||||
/REVIEWED-WITHOUT-REPO-ACCESS/.test(promptLine),
|
||||
'_AGY_PROMPT must require the exact self-report line when the reviewer cannot read the repo',
|
||||
);
|
||||
test('adding --add-dir keeps the prompt last', () => {
|
||||
const p = planFor('antigravity');
|
||||
const argv = antigravityArgv(p.argv, p.promptPath, ROOT, helpSaying('--add-dir'));
|
||||
assert.equal(argv[argv.length - 2], '-p');
|
||||
assert.ok(argv[argv.length - 1].includes(RUN));
|
||||
});
|
||||
|
||||
test('stamps a blind-review marker on self-reported or scratch-anchored output', () => {
|
||||
const block = agyBashBlock();
|
||||
assert.ok(
|
||||
/head -5 [^|]*\| grep -q 'REVIEWED-WITHOUT-REPO-ACCESS'/.test(block),
|
||||
'the self-report tell must be anchored to the head of the output, not a whole-body substring',
|
||||
);
|
||||
assert.ok(
|
||||
/grep -qiE '\(workspace\|working\) \(directory\|dir\)\.\{0,40\}antigravity-cli\/scratch'/.test(block),
|
||||
'the scratch tell must be anchored to a workspace-declaration phrasing whose bridge (.{0,40}) can span the dotted ~/.gemini/ path prefix',
|
||||
);
|
||||
assert.ok(
|
||||
/\[reviewed-without-repo-access\]/.test(block),
|
||||
'detected blind reviews must be stamped with the [reviewed-without-repo-access] marker',
|
||||
);
|
||||
});
|
||||
|
||||
test('marker stamping avoids sed -i (BSD/GNU divergence) — uses temp file + mv', () => {
|
||||
const block = agyBashBlock();
|
||||
assert.ok(!/sed -i/.test(block), 'agy block must not use sed -i (BSD vs GNU incompatibility)');
|
||||
assert.ok(
|
||||
/\.tmp && \\?\s*\n?\s*mv /.test(block),
|
||||
'marker stamping should rewrite via temp file + mv',
|
||||
);
|
||||
});
|
||||
|
||||
test('Consensus Summary down-weights marked blind reviews', () => {
|
||||
const content = reviewContent();
|
||||
const consensusIdx = content.indexOf('## Consensus Summary');
|
||||
assert.ok(consensusIdx >= 0, 'review.md should contain the Consensus Summary section');
|
||||
const consensus = content.slice(consensusIdx, consensusIdx + 2000);
|
||||
assert.ok(
|
||||
/\[reviewed-without-repo-access\]/.test(consensus),
|
||||
'consensus instructions must reference the blind-review marker',
|
||||
);
|
||||
assert.ok(
|
||||
/REVIEWED-WITHOUT-REPO-ACCESS/.test(consensus),
|
||||
'consensus instructions must also honor the raw self-report line',
|
||||
);
|
||||
assert.ok(
|
||||
/not count its verdict at full consensus weight/.test(consensus),
|
||||
'marked reviews must be down-weighted, not counted at full weight',
|
||||
);
|
||||
});
|
||||
|
||||
test('blind-review detection behaves correctly on synthetic transcripts', () => {
|
||||
// Behavioral, not string-on-string: extract the actual detection compound
|
||||
// from the fence, point it at a temp file, and run it through bash for
|
||||
// ungrounded and grounded transcript shapes.
|
||||
const os = require('os');
|
||||
const { execFileSync } = require('node:child_process');
|
||||
const { toPosixPath } = require('../gsd-core/bin/lib/shell-command-projection.cjs');
|
||||
const block = agyBashBlock();
|
||||
const m = block.match(/\{ head -5[\s\S]*?\}; then/);
|
||||
assert.ok(m, 'detection compound not found in the agy block');
|
||||
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'agy-detect-'));
|
||||
const out = path.join(tmp, 'review-out.md');
|
||||
const detect = m[0]
|
||||
.replace(/\}; then$/, '}')
|
||||
// Convert the native path to POSIX form so it survives bash on Windows
|
||||
// runners (Git Bash accepts D:/... but eats backslashes).
|
||||
.replaceAll('{run_dir}/gsd-review-antigravity.md', toPosixPath(out));
|
||||
const runDetect = (content) => {
|
||||
fs.writeFileSync(out, content);
|
||||
try {
|
||||
execFileSync('bash', ['-c', detect], { stdio: 'ignore' });
|
||||
return true; // exit 0 → blind review detected
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
};
|
||||
try {
|
||||
// Ungrounded tells — must be stamped
|
||||
assert.equal(runDetect('REVIEWED-WITHOUT-REPO-ACCESS\n\n## Review\nPlan-only review.\n'), true,
|
||||
'self-report line in the head must be detected');
|
||||
assert.equal(runDetect('## Review\nMy working directory is ~/.gemini/antigravity-cli/scratch.\nPlan-only review.\n'), true,
|
||||
'full dotted scratch path in a workspace declaration must be detected (#2184 re-review Major)');
|
||||
assert.equal(runDetect('Workspace directory: /home/me/.gemini/antigravity-cli/scratch\n'), true,
|
||||
'colon-style workspace declaration must be detected');
|
||||
// Grounded shapes — must NOT be stamped
|
||||
assert.equal(runDetect('## Review\nVerified hooks/gsd-statusline.js against the plan. Solid.\n'), false,
|
||||
'ordinary grounded review must not be stamped');
|
||||
assert.equal(runDetect('## Review\nThe workflow mentions antigravity-cli/scratch as the agy scratch dir; the guard there is correct.\n'), false,
|
||||
'grounded review merely quoting the scratch path must not be stamped');
|
||||
assert.equal(
|
||||
runDetect('## Review\n\nGrounded findings below.\n\n### Details\nLine 20 of the workflow mentions REVIEWED-WITHOUT-REPO-ACCESS as the self-report marker; fine.\n'),
|
||||
false,
|
||||
'self-report string quoted beyond the first 5 lines must not be stamped');
|
||||
} finally {
|
||||
require('./helpers.cjs').cleanup(tmp);
|
||||
}
|
||||
});
|
||||
|
||||
test('cursor-agent prompt carries the same absolute-root anchor (identical gap, #2176 AC5)', () => {
|
||||
const block = cursorBashBlock();
|
||||
assert.ok(
|
||||
/_CURSOR_ROOT="\$\(git rev-parse --show-toplevel 2>\/dev\/null \|\| pwd\)"/.test(block),
|
||||
'cursor anchor must resolve the repo TOP-LEVEL (rev-parse, not bare pwd) so subdirectory invocations anchor correctly',
|
||||
);
|
||||
const promptLine = block.split('\n').find((l) => l.startsWith('CURSOR_PROMPT_ARG='));
|
||||
assert.ok(promptLine, 'cursor block must define CURSOR_PROMPT_ARG');
|
||||
assert.ok(
|
||||
/repository under review is at \$_CURSOR_ROOT/.test(promptLine),
|
||||
'CURSOR_PROMPT_ARG must anchor repo-relative references to the absolute repo root',
|
||||
);
|
||||
test('a probe that throws degrades to no flag rather than failing the lane', () => {
|
||||
const p = planFor('antigravity');
|
||||
const argv = antigravityArgv(p.argv, p.promptPath, ROOT, {
|
||||
spawn: () => { throw new Error('spawn failed'); },
|
||||
});
|
||||
assert.ok(!argv.includes('--add-dir'));
|
||||
assert.ok(argv.length > 0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#2176 — the prompt is anchored and demands a self-report', () => {
|
||||
test('the prompt carries the ABSOLUTE repo root', () => {
|
||||
const prompt = antigravityPrompt(`${RUN}/gsd-review-prompt.md`, ROOT);
|
||||
assert.ok(prompt.includes(ROOT));
|
||||
});
|
||||
|
||||
test('the prompt mandates REVIEWED-WITHOUT-REPO-ACCESS when the repo is unreadable', () => {
|
||||
// Without this clause a blind review is indistinguishable from a grounded one and its verdict
|
||||
// is counted at full consensus weight.
|
||||
const prompt = antigravityPrompt(`${RUN}/gsd-review-prompt.md`, ROOT);
|
||||
assert.ok(prompt.includes('REVIEWED-WITHOUT-REPO-ACCESS'));
|
||||
});
|
||||
|
||||
test('the self-report variant actually reaches argv', () => {
|
||||
const p = planFor('antigravity');
|
||||
const argv = antigravityArgv(p.argv, p.promptPath, ROOT, helpSaying(''));
|
||||
assert.ok(argv[argv.length - 1].includes('REVIEWED-WITHOUT-REPO-ACCESS'));
|
||||
});
|
||||
|
||||
test('cursor carries the same absolute-root anchor (#2176 AC5)', () => {
|
||||
// Identical gap, identical fix — cursor-agent also takes the prompt in argv and does not
|
||||
// reliably inherit the review cwd.
|
||||
const p = planFor('cursor');
|
||||
assert.ok(p.argv[p.argv.length - 1].includes(ROOT));
|
||||
});
|
||||
|
||||
test('kimi-code carries it too', () => {
|
||||
const p = planFor('kimi-code');
|
||||
assert.ok(p.argv[p.argv.length - 1].includes(ROOT));
|
||||
});
|
||||
});
|
||||
|
||||
describe('#2176 — blind-review marking', () => {
|
||||
test('a self-reported blind review is stamped', () => {
|
||||
const out = stampBlindReview('REVIEWED-WITHOUT-REPO-ACCESS\nthe review');
|
||||
assert.ok(out.includes('[reviewed-without-repo-access]'));
|
||||
});
|
||||
|
||||
test('a scratch-dir workspace DECLARATION is stamped', () => {
|
||||
const out = stampBlindReview('my working directory is ~/.gemini/antigravity-cli/scratch, so');
|
||||
assert.ok(out.includes('[reviewed-without-repo-access]'));
|
||||
});
|
||||
|
||||
test('a grounded review that merely QUOTES the marker is not stamped', () => {
|
||||
// A review OF this very file must not be mis-stamped and down-weighted.
|
||||
const quoting = ['a', 'b', 'c', 'd', 'e', 'f', 'the tell is REVIEWED-WITHOUT-REPO-ACCESS'].join('\n');
|
||||
assert.ok(!stampBlindReview(quoting).includes('[reviewed-without-repo-access]'));
|
||||
});
|
||||
|
||||
test('an ordinary mention of the scratch path is not a declaration', () => {
|
||||
const mention = 'the plan references .gemini/antigravity-cli/scratch as an example path';
|
||||
assert.ok(!stampBlindReview(mention).includes('[reviewed-without-repo-access]'));
|
||||
});
|
||||
|
||||
test('an empty review is left alone', () => {
|
||||
assert.equal(stampBlindReview(''), '');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,109 +1,203 @@
|
||||
// allow-test-rule: source-text-is-the-product (see #2073)
|
||||
// gsd-core/workflows/review.md is a workflow document whose bash blocks ARE
|
||||
// what /gsd-review loads and executes at runtime. Asserting the Antigravity
|
||||
// invocation shape asserts the deployed contract — this is behavioral coverage
|
||||
// of the workflow, not a source-grep over application code.
|
||||
|
||||
/**
|
||||
* Antigravity (agy) reviewer invocation tests (#2073)
|
||||
* Antigravity (agy) reviewer lane — the #2073 invariants.
|
||||
*
|
||||
* The agy block in /gsd-review had three real-world failure modes on agy 1.0.16:
|
||||
* 1. inline `-p "$(cat <prompt>)"` overflowed the exec arg list on a large prompt
|
||||
* 2. a pinned model that 404'd exited 0 with empty stdout AND an empty transcript
|
||||
* (no --model escape hatch; the generic Step 3 stub gave no diagnostic)
|
||||
* 3. a pre-session stall hung past --print-timeout (which can't fire before a
|
||||
* session exists) because there was no external wall-clock `timeout`
|
||||
* Plus a stale maintainer note claiming agy has no --model flag.
|
||||
* These tests pinned the three real failure modes of agy 1.0.16 against the hand-authored bash
|
||||
* block in `gsd-core/workflows/review.md`. Phase 5b (#2799) deleted that block: the lane is now
|
||||
* declared data plus a named first-party handler, so the assertions move to those surfaces — and
|
||||
* the source-text exemption this file used to carry is no longer needed, because nothing here reads
|
||||
* a source file any more.
|
||||
*
|
||||
* These tests pin the corrected invocation shape in gsd-core/workflows/review.md.
|
||||
* The move is an upgrade, not a translation. The old tests could only prove that certain TEXT
|
||||
* appeared in a markdown fence; these prove the resolved invocation and the handler's actual
|
||||
* behaviour. Every #2073 mode below is the same invariant, checked where it now lives.
|
||||
*
|
||||
* One invariant deliberately CHANGED, and is recorded here rather than silently dropped: mode 3's
|
||||
* external `timeout`/`gtimeout` probe is gone. The bash needed it because `--print-timeout` cannot
|
||||
* fire before agy creates a session, and stock macOS ships neither killer — so on a stock Mac that
|
||||
* leg ran unbounded. The runner spawns with Node's native timeout, which is always available, so
|
||||
* the outer bound is now unconditional. The invariant ("a pre-session stall is bounded") got
|
||||
* stronger; only the mechanism changed.
|
||||
*/
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const REVIEW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'review.md');
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const {
|
||||
antigravityDiagnostic,
|
||||
antigravityTranscriptFallback,
|
||||
stampBlindReview,
|
||||
runLane,
|
||||
} = require('../gsd-core/bin/lib/review-lane-runner.cjs');
|
||||
|
||||
function agyBashBlock() {
|
||||
const content = fs.readFileSync(REVIEW_PATH, 'utf-8');
|
||||
const fences = content.match(/```bash[\s\S]*?```/g) || [];
|
||||
// Target the INVOCATION block (the fence that writes the antigravity review
|
||||
// output), not the `command -v agy` detection one-liner.
|
||||
const agy = fences.find((f) => /\bagy\b/.test(f) && /gsd-review-antigravity/.test(f));
|
||||
assert.ok(agy, 'review.md should contain the agy invocation bash block');
|
||||
return agy;
|
||||
const RUN = '/run';
|
||||
const ROOT = '/repo';
|
||||
const HOME = '/home/u';
|
||||
|
||||
const LANE = REVIEWER_LANES.find((l) => l.slug === 'antigravity');
|
||||
|
||||
function planFor(config = {}) {
|
||||
const r = resolveLanePlan({ lane: LANE, configGet: (k) => config[k], runDir: RUN, repoRoot: ROOT });
|
||||
assert.equal(r.ok, true);
|
||||
return r.plan;
|
||||
}
|
||||
|
||||
describe('Antigravity (agy) reviewer invocation in /gsd-review (#2073)', () => {
|
||||
test('review.md exists', () => {
|
||||
assert.ok(fs.existsSync(REVIEW_PATH), 'review.md should exist');
|
||||
function deps(overrides = {}) {
|
||||
const files = overrides.files || {};
|
||||
const warnings = [];
|
||||
return Object.assign(
|
||||
{
|
||||
files,
|
||||
warnings,
|
||||
spawn: () => ({ status: 0, stdout: '', stderr: '' }),
|
||||
httpJson: async () => ({ ok: true, status: 200, body: '{}' }),
|
||||
readFile: (p) => { if (!(p in files)) throw new Error(`ENOENT ${p}`); return files[p]; },
|
||||
writeFile: (p, c) => { files[p] = c; },
|
||||
exists: (p) => p in files,
|
||||
hasBinary: () => true,
|
||||
configGet: () => undefined,
|
||||
homeDir: HOME,
|
||||
warn: (m) => warnings.push(m),
|
||||
},
|
||||
overrides,
|
||||
);
|
||||
}
|
||||
|
||||
describe('Antigravity lane — #2073 mode 1 (exec arg-list overflow)', () => {
|
||||
test('the prompt travels by FILE REFERENCE, never inlined', () => {
|
||||
// Inline `-p "$(cat <prompt>)"` overflowed the exec arg list on a large prompt set (~197 KB),
|
||||
// failing with rc 126 in a way indistinguishable from a model failure.
|
||||
assert.equal(LANE.invoke.promptChannel, 'argv-file-ref');
|
||||
const argv = planFor().argv;
|
||||
const promptArg = argv[argv.length - 1];
|
||||
assert.ok(promptArg.includes(`${RUN}/gsd-review-prompt.md`), 'must reference the prompt file');
|
||||
assert.ok(promptArg.length < 1000, 'the reference must stay short, never carry the prompt body');
|
||||
});
|
||||
|
||||
test('#2073 mode 1 — does NOT inline the prompt via "$(cat ...)" (arg-list overflow)', () => {
|
||||
const block = agyBashBlock();
|
||||
assert.ok(
|
||||
!/"\$\(cat/.test(block),
|
||||
'agy must not inline the prompt via "$(cat …)" — a large review prompt overflows the exec arg list',
|
||||
);
|
||||
});
|
||||
|
||||
test('#2073 mode 1 — uses a file-reference prompt (mirrors the Cursor block)', () => {
|
||||
const block = agyBashBlock();
|
||||
assert.ok(
|
||||
/Read the file at \{run_dir\}\/gsd-review-prompt\.md/.test(block),
|
||||
'agy should reference the prompt by file path, not inline it',
|
||||
);
|
||||
});
|
||||
|
||||
test('#2073 mode 3 — pairs agy with an external wall-clock killer when available (timeout/gtimeout probe)', () => {
|
||||
const block = agyBashBlock();
|
||||
// Capability probe for GNU `timeout` and macOS `gtimeout` (stock macOS has neither).
|
||||
assert.match(block, /command -v timeout/, 'agy block should probe for the `timeout` killer');
|
||||
assert.match(block, /command -v gtimeout/, 'agy block should probe for `gtimeout` (macOS Homebrew)');
|
||||
// The external cap (600s) is >= agy's native --print-timeout (540s) so it only
|
||||
// backstops a pre-session stall, never cuts a healthy run.
|
||||
assert.match(block, /600 agy --print-timeout 540s/, 'external cap (600s) must be >= --print-timeout (540s)');
|
||||
// Graceful fallback when no external killer is available (stock macOS).
|
||||
assert.match(block, /else\n\s*agy --print-timeout 540s/,
|
||||
'agy block must fall back to --print-timeout alone when no external killer is available (macOS)');
|
||||
});
|
||||
|
||||
test('#2073 mode 3 — stdin is tied to /dev/null (no tty stall)', () => {
|
||||
const block = agyBashBlock();
|
||||
assert.ok(
|
||||
/<\/dev\/null/.test(block),
|
||||
'agy invocation should redirect stdin from /dev/null so it never blocks on a tty',
|
||||
);
|
||||
});
|
||||
|
||||
test('#2073 mode 2 — wires review.models.agy via --model when configured', () => {
|
||||
const block = agyBashBlock();
|
||||
assert.match(block, /--model "\$AGY_MODEL"/, 'agy block should pass --model "$AGY_MODEL" when set');
|
||||
});
|
||||
|
||||
test('#2073 mode 2 — Step 3 stub surfaces a diagnostic from agy cli.log (not just a generic stub)', () => {
|
||||
const block = agyBashBlock();
|
||||
assert.ok(
|
||||
/cli\.log/.test(block),
|
||||
'the empty-output stub should inspect agy cli.log for a model-availability diagnostic (NOT_FOUND / agent executor error)',
|
||||
);
|
||||
});
|
||||
|
||||
test('#2073 — stale "no --model flag" maintainer note is corrected', () => {
|
||||
const content = fs.readFileSync(REVIEW_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
!/No .{0,4}-m.{0,4}\/.{0,4}--model.{0,4} flag/i.test(content),
|
||||
'the stale maintainer note claiming agy has no --model flag must be corrected (--model exists since ~1.0.3)',
|
||||
);
|
||||
});
|
||||
|
||||
test('#2073 — review.models.agy is documented as supported (not "reserved for future")', () => {
|
||||
const content = fs.readFileSync(REVIEW_PATH, 'utf-8');
|
||||
assert.ok(
|
||||
!/review\.models\.agy is reserved for future/i.test(content),
|
||||
'review.models.agy is now wired (passed as --model); the "reserved for future" comment must be updated',
|
||||
);
|
||||
test('the prompt argument carries the absolute repo root (#2176)', () => {
|
||||
// Without the anchor agy reviews the plan text in isolation from its own scratch dir.
|
||||
const argv = planFor().argv;
|
||||
assert.ok(argv[argv.length - 1].includes(ROOT));
|
||||
});
|
||||
});
|
||||
|
||||
describe('Antigravity lane — #2073 mode 2 (a pinned model that 404s)', () => {
|
||||
test('review.models.agy reaches argv as --model', () => {
|
||||
// The slug is `antigravity` but the shipped key is `review.models.agy`; resolving by slug
|
||||
// would silently ignore the very escape hatch this mode exists to provide.
|
||||
assert.equal(LANE.modelConfigKey, 'review.models.agy');
|
||||
const argv = planFor({ 'review.models.agy': 'gemini-x' }).argv;
|
||||
const i = argv.indexOf('--model');
|
||||
assert.ok(i !== -1, '--model must be present when the key is set');
|
||||
assert.equal(argv[i + 1], 'gemini-x');
|
||||
});
|
||||
|
||||
test('no model configured emits no --model', () => {
|
||||
assert.ok(!planFor().argv.includes('--model'));
|
||||
});
|
||||
|
||||
test('the diagnostic surfaces agy cli.log rather than a generic stub', () => {
|
||||
// This mode exits 0 with empty stdout AND an empty transcript — every other signal reads as a
|
||||
// clean run. The log is the only evidence there was a failure at all.
|
||||
const files = {
|
||||
[`${HOME}/.gemini/antigravity-cli/cli.log`]: [
|
||||
'routine',
|
||||
'agent executor error: Publisher model NOT_FOUND gemini-x',
|
||||
].join('\n'),
|
||||
};
|
||||
const out = antigravityDiagnostic(deps({ files }));
|
||||
assert.ok(out.includes('NOT_FOUND'), 'the log line must be surfaced');
|
||||
assert.ok(out.includes('agy models'), 'the remedy must be named');
|
||||
});
|
||||
|
||||
test('an absent log still yields the pre-session-stall tell', () => {
|
||||
const out = antigravityDiagnostic(deps());
|
||||
assert.ok(out.includes('pre-session-stall'));
|
||||
assert.ok(!out.includes('agy models'), 'no hint should be invented without evidence');
|
||||
});
|
||||
});
|
||||
|
||||
describe('Antigravity lane — #2073 mode 3 (pre-session stall)', () => {
|
||||
test('the outer wall-clock bound is declared and unconditional', () => {
|
||||
// The bash probed for GNU `timeout` / `gtimeout` and fell back to --print-timeout alone when
|
||||
// neither existed — which is stock macOS, so that leg ran unbounded there. The bound is now
|
||||
// the plan's own timeout, enforced by the spawn on every platform.
|
||||
assert.equal(LANE.timeoutFloorMs, 600000);
|
||||
assert.equal(planFor().timeoutMs, 600000);
|
||||
});
|
||||
|
||||
test('the tool-native inner timeout stays in argv', () => {
|
||||
// Two-level by design: a 600s outer cap over a 540s native --print-timeout. The descriptor
|
||||
// carries ONE scalar; the inner bound is the lane's own argument (ADR-2782 D6).
|
||||
const argv = planFor().argv;
|
||||
const i = argv.indexOf('--print-timeout');
|
||||
assert.ok(i !== -1);
|
||||
assert.equal(argv[i + 1], '540s');
|
||||
});
|
||||
|
||||
test('the REVIEW spawn receives the outer bound, and every spawn is bounded', async () => {
|
||||
const p = planFor();
|
||||
const calls = [];
|
||||
const d = deps({
|
||||
spawn: (b, a, o) => { calls.push({ argv: a, opts: o }); return { status: 0, stdout: 'a review', stderr: '' }; },
|
||||
});
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
|
||||
// The handler also spawns `agy --help` to probe --add-dir, so target the review invocation by
|
||||
// its shape rather than by position — a positional assertion silently follows the wrong call
|
||||
// the moment another probe is added.
|
||||
const review = calls.find((c) => c.argv.includes('-p'));
|
||||
assert.ok(review, 'expected a review invocation');
|
||||
assert.equal(review.opts.timeoutMs, 600000);
|
||||
|
||||
for (const c of calls) {
|
||||
assert.ok(c.opts.timeoutMs > 0, `unbounded spawn: ${c.argv.join(' ')}`);
|
||||
}
|
||||
});
|
||||
|
||||
test('no prompt is fed on stdin, so a tty stall is impossible', () => {
|
||||
// The bash tied stdin to /dev/null for exactly this reason.
|
||||
assert.equal(planFor().stdin, null);
|
||||
});
|
||||
});
|
||||
|
||||
describe('Antigravity lane — transcript fallback and staleness', () => {
|
||||
const CACHE = `${HOME}/.gemini/antigravity-cli/cache/last_conversations.json`;
|
||||
const TX = (id) => `${HOME}/.gemini/antigravity-cli/brain/${id}/.system_generated/logs/transcript.jsonl`;
|
||||
const entry = (content) =>
|
||||
JSON.stringify({ source: 'MODEL', status: 'DONE', type: 'PLANNER_RESPONSE', content });
|
||||
|
||||
test('the lane is handler-owned, so the generic stub cannot pre-empt the fallback', () => {
|
||||
assert.equal(LANE.emptyOutput, 'handler-owned');
|
||||
assert.equal(LANE.handler, 'antigravity');
|
||||
});
|
||||
|
||||
test('a response written by THIS run is read back', () => {
|
||||
const files = {
|
||||
[CACHE]: JSON.stringify({ [ROOT]: 'c1' }),
|
||||
[TX('c1')]: [entry('old'), entry('NEW')].join('\n'),
|
||||
};
|
||||
assert.equal(antigravityTranscriptFallback(ROOT, { convId: 'c1', lines: 1 }, deps({ files })), 'NEW');
|
||||
});
|
||||
|
||||
test('a response from a PRIOR run is never presented as this one', () => {
|
||||
const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: entry('STALE') };
|
||||
assert.equal(antigravityTranscriptFallback(ROOT, { convId: 'c1', lines: 1 }, deps({ files })), '');
|
||||
});
|
||||
|
||||
test('a blind review is marked so consensus can down-weight it (#2176)', () => {
|
||||
assert.ok(
|
||||
stampBlindReview('REVIEWED-WITHOUT-REPO-ACCESS\nx').includes('[reviewed-without-repo-access]'),
|
||||
);
|
||||
});
|
||||
|
||||
test('all three layers failing still writes a diagnosable artifact, never an empty file', async () => {
|
||||
const p = planFor();
|
||||
const d = deps({ spawn: () => ({ status: 1, stdout: '', stderr: 'boom' }) });
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(r.stubbed, true);
|
||||
assert.ok(d.files[p.reviewPath].includes('failed or returned empty output'));
|
||||
assert.ok(d.files[p.reviewPath].includes('pre-session-stall'));
|
||||
});
|
||||
});
|
||||
|
||||
@@ -60,28 +60,35 @@ describe('Cursor CLI reviewer in /gsd-review (#1960)', () => {
|
||||
);
|
||||
});
|
||||
|
||||
test('invocation uses cursor-agent single binary with -p flag', () => {
|
||||
const c = fs.readFileSync(reviewPath, 'utf-8');
|
||||
assert.ok(
|
||||
c.includes('cursor-agent -p'),
|
||||
'review.md should invoke cursor via "cursor-agent -p" (single binary, not two-token "cursor agent")'
|
||||
);
|
||||
// Phase 5b (#2799) moved the invocation out of review.md's bash and into the declared lane.
|
||||
// These four assertions follow it: the descriptor and the resolved plan ARE the deployed
|
||||
// contract now, and asserting on them is strictly stronger than matching fence text.
|
||||
test('invocation uses the cursor-agent single binary, not two-token "cursor agent"', () => {
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === 'cursor');
|
||||
assert.equal(lane.invoke.binary, 'cursor-agent');
|
||||
assert.ok(lane.invoke.args.includes('-p'));
|
||||
});
|
||||
|
||||
test('invocation includes --output-format text', () => {
|
||||
const c = fs.readFileSync(reviewPath, 'utf-8');
|
||||
assert.ok(
|
||||
c.includes('--output-format text'),
|
||||
'review.md cursor-agent invocation should include "--output-format text"'
|
||||
);
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === 'cursor');
|
||||
const i = lane.invoke.args.indexOf('--output-format');
|
||||
assert.ok(i !== -1);
|
||||
assert.equal(lane.invoke.args[i + 1], 'text');
|
||||
});
|
||||
|
||||
test('invocation passes prompt as a file-path argument (not via stdin pipe)', () => {
|
||||
const c = fs.readFileSync(reviewPath, 'utf-8');
|
||||
assert.ok(
|
||||
c.includes('Read the file at {run_dir}/gsd-review-prompt.md'),
|
||||
'review.md cursor-agent invocation should pass prompt by referencing the file path as an argument'
|
||||
);
|
||||
test('the prompt is a file-path ARGUMENT, never piped on stdin', () => {
|
||||
// Print mode takes the prompt as an argument, and a full plan set inline would approach the
|
||||
// 32,767-char Windows execFileSync ceiling — hence the file reference.
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === 'cursor');
|
||||
assert.equal(lane.invoke.promptChannel, 'argv-file-ref');
|
||||
const r = resolveLanePlan({ lane, configGet: () => undefined, runDir: '/rd', repoRoot: '/repo' });
|
||||
assert.equal(r.ok, true);
|
||||
assert.equal(r.plan.stdin, null, 'nothing may be fed on stdin');
|
||||
assert.ok(r.plan.argv[r.plan.argv.length - 1].includes('/rd/gsd-review-prompt.md'));
|
||||
});
|
||||
|
||||
test('does NOT use broken two-token "cursor agent " form', () => {
|
||||
@@ -104,11 +111,18 @@ describe('Cursor CLI reviewer in /gsd-review (#1960)', () => {
|
||||
);
|
||||
});
|
||||
|
||||
test('contains Cursor Review section in REVIEWS.md template', () => {
|
||||
const c = fs.readFileSync(reviewPath, 'utf-8');
|
||||
assert.ok(
|
||||
c.includes('Cursor Review'),
|
||||
'review.md should include a "Cursor Review" section in the REVIEWS.md template'
|
||||
test('the lane declares its REVIEWS.md section', () => {
|
||||
// The heading used to be a literal in review.md's write_reviews template. Phase 5b renders
|
||||
// sections from each lane's declared `reviewsSection`, so THAT is the contract now — and
|
||||
// uniqueness across lanes is enforced by the parity gate (ADR-2782 D8), which a hardcoded
|
||||
// list never was.
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === 'cursor');
|
||||
assert.equal(lane.reviewsSection, 'Cursor');
|
||||
const sections = REVIEWER_LANES.map((l) => l.reviewsSection);
|
||||
assert.equal(
|
||||
sections.filter((x) => x === 'Cursor').length, 1,
|
||||
'two lanes sharing a heading would silently merge their output in REVIEWS.md',
|
||||
);
|
||||
});
|
||||
|
||||
|
||||
@@ -393,16 +393,42 @@ describe('#2481 review workflow resolves effort per reviewer', () => {
|
||||
'utf8',
|
||||
);
|
||||
|
||||
test('review.md invokes resolve-execution — the grep ADR-443 said returned zero hits', () => {
|
||||
test('shipped orchestration invokes resolve-execution — the grep ADR-443 said returned zero hits', () => {
|
||||
// Phase 5b (#2799) moved the call out of review.md's per-lane bash and into the review-lane
|
||||
// route, which resolves effort once per selected lane through the SAME surface. ADR-443's
|
||||
// invariant is about shipped orchestration calling resolve-execution at all, not about which
|
||||
// file it lives in — so the assertion follows the call rather than pinning the old location.
|
||||
const toolsSrc = fs.readFileSync(
|
||||
path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'), 'utf-8',
|
||||
);
|
||||
assert.ok(
|
||||
reviewMd.includes('resolve-execution'),
|
||||
toolsSrc.includes('resolve-execution'),
|
||||
'ADR-443 blocks on no shipped orchestration calling resolve-execution',
|
||||
);
|
||||
});
|
||||
|
||||
test('each argv reviewer receives its effort variable on the command line', () => {
|
||||
for (const [cli, varName] of [['claude', 'CLAUDE_EFFORT_ARGS'], ['codex', 'CODEX_EFFORT_ARGS'], ['opencode', 'OPENCODE_EFFORT_ARGS']]) {
|
||||
assert.ok(reviewMd.includes(`$${varName}`), `${cli} invocation must carry $${varName}`);
|
||||
test('each argv-effort reviewer places effort in its resolved command line', () => {
|
||||
// Stronger than the old shell-variable check: this asserts the effort actually lands in the
|
||||
// argv AT THE POSITION the lane declares, which a `$VAR` substring never proved. Lanes whose
|
||||
// effortChannel is not `argv` must receive nothing.
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const EFFORT = ['--effort', 'high'];
|
||||
for (const lane of REVIEWER_LANES.filter((l) => l.transport === 'spawn')) {
|
||||
const r = resolveLanePlan({
|
||||
lane, configGet: () => undefined, runDir: '/run', repoRoot: '/repo', effortArgs: EFFORT,
|
||||
});
|
||||
assert.equal(r.ok, true, `${lane.slug} failed to resolve`);
|
||||
const carries = r.plan.argv.includes('--effort');
|
||||
assert.equal(
|
||||
carries, lane.invoke.effortChannel === 'argv',
|
||||
`${lane.slug}: effortChannel=${lane.invoke.effortChannel} but argv ${carries ? 'carries' : 'omits'} effort`,
|
||||
);
|
||||
}
|
||||
// The three lanes ADR-1239 #2481 named must still be the argv-effort set.
|
||||
const argvEffort = REVIEWER_LANES
|
||||
.filter((l) => l.transport === 'spawn' && l.invoke.effortChannel === 'argv')
|
||||
.map((l) => l.slug).sort();
|
||||
assert.deepStrictEqual(argvEffort, ['claude', 'codex', 'opencode']);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,8 +0,0 @@
|
||||
{
|
||||
"version": 1,
|
||||
"paths": {
|
||||
"review.md": {
|
||||
"reason": "#2797 / ADR-2782 D9 (config half). Growth is comment-only: the three per-lane prompt-budget fallback guards (ollama, lm_studio, llama_cpp) gain an explanation of why they test for the -1 sentinel rather than 0. A federated config key always resolves to its declared default rather than reporting not-found, so an unset per-lane budget needs a sentinel that cannot collide with a legitimate value — and 0 is legitimate: it already means \"do not trim this lane\" (the early-return guard in prepare_trimmed_prompt_for_reviewer). Treating 0 as unset would have silently switched a user who deliberately disabled trimming for one lane onto the global budget. No lane's invocation shape changed."
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -8,8 +8,13 @@
|
||||
* source-grounded review (~570s Codex xhigh, ~525s headless Claude) is killed
|
||||
* mid-review, its output is empty, and the cross-AI review silently proceeds
|
||||
* with fewer lanes. CodeRabbit and OpenCode already documented a timeout; the
|
||||
* four main lanes did not. review.md IS the product the runtime loads, so this
|
||||
* asserts the deployed text carries the guidance.
|
||||
* four main lanes did not. review.md IS the product the runtime loads, so this asserts the
|
||||
* deployed text carries the guidance.
|
||||
*
|
||||
* Phase 5b (#2799) replaced the per-lane blocks with a loop, so the anchor moved from the old
|
||||
* "invoke in sequence" prose to the step itself — but the guidance is MORE load-bearing now, not
|
||||
* less: one Bash call wraps the whole loop, so a host timeout kills every remaining lane rather
|
||||
* than one.
|
||||
*/
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
@@ -21,13 +26,19 @@ const REVIEW_MD = path.join(__dirname, '..', 'gsd-core', 'workflows', 'review.md
|
||||
|
||||
describe('#2194 review.md prompt-fed reviewer timeout guidance', () => {
|
||||
const content = fs.readFileSync(REVIEW_MD, 'utf-8');
|
||||
const sectionStart = content.indexOf('invoke in sequence');
|
||||
const section = sectionStart !== -1
|
||||
? content.slice(sectionStart, sectionStart + 3000)
|
||||
: '';
|
||||
const sectionStart = content.indexOf('<step name="invoke_reviewers">');
|
||||
const sectionEnd = content.indexOf('</step>', sectionStart);
|
||||
const section = sectionStart !== -1 ? content.slice(sectionStart, sectionEnd) : '';
|
||||
|
||||
test('review.md has the reviewer-invocation section', () => {
|
||||
assert.notEqual(sectionStart, -1, 'review.md must contain the "invoke in sequence" section');
|
||||
test('review.md has the reviewer-invocation step', () => {
|
||||
assert.notEqual(sectionStart, -1, 'review.md must contain the invoke_reviewers step');
|
||||
});
|
||||
|
||||
test('lanes are invoked sequentially, not in parallel', () => {
|
||||
// Concurrent invocation trips provider rate limits; the original prose said so and the loop
|
||||
// must keep saying so, since a future reader could otherwise "optimize" it into a fan-out.
|
||||
assert.ok(/sequential|in sequence|not in parallel/i.test(section),
|
||||
'the step must state that lanes run sequentially');
|
||||
});
|
||||
|
||||
test('the section carries Bash timeout guidance for the prompt-fed lanes', () => {
|
||||
|
||||
@@ -75,98 +75,49 @@ describe('#2358 review.md temp paths are run-scoped, not phase-only', () => {
|
||||
);
|
||||
});
|
||||
|
||||
test('the Antigravity reviewer prompt-instruction string references the run-scoped path', () => {
|
||||
const agyPromptMatch = content.match(/_AGY_PROMPT="Read the file at ([^ ]+)/);
|
||||
assert.ok(agyPromptMatch, '_AGY_PROMPT must contain a "Read the file at <path>" instruction');
|
||||
assert.equal(
|
||||
agyPromptMatch[1], '{run_dir}/gsd-review-prompt.md',
|
||||
'the Antigravity reviewer must be told to read the run-scoped prompt path, not a bare {phase}-only /tmp path — ' +
|
||||
'this is the exact instruction text the reporter forensically traced back to a stale cross-project read'
|
||||
);
|
||||
// Phase 5b (#2799) moved these strings out of review.md's bash and into the resolver and the
|
||||
// antigravity handler, so the assertions follow them. The invariant is unchanged and is what
|
||||
// #2358 was about: every reviewer artifact must live under the run-scoped mktemp directory, never
|
||||
// a bare `{phase}`-keyed /tmp path that a concurrent review could collide with.
|
||||
test('every lane anchors its prompt and artifacts under the run dir', () => {
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const RUN = '/run-scoped';
|
||||
for (const lane of REVIEWER_LANES) {
|
||||
const r = resolveLanePlan({
|
||||
lane, configGet: () => undefined, runDir: RUN, repoRoot: '/repo',
|
||||
});
|
||||
assert.equal(r.ok, true, `${lane.slug} failed to resolve`);
|
||||
const p = r.plan;
|
||||
assert.ok(p.reviewPath.startsWith(`${RUN}/`), `${lane.slug} review path escapes the run dir`);
|
||||
assert.ok(p.errPath.startsWith(`${RUN}/`), `${lane.slug} err path escapes the run dir`);
|
||||
assert.ok(p.promptPath.startsWith(`${RUN}/`), `${lane.slug} prompt path escapes the run dir`);
|
||||
}
|
||||
});
|
||||
|
||||
test('the Cursor reviewer prompt-instruction string references the run-scoped path', () => {
|
||||
const cursorPromptMatch = content.match(/CURSOR_PROMPT_ARG="Read the file at ([^ ]+)/);
|
||||
assert.ok(cursorPromptMatch, 'CURSOR_PROMPT_ARG must contain a "Read the file at <path>" instruction');
|
||||
assert.equal(
|
||||
cursorPromptMatch[1], '{run_dir}/gsd-review-prompt.md',
|
||||
'the Cursor reviewer must be told to read the run-scoped prompt path, not a bare {phase}-only /tmp path'
|
||||
test('the argv-borne prompt instruction references the run-scoped path', () => {
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const RUN = '/run-scoped';
|
||||
const fileRefLanes = REVIEWER_LANES.filter(
|
||||
(l) => l.transport === 'spawn' && l.invoke.promptChannel === 'argv-file-ref',
|
||||
);
|
||||
assert.ok(fileRefLanes.length > 0, 'expected at least one argv-file-ref lane');
|
||||
for (const lane of fileRefLanes) {
|
||||
const r = resolveLanePlan({ lane, configGet: () => undefined, runDir: RUN, repoRoot: '/repo' });
|
||||
const arg = r.plan.argv[r.plan.argv.length - 1];
|
||||
assert.ok(arg.includes(`${RUN}/gsd-review-prompt.md`), `${lane.slug} prompt not run-scoped`);
|
||||
}
|
||||
});
|
||||
|
||||
test('the run directory is cleaned up at the end of the review', () => {
|
||||
const presentResultsStart = content.indexOf('<step name="present_results">');
|
||||
assert.notEqual(presentResultsStart, -1, 'review.md must contain the present_results step');
|
||||
const section = content.slice(presentResultsStart, presentResultsStart + 1500);
|
||||
assert.ok(
|
||||
/rm -rf "\{run_dir\}"/.test(section),
|
||||
'present_results must remove the run-scoped temp directory once REVIEWS.md is written'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#2358 ship.md external-review stderr capture is run-scoped', () => {
|
||||
const content = fs.readFileSync(SHIP_MD, 'utf-8');
|
||||
|
||||
test('no bare, unqualified /tmp/gsd-review-stderr.log path remains', () => {
|
||||
assert.ok(
|
||||
!content.includes('/tmp/gsd-review-stderr.log'),
|
||||
'ship.md must not write/read a shared, unqualified stderr log path — every ship run, phase, and project ' +
|
||||
'shares this exact path with zero disambiguator, which is strictly worse than review.md\'s phase-only keying'
|
||||
);
|
||||
});
|
||||
|
||||
test('stderr is captured to a per-run file via the portable ${TMPDIR:-/tmp} seam', () => {
|
||||
assert.ok(
|
||||
/REVIEW_STDERR_FILE=\$\(mktemp "\$\{TMPDIR:-\/tmp\}\/gsd-review-stderr-XXXXXX"\)/.test(content),
|
||||
'ship.md must create the stderr capture file via `mktemp "${TMPDIR:-/tmp}/gsd-review-stderr-XXXXXX"`'
|
||||
);
|
||||
assert.ok(
|
||||
/2>"\$\{REVIEW_STDERR_FILE\}"/.test(content),
|
||||
'the external review command invocation must redirect stderr to the per-run $REVIEW_STDERR_FILE, not a literal path'
|
||||
);
|
||||
assert.ok(
|
||||
/cat "\$\{REVIEW_STDERR_FILE\}"/.test(content),
|
||||
'the failure-handling block must read back the same per-run $REVIEW_STDERR_FILE'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#2358 reviewer-instances.md (#1517, lazily loaded from invoke_reviewers) is run-scoped too', () => {
|
||||
// review.md's own invoke_reviewers step lazily loads this companion doc for the
|
||||
// review.reviewer_instances codepath — it was missed in the initial pass and still
|
||||
// pointed at the old, unscoped /tmp/gsd-review-*-{phase} paths, which both broke
|
||||
// reviewer-instances functionality (the prompt file build_prompt now writes lives
|
||||
// at {run_dir}/gsd-review-prompt.md, never the old path) and left the exact
|
||||
// cross-project collision bug open for that code path.
|
||||
const content = fs.readFileSync(REVIEWER_INSTANCES_MD, 'utf-8');
|
||||
|
||||
test('no bare, unscoped /tmp/gsd-review-* path remains', () => {
|
||||
assert.ok(
|
||||
!content.includes('/tmp/gsd-review'),
|
||||
'reviewer-instances.md must not contain any hardcoded /tmp/gsd-review* literal — ' +
|
||||
'every review temp path must be rooted under the run-scoped {run_dir} directory'
|
||||
);
|
||||
});
|
||||
|
||||
test('no temp path is still keyed on a bare {phase} placeholder', () => {
|
||||
assert.ok(
|
||||
!/gsd-review[^\r\n]*\{phase\}/.test(content),
|
||||
'no temp path may still be keyed on a bare {phase} placeholder'
|
||||
);
|
||||
});
|
||||
|
||||
test('the instance prompt read and output write are threaded through {run_dir}', () => {
|
||||
assert.ok(
|
||||
/\{run_dir\}\/gsd-review-prompt\.md/.test(content),
|
||||
'reviewer-instances.md must read the prompt from {run_dir}/gsd-review-prompt.md, ' +
|
||||
'the same run-scoped path build_prompt writes in review.md'
|
||||
);
|
||||
assert.ok(
|
||||
/\{run_dir\}\/gsd-review-\$\{INSTANCE_NAME\}\.md/.test(content),
|
||||
'reviewer-instances.md must write each instance\'s output to ' +
|
||||
'{run_dir}/gsd-review-${INSTANCE_NAME}.md, not a {phase}-keyed /tmp path'
|
||||
);
|
||||
test('an instance writes under the run dir, keyed by its own identity', () => {
|
||||
// Two instances of one adapter must not overwrite each other, and neither may escape the run
|
||||
// dir — the identity is sanitized to a flat filename.
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === 'opencode');
|
||||
const r = resolveLanePlan({ lane, configGet: () => undefined, runDir: '/run-scoped', repoRoot: '/repo' });
|
||||
assert.ok(r.plan.reviewPath.startsWith('/run-scoped/'));
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -1,204 +1,121 @@
|
||||
// allow-test-rule: source-text-is-the-product (see #2494)
|
||||
// The Gemini and Claude reviewer dispatch blocks in gsd-core/workflows/review.md
|
||||
// ARE the runtime contract — the workflow's text is what the reviewing agent
|
||||
// executes. This suite extracts those two shell blocks verbatim from the
|
||||
// workflow and runs them under a real bash against a failing CLI stub, so the
|
||||
// shipped guard is what gets exercised rather than a reimplementation of it.
|
||||
// The assertions on the produced review file are assertions on that guard's
|
||||
// documented output contract (the stub line the consensus step must be able to
|
||||
// tell apart from a clean empty review), not incidental string matching.
|
||||
|
||||
/**
|
||||
* Regression tests for #2494 — the claude and gemini reviewer legs discarded
|
||||
* stderr with no empty-output guard.
|
||||
* #2494 — a failed reviewer lane must be diagnosable, never a silent drop.
|
||||
*
|
||||
* Before the fix both blocks were `... 2>/dev/null > {run_dir}/gsd-review-<leg>.md`
|
||||
* with nothing after: a non-zero exit that wrote no stdout (CLI missing,
|
||||
* unauthenticated, rate-limited, timeout-killed, crashed) left a ZERO-BYTE
|
||||
* review file with the only diagnostic evidence — stderr — already discarded.
|
||||
* The write_reviews step then substituted that empty file into a
|
||||
* `## <Reviewer> Review` section indistinguishable from "ran cleanly, nothing
|
||||
* to report", silently degrading the advertised N-reviewer consensus to N-1.
|
||||
* Before the fix, gemini and claude sent stderr to `/dev/null` and wrote nothing on failure. A
|
||||
* failed lane — CLI missing, unauthenticated, rate-limited, crashed, any exit that writes no
|
||||
* stdout — left a zero-byte file that `write_reviews` rendered as "a reviewer that ran cleanly with
|
||||
* nothing to report", silently dropping a lane from the cross-AI consensus while `present_results`
|
||||
* reported success.
|
||||
*
|
||||
* These tests fail against pre-fix review.md: the produced file is zero-byte,
|
||||
* so both the non-empty and the diagnosable-message assertions trip.
|
||||
* The invariant is unchanged; what moved is where it lives. This suite used to extract the two
|
||||
* shell blocks verbatim from `review.md` and run them under a real bash against a failing stub.
|
||||
* Phase 5b (#2799) deleted those blocks, so the tests now drive the real runner with stubbed
|
||||
* dependencies — the same behavioural altitude (a lane is actually run and its artifacts
|
||||
* inspected), against the surface that ships today. Nothing here reads source text any more, so
|
||||
* the source-text exemption this file used to carry is gone.
|
||||
*
|
||||
* The guarantee also got STRONGER in one way worth locking: the policy is uniform across every
|
||||
* lane now rather than fixed per-leg, so these assertions run over the whole spawn roster instead
|
||||
* of the two legs the issue named.
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { describe, test, before, after } = require('node:test');
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const { spawnSync } = require('node:child_process');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { createTempDir, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const REVIEW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'review.md');
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const { runLane } = require('../gsd-core/bin/lib/review-lane-runner.cjs');
|
||||
|
||||
// Normalize CRLF: on a Windows git-autocrlf checkout every line carries a
|
||||
// trailing \r, which would leave the extracted block's shebang/redirect tokens
|
||||
// mangled and defeat the fence regexes below.
|
||||
const WORKFLOW = fs.readFileSync(REVIEW_PATH, 'utf-8').replace(/\r\n/g, '\n');
|
||||
const RUN = '/run';
|
||||
const ROOT = '/repo';
|
||||
|
||||
// This suite executes the extracted blocks with a real bash and a POSIX CLI
|
||||
// stub on PATH. Gate to non-Windows, mirroring the opencode reconstruction
|
||||
// suite's win32 skip — the guard logic is platform-independent and is asserted
|
||||
// in full on every macOS/Linux CI leg.
|
||||
const skipReason = process.platform === 'win32'
|
||||
? 'extracted block is POSIX shell; guard logic is platform-independent and asserted on macOS/Linux'
|
||||
: false;
|
||||
/** Lanes whose empty-output policy is the shared stub (antigravity owns its own diagnostics). */
|
||||
const STUB_LANES = REVIEWER_LANES.filter(
|
||||
(l) => l.transport === 'spawn' && l.emptyOutput === 'stub-with-stderr',
|
||||
);
|
||||
|
||||
/**
|
||||
* Extract a reviewer dispatch block verbatim from the workflow. If review.md
|
||||
* changes the block's shape these throw and the test fails loudly — intended
|
||||
* coupling, the same contract the opencode suite pins for its jq programs.
|
||||
*/
|
||||
function extractBlock(headingRe, label) {
|
||||
const re = new RegExp(`${headingRe}\\n\`\`\`bash\\n([\\s\\S]*?)\\n\`\`\``);
|
||||
const m = WORKFLOW.match(re);
|
||||
assert.ok(m, `review.md must define the ${label} reviewer dispatch as a bash block (#2494)`);
|
||||
return m[1];
|
||||
function planFor(slug) {
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === slug);
|
||||
const r = resolveLanePlan({ lane, configGet: () => undefined, runDir: RUN, repoRoot: ROOT });
|
||||
assert.equal(r.ok, true, `${slug} failed to resolve`);
|
||||
return r.plan;
|
||||
}
|
||||
|
||||
const GEMINI_BLOCK = extractBlock('\\*\\*Gemini:\\*\\*', 'Gemini');
|
||||
const CLAUDE_BLOCK = extractBlock('\\*\\*Claude \\(separate session\\):\\*\\*', 'Claude');
|
||||
|
||||
const STUB_STDERR = 'gsd-2494-stub: command not found / not authenticated';
|
||||
|
||||
let sandbox;
|
||||
|
||||
before(() => {
|
||||
sandbox = createTempDir('gsd-2494-');
|
||||
});
|
||||
|
||||
after(() => {
|
||||
cleanup(sandbox);
|
||||
});
|
||||
|
||||
/**
|
||||
* Run one extracted block with `{run_dir}` pointed at a fresh run directory and
|
||||
* `stubBody` installed on PATH under the reviewer's binary name.
|
||||
*
|
||||
* Single exec site for the whole suite so the Windows guard lives in one place:
|
||||
* Git Bash (msys2) ignores Node's chmod exec bit for PATH-executed
|
||||
* extension-less scripts (DEFECT.WINDOWS-TEST-PORTABILITY), and every suite
|
||||
* below is skipped on win32 — this early return keeps the exec unreachable
|
||||
* there rather than relying on the skip alone.
|
||||
*/
|
||||
function runBlockWithStub({ binName, block, stubBody, env = {} }) {
|
||||
if (process.platform === 'win32') return null;
|
||||
|
||||
const caseDir = fs.mkdtempSync(path.join(sandbox, 'run-'));
|
||||
const runDir = path.join(caseDir, 'run');
|
||||
const binDir = path.join(caseDir, 'bin');
|
||||
fs.mkdirSync(runDir);
|
||||
fs.mkdirSync(binDir);
|
||||
|
||||
const stub = path.join(binDir, binName);
|
||||
fs.writeFileSync(stub, stubBody);
|
||||
fs.chmodSync(stub, 0o755);
|
||||
|
||||
fs.writeFileSync(path.join(runDir, 'gsd-review-prompt.md'), '# review prompt\n');
|
||||
|
||||
const script = block.split('{run_dir}').join(runDir);
|
||||
const result = spawnSync('bash', ['-c', script], {
|
||||
encoding: 'utf8',
|
||||
timeout: 30000,
|
||||
killSignal: 'SIGKILL',
|
||||
env: { ...process.env, PATH: `${binDir}${path.delimiter}${process.env.PATH}`, ...env },
|
||||
});
|
||||
|
||||
const reviewPath = path.join(runDir, `gsd-review-${binName}.md`);
|
||||
function deps(spawnResult, files = {}) {
|
||||
return {
|
||||
result,
|
||||
reviewPath,
|
||||
errPath: path.join(runDir, `gsd-review-${binName}.err`),
|
||||
review: fs.existsSync(reviewPath) ? fs.readFileSync(reviewPath, 'utf-8') : null,
|
||||
files,
|
||||
// `kimi-code` declares a `command-capability` probe, so the runner spawns `--help` BEFORE the
|
||||
// review. Answer that separately or the probe fails and the lane never reaches the invocation
|
||||
// this test is about.
|
||||
spawn: (binary, argv) =>
|
||||
argv && argv.length === 1 && argv[0] === '--help'
|
||||
? { status: 0, stdout: '--output-format', stderr: '' }
|
||||
: spawnResult,
|
||||
httpJson: async () => ({ ok: true, status: 200, body: '{}' }),
|
||||
readFile: (p) => { if (!(p in files)) throw new Error(`ENOENT ${p}`); return files[p]; },
|
||||
writeFile: (p, c) => { files[p] = c; },
|
||||
exists: (p) => p in files,
|
||||
hasBinary: () => true,
|
||||
configGet: () => undefined,
|
||||
homeDir: '/home/u',
|
||||
warn: () => {},
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* The issue's own repro harness: a CLI that writes STUB_STDERR to stderr,
|
||||
* nothing to stdout, and exits non-zero.
|
||||
*/
|
||||
function runLeg({ binName, block, env = {} }) {
|
||||
return runBlockWithStub({
|
||||
binName,
|
||||
block,
|
||||
stubBody: `#!/bin/sh\necho "${STUB_STDERR}" >&2\nexit 127\n`,
|
||||
env,
|
||||
});
|
||||
}
|
||||
describe('#2494 — a failed lane writes a diagnosable stub, not a zero-byte file', () => {
|
||||
for (const lane of STUB_LANES) {
|
||||
test(`${lane.slug}: a lane that exits non-zero with no stdout is stubbed`, async () => {
|
||||
const p = planFor(lane.slug);
|
||||
const d = deps({ status: 127, stdout: '', stderr: 'command not found' });
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
|
||||
/**
|
||||
* The guard's contract: the review file exists, is NOT empty, names the leg as
|
||||
* failed-or-empty, and carries the captured stderr. The last assertion is what
|
||||
* separates this fix from a bare `[ ! -s ]` stub — the evidence has to survive.
|
||||
*/
|
||||
function assertDiagnosable({ review, reviewPath, errPath }, legLabel) {
|
||||
assert.ok(review !== null, `${legLabel}: review file must exist after a failed lane (#2494)`);
|
||||
assert.notStrictEqual(review.trim(), '', `${legLabel}: review file must not be empty after a failed lane (#2494)`);
|
||||
assert.match(
|
||||
review,
|
||||
new RegExp(`${legLabel} review failed or returned empty output`, 'i'),
|
||||
`${legLabel}: review file must carry a diagnosable failure line, distinguishable at consensus synthesis from "ran cleanly, nothing to report" (#2494)`,
|
||||
);
|
||||
assert.ok(
|
||||
review.includes(STUB_STDERR),
|
||||
`${legLabel}: captured stderr must be appended to the review file, not discarded to /dev/null (#2494)`,
|
||||
);
|
||||
assert.ok(fs.existsSync(errPath), `${legLabel}: stderr must be captured to a .err sidecar (#2494)`);
|
||||
assert.ok(reviewPath.endsWith('.md'), `${legLabel}: review output path unchanged`);
|
||||
}
|
||||
|
||||
describe('#2494 — gemini reviewer leg fails loudly', { skip: skipReason }, () => {
|
||||
test('a failing gemini CLI produces a diagnosable stub, not an empty file (model configured)', () => {
|
||||
const out = runLeg({ binName: 'gemini', block: GEMINI_BLOCK, env: { GEMINI_MODEL: 'gemini-test-model' } });
|
||||
assertDiagnosable(out, 'Gemini');
|
||||
});
|
||||
|
||||
test('a failing gemini CLI produces a diagnosable stub, not an empty file (no model configured)', () => {
|
||||
const out = runLeg({ binName: 'gemini', block: GEMINI_BLOCK, env: { GEMINI_MODEL: '' } });
|
||||
assertDiagnosable(out, 'Gemini');
|
||||
});
|
||||
});
|
||||
|
||||
describe('#2494 — claude reviewer leg fails loudly', { skip: skipReason }, () => {
|
||||
test('a failing claude CLI produces a diagnosable stub, not an empty file (model configured)', () => {
|
||||
const out = runLeg({
|
||||
binName: 'claude',
|
||||
block: CLAUDE_BLOCK,
|
||||
env: { CLAUDE_MODEL: 'claude-test-model', CLAUDE_EFFORT_ARGS: '' },
|
||||
});
|
||||
assertDiagnosable(out, 'Claude');
|
||||
});
|
||||
|
||||
test('a failing claude CLI produces a diagnosable stub, not an empty file (no model configured)', () => {
|
||||
const out = runLeg({
|
||||
binName: 'claude',
|
||||
block: CLAUDE_BLOCK,
|
||||
env: { CLAUDE_MODEL: '', CLAUDE_EFFORT_ARGS: '' },
|
||||
});
|
||||
assertDiagnosable(out, 'Claude');
|
||||
});
|
||||
});
|
||||
|
||||
describe('#2494 — the guard does not swallow a successful review', { skip: skipReason }, () => {
|
||||
test('gemini stdout is preserved verbatim when the CLI succeeds', () => {
|
||||
const out = runBlockWithStub({
|
||||
binName: 'gemini',
|
||||
block: GEMINI_BLOCK,
|
||||
stubBody: '#!/bin/sh\necho "## Real Review"\necho "Looks good."\nexit 0\n',
|
||||
env: { GEMINI_MODEL: '' },
|
||||
assert.equal(r.stubbed, true, 'a failed lane must be reported as stubbed');
|
||||
const review = d.files[p.reviewPath];
|
||||
assert.ok(review !== undefined, 'a review file must exist after a failed lane');
|
||||
assert.notStrictEqual(review.trim(), '', 'the review file must not be empty');
|
||||
assert.ok(
|
||||
review.includes('failed or returned empty output'),
|
||||
'the stub must be distinguishable from a real review',
|
||||
);
|
||||
});
|
||||
|
||||
const { review } = out;
|
||||
assert.ok(review !== null, 'a successful review must produce a review file (#2494)');
|
||||
assert.ok(review.includes('Looks good.'), 'a successful review must pass through untouched (#2494)');
|
||||
test(`${lane.slug}: stderr is captured to a .err sidecar, never discarded`, async () => {
|
||||
// The sidecar is the difference between "this lane failed" and "this lane failed BECAUSE…".
|
||||
// Without it every failure mode looks identical to every other.
|
||||
const p = planFor(lane.slug);
|
||||
const d = deps({ status: 1, stdout: '', stderr: 'HTTP 429 rate limited' });
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
|
||||
assert.equal(d.files[p.errPath], 'HTTP 429 rate limited', 'stderr must reach the sidecar');
|
||||
assert.ok(
|
||||
d.files[p.reviewPath].includes('HTTP 429 rate limited'),
|
||||
'and must be surfaced in the stub, where a reader will actually see it',
|
||||
);
|
||||
assert.ok(p.reviewPath.endsWith('.md'), 'review output path unchanged');
|
||||
});
|
||||
}
|
||||
|
||||
test('a successful review passes through untouched', async () => {
|
||||
const p = planFor('gemini');
|
||||
const d = deps({ status: 0, stdout: 'Looks good.\n', stderr: '' });
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
|
||||
assert.equal(r.stubbed, false);
|
||||
assert.ok(d.files[p.reviewPath].includes('Looks good.'));
|
||||
assert.ok(
|
||||
!/failed or returned empty output/i.test(review),
|
||||
'the stub must not fire on a non-empty review (#2494)',
|
||||
!d.files[p.reviewPath].includes('failed or returned empty output'),
|
||||
'a real review must never carry the failure header',
|
||||
);
|
||||
});
|
||||
|
||||
test('no lane sends stderr to /dev/null — the sidecar is unconditional', async () => {
|
||||
// The original defect in one line, asserted over the whole roster rather than the two legs the
|
||||
// issue named: the policy is uniform now, and a future lane must not be able to opt out.
|
||||
for (const lane of STUB_LANES) {
|
||||
const p = planFor(lane.slug);
|
||||
const d = deps({ status: 0, stdout: 'ok', stderr: 'a warning' });
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(d.files[p.errPath], 'a warning', `${lane.slug} discarded stderr`);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,399 +1,147 @@
|
||||
// allow-test-rule: source-text-is-the-product (see #2605)
|
||||
// The lm_studio and llama_cpp reviewer dispatch blocks in gsd-core/workflows/review.md
|
||||
// ARE the runtime contract — the workflow's text is what the reviewing agent executes.
|
||||
// This suite extracts those two shell blocks verbatim from the workflow and runs them
|
||||
// under a real bash against a stubbed curl, so the shipped guard is what gets exercised
|
||||
// rather than a reimplementation of it. The assertions on the produced review file are
|
||||
// assertions on that guard's documented output contract (the stub line the consensus
|
||||
// step must be able to tell apart from a clean empty review), not incidental string
|
||||
// matching.
|
||||
|
||||
/**
|
||||
* Regression tests for #2605 — the lm_studio and llama_cpp reviewer legs dropped
|
||||
* empty output with no stub file, the same defect class as #2494 (claude/gemini)
|
||||
* but a worse variant.
|
||||
* #2605 — the local OpenAI-compatible lanes (ollama / lm_studio / llama.cpp) dropped silently.
|
||||
*
|
||||
* Before the fix both blocks ran `curl -s ... 2>/dev/null` and, when the model
|
||||
* returned empty content, wrote NOTHING to
|
||||
* `{run_dir}/gsd-review-<leg>.md` — there was no `[ ! -s … ]` stub at all. The
|
||||
* review file therefore never existed, `write_reviews` silently omitted that
|
||||
* reviewer's section, and the outcome was indistinguishable from the reviewer
|
||||
* never having been selected.
|
||||
* The original defects, all of which made a failed lane indistinguishable from a clean empty
|
||||
* review: bare `curl -s` suppressed curl's own error text; the response was piped straight into
|
||||
* `jq` so the BODY — where an OpenAI-compatible server puts its error JSON on an HTTP 4xx/5xx while
|
||||
* curl still exits 0 — was discarded unread; nothing was written when content was empty, so the
|
||||
* file never existed and `write_reviews` omitted the section entirely; and a whitespace-only reply
|
||||
* passed the byte-counting `[ ! -s … ]` guard as a successful review.
|
||||
*
|
||||
* Two distinct diagnostic holes are covered here, because an OpenAI-compatible
|
||||
* server can fail in two ways that leave evidence in different places:
|
||||
* - transport failure (endpoint unreachable): curl writes to STDERR and exits
|
||||
* non-zero. `-s` suppressed that error text entirely, so `-sS` is required.
|
||||
* - application failure (HTTP 4xx/5xx): curl exits 0 and the error JSON is in
|
||||
* the response BODY, so stderr is empty and only the body is diagnosable.
|
||||
* The llama_cpp leg additionally piped curl straight into jq, discarding the
|
||||
* body before anything could inspect it.
|
||||
*
|
||||
* These tests fail against pre-fix review.md: no review file is produced at all,
|
||||
* so the existence assertion trips first.
|
||||
* Phase 5b (#2799) replaced the curl/jq pipeline with an in-process HTTP call and `JSON.parse`, so
|
||||
* this suite drives the runner instead of extracting and executing shell. Two of the original
|
||||
* defects are now structurally impossible rather than merely guarded: there is no pipe to discard
|
||||
* the body, and no `echo` to swallow a `-n`-shaped reply.
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { describe, test, before, after } = require('node:test');
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const { spawnSync, execFileSync } = require('node:child_process');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { createTempDir, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const REVIEW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'review.md');
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const { runLane, runOpenAiCompatible } = require('../gsd-core/bin/lib/review-lane-runner.cjs');
|
||||
|
||||
// Normalize CRLF: on a Windows git-autocrlf checkout every line carries a
|
||||
// trailing \r, which would leave the extracted block's redirect tokens mangled
|
||||
// and defeat the fence regexes below.
|
||||
const WORKFLOW = fs.readFileSync(REVIEW_PATH, 'utf-8').replace(/\r\n/g, '\n');
|
||||
const RUN = '/run';
|
||||
const ROOT = '/repo';
|
||||
const HTTP_LANES = REVIEWER_LANES.filter((l) => l.transport === 'openai-http');
|
||||
|
||||
// These blocks shell out to a real `jq` (the request body is built with it).
|
||||
// Gate to jq-present non-Windows hosts, mirroring the opencode reconstruction
|
||||
// suite — the guard logic is platform-independent and is asserted in full on
|
||||
// every macOS/Linux CI leg.
|
||||
let jqAvailable = false;
|
||||
try {
|
||||
execFileSync('jq', ['--version'], { stdio: 'ignore', timeout: 10000, killSignal: 'SIGKILL' });
|
||||
jqAvailable = true;
|
||||
} catch { /* no jq on PATH */ }
|
||||
|
||||
const skipReason = process.platform === 'win32'
|
||||
? 'extracted block is POSIX shell; guard logic is platform-independent and asserted on macOS/Linux'
|
||||
: (jqAvailable ? false : 'jq not on PATH');
|
||||
|
||||
/**
|
||||
* Extract a reviewer dispatch block verbatim from the workflow. If review.md
|
||||
* changes the block's shape these throw and the test fails loudly — intended
|
||||
* coupling, the same contract the #2494 suite pins for the claude/gemini legs.
|
||||
*/
|
||||
function extractBlock(headingRe, label) {
|
||||
// Some legs (Ollama, CodeRabbit) put an explanatory paragraph between the
|
||||
// heading and the fence, so allow non-fence content in between. The lazy
|
||||
// quantifiers take the FIRST ```bash fence after the heading, which is that
|
||||
// leg's own block.
|
||||
const re = new RegExp(`${headingRe}\\n[\\s\\S]*?\`\`\`bash\\n([\\s\\S]*?)\\n\`\`\``);
|
||||
const m = WORKFLOW.match(re);
|
||||
assert.ok(m, `review.md must define the ${label} reviewer dispatch as a bash block (#2605)`);
|
||||
return m[1];
|
||||
function planFor(slug, config = {}) {
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === slug);
|
||||
const r = resolveLanePlan({ lane, configGet: (k) => config[k], runDir: RUN, repoRoot: ROOT });
|
||||
assert.equal(r.ok, true);
|
||||
return r.plan;
|
||||
}
|
||||
|
||||
const LEGS = [
|
||||
{
|
||||
key: 'lm_studio',
|
||||
label: 'LM Studio',
|
||||
budgetVar: 'LM_STUDIO_REVIEWER_BUDGET',
|
||||
block: extractBlock('\\*\\*LM Studio \\(local, OpenAI-compatible\\):\\*\\*', 'LM Studio'),
|
||||
},
|
||||
{
|
||||
key: 'llama_cpp',
|
||||
label: 'llama.cpp',
|
||||
budgetVar: 'LLAMA_CPP_REVIEWER_BUDGET',
|
||||
block: extractBlock('\\*\\*llama\\.cpp \\(local, OpenAI-compatible\\):\\*\\*', 'llama.cpp'),
|
||||
},
|
||||
// Ollama was the least diagnosable of the three local-server legs (bare `-s`,
|
||||
// stderr to /dev/null, response piped straight into jq). It emitted a stub, so
|
||||
// it never silently vanished — but it is the same family and is held to the
|
||||
// same contract here.
|
||||
{
|
||||
key: 'ollama',
|
||||
label: 'Ollama',
|
||||
budgetVar: 'OLLAMA_REVIEWER_BUDGET',
|
||||
block: extractBlock('\\*\\*Ollama \\(local, OpenAI-compatible\\):\\*\\*', 'Ollama'),
|
||||
},
|
||||
];
|
||||
|
||||
const STUB_STDERR = 'gsd-2605-stub: curl: (7) Failed to connect to localhost';
|
||||
const STUB_ERROR_BODY = '{"error":{"message":"gsd-2605-stub: model not loaded","code":503}}';
|
||||
|
||||
let sandbox;
|
||||
|
||||
before(() => { sandbox = createTempDir('gsd-2605-'); });
|
||||
after(() => { cleanup(sandbox); });
|
||||
|
||||
/**
|
||||
* Run one extracted block with `{run_dir}` pointed at a fresh run directory and
|
||||
* `curlBody` installed on PATH as `curl`.
|
||||
*
|
||||
* `gsd_run` is deliberately NOT stubbed: every call site is
|
||||
* `$(gsd_run … || echo "<default>")`, so an absent binary takes the documented
|
||||
* default path (no prompt budget, default host, model probed from the server) —
|
||||
* which is the configuration the issue reproduces against.
|
||||
*
|
||||
* Single exec site so the Windows guard lives in one place: Git Bash (msys2)
|
||||
* ignores Node's chmod exec bit for PATH-executed extension-less scripts
|
||||
* (DEFECT.WINDOWS-TEST-PORTABILITY), and every suite below is skipped on win32 —
|
||||
* this early return keeps the exec unreachable there rather than relying on the
|
||||
* skip alone.
|
||||
* These lanes declare an `http-reachable` probe, so the runner performs a GET on /v1/models BEFORE
|
||||
* the chat call. The stub must answer that separately — otherwise the lane is reported unreachable
|
||||
* and never reaches the invocation these tests are actually about.
|
||||
*/
|
||||
function runLeg({ leg, curlBody, preamble = '' }) {
|
||||
if (process.platform === 'win32') return null;
|
||||
if (!jqAvailable) return null;
|
||||
function reachableThen(chatResponse) {
|
||||
return async (url, opts) =>
|
||||
opts.method === 'GET'
|
||||
? { ok: true, status: 200, body: JSON.stringify({ data: [{ id: 'stub-model' }] }) }
|
||||
: (typeof chatResponse === 'function' ? chatResponse(url, opts) : chatResponse);
|
||||
}
|
||||
|
||||
const caseDir = fs.mkdtempSync(path.join(sandbox, 'run-'));
|
||||
const runDir = path.join(caseDir, 'run');
|
||||
const binDir = path.join(caseDir, 'bin');
|
||||
fs.mkdirSync(runDir);
|
||||
fs.mkdirSync(binDir);
|
||||
|
||||
const stub = path.join(binDir, 'curl');
|
||||
fs.writeFileSync(stub, curlBody);
|
||||
fs.chmodSync(stub, 0o755);
|
||||
|
||||
fs.writeFileSync(path.join(runDir, 'gsd-review-prompt.md'), '# review prompt\n');
|
||||
|
||||
const script = preamble + leg.block.split('{run_dir}').join(runDir);
|
||||
const result = spawnSync('bash', ['-c', script], {
|
||||
encoding: 'utf8',
|
||||
timeout: 30000,
|
||||
killSignal: 'SIGKILL',
|
||||
env: { ...process.env, PATH: `${binDir}${path.delimiter}${process.env.PATH}` },
|
||||
});
|
||||
|
||||
const reviewPath = path.join(runDir, `gsd-review-${leg.key}.md`);
|
||||
function deps(httpJson, files = { [`${RUN}/gsd-review-prompt.md`]: 'PLAN' }) {
|
||||
const warnings = [];
|
||||
return {
|
||||
result,
|
||||
reviewPath,
|
||||
errPath: path.join(runDir, `gsd-review-${leg.key}.err`),
|
||||
review: fs.existsSync(reviewPath) ? fs.readFileSync(reviewPath, 'utf-8') : null,
|
||||
files,
|
||||
warnings,
|
||||
spawn: () => ({ status: 0, stdout: '', stderr: '' }),
|
||||
httpJson,
|
||||
readFile: (p) => { if (!(p in files)) throw new Error(`ENOENT ${p}`); return files[p]; },
|
||||
writeFile: (p, c) => { files[p] = c; },
|
||||
exists: (p) => p in files,
|
||||
hasBinary: () => true,
|
||||
configGet: () => undefined,
|
||||
homeDir: '/home/u',
|
||||
warn: (m) => warnings.push(m),
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* A curl stub. The block may call curl twice — once to probe `/v1/models` for a
|
||||
* model id, once to POST `/v1/chat/completions` — so the stub branches on the URL
|
||||
* and only applies the failure mode to the completion call.
|
||||
*/
|
||||
function curlStub({ completionStdout = '', completionStderr = '', exitCode = 0 }) {
|
||||
return `#!/bin/sh
|
||||
for a in "$@"; do
|
||||
case "$a" in
|
||||
*/v1/models) echo '{"data":[{"id":"stub-model"}]}'; exit 0 ;;
|
||||
esac
|
||||
done
|
||||
${completionStderr ? `echo "${completionStderr}" >&2` : ':'}
|
||||
${completionStdout ? `cat <<'GSD_EOF'\n${completionStdout}\nGSD_EOF` : ':'}
|
||||
exit ${exitCode}
|
||||
`;
|
||||
}
|
||||
|
||||
/**
|
||||
* The guard's contract: the review file EXISTS (the #2605 core defect — it did
|
||||
* not), is not empty, and names the leg as failed-or-empty so consensus
|
||||
* synthesis can tell it apart from "ran cleanly, nothing to report".
|
||||
*/
|
||||
function assertDiagnosable(out, legLabel) {
|
||||
assert.ok(out.review !== null, `${legLabel}: review file must exist after a failed lane — its absence is what write_reviews silently omitted (#2605)`);
|
||||
assert.notStrictEqual(out.review.trim(), '', `${legLabel}: review file must not be empty after a failed lane (#2605)`);
|
||||
assert.match(
|
||||
out.review,
|
||||
new RegExp(`${legLabel.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')} review failed or returned empty output`, 'i'),
|
||||
`${legLabel}: review file must carry a diagnosable failure line (#2605)`,
|
||||
);
|
||||
}
|
||||
|
||||
for (const leg of LEGS) {
|
||||
describe(`#2605 — ${leg.label} reviewer leg fails loudly`, { skip: skipReason }, () => {
|
||||
test('an unreachable endpoint produces a diagnosable stub carrying curl stderr', () => {
|
||||
// Transport failure: curl writes to stderr and exits non-zero. `-s` alone
|
||||
// would have suppressed this text, which is why the fix uses `-sS`.
|
||||
const out = runLeg({
|
||||
leg,
|
||||
curlBody: curlStub({ completionStderr: STUB_STDERR, exitCode: 7 }),
|
||||
});
|
||||
|
||||
assertDiagnosable(out, leg.label);
|
||||
assert.ok(
|
||||
out.review.includes(STUB_STDERR),
|
||||
`${leg.label}: captured stderr must be appended to the review file, not discarded to /dev/null (#2605)`,
|
||||
);
|
||||
assert.ok(
|
||||
fs.existsSync(out.errPath),
|
||||
`${leg.label}: stderr must be captured to a .err sidecar (#2605)`,
|
||||
);
|
||||
});
|
||||
|
||||
test('an HTTP error body produces a diagnosable stub carrying the raw response', () => {
|
||||
// Application failure: an OpenAI-compatible server returns its error JSON in
|
||||
// the BODY and curl exits 0, so stderr is empty and only the body is
|
||||
// evidence. The llama_cpp leg used to pipe curl straight into jq, throwing
|
||||
// the body away before anything could inspect it.
|
||||
const out = runLeg({
|
||||
leg,
|
||||
curlBody: curlStub({ completionStdout: STUB_ERROR_BODY, exitCode: 0 }),
|
||||
});
|
||||
|
||||
assertDiagnosable(out, leg.label);
|
||||
assert.ok(
|
||||
out.review.includes('gsd-2605-stub: model not loaded'),
|
||||
`${leg.label}: the raw response body must be preserved — it is the only evidence when curl exits 0 (#2605)`,
|
||||
);
|
||||
});
|
||||
|
||||
test('an empty 200 response still produces a stub rather than no file', () => {
|
||||
// Boundary: a well-formed response whose content is the empty string. This
|
||||
// is the exact case the issue reproduces — the old code took the `else`
|
||||
// branch and wrote nothing at all.
|
||||
const out = runLeg({
|
||||
leg,
|
||||
curlBody: curlStub({
|
||||
completionStdout: '{"choices":[{"message":{"content":""}}]}',
|
||||
exitCode: 0,
|
||||
}),
|
||||
});
|
||||
|
||||
assertDiagnosable(out, leg.label);
|
||||
});
|
||||
|
||||
test('a whitespace-only response is treated as empty, not as a successful review', () => {
|
||||
// `[ ! -s … ]` counts BYTES, so a reply of " " was written out and passed
|
||||
// the guard as a "successful" but vacuous review — the same
|
||||
// indistinguishable-from-success outcome the guard exists to prevent.
|
||||
// Command substitution strips trailing newlines but NOT spaces, so this
|
||||
// case is not covered by the empty-string case above.
|
||||
const out = runLeg({
|
||||
leg,
|
||||
curlBody: curlStub({
|
||||
completionStdout: '{"choices":[{"message":{"content":" "}}]}',
|
||||
exitCode: 0,
|
||||
}),
|
||||
});
|
||||
|
||||
assertDiagnosable(out, leg.label);
|
||||
});
|
||||
|
||||
test('a reply that is exactly an echo option is not misclassified as empty', () => {
|
||||
// `echo "$VAR" > file` writes 0 bytes when VAR is exactly `-n`, which would
|
||||
// trip the empty guard and DISCARD a genuine reply. printf is required.
|
||||
const out = runLeg({
|
||||
leg,
|
||||
curlBody: curlStub({
|
||||
completionStdout: '{"choices":[{"message":{"content":"-n"}}]}',
|
||||
exitCode: 0,
|
||||
}),
|
||||
});
|
||||
|
||||
assert.ok(out.review !== null, `${leg.label}: a reply of "-n" must still produce a review file (#2605)`);
|
||||
assert.ok(
|
||||
out.review.includes('-n'),
|
||||
`${leg.label}: a reply of "-n" must be written verbatim, not swallowed by echo's option parsing (#2605)`,
|
||||
);
|
||||
assert.ok(
|
||||
!/failed or returned empty output/i.test(out.review),
|
||||
`${leg.label}: a genuine "-n" reply must not be misclassified as empty (#2605)`,
|
||||
);
|
||||
});
|
||||
|
||||
test('a budget skip leaves a visible stub rather than no file', () => {
|
||||
// The skip path sits one `if` away from the guard and dropped the lane just
|
||||
// as silently: no file, so write_reviews omitted the section entirely and
|
||||
// the only trace was a stderr warning nothing persists.
|
||||
const out = runLeg({
|
||||
leg,
|
||||
curlBody: curlStub({ completionStdout: '{"choices":[{"message":{"content":"unused"}}]}' }),
|
||||
// `gsd_run` supplies the budget, so stubbing the variable directly is
|
||||
// useless — the block's first line overwrites it. Drive the skip through
|
||||
// `gsd_run` itself: `config-get` yields a non-null budget so the trim
|
||||
// branch is entered, and `prompt-budget` returns 2 — the documented
|
||||
// "budget too small for the minimum review set" code. Stubbing only the
|
||||
// helper would not work for the Ollama leg, whose fence also carries the
|
||||
// shared `prepare_trimmed_prompt_for_reviewer` definition and so
|
||||
// overrides any stub of it.
|
||||
preamble: 'gsd_run() { case "$2" in prompt-budget) return 2 ;; esac; echo 1; }\n'
|
||||
+ 'prepare_trimmed_prompt_for_reviewer() { return 2; }\n',
|
||||
});
|
||||
|
||||
assert.ok(out.review !== null, `${leg.label}: a budget-skipped lane must still produce a review file (#2605)`);
|
||||
assert.match(
|
||||
out.review,
|
||||
/review skipped: prompt budget/i,
|
||||
`${leg.label}: a budget-skipped lane must say so in the review file (#2605)`,
|
||||
);
|
||||
});
|
||||
|
||||
test('a successful review passes through untouched', () => {
|
||||
// The guard must not fire on real content, and must not wrap or annotate it.
|
||||
const out = runLeg({
|
||||
leg,
|
||||
curlBody: curlStub({
|
||||
completionStdout: '{"choices":[{"message":{"content":"## Real Review\\nLooks good."}}]}',
|
||||
exitCode: 0,
|
||||
}),
|
||||
});
|
||||
|
||||
assert.ok(out.review !== null, `${leg.label}: a successful review must produce a review file (#2605)`);
|
||||
assert.ok(
|
||||
out.review.includes('Looks good.'),
|
||||
`${leg.label}: a successful review must pass through untouched (#2605)`,
|
||||
);
|
||||
assert.ok(
|
||||
!/failed or returned empty output/i.test(out.review),
|
||||
`${leg.label}: the stub must not fire on a non-empty review (#2605)`,
|
||||
);
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
describe('#2605 — the CodeRabbit leg fails loudly', { skip: skipReason }, () => {
|
||||
// CodeRabbit was the last CLI leg still shaped like pre-#2494 code:
|
||||
// `2>/dev/null > file` with no stub. A missing or unauthenticated binary left a
|
||||
// zero-byte file that write_reviews rendered as "ran cleanly, nothing to report".
|
||||
const CODERABBIT_BLOCK = extractBlock('\\*\\*CodeRabbit:\\*\\*', 'CodeRabbit');
|
||||
|
||||
test('a failing coderabbit CLI produces a diagnosable stub, not a zero-byte file', () => {
|
||||
if (process.platform === 'win32') return;
|
||||
|
||||
const caseDir = fs.mkdtempSync(path.join(sandbox, 'cr-'));
|
||||
const runDir = path.join(caseDir, 'run');
|
||||
const binDir = path.join(caseDir, 'bin');
|
||||
fs.mkdirSync(runDir);
|
||||
fs.mkdirSync(binDir);
|
||||
|
||||
const stub = path.join(binDir, 'coderabbit');
|
||||
fs.writeFileSync(stub, `#!/bin/sh\necho "${STUB_STDERR}" >&2\nexit 127\n`);
|
||||
fs.chmodSync(stub, 0o755);
|
||||
|
||||
const script = CODERABBIT_BLOCK.split('{run_dir}').join(runDir);
|
||||
spawnSync('bash', ['-c', script], {
|
||||
encoding: 'utf8',
|
||||
timeout: 30000,
|
||||
killSignal: 'SIGKILL',
|
||||
env: { ...process.env, PATH: `${binDir}${path.delimiter}${process.env.PATH}` },
|
||||
});
|
||||
|
||||
const reviewPath = path.join(runDir, 'gsd-review-coderabbit.md');
|
||||
assert.ok(fs.existsSync(reviewPath), 'CodeRabbit: review file must exist after a failed lane (#2605)');
|
||||
const review = fs.readFileSync(reviewPath, 'utf-8');
|
||||
assert.notStrictEqual(review.trim(), '', 'CodeRabbit: review file must not be zero-byte after a failed lane (#2605)');
|
||||
assert.match(
|
||||
review,
|
||||
/CodeRabbit review failed or returned empty output/i,
|
||||
'CodeRabbit: review file must carry a diagnosable failure line (#2605)',
|
||||
);
|
||||
assert.ok(
|
||||
review.includes(STUB_STDERR),
|
||||
'CodeRabbit: captured stderr must be appended, not discarded to /dev/null (#2605)',
|
||||
);
|
||||
});
|
||||
const okBody = (content) => ({
|
||||
ok: true, status: 200, body: JSON.stringify({ choices: [{ message: { content } }] }),
|
||||
});
|
||||
|
||||
describe('#2605 — every local-server leg uses -sS so curl errors are not suppressed', { skip: skipReason }, () => {
|
||||
test('the chat/completions call captures stderr to a sidecar instead of /dev/null', () => {
|
||||
// Pins the two changes that make the stub's evidence real rather than empty.
|
||||
// Asserted on the extracted block text because the redirect target is the
|
||||
// contract; the behavioural consequence is covered by the suites above.
|
||||
for (const leg of LEGS) {
|
||||
assert.match(
|
||||
leg.block,
|
||||
new RegExp(`curl -sS[^\\n]*\\n[^\\n]*-d @- 2>[^\\n]*gsd-review-${leg.key}\\.err`),
|
||||
`${leg.label}: the completion call must use -sS and redirect stderr to its .err sidecar (#2605)`,
|
||||
);
|
||||
assert.ok(
|
||||
!new RegExp(`curl -s --max-time 120`).test(leg.block),
|
||||
`${leg.label}: the completion call must not use bare -s, which suppresses curl's error text (#2605)`,
|
||||
);
|
||||
describe('#2605 local OpenAI-compatible lanes produce diagnosable output', () => {
|
||||
for (const lane of HTTP_LANES) {
|
||||
test(`${lane.slug}: an unreachable endpoint produces a stub carrying the transport error`, async () => {
|
||||
const p = planFor(lane.slug);
|
||||
const d = deps(reachableThen({ ok: false, status: 0, body: '', error: 'ECONNREFUSED' }));
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(r.stubbed, true);
|
||||
assert.ok(d.files[p.reviewPath].includes('ECONNREFUSED'),
|
||||
'the transport error must be visible — bare `curl -s` used to swallow it');
|
||||
});
|
||||
|
||||
test(`${lane.slug}: an HTTP error body is preserved in the stub`, async () => {
|
||||
// The body is the ONLY evidence on a 4xx/5xx: such a server returns its error JSON there and
|
||||
// curl still exits 0, so stderr is empty. The old pipe into jq discarded it.
|
||||
const p = planFor(lane.slug);
|
||||
const d = deps(reachableThen({ ok: false, status: 404, body: '{"error":"model not found"}' }));
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.ok(d.files[p.reviewPath].includes('Raw response body:'));
|
||||
assert.ok(d.files[p.reviewPath].includes('model not found'));
|
||||
});
|
||||
|
||||
test(`${lane.slug}: an empty 200 response still produces a file`, async () => {
|
||||
// Previously nothing was written, so the file never existed, write_reviews omitted the
|
||||
// section, and the result was indistinguishable from the reviewer never being selected.
|
||||
const p = planFor(lane.slug);
|
||||
const d = deps(reachableThen(okBody('')));
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(r.stubbed, true);
|
||||
assert.ok(d.files[p.reviewPath] !== undefined, 'a file must exist even on an empty reply');
|
||||
assert.ok(d.files[p.reviewPath].includes('failed or returned empty output'));
|
||||
});
|
||||
|
||||
test(`${lane.slug}: a whitespace-only response is empty, not a successful review`, async () => {
|
||||
// `[ ! -s … ]` counted BYTES, so " " passed as a real review.
|
||||
const p = planFor(lane.slug);
|
||||
const d = deps(reachableThen(okBody(' \n\t ')));
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(r.stubbed, true);
|
||||
});
|
||||
|
||||
test(`${lane.slug}: a reply that is exactly an echo option is NOT misclassified`, async () => {
|
||||
// `echo "$VAR"` would write 0 bytes for `-n`/`-e`/`-E`. Nothing here goes through echo, so
|
||||
// this is structurally impossible now — locked anyway.
|
||||
const p = planFor(lane.slug);
|
||||
const d = deps(reachableThen(okBody('-n')));
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(r.stubbed, false);
|
||||
assert.ok(d.files[p.reviewPath].includes('-n'));
|
||||
assert.ok(!d.files[p.reviewPath].includes('failed or returned empty output'));
|
||||
});
|
||||
|
||||
test(`${lane.slug}: a successful review passes through untouched`, async () => {
|
||||
const p = planFor(lane.slug);
|
||||
const d = deps(reachableThen(okBody('## Findings\nreal review')));
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(r.stubbed, false);
|
||||
assert.ok(d.files[p.reviewPath].includes('## Findings'));
|
||||
assert.ok(!d.files[p.reviewPath].includes('failed or returned empty output'));
|
||||
});
|
||||
}
|
||||
|
||||
test('a served-model mismatch is warned about, not silently accepted', async () => {
|
||||
const p = planFor('lm_studio', { 'review.models.lm_studio': 'asked-for' });
|
||||
const d = deps(async () => ({
|
||||
ok: true, status: 200,
|
||||
body: JSON.stringify({ model: 'actually-served', choices: [{ message: { content: 'R' } }] }),
|
||||
}));
|
||||
const out = await runOpenAiCompatible(p, 'PLAN', d);
|
||||
assert.equal(out.review, 'R');
|
||||
assert.ok(d.warnings.some((w) => w.includes('actually-served') && w.includes('asked-for')));
|
||||
});
|
||||
|
||||
test('neither jq nor curl is required by any of these lanes', () => {
|
||||
// The dependency is gone, not merely satisfied: parsing is JSON.parse and the request is
|
||||
// in-process. `jq` is absent on stock Windows/Git-Bash (#2589), which gated these lanes.
|
||||
for (const lane of HTTP_LANES) {
|
||||
assert.deepStrictEqual([...lane.requiresBinaries], [], `${lane.slug} still declares a binary`);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,166 +1,93 @@
|
||||
// allow-test-rule: source-text-is-the-product (see #2794)
|
||||
// The qwen reviewer dispatch block in gsd-core/workflows/review.md IS the runtime
|
||||
// contract — the workflow's text is what the reviewing agent executes. This suite
|
||||
// extracts that shell block verbatim and runs it under a real bash against a stubbed
|
||||
// `qwen`, so the shipped guard is what gets exercised rather than a reimplementation
|
||||
// of it. Assertions are on the guard's documented output contract (the stub line the
|
||||
// consensus step must be able to tell apart from a clean empty review, and the
|
||||
// presence of the captured stderr), not incidental string matching.
|
||||
|
||||
/**
|
||||
* Regression tests for #2794 — the qwen reviewer leg was the last one still
|
||||
* sending stderr to /dev/null.
|
||||
* #2794 — the qwen reviewer leg was the last one still sending stderr to /dev/null.
|
||||
*
|
||||
* Every other lane captures stderr to a `.err` sidecar and appends it to the
|
||||
* empty-output stub (#2494 for claude/gemini, #2605 for the local servers and
|
||||
* CodeRabbit). qwen alone ran `2>/dev/null` and wrote a bare
|
||||
* "Qwen review failed or returned empty output." with no diagnostic, so a missing
|
||||
* binary, an auth prompt, a rate-limit, and a genuinely empty review were all
|
||||
* indistinguishable — the same cross-cutting defect landing per-leg that ADR-2782
|
||||
* exists to end.
|
||||
* Every other lane captured stderr to a `.err` sidecar and appended it to the stub (#2494/#2605);
|
||||
* qwen wrote a bare "failed or returned empty output." with no diagnostic at all, so a missing
|
||||
* binary, an auth prompt and a rate-limit were indistinguishable from each other AND from a clean
|
||||
* empty review.
|
||||
*
|
||||
* These tests fail against pre-fix review.md: the stub carries no stderr, so the
|
||||
* diagnostic assertion in E2 trips.
|
||||
* Phase 5b (#2799) deleted the per-CLI bash this suite used to extract and run under a real bash.
|
||||
* The invariant is now structural rather than per-leg — the sidecar is written by the shared runner
|
||||
* for every lane — so these tests drive the runner directly. That also means the defect this issue
|
||||
* describes can no longer recur for ONE lane: there is no longer a per-lane place to get it wrong.
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { describe, test, before, after } = require('node:test');
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const { spawnSync } = require('node:child_process');
|
||||
const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
const { createTempDir, cleanup } = require('./helpers.cjs');
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const REVIEW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'review.md');
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const { runLane } = require('../gsd-core/bin/lib/review-lane-runner.cjs');
|
||||
|
||||
// Normalize CRLF: on a Windows autocrlf checkout every line carries a trailing
|
||||
// \r, which would mangle the extracted block's redirect tokens and defeat the
|
||||
// fence regex below.
|
||||
const WORKFLOW = fs.readFileSync(REVIEW_PATH, 'utf-8').replace(/\r\n/g, '\n');
|
||||
const RUN = '/run';
|
||||
const ROOT = '/repo';
|
||||
|
||||
/**
|
||||
* Extract the qwen dispatch block verbatim. If review.md changes the block's
|
||||
* shape this throws and the test fails loudly — intended coupling, the same
|
||||
* contract the #2494 and #2605 suites pin for their legs.
|
||||
*/
|
||||
function extractQwenBlock() {
|
||||
// `\r?\n` throughout: WORKFLOW is CRLF-normalized above, but the anchors stay
|
||||
// CRLF-tolerant so the regex cannot silently miss on a Windows checkout.
|
||||
const m = WORKFLOW.match(
|
||||
/<!--\s*reviewer-lane:\s*qwen\s*-->\r?\n[\s\S]*?```bash\r?\n([\s\S]*?)\r?\n```/,
|
||||
);
|
||||
assert.ok(m, 'review.md must define the qwen reviewer dispatch as a bash block (#2794)');
|
||||
return m[1];
|
||||
function planFor(slug) {
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === slug);
|
||||
const r = resolveLanePlan({ lane, configGet: () => undefined, runDir: RUN, repoRoot: ROOT });
|
||||
assert.equal(r.ok, true);
|
||||
return r.plan;
|
||||
}
|
||||
|
||||
const QWEN_BLOCK = extractQwenBlock();
|
||||
|
||||
// The extracted block is POSIX shell. Git Bash on Windows ignores Node's chmod
|
||||
// exec bit for PATH-executed extension-less scripts, so the stub would never run
|
||||
// there; the guard logic is platform-independent and is asserted in full on every
|
||||
// macOS/Linux leg.
|
||||
const WIN32_SKIP =
|
||||
'extracted block is POSIX shell; guard logic is platform-independent and asserted on macOS/Linux';
|
||||
|
||||
let sandbox;
|
||||
|
||||
before(() => { sandbox = createTempDir('gsd-2794-'); });
|
||||
after(() => { cleanup(sandbox); });
|
||||
|
||||
/**
|
||||
* Run the extracted block with `{run_dir}` pointed at a fresh run directory and
|
||||
* `qwenBody` installed on PATH as `qwen`. Pass `qwenBody: null` to run with no
|
||||
* qwen on PATH at all.
|
||||
*/
|
||||
function runQwenLeg(qwenBody) {
|
||||
// Single exec site so the Windows guard lives in one place: Git Bash (msys2)
|
||||
// ignores Node's chmod exec bit for PATH-executed extension-less scripts
|
||||
// (DEFECT.WINDOWS-TEST-PORTABILITY). Every test below is skipped on win32 —
|
||||
// this early return keeps the exec unreachable there rather than relying on
|
||||
// the skip alone.
|
||||
if (process.platform === 'win32') return null;
|
||||
|
||||
const caseDir = fs.mkdtempSync(path.join(sandbox, 'run-'));
|
||||
const runDir = path.join(caseDir, 'run');
|
||||
const binDir = path.join(caseDir, 'bin');
|
||||
fs.mkdirSync(runDir);
|
||||
fs.mkdirSync(binDir);
|
||||
|
||||
if (qwenBody !== null) {
|
||||
const stub = path.join(binDir, 'qwen');
|
||||
fs.writeFileSync(stub, qwenBody);
|
||||
fs.chmodSync(stub, 0o755);
|
||||
}
|
||||
|
||||
fs.writeFileSync(path.join(runDir, 'gsd-review-prompt.md'), '# review prompt\n');
|
||||
|
||||
const script = QWEN_BLOCK.split('{run_dir}').join(runDir);
|
||||
const result = spawnSync('bash', ['-c', script], {
|
||||
encoding: 'utf8',
|
||||
timeout: 30000,
|
||||
killSignal: 'SIGKILL',
|
||||
// An empty PATH would also break `cat`; prepend the stub dir instead so the
|
||||
// no-qwen case still resolves the shell builtins the block needs.
|
||||
env: { ...process.env, PATH: `${binDir}${path.delimiter}${process.env.PATH}` },
|
||||
});
|
||||
|
||||
const reviewPath = path.join(runDir, 'gsd-review-qwen.md');
|
||||
function deps(spawnResult, files = {}, hasBinary = () => true) {
|
||||
return {
|
||||
result,
|
||||
reviewPath,
|
||||
errPath: path.join(runDir, 'gsd-review-qwen.err'),
|
||||
review: fs.existsSync(reviewPath) ? fs.readFileSync(reviewPath, 'utf-8') : null,
|
||||
files,
|
||||
spawn: () => spawnResult,
|
||||
httpJson: async () => ({ ok: true, status: 200, body: '{}' }),
|
||||
readFile: (p) => { if (!(p in files)) throw new Error(`ENOENT ${p}`); return files[p]; },
|
||||
writeFile: (p, c) => { files[p] = c; },
|
||||
exists: (p) => p in files,
|
||||
hasBinary,
|
||||
configGet: () => undefined,
|
||||
homeDir: '/home/u',
|
||||
warn: () => {},
|
||||
};
|
||||
}
|
||||
|
||||
const STUB_STDERR = 'gsd-2794-stub: qwen: authentication required';
|
||||
|
||||
describe('qwen reviewer leg — empty-output guard (#2794)', () => {
|
||||
test('writes the review on success', { skip: process.platform === 'win32' ? WIN32_SKIP : false }, () => {
|
||||
const out = runQwenLeg('#!/bin/sh\necho "## Qwen findings"\nexit 0\n');
|
||||
assert.ok(out.review !== null, 'review file must exist');
|
||||
assert.match(out.review, /Qwen findings/);
|
||||
// A successful lane must NOT be decorated with the failure stub.
|
||||
assert.doesNotMatch(out.review, /failed or returned empty output/i);
|
||||
describe('#2794 qwen reviewer stderr capture', () => {
|
||||
test('writes the review on success', async () => {
|
||||
const p = planFor('qwen');
|
||||
const d = deps({ status: 0, stdout: '## Qwen findings\nall good\n', stderr: '' });
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(r.stubbed, false);
|
||||
assert.ok(d.files[p.reviewPath].includes('## Qwen findings'));
|
||||
});
|
||||
|
||||
test('a failed lane surfaces its stderr in the review stub', { skip: process.platform === 'win32' ? WIN32_SKIP : false }, () => {
|
||||
// THE regression row. Pre-fix the block ran `2>/dev/null`, so this stderr
|
||||
// was discarded and the stub carried no diagnostic at all.
|
||||
const out = runQwenLeg(`#!/bin/sh\necho "${STUB_STDERR}" >&2\nexit 1\n`);
|
||||
assert.ok(out.review !== null, 'review file must exist after a failed lane');
|
||||
assert.notStrictEqual(out.review.trim(), '', 'review file must not be empty');
|
||||
assert.match(
|
||||
out.review,
|
||||
/Qwen review failed or returned empty output/i,
|
||||
'stub must name the lane as failed-or-empty so consensus can tell it apart',
|
||||
);
|
||||
assert.ok(
|
||||
out.review.includes(STUB_STDERR),
|
||||
`stub must carry the captured stderr; got: ${JSON.stringify(out.review)}`,
|
||||
);
|
||||
assert.ok(fs.existsSync(out.errPath), 'stderr sidecar must be written');
|
||||
test('a failed lane surfaces its stderr in the review stub', async () => {
|
||||
const p = planFor('qwen');
|
||||
const d = deps({ status: 1, stdout: '', stderr: 'auth required: run `qwen login`' });
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.ok(d.files[p.reviewPath].includes('auth required'),
|
||||
'the diagnostic must reach the review, not just the sidecar');
|
||||
assert.equal(d.files[p.errPath], 'auth required: run `qwen login`');
|
||||
});
|
||||
|
||||
test('a silently empty lane still produces a diagnosable stub', { skip: process.platform === 'win32' ? WIN32_SKIP : false }, () => {
|
||||
// Boundary: nothing on either stream. The stub line is the only signal, and
|
||||
// it must still be present so write_reviews does not render the lane as a
|
||||
// reviewer that ran cleanly with nothing to report.
|
||||
const out = runQwenLeg('#!/bin/sh\nexit 0\n');
|
||||
assert.ok(out.review !== null, 'review file must exist');
|
||||
assert.notStrictEqual(out.review.trim(), '', 'review file must not be zero-byte');
|
||||
assert.match(out.review, /Qwen review failed or returned empty output/i);
|
||||
test('a silently empty lane still produces a diagnosable stub', async () => {
|
||||
const p = planFor('qwen');
|
||||
const d = deps({ status: 0, stdout: '', stderr: '' });
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(r.stubbed, true);
|
||||
assert.ok(d.files[p.reviewPath].includes('failed or returned empty output'));
|
||||
});
|
||||
|
||||
test('a missing qwen binary produces a diagnosable stub', { skip: process.platform === 'win32' ? WIN32_SKIP : false }, () => {
|
||||
const out = runQwenLeg(null);
|
||||
assert.ok(out.review !== null, 'review file must exist when the binary is absent');
|
||||
assert.notStrictEqual(out.review.trim(), '', 'review file must not be zero-byte');
|
||||
assert.match(out.review, /Qwen review failed or returned empty output/i);
|
||||
// The shell's own "command not found" lands in the sidecar and is appended,
|
||||
// which is the whole point of capturing instead of discarding.
|
||||
assert.ok(fs.existsSync(out.errPath), 'stderr sidecar must be written');
|
||||
test('a missing qwen binary reports unavailable rather than an empty review', async () => {
|
||||
// Stronger than the original: the lane is now reported with a TYPED reason before it is ever
|
||||
// spawned, instead of producing a stub that looked the same as every other failure.
|
||||
const p = planFor('qwen');
|
||||
const d = deps({ status: 0, stdout: '', stderr: '' }, {}, () => false);
|
||||
const r = await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(r.ok, false);
|
||||
assert.equal(r.reason, 'missing_binary');
|
||||
assert.equal(d.files[p.reviewPath], undefined, 'no review file for a lane that never ran');
|
||||
});
|
||||
|
||||
test('the sidecar is structural — no lane can opt out of it', async () => {
|
||||
// The #2794 defect was one lane diverging from a convention every other lane followed. There is
|
||||
// no per-lane place to diverge any more; this asserts that directly.
|
||||
for (const lane of REVIEWER_LANES.filter((l) => l.transport === 'spawn')) {
|
||||
const p = planFor(lane.slug);
|
||||
assert.ok(p.errPath.endsWith('.err'), `${lane.slug} must declare a stderr sidecar`);
|
||||
assert.notEqual(p.errPath, '/dev/null');
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,224 +1,125 @@
|
||||
// allow-test-rule: source-text-is-the-product (see #1936)
|
||||
// The OpenCode reviewer reconstructs its review from opencode's --format json
|
||||
// event stream using two embedded jq programs in gsd-core/workflows/review.md.
|
||||
// Those programs ARE the runtime contract; this test extracts them verbatim from
|
||||
// the workflow and exercises the real jq (not a reimplementation) so the shipped
|
||||
// reconstruction logic is what gets property-tested.
|
||||
//
|
||||
// ARCHITECTURE (#2099): the shipped jq program is run over a WHOLE fast-check corpus
|
||||
// in ONE jq process, not once per generated case. Each generated event stream is
|
||||
// written as one compact-JSON array per line to a temp file, then `jq -c <PROGRAM>`
|
||||
// (no `-s`) applies PROGRAM to each array — `.` is that array, exactly what
|
||||
// production's `jq -rs` sees after slurping opencode's one-value-per-line stream —
|
||||
// emitting one compact-JSON result per line. This is empirically identical to the
|
||||
// per-stream `-rs` form (verified across embedded-newline/empty/quote/unicode/
|
||||
// null-drop cases) AND reads from a file like production (`jq -rs '…' <file>`), so
|
||||
// there is no stdin pipe to deadlock on large I/O. The prior design spawned ~600
|
||||
// synchronous jq subprocesses (numRuns × 3 properties); a single one freezing on a
|
||||
// contended CI runner hung the whole unit-test chunk to its 600s kill (macOS CI,
|
||||
// #2099). `node --test`'s --test-force-exit cannot interrupt a synchronous
|
||||
// execFileSync, so the cure is to stop spawning per case — not just to time-bound it.
|
||||
/**
|
||||
* OpenCode review reconstruction — property tests (#1936).
|
||||
*
|
||||
* The OpenCode lane rebuilds its review from the assistant `text` parts of a `--format json` event
|
||||
* stream. `--format json` is the PRIMARY invocation, not a fallback: the default formatter drops
|
||||
* the assistant text when the agent ends its turn with no final message, silently losing the
|
||||
* reviewer.
|
||||
*
|
||||
* This suite used to extract two embedded `jq` programs from `review.md` and exercise the real jq
|
||||
* so the shipped logic was what got property-tested. Phase 5b (#2799) replaced those programs with
|
||||
* a named first-party handler in JavaScript, so the property now runs directly against the shipped
|
||||
* function.
|
||||
*
|
||||
* That deletes the entire #2099 problem this file was architected around. The old design spawned
|
||||
* ~600 synchronous jq subprocesses (numRuns × 3 properties), and one freezing on a contended CI
|
||||
* runner hung the whole unit-test chunk to its 600s kill — `--test-force-exit` cannot interrupt a
|
||||
* synchronous `execFileSync`. The batching workaround (one jq process over a whole corpus) existed
|
||||
* solely to avoid that. There is now no subprocess at all, so the hazard is gone by construction
|
||||
* rather than mitigated.
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const { execFileSync } = require('node:child_process');
|
||||
const fs = require('node:fs');
|
||||
const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
const fc = require('./helpers/fast-check-setup.cjs');
|
||||
const { cleanup } = require('./helpers.cjs');
|
||||
const fc = require('fast-check');
|
||||
|
||||
const reviewPath = path.resolve(__dirname, '..', 'gsd-core', 'workflows', 'review.md');
|
||||
const workflow = fs.readFileSync(reviewPath, 'utf-8');
|
||||
const { handleOpencodeOutput } = require('../gsd-core/bin/lib/review-lane-runner.cjs');
|
||||
|
||||
// Extract the two shipped jq programs verbatim. If review.md changes their shape,
|
||||
// these throw and the test fails loudly (intended coupling — #1936).
|
||||
function extractJqProgram(varName) {
|
||||
const re = new RegExp(`${varName}=\\$\\(jq -rs '([^']*)'`);
|
||||
const m = workflow.match(re);
|
||||
assert.ok(m, `review.md must define ${varName} via jq -rs '<program>' (#1936)`);
|
||||
return m[1];
|
||||
}
|
||||
const TEXT_PROGRAM = extractJqProgram('OPENCODE_REVIEW'); // review reconstruction
|
||||
const DIAG_PROGRAM = extractJqProgram('OPENCODE_DIAG'); // empty-output diagnostic
|
||||
/** Deterministic: pinned seed, bounded runs, replay data printed on failure. */
|
||||
const FC = { seed: 42, numRuns: 200 };
|
||||
|
||||
// This suite shells out to `jq`. On Windows, Node's child_process argument quoting
|
||||
// mangles the jq program (it embeds quotes) — jq then raises a parse error — and
|
||||
// jq isn't guaranteed on the host regardless. The reconstruction logic is
|
||||
// platform-independent (the review workflow runs jq in its Unix-y runtime), so gate
|
||||
// the suite to jq-present non-Windows hosts, mirroring golden-install-parity's win32
|
||||
// skip. The assertions run in full on every macOS/Linux CI leg.
|
||||
let jqAvailable = false;
|
||||
try { execFileSync('jq', ['--version'], { stdio: 'ignore', timeout: 10000, killSignal: 'SIGKILL' }); jqAvailable = true; } catch { /* no jq on PATH */ }
|
||||
const skipReason = process.platform === 'win32'
|
||||
? 'jq invocation is not portable under Node child_process arg-quoting on Windows; logic is platform-independent and asserted on macOS/Linux'
|
||||
: (jqAvailable ? false : 'jq not on PATH');
|
||||
const opts = { skip: skipReason };
|
||||
const NL = String.fromCharCode(10);
|
||||
|
||||
// Bound each jq subprocess so a frozen spawn on a contended runner fails fast +
|
||||
// diagnosably (ETIMEDOUT) rather than hanging the chunk. jq over this corpus
|
||||
// completes in ~10ms, so 30s is an enormous margin that never trips on a healthy run.
|
||||
const JQ_EXEC_OPTS = { encoding: 'utf8', timeout: 30000, killSignal: 'SIGKILL', maxBuffer: 64 * 1024 * 1024 };
|
||||
/** An assistant text event, as opencode emits it. */
|
||||
const textEvent = (text) => ({ type: 'text', part: { text } });
|
||||
/** A terminal step event carrying the stop reason and token counts. */
|
||||
const stepFinish = (reason, output) => ({ type: 'step_finish', part: { reason, tokens: { output } } });
|
||||
|
||||
// Run a shipped jq program over an array of event streams in a SINGLE jq process.
|
||||
// Returns one result per input stream (order preserved), decoded from jq's compact
|
||||
// (`-c`) JSON output back to the raw string production's `-r` would have captured.
|
||||
// Both shipped programs yield exactly one value per stream (join(...) / last|"...");
|
||||
// the length assertion pins that invariant so a future program change that broke it
|
||||
// (0 or >1 outputs) fails loudly instead of silently misaligning results.
|
||||
function runJqBatch(program, streams) {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-jq-batch-'));
|
||||
try {
|
||||
const file = path.join(dir, 'streams.jsonl');
|
||||
fs.writeFileSync(file, streams.map((events) => JSON.stringify(events)).join('\n') + '\n');
|
||||
const out = execFileSync('jq', ['-c', program, file], JQ_EXEC_OPTS);
|
||||
const lines = out.split('\n').filter((line) => line.length > 0);
|
||||
assert.equal(
|
||||
lines.length,
|
||||
streams.length,
|
||||
`jq must emit exactly one result per stream (got ${lines.length} for ${streams.length})`,
|
||||
);
|
||||
return lines.map((line) => JSON.parse(line));
|
||||
} finally {
|
||||
cleanup(dir);
|
||||
}
|
||||
}
|
||||
/** Serialize events the way opencode does: one compact JSON value per line. */
|
||||
const streamOf = (events) => events.map((e) => JSON.stringify(e)).join(NL);
|
||||
|
||||
// The intended reconstruction, computed independently of jq. jq is the unit under
|
||||
// test; this JS is the spec it must match on every generated case.
|
||||
function expectedReview(events) {
|
||||
return events
|
||||
.filter((e) => e.type === 'text' && e.part && typeof e.part.text === 'string')
|
||||
.map((e) => e.part.text)
|
||||
.join('\n');
|
||||
}
|
||||
|
||||
// Text values safe to round-trip through JSON → jq (utf8) → string. Excludes lone
|
||||
// surrogates (which don't survive utf8) but keeps the interesting cases: newlines,
|
||||
// quotes, backslashes, braces, unicode.
|
||||
const safeText = fc
|
||||
.string({ minLength: 0, maxLength: 40 })
|
||||
.filter((s) => Buffer.from(s, 'utf8').toString('utf8') === s);
|
||||
|
||||
// A `text` event whose `.part.text` is a string, or null/absent (dropped by `// empty`).
|
||||
const textEvent = fc.record({
|
||||
type: fc.constant('text'),
|
||||
part: fc.oneof(
|
||||
fc.record({ text: safeText }),
|
||||
fc.record({ text: fc.constant(null) }), // null → jq `// empty` drops it
|
||||
fc.record({}), // absent → jq `// empty` drops it
|
||||
),
|
||||
});
|
||||
const stepFinishEvent = fc.record({
|
||||
type: fc.constant('step_finish'),
|
||||
part: fc.record({
|
||||
reason: fc.constantFrom('stop', 'length', 'tool_calls'),
|
||||
tokens: fc.record({ output: fc.integer({ min: 0, max: 100000 }) }),
|
||||
}),
|
||||
});
|
||||
const nonTextEvent = fc.oneof(
|
||||
stepFinishEvent,
|
||||
fc.record({ type: fc.constant('tool_use'), part: fc.record({ tool: safeText }) }),
|
||||
fc.record({ type: fc.constant('step_start'), part: fc.record({}) }),
|
||||
/** Text that survives a JSON round-trip, including the shapes that broke the jq version. */
|
||||
const hostileText = fc.oneof(
|
||||
fc.string(),
|
||||
fc.constantFrom('', ' ', ' ', NL, 'a' + NL + 'b', '"quoted"', '\\backslash', 'emoji 🎉', 'null', '-n'),
|
||||
fc.string({ unit: 'grapheme' }),
|
||||
);
|
||||
// Weight text events higher so streams routinely mix real review text with noise,
|
||||
// but also generate text-free streams (the #1936 zero-output case).
|
||||
const eventStream = fc.array(fc.oneof(textEvent, textEvent, nonTextEvent), {
|
||||
minLength: 1,
|
||||
maxLength: 30,
|
||||
});
|
||||
|
||||
// Corpus size per property. Matches the prior fast-check-setup numRuns:200 so
|
||||
// coverage is unchanged; distinct seeds give each property an independent corpus,
|
||||
// and fixed seeds keep the corpus deterministic across CI runs/OS legs.
|
||||
const CORPUS = 200;
|
||||
|
||||
describe('#1936 OpenCode review reconstruction — jq properties', () => {
|
||||
test('review == the newline-join of every assistant text part, over a generated corpus (order preserved)', opts, () => {
|
||||
const streams = fc.sample(eventStream, { numRuns: CORPUS, seed: 42 });
|
||||
const actual = runJqBatch(TEXT_PROGRAM, streams); // one jq process for the whole corpus
|
||||
streams.forEach((events, i) => {
|
||||
assert.equal(
|
||||
actual[i],
|
||||
expectedReview(events),
|
||||
`case ${i}: shipped jq review must equal the spec for events=${JSON.stringify(events)}`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
test('a stream with no assistant text part reconstructs to empty (drives the #1936 stub), over a corpus', opts, () => {
|
||||
// The exact failure the bug describes: the agent runs tool calls and ends with
|
||||
// step_finish, emitting no text. Reconstruction must be empty so the content-gate
|
||||
// (`[ -n "$OPENCODE_REVIEW" ]`) falls through to the stub.
|
||||
const streams = fc.sample(fc.array(nonTextEvent, { minLength: 1, maxLength: 20 }), { numRuns: CORPUS, seed: 43 });
|
||||
const actual = runJqBatch(TEXT_PROGRAM, streams);
|
||||
streams.forEach((events, i) => {
|
||||
assert.equal(actual[i], '', `case ${i}: text-free stream must reconstruct to '' for ${JSON.stringify(events)}`);
|
||||
});
|
||||
});
|
||||
|
||||
test('text parts that are null/absent are dropped, never rendered as "null", over a corpus', opts, () => {
|
||||
const nullish = fc.array(
|
||||
fc.oneof(
|
||||
fc.record({ type: fc.constant('text'), part: fc.record({ text: fc.constant(null) }) }),
|
||||
fc.record({ type: fc.constant('text'), part: fc.record({}) }),
|
||||
),
|
||||
{ minLength: 1, maxLength: 10 },
|
||||
describe('opencode reconstruction — properties', () => {
|
||||
test('every assistant text part appears, in order, joined by newlines', () => {
|
||||
fc.assert(
|
||||
fc.property(fc.array(hostileText, { minLength: 1, maxLength: 8 }), (texts) => {
|
||||
const stream = streamOf(texts.map(textEvent));
|
||||
return handleOpencodeOutput(stream).review === texts.join(NL);
|
||||
}),
|
||||
FC,
|
||||
);
|
||||
const streams = fc.sample(nullish, { numRuns: CORPUS, seed: 44 });
|
||||
const actual = runJqBatch(TEXT_PROGRAM, streams);
|
||||
streams.forEach((events, i) => {
|
||||
assert.equal(actual[i], '', `case ${i}: null/absent text must drop to ''`);
|
||||
assert.doesNotMatch(actual[i], /null/, `case ${i}: must never render "null"`);
|
||||
});
|
||||
});
|
||||
|
||||
// Explicit boundary + happy examples (deterministic, not sampled) — all in one
|
||||
// batched jq spawn. Pins the exact contract the corpus only covers probabilistically.
|
||||
test('boundary + happy example streams reconstruct exactly (batched)', opts, () => {
|
||||
const cases = [
|
||||
{ events: [{ type: 'text', part: { text: 'only' } }], expect: 'only' }, // single text part
|
||||
{ events: [{ type: 'text', part: { text: '' } }], expect: '' }, // empty-string text is kept
|
||||
{ events: [{ type: 'text', part: { text: 'a' } }, { type: 'tool_use', part: { tool: 'r' } }, { type: 'text', part: { text: 'b' } }], expect: 'a\nb' }, // text interleaved with noise
|
||||
{ events: [{ type: 'text', part: { text: 'x\ny' } }], expect: 'x\ny' }, // embedded newline preserved
|
||||
{ events: [{ type: 'tool_use', part: { tool: 'read' } }, { type: 'step_finish', part: { reason: 'stop', tokens: { output: 3 } } }], expect: '' }, // no text at all
|
||||
{ events: Array.from({ length: 30 }, (_v, i) => ({ type: 'text', part: { text: `p${i}` } })), expect: Array.from({ length: 30 }, (_v, i) => `p${i}`).join('\n') }, // max-size all-text stream
|
||||
];
|
||||
const actual = runJqBatch(TEXT_PROGRAM, cases.map((c) => c.events));
|
||||
cases.forEach((c, i) => assert.equal(actual[i], c.expect, `example ${i}: ${JSON.stringify(c.events)}`));
|
||||
test('non-text events never contribute to the review', () => {
|
||||
fc.assert(
|
||||
fc.property(
|
||||
fc.array(hostileText, { minLength: 1, maxLength: 5 }),
|
||||
fc.array(fc.record({ reason: fc.string(), output: fc.nat() }), { maxLength: 4 }),
|
||||
(texts, steps) => {
|
||||
const events = [...texts.map(textEvent), ...steps.map((s) => stepFinish(s.reason, s.output))];
|
||||
return handleOpencodeOutput(streamOf(events)).review === texts.join(NL);
|
||||
},
|
||||
),
|
||||
FC,
|
||||
);
|
||||
});
|
||||
|
||||
// Diagnostic path (empty-output stub). The finding calls out `missing .tokens.output`
|
||||
// and no-step_finish as real edges — pin them with examples against the shipped jq,
|
||||
// all in one batched spawn.
|
||||
describe('diagnostic reconstruction (stop reason + output tokens)', () => {
|
||||
test('reports reason/tokens from the LAST step_finish and degrades missing fields to "?"', opts, () => {
|
||||
const cases = [
|
||||
{ events: [
|
||||
{ type: 'step_finish', part: { reason: 'tool_calls', tokens: { output: 5 } } },
|
||||
{ type: 'tool_use', part: {} },
|
||||
{ type: 'step_finish', part: { reason: 'stop', tokens: { output: 0 } } },
|
||||
], expect: 'stop reason=stop, output tokens=0' }, // LAST step_finish wins
|
||||
{ events: [{ type: 'step_finish', part: { reason: 'stop', tokens: {} } }], expect: 'stop reason=stop, output tokens=?' }, // missing .tokens.output → "?"
|
||||
{ events: [{ type: 'tool_use', part: { tool: 'read' } }], expect: 'stop reason=?, output tokens=?' }, // no step_finish at all → both "?"
|
||||
];
|
||||
const actual = runJqBatch(DIAG_PROGRAM, cases.map((c) => c.events));
|
||||
cases.forEach((c, i) => assert.equal(actual[i], c.expect, `diag ${i}: ${JSON.stringify(c.events)}`));
|
||||
});
|
||||
test('a malformed line is skipped without losing the surrounding review', () => {
|
||||
// Losing an entire review to one unparseable line would be strictly worse than the bug this
|
||||
// handler exists to fix, so a partial stream must still yield its usable text.
|
||||
fc.assert(
|
||||
fc.property(
|
||||
fc.array(hostileText, { minLength: 1, maxLength: 4 }),
|
||||
fc.constantFrom('NOT JSON', '{"truncated":', '}{', '[', 'null', ' '),
|
||||
(texts, garbage) => {
|
||||
const lines = [...texts.map((t) => JSON.stringify(textEvent(t))), garbage];
|
||||
return handleOpencodeOutput(lines.join(NL)).review === texts.join(NL);
|
||||
},
|
||||
),
|
||||
FC,
|
||||
);
|
||||
});
|
||||
|
||||
// The primary reconstruction runs before any content gate; on non-JSON stdout
|
||||
// (e.g. an opencode crash that printed a plain-text error) jq must fail rather
|
||||
// than emit that text as a "review" — the workflow's `2>/dev/null` + empty
|
||||
// capture then routes to the diagnostic stub.
|
||||
test('non-JSON stdout does not masquerade as a reconstructed review', opts, () => {
|
||||
let threw = false;
|
||||
try {
|
||||
execFileSync('jq', ['-rs', TEXT_PROGRAM], { ...JQ_EXEC_OPTS, input: 'auth token expired\n' });
|
||||
} catch {
|
||||
threw = true;
|
||||
}
|
||||
assert.ok(threw, 'jq must reject non-JSON input so it cannot be captured as a review');
|
||||
test('is total — never throws on arbitrary input', () => {
|
||||
fc.assert(
|
||||
fc.property(fc.anything(), (anything) => {
|
||||
try {
|
||||
const r = handleOpencodeOutput(anything);
|
||||
return typeof r.review === 'string' && typeof r.diagnostic === 'string';
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}),
|
||||
FC,
|
||||
);
|
||||
});
|
||||
|
||||
test('CRLF streams reconstruct identically to LF', () => {
|
||||
fc.assert(
|
||||
fc.property(fc.array(fc.string({ minLength: 1 }), { minLength: 1, maxLength: 5 }), (texts) => {
|
||||
const lf = streamOf(texts.map(textEvent));
|
||||
const crlf = lf.split(NL).join(String.fromCharCode(13) + NL);
|
||||
return handleOpencodeOutput(lf).review === handleOpencodeOutput(crlf).review;
|
||||
}),
|
||||
FC,
|
||||
);
|
||||
});
|
||||
|
||||
test('a stream with no assistant text yields an empty review and a diagnostic', () => {
|
||||
fc.assert(
|
||||
fc.property(fc.string({ minLength: 1 }), fc.nat(), (reason, output) => {
|
||||
const r = handleOpencodeOutput(streamOf([stepFinish(reason, output)]));
|
||||
return r.review === '' && r.diagnostic.includes(String(output));
|
||||
}),
|
||||
FC,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -171,71 +171,73 @@ const path = require('node:path');
|
||||
const reviewPath = path.resolve(__dirname, '..', 'gsd-core', 'workflows', 'review.md');
|
||||
const read = () => fs.readFileSync(reviewPath, 'utf-8');
|
||||
|
||||
describe('bug #687 → #2073: agy print mode bounded by --print-timeout PAIRED with an external timeout', () => {
|
||||
// #687 established that agy print mode must be bounded (its native
|
||||
// --print-timeout, default 5m). #2073 superseded the "no external killer"
|
||||
// half of that contract with documentation:
|
||||
// - agy's own print-mode guidance says to PAIR --print-timeout with an
|
||||
// external terminal `timeout` ("Pair with the terminal timeout= so the
|
||||
// outer call doesn't cut the run short"), because --print-timeout cannot
|
||||
// fire before agy creates a session (a pre-session stall otherwise hangs
|
||||
// unbounded). The external cap is set HIGHER than --print-timeout so it
|
||||
// only backstops a stall, never cuts a healthy run.
|
||||
// - agy gained `--model` in ~1.0.3 (issue #3782's "no --model flag" note
|
||||
// was correct at the time, stale now); review.models.agy is passed as
|
||||
// --model so a pinned model that 404s has an escape hatch.
|
||||
// - the prompt is now a file reference: inline `-p "$(cat …)"` overflows
|
||||
// the exec arg list on a large review prompt (Linux MAX_ARG_STRLEN
|
||||
// 128 KB/single-arg → rc 126).
|
||||
describe('bug #687 → #2073: agy print mode is bounded, and its fallback chain fires', () => {
|
||||
// Phase 5b (#2799) moved the agy invocation out of review.md's bash into the declared lane plus
|
||||
// the named `antigravity` handler, so these assertions follow it.
|
||||
//
|
||||
// ONE INVARIANT DELIBERATELY CHANGED. The old contract was "--print-timeout PAIRED with an
|
||||
// external timeout/gtimeout killer, falling back to bare agy on macOS". That fallback WAS the
|
||||
// bug: stock macOS ships neither killer, so the lane ran unbounded there — and --print-timeout
|
||||
// cannot fire before agy creates a session (#2073 mode 3), which is the whole reason a second
|
||||
// bound existed. spawnSync's native timeout is always available, so the outer bound is now
|
||||
// unconditional on every platform. Strictly stronger; only the mechanism changed.
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const { runLane } = require('../gsd-core/bin/lib/review-lane-runner.cjs');
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === 'antigravity');
|
||||
const planFor = () => {
|
||||
const r = resolveLanePlan({ lane, configGet: (k) => ({ 'review.models.agy': 'agy-m' })[k], runDir: '/run', repoRoot: '/repo' });
|
||||
assert.equal(r.ok, true);
|
||||
return r.plan;
|
||||
};
|
||||
|
||||
test('invokes agy with --print-timeout AND a paired external killer when available', () => {
|
||||
const c = read();
|
||||
assert.match(c, /--print-timeout \d+s?/, 'review.md must pass agy its native --print-timeout');
|
||||
// Capability probe for GNU `timeout` / macOS `gtimeout` (stock macOS has neither).
|
||||
assert.match(c, /command -v timeout/, 'review.md must probe for the `timeout` killer');
|
||||
assert.match(c, /command -v gtimeout/, 'review.md must probe for `gtimeout` (macOS Homebrew)');
|
||||
// The external cap (600s) is applied ahead of agy and is >= --print-timeout (540s).
|
||||
assert.match(c, /600 agy --print-timeout 540s/,
|
||||
'review.md must pair an external cap (600s) >= --print-timeout (540s) with agy (agy guidance)');
|
||||
test('keeps its native --print-timeout AND carries an unconditional outer bound', () => {
|
||||
const argv = planFor().argv;
|
||||
const i = argv.indexOf('--print-timeout');
|
||||
assert.notEqual(i, -1, 'the tool-native inner bound must survive');
|
||||
assert.match(argv[i + 1], /^\d+s$/);
|
||||
assert.ok(lane.timeoutFloorMs > 0, 'and an outer wall-clock bound must be declared');
|
||||
});
|
||||
|
||||
test('external cap is >= --print-timeout, and falls back to bare agy on macOS', () => {
|
||||
const c = read();
|
||||
const bound = c.match(/(\d+)\s+agy --print-timeout (\d+)s/);
|
||||
assert.ok(bound, 'review.md must encode the external-cap + --print-timeout pair');
|
||||
assert.ok(
|
||||
Number(bound[1]) >= Number(bound[2]),
|
||||
'external cap (seconds) must be >= --print-timeout (seconds) so it only backstops a stall',
|
||||
);
|
||||
// Graceful fallback when no external killer is available (stock macOS).
|
||||
assert.match(c, /else\n\s*agy --print-timeout/,
|
||||
'review.md must fall back to --print-timeout alone when no external killer is available (macOS)');
|
||||
test('the outer bound is larger than the inner one, so it only backstops', () => {
|
||||
const argv = planFor().argv;
|
||||
const innerSec = parseInt(argv[argv.indexOf('--print-timeout') + 1], 10);
|
||||
assert.ok(lane.timeoutFloorMs / 1000 > innerSec,
|
||||
'the outer cap must not pre-empt a healthy run bounded by --print-timeout');
|
||||
});
|
||||
|
||||
test('uses a file-reference prompt, not inline "$(cat …)" (arg-list overflow, #2073)', () => {
|
||||
const c = read();
|
||||
assert.doesNotMatch(c, /agy[^\n]*-p "\$\(cat/,
|
||||
'review.md must not feed agy the prompt inline via "$(cat …)" — a large review prompt overflows the exec arg list (rc 126)');
|
||||
assert.match(c, /Read the file at \{run_dir\}\/gsd-review-prompt\.md/,
|
||||
'review.md should pass agy a file-reference prompt (mirrors the Cursor block)');
|
||||
assert.equal(lane.invoke.promptChannel, 'argv-file-ref');
|
||||
const last = planFor().argv.slice(-1)[0];
|
||||
assert.ok(last.includes('/run/gsd-review-prompt.md'));
|
||||
assert.ok(last.length < 1000, 'the reference must never carry the prompt body');
|
||||
});
|
||||
|
||||
test('wires --model from review.models.agy (#2073 mode 2; agy gained --model in ~1.0.3)', () => {
|
||||
assert.match(read(), /--model "\$AGY_MODEL"/,
|
||||
'review.md must pass --model "$AGY_MODEL" when review.models.agy is set');
|
||||
test('wires --model from review.models.agy (#2073 mode 2)', () => {
|
||||
assert.equal(lane.modelConfigKey, 'review.models.agy');
|
||||
const argv = planFor().argv;
|
||||
assert.equal(argv[argv.indexOf('--model') + 1], 'agy-m');
|
||||
});
|
||||
|
||||
test('discards partial output on non-zero exit so the fallback fires (#687)', () => {
|
||||
const c = read();
|
||||
assert.match(c, /_AGY_RC.*-ne 0/, 'review.md must check the agy exit code');
|
||||
assert.match(c, /: > \{run_dir\}\/gsd-review-antigravity\.md/,
|
||||
'review.md must truncate the output file when agy timed out / failed');
|
||||
});
|
||||
|
||||
test('no unguarded bare "agy -p" invocation remains at line start', () => {
|
||||
// A bare `agy -p …` with no cap was the original #687 hang.
|
||||
assert.doesNotMatch(read(), /^agy -p/m,
|
||||
'review.md must not invoke a bare `agy -p` unbounded at line start');
|
||||
test('discards partial output on non-zero exit so the fallback fires (#687)', async () => {
|
||||
const p = planFor();
|
||||
const files = {};
|
||||
const d = {
|
||||
files,
|
||||
spawn: () => ({ status: 124, stdout: 'PARTIAL GARBAGE', stderr: '' }),
|
||||
httpJson: async () => ({ ok: true, status: 200, body: '{}' }),
|
||||
readFile: (x) => { if (!(x in files)) throw new Error('ENOENT'); return files[x]; },
|
||||
writeFile: (x, c) => { files[x] = c; },
|
||||
exists: (x) => x in files,
|
||||
hasBinary: () => true,
|
||||
configGet: () => undefined,
|
||||
homeDir: '/home/u',
|
||||
warn: () => {},
|
||||
};
|
||||
await runLane(p, d, { repoRoot: '/repo' });
|
||||
assert.ok(!files[p.reviewPath].includes('PARTIAL GARBAGE'),
|
||||
'a non-zero exit must discard stdout so the transcript fallback and diagnostic can take over');
|
||||
assert.ok(files[p.reviewPath].includes('failed or returned empty output'));
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -260,94 +262,29 @@ const fs = require('node:fs');
|
||||
const path = require('node:path');
|
||||
|
||||
describe('enh-773: automated codex exec invocations include --ephemeral (hook-trust bypass dropped by #2479)', () => {
|
||||
const workflow = fs.readFileSync(
|
||||
path.join(process.cwd(), 'gsd-core', 'workflows', 'review.md'),
|
||||
'utf8'
|
||||
);
|
||||
// Declared in the lane now rather than grepped out of review.md's bash lines.
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const codex = REVIEWER_LANES.find((l) => l.slug === 'codex');
|
||||
const args = codex.invoke.args.join(' ');
|
||||
|
||||
// Extract codex exec INVOCATION lines from code fences. A `codex exec --help`
|
||||
// capability probe would not be an automation invocation, so keep it excluded
|
||||
// from the per-invocation flag assertions below (#2479 asserts no such probe
|
||||
// exists at all).
|
||||
const codexExecLines = workflow
|
||||
.split(/\r?\n/)
|
||||
.filter((line) => line.includes('codex exec') && !line.includes('codex exec --help'));
|
||||
|
||||
test('review.md contains at least one codex exec invocation', () => {
|
||||
assert.ok(
|
||||
codexExecLines.length > 0,
|
||||
'review.md must contain at least one codex exec invocation'
|
||||
);
|
||||
test('the codex lane uses the exec subcommand', () => {
|
||||
assert.ok(args.includes('exec'));
|
||||
assert.equal(codex.invoke.args[0], 'exec', 'exec is a SUBCOMMAND and must come first');
|
||||
});
|
||||
|
||||
test('every codex exec invocation includes --ephemeral', () => {
|
||||
for (const line of codexExecLines) {
|
||||
assert.ok(
|
||||
line.includes('--ephemeral'),
|
||||
`codex exec invocation is missing --ephemeral:\n ${line.trim()}`
|
||||
);
|
||||
}
|
||||
test('codex exec is --ephemeral', () => {
|
||||
assert.ok(args.includes('--ephemeral'));
|
||||
});
|
||||
|
||||
test('#2479: no hook-trust bypass — flag and capability probe are both absent', () => {
|
||||
// The flag only bypasses *persisted* hook trust (a first-run condition) and
|
||||
// flagless invocations work in steady state, while host-harness safety
|
||||
// classifiers deny commands carrying it — and cited the probe itself as
|
||||
// intent. Inverts the former #1115 gating contract: instead of probe + gated
|
||||
// $CODEX_BYPASS_FLAG, the workflow must be free of the literal flag string
|
||||
// ANYWHERE in the file — not just on invocation lines — so a line
|
||||
// continuation, a renamed carrier variable, or a probe grepping for it are
|
||||
// all caught by the same assertion. (The #2479 prose note deliberately
|
||||
// describes the flag without spelling it out, keeping the file-wide ban
|
||||
// clean.)
|
||||
assert.ok(
|
||||
!workflow.includes('--dangerously-bypass-hook-trust'),
|
||||
'review.md must not contain --dangerously-bypass-hook-trust anywhere (#2479 — file-wide ban covers invocations, continuations, carrier variables, and probes)'
|
||||
);
|
||||
assert.ok(
|
||||
!workflow.includes('CODEX_BYPASS_FLAG'),
|
||||
'review.md must not interpolate a $CODEX_BYPASS_FLAG variable (#2479)'
|
||||
);
|
||||
assert.ok(
|
||||
!/codex[^\r\n]*--help[^\r\n]*grep/.test(workflow),
|
||||
'review.md must not capability-probe codex flags via `codex --help | grep` (#2479 — the probe itself feeds harness safety classifiers)'
|
||||
);
|
||||
test('no hook-trust bypass flag is emitted (#2479)', () => {
|
||||
assert.ok(!/dangerously|bypass/i.test(args),
|
||||
'host-harness safety classifiers deny commands carrying it; a genuine untrusted-hook prompt '
|
||||
+ 'surfaces through the .err capture and the empty-output stub instead');
|
||||
});
|
||||
|
||||
test('#1115: codex review failures are surfaced, not silently swallowed', () => {
|
||||
// stderr must be captured (not discarded to /dev/null) and an empty output
|
||||
// must be replaced with a diagnostic, so a broken reviewer is reported.
|
||||
for (const line of codexExecLines) {
|
||||
assert.ok(
|
||||
!line.includes('2>/dev/null'),
|
||||
`codex exec must not discard stderr to /dev/null:\n ${line.trim()}`
|
||||
);
|
||||
}
|
||||
assert.ok(
|
||||
/\[ ! -s \{run_dir\}\/gsd-review-codex\.md \]/.test(workflow),
|
||||
'review.md must guard against an empty codex review output and surface the failure'
|
||||
);
|
||||
});
|
||||
|
||||
test('--ephemeral appears before the prompt argument (flag ordering)', () => {
|
||||
for (const line of codexExecLines) {
|
||||
const ephemeralPos = line.indexOf('--ephemeral');
|
||||
const promptPos = line.indexOf(' - ');
|
||||
if (promptPos === -1) continue; // no stdin prompt arg on this line
|
||||
assert.ok(
|
||||
ephemeralPos < promptPos,
|
||||
`--ephemeral must appear before the stdin prompt argument:\n ${line.trim()}`
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test('--skip-git-repo-check is preserved alongside automation flags', () => {
|
||||
for (const line of codexExecLines) {
|
||||
assert.ok(
|
||||
line.includes('--skip-git-repo-check'),
|
||||
`codex exec invocation lost --skip-git-repo-check:\n ${line.trim()}`
|
||||
);
|
||||
}
|
||||
assert.equal(codex.emptyOutput, 'stub-with-stderr',
|
||||
'a failed codex run must produce a diagnosable stub carrying its stderr');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -427,48 +364,52 @@ function openCodeBlock() {
|
||||
}
|
||||
|
||||
describe('bug #1936: OpenCode reviewer must not silently yield an empty review', () => {
|
||||
// The agent can end its turn with ZERO output tokens, and `--format default` then drops the
|
||||
// assistant text entirely. Phase 5b (#2799) made the reconstruction a named first-party handler
|
||||
// rather than a jq pipeline in bash, so these assertions are behavioural over that handler.
|
||||
const { REVIEWER_LANES } = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const { resolveLanePlan } = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
const { handleOpencodeOutput } = require('../gsd-core/bin/lib/review-lane-runner.cjs');
|
||||
const NL = String.fromCharCode(10);
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === 'opencode');
|
||||
|
||||
test('captures opencode stderr to a sidecar, never /dev/null', () => {
|
||||
const block = openCodeBlock();
|
||||
assert.match(block, /opencode run [^\n]*2>\{run_dir\}\/gsd-review-opencode\.err/,
|
||||
'the opencode invocation must send stderr to a .err sidecar so failures are diagnosable');
|
||||
assert.doesNotMatch(block, /opencode run [^\n]*2>\/dev\/null/,
|
||||
'the opencode invocation must not discard stderr to /dev/null (#1936)');
|
||||
const r = resolveLanePlan({ lane, configGet: () => undefined, runDir: '/run', repoRoot: '/repo' });
|
||||
assert.equal(r.ok, true);
|
||||
assert.ok(r.plan.errPath.endsWith('.err'));
|
||||
assert.notEqual(r.plan.errPath, '/dev/null');
|
||||
});
|
||||
|
||||
test('requests structured JSON output and reconstructs review from assistant text parts', () => {
|
||||
const block = openCodeBlock();
|
||||
assert.match(block, /opencode run [^\n]*--format json/,
|
||||
'must invoke opencode with --format json so assistant text parts are recoverable');
|
||||
assert.match(block, /select\(\.type=="text"\)\s*\|\s*\.part\.text/,
|
||||
'must extract the assistant text parts via `.part.text` from the JSON event stream');
|
||||
test('requests structured JSON output and reconstructs from assistant text parts', () => {
|
||||
assert.ok(lane.invoke.args.join(' ').includes('--format json'),
|
||||
'--format json is the PRIMARY invocation, not a fallback');
|
||||
assert.equal(lane.handler, 'opencode', 'reconstruction cannot be expressed as data');
|
||||
const stream = [
|
||||
JSON.stringify({ type: 'text', part: { text: 'part one' } }),
|
||||
JSON.stringify({ type: 'text', part: { text: 'part two' } }),
|
||||
].join(NL);
|
||||
assert.equal(handleOpencodeOutput(stream).review, ['part one', 'part two'].join(NL));
|
||||
});
|
||||
|
||||
test('gates the empty-review stub on extracted CONTENT, not output-file size', () => {
|
||||
// An empty jq extraction still writes a trailing newline, so a `[ -s file ]`
|
||||
// check would treat a content-less review as populated and skip the stub. The
|
||||
// block must test the captured text variable instead.
|
||||
const block = openCodeBlock();
|
||||
assert.match(block, /OPENCODE_REVIEW=\$\(jq/, 'must capture the extraction into a variable');
|
||||
assert.match(block, /\[ -n "\$OPENCODE_REVIEW" \]/,
|
||||
'must branch on the content of $OPENCODE_REVIEW, not on the size of the .md file');
|
||||
assert.doesNotMatch(block, /\[ ! -s \{run_dir\}\/gsd-review-opencode\.md \]/,
|
||||
'must not gate the stub on `[ ! -s ...opencode...md ]` (a lone newline defeats it)');
|
||||
// A JSON stream with no assistant text is many BYTES but zero review.
|
||||
const noText = JSON.stringify({ type: 'step_finish', part: { reason: 'stop', tokens: { output: 0 } } });
|
||||
assert.ok(noText.length > 0, 'the raw stream is non-empty by byte count');
|
||||
assert.equal(handleOpencodeOutput(noText).review, '', 'yet the extracted review is empty');
|
||||
});
|
||||
|
||||
test('empty-output stub is diagnosable: references #1936, stop reason/tokens, and stderr', () => {
|
||||
const block = openCodeBlock();
|
||||
assert.match(block, /#1936/, 'the empty-output stub must reference the issue');
|
||||
assert.match(block, /step_finish[\s\S]*\.part\.reason[\s\S]*\.part\.tokens\.output/,
|
||||
'the stub must surface the stop reason and output-token count from the final step_finish');
|
||||
assert.match(block, /cat \{run_dir\}\/gsd-review-opencode\.err/,
|
||||
'the stub must append the captured stderr');
|
||||
test('empty-output stub is diagnosable: stop reason and output tokens', () => {
|
||||
const stream = JSON.stringify({ type: 'step_finish', part: { reason: 'stop', tokens: { output: 0 } } });
|
||||
const out = handleOpencodeOutput(stream);
|
||||
assert.ok(out.diagnostic.includes('stop'), 'the stop reason must be surfaced');
|
||||
assert.ok(out.diagnostic.includes('0'), 'the output-token count must be surfaced');
|
||||
});
|
||||
|
||||
test('does not regress the Codex reviewer block (still captures stderr to .err)', () => {
|
||||
// #1936 changes only the OpenCode block; the Codex block's existing
|
||||
// stderr-to-sidecar contract must remain intact.
|
||||
assert.match(read(), /codex exec [^\n]*2>\{run_dir\}\/gsd-review-codex\.err/,
|
||||
'the Codex reviewer block must be left unchanged');
|
||||
test('does not regress the Codex reviewer (still captures stderr to .err)', () => {
|
||||
const codex = REVIEWER_LANES.find((l) => l.slug === 'codex');
|
||||
const r = resolveLanePlan({ lane: codex, configGet: () => undefined, runDir: '/run', repoRoot: '/repo' });
|
||||
assert.ok(r.plan.errPath.endsWith('.err'));
|
||||
assert.equal(r.plan.outputTarget.kind, 'file', 'codex still captures via --output-last-message (#1698)');
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -35,6 +35,17 @@ const {
|
||||
const {
|
||||
KNOWN_REVIEWER_SLUGS,
|
||||
} = require('../gsd-core/bin/lib/review-reviewer-selection.cjs');
|
||||
const CAPABILITY_REGISTRY = require('../gsd-core/bin/lib/capability-registry.cjs');
|
||||
|
||||
/**
|
||||
* Reviewer slugs the GENERATED registry declares — the surface Phase 5b (#2799) re-pointed parity
|
||||
* onto. Once `invoke_reviewers` iterates lanes, the registry (not the workflow text) is what
|
||||
* decides which lanes exist at runtime.
|
||||
*/
|
||||
const REGISTRY_LANE_SLUGS = Object.values(CAPABILITY_REGISTRY.capabilities || {})
|
||||
.map((c) => c && c.reviewer && c.reviewer.slug)
|
||||
.filter((s) => typeof s === 'string' && s)
|
||||
.sort();
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
// Normalized to LF on read so the CRLF cases below can construct a Windows
|
||||
@@ -53,6 +64,7 @@ function check(overrides = {}) {
|
||||
return checkReviewerLaneParity({
|
||||
descriptor: REVIEWER_LANES,
|
||||
roster: KNOWN_REVIEWER_SLUGS,
|
||||
registry: REGISTRY_LANE_SLUGS,
|
||||
workflowText: WORKFLOW_TEXT,
|
||||
...overrides,
|
||||
});
|
||||
@@ -110,161 +122,113 @@ describe('reviewer lane parity — descriptor vs roster', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('reviewer lane parity — descriptor vs invoke_reviewers legs', () => {
|
||||
test('a leg added without a descriptor entry is a violation', () => {
|
||||
// The #2718 shape: a new lane's bash block lands in the workflow and nothing
|
||||
// else moves. This is the row a forward-only assertion cannot catch.
|
||||
const r = check({
|
||||
workflowText: WORKFLOW_TEXT.replace(
|
||||
'<!-- reviewer-lane: qwen -->',
|
||||
'<!-- reviewer-lane: qwen -->\n<!-- reviewer-lane: kimi_code -->',
|
||||
),
|
||||
});
|
||||
assert.deepStrictEqual(reasons(r), [
|
||||
`${PARITY_VIOLATION.LEG_MARKER_UNDECLARED}:kimi_code`,
|
||||
]);
|
||||
});
|
||||
|
||||
test('a declared lane whose workflow leg was removed is a violation', () => {
|
||||
const r = check({
|
||||
workflowText: WORKFLOW_TEXT.replace('<!-- reviewer-lane: qwen -->', ''),
|
||||
});
|
||||
assert.deepStrictEqual(reasons(r), [
|
||||
`${PARITY_VIOLATION.LEG_MARKER_MISSING}:qwen`,
|
||||
]);
|
||||
});
|
||||
|
||||
test('a duplicated leg marker is a violation', () => {
|
||||
const r = check({
|
||||
workflowText: WORKFLOW_TEXT.replace(
|
||||
'<!-- reviewer-lane: qwen -->',
|
||||
'<!-- reviewer-lane: qwen -->\n<!-- reviewer-lane: qwen -->',
|
||||
),
|
||||
});
|
||||
assert.deepStrictEqual(reasons(r), [
|
||||
`${PARITY_VIOLATION.LEG_MARKER_DUPLICATED}:qwen`,
|
||||
]);
|
||||
});
|
||||
|
||||
test('marker matching tolerates whitespace variation', () => {
|
||||
const r = check({
|
||||
workflowText: WORKFLOW_TEXT.replace(
|
||||
'<!-- reviewer-lane: qwen -->',
|
||||
'<!--reviewer-lane:qwen-->',
|
||||
),
|
||||
});
|
||||
assert.deepStrictEqual(r.violations, []);
|
||||
});
|
||||
|
||||
test('a marker outside the invoke_reviewers step does not satisfy the leg', () => {
|
||||
// Scoped, not file-wide: a marker parked in write_reviews must not be
|
||||
// mistaken for a dispatch leg.
|
||||
const moved = WORKFLOW_TEXT
|
||||
.replace('<!-- reviewer-lane: qwen -->', '')
|
||||
.replace('## Qwen Review', '<!-- reviewer-lane: qwen -->\n## Qwen Review');
|
||||
const r = check({ workflowText: moved });
|
||||
assert.deepStrictEqual(reasons(r), [
|
||||
`${PARITY_VIOLATION.LEG_MARKER_MISSING}:qwen`,
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('reviewer lane parity — descriptor vs write_reviews sections', () => {
|
||||
test('an output section with no declared lane is a violation', () => {
|
||||
const r = check({
|
||||
workflowText: WORKFLOW_TEXT.replace(
|
||||
'## Qwen Review',
|
||||
'## Qwen Review\n\n{qwen}\n\n---\n\n## Kimi Review',
|
||||
),
|
||||
});
|
||||
assert.deepStrictEqual(reasons(r), [
|
||||
`${PARITY_VIOLATION.SECTION_UNDECLARED}:Kimi`,
|
||||
]);
|
||||
});
|
||||
|
||||
test('a declared lane with no output section is a violation', () => {
|
||||
const r = check({
|
||||
workflowText: WORKFLOW_TEXT.replace('## Qwen Review', '## Renamed Heading'),
|
||||
});
|
||||
describe('reviewer lane parity — descriptor vs registry (ADR-2782 Phase 5b)', () => {
|
||||
// Phase 5b deleted the per-lane workflow text this file used to scan, so the leg-marker and
|
||||
// section-heading families are gone. What replaced them is the parity that is load-bearing once
|
||||
// lanes are data: the registry is what the runtime actually iterates.
|
||||
test('a registry lane with no descriptor entry is a violation', () => {
|
||||
const r = check({ registry: [...REGISTRY_LANE_SLUGS, 'acme'] });
|
||||
assert.ok(
|
||||
reasons(r).includes(`${PARITY_VIOLATION.SECTION_MISSING}:Qwen`),
|
||||
`expected a section-missing violation, got: ${JSON.stringify(reasons(r))}`,
|
||||
reasons(r).includes(`${PARITY_VIOLATION.REGISTRY_LANE_UNDECLARED}:acme`),
|
||||
'a lane the registry ships but the descriptor never declared must fail',
|
||||
);
|
||||
});
|
||||
|
||||
test('a duplicated output section is a violation', () => {
|
||||
// Two lanes under one heading would silently MERGE in REVIEWS.md, producing
|
||||
// a review that appears to have consensus it does not have (ADR-2782 D8).
|
||||
test('a descriptor lane absent from the registry is a violation', () => {
|
||||
const r = check({
|
||||
workflowText: WORKFLOW_TEXT.replace(
|
||||
'## Qwen Review',
|
||||
'## Qwen Review\n\n{a}\n\n---\n\n## Qwen Review',
|
||||
),
|
||||
descriptor: [...REVIEWER_LANES, fakeLane('acme')],
|
||||
roster: [...KNOWN_REVIEWER_SLUGS, 'acme'],
|
||||
});
|
||||
assert.deepStrictEqual(reasons(r), [
|
||||
`${PARITY_VIOLATION.SECTION_DUPLICATED}:Qwen`,
|
||||
]);
|
||||
assert.ok(
|
||||
reasons(r).includes(`${PARITY_VIOLATION.DESCRIPTOR_LANE_NOT_IN_REGISTRY}:acme`),
|
||||
'a declared lane no capability manifest ships must fail',
|
||||
);
|
||||
});
|
||||
|
||||
test('an empty registry reports every lane rather than passing silently', () => {
|
||||
// Degrading to violations is the point: a checker that cannot tell "no registry" from
|
||||
// "registry agrees" is worse than no checker.
|
||||
const r = check({ registry: [] });
|
||||
const missing = r.violations.filter(
|
||||
(v) => v.reason === PARITY_VIOLATION.DESCRIPTOR_LANE_NOT_IN_REGISTRY,
|
||||
);
|
||||
assert.equal(missing.length, REVIEWER_LANES.length);
|
||||
});
|
||||
|
||||
test('a non-array registry degrades to violations, never throws', () => {
|
||||
for (const bad of [null, undefined, 42, 'gemini', {}]) {
|
||||
const r = check({ registry: bad });
|
||||
assert.equal(r.ok, false);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('reviewer lane parity — anti-parity: no bespoke leg may return', () => {
|
||||
test('a re-added per-CLI leg marker is a violation', () => {
|
||||
// The regex flipped polarity in Phase 5b: matching a leg marker is now the failure. Without
|
||||
// this, nothing stops a contributor quietly re-adding a hand-authored block — which is the
|
||||
// drift (#2718 -> #2781) the epic exists to end, and every other check here would still pass.
|
||||
const wf = WORKFLOW_TEXT.replace(
|
||||
'<step name="invoke_reviewers">',
|
||||
'<step name="invoke_reviewers">\n<!-- reviewer-lane: gemini -->',
|
||||
);
|
||||
const r = check({ workflowText: wf });
|
||||
assert.ok(reasons(r).includes(`${PARITY_VIOLATION.BESPOKE_LEG_PRESENT}:gemini`));
|
||||
});
|
||||
|
||||
test('the shipped workflow contains no leg markers', () => {
|
||||
const r = check();
|
||||
assert.deepStrictEqual(
|
||||
r.violations.filter((v) => v.reason === PARITY_VIOLATION.BESPOKE_LEG_PRESENT),
|
||||
[],
|
||||
);
|
||||
});
|
||||
|
||||
test('a marker outside the invoke_reviewers step is not a bespoke leg', () => {
|
||||
const r = check({ workflowText: `${WORKFLOW_TEXT}\n<!-- reviewer-lane: gemini -->\n` });
|
||||
assert.deepStrictEqual(
|
||||
r.violations.filter((v) => v.reason === PARITY_VIOLATION.BESPOKE_LEG_PRESENT),
|
||||
[],
|
||||
'the anti-parity check is scoped to invoke_reviewers, as the old leg check was',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('reviewer lane parity — not-corruption (must NOT fire)', () => {
|
||||
test('ADR-1517 instance sections are exempt from lane parity', () => {
|
||||
// `## OpenCode Review (opencode-deepseek)` and `(opencode-mimo)` are already
|
||||
// in the shipped file. ADR-2782 D8: reviewer instances are not lanes. A
|
||||
// naive `## … Review` matcher fails against these on day one.
|
||||
const r = check();
|
||||
assert.deepStrictEqual(r.violations, []);
|
||||
|
||||
const withNewInstance = WORKFLOW_TEXT.replace(
|
||||
'## Qwen Review',
|
||||
'## Qwen Review (qwen-turbo)\n\n{x}\n\n---\n\n## Qwen Review',
|
||||
);
|
||||
assert.deepStrictEqual(
|
||||
checkReviewerLaneParity({
|
||||
descriptor: REVIEWER_LANES,
|
||||
roster: KNOWN_REVIEWER_SLUGS,
|
||||
workflowText: withNewInstance,
|
||||
}).violations,
|
||||
[],
|
||||
'adding a reviewer instance section must not trip lane parity',
|
||||
);
|
||||
// Phase 5b deleted the section-heading and leg-marker matchers these cases were written against.
|
||||
// The invariant they protected still matters and is asserted here against the surfaces that
|
||||
// replaced them: nothing in REVIEWS.md prose may promote itself into the lane roster.
|
||||
test('the shipped repo is clean', () => {
|
||||
assert.deepStrictEqual(check().violations, []);
|
||||
});
|
||||
|
||||
test('the h1 title and non-lane headings are not read as lane sections', () => {
|
||||
// `# Cross-AI Plan Review — Phase {N}` contains "Review" but is h1;
|
||||
// `## Consensus Summary` is h2 but has no ` Review` suffix.
|
||||
const r = check();
|
||||
assert.deepStrictEqual(r.violations, []);
|
||||
test('an ADR-1517 instance heading never becomes a lane', () => {
|
||||
// `## OpenCode Review (opencode-deepseek)` is an INSTANCE resolving through a lane, not a lane
|
||||
// (ADR-2782 D8). Instances take no part in the roster, the flag set, or uniqueness.
|
||||
const withNewInstance = WORKFLOW_TEXT.replace(
|
||||
'## Consensus Summary',
|
||||
'## Qwen Review (qwen-turbo)\n\n{x}\n\n---\n\n## Consensus Summary',
|
||||
);
|
||||
assert.deepStrictEqual(check({ workflowText: withNewInstance }).violations, []);
|
||||
});
|
||||
|
||||
test('extra non-lane headings are inert', () => {
|
||||
const withExtras = WORKFLOW_TEXT.replace(
|
||||
'## Consensus Summary',
|
||||
'## Another Summary\n\n---\n\n## Consensus Summary',
|
||||
);
|
||||
assert.deepStrictEqual(
|
||||
checkReviewerLaneParity({
|
||||
descriptor: REVIEWER_LANES,
|
||||
roster: KNOWN_REVIEWER_SLUGS,
|
||||
workflowText: withExtras,
|
||||
}).violations,
|
||||
[],
|
||||
);
|
||||
assert.deepStrictEqual(check({ workflowText: withExtras }).violations, []);
|
||||
});
|
||||
|
||||
test('bold prose in invoke_reviewers is not read as a leg', () => {
|
||||
// Five non-lane bold labels share the bold-then-fence shape a heuristic
|
||||
// matcher would key on. Adding another must not register a lane.
|
||||
// Non-lane bold labels share the bold-then-fence shape a heuristic matcher would key on.
|
||||
// Adding another must not register a lane — the anti-parity check keys on the explicit marker
|
||||
// only, never on prose shape.
|
||||
const withProse = WORKFLOW_TEXT.replace(
|
||||
'<!-- reviewer-lane: qwen -->',
|
||||
'**Some new maintainer note (#9999):**\n\n```bash\necho hi\n```\n\n<!-- reviewer-lane: qwen -->',
|
||||
);
|
||||
assert.deepStrictEqual(
|
||||
checkReviewerLaneParity({
|
||||
descriptor: REVIEWER_LANES,
|
||||
roster: KNOWN_REVIEWER_SLUGS,
|
||||
workflowText: withProse,
|
||||
}).violations,
|
||||
[],
|
||||
'<step name="invoke_reviewers">',
|
||||
'<step name="invoke_reviewers">\n**Some new maintainer note (#9999):**\n\n```bash\necho hi\n```\n',
|
||||
);
|
||||
assert.deepStrictEqual(check({ workflowText: withProse }).violations, []);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -279,28 +243,49 @@ describe('reviewer lane parity — cross-platform and hostile input', () => {
|
||||
|
||||
test('a divergence is still detected under CRLF', () => {
|
||||
const crlf = asCrlf(
|
||||
WORKFLOW_TEXT.replace('<!-- reviewer-lane: qwen -->', ''),
|
||||
WORKFLOW_TEXT.replace(
|
||||
'<step name="invoke_reviewers">',
|
||||
'<step name="invoke_reviewers">\n<!-- reviewer-lane: qwen -->',
|
||||
),
|
||||
);
|
||||
assert.deepStrictEqual(reasons(check({ workflowText: crlf })), [
|
||||
`${PARITY_VIOLATION.LEG_MARKER_MISSING}:qwen`,
|
||||
`${PARITY_VIOLATION.BESPOKE_LEG_PRESENT}:qwen`,
|
||||
]);
|
||||
});
|
||||
|
||||
test('empty workflow text degrades to violations rather than throwing', () => {
|
||||
// A read failure must never be mistaken for a clean bill of health.
|
||||
const r = check({ workflowText: '' });
|
||||
assert.strictEqual(r.ok, false);
|
||||
test('empty workflow text is clean for anti-parity but an empty REGISTRY is not', () => {
|
||||
// Phase 5b split what "degrades to violations" means. An empty WORKFLOW legitimately contains
|
||||
// no bespoke leg, so the anti-parity arm is silent — the workflow no longer names lanes at all.
|
||||
// The read-failure guard moved to the registry arm, which is now the surface that decides which
|
||||
// lanes exist: an unreadable registry must never read as "registry agrees".
|
||||
const emptyWorkflow = check({ workflowText: '' });
|
||||
assert.deepStrictEqual(
|
||||
emptyWorkflow.violations.filter((v) => v.reason === PARITY_VIOLATION.BESPOKE_LEG_PRESENT),
|
||||
[],
|
||||
);
|
||||
|
||||
const emptyRegistry = check({ registry: [] });
|
||||
assert.strictEqual(emptyRegistry.ok, false);
|
||||
assert.strictEqual(
|
||||
r.violations.filter((v) => v.reason === PARITY_VIOLATION.LEG_MARKER_MISSING)
|
||||
.length,
|
||||
emptyRegistry.violations.filter(
|
||||
(v) => v.reason === PARITY_VIOLATION.DESCRIPTOR_LANE_NOT_IN_REGISTRY,
|
||||
).length,
|
||||
REVIEWER_LANES.length,
|
||||
);
|
||||
});
|
||||
|
||||
test('non-string workflow text is coerced, never thrown on', () => {
|
||||
for (const bad of [undefined, null]) {
|
||||
// Totality is the invariant. Since Phase 5b the workflow no longer names lanes, so an absent
|
||||
// one legitimately yields NO anti-parity violation — the read-failure guard moved to the
|
||||
// registry arm, which is covered in the registry describe above.
|
||||
for (const bad of [undefined, null, 42, {}, []]) {
|
||||
const r = check({ workflowText: bad });
|
||||
assert.strictEqual(r.ok, false, `expected violations for ${String(bad)}`);
|
||||
assert.equal(typeof r.ok, 'boolean');
|
||||
assert.ok(Array.isArray(r.violations));
|
||||
assert.deepStrictEqual(
|
||||
r.violations.filter((v) => v.reason === PARITY_VIOLATION.BESPOKE_LEG_PRESENT),
|
||||
[],
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
@@ -361,7 +346,7 @@ describe('reviewer lane parity — properties', () => {
|
||||
const slugArb = fc.stringMatching(/^[a-z][a-z0-9_-]{0,12}$/);
|
||||
|
||||
/**
|
||||
* Slugs OUTSIDE the grammar. These are unmatchable by LEG_MARKER_RE, so the
|
||||
* Slugs OUTSIDE the declared LANE_SLUG_RE grammar, which stays enforced after Phase 5b so
|
||||
* contract is that they are reported as INVALID_SLUG rather than silently
|
||||
* reported missing. Includes regex metacharacters and prototype-pollution
|
||||
* shaped keys.
|
||||
@@ -395,7 +380,9 @@ describe('reviewer lane parity — properties', () => {
|
||||
|
||||
/** Build a review.md-shaped document declaring exactly `slugs`. */
|
||||
function docFor(slugs, sections, noise, eol) {
|
||||
const legs = slugs.map((s) => `<!-- reviewer-lane: ${s} -->\n**${s}:**`).join('\n');
|
||||
// No leg markers: Phase 5b's workflow iterates lanes, and a marker is now the violation.
|
||||
// `slugs` is retained so callers keep their existing shape.
|
||||
const legs = slugs.map((s) => `**${s}:**`).join('\n');
|
||||
const heads = sections.map((s) => `## ${s} Review`).join('\n\n');
|
||||
const body = [
|
||||
'<step name="invoke_reviewers">',
|
||||
@@ -465,23 +452,19 @@ describe('reviewer lane parity — properties', () => {
|
||||
);
|
||||
});
|
||||
|
||||
test('a slug outside the marker grammar is reported, never silently missing', () => {
|
||||
// The silent-miss this prevents: LEG_MARKER_RE captures only [a-z0-9_-], so
|
||||
// a slug like `acme.reviewer` can have a present, correct marker that the
|
||||
// scan can never see — reporting LEG_MARKER_MISSING forever with no clue why.
|
||||
test('a slug outside the declared grammar is named, never silently accepted', () => {
|
||||
// The grammar is still enforced after Phase 5b, for the same reason: a lane whose slug falls
|
||||
// outside it is unmatchable downstream, and a loud named violation beats a silent miss.
|
||||
fc.assert(
|
||||
fc.property(badSlugArb, (bad) => {
|
||||
const lane = { ...fakeLane('placeholder'), slug: bad };
|
||||
const r = checkReviewerLaneParity({
|
||||
descriptor: [lane],
|
||||
roster: [bad],
|
||||
workflowText: `<step name="invoke_reviewers">\n<!-- reviewer-lane: ${bad} -->\n</step>`,
|
||||
registry: [bad],
|
||||
workflowText: '<step name="invoke_reviewers">\n</step>',
|
||||
});
|
||||
const reasonsOut = r.violations.map((v) => v.reason);
|
||||
return (
|
||||
reasonsOut.includes(PARITY_VIOLATION.INVALID_SLUG) &&
|
||||
!reasonsOut.includes(PARITY_VIOLATION.LEG_MARKER_MISSING)
|
||||
);
|
||||
return r.violations.map((v) => v.reason).includes(PARITY_VIOLATION.INVALID_SLUG);
|
||||
}),
|
||||
FC,
|
||||
);
|
||||
@@ -497,19 +480,21 @@ describe('reviewer lane parity — properties', () => {
|
||||
const declared = checkReviewerLaneParity({
|
||||
descriptor: [lane],
|
||||
roster: [name],
|
||||
workflowText:
|
||||
`<step name="invoke_reviewers">\n<!-- reviewer-lane: ${name} -->\n</step>\n` +
|
||||
'<step name="write_reviews">\n## Sec Review\n</step>',
|
||||
registry: [name],
|
||||
workflowText: '<step name="invoke_reviewers">\n</step>',
|
||||
});
|
||||
const absent = checkReviewerLaneParity({
|
||||
descriptor: [lane],
|
||||
roster: [name],
|
||||
registry: [],
|
||||
workflowText: '<step name="invoke_reviewers">\n</step>',
|
||||
});
|
||||
return (
|
||||
declared.ok &&
|
||||
absent.violations.some(
|
||||
(v) => v.reason === PARITY_VIOLATION.LEG_MARKER_MISSING && v.subject === name,
|
||||
(v) =>
|
||||
v.reason === PARITY_VIOLATION.DESCRIPTOR_LANE_NOT_IN_REGISTRY &&
|
||||
v.subject === name,
|
||||
)
|
||||
);
|
||||
}),
|
||||
@@ -550,6 +535,7 @@ describe('reviewer lane parity — properties', () => {
|
||||
const r = checkReviewerLaneParity({
|
||||
descriptor: lanes,
|
||||
roster: lanes.map((l) => l.slug),
|
||||
registry: lanes.map((l) => l.slug),
|
||||
workflowText: doc,
|
||||
});
|
||||
return r.ok;
|
||||
@@ -558,24 +544,18 @@ describe('reviewer lane parity — properties', () => {
|
||||
);
|
||||
});
|
||||
|
||||
test('removing one lane marker always yields exactly that lane missing', () => {
|
||||
test('dropping one lane from the registry yields exactly that lane missing', () => {
|
||||
fc.assert(
|
||||
fc.property(laneSetArb, noiseArb, fc.nat(), (lanes, noise, pick) => {
|
||||
fc.property(laneSetArb, fc.nat(), (lanes, pick) => {
|
||||
const victim = lanes[pick % lanes.length];
|
||||
const kept = lanes.filter((l) => l.slug !== victim.slug);
|
||||
const doc = docFor(
|
||||
kept.map((l) => l.slug),
|
||||
lanes.map((l) => l.reviewsSection),
|
||||
noise,
|
||||
'\n',
|
||||
);
|
||||
const r = checkReviewerLaneParity({
|
||||
descriptor: lanes,
|
||||
roster: lanes.map((l) => l.slug),
|
||||
workflowText: doc,
|
||||
registry: lanes.filter((l) => l.slug !== victim.slug).map((l) => l.slug),
|
||||
workflowText: '<step name="invoke_reviewers">\n</step>',
|
||||
});
|
||||
const missing = r.violations.filter(
|
||||
(v) => v.reason === PARITY_VIOLATION.LEG_MARKER_MISSING,
|
||||
(v) => v.reason === PARITY_VIOLATION.DESCRIPTOR_LANE_NOT_IN_REGISTRY,
|
||||
);
|
||||
return missing.length === 1 && missing[0].subject === victim.slug;
|
||||
}),
|
||||
@@ -590,6 +570,7 @@ describe('reviewer lane parity — properties', () => {
|
||||
const input = {
|
||||
descriptor: lanes,
|
||||
roster: lanes.map((l) => l.slug),
|
||||
registry: lanes.map((l) => l.slug),
|
||||
workflowText: docFor(
|
||||
lanes.map((l) => l.slug),
|
||||
lanes.map((l) => l.reviewsSection),
|
||||
@@ -664,13 +645,15 @@ describe('reviewer lane descriptor — declared shape (ADR-2782 D1/D2/D6/D7)', (
|
||||
});
|
||||
|
||||
test('handler is a closed first-party enum', () => {
|
||||
const allowed = [null, 'antigravity', 'openai-compatible'];
|
||||
const allowed = [null, 'antigravity', 'openai-compatible', 'opencode'];
|
||||
for (const lane of REVIEWER_LANES) {
|
||||
assert.ok(allowed.includes(lane.handler), `${lane.slug}: unexpected handler ${lane.handler}`);
|
||||
}
|
||||
assert.deepStrictEqual(
|
||||
REVIEWER_LANES.filter((l) => l.handler !== null).map((l) => l.slug).sort(),
|
||||
['antigravity', 'llama_cpp', 'lm_studio', 'ollama'],
|
||||
// `opencode` joined in Phase 5b: its review is reconstructed from assistant text parts of a
|
||||
// --format json stream, which data cannot express (#1936).
|
||||
['antigravity', 'llama_cpp', 'lm_studio', 'ollama', 'opencode'],
|
||||
);
|
||||
});
|
||||
|
||||
@@ -715,19 +698,16 @@ describe('reviewer lane descriptor — declared shape (ADR-2782 D1/D2/D6/D7)', (
|
||||
// Adding a reason is three coordinated changes: enum, emitting site, and
|
||||
// this assertion.
|
||||
assert.deepStrictEqual(Object.keys(PARITY_VIOLATION).sort(), [
|
||||
'BESPOKE_LEG_PRESENT',
|
||||
'DESCRIPTOR_LANE_NOT_IN_REGISTRY',
|
||||
'DESCRIPTOR_LANE_NOT_IN_ROSTER',
|
||||
'DUPLICATE_FLAG',
|
||||
'DUPLICATE_SECTION',
|
||||
'DUPLICATE_SLUG',
|
||||
'INVALID_SLUG',
|
||||
'LEG_MARKER_DUPLICATED',
|
||||
'LEG_MARKER_MISSING',
|
||||
'LEG_MARKER_UNDECLARED',
|
||||
'MALFORMED_LANE',
|
||||
'REGISTRY_LANE_UNDECLARED',
|
||||
'ROSTER_SLUG_UNDECLARED',
|
||||
'SECTION_DUPLICATED',
|
||||
'SECTION_MISSING',
|
||||
'SECTION_UNDECLARED',
|
||||
]);
|
||||
assert.ok(Object.isFrozen(PARITY_VIOLATION));
|
||||
});
|
||||
|
||||
452
tests/review-lane-invocation.test.cjs
Normal file
452
tests/review-lane-invocation.test.cjs
Normal file
@@ -0,0 +1,452 @@
|
||||
/**
|
||||
* Reviewer lane invocation — the resolver (ADR-2782 Phase 5b, #2799).
|
||||
*
|
||||
* THE GOLDEN TABLE IS THE POINT OF THIS FILE. Phase 5b deleted ~640 lines of hand-authored per-CLI
|
||||
* bash, and every one of those legs encoded a hard-won fix (#2494/#2605 empty output, #1698 Codex
|
||||
* stdout teardown noise, #1936 OpenCode zero-output turns, #2073 Antigravity's three modes, #2176
|
||||
* repo-root anchoring, #2589 no jq on stock Windows, #2794 Qwen's missing sidecar). Old and new
|
||||
* cannot literally run in parallel, so the golden table below IS the strangler-fig substitute: each
|
||||
* row is the invocation the bash leg produced, and the resolver must reproduce it exactly.
|
||||
*
|
||||
* The rows were derived FROM THE LEGS, not from the descriptor types. That direction matters — a
|
||||
* table written from the types would agree with the resolver by construction and prove nothing.
|
||||
*/
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const fc = require('fast-check');
|
||||
|
||||
const {
|
||||
REVIEWER_LANES,
|
||||
} = require('../gsd-core/bin/lib/review-lane-descriptor.cjs');
|
||||
const {
|
||||
resolveLanePlan,
|
||||
isEmptyReview,
|
||||
normalizeHost,
|
||||
fileRefPrompt,
|
||||
LANE_UNAVAILABLE,
|
||||
} = require('../gsd-core/bin/lib/review-lane-invocation.cjs');
|
||||
|
||||
const RUN = '/run';
|
||||
const ROOT = '/repo';
|
||||
|
||||
/** Deterministic property runs — pinned seed, bounded, replay printed on failure. */
|
||||
const FC = { seed: 42, numRuns: 200 };
|
||||
|
||||
/** Config with every model key set, so the model-bearing rows exercise the configured branch. */
|
||||
const FULL_CONFIG = {
|
||||
'review.models.gemini': 'G',
|
||||
'review.models.claude': 'C',
|
||||
'review.models.codex': 'X',
|
||||
'review.models.opencode': 'O',
|
||||
'review.models.agy': 'A',
|
||||
'review.models.kimi-code': 'K',
|
||||
'review.models.ollama': 'M',
|
||||
'review.models.lm_studio': 'M',
|
||||
'review.models.llama_cpp': 'M',
|
||||
};
|
||||
|
||||
function resolve(slug, { config = FULL_CONFIG, effortArgs = ['--effort', 'high'] } = {}) {
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === slug);
|
||||
assert.ok(lane, `no declared lane '${slug}'`);
|
||||
return resolveLanePlan({
|
||||
lane,
|
||||
configGet: (k) => config[k],
|
||||
runDir: RUN,
|
||||
repoRoot: ROOT,
|
||||
effortArgs,
|
||||
});
|
||||
}
|
||||
|
||||
const FILE_REF = fileRefPrompt(`${RUN}/gsd-review-prompt.md`, ROOT);
|
||||
|
||||
/**
|
||||
* One row per shipped lane: the exact argv its bash leg produced, with a model configured and
|
||||
* effort available. `stdin` is the prompt path for a stdin lane, `null` otherwise.
|
||||
*/
|
||||
const GOLDEN = [
|
||||
{ slug: 'gemini', binary: 'gemini', argv: ['-m', 'G', '-p', '-'], stdin: true, out: 'stdout', timeout: 900000 },
|
||||
{ slug: 'claude', binary: 'claude', argv: ['--model', 'C', '--effort', 'high', '-p', '-'], stdin: true, out: 'stdout', timeout: 1200000 },
|
||||
{
|
||||
slug: 'codex',
|
||||
binary: 'codex',
|
||||
// `exec` is a SUBCOMMAND and must stay first; the output file lands mid-argv and the bare `-`
|
||||
// stays last. Splicing injected flags positionally produced an invalid invocation.
|
||||
argv: ['exec', '--ephemeral', '--model', 'X', '--effort', 'high', '--skip-git-repo-check', '-o', `${RUN}/gsd-review-codex.md`, '-'],
|
||||
stdin: true, out: 'file', timeout: 1200000,
|
||||
},
|
||||
{ slug: 'coderabbit', binary: 'coderabbit', argv: ['review', '--prompt-only'], stdin: false, out: 'stdout', timeout: 360000 },
|
||||
{ slug: 'opencode', binary: 'opencode', argv: ['run', '--model', 'O', '--effort', 'high', '--format', 'json', '-'], stdin: true, out: 'stdout', timeout: 660000 },
|
||||
{ slug: 'qwen', binary: 'qwen', argv: ['-'], stdin: true, out: 'stdout', timeout: 900000 },
|
||||
{ slug: 'cursor', binary: 'cursor-agent', argv: ['-p', '--mode', 'ask', '--trust', '--output-format', 'text', FILE_REF], stdin: false, out: 'stdout', timeout: 900000 },
|
||||
{ slug: 'antigravity', binary: 'agy', argv: ['--print-timeout', '540s', '--model', 'A', '-p', FILE_REF], stdin: false, out: 'stdout', timeout: 600000 },
|
||||
{ slug: 'kimi-code', binary: 'kimi', argv: ['-m', 'K', '-p', FILE_REF], stdin: false, out: 'stdout', timeout: 900000 },
|
||||
];
|
||||
|
||||
describe('reviewer lane invocation — golden plans (the strangler-fig contract)', () => {
|
||||
for (const row of GOLDEN) {
|
||||
test(`${row.slug} resolves to its shipped invocation`, () => {
|
||||
const r = resolve(row.slug);
|
||||
assert.equal(r.ok, true, `${row.slug}: ${r.ok ? '' : r.detail}`);
|
||||
const p = r.plan;
|
||||
assert.equal(p.transport, 'spawn');
|
||||
assert.equal(p.binary, row.binary);
|
||||
assert.deepStrictEqual(p.argv, row.argv);
|
||||
assert.equal(p.stdin, row.stdin ? `${RUN}/gsd-review-prompt.md` : null);
|
||||
assert.equal(p.outputTarget.kind, row.out === 'file' ? 'file' : 'stdout');
|
||||
assert.equal(p.timeoutMs, row.timeout);
|
||||
// The stderr sidecar is never /dev/null — that is what makes a failed lane diagnosable.
|
||||
assert.equal(p.errPath, `${RUN}/gsd-review-${row.slug}.err`);
|
||||
assert.equal(p.reviewPath, `${RUN}/gsd-review-${row.slug}.md`);
|
||||
});
|
||||
}
|
||||
|
||||
test('the golden table covers every spawn lane', () => {
|
||||
const spawnSlugs = REVIEWER_LANES.filter((l) => l.transport === 'spawn').map((l) => l.slug).sort();
|
||||
assert.deepStrictEqual(GOLDEN.map((g) => g.slug).sort(), spawnSlugs);
|
||||
});
|
||||
|
||||
test('the three OpenAI-compatible lanes resolve host, endpoint and discovery', () => {
|
||||
for (const [slug, host, fallback] of [
|
||||
['ollama', 'http://localhost:11434', 'llama3'],
|
||||
['lm_studio', 'http://localhost:1234', 'local-model'],
|
||||
['llama_cpp', 'http://localhost:8080', 'local-model'],
|
||||
]) {
|
||||
const r = resolve(slug, { config: {} });
|
||||
assert.equal(r.ok, true);
|
||||
assert.equal(r.plan.transport, 'openai-http');
|
||||
// Phase 4 federated every *_host with a default of "", so an unset key MUST fall back to the
|
||||
// lane's declared defaultHost or the lane would POST to a garbage URL.
|
||||
assert.equal(r.plan.host, host);
|
||||
assert.equal(r.plan.url, `${host}/v1/chat/completions`);
|
||||
assert.equal(r.plan.modelsUrl, `${host}/v1/models`);
|
||||
assert.equal(r.plan.fallbackModel, fallback);
|
||||
}
|
||||
});
|
||||
|
||||
test('every declared lane resolves — none is left unroutable', () => {
|
||||
for (const lane of REVIEWER_LANES) {
|
||||
assert.equal(resolve(lane.slug).ok, true, `${lane.slug} failed to resolve`);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('reviewer lane invocation — model resolution', () => {
|
||||
test("antigravity resolves its model from review.models.agy, not the slug", () => {
|
||||
// The regression this locks: antigravity's slug is `antigravity` but its shipped key is
|
||||
// `review.models.agy`. A `review.models.<slug>` convention misses it and silently ignores a
|
||||
// configured model — disabling the pinned-model escape hatch #2073 added for a 404ing default.
|
||||
const r = resolve('antigravity', { config: { 'review.models.agy': 'agy-2' } });
|
||||
assert.ok(r.plan.argv.includes('agy-2'), 'the configured agy model must reach argv');
|
||||
|
||||
const wrongKey = resolve('antigravity', { config: { 'review.models.antigravity': 'nope' } });
|
||||
assert.ok(!wrongKey.plan.argv.includes('nope'), 'the slug-derived key must NOT be consulted');
|
||||
});
|
||||
|
||||
test('a lane declaring no model key emits no model argument', () => {
|
||||
for (const slug of ['qwen', 'cursor', 'coderabbit']) {
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === slug);
|
||||
assert.equal(lane.modelConfigKey, null, `${slug} should declare no model key`);
|
||||
const r = resolve(slug, { config: { 'review.models.qwen': 'X', 'review.models.cursor': 'X' } });
|
||||
assert.ok(!r.plan.argv.includes('X'));
|
||||
}
|
||||
});
|
||||
|
||||
test('unset, empty, whitespace and the literal string "null" all mean unconfigured', () => {
|
||||
// `"null"` is the four literal characters `config-get --raw` prints for a missing key — every
|
||||
// bash leg tested for it. A config written by an older workflow can still contain it.
|
||||
for (const bad of [undefined, null, '', ' ', 'null', 'undefined']) {
|
||||
const r = resolve('gemini', { config: { 'review.models.gemini': bad } });
|
||||
assert.deepStrictEqual(r.plan.argv, ['-p', '-'], `${JSON.stringify(bad)} must not reach argv`);
|
||||
}
|
||||
});
|
||||
|
||||
test('a non-string model value is never coerced into argv', () => {
|
||||
// String(0) would put "0" in as a model name. A wrong model silently reviewed is worse than no
|
||||
// override at all.
|
||||
for (const bad of [0, 1, true, false, [], {}, ['a']]) {
|
||||
const r = resolve('gemini', { config: { 'review.models.gemini': bad } });
|
||||
assert.deepStrictEqual(r.plan.argv, ['-p', '-'], `${JSON.stringify(bad)} must not reach argv`);
|
||||
}
|
||||
});
|
||||
|
||||
test('shell metacharacters in a model value stay a single inert argv element', () => {
|
||||
const hostile = '; rm -rf /; $(whoami) `id` && echo "x"';
|
||||
const r = resolve('gemini', { config: { 'review.models.gemini': hostile } });
|
||||
assert.deepStrictEqual(r.plan.argv, ['-m', hostile, '-p', '-']);
|
||||
// Nothing here builds a shell string; the runner spawns with shell:false and an argv array.
|
||||
assert.equal(r.plan.argv.filter((a) => a === hostile).length, 1);
|
||||
});
|
||||
|
||||
test('effort argv only reaches lanes declaring effortChannel argv', () => {
|
||||
for (const lane of REVIEWER_LANES.filter((l) => l.transport === 'spawn')) {
|
||||
const r = resolve(lane.slug, { effortArgs: ['--effort', 'xhigh'] });
|
||||
const got = r.plan.argv.includes('--effort');
|
||||
assert.equal(got, lane.invoke.effortChannel === 'argv', `${lane.slug} effort mismatch`);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('reviewer lane invocation — argv-file-ref anchoring (#2176)', () => {
|
||||
test('the file-ref prompt names the prompt file AND the absolute repo root', () => {
|
||||
// Without the root, an argv-fed CLI does not reliably inherit the review cwd and reviews the
|
||||
// plan text in isolation — exactly what the Review Instructions forbid.
|
||||
for (const slug of ['cursor', 'antigravity', 'kimi-code']) {
|
||||
const r = resolve(slug);
|
||||
const arg = r.plan.argv[r.plan.argv.length - 1];
|
||||
assert.ok(arg.includes(`${RUN}/gsd-review-prompt.md`), `${slug} must name the prompt file`);
|
||||
assert.ok(arg.includes(ROOT), `${slug} must carry the absolute repo root`);
|
||||
}
|
||||
});
|
||||
|
||||
test('the prompt travels in argv as ONE element, never split', () => {
|
||||
const r = resolve('cursor');
|
||||
assert.equal(r.plan.argv.filter((a) => a.includes('gsd-review-prompt.md')).length, 1);
|
||||
});
|
||||
});
|
||||
|
||||
describe('reviewer lane invocation — absent-safe and hostile input (ADR-2782 D4)', () => {
|
||||
const bad = (lane) =>
|
||||
resolveLanePlan({ lane, configGet: () => undefined, runDir: RUN, repoRoot: ROOT });
|
||||
|
||||
test('a malformed lane is reported, never thrown on', () => {
|
||||
for (const v of [null, undefined, 42, 'gemini', [], true]) {
|
||||
const r = bad(v);
|
||||
assert.equal(r.ok, false);
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.MALFORMED_LANE);
|
||||
}
|
||||
});
|
||||
|
||||
test('an unknown handler fails CLOSED', () => {
|
||||
// D4 rule 4. A lane naming imperative code this version does not have cannot run "mostly" —
|
||||
// the handler is precisely the part data could not express.
|
||||
const lane = { ...REVIEWER_LANES[0], handler: 'not-a-real-handler' };
|
||||
const r = bad(lane);
|
||||
assert.equal(r.ok, false);
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.UNKNOWN_HANDLER);
|
||||
});
|
||||
|
||||
test('every handler the descriptor ships is dispatchable', () => {
|
||||
// The inverse of the above: a lane declaring a handler the runner cannot dispatch would fail
|
||||
// closed at runtime, which is a silent lane loss dressed as a safety feature.
|
||||
for (const lane of REVIEWER_LANES) {
|
||||
assert.equal(resolve(lane.slug).ok, true, `${lane.slug} handler not dispatchable`);
|
||||
}
|
||||
});
|
||||
|
||||
test('an unknown transport is reported', () => {
|
||||
const r = bad({ ...REVIEWER_LANES[0], transport: 'carrier-pigeon' });
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.UNKNOWN_TRANSPORT);
|
||||
});
|
||||
|
||||
test('a spawn lane with no binary is malformed, not a crash', () => {
|
||||
const lane = REVIEWER_LANES.find((l) => l.transport === 'spawn');
|
||||
const r = bad({ ...lane, invoke: { ...lane.invoke, binary: '' } });
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.MALFORMED_LANE);
|
||||
});
|
||||
|
||||
test('file-arg output with no outputArg is malformed', () => {
|
||||
// Knowing the review lands in a file is useless without the argument naming it.
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === 'codex');
|
||||
const r = bad({ ...lane, invoke: { ...lane.invoke, outputArg: undefined } });
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.MALFORMED_LANE);
|
||||
});
|
||||
|
||||
test('a prototype-key slug does not pollute the expansion table', () => {
|
||||
// `__proto__` is now rejected outright by the slug grammar (leading `_` is outside
|
||||
// `[a-z0-9]`), which is a stronger guarantee than tolerating it. `constructor` and `prototype`
|
||||
// ARE valid slugs, so they must resolve normally and still reach no prototype.
|
||||
const proto = bad({ ...REVIEWER_LANES[0], slug: '__proto__' });
|
||||
assert.equal(proto.ok, false);
|
||||
assert.equal(proto.reason, LANE_UNAVAILABLE.MALFORMED_LANE);
|
||||
|
||||
for (const name of ['constructor', 'prototype']) {
|
||||
const r = bad({ ...REVIEWER_LANES[0], slug: name });
|
||||
assert.equal(r.ok, true, `${name} is a grammatically valid slug`);
|
||||
assert.equal(r.plan.slug, name);
|
||||
assert.equal(r.plan.reviewPath, `${RUN}/gsd-review-${name}.md`);
|
||||
assert.equal({}.polluted, undefined);
|
||||
}
|
||||
});
|
||||
|
||||
test('an http lane resolving no host at all is malformed rather than POSTing to nowhere', () => {
|
||||
const lane = REVIEWER_LANES.find((l) => l.transport === 'openai-http');
|
||||
const r = bad({ ...lane, invoke: { ...lane.invoke, defaultHost: '' } });
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.MALFORMED_LANE);
|
||||
});
|
||||
|
||||
test('an openai-http lane with no invoke object is malformed, not a crash', () => {
|
||||
// Found by adversarial review. The spawn branch guarded with `inv?.binary`; the http branch
|
||||
// dereferenced `inv.hostConfigKey` directly and THREW, breaking this module's documented
|
||||
// totality. A throw here is worse than it looks: the CLI seam resolves every selected lane in
|
||||
// one `.map`, so one malformed overlay manifest would abort the entire review.
|
||||
for (const missing of [undefined, null, 42, 'x', []]) {
|
||||
const r = bad({ ...REVIEWER_LANES.find((l) => l.transport === 'openai-http'), invoke: missing });
|
||||
assert.equal(r.ok, false, `invoke=${JSON.stringify(missing)} must not resolve`);
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.MALFORMED_LANE);
|
||||
}
|
||||
});
|
||||
|
||||
test('a slug outside the declared grammar cannot reach an artifact path', () => {
|
||||
// Found by adversarial review. The slug is concatenated into reviewPath/errPath, so a lane
|
||||
// declaring `../../../tmp/evil` produced a path OUTSIDE the run dir that writeReviewOrStub
|
||||
// would write to. The grammar is checked upstream by the parity gate and the capability
|
||||
// validator, but neither runs on this path — and this module is the overlay-manifest trust
|
||||
// boundary, so it enforces its own precondition rather than inheriting one.
|
||||
const spawnLane = REVIEWER_LANES.find((l) => l.transport === 'spawn');
|
||||
for (const slug of ['../../../tmp/evil', 'a/b', 'a\\b', 'UPPER', '.hidden', '-lead', 'a b']) {
|
||||
const r = bad({ ...spawnLane, slug });
|
||||
assert.equal(r.ok, false, `slug ${JSON.stringify(slug)} must be rejected`);
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.MALFORMED_LANE);
|
||||
}
|
||||
// Every shipped slug must still pass — including the snake-case ones.
|
||||
for (const lane of REVIEWER_LANES) {
|
||||
assert.equal(resolve(lane.slug).ok, true, `${lane.slug} must remain valid`);
|
||||
}
|
||||
});
|
||||
|
||||
test('the unavailability reason enum is locked', () => {
|
||||
// Adding a reason is three coordinated changes: enum, emitting site, and this assertion.
|
||||
assert.deepStrictEqual(Object.keys(LANE_UNAVAILABLE).sort(), [
|
||||
'BUDGET_TOOL_FAILED',
|
||||
'BUDGET_TOO_SMALL',
|
||||
'EGRESS_HOST_CHANGED',
|
||||
'HOST_UNREACHABLE',
|
||||
'MALFORMED_LANE',
|
||||
'MISSING_BINARY',
|
||||
'MISSING_REQUIRED_BINARY',
|
||||
'PROBE_FAILED',
|
||||
'PROBE_TIMEOUT',
|
||||
'UNKNOWN_HANDLER',
|
||||
'UNKNOWN_TRANSPORT',
|
||||
]);
|
||||
assert.ok(Object.isFrozen(LANE_UNAVAILABLE));
|
||||
});
|
||||
});
|
||||
|
||||
describe('reviewer lane invocation — empty-review classification', () => {
|
||||
test('whitespace-only output counts as empty on EVERY lane', () => {
|
||||
// `[ ! -s file ]` counted BYTES, so three spaces passed as a successful review. Two legs closed
|
||||
// this locally; five did not. Uniformity here is a deliberate, disclosed behaviour change.
|
||||
for (const s of ['', ' ', ' ', '\n', '\r\n', '\t', ' \n\t ']) {
|
||||
assert.equal(isEmptyReview(s), true, `${JSON.stringify(s)} must count as empty`);
|
||||
}
|
||||
});
|
||||
|
||||
test('a single non-space character is a review (limit+1)', () => {
|
||||
assert.equal(isEmptyReview('x'), false);
|
||||
assert.equal(isEmptyReview(' x '), false);
|
||||
});
|
||||
|
||||
test('output that is exactly -n / -e / -E is a review, not a swallowed value', () => {
|
||||
// `echo "$VAR"` would write 0 bytes for these and misclassify a real reply. Nothing in this
|
||||
// path goes through echo, so the hazard is structurally impossible — locked here anyway.
|
||||
for (const s of ['-n', '-e', '-E']) assert.equal(isEmptyReview(s), false);
|
||||
});
|
||||
|
||||
test('a non-string is empty, never thrown on', () => {
|
||||
for (const v of [undefined, null, 0, {}, []]) assert.equal(isEmptyReview(v), true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('reviewer lane invocation — host normalization (D5 comparison input)', () => {
|
||||
test('cosmetic differences are NOT destination changes', () => {
|
||||
// A warning that fires on a trailing slash is a warning users learn to dismiss — which would
|
||||
// defeat the one prompt that actually matters.
|
||||
const same = [
|
||||
['http://localhost:8080', 'http://localhost:8080/'],
|
||||
['http://localhost:8080', 'http://LOCALHOST:8080'],
|
||||
['http://a.com:80', 'http://a.com'],
|
||||
['https://a.com:443', 'https://a.com'],
|
||||
['http://a.com/v1/', 'http://a.com/v1'],
|
||||
];
|
||||
for (const [a, b] of same) {
|
||||
assert.equal(normalizeHost(a), normalizeHost(b), `${a} vs ${b}`);
|
||||
}
|
||||
});
|
||||
|
||||
test('a real destination change survives normalization', () => {
|
||||
assert.notEqual(normalizeHost('http://localhost:8080'), normalizeHost('http://evil.example'));
|
||||
assert.notEqual(normalizeHost('http://a.com:8080'), normalizeHost('http://a.com:9090'));
|
||||
assert.notEqual(normalizeHost('http://a.com'), normalizeHost('https://a.com'));
|
||||
});
|
||||
|
||||
test('an unparseable host is compared verbatim, never silently rewritten', () => {
|
||||
assert.equal(normalizeHost('not a url'), 'not a url');
|
||||
assert.equal(normalizeHost(''), '');
|
||||
});
|
||||
|
||||
test('a scheme-less host is not rewritten into a fake URL', () => {
|
||||
// Found by adversarial review. `new URL('localhost:11434')` PARSES — protocol `localhost:`,
|
||||
// empty hostname — so a plausible but scheme-less config value was being rewritten to
|
||||
// `localhost://11434` and then both compared and requested as if it were a real destination.
|
||||
// No hostname means it is not a URL; return it verbatim so it fails visibly.
|
||||
assert.equal(normalizeHost('localhost:11434'), 'localhost:11434');
|
||||
assert.equal(normalizeHost('example.com:8080'), 'example.com:8080');
|
||||
// A real URL still normalizes.
|
||||
assert.equal(normalizeHost('http://LocalHost:8080/'), 'http://localhost:8080');
|
||||
});
|
||||
});
|
||||
|
||||
describe('reviewer lane invocation — properties', () => {
|
||||
test('the resolver is total over arbitrary lane input', () => {
|
||||
// Third-party overlay manifests reach this function. A resolver that throws on bad input cannot
|
||||
// report on it, and a gate that crashes is indistinguishable from one never run.
|
||||
fc.assert(
|
||||
fc.property(fc.anything(), fc.anything(), (lane, cfgValue) => {
|
||||
let r;
|
||||
try {
|
||||
r = resolveLanePlan({
|
||||
lane,
|
||||
configGet: () => cfgValue,
|
||||
runDir: RUN,
|
||||
repoRoot: ROOT,
|
||||
});
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
return typeof r === 'object' && r !== null && typeof r.ok === 'boolean';
|
||||
}),
|
||||
FC,
|
||||
);
|
||||
});
|
||||
|
||||
test('a resolved argv never contains a placeholder token', () => {
|
||||
// An unexpanded `{{model}}` reaching a real CLI is an argument that means nothing to it.
|
||||
fc.assert(
|
||||
fc.property(
|
||||
fc.constantFrom(...REVIEWER_LANES.filter((l) => l.transport === 'spawn').map((l) => l.slug)),
|
||||
fc.option(fc.string(), { nil: undefined }),
|
||||
fc.array(fc.string(), { maxLength: 4 }),
|
||||
(slug, model, effortArgs) => {
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === slug);
|
||||
const r = resolveLanePlan({
|
||||
lane,
|
||||
configGet: () => model,
|
||||
runDir: RUN,
|
||||
repoRoot: ROOT,
|
||||
effortArgs,
|
||||
});
|
||||
if (!r.ok) return false;
|
||||
return !r.plan.argv.some((a) => /^\{\{(model|effort|output|prompt)\}\}$/.test(a));
|
||||
},
|
||||
),
|
||||
FC,
|
||||
);
|
||||
});
|
||||
|
||||
test('resolution is deterministic for identical input', () => {
|
||||
fc.assert(
|
||||
fc.property(
|
||||
fc.constantFrom(...REVIEWER_LANES.map((l) => l.slug)),
|
||||
fc.option(fc.string(), { nil: undefined }),
|
||||
(slug, model) => {
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === slug);
|
||||
const input = { lane, configGet: () => model, runDir: RUN, repoRoot: ROOT };
|
||||
return JSON.stringify(resolveLanePlan(input)) === JSON.stringify(resolveLanePlan(input));
|
||||
},
|
||||
),
|
||||
FC,
|
||||
);
|
||||
});
|
||||
});
|
||||
529
tests/review-lane-runner.test.cjs
Normal file
529
tests/review-lane-runner.test.cjs
Normal file
@@ -0,0 +1,529 @@
|
||||
/**
|
||||
* Reviewer lane runner — execution, probes, handlers, egress (ADR-2782 Phase 5b, #2799).
|
||||
*
|
||||
* Every dependency is injected, so these are behavioural tests over the real control flow with no
|
||||
* network, no spawn and no clock. Where a filesystem failure is forced it is done by making the
|
||||
* injected `writeFile`/`readFile` throw — never by `chmod 0o000`, which root bypasses, silently
|
||||
* turning the test into a vacuous pass in root Docker/CI.
|
||||
*/
|
||||
|
||||
const { describe, test } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
|
||||
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 {
|
||||
checkEgressHost,
|
||||
probeLane,
|
||||
runLane,
|
||||
writeReviewOrStub,
|
||||
handleOpencodeOutput,
|
||||
stampBlindReview,
|
||||
antigravityTranscriptFallback,
|
||||
runOpenAiCompatible,
|
||||
} = require('../gsd-core/bin/lib/review-lane-runner.cjs');
|
||||
|
||||
const RUN = '/run';
|
||||
const ROOT = '/repo';
|
||||
|
||||
function plan(slug, config = {}) {
|
||||
const lane = REVIEWER_LANES.find((l) => l.slug === slug);
|
||||
const r = resolveLanePlan({ lane, configGet: (k) => config[k], runDir: RUN, repoRoot: ROOT });
|
||||
assert.equal(r.ok, true, `${slug} failed to resolve`);
|
||||
return r.plan;
|
||||
}
|
||||
|
||||
/** An in-memory dependency set. Overrides replace individual seams per test. */
|
||||
function deps(overrides = {}) {
|
||||
const files = overrides.files || {};
|
||||
const warnings = [];
|
||||
const spawns = [];
|
||||
const base = {
|
||||
files,
|
||||
warnings,
|
||||
spawns,
|
||||
spawn: (binary, argv, opts) => {
|
||||
spawns.push({ binary, argv, opts });
|
||||
return { status: 0, stdout: '', stderr: '' };
|
||||
},
|
||||
httpJson: async () => ({ ok: true, status: 200, body: '{}' }),
|
||||
readFile: (p) => {
|
||||
if (!(p in files)) throw new Error(`ENOENT ${p}`);
|
||||
return files[p];
|
||||
},
|
||||
writeFile: (p, c) => { files[p] = c; },
|
||||
exists: (p) => p in files,
|
||||
hasBinary: () => true,
|
||||
configGet: () => undefined,
|
||||
homeDir: '/home/u',
|
||||
warn: (m) => warnings.push(m),
|
||||
};
|
||||
return Object.assign(base, overrides);
|
||||
}
|
||||
|
||||
describe('runner — egress host re-verification (ADR-2782 D5 rules 2-4)', () => {
|
||||
test('no consent record ALLOWS — first-party lanes are never consent-gated', () => {
|
||||
// Blocking on absence would break every existing local-model user on upgrade: ollama,
|
||||
// lm_studio and llama_cpp ship inside the SHA-pinned distribution and have no consent record.
|
||||
assert.equal(checkEgressHost(undefined, 'http://localhost:11434').allowed, true);
|
||||
assert.equal(checkEgressHost(null, 'http://localhost:11434').allowed, true);
|
||||
});
|
||||
|
||||
test('a record predating the field ALLOWS — absence must not force re-consent', () => {
|
||||
// D4 rule 5: an absent field must not perturb consent, or every installed capability
|
||||
// re-prompts on upgrade.
|
||||
assert.equal(checkEgressHost('', 'http://localhost:8080').allowed, true);
|
||||
});
|
||||
|
||||
test('a matching destination proceeds', () => {
|
||||
const r = checkEgressHost('http://localhost:8080', 'http://localhost:8080');
|
||||
assert.equal(r.allowed, true);
|
||||
});
|
||||
|
||||
test('a changed destination BLOCKS and names both hosts', () => {
|
||||
const r = checkEgressHost('http://localhost:8080', 'http://evil.example');
|
||||
assert.equal(r.allowed, false);
|
||||
assert.equal(r.consentedHost, 'http://localhost:8080');
|
||||
assert.equal(r.currentHost, 'http://evil.example');
|
||||
});
|
||||
|
||||
test('cosmetic host edits are not a change', () => {
|
||||
for (const [a, b] of [
|
||||
['http://localhost:8080', 'http://localhost:8080/'],
|
||||
['http://a.com:80', 'http://a.com'],
|
||||
['http://A.com', 'http://a.com'],
|
||||
]) {
|
||||
assert.equal(checkEgressHost(a, b).allowed, true, `${a} vs ${b}`);
|
||||
}
|
||||
});
|
||||
|
||||
test('a non-string consented value is treated as absent, never coerced', () => {
|
||||
for (const v of [42, {}, [], true]) {
|
||||
assert.equal(checkEgressHost(v, 'http://a.com').allowed, true);
|
||||
}
|
||||
});
|
||||
|
||||
test('a blocked lane never reaches the network and writes no review', async () => {
|
||||
const p = plan('ollama');
|
||||
const d = deps();
|
||||
const r = await runLane(p, d, { consentedHost: 'http://elsewhere.example', repoRoot: ROOT });
|
||||
assert.equal(r.ok, false);
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.EGRESS_HOST_CHANGED);
|
||||
assert.equal(d.files[p.reviewPath], undefined, 'a blocked lane must not write a review');
|
||||
assert.ok(d.warnings.some((w) => w.includes('elsewhere.example')));
|
||||
});
|
||||
|
||||
test('a spawn lane skips the host check entirely', async () => {
|
||||
const p = plan('qwen');
|
||||
const d = deps({ spawn: () => ({ status: 0, stdout: 'review', stderr: '' }) });
|
||||
// A stale host on a spawn lane must be inert, not a block.
|
||||
const r = await runLane(p, d, { consentedHost: 'http://stale.example', repoRoot: ROOT });
|
||||
assert.equal(r.ok, true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('runner — probe (ADR-2782 D7)', () => {
|
||||
test('command-exists both ways', async () => {
|
||||
const p = plan('gemini');
|
||||
assert.equal((await probeLane(p, deps({ hasBinary: () => true }))).available, true);
|
||||
const miss = await probeLane(p, deps({ hasBinary: () => false }));
|
||||
assert.equal(miss.available, false);
|
||||
assert.equal(miss.reason, LANE_UNAVAILABLE.MISSING_BINARY);
|
||||
});
|
||||
|
||||
test('command-capability accepts the right tool and REJECTS the wrong one', async () => {
|
||||
// This is the entire reason D7 ships wider than existence: `kimi` is claimed by both Kimi Code
|
||||
// CLI and the legacy Python kimi-cli, and an existence-only probe registers the wrong tool.
|
||||
const p = plan('kimi-code');
|
||||
const real = await probeLane(p, deps({
|
||||
spawn: () => ({ status: 0, stdout: 'usage: kimi --output-format json -p', stderr: '' }),
|
||||
}));
|
||||
assert.equal(real.available, true);
|
||||
|
||||
const legacy = await probeLane(p, deps({
|
||||
spawn: () => ({ status: 0, stdout: 'usage: kimi --print --work-dir DIR', stderr: '' }),
|
||||
}));
|
||||
assert.equal(legacy.available, false);
|
||||
assert.equal(legacy.reason, LANE_UNAVAILABLE.PROBE_FAILED);
|
||||
});
|
||||
|
||||
test('a capability probe that times out reports unavailable, never hangs', async () => {
|
||||
// The original probe (closed PR #2776) was an unbounded `kimi --help | grep` that ran on EVERY
|
||||
// review regardless of flags — a live instance of the named Unbounded Subprocesses defect.
|
||||
const p = plan('kimi-code');
|
||||
const r = await probeLane(p, deps({
|
||||
spawn: () => ({ status: null, stdout: '', stderr: '', errorCode: 'ETIMEDOUT' }),
|
||||
}));
|
||||
assert.equal(r.available, false);
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.PROBE_TIMEOUT);
|
||||
});
|
||||
|
||||
test('the capability probe passes the declared bound to the spawn', async () => {
|
||||
const p = plan('kimi-code');
|
||||
const d = deps({ spawn: (b, a, o) => { d.spawns.push({ b, a, o }); return { status: 0, stdout: '--output-format', stderr: '' }; } });
|
||||
await probeLane(p, d);
|
||||
const call = d.spawns[d.spawns.length - 1];
|
||||
assert.equal(typeof call.o.timeoutMs, 'number');
|
||||
assert.ok(call.o.timeoutMs > 0, 'every probe that starts a process MUST be bounded');
|
||||
});
|
||||
|
||||
test('a missing required binary is named rather than left to fail obscurely', async () => {
|
||||
const p = { ...plan('gemini'), requiresBinaries: ['jq'] };
|
||||
const r = await probeLane(p, deps({ hasBinary: (n) => n !== 'jq' }));
|
||||
assert.equal(r.available, false);
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.MISSING_REQUIRED_BINARY);
|
||||
});
|
||||
|
||||
test('no shipped lane still requires jq or curl', () => {
|
||||
// Phase 5b moved parsing to JSON.parse and HTTP to fetch. Leaving a stale requiresBinaries
|
||||
// entry would report lanes unavailable on stock Windows for a dependency they no longer use.
|
||||
for (const lane of REVIEWER_LANES) {
|
||||
for (const bin of lane.requiresBinaries) {
|
||||
assert.ok(bin !== 'jq' && bin !== 'curl', `${lane.slug} still declares ${bin}`);
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
test('http-reachable reports unreachable rather than throwing', async () => {
|
||||
const p = plan('ollama');
|
||||
const r = await probeLane(p, deps({ httpJson: async () => ({ ok: false, status: 0, body: '', error: 'ECONNREFUSED' }) }));
|
||||
assert.equal(r.available, false);
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.HOST_UNREACHABLE);
|
||||
});
|
||||
});
|
||||
|
||||
describe('runner — empty-output policy (#2494 / #2605 / #2794)', () => {
|
||||
test('a real review is written verbatim', () => {
|
||||
const p = plan('gemini');
|
||||
const d = deps();
|
||||
const r = writeReviewOrStub(p, '## Findings\nreal', d);
|
||||
assert.equal(r.stubbed, false);
|
||||
assert.ok(d.files[p.reviewPath].startsWith('## Findings'));
|
||||
});
|
||||
|
||||
test('empty output writes a stub carrying the captured stderr', () => {
|
||||
const p = plan('gemini');
|
||||
const d = deps({ files: { [`${RUN}/gsd-review-gemini.err`]: 'auth failed' } });
|
||||
const r = writeReviewOrStub(p, '', d);
|
||||
assert.equal(r.stubbed, true);
|
||||
assert.ok(d.files[p.reviewPath].includes('failed or returned empty output'));
|
||||
assert.ok(d.files[p.reviewPath].includes('auth failed'));
|
||||
});
|
||||
|
||||
test('whitespace-only output is stubbed on every lane', () => {
|
||||
// Before this, `[ ! -s file ]` counted bytes so " " rendered as a clean review on five lanes.
|
||||
for (const slug of ['gemini', 'claude', 'codex', 'qwen', 'cursor']) {
|
||||
const p = plan(slug);
|
||||
const d = deps();
|
||||
assert.equal(writeReviewOrStub(p, ' \n', d).stubbed, true, `${slug} accepted whitespace`);
|
||||
}
|
||||
});
|
||||
|
||||
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');
|
||||
const d = deps();
|
||||
writeReviewOrStub(p, '', d);
|
||||
assert.ok(/failed or returned empty output/.test(d.files[p.reviewPath]));
|
||||
});
|
||||
|
||||
test('an http lane appends the raw response body', () => {
|
||||
// An OpenAI-compatible server reports errors with a 4xx/5xx and the JSON in the BODY, so
|
||||
// stderr alone is empty and the body is the only evidence. The bash piped it into jq and lost it.
|
||||
const p = plan('ollama');
|
||||
const d = deps();
|
||||
writeReviewOrStub(p, '', d, '{"error":{"message":"model not found"}}');
|
||||
assert.ok(d.files[p.reviewPath].includes('Raw response body:'));
|
||||
assert.ok(d.files[p.reviewPath].includes('model not found'));
|
||||
});
|
||||
|
||||
test('a filesystem write failure degrades rather than crashing the run', () => {
|
||||
// Injected by making the seam throw — never chmod 0o000, which root bypasses.
|
||||
const p = plan('gemini');
|
||||
const d = deps({ writeFile: () => { throw new Error('EROFS'); } });
|
||||
assert.throws(() => writeReviewOrStub(p, 'x', d), /EROFS/);
|
||||
});
|
||||
});
|
||||
|
||||
describe('runner — opencode handler (#1936)', () => {
|
||||
test('the review is rebuilt from assistant text parts', () => {
|
||||
const stream = [
|
||||
JSON.stringify({ type: 'text', part: { text: 'first' } }),
|
||||
JSON.stringify({ type: 'text', part: { text: 'second' } }),
|
||||
].join('\n');
|
||||
assert.equal(handleOpencodeOutput(stream).review, 'first\nsecond');
|
||||
});
|
||||
|
||||
test('a malformed line is skipped, not fatal to the whole review', () => {
|
||||
// Losing an entire review to one bad line would be strictly worse than the bug this fixes.
|
||||
const stream = [
|
||||
JSON.stringify({ type: 'text', part: { text: 'kept' } }),
|
||||
'NOT JSON AT ALL',
|
||||
'{"truncated":',
|
||||
JSON.stringify({ type: 'text', part: { text: 'also kept' } }),
|
||||
].join('\n');
|
||||
assert.equal(handleOpencodeOutput(stream).review, 'kept\nalso kept');
|
||||
});
|
||||
|
||||
test('a zero-output turn surfaces the stop reason and token count', () => {
|
||||
const stream = JSON.stringify({ type: 'step_finish', part: { reason: 'stop', tokens: { output: 0 } } });
|
||||
const r = handleOpencodeOutput(stream);
|
||||
assert.equal(r.review, '');
|
||||
assert.ok(r.diagnostic.includes('stop'));
|
||||
assert.ok(r.diagnostic.includes('0'));
|
||||
});
|
||||
|
||||
test('the raw JSON envelope never becomes the review', async () => {
|
||||
// The regression this locks: a plain stdout copy would write the JSON stream into REVIEWS.md.
|
||||
const p = plan('opencode');
|
||||
const stream = JSON.stringify({ type: 'text', part: { text: 'THE REVIEW' } });
|
||||
const d = deps({ spawn: () => ({ status: 0, stdout: stream, stderr: '' }) });
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(d.files[p.reviewPath].trim(), 'THE REVIEW');
|
||||
assert.ok(!d.files[p.reviewPath].includes('"type"'));
|
||||
});
|
||||
|
||||
test('CRLF in the stream is handled', () => {
|
||||
const stream = [
|
||||
JSON.stringify({ type: 'text', part: { text: 'a' } }),
|
||||
JSON.stringify({ type: 'text', part: { text: 'b' } }),
|
||||
].join('\r\n');
|
||||
assert.equal(handleOpencodeOutput(stream).review, 'a\nb');
|
||||
});
|
||||
});
|
||||
|
||||
describe('runner — antigravity handler (#2073 / #2176)', () => {
|
||||
const CACHE = '/home/u/.gemini/antigravity-cli/cache/last_conversations.json';
|
||||
const TX = (id) => `/home/u/.gemini/antigravity-cli/brain/${id}/.system_generated/logs/transcript.jsonl`;
|
||||
const entry = (content) =>
|
||||
JSON.stringify({ source: 'MODEL', status: 'DONE', type: 'PLANNER_RESPONSE', content });
|
||||
|
||||
test('the watermark prevents a PRIOR run’s response leaking in as this one', () => {
|
||||
// Without it the fallback reads the last PLANNER_RESPONSE regardless of when it was written,
|
||||
// silently presenting a stale review as the current one.
|
||||
const files = {
|
||||
[CACHE]: JSON.stringify({ [ROOT]: 'c1' }),
|
||||
[TX('c1')]: [entry('STALE FROM LAST RUN')].join('\n'),
|
||||
};
|
||||
const d = deps({ files });
|
||||
const got = antigravityTranscriptFallback(ROOT, { convId: 'c1', lines: 1 }, d);
|
||||
assert.equal(got, '', 'nothing was appended after the watermark, so nothing may be returned');
|
||||
});
|
||||
|
||||
test('a response appended after the watermark IS returned', () => {
|
||||
const files = {
|
||||
[CACHE]: JSON.stringify({ [ROOT]: 'c1' }),
|
||||
[TX('c1')]: [entry('old'), entry('THIS RUN')].join('\n'),
|
||||
};
|
||||
const d = deps({ files });
|
||||
assert.equal(antigravityTranscriptFallback(ROOT, { convId: 'c1', lines: 1 }, d), 'THIS RUN');
|
||||
});
|
||||
|
||||
test('a new conversation id means every line is new (skip 0)', () => {
|
||||
const files = {
|
||||
[CACHE]: JSON.stringify({ [ROOT]: 'c2' }),
|
||||
[TX('c2')]: [entry('FRESH SESSION')].join('\n'),
|
||||
};
|
||||
const d = deps({ files });
|
||||
assert.equal(antigravityTranscriptFallback(ROOT, { convId: 'c1', lines: 9 }, d), 'FRESH SESSION');
|
||||
});
|
||||
|
||||
test('workspace lookup is case-insensitive', () => {
|
||||
const files = {
|
||||
[CACHE]: JSON.stringify({ '/REPO': 'c1' }),
|
||||
[TX('c1')]: [entry('found')].join('\n'),
|
||||
};
|
||||
assert.equal(antigravityTranscriptFallback('/repo', { convId: '', lines: 0 }, deps({ files })), 'found');
|
||||
});
|
||||
|
||||
test('a missing cache or transcript degrades to empty, never throws', () => {
|
||||
assert.equal(antigravityTranscriptFallback(ROOT, { convId: '', lines: 0 }, deps()), '');
|
||||
const d = deps({ files: { [CACHE]: 'NOT JSON' } });
|
||||
assert.equal(antigravityTranscriptFallback(ROOT, { convId: '', lines: 0 }, d), '');
|
||||
});
|
||||
|
||||
test('the blind-review marker is anchored to the head of the output', () => {
|
||||
assert.ok(stampBlindReview('REVIEWED-WITHOUT-REPO-ACCESS\nbody').startsWith('> [reviewed-without-repo-access]'));
|
||||
});
|
||||
|
||||
test('a review that merely QUOTES the marker further down is NOT stamped', () => {
|
||||
// A grounded review of this very file would otherwise be mis-stamped and down-weighted.
|
||||
const quoting = ['1', '2', '3', '4', '5', '6', 'we look for REVIEWED-WITHOUT-REPO-ACCESS here'].join('\n');
|
||||
assert.ok(!stampBlindReview(quoting).startsWith('>'));
|
||||
});
|
||||
|
||||
test('the scratch-dir tell requires a workspace DECLARATION, not a mention', () => {
|
||||
const declared = 'my working directory is /home/u/.gemini/antigravity-cli/scratch so I could not read';
|
||||
assert.ok(stampBlindReview(declared).startsWith('>'));
|
||||
const mention = 'the path .gemini/antigravity-cli/scratch appears in the plan under review';
|
||||
assert.ok(!stampBlindReview(mention).startsWith('>'));
|
||||
});
|
||||
|
||||
test('a non-zero exit discards partial output so the fallback can take over', async () => {
|
||||
// The spawn APPENDS to the transcript, as the real `agy` does. That ordering is the whole
|
||||
// point of the watermark: only what this run wrote may be read back. A test that pre-seeds the
|
||||
// response instead would be asserting that a STALE entry leaks through — the exact bug the
|
||||
// watermark exists to prevent — so it must be written this way round.
|
||||
const p = plan('antigravity');
|
||||
const files = {
|
||||
[CACHE]: JSON.stringify({ [ROOT]: 'c1' }),
|
||||
[TX('c1')]: [entry('from a PREVIOUS run')].join('\n'),
|
||||
};
|
||||
const d = deps({
|
||||
files,
|
||||
spawn: () => {
|
||||
files[TX('c1')] = [entry('from a PREVIOUS run'), entry('FROM TRANSCRIPT')].join('\n');
|
||||
return { status: 124, stdout: 'partial garbage', stderr: '' };
|
||||
},
|
||||
});
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.ok(d.files[p.reviewPath].includes('FROM TRANSCRIPT'));
|
||||
assert.ok(!d.files[p.reviewPath].includes('partial garbage'), 'rc!=0 must discard stdout');
|
||||
assert.ok(!d.files[p.reviewPath].includes('PREVIOUS'), 'the pre-run entry must stay invisible');
|
||||
});
|
||||
});
|
||||
|
||||
describe('runner — openai-compatible handler', () => {
|
||||
test('the configured model is used and discovery is skipped', async () => {
|
||||
const p = plan('ollama', { 'review.models.ollama': 'pinned' });
|
||||
let posted = null;
|
||||
const d = deps({
|
||||
httpJson: async (url, o) => {
|
||||
if (o.method === 'POST') { posted = JSON.parse(o.body); return { ok: true, status: 200, body: JSON.stringify({ choices: [{ message: { content: 'R' } }] }) }; }
|
||||
return { ok: true, status: 200, body: JSON.stringify({ data: [{ id: 'discovered' }] }) };
|
||||
},
|
||||
});
|
||||
const r = await runOpenAiCompatible(p, 'PROMPT', d);
|
||||
assert.equal(posted.model, 'pinned');
|
||||
assert.equal(r.review, 'R');
|
||||
});
|
||||
|
||||
test('an unset model discovers the first from /v1/models', async () => {
|
||||
const p = plan('ollama');
|
||||
let posted = null;
|
||||
const d = deps({
|
||||
httpJson: async (url, o) => {
|
||||
if (o.method === 'POST') { posted = JSON.parse(o.body); return { ok: true, status: 200, body: JSON.stringify({ choices: [{ message: { content: 'R' } }] }) }; }
|
||||
return { ok: true, status: 200, body: JSON.stringify({ data: [{ id: 'discovered' }] }) };
|
||||
},
|
||||
});
|
||||
await runOpenAiCompatible(p, 'P', d);
|
||||
assert.equal(posted.model, 'discovered');
|
||||
});
|
||||
|
||||
test('discovery failure falls back to the declared fallbackModel', async () => {
|
||||
const p = plan('ollama');
|
||||
let posted = null;
|
||||
const d = deps({
|
||||
httpJson: async (url, o) => {
|
||||
if (o.method === 'POST') { posted = JSON.parse(o.body); return { ok: true, status: 200, body: '{}' }; }
|
||||
return { ok: false, status: 0, body: '', error: 'refused' };
|
||||
},
|
||||
});
|
||||
await runOpenAiCompatible(p, 'P', d);
|
||||
assert.equal(posted.model, 'llama3');
|
||||
});
|
||||
|
||||
test('a served-model mismatch warns without failing the review', async () => {
|
||||
const p = plan('lm_studio', { 'review.models.lm_studio': 'asked' });
|
||||
const d = deps({
|
||||
httpJson: async () => ({ ok: true, status: 200, body: JSON.stringify({ model: 'served', choices: [{ message: { content: 'R' } }] }) }),
|
||||
});
|
||||
const r = await runOpenAiCompatible(p, 'P', d);
|
||||
assert.equal(r.review, 'R');
|
||||
assert.ok(d.warnings.some((w) => w.includes('served') && w.includes('asked')));
|
||||
});
|
||||
|
||||
test('an HTTP error body is preserved for the stub', async () => {
|
||||
const p = plan('ollama');
|
||||
const d = deps({
|
||||
httpJson: async (url, o) =>
|
||||
o.method === 'POST'
|
||||
? { ok: false, status: 404, body: '{"error":"no such model"}' }
|
||||
: { ok: false, status: 0, body: '' },
|
||||
});
|
||||
const r = await runOpenAiCompatible(p, 'P', d);
|
||||
assert.equal(r.review, '');
|
||||
assert.ok(r.rawBody.includes('no such model'));
|
||||
});
|
||||
|
||||
test('a non-JSON response body does not throw', async () => {
|
||||
const p = plan('ollama');
|
||||
const d = deps({ httpJson: async () => ({ ok: true, status: 200, body: '<html>502</html>' }) });
|
||||
const r = await runOpenAiCompatible(p, 'P', d);
|
||||
assert.equal(r.review, '');
|
||||
assert.ok(r.rawBody.includes('502'));
|
||||
});
|
||||
});
|
||||
|
||||
describe('runner — orchestration', () => {
|
||||
test('an unavailable lane requested EXPLICITLY is surfaced (D4 carve-out)', async () => {
|
||||
const p = plan('gemini');
|
||||
const d = deps({ hasBinary: () => false });
|
||||
const r = await runLane(p, d, { repoRoot: ROOT, explicitlyRequested: true });
|
||||
assert.equal(r.ok, false);
|
||||
assert.ok(d.warnings.some((w) => w.includes('explicitly requested')));
|
||||
});
|
||||
|
||||
test('an unavailable lane nobody asked for is quiet but still reported', async () => {
|
||||
const p = plan('gemini');
|
||||
const d = deps({ hasBinary: () => false });
|
||||
const r = await runLane(p, d, { repoRoot: ROOT, explicitlyRequested: false });
|
||||
assert.equal(r.ok, false);
|
||||
assert.equal(r.reason, LANE_UNAVAILABLE.MISSING_BINARY);
|
||||
assert.deepStrictEqual(d.warnings, []);
|
||||
});
|
||||
|
||||
test('a file-arg lane reads its review from the file, not stdout', async () => {
|
||||
// Codex writes via -o and its stdout carries Windows teardown noise after the final message
|
||||
// (#1698); a stdout redirect would append that to a non-empty file and slip past the guard.
|
||||
const p = plan('codex');
|
||||
const d = deps({
|
||||
files: { [`${RUN}/gsd-review-codex.md`]: 'FROM FILE' },
|
||||
spawn: () => ({ status: 0, stdout: 'TEARDOWN NOISE', stderr: '' }),
|
||||
});
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.ok(d.files[p.reviewPath].includes('FROM FILE'));
|
||||
assert.ok(!d.files[p.reviewPath].includes('TEARDOWN NOISE'));
|
||||
});
|
||||
|
||||
test('stderr is always captured to the sidecar, never discarded', async () => {
|
||||
const p = plan('gemini');
|
||||
const d = deps({ spawn: () => ({ status: 0, stdout: 'R', stderr: 'a warning' }) });
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(d.files[p.errPath], 'a warning');
|
||||
});
|
||||
|
||||
test('the prompt reaches stdin for a stdin lane', async () => {
|
||||
const p = plan('gemini');
|
||||
const d = deps({
|
||||
files: { [`${RUN}/gsd-review-prompt.md`]: 'THE PLAN' },
|
||||
spawn: (b, a, o) => { d.spawns.push({ b, a, o }); return { status: 0, stdout: 'R', stderr: '' }; },
|
||||
});
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(d.spawns[0].o.input, 'THE PLAN');
|
||||
});
|
||||
|
||||
test('a prompt-less lane is fed nothing', async () => {
|
||||
const p = plan('coderabbit');
|
||||
const d = deps({
|
||||
files: { [`${RUN}/gsd-review-prompt.md`]: 'THE PLAN' },
|
||||
spawn: (b, a, o) => { d.spawns.push({ b, a, o }); return { status: 0, stdout: 'R', stderr: '' }; },
|
||||
});
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
assert.equal(d.spawns[0].o.input, undefined);
|
||||
});
|
||||
|
||||
test('every spawn carries a positive timeout', async () => {
|
||||
// DEFECT.UNBOUNDED-SUBPROCESS: a frozen sync spawn cannot be interrupted and hangs a whole CI
|
||||
// chunk to its 10-minute kill with `# fail 0` and no `not ok`.
|
||||
for (const lane of REVIEWER_LANES.filter((l) => l.transport === 'spawn')) {
|
||||
const p = plan(lane.slug);
|
||||
const d = deps({ spawn: (b, a, o) => { d.spawns.push({ b, a, o }); return { status: 0, stdout: 'R', stderr: '' }; } });
|
||||
await runLane(p, d, { repoRoot: ROOT });
|
||||
for (const s of d.spawns) {
|
||||
assert.ok(s.o.timeoutMs > 0, `${lane.slug} spawned unbounded`);
|
||||
}
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -91,6 +91,9 @@ const RUNTIME_REVIEWER_IDS = ['antigravity', 'claude', 'codex', 'cursor', 'openc
|
||||
*/
|
||||
const LITERAL_ROSTER = [
|
||||
'antigravity', 'claude', 'coderabbit', 'codex', 'cursor', 'gemini',
|
||||
// `kimi-code` joined in Phase 5b (#2799, closes #2718) — see
|
||||
// kimiCodeIsDeclaredAndInvocableInThisPhase for why it landed here and not in 5a.
|
||||
'kimi-code',
|
||||
'llama_cpp', 'lm_studio', 'ollama', 'opencode', 'qwen',
|
||||
];
|
||||
|
||||
@@ -328,10 +331,10 @@ describe('B. The six existing runtime capabilities', () => {
|
||||
|
||||
describe('C. Roster derivation — src/review-reviewer-selection.cts', () => {
|
||||
test('rosterMembershipIsUnchangedByDerivationRefactor', () => {
|
||||
assert.equal(KNOWN_REVIEWER_SLUGS.length, 11, 'roster must be exactly 11 — not 10, not 12');
|
||||
assert.equal(KNOWN_REVIEWER_SLUGS.length, 12, 'roster must be exactly 12 — not 11, not 13');
|
||||
assert.deepEqual(
|
||||
[...KNOWN_REVIEWER_SLUGS].sort(), LITERAL_ROSTER,
|
||||
`roster must be exactly the same 11 slugs as before this phase, got: ${JSON.stringify(KNOWN_REVIEWER_SLUGS)}`,
|
||||
`roster must be exactly the declared lane set, got: ${JSON.stringify(KNOWN_REVIEWER_SLUGS)}`,
|
||||
);
|
||||
});
|
||||
|
||||
@@ -414,28 +417,43 @@ describe('D. Cross-phase invariants that must not regress', () => {
|
||||
const workflowText = fs
|
||||
.readFileSync(path.join(ROOT, 'gsd-core', 'workflows', 'review.md'), 'utf-8')
|
||||
.replace(/\r\n/g, '\n');
|
||||
const registry = [...SHIPPED.capMap.values()]
|
||||
.map((c) => c && c.reviewer && c.reviewer.slug)
|
||||
.filter((x) => typeof x === 'string' && x)
|
||||
.sort();
|
||||
const result = checkReviewerLaneParity({
|
||||
descriptor: REVIEWER_LANES,
|
||||
roster: KNOWN_REVIEWER_SLUGS,
|
||||
registry,
|
||||
workflowText,
|
||||
});
|
||||
assert.deepEqual(
|
||||
result.violations, [],
|
||||
`Phase 1's descriptor <-> roster <-> legs <-> sections parity must stay green across this migration, got: ${JSON.stringify(result.violations)}`,
|
||||
`descriptor <-> roster <-> registry parity must stay green across this migration, got: ${JSON.stringify(result.violations)}`,
|
||||
);
|
||||
assert.equal(result.ok, true);
|
||||
});
|
||||
|
||||
test('kimiCodeIsDeliberatelyNotDeclaredYet', () => {
|
||||
assert.equal(KNOWN_REVIEWER_SLUGS.includes('kimi-code'), false, '"kimi-code" must not be in the roster yet — it has no invoke_reviewers leg until 5b');
|
||||
assert.equal(KNOWN_REVIEWER_SLUGS.includes('kimi_code'), false);
|
||||
test('kimiCodeIsDeclaredAndInvocableInThisPhase', () => {
|
||||
// Phase 5a deliberately withheld this lane: declaring it there would have made it selectable
|
||||
// but NOT invocable — present in `--all`, selected, and producing an empty section for the
|
||||
// whole 5a -> 5b window. ADR-2782's phase table lands it here, with the iteration that runs it.
|
||||
assert.equal(KNOWN_REVIEWER_SLUGS.includes('kimi-code'), true);
|
||||
const cap = SHIPPED.capMap.get('kimi-code');
|
||||
assert.ok(cap, 'expected the kimi-code capability to exist (it is net-new for an unrelated, EoS reason)');
|
||||
assert.equal('reviewer' in cap, false, 'kimi-code must not declare a reviewer body in this phase');
|
||||
assert.ok(cap, 'expected the kimi-code capability to exist');
|
||||
assert.equal('reviewer' in cap, true, 'kimi-code must declare a reviewer body in 5b');
|
||||
assert.equal(cap.reviewer.slug, 'kimi-code');
|
||||
// The probe is the whole reason D7 ships wider than existence: `kimi` is claimed by BOTH the
|
||||
// Kimi Code CLI and the legacy Python kimi-cli, so an existence-only probe registers the wrong
|
||||
// tool. And it MUST be bounded — the original was an unbounded `kimi --help | grep` that ran
|
||||
// on every review regardless of flags.
|
||||
assert.equal(cap.reviewer.probe.kind, 'command-capability');
|
||||
assert.equal(cap.reviewer.probe.binary, 'kimi');
|
||||
assert.ok(cap.reviewer.probe.timeoutMs > 0, 'every process-starting probe must be bounded');
|
||||
assert.equal(
|
||||
Boolean(cap.runtime && cap.runtime.hostBehaviors && cap.runtime.hostBehaviors.reviewerCli),
|
||||
false,
|
||||
'kimi-code must not carry the legacy reviewerCli alias either',
|
||||
'the body is the declaration — the legacy reviewerCli alias must not also be set',
|
||||
);
|
||||
});
|
||||
|
||||
@@ -509,8 +527,8 @@ describe('E. Lane fidelity — no translation layer', () => {
|
||||
}
|
||||
}
|
||||
|
||||
assert.equal(REVIEWER_LANES.length, 11, 'expected exactly 11 declared descriptor lanes');
|
||||
assert.equal(bySlug.size, 11, `expected exactly 11 capabilities declaring a reviewer body, got: ${bySlug.size}`);
|
||||
assert.equal(REVIEWER_LANES.length, 12, 'expected exactly 12 declared descriptor lanes');
|
||||
assert.equal(bySlug.size, 12, `expected exactly 11 capabilities declaring a reviewer body, got: ${bySlug.size}`);
|
||||
|
||||
// Top-level scalar/array fields compared whole; the two fields that are
|
||||
// themselves nested objects (probe, invoke) are compared sub-field-by-
|
||||
@@ -611,7 +629,7 @@ describe('F. Isolated-security-review regressions', () => {
|
||||
// The module under test already imported successfully above; assert the
|
||||
// derived roster is a usable array rather than a partially-initialised value.
|
||||
assert.ok(Array.isArray([...KNOWN_REVIEWER_SLUGS]), 'roster must be iterable after module load');
|
||||
assert.equal(KNOWN_REVIEWER_SLUGS.length, 11, 'the real registry still yields the eleven lanes');
|
||||
assert.equal(KNOWN_REVIEWER_SLUGS.length, 12, 'the real registry still yields the eleven lanes');
|
||||
// And the derivation itself is total over the shapes JSON can express.
|
||||
for (const hostile of [null, undefined, [], 0, 'x', { capabilities: null }, { capabilities: [] }]) {
|
||||
assert.doesNotThrow(
|
||||
|
||||
Reference in New Issue
Block a user