[malibio]

Closes #1980

What changed

Stage 1's prompt now names the record types the user's message names, next to the skills it already names:

• Built-in types: text, date, task, project, person, collection, query, chosen from the core type registry through an exhaustive match, so a new core type has to be placed on one side.
• The user's types: the display name of every stored non-core schema, read through a new AgentToolExecutor::user_type_names, the way skill names are read.

A type from either list reaches the prompt only when the message names it (whole word, singular or regular plural; "people" names person), at most eight. A message naming no type gets the same prompt as before.

What differs from the issue

• The issue proposes reusing the semantic schema retrieval. Stage 1 doesn't use it: the retrieval's similarity floor is permissive, so its five slots are full on every turn whatever the message is about, and the lexical backstop beside it only matches exact tokens. Matching every stored type name against the message is deterministic and needs no embedding search. The prompt stays bounded either way.
• The issue asks for the built-in names to be in the prompt always. Measured, that left Stage 1's decision alone but reworded its query on 7 of 17 unrelated requests ("log production incidents as a task or collection record"), and retrieval embeds that query. Listing only the types the message names removes that.
• Stage 1 already names the skills, and that alone routes the issue's literal case: "Could you tell me how many tasks we have here?" routes on main. Its siblings did not ("how many people do we have?" clarified every time), and those are what this fixes.

Measurement

Locked model (Gemma 4 E4B), the type line the only difference between arms, four reps per case, every rep of a case deciding alike.

	without the line	with the line
Requests about records of a known type that routed	4 of 7	7 of 7
Type-naming requests never in doubt, decided as expected	8 of 8	8 of 8
Mean Stage-1 prompt	792 tokens	803 tokens
Mean Stage-1 generation, 36 runs where both arms decided alike	2313 ms	2397 ms

• Moved from clarify to route: "how many people do we have?", "what projects are there?", "tell me about the vendors".
• The eight never-in-doubt requests produced the same queries in both arms. They include the routing eval's task and compound prompts and a type's name used as an ordinary word ("find the text about onboarding in my notes"). The test fails if a query gains a type word its message lacks.
• Requests with no subject ("fix it", "delete them") name no type, so their prompt and their clarification are unchanged by construction; a unit test pins that for twelve of them.

Accepted costs:

• A request that refers to a type without naming it in full ("do we have any incidents?" for an Incident Report type) gets no line and behaves as before.
• "what's the date today?" names date. Without the line it clarified; with it Stage 1 called no routing tool, which the loop treats as a query on the raw message.

The measurement is live_stage1_golden_prompts::stage1_type_line_routes_requests_that_name_a_known_type (ignored by default; needs the GGUF).

Acceptance criteria

• Stage 1's prompt includes built-in type names (when the message names them; see above)
• Stage 1's prompt includes the user's schema names (when the message names them; by word match on the stored schemas, not the semantic retrieval; see above)
• The "how many tasks" case routes (asserted in the live test)
• Cost measured and documented: 11 prompt tokens, no retrieval, under 4% generation time. The ADR-038 and prompt/skill-system doc updates land in nodespace-docs with the merge
• route_clarify still fires for genuinely vague requests (their prompt is unchanged)
• bun run test:changed passes (Rust and scripts tiers)
• bun run quality:fix clean

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3493 (issue #1980)

Review Type: Initial Review (no prior reviews or comments on the PR; one commit, 093ec052)

Summary

The change is small, well-bounded and follows the pattern the skill-name line already set. I found no defect in the code. The findings are about how far the measurement supports the claims made from it, and about three doc comments that say more than the code does.

I checked the PR's central premise against the code and it holds: in run_ai_chat_turn the workspace context (schema retrieval included) is built at local_agent_service.rs:861, before create_session at :870 and therefore before route(). The issue's "route() runs before workspace_context is assembled" no longer describes the code, so threading the names through the session, with no second retrieval call, is the right reading of the acceptance criteria.

Not run: the live model test (as instructed) and the unit tests. Everything below is from reading the code, the diff and the docs.

Requirements Check

#	Criterion		Notes
1	Stage 1's prompt includes built-in type names	✅	text, date, task, project, person, collection, query, taken from CoreNodeType::ALL through an exhaustive match; pinned by stage1_request.golden.
2	Stage 1's prompt includes semantically retrieved schema names, from the existing bounded retrieval	✅	WorkspaceContext::relevant_type_names() → TurnContext → AgentSession.relevant_type_names → stage1_type_names. It reuses the one semantic_search_nodes_of_type call (MAX_SEMANTIC_SCHEMAS, SCHEMA_SIMILARITY_THRESHOLD). The list also carries the lexical-backstop and recently-created schemas, capped at 8.
3	"Could you tell me how many tasks we have here?" routes	✅	Asserted on every rep in the live test (how_many_tasks_routed[1] == REPS). The PR says this case already routed 3/3 on main, so it is a guard, not the thing fixed.
4	Latency / turn-count cost measured and documented	✅ with a caveat	No added turn or retrieval (verified). About 25 prompt tokens. Documented in the pending ADR-038 and prompt-skill-system.md edits. The latency number does not isolate the line's cost: see finding 3.
5	route_clarify still fires for genuinely vague requests	✅ with a caveat	12 of 12 as reported, but the test only asserts "no worse than the other arm", and the controls carry at most two retrieved names: see findings 1 and 2.
6	Tests pass	✅ as reported	bun run test:changed per the PR body; not re-run here. The merge gate is the full run.
7	bun run quality:fix clean	✅ as reported	Not re-run here.

Code Review Findings

🟡 Important

1. The measurement covers type requests and vague controls, but not requests that have nothing to do with types
📁 packages/agent/tests/it/live_stage1_golden_prompts.rs:340-525

• Problem: The 20 cases are 8 requests built on a type and 12 with no subject. Nothing measures whether the new line changes Stage 1's decision on ordinary requests. ADR-064 ("Stage 1 and the word 'types'") records that exact failure for the last Stage-1 addition: a lever that scored 3 of 3 with 6 of 6 controls then took the routing eval from 18 of 19 to 16 of 19, and concludes that a new name "changes Stage 1's decision on requests that have nothing to do with it".
• Also: production can put up to 8 retrieved names on the line (STAGE1_RETRIEVED_TYPES_MAX), and with the threshold at 0.2 a workspace with several schemas will usually fill the 5 semantic slots whatever the message says. The with-types controls carry one or two names.
• Recommendation: Before merge, run the routing eval and live_stage1_compound_intent once with the line and record the result next to the existing numbers. Add two or three control cases with 5 to 8 unrelated retrieved names.
• Principle: A measurement should cover the failure mode the project has already seen, at the bounds production reaches.

2. The live test's assertions would pass on a regression in both arms
📁 packages/agent/tests/it/live_stage1_golden_prompts.rs:514-525

• Problem: Two of the three assertions are relative (with >= without). If the model or the base prompt drifts so that the vague controls clarify 6 of 12 in both arms, or the routed count falls back to 3 of 8 in both, the test passes. The doc comment says it asserts "that the controls did not move", which is true only relative to the other arm. The 6 of 8 and 12 of 12 in the doc comment, the PR body and ADR-038 are printed, not asserted.
• Recommendation: Assert the controls absolutely in the with-types arm (as_expected[Clarify][1] == total(Expect::Clarify)), and assert the three cases named as moved, the way HOW_MANY_TASKS is. Keep the relative assertions as the arm comparison.
• Principle: A test should fail when the behaviour it documents stops being true.

3. "No measurable latency" is not what the timing shows
📁 packages/agent/src/local_agent/agent_loop.rs:85-86, packages/agent/tests/it/live_stage1_golden_prompts.rs:334-337, and the pending ADR-038 text

• Problem: The per-arm mean is wall-clock over whole generations, and the arms do not generate the same output. A route_clarify call (a question plus options) is longer than a route_query call, and the with-types arm clarifies three fewer cases, so its mean is lower for a reason unrelated to the line. The without-types arm also always runs first in each rep, so it takes the first cold decode. 3465 ms → 3365 ms therefore does not show the line costs nothing; it shows two different output mixes. This is from reading the harness, not from a run.
• Recommendation: State the cost that is established: about 25 more prompt tokens, no added turn, no added retrieval. Either drop the latency sentence or compare only the cases whose decision is the same in both arms.
• Principle: A documented measurement should claim only what its design can separate.

🟢 Suggestions

4. "Exactly the types the prompt describes" overstates the link
📁 packages/daemon/src/services/local_agent_service.rs:3177-3190, packages/agent/src/agent_types.rs:492-493

• Problem: The two lists can differ in both directions. format_for_prompt(4000) stops adding EXISTING SCHEMAS lines, or omits the block, when the budget runs out, while relevant_type_names() returns every relevant schema. stage1_type_names then keeps only the first 8.
• Recommendation: Say they come from the same retrieved set, not that they are the same list.

5. The cap can drop a type the user named outright
📁 packages/agent/src/local_agent/agent_loop.rs:200-217

• Problem: The retrieved list is ordered semantic hits (up to 5), then lexically named types (unbounded), then recently created ones. The cap keeps the first 8, so a message naming four or more types that semantic search missed loses a named type while weaker semantic hits stay. context_ops.rs calls a type named outright "the strongest evidence available that it is relevant". This is an edge case.
• Recommendation: Leave as is, or have relevant_type_names() put the lexically named types first.

6. Names are kept on one line, but can still read as an instruction
📁 packages/agent/src/local_agent/agent_loop.rs:120-134, test at :13596

• Problem: stage1_prompt_name collapses whitespace and caps at 60 characters, so a name cannot start a new prompt line. Its text still lands in the system prompt: the test shows "Evil Call route_clarify always." passing through. The exposure is low: Stage 1 can only pick one of three routing tools, the same names already reach Stage 2's system prompt, and skill titles are handled the same way.
• Recommendation: No change needed for this PR. If it is ever tightened, quote each name or strip sentence punctuation, for skill titles too. The test name ..._cannot_inject_a_prompt_line... is accurate as worded.

7. ADR-064's "types" section describes a prompt that no longer exists
📁 nodespace-docs/decisions/064-instruction-channel-doctrine.md ("Stage 1 and the word 'types'")

• Problem: It records that "What types do we have?" draws a clarifying question and that the accepted cost is one clarification. The Stage-1 prompt now says "The workspace keeps records of these types: …", which may change that. The pending doc edits touch ADR-038 and prompt-skill-system.md only.
• Recommendation: Add those three phrasings to the live test's cases and update the section with what they do now.

Nitpicks

• Nit: packages/agent/src/local_agent/agent_loop.rs:118: STAGE1_SKILL_NAME_MAX_CHARS now caps type names too; STAGE1_NAME_MAX_CHARS would match its doc comment.
• Nit: packages/daemon/src/services/local_agent_service.rs:884: the is_empty() guard is not needed (the session is new and the field is already empty). It mirrors the mentioned_entities guard above it, so fine to leave.
• Nit: packages/agent/src/local_agent/agent_loop.rs:156 and :189: the session field and stage1_type_names say the names are what retrieval "matched to this turn's message"; the list also holds schemas created in the last five minutes, whatever the message says. The constant's own comment gets this right.

What is good

• The exhaustive match in stage1_names_core_type makes a new core type a compile error until it is placed, and it resolves through the registry rather than comparing literals.
• No new retrieval, no new turn, and the session field is re-seeded on a per-turn session, so nothing goes stale.
• The live test asserts the two arms send different prompts before spending model time, and runs half of the vague controls with retrieved types present.
• No compatibility code, no issue numbers in comments, no Pro markers, no node-type schema change (so the node type sequence does not apply).

Recommendation

APPROVE

The code is correct and a net improvement. Findings 1 to 3 are worth doing before merge: they decide whether the numbers written into ADR-038 can be relied on, and finding 1 is the check that caught the previous Stage-1 change.

[malibio]

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

[malibio]

Review addressed (commit ad104f31)

Finding 1 changed the design, so this needs a re-review.

What changed, and why

Finding 1 asked for requests unrelated to any type. Measuring them showed the type line left Stage 1's decision alone but reworded its query, which retrieval embeds:

What the prompt listed	Decisions on 17 unrelated requests	Queries reworded with a type name
every built-in type and five unrelated retrieved types	unchanged	"find what I wrote about rate limiting last year" became "… in Feature Writeup"; Research & Search fell out of the top three
every built-in type, retrieved types only when named	unchanged	7 of 17, e.g. "log production incidents as a task or collection record"; of 11 reworded queries, 2 lost the skill they needed and 2 gained it
only the types the message names (shipped)	prompt identical to the one without the line	none

So Stage 1 is now told of a type, built-in or retrieved, only when the message names it (whole word, singular or plural; "people" names person). A message naming no type gets the prompt it had before this PR. This departs from the issue's wording ("the prompt includes built-in type names"): they are included when the message uses them, which is the only case where they can resolve an ambiguity.

Measurement with the shipped rule

Locked model, three reps per case, every rep agreeing. 12 of 40 cases name a type and were run; the other 28 send an identical prompt in both arms.

• Requests naming a known type: 4 of 7 routed without the line, 7 of 7 with it. "how many people do we have?", "what projects are there?" and "tell me about the vendors" moved from clarify to route.
• Type-naming requests never in doubt (5, including the routing eval's task and compound prompts): same decision in both arms, same query but for a dropped word.
• Prompt: 792 → 804 tokens on a turn that has the line. Generation: 2419 ms → 2440 ms over the 27 runs where both arms decided alike.
• live_stage1_compound_intent with the line: 11 of 11 route_multi, 10 faithful and 1 that dropped an intent. I have no without-line run of that harness to compare against.

By finding

#	Finding	Outcome
1	Measurement misses unrelated requests and long type lines	Addressed. The routing eval's single-turn prompts are cases in the live test; the result is the redesign above. A long line of unrelated names can no longer occur. I did not run the daemon-based routing eval: for the 13 of its prompts that name no type, the Stage-1 prompt is byte-identical to main's.
2	Assertions only relative	Addressed. The no-subject cases are asserted to leave the prompt unchanged (also a unit test, a_message_naming_no_type_gets_no_type_line); the moved cases are asserted to route on every rep.
3	Latency claim unsupported	Addressed. Arms alternate order; latency is compared only over runs where both arms made the same decision.
4	"Exactly the types the prompt describes"	Addressed. Comments reworded.
5	Cap can drop a named type	Moot. Only named types are listed, and the cap (8) applies to them.
6	A name can still read as an instruction	Skipped, as the review recommends. Exposure is narrower now: a name reaches the prompt only if the user's own message contains its words.
7	ADR-064's "types" section may be stale	No change needed. "What types do we have?" names no type, so its prompt is unchanged and the section still describes it.
nit	STAGE1_SKILL_NAME_MAX_CHARS	Renamed to STAGE1_NAME_MAX_CHARS.
nit	is_empty() guard in the daemon	Left; it mirrors the guard above it.
nit	"matched to this turn's message"	Comments corrected to include recently created schemas.

Accepted cost

A request that refers to a type without naming it ("do we have any incidents?" for an Incident Report type) gets no line and behaves as it did before.

Checks

bun run test:changed passes (Rust and scripts tiers); bun run quality:fix clean.

Re-review: needed (the type-selection rule and its tests are new code).

[malibio]

Pragmatic Code Review: PR #3493 (issue #1980)

Review Type: Re-Review (of commit ad104f31, against the initial review of 093ec052)

Summary

The second commit replaces "list every built-in type and every retrieved type" with "list only the types the message names". The rule is implemented correctly, it is simpler to reason about than the first design, and a message that names no type now provably gets the prompt it has on main. I found no defect in the code.

What I found is in three places: two written claims that say more than the code does, one gap between the rule's matcher and the set it is applied to, and tests that do not yet guard the failure the redesign was built around (a type name leaking into Stage 1's query).

How this was checked:

• Read git diff 093ec052..ad104f31, the matcher and its helpers, the candidate-set construction in context_ops.rs, and the pending edits to ADR-038 and prompt-skill-system.md.
• Ran 21 targeted tests, all passing: the stage1 and user_message_names_type unit tests, a_message_naming_no_type_gets_no_type_line, and prompt_assembly_snapshot::stage1_request_matches_golden.
• Not run: the live model tests (as instructed), the full suite, and the daemon routing eval. The measured numbers below are the author's; I checked only that they are consistent with the case table and the harness.

Previous Review Summary

	Count
Previous recommendations	10 (7 findings, 3 nits)
Addressed	6 (findings 1 to 4, the constant rename, the "matched to this turn's message" comments)
Moot after the redesign	1 (finding 5: the cap now applies only to named types)
Not addressed, with a reason that holds	3 (finding 6, as the review recommended; finding 7; the is_empty() guard nit)
New issues found	5 findings, 5 nits

Notes on the ones I checked independently:

• Finding 1 (unrelated requests): addressed by the redesign. The routing eval's single-turn prompts are now cases in the live test. The daemon routing eval was not run; the reason given holds for the prompts that name no type (their Stage-1 prompt is the one on main), and the three that name task are measured at Stage 1 in the live test.
• Finding 2 (relative assertions): addressed for the moved cases, which are now asserted to route on every rep. Partly open for the rest: see finding 4 below.
• Finding 3 (latency): addressed. Arms alternate and latency is compared only over runs that decided alike. The 27 is consistent: 12 cases by 3 reps, less the 9 runs of the 3 moved cases.
• Finding 7 (ADR-064's "types" section): no change needed. "What types do we have?" and "which types exist in this workspace" contain none of the built-in words, so their prompt is unchanged.

Requirements Check

#	Criterion		Notes
1	Stage 1's prompt includes built-in type names	✅ with a stated deviation	Included only when the message uses the word. The deviation is justified by what the first commit measured: listing them always reworded Stage 1's query on unrelated requests, and the two partial-name cases that the stricter rule now drops ("do we have any incidents?", "what's going on with the writeups?") did not change decision with the line in the first measurement either. I could not reproduce the rejected arms: see finding 5.
2	Stage 1's prompt includes semantically retrieved schema names, from the existing bounded retrieval	✅ in a narrower form	The candidates still come from the one existing retrieval, with no added call. But the filter is lexical, so semantic retrieval now only decides which named types are candidates; a type the message refers to without naming is dropped, which the PR states as its accepted cost. See finding 2 for where the two interact.
3	"Could you tell me how many tasks we have here?" routes	✅	Asserted on every rep in the with-line arm.
4	Latency / turn-count cost measured and documented	✅	No added turn or retrieval (verified in the code). About 12 prompt tokens on a turn that has the line. The latency comparison is now sound.
5	route_clarify still fires for genuinely vague requests	✅ by construction	A no-subject message names no type, so its prompt is unchanged from main. This is a stronger guarantee than a measurement, but no test in this PR runs a vague request against the model any more: see finding 4.
6	Tests pass	✅ as reported	The 21 targeted tests I ran pass. The merge gate is the full run.
7	bun run quality:fix clean	✅ as reported	Not re-run.

Code Review Findings

🟡 Important

1. Two written claims do not match the case table
📁 nodespace-docs/decisions/038-skill-mediated-tool-routing.md:196-204 (pending edit), packages/agent/tests/it/live_stage1_golden_prompts.rs:620-628, and the PR body

• Problem (a): ADR-038's table says that with the shipped rule, on the "17 requests unrelated to any type", the prompt is "identical to the one without the line", with no reworded queries. Three of the 17 routing-eval prompts in the test's table contain the word "task" ("Create a new task called …", "Add a task to rotate …", "Create a task to move …"), so under the shipped rule they do get the line. The next paragraph of the ADR counts them among the five type-naming requests, and the test's doc comment says their query changed "but for a dropped word", while the ADR says "with the same query". This assumes the 17 are the routing-eval prompts in the test table, which is what the test's comment says.
• Problem (b): TYPE_LINE_CASES has 41 entries, not 40 (10 known-type, 12 no-subject, 19 unrelated). Twelve name a type, so 29 are unchanged, not 28. The doc comment and the PR body both say "forty" and "twenty-eight".
• Recommendation: In the ADR, make the third row say 14 of the 17 get an identical prompt and 3 get the line with the same decision, and use one wording for the query difference. Correct 40 and 28 to 41 and 29.
• Principle: A recorded measurement is read later as fact; it should state exactly what was run.

2. The rule's matcher is looser than the matcher that builds its candidate set
📁 packages/agent/src/local_agent/agent_loop.rs:234-237, packages/core/src/ops/context_ops.rs:549-569, packages/core/src/ops/skill_ops.rs:151-169

• Problem: stage1_type_names tests each candidate with user_message_names_type, which accepts plurals and words in any order. The candidates are relevant_type_names: the semantic top five, plus types the lexical backstop found, plus recently created ones. The lexical backstop uses mentions_phrase, which needs the exact token ("vendor" does not match "vendors"). So a user type named in the plural reaches Stage 1 only if semantic retrieval ranks it in its five slots. When it does not (no embedding service, a type older than the recent-creation window whose embedding is missing, or five other schemas scoring higher on a history-blended query), "tell me about the vendors" gets no line, although the message names the type by the rule's own definition.
• Verified: the code path, by reading both matchers. Not measured: how often a named type misses the semantic top five; I expect it is rare when the word is in the query.
• Recommendation: Either state this in the doc comment and ADR ("of the types the turn's context holds"), or apply user_message_names_type to the schema corpus the context build already loads, so that one matcher answers "does the message name this type" in both places.
• Principle: One question should have one answer; two matchers with different tolerances for the same question drift apart silently.

3. "Singular or plural" covers -s and -es only
📁 packages/agent/src/local_agent/agent_loop.rs:1653-1662, the PR body, and ADR-038 ("or an irregular plural")

• Problem: words_match_modulo_plural does not pair "company" with "companies" (its own comment says -ies is not recognised). That was a deliberate limit for its first use, the unrequested-schema guard. For Stage 1 it means "how many companies do we have?" gets no line for a Company type, and the same for Category, Story, Activity, Policy, Opportunity and Entry. Those are regular plurals, so the ADR's "irregular plural" does not describe them, and count and list requests are usually phrased in the plural.
• Recommendation: Add the y / ies pair to words_match_modulo_plural with a unit test (it helps both callers), or name the limit in the ADR's accepted cost.
• Principle: A helper reused for a second purpose should be re-checked against that purpose's inputs.

🟢 Suggestions

4. The tests do not guard the failure the redesign exists to prevent
📁 packages/agent/tests/it/live_stage1_golden_prompts.rs:630-768, packages/agent/src/local_agent/agent_loop.rs:13643-13675

• Query rewording is printed, not asserted. The reason for this commit is that a type name in the prompt reaches Stage 1's query. For the 12 cases that still get a line, nothing fails if the query gains a type word. The Unrelated assertion is met[1] >= met[0] on the decision only, so it also passes when both arms miss the expectation.
• Built-in words used in their English sense are not in the table. "date", "text", "query", "project", "people" and "collection" are ordinary words. "what's the date today?" or "summarise the text I pasted" now gets a type line (verified by reading the matcher), and this is the one remaining group of unrelated requests whose prompt the PR changes. Whether the line changes their decision or query is unmeasured; this part is a suspicion, not a finding.
• The no-subject guarantee is only partly in the gate. The assertion that all 12 no-subject cases leave the prompt unchanged needs no model, but it sits after load_engine() in an ignored test. The unit test that the gate runs covers 4 of the 12.
• The test name says more than the test does. ..._keep_vague_requests_clarifying never runs a vague request.
• Recommendation: Move the 12 no-subject messages into a_message_naming_no_type_gets_no_type_line (or share the table). Add two or three English-sense cases to the live table. For Unrelated cases, assert that the with-line query contains no type name the message lacks.
• Principle: A test should fail when the behaviour it documents stops being true.

5. The evidence for the rejected designs is not reproducible from the repository
📁 packages/agent/src/local_agent/agent_loop.rs:206-215, ADR-038 table rows 1 and 2

• Problem: The doc comment and the ADR cite measurements of two arms (every type listed; built-in types always listed) that the committed test can no longer run, because stage1_type_names has only the shipped rule. The deviation from the issue's wording rests on those numbers.
• Recommendation: Acceptable as an ADR record. If it is cheap, keep one live case that builds the always-listed prompt by hand and prints its query, so the claim can be re-checked on a new model.

Nitpicks

• Nit: packages/agent/src/local_agent/agent_loop.rs:156-157: the comment says the first word "is its name in the prompt", but the code renders core_type.as_str() and never reads the first word. They agree today; say so, or use the word.
• Nit: packages/agent/tests/it/live_stage1_golden_prompts.rs:315: Group::KnownType is described as "names a built-in type or one schema retrieval matched", but three of its ten cases name none by the shipped rule ("is there anything for today?", the incidents and writeups cases) and are reported as unchanged.
• Nit: packages/agent/tests/it/live_stage1_golden_prompts.rs:664: with three reps the arm order is 2 to 1, not balanced. An even rep count would balance it.
• Nit: packages/agent/src/local_agent/agent_loop.rs:1589-1594: single-character words are dropped from a name, so a type named "Type 1" or "Plan B" is named by any message containing "type" or "plan", and "Q&A" can never be named. The cost is one extra or one missing name.
• Nit: packages/agent/src/local_agent/agent_loop.rs:224-237: the message is tokenised once per candidate word. Negligible next to the model call; tokenise once if this is touched again.

Security and performance

• Prompt injection: narrower than before. A user-editable type name reaches the system prompt only when every word of it is in the user's own message, whitespace is still collapsed, and the list is capped at 8 names of 60 characters. Stage 1 can still only call one of three routing tools. No change needed.
• Performance: no added retrieval, turn or I/O. The matcher is linear in the message length per candidate.
• Project rules: no literal node_type comparison (the built-in list is an exhaustive match on CoreNodeType), no compatibility code, no issue numbers in comments, no lint suppression, no new docs in this repo.

What is good

• The no-type case is identical to main by construction, which removes the risk the initial review's finding 1 raised instead of measuring around it.
• The exhaustive match now carries the request-side words ("people" for person), so a new core type still has to be placed.
• stage1_prompt_names_the_sessions_retrieved_types pins the exact type line, including that an unnamed retrieved type is left out.
• The PR body and the ADR state the accepted cost plainly.

Recommendation

APPROVE

The code is correct and a net improvement over both main and the first commit. Finding 1 should be corrected before the docs are committed, since it is a factual error in an ADR table. Findings 2 and 3 are either a small code change or one sentence each in the ADR's accepted cost. Finding 4 is worth doing in this PR if another live run is planned anyway.

[malibio]

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

[malibio]

Re-review addressed (commit 7582fa5a)

The branch is now one commit rebased onto current main (d9f89741), so there is no incremental diff against ad104f31; the end state is git diff origin/main...HEAD. Finding 2 changed where the user's type names come from, so this needs another look.

#	Finding	Outcome
1	Two written claims don't match the case table	Fixed. The ADR table's third row now says 14 of the 17 get the prompt without the line and the 3 that name task get it and decide the same. The case counts are gone with the restructured test; the PR body is rewritten from the final run.
2	The rule's matcher is looser than the one that builds its candidates	Fixed by the second option. Stage 1 reads every non-core schema's display name through a new AgentToolExecutor::user_type_names (the pattern skill_names uses) and one matcher, user_message_names_type, decides. The session field, its setter, and the daemon and core plumbing are removed: those two files are back to main's. Stage 1 no longer depends on what semantic retrieval ranked.
3	"Singular or plural" covers -s and -es only	Fixed. words_match_modulo_plural pairs a consonant + y with ies, unit-tested through stage1_type_names ("companies" names Company). Irregular plurals stay unmatched and the ADR says so.
4	Tests don't guard the failure the redesign prevents	Fixed. The live test fails when an unrelated case's query gains a type word its message lacks (type_words_added), and asserts unrelated cases absolutely. Three cases use a type's name as an ordinary word. The no-subject group moved to the gated unit test, which now covers all twelve plus the two accepted misses. The test is renamed to what it runs. Reps are 4, so each arm goes first equally.
5	Evidence for the rejected designs isn't reproducible	Skipped. Keeping a hand-built always-listed arm means maintaining a prompt that ships nowhere; the ADR records the two rejected arms and says they can't be re-run from the repository.
nit	"the first is its name in the prompt"	Comment corrected: the prompt names a built-in type by its type id.
nit	Group::KnownType describes cases that name no type	Those three cases are out of the live table; two are in the unit test as accepted misses.
nit	Odd rep count	Now 4.
nit	Single-character words dropped from a name	Left. It is the existing matcher's rule and costs one extra or one missing name.
nit	Message tokenised once per candidate	Left; negligible next to the model call.

Final measurement

Same model, four reps per case, every rep of a case deciding alike:

• Requests about records of a known type: 4 of 7 routed without the line, 7 of 7 with it.
• Type-naming requests never in doubt: 8 of 8 in both arms, with identical queries; no type word added to any.
• Prompt 792 → 803 tokens. Generation 2313 ms → 2397 ms over 36 runs where both arms decided alike, 23 tokens generated in each.
• "what's the date today?" (no expectation): clarified without the line; with it Stage 1 called no routing tool, which the loop treats as a query on the raw message.

One correction to my earlier comment: the latency comparison there was skewed. Each case's first run without the line follows the previous case's run without the line, so it reuses an already-evaluated system prompt and takes about a quarter of the time (roughly 540 ms against 2100 ms), in that arm only. The comparison now leaves out each case's first rep.

Checks

bun run test:changed passes on the rebased branch; bun run quality:fix clean.

Re-review: needed (user_type_names and the plural rule are new code).

[malibio]

Pragmatic Code Review: PR #3493 (issue #1980)

Review Type: Re-Review (third round; end state of 7582fa5a against origin/main, compared with the re-review of ad104f31)

Summary

The branch is one commit and touches only packages/agent; the daemon and core plumbing from the earlier design is gone. Stage 1 now reads every user-defined type name through AgentToolExecutor::user_type_names and one matcher decides which of them the message names. That is simpler than the previous design and removes the dependency on what semantic retrieval ranked.

I found no defect that blocks the merge. There is one finding I rate Important: the new read skips the governance participation rule, so an archived type is named to Stage 1. The rest are suggestions about what the live test can and cannot detect.

How this was checked:

• Read git diff origin/main...HEAD (9 files, all in packages/agent), the matcher and its two callers, get_all_schemas in the store, the engine's prompt-prefix cache, and the pending edits to ADR-038 and prompt-skill-system.md.
• Ran 26 targeted tests (stage1, user_type_names, names_type, second_schema filters), all passing, including prompt_assembly_snapshot::stage1_request_matches_golden.
• Not run: the live model tests (as instructed), the full suite, the daemon routing eval. The measured numbers are the author's; I checked that they are consistent with the case table, the harness and each other.

Previous Review Summary

	Count
Previous recommendations	10 (5 findings, 5 nits)
Addressed	7 (findings 1 to 4; the three nits on the core-type comment, Group::KnownType and the rep count)
Not addressed, with a reason that holds	3 (finding 5; the single-character-word nit; the tokenise-once nit)
New issues found	1 important, 3 suggestions, 4 nits

Checked independently:

• Finding 1 (claims vs the case table): fixed. ADR-038's third row now says 14 of the 17 get the prompt without the line and the 3 that name task get it and decide the same. I counted the routing eval's fixture: 19 prompts, 2 with prior turns, so 17 single-turn, of which 3 name a built-in type. The case counts that were wrong are gone with the restructured test.
• Finding 2 (two matchers): fixed. user_message_names_type is now the only thing that decides, for built-in and user types alike. git diff origin/main...HEAD --stat -- packages/daemon packages/core is empty, and no relevant_type_names, session field or setter remains. One stale comment is left (nit below).
• Finding 3 (-ies): fixed, and the rule is correct. See "The plural rule" below.
• Finding 4 (tests): addressed in every part asked for. The twelve no-subject messages and the two accepted misses are in the gated unit test, the unrelated cases are asserted absolutely, four cases use a type's name as an ordinary word, the test is renamed, reps are 4. Two limits remain: suggestions 2 and 3.
• Finding 5 (rejected arms not reproducible): skipped, and the reason holds. The ADR now says the first two rows can't be re-run from the repository.

Requirements Check

#	Criterion		Notes
1	Stage 1's prompt includes built-in type names	✅ with a stated deviation	Included when the message names one. The deviation rests on the recorded measurement that listing them always reworded Stage 1's query on 7 of 17 unrelated requests. I can't reproduce that arm (the ADR says so), but the shipped rule's own guarantee is verifiable: a message naming no type gets a byte-identical prompt, pinned by a_message_naming_no_type_gets_no_type_line for 20 messages.
2	Stage 1's prompt includes semantically retrieved schema names, reusing semantic_search_nodes_of_type	❌ as written, ✅ for its purpose	Not done as written, deliberately: the names come from the stored schemas by word match, with no embedding search. The stated reason (the retrieval's five slots fill whatever the message says; its lexical backstop matches exact tokens only) is consistent with the code, and the criterion's aim, a bounded signal about the user's types, is met: at most 8 names of 60 characters. The PR body and ADR-038 both state the deviation.
3	"Could you tell me how many tasks we have here?" routes	✅	Asserted on every rep in the with-line arm. The PR is open that this case already routed on main and that its siblings are what moved.
4	Latency / turn-count cost measured and documented	✅	No added turn or retrieval (verified). One store read per routed turn. 792 → 803 prompt tokens; 2313 → 2397 ms is 3.6%, so "under 4%" is right. The 36 is consistent with the harness: 16 cases by 3 measured reps, less 9 runs of the 3 moved cases and 3 of the date case. See suggestion 3 on what the 3.6% contains.
5	route_clarify still fires for genuinely vague requests	✅ by construction	A no-subject message names no type, so its prompt is main's. Unit-tested for all twelve.
6	Tests pass	✅ as reported	The 26 targeted tests I ran pass. The merge gate is the full run.
7	bun run quality:fix clean	✅ as reported	Not re-run.

The PR body, the live test's doc comment, stage1_system_prompt's doc comment, ADR-038 and prompt-skill-system.md agree with one another on every number I compared (4 of 7 and 7 of 7; 8 of 8; 792 → 803; 2313 → 2397 over 36 runs; 23 tokens generated) and on the description of the rule.

Code Review Findings

🟡 Important

1. An archived type is named to Stage 1
📁 packages/agent/src/local_agent/tools.rs:4629-4649

• Problem: user_type_names reads get_all_schemas(), whose query is SELECT * FROM node WHERE <node_type is schema> ORDER BY id (packages/core/src/db/sqlite_store/nodes.rs:3878-3914), with no participation condition. A user schema that has been archived is therefore returned, and Stage 1 is told "The workspace keeps records of these types: X" for a type the user retired. ADR-087 §2 lists "workspace context assembled for the agent" among the things an archived node takes no part in. The sibling this method is modelled on does exclude them: skill_names calls query_nodes_by_type("skill", false), where false is include_archived.
• Verified: by reading the SQL and both methods. I did not write a test that archives a schema.
• Context: this is not new to the codebase. build_workspace_context and the daemon's recently-created-schema injector read the same unfiltered corpus on main. This PR adds a third reader.
• Recommendation: filter with the governance predicate, nodespace_core::governance::participates(&schema.envelope), next to the is_core filter, and add an archived schema to user_type_names_lists_the_users_schemas_and_no_core_ones. Not a blocker: the effect is one extra name on a turn whose message names a retired type.
• Principle: one participation rule, asked by every surface (ADR-087 §2); a new surface should not be the place it is forgotten.

🟢 Suggestions

2. The live test asserts four of the seven known-type cases, and its query guard cannot be tripped by the line it guards
📁 packages/agent/tests/it/live_stage1_golden_prompts.rs:467-485, :654-682

• Three known-type cases are printed, not asserted. The Group::KnownType arm asserts HOW_MANY_TASKS and the three MOVED_TO_ROUTE cases. "how many invoices are still open?", "show me my collections" and "which tasks are overdue?" can move to clarify with the line and the test passes, while the doc comment, the PR body and the ADR all say seven of seven. Asserting routed[index][1] == REPS for every KnownType case is simpler than the current condition.
• type_words_added does what its comment says, but under the shipped rule the type line cannot make it fire. The prompt lists only types whose words are already in the message, so any type word the line contributes is one the message "said". The check fires only if the model volunteers a type that is in neither the message nor the prompt, or writes person for a message that said "people". It would catch the selection rule regressing to list unnamed types, which the unit tests already catch without a model.
• The harm that remains for the ordinary-word cases is not detectable by it: the named word being re-read as a record type ("find text records about onboarding" for "find the text about onboarding in my notes"). "With the same queries" in both arms is the claim that covers this, and it is printed only.
• Recommendation: assert all seven known-type cases. For Unrelated cases, compare the with-line query to the without-line query of the same rep and fail (or at least print a diff line in the summary) when they differ. Small gap in the helper itself: singular strips only a final "s", so a query that gains "queries" is not seen as query.
• Principle: a test should fail when the behaviour it documents stops being true.

3. The latency sample is not order-balanced, in the direction that overstates the cost
📁 packages/agent/tests/it/live_stage1_golden_prompts.rs:521-522, :556, :600-610

• Problem: the comment on REPS says each arm goes first as often as the other. That holds over four reps, but latency leaves out rep 0, and over reps 1 to 3 the with-line arm goes first twice. The run order within a case is without, with, with, without, without, with, with, without. The engine reuses a cached prefix only when the new prompt is strictly longer than the shared prefix (packages/nlp-engine/src/chat/mod.rs:454, prefix_len < tokens.len()), so a run that repeats the previous run's prompt exactly takes a full decode, while a run after the other arm decodes only from the type line on. Over the measured reps the with-line arm takes two full decodes per case and the without-line arm one.
• Verified: the order and the engine condition, by reading. Not measured: how much of the 84 ms this accounts for. The shared prefix is short (the role line and the skills line), so I expect tens of milliseconds, not hundreds.
• Effect on the claim: none. The bias runs against the with-line arm, so "under 4%" is an upper bound and the documented number is safe.
• Recommendation: reword the REPS comment, or drop rep 0 from an odd count plus one (five reps, four measured) so the measured reps are balanced.
• Principle: a documented measurement should claim only what its design can separate.

4. The plural rule has one positive test and no negative one
📁 packages/agent/src/local_agent/agent_loop.rs:1657-1669

• Problem: the new branch is exercised only by "companies" naming Company, through stage1_type_names. Nothing pins the consonant condition (day must not pair with daies) or the rule's effect on the older caller.
• Recommendation: add two lines to user_message_names_type_tolerates_case_and_plurals: ("track our categories", "Category") is named, ("the daies", "Day") is not.

Nitpicks

• Nit: packages/agent/tests/it/live_stage1_golden_prompts.rs:88-89: "the prompt production sends on a turn whose schema retrieval matched nothing" describes the removed design. It is now the prompt for a workspace with no user-defined types.
• Nit: packages/agent/src/local_agent/agent_loop.rs:114: "every routed turn carries the whole list" is true of skill titles; a turn carries only the type names its message names.
• Nit: packages/agent/src/local_agent/tools.rs:4638: get_all_schemas loads every schema row, core ones included, and every declaration edge to return names. It is the third read of the schema corpus on a turn, after the workspace context build and the recent-schema injector. Negligible next to the model call; a name-only read would be the tidy form if this is touched again.
• Nit: packages/agent/src/local_agent/agent_loop.rs:218-220: "the user's follow in the order given" is the store's ORDER BY id, so which user types survive the cap of eight is by UUID. Stable across turns, which is what matters; say so, or sort by name.

What changed since the last review, on its merits

user_type_names (agent_types.rs:763-772, tools.rs:4629-4649)

• Returns the display name (envelope.content) of every schema with is_core false. That includes the user's subtypes of core types and user-defined abstract bases, both of which a request can name, and excludes every core schema, which Stage 1 gets from the registry instead. Correct, apart from finding 1.
• The display name is the right thing to match: the matcher splits at non-alphanumerics and camelCase, so "Incident Report" and its id incident_report are the same two words, and the display name is what reads naturally in the prompt.
• A failed read logs a warning and yields no names, the same as skill_names. The trait default is empty, and GraphToolExecutor is the only production implementation, so no wrapper silently drops the names.
• Names are matched against the raw user message, not the history-blended routing query, so a follow-up that refers to a type by pronoun gets no line. That is consistent with the documented accepted cost.

The plural rule (agent_loop.rs:1664-1667)

• Correct. It requires the shorter word to end in "y", the longer to be the same stem plus "ies", and the stem to end in a consonant, so "day" does not pair with "daies" and a bare "y" pairs with nothing.
• Effect on the older caller, the unrequested-schema guard: a second create_schema for "Category" is now allowed when the user said "categories". That is the direction the guard's own doc comment calls the safe one (a false refusal silently delivers half of what was asked for). Acceptable, and an improvement.
• "queries" now names the built-in query, which is intended.

Security

• A user-editable name reaches the system prompt only when every word of it (longer than one character) is in the user's own message. Whitespace is collapsed, the name is capped at 60 characters and the list at 8. The candidate set is now every stored schema, not a retrieved few, but the gate is the same, so the exposure is unchanged from the last round: punctuation and single characters of a name can ride along, nothing else. Stage 1 can only call one of three routing tools. No change needed.

Project rules

• No literal node_type comparison (the built-in list is an exhaustive match on CoreNodeType), no read of lifecycle_status, no compatibility code, no issue numbers in the new comments, no lint suppression, no docs in this repo. No node type, field or relationship changes, so the node type sequence does not apply.

Docs

• ADR-038's section and prompt-skill-system.md agree with the code: the candidate sources, the match rule, the cap of eight, the accepted costs (partial names, irregular plurals, ordinary-word use, the shorter cached prefix). The cached-prefix cost is consistent with the engine keeping one cached prompt.

What is good

• One matcher answers "does the message name this type" for both sources, and the list no longer depends on the embedding index.
• The no-type case is identical to main by construction and that is pinned without a model.
• The author found and corrected a skew in their own earlier latency comparison.
• The deviations from the issue are stated in the PR body and the ADR, with the measurement behind each.

Recommendation

APPROVE

The change is correct and a net improvement. Finding 1 is a one-line filter and one test row, worth folding in before merge. Suggestion 2's first part (assert all seven) is a small edit to an ignored test and needs no new model run to be valid. The rest is optional.

[malibio]

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

[malibio]

Re-review 2 addressed (commit b5f8288f)

#	Finding	Outcome
1	An archived type is named to Stage 1	Fixed. user_type_names keeps a schema only if governance::participates says it takes part; the unit test archives a third schema and expects it left out. The two older readers of the unfiltered corpus on main are outside this PR.
2	Live test asserts four of seven known-type cases; its query guard can't be tripped by the line	Fixed. Every known-type case is asserted to route on every rep. Each never-in-doubt case is asserted to get the same query in both arms, which is what catches a type's name being re-read as a record type. type_words_added stays as the guard against listing a type the message doesn't name. Its singular still strips only a final "s"; left, since the equal-query assertion now covers those cases.
3	Latency sample not order-balanced	Comment corrected to say so: over the three measured reps the arm with the line goes first twice, which counts against it. The documented "under 4%" stands as an upper bound.
4	Plural rule has one positive test and no negative	Fixed. "categories" names Category; "daies" does not name Day.
nit	Comment describing the removed retrieval design	Fixed.
nit	"every routed turn carries the whole list"	Fixed.
nit	get_all_schemas loads every row and edge for names	Left; negligible next to the model call.
nit	Order of the user's names is the store's	Comment now says so.

The live test passes on the locked model with the stricter assertions: 7 of 7 known-type cases routed, 8 of 8 unrelated cases decided as expected with unchanged queries, 792 → 803 prompt tokens, 2321 ms → 2409 ms over 36 runs. bun run test:changed passes.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: the only product-code change is a one-clause filter using the existing governance predicate, covered by a unit test. The rest is test assertions and comments.

[malibio]

Merge queue: #3493 was taken out of the queue — the merge gate failed on it, rebased onto main. Reproduce with git rebase origin/main and bun run test:changed, fix, push, and re-run bun run merge 3493.

Compiling rustls-platform-verifier v0.7.0
   Compiling hyper-rustls v0.27.9
   Compiling reqwest v0.13.3
   Compiling reqwest v0.12.28
   Compiling opentelemetry-http v0.32.0
   Compiling opentelemetry-otlp v0.32.0
   Compiling nodespace-agent v0.3.3 (/Users/malibio/nodespace/nodespace-core/.claude/worktrees/_gate/packages/agent)
error[E0004]: non-exhaustive patterns: `CoreNodeType::ToolNative` not covered
   --> packages/agent/src/local_agent/agent_loop.rs:170:11
    |
170 |     match core_type {
    |           ^^^^^^^^^ pattern `CoreNodeType::ToolNative` not covered
    |
note: `CoreNodeType` defined here
   --> packages/nodespace-types/src/core_type.rs:19:1
    |
 19 | pub enum CoreNodeType {
    | ^^^^^^^^^^^^^^^^^^^^^
...
 44 |     ToolNative,
    |     ---------- not covered
    = note: the matched value is of type `CoreNodeType`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
    |
194 ~         | T::Tool => None,
195 ~         CoreNodeType::ToolNative => todo!(),
    |
For more information about this error, try `rustc --explain E0004`.
error: could not compile `nodespace-agent` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `nodespace-agent` (lib test) due to 1 previous error
error: command `cargo '--color=auto' test --no-run --message-format json-render-diagnostics --workspace --exclude nodespace-app --exclude nodespace-app-lib --exclude nodespace-app-test-support --lib --bins --tests` exited with code 101
error: script "rust:test:build" exited with code 101
[stage exited with code 101]

  full output: /var/folders/6c/mkxnmpkx5zqc9jwwy20tjkyr0000gn/T/nodespace-gate-logs/_gate-merge-2026-10-02T16-08-02-203Z/compile-rust-test-binaries.log

✗ compile Rust test binaries failed — merge blocked.
  Fix the failure, push, and re-run: bun run merge <PR#>