Skip to content

fix(#410): guard step 3f context assembly against unselected security sub-agent #413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fullsend-ai-coder wants to merge 1 commit into from
Closed

fix(#410): guard step 3f context assembly against unselected security sub-agent #413

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 30 additions & 11 deletions skills/pr-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,14 @@ complex PR that triggers all conditions legitimately needs all 6.
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

@fullsend-ai-review fullsend-ai-review Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

non-empty `security_critical_files` list, auto-include the `security`
sub-agent even if the domain classification in step 3b did not
independently select it. If triage identifies security-critical files,
they must receive dedicated security review regardless of the domain
signals. This auto-inclusion takes precedence over re-review dispatch
rules that would otherwise skip the `security` sub-agent.

**Re-review dispatch (prior-finding-aware):** When
`PRIOR_REVIEW_PROVENANCE` is `app-verified` and prior findings exist
(step 3a), narrow dispatch based on which dimensions had findings:
Expand Down Expand Up @@ -370,11 +378,16 @@ complex PR that triggers all conditions legitimately needs all 6.
- `docs-currency` — re-qualifies only if `changed_since_prior`
includes documentation files (not merely because the repository
contains docs).
- `security` / `cross-repo-contracts` — re-qualify only if
`changed_since_prior` includes files matching their step 3b path
criteria (auth/permissions/secrets/config/data-handling for
`security`; public APIs, exported interfaces, schemas, or CLI
surface for `cross-repo-contracts`).
- `security` — re-qualifies if `changed_since_prior` includes
files matching step 3b path criteria
(auth/permissions/secrets/config/data-handling), OR if step 3c-1
produced a non-empty `security_critical_files` list (the security
triage auto-inclusion rule above takes precedence over the
re-review skip rule).
- `cross-repo-contracts` — re-qualifies only if
`changed_since_prior` includes files matching its step 3b path
criteria (public APIs, exported interfaces, schemas, or CLI
surface).

If the incremental delta cannot be enumerated — `changed_since_prior`
is `"all"` (the step 2a fallback for a failed compare, >250 commits,
Expand Down Expand Up @@ -605,10 +618,16 @@ the constraint first.

When step 3c-1 produced a security triage classification (i.e., step 2
selected per-file mode 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
context packages for the `correctness` sub-agent and, if selected in
step 3c, the `security` sub-agent as follows. If the `security`
sub-agent was not selected in step 3c (which should not occur when
`security_critical_files` is non-empty, due to the auto-inclusion
rule), skip the security-specific context modifications (items 1 and
the security portion of item 4) and apply only the correctness
modifications.

1. **Security sub-agent (skip if not selected in step 3c):** Provide
the full per-file diffs for all
`security_critical_files` first, clearly marked with a
`### Security-critical file: <path>` header and the triage reason.
Include standard files' diffs after, under a
Expand All @@ -628,8 +647,8 @@ follows:
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:
4. **Include the triage summary** in the context package for the
`correctness` sub-agent and, if selected, the `security` sub-agent:

```markdown
### Security triage classification
Expand Down
Loading