fix: PATCH comments and replies whose body was edited (#446)#448
Merged
tomasz-tomczyk merged 1 commit intopr-roundtrip-harnessfrom May 4, 2026
Merged
fix: PATCH comments and replies whose body was edited (#446)#448tomasz-tomczyk merged 1 commit intopr-roundtrip-harnessfrom
tomasz-tomczyk merged 1 commit intopr-roundtrip-harnessfrom
Conversation
crit push only POSTed new comments and never PATCHed existing ones, so local body edits to already-pushed comments were silently dropped. Track last-pushed body per comment/reply; on push, PATCH any whose local body diverges. Re-enables TestRoundtrip_EditPushedCommentBody.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (55.76%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## pr-roundtrip-harness #448 +/- ##
========================================================
- Coverage 68.06% 67.98% -0.09%
========================================================
Files 35 35
Lines 9839 9929 +90
========================================================
+ Hits 6697 6750 +53
- Misses 2630 2665 +35
- Partials 512 514 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tomasz-tomczyk
added a commit
that referenced
this pull request
May 4, 2026
crit push only POSTed new comments and never PATCHed existing ones, so local body edits to already-pushed comments were silently dropped. Track last-pushed body per comment/reply; on push, PATCH any whose local body diverges. Re-enables TestRoundtrip_EditPushedCommentBody.
tomasz-tomczyk
added a commit
that referenced
this pull request
May 4, 2026
* test: add e2e-roundtrip script and Makefile target * test: add roundtrip e2e helpers (no scenarios yet) * test: roundtrip helpers — SSH clone + review_file JSON key The Task 3 helpers had two issues that blocked the first scenario: - Cloned via HTTPS, but `git push -u origin` then prompted for credentials. Clone via SSH (matches gh's git protocol) and let CRIT_ROUNDTRIP_CLONE_URL override. - `crit status --json` emits `review_file`, not `review_path`. Read both keys so existing/future scenarios don't trip on the rename. * test: roundtrip — push idempotency Adds the canonical "delete and recreate" regression scenario: open a clean PR, add two local comments on diff lines, push twice, and assert the remote ends with exactly two comments whose IDs are stable across pushes and whose GitHubIDs are recorded locally. * test: roundtrip — pull idempotency * test: roundtrip — push/pull preserves IDs * test: roundtrip — reply to remote comment (skipped, issue #442) * test: roundtrip — interleaved replies (skipped, issue #442) * test: roundtrip — fresh clone picks up all comments Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: roundtrip — branch switch preserves state * test: roundtrip — force-push doesn't duplicate * test: roundtrip — range comment * docs: roundtrip e2e setup and authoring notes * docs: trigger rule — run e2e-roundtrip when touching PR sync * test: roundtrip — edit/resolve/long-lived scenarios Adds three scenarios probing the literal "comments getting recreated whole" symptom plus a shared editReviewFile helper for body / resolved mutations that have no CLI surface. - TestRoundtrip_EditPushedCommentBody: edit a pushed comment's body and push again. Skipped on issue #446 — push silently drops body edits to already-pushed comments and reports "No comments to push." - TestRoundtrip_ResolveLocallyThenPush: import a remote comment, mark resolved locally, push. Passes — push doesn't recreate, and resolved flag survives a subsequent pull. - TestRoundtrip_LongLivedBranch_NoDrift: 5 rounds of comment/pull/push with mixed local + reviewer activity. Passes — all GitHubIDs stay stable and unique, idempotent push at the end is a no-op. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: roundtrip — foreign edit + local delete scenarios Adds two new probe scenarios against the live GitHub roundtrip: - TestRoundtrip_EditForeignComment_DoesNotPropagate (author-guard probe) - TestRoundtrip_LocalDeletePropagates (skipped, issue #449) Both surface real or potential gaps in crit push behavior. Skipped if they expose bugs; tracked via follow-up issues. * fix: post local replies to imported/pushed parents (#442) (#447) crit push only collected and posted local-only replies inside the `if len(b.Postable) > 0` branch — i.e. only when there were also new top-level comments to publish. If the only thing to push was a reply under a parent that already had a github_id (imported via crit pull or posted by us in an earlier push), the reply walk never ran and the reply was silently dropped. Hoist the reply collection + post + ID write-back out of the top-level-post branch. Replies depend only on the parent already having a github_id, which is independent of whether we're publishing new top-level comments in this push. Skip only when the top-level post itself failed (parent IDs assigned in this same push wouldn't be stable). Re-enables TestRoundtrip_ReplyToRemoteComment and TestRoundtrip_InterleavedReplies. Adds a unit test pinning collectNewRepliesForPush across both parent-source variants (imported-via-pull vs pushed-by-us-previously). * fix: PATCH comments with edited bodies (#446) (#448) crit push only POSTed new comments and never PATCHed existing ones, so local body edits to already-pushed comments were silently dropped. Track last-pushed body per comment/reply; on push, PATCH any whose local body diverges. Re-enables TestRoundtrip_EditPushedCommentBody. * test: roundtrip — three-way merge, shell-safety, CRLF, outdated, GraphQL resolve (#455) Adds 5 high-value scenarios surfaced by the comment-scenario gap-finder: - ThreeWayMerge_RemoteEditedSinceLastPush — push then reviewer edits to "Y"; no-op local edit + push must not overwrite "Y". - ShellInjectionSafe — shell metacharacters in body round-trip literally; no /tmp/CRIT_PWNED_* file appears. - CRLFAndTrailingWhitespace_NoLoop — CRLF + trailing whitespace bodies survive pull/push/pull/push without producing a PATCH loop. - AnchorLineDeleted_Outdated — force-push removing the anchored line preserves the comment locally; new comments still post. - GitHubThreadResolvedOnWeb — GraphQL resolveReviewThread; subsequent pull must mirror isResolved to local resolved flag (skipped, issue #453). * test: unit tests for migration, concurrency, partial failure, sentinels (#454) Adds 5 unit tests covering invariants the live-PR roundtrip harness can't reliably exercise: - session_migration_test.go: legacy review file (pre-#446, no last_pushed_body) parses cleanly into the current CritJSON struct with no data loss; re-marshalling does not invent the field. TODO references #446 for the post-fix assertions. - concurrent_save_test.go: stress saveCritJSON with 4 concurrent read-modify-write workers; the file must remain valid JSON throughout. Probes the atomicWriteFile rename guarantee under daemon/CLI interleaving. - push_partial_failure_test.go: fake `gh` shim on PATH returns 200, 500, 200 across three reply posts; postPushReplies must capture ids for the two successes and not leak an entry for the failure (so retries don't duplicate on GitHub). - github_test.go (TestMergeGHComments_DoesNotValidatePRProvenance): pins the current contract that ghComment carries no PR/branch metadata and merge accepts whatever is fed in. Cross-PR safety must live upstream (in the fetch URL choice). - github_test.go (TestZeroGitHubID_TreatedAsNotPushed): table test pinning the GitHubID==0 sentinel meaning across critJSONToGHComments and collectNewRepliesForPush so it stays consistent across predicates. All pass with -race. No production code changes. * refactor: store body hash instead of full body for push-edit detection Stores first 16 hex chars of SHA-256 instead of duplicating the full body string in the review file. Keeps review files small for the AI agents that read them. No backward compat — the feature has not shipped. - Comment.LastPushedBody / Reply.LastPushedBody → LastPushedBodyHash - New helper bodyHashAtPush() in github.go - collectEditedForPush compares hash-to-hash - Stamping sites updated: pull import, post-POST, post-PATCH - session_migration_test.go deleted (forward-compat moot without compat) - Tests updated to use the hash form --------- Co-authored-by: Claude Opus 4.7 (1M context) <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
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.
Closes #446.
Problem
`crit push` only POSTed new comments and never PATCHed existing ones, so local body edits to already-pushed comments were silently dropped: re-pushing printed `No comments to push.` and the remote stayed stale.
Fix
Track the last-pushed body per comment/reply, and on push PATCH any whose local body diverged.
Tests
Verification
Base branch
Targets `pr-roundtrip-harness` (PR #445), not `main`, because the un-skipped scenario lives on that branch.
🤖 Generated with Claude Code