feat(review-agent): add two-pass review strategy for large PRs

[ben-alkov]

Cloned from fullsend-ai/fullsend#2303 (CLOSED)
Original author: @fullsend-ai-coder (bot) | Created: 2026-06-15T20:06:19Z
Source PR: fullsend-ai/fullsend#2303

---

For PRs with 50+ 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 50-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 fullsend-ai/fullsend#2096

Post-script verification

• Branch is not main/master (agent/2096-two-pass-review-strategy)
• Secret scan passed (gitleaks — f119a205ffb44da8687d9ff1673b6b2d2de2e927..HEAD)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

---

Original PR metadata

Field	Value
State	CLOSED (not merged)
Base	main ← agent/2096-two-pass-review-strategy
Review decision	CHANGES_REQUESTED
Labels	requires-manual-review. Skipped (not in target): agent/review, component/harness, component/skills
Milestone	None
Assignees	ben-alkov
Reactions	None
Closing issues	fullsend-ai/fullsend#2096
Files changed	2 (+261 -1)

[ben-alkov]

Comment by @github-actions[bot] on 2026-06-15T20:06:40Z (source)

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[ben-alkov]

Comment by @github-actions[bot] on 2026-06-15T20:08:43Z (source)

Site preview

Preview: https://d6666021-site.fullsend-ai.workers.dev

Commit: 0867e8eda2ab4f7a499cbcde0a78981cedab800d

[ben-alkov]

Comment by @codecov[bot] on 2026-06-15T20:09:57Z (source)

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[ben-alkov]

Comment by @fullsend-ai-review[bot] on 2026-06-15T20:10:07Z (source)

🤖 Finished Review · ✅ Success · Started 8:10 PM UTC · Completed 8:24 PM UTC
Commit: 4c0bf7e · View workflow run →

[ben-alkov]

Comment by @fullsend-ai-review[bot] on 2026-06-15T20:24:03Z (source)

Review

Findings

Medium

• [protected-path] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md, internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — Both modified files are scaffold source for skill definitions deployed to all target repos via internal/scaffold/fullsend-repo/skills/. Changes here alter the review agent's behavior across all fullsend-managed repositories. The PR links to issue Review agent: two-pass strategy for large PRs — triage security-critical files, then deep-review fullsend#2096 and explains the rationale for the changes (two-pass review strategy for large PRs with security-prioritized context allocation). Human approval is always required for protected-path changes, regardless of context.

---

Labels: PR modifies review agent skill definitions and adds a new sub-agent under skills/pr-review/.

Previous run

Review

Findings

Medium

• [protected-path] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md, internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — Both modified files are under the skills/ protected path. The PR links to issue Review agent: two-pass strategy for large PRs — triage security-critical files, then deep-review fullsend#2096 and explains the rationale for the changes. Human approval is always required for protected-path changes, regardless of context.

• [logic-error] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The PR body is internally contradictory about the file-count threshold: the first paragraph correctly states "For PRs with 50+ files" (matching the implementation), but the last paragraph states "The 30-file threshold is a starting point that may need tuning." The implementation consistently uses 50, which aligns with step 2's per-file diff boundary. The PR description should be updated to avoid misleading reviewers.
  Remediation: Update the PR body's last paragraph to say "The 50-file threshold" instead of "The 30-file threshold."

Low

• [logic-error] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — Step 3c says "All selected sub-agents run in parallel" without explicitly excluding security-triage from the selection pool. While step 3c only enumerates the 6 dimension sub-agents by name in its selection lists, and both step 4 and the "Non-standard dispatch types" paragraph clarify the exclusion, adding an explicit note to step 3c would improve clarity for future implementors.
  Remediation: Add a brief note to step 3c: "security-triage and challenger are not dimension sub-agents and are not selected here."

