[malibio]

Closes #3265.

What was wrong

A question about the user's own knowledge failed at four layers, traced on the locked model (Gemma 4 E4B):

• Stage 1 asked what the user meant. 4 of 7 such questions came back as route_clarify ("Are you asking about the concept of 'merge gate' … or a physical object?"); the rest kept only the topic.
• Retrieval missed the search skill. A bare topic embeds nearest whichever skill shares a noun with it: "find nodes in nodespace" ranked Organization, Node Creation, Node Deletion, and Research & Search wasn't retrieved.
• Stage 2 answered from nothing, or asked the user for context, with no tool call.
• search_nodes returned the chat itself, titled from the user's question.

What changed

• route_lookup, a fourth Stage 1 choice, for a question or a request to find something. It carries the topic; the system retrieves skills with search stored knowledge for <topic>. Research & Search's description carries that phrase and the retrieval verbs a user says.
• A lookup is always searched. If Stage 2 ends a lookup turn without a tool call, the system runs search_semantic on the topic (three results in full) and the model answers from the results. A prose re-prompt was measured first and changed nothing: the model repeated "I do not have a tool or information regarding that".
• A read-only skill that doesn't lead no longer costs a write skill its place among Stage 2's candidates (routing::select_candidates, at most one extra). The search skill's broader vocabulary had pushed Schema Creation to fourth on "keep track of decisions behind each feature".
• Stage 2's prompt names the registry's skills, so "which skills do you have?" is answered from them. Stage 1 routes questions about the agent as a query, not a lookup.
• search_nodes never returns a conversation (resolved through the type registry, subtypes included), and search_semantic results now carry each node's type.
• Research & Search instructions: search first, read the top three results, synthesize across them, prefer a current document over a superseded one, and read rather than offer to.
• Eval harness: --only <id,id,...> and --list on every eval; eval:decisions gains six question scenarios.

Measured

Live, on the locked models:

• Stage 1 (live_stage1_golden_prompts): 17 of 17 questions and find requests route as a lookup; writes stay queries; a write plus a lookup is split with route_multi; vague requests still clarify; questions about the agent route as a query. Stable across three runs.
• Retrieval (live_skill_retrieval_stability): 21 guards pass, including 14 lookups at rank 1 and every pre-existing routing guard.
• eval:decisions (selected scenarios): the search-before-answering scenario, the skills question, and both existing question-phrased scenarios pass.
• eval:routing: see the comment below for the run on the final build.

Notes for review

• The scoping sentence in route_lookup's description is load-bearing: without it, the tool's presence stopped the model splitting a compound request and stopped it clarifying "help me manage our roadmap". Both are now pinned by live tests.
• An exclusion on Research & Search was tried for the displacement problem and rejected: it also dropped the skill out of the top 3 on short lookups.
• Docs: nodespace-docs has matching changes to prompt-skill-system.md, data-layer.md, ADR-038 and agent-eval.md, committed alongside the merge.

Test plan

• cargo nextest -p nodespace-agent: 855 pass
• bun run test:scripts, bun run test:skill, SKILL.md drift and size checks
• Prompt goldens regenerated and reviewed
• Live Stage 1 and retrieval guards on the real models
• eval:routing and selected eval:decisions on the final build
• bun run test:changed

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review — PR #3480 (issue #3265)

Review Type: Initial Review

Verdict: REQUEST CHANGES. The change is a clear net improvement and has no security or data-integrity problem. Three things should be settled before merge: acceptance criterion 6 is only half met and one new live test pins the opposite of it; criterion 2 has no recorded evidence yet; and one accompanying doc names a constant that doesn't exist.

Reviewed by reading the three-dot diff (origin/main...HEAD, 22 files) and the surrounding code. I ran only bun test scripts/eval/runner.test.ts scripts/eval/fixtures/decisions.test.ts (111 pass). No Rust tests, live tests or evals were run, so every claim below about model behaviour is from reading, not measurement.

Requirements Check

#	Criterion	Status	Notes
1	Research & Search description carries retrieval and question vocabulary	✅	skill_pipeline.rs description names find / look up / locate / list / search for and the how / what / why forms.
2	eval:decisions shows Research & Search routed for "find" and question prompts, without regressing other skills	❌ not demonstrated	Six scenarios were added, and scoring is unit-tested. The PR body reports only "selected scenarios" and does not name the four skill-* scenarios; "see the comment below" refers to a comment that isn't on the PR; the test-plan box for the final-build eval run is unchecked. A partial --only run can't show non-regression.
3	A knowledge question always searches before answering or asking for context	✅ with a gap	The system-run search covers the zero-tool case on route_lookup turns. Stage 2 can still ask without searching through route_clarify (finding 2).
4	Explanatory questions read the top results and synthesize; no offer to read	✅	Guidance and the system call both use include_markdown=3. Each result's markdown is cut at 2,000 characters (finding 6).
5	search_nodes / search_semantic exclude ai-chat nodes	✅ with a limit	search_nodes drops them on both query paths and refuses a chat-scoped search; search_semantic never had them (chats aren't embedded). The exclusion is a post-filter with fixed headroom (finding 3).
6	Questions about the agent's own skills are answered from the skill set, not a Knowledge-scope search	❌ partial	"Which skills do you have?" is covered. "Find the skill for X", the issue's first reported failure, is deliberately routed as a lookup and so is always searched (finding 1).
7	Prompt goldens regenerated and reviewed	✅	stage1_request.golden and stage2_tool_surface.golden updated, stage2_lookup_block.golden added. I did not run prompt_assembly_snapshot.

Code Review Findings

🟡 Important

1. "Find the skill for X" is routed into the search the skill list says never to run
📁 packages/agent/tests/it/live_stage1_golden_prompts.rs:310, packages/agent/src/local_agent/routing.rs:266-279, packages/agent/src/local_agent/agent_loop.rs:3081-3101

• Problem: stage1_routes_a_knowledge_question_as_a_lookup requires "could you find the skill on how to find nodes in nodespace?" to route as route_lookup. On that turn Stage 2's prompt says "Skills are not stored in the user's notes: never search for one". If the model obeys and answers from the list, the turn has no tool call, so the loop drops that answer and runs search_semantic on the topic in the default Knowledge scope. If the model searches itself, it is the same search. Either way the turn does what criterion 6 says it must not, and the two mechanisms contradict each other on the issue's headline case.
• Recommendation: Extend the "Not for a question about you" clause in route_lookup's description to cover finding or naming a skill, move this prompt to stage1_does_not_route_talk_about_the_agent_as_a_lookup, and add an eval:decisions scenario beside op-own-skills-no-search for the "find the skill for X" form. If routing it as a lookup is the intended outcome, say so on the issue and amend the criterion instead.
• Principle: One rule per case. Two mechanisms that give opposite instructions for the same turn make the result depend on which one the model happens to trigger.

2. Stage 2 can still ask the user before searching on a lookup turn
📁 packages/agent/src/local_agent/agent_loop.rs:2564-2566, :3081

• Problem: The backstop fires only when tool_calls.is_empty(). with_stage2_clarify still adds route_clarify to a lookup turn's surface whenever the intent hasn't clarified yet. A Stage-2 route_clarify is a tool call, so the turn ends with a question to the user and no search. That is the "asked what the user meant" failure, by a different channel, and it contradicts the doc sentence "A lookup is always searched before the user sees a reply". No test covers it.
• Recommendation: Don't add route_clarify when routed.lookup_topic.is_some() until a search has run this turn, or treat a round whose only call is route_clarify as unsearched. Add a loop test: lookup round, then a route_clarify round, asserts one search ran first.
• Principle: A structural rule should hold on every exit from the turn, not only the prose one.

3. The conversation exclusion can return zero rows when matching notes exist
📁 packages/agent/src/local_agent/tools.rs:281-289, :2617-2626, :2709-2730

• Problem: Chats are dropped after the fetch, from a page of limit + 25. The constant's comment says only the current chat and a few earlier ones can match, and that an overflow "returns fewer than limit rows rather than a wrong one". Both miss the enumerate and sorted cases: search_nodes(query="", sorting=[created_at desc], limit=5) matches every chat, so a user with 30 recent chats gets an empty result and the model reports that nothing exists. An empty result is a wrong answer. The same applies to resolve_query, which shares run_node_query.
• Recommendation: Push the exclusion into the query on both paths (an excluded-types predicate on QueryNodesInput and ExecuteQueryInput, resolved from the registry once), and delete the headroom constant. If that is out of scope here, fix the comment to state the real failure and add a test with more than 25 matching chats so the limit is visible.
• Principle: Filter before limiting. A post-filter over a bounded page is correct only while the bound holds.

4. Every lookup turn's first reply is discarded, with no control for questions the conversation answers
📁 packages/agent/src/local_agent/agent_loop.rs:3064-3101, packages/agent/tests/it/live_stage1_golden_prompts.rs:377-401

• Problem: The earlier design accepted a second prose reply; this one always drops the first. That is right when Stage 1's route_lookup is right. The live controls cover "hi", "thanks" and two questions about the agent, but no follow-up the conversation itself answers ("what did you mean by that?", "summarise what you just found") and no general question ("what is 15% of 240?"). route_lookup's description ("a question about anything they may have written down or stored") reads broadly enough to take those. If it does, a correct answer is thrown away, a search on an unrelated topic runs, and the model answers again with up to three unrelated documents in context.
• Related, from reading the frontend: ai-chat-node-viewer.svelte:423-429 appends every token and clears on done. The dropped reply ("I do not have a tool or information regarding that") is therefore shown while it streams and then vanishes before the real answer. The PR's own measurement says this is the usual path on the locked model. I did not confirm it in the running app.
• Recommendation: Add a live Stage-1 control for conversation-referential and general questions, expecting route_query. Consider whether Stage 2's generation on a lookup turn should be withheld from on_chunk until the loop knows it will keep it.
• Principle: A backstop that overrides the model needs its false-positive side measured as well as its true-positive side.

5. data-layer.md names a constant that does not exist
📁 nodespace-docs/architecture/data-layer.md:297 (already committed)

• Problem: It says "The conversation types are named once, in CONVERSATION_NODE_TYPES". The code resolves through the registry (is_conversation_type → NodeService::type_is_a(.., CoreNodeType::AiChat)), which is the right design and the one the PR body describes. CONVERSATION_NODE_TYPES appears nowhere in packages/ or scripts/. The PR body says the data-layer change will be "committed alongside the merge", but that file has no uncommitted diff: the stale text is what is on disk.
• Recommendation: Replace the sentence with the registry rule, subtypes included.
• Also: the uncommitted nodespace-docs diff contains hunks unrelated to this PR: playbook-engine.md, components/node-types.md, and in prompt-skill-system.md the assemble_turn signature and the "current user" paragraph. This branch still calls assembler.assemble(..) and TemplateContext has no current user. Commit only this PR's hunks. The hunks that do belong to this PR (route_lookup, select_candidates, the system-run search, ADR-038, agent-eval.md's --only / --list) match the code.
• Principle: Docs are the reference other agents read first; a name that doesn't resolve sends them looking for code that isn't there.

🟢 Suggestions

6. "In full" and "the complete document" overstate what the model receives
📁 packages/agent/src/local_agent/routing.rs:578, packages/agent/src/skill_pipeline.rs:448, :459, packages/agent/src/local_agent/tools.rs:3149

