Skip to content

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

Merged
malibio merged 1 commit into
mainfrom
issue-3603-3632-create-shapes
Oct 6, 2026
Merged

malibio merged 1 commit into
mainfrom
issue-3603-3632-create-shapes

Conversation

@malibio

@malibio malibio commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

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_shaped recognises 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_candidates searches once more on define a new entity type with custom fields: <query> and the best match that holds create_schema joins the ranking, which it then leads. This is the mechanism the add shape already uses; the two now share with_the_best_match.

create_node and update_node store 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 under custom:, 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_schema and create_relationship tool nodes. The turn was told to use those ids, and wrote the task's title onto the create_node tool node with update_node, then reported the task as created. This is what direct-node-create does on main today from an empty database, and it is not the task__status behaviour the issue recorded.

The routing eval scores record creation on the record. createsRecord on the four record-creating scenarios: a turn passes only when a create_node call 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_WON goes 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:

  • Thirteen new-kind requests over thirteen kinds, ten named after another skill's subject: all lead with Schema Creation on five reps. On retrieval alone the ten lead with a skill that cannot define a type.
  • Seven requests that open the same way and are about one record or a view keep the candidates they had. Four the type skill already leads keep their scores.
  • Schema Creation's quoted trigger phrases and Relationship Management's last sentence were each measured for removal, and each stays: removing either lost its skill the lead or its place on requests it owns. What was lost is recorded on the seeds.

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_node call, 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 the custom: 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_fields and its distractor rule are untouched, so no ADR-038 rule is reversed.
  • instance-not-schema-invoice failed 0 of 3 on the baseline and on an earlier build of this branch: the model sends the type's display name (feature spec) as node_type where the id is feature_spec. It passed in the one smoke pass on the final build, which does not show it fixed. Not addressed here.
  • "add a severity field to tickets" and "rename the owner field on projects to lead" open with neither phrase and stay on 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.md and architecture/data-layer.md (tool nodes), development/agent-eval.md (createsRecord).

🤖 Generated with Claude Code

@malibio

malibio commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Pragmatic Code Review: PR #3655

  • Review Type: Initial Review (no prior reviews or comments on the PR)
  • Recommendation: REQUEST CHANGES (narrow: one confirmed defect, two test/eval gaps, and the live-model evidence the PR body itself says is still being produced)

Summary

The change is a net improvement and follows the patterns already in the codebase. The add path of retrieve_candidates is behaviourally unchanged by the refactor, the new-kind rule reuses the add rule's mechanism through with_the_best_match, the tool-node exclusion goes through the registry (is_not_a_sql with CoreNodeType::Tool, so subtypes are covered), and no node-type rule is broken: an undeclared key is stored under the declared custom: extension namespace, never bare.

Nothing here is a security or architectural blocker. What holds the approval back:

  1. undeclared_core_keys does not agree with the store for schema nodes, and the agreement test does not cover that type.
  2. The shape heuristic's cost side is measured only on requests that carry a determiner; several ordinary single-record wordings pass the test.
  3. createsRecord can be satisfied by a prior turn.
  4. Most acceptance criteria of both issues rest on live-model runs that are not in the PR yet.

What I ran (all passed): 27 targeted nodespace-agent lib tests (new-kind routing, add routing, undeclared_keys_on_a_built_in_type), the 7 declare_write_tool_fields pinning tests, the_recorded_matrix_is_of_the_seeded_text, the core tests undeclared_core_keys_lists_exactly_the_keys_a_write_refuses and tools_are_not_resolved, and routing.test.ts (29 pass). I did not run the #[ignore]d live retrieval tests, the confusion matrix, or any eval, so every claim that depends on the embedding model or Gemma is unverified by this review.


Requirements Check

#3603

