antidrift/docs/superpowers/specs/2026-06-01-m9-tame-session-design.md

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 notifys 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:

// 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):

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.