Record four lessons from ai-config#1042's review lifecycle - #1104
Conversation
From `hooks/no-unreviewed-pr.py`'s 9-round adversarial review (the hook is unregistered until post-merge): - `shared/principles/fail-fast.md`: extend the partial-guard section with the parallel-discharge-paths instance -- four sibling discharge/identity paths (open/draft/request/self) guarded one at a time across review rounds, which is the partial-guard failure stretched across rounds instead of shipped. - `memories/git.md`: `git commit -m "..."` runs backtick spans as shell command substitution and drops them from the message; use `-F <file>` or single-quoted `-m`. The bash counterpart of CLAUDE.md's PowerShell backtick warning. - `memories/tools.md`: two `shlex` gotchas when splitting a shell command into simple commands in Python -- newlines are dropped rather than emitted as separators, and the default `punctuation_chars` set conflates redirection (`<`,`>`) with control operators. - `shared/workflow/algorithmatize-checks.md`: mutation-check each clause of an ANDed guard condition in isolation; a sibling clause masks the mutation the same way a sibling test case aborts the suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015icfsSybE9vMSt8cUrXB6X
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code reviewReviewed This is a UMS pass recording four lessons from ai-config#1042's review lifecycle. I fact-checked the claims against the actual source PR (#1042, merged) rather than taking them at face value, since the PR description says this content may be partly machine-authored. Confirmed accurate: the One finding, posted inline: the round-number attributions in the No other CLAUDE.md/lab-manual violations found. This is prose-only, and the additions follow the corpus's existing conventions for extending a section vs. creating a new one, semantic line breaks, and citation style. VerdictNeeds more work. One finding: the round-number citations in the |
|
Claude finished review — View run Code reviewReviewed This is a UMS pass recording four lessons from ai-config#1042's review lifecycle. I fact-checked the claims against the actual source PR (#1042, merged) rather than taking them at face value, since the PR description says this content may be partly machine-authored. Confirmed accurate: the One finding, posted inline: the round-number attributions in the No other CLAUDE.md/lab-manual violations found. This is prose-only, and the additions follow the corpus's existing conventions for extending a section vs. creating a new one, semantic line breaks, and citation style. VerdictNeeds more work. One finding: the round-number citations in the |
Review of #1104 found the round-number attributions in the fail-fast.md partial-guard case record ("nine-round review", "round 8 the open path", "round 9 the self path") do not match #1042's actual review history: the open_ident fix was that PR's round 13, the review ran past round 13, and the same file's existing #1042 discharge case record already says "~12 review rounds / rounds 8-10". The two numbering schemes (this session's count vs the PR's own comment numbering) are irreconcilable and add no value to the lesson. Describe the pattern by PATH instead (shell-command parser, then the open path, then the self discharge), dropping the round numbers in both the fail-fast.md addition and the memories/tools.md shlex citation. The lesson (guard all parallel sibling paths in one change) does not depend on the numbering.
Code reviewReviewed memories/git.md, memories/tools.md, shared/principles/fail-fast.md, and shared/workflow/algorithmatize-checks.md at head 090be42 (96 additions, 0 deletions across the branch; this rounds fix commit touched shared/principles/fail-fast.md and memories/tools.md). Prior finding resolved. The round-number citation mismatch flagged in the previous round (round 8/9/nine-round-review attributions that did not match the source PRs actual review history) is fixed in commit 090be42: the shared/principles/fail-fast.md case record now describes the sequence by path (the shell-command parser underlying them, then the open path (open_ident), then the self discharge) instead of by round number, and the memories/tools.md shlex citation dropped its round reference too. One new finding, posted inline on memories/git.md line 355: the new backtick-command-substitution memory illustrative example relies on backslash-escaping a backtick inside a single-backtick code span, which CommonMark does not support (backslash escapes are not processed inside code spans, and delimiter matching is purely lexical). I traced the delimiter matching by hand and confirmed live via the rendered blob at this PRs head, it renders as garbled fragments rather than the intended two-span illustration. Suggested fix (verified to render correctly): use a double-backtick delimiter for the span containing the embedded backtick. Independently re-verified the two shlex reproduction snippets in memories/tools.md (both behave exactly as shown) and confirmed the cited source PR is merged. No other CLAUDE.md/lab-manual violations found; internal cross-reference links resolve correctly, no banned punctuation on added lines, and no multi-sentence-line violations. VerdictNeeds more work. One finding: fix the broken Markdown code-span rendering in memories/git.md (see inline comment) before merge. |
|
Claude finished review — View run Review complete. Summary of findings: PR: Morrison-Lab/ai-config#1104 — "Record four lessons from ai-config#1042's review lifecycle" (UMS pass, prose-only, 4 files, 96 additions) Prior finding check: The round-number citation mismatch I flagged in the previous review round was fixed in commit New finding (posted inline on Independently re-verified: the two VerdictNeeds more work. One finding — the broken Markdown rendering in |
The example wrapped an outer code span in single backticks and used backslash-escaped backticks for the inner `slast` span. CommonMark does not process backslash escapes inside code spans, so the escapes rendered literally on GitHub and garbled the example. Switch the outer span to a double-backtick delimiter, which lets the inner single backticks be literal without escaping. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015icfsSybE9vMSt8cUrXB6X
Code reviewReviewed memories/git.md, memories/tools.md, shared/principles/fail-fast.md, and shared/workflow/algorithmatize-checks.md at head 4b30781 (97 additions, 0 deletions). This round's diff versus the previous reviewed head (090be42) is two fix commits: a merge from main, and 4b30781 (Fix broken Markdown code-span in git.md commit-substitution example). Both prior findings are resolved, independently re-verified.
Fresh checks on this round's diff, no new issues found:
No other CLAUDE.md/lab-manual violations found. VerdictReady for merge. |
|
Claude finished review — View run The confirming review posted successfully: #1104 (comment) SummaryReviewed PR #1104 at head
Fresh checks on this round's diff (no new issues): no banned punctuation, no multi-sentence-line violations, all cross-reference links resolve, and I independently executed both VerdictReady for merge. |
2 participants
UMS pass capturing lessons from ai-config#1042's multi-round adversarial review of
hooks/no-unreviewed-pr.py(the hook is unregistered until post-merge, so this PR touches nohooks/and does not interact with #1042's branch).This is a disjoint memories/skills PR: it edits four existing sections/files, adding hand-written prose (no generated trees).
Lessons recorded
Full structural mirror across parallel guard paths -- extended
shared/principles/fail-fast.md's existing "partial is worse than absent" section (did not create a new one). The new note covers the parallel-discharge-paths instance:no-unreviewed-pr.pyhas four sibling open/draft/request/self discharge-and-identity paths, and the fail-safe guard was applied to them one at a time across the review, each subsequent round surfacing the one path still unguarded: the shell-command parser underlying them, then theopenpath (open_ident), then theselfdischarge. That is the partial-guard failure stretched across rounds rather than shipped at once. Cross-references the discharge-mechanics section below it. (The earlier round-numbered phrasing was corrected during this PR's own review — the two numbering schemes in play, this session's own round count and Add a Stop hook catching a PR opened without a reviewer request #1042's internal review-comment numbering, did not reconcile, so the pattern is described by path instead.)bash
git commit -m "..."runs backtick spans as command substitution -- new section inmemories/git.md. The shell executes the backticked text and drops it from the message; hit live on Add a Stop hook catching a PR opened without a reviewer request #1042 (commit97bf7d4lost its backtick spans). The bash counterpart ofCLAUDE.md's PowerShell backtick warning, by a different mechanism. Remedy:git commit -F <file>or single-quoted-m. (This PR's own commits used-F.)Python
shlexshell-command-splitting gotchas -- new section inmemories/tools.md, alongside the existing shell-gotcha family: (a)shlexdrops an unquoted newline rather than emitting it as a separator, merging two lines' commands; (b)punctuation_chars=Trueuses the default set();<>|&, conflating redirection (<,>) with control operators, so a redirect wrongly attaches inside a command. Use;&|()as the separator set and convert newlines to;first.Mutation-check each clause of an ANDed guard in isolation -- extended
shared/workflow/algorithmatize-checks.md's non-vacuity section (already cites Carry forward the whole-file punctuation-replace hook stranded by #1045's merge #1047'sfuzz()). A sibling clause in the same condition masks the mutation the same way a sibling test case aborts the suite: reverting one clause ofif a and b and cstill passes any case another clause keeps correct. Isolate each clause with a case where only that clause keeps the result correct.Verification
origin/main; push verified againstgit ls-remote(matches HEAD).origin/main...HEAD: 0 hits on added lines..mdlines: control OK, 0 flagged.🤖 Generated with Claude Code
https://claude.ai/code/session_015icfsSybE9vMSt8cUrXB6X