feat: implement issue #1905 — [qa-lead reach 3/3] persona-runner ignores surface=pull_request — the router-served qa-lead advisory loses its test-surface, budget and already-advised gates - #1909
Conversation
…res surface=pull_request — the router-served qa-lead advisory loses its test-surface, budget and already-advised gates
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
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 |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![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 pre-gate mechanism for persona events to prevent the runner from posting on every trusted pull request. It adds signal gathering and decision-making logic in scripts/persona-pr-pregate.sh and scripts/qa-lead-pr-gate.sh, along with comprehensive unit tests in tests/test_persona_pr_pregate.bats. The feedback highlights a critical issue in scripts/qa-lead-pr-gate.sh where a standalone conditional expression under set -e can cause premature script termination. Additionally, it is recommended to replace generic non-zero exit code assertions in the test suite with specific expected exit codes to prevent masking unexpected test failures.
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-23T04:55:50Z. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 468fa3767f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| env: | ||
| # github.token reads the (public) source repo — the same read scope the | ||
| # agent step uses. The pre-gate only READS PR signals; it never posts. | ||
| GH_TOKEN: ${{ github.token }} |
There was a problem hiding this comment.
The new pre-gate authenticates every API request with github.token, but both this reusable job and the calling job in .github/workflows/persona-runner.yml grant only contents: read; explicitly specifying that permission leaves pull-requests and issues unavailable. Consequently, a surface=pull_request dispatch for this private repository fails on its first /pulls/{pr}/files request, becomes skip:signal-unavailable, and never runs qa-lead, so the new router surface cannot be verified or used here. Request and forward at least pull-requests: read and issues: read for the caller and reusable job.
Useful? React with 👍 / 👎.
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-23T05:02:35Z. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e6cffbf9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/qa-lead-pr-gate.sh">
<violation number="1" location="scripts/qa-lead-pr-gate.sh:108">
P2: Any PR commenter can suppress the advisory by quoting `` anywhere in a comment, because this treats a raw substring as proof that qa-lead already advised. Fetch comment authors and accept only a marker-first-line comment from the configured qa-lead posting account.</violation>
<violation number="2" location="scripts/qa-lead-pr-gate.sh:108">
P1: Serialize the local and router paths with the same per-PR concurrency group, or make the idempotency check and post atomic. Otherwise concurrent runs can both pass this marker check and post duplicate advisories.</violation>
</file>
<file name="tests/test_persona_pr_pregate.bats">
<violation number="1" location="tests/test_persona_pr_pregate.bats:59">
P3: The gh stub's fallback paths return empty-success: an unrecognized `--jq` filter falls through to `printf '[]\n'` (exit 0), and an unrecognized raw URL returns `[]` in the `''` branch. If a future change adds a new gather call with a new filter or URL, the harness will silently feed fabricated empty data with a success status, so suppressor tests can go green on signals that never existed — the same silent-degradation the pre-gate fails closed on. Make both fallbacks fail loudly (non-zero exit naming the unhandled filter/URL) so stub/implementation drift breaks the suite instead of passing it.</violation>
</file>
<file name="scripts/persona-pr-pregate.sh">
<violation number="1" location="scripts/persona-pr-pregate.sh:42">
P2: The generic already-advised gate can run after an invalid comment response because `jq '.[].body'` succeeds on `{}` and yields an empty stream. Validate that each paginated response is an array before scanning it, otherwise malformed API data can bypass idempotency and post a duplicate advisory.
(Based on your team's feedback about fail-closed API gates.) .</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| _qa_lead_pr_gate_fail_closed "$repo" "$pr" "existing-advisory scan unavailable" | ||
| return 1 | ||
| fi | ||
| if grep -qF "$QA_LEAD_ADVISORY_MARKER" <<< "$comment_bodies"; then |
There was a problem hiding this comment.
P1: Serialize the local and router paths with the same per-PR concurrency group, or make the idempotency check and post atomic. Otherwise concurrent runs can both pass this marker check and post duplicate advisories.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/qa-lead-pr-gate.sh, line 108:
<comment>Serialize the local and router paths with the same per-PR concurrency group, or make the idempotency check and post atomic. Otherwise concurrent runs can both pass this marker check and post duplicate advisories.</comment>
<file context>
@@ -0,0 +1,131 @@
+ _qa_lead_pr_gate_fail_closed "$repo" "$pr" "existing-advisory scan unavailable"
+ return 1
+ fi
+ if grep -qF "$QA_LEAD_ADVISORY_MARKER" <<< "$comment_bodies"; then
+ existing_advisory=1
+ else
</file context>
| _qa_lead_pr_gate_fail_closed "$repo" "$pr" "existing-advisory scan unavailable" | ||
| return 1 | ||
| fi | ||
| if grep -qF "$QA_LEAD_ADVISORY_MARKER" <<< "$comment_bodies"; then |
There was a problem hiding this comment.
P2: Any PR commenter can suppress the advisory by quoting <!-- persona:qa-lead --> anywhere in a comment, because this treats a raw substring as proof that qa-lead already advised. Fetch comment authors and accept only a marker-first-line comment from the configured qa-lead posting account.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/qa-lead-pr-gate.sh, line 108:
<comment>Any PR commenter can suppress the advisory by quoting `<!-- persona:qa-lead -->` anywhere in a comment, because this treats a raw substring as proof that qa-lead already advised. Fetch comment authors and accept only a marker-first-line comment from the configured qa-lead posting account.</comment>
<file context>
@@ -0,0 +1,131 @@
+ _qa_lead_pr_gate_fail_closed "$repo" "$pr" "existing-advisory scan unavailable"
+ return 1
+ fi
+ if grep -qF "$QA_LEAD_ADVISORY_MARKER" <<< "$comment_bodies"; then
+ existing_advisory=1
+ else
</file context>
| local persona="$1" repo="$2" item="$3" marker bodies | ||
| marker="$(pr_agent_marker "$persona")" | ||
| if ! bodies="$(gh api --paginate \ | ||
| "repos/${repo}/issues/${item}/comments" --jq '.[].body')"; then |
There was a problem hiding this comment.
P2: The generic already-advised gate can run after an invalid comment response because jq '.[].body' succeeds on {} and yields an empty stream. Validate that each paginated response is an array before scanning it, otherwise malformed API data can bypass idempotency and post a duplicate advisory.
(Based on your team's feedback about fail-closed API gates.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/persona-pr-pregate.sh, line 42:
<comment>The generic already-advised gate can run after an invalid comment response because `jq '.[].body'` succeeds on `{}` and yields an empty stream. Validate that each paginated response is an array before scanning it, otherwise malformed API data can bypass idempotency and post a duplicate advisory.
(Based on your team's feedback about fail-closed API gates.) .</comment>
<file context>
@@ -0,0 +1,83 @@
+ local persona="$1" repo="$2" item="$3" marker bodies
+ marker="$(pr_agent_marker "$persona")"
+ if ! bodies="$(gh api --paginate \
+ "repos/${repo}/issues/${item}/comments" --jq '.[].body')"; then
+ echo "::error::persona pull_request pre-gate: existing-advisory scan unavailable for ${repo}#${item} — failing closed (skip)" >&2
+ printf 'skip:signal-unavailable\n'
</file context>
| "repos/${repo}/issues/${item}/comments" --jq '.[].body')"; then | |
| "repos/${repo}/issues/${item}/comments" --jq 'if type == "array" then .[].body else error("expected comments array") end') |
| exit "${STUB_GATHER_RC:-0}" | ||
| ;; | ||
| esac | ||
| printf '[]\n' |
There was a problem hiding this comment.
P3: The gh stub's fallback paths return empty-success: an unrecognized --jq filter falls through to printf '[]\n' (exit 0), and an unrecognized raw URL returns [] in the '' branch. If a future change adds a new gather call with a new filter or URL, the harness will silently feed fabricated empty data with a success status, so suppressor tests can go green on signals that never existed — the same silent-degradation the pre-gate fails closed on. Make both fallbacks fail loudly (non-zero exit naming the unhandled filter/URL) so stub/implementation drift breaks the suite instead of passing it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_persona_pr_pregate.bats, line 59:
<comment>The gh stub's fallback paths return empty-success: an unrecognized `--jq` filter falls through to `printf '[]\n'` (exit 0), and an unrecognized raw URL returns `[]` in the `''` branch. If a future change adds a new gather call with a new filter or URL, the harness will silently feed fabricated empty data with a success status, so suppressor tests can go green on signals that never existed — the same silent-degradation the pre-gate fails closed on. Make both fallbacks fail loudly (non-zero exit naming the unhandled filter/URL) so stub/implementation drift breaks the suite instead of passing it.</comment>
<file context>
@@ -0,0 +1,221 @@
+ exit "${STUB_GATHER_RC:-0}"
+ ;;
+esac
+printf '[]\n'
+STUB
+ chmod +x "$STUB_BIN/gh"
</file context>
| printf '[]\n' | |
| echo "::error::gh stub: unhandled --jq filter '$jqf'" >&2; exit 42 |
Dev-Lead — review-changes (partial)A commit was pushed, but not every requested change was applied. Per requested item:
The unaddressed items above still need work. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6739306fe9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # github.token reads the (public) source repo — the same read scope the | ||
| # agent step uses. The pre-gate only READS PR signals; it never posts. | ||
| GH_TOKEN: ${{ github.token }} |
There was a problem hiding this comment.
When the router dispatch originates from another private org repository, this github.token belongs only to .github-private; adding pull-requests: read and issues: read expands its permissions within that repository but does not grant access to the SOURCE_REPO supplied in the payload. The first repos/${repo}/pulls/${pr}/files request therefore returns 403/404, and the new pre-gate converts that into run=false, leaving the workflow green without producing the fleet-wide advisory this route is intended to enable. Authenticate the pre-gate with a read-capable cross-repository credential, or gather and verify these signals in the source repository before dispatch.
Useful? React with 👍 / 👎.
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-23T05:18:09Z. |
Superseded by automated re-review at
|
Superseded by automated re-review at
|
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 6739306fe994dacf24db3e776431fb344afc73e3
Cascade: triage → audit (triage: haiku 4.5 [sonnet 5] → deep: opus 4.8 [sonnet 5] + duck: o4-mini → audit: fable 5)
Summary
The PR adds a fail-closed pull_request pre-gate to the persona runner so router-served qa-lead advisories apply the same suppressors as the local surface. The only security-relevant delta — pull-requests:read + issues:read on the secrets:inherit path — was verified minimal, read-only, and correctly mirrored at all three call sites; inputs flow via env into quoted shell (no expression injection) and the PAT-posting isolation is unchanged. The failing template-drift check is NOT in the branch ruleset's required set (verified via GET /rules/branches/main: SonarCloud, CodeQL, agent-shield/AgentShield, dependency-audit/Detect ecosystems, duplicate-decl-gate — all green), so all gates pass.
Findings
- info: Added pull-requests:read and issues:read are least-privilege and mirrored at caller (persona-runner.yml), reusable (persona-runner-reusable.yml), and local advise job (qa-lead-pr-advisory.yml). Top-level permissions:{} retained; PAT selection/posting logic untouched; pre-gate uses only github.token and never posts.
- info: All dispatch-derived values (persona, surface, source_repo, item_number, event_action) enter the pre-gate step via env: and are quoted in shell — no ${{ }} interpolation inside run: blocks. gh api paths are built from these env vars but repository_dispatch already requires repo write access (trusted router), and calls are read-only GETs.
- info: template-drift is FAILURE at head, but it is NOT a required context per the main-branch ruleset (verified via gh api repos/.../rules/branches/main) and AGENTS.md documents it as routinely failing without blocking merge. The failure is pre-existing drift on petry-projects/repo-template stubs untouched by this PR. All five ruleset-required checks are SUCCESS.
- minor: Confirmed from deep review: serialization of the local vs router-served advisory depends on the external ADR-0009 router emitting source_repo identical to github.repository and empty comment_url for pull_request dispatches, so both land in the same reusable concurrency group. If it diverges, worst case is a duplicate advisory comment (noise), not a security exposure. Author has flagged post-merge verification; the already-advised marker recheck limits blast radius.
- minor: Confirmed from deep review: for a private SOURCE_REPO the github.token-based signal reads 403 and the gate fails closed, so routed advisories silently skip. Safe direction (no wrong post, no data exposure) and consistent with the documented public-source-repo assumption, but a silent functionality gap worth a follow-up issue for observability.
Reviewed by the PR-review cascade (triage: haiku 4.5 [sonnet 5] → deep: opus 4.8 [sonnet 5] + duck: o4-mini → audit: fable 5). Reply if you need a human review.
|
pr-review approved on PARTIAL advisory evidence: 5/7 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596). |
Dismissing approval due to a PR issue comment lacking a verified disposition (#1813)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-23T06:11:55Z. |
|
dev-lead is withholding action on this item. It is labeled To re-enable automated pickup: remove the |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-25T04:29:46Z. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-25T05:26:26Z. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-25T06:56:31Z. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-27T00:37:42Z. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-27T03:26:10Z. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-09-29T19:53:28Z. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
2 participants
Problem
[qa-lead reach 3/3] persona-runner ignores surface=pull_request — the router-served qa-lead advisory loses its test-surface, budget and already-advised gates
From the issue: Epic #1643 (Phase 2), decision (b) in ADR-0009 (#1869): qa-lead's
pull_requestadvisory moves from the self-contained.github/workflows/qa-lead-pr-advisory.ymlto the sharedpersona-mentionrouter (petry-projects/.github#1165, PRpetry-projects/.github#1167). The router dispatchesrepository_dispatch: persona-mentionwithclient_payload[surface]=pull_requestinto.github/workflows/persona-runner.ymlhere.Risk
Medium — changes GitHub Actions workflow behavior, which is exercised only post-merge; verify via the affected workflow runs.
Test plan
Tests added/updated:
tests/test_persona_pr_pregate.bats. Verification:bash scripts/dev-lead-lint.sh(shellcheck --severity=warning) ran pre-commit; the bats suite runs in CI.Rollback
Revert this PR. No non-revertible side effects (no tags, migrations, or external state).
Monitoring
Watch the affected workflow run(s) in the Actions tab and this PR's Lint check for regressions.
Closes #1905