Skip to content

Commit e6545d6

Browse files
authored
fix(hooks): pass failed commands through verbatim, never distill a non-zero exit into success (#120) (#121)
OMNI's normalize layer parsed each agent's failure signal and then discarded it: Codex `exit_code` was never read into CodexInput, Pi `isError` was `#[allow(dead_code)]`, and MCP `result.isError` was documented in a comment but never deserialized. So a command that failed still ran the full distiller, and a failed command's output could be summarised into something that reads as success — the worst outcome, because a fabricated success terminates investigation while a fabricated error only costs a retry. Fix, at the single point where format knowledge already lives: - NormalizedInput gains `failed`, set from each agent's own signal (Codex exit_code != 0, Pi isError, MCP result.isError). - One guard in post_tool::process_payload: `if normalized.failed { return None }` — passthrough, host keeps the raw bytes at zero marker cost. Claude Code needs no code change: it sends a failed command as a bare `tool_response` string ("Error: Exit code N…") that never matches ClaudeToolResponse, so parsing already bails to None (passthrough). A regression test locks that in so a future, more-lenient parser can't silently reintroduce the fabrication. Verified end-to-end through the real binary: a failed `docker build` (exit_code 1) that previously distilled 9207→6090 B now passes through (0 B emitted, host keeps raw); the same output with exit_code 0 still distills to 6090 B, so savings on successful commands are untouched. Teeth proven by disabling the guard and watching the Codex/Pi/MCP tests go red. Out of scope, noted for follow-up: the `omni exec`/pipe path reads piped stdout only and never sees the child exit code, so hardening it needs exec.rs to capture and forward the code — a separate change.
1 parent 34bc173 commit e6545d6

2 files changed

Lines changed: 106 additions & 5 deletions

File tree

src/hooks/normalize.rs

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ pub struct NormalizedInput {
2424
pub command: String, // command yang dieksekusi
2525
pub content: String, // output dari tool
2626
pub agent_id: String, // untuk session isolation
27+
pub failed: bool, // command exited non-zero / agent signalled an error (#120)
2728
}
2829

2930
/// Detect agent format dari raw JSON string
@@ -179,6 +180,11 @@ fn normalize_claude_code(input: &str, agent_id: String) -> Option<NormalizedInpu
179180
command,
180181
content,
181182
agent_id,
183+
// Claude Code sends a failed command as a bare `tool_response` string
184+
// ("Error: Exit code N…"), which never matches ClaudeToolResponse, so
185+
// parsing bails to None (passthrough) before reaching here. Reaching this
186+
// point means the tool_response object parsed, i.e. the command succeeded.
187+
failed: false,
182188
})
183189
}
184190

@@ -213,7 +219,6 @@ fn normalize_pi(input: &str, agent_id: String) -> Option<NormalizedInput> {
213219
#[derive(Deserialize)]
214220
struct PiToolResponse {
215221
result: Option<Value>,
216-
#[allow(dead_code)]
217222
#[serde(default)]
218223
#[serde(rename = "isError")]
219224
is_error: bool,
@@ -249,6 +254,7 @@ fn normalize_pi(input: &str, agent_id: String) -> Option<NormalizedInput> {
249254
command,
250255
content,
251256
agent_id,
257+
failed: response.is_error,
252258
})
253259
}
254260

@@ -306,6 +312,7 @@ fn normalize_opencode(input: &str, agent_id: String) -> Option<NormalizedInput>
306312
command: parsed.command.unwrap_or_default(),
307313
content,
308314
agent_id,
315+
failed: false, // OpenCode payload carries no exit/error signal
309316
})
310317
}
311318

@@ -375,6 +382,7 @@ fn normalize_vscode_continue(input: &str, agent_id: String) -> Option<Normalized
375382
command,
376383
content,
377384
agent_id,
385+
failed: false, // Continue.dev payload carries no exit/error signal
378386
})
379387
}
380388

@@ -389,6 +397,7 @@ fn normalize_codex(input: &str, agent_id: String) -> Option<NormalizedInput> {
389397
output: Option<String>,
390398
stdout: Option<String>,
391399
stderr: Option<String>,
400+
exit_code: Option<i64>,
392401
}
393402

394403
let parsed: CodexInput = serde_json::from_str(input).ok()?;
@@ -413,6 +422,7 @@ fn normalize_codex(input: &str, agent_id: String) -> Option<NormalizedInput> {
413422
command: parsed.command.unwrap_or_default(),
414423
content,
415424
agent_id,
425+
failed: parsed.exit_code.is_some_and(|c| c != 0),
416426
})
417427
}
418428

@@ -430,6 +440,7 @@ fn normalize_aider(input: &str, agent_id: String) -> Option<NormalizedInput> {
430440
command,
431441
content: input.to_string(),
432442
agent_id,
443+
failed: false, // Aider pipes raw stdout only; no exit signal available
433444
})
434445
}
435446