Criterion Notes
⏳ direct-node-create creates the task on its first create_node call, 3 of 3 Mechanism is in place and unit-tested (task__status is stored as status; tool nodes no longer resolve as mentions). ADR-064's table records 3 of 3, but the PR body says the final-build runs are still in progress. Pending evidence.
⏳ multi-task-and-search creates a task, 3 of 3 Same. Pending evidence.
⏳ single-intent-long-task creates one task, 3 of 3, from an empty database and after the type-creating scenarios Unit test covers the notes key. The ADR table has one column for it and does not separate the two database states the criterion names. Pending evidence, and the write-up should report both states.
✅ (by removal) A refused create_node is followed by a call that can succeed Met by no longer refusing the measured cases at the agent's tool boundary. The refusal text and its custom:<key> example are unchanged, so the refusals that remain (a key sent both bare and prefixed, the CLI) still carry the example the model copied.
✅ / ⏳ eval:routing record scenarios fail with no accepted create_node; a 3-rep run reports which were passing on refused calls Scoring is implemented and tested (createsRecord, createdARecord, the two summary lines). See the finding on prior turns. The before/after report is the ADR table, not yet from the final build.
✅ Distractor pinning tests still pass declare_write_tool_fields is untouched; its 7 tests pass.
⏳ quality:fix and test:changed Not verifiable from the code; the main agent's step.

#3632

Criterion Notes
⏳ Three colliding requests and the two scenario prompts measured, before and after Before-scores are recorded in ADR-038 and on the live test; "after" is asserted by a_new_kind_of_record_with_named_details_leads_with_schema_creation, which is #[ignore]d and was not run here.
⏳ skill-type-request-direct / -indirect pass 3 of 3 ADR-038 states routed to Schema Creation 3 of 3. Pending final-build evidence.
⏳ The three named guards still pass Unchanged in the diff; live tests, not run here.
✅ The lead is asserted for every request the mechanism fixes The test now asserts the lead through routed_candidates for all 13, with a companion test that fails if retrieval alone ever fixes them. Good design: the reason for the rule stays measurable.
⏳ No eval:decisions scenario regresses Pending evidence.
✅ (with a caveat) Matrix ranks through routed candidates; current.tsv and digest re-recorded; won requests off NOT_YET_WON Done; the digest gate passes; the list is 14. Caveat in the LeadCreates finding below. The wrong comment on dependency questions is corrected.
✅ Trigger-phrase list and Relationship Management's last sentence measured, kept Recorded on the seeds with what each removal lost. Not reproducible without the model.
⏳ quality:fix and test:changed As above.

Code Review Findings

🟡 Important

1. undeclared_core_keys disagrees with the store for schema nodes
📁 packages/core/src/services/node_service/crud.rs:3383-3415 (against 3197-3211 and 3229-3248)

The write path never applies the closed-bucket check to a schema node: rebucket_and_validate_with returns before reject_undeclared_core_keys when CoreNodeType::Schema.is_exactly(..), and field_ownership_for_write gives schema an empty ownership map. undeclared_core_keys has no such carve-out. CoreNodeType::from_id("schema") is Some, core_schema(CoreNodeType::Schema) is None, so owners is empty and every key that is not _-prefixed or namespaced is reported as one the store "would refuse".

Consequence at the caller: update_node on a schema node's id with field_values (the model does address types by id) now has every key rewritten to custom:<key>, and the result says updated: true with a note that '<key>' is not a field of the built-in type 'schema'. Before this PR that call was not rewritten. Found by reading, not executed.

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 task, text, gadget, chore.

Recommendation: return Ok(Vec::new()) for CoreNodeType::Schema.is_exactly(node_type) (or resolve through field_ownership_for_write and share the early return), and add schema to the test's type list. While there, add a key named after a chain scope ("task") and one derived-attribute name: the two !chain.iter().any(..) / !derived.iter().any(..) clauses are currently unpinned, so deleting either leaves the test green.

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
📁 packages/agent/src/local_agent/routing.rs:512-543; controls at packages/agent/tests/it/live_skill_retrieval_stability.rs (requests_about_one_record_that_open_like_a_new_kind_keep_their_ranking)

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 true (checked against the code):

  • keep track of my dentist appointment on Friday (possessives are excluded for "set up" but not for tracking phrases; the unit test asserts start tracking our planning cycles is a kind)
  • start tracking Priya's onboarding, track Acme's renewal (a named record with 's; the control start tracking the offline sync spec's sign-off is excluded only by "the")
  • track progress on the Q4 cycle, track time spent on the importer bug
  • keep track of when the Q4 cycle ends, track whether the deploy finished, keep track of what we decided about caching
  • track one expense, track each open ticket
  • track down the notes on the auth redesign (an idiom for a lookup)
  • on the "set up" side: set up staging with a seed script, set up weekly sync with an agenda

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 update_node/create_node (declare_write_tool_fields is top-score only), and a model offered create_schema at the lead can create a type the user did not ask for. ADR-038's Costs names only "set up meeting with a client". Stage 1 words queries tersely, which the ADR itself notes makes the article-less form more likely than it looks.

The issue named stage1_type_names as a signal for exactly this distinction; it is not used and nothing records why.

Recommendation: add these to the live cost-side test and measure. Then either narrow the test (cheap candidates: a 's in the words before the first preposition, possessive pronouns on the tracking side too, and wh-words / "one" / "each" / "down" as the first word) or record them as costs in ADR-038 with what they did to the ranking. Either outcome is fine; an unmeasured one is not, given that the rule's whole justification is measurement.

Principle: a heuristic that overrides a ranking needs its false-positive side characterised on inputs that were not chosen to pass it.

3. createsRecord is satisfied by a create_node in a prior turn
📁 scripts/eval/fixtures/routing.ts:468-473 (createdARecord), used at 511-518

turns is [...priorTurns, scored] (scripts/eval/runner.ts:1216). createdARecord is turns.some(...), so for instance-not-schema-invoice, whose prior turn is "Create a database for tracking our feature specs", an accepted create_node in that prior turn passes the scored turn's record check. The PR reports this scenario at 0 of 3, so nothing is masked today, but it is the one scenario with both priorTurns and createsRecord, and the new scoring exists precisely so a turn that created nothing cannot pass.

Recommendation: score createsRecord on the scored turn only (turns.at(-1)), with a test that has an accepted prior-turn call and a refused scored-turn call. createdAType has the same shape; it is not exposed today because no createsType scenario has prior turns, so fixing both in one helper is the smaller change.

Principle: an assertion must be about the thing it names; the same "called, not created" gap this PR closes.

4. LeadCreates relaxes two rows that were asserted as First and are won today, and takes two rows off the list without routing doing anything
📁 packages/agent/tests/it/skill_confusion_matrix.rs:115-158, 367-370; packages/agent/tests/golden/skill_retrieval/current.tsv:64-69

All six Node Creation rows moved from first(.., "Node Creation") to add(..). Per the recording:

  • create a ticket for the login bug and add a new customer called Harbor Freight are led by Node Creation on retrieval and routing. They held the strict assertion before and no longer do: a Node Creation rewording that hands them to Graph Editing now passes.
  • new note: call the plumber and make a task to renew the domain are not add-shaped ("new" and "make" are not in ADDING_VERBS), so routing leaves them as retrieval ranked them; their routed leader is Graph Editing. They came off NOT_YET_WON because the criterion changed, not because of ranking through routed candidates. That is also the state Creating a task is refused for undeclared field keys, sometimes on every call, and the routing eval passes it #3603 describes as part of its problem (Node Creation a candidate and not the leader).

The criterion itself is defensible (two skills hold create_node, and the docs say so plainly). The cost is the lost signal on the rows that were strict.

Recommendation: keep first on the two rows Node Creation leads, and use add only where the wider verdict is what is meant. Say in the PR body that two of the four removals are by criterion.

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
📁 nodespace-docs/decisions/064-instruction-channel-doctrine.md:267-275; packages/agent/src/local_agent/tools.rs:3760-3761

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 store_undeclared_keys repeats it ("it stopped the task__status key and cost the first call its node_type"). The table's "Before" column also predates the tool-node fix, which the PR identifies as what direct-node-create actually does on main.

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 task__status) and "extra keys are stored as given" (no longer true on a built-in type). Fixing the description is the simpler change if it holds, and the one run against it was confounded.

