diff --git a/docs/superpowers/plans/2026-06-01-faithful-reflection.md b/docs/superpowers/plans/2026-06-01-faithful-reflection.md new file mode 100644 index 0000000..7c29c10 --- /dev/null +++ b/docs/superpowers/plans/2026-06-01-faithful-reflection.md @@ -0,0 +1,458 @@ +# Faithful Reflection (on/off-task split) 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:** Make the session-end reflection block report how much time was on-task vs off-task, so the AI reviewer stops writing charitable recaps. + +**Architecture:** Each window segment is classified at the moment it is credited, reading the live `driftStatus` that is already correct at that instant (in `RecordWindow`, `applyEvent` credits the just-ended segment *before* `evaluateDriftLocked` reclassifies for the new window). The split is accumulated in two new per-bucket maps plus one scalar on the in-memory `EvidenceStats`; the existing `Buckets` total map is untouched. `buildReflectionFinishedLocked` then renders on/off/unclassified totals plus a top-N on-task list and a top-N off-task list. + +**Tech Stack:** Go, standard library only. Tests use the existing `testing` package with the package-local `fakeClock`, `fakeJudge`, `snap`/`obs`, `startActive`, and `waitDriftStatus` helpers in `internal/session/session_test.go`. + +**Spec:** `docs/superpowers/specs/2026-06-01-faithful-reflection-design.md` + +--- + +## File Structure + +- `internal/session/stats.go` — `EvidenceStats` gains the split fields; new `creditLocked` helper centralizes crediting; `applyEvent` routes through it. Allocation of the new maps added to both `replayStats` branches. +- `internal/session/session.go` — allocate the split maps in `StartManualCommitment`; route the end-of-session flush in `enterReview` through `creditLocked`. +- `internal/session/roles.go` — `buildReflectionFinishedLocked` renders totals + split top lists; small `writeBucketList` / `sumDurations` helpers added alongside it. +- `internal/session/session_test.go` — three tests: `creditLocked` unit mapping, rendering (incl. empty-list omission), and end-to-end credit-time wiring. + +Three tasks, each self-contained and committed independently: + +1. **Split accounting** — fields, allocation, `creditLocked`, route both credit sites. (Driven by the `creditLocked` unit test.) +2. **Reflection rendering** — rewrite `buildReflectionFinishedLocked`. (Driven by a rendering unit test, including empty-off-task omission.) +3. **End-to-end faithfulness** — a test-only task proving the live drift status reaches the right bucket through a real `RecordWindow` sequence. + +--- + +## Task 1: Split accounting in EvidenceStats + +Add the on/off/unclassified accumulators and a single crediting helper, then route the two existing credit sites through it. The existing `Buckets` total map is preserved unchanged (it still feeds the live evidence panel and the persisted history summary). + +**Files:** +- Modify: `internal/session/stats.go` (struct fields ~13-22, `applyEvent` ~27-39, `replayStats` ~42-60) +- Modify: `internal/session/session.go` (`StartManualCommitment` stats init ~248-252, `enterReview` flush ~280-283) +- Test: `internal/session/session_test.go` (new test appended) + +- [ ] **Step 1: Write the failing test** + +Append to `internal/session/session_test.go`: + +```go +func TestCreditLockedSplitsByDriftStatus(t *testing.T) { + c, _ := newTestController(t) + c.stats = &EvidenceStats{ + Buckets: map[bucketKey]time.Duration{}, + OnTask: map[bucketKey]time.Duration{}, + OffTask: map[bucketKey]time.Duration{}, + } + k := bucketKey{Class: "code", Title: "main.go"} + + c.driftStatus = driftOnTask + c.creditLocked(k, 10*time.Second) + c.driftStatus = driftDrifting + c.creditLocked(k, 5*time.Second) + c.driftStatus = driftIdle + c.creditLocked(k, 3*time.Second) + c.driftStatus = driftPending + c.creditLocked(k, 2*time.Second) + + if got := c.stats.OnTask[k]; got != 10*time.Second { + t.Errorf("OnTask = %v, want 10s", got) + } + if got := c.stats.OffTask[k]; got != 5*time.Second { + t.Errorf("OffTask = %v, want 5s", got) + } + if got := c.stats.unclassified; got != 5*time.Second { // 3s idle + 2s pending + t.Errorf("unclassified = %v, want 5s", got) + } + if got := c.stats.Buckets[k]; got != 20*time.Second { // total of all four + t.Errorf("Buckets total = %v, want 20s", got) + } +} +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `go test ./internal/session/ -run TestCreditLockedSplitsByDriftStatus` +Expected: FAIL — compile error: `c.stats.OnTask` / `c.stats.OffTask` / `c.stats.unclassified` undefined and `c.creditLocked` undefined. + +- [ ] **Step 3: Add the split fields to `EvidenceStats`** + +In `internal/session/stats.go`, replace the struct (currently lines 12-22): + +```go +// EvidenceStats is the in-memory accounting for the current session only. +type EvidenceStats struct { + SessionID string + StartedUnix int64 + Buckets map[bucketKey]time.Duration // total time per bucket (unchanged) + OnTask map[bucketKey]time.Duration // on-task portion per bucket + OffTask map[bucketKey]time.Duration // off-task portion per bucket + unclassified time.Duration // idle/pending time; aggregate only + + SwitchCount int + Current evidence.WindowSnapshot + lastFocusAt time.Time + lastKey bucketKey + hasLast bool +} +``` + +- [ ] **Step 4: Add `creditLocked` and route `applyEvent` through it** + +In `internal/session/stats.go`, change the credit line in `applyEvent` (currently lines 28-30) and add the helper. The `applyEvent` body becomes: + +```go +func (c *Controller) applyEvent(now time.Time, snap evidence.WindowSnapshot) { + if c.stats.hasLast { + c.creditLocked(c.stats.lastKey, now.Sub(c.stats.lastFocusAt)) + } + newKey := keyFor(snap) + if c.stats.hasLast && newKey != c.stats.lastKey { + c.stats.SwitchCount++ + } + c.stats.lastKey = newKey + c.stats.lastFocusAt = now + c.stats.hasLast = true + c.stats.Current = snap +} + +// creditLocked credits duration d to bucket k: always to the running total, and +// to the on/off/unclassified split per the live drift status — which, at every +// credit site, is the classification of the segment being credited (applyEvent +// runs before evaluateDriftLocked reclassifies; the end-of-session flush runs +// before the state transition). idle/pending route to unclassified: honest, never +// falsely on-task. Caller holds mu. +func (c *Controller) creditLocked(k bucketKey, d time.Duration) { + c.stats.Buckets[k] += d + switch c.driftStatus { + case driftOnTask: + c.stats.OnTask[k] += d + case driftDrifting: + c.stats.OffTask[k] += d + default: // driftIdle, driftPending + c.stats.unclassified += d + } +} +``` + +- [ ] **Step 5: Allocate the split maps everywhere `Buckets` is allocated** + +There are three allocation sites. The maps must be non-nil before `creditLocked` runs. + +In `internal/session/session.go`, `StartManualCommitment` (currently lines 248-252): + +```go + c.stats = &EvidenceStats{ + SessionID: sessionID, + StartedUnix: now.Unix(), + Buckets: map[bucketKey]time.Duration{}, + OnTask: map[bucketKey]time.Duration{}, + OffTask: map[bucketKey]time.Duration{}, + } +``` + +In `internal/session/stats.go`, `replayStats` — the empty/error branch (currently lines 45-50): + +```go + c.stats = &EvidenceStats{ + SessionID: sessionID, + StartedUnix: c.clock().Unix(), + Buckets: map[bucketKey]time.Duration{}, + OnTask: map[bucketKey]time.Duration{}, + OffTask: map[bucketKey]time.Duration{}, + } + return +``` + +and the replay branch (currently lines 52-56): + +```go + c.stats = &EvidenceStats{ + SessionID: sessionID, + StartedUnix: events[0].AtUnixMillis / 1000, + Buckets: map[bucketKey]time.Duration{}, + OnTask: map[bucketKey]time.Duration{}, + OffTask: map[bucketKey]time.Duration{}, + } +``` + +- [ ] **Step 6: Route the end-of-session flush through `creditLocked`** + +In `internal/session/session.go`, `enterReview` flush (currently lines 280-283): + +```go + // Flush the final open segment, then freeze accounting. + if c.stats != nil && c.stats.hasLast { + c.creditLocked(c.stats.lastKey, c.clock().Sub(c.stats.lastFocusAt)) + c.stats.hasLast = false + } +``` + +- [ ] **Step 7: Run the new test and the full session suite** + +Run: `go test ./internal/session/ -run TestCreditLockedSplitsByDriftStatus` +Expected: PASS + +Run: `go test ./internal/session/` +Expected: PASS — all existing tests (bucket totals, crash replay, audit summary) unaffected, since `Buckets` is unchanged. + +- [ ] **Step 8: Commit** + +```bash +git add internal/session/stats.go internal/session/session.go internal/session/session_test.go +git commit -m "Split session time into on/off/unclassified buckets" +``` + +--- + +## Task 2: Render the split in the reflection block + +Rewrite `buildReflectionFinishedLocked` to emit on/off/unclassified totals followed by a top-N on-task list and a top-N off-task list, reusing the existing `bucketViews` sorter. An empty list (and its label) is omitted. + +**Files:** +- Modify: `internal/session/roles.go` (`buildReflectionFinishedLocked` ~269-292) +- Test: `internal/session/session_test.go` (new test appended) + +- [ ] **Step 1: Write the failing test** + +Append to `internal/session/session_test.go`. This test sets the split maps directly on a started session, so it is deterministic and independent of drift timing: + +```go +func TestReflectionBlockShowsOnOffSplit(t *testing.T) { + c, _ := newTestController(t) + startActive(t, c, nil) // sets commitment ("write report" / "report drafted") and stats + + c.stats.SwitchCount = 2 + c.stats.OnTask = map[bucketKey]time.Duration{ + {Class: "code", Title: "main.go"}: 20 * time.Minute, + } + c.stats.OffTask = map[bucketKey]time.Duration{ + {Class: "firefox", Title: "YouTube"}: 8 * time.Minute, + {Class: "firefox", Title: "Reddit"}: 4 * time.Minute, + } + c.stats.unclassified = 5 * time.Minute + c.outcomePending = "completed" + + c.mu.Lock() + got := c.buildReflectionFinishedLocked() + c.mu.Unlock() + + want := "Next action: write report\n" + + "Success condition: report drafted\n" + + "Outcome: completed\n" + + "On-task 20m / Off-task 12m / Unclassified 5m\n" + + "Context switches: 2\n" + + "On-task:\n" + + "- code · main.go: 20m\n" + + "Off-task:\n" + + "- firefox · YouTube: 8m\n" + + "- firefox · Reddit: 4m" + if got != want { + t.Errorf("reflection block mismatch:\n--- got ---\n%s\n--- want ---\n%s", got, want) + } +} + +func TestReflectionBlockOmitsEmptyOffTaskList(t *testing.T) { + c, _ := newTestController(t) + startActive(t, c, nil) + + c.stats.OnTask = map[bucketKey]time.Duration{ + {Class: "code", Title: "main.go"}: 20 * time.Minute, + } + c.stats.OffTask = map[bucketKey]time.Duration{} // none + c.stats.unclassified = 0 + c.outcomePending = "completed" + + c.mu.Lock() + got := c.buildReflectionFinishedLocked() + c.mu.Unlock() + + if strings.Contains(got, "Off-task:") { + t.Errorf("fully on-task session must omit the Off-task list, got:\n%s", got) + } + if !strings.Contains(got, "On-task 20m / Off-task 0m / Unclassified 0m") { + t.Errorf("totals line missing or wrong, got:\n%s", got) + } +} +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `go test ./internal/session/ -run TestReflectionBlock` +Expected: FAIL — the current block renders top buckets from `c.stats.Buckets` (e.g. lines like `- · : 0m`) and has no `On-task NNm / Off-task NNm` totals line, so the string comparison fails and `Off-task:` is absent for the wrong reason in the second test (verify the first test fails on the totals line). + +- [ ] **Step 3: Rewrite `buildReflectionFinishedLocked`** + +In `internal/session/roles.go`, replace the function (currently lines 265-292) with: + +```go +// buildReflectionFinishedLocked renders the just-finished session as a compact +// block for the reviewer: the commitment, the outcome, on/off/unclassified time +// totals, and a top-N on-task list and top-N off-task list. The split fields are +// populated live by creditLocked. Caller holds mu; c.stats/c.commitment are still +// set (End clears them, but enterReview runs before End). +func (c *Controller) buildReflectionFinishedLocked() string { + var na, sc string + if c.commitment != nil { + na, sc = c.commitment.NextAction, c.commitment.SuccessCondition + } + outcome := c.outcomePending + if outcome == "" { + outcome = "completed" + } + var b strings.Builder + fmt.Fprintf(&b, "Next action: %s\n", na) + fmt.Fprintf(&b, "Success condition: %s\n", sc) + fmt.Fprintf(&b, "Outcome: %s\n", outcome) + if c.stats != nil { + onMin := int64(sumDurations(c.stats.OnTask).Seconds()) / 60 + offMin := int64(sumDurations(c.stats.OffTask).Seconds()) / 60 + unclMin := int64(c.stats.unclassified.Seconds()) / 60 + fmt.Fprintf(&b, "On-task %dm / Off-task %dm / Unclassified %dm\n", onMin, offMin, unclMin) + fmt.Fprintf(&b, "Context switches: %d\n", c.stats.SwitchCount) + writeBucketList(&b, "On-task", c.stats.OnTask) + writeBucketList(&b, "Off-task", c.stats.OffTask) + } + return strings.TrimRight(b.String(), "\n") +} + +// writeBucketList renders a labeled, time-descending list of buckets capped at +// reflectionTopBuckets. It writes nothing — not even the label — when the map is +// empty, so a single-sided session shows only the list that has time. +func writeBucketList(b *strings.Builder, label string, m map[bucketKey]time.Duration) { + views := bucketViews(m) + if len(views) == 0 { + return + } + fmt.Fprintf(b, "%s:\n", label) + for i, bv := range views { + if i >= reflectionTopBuckets { + break + } + fmt.Fprintf(b, "- %s · %s: %dm\n", bv.Class, bv.Title, bv.Seconds/60) + } +} + +// sumDurations totals the durations in a bucket map. +func sumDurations(m map[bucketKey]time.Duration) time.Duration { + var total time.Duration + for _, d := range m { + total += d + } + return total +} +``` + +Note: `bucketViews` (in `views.go`) already returns a `[]BucketView` sorted descending by seconds, so it serves both lists. The old loop over `bucketViews(c.stats.Buckets)` is gone; `c.stats.Buckets` is no longer read here (it remains in use by `views.go` and the audit summary). + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `go test ./internal/session/ -run TestReflectionBlock` +Expected: PASS + +Run: `go test ./internal/session/` +Expected: PASS — the existing reflection tests (`TestReflectionFetchedOnReview` etc.) assert on the reviewer's `Recap`, not the finished-block text, so they are unaffected. + +- [ ] **Step 5: Commit** + +```bash +git add internal/session/roles.go internal/session/session_test.go +git commit -m "Render on/off-task split in the reflection block" +``` + +--- + +## Task 3: End-to-end faithfulness through RecordWindow + +A test-only task: prove that the live drift status produced by the real drift pipeline reaches the correct split bucket. This is the crux of "faithful" — it exercises the credit-time ordering (`applyEvent` before `evaluateDriftLocked`) and the async drift judge, which the direct unit tests in Tasks 1-2 deliberately bypass. + +**Files:** +- Test: `internal/session/session_test.go` (new test appended) + +- [ ] **Step 1: Write the test** + +The sequence: the seed segment accrues while `idle` (unclassified, because the seed never runs the drift pipeline), then on-task code time accrues via the local allowlist match, then off-task firefox time accrues after the async judge returns drifting. `waitDriftStatus` ensures the verdict has landed before the segment that follows is credited, so the classification is deterministic despite the async judge. Durations are in minutes so the integer-minute rendering is exact and the two off-task buckets differ (no sort tie). + +```go +func TestRecordWindowCreditsSplitFaithfully(t *testing.T) { + c, _ := newTestController(t) + clk := &fakeClock{now: time.Unix(1000, 0)} + c.SetClock(clk.fn()) + c.SetDriftJudge(&fakeJudge{verdict: ai.Verdict{OnTask: false, Reason: "off"}}) + + c.RecordWindow(snap("code", "main.go")) // latest window before start + startActive(t, c, []string{"code"}) // seeds code/main.go at t=1000, driftStatus idle + + clk.advance(5 * time.Minute) + c.RecordWindow(snap("code", "main.go")) // credits 5m to code/main.go while idle -> unclassified; now on-task (local match) + clk.advance(10 * time.Minute) + c.RecordWindow(snap("code", "main.go")) // credits 10m on-task + clk.advance(10 * time.Minute) + c.RecordWindow(snap("firefox", "YouTube")) // credits 10m on-task (total 20m); firefox -> judge (pending) + waitDriftStatus(t, c, "drifting") // wait for the async verdict before crediting the firefox segment + + clk.advance(8 * time.Minute) + c.RecordWindow(snap("firefox", "Reddit")) // credits 8m to firefox/YouTube while drifting -> off-task; cache keeps drifting + clk.advance(4 * time.Minute) + if err := c.Complete(); err != nil { // flush: credits 4m to firefox/Reddit while drifting -> off-task + t.Fatalf("complete: %v", err) + } + + c.mu.Lock() + got := c.buildReflectionFinishedLocked() + c.mu.Unlock() + + want := "Next action: write report\n" + + "Success condition: report drafted\n" + + "Outcome: completed\n" + + "On-task 20m / Off-task 12m / Unclassified 5m\n" + + "Context switches: 2\n" + + "On-task:\n" + + "- code · main.go: 20m\n" + + "Off-task:\n" + + "- firefox · YouTube: 8m\n" + + "- firefox · Reddit: 4m" + if got != want { + t.Errorf("faithful split mismatch:\n--- got ---\n%s\n--- want ---\n%s", got, want) + } +} +``` + +- [ ] **Step 2: Run the test** + +Run: `go test ./internal/session/ -run TestRecordWindowCreditsSplitFaithfully` +Expected: PASS — Tasks 1 and 2 already supply the behavior; this test asserts the wiring end-to-end. + +If it FAILS on the `Unclassified 5m` portion, that confirms the credit-time ordering is being read correctly (the seed segment is genuinely idle); do not "fix" it by pre-classifying the seed — that unclassified slice is the honest result and is asserted on purpose. + +- [ ] **Step 3: Run the full suite with the race detector** + +Run: `go test -race ./internal/session/` +Expected: PASS, no race warnings. (The test spawns the real drift-judge goroutine; `waitDriftStatus` synchronizes before the next credit, and `buildReflectionFinishedLocked` is read under `c.mu`.) + +- [ ] **Step 4: Commit** + +```bash +git add internal/session/session_test.go +git commit -m "Test faithful on/off-task crediting through RecordWindow" +``` + +--- + +## Final verification + +After all three tasks: + +- [ ] Run: `go build ./...` — Expected: success. +- [ ] Run: `go vet ./...` — Expected: no diagnostics. +- [ ] Run: `go test ./...` — Expected: all packages PASS. + +## Out of scope (do not implement) + +- Persisting the split to the focus log or audit/history summary (no cross-restart reconstruction). +- Per-bucket unclassified breakdown (only the aggregate is shown). +- Any change to the live evidence panel (`views.go`), the drift/nudge pipeline, the reviewer prompt contract beyond the finished-block text, or the web UI.