• Problem: exec_search_semantic truncates each result's markdown to BODY_TRUNCATE_FULL (2,000 characters), and get_node with format=markdown does the same. The new guidance says results "come back with their full text" and that a non-empty markdown field "is the complete document … do NOT call get_node". For a long architecture doc the model reads the first 2,000 characters and is told not to look further. The truncation predates this PR; the stronger wording is new.
• Recommendation: Word the guidance and the lookup_call comment as "the opening of each document", or raise the cap for results requested through include_markdown.

7. Tests that would pin claims the PR makes

• tools.rs:3139: search_semantic now reads nodeType, a real fix (the old keys never matched, so type was always empty). Nothing pins it. Extract the row mapping into a pure function like search_result_summary and unit-test it.
• search_nodes_excludes_conversations.rs: "subtypes included" is claimed and untested. Add a case with a type that extends ai-chat, and one through resolve_query.
• routing.rs:66-80: with two read-only skills in the top three and one of them leading, lookup_took_a_place is true and a fourth candidate is added. The doc comment and prompt-skill-system.md say "a lookup that leads takes no extra". Pin whichever is intended.

8. system_lookup is reused as the tool-call id on every such turn
📁 packages/agent/src/local_agent/routing.rs:588

• Problem: The id is persisted in session.messages, so a chat with several lookup turns replays duplicate tool-call ids. The local engine doesn't care. I did not check whether any OpenAI-compatible endpoint a user configures rejects that.
• Recommendation: Make it unique per turn, for example by appending a UUID.

Nitpicks

• Nit: agent_loop.rs:3085: .filter(|_| iteration + 1 < MAX_TOOL_ITERATIONS) can't be false. With !any_real_tool_calls the loop is at iteration 0, or 1 after the clarify nudge. Drop it, or say it is defensive.
• Nit: routing.rs:71-73: next_can_write checks !c.tools.is_empty() && skill_is_mutating(c) but not clears_score_gate, while the read-only side uses is_tool_bearing_contender. A fourth candidate below its bar is kept and contributes nothing. Use the same predicate on both sides.
• Nit: agent_loop.rs:3081: any_real_tool_calls is set for a malformed call that never executed, so a lookup whose only call was a malformed search_semantic is not backstopped. Probably acceptable; worth a line in the comment.
• Nit: live_skill_retrieval_stability.rs:722-727: Box::leak to get &'static str. Returning Vec<String> and borrowing at the call site avoids it.

What holds up

• Trust boundary. select_candidates can't put a destructive tool on the surface. stage2_permitted_names admits destructive tools only from the top-scoring tool-bearing candidate, and a fourth-ranked candidate can't be that. Every candidate still has to clear its own score bar. The bound is at most four, on the multi-query path too.
• Loop mechanics. The system call goes through the normal execution path, so cancellation, the iteration cap, seen_calls (a repeat of the same search by the model trips the duplicate guard) and history shape (assistant tool call, then result, prose dropped) all behave as for a model-issued call. It takes the place of the clarification nudge cleanly, and a_lookup_in_a_clarified_intent_is_searched_not_told_to_act pins that.
• Tests fail without the fix. The six loop tests assert generation counts, the exact search arguments and history contents. search_nodes_excludes_conversations.rs includes a control proving the seed does put a matching chat in the store.
• Project rules. No issue numbers in added comments, no literal node_type comparisons in product code (resolved through type_is_a), no compatibility code, no new .md outside the skill payload, no lint suppressions.
• Eval harness. --only keeps setup turns and earlier scenarios in the same chat, rejects unknown ids, refuses --baseline, and marks partial runs in the results file.

Recommendation

REQUEST CHANGES

Before merge:

1. Resolve finding 1 (criterion 6) either way.
2. Record the eval:decisions evidence for criterion 2: the four skill-* scenarios, plus a full run for non-regression.
3. Fix the data-layer.md sentence and commit only this PR's doc hunks.

Findings 2 and 3 are small and worth doing in this PR. Finding 4 needs at least the live control. The rest is optional.

Also still open on the PR's own test plan: bun run test:changed.

[malibio]

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

[malibio]

Address-review summary (commit 0c95d7f9)

Addressed

1. "Find the skill for X" routed into a search the skill list forbade. Resolved by removing the contradiction rather than the routing, because the routing could not be moved:

• Measured on the locked model: no wording of route_lookup kept "find the skill for X" out of a lookup without also routing "which skills do you have?" into one. The wording in place keeps the second correct.
• The skill list now states that a search does not return skills, instead of forbidding one. On a lookup turn the model has the list in its prompt and answers from it.
• New eval scenario outcome-find-own-skill scores the reply (it must name the skill), since the tools called say nothing about whether the user got an answer.
• The prompt moved out of the asserting live test into stage1_lookup_false_positives_on_record, which records what Stage 1 does with it.

2. Stage 2 could ask via route_clarify before searching. route_clarify is now off a lookup turn's surface, however it would have got there. Two loop tests: a lookup turn doesn't offer it; a non-lookup turn still does.

3. The conversation exclusion could return zero rows. The fetch now repeats with a larger page when conversations filled the first one, until it yields limit rows or the result is exhausted. Pinned by a test with 40 chats ahead of the notes in a newest-first listing; it returns [] with the refetch removed (checked). I did not push the predicate into SQL: the QueryService path takes a QueryDefinition, the query node's wire type, and adding an excluded-types field there is a node-type change outside this PR.

4. The discarded first reply.

• It is no longer streamed: a lookup's first round is held and forwarded only if it stands. Two loop tests.
• The live control you asked for shows the concern is real: follow-ups on the agent's last answer ("say that again more simply") are routed as lookups. No wording moved them (same result as finding 1). The cost is one search before the reply; the conversation is still in context when the model answers. Recorded in the same diagnostic live test and in the docs.

5. Stale doc sentence. data-layer.md now describes the registry rule and the refetch. Only this PR's hunks will be committed to nodespace-docs.

6. "In full" overstated a 2,000-character cut. Guidance, the lookup_call comment and the docs now say the text is cut off.

7. Missing tests. Added: semantic_result_summary extracted and unit-tested (the nodeType fix); a type extending ai-chat is excluded and a search scoped to it is refused; two read-only skills with one leading still yields a place (and the doc comment says so).

8. Reused tool-call id. Now unique per call, with a test.

Nits. Dropped the always-true iteration filter; the fourth candidate must clear its own bar (test added); a comment covers the malformed-call case; Box::leak removed.

Not done

• resolve_query path test for the exclusion. It shares run_node_query, which the new tests cover on both engines; exercising it through resolve_query needs an inference engine in the test.
• Criterion 2 evidence. Still owed: eval:routing in full and the selected eval:decisions scenarios on this build. The machine is shared with other sessions' evals, so the run is queued; I'll post the results here.

Verification

• cargo nextest -p nodespace-agent: 865 pass. Clippy and the literal node-type check are clean.
• Live Stage 1 guards on the locked model: all pass (lookups 16 of 16, writes, agent questions, the compound split, vague requests).
• bun test for the eval fixtures and runner: 112 pass.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: The fixes change loop behaviour (tool surface on lookup turns, held streaming), add a refetch loop to the search core, and reword model-facing text. These are more than mechanical edits and should be read again.

[malibio]

Pragmatic Code Review — PR #3480 (issue #3265)

Review Type: Re-Review

Verdict: COMMENT. The commit 0c95d7f9 has no blocking code defect, and I found nothing the fixes broke. I am not approving because the fix for finding 1 (acceptance criterion 6) now rests on a model behaviour nothing has measured yet: the reply to "find the skill for X" after a search has run. The new outcome-find-own-skill scenario measures it, and its result is part of the eval evidence still to be posted.

Scope: git diff 1b7153fb..HEAD (11 files) and the surrounding code in agent_loop.rs, tools.rs, routing.rs, node_ops.rs, query_ops.rs, query_service/mod.rs, node_service/query.rs and the daemon's chunk forwarding. I ran bun test scripts/eval/fixtures/decisions.test.ts scripts/eval/runner.test.ts (112 pass). I ran no Rust tests, live tests or evals, and did not mutation-check any test, so every statement about what a test pins is from reading.

Previous Review Summary

• Previous recommendations: 12 (8 findings, 4 nits)
• Addressed: 10
• Partially addressed: 2 (findings 1 and 4)
• Not addressed: 0
• New findings: 2 Important, 3 Suggestions, 3 nits

Status of each previous finding

#	Finding	Status	Justification
1	"Find the skill for X" routed into a search the skill list forbade	Partial	The contradiction is gone: render_skill_names_for_prompt now states that a search does not return skills, and the golden matches. The turn is still a lookup, so a Knowledge-scope search still runs, which criterion 6 as written on the issue rules out. Whether the reply then names the skill is unmeasured. See "Merge conditions".
2	Stage 2 could ask through route_clarify before searching	Addressed	tools.retain at agent_loop.rs:2573-2575 runs after with_stage2_clarify, so it covers the addition, a runner-up's whitelist and the fail-open surface. The surface test fails without it. Residual gap in new finding 3.
3	The exclusion could return zero rows	Addressed	run_node_query refetches when a full page came up short. The 40-chat test takes that path (27 rows, then 108) and returns [] without the loop. The reason given for not filtering in SQL does not hold as stated (new finding 4), but deferring it is a fair scope call.
4	Every lookup's first reply is discarded, with no false-positive control	Partial	The held stream is done and correct on every exit I traced. The control was run and confirmed the concern, then recorded in a test that asserts nothing. The general-question control ("what is 15% of 240?") was not added. See new finding 2.
5	data-layer.md named a constant that does not exist	Addressed on disk	Line 297 now gives the registry rule and the refetch. The file is uncommitted, so "only this PR's hunks are committed" is still to do.
6	"In full" overstated a 2,000-character cut	Addressed	Guidance, the lookup_call comment, the golden and prompt-skill-system.md all say the text is cut off.
7	Missing tests	Addressed	semantic_result_summary is extracted and tested on nodeType; a type extending ai-chat is excluded and a search scoped to it refused; two read-only skills with one leading is pinned. The resolve_query case is deferred because it needs an inference engine, which is reasonable: it shares run_node_query.
8	Reused tool-call id	Addressed	system_lookup_<uuid>, with an inequality assertion. uuid was already a dependency.
Nit	Always-true iteration filter	Addressed	Removed.
Nit	next_can_write used a weaker predicate	Addressed	Uses is_tool_bearing_contender; a_fourth_candidate_below_its_bar_is_not_kept fails without it.
Nit	Malformed call is not backstopped	Addressed	Comment added at agent_loop.rs:3100-3103.
Nit	Box::leak	Addressed	lookup_queries returns Vec<String>.

What I checked on the three risky changes

Refetch loop (tools.rs:2691-2729). It terminates. Each pass either returns (summaries.len() == limit, or the page was shorter than fetch_limit) or multiplies fetch_limit by 4, and a full page needs at least fetch_limit rows in the store, so the pass count is logarithmic in the row count. limit = 0 returns on the first pass. saturating_mul cannot wrap. Neither node_ops::query_nodes nor QueryService clamps the limit, and both apply it in SQL on the paths this code reaches, so a short page does mean the result is exhausted. Nothing bounds the page size except the size of the store (new finding 4).

Held streaming (agent_loop.rs:2876-2889, :3124-3129). No chunk is forwarded twice and none is lost on a round that is kept:

Exit from the held round	Result
Model called a tool	lookup_run stays false, chunks replayed once, in order
No call, system runs the lookup	Chunks never forwarded; the next round streams live (lookup_run is true)
No call, lookup_call returns None	Chunks replayed once; any later round is held and replayed the same way
Empty generation	System runs the lookup; nothing to forward
Mid-generation error	Returns Err before the replay; the daemon sends its own error chunk on Err (local_agent_service.rs:879-886), so the user still sees the failure
Cancellation	The daemon's select! drops the turn and sends cancelled; held chunks are discarded

route_clarify off a lookup turn. The clarification contract still holds: a prose question on a lookup turn is replaced by the search, and a_lookup_in_a_clarified_intent_is_searched_not_told_to_act already covers the nudge ordering. The fail-open surface loses the tool too, which is consistent.

New Findings

1. [Important] The replay of a held round that stands has no test
File: packages/agent/src/local_agent/agent_loop.rs:3124-3129

• Problem: The two new streaming tests cover a lookup whose reply is dropped (replay branch not taken) and a turn that is not a lookup (never held). No test runs a lookup turn whose first round is kept, which is the usual path when the model searches by itself. From reading, deleting these six lines passes every test, and the daemon would then stop receiving tool_call_start / tool_call_args for that round.
• Recommendation: Add a lookup turn whose first Stage-2 round is tool_round("tc_1", "search_semantic", ..), collect every chunk the callback receives, and assert the ToolCallStart for tc_1 arrives exactly once and before the final text.
• Principle: A branch that exists to preserve behaviour needs a test that fails when it is removed.

2. [Important] The cost of a follow-up being treated as a lookup is recorded but not measured
File: packages/agent/tests/it/live_stage1_golden_prompts.rs:453-491, scripts/eval/fixtures/decisions.ts:547-556

• Problem: The control confirmed that "say that again more simply" is routed as a lookup. On that turn a correct answer from the conversation is dropped, a search runs on the follow-up's wording, up to three unrelated documents of 2,000 characters each enter the context, and the model answers again. The summary's claim that the answer survives because "the conversation is still in context" is an argument, not a result. stage1_lookup_false_positives_on_record prints and asserts nothing, so it cannot fail and will not show a change. The general-question control the first review asked for was not added, and the summary does not mention it.
• Recommendation: Use the replyMatches outcome this commit added. Add a two-turn scenario in one chat: a knowledge question, then "can you say that again more simply?", with the reply required to match the first answer's subject. Include it in the eval run that is already pending. Run "what is 15% of 240?" through the diagnostic test as well and record what it does.
• Principle: A known false positive is acceptable when its cost is measured; here only its existence is.

3. [Suggestion] The rule is enforced on the tool surface, not at dispatch, and for the whole turn
File: packages/agent/src/local_agent/agent_loop.rs:2573-2575, :3539-3560

• Problem: Two things follow from removing the tool once, before the loop.
  • The pre-scan at :3539 honours any call named route_clarify, offered or not. The native engine's tool-call grammar prevents one. A user-configured remote endpoint does not, and the name is in front of the model on a lookup turn whenever Graph Editing places as a runner-up (skill_pipeline.rs:209, "call route_clarify") or an earlier turn in the chat used it.
  • The first review suggested withholding it "until a search has run". It is now gone for the whole turn, so after the search the model can only ask in prose and the structured options are lost, while runner-up guidance and the duplicate-entity message (tools.rs:1022) still tell it to call the tool.
• Recommendation: On a lookup turn with no search yet, drop a route_clarify call before the pre-scan so the system-run search takes over, and add the loop test the first review described (lookup round, then a route_clarify round, assert one search ran first). Leave the whole-turn removal as it is unless the eval shows the prose question hurting.
• Principle: Enforce an invariant where the action happens; a surface is advice to engines that don't constrain it.

4. [Suggestion] Each refetch starts again from the first row
File: packages/agent/src/local_agent/tools.rs:2691-2729, packages/core/src/services/query_service/mod.rs:305-311

• Problem: Pages are 75, 300, 1,200, 4,800 rows for the default limit, each fetched from the top. On the sorted path every row is a separate get_node, then converted to its typed value. A newest-first listing in a database with 500 chats costs about 1,575 single-row reads per call, and the cost grows with the number of chats the user has ever had.
• On the reason given for not filtering in SQL: the summary says QueryDefinition is the query node's wire type, so a new field would be a node-type change. Its own doc comment says the opposite (query_service/mod.rs:57-63: "The execution shape, not the stored one"; the stored shape is nodespace_types::QueryFields). An execution-only excluded-types predicate would not touch a node type.
• Recommendation: Fine to merge as is. When this is revisited, push the predicate into both queries and delete the headroom and growth constants.
• Principle: Filter before limiting.

5. [Suggestion] replyMatches: /research/i passes replies that fail the user
File: scripts/eval/fixtures/decisions.ts:554

• Problem: "I researched your notes and couldn't find a specific skill" matches. That is the reported failure with one word changed.
• Recommendation: Match the skill's name: /research\s*(&|and)\s*search/i. Add that sentence to the unit test as a failing case.
• Principle: A scorer should not pass the failure it was written to catch.

Nitpicks

• Nit: agent_loop.rs:2874-2875: "a round that is kept here is one that called a tool, which carries no answer text" is not true when lookup_call returns None; that round's answer is then delivered in one burst.
• Nit: skill_pipeline.rs:457: "2,000 characters" is a literal copy of BODY_TRUNCATE_FULL, with nothing tying the two together.
• Nit: nodespace-docs/decisions/038-skill-mediated-tool-routing.md:172 still says Stage 1 emits "one of exactly two typed tools", beside line 78 which now describes a third choice. Line 179 lists when Stage 2 loses route_clarify and omits the lookup turn.

Docs

data-layer.md:297, prompt-skill-system.md (the Stage 1 step, the diagram, the select_candidates sentence, "Questions about stored knowledge"), ADR-038 line 78 and agent-eval.md (--only, --list) describe the code as it is now, apart from the ADR nit above. I read the files as they are on disk and did not review them as a diff.

Merge conditions

Not findings; the main agent already has the first one queued.

1. Post the eval evidence for criterion 2: eval:routing in full and the selected eval:decisions scenarios on this build.
2. Include outcome-find-own-skill in that run. It is the only evidence that criterion 6 holds for the issue's first reported failure. If it fails, finding 1 is open again.
3. Criterion 6 on the issue still reads "rather than by a Knowledge-scope search" for "find the skill for X". The turn now searches and then answers from the list. Note that on the issue, as the first review asked.
4. Commit only this PR's hunks to nodespace-docs.

Recommendation

COMMENT

The code is sound and the fixes are real. New findings 1 and 2 are small and worth doing before the eval run, since finding 2's scenario belongs in it. Findings 3 to 5 and the nits are optional.

[malibio]

Automated pragmatic-code-review (re-review): COMMENT. Full findings in the review comment above.

[malibio]

Address-review summary, round 2 (commit 375b2c51)

Addressed

• 1. The replay of a held round had no test. Added a_lookup_round_that_stands_is_forwarded_once_and_in_order: a lookup whose first round is the model's own search, asserting the call start is forwarded exactly once and before the answer. It fails with the replay removed (checked).
• 2. The follow-up false positive was recorded, not measured. New eval scenario outcome-follow-up-keeps-its-answer: a looked-up question, then "Can you say that again more simply?", with the reply required to still be about the first answer. It is in the pending eval run. "what is 15% of 240?" is added to the diagnostic Stage 1 test.
• 3. The rule was on the surface, not at dispatch. A route_clarify call on a lookup turn is now dropped until a search has run, so the system-run search takes its place. Test a_clarify_call_on_an_unsearched_lookup_is_replaced_by_the_search fails without it (checked). The whole-turn removal from the surface stays, as you suggested.
• 5. replyMatches: /research/i passed the failure it was written for. It now matches the skill's name, and the unit test includes "I researched your notes and couldn't find a specific skill" as a failing case.
• Nits. The held-stream comment covers the no-search-tool case. The guidance's "2,000 characters" is tied to BODY_TRUNCATE_FULL by a test. ADR-038 now lists four Stage 1 tools and says a lookup turn never offers route_clarify.

Not done

• 4. Filtering in SQL. You are right that my stated reason was wrong: QueryDefinition is the execution shape, not the stored one, so an excluded-types predicate would not be a node-type change. It is still a change to two query paths in nodespace-core and to every construction of NodeQuery, so I am leaving it out of this PR as you allowed. The refetch is correct; its cost is that each pass re-reads from the first row.

Still owed (merge conditions)

1. eval:routing in full and the selected eval:decisions scenarios on this build, including outcome-find-own-skill and outcome-follow-up-keeps-its-answer. Queued: other sessions are running evals on this machine.
2. A note on the issue that "find the skill for X" is searched and then answered from the skill list, rather than answered without a search as criterion 6 is worded.
3. Commit only this PR's hunks to nodespace-docs.

Verification

• cargo nextest -p nodespace-agent: 868 pass; clippy clean.
• bun test for the eval fixtures: 20 pass; scripts lint and typecheck clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The code change in this round is three lines at dispatch, implementing the re-review's own recommendation, and it is pinned by a test that fails without it. Everything else is tests, an eval scenario, comments and docs. The open items are evidence to post, not code to read.

[malibio]

Eval evidence on the final build (commit 6aa8e8b9, merged with current main)

Locked model gemma-4-e4b-q4km, n_ctx 32768, fresh isolated daemon and purged database per eval, single rep each.

eval:decisions --only (10 scenarios): 10/10

Scenario	Result	Tools
skill-find-request	pass	search_semantic
skill-question-how	pass	search_semantic
skill-question-what	pass	search_semantic
skill-question-explain	pass	search_semantic
outcome-question-searches-first	pass	search_semantic
op-own-skills-no-search	pass	none
outcome-find-own-skill	pass	search_semantic, reply names Research & Search
outcome-follow-up-keeps-its-answer	pass	none
op-list-by-type	pass	search_nodes
schema-shared-name-signed	pass	search_nodes

This is a partial run (--only), so it is evidence for these scenarios and not a full-suite score.

eval:routing (full): 18/19

• The one failure is indirect-schema-freelance ("start tracking our planning cycles" creates a node instead of a schema). It failed in every run on this branch, including the first one before any routing change, and Research & Search is not among its candidates.
• Clarification scenarios (ambiguous-client-contacts, ambiguous-manage-projects, clarification-then-fallthrough), both compound scenarios and both single-intent-at-length scenarios pass.

Two failures the previous run found, and what fixed them (commit 60cc35a0)

The previous run on the merged build scored 8/10 on the same selection.

