Skip to content

test: 5 more roundtrip scenarios (three-way merge, shell-safety, CRLF, outdated, GraphQL resolve)#455

Merged
tomasz-tomczyk merged 1 commit intopr-roundtrip-harnessfrom
harness-extras
May 4, 2026
Merged

test: 5 more roundtrip scenarios (three-way merge, shell-safety, CRLF, outdated, GraphQL resolve)#455
tomasz-tomczyk merged 1 commit intopr-roundtrip-harnessfrom
harness-extras

Conversation

@tomasz-tomczyk
Copy link
Copy Markdown
Owner

Summary

Extends the GitHub roundtrip integration harness with 5 new live-PR scenarios, each probing a distinct comment-sync invariant.

Outcomes

Scenario Outcome
ThreeWayMerge_RemoteEditedSinceLastPush PASS
ShellInjectionSafe PASS
CRLFAndTrailingWhitespace_NoLoop PASS
AnchorLineDeleted_Outdated PASS
GitHubThreadResolvedOnWeb SKIPPED — new issue #453 (#453)

Full suite: 13 PASS, 5 SKIP (4 pre-existing skips for #442/#446/#449 + new #453). 209s wall.

What each scenario probes

  • ThreeWayMerge — confirms a no-op local edit cannot overwrite a reviewer's out-of-band PATCH on github.com.
  • ShellInjectionSafe — security probe: shell metacharacters in comment bodies round-trip literally, no shell invocation.
  • CRLFAndTrailingWhitespace_NoLoop — bodies with CRLF + trailing whitespace survive pull/push/pull/push without a PATCH loop should GitHub canonicalize the stored form.
  • AnchorLineDeleted_Outdated — force-pushing away the line a comment was anchored to preserves the local comment and lets new comments still post.
  • GitHubThreadResolvedOnWeb — GraphQL resolveReviewThread then crit pull must mirror isResolved onto the local resolved flag. Currently fails because the REST pulls/{pr}/comments payload omits thread state — see crit pull does not import GitHub review-thread resolved state #453.

Test plan

  • go test -tags e2e_github -run NONEXISTENT -count=1 ./... (compile)
  • gofmt -l . clean
  • golangci-lint run ./... 0 issues
  • go test ./... (unit suite) green
  • make e2e-roundtrip green (13 PASS, 5 SKIP)

…hQL resolve

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

codecov Bot commented May 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.06%. Comparing base (1920295) to head (b4a6fc2).

Additional details and impacted files
@@                   Coverage Diff                    @@
##           pr-roundtrip-harness     #455      +/-   ##
========================================================
- Coverage                 68.09%   68.06%   -0.04%     
========================================================
  Files                        35       35              
  Lines                      9839     9839              
========================================================
- Hits                       6700     6697       -3     
- Misses                     2628     2630       +2     
- Partials                    511      512       +1     
Flag Coverage Δ
e2e 34.03% <ø> (-0.02%) ⬇️
unit 65.33% <ø> (ø)

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 3e3a9ae into pr-roundtrip-harness May 4, 2026
8 checks passed
@tomasz-tomczyk tomasz-tomczyk deleted the harness-extras branch May 4, 2026 18:59
tomasz-tomczyk added a commit that referenced this pull request May 4, 2026
…hQL 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).
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