From 9e039712b1520f73a66f7a66814e103107cc0298 Mon Sep 17 00:00:00 2001 From: Ian Gulliver Date: Fri, 16 Oct 2026 12:00:44 -0700 Subject: [PATCH] Codebase health review --- TODO.md | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/TODO.md b/TODO.md index 8b3790f..8517340 100644 --- a/TODO.md +++ b/TODO.md @@ -1,5 +1,32 @@ # Bugs +- Task accepts new turns while its merge is being finalized + `EndTurn` clears `merging_by` (`internal/agents/manager.go:899`, `:926–930`) before `finishMerge` snapshots the live worktree, pushes and runs `RemoveTask` (`internal/agents/mergein.go:238–318`), and `transitionSession` blocks only discard (`internal/store/store.go:1389`). A message sent during the push can put half-done edits into the pushed squash, or be killed when the worktree is force-removed. Add a persisted `finalizing` marker set in the same EndTurn transaction; turns, discard and subtask creation refuse while it is set, and the UI shows it as Merging. +- Merge finalization keeps no durable progress; cleanup runs before the task is closed + `internal/agents/mergein.go:303–324` runs push → notes → fast-forward → `RemoveTask` → `MergeTask`. A crash or error after the push leaves an active task with no worktree; a retried Merge fails in `Snapshot`, or squashes to no change and closes with an empty `merge_commit`. Have the `finalizing` marker record the squash commit; on startup, a recorded commit that is an ancestor of origin/ finishes notes, fast-forward, removal and MergeTask idempotently, otherwise the marker is cleared and an error is filed. +- A failed auto-commit still advances the workflow from the old HEAD + A `CommitAll` error is only logged (`internal/agents/manager.go:838–841`) and `FinishStage` still runs (`:885–889`), so `RecordStage` stores the old `git.Head` (`internal/agents/workflows.go:349–356`), the next stage branches without the turn's work, and `discardStages` later deletes the worktree that held it. On commit failure, treat the turn as unfinished: skip FinishStage, the handoff and the summary, and file the error in the task's inbox. +- One bad session at startup stops byre from starting + `Recover` returns on a `taskFor` or `message` error (`internal/agents/manager.go:300–303`, `:338–341`) and `server.New` treats it as fatal (`internal/server/server.go:72–76`), so one unreadable worktree keeps the service down. Handle a failed `message` like a failed `ensureLive` (logUndelivered, DropStageOutput, EndTurn with the error, discard and merge follow-up) and continue; a `taskFor` error skips only that session. +- A start failure after admission leaves a session `running` with no turn + `launch` resets to idle only on `ensureLive`/`syncModel` errors (`internal/agents/manager.go:439–466`); a local `message` error with a live process (missing blob, failed `rev-parse HEAD`, failed event write) leaves it running, so later messages queue forever and Merge skips the task. When `message` fails before `send`, set the session idle with the error; a `send` failure is already handled by pump. +- Codex mid-turn delivery deletes input before the steer is accepted + Queued inputs are deleted or marked delivered and recorded as `user.message` (`internal/agents/queue.go:119`, `:158`; `internal/agents/inbox.go:404`) before `Steer`, whose error is only logged (`internal/agents/codex.go:167–174`). Return the consumed ids from `deliverMidTurn`; when Steer fails, re-insert the inputs in order and clear `delivered_at` so the turn-end Deliver sends them. +- Inbox answer validation isn't atomic with resolving the question + `CheckResponse` reads open state outside the write transaction and `Respond` updates by id alone (`internal/agents/respond.go:26–34`, `:93`; `internal/store/inbox.go:140`), so two concurrent answers both land, or a withdrawal is overwritten. Condition the update on `state='open'`, check RowsAffected, roll back the batch on conflict and return 409. +- A Codex RPC can deadlock the reader it waits on + `drive` calls `Steer` synchronously while `readLoop` pushes notifications into a 256-entry channel (`internal/codex/session.go:186`, `:440–476`, `:598`); if it fills before the response is read, each waits on the other. Route RPC responses separately from notification consumption and give RPCs bounded deadlines, without replacing backpressure with an unbounded queue. +- Agent startup holds the manager's global lock + `ensureLive` holds `m.mu` through process start and protocol init (`internal/agents/manager.go:538–629`), and `deliverMu` serializes every session (`internal/agents/queue.go:15–16`), so one slow start stalls health, interrupt and event handling everywhere. Coordinate starts per session, release m.mu during process I/O and publish the result under the lock, and narrow delivery serialization to the session. +- Git network operations have no deadline and run under a cross-project lock + `exec.Command` has no context (`internal/git/git.go:348–364`) and Merge holds the shared `mergeMu` through fetch (`internal/agents/mergein.go:68`, `:101`), so one stalled remote blocks merges in every project. Add contextual deadlines and non-interactive git/SSH, and serialize the network phase per project while keeping same-project ordering. +- A malformed file image URL breaks transcript rendering + `decodeURI` throws on e.g. `file:///tmp/100%.png` (`web/src/markdown.ts:347`), and `renderAll` appends events before mounting the composer (`web/src/task.ts:608–609`), so the rest of the transcript and the composer don't render. Catch the decode error and render the plain fallback link. +- Stale reload and focus responses overwrite newer state + `loadTasks`, `loadSessions`, `loadInbox` and `setFocus` apply every response, even a late one (`web/src/state.ts:158–179`, `:203–215`), so clicking through tabs quickly can snap the pane back. Keep a request sequence per resource key and for focus, and apply only the latest response. +- operations.md describes two incompatible event-stream designs + `docs/operations.md:320–324` says selecting tasks opens dedicated SSE streams, which is stale against `web/src/state.ts:309–340`, `:401–414`, and says only `agent.delta` is transient though `admin.health` is too. Rewrite the Events section to match the current two-stream feed design, and qualify the transaction/event guarantee at `docs/operations.md:7` for internal bookkeeping writes. + # M1 — One agent end to end # M2 — Seeing the work