Polish the CLI and skill-installer reporting found in release validation

[malibio]

Closes #3570.

Nine small reporting fixes from the 2026-10-05 headless validation of main.

What changed

• Refused databases. nodespace diagnostics lists what a refused database needs after its path, and its --json entries carry unsupported_extensions and refusal, as database list does. database use still accepts a refused database and now prints a Warning: line on stderr.
• Skill installer.
  • skill status names a harness that is present without the skill (detected, skill not installed) instead of printing "No agent harnesses detected".
  • A re-run of skill install over a current install rewrites nothing and reports skill already up to date. A file that differs is still updated and reported as installed.
  • skill uninstall removes the skills/ directory install created when it is left empty. The harness's own directory is never touched.
• Logs. With NODESPACE_HOME set, nodespace logs resolves only to the log under that home.
• Error text. A failed command prints the gRPC code and message (NotFound: …) without the MetadataMap{…} dump. Diagnostics' per-query errors use the same form.
• Grammar. Type names take the right article ("an ai-chat-message") in structural-rule and type-mismatch messages.
• Refused root create. The refusal says "a new cannot be a root" and names no id.
• skill guidance confidence is held to 0.0–1.0 at the wire. The underlying score keeps its density boost for ranking.
• nodespaced --help / -h prints usage and exits 0. An unknown argument is still refused with status 2, now followed by the usage.
• Empty-query search.
  • search "" --collection <path> lists the collection's members. They are fetched by id; before, they were picked out of a capped slice of the whole table and usually weren't in it.
  • A type-only listing is newest first and leaves out nodes under a system-type root, so search "" --type text no longer leads with skill and guidance paragraphs.

Behaviour changes worth a look

• Empty-query listings were in storage order and are now most recently modified first.
• The listing's system-fragment exclusion resolves each kept candidate's root with point lookups, cached per root, scanning at most 1000 rows per type. A workspace whose newest 1000 nodes of a type are all system fragments would still list nothing for that type.
• The confidence cap means several strong matches can all show 1.0; order still ranks them. cli.md says so.

Tests

bun run test:changed passes (Rust workspace, skill package, scripts; frontend not reached by the diff). New tests cover each bullet; the two listing tests were checked to fail with the fix disabled.

Docs

packages/skill/references/cli.md updated where output changed. No architecture doc describes these outputs, so nodespace-docs is unchanged.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3585

Review Type: Initial Review (no prior reviews or comments on the PR)

Scope reviewed: the single commit e6415471 (26 files, +936/-134) against origin/main with the three-dot diff. origin/main has moved two commits past the branch base (#3575, #3576); git merge-tree reports a clean merge. #3576 touches search_ops.rs / matches_scope too, with no textual conflict. I read the code and did not run the test suites (no repository edits were allowed, so nothing below was confirmed by a new test).

Summary

A net improvement, and every acceptance criterion is met. The CLI, installer, daemon and error-text changes are small, well tested and consistent with the surrounding code. The one area with substance is enumerate_nodes: the collection fix and newest-first order are right, but the new "under a system-type root" exclusion is broader than the bullet it fixes and regresses one explicit listing. Nothing is a merge blocker.

Requirements Check (issue #3570)

#	Criterion	Status	Notes
1a	diagnostics shows the refusal and unsupported_extensions	✅	Human output appends the requirement note; --json carries unsupported_extensions and refusal. Shares refusal() / requirement_note() with database list.
1b	database use warns on a refused database	✅	Warning: on stderr, so --json stdout stays parseable. Tested both ways.
2a	skill status after uninstall	✅	detected, skill not installed. See finding 3 for the plugin-managed case.
2b	Re-install rewrites nothing and says so	✅	writeIfDifferent plus changed; the mtime test pins "rewrote nothing". Help text corrected.
2c	Uninstall removes the empty skills/ dir	✅	Bounded at the harness directory, skips symlinks, keeps a non-empty skills/.
3	logs --path-only honours NODESPACE_HOME	✅	It becomes the only candidate. Matches the daemon's nodespace_home().
4	No MetadataMap{…} dump	✅	render_status for command errors, status_text for diagnostics' per-query errors.
5	Grammar "a ai-chat-message"	✅	with_indefinite_article at 13 call sites. One site missed (nit).
6	Refused root create names no id	✅	"a new cannot be a root".
7	confidence above 1.0	✅	Clamped at the wire; ranking keeps the raw score.
8	nodespaced --help	✅	Usage on stdout, exit 0, no socket bound; unknown args still exit 2.
9a	search "" --collection	✅	Members fetched by id.
9b	search "" --type text leads with guidance fragments	⚠️	Fixed for the reported case, but the exclusion also hides types asked for by name (finding 1).

Documentation: packages/skill/references/cli.md is updated for each output change. No node type, field or relationship changes, so the node-type sequence does not apply. No Pro-boundary, lint-suppression, compatibility-code or issue-number-in-comment violations found.

Code Review Findings

Critical Issues

None.

Suggested Improvements

1. 🟡 Important: the system-root exclusion also removes types the caller named
📁 packages/core/src/ops/search_ops.rs:756-764 and sits_under_system_root (:377-407)

EnumerateScope {
    member_ids: collection_member_ids.as_ref(),
    knowledge_only: matches!(scope, SearchScope::Knowledge),
},

knowledge_only follows the default scope even when the caller names types, while should_skip_scope_filter (same file, :58) says a named type replaces the default scope "for a query and for an enumerate alike". Two consequences, both read from the code:

• search "" --type ai-chat-message now returns nothing. Every message sits under an ai-chat-native root, which is a core type outside KNOWLEDGE_CORE_TYPES, so every message is dropped. Before this PR the listing returned them. The same holds for any type that only exists under a system root.
• "System type" is defined as "core type not in KNOWLEDGE_CORE_TYPES". That list also omits quote-block, ordered-list, checkbox, horizontal-line, query and collection, so a user's text under a root of one of those types is treated as a system fragment. I did not check how often such roots have children in practice.

Recommendation: drop a candidate only when its own type is a Knowledge type and its root is not (a text or header under a skill is the ambiguous case the issue describes; an ai-chat-message was asked for by name). Add a test that lists ai-chat-message by type. If hiding messages is intended, say so in cli.md and in the should_skip_scope_filter doc comment.
Principle: least surprise; one rule for "explicit type beats default scope" rather than two that disagree.

2. 🟡 Important: the paging loop is not exercised by any test
📁 packages/core/tests/it/embedding_service_test.rs:2423-2458

// More fragments than a page holds, all newer than the user's nodes.
for i in 0..70 {

With the default limit of 20, effective_limit is 60 and page_size is min(1000, 60 * 3) = 180. Seventy fragments fit in the first page, so the second iteration (with_offset(scanned)), the fetched < page_size exit and the scanned >= ENUMERATE_FETCH_CAP exit never run. The test does pin the exclusion itself, and would fail without it, but an off-by-one in the offset would pass. The comment is also inaccurate.
Recommendation: set input.limit = Some(1) (page size 9) in that test, or seed more than 180 fragments, so the user's two nodes are only reachable through later pages.
Principle: a test should fail when the code it names is broken.

3. 🟡 Important: skill status says "skill not installed" for a plugin-managed Claude Code
📁 packages/cli/src/commands/skill.rs:851-880

status reports an agent as present only when SKILL.md is in installDir; detect reports the harness directory. When Claude Code has the skill through its plugin marketplace, install deliberately writes nothing (installer.ts:169-181) and prints "already installed via the Claude Code plugin marketplace, not overwriting". skill status now prints claude-code: detected, skill not installed for that same machine, which contradicts install and invites a pointless re-run.
Recommendation: have the installer's status (or detect) line carry the plugin-managed state and print it, or word the CLI line so it does not assert absence ("detected, no skill files installed by NodeSpace"). Doing it in the installer would also remove the second process spawn.
Principle: two commands describing the same state should agree.

4. 🟢 Suggestion: the root cache does not save the walk it is described as saving
📁 packages/core/src/ops/search_ops.rs:297-299, 383-393

// Whether each root met so far is a system type, so the nodes of one
// subtree cost one lookup between them.

get_root_id runs for every candidate before the cache is consulted, and it issues one get_parent_id query per level (hierarchy.rs:241-264). The cache only saves the final get_node_type. Worst case is about 1000 scanned rows times tree depth in point queries per named type. That is acceptable for a local SQLite listing, so this is not a blocker. Either memoise by parent id (siblings share one), or correct the comment.

5. 🟢 Suggestion: system roots still consume the listing budget when no type is named
📁 packages/core/src/ops/search_ops.rs:331-345

The doc comment says the two narrowings happen "before limit counts anything, so neither can leave a page short". A system-type root (skill, tool, play, ai-chat) is not under a system root, so it is kept, counted toward kept, and then removed by the matches_scope post-filter. search "" with no type can still come back short when the newest 60 nodes are system roots. This predates the PR; the loop is the natural place to fix it (apply the type's scope check there when the scope filter is not skipped), and #3576 adds core schemas to the set dropped afterwards.

6. 🟢 Suggestion: stale doc comments after passing None on create
📁 packages/core/src/db/sqlite_store/mod.rs:147-149, 161-162

TreeInvariantViolation.node_id is documented as "None only for a collection refused before it was given an id". It is now None for every refused root create, including one whose caller supplied an id. No consumer breaks: the daemon serialises it as nullable (node_service.rs:3372), the frontend guard accepts null (errors.ts:293), and the only frontend use is the merge path. Update the two comments.

Nitpicks

• Nit: packages/core/src/playbook/validation.rs:1168 still formats "the content of a {} node" with a type name, so it can print "a ai-chat…".
• Nit: packages/core/src/utils/mod.rs:14: a custom type named user-story, unit or one-pager now gets "an", where "a" was right before. The limitation is documented; a short prefix check (uni, use, one) would cover the common cases.
• Nit: packages/skill/src/installer.ts:449-451: "Install made every directory between…" is an assumption, not something the install record tracks. An empty skills/ the user or harness created is removed too. Harmless, but the comment should say "empty", not "ours".
• Nit: packages/daemon/src/main.rs:459-466: a help flag anywhere wins, so nodespaced --bogus --help exits 0. Fine if intended.
• Nit: packages/cli/src/commands/skill.rs:1119-1133: partitioning lines and then re-parsing each for the agent name reads harder than one pass that returns (agent, is_up_to_date).
• Nit: search_semantic already loads every member as a full Node to build member_ids, and enumerate_nodes then fetches them again by id.

Checks on the areas named in the request

• render_status: no caller loses anything. The only metadata reader (commands/node.rs:374, x-version-conflict) runs inside the command, before run maps the error. main only prints. Running after render_refusal is right: the refusal replaces the whole error, so no status is left to rewrite. A status's source() is dropped; its message already carries the text.
• with_indefinite_article in model-facing text: output changes only for vowel-initial types. I found no golden or snapshot containing the old strings.
• wire_confidence: the wire is the right place. skill_ops ranks and computes top_score on the raw score, and nothing downstream of the gRPC entry compares it to a threshold.
• Installer: writeIfDifferent writes through a symlinked file as copyFileSync did; no shipped file has an executable bit to lose. The prune cannot leave the harness directory (the loop stops at detectionDir, resolve() normalises $CLAUDE_CONFIG_DIR) and skips symlinked directories.
• Desktop parser (skill_setup.rs): unaffected. The up-to-date line keeps the ✓ marker, which is all it reads.
• with_ids: the store chunks the id list and sorts in memory, so a large collection does not hit the bind-variable limit.
• Listing order: I found no caller of search_semantic that depends on storage order, but I did not trace the frontend's use of the Tauri command end to end.

Recommendation

APPROVE. Findings 1 to 3 are worth folding into this PR; none blocks the merge.

[malibio]

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

[malibio]

Review addressed (commit 1511bfd)

Addressed

1. Exclusion hid types named outright. A listing now judges only a knowledge-type node by its root, so a named system type is listed wherever it sits. New test lists a quote-block inside a skill, and fails with the guard removed. The reviewer's specific example, --type ai-chat-message, was already empty before this PR: governance keeps messages out of default queries. Roots of the other non-Knowledge core types (quote-block, ordered-list, …) still hide knowledge-type children, which matches what a search with a query does for the same roots.
2. Paging loop untested. The fragments test now uses a limit of 2 (pages of 18 rows), so the user's nodes are reached only past several pages of fragments.
3. skill status contradicted install for a marketplace install. The installer's status prints a ⚠ claude-code: installed via the Claude Code plugin marketplace line and the CLI reports it as installed that way. The desktop app reads only ✓ lines, so it is unaffected.
4. Root cache comment corrected: it saves the root's type read, not the parent walk.
5. Doc claim about short pages corrected to say the scope's type check still runs afterwards.
6. Stale node_id comment corrected.

• Nit: the pruned skills/ comment now says it removes an empty directory whoever made it.

Skipped

• Finding 5's fix (system roots consuming an untyped listing's budget): pre-existing and outside the issue's bullets; documented in the function comment.
• Memoising the parent walk: bounded at 1000 rows per type on a local store; not worth the code yet.
• validation.rs:1168: the type names there are quoted ('task' or 'checkbox'), so the article never meets a vowel.
• Article exceptions (user-…, unit): the limitation is documented on the helper.
• --bogus --help exiting 0, the parser's two-pass shape, and the double member fetch: no behaviour problem.

bun run test:changed passes on the new commit.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The review verdict was APPROVE with no blocking findings. The follow-up is one added condition in the listing filter and one new installer status line, each pinned by a new test, plus comment corrections.