• outcome-question-searches-first failed. After two turns that set up record types, Stage 1 routed "How do we decide which venue gets a deposit refund?" as a query, so the turn was never treated as a lookup and replied "I do not have a tool or information". Reproduced in the live Stage 1 check at 3 of 3. No wording of the routing tools fixed it without breaking the compound split or a find request (five variants measured). Fix: a question-shaped message is put to Stage 1 by itself first; a lookup from that pass stands, anything else is decided on the blended view as before. Live check after the fix: 21 of 21 questions asked after unrelated turns route as lookups, and question-shaped write requests still route as queries.
• outcome-follow-up-keeps-its-answer failed. The model answered correctly, then the clarification contract's re-prompt (a chat that has only asked questions counts as already clarified) replaced it, and the second reply was "I'm not sure what you mean". Fix: the re-prompt counts only when it produces a call; otherwise the first reply stands and the re-prompt is removed from the history.

Both changes are in the agent loop and came after the last review, so I am requesting a re-review of 375b2c51..HEAD.

Also verified

• cargo nextest -p nodespace-agent: 892 pass; clippy clean.
• Live Stage 1 checks on the locked model: 7 of 7 pass.

[malibio]

Pragmatic Code Review — PR #3480 (issue #3265)

Review Type: Re-Review (2)

Verdict: COMMENT. 375b2c51 resolves the last re-review's findings. 60cc35a0 fixes two measured failures and I found no proven defect in it. I am not approving because the new two-pass routing takes a lookup from a message read without its context, and three things about that are open: one case where it can break the clarification flow (finding 1, a small structural fix), one class of message nothing has measured (finding 2), and a unit test that passes with the change removed (finding 3).

Scope: git show 375b2c51, git show 60cc35a0, the current code around each change (agent_loop.rs route, stage1_decide, the turn loop from the held stream to the final-reply guards; routing.rs; tools.rs run_node_query), the conflict resolutions in the three merge commits, and the four docs. I ran no tests, live tests or evals. Every statement about what a test pins is from reading: I did not revert a change and watch a test fail.

Previous Review Summary

• Previous recommendations: 8 (5 findings, 3 nits)
• Addressed: 7
• Partially addressed: 0
• Not addressed: 1 (finding 4, deferred as the re-review allowed)
• New findings: 3 Important, 5 Suggestions, 4 nits

Status of each previous finding

