Skip to content

The documented aggregator accepts a skipped validator where the hub's own workflow requires success #1211

Description

@ptr727

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions