Add M9 tame-session refactor design spec
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||||
Reference in New Issue
Block a user