fix(#410): guard step 3f context assembly against unselected security sub-agent#413
fix(#410): guard step 3f context assembly against unselected security sub-agent#413fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
… sub-agent Step 3f unconditionally modified context packages for the security sub-agent when triage results existed, but step 3c conditionally dispatches security based on domain classification. A large PR could trigger triage (step 3c-1) classifying files as security-critical without step 3c selecting the security sub-agent, causing step 3f to reference a context package for an unselected sub-agent. Fix: auto-include the security sub-agent in step 3c when step 3c-1 produces a non-empty security_critical_files list, ensuring triage- flagged files always receive dedicated security review. Add an explicit guard in step 3f so context modifications for the security sub-agent are skipped if it was not selected. Split the re-review dispatch rule for security from cross-repo-contracts so security can reference triage auto-inclusion as an independent qualification path during re-review. Note: pre-commit could not run (sandbox network restrictions). The post-script runs pre-commit authoritatively on the runner. Closes #410
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 6:43 AM UTC · Completed 6:56 AM UTC |
ReviewFindingsMedium
Low
Labels: PR modifies the review agent's orchestration skill definition (skills/pr-review/SKILL.md) and touches a protected path requiring manual review |
| schemas, or CLI args are modified. Skip entirely for PRs that don't | ||
| touch public API surface. | ||
|
|
||
| **Security triage auto-inclusion:** When step 3c-1 produces a |
There was a problem hiding this comment.
[low] design-direction
Step 3c's "Security triage auto-inclusion" paragraph references step 3c-1's output (security_critical_files list), but the step numbering could be read as implying 3c runs before 3c-1. The sub-step naming convention (paralleling "3a-1" at line 278) makes the ordering reasonably clear when reading top-to-bottom, but a brief clarifying note that the auto-inclusion rule is evaluated after step 3c-1 completes could improve readability.
Suggested fix: Consider adding a brief note to step 3c clarifying that the auto-inclusion rule is evaluated after step 3c-1 completes, if the sub-step naming convention is not considered self-evident.
Summary
securitysub-agent in step 3c when step 3c-1 produces a non-emptysecurity_critical_fileslist, ensuring triage-flagged security-critical files always receive dedicated security review even when domain classification (step 3b) did not independently trigger the security dimension.securitysub-agent are only applied when it was selected in step 3c, making behavior deterministic rather than relying on implicit orchestrator reasoning.securityfromcross-repo-contractsso the triage auto-inclusion is recognized as an independent qualification path during re-review (previously both were in a combined bullet that only referenced step 3b path criteria).Context
The review agent identified a logic gap in the two-pass review strategy (PR #190) across 4 review cycles: step 3f unconditionally assembled context for the
securitysub-agent when triage results existed, but step 3c conditionally dispatched it based on domain classification. A large PR could trigger triage classifying files as security-critical without step 3c selecting the security sub-agent, causing step 3f to reference a context package for an unselected sub-agent.Testing
prerequisites-creates-allowed-issuetest failure inpost-triage-test.shis pre-existing on main (unrelated to this change).Closes #410
Post-script verification
agent/410-guard-step3f-security-dispatch)e7a3660d8d2dffb418d747883540f78588c8c60f..HEAD)