Skip to content

Commit e0eabae

Browse files
authored
fix(gator): surface PR-owned security regressions in reviews (#4338)
Signed-off-by: John Myers <johntmyers@users.noreply.github.com> Co-authored-by: John Myers <johntmyers@users.noreply.github.com>
1 parent e585b49 commit e0eabae

6 files changed

Lines changed: 42 additions & 3 deletions

File tree

‎.agents/skills/launch-openshell-gator/SKILL.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,11 @@ The launcher streams image-build and provisioning output to the terminal. Import
286286
size-limited full-diff endpoint. An unavailable patch ID alone does not block
287287
the ledger; it disables rebase-equivalence shortcuts. Reviewers should inspect
288288
oversized changes file-by-file locally.
289+
- Security regressions introduced or newly exposed by an unmerged PR belong
290+
in its normal review findings. Private follow-up under `SECURITY.md` is for
291+
pre-existing vulnerabilities independent of the PR; security classification
292+
alone is not a private-triage or test-dispatch gate. Gator must not initiate
293+
external security reporting without explicit operator authorization.
289294

290295
### Inspect Active Sandboxes
291296

‎.claude/agents/principal-engineer-reviewer.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,11 @@ When reviewing code or diffs:
9999
- Treat pre-existing security issues as private security follow-up, not public
100100
blockers on the current pull request. Treat other pre-existing defects as
101101
non-blocking follow-up work.
102+
- Surface security regressions introduced or newly exposed by an unmerged PR
103+
in the normal PR review evidence contract. A new reachable path, trusted sink,
104+
or security contract is PR-owned even when an underlying unsafe primitive
105+
already existed. Security classification alone is not a private-triage gate.
106+
Do not contact external reporting channels without operator authorization.
102107
- Keep docs, skill drift, diagnostic wording, and test-strength feedback
103108
advisory unless the published contract is materially false, the diagnostic
104109
creates an operational or safety failure, or missing coverage leaves a

‎scripts/agents/gator/agent.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
# SPDX-License-Identifier: Apache-2.0
33

44
id: gator
5-
payload_version: 11
5+
payload_version: 12
66
display_name: Gator Gate Agent
77
description: Validate and monitor OpenShell GitHub issues and pull requests through the gator state machine.
88

‎scripts/agents/gator/bin/review_feedback_ledger_test.sh‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -406,7 +406,7 @@ rg -q 'COPY bin/validate-review-findings /usr/local/bin/validate-review-findings
406406
"$GATOR_DIR/Dockerfile"
407407
ruby -ryaml -e '
408408
manifest = YAML.load_file(ARGV.fetch(0))
409-
abort unless manifest.fetch("payload_version") == 11
409+
abort unless manifest.fetch("payload_version") == 12
410410
resource = manifest.fetch("resources").find {
411411
|entry| entry.fetch("id") == "gator-review-findings-schema"
412412
}
@@ -421,6 +421,14 @@ rg -q 'review-feedback-ledger NVIDIA OpenShell <pr-number>' \
421421
rg -q 'Every prior Gator finding is a durable review disposition' \
422422
"$GATOR_DIR/skills/gator-gate/SKILL.md"
423423
rg -q 'review feedback ledger' "$GATOR_DIR/prompts/gator.md"
424+
# Keep security routing consistent across the gate, top-level prompt, and
425+
# independent reviewer; PR-owned regressions must remain actionable reviews.
426+
rg -Fq 'PR in that PR' "$GATOR_DIR/skills/gator-gate/SKILL.md"
427+
rg -Fq 'private_security_review_required' "$GATOR_DIR/skills/gator-gate/SKILL.md"
428+
rg -Fq 'Do not initiate external disclosure' "$GATOR_DIR/skills/gator-gate/SKILL.md"
429+
rg -Fq 'not a generic private-security blocker' "$GATOR_DIR/prompts/gator.md"
430+
rg -Fq 'Security classification alone is not a private-triage gate' \
431+
"$GATOR_DIR/../../../.claude/agents/principal-engineer-reviewer.md"
424432
rg -q '### Pragmatic review calibration' \
425433
"$GATOR_DIR/skills/gator-gate/SKILL.md"
426434
rg -q 'A new commit permits a delta review' \

‎scripts/agents/gator/prompts/gator.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@ Important sandbox constraints:
3434
- Use `gator:approval-needed` only when gator is complete but maintainer approval is still missing. Once maintainer approval is present and required checks remain green with no unresolved feedback, move to `gator:merge-ready` for the final merge or close decision.
3535
- Before running the `principal-engineer-reviewer` sub-agent or posting a review disposition, check existing gator comments and PR reviews for the current `headRefOid`. Do not run a reviewer or post another marked review/status disposition for a head SHA that already has one unless a maintainer explicitly requests a same-SHA public response, the PR is merged/closed and needs terminal cleanup, or the earlier attempt failed before posting. A prior marked comment that only says the reviewer sub-agent failed before producing output is a legacy infrastructure-failure report, not a valid review disposition; ignore it and retry the reviewer. A prior marked `## Blocked` comment whose only blocker was that the PR was draft is also not a valid code-review disposition after the PR becomes ready for review; ignore it for review suppression and run the reviewer once. Same-SHA CI changes, human replies, label changes, and reviewer comments must not create public status comments; record them only in the supervised result sentinel. A state-specific TTL nudge is the exception: after 48 business hours and no more often than once per 48 business hours for the same state and responsible actor, post the matching `## Author Follow-Up Nudge`, `## Maintainer Review Nudge`, `## Merge Decision Nudge`, or `## Blocker Follow-Up Nudge` template even when the head SHA is unchanged. A nudge must name the pending action, does not authorize a re-review, and does not consume or replace the one review disposition for that SHA.
3636
- When the gator skill requires the `principal-engineer-reviewer` sub-agent and the current effective patch has not already been reviewed by gator, first build the required review feedback ledger with `review-feedback-ledger`, then run a bounded independent review with `{{REVIEWER_COMMAND}}`. Treat the ledger's review mode, tree identity, patch identity, previous reviewed SHA, review budget, and telemetry as authoritative. Use the full PR diff for an initial review; for a follow-up, inspect unresolved feedback plus the author-only delta and do not mine unchanged or upstream-only code for new findings. Carry open findings without duplicating them, and preserve resolved or waived dispositions unless the new diff materially invalidates them.
37+
- Surface security regressions introduced or newly exposed by an unmerged PR
38+
as normal PR review findings, not a generic private-security blocker. Reserve
39+
private follow-up under `SECURITY.md` for pre-existing vulnerabilities
40+
independent of the PR. Do not initiate external reporting without explicit
41+
operator authorization; security classification alone does not suspend
42+
operator-authorized test dispatch or waive normal review blockers.
3743
- Require reviewer output to follow the JSON evidence contract in
3844
`/etc/openshell/agent-payload/skills/gator-gate/references/review-findings-schema.md`.
3945
Normalize it with `validate-review-findings`; only entries with

‎scripts/agents/gator/skills/gator-gate/SKILL.md‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,14 @@ If the `principal-engineer-reviewer` sub-agent fails before producing usable rev
4040
- You may push changes only when explicitly instructed by a GitHub comment from a maintainer or by a direct operator prompt.
4141
- Do not post `/ok to test <full-head-sha>` unless the current GitHub user has maintainer authority.
4242
- Code review is code-only. Do not run pre-commit, unit tests, or E2E locally as part of the initial PR review unless explicitly instructed.
43-
- Security vulnerabilities must not be triaged through public GitHub issues. Follow `SECURITY.md`.
43+
- Surface security-related findings introduced or newly exposed by an unmerged
44+
PR in that PR's normal review, using the same evidence, severity, and blocker
45+
rules as other findings. A security classification alone does not require
46+
private triage or suspend operator-authorized test dispatch.
47+
- For pre-existing vulnerabilities independent of the PR, follow `SECURITY.md`:
48+
retain detailed evidence privately and notify the operator without publishing
49+
exploit details. Do not initiate external disclosure, contact PSIRT, or submit
50+
a security report without explicit operator authorization.
4451

4552
Maintainer authority means one of:
4653

@@ -651,6 +658,14 @@ Keep reviews proportional, scope-bound, and convergent:
651658
- Route pre-existing security defects through the private security process.
652659
Do not publish exploit details or make them blockers on the current PR.
653660
Route other pre-existing defects to a non-blocking follow-up.
661+
- A pre-existing unsafe primitive does not make a new PR-owned exposure
662+
pre-existing. Compare base and head: if the PR creates a new reachable path,
663+
trusted sink, or security contract that makes the defect exploitable, surface
664+
the regression in the PR review with its invariant, impact, fix, and verification.
665+
Do not substitute a generic private-security notice for actionable feedback
666+
or invent a `private_security_review_required` gate solely because the finding
667+
is security-related. Existing review blockers still follow normal state rules;
668+
this distinction does not waive findings or authorize tests otherwise forbidden.
654669
- Treat docs, skill drift, diagnostic wording, and test-strength feedback as
655670
non-blocking unless the published contract is materially false, the
656671
diagnostic causes an operational or safety failure, or missing coverage

0 commit comments

Comments
 (0)