Migrate diagnostics to nodespace CLI subcommand (closes #1139)

[malibio]

Summary

• Adds nodespace diagnostics CLI subcommand reporting database path, existence, size, total node count, root node count, schema count, and recent node IDs (with --json mode).
• Deletes commands/diagnostics.rs from the Tauri app along with its two #[tauri::command] handlers and the now-broken "Database" / "Persistence Test" tabs in the developer panel. Call Logs tab stays.
• Hoists resolve_db_path() into nodespace-daemon's library surface so the CLI and daemon agree on the database location without a settings RPC.

Why a CLI, not a Tauri command

Under ADR-031, diagnostic data lives in nodespaced. The intended audience (developers debugging persistence) uses the shell, not the desktop UI — so a CLI subcommand is the correct surface, and we avoid adding a dedicated gRPC service just for debugging data.

Drive-by fix

PR #1147 added id and insert_after_node_id fields to CreateNodeRequest in the proto but missed five call sites that still use the literal-struct form: packages/cli/src/commands/node.rs, packages/cli/tests/cli_integration.rs, and nine sites in packages/daemon/tests/grpc_round_trip.rs. Without populating those fields, `cargo build -p nodespace-cli` and `cargo test` on both test crates fail on a fresh clone of main. Fixed here so this PR is testable; could split out if preferred.

Sample output

```
NodeSpace Diagnostics
─────────────────────────────────────
Database path: /Users/me/.nodespace/daemon-db
Database exists: yes
Database size: 142.31 MB
Total nodes: 1847
Root nodes: 23
Schemas: 5
Recent node IDs: node:abc123, node:def456, ...
```

Acceptance criteria

• commands/diagnostics.rs deleted from Tauri app with all references removed
• nodespace diagnostics CLI subcommand implemented in packages/cli
• Reports: database path, existence, size, total node count, root node count, recent node IDs
• Output is human-readable (labeled lines) + JSON via --json
• cargo build succeeds (workspace)
• cargo clippy --all-targets -- -D warnings -D clippy::unused-async clean
• Frontend bun run test: 128 files / 3916 tests pass (baseline was 128 / 3918 — diff = 2 tests for the deleted helpers)
• Desktop-app bun run quality:fix (eslint + svelte-check) clean

Notes

• `bun run quality:fix` at the repo root fails inside `packages/agent-tools` with `TS2688: Cannot find type definition file for 'node'` — pre-existing on main (PR Add @nodespace/agent-tools TypeScript gRPC client package (closes #1122) #1146 declared `@types/node` in agent-tools `devDependencies` but it doesn't hoist into the root workspace). Out of scope here.
• "Recent node IDs" returns the first N from `QueryNodesSimple` (no `ORDER BY created_at DESC` available in the current RPC). Matches the original Tauri implementation's stated limitation. Can tighten when a sort field lands.
• The diagnostic panel keeps its Call Logs view because `backend-adapter.ts` still wraps 18 Tauri calls through `withDiagnosticLogging`; that's an independent debugging surface.

Test plan

• `cargo build --workspace` succeeds
• `cargo test -p nodespace-cli --lib` — new `format_size_picks_units` and `directory_size_handles_missing_path` pass
• With `nodespaced` running, `nodespace diagnostics` prints the expected summary
• `nodespace diagnostics --json` parses as valid JSON
• `bun run test` from `packages/desktop-app`: no regressions vs baseline
• Ctrl+Shift+D in the desktop app still toggles the diagnostic panel; Call Logs tab works

🤖 Generated with Claude Code

[malibio]

Code Review: PR #1150 — Migrate diagnostics to nodespace CLI subcommand

Review Type: Initial review
Branch: worktree-issue-1139-cli-diagnostics
Verdict: Approve with minor improvements suggested

Requirements Check (Issue #1139)

Acceptance Criterion	Status	Notes
commands/diagnostics.rs deleted from Tauri app, all refs removed	✅	Confirmed via grep — no orphan references.
nodespace diagnostics CLI subcommand in packages/cli	✅	packages/cli/src/commands/diagnostics.rs
Reports: db path, existence, size, total/root node count, recent IDs	✅	Plus schema count + --json bonus
Output human-readable (table/labeled lines)	✅	Labeled lines, mirrors issue spec
cargo build succeeds	✅	cargo build -p nodespace-cli clean locally
bun run quality:fix passes	✅	Frontend test for diagnostic-logger.ts passes (16/16)
cargo clippy --all-targets -- -D warnings -D clippy::unused-async	✅	Verified clean

Findings

🟡 Important

1. recent_node_ids label is misleading — packages/cli/src/commands/diagnostics.rs:94-102
The field is documented as "recent" in the proto, output, and JSON key, but the implementation takes the first N from an unordered QueryNodesSimple result with no ORDER BY semantics. The inline comment honestly acknowledges this, but the user-visible label does not. A developer running nodespace diagnostics reasonably expects "most recent" to mean "most recently created" — getting the first N from an unspecified iteration order is a different (and arguably useless) signal for debugging persistence. Rationale: an honest diagnostic tool should not lie about what it reports.

Suggested fix: Rename to sample_node_ids (output, JSON key, struct field) until QueryNodesSimple exposes an ordered variant; or filter all_nodes by created_at descending in-memory before taking N. The second fix is one line and reuses data already in hand:

let mut sorted = all_nodes.clone();
sorted.sort_by(|a, b| b.created_at.cmp(&a.created_at));
let recent_node_ids: Vec<String> = sorted.iter().take(RECENT_LIMIT).map(|n| n.id.clone()).collect();

This is O(n log n) on a bounded n ≤ 100k — diagnostics is not a hot path, so the cost is fine.

2. directory_size swallows IO errors silently — packages/cli/src/commands/diagnostics.rs:124-143
fs::read_dir errors, entry.metadata() failures, and unknown file types all return 0 with no signal. The errors: Vec<String> field exists on the report exactly to surface this kind of partial failure, but directory_size has no way to push into it. For a diagnostics tool whose users are debugging "why are my nodes not persisting" issues, silently reporting Database size: 0.00 B when the real cause is a permissions error is exactly the wrong behavior. Rationale: the principle of error visibility — a debug tool should report its own failure modes, not hide them.

Suggested fix: Have directory_size return (u64, Vec<String>) or take &mut Vec<String> to accumulate errors; report them through the existing errors field.

3. Symlink handling is correct but undocumented — packages/cli/src/commands/diagnostics.rs:124-143
Verified: DirEntry::file_type() does NOT follow symlinks on Unix (uses lstat), so the implementation cannot loop infinitely on a symlinked directory. However, this also means symlinked files are silently excluded from the size total. For RocksDB this is almost certainly fine (no symlinks expected), but a one-line // comment explaining the choice ("symlinks are intentionally skipped to avoid follow loops; RocksDB stores no symlinks") would prevent a future maintainer from "fixing" this back to fs::metadata() (which DOES follow symlinks and would reintroduce the loop risk).

🟢 Suggestions

4. QUERY_LIMIT = 100_000 silently truncates with no signal — packages/cli/src/commands/diagnostics.rs:19, 88
If a database exceeds 100k nodes, total_node_count becomes meaningless without the user knowing. Diagnostics is precisely the tool that should be honest about this. Suggested fix: if all_nodes.len() == QUERY_LIMIT as usize, push a warning into errors: format!("Result truncated at QUERY_LIMIT={QUERY_LIMIT}; counts may be undercounts"). Cheap, defensive, no behavior change for normal cases.

5. Serial RPC calls — packages/cli/src/commands/diagnostics.rs:69, 104
query_nodes_simple and get_all_schemas are independent and could be tokio::try_join!'d. This is a developer tool over localhost gRPC, so the latency savings are negligible (low ms) and the current code is more readable. Not worth changing unless there's another reason to touch the file. Nit.

6. bind_addr in main.rs left behind — packages/daemon/src/main.rs:28-33
Now that resolve_db_path() is hoisted to lib.rs, the symmetric bind_addr() helper remains in main.rs. Not a bug — bind_addr is genuinely binary-only — but worth a future thought if a settings RPC ever needs to expose the bind address too.

CLAUDE.md Compliance

• ✅ No #[allow(dead_code)], #[deprecated], or lint suppressions introduced.
• ✅ Comments explain "why" not "what" throughout the new file (e.g., the rationale for QUERY_LIMIT, the parent_id filter explanation, the symlink-skipping behavior is implicit).
• ✅ Old code deleted cleanly — no orphaned types, helpers, or tests. The getDatabaseDiagnostics / testNodePersistence helpers and their tests in diagnostic-logger.ts were correctly removed alongside the Tauri commands they wrapped. The two now-broken panel tabs ("Database", "Persistence Test") were correctly removed; the "Call Logs" tab stays because backend-adapter.ts still uses withDiagnosticLogging to wrap 18 Tauri calls. This is the right call under the "delete deprecated code immediately" policy — leaving the tabs would have left broken UI calling deleted commands.
• ✅ The CreateNodeRequest drive-by fix (11 sites) is purely mechanical — adds two empty-string fields to literal struct constructions that proto required after #1147. Verified: no behavioral implications, the proto fields are optional inputs. Bundling this into the diagnostics PR is defensible because without it cargo build -p nodespace-cli fails on a fresh main clone, blocking testability of this PR. Acceptable scope inclusion.
• ✅ The two cargo fmt-only changes (packages/agent/src/pty/manager.rs, packages/daemon/src/services/node_service.rs) are noise but harmless. Including them is consistent with running rust:quality:fix and not selectively reverting unrelated formatting.

Architecture & Design

• ✅ Hoisting resolve_db_path() into nodespace-daemon's library surface (rather than going through a not-yet-merged settings RPC) is the correct call. The CLI is documented as a "stateless gRPC client" but in practice, for an inherently colocated developer tool that introspects on-disk state, reading the same env var the daemon reads is simpler and has no failure modes the RPC path wouldn't share. The function's docstring explicitly calls out this dual-purpose use. When nodespaced eventually moves off-host (if ever), the CLI's diagnostics will need to switch to a settings RPC, but per CLAUDE.md ("no migration strategies, implement final architecture") that's a problem for the day it actually happens, not now.
• ✅ The serde_json::json!() macro usage for output is consistent with packages/cli/src/output.rs's existing style. No inconsistency concern.

Testing Strategy

• 🟡 Coverage gap: only format_size and directory_size_handles_missing_path have unit tests. The risky surface — collect() against a live gRPC channel, root-vs-non-root counting, error-path accumulation — has zero tests despite the established spawn_test_daemon() pattern in packages/cli/tests/cli_integration.rs making this cheap. Suggested test: spawn the in-process daemon, create one root and two child nodes, run collect(), assert total_node_count == 3, root_node_count == 1, recent_node_ids.len() == 3. That's ~15 lines and would have caught (for example) a future regression in the parent_id filter or a swap of RECENT_LIMIT vs QUERY_LIMIT. Not blocking, but the gap is disproportionate to the risk.

Recommendation

Approve with minor improvements. The PR is a clear net improvement: it correctly relocates a feature that lived in the wrong process (the Tauri side could not access ADR-031 daemon state), cleans up all orphaned code, and includes a defensible drive-by that unblocks fresh-clone testability. The findings above are quality refinements, not blockers — the "recent" label honesty and silent-IO-error issues are the highest-priority follow-ups and could be addressed in a small follow-up PR or in-PR before merge per the author's preference.

🤖 Generated with Claude Code

[malibio]

Review feedback addressed

Pushed 0b3183d7. Summary of what changed vs the review:

#	Finding	Status	Notes
1	🟡 recent_node_ids ordering misleading	✅ Addressed	Sort by created_at DESC in-memory before slicing — label is now accurate.
2	🟡 directory_size swallows IO errors	✅ Addressed	Takes &mut Vec<String>; surfaces read_dir, file_type, and metadata failures into the report's errors.
3	🟡 Symlink behavior undocumented	✅ Addressed	Doc comment on directory_size explains the lstat choice and warns future maintainers not to "fix" it to fs::metadata (which would reintroduce loop risk).
4	🟢 QUERY_LIMIT truncation silent	✅ Addressed	When all_nodes.len() == QUERY_LIMIT, pushes a warning into errors.
5	🟢 Serial RPC calls (nit)	⏭️ Skipped	Reviewer explicitly noted "Not worth changing unless there's another reason to touch the file." Localhost, ms-scale.
6	🟢 bind_addr hoist	⏭️ Skipped	Reviewer noted "Not a bug" — bind_addr is binary-only and the CLI doesn't need it.
—	🟡 Integration test coverage gap	✅ Addressed	New diagnostics_collect_reports_counts_and_recency in cli_integration.rs — spawns the in-process daemon, captures a baseline, seeds 3 nodes with timestamp gaps, asserts +3 total + newest-first + no errors. Also added a unit test exercising the happy path of directory_size on a nested tempdir.

Bug uncovered by the new integration test

The new test caught that root_node_count was always equal to total_node_count, regardless of how many child nodes were created. Root cause: the daemon's query_nodes_simple handler returns NodeData with parent_id = None for every row because parentage is a graph edge in SurrealDB, not a column on the node table.

There is no honest way to compute root count from the current RPC surface. I dropped root_node_count from the report (and the human/JSON output) with an inline comment noting that a GetRoots RPC or extending NodeListResponse with parent IDs is needed before this can come back.

This means the original issue's "Reports… root node count" acceptance criterion now requires a follow-up issue rather than being met by this PR. Flagging explicitly — happy to file the follow-up if reviewers agree this scope split is correct. The alternative (adding a GetRoots RPC inside this PR) felt like it would balloon scope inappropriately for a "migrate command from A to B" task.

Test status

• cargo test -p nodespace-cli: 5/5 integration + 5/5 lib tests pass (was 4 integration + 4 lib before; +1 integration test, +1 lib test).
• cargo fmt --all: clean
• cargo clippy --all-targets -- -D warnings -D clippy::unused-async: clean
• bun run --cwd packages/desktop-app quality:fix: clean
• bun run test (frontend): 3916 / 3916 in isolation. A perf-timing test (multi-tab-reactivity) and marked-config large-text flaked under heavy CPU load on one run; both pass in isolation — unrelated to this change (Rust-only diff this round).

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: I made a non-trivial behavior change (removing root_node_count from the report) that goes beyond pure feedback-addressing and partially affects the issue's acceptance criteria. Also added a new public API (pub async fn collect, pub struct DiagnosticsReport) for test access. Worth a second look at: (a) whether dropping root count is the right call vs. expanding scope to add a GetRoots RPC, (b) whether the new public surface on the CLI lib is appropriate or should be gated behind a #[cfg(any(test, feature = "test-support"))].

Run /pragmatic-code-review to validate.

[malibio]

Code Review: PR #1150 — Re-Review (round 2)

Review Type: Re-Review (second pass)
Diff under review: git diff 6a18b53c..0b3183d7 — 2 files, +200/-47
Verdict: Approve

Prior-Review Status Table

#	Severity	Finding	Status	Verified by
1	🟡	recent_node_ids ordering misleading	✅ Addressed	`all_nodes.sort_by(
2	🟡	directory_size swallows IO errors	✅ Addressed	Signature now fn directory_size(path: &Path, errors: &mut Vec<String>) -> u64; read_dir / dir-entry iter / file_type / metadata all push into errors with file paths; new unit test pins this
3	🟡	Symlink behavior undocumented	✅ Addressed	6-line doc comment at lines 140–150 explains lstat semantics and explicitly warns against "fixing" to fs::metadata
4	🟢	QUERY_LIMIT truncation silent	✅ Addressed	Lines 94–98 push format!("Result truncated at QUERY_LIMIT=...") when all_nodes.len() == QUERY_LIMIT as usize; safe when RPC errored (vector is empty)
5	🟢	Serial RPC calls	⏭️ Skipped (acceptable)	Skip rationale matches reviewer's own framing ("not worth changing unless touching the file")
6	🟢	bind_addr hoist	⏭️ Skipped (acceptable)	Skip rationale matches reviewer's own framing ("not a bug — bind_addr is binary-only")
—	🟡	Coverage gap on collect()	✅ Addressed	diagnostics_collect_reports_counts_and_recency integration test + happy-path unit test for directory_size
—	🐛	New — root_node_count was structurally always equal to total_node_count	Field dropped from report	See finding R1 below

New Findings (Round 2)

🟡 Important

R1. Dropping root_node_count partially violates issue #1139's acceptance criteria — defensible but should be tracked. — packages/cli/src/commands/diagnostics.rs:102–108

Issue #1139 lists "Reports: database path, existence, size, total node count, root node count, recent node IDs" as an acceptance criterion. This PR no longer satisfies that bullet. I spent the most time on this question and I think the call is right, but it should not pass review without an explicit follow-up.

The author's analysis is verifiable and correct:

• node_service.rs:417 hardcodes node_to_proto(n, None, None) — every row from QueryNodesSimple ships parent_id: None
• The previous code (n.parent_id.as_deref().unwrap_or("").is_empty()) was therefore equivalent to true for every row, making root_node_count == total_node_count always
• The original Tauri implementation in commands/diagnostics.rs had the same bug — this is not a regression introduced by the migration, it's a latent bug the migration exposed via the new test
• There is no GetRoots RPC, and GetChildren("") returns empty (no synthetic root)

Three options were available:

1. Drop the field (chosen) — Reports nothing rather than reporting a number that's wrong by construction
2. Add a GetRoots RPC — Correct but blows scope of a "move command from A to B" PR
3. Compute via N+1 GetChildren calls or extend NodeListResponse — Inefficient or scope-creeping

CLAUDE.md's "implement final architecture directly, skip intermediate steps" argues for (2), but it also says "Make breaking changes without hesitation" and "delete deprecated code immediately". Reporting a known-wrong number violates the principle of an honest diagnostic tool more severely than missing one row violates the acceptance criterion. For a developer-debugging surface, omission with an inline comment explaining why is strictly better than a lie. The inline comment at lines 102–108 explicitly names the future fix path, so a GetRoots follow-up is properly seeded.

Action requested: File a follow-up issue ("Add GetRoots RPC and restore root_node_count to nodespace diagnostics") before merging. The author offered to do this; please confirm. Not blocking the merge itself, but this is the bookkeeping that keeps the dropped criterion from getting lost.

🟢 Suggestions

R2. Lexicographic sort on created_at strings is correct by accident of ASCII — worth a one-line comment. — packages/cli/src/commands/diagnostics.rs:114

I dug into this because RFC3339 string-sort is a notorious foot-gun. Verified empirically: chrono's DateTime<Utc>::to_rfc3339() produces output like 2026-05-18T03:00:00+00:00 (no fractional seconds when zero) and 2026-05-18T03:00:00.020+00:00 (variable precision otherwise). Lexicographic ordering works correctly here only because:

1. All node created_at values come from a single serialization path (node_service.rs:1113, only call site for Node.created_at.to_rfc3339())
2. All inputs are DateTime<Utc>, so the suffix is invariably +00:00
3. ASCII collation: + (0x2B) < digit/. (0x2E, 0x30–0x39), so a whole-second timestamp sorts strictly before a sub-second timestamp at the same whole-second mark

If anyone ever migrates this to a different time type or adds a second serialization path with Z instead of +00:00, this sort breaks silently. Suggest a one-liner above line 114:

// String sort is sound only because all created_at values come from
// DateTime<Utc>::to_rfc3339() — same timezone (+00:00) and ASCII
// '+' < digits/'.' makes whole-second values sort before sub-second.

Nit, not blocking. The integration test would catch a regression but only if timestamps happen to straddle a whole-second boundary.

R3. New pub surface (collect, DiagnosticsReport) is consistent with existing pattern — no change needed. — packages/cli/src/commands/diagnostics.rs:30, 58

The author flagged this and asked whether a #[cfg(any(test, feature = "test-support"))] gate is warranted. Comparable: commands::node::run and commands::node::{Get,Create,Update,Delete,Children}Args are all pub for the same integration-test reason. Gating only diagnostics::collect would be inconsistent without a defensible boundary. For a binary crate's command modules — where the "public" surface is exclusively for in-process tests in a sibling crate of the same workspace — pub is the simplest pragma. DiagnosticsReport's fields being pub is fine; nothing downstream constructs one for behavioral effect, only consumes one in assertions.

R4. Integration test's 20ms sleep margin is adequate. — packages/cli/tests/cli_integration.rs:321

The concern: on a slow CI runner, could created_at of two consecutive create_node calls collide and break the sort assertion? Utc::now() resolves to microsecond/nanosecond precision on Linux/macOS; 20ms gives 20,000–20,000,000 ticks of headroom. The only way to collide would be a clock-stepped-backward scenario, which is unrelated to load. Margin is fine. The test does not exercise the truncation-warning or IO-error paths — both would require either a 100k-node fixture or a permissions-stripped tempdir, which are not worth the maintenance overhead for a developer tool. Confirmed.

Architecture & Consistency

• ✅ Stale comment "We compute total count and root count from the same batch" was correctly replaced with "Pull all nodes once (bounded by QUERY_LIMIT) to compute total count and surface the most-recently-created IDs from a single snapshot." Comment now matches behavior.
• ✅ The error-accumulation pattern (&mut Vec<String> parameter) is somewhat unusual; a Result<u64> returning a partial total with an error list might be cleaner, but the current shape works and keeps the call site in collect() linear. Not worth churning.
• ✅ directory_size_handles_missing_path correctly asserts errors.len() == 1. The top-level read_dir failure short-circuits via early return; a partial deep-tree failure would push more than one. The unit test pins the shallow case; the recursive accumulation is exercised by the happy-path directory_size_sums_nested_files test (which asserts zero errors on a real tree).
• ✅ Test tempdir lifetime correctly held by the test scope; daemon shutdown happens before tempdir drop (last let _ = shutdown.send(()) at line 363).

Build & Test Verification

• ✅ cargo test -p nodespace-cli: 5/5 lib + 5/5 integration tests pass locally (verified)
• ✅ Diff is contained to two files (diagnostics.rs, cli_integration.rs) — no scope creep
• ✅ Frontend flaky tests (multi-tab-reactivity, marked-config large-text) are unrelated — grepped, no overlap with diagnostics. Tests are timing-sensitive Svelte reactivity tests and markdown rendering tests, not Rust code paths

Recommendation

Approve. All three 🟡 Important and the truncation 🟢 from round 1 are correctly addressed. The dropped root_node_count is a legitimate engineering judgment call — reporting a structurally-broken number is worse than omitting the field with an inline TODO and an explicit follow-up. The two 🟢 skips match the original reviewer's own framing. The new integration test is the right shape (delta-from-baseline, explicit timestamp gaps, asserts the recency ordering claim it validates), and it caught a real latent bug from the Tauri-era code rather than just being checkbox coverage.

Before merge, please: file the GetRoots follow-up issue so the dropped acceptance criterion is tracked. The PR is mergeable on its own merits; the follow-up is for issue-completeness bookkeeping.

🤖 Generated with Claude Code

[malibio]

Re-review feedback addressed

Pushed 104fa895.

#	Finding	Status	Notes
R1	🟡 File GetRoots follow-up	✅ Filed	#1153 — full RPC shape, daemon wiring, CLI restoration steps, alternatives considered, acceptance criteria.
R2	🟢 RFC3339 lexicographic-sort invariant undocumented	✅ Addressed	Added comment block above all_nodes.sort_by(...) explaining the single-serialization-path + +00:00-suffix invariant and naming the fix path (parse to DateTime) if a second path appears.
R3	🟢 Pub surface concern	⏭️ No change	Reviewer's own answer: consistent with commands::node's existing pattern.
R4	🟢 20ms sleep margin	⏭️ No change	Reviewer's own answer: microsecond clock precision makes the margin fine.

Verified cargo build -p nodespace-cli clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: Only a 7-line doc comment was added in this round; no logic changes, no API surface changes, no test changes. The follow-up issue (#1153) is administrative tracking and does not affect this PR's code. Ready for merge (subject to #1153 being filed, which it is).