Skip to content

Commit df028fe

Browse files
authored
🤖 refactor: segregate the task/workspace seam into role interfaces (#4012)
## Summary Splits the 36-method `WorkspaceHost` grab-bag on the task/workspace seam into five role interfaces named for what task-side callers do (`WorkspaceTurnHost`, `TurnAdmissionHost`, `WorkspaceLifecycleHost`, `WorkspaceProvisioningHost`, `WorkspaceMetadataHost`), keeps `WorkspaceHost` as their intersection so no call site or wiring changes, and collapses the ~180-line hand-rolled test mock onto one shared `makeWorkspaceHostFake`. ## Background #3996 cut the taskService/workspaceService dependency cycle at a typed seam, but the seam stayed shallow: one interface mirroring 36 of WorkspaceService's internal mechanics. Every task test stubbed all 36 methods through a ~180-line mock in `taskService.test.ts` (reached from 318 call sites), and the seam, service, and test harness churned in lockstep on every change. This builds on #3996 rather than reverting it: same seam, deeper interface. Refactor #5 from the 2026-08-29 architecture review (evidence at main @ f04e0f8). ## Implementation - `taskWorkspaceSeam.ts`: the five role interfaces group methods by caller intent (turn execution, queue/admission probes, archive/remove lifecycle, child-workspace provisioning, metadata/events). Every method signature is byte-identical to before; `WorkspaceHost` is now `WorkspaceTurnHost & TurnAdmissionHost & WorkspaceLifecycleHost & WorkspaceProvisioningHost & WorkspaceMetadataHost`. `TaskService` (the only production consumer) legitimately uses all five roles, so its single constructor param stays; new narrow consumers can now depend on one role instead of the full host. - `taskWorkspaceSeam.testUtils.ts`: adds framework-free `makeWorkspaceHostFake(overrides)` beside the existing `makeAgentTaskIntegrationFake`, carrying the harness's default stub semantics (granted archive hold, "keep"-style snapshot eligibility, sanitizer no-op). - `taskService.test.ts`: `createWorkspaceServiceMocks` shrinks from ~180 lines to ~55 on top of the shared fake, with a mapped type over `keyof WorkspaceHost` replacing the hand-written 36-entry overrides list. Returned mock handles and the archive/remove locked-sink aliasing are preserved, so all 318 harness call sites are untouched. ## Net LOC delta vs main (f04e0f8) - Production (`taskWorkspaceSeam.ts`): **+18** (+103/-85) - Tests (`taskService.test.ts` + `taskWorkspaceSeam.testUtils.ts`): **-76** (+112/-188) - Overall: **-58** Irreducible production additions: the five role interface declarations plus the intersection type (the point of the refactor), and the archive race-invariant docs on `ArchiveWorkspaceOptions`, which review feedback correctly required keeping verbatim rather than counting as savings. Test additions are `makeWorkspaceHostFake`'s default bodies (moved from the harness, now reusable by any seam consumer's tests). ## Validation - Remote dogfood UAT ran against the pushed SHA and passed: sub-agent spawn/report/interrupt/reawaken, workspace turns (new + queued follow-up race), archive/unarchive including `interrupt_active` and live-activity refusal, heartbeats, tree listing, and monitor wakes. The UAT runner additionally verified the emitted JavaScript is byte-identical between base and feature for the production file. - Whole-file `bun test` of `taskService.test.ts`, `workspaceService.test.ts`, `heartbeatService.test.ts`, `tools/task_list.test.ts`: the fail set is identical to a clean worktree at base f04e0f8 (3 pre-existing host-environment failures; none branch-attributable). ## Risks Low. The production change is type-only interface restructuring with byte-identical emitted JS; regression surface is the test-harness consolidation, which preserves each mock's default behavior and aliasing semantics. --- _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh` • Cost: `$29.31`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh costs=29.31 --> ## Stack Layer 4/10 of the architecture refactor stack (net -5,101 LOC overall). This PR's diff is only this layer, against `mike/arch-peer-message-broker`.
1 parent ad7f569 commit df028fe

3 files changed

Lines changed: 215 additions & 273 deletions

File tree

‎src/node/services/taskService.test.ts‎

Lines changed: 56 additions & 187 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,10 @@ import {
8181
WORKFLOW_RUN_CARD_DISPLAY_METADATA_TYPE,
8282
} from "@/common/utils/workflowRunMessages";
8383
import type { WorkspaceMetadata } from "@/common/types/workspace";
84-
import type { ProvidersConfigMap, WorkspaceChatMessage } from "@/common/orpc/types";
84+
import type { ProvidersConfigMap } from "@/common/orpc/types";
8585
import type { AIService } from "@/node/services/aiService";
8686
import type { WorkspaceHost } from "@/node/services/taskWorkspaceSeam";
87+
import { makeWorkspaceHostFake } from "@/node/services/taskWorkspaceSeam.testUtils";
8788
import type { InitStateManager } from "@/node/services/initStateManager";
8889
import { InitStateManager as RealInitStateManager } from "@/node/services/initStateManager";
8990
import assert from "node:assert";
@@ -519,193 +520,61 @@ function simulateAcceptedFamilySends(
519520
);
520521
}
521522

