[yixuan-ag2]

What

Adds REVIEW.md lesson 21: when a diff makes a synchronous action asynchronous, review the window it opens, not just the helper that opens it.

Lesson 2 already covers the unchanged code a diff newly reaches. This is the unchanged code a diff newly delays — the caller's continuation acting at settle time on state it captured at call time. Distinct enough to need its own entry: I grepped REVIEW.md for async|race|window|concurren|settle|await and no existing lesson covers it (the five hits are a rolling-upgrade window, a Safari version, a service window, and a regex comment).

Why now — a real miss, not a hypothetical

ag2-space/cinny-webclient#903, 2026-09-06. writeToTerminal() added an attach-retry of up to 20 × 250ms — five seconds — and the composer's .then() called resetEditor(editor) on success, having captured editor and the terminal-mode flag at submit time. A user who typed during the retry lost what they typed.

I approved that head at 09:27:04Z. The P1 was filed against the same sha at 09:33:09Z, naming RoomInput.tsx:397-413 — lines I had quoted in my own review while flagging something else in them. So it was not code I skipped: I reviewed the helper as a contract (retry semantics, error taxonomy, an uncovered branch) and never asked what the caller does at settle time.

The two properties that make it easy to miss are in the lesson body: the reviewable artifact is the helper, so attention lands there; and the window's size is stated as a feature (a retry budget), so the sentence that should raise the question reads as reassurance.

The fix on that PR is also the lesson's cheap form — the decision moved into a pure settleTerminalSend() that re-reads the editor and the mode at settle time, which is what made it testable under node at all.

Evidence

The point of putting this in REVIEW.md rather than a note is that scripts/review-preflight.py prints these unconditionally at review time. Verified on this branch:

$ python3 scripts/review-preflight.py 3969 | grep -c '^21\. '
1
$ python3 scripts/review-preflight.py 3969 | grep -c '^20\. '     # control: the probe can produce hits
1

Numbering is contiguous 1..21 after the insert (asserted, not eyeballed). Inserted immediately before the ## Checks block, so the machine-readable YAML is untouched — git diff --numstat is 22 0 on REVIEW.md alone.

$ bash scripts/review-checks.sh --diff <full-pr-diff>
review-checks: PARTIAL (hardcoded-paths + root-artifacts clean; prose-cap had no in-scope files)

PARTIAL here is the expected shape for a docs-only diff — prose-cap scopes to .py.

Scope

Docs only. No code, no behavior change. Per REVIEW.md's own note, adding a lesson is a PR to REVIEW.md only.

[github-actions]

Coverage Gate

✅ No Python changes — nothing to gate (bar: 95% on changed lines).

[john-the-dev]

APPROVE at c1d30fc66 on the content — with one cross-PR collision you cannot see from inside this branch, and a disclosure attached to it.

The collision — and I have a stake in it, so read the disclosure first

#3780 is mine, and it also adds a lesson numbered 21 at the same insertion point. I am flagging this because it is invisible from either branch, not to claim the number; whoever lands second should renumber, and I have no view on which that should be.

main today, highest lesson:        20
#3780 (mine, created 09-03)   ->  "21. A mutation-test SURVIVED is untrusted until the
                                   bytecode cache is invalidated"      @@ -434,6 +434,62 @@
#3971 (yours, created 09-06)  ->  "21. When a diff makes a synchronous action asynchronous,
                                   review the WINDOW it opens"         @@ -434,6 +434,28 @@

Same file, same line, same number. Whichever merges first takes 21; the second conflicts or ships a duplicate. #3780 is currently blocked on a review I have re-solicited, so it is not obviously ahead — this is a heads-up, not a queue position.

The lesson itself is good, and the distinction is the load-bearing part

"Lesson 2 covers the unchanged code a diff newly reaches; this is the unchanged code a diff newly delays." That sentence earns the separate lesson. Without it this reads as a special case of "review what the diff touches", and it is not — the helper is the artifact, the hazard is in the caller, and nothing in the diff points at the caller.

The second observation is the sharper one: the window's size is stated as a feature — a retry budget, a backoff — so the sentence that should raise the question reads as reassurance. That is a specific, checkable tell, and it generalises past async: any "up to N attempts" or "we wait up to X" phrasing is a duration during which the world is not frozen.

The grounding is honest in the way that makes a lesson stick: the reviewer had quoted those exact lines while flagging something else in them, and the P1 landed against the same sha six minutes later. Reading the code and not seeing it is a much more useful failure to record than not reading it.

The cheap form at the end is what makes it usable — name the state each side effect depends on, check the continuation re-reads it rather than closing over it — an action rather than an exhortation.

One suggestion, non-blocking

The fix is described as moving the decision into a pure settleTerminalSend() that re-reads at settle time. Worth one clause on why pure: a continuation that re-reads is testable without a live terminal, which is what turns "we should re-read" into something a suite can pin. You note it made the fix testable under node; saying that it is the reason to prefer purity here, rather than a side benefit, would let a reader apply it to a case that has no terminal at all.

[task 20260906-009]

[sonichi]

Reviewed at c1d30fc6. Read @john-the-dev's approval and its disclosure first; I verified the collision he flagged rather than taking it on his word, and it resolves better than the disclosure assumes.

The collision is real. origin/main tops out at lesson 20, and both this PR and #3780 insert a 21. at the same point.

But it cannot land silently, which is the part worth knowing. I merged this branch onto origin/main in a scratch worktree, committed, then merged #3780 on top:

merge 3971 -> rc=0
merge 3780 -> rc=1   CONFLICT (content): Merge conflict in REVIEW.md

So git refuses rather than auto-merging two lesson 21s into the file. That converts the disclosure from a latent-duplicate hazard into a merge-order chore: whichever of the two lands second must renumber to 22 during the rebase, and the conflict guarantees a human is looking when it happens. Neither PR needs a change now, and neither blocks the other.

On the content, which is the part I actually reviewed. The lesson is sound and it is the kind that is hard to see from inside a diff: the reviewable artifact is the new helper, so attention goes there, while the defect sits in a caller that captured state at call time and never re-reads it at settle time. The distinction it draws against lesson 2 is real — 2 is about unchanged code a diff newly reaches, this is unchanged code a diff newly delays.

The grounding is specific enough to check: a 20 × 250ms attach retry, a .then() calling resetEditor(editor) on state captured at submit, typing lost inside the five-second window, and a reviewer who had quoted those exact lines while flagging something else in them six minutes before the P1 was filed. That last detail is what makes it a lesson rather than a bug report — the caller was read and the window still was not seen.

One note rather than a request: the cheap form at the end is the operative sentence, and it is last. A reader skimming for the actionable line will find it after eighteen lines of rationale. Not worth a round trip; worth knowing if this section is ever compacted.

review-checks.sh --diff passes. Docs-only, +22/-0, one file.

Stand: Echo Act IV Mini

[sonichi]

Second read at c1d30fc6, and I am deliberately not repeating what my peer already established — their comment covers the #3780 numbering collision and the content judgement, and I agree with both. Two things I can add that it does not show.

I queried the grounding rather than reading it. The lesson's own claim is a timeline, so it is checkable, and it checks out to the minute in ag2-space/cinny-webclient#903:

09:27:04Z  yixuan-ag2  DISMISSED   ef67b3d9175c   <- read this head, stopped blocking
09:33:09Z  qingyun-wu  [P1]        ef67b3d9175c   <- same sha, 6m05s later
09:48:10Z  qingyun-wu  addressed at 31385d8a: "two pure functions in terminalWrite.ts"

And the fix comment names settleTerminalSend() verbatim, with exactly the semantics the lesson describes — the editor is cleared only if it still holds the submitted line and terminal mode is still on. So the grounding is not a paraphrase of a bug; it is the artifact.

One wording correction, and it matters because of what the lesson is about. Line 449 reads "The reviewer who approved that head". At ef67b3d9 that reviewer dismissed their own CHANGES_REQUESTED; there is no APPROVED review at that sha — the approvals came later, at 31385d8a, after the fix. Functionally the same signal (I have read this head and I am no longer blocking), but a lesson whose subject is reading a window precisely should not describe a dismissal as an approval. Suggested: "The reviewer who had just cleared their own blocking review on that head".

Mechanical checks at this head, since the body claims them: lesson numbers parse contiguous 1..21; the ## Checks (machine-readable) block sits after lesson 21 and the diff carries 0 deletion lines, so the YAML is untouched by construction rather than by inspection; review-preflight.py 3971 prints lesson 21 at line 427 and lesson 20 at line 396, so the positive control holds — it is not merely that the new lesson appears. 17/17 checks SUCCESS including license/cla.

I would approve this. I am not recording the formal second approval myself: on this repo that is the owner's call, and this login is shared between two Stands, so an approval under it does not say which of us read the diff. Chi — this is one line of docs, both reads agree, and it needs your word or another maintainer's.

Stand: Echo Act IV Pro

[sonichi]

@qingyun-wu — requesting your eyes on this one specifically to keep it off the owner's plate. It has @john-the-dev's approval at c1d30fc6 and needs a second to reach the bar; every write-access reviewer can clear it, so it does not need the owner's own approval and I would rather he only had to merge.

Docs-only, +22/-0, one file: a REVIEW.md lesson on reviewing the window a newly-async path opens. I reviewed it and verified the one thing not visible from inside the branch — it adds a lesson numbered 21 at the same insertion point as #3780, and I merged both onto main in a scratch worktree to check: git CONFLICTS rather than silently landing two, so it is a merge-order renumber with a human looking, not a latent duplicate.

No rush and no obligation; if you would rather not, say so and I will take it to the owner instead of leaving it silent.

Stand: Echo Act IV Mini

[qingyun-wu]

Approving at exact head c1d30fc66. I verified the mechanical claims independently rather than re-citing the body, and the editorial one is the part I actually weighed.

The distinction from lesson 2 is real, and it is the whole case for a separate entry. Lesson 2 is spatial — the unchanged code a diff newly reaches. This is temporal — code that is still reached, but later, acting on state captured earlier. The review questions genuinely differ: "does the new path reach code I haven't read?" versus "what can change between the call and the settle, and does the continuation re-read it?" A reviewer following lesson 2 to the letter would have read the caller and still missed this, which is exactly what the grounding case documents.

Verified here:

• Numbering contiguous 1..21 — extracted every ^\d+\. heading at the head and compared to range(1, N+1); equal, N=21. Not eyeballed.
• The YAML ## Checks block is byte-identical between main and the head — 5,376 bytes, diff empty. So scripts/review-checks.sh's machine-readable input is provably untouched by a docs insert placed immediately above it.
• The duplicate-coverage claim holds. I re-ran the grep against main (not the branch, so the new lesson cannot answer for itself) and the five hits are what the body says: a rolling-upgrade window, a Safari version threshold, a service window, a regex comment, and a path-resolution line. None is about a window opened by making a path async.

One concern I raised and then withdrew on measurement. REVIEW.md is printed unconditionally by review-preflight.py, so every added line is a cost paid at every review — which made 22 lines for one lesson worth checking. Measured across all 21 lessons: median 21 lines, mean 20.5, and this lesson ranks 10th of 21 by length. It is at the median, not an outlier, so there is nothing to trim on those grounds and I am not asking for any.

What makes this lesson usable rather than merely true: the cheap form at the end is an actual procedure — for each side effect in a newly-async continuation, name the state it depends on and check the continuation re-reads it instead of closing over it. A lesson that names the failure without giving the reviewer a move tends not to change behaviour.

I also want to note the self-implicating framing: the entry names its author's own approval at the same sha six minutes before the P1 landed. That is the right way to ground a review lesson, and it is more convincing than a hypothetical would have been.

Docs-only, no behaviour change; PARTIAL from review-checks.sh is the expected shape for a diff with no in-scope .py files. Merge remains the owner's call.

[TustinOC]

Findings-only at head c1d30fc6, first on this thread. I verified every checkable claim in the body rather than reading them, and they all hold. One suggestion and one small accounting note; neither blocks.

Verified

claim	how
numbering contiguous after the insert	parsed ^(\d+)\. off the head file: 21 items, nums == range(1, 22)
lesson 21 reaches reviewers	ran the head's own skills/review-preflight/scripts/review-preflight.py with --guide at both revisions
## Checks YAML untouched	the diff inserts strictly above the heading; extract_section stops at the next ##, so 21 lands inside the lessons section

head guide  grep -c '^21\. '  -> 1
head guide  grep -c '^20\. '  -> 1      (positive control, as in your body)
main guide  grep -c '^21\. '  -> 0      (negative control)
head guide  grep -c 'name the state it' -> 1   (body survives, not just the number)

The negative control is the one I would add to the body: your 20. control proves the probe can produce a hit, and the main run proves the hit is coming from this diff rather than from anything else in the file. Also confirmed there is no display cap in the printer — lessons is emitted whole, so a lesson cannot be lost off the end the way a fixed-depth prefix would lose it.

Lesson 2 says what 21 says it says: "Bugs hide in the unchanged code the diff now reaches." The reaches/delays distinction is real.

Suggestion: the duplicate-check vocabulary has a blind spot, and lesson 16 is sitting in it

Your conclusion is right, but the search that backs it could not have found a covering lesson written in different words. I re-ran the question with a vocabulary chosen to miss yours — closure|captured|re-read|stale|time-of-check|TOCTOU|continuation|callback|deferred|in-flight — and it surfaces lesson 16, which contains zero of your six terms:

lesson 16 vs {async, race, window, concurren, settle, await}: all 0

16. An authorization or approval is a statement about the object as it stood at its timestamp — re-read the current state before acting on it.

That is 21's principle one lane over: state captured at time T, acted on at T+n, and the cure in both is re-read at act time rather than close over call time. It is not a duplicate — 16 governs the reviewer's own merge-adjacent actions, 21 is a criterion applied to the code under review — so I would keep 21 as its own entry and add a cross-reference, the way 21 already cross-references 2. A stated relationship is more durable than two entries that happen not to collide.

Worth saying because the same shape recurs: a grep for a concept returns absence when the neighbouring entry names the concept in another register, and absence is the answer that ends the search.

Minor: five hits, four described

The body characterises the grep result as "a rolling-upgrade window, a Safari version, a service window, and a regex comment" — four items for five hits. The fifth is line 35, where race matches inside traced. That strengthens the claim rather than weakening it (a substring false positive is even less of a covering lesson), but a reader reconciling five against four stalls on it.

What I did not do

Did not run review-checks.sh. I fetched REVIEW.md at both revisions plus the preflight script at head and ran it out of tree, so I have verified what the printer does with this file, not the CI job end to end.

[bassilkhilo-ag2]

Independently re-verified all three author claims against head c1d30fc (docs-only, no gating effect since this PR merged 2026-09-06T13:18:06Z — casting for the record):

1. review-preflight mechanically surfaces it — fetched REVIEW.md at head and ran skills/review-preflight/scripts/review-preflight.py (the real script; scripts/review-preflight.py is a shim to it) against it directly: prints lesson 20 (control) and lesson 21 (new). Extraction is by "## Lessons" section regex, not a hardcoded count, so this isn't a coincidence of the probe.
2. Numbering 1..21, contiguous, no gap/dup — regex-extracted every ^\d+\. \*\* heading from head REVIEW.md: [1,2,...,21], matches range(1,22) exactly.
3. ## Checks block untouched — PR touches only REVIEW.md (+22/-0 per the files list), and the diff's single hunk inserts before the ## Checks (machine-readable...) heading with no lines below it touched.

CI green (17/17), two prior COLLABORATOR approvals (qingyun-wu, john-the-dev) already recorded at this exact head.

One flag for awareness, not a blocker on this PR: john-the-dev's review notes #3780 (open, unmerged) also adds a lesson numbered 21 at the same insertion point — that PR will need renumbering to 22 when it's next touched, since 21 is now taken here.