Skip to content

Give validate and run one shared notion of an action (#241) - #438

Merged
jeremymanning merged 1 commit into
mainfrom
fix/validate-run-agreement-241
Aug 1, 2026
Merged

Give validate and run one shared notion of an action (#241)#438
jeremymanning merged 1 commit into
mainfrom
fix/validate-run-agreement-241

Conversation

@jeremymanning

Copy link
Copy Markdown
Member

Closes #241.

The disagreement

ToolValidator._extract_tool_name treated a step's action: as a tool name whenever the step had no tool: key. Correct for the legacy single-field form (action: filesystem), wrong for a model step, which by design names no tool:

validate run
action: generate Tool 'generate' not found in registry ✓ runs correctly

They disagreed because they held two separate, unshared notions of what an action is. The executor's lived in ~12 if action_str == ... branches in HybridControlSystem, plus a supported_actions list in ModelBasedControlSystem — and those two already disagreed with each other. That is how it went unnoticed that only generate-structured dispatched while generate_structured fell through to the prompt fallback (fixed in #437).

The fix

core/actions.py is the single source of truth. The important part is that HybridControlSystem now dispatches off BUILTIN_ACTION_HANDLERS instead of keeping a parallel if-chain:

handler_name = BUILTIN_ACTION_HANDLERS.get(action_str.strip())
if handler_name:
    return await getattr(self, handler_name)(task, context)

So the mapping cannot claim an action the executor does not actually run — drift is structurally impossible rather than merely discouraged. Adding a runtime action is one entry; both consumers follow.

Three things deliberately preserved:

  • The tool lookup still runs first. filesystem is both a registered tool and a built-in action name, so the legacy single-field form keeps its full parameter validation rather than being skipped as an action.
  • Unknown actions are still rejected. The fix must not turn the validator into a rubber stamp; there is a test for exactly that.
  • The two natural-language families"echo ..." and "write the following content to report.md" — stay pattern-matched, since prose has no name to register, and keep their existing order relative to each other.

The sentinel worked

The strict xfail added in #437 XPASSed the moment the two agreed:

[XPASS(strict)] #241 / #104: a model step runs but does not validate...

It is now the plain assertion it was standing in for. Its disappearance is the evidence this issue is closed, which is the whole reason it was strict=True.

Coverage

tests/test_builtin_actions.py pins both directions:

  • every registered action resolves to a handler that exists on the control system (parametrised, so a bad entry names itself instead of surfacing as a mystery failure inside whichever pipeline used it)
  • every built-in action is accepted by a strict validator (allow_unknown_tools=False)
  • an action that is neither tool nor built-in is still rejected
  • the legacy single-field form still resolves as a tool; the two-field form still resolves tool:, never action:
  • a built-in action dispatches away from the model — pinning the boundary that let generate_structured return prose

Numbers

before after
Blocking suite 398 passed, 1 xfailed 435 passed, 13 skipped, 0 failed, 0 xfailed
Collection 3194 3230, 0 warnings
Examples validating 2 / 111 3 / 111
uv lock --check / ruff on src / git diff --check clean

The catalog gain is small but it is examples/basic/hello_world.yaml — the flagship example, which did not compile before this.

Not in this PR

  • Fail on unresolved template references instead of writing literal {{ ... }} and exiting 0 #153 — an unresolved template still passes through to disk with success: true. Root cause located: TemplateManager catches UndefinedError, logs a warning, and returns the original template text (template_manager.py:516, and again at :81). That fallback exists because resolution runs in multiple passes, so simply re-raising would break legitimate deferred resolution; the fix belongs at the point where resolved parameters are handed to a tool, before any side effect. Separate change.
  • supported_actions capability lists. Both control systems still declare their own, and both are incomplete (HybridControlSystem omits generate_text, evaluate_condition, loop_complete, …). Those lists feed control-system selection, not dispatch, so aligning them can change which system claims a task — a behavioural change that deserves its own PR rather than a drive-by.

`ToolValidator` treated a step's `action:` as a tool name whenever the
step had no `tool:` key. That is right for the legacy single-field form
(`action: filesystem`) and wrong for a model step, which by design names
no tool -- so `validate` reported "Tool 'generate' not found in registry"
while the executor ran the very same document correctly.

They disagreed because they held two separate, unshared notions of what
an action is. The executor's lived in ~12 `if action_str == ...` branches
in HybridControlSystem plus a `supported_actions` list in
ModelBasedControlSystem, and those two already disagreed with each other:
`generate_structured` was implemented but undeclared, which is how it
went unnoticed that only the hyphenated spelling dispatched.

core/actions.py is now the single source of truth. HybridControlSystem
*dispatches off* BUILTIN_ACTION_HANDLERS rather than keeping a parallel
if-chain, so the mapping cannot claim an action the executor does not run
-- the drift is structurally impossible rather than merely discouraged.
The validator accepts exactly the names in it.

The tool lookup still runs first, so the legacy single-field form keeps
its full parameter validation: `filesystem` is both a registered tool and
a built-in action name, and it must resolve as the tool.

The two natural-language families -- "echo ..." and "write the following
content to report.md" -- stay pattern-matched, since prose has no name to
register. They keep their existing order relative to each other.

The strict xfail added in #437 did its job: it XPASSed the moment the two
agreed, and is now the plain assertion it was standing in for. Its
disappearance is the evidence this issue is closed.

tests/test_builtin_actions.py pins both directions: every registered
action resolves to a handler that exists on the control system, every
built-in action is accepted by a strict validator, an action that is
neither tool nor built-in is still rejected, and the legacy and two-field
forms still resolve to the tool.

Blocking suite 398 -> 435 passed, 13 skipped, 0 failed, and now 0 xfailed
-- the "no xfails in supported contracts" release gate is met for this
module. Collection 3194 -> 3230, 0 warnings. Example catalog 2 -> 3 of
111 validating: examples/basic/hello_world.yaml now compiles.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jeremymanning

Copy link
Copy Markdown
Member Author

CI 9/9 green, and the legacy backlog moved in the right direction.

Same job, same environment, main (run 30714545617) vs this branch (run 30714744110):

main this PR
failed 471 469
errors 195 195
passed 1843 1845
total red 666 664

Two legacy tests that were red now pass, nothing regressed. The deselected count moves 498 → 534 because this PR adds 36 marked tests, which the legacy selection excludes by construction.

Worth noting what this comparison cost: the legacy job publishes a scalar and runs with -rN, so "which two?" is not answerable from CI output. Switching it to -raE would make the backlog diffable run-to-run rather than only countable — currently the only way to attribute a change is to re-run both sides locally.

@jeremymanning
jeremymanning merged commit 8d92913 into main Aug 1, 2026
9 checks passed
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.

Make 'run' and 'validate' agree: run bypasses the validation gate entirely

1 participant