diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md index 7d3226834..5f40ef316 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md @@ -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 @@ -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 | + |------|-----------|-----------| + | | | | + ... + + ## Diff summaries + ### + + ... + ``` + +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: @@ -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: ` 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 + + Security-critical files: + ``` + +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 diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md b/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md new file mode 100644 index 000000000..7edcc1594 --- /dev/null +++ b/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md @@ -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": "", + "reason": "" + } + ], + "standard_files": ["", "..."], + "summary": "" +} +``` + +## 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