feat(lifecycle): ACT layer — reaction table + escalation engine (split B)

[harshitsinghbhandari]

Summary

Implements split B of the Lifecycle Manager: the ACT layer that turns persisted status transitions into reactions and drives escalation. Builds on split A's synchronous Apply* pipeline (load→decide→diff→persist).

• Reaction table (reactions.go) — the full distillation §4.2 default: ci-failed (send, retries 2, persistent), changes-requested/bugbot-comments (send, 30m), merge-conflicts (send, 15m), agent-idle (send, retries 2 + 15m), approved-and-green (notify — human decides to merge), agent-stuck/agent-needs-input/agent-exited (notify urgent), pr-closed (notify action), all-complete (notify info). auto-merge action kind exists but off by default (no row uses it). bugbot-comments/merge-conflicts are configured but dormant — split-A's decide core has no producer for them yet.
• reactionEventFor — maps a canonical record → reaction key, mirroring DeriveLegacyStatus. Closed-PR is detected off the PR axis (it derives to idle).
• Escalation engine — in-memory reactionTracker{attempts, escalated, firstAttemptAt} keyed by (sessionID, reactionKey). Numeric cap or duration → emit reaction.escalated + notify + silence further auto-dispatch. Send failures don't burn budget. Trackers clear on leaving the state, except ci-failed (persistent across fail→pending→fail; resets only when the PR leaves open / session terminal).
• Real TickEscalations(now) — fires duration escalations the synchronous LCM can't wake itself for; never writes canonical state.
• Wiring (manager.go, additive) — mutate returns a *transition; each Apply* path fires the mapped reaction after persist via the single synchronous react() chokepoint (the seam that keeps the async move localized later). OnKillRequested intentionally does not react (explicit kill ≠ inferred event). Split-A behavior unchanged.

Design decisions (agreed with @HARSHIT)

• Synchronous dispatch through one react() chokepoint — the reaper/daemon that would own a worker goroutine is out of this split; making dispatch async later is a one-function change.
• In-memory trackers — restart resets budgets (harmless: extra agent retries, never fewer human notifications); keeps ACT policy out of the canonical store.

Test plan

• gofmt -l . clean
• go build ./...
• go vet ./...
• go test -race ./...
• Covers: right reaction per transition, numeric escalation (2 sends + 1 escalation), duration escalation via TickEscalations, non-persistent tracker clearing, pr-closed/merged, agent-exited on inferred death, explicit kill fires nothing.

🤖 Generated with Claude Code

[harshitsinghbhandari]

Review — ACT layer (split B), at 8b8da8e