Recommendation: re-run the description variant on the final build with --only direct-node-create,multi-task-and-search,single-intent-long-task (three scenarios, three reps), and record the result. If it still costs an argument, the ADR's claim stands on clean evidence; if not, the description is the fix and the tool-boundary rewrite can shrink. Until then, word both places as the PR body does. Re-record the table from the final build either way.

Principle: KISS, and docs that state only what was measured.

🟢 Suggestions

6. declared is inferred from "not refused", which is wider than "declared"
📁 packages/agent/src/local_agent/tools.rs:3784-3798

undeclared_core_keys(node_type, [field]).is_empty() is also true for an open key, a chain-scope name and a derived attribute. task__custom:x is stored as custom:x with the note "is the 'task' field 'custom:x'"; task__task is stored under the key task, which is the type's bucket name. Each <type>__ key also costs one more chain-and-schema resolution. Resolving ownership once and asking "does a schema in the chain declare this name" answers the real question with one lookup.

7. A dry run does not say the key would be renamed
📁 packages/agent/src/local_agent/tools.rs:4836-4854

The dry_run branch returns before with_stored_as_note, so a dry run of {"notes": ..} predicts success with no mention of custom:notes.

8. The second search for a new kind asks for RETRIEVAL_FETCH, the add's asks for ADD_RETRIEVAL_FETCH
📁 packages/agent/src/local_agent/routing.rs:634

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. update_node reads the node twice on a content-only-plus-fields path, and once more than before on every call with fields
📁 packages/agent/src/local_agent/tools.rs:4739-4747

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 property_count is still taken from the call's keys.

Nits

  • Nit: routing.rs:497-503: the doc comment lists five words after a tracking phrase; the constant has nine. One line there is also far over the wrap width.
  • Nit: tools.rs:9197-9204: the new test module's doc comment is appended to an orphaned paragraph about update_node's no-op gate, so the module now opens with another module's description. The orphan predates this PR; this is a good moment to move it.
  • Nit: routing.ts:701: "Asked for a new record and created one: n/m" counts scenarios that passed overall, as the type line does; a turn that created the record and failed routing is not in the numerator.
  • Nit: the agreement test treats any error without the refusal text as "not refused"; a comment saying so would stop a reader assuming status: "x" was accepted.

What is good

  • with_the_best_match removes real duplication, and the add path's order of operations (without_destructive_skills on both searches, score gate, move-not-repeat) is preserved exactly.
  • Checking the lead and not a place, with the reason written down, and without_the_new_kind_rule_those_requests_lead_with_another_skill to keep that reason falsifiable.
  • The tool-node exclusion is the right layer and the test proves the type, not the title, keeps it out.
  • The leniency is confined to the agent's tool boundary; the CLI and the store keep the closed schema, and a collision is left to be refused instead of guessed at.

Verdict

REQUEST 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 malibio left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@malibio

malibio commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Review addressed, and final-build results

Findings

