[malibio]

Closes #3268.

Three scenario failures were left on main after #3504. Each reproduced at 0/3 on b1d0e287 (gemma-4-e4b-q4km, database purged and daemon restarted between reps) before anything was changed.

What ships

Two production changes and one scenario expectation.

1. A type listing whose links escape their ids is kept (outcome-list-types). "What schemas do we have here?" ran search_nodes, and the model wrote a correct listing, but escaped the underscores of a type id inside its link: [Company Sold To](nodespace://company\_sold\_to). The escaped form matched no id a tool returned, so the fabricated-id guard replaced the whole reply with the request to confirm. Reply normalization now unescapes \_ and \- inside a nodespace:// URI before the guards read it (response_processing::unescape_uri_ids). The reply stands, and the existing type-listing backstop appends the types it left out. The guard is not weakened: the unescaped id is still compared by exact match against tool-result ids.

2. Schema Creation's description opens with "Start tracking:" (indirect-schema-freelance). "start tracking our planning cycles" reached retrieval as "start tracking planning cycles", which ranked Graph Editing 0.828, Relationship Management 0.800, Node Creation 0.786, Schema Creation 0.778. create_schema was not offered and the turn retried create_node until it gave up. The description said "keep track of" and never "start tracking". With the two-word opening it leads at 0.872.

3. skill-instance-request asserts what it is for. "Add Contoso Ltd to the companies we sell to." scores Bulk Import 0.742, Organization 0.741, Node Creation 0.737: which of them leads is chance, and the turn creates the record either way. The scenario named the skills that should lead and failed on that chance. It now asserts that Schema Creation does not lead and that a skill holding create_node is among the candidates, through a new { decision: "skill"; not; reaches } expectation. The issue's criterion allows this with the reason recorded, and it is recorded on the scenario.

What was tried and withdrawn

• A clause on Node Creation's description that made it lead the Contoso request by 0.010. That is a shipped description tuned to one sentence, in a fixture that is still in a small-business domain (Re-theme the decision eval's scenarios and seeded workspace into NodeSpace's developer-workflow domain #3513). Reverted.
• A wider Schema Creation opening ("Start tracking or begin tracking a new kind of thing: …"). The full decision run showed its cost: it lifted Schema Creation by about 0.03 on requests of every kind and took the lead from Graph Editing on "update Northwind Trading booking to April". The two-word opening wins the tracking requests by margins as large and gives that lead back.
• An exclusion on Node Deletion, suggested in review for an add request it leads. It did nothing for that request and cost a real deletion request its lead. Reverted.

Known costs of the opening

Both are on requests naming a spec, which Schema Creation's description lists. Both are recorded in the guards and in the seed's comment as costs not accepted. In both the needed tool is still offered, but without its declared fields: routing::declare_write_tool_fields declares them only from the candidates at the turn's top score, which is now Schema Creation. Neither request was run through the model (the decision fixture holds no spec record), so the effect on what Stage 2 does is unmeasured.

Request	main	This branch
mark the offline sync spec as signed off	Graph Editing 0.790, Schema Creation 0.773	Schema Creation 0.791, Graph Editing 0.790
add the offline sync spec to the specs for this cycle	Node Creation 0.841, Schema Creation 0.835	Schema Creation 0.846, Node Creation 0.841

Live measurement

gemma-4-e4b-q4km, three reps, database purged and daemon restarted between reps.

On the final production code (4b58d77a), targeted run:

Scenario	Fixture	Result
outcome-list-types	decisions	3/3
skill-instance-request	decisions	3/3 (create_node ran and landed in every rep; scored before the expectation gained its reaches half, which the recorded candidates also satisfy)
op-read-then-write	decisions	3/3 (Graph Editing leads, as on main)
skill-type-request-indirect	decisions	3/3
indirect-schema-freelance	routing	3/3
instance-not-schema-invoice	routing	3/3

Full runs, on the earlier wording (the wide opening and the Node Creation clause, both since withdrawn):

• eval:routing: 19/19 scenarios passed all three reps.
• eval:decisions: 22/24 passed all three reps. The two that flipped (op-read-then-write, outcome-follow-up-statement-keeps-its-answer) each failed one rep on Decode Error -3, an inference-engine failure in which the turn never reached the model.

The full fixtures were not re-run on the final wording. The targeted set covers the scenarios the two wording changes could reach.

Two observations from the traces, not addressed here: op-read-then-write passes while its update_node call errors (it grades the tool chosen, not the write), and indirect-schema-freelance passes while making three to five schema calls for one request.

Tests

• response_processing: unit tests for the unescape (ids in links and bare, escapes in prose and after the id left alone, ordering before the steps that read the URI).
• agent_loop: a listing with escaped ids is kept and completed; an escaped link to an unknown type name keeps its label.
• decisions.test.ts: the new expectation fails on the named skill leading, on no leader, and on no candidate that can create the record.
• live_skill_retrieval_stability (ignored, needs the embedding model), 24/24: start_tracking_requests_route_schema_creation, and three cost-side controls for the opening: tracking_one_existing_record_still_reaches_a_record_skill, updates_to_one_record_do_not_lead_with_schema_creation, adds_to_an_existing_list_keep_a_skill_that_can_create.
• completion_state_updates_route_graph_editing gains one request, and the agent_loop type-listing test helper takes its listing as a parameter.
• The Stage-2 prompt golden changes by Schema Creation's one description line.
• nodespace-agent 1005 passed; test:scripts 1340 passed.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3515 (issue #3268)

Review Type: Initial review (no earlier reviews or comments on the PR). Reviewed at c20a6327.

Summary

The change is small, atomic to the three folded-in failures, and a net improvement. The normalizer is correct and cannot weaken the fabricated-id guard. No Critical findings. Two Important findings concern what the new retrieval guards accept or leave unguarded, and the issue's acceptance criteria are all "3/3 in a live run", which this PR does not yet show.

What I ran: cargo-nextest -p nodespace-agent -E "test(response_processing) | test(type_listing)": 40 passed, including the four new tests. I did not run the ignored live_skill_retrieval_stability tests or any live model run (GPU in use), so every retrieval score quoted in the comments and PR body is unverified by this review.

Requirements Check

Acceptance criterion (#3268)	Status
Nested array-of-objects argument capture, with a parser test	Already checked off on the issue by earlier PRs. Not touched here.
Prose-in-arguments detected as a malformed call	Already checked off. Not touched here.
"What schemas/types do we have?" returns every schema, core and custom	Partly shown. The loop test proves the deterministic half: an escaped-id listing is kept and the backstop appends the missing types. Whether the live turn does so needs the model run.
Safety-net replacement says what failed in plain terms	Already checked off. Not touched here.
Scenario covering "what schemas do we have" on the default model	Already checked off (outcome-list-types exists).
outcome-list-types passes 3/3 in a full decisions run	Not shown. Needs the live run (PR body: pending). The code path for the measured cause is fixed and unit-tested.
indirect-schema-freelance passes 3/3 in a full routing run	Not shown. Needs the live run. Retrieval-level guard added (ignored test, not run here).
skill-instance-request passes 3/3 in a full decisions run	Not shown. Needs the live run. The fixture accepts Node Creation, Graph Editing or Organization as leader; the stated margin over Bulk Import is 0.010, so this is the criterion most at risk if Stage 1 words the query differently from the two wordings guarded.

None is unmet in code. The three folded-in criteria cannot be ticked, and the PR should not close the issue, until the live numbers are posted.

Verified as correct

• Unescaping cannot let an invented id through. unescape_uri_ids runs inside normalize_response_traced, before the guard at agent_loop.rs:3802. The guard still compares the unescaped id by exact match against tool-result ids. a\_b becomes a_b, which is either a real grounded id or still flagged. A double backslash leaves one behind and stays flagged. Grounding reads tool results and system messages only, never the model's text, so nothing the model writes can enter the grounded set.
• Ordering. Running before collapse_uri_labelled_links and fix_backtick_wrapped_uris is right: both then see the real id. All three other normalize_response call sites share the pipeline.
• create_node stays offered at rank 2 or 3. stage2_permitted_names (routing.rs:910) takes the union of eligible candidates' tools, so the claim in the test comment holds.
• Standards. No issue numbers in new comments, no lint suppression, no compatibility code. The golden changes by exactly the two description lines.

Findings

🔴 Critical

None.

🟡 Important

1. The single-record guard accepts an add request that leads with Node Deletion.
📁 packages/agent/tests/it/live_skill_retrieval_stability.rs:1208 (and the PR body)

The test comment records that "add the cache invalidation decision to our architecture decisions" leads with Node Deletion, and asserts only that Bulk Import does not lead and Node Creation is in the top 3. But stage2_permitted_names (routing.rs:917-927) admits destructive tools from the retrieval winner. On that request delete_node is on the Stage-2 surface for a turn that is a create, and the turn is recorded as routed to the deletion skill. The comment says "create_node stays on the surface, which is what is asserted" and does not say what else is on it.

This is probably not caused by this PR (Node Deletion's description is unchanged), but I could not measure before and after, and the test now writes the state down as acceptable. Principle: ADR-038 gates the expensive error hardest; a guard should not record a destructive lead on a non-destructive request as passing. Suggested, in order of preference:

• fix it here: Node Deletion's seed has the exclusion margin-penalty mechanism for verb-only overlaps, and "invalidation" on an "add" request is that shape; then assert no destructive skill leads any of the six;
• or, if it is left, say in the comment that delete_node is offered on that query and whether it was so before this change.

2. The Schema Creation change has no cost-side control.
📁 packages/agent/src/skill_pipeline.rs:239, packages/agent/tests/it/live_skill_retrieval_stability.rs:1168

The Node Creation change ships with a control for what it could cost (import_requests_still_route_bulk_import). The Schema Creation change, the larger one (a new opening clause, +0.088 on the target query), ships only with queries it should win. "Start tracking or begin tracking" now leads the description, and tracking verbs also appear in requests about a type that already exists: "start tracking the login timeout bug", "track this task's progress", "start tracking Contoso". Schema Creation leading those is the instance-versus-type failure skill-instance-request exists for, in the other direction, and tools: ["create_schema", "update_schema", "get_node"] carries no create tool if Node Creation drops out of the top 3. Principle: a wording change to a retrieval key is tested on both sides, as this file does elsewhere (every routing test has a control_ twin). Add a control asserting Node Creation or Graph Editing stays in the top RETRIEVAL_TOP_K on three or four tracking requests that name one existing record.

🟢 Suggestions

3. The regex can unescape text past the end of the id.
📁 packages/agent/src/local_agent/response_processing.rs:18

The match runs to whitespace, ) or ], and the replacement applies to the whole match. nodespace://task|due\_date (a table cell with no spaces) or nodespace://abc,my\_field loses the escape in the adjacent prose, which the doc comment and leaves_escapes_outside_a_uri_unchanged say is left alone. The effect is cosmetic and the guard is unaffected. This is also a third definition of where a URI ends, beside node_uri_re (agent_loop.rs:2439) and replace_status_outside_special (line 280). Bounding the match to id characters and the two escapes, for example nodespace://(?:[A-Za-z0-9_:-]|\\[_-])+, would make the code match its comment.

4. Two behaviours the change relies on are not pinned by a test.
📁 packages/agent/src/local_agent/response_processing.rs:479

• Ordering: no test covers [nodespace://a\_b](nodespace://a\_b) collapsing to the bare id, or a backtick-wrapped escaped URI. A reorder of the pipeline would pass every current test.
• An escaped link to an ungrounded type name ([Event Venue](nodespace://event\_venue)) used to be replaced whole and is now unlinked with its label kept, since type_name_target_re now matches. That is consistent with the existing policy and is a real behaviour change; one loop-level test would record it.

5. The single-record guard measures one rep.
📁 packages/agent/tests/it/live_skill_retrieval_stability.rs:1222

It calls scored_ranking once per query, while routing_misses checks every rep of repeated_rankings, and this file exists because rankings were found to vary across identical calls. With a stated margin of 0.010, use the repeated form.

6. Nit: "The measured reply".
📁 packages/agent/src/local_agent/agent_loop.rs:10778

The test's reply uses developer-domain ids; the measured one was company\_sold\_to. "The shape of the measured reply" would be accurate.

7. Nit: the new Node Creation clause as a Purpose: line.
📁 packages/agent/src/skill_pipeline.rs:155

"added to the ones already listed" follows the description's own list (tasks, text notes, Spec, ADR, Ticket), so to the Stage-2 model the antecedent reads as that list, not the user's. The wording is tuned for the embedding and the comment says so; worth a glance at the Stage-2 trace in the live run.

Recommendation

COMMENT. The code is sound and I would approve it on its own. Before merge: post the three live 3/3 results the issue's criteria require, and address or answer findings 1 and 2. Findings 3 to 7 are optional.

🤖 Generated with Claude Code

[malibio]

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

[malibio]

Review addressed (55b9ed53)

1. Add request that leads with Node Deletion — answered, not fixed here. Node Deletion scores 0.840 on "add the cache invalidation decision to our architecture decisions" whatever Node Creation's description says, so it led before this PR; nothing in its own seed changed. I tried the suggested fix: an exclusion on Node Deletion naming the adding verbs ("Add them, create them, record them, log them."). It left that score at 0.840 and took about 0.02 off real deletions, costing "remove all the resolved incidents" its lead in remove_requests_mentioning_a_state_route_node_deletion. Reverted. The guard's comment now says delete_node is offered on that request, that it predates the change, and that the exclusion was measured and rejected. It is recorded as a standing defect, not as acceptable.

2. Cost-side control for Schema Creation — added. tracking_one_existing_record_still_reaches_a_record_skill: four requests to track one existing record must keep Node Creation or Graph Editing in the top 3 on every rep. All four pass. Graph Editing leads three; Schema Creation leads "start tracking the offline sync spec's sign-off" (0.852, Graph Editing 0.811, Node Creation 0.792), with both record skills still offered.

3. Regex past the end of the id — fixed. The match is now nodespace://(?:[A-Za-z0-9_:-]|\\[_-])+, so it ends where the id does. New test: leaves_an_escape_after_the_end_of_a_uri_unchanged.

4. Unpinned behaviours — pinned. unescapes_a_uri_id_before_the_steps_that_read_it covers the URI-labelled link and the backtick-wrapped URI. an_escaped_link_to_an_unknown_type_name_keeps_its_label records the unlink behaviour at loop level.

5. One rep — fixed. The single-record guard checks every rep of repeated_rankings.

6. "The measured reply" — reworded to "The shape of the measured reply".

7. Purpose: reading of the new clause — no code change. I will read the Stage-2 trace for it in the live run.

Tests: nodespace-agent 1004 passed; live retrieval suite 24/24.

Still pending before merge: the live three-rep runs on the fixed build. The GPU is held by another session's model.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: The regex was rewritten, not just commented, and two tests assert new behaviour (a new live control and a loop-level unlink test). Those are logic changes the first review did not see.

[malibio]

Pragmatic Code Review: PR #3515 (issue #3268)

Review Type: Re-Review. The first review was at c20a6327; this one covers 55b9ed53 only (git diff c20a6327 55b9ed53: response_processing.rs, the tests module of agent_loop.rs, live_skill_retrieval_stability.rs).

Previous Review Summary

	Count
Previous recommendations	7
Addressed	4 (findings 2, 3, 5, 6)
Partly addressed	1 (finding 4: the loop-level half is pinned, the ordering half is not)
Answered without a code fix	2 (finding 1, adequate for merge; finding 7, waits on the live trace)
Not addressed	0
New issues	3, all Suggestions

What I ran: cargo-nextest -p nodespace-agent -E "test(response_processing) | test(type_listing) | test(an_escaped_link)": 43 passed, including the three tests this commit adds. I did not run the ignored live_skill_retrieval_stability tests or any model, so every retrieval score quoted in the comments and in the author's response is still unverified by review. The claims about what the routing code offers are checked against the code.

Status of the earlier findings

#	Finding	Status
1	Single-record guard accepts an add request that leads with Node Deletion	Answered, adequate for merge. See below.
2	No cost-side control for the Schema Creation change	Addressed. tracking_one_existing_record_still_reaches_a_record_skill added. See new finding B.
3	Regex unescapes past the end of the id	Addressed. See the regex check below.
4	Two behaviours not pinned by a test	Partly. The unlink behaviour is pinned. The ordering is not: see new finding A.
5	Single-record guard measured one rep	Addressed. It now checks every rep of repeated_rankings, which asks for RETRIEVAL_TOP_K, so .any(..) over the whole list is the same top-3 check as before. An empty ranking counts as a miss.
6	"The measured reply"	Addressed.
7	The new Node Creation clause read as a Purpose: line	Open by agreement. No code change; to be read in the Stage-2 trace of the live run.

Finding 1, in detail. The first review offered two ways out: fix it, or say in the comment that delete_node is offered and whether that predates the change. The author tried the first, measured it, and took the second. I checked the three things the new comment asserts:

• "the winner's destructive tool is offered, so delete_node is on the surface": accurate. stage2_permitted_names (routing.rs:910-930) keeps a destructive tool only when c.score >= top_tool_bearing_score, Node Deletion whitelists delete_node (skill_pipeline.rs:403), and 0.840 clears DESTRUCTIVE_SKILL_SCORE_BAR (0.45).
• "scores 0.840 on it whatever Node Creation's description says": accurate by construction. A skill's score comes from its own description and its own exclusion (skill_ops.rs:404), and Node Deletion has neither changed in this PR.
• "so it led before the clause was added": an inference, not a measurement. It also needs Schema Creation, whose description this PR changes too, to have been under 0.840 on that query before. That is very likely and is not shown.

That is enough for merge: the defect is not introduced here, the one fix suggested was tried and cost a real deletion request its lead, and the test no longer describes the state as fine. Two things I would still ask for, neither blocking:

• when the live runs are posted, add the one-line ranking of that query on b1d0e287, so "it led before" is a measured statement;
• the test comment is now the only record of an add request that offers delete_node. A comment is not a place work gets picked up from. Whether to file it is the maintainer's call; I would.

The rewritten regex

nodespace://(?:[A-Za-z0-9_:-]|\\[_-])+ (response_processing.rs:20). Checked by reading and by the passing tests:

• Matches a URI's id characters, :, and a backslash only when _ or - follows it. nodespace://design\_decision, nodespace://a1b2\-c3d4 and nodespace://schema:release\_train are all unescaped. The : is needed for the last one.
• Stops at anything else: whitespace, ), ], |, ,, ., a backtick, *, and a backslash before any other character. nodespace://task|due\_date and nodespace://abc\*def are left as written. A double backslash (a\\_b) stops the match at the first backslash, so both stay and the id is still flagged as invented. That direction is the safe one.
• Trailing punctuation. A trailing : is inside the match (nodespace://abc: next), but the replacement only rewrites \_ and \-, so nothing changes. In the measured reply shape, (nodespace://planning\_cycle): tracks…, the ) ends the match first.
• The guard is not weakened. The unescaped id is still compared by exact match against tool-result ids; nothing the model writes enters the grounded set.
• What it can still over-reach on: prose joined to the id by :, - or _ with no other separator, such as nodespace://task:due\_date. I could not construct a reply where that matters, and the effect would be one lost backslash in prose. Not worth more code.

One difference from the old pattern: it now matches every nodespace:// URI, not only those holding a backslash. The closure returns the text unchanged for those, and strippers_fired records the step only when the output differs, so tracing is unaffected.

New findings

🔴 Critical

None.

🟡 Important

None.

🟢 Suggestions

A. The ordering test passes under either order.
📁 packages/agent/src/local_agent/response_processing.rs:522 (unescapes_a_uri_id_before_the_steps_that_read_it)

The name and comment say the unescape must run before the two steps that read the URI. Neither assertion depends on that. In [nodespace://a\_b](nodespace://a\_b) the label and the target are escaped identically, so collapse_uri_labelled_links finds them equal and collapses them with the backslashes still in; unescaping afterwards gives the same nodespace://a_b. The backtick case is the same: strip the backticks, then unescape. Move unescape_uri_ids to the end of the pipeline and this test still passes, which is what finding 4 asked to prevent.

The order only matters when the two sides differ. One changed input pins it: [nodespace://a_b](nodespace://a\_b) collapses to nodespace://a_b only if the unescape ran first. Principle: a test should fail when the property in its name is broken.

B. The new control accepts Schema Creation leading a request about an existing record, and says so only in the PR thread.
📁 packages/agent/tests/it/live_skill_retrieval_stability.rs:1193-1228

The assertion is sound for what it claims: Node Creation and Graph Editing both whitelist create_node and update_node (skill_pipeline.rs:195, :287), so either one in the top 3 keeps both write tools reachable, and Schema Creation holds neither (:240). The check runs on every rep.

It does not look at who leads. The author's response reports that "start tracking the offline sync spec's sign-off" leads with Schema Creation at 0.852, so that turn is recorded as routed to the type skill with create_schema on its surface. That is the instance-versus-type confusion the control exists for, at rank 1, on one of four queries. It is a much cheaper error than finding 1 (create_schema removes nothing, and the record tools are still offered), and I would not hold the PR for it. The single-record guard's comment records its two non-leading cases with scores; this one should do the same, so the next person to tune Schema Creation's opening clause knows the starting state. Worth a look at that query in the live run as well.

C. Nit: two comments now disagree about what an id holds.
📁 packages/agent/src/local_agent/response_processing.rs:14-20 and :156

The regex's doc comment says it matches "the characters an id can hold", and the class includes :. Twelve lines down, unescape_uri_ids says "An id is alphanumeric plus - and _", as do node_uri_re and unlink_ungrounded_node_links in agent_loop.rs. The : is there for the schema: prefix of a type-name target, not because an id holds one. Saying so in the regex's comment removes the contradiction.

Tests added in this commit, against their names

Test	Asserts what it claims?
leaves_an_escape_after_the_end_of_a_uri_unchanged	Yes. Fails on the old pattern, passes on the new.
unescapes_a_uri_id_before_the_steps_that_read_it	The outputs are right; the "before" is not tested (finding A).
an_escaped_link_to_an_unknown_type_name_keeps_its_label	Yes. Without the unescape, release\_train fails type_name_target_re and the reply is replaced with the request to confirm; with it, the link is reduced to its label. The search is narrowed ("query":"release"), so the listing backstop does not interfere.
tracking_one_existing_record_still_reaches_a_record_skill	Yes, for presence in the top 3 (finding B on what it leaves out). Not run here.
single_record_adds_do_not_lead_with_bulk_import (changed)	Yes, now on every rep. Not run here.

No standards issues in the commit: no issue numbers in comments, no lint suppression, no compatibility code, no node-type or Pro-boundary surface touched.

Recommendation

COMMENT. The commit resolves what the first review asked for, the regex is correct, and nothing in it blocks. I would approve the code as it stands.

Before merge, unchanged from the first review: post the three live 3/3 results the issue's criteria require (outcome-list-types, indirect-schema-freelance, skill-instance-request). With them, please include the b1d0e287 ranking for the cache-invalidation query (finding 1) and a read of the Stage-2 trace for finding 7. Findings A to C are optional; A is a one-line change and worth taking.

🤖 Generated with Claude Code

[malibio]

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

[malibio]

Re-review addressed (67349266)

• A. Ordering test — taken. The link now escapes only its target ([nodespace://a_b](nodespace://a\_b)), so it collapses only when the unescape ran first.
• B. Who leads the tracking control — taken. The test's comment records that Schema Creation leads "start tracking the offline sync spec's sign-off" (0.852, Graph Editing 0.811, Node Creation 0.792) and that the lead is not asserted.
• C. What an id holds — taken. The regex comment says the : is the schema: prefix of a type-name target.

Still to post before merge, with the live runs: the three scenarios at three reps, the b1d0e287 ranking for the cache-invalidation query, and a read of the Stage-2 trace for the new Node Creation clause.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: one test input and two comments changed. No production code changed in this commit.

[malibio]

Pragmatic Code Review: PR #3515 (issue #3268)

Review Type: Re-Review (second). The last review was at 55b9ed53. This one covers 67349266 (test and comment changes) and d3be9aab (Schema Creation's description opening narrowed, a new live guard, a regenerated golden). The merge of main (03f5047e) is not reviewed.

Previous Review Summary

	Count
Previous recommendations (second review)	3 suggestions (A, B, C), plus 2 requests carried with the live runs
Addressed	4 (A, B, C, and the before-measurement for the cache-invalidation query)
Not addressed	0
Still open by agreement	1 (first review's finding 7: read the Stage-2 trace for the Node Creation clause in the live run)
New issues	1 Important, 3 Suggestions

What I ran: cargo-nextest -p nodespace-agent -E "test(response_processing) | test(prompt_assembly_snapshot) | test(skill_pipeline)": 75 passed, including unescapes_a_uri_id_before_the_steps_that_read_it and stage2_candidate_block_matches_golden.

What I did not run: the ignored live_skill_retrieval_stability tests, or any model. Every retrieval score quoted in these commits is unverified by this review. I checked the scores only for agreement with each other, and checked the claims about what routing offers and records against the code.

Status of the second review's suggestions

#	Suggestion	Status
A	The ordering test passed under either order	Taken. The input is now [nodespace://a_b](nodespace://a\_b). collapse_uri_labelled_links (response_processing.rs:179) collapses only when label and target are equal, and they are equal only after the unescape. Move the unescape after it and the link is left as a link, so the test fails. The backtick assertion is still order-independent, which is fine: one assertion carries the name.
B	Record who leads the tracking control	Taken. The comment names the query, the three scores and that the lead is not asserted. d3be9aab updates the score to 0.853 and points at the new guard.
C	Two comments disagreed about what an id holds	Taken. The regex comment now says the : is the schema: prefix of a type-name target, which agrees with "An id is alphanumeric plus - and _" at :156.
(1)	Measure, not infer, that Node Deletion led the cache-invalidation add before this PR	Taken. The comment now reads "Measured without the clause, Node Deletion led it at the same 0.840 and Node Creation was outside the top 5".

d3be9aab: the description and its comment

• Description, comment and golden agree. The description opens Start tracking: set up a structured way to keep track of… (skill_pipeline.rs:249). The comment says "opens with the tracking verb itself" and "Two words and no more". The golden differs from the previous one by that one Purpose: line and the snapshot test passes.
• The three tests the comment names exist under those names (live_skill_retrieval_stability.rs:1168, :1206, :1252).
• Scores agree across the two files. The rejected opening is quoted as 0.799 against Graph Editing's 0.793 on "update Northwind Trading booking to April", and as a lift of "about 0.03 on requests of every kind", in both skill_pipeline.rs:236-240 and the test comment at :1238-1241. The tracking control's 0.852 became 0.853 in the same commit that changed the description. The single-record guard's comment moved from "leads four of six, loses the spec add to Schema Creation by 0.010" to "leads five of six, the spec add by 0.002", which is the direction a lower Schema Creation score predicts.
• Standards. No issue numbers in comments, no lint suppression, no compatibility code, nothing touching node types or the Pro boundary. The eprintln! in the new test is the file's existing pattern (routing_misses does the same), not a leftover diagnostic.

Findings

🔴 Critical

None.

🟡 Important

1. The request left out of the new guard is very likely a lead this PR moves, and the comment does not say so.
📁 packages/agent/tests/it/live_skill_retrieval_stability.rs:1245-1249

The comment records that "mark the offline sync spec as signed off" leads with Schema Creation by 0.001 (0.791, Graph Editing 0.790), and that it is "not held, and not asserted". As a record of the current state it is honest: the query, both scores and the omission are written down, and the commit message repeats it. What it leaves out is the one fact a reader needs to judge it: who led that request on main.

The comments themselves point to the answer. "No opening measured both won the tracking requests and stayed under Graph Editing there" implies that with no opening, which is main, Graph Editing led. The numbers agree: the rejected opening lifted Schema Creation by about 0.03 everywhere, and the single-record comment shows the narrowing gave back about 0.012 of that on the spec add (0.010 behind to 0.002 ahead). So some of the lift probably remains, and a 0.001 lead does not survive without it. That is an inference from the quoted scores, not a measurement. If it holds, this is a completion-state update, the request shape completion_state_updates_route_graph_editing exists for, whose leader this PR changes. The cache-invalidation case in the same file is recorded with its before-measurement ("Measured without the clause…") and called a standing defect. This one should be recorded the same way.

What the code does when Schema Creation leads here, so the cost is stated correctly:

• Offered tools do not change with who leads. stage2_permitted_names (routing.rs:910-930) takes the union of every gate-clearing candidate's tools and restricts only tools that remove user data to the winner. Schema Creation's whitelist (create_schema, update_schema, get_node) holds none of those (tools.rs:2605: only delete_node and merge_conflict). With Graph Editing second at 0.790, update_node is on the surface, so "update_node is still offered" is correct. create_schema and update_schema are on it too, as they would be with Schema Creation second.
• What changes is the record and the order. record_skill (decisions.rs:266) writes selected from leading_tool_bearing_candidate, so the turn's skill decision is recorded as Schema Creation. render_candidates_for_prompt (routing.rs:791) renders it as Candidate 1, so its schema-editing procedure is the first one the model reads on a request to set a field.
• At 0.001 the leader is not stable. This file exists because identical calls were found to rank differently. The comment says "leads"; some reps may not.

No destructive tool is involved and the write tool is offered, so I would not hold the code for it. Three things to do before merge, in order of value:

• Say in the comment who led that request before this PR, as a measurement. If Graph Editing led, call it what the neighbouring comment calls its case: a cost of this change that the test records and does not accept.
• Assert the half that is claimed. "update_node is still offered" is a sentence, not a check. Adding the request to completion_state_updates_route_graph_editing (top 3, rank_one: false) makes it one, and that test is where a "mark X as " request belongs.
• Include the request in the live three-rep run, and report what Stage 2 called. A model that reads Schema Creation's procedure first and answers "signed off" with update_schema is the failure to look for.

Principle: a guard that leaves out an input it is known to fail on has to say whether the change under test caused the failure. Otherwise the test's name ("updates to one record do not lead with Schema Creation") claims more than was measured.

🟢 Suggestions

2. The new guard checks who does not lead, not whether the turn can write.
📁 packages/agent/tests/it/live_skill_retrieval_stability.rs:1257-1273

The assertion matches the name: Schema Creation is not first on any rep, and an empty ranking is not counted as a miss, which is right for this claim. But the comment says the second request ("move the offline sync spec's review to next week") leads with Conflict Journal, and that the narrowing "leaves both leaders where they were". Conflict Journal whitelists no update_node (skill_pipeline.rs:444). The comment does not say whether Graph Editing or Node Creation is in the top 3 there, and the test does not check it. That lead is not this PR's doing. Still, the test now writes down "an update leads with the conflict skill" as a state it passes on. The holds closure from tracking_one_existing_record_still_reaches_a_record_skill (a record skill in the top 3 on every rep) would cover it on these three requests, or the comment can state where Graph Editing places.

3. Nit: "the same margins" against the numbers beside it.
📁 packages/agent/src/skill_pipeline.rs:232-237, packages/agent/tests/it/live_skill_retrieval_stability.rs:1242

The comment before this commit gave the wide opening 0.866 on "start tracking planning cycles" and leads of "0.01 to 0.05". This one gives the narrow opening 0.872 and "0.01 to 0.06", and says both openings win "by the same margins". The narrow one does slightly better by the author's own figures. "By margins as large" is accurate.

4. Nit: two leftovers from the edit.
📁 packages/agent/tests/it/live_skill_retrieval_stability.rs:1193, :1298

• :1193 still says "opening with the tracking verbs"; the description now opens with one verb, and skill_pipeline.rs:227 was changed to say so.
• :1298 breaks a line after "An exclusion on Node Deletion" where the paragraph was spliced.

Tests in these commits, against their names

Test	Asserts what it claims?
unescapes_a_uri_id_before_the_steps_that_read_it (changed)	Yes, now. Run here, passes.
updates_to_one_record_do_not_lead_with_schema_creation (new)	Yes for the three requests listed, on every rep. The name is general and one measured request is excluded (finding 1). Not run here.
tracking_one_existing_record_still_reaches_a_record_skill (comment only)	Unchanged in logic. Not run here.
stage2_candidate_block_matches_golden	Yes. Run here, passes.

Recommendation

COMMENT. The narrowing is the right response to what the decision run showed, the three suggestions were taken as described, and the code, comment and golden agree. Nothing here blocks on the code.

Before merge, with the live three-rep results already owed: the before-ranking for "mark the offline sync spec as signed off" and that request in the live run (finding 1). Turning its "update_node is still offered" into an assertion is a one-line addition to an existing test. Findings 2 to 4 are optional. Still open from the first review: the Stage-2 trace read for the Node Creation clause.

🤖 Generated with Claude Code

[malibio]

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

[malibio]

Third review addressed, and one fix withdrawn (4b58d77a)

Scope change: the Node Creation clause is reverted

The clause made Node Creation lead "add Contoso Ltd to companies we sell to" by 0.010, in a band where three skills sit within 0.005 of each other. That is a shipped description tuned to one sentence, so it is reverted, along with the two live guards that belonged to it (single_record_adds_do_not_lead_with_bulk_import, import_requests_still_route_bulk_import).

skill-instance-request now asserts what its title says, that the type skill does not lead, through a new { decision: "skill"; not: RegExp } expectation. The reason is recorded on the scenario. The turn creates the record whichever near-tied skill leads. decisions.test.ts covers the new branch: a near-tied skill leading passes, Schema Creation leading fails, nothing clearing its bar fails.

This PR therefore ships two production changes, not three: the id unescape, and Schema Creation's two-word opening.

Review findings

1. The request left out of the update guard — measured, and recorded as a cost of this change. On main's wording, "mark the offline sync spec as signed off" led with Graph Editing, 0.790 to Schema Creation's 0.773. With the opening it leads with Schema Creation by 0.001 (0.791 to 0.790). The guard's comment now says that, as a cost it records and does not accept. The request is added to completion_state_updates_route_graph_editing, so "Graph Editing still reaches Stage 2, and update_node is offered" is an assertion.

Not done: a live three-rep run of that request. The decision fixture's seeded workspace holds no spec record, so the request cannot succeed there. It belongs in the re-themed fixture (#3513).

2. The update guard checked only who does not lead — fixed. Each request must also keep Graph Editing or Node Creation in the top 3 on every rep. On "move the offline sync spec's review to next week", Conflict Journal leads on main and on this branch alike (0.825); Graph Editing is third here.

3. "the same margins" — reworded in both files.

4. Leftovers — "tracking verbs" is now "tracking verb". The spliced paragraph went with the removed test.

Other measurements on main's wording against this branch's

Request	main	This branch
start tracking planning cycles	Graph Editing 0.828, Schema Creation fourth at 0.778	Schema Creation 0.872
update Northwind Trading booking to April	Graph Editing 0.793, Schema Creation 0.774	Graph Editing 0.793, Schema Creation 0.787
change the owner of the Q4 cycle to Priya	Graph Editing 0.794, Schema Creation 0.759	Graph Editing 0.794, Schema Creation 0.766
mark the offline sync spec as signed off	Graph Editing 0.790, Schema Creation 0.773	Schema Creation 0.791, Graph Editing 0.790

Tests: nodespace-agent 1005 passed, test:scripts 1340 passed, live retrieval suite 23/23. The targeted live runs on this commit are in progress and will be posted with the PR description update.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: a production description was reverted, a new expectation shape and scoring branch were added to the decision fixture, and two live guards changed their assertions.

[malibio]

Pragmatic Code Review: PR #3515 (issue #3268)

Review Type: Re-Review (third). The last review was at d3be9aab. This one covers 4b58d77a only: the Node Creation clause reverted with its two live guards, a new must-not-lead skill expectation in the decision fixture, and the third review's four findings.

Previous Review Summary

	Count
Previous recommendations (third review)	4 (1 Important, 3 Suggestions)
Addressed	4 (finding 1 in its first two parts; the live run of that request is deferred with a reason)
Not addressed	0
Closed by the revert	1 (first review's finding 7, the Stage-2 reading of the Node Creation clause: the clause is gone)
New issues	1 Important, 3 Suggestions

What I ran, from the worktree:

• bun run test:scripts: 1340 passed, 0 failed, including the new a must-not-lead skill expectation fails only on the named skill.
• cargo-nextest -p nodespace-agent -E "test(prompt_assembly_snapshot) | test(skill_pipeline)": 42 passed, including stage2_candidate_block_matches_golden.
• bunx tsc --noEmit -p scripts/tsconfig.json and eslint on the two fixture files: clean.

What I did not run: the ignored live_skill_retrieval_stability tests, any daemon, any model. Every retrieval score quoted in this commit and in the author's response is unverified by this review. I checked them for agreement with each other and checked the claims about what routing offers against the code. The live three-rep runs on this commit are in progress and their absence is not a finding.

The revert

• Node Creation is back to main. git diff origin/main -- packages/agent/src/skill_pipeline.rs shows one hunk, Schema Creation's comment and description. Node Creation's description and the comment above it are identical to main.
• The golden agrees. Against main it differs by one line, Schema Creation's Purpose:.
• No comment refers to what was removed. No hit in packages/agent or the fixtures for single_record_adds_do_not_lead_with_bulk_import, import_requests_still_route_bulk_import, "already listed" as the clause, or "one more named". The reference to control_conflict_requests_still_route_conflict_journal in the removed comment went with it; that test is main's own and stays.
• The decision is within the issue's criterion. Chat 'what schemas do we have' still fails, plus two skill-retrieval misses on plain requests (type listing, 'start tracking', single-instance add) #3268 reads "skill-instance-request passes 3/3 […], or the scenario's expectation is changed with the reason recorded". The reason is recorded on the scenario.

The new expectation: { decision: "skill"; not: RegExp }

📁 scripts/eval/fixtures/decisions.ts:85-92, :1263-1293

• Typed correctly. Both skill members share decision: "skill", and "not" in expected narrows the pair, so the expected.matches reads after it are on the matches member only. The schema branch's "onMenu" in expected narrowing is unaffected. tsc passes with no cast and no any.
• The branches before it apply to it. No skill decision recorded fails (:1237-1245). selected === null fails (:1264-1271) before the not test, so a turn that fell open to the full surface is not a pass by default. The unit test covers the second.
• Nothing else reads matches for a skill decision. extra stores expected whole (:1404), and summary reads only expected.decision and heldToOfferedTypes (:1462, :1474-1476), so the scenario is still counted under "Skill routing" and "Instance-vs-type boundary". The runner and routing.ts do not read this fixture's expected. There is no required-tools reader for skill expectations.
• The failure message names the leader, the pattern and the retrieved candidates, which is what a failed rep needs.

Findings

🔴 Critical

None.

🟡 Important

1. The recorded reason rests on a 0.003 margin that nothing asserts any more, and this PR moves the skill on the other side of it.
📁 scripts/eval/fixtures/decisions.ts:625-635, packages/agent/tests/it/live_skill_retrieval_stability.rs (the removed single_record_adds_do_not_lead_with_bulk_import)

The reason says: "The turn calls create_node and creates the record whichever of them leads, since the candidates' tools are offered together." That is true only while Node Creation is among the candidates. On the scores the comment itself gives, it is third at 0.737 and Schema Creation is fourth at 0.734:

• Of the three that place, only Node Creation holds create_node. Bulk Import holds create_nodes_from_markdown alone (skill_pipeline.rs:469) and Organization holds create_relationship and read tools (:482).
• select_candidates (routing.rs:68-86) takes a fourth candidate only when a read-only skill that does not lead sits in the top 3. Bulk Import, Organization and Schema Creation all write, so there is no fourth here.
• So if Schema Creation passes Node Creation by 0.003, the surface is create_nodes_from_markdown, create_relationship, create_schema, update_schema and read tools. The turn cannot create the record, the type tool is offered, and the scenario passes, because Bulk Import still leads and /schema/i does not match it.

That is the failure the scenario exists for (a single-instance request reaching the type skill's tools), one rank below where the expectation looks. Two things make it more than hypothetical:

• This PR raises Schema Creation's score on requests with no tracking verb. The author's own table shows +0.013, +0.007 and +0.018 on three such requests. The comment does not say whether the 0.734 was measured on main's wording or with the opening. If on main's, any of those lifts puts Schema Creation third or better here.
• The guard removed in this commit asserted two things on this request: Bulk Import does not lead (the tuned half, rightly dropped), and Node Creation is in the top 3 on every rep. The second half was not tuning; it is the fact the recorded reason depends on. No live guard and no scenario asserts it now. The Schema Creation opening has cost-side controls for tracking requests and for updates, and none for adds.

The decision to stop tuning Node Creation's description is sound and I am not asking for it back. What is needed is that the weakened expectation still fails when the record cannot be written. In order of value:

• Make the scenario assert the sentence its reason relies on. Either the turn called create_node without error and did not call create_schema (the turn-level records are already in TurnRecord, as assertNoDuplicate uses them), or a skill matching /node creation|graph editing/i is in decision.candidates. The second keeps it a pure skill-decision assertion: not on the leader, plus presence of a record skill.
• Keep a retrieval-level control for the opening on adds: the two Contoso wordings in a guard shaped like updates_to_one_record_do_not_lead_with_schema_creation (Schema Creation does not lead, and Node Creation or Graph Editing is in the top 3 on every rep). That pins nobody's lead in the tied band.
• State in the comment which build the four scores are from, and give Schema Creation's distance from third (0.003), not only the top three's spread.

If the live three-rep run on this commit shows create_node called on every rep, that answers the question for today and the first bullet makes it stay answered. Principle: an expectation relaxed "because the outcome is right either way" has to check the outcome, or it stops failing on the failure it was written for.

🟢 Suggestions

2. The pattern is wider than the skill it names.
📁 scripts/eval/fixtures/decisions.ts:635

/schema/i fails on any leading skill with "schema" in its title, and passes on every other leader: Node Deletion, Conflict Journal or Node Merge leading an add would pass. With today's scores none of those is near, and the scenario's title is about Schema Creation only, so this is consistent with the stated intent. /^schema creation$/i would say exactly what is meant; finding 1's presence check covers the rest.

3. The opening's one recorded cost lives only in the test file.
📁 packages/agent/src/skill_pipeline.rs:205-224

The seed comment explains why the opening is two words and names its three guards. It does not mention that "mark the offline sync spec as signed off" now leads with Schema Creation by 0.001, which the test comment records as a cost not accepted. The seed comment is where the next person tuning that description starts, so one sentence there, or a pointer to the guard's comment, puts the cost where the decision is.

4. Nit: the PR title and description still describe three production changes.
The title says "route start-tracking and single-record requests to their skills", and the body describes the Node Creation clause, the two removed guards, 0.866 for the wide opening and "opens with the tracking verbs". The title becomes the squash commit on main. The author's response already says the description update is coming with the live runs; the title needs it too.

5. Nit: the unit test's name.
📁 scripts/eval/fixtures/decisions.test.ts:219

"fails only on the named skill" and the last assertion (a null leader also fails) disagree by one word. "fails on the named skill and on no leader" matches the body.

Third review's findings, against the code

#	Finding	Status
1	The request left out of the update guard is a lead this PR moves, and the comment did not say so	Addressed, two of three parts. The comment now gives the before-measurement (Graph Editing 0.790 to 0.773 without the opening) and calls it a cost recorded and not accepted (live_skill_retrieval_stability.rs:1246-1253). The request is in completion_state_updates_route_graph_editing (:487). The live three-rep run of it is deferred: the fixture's workspace holds no spec record. That is a fair reason, and it leaves the Stage-2 behaviour on that request unmeasured.
2	The update guard checked only who does not lead	Addressed. holds now requires Schema Creation not first and Graph Editing or Node Creation in the list, on every rep (:1274-1282). An empty ranking now counts as a miss, which is right for the second half. Both named skills hold update_node (skill_pipeline.rs:173, :275).
3	"the same margins"	Addressed in both files ("margins no larger", "margins as large").
4	Two leftovers	Addressed. "tracking verb" at :1194; the spliced paragraph went with the removed test.

Does the added request assert what the other comment says? The comment at :1251-1253 says the completion-state test "asserts Graph Editing still reaches Stage 2 on it, so update_node is offered". The test calls routing_misses(.., "Graph Editing", false), which requires Graph Editing among the RETRIEVAL_TOP_K names on every rep (:445-453). That is presence in the top 3, which is what the file means by reaching Stage 2 throughout, and stage2_permitted_names offers the union of the eligible candidates' tools. So yes, with the usual limit of these guards: they check rank, not the score bar, and at 0.790 the bar is not in question.

Tests in this commit, against their names

Test	Asserts what it claims?
a must-not-lead skill expectation fails only on the named skill (new)	Yes for the four cases: two near-tied leaders pass, Schema Creation fails, a null leader fails. It scores the real scenario, so it also pins that skill-instance-request carries this expectation. Run here, passes. See nit 5 on the name.
completion_state_updates_route_graph_editing (one request added)	Yes, top 3 on every rep. Not run here.
updates_to_one_record_do_not_lead_with_schema_creation (changed)	Yes, both halves, and the failure message names both. Not run here.
stage2_candidate_block_matches_golden	Yes. Run here, passes.

Standards

No issue numbers in code comments in the four touched source files. No lint suppression, no any, no compatibility code, no leftover diagnostics (the eprintln! calls are the file's existing pattern). Nothing touches node types or the Pro boundary. The change is atomic: one revert and the fixture change that replaces it, plus the review items.

Recommendation

COMMENT. The revert is clean and complete, the new expectation is typed and scored correctly, and the third review's findings are addressed as claimed. Nothing blocks on correctness of the code as written.

Before merge: finding 1. The scenario's recorded reason is "the record is created whichever skill leads", the scenario does not check that, the guard that did was removed here, and the margin it depends on is 0.003 against a skill whose score this PR raises. One added condition on the scenario, or two requests in an existing-shaped guard, closes it. With the live runs already owed, report whether create_node ran on each rep of skill-instance-request and which build the 0.734 is from. Findings 2 to 5 are optional; 4 should be done before the squash.

🤖 Generated with Claude Code

[malibio]

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

[malibio]

Fourth review addressed (6a421699)

1. The relaxed scenario did not check that the record can be created — fixed. The expectation is now { decision: "skill", not: /^schema creation$/i, reaches: /^(node creation|graph editing)$/i }: Schema Creation must not lead, and a skill holding create_node must be among the candidates. The unit test covers the case described in the review (Bulk Import still leads, Schema Creation has taken Node Creation's place: fails).

A retrieval-level control is added too: adds_to_an_existing_list_keep_a_skill_that_can_create, on four add requests, every rep.

Which build the scores are from: 0.742 / 0.741 / 0.737 are the same on main and here, since none of those three descriptions changed. On this branch the next two are Graph Editing 0.732 and Schema Creation 0.730; the opening lowers Schema Creation on this request (it was 0.734 on main). In the live run on 4b58d77a, create_node ran and landed in all three reps.

That control found a second cost of the opening, recorded and not accepted: "add the offline sync spec to the specs for this cycle" led with Node Creation (0.841 to 0.835) and now leads with Schema Creation (0.846 to 0.841). Node Creation is second, so create_node is offered; for that request only that half is asserted.

2. Pattern wider than the skill — now /^schema creation$/i.

3. The opening's cost lived only in the test file — the seed comment now names both costs.

4. PR title and description — rewritten for what ships: two production changes and one expectation change, with what was withdrawn and the final live numbers.

5. Unit test name — renamed to say all three failing cases.

Tests: nodespace-agent 1005 passed, test:scripts 1340 passed, live retrieval suite 24/24. Live targeted run on the final production code: six scenarios, each 3/3 (table in the PR description).

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: the fixture's scoring branch gained a second condition and a new live guard was added. No production code changed in this commit beyond a comment.

[malibio]

Pragmatic Code Review: PR #3515 (issue #3268)

Review Type: Re-Review (fifth pass). The last review was at 4b58d77a. This one covers 6a421699 only, plus whether the rewritten title and description match the diff.

Nothing here blocks the merge. One finding (1) is a comment and description that understate a cost; I recommend one sentence before the squash. The rest is optional.

Previous Review Summary

	Count
Previous recommendations (fourth review)	5 (1 Important, 2 Suggestions, 2 Nits)
Addressed	5
Not addressed	0
New issues	1 Important (non-blocking), 3 Suggestions

What I ran, from the worktree:

• bun run test:scripts: 1340 passed, 0 failed, including the renamed a must-not-lead skill expectation fails on the named skill, on no leader, and on no skill that can act.
• cargo-nextest -p nodespace-agent -E "test(prompt_assembly_snapshot) | test(skill_pipeline)": 42 passed.

What I did not run: the ignored live_skill_retrieval_stability tests, any daemon, any model. Every retrieval score and every live result quoted in the commit, the comments and the PR description is unverified by this review. I checked them against each other and checked the claims about what routing offers against the code.

Fourth review's findings, against the code

#	Finding	Status
1	The relaxed scenario did not check that the record can be created	Closed. Details below.
2	/schema/i wider than the skill it names	Closed. not: /^schema creation$/i (decisions.ts:643).
3	The opening's cost lived only in the test file	Closed. The seed comment names both costs and all four guards (skill_pipeline.rs:222-231). See finding 1 on what it says about them.
4	Title and description described three production changes	Closed. See "Title and description".
5	Unit test name	Closed. The name lists the three failing cases the body asserts.

Finding 1 of the fourth review, in detail

📁 scripts/eval/fixtures/decisions.ts:85-95, :628-645, :1282-1296

• Typed correctly. { decision: "skill"; not: RegExp; reaches: RegExp } has both fields required, so a scenario cannot use not without saying what must be reached. "not" in expected narrows the two skill members; expected.reaches and the later expected.matches are each read on the right member. No cast, no any.
• Scored correctly. Order is: no decision recorded fails, selected === null fails, the leader matching not fails, no candidate matching reaches fails, otherwise pass. The displacement case from the fourth review (Bulk Import leads, Schema Creation in Node Creation's place) now fails.
• The patterns name exactly the skills that hold create_node. In SKILL_SEEDS, create_node is whitelisted by Node Creation (skill_pipeline.rs:173) and Graph Editing (:283) and by no other skill. Bulk Import holds create_nodes_from_markdown only (:477), Organization holds create_relationship and read tools (:490), Schema Creation holds create_schema, update_schema, get_node (:236). Both patterns are anchored and match the seeded titles.
• The two failures read differently. "Led with 'X', which matches …" against "No candidate matches …, so the turn was not offered the tool it needs", each followed by the retrieved list.
• The unit test covers displacement, and the Graph Editing alternative, and scores the real scenario, so it also pins that skill-instance-request carries this expectation.

The new live guard

📁 packages/agent/tests/it/live_skill_retrieval_stability.rs:1291-1343

adds_to_an_existing_list_keep_a_skill_that_can_create asserts what its name and comment say. holds is (!lead_asserted || first != "Schema Creation") && any(Node Creation | Graph Editing), on every one of the five reps, over the top RETRIEVAL_TOP_K. An empty ranking is a miss on both paths. For the one request with lead_asserted: false only the second half applies, which is what the comment states. The flag is per request, so the three other requests keep the lead assertion. Not run here.

Findings

🔴 Critical

None.

🟡 Important

1. The two recorded costs are described as "the tool is still offered". The lead does more than that in the code.
📁 packages/agent/src/skill_pipeline.rs:222-227, packages/agent/tests/it/live_skill_retrieval_stability.rs:1299-1306 and :1245-1253, PR description "Known costs of the opening"

The seed comment says "The tool each needs is still offered", the guard says "Node Creation is in the top 3, so create_node is offered", and the description says "in both the needed tool is still offered". All true. But which skill leads is not only a record:

• routing::declare_write_tool_fields (routing.rs:1154-1185, called at agent_loop.rs:3082) declares field_values sub-properties on create_node / update_node only from candidates at the turn's global top score. Its doc comment says why: so that when a type skill outscores a record skill, the record tool stays on the bare-object fallback and "the wrong tool" is not made easier to use.
• On "add the offline sync spec to the specs for this cycle", Node Creation led on main and now Schema Creation leads by 0.005. On "mark the offline sync spec as signed off", Graph Editing led by 0.017 and now Schema Creation leads by 0.001. On both, when the candidates carry schema data, the record tool the request needs goes from declared fields to the bare-object form, by the mechanism built to disadvantage it.

So the cost is: the tool is offered, in its undeclared form, with the type skill first in the candidate block. Neither request was run through Stage 2 (the fixture holds no spec record), so the effect on what the model does is unmeasured, and the comments do not say that either.

This does not block. The margins on both sides are inside what the PR itself calls chance, the tool surface is the same set as on main for the add request (Schema Creation was already second at 0.835), and the PR fixes three scenarios that failed 0 of 3. What I am asking for is that the record be complete, since the seed comment is where the next person tuning this description starts: one sentence in the seed comment and the description, such as "the record tool is still offered, without its declared fields, because the type skill is the top scorer (declare_write_tool_fields); Stage 2 was not measured on either request." Principle: a recorded trade-off has to state its full effect, or it gets weighed as smaller than it is.

🟢 Suggestions

2. The scenario and guard comments give the distance to "the next skill down" from main's ordering, not this branch's.
📁 scripts/eval/fixtures/decisions.ts:637-639, live_skill_retrieval_stability.rs:1296-1297

The scenario says "the next skill down is 0.003 to 0.005 behind" Node Creation; the guard says "the skill behind them takes its place, and create_node with it". The author's response on the PR gives this branch's numbers: Graph Editing 0.732, then Schema Creation 0.730 (it was 0.734 on main). On those, the next skill down is Graph Editing, which holds create_node, and Schema Creation is 0.007 behind Node Creation and has to pass both. The assertion is right either way; the comment should carry the branch's two numbers, which today are only in a PR comment.

3. reaches reads retrieved candidates, which include ones below their score bar.
📁 scripts/eval/fixtures/decisions.ts:1290, packages/agent/src/local_agent/decisions.rs:262-270

record_skill lists every retrieved candidate, cleared or not (skill_keeps_below_bar_candidates_visible_as_candidates), while stage2_permitted_names offers tools only from those that clear the gate. So a pass means a create_node holder was retrieved, not that create_node was offered. The doc comment words it as a necessary condition ("only while"), which is accurate, and at 0.73 against a 0.30 bar this cannot bite today. If it ever matters, the turn's own tool calls are in TurnRecord. No change needed now.

4. Nit: the description's live table predates the reaches half.
The skill-instance-request 3/3 was scored on 4b58d77a, under not alone. "create_node ran and landed in every rep" implies a holder was among the candidates, so the result carries over, but the description should say the expectation was tightened after that run. It also leaves out one small item that ships: "mark the offline sync spec as signed off" added to completion_state_updates_route_graph_editing.

Scores, checked against each other

Claim	Scenario comment	Guard comments	Seed comment	PR description	Agree
Contoso: Bulk Import 0.742, Organization 0.741, Node Creation 0.737	yes	"inside 0.005"	n/a	yes	Yes
"start tracking planning cycles": Schema Creation 0.778 before, 0.872 after	n/a	0.778	0.778, 0.872	0.778, 0.872	Yes
Wide opening on "update Northwind…": 0.799 against 0.793	n/a	yes	yes	named, no numbers	Yes
"mark … signed off": 0.790 / 0.773 before, 0.791 / 0.790 after	n/a	yes	"by 0.017", "by 0.001"	table	Yes (0.017 and 0.001 follow)
"add the offline sync spec…": 0.841 / 0.835 before, 0.846 / 0.841 after	n/a	yes	"by 0.006", "by 0.005"	table	Yes (0.006 and 0.005 follow)
Distance from Node Creation to the next skill on Contoso	"0.003 to 0.005"	"a lift of that size"	n/a	n/a	See finding 2

The live suite count in the description (24/24) matches the 24 tests in the file.

Title and description against the diff

git diff origin/main...HEAD --stat: 7 files, +432 / -7. (The two-dot diff shows 88 files because main has moved; the merge queue replays the branch onto it.)

• Title names the two production changes. The scenario change is in the body. Accurate.
• Change 1 (unescape_uri_ids, ordered before the steps that read the URI, five unit tests, two agent_loop tests): in the diff as described.
• Change 2 (the two-word opening, the golden's one line, four guards): in the diff as described.
• Change 3 ({ not; reaches }, reason recorded on the scenario): in the diff as described.
• Withdrawn items: none of the three is in the diff. Node Creation's and Node Deletion's seeds are identical to main.
• Not claimed and not there: nothing found. Shipped and not mentioned: the one request added to completion_state_updates_route_graph_editing, and a test-helper change in agent_loop.rs (TypeListingExecutor takes its listing). Neither needs more than a clause.
• The description is plain that the full fixtures were not re-run on the final wording.

Standards

No issue numbers in code comments in the touched files. No lint suppression, no any, no compatibility code. The eprintln! calls are the test file's existing pattern. Nothing touches node types or the Pro boundary. The commit is atomic.

Recommendation

COMMENT. No blocker remains. The fourth review's five findings are closed, the new expectation is typed and scored correctly, its patterns match the whitelists, and the new guard asserts what it claims.

Recommended before the squash, one sentence each in the seed comment and the description: finding 1, that losing the lead also drops the record tool's declared fields and that Stage 2 was not measured on the two spec requests. Findings 2 to 4 are optional.

🤖 Generated with Claude Code

[malibio]

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

[malibio]

Fifth review addressed (b63db278)

1. The recorded costs understated what losing the lead does — taken. I checked routing::declare_write_tool_fields: it declares field_values only from candidates at the turn's top tool-bearing score. So on the two spec requests where Schema Creation now leads, create_node / update_node are offered without their declared fields. The seed comment, both guard comments and the PR description now say that, and that Stage 2 was not measured on either request.

2. Distances from main's ordering — the scenario and guard comments now give this branch's numbers: Graph Editing 0.732 and Schema Creation 0.730 behind Node Creation's 0.737.

3. reaches reads retrieved candidates, including ones below their bar — no change, as suggested. It is a necessary condition, and the bar is far from these scores.

4. Description — notes that skill-instance-request was scored before the expectation gained its reaches half, and mentions the request added to the completion-state guard and the test-helper change.

Tests: test:scripts 1340 passed.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: comments and the PR description only. No code path, test assertion or fixture expectation changed in this commit.