Conversation
Greptile SummaryThis PR introduces a provider-neutral SCM polling observer that owns the full lifecycle of PR/CI/review data: ETag-guarded polling, semantic hashing, transactional SQLite persistence, and lifecycle notification. It also extends the SQLite schema with 30+ new columns, a new
Confidence Score: 4/5Safe to merge with awareness that most of the complex two-phase write and hash-preservation paths have been addressed in this revision; the remaining observations are minor gaps in edge-case review cadence and log-tail attribution. The PR is a large, well-structured feature with thorough tests. Issues raised in prior review rounds — review-thread table clobbering, corrupted hashes on review-only refresh, credential gate bypass, transient-error retry, and legacy comment stranding — all appear addressed correctly in this version. The two open findings are: a log-tail fallback in scmToPRObservation that could attribute a combined multi-check log to a single failing check name (confusing agent nudge), and a narrow cadence gap where a review change on an already-approved PR could be missed until the next ETag invalidation. Neither affects data integrity or security, but both touch the agent-nudge path. backend/internal/lifecycle/reactions.go (log-tail fallback in scmToPRObservation) and backend/internal/observe/scm/observer.go (needsReviewRefresh cadence for non-ChangesRequest reviews) are the two areas worth a second look. Important Files Changed
Sequence DiagramsequenceDiagram
participant D as Daemon
participant O as Observer (30s tick)
participant P as GitHub Provider
participant S as SQLite Store
participant L as Lifecycle Manager
D->>O: Start(ctx)
loop Every 30s
O->>S: ListAllSessions / GetProject / ListPRsBySession
O->>P: SCMCredentialsAvailable (first poll only)
O->>P: RepoPRListGuard (ETag, per repo)
alt ETag changed or no prior ETag
O->>P: DetectPRByBranch (for sessions with no known PR)
O->>P: CommitChecksGuard (ETag, per PR head SHA)
O->>P: FetchPullRequests (GraphQL batch ≤25 PRs)
O->>P: FetchFailedCheckLogTail (per failed check)
end
alt Review cadence triggered
O->>P: FetchReviewThreads (2-page max, 50 threads/page)
end
O->>O: prepareForPersistence (semantic hash diff)
alt Changed.Metadata or Changed.CI or Changed.Review
O->>S: WriteSCMObservation (pending hashes)
O->>L: ApplySCMObservation → agent nudge
O->>S: WriteSCMObservation (commit real hashes)
end
O->>O: Update ETag/review caches (on success)
end
Reviews (5): Last reviewed commit: "fix: harden scm observer review follow-u..." | Re-trigger Greptile |
illegalcall
left a comment
There was a problem hiding this comment.
Detailed SCM observer migration review. I focused on behavior that can change persisted PR/CI/review state or user-visible session status.
illegalcall
left a comment
There was a problem hiding this comment.
Follow-up after a second pass: two more observer-level failure modes that look worth fixing before merge.
illegalcall
left a comment
There was a problem hiding this comment.
Follow-up on latest head b3c10b0: prior fixes address most of the earlier findings, but these two current failure modes still look worth fixing before merge.
illegalcall
left a comment
There was a problem hiding this comment.
Follow-up on latest head 481f44a: the CI pagination fix looks reasonable; one lifecycle retry durability issue remains.
|
Implementation plan for the CI/review GraphQL cost tuning discussed in this thread: CI contexts
Review threads
Persistence behavior
Correctness constraints
Tests to add/update
|
There was a problem hiding this comment.
Pull request overview
Introduces a provider-neutral SCM observer pipeline (DTOs + polling observer + GitHub provider implementation), persists normalized SCM state (PR metadata, CI checks, review threads/comments, semantic hashes) in SQLite, and wires the observer into the daemon when GitHub credentials are available. This implements the Issue #75 direction of moving the observer→lifecycle contract from provider-specific data / legacy PRObservation to a new ports.SCMObservation plus ApplySCMObservation.
Changes:
- Add provider-neutral SCM DTOs (
ports.SCMObservationet al.) and a new lifecycle entrypoint (ApplySCMObservation) that projects into the existing PR reaction path. - Implement the SCM polling observer (ETag guards, batching, review refresh cadence, semantic hashing, persistence + lifecycle notification ordering).
- Extend SQLite schema/queries/mappings to persist SCM metadata, CI check detail, review threads, bot filtering, and observation timestamps; wire GitHub-backed observer into daemon startup.
Reviewed changes
Copilot reviewed 24 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/internal/storage/sqlite/store/store_test.go | Adds store tests validating WriteSCMObservation persistence semantics (replace/merge/preserve) and legacy WritePR compatibility. |
| backend/internal/storage/sqlite/store/pr_store.go | Implements SCMWriter via WriteSCMObservation, adds review thread persistence + merge/replace modes, and expands PR/check/comment mappings. |
| backend/internal/storage/sqlite/queries/pr.sql | Expands PR upsert/select to include SCM metadata fields + semantic hashes/timestamps; excludes bot comments from display review count. |
| backend/internal/storage/sqlite/queries/pr_review_threads.sql | New sqlc queries for upserting/listing/deleting normalized PR review threads. |
| backend/internal/storage/sqlite/queries/pr_comment.sql | Extends PR comments schema interactions (thread_id/url/is_bot) and adds delete-by-thread for merge refresh. |
| backend/internal/storage/sqlite/queries/pr_checks.sql | Extends PR checks persistence with conclusion/details fields. |
| backend/internal/storage/sqlite/migrations/0003_scm_observer_schema.sql | Adds SCM observer columns/tables (PR metadata, hashes, timestamps, check details, comment bot/thread info, pr_review_threads). |
| backend/internal/storage/sqlite/gen/projects.sql.go | Tightens ArchiveProject update to be idempotent (only archive once). |
| backend/internal/storage/sqlite/gen/pr.sql.go | Regenerates sqlc PR query bindings for expanded PR schema + bot filtering in display facts. |
| backend/internal/storage/sqlite/gen/pr_review_threads.sql.go | New generated sqlc bindings for review thread queries. |
| backend/internal/storage/sqlite/gen/pr_comment.sql.go | Regenerates sqlc bindings for extended PR comment schema + delete-by-thread. |
| backend/internal/storage/sqlite/gen/pr_checks.sql.go | Regenerates sqlc bindings for extended PR check schema. |
| backend/internal/storage/sqlite/gen/models.go | Extends generated models for new PR/check/comment/review-thread fields. |
| backend/internal/session_manager/manager.go | Minor allocation improvement when collecting cleaned session IDs. |
| backend/internal/ports/scm_observations.go | Adds provider-neutral SCM DTOs used across provider/observer/store/lifecycle boundaries. |
| backend/internal/ports/outbound.go | Adds ReviewWriteMode and SCMWriter port for transactional SCM persistence semantics. |
| backend/internal/observe/scm/observer.go | New SCM observer orchestrator (polling loop, guards, batching, review refresh, semantic hashing, persistence + lifecycle notification). |
| backend/internal/observe/scm/observer_test.go | Adds orchestration tests for observer behavior (ETags, batching, log tails, review cadence, lifecycle/hash acknowledgement). |
| backend/internal/lifecycle/reactions.go | Adds ApplySCMObservation that projects SCM observations into legacy PRObservation reactions; minor slice preallocation. |
| backend/internal/lifecycle/manager_test.go | Adds test asserting SCM observation projection triggers existing CI nudge behavior. |
| backend/internal/domain/pr.go | Extends domain PR/check/comment models and introduces normalized review-thread model + semantic hashes/timestamps. |
| backend/internal/daemon/scm_wiring.go | Wires SCM observer into daemon with GitHub provider + lazy token preflight avoidance. |
| backend/internal/daemon/lifecycle_wiring.go | Ensures daemon stop waits for SCM observer goroutine as well as reaper. |
| backend/internal/daemon/daemon.go | Starts SCM observer during daemon boot via lifecycle stack. |
| backend/internal/adapters/scm/github/provider.go | Adds lazy credential preflight option + SCMCredentialsAvailable; adjusts mergeability blocking for draft/review-required. |
| backend/internal/adapters/scm/github/provider_test.go | Adds tests covering SCM check normalization, mergeability blockers, GraphQL batching fallback, and review thread window behavior. |
| backend/internal/adapters/scm/github/observer_provider.go | New GitHub implementation of the provider-neutral observer Provider contract (guards, detection, batching, log tail, review pagination). |
| backend/internal/adapters/scm/github/doc.go | Updates package docs to reflect SCM observer usage and cache ownership split. |
| backend/internal/adapters/scm/github/client.go | Adds doRESTWithETag for observer-owned ETag handling and helper for ETag header fallback. |
| backend/internal/adapters/scm/github/auth.go | Adds FallbackTokenSource for env/gh token lookup and invalidation forwarding. |
Files not reviewed (6)
- backend/internal/storage/sqlite/gen/models.go: Language not supported
- backend/internal/storage/sqlite/gen/pr.sql.go: Language not supported
- backend/internal/storage/sqlite/gen/pr_checks.sql.go: Language not supported
- backend/internal/storage/sqlite/gen/pr_comment.sql.go: Language not supported
- backend/internal/storage/sqlite/gen/pr_review_threads.sql.go: Language not supported
- backend/internal/storage/sqlite/gen/projects.sql.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
illegalcall
left a comment
There was a problem hiding this comment.
Follow-up on latest head e84d4ee: the previous durability and pagination comments are addressed; I found one new CDC side effect in the acknowledgement write.
harshitsinghbhandari
left a comment
There was a problem hiding this comment.
High-recall review pass against feat/75 at 6302844. 10 findings, ranked by severity — correctness first, then durability/efficiency, then altitude/cleanup. Most prior @illegalcall + @greptile threads look correctly addressed; these are what's left after deduping against resolved threads.
Correctness
1. backend/internal/lifecycle/reactions.go:116 — failed-check dedup signature degenerates when CI.HeadSHA is empty
scmToPRObservation copies obs.CI.HeadSHA into every PRCheckObservation.CommitHash (reactions.go:116). On a review-only refresh that fetches threads but no fresh CI batch, the observation flows back through ApplyPRObservation with CommitHash="". ApplyPRObservation then builds sig = ch.CommitHash + ":" + ch.LogTail (reactions.go:52). If both are empty/stable the signature collapses to a constant for that (key, PR, check-name); a later genuinely-new failing CI on a different commit silently no-ops in sendOnce because react.seen[key] == sig already. The agent never gets the new failure nudge.
2. backend/internal/storage/sqlite/queries/pr_comment.sql:5 — InsertLegacyPRComment is INSERT OR IGNORE, silently dropping updates
Pre-PR the legacy WritePR path replaced the full comment set per PR (DeletePRComments → InsertPRComment). The new InsertLegacyPRComment uses INSERT OR IGNORE and writePR no longer deletes legacy comments first (pr_store.go:74-77 gates delete on ReviewWriteReplace). If a reviewer edits the body of an existing comment and the legacy path runs (e.g. through service/pr.Manager.ObservePR, today only wired by integration/lifecycle_sqlite_test.go:97), the updated body is dropped and the stale body persists. The fix was likely targeted at not clobbering observer-written rows, but it changed the semantics for the legacy path too.
Durability / efficiency
3. backend/internal/observe/scm/observer.go:340-374 — two-phase "lifecycle is the cursor" is only durable within one daemon process
Poll writes the row with preserved (old) hashes (line 358), calls lifecycle.ApplySCMObservation (line 364), then writes again with final hashes (line 369). reactions.sendOnce dedups in an in-memory react.seen/attempts map (reactions.go:172-194). If the daemon crashes after ApplySCMObservation returns nil but before the second write commits, the in-memory dedup is lost; on the next start, the same observation re-fetches (hashes unchanged in DB), lifecycle re-fires, and the agent receives a duplicate nudge. The design comment at observer.go:340-345 claims "lifecycle is the cursor" — but the cursor is durable only across polls within the same process. A durable outbox row (or persisting react.seen/attempts in the DB) would close this.
4. backend/internal/adapters/scm/github/observer_provider.go:168 — FetchReviewThreads always fetches a second 50-thread page when the newest thread is unresolved
After fetching the latest 50 threads, the code issues a second GraphQL page whenever len(latest) == 0 || !latest[0].Resolved (observer_provider.go:168). For any actively-reviewed PR the newest thread is typically unresolved, so the 2-minute review cadence fetches 100 threads + their leading 5 comments every cycle, not 50. The intent (recover older unresolved threads) only fires when the newest happens to be unresolved — which is the wrong signal, since older threads need a revisit regardless of the newest one's state. Cost: ~2× GraphQL points per review poll across all open PRs.
5. backend/internal/observe/scm/observer.go:339, 357 — domainFromObservation is called twice per changed PR; each call recomputes three SHA256 hashes
When lifecycle is wired, domainFromObservation runs once with the pending-hash options (line 338) and again with the final-hash options (line 357). Each call invokes metadataSemanticHash, ciSemanticHash, and reviewSemanticHash (observer.go:765-783). prepareForPersistence (line 740) also computes the same three hashes. For 50 changed PRs/poll that's ~450 hash invocations where ~150 suffice. Compute the three hash pairs once in prepareForPersistence and thread them through.
6. backend/internal/observe/scm/observer.go:626 — enrichFailureLogs does O(M×N) match between FailedChecks and all Checks
For each failed check (~M) the inner loop walks all checks (~N) to copy LogTail back into obs.CI.Checks. A PR with 80 checks and 10 failing is 800 string comparisons per CI refresh. Pre-build map[checkKey]int once, then update obs.CI.Checks[idx].LogTail directly. applyStoredFailedLogTails (line 635-665) already uses the cleaner name → tail map shape — same pattern is applicable here.
Altitude / cleanup
7. backend/internal/storage/sqlite/store/pr_store.go:50 — writePR mixes legacy and SCM-observer modes via a bool + enum; legacy WritePR has no production caller
writePR carries replaceLegacyComments bool plus ReviewWriteMode and branches on the combination (lines 69-101). Searching the tree, service/pr.Manager (the only legacy WritePR consumer) is wired only by integration/lifecycle_sqlite_test.go:97 — no daemon path uses it. Deleting service/pr.Manager, InsertLegacyPRComment, genLegacyCommentParams, and the bool collapses writePR to just WriteSCMObservation's body and incidentally resolves finding #2.
8. backend/internal/observe/scm/observer.go:1128-1166 — three near-identical FIFO eviction helpers
cacheSetString, cacheSetTime, cacheSetBool, and evictStrings duplicate the same "append-to-order-slice; evict-when-over-cap" pattern, parameterized only by value type. A generic cacheSet[V any](m map[string]V, order *[]string, key string, value V, max int) collapses to one helper. Any future fix (proper LRU touch, deterministic eviction, TTL) currently has to land in four places.
9. backend/internal/observe/scm/observer.go:716-718 — ReviewRefreshFailed reset only flips value to false; reviewFailedOrder grows monotonically
On a successful review refresh, Cache.ReviewRefreshFailed[pkey] = false is set but the key is never removed from reviewFailedOrder (line 1146-1155 evicts by slice length). Over a long-lived daemon, eviction begins discarding genuinely-failed entries earlier than the configured cap implies, because cleared false entries still hold slots. Fix: delete(map, pkey) and remove from the order slice on success.
10. backend/internal/domain/pr.go:51-53 — provider-specific enum strings leak into the provider-neutral domain
domain.PullRequest carries ProviderState, ProviderMergeable, ProviderMergeStateStatus — raw GitHub strings — alongside the normalized AO enums. mergeabilityFromProviderFacts (observer.go:950) inspects GitHub-flavored values like DIRTY/BEHIND/UNSTABLE. A second provider (Linear/GitLab) repeats this contamination. A provider-opaque ProviderPayload []byte (or a JSON column) plus moving the provider→AO normalization fully into the provider package keeps the domain layer purely normalized.
Verified refuted (not findings, for transparency)
observer_provider.go:406scmChecksFromGraphQLdoesn't nil-panic — Go nil-map reads return zero values, and the loop overnodes(nil["nodes"])runs zero iterations.reactions.go:122-138doesn't drop human review comments via theIsBotskip —scmThreadFromGraphQL(observer_provider.go:555-570) only setsthread.IsBot=truewhen every comment is bot-authored.0004_scm_observer_schema.sqlchange_logdrop+recreate is safe — goose migrations run in a transaction; no concurrent write window exists.cdc.EventTypewidening (pr_review_thread_added/_resolved) is safe — no exhaustive switch overEventTypeexists in the tree.
|
@harshitsinghbhandari thanks for the high-recall pass. I pushed
Local validation run after the patch:
|
|
Let's merge this PR @whoisasx |
Closes #75
Summary
add provider-neutral ports.SCMObservation DTOs and lifecycle ApplySCMObservation projection
Tests