SetMaxOpenConns(1) forced every read (List/Get/GetPR/...) to queue behind
the single connection, so the dashboard's reads contended with the LCM's
writes. WAL already supports many concurrent readers, so raise the pool to 8
and instead serialize *writes* with a Store.writeMu. That keeps WAL's
single-writer rule and the revision-CAS read-then-write atomic regardless of
pool size, while reads now run in parallel across the pool.
Every write method takes writeMu (Upsert, PatchMetadata, UpsertPR/DeletePR,
the pr_check/pr_comment Replace* via inTx, the CDC outbox/offset writes,
project writes, reaction-tracker writes); reads take nothing. Added
TestConcurrentReadsAndWrites (16 writers + 16 readers) which passes under
-race.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The first storage cut modelled two side tables as free-form blobs. This
replaces both with opinionated, statically-typed schema so what a session
can carry is fixed by the schema, not by convention.
session_metadata: was a (session_id, key, value) KV bag with six
convention-only keys. Now a 1:1 table of named, typed columns. The domain
currency is a typed domain.SessionMetadata struct (was map[string]string),
threaded through ports.LifecycleStore, the LCM, the Session Manager and the
reaper, so an unknown key is a compile error rather than a silently-dropped
write. PatchMetadata keeps its non-destructive merge ("empty = leave
unchanged"). The off-canonical invariant is now enforced at the type level
via json:"-" on SessionRecord.Metadata, removing the manual `Metadata = nil`
scrub the change_log/snapshot paths had to remember; the Meta* string-key
constants are deleted.
pr_enrichment -> pr (+ pr_check, pr_comment): the scalar facts are now
typed columns with CHECK-constrained enums (review_decision, mergeability,
ci_state) and integer CI counts instead of opaque TEXT. The two list facts
the old `pending_comments`/ci_summary strings smuggled are normalized into
child tables (pr_check, pr_comment) that cascade from pr. The store exposes
UpsertPR/GetPR plus atomic ReplacePRChecks/ReplacePRComments + List.
Both tables remain off the canonical CDC path. sqlc regenerated; migrations
0001/0002 revised in place (nothing released). gofmt/vet clean; go test
-race green; daemon smoke-boots and creates the new schema.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Six fixes from the second code review pass — none load-bearing but all
improve the contract honesty or prevent future churn. Tests pass with
-race (49 in package, 299 across the backend).
1. Preflight: atomic.Bool fast-path before the mutex so cached-success
calls don't contend on the lock. Double-checked locking on the
mutex side so concurrent first-callers still serialize the single
GET /user request.
2. Preflight godoc: tighten to say it verifies token validity, not
repo authorization — Get/List against a specific repo may still
return ErrAuthFailed after a green Preflight if the token lacks
the scope or the repo isn't visible.
3. domain.ListFilter.Limit godoc: explicitly note that exceeding the
provider per-page max is SILENTLY capped (no error, no truncation
signal) and that auto-pagination is deferred to #35.
4. Extract issueFromGH helper. Get and List were duplicating the
identical ghIssue -> domain.Issue projection; consolidating now
prevents a 3-way merge when #40 (Comment/Transition) lands.
5. parseGitHubRepo: reject whitespace and # in both owner and repo
segments. Leading dots stay legal (the "owner/.github" repo
convention). New test cases cover the rejections plus a positive
guard for the leading-dot case.
6. fakeGH test helper: lock the handlers map on both read and write,
so future tests registering handlers from goroutines won't trip
-race.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Brings the read-side surface up to parity with the legacy TS impl's
useful read methods. Closes a gap flagged during scope review.
Port additions:
- List(ctx, repo, filter) ([]Issue, error)
- Preflight(ctx) error
Domain additions: TrackerRepo, ListStateFilter (open/closed/""=all),
ListFilter (State, Labels, Assignee, Limit).
GitHub adapter:
- List hits GET /repos/{o}/{r}/issues with query-encoded filter.
Defaults: state=all, per_page=30; per_page is capped at 100.
PRs are filtered out client-side (GitHub conflates them with
issues on that endpoint).
- Preflight hits GET /user. Success is cached for the Tracker's
lifetime via sync.Mutex + bool; failures are intentionally NOT
cached so a transient startup glitch is recoverable.
- New ErrAuthFailed sentinel. classifyError now maps 401, and 403
without rate-limit signals, to ErrAuthFailed instead of an opaque
error — so Preflight callers can distinguish bad-token from other
failures.
Pagination beyond the first page is intentionally out of scope for v1
(see doc.go); the observer/polling work in #35 will own that.
Tests: 43 pass (was 25). Adds Preflight cache + recovery, List query
encoding, PR filtering, repo parser rejection, and ErrAuthFailed
classification.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Scope correction: mirroring agent lifecycle onto the tracker (status
comments, label/state updates) is not wanted in the current rewrite.
That work is now tracked as issue #40 and will land once we decide on
the opt-out knob, label setup, and Linear's workflow-state fit.
Removes from the port and the GitHub adapter:
- Comment(ctx, id, body) and ErrEmptyBody
- Transition(ctx, id, state) and ErrUnknownState
- planForState / transitionPlan and the forward state mapping
- reasonComplete constant (only the Get reverse mapping is kept)
- 11 tests + the transitionCall normalization helpers
Kept (still load-bearing for Get):
- All 5 NormalizedIssueState values — Get reports them faithfully
when a repo carries the in-progress / in-review labels.
- The reverse mapping in mapStateFromGitHub.
- RateLimitError with ResetAt + RetryAfter (#35 will use it).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Migration 0002 adds two tables off the canonical CDC path:
- projects: durable registry of managed repos (the twin of the old YAML
config). Soft-deletable via archived_at so a session's project_id always
resolves; ListProjects returns active rows only, GetProject resolves any.
- pr_enrichment: per-session cache of rich SCM facts (CI summary, review
decision, mergeability, pending comments, CI log tail) that do not live
in the canonical lifecycle. 1:1 with a session, cascades on session delete.
Both are written outside the LCM write path: no revision bump, no
change_log/outbox event. Store methods mirror the reaction_trackers adapter
pattern with storage-local row structs.
Add the two real outbound adapters that replace the in-memory fakeStore:
internal/storage/sqlite (persistence satisfying ports.LifecycleStore) and
internal/cdc (transactional-outbox publisher, JSONL delivery, durable
consumer). Wire them into main.go alongside the Lifecycle Manager and reaper
so the write path is live end-to-end: LCM.Upsert -> store -> outbox -> JSONL
-> broadcaster.
Storage (internal/storage/sqlite):
- modernc.org/sqlite (pure Go, no CGO) for clean cross-compile; goose
embedded migrations; sqlc-generated typed queries under gen/.
- Atomic Upsert: session row + change_log + outbox written in one tx.
- revision is an optimistic-concurrency (CAS) check: insert requires
revision 0 and persists 1; update requires loaded revision == stored and
bumps +1; zero rows affected returns a revision-mismatch error.
- Metadata is an opaque map in session_metadata, off the CDC path.
- Durable reaction_trackers (fixes the in-memory-only escalation budget that
re-fired human pages on restart).
CDC (internal/cdc):
- Publisher drains the outbox to a JSONL log; size-based rotation with a
reset marker.
- Consumer tails via byte cursor, detects rotation (os.SameFile), resyncs
from a full-state snapshot on gaps, and tracks a durable consumer_offsets
cursor.
- Janitor reclaims acknowledged outbox rows.
- Broadcaster is the in-process fan-out port the FE transport will subscribe
to (WS/SSE wiring deferred).
Composition root (main.go + *_wiring.go):
- startCDC stands up publisher/consumer/janitor + broadcaster.
- startLifecycle constructs the LCM, makes escalation budgets durable via
WithReactionStore, teaches it to enumerate sessions via WithSessionLister,
and starts the reaper.
- Notifier, AgentMessenger, and the reaper's runtime registry are TEMPORARY
no-op/empty stubs (lifecycle_wiring.go) with TODO markers; see the PR
description for how to fill them in.
Tests: contract-parity, revision CAS, outbox atomicity, CDC ordering and
idempotency, rotation/resync, janitor vacuum, reaction durability across a
simulated restart, and composition-root adapters. gofmt/build/vet clean and
go test -race ./... green.
Reference implementation; GitLab and Linear follow in separate PRs.
Issue observer loop (poll + ApplyTrackerFacts) is deferred to #35.
Three-layer split mirrors the SCM layout adil is landing in PR #28:
- domain/tracker.go — value types (TrackerProvider, TrackerID,
NormalizedIssueState, Issue)
- ports/tracker.go — the Tracker interface
- adapters/tracker/github/ — REST-backed adapter
v1 is write-mostly: Get, Comment, Transition. No cache, no inflight
dedup, no polling. State mapping is documented in the package doc and
exercised by table-driven tests against an httptest fake — no real
GitHub traffic from CI.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address blocker found in self-review (B1 + I1):
- Manager.RunningSessions previously filtered to runtime.State == RuntimeAlive,
but a session enters Detecting with runtime axis = RuntimeProbeFailed (failed
probe path, decide_bridge.go:72) or RuntimeMissing (detectingLC in
manager_test.go:539). The filter silently parked every Detecting session, so
the recovery path proved by manager_test.go:59 ("healthy probe recovers
liveness-owned detecting -> working") and the terminal path proved by
manager_test.go:79 ("dead+dead with no recent activity concludes killed")
were both unreachable through the reaper. Broaden the predicate to "session
is not in a terminal state" (mirrors the LCM's existing isTerminal helper)
and document the wider semantics.
- reaper.probeOne now reports every probe result — including alive — back to
the LCM as ApplyRuntimeObservation facts. The previous skip-alive
optimization was a layering violation: the reaper has no business deciding
what counts as a no-op. The LCM's ApplyRuntimeObservation already diffs
against canonical and only Upserts on actual change, so steady-state alive
stays cheap. With the broadened poll set, an alive probe for a Detecting
session IS the recovery fact.
- Add unit tests for Manager.RunningSessions covering: nil-lister no-op, lister
error propagation, and the full canonical state matrix (working/idle/
needs_input/detecting-probefailed/detecting-missing/not_started included;
terminated/done excluded).
- Update reaper tests: alive case now asserts the alive fact is reported; new
"detecting session: alive probe reported so LCM can recover from quarantine"
case locks in the recovery path; multi-runtime case now asserts both runtime
facts flow through.
- Bump "session in poll set without handle metadata" log from Debug to Warn —
it is an anomaly (OnSpawnCompleted should have written both keys), not a
routine event.
- Document WithSessionLister must be called before any reaper attached to the
Manager starts running (it is a bare field read; concurrent re-injection is
meaningless anyway).
The reaper sits OUTSIDE the LCM's per-session serial loop. On every tick it:
1. Fires lcm.TickEscalations(now) — the duration-based escalation heartbeat
a non-polling LCM cannot wake itself to drive.
2. Asks lcm.RunningSessions for the snapshot of sessions whose runtime axis is
alive, then calls runtime.IsAlive(handle) per session via a RuntimeRegistry
that dispatches by RuntimeHandle.RuntimeName (so a single reaper covers
tmux + zellij side by side).
3. Reports any non-alive result back as a fact via ApplyRuntimeObservation —
dead -> RuntimeProbeDead, probe error -> RuntimeProbeFailed (never
collapsed to alive: failed probe ≠ dead, but it ≠ alive either). Steady-
state alive is skipped so we don't churn the LCM with no-op load/diff work.
The reaper REPORTS facts; the LCM owns DECIDE (anti-flap Detecting quarantine,
terminal-session rules). The reaper never writes.
Open-question resolution: add RunningSessions(ctx) to ports.LifecycleManager
(option a). The Manager implements it via an injectable session lister
(Manager.WithSessionLister) so the LCM itself does not require a new
LifecycleStore method — Tom's store contract is untouched, daemon wiring (lane
#10) will inject the production lister at startup.
Scope: reaper goroutine + the minimum LCM seam. No activity ingest, no FS
watcher, no daemon wiring, no new schema fields, no store changes.
Four narrowly-scoped fixes against the LCM + Session Manager lane
from an external review of the current backend state. R2 (failed-restore
lifecycle stranding) is intentionally deferred to PR #15, which already
closes it via the new OnSpawnInitiated path; R3 also stays on that PR.
- R1 (BLOCKER): Manager.Spawn never persisted AgentSessionID, so
Manager.Restore's hard-required metadata key was always missing and
every restore failed. Persist the assembled launch prompt as
MetaPrompt at spawn time and add a fresh-launch fallback to Restore
that uses Agent.GetLaunchCommand with the seeded prompt when the
captured agent session id is absent (the id-capture hook is a separate
path that may never have run). Restore still fails fast when neither
the id nor a prompt is on hand — there is nothing to relaunch from.
- RA (BLOCKER): adapters/workspace/gitworktree/commands.go's
worktreeRemoveForceArgs passed --force, which deletes uncommitted
agent work. Renamed to worktreeRemoveArgs and dropped --force so the
post-prune "still registered" guard in Workspace.Destroy surfaces the
refusal to Manager.Cleanup, which routes the session to Skipped
instead of destroying in-progress changes.
- R11 (SHOULD-FIX): reactions.go's two Notifier.Notify call sites
(executeReaction's notify and escalate) built OrchestratorEvent
without ProjectID. Captured projectID on the transition (via a
store.Get in mutate) and on reactionTracker (so TickEscalations can
still populate it on duration-based escalations), and threaded it
through executeReaction/sendToAgent/escalate.
- RB (SHOULD-FIX): gitworktree.Workspace.managedPath used filepath.Join
which cleans .. segments before validateManagedPath ran, so
session=\"../other\" stayed inside managedRoot while breaking
per-project isolation. validateConfig now rejects path separators and
the . / .. components on ProjectID and SessionID at the source.
go build ./..., go vet ./..., and go test -race ./... all pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(backend): HTTP daemon skeleton — config, health, runfile, graceful shutdown (#10)
Phase 1a of the Go HTTP daemon lane (#10). Stands up the loopback-only
sidecar skeleton the later REST/SSE/WS/static surfaces build on:
- config: env-driven (AO_HOST/PORT/ENV/timeouts/run-file) with zero-config
defaults; binds 127.0.0.1:3001; validates and fails fast on bad input.
- httpd: chi router with the recoverer → request-id → logger → real-ip
middleware stack and /healthz + /readyz probes. Per-request timeout is
carried in config but intentionally not global — it scopes to /api/v1 in
Phase 1b so it never throttles SSE/WS/health.
- runfile: atomic PID + port handshake (running.json) for the Electron
supervisor, with a dead-PID stale check so a crashed predecessor doesn't
block startup while a live one fails fast.
- server: bind-before-publish (port conflict fails fast), graceful shutdown
on SIGINT/SIGTERM via signal.NotifyContext with a 10s hard timeout, and
run-file cleanup on exit.
Why: the daemon must be safely supervisable as a child process — the
supervisor needs a discoverable PID/port and the daemon must not leave a
half-started process or stale handshake behind. Locking the lifecycle down
now keeps the future port split a small change rather than a rewrite.
Tests cover config defaults/overrides/validation, run-file round-trip and
live/dead PID detection, health probes, full Run lifecycle, and port-conflict
fail-fast.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* refactor(backend): drop Env config field — not needed yet (#10)
Per review on #14: AO_ENV / Config.Env / IsProduction() weren't load-bearing
for Phase 1a — they only switched the slog handler. Removing them now keeps
the surface minimal; the env knob can come back later when a real consumer
needs it.
- config: remove Env field, AO_ENV parsing, and IsProduction helper.
- main: collapse newLogger to a single text-handler path.
- httpd: drop the env field from the listening log line.
- tests: drop the env assertions and AO_ENV fixture.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs: add backend run + config quick-start to README (#10)
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(backend): address Phase 1a review comments (#10)
- config: drop AO_HOST entirely — the daemon is loopback-only by design,
so making the bind host env-configurable was a security footgun
- config: use net.JoinHostPort in Addr() so IPv6 literals stay valid
- config: reject zero/negative AO_REQUEST_TIMEOUT and AO_SHUTDOWN_TIMEOUT
(time.ParseDuration accepts both; either would silently break the
daemon — instant request expiry / no graceful drain)
- runfile: split processAlive into unix/windows build-tagged files so
liveness detection is reliable on both platforms (Windows uses
OpenProcess; POSIX keeps signal 0)
- runfile: document os.Rename overwrite semantics (atomic on POSIX,
REPLACE_EXISTING on Windows) so the temp-then-rename pattern's
cross-platform behaviour is explicit
- httpd tests: give probe/waitForHealth clients an explicit per-request
timeout so a stalled connect can't hang the test on the outer deadline
* fix(backend): strip trailing blank line from runfile.go (#10)
gofmt CI was failing because removing the orphan processAlive doc
comment left an extra newline at EOF.
* fix(backend): cross-platform run-file replace + AO_HOST rationale (#10)
- runfile: introduce build-tagged atomicReplace — POSIX rename(2) on
Unix, MoveFileEx with MOVEFILE_REPLACE_EXISTING on Windows. The Go
runtime happens to do the Windows call internally already, but
invoking it directly makes the cross-platform contract explicit
instead of a runtime implementation detail
- runfile: tighten process_unix.go build tag from `!windows` to `unix`
so plan9/js/wasm fail to build rather than silently using a broken
signal-0 probe
- runfile: add TestWriteOverwritesExisting covering the stale run-file
replace path that none of the previous tests exercised
- config: anchor the loopback-only decision in the LoopbackHost doc so
the next contributor doesn't reintroduce AO_HOST without the security
rationale
* fix(backend): route chi access logs through slog/stderr (#10)
chi's middleware.Logger writes via stdlib log to stdout, but the
daemon's slog logger writes to stderr — so REST traffic and daemon
logs landed on different streams in different formats. Replace it
with a small slog-backed requestLogger that:
- Wraps the response writer via middleware.NewWrapResponseWriter so
status/bytes are accurate even when handlers return without an
explicit WriteHeader.
- Reads the request id off the context set by middleware.RequestID
(kept mounted just before this middleware so the id is available).
- Emits one structured Info line per request with method, path,
status, bytes, duration, and remote — same key=value shape as the
rest of the daemon, one stream for the Electron supervisor to
capture.
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Address PR #2 Copilot review comments on the merged LCM+SM lane:
- session: validate runtime handle + workspace path before Kill/Cleanup
teardown; refuse (ErrIncompleteTeardownMetadata) or skip rather than
hand empty args to a real adapter's Destroy (unsafe delete).
- session: reject Restore unless the session is terminal
(ErrNotRestorable) so a live session can't spawn a duplicate
runtime/workspace.
- ports: document SpawnConfig.OpenTerminal as reserved/not yet honored.
- lifecycle: remove the unread reactionConfig.auto field; note
approved-and-green is notify-only (human decides to merge).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds an injectable OnSpawnCompleted failure to the recording LCM and two tests:
- Spawn: when OnSpawnCompleted fails, the seeded record is parked terminal/errored
(via OnKillRequested(KillError)) and runtime+workspace are torn down.
- Restore: when OnSpawnCompleted fails post-create, the new runtime is destroyed
while the workspace is preserved.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Restore now fails early with a clear error if MetaAgentSessionID is missing,
rather than emitting an ambiguous "resume nothing" launch command (no stored
prompt means a fresh-launch fallback isn't possible).
- On a post-runtime-create failure (reopen patch or OnSpawnCompleted), best-effort
destroy the newly created runtime (never the workspace, which holds prior work)
so we don't strand a live process while parking the session terminal.
- Added a test for Restore with a missing agent session id: errors early, touches
no workspace/runtime, leaves the session terminal.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements ports.SessionManager against fakes for the outbound ports. The SM is
the explicit-mutation half of the lane: it drives Runtime/Agent/Workspace, seeds
the initial lifecycle, and routes outcomes to the LCM (OnSpawnCompleted /
OnKillRequested). It never derives observed state and is the single producer of
the derived display status (attached on read, never persisted).
- Spawn: Workspace.Create -> Runtime.Create (AO_* identity env) -> Seed ->
OnSpawnCompleted, with eager rollback of completed steps on failure.
- Kill: OnKillRequested first -> Runtime.Destroy -> Workspace.Destroy, honoring
the worktree-remove safety (refusal surfaced, never forced).
- List/Get: derive status via DeriveLegacyStatus. Send: via AgentMessenger.
Restore: re-seed (reopen) + relaunch via GetRestoreCommand. Cleanup: reclaim
terminal sessions, skip worktrees holding uncommitted work.
Store-contract additions (co-owned with Tom's persistence layer, flagged for
review): LifecycleStore.Seed (explicit create-with-identity; OnSpawnCompleted
requires a seeded record) and LifecycleStore.Get (single record-with-identity
read; Load is lifecycle-only). Lifecycle test fake updated to satisfy both.
Tests route through the real LCM Manager (wrapped to record call order).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- OnKillRequested now clears the session's escalation trackers after a
successful kill, so a later duration-based TickEscalations can't emit
reaction.escalated for a dead session (dispatch is still skipped).
- sendToAgent rolls back the attempt (and firstAttemptAt when it set it) on a
messenger.Send error, so undelivered messages don't march a reaction toward
escalation — honoring "send failures retry next tick" (§4.3).
- Duration escalation now uses an inclusive boundary (>=) in both shouldEscalate
and TickEscalations, so a 30m reaction escalates at exactly 30m instead of
waiting for the next tick.
Tests: kill clears trackers + no post-kill escalation; repeated failed delivery
never escalates; duration escalation fires at exactly escalateAfter.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address review finding #1: the persistent ci-failed tracker leaked and could
stale-silence a future regression. It was only cleared when leaving the
ci-failed reaction AND incidentOver held at that moment — so a recovery to
another open-PR state (ci-failed -> approved -> merged) never cleared it.
- react() now clears ALL of a session's trackers when the state REACHED is
incident-over (PR resolved / session terminal) OR a genuine recovery
(approved/mergeable, which the open-PR ladder guarantees means CI is no
longer failing). Keyed on the state reached, not the one left, since the
recovery transition is typically review_pending->approved (empty beforeKey).
- Persistent ci-failed still survives the ambiguous review_pending limbo, so
fail->pending->fail keeps one shared budget (§4.2).
- Document the out-of-lock react() dispatch caveat for the daemon integration
step (review #2) and the intentionally-skipped agent-stuck 10m threshold.
Tests: re-arm after a genuine recovery (regression re-nudges, not silenced);
all session trackers cleared once the incident is over.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add the ACT half of the LCM: map persisted status transitions to reactions
(send-to-agent / notify / auto-merge) and drive escalation.
- reactions.go: the §4.2 default reaction table, reactionEventFor (mirrors
DeriveLegacyStatus for the ACT layer), in-memory per-(session,reaction)
escalation trackers, the react() dispatch chokepoint, and a real
TickEscalations for duration-based escalations the synchronous LCM can't
wake itself for. auto-merge action exists but is off by default;
bugbot-comments/merge-conflicts are configured but dormant (no decide-core
producer yet).
- manager.go: mutate now returns a transition; each Apply* path fires the
mapped reaction after persist via the single synchronous react() seam.
OnKillRequested intentionally does not react (explicit kill != inferred
event). Split-A load->decide->diff->persist behavior is unchanged.
ci-failed budget is persistent across fail->pending->fail oscillation;
non-persistent trackers reset when the status leaves the triggering state.
Escalation silences further auto-dispatch until the condition clears.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Harshit's PR #5 review (approve w/ design confirms + polish):
- #1 (design decision): a valid activity signal is proof of life, so it now
resolves a detecting session — writes the activity-mapped session state and
clears the quarantine memory. Scoped to detecting only; a liveness-escalated
stuck stays the probe pipeline's to resolve. Terminal still never reopens.
- #2: document why a merged/closed PR parks the session axis even over an
activity-owned needs_input/blocked (a merge is a milestone), unlike the
open-PR path that defers to activity.
- #3: map plain idle activity to a neutral session reason instead of the
misleading research_complete (kept for ready, which implies completion).
- #6: cover all three kill kinds (manual/cleanup/error), the open-PR review
branches (changes_requested/mergeable/review_pending), and the neutral idle
reason. Coverage 86.5% -> 88.6%.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- shouldWriteSessionRuntime: never resurrect a terminal session; an
observation may refresh the runtime axis but must touch neither the
session axis nor the detecting memory (gated in ApplyRuntimeObservation).
- OnSpawnCompleted: error on an unseeded session instead of fabricating a
partial record (SM must seed first — a missing seed is a contract violation).
- OnKillRequested: no-op on an unknown/already-gone session (benign race)
instead of fabricating a terminal record.
- keyedMutex: reference-count entries and evict on last release so the lock
map stays bounded in a long-running daemon.
- runtimeSubstateFromFacts: map RuntimeProbeIndeterminate to RuntimeUnknown
with a neutral reason, distinct from the probe_error of a failed probe.
Adds tests for terminal non-resurrection, unseeded spawn-completed error,
and unknown-session kill no-op.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements ports.LifecycleManager as a synchronous observe->decide->persist
reducer. Every entrypoint runs the shared pipeline under a per-session lock:
load canonical -> run the matching pure decider -> diff into a sparse
merge-patch -> persist. Never polls, never writes the display status.
- ApplyRuntimeObservation -> probe decider; always writes the runtime axis.
- ApplySCMObservation -> open-PR / terminal-PR deciders (failed fetch is a
no-op: failed probe != "no PR"). Open PRs write only the PR axis.
- ApplyActivitySignal -> updates the activity axis + maps onto the session
axis; only valid-confidence signals are authoritative.
- OnSpawnCompleted -> runtime alive + handles to metadata; session stays
not_started (display: spawning).
- OnKillRequested -> SM's explicit terminal-write authority.
- TickEscalations -> no-op stub (reaction/escalation engine is split B).
Composition rule (#1): liveness owns the runtime + death axis; activity owns
the working/idle/waiting axis. A healthy probe verdict writes the session axis
only to recover a liveness-owned state, so it never clobbers an activity-owned
needs_input/blocked. Activity is the mirror: it stays off the death axis.
Detecting clear (#3): a non-detecting probe verdict clears stale detecting
memory so the next probe reads no phantom Prior.
Built/tested against in-memory fakes (LifecycleStore with full merge-patch +
ExpectedRevision, recording Notifier/AgentMessenger). Per-session
serialisation verified under -race.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Copilot review:
- ResolveOpenPRDecision now sets a stable, timestamp-free Evidence summary
"<condition> #<num> <url>" for every ladder outcome, consuming the
previously-unused OpenPRInput.Number/URL identity inputs and making PR
decisions traceable in logs. Covered by TestResolveOpenPRDecisionEvidence.
- Document LifecycleDecision's zero-value contract: an empty PRState/PRReason
means "this decider does not address PR — leave unchanged", not PRNone. The
LCM must map empty PR fields to a nil LifecyclePatch.PR; writing PRNone on a
probe tick would clobber a live PR. (Pointers were considered but the empty
sentinel is distinguishable from every valid state and consistent with the
codebase's value-enum style; LifecyclePatch already owns nil-means-unchanged.)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clarify the timestamp-stripping block flagged in review: spell out what
each of the three regexes matches (full ISO/RFC3339 datetime, bare
time-of-day, bare unix epoch) and why order matters.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address PR review (aa-1):
- Move the decider input/output type definitions (LifecycleDecision,
ProbeInput, ProcessLiveness, OpenPRInput, DetectingInput) out of the
execution file into a dedicated types.go, leaving decide.go for behaviour.
- Expand the detecting() helper doc to spell out that it adapts a probe
verdict into the shared CreateDetectingDecision anti-flap path so each
probe branch doesn't re-implement the quarantine counter.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address PR review (aa-1):
- ResolveOpenPRDecision now keys MERGEABLE on Mergeable alone (checked
before Approved). Mergeability is the authoritative merge gate, so a PR
on a no-required-review repo no longer falls through to PR_OPEN. The
approved+mergeable and approved-only cases are unchanged.
- Broaden the derive-consistency test to cover the probe and terminal
deciders too, not just the open-PR ladder.
- Document the HashEvidence epoch-stripping regex's breadth.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the stubbed deciders in domain/decide with real, total,
side-effect-free implementations:
- ResolveProbeDecision: kill-intent short-circuits to terminal; failed
probes and probe disagreement route to detecting; only runtime-dead +
process-dead + no-recent-activity concludes killed.
- ResolveOpenPRDecision: the PR pipeline ladder
(ci_failing > changes_requested > approved+mergeable > approved >
review_pending > idle-beyond > open).
- ResolveTerminalPRStateDecision: merged -> idle/merged_waiting_decision,
closed -> idle.
- CreateDetectingDecision: anti-flap quarantine — unchanged-evidence
counter with StartedAt preserved across the episode so the duration cap
is a real wall-clock safety net; escalates to stuck at 3 ticks or 5m.
- HashEvidence: strips timestamps/epochs and collapses whitespace before
hashing so restamped-but-unchanged signals compare equal.
Table tests cover every branch (100% statement coverage), including a
consistency check that the open-PR ladder's display Status matches
DeriveLegacyStatus over the canonical state it emits.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
Contract-first boundary for the Lifecycle Manager + Session Manager lane.
Pure shapes only — types, interfaces, and the display-status derivation —
so adil (SCM poller), Tom (persistence), and aditi (API) can review and
build against a stable boundary before any behaviour lands.
domain/
- CanonicalSessionLifecycle: the only persisted state (session/pr/runtime
sub-states), with Activity + Detecting sub-states added as decider inputs
that must survive between observations.
- DeriveLegacyStatus: the sole producer of the derived display status
(never persisted), with 11 table tests.
ports/
- inbound: LifecycleManager (Apply* pipeline, per-session serialised) and
SessionManager.
- outbound: LifecycleStore (Tom), Notifier, AgentMessenger, and the
Runtime/Agent/Workspace plugin ports (co-owned with the agents lane).
- facts: SCMFacts / RuntimeFacts / ActivitySignal DTOs.
decide/ pure-core signatures + I/O types; bodies stubbed for the next PR.
Folds in four design-review fixes (documented in-code, pending team confirm):
1. Activity + Detecting persisted so the pure decider has memory across calls.
2. Per-session serialisation documented; LifecyclePatch.ExpectedVersion
offers optimistic-locking as an alternative.
3. LifecyclePatch is a sparse pointer-field merge-patch (+ ClearDetecting).
4. SCMFacts gains Fetched (failed fetch != "PR closed") and per-comment
IsBot (bot vs human route to different reactions).
go build / go vet / go test all pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
prateek
harshitsinghbhandari
Pritom14
Harshit Singh Bhandari
neversettle