Repository navigation
ci: publish AI triage comments from a structured payload, not model free text - #582
Conversation
Adds scripts/render-triage-comment.mjs (rule 9): the triage session emits a structured payload on its existing TRIAGE-RESULT sentinel line, and this script validates it, sanitizes the two free-text fields, renders the body from renderer-owned constants and validated integers, and upserts it scoped to the marker it owns. Two modes so the execution file - the whole transcript, echoed untrusted issue/PR text included - never crosses into the job that holds the PAT. Everything a reader or auto-close-duplicates.mjs acts on is renderer-owned: the markers, `Duplicate of #N`, `Fixes #N` and the auto-close notice, whose day count is imported so the comment cannot promise a window the cron will not honor. Fields are escaped and stripped of every autolink vector - mentions, issue URLs, `GH-123`, `#N` - so quoted titles stop pinging people and cross-referencing issues. assertRenderedInvariants is the backstop that holds even if a defang rule is later weakened. Two consumer-side fixes the renderer surfaced: - findMarkerComment skipped past a marker comment naming no duplicate and kept scanning older ones, so a retraction resurrected the target it retracted. A retraction posted as a fresh comment is the normal case whenever the previous comment belongs to a different automation identity, which is true of most marker comments predating bestaxbot. - lastNonEmptyLines is factored out of lastNonEmptyLine so the renderer shares the watchdog's exact jq-parity line semantics, and never pads.
Four jobs (gate / triage / publish / cleanup), modeled on claude-repro.yml. Drops `gh issue comment` and `gh pr comment` from the session allowlist and moves publishing into a job the model never runs in. The credential is the point. The Claude action's github_token becomes the session's GH_TOKEN, so passing AI_LOOP_PAT gave a session that ingests untrusted issue/PR text a credential with full repo write, unscoped by the job's permissions block and confined only by the allowlist. It now gets the job GITHUB_TOKEN scoped contents/issues/pull-requests: read, and the PAT lives only in `publish`, which runs no model. In a single job there was no arrangement that achieved this without minting a new secret: GITHUB_TOKEN is job-scoped and `gate` needs issues: write for the budget marker. Also closes a live exfiltration channel: `Read` is not confined to the workspace, and the allowlist's prefix match accepted `gh issue comment N --body "$CLAUDE_CODE_OAUTH_TOKEN"`. The sink is gone, and the render step compares the finished body against both of the job's secrets in literal, base64 and hex form before writing it. That is a backstop that raises the cost of exfiltration, not a proof - the tool restriction remains the control. The watchdog jq is unchanged: it is verb-agnostic, and a compact one-line JSON suffix cannot alter the sentinel count. Verified against synthetic execution files at n=1 and n=2; a pretty-printed payload fails it, which is the correct direction. cleanup's `if` reproduces the old step-level behavior exactly. Label removal used to be a step, so it never ran when the job-level `if` was false; as a job under a bare always() it would newly fire on fork PRs, where the DELETE 403s. The `needs.gate.result != 'skipped'` term is what prevents that. The concurrency comment is corrected rather than compounded: GitHub keeps one pending run per group, so a burst silently drops the items in the middle - cancelled, not skipped, so no gate log and no budget charge explains the gap. `cancel-in-progress: false` never protected the queue. Fixing it properly means making the counter safe under concurrency; re-scoping the group per item would reintroduce the counter race, which is worse.
… they carried The three .claude/commands/triage-*.md files lose their comment tools and their --edit-last caveats: the publisher's marker-scoped upsert satisfies rule 6 structurally, so three long warnings about a hazard the model can no longer create were pure noise. They gain the size budget the model was never told (an over-long payload fails the run), a `skip (search failed)` path so a rate-limited fan-out cannot publish a confident "No duplicates found.", and a local-run recipe that actually works - the previous "when run locally" wording dead-ended at a printed sentinel with nothing able to render it. Three governing documents had been left inaccurate, and CodeRabbit and the @claude action read two of them as project instructions: - .github/CLAUDE.md rule 2 still listed ai-triage as the second session where the allowlist is the only control, holding AI_LOOP_PAT with "GET-only gh reads plus the two comment commands". It is now the worked example for the rule's own "prefer removing the need for the boundary" bullet. Rule 9 gains its third extracted parser. - CLAUDE.md said the session comments with related issues/duplicates. - The ai-development guide claimed no model or issue-author free text reaches the published comment. Candidate titles and reasons still do - escaped, and stripped of anything that could mention, link or re-trigger - so it now says which half is renderer-owned instead of overclaiming.
|
Warning Review limit reachedNext included review available in 8 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
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://b94df178.bestax.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
Sanitization can still expose links and credentials, while one verdict transition can bypass the objection window.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Refactors AI triage so model output becomes validated structured data before deterministic publication by bestaxbot.
Changes:
- Splits triage gating, model execution, publishing, and cleanup into separate jobs.
- Adds a tested renderer, sanitizer, and marker-scoped publisher.
- Corrects duplicate retraction handling and updates documentation.
File summaries
| File | Description |
|---|---|
scripts/render-triage-comment.test.mjs |
Tests rendering, sanitization, publishing, and invariants. |
scripts/render-triage-comment.mjs |
Implements structured payload rendering and publication. |
scripts/parse-scan-verdict.test.mjs |
Tests multi-line sentinel extraction. |
scripts/parse-scan-verdict.mjs |
Adds shared final-line extraction helper. |
scripts/auto-close-duplicates.test.mjs |
Covers final duplicate retractions. |
scripts/auto-close-duplicates.mjs |
Single-sources wait time and honors latest verdict. |
docs/docs/guides/getting-started/ai-development.md |
Documents deterministic triage publication. |
CLAUDE.md |
Updates repository automation guidance. |
.github/workflows/ai-triage.yml |
Separates credentials and introduces structured publication. |
.github/CLAUDE.md |
Documents the new workflow security pattern. |
.claude/commands/triage-find-issues.md |
Changes issue matching to structured reporting. |
.claude/commands/triage-find-duplicate-prs.md |
Changes PR matching to structured reporting. |
.claude/commands/triage-dedupe.md |
Changes duplicate search to structured reporting. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 1 blocking · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟠 Major | Correctness | verdictChanged misses the No duplicates → Duplicate of #N transition, so the refresh PATCHes an old comment in place and the auto-close cron reuses an already-expired 14-day objection window |
scripts/render-triage-comment.mjs:862 |
| 2 | 🔵 Advisory | Robustness | On a changed verdict the old marker comment is left in place (a second bestaxbot marker accumulates); findMarkerComment/findTriageComment read the latest so it is inert, but stale duplicate comments pile up |
scripts/render-triage-comment.mjs:882 |
| 3 | 🔵 Advisory | Robustness | cleanup cannot unwedge the ai-triage label on fork-PR labels (gate skipped → cleanup skipped; and fork GITHUB_TOKEN DELETE 403s anyway) — documented and accepted, on the record |
.github/workflows/ai-triage.yml:770 |
Overall: The core security refactor is sound and lands its stated goal: the model session drops to a read-only GITHUB_TOKEN, the PAT lives only in a no-model publish job, comment bodies are built from renderer-owned constants + validated integers, and the free-text fields are aggressively defanged with an independent assertRenderedInvariants backstop. The four-job split, the gate/budget path, the watchdog-contract re-check, and the cleanup if: all hold up under scrutiny, and the 110 script tests pass. The one real defect is downstream of the security change: the objection-window guard in runPublish only fires when two duplicate targets differ, so promoting a prior "No duplicates found." comment into a duplicate verdict silently reuses a stale clock — which, with AI_TRIAGE_AUTOCLOSE active, can auto-close an issue with no fresh objection window while the body promises one. Focus review there first; the fix is a one-line predicate change plus a test for the undefined -> defined edge.
Residual risk: the failure class this PR targets is "model/untrusted free text reaching a re-trigger-capable identity or a consumer."
- Free text → comment structure: refuted. Fields pass through
sanitizeField(remove-invisibles → flatten-with-space → escape markup → defang#N/mentions/URLs/GH-/creds) thenassertRenderedInvariantsre-checks marker count, no raw markup, no live mention, no smuggled sentinel, and that every raw#Nis renderer-emitted — verified by the injection suite running the realauto-close-duplicates.mjsover rendered output. - Free text → auto-close consumer: the
Duplicate of #Nvalue is always a validated integer and single-target, andfindMarkerComment's newreturn nullcorrectly makes the latest marker authoritative. The one gap is timing, not content — finding #1 (stalecreated_atreused across a verdict change), not a forged target. - Payload smuggled mid-transcript: refuted. Extraction reads only the last N non-empty lines of the final result record, and
parseSentinelsre-counts allTRIAGE-RESULT:lines and fails closed on any extra — independent of the YAML watchdog.
🏄 Righteous refactor, dude — the PAT's off the model's beach and the sanitizer's got a solid backstop set. Just one gnarly sleeper current: an old "no dupes" comment can get a duplicate verdict slapped on it with a clock that already ran out, and the closer'll pull the issue under before anyone paddles out to object. Patch that one predicate and this thing's glassy.
…ed one The repost guard keyed on two defined targets differing, so it missed the "No duplicates found." -> `Duplicate of #N` promotion: with no previous target to differ from, a months-old comment was PATCHed in place and the auto-close cron inherited a clock that had already run out. Same stale-window bug as a changed target, reached from the other side, and with AI_TRIAGE_AUTOCLOSE active it closes an issue whose body promises 14 days. Keying on the new target alone covers both directions and leaves a retraction patching in place, which is what strips the `Duplicate of #N` the cron reads. Both edges now have tests; the marker-selection test gains a matching target so it still exercises the PATCH path it exists to check.
|
Deep review finding 1 is fixed in 93bffda. The predicate required both the previous and the new It now keys on the new target alone, which covers both directions and leaves the retraction case patching in place (that edit is what strips the
Both new edges have tests, and the marker-selection test gained a matching target so it still exercises the PATCH path rather than passing through the repost branch. Findings 2 and 3 are accepted as advisory and left as they are. On 2, the extra marker comment is inert by construction now that |
Preview DeploymentPreview URL: https://d5754af1.bestax.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
Sanitization still permits active Markdown links and reversibly encoded credential-shaped values in published comments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
scripts/render-triage-comment.mjs:215
- Encoding only
://does not neutralize explicit Markdown links. GFM decodes character references in link destinations, so a model field such as[click](https://github.com/.../issues/999)still renders as an active link and can emit the cross-reference this sanitizer is intended to prevent. Neutralize Markdown link delimiters as well.
s = s.replace(/:\/\//g, '://');
scripts/render-triage-comment.mjs:226
- This transformation hides matching GitHub token shapes from the later exact-value check without actually redacting them for readers:
ghs_plus a 20+ character body becomesghs_…, which GFM renders and copies with the original underscore. ConsequentlyfindCredentialLeak()sees neither the literal secret nor theghs_shape and permits publication. Compare known secret forms before entity transformation, and replace unknown credential shapes with irreversible redaction rather than an HTML entity.
// 9. Defang a credential-shaped string so the published comment can never
// carry a working token. findCredentialLeak still REFUSES outright on
// the job's own secrets; this only covers shapes it cannot compare
// against, and defanging beats publishing them intact.
s = s.replace(CREDENTIAL_SHAPE_RE, m => m.replace(/_/g, '_'));
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
#455 landed the same restructure for ai-scan that this branch does for ai-triage, and both rewrote rule 2's table, so the conflict was semantic rather than textual. Resolved toward main's framing, which is the better one: the table is about which CREDENTIAL the allowlist stands between untrusted text and, not about repository write, and "the session can't write to the repo any more" is not the same claim as "the allowlist stopped mattering". Both rows now tell that same two-part story - repository write: nothing; the model credential: everything, because each session still runs Bash beside CLAUDE_CODE_OAUTH_TOKEN and egress is not enforced. The "prefer removing the boundary" bullet keeps #455 as the first worked example and gains #457 as the harder one: ai-triage's session did not merely sit near a write credential, it held one, because the Claude action installs its github_token as the session's GH_TOKEN. Narrowing an allowlist cannot reach a credential passed to the action - only moving the work can.
Preview DeploymentPreview URL: https://36e54414.bestax.pages.dev |
Preview DeploymentPreview URL: https://ab2cacb3.bestax.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
Sanitized fields can still produce rendered links, and credential normalization can bypass live-token detection.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
scripts/render-triage-comment.mjs:254
- This comparison runs after
sanitizeFieldconverts underscores in credential-shaped values to_. A literal job token such asghs_...therefore becomesghs_..., no longer matches the actualGITHUB_TOKEN, and can be published even though it renders/copies back to the live token. Normalize the entity introduced by the sanitizer before comparing, and cover a live secret containing an underscore.
const haystack = body.toLowerCase();
scripts/render-triage-comment.mjs:215
- Encoding only
://does not neutralize all links. CommonMark decodes character references in explicit link destinations, so[click](https://evil.example)still renders anhttps://evil.examplelink (andcan render an image); GFM also autolinks barewww.example.com. This violates the guarantee that model fields are prose and cannot link. Neutralize Markdown link delimiters and the bare-wwwform, then add rendering-aware regressions.
s = s.replace(/:\/\//g, '://');
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Markdown links remain actionable, and sanitized GitHub tokens bypass the intended credential-leak refusal signal.
Review details
Suppressed comments (3)
scripts/render-triage-comment.mjs:215
- Encoding only
://does not neutralize Markdown links. GFM decodes character references in link destinations, so model text such as[click](https://evil.example)still renders a live link (andcan render an image), despite the renderer's no-link guarantee. Neutralize Markdown link/image delimiters as well and add a regression that checks rendered GFM rather than only the raw absence of://.
s = s.replace(/:\/\//g, '://');
scripts/render-triage-comment.mjs:257
- The live-secret comparison occurs after sanitization, but step 9 rewrites underscores in
ghs_,ghp_, andgithub_pat_tokens. Consequently a literal job token becomesghs_…; none of the literal/base64/hex forms match, and the shape regex no longer matches either, so the renderer publishes the defanged value instead of refusing and surfacing the intended exfiltration signal. Include the post-sanitization form in the comparison (and cover a GitHub-token-shaped secret in the regression test).
const forms = [
scripts/auto-close-duplicates.mjs:121
- The JSDoc still says this function finds the latest marker comment with a parseable duplicate line, but the new behavior intentionally stops at the latest marker even when it is a retraction. Update the contract so future callers do not reintroduce the superseded-target behavior by relying on the old description.
// The LATEST marker comment IS the current verdict — if it names no
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Two gaps Copilot found in the field sanitizer, both real. The credential "defang" was worse than doing nothing. Entity-encoding the underscore in a credential-shaped match left `ghs_...` as `ghs_...`, which GitHub renders and copies back as the original token - and because it ran before findCredentialLeak, it also hid the value from the exact-value comparison against the job's own secrets. So a live GITHUB_TOKEN in a title would have been published, defeating the check added to catch exactly that. Sanitizing a secret was the wrong instinct: the caller REFUSES to publish a body carrying one, and that only works while the value survives intact. Removed; both the exact-value and shape checks see it again. Escaping `<`/`>` did not cover Markdown structure. CommonMark decodes character references inside an explicit link destination, so `[click](https://evil.example)` still rendered a live link, and `` a tracking beacon under bestaxbot - the cross-reference silence step 7 claims was not delivered. Link and image delimiters are now escaped, backslashes FIRST so that model text of `\[` cannot become `\\[` and reopen the delimiter, and GFM's bare `www.` autolink (a raw scan that decodes no entities) gets its own break. An ordinary `[Refactor] ...` title still reads correctly. Also, two comments that no longer described their mechanism: - findMarkerComment's JSDoc still said it searches back for the latest marker comment with a parseable target. It stops at the latest marker comment either way and returns null for a retraction, which is the whole point of the change - a reader following the old wording would reintroduce the fall-through. - The triage checkout said the job "never runs project code" while the render step executes scripts/render-triage-comment.mjs beside both tokens. The boundary that actually holds is narrower and now stated: the code is pinned to the default branch, so no PR author can substitute their own.
Preview DeploymentPreview URL: https://238e15ec.bestax.pages.dev |
There was a problem hiding this comment.
🔵 Needs a closer look
Auto-close notice transitions can reuse an expired objection clock, and valid astral-character payloads can exceed the stated bound.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
scripts/render-triage-comment.mjs:91
- The 32 KB bound only covers BMP characters. A model-visible astral character such as an emoji can be encoded as two
\uXXXXsurrogate escapes (12 source bytes), so six otherwise compliant title/reason entries can exceed 47 KB andparsePayloadrejects them before parsing, contradicting the documented guarantee. Account for surrogate pairs and add an escaped-emoji boundary test.
export const MAX_PAYLOAD_BYTES = 32_000;
.github/workflows/ai-triage.yml:480
- With full output disabled, the watchdog failure still tells maintainers to “See the session output above,” but that output is no longer published. It also reports one combined list of possible causes rather than the “specific diagnosis” claimed here. Update the failure message to reference diagnostics that are actually available and remove the nonexistent-output instruction.
# The debuggability #317 wanted is now covered without it. "Fail on
# silent session error" reads the execution file and fails the job with
# a specific diagnosis for both known silent-no-op shapes, and the
# render step reports per-command outcomes — so an early-ending session
# is no longer indistinguishable from a healthy one, which was the
scripts/render-triage-comment.mjs:1085
- Treat adding the auto-close notice as a clock-changing transition too. If an old same-target bestaxbot comment was created while auto-close was off, a labeled rerun after enabling it leaves
verdictChangedfalse and PATCHes the notice onto the oldcreated_at; the cron can then close immediately instead of allowing the promised 14-day objection window. POST fresh whenAUTOCLOSE_SENTENCEchanges from absent to present, and include that state in the legacy-identity equality check; add this transition as a regression test.
const previousTarget = existing?.body?.match(/Duplicate of #(\d+)/)?.[1];
const nextTarget = dupe?.[1];
const verdictChanged =
nextTarget !== undefined && previousTarget !== nextTarget;
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Two of three findings from the latest pass; the third is refuted below. Enabling AI_TRIAGE_AUTOCLOSE and re-labelling an item whose comment already named the same target left `verdictChanged` false, so the notice was PATCHed onto a months-old created_at and the cron could close immediately - the promised 14 days having elapsed before the promise was ever made. The notice APPEARING is itself a new obligation, so it now restarts the clock, and the legacy-identity skip compares notice state as well as target. Both directions have tests: a newly-warned comment reposts, an already-warned one still refreshes in place. The watchdog's failure message still told maintainers to "See the session output above" after I stopped publishing it, and the comment justifying that removal claimed a "specific diagnosis" the step does not produce. The message now names the candidate causes and says plainly that the transcript is deliberately unpublished; the comment says what the step actually delivers - a reliable signal plus a short list of causes, not a per-run diagnosis. REFUTED: that astral characters can push a compliant payload past the 32,000 cap. `\uXXXX` costs six bytes per UTF-16 code unit, and MAX_TITLE_CHARS is enforced on `String.length`, which counts an astral character as the two units it is - so it consumes twice the budget for twice the bytes, and the ratio is unchanged. Measured: a payload filled entirely with emoji and one filled entirely with CJK both escape to exactly 23,885 bytes. The 47 KB figure would require an astral character to cost one unit of the limit and twelve bytes of encoding, which `String.length` does not do.
|
Latest Copilot pass handled in 56798aa — two fixed, one refuted. All three were suppressed comments, so replying here rather than on threads. Fixed — the notice appearing is itself a new promise. Enabling Fixed — the watchdog pointed at output I had stopped publishing. It still said "See the session output above" after Refuted — astral characters cannot push a compliant payload past the cap. 540 tests, lint, format and conformance green. |
Preview DeploymentPreview URL: https://45e7324c.bestax.pages.dev |
There was a problem hiding this comment.
🔵 Needs a closer look
Unicode line separators in valid JSON can currently cause sentinel parsing to discard triage results.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/render-triage-comment.mjs:152
.*does not match U+2028/U+2029, although either separator is legal inside a JSON string andsanitizeFieldexplicitly flattens it later. A candidate title/reason containing one can therefore pass the watchdog but make sentinel parsing fail before the payload reaches the sanitizer, dropping the triage result. Make this regex dot-all (the caller has already split physical\nlines) and add a separator-bearing payload case.
export const SENTINEL_RE =
/^TRIAGE-RESULT: ([a-z][a-z-]{0,39}) (publish|skip)(?: (.*))?$/;
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced
U+2028 and U+2029 are legal unescaped inside a JSON string and JSON.stringify does not escape them, but JS `.` excludes them alongside \n and \r. So a candidate title carrying one produced a payload SENTINEL_RE could not match: parseSentinels returned null, the run failed closed, and nothing published - while the watchdog passed the line happily, because it splits on \n only. One invisible character in an issue title was therefore a denial of service against every triage run citing that issue, in the same class as the `#@2x` splice. The regex is dot-all now. That costs no strictness: the caller has already split the result into physical \n lines, and the captured payload is JSON-parsed and schema-validated immediately after. A separator that survives into a field is flattened by sanitizeField, which is where it was always meant to be handled - the bug was that parsing never got that far. Making it dot-all also swallowed a trailing CR, so that is now handled deliberately rather than as a side effect: a lone trailing CR is stripped before matching, because the watchdog splits on \n and a CRLF transcript leaves one on every line. Rejecting it failed an entire run over a line ending, while the content rules - leading space, a missing or doubled space, an unknown verb - all still fail closed.
|
Fixed in f84c80e — legit finding, and the sharpest one in a while. Verified end to end before changing anything: Dot-all now, and it costs no strictness — the caller has already split the result into physical One consequence worth naming rather than leaving as a side effect: dot-all also made Regression tests cover both separators through extraction and render, asserting the fixture is valid JSON first so it cannot pass vacuously, plus the CRLF case. 542 tests, lint, format and conformance green. |
Preview DeploymentPreview URL: https://9e679844.bestax.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
Model-authored prose still reaches PAT-authored comments, so the claimed structural re-trigger guarantee remains incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
| kept.push({ | ||
| number: entry.number, | ||
| title, | ||
| reason: sanitizeField(entry.reason, MAX_REASON_CHARS), | ||
| }); |
There was a problem hiding this comment.
Not taking the change, but the observation is correct and I am not disputing it — so let me separate the two.
The facts hold. I measured rather than argued: /retest, /deploy, rebase and LGTM all survive sanitizeField verbatim into the PAT-authored body. The defanging covers the vectors we enumerated — mentions, markers, autolinks, machine sentinels — and by construction it cannot cover a trigger token nobody has invented yet. Your conclusion follows: for that residual class rule 8's sender exclusions are still THE layer, not a second one.
And the claim you were reading was wrong. That is fixed in ab8460c, because it was worth fixing on its own. .github/CLAUDE.md said the renderer "builds the body from renderer-owned constants and validated integers" and, in the same paragraph, that "publishing model prose under a PAT is not" acceptable — while the body carries two model-authored fields. Self-contradicting, in the file CodeRabbit and the @claude action read as project instructions. Both that file and the workflow header now state what is actually delivered and what is not, and explicitly tell the next reader not to describe the triage path as closing I2 by construction.
What we are declining, and why. The remedy — a prose-free body — is a product decision, not a defect fix: it trades away the one-line "why" that is the reason a human reads a triage comment at all. The maintainer weighed it and chose to keep the prose for now, so the residual is accepted knowingly rather than by oversight. It is recorded in both documents as a deliberate, revisitable trade, with the two concrete shapes it could take (bare #N references, or a renderer-owned enum in place of the free-text reason) written down so the option is not lost.
For scope: what this PR does deliver structurally is that every structural string is renderer-owned and every issue reference is a validated integer, so a payload cannot forge a marker, a duplicate verdict, or an auto-close promise. That is narrower than "I2 by construction", and the docs now say so.
The finding behind this is correct on the facts and the claim was mine: I wrote that the renderer "builds the body from renderer-owned constants and validated integers" and that "publishing model prose under a PAT is not" acceptable, in the same paragraph, while the body carries two model-authored fields. That is self-contradicting, and it sits in the file CodeRabbit and the @claude action read as project instructions. Measured, not argued: `/retest`, `/deploy`, `rebase` and `LGTM` all survive sanitizeField verbatim into a PAT-authored comment. The defanging covers the vectors we enumerated - mentions, markers, autolinks, machine sentinels - and cannot cover a trigger token nobody has invented yet. So for that residual class rule 8's sender exclusions are still THE layer, not a second one. Both documents now say that plainly, and say what IS structural: every structural string is renderer-owned and every issue reference is a validated integer, so a payload cannot forge a marker, a duplicate verdict or an auto-close promise. Publishing the two prose fields is recorded as a deliberate, revisitable trade rather than an oversight - a prose-free body would make this structural too, at the cost of the signal a human reads the comment for. No behavior change.
Preview DeploymentPreview URL: https://656ee8d5.bestax.pages.dev |
There was a problem hiding this comment.
🟡 Changes recommended
Secondary rate-limit handling remains incomplete, and public documentation overstates the sanitization guarantee.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/render-triage-comment.mjs:706
- Secondary-limit 403 responses are not guaranteed to carry either header checked here:
x-ratelimit-remainingcan remain nonzero and the response message can be the only rate-limit evidence. This path then fails immediately and cleanup consumes the triage label, recreating the lost-result case this retry loop is meant to prevent. Distinguish permission failures from secondary-limit response bodies and use GitHub's documented backoff (at least one minute whenRetry-Afteris absent), with tests for both 403 shapes.
const rateLimited =
res.status === 429 ||
(res.status === 403 &&
(retryAfterMs(res.headers) !== null ||
res.headers.get('x-ratelimit-remaining') === '0'));
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced
| validated issue numbers; the candidate titles and one-line reasons are still the session's | ||
| words, escaped and stripped of anything that could mention, link or re-trigger. |
|
🎉 This PR is included in version 5.11.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.1.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.2.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Fixes #457.
The triage session used to build its own comment bodies and post them with
gh issue commentunder bestaxbot's PAT. PAT-authored comments do emitworkflow events (GITHUB_TOKEN ones do not), so model free text was reaching a
re-trigger-capable identity — invariant I2, held up only by every
comment-triggered workflow remembering to exclude bestaxbot. That is defense by
enumeration; this replaces it with a structural guarantee.
The session now emits a structured payload on its existing
TRIAGE-RESULT:sentinel line.
scripts/render-triage-comment.mjsvalidates it, sanitizes thetwo free-text fields, renders the body from renderer-owned constants and
validated integers, and upserts it scoped to the marker it owns. bestaxbot is
still the author — #361's point stands — but the body is no longer model prose.
@allxsmith/bestax-bulma)create-bestax)@allxsmith/bestax-docs)Security review checklist
Does any allowlist grow? No — it strictly narrows.
Bash(gh pr comment:*)and
Bash(gh issue comment:*)are removed and nothing is added. The credential inthat job also changes: it was
AI_LOOP_PAT— full repo write, unscoped by thejob's
permissions:block — and is now the jobGITHUB_TOKENscopedcontents/issues/pull-requests: read. Both directions are the ones rule 2asks for.
Why the four-job split. The Claude action's
github_tokenbecomes thesession's
GH_TOKEN, so the PAT was not merely co-resident — it was thesession's credential. In a single job there were only three options: keep the
full-write PAT, swap to
GITHUB_TOKEN(job-scoped, andgateneedsissues: writefor the budget marker — i.e. reproduce #455 in a secondworkflow), or mint a new read-only secret. The split delivers a genuinely
read-only session credential using secrets the repo already has, per rule 2's
"prefer removing the need for the boundary over hardening it".
A live exfiltration channel is closed.
Readis always available and is notconfined to the workspace (
/proc/self/environ,~/.claude/.credentials.json),and the allowlist's prefix match accepted
gh issue comment N --body "$CLAUDE_CODE_OAUTH_TOKEN". The sink is gone, andthe render step now compares the finished body against both of that job's
secrets in literal, base64 and hex form. Stated precisely: this required a
prompt-injected session to exercise, there is no evidence it ever was, and the
credential check is a backstop that raises the cost of exfiltration — not a
proof. The tool restriction remains the control.
Does model or issue text reach a comment body? Yes, bounded: candidate
titles and one-line reasons, escaped and stripped of every autolink vector
(mentions, issue URLs,
GH-123,#N), capped, and validated byassertRenderedInvariants— which holds even if a defang rule is laterweakened. Everything structural is renderer-owned.
Posting identity: still the PAT (bestaxbot), deliberately. #457 rejects
reverting to
github-actions[bot]. The bestaxbot sender exclusions in otherworkflows are untouched — rule 8 still stands; they are now a second layer
rather than the only one.
New repository variable? None. Action SHAs: no new actions; all four
pins remain single-valued repo-wide.
Highest-risk line in the diff:
cleanup'sif:. Label removal used to be astep, so it never ran when the job-level
ifwas false; as a job under a barealways()it would newly fire on fork PRs, where the DELETE 403s. Theneeds.gate.result != 'skipped'term is what reproduces the old behavior. Pleasecheck that expression specifically.
Expected on this PR
The
ai-triagerun will fail here if anyone labels it — self-inflicted andexpected. Per rule 9's bootstrap paragraph, this workflow checks out the
default branch, never PR head, so
scripts/render-triage-comment.mjsdoes notexist in the workspace its own run reads. The render step exits
MODULE_NOT_FOUNDand nothing is published, which is the correct direction. The
Fixes #457lineabove also makes the auto path self-skip, so the practical exposure is a manual
label only. Do not work around it by checking out PR head, and do not add a
fallback that posts unrendered text.
Behavior change to watch: the session's reads are now attributed to
github-actionsrather than bestaxbot, so they draw on the 1,000/hr/repo RESTbudget instead of 5,000/hr/user. A run does roughly 30 searches plus ~20 views,
which fits, but it is the thing that could surprise under the global concurrency
group when runs queue.
Known, recorded, not fixed here
a burst silently loses the items in the middle — cancelled, not skipped, so no
gate log and no budget charge explains the gap.
cancel-in-progress: falsenever protected the queue, and the split widens the window. The comment is
corrected rather than compounded; the real fix is making the budget counter
safe under concurrency, since re-scoping the group per item would reintroduce
the counter race. Worth its own issue.
because the numbers are derived from the same body being scanned. What is live
across that hop — marker, markup, mentions, sentinels, size — is real, and the
comment now says exactly that instead of overclaiming.
Type of Change
Checklist
scripts/render-triage-comment.test.mjs, 65 cases: the fail-closed matrix, a no-coercion table, golden bodies pinned to published comments, and an injection suite that runs the realauto-close-duplicates.mjsconsumer over rendered output so the two files cannot driftpnpm test(522 root-script cases),eslint,prettier --checkandcheck:conformanceare green