Repository navigation
fix(toml): fail open when batch filters strip every line - #253
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ); | ||
| } | ||
|
|
||
| /// #224: the Black TOML filter stripped every row and returned an empty | ||
| /// string. The hook accepted it as 100% compression and replaced non-empty | ||
| /// stdout with nothing. A batch zero-state with no explicit fallback must | ||
| /// decline the rewrite so the host retains its original result. | ||
| #[test] | ||
| fn declines_a_batch_filter_that_removes_every_line() { | ||
| let content = (0..200) | ||
| .map(|i| format!("would reformat src/module_{i}.py")) | ||
| .collect::<Vec<_>>() | ||
| .join("\n"); | ||
| let payload = json!({ | ||
| "tool_name": "Bash", | ||
| "tool_input": {"command": "black --check ."}, | ||
| "tool_response": bash_response(&content), | ||
| }) | ||
| .to_string(); | ||
|
|
||
| assert!( | ||
| process_payload(&payload, None, None).is_none(), | ||
| "the hook must decline instead of replacing stdout with an empty or fabricated result" | ||
| ); | ||
| } | ||
|
|
||
| /// The counter-case: a Black summary is a real signal that survives the | ||
| /// line filter, so the TOML filter must still win rather than turning every | ||
| /// matching command into passthrough. | ||
| #[test] | ||
| fn still_applies_a_batch_filter_when_a_signal_survives() { | ||
| let mut content = String::new(); | ||
| for i in 0..20 { | ||
| content.push_str(&format!("would reformat src/module_{i}.py\n")); | ||
| } | ||
| content.push_str("Oh no! 20 files would be reformatted."); | ||
| let payload = json!({ | ||
| "tool_name": "Bash", | ||
| "tool_input": {"command": "black --check ."}, | ||
| "tool_response": bash_response(&content), | ||
| }) | ||
| .to_string(); | ||
|
|
||
| let out = process_payload(&payload, None, None).expect("signal should be distilled"); | ||
| let v: serde_json::Value = serde_json::from_str(&out).expect("valid hook json"); | ||
| let stdout = v["hookSpecificOutput"]["updatedToolOutput"]["stdout"] | ||
| .as_str() | ||
| .expect("stdout is a string"); | ||
|
|
||
| assert!( | ||
| stdout.contains("Oh no! 20 files would be reformatted."), | ||
| "the surviving signal was lost: {stdout}" | ||
| ); | ||
| assert!( | ||
| !stdout.contains("would reformat src/module_"), | ||
| "configured noise rows survived: {stdout}" | ||
| ); | ||
| } | ||
|
|
||
| /// #212: Claude Code caps the hook payload, and above roughly the same size | ||
| /// it persists the **raw** output to a file, previews the **raw** first 2 KB, | ||
| /// and drops whatever the hook returns. A payload arriving at the cap is |
There was a problem hiding this comment.
declines_a_batch_filter_that_removes_every_line and still_applies_a_batch_filter_when_a_signal_survives both call process_payload with command: "black --check .". process_payload calls load_all_filters(), which includes the embedded signals/tools/black.toml. Those tests therefore depend on that file having no fallback_message. If black.toml gains an on_empty/fallback_message in a future signal update, apply_batch would return Filtered(fallback) instead of Passthrough, flipping the first test from pass to fail — even though the hook logic is untouched.
The three unit tests in toml_filter.rs side-step this by using the in-memory black_noise_filter fixture. The same approach would make the post_tool.rs hook-path tests resilient: inject a TomlFilter directly (or a thin test-only override for load_all_filters) rather than relying on a particular state of the shipped signal file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/hooks/post_tool.rs
Line: 973-1034
Comment:
**Tests couple hook logic to production signal file content**
`declines_a_batch_filter_that_removes_every_line` and `still_applies_a_batch_filter_when_a_signal_survives` both call `process_payload` with `command: "black --check ."`. `process_payload` calls `load_all_filters()`, which includes the embedded `signals/tools/black.toml`. Those tests therefore depend on that file having no `fallback_message`. If `black.toml` gains an `on_empty`/`fallback_message` in a future signal update, `apply_batch` would return `Filtered(fallback)` instead of `Passthrough`, flipping the first test from pass to fail — even though the hook logic is untouched.
The three unit tests in `toml_filter.rs` side-step this by using the in-memory `black_noise_filter` fixture. The same approach would make the `post_tool.rs` hook-path tests resilient: inject a `TomlFilter` directly (or a thin test-only override for `load_all_filters`) rather than relying on a particular state of the shipped signal file.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Not changing this. The coupling is deliberate: the test drives the shipped black.toml through the real hook path, which is where #224 was observed. A test-only override of load_all_filters would be indirection added for one assertion, and the toml_filter.rs unit tests already cover the outcome logic against an in-memory fixture.
If black.toml ever gains a fallback_message, a red test here is the signal worth having, since the filter's zero-state would have changed and someone should confirm the hook still declines for the filters that have none.
There was a problem hiding this comment.
That's a sound justification. The test-as-canary argument is valid — if black.toml gains fallback_message, the red test correctly surfaces that the zero-state contract has changed and needs a conscious review. The toml_filter.rs unit tests cover the outcome logic in isolation; these cover the full hook path. The coupling is load-bearing rather than accidental. Fair call, I'll leave it.
fajarhide
left a comment
fajarhide
left a comment
There was a problem hiding this comment.
CI is green on 89723d5 across all three jobs, and the change is correct. Three notes, none of them blocking.
Verified locally in a worktree before the workflow was approved, on toolchain 1.97.1 rather than the pinned 1.97.0, so CI remains the authoritative result:
cargo fmt --all -- --checkandcargo clippy --all-targets --all-features -- -D warnings: cleancargo test --lib: 525 passedcargo test --test toml_filter_integration: 44 passed- Mutation check: replacing the
apply_batchguard condition withif falsefailsdeclines_a_batch_filter_that_removes_every_lineandpasses_through_a_batch_filter_that_removes_every_line, and leaves the surviving-signal counter-case passing.
The guard mirrors the on_empty check already inside apply, apply itself is untouched so stream mode keeps its per-line semantics, and both hook boundaries follow the fail-open pattern the format-sniff and host-cap gates above them already use.
1. The CHANGELOG entry does not say what the fix costs. The short-circuit takes the distiller out of the picture for every TOML filter that strips its whole input, which #224 counts as 15 of 45 shipped filters. On kubectl, make and terraform that is compression traded for safety. The trade is right, but the entry only describes the safety half of it.
2. Build: ok is #250, and this PR does not close it. The fabrication the passthrough avoids is still reachable on any build-routed command that no TOML filter matches. On 0.6.7:
$ omni exec rustc --print cfg
Build: ok[OMNI Active] ⏺ 99.3% reduction (1.2 KB → 9 B) 77ms
1,208 bytes of configuration data for a 9-byte success claim. src/distillers/build.rs:100 still returns Build: ok whenever no segment scores Critical or Important, and build.rs is on #250's list of nine distillers that never call require_parsed.
3. Tag the new passthrough event. record_passthrough(clean_command, content.len()) writes the same string as the guardrail passthrough at post_tool.rs:547, so the two cannot be told apart in passthrough_events. The two gates above it carry a reason ([host output cap], [{format}]). &format!("{clean_command} [toml zero-state]") keeps the telemetry readable.
|
Merged, thanks for the contribution. Two things made this quick to review. The mutation check in your PR body was real: I reran it here, and removing the passthrough branch does fail both hook regressions while leaving the surviving-signal counter-case green. And you said plainly that the Black trigger is synthetic and that your Windows host could not run the full suite, instead of letting CI discover it. The two notes I left are follow-ups on our side. Nothing to redo here. |
* fix(store): name the gate that produced each passthrough row `passthrough_events` has no reason column, so the reason rides on the end of the command string. Two of the four gates wrote a tag and two wrote the bare command, and after #253 the untagged pair sat on opposite sides of the pipeline recording the same string for opposite findings: black --check . a signal file stripped its whole input, no fallback_message date a distiller produced something too weak to keep Read as one bucket those are the same row. The first names a filter that needs a zero-state written for it; the second names a distiller to improve. This table is queried by hand to decide what to build next, so the ambiguity costs work rather than correctness. `recent_passthroughs` exists only so the tag can be asserted. Nothing in the shipped binary reads this table yet. Closes #254 * docs(changelog): record the passthrough gate tags (#254)
What
fallback_messagefallback_messagevalues, surviving signal lines, and stream-mode suppression unchangedWhy
TomlFilter::applyreturned an empty string when line rules removed every row and the filter had nofallback_message. The post-hook accepted that as 100% compression and replaced non-empty stdout with nothing.Simply returning the input is not sufficient: it misses the size guardrail and falls through to another distiller, which changed the synthetic 200-row Black reproduction into
Build: ok. The batch outcome now makes fail-open explicit at both caller boundaries.The reported Black trigger is deliberately synthetic; normal Black output also prints a summary line, which survives the filter.
Validation
The three Rust files in this patch were fully validated before the upstream 0.6.8 release cut:
cargo fmt --all -- --check(Rust 1.97.0 and 1.97.1)cargo check --all-targets --all-featurescargo buildcargo clippy --all-targets --all-features -- -D warningscargo test --lib: 526 passedcargo test --test toml_filter_integration: 44 passedAfter rebasing onto current
main, those Rust files are byte-identical and the pinned Rust 1.97.0 formatter still passes. On this Windows host, Application Control blocks the newly hashed 0.6.8 build-script executable before check/test can run; it produces no compiler diagnostic or failed assertion. CI should provide the authoritative pinned-toolchain full-suite result.Closes #224
Greptile Summary
This PR fixes a data-loss bug (#224) where
TomlFilter::applyreturned an empty string when its line rules stripped every row and nofallback_messagewas configured. The post-hook accepted that empty string as valid 100%-compression output and silently replaced non-empty tool stdout with nothing.BatchFilterOutcome::Passthrough | Filtered(String)andapply_batch, which distinguishes an all-stripped-no-fallback result from an intentional filter output —applyis left unchanged so stream-mode (one-line-at-a-time) callers that expect\"\"to mean "drop this line" are unaffected.post_tool.rsreturnsNoneonPassthrough(host keeps original bytes) and records a passthrough telemetry event;pipe.rsreturns the originalinput_textwithRoute::Passthroughso the output flows through the normal downstream logging path.Confidence Score: 4/5
The core fix is logically sound and well-covered by targeted unit tests; the one concern is a test-to-signal-file coupling that would surface only if
black.tomlis updatedThe
apply_batchcondition correctly mirrors theon_emptycheck already insideapply, so the two paths stay in sync. Both hook boundaries handlePassthroughcorrectly —post_toolreturnsNoneandpipereturns the original bytes withRoute::Passthrough. The only concern is that the newpost_tool.rsintegration tests load the livesignals/tools/black.tomlviaload_all_filters()and will break if that file ever gains anon_empty/fallback_messagefield, even though the hook code itself would remain correct.Files Needing Attention: src/hooks/post_tool.rs — the two new integration tests are implicitly coupled to the content of signals/tools/black.toml
Important Files Changed
BatchFilterOutcomeenum andapply_batchmethod; cleanly separates "all-lines-stripped with no fallback" (Passthrough) from "filtered result" (Filtered); refactorscapped_filterto use struct update syntax; adds 4 focused unit tests covering all outcome branchesapplycall withapply_batch, early-returnsNoneon Passthrough (fail-open), records telemetry viarecord_passthrough; new integration tests rely on the productionblack.tomlsignal file content, creating a coupling that could silently break if that file gains afallback_messageapplycall withapply_batch, propagatesRoute::Passthroughwhen filter strips every line (returning original input bytes), adds a regression test; asymmetry withpost_tool.rs(no explicitrecord_passthroughcall) is intentional — telemetry flows through theRouteenum downstreamFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["TOML filter matches command"] --> B["apply_batch(input)"] B --> C{"apply() result"} C -->|"output non-empty\n(signal survives or fallback set)"| D["Filtered(output)"] C -->|"output empty/whitespace\nAND on_empty is None\nAND input non-empty"| E["Passthrough"] C -->|"on_empty is Some\n(output = fallback from apply)"| D D --> F{"beats_guardrail?"} F -->|"Yes"| G_hook["post_tool: return Some(distilled)\npipe: Route::Keep"] F -->|"No"| H["Fall through to distiller"] E --> I_hook["post_tool: record_passthrough\nreturn None (host keeps original)"] E --> I_pipe["pipe: return input_text\nRoute::Passthrough"]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(toml): fail open on empty batch outp..." | Re-trigger Greptile