[mstomar125]

Closes #2107

Which approach, and why

Chose a minimal, dependency-free diagnostic hook (issue's approach 2) — specifically, SchemaNode::from_node now delegates its fields extraction to a new private parse_fields helper that returns (Vec<SchemaField>, Option<String>): the same (possibly empty) field list as before, plus a ready-to-print diagnostic message when the parse genuinely failed. from_node prints that message via eprintln! before continuing.

from_node has three production callers:

• node_to_typed_value, reachable from every entry point (Tauri commands, MCP, HTTP) via nodes_to_typed_values, which .collect()s a Vec<Result<_, _>> into a single Result<Vec<_>, _>.
• Two direct call sites in packages/desktop-app/src-tauri/src/commands/schemas.rs (get_all_schemas/get_schema_definition) that call SchemaNode::from_node directly and do not go through nodes_to_typed_values.

Fixing the diagnostic inside from_node itself (rather than at any one caller) covers all three uniformly, without touching any of their signatures.

Ruled out the other two:

• Approach 1 (thread the error through Result) — rejected, but only for the path that actually carries the risk the issue flagged: nodes_to_typed_values is called with ? from ~10 batch-read call sites across packages/core/src/ops/{query_ops,node_ops}.rs, packages/desktop-app/src-tauri/src/commands/{nodes,collections}.rs, and the daemon. Propagating the fields-parse failure as from_node's own Err would make one schema node with stale/malformed field JSON fail an entire unrelated batch read (a task list, a collection view, a generic list_nodes query that happens to include one broken schema) on that path. Making that safe would mean changing nodes_to_typed_values's batch-collect semantics (partition into successes + individually-reported failures) — a much larger, riskier change touching every batch-read call site in three crates, for a bug whose actual fix doesn't require it. (The two direct schemas.rs callers don't share this risk — get_all_schemas already filter_maps out bad schemas today, and get_schema_definition operates on a single schema — but the fix needed to be uniform across all three callers anyway, so this doesn't change the choice.)
• Approach 3 (downstream guard, don't touch nodespace-types) — rejected. A debug-assertion or startup healthcheck would only ever catch this in dev/CI or at daemon boot, not for a schema written or corrupted after boot. The whole point of the bug report is that this fires "silently, everywhere" on a normal read — a healthcheck doesn't close that gap for a schema that goes bad mid-session; it just adds a separate detector that lags behind the read path itself.

Approach 2's callback/closure variant was also considered (from_node(node, on_error: impl FnMut(&str))) but rejected in favor of the current shape: threading a callback through would require touching all three call sites above and would ripple into node_to_typed_value's own signature (used from dozens of places), for no benefit over an unconditional eprintln! — the failure is rare (a genuine parse error, not a hot path) and eprintln! requires zero new dependency and zero signature changes anywhere.

Known limitation, tracked as a follow-up, not fixed here: the "fires the same way no matter which binary embeds this crate" property doesn't fully hold on Windows — the daemon's child process there is spawned with stderr piped to null, and a release desktop-app build has no console, so this diagnostic is currently invisible on a Windows production build (macOS/Linux both redirect the daemon's stderr to a log file). Filed as #2129, out of scope for this PR (fixing it is a daemon/app stdio-plumbing change, not a nodespace-types change).

What changed

• packages/nodespace-types/src/schema.rs: extracted the fields JSON parse into a new private parse_fields(properties, node_id) -> (Vec<SchemaField>, Option<String>). On a genuine parse failure it still returns an empty Vec (unchanged behavior — no new error path, no signature change on from_node), but now also returns a diagnostic message naming the schema node id and the underlying serde_json error. from_node prints that message via eprintln!.
• packages/nodespace-types/src/convert.rs: added a regression test (schema_with_malformed_fields_does_not_fail_an_unrelated_batch_read) pinning that a malformed schema node in a nodes_to_typed_values batch does not fail the other, unrelated nodes in the same batch — the invariant approach 1 would have put at risk, explicitly locked down even though this PR didn't touch batch semantics.

No dependency was added to packages/nodespace-types/Cargo.toml — the crate's minimal-dependency-footprint design goal (its own lib.rs doc comment: "Intentionally minimal: serde, serde_json, chrono, uuid, thiserror. No database, HTTP, or NLP dependencies.") is unchanged.

The relationships extraction two lines below fields in the same function has the identical silent-swallow shape and was left untouched (out of scope for #2107, which named only fields) — filed as #2130.

Testing

• New unit tests in packages/nodespace-types/src/schema.rs:
  • test_parse_fields_malformed_json_surfaces_diagnostic — malformed fields JSON now produces a diagnostic naming the schema id and the property.
  • test_parse_fields_valid_json_no_diagnostic / test_parse_fields_absent_key_no_diagnostic — no false-positive diagnostic on a clean parse or an absent key.
  • test_from_node_malformed_fields_still_succeeds_with_empty_fields — end-to-end through from_node: still Ok with an empty fields Vec (unchanged), now with the diagnostic printed along the way. Verified manually with cargo test -p nodespace-types test_from_node_malformed_fields_still_succeeds_with_empty_fields -- --nocapture, which prints:

    nodespace-types: SchemaNode::from_node: failed to parse `fields` for schema node `<id>`: invalid type: integer `42`, expected a string — reading back as an empty field list. Likely a stale/corrupted storage format.
    

• New regression test in packages/nodespace-types/src/convert.rs: schema_with_malformed_fields_does_not_fail_an_unrelated_batch_read — proves a bad schema node doesn't zero out an unrelated batch (see above).
• cargo test -p nodespace-types: 50 passed, 0 failed.
• cargo test -p nodespace-core --lib: 1154 passed, 0 failed.
• cargo test --workspace --lib --bins: all green (full workspace, including daemon/cli/nlp-engine/agent).
• cargo clippy -p nodespace-types -p nodespace-core -p nodespace-daemon -p nodespace-cli -p nodespace-app --all-targets -- -D warnings: clean.
• cargo fmt -p nodespace-types -- --check: clean.

Acceptance criteria

• A parse failure in nodespace_types::SchemaNode::from_node's fields extraction is now surfaced via eprintln! (node id + underlying serde_json error) instead of producing an empty Vec with zero signal.
• Approach 1's batch-collect blast radius was evaluated and explicitly rejected (see above) rather than reflexively threaded through; a regression test pins the batch-safety property that would have been at risk.
• No dependency added to nodespace-types — the crate's minimal-dependency design goal is unchanged.

Review

An adversarial review sub-agent caught two real issues, both addressed:

1. The original doc comment/PR text claimed node_to_typed_value was the "sole caller" of from_node — false, per the two direct schemas.rs call sites above. Corrected in both the code comment and this description; the fix already covered all three callers (it lives inside from_node itself), so this was a documentation-accuracy fix, not a behavior change.
2. The Windows stderr-visibility gap described above was not originally acknowledged. Now called out explicitly and tracked as eprintln! diagnostics are silently discarded on Windows (daemon stderr piped to null, desktop app has no console) #2129.

A third (nit) finding — a since-removed citation to an unrelated #[cfg(test)] eprintln! as a "no-logging-dependency precedent" — has been dropped from this description as inaccurate framing; it wasn't load-bearing for the approach choice.

[mstomar125]

Spawned an adversarial review sub-agent against this PR. Verdict was "request changes" — two real findings, both addressed in the follow-up commit (8d988f98):

1. The doc comment/description claimed node_to_typed_value was the "sole caller" of SchemaNode::from_node — false. packages/desktop-app/src-tauri/src/commands/schemas.rs has two direct call sites (get_all_schemas, get_schema_definition) that bypass node_to_typed_value entirely. The fix already covered all three callers (it lives inside from_node itself), so this was a documentation-accuracy issue, not a functional gap — but the batch-blast-radius argument for rejecting Approach 1 only actually applies to the node_to_typed_value → nodes_to_typed_values path, not universally. Corrected the doc comment in schema.rs and the PR description to name all three callers and scope the risk accurately.

2. The eprintln!'s "fires the same way no matter which binary embeds this crate" claim doesn't hold on Windows — the daemon's child process is spawned with stderr piped to null there, and a release desktop-app build has no console, so this diagnostic is currently invisible on a Windows production build (macOS/Linux both redirect the daemon's stderr to a log file). Not a regression relative to before this PR (same visibility as the original bug on that platform), but the PR's own rationale overclaimed uniformity. Now called out explicitly in the code comment and PR description, and filed as eprintln! diagnostics are silently discarded on Windows (daemon stderr piped to null, desktop app has no console) #2129 (out of scope for this PR — it's a daemon/app stdio-plumbing fix, not a nodespace-types change).

Also filed #2130 for the sibling relationships extraction in the same function, which has the identical silent-swallow shape and was correctly left out of scope for #2107 (which named only fields).

No functional regression found; review confirmed parse_fields correctly distinguishes absent-key (silent, correct) from genuine parse failure (diagnostic), the batch-safety regression test is sound, and scope is contained to the two nodespace-types files touched.

Re-ran cargo test -p nodespace-types, cargo clippy -p nodespace-types --all-targets -- -D warnings, and cargo fmt -p nodespace-types -- --check after the fixup commit — all clean. Full pre-push gate (bun run test:all + cargo build sidecars + e2e) also passed clean on push.

[mstomar125]

APPROVE

Independently re-verified after addressing the adversarial review's findings:

• cargo test -p nodespace-types: 50/50 pass, including the 3 new unit tests on parse_fields and the end-to-end from_node regression test.
• cargo clippy -p nodespace-types --all-targets -- -D warnings: clean.
• cargo fmt -p nodespace-types -- --check: clean.
• Full pre-push gate (bun run test:all — frontend + skill + Rust workspace, plus cargo build sidecars and e2e Tauri-seam tests) passed clean on push of the fixup commit.
• gh pr diff confirms scope is contained to packages/nodespace-types/src/schema.rs and packages/nodespace-types/src/convert.rs only — no dependency added, no other package touched.

Approach chosen: a minimal, dependency-free diagnostic hook (issue's approach 2) — SchemaNode::from_node's fields extraction now goes through a parse_fields helper that surfaces a diagnostic via eprintln! on a genuine parse failure, while still defaulting to an empty Vec (unchanged behavior) so no caller's Result shape changes. Approach 1 (thread the error through Result) was evaluated and rejected specifically because node_to_typed_value feeds nodes_to_typed_values's Vec<Result<_,_>> -> Result<Vec<_>,_> collect across ~10 batch-read call sites -- turning this into an Err would fail an entire unrelated batch over one bad schema node, exactly the risk the issue flagged. A regression test (schema_with_malformed_fields_does_not_fail_an_unrelated_batch_read) pins that this batch-safety property holds. Approach 3 (downstream-only guard) was rejected as it wouldn't catch a schema that goes bad mid-session, only at boot/CI.

Merging (self-approval authorized per repo convention; GitHub blocks a formal self-review on the author's own PR).