From 79b3c0be95aaf0bdfd586f394b6369f3d33e7e6b Mon Sep 17 00:00:00 2001 From: Felix Martin Date: Mon, 1 Jun 2026 17:24:36 -0400 Subject: [PATCH] Add M9 tame-session refactor design spec Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-06-01-m9-tame-session-design.md | 201 ++++++++++++++++++ 1 file changed, 201 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-01-m9-tame-session-design.md diff --git a/docs/superpowers/specs/2026-06-01-m9-tame-session-design.md b/docs/superpowers/specs/2026-06-01-m9-tame-session-design.md new file mode 100644 index 0000000..fc8b1fc --- /dev/null +++ b/docs/superpowers/specs/2026-06-01-m9-tame-session-design.md @@ -0,0 +1,201 @@ +# M9 — Tame `session.go`: Design + +**Status:** approved +**Date:** 2026-06-01 +**Milestone:** M9 — Maintainability: split the monolithic `session.go` and +consolidate the duplicated async-fetch boilerplate, with zero behavior change + +## Purpose + +The M0–M8 feature arc left `session.Controller` carrying five responsibilities +in a single 1278-line file — by far the largest in the codebase (the next is +`web.go` at 243). Every milestone's design doc has flagged two specific debts: +the file is too big to hold in context at once, and the per-role async-fetch +block (capture generation → goroutine → re-lock → latest-wins) is copy-pasted +across coach, tasks, knowledge, and reflection. + +M9 pays both down so the controller is easy to extend before any new feature +lands. It is a **pure maintainability milestone**: no new behavior, no API +change, no exported-symbol rename. Success is the existing test suite passing +**green-to-green under `-race`**, before and after. + +## Scope + +Two changes, both confined to `package session`: + +1. **File split** — move declarations (no logic edits) out of the monolith into + focused files, each with one clear responsibility. +2. **Async-fetch consolidation** — extract the mechanical goroutine dance shared + by the four async fetches into one helper, while every real per-role + difference stays explicit at the call site. + +Everything else — drift/stats/web/daemon logic, the deferred M8 Tiers B/C — +is untouched. + +## The async-fetch helper + +Today four methods (`RequestCoach`, `startTasksFetchLocked`, +`startKnowledgeFetchLocked`, `startReflectionFetchLocked`) repeat the same +goroutine skeleton: open a timeout context, perform the I/O with no lock held, +re-acquire `c.mu`, discard the result if a generation guard says it is stale, +otherwise record it and `notify`. The role-specific parts around that skeleton +genuinely differ and **must stay per-role**: + +- the generation field (`coachGen` / `tasksGen` / `knowledgeGen` / + `reflectionGen`) and status enum; +- the stale guard — coach/tasks/knowledge check *gen mismatch **or** left + Planning*; reflection checks *gen only* (its carry-forward must survive `End` + before the reviewer returns); +- the apply logic — tasks/coach are two-branch; knowledge is three-branch and + writes `knowledgePath` back; reflection is two-branch and calls + `persistLocked`; +- pre-goroutine work — reflection reads `history` synchronously under the lock, + a happens-before requirement against `End`'s audit-chain append, which must be + preserved; +- `RequestCoach` manages its own lock and `notify`s the pending state before + launching; the `*Locked` variants are launched mid-transition from + `EnterPlanning` / `enterReview` while the caller still holds `c.mu`. + +The helper therefore extracts **only** the mechanical dance and takes three +closures plus the timeout: + +```go +// runFetchAsync launches a generation-guarded background fetch. The caller has +// captured its dependencies and (for the *Locked callers) holds c.mu; this +// method only spawns the goroutine. fetch performs the I/O with no lock held; +// stale reports whether to discard the result; apply records it under the +// re-acquired lock (and persists itself when the role requires it). +func (c *Controller) runFetchAsync(timeout time.Duration, fetch func(ctx context.Context), stale func() bool, apply func()) { + go func() { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + fetch(ctx) + c.mu.Lock() + if stale() { + c.mu.Unlock() + return + } + apply() + c.mu.Unlock() + c.notify() + }() +} +``` + +Each role keeps its own setup (clear cache, nil-provider short-circuit, `gen++`, +pending status, dep capture) and passes `fetch` / `stale` / `apply` closures +over its locals. Example (tasks): + +```go +func (c *Controller) startTasksFetchLocked() { + c.tasksList = nil + if c.tasksProvider == nil { + c.tasksStatus = tasksIdle + return + } + c.tasksGen++ + gen := c.tasksGen + c.tasksStatus = tasksPending + p := c.tasksProvider + var list []tasks.Task + var err error + c.runFetchAsync(tasksTimeout, + func(ctx context.Context) { list, err = p.Today(ctx) }, + func() bool { return gen != c.tasksGen || c.runtimeState != domain.RuntimePlanning }, + func() { + if err != nil { + c.tasksStatus = tasksError + c.tasksList = nil + } else { + c.tasksStatus = tasksReady + c.tasksList = list + } + }) +} +``` + +`runFetchAsync` does not require the lock to be held (it only spawns the +goroutine, which re-acquires `c.mu` itself), so it is safe to call both from a +`*Locked` caller still inside a transition and from `RequestCoach` after it has +unlocked and notified. + +**Rejected alternatives:** + +- *Generic free function* `asyncFetch[T](c, timeout, fetch (ctx)(T,error), + stale, apply func(T,error))`. More type-safe — the result flows as a typed + value rather than a captured closure var — but Go methods cannot be generic, + so it must be a package-level function, and the per-role branches still live + in `apply`. The closure-method form is the smaller, lock-idiomatic diff. +- *Struct-per-role value* encapsulating `gen` + status + timeout. The most + structure but the most churn; four small roles do not justify the machinery + (YAGNI). +- *Unifying the four `gen` int fields* into one shared counter type. Pure churn + for no payoff; out of scope. + +## File decomposition + +All files remain `package session`. **No exported symbol moves out of the +package, is renamed, or changes signature** — only the file a declaration lives +in changes. + +| File | Responsibility | Declarations | +| ---- | -------------- | ------------ | +| `session.go` | core controller + lifecycle | `Controller` struct, `New`, `SetClock`/`SetOnChange`/`notify`, `State`/`Deadline`, `persistLocked`, the lifecycle transitions (`EnterPlanning`, `StartManualCommitment`, `Complete`/`Expire`/`enterReview`, `End`, `buildSummaryLocked`), `ErrNotPlanning`/`ErrNotActive` | +| `views.go` | UI projection (pure data shaping) | the 11 `*View` types, the `State` type, `stateLocked`, `bucketViews` | +| `roles.go` | AI roles + the async-fetch helper | `runFetchAsync`; coach (`SetCoach`, `resetCoachLocked`, `composedGroundingLocked`, `RequestCoach`, `coachErrorMessage`); tasks (`SetTasks`, `startTasksFetchLocked`); knowledge (`SetKnowledge`, `SetKnowledgePath`, `startKnowledgeFetchLocked`); reflection (`SetReviewer`, `startReflectionFetchLocked`, `buildReflectionFinishedLocked`, `buildReflectionHistory`); the coach/tasks/knowledge/reflection timeout + status consts | +| `drift.go` | Active-state drift/nudge/enforcement | `RecordWindow`, `evaluateDriftLocked`, `maybeNudgeLocked`, `enforceActionLocked`, `applyVerdictLocked`, `resetDriftLocked`, `OnTask`, `Refocus`, `recordTitleLocked`, `commitmentLineLocked`, `Set{DriftJudge,Guard,Nudge}`, the drift/nudge/enforce consts | +| `stats.go` | per-session evidence accounting | `EvidenceStats`, `bucketKey`, `applyEvent`, `replayStats`, `keyFor`, `focusEvent`, `snapFromEvent` | + +The `*ForTest` accessors (`AllowedClassesForTest`, `EnforcementLevelForTest`, +`recentTitlesForTest`) stay in regular `.go` files (not `_test.go`) grouped with +the cluster they expose, because `internal/web/web_test.go` reaches some of them +across the package boundary; a `_test.go` placement would be invisible to that +package and break the build. The plan confirms each accessor's call sites before +choosing its file. + +Splitting into *sub-packages* is explicitly rejected: every method mutates one +`Controller` behind one `sync.Mutex`, so sub-packages would force that private +state to be exported. One package across several files is the idiomatic Go shape +and keeps the locking invariant intact. + +## Sequencing & safety + +The discipline for a refactor of the controller is behavior preservation proven +by the current tests: + +- **Phase 1 — file split (zero logic change).** Move declarations into the new + files. `go build ./...` + `go vet ./...` + `go test -race ./...` green. Lowest + risk, done first so the structure exists before any logic moves. One small + commit per file extracted. +- **Phase 2 — consolidation.** Add `runFetchAsync`; migrate the four roles to it + **one at a time**, each its own commit, the full `-race` suite green between + each migration. Performing this after the split means each migration is a + clean diff inside `roles.go` rather than inside the old monolith. + +At no point is the build or the suite left red. The split being first means a +mistake there is caught before any semantically-meaningful change is layered on. + +## Testing + +No new behavior means no new behavioral tests are *required*; the contract is +green-to-green under `-race`. The plan first **audits** that the existing suite +covers each async role's: + +- stale-generation discard, +- the not-Planning completion gate (coach/tasks/knowledge), +- reflection's gen-only guard plus its `persistLocked` on completion, +- knowledge's three-branch apply and `knowledgePath` write-back. + +A characterization test is added **only where the audit finds a real gap** — so +the consolidation cannot silently change a path the suite never exercised. +Otherwise the existing `session_test.go` and `web_test.go` are the safety net, +run after every commit. + +## Out of scope + +- Any behavior change, API/signature change, or exported-symbol rename. +- Unifying the four `gen` int fields into a shared type. +- Touching drift/stats/web/daemon **logic** (only moving declarations). +- Splitting `session` into sub-packages. +- M8 Tiers B (network blocking) and C (privileged entry gate) — separate + milestones.