Bound nodespace logs memory and reuse cross-harness skill removal in uninstall

[mstomar125]

Summary

Two small, independent fixes in packages/cli:

nodespace logs (packages/cli/src/commands/logs.rs) no longer loads the whole daemon log into memory. It used std::fs::read_to_string unconditionally, then filtered/tailed in memory — fine for a desktop-app install where the log is rotated, but a headless (Homebrew/systemd) install has no active log rotation, so its log can grow unbounded for the life of the service. run() now calls:

• tail_matching_lines — scans backward from EOF in fixed-size chunks, stopping as soon as --lines matches are found, instead of reading the whole file
• count_matching_lines — streams forward one line at a time (BufReader::lines) to report the total match count

Output shape is unchanged: same matched/shown JSON fields, same "(N of M matching lines shown …)" message, same line ordering (oldest-first).

Log rotation for headless installs itself is a separate, bigger design question (who rotates — the daemon or the OS service manager — given the log files are the daemon's inherited stdio, not something it opens itself) and is filed as a follow-up: #3225.

nodespace uninstall (packages/cli/src/commands/uninstall.rs) previously only removed ~/.claude/skills/nodespace via a hardcoded remove_claude_skill, leaving Codex/Antigravity/OpenCode/Pi skill files behind on a full uninstall. It now calls report_skill_removal, which reuses commands::skill's existing cross-harness installer (resolve_installer + run_installer_subcommand(&installer, "uninstall")) — the same path nodespace skill uninstall already uses — and reports each detected agent's outcome. A missing or failing installer prints an explicit warning instead of silently leaving skill files behind. remove_claude_skill is deleted.

Test plan

• cargo test -p nodespace-cli — 130 tests pass, including:
  • logs::tests (6 tests): line reconstruction across tiny/adversarial chunk boundaries, limit+filter correctness against a naive whole-file reference across multiple chunk sizes, and an end-to-end correctness check against a real ~48MB generated log file
  • uninstall::tests (4 tests): missing installer, failing installer, cross-harness multi-agent uninstall (via a fake installer script asserting it's invoked with the uninstall subcommand), and the "nothing installed" case
• bun run test:changed — full pyramid ran (bun.lock changed), all tiers passed including rust:test (nextest)
• checked_in_skill_md_is_up_to_date (part of the existing skill_md_generation suite) passed unchanged — no clap doc comments were touched, so packages/skill/references/cli.md needed no regeneration

Closes #2964
Part of #2963

🤖 Generated with Claude Code

[mstomar125]

Review Type: Initial Review — no prior reviews or comments exist on this PR (gh pr view 3226 --json reviews,comments returned empty arrays for both).

Requirements Check:

core#2963 ("nodespace logs loads the entire daemon log into memory, headless installs have no rotation"):

• ✅ read_to_string replaced with a bounded-memory reverse-chunk tail scan (tail_matching_lines) that stops once --lines matches are found, instead of reading the whole file.
• ✅ Total match count computed via a separate forward streaming pass (count_matching_lines, BufReader::lines) rather than materializing every matching line.
• ✅ Output shape/ordering unchanged (verified by direct diff read + passing tests): same matched/shown JSON fields, same "(N of M matching lines shown …)" message, oldest-first ordering.
• N/A (explicitly out of scope, correctly deferred): actual log-rotation for headless installs — filed as Headless (Homebrew/systemd) daemon installs have no log rotation #3225, PR body says "Part of nodespace logs loads the entire daemon log file into memory with no cap, and headless installs have no active log rotation #2963" not "Closes", which is honest scoping.

core#2964 ("nodespace uninstall only removes the Claude Code skill, leaving other harnesses' files behind"):

• ✅ remove_claude_skill's hardcoded ~/.claude/skills/nodespace path fully deleted (verified via grep -rn remove_claude_skill packages/ — zero references remain).
• ✅ uninstall::run now calls the same cross-harness path nodespace skill uninstall uses: skill::resolve_installer() + skill::run_installer_subcommand(&installer, "uninstall").
• ✅ Per-agent outcome reported (installed/removed vs. skipped-with-reason), not a silent pass/fail.
• ✅ Missing/failing installer degrades to an explicit, worded warning rather than a silent no-op (both paths tested).
• ✅ stop_daemon → remove_bin_dir → remove_sock → skill removal → final message ordering preserved exactly from before.

Code Review Findings:

🟡 Important: count_matching_lines and tail_matching_lines disagree on how to handle invalid UTF-8, undermining robustness for exactly the long-lived headless logs this PR targets

📁 packages/cli/src/commands/logs.rs:98-108 (count_matching_lines) vs. 178-186 (String::from_utf8_lossy usage in tail_matching_lines_with_chunk)

count_matching_lines uses BufReader::lines() with line? (logs.rs:104), and BufRead::lines() returns Err for any line containing invalid UTF-8 (confirmed by direct test: BufReader::new(&[b'a', b'\n', 0xFF, 0xFE, b'\n', b'b'][..]).lines() yields Ok("a"), Err("stream did not contain valid UTF-8"), Ok("b")). The ? means the entire count_matching_lines call aborts on the first bad byte anywhere in the file. run() calls count_matching_lines first (logs.rs:248) and propagates its error via ?, so a single stray non-UTF-8 byte anywhere in the log — plausible for a long-lived, never-rotated headless daemon log accumulating raw subprocess output, a truncated write mid-crash, etc. — makes nodespace logs fail outright, even for just viewing the last 20 lines.

Meanwhile tail_matching_lines_with_chunk uses String::from_utf8_lossy per-line (logs.rs ~187), which is fully tolerant of the same bytes and would show the tail fine.

This is not a regression — the old read_to_string also hard-failed on any invalid UTF-8 anywhere in the file, so overall CLI behavior (fail entirely on non-UTF-8) is unchanged. But it is a missed opportunity specific to this PR's own stated goal: the doc comment (logs.rs:15-29) frames this fix around making logs robust for exactly the class of file most likely to accumulate such bytes over a long unrotated lifetime. Recommend: make count_matching_lines lossy too (e.g. read via the same byte-oriented approach, or wrap line? with a lossy fallback) so the two passes agree, and so the much-more-useful "show me the tail" path isn't blocked by a failure in the "count the total" path. Non-blocking for this PR given it doesn't regress prior behavior, but worth a quick follow-up given it's directly in the fix's own problem space.

🟢 Suggestion: near-duplicate InstallOutcome formatting logic now exists in three places

📁 packages/cli/src/commands/uninstall.rs:47-60 (report_skill_removal) vs. packages/cli/src/commands/skill.rs:626-637 (uninstall()) and :657-666 (report_install_outcome)

All three iterate outcome.installed/outcome.skipped and print near-identical ✓ {agent}: … / ⚠ {agent}: {reason} lines, with slightly different wording each time (skill.rs's uninstall() prints "removed", the new report_skill_removal prints "skill removed"). Not a bug — the differing error-handling between report_skill_removal (catches and always degrades gracefully, since the outer nodespace uninstall must never fail on this step) and skill::uninstall() (propagates via ?, since for nodespace skill uninstall failure is the useful signal) is a legitimate reason these aren't literally the same function. But the output-formatting portion (the two for loops) could be factored into one small shared helper taking a writer + &InstallOutcome, cutting duplication and preventing the wording drift already visible ("removed" vs. "skill removed"). Nit-level, not blocking.

🟢 Suggestion: two full linear I/O passes over the log file per invocation

📁 packages/cli/src/commands/logs.rs:243-249

run() always calls both count_matching_lines (full forward scan) and tail_matching_lines (backward scan, early-exits once --lines matches are found) — so every invocation does at least one full read of the file to produce the "N of M" total, regardless of how small --lines is. This is the right tradeoff for the stated goal (bounded memory, not bounded time) and is clearly a deliberate, tested choice (the 50MB fixture test exists specifically to validate this is fast enough in practice). Just flagging as something to watch if headless logs grow into the multi-GB range before #3225 (rotation) lands — not asking for a change here.

Verification performed (beyond reading the diff):

• Ran cargo test --lib logs:: and cargo test --lib uninstall:: in the worktree — all 6 new logs tests and all 4 new uninstall tests pass.
• Ran cargo clippy --lib --tests -- -D warnings on packages/cli — clean, zero warnings.
• Manually traced tail_matching_lines_with_chunk's reverse-chunk/carry reconstruction against a line-spanning-chunks case, a blank line at a chunk boundary, a no-trailing-newline file, and the first-vs-later chunk trailing-empty-piece distinction — the logic matches what the doc comments claim (the carry is always a piece that provably contains no \n, since it comes from a split(b'\n') fragment, so chunk.extend_from_slice(&carry)'s reconstructed last byte is never spuriously treated as a real trailing newline outside the very first, EOF-terminated chunk).
• Confirmed naive_tail reference test helper is independent of the implementation under test (a plain .lines().filter().rev().take(limit).rev() reimplementation, not a call into the code being tested) — the cross-chunk-size property tests are genuine, not tautological.
• grep -rn "2963\|2964\|3225" packages/cli/src/commands/logs.rs packages/cli/src/commands/uninstall.rs — clean, no issue numbers embedded in code/comments.
• grep -rn remove_claude_skill packages/ — clean, fully removed, no dangling references.
• Confirmed remove_claude_skill's deletion is safe: its replacement's failure paths (skill::resolve_installer() returning Err, or run_installer_subcommand returning Err on a non-zero exit) are both explicitly tested and produce a clear stdout warning rather than silently doing nothing — matches nodespace uninstall only removes the Claude Code skill directory, leaving Codex/Gemini CLI/OpenCode/Pi skill files behind #2964's explicit requirement.

Recommendation: APPROVE

Both fixes are correctly scoped, atomic (two independent commands changed, no unrelated bundling), match their issues' suggested fixes precisely, are well-tested (including a genuine 50MB-generated-log end-to-end test and cross-chunk-size property tests against an independent naive reference), pass clippy clean, and preserve all pre-existing behavior/ordering that wasn't meant to change. The one 🟡 finding (UTF-8 handling asymmetry between the two log-reading passes) is real but does not regress current behavior and is reasonable as a fast follow-up rather than a blocker.

🤖 Generated with Claude Code

[mstomar125]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[mstomar125]

Address-review summary

Following up on the pragmatic-code-review above (APPROVE, no Critical findings).

✅ Addressed (2):

1. 🟡 Important — count_matching_lines hard-errored on invalid UTF-8. It used BufRead::lines(), which fails the whole command on the first invalid-UTF-8 byte anywhere in the log, while tail_matching_lines_with_chunk already decoded lossily. Fixed: switched to a byte-oriented read_until + String::from_utf8_lossy loop matching the tail path's existing decoding, so a stray bad byte in a long-lived, never-rotated headless log (exactly this fix's target case) no longer breaks nodespace logs outright. Added a regression test (count_and_tail_tolerate_invalid_utf8_instead_of_erroring) covering both passes against a log with an embedded invalid byte.

2. 🟢 Suggestion — wording drift. report_skill_removal's success line said "skill removed"; commands::skill's own uninstall() says "removed" for the identical underlying outcome. Aligned to "removed" so nodespace uninstall and nodespace skill uninstall read identically for the same result.

⏭️ Skipped (2), with reasoning:

1. 🟢 Suggestion — extract a shared InstallOutcome formatter across the 3 call sites (report_skill_removal, skill::uninstall(), skill::report_install_outcome). Evaluated and skipped: the codebase already formats the shared InstallOutcome/SkippedAgent structs per-consumer rather than through one shared print function — commands::skill's own doc comment says the struct is shared (with commands::mcp too), not its presentation, and status() already uses a different skipped-line format (indent, no ⚠) than install()/uninstall(). Full extraction would go against an established pattern in this codebase for marginal benefit on a 🟢-rated suggestion. The one concrete inconsistency it evidenced (wording drift) is fixed above.

2. 🟢 Suggestion — two full I/O passes per nodespace logs call (count then tail). The reviewer explicitly flagged this "for awareness," not as a change request — it's a deliberate, tested memory-vs-time tradeoff (bounded memory was the actual bug; two O(n)/O(scanned) passes is the accepted cost), covered by the existing 50MB-generated-log test.

Verification after these fixes:

• cargo fmt -p nodespace-cli — one formatting fix applied and committed (line-wrap in a test helper)
• cargo clippy -p nodespace-cli --all-targets -- -D warnings -D clippy::unused-async — clean, zero warnings
• cargo test -p nodespace-cli — all green (131 lib tests incl. 7 in logs::, 4 in uninstall::)
• bun run test:changed — scoped to rust+scripts for this diff, all tiers passed including the Rust nextest tier

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: Both addressed items are small, mechanical, low-risk changes that align existing code with an already-established pattern elsewhere in the same crate (matching tail_matching_lines_with_chunk's existing lossy-decoding convention, and skill.rs's existing wording convention) rather than introducing new logic, architecture, or design decisions. Both skipped items were explicitly low-priority (🟢 Suggestion) with reasoning given above. No new functionality was added, no interconnected changes were made, and the fix is verified with a new regression test plus a full clean test/lint run.

PR remains unmerged, pending the user's decision to merge.