From 1f486b85f0568b761b25318ff9b3fcf73c9f8b27 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 14 Sep 2026 12:37:07 -0700 Subject: [PATCH 1/2] fix(sync): avoid downloading unchanged issue comments --- CHANGELOG.md | 2 + docs/commands.md | 2 +- docs/refresh-and-embed.md | 2 +- docs/sync.md | 21 ++ internal/cli/comment_reuse_test.go | 66 +++++++ internal/cli/help.go | 8 +- internal/cli/refresh.go | 2 + internal/cli/sync.go | 4 + internal/store/comment_reuse.go | 76 ++++++++ internal/syncer/comment_reuse_test.go | 263 ++++++++++++++++++++++++++ internal/syncer/syncer.go | 71 +++++-- 11 files changed, 502 insertions(+), 15 deletions(-) create mode 100644 internal/cli/comment_reuse_test.go create mode 100644 internal/store/comment_reuse.go create mode 100644 internal/syncer/comment_reuse_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index d03f6a9d..8045b203 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Skip unchanged issue-comment downloads on issues and PRs using parent timestamps, comment counts, and completed saved observations; keep PR review and detail data live, and add `sync`/`refresh --force` for a full selected refresh. Thanks @vlsi for the report. + ## 0.10.0 - 2026-09-13 **Highlights:** Explicit cloud archive admission (thanks @vincentkoc), `last_export_at` for portable publication time, and sync that no longer loses completed items when one fails. diff --git a/docs/commands.md b/docs/commands.md index 143aa9da..a11c7ef0 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -42,7 +42,7 @@ These work on every command. | Command | Purpose | Docs | | --- | --- | --- | -| `gitcrawl sync owner/repo [--state --since --numbers --limit --include-comments --include-pr-details --with pr-details --progress-file --json]` | Sync issues and PRs from GitHub into local SQLite | [Sync](/sync/) | +| `gitcrawl sync owner/repo [--state --since --numbers --limit --include-comments --include-pr-details --with pr-details --force --progress-file --json]` | Sync issues and PRs from GitHub into local SQLite | [Sync](/sync/) | | `gitcrawl sync-failures owner/repo [--include-resolved --limit N --json]` | List failed issue, comment, and PR hydration attempts and optional resolved history | [Sync](/sync/#hydration-depth) | | `gitcrawl coverage [owner/repo \| --repos owner/a,owner/b] [--min-missing-pr-details N --json]` | Report archive, PR-detail, and enrichment coverage/freshness | — | | `gitcrawl fill-pr-details owner/repo [--limit --order --batch-size --reserve-rate-limit --include-comments --json-progress --json]` | Hydrate locally missing pull request detail rows in bounded batches | — | diff --git a/docs/refresh-and-embed.md b/docs/refresh-and-embed.md index e5caa3c7..0a5a4e9f 100644 --- a/docs/refresh-and-embed.md +++ b/docs/refresh-and-embed.md @@ -33,7 +33,7 @@ Disable any stage with `--no-sync`, `--no-embed`, `--no-cluster`. The remaining | Forwarded to | Flag | | --- | --- | -| sync | `--since`, `--state`, `--limit`, `--include-comments`, `--include-pr-details`, `--with pr-details` | +| sync | `--since`, `--state`, `--limit`, `--include-comments`, `--include-pr-details`, `--with pr-details`, `--force` (bypass unchanged issue-comment reuse) | | embed | `--limit` | | cluster | `--threshold` (0.80), `--min-size` (1), `--max-cluster-size` (40), `--k` (16), `--cross-kind-threshold` (0.93), `--strict-vectors` | diff --git a/docs/sync.md b/docs/sync.md index 42c1e58a..f8215441 100644 --- a/docs/sync.md +++ b/docs/sync.md @@ -83,6 +83,7 @@ issue or pull request URLs. | `--with pr-metadata` | PR object, including merge attribution, head/base references, and diff counts | | `--include-pr-details` | PR object, files, commits, status checks, workflow runs, review threads | | `--with pr-details` | Same as `--include-pr-details` (gh-style flag) | +| `--force` | Download selected data again, bypassing unchanged issue-comment reuse | | `--progress-file ` | Atomically publish sanitized machine-readable activity | `pr-metadata` writes only `pull_request_details`, using the normal per-thread @@ -96,6 +97,26 @@ child collections. Full PR details also populate `pull_request_files`, `pull_request_commits`, `pull_request_checks`, and `github_workflow_runs` for local review and search. +With `--include-comments`, sync reuses issue comments (including the general +discussion on PRs) when the freshly fetched parent `updated_at` matches the last +completed comment observation and its `comments` count matches the exact saved +issue-comment membership. Missing or pruned payloads, observations, membership, timestamps or counts, +changed timestamps or counts, and unresolved issue-comment failures require a +download. Historical comments omitted from the last observation are never reused. +Reused comments remain in the archive and revision evidence; they do not count +toward `comments_synced` or progress `comments_received`. The saved observation +is checked again in the item transaction; concurrent replacement requires a retry. + +PR reviews, inline review comments, review-thread resolution, and all selected +PR details are still fetched on every sync: a parent timestamp or head SHA alone +does not prove those collections are unchanged. This preserves check and workflow +transitions in fully hydrated revision evidence. + +To refresh issue comments that may have changed without moving either parent +signal, use `gitcrawl sync owner/repo --include-comments --force` (or the same +flags with `refresh`). `--force` bypasses reuse within the selected state, time +window, or `--numbers`; it does not expand that scope or select extra families. + `fill-pr-details` selects PRs without a metadata row; it does not upgrade existing metadata-only rows. To upgrade them, use `gitcrawl sync owner/repo --numbers 123,456 --with pr-details` diff --git a/internal/cli/comment_reuse_test.go b/internal/cli/comment_reuse_test.go new file mode 100644 index 00000000..a6371f77 --- /dev/null +++ b/internal/cli/comment_reuse_test.go @@ -0,0 +1,66 @@ +package cli + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "testing" +) + +func TestSyncAndRefreshForceCommentDownload(t *testing.T) { + for _, command := range []string{"sync", "refresh"} { + t.Run(command, func(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + configPath, dbPath := filepath.Join(dir, "config.toml"), filepath.Join(dir, "archive.db") + if err := New().Run(ctx, []string{"--config", configPath, "init", "--db", dbPath}); err != nil { + t.Fatal(err) + } + calls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/repos/fixture/repo": + _ = json.NewEncoder(w).Encode(map[string]any{"id": 123}) + case "/repos/fixture/repo/issues": + row := githubIssueJSON(1, "issue", "Discussion") + row["comments"] = 1 + _ = json.NewEncoder(w).Encode([]map[string]any{row}) + case "/repos/fixture/repo/issues/1/comments": + calls++ + _ = json.NewEncoder(w).Encode([]map[string]any{{"id": 11, "body": "saved discussion"}}) + default: + t.Errorf("unexpected request: %s", r.URL.Path) + http.Error(w, "unexpected", http.StatusBadRequest) + } + })) + defer server.Close() + t.Setenv("GITHUB_TOKEN", "test-gh-token") + t.Setenv("GITCRAWL_GITHUB_BASE_URL", server.URL) + args := []string{"--config", configPath, command, "fixture/repo", "--include-comments", "--limit", "1", "--json"} + if command == "refresh" { + args = append(args, "--no-embed", "--no-cluster") + } + for run, wantCalls := range []int{1, 1, 2} { + var stdout bytes.Buffer + app := New() + app.Stdout = &stdout + if run == 2 { + args = append(args, "--force") + } + if err := app.Run(ctx, args); err != nil { + t.Fatal(err) + } + if calls != wantCalls { + t.Fatalf("run=%d calls=%d want=%d", run, calls, wantCalls) + } + if run == 1 && !strings.Contains(stdout.String(), `"comments_synced": 0`) { + t.Fatalf("reused comments counted as downloaded: %s", stdout.String()) + } + } + }) + } +} diff --git a/internal/cli/help.go b/internal/cli/help.go index d54b7006..6e2841a8 100644 --- a/internal/cli/help.go +++ b/internal/cli/help.go @@ -142,10 +142,12 @@ Usage: "sync": `gitcrawl sync mirrors GitHub issue and pull request metadata. Usage: - gitcrawl sync owner/repo [--state open|closed|all] [--numbers refs] [--with pr-metadata|pr-details] [--include-pr-details] [--json] + gitcrawl sync owner/repo [--state open|closed|all] [--numbers refs] [--with pr-metadata|pr-details] [--include-comments] [--include-pr-details] [--force] [--json] pr-metadata fetches only the pull request object; pr-details also hydrates files, commits, checks, workflows, and review threads. Comments are selected separately. +Unchanged issue comments are reused; --force downloads them again. PR reviews +and PR details are always fetched when selected. `, "sync-failures": `gitcrawl sync-failures lists failed sync hydration attempts. @@ -172,7 +174,9 @@ choose a different floor. "refresh": `gitcrawl refresh runs sync, enrichment, embedding, and clustering. Usage: - gitcrawl refresh owner/repo [--state open|closed|all] [--with pr-metadata|pr-details] [--include-pr-details] [--no-sync] [--no-embed] [--no-cluster] [--strict-vectors] [--json] + gitcrawl refresh owner/repo [--state open|closed|all] [--with pr-metadata|pr-details] [--include-pr-details] [--force] [--no-sync] [--no-embed] [--no-cluster] [--strict-vectors] [--json] + +--force bypasses unchanged issue-comment reuse during sync. `, "summarize": `gitcrawl summarize generates key summaries for current thread revisions. diff --git a/internal/cli/refresh.go b/internal/cli/refresh.go index 38ed4b6f..74bf546a 100644 --- a/internal/cli/refresh.go +++ b/internal/cli/refresh.go @@ -28,6 +28,7 @@ func (a *App) runRefresh(ctx context.Context, args []string) error { noEmbed := fs.Bool("no-embed", false, "skip embedding stage") noCluster := fs.Bool("no-cluster", false, "skip clustering stage") includeComments := fs.Bool("include-comments", false, "hydrate comments during sync") + force := fs.Bool("force", false, "download selected sync data even when issue comments are unchanged") includePRDetails := fs.Bool("include-pr-details", false, "hydrate PR files, commits, checks, workflow runs, and review threads") withRaw := fs.String("with", "", "additional sync hydration: pr-metadata, pr-details") fs.Bool("include-code", false, "accepted for compatibility; code hydration is not implemented yet") @@ -97,6 +98,7 @@ func (a *App) runRefresh(ctx context.Context, args []string) error { State: strings.TrimSpace(*state), Limit: limit, IncludeComments: *includeComments, + Force: *force, IncludePRMetadata: with["pr-metadata"], IncludePRDetails: *includePRDetails || with["pr-details"], }) diff --git a/internal/cli/sync.go b/internal/cli/sync.go index fef5348b..aae408a0 100644 --- a/internal/cli/sync.go +++ b/internal/cli/sync.go @@ -26,6 +26,7 @@ func (a *App) runSync(ctx context.Context, args []string) error { limitRaw := fs.String("limit", "", "maximum issue/PR rows") jsonOut := fs.Bool("json", false, "write JSON output") includeComments := fs.Bool("include-comments", false, "hydrate issue comments, PR reviews, and PR review comments") + force := fs.Bool("force", false, "download selected data even when issue comments are unchanged") includePRDetails := fs.Bool("include-pr-details", false, "hydrate PR files, commits, checks, and workflow runs") withRaw := fs.String("with", "", "extra hydration: pr-metadata, pr-details") progressFile := fs.String("progress-file", "", "write an atomic sanitized sync progress snapshot") @@ -72,6 +73,7 @@ func (a *App) runSync(ctx context.Context, args []string) error { Limit: limit, Numbers: numbers, IncludeComments: *includeComments, + Force: *force, IncludePRMetadata: with["pr-metadata"], IncludePRDetails: *includePRDetails || with["pr-details"], Progress: progress.report, @@ -97,6 +99,7 @@ type syncOptions struct { Limit int Numbers []int IncludeComments bool + Force bool IncludePRMetadata bool IncludePRDetails bool Quiet bool @@ -429,6 +432,7 @@ func (a *App) syncRepository(ctx context.Context, owner, repo string, options sy Limit: options.Limit, Numbers: options.Numbers, IncludeComments: options.IncludeComments, + Force: options.Force, IncludePRMetadata: options.IncludePRMetadata, IncludePRDetails: options.IncludePRDetails, Reporter: reporter, diff --git a/internal/store/comment_reuse.go b/internal/store/comment_reuse.go new file mode 100644 index 00000000..b4217bc9 --- /dev/null +++ b/internal/store/comment_reuse.go @@ -0,0 +1,76 @@ +package store + +import ( + "context" + "database/sql" + "fmt" + "time" +) + +// IssueCommentReuse identifies the exact saved discussion validated by a fresh +// parent timestamp and comment count. PR reviews are deliberately excluded. +type IssueCommentReuse struct { + ThreadID int64 + ObservationSequence int64 + CommentIDs []int64 +} + +func (s *Store) ReusableIssueComments(ctx context.Context, repoID int64, number int, updatedAt string, count int) (*IssueCommentReuse, error) { + updated, err := time.Parse(time.RFC3339Nano, updatedAt) + if err != nil || count < 0 { + return nil, nil + } + var snapshot IssueCommentReuse + var source string + err = s.q().QueryRowContext(ctx, ` + select t.id, r.source_updated_at, r.observation_sequence + from threads t + join thread_child_observation_reservations r on r.thread_id = t.id and r.family = 'comments' + where t.repo_id = ? and t.number = ? + and not exists ( + select 1 from sync_attempt_failures f + where f.repo_id = t.repo_id and f.number = t.number + and f.operation = 'issue_comments' and f.resolved_at is null + ) + `, repoID, number).Scan(&snapshot.ThreadID, &source, &snapshot.ObservationSequence) + if err == sql.ErrNoRows { + return nil, nil + } + if err != nil { + 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 { + return nil, nil + } + memberIDs, found, err := s.ThreadChildObservationMemberIDs(ctx, snapshot.ThreadID, ThreadChildComments, snapshot.ObservationSequence) + if err != nil || !found { + return nil, err + } + comments, err := s.ListComments(ctx, snapshot.ThreadID) + if err != nil { + return nil, err + } + byID := make(map[int64]Comment, len(comments)) + for _, comment := range comments { + byID[comment.ID] = comment + } + for _, id := range memberIDs { + comment, found := byID[id] + if !found { + return nil, nil + } + if comment.CommentType == "issue_comment" { + // Portable pruning keeps observation membership but strips raw payloads + // and may truncate bodies. Fetch again before claiming full evidence. + if comment.RawJSON == "" || comment.Body == "" { + return nil, nil + } + snapshot.CommentIDs = append(snapshot.CommentIDs, id) + } + } + if len(snapshot.CommentIDs) != count { + return nil, nil + } + return &snapshot, nil +} diff --git a/internal/syncer/comment_reuse_test.go b/internal/syncer/comment_reuse_test.go new file mode 100644 index 00000000..d17ce6d2 --- /dev/null +++ b/internal/syncer/comment_reuse_test.go @@ -0,0 +1,263 @@ +package syncer + +import ( + "context" + "errors" + "path/filepath" + "strings" + "testing" + + gh "github.com/openclaw/gitcrawl/internal/github" + "github.com/openclaw/gitcrawl/internal/store" +) + +type commentReuseGitHub struct { + fakeGitHub + row map[string]any + comments []map[string]any + commentCalls int + reviewCalls int + reviews []map[string]any + commentErr error +} + +func (f *commentReuseGitHub) GetIssue(context.Context, string, string, int, gh.Reporter) (map[string]any, error) { + return f.row, nil +} + +func (f *commentReuseGitHub) ListIssueComments(context.Context, string, string, int, gh.Reporter) ([]map[string]any, error) { + f.commentCalls++ + return f.comments, f.commentErr +} + +func (f *commentReuseGitHub) ListPullReviews(context.Context, string, string, int, gh.Reporter) ([]map[string]any, error) { + f.reviewCalls++ + return f.reviews, nil +} + +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"} { + t.Run(scenario, func(t *testing.T) { + ctx := context.Background() + st, err := store.Open(ctx, filepath.Join(t.TempDir(), "archive.db")) + if err != nil { + t.Fatal(err) + } + defer st.Close() + row, _ := (fakeGitHub{}).GetIssue(ctx, "openclaw", "gitcrawl", 7, nil) + comments, _ := (fakeGitHub{}).ListIssueComments(ctx, "openclaw", "gitcrawl", 7, nil) + row["comments"] = 1 + client := &commentReuseGitHub{row: row, comments: comments} + s := New(client, st) + opts := Options{Owner: "openclaw", Repo: "gitcrawl", Numbers: []int{7}, IncludeComments: scenario != "metadata-only"} + if _, err := s.Sync(ctx, opts); err != nil { + t.Fatal(err) + } + opts.IncludeComments = true + switch scenario { + case "timestamp": + row["updated_at"] = "2026-04-27T00:00:00Z" + case "count": + row["comments"] = 2 + case "missing-count": + delete(row, "comments") + case "malformed-time": + row["updated_at"] = "invalid" + case "force": + opts.Force = true + case "pruned": + if _, err := st.PrunePortablePayloads(ctx, store.PortablePruneOptions{BodyChars: 4}); err != nil { + t.Fatal(err) + } + if err := st.Close(); err != nil { + t.Fatal(err) + } + st, err = store.Open(ctx, st.Path()) + if err != nil { + t.Fatal(err) + } + defer st.Close() + s = New(client, st) + case "missing-membership": + if _, err := st.DB().ExecContext(ctx, "delete from thread_child_observation_memberships"); err != nil { + t.Fatal(err) + } + case "missing-member": + if _, err := st.DB().ExecContext(ctx, "update comments set deleted_at = '2026-04-27T00:00:00Z', deletion_reason = 'fixture'"); err != nil { + t.Fatal(err) + } + case "failed-force": + opts.Force = true + client.commentErr = errors.New("fixture download failure") + if _, err := s.Sync(ctx, opts); err == nil { + t.Fatal("failed download succeeded") + } + opts.Force = false + client.commentErr = nil + } + before := client.commentCalls + if _, err := s.Sync(ctx, opts); err != nil { + t.Fatal(err) + } + if client.commentCalls != before+1 { + t.Fatal("invalidated comments were reused") + } + if scenario == "pruned" { + var body string + if err := st.DB().QueryRowContext(ctx, "select body from comments where github_id='11'").Scan(&body); err != nil { + t.Fatal(err) + } + if body != "same bug here" { + t.Fatalf("pruned comment not restored: %q", body) + } + } + }) + } +} + +func TestSyncCommentReuseEmptyAndDeletedMembership(t *testing.T) { + ctx := context.Background() + st, err := store.Open(ctx, filepath.Join(t.TempDir(), "archive.db")) + if err != nil { + t.Fatal(err) + } + defer st.Close() + row, _ := (fakeGitHub{}).GetIssue(ctx, "openclaw", "gitcrawl", 7, nil) + comments, _ := (fakeGitHub{}).ListIssueComments(ctx, "openclaw", "gitcrawl", 7, nil) + row["comments"] = 1 + client := &commentReuseGitHub{row: row, comments: comments} + s := New(client, st) + opts := Options{Owner: "openclaw", Repo: "gitcrawl", Numbers: []int{7}, IncludeComments: true} + if _, err := s.Sync(ctx, opts); err != nil { + t.Fatal(err) + } + row["comments"] = 0 + client.comments = nil + if _, err := s.Sync(ctx, opts); err != nil { + t.Fatal(err) + } + if _, err := s.Sync(ctx, opts); err != nil { + t.Fatal(err) + } + if client.commentCalls != 2 { + t.Fatalf("empty completed snapshot not reused: %d", client.commentCalls) + } + var encoded string + if err := st.DB().QueryRowContext(ctx, `select m.member_ids_json from thread_child_observation_memberships m join thread_child_observation_reservations r using(thread_id,family,observation_sequence) where r.family='comments'`).Scan(&encoded); err != nil { + t.Fatal(err) + } + if encoded != "[]" { + t.Fatalf("reuse resurrected historical comments: %s", encoded) + } +} + +func TestSyncCommentReuseKeepsPRReviewEvidenceLive(t *testing.T) { + ctx := context.Background() + st, err := store.Open(ctx, filepath.Join(t.TempDir(), "archive.db")) + if err != nil { + t.Fatal(err) + } + defer st.Close() + row, _ := (fakeGitHub{}).GetIssue(ctx, "openclaw", "gitcrawl", 8, nil) + comments, _ := (fakeGitHub{}).ListIssueComments(ctx, "openclaw", "gitcrawl", 7, nil) + row["comments"] = 1 + client := &commentReuseGitHub{row: row, comments: comments} + s := New(client, st) + opts := Options{Owner: "openclaw", Repo: "gitcrawl", Numbers: []int{8}, IncludeComments: true, IncludePRDetails: true} + if _, err := s.Sync(ctx, opts); err != nil { + t.Fatal(err) + } + client.reviews = []map[string]any{{"id": 123, "state": "CHANGES_REQUESTED", "body": "please fix"}} + stats, err := s.Sync(ctx, opts) + if err != nil { + t.Fatal(err) + } + if client.commentCalls != 1 || client.reviewCalls != 2 || stats.CommentsSynced != 1 || stats.RevisionsCreated != 1 || stats.PRDetailsSynced != 1 { + t.Fatalf("fresh PR evidence lost with reused discussion: calls=%d reviews=%d stats=%+v", client.commentCalls, client.reviewCalls, stats) + } +} + +func TestSyncCommentReuseRejectsConcurrentReplacement(t *testing.T) { + ctx := context.Background() + st, err := store.Open(ctx, filepath.Join(t.TempDir(), "archive.db")) + if err != nil { + t.Fatal(err) + } + defer st.Close() + row, _ := (fakeGitHub{}).GetIssue(ctx, "openclaw", "gitcrawl", 7, nil) + comments, _ := (fakeGitHub{}).ListIssueComments(ctx, "openclaw", "gitcrawl", 7, nil) + row["comments"] = 1 + client := &commentReuseGitHub{row: row, comments: comments} + s := New(client, st) + opts := Options{Owner: "openclaw", Repo: "gitcrawl", Numbers: []int{7}, IncludeComments: true} + if _, err := s.Sync(ctx, opts); err != nil { + t.Fatal(err) + } + s.beforePersist = func() { + client.comments[0]["body"] = "new concurrent body" + forced := opts + forced.Force = true + if _, err := New(client, st).Sync(ctx, forced); err != nil { + t.Fatal(err) + } + } + stats, err := s.Sync(ctx, opts) + if err == nil || !strings.Contains(err.Error(), "saved issue comments changed") { + t.Fatalf("err=%v", err) + } + if stats.EvidenceObserved != 0 || stats.ThreadsSynced != 0 { + t.Fatalf("certified stale cached evidence: %+v", stats) + } + var body string + if err := st.DB().QueryRowContext(ctx, "select body from comments where github_id='11'").Scan(&body); err != nil { + t.Fatal(err) + } + if body != "new concurrent body" { + t.Fatalf("concurrent comment overwritten: %s", body) + } +} + +func TestSyncReusesUnchangedIssueComments(t *testing.T) { + for _, number := range []int{7, 8} { + t.Run(map[int]string{7: "issue", 8: "pull_request"}[number], func(t *testing.T) { + ctx := context.Background() + st, err := store.Open(ctx, filepath.Join(t.TempDir(), "archive.db")) + if err != nil { + t.Fatal(err) + } + defer st.Close() + row, _ := (fakeGitHub{}).GetIssue(ctx, "openclaw", "gitcrawl", number, nil) + comments, _ := (fakeGitHub{}).ListIssueComments(ctx, "openclaw", "gitcrawl", 7, nil) + row["comments"] = len(comments) + client := &commentReuseGitHub{row: row, comments: comments} + s := New(client, st) + var received SyncProgress + opts := Options{Owner: "openclaw", Repo: "gitcrawl", Numbers: []int{number}, IncludeComments: true, + Progress: func(p SyncProgress) error { received = p; return nil }} + first, err := s.Sync(ctx, opts) + if err != nil { + t.Fatal(err) + } + if first.CommentsSynced != 1 { + t.Fatalf("first: %+v", first) + } + second, err := s.Sync(ctx, opts) + if err != nil { + t.Fatal(err) + } + if client.commentCalls != 1 || second.CommentsSynced != 0 || received.CommentsReceived != 0 { + t.Fatalf("unchanged thread re-downloaded comments: calls=%d synced=%d received=%d", client.commentCalls, second.CommentsSynced, received.CommentsReceived) + } + if number == 8 && client.reviewCalls != 2 { + t.Fatal("PR reviews must remain live") + } + var count int + if err := st.DB().QueryRowContext(ctx, "select count(*) from comments where body = 'same bug here'").Scan(&count); err != nil { + t.Fatal(err) + } + if count != 1 { + t.Fatalf("reuse lost archived comments: %d", count) + } + }) + } +} diff --git a/internal/syncer/syncer.go b/internal/syncer/syncer.go index c605bda7..ee978385 100644 --- a/internal/syncer/syncer.go +++ b/internal/syncer/syncer.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "log/slog" + "slices" "strconv" "strings" "time" @@ -49,6 +50,7 @@ type Options struct { Limit int Numbers []int IncludeComments bool + Force bool IncludePRMetadata bool IncludePRDetails bool Reporter gh.Reporter @@ -103,6 +105,7 @@ type syncPersistStats struct { type threadSyncPayload struct { row map[string]any commentRows []commentRow + commentReuse *store.IssueCommentReuse reviewThreads []map[string]any reviewThreadsFetchedAt string pullDetails pullRequestDetailRows @@ -137,8 +140,10 @@ func (s *Syncer) Sync(ctx context.Context, options Options) (Stats, error) { if err != nil { return Stats{}, err } + var repoID int64 if err := s.store.WithTx(ctx, func(st *store.Store) error { - repoID, err := s.upsertRepository(ctx, st, options, repoRaw) + var err error + repoID, err = s.upsertRepository(ctx, st, options, repoRaw) if err != nil { return err } @@ -239,7 +244,17 @@ func (s *Syncer) Sync(ctx context.Context, options Options) (Stats, error) { continue } if options.IncludeComments { - commentRows, operation, err := s.fetchCommentRows(ctx, options, kind, number) + if !options.Force { + err = s.store.WithTx(ctx, func(st *store.Store) error { + var err error + payload.commentReuse, err = reusableIssueComments(ctx, st, repoID, row) + return err + }) + if err != nil { + return Stats{}, err + } + } + commentRows, operation, err := s.fetchCommentRows(ctx, options, kind, number, payload.commentReuse) if err != nil { if err := recordFailure(number, row, err, operation); err != nil { return Stats{}, err @@ -247,7 +262,11 @@ func (s *Syncer) Sync(ctx context.Context, options Options) (Stats, error) { continue } payload.commentRows = commentRows - received.CommentsReceived += len(commentRows) + for _, comment := range commentRows { + if comment.reusedID == 0 { + received.CommentsReceived++ + } + } } if options.IncludePRDetails && kind == "pull_request" { reviewThreads, reviewThreadsFetchedAt, err := s.fetchPullReviewThreadRows(ctx, options, number) @@ -346,6 +365,17 @@ func (s *Syncer) Sync(ctx context.Context, options Options) (Stats, error) { if err != nil { return err } + if payload.commentReuse != nil { + current, err := reusableIssueComments(ctx, st, repoID, payload.row) + if err != nil { + return err + } + if current == nil || current.ThreadID != payload.commentReuse.ThreadID || + current.ObservationSequence != payload.commentReuse.ObservationSequence || + !slices.Equal(current.CommentIDs, payload.commentReuse.CommentIDs) { + return fmt.Errorf("saved issue comments changed during sync; retry #%d", intValue(payload.row["number"])) + } + } thread := mapIssueToThread(repoID, payload.row, s.now().Format(time.RFC3339Nano)) _, hasIssueDraft := payload.row["draft"] if payload.hasPullDetails { @@ -979,14 +1009,28 @@ func persistThreadEnrichment( return st.UpsertThreadRevisionAndFingerprint(ctx, evidence, createdAt) } -func (s *Syncer) fetchCommentRows(ctx context.Context, options Options, threadKind string, number int) ([]commentRow, string, error) { - var rows []commentRow - issueComments, err := s.client.ListIssueComments(ctx, options.Owner, options.Repo, number, options.Reporter) +func reusableIssueComments(ctx context.Context, st *store.Store, repoID int64, row map[string]any) (*store.IssueCommentReuse, error) { + count, err := strconv.Atoi(fmt.Sprint(row["comments"])) if err != nil { - return nil, "issue_comments", err + return nil, nil } - for _, row := range issueComments { - rows = append(rows, commentRow{kind: "issue_comment", raw: row}) + return st.ReusableIssueComments(ctx, repoID, intValue(row["number"]), stringValue(row["updated_at"]), count) +} + +func (s *Syncer) fetchCommentRows(ctx context.Context, options Options, threadKind string, number int, reuse *store.IssueCommentReuse) ([]commentRow, string, error) { + var rows []commentRow + if reuse != nil { + for _, id := range reuse.CommentIDs { + rows = append(rows, commentRow{kind: "issue_comment", reusedID: id}) + } + } else { + issueComments, err := s.client.ListIssueComments(ctx, options.Owner, options.Repo, number, options.Reporter) + if err != nil { + return nil, "issue_comments", err + } + for _, row := range issueComments { + rows = append(rows, commentRow{kind: "issue_comment", raw: row}) + } } if threadKind == "pull_request" { reviews, err := s.client.ListPullReviews(ctx, options.Owner, options.Repo, number, options.Reporter) @@ -1017,6 +1061,10 @@ func persistComments( synced := 0 observedIDs := make([]int64, 0, len(rows)) for _, row := range rows { + if row.reusedID != 0 { + observedIDs = append(observedIDs, row.reusedID) + continue + } comment := mapComment(thread.ID, row.kind, row.raw) if comment.Body == "" && row.kind != "pull_review" && comment.DeletedAt == "" { continue @@ -1108,8 +1156,9 @@ func mapPullReviewThread(threadID int64, row map[string]any, fetchedAt string) s } type commentRow struct { - kind string - raw map[string]any + kind string + raw map[string]any + reusedID int64 } func mapComment(threadID int64, kind string, row map[string]any) store.Comment { From 20e3148d6541da37d64bbb58b9cc0bdd0c683f76 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Mon, 14 Sep 2026 12:59:33 -0700 Subject: [PATCH 2/2] test(sync): retain comment reuse across repeated syncs 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> --- internal/syncer/comment_reuse_test.go | 65 ++++++++++++++++++++------- 1 file changed, 50 insertions(+), 15 deletions(-) diff --git a/internal/syncer/comment_reuse_test.go b/internal/syncer/comment_reuse_test.go index d17ce6d2..8f44469e 100644 --- a/internal/syncer/comment_reuse_test.go +++ b/internal/syncer/comment_reuse_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "path/filepath" + "slices" "strings" "testing" @@ -100,7 +101,7 @@ func TestSyncCommentReuseInvalidation(t *testing.T) { t.Fatal(err) } if client.commentCalls != before+1 { - t.Fatal("invalidated comments were reused") + t.Fatalf("ListIssueComments calls = %d, want %d after invalidation", client.commentCalls, before+1) } if scenario == "pruned" { var body string @@ -172,8 +173,16 @@ func TestSyncCommentReuseKeepsPRReviewEvidenceLive(t *testing.T) { if err != nil { t.Fatal(err) } - if client.commentCalls != 1 || client.reviewCalls != 2 || stats.CommentsSynced != 1 || stats.RevisionsCreated != 1 || stats.PRDetailsSynced != 1 { - t.Fatalf("fresh PR evidence lost with reused discussion: calls=%d reviews=%d stats=%+v", client.commentCalls, client.reviewCalls, stats) + for name, result := range map[string]struct{ got, want int }{ + "issue comment requests": {client.commentCalls, 1}, + "review requests": {client.reviewCalls, 2}, + "comments synced": {stats.CommentsSynced, 1}, + "revisions created": {stats.RevisionsCreated, 1}, + "PR details synced": {stats.PRDetailsSynced, 1}, + } { + if result.got != result.want { + t.Errorf("%s = %d, want %d", name, result.got, result.want) + } } } @@ -203,7 +212,7 @@ func TestSyncCommentReuseRejectsConcurrentReplacement(t *testing.T) { } stats, err := s.Sync(ctx, opts) if err == nil || !strings.Contains(err.Error(), "saved issue comments changed") { - t.Fatalf("err=%v", err) + t.Fatalf("Sync error = %v, want saved-comment replacement error", err) } if stats.EvidenceObserved != 0 || stats.ThreadsSynced != 0 { t.Fatalf("certified stale cached evidence: %+v", stats) @@ -218,29 +227,40 @@ func TestSyncCommentReuseRejectsConcurrentReplacement(t *testing.T) { } func TestSyncReusesUnchangedIssueComments(t *testing.T) { - for _, number := range []int{7, 8} { - t.Run(map[int]string{7: "issue", 8: "pull_request"}[number], func(t *testing.T) { + for _, test := range []struct { + name string + number int + updatedAt string + wantReviews int + }{ + {"issue_discussion_remains_reusable", 7, "2026-04-26T00:00:00Z", 0}, + {"PR_discussion_remains_reusable", 8, "2026-04-26T00:00:00Z", 2}, + {"equivalent_timezone_reuses_comments", 7, "2026-04-26T00:00:00+00:00", 0}, + {"equivalent_fraction_reuses_comments", 7, "2026-04-26T00:00:00.000Z", 0}, + } { + t.Run(test.name, func(t *testing.T) { ctx := context.Background() st, err := store.Open(ctx, filepath.Join(t.TempDir(), "archive.db")) if err != nil { t.Fatal(err) } defer st.Close() - row, _ := (fakeGitHub{}).GetIssue(ctx, "openclaw", "gitcrawl", number, nil) + row, _ := (fakeGitHub{}).GetIssue(ctx, "openclaw", "gitcrawl", test.number, nil) comments, _ := (fakeGitHub{}).ListIssueComments(ctx, "openclaw", "gitcrawl", 7, nil) row["comments"] = len(comments) client := &commentReuseGitHub{row: row, comments: comments} s := New(client, st) var received SyncProgress - opts := Options{Owner: "openclaw", Repo: "gitcrawl", Numbers: []int{number}, IncludeComments: true, + opts := Options{Owner: "openclaw", Repo: "gitcrawl", Numbers: []int{test.number}, IncludeComments: true, Progress: func(p SyncProgress) error { received = p; return nil }} first, err := s.Sync(ctx, opts) if err != nil { t.Fatal(err) } if first.CommentsSynced != 1 { - t.Fatalf("first: %+v", first) + t.Fatalf("initial CommentsSynced = %d, want 1", first.CommentsSynced) } + row["updated_at"] = test.updatedAt second, err := s.Sync(ctx, opts) if err != nil { t.Fatal(err) @@ -248,15 +268,30 @@ func TestSyncReusesUnchangedIssueComments(t *testing.T) { if client.commentCalls != 1 || second.CommentsSynced != 0 || received.CommentsReceived != 0 { t.Fatalf("unchanged thread re-downloaded comments: calls=%d synced=%d received=%d", client.commentCalls, second.CommentsSynced, received.CommentsReceived) } - if number == 8 && client.reviewCalls != 2 { - t.Fatal("PR reviews must remain live") + if client.reviewCalls != test.wantReviews { + t.Errorf("review requests after second sync = %d, want %d", client.reviewCalls, test.wantReviews) + } + var threadID, commentID int64 + var body, deletedAt string + if err := st.DB().QueryRowContext(ctx, "select thread_id, id, body, coalesce(deleted_at, '') from comments where github_id='11'").Scan(&threadID, &commentID, &body, &deletedAt); err != nil { + t.Fatal(err) + } + if body != "same bug here" || deletedAt != "" { + t.Errorf("saved comment = (%q, deleted_at=%q), want original live comment", body, deletedAt) } - var count int - if err := st.DB().QueryRowContext(ctx, "select count(*) from comments where body = 'same bug here'").Scan(&count); err != nil { + _, sequence, found, err := st.ThreadChildObservation(ctx, threadID, store.ThreadChildComments) + if err != nil || !found { + t.Fatalf("completed comment observation found=%t, err=%v; want present", found, err) + } + members, found, err := st.ThreadChildObservationMemberIDs(ctx, threadID, store.ThreadChildComments, sequence) + if err != nil || !found || !slices.Equal(members, []int64{commentID}) { + t.Errorf("latest comment membership = %v, found=%t, err=%v; want [%d]", members, found, err, commentID) + } + if _, err := s.Sync(ctx, opts); err != nil { t.Fatal(err) } - if count != 1 { - t.Fatalf("reuse lost archived comments: %d", count) + if client.commentCalls != 1 { + t.Errorf("ListIssueComments calls after third sync = %d, want 1", client.commentCalls) } }) }