Skip to content

Commit af03760

Browse files
BaseInfinityBaseInfinity
committed
fix(review): reject forbidden Claude tool use
Closes #158 Review-Exception: one-time issue-158 meta-review bootstrap Owner-Authorized: #158 (comment) Reviewed-By: Sol High Fable-Review: invalid-forbidden-advisor Opus-Review: invalid-forbidden-advisor
1 parent 5003842 commit af03760

3 files changed

Lines changed: 195 additions & 10 deletions

File tree

‎.codex/hooks/dual-review.cjs‎

Lines changed: 47 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -243,24 +243,35 @@ function runSupervisedProcess(command, args, options = {}) {
243243
let cancelled = false;
244244
let timeoutReason = null;
245245
let terminationRequested = false;
246+
let rejectedEvent = null;
247+
let requestTermination = () => {};
246248
let lineBuffer = "";
247249
child.stdout.setEncoding("utf8");
248250
child.stderr.setEncoding("utf8");
249251
const recordChunk = (stream, chunk) => {
250252
lastActivityAt = Date.now();
251253
if (logPath !== "") fs.appendFileSync(logPath, `[${stream}] ${chunk}`);
252254
};
253-
const recordEvents = (chunk) => {
255+
const recordEvents = (chunk, flush = false) => {
254256
lineBuffer += chunk;
255257
const lines = lineBuffer.split(/\r?\n/);
256-
lineBuffer = lines.pop() || "";
258+
if (flush) lineBuffer = "";
259+
else lineBuffer = lines.pop() || "";
257260
for (const line of lines) {
258261
if (line.trim() === "") continue;
259262
try {
260263
const event = JSON.parse(line);
261264
lastEventAt = Date.now();
262265
lastEventType = String(event.type || event.event || event.subtype || "json");
263266
options.onEvent?.(event);
267+
const rejection = options.rejectEvent?.(event);
268+
if (rejectedEvent === null && rejection) {
269+
rejectedEvent = {
270+
type: lastEventType,
271+
reason: String(rejection),
272+
};
273+
requestTermination();
274+
}
264275
} catch {
265276
// Human-readable diagnostics remain in the local log and terminal result.
266277
}
@@ -271,6 +282,7 @@ function runSupervisedProcess(command, args, options = {}) {
271282
recordChunk("stdout", chunk);
272283
recordEvents(chunk);
273284
});
285+
child.stdout.on("end", () => recordEvents("", true));
274286
child.stderr.on("data", (chunk) => {
275287
stderr += chunk;
276288
recordChunk("stderr", chunk);
@@ -294,9 +306,12 @@ function runSupervisedProcess(command, args, options = {}) {
294306
timedOut,
295307
cancelled,
296308
timeout_reason: timeoutReason,
297-
terminal_state: timedOut
298-
? REVIEW_STATES.TIMED_OUT
299-
: cancelled
309+
rejected_event: rejectedEvent,
310+
terminal_state: rejectedEvent !== null
311+
? REVIEW_STATES.INVALID_VERDICT
312+
: timedOut
313+
? REVIEW_STATES.TIMED_OUT
314+
: cancelled
300315
? REVIEW_STATES.CANCELLED
301316
: result.error
302317
? REVIEW_STATES.INTERNAL_RUNNER_FAILURE
@@ -311,7 +326,7 @@ function runSupervisedProcess(command, args, options = {}) {
311326
log_path: logPath || null,
312327
});
313328
};
314-
const requestTermination = () => {
329+
requestTermination = () => {
315330
if (terminationRequested) return;
316331
terminationRequested = true;
317332
terminateProcessTree(child, "SIGTERM");
@@ -392,6 +407,7 @@ function runtimeAttemptMetadata(attempt) {
392407
timeout_reason: attempt.timeout_reason || null,
393408
last_event_at: attempt.last_event_at || null,
394409
last_event_type: attempt.last_event_type || "none",
410+
rejected_event: attempt.rejected_event || null,
395411
log_path: attempt.log_path || null,
396412
retry_reason: attempt.retry_reason || null,
397413
};
@@ -1127,11 +1143,23 @@ function actualClaudeModel(parsed, envelope) {
11271143
return candidates.find((candidate) => typeof candidate === "string" && candidate !== "") || "";
11281144
}
11291145

1146+
function rejectClaudeReviewerToolUse(event) {
1147+
const events = Array.isArray(event) ? event : [event];
1148+
const blocks = events.flatMap((entry) => [
1149+
entry,
1150+
entry?.content_block,
1151+
...(Array.isArray(entry?.message?.content) ? entry.message.content : []),
1152+
]).filter((block) => block && typeof block === "object");
1153+
const toolBlock = blocks.find((block) => /(?:^|_)tool_use$/.test(String(block.type || "")));
1154+
if (!toolBlock) return "";
1155+
return `forbidden reviewer tool use: ${toolBlock.name || "unknown"}`;
1156+
}
1157+
11301158
async function runClaudeReviewer(prompt, temporaryDirectory, signal, reviewer, route, fallbackReason = null, logDirectory, phase = "initial") {
11311159
const launch = claudeLaunch();
11321160
const args = [
11331161
"-p", "--model", reviewer.model, "--effort", reviewer.effort, "--safe-mode", "--max-turns", "2",
1134-
"--setting-sources", "user", "--tools", "", "--disable-slash-commands",
1162+
"--setting-sources", "user", "--tools", "", "--disallowedTools", "advisor", "--disable-slash-commands",
11351163
"--no-session-persistence", "--mcp-config", '{"mcpServers":{}}', "--strict-mcp-config",
11361164
"--json-schema", JSON.stringify(REVIEW_SCHEMA), "--output-format", "stream-json", "--verbose",
11371165
];
@@ -1146,11 +1174,19 @@ async function runClaudeReviewer(prompt, temporaryDirectory, signal, reviewer, r
11461174
heartbeatInterval: configuredDuration("CODEX_SDLC_REVIEW_HEARTBEAT_MS", 30 * 1000),
11471175
killGrace: configuredDuration("CODEX_SDLC_REVIEW_KILL_GRACE_MS", 2000),
11481176
structuredEventsRequired: true,
1177+
rejectEvent: rejectClaudeReviewerToolUse,
11491178
signal,
11501179
label: `${reviewer.label} ${phase}`,
11511180
logPath: path.join(logDirectory, `cross-model-${phase}-attempt-${attempt}.log`),
11521181
windowsVerbatimArguments: prepared.windowsVerbatimArguments,
11531182
});
1183+
if (result.rejected_event) {
1184+
return {
1185+
...result,
1186+
retryable: false,
1187+
terminal_state: REVIEW_STATES.INVALID_VERDICT,
1188+
};
1189+
}
11541190
const availabilityReason = claudeAvailabilityReason(result);
11551191
if (availabilityReason !== "") {
11561192
return {
@@ -1208,7 +1244,7 @@ async function confirmClaudeReviewerUnavailable(temporaryDirectory, signal, revi
12081244
const launch = claudeLaunch();
12091245
const args = [
12101246
"-p", "--model", reviewer.model, "--effort", reviewer.effort, "--safe-mode", "--max-turns", "1",
1211-
"--setting-sources", "user", "--tools", "", "--disable-slash-commands",
1247+
"--setting-sources", "user", "--tools", "", "--disallowedTools", "advisor", "--disable-slash-commands",
12121248
"--no-session-persistence", "--mcp-config", '{"mcpServers":{}}', "--strict-mcp-config",
12131249
"--output-format", "stream-json", "--verbose",
12141250
];
@@ -1222,12 +1258,13 @@ async function confirmClaudeReviewerUnavailable(temporaryDirectory, signal, revi
12221258
heartbeatInterval: configuredDuration("CODEX_SDLC_REVIEW_HEARTBEAT_MS", 30 * 1000),
12231259
killGrace: configuredDuration("CODEX_SDLC_REVIEW_KILL_GRACE_MS", 2000),
12241260
structuredEventsRequired: true,
1261+
rejectEvent: rejectClaudeReviewerToolUse,
12251262
signal,
12261263
label: `${reviewer.label} availability probe`,
12271264
logPath: path.join(logDirectory, "cross-model-availability-probe.log"),
12281265
windowsVerbatimArguments: prepared.windowsVerbatimArguments,
12291266
});
1230-
if (result.error || result.timedOut || result.cancelled) return false;
1267+
if (result.rejected_event || result.error || result.timedOut || result.cancelled) return false;
12311268
return claudeAvailabilityReason(result) === suspectedReason;
12321269
}
12331270

@@ -1471,6 +1508,7 @@ module.exports = {
14711508
REVIEW_STATES,
14721509
buildWindowsCommandLine,
14731510
classifyReviewFailure,
1511+
rejectClaudeReviewerToolUse,
14741512
runSupervisedProcess,
14751513
runWithInfrastructureRetry,
14761514
};

‎tests/test-adapter.sh‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5394,6 +5394,13 @@ if (args[0] === "auth" && args[1] === "status") {
53945394
const promptBuffer = fs.readFileSync(0);
53955395
const prompt = promptBuffer.toString("utf8");
53965396
const mode = process.env.DUAL_TEST_MODE || "clean";
5397+
if (mode === "forbidden-tool-then-clean") {
5398+
const forbidden = { type: "assistant", message: { content: [{ type: "server_tool_use", name: "advisor" }] } };
5399+
const clean = { type: "result", model: "fable", structured_output: { findings: [], verdict: "CERTIFIED", confidence: 99 } };
5400+
fs.appendFileSync(process.env.FABLE_MARKER, `${JSON.stringify({ args, prompt, promptBase64: promptBuffer.toString("base64") })}\n`);
5401+
process.stdout.write(JSON.stringify([forbidden, clean]));
5402+
return;
5403+
}
53975404
if (mode === "sol-fail-fable-hang") {
53985405
process.on("SIGTERM", () => {
53995406
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) =>
54445451
if (!solCalls[0].args.includes("--json")) process.exit(1);
54455452
if (!fableCalls[0].args.includes("fable") || !fableCalls[0].args.includes("high")) process.exit(1);
54465453
if (!fableCalls[0].args.includes("stream-json") || !fableCalls[0].args.includes("--verbose")) process.exit(1);
5454+
const denyIndex = fableCalls[0].args.indexOf("--disallowedTools");
5455+
if (denyIndex === -1 || fableCalls[0].args[denyIndex + 1] !== "advisor") process.exit(1);
54475456
const expectedPatch = require("node:child_process").spawnSync(
54485457
"git", ["-C", process.env.REPO_PATH, "diff", "--cached", "--binary", "HEAD"],
54495458
).stdout;
@@ -5453,6 +5462,19 @@ const fablePrompt = Buffer.from(fableCalls[0].promptBase64, "base64");
54535462
if (!fablePrompt.includes(expectedPatch)) process.exit(1);
54545463
NODE
54555464

5465+
: > "$fable_marker"
5466+
set +e
5467+
output=$(cd "$ws" && CODEX_SDLC_TEST_MODE=1 CODEX_SDLC_CODEX_PATH="$fake_codex" \
5468+
CODEX_SDLC_CLAUDE_PATH="$fake_claude" SOL_MARKER="$sol_marker" \
5469+
FABLE_MARKER="$fable_marker" DUAL_TEST_MODE=forbidden-tool-then-clean \
5470+
node .codex/hooks/dual-review.cjs --base HEAD --consent-subscription-quota 2>&1)
5471+
status=$?
5472+
set -e
5473+
[ "$status" -eq 2 ] || valid=false
5474+
[ ! -f "$receipt" ] || valid=false
5475+
[ "$(wc -l < "$fable_marker" | tr -d ' ')" -eq 1 ] || valid=false
5476+
[[ "$output" == *"review INVALID_VERDICT"* ]] || valid=false
5477+
54565478
: > "$sol_marker"
54575479
: > "$fable_marker"
54585480
set +e
@@ -5626,6 +5648,10 @@ if (model === "fable" && ["fallback", "both-unavailable"].includes(mode)) {
56265648
process.stdout.write("You're out of usage credits. Run /usage-credits to keep using Fable 5.\n");
56275649
process.exit(1);
56285650
}
5651+
if (model === "fable" && mode === "probe-tool-use" && !probe) {
5652+
process.stdout.write("You're out of usage credits. Run /usage-credits to keep using Fable 5.\n");
5653+
process.exit(1);
5654+
}
56295655
if (model === "fable" && mode === "zero-exit-quota") {
56305656
process.stdout.write(`${JSON.stringify({ type: "rate_limit_event", rate_limit_info: { status: "rejected" } })}\n`);
56315657
process.stdout.write(`${JSON.stringify({
@@ -5652,6 +5678,13 @@ if (model === "fable" && mode === "zero-exit-malformed" && !probe) {
56525678
}));
56535679
process.exit(0);
56545680
}
5681+
if (probe && mode === "probe-tool-use") {
5682+
process.stdout.write(JSON.stringify([
5683+
{ type: "assistant", message: { content: [{ type: "server_tool_use", name: "advisor" }] } },
5684+
{ type: "rate_limit_event", rate_limit_info: { status: "rejected" } },
5685+
]));
5686+
process.exit(0);
5687+
}
56555688
if (probe) {
56565689
process.stdout.write("available\n");
56575690
process.exit(0);
@@ -5759,9 +5792,24 @@ if (calls.length !== 3 || !calls[1].probe) process.exit(1);
57595792
if (calls[0].model !== "fable" || calls[1].model !== "fable" || calls[2].model !== "claude-opus-4-8") process.exit(1);
57605793
if (receipt.reviewers.cross_model.route !== "fallback") process.exit(1);
57615794
if (receipt.reviewers.cross_model.fallback_reason !== "quota_exhausted") process.exit(1);
5762-
if (receipt.reviewers.cross_model.model !== "claude-opus-4-8-20260801") process.exit(1);
5795+
if (receipt.reviewers.cross_model.model !== "claude-opus-4-8-20260801") process.exit(1);
57635796
NODE
57645797

5798+
rm -f "$receipt"
5799+
: > "$calls"
5800+
set +e
5801+
output=$(cd "$ws" && CODEX_SDLC_TEST_MODE=1 CODEX_SDLC_CODEX_PATH="$fake_codex" \
5802+
CODEX_SDLC_CLAUDE_PATH="$fake_claude" CLAUDE_CALLS="$calls" \
5803+
FALLBACK_TEST_MODE=probe-tool-use node .codex/hooks/dual-review.cjs --base HEAD \
5804+
--consent-subscription-quota 2>&1)
5805+
status=$?
5806+
set -e
5807+
[ "$status" -eq 2 ] || valid=false
5808+
[ "$(wc -l < "$calls" | tr -d ' ')" -eq 2 ] || valid=false
5809+
[ "$(grep -c '"model":"fable"' "$calls")" -eq 2 ] || valid=false
5810+
grep -q 'claude-opus-4-8' "$calls" && valid=false
5811+
[ ! -f "$receipt" ] || valid=false
5812+
57655813
rm -f "$receipt"
57665814
: > "$calls"
57675815
set +e

‎tests/test-review-supervision.cjs‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ const path = require("node:path");
1010
const {
1111
REVIEW_STATES,
1212
classifyReviewFailure,
13+
rejectClaudeReviewerToolUse,
1314
runSupervisedProcess,
1415
runWithInfrastructureRetry,
1516
} = require("../.codex/hooks/dual-review.cjs");
@@ -98,6 +99,99 @@ async function testUnstructuredChatterDoesNotFakeProgress() {
9899
assert.equal(result.terminal_state, REVIEW_STATES.TIMED_OUT);
99100
}
100101

102+
function realClaudeAdvisorEvent() {
103+
return {
104+
type: "assistant",
105+
message: {
106+
content: [
107+
{ type: "thinking", thinking: "" },
108+
{ type: "server_tool_use", name: "advisor", input: { task: "review" } },
109+
],
110+
},
111+
};
112+
}
113+
114+
function testClaudeToolUseDetectorScansRealAssistantEnvelope() {
115+
assert.equal(
116+
rejectClaudeReviewerToolUse(realClaudeAdvisorEvent()),
117+
"forbidden reviewer tool use: advisor",
118+
);
119+
}
120+
121+
function testClaudeToolUseDetectorScansArrayEnvelope() {
122+
assert.equal(
123+
rejectClaudeReviewerToolUse([
124+
realClaudeAdvisorEvent(),
125+
{ type: "result", structured_output: { findings: [], verdict: "CERTIFIED", confidence: 99 } },
126+
]),
127+
"forbidden reviewer tool use: advisor",
128+
);
129+
}
130+
131+
async function testRejectedToolUseCannotBecomeCleanResult() {
132+
const source = [
133+
"const forbidden={type:'assistant',message:{content:[{type:'server_tool_use',name:'advisor'}]}};",
134+
"const clean={type:'result',model:'fable',structured_output:{findings:[],verdict:'CERTIFIED',confidence:99}};",
135+
"process.on('SIGTERM',()=>process.exit(0));",
136+
"process.stdout.write(JSON.stringify(forbidden)+'\\n'+JSON.stringify(clean)+'\\n');",
137+
"setInterval(()=>{},1000);",
138+
].join("");
139+
const result = await runSupervisedProcess(process.execPath, ["-e", source], {
140+
heartbeatInterval: 20,
141+
stallTimeout: 500,
142+
timeout: 800,
143+
killGrace: 30,
144+
structuredEventsRequired: true,
145+
rejectEvent: rejectClaudeReviewerToolUse,
146+
});
147+
148+
assert.equal(result.timedOut, false);
149+
assert.equal(result.terminal_state, REVIEW_STATES.INVALID_VERDICT);
150+
assert.equal(result.rejected_event.reason, "forbidden reviewer tool use: advisor");
151+
assert.match(result.stdout, /\"verdict\":\"CERTIFIED\"/);
152+
}
153+
154+
async function testRejectedFinalBufferCannotBecomeCleanResult() {
155+
const source = [
156+
"const forbidden={type:'assistant',message:{content:[{type:'server_tool_use',name:'advisor'}]}};",
157+
"const clean={type:'result',model:'fable',structured_output:{findings:[],verdict:'CERTIFIED',confidence:99}};",
158+
"process.stdout.write(JSON.stringify([forbidden,clean]));",
159+
].join("");
160+
const result = await runSupervisedProcess(process.execPath, ["-e", source], {
161+
heartbeatInterval: 20,
162+
stallTimeout: 500,
163+
timeout: 800,
164+
killGrace: 30,
165+
structuredEventsRequired: true,
166+
rejectEvent: rejectClaudeReviewerToolUse,
167+
});
168+
169+
assert.equal(result.timedOut, false);
170+
assert.equal(result.terminal_state, REVIEW_STATES.INVALID_VERDICT);
171+
assert.equal(result.rejected_event.reason, "forbidden reviewer tool use: advisor");
172+
}
173+
174+
async function testRejectedToolUseOutranksTimeout() {
175+
const source = [
176+
"process.on('SIGTERM',()=>{});",
177+
"process.stdout.write(JSON.stringify({type:'assistant',message:{content:[{type:'server_tool_use',name:'advisor'}]}})+'\\n');",
178+
"setInterval(()=>{},1000);",
179+
].join("");
180+
const result = await runSupervisedProcess(process.execPath, ["-e", source], {
181+
heartbeatInterval: 25,
182+
stallTimeout: 2000,
183+
timeout: 500,
184+
killGrace: 650,
185+
structuredEventsRequired: true,
186+
rejectEvent: rejectClaudeReviewerToolUse,
187+
});
188+
189+
assert.equal(result.timedOut, true);
190+
assert.equal(result.timeout_reason, "wall");
191+
assert.equal(result.terminal_state, REVIEW_STATES.INVALID_VERDICT);
192+
assert.equal(result.rejected_event.reason, "forbidden reviewer tool use: advisor");
193+
}
194+
101195
function testAuthAndInvalidVerdictClassification() {
102196
assert.equal(classifyReviewFailure({
103197
status: 1,
@@ -139,6 +233,11 @@ async function testOneInfrastructureRetryOnly() {
139233
await testHardTimeoutReapsDescendantTree();
140234
await testCancellationStopsReviewerAndRetainsPrivateLog();
141235
await testUnstructuredChatterDoesNotFakeProgress();
236+
testClaudeToolUseDetectorScansRealAssistantEnvelope();
237+
testClaudeToolUseDetectorScansArrayEnvelope();
238+
await testRejectedToolUseCannotBecomeCleanResult();
239+
await testRejectedFinalBufferCannotBecomeCleanResult();
240+
await testRejectedToolUseOutranksTimeout();
142241
testAuthAndInvalidVerdictClassification();
143242
await testOneInfrastructureRetryOnly();
144243
process.stdout.write("review supervision tests passed\n");

0 commit comments

Comments
 (0)