docs(review): decide on a suggestion instead of executing it - #126
Conversation
The reference already says to verify an AI reviewer's factual claim before applying or declining. Two cases sit next to that one, where the claim is true and applying it is still wrong. A suggestion against verbatim material is a suggestion to falsify it. Reviewers read a diff, not its provenance. A review asked to correct "occured" to "occurred" in a captured before/after transcript - but the misspelling belongs to the server under discussion, which has thrown that exact string for years. Improving the line would have turned evidence into an approximation and broken anyone grepping their logs for the string as emitted. The section asks for the source citation in the reply, since a bare refusal reads as resistance while a quoted line is checkable in seconds. Check whether the literal fix is complete. A comment correctly objected to time() in a test-setup example and asked for a fixed instant. Applying that to the commented line alone would have frozen the clock the code reads while the fixtures around it still came from time(), putting the two years apart and failing every assertion as expired - a rare flake traded for a certain failure. The working fix pins one constant and drives both from it, and the reply has to say so, because that part is invisible to the reviewer. Adds two evals covering both cases. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
github-actions
Bot
added
documentation
There was a problem hiding this comment.
🟢 Ready to approve
Documentation-only update with minor wording/example-shape nits that don’t affect correctness of the overall guidance.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR extends the Git workflow “pull request workflow” reference guidance with two additional cases where an AI/code review suggestion can be factually correct yet still wrong to apply verbatim, and adds corresponding eval scenarios to codify the behavior.
Changes:
- Documented how to handle suggestions that target verbatim artifacts (transcripts/log lines/recorded outputs) where “fixing” them would falsify evidence.
- Documented how to evaluate whether a literal suggestion is complete/sufficient before applying it (avoiding half-fixes).
- Added two new eval prompts/expected outputs (ids 23–24) covering these scenarios.
File summaries
| File | Description |
|---|---|
| skills/git-workflow/references/pull-request-workflow.md | Adds two new review-decision sections: verbatim-material handling and “literal fix completeness” guidance. |
| evals/evals.json | Adds eval entries 23–24 to enforce the new documented behaviors. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Review feedback, and the second point lands squarely on the section itself: the citation offered as verbatim carried an ellipsis for the class name, so it was not literally checkable - the softened version of exactly what the text warns against. It now quotes the line as it stands, with its file and line number, verified byte-identical against the source. Also "copied in verbatim" -> "copied verbatim". Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
|
There was a problem hiding this comment.
🟡 Not ready to approve
The new “verbatim” example currently presents a not-quite-literal quoted source line (leading : ), which undermines the section’s core point about byte-identical evidence.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
skills/git-workflow/references/pull-request-workflow.md:283
- This snippet is introduced as “quoted as it stands” (verbatim), but the line shown starts with
:, which is unlikely to be part of the actual PHP source and makes the quote not byte-identical/copy-pastable. If you’re aiming for an exact source line, drop the leading colon (or alternatively show the fullpath:line:prefix on the same line if the intent is to quote grep output).
: $this->responseFactory->createErrorResponse($request, 1603956982, 'An error occured on handling the request.');
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
pull-request-workflow.mdalready covers verifying an AI reviewer's factual claim before applying or declining it. This adds the two cases next to that one, where the claim is true and applying it is still wrong — both from a single review in one session.A suggestion against verbatim material is a suggestion to falsify it
Reviewers read a diff, not its provenance. A comment asked to correct "occured" to "occurred" in a captured before/after transcript. The misspelling belongs to the server under discussion:
Improving that line would have turned evidence into an approximation — in a section that argues transcripts must be captured rather than illustrated — and broken anyone grepping their own logs for the string as it is actually emitted.
The section also asks for the source citation in the reply. A bare "no" reads as resistance; a quoted line makes the reason checkable in seconds. The general form: before editing any line, know whether you are its author or its witness.
Check whether the literal fix is complete before applying it
A correct diagnosis does not guarantee a sufficient remedy. A comment correctly objected to
time()in a test-setup example as non-deterministic and asked for a fixed instant. Applying that to the commented line alone would have frozen the clock the code reads while the fixtures around it still came fromtime()— putting the two years apart and failing every assertion as expired. A rare flake would have become a certain failure.The fix that works pins one constant and drives both from it. The reply has to say that, because the incompleteness is exactly what the reviewer could not see.
Notes
Two evals added (ids 23–24).
Build/Scripts/validate-skill.shpasses with 0 errors / 0 warnings. Documentation only.Came from /retro: yes