fix(pr-agent): fork gate for /commands, and AI_TIMEOUT under its own step cap - #90
fix(pr-agent): fork gate for /commands, and AI_TIMEOUT under its own step cap#90yakimoto wants to merge 4 commits into
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) |
|
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
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR materially changes a privileged, secret-bearing GitHub Actions workflow by adding fork gating, API-token usage, concurrency behavior, and timeout/retry handling. An unresolved security comment specifically questions the new raw token exposure pattern, so the changes warrant human review. Not approved because:
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 |
1 participant
User description
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 workflow that holds OPENAI_KEY and PR write, including a new fail-closed fork gate. Scope is CI-only with 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 API key on forks, or lie about timeouts.Concurrency now tags
prvsissueso a comment on Issue #N cannot cancel an in-flight review of PR #N (same GitHub number sequence).Forks on
issue_commentare gated in a new step that queries the pulls API (fork status is missing from that event). Only a literalfalseruns the agent; 404s, rate limits, and unknown answers skip. The agent step is positively gated on that output. This is cost/defence-in-depth — the job still does not check out fork code.Timeouts and verdicts:
CONFIG__AI_TIMEOUTdrops from 600s to 300s so it sits under the 6-minute step and fallbacks can run. Attempt-2 duration is computed only when a start stamp exists, so a skipped retry is no longer reported as a multi-year hang. A dedicatedskippedbranch records a fork-gate decline instead of a workflow fault.Reviewed by Cursor Bugbot for commit 8e6bda5. 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.CodeAnt-AI Description
Safely skip fork pull requests and classify reviewer runs correctly
What Changed
Impact
✅ Fewer secret-exposure risks from fork review commands✅ More reliable AI review timeouts and retries✅ Clearer workflow failure and skip messages💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
PR Type
Bug fix, Enhancement
Description
Added fork gate step for issue_comment events to check PR origin
Reduced CONFIG__AI_TIMEOUT from 600 to 300 seconds to fit within step budget
Fixed duration calculation to avoid false TIMED OUT classifications
Updated changelog with detailed defect explanations
Diagram Walkthrough
File Walkthrough
pr-agent.yml
Enhanced security controls and budget management.github/workflows/pr-agent.yml
CHANGELOG.md
Updated changelog with defect detailsCHANGELOG.md