Skip to content

refactor(hooks): extract SnapshotController and stop the runtime from knowing snapshots exist #2701

@dgageot

Description

@dgageot

Background

snapshot is a builtin hook in pkg/hooks/builtins/snapshot.go that records shadow-git checkpoints at six different events: session_start, turn_start, turn_end, pre_tool_use, post_tool_use, and session_end. The implementation lives in pkg/hooks/builtins, but it is by far the leakiest builtin in terms of API surface.

Current coupling

  • The builtin keeps per-session state (shadow-git repo + checkpoint history) on *State.snapshot.
  • The runtime exposes four public methods on *LocalRuntime that simply delegate into that state:
    • LocalRuntime.SnapshotsEnabled()
    • LocalRuntime.UndoLastSnapshot(...)
    • LocalRuntime.ListSnapshots(...)
    • LocalRuntime.ResetSnapshot(...)
  • The TUI's /undo and /snapshots commands call those runtime methods.
  • WithSnapshots(bool) is a runtime.Opt that flips a runtime field (r.snapshotsEnabled), which is then read by buildHooksExecutors to auto-inject the six hook entries.

The result: the builtin has two parallel APIs — the hook protocol (six events) and a Go method API (four runtime methods). The runtime knows snapshots exist; the TUI talks to a builtin through the runtime; configuration of an auto-injection flag lives on a runtime opt instead of with the builtin.

Isolation grade

C — implementation is isolated, but coupling flows backward (builtin → runtime → TUI) through Go method calls outside the hook protocol.

Proposed fixes

1. Extract a SnapshotController and have the TUI talk to it directly

Introduce something like:

// pkg/hooks/builtins/snapshot.go
type SnapshotController interface {
    UndoLast(ctx context.Context, sessionID, cwd string) (files int, ok bool, err error)
    List(sessionID string) []SnapshotInfo
    Reset(ctx context.Context, sessionID, cwd string, keep int) (files int, ok bool, err error)
    Enabled() bool
}

The snapshot builtin returns this controller from a registration helper (e.g. builtins.RegisterSnapshot(r, opts) (SnapshotController, error)). The TUI obtains it the same way it gets a hook executor today — through whatever wiring the embedder (CLI/TUI) already does — instead of asking the runtime.

Outcome:

  • LocalRuntime.SnapshotsEnabled / UndoLastSnapshot / ListSnapshots / ResetSnapshot go away.
  • LocalRuntime.snapshotsEnabled, LocalRuntime.snapshotCwd, runtime/snapshot.go go away.
  • The runtime stops knowing snapshots exist.

2. Move auto-injection metadata next to the builtin

ApplyAgentDefaults currently switch-cases each builtin (AddDate, AddEnvironmentInfo, Snapshot, …) to decide which events to wire and from which flag. This central table grows every time we add a builtin.

Replace it with per-builtin defaulting. Each builtin declares its own auto-injection rule:

// e.g. on registration
type AutoInject struct {
    Predicate func(AgentDefaults) bool
    Events    []EventType
    Hook      Hook
}

ApplyAgentDefaults becomes a generic loop over registered builtins, asking each whether it wants to inject itself for the supplied AgentDefaults. The snapshot's six-event injection lives in the snapshot file, not in builtins.go.

3. Move the WithSnapshots opt out of runtime.Opt

If (1) and (2) land, the snapshots-on/off boolean is a property of the snapshot subsystem, not of the runtime. The CLI/TUI can pass it directly when constructing the SnapshotController, and runtime.WithSnapshots is deleted. The runtime is then unaware of the snapshot feature in any way.

Recommendation

Land (1) first — it removes the four passthrough methods on *LocalRuntime and makes the boundary clean — then (2) as a follow-up to remove the central ApplyAgentDefaults table for the rest of the builtins, and (3) as the final cleanup once nothing in the runtime references snapshot state.

Acceptance criteria

  • pkg/runtime/snapshot.go is gone (or contains only a small wiring shim).
  • LocalRuntime exposes no snapshot-specific methods.
  • TUI commands (/undo, /snapshots, /reset) drive the snapshot controller directly without going through the runtime.
  • WithSnapshots is removed from runtime.Opt (configuration moves to the controller).
  • Existing snapshot tests pass; a new test confirms the runtime can be built without snapshot wiring.

Metadata

Metadata

Assignees

Labels

area/agentFor work that has to do with the general agent loop/agentic features of the appeffort:mediumMultiple files or components, some design decisions neededpriority:mediumNormal priority, standard sprint work

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions