The canonical pull request stub in docs/reusable-workflows.md documents an aggregator that accepts skipped for every job it checks, including the validation job. The hub's own shipped workflow does not. An adopter copying the documented stub therefore gets a weaker gate than the repository that publishes it.
The divergence
docs/reusable-workflows.md:351-357, the stub adopters copy:
for result in "changes:${{ needs.changes.result }}" "validate:${{ needs.validate.result }}" "smoke-build:${{ needs.smoke-build.result }}"; do
name="${result%%:*}"
value="${result#*:}"
if [[ "$value" != "success" && "$value" != "skipped" ]]; then
echo "::error::Job '$name' did not succeed ($value)."
exit 1
fi
done
.github/workflows/test-pull-request.yml:32-35, what this repository actually runs:
if [[ "${{ needs.validate.result }}" != "success" ]]; then
echo "Job 'validate' did not succeed (${{ needs.validate.result }}); refusing to pass."
exit 1
fi
The hub requires success for validate. The stub accepts skipped for it.
Why the allowlist does not reach this case
GOVERNANCE.md "Workflow YAML Conventions" states the rule the stub is applying: "Allowlist success and skipped explicitly when chaining jobs across optional dependencies, since != 'failure' lets cancelled through". The allowlist is scoped to optional dependencies. smoke-build is optional and skips by design when the paths filter marks nothing, which is the case D1.5 requires be treated as pass. validate is not optional. D1.2's output is that "a validation job runs unconditionally and the aggregator needs: it", and its Prevents clause names the failure exactly: "a PR merging with no validation".
Applying the optional-dependency allowlist to a mandatory dependency is what makes the two files disagree.
Reachability, stated honestly
Not reachable for the validator the stub ships. The stub's validate job carries uses: and permissions: with no if: and no needs:, so neither skip mechanism exists, and the reusable validate-task.yml it calls declares its three jobs with no job-level if: or needs: either, so no leg of the callee can skip and take the caller with it.
Reachable for a replacement validator, which D1.2 explicitly permits. D1.2 says "A repo whose validation it cannot express replaces the call (not deletes it) with its own validator and re-points the aggregator's needs: to the replacement." A replacement carrying a conditional job can reach skipped, and this loop passes it. So the hole is latent rather than live, and it opens on exactly the customization the contract invites.
Provenance
Introduced in #760 (b192e3b4, "Host the Validate Task and Reshape the Test Pull Request Stub"), in one commit, and unchanged since. It is not drift from the hub's own workflow, since both were written in that reshape.
Suggested resolution
Require success for validate and changes, and accept skipped only for smoke-build. That matches the hub's own workflow, matches D1.5's "treat a skipped smoke build as pass" as the narrow exception it is written as, and keeps cancelled blocking on every job.
Whether the loop should stay a loop or split into the hub's explicit per-job form is a style question this does not settle.
Related
Raised by CodeRabbit on #1203, the develop-to-main promotion PR, as an outside-diff finding, and deferred out of it because the text is pre-existing on main rather than something that promotion introduced. Adjacent to #1201, which covers what the section 5 audit does and does not reach.
The canonical pull request stub in
docs/reusable-workflows.mddocuments an aggregator that acceptsskippedfor every job it checks, including the validation job. The hub's own shipped workflow does not. An adopter copying the documented stub therefore gets a weaker gate than the repository that publishes it.The divergence
docs/reusable-workflows.md:351-357, the stub adopters copy:.github/workflows/test-pull-request.yml:32-35, what this repository actually runs:The hub requires
successforvalidate. The stub acceptsskippedfor it.Why the allowlist does not reach this case
GOVERNANCE.md"Workflow YAML Conventions" states the rule the stub is applying: "Allowlistsuccessandskippedexplicitly when chaining jobs across optional dependencies, since!= 'failure'letscancelledthrough". The allowlist is scoped to optional dependencies.smoke-buildis optional and skips by design when the paths filter marks nothing, which is the case D1.5 requires be treated as pass.validateis not optional. D1.2's output is that "a validation job runs unconditionally and the aggregatorneeds:it", and its Prevents clause names the failure exactly: "a PR merging with no validation".Applying the optional-dependency allowlist to a mandatory dependency is what makes the two files disagree.
Reachability, stated honestly
Not reachable for the validator the stub ships. The stub's
validatejob carriesuses:andpermissions:with noif:and noneeds:, so neither skip mechanism exists, and the reusablevalidate-task.ymlit calls declares its three jobs with no job-levelif:orneeds:either, so no leg of the callee can skip and take the caller with it.Reachable for a replacement validator, which D1.2 explicitly permits. D1.2 says "A repo whose validation it cannot express replaces the call (not deletes it) with its own validator and re-points the aggregator's
needs:to the replacement." A replacement carrying a conditional job can reachskipped, and this loop passes it. So the hole is latent rather than live, and it opens on exactly the customization the contract invites.Provenance
Introduced in #760 (
b192e3b4, "Host the Validate Task and Reshape the Test Pull Request Stub"), in one commit, and unchanged since. It is not drift from the hub's own workflow, since both were written in that reshape.Suggested resolution
Require
successforvalidateandchanges, and acceptskippedonly forsmoke-build. That matches the hub's own workflow, matches D1.5's "treat a skipped smoke build as pass" as the narrow exception it is written as, and keepscancelledblocking on every job.Whether the loop should stay a loop or split into the hub's explicit per-job form is a style question this does not settle.
Related
Raised by CodeRabbit on #1203, the develop-to-main promotion PR, as an outside-diff finding, and deferred out of it because the text is pre-existing on
mainrather than something that promotion introduced. Adjacent to #1201, which covers what the section 5 audit does and does not reach.