docs: add Session god-object decomposition design sketch (#33)

* docs: remove generic LLM boilerplate ai_passage.md

ai_passage.md was a 53-line, ~1000-word essay on the history and
ethics of AI in general — entirely unrelated to the hawk project,
no README/AGENTS.md/CHANGELOG.md reference to it. It looks like
LLM-generated filler committed in '99261ca Fix CI formatting and
toolchain hygiene' to satisfy a 'must have an essay' requirement
that no longer applies. Untrack and delete.

* fix(engine): respect ctx cancel on stream retry, surface self-review revert failure

Three related fixes in the hawk agent loop:

1. **stream.go Stream retry no longer blocks ctx cancellation.**
   Previously: `time.Sleep(time.Duration(streamAttempt+1) * time.Second)` — a
   user-initiated cancel during a reasoning-only retry's backoff was ignored for
   up to 3s. Now uses a select with ctx.Done() so cancel is observed immediately,
   the in-flight stream is closed, and the loop exits with a structured error event.

2. **stream_tool_exec.go Self-review-before-apply now returns a hard error
   when the revert itself fails.** Previously: if the LLM said 'this diff is bad'
   and we tried to revert (os.Remove or os.WriteFile) and that syscall failed, the
   code only logged s.log.Warn and proceeded with the rejected diff still on disk.
   Now: capture the revert error, log it at Error level, and surface a hard tool
   error ('Self-review rejected the change AND the revert failed: <err>. Manual
   intervention required.') so the LLM can't continue building on top of code it
   just flagged as broken.

3. **internal/tool/tool.go now exports ReadOnlyTools + IsReadOnly() so the
   safe-concurrent allowlist has a single source of truth.** The map was previously
   duplicated in stream.go:716 and stream_tool_exec.go:29 with identical content
   — drift waiting to happen. Both call sites now go through tool.IsReadOnly(name),
   which canonicalises aliases (read/file_read, ls, etc.) before lookup. A new
   test (TestIsReadOnly, TestReadOnlyToolsSetContainsExpectedNames) locks the
   contract so a future removal of e.g. 'Read' from the allowlist fails CI in
   internal/tool/ rather than silently breaking concurrency classification in
   internal/engine/.

* docs: add Session god-object decomposition design sketch

hawk/internal/engine/session.go has 35 collaborators, 30 of them optional
*T pointer fields. The agentLoop is an 833-line function that reads ~25 of
these fields, creating a hidden web of dependencies. New contributors can't
tell which field is required vs optional vs diagnostic, and unit tests can't
construct a Session without wiring all 30 fields.

This is a design proposal (NOT yet implemented) for breaking Session into 6
cohesive sub-services:

  - ChatService        (150 LOC) — LLM transport, retry, circuit-breaker
  - MemoryService      (200 LOC) — yaad bridge, recall, sleeptime, skill distill
  - ToolService        (400 LOC) — registry, 15-stage post-call pipeline
  - PermissionService  (150 LOC) — guardian, approval, budget tracking
  - LifecycleService   (250 LOC) — self-improvement, beliefs, doom-loop
  - PersistenceService (300 LOC) — checkpoint, compaction, branching DAG

The proposed 7-PR migration sequence is documented in the doc. Phases 2-7
are each independent and additive; each gets its own focused test suite.
The 4 concrete fixes in the current PR (time.Sleep, ReadOnlyTools, AGENTS.md,
self-review revert) are independent of this refactor and ship ahead of it.

See: docs/session-decomposition.md (this file)

Author: Patel230

docs/session-decomposition.md

@@ -0,0 +1,256 @@
+# Session God-Object Decomposition — Design Sketch
+
+> Status: **DRAFT / NOT YET IMPLEMENTED**
+> Author: opencode session
+> Date: 2026-06-12
+> Scope: `hawk/internal/engine/session.go` (the 35-collaborator `Session` struct)
+
+## Problem
+
+`Session` at `hawk/internal/engine/session.go:42-141` is a god object:
+
+- 35 fields, ~30 of them `*T` pointer collaborators with optional behavior
+- Every new feature adds a field (Beliefs, Critic, Trajectory, Steer­ing, etc.)
+- Construction requires wiring 30+ fields; unit testing is impractical
+- `NewSessionWithClient` (lines 149-187) already has a 35-line constructor body
+- The `AgentLoop` 833-line function reads/writes ~25 of these fields, creating a hidden web of dependencies
+- New contributors can't tell which field is required vs optional vs diagnostic
+
+## Goal
+
+Break `Session` into ~6 cohesive sub-services, each with:
+- a single responsibility
+- an interface (so tests can use stubs)
+- a clear lifecycle (created once in `NewSessionWithClient`, passed to `agentLoop`)
+
+The `agentLoop` should consume these sub-services as named dependencies — no implicit `s.Beliefs.Size()` reach-throughs.
+
+## Proposed Decomposition
+
+### 1. `ChatService` — owns the LLM transport
+
+**Owns:** `client ChatClient`, `provider string`, `model string`, `apiKeys map[string]string`, `Router *modelPkg.Router`, `DeploymentRouting bool`, `RateLimiter *ratelimit.Limiter`, `OutputSchema string`, `GLMThinkingEnabled *bool`, `ContCfg types.ContinuationConfig`, `ChatOptions{}` builder.
+
+**Methods:**
+- `BuildOptions(systemPrompt string, tools []client.EyrieTool, activeModel string, maxTokens int) types.ChatOptions`
+- `Chat(ctx, msgs, opts) (*types.EyrieResponse, error)` — non-streaming, used by background goroutines
+- `Stream(ctx, msgs, opts) (*types.StreamResult, error)` — with retry, circuit-breaker record, rate-limit wait
+- `RecordSuccess(latency time.Duration)` / `RecordFailure(err error)`
+- `BuildContinuationConfig() types.ContinuationConfig`
+
+**File:** `hawk/internal/engine/chat_service.go` (~150 LOC)
+
+### 2. `MemoryService` — owns yaad bridge + recall/remember
+
+**Owns:** `Memory MemoryRecaller`, `YaadBridge *memory.YaadBridge`, `EnhancedMemory *memory.EnhancedMemoryManager`, `SkillDistiller *memory.SkillDistiller`, `Sleeptime *memory.SleeptimeAgent`, `Activity *memory.ActivityTracker`, `AgentsAccum *prompts.AgentsAccum`, `FewShotStore *FewShotStore`, `AdaptivePrompt *AdaptivePrompt`.
+
+**Methods:**
+- `RecallContext(ctx, lastUserMsg string, budget int) (string, error)` — unifies yaad + few-shot + agents-accum
+- `Remember(ctx, content, category string)` — wraps memory.Remember
+- `RunSleeptimeConsolidation(ctx, provider ChatService, messages []types.EyrieMessage)` — background
+- `RunSkillDistillation(ctx, provider ChatService, ...)` — background
+- `RecordFeedback(ctx, content string, action string)` — wraps EnhancedMemory feedback
+- `ShouldRemember(text string) bool` — heuristic for auto-remember
+
+**File:** `hawk/internal/engine/memory_service.go` (~200 LOC)
+
+### 3. `ToolService` — owns the registry + tool execution
+
+**Owns:** `registry *tool.Registry`, `ContainerExecutor tool.ContainerExecutor`, `ContainerRequired bool`, `Snapshots SnapshotTracker`, `BackgroundManager *tool.BackgroundAgentManager`, `AllowedDirs []string`, `Protected PathProtector`.
+
+**Methods:**
+- `Classify(calls []types.ToolCall) (concurrent, sequential []types.ToolCall)` — uses `tool.IsReadOnly`
+- `ExecuteAll(ctx, calls []types.ToolCall, ch chan<- StreamEvent, turn int, intent string) []toolExecResult` — the 15-stage post-call pipeline moved out of `stream_tool_exec.go`
+- `EstimateBlastRadius(calls []types.ToolCall) BlastReport`
+- `SpawnBackgroundAgent(ctx, prompt string) (id string, err error)`
+- `PollBackgroundAgent(id string) (output string, done bool, err error)`
+
+**File:** `hawk/internal/engine/tool_service.go` (~400 LOC — biggest, owns the 15-stage pipeline)
+
+### 4. `PermissionService` — owns the safety layer
+
+**Owns:** `Perm *PermissionEngine`, `Permissions *PermissionMemory`, `AutoMode *permissions.AutoModeState`, `Classifier *permissions.Classifier`, `BypassKill *permissions.BypassKillswitch`, `Mode PermissionMode`, `Autonomy AutonomyLevel`, `PermissionFn func(PermissionRequest)`, `Approval *ApprovalGate`, `MaxBudgetUSD float64`.
+
+**Methods:**
+- `CheckTool(ctx, info ToolCallInfo) (granted bool, denyMsg string)` — wraps Perm.CheckTool + Approval.CheckApproval
+- `ExceedsBudget(cost float64) bool`
+- `RecordToolCall(name string)`
+- `RecordCost(cost float64)`
+- `SetMode(mode string) error` — validates + applies
+- `SetMaxTurns(n int)`, `SetMaxBudgetUSD(usd float64)`
+
+**File:** `hawk/internal/engine/permission_service.go` (~150 LOC)
+
+### 5. `LifecycleService` — owns the self-improvement loop
+
+**Owns:** `Lifecycle *SessionLifecycle`, `Reflector *Reflector`, `Beliefs *BeliefState`, `Backtrack *BacktrackEngine`, `Limits *LimitTracker`, `Critic *Critic`, `Backtrack`, `Trajectory *TrajectoryDistiller`, `Shadow *branching.ShadowWorkspace`, `Cascade *branching.CascadeRouter`, `Steering *SteeringQueue`, `LoopDet *LoopDetector`, `Snowball *branching.SnowballDetector`.
+
+**Methods:**
+- `OnSessionStart(ctx, lastUserMsg string) (learnedCtx string)`
+- `OnSessionEnd(ctx, success bool, duration time.Duration, messages []types.EyrieMessage)`
+- `SelectModel(taskType, currentModel, hint string) string` — wraps Cascade
+- `OnToolFailure(ctx, intent string, history []types.EyrieMessage, errOutput string) (*Reflection, error)`
+- `RecordDecision(turn int, toolNames string, msgs []types.EyrieMessage)`
+- `CheckLimits(turnCount int) (cont bool, reason string)` — wraps Limits
+- `RecordTurn(turn int)`, `RecordToolCall(name string)`
+- `DetectDoomLoop(steps []ToolStep) bool`
+- `InjectSteering(messages []types.EyrieMessage) []types.EyrieMessage`
+
+**File:** `hawk/internal/engine/lifecycle_service.go` (~250 LOC)
+
+### 6. `PersistenceService` — owns checkpoint, session, yaad snapshot
+
+**Owns:** `persistID string`, `checkpointMgr *session.CheckpointManager`, `lastPromptTokens int`, `lastCompletionTokens int`, `ConvoDAG *storage.DAG`, `OnCompaction OnCompaction`, `PinnedMessages int`, `AutoCompactThresholdPct int`, `ContextWindowCached int`, `AutoCompactor *AutoCompactor`, `CompactSplit`, `CompactProviderNative`, `CompactStrategyEngine`, `Files *FileTracker`.
+
+**Methods:**
+- `PersistID() string`, `SetPersistID(id string)`
+- `ManageContextBeforeTurn(ctx) (strategy string, compacted bool)`
+- `Compact()`, `SmartCompact()`, `ShouldAutoCompact()`, `ShouldCompactByBudget()`
+- `AddUser(content string)`, `AddAssistant(content string)`, `AddUserWithImage(...)`, `AddUserWithAttachment(...)`
+- `AddUserWithDocumentText(content, label, extracted string)`
+- `AppendSystemContext(content string)`, `ReplaceSystemContextSection(header, content string)`
+- `Messages() []types.EyrieMessage`, `RawMessages()`, `MessageCount()`, `LoadMessages([]types.EyrieMessage)`
+- `RemoveLastExchange()`
+- `ConvoHead() string`, `ForkConversation(nodeID string) (string, error)`, `SwitchBranch(nodeID string) error`, `ListBranches(nodeID string) ([]*storage.DAGNode, error)`
+- `CheckpointOnCompaction(strategy string, before, after int, manual bool)`
+- `TokenTracking() (prompt, completion int)`, `RecordAPIUsage(prompt, completion int)`
+
+**File:** `hawk/internal/engine/persistence_service.go` (~300 LOC)
+
+## What stays on `Session`
+
+After the refactor, `Session` becomes a thin facade:
+
+```go
+type Session struct {
+    // Identity (still here)
+    ID           string
+    Provider     string
+    Model        string
+    SystemPrompt string
+    // The 6 sub-services
+    Chat        *ChatService
+    Memory      *MemoryService
+    Tools       *ToolService
+    Permission  *PermissionService
+    Lifecycle   *LifecycleService
+    Persistence *PersistenceService
+    // Observability pass-through
+    Tracer    *oteltrace.Tracer
+    Metrics   *metrics.Registry
+    Log       *logger.Logger
+    // Loop control (still on Session because the agent loop reads them every turn)
+    Sandbox        *DiffSandbox
+    Plan           *PlanState
+    OutputSchema   string
+    Teach          TeachConfig
+}
+```
+
+~15 fields total, down from 35. The 6 sub-services are constructed once in `NewSessionWithClient` and stay for the session's lifetime.
+
+## `agentLoop` becomes a 250-line orchestrator
+
+```go
+func (s *Session) agentLoop(ctx context.Context, ch chan<- StreamEvent) {
+    defer close(ch)
+    defer s.Lifecycle.OnSessionEnd(ctx, ...)
+
+    hooks.ExecuteAsync(ctx, hooks.EventSessionStart, ...)
+    s.Lifecycle.OnSessionStart(ctx, s.lastUserMessage())
+
+    s.Persistence.ManageContextBeforeTurn(ctx)
+    msgs := s.Persistence.Messages()
+    sysCtx, err := s.Memory.RecallContext(ctx, s.lastUserMessage(), 2000)
+    if err == nil && sysCtx != "" {
+        s.Persistence.AppendSystemContext(sysCtx)
+    }
+
+    activeModel := s.Lifecycle.SelectModel(taskType, s.Model, "")
+
+    for turn := 0; ; turn++ {
+        if !s.Lifecycle.CheckLimits(turn) { return }
+
+        opts := s.Chat.BuildOptions(s.system, s.Tools.Registry().EyrieTools(), activeModel, maxTok)
+        result, err := s.Chat.Stream(ctx, s.Persistence.Messages(), opts)
+        if err != nil { /* error path */ }
+        defer result.Close()
+
+        // Consume stream: textContent, toolCalls, sawThinking, stopReason
+        ...
+
+        if len(toolCalls) == 0 {
+            s.Persistence.AddAssistant(textContent)
+            s.Memory.AutoRemember(ctx, textContent)
+            ch <- StreamEvent{Type: "done"}
+            return
+        }
+        s.Persistence.AddAssistantWithToolUse(textContent, toolCalls)
+        results := s.Tools.ExecuteAll(ctx, toolCalls, ch, turn, textContent)
+        s.Persistence.AddToolResults(results)
+    }
+}
+```
+
+The loop body shrinks from 800 lines to ~250 because:
+- 15-stage tool pipeline moves to `ToolService.ExecuteAll`
+- 20+ post-call side effects (beliefs, memory, critic, shadow, sandbox, snapshot, agents-accum) move to sub-service methods
+- Stream retry with ctx-cancel moves to `ChatService.Stream`
+- Permission/approval flow moves to `PermissionService.CheckTool`
+
+## Migration Plan
+
+1. **Phase 1** (this PR's 4 fixes): just the time.Sleep, ReadOnlyTools, AGENTS.md, self-review fixes. **No Session refactor yet.**
+2. **Phase 2** (separate PR): extract `ChatService` first because it has the fewest field touches. Verify `agentLoop` builds and tests pass.
+3. **Phase 3**: extract `MemoryService` (most fields, but only consumed in the preamble/postamble of `agentLoop`).
+4. **Phase 4**: extract `PermissionService` (smallest surface).
+5. **Phase 5**: extract `LifecycleService` (biggest behavioral surface, but mostly reads `messages` and `tools`).
+6. **Phase 6**: extract `PersistenceService` (last because it owns the `messages []types.EyrieMessage` field which is touched everywhere).
+7. **Phase 7**: extract `ToolService` (depends on 4, 5; do last).
+8. **Phase 8**: delete fields from `Session`, add backwards-compatible shims that delegate to the new services, mark for removal in v0.2.0.
+
+Each phase is a separate PR with a focused test suite.
+
+## Testing Strategy
+
+After the refactor, the following tests should be trivial (impossible to write today):
+
+```go
+func TestAgentLoopRespectsContextCancellation(t *testing.T) {
+    chat := &mockChatService{streamErr: ctx.Err()}
+    session := &Session{Chat: chat, /* no other services wired */}
+    stream, _ := session.Stream(ctx)
+    // Assert: loop exits on first event
+}
+
+func TestToolExecutionRejectsWithoutPermission(t *testing.T) {
+    perm := &mockPermissionService{granted: false, denyMsg: "nope"}
+    session := &Session{Permission: perm}
+    result := session.Tools.ExecuteAll(ctx, calls, ch, 0, "")
+    // Assert: all results are isErr=true with "nope" content
+}
+
+func TestMemoryRecallReturnsEmptyWhenNoBridge(t *testing.T) {
+    session := &Session{Memory: &MemoryService{}} // no YaadBridge
+    ctx, _ := session.Memory.RecallContext(ctx, "test", 100)
+    assert.Equal(t, "", ctx)
+}
+```
+
+These tests don't need to construct a `Session` anymore; they can construct just the sub-service under test.
+
+## Open Questions
+
+1. Should `SubSession` (the sub-agent factory at `session.go:207-216`) become a method on `Session` or move to a dedicated `SubAgent` service? Currently it copies the parent's LLM transport — that's a `ChatService` concern, not a `Session` concern. Proposal: move to `SubAgentService`.
+2. Should the sub-service interfaces be exported (so external test packages can mock them) or kept internal? The `MockChatService` test above would require either exporting or building test-helper shims.
+3. Where do `OutputSchema`, `GLMThinkingEnabled`, `TeachConfig`, `Sandbox`, `Plan` belong? They're all chat/options concerns. Proposal: fold into `ChatService` or extract a `SessionOptions` struct that `ChatService.BuildOptions` reads.
+
+## Estimated Effort
+
+- Phase 1: ✅ done (this PR)
+- Phases 2-7: ~6 PRs, ~1500 LOC moved, ~800 LOC of `agentLoop` removed, ~600 LOC of new sub-service files added
+- Total: ~5 days of focused refactor work + tests
+
+## Status
+
+**NOT YET IMPLEMENTED.** The above is a design proposal pending review. The 4 concrete fixes (time.Sleep, ReadOnlyTools, AGENTS.md, self-review revert) are merged independently of this refactor.

internal/engine/stream.go

@@ -14,6 +14,7 @@ import (
 	"github.com/GrayCodeAI/hawk/internal/hooks"
 	"github.com/GrayCodeAI/hawk/internal/observability/oteltrace"
 	"github.com/GrayCodeAI/hawk/internal/resilience/retry"
+	"github.com/GrayCodeAI/hawk/internal/tool"
 )
 
 // Stream runs the agentic loop: LLM → tool_use → execute → loop.
@@ -476,7 +477,13 @@ func (s *Session) agentLoop(ctx context.Context, ch chan<- StreamEvent) {
 				"reason":  retryReason,
 				"error":   streamErr.Error(),
 			})
-			time.Sleep(time.Duration(streamAttempt+1) * time.Second)
+			select {
+			case <-time.After(time.Duration(streamAttempt+1) * time.Second):
+			case <-ctx.Done():
+				ch <- StreamEvent{Type: "error", Content: "stream retry cancelled: " + ctx.Err().Error()}
+				result.Close()
+				return
+			}
 
 			// Notify consumer to discard previously streamed content for this turn.
 			ch <- StreamEvent{Type: "retry", Content: fmt.Sprintf("retrying after %s (attempt %d)", retryReason, streamAttempt+2)}
@@ -713,9 +720,8 @@ func (s *Session) agentLoop(ctx context.Context, ch chan<- StreamEvent) {
 		// Auto-snapshot after write operations for granular undo
 		if s.Snapshots != nil && len(toolCalls) > 0 {
 			var writeNames []string
-			safeConcurrent := map[string]bool{"Read": true, "Grep": true, "Glob": true, "LS": true, "WebSearch": true, "WebFetch": true, "ToolSearch": true}
 			for _, tc := range toolCalls {
-				if !safeConcurrent[tc.Name] {
+				if !tool.IsReadOnly(tc.Name) {
 					writeNames = append(writeNames, tc.Name)
 				}
 			}

internal/engine/stream_tool_exec.go

@@ -26,9 +26,8 @@ type toolExecResult struct {
 
 // classifyToolCalls splits tool calls into concurrent (read-only) and sequential (write) batches.
 func classifyToolCalls(calls []types.ToolCall) (concurrent, sequential []types.ToolCall) {
-	safeConcurrent := map[string]bool{"Read": true, "Grep": true, "Glob": true, "LS": true, "WebSearch": true, "WebFetch": true, "ToolSearch": true}
 	for _, tc := range calls {
-		if safeConcurrent[tc.Name] {
+		if tool.IsReadOnly(tc.Name) {
 			concurrent = append(concurrent, tc)
 		} else {
 			sequential = append(sequential, tc)
@@ -207,28 +206,33 @@ func (s *Session) executeSingleTool(ctx context.Context, tc types.ToolCall, ch c
 			if newContent, readErr := readFileContent(preEditPath); readErr == nil && newContent != preEditContent {
 				reviewResult, reviewErr := ReviewBeforeWrite(ctx, s.client, s.model, intentText, preEditPath, preEditContent, newContent)
 				if reviewErr == nil && reviewResult != nil && !reviewResult.Approved {
-					// Revert the file to its original state
+					// Revert the file to its original state. If revert fails we
+					// MUST surface that as a hard tool error: silently leaving
+					// the rejected diff on disk would let a downstream turn
+					// build on top of code the LLM just said was wrong.
+					var revertErr error
 					if preEditContent == "" {
-						if removeErr := os.Remove(preEditPath); removeErr != nil {
-							s.log.Warn("failed to remove file during self-review revert", map[string]interface{}{
-								"path":  preEditPath,
-								"error": removeErr.Error(),
-							})
-						}
+						revertErr = os.Remove(preEditPath)
 					} else {
-						if writeErr := os.WriteFile(preEditPath, []byte(preEditContent), 0o644); writeErr != nil {
-							s.log.Warn("failed to revert file during self-review", map[string]interface{}{
-								"path":  preEditPath,
-								"error": writeErr.Error(),
-							})
-						}
+						revertErr = os.WriteFile(preEditPath, []byte(preEditContent), 0o644)
 					}
-					issueStr := "Self-review found issues: " + strings.Join(reviewResult.Issues, "; ")
-					if len(reviewResult.Suggestions) > 0 {
-						issueStr += ". Suggestions: " + strings.Join(reviewResult.Suggestions, "; ")
+					if revertErr != nil {
+						s.log.Error("self-review revert failed; rejecting diff loudly", map[string]interface{}{
+							"path":  preEditPath,
+							"error": revertErr.Error(),
+						})
+						output = fmt.Sprintf("Self-review rejected the change AND the revert failed: %s. "+
+							"Original review issues: %s. Manual intervention required.",
+							revertErr.Error(), strings.Join(reviewResult.Issues, "; "))
+						isErr = true
+					} else {
+						issueStr := "Self-review found issues: " + strings.Join(reviewResult.Issues, "; ")
+						if len(reviewResult.Suggestions) > 0 {
+							issueStr += ". Suggestions: " + strings.Join(reviewResult.Suggestions, "; ")
+						}
+						output = issueStr + ". Please fix these issues and try again."
+						isErr = true
 					}
-					output = issueStr + ". Please fix these issues and try again."
-					isErr = true
 				} else if reviewErr == nil && reviewResult != nil && reviewResult.Approved {
 					// Append diff summary to output for TUI display
 					diffSummary := generateDiffSummary(preEditContent, newContent, preEditPath)