Skip to content

test(scm): end-to-end integration coverage for SCM observer (#109)#115

Merged
harshitsinghbhandari merged 2 commits into
mainfrom
test/109-scm-e2e
Jun 5, 2026
Merged

test(scm): end-to-end integration coverage for SCM observer (#109)#115
harshitsinghbhandari merged 2 commits into
mainfrom
test/109-scm-e2e

Conversation

@harshitsinghbhandari
Copy link
Copy Markdown
Collaborator

Summary

Adds the SCM observer end-to-end integration test from issue #109. Drives a real scmobserve.Observer.Poll against a real sqlite.Store, a real lifecycle.Manager wired with a recording messenger spy, and a canned observe/scm.Provider, asserting the full observation → reducer → store → messenger pipeline the daemon runs after the wiring landed in #114. New file is test-only: backend/internal/integration/scm_observer_test.go.

This is the regression guard for #108 — without it, a future change that re-introduces a nil messenger in daemon/lifecycle_wiring.go, drops the RepoOriginURL plumbing, or breaks the persisted dedup signature would land silently. Existing unit tests in observe/scm/observer_test.go and storage/sqlite/store/store_test.go cover each layer in isolation; nothing previously wired them together against a live SQLite + a live LCM.

Cases

Three table-driven subtests, each on its own tmpdir DB fixture so writes/messenger/lifecycle state never leak between them:

  • CI-failing observation (acceptance items 1–4): the pr row reflects the observation (provider-neutral provider/host/repo columns, head_sha, semantic metadata_hash/ci_hash); pr_checks mirrors CI.Checks (name, status, commit hash, log tail); exactly one nudge reaches the messenger spy, addressed to the right session, body containing both the "CI is failing" cue and the failed-log tail. pr.last_nudge_signature is persisted, and a second identical Poll produces zero new nudges and leaves the signature unchanged (idempotency).
  • Merged: true observation (acceptance item 5): the session row is MarkTerminated'd via the lifecycle reducer's merged branch, and no nudge is sent.
  • Branch with no open PR (acceptance item 6): DetectPRByBranch returns ErrSCMNotFound, no pr row is written, and no nudge is sent.

Per the spec, the test drives Poll(ctx) directly with Config{Tick: time.Hour} rather than relying on the ticker.

Test plan

  • cd backend && go build ./... — clean.
  • cd backend && go test -race -run TestSCMObserverEndToEnd ./internal/integration/ — 3/3 pass.
  • cd backend && go test -race ./... — full suite. One pre-existing environmental failure in internal/terminal (TestSessionStreamsRealZellijPane: zellij IPC socket path exceeds 103 bytes under macOS's /var/folders/... TMPDIR) is unrelated to this change and will pass under Linux CI.
  • Remote GitHub CI (authoritative gate).

🤖 Generated with Claude Code

Adds backend/internal/integration/scm_observer_test.go, the regression
guard for the SCM observer wiring landed in PR #114. Drives
scmobserve.Observer.Poll against a real sqlite.Store, a real
lifecycle.Manager with a recording messenger spy, and a canned
observe/scm.Provider, asserting the full observation -> reducer ->
store -> messenger pipeline.

Three table-driven subtests, each on its own tmpdir fixture:

- A CI-failing observation persists the pr row (provider-neutral
  columns + semantic hashes), persists pr_checks mirroring the
  observation, delivers exactly one nudge with the failed-log tail,
  persists last_nudge_signature, and produces no additional nudge on
  an identical re-poll.
- A Merged: true observation MarkTerminated's the session and sends
  no nudge.
- A branch with no open PR writes nothing and sends no nudge.

Closes #109

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@harshitsinghbhandari harshitsinghbhandari added this to the rewrite milestone Jun 5, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 5, 2026

Greptile Summary

This PR adds a single end-to-end integration test file (backend/internal/integration/scm_observer_test.go) that wires a real SQLite store, a real lifecycle manager with a messenger spy, and a canned SCM provider into the real SCM observer, then drives Observer.Poll directly to assert the full observation → reducer → store → messenger pipeline.

  • Three table-driven sub-tests cover: CI-failing observation with dedup idempotency, merged-PR termination, and branch-with-no-PR no-op — each in an isolated tmpdir DB fixture.
  • Tracing through the production code confirms all assertions are reachable and correct: the CI-failing nudge, the hash-match short-circuit on re-poll, MarkTerminated via the merged branch, and ErrSCMNotFound quiescing gracefully without writing any rows.

Confidence Score: 5/5

Safe to merge — test-only addition with no production code changes and no risk of regressions.

The change adds a single test file that exercises an existing wiring path. Tracing through the production code confirms every assertion is reachable: the hash-match short-circuit correctly prevents a second nudge on re-poll, MarkTerminated fires through the merged branch, and ErrSCMNotFound quiesces cleanly. Each sub-test uses its own isolated tmpdir DB so no state leaks between cases. No production logic is modified.

No files require special attention.

Important Files Changed

Filename Overview
backend/internal/integration/scm_observer_test.go New end-to-end integration test covering three SCM observer scenarios; wiring, assertions, and idempotency logic are all correct.

Sequence Diagram

sequenceDiagram
    participant T as Test
    participant O as Observer.Poll
    participant P as cannedSCMProvider
    participant DB as sqlite.Store
    participant LCM as lifecycle.Manager
    participant Spy as messengerSpy

    T->>O: Poll(ctx)
    O->>DB: ListAllSessions
    DB-->>O: [session]
    O->>P: DetectPRByBranch(repo, branch)
    P-->>O: SCMPRObservation
    O->>P: FetchPullRequests
    P-->>O: SCMObservation
    O->>DB: WriteSCMObservation(pendingPR)
    O->>LCM: ApplySCMObservation
    LCM->>Spy: Send nudge
    LCM->>DB: UpdatePRLastNudgeSignature
    O->>DB: WriteSCMObservation(finalPR)
    O-->>T: nil
Loading

Reviews (2): Last reviewed commit: "test(scm): address review — drop string ..." | Re-trigger Greptile

Comment thread backend/internal/integration/scm_observer_test.go
Comment thread backend/internal/integration/scm_observer_test.go
…mpotency path

- Key cannedSCMProvider.observations/reviews by PR number directly so
  the fake no longer carries a string key that resembled (but did not
  actually need to mirror) the observer's internal prKey. Every case
  in this test uses scmTestRepo, so number alone is unambiguous.
- Add an explicit pointer in the CI-failing subtest noting it
  exercises the hash-match short-circuit in prepareForPersistence;
  the ETag-driven 304 short-circuit on the same SHA is covered by
  observe/scm/observer_test.go (Poll_RepoETag304SkipsDetectPR,
  Poll_CIETagChangeRefreshesWhenRepoUnchanged).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@harshitsinghbhandari harshitsinghbhandari merged commit 378addf into main Jun 5, 2026
8 checks passed
harshitsinghbhandari added a commit that referenced this pull request Jun 7, 2026
Move the observer-pattern-general pieces of the SCM observer into a new
backend/internal/observe package so the tracker observer (issue #35) can
build on the same primitives:

- StartPollLoop: goroutine supervisor with immediate-first-poll + ticker
  + ctx-done exit. SCM Observer.Start now delegates to it.
- CheckCredentialsOnce: lazy first-poll credential gate driven by a
  CredentialProbe closure. SCM observer keeps credentialsChecked/disabled
  as Observer fields; the shared helper mutates them via pointer so
  state ownership stays single-source.
- CacheSet[V any] / CacheDelete[V any]: one generic bounded-FIFO helper
  replaces the three near-identical cacheSet{String,Time,Bool} bodies
  and the standalone evictStrings. The SCM-side methods are now
  one-line wrappers that thread o.Cache.max into the shared helper, so
  existing call sites and tests are untouched.

SCM behavior is unchanged. The full 21-test SCM suite (including the
end-to-end test added in PR #115) plus 577 backend tests stay green
under `go test -race`.

Part of #112.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
harshitsinghbhandari added a commit that referenced this pull request Jun 7, 2026
…112) (#116)

* refactor(observe): extract shared observer skeleton

Move the observer-pattern-general pieces of the SCM observer into a new
backend/internal/observe package so the tracker observer (issue #35) can
build on the same primitives:

- StartPollLoop: goroutine supervisor with immediate-first-poll + ticker
  + ctx-done exit. SCM Observer.Start now delegates to it.
- CheckCredentialsOnce: lazy first-poll credential gate driven by a
  CredentialProbe closure. SCM observer keeps credentialsChecked/disabled
  as Observer fields; the shared helper mutates them via pointer so
  state ownership stays single-source.
- CacheSet[V any] / CacheDelete[V any]: one generic bounded-FIFO helper
  replaces the three near-identical cacheSet{String,Time,Bool} bodies
  and the standalone evictStrings. The SCM-side methods are now
  one-line wrappers that thread o.Cache.max into the shared helper, so
  existing call sites and tests are untouched.

SCM behavior is unchanged. The full 21-test SCM suite (including the
end-to-end test added in PR #115) plus 577 backend tests stay green
under `go test -race`.

Part of #112.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* feat(tracker): ports.TrackerObservation DTO + ApplyTrackerFacts reducer

Land the contract that the future Tracker observer (issue #35) and its
provider adapters must satisfy. No observer is wired in this PR — the
DTO + reducer are the deliverable, and locking the shape now lets the
observer + adapter work happen in small follow-up PRs.

DTO (backend/internal/ports/tracker_observations.go):
- TrackerObservation mirrors ports.SCMObservation: Fetched bool,
  ObservedAt time.Time, Provider/Host/Repo, normalized Issue facts,
  Comments, and a Changed{State, Assignee, Comments} discriminator.
- TrackerIssueObservation carries the minimal facts lifecycle needs
  today (state, assignee, title, body, timestamps); richer
  per-provider metadata stays inside each adapter.
- TrackerCommentObservation carries the comment fields needed for the
  bot-mention nudge (Author, Body, IsBot, ID for dedup).

Reducer (backend/internal/lifecycle/reactions.go):
- ApplyTrackerFacts(ctx, sessionID, ports.TrackerObservation) error,
  mirroring ApplySCMObservation's "Fetched gate → terminal-state →
  per-bucket reactions" shape.
- Three initial reactions:
    * Issue state == done | cancelled → MarkTerminated (idempotent).
    * Changed.Assignee → log only via slog.Default(). The "assignee
      changed away from AO" policy is reserved for #40.
    * Changed.Comments with bot comments → one-time nudge with
      strings.Join'd bot bodies, deduped by comment IDs.
- The nudge path reuses sendOnce with an empty prURL so the in-memory
  dedup applies but the PR-row persistence path is skipped. Tracker
  signature persistence will land with #35 alongside issue-row storage.

Tests in backend/internal/lifecycle/manager_test.go cover each branch:
terminate (done + cancelled), log-only assignee, nudge fires on new
bot comment, nudge suppressed on repeat, new bot comment id refires,
not-fetched is no-op, terminated session ignores observations.

Part of #112.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(observe): rename CacheSet param to avoid shadowing built-in max

golangci-lint revive flagged the CacheSet generic helper's max
parameter as shadowing the built-in max() function. Rename to
maxEntries; signature change is internal to the observe package and
the SCM observer's one-line wrappers pass the value positionally, so
no call sites need updating.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix: honour disabled state in CheckCredentialsOnce + tighten bot-comment filter

Two P1 review findings on #116:

1. observe.CheckCredentialsOnce was returning (true, nil) on every
   call after the gate ran, even when the probe had marked the
   observer disabled, because the *checked short-circuit ignored
   *disabled. The SCM observer didn't surface this in practice — its
   Poll method has an independent `if o.disabled { return nil }`
   guard that runs first — but a future Tracker observer that relies
   on the helper's documented contract ("Observer stays disabled")
   would silently flip back to "credentials available" after the
   first poll. Change the short-circuit to `return !*disabled, nil`
   and lock the behavior with a regression test that issues repeat
   calls after the probe reported unavailable.

2. lifecycle.newBotCommentContent's "skip uninteresting comments"
   filter used && where it needed ||. A bot comment with an empty ID
   but a non-empty body slipped through and appended "" to the ids
   slice. If every bot comment in the observation had an empty ID,
   strings.Join produced "" — which matches the zero value of the
   in-memory dedup map, so sendOnce treated the nudge as
   already-sent and silently suppressed it forever. Switch to || so
   any comment missing either an ID or a body is dropped, and add a
   regression test that an empty-ID bot comment never nudges (and
   does not pollute the dedup state for a follow-up comment that has
   a real ID).

586 tests pass with -race.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* test(observe): capture deadline once in poll-error spin-wait

The `TestStartPollLoop_LogsPollErrorWithoutPanic` spin-wait was
computing the loop bound as `time.Now().Before(time.Now().Add(200ms))`
on every iteration, which is permanently true — the loop could only
exit via the `break`. Under a scheduler delay (heavy CI load or
`GOMAXPROCS=1`) where two polls never land in time, the test would
hang until the wall-clock kill rather than failing fast.

Capture the deadline once before the loop, and tighten the assertion
to actually require two polls + done-channel closure within a bounded
window, matching `TestStartPollLoop_FirstPollImmediateThenTicks`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <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