docs(pr-review): tighten weak language and add trigger phrase - #753
docs(pr-review): tighten weak language and add trigger phrase#753ralphbean wants to merge 4 commits into
Conversation
PR Summary by QodoPR review skill docs: add trigger phrase and tighten verification/footers language
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
|
🤖 Finished Review · ✅ Success · Started 5:01 PM UTC · Completed 5:14 PM UTC Commit: |
Code Review by Qodo
1. Protected skills/ file modified
|
ReviewFindingsMedium
Previous runReviewFindingsMedium
Previous run (2)ReviewFindingsMedium
Previous run (3)ReviewFindingsMedium
Previous run (4)ReviewFindingsMedium
Previous run (5)ReviewFindingsMedium
Previous run (6)ReviewFindingsMedium
Low
Previous run (7)ReviewFindingsMedium
Low
Previous run (8)ReviewFindingsMedium
Low
Labels: PR modifies the pr-review skill definition (skills/pr-review/SKILL.md), a documentation-only change to the review agent orchestrator skill |
waynesun09
left a comment
There was a problem hiding this comment.
MEDIUM: Parallel 'Insufficient context' bullet still hedges with "note that" instead of "state that"
File: skills/pr-review/SKILL.md (line ~1039, outside this PR's diff hunks, so not addressable as an inline comment)
The PR converts exactly three "note that" -> "state that" instances (the omitted-files line, the PR-head-verification line, and the case-2 "Sufficient context" protected-path bullet), but leaves the structurally identical case-1 "Insufficient context" bullet two lines above untouched. It still reads:
The description MUST list the affected protected files and note that the PR lacks justification for modifying governance or infrastructure files.
while the case-2 sibling now reads "...and state that human approval is always required...". Both bullets share the same "MUST list ... and {note|state} that ..." template for the two branches of the protected-path check, so leaving one on the old hedge word reintroduces the exact weak-language inconsistency this PR is meant to eliminate.
Suggestion: Change the case-1 bullet's "...and note that the PR lacks justification..." to "...and state that the PR lacks justification..." to match the sibling bullet.
|
/fs-fix address @waynesun09's findings. |
|
🤖 Finished Fix · ✅ Success · Started 8:44 PM UTC · Completed 8:49 PM UTC Commit: |
🔧 Fix agent — iteration 1 (human-triggered)Addressed waynesun09's finding: replaced the remaining 'note that' hedge in the protected-path Insufficient context bullet with 'state that' to match its sibling bullet. One pre-existing test failure (prerequisites-creates-allowed-issue) is unrelated to this change. Fixed (1):
Tests: passedNext steps:
|
Change the 'Insufficient context' bullet's 'and note that' to 'and state that' to match the sibling 'Sufficient context' bullet, closing the weak-language inconsistency flagged by @waynesun09. Addresses review feedback on #753
|
🤖 Finished Review · ✅ Success · Started 8:50 PM UTC · Completed 9:02 PM UTC Commit: |
bc717f2 to
2d76cd1
Compare
|
🤖 Finished Review · ✅ Success · Started 9:39 PM UTC · Completed 9:53 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Reviewed via 4-agent squad (claude-coder, claude-researcher, grok-review-agent, cursor-code-review) + verification pass at head 2d76cd1 — no CRITICAL/HIGH/MEDIUM findings survived verification.
|
/fs-fix fix the "New trigger phrase appended" problem. |
|
🤖 Finished Fix · ✅ Success · Started 10:40 PM UTC · Completed 10:49 PM UTC Commit: |
🔧 Fix agent — iteration 2 (human-triggered)Addressed 2 findings: (1) rewrote the redundant trigger phrase that repeated 'structured review result' from the preceding description sentence, and (2) fixed the docs-review link formatting to match the security-triage link pattern. Pre-existing test failures in post-retro-test.sh (10 tests) are unrelated to this change. Fixed (2):
Tests: passedNext steps:
|
Rewrites the "Use when" trigger phrase to avoid repeating "structured review result" from the preceding sentence. Also removes double quotes around the docs-review markdown link to match the security-triage link pattern at line 438. Addresses review feedback on #753
|
🤖 Finished Review · ✅ Success · Started 10:50 PM UTC · Completed 11:01 PM UTC Commit: |
|
/fs-fix move the "use when" trigger language to the beginning of the description |
|
🤖 Finished Fix · ✅ Success · Started 11:25 PM UTC · Completed 11:29 PM UTC Commit: |
Moves the "Use when" trigger sentence from the end of the YAML frontmatter description to the beginning, per human instruction. Addresses review feedback on #753
🔧 Fix agent — iteration 3 (human-triggered)Moved the trigger phrase to the beginning of the description as instructed. The 'Use when...' sentence was at the end of the YAML frontmatter description and is now the first sentence. Fixed (1):
Tests: passedNext steps:
|
|
🤖 Finished Review · ✅ Success · Started 11:31 PM UTC · Completed 11:45 PM UTC Commit: |
40664c3 to
43bc250
Compare
|
🤖 Finished Review · ✅ Success · Started 3:38 PM UTC · Completed 3:52 PM UTC Commit: |
43bc250 to
bb51782
Compare
|
🤖 Finished Review · ✅ Success · Started 4:11 PM UTC · Completed 4:23 PM UTC Commit: |
Change the 'Insufficient context' bullet's 'and note that' to 'and state that' to match the sibling 'Sufficient context' bullet, closing the weak-language inconsistency flagged by @waynesun09. Addresses review feedback on #753
Rewrites the "Use when" trigger phrase to avoid repeating "structured review result" from the preceding sentence. Also removes double quotes around the docs-review markdown link to match the security-triage link pattern at line 438. Addresses review feedback on #753
Moves the "Use when" trigger sentence from the end of the YAML frontmatter description to the beginning, per human instruction. Addresses review feedback on #753
bb51782 to
8263546
Compare
|
🤖 Finished Review · ✅ Success · Started 6:42 PM UTC · Completed 6:52 PM UTC Commit: |
Ran skillsaw (evaluating it per #188) against the repo's skills. Adds a "Use when..." trigger phrase to the description, replaces three "note that" hedges with direct "state"/"must state" instructions, converts two bare file paths into markdown links, and makes the action-hints footer condition explicit (the post-review pipeline only appends it when the outcome is `request-changes`). Related to #188 Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
Change the 'Insufficient context' bullet's 'and note that' to 'and state that' to match the sibling 'Sufficient context' bullet, closing the weak-language inconsistency flagged by @waynesun09. Addresses review feedback on #753
Rewrites the "Use when" trigger phrase to avoid repeating "structured review result" from the preceding sentence. Also removes double quotes around the docs-review markdown link to match the security-triage link pattern at line 438. Addresses review feedback on #753
Moves the "Use when" trigger sentence from the end of the YAML frontmatter description to the beginning, per human instruction. Addresses review feedback on #753
8263546 to
a994da0
Compare
|
🤖 Finished Review · ✅ Success · Started 6:56 PM UTC · Completed 7:06 PM UTC Commit: |
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep: 2 findings below.
| - **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 when the outcome is `request-changes`. |
There was a problem hiding this comment.
MEDIUM: Action-hints footer condition is precise for request-changes but silently omits reject, which also becomes a GitHub REQUEST_CHANGES review
The new wording states: "The post-review pipeline appends action hints deterministically when the outcome is request-changes." Verified against scripts/post-review.sh at PR head (a994da05): the ACTION_HINTS_FOOTER block is gated by the literal bash check if [ "${ACTION}" = "request-changes" ] (line 384), so the statement is technically accurate for that one internal action value. However, the schema documented a few dozen lines below in this same file (the outcome/action table) lists reject as a distinct, valid action with its own required fields, and fullsend/internal/cli/postreview.go maps both request-changes and reject to the same GitHub review event REQUEST_CHANGES (lines 183-188). So a PR reviewed with action=reject is posted to GitHub identically to a request-changes review, but per post-review.sh it will NOT receive the "Next steps" action-hints footer, since the script only checks for the literal string request-changes. The new, more "precise" sentence therefore creates a documentation gap: a reader could reasonably assume any REQUEST_CHANGES-style outcome (including reject) gets the footer, when only the literal request-changes action does. This doesn't currently cause an agent to violate the (unconditional) "No footer" rule, since that rule already applies regardless of outcome, but it is a real, verifiable inaccuracy/incompleteness introduced by this PR's tightening pass.
Suggestion: Either broaden the condition to reflect both cases if that's the intent (and file a companion fix in post-review.sh to also cover reject), or make the sentence non-exclusive/accurate as-is, e.g.: "the post-review pipeline appends action hints deterministically for the request-changes action (not for reject, approve, or comment)."
| 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 |
There was a problem hiding this comment.
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).
Ran skillsaw (evaluating it per #188) against the repo's skills.
Adds a "Use when..." trigger phrase to the description, replaces
three "note that" hedges with direct "state"/"must state" instructions,
converts two bare file paths into markdown links, and makes the
action-hints footer condition explicit (the post-review pipeline only
appends it when the outcome is
request-changes).Related to #188
Assisted-by: Claude Opus 4.6 noreply@anthropic.com
Signed-off-by: Ralph Bean rbean@redhat.com
Stack created with GitHub Stacks CLI • Give Feedback 💬