ci: split the Claude PR-severity and issue-dedupe workflows into two jobs#10955
ci: split the Claude PR-severity and issue-dedupe workflows into two jobs#10955Roasbeef wants to merge 2 commits into
Conversation
🟢 PR Severity: LOW
🟢 Low (2 files)
AnalysisThis PR only touches files under No severity bump applies: only 2 files changed (well under the 20-file threshold) and 317 lines changed (well under the 500-line threshold), and no critical packages are touched. No override label or prior classification was found for this PR. To override, add a |
Roasbeef
requested review from
a team,
bhandras and
ellemouton
and removed request for
a team
|
/gateway review |
|
✅ Review posted: #10955 (review) 6 finding(s); 6 inline, 0 in body. 🔁 Need a re-review after pushing changes? Reply with |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR splits both Claude-driven CI workflows into a read-only, model-facing job that records a verdict to files and a model-free, write-scoped job that consumes those files and mutates GitHub. The privilege-separation intent is sound and the mechanics are mostly right — the severity.txt allowlist, --body-file instead of shell interpolation, persist-credentials: false, and dropping id-token are all correct hardening.
Two issues dominate. First, both workflows pass data between jobs via upload-artifact/download-artifact with if-no-files-found: warn, which makes the downstream "no verdict produced" guards dead code: if the model writes no file at all, download-artifact@v4 errors before the guard runs. Second, the separation leaks through comment.md — the read-only job runs the model against fully attacker-controlled fork-PR input and the privileged job posts that content verbatim with no validation, so a prompt injection can dictate the bot's comment body. The remaining findings are smaller consistency and defense-in-depth gaps.
Findings: 🔴 0 Blocker · 🟠 3 Major · 🟡 3 Minor · 🔵 0 Nit
| persist-credentials: false | ||
|
|
||
| - name: Download duplicate candidates | ||
| uses: actions/download-artifact@v4 |
There was a problem hiding this comment.
🟠 Major F1: download-artifact hard-fails when the model writes no file, defeating the no-op guard
find-duplicates uploads duplicates.txt with if-no-files-found: warn (line 95), so when the model produces no file at all — a routine LLM failure mode (timeout, refusal, not following the "write an empty duplicates.txt and stop" instruction) — no dedupe-result artifact is created. actions/download-artifact@v4 here then errors with "Unable to find any artifacts" and fails the job before the downstream if [[ ! -s result/duplicates.txt ]]; then ... exit 0 guard (line 129) can run. That guard was written to handle exactly the missing/empty case but is unreachable for the missing-file case. The empty-file case does work (an empty file still creates the artifact). Make the download tolerant of a missing artifact (continue-on-error: true plus a preceding mkdir -p result, or guarantee the model job always writes the file) so the existing guard is reachable.
| pull-requests: write | ||
| steps: | ||
| - name: Download classification result | ||
| uses: actions/download-artifact@v4 |
There was a problem hiding this comment.
🟠 Major F2: download-artifact hard-fails when classify produces no severity.txt
Same artifact-contract cliff as F1. classify uploads with if-no-files-found: warn (line 231), so if the model writes none of severity.txt/should_comment.txt/comment.md (model timeout, refusal, or a future maintainer tightening --allowedTools and dropping Write), no pr-severity-result artifact exists and download-artifact@v4 fails the apply job. The intended fail-safe if [[ ! -f result/severity.txt ]]; then ... exit 0 (line 259) never executes — the job errors instead of degrading quietly. Give the download step tolerance for a missing artifact so the -f guard actually handles the no-verdict path.
| # the body is treated purely as data. | ||
| if [[ "$SHOULD_COMMENT" == "true" && -f result/comment.md ]]; then | ||
| gh pr comment "$PR_NUMBER" --repo "$REPO" \ | ||
| --body-file result/comment.md |
There was a problem hiding this comment.
🟠 Major F3: Unvalidated comment.md lets prompt injection dictate the bot's PR comment
The privilege split leaks through comment.md. The read-only classify job runs the model against fully attacker-controlled input — gh pr view on a fork PR reads the attacker's title/body — and the privileged apply job (holding pull-requests: write) validates severity.txt against critical|high|medium|low and should_comment.txt against true, but performs zero validation on the comment body: gh pr comment "$PR_NUMBER" --repo "$REPO" --body-file result/comment.md. A prompt injection in the PR content can therefore make the model write arbitrary bytes into comment.md and true into should_comment.txt, and an untrusted fork author gets the trusted project bot to post a comment with attacker-chosen content — fake maintainer instructions, phishing links, @mention notification spam — under the bot's identity. --body-file prevents shell/command injection (content is sent as a literal body), which is why this is not a blocker, but the read-only job fully controls a write-scoped action's payload, defeating the stated separation. Constrain the comment: have the model emit only structured fields the apply job renders from a fixed template, or validate/sanitize comment.md (length cap, strip @-mentions and HTML, reject content not matching the expected shape) before posting.
| fi | ||
|
|
||
| ./scripts/comment-on-duplicates.sh \ | ||
| --base-issue "$BASE_ISSUE" \ |
There was a problem hiding this comment.
🟡 Minor F4: BASE_ISSUE passed to the script without the numeric validation applied to DUPS
The duplicate ids are filtered through grep -oE '^[0-9]+$', but BASE_ISSUE (${{ github.event.issue.number || inputs.issue_number }}) is passed straight to comment-on-duplicates.sh --base-issue "$BASE_ISSUE" with no validation. For the workflow_dispatch path inputs.issue_number is a free-form string, so a non-numeric dispatch value flows unchecked into the script. It is quoted, so this is not shell injection, but the inconsistency means a bad base issue fails deep in the script rather than being rejected up front. Validate BASE_ISSUE against ^[0-9]+$ alongside the DUPS check.
|
|
||
| ./scripts/comment-on-duplicates.sh \ | ||
| --base-issue "$BASE_ISSUE" \ | ||
| --potential-duplicates "${DUPS[@]}" |
There was a problem hiding this comment.
🟡 Minor F5: Dedupe model output selects which issues get cross-referenced (notification spam)
The post-comment job correctly filters duplicates.txt to digits-only and passes the ids as array args, so there is no argument injection. However the selection of issue numbers is model output and thus prompt-injectable: an issue author can inject instructions steering the dedupe bot to reference arbitrary issue numbers, and each reference the bot posts creates a GitHub cross-link that notifies the target issues' participants. This is a low-amplitude notification/cross-reference spam vector (bounded to 3 per opened issue), not a fund or data risk. Defense-in-depth: confirm each candidate is a genuine search hit before referencing it, rather than trusting the model's chosen numbers.
| # Allow any user: this job only reads PR metadata via the API with a | ||
| # read-only token and writes its verdict to a file. The privileged | ||
| # label/comment step runs separately without the model. | ||
| allowed_non_write_users: "*" |
There was a problem hiding this comment.
🟡 Minor F6: *allowed_non_write_users: *
allowed_non_write_users: "*" (present in both workflows) disables claude-code-action's author-association gate, feeding untrusted fork/first-time-contributor content to the model unconditionally. For a classifier that must run on every PR this is intentional and is bounded today by the read-only github_token scope and the tool allowlist. But under pull_request_target it is a "trust everyone" default sitting one edit away from danger: a future maintainer who adds a mutating Bash(...) tool or raises the classify job's permissions grants arbitrary fork authors that power without touching this line. Prefer an explicit allowlist, or at minimum a comment marking "*" as safe only while the model job stays read-only.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
Roasbeef
force-pushed
the
ci/workflow-job-split
branch
from
c60c990 to
3565a16
Compare
|
/gateway re-review |
|
✅ Re-review posted: #10955 (review) Prior findings: 4 addressed, 2 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
Solid pass on the prior review: F1 and F2 (the download-artifact cliffs) are fixed with continue-on-error: true, F4 (base-issue validation) and F6 (the "*" safety comment) are fully addressed, and F3's core redesign — model writes structured fields, a deterministic template renderer posts via --body-file — is exactly the shape requested and closes the shell/command-injection and raw-markdown-control vectors.
Two priors are only partially resolved. F3's new clean() sanitizer defangs @ and escapes &<> but leaves # cross-references and markdown link/image syntax live, and the files.tsv reason field can still smuggle a newline — so an attacker-steered classification can still spam notifications and embed tracking/phishing links in a bot-authored comment, just less freely than before. F5's candidate-verification loop drops hallucinated issue numbers but a prompt injection can still steer the model to real, unrelated issue numbers that pass verification.
The redesign also introduces one genuinely-new blocker-adjacent defect (F7): the truncating clean() pipeline can abort the apply job under set -euo pipefail on large input — after the labels have already been edited but before the comment posts. Verdict is REQUEST_CHANGES on F3 (partial, major) and F7 (new, major).
Findings: 🔴 0 Blocker · 🟠 2 Major · 🟡 5 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Fixed in
.github/workflows/claude-dedupe-issues.yml— theDownload duplicate candidatesstep now setscontinue-on-error: true, so a missingdedupe-resultartifact no longer hard-fails the job and the[[ ! -s result/duplicates.txt ]]guard is reachable. - F2 addressed: Fixed in
.github/workflows/pr-severity.yml— theDownload classification resultstep setscontinue-on-error: true, so the[[ ! -f result/severity.txt ]]fail-safe in theapplyjob now executes on the no-verdict path instead of the download erroring first. - F4 addressed: Fixed in
.github/workflows/claude-dedupe-issues.yml—BASE_ISSUEis now validated with[[ "$BASE_ISSUE" =~ ^[0-9]+$ ]](exit 1 otherwise) before being passed tocomment-on-duplicates.sh, matching the numeric check on the candidate ids. - F6 addressed: Fixed in both workflows — the
allowed_non_write_users: "*"lines now carry the requested comment marking"*"safe only while the job stays read-only and warning against adding a write token or mutating tool without replacing it with an allowlist.
| printf '\n</details>\n\n' >> "$OUT" | ||
| done | ||
|
|
||
| if [[ -f result/analysis.txt ]]; then |
There was a problem hiding this comment.
🟠 Major F7: Truncating clean() pipeline can abort apply job under pipefail after labels are edited
clean() is tr … | sed … | head -c "$1". Under set -euo pipefail, when the input exceeds the byte cap, head -c closes the pipe after reading its quota and the upstream sed/tr die on SIGPIPE (exit 141), which pipefail propagates as the pipeline's status. In clean 4000 < result/analysis.txt >> "$OUT" (and the CLEAN_REASON=$(… | clean 300) substitutions), that non-zero status trips set -e and aborts the apply job — which fires precisely on the oversized input the truncation exists to defend against (an attacker-steered analysis.txt larger than the pipe buffer). The abort happens after gh pr edit --add-label has already churned the labels but before gh pr comment, so the PR is left relabeled with no comment and a red job. The authors already guarded the sibling grep -c pipelines with || true but missed these. Append || true to the truncating pipelines (partial stdout is still the desired truncated output) or truncate without a pipe.
| - name: Download classification result | ||
| uses: actions/download-artifact@v4 | ||
| # classify uploads with if-no-files-found: warn, so if the model writes | ||
| # no verdict at all (timeout, refusal) no artifact exists and |
There was a problem hiding this comment.
🟡 Minor F8: warn + continue-on-error + exit 0 makes a crashed classifier indistinguishable from a no-op
The F1/F2 fix (if-no-files-found: warn on upload + continue-on-error: true on download + [[ ! -f result/severity.txt ]]; exit 0) makes the pipeline fail open: a classify job that crashed before writing any output produces the same green, no-label, no-comment outcome as a deliberate "nothing to do" decision. Broken model runs are silently laundered into success and PRs go unlabeled with no signal. Consider having classify always write an explicit sentinel (e.g. severity.txt present with should_comment.txt=false) so the apply guard can distinguish "model chose not to act" from "model produced nothing" and surface the latter as a notice/failure.
| # The classify job runs the model on untrusted PR text, so treat every | ||
| # field it produced as tainted. This step owns the comment's entire | ||
| # structure and sanitizes each interpolated field: control chars are | ||
| # stripped, HTML metacharacters are escaped, and @-mentions are |
There was a problem hiding this comment.
🟡 Minor F9: Unguarded add-label after removing all severity labels can leave PR unlabeled
The apply job removes every severity-* label (each --remove-label … 2>/dev/null || true) and then runs an unguarded gh pr edit … --add-label "severity-$SEVERITY". gh errors if the label does not already exist in the repo (or on a transient API failure); under set -e that aborts the job after the old label was removed, leaving the PR with no severity label and no comment. Add the new label before removing the others, or guard the add.
| } | ||
|
|
||
| UPPER_SEV="$(printf '%s' "$SEVERITY" | tr '[:lower:]' '[:upper:]')" | ||
| FILE_COUNT="$(grep -c . result/files.tsv || true)" |
There was a problem hiding this comment.
🟡 Minor F10: Displayed file count counts unrendered rows
FILE_COUNT="$(grep -c . result/files.tsv || true)" counts every non-empty line, but the renderer only iterates the fixed tiers critical high medium low. A row whose first field is anything else (model typo, stray line, a phantom row forged via a newline in <reason> — see F3) inflates the "%s files" header without appearing in any <details> block, so the stated count can exceed the bullets shown. Compute the header count from the same tier-validated row set the renderer emits.
| for tier in critical high medium low; do | ||
| COUNT="$(awk -F'\t' -v t="$tier" '$1==t' result/files.tsv \ | ||
| | grep -c . || true)" | ||
| [[ "$COUNT" -gt 0 ]] || continue |
There was a problem hiding this comment.
🟡 Minor F11: Final files.tsv row dropped when the file lacks a trailing newline
while IFS=$'\t' read -r f_tier f_path f_reason; do … done < result/files.tsv skips the last record if the model writes files.tsv without a trailing newline: read returns non-zero at EOF and the loop body is not run for that line, even though the variables were populated. The awk/grep -c counts still include it, so a tier whose only entry is the unterminated last line renders an empty <details> section and the header count won't match the bullets. Guard with while … read -r … || [[ -n "$f_tier" ]].
| critical) printf 'Critical' ;; | ||
| high) printf 'High' ;; | ||
| medium) printf 'Medium' ;; | ||
| low) printf 'Low' ;; |
There was a problem hiding this comment.
🟠 Major F3 · partially_addressed
The structural fix requested by F3 landed: the classifier now emits only structured fields (severity.txt/should_comment.txt/files.tsv/line_count.txt/analysis.txt), the apply job owns the entire comment structure via a fixed template, and it posts with --body-file. That eliminates command injection and raw-model-markdown control. What remains: the clean() sanitizer defangs @ and HTML-escapes &<>, but does not neutralize (a) #N cross-references — a #-laden analysis.txt (up to 4000 bytes of attacker-influenced text) still generates cross-reference notifications on arbitrary issues/PRs; (b) markdown link/image syntax — [x](https://evil) /  render live in the bot-authored comment (phishing + tracking-beacon surface); and (c) a literal newline inside the files.tsv <reason> field, which is split by while IFS=$'\t' read before clean() runs and forges an entire extra file row. Under pull_request_target + allowed_non_write_users:"*" any fork author reaches this. Defang # like @, escape markdown link/image metacharacters, and strip \t/\n/\r from <reason> at write time (or reject rows whose TAB-field-count ≠ 3).
| echo "No valid numeric duplicate ids; nothing to post." | ||
| exit 0 | ||
| fi | ||
|
|
There was a problem hiding this comment.
🟡 Minor F5 · partially_addressed
The new VERIFIED loop confirms each candidate resolves to a real issue via gh issue view and drops unknown ids — good defense-in-depth against hallucinated numbers. But F5's concern was the selection being prompt-injectable: an issue author can still steer the model toward real, unrelated issue numbers (#1, #2, #3), which pass the existence check and get cross-referenced, notifying those issues' participants. Existence-verification closes the "nonexistent number" sub-case only; the bounded (≤3) notification-spam vector on genuine issues survives. To fully address, confirm each candidate was an actual search hit for this issue, not merely that it exists.
|
/gateway re-review |
Roasbeef
force-pushed
the
ci/workflow-job-split
branch
from
3565a16 to
5336dff
Compare
|
✅ Re-review posted: #10955 (review) Prior findings: 10 addressed, 1 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
github-actions
Bot
added
severity-medium
🟡 PR Severity: MEDIUM
🟢 Low (2 files)
AnalysisBoth changed files fall under However, the total lines changed (456 additions + 91 deletions = 547 lines) exceeds the 500-line threshold for a severity bump (none of the changed lines belong to test files or auto-generated files, so all lines count toward this total). Per the classification rules, this triggers a one-level bump from LOW to MEDIUM. No override labels are present, and no critical/high packages are touched, so MEDIUM is the final severity. This warrants a focused review of the workflow logic changes, even though no application code is affected. To override, add a |
There was a problem hiding this comment.
This revision resolves nearly all outstanding findings. The two artifact-contract cliffs (F1, F2) are closed with continue-on-error on the downloads; the comment-injection concern (F3) is properly fixed by having the model emit only structured fields that the model-free apply job renders from a fixed template with per-field sanitization; the clean() SIGPIPE-under-pipefail bug (F7) is gone now that truncation is a bash substring rather than a head -c pipe; the label-ordering hazard (F9) is fixed by adding before removing; and the three rendering bugs (F8 signal, F10 count, F11 trailing-record) are all addressed.
One prior finding remains partially addressed: F5's existence check drops hallucinated issue numbers but the selection is still model output, so a prompt injection can steer the bot toward any real issue.
Two new minor, defense-in-depth items surfaced, both on the classify/apply split in pr-severity.yml; neither is a regression (this PR strictly narrows the prior single-job attack surface). Verdict is COMMENT.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 3 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Fixed —
post-comment'sDownload duplicate candidatesstep now setscontinue-on-error: true, so a missingdedupe-resultartifact no longer hard-fails the job and the[[ ! -s result/duplicates.txt ]]; exit 0guard is reachable. - F2 addressed: Fixed —
apply'sDownload classification resultstep now setscontinue-on-error: true; the[[ ! -f result/severity.txt ]]guard now executes and exits 0 with a warning instead of the job erroring on a missing artifact. - F3 addressed: Fixed — the
classifymodel now writes only structured fields (severity.txt,should_comment.txt,files.tsv,line_count.txt,analysis.txt) and the model-freeapplyjob owns the comment's entire structure: it validatesseverityagainst thecritical|high|medium|lowenum, tier-validates the file rows, and passes every untrusted field throughclean()(C0-control strip,&<>HTML-escape,[]escape,@-defang via zero-width joiner,http(s)://→hxxp://, length cap) before templating and posting via--body-file. The read-only job no longer controls the write-scoped payload's shape — exactly the fix requested. - F4 addressed: Fixed —
post-commentnow validatesBASE_ISSUEagainst^[0-9]+$up front and exits 1 on a non-numeric value, matching the candidate-id check. - F6 addressed: Fixed —
allowed_non_write_users: "*"now carries an explicit comment in both workflows marking it safe only while the model job stays read-only, and naming the exact edits (a write token or a mutating tool) that would require replacing"*"with an allowlist. - F7 addressed: Fixed —
clean()no longer ends in| head -c "$1". It reads the full sanitized input intooutand truncates with the bash substring${out:0:$1}, so an oversized field can no longer SIGPIPE the upstreamtr/sedand abort theapplyjob underpipefail. - F8 addressed: Fixed — a missing
result/severity.txtnow emits::warning::PR severity classifier produced no verdict; PR left unlabeled.and exits 0, so a no-output run surfaces as a distinct annotation rather than an indistinguishable silent green. The maintainer deliberately warns rather than fails since classification is advisory, which is a valid resolution of the fail-open concern. - F9 addressed: Fixed —
applynow runs--add-label "severity-$SEVERITY"before the--remove-labelloop. A failed add aborts underset -ewith the PR still carrying its previous severity label rather than being left with none. - F10 addressed: Fixed —
FILE_COUNTis now computed from the tier-validated row set (awkfilter oncritical|high|medium|low), the same set the renderer emits, so a bogus/mistyped tier or a phantom row can no longer inflate the header past the bullets shown. - F11 addressed: Fixed — the per-tier render loop is now
while IFS=$'\t' read -r f_tier f_path f_reason || [[ -n "$f_tier" ]], so a finalfiles.tsvrecord without a trailing newline is still emitted and the counts match the bullets.
| fi | ||
|
|
||
| if [[ ! -f result/files.tsv ]]; then | ||
| echo "should_comment is true but files.tsv is missing; skipping comment." >&2 |
There was a problem hiding this comment.
🟡 Minor F13: cancel-in-progress can interrupt the label swap and leave two severity labels
The F9 fix (add the new label before removing the others) combined with the pre-existing workflow-level concurrency: … cancel-in-progress: true opens a small window: on a rapid synchronize/labeled sequence, if a new run cancels the in-flight apply job between the --add-label and the completion of the --remove-label loop, the PR is left carrying two severity-* labels. It self-heals on the next run (which adds the current and removes the rest), so impact is low, but any downstream consumer assuming exactly one severity label reads an ambiguous state in the interim. Consider excluding apply from cancel-in-progress, or computing the desired label set and applying it as one reconciliation.
Roasbeef
force-pushed
the
ci/workflow-job-split
branch
from
5336dff to
3806ebe
Compare
|
/gateway dismiss F5 F12 F5: accepted defense-in-depth residual. The F12: accepted residual, not a regression — this PR strictly narrows the prior single-job form (which held a write token plus |
|
🚫 Dismissed F5 (minor) by @Roasbeef — F12 Open findings on this PR: 🟡 F12 (minor) · 🟡 F13 (minor) |
|
/gateway dismiss F5 per #10955 (comment) |
|
/gateway dismiss F12 per #10955 (comment) |
|
ℹ️ |
|
🚫 Dismissed F12 (minor) by @saubyk — per #10955 (comment) Open findings on this PR: 🟡 F13 (minor) |
Roasbeef
force-pushed
the
ci/workflow-job-split
branch
from
3806ebe to
e82ddd3
Compare
Roasbeef
force-pushed
the
ci/workflow-job-split
branch
from
4295334 to
7499871
Compare
|
/gateway dismiss F4 F23 F4: redundant. F23: not applicable. |
|
🚫 Dismissed F4 (minor) by @Roasbeef — F23 Open findings on this PR: 🟡 F19 (minor) · 🟡 F20 (minor) · 🟡 F21 (minor) · 🟡 F22 (minor) · 🟡 F23 (minor) |
github-actions
Bot
added
severity-medium
🟡 PR Severity: MEDIUM
🟢 Low (4 files)
AnalysisEvery changed file falls under However, the total diff is 480 additions + 66 deletions = 546 lines changed. No critical/high packages are involved, and the file count (4) is well under the 20-file bump threshold, so this bump is driven solely by line volume. To override, add a |
|
/gateway dismiss F23 per #10955 (comment) |
|
🚫 Dismissed F23 (minor) by @saubyk — per #10955 (comment) Open findings on this PR: 🟡 F19 (minor) · 🟡 F20 (minor) · 🟡 F21 (minor) · 🟡 F22 (minor) |
|
/gateway re-review |
|
✅ Re-review posted: #10955 (review) Prior findings: 17 addressed, 1 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
This round closes out the last open threads from prior review. The apply-side shell was extracted into scripts/apply-pr-severity.sh with a unit test (closing F21), the comment size cap now measures the sanitized body (F22), the #N defang landed (F18), and the dedupe post job now warns rather than silently no-ops on a missing artifact (F19). The claude-code-action reference is SHA-pinned in both workflows (F20's core ask). The privilege split itself is sound: the model jobs hold read-only tokens, write scope lives only in the model-free apply/post jobs, artifacts are same-run scoped, and both mutating jobs re-validate their inputs.
One prior finding is only partially addressed: F20's supply-chain concern — actions/checkout, actions/upload-artifact, and actions/download-artifact are still on mutable @v4 tags. The genuinely-new items are all minor robustness/consistency gaps: a gh pr view read failure can leave two severity labels despite the single-edit design; the classify step omits the env: GH_TOKEN its sibling dedupe job sets; the apply script guards two of its three documented env vars; neither pr-severity job sets a timeout-minutes its sibling does; and the new test covers only sanitize_comment, not main's branching. Nothing here carries fund, data, or write-scope risk.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 6 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed:
post-commentnow downloads withcontinue-on-error: trueand guards[[ ! -f result/duplicates.txt ]], so the missing-artifact path is reachable. - F2 addressed:
applydownloads withcontinue-on-error: trueandscripts/apply-pr-severity.shguards[[ ! -f "$result_dir/severity.txt" ]]; the no-verdict path degrades instead of hard-failing. - F6 addressed:
allowed_non_write_users: "*"now carries the inline comment marking it safe only while the model job stays read-only, in both workflows. - F7 addressed: The inline
clean()tr | sed | head -cpipeline is gone; truncation is now a post-sanitizationwc -cbyte check inapply-pr-severity.sh, so theset -o pipefailSIGPIPE abort no longer exists. - F8 addressed:
apply-pr-severity.sh:54emits::warning::PR severity classifier produced no verdicton a missingseverity.txt, distinguishing a broken run from a healthy no-op. The maintainer accepted warn-not-fail as intentional (classification is advisory). - F9 addressed: Label reconciliation is now a single
gh pr editcombining--add-labelwith the--remove-labelset, so a failed add no longer leaves the PR unlabeled. - F10 addressed: The header-count-vs-bullets mismatch is moot: the comment body is now fully model-authored (
comment.md), and the shell no longer renders tiers from afiles.tsv. - F11 addressed: Same as F10 — the
while readloop overfiles.tsvwas removed with the shell-side rendering. - F13 addressed: The single-edit reconciliation (F9) also closes the cancel-in-progress two-label window: a cancelled run cannot stop between a separate add and remove.
- F14 addressed:
severityis lowercased on read (tr '[:upper:]' '[:lower:]',apply-pr-severity.sh:63), so a strayLow/HIGHmatches rather than failing red. - F15 addressed: The comment gate now uses
! -s "$result_dir/comment.md"(non-empty), so a 0-byte body no longer reachesgh pr comment --body-file. - F16 addressed:
claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}is restored on thefind-duplicatesstep. - F17 addressed:
claude-dedupe-issues.ymlnow declares aconcurrencygroup keyed on the issue number withcancel-in-progress, mirroring pr-severity. - F18 addressed:
sanitize_commentincludess/#([0-9])/#${z}\1/g, defanging bare#Ncross-references; the test asserts#123andrepo#4567are defanged. - F19 addressed:
post-commentnow emits::warning::dedupe find-duplicates produced no resultwhenresult/duplicates.txtis absent, reserving the silentexit 0for the present-but-empty case — parity with pr-severity's apply. - F21 addressed: The ~100 lines of security-relevant apply-side shell were extracted into the checked-in
scripts/apply-pr-severity.sh, invoked as./scripts/apply-pr-severity.sh result, withscripts/apply-pr-severity_test.shcovering the sanitizer. (See new finding F28 for a coverage gap in that test.) - F22 addressed: The size cap is now measured on the sanitized body —
sanitize_commentwritescomment.sanitized.md, thenwc -c < "$result_dir/comment.sanitized.md"is compared tomax_bytes, with an inline comment explaining why post-sanitization is the correct measurement point.
| @@ -20,6 +23,10 @@ jobs: | |||
| classify: | |||
| name: Classify PR Severity | |||
| runs-on: ubuntu-latest | |||
There was a problem hiding this comment.
🟡 Minor F24: pr-severity jobs set no timeout-minutes; sibling dedupe does
Neither classify nor apply sets timeout-minutes, whereas the sibling claude-dedupe-issues.yml caps find-duplicates at 10m and post-comment at 5m. classify runs the model on attacker-controlled PR text and fires on every pull_request_target synchronize; concurrency: cancel-in-progress only serializes within one PR's group, so a fork author pushing across many PRs can spawn parallel model runs bounded only by GitHub's 6h default — cheap input driving paid CLAUDE_CODE_OAUTH_TOKEN and runner-minute consumption. Add a short timeout-minutes to both jobs to match the dedupe workflow.
| uses: anthropics/claude-code-action@ba0aafd4308cbba7165f9f2cdb0cfbed5a3c99ce # v1 | ||
| with: | ||
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} | ||
| github_token: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
🟡 Minor F25: classify step omits the env: GH_TOKEN its sibling dedupe job sets
The Classify PR with Claude step passes github_token as an action input but sets no env: GH_TOKEN, while the analogous find-duplicates step in the same PR sets both env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} and the identical github_token input. The classify prompt drives gh pr view through the Bash tool, which authenticates from the GH_TOKEN environment. I cannot tell from the diff whether claude-code-action propagates its github_token input into the Bash tool's environment or whether the explicit env is required: if the latter, gh pr view fails, classify writes no severity.txt, and apply warns and no-ops — the severity feature silently stops labeling. Either the dedupe env is redundant or the classify job is missing it; make the two consistent and confirm which.
| main() { | ||
| local result_dir="${1:-result}" | ||
| : "${PR_NUMBER:?PR_NUMBER is required}" | ||
| : "${REPO:?REPO is required}" |
There was a problem hiding this comment.
🟡 Minor F26: apply script guards PR_NUMBER and REPO but not GH_TOKEN
The script header documents three required env vars (GH_TOKEN, PR_NUMBER, REPO) but only two are enforced with : "${VAR:?...}" fail-fast guards; GH_TOKEN has none. In the workflow it is always set, so this is not a live defect — but a future caller wiring a new job or running locally who omits GH_TOKEN gets an opaque gh auth error deep in main (possibly after label reconciliation has run) rather than the clean up-front failure the other two vars produce, or silently acts with whatever ambient gh credential is present. Add a matching : "${GH_TOKEN:?...}" guard so enforcement matches the documented contract.
| # exactly the ones present. (The severity-change banner is authored by the | ||
| # model in comment.md, so no previous severity is needed here.) | ||
| local cur_labels | ||
| cur_labels="$(gh pr view "$PR_NUMBER" --repo "$REPO" \ |
There was a problem hiding this comment.
🟡 Minor F27: gh pr view read failure can leave the PR with two severity labels
cur_labels="$(gh pr view ... --jq '.labels[].name' || true)" swallows any read failure into an empty string. The reconciliation loop then finds no severity-* labels to remove, but the subsequent gh pr edit --add-label still runs — so a transient read failure (rate limit, 5xx) adds the new label while leaving the old one, exactly the two-label state the single-edit design (and its inline comment) claims to prevent. It self-heals on the next successful run, so impact is low, but the stated guarantee holds only when the label read succeeds. Consider failing closed (drop || true) or distinguishing "no labels" from "read failed" before proceeding to the edit.
|
|
||
| DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| # shellcheck source=scripts/apply-pr-severity.sh | ||
| source "$DIR/apply-pr-severity.sh" |
There was a problem hiding this comment.
🟡 Minor F28: test exercises only sanitize_comment, not main()'s branches
The new test sources the script and thoroughly covers sanitize_comment, but never invokes main, so its real branching is untested: the invalid-severity return 1, the add/remove label reconciliation (F9/F13's fix and F27's gap), the should_comment/-s comment.md gating, and the max_bytes cutoff (F22's fix) — precisely the paths with gh side effects most likely to regress. Add cases that stub gh via a PATH shim and assert the emitted gh pr edit/gh pr comment argument lists per branch.
Roasbeef
force-pushed
the
ci/workflow-job-split
branch
from
7499871 to
b5c59b0
Compare
|
/gateway dismiss F20 Accepted partial by design. The token-bearing third-party action ( |
|
🚫 Dismissed F20 (minor) by @Roasbeef — no reason given Open findings on this PR: 🟡 F24 (minor) · 🟡 F25 (minor) · 🟡 F26 (minor) · 🟡 F27 (minor) · 🟡 F28 (minor) |
|
/gateway re-review |
|
✅ Re-review posted: #10955 (review) Prior findings: 22 addressed, 0 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
Every non-dismissed prior finding is now addressed. The five that were still open last cycle are all fixed in the current head: classify/apply now carry timeout-minutes (F24), classify sets env: GH_TOKEN (F25), apply-pr-severity.sh guards all three env vars including GH_TOKEN (F26), the label read is fail-closed with no || true (F27), and the new apply-pr-severity_test.sh exercises main()'s branches via a gh PATH-shim (F28). The extraction of the label/comment logic into a unit-tested script, the single-edit label reconciliation, the post-sanitization size cap, and the SHA-pinned action are all in place and hold up.
One new minor point: the classify step pins no model: while the sibling find-duplicates step pins claude-haiku-4-5-20251001, so a security-classification gate inherits a model that can drift silently across action updates.
One residual I am not raising as a finding: a specialist noted bare commit-SHA strings in comment.md still autolink and were not added to the sanitizer. This is the same notification-amplification class as the already-dismissed F5/F23 (accepted residual, bounded, no fund/data/write-scope risk), and the bare-SHA variant is weaker still (requires a real matching commit oid). Flagging for awareness only.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 1 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Fixed —
post-commentdownload usescontinue-on-error: trueand the step guards[[ ! -f result/duplicates.txt ]]before use, so the missing-artifact path is reachable. - F2 addressed: Fixed —
applydownload usescontinue-on-error: trueandapply-pr-severity.shguards missingseverity.txtwith a::warning::+return 0. - F6 addressed: Fixed — both
allowed_non_write_users: "*"sites now carry the inline comment marking"*"safe only while the job stays read-only. - F7 addressed: Fixed — the inline truncating-pipeline / SIGPIPE hazard was removed when the logic moved into
apply-pr-severity.sh, which caps size withwc -con a materialized file rather than ahead -cpipe underpipefail. - F8 addressed: Fixed by design —
apply-pr-severity.shemits::warning::PR severity classifier produced no verdicton a missingseverity.txt, giving the operator a distinguishable signal rather than a fully silent green. Not failing the job is the accepted "classification is advisory" tradeoff. - F9 addressed: Fixed — labels are reconciled in a single
gh pr edit(add target + remove only present others), so no window exists where the old label is removed before the add. - F10 addressed: Fixed — the file-count/tier rendering that produced this mismatch was removed; the header is now authored by the model in
comment.md, not recomputed in the apply path. - F11 addressed: Fixed — the
files.tsvread loop that dropped an unterminated last record no longer exists after the logic moved into the script. - F13 addressed: Fixed — the single-edit reconciliation removes the add-then-remove window entirely, so a cancelled run can never leave two
severity-*labels. - F14 addressed: Fixed —
apply-pr-severity.shlowercases on read (tr '[:upper:]' '[:lower:]') before thecritical|high|medium|lowcase, so a strayHIGH/Lowstill matches. - F15 addressed: Fixed — the comment gate uses
! -s "$result_dir/comment.md"(exists and non-empty), so a 0-byte body never reachesgh pr comment --body-file. - F16 addressed: Fixed — the
find-duplicatesstep passesclaude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}again, so the action can authenticate. - F17 addressed: Fixed —
claude-dedupe-issues.ymlnow declares a workflow-levelconcurrencygroup keyed on the issue number withcancel-in-progress: true. - F18 addressed: Fixed —
sanitize_comment()includess/#([0-9])/#${z}\1/g, defanging#Ncross-references while leaving##/###headings intact. - F19 addressed: Fixed — the
post-commentstep emits::warning::dedupe find-duplicates produced no resultfor the absent-file case, reserving the silentexit 0for the present-but-empty case. - F21 addressed: Fixed — the ~100 lines of security-relevant shell were extracted into the checked-in
scripts/apply-pr-severity.sh, and the sanitizer/control-flow now have regression coverage inscripts/apply-pr-severity_test.sh. - F22 addressed: Fixed — the size cap is measured on
comment.sanitized.mdaftersanitize_commentruns (wc -c < "$result_dir/comment.sanitized.md"), so byte-growing rules can't push a posted body past GitHub's limit. - F24 addressed: Fixed —
pr-severity.ymlnow setstimeout-minutes: 10onclassifyandtimeout-minutes: 5onapply, matching the dedupe workflow. - F25 addressed: Fixed — the
Classify PR with Claudestep now setsenv: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}, consistent with the dedupefind-duplicatesstep, sogh pr viewin the Bash tool authenticates. - F26 addressed: Fixed —
apply-pr-severity.shnow guards: "${GH_TOKEN:?GH_TOKEN is required}"alongsidePR_NUMBERandREPO, matching the documented contract. - F27 addressed: Fixed — the
cur_labelsread dropped the|| trueand now fails closed underset -e; the inline comment documents that a read failure aborts before any edit, preventing the two-label state. - F28 addressed: Fixed —
apply-pr-severity_test.shadds a# main()section that stubsghvia a PATH shim and asserts the emittedgh pr edit/gh pr commentarg lists across the invalid-severity, missing-verdict, label-only reconciliation, comment-posting, empty-body, and oversized-body branches.
| # GITHUB_TOKEN in-process and is reachable by any fork author, so a | ||
| # repointed tag would run attacker-controlled action code with those | ||
| # secrets present. Bump deliberately when updating. | ||
| uses: anthropics/claude-code-action@ba0aafd4308cbba7165f9f2cdb0cfbed5a3c99ce # v1 |
There was a problem hiding this comment.
🟡 Minor F29: classify step pins no model; sibling dedupe step does
The Classify PR with Claude step sets no model: input, so it inherits whatever anthropics/claude-code-action defaults to, while the sibling find-duplicates step in claude-dedupe-issues.yml explicitly pins model: claude-haiku-4-5-20251001. Because classify drives an automated write-labeling gate (its severity.txt decides the label the privileged apply job applies), an unpinned model means the classification behavior — and its cost — can drift silently on an action update, and the decision is not reproducible from the workflow file alone. Pin an explicit model: on the classify step so the severity decision is pinned the same way the action version now is, matching the dedupe workflow's convention. Minor and pre-existing rather than introduced here, but in scope for a hardening refactor that otherwise made the two workflows symmetric.
Roasbeef
force-pushed
the
ci/workflow-job-split
branch
from
b5c59b0 to
e07d752
Compare
|
/gateway re-review |
|
✅ Re-review posted: #10955 (review) Prior findings: 23 addressed, 0 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
The refactor is in good shape. Every non-dismissed prior finding is now addressed: the apply logic was extracted into the tested scripts/apply-pr-severity.sh (F21), the download steps tolerate a missing artifact (F1/F2), the sanitizer defangs @/#N/links with a post-sanitization size cap (F18/F22), both workflows gained timeouts, SHA-pinned the token-bearing action, restored the OAuth token on the dedupe job (F16), and — new this round — the classify step pins model: claude-sonnet-5, closing the last open finding F29.
Two minor consistency gaps remain in the new script, both around how it handles malformed classifier output: should_comment.txt is not case-normalized the way severity.txt is, and a present-but-empty severity.txt fails the job red rather than degrading like the missing-file path. Neither is a fund/write-scope risk on this advisory labeler.
Findings: 🔴 0 Blocker · 🟠 0 Major · 🟡 2 Minor · 🔵 0 Nit
Status of prior findings
- F1 addressed: Fixed —
post-commentdownload-artifact usescontinue-on-error: true, making the[[ ! -f result/duplicates.txt ]]guard reachable. - F2 addressed: Fixed —
applydownload-artifact usescontinue-on-error: true; the script's[[ ! -f "$result_dir/severity.txt" ]]guard now handles the no-verdict path. - F6 addressed:
allowed_non_write_users: "*"now carries the inline "safe ONLY while this job stays read-only" comment in both workflows — the minimum bar this finding asked for. - F7 addressed: The
clean()/head -cSIGPIPE pipeline no longer exists; comment rendering moved out of inline shell intoapply-pr-severity.sh, which caps size without a truncating pipe. - F8 addressed: The script emits
::warning::PR severity classifier produced no verdictfor the missing-verdict case rather than laundering it into a silent green. - F9 addressed: Label reconciliation is now a single
gh pr editthat adds the target and removes only present siblings — no unguarded add after removals. - F10 addressed: The
files.tsvheader/bullet count mismatch is moot; the comment is now the model-authoredcomment.md, not rendered from a tier table. - F11 addressed: Same as F10 — the trailing-newline
readloop overfiles.tsvno longer exists. - F13 addressed: The single-edit reconciliation (add + removes in one
gh pr edit) closes the two-label cancellation window; documented inline in the script. - F14 addressed:
severityis lowercased on read (tr '[:upper:]' '[:lower:]'), soCritical/HIGHstill match. - F15 addressed: The comment guard uses
-s(! -s "$result_dir/comment.md"), so a 0-byte body no longer reachesgh pr comment. - F16 addressed:
claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}is restored on the dedupefind-duplicatesstep. - F17 addressed:
claude-dedupe-issues.ymlnow declares aconcurrencygroup keyed on the issue number withcancel-in-progress: true. - F18 addressed: The sanitizer defangs
#-before-digit (s/#([0-9])/#${z}\1/g); covered by the test's#123/repo#4567cases. - F19 addressed: The dedupe
post-commentstep emits::warning::dedupe find-duplicates produced no resultfor the missing-artifact case, reserving the silent no-op for present-but-empty. - F21 addressed: The ~100 lines of apply-time shell were extracted into checked-in
scripts/apply-pr-severity.shwith a unit test (apply-pr-severity_test.sh). - F22 addressed: The 16 KB cap is measured on the sanitized body (
wc -c < "$result_dir/comment.sanitized.md") aftersanitize_commentruns. - F24 addressed:
classifysetstimeout-minutes: 10andapplysetstimeout-minutes: 5, matching the dedupe workflow. - F25 addressed: The classify step now sets
env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}, consistent with the dedupe find step; the inline comment documents why. - F26 addressed:
apply-pr-severity.shguards all three documented env vars, including: "${GH_TOKEN:?GH_TOKEN is required}". - F27 addressed:
cur_labels="$(gh pr view ...)"drops the|| true, so a read failure aborts underset -ebefore any edit — failing closed as the finding asked. - F28 addressed:
apply-pr-severity_test.shnow stubsghvia a PATH shim and asserts thegh pr edit/gh pr commentargument lists across the invalid-severity, missing-verdict, label-reconciliation, comment, empty-comment, and oversized branches. - F29 addressed: Fixed at
.github/workflows/pr-severity.yml— theClassify PR with Claudestep now pinsmodel: claude-sonnet-5, so the classification that gates the applied label is reproducible from the workflow file, matching the dedupe step's convention.
| # privileged, semantically meaningful output. Lowercase on read so a stray | ||
| # `Low`/`HIGH` from the model still matches rather than failing the job red. | ||
| local severity | ||
| severity="$(tr -d '[:space:]' < "$result_dir/severity.txt" | tr '[:upper:]' '[:lower:]')" |
There was a problem hiding this comment.
🟡 Minor F30: Empty severity.txt fails the job red instead of degrading
The two "no usable verdict" cases are handled inconsistently. A missing severity.txt returns 0 with a ::warning:: (advisory degrade), but a present-but-empty or whitespace-only file collapses to severity="", falls through the case to *), and hits return 1, turning the apply job red. An LLM that creates the artifact file but writes nothing into it — a plausible failure mode, and the exact shape if-no-files-found: warn + continue-on-error were tuned to tolerate elsewhere — would spuriously fail the check rather than degrading like the missing-file path. Treat an empty/blank severity the same as a missing verdict (warn + return 0) so the fail-open contract is consistent.
|
|
||
| local should_comment="false" | ||
| if [[ -f "$result_dir/should_comment.txt" ]]; then | ||
| should_comment="$(tr -d '[:space:]' < "$result_dir/should_comment.txt")" |
There was a problem hiding this comment.
🟡 Minor F31: should_comment not case-normalized unlike severity
should_comment is only stripped of whitespace, while severity is additionally lowercased (line 64). The classifier prompt asks for exactly true/false, but a single model slip (True/TRUE) fails the != "true" check and the comment is silently dropped under a misleading "No comment requested" message — the same class of LLM-output-normalization gap F14 closed for severity, left open on this sibling field. Lowercase on read for parity.
| should_comment="$(tr -d '[:space:]' < "$result_dir/should_comment.txt")" | |
| should_comment="$(tr -d '[:space:]' < "$result_dir/should_comment.txt" | tr '[:upper:]' '[:lower:]')" |
In this commit, we separate the two concerns in the PR severity workflow: working out the severity, and applying it. The classify job inspects the PR and records its verdict (the severity level, whether to comment, and the comment body) to a few files. A second apply job reads those files and does the mechanical work of setting the label and posting the comment. Pulling the classification apart from the application keeps each job doing one thing and makes the flow easier to follow. The apply job takes the severity the classifier picked and checks it against the known set before touching a label, and posts the comment from a file via --body-file so the body is handled as plain data. We also turn off checkout credential persistence, since neither job needs a git credential on disk.
In this commit, we give the issue dedupe workflow the same shape: one job finds the duplicate candidates, another posts the comment. The find job records the candidate issue numbers to a file, and the post job hands those numbers to comment-on-duplicates.sh, which already validates each number and renders the comment from a fixed template. Keeping detection and posting apart mirrors how the script is already factored, so the post job ends up a thin wrapper over it. We also drop the unused id-token permission and turn off checkout credential persistence while we're in here.
|
/gateway re-review |
Roasbeef
force-pushed
the
ci/workflow-job-split
branch
from
e07d752 to
d1ea868
Compare
|
✅ Re-review posted: #10955 (review) Prior findings: 24 addressed, 0 still unresolved, 0 withdrawn. 🔁 Need another re-review after pushing changes? Reply with |
There was a problem hiding this comment.
This round resolves the last two open findings. scripts/apply-pr-severity.sh now treats an empty/whitespace-only severity.txt the same as a missing one (warn + return 0, F30) and lowercases should_comment on read so a True/TRUE slip still posts the comment (F31); both gained regression tests in apply-pr-severity_test.sh (emptysev, truecase). Every other prior finding remains addressed — the classify/apply and find/post split, the token/permission scoping, the artifact-cliff tolerance, the pinned action SHA and model, and the extracted, unit-tested sanitizer are all in place and unregressed.
The dismissed residuals (F3, F4, F5, F12, F20, F23) are dropped per the maintainer's dismissals and are not re-raised — the specialists' re-surfacing of the first-party-action pinning (F20), the GH-/SHA autolink gap (F23), and the BASE_ISSUE validation asymmetry (F4) all fall inside those dismissals. No genuinely-new findings this round.
Status of prior findings
- F30 addressed: Fixed. Empty/whitespace-only
severity.txtnow collapses toseverity=""viatr -d '[:space:]'and hits the[[ -z "$severity" ]]→::warning::…no verdict→return 0path (script L54–63), matching the missing-file degrade rather than falling through toreturn 1. Covered by theemptysevtest case ("empty severity.txt returns 0" / "no label edit"). - F31 addressed: Fixed.
should_commentis now read withtr -d '[:space:]' | tr '[:upper:]' '[:lower:]', matching the severity normalization, so aTrue/TRUEslip no longer silently drops the comment. Covered by thetruecasetest ("TRUE still posts the comment"). - F1 addressed:
post-commentdownload usescontinue-on-error: true, so the[[ ! -f result/duplicates.txt ]]guard is reachable. - F2 addressed:
applydownload usescontinue-on-error: true; the script's missing/empty-verdict guards are reachable. - F6 addressed: Both workflows now carry the inline comment marking
allowed_non_write_users: "*"safe only while the job stays read-only. - F7 addressed: The
clean()/head -cSIGPIPE-under-pipefail hazard is gone — the inline block was replaced byapply-pr-severity.sh, which caps size withwc -con a plain file, not a truncating pipe. - F8 addressed: The no-verdict path emits
::warning::PR severity classifier produced no verdict, so a broken model run no longer reads as a healthy no-op. - F9 addressed: Label reconciliation is now a single
gh pr editthat adds the target and removes only the present others — no unguarded add after a remove. - F10 addressed: The
files.tsvheader-count mismatch is moot; the model no longer emitsfiles.tsvand the header is authored incomment.md. - F11 addressed: The trailing-newline
readloop overfiles.tsvno longer exists in the refactored design. - F13 addressed: The two-label race is closed by the single-edit reconciliation; a cancelled run cannot stop between a separate add and remove.
- F14 addressed:
severityis lowercased on read (tr '[:upper:]' '[:lower:]'), soCritical/HIGHno longer fall through to the error path. - F15 addressed: The comment gate uses
! -s "$result_dir/comment.md"(exists-and-non-empty), so a 0-byte body no longer reachesgh pr comment --body-file. Covered by theemptytest. - F16 addressed:
claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}is restored on thefind-duplicatesstep. - F17 addressed:
claude-dedupe-issues.ymlnow declares aconcurrencygroup keyed on the issue number withcancel-in-progress: true. - F19 addressed:
post-commentemits::warning::dedupe find-duplicates produced no resultfor the absent-artifact case, reserving the silentexit 0for present-but-empty. - F21 addressed: The apply logic is extracted into checked-in
scripts/apply-pr-severity.shand unit-tested byapply-pr-severity_test.sh. - F22 addressed: The size cap is measured on the sanitized body (
wc -c < "$result_dir/comment.sanitized.md"), after sanitization expands bytes. Covered by thebigtest. - F24 addressed: Both
classify(10m) andapply(5m) now settimeout-minutes, matching the dedupe workflow. - F25 addressed: The
classifystep now setsenv: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}, consistent withfind-duplicates. - F26 addressed:
GH_TOKENnow has a: "${GH_TOKEN:?GH_TOKEN is required}"fail-fast guard matchingPR_NUMBER/REPO. - F27 addressed:
cur_labelsis read without|| true, so agh pr viewread failure now fails closed underset -ebefore any edit, upholding the single-edit no-two-label guarantee. - F28 addressed:
apply-pr-severity_test.shnow stubsghvia a PATH shim and asserts the emittedgh pr edit/gh pr commentargument lists across the invalid/missing/empty/label-only/comment/oversized branches. - F29 addressed: The
classifystep pinsmodel: claude-sonnet-5, so the label-gating classification no longer drifts on an action default.
|
/gateway approve |
There was a problem hiding this comment.
✅ Approved — all prior findings addressed.
Findings recap
Findings:
F1(major) — addressed:find-duplicatesuploadsduplicates.txtwithif-no-files-found: warn(line 95), so when the model produces no file at all — a routine L…F2(major) — addressed: Same artifact-contract cliff as F1.classifyuploads withif-no-files-found: warn(line 231), so if the model writes none of `severity.t…F3(major) — partially_addressed: The privilege split leaks throughcomment.md. The read-onlyclassifyjob runs the model against fully attacker-controlled input — `gh pr…F4(minor) — unresolved: The duplicate ids are filtered throughgrep -oE '^[0-9]+$', butBASE_ISSUE(${{ github.event.issue.number || inputs.issue_number }}) i…F5(minor) — partially_addressed: Thepost-commentjob correctly filtersduplicates.txtto digits-only and passes the ids as array args, so there is no argument injection…F6(minor) — addressed:allowed_non_write_users: "*"(present in both workflows) disables claude-code-action's author-association gate, feeding untrusted fork/fir…F7(major) — addressed:clean()istr … | sed … | head -c "$1". Underset -euo pipefail, when the input exceeds the byte cap,head -ccloses the pipe after …F8(minor) — addressed: The F1/F2 fix (if-no-files-found: warnon upload +continue-on-error: trueon download +[[ ! -f result/severity.txt ]]; exit 0) makes…F9(minor) — addressed: Theapplyjob removes everyseverity-*label (each--remove-label … 2>/dev/null || true) and then runs an unguarded `gh pr edit … …F10(minor) — addressed:FILE_COUNT="$(grep -c . result/files.tsv || true)"counts every non-empty line, but the renderer only iterates the fixed tiers `critical h…F11(minor) — addressed:while IFS=$'\t' read -r f_tier f_path f_reason; do … done < result/files.tsvskips the last record if the model writesfiles.tsvwithout…F12(minor) — unresolved: The privilege split protects the repo write token but not the secrets already in theclassifyjob's environment.classifyruns on `pull_…F13(minor) — addressed: The F9 fix (add the new label before removing the others) combined with the pre-existing workflow-level `concurrency: … cancel-in-progress: …F14(minor) — addressed:SEVERITY="$(tr -d '[:space:]' < result/severity.txt)"strips whitespace but does not normalize case, and thecaseaccepts only `critical…F15(minor) — addressed: The guard[[ "$SHOULD_COMMENT" != "true" || ! -f result/comment.md ]]checks only file existence. If the classifier sets `should_comment=t…F16(major) — addressed: The split removed theclaude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}input the pre-split step carried (it is a removed `-…F17(minor) — addressed: Unlikepr-severity.yml, this workflow declares noconcurrencygroup, and the split widens the check-then-post window: the "already has a…F18(minor) — addressed: Theapplysedsanitizer neutralizes@-mentions,www.,http(s)://, and Markdown brackets, but leaves bare#<number>references in…F19(minor) — addressed: Thepost-commentguard[[ ! -s result/duplicates.txt ]] && exit 0(line 146) collapses two distinct states into a silent green: (a) the …F20(minor) — partially_addressed: Actions are pinned to mutable tags rather than commit SHAs:anthropics/claude-code-action@v1(pr-severity line 45, dedupe line 44) and `ac…F21(minor) — addressed: Theapplystep inlines ~100 lines of security-relevant shell in YAML — severity validation, label reconciliation, size-capping, and the un…F22(minor) — addressed: The 16 KB cap is measured on the rawcomment.md(wc -c, line 360) beforesedsanitization runs (line 364). Every sanitizer rule only g…F23(minor) — unresolved: The sanitizer (line 364) defangs@-mentions,www.,http(s)://,#-before-digit, and Markdown brackets, but notGH-<number>referenc…F24(minor) — addressed: Neitherclassifynorapplysetstimeout-minutes, whereas the siblingclaude-dedupe-issues.ymlcapsfind-duplicatesat 10m and `pos…F25(minor) — addressed: TheClassify PR with Claudestep passesgithub_tokenas an action input but sets noenv: GH_TOKEN, while the analogous `find-duplicate…F26(minor) — addressed: The script header documents three required env vars (GH_TOKEN,PR_NUMBER,REPO) but only two are enforced with: "${VAR:?...}"fail-…F27(minor) — addressed:cur_labels="$(gh pr view ... --jq '.labels[].name' || true)"swallows any read failure into an empty string. The reconciliation loop then …F28(minor) — addressed: The new test sources the script and thoroughly coverssanitize_comment, but never invokesmain, so its real branching is untested: the i…F29(minor) — addressed: TheClassify PR with Claudestep sets nomodel:input, so it inherits whateveranthropics/claude-code-actiondefaults to, while the si…F30(minor) — addressed: The two "no usable verdict" cases are handled inconsistently. A missingseverity.txtreturns 0 with a::warning::(advisory degrade), …F31(minor) — addressed:should_commentis only stripped of whitespace, whileseverityis additionally lowercased (line 64). The classifier prompt asks for exact…
Dismissed:
F5by @Roasbeef — F12F12by @saubyk — per #10955 (comment)F3by @Roasbeef —F4by @Roasbeef — F23F23by @saubyk — per #10955 (comment)F20by @Roasbeef —
Approved by @Roasbeef via /gateway approve. Last reviewed at d1ea868. Skill v0.3.0, model claude-opus-4-8.
3 participants
In this PR, we clean up the two Claude-driven CI workflows by splitting
each one into a pair of jobs along a clear seam: the part that figures out
what to do, and the part that actually does it. Both workflows had grown
into a single job that mixed the model-driven analysis with the mechanical
GitHub mutation, and pulling those apart makes each job do one thing and
makes the whole flow easier to follow.
PR severity
The
pr-severityworkflow now has aclassifyjob and anapplyjob. Theclassify job inspects the PR and writes its verdict (the severity level,
whether a comment is warranted, and the comment body) out to a few files.
The apply job reads those files and does the label/comment bookkeeping:
strip any stale
severity-*label, set the new one, and post the comment.The severity the classifier picks is checked against the known set
(
critical/high/medium/low) before we touch a label, so a garbledverdict just no-ops instead of creating junk labels. The comment goes out
via
--body-fileso the body is always handled as plain data rather thanbeing spliced into a shell command.
Issue dedupe
The
claude-dedupe-issuesworkflow gets the same shape: afind-duplicatesjob and a
post-commentjob. The find job records the candidate issuenumbers to a file, and the post job hands them to
scripts/comment-on-duplicates.sh, which already validates each number andrenders the comment from a fixed template. This mostly just mirrors how the
script is already factored, so the post job ends up a thin wrapper over it.
Misc
While in here we also drop the unused
id-tokenpermission from the dedupeworkflow and turn off checkout credential persistence in both, since none
of these jobs need a git credential sitting on disk.
See each commit message for the per-workflow details.