Skip to content

ci: do not fail Forge skipped-status on fork-token 403 - #1357

Merged
chaojhou merged 4 commits into
mainfrom
ci/forge-e2e-skipped-status-fork-403
Sep 1, 2026
Merged

ci: do not fail Forge skipped-status on fork-token 403#1357
chaojhou merged 4 commits into
mainfrom
ci/forge-e2e-skipped-status-fork-403

Conversation

@jiagaoxiang

Copy link
Copy Markdown
Collaborator

Summary

skipped-status in .github/workflows/forge-e2e.yml does curl -fsS POST /repos/.../statuses/{sha} with GITHUB_TOKEN. Fork pull_request jobs get a read-only token even with permissions.statuses: write, so that POST is a deterministic 403 and the job check stays red.

ci-e2e/kernelforge is already SUCCESS on the skip-e2e test-gap PRs (posted by a write-capable actor). Only the Actions job check is failing, and that is what merge-blocks #1324, #1327#1330 (and the Forge skip path on #1325).

This is the fail-open one-liner: treat a failed status POST as non-fatal so the skip path can go green.

curl -fsS -X POST ... || echo "status POST not permitted from a fork token; continuing"

Proper fix (not in this PR)

Moving the status POST to pull_request_target or a workflow_run trigger would give a real write token. That is a security tradeoff (those events run in the base-repo context with write + secrets) and should be weighed by the CI owner. This PR does not do that; it only stops a known-unwritable POST from failing a required job.

The same 403 can still hit the e2e job's backstop status POST on fork PRs that do run GPU smoke. Out of scope here — those PRs are gated on the actual e2e result, not skipped-status.

After merge

This file is read from main, so the test-gap PRs stay blocked until this lands and each is re-triggered (empty commit, retest label, or Actions re-run).

Unblocks skip-path merge on: #1324, #1325, #1327, #1328, #1329, #1330.

Test plan

Fork pull_request jobs get a read-only GITHUB_TOKEN even with
permissions.statuses: write, so curl -f POST /statuses/{sha} 403s and
fails the required skipped-status check. ci-e2e/kernelforge is already
SUCCESS on the skip-e2e PRs; keep the job green on that 403.

A pull_request_target or workflow_run trigger would get a real write
token; this change is the fail-open one-liner so skip-path PRs can merge.
@jiagaoxiang
jiagaoxiang requested a review from a team as a code owner September 1, 2026 03:35
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

@zoroyihan7 CI PR is up.

Thanks for the re-review on #1325 / #1327 / #1328 / #1329 / #1330 — noted on the two extras:

This PR is the fail-open one-liner on skipped-status only. Description calls out pull_request_target / workflow_run as the write-token alternative so it doesn't have to block this.

Once this is on main I'll re-trigger the skip-path PRs so they pick up the workflow.

@zoroyihan7 zoroyihan7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — approving. The diagnosis is right and the fix is the minimal thing that works.

I verified the premise before signing off, since the whole fix rests on it: ci-e2e/kernelforge is SUCCESS on all six affected PRs (#1324, #1325, #1327, #1328, #1329, #1330), so turning the job green really does unblock every one of them rather than trading a red check for a permanently pending one.

Also confirmed mechanically:

  • the file parses, and skipped-status has no shell: override, so it runs under the default bash -e
  • under bash -e, curl -fsS ... || echo ... exits 0 on a 403 — simulated it, step exit code 0

One non-blocking suggestion. || echo swallows every failure, not just the fork-token 403. If the POST ever fails for another reason on a PR where kernelforge has not already been posted — API 5xx, a malformed SHA, a future permissions change — the job goes green, the status is never written, and the PR sits pending with nothing red to explain why. The log line is there but nobody reads a green job's log. Cheap hardening:

code=$(curl -sS -o /dev/null -w '%{http_code}' -X POST \
  -H "Authorization: Bearer $GH_TOKEN" \
  -H "Accept: application/vnd.github+json" \
  "$API_URL/repos/$REPO/statuses/$SHA" \
  -d "$(jq -n --arg d "$description" \
    '{state:"success", context:"ci-e2e/kernelforge", description:$d}')")
case "$code" in
  2*)  echo "posted ci-e2e/kernelforge" ;;
  403) echo "::warning::status POST forbidden (fork token is read-only); continuing" ;;
  *)   echo "::error::status POST failed with HTTP $code"; exit 1 ;;
esac

That keeps the fork case green, surfaces it as a warning annotation instead of a buried log line, and still fails loudly on anything unexpected. Happy either way — I don't want to hold up six merges over it, so take it now or as a follow-up.

Agreed on pull_request_target / workflow_run being the real fix and out of scope here, and on the e2e backstop POST having the same problem.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

Hyperloom Forge E2E — ❌ Timeout

item value
result ❌ Timeout
example triton-softmax-forge-loop (max_hours=1.0, max_iters=100)
resources 1× GPU
workspace control-plan-hyperloom-ci
PR branch ci/forge-e2e-skipped-status-fork-403
commit dc235c5f04a34fa2a38890fcc8f65f9458faa9de
session_id 578cc4d6-e97b-467a-bb2d-256b888e19f9
queue → dispatch
run time
total 1m 11s

Actions run

@jiagaoxiang jiagaoxiang added the skip-e2e-test It's a PR that doesn't need to be e2e tested label Sep 1, 2026
… on CI E2E label

The previous curl fallback swallowed every POST failure. Keep the job green
only when the fork token is read-only (403). Also start CI E2E resolve when
skip-e2e-test is added so concurrency can cancel an in-flight GPU smoke.
@jiagaoxiang
jiagaoxiang enabled auto-merge (squash) September 1, 2026 05:57
Skip-path PRs were still queuing on Hyperloom-e2e-ci just to decide not to
run. Resolve only talks to the GitHub API; match Forge E2E and keep the
scarce runner for the actual smoke.
@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

Forge E2E on the previous SHA never left Queued (2h poll deadline, no GPU). This PR only changes skip-path CI, so I added skip-e2e-test rather than occupying the cluster again.

Follow-up commits on this branch:

  • Swallow only HTTP 403 on skipped-status (other curl/HTTP failures still fail the job).
  • CI E2E now re-evaluates when skip-e2e-test is added, so concurrency can cancel an in-flight GPU smoke.
  • CI E2E resolve runs on ubuntu-latest (GitHub API only); Hyperloom-e2e-ci stays on the actual smoke job.

After this lands, skip-path PRs (#1324, #1327#1330) still need a Forge re-trigger so skipped-status picks up the 403 fallback from main.

@jiagaoxiang

Copy link
Copy Markdown
Collaborator Author

@zoroyihan7 checks are green and squash auto-merge is on, but GitHub still reports BLOCKED (the base branch policy prohibits the merge).

Likely cause: require_extra_approval_for_unattributed_changes after I updated this branch from main. That produced merge commit 24b16611 with committer GitHub <noreply@github.com>. Your earlier approval (before that update) may not cover it.

A re-approve on this SHA should let auto-merge land. After that I’ll re-trigger Forge skipped-status on the skip-path test-gap PRs.

@zoroyihan7
zoroyihan7 disabled auto-merge September 1, 2026 08:51
@zoroyihan7
zoroyihan7 enabled auto-merge (squash) September 1, 2026 08:51
@zoroyihan7
zoroyihan7 requested a review from a team September 1, 2026 08:53
@zoroyihan7
zoroyihan7 disabled auto-merge September 1, 2026 08:53
@zoroyihan7
zoroyihan7 requested a review from a team September 1, 2026 08:53
@zoroyihan7
zoroyihan7 requested review from a team and zoroyihan7 and removed request for zoroyihan7 September 1, 2026 09:07
@chaojhou
chaojhou merged commit a7b86a4 into main Sep 1, 2026
31 checks passed
@chaojhou
chaojhou deleted the ci/forge-e2e-skipped-status-fork-403 branch September 1, 2026 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-e2e-test It's a PR that doesn't need to be e2e tested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants