Skip to content

docs(pr-review): tighten weak language and add trigger phrase #753

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

Open
ralphbean wants to merge 5 commits into from
+11 −8
Open

docs(pr-review): tighten weak language and add trigger phrase #753

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d5c7e34
docs(pr-review): tighten weak language and add trigger phrase
ralphbean Aug 11, 2026
cef4e68
fix: replace remaining 'note that' hedge in protected-path check
fullsend-ai-coder[bot] Aug 12, 2026
838d8c2
fix(pr-review): rewrite redundant trigger phrase and fix link quotes
fullsend-ai-coder[bot] Aug 13, 2026
a994da0
fix(pr-review): move trigger phrase to beginning of description
fullsend-ai-coder[bot] Aug 13, 2026
8418a95
docs(pr-review): align spawn-prompt strength and clarify footer condi…
fullsend-ai-coder[bot] Aug 17, 2026
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: 11 additions & 8 deletions skills/pr-review/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
---
name: pr-review
description: >-
PR review orchestrator. Triages the change, dispatches specialized
Use when a pull request needs end-to-end review encompassing
triage, code quality, security, and documentation. PR review
orchestrator. Triages the change, dispatches specialized
sub-agents in parallel across review dimensions, synthesizes their
findings, runs PR-specific checks, and produces a structured review
result. Sub-agent definitions live in sub-agents/ relative to this
Expand Down Expand Up @@ -198,7 +200,7 @@ orchestrator selects dimension-relevant files for each sub-agent:

For omitted changed files in large PRs, sub-agents should treat those
files as unavailable for PR-head verification. Any findings about
omitted files should note that the file contents could not be verified
omitted files must state that the file contents could not be verified

@waynesun09 waynesun09 Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

MEDIUM: Omitted-file instruction strength diverges between orchestrator body ("must state") and spawn-prompt template ("state") sent verbatim to sub-agents

Verified at PR head (a994da05): line 203, in the orchestrator-facing guidance (step 3), now reads "...omitted files must state that the file contents could not be verified against the PR head." Line 727, inside the literal "Context package" spawn-prompt template that is assembled and sent verbatim to each dispatched sub-agent (Part 4, under "### 4. Dispatch sub-agents"), reads "...state that the file contents could not be verified against the PR head" — without the "must". Since sub-agents only ever receive the copy at line 727 (they never see the orchestrator-body text at line 203), the RFC-2119-style MUST strength this PR intentionally added to the orchestrator's own description of the requirement is not carried through to the actual instruction text the sub-agents act on.

Suggestion: Change line 727's spawn-prompt template text to "must state that the file contents could not be verified against the PR head" to match line 203's strength, or document why the two are deliberately different (e.g. because the orchestrator body is a summary and 203's MUST is meant only as a paraphrase, not a literal instruction).

@ralphbean ralphbean Aug 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sure, ok.

against the PR head. Sub-agents must not read omitted changed files
from disk, since disk contains base-branch code, not the PR head.

Expand Down Expand Up @@ -434,7 +436,7 @@ incident.

**Procedure:**

1. Read `sub-agents/security-triage.md` for the sub-agent definition.
1. Read [`sub-agents/security-triage.md`](sub-agents/security-triage.md) for the sub-agent definition.
2. Resolve the active governance paths list, matching
`post-review.sh`'s resolution: if `REVIEW_PROTECTED_PATHS` is
non-empty, split on commas and trim whitespace; if it's explicitly
Expand Down Expand Up @@ -690,7 +692,7 @@ runs in step 6d):
reviewing PR" template, and include everything else verbatim

**Part 3 — Doc review skill:** *If and only if* the roster key is
Comment thread
ralphbean marked this conversation as resolved.
"docs-currency", read "../docs-review/SKILL.md" and include its
"docs-currency", read [`../docs-review/SKILL.md`](../docs-review/SKILL.md) and include its
contents verbatim

**Part 4 — Context package:** the assembled context from step 3d,
Expand Down Expand Up @@ -722,7 +724,7 @@ runs in step 6d):
**Note:** Not all changed files are included above due to PR size.
Changed files not listed here should be treated as unavailable for
PR-head verification. If you produce findings about files not included
above, note that the file contents could not be verified against the
above, you must state that the file contents could not be verified against the
PR head. Do not read changed files from disk — disk contains
base-branch code, not the PR head.

Expand Down Expand Up @@ -1035,14 +1037,14 @@ the finding.
1. **Insufficient context** — the PR has no linked issue, or the PR
description does not explain why the protected files are being
changed: raise a **high** finding with category `protected-path`.
The description MUST list the affected protected files and note
The description MUST list the affected protected files and state
that the PR lacks justification for modifying governance or
infrastructure files.

2. **Sufficient context** — the PR links to an issue and the
description explains the rationale for the change: raise a
**medium** finding with category `protected-path`. The description
MUST list the affected protected files and note that human
MUST list the affected protected files and state that human
approval is always required for protected-path changes, regardless
of context.

Expand Down Expand Up @@ -1206,7 +1208,8 @@ where `[open]` = `<` + `!--` and `[close]` = `--` + `>`.
— omit the `## Review` header and `### Findings` section entirely.
- **No footer.** Do not append any footer, action-hints block, or
boilerplate after findings. The post-review pipeline appends
action hints deterministically when appropriate.
action hints deterministically for the `request-changes` action

@waynesun09 waynesun09 Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

MEDIUM: Action-hints footer exception list omits the failure action

The sentence at lines 1210-1212 reads: "The post-review pipeline appends action hints deterministically for the request-changes action (not for reject, approve, or comment)." This was added to close a prior gap where reject was missing from the exception list. However the enumerated list still omits failure, which is a fifth valid action documented two lines below in this file's outcome-to-action table (line 1230: | failure | failure | reason (body optional) |) and is actually emitted by scripts/post-review.sh (lines 86 and 118: echo '{"action":"failure",...}'). The script's footer gate (line 384: if [ "${ACTION}" = "request-changes" ]) also does not append the footer for failure, so the doc's claim to be an exhaustive list of non-footer actions is still incomplete — the same class of gap this sentence was written to fix.

Suggestion: Either add failure to the exception list ("not for approve, comment, reject, or failure") or reframe positively as "appends action hints only when the action is request-changes" so the statement can't go stale if new action values are added later.

(not for `reject`, `approve`, or `comment`).

If `PRIOR_REVIEW_PROVENANCE` starts with `unverifiable-`, include an
info-level finding in the review output:
Expand Down
Loading