# 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.