Skip to content

fix: post local replies whose parent has a GitHub ID (#442)#447

Merged
tomasz-tomczyk merged 1 commit intopr-roundtrip-harnessfrom
fix-442-reply-push
May 4, 2026
Merged

fix: post local replies whose parent has a GitHub ID (#442)#447
tomasz-tomczyk merged 1 commit intopr-roundtrip-harnessfrom
fix-442-reply-push

Conversation

@tomasz-tomczyk
Copy link
Copy Markdown
Owner

Closes #442.

Problem

`crit push` posted nothing when the only local-only items were replies under parents that already had a `github_id` — whether from `crit pull` (imported) or from a prior push. To the user this looks like "I replied, my reply vanished."

Root cause

In `runPushLive` the entire reply-collection-and-post block was nested inside `if len(b.Postable) > 0`. If the push had no new top-level comments, that branch never ran. `collectNewRepliesForPush` itself was correct.

Fix

Hoist reply collection, posting, and ID write-back out of the new-roots conditional. Replies only need `parent.GitHubID != 0`, which is independent of whether new top-level comments are also being posted. Guarded by `!postFailed` so we don't post replies referencing parent IDs that the same batch failed to create. Idempotency is preserved by the existing `r.GitHubID == 0` gate in `collectNewRepliesForPush`.

Tests

  • Re-enables `TestRoundtrip_ReplyToRemoteComment` and `TestRoundtrip_InterleavedReplies` (removed `t.Skip` lines from PR test: GitHub PR roundtrip integration harness #445's harness).
  • Adds unit test `TestCollectNewRepliesForPush_ParentSources` pinning the three relevant parent states.

Verification

Base branch

This PR targets `pr-roundtrip-harness` (PR #445), not `main`, because the un-skipped tests live on that branch.

🤖 Generated with Claude Code

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).
@codecov
Copy link
Copy Markdown

codecov Bot commented May 4, 2026

Codecov Report

❌ Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.03%. Comparing base (c8bcf84) to head (d2821d6).

Files with missing lines Patch % Lines
main.go 0.00% 9 Missing ⚠️

❌ Your patch check has failed because the patch coverage (0.00%) 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     #447      +/-   ##
========================================================
- Coverage                 68.06%   68.03%   -0.03%     
========================================================
  Files                        35       35              
  Lines                      9839     9843       +4     
========================================================
  Hits                       6697     6697              
- Misses                     2630     2634       +4     
  Partials                    512      512              
Flag Coverage Δ
e2e 34.00% <0.00%> (-0.02%) ⬇️
unit 65.30% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tomasz-tomczyk tomasz-tomczyk merged commit 05e8641 into pr-roundtrip-harness May 4, 2026
7 of 8 checks passed
@tomasz-tomczyk tomasz-tomczyk deleted the fix-442-reply-push branch May 4, 2026 18:58
tomasz-tomczyk added a commit that referenced this pull request May 4, 2026
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).
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>
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