From f3cfd2e808e62f83dcc1849d2be0060fa0e1a7f3 Mon Sep 17 00:00:00 2001 From: Nathan Heskew Date: Fri, 24 Apr 2026 09:00:14 -0700 Subject: [PATCH] ci(claude): address external review findings (mirror of harper #402) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same finding-fix pass applied to harper's claude workflows (HarperFast/harper#402), trimmed to what applies to oauth: - #5 (label match too permissive): tighten `startsWith('claude-fix:')` → explicit whitelist of the four supported labels. - #6 (heredoc marker collision risk): replace fixed `CLAUDE_SCOPE_EOF` with a random `EOF_$(openssl rand -hex 16)` delimiter. - #7a (eager `npm ci` on mention): drop. Most mentions don't need deps; prompt tells the agent to `npm ci` before scripts that do. issue-to-pr keeps its eager install. - #8a (Opus cost on every mention): shift to Sonnet default with Opus opt-in via case-insensitive word-boundary `deep` anywhere in the comment body. - #9 (no scope-to-diff guidance): review prompt now tells the agent to start from `git diff --name-only origin/main...HEAD` and expand only when a finding demands it. Plus a mention-parsing shell step that enforces: - `@claude` must be the first non-whitespace token (word-boundary after) — rules out `@claudette`, inline prose mentions, and quoted replies where `@claude` addresses a human. - Subsequent steps guard on `steps.mention.outputs.proceed == 'true'`. Comment sharpening (tradeoff accepted, rationale tightened): - #1 (postinstall RCE via package.json edit): allowlist comment now names the path explicitly. The no-arg `Bash(npm install)` blocks `npm install @attacker/x` but does NOT close the Write+postinstall chain — that's bounded by branch protection + author_association. Structural follow-up (`.npmrc ignore-scripts=true` or dropping `Bash(npm install)` entirely) deserves its own PR. - #2 (`Bash(git:*)` contradicts review's rationale): review.yml's comment now says "this workflow is READ-ONLY by design" and notes that the authoring workflows grant broader git access bounded by branch protection, not by this allowlist. Not applicable to oauth (applicable on harper): - #3 (ai-review-log log step) — oauth already has it. - #4 (dead `documentation/**` glob) — oauth uses the real `docs/` dir, doesn't have the `documentation/**` typo. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/claude-issue-to-pr.yml | 20 ++++++-- .github/workflows/claude-mention.yml | 61 +++++++++++++++++++++--- .github/workflows/claude-review.yml | 30 +++++++++--- 3 files changed, 93 insertions(+), 18 deletions(-) diff --git a/.github/workflows/claude-issue-to-pr.yml b/.github/workflows/claude-issue-to-pr.yml index 8aa8c68..383213b 100644 --- a/.github/workflows/claude-issue-to-pr.yml +++ b/.github/workflows/claude-issue-to-pr.yml @@ -25,8 +25,11 @@ jobs: # opened by a HarperFast org member or collaborator. Labels added # to issues opened by outside contributors are ignored to keep # the trigger surface tight during calibration. + # Explicit whitelist of allowed labels — `startsWith('claude-fix:')` + # would match typoed variants (`claude-fix:typos`, `claude-fix:foo`) + # and the agent would waste turns trying to interpret them. if: >- - startsWith(github.event.label.name, 'claude-fix:') && + contains(fromJSON('["claude-fix:typo","claude-fix:docs","claude-fix:deps","claude-fix:bug"]'), github.event.label.name) && contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.issue.author_association) runs-on: ubuntu-latest @@ -84,11 +87,18 @@ jobs: # * `Bash(npx:*)` — would let an injected instruction run # arbitrary published packages. Whose subprocess reads # GITHUB_TOKEN from env. - # Deliberately TIGHTENED: + # Partial mitigation (NOT a full boundary): # * `Bash(npm install)` (no-arg, not `Bash(npm install:*)`) — - # the `claude-fix:deps` path needs to regenerate the - # lockfile from an edited package.json. Bare form does - # that; the :* glob would let `npm install @attacker/x`. + # blocks `npm install @attacker/`. BUT: the agent + # also has `Write`/`Edit` on package.json. A successful + # injection could add a malicious `postinstall` script + # and then invoke bare `npm install` to execute it, with + # GITHUB_TOKEN + the claude[bot] installation token + # reachable from the subprocess. Branch protection + the + # author_association gate are what actually bound blast + # radius. A future PR may add `ignore-scripts=true` via + # .npmrc and/or drop `Bash(npm install)` entirely, + # deferring installs to a separate CI job. --allowedTools "Read,Write,Edit,Grep,Glob,Bash(gh pr view:*),Bash(gh pr diff:*),Bash(gh pr comment:*),Bash(gh pr create:*),Bash(gh issue view:*),Bash(gh issue comment:*),Bash(git:*),Bash(npm install),Bash(npm ci:*),Bash(npm run:*),Bash(npm test:*),Bash(bun install:*),Bash(bun run:*),Bash(bun test:*)" prompt: | You were invoked because issue #${{ github.event.issue.number }} diff --git a/.github/workflows/claude-mention.yml b/.github/workflows/claude-mention.yml index 1faf804..f6a1719 100644 --- a/.github/workflows/claude-mention.yml +++ b/.github/workflows/claude-mention.yml @@ -51,10 +51,41 @@ jobs: # agent blocked on history lookups. uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 + - name: Parse mention + # Real precision gate (the job-level `if:` is a cheap pre-filter). + # Enforces: + # 1. `@claude` must be the FIRST non-whitespace token (word- + # boundary after) — rules out `@claudette`, inline prose + # mentions ("saw @claude's fix"), and quoted replies + # (`> @claude ...`) where the reply is addressing a human. + # 2. Case-insensitive word-boundary `deep` anywhere in the body + # → escalate to Opus. Sonnet is the default. + id: mention + env: + BODY: ${{ github.event.comment.body }} + run: | + set -uo pipefail + + if ! printf '%s' "$BODY" | grep -Pqz '\A\s*@claude\b'; then + echo "Comment does not start with @claude; skipping." + echo "proceed=false" >> "$GITHUB_OUTPUT" + exit 0 + fi + + if printf '%s' "$BODY" | grep -Piq '\bdeep\b'; then + echo "model=claude-opus-4-7" >> "$GITHUB_OUTPUT" + echo "Selected claude-opus-4-7 (deep requested)" + else + echo "model=claude-sonnet-4-6" >> "$GITHUB_OUTPUT" + echo "Selected claude-sonnet-4-6 (default)" + fi + echo "proceed=true" >> "$GITHUB_OUTPUT" + - name: Clone shared Harper skills # Pinned to a SHA (not `main`) so agent behavior is reproducible # across runs — updates to the skills repo require an explicit # pin bump in this workflow. + if: steps.mention.outputs.proceed == 'true' uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 with: repository: HarperFast/skills @@ -62,27 +93,33 @@ jobs: path: .harper-skills - name: Setup Node.js + # Needed so the agent can run `npm ci` / `npm run