Repository navigation
ci: triage-as-bestaxbot, safe robobun-style repro drafting + read-only security scan - #361
Conversation
WalkthroughAdds read-only security scanning and author-only reproduction drafting. It tightens AI workflow gates, switches triage attribution to a PAT-backed bot, broadens deduplication marker recognition, and adds validation coverage. ChangesSecurity controls and reproduction workflow
Triage execution and attribution
Automation-authored deduplication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubEvent
participant ai-scan
participant GitHubAPI
participant Claude
GitHubEvent->>ai-scan: open issue or pull request
ai-scan->>GitHubAPI: check and charge daily scan budget
ai-scan->>Claude: run restricted read-only scan
Claude-->>ai-scan: return security verdict
ai-scan->>GitHubAPI: apply needs-security-review when verdict is not clean
sequenceDiagram
participant IssueAuthor
participant claude-repro
participant GitHubIssue
participant Claude
IssueAuthor->>claude-repro: apply claude-repro label
claude-repro->>GitHubIssue: verify authorization and labels
claude-repro->>Claude: request REPRO-DRAFT
Claude-->>claude-repro: return draft or infeasible result
claude-repro->>GitHubIssue: publish sanitized comment
claude-repro->>GitHubIssue: remove claude-repro label
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Preview DeploymentPreview URL: https://0e0bd899.bestax.pages.dev |
allxsmith
changed the title
![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
🧹 Nitpick comments (1)
scripts/auto-close-duplicates.mjs (1)
85-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the expanded automation-author contract.
Cover
bestaxbotas a regularUser, legacy[bot]authors, human users, marker selection, veto filtering, and issue skipping to prevent future changes from silently reintroducing incorrect auto-closes.🤖 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 `@scripts/auto-close-duplicates.mjs` around lines 85 - 109, Add regression tests for isAutomationAuthor and findMarkerComment covering bestaxbot as a regular User, legacy [bot] authors, and human users; verify the latest valid marker comment is selected, veto comments prevent closure, and issues with no eligible marker are skipped. Use representative comment and issue fixtures to preserve the expanded automation-author contract and auto-close behavior.
🤖 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/workflows/ai-triage.yml:
- Around line 332-350: Constrain the PAT-backed comment commands in the
workflow’s Claude tool allowlist by routing issue and pull-request comment
operations through a trusted wrapper. The wrapper must fix the repository and
target number, allow only body updates and the required edit-last behavior, and
reject delete-last, repo overrides, and alternate targets; update the existing
Bash permissions to use this wrapper instead of unrestricted gh issue/pr comment
commands.
In @.github/workflows/auto-close-duplicates.yml:
- Around line 8-11: Revert the comment-only change in the workflow file so no
modifications remain under .github; do not move or add documentation there as
part of this fix.
In @.github/workflows/claude-repro.yml:
- Around line 105-106: Add issues: read to the job-level permissions block for
the author job alongside contents: read, preserving the existing contents
permission so gh issue view can access issue metadata and comments.
In @.github/workflows/claude.yml:
- Around line 35-60: Keep the existing blocking behavior for flagged items and
update the documentation to match it: retain the needs-security-review checks in
.github/workflows/claude.yml lines 35-60 and
.github/workflows/bestaxbot-reply.yml lines 55-56, then revise
.github/workflows/ai-scan.yml lines 10-15, CLAUDE.md lines 107-113, and
docs/docs/guides/getting-started/ai-development.md lines 97-111 to state that
the label gates both `@claude` and `@bestaxbot` entry points and remove
contradictory advisory-only guidance.
---
Nitpick comments:
In `@scripts/auto-close-duplicates.mjs`:
- Around line 85-109: Add regression tests for isAutomationAuthor and
findMarkerComment covering bestaxbot as a regular User, legacy [bot] authors,
and human users; verify the latest valid marker comment is selected, veto
comments prevent closure, and issues with no eligible marker are skipped. Use
representative comment and issue fixtures to preserve the expanded
automation-author contract and auto-close behavior.
🪄 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: 3c258c1b-f56d-4775-8490-b4b0401bac2f
📒 Files selected for processing (13)
.claude/commands/triage-dedupe.md.claude/commands/triage-find-duplicate-prs.md.claude/commands/triage-find-issues.md.github/workflows/ai-scan.yml.github/workflows/ai-triage.yml.github/workflows/auto-close-duplicates.yml.github/workflows/bestaxbot-reply.yml.github/workflows/claude-implement.yml.github/workflows/claude-repro.yml.github/workflows/claude.ymlCLAUDE.mddocs/docs/guides/getting-started/ai-development.mdscripts/auto-close-duplicates.mjs
| # passed explicitly so the action skips its OIDC→app token | ||
| # exchange (#312). It is bestaxbot's PAT (AI_LOOP_PAT — the same | ||
| # machine account claude-implement.yml and the PR loop use), so | ||
| # triage comments are credited to bestaxbot instead of the | ||
| # anonymous github-actions[bot] that GITHUB_TOKEN produced | ||
| # before (and claude[bot] before #312). bestaxbot is a machine | ||
| # USER account, not a Bot-type app: anything probing for triage | ||
| # comments must match the marker + (bestaxbot OR a Bot-type | ||
| # author) — auto-close-duplicates.mjs and the marker checks in | ||
| # .claude/commands/triage-*.md do exactly that; never probe one | ||
| # specific login. Trade-off vs GITHUB_TOKEN: PAT-authored | ||
| # comments DO emit issue_comment events that can re-trigger | ||
| # workflows — every comment-triggered workflow already gates | ||
| # bestaxbot out (bestaxbot-reply.yml excludes it as sender; | ||
| # claude.yml requires a literal "@claude", which the HARD RULES | ||
| # below forbid writing), and the tool allowlist below confines | ||
| # the PAT to GET-only reads plus the two comment commands. | ||
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} | ||
| github_token: ${{ secrets.GITHUB_TOKEN }} | ||
| github_token: ${{ secrets.AI_LOOP_PAT }} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '326,370p' .github/workflows/ai-triage.yml
gh issue comment --help | rg -- '--(edit-last|delete-last)|--repo'
gh pr comment --help | rg -- '--(edit-last|delete-last)|--repo'Repository: allxsmith/bestax
Length of output: 3975
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the command allowlist, prompt boundaries, and any wrapper usage around gh comment commands.
rg -n "allowedTools|disallowedTools|gh pr comment|gh issue comment|comment:|REPO|NUMBER|delete-last|edit-last|--repo|-R" .github .claude 2>/dev/null || true
# Look for files that may define/parse command arguments for gh comment commands.
git ls-files | rg '(^|/)(claude|ai-triage|triage|comment|wrap|bash)' | sed -n '1,200p'Repository: allxsmith/bestax
Length of output: 29633
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Demonstrate that a directly allowed Bash command can invoke the same gh command
# with --delete-last and target another repo, without a wrapper or sentinel.
python3 - <<'PY'
from pathlib import Path
for path in [
".github/workflows/ai-triage.yml",
".claude/commands/triage-find-issues.md",
".claude/commands/triage-find-duplicate-prs.md",
".claude/commands/triage-dedupe.md",
]:
text = Path(path).read_text()
print(f'--- {path} ---')
for needle in ('allowed-tools', '--edit-last', '--delete-last', 'gh issue comment', 'gh pr comment', '--repo', 'REPO', 'NUMBER'):
if needle in text:
print(f'{needle}: present')
print("--- command-wildcard parser behavior ---")
allowed = ["Bash(gh issue comment:*)", "Bash(gh pr comment:*)", "Bash(gh issue view:*)"]
samples = [
"gh issue comment 123 --repo allxsmith/bad --delete-last --edit-last --yes",
"gh issue comment 123 --repo allxsmith/bad --edit-last --body x",
"gh issue comment 123",
"gh pr comment 123 --repo allxsmith/bad --delete-last --edit-last --yes",
"gh issue view 123 --raw-field name",
]
for cmd in samples:
print(cmd, "=>", any(cmd.startswith(prefix.split("(",1)[1][:-1]) for prefix in allowed))
PY
# Read the small triage command prompt files that define allowed command usage.
for f in .claude/commands/triage-dedupe.md .claude/commands/triage-find-duplicate-prs.md .claude/commands/triage-find-issues.md; do
echo "===== $f ====="
sed -n '1,90p' "$f"
doneRepository: allxsmith/bestax
Length of output: 12990
Constrain comment commands before exposing the PAT.
The workflow runs with github_token: ${{ secrets.AI_LOOP_PAT }} and allows Bash(gh issue comment:*) and Bash(gh pr comment:*), so a direct gh ... --edit-last --delete-last --yes command can delete bestaxbot comments without a wrapper guard. Put comment updates through a trusted command that fixes [REPO]/[NUMBER], allows only --body and required --edit-last, and rejects --delete-last, --repo, and alternate targets.
🤖 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/ai-triage.yml around lines 332 - 350, Constrain the
PAT-backed comment commands in the workflow’s Claude tool allowlist by routing
issue and pull-request comment operations through a trusted wrapper. The wrapper
must fix the repository and target number, allow only body updates and the
required edit-last behavior, and reject delete-last, repo overrides, and
alternate targets; update the existing Bash permissions to use this wrapper
instead of unrestricted gh issue/pr comment commands.
| permissions: | ||
| contents: read # checkout the base default branch so Claude can read the API |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate file overview =="
if [ -f .github/workflows/claude-repro.yml ]; then
wc -l .github/workflows/claude-repro.yml
sed -n '90,155p' .github/workflows/claude-repro.yml
else
echo "missing .github/workflows/claude-repro.yml"
fi
echo "== github issue/pr gh calls in workflows =="
rg -n "gh (issue|pr) view|permissions:" .github/workflows -g '*.yml' -g '*.yaml' || trueRepository: allxsmith/bestax
Length of output: 10316
🌐 Web query:
GitHub Actions GITHUB_TOKEN default permissions jobs permissions none gh API issues read
💡 Result:
When configuring the GITHUB_TOKEN in GitHub Actions, you can manage permissions at either the workflow level or the job level using the permissions key [1][2]. Key behaviors include: 1. Default Permissions: By default, newly created repositories in personal accounts or organizations are configured to grant the GITHUB_TOKEN read-only access to contents and packages [3][4]. You can change this default setting in the repository or organization's Actions settings to be more permissive if necessary [3]. 2. Using "none": When you use the permissions key, all permissions that are not explicitly specified are set to none [2]. For example, if you define permissions for a specific scope (e.g., contents: read) and omit others, those omitted scopes will have no access [5][2]. The metadata scope is an exception and will always retain read access [6]. 3. Handling "none" with API Issues: If you set permissions to none (or do not include a specific scope), any attempt to interact with the corresponding GitHub API endpoint (such as issues, pull requests, etc.) using the GITHUB_TOKEN will result in a 403 Forbidden error because the token lacks the necessary authorization [5][2]. 4. Implementation: To configure this, you can set permissions as follows in your YAML: permissions: issues: read # All other permissions default to 'none' To resolve issues where the GITHUB_TOKEN is being denied access, you must explicitly grant the required permission (e.g., issues: write or issues: read) within the job or workflow definition [1][2]. If your workflow requires access to resources outside the scope of the GITHUB_TOKEN, you should use a GitHub App installation access token or a Personal Access Token (PAT) stored as a secret instead [1][7].
Citations:
- 1: https://docs.github.com/en/actions/tutorials/authenticate-with-github_token
- 2: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
- 3: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository
- 4: https://github.blog/changelog/2023-02-02-github-actions-updating-the-default-github_token-permissions-to-read-only/
- 5: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/
- 6: https://docs.github.com/en/enterprise-server@3.13/actions/security-guides/automatic-token-authentication
- 7: https://docs.github.com/actions/reference/authentication-in-a-workflow
🌐 Web query:
GitHub Actions repository settings GITHUB_TOKEN public repository contents read checkout contents permission contents read action-gh-issues read
💡 Result:
In GitHub Actions, the GITHUB_TOKEN is a unique secret created for each workflow job to authenticate with GitHub on behalf of the workflow [1]. Its permissions are critical for security and functionality [2][3]. Repository Settings and Defaults The default permissions for the GITHUB_TOKEN depend on your repository's configuration [4][5]. In many repositories, especially those in personal accounts, the default is set to restricted permissions, providing only read access to repository contents and packages [4][5]. You can manage these settings under the repository's Settings > Actions > General page, where you can choose between a permissive setting (read and write access for all scopes) or a restricted one [4][5]. Required Permissions for Actions Certain actions require explicit permissions to function correctly: 1. actions/checkout: This action requires at least read access to the contents scope to check out your repository code [6][7][8]. If you use the permissions key to modify token access, failing to include contents: read will cause the checkout action to fail [7]. 2. Issue and Pull Request operations: If your workflow uses GitHub API toolsets or tools that interact with issues or pull requests, you must explicitly grant read access to those scopes in your workflow configuration [9]. For example, tools or commands (like those in GitHub CLI) that list or search issues often require explicit issues: read and pull-requests: read permissions [9][10]. Best Practices To ensure security and reliability, follow these guidelines: - Apply the Principle of Least Privilege: Use the permissions key in your workflow file to grant the GITHUB_TOKEN only the specific access required for the job [2][3][11]. - Declare Permissions Explicitly: Even if defaults exist, explicitly defining permissions in your workflow (at the workflow or job level) prevents issues caused by changes in repository settings [4][7]. Example Configuration: permissions: contents: read issues: read pull-requests: read
Citations:
- 1: https://docs.github.com/en/actions/concepts/security/github_token
- 2: https://docs.github.com/en/actions/tutorials/authenticate-with-github_token
- 3: https://docs.github.com/actions/reference/authentication-in-a-workflow
- 4: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository
- 5: https://github.com/github/docs/blob/main/content/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository.md
- 6: https://github.com/actions/checkout?tab=readme-ov-file
- 7: https://mickeygousset.com/posts/github-actions-checkout-fails-with-two-possible-error-messages/
- 8: https://github.com/actions/checkout/blob/main/README.md
- 9: Add missing GitHub permissions to workflows using GitHub API toolsets github/gh-aw#10020
- 10:
gh pr listdepends on the read permission for Issues cli/cli#5986 - 11: https://github.com/github/docs/blob/main/content/actions/tutorials/authenticate-with-github_token.md
Grant issues: read for the issue fetch in the author job.
Declaring permissions: contents: read at the job level scopes down the token; omitted scopes default to none, so gh issue view ... --json title,body,comments lacks the required issues API access and can fail before Claude runs. Add issues: read to the job-level permissions.
🤖 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/claude-repro.yml around lines 105 - 106, Add issues: read
to the job-level permissions block for the author job alongside contents: read,
preserving the existing contents permission so gh issue view can access issue
metadata and comments.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| claude_args: | | ||
| --max-turns 30 | ||
| --model claude-sonnet-5 | ||
| --allowedTools "Read,Glob,Grep,Write,Edit" |
There was a problem hiding this comment.
Invariant I1 is weaker than claimed — the Read tool can reach secrets and publish posts them publicly — 🟠 Major · Security
What: I1 (lines 13–17) claims that granting Claude only Read,Glob,Grep,Write,Edit (no Bash/Task/network) means "an injected session cannot read process.env." But removing Bash only closes the shell path. The Read tool is not confined to the workspace, so a prompt-injected drafter can Read /proc/self/environ (the Claude CLI's own environment, which holds CLAUDE_CODE_OAUTH_TOKEN and the job GITHUB_TOKEN) or the on-disk OAuth credential store (~/.claude/.credentials.json), embed the value in .repro/draft.test.tsx, and the publish job posts that file to a public comment. The sanitizer defangs @mentions/markers but does not redact secret-shaped strings, so the token leaks in cleartext.
Why it matters: This is exactly the leak I1 is meant to prevent, within the PR's own threat model (I1/I2 exist because injection is assumed possible). A leaked CLAUDE_CODE_OAUTH_TOKEN lets an attacker spend the maintainer's Claude quota / impersonate the session; secret-masking scrubs logs, not comment bodies (the PR body acknowledges this for the token case).
Fix: Either prove Read is confined so it cannot reach /proc or ~/.claude (and document that), or add a value-based redaction step in the author job (which is the only job that holds the secrets) before the draft is exported to the job output — e.g.:
- name: Redact any secret values from the draft
if: steps.claude.outcome == 'success'
env:
OAUTH: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
GHT: ${{ secrets.GITHUB_TOKEN }}
run: |
set -euo pipefail
[ -s .repro/draft.test.tsx ] || exit 0
python3 - "$OAUTH" "$GHT" <<'PY'
import sys, pathlib
p = pathlib.Path(".repro/draft.test.tsx")
t = p.read_text()
for s in sys.argv[1:]:
if s:
t = t.replace(s, "[REDACTED]")
p.write_text(t)
PYWhy the sibling ai-scan job is not exposed the same way
ai-scan.yml grants only Bash(gh …) (a strict whitelist) and its session has no public output channel (show_full_output off, no write tools, egress blocked), so even if it read a secret it could not exfiltrate it. claude-repro's author job is different precisely because it pairs a broad Read/Write grant with a downstream job that publishes the written file.
| `AI_SCAN_DAILY_LIMIT`). That flag is advisory — it pauses `claude-repro`/`claude-fix` but does | ||
| **not** block `@claude`/`@bestaxbot`, so never `@claude` a flagged item to investigate it. |
There was a problem hiding this comment.
Docs contradict the code: needs-security-review does block @claude/@bestaxbot — 🟡 Minor · Correctness
What: This line (and the identical claim in docs/docs/guides/getting-started/ai-development.md:108) states the flag "does not block @claude/@bestaxbot." But commit 3 added !contains(github.event.issue.labels.*.name, 'needs-security-review') (and the pull_request variant) to the job if: of both claude.yml and bestaxbot-reply.yml. So a flagged item makes both entry points a silent no-op — they are blocked.
Why it matters: CLAUDE.md is the canonical instruction file read by CodeRabbit and the @claude action, and this describes a security control's behavior. The follow-on guidance ("so never @claude a flagged item to investigate it") only makes sense if the mention still fired — a maintainer will expect a response and get silence, or trust a false model of the control.
Fix: Blocking is the intended, safer behavior (per commit 3's rationale), so correct the docs to match the code:
| `AI_SCAN_DAILY_LIMIT`). That flag is advisory — it pauses `claude-repro`/`claude-fix` but does | |
| **not** block `@claude`/`@bestaxbot`, so never `@claude` a flagged item to investigate it. | |
| `AI_SCAN_DAILY_LIMIT`). That flag pauses `claude-repro`/`claude-fix` and, since commit 3, | |
| also gates `@claude`/`@bestaxbot` — a flagged item makes those entry points a silent no-op | |
| until a maintainer removes the label, so investigate a flagged item by hand rather than mentioning a bot. |
Apply the same correction to ai-development.md:108.
There was a problem hiding this comment.
Deep review — 2 blocking · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟠 Major | Security | I1 is overstated: the Read tool (not confined to the workspace) can reach /proc/self/environ or ~/.claude/.credentials.json, so an injected drafter can write the OAuth token into the draft, which publish posts to a public comment un-redacted |
.github/workflows/claude-repro.yml:161 |
| 2 | 🟡 Minor | Correctness | Docs claim needs-security-review does not block @claude/@bestaxbot, but claude.yml and bestaxbot-reply.yml both gate their job if: on the label — it does block them |
CLAUDE.md:113, docs/.../ai-development.md:108 |
| 3 | 🔵 Advisory | Robustness | The publish sanitizer defangs @mentions/markers but not triple-backticks, so a drafted test containing a fence can break out of the tsx code block into raw markdown (safe: posted as github-actions[bot], cannot re-trigger; markers already globally defanged — cosmetic only) |
.github/workflows/claude-repro.yml:290 |
| 4 | 🔵 Advisory | Coverage | PR body says the sanitizer, the fail-closed verdict parser, and isAutomationAuthor are "unit-tested locally," but no test is committed for scripts/auto-close-duplicates.mjs or the shell parsers — load-bearing security logic has no in-repo regression guard |
scripts/auto-close-duplicates.mjs:95 |
Overall: The architecture is careful and the fail-closed labeler, budget fail-open, fork-exclusion, live-role re-checks, and PAT/token compartmentalization are all sound — I traced the ai-scan verdict parser by hand across clean / flagged / is_error+clean / missing-record / trailing-clean / empty / missing-file inputs and it fails closed in every case. The riskiest part is finding #1: I1's "no Bash means can't read env" reasoning does not hold because Read remains and the author job is uniquely paired with a public-publish downstream job, so the OAuth-token exfil path the invariant advertises as closed is plausibly open. The human should focus there first — either confirm Read is confined on the pinned claude-code-action, or add value-based secret redaction in the author job. Finding #2 is a doc/behavior mismatch in the canonical instruction file and is a quick fix.
Residual risk — for the failure class this PR targets (untrusted issue/PR text steering write-capable automation):
- Token exfil via a non-Bash tool — NOT refuted; this is finding #1.
Readreaching/proc/creds, thenWrite, then publicpublishis a concrete channel the "remove Bash" mitigation does not cover. - Re-trigger via smuggled
@claude/markers — refuted: repro drafts post asgithub-actions[bot](Bot sender, soclaude.yml'ssender.type == 'User'gate rejects it) and markers are deterministically defanged; the bestaxbot PAT path is fenced by the newsender.login != 'bestaxbot'exclusions plus the GET-only tool allowlist. - Cross-issue indirect injection in repro — refuted: the
authorjob has nogh/Bashtool and the issue is fetched deterministically by pinned number, so other-issue payloads are inert text.
🏄 Solid swell overall, brah — the compartments are watertight and the fail-closed sets hold their line. But there is one rogue current: I1 says "no shell, no env," yet the Read tool paddles right around it into
/procand out through the public comment. Patch that leak and this one is ready to drop in.
The ai-triage Claude step now uses bestaxbot's PAT (AI_LOOP_PAT, the same machine account claude-implement.yml uses) as its github_token, so triage comments are credited to bestaxbot rather than the anonymous github-actions[bot]. Explicit github_token still short-circuits the action's OIDC exchange (#312). bestaxbot is a machine USER account, not a Bot-type app, so every probe for triage comments now matches marker + (bestaxbot OR a Bot-type author) instead of Bot-type alone: - auto-close-duplicates.mjs gains isAutomationAuthor (isBot OR the bestaxbot login) for the marker finder, the human-objection veto, and the automation-issue skip — the last of which had silently never matched bestaxbot despite its comment claiming it did. - The three .claude/commands/triage-*.md marker checks and the docs guide are updated to name the new author and the historical ones. Trade-off vs GITHUB_TOKEN: PAT-authored comments emit issue_comment events. All comment-triggered workflows already gate bestaxbot out (bestaxbot-reply excludes it as sender; claude.yml needs a literal @claude, which the triage HARD RULES forbid), and the triage tool allowlist confines the PAT to GET-only reads plus the two comment commands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013xQ1BdCcYmxB13CWnNV5Nx
Two robobun-style automations, hardened to run safely on GitHub runners. claude-repro.yml (author-only, PAT-free): a triage+ maintainer labels an issue `claude-repro`; Claude DRAFTS a minimal Jest reproduction test and github-actions[bot] posts it for a human to run. Nothing here executes the draft. Two invariants: - The model-auth token never shares a job with code execution. The `author` job holds CLAUDE_CODE_OAUTH_TOKEN but grants Claude no Bash/gh/network/Task tool, so an injected session cannot read env, fetch, or run anything. - No attacker/Claude free text reaches a re-trigger-capable comment. The `publish` job defangs machine markers and @mentions in the draft, then posts via GITHUB_TOKEN (whose comments never re-trigger workflows). No PAT anywhere. The triggering issue is fetched deterministically by its number and Claude has no gh tool, so it cannot be steered into a second issue where a payload is staged. harden-runner egress-block, timeouts, a REPRO-DRAFT sentinel watchdog, and an always() cleanup that de-wedges the label round it out. ai-scan.yml (separate workflow, per-item concurrency so bursts cannot drop a scan): a read-only Claude session assesses new issues/PRs for malicious code, prompt injection, and social engineering. It holds no PAT and no write tools, show_full_output is off (its reasoning is the detection logic), and a deterministic step applies `needs-security-review` fail-CLOSED (a crashed or inconclusive scan flags rather than passes). Own kill switches (AI_SCAN_MODE, AI_SCAN_DAILY_LIMIT) and a fail-open daily budget marker on #290. Co-Authored-By: Claude <noreply@anthropic.com>
Repo-wide hardening surfaced by the security red-team of the repro/scan design. - claude.yml: fence the @claude trigger with a real-User + not-bestaxbot sender guard (contains() matches the RAW body, so a bot/PAT-authored comment carrying the literal "@claude" could otherwise re-trigger this write-capable session), and refuse to run on an issue/PR flagged needs-security-review. This also retroactively protects the shipped triage-as-bestaxbot path. - bestaxbot-reply.yml and claude-implement.yml: refuse needs-security-review items, so the flag actually pauses the automation paths that consume it. - ai-triage.yml: add harden-runner to the PAT-holding triage job (audit mode first — a live workflow whose action egress is undocumented; flip to block after one run confirms the allowlist), and rewrite the kill-switch header to enumerate per-surface behavior including the new AI_SCAN_MODE/AI_SCAN_DAILY_LIMIT. Co-Authored-By: Claude <noreply@anthropic.com>
Add `claude-repro` and `needs-security-review` to the label table, and new "Reproduction (author-only)" and "Security scan" sections covering how each runs, the no-credentials-near-untrusted-code posture, and — importantly — that the security flag is advisory: it pauses claude-repro/claude-fix but does NOT block @claude/@bestaxbot, so a flagged item must be inspected by hand, not investigated by mentioning a bot. Mirror the summary into the root CLAUDE.md AI loop section. Co-Authored-By: Claude <noreply@anthropic.com>
Deep-review findings on #361 — the two blocking ones plus the sanitizer advisory. **I1 was overstated, and the gap was real.** It claimed the `author` job's lack of a Bash tool meant an injected session "cannot read process.env". Removing Bash does stop shelling out, but `Read` is not confined to the workspace: /proc/self/environ and ~/.claude/.credentials.json stay readable, `Write` puts whatever is found into the draft, and `publish` posts that draft to a public comment. The one job holding CLAUDE_CODE_OAUTH_TOKEN is precisely the one feeding a public-publish job, so the channel the invariant advertised as closed was open. `Collect draft` now refuses to emit a draft containing a live credential value (compared against the tokens the job actually holds) or a credential-shaped string (for a re-encoded token, or a secret this job cannot compare against). Refuse rather than redact: a credential in the draft means the session went somewhere it had no business going, and publishing a scrubbed copy would destroy the only signal that it happened. The job fails, `cleanup` still removes the label, and the reason stays in the private run log. I1 now describes what removing Bash does and does not buy, and names this check as load-bearing rather than belt-and-braces. **The security flag blocks more than the docs said.** Both CLAUDE.md and the AI-development guide stated `needs-security-review` "does not block `@claude`/`@bestaxbot`" — but claude.yml and bestaxbot-reply.yml both gate their job `if:` on that label, so it does. Wrong in the safer direction, but it would send someone hunting a broken workflow when the label is the answer. Corrected in both, and in the label table, keeping the accurate half: a *clean verdict* is advisory (it only covers the text as scanned at open time) while the *flag* is a gate. **Fence delimiters are now defanged** in the publish sanitizer. A drafted line of backticks closed the ```tsx block early and rendered the rest as markdown. Cosmetic — it posts as github-actions[bot] and the machine markers were already neutralized — but a broken-out draft is what a reviewer skims past. Verified: all three workflows parse; the sanitizer neutralizes ``` and ~~~ as well as mentions; and the refusal logic allows a clean test while rejecting an exact OAuth token, an exact job token, a foreign sk-ant key, a github_pat_, and a PRIVATE KEY block. Claude-Session: https://claude.ai/code/session_01TGA6sFTUGsJ6oXhfpjKEnh
065ea43 to
27fbb2d
Compare
Rebased and review findings addressedWas 70 commits behind (branch point 2026-07-24). Rebased onto main — no conflicts. 🟠 Blocking #1 — I1's token-exfil path. Fixed, and the finding was right.I1 claimed the
Refuse rather than redact, deliberately: a credential in the draft means the session went I1 is rewritten to say what removing Bash does and does not buy, and to name this check as Verified against the actual logic:
🟡 Blocking #2 — the flag blocks more than the docs said. Fixed.
Corrected in both files and in the label table, keeping the accurate half of the original claim: 🔵 Advisory #3 — fence break-out. Fixed.The publish sanitizer now defangs 🔵 Advisory #4 — no committed tests. Not addressed.Still true: the sanitizer, the fail-closed verdict parser and All three workflows parse ( |
Preview DeploymentPreview URL: https://14edb508.bestax.pages.dev |
|
Went through the current branch ( Heads-up though — three CodeRabbit threads are still open and didn't make it into your summary. My read on each: 1. 2. 3. All three live under On #4 — happy to file the issue for a shell/workflow test harness covering the sanitizer, the fail-closed verdict parser, and |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/docs/guides/getting-started/ai-development.md (2)
58-58: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueClarify
author-onlyin the guide.
author-onlydoes not limit applyingclaude-reproor requesting a draft; the docs already say triage+ users apply it. Add a brief note thatauthor-onlymeans the workflow drafts the reproduction but never runs it in CI.🤖 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 `@docs/docs/guides/getting-started/ai-development.md` at line 58, Update the claude-repro workflow description in the getting-started guide to clarify that “author-only” means CI drafts the reproduction test but never runs it; preserve the existing statement that triage+ users can apply it and request drafts.
102-103: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTie the no-leak claim back to the documented controls.
No write tools and no PAT remove GitHub write access here. The workflow also keeps
show_full_outputoff, allows onlyghBash tools against the triggering issue/PR, blocks non-approved tools such asWebFetch, and applies egress blocking with harden-runner. State those documented controls, or revise the absolute “no channel to post or leak anything” wording if log, model, network, or other runtime paths are not bounded by the same controls.🤖 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 `@docs/docs/guides/getting-started/ai-development.md` around lines 102 - 103, Update the scan security explanation near the “no write tools and no PAT” statement to enumerate the documented controls: disabled show_full_output, restricted gh Bash access to the triggering issue/PR, blocked non-approved tools such as WebFetch, and harden-runner egress blocking. If those controls do not bound all log, model, network, and runtime paths, replace the absolute “no channel to post or leak anything” claim with appropriately qualified wording.
🤖 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 `@docs/docs/guides/getting-started/ai-development.md`:
- Line 59: Update the needs-security-review documentation entry and the related
workflow section at lines 111-113 to limit the blocking claim to
repository-controlled AI workflows, including claude-repro, claude-fix, `@claude`,
and `@bestaxbot` only; do not imply that CodeRabbit is blocked unless its behavior
is documented separately.
---
Nitpick comments:
In `@docs/docs/guides/getting-started/ai-development.md`:
- Line 58: Update the claude-repro workflow description in the getting-started
guide to clarify that “author-only” means CI drafts the reproduction test but
never runs it; preserve the existing statement that triage+ users can apply it
and request drafts.
- Around line 102-103: Update the scan security explanation near the “no write
tools and no PAT” statement to enumerate the documented controls: disabled
show_full_output, restricted gh Bash access to the triggering issue/PR, blocked
non-approved tools such as WebFetch, and harden-runner egress blocking. If those
controls do not bound all log, model, network, and runtime paths, replace the
absolute “no channel to post or leak anything” claim with appropriately
qualified wording.
🪄 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: f031d713-6dbe-443f-ad34-9d1e6fe6fd80
📒 Files selected for processing (12)
.claude/commands/triage-dedupe.md.claude/commands/triage-find-duplicate-prs.md.claude/commands/triage-find-issues.md.github/workflows/ai-scan.yml.github/workflows/ai-triage.yml.github/workflows/auto-close-duplicates.yml.github/workflows/bestaxbot-reply.yml.github/workflows/claude-implement.yml.github/workflows/claude-repro.yml.github/workflows/claude.ymlCLAUDE.mddocs/docs/guides/getting-started/ai-development.md
🚧 Files skipped from review as they are similar to previous changes (10)
- .github/workflows/claude-implement.yml
- .github/workflows/auto-close-duplicates.yml
- .github/workflows/claude.yml
- .claude/commands/triage-dedupe.md
- .github/workflows/bestaxbot-reply.yml
- .claude/commands/triage-find-duplicate-prs.md
- .github/workflows/ai-scan.yml
- .claude/commands/triage-find-issues.md
- .github/workflows/ai-triage.yml
- .github/workflows/claude-repro.yml
Addresses the coverage advisory on #361, for the half of it that does not need new infrastructure. `scripts/auto-close-duplicates.mjs` closes people's issues and had no test at all — root `test` was `turbo run test`, which fans out to the four packages and never reached root `scripts/`. It now also runs `node --test "scripts/*.test.mjs"`, matching how docs/scripts is already covered. The assertions are written around the consequence rather than the shape, because both failure modes are invisible in review — the code reads fine either way: - classify a human as automation and `humanCommentAfter` stops seeing their objection, so an issue closes over a live veto - loosen the marker match and a quoted or forged `Duplicate of #N` closes the wrong issue One test exists purely to document a cross-file coupling: claude-repro.yml defangs `Duplicate of #` in drafted tests *because* github-actions[bot] is an author this parser trusts. The sanitizer and this consumer have to stay in step, and nothing else says so. Checked the tests can actually fail, by mutating the source three ways and confirming each is caught by exactly the intended assertion: isAutomationAuthor matches any login containing "bot" -> "a real contributor is never automation" fails findMarkerComment drops its automation-author check -> "a human cannot forge a close by writing the marker themselves" fails humanCommentAfter drops the same-second id tiebreak -> "same-second posts are ordered by id, not dropped" fails Still uncovered, and deliberately left: the ai-scan verdict parser and the claude-repro publish sanitizer. Both are shell embedded in workflow YAML, so testing them means extracting them to files the workflows call — a refactor of security-critical automation that wants its own PR and its own review. Claude-Session: https://claude.ai/code/session_01TGA6sFTUGsJ6oXhfpjKEnh
Preview DeploymentPreview URL: https://dbc46c09.bestax.pages.dev |
The switches were documented in prose across three sections, one was not documented anywhere, and the thing most likely to surprise someone — which variables are ON when unset — was never stated plainly. `AI_LOOP_COPILOT` had no mention in the docs or CLAUDE.md despite being read by claude-implement.yml and set on the repo. It requests a Copilot review on loop PRs, which has to be asked for explicitly because Copilot's own automatic review skips bot-authored PRs on personal repos. The table leads with "unset means", not the value list, because two of these (`AI_LOOP_ENABLED`, `AI_SCAN_MODE`) are enabled when absent and so start spending Claude usage the moment their workflows land. Anyone staging a rollout needs to set them before merging, not after. Also notes that COPILOT_AGENT_FIREWALL_ENABLED and COPILOT_AGENT_FIREWALL_ALLOW_LIST_ADDITIONS appear in the same settings list but belong to GitHub's Copilot coding agent — nothing in .github/workflows/ reads them — so nobody hunts for the code that does. These are variables, not secrets, and the workflows that read them are public, so the names and effects are already discoverable by reading .github/workflows/. Documenting them discloses nothing and makes the kill switches findable, which is the point of having them. Claude-Session: https://claude.ai/code/session_01TGA6sFTUGsJ6oXhfpjKEnh
Preview DeploymentPreview URL: https://b44eeaf0.bestax.pages.dev |
|
@coderabbitai Full Review again |
Preview DeploymentPreview URL: https://ba9e8018.bestax.pages.dev |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (6)
.claude/commands/triage-find-issues.md:25
- This still relies on
--edit-last, which can overwrite the wrong bestaxbot triage comment when multiple triage commands run on the same PR (the exact failure .github/CLAUDE.md warns about). Instead, only use--edit-lastafter verifying the last bestaxbot comment contains this command’s marker; otherwise post a fresh comment.
- `TRIGGER=labeled` and marker present → continue; at the end refresh
that comment (`gh pr comment NUMBER --repo REPO --edit-last --body ...`
if it is your most recent comment on the PR; otherwise post fresh).
.claude/commands/triage-find-duplicate-prs.md:25
- This still relies on
--edit-last, which can overwrite a different bestaxbot marker comment if multiple triage commands have posted on the same PR. Make the refresh step marker-scoped: only use--edit-lastwhen your most recent comment contains the<!-- ai-triage:find-duplicate-prs -->marker; otherwise post a new comment.
- `TRIGGER=labeled` and marker present → continue; at the end refresh
that comment (`gh pr comment NUMBER --repo REPO --edit-last --body ...`
if it is your most recent comment on the PR; otherwise post fresh).
.claude/commands/triage-dedupe.md:29
- The REFRESH guidance uses
--edit-last, which can clobber the wrong bestaxbot comment when multiple automation markers exist on the same issue. Make the instruction marker-scoped so the agent only edits when it can confirm the last comment is actually the<!-- ai-triage:dedupe -->marker comment; otherwise it should post a fresh comment.
- `TRIGGER=labeled` and marker present → continue; at the end REFRESH
that comment instead of posting a new one: if it is your most recent
comment on the issue, use
`gh issue comment NUMBER --repo REPO --edit-last --body ...`;
otherwise post a fresh comment (the old one stays as history).
.github/workflows/claude.yml:50
- The label guards call contains() on github.event.issue.labels / github.event.pull_request.labels across multiple event shapes. For review_comment/review events, github.event.issue is absent; depending on expression coercion, contains(null, ...) can error and prevent the job from ever running. Make the label checks null-safe so they are inert when the object is missing.
!contains(github.event.issue.labels.*.name, 'needs-security-review') &&
!contains(github.event.pull_request.labels.*.name, 'needs-security-review') &&
.github/workflows/bestaxbot-reply.yml:56
- Same as claude.yml: this workflow handles multiple event types, so github.event.issue/pull_request can be null. Guard the needs-security-review label checks to avoid contains(null, ...) potentially breaking the entire job condition.
!contains(github.event.issue.labels.*.name, 'needs-security-review') &&
!contains(github.event.pull_request.labels.*.name, 'needs-security-review') &&
.github/workflows/claude-repro.yml:299
- Collecting the drafted test into a job output uses an unbounded
cat, which can exceed GitHub’s step output size limits if the drafter emits an unexpectedly large file (prompt injection / runaway output). Truncate the payload before writing it to $GITHUB_OUTPUT (the publish step already truncates for the comment).
{
echo "test<<$DELIM"
cat .repro/draft.test.tsx
echo "$DELIM"
} >> "$GITHUB_OUTPUT"
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/CLAUDE.md:
- Around line 55-71: Remove the PAT-backed comment permissions from the Claude
session configuration in ai-triage.yml by deleting Bash(gh pr comment:*) and
Bash(gh issue comment:*) from its allowlist. Route comment publication through a
deterministic publisher using GITHUB_TOKEN or a separate publisher that accepts
only a fixed, validated payload, while preserving the model’s read-only gh
access.
In `@SECURITY.md`:
- Around line 77-87: The public documentation overstates the inbound security
triage fail-closed guarantee. In SECURITY.md lines 77-87, state that non-clean
or invalid verdicts receive needs-security-review, while explicitly documenting
that budget or charging failures may skip scanning without applying that label;
in README.md line 194, replace the unconditional fail-closed statement with the
same distinction.
🪄 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: 7bf43414-1846-4fb3-b14a-3b42a393524d
📒 Files selected for processing (9)
.github/CLAUDE.md.github/workflows/ai-scan.yml.github/workflows/ai-triage.yml.github/workflows/claude-repro.ymlCLAUDE.mdREADME.mdSECURITY.mddocs/docs/guides/getting-started/ai-development.mdscripts/auto-close-duplicates.mjs
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/auto-close-duplicates.mjs
- .github/workflows/ai-triage.yml
- .github/workflows/claude-repro.yml
- docs/docs/guides/getting-started/ai-development.md
- .github/workflows/ai-scan.yml
| Two sessions where the allowlist is the _only_ thing between untrusted text and repository | ||
| write: | ||
|
|
||
| | Workflow | Credential in the job | What the allowlist is holding back | | ||
| | --------------- | ---------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | ||
| | `ai-triage.yml` | `AI_LOOP_PAT` (bestaxbot) | Full repo write. Confined to GET-only `gh` reads plus the two comment commands. | | ||
| | `ai-scan.yml` | job `GITHUB_TOKEN`, **write**-scoped (`issues`, `pull-requests`) | The gate charges a budget marker and the labeler applies `needs-security-review`, so the token must be write-scoped. The session cannot use it _only_ because the Bash allowlist admits nothing that writes. | | ||
|
|
||
| Concrete rules: | ||
|
|
||
| - Adding **any** entry to those two allowlists is a security change. Say so in the PR | ||
| description and explain why the entry cannot write. | ||
| - Never add `Bash(gh api:*)` to a session that ingests untrusted issue/PR text — it is a | ||
| general-purpose write primitive wearing a read-shaped name. | ||
| - Never add `Edit`, `Write`, `MultiEdit`, or `Task` to `ai-scan.yml`. | ||
| - `--disallowedTools` is defense in depth, and its deny rules do take precedence over the | ||
| allows — but do not lean on it as the primary control. Narrow the allowlist. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 -e 'AI_LOOP_PAT|allowedTools|Bash\(gh|gh (issue|pr) comment|gh api' .github/workflows/ai-triage.ymlRepository: allxsmith/bestax
Length of output: 13303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'CLAUDE.md relevant lines:\n'
sed -n '50,75p' .github/CLAUDE.md | cat -n
printf '\nai-triage relevant lines:\n'
sed -n '340,372p' .github/workflows/ai-triage.yml | cat -n
printf '\nAllowlist strings in ai-triage.yml:\n'
rg -n -C 2 -- '--allowedTools|--disallowedTools' .github/workflows/ai-triage.ymlRepository: allxsmith/bestax
Length of output: 6043
Remove PAT-backed comment writes from the model session.
ai-triage.yml gives the Claude session AI_LOOP_PAT plus Bash(gh pr comment:*) and Bash(gh issue comment:*). A prompt-injected request can reach a re-trigger-capable issue/PR comment as bestaxbot. Remove those comment tools from the model allowance; use a deterministic publisher with GITHUB_TOKEN, or pass only a fixed, validated payload through a separate publisher.
🤖 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/CLAUDE.md around lines 55 - 71, Remove the PAT-backed comment
permissions from the Claude session configuration in ai-triage.yml by deleting
Bash(gh pr comment:*) and Bash(gh issue comment:*) from its allowlist. Route
comment publication through a deterministic publisher using GITHUB_TOKEN or a
separate publisher that accepts only a fixed, validated payload, while
preserving the model’s read-only gh access.
| - **Inbound security triage** — new issues and pull requests are assessed by a | ||
| read-only AI session for three things: code crafted to harm whoever runs it, | ||
| prompt injection aimed at this repository's own automation, and social | ||
| engineering. Anything not positively clean is labeled `needs-security-review`, | ||
| which every AI entry point we control refuses until a maintainer clears it. | ||
| Three properties make it worth trusting: it **fails closed** (a crashed or | ||
| unparsable scan flags rather than passes), the session holds **no write tools | ||
| and no PAT** so an injected scan cannot post or act, and its reasoning is never | ||
| published — only a coarse category — so a flag cannot be used as an oracle for | ||
| tuning an evasion. A clean verdict covers the text as it stood when the item | ||
| opened, not edits made afterwards. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Align the public descriptions with the scan contract.
Only verdict parsing fails closed. Budget and charging errors can fail open and skip scanning, so an item can remain unscanned without needs-security-review.
SECURITY.md#L77-L87: state that non-clean or invalid verdicts are labeled, and document that budget failures can skip scanning.README.md#L194-L194: replace the unconditional fail-closed statement with the same verdict and budget distinction.
🧰 Tools
🪛 LanguageTool
[locale-violation] ~87-~87: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ... when the item opened, not edits made afterwards. Consumers can verify provenance thems...
(AFTERWARDS_US)
📍 Affects 2 files
SECURITY.md#L77-L87(this comment)README.md#L194-L194
🤖 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 `@SECURITY.md` around lines 77 - 87, The public documentation overstates the
inbound security triage fail-closed guarantee. In SECURITY.md lines 77-87, state
that non-clean or invalid verdicts receive needs-security-review, while
explicitly documenting that budget or charging failures may skip scanning
without applying that label; in README.md line 194, replace the unconditional
fail-closed statement with the same distinction.
The three triage commands told the session to refresh its marker comment with `--edit-last` "if it is your most recent comment" — an authorship test, not a marker test. `--edit-last` selects the newest comment by that author, and after this PR every triage path posts as bestaxbot, so the newest bestaxbot comment on an item is often a DIFFERENT marker: find-issues and find-duplicate-prs both run on PRs, and an issue can carry a dedupe marker plus a repro draft. Overwriting `<!-- ai-triage:dedupe -->` is the one with teeth — auto-close- duplicates.mjs reads that marker, so clobbering it destroys an auto-close candidate silently. Same defect the workflow-side fix already closed; these three prompt files were the remaining instances. Each now permits `--edit-last` only when the session's most recent comment IS its own marker comment, and points at .github/CLAUDE.md rule 6. Found by Copilot against the rule the new .github/CLAUDE.md had just added. Refs #362 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Security | The AI verdict is defeatable by a successful prompt-injection that makes the model's final line SECURITY-SCAN: clean; the design accepts this (advisory verdict + no write tools + triage+ live-role gate is the real barrier). |
.github/workflows/ai-scan.yml:452 |
| 2 | 🔵 Advisory | Robustness | The scan/repro gates fail open on any transient GitHub API error during the budget/label read — the item is silently unscanned and indistinguishable from clean downstream. Deliberate and documented. | .github/workflows/ai-scan.yml:389 |
| 3 | 🔵 Advisory | API | The Apply label comment claims the REST add-labels endpoint "auto-creates the label if missing." That endpoint returns 422 for an unknown label — it does not create one. Moot today (both needs-security-review and claude-repro already exist in the repo), but a false recovery assumption if a label is ever deleted. |
.github/workflows/ai-scan.yml:538 |
Overall: The change is sound and unusually well-defended for CI/AI automation. The two stated invariants hold under scrutiny — I1 (the OAuth token never shares a job with attacker-influenced execution) is preserved because the author job grants Claude no Bash/Task/network tool and the credential-shape check in Collect draft backstops the Read-can-reach-/proc/environ gap; I2 (no untrusted free text reaches a re-trigger-capable identity) holds because the draft is deterministically defanged and posted via GITHUB_TOKEN, whose comments emit no workflow events. The riskiest surface is the switch of ai-triage.yml to bestaxbot's PAT (AI_LOOP_PAT), whose comments do re-trigger — but every comment-triggered workflow (claude.yml, bestaxbot-reply.yml) now excludes sender.login == 'bestaxbot', and the only other issue_comment-triggered file is ai-triage.yml itself, which triggers on issues/pull_request opened/labeled, not comments, so the loop is closed. A human should focus first on the PAT-confinement allowlist in ai-triage.yml (the .github/CLAUDE.md contract correctly flags it as the one load-bearing control) and on running the canary described in the PR body before flipping AI_SCAN_MODE on.
Residual risk: the failure class this PR addresses is "untrusted issue/PR content escalating through AI automation." Ways it could still occur, each checked:
- Re-trigger via a bestaxbot PAT comment — refuted: the only
issue_commentconsumers (claude.yml,bestaxbot-reply.yml) both gatesender.login != 'bestaxbot';ai-triage.yml/ai-scan.ymldon't listen to comments;auto-close-duplicatestrusts bestaxbot but requires marker +Duplicate of #N+ a 14-day no-objection window +AI_TRIAGE_AUTOCLOSE=on(default off), and the repro publisher defangs both. - Token exfil via the drafting session — refuted for the naive path (no execution primitive, egress-block, exact+shape credential check, publish via a separate PAT-free job); the encoding check is correctly documented as a backstop, not a proof, so a chunked/rot13 exfil stays theoretically open — mitigated because the only published byte-stream is
draft.test.tsxand it is scanned. - Open-time window on the flag — confirmed as an accepted limitation (scanner and consumers both fire on
opened); bounded because every same-event consumer independently requires a trusted author, and external actors cannot satisfy@claude's OWNER/MEMBER/COLLABORATOR gate.
🏄 Dude, this PR is a triple-reef break with the channel clearly marked — split the token off from the gnarly code, defang the froth before it hits the lineup, and every entry point runs the buddy system. Nothing here's gonna pitch you over the falls. Paddle out, it's good to go — just watch that
audit→blockset wave and run the canary first. 🌊
Preview DeploymentPreview URL: https://a940c36c.bestax.pages.dev |
The `Apply label` comment asserted that REST addLabels auto-creates a missing label, unlike `gh --add-label`. Deep review flagged it as false (422 on an unknown label); GitHub's REST docs state neither behavior, so the claim is unverified in both directions rather than simply backwards. Removed instead of inverted. The comment now says what is actually true and load-bearing: needs-security-review is created ahead of merge, so this step never depends on on-demand creation, and that behavior must not be treated as recovery if the label is later deleted. Also records that a failure here leaves the item unlabeled — the gate fails open by design. Moot today (both labels exist), but the repo's own review checklist says a security comment must claim exactly what the mechanism delivers. Refs #362 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Preview DeploymentPreview URL: https://125cbdb7.bestax.pages.dev |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/auto-close-duplicates.yml:13
- The workflow header still says the first veto is a “non-bot comment”, but the underlying decision logic now treats bestaxbot (a machine User account) as automation. To avoid operators misunderstanding what counts as an objection, update this comment to say “non-automation” (human) rather than “non-bot”.
# the `<!-- ai-triage:dedupe -->` marker and a `Duplicate of #N` line, aged
# >= 14 days. Three vetoes, any one of which blocks the close forever or until it
# clears: a non-bot comment after the marker, a 👎 reaction on the marker
# comment, or a `reopened` event in the issue timeline (a reopened issue is
.github/workflows/claude-repro.yml:214
- The prompt claims the job “FAILS without” the REPRO-DRAFT final-line sentinel, but the workflow intentionally skips the sentinel check when the Claude action produces no execution_file (“no session to judge”). Update the wording so it matches the actual enforcement, otherwise it overstates the guarantee to future maintainers.
FINAL MESSAGE (machine-checked; the job FAILS without it): your last
output message must be exactly one line and nothing else, of the form
REPRO-DRAFT: drafted
or
REPRO-DRAFT: infeasible (<short reason>)
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | The documented pre-PR gate pnpm all does not run the new scripts/*.test.mjs — only ci.yml's separate pnpm run test step does. |
package.json:24 |
| 2 | 🔵 Advisory | Security | I2 is now held by convention across every issue_comment consumer excluding bestaxbot, not by construction (triage posts via bestaxbot's PAT). Verified both consumers exclude it. |
.github/workflows/ai-triage.yml:687 |
| 3 | 🔵 Advisory | Robustness | claude-repro's security-flag interlock fails open on a transient gh error (empty pipe input → no match → repro proceeds). |
.github/workflows/claude-repro.yml:849 |
Overall: The change is sound and unusually well-hardened — the two invariants (I1 token/execution separation, I2 no free text to a re-trigger identity) are coherently enforced, the verdict paths fail closed, the budget paths fail open, SHA pins are uniform (21/21 on the single repo-wide checkout SHA), and the new .github/CLAUDE.md codifies the rules that prior review rounds surfaced. The riskiest surface is the triage-as-bestaxbot switch, because it trades a construction-level I2 guarantee (GITHUB_TOKEN comments never re-trigger) for a convention (every issue_comment consumer must exclude bestaxbot) — I confirmed the only two such consumers, claude.yml and bestaxbot-reply.yml, both do. A human should focus first on the bestaxbot PAT scope confinement (the tool allowlist is the only thing between injected issue text and repo write) and confirm the post-merge canary before flipping AI_SCAN_MODE on.
Residual risk — ways the re-trigger / injection class could still occur:
- PAT-authored triage comment re-triggering a write-capable session — refuted: the only two
issue_comment:-triggered workflows (grep -rln "issue_comment:"→claude.yml,bestaxbot-reply.yml) both gatesender.login != 'bestaxbot';claude.ymladditionally requires a literal@claudethat the HARD RULES forbid the session from writing.claude-pr-loop.ymlfires onpull_request_review, not comments, so triage comments cannot reach it. - A flagged item slipping past a consumer — bounded, and documented: the
opened-event open-time window is real but every same-event path independently requires a trusted author, and third-party reviewers (CodeRabbit/Copilot) are explicitly out of scope. On the record, not a new defect. - Token exfil via the repro drafter — refuted for the naive path:
authorjob has no Bash/Task/network tool, harden-runner blocks egress,Collect draftrefuses literal/base64/hex/shape-matched credentials, andpublishnever co-locates with the OAuth token. The encoding check is correctly documented as a backstop, not a proof (chunked/rot13/arithmetic encodings defeat it) — an accepted, written-down limitation.
🏄 Gnarly amount of defense-in-depth on this one, dude — the token never surfs the same wave as the code, the verdicts wipe out safe, and every re-trigger channel's got a bestaxbot buoy in it. No blockers in the lineup; just three little ripples on the record. Paddle it out. 🌊
|
🎉 This PR is included in version 4.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.8.1 🎉 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 2.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
GitHub Actions / CI automation only (no product code). Four commits, building up from the triage-author fix that motivated the rest:
ci: post AI triage comments as bestaxbot— triage dedupe comments now post under the bestaxbot machine identity (robobun-style) instead of the anonymous github-actions[bot];auto-close-duplicates.mjs+ the triage command marker-checks learn the new author.ci: add author-only repro and security-scan workflows— the two new automations below.ci: guard AI entry points against re-trigger and flagged content— repo-wide hardening.docs: document repro and security-scan automation.@allxsmith/bestax-bulma)create-bestax)@allxsmith/bestax-docs)The two new automations (robobun-style, hardened for GitHub-hosted runners):
claude-repro.yml(author-only, 4 jobs, PAT-free) — a triage+ maintainer labels an issueclaude-repro; Claude drafts a minimal Jest reproduction test andgithub-actions[bot]posts it for a human to run. Nothing executes the draft.ai-scan.yml(separate workflow) — read-only Claude scan of new issues/PRs for malicious code, prompt injection, and social engineering; appliesneeds-security-reviewfail-closed. Holds no PAT and no write tools; the flag is advisory (pausesclaude-repro/claude-fix, does not block@claude/@bestaxbot).Two security invariants drive the design. A red-team of the first-draft two-job repro found it would leak
CLAUDE_CODE_OAUTH_TOKENto a public comment (secret-masking scrubs logs, not comment bodies) and let an@mentionre-trigger a write-capable session (contains()matches the raw substring):GITHUB_TOKEN, whose comments never re-trigger workflows).Repo-wide guards (commit 3):
claude.ymlbestaxbot sender-exclusion (also closes the re-trigger class for the shipped triage path) +needs-security-reviewrefusal;bestaxbot-reply.yml/claude-implement.ymlflag refusal; harden-runner + kill-switch header onai-triage.yml.Related Issue(s)
Refs #362
Type of Change
Checklist
scripts/auto-close-duplicates.test.mjsnow covers the auto-close decision logic (15 tests, mutation-checked), and rootpnpm testrunsnode --test "scripts/*.test.mjs"so root scripts are no longer invisible to CI. The two shell parsers (publish sanitizer, scan-verdict parser) remain untested — they are embedded in workflow YAML and need extracting first: [Refactor] Extract the shell security parsers from workflow YAML so they can be tested #454. Workflows validated below and need a canary run (see Additional Context)CLAUDE.mdfiles are updatedScreenshots / Demos
n/a — CI automation.
Additional Context
Verification done: YAML valid on all 18 workflows; the publish sanitizer unit-tested (machine markers broken,
@mentionsdefanged, legitimate imports untouched); the scan verdict parser tested fail-closed against clean/flagged/crashed/missing/forged inputs; prettier clean; no tabs; commitlint passed all commits.Two rollout notes:
ai-triage.ymljob ships inaudit, notblock. Flipping a live workflow straight to egress-block risks breaking it ifclaude-code-action's runtime egress needs an un-allow-listed host (it isn't cleanly documented). Audit monitors without blocking. Follow-up: after one real triage run, confirm the endpoint list and flip toblock. The new jobs already run inblock.claude-repro(confirm no PAT in the jobs, the draft posts as github-actions[bot], the label auto-removes); open a benign fake-injection issue (confirm it flags with no comment posted) and a normal issue (confirm it scans clean).New labels needed:
claude-repro(green#0e8a16),needs-security-review(red#b60205). Both were created ahead of merge, so neither path depends on a label being created on demand.🤖 Generated with Claude Code
Summary by CodeRabbit
State at merge (updated after review rounds)
The security scanner does not start on merge.
AI_SCAN_MODEis now explicit opt-in — itruns only when set to
onory, and every other value (off, unset, empty, a typo)disables it. Enable it deliberately once you are ready; merging alone changes nothing. Same for
claude-repro, which needs its label applied by a triage+ user.Labels exist:
claude-reproandneeds-security-reviewwere created ahead of merge, soneither feature is inert on arrival, and neither depends on a label being created on demand.
(An earlier revision claimed the REST add-labels endpoint auto-creates a missing label. GitHub
does not document that either way, so the claim was removed rather than inverted — deep review
finding 3.)
Changes made in response to review, beyond the original four commits:
Read-based token-exfil path inclaude-repro(Collect draftrefuses a draftcarrying a live credential, literal or base64/hex encoded), and rewrote I1 to state plainly
that the encoding check is a backstop rather than a proof.
--edit-lastcould overwrite an unrelatedgithub-actions[bot]comment — including a historical<!-- ai-triage:dedupe -->markerthat
auto-close-duplicates.mjsstill reads — destroying an auto-close candidate.needs-security-reviewblast radius (it gates every entry point this repo controls; thirdparty reviewers are not gated, and there is an open-time window), and the scan session's
token, which is write-scoped rather than read-only.
AI_LOOP_ENABLEDhad been documented backwards.Final review round added
.github/CLAUDE.md— the security contract for anything in.github/workflows/. Copilot'sallowedToolsadvisory asked for a comment at one call site;the durable form is a document covering every workflow, so it states the two invariants,
allowlists as a confinement boundary that never widens casually, SHA pinning,
== 'on'opt-ingating, fail-closed verdicts vs fail-open budgets, marker-scoped comment edits, and a reviewer
checklist. Both root
CLAUDE.mdand this PR's other lessons feed it. Same commit also:actions/checkoutinai-scan.ymlandclaude-repro.ymlto the repo-wide pin. Bothsat on
9c091bb(v7.0.0, twelve commits behind) while the other nineteen usages were on3d3c42e5(v7.0.1) — and both were commented# v7, so the drift was invisible to areader. Now exactly one SHA repo-wide.
auto-close-duplicates.mjsheader, which still described the objection rule as "nonon-bot user" after the logic moved to
isAutomationAuthor().SECURITY.mdandREADME.mddescribe the scanner as active whileAI_SCAN_MODEshipsoff. That is deliberate: the variable is turned on shortly after merge, and the steady state is
on, so wording the docs around a temporary off-state would need reverting immediately.
Known gaps, all filed: #454 (extract and test the two shell parsers), #455 (split
ai-scanso the model session runs with
contents: read), #457 (publish triage comments from astructured payload — this PR moves triage to a PAT-authored identity, whose comments can
re-trigger workflows, so I2 is currently held by every consumer excluding bestaxbot rather than
by construction; raised by CodeRabbit and verified), and #456 (the two post-merge obligations —
flipping
ai-triageharden-runner fromaudittoblockafter one real run, and the canarychecklist for both new automations). #456 exists because those two were previously recorded
only in this description, which a squash merge discards.