Track faithful-reflection implementation plan

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-01 21:56:48 -04:00
parent ca5e76baf5
commit b447432df0
1 changed files with 458 additions and 0 deletions
@@ -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.