[malibio]

Closes #1637

[malibio]

Code Review Summary

This is a well-scoped, well-tested fix for a measured 0%→100% behavioral regression. resolve_query now performs its search internally via a shared run_node_query core and returns a discriminated result (resolved: true node / no_match / multiple_matches) instead of query fragments the model was reliably mis-routing. The run_node_query extraction is a faithful, behavior-preserving refactor of the old exec_search_nodes body — verified by diff inspection and by the full existing search_nodes test suite passing unchanged (10/10). Tool description, skill guidance, and the eval fixture are all updated consistently with the new contract, and the previously-flagged compensating paragraph in agent_guidance.rs is not just gone but actively guarded against regression by an existing test.

The one acceptance criterion that is not actually closed by this PR is "the real scoped inference call is exercised" — all three new regression tests still run through FixedJsonEngine, the same canned-JSON stub used before this PR, so the NL→filter decomposition step itself remains unverified by cargo test; only the downstream search/discrimination logic is exercised. This is a real, acknowledged gap against the issue's explicit motivation, not a nitpick, but I don't think it should block merge — see rationale below.

There is one substantive new-code concern worth fixing before merge: silently dropping individual malformed filters via .ok() in exec_resolve_query is inconsistent with the rigor PR #1823 just established for the same struct, and has a plausible failure mode (silent under-filtering → wrong resolution).

Net assessment: this is a clear net positive that fixes a measured, high-value bug with real regression tests, no scope creep beyond the explicitly-authorized bundled fix, and no architectural or security regressions. Recommend COMMENT (approve-with-strong-suggestions) rather than blocking — the two Important items below are worth addressing but neither is a correctness regression introduced by this PR relative to main.

Findings

Critical Issues

None.

Suggested Improvements

• packages/agent/src/local_agent/tools.rs:1441-1449 — silently dropping malformed decomposition filters undermines Audit all agent-facing parameter structs for silent unknown-field discard #1823's own rigor. filter_map(|f| serde_json::from_value(f.clone()).ok()) drops any filter object that fails to deserialize as AgentFilterItem, with no logging and no signal to the caller. AgentFilterItem on main (merged just before this branch was cut, not yet rebased in) now carries #[serde(deny_unknown_fields)] specifically so a malformed/misspelled key becomes a loud, immediate error rather than a silent drop — PR Audit all agent-facing parameter structs for silent unknown-field discard #1823's entire point was that this exact failure mode ("surfacing only as an unrelated downstream error two layers from the cause") is bad. Here it's reintroduced one layer up: if the decomposition LLM emits 2 filters and one has a bad key, that filter vanishes and the search proceeds on the other one alone — which can silently turn a would-be unique match into a multiple_matches (or worse, a different unique match) with no indication anything went wrong. Recommend at minimum a log::warn! per dropped filter (cheap, no behavior change, restores debuggability), and consider whether a decomposition-parse failure should itself count as a resolution failure (akin to no_match) rather than resolving on a subset of filters. Not blocking because it's a stub decomposition path with a low-cardinality prompt-constrained format, but it deserves the same discipline Audit all agent-facing parameter structs for silent unknown-field discard #1823 just applied everywhere else on this exact struct.

• "Real scoped inference call is exercised" is not actually satisfied — treat as a follow-up, not silently closed. All three new regression tests (resolve_query_regression_dollar_amount_phrasing, _relative_date_phrasing, _paraphrased_identifier_phrasing) construct their executor via executor_with(ns, engine_json), which wires a FixedJsonEngine that ignores the prompt and returns a hardcoded, already-decomposed filter string — the same stub pattern the pre-existing tests used. They verify the downstream search/discrimination logic thoroughly and correctly, but none of them exercise a real model decomposing "Mark the $500 invoice as paid" into {"property":"amount","value":500}. The issue's own acceptance criterion exists precisely because "the measurements above used a stubbed resolver, so its live accuracy is unverified" — that sentence is still true after this PR for the cargo test surface. scripts/eval/fixtures/agent-matrix.ts scenario 6 does exercise a real model end-to-end and is updated correctly, but it's a manually-run eval harness, not part of the automated test surface, and only covers one of the three phrasings (the paraphrased-identifier one — the one that historically was hardest). Given CLAUDE.md's stated "no CI, pre-push gate is the test surface" posture and the precedent this PR itself cites (agent_loop::maybe_summarize_history also isn't covered by a real-model cargo test), I don't think this needs to block merge, but the PR body / issue closure should say explicitly that this criterion is closed by the eval harness for scenario 6 only, not by an automated regression test for all three phrasings — otherwise a future reader will believe live decomposition accuracy for the dollar-amount and relative-date phrasings is verified when it isn't.

