Skip to content

feat(scm): wire observer messenger + RepoOriginURL + persist dedup (#108)#114

Merged
harshitsinghbhandari merged 5 commits into
mainfrom
feat/issue-108
Jun 5, 2026
Merged

feat(scm): wire observer messenger + RepoOriginURL + persist dedup (#108)#114
harshitsinghbhandari merged 5 commits into
mainfrom
feat/issue-108

Conversation

@harshitsinghbhandari
Copy link
Copy Markdown
Collaborator

Summary

Closes the SCM-observer end-to-end nudge path called out in #108 by landing all three wiring fixes in one PR. Without these three pieces stacked together the observer runs and persists but never reaches the agent.

1) Thread the runtime messenger into the Lifecycle Manager

startLifecycle used to construct the LCM with a nil messenger, so every SCM-driven nudge dropped silently inside sendOnce (backend/internal/lifecycle/reactions.go). Built newSessionMessenger before startLifecycle in daemon.go and added messenger ports.AgentMessenger to the startLifecycle signature so the wired messenger reaches lifecycle.New. Closes the "Daemon constructs LCM with a real messenger (no nil at lifecycle_wiring.go)" acceptance item.

2) Populate RepoOriginURL at project add + lazy observer backfill

project.Service.Add now shells out to git -C path remote get-url origin and assigns the trimmed URL onto the row. A missing/erroring git invocation logs INFO and falls back to "" so ao project add never fails on a remoteless or non-git path. To cover already-registered projects, the SCM observer's discoverSubjects lazily backfills RepoOriginURL via the same shell-out and persists it back through UpsertProject, so subsequent polls skip the fork-exec. Implements Option A from the issue. Closes the "populates RepoOriginURL" and "backfilled lazily" acceptance items.

3) Persist reaction-dedup signatures across restart

Added migration 0005_pr_last_nudge_signature.sql (ALTER TABLE pr ADD COLUMN last_nudge_signature TEXT NOT NULL DEFAULT '') plus two scoped sqlc queries (GetPRLastNudgeSignature, UpdatePRLastNudgeSignature). lifecycle.Manager.sendOnce now loads the JSON payload ({"seen":{…},"attempts":{…}}) on first touch of each PR and persists after every successful send, so a bouncing daemon stops re-nudging the agent for still-failing CI / unresolved review threads on the first post-restart poll. Closes the "pr.last_nudge_signature column exists; sendOnce consults + updates it" acceptance item.

Test plan

  • cd backend && go build ./... — clean
  • cd backend && go test -race ./... — clean except TestSessionStreamsRealZellijPane (zellij UNIX-socket path > macOS 103-byte limit; pre-existing on main, unrelated to this PR — verified by stashing changes and re-running)
  • New unit tests:
    • TestWiring_StartLifecycleThreadsMessengerIntoLCM — wires real LCM with a captureMessenger, drives an SCM observation, asserts the messenger is invoked
    • TestManager_AddPopulatesRepoOriginURL — table-driven: git repo with origin → populated; git repo without origin → empty
    • TestDiscoverSubjects_BackfillsRepoOriginURL / TestDiscoverSubjects_NonGitPathDoesNotBackfill — lazy backfill happy path + non-git fallback
    • TestPRObservation_DedupSurvivesManagerRestart — same signature suppressed across a fresh Manager over the same store; genuinely new signature still fires
    • TestPRObservation_DedupPersistsAcrossPRs — payloads keyed per-PR, not collated
  • Validation gate: agent-ci was skipped — Docker isn't available locally and the orchestrator confirmed GitHub Actions is the authoritative pre-merge gate for this repo. Local go build + go test -race is clean.

Notes

  • sqlc isn't installed locally; the two new queries were appended to internal/storage/sqlite/gen/pr.sql.go by hand in the existing sqlc style. The existing gen.PR struct wasn't touched (no SELECT *; existing queries enumerate columns explicitly), so adding the column is non-breaking for unrelated code paths.
  • The dedup payload is opaque to storage; lifecycle owns the JSON schema. Corrupt payloads degrade to "empty" (one extra nudge) rather than crashing the lifecycle write path.

Closes #108

🤖 Generated with Claude Code

harshitsinghbhandari and others added 4 commits June 5, 2026 22:54
The daemon used to construct the LCM with a nil messenger, so every
SCM-driven nudge dropped silently inside sendOnce. Move newSessionMessenger
above startLifecycle and pass the real messenger through, so CI-failure,
review-feedback, and merge-conflict nudges actually reach the agent.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…#108)

project.Add now shells out to `git -C path remote get-url origin` and
captures the result on the new project row, so the SCM observer can parse
it on the first poll. A missing remote falls back to "" rather than failing
project add — non-git roots and remoteless repos stay registerable.

To cover projects added before this change, the observer's discoverSubjects
lazily backfills RepoOriginURL via the same shell-out and persists it
through UpsertProject, so subsequent polls skip the fork-exec.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add migration 0005 with `pr.last_nudge_signature TEXT NOT NULL DEFAULT ''`
and two scoped sqlc queries (Get/UpdatePRLastNudgeSignature). Lifecycle
serialises the per-PR slice of its seen/attempts maps to that column as a
small JSON document; sendOnce loads it lazily on first touch of each PR
and persists after every successful send.

This closes the post-restart re-nudge gap: the daemon used to lose the
seen map on bounce, so a still-failing CI re-prompted the agent on the
first post-restart observer poll even when it had already been told.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
golangci-lint's nilerr flagged the `if err := json.Unmarshal(...); err != nil { return nil }`
path in loadPRSignaturesLocked. The swallow is deliberate (a corrupt persisted
payload should not crash the lifecycle write path), so compare against nil
directly so no `err` is bound and the lint goes quiet.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 5, 2026

Greptile Summary

This PR completes the SCM-observer end-to-end nudge path by landing three wiring fixes: threading the runtime messenger into the Lifecycle Manager, populating RepoOriginURL at project registration with lazy backfill in the SCM observer, and persisting per-PR reaction-dedup signatures across daemon restarts via a new last_nudge_signature column.

  • Messenger wiring: startLifecycle now receives the pre-built messenger so lifecycle.New is constructed with a real messenger instead of nil, closing the silent-drop path in sendOnce.
  • RepoOriginURL population: project.Service.Add shells out to git remote get-url origin; the SCM observer's discoverSubjects lazily backfills existing projects with best-effort semantics.
  • Dedup persistence: A new migration adds last_nudge_signature TEXT NOT NULL DEFAULT ''; sendOnce lazily loads the per-PR JSON payload on first touch and writes it back after each successful send.

Confidence Score: 5/5

Safe to merge — all three wiring fixes are correct, the ordering invariant (WriteSCMObservation before ApplySCMObservation) is clearly maintained in the observer, and the new dedup persistence degrades gracefully under failure.

The messenger wiring, RepoOriginURL population, and dedup-persistence changes are all well-scoped and independently tested. The one non-obvious assumption — that the pr row always exists when UpdatePRLastNudgeSignature is called — is guaranteed by the observer's write-before-notify ordering and is clearly documented. No data-loss or incorrect-state paths were found.

No files require special attention; the hand-written sqlc functions in gen/pr.sql.go match the existing style and the SQL parameter ordering is correct.

Important Files Changed

Filename Overview
backend/internal/lifecycle/reactions.go Adds prURL parameter to sendOnce, lazy DB load via loadPRSignaturesLocked, and post-send persist via persistPRSignaturesLocked. The reactionKeyTargetsPR helper correctly handles HTTPS PR URLs with SplitN(key,":",2). The mutex is held throughout the DB I/O and the messenger Send call — pre-existing pattern, not a regression.
backend/internal/daemon/daemon.go Moves newSessionMessenger before startLifecycle and threads it through the signature; straightforward ordering fix, no logic changes.
backend/internal/observe/scm/observer.go Adds UpsertProject to the Store interface and backfills RepoOriginURL in discoverSubjects. The in-memory cache is per-call, so a failed upsert only affects the current tick; the next tick retries from the DB.
backend/internal/storage/sqlite/store/pr_store.go Adds GetPRLastNudgeSignature (with sql.ErrNoRows"" mapping) and UpdatePRLastNudgeSignature. The update is a plain SQL UPDATE; 0 rows affected is silently ignored, relying on the observer always having written the PR row first.
backend/internal/storage/sqlite/gen/pr.sql.go Hand-written sqlc-style functions for GetPRLastNudgeSignature and UpdatePRLastNudgeSignature. Parameter order and SQL placeholders are correct; ExecContext result is discarded, matching existing sqlc :exec style.
backend/internal/storage/sqlite/migrations/0005_pr_last_nudge_signature.sql Adds last_nudge_signature TEXT NOT NULL DEFAULT '' with goose Up/Down; the DEFAULT '' ensures existing rows are non-null, consistent with the store's empty-string semantics.

Sequence Diagram

sequenceDiagram
    participant Observer as SCM Observer
    participant Store as SQLite Store
    participant LCM as Lifecycle Manager
    participant Messenger as Session Messenger

    Observer->>Store: WriteSCMObservation(pr, checks, ...)
    Store-->>Observer: ok (pr row now exists)

    Observer->>LCM: ApplySCMObservation(sessionID, obs)
    LCM->>LCM: scmToPRObservation(obs)
    LCM->>LCM: sendOnce(prURL, key, sig, msg)

    alt First touch of prURL this process
        LCM->>Store: GetPRLastNudgeSignature(prURL)
        Store-->>LCM: empty or JSON payload
        LCM->>LCM: merge into react.seen / react.attempts
    end

    alt sig not in seen (new nudge)
        LCM->>Messenger: Send(sessionID, msg)
        Messenger-->>LCM: ok
        LCM->>LCM: update react.seen[key], react.attempts[key]
        LCM->>Store: UpdatePRLastNudgeSignature(prURL, JSON)
        Store-->>LCM: ok
    end

    LCM-->>Observer: ok
    Observer->>Store: WriteSCMObservation(finalPR, ...) [ack hashes]
Loading

Reviews (2): Last reviewed commit: "fix: silence nilerr, address reviewer no..." | Re-trigger Greptile

Comment thread backend/internal/service/project/service.go
Comment thread backend/internal/lifecycle/reactions.go
- reactions.go: discard the json.Unmarshal error explicitly via `_ =` so
  golangci-lint's nilerr stops flagging the intentional corrupt-payload
  swallow; behavior unchanged.
- reactions.go: document the Send → memory → persist order in sendOnce so
  the "one extra nudge on restart after a transient persist failure"
  trade-off is explicit (vs. the inverse risk of losing a real nudge).
- service.go: stop reaching for slog.Default() in resolveGitOriginURL;
  align with the observer's identical helper that just returns "" on git
  failure rather than logging through the global logger.
- tests: drop "issue #108" / "guards the regression from #X" framing in
  test docstrings — explain WHAT the test asserts, not the PR context.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@harshitsinghbhandari harshitsinghbhandari merged commit bfb6e98 into main Jun 5, 2026
8 checks passed
harshitsinghbhandari added a commit that referenced this pull request Jun 5, 2026
)

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

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>

* test(scm): address review — drop string key indirection, document idempotency 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>

---------

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.

SCM observer: thread messenger + populate RepoOriginURL + persist reaction dedup

1 participant