Repository navigation
ci: least-privilege workflow tokens; pin AI thread ops to gh-thread helper - #314
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a strict ChangesReview-thread operations and workflow security
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeAgent
participant gh-thread
participant GitHubGraphQL
ClaudeAgent->>gh-thread: List, reply, or resolve review thread
gh-thread->>GitHubGraphQL: Send review-thread query or mutation
GitHubGraphQL-->>gh-thread: Return thread data or operation result
gh-thread-->>ClaudeAgent: Return command output
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Preview DeploymentPreview URL: https://5776f645.bestax.pages.dev |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 finding(s)
| Item | Severity | Area | Finding | Location |
|---|---|---|---|---|
| — | — | — | No blocking defects found. | — |
Overall: This is a tight, well-reasoned least-privilege hardening pass and it holds up under adversarial reading. I verified the gh-thread wrapper (fixed GraphQL surface, correct -F n int vs -f string typing, numeric-PR guard, owner/repo split), the origin/main staging (fetch-depth: 0 reliably creates refs/remotes/origin/main in the fix/verify/reply jobs — and for fork PRs that ref is the trusted base main, which is exactly the intended property), and the permission narrowing (no test-matrix job used the dropped pull-requests: write; publish and commit-screenshots re-declare precisely what they need; same-run download-artifact@v8 does not require actions: read). Every remaining raw gh api is in a trusted run: step, and all three session prompts were updated to gh-thread. The riskiest surface is the origin/main staging assumption — if fetch-depth: 0 ever regressed, every AI-loop job would fail closed at the stage step, which is the safe direction. The authors already flag the real residual gap (an injected agent still has node/pnpm run/sed locally), correctly scoping the PAT migration as separate follow-up.
🏄 Dude, this one is all about paddling the token power waaay back from the sketchy waves — untrusted thread text gets its own little
gh-threadkiddie pool instead of the whole open ocean ofgh api. Clean lines, no wipeouts, green light to send it. 🌊
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/gh-thread.sh (1)
1-68: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not submit
.github/**changes through the autonomous AI loop.The repository policy says the autonomous loop refuses PRs modifying
.github/**; this PR changes all three listed paths. Use the approved non-autonomous process or obtain an explicit policy exception before proceeding.
.github/scripts/gh-thread.sh#L1-L68: remove this path from the autonomous-loop PR..github/workflows/bestaxbot-reply.yml#L128-L140: remove this path from the autonomous-loop PR..github/workflows/claude-pr-loop.yml#L336-L347: remove this path from the autonomous-loop PR.As per coding guidelines, “The autonomous AI loop refuses pull requests that modify
.github/**.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/gh-thread.sh around lines 1 - 68, Remove the autonomous-loop changes from .github/scripts/gh-thread.sh (lines 1-68), .github/workflows/bestaxbot-reply.yml (lines 128-140), and .github/workflows/claude-pr-loop.yml (lines 336-347); these .github/** paths must not be included in the autonomous-loop PR. Use the approved non-autonomous process or obtain an explicit policy exception instead.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/gh-thread.sh:
- Around line 40-46: Update the GraphQL query and surrounding gh-thread.sh
pagination flow to paginate both the reviewThreads and nested comments
connections, adding cursors and pageInfo and repeatedly requesting pages until
each connection is exhausted. Ensure later review threads and replies are
included while preserving the existing node fields and output handling.
In @.github/workflows/bestaxbot-reply.yml:
- Around line 134-139: Bootstrap .github/scripts/gh-thread.sh on the trusted
base branch before workflows depend on it, then update the staging steps in
.github/workflows/bestaxbot-reply.yml lines 134-139,
.github/workflows/claude-pr-loop.yml lines 342-346, and
.github/workflows/claude-pr-loop.yml lines 623-627 to load the helper from
origin/main only after it is available there.
In @.github/workflows/ci.yml:
- Around line 9-14: Remove the workflow modifications at
.github/workflows/ci.yml lines 9-14 and .github/workflows/visual-regression.yml
lines 23-29, unless an explicit exception to the repository’s .github/**
modification policy has been obtained; no other changes are requested.
In @.github/workflows/claude-pr-loop.yml:
- Around line 391-395: The model value in claude_args is invalid because it
includes the stray “[1m]” suffix. Update the model setting to use the existing
claude-opus-4-8 identifier, leaving the surrounding workflow arguments
unchanged.
---
Outside diff comments:
In @.github/scripts/gh-thread.sh:
- Around line 1-68: Remove the autonomous-loop changes from
.github/scripts/gh-thread.sh (lines 1-68), .github/workflows/bestaxbot-reply.yml
(lines 128-140), and .github/workflows/claude-pr-loop.yml (lines 336-347); these
.github/** paths must not be included in the autonomous-loop PR. Use the
approved non-autonomous process or obtain an explicit policy exception instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f7d157fc-95f5-4f86-8f95-09c064796e18
📒 Files selected for processing (5)
.github/scripts/gh-thread.sh.github/workflows/bestaxbot-reply.yml.github/workflows/ci.yml.github/workflows/claude-pr-loop.yml.github/workflows/visual-regression.yml
| gh api graphql \ | ||
| -f query='query($o:String!,$r:String!,$n:Int!){ | ||
| repository(owner:$o,name:$r){pullRequest(number:$n){ | ||
| reviewThreads(first:100){nodes{ | ||
| id isResolved path line | ||
| comments(first:50){nodes{author{login} body url}}}}}}}' \ | ||
| -f o="${REPO%/*}" -f r="${REPO#*/}" -F n="$PR" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# The revised query should expose pageInfo/cursors for both paginated
# connections, rather than relying on only first:100 / first:50.
rg -n 'reviewThreads\(first:.*pageInfo|comments\(first:.*pageInfo' \
.github/scripts/gh-thread.shRepository: allxsmith/bestax
Length of output: 154
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,220p' .github/scripts/gh-thread.sh | cat -nRepository: allxsmith/bestax
Length of output: 3109
🏁 Script executed:
rg -n '\bgh-thread list\b|gh-thread\b' .githubRepository: allxsmith/bestax
Length of output: 5580
Paginate both connections. .github/scripts/gh-thread.sh:40-46
reviewThreads(first:100) and comments(first:50) truncate the output, so later threads or replies can be missed. Add cursors/pageInfo and loop until both connections are exhausted.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 41-45: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/scripts/gh-thread.sh around lines 40 - 46, Update the GraphQL query
and surrounding gh-thread.sh pagination flow to paginate both the reviewThreads
and nested comments connections, adding cursors and pageInfo and repeatedly
requesting pages until each connection is exhausted. Ensure later review threads
and replies are included while preserving the existing node fields and output
handling.
There was a problem hiding this comment.
Fixed in 70749eb — gh api graphql --paginate --slurp now follows the reviewThreads cursor (output is an array of pages, documented in the usage header), and comments is raised to the per-thread API maximum of 100. Smoke-tested against a live PR.
| # Read-only by default: the test jobs execute PR-authored code (pnpm install, | ||
| # tests, builds) — including AI-pushed claude/* branches — so the token they | ||
| # can reach must not be able to write. The publish job re-declares the write | ||
| # scopes semantic-release needs. | ||
| permissions: | ||
| contents: write | ||
| pull-requests: write | ||
| contents: read |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | 🏗️ Heavy lift
The PR violates the repository’s .github/** modification policy.
.github/workflows/ci.yml#L9-L14: obtain an explicit exception or remove this workflow change..github/workflows/visual-regression.yml#L23-L29: obtain an explicit exception or remove this workflow change.
As per coding guidelines, the autonomous AI loop refuses pull requests that modify .github/**.
📍 Affects 2 files
.github/workflows/ci.yml#L9-L14(this comment).github/workflows/visual-regression.yml#L23-L29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 9 - 14, Remove the workflow
modifications at .github/workflows/ci.yml lines 9-14 and
.github/workflows/visual-regression.yml lines 23-29, unless an explicit
exception to the repository’s .github/** modification policy has been obtained;
no other changes are requested.
Source: Coding guidelines
There was a problem hiding this comment.
Refuting: the guideline being cited ("the autonomous AI loop refuses PRs that touch .github/**") is a constraint on the AI loop's own PRs — by design the loop cannot modify its own gates. This is a session PR authored by the repo owner, which is exactly the channel .github/** changes are supposed to go through (see CLAUDE.md: workflow changes are explicitly "session/human PR" work). No exception needed.
CodeRabbit finding on #314: reviewThreads(first:100)/comments(first:50) truncated silently. `gh api graphql --paginate --slurp` now follows the thread cursor (output becomes an array of pages, documented in the usage header); comments raised to the per-thread API maximum of 100. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Preview DeploymentPreview URL: https://af0376f2.bestax.pages.dev |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
…elper Security hardening from the workflow prompt-injection review: - claude-pr-loop (fix/verify) and bestaxbot-reply no longer allowlist Bash(gh api:*). These sessions ingest review-thread text that anyone on a public repo can reply into, and the fix/reply agents hold the bestaxbot PAT — raw gh api let a prompt-injected session write to anything the token could reach. Thread ops now go through .github/scripts/gh-thread.sh (list/reply/resolve only), staged onto PATH from origin/main so a checked-out PR branch can neither miss it nor substitute its own copy. - ci.yml: workflow default drops contents:write + pull-requests:write to contents:read (the publish job already re-declares its own scopes); the jobs that execute PR-authored code no longer persist git credentials. - visual-regression.yml: same read-only default — the matrix job runs scaffolded-app installs with --ignore-workspace, outside pnpm-workspace.yaml's supply-chain hardening — with contents:write moved down to the workflow_dispatch-gated commit-screenshots job.
CodeRabbit finding on #314: reviewThreads(first:100)/comments(first:50) truncated silently. `gh api graphql --paginate --slurp` now follows the thread cursor (output becomes an array of pages, documented in the usage header); comments raised to the per-thread API maximum of 100. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/gh-thread.sh:
- Around line 19-21: Relocate the gh-thread helper out of .github/** to a
repository location permitted by the PR policy, then update every workflow or
script reference to the new path; alternatively, obtain and document an explicit
exception before merging. Preserve the helper’s behavior, including paginated
review-thread processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 049f8382-50a5-40fb-a611-14d734fe6868
📒 Files selected for processing (1)
.github/scripts/gh-thread.sh
70749eb to
99c3091
Compare
Preview DeploymentPreview URL: https://574dd638.bestax.pages.dev |
|
🎉 This PR is included in version 3.3.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
1 participant
Implements findings 1, 3, and 4 from the workflow prompt-injection security review (finding 2 — the deep reviewer's
gh api:*— and the AI_LOOP_PAT migration are deliberately out of scope here).1. AI agents lose raw
gh api(HIGH)The claude-pr-loop fix/verify sessions and bestaxbot-reply read entire review threads — including replies from arbitrary users on this public repo — while the fix/reply agents hold the bestaxbot PAT.
Bash(gh api:*)gave a prompt-injected session read-write access to everything the token can reach, plus a public exfiltration channel..github/scripts/gh-thread.sh:list <pr>/reply <thread-id> <body>/resolve <thread-id>— the loop's three GraphQL thread operations and nothing else.origin/main, never from the checked-out PR branch, so olderclaude/*branches still work and the branch's own copy is never the one that runs. It lives under.github/, so the protected-path gate and the agents' Edit/Write deny rules already cover it.Bash(gh api:*)→Bash(gh-thread:*); prompts updated to match (listing, thread replies, Copilot/verify resolves).3. ci.yml: read-only default token (MEDIUM)
build-and-testandreact-compatexecute PR-authored code — including AI-pushedclaude/*branches — with acontents: writetoken persisted in.git/config. Workflow default is nowcontents: read(thepublishjob re-declares its own scopes), and both test jobs setpersist-credentials: false.4. visual-regression.yml: same treatment (MEDIUM)
The matrix job runs scaffolded-app installs with
--ignore-workspace, i.e. outside pnpm-workspace.yaml's install-script blocking and release-age cooldown, while holding a write-capable persisted token. Workflow default is nowcontents: read+persist-credentials: false;contents: writemoved down to theworkflow_dispatch-gatedcommit-screenshotsjob (verified-commit uses the git data API, so that one scope suffices).Notes for review
gh apicalls in trustedrun:shell steps (gate, terminal-state, continue, handoff, halt) are unchanged — only the Claude session allowlists narrowed.ai-loopPRs don't need a rebase: the helper is staged fromorigin/main, not the PR branch.sed -i/pnpm runcan still do local mischief inside the job — the remaining hard boundary is the PAT migration (fine-grained, single-repo, noworkflowscope), tracked separately.Summary by CodeRabbit
gh-threadhelper CLI to list PR review threads, post replies, and resolve threads.gh-threadfor thread operations and adjusted tool permissions/prompts accordingly.