Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 19 additions & 0 deletions schemas/review-result.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,25 @@
{
"if": { "properties": { "action": { "const": "failure" } }, "required": ["action"] },
"then": { "required": ["reason"] }
},
{
"if": {
"properties": { "action": { "const": "approve" } },
"required": ["action"]
},
"then": {
"not": {
"properties": {
"findings": {
"contains": {
"properties": { "category": { "const": "protected-path" } },
"required": ["category"]
}
}
},
"required": ["findings"]
}
}
}
],
"$defs": {
Expand Down
26 changes: 26 additions & 0 deletions scripts/validate-output-schema-test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,32 @@ run_test_custom_filename "review-approve-valid" \
"${REVIEW_SCHEMA}" \
"true"

# --- review-result.schema.json protected-path constraint ---

run_test_custom_filename "review-approve-with-protected-path-rejected" \
'{"action":"approve","pr_number":1,"repo":"org/repo","head_sha":"abc1234","body":"Approved.","findings":[{"severity":"high","category":"protected-path","file":".github/workflows/ci.yml","description":"PR modifies protected path."}]}' \
"agent-result.json" \
"${REVIEW_SCHEMA}" \
"false"

run_test_custom_filename "review-comment-with-protected-path-valid" \
'{"action":"comment","pr_number":1,"repo":"org/repo","head_sha":"abc1234","body":"Protected paths detected.","findings":[{"severity":"medium","category":"protected-path","file":"CODEOWNERS","description":"PR modifies protected path."}]}' \
"agent-result.json" \
"${REVIEW_SCHEMA}" \
"true"

run_test_custom_filename "review-request-changes-with-protected-path-valid" \
'{"action":"request-changes","pr_number":1,"repo":"org/repo","head_sha":"abc1234","body":"Protected paths.","findings":[{"severity":"high","category":"protected-path","file":"scripts/deploy.sh","description":"PR modifies protected path."}]}' \
"agent-result.json" \
"${REVIEW_SCHEMA}" \
"true"

run_test_custom_filename "review-approve-no-protected-path-valid" \
'{"action":"approve","pr_number":1,"repo":"org/repo","head_sha":"abc1234","body":"LGTM.","findings":[{"severity":"low","category":"style","file":"main.go","description":"Minor style nit."}]}' \
"agent-result.json" \
"${REVIEW_SCHEMA}" \
"true"

# --- Summary ---

echo ""
Expand Down
17 changes: 12 additions & 5 deletions skills/pr-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,12 @@ Protected paths (kept in sync with `post-review.sh`):
For each file in the PR diff, check whether its path starts with (or
exactly matches) any entry in the list above.

If **any** protected files are modified:
If **any** protected files are modified, you MUST emit a structured
finding with `category: "protected-path"`. This is not optional —
the `review-result.schema.json` schema rejects `action: "approve"`
when any finding has `category: "protected-path"`, so omitting the
finding is the only way an approval can slip through. Always emit
the finding.

1. **Insufficient context** — the PR has no linked issue, or the PR
description does not explain why the protected files are being
Expand All @@ -791,11 +796,13 @@ If **any** protected files are modified:
approval is always required for protected-path changes, regardless
of context.

In either case, the presence of a `protected-path` finding at high or
medium severity means the outcome MUST NOT be `approve`.
In either case, the presence of a `protected-path` finding means the
outcome MUST NOT be `approve`. The schema enforces this — validation
will reject the result if `action` is `approve` and any finding has
`category: "protected-path"`.

- For high severity, the finding MUST be `request-changes`
- For medium severity (with sufficient context), the finding MUST be
- For high severity, the outcome MUST be `request-changes`
- For medium severity (with sufficient context), the outcome MUST be
`comment-only`

The `post-review.sh` script independently downgrades approvals on
Expand Down
Loading