docs: three-command review split design#61
Merged
Conversation
5 tasks
There was a problem hiding this comment.
Based on my analysis, this is a docs-only PR. Let me compile the review findings:
Changed files: CHANGELOG.md, docs/three-command-review-split.md
The diff is entirely prose documentation — a well-structured design note. The mechanical funnel (ast-grep) scans for code patterns in .go/.py files; there are none in this diff, so no mechanical findings are expected.
Manual review findings:
The design note is generally well-crafted. The open questions at the bottom are appropriately flagged. However:
- Baseline file location — The doc sketches
.code-review-baseline.yamlin the repo root, but doesn't resolve the trade-off (discoverable vs. noisy). The decision should be made before PR 4 ships, not deferred. - Monorepo handling — The open question notes this but offers no principled heuristic. A default of "one per repo, with override mechanism" would be more useful than leaving it entirely open.
--refresh-baseline+ dirty tree — The open question asks "should it require clean?" but the doc doesn't propose an answer. Given the rationale stated (avoid baking in accidental cruft), the answer is almost certainly "yes, require clean", which should be stated as the default assumption.
These are all appropriately captured as open questions rather than Must Fix items for a draft design note.
{
"verdict": "approve",
"summary": "Docs-only design note that thoughtfully partitions three review command scopes and proposes a clean 5-PR migration plan. The open questions at the bottom are appropriately marked as deferred, but baseline file location and monorepo handling should be resolved before PR 4 rather than left open-ended.",
"comments": [
{
"file": "docs/three-command-review-split.md",
"line": 97,
"severity": "minor",
"message": "Should Fix: baseline file location trade-off is left unresolved ('repo root vs hidden'). Propose a default: repo root `.code-review-baseline.yaml` with an override path via CLI flag. Decision needed before PR 4, not deferred further."
},
{
"file": "docs/three-command-review-split.md",
"line": 100,
"severity": "minor",
"message": "Should Fix: monorepo handling (one baseline vs per-project) lacks a default heuristic. Suggest defaulting to one baseline per repo with `--baseline-path=<subproject/.code-review-baseline.yaml>` override for monorepos."
},
{
"file": "docs/three-command-review-split.md",
"line": 91,
"severity": "minor",
"message": "Should Fix: `--refresh-baseline` + dirty working tree is posed as an open question, but the stated rationale ('avoid baking in accidental local cruft') strongly implies 'yes, require clean'. State this as the intended default rather than leaving it as a question."
}
],
"concerns_addressed": [
"tests: baseline file location (.code-review-baseline.yaml vs .claude/code-review-baseline.yaml) — raised as Should Fix; open question still needs resolution before PR 4",
"tests: monorepo handling (one baseline vs per-project) — raised as Should Fix; needs a default heuristic before PR 4",
"tests: .gitignore/.claude-ignore interaction with auditor — doc correctly defers this; not a concern for a design note",
"tests: whether --refresh-baseline requires a clean working tree — raised as Should Fix; answer is almost certainly yes per stated rationale"
]
}…resh-baseline clean-tree requirement, gitignore semantics
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Design note locking the shape of the proposed review-command split. No code change in this PR — locks the contract before any command files move.
/coding:pr-review(unchanged) — remote, branch diff vs target/coding:local-review(renamed from current/coding:code-review) — local, uncommitted/recent/coding:code-review(new, built ground-up) — whole codebase, severity-filtered, baseline-awareWhy
Current
/coding:code-reviewdoes a 1-commit diff against HEAD~1 — operator's mental model when typing the name is "review the code", not "review this delta". Aligning behavior with name reduces "which one does what?" confusion. The contrast pairpr-review(remote) /local-review(local) reads cleanly; the new whole-codebase command takes the unmarkedcode-reviewslot.Critical mechanisms — without these the whole-codebase command is worse than nothing (drowns in pre-existing tech debt):
.code-review-baseline.yaml— operator commits accepted findings; subsequent runs flag NEW findings only ("what's drifted since last sweep")golangci-lintpassthrough (Go projects)Migration plan
5 PRs across one minor-version cycle:
commands/code-review.md→commands/local-review.mdcommands/code-review.mdpointing to the new namecommands/code-review.mdinvokes the new logicTest plan
.gitignoreinteraction,--refresh-baselineclean-tree requirement)