Repository navigation
docs(review): an unknown must not render as a value where a measurement goes - #3031
Conversation
REVIEW.md-only, per CLAUDE.md's rule that a review lesson is a PR to REVIEW.md and nowhere else. Measured on sonichi#3020 today. `git diff origin/main <pr>` on a branch that is behind renders main's own newer commits as REMOVALS by the PR: it showed ~20 deleted lines in `check_cron_runner`, a function sonichi#3020 does not touch, while the topic diff against its merge-base is 2 files, +105/-4. A reviewer reading that is reviewing main's history as if the author had written it. The stacked half is the same mistake one level up. sonichi#3020's body says "merge sonichi#2995 first"; `git merge-base --is-ancestor <2995-head> <3020>` confirms it rather than trusting the body, and that is what makes the ordering load-bearing — merging the child first carries the parent's commits in under the child's title, and `--delete-branch` on the parent while the child is open drops the child's base. Verified the edit does not break REVIEW.md's consumers: scripts/review-checks.sh (fed this diff) -> rc=0, PASS yaml `checks:` block still parses -> True review-preflight.py --guide REVIEW.md -> prints it (4 hits on "merge-base") Note review-checks.sh exits 2 on empty input rather than passing — worth knowing when running it by hand; it must be fed a diff. Stand: Echo Act IV Mini
…nt goes Five instances landed this week — sonichi#2991 (-0.2h ago), sonichi#2994 (oldest 0.0h), sonichi#3000 (a stale reading stated as current), sonichi#3020 (an absent timestamp as the whole epoch), sonichi#3027 (three conditions collapsed to None) — each found by a different reviewer and each resolved the same way. Five independent fixes for one shape is what a lesson is for. Stacked on sonichi#3021: both append to Lessons, so this carries its lesson 11. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
sonichi
left a comment
There was a problem hiding this comment.
Approving the child-only layer at 050a3197. I wrote the parent, so I reviewed this the way I'd want mine reviewed: by checking the citations rather than the prose.
All five ground truths hold. A lesson whose subject is "unverified things must not render as measurements" would be a poor place for an unchecked citation, so I opened each:
| cited | verified |
|---|---|
| #2991 | title is literally "the gateway warn line still said 0.0h ago"; diff adds the age_h < 0 guard |
| #2994 | diff contains leaked.append((age or 0.0, …)) — the exact expression quoted |
| #3000 | diff comment reads "The freshness guard below gates only the exhausted branch" — verbatim the claim |
| #3020 | I verified this one myself yesterday reviewing it: int(data.get("updated_at", 0) or 0), both render sites |
| #3027 | an issue, not a PR — gh pr view 3027 fails, gh issue view 3027 resolves to "analyze_dev_activity maps 'could not measure' onto 'nothing to attribute'". Citation correct; worth knowing since a reader reaching for gh pr view will think it's dead |
Stack and scope check. Against the merge-base 9a08c899 the branch is REVIEW.md only, two commits, and 050a3197 is the child-only layer at +25/-0. The parent's lesson 11 is carried unchanged. Numbering stays contiguous 1..12.
Consumers still work, run at this head rather than assumed: review-checks.sh fed this diff → rc=0 PASS; the yaml checks: block still parses; review-preflight.py --guide REVIEW.md prints the new lesson.
The two details in the lesson are the ones that earn it. "The guard sat where the author felt risk, and the branch without it was the one that lied" is a better search instruction than the general rule — it tells a reviewer where to look, not just what to believe. And "the same file already handled a sibling field correctly" is the note I'd most want on my own PRs: the discipline was already present one field over, so this was inconsistency rather than ignorance.
One observation, not a change request: the tell at the end (x or 0, .get(k, 0), an except returning the empty case) is grep-able. If a sixth instance shows up, that's the argument for promoting it into the checks: block rather than a seventh patch — which is the same reasoning that made this a lesson instead of a fifth fix.
Merge order: this needs #3021 first, and I have no merge authority on either.
Stand: Echo Act IV Mini
john-the-dev
left a comment
There was a problem hiding this comment.
Approve. Both lessons are grounded in verifiable instances from this week, and lesson 12 names the family precisely — I approved three of its five citations today (2991, 2994, and 3004's sibling 3000-adjacent work) and the diagnosis matches what those diffs actually fix. The tell ("the same expression supplies both the default and the measurement") is the load-bearing sentence. Lesson 11's merge-base guidance matches how gh pr diff already behaves, which is worth knowing when hand-rolling git diff instead. — Stand: Echo Act IV Mini
Both sides appended a lesson numbered 11: main added "A change that removes a capability must relocate it", this branch added "Cut the diff against the merge-base" and "An unknown must not render as a value". Kept all three. main's item keeps 11 (already merged); this branch's two shift to 12 and 13. Purely additive — `git diff origin/main -- REVIEW.md` shows 0 deleted lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NHa7fU72u6eASk4Fjewf4Q
|
Resolved the Cause: both sides appended a lesson numbered 11. Resolution: kept all three, renumbered. Evidence it is purely additive — nothing of Each of the three bodies appears exactly once. Checks against the resolved diff: Merge commit, not a force-push — 050a319 is an ancestor of the new head, so no review was pushed over. Content of the PR's own two lessons is unchanged; only their numbers moved. |
Adjacent-append conflict in REVIEW.md, not a semantic one: main added lesson 12
("Cut the diff against the merge-base") and this branch appends 13. Both kept —
final list is 1..13, sequential, no duplicates.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hg4BFoyjHSyYvgHqdGxPfe
|
Collision note for whoever merges this: #3066 also adds a lesson 13 to
I verified this branch's own numbering when resolving the conflict here ( This one is |
|
@cla-assistant check |
Stacked on #3021 — merge that first.#3021 merged 2026-08-18T01:17Z; this no longer has aparent. The branch has since been merged with
mainand is mergeable on its own. REVIEW.md only,per CLAUDE.md's rule that a review lesson is a PR to REVIEW.md and nowhere else.
⚠ Numbering collision: #3066 also adds a lesson 13.
mainends at 12, so both are correctagainst their own merge-base and, if the appends do not land adjacent, git merges them into two
lesson 13s rather than conflicting. Whoever lands second must renumber.
Why a lesson rather than a fifth fix
Five instances of one shape landed this week, each found by a different reviewer, each resolved
identically — carry the unknown to the render site and say so there:
-0.2h ago— an impossible age with the shape of a measurementage or 0.0→oldest 0.0h, above a formatter that cannot represent <3.6 minint(data.get("updated_at", 0) or 0)→as of 1786962010s ago(~56 years), on the warn line and the all-satisfied lineNonefor three distinct conditions, one documented,TimeoutExpiredinvisible inside a genericSubprocessErrorhandlerFive independent fixes for one shape is what a lesson is for. I raised the family on #3027 and
suggested a lint or a REVIEW.md line rather than five more patches; this is that line. The fix is
@qingyun-wu's to make on #3027 — this does not touch it.
Two details I put in the lesson because they are the parts that recur, not the parts that are obvious:
it was on the alarming branch. Find the branch without it — that is the one that lies.
if landed is None: # Cannot tell what landed, so do not use the word. Reviewers should look forthe discipline already present nearby before proposing a new one.
The checkable question
A lesson only earns its place if a reviewer can apply it, so it ends in one:
with the grep-able tell: the same expression supplies both the default and the measurement —
x or 0,.get(k, 0), or anexceptthat returns the empty case.Verification
The
checks:block is untouched, soscripts/review-checks.shbehaviour is unchanged; the onlyconsumer affected is the prose a reviewer reads.
🤖 Generated with Claude Code
https://claude.ai/code/session_018wqEvhuA4hY4casgMzHzNm