feat: implement issue #500 — pr-review: dispatcher fails on review_requested (empty gh api URL) + agent comment runaway - #501
Conversation
…quested (empty gh api URL) + agent comment runaway
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe "Resolve PR URL" step in ChangesPR URL Resolution Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new script, resolve-pr-url.sh, to correctly resolve the PR HTML URL across different GitHub event triggers, along with a comprehensive bats test suite and a mock gh stub. The feedback suggests adding error handling to the gh api call in the resolver script to handle failures gracefully under set -e, and refactoring the tab-separated line parsing in the gh stub to use Bash's built-in read instead of multiple awk subshells.
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
|
@donpetry-bot please review — fixes the pr-review-mention dispatcher (#500). Event-aware resolve-pr-url.sh + tests. |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
…heckout) The first cut fixed #500 by extracting resolve-pr-url.sh and adding a `Checkout dispatcher tooling` step to fetch it. In this privileged, comment-triggered reusable that checkout tripped CodeQL (actions/untrusted-checkout/high + the TOCTOU variant) — a self-inflicted high-severity finding. Resolve the PR URL inline in the `Resolve PR URL` step instead: same event-aware logic (issue_comment / pull_request_review_comment / pull_request[review_requested]) with the empty-URL guard so we never call `gh api ""` (#500), but with no checkout, no `tooling_ref` input, and no extra script — so there's no untrusted-checkout to flag. Removes the now-unneeded resolve-pr-url.sh, its bats suite, and the pr-review-mention-tests.yml gate (the ~12-line resolver lives inline in the workflow now). The tooling_ref+checkout pattern in auto-rebase/feature-ideation reusables is unaffected — they don't run in the privileged issue_comment context that makes the checkout risky here. Refs #500. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@donpetry-bot please review — reworked to resolve the PR URL inline (removed the privileged checkout that introduced the CodeQL untrusted-checkout finding). Same #500 fix, no checkout. |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (no-changes)Agent reasoning |
Superseded by automated re-review at
|
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
@donpetry-bot review please — CodeQL now green after the inline rework; this is the fix for the pr-review dispatcher (#500). |
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 5044863883b998de7adf9e164fa54e8965263ff6
Review mode: triage-approved (single reviewer)
Summary
Confirmation re-review of the inline-resolver rework of the privileged, comment-triggered pr-review-mention-reusable.yml (#500). The prior single-reviewer pass on this exact SHA held approval for one reason only: the CodeQL Analyze (actions) run — which must confirm the earlier untrusted-checkout HIGH was cleared — was still IN_PROGRESS. That check is now COMPLETED / SUCCESS, so the sole hold is resolved. The change itself is a net security improvement: event payload values are read from env vars (EVENT_NAME, PR_HTML_URL, ISSUE_PR_API_URL) instead of inline ${{ }} shell interpolation, with an empty/null URL guard so gh api "" is never called.
Linked issue analysis
Closes #500. The case "$EVENT_NAME" block handles all three triggers — pull_request/pull_request_review_comment use the PR html_url directly; issue_comment guards against a missing issue.pull_request.url before the API round-trip — and a final empty/null guard prevents an empty gh api call. This directly fixes the "dispatcher fails on review_requested / empty gh api URL" failure. Bug 2 (agent comment runaway) is out of scope for this workflow file and tracked separately.
Findings
- Fix is correct and complete for #500; all event types handled with empty/null guards.
- Security improvement: privileged-workflow event values now flow through env vars, removing the inline-interpolation script-injection surface;
gh api "$ISSUE_PR_API_URL"is quoted and GitHub-generated. - The prior iteration's privileged checkout (CodeQL untrusted-checkout HIGH) was removed and the resolver inlined; the now-moot
resolve-pr-url.sh, its bats suite, and the test gate were deleted accordingly. - gemini-code-assist's two suggestions (add
gh apierror handling; refactor awk in the test stub) are on the deleted files; both review threads are RESOLVED + OUTDATED, and the error-handling suggestion is in fact now implemented inline. No open or CHANGES_REQUESTED threads remain. - coderabbitai APPROVED at this SHA.
CI status
- CodeQL
Analyze (actions): SUCCESS (was the sole prior hold — now green, confirms untrusted-checkout HIGH cleared); CodeQL: SUCCESS - coderabbitai: APPROVED; CodeRabbit status: SUCCESS
- SonarCloud: Quality Gate passed (0 new issues, 0 security hotspots)
- Lint, ShellCheck, Agent Security Scan, AgentShield, Secret scan (gitleaks): SUCCESS
- dev-lead / dispatch, pr-auto-review / check-and-dispatch: SUCCESS
- dependency audits, dev-lead / ci-relay, dependabot-automerge: SKIPPED
- Note: reviewDecision REVIEW_REQUIRED (org-leads team) — org-level human approval still gates merge; this agent verdict does not bypass it.
- GitHub Secret Protection MCP scan unavailable in this run; relied on the passing gitleaks CI check.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
|
…DR (#869) (#520) * docs(standards): formalize the agent canary-ring/soak/promotion model + ADR (#869) The rings model in ci-standards.md was documented aspirationally and as a cut-release.sh-only (manual) process. dev-lead v1.4.0 has now shipped end-to-end through the automated, health-gated pipeline, so promote the standard to match: - Ring table → the canonical host-relative model: next=host, ring0=other org-infra repo, ring1=named low-traffic consumers, stable=rest. Points at canary-rings.json as the membership SoT. - "Rollout status" → reflects the built automation: canary-rollout.sh + the 4h evaluate / dispatch-gated promote, the concrete soak gate (healthy >= ceil(baseline/7) AND failure-rate <= baseline, no floor), rollback mechanics, and the codified release-channel-tags ruleset (apply-rulesets.sh). - New ADR docs/initiatives/agent-canary-rings-adr.md — records the decision + rationale (immutable releases + moving channels, host-relative rings, health-gated promotion, circular-dependency fix). DRAFT until the host-repo implementation PRs land (.github-private #878 canary-rollout, #889 ruleset codify, #880 SHA-pin guardrail) — this standard references them. Closes #869. Refs #495, #499, #500, #501, #502, #868. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(bot): address bot feedback [skip ci-relay] * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com> Co-authored-by: Don Petry Bot <donpetry+bot@gmail.com>
2 participants
Closes #500
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit