From 00a02b19f71049fc695fe8e8972512a7e02d54ae Mon Sep 17 00:00:00 2001 From: fullsend-code <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Mon, 15 Jun 2026 20:05:01 +0000 Subject: [PATCH 1/4] feat(#2096): add two-pass review strategy for large PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../fullsend-repo/skills/pr-review/SKILL.md | 112 ++++++++++++++++++ .../pr-review/sub-agents/security-triage.md | 95 +++++++++++++++ 2 files changed, 207 insertions(+) create mode 100644 internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md index 7d3226834..a8908b7ec 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md @@ -45,6 +45,7 @@ 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 (≥ 30 files) | | `challenger` | opus | sequential | Adversarial challenge of findings, false-positive removal, deduplication | 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. | 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 **≥ 30**, run a lightweight triage +pass to identify security-critical files before preparing context +packages. For PRs under 30 files, skip this step — all files receive +uniform attention. + +**Why:** On large PRs, 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. 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 (first 20 lines of each file's diff, enough for import + statements and function signatures). 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. 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. + +**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,6 +380,45 @@ 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 ≥ 30 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 30 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: 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..2b00ec73c --- /dev/null +++ b/internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md @@ -0,0 +1,95 @@ +--- +name: review-security-triage +description: Lightweight classifier that identifies security-critical files in large PRs for prioritized deep review. +model: haiku +--- + +# Security Triage (File Classifier) + +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 +- `.github/workflows/*.yml` — workflow files (check for `permissions:` + blocks, `secrets:` inheritance, `pull_request_target` triggers) +- `**/CODEOWNERS` — access control governance +- `**/policies/**` — policy 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 +- 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 From b058796940670d9ae443400a50b85f67806687ee Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Thu, 18 Jun 2026 19:09:50 +0000 Subject: [PATCH 2/4] fix: address review feedback on PR #2303 - Raise security triage threshold from 30 to 50 files to align with step 2's per-file diff boundary, resolving ambiguity in the 30-49 file range where per-file diffs were not available [edge-case] - Add clarifying note to roster table documenting that security-triage and challenger use non-standard dispatch and are excluded from step 4's parallel loop [logic-error, design-direction] - Clarify step 4 heading to explicitly scope dispatch to dimension sub-agents only [logic-error] - Remove parenthetical from security-triage sub-agent title to match naming convention of other sub-agents [naming-convention] Addresses review feedback on #2303 --- .../fullsend-repo/skills/pr-review/SKILL.md | 36 ++++++++++++------- .../pr-review/sub-agents/security-triage.md | 2 +- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md index a8908b7ec..780f03404 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md @@ -45,13 +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 (≥ 30 files) | +| `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 @@ -290,17 +297,20 @@ complex PR that triggers all conditions legitimately needs all 6. #### 3c-1. Security-critical file triage (large PRs) -When `FILE_COUNT` (from step 2) is **≥ 30**, run a lightweight triage +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 30 files, skip this step — all files receive +packages. For PRs under 50 files, skip this step — all files receive uniform attention. -**Why:** On large PRs, 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. See #2096 for the motivating incident. +**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:** @@ -383,7 +393,7 @@ For each selected sub-agent, assemble a context package containing: **Security-prioritized context (large PRs with triage results):** When step 3c-1 produced a security triage classification (i.e., the PR -has ≥ 30 files and the triage pass succeeded), modify the context +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 @@ -415,13 +425,15 @@ packages for the `security` and `correctness` sub-agents as follows: Security-critical files: ``` -If step 3c-1 was skipped (PR under 30 files) or the triage sub-agent +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 index 2b00ec73c..cecbed71b 100644 --- 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 @@ -4,7 +4,7 @@ description: Lightweight classifier that identifies security-critical files in l model: haiku --- -# Security Triage (File Classifier) +# 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 From 761679df02adff33d8c23b61a767d4da83654313 Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Mon, 22 Jun 2026 20:12:43 +0000 Subject: [PATCH 3/4] fix: address low-severity review findings on PR #2303 - [fail-open] Increase diff summary window from ~20 to ~50 lines for files that don't match security path patterns, giving the classifier more content signal for non-obvious security-relevant changes - [fail-open] Treat empty security_critical_files as triage failure when changed files match known security-sensitive path patterns - [api-shape] Add explicit constraint requiring raw JSON output (no markdown code fences) to security-triage sub-agent - [doc-style] Convert bold-with-colon header to proper #### heading for the security-prioritized context subsection Addresses review feedback on #2303 --- .../fullsend-repo/skills/pr-review/SKILL.md | 29 ++++++++++++++----- .../pr-review/sub-agents/security-triage.md | 3 ++ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md index 780f03404..01fac7d36 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md @@ -322,8 +322,14 @@ See #2096 for the motivating incident. **Part 2 — Context:** the PR's changed file list with per-file diff stats (additions, deletions), plus a brief diff summary for - each file (first 20 lines of each file's diff, enough for import - statements and function signatures). Format as: + 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 @@ -353,11 +359,18 @@ See #2096 for the motivating incident. `file` and `reason`), `standard_files` (array of paths), and `summary` (string). -5. 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. +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/**`), 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:** @@ -390,7 +403,7 @@ 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):** +#### 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 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 index cecbed71b..7e5f9c8aa 100644 --- 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 @@ -89,6 +89,9 @@ Return a JSON object: `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 From 0867e8eda2ab4f7a499cbcde0a78981cedab800d Mon Sep 17 00:00:00 2001 From: fullsend-fix <278716306+fullsend-ai-coder[bot]@users.noreply.github.com> Date: Mon, 29 Jun 2026 19:14:32 +0000 Subject: [PATCH 4/4] fix(pr-review): align security-triage path patterns with orchestrator protected paths Add governance and infrastructure paths (`.claude/`, `.github/`, `agents/`, `scripts/`, `skills/`, etc.) to the security-triage sub-agent's classification criteria. These paths were present in the orchestrator's step 6e protected-path list but missing from the triage classifier, which could cause files under those paths to be classified as "standard" and receive deprioritized context in large PRs. All directory paths use `/**` glob suffix to ensure recursive matching of files below the directory root. The narrower `.github/workflows/*.yml` pattern is replaced by `.github/**` to cover all GitHub configuration files. The orchestrator's fallback validation examples in step 3c-1 are updated to reference the new governance patterns. Addresses review feedback on #2303 --- .../fullsend-repo/skills/pr-review/SKILL.md | 3 +- .../pr-review/sub-agents/security-triage.md | 28 +++++++++++++++++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md index 01fac7d36..5f40ef316 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md @@ -367,7 +367,8 @@ See #2096 for the motivating incident. 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/**`), treat this as a + `**/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. 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 index 7e5f9c8aa..7edcc1594 100644 --- 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 @@ -32,11 +32,35 @@ A file is **security-critical** if it matches ANY of the following: - `**/crypto/**` — cryptographic operations - `**/token/**` or `**/tokens/**` — token handling - `**/trust/**` — trust boundary definitions -- `.github/workflows/*.yml` — workflow files (check for `permissions:` - blocks, `secrets:` inheritance, `pull_request_target` triggers) - `**/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