daemon: extract_err_from_stderr fallback returns 11 lines (not the documented 10) and is entirely untested

[phil-opp]

Note: This issue was created by a scheduled automated Claude code-review check. A component was selected at random (binaries/daemon/src/extract_err_from_stderr.rs) and reviewed for correctness issues. Please treat the severity below as a starting point for human triage, not a confirmed defect. Severity: low / cosmetic — this affects only the diagnostic error string surfaced when a node dies; it does not affect dataflow execution.

(For context: I first looked at mavlink2-bridge, but that component is already covered by the earlier automated reviews #2075 and #2023, so I moved to a component that had not been reviewed.)

Summary

extract_err_from_stderr (used by the daemon to build the NodeErrorCause::Other { stderr } diagnostic when a node exits with an error — binaries/daemon/src/lib.rs:4162-4177) has a small off-by-one in its fallback path, plus a cosmetic line-joining glitch, and has no test coverage at all.

Affected code

// binaries/daemon/src/extract_err_from_stderr.rs:19-27
// default to last 10 lines if no start marker was found
let start_line_idx_from_end = start_line_idx_from_end.unwrap_or(10);

let start_line_idx = stderr
    .len()
    .saturating_sub(1)
    .saturating_sub(start_line_idx_from_end);

stderr.into_iter().skip(start_line_idx).collect()

1. Off-by-one in the "last 10 lines" fallback (real, minor)

When no start marker is found, start_line_idx_from_end = 10, so for a buffer of len lines:

start_line_idx = len - 1 - 10 = len - 11
skip(len - 11)  ->  yields indices [len-11, len)  ->  11 lines

So the fallback returns the last 11 lines, while the comment says "last 10 lines". To actually return 10, the constant should be 9 (or the comment should say 11). start_line_idx_from_end is "distance from the last line", so N lines back from the end spans N + 1 lines inclusive.

2. Cosmetic: the "[...]" truncation marker is concatenated without a newline

The caller prepends a marker when the ring buffer overflowed:

// binaries/daemon/src/lib.rs:4165-4172
let mut lines = Vec::new();
if queue.is_full() {
    lines.push("[...]".into());   // <- no trailing '\n'
}
while let Some(line) = queue.pop() {
    lines.push(line);             // these DO carry '\n' (read_until(b'\n', ..) keeps the delimiter)
}

extract_err_from_stderr finishes with stderr.into_iter().collect::<String>(), which relies on each element already carrying its own \n. That holds for the real stderr lines (see binaries/daemon/src/spawn/prepared.rs:614-643, which uses read_until(b'\n', ..) and keeps the delimiter), but not for the injected "[...]" marker. When that marker survives into the output (i.e. when the whole buffer is short enough that start_line_idx == 0), it is glued directly onto the first real line, e.g. [...]thread 'main' panicked at .... Minor, but it muddies the very diagnostic this function exists to produce.

Why it matters (and why it's only low severity)

This function is purely diagnostic: its output becomes the human-readable stderr string attached to a NodeError. The off-by-one means an error report shows one extra line of context than advertised, and the marker glitch can fuse [...] to a line. Neither corrupts state or behavior, but both make the surfaced error noisier than intended, and the function being untested means future heuristic tweaks (the marker matchers in StderrMatcher) can regress silently.

Verification

• grep -rn "extract_err_from_stderr\|is_negative_marker\|is_error_start_marker" → only definition + one call site in lib.rs; no #[test] anywhere for this module.
• Hand-traced len=20, no marker: start_line_idx = 9, skip(9) → 11 lines.
• Confirmed each queued stderr line retains its \n via read_until(b'\n', &mut raw) in prepared.rs:615-643, so the collect::<String>() join is correct for real lines but not for the marker.

Suggested fix

1. Make the fallback honest about its count — either unwrap_or(9) (to truly return 10) or update the comment to say 11. Prefer naming the intent:

   const FALLBACK_LINES: usize = 10;
   let start_line_idx_from_end = start_line_idx_from_end.unwrap_or(FALLBACK_LINES - 1);

2. Either push "[...]\n" at lib.rs:4167, or join explicitly in extract_err_from_stderr so the marker can't fuse to the next line.
3. Add a unit-test module covering: the positive markers (Error:, Traceback, panic line), the Python SyntaxError + File two-step, the Warning: negative marker (including the idx == 0 edge where it lands on the last line), and the no-marker fallback line count.

Things I checked and ruled out (no action needed)

• Reverse-scan negative-marker math (idx.saturating_sub(1)): correct — it starts the error at the line just after the Warning: boundary and includes everything to the end. The idx == 0 case (last line is a Warning:) degenerates to returning that warning line, which is a harmless edge case, not a bug.
• Stateful python_syntax_error_found across the reverse scan: correct for real Python output, where SyntaxError: appears below its File line.
• starts_with / contains matchers vs. trailing \n: fine — prefix/substring matching is unaffected by the retained newline.