feat(mcp): list and close preview tabs - #8724

1. browse_code — apps/server/src/mcp/PreviewMcpService.ts

2. browse_code — apps/server/src/mcp/toolkits/preview/handlers.ts

💭 I'm noticing the new service duplicates the capability check inline by constructing PreviewAutomationUnavailableError, rather than reusing McpInvocationContext.requireMcpCapability like the existing preview_list/close handlers do. I want to check McpInvocationContext to see if this is a convention violation or just acceptable duplication.

3. browse_code — apps/server/src/mcp/McpInvocationContext.ts

💭 I'm noticing requirePreviewScope duplicates requireMcpCapability's logic, though it's not purely a trivial error-wrapper the conventions explicitly forbid. I'm also questioning whether the service should accept scope as a parameter versus deriving it internally, since conventions around production service construction may apply here. Actually, thinking about it more, McpInvocationScope appears to be a plain immutable data value passed per-request rather than an Effect-returning service dependency, so passing it explicitly to list/close methods doesn't seem to violate the factory-parameter convention. I'm now scanning for other issues like the node:crypto import and Buffer usage, plus checking whether module ordering (imports, errors, Context.Service tag, make, layer) is followed correctly given some helper functions appear before that structure. The error class and predicate usage both look fine on closer inspection -- the error class properly bounds its message, and the private isPreviewTabId constant is used inline as a local predicate rather than wrapped, so I don't think it needs flagging either. Now I'm moving on to review the changes in PreviewAutomationBroker.ts and Manager.ts.

4. browse_code — apps/server/src/mcp/PreviewAutomationBroker.ts

💭 Now I'm looking at the remaining methods: respond, commitSuccessfulResponse, forgetClosedTab, and invoke.

5. browse_code — apps/server/src/mcp/PreviewAutomationBroker.ts

💭 The broker changes look conventionally fine. Now I'm looking at Manager.ts, where closeExact returns a boolean and closeSessions returns a count -- worth noting that closeExact uses a boolean-vs-typed-error design where the caller converts a false result into a PreviewSessionLookupError. I want to check whether this violates the convention that failable acquisitions should keep failures typed in Effect rather than as booleans, but it's not clearly flagged as a listed convention. I should look at the actual diff region in Manager.ts to verify.

6. browse_code — apps/server/src/preview/Manager.ts

💭 I'm noticing a type mismatch in previewAutomation.ts: the close input's tabId is validated as a plain string, but the service passes it into closeExact which actually expects a PreviewTabId type. If that's a branded type, a raw string wouldn't type-check, so I need to check how PreviewTabId is defined.

7. browse_code — Show the definition of PreviewTabId, PreviewSessionLookupError, PreviewSessionSnapshot, and TrimmedNonEmptyString in packages/contracts/src (preview.ts / primitives). Is PreviewTabId a branded schema type or a plain string alias?

💭 No type mismatch there. Looking at the contracts file, I notice PreviewMcpCloseInput.tabId redefines the same string constraints as PreviewTabId instead of reusing it, which is a duplication issue even if not one of the explicitly listed conventions, and there's also a subtlety with isTrimmed() versus the TrimmedString decode-transform behavior. I'm weighing whether flagging requirePreviewScope duplicating requireMcpCapability is in scope, and checking whether the type-only import of McpInvocationScope from McpInvocationContext violates the namespace-import convention — but the convention actually permits named imports when only a type is being used, so this looks acceptable. Looking further, the preview handlers bypass requireMcpCapability and rely on service-level checks instead, which duplicates capability-gating logic across PreviewMcpService and McpInvocationContext — but since it's not a helper that's purely wrapping error construction, this seems like a risky nit to raise rather than a clear convention violation. The PreviewMcpInvalidCursorError schema, union types, module ordering, and dependency capture pattern in PreviewMcpService all look fine on inspection — no red flags there. I'm now moving on to check t3McpToolPresentation.ts for similar record entries. Checking ClaudeAdapterV2.ts, I added tool names (preview_status, preview_list, preview_snapshot, preview_wait_for) to the read-only allowlist, and I want to verify these match the Readonly annotations in tools.ts, though since the prerequisite check already passed, consistency is likely confirmed already. Now I'm tracing dependency handling in PreviewMcpService.close, confirming broker.forgetClosedTab correctly passes scope and that Effect.uninterruptible wrapping is fine. I'm also weighing whether PreviewMcpCloseInput.tabId should reuse the PreviewTabId schema instead of re-declaring checks, but since schema duplication isn't explicitly covered by the conventions, I'm leaning toward not flagging it, and I'm double-checking whether PreviewMcpService sits in its proper canonical module location. I'm now noticing that PreviewMcpService.ts imports node:crypto directly for hashing while the broker uses effect/Crypto, which might violate the convention against hiding runtime-backed imperative APIs inside pure helpers or layer implementations. I want to check whether this is actually a documented pattern elsewhere in the codebase, so I'm planning to grep for other node:crypto usages in apps/server to see if this is consistent with existing practice.

8. browse_code — Where is node:crypto imported in apps/server/src and packages/? Also, how is effect/Crypto used in PreviewAutomationBroker.ts (what for)? Is there a convention/lint forbidding direct node:crypto use in server services?

💭 The synchronous hashing pattern in the pure helper matches existing precedent, and CI passing confirms no import issues there. No new lint-disabling directives appear in this diff either. I'm now looking more closely at how the new handlers in handlers.ts use McpInvocationContext and delegate capabilities. Checking whether PreviewMcpService's explicit scope parameter counts as improper service injection versus passing an immutable domain value — since McpInvocationScope looks like plain data rather than an Effect service tag, it seems consistent with the convention's exception for configuration values. I'm also scanning the contracts diff for error-class conventions, checking for redundant description annotations on schemas, and verifying error tags don't duplicate reason literals unnecessarily. Nothing here looks like a real violation so far — just confirming there's no dependency array or type mismatch issue in the tools.ts additions. I'm now checking whether PreviewMcpService.layer is wired into McpHttpServer for both toolkit registrations, since it's only provided to PreviewStandardToolkitRegistrationLive and I need to see if the other registration also requires it — a missing layer there would surface as a type error, so I want to trace through McpHttpServer.ts to be sure.

9. browse_code — apps/server/src/mcp/McpHttpServer.ts

💭 This matches the OrchestratorMcpService pattern well. I want to compare PreviewMcpService against sibling services like WorktreeMcpService to check if there's a shared capability-check helper that might be duplicated rather than reused.

10. browse_code — Show apps/server/src/mcp/WorktreeMcpService.ts and OrchestratorMcpService.ts structure: how do they define the Context.Service, how do they obtain the McpInvocationScope (parameter vs environment), and do they have their own capability-check helper duplicating McpInvocationContext.requireMcpCapability?

💭 PreviewMcpService matches the established repo pattern, and the duplicated requireMcpCapability body is just a minor DRY nit rather than a genuine convention violation, so per the check's scope I shouldn't flag it or suggest improvements beyond what's required. I'll skip commenting on it and move on to check OrchestratorM. I conclude there are no findings to report, so my final response needs to be exactly "All clear" with nothing else — I'll set the summary field to that string precisely.