Repository navigation
Read a new kind of record off the routing query, and create the task a refused field key used to lose (closes #3603, closes #3632) - #3655
Conversation
Pragmatic Code Review: PR #3655
SummaryThe change is a net improvement and follows the patterns already in the codebase. The add path of Nothing here is a security or architectural blocker. What holds the approval back:
What I ran (all passed): 27 targeted Requirements Check#3603
#3632
Code Review Findings🟡 Important1. The write path never applies the closed-bucket check to a schema node: Consequence at the caller: The doc comment promises "refused exactly when", ADR-064 says "a test holds the two to the same answer key by key", and the test iterates only Recommendation: return Principle: a predicate that mirrors another must be held to it over the whole input domain, not a sample (the project's own "assert the invariant, mutation-test both directions"). 2. The new-kind shape's cost side is measured only on requests with a determiner After a tracking phrase the only test is "the next word is not one of nine determiners/pronouns". Every one-record control in the unit test and in the live cost-side test starts with "the", "this", "a" or "an". These all return
When one of these does not already lead with a type skill, Schema Creation joins at about 1.0 and leads: the turn is recorded as routed to Schema Creation, the record skill's declared fields come off The issue named Recommendation: add these to the live cost-side test and measure. Then either narrow the test (cheap candidates: a Principle: a heuristic that overrides a ranking needs its false-positive side characterised on inputs that were not chosen to pass it. 3.
Recommendation: score Principle: an assertion must be about the thing it names; the same "called, not created" gap this PR closes. 4. All six Node Creation rows moved from
The criterion itself is defensible (two skills hold Recommendation: keep Principle: a ratchet should only loosen where it was shown to be too tight. 5. A causal claim in ADR-064 and in the code comment that the PR body says is not isolated The PR body: the description-without-the-prefix-rule run "also had the tool-node mentions in it, so the cause is not isolated". ADR-064 states it as a finding ("removing one sentence from it moved a different argument"), on two reps, and the doc comment on This matters beyond wording: per ADR-064 rule 1 the description owns argument shape, and the description still says "prefix it by type" (the phrase the PR credits for Recommendation: re-run the description variant on the final build with Principle: KISS, and docs that state only what was measured. 🟢 Suggestions6.
7. A dry run does not say the key would be renamed The 8. The second search for a new kind asks for The add path's comment gives the reason for the wider fetch: a workspace's own types can fill the first places of a short ranking. The same holds here and the live tests seed no workspace types. Either use the wider fetch or say why four is enough. 9. Acceptable for a local store. The reorder itself is sound: the type is read only when there are keys to judge, an unreadable node falls through to the update's own error, and Nits
What is good
VerdictREQUEST CHANGES. Fix finding 1, address 2 to 4 (measure-and-record is an acceptable resolution for 2), align the ADR and comment with what was measured (5), and post the final-build results for the criteria marked ⏳. Nothing here needs a redesign. |
malibio
left a comment
There was a problem hiding this comment.
Automated pragmatic-code-review: REQUEST CHANGES (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.
Review addressed, and final-build resultsFindings
Nits: the shape's doc comment now matches its word list. The orphaned paragraph above the new test module predates this PR and is left. The summary-line wording in Not done from finding 5: a clean run of the description without the prefix sentence. It is a separate measurement with its own build and eval cycle, and nothing here depends on it. Results,
|
| Scenario | Before | After |
|---|---|---|
skill-type-request-direct |
0 of 3 (recorded on #3632) | 3 of 3 |
skill-type-request-indirect |
0 of 3 (recorded on #3632) | 3 of 3 |
direct-node-create, a type exists |
created on the third create_node call, 3 of 3 |
first call, 3 of 3 |
multi-task-and-search, a type exists |
call refused, no task, 2 of 2 clean reps | first call, 3 of 3 |
single-intent-long-task, a type exists |
first call, 2 of 2 clean reps | first call, 3 of 3 |
| the same three, empty database | direct-node-create failed in 1 rep: no node_type, then update_node on the create_node tool node |
first call, 3 of 3 each |
The baseline's third rep ran under machine load and is left out where a cell says "clean reps".
One pass of each full fixture as a smoke:
- Routing: 18 of 19.
indirect-schema-expensesfails, as it did in all three baseline reps.instance-not-schema-invoicepassed in this pass; it failed 0 of 3 on the baseline and on an earlier build of this branch, because the model sends the display namefeature specasnode_type. One pass does not show it fixed. - Decisions: 22 of 25. Failing:
outcome-find-own-skill,declarative-fact-report-resolved,held-off-menu-node.
One scenario moved the wrong way
held-off-menu-node passed 3 of 3 and one further rep on the baseline build, and 2 of 3 and failed one further rep on the final build. On both builds the model answers the request with create_node on feature_spec twice; the first is refused for a missing title on both. The second is refused by the duplicate guard when its title is "Kestrel Gateway" (baseline) and lands when it is "Kestrel Gateway Update" (final build, failing reps). feature_spec is a user-defined type, so the key handling in this PR does not touch the call. What changed the retry's title is not isolated: the final build differs from the baseline in the mention list (tool nodes gone), the routing shape and eight merged commits from main. The criterion "no eval:decisions scenario that passes 3 of 3 beforehand regresses" is therefore not shown to hold for this scenario.
Checks
bun run quality:fix clean. nodespace-core and nodespace-agent: 3958 tests pass. The live retrieval suite and the confusion matrix pass on the locked embedding model (42 tests), and the matrix is re-recorded. bun run test:changed passed on the merged branch before the review fixes; after them it stops at node-types:check, on an uncommitted skill.role edit in the shared docs checkout that is not part of this PR.
Re-Review Decision
Decision: RE-REVIEW NEEDED
Rationale: Finding 1 changed a public core method's signature and behaviour, finding 2 changed the routing heuristic, and finding 6 added a core method.
Pragmatic Code Review: PR #3655 (re-review)
Previous Review Summary
What I ran, all passing: Per-finding verification
Nits: the shape's doc comment now matches the constant. The two left alone (the orphaned paragraph above the test module, the summary-line numerator) are fine to leave. Finding 2: is recording four false positives as a measured cost acceptable?Yes. The first review offered measure-and-record as a resolution, and this is a good version of it:
What stays open is stated in the ADR: what Stage 2 does when Schema Creation leads one of these was not measured (N3). New findingsN1. 🟢 Suggestion: one input where A map that holds the node's own bucket as an object is taken as storage shape whole: Before this PR the same call left Principle: the same one as finding 1, a mirrored predicate held over the whole input domain. N2. 🟢 Suggestion: the dry-run note says the key "is stored" The dry-run branch words its own N3. 🟢 Suggestion: put a number on the accepted cost One decisions scenario on "track progress on the Q4 cycle" against a seeded cycle would replace "what Stage 2 does with these was not measured" with a result, and would say whether a type the user did not ask for is ever created. Not required for this merge.
No new Critical or Important issue. Nothing in the commit touches a node type, field or schema; the two core methods resolve types through the registry (
|
malibio
left a comment
There was a problem hiding this comment.
Automated pragmatic-code-review (re-review): APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.
Re-review addressedThe re-review approved with three suggestions and a nit.
Re-Review DecisionDecision: NO RE-REVIEW NEEDED Rationale: One early return in a helper with a test that holds it to the store, one word in a result note, and a comment. No routing or model-facing behaviour on a real write changed. Open for the merge decision: |
|
Merge queue: #3655 was taken out of the queue — it conflicts with main in: packages/agent/src/local_agent/routing.rs, packages/agent/tests/golden/skill_retrieval/current.sha256, packages/agent/tests/golden/skill_retrieval/current.tsv, packages/agent/tests/it/skill_confusion_matrix.rs. Merge or rebase origin/main, resolve the conflicts, push, and re-run |
f117e89 to
6fd13ea
Compare
Closes #3603. Closes #3632.
What changes
A request for a new kind of record is read off the routing query (#3632).
routing::is_new_kind_shapedrecognises a query that opens with a tracking phrase ("keep track of", "start tracking", …) or with "set up", and passes that opening's test for a kind of thing rather than one record. When the candidate that would lead the turn cannot define a type,retrieve_candidatessearches once more ondefine a new entity type with custom fields: <query>and the best match that holdscreate_schemajoins the ranking, which it then leads. This is the mechanism the add shape already uses; the two now sharewith_the_best_match.create_nodeandupdate_nodestore a key a built-in type does not declare (#3603). At the local agent's tool boundary,<type>__<field>is stored as the field when the type declares it, any other key the store would refuse is stored undercustom:, and the result carries a note saying so. The store answers which keys it would refuse (NodeService::undeclared_core_keys), and a core test holds that answer and the refusal to agree key for key. The CLI and the store still refuse the bare key.Tool nodes are no longer resolved as mentioned entities. From an empty database, "Create a new task called …" resolved to the
create_node,create_schemaandcreate_relationshiptool nodes. The turn was told to use those ids, and wrote the task's title onto thecreate_nodetool node withupdate_node, then reported the task as created. This is whatdirect-node-createdoes onmaintoday from an empty database, and it is not thetask__statusbehaviour the issue recorded.The routing eval scores record creation on the record.
createsRecordon the four record-creating scenarios: a turn passes only when acreate_nodecall was accepted.The confusion matrix ranks through the candidates routing judges, and one new record of an existing type is won when the leading candidate can create a record.
NOT_YET_WONgoes from 18 requests to 14 (two of the four by the new verdict, not by routing), and three new-kind rows are added that routing wins and retrieval alone does not.Measurements
Retrieval, locked embedding model:
gemma-4-e4b-q4km, isolated daemon, three independent reps:The tables are in the "Review addressed, and final-build results" comment. In short: both type-request scenarios go from 0 of 3 to 3 of 3; the three task scenarios create the task on the first
create_nodecall, 3 of 3, with a type in the workspace and from an empty database. One decisions scenario,held-off-menu-node, went from 3 of 3 to 2 of 3 for a cause that is not isolated; the comment has the detail.Not changed, and why
field_values's description still asks for thecustom:prefix. A version without that sentence was tried once, on a build that still resolved tool nodes as mentions, so the result is not a clean comparison. The wording is unmeasured and was put back.declare_write_tool_fieldsand its distractor rule are untouched, so no ADR-038 rule is reversed.instance-not-schema-invoicefailed 0 of 3 on the baseline and on an earlier build of this branch: the model sends the type's display name (feature spec) asnode_typewhere the id isfeature_spec. It passed in the one smoke pass on the final build, which does not show it fixed. Not addressed here.NOT_YET_WON.Docs
nodespace-docs: ADR-038 (amendment: a request for a new kind of record), ADR-064 (amendment: a key a built-in type does not declare),architecture/prompt-skill-system.md(requests read by their shape; the matrix's verdict),architecture/agent-context-tiers.mdandarchitecture/data-layer.md(tool nodes),development/agent-eval.md(createsRecord).🤖 Generated with Claude Code