Add opt-in pre-execute argument validation (spec 1.5) - #86
Merged
Merged
Conversation
`ToolRegistry(validate_arguments=False)` gains an opt-in JSON Schema check
of `call.arguments` against `tool.definition.parameters`, run once ahead of
the local-execute/dispatch branch so a single check covers both paths — and,
for dispatch=True tools, fires before `pre_dispatch` so a malformed call
never ships to a worker. That last case is the point: deep worker-side
failure under an attempt state machine is a terminal trap (the #295 shape).
Error rendering deliberately consults two jsonschema error objects. Plain
`next(iter_errors(...))` reads fine for a scalar field but reports an `anyOf`
(`int | None`) or `$ref` (nested model) failure against the union itself —
"-1 is not valid under any of the given schemas" — which tells the model
nothing. `best_match` descends to the real constraint but loses the property
path and its `description`, and that description is the whole value of the
feature: it carries the field's own prose into the model-visible error.
So the path and description are anchored on the top-level error and only the
message is taken from `best_match`. Verified against real pydantic-generated
schemas; `tests/test_validate_arguments.py` pins all five error shapes, with
the anyOf and $ref cases as the regression canaries.
Off by default, lazily imported, and gated behind a new `validate` extra
(`jsonschema>=4.18` — the referencing-rewrite boundary), rolled into `all`.
Non-adopters pay no install weight, no import time, no new failure mode.
Adds 4 packages to the lock; `attrs` was already present.
`jsonschema` is also added to the `dev` extra, for the same reason `pyyaml`
already is: it is an optional extra's runtime dependency that the suite needs
regardless, because the new tests guard on `importorskip("jsonschema")`.
Without it, CI's `uv sync --extra dev --extra callback` would skip the module
wholesale and the feature would ship with zero coverage while CI read green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9LJHwXePrXZm5wVSey4QU
There was a problem hiding this comment.
Pull request overview
Adds an opt-in argument-validation path to ToolRegistry that validates ToolCall.arguments against each tool’s JSON Schema before either local execution or dispatch, preventing malformed calls from reaching pre_dispatch or being shipped to workers. This introduces an optional jsonschema dependency behind a new validate extra, and includes a dedicated test suite that pins error rendering across several schema shapes.
Changes:
- Add
ToolRegistry(validate_arguments: bool = False)and pre-execute schema validation shared by local and dispatched tool paths. - Introduce
validateextra (and includejsonschemaindev/all) plus lockfile updates forjsonschemaand its dependencies. - Add
tests/test_validate_arguments.pyto cover default-off behavior, dispatch ordering, and error message rendering.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/jig/tools/registry.py |
Implements opt-in JSON Schema validation and validator caching keyed by tool name. |
tests/test_validate_arguments.py |
Adds coverage for validation behavior, error rendering, and import-missing handling. |
pyproject.toml |
Adds validate extra and ensures test runs include jsonschema via dev. |
uv.lock |
Locks jsonschema and transitive dependencies; wires extras (validate, all, dev). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
validate_arguments=True could take a tool that worked and make it crash the agent loop. Draft202012Validator resolves $refs lazily, so a tool whose schema $refs a missing definition registers fine and then raises _WrappedReferencingError out of ToolRegistry.execute() on the first call — contradicting execute()'s "returns a ToolResult, never raises" contract, which this feature's own code comments rely on. Registering more eagerly is not a fix: check_schema() passes a dangling $ref as syntactically valid JSON Schema, so only a call-time guard catches it. Fails open rather than closed. A validator that raises says nothing about whether call.arguments are valid — the tool's schema is what is broken, and the model cannot remediate that. Skipping validation keeps the promise this opt-in flag has to make, that enabling it never breaks a tool that worked with it off. Failing closed would hand the model an unfixable error to retry against and burn the run down: the terminal-trap shape spec 1.5 exists to prevent, arriving from the other direction. Loud in the log for operators (warning + exc_info), invisible to the model. Scoped to the offending tool, not the registry — a sibling tool with a sound schema still rejects bad arguments. Both properties are pinned: mutation-testing confirms removing the guard fails both new tests, renaming the warning fails only the log assertion, and clearing the validator dict registry-wide fails only the sibling test. 1155 -> 1157 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A9LJHwXePrXZm5wVSey4QU
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements §1.5 of
jig-lifecycle-hooks-spec.md— the last item of PR 1, split out because it is the only one in the document carrying a dependency.ToolRegistry(validate_arguments: bool = False)checkscall.argumentsagainsttool.definition.parametersonce, ahead of the local-execute/dispatch branch, so one check covers both paths. Fordispatch=Truetools it fires beforepre_dispatch, so a malformed call never ships — which is the actual motivation: a bad argument that surfaces worker-side, under an attempt state machine, is a terminal trap rather than a retryable error (the #295 shape).Off by default. Existing registries are byte-identical and never import
jsonschema.The error rendering is not the spec's snippet, deliberately
The spec's pasted code uses
next(iter(errors)). Probed against real pydantic-generated schemas, that is wrong in two ways, and both fixes are load-bearing.1. It produces useless messages on exactly the schemas this feature exists for. The spec's own rationale for taking the dependency was "
$refis the decider — vendored minimal validation silently no-ops on exactly the tools that need it." Plainiter_errorshas the same failure:next(iter_errors)best_match-30 is less than or equal to the minimum of 0✅int | None(anyOf)-1 is not valid under any of the given schemas❌-1 is less than the minimum of 0✅$ref){'depth': 0} is not valid under any of the given schemas❌0 is less than the minimum of 1✅But
best_matchalone descends past the property-level schema into the failing branch, losing the property path and itsdescription— and the description passthrough is the entire justification for the feature (it carries the field's own prose into the model-visible error; per the companion ergonomics spec that prose is what fixed an observed 8-of-10 first-call fumble rate).So the path and description are anchored on the top-level error and only the message is taken from
best_match. Rendered output, pinned by tests:2. The spec's format string emits a dangling
at. A missing required property has an emptyerr.path, sof"... at {'/'.join(...)}"renders... is a required property atwith nothing after it. The conditional is required, not cosmetic.The combining step carries a comment explaining why two error objects are consulted — the obvious "simplification" back to one silently regresses anyOf/
$refhandling, and mutation testing confirms only two tests would catch it.Error channel
Returns a
ToolResultwith theschema:prefix baked in rather than raisingJigToolError(phase="schema"). The existingJigToolError → "{phase}: {msg}"handler only wraps the non-dispatch innertry, so a raise at the single insertion point would escape both it and_execute_dispatched's handler and propagate uncaught out ofexecute()— which today never raises to its caller. Building the prefix in also structurally rules outschema: schema: ….Dependency
validate = ["jsonschema>=4.18"](4.18 is thereferencing-rewrite boundary), rolled intoall. Lazily imported, with an actionableImportErrornaming the extra. Adds 4 packages to the lock —jsonschema,jsonschema-specifications,referencing,rpds-py;attrswas already present, so this is one fewer than the spec's estimate, which was measured against gecko's tree.rpds-pyis a compiled Rust extension. Confirmedcp312 manylinux_2_17 x86_64wheels publish, and that this matters: gecko's researcher image ispython:3.12-slimwith onlygitapt-installed — no cargo — so a missing wheel would hard-fail the image build rather than degrade to a source build.jsonschemais also added to thedevextra, for the same reasonpyyamlalready is: it's an optional extra's runtime dependency that the suite needs regardless. The new tests guard onimportorskip("jsonschema"), so without this CI'suv sync --extra dev --extra callbackwould skip the module wholesale and the feature would ship with zero coverage while CI still read green. (Adding--extra validateto the workflow was the first fix attempted; thedevroute follows existing precedent and avoids touching.github/workflows/.)Tests
tests/test_validate_arguments.py, 11 tests: default-off byte-identical behavior and no import, valid pass-through on both paths, all five error shapes above, description passthrough on the anyOf case specifically, dispatch rejection landing beforepre_dispatch, and an actionableImportError.1144 → 1155 passed, 0 skipped under CI's exact
uv sync --extra dev --extra callback.Every new test was mutation-tested. Notably, defeating the
best_matchcombination (deepest.message→top.message) fails exactly the anyOf and$reftests and nothing else — correct, sincetop == deepestfor the other three shapes.Not included
Gecko adoption is a separate judgment call. Per the spec, the value is not in
run_backtest(which has a hand-rolled pydantic gate and keeps it) but in the ungated tools —write_strategy,validate_strategy,analyze_results— which today accept arbitrary junk and fail deep in execution.