Nitpicks

• Nit: packages/agent/src/local_agent/tools.rs:164,1451-1460 — RESOLVE_QUERY_MATCH_LIMIT = 10 truncates before counting, so a query with e.g. 40 true matches returns exactly 10 candidates with no indication the list is truncated. Functionally harmless today (both "10 candidates" and "40 candidates" call for the same model behavior — ask the user to disambiguate), but the payload can't be distinguished from a genuine 10-way tie. If this ever matters, a cheap fix is a boolean like "truncated": matches.len() == RESOLVE_QUERY_MATCH_LIMIT rather than a fetch-with-count-plus-one. Given the const's own doc comment explicitly scopes this to "tell unique from ambiguous from not found, not page through a large result set," leaving this as-is is a reasonable YAGNI call for now.

• Nit: packages/agent/src/local_agent/tools.rs:2899-2905 — the multi-match regression test only asserts candidates.len() == 2; it doesn't assert on the shape of an individual candidate (id/title/properties present). Low risk since run_node_query's summary shape is already covered by search_nodes tests, but a one-line assertion here would directly pin the claim in skill_pipeline.rs's guidance that the model has enough info to ask the user which one they meant.

• Nit: branch is currently behind main by one commit (Audit all agent-facing parameter structs for silent unknown-field discard #1823, Audit all agent-facing parameter structs for silent unknown-field discard) — gh pr view reports mergeState: CLEAN so this resolves automatically on merge, but worth a rebase before merge just to run this PR's tests against the deny_unknown_fields-equipped AgentFilterItem, given the Suggested Improvement above about filter-drop behavior interacting with that exact attribute.

Requirements Validation (issue #1637 acceptance criteria)

• resolve_query performs the search internally and returns id/title/identifying properties — confirmed in exec_resolve_query, verified by test and by reading run_node_query.
• Zero-match and multi-match cases return a discriminated result, not a plan to search again — match matches.len() { 0 => no_match, 1 => resolved, _ => multiple_matches }, covered by dedicated tests.
• The tool description is rewritten for the new contract — def_resolve_query description fully rewritten, explicitly says "does NOT return query arguments" and documents all three outcomes.
• The compensating paragraph at agent_guidance.rs:54 is deleted — confirmed gone, and actively guarded by tool_strategy_rules_contain_no_per_operation_routing (asserts "resolve_query" never reappears in TOOL_STRATEGY_RULES).
• The three request phrasings become regression tests — all three present and passing, but see the Suggested Improvement above: they test the discrimination logic, not live decomposition.
• Existing callers updated in the same change, no dual-return-shape support — skill_pipeline.rs guidance and agent-matrix.ts scenario 6 both updated in this PR; no code path returns the old {query, filters} shape any longer (confirmed by grep and by the explicit "old fragment shape must not leak back" test assertion).
• Partially met: the real scoped inference call is exercised — not satisfied for cargo test (still FixedJsonEngine stub); the eval harness (agent-matrix.ts scenario 6, one of three phrasings) does exercise a real model but isn't part of the automated test surface. See Suggested Improvements.

Code Quality

run_node_query is a clean extraction — same filter-empty/non-empty branching, same ops_error_to_tool calls now correctly parameterized by tool_name so error messages still say "search_nodes" or "resolve_query" as appropriate, same summary-truncation logic. Doc comments on both run_node_query and exec_resolve_query clearly explain the ADR-064 rationale and the sharing relationship between the two tools. No dead code left behind from the old return shape.

Security

No new external input surface. The decomposition-call output remains constrained to a temperature-0, prompt-scoped inference call, same as pre-existing code (unchanged in this PR). The one item worth attention is the filter-swallowing behavior discussed above — not a security issue per se (no injection risk; AgentFilterItem deserialization failure just drops data) but a defensive-correctness gap.

Performance

RESOLVE_QUERY_MATCH_LIMIT = 10 bounds resolve_query's search cost appropriately — same order of magnitude as search_nodes's existing default limit, no new N+1 or unbounded-scan risk introduced.

Testing

13 resolve_query-related tests pass (verified via cargo test -p nodespace-agent resolve_query --lib), 10 search_nodes tests pass unchanged, full nodespace-agent suite (322 tests) passes with no new failures. Good coverage of the discrimination logic (unique/zero/multi-match) and of the three measured phrasings at the search level. Gap: no test for the malformed-filter-drop scenario in exec_resolve_query's defensive deserialization, and no automated test exercises live decomposition (see above).

Documentation

Tool description and skill guidance are both rewritten clearly and consistently with the new contract. RESOLVE_QUERY_MATCH_LIMIT's doc comment correctly scopes its purpose. No stale references to the old {query, filters} shape found anywhere in the diff or in a codebase grep.

Recommendation

COMMENT — approve the direction and implementation; the "real inference call" gap and the filter-swallowing behavior are worth addressing (ideally before merge for the filter-swallowing fix, since it's a small, cheap change) but neither is a regression this PR introduces relative to main, and both are reasonable to explicitly accept as follow-up given the project's pre-release/no-CI stance. This is a clear net positive that closes the measured 0%→100% bug with solid regression coverage at the layer it can practically be tested at.

[malibio]

Code Review Summary

This is a well-scoped, well-tested fix for a measured 0%→100% behavioral regression. resolve_query now performs its search internally via a shared run_node_query core and returns a discriminated result (resolved: true node / no_match / multiple_matches) instead of query fragments the model was reliably mis-routing. The run_node_query extraction is a faithful, behavior-preserving refactor of the old exec_search_nodes body — verified by diff inspection and by the full existing search_nodes test suite passing unchanged (10/10). Tool description, skill guidance, and the eval fixture are all updated consistently with the new contract, and the previously-flagged compensating paragraph in agent_guidance.rs is not just gone but actively guarded against regression by an existing test.

The one acceptance criterion that is not actually closed by this PR is "the real scoped inference call is exercised" — all three new regression tests still run through FixedJsonEngine, the same canned-JSON stub used before this PR, so the NL→filter decomposition step itself remains unverified by cargo test; only the downstream search/discrimination logic is exercised. This is a real, acknowledged gap against the issue's explicit motivation, not a nitpick, but I don't think it should block merge — see rationale below.

There is one substantive new-code concern worth fixing before merge: silently dropping individual malformed filters via .ok() in exec_resolve_query is inconsistent with the rigor PR #1823 just established for the same struct, and has a plausible failure mode (silent under-filtering → wrong resolution).

Net assessment: this is a clear net positive that fixes a measured, high-value bug with real regression tests, no scope creep beyond the explicitly-authorized bundled fix, and no architectural or security regressions. Recommend COMMENT (approve-with-strong-suggestions) rather than blocking — the two Important items below are worth addressing but neither is a correctness regression introduced by this PR relative to main.

Findings

Critical Issues

None.

Suggested Improvements

• packages/agent/src/local_agent/tools.rs:1441-1449 — silently dropping malformed decomposition filters undermines #1823's own rigor. filter_map(|f| serde_json::from_value(f.clone()).ok()) drops any filter object that fails to deserialize as AgentFilterItem, with no logging and no signal to the caller. AgentFilterItem on main (merged just before this branch was cut, not yet rebased in) now carries #[serde(deny_unknown_fields)] specifically so a malformed/misspelled key becomes a loud, immediate error rather than a silent drop — PR #1823's entire point was that this exact failure mode ("surfacing only as an unrelated downstream error two layers from the cause") is bad. Here it's reintroduced one layer up: if the decomposition LLM emits 2 filters and one has a bad key, that filter vanishes and the search proceeds on the other one alone — which can silently turn a would-be unique match into a multiple_matches (or worse, a different unique match) with no indication anything went wrong. Recommend at minimum a log::warn! per dropped filter (cheap, no behavior change, restores debuggability), and consider whether a decomposition-parse failure should itself count as a resolution failure (akin to no_match) rather than resolving on a subset of filters. Not blocking because it's a stub decomposition path with a low-cardinality prompt-constrained format, but it deserves the same discipline #1823 just applied everywhere else on this exact struct.

• "Real scoped inference call is exercised" is not actually satisfied — treat as a follow-up, not silently closed. All three new regression tests (resolve_query_regression_dollar_amount_phrasing, _relative_date_phrasing, _paraphrased_identifier_phrasing) construct their executor via executor_with(ns, engine_json), which wires a FixedJsonEngine that ignores the prompt and returns a hardcoded, already-decomposed filter string — the same stub pattern the pre-existing tests used. They verify the downstream search/discrimination logic thoroughly and correctly, but none of them exercise a real model decomposing "Mark the $500 invoice as paid" into {"property":"amount","value":500}. The issue's own acceptance criterion exists precisely because "the measurements above used a stubbed resolver, so its live accuracy is unverified" — that sentence is still true after this PR for the cargo test surface. scripts/eval/fixtures/agent-matrix.ts scenario 6 does exercise a real model end-to-end and is updated correctly, but it's a manually-run eval harness, not part of the automated test surface, and only covers one of the three phrasings (the paraphrased-identifier one — the one that historically was hardest). Given CLAUDE.md's stated "no CI, pre-push gate is the test surface" posture and the precedent this PR itself cites (agent_loop::maybe_summarize_history also isn't covered by a real-model cargo test), I don't think this needs to block merge, but the PR body / issue closure should say explicitly that this criterion is closed by the eval harness for scenario 6 only, not by an automated regression test for all three phrasings — otherwise a future reader will believe live decomposition accuracy for the dollar-amount and relative-date phrasings is verified when it isn't.

Nitpicks

• Nit: packages/agent/src/local_agent/tools.rs:164,1451-1460 — RESOLVE_QUERY_MATCH_LIMIT = 10 truncates before counting, so a query with e.g. 40 true matches returns exactly 10 candidates with no indication the list is truncated. Functionally harmless today (both "10 candidates" and "40 candidates" call for the same model behavior — ask the user to disambiguate), but the payload can't be distinguished from a genuine 10-way tie. If this ever matters, a cheap fix is a boolean like "truncated": matches.len() == RESOLVE_QUERY_MATCH_LIMIT rather than a fetch-with-count-plus-one. Given the const's own doc comment explicitly scopes this to "tell unique from ambiguous from not found, not page through a large result set," leaving this as-is is a reasonable YAGNI call for now.

• Nit: packages/agent/src/local_agent/tools.rs:2899-2905 — the multi-match regression test only asserts candidates.len() == 2; it doesn't assert on the shape of an individual candidate (id/title/properties present). Low risk since run_node_query's summary shape is already covered by search_nodes tests, but a one-line assertion here would directly pin the claim in skill_pipeline.rs's guidance that the model has enough info to ask the user which one they meant.

• Nit: branch is currently behind main by one commit (#1823, Audit all agent-facing parameter structs for silent unknown-field discard) — gh pr view reports mergeState: CLEAN so this resolves automatically on merge, but worth a rebase before merge just to run this PR's tests against the deny_unknown_fields-equipped AgentFilterItem, given the Suggested Improvement above about filter-drop behavior interacting with that exact attribute.

Requirements Validation (issue #1637 acceptance criteria)

• resolve_query performs the search internally and returns id/title/identifying properties — confirmed in exec_resolve_query, verified by test and by reading run_node_query.
• Zero-match and multi-match cases return a discriminated result, not a plan to search again — match matches.len() { 0 => no_match, 1 => resolved, _ => multiple_matches }, covered by dedicated tests.
• The tool description is rewritten for the new contract — def_resolve_query description fully rewritten, explicitly says "does NOT return query arguments" and documents all three outcomes.
• The compensating paragraph at agent_guidance.rs:54 is deleted — confirmed gone, and actively guarded by tool_strategy_rules_contain_no_per_operation_routing (asserts "resolve_query" never reappears in TOOL_STRATEGY_RULES).
• The three request phrasings become regression tests — all three present and passing, but see the Suggested Improvement above: they test the discrimination logic, not live decomposition.
• Existing callers updated in the same change, no dual-return-shape support — skill_pipeline.rs guidance and agent-matrix.ts scenario 6 both updated in this PR; no code path returns the old {query, filters} shape any longer (confirmed by grep and by the explicit "old fragment shape must not leak back" test assertion).
• Partially met: the real scoped inference call is exercised — not satisfied for cargo test (still FixedJsonEngine stub); the eval harness (agent-matrix.ts scenario 6, one of three phrasings) does exercise a real model but isn't part of the automated test surface. See Suggested Improvements.

Code Quality

run_node_query is a clean extraction — same filter-empty/non-empty branching, same ops_error_to_tool calls now correctly parameterized by tool_name so error messages still say "search_nodes" or "resolve_query" as appropriate, same summary-truncation logic. Doc comments on both run_node_query and exec_resolve_query clearly explain the ADR-064 rationale and the sharing relationship between the two tools. No dead code left behind from the old return shape.

Security

No new external input surface. The decomposition-call output remains constrained to a temperature-0, prompt-scoped inference call, same as pre-existing code (unchanged in this PR). The one item worth attention is the filter-swallowing behavior discussed above — not a security issue per se (no injection risk; AgentFilterItem deserialization failure just drops data) but a defensive-correctness gap.

Performance

RESOLVE_QUERY_MATCH_LIMIT = 10 bounds resolve_query's search cost appropriately — same order of magnitude as search_nodes's existing default limit, no new N+1 or unbounded-scan risk introduced.

Testing

13 resolve_query-related tests pass (verified via cargo test -p nodespace-agent resolve_query --lib), 10 search_nodes tests pass unchanged, full nodespace-agent suite (322 tests) passes with no new failures. Good coverage of the discrimination logic (unique/zero/multi-match) and of the three measured phrasings at the search level. Gap: no test for the malformed-filter-drop scenario in exec_resolve_query's defensive deserialization, and no automated test exercises live decomposition (see above).

Documentation

Tool description and skill guidance are both rewritten clearly and consistently with the new contract. RESOLVE_QUERY_MATCH_LIMIT's doc comment correctly scopes its purpose. No stale references to the old {query, filters} shape found anywhere in the diff or in a codebase grep.

Recommendation

COMMENT — approve the direction and implementation; the "real inference call" gap and the filter-swallowing behavior are worth addressing (ideally before merge for the filter-swallowing fix, since it's a small, cheap change) but neither is a regression this PR introduces relative to main, and both are reasonable to explicitly accept as follow-up given the project's pre-release/no-CI stance. This is a clear net positive that closes the measured 0%→100% bug with solid regression coverage at the layer it can practically be tested at.

[malibio]

Address Review

Addressed both suggested improvements from the initial review:

1. Silent filter-dropping (fixed)
packages/agent/src/local_agent/tools.rs — malformed filters from the decomposition step were silently discarded via .ok(). Now each dropped filter is logged with tracing::warn! (name, error, and the raw JSON), matching the deny_unknown_fields rigor #1823 established for AgentFilterItem elsewhere. Added resolve_query_drops_only_the_malformed_filter_not_the_whole_resolution, which verifies one malformed filter (unknown key) is dropped while a valid sibling filter still uniquely resolves the target node.

2. "Real scoped inference call exercised" (documented, not closed)
This acceptance criterion is not actually satisfied by this PR — all resolve_query integration tests use FixedJsonEngine, a stub that ignores the prompt. Closing it properly means standing up real-model test infrastructure (model download/load, machine-load-sensitive timing) matching ai_chat_send_to_idle_test.rs's pattern, which is out of scope for this fix. Rather than leave that gap implicit, I added an explicit doc comment on the resolve_query_integration test module stating what is and isn't covered, and pointing at scripts/eval/fixtures/agent-matrix.ts scenario 6 as the one place a real model exercises this path today (one phrasing, via the eval harness, not cargo test).

Also: rebased onto main to pick up #1823 (deny_unknown_fields on AgentFilterItem) before making the filter-drop fix, since the fix's rationale depends on that struct's current behavior.

Verification: cargo test --lib --bins -p nodespace-agent (333 lib + 3 bin tests, matches the actual pre-push gate command) and cargo clippy --all-targets -p nodespace-agent -- -D warnings -D clippy::unused-async both clean. Full pre-push gate (test:all + cargo build --bin nodespaced + full Tauri-seam e2e including the real-model ai-chat test) passed on push.

Commit: 8ba6a40