Security Review (pre-merge)

.github/workflows/security-review.yml runs the claude-code-security-review action over a PR diff. It is not automatic. It runs when the PR carries the security-review label.

Subscription auth

The workflow points at Minim-Digital/claude-code-security-review, a fork of upstream 0c6a49f (the exact commit it ran before) with one change: a claude-code-oauth-token input. The scan authenticates against Dan's Claude Max subscription via the CLAUDE_CODE_OAUTH_TOKEN repository secret, so it spends subscription usage rather than API dollars. Everything else is upstream and unchanged, which is why the marker-cache mechanics below still apply exactly.

The fork exists because the alternative, anthropics/claude-code-action (which supports OAuth natively), restores .claude/, CLAUDE.md and .mcp.json from the base branch before reviewing, so a PR's changes to those files are never scanned (verified on #448). This workflow deliberately reviews those files, so that action would have been a silent coverage regression. See the workflow header for the fork-sync procedure and the secret's ~1-year expiry.

The procedure

1. Work on the PR as normal. Nothing scans, and nothing pretends to.
2. When the PR is ready to merge, add the security-review label. That scans the final state, which is the thing actually being merged.
3. Any push made while the label is on rescans, so fixes written in response to findings are reviewed too.
4. Read the verdict before merging. See "Verify the verdict" below, because a green tick is not sufficient evidence on its own.

The label is a required step in the merge ritual. There is no automation that will do it for you, and no check that fails if you forget.

Many more commits after a scan

Removing the label stops scanning until you put it back. unlabeled is deliberately not in the trigger's types, so taking the label off fires nothing at all, and the job's if condition then evaluates false on every subsequent push.

So when a scanned PR turns out to need a lot more work:

1. Remove the security-review label. Nothing fires.
2. Push as many commits as you like. Each one triggers the workflow, the condition is false, and the job is skipped without spending anything.
3. Re-add the label when you are done. That scans the new final state, and because run-every-commit: true is set it is a genuine scan rather than a marker skip.

Skipped runs show in the checks list as skipped, not green, so they cannot be mistaken for a pass.

The simpler version of the same thing: do not add the label until you are actually ready. The label is the "I am done" signal. The remove-and-re-add cycle only exists for when you labelled early and then found more work, which is exactly what happened on the PR that introduced this workflow.

WARNING

Do not push and apply the label at the same moment. synchronize and labeled fire together, and the synchronize run evaluates its condition before the label lands, so it skips. Until 2026-08-19 the two also shared a concurrency group, so one cancelled the other and the PR got no scan at all, reported as skipped rather than failed. Observed on #443.

The group is now keyed on github.event.action so a labeled run cannot be cancelled by a synchronize run. If you ever see a scan you expected simply not appear, remove the label, wait a moment, and add it back: that fires a clean labeled event with nothing to race.

Why it is not automatic

It used to run on every push. That sounded thorough and was not.

The action caches a per-PR marker file and honours it on later runs, so only the first commit of any PR was ever actually analysed. Every run after that found the marker, exited green, and read nothing. Measured on 2026-08-19:

PR	commits	actually analysed
#442	7	2b179660 only
#443	5	b0d4b4d3 only

On both, the substantive work landed after the first commit, and the security check was green throughout. Scanning the merge candidate once beats scanning the opening commit and calling it coverage.

WARNING

A green security check on any PR merged before 2026-08-19 covers its first commit only. Do not read those as whole-PR coverage.

run-every-commit: true on the action defeats the marker and is what makes step 3 above real. Do not remove it: without it, the first scan after labelling is genuine and every rescan is a vacuous green, which is the original bug wearing a different hat.

Verify the verdict, not the exit status

A real scan prints this as output:

Running ClaudeCode AI security analysis...

