Skip to content

fix(ci): restore fork-safe token fallback on the CI timing-report job - #66659

Closed
Drexuxux wants to merge 1 commit into
NousResearch:mainfrom
Drexuxux:fix/ci-timing-report-fork-safe-token
Closed

fix(ci): restore fork-safe token fallback on the CI timing-report job#66659
Drexuxux wants to merge 1 commit into
NousResearch:mainfrom
Drexuxux:fix/ci-timing-report-fork-safe-token

Conversation

@Drexuxux

@Drexuxux Drexuxux commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

What

#66373 swapped GITHUB_TOKENAUTOFIX_BOT_PAT across the workflows. That PAT is empty on fork PRs (forks get no repo secrets). 1e01a4b restored the || github.token fallback for detect-changes / lint / supply-chain — but the ci-timings job was missed: its Collect timings and generate report step still passed a bare GITHUB_TOKEN: ${{ secrets.AUTOFIX_BOT_PAT }}.

So on every fork PR the step received an empty GITHUB_TOKEN, and scripts/ci/timings_report.py crashed at expect_env("GITHUB_TOKEN"):

ValueError: missing environment variable GITHUB_TOKEN

reddening the PR — even though this job's own contract is "a missing report must never redden the PR" (it already exits 0 on TimingsUnavailable).

Fix

Two layers:

  • .github/workflows/ci.yml — add the || github.token fallback so the observability job gets the run's read-only token on fork PRs (mirrors the detect-changes fallback), so timings are actually collected.
  • scripts/ci/timings_report.py — treat an absent/empty GITHUB_TOKEN as TimingsUnavailable and route it through the existing graceful degraded path (placeholder report + summary, exit 0) instead of a hard crash — keeping the "never reddens the PR" invariant true regardless of how the token is wired.

Tests

tests/ci/test_timings_report.py — an unset and an empty GITHUB_TOKEN both exit 0 with a placeholder report and no cached JSON. Before the fix the run raised ValueError.

pytest tests/ci/test_timings_report.py -q
# 2 passed

NousResearch#66373 swapped `GITHUB_TOKEN` → `AUTOFIX_BOT_PAT` across the workflows; that
PAT is empty on fork PRs (forks get no repo secrets). 1e01a4b restored the
`|| github.token` fallback for detect-changes / lint / supply-chain, but the
`ci-timings` job was missed — its "Collect timings" step still passed a bare
`GITHUB_TOKEN: ${{ secrets.AUTOFIX_BOT_PAT }}`.

So on every fork PR the timing-report step received an empty `GITHUB_TOKEN`
and `timings_report.py` crashed at `expect_env("GITHUB_TOKEN")` with
`ValueError: missing environment variable GITHUB_TOKEN`, reddening the PR —
even though the job's own contract is "a missing report must never redden the
PR" (it already exits 0 on `TimingsUnavailable`).

Two-layer fix:
- ci.yml: add the `|| github.token` fallback so the observability job gets the
  run's read-only token on fork PRs (mirrors the detect-changes fallback), so
  timings are actually collected.
- timings_report.py: treat an absent/empty `GITHUB_TOKEN` as `TimingsUnavailable`
  and route it through the existing graceful degraded path (placeholder report
  + summary, exit 0) instead of a hard crash — keeping the "never reddens the
  PR" invariant true regardless of how the token is wired.

Tests: `tests/ci/test_timings_report.py` — an unset and an empty `GITHUB_TOKEN`
both exit 0 with a placeholder report and no cached JSON; before the fix the
run raised `ValueError`.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation labels Jul 18, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to merged #66577: that fork-token repair covered change classification and review gates; this PR independently covers the CI timings job and its missing-token degradation path.

@joelbrilliant

Copy link
Copy Markdown
Contributor

Independent confirmation from an affected fork contributor: I hit this on every PR I opened this week (#66646, #66653, #66656 all show the red "CI timing report" job with ValueError: missing environment variable GITHUB_TOKEN at timings_report.py:926 while "All required checks pass" is green) and root-caused it to the same missing fallback before finding this PR — my narrower script-only fix (#66665) is now closed in favour of this one. The workflow-layer || github.token restore here is the right shape since it makes the report actually work on fork PRs instead of just degrading, and the script-side guard keeps the never-redden invariant honest for any future wiring gap. Would love to see this land — it currently emails every community contributor a failure per push.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Fixes CI timing-report job for fork PRs with two-layer defense:

  • Adds || github.token fallback in ci.yml so the job gets a read-only token on fork PRs
  • Makes scripts/ci/timings_report.py treat absent/empty GITHUB_TOKEN as TimingsUnavailable (graceful degraded path, exits 0) — keeps the "never reddens the PR" invariant
  • 2 new tests covering both unset and empty GITHUB_TOKEN

Clean fix. LGTM.


Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused fork-CI repair.

Automated hermes-sweeper review found the requested behavior already on current main via merged #66738:

  • .github/workflows/ci.yml:232 now uses secrets.AUTOFIX_BOT_PAT || github.token for the ci-timings collection step.
  • scripts/ci/timings_report.py:934-949 routes a missing or empty token through TimingsUnavailable, writes the degraded outputs, and exits 0.
  • The implementation was merged as commit ecd54a001e6a7c13fa2562859ec4703b9520977f in fix(ci): make timings report fork-safe (missed by #66577) #66738.

Closing as implemented on main.

@teknium1 teknium1 closed this Jul 18, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants