fix(582): preflight warns when kind:tool UDF may not always emit a required schema field#799
Merged
Merged
Conversation
Wire-point assertion at PreflightService.validate() proves the missing warning for a tool whose UDF only conditionally produces a required output-schema field. Reproduces the qanalabs reconstruct_options mid-run crash class (post-568 required-by-default). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ma field
Post-568 (flat schema fields required-by-default), a `kind: tool` action
whose UDF only conditionally produces a declared-required field crashes
mid-run with `Output schema validation failed … 'X' is a required property`.
qanalabs `reconstruct_options` hit this at 39/52 records: `source_quote` was
required, but `apply_edited_distractors` emits it only `if 'source_quote' in
flat`.
Add `find_conditional_required_field_risks`: a bounded intra-function AST
scan that collects the set of constant string keys a UDF unconditionally
writes to its returned mapping (initial dict-literal + top-level subscript
assigns + top-level `update({...})`) and warns on every declared-required
schema field not in that set. The check never imports or executes the UDF.
Any construction shape it does not fully recognise (helper-initialised name,
`**spread`, list return, opaque top-level assignment) yields `None` and no
warning — errs toward silence so unconditionally-emitting tools stay clean.
Wired into `PreflightService.validate()` (step 8) alongside the sibling
passthrough check. Warning only; exit stays 0 — consistent with 566/567/569.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
_last_return used ast.walk, which descends into nested FunctionDef/Lambda bodies. A helper defined AFTER the outer's tail return (dead code, but possible) would provide a return at a higher lineno and shadow the outer's, silently exonerating conditionally-emitted required fields. Iterate func.body statements directly, stopping at nested function/lambda boundaries. Add a regression test. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Static AST checks that reason about a function's OWN control flow must never use ast.walk on the function node — it descends into nested FunctionDef/AsyncFunctionDef/Lambda bodies and their returns will shadow the outer's. Bounded walker starting from func.body is the fix. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Summary
Post-568 (flat schema fields required-by-default), a
kind: toolaction's UDF can declare a field required in the compiled output schema but only emit it inside a conditional branch. The runtime crashes mid-pipeline (_validate_udf_output→'X' is a required property) whenever the guard misses. The qanalabsreconstruct_optionsaction hit this at 39/52 records:source_quotewas required, butapply_edited_distractorsonly writesresult['source_quote'] = flat['source_quote']insideif 'source_quote' in flat.This PR adds a static-AST preflight warning that names action + field before the run starts. Warning-only;
agac inspectexit stays 0, consistent with the 566/567/569 preflight family.Design
agent_actions/validation/udf_required_field_validator.py:inspect.getsource(function).return {"k": ...}, initialresult = {"k": ...}dict-literal assignment, top-levelresult["k"] = ..., top-levelresult.update({"k": ...}).**spread, list return, mid-function reassignment) yieldsNoneand no warning. This preserves "0 false positives on tools that unconditionally emit their required fields" (spec's Verification bullet).FunctionDef/AsyncFunctionDef/Lambdaboundaries so a helper defined below the outer's return cannot shadow it.agent_actions/services/preflight_service.pyadds_collect_tool_required_field_inputs+_warn_tool_conditional_required_field_risks, wired as step 8 after the sibling passthrough check. Readsjson_output_schema.required(the tool path always compiles via theopenaidialect → dict shape).Deviations from the spec: the spec listed
PreflightService.validate()and/orValidateUDFsCommandas wire points.ValidateUDFsCommandoperates on raw config that has not yet been through schema compilation (nojson_output_schema.requiredto compare against), so wiring landed only inPreflightService.agac inspectandagac runboth go throughPreflightService, matching the spec's Success metric.Verification story
gate.sh redrecorded a genuine assertion failure at thePreflightService.validate()wire point (no ImportError-based fake-RED).gate.sh greenre-runs the RED test hash-checked against tampering, then fullpytest(8136 passed),ruff format --check,ruff check,mypy agent_actions.ast.walk); fixed in the follow-up commit with a dedicated regression test (test_nested_helper_return_does_not_shadow_outer_tail).risk.shreportssmoke_required=no; the diff is preflight-only (new validator + wiring), touches no runtime/config-load surface or smoke-registry paths.@udf_toolfunction in the qanalabstools/qanalabs-quiz-gen/tree during development. Onapply_edited_distractorsthe unconditional set is exactly{options, answer, answer_text}(matching the initial dict literal); every other required field is correctly flagged. On tools that emit unconditionally via a full dict literal (e.g.assemble_question), the emitted set covers every required field and no warning fires.Test plan
tests/validation/test_udf_required_field_wiring.py) proves the warning behavior at the publicPreflightService.validate()API without importing the new symbol.tests/validation/test_udf_required_field_validator.py) — 17 cases covering: qanalabs reproducer, unconditional dict literal / via-name / top-level subscript /.update({...}), conditional-only, helper init,**spread, early guard return, list return, subscript-inside-if,additionalProperties: truedoes not suppress the check, no-required-fields no-op, missing source ignored, syntactically-invalid source ignored, multi-action correctness, nested-helper-does-not-shadow-outer-tail.pytestgreen (8136 passed).mypy agent_actionsclean.ruffclean.🤖 Generated with Claude Code