0e6fa2e2cf
* test(#3118): failing-first coverage for the dead injectables and the shell projection Adds the counter-tests Wave 4 closes against, before any fix: - antigravityWatermark had zero test references. The four existing tests that look like watermark coverage hand the fallback a literal mark and never call the producer, so nothing pinned whether a real run's mark is correct. Covers all six branches plus the non-object cache classes. - Pins the fail-open: a transcript read that throws reports lines:0, indistinguishable from a genuinely empty transcript, and the consumer then replays a previous run's review as this run's. - Pins the export-line escaping across the repair, persist and win32 bash lanes, including the parity assertion that they must not diverge. - sliceCurrentPositionSection: empty-vs-absent, fenced heading, second occurrence, H3, CRLF. - Proves deps.progressProvider is inert by supplying a throwing stub to all ten transition intents. Verification through the remote runner only. Refs #3118 * fix(#3118): distinguish an unreadable transcript from an empty one antigravityWatermark's final read can throw on a transcript that indisputably exists. It returned lines:0, which is the same value a genuinely empty transcript produces, so the caller could not tell the two apart. antigravityTranscriptFallback derives its skip from that count. A mark of {convId:'c1', lines:0} for a conversation that pre-dates the run makes it skip nothing and return the last PLANNER_RESPONSE in a transcript written before this run started — a previous review presented as this one's, which is exactly what the function's own 'never stale' docstring promises cannot happen. The unreadable case now sets unreadable:true and the fallback declines for a same-conv-id unreadable mark. An absent or empty transcript is untouched: those genuinely have zero prior lines. * fix(#3118): escape the export line for the file it lands in, not the echo Three lanes emit export PATH="<dir>:$PATH". repair escaped it with escapePosixDoubleQuoted; persist and the win32 Git Bash lane escaped it with escapeSingleQuotedShellLiteral instead. The single-quoting is correct for the echo, so nothing runs when the user pastes the command. But the bytes appended to ~/.bashrc are the export line itself, and inside double quotes in an rc file a $(...) or a backtick in the directory name is command substitution that runs on every new shell. Those characters are legal in a path on both POSIX and Windows, so the path was reachable. projectPathExportLine is now the single source of that line and escapes for its final rc-file context; each lane still applies its own transport escaping on top. fish keeps the single-quote escaper — its value really does stay single-quoted. The cmd.exe lane interpolated into a cmd double-quoted string with no cmd-level escaping, so a quote closed the region and &cmd& ran. A quote is reserved on Windows and cannot appear in a real path, so there is no correct command to suggest: the win32 lanes now fail closed for one. Metacharacter-free paths render byte-identically on every lane. * fix(#3118): drop a stray carriage return and a deps field nobody reads locateCurrentPosition subtracted a fixed one byte to exclude the newline before the next heading, which assumes LF. On a CRLF document the slice kept an unpaired trailing carriage return. It now walks back over the newline and over a preceding carriage return if there is one. StateTransitionDeps also required a progressProvider that 33 sites supplied and no site ever called. A required field nothing reads widens the module's interface without changing its implementation, which is the shape epic #3051 cites as its reason for refusing blanket injection. Removed along with the ProgressRecord alias that existed only as its return type; state-document.cts's unrelated interface of the same name is untouched. * fix(#3118): stop an empty span duplicating bytes, and name the empty results Three findings from the isolated review pass. locateCurrentPosition could return end < start when the section was empty and the next heading followed with no blank line between. Every mutator splices with slice(0,start) + body + slice(end), so an inverted span duplicated the region between them — a blank line silently inserted into STATE.md on every transition, two bytes on CRLF. The span is now clamped, and an empty section is a zero-length span, which is what it always meant. The win32 fail-closed path left the installer printing 'Add it with one of:' with nothing under it. An empty shellActions folded two different facts together, so projectPathActionProjection now carries a frozen PATH_ACTION_REASON and the installer branches on it. Two empty results with different causes staying distinguishable is the subject of the epic this belongs to. fish_add_path parses a leading dash as an option, so a directory named -v printed 'No paths to add' instead of being added. Verified against fish 4.8.1: the end-of-options separator fixes it. Replaces the console-prose test the second fix first arrived with — a regex over captured stdout is what CONTRIBUTING prohibits, and the typed reason is the surface it asks for instead. * fix(#3118): escape TOML control characters, and stop a test name overstating Five findings from the two review axes. escapeTomlDoubleQuotedString escaped only backslash and quote. TOML basic strings also require U+0000-U+0008, U+000A-U+001F and U+007F to be escaped, so a value carrying a raw newline or NUL wrote a config.toml no parser accepts — rejecting the whole file, not just that value. Four of its call sites write real config. Tab stays raw; the grammar exempts it. The byte-identity test claimed every lane was unchanged for an ordinary path, which is false: fish now takes the end-of-options separator on every path, not only hostile ones. Renamed, and the one intended delta now has its own named test instead of hiding inside a claim that read as broader than it was. Also: exact-equality assertions in place of substring checks that could pass on a subtly wrong escape, newline and null-byte cases for all five quoting primitives, and a temp dir registered with t.after so it is removed when an assertion fails. * docs(#3118): add the changeset fragments * fix(#3118): degrade instead of throwing on a null conversation cache A cache file whose whole content is the literal null — what a truncated or zeroed write leaves behind — made both antigravityWatermark and antigravityTranscriptFallback throw. JSON.parse('null') succeeds, so the try/catch wrapping the parse never fired, and resolveConvId then called hasOwnProperty on null. Both functions advertise the opposite; the existing test next to them is named 'a missing cache or transcript degrades to empty, never throws'. Parsing successfully is not the same fact as the payload being usable, and a guard that only wraps the parse cannot tell them apart. resolveConvId is now total for any non-object input, so one guard covers both callers. Caught by the null case in this wave's own cache matrix. * test(#3118): correct a stale fish expectation and a parity comparison The pre-existing 'POSIX persist mode escapes single quotes' test pinned fish_add_path without the end-of-options separator this wave adds, so it asserted behavior that is no longer correct. A repo-wide scan found one such hardcoded expectation; every other site derives its expectation from the projection. The new parity test compared the token from a POSIX path against the win32 lane, which posix-normalizes its input first — two different inputs, so the tokens differed for a reason that had nothing to do with the parity it claims to check. It now derives the win32 expectation from the same input the lane receives. * docs(#3118): reword a comment the injection scanner reads as an instruction The scanner pattern act\s+as\s+(?:a|an|the)\s+ carries no word boundary, so 'the same fact as the payload' matched on the tail of 'fact'. Reworded per the documented remedy for this collision. The missing boundary is a scanner defect rather than a prose problem — any contributor writing 'fact as the' trips it — but the pattern is gate plumbing, which the sibling epic owns, so it is surfaced rather than changed here. * chore(#3118): backfill changeset pr number to 3124 * chore(#3118): backfill changeset pr number to 3124 * fix(#2784): make the negation scan single-pass and index it correctly Three defects in the negation suppression added by #3127, all in one block, none of which had a test. The pair scan was verbs.some(nouns.some(...)) with a slice and a split per pair, so it grew cubically with clause length: 1.1ms before that PR and 8462ms after, on 800 verb+noun pairs in one clause. api-coverage's property test generates documents large enough to reach the runner's 600s file cap, which is why it hangs as 'fail 0, cancelled 1' rather than failing an assertion. Every (verb, noun) window is a subset of the single widest one, so one scan of that window answers the same question in a linear pass. Verified equivalent against the old predicate over 20,000 generated clauses. Both checks also subtracted clause.start from offsets that collectTerm- Matches already returns clause-local. The first clause on a line has start 0 so it worked there and nowhere else: later clauses went negative, and slice reads a negative index from the end, so suppression silently examined unrelated text. The comment claimed 'without any API integration' was suppressed. It is not — the qualifier sits outside the two-word lookback and the noun precedes the verb. Widening the window would trade a false positive that costs one declaration line for a false negative that slips a real integration past a blocking gate, so the behavior stands and the comment now says so. Pinned by a test. The qualifier sets were also rebuilt for every line of every document.
714 lines
32 KiB
TypeScript
714 lines
32 KiB
TypeScript
/**
|
||
* Reviewer Lane Runner (ADR-2782 Phase 5b, #2799).
|
||
*
|
||
* Executes an invocation plan from `review-lane-invocation.cts`: probes the lane, spawns the binary
|
||
* or calls the HTTP endpoint, applies the empty-output policy, and dispatches the three first-party
|
||
* handlers D6 names. This is the module that replaces ~640 lines of hand-authored per-CLI bash.
|
||
*
|
||
* THREE RUNTIME DEPENDENCIES DISAPPEAR HERE, and that is a correctness win rather than a tidy-up:
|
||
* - `jq` — five legs needed it on PATH; it is absent on stock Windows/Git-Bash (#2589). Parsing is
|
||
* `JSON.parse` now.
|
||
* - `curl` — the three OpenAI-compatible legs shelled out to it. Node's `fetch` replaces it, and
|
||
* the raw response body (where an OpenAI-compatible server puts its error JSON on a 4xx/5xx
|
||
* while curl still exits 0) is no longer discarded by a pipe.
|
||
* - external `timeout` / `gtimeout` — the Antigravity leg probed for one because
|
||
* `--print-timeout` cannot fire before a session exists, and STOCK MACOS SHIPS NEITHER, so on a
|
||
* stock Mac that leg ran unbounded. `spawnSync`'s native `timeout` is always available, so D7's
|
||
* "skip the probe where no bounding mechanism exists" carve-out is no longer needed.
|
||
*
|
||
* EVERY subprocess call in this file passes `timeout` + `killSignal` + `maxBuffer`. This repo has a
|
||
* named `DEFECT.UNBOUNDED-SUBPROCESS` class (`CONTEXT.md:772`) and an unbounded sync spawn is not a
|
||
* slow test, it is a hung one: `--test-force-exit` cannot interrupt a synchronous call, so a frozen
|
||
* spawn hangs the whole CI chunk to its 10-minute kill with `# fail 0` and no `not ok` (#2099).
|
||
*
|
||
* TOTAL. No function here throws for a lane-level failure; every one returns a typed outcome. A
|
||
* reviewer lane that cannot run must produce a diagnosable artifact, because the ambiguity between
|
||
* "failed" and "ran cleanly with nothing to report" IS the defect this epic closes (#2494/#2605).
|
||
*/
|
||
|
||
import type { LanePlan, SpawnPlan, HttpPlan, LaneUnavailableReason } from './review-lane-invocation.cjs';
|
||
import {
|
||
LANE_UNAVAILABLE,
|
||
isEmptyReview,
|
||
normalizeHost,
|
||
fileRefPrompt as fileRefPromptText,
|
||
} from './review-lane-invocation.cjs';
|
||
|
||
/* ------------------------------------------------------------------ *
|
||
* Injected seams
|
||
* ------------------------------------------------------------------ */
|
||
|
||
export interface SpawnOutcome {
|
||
status: number | null;
|
||
stdout: string;
|
||
stderr: string;
|
||
/** Node sets this to ETIMEDOUT when `timeout` fired. */
|
||
errorCode?: string;
|
||
}
|
||
|
||
export interface RunnerDeps {
|
||
/** Bounded synchronous spawn. Production wires `child_process.spawnSync`. */
|
||
spawn: (binary: string, argv: string[], opts: { input?: string; timeoutMs: number }) => SpawnOutcome;
|
||
/** Bounded HTTP POST/GET returning the RAW body — never pre-parsed, so errors stay diagnosable. */
|
||
httpJson: (url: string, opts: { method: 'GET' | 'POST'; body?: string; timeoutMs: number }) =>
|
||
Promise<{ ok: boolean; status: number; body: string; error?: string }>;
|
||
readFile: (p: string) => string;
|
||
writeFile: (p: string, content: string) => void;
|
||
exists: (p: string) => boolean;
|
||
/** `command -v` equivalent. */
|
||
hasBinary: (name: string) => boolean;
|
||
/** Resolved config lookup, for the egress-host re-verification. */
|
||
configGet: (key: string) => unknown;
|
||
/** Home dir, for the Antigravity transcript paths. */
|
||
homeDir: string;
|
||
warn: (msg: string) => void;
|
||
}
|
||
|
||
export interface LaneRunResult {
|
||
slug: string;
|
||
ok: boolean;
|
||
/** Present when `ok` is false. */
|
||
reason?: LaneUnavailableReason;
|
||
detail?: string;
|
||
/** True when a diagnostic stub was written instead of a real review. */
|
||
stubbed: boolean;
|
||
}
|
||
|
||
/* ------------------------------------------------------------------ *
|
||
* D5 rules 2–4 — the egress destination is re-verified at INVOCATION
|
||
* ------------------------------------------------------------------ */
|
||
|
||
export interface EgressCheck {
|
||
allowed: boolean;
|
||
consentedHost?: string;
|
||
currentHost?: string;
|
||
}
|
||
|
||
/**
|
||
* Compare a lane's re-resolved egress destination against the one the user consented to.
|
||
*
|
||
* ADR-2782 D5 puts this on the invocation path deliberately. `hostConfigKey` names a key in
|
||
* `.planning/config.json`, which is the ONE consent-bound value living outside the SHA-pinned
|
||
* bundle: user- and CI-editable at any time, with no re-install and no integrity check. Without
|
||
* this check a lane consented against `http://localhost:8080` could be silently redirected to a
|
||
* remote host by an ordinary pull request touching a JSON file, and every subsequent review would
|
||
* egress plans, requirements, research and decisions to the new destination with no re-prompt.
|
||
*
|
||
* ABSENCE IS NOT A MISMATCH, and this is the part that must not be "hardened" later by someone who
|
||
* reads only the rule name:
|
||
* - No consent record at all ⇒ ALLOW. First-party lanes (`ollama`, `lm_studio`, `llama_cpp`) ship
|
||
* inside the SHA-pinned distribution and are never consent-gated, so blocking on a missing
|
||
* record would break every existing local-model user on upgrade.
|
||
* - A consent record predating this feature ⇒ ALLOW. Those records were written before a host was
|
||
* recorded; treating the absence as a change would force spurious re-consent across every
|
||
* installed capability, which ADR-2782 D4 rule 5 explicitly forbids.
|
||
*
|
||
* Hosts are compared NORMALIZED, so a trailing slash or an explicit `:80` is not a "destination
|
||
* change". A warning that fires on cosmetic edits is a warning users learn to dismiss.
|
||
*/
|
||
export function checkEgressHost(consentedHost: unknown, currentHost: string): EgressCheck {
|
||
const consented = typeof consentedHost === 'string' ? normalizeHost(consentedHost) : '';
|
||
if (!consented) return { allowed: true };
|
||
const current = normalizeHost(currentHost);
|
||
if (consented === current) return { allowed: true, consentedHost: consented, currentHost: current };
|
||
return { allowed: false, consentedHost: consented, currentHost: current };
|
||
}
|
||
|
||
/* ------------------------------------------------------------------ *
|
||
* Probe (D7) — every probe bounded, and only for a SELECTED lane
|
||
* ------------------------------------------------------------------ */
|
||
|
||
export async function probeLane(
|
||
plan: LanePlan,
|
||
deps: RunnerDeps,
|
||
): Promise<{ available: true } | { available: false; reason: LaneUnavailableReason; detail: string }> {
|
||
// Prerequisites first: a missing `jq`-style helper is a clearer failure than whatever the tool
|
||
// does without it, and reporting it costs nothing.
|
||
for (const bin of plan.requiresBinaries) {
|
||
if (!deps.hasBinary(bin)) {
|
||
return {
|
||
available: false,
|
||
reason: LANE_UNAVAILABLE.MISSING_REQUIRED_BINARY,
|
||
detail: `lane '${plan.slug}' requires '${bin}' on PATH`,
|
||
};
|
||
}
|
||
}
|
||
|
||
const probe = plan.probe;
|
||
if (!probe || typeof probe !== 'object') {
|
||
return { available: false, reason: LANE_UNAVAILABLE.PROBE_FAILED, detail: 'lane declares no probe' };
|
||
}
|
||
|
||
switch (probe.kind) {
|
||
case 'command-exists':
|
||
return deps.hasBinary(probe.binary)
|
||
? { available: true }
|
||
: {
|
||
available: false,
|
||
reason: LANE_UNAVAILABLE.MISSING_BINARY,
|
||
detail: `'${probe.binary}' not found on PATH`,
|
||
};
|
||
|
||
case 'command-capability': {
|
||
// Existence alone is structurally insufficient here: `kimi` is claimed by BOTH the Kimi Code
|
||
// CLI and the legacy Python kimi-cli, and an existence-only probe registers the wrong tool.
|
||
if (!deps.hasBinary(probe.binary)) {
|
||
return {
|
||
available: false,
|
||
reason: LANE_UNAVAILABLE.MISSING_BINARY,
|
||
detail: `'${probe.binary}' not found on PATH`,
|
||
};
|
||
}
|
||
const out = deps.spawn(probe.binary, ['--help'], { timeoutMs: probe.timeoutMs });
|
||
if (out.errorCode === 'ETIMEDOUT') {
|
||
return {
|
||
available: false,
|
||
reason: LANE_UNAVAILABLE.PROBE_TIMEOUT,
|
||
detail: `'${probe.binary} --help' exceeded ${probe.timeoutMs}ms`,
|
||
};
|
||
}
|
||
const help = `${out.stdout}\n${out.stderr}`;
|
||
return help.includes(probe.needle)
|
||
? { available: true }
|
||
: {
|
||
available: false,
|
||
reason: LANE_UNAVAILABLE.PROBE_FAILED,
|
||
detail: `'${probe.binary}' is on PATH but its --help lacks '${probe.needle}' — this is a different tool sharing the name`,
|
||
};
|
||
}
|
||
|
||
case 'http-reachable': {
|
||
// Only a STRING config value names a host. `String(unknown)` would turn an object into the
|
||
// literal '[object Object]' and probe that as a URL — a nonsense destination reported as
|
||
// "unreachable" rather than as the misconfiguration it is.
|
||
const configured = deps.configGet(probe.hostConfigKey);
|
||
const base = normalizeHost(
|
||
(typeof configured === 'string' ? configured : '') ||
|
||
(plan.transport === 'openai-http' ? plan.host : ''),
|
||
);
|
||
if (!base) {
|
||
return { available: false, reason: LANE_UNAVAILABLE.HOST_UNREACHABLE, detail: 'no host resolved' };
|
||
}
|
||
const r = await deps.httpJson(`${base}${probe.path}`, { method: 'GET', timeoutMs: probe.timeoutMs });
|
||
return r.ok
|
||
? { available: true }
|
||
: {
|
||
available: false,
|
||
reason: LANE_UNAVAILABLE.HOST_UNREACHABLE,
|
||
detail: `${base}${probe.path} unreachable: ${r.error ?? `HTTP ${r.status}`}`,
|
||
};
|
||
}
|
||
|
||
default:
|
||
return {
|
||
available: false,
|
||
reason: LANE_UNAVAILABLE.PROBE_FAILED,
|
||
detail: `unknown probe kind '${String((probe as { kind?: unknown }).kind)}'`,
|
||
};
|
||
}
|
||
}
|
||
|
||
/* ------------------------------------------------------------------ *
|
||
* Empty-output policy (#2494 / #2605 / #2794)
|
||
* ------------------------------------------------------------------ */
|
||
|
||
/**
|
||
* Write the review, or a diagnostic stub when the lane produced nothing usable.
|
||
*
|
||
* The stub is not cosmetic. Before #2494/#2605 a failed lane left a zero-byte file, `write_reviews`
|
||
* rendered it as "a reviewer that ran cleanly with nothing to report", and the lane vanished from
|
||
* the cross-AI consensus while `present_results` reported success — a review blind in one eye. The
|
||
* stub keeps its "failed or returned empty output" header precisely so it can never be mistaken for
|
||
* a real review.
|
||
*
|
||
* `extraDiagnostics` carries the raw HTTP response body for the OpenAI-compatible lanes: an error
|
||
* from such a server arrives with HTTP 4xx/5xx and the JSON in the BODY, so stderr alone is empty
|
||
* and the body is the only evidence.
|
||
*/
|
||
export function writeReviewOrStub(
|
||
plan: LanePlan,
|
||
content: string,
|
||
deps: RunnerDeps,
|
||
extraDiagnostics?: string,
|
||
): { stubbed: boolean } {
|
||
if (!isEmptyReview(content)) {
|
||
deps.writeFile(plan.reviewPath, content.endsWith('\n') ? content : `${content}\n`);
|
||
return { stubbed: false };
|
||
}
|
||
const stderr = deps.exists(plan.errPath) ? deps.readFile(plan.errPath) : '';
|
||
const parts = [`${plan.slug} review failed or returned empty output. stderr:`, stderr];
|
||
if (extraDiagnostics) parts.push('Raw response body:', extraDiagnostics);
|
||
deps.writeFile(plan.reviewPath, `${parts.join('\n')}\n`);
|
||
return { stubbed: true };
|
||
}
|
||
|
||
/* ------------------------------------------------------------------ *
|
||
* Handlers (D6) — named first-party code, never conditionals in data
|
||
* ------------------------------------------------------------------ */
|
||
|
||
/**
|
||
* `opencode` — reconstruct the review from the assistant `text` parts of a `--format json` stream.
|
||
*
|
||
* `--format json` is the PRIMARY invocation, not a fallback: OpenCode's default `build` agent is an
|
||
* agentic coder, and on a large review prompt it may run a few `read` tool calls then end its turn
|
||
* with ZERO output tokens, so `--format default` yields empty stdout and the reviewer is silently
|
||
* lost (#1936). When no assistant text was emitted, surface the stop reason and output-token count
|
||
* so the failure is diagnosable rather than a generic empty stub.
|
||
*
|
||
* The stream is JSONL-ish: one JSON value per line. A line that does not parse is SKIPPED rather
|
||
* than failing the lane — a partial stream still carries usable review text, and losing the whole
|
||
* review to one malformed line would be strictly worse than the bug this handler exists to fix.
|
||
*/
|
||
export function handleOpencodeOutput(rawStdout: string): { review: string; diagnostic: string } {
|
||
const texts: string[] = [];
|
||
let stopReason = '?';
|
||
let outputTokens = '?';
|
||
for (const line of String(rawStdout ?? '').split(/\r?\n/)) {
|
||
const trimmed = line.trim();
|
||
if (!trimmed) continue;
|
||
let evt: unknown;
|
||
try {
|
||
evt = JSON.parse(trimmed);
|
||
} catch {
|
||
continue;
|
||
}
|
||
if (evt === null || typeof evt !== 'object') continue;
|
||
const e = evt as { type?: unknown; part?: unknown };
|
||
const part = (e.part ?? {}) as { text?: unknown; reason?: unknown; tokens?: unknown };
|
||
if (e.type === 'text' && typeof part.text === 'string') {
|
||
// An EMPTY string is kept, not skipped. The shipped jq was `.part.text // empty`, and `//`
|
||
// only substitutes for `false`/`null` — an empty string is truthy in jq, so it survived and
|
||
// contributed a blank line. Dropping it here would quietly close up blank lines between
|
||
// parts, which is a real (if cosmetic) divergence from the behaviour being ported.
|
||
texts.push(part.text);
|
||
} else if (e.type === 'step_finish') {
|
||
if (typeof part.reason === 'string') stopReason = part.reason;
|
||
const tokens = (part.tokens ?? {}) as { output?: unknown };
|
||
if (typeof tokens.output === 'number') outputTokens = String(tokens.output);
|
||
}
|
||
}
|
||
return {
|
||
review: texts.join('\n'),
|
||
diagnostic: `stop reason=${stopReason}, output tokens=${outputTokens}`,
|
||
};
|
||
}
|
||
|
||
/** One `PLANNER_RESPONSE` line of an Antigravity transcript. */
|
||
interface TranscriptEntry {
|
||
source?: unknown;
|
||
status?: unknown;
|
||
type?: unknown;
|
||
content?: unknown;
|
||
}
|
||
|
||
/**
|
||
* `antigravity` — three layers, a two-level timeout, and a stale-response watermark.
|
||
*
|
||
* Layer 1 is stdout, which works on macOS/Linux/WSL. On native Windows `agy -p` silently produces
|
||
* no stdout despite the API call succeeding (an upstream `text_drip.go` non-TTY flush bug), so
|
||
* layer 2 reads the transcript `agy` always persists to disk. Layer 3 is a diagnostic stub.
|
||
*
|
||
* THE WATERMARK IS THE SUBTLE PART. Without it, layer 2 reads the last `PLANNER_RESPONSE` in the
|
||
* transcript regardless of when it was written — including one from a PREVIOUS invocation in the
|
||
* same workspace, silently presenting a stale review as this run's. So the transcript line count is
|
||
* snapshotted BEFORE the spawn and only lines appended after it are considered. If the conversation
|
||
* id changed, `agy` started a fresh session and every line is new (skip 0).
|
||
*
|
||
* `takeWatermark` must therefore be called before `runAntigravity`; the plan's outer timeout is the
|
||
* external wall-clock cap that `--print-timeout` cannot provide (it cannot fire before a session
|
||
* exists — a process can stall pre-session and outlive its own native timeout, #2073 mode 3).
|
||
*
|
||
* KNOWN LIMIT — the watermark is sequential, not concurrent. `last_conversations.json` is keyed by
|
||
* WORKSPACE, so two `/gsd:review` runs against the same repo at the same time resolve the same
|
||
* conversation id and share one transcript. The fallback then takes "the latest DONE
|
||
* PLANNER_RESPONSE after the watermark" with nothing to tell the two runs apart, so one run could
|
||
* read the other's response. This cannot be closed here: `agy` exposes no per-invocation id to
|
||
* filter on, and the transcript carries none. It is stated rather than silently tolerated because
|
||
* the guarantee this function advertises ("never stale") holds only for sequential use, and a
|
||
* future reader deserves to know which half is actually guaranteed.
|
||
*/
|
||
export function antigravityWatermark(
|
||
workspace: string,
|
||
deps: RunnerDeps,
|
||
): { convId: string; lines: number; unreadable?: boolean } {
|
||
const cachePath = `${deps.homeDir}/.gemini/antigravity-cli/cache/last_conversations.json`;
|
||
if (!deps.exists(cachePath)) return { convId: '', lines: 0 };
|
||
let cache: Record<string, unknown>;
|
||
try {
|
||
cache = JSON.parse(deps.readFile(cachePath)) as Record<string, unknown>;
|
||
} catch {
|
||
return { convId: '', lines: 0 };
|
||
}
|
||
const convId = resolveConvId(cache, workspace);
|
||
if (!convId) return { convId: '', lines: 0 };
|
||
const tx = transcriptPath(deps.homeDir, convId);
|
||
if (!deps.exists(tx)) return { convId, lines: 0 };
|
||
try {
|
||
return { convId, lines: deps.readFile(tx).split(/\r?\n/).filter((l) => l.trim()).length };
|
||
} catch {
|
||
// #3118: this conv-id pre-dates this run, so its transcript exists but this run cannot verify
|
||
// its line count. Reporting `lines: 0` would assert a fact we could not check — flag it instead
|
||
// so the fallback can decline rather than silently skip zero and replay a stale response.
|
||
return { convId, lines: 0, unreadable: true };
|
||
}
|
||
}
|
||
|
||
/**
|
||
* Workspace lookup is case-insensitive — the leg's jq did `ascii_downcase` on both sides.
|
||
*
|
||
* #3118: a successful `JSON.parse` does not by itself make the payload a usable object —
|
||
* `JSON.parse('null')` succeeds and returns `null`, so a truncated/zeroed cache file slips past
|
||
* the callers' parse-only try/catch. `typeof null === 'object'`, so the guard below must exclude
|
||
* `null` explicitly. Arrays are excluded too (not a workspace map), which also falls out of the
|
||
* `Object.entries`/`hasOwnProperty` lookups below returning nothing for array input.
|
||
*/
|
||
function resolveConvId(cache: unknown, workspace: string): string {
|
||
if (cache === null || typeof cache !== 'object') return '';
|
||
const record = cache as Record<string, unknown>;
|
||
if (Object.prototype.hasOwnProperty.call(record, workspace)) {
|
||
const direct = record[workspace];
|
||
if (typeof direct === 'string' && direct) return direct;
|
||
}
|
||
const target = workspace.toLowerCase();
|
||
for (const [k, v] of Object.entries(record)) {
|
||
if (k.toLowerCase() === target && typeof v === 'string' && v) return v;
|
||
}
|
||
return '';
|
||
}
|
||
|
||
function transcriptPath(homeDir: string, convId: string): string {
|
||
return `${homeDir}/.gemini/antigravity-cli/brain/${convId}/.system_generated/logs/transcript.jsonl`;
|
||
}
|
||
|
||
/**
|
||
* Layer 2: the newest `PLANNER_RESPONSE` written AFTER the watermark, or `''`.
|
||
*
|
||
* Returning `''` rather than the newest entry overall is the whole point — an empty result lets
|
||
* layer 3 fire with an honest diagnostic, where a stale one would be indistinguishable from a
|
||
* successful review.
|
||
*/
|
||
export function antigravityTranscriptFallback(
|
||
workspace: string,
|
||
mark: { convId: string; lines: number; unreadable?: boolean },
|
||
deps: RunnerDeps,
|
||
): string {
|
||
const cachePath = `${deps.homeDir}/.gemini/antigravity-cli/cache/last_conversations.json`;
|
||
if (!deps.exists(cachePath)) return '';
|
||
let cache: Record<string, unknown>;
|
||
try {
|
||
cache = JSON.parse(deps.readFile(cachePath)) as Record<string, unknown>;
|
||
} catch {
|
||
return '';
|
||
}
|
||
const convId = resolveConvId(cache, workspace);
|
||
if (!convId) return '';
|
||
// #3118: the watermark could not read this conversation's transcript, so there is no trustworthy
|
||
// skip for it. Declining is the fail-closed answer; skipping 0 would replay a prior run's review.
|
||
if (mark.unreadable === true && convId === mark.convId) return '';
|
||
const tx = transcriptPath(deps.homeDir, convId);
|
||
if (!deps.exists(tx)) return '';
|
||
let lines: string[];
|
||
try {
|
||
lines = deps.readFile(tx).split(/\r?\n/).filter((l) => l.trim());
|
||
} catch {
|
||
return '';
|
||
}
|
||
// Same conv-id ⇒ only lines beyond the watermark are this run's. Different id ⇒ fresh session,
|
||
// so every line is new.
|
||
const skip = convId === mark.convId ? mark.lines : 0;
|
||
let latest = '';
|
||
for (const line of lines.slice(skip)) {
|
||
let entry: TranscriptEntry;
|
||
try {
|
||
entry = JSON.parse(line) as TranscriptEntry;
|
||
} catch {
|
||
continue;
|
||
}
|
||
if (
|
||
entry.source === 'MODEL' &&
|
||
entry.status === 'DONE' &&
|
||
entry.type === 'PLANNER_RESPONSE' &&
|
||
typeof entry.content === 'string'
|
||
) {
|
||
latest = entry.content;
|
||
}
|
||
}
|
||
return latest;
|
||
}
|
||
|
||
/**
|
||
* Antigravity's prompt variant (#2176).
|
||
*
|
||
* Differs from the standard `argv-file-ref` text by one clause, and that clause is load-bearing:
|
||
* it REQUIRES the reviewer to self-report when it cannot read the repo. Without it a blind review
|
||
* is indistinguishable from a grounded one — the reviewer happily reviews the plan text in
|
||
* isolation and its verdict is counted at full weight in the consensus. `stampBlindReview` reads
|
||
* this self-report back out.
|
||
*/
|
||
export function antigravityPrompt(promptPath: string, repoRoot: string): string {
|
||
return (
|
||
`Read the file at ${promptPath} in full and carry out the review request it contains. ` +
|
||
`The repository under review is at ${repoRoot} — resolve every relative file path in the ` +
|
||
`review request against that absolute root and verify claims against those files. ` +
|
||
`If you cannot read files under ${repoRoot}, begin your output with the exact line ` +
|
||
`REVIEWED-WITHOUT-REPO-ACCESS before the review. Output only the resulting markdown review. ` +
|
||
`Do not edit any files.`
|
||
);
|
||
}
|
||
|
||
/**
|
||
* Antigravity's argv adjustments — the two things data cannot express for this lane.
|
||
*
|
||
* 1. `--add-dir <repo>`, CAPABILITY-PROBED. Without it agy's permission context never receives the
|
||
* cwd repo: the agent anchors on its own `~/.gemini/antigravity-cli/scratch` dir and reviews the
|
||
* plan text in isolation, which is exactly what the Review Instructions forbid (#2176). It is
|
||
* probed rather than assumed because older agy builds reject the unknown flag outright, and a
|
||
* lane that fails to start is worse than one that runs on the prompt anchor alone.
|
||
* 2. The self-report prompt variant above, swapped in for the standard file-ref text.
|
||
*
|
||
* Both are argv shape, so they belong here rather than in the descriptor: expressing "add this flag
|
||
* only if the binary's --help mentions it" as data would need a conditional, which is precisely
|
||
* what the named-handler seam exists to absorb (ADR-2782 D6).
|
||
*/
|
||
export function antigravityArgv(
|
||
argv: readonly string[],
|
||
promptPath: string,
|
||
repoRoot: string,
|
||
deps: RunnerDeps,
|
||
): string[] {
|
||
const standard = fileRefPromptText(promptPath, repoRoot);
|
||
const out = argv.map((a) => (a === standard ? antigravityPrompt(promptPath, repoRoot) : a));
|
||
|
||
let supportsAddDir = false;
|
||
try {
|
||
const help = deps.spawn('agy', ['--help'], { timeoutMs: 5_000 });
|
||
supportsAddDir = `${help.stdout}\n${help.stderr}`.includes('--add-dir');
|
||
} catch {
|
||
supportsAddDir = false;
|
||
}
|
||
if (!supportsAddDir) return out;
|
||
|
||
// Insert before the trailing `-p <prompt>` pair so the prompt stays last, as the leg had it.
|
||
const pIdx = out.lastIndexOf('-p');
|
||
if (pIdx === -1) return [...out, '--add-dir', repoRoot];
|
||
return [...out.slice(0, pIdx), '--add-dir', repoRoot, ...out.slice(pIdx)];
|
||
}
|
||
|
||
/**
|
||
* Layer 3's diagnostic: what `agy` itself logged, when both stdout and the transcript were empty.
|
||
*
|
||
* #2073 mode 2 is invisible without this. A pinned model that 404s server-side exits **0** with
|
||
* empty stdout AND an empty transcript — every signal the other two layers read says "clean run,
|
||
* nothing to report". The only evidence is in `agy`'s own log, so a bare generic stub would leave
|
||
* the user with a silently missing reviewer and nothing to diagnose.
|
||
*
|
||
* Mode 3 (a pre-session stall, which `--print-timeout` cannot bound because it cannot fire before a
|
||
* session exists) leaves no log line at all, so its tell is stated rather than searched for.
|
||
*/
|
||
export function antigravityDiagnostic(deps: RunnerDeps): string {
|
||
const lines = [
|
||
'Antigravity review failed or returned empty output.',
|
||
];
|
||
const logPath = `${deps.homeDir}/.gemini/antigravity-cli/cli.log`;
|
||
if (deps.exists(logPath)) {
|
||
try {
|
||
const hits = deps
|
||
.readFile(logPath)
|
||
.split(/\r?\n/)
|
||
.filter((l) => /agent executor error|NOT_FOUND|Publisher model/i.test(l))
|
||
.slice(-3);
|
||
if (hits.length) {
|
||
lines.push(
|
||
"agy log hint (pinned model may be unavailable — run 'agy models' and set review.models.agy):",
|
||
...hits,
|
||
);
|
||
}
|
||
} catch {
|
||
/* an unreadable log is not worth failing the lane over */
|
||
}
|
||
}
|
||
lines.push(
|
||
'If no agy run started, that is the pre-session-stall case: check whether a new ' +
|
||
'~/.gemini/antigravity-cli/brain/<conv-id>/ dir appeared within ~30s of launch.',
|
||
);
|
||
return lines.join('\n');
|
||
}
|
||
|
||
/**
|
||
* Stamp a machine-readable marker when the reviewer plainly ran without repo access (#2176).
|
||
*
|
||
* BOTH tells are ANCHORED, and that anchoring is load-bearing: a grounded review that merely QUOTES
|
||
* `REVIEWED-WITHOUT-REPO-ACCESS` — reviewing this very file, say — must never be mis-stamped. So
|
||
* the self-report is only honoured in the first five lines, and the scratch-dir tell only in a
|
||
* workspace-DECLARATION phrasing.
|
||
*/
|
||
export function stampBlindReview(review: string): string {
|
||
if (isEmptyReview(review)) return review;
|
||
const head = review.split(/\r?\n/).slice(0, 5).join('\n');
|
||
const selfReported = head.includes('REVIEWED-WITHOUT-REPO-ACCESS');
|
||
const scratchTell = /(workspace|working) (directory|dir)[\s\S]{0,40}antigravity-cli\/scratch/i.test(review);
|
||
if (!selfReported && !scratchTell) return review;
|
||
return (
|
||
'> [reviewed-without-repo-access] This reviewer ran without visibility into the repo under ' +
|
||
'review — down-weight its verdict in the Consensus Summary.\n\n' +
|
||
review
|
||
);
|
||
}
|
||
|
||
/**
|
||
* `openai-compatible` — model discovery, the chat-completions round trip, and the served-model
|
||
* mismatch warning.
|
||
*
|
||
* The raw body is returned alongside the content because an OpenAI-compatible server reports errors
|
||
* with an HTTP 4xx/5xx and the JSON in the BODY. The bash piped the response straight into `jq`,
|
||
* which discarded exactly that evidence; the stub appends it now.
|
||
*/
|
||
export async function runOpenAiCompatible(
|
||
plan: HttpPlan,
|
||
promptText: string,
|
||
deps: RunnerDeps,
|
||
): Promise<{ review: string; rawBody: string }> {
|
||
let model = plan.model;
|
||
if (!model && plan.modelsUrl) {
|
||
const listed = await deps.httpJson(plan.modelsUrl, { method: 'GET', timeoutMs: 2_000 });
|
||
if (listed.ok) {
|
||
try {
|
||
const parsed = JSON.parse(listed.body) as { data?: Array<{ id?: unknown }> };
|
||
const first = Array.isArray(parsed.data) ? parsed.data[0] : undefined;
|
||
if (first && typeof first.id === 'string' && first.id) model = first.id;
|
||
} catch {
|
||
/* fall through to the declared fallback */
|
||
}
|
||
}
|
||
}
|
||
if (!model) model = plan.fallbackModel;
|
||
|
||
const body = JSON.stringify({ model, messages: [{ role: 'user', content: promptText }] });
|
||
const res = await deps.httpJson(plan.url, { method: 'POST', body, timeoutMs: plan.timeoutMs });
|
||
if (!res.ok && !res.body) {
|
||
return { review: '', rawBody: res.error ?? `HTTP ${res.status}` };
|
||
}
|
||
let review = '';
|
||
try {
|
||
const parsed = JSON.parse(res.body) as {
|
||
model?: unknown;
|
||
choices?: Array<{ message?: { content?: unknown } }>;
|
||
};
|
||
// A server that quietly serves a different model than requested produces a review the user
|
||
// will attribute to the wrong system. Warn rather than fail — the review is still real.
|
||
if (typeof parsed.model === 'string' && parsed.model && parsed.model !== model) {
|
||
deps.warn(
|
||
`${plan.slug} served model '${parsed.model}' but '${model}' was requested. ` +
|
||
`Review may be from a different model.`,
|
||
);
|
||
}
|
||
const content = parsed.choices?.[0]?.message?.content;
|
||
if (typeof content === 'string') review = content;
|
||
} catch {
|
||
/* leave review empty — the raw body carries the diagnosis */
|
||
}
|
||
return { review, rawBody: res.body };
|
||
}
|
||
|
||
/* ------------------------------------------------------------------ *
|
||
* Orchestration
|
||
* ------------------------------------------------------------------ */
|
||
|
||
/**
|
||
* Run one resolved lane end to end.
|
||
*
|
||
* Order is deliberate: egress re-verification happens BEFORE the probe and before any spawn, so a
|
||
* lane whose destination changed never receives the plan text even once.
|
||
*/
|
||
export async function runLane(
|
||
plan: LanePlan,
|
||
deps: RunnerDeps,
|
||
opts: { consentedHost?: unknown; explicitlyRequested?: boolean; repoRoot: string },
|
||
): Promise<LaneRunResult> {
|
||
const base = { slug: plan.slug, stubbed: false };
|
||
|
||
if (plan.transport === 'openai-http') {
|
||
const egress = checkEgressHost(opts.consentedHost, plan.host);
|
||
if (!egress.allowed) {
|
||
const detail =
|
||
`lane '${plan.slug}' was consented to send plans to ${egress.consentedHost} but ` +
|
||
`${plan.hostConfigKey} now resolves to ${egress.currentHost}. Re-consent to allow the new ` +
|
||
`destination — this lane will not run against a host you did not approve.`;
|
||
deps.warn(detail);
|
||
return { ...base, ok: false, reason: LANE_UNAVAILABLE.EGRESS_HOST_CHANGED, detail };
|
||
}
|
||
}
|
||
|
||
const probed = await probeLane(plan, deps);
|
||
if (!probed.available) {
|
||
// D4's carve-out: not finding a lane nobody asked for is normal; failing to run a lane somebody
|
||
// asked for is an ERROR. Both are visible; only the second is fatal to the run.
|
||
if (opts.explicitlyRequested) deps.warn(`explicitly requested reviewer '${plan.slug}': ${probed.detail}`);
|
||
return { ...base, ok: false, reason: probed.reason, detail: probed.detail };
|
||
}
|
||
|
||
return plan.transport === 'spawn'
|
||
? runSpawnLane(plan, deps, opts.repoRoot)
|
||
: runHttpLane(plan, deps);
|
||
}
|
||
|
||
function runSpawnLane(plan: SpawnPlan, deps: RunnerDeps, repoRoot: string): LaneRunResult {
|
||
const input = plan.stdin && deps.exists(plan.stdin) ? deps.readFile(plan.stdin) : undefined;
|
||
|
||
const mark =
|
||
plan.handler === 'antigravity' ? antigravityWatermark(repoRoot, deps) : { convId: '', lines: 0 };
|
||
|
||
const argv =
|
||
plan.handler === 'antigravity'
|
||
? antigravityArgv(plan.argv, plan.promptPath, repoRoot, deps)
|
||
: plan.argv;
|
||
|
||
const out = deps.spawn(plan.binary, argv, { input, timeoutMs: plan.timeoutMs });
|
||
deps.writeFile(plan.errPath, out.stderr ?? '');
|
||
|
||
// `file-arg` lanes write the review themselves and their stdout is deliberately discarded (#1698).
|
||
let review =
|
||
plan.outputTarget.kind === 'file'
|
||
? deps.exists(plan.outputTarget.path)
|
||
? deps.readFile(plan.outputTarget.path)
|
||
: ''
|
||
: (out.stdout ?? '');
|
||
|
||
let extra: string | undefined;
|
||
|
||
if (plan.handler === 'opencode') {
|
||
const rebuilt = handleOpencodeOutput(review);
|
||
if (!isEmptyReview(rebuilt.review)) {
|
||
review = rebuilt.review;
|
||
} else {
|
||
review = '';
|
||
extra = `OpenCode review returned no assistant text (#1936: agent ended its turn with no final message).\nDiagnostic: ${rebuilt.diagnostic}`;
|
||
}
|
||
}
|
||
|
||
if (plan.handler === 'antigravity') {
|
||
// A non-zero exit (timeout kill, crash) discards partial output so the transcript fallback and
|
||
// the diagnostic stub can take over.
|
||
if (out.status !== 0) review = '';
|
||
if (isEmptyReview(review)) review = antigravityTranscriptFallback(repoRoot, mark, deps);
|
||
review = stampBlindReview(review);
|
||
// Layer 3. `emptyOutput: 'handler-owned'` means the generic stub does not fire for this lane,
|
||
// so if nothing is written here the lane goes out empty — the #2073 failure itself.
|
||
if (isEmptyReview(review)) {
|
||
deps.writeFile(plan.reviewPath, `${antigravityDiagnostic(deps)}\n`);
|
||
return { slug: plan.slug, ok: true, stubbed: true };
|
||
}
|
||
}
|
||
|
||
const { stubbed } = writeReviewOrStub(plan, review, deps, extra);
|
||
return { slug: plan.slug, ok: true, stubbed };
|
||
}
|
||
|
||
async function runHttpLane(plan: HttpPlan, deps: RunnerDeps): Promise<LaneRunResult> {
|
||
const promptText = deps.exists(plan.promptPath) ? deps.readFile(plan.promptPath) : '';
|
||
const { review, rawBody } = await runOpenAiCompatible(plan, promptText, deps);
|
||
deps.writeFile(plan.errPath, '');
|
||
const { stubbed } = writeReviewOrStub(plan, review, deps, rawBody);
|
||
return { slug: plan.slug, ok: true, stubbed };
|
||
}
|