diff --git a/orchestration/skills/quad-cli-consensus-gate.md b/orchestration/skills/quad-cli-consensus-gate.md index bbbd607..bfc0d98 100644 --- a/orchestration/skills/quad-cli-consensus-gate.md +++ b/orchestration/skills/quad-cli-consensus-gate.md @@ -209,9 +209,50 @@ The top-level report additionally carries, when relevant: - `status: "advisory-degraded" | "no-reviewers"` — present only when reviewer count is below `--min-reviewers` or zero - `reviewers_effective: N` — the number of CLIs that returned schema-valid output this run -- `reviewers` — the declared and effective tool names plus structured stage/cause diagnostics for every dropped reviewer + +The following two fields are **only** populated when the caller passes `--artifact ` — unlike +`status`/`reviewers_effective`, which are always present when relevant, these carry an additional +per-tool CLI re-invocation cost (see below) that is not paid on a default run: + +- `reviewers` — the declared and effective tool names, transport-integrity nonce status per declared + tool (see Transport-Integrity Nonce), plus structured stage/cause diagnostics for every dropped reviewer - `environment` — the orchestrator commit, OS/architecture, Node version, and per-tool CLI versions +> **Cost of `--artifact`:** populating `environment.cli_versions` re-invokes every declared CLI with +> `--version` (up to four sequential, 10-second-capped `spawnSync` calls) even for tools that already +> ran successfully in the main review. This happens **after** the gate-timeout-bounded review phase, so +> it can add up to ~40s of wall-clock time that is **not** covered by `--gate-timeout`. Mock runs +> (`QUAD_CLI_MOCK_DIR`) skip the real spawn and resolve instantly. + +## Transport-Integrity Nonce + +A schema-valid JSON response only proves a reviewer returned *some* well-formed report — it does not +prove the reviewer actually received and processed the *entire* prompt this run sent it. A truncated +transport, a stale cached response, or a model silently working from a partial prompt can all still +produce schema-valid JSON. + +Each run generates a random per-run token (`transport_nonce`, 16 hex characters) and instructs every +reviewer to echo it back verbatim as a top-level `transport_nonce` string field. The instruction and +token are placed **after** the `...` block in the prompt, not before it — the realistic +failure mode is tail truncation of the (usually large) diff body, not truncation of the short +instructions header, so the token is only reachable once the reviewer has received the whole diff. Any +truncation before that point yields `not-echoed`, never a false `confirmed`. + +The orchestrator compares the echoed value against the token it issued and records one of four states +per declared tool in `reviewers.nonce_status` (only present under `--artifact`, alongside the rest of +`reviewers`): + +| Status | Meaning | +|--------|---------| +| `confirmed` | The reviewer echoed the exact token — positive proof it received this run's full, unmodified prompt. | +| `mismatch` | The reviewer echoed a *different* value. This is unambiguous: the transport or the reviewer altered the prompt, so the report is dropped from consensus even though its JSON was otherwise schema-valid (`stage: "response"`, `primary_cause: "transport-integrity"`). | +| `not-echoed` | No `transport_nonce` field was present. This is **deliberately not a failure** — an older CLI or a model that never echoes unrecognized fields is indistinguishable, from the orchestrator's side, from one that silently ignored the instruction. Recorded for observability only; does not affect `reviewers_effective` or `blocking`. | +| `unavailable` | The reviewer never produced a parseable response at all (resolve/spawn/transport failure) — there was nothing to check the nonce against. | + +`QUAD_CLI_TRANSPORT_NONCE` overrides the generated token for deterministic test fixtures; it is +test-only and must never be set for a production run (a fixed token defeats the transport-integrity +guarantee the mechanism exists to provide). + Normalized matching uses repo-relative paths with forward slashes. Paths are **not blindly lowercased** by default — `rule_id` normalization **is** lowercased (via `schemas/rule-aliases.json` alias resolution), since rule identifiers are conventionally case-insensitive across tools while file paths are not. ## Exit Codes diff --git a/schemas/quad-cli-merged-report.json b/schemas/quad-cli-merged-report.json index b147170..a392176 100644 --- a/schemas/quad-cli-merged-report.json +++ b/schemas/quad-cli-merged-report.json @@ -58,6 +58,12 @@ "ms": { "type": "integer" } } } + }, + "nonce_status": { + "type": "object", + "additionalProperties": { + "enum": ["confirmed", "not-echoed", "mismatch", "unavailable"] + } } } }, diff --git a/scripts/quad-cli-orchestrate.mjs b/scripts/quad-cli-orchestrate.mjs index 65e7b9e..a58b2d5 100644 --- a/scripts/quad-cli-orchestrate.mjs +++ b/scripts/quad-cli-orchestrate.mjs @@ -1,3 +1,4 @@ +import { randomBytes } from "node:crypto"; import { existsSync, readFileSync, writeFileSync } from "node:fs"; import { homedir } from "node:os"; import { basename, dirname, isAbsolute, join, resolve } from "node:path"; @@ -51,6 +52,16 @@ const SEVERITY_RANK = { blocker: 4, }; const DEFAULT_MAX_PROMPT_BYTES = 256 * 1024; +const NONCE_STATUSES = ["confirmed", "not-echoed", "mismatch", "unavailable"]; + +// Test-only override so fixture mock responses (static JSON files) can deterministically +// match this run's issued nonce; never set in production use. Mirrors the existing +// QUAD_CLI_MOCK_DIR / QUAD_CLI_TIMEOUT_MS test-only environment variable convention. +function generateTransportNonce() { + const override = process.env.QUAD_CLI_TRANSPORT_NONCE; + if (override) return override; + return randomBytes(8).toString("hex"); +} export async function main() { try { @@ -68,7 +79,7 @@ export async function main() { if (!diffResult.body.trim()) { const emptyReport = createReport([], { - reviewers: options.artifact ? { declared: [], effective: [], dropped: [] } : undefined, + reviewers: options.artifact ? { declared: [], effective: [], dropped: [], nonce_status: {} } : undefined, environment: options.artifact ? buildEnvironment(options.mockDir) : undefined, }); if (!validateMergedReport(emptyReport).ok) { @@ -82,7 +93,8 @@ export async function main() { process.exit(0); } - const prompt = buildPrompt(diffResult.body); + const transportNonce = generateTransportNonce(); + const prompt = buildPrompt(diffResult.body, transportNonce); const promptBytes = Buffer.byteLength(prompt, "utf8"); if (promptBytes > options.maxPromptBytes) { @@ -159,6 +171,20 @@ export async function main() { result.findingsCount = parsed.value.findings.length; result.ignoredTopLevelKeys = validation.ignoredTopLevelKeys; + const nonceStatus = deriveNonceStatus(parsed.value.transport_nonce, transportNonce); + result.nonceStatus = nonceStatus; + if (nonceStatus === "mismatch") { + // A wrong (but present) echo proves this reviewer did not faithfully receive/process + // this run's exact prompt — do not trust its findings for consensus, even though the + // JSON itself is schema-valid. A missing echo ("not-echoed") stays ambiguous (older + // CLI, or a model that ignored the instruction) and is NOT treated as a failure. + result.status = "error"; + result.class = "transport-integrity"; + result.reason = "transport_nonce did not match this run's issued token"; + erroredReviewers += 1; + continue; + } + validReports.push({ tool: result.tool, findings: parsed.value.findings.map((finding) => ({ @@ -635,7 +661,7 @@ function countPatchLines(section) { .length; } -function buildPrompt(diffBody) { +function buildPrompt(diffBody, transportNonce = null) { return [ "You are one reviewer in a four-CLI consensus gate.", "Review the diff for correctness, security, reliability, and maintainability issues that deserve structured findings.", @@ -645,6 +671,7 @@ function buildPrompt(diffBody) { { schema_version: "1", generated_by: "quad-cli-orchestrate", + transport_nonce: "", findings: [ { path: "path/to/file", @@ -667,6 +694,18 @@ function buildPrompt(diffBody) { "", diffBody, "", + // The transport-integrity instruction is deliberately placed AFTER the diff, not before + // it. The realistic transport failure this guards against is tail truncation of the + // (usually large) diff body, not truncation of the short instructions header. Putting the + // token/instruction here means a reviewer can only produce "confirmed" by having received + // this entire prompt, diff included; any truncation before this point yields "not-echoed" + // (never a false "confirmed"). + ...(transportNonce + ? [ + `This run's transport-integrity token is: ${transportNonce}`, + 'Set "transport_nonce" to that exact token, unmodified, in your JSON response — this proves you received this entire prompt, including the diff above.', + ] + : []), ].join("\n"); } @@ -985,6 +1024,7 @@ function emitReport(report, artifactPath) { } function deriveFailureStage(result) { + if (result.class === "transport-integrity") return "response"; if (result.class === "schema-invalid") return "schema"; if (result.class === "invalid-response") return "response"; if (result.launcherKind === "not-found") return "resolve"; @@ -993,6 +1033,19 @@ function deriveFailureStage(result) { return "spawn"; } +// Compares a reviewer's echoed transport_nonce against the token this run issued. +// - "confirmed": the reviewer echoed back the exact token — proves it received/processed +// this run's full prompt, not a truncated or stale one. +// - "mismatch": the reviewer echoed a *different* value — proves the transport or the +// reviewer corrupted/altered the prompt; the report is not trustworthy for consensus. +// - "not-echoed": the field is absent (or not a string). This is deliberately NOT treated +// as a failure: older CLIs and models that never learned this field will always land +// here, and that is indistinguishable from a model that silently ignored the instruction. +function deriveNonceStatus(echoed, expected) { + if (typeof echoed !== "string" || echoed.length === 0) return "not-echoed"; + return echoed === expected ? "confirmed" : "mismatch"; +} + function redactResolvedPath(value, home = homedir()) { if (!value) return "-"; const rawPath = String(value); @@ -1006,9 +1059,40 @@ function redactResolvedPath(value, home = homedir()) { return isAbsolute(rawPath) ? basename(rawPath) : basename(normalizedPath); } +const TRAILING_PUNCTUATION = /[.,;:)\]]+$/; + +function looksLikePathToken(token) { + return isAbsolute(token) || /^[A-Za-z]:[\\/]/.test(token) || token.startsWith("/") || token.startsWith("\\\\"); +} + +// observed_failure is free-form text (a CLI's stderr/error first line) and can contain an +// absolute path even though it already passed through sanitizeReason (which only strips +// control characters and truncates length, not paths). Apply the same home-dir/basename +// redaction as redactResolvedPath, but per whitespace-delimited token, so surrounding +// diagnostic text (e.g. "Mock response not found: ") is preserved. +function redactPathTokensInText(text, home = homedir()) { + if (!text) return text; + return text + .split(/(\s+)/) + .map((token) => { + if (!token || /^\s+$/.test(token)) return token; + const trailingMatch = token.match(TRAILING_PUNCTUATION); + const trailing = trailingMatch ? trailingMatch[0] : ""; + const core = trailing ? token.slice(0, -trailing.length) : token; + if (!looksLikePathToken(core)) return token; + return `${redactResolvedPath(core, home)}${trailing}`; + }) + .join(""); +} + function buildReviewerRoster(parsedResults, validReports) { const effective = validReports.map(({ tool }) => tool); const effectiveSet = new Set(effective); + // "unavailable" covers every result that never reached JSON parsing (resolve/spawn/ + // transport failures) — there was no response body to check for an echoed nonce at all. + const nonceStatus = Object.fromEntries( + parsedResults.map((result) => [result.tool, result.nonceStatus ?? "unavailable"]) + ); return { declared: parsedResults.map(({ tool }) => tool), effective, @@ -1016,12 +1100,13 @@ function buildReviewerRoster(parsedResults, validReports) { tool: result.tool, stage: deriveFailureStage(result), primary_cause: result.class ?? "internal", - observed_failure: sanitizeReason(result.reason || result.stderr || result.error || "-") || "-", + observed_failure: redactPathTokensInText(sanitizeReason(result.reason || result.stderr || result.error || "-")) || "-", launcher_kind: result.launcherKind ?? "-", resolved_path: redactResolvedPath(result.resolvedPath), exit_code: Number.isInteger(result.exitCode) ? result.exitCode : null, ms: Number.isInteger(result.ms) ? result.ms : 0, })), + nonce_status: nonceStatus, }; } @@ -1059,10 +1144,20 @@ function buildEnvironment(mockDir) { function validateReviewerRoster(reviewers) { if (!reviewers || typeof reviewers !== "object" || Array.isArray(reviewers)) return false; - if (!Object.keys(reviewers).every((key) => ["declared", "effective", "dropped"].includes(key))) return false; + if (!Object.keys(reviewers).every((key) => ["declared", "effective", "dropped", "nonce_status"].includes(key))) + return false; if (!Array.isArray(reviewers.declared) || !reviewers.declared.every((tool) => typeof tool === "string")) return false; if (!Array.isArray(reviewers.effective) || !reviewers.effective.every((tool) => typeof tool === "string")) return false; if (!Array.isArray(reviewers.dropped)) return false; + if ( + Object.hasOwn(reviewers, "nonce_status") && + (!reviewers.nonce_status || + typeof reviewers.nonce_status !== "object" || + Array.isArray(reviewers.nonce_status) || + !Object.values(reviewers.nonce_status).every((value) => NONCE_STATUSES.includes(value))) + ) { + return false; + } const allowedStages = ["resolve", "spawn", "transport", "response", "schema"]; const droppedKeys = ["tool", "stage", "primary_cause", "observed_failure", "launcher_kind", "resolved_path", "exit_code", "ms"]; return reviewers.dropped.every((entry) => @@ -1103,7 +1198,10 @@ export { normalizeRuleId, createReport, deriveFailureStage, + deriveNonceStatus, + generateTransportNonce, redactResolvedPath, + redactPathTokensInText, buildReviewerRoster, }; diff --git a/tests/fixtures/quad-cli/mock-responses-nonce/claude.json b/tests/fixtures/quad-cli/mock-responses-nonce/claude.json new file mode 100644 index 0000000..b0347c9 --- /dev/null +++ b/tests/fixtures/quad-cli/mock-responses-nonce/claude.json @@ -0,0 +1,6 @@ +{ + "schema_version": "1", + "generated_by": "quad-cli-orchestrate", + "transport_nonce": "test-fixture-nonce", + "findings": [] +} diff --git a/tests/fixtures/quad-cli/mock-responses-nonce/codex.json b/tests/fixtures/quad-cli/mock-responses-nonce/codex.json new file mode 100644 index 0000000..abd216e --- /dev/null +++ b/tests/fixtures/quad-cli/mock-responses-nonce/codex.json @@ -0,0 +1,5 @@ +{ + "schema_version": "1", + "generated_by": "quad-cli-orchestrate", + "findings": [] +} diff --git a/tests/fixtures/quad-cli/mock-responses-nonce/cursor-agent.json b/tests/fixtures/quad-cli/mock-responses-nonce/cursor-agent.json new file mode 100644 index 0000000..24fe293 --- /dev/null +++ b/tests/fixtures/quad-cli/mock-responses-nonce/cursor-agent.json @@ -0,0 +1,6 @@ +{ + "schema_version": "1", + "generated_by": "quad-cli-orchestrate", + "transport_nonce": "a-different-token-entirely", + "findings": [] +} diff --git a/tests/quad-cli-orchestrate.test.js b/tests/quad-cli-orchestrate.test.js index 52065b6..e2703e0 100644 --- a/tests/quad-cli-orchestrate.test.js +++ b/tests/quad-cli-orchestrate.test.js @@ -16,7 +16,9 @@ import { validateMergedReport, parseArgs, deriveFailureStage, + deriveNonceStatus, redactResolvedPath, + redactPathTokensInText, } from "../scripts/quad-cli-orchestrate.mjs"; const ROOT = resolve(import.meta.dirname, ".."); @@ -405,4 +407,77 @@ describe("reviewer roster and environment artifact", () => { assert.equal(validateMergedReport(report).ok, true); }); }); + + it("redacts a home-directory path embedded inside free-form observed_failure text (does not just basename resolved_path)", () => { + const home = homedir(); + const leaky = `Mock response not found: ${join(home, "private", "agy.json")}`; + const redacted = redactPathTokensInText(leaky, home); + assert.match(redacted, /^Mock response not found: ~[\\/]/); + assert.equal(redacted.toLowerCase().includes(home.replace(/\\/g, "/").toLowerCase()), false); + assert.equal(redactPathTokensInText("no path here at all", home), "no path here at all"); + }); +}); + +describe("quad-cli transport-integrity nonce (--diff-file fixture, mock-responses-nonce)", () => { + const NONCE_MOCK_DIR = resolve(FIXTURES_DIR, "mock-responses-nonce"); + const TEST_NONCE = "test-fixture-nonce"; + + function withNonceFixture(callback) { + const tempDir = mkdtempSync(join(tmpdir(), "quad-cli-nonce-")); + const artifactPath = join(tempDir, "report.json"); + try { + const result = runScript(["--diff-file", resolve(FIXTURES_DIR, "smoke.diff"), "--artifact", artifactPath], { + QUAD_CLI_MOCK_DIR: NONCE_MOCK_DIR, + QUAD_CLI_TRANSPORT_NONCE: TEST_NONCE, + }); + callback({ result, report: JSON.parse(result.stdout) }); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + } + + it("deriveNonceStatus: exact match is confirmed, different value is mismatch, missing/non-string is not-echoed", () => { + assert.equal(deriveNonceStatus("abc123", "abc123"), "confirmed"); + assert.equal(deriveNonceStatus("wrong", "abc123"), "mismatch"); + assert.equal(deriveNonceStatus(undefined, "abc123"), "not-echoed"); + assert.equal(deriveNonceStatus(42, "abc123"), "not-echoed"); + assert.equal(deriveNonceStatus("", "abc123"), "not-echoed"); + }); + + it("records confirmed/not-echoed/mismatch/unavailable per declared tool in reviewers.nonce_status", () => { + withNonceFixture(({ report }) => { + assert.deepEqual(report.reviewers.nonce_status, { + claude: "confirmed", + codex: "not-echoed", + "cursor-agent": "mismatch", + agy: "unavailable", + }); + }); + }); + + it("drops a reviewer whose echoed nonce mismatches, even though its JSON was schema-valid", () => { + withNonceFixture(({ report }) => { + assert.deepEqual(report.reviewers.effective, ["claude", "codex"]); + const dropped = report.reviewers.dropped.find(({ tool }) => tool === "cursor-agent"); + assert.ok(dropped, "cursor-agent must be recorded as dropped"); + assert.equal(dropped.stage, "response"); + assert.equal(dropped.primary_cause, "transport-integrity"); + }); + }); + + it("does NOT drop a reviewer that simply never echoed the field (not-echoed stays effective)", () => { + withNonceFixture(({ report }) => { + assert.ok(report.reviewers.effective.includes("codex")); + assert.equal(report.reviewers.dropped.some(({ tool }) => tool === "codex"), false); + }); + }); + + it("QUAD_CLI_TRANSPORT_NONCE override is test-only and produces a deterministic, non-empty nonce", () => { + withNonceFixture(({ report }) => { + // reviewers_effective reflects the two non-dropped reviewers (claude confirmed, codex not-echoed); + // this indirectly proves the same override value was used to build the prompt every declared + // reviewer saw, since claude's canned response only matches "test-fixture-nonce" by fixture design. + assert.equal(report.reviewers_effective, 2); + }); + }); });