fix(pr-agent): fork gate for /commands, and AI_TIMEOUT under its own step cap - #41
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_9ef394b0-18cf-45ed-a958-d72f3c7adbb3) |
|
Warning Review limit reachedNext included review available in 26 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 91 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Comment |
codeant-ai
Bot
added
the
size:L
|
Running ultrareview automatically — This PR implements a security-critical 'fork gate' to protect secrets within CI/CD infrastructure and refines sensitive timeout logic using shell-scripted arithmetic that is prone to subtle failures.. I'll post findings when complete. |
PR Summary by QodoFix pr-agent fork gating for /commands and align AI timeout with step budget
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — The PR materially changes a secret-bearing GitHub Actions review lane by adding fork/API gating, separating concurrency lanes, and altering timeout and retry classification. Although the gate fails closed and the changes are documented, the new execution gate and security/cost implications warrant human review. Not approved because:
Review your spending limits in Billing settings. You can add or adjust custom eligibility rules. Learn more. |
|
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 fork gate to block unsafe PR-agent runs on forked pull requests, aligns the AI timeout to fit within the step budget, and fixes a classifier arithmetic bug that caused false timeout reports. OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
I can't run this ultrareview because your workspace has reached its monthly review limit. cubic has reviewed 100,145 of the 100,000 allowed lines of code this month. Reviews resume on 4 September 2026 (in 12 days). Enable flex capacity to cover overages automatically and resume reviews now. Learn how flex capacity works. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
The pull request introduces significant changes to the PR-agent workflow that should be documented in the
|
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (1) 🔗 Fix PR: #42 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 1 fixed
|
…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_8b246f15-da38-47cd-aa1f-57aa717d5db2) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's GuideHardens the pr-agent workflow by adding a fail-closed API-based fork gate for slash-command reviews, reducing the AI timeout to fit the step cap, and fixing per-attempt duration and skipped-run classification without changing the pr_agent check context. Sequence diagram for the fail-closed pr-agent fork gatesequenceDiagram
participant GitHub as GitHub Actions
participant Gate as fork gate
participant API as GitHub pulls API
participant Agent as PR-Agent
GitHub->>Gate: Evaluate event and initialize fork=true
alt pull_request event
Gate->>Gate: Set fork=false from job-level guard
else issue_comment event
Gate->>API: gh api repos/REPO/pulls/PR_NUMBER
API-->>Gate: .head.repo.fork
alt response is false
Gate->>Gate: Set fork=false
else response is true or unreadable
Gate->>Gate: Keep fork=true and warn
end
end
Gate-->>Agent: fork output
alt fork == false
Agent->>Agent: Run with OPENAI_KEY
else fork == true
Agent-->>GitHub: Skip agent and classify as skipped
end
State diagram for pr-agent verdict outcomesstateDiagram-v2
[*] --> Gate
Gate --> Skipped: fork output is true
Gate --> Attempt1: fork output is false
Attempt1 --> Success: success
Attempt1 --> Attempt2: failure
Attempt2 --> Success: success
Attempt2 --> TimedOut: longest attempt reaches budget minus slack
Attempt2 --> Failed: both attempts fail below budget
Skipped --> [*]
Success --> [*]
TimedOut --> [*]
Failed --> [*]
Flow diagram for per-attempt timeout classificationflowchart LR
A["Stamp attempt 1 start"] --> B["PR-Agent attempt 1\nAI timeout 300s"]
B --> C["Stamp attempt 1 end"]
C --> D{"Attempt 1 failed?"}
D -- No --> E["Classify outcome"]
D -- Yes --> F["Backoff 45s"]
F --> G["Stamp attempt 2 start"]
G --> H["PR-Agent retry\nAI timeout 300s"]
H --> I["Stamp attempt 2 end"]
I --> E
E --> J["Compute A1 and A2"]
J --> K["Compare longest attempt\nwith STEP_BUDGET_S - 15"]
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_832244e1-5033-4430-86e5-85eac920df2f) |
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_7c19ab8f-863c-4c69-a0e0-edfbf729aa3f) |
Review disposition — every finding on this PR, actioned or refuted with evidenceThis branch has been updated. Reviews across this wave raised nine distinct findings; four were real and are fixed, three are working-as-intended, two are refuted with measurements. Nothing was silently skipped. Source of truth for the template: wave-av/wave-foundation-public#73. Tracked as wave-pen#418. Fixed — the reviewers were right
Refuted — with the measurement, not an opinion
GitHub is following an org rename (Codium-ai → qodo-ai → The-PR-Agent) transparently. The suggested remedy would make things worse:
Normalised anyway. An expression that reads wrong on 27 repos gets re-filed on 27 repos, so consistency is worth more than being right about it. Committable suggestions lack Working as intended
One defect no reviewer found, surfaced by fixing the first
A 56-year attempt, stated as a confident diagnosis. Latent since #72; the fork gate is simply the first path that reaches it. Fixed at the arithmetic, and the verdict gained an explicit Receipts
|
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
Touches a secret-bearing GitHub Actions lane (
OPENAI_KEY, PR write) and fork/concurrency gating. Fail-closed skips are the main behavioral change; there is still no checkout of fork code.Overview
Fixes three defects in the inline
pr-agentlane so slash-command reviews no longer share lanes with issues, spend the LLM key on forks, or mis-report hangs.Forks and concurrency.
issue_commentcannot seehead.repo.fork, so a new fail-closedfork gatestep queries the pulls API and only runs the agent on an explicitfalse. Concurrency groups now distinguish PR vs issue so a comment on Issue #N cannot cancel a review of PR #N.Timeouts and verdicts.
CONFIG__AI_TIMEOUTdrops from 600s to 300s so it sits under the 6-minute step (fallback models can actually fire). Verdict timing is per-attempt, not wall-clock across retries, and skipped/gated runs are classified as notices instead of fake TIMED OUT. Maintainer/reviewon a fork PR is now declined with a warning.Reviewed by Cursor Bugbot for commit 087ab9b. 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, Enhancement
Description
Added fork gate to handle issue_comment events properly
Adjusted CONFIG__AI_TIMEOUT from 600 to 300 seconds
Fixed attempt duration calculation to avoid false timeouts
Updated changelog with detailed fix documentation
Diagram Walkthrough
File Walkthrough
pr-agent.yml
Enhanced security and reliability in pr-agent workflow.github/workflows/pr-agent.yml
CHANGELOG.md
Updated changelog with detailed fix documentationCHANGELOG.md