Repository navigation
Conversation
…ee text The triage session goes GET-only: it reports TRIAGE-PAYLOAD lines that a fail-closed validator parses and renders into comment bodies, and a separate step upserts them by marker as bestaxbot. Both comment commands leave the allowlist, which narrows rather than widens it. AI_LOOP_PAT leaves the Claude step; the session now holds only the job GITHUB_TOKEN. That is the part worth reviewing: the PAT is full repo write and the same durable credential claude-implement.yml pushes with, so leaking it means revoking the whole loop's identity — and this session runs Bash and Task over attacker-controlled text with egress unenforced (#487). Reading an env var is not a write, so the allowlist never guarded that credential at all. Comments are still authored by bestaxbot, so their provenance and event behavior are unchanged. scripts/render-triage-comments.mjs replaces the workflow's inline sentinel jq (rule 9), reusing parse-scan-verdict.mjs's helpers. It end-anchors the payload lines and requires an exact count, validates a closed schema, and renders from a trusted skeleton — sanitizing only the model's title/reason strings, before interpolation, never the assembled body, so the marker and Duplicate-of line auto-close-duplicates.mjs consumes cannot be self-defanged. Its test sibling pins the bodies byte-for-byte and the fail-closed matrix, and asserts a hostile field cannot forge either. The empty-result trigger policy moves out of the prompt into the renderer, so a labeled rerun can no longer be silently skipped. Fixes #457. Refs #361, #340, #317, #338, #312.
|
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 (13)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe AI triage workflow now emits structured payloads. A deterministic renderer validates and formats comments. A separate publisher uses ChangesStructured triage publishing
Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: 🔵 Low · up to The change safely separates validated triage rendering from comment publication, but merge readiness still requires owner awareness because marked comments can be updated when authored by any bot-class account rather than only the intended publisher, and the publishing credential’s effective scope is not established in repository configuration. Sequence Diagram(s)sequenceDiagram
participant TriageSession
participant RenderTriageComments
participant PickTriageUpsert
participant GitHub
TriageSession->>RenderTriageComments: Emit TRIAGE-PAYLOAD JSON
RenderTriageComments->>RenderTriageComments: Validate and sanitize payload
RenderTriageComments->>PickTriageUpsert: Select marker comment for refresh
PickTriageUpsert->>GitHub: Read comments
GitHub-->>PickTriageUpsert: Return comment records
PickTriageUpsert-->>RenderTriageComments: Return target comment ID
RenderTriageComments->>GitHub: Render and upsert comment as bestaxbot
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed change summary, linked issues, security rationale, behavior changes, risks, merge guidance, and verification results. It does not reproduce all template headings or checklist items, but the missing items are non-critical. Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 47.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 6 files. (7 skipped: 7 unsupported.)
✨ Finishing Touches 💡 1📝 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://463f072f.bestax.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
The security-sensitive PAT publication logic remains as untested inline workflow shell, contrary to the repository’s extraction convention.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Moves AI triage to validated structured payloads, addressing #457 while isolating the bestaxbot PAT from the model session.
Changes:
- Adds a fail-closed payload validator, sanitizer, renderer, and comprehensive tests.
- Restricts the model session to read-only tools and publishes comments deterministically.
- Updates triage commands and documentation for the new architecture.
File summaries
| File | Description |
|---|---|
.github/workflows/ai-triage.yml |
Validates, renders, and publishes structured triage results. |
.github/CLAUDE.md |
Documents the security boundary and renderer. |
.claude/commands/triage-dedupe.md |
Defines CI payload reporting for issue deduplication. |
.claude/commands/triage-find-issues.md |
Defines payload reporting for related issues. |
.claude/commands/triage-find-duplicate-prs.md |
Defines payload reporting for duplicate PRs. |
scripts/render-triage-comments.mjs |
Implements validation, sanitization, and rendering. |
scripts/render-triage-comments.test.mjs |
Covers renderer and payload behavior. |
scripts/sanitize-repro-draft.mjs |
Defangs triage payload sentinels in drafts. |
scripts/sanitize-repro-draft.test.mjs |
Tests sentinel sanitization. |
docs/docs/guides/getting-started/ai-development.md |
Explains deterministic triage publication. |
CLAUDE.md |
Updates repository-level triage documentation. |
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| run: | | ||
| set -euo pipefail | ||
| COUNT=$(jq '.comments | length' "$ENVELOPE") | ||
| for i in $(seq 0 $((COUNT - 1))); do |
Copilot's review of the publish step was right that the marker/author comment selection is exactly the shape rule 9 sends to a tested script. Extracting it turned up a second problem: the jq was a reimplementation of logic auto-close-duplicates.mjs already exports, and it had drifted. Its author test was `login == "bestaxbot" or type == "Bot"`, which misses a `[bot]`-suffixed login typed as User, where the shared isAutomationAuthor covers Bot-type, the suffix, and named machine users. scripts/pick-triage-upsert.mjs now makes that choice, importing that helper so the two consumers of the triage markers cannot drift again, and parse-scan-verdict.mjs's parseExecutionRecords to flatten the one-array-per-line stream `gh api --paginate` produces when --jq is applied per page. That pagination shape is what the old `tail -n 1` survived only by accident: a marker comment on any page but the last was found only if the last page happened to hold one too. Its test sibling pins that case, the legacy author classes (claude[bot], github-actions[bot]) a labeled re-run on an old item depends on, and the rule 6 failure the whole thing exists to prevent — never selecting a human comment that quotes the marker. The remaining shell is a loop, a disposition check, and PATCH-vs-POST, which is the size claude-repro.yml's publish keeps inline.
|
Thanks — the rule 9 point on the publish shell was right, and acting on it surfaced a second problem the comment did not mention. Extracted to It also imports On scope, I stopped short of your suggestion to cover "refresh/post and |
Preview DeploymentPreview URL: https://b78d49e8.bestax.pages.dev |
There was a problem hiding this comment.
🔵 Needs a closer look
Unicode sanitization gaps and inaccurate credential-boundary documentation should be corrected before approval.
Review details
Suppressed comments (4)
Previously missed (4) — in code that hasn't changed since the last review.
scripts/render-triage-comments.mjs:142
- The single-line guard omits C1 controls and Unicode line/paragraph separators. A payload can encode U+0085, U+2028, or U+2029, pass validation, and place a line-separator character into the bestaxbot comment despite the closed schema’s no-control/single-line contract. Reject these ranges as well and cover them in the hostile-text test.
const CONTROL_RE = /[\u0000-\u001F\u007F]/;
scripts/render-triage-comments.mjs:163
- This is not the complete Unicode Bidi_Control set: U+061C ARABIC LETTER MARK is missing, so attacker-controlled titles/reasons can still alter bidirectional rendering even though this sanitizer is intended to remove bidi controls. Include U+061C and add it to the bidi regression test.
[/[\u200E\u200F\u202A-\u202E\u2066-\u2069]/g, ''],
.github/workflows/ai-triage.yml:172
- Job-level permissions apply to the Claude step too, and line 373 passes that write-scoped
GITHUB_TOKENinto the session. Saying these grants only cover the gate and label-removal steps contradicts the later security note and obscures that the read-only tool allowlist is the session’s confinement boundary.
# Triage comments themselves post via bestaxbot's PAT (see the publish
# step; the Claude step holds only this job's GITHUB_TOKEN), not
# GITHUB_TOKEN — these grants only cover the gate and label-removal
# steps.
.github/CLAUDE.md:85
- The renderer and PAT holder are different steps: validation/rendering occurs at lines 512–544, while only the publish step at lines 565–611 receives
AI_LOOP_PAT. The current wording incorrectly says the rendering step holds the credential, which misdocuments the security boundary this PR introduces.
payload, a deterministic step renders the body from a trusted skeleton, and only that
step holds bestaxbot's PAT — which also means no model session shares an environment
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | The strict NDJSON parse of gh api --paginate --jq '[…]' output rests on gh emitting one compact array per page; no in-repo run has exercised >1 page through this stricter path (the budget probe uses a tolerant jq -s). Fails safe. |
scripts/pick-triage-upsert.mjs:96 |
| 2 | 🔵 Advisory | Robustness | Bash(gh search:*) has no prior in-repo precedent under GITHUB_TOKEN; a 403 would fail closed (no payload → job errors, no comment) rather than post wrong. |
.github/workflows/ai-triage.yml:393 |
| 3 | 🔵 Advisory | Robustness | Documented rule-9 bootstrap gap: a manual ai-triage label run on this branch fails MODULE_NOT_FOUND until merged, since the job checks out the default branch. Accepted; label removal is always(). |
.github/workflows/ai-triage.yml:344 |
Overall: The change is sound and, as advertised, strictly narrowing — it removes the two gh … comment allowlist entries and moves AI_LOOP_PAT out of the model session into a deterministic publish step, so no model session shares an environment with a re-trigger-capable credential (I2). The riskiest surface is the new render-triage-comments.mjs trust boundary between model output and a posted comment; I exercised it and it holds: end-anchoring plus an exact per-command payload count reject echoed/interleaved/trailing sentinels, the schema is closed (unknown keys including __proto__, control chars, out-of-range/self numbers, and a duplicateOf not in the listed duplicates all reject), and sanitizeField runs on title/reason before interpolation so a hostile field cannot forge the <!-- ai-triage:dedupe --> marker or the Duplicate of #N line the auto-close cron consumes. The human should focus first on confirming the operational watch-items (the gh search token class and the multi-page comment fetch) on the first live run — both fail safe, so they gate nothing.
Residual risk (for the class this PR addresses — model free text reaching a re-trigger-capable comment):
- Forged marker /
Duplicate of #Ninjection — refuted:sanitizeFieldencodes<!--, both-->/--!>spellings, andDuplicate of #;CONTROL_RErejects newline/tab at validation so no title can inject an extra line. Verified againstauto-close-duplicates.mjs's ownMARKER/DUPLICATE_REin the test sibling (65/65 pass). - Attacker text echoed as a payload — refuted: any extra sentinel line makes the count differ from expected and fails closed; the last result record's tail must be exactly the expected payloads, position-matched to the command.
- Wrong comment PATCHed / duplicated on re-run — refuted:
pickUpsertTargetselects only automation-authored comments carrying the exact marker (sharedisAutomationAuthor); a human comment quoting a marker is never selected, and "print nothing → POST" is the safe direction. - Silent no-op (a bailed session posting nothing quietly) — refuted: a session that dies mid-task emits no payloads, the count check fails, and the job fails loudly; the #317/#338 shapes both map to a non-zero exit with empty stdout.
🏄 Dude, this one's a clean drop-in — they didn't paddle out for more capability, they trimmed the sail: two comment tools gone, the gnarly PAT off the model's board and locked in the deterministic publish channel, and a fail-closed renderer standing between the model's words and the wave. Tests are glassy at 65/65. Send it. 🌊
#455 landed while this branch was open, and both changes rewrote the same rule 2 table in .github/CLAUDE.md — #455 the ai-scan row, this branch the ai-triage one. Resolved by taking both rows on #455's wider header, which already frames the column as "which credential", and by carrying that framing into the ai-triage row: repository write is still the allowlist's to hold there (the gate and label removal need a write-scoped job token), the model credential likewise, and what changed is that AI_LOOP_PAT is no longer in the job at all. Everything else auto-merged: rule 9 keeps this branch's entry for the two new scripts, rule 2's closing bullet keeps #455's worked example, and the ai-development guide keeps both edited paragraphs.
Preview DeploymentPreview URL: https://7298462a.bestax.pages.dev |
There was a problem hiding this comment.
🔵 Needs a closer look
The PAT-backed publish step executes scripts from a workspace that the untrusted model session can potentially modify.
Review details
Suppressed comments (1)
.github/workflows/ai-triage.yml:601
- Blocking: this PAT-holding step executes
pick-triage-upsert.mjsfrom the same checkout the untrusted Claude session just had Bash access to. Bash redirection can modify tracked files despiteEdit/Writedenies (the repository already documents this at.github/workflows/claude-pr-loop.yml:163-166); for example, an injected session can redirect an attacker-controlled issue body into this script, emit an otherwise-valid payload, and then have line 600 execute that body withGH_TOKEN=AI_LOOP_PAT. The validator invoked at line 530 is mutable for the same reason, so step-scoped PAT injection does not establish I2. Perform validation and publishing in fresh isolated jobs/checkouts, transferring only the execution file and validated envelope between them, so no PAT-backed step executes a workspace the model could modify.
EXISTING=$(node scripts/pick-triage-upsert.mjs \
--comments-file="$COMMENTS" --marker="$MARKER")
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Superseded by #582, which merged as 4361064. Both carry Recording what was compared, since the two designs differ in ways worth remembering if this is ever revisited:
The reasoning in this description was sound and largely matches where #582 landed after review, including the point that reading an environment variable is not a write, so the tool allowlist never guarded |
Fixes #457. Refs #361, #340, #317, #338, #312.
The triage session stops posting. It reports
TRIAGE-PAYLOAD:lines; a fail-closed validator parses them and renders the comment bodies from a trusted skeleton; a separate step upserts those bodies by marker as bestaxbot. This is the shapeclaude-repro.ymlalready uses, applied to the sibling that did not have it.This is a security change (
.github/CLAUDE.mdrule 2)Two parts, both narrowing:
1. The allowlist shrinks.
Bash(gh pr comment:*)andBash(gh issue comment:*)are removed, leaving GET-onlyghreads plusTask. Nothing is added.2.
AI_LOOP_PATleaves the Claude step. The session now gets the job'sGITHUB_TOKEN; the PAT appears in exactly one place, the publish step'senv.The second is the part worth reviewing, and the argument for it is not the re-trigger one:
AI_LOOP_PATis documented as full repo write and is the same durable credentialclaude-implement.ymlpushes with andclaude-pr-loop.ymluses across four steps. Leaking it means revoking the whole loop's identity.GITHUB_TOKENexpires with the job and is scoped to this repo.BashandTask, ingests attacker-controlled issue and PR text by design, and runs with egress unenforced ([Security] harden-runner silently downgrades egress-policy: block to audit — no AI job has ever enforced egress #487).The re-trigger and full-repo-write arguments are real but secondary: both require the allowlist to fail first, and after this change the session has no comment tool regardless of which token it holds.
What does not change
Published comments are identical in provenance: bestaxbot authors them, via the PAT, in the publish step, so they still emit
issue_commentevents exactly as before.claude-implement.yml(gh pr edit --add-reviewer "@copilot"), which already runs onGITHUB_TOKEN, and its auto-review fires on PR open/push — never on triage comments.bestaxbot-reply.ymlandclaude.ymlboth gate onsender.login != 'bestaxbot', andclaude-pr-loop.ymlfires on CodeRabbit reviews.Watch item for the first live run
ai-scan.ymlprovesgh issue view/gh pr view/gh pr diffwork on the same action SHA withGITHUB_TOKEN, butgh searchhas no in-repo precedent — triage is the only session that searches. Same 30 req/min class, public repo, and the command files already cap 6 searches per agent, so this should be a non-event. If it 403s, the revert is one field: restoregithub_token: ${{ secrets.AI_LOOP_PAT }}on the Claude step. Everything else, publish step included, is unaffected.The renderer
scripts/render-triage-comments.mjsreplaces the workflow's inline sentinel jq (rule 9) and reusesparse-scan-verdict.mjs's exported helpers rather than growing the YAML.__proto__included — rejects. Numbers must be safe integers in range and not the item being triaged;duplicateOfmust name one of the listed duplicates; titles and reasons are byte-capped and must contain no control characters, which is the structural kill for newline smuggling.sanitize-repro-draft.mjs, and the header says why: a whole-body pass would defang the skeleton's own<!-- ai-triage:dedupe -->marker andDuplicate of #Nline, whichauto-close-duplicates.mjsconsumes, and silently break auto-close. Every@is encoded, not just the three re-trigger handles, because bestaxbot authors these comments so any live mention pings a person.set -euo pipefailwith no fallback plus an exit-0-empty-stdout guard (the claude-repro precedent).stderrcarries fixed strings only, never payload-derived text.The test sibling pins the rendered bodies byte-for-byte, imports the consumer's own
MARKER/DUPLICATE_REso the coupling cannot drift, and pre-asserts its hostile fixtures are actually hostile before checking they are neutralized.Behavior change worth noting
The empty-result trigger policy moves out of the prompt and into
buildEnvelope: dedupe reports its empty result on any trigger; the two PR commands stay silent onopenedand post onlabeled. That rule used to depend on the model remembering it. It is now structural — a labeled rerun cannot be silently skipped.Incidentally this also fixes a real cosmetic bug: existing triage comments show the literal template text
**Related** (optional, at most 3), because the model copied the parenthetical out of the command file. The renderer emits**Related**.Expected self-inflicted failure (rule 9 bootstrap gap)
This PR's own auto-triage run did not exercise the gap: the gate skipped it with
PR body already links an issue (Fixes/Closes) — skipping, so no session started (run 33137204485). The gap is still real for a manualai-triagelabel run on this branch — the job checks out the default branch, which does not yet containscripts/render-triage-comments.mjs, so the Validate step would exitMODULE_NOT_FOUNDand fail the job.That is the documented cost of extraction and is not to be fixed by checking out PR head — running PR-authored code in a credential-holding job is the thing that pin prevents. The label-removal step is
always(), so theai-triagebutton would not wedge.Merge order
Please merge #580 (#455) first. Both PRs edit
.github/CLAUDE.mdrule 2's table — that one changes theai-scanrow and the table's intro line, this one changes theai-triagerow — so this branch will want a rebase afterward.Review checklist
--disallowedToolsunchanged.sanitizeField, into a trusted skeleton.GITHUB_TOKEN(still write-scoped for the gate and label removal — the allowlist is the control there, and the header says so).AI_TRIAGE_AUTOCLOSEmoves from the prompt to the render step; the model is no longer told it.--edit-last(rule 6).Verification
pnpm allgreen. 500 script tests pass, 47 of them new. The two new workflow steps were also run end-to-end locally against fixture execution files withghstubbed: full dedupe render with the auto-close notice, PR-modeopenedsilence vslabeledposting on identical payloads, trailing-narration rejection with exit 1, and the missing-execution-file tolerance path.Summary by CodeRabbit
New Features
Bug Fixes
Documentation