Repository navigation
ci: drop ready_for_review from the witness-floor pull-request trigger - #9361
Conversation
A draft PR is already tested from Opened, so ready_for_review only ever re-runs a subject the floor has already measured on the same head — a duplicate run against a fixed runner pool. Operator ruling. The activity set is authored in gunbc.witness_floor_workflow; the emitted .github/workflows/witnesses.yml is updated to match. ReadyForReview stays declared in extdeps.github.actions (it is a real GitHub activity type); only this workflow's subscription is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
The authority cut is correct, but this head is missing two parts of the operator-ruled scope.
-
Enrol a discriminating emitted-shape witness in
dag/test/claim/witness_floor_workflow_consolidation_witness_test.dag: the exact pull-request activity list still containsopened,synchronize, andreopened, and does not containready_for_review. The positive arm is required so deleting the entire pull-request trigger cannot satisfy the negative. -
Correct the body’s absolute claim that readiness “only ever” reruns the same subject. An unchanged head can face a moved base while the PR sat in draft. That does not rescue the lifecycle trigger—it is accidental, incomplete base-congruence coverage—but the limit must be stated.
The current build is green and floor is still executing; no merge decision is requested on this head.
The trigger change in this PR was +3/-3 over a fact nothing asserted, so neither the deletion nor its reversion was observable. This row reads the emitted workflow text and asserts the surviving three activities BY NAME plus the absence of ready_for_review -- an absence-only assertion greens over a total deletion of the activity list, which is a different defect. Spellings come from the emitter's own pr_activity_string, so a respelling moves both sides together instead of vacating the negative. Executed both ways: PASS as authored; FAIL with ReadyForReview re-added, and FAIL with the activity list emptied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
Both requested changes are closed on exact head dea86ed05c424d481ebd530fb70cfb7a1eccf2c2.
-
The emitted-shape witness is discriminating in both directions: it asserts the exact surviving activity line through the emitter's own
pr_activity_stringauthority, rejectsReadyForReview, rejectsClosed, returns false on generation refusal, and has executed RED controls for both re-addingReadyForReviewand deleting the whole activity list. -
The moved-base limitation is now explicit: draft-to-ready may coincide with a new base under an unchanged head, but it is not a base-congruence carrier and cannot substitute for one.
The prior CHANGES_REQUESTED applied to the earlier head and is superseded by this approval.
Operator-ruled, exact scope given: delete
ReadyForReviewfrom the witness-floor workflow's pull-request activity set.Why the trigger goes, stated at the grain that is true
The question that matters is: has this head been verified against the relevant base? The event being consumed is: did the PR move from draft to ready? Those correlate and are not equivalent.
ready_for_reviewaccidentally catches some base movement that happened before the undraft, misses base movement after it, misses drafts that stay drafts, and re-runs subjects where neither head nor base moved.This workflow already tests drafts from
Opened(there is no draft filter anywhere in the emitted file), so what remains of the trigger is duplicate runs against a fixed runner pool.This PR does not claim the trigger can never observe anything new. Main may move while the head is unchanged, so an undraft can face a base it has genuinely not been tested against. That is a real case, and it is why any future base-congruence requirement needs a carrier naming the base sha or merge subject directly — a lifecycle event cannot stand in for it. The limitation is stated as a limitation, not as an incapacity.
Scope: this is an independent waste repair. It does not touch push-over-push cancellation semantics or the superseded-run handling in #9339/#9326, and claims nothing about them.
Changes
gunbc.witness_floor_workflowwitness_floor_workflow— activity set now[Opened, Synchronize, Reopened], unusedReadyForReviewimport dropped.github/workflows/witnesses.yml—types: [opened, synchronize, reopened]test.claim.witness_floor_workflow_consolidation_witness_testw_RED_the_pull_request_activity_set_is_the_three_that_carry_a_new_subjectThe witness, and why both halves
The activity set was previously asserted by nothing, so the deletion and its reversion were both invisible. The row reads the emitted workflow text and asserts:
opened,synchronize,reopened— as the exact emittedtypes:lineready_for_reviewabsent (andclosed, which this workflow also does not subscribe to)Absence alone is not enough: an absence-only row greens over a total deletion of the activity list, which is a different defect (the workflow would then run on every PR activity GitHub defines) passing the same check. Spellings are read through the emitter's own
pr_activity_string, so a respelling moves both sides together rather than silently vacating the negative.Executed, both directions, via
claim_batchon this head:PASSReadyForReviewre-added to the activity setFAILtypes: [])FAILTwo independent REDs, each authored on the authority (not the witness), so the row distinguishes this change from an over-deletion.
ReadyForReviewremains declared inextdeps.github.actionsand mapped inextdeps.languages.yaml.gha_workflow— it is a real upstream GitHub activity type; only this workflow's subscription to it is removed. TheMarkReadyForReviewhits ingunbc.code_change_workfloware the code-change state machine, a different subject, untouched.🤖 Generated with Claude Code