Complete CP1 to CP2 vertical slice - #6
Merged
Merged
Conversation
Review finding 7: every CLI command's except branch relies on this helper always raising; with a plain None return type, a future edit that stops raising would leave five commands reading an unbound result. NoReturn lets mypy enforce the assumption. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 1, resolved as author-approved option C: get_status used to persist the reconciled atom cache without holding SessionLock, an unlocked read-modify-write that could clobber a concurrent locked assign or publish save (losing fresh attribution or PR-number mappings). Reconciliation is deterministic, so every verb re-derives identical atoms from the live diff and the cache write bought nothing. Drop the save; the status test now pins the session file byte-identical across status runs. IMPLEMENTATION_PLAN.md §4 invariant 3, §2.7, and A7 were revised to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 4: on a duplicate slice marker, _upsert_slice_pr re-listed open PRs through find_open_pr_by_marker just to trigger its raise; if the duplicate had been closed between the two listings, the fallback surfaced a bare AssertionError with no guidance. Build the DuplicatePullRequestMarkerError from the local matches instead, which also drops a network call made purely to raise. A publish-level integration test now covers the duplicate path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 2: upsert_draft_pr_by_marker, PullRequestUpsert, find_open_pr_by_marker, and MissingPullRequestMarkerError had no production caller — publish implements marker-first upsert itself against one batched open-PR listing — so the module carried a second, subtly different implementation of the same invariant, and five tests exercised only that dead code. The duplicate-marker behavior is covered at the publish level; the protocol-conformance test for FakeBackend stays. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 5: gh pr list ran with a silent --limit 1000. In a repository with more open PRs, marker search over the truncated listing could miss an existing slice PR and let publish create a duplicate, breaking the stable PR identity invariant. Fail loudly when the listing fills the limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 6: deriving the integration PR base from the session's base ref only works when that ref names a branch, plain or prefixed with the publish remote. Record the assumption and that other init --base forms (an OID, a tag, another remote) fail loudly at gh pr create rather than silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 8: docstrings and the evidence record cited T05/T07/T09/T12, labels defined only in local planning documents that are not part of the repository, so a reader here cannot resolve them. The scope statements stay (what is implemented, what is deliberately deferred); only the unresolvable ids go. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 9, passing through check-code-prose: the projection module
docstring had been edited in place instead of rewritten; the publish module
docstring ranked itself ('the only core path') instead of stating the rule,
and carried a task id; and the batch-removal comment claimed the delete-
before-set ordering was about determinism when it is what lets a path swap
between file and directory without colliding with its old entry.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 3: the closing sentence was both garbled ('Author sign-off
remains awaiting author sign-off') and stale — it was written before the
author approved A3/A4/A5 and signed off CP2 on 2026-08-29, and contradicted
the execution status log and the PR description. State the sign-off as it
happened.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 10, from check-md-style: two list items carried three sentences each and are now split; lowercase base/head sub-items open capitalized; and the bold A3/A4/A5 label bullets read as plain sentences. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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
Validation
Commit structure
The private scratch repository and Draft PR evidence are linked from docs/evidence/cp2-live-2026-08-29.md.