Skip to content

docs(review-checklist): rules 27-32 — evidence instruments, rendered strings, and merge verification - #1771

Merged
lilyshen0722 merged 2 commits into
mainfrom
kai/checklist-rules-27-32
Sep 19, 2026
Merged

lilyshen0722 merged 2 commits into
mainfrom
kai/checklist-rules-27-32

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Six incident-derived rules appended to the reviewer checklist, per that file's own convention (rules ascend in document order, new rules go at the end). All six come from this session's Sharpen lane work.

27 — an evidence tool that accepts a parameter it does not implement makes its own evidence lie, and it lies in the filename. scripts/ui-evidence-shot.mjs accepted --width and never resized: a capture written as --out …-390.png was a 1440×900 desktop render under a phone's name, and a before/after pair was posted on that naming. Obligations: never accept a parameter you do not consume; echo the parameters that determine the output next to the output; refuse an unknown flag instead of defaulting. Second instance from the same arc: a symlinked frontend/node_modules made vite 403 the @fontsource woff2s, so the before render used fallback fonts — a pair differing by typeface, and a hole no parameter set could have exposed, which is why the run line reports non-2xx asset fetches.

28 — a byte-identical before/after pair is evidence about the surface, not a null result. Three states render the same: nothing changed, the element never mounted, the element is in the DOM but unpainted. Discriminate with an element read (innerText is rendering-aware, textContent is not), not a pixel compare. Corollaries: byte-identity is not a property of two capture sessions; and when a stylesheet hides the target at the width you are shooting, name the surface that does paint it.

29 — one renderer, two meanings. seatLabel renders both the aside's agents allowed line and every trail row. The aside's an agent for a resolvable member was the defect; the trail's was the fallback firing correctly for a callable seat with no installation and no membership. Classify a shared label's call sites by the data they receive — the ledger, not the string, decides which one is wrong.

30 — what a rendered value means is answered from the store, not the renderer. The Unknown User on the #1758 fixture came from models/pg/Message.ts:214,261 (LEFT JOIN users) with no PG row for a hand-inserted author — not from pod membership, which a one-write probe refuted and then reverted. Rider: a hand-seeded row is not a posted one, and a screenshot cannot distinguish a fixture row from live usage, so provenance travels with the artifact.

31 — a flake fix is proven by making the defect reproducible, not by rerunning the flake. Same for an assertion that counts a process-global resource: it must watch the subject's own identity (codex.js:442 creates the dir, :506 removes it; the test now watches that dir, with an in-seam existsSync control). Recorded negative: process.env.TMPDIR does not redirect os.tmpdir() inside jest.

32 — verify a merge at the consumer. For a squash merge, git rev-parse <merge>^{tree} must equal git rev-parse <carried-head>^{tree}; a green tick proves the workflow ran, not that the reviewed artifact landed. Riders: a carry is a per-file claim and a docs-only addition on a cleared head is a re-stamp; know the required-check set, because a required check that has not reported renders as BLOCKED, which is in-flight rather than a verdict.

Scope: docs/development/review-checklist.md only — 16 lines added, no code, no version slot needed (Source changed ⇒ version bumped inspects cli/src and commonly-mcp/src). No pointer edits: rules 27–32 are self-contained and the file's index/pointer lines already landed with #1755.

Request: @vera's review gate, same as the preceding checklist PRs. Anchors in 29–31 are origin/main-verified (V2ConnectorTools.tsx:271–274/727/786, models/pg/Message.ts:214,261, agentIdentityService.ts:618, agentMessageService.ts:1245–1249, codex.js:442/506).

Revision 1ae1c8ec (tip on the cleared 2d1492fc): rule 27 now attributes the harness fix to #1770 instead of implying it is on main, and states what main does today — scripts/ui-evidence-shot.mjs:87 hardcodes a 1440×900 viewport and drops --width without a warning. Rule 28 carried the same latent reading about the pair's widths, so it now names #1770 too. Two lines, one file, docs-only; the diff against the cleared head is 1 file changed, 2 insertions(+), 2 deletions(-).

…strings, and merge verification

Six incident-derived rules, appended per the file's own numbering convention:

- 27: an evidence tool that accepts a parameter it does not implement makes
  its own evidence lie, and it lies in the filename (the harness --width hole,
  plus the woff2 403 that changed the typeface between revisions).
- 28: a byte-identical before/after pair is evidence about the surface, not a
  null result; rendering-aware innerText vs textContent separates hidden from
  absent, and byte-identity is not a property of two sessions.
- 29: one renderer, two meanings — classify a label helper's call sites by the
  data they receive; the ledger settles which string was a defect.
- 30: what a rendered value means is answered from the store, not the renderer;
  and fixture provenance travels with the artifact.
- 31: a flake fix is proven by making the defect reproducible; an assertion over
  a process-global resource must watch the subject's own identity.
- 32: verify a merge at the consumer — squash merge tree == carried head tree.

Docs-only; no version slot.
Rule 27 said the fix "made the sizes real" as though it were on `main`.
It is not: on `main` `scripts/ui-evidence-shot.mjs:87` still hardcodes a
1440x900 viewport and drops `--width` without a warning. Rule 27 (and the
same latent reading in rule 28) now name #1770 the way rule 26 names its PR.

Tip commit on the cleared head; docs-only, one file, no version slot.
@lilyshen0722
lilyshen0722 force-pushed the kai/checklist-rules-27-32 branch from 1ae1c8e to b8091c4 Compare September 19, 2026 16:33
@lilyshen0722
lilyshen0722 merged commit 2b19995 into main Sep 19, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant