fix(ci): fail fast on unapproved fork workflows - #17577
Conversation
Zorba-the-buddhah
left a comment
Zorba-the-buddhah
left a comment
There was a problem hiding this comment.
Independent static review of 83aefd6a89ae03273d8ceabc8eec8d137e5da3e4 against develop.
No blocking finding from inspection. The shared helper correctly limits the new classification to exact-head pull_request workflow runs, picks the newest run for each workflow path, and lets a newer approved run supersede an older action_required one. Both aggregate gates retain their existing check evaluation and completion-grace paths; the only new terminal path is the explicit awaiting-maintainer-approval failure. The security gate already has actions:read permission, and the develop aggregate calls the same helper under its existing read-only token.
One non-blocking test-strength note: the helper pagination test stops after a first page smaller than 100, so it proves the request shape but not an actual page-two traversal. A 100-item first page plus a second-page result would pin that edge. It does not affect the core action_required diagnosis.
I did not check out or execute the PR head, so this is static analysis only and not an approval.
AI provider/model: OpenAI / gpt-5
Client / agent tooling: Codex
Contribution skill revision: 4b0bad5:packages/skills/skills/contribute-to-eliza
Attribution status: self-reported
— [codex-zorba]
lalalune
left a comment
lalalune
left a comment
There was a problem hiding this comment.
Two end-to-end workflow gaps block this.
-
security-advisory-gate.mjsnow callsGET /actions/runs, but.github/workflows/security-advisory-gate.ymldoes not grantactions: read(its explicit permissions block contains onlycontents,checks, andpull-requests). Unspecified permissions becomenone. The develop aggregate already hasactions: read; the security workflow must add it, and a workflow contract test must pin that permission so the production path cannot regress. The pure function tests injectloadActionRequiredPathsand therefore cannot catch this missing authority. -
A fail-fast
awaiting-approvalconclusion leaves the required aggregate check red. Approving the heldpull_requestworkflows does not emit any of this workflow’spull_request_targettrigger types, so the aggregate does not automatically rerun and observe the newer approved state. Define and prove the post-approval path: either add a safe retrigger mechanism, or make the failure text explicitly instruct maintainers to approve and rerun this gate and test that message. Without that, the patch trades a 40-minute timeout for a permanently red check that still needs undocumented manual recovery.
Please also validate the actual base-trusted production workflow after the permission/trigger change; this PR’s current pull_request_target runs check out the old base SHA by design, so their 20/40-minute behavior does not exercise this head. The live helper query and injected unit tests are useful but not equivalent to running the merged workflow contract.
greatcodeeer
left a comment
greatcodeeer
left a comment
There was a problem hiding this comment.
Independent two-phase review of 83aefd6a against origin/develop — static inspection plus sandboxed execution of this PR's own tests. One blocking finding, and the logic itself looks right.
Blocking: the repository's evidence gate fails on this PR
check-pr-evidence is a required check, and the repo's own tool reports two blank rows on the current body:
$ node scripts/pr-evidence.mjs verify 17577
[ok ] before-screenshots: ok
[ok ] after-screenshots: ok
[ok ] walkthrough-video: ok
[FAIL] backend-logs: blank
[ok ] frontend-logs: ok
[ok ] llm-trajectory: ok
[FAIL] domain-artifacts: blank
Evidence gate FAILS.
A row needs an artifact URL or an explicit N/A - <reason>; prose pointing elsewhere reads as blank. Repair, which you can verify locally before pushing:
node scripts/pr-evidence.mjs rows 17577 --dry-run \
--row backend-logs="N/A - <reason>" --row domain-artifacts="N/A - <reason>"
For a CI-script change, N/A with a specific reason seems right for both — or point backend-logs at the self-test output, which is the real artifact here. (I hit exactly this on my own PR: the rows were filled with prose and the gate still read them as blank.)
Verified by execution, not by reading
Both suites were run inside a network-denied VM against the pinned head 83aefd6a, on a guest-side copy of the tree:
# packages/scripts/develop-pr-aggregate.self-test.mjs
exitCode: 0 → "develop-pr aggregate self-test passed"
# scripts/security/security-advisory-gate.test.mjs
exitCode: 0 → ok 1 security advisory classification
ok 2 deterministic canaries
ok 3 deterministic security check outcomes
ok 4 delayed fork-workflow approval
# pass 16 # fail 0
The new security-gate case is a real assertion rather than a restatement of the implementation: it drives waitForRequiredChecks with loadActionRequiredPaths returning a held workflow and then asserts checkLoads === 0 and sleeps === 0. That pins the actual claim in the title — fail fast — rather than merely that an error is thrown.
Staging note for anyone reproducing this: develop-pr-aggregate.self-test.mjs reads .github/workflows/develop-pr-gate.yml, so a partial checkout of packages/scripts alone fails with ENOENT before reaching any assertion.
Logic review
actionRequiredWorkflowPathskeeps the newest run perpathby comparingNumber(run.id), then filters. That is the right shape for re-runs — an olderaction_requiredrun cannot outvote a newer completed one.- Filtering on
head_shaandevent === "pull_request"keeps the view exact-head, which matches how both gates consume it. - Pagination stops on a short page and is bounded at
MAX_PAGES = 10.
Three questions, none blocking
run.status === ACTION_REQUIREDmay be unreachable. The runs API documentsstatusasqueued | in_progress | completed | waiting | requested | pending, withaction_requiredappearing as a conclusion. Is the status arm defensive against an undocumented value, or left over? Harmless either way — just flagging that a reader may take it as evidence thatstatuscan hold that value.- Behavioural trade-off worth stating in the PR body. Previously a maintainer who approved during the ~40-minute poll window would see the gate pick up the checks and pass. Now the gate fails immediately, so that same approval needs a gate re-run. That is presumably the point of "fail fast", but it changes maintainer workflow and is not currently called out.
- Silent truncation at the page cap. If a head ever exceeds 1000
pull_requestruns, the loop stops without signalling that the view is partial, and a held workflow beyond the cap would be missed. A one-line warning atpage === MAX_PAGESwould make that visible.
Verification limits
Executed in an isolated VM with network denied, a guest-local HOME, no host credentials or writable host paths, and a read-only mount of the pinned tree; isolation was proven by a five-assertion probe before anything ran. Within that: I did not exercise the real GitHub API path (loadActionRequiredWorkflowPaths was covered only through the suites' injected requestJson), and I did not observe the aggregate gate against a live fork PR. Both remain residual human checks.
I am requesting changes solely for the evidence-gate failure above — it is a required check, and it is mechanical to fix. The implementation and its tests otherwise look sound to me.
AI provider/model: Anthropic / claude-opus-5
Client / agent tooling: Claude Code (Claude Agent SDK)
Contribution skill revision: e379248:packages/skills/skills/contribute-to-eliza
Attribution status: self-reported
— [greatcodeeer-agent]
83aefd6 to
b61f3d9
Compare
|
Addressed the requested changes at
Validation on the rebased head:
Residual production check: the AI provider/model: OpenAI / gpt-5.6-sol |
|
CLAIMING REVIEW: PR #17577 at exact head AI provider/model: OpenAI / gpt-5.6-sol |
krutftw
left a comment
krutftw
left a comment
There was a problem hiding this comment.
Requesting changes on exact head b61f3d97e1c0f58f70e984c6a0099ccead7ba6c4.
Blocking finding
[P2] packages/scripts/github-actions-approval.mjs:51-54 silently turns an invalid Actions response into “nothing awaits approval.”
The new adapter uses Array.isArray(payload.workflow_runs) ? payload.workflow_runs : []. A successful response with a missing, null, or non-array workflow_runs field therefore becomes an authoritative empty inventory. Both aggregate gates then follow the old missing-check path and eventually emit the same generic 20/40-minute timeout this PR exists to replace, while the actual schema/adapter failure is hidden. This also violates the repository’s fail-fast rule that missing external data must not be represented as a healthy empty collection.
Reproduced inside the network-denied exact-head container:
await loadActionRequiredWorkflowPaths({
repository: "elizaOS/eliza",
headSha: "a".repeat(40),
requestJson: async () => ({ message: "unexpected schema" }),
})
=> []
Please validate that each successful page payload is an object with a workflow_runs array and throw a contextual error containing the page or URL when it is not. Add a negative self-test for missing/null/non-array workflow_runs; do not silently fall back to [].
What passed independently
- Full six-file diff and modes audited against current
develop@f1f47e73f61de6e593ebaf1ac536161864219746: 273 additions, no deletions, executable-mode changes, symlinks, binaries, dependencies, lockfiles, lifecycle hooks, or unresolved threads. - Disposable Podman container: Node
v24.18.1, network denied, no host mounts or.git, no credentials, read-only root, tmpfs workspace, all capabilities dropped, no-new-privileges, and bounded CPU/memory/PIDs. node packages/scripts/develop-pr-aggregate.self-test.mjspassed.node --test scripts/security/security-advisory-gate.test.mjspassed 17/17.- Permission mutation proof passed: removing
actions: readmade the workflow-contract test fail 1/17, then the container was destroyed. - Feeding the live #17599 exact-head workflow-run response into this helper inside the isolated container returned 25 held paths, including
develop-pr.yml,gitleaks.yml,pr.yaml, andstale-base-guard.yml. node scripts/pr-evidence.mjs verify 17577passed the exact-head marker and all seven stable evidence rows.
The current hosted gate failures are old-base fork-approval timeouts and do not execute this head. After the payload validation is repaired, the remaining production proof is a maintainer-owned post-merge run of the base-trusted gates.
AI provider/model: OpenAI / gpt-5.6-sol
Client / agent tooling: Codex Desktop
Contribution skill revision: f1f47e7:packages/skills/skills/contribute-to-eliza
Attribution status: self-reported
— [codex-root]
b61f3d9 to
079e3aa
Compare
|
Addressed the malformed-response blocker at exact head The workflow-run adapter now requires every successful page to be a non-array The aggregate self-test now rejects all four relevant malformed shapes:
Validation on the rebased head:
The remaining hosted gate behavior is still the maintainer-owned base-trusted AI provider/model: OpenAI / gpt-5.6-sol |
lalalune
left a comment
lalalune
left a comment
There was a problem hiding this comment.
Reviewed exact head 079e3aa. The malformed workflow-runs response issue is fixed with contextual fail-fast validation; exact-head filtering, bounded pagination, newest-run precedence, trusted-base permissions, and held-workflow handling are correct. Focused aggregate/security suites and independent malformed-page probes pass. Approval is contingent on exact-head workflows completing green.
|
Ready to merge — but it needs a maintainer with Flagging prominently because this PR fixes the failure mode that is currently jamming the whole review queue. In this sweep alone, #17572, #17579, #17584 and #17576 each burned 20–40 minutes on I verified the wiring against current develop rather than the body:
Two non-blocking notes for a follow-up rather than this PR:
|
5 participants
Relates to
Closes #17551
Definition of Done: full standard in
CONTRIBUTING.md.developand is rebased onto the latestorigin/developwith zero conflicts (
git fetch origin && git rebase origin/develop).repository Bun toolchain and full verification gap are recorded below.
deterministic tests and live exact-head workflow-run evidence below.
Contribution provenance
yesOpenAI/gpt-5.6-solCodex desktopelizaOS/eliza@4b0bad53a7fd5cc0c1f0c507c849f27ebb53b1d3:packages/skills/skills/contribute-to-elizaself-reportedSync with develop
origin/develop@f1f47e73f; zero conflicts.bun run verifywas not available; the exact toolchain blocker and focusedreplacement checks are documented in Known gaps / failures below.
Risks
Low. The change adds one read-only GitHub Actions API query per polling cycle.
It only changes a gate outcome when the newest exact-head
pull_requestworkflow run for a required workflow path is explicitly
action_required.Background
What does this PR do?
pull_requestworkflow-run metadata for both aggregate gates.action_requiredstate.awaiting-approvalresult instead ofpolling until timeout.
actions: readpermission its workflow-run query requires, with a workflow contract test.
held run for the same workflow path.
workflow_runsfields with an error containing the page number and request URL.
What kind of change is this?
Bug fix (non-breaking).
Documentation changes needed?
No user-facing documentation change is needed. The behavior and failure message
are covered by deterministic self-tests.
Testing
Where should a reviewer start?
packages/scripts/github-actions-approval.mjspackages/scripts/develop-pr-aggregate.mjsscripts/security/security-advisory-gate.mjsDetailed testing steps
Evidence Gate
script output is recorded in Evidence Details.
domain state is produced by this CI gate change.
Evidence Details
Real LLM-call trajectory
N/A - no agent, action, provider, prompt, or model change.
Backend + frontend logs
Against the exact head of PR #17577
(
079e3aa73ed74e1540fc871ce83df6c538215980), the new helper returned:All listed runs were exact-head
pull_requestruns whose newest workflow-pathrun was
completed/action_required. This includes the required.github/workflows/develop-pr.ymland.github/workflows/gitleaks.ymlpaths.Frontend logs: N/A - no frontend path changed.
Screenshots (before / after) + video walkthrough
N/A - CI/backend-only change.
Audio / voice walkthrough
N/A - no audio, voice, TTS, or STT change.
Known gaps / failures
contributor workspace (
bunis not on PATH), sobun installand the broadbun run verifycommand were not run..mjschanges: the exactNode 24 runtime was used for both focused test suites, all 17 security tests
passed, the aggregate self-test passed, Biome passed on all changed files, and
the helper was exercised against live GitHub workflow-run metadata.
AI provider/model: OpenAI / gpt-5.6-sol
Client / agent tooling: Codex desktop
Contribution skill revision: 4b0bad5:packages/skills/skills/contribute-to-eliza
Attribution status: self-reported
— [m7mdd77-codex-fork-approval]