test(ci): prove the lockstep PR job grades the PR's own tree, both directions - #206
Merged
Merged
Conversation
…rections (alpha-engine-config-I10468) dispatch-lockstep.yml's `crucible-dispatch-lockstep-pr` job already fetches both the PR's components.yaml AND its .github/workflows tree as data over the Contents API (landed in crucible-PR198/ab8ed65, alongside the I10467 fix) rather than reading workflows from the pull_request_target base ref — this is the structural fix I10468 asked for. What was missing was proof: nothing exercised the two data-fetch steps' real run: bodies end to end, so nothing would have noticed a regression back to the old base-tree read. Adds three tests to tests/test_workflow_triggers.py, extracting and running the actual "Fetch the PR's components.yaml as data" and "Mirror the PR's .github/workflows as data" step bodies under bash against a fake `gh api` serving a fixture PR-head tree that differs from the base tree already on disk: - test_the_lockstep_pr_job_mirrors_the_prs_tree_not_the_base_tree: an added workflow appears with the PR's content, a deleted one does not survive as base-tree leftover, an unrelated one passes through unchanged, and components.yaml is the PR's content, not main's. - test_the_mirrored_tree_grades_a_new_cron_workflow_both_directions (parametrized): the mirrored tree, graded by the same predicate test_crucible_dispatch_lockstep.py uses, passes when the new cron workflow's components.yaml row is present and fails when it is absent. Verified the tests actually catch the regression they exist to catch: temporarily neutering the Mirror step (`run: "true"`, reproducing the pre-fix base-tree read) makes all three tests fail loudly, not silently pass. Reverted before committing — git diff against origin/main touches only tests/test_workflow_triggers.py. No production code changed: the fix this issue asked for was already live on crucible main (2026-09-10). This closes the remaining gap in I10468's deliverable 2 (a self-test proving the property) and deliverable 3 (pull_request_target carries no PR-head checkout — already covered by test_the_lockstep_pr_job_checks_out_no_pr_head_and_executes_no_pr_supplied_code, unchanged here). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WARhC81oatq9czVeyaM66L
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.
What and why
alpha-engine-config-I10468:dispatch-lockstep.yml'scrucible-dispatch-lockstep-prjob used to grade live main's workflow files against the PR'scomponents.yaml— two different commits, so a PR that only added or removed a workflow's cron could never turn the check green (measured oncrucible-PR198, which removed the offending cron and still failed).Corrected premise, verified against the live repos before writing anything: the issue was filed to
alpha-engine-config(the tracker), and the task that reached me namednous-ergon-opsas the repo to implement in.dispatch-lockstep.ymldoes not live innous-ergon-ops— it lives here, incrucible. And the structural fix I10468 asks for (deliverable 1: readcomponents.yamland the workflow tree from the same commit, without apull_request_targetjob checking out PR-head code) is already merged oncruciblemain —crucible-PR198/ab8ed65(2026-09-10) added the "Mirror the PR's.github/workflowsas data" step alongside the existing "Fetch the PR'scomponents.yamlas data" step, both over the read-only Contents API, so nothing executes PR-supplied code and nothing checks out the PR head.What was not done: a self-test proving that property (I10468 deliverable 2), and end-to-end proof of both directions (a PR adding a workflow with its registry row passes; without the row it fails) against the actual mirrored tree rather than only the pure grading functions.
nous-ergon-ops/tests/crossrepo/test_crucible_dispatch_lockstep.py::TestTheGuardItselfFiresalready proves both directions of the predicate, but nothing exercised the CI job's realrun:bodies that produce the tree the predicate is run against — so a regression back to the base-tree read (the exact I10468 bug) would not have been caught by anything in this repo.This PR closes that gap only. No production code changes.
What changed
tests/test_workflow_triggers.py, three new tests:test_the_lockstep_pr_job_mirrors_the_prs_tree_not_the_base_tree— extracts and runs the real "Fetch ... components.yaml" and "Mirror ... workflows" step bodies under bash, against a fakegh apiserving a fixture PR-head tree that differs from the base tree already on disk. Asserts: an added workflow appears with the PR's content, a deleted workflow does not survive as base-tree leftover, an unrelated workflow passes through unchanged, andcomponents.yamlends up as the PR's content, not main's.test_the_mirrored_tree_grades_a_new_cron_workflow_both_directions(parametrizedwith-registry-row/without-registry-row) — runs the same mirror, then applies the guard's own predicate (_check_every_cron_workflow_is_named_by_a_row, restated minimally so this test has no dependency on thenous-ergon-opscheckout) to the resulting tree. Both-directions proof: the new cron workflow grades named when itsdispatch_workflowrow is present, undeclared when it is absent.Both-directions proof (deliverable 2)
Verified these tests actually catch the regression they exist to catch — not just that they pass. Temporarily neutered the Mirror step (
run: "true", reproducing the pre-fix base-tree read) and reran:All three fail loudly, not silently pass.
dispatch-lockstep.ymlwas reverted (git checkout --) before committing — the diff touches onlytests/test_workflow_triggers.py.Supply-chain note (deliverable 3)
No change needed: the
crucible-dispatch-lockstep-prjob already avoids checking out PR-head code underpull_request_target— its self-checkout takes the implicit base ref, and the two pieces of PR content it needs (components.yaml,.github/workflows/*) arrive as data over the read-only Contents API, base64-decoded straight to files, never executed. That property is already asserted bytest_the_lockstep_pr_job_checks_out_no_pr_head_and_executes_no_pr_supplied_code(unchanged here).Test plan (full suite, before opening this PR)
tests/acceptance/is excluded per this repo's own convention — it fails by design as the phase gate.Closes: none across repos (the tracker is
alpha-engine-config; per fleet convention the issue is closed by hand with this PR cited, not by a keyword).Prepared by: Claude Opus 5 (1M context) via Claude Code
https://claude.ai/code/session_01WARhC81oatq9czVeyaM66L