Skip to content

fix(hooks): pass failed commands through verbatim, never distill a non-zero exit into success#121

Merged
fajarhide merged 1 commit into
mainfrom
fix/120-fail-passthrough
Jul 21, 2026
Merged

fix(hooks): pass failed commands through verbatim, never distill a non-zero exit into success#121
fajarhide merged 1 commit into
mainfrom
fix/120-fail-passthrough

Conversation

@fajarhide

@fajarhide fajarhide commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Closes #120

Problem

A command that failed could be distilled into output that reads as success. That is the worst failure mode: a fabricated success terminates investigation, while a fabricated error only costs a retry. In the filed case a vault call that failed exit=2 on a network timeout surfaced a clean, plausible, fictional ["n8n"].

Root cause (code-visible, not the unreproducible ["n8n"] itself): OMNI's normalize layer parsed each agent's failure signal and threw it away.

agent failure signal state before
Codex CLI exit_code never read into CodexInput
Pi toolResponse.isError #[allow(dead_code)]
Generic MCP result.isError in a doc comment, never deserialized
Claude Code tool_response is a bare string "Error: Exit code N…" parse-failed to None (safe by accident)

So failed commands from Codex/Pi/MCP ran the full distiller.

Fix

The invariant from the issue: a non-zero exit passes through verbatim, never distilled. Implemented at the one place format knowledge already lives:

  • NormalizedInput gains failed, set from each agent's own signal (exit_code != 0, isError).
  • One guard in post_tool::process_payload: if normalized.failed { return None } — passthrough, host keeps the raw bytes at zero marker cost.
  • No is_command_error text heuristic: the guard keys on exit status only, so a successful build log full of the word "error" still distills. Net savings on passing commands are untouched.

Claude Code needs no code change (its failure string already passes through); a regression test locks that in so a future lenient parser can't silently reintroduce the fabrication.

Verified end-to-end (real binary, omni --hook)

input before after
docker build exit_code 1, 9207 B distilled → 6090 B 0 B → passthrough (host keeps raw)
docker build exit_code 0, 9207 B distilled → 6090 B distilled → 6090 B (unchanged)

Teeth: disabling the guard turns the Codex/Pi/MCP tests red; restoring it makes them green.

Gates: cargo fmt / clippy -D warnings / cargo test (1027 passed, 0 failed) — run on local Homebrew 1.94.0, not CI's pinned 1.97.0, so CI clippy is the source of truth.

Out of scope (follow-up)

The omni exec/pipe.rs path reads piped stdout only and never sees the child exit code; hardening it needs exec.rs to capture and forward the code — a separate change, not this one.

PR Auto Describe

Summary

Added failed field to NormalizedInput to track non-zero command exits or error signals per-agent. Failed commands now pass through verbatim instead of being distilled — preventing fabricated success summaries that would terminate investigation early. Fixes #120 with full test coverage across all affected parsers.

Key Changes

  • New field: NormalizedInput.failed: bool tracks command failure per agent format
  • Passthrough guard: process_payload returns None (verbatim pass) when failed == true
  • Per-agent parsing: exit codes / isError flags wired into failed where available
  • Tests: 5 new cases covering Codex, Pi, MCP, and Claude Code failure paths

Detailed Breakdown

  • normalize_claude_code: always sets failed: false — failures arrive as bare strings, not objects, so never reach the normalizer
  • normalize_pi: reads isError boolean into failed
  • normalize_opencode / normalize_vscode_continue / normalize_aider: set failed: false — no exit/error signal in payload
  • normalize_codex: reads exit_code field; failed = exit_code.is_some_and(|c| c != 0)
  • normalize_generic_mcp: reads isError from McpResultContent into failed
  • process_payload: early return None when normalized.failed — host keeps original bytes; no risk of a distilled success hiding a real failure

Notes

  • Design decision: "fabricated success terminates investigation; fabricated error only costs a retry" — skewed toward preserving fidelity of failed commands.
  • Claude Code bare-string failures already passthrough via passthrough guard; the failed: false here documents intent and guards future parser changes.

Breaking Changes

None.

Last updated: 2026-07-21 00:51:52

…n-zero exit into success (#120)

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.
@fajarhide
fajarhide merged commit e6545d6 into main Jul 21, 2026
9 checks passed
@fajarhide
fajarhide deleted the fix/120-fail-passthrough branch July 21, 2026 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Distillation can fabricate a successful result for a command that failed with a network error

1 participant