The trap: the action also echoes its own script, so that string and the marker message both appear in the log on runs that analysed nothing. Only lines without the \033[36;1m escape prefix are real output. Grepping for the text alone will mislead you.

gh run view --job <job-id> --log > /tmp/sec.log
python3 -c "
ran = skip = 0
for l in open('/tmp/sec.log', errors='ignore'):
    body = l.split('\t')[-1]
    # gh renders the ANSI escape as the LITERAL two characters '^[', not 0x1b
    if '^[[36;1m' in body or '##[' in body:   # echoed script, or a group header
        continue
    if 'Running ClaudeCode AI security analysis' in body: ran += 1
    if 'has already run on PR' in body and 'forcing disable' in body: skip += 1
print(f'analysed={ran} markerSkipped={skip}')"

WARNING

Match on substring, not startswith. Every log line begins with a timestamp, so startswith('Running ClaudeCode...') never matches and silently reports analysed=0 on a run that analysed perfectly well. That mistake was made twice while writing this page, once in each direction, and both times it produced a confident wrong conclusion.

analysed=0 with markerSkipped=0 usually means the diff was entirely inside exclude-directories, so there was genuinely nothing to scan. That is coverage-neutral by construction, not a failure.

Force a genuine re-scan

Needed when a scan was vacuous, or to re-review an older PR. Delete every cache for the PR: the action restores by key prefix, so one surviving entry re-triggers the skip.

R=Minim-Digital/podcaster-plus-app
N=<pr-number>
gh api /repos/$R/actions/caches --paginate \
  -q ".actions_caches[]|select(.key|test(\"pr-$N\"))|.id" \
  | xargs -I{} gh api -X DELETE /repos/$R/actions/caches/{}

Then re-run the workflow. Confirm with the check above that it actually analysed.

Scope and cost

• Skipped paths. docs/, docs-public/, docs-internal/ and src/lib/components/ui/ are in paths-ignore, mirroring the action's exclude-directories. A PR touching only those reviews zero files, so skipping is coverage-neutral by construction. That list is a denylist on purpose; see the workflow header for why an allowlist was rejected.
• Model is pinned to claude-sonnet-5. The action's own default is a deprecated Opus tier that burns far more of the subscription's usage window for the same diff.
• Cost. Since 2026-08-19 the scan runs on Dan's Max subscription (see the auth note above), so it spends no API dollars — a slice of the subscription's usage window instead, a few minutes of Sonnet agent time per scan. The label trigger keeps that to roughly one scan per PR plus one per post-label fix. For context, before label-gating, 203 API runs in August cost about $49, and most of those were not scans.
• False-positive filtering. The upstream action runs a second Claude pass to filter false positives through the Anthropic SDK, which needs an API key. On subscription auth that pass degrades to hard-rule filtering only; the main scan prompt does its own confidence-based filtering (findings below confidence 8 are dropped), so this is a minor reduction in a second-opinion layer, not the primary filter.

Cost bounds

Two layers, and they cap different things. Do not remove either.

Bound	Where	Caps
claudecode-timeout: 10	action input	analysis work per scan (subscription usage)
timeout-minutes: 20	the job	billed runner time, including parts of the action we do not control

claudecode-timeout is deliberately lower than the job timeout, so the analysis aborts and reports rather than being killed mid-flight. The action's own default is 20 minutes; observed genuine analysis is well under 2.

The job bound exists because of a real incident, twice on 2026-08-19. The action's own step wedged for the full 20 minutes:

sudo apt-get update && sudo apt-get install -y gh   # action.yml:68

The cause is the Azure apt mirror, not the install. azure.archive.ubuntu.com is what GitHub's hosted images point at, and it is intermittently unreachable. Apt retries it with long socket timeouts across every index file before falling back to the public mirror, and prints nothing while blocked on a socket. It presents as a silent hang, not an error.

The evidence from the killed run:

15:49:44  start-action: Install GitHub CLI
15:50:59  Ign: http://azure.archive.ubuntu.com/ubuntu noble InRelease   <- unreachable
15:51:04  Get: https://archive.ubuntu.com/ubuntu noble-security         <- fell back
          ......... 18m51s of silence, still in the same step .........
16:09:54  end-action: Install GitHub CLI  outcome=cancelled

Every later step shows skipped, so no Anthropic API spend occurred: the analysis never started. Worth knowing, because a hang here looks alarming and is not the expensive kind.

The workaround is the Neutralise the Azure apt mirror step, which is the first step of every job that installs packages, including this one. Apt config is global, so repointing the sources and capping Acquire::Retries there reaches the action's own uneditable calls. The same step is on dr-reconcile and dr-db-backup, which install rclone, postgresql-client and age.

DANGER

That step must fail closed, and its first version did not. It hardcoded the source paths, swallowed errors with 2>/dev/null || true, and echoed success unconditionally. On the ubuntu-24.04 image it matched nothing, so it printed apt sources repointed off the Azure mirror while apt carried on using Azure and the job wedged for 17 minutes regardless (#441). The log looked like the fix had worked.

It now greps /etc/apt for whatever files actually reference the mirror, rewrites those, and then verifies none remain, exiting 1 if any do. If you touch this step, keep the verification. A guard that cannot fail is not a guard, it is a comment.

The grep is not defensive vagueness, it is load-bearing. On ubuntu-24.04 the mirror is not in sources.list or sources.list.d/*.sources at all. The image uses a mirrorlist indirection and the Azure host is named in:

/etc/apt/apt-mirrors.txt

which sources.list.d globs can never reach. Do not replace the grep with a hardcoded path; that is exactly the mistake that made the first version a no-op. Confirmed working 2026-08-19: the step logged rewriting: /etc/apt/apt-mirrors.txt, apt then went straight to archive.ubuntu.com with zero Ign: lines, and the job took 1m54s against 17 to 20 minutes when it wedged.

DEBIAN_FRONTEND=noninteractive is also set, but it does not address this. It guards interactive prompt hangs, a real but different mechanism. Do not conflate the two.

The timeout remains the backstop for whatever the workaround does not catch. Without one this would have run to GitHub's 360-minute default.

If a scan exceeds a few minutes, suspect that step rather than the analysis. Check which step is in_progress:

gh api /repos/OWNER/REPO/actions/runs/<id>/jobs \
  -q '.jobs[0].steps[]|"\(.number). \(.name): \(.status)"'

Every job in .github/workflows/ carries a timeout-minutes as of that date. A cron without one is the worst case: a persistent hang repeats daily, so 360 minutes a day is roughly $86 a month of billed runner time from a single wedged job.

What it has caught

Both of these were missed by the other reviewer in the same pass, which is the argument for keeping it alongside Codex rather than choosing between them:

• A HIGH stored XSS in src/lib/components/listen/PeopleRow.svelte (#396), which needed reasoning across the component, the Zod schema and the shared cookie domain.
• The shared-node_modules flaw on #443, flagged MEDIUM fifteen seconds before Codex returned a clean verdict on the same commit.

Related

• Deploy Pipeline
• CI Runner