# Finding Outcome
1 undeclared_core_keys disagreed with the store for schema nodes Fixed. It refuses nothing for a schema node, and resolves ownership the way a write does. Extending the agreement test also showed a second disagreement, on a key named after a bucket: the store lifts it only when it holds an object, so the helper now takes the properties and judges that key by its value. The test covers schema, checkbox with its derived attribute, and a bucket name with an object and with a string.
2 The shape's cost side was measured only on requests with a determiner Narrowed and measured. After a tracking phrase or "set up" the next word must be one that can open a kind's name: no determiner, pronoun, possessive, quantifier, clause opener, "down", or possessive name. Nine of the reviewer's requests are now controls that keep their candidates. Four still read as a kind ("track progress on the Q4 cycle", "track time spent on the importer bug", "set up weekly sync with an agenda", "set up staging with a seed script"); a live test asserts that and prints what the rule does to each, and ADR-038 records the rankings as a cost. stage1_type_names is not used, with the reason in ADR-038.
3 createsRecord could be satisfied by a prior turn Fixed for both createdARecord and createdAType: the scored turn only, with a test.
4 LeadCreates loosened two rows Node Creation leads Those two rows are First again. Of the four requests off NOT_YET_WON, two ("new note: call the plumber", "make a task to renew the domain") came off by the criterion, not by routing: neither opens with an adding verb.
5 A causal claim in ADR-064 and a code comment that was not isolated Both now say the description without the prefix rule is unmeasured. Not re-run: see below.
6 declared inferred from "not refused" Fixed: NodeService::declares_field, with a test for task__custom:x and task__task.
7 A dry run did not say the key would be renamed Fixed, with a test.
8 Second search asked for RETRIEVAL_FETCH Now ADD_RETRIEVAL_FETCH, for the add path's reason.
9 One more node read in update_node No change, as the review concluded.

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 routing.ts matches the existing type line and is left.

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, gemma-4-e4b-q4km, final build

Scenarios the change bears on, three independent reps:

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-expenses fails, as it did in all three baseline reps. instance-not-schema-invoice passed 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 name feature spec as node_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.

@malibio

malibio commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Pragmatic Code Review: PR #3655 (re-review)

  • Review Type: Re-Review, scoped to b54ba77ce (git diff f34ccaadb..b54ba77ce, 10 files, +296 / -84)
  • Recommendation: APPROVE, with one evidence item left for the merge decision (held-off-menu-node, below)

Previous Review Summary

Count
Previous recommendations 9
Addressed 8 (finding 5 by rewording, without the re-run; finding 9 needed no change)
Partly addressed 1 (finding 1: one input shape still disagrees, low impact, see N1)
Not addressed 0
New issues found 3 suggestions, 0 critical, 0 important

What I ran, all passing: core_type_registry_test:: (26, including undeclared_core_keys_lists_exactly_the_keys_a_write_refuses), nodespace-agent --lib routing:: undeclared_keys (131, including the two new tool tests), skill_confusion_matrix::the_recorded_matrix_is_of_the_seeded_text (the digest gate on the re-recorded matrix), and scripts/eval/fixtures/routing.test.ts (30). I did not run the #[ignore]d live retrieval tests, the confusion matrix itself or any eval: the rankings in ADR-038's cost table and every Gemma result are the author's measurements, not verified here.

Per-finding verification

