[malibio]

Closes #3419.

What changes

A routed turn whose matched skills all link to their schemas is now held to the types its candidate block lists.

• When a turn is held (routing::offered_types): when every candidate that clears its score bar and whitelists a tool carries its linked schemas. One unlinked tool-bearing candidate, an unrouted turn, a fail-open surface or a withheld candidate block (routing_disabled) leaves the turn exactly as it was.
• What it is held to: every type the Stage-2 candidate block lists, which is the schema_metadata of every candidate that clears its bar, deduplicated in presentation order. That is the linked schemas and their subtypes, plus the type of any schema-typed retrieval hit, which is how a custom type the request names outright reaches the block.
• Linked or fallback (find_skills → schemas_linked): retrieval marks each candidate's metadata. The types alone cannot tell the two apart: an unlinked skill whose query names invoice carries the same single type a link to invoice would.
• Tool schema (routing::hold_to_offered_types): enum: [<set>] on the existing-type parameter of create_node, search_nodes, resolve_query (node_type) and update_schema (schema_id), with the parameter's description replaced by one that agrees with the enum. The list comes from Tool::existing_type_parameter, an exhaustive match.
• Dispatch: a call naming a type outside the set is not executed. The tool result is an error (type_not_offered) with the allowed ids. Its message tells the model to re-send only if one of them is what the user meant. If none is, it says to leave an optional type out (which searches every type), or, for a required type, not to use one anyway and to call route_clarify when that is on the turn's surface.
• Decision record: enforced on the record and in the log payload. On a held turn the schema decision's candidates are the offered set, and the record is enforced when its selection came from a call dispatch holds. The Tool executed log line carries type_refused and off_menu_ran (a call naming an off-menu type reached the executor, which the refusal makes false).

Where this differs from the issue text

• update_node gets no enum and is never refused. The issue lists it, but it takes a node id and has no type parameter. update_schema names its type in schema_id, and that is the parameter held.
• The set includes a schema-typed hit's type, not only the tool-bearing candidates' linked schemas. The issue asks for both "the union of the tool-bearing candidates' linked schemas" and "the enforced set is the set the model was shown"; the block also shows the schema hit, and leaving it out refused a call on the custom type the user named.
• selected_schema reads schema_id for update_schema. It read only node_type/type, so an update_schema selection was never recorded. Schema-decision counts for update_schema turns shift for that reason alone.

Consequences a reviewer should weigh

• How often a turn is held. No built-in skill links to a schema, so a built-in that clears its bar alongside a linked skill leaves the turn open. The set binds only turns that retrieval matched to linked skills alone.
• A named core type is refused on a held turn unless a skill links to it. A core type is never a schema-typed hit, so "add a task …" on a turn matched only to skills about another type is refused, and the model asks or says so.
• A held turn cannot look up a type its skills do not link to. search_nodes and resolve_query are held too, and a relationship's target type is not part of a skill's linked set.
• search_nodes with node_type: "schema" is refused on a held turn. Leaving node_type out still searches every type.
• Prompt-prefix reuse: tool definitions were already rebuilt per turn (declare_write_tool_fields), so the per-turn enum adds no new divergence point.

Eval fixture

• Two scenarios run against skills linked through applies_to (held-on-menu-type, held-off-menu-type). Their groups seed a warranty_claim type and three skills linked to it. They sit last so the extra skills do not change what the other scenarios are scored against within a rep.
• Refusals and off-menu calls that ran are read from the daemon's own per-call report (type_refused, off_menu_ran), not inferred from the decision records. The off-menu scenario fails if a record is created on the held turn at all, refused first or not, since any record created there is of an offered type. What followed a refusal is recorded: asked the user, replied in prose, or wrote a record.
• The fixture read schema list in a shape the CLI no longer emits (nodes[].properties.isCore), so on a warm database it found no schema. One decoder with a test now reads schemas[] with flat is_core and fields.

Goldens

• The existing prompt goldens are unchanged, since their fixture has no linked turn.
• stage2_tool_surface_offered_types.golden (new) holds each parameter that changes on a held turn, in full.
• decision-wire-format.golden gains enforced on every line and a tenth line for a refused selection.

Tests

• routing: when a set exists, the set equalling the ids the candidate block renders, tool-less and below-bar candidates, the enum on exactly the tools that name an existing type.
• tools: which tools name an existing type and that each declares that parameter, the held descriptions, off_menu_type per tool.
• agent_loop: the enum on the Stage-2 surface, refusal per tool with the allowed ids, a re-sent call running, a mixed round, the refusal's wording, the schema hit's type being offered, unlinked / mixed / routing-disabled / fail-open turns unchanged, the enforced decision log line.
• find_skills integration: schemas_linked for linked and fallback metadata.
• Scripts: the parser, the scrape marker, the held-turn scoring and the schema-list decoder.
• Mutation-checked in both directions: switching off the enum, the refusal, the fail-open gate or the precise enforced flag fails a test, and enforcing on unlinked or disabled turns fails a test.

bun run test:changed passes. bun run quality:fix is clean.

Eval results

eval:decisions on the locked model, one rep each, before (offered set switched off) and after. Full write-up: #3419 (comment)

• The twelve existing scenarios: the same verdict and the same first tool call in both runs (10 of 12 pass; the same two fail, for reasons unrelated to this change). None of those turns is held.
• The linked off-menu scenario: before, the model named task twice and both calls reached the executor. After, the turn was held, no call named an off-menu type, no record was created, and the model asked the user.
• The linked on-menu scenario: after, create_node on the linked type ran with its fields.
• The refusal did not fire in the final run (the model did not name task). It did fire in an earlier trial of the same scenario on this branch.

The ADR-038 amendment is committed in nodespace-docs.

🤖 Generated with Claude Code

[malibio]

Pragmatic Code Review: PR #3497 (issue #3419)

Review Type: Initial Review (no prior reviews or comments on the PR)

Summary

The Rust change is correct, small for what it does, and consistent with the surrounding code. I found no defect in the enforcement path. The open points are about what the eval can prove, what the model is told on a held turn, and two acceptance criteria that are still outstanding.

How I checked: I read the diff and the surrounding code in routing.rs, tools.rs, decisions.rs, agent_loop.rs, skill_ops.rs, the eval scripts and the goldens. I did not run the test suite or a build (a release build was running in the checkout), so the test:changed and quality:fix results are the author's.

What holds up:

• The set is derived once (routing::offered_types) and that one value drives the enum, the dispatch refusal and the decision record.
• Every enforced type was shown to the model. The set is read from the same schema_metadata that render_candidates_for_prompt renders, with no cap or filter between the two.
• The tool list is an exhaustive match on Tool, so a new tool forces a decision.
• No argument alias bypasses the check. node_type and schema_id have no serde alias, and repair_parsed_tool_arguments runs before the check.
• A refusal does not poison later guards. It is is_error: true, so it stays out of prior_writes, and a second identical refused call hits the existing duplicate-loop break.
• The dispatch order is sound. The check sits after the route_clarify supersede and the cross-turn duplicate guard (neither executes anything) and before the entity-duplicate and second-schema guards.

Both deliberate differences from the issue text are right:

• update_node: it declares only id, content and field_values, so there is no type parameter to put an enum on or to check. Holding it would need a node-type lookup at dispatch, which the issue does not ask for. The acceptance-criterion text on the issue should be amended to match.
• update_schema / schema_id: that is the parameter the tool declares. The selected_schema fix is a real bug fix: an update_schema selection was never recorded before.

Requirements Check

#	Criterion	Status	Notes
1	Offered set = union of linked schemas plus subtypes, deduplicated (unit test)	✅	offered_types plus six unit tests. Subtypes arrive through find_skills' linked_schemas; the integration test asserts schemas_linked beside ["invoice","retainer_invoice"].
2	Any unlinked candidate, or no routing: no set, schemas and dispatch unchanged (test)	✅	Unlinked, mixed, unrouted and routing-disabled are covered. The fail-open gate is not (finding 7).
3	enum on the five tools, never on create_schema, list derived from the Tool enum (tests)	⚠️	Met for create_node, search_nodes, resolve_query and update_schema (schema_id). update_node is deliberately excluded, and I agree.
4	Off-menu type not executed, result names the allowed ids (test per tool)	✅	Tested for the four held tools, with executed.is_empty() and allowed_types asserted.
5	Decision record shows whether the set was enforced	✅	enforced on the record, the payload, the TypeScript parser and the wire golden. See finding 4 for a gap in what it implies.
6	eval:decisions before and after, results posted	❌	Not run. As the fixture stands it cannot show this criterion (finding 1).
7	Prompt golden regenerated and reviewed	✅	Existing goldens are unchanged, correctly, since no fixture turn is linked. The new stage2_tool_surface_offered_types.golden is where finding 2 is visible.
8	ADR-038 amendment committed to nodespace-docs	❌	Written, not committed. See the docs note under finding 3.
9	test:changed passes, quality:fix clean	✅ (author's run)	Not re-run in this review.

Code Review Findings

🟡 1. The eval cannot demonstrate criterion 6 as the fixture stands

📁 scripts/eval/fixtures/decisions.ts:943-985

There are two separate problems.

• No scenario produces an offered set. The fixture has no linked skill: there is no applies_to, playbook or link anywhere in it, and its skill scenarios target Schema Creation, Node Creation and Graph Editing, none of which is linked. Every turn will record enforced: false, so before and after will be identical for "linked turns" because there are none.
• "Executed" is zero on a linked turn by construction. It is computed as d.offMenu && !d.enforced, and enforced is true on every linked turn. The number is derived from the flag, not observed from what dispatch did, so it cannot fail.

Recommendation, before the eval run:

• Add at least one scenario whose retrieval matches only a linked skill, for example a playbook skill (methodology/linear.rs links issue) or a skill seeded with an applies_to edge, with a prompt that tempts an off-menu type.
• Count refusals from the tool result ("error":"type_not_offered"), not from enforced.

Principle: a measurement has to be able to fail to count as evidence.

🟡 2. On a held turn the parameter description contradicts its own enum

📁 packages/agent/src/local_agent/tools.rs:1296-1311, packages/agent/tests/golden/prompt_assembly/stage2_tool_surface_offered_types.golden

The new golden shows the contradiction:

### create_node.node_type
"description": "Node type: 'text', 'task', or a custom schema ID (e.g. 'ticket', 'adr'). ... If the type is not listed there, it does not exist yet ...",
"enum": ["ticket", "adr"]

• create_node names 'text' and 'task' as valid, and the enum excludes them. With a custom-only set, a "jot this down" inside a held turn is refused.
• search_nodes says "e.g. 'task', 'text'" and "Omit to search all types".
• All of them point at the EXISTING SCHEMAS block, which on a held turn can list more types than the enum (finding 3).

ADR-064 puts the constraint in the tool schema because that channel is followed. Two statements in the same parameter that disagree weaken it.

Recommendation:

• When holding a parameter, also replace or trim its description so it does not name excluded types.
• For an optional parameter (search_nodes), have the refusal say the model may omit node_type to search every type. Today it offers only "one of them" or route_clarify.

If you would rather keep descriptions static, treat this as a measured risk and check it in the eval.

🟡 3. The enforced set is a subset of what the model is shown, and the excluded part includes the type the user named

📁 packages/agent/src/local_agent/routing.rs:965-986 (against render_candidates_for_prompt, lines 517-564)

render_candidates_for_prompt renders schema_metadata under EXISTING SCHEMAS for every gate-clearing candidate, including tool-less schema-typed hits. offered_types excludes those hits. This is deliberate and tested (a_tool_less_candidate_has_no_say_in_the_offered_set), and it matches the issue's "tool-bearing" wording. Two consequences are worth a decision.

• A type the user named outright is refused. A schema-typed hit is the lexical backstop for exactly that case. If only linked skills for another type cleared their bars, the turn shows the named type in the block, and create_node on it is refused. The unlinked fallback in find_skills treats "the type the query names" as the strongest signal available; the held path discards it.
• Cross-type lookups are refused. search_nodes and resolve_query are held too. A skill linked to invoice that needs to find the customer an invoice points at cannot filter by customer. Relationship target types are not part of linked_schemas.

Recommendation: decide one of these and record it in the ADR amendment.

• Add gate-clearing schema-typed hits' types to the set (they are in the same block the model reads).
• Or leave the turn open when one is present.
• Or keep the current behaviour and measure it.

Comment and doc wording to correct either way:

• agent_loop.rs:2854-2856 says "the set is what the candidate block shows".
• seeded-nodes.md (uncommitted) says "the enforced set is always the set the model was shown".
• The accurate statement is the one in offered_types' own doc comment and the ADR amendment: every enforced type was shown.

Docs commit note: the uncommitted decisions/038-skill-mediated-tool-routing.md diff also contains hunks from another session (the route_lookup text and a candidate-bound paragraph). architecture/prompt-skill-system.md and seeded-nodes.md have other sessions' edits as well. Stage only this PR's hunks.

🟢 4. enforced with off_menu does not always mean "refused"

📁 packages/agent/src/local_agent/decisions.rs:306-318, packages/agent/src/local_agent/agent_loop.rs:3243-3264

The doc comments on DecisionRecord::enforced and selected_off_menu state that on an enforced turn dispatch refused the off-menu call. enforced is turn-level (offered_types.is_some()), but selected_schema still reads node_type for update_node and node_type / type for create_schema, and neither tool is held. update_node {id, node_type: "album"} on a held turn is recorded off_menu: true, enforced: true and executes (off_menu_type returns None for it, as its own test asserts). The fixture would count it as "refused at dispatch".

This is rare, since neither tool declares that parameter. Recommendation: set the record's enforced only when the first call's tool has an existing_type_parameter (or there is no call), or soften the doc comments.

🟢 5. search_semantic.node_types names existing types and is not held, with no stated reason

📁 packages/agent/src/local_agent/tools.rs:2548-2570

existing_type_parameter documents why update_node and create_schema return None. search_semantic declares node_types (an array of existing type ids) and sits silently in the None arm. The result is that search_nodes(node_type: "schema") is refused while search_semantic(node_types: ["schema"]) runs. The issue does not list search_semantic, so leaving it out is defensible. Add a sentence saying so, since the exhaustive match exists to force that decision.

🟢 6. The refusal names route_clarify, which may not be on the surface

📁 packages/agent/src/local_agent/agent_loop.rs:1599-1614

with_stage2_clarify is skipped once session_already_clarified(session) is true (lines 2788-2790). On such a turn, unless a matched skill whitelists route_clarify, the refusal tells the model to call a tool it was not given. duplicate_entity_refused_result has the same wording, so this follows precedent. Passing a flag for whether route_clarify is in tools and dropping that sentence when it is not would be a small fix.

🟢 7. Test gaps

📁 packages/agent/src/local_agent/agent_loop.rs:14115-14425

• Fail-open gate. No test covers the scoped_surface && half of the gate at line 2858. It matters only when a linked contender whitelists nothing this build registers, but removing it would fail no test I can see. The PR's mutation checks cover the unlinked and disabled halves.
• Retry in the same turn. routed_engine scripts one call and then prose. A turn that is refused and then re-sends with an offered type is the path the refusal message asks for, and nothing pins it.
• Mixed round. No test has one on-menu and one off-menu call in the same round, to pin that the refusal is per call.

🟢 8. Minor code points

• 📁 packages/agent/src/local_agent/routing.rs:972-984: the dedup loop repeats decisions::schema_candidates (lines 281-292) with a different candidate filter. A shared helper taking an iterator of candidates would keep the two from drifting, which is the concern both doc comments raise.
• 📁 packages/agent/src/local_agent/agent_loop.rs:3238-3242: "the recorded candidate set is what the model was shown" is no longer exact on a held turn, where the recorded set is the narrower offered set.
• Nit: 📁 packages/agent/src/local_agent/tools.rs:1328: offered.iter().any(|offered| offered == named) shadows the parameter; offered.iter().any(|id| id == named) reads more easily.
• Nit: selected_schema now records update_schema selections that were None before. Schema-decision counts for update_schema scenarios will shift between the before run (main) and the after run for that reason alone. Say so when posting results.

Security and performance

• Security: nothing new. The refusal echoes the model's own argument and type ids already in the prompt. The enum is bounded by the top-K candidates' linked schemas.
• Performance: one extra pass over at most a handful of candidates per turn, and a linear contains on a small list per call. Tool definitions were already rebuilt per turn, so the enum adds no new prefix-cache divergence, as the PR says.

Recommendation: COMMENT

The implementation is sound and I found nothing that blocks on code grounds. It is not ready to merge as "closes #3419":

• Criteria 6 (eval) and 8 (docs commit) are open.
• Finding 1 should be fixed before the eval is run, or the run proves nothing about this change.
• Findings 2 and 3 are model-facing design points that are cheap to settle now and expensive to discover later. Each needs either a change or an explicit decision in the ADR amendment.
• Findings 4 to 8 are optional.

[malibio]

Automated pragmatic-code-review: COMMENT. No blocking code defects; acceptance criteria 6 (eval) and 8 (docs commit) are still open, and findings 1-3 need a change or an explicit decision before merge. Full findings in the review comment above.

[malibio]

Review addressed (commit 22903e2)

All eight findings were acted on. Nothing was skipped.

1. The eval could not demonstrate the change — fixed in the fixture.

• Two scenarios now run against skills linked through applies_to (held-on-menu-type, held-off-menu-type). Their group seeds a warranty_claim type and three skills linked to it, and waits for the skill index. Three, because a turn is held only when every tool-bearing candidate in the top three is linked. They sit last so the extra skills do not change what any other scenario is scored against.
• Refusals are counted from the tool result ("error":"type_not_offered", scraped into a [type-refused] marker), not from the enforced flag. "An off-menu call ran on a held turn" is now two observations that have to agree, so it can fail.
• Tried on the locked model before the full run: the three linked skills led retrieval, the turn was held, the model named task despite the enum, dispatch refused it, and nothing off-menu ran.

2. Description contradicted the enum — a held parameter's description is replaced with one that says only what the enum says, keeping "Omit to search all types" on search_nodes. The refusal now also offers leaving an optional type out. The tool's top-level description is unchanged, so search_nodes still mentions node_type='schema' there.

3. The type the user named was excluded — took the first option. The set is now every type the Stage-2 candidate block lists (the schema_metadata of every candidate that clears its bar), once every tool-bearing candidate is linked. A schema-typed hit's type is therefore offered. A tool-less candidate still cannot leave the turn open. A test asserts the set equals the ids the block renders. The cross-type lookup limit is kept and recorded in the ADR amendment, along with the resident workspace block not being the set. Comment and doc wording corrected.

4. enforced + off_menu did not always mean refused — the record is enforced only when the selection came from a call dispatch holds, so a stray node_type on update_node is recorded as not enforced. Tested.

5. search_semantic — the reason it is not held is stated on existing_type_parameter.

6. route_clarify wording — the refusal names route_clarify only when it is on the turn's surface.

7. Test gaps — added: the fail-open surface is not held, a refused call re-sent with an offered type runs, and one round refuses only its off-menu call. The fail-open gate and the precise enforced flag were mutation-checked.

8. Minor points — routing::type_ids is shared by offered_types and schema_candidates; the stale comment and the shadowed name are fixed. The update_schema selection now being recorded will be called out with the eval results.

Also fixed, found while running the fixture: the decision fixture read schema list as nodes[].properties.isCore, a shape the CLI no longer emits (schemas[] with flat is_core and fields). It found no schema on a warm database, tried to create types that existed, and reported setup state as missing. One decoder with a test now reads it.

Still open: the full eval:decisions before/after run is in progress, and the docs commit follows it.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: finding 3 changed what the offered set contains, finding 2 changed model-facing text, and the fixture gained scenarios and scoring. Those are design changes, not mechanical ones.

[malibio]

Pragmatic Code Review: PR #3497 (issue #3419)

Review Type: Re-Review (covers 0561f60f..22903e21)

How I checked: I read the follow-up diff and the surrounding code in routing.rs, tools.rs, decisions.rs, agent_loop.rs, skill_ops.rs, entity_types_block.rs, scripts/aichat.ts, scripts/eval/{runner.ts,types.ts,preflight.ts}, the decision fixture and its tests, the goldens, and the uncommitted ADR-038 amendment and seeded-nodes.md section. I ran no build and no tests (a model run is using this checkout), so test results are the author's.

Previous Review Summary

• Previous recommendations: 8
• Addressed: 8 (two with a residual point, listed under new findings)
• Not addressed: 0
• New issues found: 8 (2 important, 6 suggestions)

#	Previous finding	Status
1	The eval could not demonstrate criterion 6	Addressed. Two linked scenarios, seeded per group; refusals are read from the tool result. Residuals: findings B, C, D.
2	Held parameter's description contradicted its enum	Addressed for the parameter. The refusal offers "leave node_type out" only where the parameter is optional. Residual: finding F.
3	The enforced set was a subset of the block and excluded the named type	Addressed (first option). offered_types now reads type_ids over the same clears_score_gate filter render_candidates_for_prompt uses, through the same descriptor decoder, and the renderer has no cap, so the set equals the block. Code, doc comments, the ADR amendment and seeded-nodes.md agree. Residual: finding A.
4	enforced + off_menu did not always mean refused	Addressed. selection_held clears the flag when the first call's tool has no existing-type parameter; tested with a stray node_type on update_node.
5	search_semantic.node_types not held, no reason stated	Addressed in the doc comment on existing_type_parameter and in the ADR.
6	Refusal named route_clarify when it was not offered	Addressed. The sentence follows whether route_clarify is in the turn's tools.
7	Test gaps (fail-open gate, retry, mixed round)	Addressed. All three tests pin what they claim: the stranded candidate would produce a set without the scoped_surface gate, the re-sent call is the only one executed, and the mixed round executes only search_nodes.
8	Minor code points	Addressed. routing::type_ids is shared, the stale comment and the shadowed name are fixed.

Requirements Check

#	Criterion	Status	Notes
1	Offered set = union of linked schemas plus subtypes, deduplicated (unit test)	✅	Now wider by design: also the type of every gate-clearing tool-less candidate. Recorded in the ADR amendment.
2	Any unlinked candidate, or no routing: no set, nothing changes (test)	✅	Unlinked, mixed, unrouted, routing-disabled and now fail-open are covered.
3	enum on the five tools, never on create_schema, list derived from Tool	⚠️	Four tools held; update_node deliberately excluded (agreed in the first review). The issue's criterion text still lists it.
4	Off-menu type not executed, result names the allowed ids (test per tool)	✅
5	Decision record shows whether the set was enforced	✅	Now per selection, not per turn (finding E).
6	eval:decisions before and after, results posted	❌	Known open. The fixture can now measure it. See findings A to D and the note on the "before" run.
7	Prompt golden regenerated and reviewed	✅	stage2_tool_surface_offered_types.golden shows the replaced descriptions. It has no resolve_query entry (finding F).
8	ADR-038 amendment committed	❌	Known open. The text is consistent with the code.
9	test:changed passes, quality:fix clean	✅ (author's run)	Not re-run.

Code Review Findings

🟡 A. A core type the user names is still refused, and the new scenario scores a wrong-type write as a pass

📁 scripts/eval/fixtures/decisions.ts:804-812, 932-953; packages/core/src/ops/skill_ops.rs:346, 655; packages/agent/src/local_agent/routing.rs:975-980

• The fix for finding 3 covers custom types only. A named type reaches the candidate block through append_named_schema_candidates, which filters !s.is_core. The unlinked fallback and the workspace block do the same. So task and text are never in the block, and on a held turn they are never in the set unless a skill links to them.
• The doc comment and the ADR overstate it. Both say leaving the schema hit out "would refuse a call on the one type the user asked for by name". That holds for a custom type. For task the call is still refused.
• held-off-menu-type is that case. Its prompt is "add a task to ring the buyer back on Friday", which is a legitimate request for a core type. The scenario passes once dispatch refuses task.
• It passes whatever happens next. The refusal tells the model to "re-send the call with one of them". If the model re-sends create_node with warranty_claim, a call-back reminder is written as a warranty claim, the call is on the menu, and the verdict is a pass. The author's trial reports the refusal, not what the model did afterwards.

Recommendation:

• Record in the scenario's extra what followed the refusal (an on-menu write, route_clarify, or a prose reply), and report it with the eval results. That is the number that shows what holding a turn costs.
• Add the core-type consequence to the ADR amendment's Consequences, and narrow the "asked for by name" sentence to custom types in offered_types' doc comment and the ADR.
• Decide whether that consequence is acceptable. If it is not, the options are the same as before: offer core types on a held turn, or leave the turn open when the request names one.

Principle: a passing verdict should not be reachable through the failure it sits next to.

🟡 B. The PR body describes the first commit, not the code

📁 PR #3497 description

These statements are no longer true:

• "Offered set: the union of the schema_metadata type ids of every candidate that clears its score bar and whitelists a tool."
• The summary line Off-menu type named: N (X executed, Y refused at dispatch). The fixture now prints Off-menu type named: N and a separate Held to an offered set: … line.
• "On an enforced turn … off-menu there means refused" without the per-selection qualification.
• Tests: "tool-less candidates having no say".
• It also omits the replaced parameter descriptions, the two linked scenarios and the schema list decoder fix.

Recommendation: rewrite the body before merge. It is the record of what the change does.

🟢 C. The control scenario passes on a turn that was not held

📁 scripts/eval/fixtures/decisions.ts:795-802, 1063-1078

held-on-menu-type expects { decision: "schema", onMenu: true }. That path never checks enforced. If an unlinked built-in clears its bar, the turn is open, the model names warranty_claim, and the control passes without having measured a held turn. The summary's "Held to an offered set: N" count shows it, but the verdict does not.

Recommendation: fail it as unmeasured when no schema decision is enforced, as assertHeldToOfferedTypes does.

🟢 D. "An off-menu call ran on a held turn" can miss a call that ran

📁 scripts/eval/fixtures/decisions.ts:922-926; scripts/aichat.ts:397; packages/agent/src/local_agent/agent_loop.rs:4136-4138

It can fail in the simple case (one off-menu selection, no refusal), and the test pins that. Two gaps remain:

• Any refusal covers every selection. The check is "some enforced off-menu decision" and "no refused call anywhere in the turn". A turn that names task twice, is refused once and runs once, reads as clean. Comparing counts (enforced off-menu decisions ≤ typeRefused calls) closes it.
• Only the first call of a round is recorded, so an off-menu call in second position is never counted as named.

A separate fragility in the marker: [type-refused] is matched in result_preview, which is cut at 300 characters. serde_json writes keys alphabetically here (no preserve_order in Cargo.lock), so allowed_types comes before error. A set of roughly fourteen or more ids pushes the error code past the cut, the marker is lost, and the turn is reported as "ran". The fixture's set is one or two ids, so this run is safe, and the failure direction is loud. A bare log field, as result_field_count is, would not depend on the preview.

🟢 E. enforced is now per selection, and the fixture still reads it as "the turn was held"

📁 scripts/eval/fixtures/decisions.ts:910-912; scripts/eval/types.ts:52; packages/agent/src/local_agent/agent_loop.rs:3281-3289

After the fix for finding 4, a held turn whose only typed call is an unheld tool (update_node with a stray node_type) records enforced: false. turnHeld then reports "the turn was not held … a retrieval result", which is the wrong diagnosis. It is rare. The comment in types.ts ("enforced flag says the turn was held") should say what the flag now means.

🟢 F. Wording of the replaced descriptions

📁 packages/agent/src/local_agent/tools.rs:1316-1322

• The parameter's role is dropped. update_schema.schema_id goes from "ID of the schema to update" to "The type id: …", and resolve_query.node_type loses "the target node type to resolve against". The name and the tool description carry most of it, but one clause per tool ("The schema to update: one of …") costs nothing.
• "the listed type ids" is ambiguous. The system prompt can also hold the resident EXISTING SCHEMAS block, which the ADR says can list more than the set. "One of these type ids" points at the enum only.
• resolve_query is not in the golden, so its held description is covered only by the unit test.
• search_nodes' tool-level description still says "call this with node_type='schema'", which the held enum excludes. The author noted this. The refusal's "leave node_type out" is the recovery path.

The refusal message itself reads correctly for all four tools, including update_schema and the no-route_clarify ending.

🟢 G. A tool-less skill that is not a schema hit widens the set with fallback types

📁 packages/agent/src/local_agent/routing.rs:984-998; packages/core/src/ops/skill_ops.rs:647-659

find_skills gives every unlinked skill the fallback metadata (the named type, or the first five custom types), whatever its whitelist. A skill with an empty tool_whitelist that clears the read bar cannot open the turn, and its fallback types join both the block and the set. The set still equals the block, and it only gets wider, so nothing is wrongly refused. But "held to the schemas its skills link to" then includes up to five unrelated types. I did not confirm that a skill with an empty whitelist can be created; SkillNodeBehavior::validate only decodes the fields. The doc comment, the ADR and seeded-nodes.md describe the tool-less contributor as a schema hit only; one sentence would cover this case.

🟢 H. Seeding notes

📁 scripts/eval/fixtures/decisions.ts:481-540, 1127-1133; scripts/eval/runner.ts:1021-1033, 1367-1373

Seeding per group is sound for how the runner calls it: seedGroup runs once per group before its first turn, the two linked groups are last (pinned by a test), the seed is idempotent, and awaitSkillIndex counts every skill node, so it does wait for the three new embeddings.

• It depends on --between-runs. Without a wipe, rep 2 onwards scores every scenario against the three extra skills and warranty_claim. The runner already warns about a shared database; the fixture comment ("kept out of every other scenario's workspace") should say it holds per rep.
• Nit: if a skill is created and its applies_to call fails, the next seed finds the skill by name and skips it, leaving it unlinked. The scenario then fails as "not held", so it is loud.

The listedSchemas decoder is correct and its test pins the shape.

Note on the "before" run (criterion 6): decisionFromPayload (scripts/eval/runner.ts:208) rejects a payload without a boolean enforced. A daemon built from main does not emit it, so this branch's scripts read no decisions at all from it. The "before" figure for the linked scenarios has to come from the tool calls, or from a run whose method is stated with the results.

Recommendation: COMMENT

All eight findings were addressed, and I found no code defect in the follow-up commit. The set definition is sound and matches the block, the docs match the code, and the new Rust tests pin what they claim.

Before merge:

• Finding A needs a decision and a line in the ADR, and the eval should report what followed each refusal.
• Finding B: rewrite the PR body.
• Criteria 6 and 8 are still open, as known.
• Findings C to H are optional. C and D are cheap and make the eval results harder to misread.

[malibio]

Automated pragmatic-code-review (re-review): COMMENT. All 8 previous findings addressed; no code defect in the follow-up commit. Open before merge: a decision on core types named on a held turn (finding A), the stale PR body (finding B), and acceptance criteria 6 (eval) and 8 (docs commit). Full findings in the review comment above.

[malibio]

Re-review addressed (commit dbf3ee0)

A. A named core type is still refused, and the scenario could pass after a wrong-type write

• Kept the behaviour: a held turn refuses any type outside its set, core types included, unless a skill links to it. This is now a stated consequence in the ADR amendment and in seeded-nodes.md, and the "asked for by name" sentence is narrowed to custom types in offered_types' doc comment and the ADR. Whether that consequence is acceptable is a product decision I am leaving with the issue owner.
• The refusal no longer says "re-send the call with one of them" unconditionally. It says to re-send only if one of the offered types is what the user meant, and not to use one otherwise.
• held-off-menu-type now fails when a write follows the refusal, and every scenario records what followed a refusal (afterRefusal: wrote / clarified / replied). The summary prints the three counts.

B. Stale PR body — will be rewritten when the eval results are in, so it carries them.

C. Control passed on an open turn — a linked-skill scenario now fails as unmeasured when no schema decision on its turn was enforced.

D. A refused call could cover a second one that ran — the check compares counts (enforced off-menu decisions against refused calls). The first-call-per-round limit is stated in the comment. The marker no longer depends on the truncated preview: the Tool executed log line carries type_refused as its own field, and the scrape reads it from the fields ahead of args_preview.

E. enforced is per selection — the types.ts comments say what the flag means, and the not-held failure text says "no schema decision on this turn was enforced". turnHeld still reads any enforced decision in the turn. A held turn whose every round opens with an unheld typed call would read as not held; that is rare and the failure is loud.

F. Wording — a held description opens with what the parameter is for (The schema to update, The type to resolve against, …) and says "these type ids". The golden is taken over every model-facing tool, so resolve_query is in it. search_nodes' tool-level description still mentions node_type='schema'; left as is, with the refusal's "leave node_type out" as the recovery.

G. Tool-less skill fallback — one sentence in the doc comment and the ADR.

H. Seeding — the comment says the isolation holds within a rep and needs --between-runs across reps, and notes the failed-link case.

Tests: 917 agent tests and 1272 script tests pass; quality:fix clean.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: the refusal's model-facing wording changed, the log line gained a field the scrape depends on, and the fixture's scoring changed in several connected places.

[malibio]

Pragmatic Code Review: PR #3497 (issue #3419)

Review Type: Re-Review (second), covering 22903e21..dbf3ee02

How I checked: I read the follow-up diff and the committed code around it (agent_loop.rs, tools.rs, routing.rs, the held-surface golden and its test, scripts/aichat.ts, scripts/eval/{types.ts,runner.ts}, the decision fixture and its tests), the rewritten PR body, and the uncommitted ADR-038 amendment and seeded-nodes.md section. I ran no build and no tests (a model run and a release build are using this checkout), so test results are the author's.

Previous Review Summary

• Previous findings: 8 (A to H)
• Addressed: 6 (B, C, E, F, G, H)
• Partially addressed: 2 (A, D)
• Not addressed: 0
• New issues found: 3 important (all in eval scoring or model-facing wording, none in dispatch), 4 suggestions

#	Previous finding	Status
A	A named core type is refused; the off-menu scenario passed after a wrong-type write	Partially addressed. The behaviour is deliberately kept and recorded as a consequence in the ADR and seeded-nodes.md; that reason holds as a pending owner decision. The refusal's re-send is now conditional, and a write after a refusal fails the scenario. Residual: the same wrong-type write with no refusal still passes (finding 2).
B	PR body described the first commit	Addressed. The body matches the code on the set, the held tools, the refusal wording, type_refused, the scenarios and the summary lines. Eval results are still to come, as known.
C	Control passed on an open turn	Addressed. assertFixture returns notHeld for a linked scenario with no enforced schema decision, and a test pins both directions.
D	One refusal covered a second call that ran; marker depended on a truncated preview	Partially addressed. The marker is now the type_refused log field and is sound. The count comparison closes the gap named, but it reports a call as "ran" on three paths where nothing ran (finding 1).
E	enforced is per selection	Addressed. types.ts and the notHeld text say what the flag means; turnHeld's limit is stated.
F	Wording of the held descriptions	Addressed. Each description opens with its role, says "these type ids", and resolve_query is in the golden. search_nodes' tool-level mention of node_type='schema' is deliberately left; see finding 3 for why the stated recovery is now weaker.
G	Tool-less skill fallback widens the set	Addressed in the doc comment and the ADR.
H	Seeding notes	Addressed in the comment on seedLinkedSkills.

Requirements Check

#	Criterion	Status	Notes
1	Offered set = union of linked schemas plus subtypes, deduplicated (unit test)	✅	Wider by design (every type the candidate block lists); recorded in the ADR.
2	Any unlinked candidate, or no routing: no set, nothing changes (test)	✅
3	enum on the five tools, never on create_schema, list derived from Tool	⚠️	Four tools held. update_node has no type parameter; the PR body and ADR say so. The issue text still lists it.
4	Off-menu type not executed, result names the allowed ids (test per tool)	✅
5	Decision record shows whether the set was enforced	✅
6	eval:decisions before and after, results posted	❌	Known open. Findings 1 and 2 affect how the run in progress should be read.
7	Prompt golden regenerated and reviewed	✅	The held-surface golden now has all four parameters.
8	ADR-038 amendment committed	❌	Known open. The text matches the code.
9	test:changed passes, quality:fix clean	✅ (author's run)	Not re-run.

Code Review Findings

🟡 1. The count comparison reports "ran" for calls that did not run

📁 scripts/eval/fixtures/decisions.ts:939-945; packages/agent/src/local_agent/agent_loop.rs:3271-3306, 3704-3720, 3862-3907, 3939-3963

offMenuCallRanOnHeldTurn infers execution by subtraction: enforced off-menu decisions minus typeRefused calls. The schema decision is recorded at the top of every round, before dispatch. Three dispatch paths then stop the round's first call without an off-menu refusal:

• The identical re-send. The model repeats the refused call unchanged. Round 2 records a second enforced off-menu decision, then the duplicate-call guard (all_duplicate) breaks out before any call is dispatched. No Tool executed line is written. Result: named 2, refused 1, verdict "A call naming a type outside the offered set ran on a held turn". Nothing ran. Repeating a failed call is the behaviour that guard exists for, so this is the likely one.
• A route_clarify in the same round. The off-menu call gets superseded_by_clarification, which is checked before the off-menu branch. Named 1, refused 0.
• A cross-turn duplicate write, also checked first.

The previous some-based check passed the first case. The failure direction is loud, but it lands on the headline number of criterion 6: the summary would print "N ran" for a turn where dispatch held.

Recommendation:

• Observe execution directly instead of deriving it. The Tool executed line already has what is needed in scope: log whether the call named an off-menu type (the same off_menu_type check) next to type_refused, and score "ran" as a call that named one and was neither refused nor skipped.
• For the run in progress: before reporting any non-zero "ran", check the turn's [tool] lines. A "ran" with no successful held-tool call after the refusal is one of the three paths above.
• Add a test for the identical re-send (two enforced off-menu decisions, one refused call, no other call).

Principle: a measurement should observe the event it reports, not infer it from two records that can disagree for other reasons.

🟡 2. A wrong-type write with no refusal still passes the off-menu scenario

📁 scripts/eval/fixtures/decisions.ts:811-821, 962-973, 992-1014; scripts/eval/fixtures/decisions.test.ts:283-289

The scenario's doc comment now says the record "must not then be written as an offered type". That is checked only after a refusal (afterRefusal(turns) === "wrote"). If the model follows the enum on its first call and sends create_node with warranty_claim for the call-back task, there is no refusal, afterRefusal is undefined, no off-menu decision exists, and the verdict is a pass. The test "a held turn that stayed on the menu passes" pins that outcome against this scenario.

This is the outcome the enum is there to produce (ADR-064 measures the tool-schema channel as the most followed), so it is the more likely route to the wrong record, and the "After a refusal" summary line does not count it.

Two smaller points in the same function:

• wrote is WRITE_TOOLS.includes(name) && !isError. A create_node skipped for a same-round route_clarify, or answered by the duplicate-write guard, has isError: false and wrote nothing. fieldCount !== undefined || contentOnly is the persisted signal the record already carries.
• create_schema is not in WRITE_TOOLS and is never held, so "refused, then created a new type" is counted as "replied in prose". The seeded skills do not whitelist it, so this cannot happen in this fixture today.

Recommendation: for held-off-menu-type, record whether any write persisted on the turn, refusal or not, and report it with the results. Whether it fails the scenario depends on the decision pending under finding A; until then the number should at least be visible. Also update the scenario comment at line 816-817, which still says the model "either re-sends on an offered type or asks": re-sending now fails.

Principle: a passing verdict should not be reachable through the failure the scenario is named for.

🟡 3. The refusal now ties "leave node_type out" to the wrong condition

📁 packages/agent/src/local_agent/agent_loop.rs:1603-1632

For search_nodes the message composes to:

If one of them is what the user meant, re-send the call with it, copied exactly, or leave node_type out to search every type. If none of them is, do not use one anyway: say so in your reply.

Leaving the type out is the recovery for the case where none of the offered types is what the user meant (a search for tasks, or node_type: "schema" to list types). The sentence now offers it only when one of them is, and tells the model to stop and reply, or call route_clarify, when none is. Before this commit the re-send and the omit were both unconditional. The PR body, the ADR consequence ("Leaving node_type out still searches every type") and the reply on finding F all rely on that recovery.

The "do not use one anyway" clause exists to prevent a wrong-type write. A read has no such risk.

For create_node, resolve_query and update_schema the message is accurate and unambiguous, with and without route_clarify.

Recommendation: when may_omit is set, put the omit on the "none of them" branch, for example "If none of them is, leave node_type out to search every type." Pin the full composed message for the optional and required cases; today the tests assert fragments only (leave node_type out, call route_clarify), so the conditional wording added in this commit could be reverted without a test failing.

Principle: a recovery instruction should be attached to the condition it recovers from.

🟢 4. held_parameter_role could be exhaustive

📁 packages/agent/src/local_agent/tools.rs:1335-1345

The wildcard arm is guarded by the test (!text.starts_with("The type:") over every tool with an existing-type parameter), and that guard works. Returning the role from the same exhaustive match as Tool::existing_type_parameter would make a new held tool a compile error instead of a test failure, and remove the fallback string. Optional.

🟢 5. A refused non-first call can offset a first call

📁 scripts/eval/fixtures/decisions.ts:927-937

The comment says a later off-menu call in a round "still shows as a refused call". It also adds one to refused with nothing in named, so it can cancel an off-menu first call elsewhere in the turn. The direct observation in finding 1 removes this too.

🟢 6. type_refused scrape

📁 scripts/aichat.ts:400-404; packages/agent/src/local_agent/agent_loop.rs:4152-4163

Sound. The field is a plain bool, declared after result_field_count and before args_preview, so it is always emitted ahead of the arguments. There are no spans on this path. The only model-supplied value ahead of it is the tool name, which would have to contain a space and the literal type_refused=true. The test covers the argument-text case. No change needed.

🟢 7. Nits

• scripts/eval/types.ts:49-51: typeRefused is described as "read from the tool result the call got back (type_not_offered)". It is now read from the log line's type_refused field.
• The ADR's decision-record bullet says an off-menu selection on an enforced record "is one that was refused". With the paths in finding 1 it is one that did not run, which is the property that matters; "refused or otherwise not executed" would be exact.

PR body and docs

• PR body: matches the code. No stale statement found.
• ADR-038 amendment and seeded-nodes.md: match the code, including the conditional re-send, the core-type consequence and the tool-less fallback. The only gap is the search_nodes recovery in finding 3, which the message no longer states the way the docs describe it.

Recommendation: COMMENT

Dispatch, the tool schemas, the log field and the Rust tests are correct, and I found no defect in the production path. The three important findings are about measurement and wording:

• Finding 1 can put a false "ran" into the eval results that criterion 6 is judged on. Fix it, or check each "ran" by hand before posting.
• Finding 2 leaves the cost of holding a turn under-counted. Report the number even if the verdict waits on the finding A decision.
• Finding 3 is a one-sentence change to model-facing text, so it is cheaper to make before the "after" run than after it.

Still open as known: criteria 6 and 8, and the owner's decision on finding A.

[malibio]

Automated pragmatic-code-review (second re-review): COMMENT. No defect in the production path. Three important findings on eval scoring and refusal wording (a false "ran" count, a wrong-type write that passes with no refusal, the search_nodes omit recovery tied to the wrong condition). Criteria 6 (eval) and 8 (docs commit) and the finding A decision are still open. Full findings in the review comment above.

[malibio]

Second re-review addressed (commit b1aeb7c)

1. The count comparison reported "ran" for calls that did not run — replaced with a direct observation. The Tool executed log line now carries off_menu_ran: the call named a type outside the turn's offered set and reached the executor. It is worked out at the log site from the call's arguments and from whether the call was dispatched, not from which guard branch ran. The scrape emits [off-menu-ran], and the fixture scores "ran" from that. The decision-count inference is gone, which also removes finding 5. Tests: the identical re-send (two off-menu decisions, one refusal) passes; a call the daemon reports as run fails. Mutation-checked: with the refusal disabled, the log line reads off_menu_ran=true and the test fails.

2. A wrong-type write with no refusal still passed — held-off-menu-type now fails when a record is created on the held turn at all, refused first or not, and the failure says which. A write counts only if it persisted (a field count or the content-only marker), so a create skipped for a same-round route_clarify or answered by the duplicate-write guard is not one. The summary prints how many requests for a type outside a held turn's set created a record as an offered type instead. The scenario comment is updated. create_schema after a refusal is still counted as a prose reply; the seeded skills do not whitelist it.

3. The refusal tied "leave node_type out" to the wrong condition — for an optional type the message now reads "If none of them is, leave node_type out to search every type." For a required type it keeps "do not use one anyway" and then route_clarify or a plain reply. The three composed messages are asserted in full.

4. held_parameter_role exhaustive — skipped. It would mean a second exhaustive match over all 21 tools, or changing existing_type_parameter's return type across its callers, to replace a fallback string that a test already guards. Worth doing if a fifth held tool appears.

6. No change needed, and the field order is now confirmed against a real log line: … result_field_count=1 type_refused=false off_menu_ran=true args_preview=…. A test asserts both fields sit ahead of args_preview.

7. Nits — types.ts says where typeRefused is read from; the ADR's decision-record bullet says "did not run (refused for its type, or stopped by an earlier guard)".

Tests: 918 agent tests and 1275 script tests pass; quality:fix clean. The eval runs were restarted on this commit, since the refusal wording is model-facing.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: a new log field and the flag behind it were added to the dispatch path, the refusal's wording changed again, and the scenario's pass condition changed.

[malibio]

Pragmatic Code Review: PR #3497 (issue #3419)

Review Type: Re-Review (third), covering dbf3ee02..b1aeb7c1

How I checked: I read the follow-up diff and the committed code around it (agent_loop.rs at HEAD for the dispatch chain and the Tool executed log site, tools.rs for off_menu_type and the create_node executor, scripts/aichat.ts, scripts/eval/{runner.ts,types.ts}, the decision fixture and all four test files), the PR body, and the uncommitted ADR-038 amendment and seeded-nodes.md section. I ran no build and no tests (release builds and a model run are using this checkout), so test results are the author's.

Previous Review Summary

• Previous findings: 7
• Addressed: 5 (1, 2, 3, 5, 7)
• Deliberately skipped, reason holds: 1 (4)
• No change needed, confirmed: 1 (6)
• Not addressed: 0
• New issues found: 0 critical, 0 important, 3 suggestions

#	Previous finding	Status
1	Count comparison reported "ran" for calls that did not run	Addressed. offMenuCallRan reads the daemon's per-call off_menu_ran; the subtraction is gone. The identical re-send, the same-round route_clarify and the duplicate write all leave dispatched false, so none reads as "ran". A test pins the re-send case.
2	A wrong-type write with no refusal passed the off-menu scenario	Addressed. held-off-menu-type fails on any persisted create_node, refused first or not. persisted requires a field count or the content-only marker, so a skipped or duplicate-guarded create is not a write. The scenario comment is updated and a summary line reports the count.
3	Refusal tied "leave node_type out" to the wrong condition	Addressed. For an optional type the none-of-them branch reads "leave node_type out to search every type"; the required cases keep "do not use one anyway" with route_clarify or a plain reply. All three composed messages are asserted with assert_eq!.
4	held_parameter_role could be exhaustive	Deliberately skipped. The reason holds: the fallback arm is guarded by a_held_parameters_description_agrees_with_its_enum, and a second exhaustive match over every tool costs more than it buys for four tools.
5	A refused non-first call could offset a first call	Addressed by removal: the count comparison it lived in is gone.
6	type_refused scrape	No change was needed. off_menu_ran is declared next to it, ahead of args_preview, and read from the same pre-arguments slice. A Rust test asserts both fields precede args_preview.
7	Nits (types.ts comment, ADR wording)	Addressed. types.ts names the log field; the ADR bullet says "did not run (refused for its type, or stopped by an earlier guard)".

Requirements Check

#	Criterion	Status	Notes
1	Offered set = union of linked schemas plus subtypes, deduplicated (unit test)	✅	Wider by design (every type the candidate block lists); recorded in the ADR.
2	Any unlinked candidate, or no routing: no set, nothing changes (test)	✅
3	enum on the five tools, never on create_schema, list derived from Tool	⚠️	Four tools held. update_node has no type parameter; the PR body and ADR say so. Known.
4	Off-menu type not executed, result names the allowed ids (test per tool)	✅
5	Decision record shows whether the set was enforced	✅
6	eval:decisions before and after, results posted	❌	Known open, run in progress. The scoring it will be read through is now sound.
7	Prompt golden regenerated and reviewed	✅	This commit changes the refusal text, which is a tool result and is pinned by assert_eq! rather than a golden.
8	ADR-038 amendment committed	❌	Known open. The text matches the code.
9	test:changed passes, quality:fix clean	✅ (author's run)	Not re-run.

What I verified in this commit

dispatched and off_menu_ran (packages/agent/src/local_agent/agent_loop.rs:3844-3846, 4089-4094, 4161-4164)

• dispatched is set in one place: the final else of the guard chain, on the line before the only self.tool_executor.execute(...) call in the loop. The route_clarify acknowledgement, the superseded skip, the cross-turn duplicate, the off-menu refusal, the mentioned-entity refusal, the second-create_schema refusal and the malformed-arguments arm all leave it false.
• off_menu_ran is computed from the args the match returns. The executor received args.clone() and nothing mutates args between the off-menu guard and the log site, so the check sees what the executor saw. On the malformed arm args is {} and dispatched is false.
• It cannot be true where nothing ran (it requires dispatched). It cannot be false where an off-menu call ran, as far as off_menu_type defines "off-menu": I checked that create_node deserializes node_type as a required String with no alias and no default, so a call that omits it or sends a non-string errors in the executor and creates nothing.
• On an unheld turn it is false by definition, which the new Rust test pins.

Refusal message: the three compositions read correctly, and the test would fail on any wording change.

Scrape and parser: aichat.ts emits [ERROR] [fields=N] [content-only] [type-refused] [off-menu-ran] in that order and the runner.ts regex consumes them in the same order. Argument text spelling a field out is not matched (tested for both fields).

Fixture scoring

• fixture.score is called with the one scored turn, so createdRecord cannot pick up a setup turn's create.
• Every successful create_node reports property_count or content_only, so persisted cannot miss one. A skipped or duplicate-guarded create reports neither.
• Failing held-off-menu-type on any persisted create_node is sound for its prompt. The request is for a task. On a held turn a create_node naming task is refused, one naming no type errors, so any create that persists names an offered type, and task is not offered (the seeded skills link only to warranty_claim, and a core type is never a schema-typed hit). A schema hit can add another custom type to the set, but never task. No correct outcome for this prompt creates a record on a held turn. The verdict does encode the pending owner decision on core types: if task later becomes reachable on a held turn, this assertion is the one to revisit.
• The linked-scenario gate (notHeld) still precedes every assertion, so an open turn fails as unmeasured and not as a model failure.

PR body and docs: both match the code, including off_menu_ran, the three-way refusal wording and the any-record-fails rule. No stale statement found.

Code Review Findings

No critical or important findings. Three suggestions, none of which changes a verdict or the headline numbers.

🟢 1. afterRefusal and createdRecord count different things under the same words

📁 scripts/eval/fixtures/decisions.ts:957-969, 983-995, 1334-1336

• WRITE_TOOLS includes update_node, so a refused task followed by an update_node on the existing claim is reported as "wrote the record as an offered type" in the "After a refusal" line. The scenario passes (only create_node fails it) and the next line reports 0 created. Both are defensible, but the two lines can disagree, and an update_node did not write the record as a type. The same update_node with no refusal first is not counted anywhere.
• create_nodes_from_markdown is in WRITE_TOOLS but can never satisfy persisted: it is not a field-count-reporting write and has no content-only marker. Before this commit it counted when it did not error. It is unreachable here, since no seeded skill whitelists it.

Recommendation: when posting the results, read a non-zero "wrote" next to the tools called. Optionally rename the label to "wrote something" or split creates from updates, and drop create_nodes_from_markdown from the list or say it is not observable.

Principle: two reported numbers that sound alike should count the same event.

🟢 2. off_menu_ran shares its definition with the guard it watches

📁 packages/agent/src/local_agent/agent_loop.rs:4156-4164

In the committed chain off_menu_ran cannot be true: reaching the executor on a held turn means off_menu_type already returned None for the same arguments. That is the intended shape (a tripwire that fires if the guard is removed, reordered or bypassed by a second executor call), and the author's mutation check confirms it fires. It is worth one clause when the results are posted: "0 ran" confirms the guard was in force on every call of the run. It would not detect the guard and an executor disagreeing about which argument names the type. I checked that they agree today.

No code change needed.

🟢 3. Nit: the failure label when the create came first

📁 scripts/eval/fixtures/decisions.ts:1033

A turn that creates a warranty_claim and is then refused on task fails correctly, but the text says "with no refusal: the first call named an offered type", because afterRefusal only looks after the first refusal. "not after a refusal" would cover both orders.

Recommendation: APPROVE

Nothing needs to change in the code before merge. The production path is unchanged in behaviour by this commit apart from the refusal wording, which is correct for all three cases and pinned in full. The eval scoring now observes what it reports, and the off-menu scenario can no longer pass through the failure it is named for.

Still open as known, and still required by the issue before it closes: the eval:decisions before/after results (criterion 6), committing the docs edits (criterion 8), and the owner's decision on a core type named on a held turn.

[malibio]

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

[malibio]

Third re-review addressed (commit cf28c4e)

The third re-review approved with three suggestions.

1. "After a refusal" and the wrong-type-record figure counted different things — fixed. afterRefusal now counts a create_node that landed, the same event createdRecord counts, and its label reads "created the record as an offered type". update_node is no longer counted there (it changes an existing record), and create_nodes_from_markdown, which the scrape cannot observe persisting, is gone from the list.

2. off_menu_ran shares its definition with the guard it watches — no code change, as the review says. The eval results will state it: "0 ran" confirms the guard was in force on every call of the run, and would not detect the guard and an executor disagreeing about which argument names the type.

3. Failure label when the create came before the refusal — now "not after a refusal".

Tests: 1275 script tests pass; quality:fix clean. No Rust changed.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: the only changes since the approved commit are a label and a counting rule in the eval fixture's reporting, with their tests. No production code, no model-facing text and no verdict logic changed.

Open before this closes: the eval:decisions before/after results (in progress), and committing the ADR-038 amendment.

[malibio]

Merge queue: #3497 was taken out of the queue — it conflicts with main in: packages/agent/src/local_agent/agent_loop.rs. Rebase onto origin/main, push, and re-run bun run merge 3497.