Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 139 additions & 1 deletion internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,20 @@ and `description`.
| `style-conventions` | sonnet | parallel | Naming, error handling idioms, API shape, code organization |
| `docs-currency` | sonnet | parallel | Documentation staleness (follows docs-review skill inline) |
| `cross-repo-contracts` | sonnet | parallel | API contract breakage affecting other repos (conditional) |
| `security-triage` | haiku | pre-pass | Classifies files by security relevance for large PRs (≥ 50 files) |
| `challenger` | opus | sequential | Adversarial challenge of findings, false-positive removal, deduplication |

The Model column reflects each sub-agent's current frontmatter. Any
value accepted by the Agent tool's `model` parameter is valid in
sub-agent frontmatter.

**Non-standard dispatch types:** `security-triage` (pre-pass) and
`challenger` (sequential) are not dimension sub-agents and are NOT
dispatched in step 4's parallel loop. `security-triage` runs as a
preprocessing classifier in step 3c-1; `challenger` runs as a
post-processing adversarial pass in step 6d. Both produce different
output formats from the standard findings array.

## Findings vs inline comments

Findings are the canonical review output. Each finding records a
Expand Down Expand Up @@ -287,6 +295,95 @@ complex PR that triggers all conditions legitimately needs all 6.
| CI/CD pipeline change | correctness, security, style-conventions, intent-coherence |
| DB migration + API change | correctness, security, style-conventions, cross-repo-contracts, docs-currency |

#### 3c-1. Security-critical file triage (large PRs)

When `FILE_COUNT` (from step 2) is **≥ 50**, run a lightweight triage
pass to identify security-critical files before preparing context
packages. For PRs under 50 files, skip this step — all files receive
uniform attention.

**Why:** On large PRs (≥ 50 files, where step 2 already produces
per-file diffs), security-critical files compete with boilerplate for
the review agent's context window and reasoning budget. A triage pass
ensures files touching auth, permissions, token handling, trust
boundaries, and similar concerns receive dedicated review context
rather than being diluted across dozens of routine changes. The
threshold aligns with step 2's per-file diff boundary so that
per-file diff summaries are always available for the triage prompt.
See #2096 for the motivating incident.

**Procedure:**

1. Read `sub-agents/security-triage.md` for the sub-agent definition.
2. Compose a spawn prompt containing:

**Part 1 — Sub-agent definition:** the full markdown body of the
security-triage sub-agent file (everything after the frontmatter)

**Part 2 — Context:** the PR's changed file list with per-file
diff stats (additions, deletions), plus a brief diff summary for
each file. For files that match a path pattern from the
classification criteria, include the first ~20 lines of the diff
(path patterns are sufficient for classification; the diff summary
confirms rather than drives the decision). For files that do NOT
match any path pattern, include the first ~50 lines of the diff
to give the classifier enough content signal to detect
security-relevant changes (auth logic, token handling, permission
checks) that only appear in the diff body. Format as:

```markdown
## Files to classify

| File | Additions | Deletions |
|------|-----------|-----------|
| <path> | <n> | <n> |
...

## Diff summaries
### <path>
<first ~20 lines of the file's diff>
...
```

3. Spawn via Agent tool with:
- `model`: `haiku` (from the sub-agent frontmatter)
- `subagent_type`: `Explore` (read-only)
- `prompt`: composed from parts 1–2

This agent runs **synchronously** (not in the background) because
its output feeds into step 3d's context package assembly. It uses
haiku for speed — classification does not require deep reasoning.

4. Parse the triage output. The security-triage sub-agent returns a
JSON object with `security_critical_files` (array of objects with
`file` and `reason`), `standard_files` (array of paths), and
`summary` (string).

5. Validate and store the classification result for use in step 3d:

- If the security-triage sub-agent fails (timeout, parse error,
empty response), fall back to treating **all files as
security-critical** — this preserves the existing
uniform-attention behavior as a safe default.
- If `security_critical_files` is empty but any changed files
match the path patterns from the classification criteria (e.g.,
`**/auth/**`, `**/mint/**`, `**/token/**`, `.claude/**`,
`.github/**`, `agents/**`, `scripts/**`), treat this as a
triage failure and apply the same fallback. An empty
classification when path-pattern matches exist indicates the
classifier missed obvious signals.

**Edge cases:**

- **All files classified as security-critical:** The deep-review pass
covers all files with full context. This is equivalent to the
standard review behavior for smaller PRs — no degradation.
- **No files classified as security-critical:** All files receive
standard review. The triage cost (one haiku call) is minimal.
- **Triage sub-agent failure:** Fall back to uniform attention (all
files treated as security-critical). Log an info-level note in the
review output.

#### 3d. Prepare context packages

For each selected sub-agent, assemble a context package containing:
Expand All @@ -307,9 +404,50 @@ For each selected sub-agent, assemble a context package containing:
`intent-coherence`)
- `cross_repo_context`: findings from 3a for `cross-repo-contracts`

#### Security-prioritized context (large PRs with triage results)

When step 3c-1 produced a security triage classification (i.e., the PR
has ≥ 50 files and the triage pass succeeded), modify the context
packages for the `security` and `correctness` sub-agents as follows:

