Skip to content

refactor(coding-agent): remove the test-only daemon-lookup seam from main.ts - #1824

Closed
snimu wants to merge 108 commits into
mainfrom
snimu/remove-daemon-lookup-fake
Closed

refactor(coding-agent): remove the test-only daemon-lookup seam from main.ts#1824
snimu wants to merge 108 commits into
mainfrom
snimu/remove-daemon-lookup-fake

Conversation

@snimu

@snimu snimu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What was wrong

main.ts's interactive-startup wrapper accepted an optional injected daemon-lookup function that only three unit tests ever passed — a test-only seam in production code. Those tests asserted the wrapper's own try/catch branches through the fake, i.e. they restated the implementation (audit: test-only production code, test-only.md finding 5).

The fix

+2/−48: the lookup type and option are deleted, findActiveDaemonSessionSummary is called directly, the three fake-lookup tests and their import are gone, and the function's now-unimported export keyword is dropped. The real fallbackOnError policy and its production caller are byte-identical.

How it's verified

Reviewer confirmed no other caller passed the option, the deleted tests covered no user-relevant obligation, and the remaining 52 routing tests pass. tsgo/biome clean; full CI-style suite failure set identical to stack base (two unrelated full-load flakes pass focused on both branches). Two-model implement/review loop, approved first pass.

Stacked on #1737 (test the whole stack at the leaf; merge base-first).

Note: intentionally no Linear ticket for this cleanup stack, so that check stays red.


Note

Medium Risk
Changes agent turn timing, compaction continuation, cross-daemon messaging, and RPC timeout defaults in core session/daemon paths; telemetry may run in test unless explicitly disabled.

Overview
This PR tightens coding-agent runtime and TUI behavior while stripping test-only APIs and several correctness bugs around daemons, compaction, and child agents.

Interactive queue editing no longer mirrors the queue in connectionQueue. The TUI reads connectionState.sessionActions and revalidates the selected entry with QueueSelection.refreshAt after mutations or session_action_update events, so duplicate prompts and optimistic double-patches are avoided. RpcClient drops default request timeouts (including refine); long turns wait until idle unless callers pass an explicit timeout, and transport/process death fails all pending waits.

AgentSession removes the post-compaction 100ms delay and runs continuation behind idle/retry/refine gates, optional session-input draining, and commit fences. RLM child snapshots are built in-session (activity, previews, retained runs) and the daemon attach path maps activeSessionId from resident children. Legacy rlm-subagents.jsonl entries without sessionDir hydrate via readLegacyRlmSubagentRegistry. Kernel graceful dispose waits on the execution queue (bounded by SNAPSHOT_EXECUTION_TIMEOUT_MS) before a final snapshot instead of racing a dispose timeout.

Daemon/worker fixes: remote send_message connects once and does not resend after a post-send disconnect; supervised renames on workers apply names without a second availability check; passivation no longer re-registers children on failed close. looksLikeSessionPath is shared from session-resolver. Removed: injectable daemon lookup in main, clearConfigValueCache, getOverflowPatterns, and implicit NODE_ENV=test telemetry opt-out (tests should use DO_NOT_TRACK / env overrides).

Reviewed by Cursor Bugbot for commit 2e87f0f. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Remove test-only seams and rework queue, compaction, RPC in coding-agent

  • Removes test-only seams: the exportable findActiveDaemonSessionSummaryForInteractiveStartup lookup override in main.ts, clearConfigValueCache/clearApiKeyCache in resolve-config-value.ts and model-registry.ts, and the NODE_ENV === 'test' telemetry opt-out in telemetry.ts
  • Reworks InteractiveMode to derive the connection queue from connectionState.sessionActions via getConnectionQueue() instead of maintaining a local mirror; move and edit flows now consult authoritative state and avoid optimistic local patching (interactive-mode.ts, queue-selection.ts)
  • Removes the artificial 100 ms delay from post-compaction continuation in AgentSession._runScheduledPostCompactionContinue; the method now coordinates with queued work, compaction races, and ownership without a fixed timer (agent-session.ts)
  • Reworks RpcClient to propagate transport/process failures to all pending operations via transportError, removes per-request timeouts from send(), and makes waitForIdle/promptAndWait/collectEvents cancellable with optional timeouts (rpc-client.ts)
  • Risk: RpcClient.send() no longer applies a default timeout; long-running commands wait indefinitely unless the caller passes an explicit timeout. InteractiveMode no longer optimistically patches the queue mirror, so UI updates depend on server responses. isTelemetryEnabled may return true during tests unless DO_NOT_TRACK=1 is set (now added to vitest.config.ts)

Macroscope summarized 2e87f0f.

Linear ticket: ENG-5658
(ticket linked above)


Supersedes #1738 (recreated as a plain PR against main; GitHub's stack lock prevented retargeting the stacked PR).