Verified independently: gofmt clean, build/vet pass, go test -race green at 89.2% coverage. The reaction table matches §4.2, the wiring is clean (CI log tail threaded into the SCM reaction; OnKillRequested correctly discards its transition so an explicit kill fires nothing; react() runs outside the per-session lock so a busy-waiting send-to-agent doesn't hold the session mutex), and the synchronous-single-chokepoint design is well-documented. Two findings.

1. Persistent ci-failed tracker can leak / never clear (fix before merge)

react() only clears the tracker for beforeKey, and for a persistent reaction only when incidentOver(afterLC) holds at the moment it leaves that reaction. So the clear never happens if the session leaves ci-failed to another open-PR state first:

ci-failed → approved → merged
  └ leaving ci-failed: incidentOver(approved)=false (PR still open) → NOT cleared
                        leaving approved: only the approved tracker is considered
  ⇒ the ci-failed tracker is never cleared → leaks

Two consequences:

• Memory leak: one orphaned reactionTracker per session that had a ci-failed then recovered before merging.
• Stale silencing: if such a tracker already hit escalated=true, a later regression approved → ci-failed re-enters sendToAgent, sees escalated, and is silenced — the agent is never re-nudged and no fresh budget is granted, even though the prior incident effectively ended.

Suggested fix: when a transition reaches incidentOver(afterLC) (PR no longer open / session terminal), clear all trackers for that session, not just beforeKey. That also forces a decision worth making explicit: does a genuine recovery to approved/green count as the incident ending (re-arm the budget), or only a merge/close? The persistent flag was for fail→pending→fail flap — confirm it shouldn't also reset on a real recovery.

2. react() runs outside the per-session lock → unserialized dispatch (integration-time)

Persists are serialized under keyedMutex, but react() fires after the lock releases, so for a live daemon with concurrent observers (SCM poller + reaper + activity ingest) reactions for one session can interleave / run on a stale afterLC snapshot — e.g. a ci-failed send-to-agent dispatching after the session already moved to approved. Not observable in the current single-threaded tests, and the out-of-lock placement is the right call to avoid holding the mutex during a busy-wait. Flag for the integration step: either re-verify the session is still in the triggering state before dispatching, or give react() a per-session ordering (a small react queue). Worth a code comment now so it isn't forgotten.

Minor

• Notify reactions have no cross-re-entry dedup: an approved → ci-failed → approved oscillation re-fires "ready to merge" each time it re-enters approved. Probably fine, but noisy under flap.
• agent-stuck's §4.2 threshold: 10m isn't implemented (notify fires immediately on entering stuck). Likely redundant given the detecting→stuck debounce already gates it — fine to skip, just noting the divergence.

Recommendation: Approve once #1 is addressed (it's a real bug in the escalation engine, the core of this PR); #2 as a tracked integration-time item; minors optional. Nice work on the chokepoint design and the lock-free TickEscalations.

[harshitsinghbhandari]

Thanks for the careful review — pushed b2161d5.

#1 (the real bug) — fixed. You're right, and the trace was spot-on. My original clear was keyed on beforeKey + incidentOver-at-leave-time, which never fires on ci-failed → approved → merged (leaving ci-failed to an open state, then the merge transition only considers the approved tracker). I restructured so clearing is keyed on the state reached, not the one left:

• react() now clears all of a session's trackers when afterLC is either incidentOver (PR merged/closed or session terminal) or recovered (approved/mergeable). This kills the leak and the stale escalated=true silencing.
• On your explicit question — yes, a genuine recovery re-arms the budget. recovered = approved/merge_ready, which the open-PR ladder guarantees means CI is no longer failing (it ranks ci_failing above approved, so the two can't co-display). A later regression approved → ci-failed now gets a fresh budget and re-nudges the agent.
• The persistent flag still does its §4.2 job: it survives only the ambiguous review_pending limbo (where CISummary=pending is indistinguishable from CI-passed-awaiting-review at the canonical level), so fail→pending→fail keeps one shared budget. It no longer survives a real recovery.
• New tests: TestReaction_CIFailedRearmsOnGenuineRecovery (regression after recovery re-nudges, not silenced) and TestReaction_IncidentOverClearsAllSessionTrackers (no tracker survives a merge).

#2 (out-of-lock dispatch) — documented as a tracked integration item. Added a comment on react(): under a live daemon with concurrent observers the afterLC snapshot can be stale by dispatch time; when the daemon lands, either re-check the triggering state before dispatching or give react() a per-session react queue. Kept the dispatch outside the lock as you noted (so a busy-waiting send never holds the mutex).

Minors:

• agent-stuck threshold: 10m — added a comment explaining it's intentionally not gated (entry into stuck is already debounced upstream by the detecting→stuck quarantine, so a second timer is redundant).
• notify re-entry dedup on approved → ci-failed → approved flap — left as-is per your call; noting it's bounded by how often a PR realistically flaps green↔red.

Gates green after the fix: gofmt clean, build, vet, go test -race ./....

[harshitsinghbhandari]

Re-review — b2161d5

Verified independently: gofmt clean, build/vet pass, go test -race green at 89.3% coverage. Finding #1 is correctly resolved, finding #2 documented.

#1 — persistent-tracker lifecycle ✅ The clear is now keyed on the state reached (incidentOver || recovered) and clears all of the session's trackers there, with recovered scoped to unambiguous green (approved/merge_ready) — which, since the open-PR ladder ranks ci_failing above approval, can't coexist with failing CI. This is the right call and resolves the open question well:

• ci-failed → approved → merged no longer leaks the ci-failed tracker (cleared on reaching approved).
• a regression approved → ci-failed re-arms with a fresh budget instead of staying silenced.
• the fail → pending → fail flap still drains one budget — review_pending/pr_open are neither incident-over nor recovered, so the persistent tracker survives the limbo.
• covered by TestReaction_CIFailedRearmsOnGenuineRecovery and TestReaction_IncidentOverClearsAllSessionTrackers.

#2 — out-of-lock dispatch ✅ Documented as an integration-time caveat on react() with the concrete options (per-session react queue / re-check before dispatch). Good enough until the daemon runs concurrent observers.

agent-stuck threshold — documented why it's intentionally skipped (upstream detecting→stuck debounce). Agreed.

LGTM — approving. Clean turnaround.

[Copilot]

Pull request overview

Implements the Lifecycle Manager ACT layer, adding reaction dispatch and escalation behavior after persisted lifecycle transitions.

Changes:

• Adds the default reaction table, reaction mapping, tracker-based escalation, and TickEscalations.
• Wires Apply* paths to return persisted transitions and invoke react() after mutation.
• Adds reaction-focused tests and replaces the prior no-op escalation test.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
backend/internal/lifecycle/reactions.go	Adds reaction definitions, dispatch, escalation tracking, and duration tick handling.
backend/internal/lifecycle/reactions_test.go	Adds tests for reaction mapping, escalation, tracker clearing, and tick behavior.
backend/internal/lifecycle/manager.go	Wires lifecycle mutations to post-persist reactions and initializes tracker state.
backend/internal/lifecycle/manager_test.go	Removes the obsolete no-op TickEscalations test.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[harshitsinghbhandari]

Re-review — 5081cf7 (Copilot fixes)

Verified the delta over the LGTM'd b2161d5; gofmt clean, build/vet pass, go test -race green at 89.3%. All five Copilot fixes are sound:

• OnKillRequested clears the session's trackers — correct; a kill is terminal but fires no reaction, so without this a leftover tracker could later emit reaction.escalated for a dead session via TickEscalations. Covered by TestReaction_KillClearsEscalationTrackers.
• Send-error attempt rollback — decrements attempts (and resets firstAttemptAt only when this call set it, via freshFirst) under trackerMu, so an undelivered message doesn't march toward escalation (§4.3). Covered by TestReaction_SendFailureDoesNotBurnBudget.
• Inclusive >= boundary — applied consistently in both shouldEscalate and TickEscalations, so escalation fires at exactly escalateAfter instead of waiting a tick.
• Out-of-lock dispatch note — = my finding #2, documented and deferred to daemon integration.

Still LGTM. Good to merge.