1. **Security sub-agent:** Provide the full per-file diffs for all
`security_critical_files` first, clearly marked with a
`### Security-critical file: <path>` header and the triage reason.
Include standard files' diffs after, under a
`### Standard files` header. This ordering ensures
security-critical files receive primary attention within the
sub-agent's context window.

2. **Correctness sub-agent:** Same prioritized ordering — security-
critical files first with their triage classification, then
standard files. Correctness and security findings often overlap on
the same code (e.g., a fail-open bug is both a logic error and a
security vulnerability), so the correctness sub-agent also benefits
from knowing which files the triage pass flagged.

3. **Other sub-agents** (`intent-coherence`, `style-conventions`,
`docs-currency`, `cross-repo-contracts`): Receive the standard
context package without prioritization. These dimensions are not
affected by the security triage classification.

4. **Include the triage summary** in the context package for both
`security` and `correctness` sub-agents:

```markdown
### Security triage classification
<triage summary from step 3c-1>
Security-critical files: <list with reasons>
```

If step 3c-1 was skipped (PR under 50 files) or the triage sub-agent
failed (fallback to uniform attention), prepare all context packages
using the standard format described above — no prioritization.

### 4. Dispatch sub-agents

For each selected sub-agent:
For each selected **dimension** sub-agent (from step 3c — excludes
`security-triage` which runs in step 3c-1, and `challenger` which
runs in step 6d):

1. Read the sub-agent definition from `sub-agents/{name}.md`
2. Extract the `model` from frontmatter
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
---
name: review-security-triage
description: Lightweight classifier that identifies security-critical files in large PRs for prioritized deep review.
model: haiku
---

# Security Triage

You are a security triage classifier. Your job is to scan a PR's
changed file list and diff summary to identify **security-critical
files** that need dedicated review attention. You classify files — you
do not review them.

**Own:** File classification by security relevance based on path
patterns, diff content heuristics, and file purpose.

**Do not own:** Reviewing code, generating findings, evaluating
correctness or style. You only classify.

## Classification criteria

A file is **security-critical** if it matches ANY of the following:

### Path patterns

- `**/mint/**` — token minting and signing
- `**/auth/**` — authentication
- `**/oidc/**` — OIDC validation
- `**/rbac/**` — role-based access control
- `**/permissions/**` — permission definitions
- `**/secrets/**` — secret management
- `**/crypto/**` — cryptographic operations
- `**/token/**` or `**/tokens/**` — token handling
- `**/trust/**` — trust boundary definitions
- `**/CODEOWNERS` — access control governance
- `**/policies/**` — policy definitions

### Governance and infrastructure paths

These paths correspond to the orchestrator's protected-path list
(step 6e). Files here control agent behavior, CI/CD, container
builds, and access governance — changes can alter trust boundaries
or weaken security controls. Classify as security-critical so they
receive prioritized review context.

- `.claude/**` — agent settings and configuration
- `.cursor/**` — editor agent configuration
- `.gitattributes` — file handling attributes
- `.github/**` — CI, GitHub Actions, and repository configuration
(includes workflows, action definitions, and Dependabot config)
- `.pre-commit-config.yaml` — pre-commit hook configuration
- `AGENTS.md` — agent governance rules
- `agents/**` — agent definitions
- `api-servers/**` — API server configurations
- `CLAUDE.md` — project-level agent instructions
- `Containerfile` — container image definitions
- `Dockerfile` — container image definitions
- `harness/**` — harness definitions
- `images/**` — container image build contexts
- `plugins/**` — plugin definitions
- `scripts/**` — pre/post scripts (CI and deployment)
- `skills/**` — skill definitions

### Content heuristics (from diff summary)

- Functions or methods related to authentication, authorization, or
session management
- Token generation, validation, exchange, or scoping logic
- Permission checks, RBAC enforcement, or access control lists
- Secret handling, key management, or credential storage
- OIDC claims parsing, JWT validation, or certificate verification
- Workflow permission declarations or secret exposure
- Trust boundary enforcement or sandbox escape vectors
- Input validation or sanitization for injection defense

### File type signals

- Go files importing `crypto/*`, `oauth2`, `jwt`, or auth-related
packages
- Configuration files declaring permissions, roles, or access policies
- Terraform/IAM/Kubernetes RBAC manifests
- GitHub Actions workflow files with `permissions:` blocks

## Procedure

1. Review the full list of changed files and their diff stats
(additions, deletions, changes summary).
2. For each file, evaluate against the classification criteria above.
3. If in doubt, classify as security-critical — false positives are
acceptable, false negatives are not.
4. Return the classification result.

## Output format

Return a JSON object:

```json
{
"security_critical_files": [
{
"file": "<relative path>",
"reason": "<brief reason for classification>"
}
],
"standard_files": ["<relative path>", "..."],
"summary": "<one-line summary, e.g., '5 of 42 files classified as security-critical'>"
}
```

## Constraints

- Classify ALL files — every file must appear in either
`security_critical_files` or `standard_files`
- Err on the side of inclusion — when uncertain, mark as
security-critical
- Return raw JSON only — do not wrap the output in markdown code
fences (`` ```json ... ``` ``). The orchestrator parses your
response directly as JSON
- Do not read file contents beyond what is provided in the diff
summary — this is a fast classification pass
- Do not write any files
- Do not generate review findings
Loading