snimu added 30 commits August 24, 2026 11:17
snimu added 23 commits August 27, 2026 10:17
/** Best-effort final snapshot before a graceful dispose, bounded by a timeout. */
private async flushSnapshotForDispose(): Promise<void> {
if (!this.options.snapshot || !this.isRunning) return;
const pendingExecutions = this.executionQueue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High kernel/repl-manager.ts:1104

dispose() and shutdown({ snapshot: true }) can wait indefinitely instead of honoring SNAPSHOT_EXECUTION_TIMEOUT_MS. flushSnapshotForDispose() captures executionQueue before waiting, so a concurrent execute() can enqueue a non-terminating request afterward; captureSnapshot() queues behind it, and its timeout does not start until that request finishes. Reserve the queue before awaiting it (or otherwise block new executions) so the final snapshot cannot be overtaken.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/core/kernel/repl-manager.ts around line 1104:

`dispose()` and `shutdown({ snapshot: true })` can wait indefinitely instead of honoring `SNAPSHOT_EXECUTION_TIMEOUT_MS`. `flushSnapshotForDispose()` captures `executionQueue` before waiting, so a concurrent `execute()` can enqueue a non-terminating request afterward; `captureSnapshot()` queues behind it, and its timeout does not start until that request finishes. Reserve the queue before awaiting it (or otherwise block new executions) so the final snapshot cannot be overtaken.

Evidence trail:
packages/coding-agent/src/core/kernel/repl-manager.ts:434-450, 453-497, 897-910, 1002-1020, 1102-1117 at REVIEWED_COMMIT

// The session transition and transcript are already authoritative here;
// a transient queue read must not turn a successful switch into a fatal error.
await this.refreshConnectionQueue().catch(() => undefined);
this.updatePendingMessagesDisplay();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium interactive/interactive-mode.ts:2885

During session replacement, a queue update from another client can be overwritten by the stale state returned from getInitialSnapshot(), leaving pending previews and queue-edit indices/text stale until another queue event arrives; subsequent edits are then rejected. renderCurrentSessionState() should refresh the authoritative queue after rendering the snapshot instead of only calling updatePendingMessagesDisplay().

- 		this.updatePendingMessagesDisplay();
+		await this.refreshConnectionQueue().catch(() => undefined);
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/modes/interactive/interactive-mode.ts around line 2885:

During session replacement, a queue update from another client can be overwritten by the stale state returned from `getInitialSnapshot()`, leaving pending previews and queue-edit indices/text stale until another queue event arrives; subsequent edits are then rejected. `renderCurrentSessionState()` should refresh the authoritative queue after rendering the snapshot instead of only calling `updatePendingMessagesDisplay()`.

Evidence trail:
packages/coding-agent/src/modes/interactive/interactive-mode.ts:2878-2887, 5060-5111, 6555-6589, 2580-2600, 2640-2661, 7385-7423 at e29065245ef3209669ab7e64c3f478a8daf94d8e; git diff MERGE_BASE..REVIEWED_COMMIT -- packages/coding-agent/src/modes/interactive/interactive-mode.ts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium

A stale rejected queue mutation leaves queueSelection pointed at the removed or moved item, so the next Enter/Alt+Enter retries the same obsolete (lane, index, text) tuple until the user exits browse mode. Reset the selection when restoring a rejected edit so subsequent submissions operate on the current editor draft.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/modes/interactive/interactive-mode.ts around line 7127:

A stale `rejected` queue mutation leaves `queueSelection` pointed at the removed or moved item, so the next Enter/Alt+Enter retries the same obsolete `(lane, index, text)` tuple until the user exits browse mode. Reset the selection when restoring a rejected edit so subsequent submissions operate on the current editor draft.

Evidence trail:
packages/coding-agent/src/modes/interactive/interactive-mode.ts:2640-2645, 7057-7061, 7086-7138 at e29065245ef3209669ab7e64c3f478a8daf94d8e
packages/coding-agent/src/modes/interactive/queue-selection.ts:24-30, 76-83 at e29065245ef3209669ab7e64c3f478a8daf94d8e
packages/coding-agent/src/core/agent-session.ts:6371-6404 at e29065245ef3209669ab7e64c3f478a8daf94d8e

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2e87f0f. Configure here.

if (
this.connectionQueue === queueBefore &&
lane[selected.index] === selected.text &&
target >= 0 &&

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Queue browse lost after move race

Medium Severity

A successful queue reorder can still drop browse mode when the mutation response arrives before session_action_update. refreshQueueSelectionAt then reads the stale snapshot at the predicted index, treats the text mismatch as a lost item, and restores the draft. pendingQueueMove also suppresses the late event from repairing selection, so chained moves/edits no-op even though the server applied the reorder.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2e87f0f. Configure here.

@snimu

snimu commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Superseded: the chained stack was restructured into independent PRs (byte-identical combined tree). A fresh standalone PR for this change follows on the same branch name.

@snimu snimu closed this Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant