Refactor: Introduce SessionBackend interface#58
Conversation
…aming - Define SessionBackend interface with 13 methods and role interfaces (SessionLifecycle, SessionObserver, SessionActor) for narrow dependencies - Implement TmuxSessionBackend wrapping existing tmux functions (zero behavior change) - Add server backend registry with backendFor(agent) and backendByName(name) helpers - Rename AgentUpdate.TmuxSession → SessionID, add BackendType field - Rename SSE event tmux_liveness → session_liveness - Rename API endpoint /api/tmux-status → /api/session-status - Add DB migration to copy tmux_session column to session_id - Update frontend types, API client, SSE composable, and all components - Update all docs, scripts, and protocol references to use session_id - Remove old agent_backend.go (superseded by SessionBackend)
jsell-rh
left a comment
There was a problem hiding this comment.
Review — SessionBackend Interface Refactor
Overall: Well-scoped refactoring that correctly abstracts the session layer behind an interface. The TmuxSessionBackend is a clean wrapper with zero behavior change. A few correctness and compatibility concerns to address.
Required fixes
1. DB migration leaves tmux_session column permanently in the schema
The migration copies tmux_session → session_id but never drops the old column:
if db.Migrator().HasColumn(&Agent{}, "tmux_session") {
db.Exec(`UPDATE agents SET session_id = tmux_session WHERE ...`)
}GORM AutoMigrate adds session_id but never drops tmux_session. The old column persists forever. Add an explicit drop after the copy:
if db.Migrator().HasColumn(&Agent{}, "tmux_session") {
db.Exec(`UPDATE agents SET session_id = tmux_session WHERE ...`)
db.Migrator().DropColumn(&Agent{}, "tmux_session")
}2. Hard break for running agents: ?tmux_session= ignition parameter removed
The ignition endpoint now only accepts ?session_id=. Any agent currently using /boss.ignite (which generates ?tmux_session=YOUR_TMUX_SESSION) will silently fail to register its session — it will be ignored as an unknown query param. The fix is to accept both during a transition window:
sessionID := r.URL.Query().Get("session_id")
if sessionID == "" {
sessionID = r.URL.Query().Get("tmux_session") // backward compat
}3. Hard break for running agents: "tmux_session" JSON POST field
The AgentUpdate struct rename from TmuxSession to SessionID changes the JSON key from "tmux_session" to "session_id". Any running agent posting {"tmux_session": "MySession"} will have that field silently dropped — their session will be de-registered on the next POST. The struct should keep backward compat via JSON tags:
SessionID string `json:"session_id,omitempty"`
// Note: consider also accepting "tmux_session" via a custom UnmarshalJSON
// or a transitional alias fieldThis is particularly important because boss.check and boss.ignite skill documents cached by running agents all use "tmux_session".
Recommendations
4. agent_backend.go deletion — verify nothing is lost
The diff shows agent_backend.go is deleted but I can't see its full content. Ensure all behavior (especially the backendFor/backendByName helpers mentioned in the PR description, plus any startup logic) is accounted for in session_backend.go or lifecycle.go.
5. Role interfaces not visible in diff
The PR description mentions SessionLifecycle, SessionObserver, SessionActor role interfaces for narrow dependencies. These are only referenced in the compile-time checks in session_backend_tmux.go. If these interfaces are defined in session_backend.go, they should also appear in the diff — but they're not shown. Confirm they're included.
6. session_liveness SSE rename breaks external consumers
Renaming the SSE event from tmux_liveness to session_liveness is a silent breaking change for any external tool or script listening to the event stream. The frontend is updated, but consider whether external integrations (OpenShift monitoring, scripts, etc.) need a deprecation notice.
7. migrate_from_json.go alignment issue
SessionID: ja.TmuxSession, // inconsistent indentation vs surrounding codeMinor but worth fixing for code consistency.
Verdict
Request changes on items 1–3. The interface design is sound and the tmux wrapping is clean. The DB migration issue (#1) could cause data inconsistency for existing deployments. Items #2 and #3 will break the entire active agent fleet on deploy without a compatibility shim — the PR description even acknowledges this. A one-cycle transition period (accept both tmux_session and session_id) would make this safely deployable.
| SendInput(sessionID string, text string) error | ||
| Approve(sessionID string) error | ||
| Interrupt(ctx context.Context, sessionID string) error | ||
| } |
The one-time migration copied tmux_session → session_id but left the old column permanently in the schema. Add an explicit DropColumn after the data copy so the column is cleaned up.
Running agents with cached /boss.ignite and /boss.check commands still
use ?tmux_session= and {"tmux_session": "..."} in their requests. Accept
both the old and new names during the transition window:
- Ignition endpoint falls back to ?tmux_session= if ?session_id= is empty
- AgentUpdate accepts "tmux_session" JSON field, mapped to SessionID
in sanitizeAgentUpdate
All shims are marked with ## TODO - REMOVE ME for easy cleanup.
Review Feedback FixesAddressed the following review items in two separate commits: 1. DB migration drops obsolete
|
…ckend (TASK-054) Without a sleep after tmux new-session, the first send-keys (cd workDir) races shell startup and is silently dropped, causing agents to start in the wrong working directory. Add 300ms sleep before sending any keys — matches the pattern already used in handleAgentSpawn (lifecycle.go). The original fix targeted agent_backend.go which no longer exists after the SessionBackend interface refactor (PR #58); this applies the same fix to the correct file: session_backend_tmux.go. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ckend (TASK-054) (#65) Without a sleep after tmux new-session, the first send-keys (cd workDir) races shell startup and is silently dropped, causing agents to start in the wrong working directory. Add 300ms sleep before sending any keys — matches the pattern already used in handleAgentSpawn (lifecycle.go). The original fix targeted agent_backend.go which no longer exists after the SessionBackend interface refactor (PR #58); this applies the same fix to the correct file: session_backend_tmux.go. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Based on
.spec/SessionBackendsspecifications, implement the fullSessionBackendinterface and theTmuxSessionBackendto ensure existing functionality remains.NOTE: This updates the guidance provided to all agents in docs/ and in the various commands/scripts to send
session_idrather thantmux_session. Its likely to break any existing agent team because of that.General list of changes: