Give Python callers the findings the CLI already prints (#104) - #481
Open
jeremymanning wants to merge 2 commits into
Open
Give Python callers the findings the CLI already prints (#104)#481jeremymanning wants to merge 2 commits into
jeremymanning wants to merge 2 commits into
Conversation
#471 established which parts of a step the runtime renders and wired that into dependency inference, so two inert strings stopped inventing a cycle. Template validation was still reading those same fields as if they resolved, and got both directions wrong: - id: a name: "{{ b.result }}" # "will be resolved at runtime" description: "{{ nosuch }}" # a hard error Nothing substitutes into `name`, so the braces reach the log verbatim and the warning told the reader the opposite of what happens. And a stray brace in prose *failed the pipeline* -- a false rejection of a document that runs correctly, which is the class removed by #465, #469 and #472 elsewhere. Inert fields now produce a warning that says what actually occurs, and nothing else. Renderable fields are untouched: an undefined name in `parameters`, `action` or `location` is still an error. The same defect exists one level up -- a pipeline's own `name:` was rejected too -- so `INERT_PIPELINE_FIELDS` sits alongside the step set. It is deliberately shorter than it first was: `version` is schema-constrained to `\d+\.\d+\.\d+`, so a template there is a real error and calling the field inert would describe it wrongly. The run in `test_a_pipeline_with_templates_in_prose_still_runs` is what caught that, and is the evidence for every field in both sets: a pipeline carrying an unresolvable reference in each prose field still executes and still writes the right contents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`orchestrator validate --json` has emitted structured findings since #467. `PipelineAPI.validate_yaml` returns a bare `bool`, so a caller embedding the orchestrator could learn *that* a document was rejected and nothing about why, and could not see warnings at all -- which is where "this reference could not be checked" lives, the warning that precedes the run-time failure in #465. The findings were not missing, only private to `cli.py`. Exposing them by writing a second implementation would have created two things to drift apart, which is the bug #466 removed for dependencies, so `validation/pipeline_report` is the one implementation and the CLI formats what it returns. A test compares the CLI's JSON against the API's objects to keep it that way. While testing it, `validate` turned out not to be reproducible. Three runs of the same command over the same file produced the same 44 findings in three different orders, from two hash-order sources: * findings were emitted while iterating `set(var_names) | set(...)`; * `_suggest_similar_names` iterated an unsorted candidate list *and truncates to three*, so on a longer list it would have offered different suggestions run to run rather than merely reordering them. Both are now sorted, and repeated runs are byte-identical. The in-process check cannot catch this -- `PYTHONHASHSEED` is fixed for the life of a process -- so the test runs three subprocesses. The first version of this suite was written against `examples/supported/01_hello_filesystem.yaml`, which produces no findings at all: deleting every warning from the implementation still passed all of it. It is now anchored on a document carrying 44 findings, with explicit guards so an empty list cannot pass as agreement. Co-Authored-By: Claude Opus 5 (1M context) <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 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.
Review item 4. Merge after #480 — this branch is stacked on it, so its diff includes #480's commit until that lands.
The gap
orchestrator validate --jsonhas emitted structured findings since #467.PipelineAPI.validate_yamlreturns a barebool, so a caller embedding the orchestrator could learn that a document was rejected and nothing about why — and could not see warnings at all, which is where "this reference could not be checked" lives: the warning that precedes the run-time failure in #465.The findings were never missing, only private to
cli.py(_reportable_issues,_issue_payload).Why this is a move, not an addition
Writing a second implementation for the API would have created two things to drift apart — the bug #466 removed for dependencies.
validation/pipeline_report.pyis the one implementation; the CLI now formats what it returns, and its private helpers are deleted.test_the_cli_and_the_api_report_the_same_findingscompares the CLI's JSON against the API's objects to keep it that way.Exported from the package:
validate_pipeline_file,validate_pipeline_text,PipelineValidation,Finding.Compilation failure is a result, not an exception: a caller validating user input wants the findings alongside the failure, and raising discards them at the moment they are most useful.
orchestrator validatewas not reproducibleYour list asked for deterministic warning order. It wasn't:
Same 44 findings, three orders. Two hash-order sources:
set(var_names) | set(loop_var_matches);_suggest_similar_namesiterating an unsorted candidate list and truncating to three — so on a longer list it would have offered genuinely different suggestions run to run, not merely reordered ones.Both sorted. Three separate processes now produce byte-identical output.
Two of my own tests proved nothing
Reporting these because they are the reason to trust the rest:
examples/supported/01_hello_filesystem.yaml, which produces no findings. Deleting every warning from the implementation still passed all seven tests — a suite for "warnings reach the API" that could not detect warnings never arriving. It is now anchored on a document carrying 44 findings, with explicitassert result.findingsguards so an empty list cannot pass as agreement.PYTHONHASHSEEDis fixed, and passed while the CLI visibly varied. The real check runs three subprocesses.Neither was caught by rereading the tests; both were caught by mutating the implementation and checking that a test failed.
Verification
ruff check src/orchestrator --select E9,F63,F7,F82,F821,F823,F601,F811— cleanGates were re-run on the rebased branch in an isolated
git worktree, after an earlier post-rebase run was invalidated by the shared checkout switching branches mid-run.