[malibio]

Summary

Closes #3499. Decides whether Stage 1's type line still earns its cost now that route_lookup exists: it does, for a narrower set of requests than it was added for, so it stays and the test is re-aimed at those.

With route_lookup offered, questions about records route as a lookup whether or not the type is named, so the original 7 known-type cases no longer separate the arms. I measured the three candidate shapes the issue lists, 28 requests, both arms, four reps each, on the locked E4B model:

Request	Without the line	With the line
"my projects"	clarify (list or create?)	lookup	helps
"collections"	clarify (list or create?)	lookup	helps
"the matches" (Match type)	clarify (view or create?)	lookup	helps
"open deals" (Deal type)	query	clarify (list them, or open one?)	hurts
"now the invoices", after setup turns	query	lookup	routes either way
"how many tasks do we have?", after setup turns	lookup	query	routes either way
the other 22	same	same

Every rep of every case decided alike. What the line decides now: a request that names a type and nothing else. It isn't shaped like a question, so it isn't put to Stage 1 as a lookup first, and without the line Stage 1 asks whether to list the records or create one. Three gained, one lost, for the same ~11 prompt tokens.

Changes

• live_stage1_golden_prompts.rs: new Group::BareType with the three cases. It asserts each one clarifies on every rep without the line and routes on every rep with it. If one starts routing without the line, the test fails and says the line's cost needs asking again. "open deals" is added as an unrelated case with no expectation (accepted cost); Match and Deal are added to the measured workspace's types. The doc comment records the 28-request measurement and states what the line is for.
• stage1_system_prompt doc comment: the reason for the line is updated to the bare type case. The code is unchanged.
• nodespace-docs (pushed to main): ADR-038's type-line section and prompt-skill-system.md describe this outcome.

Test results

• Live type-line test, reworked, locked model: pass. Bare type 0/3 without → 3/3 with; known type 7/7 → 7/7; unrelated 8/8 → 8/8. Mean prompt 1038 → 1049 tokens; mean generation 3016 → 3102 ms over 45 runs where both arms decided alike.
• bun run test:changed (rust, scripts, types:check, node-types:check): pass.
• eval:routing / eval:decisions: running; results to follow in a comment. The only production change is a doc comment, so one run of each serves as both before and after.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3505 (closes #3499)

Review Type

Initial review. The PR has no earlier reviews or comments.

Requirements Check

Acceptance criterion (#3499)	Status
The type-line test has cases chosen to separate the two arms with route_lookup offered, or a note that none were found	✅ Three Group::BareType cases separate the arms. The 28-request search is recorded in the test doc comment.
If the line decides nothing: remove stage1_type_names, the line, user_type_names and the two-arm structure	✅ N/A. The line decides something, so it stays.
If it does decide something: the test asserts those cases, and its doc comment states what the line is for now	✅ It asserts clarify on every rep without the line and Lookup on every rep with it. The doc comment says the line is for "a request that names a type and nothing else".
eval:routing and eval:decisions run before and after, with the result on the PR	⏳ Pending. The PR body says they are running and results will follow in a comment. It also says one run serves as both before and after, which holds because no production code changed (only a doc comment). That is reported honestly. The criterion is open until the comment is posted.
architecture/prompt-skill-system.md describes the outcome	✅ nodespace-docs df7739b updates it, plus the type-line section of ADR-038.

The live-model measurements were not reproduced here, because the GPU is in use. I checked them for internal consistency instead:

• In the PR table: 3 helped + 1 hurt + 2 changed tool after earlier turns + 22 unchanged = 28.
• With 20 cases, 15 decide alike with a decision: 20 minus 3 bare-type, minus "open deals", minus "what's the date today?" (no tool in either arm). 15 cases × 3 reps after rep 0 = 45, which matches "45 runs where both arms decided alike".
• 3016 → 3102 ms is +2.9%, which fits "under 4%" and "2 to 4%".

Plural matching and the new USER_TYPES entries

• words_match_modulo_plural("matches", "match"): the remainder after the prefix is "es", and "match" ends in "ch", so it matches. "deals"/"deal" matches on "s". So stage1_type_names returns Match for "the matches" and Deal for "open deals". The arms differ, and the assert_ne!(prompts…) guard holds.
• No other message in TYPE_LINE_CASES contains a form of "match" or "deal", so adding Match and Deal changes no other case's type list. It does widen type_words_added (see the 🟢 finding below).

Code Review Findings

🟡 The stated mechanism ("not put to Stage 1 as a lookup first") isn't what the measurement shows

📁 packages/agent/src/local_agent/agent_loop.rs:83-85, packages/agent/tests/it/live_stage1_golden_prompts.rs:675-677 (Group::BareType doc), :855-858 (test doc), nodespace-docs architecture/prompt-skill-system.md (step 1) and decisions/038-skill-mediated-tool-routing.md (new bullet)

All five places explain the bare-type result the same way: these requests "are not shaped like a question, so they are not put to Stage 1 as a lookup first." That refers to the message-alone pass gated by routing::asks_about_the_message_first. That gate fires only when blended_query != message, meaning there are earlier turns. Two problems follow:

1. The test runs no such pass. run_stage1_request sends one system message and one user message, with no history, to both arms. A question-shaped and a bare-noun message get the same single generation. So the measured difference can't come from the gate. It is the model's own choice: for a question it reaches for route_lookup unprompted, and for a bare noun it doesn't unless told the noun is a record type. That is what the ADR's second clause ("nothing else tells Stage 1 the noun is a kind of record") actually says.
2. "As a lookup first" misdescribes the gate even where it applies. The first pass puts the message alone to Stage 1. It isn't put "as a lookup"; its result is kept only if it comes back as one.

This matters because the comment is the recorded reason the line exists. A future reader who changes is_lookup_shaped or the gate will think the type line depends on it, and it doesn't. This codebase has been bitten by stale or misleading comments before (CLAUDE.md: "Do not infer architecture from existing code comments").

Suggested wording, in all five places: "…a request that names a type and nothing else isn't a question, so Stage 1 doesn't read it as a lookup by itself, and nothing else tells it the noun is a kind of record." Drop the "put to Stage 1 … first" clause. (Principle: comments must explain the real why. A plausible but wrong causal story is worse than none.)

🟢 Group::Unrelated's contract no longer holds for one of its members

📁 packages/agent/tests/it/live_stage1_golden_prompts.rs:681-684, :807-809

The group doc says "The line must change neither what Stage 1 decides for it nor which types its query mentions." "open deals" is in the group because the line changes its decision. It passes only because expect: None skips the decision assertion. Also, "open deals" isn't really a type name used as an ordinary word: it is a request about Deal records, and the accepted cost is a real list-or-open ambiguity. The ADR's "Accepted costs" bullet files it under "a type's name used as an ordinary word", which has the same mismatch. Either add a sentence to the group doc ("a case with no expectation is measured and printed, not asserted; it may be one the line is known to change"), or give accepted regressions their own group so each group doc stays true. (Principle: an enum variant's doc is its invariant; keep it true.)

🟢 Ordinary-word types widen type_words_added, and "the matches" will print a spurious flag

📁 packages/agent/tests/it/live_stage1_golden_prompts.rs:828-845

The test's singular closure only strips a trailing s, so "matches" becomes "matche", but the type word is "match". If the with-line lookup topic for "the matches" says "Match records", the summary prints type words added to the query of "the matches": ["match"], a false positive. It isn't asserted for BareType, so it is only noise.

The more relevant risk is that any Unrelated query that now uses "match" or "deal" as a plain word (e.g. "notes that match onboarding") fails the added[index].is_empty() assertion. Low likelihood with today's cases, and arguably that is the check doing its job. Consider reusing the production plural rule, or adding es handling, so the test's notion of "the same word" matches stage1_type_names's. (Principle: DRY. Two plural rules in one comparison drift apart.)

🟢 Nit: the latest run's numbers aren't in the test doc

📁 packages/agent/tests/it/live_stage1_golden_prompts.rs:880-912

The doc records the pre-route_lookup run, the first route_lookup run (1040 → 1051, 2373 → 2460 ms) and the 28-request search. It doesn't record the run of the reworked test itself, which is in the PR body: bare type 0/3 → 3/3, 1038 → 1049 tokens, 3016 → 3102 ms. Absolute generation time rose about 27% in both arms between the runs. That points to machine load, not the change, so recording the new run with that caveat would stop a later reader comparing the absolute figures across paragraphs.

🟢 Note: the "clarifies without the line" assertion fails when the model improves (intended)

📁 packages/agent/tests/it/live_stage1_golden_prompts.rs:1069-1081

Requiring clarified[index][0] == REPS makes the test fail when the baseline gets better. The doc and the assertion message say this is deliberate: a tripwire that reopens the cost question. With an #[ignore] live test and that message, it is a reasonable design. The counters are right under the alternating arm order: clarified and met are indexed by arm, not by position in order.

Other checks

• No issue numbers in code comments or docs. No lint suppression. No production behaviour change: only the doc comment on stage1_system_prompt changed, so no Stage-1 prompt golden needs regenerating. The change does one thing.
• Node-type sequence: not triggered. Match and Deal are test-fixture user type names, not schema changes.

Recommendation

APPROVE. The change is a net positive. It answers the issue's question with measurements and turns them into a test that pins exactly what the line now decides. Before merge, I'd fix the 🟡 mechanism wording in the code and docs, which is a small text-only edit in five places. The eval criterion stays open until the eval:routing / eval:decisions comment is posted.

[malibio]

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

[malibio]

Review addressed

Addressed (3):

• 🟡 Mechanism wording. The reviewer is right: asks_about_the_message_first needs blended_query != message, i.e. earlier turns, and the test sends none, so the message-alone pass is not why a bare noun phrase needs the line. The cause is that Stage 1 reads a question as a lookup by itself, and nothing but the line tells it a bare noun is a kind of record. Fixed in stage1_system_prompt's doc comment, both test doc comments, and nodespace-docs (prompt-skill-system.md, ADR-038; cfdc2de on main).
• 🟢 Group::Unrelated doc. It now says a case with no expectation is measured and printed, with only its query asserted, which is where the accepted cost "open deals" sits.
• 🟢 Latest run in the test doc. Recorded, with the note that both arms ran about a quarter slower than the earlier run (machine load), so arms are compared within a run.

Skipped (2):

• 🟢 Test plural rule vs production's. It only adds a printed "added type word" flag for "the matches", which is a bare type case and not asserted on that, and no asserted case uses "match" or "deal". Revisit if an Unrelated case ever does.
• 🟢 Tripwire note. Intended; nothing to change.

Comment-only changes; cargo fmt --check is clean.

Re-Review Decision

NO RE-REVIEW NEEDED. Every change is to comments or docs, there is no logic change, and each one implements the reviewer's own suggested wording.

[malibio]

eval:routing / eval:decisions

Single rep each, locked E4B, isolated daemon (NODESPACE_HOME=/tmp/nodespaced-3499, purged DB, release build of this branch). The PR changes no production code, only doc comments, so these numbers are both "before" and "after": they measure main's routing, with the type line in place.

Eval	pass^1	Failed
eval:routing	17/19	indirect-schema-freelance, ambiguous-client-contacts
eval:decisions	22/23 (1 excluded: empty generation on declarative-fact-report-resolved)	skill-instance-request

Whether the type line can be involved:

• indirect-schema-freelance ("start tracking our planning cycles") and ambiguous-client-contacts ("organize my design docs") name no type, so their Stage-1 prompt has no type line. The line can't cause them.
• skill-instance-request ("Add Contoso Ltd to the companies we sell to.") names Company, so the line is present. The turn took the right action (create_node) but led with Bulk Import over Node Creation, a retrieval-ranking miss. A single run without an arm can't say whether the line moved that ranking. The live Stage-1 test covers the routing decision itself, not retrieval.

These are single-rep figures, so they're one draw, not a reliability number.