refactor(backend): address contract review on LCM+SM ports

Review feedback on PR #2:

- Add CanonicalSessionLifecycle.Revision (monotonic write counter) distinct
  from the schema Version; LifecyclePatch.ExpectedVersion -> ExpectedRevision
  now compares it, so optimistic locking actually works.
- LifecycleStore.List returns []domain.SessionRecord (persistence shape, no
  derived status); add SessionRecord and make Session embed it. Keeps the
  Session Manager the single producer of the derived display status.
- SpawnOutcome.RuntimeHandle is now the structured ports.RuntimeHandle, not a
  string, so Destroy/SendMessage get the handle without ad-hoc encoding.
- Agent.IsProcessRunning -> ProbeProcess returning ProcessProbe (liveness),
  not domain.ActivityState; the name no longer implies a boolean.
- Document LifecyclePatch Detecting vs ClearDetecting precedence (clear wins).
- Correct the DeriveLegacyStatus doc: hard session states outrank PR facts;
  "PR dominates" applies only to the soft idle/working states. Implementation
  was already correct (matches canonical AO); only the comment overstated it.
- Replace personal-name attributions in package/interface comments with
  role-based terms (SCM poller / persistence adapter / API layer).

go build / go vet / go test all pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

backend/internal/domain/lifecycle.go

@@ -20,10 +20,15 @@ const LifecycleVersion = 1
 // between observations (they are read back by the pure decide core), so they
 // live in the persisted record too.
 type CanonicalSessionLifecycle struct {
-	Version int             `json:"version"`
-	Session SessionSubstate `json:"session"`
-	PR      PRSubstate      `json:"pr"`
-	Runtime RuntimeSubstate `json:"runtime"`
+	// Version is the schema version of this record's shape (LifecycleVersion).
+	Version int `json:"version"`
+	// Revision is a monotonic counter the store bumps on every write. It is used
+	// for optimistic-concurrency checks (LifecyclePatch.ExpectedRevision) and is
+	// distinct from the schema Version above.
+	Revision int             `json:"revision"`
+	Session  SessionSubstate `json:"session"`
+	PR       PRSubstate      `json:"pr"`
+	Runtime  RuntimeSubstate `json:"runtime"`
 
 	// Activity is the last-known agent activity. It arrives on a different
 	// cadence (ApplyActivitySignal) than runtime probes (the reaper), so the

backend/internal/domain/session.go

@@ -17,17 +17,26 @@ const (
 	KindOrchestrator SessionKind = "orchestrator"
 )
 
-// Session is the read-model returned across the API boundary (to controllers,
-// then the frontend). Status is the DERIVED display status, attached on read by
-// the Session Manager so the API layer never recomputes it (single producer).
-type Session struct {
+// SessionRecord is the PERSISTENCE shape: identity, canonical lifecycle, and
+// metadata — everything the store holds, and nothing derived. The store reads
+// and writes records; it never produces the derived display status.
+type SessionRecord struct {
 	ID        SessionID                 `json:"id"`
 	ProjectID ProjectID                 `json:"projectId"`
 	IssueID   IssueID                   `json:"issueId,omitempty"`
 	Kind      SessionKind               `json:"kind"`
 	Lifecycle CanonicalSessionLifecycle `json:"lifecycle"`
-	Status    SessionStatus             `json:"status"`
 	Metadata  map[string]string         `json:"metadata,omitempty"`
 	CreatedAt time.Time                 `json:"createdAt"`
 	UpdatedAt time.Time                 `json:"updatedAt"`
 }
+
+// Session is the read-model returned across the API boundary (to controllers,
+// then the frontend): a SessionRecord plus the DERIVED display Status. The
+// Session Manager is the single producer of Status — it builds a Session from a
+// stored SessionRecord by calling DeriveLegacyStatus, so the store and API
+// never recompute (or accidentally persist) it.
+type Session struct {
+	SessionRecord
+	Status SessionStatus `json:"status"`
+}

backend/internal/domain/status.go

@@ -28,12 +28,17 @@ const (
 // DeriveLegacyStatus is the ONLY producer of the display status. It must stay a
 // pure, total function of the canonical record.
 //
-// Order matters and encodes the core invariant "PR facts dominate session facts
-// once a PR exists":
-//  1. Terminal / hard session states map directly (terminated sub-switches on reason).
-//  2. A merged PR wins.
-//  3. An open PR maps by its reason.
-//  4. Otherwise fall through to the raw session state.
+// Order matters:
+//  1. Terminal / hard session states (done, terminated, needs_input, stuck,
+//     detecting, not_started) map directly — these OUTRANK PR facts.
+//  2. Otherwise a merged PR wins.
+//  3. Otherwise an open PR maps by its reason.
+//  4. Otherwise fall through to the SOFT session state (idle/working).
+//
+// So "PR facts dominate session facts" applies only to the soft states: an idle
+// or working session with an open, CI-failing PR displays as ci_failed — but a
+// session that is stuck or needs_input shows that regardless of PR state, since
+// it needs a human either way.
 func DeriveLegacyStatus(l CanonicalSessionLifecycle) SessionStatus {
 	switch l.Session.State {
 	case SessionDone:

backend/internal/ports/facts.go

@@ -2,8 +2,8 @@
 // lane: the inbound interfaces we implement, the outbound interfaces others
 // implement for us, and the fact DTOs that cross those boundaries.
 //
-// These are the types adil (SCM poller), Tom (persistence), and aditi (API)
-// build against, so they are committed and stabilised before the LCM/SM logic.
+// These are the types the SCM poller, persistence adapter, and API layer build
+// against, so they are committed and stabilised before the LCM/SM logic.
 package ports
 
 import (
@@ -12,7 +12,7 @@ import (
 	"github.com/aoagents/agent-orchestrator/backend/internal/domain"
 )
 
-// SCMFacts is produced by adil's SCM poller and handed to ApplySCMObservation.
+// SCMFacts is produced by the SCM poller and handed to ApplySCMObservation.
 //
 // Fetched is the failed-probe guard: when false, the GitHub query timed out or
 // errored and the rest of the struct is meaningless — the LCM must NOT read it
@@ -120,10 +120,12 @@ const (
 )
 
 // SpawnOutcome is what the Session Manager reports to the LCM after a spawn.
+// RuntimeHandle is the same structured handle the Runtime port returns, so no
+// ad-hoc string encoding is needed for later Destroy/SendMessage calls.
 type SpawnOutcome struct {
 	Branch         string
 	WorkspacePath  string
-	RuntimeHandle  string
+	RuntimeHandle  RuntimeHandle
 	AgentSessionID string
 }
 

backend/internal/ports/outbound.go

@@ -6,33 +6,41 @@ import (
 	"github.com/aoagents/agent-orchestrator/backend/internal/domain"
 )
 
-// LifecycleStore is Tom's persistence layer, the ONLY disk writer. It owns
+// LifecycleStore is the persistence adapter, the ONLY disk writer. It owns
 // merge-patch, atomic write, file lock, and CDC eventing. The LCM and SM only
 // ever touch state through this narrow interface.
+//
+// List returns persistence records (no derived status); the Session Manager
+// turns those into domain.Session by attaching the derived display status.
 type LifecycleStore interface {
 	Load(ctx context.Context, id domain.SessionID) (domain.CanonicalSessionLifecycle, bool, error)
 	PatchLifecycle(ctx context.Context, id domain.SessionID, patch LifecyclePatch) error
-	List(ctx context.Context, project domain.ProjectID) ([]domain.Session, error)
+	List(ctx context.Context, project domain.ProjectID) ([]domain.SessionRecord, error)
 	GetMetadata(ctx context.Context, id domain.SessionID) (map[string]string, error)
 	PatchMetadata(ctx context.Context, id domain.SessionID, kv map[string]string) error
 }
 
 // LifecyclePatch is a sparse merge-patch: a nil field is left untouched, a
-// non-nil field is written. Detecting needs three-way semantics (leave / set /
-// clear-to-nil) which a single pointer can't express, so ClearDetecting handles
-// the clear case explicitly.
+// non-nil field is written.
 //
-// ExpectedVersion supports optimistic concurrency: when non-nil the store must
-// reject the patch if the stored Version differs. (Open for Tom to confirm vs.
-// the LCM owning all serialisation itself.)
+// Detecting needs three-way semantics (leave / set / clear-to-nil):
+//   - ClearDetecting == true  → store clears the detecting memory and IGNORES
+//     the Detecting field (clear wins; setting both is a caller bug).
+//   - ClearDetecting == false, Detecting != nil → set/replace the memory.
+//   - ClearDetecting == false, Detecting == nil  → leave it untouched.
+//
+// ExpectedRevision supports optimistic concurrency: when non-nil the store must
+// reject the patch if the stored Revision (the monotonic write counter, NOT the
+// schema Version) differs. This is the alternative to the LCM owning all
+// per-session serialisation itself.
 type LifecyclePatch struct {
-	Session         *domain.SessionSubstate
-	PR              *domain.PRSubstate
-	Runtime         *domain.RuntimeSubstate
-	Activity        *domain.ActivitySubstate
-	Detecting       *domain.DetectingState
-	ClearDetecting  bool
-	ExpectedVersion *int
+	Session          *domain.SessionSubstate
+	PR               *domain.PRSubstate
+	Runtime          *domain.RuntimeSubstate
+	Activity         *domain.ActivitySubstate
+	Detecting        *domain.DetectingState
+	ClearDetecting   bool
+	ExpectedRevision *int
 }
 
 // Notifier delivers events to the human (desktop/Slack later). Push, never pull.
@@ -92,7 +100,10 @@ type RuntimeHandle struct {
 type Agent interface {
 	GetLaunchCommand(cfg AgentConfig) string
 	GetEnvironment(cfg AgentConfig) map[string]string
-	IsProcessRunning(ctx context.Context, handle RuntimeHandle) (domain.ActivityState, error)
+	// ProbeProcess returns the agent process liveness classification
+	// (alive/dead/indeterminate/failed) — not a boolean and not an activity
+	// state. Activity classification arrives separately via ActivitySignal.
+	ProbeProcess(ctx context.Context, handle RuntimeHandle) (ProcessProbe, error)
 	GetRestoreCommand(agentSessionID string) string
 }