fix: M1-M11 medium-severity issues from prior code review (#53)

* fix(cmd): M1 part 1 - pinSubcommand, GLM thinking, status use accessors

- chat_subcommand_pin.go: use m.session.SetPinnedMessages
- chat_subcommand_simple.go: use m.session.SetGLMThinkingEnabled
- chat_subcommand_status.go: use m.session.CostValue().Summary()
- session.go: add SetPinnedMessages accessor

* fix(cmd): M1 part 2 - Autonomy, ContainerExecutor, ConvoDAG accessors

- chat.go: use PermSvc().Autonomy()/SetAutonomy, SetContainerExecutor, SetContainerRequired
- permissions_center.go: use PermSvc().SetAutonomy
- welcome_gate.go, statusbar.go: use PermSvc().Autonomy()
- hud_panel.go: use MemorySvc().Yaad()
- chat_status.go: use ContextWindowCachedValue, Persistence().SetContextWindowCached
- chat_subcommand_branches.go: use Persistence().DAG()
- chat_commands_session.go: use Persistence().DAG()
- session.go: add SetContainerExecutor accessor
- audit_test.go: regex now matches m.session.X; cmdHardFailThreshold=0

* fix(cmd): M1 part 3 - remaining cmd/ Cost/Mode/ContextWindow accessors

- statusbar.go: use m.session.CostValue()
- chat_subcommand_cost.go, chat_subcommand_usage.go: use CostValue()
- chat_commands_session.go, chat_commands_util.go: use CostValue()
- chat.go: use ContextWindowCachedValue()
- permissions_center.go: use ModeValue() (safe fallback for test struct literal)
- chat_model_test.go, statusbar_test.go: use accessors
- session_sync.go, snapshot_cmd.go: avoid audit regex false positives in text
- session.go: add SetContextWindowCached, ModeValue, SetMode accessors

* fix(engine): M2 - AddUser/AddAssistant use MemorySvc() and Persistence().DAG()

- AddUser: read Memory from s.MemorySvc().Memory() (was s.Memory)
- AddUser, AddUserWithImage, AddAssistant, ForkConversation, SwitchBranch,
  ListBranches, ConvoHead: read DAG from s.Persistence().DAG() (was s.ConvoDAG)
- Aligns with the H6 sub-service decomposition: the legacy ConvoDAG and
  Memory fields on Session are aliases set by NewSessionWithClient; new
  code goes through the sub-service getters to keep the
  god-object-decomposition contract consistent.

* fix(engine): M3 - deprecation comments point at real accessors

- FewShot() -> FewShotStore() (typo fix in comment)
- PlanState, Trajectory, LintLoop, TestLoop, FileMentions, Files,
  Snapshots, Tracer, RateLimiter: comments now indicate these
  remain on the legacy Session struct (no sub-service accessor
  exists); the previous deprecation text pointed to non-existent
  methods that callers could not find.

* fix(engine): M4 - PermissionService constructor no longer pre-sets mode

NewPermissionService no longer initializes mode to PermissionModeDefault;
the zero value ("") is preserved so IsZero() can correctly report a
freshly constructed service. IsZero() now checks for the empty string
instead of PermissionModeDefault. This is the right semantics: callers
that want a default mode should call SetMode explicitly, and tests can
rely on IsZero() being true for a fresh service.

* fix(cmd): M5 - SubcommandRegistry.Register detects alias collision

The previous implementation checked for primary-name duplicates
(`r.primary[name]`) but not alias collisions. Two subcommands
that each register a different primary but share an alias would
silently overwrite `r.aliasOf[alias]`. Now the alias check
mirrors the primary-name check: registration is rejected if any
of the subcommand's aliases is already taken. The existing
silent-no-op pattern is preserved (no error return) so init()
callers don't have to change.

* fix(cmd): M6 - sessionSubcommand.Handle prepends command name to args

The previous Handle passed the post-name args slice directly to
handleSessionCommand, which expects parts[0] to be the command
name. This broke /recover <id>, /resume <id>, and /tag <label>:
the trailing arg landed at parts[0] instead of parts[1], and the
`len(parts) >= 2` check failed, hitting the usage error path.

The fix reconstructs the full parts slice (name + args) before
calling handleSessionCommand, matching the contract the
simple.go delegatingCommand handlers use. The parts-building
logic is extracted into a buildSessionParts helper for testability.

New tests (TestBuildSessionParts_*, TestSessionSubcommand_RecoverIDReachesPartsIndex1,
TestResolveSessionName_PicksLongestMatch) assert the fix and the
dispatcher contract.

* fix(multiagent): M7 - WaitForLock no longer busy-spins

The previous for-select loop had two problems: (1) after the first
\<-w.done fired, the closed channel kept firing and the loop
re-called AcquireLock, busy-spinning until the timer.C fired.
(2) Both <-w.done and <-timer.C become ready at timeout, and Go's
select picks randomly — so the function could miss the timeout
and loop indefinitely.

Fix: replace the for-loop with a one-shot select. On the first
signal (w.done or timer), try AcquireLock exactly once. If it
succeeds, return nil; if the timer fired, return a timeout error.
The race-loser no longer re-registers — it returns an error.

TestWaitForLock_MultipleWaiters_OnlyOneAcquires updated to assert
the new semantics: the race-loser gets an error rather than
re-registering and waiting for a second signal.

* fix(multiagent): M8 - logDroppedMessage moved out of mb.mu critical section

The previous implementation called logDroppedMessage while holding
mb.mu (write lock). The slog.Warn call may acquire its own internal
lock (the slog handler's mutex); if the handler is slow (file I/O,
network sink), this serializes all bus operations for the duration
of the log write.

Fix: snapshot the (from, to, topic) of each drop into a local
slice while still holding the lock, then iterate the slice after
the lock is released and call logDroppedMessage on each entry.
The error return path is also collected into a local variable
and returned after the lock is released.

No behavior change for the caller; only the order of operations
differs (logs now happen after the bus is unlocked).

* fix(multiagent): M9 - extract dropLogEveryN as a named constant

The previous 100 was a magic number inlined in the
`n%100 != 0` check. Extracted as a package-level const
`dropLogEveryN = 100` with a doc comment explaining the
rationale (balancing observability against log volume under
sustained pressure).

* test(multiagent): M10 - rename test + add real waiter-assertion test

- TestWaitForLock_OwnerMismatchOnRelease renamed to
  TestReleaseLock_NonOwnerReturnsError; the original name claimed
  it was about waiters not being woken, but the body only checked
  the error return.
- New TestWaitForLock_ReleaseByNonOwnerDoesNotWakeWaiter registers
  a real waiter, attempts a ReleaseLock from a non-owner, and
  asserts the waiter times out (rather than being woken by the
  failed ReleaseLock). This is the test the original name
  promised but never delivered.

* test(multiagent): M11 - join the WaitForResponse goroutine via a done channel

The previous TestStats_NotAffectedByWaiters launched a goroutine
that called WaitForResponse and never joined it; the goroutine
ran for 100ms after the test exited. Fixed by:
- Adding a done channel that the goroutine closes on return.
- Adding a 1-second join timeout via select on the done channel.
- The test now fails loudly if the goroutine does not return
  within 1s (which would indicate a future regression).

* fix(engine): M2 - make AddUser/AddAssistant/ForkConversation/etc nil-safe

The M2 refactor changed these methods to call
s.Persistence().DAG() and s.MemorySvc().Memory() directly. That
panics on &engine.Session{} struct literals (used by cmd tests)
where s.persist and s.memory are nil.

This commit wraps each call site in a nil-check on the parent
service (s.Persistence() / s.MemorySvc()) so that struct-literal
sessions still work the same way as before — the methods
become no-ops when their backing sub-service is nil, which
matches the previous behavior on s.ConvoDAG == nil and
s.Memory == nil.

Author: Patel230

cmd/chat.go

@@ -315,7 +315,7 @@ func newChatModel(ref *progRef, systemPrompt string, settings hawkconfig.Setting
 	startup.EndPhase("newChatModel:commandPalette")
 
 	// Pre-warm footer connection line so ctx (e.g. 0k/1.0m) shows on first paint.
-	if m.session != nil && m.session.ContextWindowCached > 0 {
+	if m.session != nil && m.session.ContextWindowCachedValue() > 0 {
 		m.connStatusVal = m.buildConnectionStatusPlain()
 		m.connStatusKey = m.connStatusFingerprint()
 	}
@@ -896,11 +896,11 @@ func (m chatModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 				m.updateViewportContent()
 				return m, nil
 			}
-			next := nextAutonomyTier(m.session.Autonomy)
-			if m.session.Autonomy == 0 || autonomyTierIndex(m.session.Autonomy) < 0 {
+			next := nextAutonomyTier(m.session.PermSvc().Autonomy())
+			if m.session.PermSvc().Autonomy() == 0 || autonomyTierIndex(m.session.PermSvc().Autonomy()) < 0 {
 				next = DefaultContainerAutonomy
 			}
-			m.session.Autonomy = next
+			m.session.PermSvc().SetAutonomy(next)
 			m.invalidateConnStatus()
 			m.messages = append(m.messages, displayMsg{role: "system", content: formatAutonomyTierMessage(next)})
 			m.viewDirty = true
@@ -1275,17 +1275,17 @@ func (m chatModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 		if msg.sandbox != nil {
 			m.containerSandbox = msg.sandbox
 			if m.session != nil {
-				m.session.ContainerExecutor = msg.sandbox
+				m.session.SetContainerExecutor(msg.sandbox)
 			}
 		}
 		if msg.ready && m.session != nil {
-			if m.session.Autonomy == 0 {
-				m.session.Autonomy = DefaultContainerAutonomy
+			if m.session.PermSvc().Autonomy() == 0 {
+				m.session.PermSvc().SetAutonomy(DefaultContainerAutonomy)
 			}
 			if m.phase == phaseWelcomeGate {
 				m.sandboxReadyPending = true
 			} else {
-				m.messages = append(m.messages, displayMsg{role: "system", content: formatSandboxReadyAutonomyMessage(m.session.Autonomy)})
+				m.messages = append(m.messages, displayMsg{role: "system", content: formatSandboxReadyAutonomyMessage(m.session.PermSvc().Autonomy())})
 			}
 			m.invalidateConnStatus()
 		}
@@ -1294,8 +1294,8 @@ func (m chatModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
 			m.containerEnabled = false
 			m.containerReady = false
 			if m.session != nil {
-				m.session.ContainerRequired = false
-				m.session.ContainerExecutor = nil
+				m.session.SetContainerRequired(false)
+				m.session.SetContainerExecutor(nil)
 			}
 			m.messages = append(m.messages, displayMsg{
 				role:    "system",

cmd/chat_commands_session.go

@@ -200,7 +200,7 @@ func (m *chatModel) handleSessionCommand(cmd string, parts []string, text string
 
 	case "/fork":
 		// If convodag is active, fork from the current head node
-		if m.session.ConvoDAG != nil {
+		if m.session.Persistence().DAG() != nil {
 			headID := m.session.ConvoHead()
 			if headID == "" {
 				m.messages = append(m.messages, displayMsg{role: "error", content: "No conversation to fork from."})
@@ -415,7 +415,7 @@ func (m *chatModel) handleSessionCommand(cmd string, parts []string, text string
 	case "/session":
 		info := fmt.Sprintf("Session: %s\nModel: %s/%s\nPermission mode: %s\nMessages: %d\nTools: %d\n%s",
 			m.sessionID, m.session.Provider(), m.session.Model(),
-			permissionModeLabel(m.session), m.session.MessageCount(), len(m.registry.EyrieTools()), m.session.Cost.Summary())
+			permissionModeLabel(m.session), m.session.MessageCount(), len(m.registry.EyrieTools()), m.session.CostValue().Summary())
 		m.messages = append(m.messages, displayMsg{role: "system", content: info})
 		return m, nil
 

cmd/chat_commands_util.go

@@ -123,7 +123,7 @@ func (m *chatModel) mcpSummary() string {
 
 func sessionStats(sess *engine.Session, id string) string {
 	return fmt.Sprintf("Session: %s\nMessages: %d\nModel: %s/%s\n%s",
-		id, sess.MessageCount(), sess.Provider(), sess.Model(), sess.Cost.Summary())
+		id, sess.MessageCount(), sess.Provider(), sess.Model(), sess.CostValue().Summary())
 }
 
 func hooksSummary() string {

cmd/chat_model_test.go

@@ -12,7 +12,7 @@ import (
 
 func newTestChatModel() *chatModel {
 	sess := engine.NewSession("", "test-model", "you are helpful", nil)
-	sess.MaxTurns = 1
+	sess.PermSvc().SetMaxTurns(1)
 	sess.SetTestClient(engine.NewMockClientForTest())
 
 	m := &chatModel{

cmd/chat_status.go

@@ -139,7 +139,7 @@ func (m chatModel) connectionStatusParts() (gateway, model, contextLabel string)
 	model, contextLabel = modelStatusMeta(gw, modelID)
 	if contextLabel == "" || contextLabel == "—" || contextLabel == "0k" {
 		if m.session != nil {
-			if w := m.session.ContextWindowCached; w > 0 {
+			if w := m.session.ContextWindowCachedValue(); w > 0 {
 				contextLabel = formatModelTableContext(w)
 			} else if w := m.session.ContextWindowSize(); w > 0 && w != engine.DefaultContextWindow {
 				contextLabel = formatModelTableContext(w)
@@ -149,7 +149,7 @@ func (m chatModel) connectionStatusParts() (gateway, model, contextLabel string)
 			if w := platformContextForNativeModel(modelID); w > 0 {
 				contextLabel = formatModelTableContext(w)
 				if m.session != nil {
-					m.session.ContextWindowCached = w
+					m.session.SetContextWindowCached(w)
 					m.session.EnsureAutoCompactor()
 				}
 			}

cmd/chat_subcommand.go

@@ -89,7 +89,10 @@ func NewSubcommandRegistry() *SubcommandRegistry {
 // all aliases are indexed. If a name is already registered, this
 // is a no-op (the existing entry is kept) — duplicate registration
 // is treated as a configuration error but doesn't panic, so test
-// ordering and re-init don't blow up the binary.
+// ordering and re-init don't blow up the binary. The same applies
+// to alias collisions: if any of the subcommand's aliases is already
+// registered (either as a primary or as another alias), registration
+// is rejected (see M5 in the code review).
 func (r *SubcommandRegistry) Register(cmd ChatSubcommand) {
 	if cmd == nil {
 		return
@@ -100,6 +103,11 @@ func (r *SubcommandRegistry) Register(cmd ChatSubcommand) {
 	if _, exists := r.primary[name]; exists {
 		return // duplicate
 	}
+	for _, alias := range cmd.Aliases() {
+		if _, exists := r.aliasOf[alias]; exists {
+			return // alias collision
+		}
+	}
 	r.primary[name] = cmd
 	for _, alias := range cmd.Aliases() {
 		r.aliasOf[alias] = name

cmd/chat_subcommand_branches.go

@@ -14,7 +14,7 @@ func (b *branchesSubcommand) Aliases() []string   { return nil }
 func (b *branchesSubcommand) Description() string { return "list conversation DAG branches" }
 func (b *branchesSubcommand) Usage() string       { return "" }
 func (b *branchesSubcommand) Handle(m *chatModel, args []string, text string) (tea.Model, tea.Cmd) {
-	if m.session.ConvoDAG == nil {
+	if m.session.Persistence().DAG() == nil {
 		m.messages = append(m.messages, displayMsg{role: "system", content: "No conversation branches (DAG not active)."})
 		return m, nil
 	}

cmd/chat_subcommand_cost.go

@@ -13,7 +13,7 @@ func (c *costSubcommand) Aliases() []string   { return nil }
 func (c *costSubcommand) Description() string { return "print session cost and token usage summary" }
 func (c *costSubcommand) Usage() string       { return "" }
 func (c *costSubcommand) Handle(m *chatModel, args []string, text string) (tea.Model, tea.Cmd) {
-	m.messages = append(m.messages, displayMsg{role: "system", content: m.session.Cost.Summary()})
+	m.messages = append(m.messages, displayMsg{role: "system", content: m.session.CostValue().Summary()})
 	return m, nil
 }
 

cmd/chat_subcommand_pin.go

@@ -25,7 +25,7 @@ func (p *pinSubcommand) Handle(m *chatModel, args []string, text string) (tea.Mo
 			n = parsed
 		}
 	}
-	m.session.PinnedMessages = n
+	m.session.SetPinnedMessages(n)
 	m.messages = append(m.messages, displayMsg{role: "system", content: fmt.Sprintf("Pinned last %d messages (protected from compaction).", n)})
 	return m, nil
 }

cmd/chat_subcommand_session.go

@@ -24,20 +24,45 @@ func (s *sessionSubcommand) Description() string {
 	return "session management: clear, compact, diff, recover, resume, history, quit, exit"
 }
 func (s *sessionSubcommand) Usage() string { return "" }
-func (s *sessionSubcommand) Handle(m *chatModel, args []string, text string) (tea.Model, tea.Cmd) {
-	name := ""
+
+// sessionSubcommandNames is the ordered list of slash names covered
+// by sessionSubcommand. Exposed for tests and help rendering.
+var sessionSubcommandNames = []string{"/clear", "/compact", "/diff", "/recover", "/resume", "/history", "/quit", "/exit"}
+
+// resolveSessionName inspects the raw slash text and returns the
+// matching command name (with the leading "/"). Falls back to
+// "/<primary>" when text doesn't start with a slash.
+func resolveSessionName(text string, primary string) string {
 	if len(text) > 0 && text[0] == '/' {
-		for _, c := range []string{"/clear", "/compact", "/diff", "/recover", "/resume", "/history", "/quit", "/exit"} {
+		for _, c := range sessionSubcommandNames {
 			if len(text) >= len(c) && text[:len(c)] == c {
-				name = c
-				break
+				return c
 			}
 		}
 	}
-	if name == "" {
-		name = "/" + s.Name()
-	}
-	return m.handleSessionCommand(name, args, text)
+	return "/" + primary
+}
+
+// buildSessionParts reconstructs the full parts slice that
+// handleSessionCommand expects: parts[0] is the command name,
+// parts[1:] is the post-name argument list. Exposed for testing
+// the dispatcher contract (see M6 in the code review).
+func buildSessionParts(name string, args []string) []string {
+	parts := make([]string, 0, 1+len(args))
+	parts = append(parts, name)
+	parts = append(parts, args...)
+	return parts
+}
+
+func (s *sessionSubcommand) Handle(m *chatModel, args []string, text string) (tea.Model, tea.Cmd) {
+	name := resolveSessionName(text, s.Name())
+	// handleSessionCommand expects parts[0] to be the command name (e.g.
+	// "/recover"), with the remaining entries being the post-name args.
+	// The dispatcher hands us the post-name slice; reconstruct the
+	// full parts slice so /recover <id>, /resume <id>, /tag <label>, etc.
+	// receive the trailing argument.
+	parts := buildSessionParts(name, args)
+	return m.handleSessionCommand(name, parts, text)
 }
 
 func init() {