# Finding Status Verified from the code
1 undeclared_core_keys disagreed with the store on schema nodes Addressed, one residual (N1) crud.rs:3405-3409 returns empty for CoreNodeType::Schema.is_exactly, and ownership now comes from field_ownership_for_write, the function the write uses. A chain-scope key is exempt only when its value is an object, which is unnest_ancestor_buckets' own condition and the already-bucketed test in normalize_flat_properties_to_namespace. The test now runs task, text, checkbox, gadget, chore, schema against 13 key/value cases, including task as an object and as a string and the derived checked, so both clauses I called unpinned are pinned. ADR-064's sentence about the test matches its type list.
2 The shape's cost side was measured only with a determiner Addressed OPENS_NO_KIND (26 words) and opens_a_kind are shared by both openings; possessive names are caught for 's and ’s. Nine of my requests are controls in the unit test and in the live cost-side test. The four that still pass are asserted in requests_about_one_thing_worded_with_a_bare_noun_read_as_a_kind, and ADR-038's Costs carries their before/after rankings, the reason stage1_type_names is not used, and the kinds the narrowing now misses. See the judgment below.
3 createsRecord satisfied by a prior turn Addressed acceptedInScoredTurn reads turns.at(-1) only and backs both createdARecord and createdAType. The new test covers a prior accepted call with a refused scored call, both orders for the type, and assertFixture on instance-not-schema-invoice.
4 LeadCreates loosened two rows Addressed skill_confusion_matrix.rs:153-155 is first(.., "Node Creation") for both; current.tsv records First for them and the digest gate passes. The response states that two removals from NOT_YET_WON are by criterion.
5 Causal claim not isolated Addressed by wording The doc comment on store_undeclared_keys (tools.rs:3760-3763) and ADR-064 both say the run had other differences and the wording is unmeasured. ADR-064 also states that the description still says "prefix it by type" and "extra keys are stored as given". The table now has both database states. The clean re-run was not done; the first review allowed the rewording as the interim resolution, and no code here depends on the outcome.
6 declared inferred from "not refused" Addressed NodeService::declares_field answers from the ownership map. a_type_prefixed_extension_or_bucket_name_is_not_read_as_a_field pins task__custom:weight and task__task.
7 Dry run did not name the renamed key Addressed tools.rs:4851-4855, with a_dry_run_names_the_key_it_would_store, which also asserts nothing was written. Wording: N2.
8 Second search used RETRIEVAL_FETCH Addressed routing.rs:648, with the reason in a comment and the unit test's expected fetch updated. ADR-038 says the same.
9 Extra node read in update_node No change needed As the first review concluded.

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:

  • The class is named ("one thing worded with a bare noun after the opening"), not just the four examples, and ADR-038 says outright that Stage 1's terse wording can put a message that had an article into it.
  • The live test fails in the useful direction: a narrower test for a kind shows up as a request to move to the controls.
  • The rule adds a candidate and decides no route. In three of the four the record skill is still a candidate, and in the fourth Schema Creation already led on retrieval alone, so the rule changes nothing there.
  • The narrowing has its own cost and it is written down: "start tracking our planning cycles" and "start keeping track of who owes me money" are no longer the shape. The first is a routing-fixture prompt (indirect-schema-freelance); ADR-038 records that it leads with Schema Creation on retrieval alone, and start_tracking_requests_route_schema_creation still asserts 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 findings

N1. 🟢 Suggestion: one input where undeclared_core_keys and the store still disagree
📁 packages/core/src/services/node_service/crud.rs:3400-3423 against 3103-3107

A map that holds the node's own bucket as an object is taken as storage shape whole: normalize_flat_properties_to_namespace returns it unchanged, so its other top-level keys are never moved into a bucket, and reject_undeclared_core_keys reads only the buckets. For {"task": {"status": "open"}, "notes": "x"} on a task, the helper lists notes and the store does not refuse it with this message. At the agent boundary the key becomes custom:notes at the top level, beside the bucket, and the result says it "is stored as 'custom:notes'" in a place the read surfaces do not show. Found by reading, not executed; the test's cases are all single-key maps, so it cannot see this.

Before this PR the same call left notes at the top level with no note at all, so nothing regressed. Smallest fix: when properties holds node_type as an object, return an empty list (the write is not flat, which is what the doc comment's first sentence already scopes to), plus one two-key case in the test. The underlying stray-sibling behaviour is the store's and is not this PR's to fix.

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"
📁 packages/agent/src/local_agent/tools.rs:4851-4855, 3816-3821

The dry-run branch words its own note so "the result cannot be echoed as a confirmation", then adds notes: ["'registrar' is not a field of the built-in type 'task', so it is stored as 'custom:registrar'."]. That sentence states a write that did not happen, in the one result built to avoid that. A would be stored as variant for the dry run keeps ADR-064's rule that a result states facts.

N3. 🟢 Suggestion: put a number on the accepted cost
📁 packages/agent/tests/it/live_skill_retrieval_stability.rs:1383-1429; ADR-038 "Costs"

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.

  • Nit: the live test's doc comment calls the four "measured and not accepted"; ADR-038 lists them under Costs, which is accepted. Pick one word.

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 (CoreNodeType::from_id, is_exactly), with no literal type comparison and no compatibility code.

held-off-menu-node: not a merge blocker, but an unmet criterion that needs a decision

Why it does not block on the code:

  • The failing call is create_node on feature_spec, a user-defined type. undeclared_core_keys returns empty for any type that is not a core type (crud.rs:3405), pinned by a_user_defined_type_keeps_the_bare_key, so this PR's key handling does not change that call's arguments or its result.
  • The prompt ("That date is wrong. Follow up on …") opens with no adding verb, tracking phrase or "set up", so neither routing rule runs on it.
  • The model's behaviour is the same on both builds: two create_node calls, the first refused for a missing title. What differs is the title the retry chose, and whether the duplicate guard happened to match it. The baseline's passes rest on that guard matching a model-chosen string, which is not what the scenario is written to test (it expects update_node refused by the hold).
  • 4 of 4 against 2 of 4 does not separate two pass rates. A scenario that truly passes three times in four gives 4 of 4 about a third of the time.

Why it should not be waved through either:

  • The issue's criterion is "no eval:decisions scenario that passes 3 of 3 beforehand regresses", and for this scenario that is not shown. The response says so plainly, which is the right way to report it.
  • Two of the three differences between the builds are this PR's (tool nodes out of the mention list, the routing shape); only the eight commits from main are not. The cheap isolation is that scenario alone on current origin/main, which has those eight commits and none of this PR: if it fails there at a similar rate, the PR is cleared; if it holds, the mention list is the remaining suspect.
  • Separately from this PR: a create_node on an off-menu type landing on a turn the scenario expects to be held is worth a look in the transcripts of the failing reps. Either the turn was not held in those reps, or the hold does not cover a create by type. Both are main's behaviour, and the second would be a gap the scenario found by accident.

So: approve the code; before merging, either run the one-scenario isolation or accept the criterion as unshown for this scenario, knowingly.

Recommendation

APPROVE. The one confirmed defect from the first review is fixed and pinned, the scoring and matrix gaps are closed, and the docs now claim only what was measured. N1 to N3 are optional. The held-off-menu-node result is a decision for whoever merges, not a code change.

@malibio malibio left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@malibio

malibio commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Re-review addressed

The re-review approved with three suggestions and a nit.

Suggestion Outcome
N1 undeclared_core_keys and the store disagree on a flat key beside the node's own bucket Fixed. The helper lists nothing when the map already holds the type's bucket as an object, and the agreement test has that two-key case.
N2 The dry-run note says the key "is stored" Fixed. A dry run says it "would be stored as"; the test asserts the wording.
N3 A decisions scenario for "track progress on the Q4 cycle" Skipped. It is a new eval scenario with its own seeding and measurement, and ADR-038 already records that what Stage 2 does with these is unmeasured.
Nit "measured and not accepted" against ADR-038's "Costs" The test now says "recorded as its cost".

nodespace-core and nodespace-agent: 3958 tests pass. quality:fix clean.

Re-Review Decision

Decision: 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: held-off-menu-node is 3 of 3 on the baseline build and 2 of 3 on the final build, cause not isolated (see the previous comment and the re-review). One run of that scenario on current origin/main would separate this PR's changes from the eight merged commits.

@malibio

malibio commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

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 bun run merge 3655.

…a refused field key used to lose (closes #3603, closes #3632)
@malibio
malibio force-pushed the issue-3603-3632-create-shapes branch from f117e89 to 6fd13ea Compare October 6, 2026 09:44
@malibio
malibio merged commit e194969 into main Oct 6, 2026
@malibio
malibio deleted the issue-3603-3632-create-shapes branch October 6, 2026 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant