Hunting redundancy, inconsistency and races in four subsystems

A worktree the user declined to merge was published to their branch anywayrace

Tasks can run in a git worktree. When one finishes, the user gets a bar offering merge or discard, and the worktree sits pending until they choose. The daemon decided who still owned that pending worktree using the current run's "use worktree" setting rather than asking whether the agent was still holding one. So if the next prompt on that tab ran without a worktree, the pending branch lost its owner, its preserve marker and nothing else — it stayed registered. A later worktree task's orphan-reclaim sweep then found it and squash-merged it into the working branch. The user had been asked, had implicitly declined by moving on, and the work landed regardless.

The end-to-end reproduction ends with the plainest assertion in the whole change set:

assert 'declined.txt' not in ['declined.txt', 'main_work.txt', 'seed.txt']

Three changes, all at the root: ownership is now decided by agent._wt_pending, the release guard is no longer gated on the run's mode, and the "what changed in this worktree" probe no longer returns an empty list for a tab whose current run happens to be worktree-free — left alone, that last one would have made the new release path discard real work instead of publishing it.

"Auto commit" switched off still committedinconsistency

WorktreeSorcarAgent.auto_commit_enabled was hardcoded True, and it guarded messages that blamed a --no-auto-commit command-line flag that does not exist. The user's toggle never reached the agent at all. The visible consequence: with auto-commit off, typing a follow-up prompt silently committed and squash-merged the previous task. The toggle now arrives as a run(auto_commit=…) argument, and an explicit click on the Merge button still commits, because refusing the user's own merge click was itself a defect in the first attempt at this fix.

The independent review found two paths where the per-run value still lost to the value persisted on disk: the post-task commit on a non-worktree run ran unconditionally, even when the task had failed, and the pre-run retirement of a carried-over worktree obeyed whatever the previous run had bound. Both now read the same state.auto_commit_mode the runner uses for its own decision. Two existing tests had written the old behaviour down as policy — one docstring said a dirty main tree is "ALWAYS auto-committed … auto-commit toggle and task outcome notwithstanding" — which contradicted the public API contract in server/sorcar.py. They were rewritten against the contract.

A sub-agent's last file could be deleted by the parent's cleanuprace

When a parallel sub-agent is abandoned, the parent waits up to five seconds for it before tearing down the worktree they share. The wait happened after the final staging pass, so a child that wrote a file and finished inside that window had its work staged by nobody and then removed with the directory. The reproduction abandons a real child agent thread, releases it half a second into the parent's five-second wait, and observes COMMITTED_AND_REMOVED with the file gone from disk and from the branch. Cleanup now reclaims first, then commits.

A booting daemon marked another live process's task as killedrace

On startup the daemon sweeps the task history for rows still marked running and rewrites them as "process killed", on the theory that a running row without a live task must be debris from a crash. Liveness was judged from the booting process's own in-memory registry, which knows nothing about any other process. Start a second daemon, or a second VS Code window, and the first one's running task was relabelled as killed — and stickily, because the rewrite is what the next sweep reads.

Liveness is now a fact on disk. Each process mints a token, holds an exclusive flock on <KISS_HOME>/task-owners/<token>.lock for its whole life, and stamps the token into every row it inserts. The kernel releases that lock however violently the process dies, so a dead owner is provable rather than assumed. The first implementation of this had its own bug, caught by its own test: it updated rows by id, a TEXT primary key, and SQLite permits NULL there, so rows written by an older release could never be targeted — 0 of 300 recovered. It works on rowid now.

Shutdown could abandon a task, or interrupt a merge halfwayrace

Three sweeps asked "is this task alive?" with a narrower test than the rest of the codebase uses. A worker that had started but not yet flagged itself busy was invisible to all three, which meant SIGTERM could abandon it mid-run, and the extension's activeTasksQuery could report an idle daemon and restart it on top of a task that had just begun. All three now use the canonical AgentState.busy().

Separately, stop_async() returned while an interactive merge was still mutating git — measured at 9 ms, with the merge still committing. Merge threads are now published on the agent state and joined on all three shutdown paths, with a 30-second bound. They are waited for, never interrupted: half a stash-commit-checkout-merge is worse than all of it.

Constructing a second server wiped the first one's task registryinconsistency

VSCodeServer.__init__ called agent_states.clear() on a process-global registry. Anything embedding two servers in one process — which the test suite does routinely, and an embedder may do — lost every live task's state on the second constructor call, including the exemption that keeps a live task out of the orphan sweep. It is now a selective purge: busy entries stay, finished ones go.

A tab's input box could stay disabled foreverinconsistency

Two independent paths did this. The daemon refuses a run while a merge is in flight, and it sent the error without the matching status running:false — but the client had already optimistically marked the tab as running when the user pressed Enter, so the composer stayed locked with no agent behind it. The refusal now clears the flag first, matching the sibling refusal a few hundred lines away that always did.

The second path was created by this very change set. Reconnect handling grew a time-to-live and a queue cap, and dropped frames silently. The review found that a run dropped by either rule left the tab spinning permanently, because the extension host posts running:true before the daemon has seen anything. The client now emits commandDropped(cmd, reason) for both discard paths, the sidebar unwinds the tab with a warning naming the reason, and a dropped commit-message request fails its promise and its countdown instead of hanging.

Two windows killed each other's booting daemonrace

Open two VS Code windows on the same machine and both may decide the daemon needs restarting. Each probes the port, kills what it finds, and bootstraps — including the process the other one had just launched. The first fix was an O_EXCL lock file. The review then showed that lock could be stolen from a live owner: staleness was judged from the file's mtime after 120 seconds, while the startup verifier alone may run for 180, plus however long a laptop was asleep. Worse, the release path unlinked the file unconditionally, so once a lock had been broken the original owner's release deleted its successor's lock and let a third window in. The lock now carries {pid, token}, breaks only on a provably dead owner (an EPERM from kill(pid, 0) counts as alive) or a 600-second backstop, and unlinks only on an exact token match.

The commit-message button: blank box, wrong repositoryraceinconsistency

Clicking the sparkle in the SCM view twice discarded the real answer and left the box empty, because the in-flight guard was registered after an await. It is now registered synchronously and the second caller joins the first promise. The review then found a second, quieter half: the command resolved the repository VS Code passed in, used it to detect staged changes and to pick the input box, and then asked the sidebar to generate — without passing it. The sidebar sent the workspace folder. In a multi-root or submodule setup, clicking the sparkle in repository B asked the daemon to diff A. Both the work directory and a per-repository tab id are now threaded through, which also stops two repositories' concurrent requests from collapsing into one.

Progress text nobody rendered, and a progress line nobody clearedinconsistency

The daemon broadcast autocommit_progress ("Staging changes…", "Generating commit message…", "Committing…") three times per non-worktree task, and worktree_progress during a merge. Nothing outside the VS Code extension host rendered either, so the remote browser and mobile clients showed a frozen UI through however long a merge takes. The webview now renders both.

Then a follow-up audit of that fix found the harder half. Both flows run inside except BaseException handlers that log and continue, and the progress events are emitted from inside the call they wrap. A git binary that dies, an LLM call that throws while writing the commit subject, or a stopped task unwinding through the merge leaves the transcript reading "Staging changes…" for ever — not as a stale label, but as if the operation were still running. The guaranteed events are the task-end ones, so the line is now cleared from all four of them, routed to the owning tab's fragment rather than whatever is on screen. Two of that suite's five cases are negative guards: another tab's task ending must not cancel this tab's live merge line.

The step counter ran backwardsredundancy

The webview's stream state machine existed in four hand-maintained copies: the visible tab, background tabs, replay, and a fourth that only counted steps. They had already drifted — the counter seeded one flag false where the renderer it shadowed seeded it true. Collapsing them into one context object exposed a real bug that none of the four had: the shared code adopted the daemon's step count from a result event but had no equivalent rule for the live usage report, so after every usage line the panel counting resumed from its own stale base. On a run_parallel fan-out, where the monitor adds every sub-agent's steps to the parent's, the header visibly went backwards:

thinking_start  "Steps: 1"
usage_info      "Steps: 43"
thinking_start  "Steps: 2"      <-- regression

run_parallel is on by default, so most tasks could show this. One rule in the one shared place fixed it for the visible tab, background tabs and replay at once. The ctx.stepCount > 0 guard in that rule is load-bearing, and the note in the fix report says why: zero is also the flag that tells the renderer the first Thoughts panel is still to be opened, and the monitor commonly reports a step in progress before the first token arrives.

Smaller client defects, same shapeinconsistency

• A late or unsolicited configData push painted over whatever the user was typing in Settings. Edited fields are now tracked and skipped, and only touched fields are sent — safe because the config save merges server-side.
• The budget box kept its own default of 100. Two user-visible consequences, both present before any drift: a user on a 250-dollar budget who cleared the box to retype it and then closed the panel was put back on 100, and parseFloat(v) || 100 swallowed a legitimate 0. The box now shows the daemon's number and says nothing when it has none.
• Pending file-link state grew without bound and a lost reply left links grey for the rest of the session; a reconnect never re-asked. It now re-asks, batched per tab and work directory, and prunes on clear, close and welcome.
• Two of eleven file-link call sites dropped the owning tab id, so a background tab's path check was attributed to whatever tab was visible.
• The reconnect loop retried at a flat 500 ms for ever and replayed an unbounded queue — in the worst case pushing a queued run into a different daemon process minutes later. It now backs off to a 15-second cap with jitter, resets only after a connection has been stable for 5 seconds, and expires queued frames after 10 seconds with a 256-frame cap.

Gemini could not be stopped and had no stall timeout at allinconsistency

Every other network adapter reads the thread's stop signal and bounds a silent stream. Gemini read neither. Pressing Stop during a Gemini turn did nothing until the provider finished on its own; a wedged gateway held the turn open indefinitely. The adapter now iterates its stream through the shared abort wrapper, with two independent clocks. The second one is not belt-and-braces: the installed SDK skips blank lines while reading the event stream, so ordinary SSE keep-alives — and whatever an eager proxy emits to hold a connection open — are real bytes that reset the transport's read clock while yielding the adapter nothing. Reproduced before fixing, with a harness that sends nothing but blank lines:

generate() was still reading a keep-alive-only stream 20.0s later —
stream_stall_timeout=2.0s is not enforced at the event level, only at the byte level

Two more Gemini defects came with it. Reasoning text was copied into the assistant message on the tool-calling path but not on the plain path, so a tool-using conversation carried its own thoughts forward and paid for them as input tokens on every later step. And usage was read from the last chunk of the stream rather than from the chunk that carries usage. Both paths now share one parts parser, so they cannot disagree again.

The command-line adapters: a reader thread that outlived its turnrace

claude and codex are driven as child processes. Six defects sat in that plumbing, and they compounded.

• Parsing ran on a background reader thread that was joined for five seconds and then abandoned. A stand-in CLI that spawns a grandchild inheriting stdout keeps the pipe open after kill(), so the abandoned reader really did survive — and emitted ['LATE'] into the next run's callbacks. Parsing now happens on the calling thread; the background threads only move bytes, so an abandoned reader has nothing to emit with.
• Children were killed without being reaped and pipes were never closed. Twenty timed-out calls left unreaped children: [29959] for codex and two for claude, counted from real ps output and real /dev/fd entries.
• stderr was piped and never drained, so a chatty CLI filled the pipe and deadlocked. The failure was then reported to the user as a stall: timed out after 20s for a process that was perfectly healthy and had written 200 KiB of diagnostics.
• Neither adapter read the stop signal, and a stall raised a non-retryable error. The agent loop re-raises that class and retries others, so a transient hiccup during a cc/* step aborted the whole task where the equivalent Anthropic condition would have retried the step. A stall is now a plain TimeoutError.
• One adapter streamed raw {"tool_calls": …} JSON to the user while the other stripped it. Both now use one filter that passes thinking through, streams text incrementally, and buffers each top-level JSON object until it closes before deciding whether to drop it.
• Usage getters returned None on a JSON null instead of zero.

Then the independent review found the hole in the new supervisor. The turn deadline and the stop check lived in the read loop, and the prompt was written to the child's stdin before that loop was entered. A child that stays alive but never reads stdin fills the pipe buffer, and the write blocks with no timeout and no stop check — reproduced with a 2 MB prompt and a 0.2-second timeout, the writer thread still blocked 1.5 seconds later. The write now runs on its own thread, polled against the same two conditions the read loop obeys.

A stalled stream was returned as a truncated successinconsistency

The abort wrapper accepted a stall_timeout, documented it, and never read the watchdog's stalled flag — and no caller passed the argument anyway. A stream that went quiet mid-answer therefore came back as a short but successful reply, and the agent carried on with a truncated turn. It now raises a retryable timeout, and all three call sites pass the bound. Related: the watchdog could fire up to one poll interval after the caller was finished, shutting down a socket already back in the connection pool. Claiming the abort and disarming it are now serialised under one lock — but the socket teardown itself runs outside that lock, because the first version of this fix held the lock across teardown and made Stop flaky one run in five.

The retry rendered its answer as reasoninginconsistency

The OpenAI-compatible stream loop closed its "thinking" bracket on normal exhaustion and on a stop or a stall, but not on an ordinary transport failure. The agent retries such failures inside the same run, so the retry's plain answer was printed inside the dead stream's Thinking block. The reproduction renders it through a real console printer and quotes the damage:

the answer was rendered inside the dead stream's thinking block —
no closing rule precedes it:
'──────── Thinking ────────\n\nLet me thinkTwo plus two is four'

The bracket now closes in a finally, and a redundant local flag that shadowed the real one is gone. The test that had previously covered this area asserted the leak as setup — "test setup no longer reproduces the leak" — which the turn-level fix makes impossible; it was rewritten to assert the stronger guarantee.

Delegate and cache defects in the OpenAI transportsredundancy

The Chat Completions adapter delegates some turns to a cached Responses-protocol sibling. That sibling captured its callbacks at construction, so on the second run of a reused model it streamed into the previous run's printer — the tokens went nowhere visible. It also rebuilt a whole HTTP client and connection pool before every delegated step (five distinct TCP peers for five steps), never checked finish_reason so truncated tool arguments silently became {}, raised a bare IndexError on an empty choices list, and kept a raw-item cache that grew by one whole turn per tool call and survived a conversation reset.

One shared cache decided whether an endpoint accepts reasoning_effort alongside tools. It was an unsynchronised process-global read-modify-write straddling an HTTP round trip, keyed by base URL alone — so two models on one endpoint contaminated each other, and parallel sub-agents raced: last writer won. It is now locked, keyed by endpoint and model, with compare-and-set semantics where a rejection is definitive.

The model catalog could be read tornrace

Two JSON files were written with a truncating open and read with an unguarded json.loads at import time. A reader that arrived mid-write got a parse error; worse, the seeding of the user-model file was a check-then-act, so a concurrent importer could silently lose every user-defined model. Both writes are now staged and published with os.replace, and the reader retries briefly and then raises an error that names the file. The review added the last piece: valid JSON of the wrong shape (null, [], 42) escaped as a raw TypeError from a function whose docstring promised a classified error.

Provider routing was the redundancy half of the same file. One registry was described in the source as the single source of truth, and three hand-written prefix tables restated it — one of them as an if-chain. A fourth copy lived in the test helper that decides which live tests to skip. There is one lookup now, and measuring the helper against the catalog's 601 entries before changing it showed nine behavioural differences, all codex/*, where the old copy demanded an API key that the Codex CLI does not use.

Two copies of the parallel fan-out engineredundancy

The engine that runs sub-agents in a thread pool existed twice: once on the base agent, which around twenty third-party channel agents inherit, and once on the chat agent, which had received four fixes the base copy never got. The consequences were not cosmetic.

• Every child was handed the parent's stop event, so stopping one sub-agent killed the parent and its siblings.
• There was no KeyboardInterrupt arm, so stopping one child discarded its finished siblings' results.
• The parent task id was the empty string, which the history reader interprets as "top-level task" — so every sub-task from the base path appeared as its own row in the user's task list. The review found the remaining half of this after the merge: a plain parent, which is what every channel agent is, still produced blank parent ids.
• Chat and tab identity were not propagated, yet the "sub-agent done" event named a tab nothing had opened.

One engine now, parameterised. About 150 lines of the duplicate went, along with a duplicated stop-event class.

Persistence: transactions, journals and a 500 Hz pollrace

Beyond the orphan sweep, the history database had four more problems. Multi-statement read-modify-write sequences ran in autocommit under an in-process lock only, so two processes could exceed a hard cap — 101 rows where the cap is 100, reproduced with two real processes released by a barrier. Ten no-op commit() calls that read as durability guarantees were deleted and the sequences wrapped in BEGIN IMMEDIATE. Events that the database permanently refused were acknowledged as flushed, i.e. lost; they are now journalled beside their own database file and replayed once writes are accepted. A cross-file idempotence check held only within one process. And the flush waited by polling a counter five hundred times a second: an idle task's flush took 29.9 seconds behind a busy one, now under 0.3.

The review then showed the journal replay was not safe between processes: two of them could persist the same batch twice, or one could delete events the other had just journalled. Replay now takes an inter-process lock and claims the journal by renaming it, so the file it deletes is exactly the file it read. A snapshot the database still refuses is renamed back.

Docker mode dropped its own guaranteesinconsistency

In the sandboxed mode, Write returned "Successfully wrote 24 characters" for a file that was never created, because the timeout path carries no exit code and success was assumed. Bash ignored timeout_seconds entirely on the path the product uses — sleep 30 with a 2-second timeout blocked for the full 30 — and never truncated output, so a chatty command could flood the model's context. All four output decodes were strict UTF-8, so a single binary byte raised UnicodeDecodeError out of the tool, and a multi-byte character split across two stream frames did the same. Write now proves success with a sentinel, Bash honours both limits, and decoding is incremental with replacement.

The timeout kill took two attempts. The first used the pid from Docker's exec inspection, which on macOS is a pid inside the Docker VM's namespace — it would have killed nothing, or something unrelated. The exec is now tagged with a token in its environment and the kill script matches /proc/<pid>/environ, using only shell builtins and tr, because slim images have no ps or pkill.

MCP tool servers leaked, went dead, and were evicted mid-callrace

Each configuration revision spawned a new stdio child and never reaped the old one, and the pool had no cap. After a server was killed, the connection object still held what looked like a live session, so the next tool call blocked until the 305-second timeout rather than reconnecting. Connections now carry a last-used stamp, idle ones are reaped, the pool is LRU-trimmed to eight, an idle connection is pinged so a dead server surfaces promptly, and a call reconnects on demand. Eviction is deliberately not by server name: two projects may each configure a server called github, and evicting by name would make them thrash.

The review found the flaw in that cap: it could tear down a connection with a tool call in flight, stranding the call for the full 305 seconds — the reproduction took 121 seconds to demonstrate. Connections now carry a lease counter, and leased connections are excluded from both the idle sweep and the over-cap list. Separately, nothing in the daemon ever disconnected these servers, so the children were reaped only by an atexit hook that a kill never runs; a real stdio server was still answering pings 30 seconds after its daemon stopped.

The browser profile guard was per-processrace

Chromium's user-data directory is machine-global; the guard around launching it was a thread lock. Two processes resolved the same profile, the second deleted a live singleton lock, and Chromium refused: Failed to create a ProcessSingleton for your profile directory … Aborting now to avoid profile corruption, exit 21. The launch now holds a machine-wide file lock across cleanup, resolution and launch.

One console printer, many sub-agent threadsrace

The console printer is handed, unchanged, to every parallel sub-agent. Its mid-line, bash-streaming and current-block state was plain instance state, so siblings interleaved into each other's output. The reproduction caught a raw write spliced into a panel border:

'rawtextrawtext…rawtext╭──── Bash ────╮'

Per-thread state, plus one lock around the genuine shared cursor, fixed the rendering. The review then pointed out that the same class's token, cost and step offsets — the numbers printed in a result panel — were still process-shared, so the last sub-agent to start controlled every sibling's rendered totals:

the sibling's token offset won:
╭──── Result ────╮
│  early message                 │
╰── tokens=910  cost=$9.1000 ─╯

Before moving them, every reader and writer was traced to confirm each runs on the printing agent's own thread — including the live usage monitor, which prints from its own daemon thread but computes its totals itself and reads no offset, so the status line could not regress.

Trajectories written non-atomically, and a global YAML mutationrace

Agent trajectories were written with a truncating open("w") and streamed into. A concurrent reader — the visualizer is one — saw partial files; the reproduction observed truncated reads of 451,168, 121,325 and 95,863 bytes among others. There is now one atomic write helper, and a duplicated 22-line copy of the same staging logic elsewhere in core now delegates to it.

The review found the helper's own bug: it called os.write() once and ignored the returned count. POSIX permits a short write, so the helper could atomically publish truncated content — precisely the outcome it existed to prevent. Reproduced in a child process with a real 1 KiB file-size limit and SIGXFSZ ignored, so that write(2) returns its partial count instead of killing the process: {'requested': 4096, 'published': 1024}. It now uses the buffered pattern the model-catalog writer had already chosen — the inconsistency was inside the same commit.

Separately, importing one core module registered a representer on PyYAML's process-global dumper, so merely importing the agent framework changed how an unrelated part of the host program serialised YAML. The style is now applied through a private dumper subclass, and only to genuinely multi-line strings rather than to every key and one-liner.

Settings that could not be saved and a budget with two defaultsinconsistency

save_config wrote only keys present in its defaults table, so four live settings — the tunnel token, skill and MCP permissions, and the email address — were silently dropped on every save. Refreshing the config after saving an API key rebuilt the config object and reverted a max_budget that had just been applied. The budget default itself was written in four places with two different values, 100 and 200; there is one authoritative constant now, and the last hardcoded literal in the daemon and the three in the client were removed too. A retired key was still being read back and re-persisted. And an artifact-directory setter with no production caller — the only runtime mutator of a process-global — was deleted, which makes a whole class of mid-run relocation unconstructible rather than merely untested.