feat(#2096): add two-pass review strategy for large PRs#2303
feat(#2096): add two-pass review strategy for large PRs#2303fullsend-ai-coder[bot] wants to merge 4 commits into
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
Site previewPreview: https://d6666021-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🤖 Finished Review · ✅ Success · Started 8:10 PM UTC · Completed 8:24 PM UTC |
ReviewFindingsMedium
Labels: PR modifies review agent skill definitions and adds a new sub-agent under skills/pr-review/. Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsLow
Info
Previous run (4)ReviewFindingsMedium
Low
Info
Labels: PR modifies review agent skill definitions and sub-agent roster, which is harness/skills infrastructure. Previous run (5)ReviewFindingsMedium
Low
Info
|
4c0bf7e to
84a141c
Compare
|
🤖 Review · ❌ Terminated · Started 6:57 PM UTC · Ended 7:10 PM UTC |
|
/fs-fix but ignore
|
|
🤖 Finished Fix · ✅ Success · Started 7:03 PM UTC · Completed 7:11 PM UTC |
|
🤖 Finished Review · ✅ Success · Started 6:57 PM UTC · Completed 7:10 PM UTC |
- 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
🔧 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):
Disagreed (4):
Tests: passed Updated by fullsend fix agent |
|
🤖 Finished Review · ✅ Success · Started 7:15 PM UTC · Completed 7:28 PM UTC |
|
/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). |
|
🤖 Finished Fix · ✅ Success · Started 7:08 PM UTC · Completed 7:16 PM UTC |
🔧 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):
Tests: passed Updated by fullsend fix agent |
… 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
- 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
- [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
… 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
2fa8ae9 to
be2998e
Compare
- 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
- [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
… 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
be2998e to
9bec0e6
Compare
|
🤖 Finished Review · ❌ Failure · Started 5:42 PM UTC · Completed 5:49 PM UTC |
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
- 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
- [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
… 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
9bec0e6 to
0867e8e
Compare
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 7:25 PM UTC · Completed 7:37 PM UTC |
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:
model, read-only classifier)
triage, runs synchronously before context package assembly
assembly for security and correctness sub-agents when triage
results are available
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 #2096
Post-script verification
agent/2096-two-pass-review-strategy)f119a205ffb44da8687d9ff1673b6b2d2de2e927..HEAD)