fix: 7 bugs from discussion #149 smoke walk (envelope, spawn, CDC, observer)

[harshitsinghbhandari]

Summary

Fixes the 7 bugs surfaced by the discussion #149 smoke walk. RCA + suggested fixes are written up in #152; this PR implements them.

Three commits, one per investigation lane (file-disjoint, dispatched in parallel):

Commit	Bugs	Subsystem
fix(api,spawn): typed errors + project/branch/binary preflight	1, 2, 3, 4, 6	service/session, session_manager, gitworktree, all 21 agent adapters, /rollback endpoint
fix(cdc): emit pr_review_thread_resolved on replace polls	5	sqlite pr_store set-diff
fix(observe): emit scm-disabled log on startup with no subjects	7	observer.go pre-Poll credential check

Per-bug breakdown

Bug 1 — orphan terminated row + opaque 500 on unknown projectId. Service.Spawn/SpawnOrchestrator now call store.GetProject first and return apierr.NotFound("PROJECT_NOT_FOUND", ...) before manager.Spawn. No row is created when the project doesn't exist.

Bug 2 — Restore opaque 500 on half-spawned/terminated session. Manager.Restore gained the ErrIncompleteHandle guard that Manager.Kill already had. Both endpoints now return the same SESSION_INCOMPLETE_HANDLE 409.

Bug 3 — --branch unfetched or checked-out-elsewhere → opaque 500. Two new port sentinels: ErrWorkspaceBranchCheckedOutElsewhere (mapped to BRANCH_CHECKED_OUT_ELSEWHERE 409) and ErrWorkspaceBranchNotFetched (mapped to BRANCH_NOT_FETCHED 400). gitworktree pre-checks listRecords for branch-in-other-worktree and falls back to refs/tags on missing local/remote head.

Bug 4 — orphan terminated row on --claim-pr rollback. New Store.DeleteSession gated to seed-state rows only (preserves the no-resurrection guarantee for live sessions), transactional change_log cleanup, Manager.RollbackSpawn (delete-then-fallback-to-kill), new POST /sessions/{id}/rollback endpoint, CLI rewired to use it. The exit-0 sub-symptom from the smoke walk was unreproducible from current source (every traced path returns a non-nil error → os.Exit(1)) and is intentionally not addressed here — see #152 for the trace.

Bug 5 — pr_review_thread_resolved CDC event never lands. writePRRows no longer does DELETE-then-UPSERT (which made every poll hit the INSERT branch and bypass the AFTER UPDATE WHEN OLD.resolved <> NEW.resolved trigger). It now upserts observed threads (real updates fire the trigger) and set-diff-deletes orphans, all inside the existing transaction.

Bug 6 — agent binary not on PATH succeeds silently. Dropped the return "<name>", nil anti-pattern from all 21 agent adapters; on exec.LookPath miss they now return ports.ErrAgentBinaryNotFound. Manager.Spawn gained a validateAgentBinary pre-flight (with injectable LookPath for tests) that aborts before runtime.Create. Mapped to AGENT_BINARY_NOT_FOUND 400. The longer-term zellij-pane-liveness fix is a separate concern.

Bug 7 — SCM observer disabled log never emitted. checkCredentials is now called once in Observer.loop before the first Poll, independent of subject discovery. The existing credentialsChecked guard keeps it once-per-process. Daemon wiring still uses SkipTokenPreflight: true so readiness doesn't block on gh.

Cross-cutting cleanup

• toAPIError whitelist extended (no refactor into a registry — kept tight per the RCA recommendation).
• New port sentinels live in ports/ so service doesn't import adapters; adapter packages keep aliases.
• Store grew DeleteSession (seed-state gated) and DeleteSeedSession / DeleteChangeLogForSession queries.
• OpenAPI regenerated for /rollback.

Test plan

• go build ./... clean
• go vet ./... clean
• go test -race ./... — 1097 passed, 0 failed across 61 packages
• Each bug fix has at least one test that fails without the fix
• Integration tests in internal/integration/ updated to stub LookPath for the new pre-flight
• Smoke-walk repro recipes from discussion Smoke test plan: what's testable on main today #149 re-run against this branch (recommended before merge)

Notes for reviewer

1. Lane B's pr_review_threads.sql.go was hand-edited because sqlc isn't on this machine's PATH. The file matches what sqlc generate from backend/ would produce; please regenerate before merge if your environment has it, just to confirm.
2. Three commits, not squashed — each maps cleanly to one lane / one investigation and the per-bug change sets are easy to bisect.
3. Bug 6 audit touched 21 adapter files (one-line return-error change each); they're mechanical and bundled into the Lane A commit.

Closes #152
EOF

[greptile-apps]

Greptile Summary

This PR fixes 7 bugs from the #149 smoke walk across four subsystems: session spawn/lifecycle, CDC PR review threads, gitworktree branch handling, and the SCM observer credential gate. The changes span 57 files across three parallel investigation lanes.

