fix(sync): avoid downloading unchanged issue comments - #203
Conversation
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. Reviewed September 14, 2026, 3:46 PM ET / 19:46 UTC. ClawSweeper reviewWhat this changesReuse saved issue and PR discussion comments when parent timestamps, counts, and completed observations match, while adding a force-refresh flag and keeping selected PR review data live. Merge readiness⛔ Blocked before merge - 3 items remain The optimization remains useful and is absent from main and v0.10.0. The implementation and supplied live measurements support it, but changing default comment freshness needs explicit maintainer acceptance. Priority: P2 Review scores
Verification
How this fits togetherGitcrawl synchronizes GitHub discussions into a local SQLite archive used for search and revision evidence. This change decides whether to download general comments or retain validated saved rows before committing each thread. flowchart TD
A[Sync or refresh request] --> B[Fetch GitHub parent]
B --> C{Saved comments reusable?}
D[SQLite observations and payloads] --> C
C -->|Yes| E[Retain saved comment IDs]
C -->|No or force| F[Download comments]
E --> G[Revalidate and commit archive]
F --> G
B --> H[Fetch selected PR reviews and details]
H --> G
Decision needed
Why: The patch documents and demonstrates its optimization, but neither the live measurements nor matching parent signals establish that every comment edit is observable; accepting that default tradeoff requires product intent. Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Provide conservative comment reuse with a maintainer-approved freshness contract, preserving unconditional downloads by default if that compatibility guarantee remains required. Do we have a high-confidence way to reproduce the issue? Yes: current main unconditionally requests comments on repeated selected syncs, matching the reporter's transcript and the supplied baseline measurements; no runtime reproduction was executed here. Is this the best way to solve the issue? The reuse implementation is appropriately narrow, but its default freshness policy remains a product choice; opt-in reuse would preserve existing behavior. AGENTS.md: not found in the target repository. Codex review notes: model internal, reasoning medium; reviewed against fda087ac618b. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
vlsi
left a comment
vlsi
left a comment
There was a problem hiding this comment.
I ran the new tests against the base commit and ran one mutant against the reuse path. The regression tests fail on fda087a for the right reason, but no test catches a reused comment being dropped from the saved observation, and that drop brings the duplicate download back one sync later. Line-level notes are inline.
The tests on the base commit
The syncer test file does not compile on fda087a (opts.Force undefined), so I replaced the four .Force assignments with _ = true / _ = false and left the rest of the file unchanged. The CLI test ran as is.
Test on fda087a |
Result | Output |
|---|---|---|
TestSyncReusesUnchangedIssueComments/issue and /pull_request |
FAIL | unchanged thread re-downloaded comments: calls=2 synced=1 received=1 |
TestSyncCommentReuseKeepsPRReviewEvidenceLive |
FAIL | calls=2 reviews=2 … CommentsSynced:2 |
TestSyncCommentReuseEmptyAndDeletedMembership |
FAIL | empty completed snapshot not reused: 3 |
TestSyncCommentReuseRejectsConcurrentReplacement |
FAIL | err=<nil> |
TestSyncAndRefreshForceCommentDownload/sync and /refresh |
FAIL | run=1 calls=2 want=1 |
TestSyncCommentReuseInvalidation, all 10 subtests |
PASS | expected, because the base commit always downloads |
The failures show the #198 symptom rather than a missing symbol, so it would help to paste this output into the description. TestSyncCommentReuseInvalidation guards against reusing too eagerly and cannot tell the base commit from the fix; that is fine for what it tests. All new tests pass on 1f486b8.
A reused comment dropped from the observation goes unnoticed
I deleted one line in persistComments:
if row.reusedID != 0 {
- observedIDs = append(observedIDs, row.reusedID)
continue
}With this mutant, every new test passes, and so do the full internal/syncer and internal/store packages. The mutant does change behavior: after the second sync the latest comment membership is [], so the third sync no longer finds a reusable snapshot and downloads the comments again. A throwaway test that passes on 1f486b8 fails on the mutant:
probe_test.go:43: latest comment membership = [], want [1]
probe_test.go:49: ListIssueComments calls after third sync = 2, want 1
The inline suggestion on TestSyncReusesUnchangedIssueComments adds the third sync.
Unrelated to this PR: TestSyncPortableStoreReturnsResetFailure/dirty-before-pull=false fails for me on both fda087a and 1f486b8 (exit status 1 instead of exit 79), so it looks environmental.
| if count != 1 { | ||
| t.Fatalf("reuse lost archived comments: %d", count) | ||
| } |
There was a problem hiding this comment.
This test passes when a reused comment is dropped from the saved observation (the mutant in the review body): the row is still there, so the count stays 1. The next sync then downloads the comments again. A third sync catches it:
| if count != 1 { | |
| t.Fatalf("reuse lost archived comments: %d", count) | |
| } | |
| if count != 1 { | |
| t.Fatalf("reuse lost archived comments: %d", count) | |
| } | |
| if _, err := s.Sync(ctx, opts); err != nil { | |
| t.Fatal(err) | |
| } | |
| if client.commentCalls != 1 { | |
| t.Errorf("ListIssueComments calls after third sync = %d, want 1", client.commentCalls) | |
| } |
If you also want the revision-evidence claim in docs/sync.md covered directly, assert after the second sync that the comment's ID is in the latest thread_child_observation_memberships row for comments and that its deleted_at is empty.
| t.Fatal(err) | ||
| } | ||
| if client.commentCalls != before+1 { | ||
| t.Fatal("invalidated comments were reused") |
There was a problem hiding this comment.
A failure here prints no values, so a red CI run doesn't say which scenario reused what. Something like t.Fatalf("ListIssueComments calls = %d, want %d", client.commentCalls, before+1) does. The same applies to:
- line 206,
t.Fatalf("err=%v", err), which doesn't say which error was expected; - line 242,
t.Fatalf("first: %+v", first), whereCommentsSynced = %d, want 1names the field; - line 252,
t.Fatal("PR reviews must remain live"); - line 176, where five conditions joined with
||share one message that prints the values but none of the expectations. Separatet.Errorfcalls report every mismatch in one run.
| } | ||
|
|
||
| func TestSyncCommentReuseInvalidation(t *testing.T) { | ||
| for _, scenario := range []string{"timestamp", "count", "missing-count", "malformed-time", "force", "missing-membership", "missing-member", "metadata-only", "failed-force", "pruned"} { |
There was a problem hiding this comment.
The subtest names describe the input, so the report reads TestSyncCommentReuseInvalidation/count or /pruned. A name that states the outcome reads as a finding on its own, for example changed_comment_count_downloads_again or pruned_payload_downloads_again.
| if client.commentCalls != before+1 { | ||
| t.Fatal("invalidated comments were reused") | ||
| } | ||
| if scenario == "pruned" { |
There was a problem hiding this comment.
This condition decides whether the body assertion runs at all, and the switch at line 57 hides each scenario's setup away from its expectation. Line 251 (if number == 8) and line 222 (map[int]string{...}[number] as the subtest name) have the same shape. A table of rows such as {name, arrange func(...), wantCalls, wantBody} keeps each scenario's setup and expected values in one place, and every assertion runs for every row.
| } | ||
| } | ||
| stats, err := s.Sync(ctx, opts) | ||
| if err == nil || !strings.Contains(err.Error(), "saved issue comments changed") { |
There was a problem hiding this comment.
Matching the message text ties the test to the wording. An exported sentinel (for example ErrSavedCommentsChanged, wrapped with %w together with the issue number) lets this check use errors.Is, and callers that want to retry can use it too.
| return nil, fmt.Errorf("read reusable issue comments: %w", err) | ||
| } | ||
| observed, err := time.Parse(time.RFC3339Nano, source) | ||
| if err != nil || !observed.Equal(updated) || snapshot.ObservationSequence <= 0 { |
There was a problem hiding this comment.
No test feeds the same instant in another form (+00:00 instead of Z, or fractional seconds), so replacing Equal with a string comparison keeps every test green. One invalidation-table row where the fresh updated_at is 2026-04-26T00:00:00+00:00 and reuse still happens would pin the Equal semantics.
Verify the exact live comment membership after reuse and exercise a third sync. Cover equivalent timestamp representations and improve diagnostics. Thanks @vlsi for the mutation testing and regression suggestion. Co-authored-by: Vladimir Sitnikov <213894+vlsi@users.noreply.github.com>
|
Thanks @vlsi. The test follow-up in 20e3148 now checks exact membership and the live, unmodified comment after the second sync, then performs a third sync. It also covers equivalent timezone/fraction timestamps and improves the failure diagnostics. Both mutants are now rejected. Dropping the reused ID reports I kept the existing error interface; a new exported sentinel solely for this assertion would widen the production change. The independent review of the test follow-up is clean through P2. The default-policy decision is also recorded in the description: automatic issue-comment reuse is the maintainer-selected behavior for #198, with |
2 participants
What Problem This Solves
Repeated
sync --include-commentsruns download the general discussion of every returned issue and PR, even when GitHub reports no change. Fixes #198. Thanks @vlsi for the report, reproduction, and mutation testing.User Impact
Unchanged issue comments are reused automatically on issues and PRs.
sync --forceandrefresh --forcere-download selected data within the existing scope. PR reviews, inline review comments, review threads, files, commits, checks, and workflows remain live when selected; parent timestamps alone cannot validate them.Maintainer decision for #198: automatic issue-comment reuse is the selected default, with
--forcefor changes invisible to the parent signals. This is the intended freshness tradeoff for existing invocations.Why This Change Was Made
The acquisition loop previously fetched comments before consulting any saved observation. Reuse now requires a matching fresh parent timestamp, matching comment count, exact completed membership, retained payloads, and no unresolved issue-comment failure. The item transaction revalidates the saved generation before retaining those IDs, preserving observation ordering and revision evidence without counting cached rows as downloads. Missing, failed, pruned, or changed data is fetched again. Changes that move neither parent signal can be refreshed with
--force.Evidence
Regression tests first reproduced the duplicate request on both issues and PRs. Tests cover invalidation, missing membership, empty/deleted membership, failed-force recovery, concurrent replacement, portable prune/reopen/body restoration, live PR review revisions, counters, and both CLI force paths.
Built binaries were measured against the real public
pgjdbc/pgjdbcrepository on September 14, 2026, using the same isolated archive and ten open PRs with identical parent timestamps. Baseline:fda087a.--forceThe command was
sync pgjdbc/pgjdbc --include-comments --limit 10 --json, with separate progress files and request logs for each run. The remaining 100 records are PR reviews and inline review comments, deliberately still fetched. Onopenclaw/gitcrawl#198, the built CLI reduced requests from 3 to 2, skipped the comment endpoint, and reportedcomments_received: 0.All local gates passed: module tidiness, formatting, vet, vulnerability scan, deadcode, full tests and 85.5% total coverage, CLI smoke, release-script tests, docs, and snapshot packaging (Go 1.27.1, Node 26.8.2, GoReleaser 2.18.1). The CLI package exceeded the default ten-minute timeout on the loaded local Mac; the full suite passed with a thirty-minute allowance and no assertion failures. Independent reviews are clean through P2. Exact-head CI passed on
20e3148d6541da37d64bbb58b9cc0bdd0c683f76(Linux, macOS, Windows, and docs).Before implementation, the focused regression failed on the baseline for both issues and PRs:
The test-only follow-up verifies exact live membership, a third sync, and equivalent timezone/fraction timestamp representations. It passes on the implementation and rejects both tested mutants: dropping reused IDs reports
latest comment membership = []and a third-sync request count of 2 instead of 1; imposing string equality on timestamps fails both equivalent-format cases. The production source is unchanged by this follow-up.The changelog entry remains under Unreleased; no release or tag is part of this change.