681 lines
26 KiB
Markdown
681 lines
26 KiB
Markdown
# M9 — Tame `session.go` Implementation Plan
|
||
|
||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||
|
||
**Goal:** Split the 1278-line `internal/session/session.go` into five focused files and consolidate the 4×-duplicated async-fetch boilerplate into one helper, with **zero behavior change**.
|
||
|
||
**Architecture:** Everything stays in `package session` (one `Controller` behind one `sync.Mutex` — sub-packages would force private state to be exported). Phase 1 moves declarations into focused files with no logic edits. Phase 2 adds a `runFetchAsync(timeout, fetch, stale, apply)` helper and migrates the four async roles to it one at a time. The existing `session_test.go` + `web_test.go` suites are the safety net; the contract is **green-to-green under `-race`** after every commit.
|
||
|
||
**Tech Stack:** Go 1.26, stdlib `testing`, `go test -race`. No new dependencies.
|
||
|
||
---
|
||
|
||
## Critical rules for every task
|
||
|
||
- **No behavior change.** Move and re-shape code; never alter what it does. No exported symbol is renamed, removed, or has its signature changed.
|
||
- **Locate declarations by name** (`grep -n`), not by the line numbers in this plan — line numbers shift as earlier tasks move code. The line numbers here are hints from the pre-refactor file.
|
||
- **Move bodies verbatim.** In Phase 1, cut each declaration exactly as written and paste it into the new file. Do not edit logic.
|
||
- **Resolve imports with the compiler.** After moving declarations, run `go build ./internal/session/`. Add the imports the new file needs; delete imports the compiler reports as now-unused in `session.go`. If `goimports` is available (`go run golang.org/x/tools/cmd/goimports@latest -w internal/session/`), it does both automatically. The "expected imports" lists below are guidance, not gospel.
|
||
- **Green gate after every commit:** `go build ./... && go vet ./... && go test -race ./internal/session/ ./internal/web/`. All must pass. Never commit red.
|
||
- All commands run from the repo root `/home/felixm/dev/antidrift`. Commit messages end with the `Co-Authored-By` trailer used in this repo.
|
||
|
||
---
|
||
|
||
## File Structure (target)
|
||
|
||
All `package session`:
|
||
|
||
- `session.go` — `Controller` struct, `New`, `SetClock`/`SetOnChange`/`notify`, `State`/`Deadline`, `persistLocked`, lifecycle transitions (`EnterPlanning`, `StartManualCommitment`, `Complete`/`Expire`/`enterReview`, `End`, `buildSummaryLocked`), `ErrNotPlanning`/`ErrNotActive`, the `unavailableTitle`/`sessionRetention` consts, `AllowedClassesForTest`/`EnforcementLevelForTest`.
|
||
- `views.go` — the 11 `*View` types, the `State` type, `stateLocked`, `bucketViews`.
|
||
- `roles.go` — `runFetchAsync` + coach/tasks/knowledge/reflection (`Set*`, `start*FetchLocked`/`RequestCoach`, `composedGroundingLocked`, `coachErrorMessage`, `buildReflectionFinishedLocked`, `buildReflectionHistory`) + their timeout/status consts.
|
||
- `drift.go` — `RecordWindow`, `evaluateDriftLocked`, `maybeNudgeLocked`, `enforceActionLocked`, `applyVerdictLocked`, `resetDriftLocked`, `OnTask`, `Refocus`, `recordTitleLocked`, `commitmentLineLocked`, `Set{DriftJudge,Guard,Nudge}`, `recentTitlesForTest`, the drift/nudge/enforce consts (`driftDebounce`/`driftTimeout`/`enforceTimeout`/`nudgeDebounce`/`nudgeTimeout`, the `drift*` status consts, `recentTitlesMax`).
|
||
- `stats.go` — `EvidenceStats`, `bucketKey`, `applyEvent`, `replayStats`, `keyFor`, `focusEvent`, `snapFromEvent`.
|
||
|
||
---
|
||
|
||
## Task 1: Close the coverage gap — knowledge stale-discard test
|
||
|
||
The audit found one gap: knowledge has no dedicated stale-discard test, while coach/tasks/reflection do. Add one **before** refactoring so the shared helper's stale path is covered for every role. This is a **characterization test** — it documents existing behavior and must pass against the current (un-refactored) code.
|
||
|
||
**Files:**
|
||
- Modify: `internal/session/session_test.go`
|
||
|
||
- [ ] **Step 1: Add a gate channel to `fakeSource`**
|
||
|
||
The knowledge test double `fakeSource` (defined around line 1027 of `internal/session/session_test.go`) currently has no gate, so a load can't be held in flight. Add one, mirroring `fakeProvider` (line ~885) and `fakeCoach` (line ~251). Replace:
|
||
|
||
```go
|
||
type fakeSource struct {
|
||
profile knowledge.Profile
|
||
err error
|
||
}
|
||
|
||
func (f *fakeSource) Load(ctx context.Context, path string) (knowledge.Profile, error) {
|
||
return f.profile, f.err
|
||
}
|
||
```
|
||
|
||
with:
|
||
|
||
```go
|
||
type fakeSource struct {
|
||
profile knowledge.Profile
|
||
err error
|
||
gate chan struct{} // if non-nil, Load blocks until it receives
|
||
}
|
||
|
||
func (f *fakeSource) Load(ctx context.Context, path string) (knowledge.Profile, error) {
|
||
if f.gate != nil {
|
||
<-f.gate
|
||
}
|
||
return f.profile, f.err
|
||
}
|
||
```
|
||
|
||
This is behavior-preserving for the existing knowledge tests (they construct `fakeSource` without a `gate`, so it stays nil and `Load` never blocks).
|
||
|
||
- [ ] **Step 2: Write the characterization test**
|
||
|
||
Add to `internal/session/session_test.go`, mirroring `TestStaleTasksFetchDiscardedAfterLeavingPlanning` (line ~964). The `State.Knowledge` field is a `*KnowledgeView` accessed as `st.Knowledge.Path`/`st.Knowledge.Chars` in the existing knowledge tests:
|
||
|
||
```go
|
||
// TestStaleKnowledgeFetchDiscardedAfterLeavingPlanning exercises the
|
||
// "left planning" arm of the discard guard in startKnowledgeFetchLocked:
|
||
// a slow knowledge load that returns after the user has left planning must
|
||
// not clobber fresh state.
|
||
func TestStaleKnowledgeFetchDiscardedAfterLeavingPlanning(t *testing.T) {
|
||
c, _ := newTestController(t)
|
||
staleGate := make(chan struct{})
|
||
stale := &fakeSource{
|
||
profile: knowledge.Profile{Text: "STALE", Path: "/stale"},
|
||
gate: staleGate,
|
||
}
|
||
c.SetKnowledge(stale)
|
||
|
||
if err := c.EnterPlanning(); err != nil { // launches the gated load (gen 1)
|
||
t.Fatal(err)
|
||
}
|
||
// Leave planning while the gen-1 load is still blocked on the gate.
|
||
if err := c.StartManualCommitment("write report", "report drafted", 25*time.Minute, []string{"code"}, domain.EnforcementWarn); err != nil {
|
||
t.Fatal(err)
|
||
}
|
||
|
||
close(staleGate) // release gen 1; it must be discarded (left planning)
|
||
// Give the released goroutine time to attempt its commit.
|
||
time.Sleep(50 * time.Millisecond)
|
||
|
||
st := c.State()
|
||
if st.Knowledge != nil && st.Knowledge.Chars != 0 {
|
||
t.Fatalf("stale knowledge fetch clobbered state: %+v", st.Knowledge)
|
||
}
|
||
}
|
||
```
|
||
|
||
- [ ] **Step 3: Run it — expect PASS (characterizes current behavior)**
|
||
|
||
Run: `go test -race ./internal/session/ -run TestStaleKnowledgeFetchDiscardedAfterLeavingPlanning -v`
|
||
Expected: PASS. (It documents existing behavior; if it fails, the test is wrong or the fake's gate is mis-wired — fix the test, not production code.)
|
||
|
||
- [ ] **Step 4: Full green gate**
|
||
|
||
Run: `go build ./... && go vet ./... && go test -race ./internal/session/ ./internal/web/`
|
||
Expected: all PASS.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add internal/session/session_test.go
|
||
git commit -m "Add knowledge stale-discard characterization test
|
||
|
||
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 2: Extract `views.go` (file split, no logic change)
|
||
|
||
**Files:**
|
||
- Create: `internal/session/views.go`
|
||
- Modify: `internal/session/session.go`
|
||
|
||
- [ ] **Step 1: Create the new file with the package clause**
|
||
|
||
Create `internal/session/views.go`:
|
||
|
||
```go
|
||
package session
|
||
```
|
||
|
||
- [ ] **Step 2: Move the declarations verbatim**
|
||
|
||
Cut these declarations from `session.go` (locate each by name with `grep -n 'type CommitmentView' internal/session/session.go` etc.) and paste them, unedited, into `views.go`:
|
||
|
||
- the view types, in this order: `CommitmentView`, `ProposalView`, `DriftView`, `CoachView`, `TaskView`, `TasksView`, `KnowledgeView`, `ReflectionView`, `WindowView`, `BucketView`, `EvidenceView`, `State`
|
||
- the method `func (c *Controller) stateLocked() State { ... }`
|
||
- the function `func bucketViews(buckets map[bucketKey]time.Duration) []BucketView { ... }`
|
||
|
||
Do not change a single line of their bodies.
|
||
|
||
- [ ] **Step 3: Fix imports**
|
||
|
||
Run: `go build ./internal/session/`
|
||
`views.go` is expected to need: `sort`, `time`, `antidrift/internal/domain`, `antidrift/internal/evidence`, `antidrift/internal/tasks`. Add whatever the compiler reports as undefined, and remove from `session.go` any import the compiler now reports as unused. (Or run `goimports -w internal/session/` to do both.)
|
||
|
||
- [ ] **Step 4: Green gate**
|
||
|
||
Run: `go build ./... && go vet ./... && go test -race ./internal/session/ ./internal/web/`
|
||
Expected: all PASS (pure code motion — behavior is identical).
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add internal/session/views.go internal/session/session.go
|
||
git commit -m "Split session views into views.go
|
||
|
||
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 3: Extract `stats.go` (file split, no logic change)
|
||
|
||
**Files:**
|
||
- Create: `internal/session/stats.go`
|
||
- Modify: `internal/session/session.go`
|
||
|
||
- [ ] **Step 1: Create the new file**
|
||
|
||
Create `internal/session/stats.go`:
|
||
|
||
```go
|
||
package session
|
||
```
|
||
|
||
- [ ] **Step 2: Move the declarations verbatim**
|
||
|
||
Cut these from `session.go` and paste unedited into `stats.go`:
|
||
|
||
- `type bucketKey struct{ Class, Title string }`
|
||
- `type EvidenceStats struct { ... }`
|
||
- `func (c *Controller) applyEvent(now time.Time, snap evidence.WindowSnapshot)`
|
||
- `func (c *Controller) replayStats(sessionID string)`
|
||
- `func keyFor(snap evidence.WindowSnapshot) bucketKey`
|
||
- `func focusEvent(now time.Time, snap evidence.WindowSnapshot) store.FocusEvent`
|
||
- `func snapFromEvent(e store.FocusEvent) evidence.WindowSnapshot`
|
||
|
||
- [ ] **Step 3: Fix imports**
|
||
|
||
Run: `go build ./internal/session/`
|
||
`stats.go` is expected to need: `time`, `antidrift/internal/evidence`, `antidrift/internal/store`. Add/remove per the compiler (or `goimports -w`).
|
||
|
||
- [ ] **Step 4: Green gate**
|
||
|
||
Run: `go build ./... && go vet ./... && go test -race ./internal/session/ ./internal/web/`
|
||
Expected: all PASS.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add internal/session/stats.go internal/session/session.go
|
||
git commit -m "Split evidence-stats accounting into stats.go
|
||
|
||
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 4: Extract `drift.go` (file split, no logic change)
|
||
|
||
**Files:**
|
||
- Create: `internal/session/drift.go`
|
||
- Modify: `internal/session/session.go`
|
||
|
||
- [ ] **Step 1: Create the new file**
|
||
|
||
Create `internal/session/drift.go`:
|
||
|
||
```go
|
||
package session
|
||
```
|
||
|
||
- [ ] **Step 2: Move the consts verbatim**
|
||
|
||
Cut these const groups from `session.go` and paste into `drift.go`:
|
||
|
||
- the block `const ( driftDebounce ... driftTimeout ... enforceTimeout ... nudgeDebounce ... nudgeTimeout ... )`
|
||
- `const recentTitlesMax = 10`
|
||
- the block `const ( driftIdle ... driftPending ... driftOnTask ... driftDrifting ... )`
|
||
|
||
- [ ] **Step 3: Move the methods verbatim**
|
||
|
||
Cut these from `session.go` and paste unedited into `drift.go`:
|
||
|
||
- `Set{DriftJudge,Guard,Nudge}` — i.e. `SetDriftJudge`, `SetGuard`, `SetNudge`
|
||
- `resetDriftLocked`, `OnTask`, `Refocus`
|
||
- `commitmentLineLocked`
|
||
- `RecordWindow`, `enforceActionLocked`, `evaluateDriftLocked`, `maybeNudgeLocked`
|
||
- `recordTitleLocked`, `applyVerdictLocked`
|
||
- `recentTitlesForTest`
|
||
|
||
- [ ] **Step 4: Fix imports**
|
||
|
||
Run: `go build ./internal/session/`
|
||
`drift.go` is expected to need: `context`, `log`, `time`, `antidrift/internal/ai`, `antidrift/internal/domain`, `antidrift/internal/enforce`, `antidrift/internal/evidence`. Add/remove per the compiler (or `goimports -w`). Pay attention: `session.go` may still need `enforce`/`ai` for fields in the `Controller` struct (the struct itself stays in `session.go`), so do not blindly delete its imports — let the compiler decide.
|
||
|
||
- [ ] **Step 5: Green gate**
|
||
|
||
Run: `go build ./... && go vet ./... && go test -race ./internal/session/ ./internal/web/`
|
||
Expected: all PASS.
|
||
|
||
- [ ] **Step 6: Commit**
|
||
|
||
```bash
|
||
git add internal/session/drift.go internal/session/session.go
|
||
git commit -m "Split drift/nudge/enforcement into drift.go
|
||
|
||
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 5: Extract `roles.go` (file split, no logic change)
|
||
|
||
This isolates the four async roles so Phase 2's consolidation is a clean diff. `session.go` becomes the remainder (core controller + lifecycle).
|
||
|
||
**Files:**
|
||
- Create: `internal/session/roles.go`
|
||
- Modify: `internal/session/session.go`
|
||
|
||
- [ ] **Step 1: Create the new file**
|
||
|
||
Create `internal/session/roles.go`:
|
||
|
||
```go
|
||
package session
|
||
```
|
||
|
||
- [ ] **Step 2: Move the consts verbatim**
|
||
|
||
Cut these from `session.go` into `roles.go`:
|
||
|
||
- `const coachTimeout = 60 * time.Second` and the `const ( coachIdle ... coachPending ... coachReady ... coachError )` block
|
||
- `const tasksTimeout = 30 * time.Second` and the `const ( tasksIdle ... tasksReady ... tasksError )` block
|
||
- `const knowledgeTimeout = 10 * time.Second` and the `const ( knowledgeIdle ... knowledgeError )` block
|
||
- `const reflectionTimeout = 30 * time.Second`, `const reflectionHistoryN = 5`, `const reflectionTopBuckets = 3`, and the `const ( reflectionIdle ... reflectionAbsent )` block
|
||
|
||
- [ ] **Step 3: Move the methods/functions verbatim**
|
||
|
||
Cut these from `session.go` into `roles.go`:
|
||
|
||
- coach: `SetCoach`, `resetCoachLocked`, `composedGroundingLocked`, `RequestCoach`, `coachErrorMessage`
|
||
- tasks: `SetTasks`, `startTasksFetchLocked`
|
||
- knowledge: `SetKnowledge`, `SetKnowledgePath`, `startKnowledgeFetchLocked`
|
||
- reflection: `SetReviewer`, `startReflectionFetchLocked`, `buildReflectionFinishedLocked`, `buildReflectionHistory`
|
||
|
||
- [ ] **Step 4: Fix imports**
|
||
|
||
Run: `go build ./internal/session/`
|
||
`roles.go` is expected to need: `context`, `fmt`, `strings`, `time`, `antidrift/internal/ai`, `antidrift/internal/domain`, `antidrift/internal/knowledge`, `antidrift/internal/store`, `antidrift/internal/tasks`. Add/remove per the compiler (or `goimports -w`).
|
||
|
||
- [ ] **Step 5: Green gate + confirm the monolith shrank**
|
||
|
||
Run: `go build ./... && go vet ./... && go test -race ./internal/session/ ./internal/web/`
|
||
Expected: all PASS.
|
||
Run: `wc -l internal/session/*.go`
|
||
Expected: `session.go` is now well under ~450 lines; `views.go`/`roles.go`/`drift.go`/`stats.go` carry the rest.
|
||
|
||
- [ ] **Step 6: Commit**
|
||
|
||
```bash
|
||
git add internal/session/roles.go internal/session/session.go
|
||
git commit -m "Split async AI roles into roles.go
|
||
|
||
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 6: Add `runFetchAsync` and migrate the tasks role
|
||
|
||
Phase 2 begins. Introduce the helper and convert the first role. The helper owns only the goroutine dance; the role keeps its setup, stale guard, and apply branches.
|
||
|
||
**Files:**
|
||
- Modify: `internal/session/roles.go`
|
||
|
||
- [ ] **Step 1: Add the helper**
|
||
|
||
In `internal/session/roles.go`, add:
|
||
|
||
```go
|
||
// runFetchAsync launches a generation-guarded background fetch. The caller has
|
||
// already captured its dependencies and (for the *Locked callers) holds c.mu;
|
||
// this method only spawns the goroutine, which re-acquires the lock itself.
|
||
// 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). On a non-stale completion the controller notifies.
|
||
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()
|
||
}()
|
||
}
|
||
```
|
||
|
||
- [ ] **Step 2: Migrate `startTasksFetchLocked`**
|
||
|
||
Replace the body of `startTasksFetchLocked` (the `go func() { ... }()` block) so the function reads exactly:
|
||
|
||
```go
|
||
func (c *Controller) startTasksFetchLocked() {
|
||
c.tasksList = nil
|
||
if c.tasksProvider == nil {
|
||
c.tasksStatus = tasksIdle
|
||
return
|
||
}
|
||
c.tasksGen++
|
||
gen := c.tasksGen
|
||
c.tasksStatus = tasksPending
|
||
provider := c.tasksProvider
|
||
var list []tasks.Task
|
||
var err error
|
||
c.runFetchAsync(tasksTimeout,
|
||
func(ctx context.Context) { list, err = provider.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
|
||
}
|
||
})
|
||
}
|
||
```
|
||
|
||
- [ ] **Step 3: Targeted tests**
|
||
|
||
Run: `go test -race ./internal/session/ -run 'Tasks' -v`
|
||
Expected: PASS — including `TestEnterPlanningFetchesTasks`, `TestTasksFetchError`, `TestNoProviderNoTasksView`, `TestTasksViewAbsentOutsidePlanning`, `TestStaleTasksFetchDiscardedAfterLeavingPlanning`.
|
||
|
||
- [ ] **Step 4: Full green gate**
|
||
|
||
Run: `go build ./... && go vet ./... && go test -race ./internal/session/ ./internal/web/`
|
||
Expected: all PASS.
|
||
|
||
- [ ] **Step 5: Commit**
|
||
|
||
```bash
|
||
git add internal/session/roles.go
|
||
git commit -m "Add runFetchAsync helper and migrate the tasks fetch
|
||
|
||
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 7: Migrate the knowledge role
|
||
|
||
**Files:**
|
||
- Modify: `internal/session/roles.go`
|
||
|
||
- [ ] **Step 1: Migrate `startKnowledgeFetchLocked`**
|
||
|
||
Replace the `go func() { ... }()` block so the function reads exactly (note the three-branch apply and the `knowledgePath` write-back are preserved verbatim, now inside the `apply` closure):
|
||
|
||
```go
|
||
func (c *Controller) startKnowledgeFetchLocked() {
|
||
c.knowledgeText = ""
|
||
c.knowledgeChars = 0
|
||
if c.knowledgeSrc == nil {
|
||
c.knowledgeStatus = knowledgeIdle
|
||
return
|
||
}
|
||
c.knowledgeGen++
|
||
gen := c.knowledgeGen
|
||
c.knowledgeStatus = knowledgePending
|
||
src := c.knowledgeSrc
|
||
path := c.knowledgePath
|
||
var prof knowledge.Profile
|
||
var err error
|
||
c.runFetchAsync(knowledgeTimeout,
|
||
func(ctx context.Context) { prof, err = src.Load(ctx, path) },
|
||
func() bool { return gen != c.knowledgeGen || c.runtimeState != domain.RuntimePlanning },
|
||
func() {
|
||
if err != nil {
|
||
c.knowledgeStatus = knowledgeError
|
||
c.knowledgeText = ""
|
||
c.knowledgeChars = 0
|
||
if prof.Path != "" {
|
||
c.knowledgePath = prof.Path
|
||
}
|
||
} else if prof.Text == "" {
|
||
c.knowledgeStatus = knowledgeAbsent
|
||
c.knowledgeText = ""
|
||
c.knowledgeChars = 0
|
||
c.knowledgePath = prof.Path
|
||
} else {
|
||
c.knowledgeStatus = knowledgeReady
|
||
c.knowledgeText = prof.Text
|
||
c.knowledgeChars = len(prof.Text)
|
||
c.knowledgePath = prof.Path
|
||
}
|
||
})
|
||
}
|
||
```
|
||
|
||
- [ ] **Step 2: Targeted tests**
|
||
|
||
Run: `go test -race ./internal/session/ -run 'Knowledge' -v`
|
||
Expected: PASS — including `TestEnterPlanningLoadsKnowledge`, `TestKnowledgeAbsentWhenEmptyText`, `TestKnowledgeLoadError`, `TestNoSourceNoKnowledgeView`, `TestKnowledgeViewAbsentOutsidePlanning`, `TestStaleKnowledgeFetchDiscardedAfterLeavingPlanning` (from Task 1).
|
||
|
||
- [ ] **Step 3: Full green gate**
|
||
|
||
Run: `go build ./... && go vet ./... && go test -race ./internal/session/ ./internal/web/`
|
||
Expected: all PASS.
|
||
|
||
- [ ] **Step 4: Commit**
|
||
|
||
```bash
|
||
git add internal/session/roles.go
|
||
git commit -m "Migrate the knowledge fetch onto runFetchAsync
|
||
|
||
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 8: Migrate the coach role
|
||
|
||
`RequestCoach` is the request-triggered entry point: it manages its own lock and notifies the pending state **before** launching. Keep that pre-launch unlock+notify; only the trailing `go func() { ... }()` is replaced.
|
||
|
||
**Files:**
|
||
- Modify: `internal/session/roles.go`
|
||
|
||
- [ ] **Step 1: Migrate `RequestCoach`**
|
||
|
||
Replace the trailing goroutine. The function must read exactly:
|
||
|
||
```go
|
||
func (c *Controller) RequestCoach(intent string) error {
|
||
c.mu.Lock()
|
||
if c.runtimeState != domain.RuntimePlanning {
|
||
c.mu.Unlock()
|
||
return ErrNotPlanning
|
||
}
|
||
if c.coach == nil {
|
||
c.coachStatus = coachError
|
||
c.coachErr = "coach unavailable"
|
||
c.coachProposal = nil
|
||
c.mu.Unlock()
|
||
c.notify()
|
||
return nil
|
||
}
|
||
c.coachGen++
|
||
gen := c.coachGen
|
||
c.coachStatus = coachPending
|
||
c.coachErr = ""
|
||
c.coachProposal = nil
|
||
coach := c.coach
|
||
grounding := c.composedGroundingLocked()
|
||
c.mu.Unlock()
|
||
c.notify()
|
||
|
||
var prop ai.Proposal
|
||
var err error
|
||
c.runFetchAsync(coachTimeout,
|
||
func(ctx context.Context) { prop, err = coach.Coach(ctx, intent, grounding) },
|
||
func() bool { return gen != c.coachGen || c.runtimeState != domain.RuntimePlanning },
|
||
func() {
|
||
if err != nil {
|
||
c.coachStatus = coachError
|
||
c.coachErr = coachErrorMessage(err)
|
||
c.coachProposal = nil
|
||
} else {
|
||
c.coachStatus = coachReady
|
||
c.coachProposal = &prop
|
||
c.coachErr = ""
|
||
}
|
||
})
|
||
return nil
|
||
}
|
||
```
|
||
|
||
Note: `runFetchAsync` is called here **after** `c.mu.Unlock()`. That is correct — the helper only spawns the goroutine and touches no `c` field before the goroutine re-locks, so holding the lock is not required.
|
||
|
||
- [ ] **Step 2: Targeted tests**
|
||
|
||
Run: `go test -race ./internal/session/ -run 'Coach' -v`
|
||
Expected: PASS — including `TestRequestCoachReady`, `TestRequestCoachError`, `TestRequestCoachUnavailable`, `TestRequestCoachWrongState`, `TestRequestCoachStaleResultDiscarded`, `TestCoachReceivesCachedGrounding`, `TestLeavingPlanningClearsCoach`, `TestCarryForwardGroundsNextCoach`.
|
||
|
||
- [ ] **Step 3: Full green gate**
|
||
|
||
Run: `go build ./... && go vet ./... && go test -race ./internal/session/ ./internal/web/`
|
||
Expected: all PASS.
|
||
|
||
- [ ] **Step 4: Commit**
|
||
|
||
```bash
|
||
git add internal/session/roles.go
|
||
git commit -m "Migrate the coach fetch onto runFetchAsync
|
||
|
||
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 9: Migrate the reflection role
|
||
|
||
Reflection is the subtle one: its stale guard is **gen-only** (no Planning gate — the carry-forward must survive `End` before the reviewer returns), it reads `history` **synchronously under the lock before** the goroutine (a happens-before requirement against `End`'s audit-chain append), and its apply calls `persistLocked`. All three must be preserved.
|
||
|
||
**Files:**
|
||
- Modify: `internal/session/roles.go`
|
||
|
||
- [ ] **Step 1: Migrate `startReflectionFetchLocked`**
|
||
|
||
Replace the trailing `go func() { ... }()` block. The function must read exactly (the synchronous `history := buildReflectionHistory(c.auditPath)` stays **outside** the closures, before the `runFetchAsync` call; `persistLocked` moves **into** the `apply` closure):
|
||
|
||
```go
|
||
func (c *Controller) startReflectionFetchLocked() {
|
||
c.reflectionRecap = ""
|
||
c.carryForward = ""
|
||
if c.reviewer == nil {
|
||
c.reflectionStatus = reflectionIdle
|
||
return
|
||
}
|
||
c.reflectionGen++
|
||
gen := c.reflectionGen
|
||
c.reflectionStatus = reflectionPending
|
||
reviewer := c.reviewer
|
||
finished := c.buildReflectionFinishedLocked()
|
||
// Read the history synchronously, here under the lock, on purpose: it must
|
||
// happen-before End appends the just-finished session to the audit chain, so
|
||
// that session is excluded from "recent history" and not double-counted (it
|
||
// is already carried in `finished`). Moving this into the goroutine would
|
||
// race with End's append and reintroduce that double-count. The read is
|
||
// bounded to reflectionHistoryN summaries and runs once per Review entry, not
|
||
// on any hot path.
|
||
history := buildReflectionHistory(c.auditPath)
|
||
var refl ai.Reflection
|
||
var err error
|
||
c.runFetchAsync(reflectionTimeout,
|
||
func(ctx context.Context) { refl, err = reviewer.Review(ctx, finished, history) },
|
||
func() bool { return gen != c.reflectionGen },
|
||
func() {
|
||
if err != nil || strings.TrimSpace(refl.Recap) == "" {
|
||
c.reflectionStatus = reflectionAbsent
|
||
c.reflectionRecap = ""
|
||
c.carryForward = ""
|
||
} else {
|
||
c.reflectionStatus = reflectionReady
|
||
c.reflectionRecap = refl.Recap
|
||
c.carryForward = refl.CarryForward
|
||
}
|
||
_ = c.persistLocked()
|
||
})
|
||
}
|
||
```
|
||
|
||
Confirm the reviewer's return type is `ai.Reflection` (grep `internal/ai` for `func (b *Backend) Review`); if the type name differs, match it in the `var refl` declaration.
|
||
|
||
- [ ] **Step 2: Targeted tests**
|
||
|
||
Run: `go test -race ./internal/session/ -run 'Reflection|CarryForward' -v`
|
||
Expected: PASS — including `TestReflectionFetchedOnReview`, `TestNoReviewerYieldsIdleReflection`, `TestCarryForwardGroundsNextCoach`, `TestReflectionStaleResultDiscarded`, `TestCarryForwardSurvivesRestart`.
|
||
|
||
- [ ] **Step 3: Full green gate**
|
||
|
||
Run: `go build ./... && go vet ./... && go test -race ./internal/session/ ./internal/web/`
|
||
Expected: all PASS.
|
||
|
||
- [ ] **Step 4: Commit**
|
||
|
||
```bash
|
||
git add internal/session/roles.go
|
||
git commit -m "Migrate the reflection fetch onto runFetchAsync
|
||
|
||
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>"
|
||
```
|
||
|
||
---
|
||
|
||
## Task 10: Final verification
|
||
|
||
**Files:** none (verification only).
|
||
|
||
- [ ] **Step 1: Confirm no inline async-fetch goroutines remain**
|
||
|
||
Run: `grep -n 'go func()' internal/session/*.go`
|
||
Expected: the only matches are inside `runFetchAsync` (in `roles.go`) and any pre-existing goroutines in `drift.go`/elsewhere that are **not** the four migrated role fetches. The coach/tasks/knowledge/reflection fetches must no longer contain their own `go func()` — they go through `runFetchAsync`.
|
||
|
||
- [ ] **Step 2: Confirm the split**
|
||
|
||
Run: `wc -l internal/session/*.go`
|
||
Expected: five focused files (`session.go`, `views.go`, `roles.go`, `drift.go`, `stats.go`), none dominating; the old 1278-line monolith is gone.
|
||
|
||
- [ ] **Step 3: Full suite under the race detector**
|
||
|
||
Run: `go build ./... && go vet ./... && go test -race ./...`
|
||
Expected: all packages PASS, no race warnings.
|
||
|
||
- [ ] **Step 4: Confirm behavior contract**
|
||
|
||
Confirm via `git log --oneline` that the milestone is a series of small commits, each green, with no change to any exported signature (spot-check: `git diff <first-m9-commit>^..HEAD -- internal/session/session.go internal/web internal/store cmd` shows no signature/behavior changes outside the moved/▸consolidated session code, and no `.go` file outside `internal/session/` was modified except as already committed in prior milestones).
|
||
|
||
---
|
||
|
||
## Self-review notes (for the executor)
|
||
|
||
- Every task is **green-to-green**; if any `go test -race` goes red, stop and fix before committing — a red gate means a real behavior change slipped in.
|
||
- The risky preservation points, all called out in their tasks: reflection's **gen-only** stale guard, its **synchronous history read** before the goroutine, and its **`persistLocked`** in apply; coach's **pre-launch unlock+notify**; knowledge's **three-branch apply + `knowledgePath` write-back**.
|
||
- No exported symbol is renamed or moved out of `package session`; `web_test.go`'s cross-package use of `AllowedClassesForTest`/`EnforcementLevelForTest` keeps working because those stay in `session.go` as regular (non-`_test.go`) methods.
|