522-
function createWorkspaceServiceMocks(
523-
overrides?: Partial<{
524-
sendMessage: ReturnType<typeof mock>;
525-
resumeStream: ReturnType<typeof mock>;
526-
clearQueue: ReturnType<typeof mock>;
527-
removeQueuedWorkspaceTurn: ReturnType<typeof mock>;
528-
removeQueuedMessagesByDedupeKeyPrefix: ReturnType<typeof mock>;
529-
hasQueuedWorkspaceTurn: ReturnType<typeof mock>;
530-
hasQueuedMessages: ReturnType<typeof mock>;
531-
isBusyForMessage: ReturnType<typeof mock>;
532-
hasPendingQueuedOrPreparingTurn: ReturnType<typeof mock>;
533-
hasPendingBashMonitorWakeContinuation: ReturnType<typeof mock>;
534-
hasPendingWorkspaceTurnContinuation: ReturnType<typeof mock>;
535-
getQueueCutCutter: ReturnType<typeof mock>;
536-
hasPendingAutoRetry: ReturnType<typeof mock>;
537-
waitForIdleAndNoQueuedMessages: ReturnType<typeof mock>;
538-
waitForPendingCompactionCompletionDecision: ReturnType<typeof mock>;
539-
waitForPendingStreamErrorRecoveryDecision: ReturnType<typeof mock>;
540-
archive: ReturnType<typeof mock>;
541-
unarchive: ReturnType<typeof mock>;
542-
preflightArchive: ReturnType<typeof mock>;
543-
listLiveWorkspaceActivity: ReturnType<typeof mock>;
544-
hasRunningBackgroundBashProcesses: ReturnType<typeof mock>;
545-
isSnapshotArchiveEligibilityMutationSensitive: ReturnType<typeof mock>;
546-
hasUntrackableExternalAppOpen: ReturnType<typeof mock>;
547-
acquirePreInterruptionArchiveHold: ReturnType<typeof mock>;
548-
remove: ReturnType<typeof mock>;
549-
emit: ReturnType<typeof mock>;
550-
getInfo: ReturnType<typeof mock>;
551-
replaceHistory: ReturnType<typeof mock>;
552-
updateTitle: ReturnType<typeof mock>;
553-
isExperimentEnabled: ReturnType<typeof mock>;
554-
emitChatEvent: ReturnType<typeof mock>;
555-
isWorkflowInvocationCurrent: ReturnType<typeof mock>;
556-
create: ReturnType<typeof mock>;
557-
countQueuedAgentPeerMessages: ReturnType<typeof mock>;
558-
}>
559-
) {
560-
const sendMessage =
561-
overrides?.sendMessage ?? mock((): Promise<Result<void>> => Promise.resolve(Ok(undefined)));
562-
const resumeStream =
563-
overrides?.resumeStream ??
564-
mock((): Promise<Result<{ started: boolean }>> => Promise.resolve(Ok({ started: true })));
565-
const clearQueue = overrides?.clearQueue ?? mock((): Result<void> => Ok(undefined));
566-
const removeQueuedWorkspaceTurn =
567-
overrides?.removeQueuedWorkspaceTurn ?? mock((): Result<boolean> => Ok(true));
568-
const removeQueuedMessagesByDedupeKeyPrefix =
569-
overrides?.removeQueuedMessagesByDedupeKeyPrefix ?? mock((): Result<number> => Ok(0));
570-
const hasQueuedWorkspaceTurn = overrides?.hasQueuedWorkspaceTurn ?? mock(() => false);
571-
const hasQueuedMessages = overrides?.hasQueuedMessages ?? mock(() => false);
572-
const isBusyForMessage = overrides?.isBusyForMessage ?? mock(() => false);
573-
const hasPendingQueuedOrPreparingTurn =
574-
overrides?.hasPendingQueuedOrPreparingTurn ?? mock(() => false);
575-
const hasPendingBashMonitorWakeContinuation =
576-
overrides?.hasPendingBashMonitorWakeContinuation ?? mock(() => false);
577-
const hasPendingWorkspaceTurnContinuation =
578-
overrides?.hasPendingWorkspaceTurnContinuation ?? mock(() => false);
579-
const getQueueCutCutter = overrides?.getQueueCutCutter ?? mock(() => undefined);
580-
const hasPendingAutoRetry = overrides?.hasPendingAutoRetry ?? mock(() => false);
581-
const waitForIdleAndNoQueuedMessages =
582-
overrides?.waitForIdleAndNoQueuedMessages ?? mock((): Promise<void> => Promise.resolve());
583-
const waitForPendingCompactionCompletionDecision =
584-
overrides?.waitForPendingCompactionCompletionDecision ??
585-
mock((): Promise<boolean> => Promise.resolve(true));
586-
const waitForPendingStreamErrorRecoveryDecision =
587-
overrides?.waitForPendingStreamErrorRecoveryDecision ??
588-
mock((): Promise<void> => Promise.resolve());
589-
const archive =
590-
overrides?.archive ??
591-
mock((): Promise<Result<{ kind: "archived" }>> => Promise.resolve(Ok({ kind: "archived" })));
592-
const unarchive =
593-
overrides?.unarchive ?? mock((): Promise<Result<void>> => Promise.resolve(Ok(undefined)));
594-
const preflightArchive =
595-
overrides?.preflightArchive ??
596-
mock((): Promise<Result<{ kind: "ready" }>> => Promise.resolve(Ok({ kind: "ready" })));
597-
const listLiveWorkspaceActivity =
598-
overrides?.listLiveWorkspaceActivity ??
599-
mock(() => ({
600-
streaming: false,
601-
queuedMessages: false,
602-
backgroundBashProcesses: false,
603-
terminalSessions: false,
604-
desktopSession: false,
605-
}));
606-
const hasRunningBackgroundBashProcesses =
607-
overrides?.hasRunningBackgroundBashProcesses ??
608-
mock((): Promise<boolean> => Promise.resolve(false));
609-
// Default false = "keep"-style behavior where archive eligibility never depends on the
610-
// untracked-file set, so interrupt_active tests exercise the interruption path.
611-
const isSnapshotArchiveEligibilityMutationSensitive =
612-
overrides?.isSnapshotArchiveEligibilityMutationSensitive ?? mock(() => false);
613-
const hasUntrackableExternalAppOpen =
614-
overrides?.hasUntrackableExternalAppOpen ?? mock(() => false);
615-
const remove =
616-
overrides?.remove ?? mock((): Promise<Result<void>> => Promise.resolve(Ok(undefined)));
617-
const emit = overrides?.emit ?? mock(() => true);
618-
const getInfo = overrides?.getInfo ?? mock(() => Promise.resolve(null));
619-
const replaceHistory =
620-
overrides?.replaceHistory ?? mock((): Promise<Result<void>> => Promise.resolve(Ok(undefined)));
621-
const updateTitle =
622-
overrides?.updateTitle ?? mock((): Promise<Result<void>> => Promise.resolve(Ok(undefined)));
623-
const isExperimentEnabled = overrides?.isExperimentEnabled ?? mock(() => false);
624-
const emitChatEvent =
625-
overrides?.emitChatEvent ??
626-
mock((_workspaceId: string, _message: WorkspaceChatMessage) => undefined);
627-
const isWorkflowInvocationCurrent =
628-
overrides?.isWorkflowInvocationCurrent ?? mock(() => Promise.resolve(true));
629-
const countQueuedAgentPeerMessages = overrides?.countQueuedAgentPeerMessages ?? mock(() => 0);
630-
// Granted by default (no live user activity): interrupt_active tests exercise the
631-
// interruption/archive flow; the hold's own refusal logic lives in workspaceService.test.ts.
632-
const acquirePreInterruptionArchiveHold =
633-
overrides?.acquirePreInterruptionArchiveHold ??
634-
mock((): Result<Disposable> => Ok({ [Symbol.dispose]: () => undefined }));
635-
636-
const create =
637-
overrides?.create ??
638-
mock(
639-
(): Promise<Result<{ metadata: WorkspaceMetadata }>> =>
640-
Promise.resolve(Err("workspaceService.create not mocked"))
641-
);
642-
const discardExtensionMetadataEntry = mock((): Promise<void> => Promise.resolve());
643-
644-
return {
645-
workspaceService: {
646-
create,
647-
discardExtensionMetadataEntry,
648-
// No-op by default: task-create tests exercise launch flow, not the
649-
// registration-time plugin-override sanitizer (workspaceService.test.ts
650-
// covers it). Returning undefined means "clean".
651-
sanitizeMaterializedTaskWorkspace: mock(() => Promise.resolve(undefined)),
652-
sendMessage,
653-
resumeStream,
654-
clearQueue,
655-
removeQueuedWorkspaceTurn,
656-
removeQueuedMessagesByDedupeKeyPrefix,
657-
isBusyForMessage,
658-
hasQueuedWorkspaceTurn,
659-
hasQueuedMessages,
660-
hasPendingQueuedOrPreparingTurn,
661-
hasPendingBashMonitorWakeContinuation,
662-
hasPendingWorkspaceTurnContinuation,
663-
getQueueCutCutter,
664-
hasPendingAutoRetry,
665-
waitForIdleAndNoQueuedMessages,
666-
waitForPendingCompactionCompletionDecision,
667-
waitForPendingStreamErrorRecoveryDecision,
668-
archive,
669-
// Same mocks: the lifecycle path holds the (real) task-tree lock and calls the
670-
// WhileTaskTreeLocked sinks; assertions target one archive/unarchive surface.
671-
archiveWhileTaskTreeLocked: archive,
672-
unarchiveWhileTaskTreeLocked: unarchive,
673-
preflightArchive,
674-
listLiveWorkspaceActivity,
675-
hasRunningBackgroundBashProcesses,
676-
isSnapshotArchiveEligibilityMutationSensitive,
677-
hasUntrackableExternalAppOpen,
678-
acquirePreInterruptionArchiveHold,
679-
// Task launches register their fire-and-forget background inits for archive gating;
680-
// a no-op suffices since these tests archive nothing mid-init.
681-
registerExternalBackgroundInit: mock(() => undefined),
682-
removeWhileTaskTreeLocked: remove,
683-
remove,
684-
emit,
685-
getInfo,
686-
replaceHistory,
687-
updateTitle,
688-
isExperimentEnabled,
689-
emitChatEvent,
690-
isWorkflowInvocationCurrent,
691-
countQueuedAgentPeerMessages,
692-
} satisfies WorkspaceHost,
693-
create,
694-
discardExtensionMetadataEntry,
695-
sendMessage,
696-
resumeStream,
697-
clearQueue,
698-
removeQueuedWorkspaceTurn,
699-
isBusyForMessage,
700-
getQueueCutCutter,
701-
remove,
702-
updateTitle,
703-
emitChatEvent,
704-
emit,
705-
archive,
706-
unarchive,
707-
isWorkflowInvocationCurrent,
523+
type WorkspaceHostMockOverrides = Partial<{
524+
[K in keyof WorkspaceHost]: ReturnType<typeof mock>;
525+
}> & { unarchive?: ReturnType<typeof mock> };
526+
527+
function createWorkspaceServiceMocks(overrides: WorkspaceHostMockOverrides = {}) {
528+
const mocks = {
529+
sendMessage:
530+
overrides.sendMessage ?? mock((): Promise<Result<void>> => Promise.resolve(Ok(undefined))),
531+
resumeStream:
532+
overrides.resumeStream ??
533+
mock((): Promise<Result<{ started: boolean }>> => Promise.resolve(Ok({ started: true }))),
534+
clearQueue: overrides.clearQueue ?? mock((): Result<void> => Ok(undefined)),
535+
removeQueuedWorkspaceTurn:
536+
overrides.removeQueuedWorkspaceTurn ?? mock((): Result<boolean> => Ok(true)),
537+
isBusyForMessage: overrides.isBusyForMessage ?? mock(() => false),
538+
getQueueCutCutter: overrides.getQueueCutCutter ?? mock(() => undefined),
539+
remove:
540+
overrides.remove ??
541+
overrides.removeWhileTaskTreeLocked ??
542+
mock((): Promise<Result<void>> => Promise.resolve(Ok(undefined))),
543+
updateTitle:
544+
overrides.updateTitle ?? mock((): Promise<Result<void>> => Promise.resolve(Ok(undefined))),
545+
emitChatEvent: overrides.emitChatEvent ?? mock(() => undefined),
546+
emit: overrides.emit ?? mock(() => true),
547+
archive:
548+
overrides.archive ??
549+
overrides.archiveWhileTaskTreeLocked ??
550+
mock((): Promise<Result<{ kind: "archived" }>> => Promise.resolve(Ok({ kind: "archived" }))),
551+
unarchive:
552+
overrides.unarchive ??
553+
overrides.unarchiveWhileTaskTreeLocked ??
554+
mock((): Promise<Result<void>> => Promise.resolve(Ok(undefined))),
555+
isWorkflowInvocationCurrent:
556+
overrides.isWorkflowInvocationCurrent ?? mock(() => Promise.resolve(true)),
557+
create:
558+
overrides.create ??
559+
mock(
560+
(): Promise<Result<{ metadata: WorkspaceMetadata }>> =>
561+
Promise.resolve(Err("workspaceHost.create not mocked"))
562+
),
563+
discardExtensionMetadataEntry:
564+
overrides.discardExtensionMetadataEntry ?? mock(() => Promise.resolve()),
708565
};
566+
const { unarchive, ...hostMocks } = mocks;
567+
// Same mocks for the locked sinks: the lifecycle path holds the (real) task-tree lock and
568+
// calls the WhileTaskTreeLocked variants; assertions target one archive/remove surface.
569+
const workspaceService = makeWorkspaceHostFake({
570+
...overrides,
571+
...hostMocks,
572+
archiveWhileTaskTreeLocked: mocks.archive,
573+
unarchiveWhileTaskTreeLocked: unarchive,
574+
removeWhileTaskTreeLocked: mocks.remove,
575+
});
576+
577+
return { workspaceService, ...mocks };
709578
}
710579

711580
// Registers the created workspace-turn checkout in config the way the real create()

‎src/node/services/taskWorkspaceSeam.testUtils.ts‎

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,59 @@
1-
import type { AgentTaskIntegration } from "@/node/services/taskWorkspaceSeam";
1+
import { Err, Ok } from "@/common/types/result";
2+
import type { AgentTaskIntegration, WorkspaceHost } from "@/node/services/taskWorkspaceSeam";
3+
4+
export function makeWorkspaceHostFake(overrides: Partial<WorkspaceHost> = {}): WorkspaceHost {
5+
return {
6+
sendMessage: () => Promise.resolve(Ok(undefined)),
7+
resumeStream: () => Promise.resolve(Ok({ started: true })),
8+
clearQueue: () => Ok(undefined),
9+
replaceHistory: () => Promise.resolve(Ok(undefined)),
10+
waitForIdleAndNoQueuedMessages: () => Promise.resolve(),
11+
waitForPendingCompactionCompletionDecision: () => Promise.resolve(true),
12+
waitForPendingStreamErrorRecoveryDecision: () => Promise.resolve(undefined),
13+
isBusyForMessage: () => false,
14+
hasQueuedMessages: () => false,
15+
hasPendingQueuedOrPreparingTurn: () => false,
16+
hasPendingAutoRetry: () => false,
17+
hasPendingBashMonitorWakeContinuation: () => false,
18+
hasPendingWorkspaceTurnContinuation: () => false,
19+
hasQueuedWorkspaceTurn: () => false,
20+
removeQueuedWorkspaceTurn: () => Ok(true),
21+
removeQueuedMessagesByDedupeKeyPrefix: () => Ok(0),
22+
getQueueCutCutter: () => undefined,
23+
countQueuedAgentPeerMessages: () => 0,
24+
archive: () => Promise.resolve(Ok({ kind: "archived" })),
25+
archiveWhileTaskTreeLocked: () => Promise.resolve(Ok({ kind: "archived" })),
26+
unarchiveWhileTaskTreeLocked: () => Promise.resolve(Ok(undefined)),
27+
preflightArchive: () => Promise.resolve(Ok({ kind: "ready" })),
28+
// No live activity grants the hold so task tests reach interruption behavior.
29+
acquirePreInterruptionArchiveHold: () => Ok({ [Symbol.dispose]: () => undefined }),
30+
listLiveWorkspaceActivity: () => ({
31+
streaming: false,
32+
queuedMessages: false,
33+
backgroundBashProcesses: false,
34+
terminalSessions: false,
35+
desktopSession: false,
36+
}),
37+
hasRunningBackgroundBashProcesses: () => Promise.resolve(false),
38+
hasUntrackableExternalAppOpen: () => Promise.resolve(false),
39+
// Keep-style behavior makes archive eligibility independent of untracked files.
40+
isSnapshotArchiveEligibilityMutationSensitive: () => false,
41+
remove: () => Promise.resolve(Ok(undefined)),
42+
removeWhileTaskTreeLocked: () => Promise.resolve(Ok(undefined)),
43+
create: () => Promise.resolve(Err("workspaceHost.create not mocked")),
44+
// Task-create tests exercise launch flow, not plugin-override sanitization.
45+
sanitizeMaterializedTaskWorkspace: () => Promise.resolve(undefined),
46+
discardExtensionMetadataEntry: () => Promise.resolve(),
47+
registerExternalBackgroundInit: () => undefined,
48+
getInfo: () => Promise.resolve(null),
49+
updateTitle: () => Promise.resolve(Ok(undefined)),
50+
emit: () => true,
51+
emitChatEvent: () => undefined,
52+
isExperimentEnabled: () => false,
53+
isWorkflowInvocationCurrent: () => Promise.resolve(true),
54+
...overrides,
55+
};
56+
}
257

358
export function makeAgentTaskIntegrationFake(
459
overrides: Partial<AgentTaskIntegration> = {}

0 commit comments

Comments
 (0)