• [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The security triage step (3c-1) provides only the first ~20 lines of diff for path-matched files and ~50 lines for non-matched files to the Haiku classifier. Security-relevant changes beyond these thresholds could be missed by content heuristics, causing a file to be classified as "standard" and receive deprioritized context. Partially mitigated by path-pattern heuristics (path alone triggers classification), the "err on the side of inclusion" instruction, and the fail-closed fallback.

• [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — The triage sub-agent's path pattern list does not include several security-sensitive paths from the orchestrator's protected-path list (e.g., .github/ beyond workflows, .claude/, root-level policies/). A file under .claude/ could be classified as "standard" by triage, receiving deprioritized context. Mitigated by step 6e's protected-path check running independently in the orchestrator regardless of triage classification.

• [architectural-conflict] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The PR adds a new "pre-pass" dispatch type that extends the existing LLM-based orchestration pattern, which the SKILL.md header (lines 13–19) already acknowledges departs from ADR-0018's prohibition on LLM-driven orchestration. No superseding ADR exists. This is pre-existing architectural debt incrementally deepened by this PR, not introduced by it.

• [design-smell] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — Step 4 carves out security-triage and challenger as non-dimension sub-agents, and the roster table now mixes review dimension agents with orchestration helpers. The "Non-standard dispatch types" paragraph documents this distinction, but the overloaded roster may create confusion as the sub-agent count grows.

Previous run (2)

Review

Findings

Medium

• [logic-error] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The PR body states the threshold is "30+ files" but the SKILL.md implementation consistently uses 50 files as the threshold ("≥ 50" in step 3c-1, "< 50 files" in step 3d). The implementation is internally consistent and the 50-file threshold is defensible (aligns with step 2's per-file diff boundary), but the PR description will mislead reviewers about what was actually implemented.
  Remediation: Update the PR body to state the threshold is 50 files (not 30), or add a note explaining why the threshold was raised from the issue's suggested 30 to 50.

Low

• [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The security triage step (3c-1) provides only the "first ~20 lines" of each file's diff summary to the Haiku classifier. Security-relevant changes beyond line 20 could cause a file to be categorized as "standard" and receive diluted review attention. Partially mitigated by path-pattern heuristics in the triage sub-agent.

• [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — An empty security_critical_files array from the triage agent is accepted at face value ("all files get standard review") rather than triggering the failure fallback. A manipulated or confused triage output returning an empty classification would degrade security review quality without triggering the safe fallback. Consider treating an empty array the same as a triage failure when the PR contains files matching known security-sensitive path patterns.

• [api-shape] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — The security-triage sub-agent's output format (JSON object with security_critical_files, standard_files, summary) differs from the standard findings array. The sub-agent definition does not explicitly state that output must be valid JSON (not wrapped in markdown code fences), which could cause parsing failures in step 3c-1.

• [doc-style] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The section header "Security-prioritized context (large PRs with triage results):" uses bold text with trailing colon rather than a heading marker (####). While bold-with-colon is used elsewhere for inline categorical labels, this header introduces a substantial sub-section with its own numbered list.

Previous run (3)

Review

Findings

Low

• [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — Classification criteria use **/CODEOWNERS (glob matching at any depth) while SKILL.md's protected paths use CODEOWNERS (root-level prefix match). Minor inconsistency — both match root-level CODEOWNERS, and nested CODEOWNERS files are legitimately security-relevant, so the triage agent's broader glob is defensible.

• [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The security triage step (3c-1) provides only the "first 20 lines" of each file's diff summary to the Haiku classifier. Security-relevant changes that appear beyond line 20 of a file's diff could be missed by the classifier, causing the file to be categorized as "standard" and receive diluted security review attention. The fallback only triggers on outright triage failure, not on subtle misclassification. Consider increasing the diff summary window for files that don't match any path pattern, or adding a secondary keyword-based heuristic.

• [architectural-conflict] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — This PR adds another LLM-based orchestration decision to a system that already acknowledges a conflict with ADR-0018's prohibition on LLM-based orchestration. The SKILL.md header notes a superseding ADR is needed. This is pre-existing architectural debt that this PR incrementally deepens. Consider filing a follow-up issue to write the superseding ADR.

• [api-shape] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — The security-triage sub-agent's output format (JSON object with security_critical_files, standard_files, summary) differs from the standard findings array format. The "Non-standard dispatch types" paragraph provides a high-level warning, and step 3c-1 point 4 documents the structure, but an explicit "different format" parsing note mirroring step 6d-4's clarity would improve consistency.

• [doc-style] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — Step 3c-1 uses a numbered sub-step scheme (3c-1) while existing steps use letter-based sub-steps (2a, 3a, 3b). The 3a-1 precedent exists for budget allocation priority, but the pattern is unusual.

• [doc-style] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — Frontmatter uses model: haiku (bare name) while some sub-agents use fully qualified names like claude-sonnet-4-6@default. Consistent with the opus bare-name pattern but inconsistent with sonnet sub-agents.

Info

• [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — The triage agent uses Haiku (the weakest available model) for security-critical classification decisions. The "err on the side of inclusion" instruction and path-pattern matching mitigate most risk. The design accepts this tradeoff for latency/cost reasons.

• [scope-alignment] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The PR implements a 50-file threshold while issue Review agent: two-pass strategy for large PRs — triage security-critical files, then deep-review fullsend#2096 suggests 30 files as a starting point. The discrepancy is a reasonable engineering judgment — aligning with step 2's per-file diff boundary ensures per-file diff summaries are always available for the triage prompt.

• [design-direction] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The PR now explicitly acknowledges the third sub-agent pattern (security-triage as pre-pass) via the "Non-standard dispatch types" note, addressing the prior review's recommendation to document the distinction.

• [pattern-consistency] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The sub-agent roster table introduces a new "pre-pass" dispatch type distinct from "parallel" and "sequential". Semantically accurate for the new workflow.

• [pattern-consistency] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — The sub-agent follows the established structure pattern (frontmatter, H1 title, role statement, Own/Do not own sections, procedure, output format, constraints).

• [doc-style] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The section header "Security-prioritized context (large PRs with triage results):" uses bold text with trailing colon rather than heading markers. Appropriate as a variant within step 3d rather than a standalone section.

Previous run (4)

Review

Findings

Medium

• [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — Step 3c-1 triggers the security triage pre-pass when FILE_COUNT >= 30, but step 2 only produces per-file diffs when FILE_COUNT >= 50. For PRs with 30–49 files, step 2 uses gh pr diff (a single unified diff blob). Step 3c-1's procedure asks the orchestrator to compose "first ~20 lines of each file's diff" as diff summaries, and step 3d's security-prioritized context references "full per-file diffs for all security_critical_files first" — both presuppose per-file diffs are available, but they are not produced until FILE_COUNT >= 50. This creates an ambiguous gap for PRs in the 30–49 file range.
  Remediation: Either lower step 2's per-file diff threshold from 50 to 30 to align with the triage trigger, or add explicit instructions in step 3c-1 for extracting per-file diff segments from the unified gh pr diff output when FILE_COUNT is between 30 and 49.

Low

• [logic-error] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The security-triage sub-agent is listed in the roster table with dispatch type pre-pass, but step 3c and step 4 do not explicitly exclude pre-pass type agents from the selection and dispatch loop. Double-dispatch is unlikely in practice (step 3c lists only dimension agents, and the challenger precedent shows non-parallel types are handled separately), but a clarifying note would remove ambiguity.

• [architectural-conflict] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — This PR adds another LLM-based orchestration decision to a system that already acknowledges a conflict with ADR-0018's prohibition on LLM-based orchestration. The SKILL.md header notes a superseding ADR is needed. This is a pre-existing concern not introduced by this PR, but each incremental addition deepens the unresolved debt.

• [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — Classification criteria use **/CODEOWNERS (glob matching at any depth) while SKILL.md's protected paths use CODEOWNERS (root-level prefix match). Minor inconsistency — both would match root-level CODEOWNERS, and nested CODEOWNERS files are legitimately security-relevant.

• [design-direction] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The security-triage sub-agent is structurally different from the six dimension sub-agents: it produces file classifications (not review findings) and runs as a preprocessing step. This creates a third sub-agent pattern alongside dimension agents (parallel) and the challenger (sequential). Consider documenting this distinction explicitly in the roster section.

• [api-shape] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — The output format (JSON object with security_critical_files, standard_files, summary) differs from the standard findings array. Step 3c-1 point 4 documents the expected structure, but lacks the explicit "different format" parsing note that step 6d-4 provides for the challenger's non-standard output.

• [doc-style] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — Step 3c-1 uses a numbered sub-step scheme (3c-1) while existing steps use letter-based sub-steps (2a, 3a, 3b, etc.). The 3a-1 precedent exists for budget allocation priority, but the pattern is unusual within the document.

Info

• [pattern-inconsistency] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The sub-agent roster table introduces a new pre-pass dispatch type distinct from parallel and sequential. Semantically accurate for the new workflow.

• [design-pattern] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The security-triage sub-agent introduces a preprocessing classification pattern to inform context allocation for downstream review agents, aligning with issue Review agent: two-pass strategy for large PRs — triage security-critical files, then deep-review fullsend#2096's requirement.

---

Labels: PR modifies review agent skill definitions and sub-agent roster, which is harness/skills infrastructure.

Previous run (5)

Review

Findings

Medium

• [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — Step 3c-1 triggers the security triage pre-pass when FILE_COUNT >= 30, but step 2 only produces per-file diffs for FILE_COUNT >= 50. The triage procedure references "first 20 lines of each file's diff" and step 3d's security-prioritized context references "full per-file diffs for all security_critical_files first" — but for PRs with 30–49 files, step 2 uses gh pr diff (a single unified blob), so per-file diff summaries are not directly available. This creates ambiguity for the orchestrator in the 30–49 file range.
  Remediation: Either raise the triage threshold to 50 to align with step 2's large-PR boundary, or add explicit instructions for how to produce per-file diff summaries when the PR has 30–49 files (e.g., by splitting the unified diff on file headers, or by running per-file diffs specifically for the triage step).

• [architectural-conflict] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — This PR adds another LLM-based orchestration decision (security-triage pre-pass) to a system that already acknowledges a conflict with ADR-0018's prohibition on LLM-based orchestration. The SKILL.md header (lines 12–19) notes "A superseding ADR is needed to formally retire ADR-0018's prohibition." Each incremental addition to the LLM orchestrator deepens this unresolved architectural debt.
  Remediation: Write and accept a superseding ADR that formally retires ADR-0018's prohibition on LLM-based orchestration for the pr-review skill.

Low

• [logic-error] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The security-triage sub-agent is listed in the roster table with dispatch type pre-pass, but nothing in the document explicitly excludes it from step 4's "For each selected sub-agent" dispatch loop. Step 3c's selection lists only dimension agents, so double-dispatch is unlikely in practice, but a clarifying note would prevent ambiguity.

• [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The haiku model used for security triage may not reliably detect subtle security relevance (e.g., utility functions that handle token scoping, or files whose security relevance is only apparent from content rather than path). A misclassified file still appears in the security sub-agent's context under "Standard files" so it is still reviewed, but receives less prominent positioning.

• [design-direction] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The security-triage sub-agent is structurally different from the six dimension sub-agents: it produces file classifications rather than review findings and runs as a preprocessing step. Listing it in the roster table alongside dimension sub-agents may create a misleading abstraction. The challenger sub-agent has a similar deviation (sequential dispatch, different output format), establishing precedent, but the difference could be made more explicit.

• [naming-convention] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — The H1 title # Security Triage (File Classifier) includes a parenthetical clarification not present in other sub-agent titles (Correctness, Security, Challenger). Minor deviation from established convention.

• [api-shape] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — The output format (JSON object with security_critical_files, standard_files, summary) differs from the standard findings array. Step 3c-1 point 4 documents this format, analogous to step 6d's documentation of the challenger's format.

• [doc-style] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — Step 3c-1 uses a 3c-1 numbering scheme. The existing document uses 3a-1 for budget allocation priority, so this follows precedent, but the third-level numbering pattern is unusual.

• [edge-case] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — Classification criteria use **/CODEOWNERS (glob matching at any depth) while SKILL.md's protected paths use CODEOWNERS (prefix matching at repo root). Functionally different but both serve their respective purposes.

• [scope-exceeded] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The PR title says "two-pass review strategy" and the implementation does involve two passes (triage then review), but the review pass uses context reordering rather than separate file-set dispatches. This is a reasonable design choice but may warrant a clarifying note.

Info

• [design-smell] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The edge cases section specifies logging an info-level note when the triage pass fails, which provides basic observability. Consider whether deeper alerting or metrics would help track whether the feature is providing value.

• [pattern-inconsistency] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md — The sub-agent roster table introduces a new pre-pass dispatch type distinct from parallel and sequential. This is intentional and well-documented.

[ben-alkov]

Comment by @fullsend-ai-review[bot] on 2026-06-18T18:57:50Z (source)

🤖 Review · ❌ Terminated · Started 6:57 PM UTC · Ended 7:10 PM UTC
Commit: 4e21a60 · View workflow run →

[ben-alkov]

Comment by @ben-alkov on 2026-06-18T18:59:34Z (source)

/fs-fix but ignore

• [architectural-conflict] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md
• [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md
• info-level findings

[ben-alkov]

Comment by @fullsend-ai-coder[bot] on 2026-06-18T19:03:40Z (source)

🤖 Finished Fix · ✅ Success · Started 7:03 PM UTC · Completed 7:11 PM UTC
Commit: 4e21a60 · View workflow run →

[ben-alkov]

Comment by @fullsend-ai-review[bot] on 2026-06-18T19:10:39Z (source)

🤖 Finished Review · ✅ Success · Started 6:57 PM UTC · Completed 7:10 PM UTC
Commit: 84a141c · View workflow run →

[ben-alkov]

Comment by @fullsend-ai-coder[bot] on 2026-06-18T19:11:47Z (source)

🔧 Fix agent — iteration 1 (human-triggered)

Addressed 4 of 8 non-excluded review findings (1 medium, 3 low). Fixed threshold mismatch by raising triage trigger from 30 to 50 files to align with step 2's per-file diff boundary. Added clarifying notes for non-standard dispatch types in roster table and step 4. Removed parenthetical from sub-agent title. Disagreed with 4 findings that were already documented, followed existing convention, or were out of scope. Per human instruction, ignored [architectural-conflict], [fail-open], and info-level findings.

Fixed (4):

1. [edge-case] threshold mismatch between step 2 (>=50) and step 3c-1 (>=30) (internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md): Raised security triage threshold from 30 to 50 files to align with step 2's per-file diff boundary. Updated all references (step 3c-1 trigger, Why section, step 3d context, and fallback clause). Added explanation that the threshold aligns with step 2 so per-file diff summaries are always available.
2. [logic-error] security-triage not explicitly excluded from step 4 dispatch loop (internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md): Added clarifying note to step 4 heading that dispatch applies to dimension sub-agents only, explicitly excluding security-triage (step 3c-1) and challenger (step 6d).
3. [design-direction] security-triage structurally different from dimension sub-agents in roster table (internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md): Added 'Non-standard dispatch types' paragraph after the roster table explaining that security-triage and challenger are not dimension sub-agents, documenting their separate dispatch points and different output formats.
4. [naming-convention] parenthetical in security-triage H1 title (internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md): Removed '(File Classifier)' parenthetical from H1 title to match the naming convention of other sub-agents (Correctness, Security, Challenger).

Disagreed (4):

1. [api-shape] output format differs from standard findings array: The reviewer notes this is already documented in step 3c-1 point 4, analogous to the challenger's format documentation in step 6d. No change needed — the difference is intentional and documented.
2. [doc-style] 3c-1 numbering scheme: The reviewer acknowledges this follows the existing 3a-1 precedent. Consistent with established convention — no change needed.
3. [edge-case] CODEOWNERS glob pattern difference between security-triage and SKILL.md protected paths: The reviewer notes these are 'functionally different but both serve their respective purposes.' The security-triage classifier uses glob patterns for file matching across any depth, while SKILL.md's protected paths use prefix matching at repo root. Both are correct for their contexts.
4. [scope-exceeded] two-pass naming vs implementation details: This concerns the PR title, which is outside the scope of code fixes. The implementation does involve two passes (triage then review with prioritized context), and the reviewer notes it is 'a reasonable design choice.'

Tests: passed

_{Updated by fullsend fix agent}

[ben-alkov]

Comment by @fullsend-ai-review[bot] on 2026-06-18T19:15:27Z (source)

🤖 Finished Review · ✅ Success · Started 7:15 PM UTC · Completed 7:28 PM UTC
Commit: 9a7370e · View workflow run →

[ben-alkov]

Comment by @fullsend-ai-review[bot] on 2026-06-22T19:39:38Z (source)

🤖 Finished Review · ✅ Success · Started 7:39 PM UTC · Completed 7:53 PM UTC
Commit: 82e38e3 · View workflow run →

[ben-alkov]

Comment by @ben-alkov on 2026-06-22T19:58:53Z (source)

/fs-fix Address these Low review findings

• [fail-open] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md
• [api-shape] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md
• [doc-style] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:56 PM UTC · Completed 9:15 PM UTC
Commit: 4856ff9 · View workflow run →

[ben-alkov]

Comment by @fullsend-ai-coder[bot] on 2026-06-22T20:02:43Z (source)

🤖 Finished Fix · ✅ Success · Started 8:02 PM UTC · Completed 8:14 PM UTC
Commit: 4e21a60 · View workflow run →

[ben-alkov]

Comment by @fullsend-ai-coder[bot] on 2026-06-22T20:14:23Z (source)

🔧 Fix agent — iteration 2 (human-triggered)

Addressed all 4 low-severity review findings (2 fail-open, 1 api-shape, 1 doc-style). Hardened triage classification with larger diff windows for non-path-matched files, empty-result validation against path patterns, explicit raw JSON output constraint, and corrected heading level.

Fixed (4):

1. [fail-open] ~20 lines of diff summary may miss security-relevant changes (internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md): Increased diff summary window from ~20 to ~50 lines for files that don't match security path patterns. Path-matched files keep the ~20 line window since path patterns are sufficient for classification. Non-path-matched files get ~50 lines to detect security-relevant changes in diff content.
2. [fail-open] empty security_critical_files accepted without validation (internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md): Added validation in step 3c-1 point 5: if security_critical_files is empty but changed files match known security-sensitive path patterns, treat as triage failure and fall back to uniform attention. Prevents a confused or manipulated classifier from silently downgrading security review.
3. [api-shape] security-triage output format lacks explicit JSON requirement (internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md): Added explicit constraint requiring raw JSON output without markdown code fences. The orchestrator parses the response directly as JSON, so wrapped output would cause parsing failures in step 3c-1.
4. [doc-style] bold-with-colon header instead of heading marker (internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md): Converted 'Security-prioritized context (large PRs with triage results):' from bold text with trailing colon to a proper #### heading, matching the document's convention for substantial subsections with their own numbered lists.

Tests: passed

_{Updated by fullsend fix agent}

[ben-alkov]

Comment by @fullsend-ai-review[bot] on 2026-06-22T20:18:07Z (source)

🤖 Finished Review · ✅ Success · Started 8:18 PM UTC · Completed 8:35 PM UTC
Commit: 4216d02 · View workflow run →

[ben-alkov]

Comment by @ben-alkov on 2026-06-29T19:05:49Z (source)

/fs-fix Address

[fail-open] internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md — The triage sub-agent's path pattern list does not include several security-sensitive paths from the orchestrator's protected-path list (e.g., .github/ beyond workflows, .claude/, root-level policies/). A file under .claude/ could be classified as "standard" by triage, receiving deprioritized context. Mitigated by step 6e's protected-path check running independently in the orchestrator regardless of triage classification.

and also make sure that all of the security-sensitive paths in the orchestrator, subagents, and related skills are guarded again recursion (i.e. paths below e.g. ".claude/" must also be protected).

[ben-alkov]

Comment by @fullsend-ai-coder[bot] on 2026-06-29T19:08:59Z (source)

🤖 Finished Fix · ✅ Success · Started 7:08 PM UTC · Completed 7:16 PM UTC
Commit: 657bf48 · View workflow run →

[ben-alkov]

Comment by @fullsend-ai-coder[bot] on 2026-06-29T19:16:31Z (source)

🔧 Fix agent — iteration 3 (human-triggered)

Aligned security-triage sub-agent path patterns with the orchestrator's full protected-path list. Added 16 governance/infrastructure paths as a new classification category with recursive glob patterns. Updated orchestrator fallback validation examples to reference the new patterns.

Fixed (3):

1. security-triage missing protected paths from orchestrator (internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md): Added a new 'Governance and infrastructure paths' subsection to the security-triage sub-agent's classification criteria, covering all protected paths from the orchestrator's step 6e list: .claude/, .cursor/, .gitattributes, .github/, .pre-commit-config.yaml, AGENTS.md, agents/, api-servers/, CLAUDE.md, Containerfile, Dockerfile, harness/, images/, plugins/, scripts/, skills/. The narrower .github/workflows/*.yml pattern was replaced by .github/** to cover all GitHub configuration.
2. ensure recursive matching for directory paths (internal/scaffold/fullsend-repo/skills/pr-review/sub-agents/security-triage.md): All directory paths use /** glob suffix (e.g., .claude/, agents/, scripts/**) to ensure files below the directory root are matched recursively. Single-file entries (.gitattributes, AGENTS.md, CLAUDE.md, Containerfile, Dockerfile, .pre-commit-config.yaml) use exact match since they are individual files, not directories.
3. update orchestrator fallback validation examples (internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md): Updated the fallback validation example list in step 3c-1 to include governance path patterns (.claude/, .github/, agents/, scripts/) alongside the existing examples, so the orchestrator validates against the full set of security-sensitive patterns when checking for triage failures.

Tests: passed

_{Updated by fullsend fix agent}

[ben-alkov]

Comment by @fullsend-ai-review[bot] on 2026-06-30T17:42:36Z (source)

🤖 Finished Review · ❌ Failure · Started 5:42 PM UTC · Completed 5:49 PM UTC
Commit: 104508d · View workflow run →

[ben-alkov]

Comment by @ben-alkov on 2026-06-30T19:21:58Z (source)

/fs-review

[ben-alkov]

Comment by @fullsend-ai-review[bot] on 2026-06-30T19:25:23Z (source)

🤖 Finished Review · ✅ Success · Started 7:25 PM UTC · Completed 7:37 PM UTC
Commit: f1117dd · View workflow run →

[qodo-code-review]

PR Summary by Qodo

Add security-triage pre-pass to prioritize security review in large PRs

✨ Enhancement 📝 Documentation 🕐 10-20 Minutes

AI Description

• Add a haiku-based security-triage pre-pass for PRs with ≥50 changed files.
• Prioritize security-critical diffs in security/correctness context packages using triage results.
• Document triage fallback behavior and clarify non-standard sub-agent dispatch semantics.

Diagram

graph TD
  Orch["PR review orchestrator"] --> Fetch["Step 2: file stats + diffs"] --> Triage[["security-triage" sub-agent (haiku)]] --> Assemble["Step 3d: build context packages"]
  Assemble --> SecCtx["Security context (prioritized)"] --> Sec[["security" sub-agent]]
  Assemble --> CorCtx["Correctness context (prioritized)"] --> Cor[["correctness" sub-agent]]
  Assemble --> StdCtx["Other contexts (standard)"] --> Other[["other dimension" sub-agents]]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Deterministic path-based prioritization (no LLM triage)

• ➕ Fully deterministic and easy to test/reason about
• ➕ No extra model call latency/cost
• ➕ Avoids parse/timeout failure modes entirely
• ➖ Misses security-relevant changes in non-obvious paths
• ➖ Requires continual manual upkeep of path allow/deny patterns

2. Inline prioritization inside security/correctness sub-agent prompts

• ➕ No new orchestration step or JSON parsing contract
• ➕ Keeps responsibility localized to the consuming agents
• ➖ Competes with deep review for context/reasoning budget (the core problem this PR targets)
• ➖ Harder to ensure consistent ordering across agents

3. Trigger triage by line count / diff size instead of file count

• ➕ Better correlates with context pressure than raw file count
• ➕ Avoids triaging PRs with many tiny files where prioritization may not help
• ➖ More complicated thresholding and more edge cases
• ➖ Still needs per-file summaries to be reliable for classification

Recommendation: Keep the PR’s two-pass approach: a fast, synchronous triage that produces a machine-readable classification, then deterministic context reordering for security/correctness. This most directly addresses context-window dilution on very large PRs while keeping the deep-review agents focused on reviewing rather than sorting. If the triage step proves noisy, consider tightening the criteria with a small deterministic path-only baseline before resorting to more complex trigger logic.

Files changed (2) +261 / -1

Enhancement (2) +261 / -1

SKILL.mdDocument security-triage pre-pass and prioritized context packaging +139/-1

Document security-triage pre-pass and prioritized context packaging

• Adds a new step (3c-1) to run a synchronous security-critical file triage when FILE_COUNT ≥ 50, including failure fallbacks. Updates step 3d to reorder/annotate diffs for the security and correctness sub-agents based on triage results, and clarifies that security-triage/challenger are non-standard dispatch types.

skills/pr-review/SKILL.md

security-triage.mdAdd haiku security triage sub-agent definition and JSON output contract +122/-0

Add haiku security triage sub-agent definition and JSON output contract

• Introduces a read-only classifier sub-agent that labels changed files as security-critical vs standard using path patterns and diff heuristics. Defines strict output requirements (raw JSON, full coverage of files) and an inclusion-biased policy to minimize false negatives.

skills/pr-review/sub-agents/security-triage.md

[fullsend-ai-review]

Review

Findings

Medium

• [logic-error] skills/pr-review/SKILL.md — Step 3f instructs the orchestrator to modify context packages for the security and correctness sub-agents when triage results exist, but step 3c makes security sub-agent dispatch conditional. A large PR can trigger the triage pass and classify files as security-critical based on governance path patterns (e.g., skills/**, .github/**) without the step 3c domain classifier selecting the security sub-agent. Step 3f would then reference a context package that does not exist. The correctness sub-agent (always dispatched) still receives prioritized context, partially mitigating this gap.
  Remediation: When step 3c-1 produces a non-empty security_critical_files list, auto-include the security sub-agent in the dispatch set.

• [pattern-inconsistency] skills/pr-review/sub-agents/security-triage.md:2 — The frontmatter name field uses review-security-triage, deviating from the convention where all other sub-agent files use a name matching the filename stem exactly (correctness for correctness.md, challenger for challenger.md, etc.).
  Remediation: Change name: review-security-triage to name: security-triage.

• [pattern-inconsistency] skills/pr-review/sub-agents/security-triage.md — The frontmatter is missing tools, permissionMode, and background fields present in all seven other sub-agent files. The background field is particularly relevant: background: false would document the synchronous execution mode specified in step 3c-1.
  Remediation: Add the missing fields to match the established pattern.

• [protected-path] skills/pr-review/SKILL.md — This PR modifies 2 files under the protected path skills/: skills/pr-review/SKILL.md and skills/pr-review/sub-agents/security-triage.md. The PR links to issue Review agent: two-pass strategy for large PRs — triage security-critical files, then deep-review fullsend#2096 and the PR description explains the rationale. Human approval is always required for protected-path changes regardless of context.

Low

• [edge-case] skills/pr-review/SKILL.md — The empty-classification guard uses an e.g. qualifier listing 7 of ~22+ path patterns from the triage sub-agent's classification criteria. While LLM orchestrators may generalize from examples, the ambiguity weakens this safety-critical fallback.
  Remediation: Replace e.g. with an explicit reference to the full classification criteria in sub-agents/security-triage.md.

• [fail-open] skills/pr-review/SKILL.md — The security-prioritized context mechanism (step 3f) is a soft positional prioritization in the context window. A misclassification by the haiku triage model degrades review quality without producing a detectable failure signal. Partially mitigated by the triage classifier's "err on inclusion" instruction, the fail-closed fallback, and the new structural validation with path-pattern override.

• [code-organization] skills/pr-review/SKILL.md — The security-triage sub-agent is not listed in the sub-agent roster table, though all other sub-agents — including the non-dimension challenger — have rows. The PR body claims "Updated sub-agent roster table with security-triage entry" but only a note below the table was added.
  Remediation: Add a row to the roster table with dispatch type pre-pass.

• [incomplete-doc] docs/review.md:12 — The description states the orchestrator "dispatches specialized sub-agents in parallel" but does not mention the security-triage pre-pass introduced by this PR. The two-pass review strategy for large PRs is a significant behavioral change not reflected in user-facing documentation.
  Remediation: Add a sentence noting the security-triage pre-pass for large PRs.

---

Resolved since prior review: [edge-case] partial failure validation — now addressed by the path-pattern override mechanism that authoritatively reclassifies files matching known path patterns. [edge-case] completeness invariant — now addressed by the structural validation that verifies the union of classified files exactly equals the changed-file set.

Previous run

Review

Findings

Medium

• [edge-case] skills/pr-review/SKILL.md — The validation logic in step 3c-1 (procedure step 5) only catches total triage failures: it fires when security_critical_files is empty but path-pattern matches exist. It does not detect partial failures where the classifier returns a non-empty security_critical_files array but omits some path-pattern-matching files, placing them in standard_files. For example, if 3 of 5 **/auth/** files are classified as security-critical but the other 2 are placed in standard_files, the validation passes because security_critical_files is non-empty, even though the classifier missed obvious signals on the 2 omitted files.
  Remediation: Add a validation step that checks whether every changed file matching a security path pattern appears in security_critical_files. If any path-pattern-matching file is found only in standard_files, either reclassify it as security-critical or treat the triage as failed.

• [logic-error] skills/pr-review/SKILL.md — Step 3f instructs the orchestrator to modify context packages for the security and correctness sub-agents when triage results exist, but step 3c makes security sub-agent dispatch conditional. A large PR (≥ 50 files) can trigger the triage pass and classify files as security-critical based on governance path patterns (e.g., skills/**, .github/**) without the step 3c domain classifier selecting the security sub-agent. Step 3f would then reference a context package that does not exist. The correctness sub-agent (always dispatched) still receives prioritized context, partially mitigating this gap.
  Remediation: When step 3c-1 produces a non-empty security_critical_files list, auto-include the security sub-agent in the dispatch set.

• [pattern-inconsistency] skills/pr-review/sub-agents/security-triage.md:3 — The frontmatter name field uses review-security-triage, deviating from the convention where all other sub-agent files use a name matching the filename stem exactly (correctness for correctness.md, challenger for challenger.md, etc.).
  Remediation: Change name: review-security-triage to name: security-triage.

• [pattern-inconsistency] skills/pr-review/sub-agents/security-triage.md — The frontmatter is missing tools, permissionMode, and background fields present in all seven other sub-agent files. The background field is particularly relevant: background: false would document the synchronous execution mode specified in step 3c-1.
  Remediation: Add the missing fields to match the established pattern.

• [protected-path] skills/pr-review/SKILL.md — This PR modifies 2 files under the protected path skills/: skills/pr-review/SKILL.md and skills/pr-review/sub-agents/security-triage.md. The PR links to issue Review agent: two-pass strategy for large PRs — triage security-critical files, then deep-review fullsend#2096 and the PR description explains the rationale. Human approval is always required for protected-path changes regardless of context.

Low

• [edge-case] skills/pr-review/SKILL.md — The fallback validation in step 3c-1 uses an e.g. qualifier listing 7 of ~22+ path patterns from the triage sub-agent's classification criteria. While LLM orchestrators may generalize from examples, the ambiguity weakens this safety-critical fallback. See also: [fail-open] finding at this location.
  Remediation: Replace e.g. with an explicit reference to the full classification criteria in sub-agents/security-triage.md.

• [fail-open] skills/pr-review/SKILL.md — The security-prioritized context mechanism (step 3f) is a soft positional prioritization in the context window. A misclassification by the haiku triage model degrades review quality without producing a detectable failure signal. Partially mitigated by the triage classifier's "err on inclusion" instruction and the fail-closed fallback.

• [edge-case] skills/pr-review/sub-agents/security-triage.md — The sub-agent constraints require "Classify ALL files" but step 3c-1 does not validate this completeness invariant. If the classifier omits a file from both arrays, step 3f's prioritization logic would silently skip it. See also: [fail-open] finding at this location.

• [code-organization] skills/pr-review/SKILL.md — The security-triage sub-agent is not listed in the sub-agent roster table, though all other sub-agents — including the non-dimension challenger — have rows. The PR body claims "Updated sub-agent roster table with security-triage entry" but only a note below the table was added.
  Remediation: Add a row to the roster table with dispatch type pre-pass.

• [incomplete-doc] docs/review.md:12 — The description states the orchestrator "dispatches specialized sub-agents in parallel" but does not mention the security-triage pre-pass introduced by this PR. The two-pass review strategy for large PRs (≥ 50 files) is a significant behavioral change not reflected in user-facing documentation.
  Remediation: Add a sentence noting the security-triage pre-pass for large PRs.

---

Severity change: [logic-error] upgraded from low (prior review) to medium — the interaction between triage results and conditional security sub-agent dispatch is a functional gap, not just a narrow edge case.

Previous run (2)

Review

Findings

Medium

• [edge-case] skills/pr-review/SKILL.md — The validation logic in step 3c-1 (procedure step 5) only catches total triage failures: it fires when security_critical_files is empty but path-pattern matches exist. It does not detect partial failures where the classifier returns a non-empty security_critical_files array but omits some path-pattern-matching files, placing them in standard_files. For example, if 3 of 5 **/auth/** files are classified as security-critical but the other 2 are placed in standard_files, the validation passes because security_critical_files is non-empty, even though the classifier missed obvious signals on the 2 omitted files.
  Remediation: Add a validation step that checks whether every changed file matching a security path pattern appears in security_critical_files. If any path-pattern-matching file is found only in standard_files, either reclassify it as security-critical or treat the triage as failed.

• [protected-path] skills/pr-review/SKILL.md — This PR modifies 6 files under the protected path skills/: skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md, skills/pr-review/sub-agents/cross-repo-contracts.md, skills/pr-review/sub-agents/docs-currency.md, skills/pr-review/sub-agents/intent-coherence.md, and skills/pr-review/sub-agents/style-conventions.md. The PR links to issue Review agent: two-pass strategy for large PRs — triage security-critical files, then deep-review fullsend#2096 and the PR description explains the rationale. Human approval is always required for protected-path changes regardless of context.

Low

• [logic-error] skills/pr-review/SKILL.md — Step 3f assumes the security sub-agent is always dispatched when triage results exist, but step 3c makes security sub-agent dispatch conditional. When the triage classifies files as security-critical on a large PR where step 3c did not select security, step 3f would attempt to modify a context package that was never created. In practice this scenario is narrow — files classified as security-critical typically match the content-based criteria that trigger security sub-agent selection in step 3c — but the specification should be explicit.

• [fail-open] skills/pr-review/SKILL.md — The fallback validation in step 3c-1 (bullet 5) checks only a subset of path patterns using an e.g. qualifier listing 7 of approximately 28 path patterns from the triage sub-agent's classification criteria. While LLM orchestrators may generalize from e.g. examples, a deterministic or exhaustive reference would strengthen this safety-critical fallback.

• [edge-case] skills/pr-review/SKILL.md — Step 3c-1 does not specify what happens when the triage output contains files missing from both security_critical_files and standard_files. The triage sub-agent constraint says "Classify ALL files" but the orchestrator does not validate exhaustiveness of the classification. See also: [fail-open] finding at this location.

• [fail-open] skills/pr-review/SKILL.md — The security-prioritized context mechanism (step 3f) is a soft positional prioritization in the context window. A misclassification by the haiku triage model degrades review quality without producing a detectable failure signal. Partially mitigated by the triage classifier's "err on inclusion" instruction and the fail-closed fallback.

• [incomplete-doc] docs/review.md:12 — The description states the orchestrator "dispatches specialized sub-agents in parallel" but does not mention the security-triage pre-pass introduced by this PR. The two-pass review strategy for large PRs (≥ 50 files) is a significant behavioral change not reflected in user-facing documentation.
  Remediation: Update the description to mention the security-triage pre-pass for large PRs, and consider adding a brief section explaining when the two-pass strategy triggers.

---

Resolved since prior review: [doc-style] #2096 cross-repo reference — now correctly uses fullsend-ai/fullsend#2096.

Previous run (3)

Review

Findings

Medium

• [edge-case] skills/pr-review/SKILL.md — The validation logic in step 3c-1 (procedure step 5) only catches total triage failures: it fires when security_critical_files is empty but path-pattern matches exist. It does not detect partial failures where the classifier returns a non-empty security_critical_files array but omits some path-pattern-matching files, placing them in standard_files. For example, if 3 of 5 **/auth/** files are classified as security-critical but the other 2 are placed in standard_files, the validation passes because security_critical_files is non-empty, even though the classifier missed obvious signals on the 2 omitted files.
  Remediation: Add a validation step that checks whether every changed file matching a security path pattern appears in security_critical_files. If any path-pattern-matching file is found only in standard_files, either reclassify it as security-critical or treat the triage as failed.

• [protected-path] skills/pr-review/SKILL.md — This PR modifies files under the protected path skills/ (skills/pr-review/SKILL.md, skills/pr-review/sub-agents/security-triage.md, and 4 other sub-agent files). The PR links to issue Review agent: two-pass strategy for large PRs — triage security-critical files, then deep-review fullsend#2096 and the PR description explains the rationale. Human approval is always required for protected-path changes regardless of context.

Low

• [logic-error] skills/pr-review/SKILL.md — Step 3f assumes the security sub-agent is always dispatched when triage results exist, but step 3c makes security sub-agent dispatch conditional. When the triage classifies files as security-critical on a large PR where step 3c did not select security, step 3f would attempt to modify a context package that was never created. In practice this scenario is narrow — files classified as security-critical typically match the content-based criteria that trigger security sub-agent selection in step 3c — but the specification should be explicit.

• [fail-open] skills/pr-review/SKILL.md — The fallback validation in step 3c-1 (bullet 5) checks only a subset of path patterns using an e.g. qualifier listing 7 of approximately 28 path patterns from the triage sub-agent's classification criteria. While LLM orchestrators may generalize from e.g. examples, a deterministic or exhaustive reference would strengthen this safety-critical fallback.

• [edge-case] skills/pr-review/SKILL.md — Step 3c-1 does not specify what happens when the triage output contains files missing from both security_critical_files and standard_files. The triage sub-agent constraint says "Classify ALL files" but the orchestrator does not validate exhaustiveness of the classification. See also: [fail-open] finding at this location.

• [fail-open] skills/pr-review/SKILL.md — The security-prioritized context mechanism (step 3f) is a soft positional prioritization in the context window. A misclassification by the haiku triage model degrades review quality without producing a detectable failure signal. Partially mitigated by the triage classifier's "err on inclusion" instruction and the fail-closed fallback.

• [incomplete-doc] docs/review.md:12 — The description states the orchestrator "dispatches specialized sub-agents in parallel" but does not mention the security-triage pre-pass introduced by this PR. The two-pass review strategy for large PRs (≥ 50 files) is a significant behavioral change not reflected in user-facing documentation.
  Remediation: Update the sandbox description to mention the security-triage pre-pass for large PRs, and consider adding a brief section explaining when the two-pass strategy triggers.

• [doc-style] skills/pr-review/SKILL.md — See #2096 references an issue in fullsend-ai/fullsend, not this repo. Should use full cross-repo format fullsend-ai/fullsend#2096 to avoid ambiguity. GitHub renders #2096 as a link to fullsend-ai/agents#2096.

---

Labels: PR modifies review agent skill definitions and adds a new sub-agent

Previous run (4)

Review

Findings

Medium

• [edge-case] skills/pr-review/SKILL.md — The validation logic in step 3c-1 (procedure step 5) only catches total triage failures: it fires when security_critical_files is empty but path-pattern matches exist. It does not detect partial failures where the classifier returns a non-empty security_critical_files array but omits some path-pattern-matching files, placing them in standard_files. For example, if a 60-file PR contains 5 files under **/auth/** and the haiku classifier only flags 2 of them, the remaining 3 receive deprioritized context despite matching a security path pattern. The triage sub-agent is instructed to err on inclusion, but haiku-class models are less reliable at exhaustive enumeration, and the orchestrator has no guard against partial misses.
  Remediation: Add a validation step that checks whether every changed file matching a security path pattern appears in security_critical_files. If any path-pattern-matching file is found in standard_files, either reclassify it as security-critical or treat the triage as failed.

• [fail-open] skills/pr-review/SKILL.md — The fallback validation in step 3c-1 (bullet 5) checks only a subset of path patterns when validating an empty security_critical_files result. The example list uses an e.g. qualifier listing 7 of ~30 path patterns from the triage sub-agent's classification criteria, making the check illustrative rather than normative. For a safety-critical fallback, the check should be deterministic and exhaustive.
  Remediation: Replace the illustrative e.g. list with an exhaustive enumeration of all path patterns from the security-triage sub-agent's classification criteria, or reference the sub-agent file directly.

• [protected-path] skills/pr-review/SKILL.md — This PR modifies files under the protected path skills/ (skills/pr-review/SKILL.md and skills/pr-review/sub-agents/security-triage.md). The PR links to issue Review agent: two-pass strategy for large PRs — triage security-critical files, then deep-review fullsend#2096 and the PR description explains the rationale. Human approval is always required for protected-path changes regardless of context.

Low

• [edge-case] skills/pr-review/SKILL.md — Step 3c-1 does not specify what happens when the triage output contains files missing from both security_critical_files and standard_files. The triage sub-agent constraint says "Classify ALL files" but the orchestrator does not validate exhaustiveness of the classification. See also: [fail-open] finding at this location.

• [fail-open] skills/pr-review/SKILL.md — The security-prioritized context mechanism is a soft prioritization (positional ordering in context window). A subtle misclassification by the haiku triage model degrades review quality without producing a detectable failure. Partially mitigated by the triage classifier's "if in doubt, classify as security-critical" instruction.

• [logic-error] skills/pr-review/sub-agents/security-triage.md — The "Governance and infrastructure paths" section claims correspondence with the orchestrator's protected-path list (step 6e) but is incomplete within the section: CODEOWNERS and policies/ appear only in the "Path patterns" section above. No functional gap since the classifier uses the union of all sections.

• [architectural-conflict] skills/pr-review/SKILL.md — Introduces a third dispatch pattern (pre-pass) alongside existing parallel and sequential patterns. The skill already documents tension with ADR-0018 ("a superseding ADR is needed"), and this change increases that technical debt without resolution.

• [doc-style] skills/pr-review/SKILL.md — See #2096 references an issue in fullsend-ai/fullsend, not this repo. Should use full cross-repo format fullsend-ai/fullsend#2096 to avoid ambiguity.

• [stale-doc] docs/review.md:12 — The description states the orchestrator "dispatches specialized sub-agents in parallel" but the PR introduces security-triage (pre-pass, runs before dimension sub-agents). The description was already incomplete regarding the challenger's sequential dispatch.

• [incomplete-doc] docs/review.md — User-facing documentation does not mention the new two-pass review strategy for large PRs (≥ 50 files).

---

Labels: PR modifies review agent skill definitions and adds a new sub-agent

[rh-hemartin]

LGTM

[ben-alkov]

/fs-fix

• Fix outstanding review findings
• Change the sub-agent's model to sonnet

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 8:10 PM UTC · Completed 8:27 PM UTC
Commit: 12bd957 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 4 (human-triggered)

Fixed three categories of outstanding issues: (1) Normalized model frontmatter in 4 sonnet-tier sub-agents from claude-sonnet-4-6@default to sonnet, aligning with the roster table's claim and the Agent tool's model parameter enum. (2) Updated the diff summary format block in step 3c-1 to reflect the two-tier excerpt system (~20 vs ~50 lines). (3) Added step number 3f to the Security-prioritized context section for consistency with the rest of the step 3 hierarchy.

Fixed (6):

1. Sub-agent model frontmatter values inconsistent with roster table and Agent tool enum (skills/pr-review/sub-agents/intent-coherence.md): Changed model from claude-sonnet-4-6@default to sonnet. The roster table claims 'The Model column reflects each sub-agent's current frontmatter' and shows sonnet, but the actual frontmatter used claude-sonnet-4-6@default which is not a valid Agent tool model parameter. Same fix applied to style-conventions.md, docs-currency.md, and cross-repo-contracts.md.
2. Sub-agent model frontmatter values inconsistent with roster table and Agent tool enum (skills/pr-review/sub-agents/style-conventions.md): Changed model from claude-sonnet-4-6@default to sonnet to match roster table and Agent tool's model parameter enum.
3. Sub-agent model frontmatter values inconsistent with roster table and Agent tool enum (skills/pr-review/sub-agents/docs-currency.md): Changed model from claude-sonnet-4-6@default to sonnet to match roster table and Agent tool's model parameter enum.
4. Sub-agent model frontmatter values inconsistent with roster table and Agent tool enum (skills/pr-review/sub-agents/cross-repo-contracts.md): Changed model from claude-sonnet-4-6@default to sonnet to match roster table and Agent tool's model parameter enum.
5. Diff summary format block hardcodes ~20 lines, ignoring two-tier excerpt system (skills/pr-review/SKILL.md): Updated the diff summary format block placeholder in step 3c-1 from '<first ~20 lines of the file's diff>' to '<diff excerpt: ~20 lines if path matches a classification pattern, ~50 lines otherwise>' to match the two-tier system described in the prose above.
6. Security-prioritized context section lacks step number (skills/pr-review/SKILL.md): Added step number 3f to the Security-prioritized context heading for consistency with the other numbered sub-sections under step 3 (3a through 3e).

Tests: passed

Strategy change: Iteration 4 (past escalation threshold of 3): shifted from addressing specific review-body findings to a thorough cross-file consistency audit, since previous iterations fixed all explicitly stated findings but cross-file inconsistencies (model values, format blocks, step numbering) persisted.

Decision points

• Chose to normalize sub-agent frontmatter model values rather than updating the roster table description (alternatives: Update roster table note to say Model column shows normalized model family, Update roster table cells to show claude-sonnet-4-6@default; rationale: Normalizing frontmatter to sonnet makes the roster table accurate, makes all sub-agents consistent (opus/haiku already use short form), and aligns with the Agent tool's model parameter enum. The dispatch instruction says 'any value accepted by the Agent tool's model parameter' which is sonnet/opus/haiku.)
• Chose step number 3f (not 3d-1) for the Security-prioritized context section (alternatives: Number as 3d-1 (sub-step of context package preparation), Leave unnumbered as an addendum; rationale: The section is positioned after 3e in the document flow and is a distinct step that conditionally modifies context packages after scope constraints are applied. 3f follows the sequential numbering pattern. 3d-1 would imply it is structurally subordinate to 3d, but it runs after 3e.)

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 8:28 PM UTC · Ended 8:47 PM UTC
Commit: 12bd957 · View workflow run →

[ben-alkov]

/fs-fix

• The new triage step is gated solely on FILE_COUNT ≥ 50, but the earlier “large
  PR → per-file mode” criteria is documented as a combined FILE_COUNT+LINE_COUNT
  decision. The triage rationale and prompt composition assume per-file
  diffs/summaries are available, which is not guaranteed by the current gating
• Step 3c-1’s validation logic is underspecified: it doesn’t require that the
  union of security_critical_files and standard_files equals the changed-file
  set, doesn’t reject duplicates/unknown paths, and doesn’t enforce that explicit
  path-pattern/protected-path matches are treated as security-critical when the
  classifier returns a mixed result

[fullsend-ai-coder]

🤖 Finished Fix · ❌ Failure · Started 12:40 PM UTC · Completed 12:48 PM UTC
Commit: cc7a526 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:49 PM UTC · Completed 12:57 PM UTC
Commit: 7224e37 · View workflow run →

[ben-alkov]

/fs-fix - The new triage step is gated solely on FILE_COUNT ≥ 50, but the earlier “large PR → per-file mode” criteria is documented as a combined FILE_COUNT+LINE_COUNT decision. The triage rationale and prompt composition assume per-file diffs/summaries are available, which is not guaranteed by the current gating - Step 3c-1’s validation logic is underspecified: it doesn’t require that the union of security_critical_files and standard_files equals the changed-file set, doesn’t reject duplicates/unknown paths, and doesn’t enforce that explicit path-pattern/protected-path matches are treated as security-critical when the classifier returns a mixed result

[fullsend-ai-coder]

🤖 Finished Fix · ✅ Success · Started 2:26 PM UTC · Completed 2:33 PM UTC
Commit: cc7a526 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:15 PM UTC · Completed 2:31 PM UTC
Commit: f5e9b74 · View workflow run →

[fullsend-ai-coder]

🔧 Fix agent — iteration 5 (human-triggered)

Aligned step 3c-1 triage gating with step 2's per-file mode decision (combined FILE_COUNT+LINE_COUNT) instead of FILE_COUNT alone, and strengthened validation with completeness/dedup checks and deterministic path-pattern override enforcement.

Fixed (2):

1. Triage gating misaligned with step 2 per-file mode criteria (skills/pr-review/SKILL.md): Changed step 3c-1 gating from FILE_COUNT >= 50 to reference step 2's per-file mode decision (combined FILE_COUNT and LINE_COUNT thresholds). Updated rationale to explain the dependency on per-file diffs and why FILE_COUNT alone is insufficient. Updated step 3f and its fallback text to reference per-file mode instead of the 50-file threshold.
2. Step 3c-1 validation logic underspecified (skills/pr-review/SKILL.md): Added structural validation to step 3c-1 point 5: (a) completeness check requiring the union of classified files to equal the changed-file set, rejecting missing files and hallucinated paths; (b) no-duplicates check rejecting paths appearing in both arrays or repeated within either; (c) path-pattern override that moves files matching classification path patterns from standard_files to security_critical_files regardless of classifier output; (d) repositioned existing empty-classification guard to run after path-pattern override.

Tests: passed

_{Updated by fullsend fix agent}

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:35 PM UTC · Completed 2:43 PM UTC
Commit: b849563 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] pattern-inconsistency

The frontmatter name field uses review-security-triage, deviating from the convention where all other sub-agent files use a name matching the filename stem exactly (correctness for correctness.md, challenger for challenger.md, etc.).

Suggested fix: Change name: review-security-triage to name: security-triage.