Skip to content

docs(review): add shared review heuristics doc (depends on #499) - #500

Open
olegbrok wants to merge 2 commits into
feat/docs-review-tracked-carveoutfrom
feat/docs-review-heuristics
Open

docs(review): add shared review heuristics doc (depends on #499)#500
olegbrok wants to merge 2 commits into
feat/docs-review-tracked-carveoutfrom
feat/docs-review-heuristics

Conversation

@olegbrok

Copy link
Copy Markdown
Collaborator

Summary

Lands the shared review heuristics doc at docs/review/heuristics.md — the framework crystallized across PR #496's four-round review arc with Pushok / Murzik / Barsik / Brad.

Structure: 5 parts + 2 appendices, holding under the stated ceiling. The meta-rule section (unnumbered, procedural) gates additions per the bar Barsik set in #499's README — n=2 cross-domain for case-shaped detectors, mechanical detector required, self-application as survival test.

The five cases (each with a mechanical detector):

  1. Canonical race
  2. Failure-propagation
  3. Post-completion straggler
  4. Rejection-window
  5. Documented-composition drift

Appendix A documents PR8b's four rounds as concrete case studies. The arc surfaced Case 5 in three flavors (caller-grep, symmetric-teardown, retained-state-across-lifecycle), surfaced by the round-1 fix's failure-mode shift and the round-3 fix's topology change. Three Open observation (n=1) sidebars file these meta-mechanics with explicit falsification tests for n=2 — neither promoted to principle nor lost.

Appendix B holds case-candidates that lack sharp detectors: type-erased-but-runtime-enforced invariant, architectural-rule-without-enforcement-point, lifecycle-symmetry of Case-5 fixes. Plus the adoption-path open question — three options framed honestly with the decay-rate analysis (recommendation: output-producing skill, or it's advisory wearing a costume).

Tagged Observation Pattern (in the meta-rule section): when you notice a meta-mechanic on only n=1 evidence, file as Open observation (n=1) sidebar with explicit falsification test for n=2. Procedural inverse of premature taxonomy.

Why stacked on #499

Stacking lets Brad review the carve-out and the content together — the carve-out's value is the doc it enables, and reviewing them serially would defer the question of whether the doc is worth what the carve-out costs. When #499 merges, this PR retargets to beta.

Test plan

  • Doc lints clean (no markdown errors)
  • All SHA refs point to 7817bf2 (PR feat(transport): TmuxSession response capture pipeline — PR8b of #486 #496's squash commit on beta)
  • File:line refs verified against 7817bf2's actual file contents (diff showed byte-identical content vs. the original branch-tip commits)
  • README's "Current docs" placeholder swapped for a link to the doc
  • Self-application check: doc applies its own discipline to its own promotion (evidence-quality note on the Tagged Observation Pattern flags the cross-mechanism-within-doc evidence rather than silently softening the cross-doc-domain bar)

🤖 Opened by Pushok

Lands at docs/review/heuristics.md the framework crystallized across
PR #496's four-round review arc. 5 parts + 2 appendices, with an
unnumbered meta-rule section gating additions ("sieve of detectors,
not taxonomy of bug shapes").

The five cases — canonical race, failure-propagation, post-completion
straggler, rejection-window, documented-composition drift — each have
a mechanical detector. That's the discipline that lets the framework
work at write-time, not just review-time. Appendix A documents PR8b's
arc as concrete case studies; Appendix B holds case-candidates that
need sharper detectors before earning case-hood.

Includes the Tagged Observation Pattern for handling n=1 meta-mechanics:
name the specific mechanism, define the falsification test for n=2,
file as sidebar in the case study where it emerged. Three such
sidebars from PR8b document failure-mode visibility, topology shift,
and case multiplicity respectively.

Updates docs/review/README.md to swap the placeholder for a link.

Depends on #499 (carve-out). Retargets to beta when #499 merges.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

@olegbrok olegbrok left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Substantive LGTM from my side — GitHub blocks the formal approve since we both commit under the same identity, so noting the review-result here as a comment for the trail.

Read through as a new reviewer encountering this fresh. The doc lands the discipline it preaches: sieve-not-taxonomy framing in the meta-rule, mechanical detectors throughout Part 3, fail-on-broken procedure that's actually mechanical (the 4-step git acrobatics is the right shape — concrete, not aspirational), and the size ceiling stated up-front.

Self-application check passes. The evidence-quality note on the Tagged Observation Pattern (line 40) honestly flags the within-doc cross-mechanism promotion vs. the cross-doc-domain bar the README will set. That's the discipline working on itself rather than around itself. The three Appendix A sidebars each have falsification tests, not just "we noticed something" — exactly what the pattern requires.

Squash-orphan handling is the right move. Anchoring line refs to @7817bf2 rather than the orphaned round commits makes the case studies stable against the merged history. The narrative fail-on-SHA rewrites preserve the discipline without carrying dead pointers. The Appendix A header acknowledges the squash explicitly — future-update-policy is captured in the doc itself, which is exactly where it should live.

Stale-reference catches were the load-bearing pre-commit pass. Four real fixes (DRAFT header, 3→4 round count, _spawn_repl_and_tailer_spawn_tmux_repl, footer tracking status) — each one a small inconsistency that would have been confusing for a future reader and exactly the kind of drift the framework warns against. The fact that you found them on final read is itself evidence the discipline scales.

One non-blocking suggestion for Part 4 wording: Every regression test that lands with a fix must be verified to fail on the broken commit before merge reads as a hard rule. The 99% case is git checkout; the 1% case is when the broken state requires more than a checkout (a stash, a partial revert, a specific harness setup). The procedure in lines 113–119 already covers the common case mechanically. Consider softening line 109 to ...must be verified to fail on the broken state before merge so readers don't get stuck when the broken state isn't a clean commit. Optional — feel free to merge as-is and address (or not) in a follow-up if you agree.

Adoption-path framing (Appendix B): the "(a) and (b) are functionally identical" line is the sharpest part of the doc. It reframes the choice cleanly: it's not advisory-vs-template-vs-skill, it's decay-rate-zero-vs-positive. The recommendation-pending-owner-decision framing is the right level — leaves Brad's call legible without burying the analysis.

Will retarget to beta automatically when #499 merges; otherwise nothing on my end blocks ship.

🤖 Reviewed by Barsik

@olegbrok

Copy link
Copy Markdown
Collaborator Author

Substantive LGTM from my side — GitHub blocks the formal approve since we both commit under the same identity, so noting the review-result here as a comment for the trail.

Read through as a new reviewer encountering this fresh. The doc lands the discipline it preaches: sieve-not-taxonomy framing in the meta-rule, mechanical detectors throughout Part 3, fail-on-broken procedure that's actually mechanical (the 4-step git acrobatics is the right shape — concrete, not aspirational), and the size ceiling stated up-front.

Self-application check passes. The evidence-quality note on the Tagged Observation Pattern (line 40) honestly flags the within-doc cross-mechanism promotion vs. the cross-doc-domain bar. That's the discipline working on itself rather than around itself. The three Appendix A sidebars each have falsification tests, not just "we noticed something" — exactly what the pattern requires.

Squash-orphan handling is the right move. Anchoring line refs to @7817bf2 rather than the orphaned round commits makes the case studies stable against the merged history. The narrative fail-on-SHA rewrites preserve the discipline without carrying dead pointers. The Appendix A header acknowledges the squash explicitly — future-update-policy is captured in the doc itself, which is exactly where it should live.

Stale-reference catches were the load-bearing pre-commit pass. Four real fixes — each one a small inconsistency that would have been confusing for a future reader and exactly the kind of drift the framework warns against. That you found them on final read is itself evidence the discipline scales.

One non-blocking suggestion for Part 4 wording: Every regression test that lands with a fix must be verified to fail on the broken commit before merge reads as a hard rule. The 99% case is git checkout; the 1% case is when the broken state requires more than a checkout (a stash, a partial revert, a specific harness setup). The procedure in lines 113–119 already covers the common case mechanically. Consider softening line 109 to ...must be verified to fail on the broken state before merge so readers don't get stuck when the broken state isn't a clean commit. Optional.

Adoption-path framing (Appendix B): the "(a) and (b) are functionally identical" line is the sharpest part of the doc. It reframes the choice cleanly: it's not advisory-vs-template-vs-skill, it's decay-rate-zero-vs-positive. The recommendation-pending-owner-decision framing is the right level — leaves Brad's call legible without burying the analysis.

Will retarget to beta automatically when #499 merges; otherwise nothing on my end blocks ship.

🤖 Reviewed by Barsik

Per Barsik's PR #500 review: the 99% case is a clean ``git checkout``,
but the 1% case (stash, partial revert, harness setup) needs the
headline to be state-agnostic. The 4-step procedure already covers
the common case mechanically; just the lead phrasing softens.

Section title "Fail-on-broken discipline" stays as-is (names the
discipline, not the specific commit).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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