Conversation
When `context` failed the gate job was skipped entirely, so whatever status was last written to that head SHA stood. A signoff edited or deleted without a push therefore left an earlier `success` green on an unsigned major. The PR lookup now retries, and a `pull_request` run whose context still fails posts a failing status from the event payload. `issue_comment` payloads carry no head SHA, so that path relies on the retry and on the next push. Found by Greptile on the RN Expo port (#201); the condition is identical in both workflows, so this lands here first.
🦋 Changeset detectedLatest commit: 6807b30 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
jhampton
left a comment
jhampton
left a comment
There was a problem hiding this comment.
Interestingly, this seems like it also forces a changeset to be present, and I think that's right...except for the "pure" DevOps cases. At a minimum, can you make it clear in the wording that PR's require a changeset where this logic is tripped?
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Summary
Standards: 0 must-fix. Spec: 0 must-fix. Primary concern: none. One non-blocking suggestion would make the regression test prove that the fallback status is actually red.
Review evidence
- Scope: reviewed revision, YPE-5942, the complete PR diff, surrounding workflow, tests, permissions, and existing review discussion.
- Method: independently evaluated repository standards, ticket intent, correctness, cancellation, retries, event payloads, SHA recovery, and the combined result with current
main.
| Behavior or check | Method / command | Result | Evidence source |
|---|---|---|---|
| CI script regressions | pnpm test:ci-scripts |
49 passed | Coordinator |
Combined PR and current main |
Synthetic merge plus pnpm test:ci-scripts |
50 passed, including 38 signoff tests | Coordinator |
| Formatting and patch integrity | Prettier and git diff --check |
Passed | Coordinator |
| Test detects a green fallback mutation | Changed state=failure to state=success in a temporary copy |
It did not; all 37 tests still passed | Coordinator |
- Limits: no live GitHub API outage was induced. GitHub event and status semantics were source-traced and exercised through the repository's mocks.
- CI and bot review: CI is green at the reviewed revision. Greptile's missing-changeset finding is addressed by the empty changeset, though its thread remains open.
- Event: APPROVE.
Written by Code Reviewer bot on behalf of Cam.
| "https://x-access-token:$GH_TOKEN@github.com/$REPOSITORY" \ | ||
| "refs/pull/$EVENT_ISSUE_NUMBER/head" | cut -f1) || HEAD_SHA="" | ||
| fi | ||
| if ! [[ "$HEAD_SHA" =~ ^[0-9a-f]{40}$ ]]; then |
There was a problem hiding this comment.
praise: Validating the recovered value as a full SHA before writing status keeps this fallback narrow and fail-closed.
For Agents: event-safe SHA recovery
This path prefers the immutable payload SHA for
pull_request, uses the exact pull ref only whenissue_commentlacks that field, and refuses to address a status when neither source yields a valid commit. The separate failure-only job also avoids turning superseded cancellations into misleading red statuses.
Written by Code Reviewer bot on behalf of Cam.
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Summary
Standards: 0 must-fix. Spec: 0 must-fix. Primary concern: none. The prior test-coverage suggestion is resolved.
Review evidence
- Scope: reviewed revision, the complete current PR, changes since the prior review, both merges from
main, YPE-5942, and existing review threads. - Method: traced the status fallback and strengthened test assertions, then reran the focused suite and mutations from the earlier finding.
| Behavior or check | Method / command | Result | Evidence source |
|---|---|---|---|
| CI script regressions | pnpm test:ci-scripts |
50 passed | Coordinator |
| Green fallback mutation | Changed state=failure to state=success in a temporary copy |
Detected: 2 tests failed | Coordinator |
| Successful fallback-job mutation | Changed the fallback's final exit to zero in a temporary copy | Detected: 2 tests failed | Coordinator |
| Formatting and patch integrity | Prettier and git diff --check |
Passed | Coordinator |
- Limits: no live GitHub API outage was induced. Event and status behavior remains covered through source tracing and repository mocks.
- CI and bot review: all checks passed at the reviewed revision.
- Event: APPROVE.
Written by Code Reviewer bot on behalf of Cam.
| [[ "$call" == *"target_url=https://example.invalid/run"* ]] || problem="${problem:-no target_url}" | ||
| [[ "$call" == *"Could not resolve PR context"* ]] || problem="${problem:-no description}" | ||
| # The job exits nonzero so the check is red in the PR UI, not merely recorded. | ||
| [ "$result" -ne 0 ] || problem="${problem:-job exited 0}" |
There was a problem hiding this comment.
praise: Checking the posted failure payload and the job result closes the exact false-confidence gap from the earlier test.
For Agents: fail-closed regression coverage
The test now proves both observable outcomes: the commit receives the
major-release-signofffailure status, and the workflow job remains red. Independent mutations to postsuccessor exit zero each fail the focused cases.
Written by Code Reviewer bot on behalf of Cam.
This branch has not been deployed
3 participants
Closes YPE-5942.
Problem
contextresolves the head SHA for the whole workflow. When it fails, every downstream job is skipped and whatever status was last written to that SHA stands, including asuccesswhose signoff comment has since been edited or deleted. A major release could keep a green required check with its approval evidence gone.Greptile flagged the same thing on the Expo port (#201) and agreed to defer it to this upstream fix.
Change
context_unresolved, which posts a failing status whencontextfails. It runs on every event, not justpull_request:issue_commentpayloads carry no head SHA, so it readsrefs/pull/N/headover git. That is a different backend from the REST call that just failed, and needs no scope beyondcontents: read.The job is gated on
failure, not!= success, so a cancelled run superseded by a newer one does not paint the PR red.Tests
.github/scripts/major-release-signoff.test.sh, 37 passing in ~2.5s.Five new cases cover the retry (recovery and give-up, asserted by call count) and the status job (payload head, git-recovered head, unrecoverable head). Each was mutation-tested: breaking any one of the five production paths turns the suite red. A loop that never retries, a job re-gated to
pull_request, a dropped SHA recovery, an ignored payload SHA, and a removed SHA validation are all caught.The previous revision carried a
RETRY_BACKOFF_SECONDSenv override so the suite could skip the backoff. That was test-only surface in a production workflow; the suite now stubssleepinstead.https://claude.ai/code/session_01Gnt93sVjs6ApXSzkDC2efU
The PR appears safe to merge; no new actionable failure was established.
Summary
The PR retries PR context lookup and adds a fallback job that posts a failing signoff status when context resolution fails. It also adds an empty changeset and regression tests.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD Event[PR or comment event] --> Context[Resolve PR context: up to 3 attempts] Context -->|success| Gate[Evaluate release and signoff] Context -->|failure| Head{Head SHA available?} Head -->|PR payload| Failure[Post failing signoff status] Head -->|Comment: git PR head ref| Failure Head -->|No recoverable SHA| NoStatus[Fail job without posting a status]Reviews (5) · Last reviewed commit: "Merge branch 'main' into YPE-5942/gate-s..."