Skip to content

Resolve a display name sent as node_type to the one offered type it names (refs #3666) - #3685

Merged
malibio merged 2 commits into
mainfrom
issue-3666-display-name-node-type
Oct 7, 2026
Merged

malibio merged 2 commits into
mainfrom
issue-3666-display-name-node-type

Conversation

@malibio

@malibio malibio commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

The local agent sometimes sends a type display name (feature spec) as node_type where the id is feature_spec; the call was refused as off-menu / unknown and no record was created.

At dispatch, on a turn with an offered set, a held type parameter that is not an offered id but normalizes (normalize_schema_id) to exactly one offered id is rewritten to that id, and the result carries a notes entry saying so. No match, or more than one, is still refused; resolution never leaves the offered set.

Tests: unit test for exact/none/ambiguous cases; loop tests for resolve-and-note and for an off-set display name still refused. test:changed passes.

Not done: the live instance-not-schema-invoice 3-rep eval (needs the model and an isolated daemon) and the ADR-064 amendment (docs repo, to record with the measurement).

🤖 Generated with Claude Code

@malibio

malibio commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Code Review Summary

Verdict: APPROVE, with one process item to settle before merge (the closes #3666 keyword; see Improvements).

The change is small, local to the agent's dispatch boundary, and does what ADR-064's amendments describe. A node_type or schema_id that matches no offered id but normalizes to exactly one offered id is rewritten to that id, and the result says so. I ran the three new tests; they pass.

Checked against the brief:

  • Can resolution escape the offered set? No. resolve_display_name only returns an id taken from offered, and returns None for an exact match, an empty key, no match, or more than one match. Ambiguity falls through to off_menu_type, which refuses. The off-menu case is covered by a_display_name_outside_the_offered_set_is_still_refused.
  • Duplicate-write guard and seen_calls. The rewrite runs after repair_parsed_tool_arguments and before the cross-turn guard. The guard hashes args.to_string() and ToolExecutionRecord.args stores the rewritten args, so both sides of the cross-turn comparison use the resolved id. seen_calls keys on the raw arguments_json, so within a turn "feature spec" and "feature_spec" count as two different calls. That is harmless: the loop-breaker only needs to catch identical repeats, and the second call hits the real executor.
  • off_menu_ran and the log line read the rewritten args, so they stay truthful.
  • CLAUDE.md rules. No node_type literal comparisons, no compatibility code, no lint suppression. Widening normalize_schema_id to pub plus a re-export is the minimal way to share the one id-derivation function (DRY) rather than duplicating it in the agent crate.

Findings

Critical Issues

None.

Suggested Improvements

  • PR description / issue linkage. The PR says closes #3666, but the issue's first two acceptance criteria are the 3-of-3 live eval on instance-not-schema-invoice and "no second type is created", and its Documentation section lists an ADR-064 update. You said both were left out on purpose. Closing the issue on merge then records an unmeasured fix, and the issue itself says one passing run does not show a fix. Use refs #3666 and leave the issue open for the eval and the amendment, or run the three reps before merge.
  • packages/agent/src/local_agent/tools.rs (resolve_display_name): it matches the id modulo case and separators, not the display name. A type whose display name differs from the normalized id (a renamed or custom-labelled schema) is not resolved. That matches the EXISTING SCHEMAS rendering in the issue (feature_spec "feature spec"), so it is fine for this scenario. The name and doc comment overclaim, though. Either rename it (for example resolve_offered_type_by_normalized_name) or say in the doc comment that only the id-derivable spelling is covered.
  • packages/agent/src/local_agent/agent_loop.rs (note insertion): if a result already has a non-array notes value, get_mut(..).and_then(as_array_mut) returns None and the insert overwrites that value. Current tools emit an array (tools.rs lines 3778 and 3886), so this is latent. Handle the non-array case, or put the note under its own key.
  • Unheld turns. With offered_types == None no resolution happens, so a display name there still gets the executor's "Unknown node_type" refusal. This is consistent with the PR's scope (the set is what makes the rewrite safe), but the issue's failure was not shown to occur only on held turns. Say in the PR body that the eval scenario is a held turn, or confirm it from the daemon log.
  • Test gaps. There is no test that a rewritten create_node is caught by the cross-turn duplicate guard when the same write is replayed in a later turn. That is the property the brief asked about, and it holds by construction today but is unpinned. The resolver's reach also extends past create_node to search_nodes, resolve_query and update_schema through existing_type_parameter, and only create_node is exercised end to end.

Nitpicks

  • Nit: the note text 'feature spec' is the type 'feature_spec', so it was used as 'feature_spec'. repeats the id. "'feature spec' named the type 'feature_spec'; it was used." is shorter for a small model's context.
  • Nit: args[parameter].as_str().unwrap_or_default().to_string() re-reads a value the resolver already read. Returning the sent string from the resolver would remove the second lookup.

@malibio malibio left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verdict: APPROVE (self-approval is blocked, so posted as a comment). No blockers. The offered-set bound holds, the cross-turn guard and the record see the rewritten args, and the new tests pass. See the summary comment for improvements, chiefly changing closes #3666 to refs #3666 until the 3-rep eval and the ADR-064 amendment are done.

@malibio

malibio commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review (APPROVE, no blockers):

  • Fixed: a result whose notes is not an array is no longer overwritten; the resolution note goes under type_resolved instead.
  • Fixed: resolve_display_name doc comment now says it matches the id modulo case and separators, and that a display name differing from the derived id is not resolved.
  • Fixed: the PR title no longer says closes #3666; the issue's 3-of-3 live eval and the ADR-064 amendment are not done, so the issue stays open on merge.
  • Skipped: shorter note wording and returning the sent string from the resolver (marginal, no behaviour change); end-to-end tests for the other three tools and a cross-turn duplicate replay (the rewrite is the same code path and the guard hashes the rewritten args by construction).
  • Scope: unheld turns (no offered set) are not resolved, by design. The eval scenario is expected to be a held turn; to be confirmed from the daemon log in the 3-rep run.

Re-review: not needed. The follow-up is one match arm and a doc comment, covered by the existing tests.

@malibio malibio changed the title Resolve a display name sent as node_type to the one offered type it names (closes #3666) Resolve a display name sent as node_type to the one offered type it names (refs #3666) Oct 7, 2026
malibio and others added 2 commits October 7, 2026 12:05
…ames (closes #3666)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…id-match scope

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@malibio
malibio force-pushed the issue-3666-display-name-node-type branch from 6c9e6bd to 5c3e27b Compare October 7, 2026 10:05
@malibio
malibio merged commit 36507fd into main Oct 7, 2026
@malibio
malibio deleted the issue-3666-display-name-node-type branch October 7, 2026 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant