fix: parse_chained_command must isolate the failing sub-command, not parts[0] #24

New Issue

2026-04-25T04:01:42+02:00

product-owner commented

2026-04-25 04:01:42 +02:00

Problem Statement

learnings/capture.rs::parse_chained_command always returns parts[0] as the failing sub-command, regardless of which sub-command actually failed. The current implementation has an explicit TODO marker:

// For now, return the first part as the failing command
// In a more sophisticated implementation, we would track
// which command actually failed based on execution order
return (parts[0].trim().to_string(), Some(command.to_string()));

This contradicts the learning-capture interview spec (docs/specifications/learning-capture-specification-interview.md lines 25-26 and 162-180), which mandates: "Split cmd1 && cmd2 into separate learning documents when cmd2 fails. Each sub-command gets its own capture." The spec example shows that for docker compose up -d && npm run migrate failing on npm run migrate, the captured learning must record command: npm run migrate, not docker compose up -d.

Today's behaviour produces wrong KG entity annotations and wrong correction suggestions for any chained command that fails after the first step -- the most common case for build-test-deploy chains.

Acceptance Criteria

parse_chained_command(command, exit_code) accepts the exit code AND optional position/stderr-line hints, and isolates the actual failing sub-command:
- For && chains: failing sub-command is the last one that ran (inferred from stderr line context where available, otherwise documented as "best-effort: last sub-command in chain")
- For || chains: failing sub-command is the last one that ran (whole chain failed only if all failed)
- For ; chains: failing sub-command is whichever the captured stderr most strongly references; fall back to last
Add failing_subcommand_index: Option<usize> to the return type so the markdown writer can emit failed_at: position N
When stderr/stdout text contains substring matches for one and only one sub-command tail, prefer that sub-command (heuristic: tokenise sub-command, match against last 20 lines of stderr)
Update capture_failed_command callsite to use the richer return type and emit failed_at in the markdown frontmatter
Behaviour is documented in parse_chained_command doc comment with the "best-effort" caveat
Unit tests for at least: simple cmd1 && cmd2 (cmd2 fails), cmd1 || cmd2 (cmd1 fails, cmd2 also fails), cmd1; cmd2; cmd3 (cmd3 fails), pipe-split commands ignored (pipes are not chains for this purpose), single-command (no chain) still returns (command, None)
cargo test -p terraphim_agent --features learn capture::parse_chained_command passes (no env vars; pure function)
cargo clippy -p terraphim_agent --features learn -- -D warnings passes

Proposed Approach

Crates touched: terraphim_agent (learnings/capture.rs only)
Add private helper find_failing_subcommand(parts, error_output) -> Option<usize> that scores each sub-command by stderr-substring overlap (use .split_whitespace().next() to pick the program name, then error_output.contains(prog))
Tie-breaker: latest sub-command in chain order
No new dependencies; reuses existing string operations

Dependencies

None. Self-contained pure-function refactor with tests.

Verification

cargo test -p terraphim_agent --features learn learnings::capture::tests
cargo test -p terraphim_agent --features learn -- chained
cargo clippy -p terraphim_agent --features learn -- -D warnings

Scope

Single PR, ~120-180 lines (helper ~50, callsite update ~10, tests ~80, doc ~10). Pure correctness fix; no public API churn beyond the function's own return type.

## Problem Statement `learnings/capture.rs::parse_chained_command` always returns `parts[0]` as the failing sub-command, regardless of which sub-command actually failed. The current implementation has an explicit TODO marker: ```rust // For now, return the first part as the failing command // In a more sophisticated implementation, we would track // which command actually failed based on execution order return (parts[0].trim().to_string(), Some(command.to_string())); ``` This contradicts the learning-capture interview spec (`docs/specifications/learning-capture-specification-interview.md` lines 25-26 and 162-180), which mandates: "Split `cmd1 && cmd2` into separate learning documents when `cmd2` fails. Each sub-command gets its own capture." The spec example shows that for `docker compose up -d && npm run migrate` failing on `npm run migrate`, the captured learning must record `command: npm run migrate`, not `docker compose up -d`. Today's behaviour produces wrong KG entity annotations and wrong correction suggestions for any chained command that fails after the first step -- the most common case for build-test-deploy chains. ## Acceptance Criteria - [ ] `parse_chained_command(command, exit_code)` accepts the exit code AND optional position/stderr-line hints, and isolates the actual failing sub-command: - For `&&` chains: failing sub-command is the last one that ran (inferred from stderr line context where available, otherwise documented as "best-effort: last sub-command in chain") - For `||` chains: failing sub-command is the last one that ran (whole chain failed only if all failed) - For `;` chains: failing sub-command is whichever the captured stderr most strongly references; fall back to last - [ ] Add `failing_subcommand_index: Option<usize>` to the return type so the markdown writer can emit `failed_at: position N` - [ ] When stderr/stdout text contains substring matches for one and only one sub-command tail, prefer that sub-command (heuristic: tokenise sub-command, match against last 20 lines of stderr) - [ ] Update `capture_failed_command` callsite to use the richer return type and emit `failed_at` in the markdown frontmatter - [ ] Behaviour is documented in `parse_chained_command` doc comment with the "best-effort" caveat - [ ] Unit tests for at least: simple `cmd1 && cmd2` (cmd2 fails), `cmd1 || cmd2` (cmd1 fails, cmd2 also fails), `cmd1; cmd2; cmd3` (cmd3 fails), pipe-split commands ignored (pipes are not chains for this purpose), single-command (no chain) still returns `(command, None)` - [ ] `cargo test -p terraphim_agent --features learn capture::parse_chained_command` passes (no env vars; pure function) - [ ] `cargo clippy -p terraphim_agent --features learn -- -D warnings` passes ## Proposed Approach - Crates touched: `terraphim_agent` (`learnings/capture.rs` only) - Add private helper `find_failing_subcommand(parts, error_output) -> Option<usize>` that scores each sub-command by stderr-substring overlap (use `.split_whitespace().next()` to pick the program name, then `error_output.contains(prog)`) - Tie-breaker: latest sub-command in chain order - No new dependencies; reuses existing string operations ## Dependencies None. Self-contained pure-function refactor with tests. ## Verification ``` cargo test -p terraphim_agent --features learn learnings::capture::tests cargo test -p terraphim_agent --features learn -- chained cargo clippy -p terraphim_agent --features learn -- -D warnings ``` ## Scope Single PR, ~120-180 lines (helper ~50, callsite update ~10, tests ~80, doc ~10). Pure correctness fix; no public API churn beyond the function's own return type.

root referenced this issue

2026-04-25 05:07:16 +02:00

feat: redact api keys/secrets/JWTs during session import (Privacy) #26

product-owner referenced this issue

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

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

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

product-owner referenced this issue

2026-04-27 11:04:34 +02:00

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