feat(grok): native ACP parity (catalog, effort, usage, plan, multi-agent, compact) - #5423

💭 I'm reviewing the diff to identify Effect service convention violations across several key files like GrokDriver, GrokAdapter, and the ACP-related modules. I need to check for proper consolidation of effect imports, correct service-instance parameter patterns, and verify the usage of Layer.succeed, ManagedRuntime, runPromise, and error definitions. Looking at applyGrokAcpConfigSelections which takes a runtime parameter — this follows the existing pattern from applyGrokAcpModelSelection, and since these are helper functions rather than service constructors, they're acceptable. I'm also verifying that patterns like Effect.acquireRelease for PubSub management and Effect.catch for intentional error handling are used correctly, then I'll examine the actual imports across the touched files to ensure compliance. Looking at the imports and error handling patterns in GrokAdapter, particularly how Effect.catch is being used to handle the tagged errors. I'm noticing that line 82 imports a GrokAdapterShape type from the Services directory, which is a pre-existing pattern in the codebase. The question is whether I should flag this as a violation since the conventions discourage standalone shape types, but the PR guidelines say to only report violations introduced or retained within the changed scope—and this particular architecture predates the PR itself. Looking at the actual changes: GrokDriver creates a snapshot object with a getter for stream changes, which isn't a convention issue. The Effect.acquireRelease call for PubSub appears to be scoped correctly within the provider instance. For errors, there are no new error classes introduced—just a signature change in setSessionModel. The interesting part is how GrokProvider uses String(discoveryExit.cause) for regex matching on stringified causes to detect auth errors, but it's only used for pattern matching, not stored as a message or detail, so it doesn't violate the rule about keeping error attributes bounded. In GrokAdapter, the Effect.catch on onAvailableCommands is applied to an effect with error type never, making it a no-op, which is fine. The logging of { cause } doesn't reintroduce removed legacy fields. There's also an intentional error swallowing with .pipe(Effect.catch(() => Effect.void)) around line 1975, which is acceptable since the entire error channel is being handled deliberately. Now checking dependency acquisition patterns... The makeGrokAdapter call passes options with a callback strategy rather than service injection, which is appropriate. GrokAdapter uses mutable session context with Sets and direct field mutations instead of Ref, but that's not a listed convention violation. For GrokAcpSupport's applyGrokAcpConfigSelections, it receives a runtime parameter using the Foo["Service"] reference pattern, which is the recommended approach for referring to interfaces. It's a pure helper matching the existing applyGrokAcpModelSelection pattern, not a service constructor. Checking the diff for any problematic patterns like catchTag, catchIf, or runPromise in the changed files, and verifying that capabilitiesFromGrokModelMeta in GrokProvider.ts is a pure function—it looks clean so far. Looking through the PR changes, I don't see anything new being introduced here—the Layer.succeed(ChildProcessSpawner...) call on line 189 of GrokAcpSupport was already there. Let me verify the other candidates: the interface method signature in AcpSessionRuntime.ts with options looks fine, but isGrokEnterPlanModeToolCall exported from GrokAdapter.ts isn't covered. There's also the snapshot object in GrokDriver with a getter for streamCh that's cut off. The getter recomputes streams, which is more of a functional concern than an Effect service convention issue. I should check whether the Effect.acquireRelease call with PubSub.unbounded and PubSub.shutdown in GrokDriver is properly scoped—I need to look at lines 100-230 to confirm it's wrapped in an Effect.gen with Scope. Looking at the diff more carefully, I see the variable name changed from snapshot to managedSnapshot at line 123, but the mapError block with the error detail message was already there before this PR. So the way it's copying cause.message into the error detail is pre-existing code, not something introduced by these changes. Now checking the GrokAdapter for how it handles raw payloads and logging — the rawPayload: meta pattern in runtime events is standard across this codebase, so that's fine. The warning log for unknown sessionUpdate events is properly bounded with just the sessionUpdate and threadId, which is appropriate. Looking at the catch handler that logs the cause object directly — that's acceptable since we're preserving the exact underlying error value as intended. Scanning through the other changes like the AcpSessionRuntime interface extension, the GrokAdapterShape service type, and how the driver passes service instances like Crypto and ChildProcessSpawner — all of these are either pre-existing patterns or properly scoped changes. The review looks clean so far. The GrokAdapter's type imports from AcpSessionRuntime are fine. Since no issues were found, I need to respond with "All clear" — though there's a tension between the instruction to output exactly that phrase and the requirement to use JSON schema. I'll resolve this by putting "All clear" in the summary and title fields while maintaining the required JSON structure, marking it as a successful check. Let me do one final pass through the GrokProvider diff to confirm there are no violations: the auth-failure detection uses regex pattern matching on the cause string, which is a heuristic approach but doesn't violate the error-model rules since it doesn't store the error itself. The discovery labels and other fields are all fine. Everything checks out.