From af037604a6f8d8ac2b9a3279b176aebed61aefb5 Mon Sep 17 00:00:00 2001 From: Stefan Ayala Date: Sat, 22 Aug 2026 15:35:26 -0700 Subject: [PATCH] fix(review): reject forbidden Claude tool use Closes #158 Review-Exception: one-time issue-158 meta-review bootstrap Owner-Authorized: https://github.com/BaseInfinity/codex-sdlc-wizard/issues/158#issuecomment-5382636546 Reviewed-By: Sol High Fable-Review: invalid-forbidden-advisor Opus-Review: invalid-forbidden-advisor --- .codex/hooks/dual-review.cjs | 56 ++++++++++++++--- tests/test-adapter.sh | 50 +++++++++++++++- tests/test-review-supervision.cjs | 99 +++++++++++++++++++++++++++++++ 3 files changed, 195 insertions(+), 10 deletions(-) diff --git a/.codex/hooks/dual-review.cjs b/.codex/hooks/dual-review.cjs index 9284368..665b2f6 100644 --- a/.codex/hooks/dual-review.cjs +++ b/.codex/hooks/dual-review.cjs @@ -243,6 +243,8 @@ function runSupervisedProcess(command, args, options = {}) { let cancelled = false; let timeoutReason = null; let terminationRequested = false; + let rejectedEvent = null; + let requestTermination = () => {}; let lineBuffer = ""; child.stdout.setEncoding("utf8"); child.stderr.setEncoding("utf8"); @@ -250,10 +252,11 @@ function runSupervisedProcess(command, args, options = {}) { lastActivityAt = Date.now(); if (logPath !== "") fs.appendFileSync(logPath, `[${stream}] ${chunk}`); }; - const recordEvents = (chunk) => { + const recordEvents = (chunk, flush = false) => { lineBuffer += chunk; const lines = lineBuffer.split(/\r?\n/); - lineBuffer = lines.pop() || ""; + if (flush) lineBuffer = ""; + else lineBuffer = lines.pop() || ""; for (const line of lines) { if (line.trim() === "") continue; try { @@ -261,6 +264,14 @@ function runSupervisedProcess(command, args, options = {}) { lastEventAt = Date.now(); lastEventType = String(event.type || event.event || event.subtype || "json"); options.onEvent?.(event); + const rejection = options.rejectEvent?.(event); + if (rejectedEvent === null && rejection) { + rejectedEvent = { + type: lastEventType, + reason: String(rejection), + }; + requestTermination(); + } } catch { // Human-readable diagnostics remain in the local log and terminal result. } @@ -271,6 +282,7 @@ function runSupervisedProcess(command, args, options = {}) { recordChunk("stdout", chunk); recordEvents(chunk); }); + child.stdout.on("end", () => recordEvents("", true)); child.stderr.on("data", (chunk) => { stderr += chunk; recordChunk("stderr", chunk); @@ -294,9 +306,12 @@ function runSupervisedProcess(command, args, options = {}) { timedOut, cancelled, timeout_reason: timeoutReason, - terminal_state: timedOut - ? REVIEW_STATES.TIMED_OUT - : cancelled + rejected_event: rejectedEvent, + terminal_state: rejectedEvent !== null + ? REVIEW_STATES.INVALID_VERDICT + : timedOut + ? REVIEW_STATES.TIMED_OUT + : cancelled ? REVIEW_STATES.CANCELLED : result.error ? REVIEW_STATES.INTERNAL_RUNNER_FAILURE @@ -311,7 +326,7 @@ function runSupervisedProcess(command, args, options = {}) { log_path: logPath || null, }); }; - const requestTermination = () => { + requestTermination = () => { if (terminationRequested) return; terminationRequested = true; terminateProcessTree(child, "SIGTERM"); @@ -392,6 +407,7 @@ function runtimeAttemptMetadata(attempt) { timeout_reason: attempt.timeout_reason || null, last_event_at: attempt.last_event_at || null, last_event_type: attempt.last_event_type || "none", + rejected_event: attempt.rejected_event || null, log_path: attempt.log_path || null, retry_reason: attempt.retry_reason || null, }; @@ -1127,11 +1143,23 @@ function actualClaudeModel(parsed, envelope) { return candidates.find((candidate) => typeof candidate === "string" && candidate !== "") || ""; } +function rejectClaudeReviewerToolUse(event) { + const events = Array.isArray(event) ? event : [event]; + const blocks = events.flatMap((entry) => [ + entry, + entry?.content_block, + ...(Array.isArray(entry?.message?.content) ? entry.message.content : []), + ]).filter((block) => block && typeof block === "object"); + const toolBlock = blocks.find((block) => /(?:^|_)tool_use$/.test(String(block.type || ""))); + if (!toolBlock) return ""; + return `forbidden reviewer tool use: ${toolBlock.name || "unknown"}`; +} + async function runClaudeReviewer(prompt, temporaryDirectory, signal, reviewer, route, fallbackReason = null, logDirectory, phase = "initial") { const launch = claudeLaunch(); const args = [ "-p", "--model", reviewer.model, "--effort", reviewer.effort, "--safe-mode", "--max-turns", "2", - "--setting-sources", "user", "--tools", "", "--disable-slash-commands", + "--setting-sources", "user", "--tools", "", "--disallowedTools", "advisor", "--disable-slash-commands", "--no-session-persistence", "--mcp-config", '{"mcpServers":{}}', "--strict-mcp-config", "--json-schema", JSON.stringify(REVIEW_SCHEMA), "--output-format", "stream-json", "--verbose", ]; @@ -1146,11 +1174,19 @@ async function runClaudeReviewer(prompt, temporaryDirectory, signal, reviewer, r heartbeatInterval: configuredDuration("CODEX_SDLC_REVIEW_HEARTBEAT_MS", 30 * 1000), killGrace: configuredDuration("CODEX_SDLC_REVIEW_KILL_GRACE_MS", 2000), structuredEventsRequired: true, + rejectEvent: rejectClaudeReviewerToolUse, signal, label: `${reviewer.label} ${phase}`, logPath: path.join(logDirectory, `cross-model-${phase}-attempt-${attempt}.log`), windowsVerbatimArguments: prepared.windowsVerbatimArguments, }); + if (result.rejected_event) { + return { + ...result, + retryable: false, + terminal_state: REVIEW_STATES.INVALID_VERDICT, + }; + } const availabilityReason = claudeAvailabilityReason(result); if (availabilityReason !== "") { return { @@ -1208,7 +1244,7 @@ async function confirmClaudeReviewerUnavailable(temporaryDirectory, signal, revi const launch = claudeLaunch(); const args = [ "-p", "--model", reviewer.model, "--effort", reviewer.effort, "--safe-mode", "--max-turns", "1", - "--setting-sources", "user", "--tools", "", "--disable-slash-commands", + "--setting-sources", "user", "--tools", "", "--disallowedTools", "advisor", "--disable-slash-commands", "--no-session-persistence", "--mcp-config", '{"mcpServers":{}}', "--strict-mcp-config", "--output-format", "stream-json", "--verbose", ]; @@ -1222,12 +1258,13 @@ async function confirmClaudeReviewerUnavailable(temporaryDirectory, signal, revi heartbeatInterval: configuredDuration("CODEX_SDLC_REVIEW_HEARTBEAT_MS", 30 * 1000), killGrace: configuredDuration("CODEX_SDLC_REVIEW_KILL_GRACE_MS", 2000), structuredEventsRequired: true, + rejectEvent: rejectClaudeReviewerToolUse, signal, label: `${reviewer.label} availability probe`, logPath: path.join(logDirectory, "cross-model-availability-probe.log"), windowsVerbatimArguments: prepared.windowsVerbatimArguments, }); - if (result.error || result.timedOut || result.cancelled) return false; + if (result.rejected_event || result.error || result.timedOut || result.cancelled) return false; return claudeAvailabilityReason(result) === suspectedReason; } @@ -1471,6 +1508,7 @@ module.exports = { REVIEW_STATES, buildWindowsCommandLine, classifyReviewFailure, + rejectClaudeReviewerToolUse, runSupervisedProcess, runWithInfrastructureRetry, }; diff --git a/tests/test-adapter.sh b/tests/test-adapter.sh index 57902ec..dca32ed 100755 --- a/tests/test-adapter.sh +++ b/tests/test-adapter.sh @@ -5394,6 +5394,13 @@ if (args[0] === "auth" && args[1] === "status") { const promptBuffer = fs.readFileSync(0); const prompt = promptBuffer.toString("utf8"); const mode = process.env.DUAL_TEST_MODE || "clean"; +if (mode === "forbidden-tool-then-clean") { + const forbidden = { type: "assistant", message: { content: [{ type: "server_tool_use", name: "advisor" }] } }; + const clean = { type: "result", model: "fable", structured_output: { findings: [], verdict: "CERTIFIED", confidence: 99 } }; + fs.appendFileSync(process.env.FABLE_MARKER, `${JSON.stringify({ args, prompt, promptBase64: promptBuffer.toString("base64") })}\n`); + process.stdout.write(JSON.stringify([forbidden, clean])); + return; +} if (mode === "sol-fail-fable-hang") { process.on("SIGTERM", () => { fs.writeFileSync(process.env.CANCEL_MARKER, "cancelled\n"); @@ -5444,6 +5451,8 @@ if (!solCalls[0].args.includes("gpt-5.6-sol") || !solCalls[0].args.some((arg) => if (!solCalls[0].args.includes("--json")) process.exit(1); if (!fableCalls[0].args.includes("fable") || !fableCalls[0].args.includes("high")) process.exit(1); if (!fableCalls[0].args.includes("stream-json") || !fableCalls[0].args.includes("--verbose")) process.exit(1); +const denyIndex = fableCalls[0].args.indexOf("--disallowedTools"); +if (denyIndex === -1 || fableCalls[0].args[denyIndex + 1] !== "advisor") process.exit(1); const expectedPatch = require("node:child_process").spawnSync( "git", ["-C", process.env.REPO_PATH, "diff", "--cached", "--binary", "HEAD"], ).stdout; @@ -5453,6 +5462,19 @@ const fablePrompt = Buffer.from(fableCalls[0].promptBase64, "base64"); if (!fablePrompt.includes(expectedPatch)) process.exit(1); NODE + : > "$fable_marker" + set +e + output=$(cd "$ws" && CODEX_SDLC_TEST_MODE=1 CODEX_SDLC_CODEX_PATH="$fake_codex" \ + CODEX_SDLC_CLAUDE_PATH="$fake_claude" SOL_MARKER="$sol_marker" \ + FABLE_MARKER="$fable_marker" DUAL_TEST_MODE=forbidden-tool-then-clean \ + node .codex/hooks/dual-review.cjs --base HEAD --consent-subscription-quota 2>&1) + status=$? + set -e + [ "$status" -eq 2 ] || valid=false + [ ! -f "$receipt" ] || valid=false + [ "$(wc -l < "$fable_marker" | tr -d ' ')" -eq 1 ] || valid=false + [[ "$output" == *"review INVALID_VERDICT"* ]] || valid=false + : > "$sol_marker" : > "$fable_marker" set +e @@ -5626,6 +5648,10 @@ if (model === "fable" && ["fallback", "both-unavailable"].includes(mode)) { process.stdout.write("You're out of usage credits. Run /usage-credits to keep using Fable 5.\n"); process.exit(1); } +if (model === "fable" && mode === "probe-tool-use" && !probe) { + process.stdout.write("You're out of usage credits. Run /usage-credits to keep using Fable 5.\n"); + process.exit(1); +} if (model === "fable" && mode === "zero-exit-quota") { process.stdout.write(`${JSON.stringify({ type: "rate_limit_event", rate_limit_info: { status: "rejected" } })}\n`); process.stdout.write(`${JSON.stringify({ @@ -5652,6 +5678,13 @@ if (model === "fable" && mode === "zero-exit-malformed" && !probe) { })); process.exit(0); } +if (probe && mode === "probe-tool-use") { + process.stdout.write(JSON.stringify([ + { type: "assistant", message: { content: [{ type: "server_tool_use", name: "advisor" }] } }, + { type: "rate_limit_event", rate_limit_info: { status: "rejected" } }, + ])); + process.exit(0); +} if (probe) { process.stdout.write("available\n"); process.exit(0); @@ -5759,9 +5792,24 @@ if (calls.length !== 3 || !calls[1].probe) process.exit(1); if (calls[0].model !== "fable" || calls[1].model !== "fable" || calls[2].model !== "claude-opus-4-8") process.exit(1); if (receipt.reviewers.cross_model.route !== "fallback") process.exit(1); if (receipt.reviewers.cross_model.fallback_reason !== "quota_exhausted") process.exit(1); -if (receipt.reviewers.cross_model.model !== "claude-opus-4-8-20260801") process.exit(1); + if (receipt.reviewers.cross_model.model !== "claude-opus-4-8-20260801") process.exit(1); NODE + rm -f "$receipt" + : > "$calls" + set +e + output=$(cd "$ws" && CODEX_SDLC_TEST_MODE=1 CODEX_SDLC_CODEX_PATH="$fake_codex" \ + CODEX_SDLC_CLAUDE_PATH="$fake_claude" CLAUDE_CALLS="$calls" \ + FALLBACK_TEST_MODE=probe-tool-use node .codex/hooks/dual-review.cjs --base HEAD \ + --consent-subscription-quota 2>&1) + status=$? + set -e + [ "$status" -eq 2 ] || valid=false + [ "$(wc -l < "$calls" | tr -d ' ')" -eq 2 ] || valid=false + [ "$(grep -c '"model":"fable"' "$calls")" -eq 2 ] || valid=false + grep -q 'claude-opus-4-8' "$calls" && valid=false + [ ! -f "$receipt" ] || valid=false + rm -f "$receipt" : > "$calls" set +e diff --git a/tests/test-review-supervision.cjs b/tests/test-review-supervision.cjs index 9c7a9bf..4627763 100644 --- a/tests/test-review-supervision.cjs +++ b/tests/test-review-supervision.cjs @@ -10,6 +10,7 @@ const path = require("node:path"); const { REVIEW_STATES, classifyReviewFailure, + rejectClaudeReviewerToolUse, runSupervisedProcess, runWithInfrastructureRetry, } = require("../.codex/hooks/dual-review.cjs"); @@ -98,6 +99,99 @@ async function testUnstructuredChatterDoesNotFakeProgress() { assert.equal(result.terminal_state, REVIEW_STATES.TIMED_OUT); } +function realClaudeAdvisorEvent() { + return { + type: "assistant", + message: { + content: [ + { type: "thinking", thinking: "" }, + { type: "server_tool_use", name: "advisor", input: { task: "review" } }, + ], + }, + }; +} + +function testClaudeToolUseDetectorScansRealAssistantEnvelope() { + assert.equal( + rejectClaudeReviewerToolUse(realClaudeAdvisorEvent()), + "forbidden reviewer tool use: advisor", + ); +} + +function testClaudeToolUseDetectorScansArrayEnvelope() { + assert.equal( + rejectClaudeReviewerToolUse([ + realClaudeAdvisorEvent(), + { type: "result", structured_output: { findings: [], verdict: "CERTIFIED", confidence: 99 } }, + ]), + "forbidden reviewer tool use: advisor", + ); +} + +async function testRejectedToolUseCannotBecomeCleanResult() { + const source = [ + "const forbidden={type:'assistant',message:{content:[{type:'server_tool_use',name:'advisor'}]}};", + "const clean={type:'result',model:'fable',structured_output:{findings:[],verdict:'CERTIFIED',confidence:99}};", + "process.on('SIGTERM',()=>process.exit(0));", + "process.stdout.write(JSON.stringify(forbidden)+'\\n'+JSON.stringify(clean)+'\\n');", + "setInterval(()=>{},1000);", + ].join(""); + const result = await runSupervisedProcess(process.execPath, ["-e", source], { + heartbeatInterval: 20, + stallTimeout: 500, + timeout: 800, + killGrace: 30, + structuredEventsRequired: true, + rejectEvent: rejectClaudeReviewerToolUse, + }); + + assert.equal(result.timedOut, false); + assert.equal(result.terminal_state, REVIEW_STATES.INVALID_VERDICT); + assert.equal(result.rejected_event.reason, "forbidden reviewer tool use: advisor"); + assert.match(result.stdout, /\"verdict\":\"CERTIFIED\"/); +} + +async function testRejectedFinalBufferCannotBecomeCleanResult() { + const source = [ + "const forbidden={type:'assistant',message:{content:[{type:'server_tool_use',name:'advisor'}]}};", + "const clean={type:'result',model:'fable',structured_output:{findings:[],verdict:'CERTIFIED',confidence:99}};", + "process.stdout.write(JSON.stringify([forbidden,clean]));", + ].join(""); + const result = await runSupervisedProcess(process.execPath, ["-e", source], { + heartbeatInterval: 20, + stallTimeout: 500, + timeout: 800, + killGrace: 30, + structuredEventsRequired: true, + rejectEvent: rejectClaudeReviewerToolUse, + }); + + assert.equal(result.timedOut, false); + assert.equal(result.terminal_state, REVIEW_STATES.INVALID_VERDICT); + assert.equal(result.rejected_event.reason, "forbidden reviewer tool use: advisor"); +} + +async function testRejectedToolUseOutranksTimeout() { + const source = [ + "process.on('SIGTERM',()=>{});", + "process.stdout.write(JSON.stringify({type:'assistant',message:{content:[{type:'server_tool_use',name:'advisor'}]}})+'\\n');", + "setInterval(()=>{},1000);", + ].join(""); + const result = await runSupervisedProcess(process.execPath, ["-e", source], { + heartbeatInterval: 25, + stallTimeout: 2000, + timeout: 500, + killGrace: 650, + structuredEventsRequired: true, + rejectEvent: rejectClaudeReviewerToolUse, + }); + + assert.equal(result.timedOut, true); + assert.equal(result.timeout_reason, "wall"); + assert.equal(result.terminal_state, REVIEW_STATES.INVALID_VERDICT); + assert.equal(result.rejected_event.reason, "forbidden reviewer tool use: advisor"); +} + function testAuthAndInvalidVerdictClassification() { assert.equal(classifyReviewFailure({ status: 1, @@ -139,6 +233,11 @@ async function testOneInfrastructureRetryOnly() { await testHardTimeoutReapsDescendantTree(); await testCancellationStopsReviewerAndRetainsPrivateLog(); await testUnstructuredChatterDoesNotFakeProgress(); + testClaudeToolUseDetectorScansRealAssistantEnvelope(); + testClaudeToolUseDetectorScansArrayEnvelope(); + await testRejectedToolUseCannotBecomeCleanResult(); + await testRejectedFinalBufferCannotBecomeCleanResult(); + await testRejectedToolUseOutranksTimeout(); testAuthAndInvalidVerdictClassification(); await testOneInfrastructureRetryOnly(); process.stdout.write("review supervision tests passed\n");