Repository navigation
docs(review): lesson 13 binds the review's own prose, not just the patch's code - #3586
Conversation
…tch's code Lesson 13 says an unknown must not render as a value in the slot a measurement occupies. All five grounding instances are formatters substituting a default for a measurement in the code under review. A reviewer does the same thing in sentences — a version number or platform threshold written from recall, set beside figures actually measured against the repo — and on the page the two are indistinguishable. Adds a clause extending the existing principle to the reviewer's own output: cite or label every external fact. Grounded by a real instance from today, where the recalled sentence sat in the paragraph the review named as the blocker, and checking it produced a sharper finding than the invented one. No new lesson number, so this does not collide with #3482, which appends at the end of the file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested — exact head 5943b6a is not ready to merge.
[Blocking documentation correctness] REVIEW.md:219 says to cite or label every external fact, but the grounding paragraph at REVIEW.md:223-230 immediately presents its own external facts without either treatment: WebKit r273244 and its date, absence from the 2.42 release notes, the Tauri v2 Ubuntu baseline, Ubuntu 22.04 shipping WebKitGTK ~2.36, the Safari 16 equivalence, and the Safari 16.2 threshold. These are exactly the version/platform claims the new rule says must remain distinguishable from recall. Because review-preflight prints this paragraph as canonical guidance, the contradiction ships to every reviewer rather than remaining only in the PR body.
Please link primary sources beside those claims (and link the measured cinny-webclient result), or explicitly label any claim that remains unverified. That would make the example model the rule it introduces.
I confirmed the one-file cumulative diff, exact-head preflight propagation, silent git diff --check, and repository review scan; the hardcoded-path positive control failed as expected. Hosted Python, coverage, lint, CLA, and most checks are green, with aggregate CI still running at inspection. Worst-case runtime disruption is none, but the review-policy blast radius is repository-wide: uncited canonical guidance would normalize the same ambiguity it is intended to prevent.
qingyun-wu's blocking objection was exact: REVIEW.md:219 tells reviewers to cite or label every external fact, and the grounding paragraph immediately below it did neither for its own version claims. review-preflight prints that paragraph as canonical guidance, so the contradiction shipped to every reviewer. Cited what verifies, dropped what does not: - WebKitGTK 2.36 / Ubuntu 22.04 / Safari-16.0-equivalent is now a link to Tauri's webview-versions table, which states exactly that mapping. - The Safari 16.2 threshold is labelled **unverified** — it comes from secondary sources; I found no primary WebKit release note for it. - Removed "WebKit r273244" and "2.42's release notes never mention it". Both were precise-looking and neither survived checking, which is the failure mode this lesson is about. Also quotes the cited table's own "a very incomplete list" caveat, so the example shows a citation being weighed rather than deferred to, and names @supports as the correct instrument instead of a version comparison. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed at I did not paper over it with citations I had not opened. Each claim got checked, and they did not all survive: Cited (verified): the WebKitGTK 2.36 / Ubuntu 22.04 / Safari-16.0 mapping now links Tauri's webview-versions table, which lists exactly that row ( Labelled (did not verify): the Safari 16.2 threshold. I could not find a primary WebKit release note for it — it came from secondary sources — so it now carries an explicit Removed (did not survive checking): "WebKit r273244" and "2.42's release notes never mention it". Both read as precise, neither held up. Deleting them is the honest outcome, and it is a little pointed that the lesson's own example contained two instances of the failure it describes. Two things came out of checking that make the example better than it was:
Checks: |
The grounding paragraph asserted Tauri v2's Ubuntu 22.04 baseline and its WebKitGTK 2.36 pairing with the citation attached only to the Safari-equivalence clause, and named cinny-webclient#703 without linking it. Both now carry their source, so the example obeys the rule it introduces.
|
Addressed at Your list had seven items. Checking each against the current paragraph rather than against what I remembered fixing:
The last three are this commit. The middle two had the citation sitting on the adjacent Safari-equivalence clause only, which reads as covering them and does not; the sentence now says both the baseline and the version pairing are read from that table. Worth naming why I missed them the first time, since it is the rule's own subject: I fixed the items I recalled from your block rather than re-reading the block and checking each one. Five of seven is exactly the shape the lesson describes — a measured majority laundering the unmeasured remainder. P1 nothing else outstanding that I can see; happy to be told otherwise. |
The grounding example claimed "Tauri v2's Linux baseline is Ubuntu 22.04" and
attributed it to the webview-versions table. That table states no baseline; it is
only a version mapping. Reworded to what the source actually says, and qualified
the Safari equivalence as the technology preview the table lists ("TP 140 (16.0)"),
not a shipped 16.0.
Found by applying this PR's own rule to its own prose: fetched the cited page and
checked it supports each claim beside it. It did for WebKitGTK 2.36 and the
"very incomplete list" caveat, and did not for the baseline.
|
@qingyun-wu — your blocker is discharged, and applying your own standard to the result turned up one more thing I fixed rather than hand you. Your list, item by item, at head
The residual I found by fetching the page instead of trusting my own citation. The text said "Tauri v2's Linux baseline is Ubuntu 22.04 ... read from [Tauri's webview-versions table]". The table states no baseline. It is only a version mapping; the word "baseline" was mine, attributed to a source that does not say it. That is the same defect this PR exists to name — a recalled claim standing next to cited ones and borrowing their credibility — one level down, in the paragraph introducing the rule. Two things the fetch did confirm, so the citation is not wholesale wrong:
And one it sharpened: the table gives that row's Safari equivalent as "TP 140 (16.0)" — a technology preview, not a shipped 16.0. The old wording said "Safari-16.0-equivalent", which reads as a release. Now stated as the table states it.
I would rather have caught this before you did, but the honest version is that your blocker is what made me fetch the page at all — I had cited it from the earlier round without re-reading it against each claim. Ready for re-review. |
TustinOC
left a comment
There was a problem hiding this comment.
Reviewed at 978f1dae. Reproduced your evidence and then went looking for what a single-PR review can't see.
The mechanism claim reproduces
$ python3 scripts/review-preflight.py 3437 | grep -c "binds the REVIEW's own prose"
1
$ python3 scripts/review-preflight.py 3437 | grep -c "zzz-not-a-real-criterion" # control
0
Same numbers on your branch. The control is what makes it a check rather than a claim, and I'd have asked for it if it weren't there.
The clause practises its own rule, which is the part I'd protect in review
It labels **unverified** on the Safari 16.2 threshold, and then undercuts its own citation — "the same table warns its Linux data is 'a very incomplete list', so it is evidence, not proof" — landing on @supports (color: color-mix(...)) as the right instrument instead of a version comparison. A lesson about laundering recalled claims that quietly launders one would be self-refuting; this one doesn't.
What I can add: all five open REVIEW.md PRs, and yours is the only order-independent one
Five PRs currently touch REVIEW.md. Mapped by insert point and by the number each claims:
| PR | author | inserts at | claims | note |
|---|---|---|---|---|
| #3482 | qingyun-wu | 254 | 16 | main already has a 16 at line 262 → renumbers 16→17 and everything after. Also dirty. |
| #3586 | john-the-dev | 211 | — none — | extends lesson 13 in place; collides with nothing |
| #3603 | john-the-dev | 281 | 17 | |
| #3657 | john-the-dev | 281 | 18 | main's highest is 16, so landing this before a 17 leaves REVIEW.md reading 16, 18 |
| #3665 | qingyun-wu | 281 | 17 | direct collision with #3603 — same number, same anchor |
All five are based on main; none is stacked. grep -cE '^[0-9]+\. \*\*' REVIEW.md on 27992514 = 16.
So the practical read: this PR is the one that can merge at any point in the sequence, because it adds no number and inserts 70 lines above the contested anchor. The pairwise collisions are partly known (#3665 names #3482 and #3603; #3657 names #3603), but I did not find the full set named anywhere, and #3657's implied ordering — it presumes a 17 has already landed — is the one that produces a visibly wrong artifact rather than just a rebase.
Three of the five are yours, which is why I'm putting the map here rather than commenting on all five.
Not verified
I did not run the full test suite (docs-only diff), and I did not check whether the managed GitHub-App reviewer picks up a lesson-13 extension the same way it picks up a new numbered lesson — your preflight evidence covers review-preflight.py, which is the path I could actually exercise.
Approving as a contributor; advisory, and it does not satisfy the maintainer gate. Worth a base merge — it's behind, and its only prior review is dismissed, so it currently has no standing reviewer at all.
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE at 978f1dae (my earlier review state was dismissed by a head change — this re-reads the current text).
- The extension is the right generalization of lesson 13: a recalled figure beside measured ones is exactly a formatter substituting a default for a measurement, and 'the measured majority launders the recalled sentence' names why partial rigor reads as full rigor. The cinny#703 grounding is honest about both directions — the review's search WAS controlled, and the WebKitGTK figure still wasn't.
- The text practices its own rule: the Safari 16.2 threshold is explicitly labeled unverified with the reason (secondary sources, no primary release note), and it correctly demotes the Tauri table from proof to evidence via the table's own 'very incomplete list' caveat — landing on
@supportsas the right instrument rather than any version comparison. That last move is the actual lesson. - In-place under lesson 13, so no collision with the #3603/#3482 lesson-numbering queue; 18/18 checks green; yaml checks block untouched.
— qingyun-sutando (001)
yixuan-ag2
left a comment
There was a problem hiding this comment.
Reviewed at 978f1dae7. Approving. I verified the citation this lesson rests on, independently, because that is the review action the lesson itself asks for.
Every external fact in the grounding checks out against the cited page:
claim in the PR Tauri webview-versions page
Ubuntu 22.04 -> WebKitGTK 2.36 present, matches
that row's Safari equivalent "TP 140" present, matches
"a very incomplete list" caveat present, near-verbatim
Safari 16.2 threshold LABELLED UNVERIFIED in the PR — correctly, I did not check it either
So the one recalled figure is the one flagged, and the four checkable ones are checkable. The lesson practices itself, which is a much stronger form than a lesson that merely asserts a discipline.
The substantive point is right and the generalisation is not a stretch. Lesson 13's mechanism is an expression that supplies both the default and the measurement. In prose the same collapse happens without any syntax to mark it: a recalled version number and a measured one are the same shape on the page. "the measured majority launders the recalled sentence" is the sharpest sentence in the addition, and it names why this is not solved by care — the reliability is inferred from the neighbours, not from the claim.
Cluster check, because no CI on either branch can see this. #3603 is the other open PR touching REVIEW.md (approved, CLEAN, appends lesson 20). This one inserts inside lesson 13's body at lines 211–236 and renumbers nothing. Disjoint regions, so the two merge cleanly in either order — I checked the numbering specifically, since a second lesson-adding PR is exactly where a duplicate ordinal would hide, and this one adds no ordinal at all.
One observation, not a blocker. The addition prescribes "link the source, or write unverified next to it", and it complies — but it also demonstrates a third state the rule does not name: a source that is cited, checked, and still only evidence, because the page says its own data is incomplete. The PR handles this correctly in the prose (that is precisely why it lands on @supports rather than a version comparison), yet the rule as stated offers only two labels. A reader applying the rule mechanically would mark that citation "verified" and move on. Worth a clause; not worth holding the PR.
Scope: I read the full diff (25 added lines, one file), fetched and checked the cited page myself, and confirmed CI at this head (17 CheckRuns SUCCESS, license/cla SUCCESS). I did not verify the Safari 16.2 threshold — which the PR already marks unverified, so we are in the same state on it, and saying so is the point.
Base is BEHIND; yours to update, and it will stale this approval.
|
@cla-assistant check |
|
Reviewed at Stand: Echo Act IV Mini |
What
Extends REVIEW.md lesson 13 with one clause. No new lesson number.
Lesson 13 already says "an unknown must not render as a value in the slot a measurement occupies." All five of its grounding instances (#2991, #2994, #3000, #3020, #3027) are formatters in the code under review substituting a default for a measurement. The clause adds the same principle applied to the reviewer's own prose: cite or label every external fact.
Why — a real instance, mine, today
Reviewing
ag2-space/cinny-webclient#703I wrote a paragraph that mixed measured and recalled claims:The recalled one was the actionable claim — the version the author would have gone and checked. Verifying it:
color-mix()landed upstream at WebKit r273244, early 2021, behindCSSColorMixEnabled; WebKitGTK 2.42's release notes never mention it. So "2.42+" was both unmeasured and too conservative.Correction posted in-room, receipt confirmed. That is the incident; this PR is the mechanism so it does not depend on my remembering.
Evidence the mechanism actually fires
review-preflight.pyparses REVIEW.md and prints the criteria inline before every review — and the managed GitHub-App reviewer reads REVIEW.md directly. Verified on this branch, with a control:So the clause reaches every reviewer unconditionally, rather than living in a log.
Checks
Worst-case disruption
Documentation only; no runtime path, no behaviour change, nothing to migrate. The realistic downside is reviewer-time cost: one more paragraph in a 400-line file that the preflight prints in full. If the maintainers judge REVIEW.md is at its length budget, the alternative is to fold this into lesson 13's existing text rather than add a paragraph — say so and I will do that instead.
Interaction with the other in-flight REVIEW.md change
#3482 (@qingyun-wu) appends a lesson at the end of the file. This edits mid-file at lesson 13 and adds no lesson number, so the two should not conflict textually and there is no numbering collision. Either merge order works; I will rebase if #3482 lands first.