fix(pr-agent): fork gate for /commands, and AI_TIMEOUT under its own step cap - #90
Conversation
…step cap This repo merged the inline pr-agent lane before two defects in it were found. The 16 repos whose adoption PRs are still open were re-synced in place; this one already merged, so it needs its own PR. Source of truth: wave-foundation-public#73. 1. Fork status is now RESOLVED for slash commands, not assumed. The job-level `if:` refuses forks on the `pull_request` arm; it structurally cannot on `issue_comment`, because fork status is absent from that payload — measured, with a positive control: `issues/<n>.pull_request` carries exactly [diff_url, html_url, merged_at, patch_url, url], while `pulls/<n>.head.repo.fork` answers. A `fork gate` step asks the pulls endpoint and FAILS CLOSED: only a literal `false` proceeds; a 404, a revoked token, a rate limit and `.head.repo = null` (fork deleted after the PR opened) all skip. Scope, stated rather than inflated: this lane runs no `actions/checkout`, so fork code is never fetched or executed and no exfiltration path existed. What a /review on a fork PR reaches is the fork diff, sent to the LLM router on our key — cost surface, already narrowed by the author_association allowlist. The durable defect was the COMMENT claiming "Forks skipped (no secrets there)": true of one arm, false of the other, and exactly what would mislead whoever adds a checkout step later. 2. CONFIG__AI_TIMEOUT 600 -> 300, in both env blocks. A 600s AI budget inside a 360s step is unreachable: the runner killed the step first, so pr-agent never reached its own timeout, never fell back to CONFIG__FALLBACK_MODELS, and returned no error the retry could classify. 3. A latent classifier bug the gate exposed. `stamp attempt 2 end` runs under `if: always()`, so when attempt 2 never ran the arithmetic subtracted from ZERO and reported a 1787580408-second attempt as a confident TIMED OUT. Fixed at the arithmetic rather than by special-casing the caller; the verdict also gains an explicit `skipped` branch. The job id stays `pr_agent`, so the check-run context is unchanged and no branch protection rule needs touching. Refs wave-pen#418, wave-pen#417, wave-pen#388 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🤖 CodeAnt AI — Review Status
|
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_80699c96-3406-4f2f-bd26-637077914da4) |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
🧰 Additional context used📓 Path-based instructions (1)Conventional Commit titles; update `CHANGELOG.md` (`Unreleased`) for user-facing changes.📄 CodeRabbit inference engine (AGENTS.md) Files:
🪛 zizmor (1.29.0).github/workflows/pr-agent.yml[info] 65-65: workflow or action definition without a name (anonymous-definition): this job (anonymous-definition) 📝 SummarySummary by CodeRabbit
WalkthroughThe PR updates the agent workflow to block fork-triggered reviews, separate event concurrency, reduce AI timeouts, handle intentional skips, and avoid invalid retry-duration values. The changelog records these fixes. ChangesPR agent workflow safeguards
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This updates the PR-agent workflow to safely reject fork-triggered commands, prevent conflicting comment runs, bound AI execution time, and correctly report skipped retries. No current merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant issue_comment
participant PullRequestAPI
participant fork_gate
participant agent_step
participant verdict_step
issue_comment->>PullRequestAPI: Request pull-request metadata
PullRequestAPI-->>fork_gate: Return fork status
fork_gate->>agent_step: Run only when fork is false
fork_gate->>verdict_step: Report intentional skip when fork is true or unreadable
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
codeant-ai
Bot
added
the
size:L
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR changes a secret-bearing GitHub Actions lane that processes fork pull requests, including its fork authorization gate and conditions for running AI review work. Although the timeout and classifier fixes are focused and well-contained, the security-sensitive execution changes require human review. Not approved because:
No code changes detected at Review your spending limits in Billing settings. You can add or adjust custom eligibility rules. Learn more. |
PR Summary by QodoGate pr-agent /commands on forks and align AI timeout with step budget
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
Note Automatic reviews are paused because your team has used its included automatic processing for this billing period (headroom scales with your seat count). You can still comment "Gitar review" to run one anytime, and automatic reviews resume on their own by September 1. Add seats for more headroom. Code Review ✅ ApprovedAdds a fail-closed fork gate for issue comment commands and reduces AI timeout limits to fit within step budgets. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Code Review by Qodo
1. GH_TOKEN passed to shell
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo Fixer✅ Merged (0) · ☑ Fixed (0) Process
|
…ce of a true
Review of this wave found the fail-closed gate had a fail-OPEN consumer. Two
reviewers flagged it independently, on two different repos, and they were right.
if: steps.gate.outputs.fork != 'true' # grants when the output is EMPTY
The gate could only fail closed if it always wrote an output. It did, on every
path — so this did not fail open today, and the implicit success() on the
consumer covers a gate that errors outright. But the safety rested on an
argument rather than on the structure, and it is the very argument this change
exists to delete: absence must not read as permission.
Two independent changes, so neither carries the invariant alone:
- the gate now assigns a shell variable that STARTS at `true` and writes ONCE
at the end, so no future edit adding an early exit can emit nothing;
- the consumer requires `== 'false'`, an explicit affirmative, so an empty or
missing output skips the agent.
Also braces both sides of the A2 subtraction in the verdict step. The bare
`ATTEMPT2_START` was CORRECT — POSIX arithmetic expansion evaluates a bare name
as a variable, verified identical (180 == 180) — but a reviewer read it as a
literal token and filed it High. An expression that reads wrong on 27 repos gets
re-filed on 27 repos, so it is normalised rather than defended.
RECEIPTS. actionlint clean; zizmor clean; shellcheck clean. The gate was driven
through all six branches plus the reviewers' no-output scenario: only a literal
`false` reaches AGENT RUNS. The verdict was re-run across all six states and is
unchanged on the five that already worked.
LIVE: wave-av/api-spec merged the previous revision and its pull_request run
executed `fork gate (issue_comment only) -> success` in production, then ran the
agent — so the gate does not wrongly refuse a legitimate same-repo PR.
Upstream: wave-av/wave-foundation-public#73. Refs wave-pen#418, wave-pen#417.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_7cb72b88-4787-41fd-87e2-cc77adbc6a95) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideUpdates the pr-agent workflow with a fail-closed fork check for issue-comment commands, a 300-second AI timeout that fits the step budget, and corrected verdict logic for gated skips and absent retry attempts. Sequence diagram for the fail-closed fork gatesequenceDiagram
participant Comment as issue_comment
participant Workflow as pr_agent workflow
participant GitHub as GitHub pulls API
participant Agent as PR-Agent
Comment->>Workflow: Trigger slash command
Workflow->>GitHub: gh api repos/{repo}/pulls/{number}
GitHub-->>Workflow: .head.repo.fork
alt fork is false
Workflow->>Agent: Run with OPENAI_KEY
else fork is true or response unusable
Workflow-->>Comment: Skip and emit warning
end
Flow diagram for the bounded AI review and verdictflowchart LR
A["Fork gate"] -->|fork=false| B["PR-Agent step"]
A -->|fork=true or unknown| C["agent outcome: skipped"]
B --> D["AI timeout: 300s"]
D -->|within 360s step cap| E["fallback or retry classification"]
C --> F["verdict: notice and success"]
Flow diagram for corrected retry duration classificationflowchart TD
A["Read attempt stamps"] --> B{"Attempt 2 start exists and is > 0?"}
B -->|yes| C["Compute A2 from end - start"]
B -->|no| D["Set A2 = 0"]
C --> E["Clamp negative durations"]
D --> E
E --> F["Select longest attempt"]
F --> G["Classify timeout or failure"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…mber space Review found a SECOND concurrency collision, on a different axis from the one this template already documents (wave-pen#386). `issue_comment` fires for ISSUES as well as PRs, and GitHub draws both from ONE number sequence. So a comment on Issue #30 and a `/review` on PR #30 entered the same concurrency group. Concurrency is evaluated at WORKFLOW level, BEFORE the job-level `if:` runs — so the Issue comment cancelled the PR review already in flight, and was then skipped itself, having done nothing. That is the identical shape as the #386 defect the block above exists to fix, one axis over: a run that will not review taking the lane from the run that would have. #386 separated the two EVENTS; it did not separate the two number spaces inside one event. pull_request PR 433 -> pr-agent-pull_request-pr-433 issue_comment on PR 30 -> pr-agent-issue_comment-pr-30 issue_comment on ISSUE 30 -> pr-agent-issue_comment-issue-30 The last two used to be one group. actionlint and zizmor clean. Upstream: wave-av/wave-foundation-public#73. Refs wave-pen#418, wave-pen#417. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_88fb8546-d2bb-4002-be06-f2119d31a3bf) |
A reviewer flagged the missing entry on wave-modules#41. 25 of the 28 repos in this wave keep the same Keep-a-Changelog convention, so the entry lands in all of them rather than only the repo whose review happened to catch it — fixing the reported instance and leaving the class is the pattern this wave keeps undoing. The change IS user-visible, which is why it belongs here: a maintainer's `/review` on a fork PR is now declined with a warning instead of silently running, so contributors on forks see different behaviour. Refs wave-pen#418, wave-av/wave-foundation-public#73 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_5ab38202-81ac-4038-a097-02b716a51107) |
|
Working as intended, and the alternatives are worse — but the rule is pointing at something real, so here is the reasoning rather than a dismissal. The Three things bound it:
The alternatives, and why none is an improvement:
So the token is what makes the fail-closed posture honest rather than decorative. One thing I will grant: the read is the only reason this job needs an API call at all, and if GitHub ever surfaced |
CHANGELOG conflict resolved by keeping BOTH sides: this branch keeps its Unreleased entry for the pr-agent fork gate and AI_TIMEOUT fix, and main 2.1.3 released section is retained below it.
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_8ac4f461-dd6e-410a-a203-d90679eabc0e) |
CHANGELOG Unreleased conflict resolved by keeping BOTH sides: this branch pr-agent fork-gate entry, then the Changed/Fixed blocks main picked up from the live-gateway contract work.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_ab089e40-38fb-402a-82db-0495f92e7ec3) |
CHANGELOG Unreleased conflict resolved by keeping BOTH sides: this branch pr-agent fork-gate Fixed entry, then the Added block main picked up from the compose module work.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_974a8ecc-7b74-4d8f-b820-b6d6d8e3358e) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_639424d0-8a88-421c-880c-a21de86ac146) |
1 participant
User description
This repo merged the inline
pr-agentlane before two defects in it were found. The 16 repos whose adoption PRs are still open were re-synced in place; this one had already merged, so it needs its own PR.Source of truth: wave-av/wave-foundation-public#73. Findings tracked as wave-pen#418; the fan-out wave as wave-pen#417.
1. Forks were unchecked on the
issue_commentarm — and not by omissionThe job-level
if:refuses forks onpull_requestviahead.repo.fork == false. Theissue_commentarm carried no such check, while the header comment claimed "Forks skipped (no secrets there)" — true of one arm, false of the other.The reason it was missing is structural. Fork status is not in an
issue_commentpayload. Measured, with a positive control so the absence is a measurement and not a guess:Five URLs. No
head, norepo. There was never an expression to write — so the check moves to afork gatestep that asks the pulls endpoint, which does carry it.It fails closed. Only a literal
falseyieldsfork=false; everything else skips. Each branch was driven against a stubbedgh, not reasoned about:falsefork=false— proceedtruefork=true— skip, warnfork=true— skipfork=true— skipnullfork=true— skip"I could not tell" must not reach the same answer as "not a fork" on the arm that carries
OPENAI_KEY. The cost of erring this way is one skipped advisory review.Severity, stated precisely rather than inflated
This lane runs no
actions/checkout. Fork code is never fetched or executed, so there was no exfiltration path. What a/reviewon a fork PR actually reaches is the fork's diff, sent to the LLM router on our key — cost surface, already narrowed by theauthor_associationallowlist.So this is defence in depth. The durable risk was the comment, not the missing check: it told the next editor the guard was already there, and the day someone adds a checkout step to this lane, that belief is what would make it real.
2.
CONFIG__AI_TIMEOUTwas 600s inside a 360s step — in both env blocksUnreachable by construction. The runner killed the step first, so pr-agent never reached its own timeout, never fell back to
CONFIG__FALLBACK_MODELS, and returned no error the retry could classify. It also undercut the per-attempt classifier, which reasons aboutSTEP_BUDGET_S: "360"— a budget the AI layer inside the step did not respect.Now
300: 60s of headroom under the cap, and above both observed successful reviews (64s, 180s).3. A latent classifier bug the gate exposed — fixed at the root
stamp attempt 2 endcarriesif: always(), so it fires even when attempt 2 never ran, andEND - ${START:-0}then subtracted from zero. Running the unmodified classifier against that state:A 56-year attempt, reported as a confident diagnosis. Fixed in the arithmetic rather than by special-casing the caller, and the verdict gains an explicit
skippedbranch so a gated skip is not misread as "failed after 2 attempts".Receipts
actionlintclean ·zizmor --persona=regularclean · both newrun:blocksshellcheckclean.success,cancelled,never-ran, real-double-failure, and a genuine 350s timeout are all byte-identical between old and new.env:, never${{ }}in a script body.wave-av/api-specmerged this exact file and itsmainis byte-identical to the template.The job id stays
pr_agent, so the check-run context is unchanged and no branch protection rule needs touching.Refs wave-pen#418, wave-pen#417, wave-pen#388
Note
Medium Risk
Changes guard a workflow lane that holds OPENAI_KEY and pull-request write permissions; behavior is fail-closed but misconfiguration could skip legitimate reviews or still affect cost/abuse surface on fork diffs.
Overview
Hardens the inline
pr-agentGitHub Actions workflow so slash commands and concurrency behave correctly and timeouts are classified accurately.Concurrency now distinguishes PR vs plain issue in the group key, so an unrelated issue comment cannot cancel an in-flight review on a PR that shares the same numeric id.
Fork handling on
/commands: a newfork gatestep resolves fork status via the pulls API forissue_commentevents (where the jobif:cannot), defaults to refusing, and only runs PR-Agent whenfork == 'false'. Maintainer/reviewon fork PRs is skipped with a warning instead of usingOPENAI_KEYon the fork diff.CONFIG__AI_TIMEOUTis lowered from 600s to 300s in both agent steps so the AI budget stays inside the 6-minute step cap and fallback/retry logic can run.The verdict step treats fork-gated
skippedas a benign exit and fixes attempt-2 duration math when retry never started (avoiding bogus multi-billion-second “TIMED OUT” warnings). CHANGELOG documents these three fixes.Reviewed by Cursor Bugbot for commit 2f76b95. Bugbot is set up for automated code reviews on this repo. Configure here.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.PR Type
Bug fix
Description
Added fork gate step for issue_comment events to prevent execution on forks
Reduced CONFIG__AI_TIMEOUT from 600s to 300s to fit within step budget
Fixed timeout detection logic to handle failed attempts correctly
Updated changelog with detailed defect fixes
Diagram Walkthrough
File Walkthrough
pr-agent.yml
Enhanced fork protection and timeout management.github/workflows/pr-agent.yml
events
budget
didn't run
CHANGELOG.md
Updated changelog with defect fixesCHANGELOG.md
timeout budget, and timeout detection logic