• Session lifecycle (Bugs 1–4, 6): requireProject preflight in Service.Spawn/SpawnOrchestrator, ErrIncompleteHandle guard in Manager.Restore, typed branch/binary sentinels in ports/, validateAgentBinary pre-flight before runtime.Create, new Store.DeleteSession (seed-state gated) + Manager.RollbackSpawn + POST /sessions/{id}/rollback endpoint, and a one-liner fix to all 21 agent adapters that previously returned a bare binary name on LookPath miss.
• CDC fix (Bug 5): writePRRows Replace mode now upserts observed threads first then set-diff-deletes orphans, so unchanged threads hit ON CONFLICT DO UPDATE and the AFTER UPDATE trigger that emits pr_review_thread_resolved can fire.
• Observer fix (Bug 7): Observer.loop calls checkCredentials once before the first Poll so the "scm observer disabled" warning is emitted on a fresh daemon with no tracked sessions.

Confidence Score: 3/5

The CDC, observer, branch, binary-preflight, and project-preflight fixes are all correct and well-tested; one transaction ordering defect in DeleteSession will silently destroy CDC history for any fully-spawned session that goes through the rollback-fallback-to-kill path.

The store's DeleteSession always runs DeleteChangeLogForSession before checking whether the row is in seed state. For the primary caller (RollbackSpawn on a fully-spawned session), this unconditionally wipes the session's CDC history — session_created and session_updated events — even though the session itself is not deleted and is subsequently killed via the fallback path. This defect lives in the exact flow Bug 4 was designed to fix, and the new test does not catch it because it only checks the row-deletion gate, not whether change_log entries survive.

backend/internal/storage/sqlite/store/session_store.go — the DeleteSession transaction ordering needs to be reversed; backend/internal/storage/sqlite/gen/pr_review_threads.sql.go — hand-edited generated file should be regenerated via sqlc before merge.

Important Files Changed

Filename	Overview
backend/internal/storage/sqlite/store/session_store.go	Adds DeleteSession with a transaction that unconditionally purges CDC events before checking seed state — destroys change_log rows for live sessions when the seed-state gate is a no-op.
backend/internal/storage/sqlite/store/pr_store.go	Replaces DELETE-all+upsert with upsert-first then set-diff deletion for Replace mode, correctly letting UPDATE trigger fire for resolved thread transitions.
backend/internal/session_manager/manager.go	Adds RollbackSpawn (delete-then-kill fallback), ErrIncompleteHandle guard for Restore, and validateAgentBinary pre-flight before runtime.Create; logic is correct.
backend/internal/observe/scm/observer.go	Pre-poll checkCredentials call ensures disabled warning is emitted even when discoverSubjects returns empty; credentialsChecked guard prevents double-check on the subsequent Poll.
backend/internal/service/session/service.go	Adds requireProject pre-validation for Spawn/SpawnOrchestrator and RollbackSpawn delegation; toAPIError extended with branch and binary sentinels.
backend/internal/adapters/workspace/gitworktree/workspace.go	Adds branch-in-other-worktree preflight check and ErrBranchNotFetched sentinel with tag-ref fallback in resolveBaseRef; error types are correctly wired to ports sentinels.
backend/internal/storage/sqlite/gen/pr_review_threads.sql.go	Hand-edited generated file adds DeletePRReviewThread per-row delete; looks structurally correct but should be regenerated via sqlc before merge to eliminate drift risk.
backend/internal/httpd/controllers/sessions.go	Wires new rollback endpoint, delegates to service, returns typed RollbackSessionResponse; controller pattern matches existing kill/restore handlers.

Sequence Diagram

sequenceDiagram
    participant CLI
    participant HTTP as POST /rollback
    participant Svc as Service.RollbackSpawn
    participant Mgr as Manager.RollbackSpawn
    participant Store
    participant Kill as Manager.Kill

    CLI->>HTTP: "POST /sessions/{id}/rollback"
    HTTP->>Svc: RollbackSpawn(id)
    Svc->>Mgr: RollbackSpawn(id)
    Mgr->>Store: DeleteSession(id)
    Note over Store: tx: DeleteChangeLogForSession(id) runs unconditionally<br/>then DeleteSeedSession(id) is no-op for live sessions
    Store-->>Mgr: "deleted=false"
    Mgr->>Kill: Kill(id)
    Kill-->>Mgr: "freed=true"
    Mgr-->>Svc: "deleted=false, killed=true"
    Svc-->>HTTP: RollbackOutcome
    HTTP-->>CLI: "200 killed=true"

Loading

_{Reviews (1): Last reviewed commit: "chore: gofmt + regen frontend schema.ts ..." | Re-trigger Greptile}

[greptile-apps]

harshitsinghbhandari has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.