#	Finding	Status	Justification
1	The replay of a held round that stands had no test	Addressed	a_lookup_round_that_stands_is_forwarded_once_and_in_order (agent_loop.rs:15540) runs a lookup whose first round is the model's own search. Without the replay at :3358-3362 no ToolCallStart for tc_1 reaches the caller and the count assertion fails.
2	The follow-up false positive was recorded, not measured	Addressed	outcome-follow-up-keeps-its-answer exists and passed in the posted run; "what is 15% of 240?" is in the diagnostic test. The diagnostic still routes its follow-ups on the blended view only, which is no longer what the loop does (see new finding 2).
3	The lookup rule was on the surface, not at dispatch	Addressed	agent_loop.rs:3334-3336 drops a route_clarify call until a search has run. a_clarify_call_on_an_unsearched_lookup_is_replaced_by_the_search would end on clarify: Some without it. One narrow gap in new finding 8.
4	Each refetch starts again from the first row	Not addressed	Deferred, as the re-review allowed. The summary now gives the right reason (QueryDefinition is the execution shape).
5	replyMatches: /research/i passed the reported failure	Addressed	Now `/research\s*(&
Nit	Held-stream comment was untrue for a surface with no search tool	Addressed	agent_loop.rs:3095-3098.
Nit	"2,000 characters" was a literal copy	Addressed	research_guidance_states_the_real_markdown_cut builds the phrase from BODY_TRUNCATE_FULL.
Nit	ADR-038 said "two typed tools" and omitted the lookup turn	Addressed	Lines 172 and 179 on disk.

Merge conflict resolutions

All five look right.

• tools.rs: main's "a row carries only what it has" pruning and this PR's wire_node_type / semantic_result_summary are both kept. run_node_query reads the type through wire_node_type, and an empty type is not a conversation, so a pruned schema row is handled.
• query_ops.rs: Clone is added to main's reworded struct; nothing else changed.
• search_nodes_excludes_conversations.rs: chats are created as ai-chat-native / ai-chat-pty, and a search scoped to the base or either subtype is refused. The exclusion still resolves through type_is_a(.., CoreNodeType::AiChat). The starts_with("ai-chat") check is in a test.
• decisions.test.ts, runner.test.ts: both sides' tests and imports are kept.

What I checked on the two new behaviours

Two-pass routing (agent_loop.rs:4676-4721).

Path	Result
First pass returns a lookup	Used; one Stage 1 generation
First pass returns query / multi / clarify / nothing	Discarded; the blended pass decides. A first-pass clarify is never shown to the user
First pass errors	Warned, blended pass runs. The warning carries no routing_decision field and does not contain "stage-1 routing failed", so scripts/aichat.ts:236-244 still picks exactly one routing line per turn
Both passes error	routing_decision = "failed", as before
Cancelled between passes	No check between them, but the daemon's select! drops the turn at the next await, so the second generation does not complete
Token accounting	Both passes are added, including a discarded first pass, on the clarify return too. InferenceUsage has only the two fields that are summed
Latency	routing_latency_ms covers both passes (see finding 5)

Keeping the first reply (agent_loop.rs:3376-3424).

• The kept reply shadows response_text before normalisation, so the anti-fabrication, fabricated-id, narrated-tool-call, no-op-success and tool-failure guards all judge the reply the user gets.
• any_real_tool_calls is checked on both sides: a re-prompt that produced a call leaves the later reply and the nudge in place, as before.
• An empty second generation now returns the first reply instead of the "empty response" error. That is an improvement.
• The pop cannot remove the wrong message: it checks that the last message is the System nudge by role and content. If maybe_summarize_history rewrote the tail, nothing is popped.
• session.messages ends on the kept reply, and prior_turns records it as Replied, which the test asserts.
• On a lookup turn the nudge can only fire when the surface has no search_semantic; both rounds are then held and replayed, the same as finding 6 describes for other turns.

New Findings

1. [Important] A question-shaped answer to a clarification can be routed as a lookup
File: packages/agent/src/local_agent/agent_loop.rs:4676-4697

• Problem: The comment above the blend (:4652-4656) says it matters most right after a clarification, because the answer is "the message with the least standalone routing signal". The new first pass takes exactly that message by itself whenever it ends in a question mark. After "I can take that a couple of ways: client contacts or vendor contacts?", a reply of "the client ones?" or "clients?" is put to Stage 1 alone, and route_lookup's description tells the model a lookup is "always clear enough to route … never a reason to ask". If that pass returns a lookup, the turn searches for "the client ones", route_clarify comes off the surface, and the request the user was clarifying is not acted on. I could not run this, so it is a risk read from the code, not an observed failure.
• Recommendation: Skip the first pass when the previous turn ended Clarified (session.prior_turns.last()): the message is then an answer, whatever its punctuation. Add a unit test that seeds a Clarified turn, sends "the first one?" and asserts one Stage 1 generation.
• Principle: A new rule should not weaken an invariant the same function documents; the outcome of the previous turn is structure the loop already records.

2. [Important] A question that needs its context is unmeasured, and the diagnostic no longer follows the loop
File: packages/agent/src/local_agent/agent_loop.rs:4671-4688, packages/agent/tests/it/live_stage1_golden_prompts.rs:477-501, :533-563

• Problem: The comment says a follow-up "that means nothing without its context" is decided on the blended view as before. That holds only if the model declines to call route_lookup on it, and the tool's description discourages declining. When the first pass does return a lookup for "And who approved it?" or "How does that work?", the topic is the model's paraphrase of a sentence with no referent. That topic is what retrieval is asked for and what the system-run search sends (routing::lookup_call) when Stage 2 makes no call, so three unrelated documents of up to 2,000 characters replace an answer the conversation could have given.
  • The new live test asks only self-contained questions after unrelated turns, and three write requests.
  • stage1_lookup_false_positives_on_record still sends "what did you mean by backing off?" and "can you say that again more simply?" through run_stage1_with_history, the blended view alone. Both are question-shaped, so the loop now asks about them by themselves first. The record describes a path the loop no longer takes for them.
  • The one eval scenario for a follow-up passed with no tool call, on a single rep.
• Recommendation: Move the diagnostic's follow-ups to route_stage1_as_the_loop_does. Add two or three pronoun questions after a related exchange and assert that a lookup's topic names the referent ("retry"), or that the decision came from the blended pass. If they fail, keep the first pass's decision that the turn is a lookup but take the topic from the blended pass.
• Principle: A claim in a comment about model behaviour needs the same measurement the rest of this PR gives its claims.

3. [Important] The unit test for "routed on the message alone" passes without the change
File: packages/agent/src/local_agent/agent_loop.rs:15616-15677, packages/agent/tests/it/live_stage1_golden_prompts.rs:506-518

• Problem: MockEngine returns its rounds in order and records no request. In a_question_with_history_is_routed_on_the_message_alone the first round is a lookup, so the single blended pass that existed before this commit gives the same retrieval query and the same two generations. The test pins "a first-pass lookup is used and no second pass runs". It does not pin what Stage 1 was shown: passing routing_query to both passes leaves every unit test green. The live test cannot catch that either, because route_stage1_as_the_loop_does is a second copy of the loop's logic with its own gate (!prior_turns.is_empty() against the loop's routing_query != user_message), not the loop.
• Recommendation: Have the mock engine record each request's last user message, and assert the first Stage 1 request is the bare message and the second starts with PRIOR CONTEXT. Move the gate into one function in routing.rs (prior turns and message in, "ask alone first" out) that both the loop and the live helper call.
• Principle: A test should fail when the behaviour it names is removed. Two copies of a rule drift.

4. [Suggestion] A lookup that is not shaped like a question is still routed on the blended view only
File: packages/agent/src/local_agent/routing.rs:194-206

• Problem: "Explain our retry policy", "find the record for the Lisbon offsite", "list the specs we have on sync" and "tell me how we onboard a sponsor" are lookups by route_lookup's own description and are not question-shaped. After unrelated turns they take the path the commit found unreliable for questions. Criterion 2 names "find …" and "explain Z". The commit message says a find request was part of what the wording experiments had to keep working, so some of this was measured, but no test in the tree records it after unrelated turns.
• Recommendation: Add those four to stage1_routes_a_question_as_a_lookup_after_unrelated_turns through the blended path and record the result. If they fail, the gate needs to be wider than the message's shape.
• Principle: Fix the class, or record where the fix stops.

5. [Suggestion] The text log does not say which view decided or how many passes ran
File: packages/agent/src/local_agent/agent_loop.rs:4686, :4871-4878; scripts/aichat.ts:266-273

• Problem: routing.decided_on is a span attribute only. The comment at :4772-4779 explains that the daemon's text log has no OTel exporter and that evals read the text fields. So an eval cannot tell a lookup decided on the message from one decided on the blend, and routing_latency_ms now covers one or two generations with nothing to separate them. aichat.ts still describes the figure as "one generative pass … among three routing tools". Every "Can you …?" request in a chat with history now pays two passes, and the posted evidence has no latency figure.
• Recommendation: Add routing_decided_on (message / blended) and routing_passes to the two routing_decision lines, ahead of routed_skills, which must stay last. Update the aichat.ts comment. Post the routing latency for a question-shaped write before and after.
• Principle: ADR-038 asks for the routing cost to be measured, not assumed.

6. [Suggestion] The second reply is still streamed, then replaced
File: packages/agent/src/local_agent/agent_loop.rs:3099, :3409-3424

• Problem: The round after the nudge streams live. The viewer appends tokens to one buffer and swaps it for the persisted message on done (ai-chat-native-node-viewer.svelte:320-331), so the user sees the answer, then "I'm not sure what you mean" appended to it, then the answer alone. This is the flicker the held stream removed for lookups. Before this commit the same thing happened the other way round, so it is not a regression.
• Recommendation: Hold the round when reply_before_nudge.is_some() and forward it only if it made a call.
• Principle: The user should not be shown text the system has already decided to discard.

7. [Suggestion] In an intent with a composed clarification, the kept reply is the repeated question
File: packages/agent/src/local_agent/agent_loop.rs:3399-3424

• Problem: The reasoning for keeping the first reply is the read-only chat, where the nudge's premise is false and the first reply is an answer. In the other branch the premise is true and the first reply is most likely a second question, which is the loop the contract exists to stop. A second prose reply there has at least been told not to ask again; it is now discarded in favour of the question. The two branches are structurally distinguishable (composed_clarifications is non-empty in one). "It cannot make an answer worse" is true; it can discard a better one. The test seeds a Clarified turn but uses the read-only chat's replies, so it does not show this case.
• Recommendation: Keep the first reply only when the intent holds no composed clarification, or state in ADR-038 that the trade was made for both branches. Either way, add a test whose first reply is a question.
• Principle: One rule over two cases with opposite premises should be decided per case.

8. [Suggestion] A dropped route_clarify has nothing in its place when the surface has no search_semantic
File: packages/agent/src/local_agent/agent_loop.rs:3334-3356

• Problem: The call is dropped whenever the turn is an unsearched lookup, but routing::lookup_call returns None when search_semantic is not offered. A round whose only output was that call then reaches the final-reply path with empty text and fails with "model produced empty response with no tool calls". It needs an engine that ignores the surface and a registry without the search tool, so it is narrow.
• Recommendation: Compute the replacement call first and drop route_clarify only when there is one.
• Principle: Remove an action only when its replacement exists.

Nitpicks

• Nit: agent_loop.rs:4677-4679: stage1_query trims, so a first message with surrounding whitespace is unequal to routing_query and takes the first pass with no history; that pass is also sent untrimmed. Compare against user_message.trim() and send the trimmed text.
• Nit: routing.rs:197: the full-width question mark (？) is not recognised, though the function is otherwise script-neutral on that branch.
• Nit: nodespace-docs/architecture/prompt-skill-system.md:162 says "four rules" above five bullets. Line 170 and live_stage1_golden_prompts.rs:462-466 still say a follow-up on the last answer is routed as a lookup and searched first; the posted run shows that scenario with no tool call, and decisions.ts:590-597 already describes it the new way.
• Nit: decisions.ts:600: /march|2025|14/i passes on any "14" in the reply.

Docs

• prompt-skill-system.md "A question is recognised on the message alone" matches the code, including the fallback and the is_question_shaped rule.
• ADR-038 lines 180-181 match the kept-reply behaviour, including "costing a generation, though no longer the reply".
• data-layer.md:297 names type_is_a(.., CoreNodeType::AiChat) and the refetch, which is what tools.rs does after the merge.
• agent-eval.md covers --only and --list.
• The two stale statements are in the nits above. I read the files on disk and did not check what is committed there.

Requirements check

#	Criterion	Status
1	Research & Search's description carries retrieval and question vocabulary	Met (unchanged since the first review)
2	eval:decisions shows it routed for "find …" and question prompts, without regressing other skills	Met on the posted evidence: the four skill-* scenarios pass, eval:routing is 18 of 19 and the one failure predates the branch. The decision run is --only, one rep, and 60cc35a0 changes routing for every question-shaped message with history, so the rest of eval:decisions has not been run against it
3	A knowledge question always searches before answering or asking; the zero-tool case is covered	Met, structurally: the system runs the search when Stage 2 makes no call. outcome-question-searches-first passes. Findings 1, 2 and 4 are about which turns reach that rule
4	Explanatory questions read several results and do not offer to read	Met by the guidance and the system-run search (include_markdown: 3). No scenario scores synthesis across results directly
5	Search excludes ai-chat nodes	Met, and the tests now cover both chat subtypes and the abstract base
6	Questions about the agent's own skills are answered from the skill set	Met for "what skills do you have" (op-own-skills-no-search, no tool call). "Find the skill for X" is searched and then answered from the list (outcome-find-own-skill passes); the deviation from the criterion's wording is noted on the issue
7	Prompt goldens regenerated and reviewed	Met. 60cc35a0 only re-wraps the two tool descriptions; their text is unchanged, so the goldens stand

Recommendation

COMMENT

The work is a net improvement and the eval evidence supports the acceptance criteria. I would approve once finding 1 has its guard and test, finding 3's test asserts what Stage 1 was shown, and finding 2's cases have been run through the live helper with the result recorded. Findings 4 to 8 and the nits are optional. A full eval:decisions run on the final build would close the caveat on criterion 2; whether to spend it is your call.

[malibio]

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

[malibio]

Address-review, round 3 (after re-review 2)

Head is now a84fafef. Since re-review 2 the branch has the two commits that answered it, six more from eval findings, and four merges of main.

Addressed from re-review 2

• 57cc6fc2 — the answer to a clarifying question is never asked about alone; what Stage 1 is asked is pinned in tests; the round after a clarification re-prompt is held until it is known to stand; a route_clarify call on a lookup is dropped only when a search takes its place; keep-first-reply is scoped to chats that never composed a clarification.
• 4f48fbde — the message-first pass covers retrieval verbs as well as questions (is_lookup_shaped).

Changed since, from eval findings

Commit	Change	Why
082ef300	A node id is grounded by the system's record of an earlier turn, not only by a tool message	A chat rebuilt from storage holds no tool messages. A follow-up that linked the node the previous turn looked up was read as inventing the id and replaced with "could you confirm". Mutation-checked.
897b7db3	Eval harness follows the daemon's log across a rotation during a turn	The daemon now rotates its log at 10 MB. The harness read by byte offset, so the turn in flight at each rotation was scored as having made no decision.
a526cb2c	outcome-follow-up-keeps-its-answer scores that the reply is still about Northwind, not that it contains the date	Whether the turn before found the date is not the scenario's subject, and it varies: the model reads the record as JSON on some runs and as markdown, which omits properties, on others.
b60aa499	Main's type-line live test recognises a lookup as a routed decision	With route_lookup offered, all seven "records of a known type" questions route with or without the type line. Expectations updated and the measurement recorded beside main's.
dd9be0aa	Stage 2 is shown the skill list only on a turn shaped like a question or a find request	Shown on every turn it contributed to a write turn producing malformed create_node arguments. Ablation: without the list the same turn recovered.
0fa4cc2c	A question answered in prose is not put back to act in a chat that never composed a clarification	"Which skills do you have?" was answered correctly from the list, re-prompted, and then searched the user's notes for its own skills. Mutation-checked.

Merges of main

Four, the substantive ones being the move of built-in skills to Markdown seed files (the Research & Search description, its measurement notes and guidance body are ported to the new layout), the Stage 1 type line, and type arguments held to linked schemas.

Not addressed here

• schema-on-menu-company fails on current main 0 of 3 in the state a full fixture run puts it in (the types already exist when the chat's setup runs), with malformed create_node arguments. This branch passes 1 of 3 in the same state. It passed on main as of earlier today, so a recent main commit regressed it. Not this PR; to be filed.
• declarative-fact-report-resolved fails on main too.
• get_node with format=markdown omits a record's properties, so a question about a field ("when did we sign X?") is answered "not visible" on the runs where the model picks markdown. Pre-existing; noted for a follow-up decision.
• The decision fixture re-runs its setup turns per chat against one database, so later chats' setups fail with "already exists". The harness labels some of those "degenerate empty generation", which they are not.

Verification

• bun run test:changed passed at 897b7db3; since then 1,668 agent, daemon and CLI tests and 1,333 script tests pass at a84fafef.
• Live Stage 1 and retrieval tests on the merged tree: 34 of 34.
• eval:decisions at dd9be0aa: 18/20, skill routing 7/7, no blocked setups. The one failure in this PR's area (op-own-skills-no-search) is what 0fa4cc2c fixes; it passes in a selected run at 0fa4cc2c. eval:routing: 17/19 (indirect-schema-freelance, which fails in every run, and one Stage 2 sampling difference on an identically routed turn).
• A final full run on a84fafef is in progress; results will be posted here.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: The agent loop's behaviour changed in three places after re-review 2 (id grounding, when the skill list is shown, when the clarification re-prompt fires), and the merge with main reworked how this PR's skill text is seeded.

[malibio]

Pragmatic Code Review — PR #3480 (issue #3265)

Review Type: Re-Review (3)

Verdict: COMMENT. Re-review 2's findings are addressed, and the six later commits are each a reasonable answer to a measured failure. I am not approving because the fix for "the second reply is streamed, then replaced" introduced a streaming regression on the neighbouring path (finding 1, a one-line fix), the new id-grounding rule rests on a statement about system messages that is not true of one of them (finding 2), and half of the newest condition in the loop has no test that fails without it (finding 3).

Scope: 57cc6fc2, 4f48fbde, 082ef300, 897b7db3, a526cb2c, b60aa499, dd9be0aa, 0fa4cc2c; the current code around each change in agent_loop.rs (run_turn from the tool surface to the final-reply guards, route, grounded_node_uris_from_history, maybe_summarize_history) and routing.rs; the merge results in the six named files; the docs repo's working tree. Head a84fafef.

I ran no builds, tests, live tests or evals, as instructed. Everything below about what a test pins, and about runtime behaviour, is from reading the code. I did not revert a change and watch a test fail.

Previous Review Summary

• Re-review 2 recommendations: 12 (3 Important, 5 Suggestions, 4 nits)
• Addressed: 10
• Partially addressed: 2 (findings 2 and 5)
• Not addressed: 0
• New findings: 3 Important, 5 Suggestions, 5 nits

Status of each re-review 2 finding

#	Finding	Status	Justification
1	A question-shaped answer to a clarification could be routed as a lookup	Addressed	routing::asks_about_the_message_first takes answering_a_clarification (agent_loop.rs:5020-5030). an_answer_to_a_clarification_is_never_asked_about_alone asserts the one Stage 1 request starts with PRIOR CONTEXT.
2	A question that needs its context was unmeasured; the diagnostic did not follow the loop	Partially addressed	The diagnostic now routes through route_stage1_as_the_loop_does, which calls the loop's own gate, and three pronoun questions are added. They are printed, not asserted, and their output is not in the PR. The comment at agent_loop.rs:5012-5018 states the trade (a poor topic is used only when Stage 2 makes no search). That is a defensible decision; the measurement behind it is still not posted.
3	The unit test passed without the change	Addressed	MockEngine.asked records each request's last user message; the tests assert the bare, trimmed message on the first pass and PRIOR CONTEXT on the second. The gate is one function shared with the live helper.
4	A lookup not shaped like a question took the blended path only	Addressed	is_lookup_shaped adds nine retrieval verbs; the four examples are in both the unit test and the live test.
5	The text log did not say which view decided or how many passes ran	Mostly addressed	routing_decided_on and routing_passes are on the main line, ahead of routed_skills. The early-return clarify line (agent_loop.rs:5143-5147) carries neither. No before/after latency for a question-shaped write was posted.
6	The second reply was streamed, then replaced	Addressed, with a regression	The round after the re-prompt is held. The hold never releases: see new finding 1.
7	Keep-first-reply applied where the first reply is the repeated question	Addressed	reply_before_nudge is set only when composed_clarifications is empty; after_a_composed_clarification_a_second_prose_reply_is_the_one_accepted covers the other branch.
8	A dropped route_clarify had nothing in its place without search_semantic	Addressed	system_lookup is computed first and the call is dropped only when it is Some (agent_loop.rs:3575-3585), with a test.
Nit	Untrimmed first-turn message took the first pass	Addressed	message_alone = user_message.trim() is what is compared and sent.
Nit	Full-width question mark	Addressed	With a test.
Nit	"four rules" above five bullets; stale follow-up sentence	Addressed	The docs paragraph no longer counts, and the follow-up sentence matches the code.
Nit	`/march	2025	14/i` passed on any "14"

Merge resolutions

All six look right.

• skill_pipeline.rs (SKILL_SEEDS, Research & Search row): main's fixed-UUID seed row carries this PR's description and its measurement comment. Tools and max_iterations are main's.
• seeds/skills/research-and-search.md: I compared it line by line with the body in a526cb2c:packages/agent/src/skill_pipeline.rs. SEARCH FIRST, READ BEFORE ANSWERING, SYNTHESIZE, the reworded markdown bullet, USE MARKDOWN DIRECTLY and FETCH ADDITIONAL CONTENT are all ported unchanged; nothing of main's was dropped below them.
• agent_loop.rs route(): stage1_type_names(.., user_message) is computed once and the same system prompt goes to both passes. The type line is derived from the message, not the blend, so the two passes differ only in the user text, which is what the two-pass design needs.
• live_stage1_golden_prompts.rs: main's type-line test maps RouteDecision::Lookup to a routed decision with its topic as the query, so routed, reworded and type_words_added all still measure something. Expectations changed from Query to Lookup for eight cases, with the second measurement recorded beside the first.
• prompt_assembly_snapshot.rs: both sides' imports are kept, and the lookup-block golden uses main's schemas_linked field.
• decisions.ts: main's heldToOfferedTypes branch is checked first, then this PR's listsTypes / replyMatches / noDuplicateOf / searchesBeforeReplying. The summary line keeps main's off-menu lines and this PR's "one or two passes" wording.

New Findings

1. [Important] After a re-prompt that works, the final answer is never streamed
File: packages/agent/src/local_agent/agent_loop.rs:3321, :3605-3614, :3681-3696

• Problem: hold_for_reprompt is reply_before_nudge.is_some(), and reply_before_nudge is only taken on the final-reply path. When the held round makes a tool call (the outcome the re-prompt exists for), it is forwarded and the loop continues with reply_before_nudge still Some. Every later round is therefore held too. The round that carries the answer has no tool call, so held_round_stands (!tool_calls.is_empty()) is false and its chunks are never forwarded. The match at :3681 then falls to _ => response_text because any_real_tool_calls is true, so the answer is returned and persisted but was never sent to on_chunk. The only on_chunk call in the loop is the replay at :3612.
  • The user sees the first prose reply streamed, then the tool activity, then nothing until the turn completes and the viewer swaps in the stored message.
  • Before 57cc6fc2 that round streamed live. The lookup hold does not have this problem: its condition includes !any_real_tool_calls, so it releases itself.
  • two_search_then_prose_turns_count_as_the_intents_clarification (:16294-16338) runs exactly this path (a chat that only read, a re-prompt, a search_nodes call, then "Here are your contacts.") but passes |_| {} for the chunk callback and asserts only response.
• Recommendation: let hold_for_reprompt = reply_before_nudge.is_some() && !any_real_tool_calls;. Add a streaming assertion to that test, or a sibling of a_reply_discarded_after_the_re_prompt_is_not_streamed, that the final answer's tokens reach the caller.
• Principle: A hold needs a release condition as explicit as its entry condition; the two holds in this function should have the same shape.

2. [Important] System messages are not all written from tool results: the conversation summary is the model's
File: packages/agent/src/local_agent/agent_loop.rs:2504-2521 (rule and comment), :5487-5497 (summary), :9180-9212 (tests)

• Problem: The new comment says system messages and tool messages "are written by the system from tool results, never by the model, so an id in either is one a tool produced". Two of the three system messages a session holds fit that: the daemon's completed-writes and looked-up-entities records, and the fixed clarification nudge. The third does not. maybe_summarize_history inserts [Conversation summary]: <text> as Role::System, and that text is a model generation over the drained user, assistant, tool and system messages.
  • An id the user typed, which ungrounded_node_uris_is_not_grounded_by_what_was_said_in_the_chat asserts grounds nothing, grounds once the turn it was typed in has been summarised.
  • An id the summariser miscopies (a UUID through a small model at temperature 0.1) is grounded, and a link to it passes the fabricated-id guard as a dead link.
  • It needs a chat long enough to summarise, so it is narrow. Before this commit the summary grounded nothing, which had the opposite cost: real ids from drained tool results became ungrounded.
• Recommendation: Give the summary prefix a constant and skip a system message that starts with it in grounded_node_uris_from_history, with a test beside the two new ones. If ids from drained turns should stay grounded, collect them from the drained Tool and System messages at summarisation time and append them to the summary message as a system-written line, so the grounded set comes from the system's text and not the model's. Correct the comment either way.
• Principle: The guard's invariant is "an id comes from a tool result or it does not exist". A rule that widens what counts as a tool result should be scoped by provenance, not by role.

3. [Important] Half of the new re-prompt condition has no test that fails without it
File: packages/agent/src/local_agent/agent_loop.rs:3636-3641; tests at :17183, :17243, :16199

• Problem: a_question_nothing_clarified is composed_clarifications.is_empty() && is_lookup_shaped(user_message). in_a_chat_that_only_read_a_question_answered_in_prose_is_not_put_back pins the case where both are true. No test has a composed clarification in the intent, a lookup-shaped message and a prose reply:

  • after_a_composed_clarification_… and a_prose_reply_after_an_answered_clarification_is_put_back_to_act send "just show me what I have" and "I just want to search what I already have", neither of which is lookup-shaped;
  • an_answer_to_a_clarification_is_never_asked_about_alone sends "the client ones?" but Stage 2 acts at once, so the re-prompt is never in question.

  Reducing the condition to is_lookup_shaped(user_message) therefore leaves every unit test green, as far as I can read them, and it would stop re-prompting "the client ones?" answered with a second prose question. That is the case re-review 2's finding 1 was about, on the Stage 2 side.

• Recommendation: Add a test that seeds a Clarified turn, sends "the client ones?", has Stage 2 reply in prose, and asserts the nudge is in the history and the second reply is the one returned.

• Principle: Each conjunct of a guard needs a case that only it decides. This is the mutation check the round-3 summary describes, applied to the other half.

4. [Suggestion] is_lookup_shaped now decides an outcome, and its own doc says it does not
File: packages/agent/src/local_agent/routing.rs:184-201; agent_loop.rs:3629-3641, :2049-2055

• Problem: The function's doc lists two uses and argues that a misread is cheap in both: one short generation, or a list the turn had no use for. 0fa4cc2c adds a third use, and there a misread changes what the user gets. In an intent with two non-writing turns and no composed clarification, "Can you add a task to renew the domain?" or "find and archive the old notes", answered with a prose question, is no longer put back to act. route_clarify is already off that turn's surface (:2975), so the prose question is the reply. The loop is bounded, because the user's answer is rarely lookup-shaped and is re-prompted, and the doc itself names both messages as ones the shape misreads. The rule is also English-only, which mattered less when a miss cost a generation.
  • The doc on session_already_clarified (:2049-2055) still says a tool-free reply is re-prompted once from the third non-writing turn, without the exemption.
• Recommendation: List the third use in the function's doc with its cost. Add "Can you add a task …?" after two read-only turns to eval:decisions or the live tests and record what the model does, since that is the write-shaped case the exemption now covers. Update the session_already_clarified doc.
• Principle: A heuristic documented as safe because it never decides should have its doc and its measurements revisited when it starts deciding.

5. [Suggestion] outcome-follow-up-keeps-its-answer no longer reaches the code it was written for, and its pattern is loose
File: scripts/eval/fixtures/decisions.ts:753-774

• Problem: Two things changed under this scenario.
  • Its prompt, "Can you say that again more simply?", is lookup-shaped. After 0fa4cc2c it is never re-prompted in a chat with no composed clarification, so the keep-first-reply path (reply_before_nudge, the held round, the nudge's removal from history) has no eval coverage. The comment still describes the put-back as one of the two things that can take the reply away.
  • /northwind/i passes "I don't see anything about Northwind Trading in this conversation" and "I searched for Northwind and found nothing". The comment's claim that every failing reply "names nothing" is an assumption about wording.
• Recommendation: Update the comment. Add a sibling scenario with a prompt that is not lookup-shaped ("Say that again more simply.") so the keep-first path is still measured. Consider asserting on tools as well for the lookup-shaped one: the reply is expected with no search of the notes for "say that again".
• Principle: A scenario's comment and pattern should match the path the prompt takes today.

6. [Suggestion] The own-skills fix does not hold in an intent with a composed clarification
File: packages/agent/src/local_agent/agent_loop.rs:3636-3656

• Problem: A Clarified turn stays in the intent until something writes. In that window "Which skills do you have?" is answered from the list, re-prompted (the exemption requires no composed clarification), and the second reply is the one accepted (keep-first requires the same). That is the reported failure for criterion 6, in a narrower setting. The re-prompt's premise is structurally true only for the message that answers the clarification, and route already computes that (answering_a_clarification, :5020-5023).
• Recommendation: Not for this PR unless it is cheap: consider exempting a lookup-shaped message whenever it is not the direct answer to a composed clarification, and measure it. Otherwise record it in ADR-038's accepted costs.
• Principle: Key a rule on the narrowest structural fact that makes its premise true.

7. [Suggestion] The grounding test uses a hand-written copy of the daemon's record
File: packages/agent/src/local_agent/agent_loop.rs:9180-9190; packages/daemon/src/services/local_agent_service.rs:2863-2886

• Problem: The fix works because ResolvedEntity.node_id is a nodespace:// URI and resolved_entities_message writes it verbatim. The agent-side test writes its own record text, so a change to how the daemon renders the record (a bare id, say; CompletedWrite.node_id is already documented as "a URI, or a schema's id") leaves it green while the fix stops working.
• Recommendation: Add a daemon-side test that builds history with node_history_from_messages from a message with a resolved entity and asserts the record contains the entity's URI in the form extract_node_uris reads.
• Principle: When two crates must agree on a format, test the agreement where both sides are visible.

8. [Suggestion] With route_lookup offered, main's type line no longer changes any measured decision
File: packages/agent/tests/it/live_stage1_golden_prompts.rs:868-876

• Problem: The second measurement says both arms decide alike on every case, at 11 more prompt tokens and about 4% more generation time. That is recorded honestly. It also means the line's measured benefit is gone on this test's cases once this PR lands.
• Recommendation: No change here. Raise it with the owner of the type line: either find cases where it still decides, or remove it.
• Principle: YAGNI applies to prompt text as well; a measured cost needs a measured benefit.

Nitpicks

• Nit: agent_loop.rs:5143-5147: the early-return clarify log line has no routing_passes. A clarify is always decided on the blended view, but it can have cost one pass or two.
• Nit: packages/agent/src/agent_guidance.rs:223: "Injected into Stage 2's system prompt on every routed turn" is no longer true after dd9be0aa.
• Nit: live_stage1_golden_prompts.rs:891: "the reps that routed (query or multi)" now includes lookups.
• Nit: ADR-038's new bullet says "where no clarification was composed in the chat"; the code reads the current intent (composed_clarifications stops at the last turn that wrote). "A message that isn't a question" in the accepted costs is is_lookup_shaped, which also covers retrieval verbs. The same "in a chat that never composed" wording is in the code comments at :3629 and :3647.
• Nit: scripts/aichat.ts:416-446: readLogFrom reads the whole file to slice its tail, up to 10 MB per turn where tail -c read only the new bytes. Fine for a harness; Bun.file(path).slice(fromByte) would keep the old cost.

What I checked and found sound

• Clarification-answer guard and two-pass routing. passes counts correctly on every path (first pass used, first pass discarded, first pass errored). A first-pass error still produces no routing_decision line, so the log scraper sees one per turn.
• Dropping route_clarify on a lookup. The call is removed only when system_lookup is Some; the system search is pushed only when nothing else was called; lookup_run is set only then.
• Skill list gating (dd9be0aa). The list sits between the resident prompt and the candidate block, so withholding it does not change the prefix Stage 1 and Stage 2 share. The lookup-block golden still renders it. The gate and the re-prompt exemption use the same predicate, so the turn that is shown the list is the turn whose prose answer stands.
• Log rotation (897b7db3). The daemon rotates to <log>.1 (stdio_log_rotation.rs), which is what readTurnLog reads. The "shorter than the offset" test is sound for one rotation, and the doc says it follows only one. Three tests cover append, rotation and a missing file.
• CLAUDE.md. No issue numbers in the comments or docs these commits add, no lint suppression, no compatibility code, no #[allow]. No node-type, schema or wire-type change, so the node type sequence does not apply.

Docs

Only architecture/prompt-skill-system.md and decisions/038-skill-mediated-tool-routing.md have uncommitted edits in /Users/malibio/nodespace/nodespace-docs; architecture/data-layer.md and development/agent-eval.md are clean, and their committed text already covers this PR (the search exclusion; --only, --list and the log rotation).

• prompt-skill-system.md: the "recognised on the message alone" rule names is_lookup_shaped, the retrieval verbs and the clarification-answer exception. The skill-list paragraph matches dd9be0aa.
• ADR-038: the re-prompt paragraph matches the two branches and the held round. The new bullet matches 0fa4cc2c apart from the wording in the nits.
• Not described anywhere: that a node id is grounded by the system's record of an earlier turn. If the fabricated-id guard is documented in the docs repo I did not find it, so this may be nothing to update.

Requirements check

#	Criterion	Status
1	Research & Search's description carries retrieval and question vocabulary	Met. The description survived the move to main's seed table unchanged.
2	eval:decisions shows it routed for "find …" and question prompts, without regressing other skills	Met on the posted evidence: 18/20 at dd9be0aa with skill routing 7/7, the one failure in this PR's area fixed by 0fa4cc2c and passing in a selected run. The full run on a84fafef was still in progress when the summary was posted; its result is not on the PR.
3	A knowledge question always searches before answering or asking; the zero-tool case is covered	Met, structurally. Retrieval-verb lookups after unrelated turns are now covered too (33 of 33 in the live test, as reported).
4	Explanatory questions read several results and do not offer to read	Met by the guidance, ported intact, and the system-run search. No scenario scores synthesis directly (unchanged).
5	Search excludes ai-chat nodes	Met (unchanged).
6	Questions about the agent's own skills are answered from the skill set	Met for a chat with no composed clarification in the current intent: 0fa4cc2c removes the re-prompt that turned the correct list into a notes search. Suggestion 6 is the remaining case.
7	Prompt goldens regenerated and reviewed	Met. These commits change no model-facing text; the skill list is unchanged, only when it is shown.

Recommendation

COMMENT

The branch is a net improvement and the acceptance criteria are met on the posted evidence. I would approve once finding 1 has its one-line fix and a streaming assertion, finding 2's summary message is excluded or its ids are system-written, and finding 3's test exists. Findings 4 to 8 and the nits are optional. The full eval:decisions result on a84fafef should be posted before merge, since 0fa4cc2c changes when the re-prompt fires for every question-shaped message.

[malibio]

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

[malibio]

Address-review, round 4 (after re-review 3)

Head is 1bdb8768. Two commits answer re-review 3: c1b77896 and 1bdb8768.

Addressed

#	Finding	Change
1	After a re-prompt that produced a call, the final answer was never streamed	hold_for_reprompt now releases once a call has been made (&& !any_real_tool_calls), the same shape as the lookup hold. the_answer_after_a_re_prompt_that_produced_a_call_is_streamed asserts the answer's tokens reach the caller; it fails without the fix.
2	The conversation summary is the model's text, but grounded ids as a system message	grounded_node_uris_from_history skips a system message that opens with CONVERSATION_SUMMARY_PREFIX (now a constant shared with the summariser). The comment no longer says every system message is system-written. ungrounded_node_uris_is_not_grounded_by_the_conversation_summary fails without the fix.
3	Half of the re-prompt exemption had no test that fails without it	after_a_composed_clarification_a_question_shaped_answer_is_still_put_back: a Clarified turn, "the client ones?", a prose reply, and the nudge must be in history. It fails when the condition is reduced to the message's shape.
4	is_lookup_shaped now decides an outcome and its doc said it does not	Its doc lists the third use and what a misread costs there; the session_already_clarified doc and the comment at the re-prompt name the exemption and the question-shaped-write case. The cost is recorded in ADR-038. No new measurement of a question-shaped write after two read-only turns: it is recorded as an accepted cost rather than measured.
5	The follow-up scenario no longer reached keep-first-reply	Comment updated. New sibling outcome-follow-up-statement-keeps-its-answer ("Say that again more simply."), which is not lookup-shaped and takes the put-back path.
7	The grounding test used a hand-written copy of the daemon's record	Daemon test the_record_of_looked_up_entities_carries_their_uris_in_a_system_message builds the record from a tool execution and asserts the role and the URI.
Nits		The early clarify log line carries routing_decided_on and routing_passes; the agent_guidance.rs and live-test comments are corrected; ADR-038 says "current intent" and "shaped like a question or a find request"; readLogFrom reads only the log's tail.

Not changed

• 6 (a question about the agent's own skills inside an intent that composed a clarification can still be re-prompted into a search): recorded in ADR-038's accepted costs. Narrowing the re-prompt to the message that directly answers a clarification is a larger change to the contract than this PR should make unmeasured.
• 8 (with route_lookup offered, the Stage 1 type line changes no measured decision): no change here; for the type line's owner.
• The before/after routing latency asked for under re-review 2's finding 5 is not posted. The log now carries routing_passes, so eval:decisions output can be split by it.

Verification

• 985 agent tests, the daemon suite and 1,333 script tests pass at 1bdb8768. Each of findings 1 to 3 was mutation-checked: the fix reverted, the new test failed, the fix restored.
• eval:decisions, full fixture at a84fafef: 20/22. Skill routing 7/7, questions about stored knowledge 8/8, no blocked setups. The two failures both fail on main:
  • schema-on-menu-company: main at 9dbc8040 fails it 0 of 3 in the state a full run puts it in; this branch 1 of 3.
  • declarative-fact-report-resolved: fails on main.
• eval:routing, full fixture at 1bdb8768: 17/19. Both failures fail on main:
  • indirect-schema-freelance: fails in every run, before this PR included.
  • ambiguous-client-contacts: main at 9dbc8040 fails it 0 of 3, as does this branch (search, then a prose question).
• Selected run at 1bdb8768 (op-own-skills-no-search, outcome-find-own-skill, both follow-up scenarios): 5/5.
• Live Stage 1 and retrieval tests on the tree merged with main: 34 of 34.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: Findings 1 and 2 changed loop behaviour (when a held round is released, which system messages ground an id). The change is small, but it is in the code path the last three rounds each found something in.

[malibio]

Pragmatic Code Review — PR #3480 (issue #3265)

Review Type: Re-Review (4)

Verdict: APPROVE. The three Important findings from re-review 3 are fixed, each with a test that fails without its fix as far as I can read them. I traced every combination of the two holds and found no round that is held and never forwarded, and none forwarded twice. Nothing below blocks the merge. The one finding worth a few minutes before merging is finding 1: the new eval scenario probably does not reach the path its comment says it measures.

Scope: c1b77896 and 1bdb8768 (git diff a84fafef..1bdb8768), the current run_turn code around the holds (agent_loop.rs:3240-3725, :4036-4080), grounded_node_uris_from_history, maybe_summarize_history, scripts/aichat.ts and its tests, the two fixture scenarios, the daemon test, and the uncommitted docs diff in /Users/malibio/nodespace/nodespace-docs.

I ran no builds, tests or evals, as instructed. Statements about what a test pins are from reading the code; I did not revert a fix and watch a test fail.

Previous Review Summary

• Re-review 3 recommendations: 13 (3 Important, 5 Suggestions, 5 nits)
• Addressed: 9 (findings 1, 2, 3, 7 and the five nits)
• Partially addressed: 2 (findings 4 and 5)
• Declined with a stated reason: 2 (findings 6 and 8)
• New findings: 4 Suggestions, 3 nits. None blocking.

Status of each re-review 3 finding

#	Finding	Status	Justification
1	After a re-prompt that produced a call, the final answer was never streamed	Addressed	hold_for_reprompt = reply_before_nudge.is_some() && !any_real_tool_calls (agent_loop.rs:3339). the_answer_after_a_re_prompt_that_produced_a_call_is_streamed (:17346) fails without it: the answer round would be held, held_round_stands false, and the stream would end on the first reply.
2	The conversation summary grounded ids as a system message	Addressed	CONVERSATION_SUMMARY_PREFIX is shared by the summariser (:5518) and the grounding rule (:2526). The test at :9207 fails without the skip. Four older tests assert the literal [Conversation summary], so the prefix is pinned independently of the constant. See new finding 4 for the cost of the option chosen.
3	Half of the re-prompt exemption had no test	Addressed	after_a_composed_clarification_a_question_shaped_answer_is_still_put_back (:17289). With the condition reduced to is_lookup_shaped(user_message), "the client ones?" is exempt, the response is "Which client contacts?" and the engine is called twice, so all three assertions fail.
4	is_lookup_shaped decides an outcome and its doc said it does not	Partially addressed	The doc (routing.rs:187-203), the session_already_clarified doc and the comment at the re-prompt all name the third use and its cost; ADR-038 records it. The measurement of a question-shaped write after two read-only turns was not taken, and the summary says so.
5	The follow-up scenario no longer reached keep-first-reply	Partially addressed	The comment is corrected and a sibling scenario is added. The sibling probably does not reach the put-back either: new finding 1. The loose /northwind/i pattern is unchanged in both.
6	Own-skills fix does not hold inside an intent with a composed clarification	Declined, recorded	ADR-038's new bullet states it as an accepted cost. Reasonable for this PR.
7	The grounding test used a hand-written copy of the daemon's record	Addressed	the_record_of_looked_up_entities_carries_their_uris_in_a_system_message (local_agent_service.rs:6001) goes through resolved_entities_from and resolved_entities_message and asserts the role and the URI. It uses contains rather than the agent's extractor, which is private to the agent crate; - nodespace://<id> "title" ends the id at a space, which extract_node_uris handles.
8	The Stage 1 type line changes no measured decision	Declined	Re-review 3 asked for no change here.
Nit	Early clarify log line had no routing_passes	Addressed	:5171-5172. The scraper matches only routing_decision=, so the added fields do not disturb it.
Nit	agent_guidance.rs comment	Addressed
Nit	Live-test comment "query or multi"	Addressed
Nit	"in the chat" where the code reads the current intent	Mostly addressed	ADR-038 and the comment at :3647 are corrected. Two code comments and one fixture comment keep the old wording (nit below).
Nit	readLogFrom read the whole file	Addressed	See "What I checked" below.

The holds: paths traced

any_real_tool_calls is set at :4057, immediately after the tool-call assistant message is pushed, with no continue, break or return between the forwarding decision at :3628 and that line for a round that has calls. The only continue in the body is the re-prompt at :3693. With that:

Turn shape	Round	Held by	Outcome
Not a lookup, re-prompted, second reply is prose	2	re-prompt	Not forwarded; first reply returned; nudge popped.
Not a lookup, re-prompted, second round calls a tool	2	re-prompt	Replayed once. any_real_tool_calls becomes true.
Same, rounds after the call	3+	nothing	Streamed live; held_round_stands is false (hold_for_lookup also needs !any_real_tool_calls), so no replay.
Lookup, search tool on the surface, first round is prose	1	lookup	System search pushed, lookup_run true, round not forwarded. Later rounds stream live. The re-prompt cannot fire: tool_calls is no longer empty.
Lookup, first round calls a tool	1	lookup	Replayed once. Later rounds stream live.
Lookup, no search tool, first round is prose	1	lookup	Stands, replayed once, then may be re-prompted.
Same, round after the re-prompt	2	both	The if hold_for_reprompt arm decides: a call is replayed once; prose is not forwarded and the first reply (already shown) is returned.
Lookup, no search tool, composed clarification, two prose rounds	1, 2	lookup	Each replayed once. Same as the non-lookup composed path, where both stream live.

The re-prompt requires !any_real_tool_calls, so the round directly after it is always held, and the new release only affects rounds after a call. A round is forwarded either live or by replay, never both, because held_round_stands can only be true when hold_stream was. The duplicate-call and malformed-call breaks leave reply_before_nudge unconsumed, which is harmless: nothing reads it after the loop.

New Findings

1. [Suggestion] The new eval scenario probably does not reach the put-back its comment describes
File: scripts/eval/fixtures/decisions.ts:776-788 (and :1390); packages/agent/src/local_agent/agent_loop.rs:2062-2071

• Problem: The comment says "This one is put back to act when the reply makes no call". The put-back needs already_clarified, which is a composed clarification or two non-writing turns since the last write. Each scenario runs in its own chat as [...SETUP, scenario], and the two setup turns call create_schema. When they write, the intent holds one turn (the single priorTurns entry) and the re-prompt cannot fire. When they do not write (the types already exist from an earlier group), it depends on how those two turns ended: two prose replies reach keep-first, a composed clarification reaches the other branch. Which of these happens is decided by the model and the database state, not by the fixture.
  • The pattern passes on every path, and the harness does not report whether a re-prompt fired (formatTurnLogLines reads neither "Clarification contract" log line). So the 5/5 selected run does not show that keep-first was exercised.
  • I did not run it, so I can't say which path the selected run took. The measurement recorded in the first scenario's comment implies the re-prompt did fire there once, which fits setup turns that did not write.
• Recommendation: Add a second non-writing prior turn so the precondition holds when the setup turns write. Have formatTurnLogLines emit a marker for the two "Clarification contract" lines, so a run shows which path was taken and the scenario can assert it. Reword the comment until then ("is put back when the intent holds two turns that did not write").
• Principle: A scenario should establish its own precondition and expose the path it took. Otherwise a pass says nothing about the code it was added for.

2. [Suggestion] The streaming test would pass if the answer were forwarded twice
File: packages/agent/src/local_agent/agent_loop.rs:17388-17396

• Problem: The assertion is ends_with("Here are your contacts."). It pins "the answer reaches the caller" but not "once": a stream of the answer twice passes. The fix moves a round from the replay path to the live path, so a double forward is the regression most likely to follow from a later change to these conditions.
• Recommendation: Assert the whole token stream: "Could you tell me which list they are in?Here are your contacts.". That also pins that the first reply was shown and the tool round added no text.
• Principle: When two paths can each deliver the same output, assert the exact output, so the test fails in both directions.

3. [Suggestion] The grounding sentences were inserted in the middle of the skill-list paragraph
File: /Users/malibio/nodespace/nodespace-docs/architecture/prompt-skill-system.md:170 (uncommitted)

• Problem: The paragraph now ends: "The list is the one Stage 1 carries. A node id in a reply is checked against the ids tools have produced in the session … The conversation summary is the model's text and grounds nothing. It is withheld from a model the routing probe found unsafe for prompt injection, like the candidate block." "It" was the skill list; it now reads as the conversation summary. The two grounding sentences are also about a different mechanism from the paragraph's heading.
• Recommendation: Move the two grounding sentences to their own paragraph and leave "It is withheld…" directly after "The list is the one Stage 1 carries."
• Principle: Docs describe the design; a sentence whose subject changed states something false.

4. [Suggestion] Excluding the summary also drops real ids, and the comment says otherwise
File: packages/agent/src/local_agent/agent_loop.rs:2516-2520, :5446

• Problem: The comment says an id in the summary "grounds nothing, as when it was still in those turns". That is true of an id the user typed or the model wrote. It is not true of an id in a drained tool result or a drained record of lookups and writes: those were grounded before the summarisation and are not after it. In a chat long enough to summarise, a follow-up that links a node from a summarised turn is replaced with the request to confirm, which is the failure the system-record rule was added to stop. This is the behaviour before this PR for tool messages, and the safe direction for the guard, so it is a fair choice. It is not written down as one.
• Recommendation: Either collect the ids from the drained Tool and System messages at :5446 and push them as a separate system-written message beside the summary (re-review 3's second option), or state the cost in the comment and in the docs sentence. Correct "as when it was still in those turns" either way.
• Principle: A guard's known false-positive case belongs next to the rule that causes it.

Nitpicks

• Nit: agent_loop.rs:3672-3674 and :3696-3698 still say "a chat that never composed a clarification" and "a chat that has only asked questions"; decisions.ts:761-762 says "a chat that asked no clarifying question". The code reads the current intent.
• Nit: scripts/aichat.ts:438-451: one readSync call; a short read returns a truncated tail without notice. Rare on a regular file and fine for a harness. A loop until length bytes or 0 would remove the case.
• Nit: /northwind/i is still the pattern in both follow-up scenarios; re-review 3's point that it passes "I searched for Northwind and found nothing" stands.

What I checked and found sound

• readLogFrom. A fromByte past the end gives a zero-length buffer and an empty string, which is what the rotated case needs when <log>.1 is shorter than the offset. A missing file throws in openSync and is caught. The descriptor is closed in finally. A file that grows between fstatSync and readSync is read up to the measured length. The three existing tests cover a non-zero offset, a rotation and a missing file, and pass with either implementation, which is right for a change that only alters cost.
• grounded_node_uris_from_history. The three system messages a session can hold are the daemon's two records, the fixed nudge and the summary. The summariser's fallback text carries no ids. A second summarisation re-summarises the first summary under the same prefix.
• Log fields. decided_on and passes are in scope at the early return.
• ADR-038. The new bullet and the reworded accepted costs match the code: the exemption reads the current intent and is_lookup_shaped, and both accepted costs are the ones the code has.
• development/agent-eval.md has no uncommitted edit and needs none: it does not list scenarios or counts.
• CLAUDE.md. No issue numbers in the comments or docs these commits add, no lint suppression, no #[allow], no compatibility code, no new .md files in the repo. No node type, schema or wire type is touched.

Recommendation

APPROVE

No finding is blocking. Findings 1 to 4 are small and local; 1 and 3 are the two I would do before merging, since 1 decides whether the new scenario measures anything and 3 is a sentence in the docs that now says the wrong thing.

[malibio]

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

[malibio]

Address-review, round 5 (after re-review 4: APPROVE)

Head is 09c8f35f. One commit answers re-review 4's four suggestions. It changes a test assertion, comments, an eval fixture and docs; no production logic.

#	Suggestion	Change
1	The new eval scenario did not reach the put-back its comment describes	Confirmed: the earlier selected run's daemon log shows no re-prompt. outcome-follow-up-statement-keeps-its-answer now has two questions ahead of the follow-up, so the put-back's precondition (two turns that wrote nothing) holds in a chat whose setup turns write. Run at 09c8f35f's build: pass, and the log shows both "re-prompting to act" and "the re-prompt produced no call — keeping the first reply".
2	The streaming test would pass if the answer were forwarded twice	It now asserts the whole stream: the first reply, then the answer, each once.
3	The grounding sentences split the skill-list paragraph in the architecture doc	Moved to their own paragraph; "It is withheld…" follows the skill list again.
4	Excluding the summary also drops real ids, and the comment said otherwise	The comment and the doc state the cost: once a long chat is summarized, a reply that links a node from a summarized turn is refused. Collecting the drained ids into a system-written line is not done here.
Nits	"a chat that never composed…" where the code reads the current intent	Corrected in the two code comments and the fixture comment.

Not changed: the harness does not print a marker for the two "Clarification contract" log lines, so a scenario cannot assert which path it took; the /northwind/i pattern; the single readSync in readLogFrom.

Verification

• bun run test:changed passed at 1bdb8768 (scripts, skill package, generated-type drift, node-types check, Rust workspace). At 09c8f35f: 916 agent lib tests and 1,333 script tests pass, scripts lint clean.
• Eval numbers are those in the round-4 summary: eval:decisions 20/22 and eval:routing 17/19, with every failing scenario also failing on main.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: Re-review 4 approved with nothing blocking. The one commit since changes no production logic: a tighter test assertion, comments, one eval fixture's prior turns, and docs.

[malibio]

Merge queue: #3480 was taken out of the queue — it conflicts with main in: packages/agent/src/local_agent/agent_loop.rs, packages/agent/src/local_agent/tools.rs, packages/agent/src/skill_pipeline.rs, packages/agent/tests/golden/prompt_assembly/stage1_request.golden, packages/agent/tests/it/live_stage1_golden_prompts.rs, packages/agent/tests/it/prompt_assembly_snapshot.rs, packages/core/src/services/embedding_service.rs, scripts/eval/fixtures/decisions.test.ts, scripts/eval/fixtures/decisions.ts. Rebase onto origin/main, push, and re-run bun run merge 3480.