@@ -444,13 +455,15 @@ fn normalize_generic_mcp(input: &str, agent_id: String) -> Option<NormalizedInpu
444455
#[derive(Deserialize)]
445456
struct McpResultContent {
446457
content: Option<Value>,
458+
#[serde(default)]
459+
#[serde(rename = "isError")]
460+
is_error: bool,
447461
}
448462

449463
let parsed: McpResult = serde_json::from_str(input).ok()?;
450-
let content = parsed
451-
.result
452-
.and_then(|r| r.content)
453-
.and_then(|c| extract_value_content(&c))?;
464+
let result = parsed.result?;
465+
let failed = result.is_error;
466+
let content = result.content.and_then(|c| extract_value_content(&c))?;
454467

455468
if content.is_empty() {
456469
return None;
@@ -465,6 +478,7 @@ fn normalize_generic_mcp(input: &str, agent_id: String) -> Option<NormalizedInpu
465478
command,
466479
content,
467480
agent_id,
481+
failed,
468482
})
469483
}
470484

src/hooks/post_tool.rs

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,14 @@ pub fn process_payload(
3131
) -> Option<String> {
3232
let normalized = crate::hooks::normalize::normalize(input_str)?;
3333

34+
// #120: a command that exited non-zero passes through verbatim — never distilled.
35+
// Distillation must never turn a failed command into output that reads as success
36+
// (a fabricated success terminates investigation; a fabricated error only costs a
37+
// retry). Emit nothing: the host keeps the original bytes at zero marker cost.
38+
if normalized.failed {
39+
return None;
40+
}
41+
3442
if crate::guard::env::is_passthrough() {
3543
return None;
3644
}
@@ -1129,4 +1137,83 @@ mod tests {
11291137
let out = process_payload(&input.to_string(), None, None);
11301138
assert!(out.is_none(), "Edit tool should still return None");
11311139
}
1140+
1141+
// ── #120: failed commands pass through verbatim, never distilled ──────
1142+
1143+
/// Distillable output that a passing command WOULD get summarised, but the
1144+
/// non-zero exit must force passthrough (None).
1145+
fn distillable_noise() -> String {
1146+
std::fs::read_to_string("tests/fixtures/heavy_noise.txt")
1147+
.expect("heavy_noise fixture missing")
1148+
}
1149+
1150+
#[test]
1151+
fn codex_nonzero_exit_passes_through() {
1152+
let input = serde_json::json!({
1153+
"action": "run",
1154+
"command": "docker build .",
1155+
"result": distillable_noise(),
1156+
"exit_code": 1,
1157+
});
1158+
let out = process_payload(&input.to_string(), None, None);
1159+
assert!(
1160+
out.is_none(),
1161+
"a failed command must pass through verbatim, not be distilled"
1162+
);
1163+
}
1164+
1165+
#[test]
1166+
fn codex_zero_exit_still_distills() {
1167+
// Guards the guard: a successful command with the same output is still distilled.
1168+
let input = serde_json::json!({
1169+
"action": "run",
1170+
"command": "docker build .",
1171+
"result": distillable_noise(),
1172+
"exit_code": 0,
1173+
});
1174+
let out = process_payload(&input.to_string(), None, None);
1175+
assert!(
1176+
out.is_some(),
1177+
"a successful command must still be distilled"
1178+
);
1179+
}
1180+
1181+
#[test]
1182+
fn pi_error_passes_through() {
1183+
let input = serde_json::json!({
1184+
"toolName": "Bash",
1185+
"command": "vault kv list apps/x",
1186+
"toolResponse": { "result": distillable_noise(), "isError": true },
1187+
});
1188+
let out = process_payload(&input.to_string(), None, None);
1189+
assert!(out.is_none(), "Pi isError=true must pass through verbatim");
1190+
}
1191+
1192+
#[test]
1193+
fn mcp_error_passes_through() {
1194+
let input = serde_json::json!({
1195+
"jsonrpc": "2.0",
1196+
"id": 1,
1197+
"result": { "content": distillable_noise(), "isError": true },
1198+
});
1199+
let out = process_payload(&input.to_string(), None, None);
1200+
assert!(out.is_none(), "MCP isError=true must pass through verbatim");
1201+
}
1202+
1203+
#[test]
1204+
fn claude_code_failure_string_passes_through() {
1205+
// Claude Code sends a failed command as a bare `tool_response` STRING, which
1206+
// must never be parsed into a success summary. Locks in the passthrough so a
1207+
// future, more-lenient parser can't silently reintroduce the fabrication.
1208+
let input = serde_json::json!({
1209+
"tool_name": "Bash",
1210+
"tool_input": {"command": "vault kv list apps/x"},
1211+
"tool_response": "Error: Exit code 2\nGet \"https://vault/…\": i/o timeout",
1212+
});
1213+
let out = process_payload(&input.to_string(), None, None);
1214+
assert!(
1215+
out.is_none(),
1216+
"Claude Code failure string must pass through verbatim"
1217+
);
1218+
}
11321219
}

0 commit comments

Comments
 (0)