feat: stop captured error_output at first null byte (learning-capture spec edge case) #41

New Issue

2026-04-25T11:05:06+02:00

product-owner commented

2026-04-25 11:05:06 +02:00

Problem Statement

The learning-capture specification interview (docs/specifications/learning-capture-specification-interview.md, "Edge Cases & Boundaries" section) explicitly mandates:

Binary output: Truncate at first null byte. Store only the text portion of error output.

capture_failed_command() at crates/terraphim_agent/src/learnings/capture.rs:933 does not honour this. The current pipeline is:

ignore-check -> parse_chained_command -> redact_secrets -> annotate_with_entities -> write file

There is no null-byte truncation step. When a captured command emits binary stderr (e.g. a Rust panic with binary debug bytes, a coredump path containing NUL, an ldd of a stripped ELF, a hex-dumped buffer), the entire byte sequence -- including unprintable bytes after the first \0 -- is fed into redact_secrets() and ultimately written to the markdown learning file. This produces:

Markdown files that render as garbled binary in editors and break from_markdown() parsers.
redact_secrets() regex passes that scan past the first \0 and either miss redactions on the post-NUL tail or panic on non-UTF-8 sequences.
annotate_with_entities() calls into terraphim_automata::find_matches() with binary input, polluting the entity index with junk tokens.
Storage bloat for what is effectively unrecoverable garbage.

Verification:

rg -n "find\(.0|null.byte|truncate_at_null|\\\\x00" crates/terraphim_agent/src/learnings/   -> 0 matches
rg -n "binary|null" crates/terraphim_agent/src/learnings/capture.rs                          -> only doc-comments, no truncation

The spec is unambiguous: store only the text portion. We have not implemented it.

Acceptance Criteria

Add a private helper truncate_at_null_byte(s: &str) -> &str (or Cow<str>) in crates/terraphim_agent/src/learnings/capture.rs that returns the slice up to the first \0, full input if none found
capture_failed_command() calls truncate_at_null_byte(error_output) BEFORE redact_secrets(), so all downstream stages (redaction, annotation, file write) see only the text portion
If truncation occurred, append a single line marker to the captured error output: [binary output truncated at offset N] so the reviewer knows data was dropped
parse_chained_command() is also passed truncated command if the command itself contains \0 (defensive; should not happen in practice but matches the spec's "raw string" intent)
Unit tests in capture.rs cover:
- error_output with no NUL byte (passthrough, no marker appended)
- error_output with NUL at offset 0 (empty text portion, marker appended)
- error_output with NUL mid-stream (text up to NUL plus marker)
- error_output containing both a secret BEFORE the NUL and binary garbage AFTER (verify secret redacted, garbage dropped)
- error_output with multiple NUL bytes (only first matters)
cargo test -p terraphim_agent learnings::capture passes
cargo clippy -p terraphim_agent -- -D warnings passes
No new dependencies introduced

Proposed Approach

Crates affected: crates/terraphim_agent only.

Files to modify:

crates/terraphim_agent/src/learnings/capture.rs:
- Add fn truncate_at_null_byte(s: &str) -> (&str, Option<usize>) returning the truncated slice and the offset at which truncation occurred (None = no NUL found).
- In capture_failed_command() insert one call after the ignore/chain checks:
```
let (clean_error, truncated_at) = truncate_at_null_byte(error_output);
let error_for_storage = match truncated_at {
    Some(off) => format!("{}\n[binary output truncated at offset {}]", clean_error, off),
    None => clean_error.to_string(),
};
let redacted_error = redact_secrets(&error_for_storage);
```
- Apply the same helper to command defensively before parse_chained_command.
Add the five unit tests listed above to the existing #[cfg(test)] mod tests block at the bottom of the same file.

Why this approach: Pure additive change inside one function. Keeps the redaction / annotation / write order unchanged. The marker line is plain ASCII so it round-trips through CapturedLearning::to_markdown and from_markdown without escaping.

Dependencies

None. Independent of #38 (config loading), #40 (auto-suggest), #24 (chained command isolation). May share a future config knob (learnings.truncation.binary_marker) but that is out of scope for this PR.

Verification

cargo test -p terraphim_agent learnings::capture::tests::test_truncate_at_null_byte_no_nul
cargo test -p terraphim_agent learnings::capture::tests::test_truncate_at_null_byte_at_zero
cargo test -p terraphim_agent learnings::capture::tests::test_truncate_at_null_byte_mid_stream
cargo test -p terraphim_agent learnings::capture::tests::test_truncate_preserves_secret_redaction
cargo test -p terraphim_agent learnings::capture
cargo clippy -p terraphim_agent -- -D warnings

Manual smoke test (records a capture with embedded NUL and confirms file is text-only):

printf 'visible-text\0\x01\x02\x03binary' | xargs -0 -I{} \
  ./target/debug/terraphim-agent learn capture "demo" --error "{}" --exit-code 1 --debug
ls -la ~/.terraphim/learnings/ | head
file ~/.terraphim/learnings/learning-*.md   # expect 'ASCII text' or 'UTF-8 Unicode text'

Scope

Single-file change (~40-60 lines of code + ~80 lines of tests). Single PR, well under the 500-line cap.

Upstream: This issue tracks a feature for terraphim/terraphim-ai

## Problem Statement The learning-capture specification interview (`docs/specifications/learning-capture-specification-interview.md`, "Edge Cases & Boundaries" section) explicitly mandates: > **Binary output**: Truncate at first null byte. Store only the text portion of error output. `capture_failed_command()` at `crates/terraphim_agent/src/learnings/capture.rs:933` does not honour this. The current pipeline is: ``` ignore-check -> parse_chained_command -> redact_secrets -> annotate_with_entities -> write file ``` There is no null-byte truncation step. When a captured command emits binary stderr (e.g. a Rust panic with binary debug bytes, a coredump path containing NUL, an `ldd` of a stripped ELF, a hex-dumped buffer), the entire byte sequence -- including unprintable bytes after the first `\0` -- is fed into `redact_secrets()` and ultimately written to the markdown learning file. This produces: - Markdown files that render as garbled binary in editors and break `from_markdown()` parsers. - `redact_secrets()` regex passes that scan past the first `\0` and either miss redactions on the post-NUL tail or panic on non-UTF-8 sequences. - `annotate_with_entities()` calls into `terraphim_automata::find_matches()` with binary input, polluting the entity index with junk tokens. - Storage bloat for what is effectively unrecoverable garbage. Verification: ``` rg -n "find\(.0|null.byte|truncate_at_null|\\\\x00" crates/terraphim_agent/src/learnings/ -> 0 matches rg -n "binary|null" crates/terraphim_agent/src/learnings/capture.rs -> only doc-comments, no truncation ``` The spec is unambiguous: store only the text portion. We have not implemented it. ## Acceptance Criteria - [ ] Add a private helper `truncate_at_null_byte(s: &str) -> &str` (or `Cow<str>`) in `crates/terraphim_agent/src/learnings/capture.rs` that returns the slice up to the first `\0`, full input if none found - [ ] `capture_failed_command()` calls `truncate_at_null_byte(error_output)` BEFORE `redact_secrets()`, so all downstream stages (redaction, annotation, file write) see only the text portion - [ ] If truncation occurred, append a single line marker to the captured error output: `[binary output truncated at offset N]` so the reviewer knows data was dropped - [ ] `parse_chained_command()` is also passed truncated `command` if the command itself contains `\0` (defensive; should not happen in practice but matches the spec's "raw string" intent) - [ ] Unit tests in `capture.rs` cover: - `error_output` with no NUL byte (passthrough, no marker appended) - `error_output` with NUL at offset 0 (empty text portion, marker appended) - `error_output` with NUL mid-stream (text up to NUL plus marker) - `error_output` containing both a secret BEFORE the NUL and binary garbage AFTER (verify secret redacted, garbage dropped) - `error_output` with multiple NUL bytes (only first matters) - [ ] `cargo test -p terraphim_agent learnings::capture` passes - [ ] `cargo clippy -p terraphim_agent -- -D warnings` passes - [ ] No new dependencies introduced ## Proposed Approach **Crates affected:** `crates/terraphim_agent` only. **Files to modify:** - `crates/terraphim_agent/src/learnings/capture.rs`: - Add `fn truncate_at_null_byte(s: &str) -> (&str, Option<usize>)` returning the truncated slice and the offset at which truncation occurred (None = no NUL found). - In `capture_failed_command()` insert one call after the ignore/chain checks: ```rust let (clean_error, truncated_at) = truncate_at_null_byte(error_output); let error_for_storage = match truncated_at { Some(off) => format!("{}\n[binary output truncated at offset {}]", clean_error, off), None => clean_error.to_string(), }; let redacted_error = redact_secrets(&error_for_storage); ``` - Apply the same helper to `command` defensively before `parse_chained_command`. - Add the five unit tests listed above to the existing `#[cfg(test)] mod tests` block at the bottom of the same file. **Why this approach:** Pure additive change inside one function. Keeps the redaction / annotation / write order unchanged. The marker line is plain ASCII so it round-trips through `CapturedLearning::to_markdown` and `from_markdown` without escaping. ## Dependencies None. Independent of #38 (config loading), #40 (auto-suggest), #24 (chained command isolation). May share a future config knob (`learnings.truncation.binary_marker`) but that is out of scope for this PR. ## Verification ```bash cargo test -p terraphim_agent learnings::capture::tests::test_truncate_at_null_byte_no_nul cargo test -p terraphim_agent learnings::capture::tests::test_truncate_at_null_byte_at_zero cargo test -p terraphim_agent learnings::capture::tests::test_truncate_at_null_byte_mid_stream cargo test -p terraphim_agent learnings::capture::tests::test_truncate_preserves_secret_redaction cargo test -p terraphim_agent learnings::capture cargo clippy -p terraphim_agent -- -D warnings ``` Manual smoke test (records a capture with embedded NUL and confirms file is text-only): ```bash printf 'visible-text\0\x01\x02\x03binary' | xargs -0 -I{} \ ./target/debug/terraphim-agent learn capture "demo" --error "{}" --exit-code 1 --debug ls -la ~/.terraphim/learnings/ | head file ~/.terraphim/learnings/learning-*.md # expect 'ASCII text' or 'UTF-8 Unicode text' ``` ## Scope Single-file change (~40-60 lines of code + ~80 lines of tests). Single PR, well under the 500-line cap. Upstream: This issue tracks a feature for terraphim/terraphim-ai

product-owner referenced this issue

2026-04-25 11:05:54 +02:00

feat: lesson extractor for F5.3 cross-session learning bullet 1 #42

product-owner referenced this issue

2026-04-25 11:06:25 +02:00

feat: criterion benchmark suite for G1 performance targets #43

product-owner referenced this issue

2026-04-25 11:09:06 +02:00

feat: criterion benchmark suite for G1 performance targets #43

product-owner referenced this issue

2026-04-27 07:00:23 +02:00

feat: wire ExitCode into main() return for f1.2 robot exit code contract #35