Skip to content

Commit 84a141c

Browse files
fullsend-ai-coder[bot]ben-alkov
authored andcommitted
feat(#2096): add two-pass review strategy for large PRs
For PRs with 30+ files, the review orchestrator now runs a lightweight security-triage pre-pass before dispatching dimension sub-agents. The triage pass uses a haiku-model sub-agent to classify changed files as security-critical or standard based on path patterns (e.g., **/mint/**, **/auth/**, **/oidc/**) and diff content heuristics (auth logic, token handling, permission changes). Security-critical files identified by the triage pass receive prioritized context in the security and correctness sub-agent context packages — their full diffs appear first with explicit classification headers, ensuring they get dedicated reasoning budget rather than competing with boilerplate changes. Changes: - New sub-agent definition: sub-agents/security-triage.md (haiku model, read-only classifier) - New orchestrator step 3c-1 in SKILL.md: security-critical file triage, runs synchronously before context package assembly - Updated step 3d in SKILL.md: security-prioritized context package assembly for security and correctness sub-agents when triage results are available - Updated sub-agent roster table with security-triage entry The 30-file threshold is a starting point that may need tuning. Triage failures fall back to uniform attention (all files treated as security-critical) to preserve existing behavior as a safe default. Closes #2096
1 parent 650261c commit 84a141c

2 files changed

Lines changed: 207 additions & 0 deletions

File tree

internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ and `description`.
4545
| `style-conventions` | sonnet | parallel | Naming, error handling idioms, API shape, code organization |
4646
| `docs-currency` | sonnet | parallel | Documentation staleness (follows docs-review skill inline) |
4747
| `cross-repo-contracts` | sonnet | parallel | API contract breakage affecting other repos (conditional) |
48+
| `security-triage` | haiku | pre-pass | Classifies files by security relevance for large PRs (≥ 30 files) |
4849
| `challenger` | opus | sequential | Adversarial challenge of findings, false-positive removal, deduplication |
4950

5051
The Model column reflects each sub-agent's current frontmatter. Any
@@ -287,6 +288,78 @@ complex PR that triggers all conditions legitimately needs all 6.
287288
| CI/CD pipeline change | correctness, security, style-conventions, intent-coherence |
288289
| DB migration + API change | correctness, security, style-conventions, cross-repo-contracts, docs-currency |
289290

291+
#### 3c-1. Security-critical file triage (large PRs)
292+
293+
When `FILE_COUNT` (from step 2) is **≥ 30**, run a lightweight triage
294+
pass to identify security-critical files before preparing context
295+
packages. For PRs under 30 files, skip this step — all files receive
296+
uniform attention.
297+
298+
**Why:** On large PRs, security-critical files compete with
299+
boilerplate for the review agent's context window and reasoning
300+
budget. A triage pass ensures files touching auth, permissions,
301+
token handling, trust boundaries, and similar concerns receive
302+
dedicated review context rather than being diluted across dozens of
303+
routine changes. See #2096 for the motivating incident.
304+
305+
**Procedure:**
306+
307+
1. Read `sub-agents/security-triage.md` for the sub-agent definition.
308+
2. Compose a spawn prompt containing:
309+
310+
**Part 1 — Sub-agent definition:** the full markdown body of the
311+
security-triage sub-agent file (everything after the frontmatter)
312+
313+
**Part 2 — Context:** the PR's changed file list with per-file
314+
diff stats (additions, deletions), plus a brief diff summary for
315+
each file (first 20 lines of each file's diff, enough for import
316+
statements and function signatures). Format as:
317+
318+
```markdown
319+
## Files to classify
320+
321+
| File | Additions | Deletions |
322+
|------|-----------|-----------|
323+
| <path> | <n> | <n> |
324+
...
325+
326+
## Diff summaries
327+
### <path>
328+
<first ~20 lines of the file's diff>
329+
...
330+
```
331+
332+
3. Spawn via Agent tool with:
333+
- `model`: `haiku` (from the sub-agent frontmatter)
334+
- `subagent_type`: `Explore` (read-only)
335+
- `prompt`: composed from parts 1–2
336+
337+
This agent runs **synchronously** (not in the background) because
338+
its output feeds into step 3d's context package assembly. It uses
339+
haiku for speed — classification does not require deep reasoning.
340+
341+
4. Parse the triage output. The security-triage sub-agent returns a
342+
JSON object with `security_critical_files` (array of objects with
343+
`file` and `reason`), `standard_files` (array of paths), and
344+
`summary` (string).
345+
346+
5. Store the classification result for use in step 3d. If the
347+
security-triage sub-agent fails (timeout, parse error, empty
348+
response), fall back to treating **all files as
349+
security-critical** — this preserves the existing uniform-attention
350+
behavior as a safe default.
351+
352+
**Edge cases:**
353+
354+
- **All files classified as security-critical:** The deep-review pass
355+
covers all files with full context. This is equivalent to the
356+
standard review behavior for smaller PRs — no degradation.
357+
- **No files classified as security-critical:** All files receive
358+
standard review. The triage cost (one haiku call) is minimal.
359+
- **Triage sub-agent failure:** Fall back to uniform attention (all
360+
files treated as security-critical). Log an info-level note in the
361+
review output.
362+
290363
#### 3d. Prepare context packages
291364

292365
For each selected sub-agent, assemble a context package containing:
@@ -307,6 +380,45 @@ For each selected sub-agent, assemble a context package containing:
307380
`intent-coherence`)
308381
- `cross_repo_context`: findings from 3a for `cross-repo-contracts`
309382

383+
**Security-prioritized context (large PRs with triage results):**
384+
385+
When step 3c-1 produced a security triage classification (i.e., the PR
386+
has ≥ 30 files and the triage pass succeeded), modify the context
387+
packages for the `security` and `correctness` sub-agents as follows:
388+
389+
1. **Security sub-agent:** Provide the full per-file diffs for all
390+
`security_critical_files` first, clearly marked with a
391+
`### Security-critical file: <path>` header and the triage reason.
392+
Include standard files' diffs after, under a
393+
`### Standard files` header. This ordering ensures
394+
security-critical files receive primary attention within the
395+
sub-agent's context window.
396+
397+
2. **Correctness sub-agent:** Same prioritized ordering — security-
398+
critical files first with their triage classification, then
399+
standard files. Correctness and security findings often overlap on
400+
the same code (e.g., a fail-open bug is both a logic error and a
401+
security vulnerability), so the correctness sub-agent also benefits
402+
from knowing which files the triage pass flagged.
403+
404+
3. **Other sub-agents** (`intent-coherence`, `style-conventions`,
405+
`docs-currency`, `cross-repo-contracts`): Receive the standard
406+
context package without prioritization. These dimensions are not
407+
affected by the security triage classification.
408+
409+
4. **Include the triage summary** in the context package for both
410+
`security` and `correctness` sub-agents:
411+
412+
```markdown
413+
### Security triage classification
414+
<triage summary from step 3c-1>
415+
Security-critical files: <list with reasons>
416+
```
417+
418+
If step 3c-1 was skipped (PR under 30 files) or the triage sub-agent
419+
failed (fallback to uniform attention), prepare all context packages
420+
using the standard format described above — no prioritization.
421+
310422
### 4. Dispatch sub-agents
311423

312424
For each selected sub-agent:
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
---
2+
name: review-security-triage
3+
description: Lightweight classifier that identifies security-critical files in large PRs for prioritized deep review.
4+
model: haiku
5+
---
6+
7+
# Security Triage (File Classifier)
8+
9+
You are a security triage classifier. Your job is to scan a PR's
10+
changed file list and diff summary to identify **security-critical
11+
files** that need dedicated review attention. You classify files — you
12+
do not review them.
13+
14+
**Own:** File classification by security relevance based on path
15+
patterns, diff content heuristics, and file purpose.
16+
17+
**Do not own:** Reviewing code, generating findings, evaluating
18+
correctness or style. You only classify.
19+
20+
## Classification criteria
21+
22+
A file is **security-critical** if it matches ANY of the following:
23+
24+
### Path patterns
25+
26+
- `**/mint/**` — token minting and signing
27+
- `**/auth/**` — authentication
28+
- `**/oidc/**` — OIDC validation
29+
- `**/rbac/**` — role-based access control
30+
- `**/permissions/**` — permission definitions
31+
- `**/secrets/**` — secret management
32+
- `**/crypto/**` — cryptographic operations
33+
- `**/token/**` or `**/tokens/**` — token handling
34+
- `**/trust/**` — trust boundary definitions
35+
- `.github/workflows/*.yml` — workflow files (check for `permissions:`
36+
blocks, `secrets:` inheritance, `pull_request_target` triggers)
37+
- `**/CODEOWNERS` — access control governance
38+
- `**/policies/**` — policy definitions
39+
40+
### Content heuristics (from diff summary)
41+
42+
- Functions or methods related to authentication, authorization, or
43+
session management
44+
- Token generation, validation, exchange, or scoping logic
45+
- Permission checks, RBAC enforcement, or access control lists
46+
- Secret handling, key management, or credential storage
47+
- OIDC claims parsing, JWT validation, or certificate verification
48+
- Workflow permission declarations or secret exposure
49+
- Trust boundary enforcement or sandbox escape vectors
50+
- Input validation or sanitization for injection defense
51+
52+
### File type signals
53+
54+
- Go files importing `crypto/*`, `oauth2`, `jwt`, or auth-related
55+
packages
56+
- Configuration files declaring permissions, roles, or access policies
57+
- Terraform/IAM/Kubernetes RBAC manifests
58+
- GitHub Actions workflow files with `permissions:` blocks
59+
60+
## Procedure
61+
62+
1. Review the full list of changed files and their diff stats
63+
(additions, deletions, changes summary).
64+
2. For each file, evaluate against the classification criteria above.
65+
3. If in doubt, classify as security-critical — false positives are
66+
acceptable, false negatives are not.
67+
4. Return the classification result.
68+
69+
## Output format
70+
71+
Return a JSON object:
72+
73+
```json
74+
{
75+
"security_critical_files": [
76+
{
77+
"file": "<relative path>",
78+
"reason": "<brief reason for classification>"
79+
}
80+
],
81+
"standard_files": ["<relative path>", "..."],
82+
"summary": "<one-line summary, e.g., '5 of 42 files classified as security-critical'>"
83+
}
84+
```
85+
86+
## Constraints
87+
88+
- Classify ALL files — every file must appear in either
89+
`security_critical_files` or `standard_files`
90+
- Err on the side of inclusion — when uncertain, mark as
91+
security-critical
92+
- Do not read file contents beyond what is provided in the diff
93+
summary — this is a fast classification pass
94+
- Do not write any files
95+
- Do not generate review findings

0 commit comments

Comments
 (0)