feat: consolidated provider architecture (PR #28 base + PR #27 cherry-picks) - #29
Conversation
Make HarnessClientAdapter the default path for Codex sessions. Direct CodexAdapter path preserved behind T3CODE_CODEX_LEGACY=1 feature flag for rollback safety. - serverLayers.ts: 4-path routing (harness default, legacy flag, graceful degradation) - ProviderSessionDirectory: read-time adapter_key migration (codex -> harness:codex) - ProviderService: resume_cursor validation (invalid -> fresh session) - CodexAdapter + CodexAppServerManager: @deprecated JSDoc markers - 21 new tests covering cutover, migration, and validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add MCP configuration resolution service with in-memory snapshot storage and wiring into the server layer composition. Includes: - McpServerConfig and ResolvedMcpConfig schemas in contracts - McpConfigService Effect service tag + McpConfigServiceLive layer - McpConfigError in the provider error taxonomy - McpConfigServiceLive wired into makeServerProviderLayer The resolver is currently a stub returning empty configs. Claude manages its own MCP natively through the Agent SDK, so the empty config is intentional for that provider. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add translateMcpConfig method to the ProviderAdapterShape contract and implement it across all adapters: - ClaudeAdapter: no-op (returns null) -- Claude manages its own MCP - CodexAdapter: translates to mcpServers array for codex app-server - HarnessClientAdapter: wraps as mcp_config object for Elixir harness - TestProviderAdapter: no-op stub for tests ProviderService.startSession now resolves MCP config via McpConfigService, translates via the adapter, and includes the result in the start params. The mcpConfigVersion is stored in the runtime payload for change detection. Elixir session modules (CodexSession, CursorSession, OpenCodeSession) now accept and store mcp_config from start_session params. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add capability-driven contract integration tests that auto-skip for unsupported capabilities using describe.skipIf. Covers session lifecycle, rollback, resume, approval, user-input, and tool execution. Create Harness.Providers.ProviderBehaviour Elixir behaviour module with 8 required callbacks (start_link, send_turn, interrupt_turn, respond_to_approval, respond_to_user_input, read_thread, rollback_thread, stop). Add @behaviour and @impl annotations to CodexSession, CursorSession, OpenCodeSession, and ClaudeSession. Add provider onboarding playbook at ai_docs/provider_onboarding.md with step-by-step guide for both Elixir harness and Node SDK paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add McpConfigServiceLive to capabilitySuite test layer (was causing unresolved service error at runtime) - Deduplicate adapter_key migration logs with a per-thread Set to avoid unbounded log spam on every getBinding read Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cherry-picked from feat/provider-architecture-consolidation (PR #27): - ai_docs/failure_matrix.md: 22-row operation × provider × error matrix - ai_docs/provider_onboarding.md: 9-step playbook for adding new providers - validateResumeCursor(): cursor validation in recovery and start paths - codexHarnessCutover.test.ts: 21 tests for cutover logic - provider_behaviour.ex: Elixir behaviour with 8 callbacks + @impl - Path C in serverLayers.ts: graceful error when harness not configured - State machine diagram in contracts/provider.ts Co-Authored-By: Bastian Venegas Arevalo <r2d2@ranvier-technologies.com>
Original prompt from Bastian
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
📝 WalkthroughWalkthroughAdds a ProviderSession behaviour and provider capability/schema extensions, integrates an MCP configuration service and translation/materialization into session startup and adapters, introduces error classification, updates harness/direct adapter routing (including legacy flag), and expands telemetry/metrics and tests across harness and server layers. Changes
Sequence Diagram(s)mermaid Client->>+ProviderService: startSession(threadId, provider, threadMeta) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size:XXL
vouch:trusted
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| yield* analytics.record("provider.conversation.rolled_back", { | ||
| provider: routed.adapter.provider, | ||
| adapterPath: routed.adapterPath, | ||
| turns: input.numTurns, | ||
| }); |
There was a problem hiding this comment.
🟡 Rollback analytics event provider.conversation.rolled_back emitted even on failure
In ProviderService.rollbackConversation, the provider.conversation.rolled_back analytics event is recorded unconditionally before checking whether the rollback actually succeeded. The event name implies the rollback completed, but it's emitted even when the rollbackResult is a Failure. The subsequent provider.rollback.outcome event does correctly track success/failure, but the misleading rolled_back event will inflate rollback success counts in analytics dashboards.
| yield* analytics.record("provider.conversation.rolled_back", { | |
| provider: routed.adapter.provider, | |
| adapterPath: routed.adapterPath, | |
| turns: input.numTurns, | |
| }); | |
| yield* analytics.record("provider.conversation.rollback_attempted", { | |
| provider: routed.adapter.provider, | |
| adapterPath: routed.adapterPath, | |
| turns: input.numTurns, | |
| }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Devin Review found 3 issues — all on code from PR #28 (not the cherry-picked additions from PR #27):
@Bastian — should I fix these in this PR or leave them for a separate pass? The config.toml overwrite (#1) is the only one flagged as a real bug. |
- CodexAdapter.ts and HarnessClientAdapter.ts now read-then-append instead of overwriting config.toml, preserving user's non-MCP settings - codexTomlFromResolved adds trailing newline for POSIX consistency Co-Authored-By: Bastian Venegas Arevalo <r2d2@ranvier-technologies.com>
Co-Authored-By: Bastian Venegas Arevalo <r2d2@ranvier-technologies.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| case "codex": { | ||
| const generatedDir = generatedMcpDir( | ||
| serverConfig.stateDir, | ||
| "codex", | ||
| input.threadId, | ||
| ); | ||
| fs.rmSync(generatedDir, { recursive: true, force: true }); | ||
| const generatedHomePath = path.join(generatedDir, "home"); | ||
| fs.mkdirSync(generatedHomePath, { recursive: true }); | ||
| if ( | ||
| baseCodexHomePath && | ||
| fs.existsSync(baseCodexHomePath) && | ||
| baseCodexHomePath !== generatedHomePath | ||
| ) { | ||
| fs.cpSync(baseCodexHomePath, generatedHomePath, { | ||
| recursive: true, | ||
| force: true, | ||
| }); | ||
| } | ||
| const configTomlPath = path.join(generatedHomePath, "config.toml"); | ||
| const existingConfig = fs.existsSync(configTomlPath) | ||
| ? fs.readFileSync(configTomlPath, "utf8") | ||
| : ""; | ||
| fs.writeFileSync( | ||
| configTomlPath, | ||
| existingConfig + codexTomlFromResolved(resolvedMcp), | ||
| "utf8", | ||
| ); | ||
| return { | ||
| codex: { | ||
| homePath: generatedHomePath, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🟡 Duplicate Codex MCP home-directory materialization logic across CodexAdapter and HarnessClientAdapter
The Codex MCP config materialization logic — creating a generated directory, copying the base home path, reading existing config.toml, and appending generated TOML — is duplicated nearly identically between CodexAdapter.ts:1396-1432 and HarnessClientAdapter.ts:1105-1137. This violates the AGENTS.md maintainability rule: "Duplicate logic across multiple files is a code smell and should be avoided. Don't be afraid to change existing code. Don't take shortcuts by just adding local logic to solve a problem." The shared logic should be extracted to a function in mcpTranslation.ts (which already contains the codexTomlFromResolved and generatedMcpDir helpers).
Prompt for agents
Extract the duplicated Codex MCP home-directory materialization logic into a shared function in apps/server/src/provider/mcpTranslation.ts. The function should accept (stateDir, threadId, baseHomePath, resolvedMcp) and return the generated home path. Then update both apps/server/src/provider/Layers/CodexAdapter.ts (lines 1396-1432) and apps/server/src/provider/Layers/HarnessClientAdapter.ts (lines 1105-1137) to call the shared function instead of duplicating the logic.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Devin is currently unreachable - the session may have died. |
1 similar comment
|
Devin is currently unreachable - the session may have died. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
apps/harness/lib/harness/metrics.ex (1)
91-98: Consider single-pass aggregation inlifecycle_metrics/1.Line 93 through Line 96 iterate the same list multiple times. A single
Enum.reduce/3keeps behavior the same with lower per-request overhead under high session counts.♻️ Proposed refactor
defp lifecycle_metrics(sessions) do - %{ - active_sessions: length(sessions), - sessions_by_provider: Enum.frequencies_by(sessions, & &1.provider), - sessions_with_backlog: Enum.count(sessions, &(&1.message_queue_len > 0)), - total_message_queue_len: Enum.reduce(sessions, 0, &(&1.message_queue_len + &2)) - } + Enum.reduce(sessions, %{ + active_sessions: 0, + sessions_by_provider: %{}, + sessions_with_backlog: 0, + total_message_queue_len: 0 + }, fn session, acc -> + %{ + active_sessions: acc.active_sessions + 1, + sessions_by_provider: + Map.update(acc.sessions_by_provider, session.provider, 1, &(&1 + 1)), + sessions_with_backlog: + acc.sessions_with_backlog + if(session.message_queue_len > 0, do: 1, else: 0), + total_message_queue_len: + acc.total_message_queue_len + session.message_queue_len + } + end) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/harness/lib/harness/metrics.ex` around lines 91 - 98, The lifecycle_metrics/1 function currently traverses sessions multiple times to compute active_sessions, sessions_by_provider, sessions_with_backlog and total_message_queue_len; change it to a single Enum.reduce/3 pass that folds an accumulator map (keys: :active_sessions, :sessions_by_provider, :sessions_with_backlog, :total_message_queue_len) and for each session increments the active_sessions counter, updates sessions_by_provider via Map.update/3 using session.provider, increments sessions_with_backlog when session.message_queue_len > 0, and adds message_queue_len to total_message_queue_len, then return that accumulator as the result structure expected by lifecycle_metrics/1.apps/server/src/provider/Layers/ProviderRegistry.ts (1)
119-130: Consider extracting capability override to avoid spread in map callbacks.Static analysis flags the object spread inside
Effect.mapandStream.mapas potentially inefficient. While this is low-impact (these are infrequent calls), extracting a helper could improve clarity.♻️ Optional refactor to address static analysis hint
+const withHarnessCodexCapabilities = <T extends { capabilities?: unknown }>( + provider: T, +): T & { capabilities: typeof HARNESS_PROVIDER_CAPABILITIES.codex } => ({ + ...provider, + capabilities: HARNESS_PROVIDER_CAPABILITIES.codex, +}); + const codexProvider: CodexProviderShape = { getSnapshot: codexProviderBase.getSnapshot.pipe( - Effect.map((provider) => ({ - ...provider, - capabilities: HARNESS_PROVIDER_CAPABILITIES.codex, - })), + Effect.map(withHarnessCodexCapabilities), ), refresh: codexProviderBase.refresh.pipe( - Effect.map((provider) => ({ - ...provider, - capabilities: HARNESS_PROVIDER_CAPABILITIES.codex, - })), + Effect.map(withHarnessCodexCapabilities), ), streamChanges: codexProviderBase.streamChanges.pipe( - Stream.map((provider) => ({ - ...provider, - capabilities: HARNESS_PROVIDER_CAPABILITIES.codex, - })), + Stream.map(withHarnessCodexCapabilities), ), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderRegistry.ts` around lines 119 - 130, Extract the capability-override into a small helper and use it in both mapping chains to avoid in-line object spread: create a function (e.g., applyCodexCapabilities or overrideWithCodexCapabilities) that accepts a provider object and returns a new object with provider.capabilities set to HARNESS_PROVIDER_CAPABILITIES.codex, then replace the inline map callbacks on codexProviderBase.refresh (Effect.map) and codexProviderBase.streamChanges (Stream.map) to call that helper instead of spreading inside the map.apps/server/src/provider/Layers/HarnessProvider.ts (1)
33-33: Redundant type cast after parameter narrowing.Since the
providerparameter is now typed as"cursor" | "opencode", the castas "cursor" | "opencode"on line 33 is redundant. The same applies to line 92.♻️ Proposed cleanup
Effect.map( - (settings) => - settings.providers[provider as "cursor" | "opencode"] as HarnessProviderSettings, + (settings) => settings.providers[provider] as HarnessProviderSettings, ),And on line 92:
Stream.map( - (settings) => - settings.providers[provider as "cursor" | "opencode"] as HarnessProviderSettings, + (settings) => settings.providers[provider] as HarnessProviderSettings, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/HarnessProvider.ts` at line 33, The code uses redundant type assertions on settings.providers when the provider parameter is already narrowed to "cursor" | "opencode"; remove the unnecessary cast and directly index settings.providers[provider] and settings.providers[provider] as HarnessProviderSettings where needed (reference the usages in HarnessProvider, e.g., the expression currently written as settings.providers[provider as "cursor" | "opencode"] and the similar occurrence later around the HarnessProviderSettings access), and update both occurrences so the compiler continues to infer the correct union type without the redundant "as" cast.apps/server/src/provider/Layers/codexHarnessCutover.test.ts (1)
10-10: Remove unusedviimport.The
vimock utility is imported but not used in any test.-import { describe, it, expect, vi, afterEach } from "vitest"; +import { describe, it, expect, afterEach } from "vitest";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/codexHarnessCutover.test.ts` at line 10, The import line in codexHarnessCutover.test.ts currently pulls in the unused mock utility "vi"; remove "vi" from the named imports (i.e., change the import of { describe, it, expect, vi, afterEach } to exclude vi) so the test file only imports used symbols and eliminates the unused-import lint warning.apps/server/src/provider/Layers/HarnessClientAdapter.ts (1)
1112-1122: Consider documenting the force-copy behavior.Using
fs.cpSyncwithforce: truewill overwrite any user modifications in the generated home directory. This is likely intentional (ensuring clean state), but a brief comment would clarify the design decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/HarnessClientAdapter.ts` around lines 1112 - 1122, Add a short comment above the fs.cpSync call in HarnessClientAdapter (around generatedHomePath/baseCodexHomePath logic) that documents the intentional use of force: true — explicitly state that copying from baseCodexHomePath will overwrite any existing files in generatedHomePath to ensure a clean/consistent generated home state and that this behavior is deliberate. Keep the comment concise and reference generatedHomePath, baseCodexHomePath, and the fs.cpSync(force: true) usage.ai_docs/provider_onboarding.md (1)
9-19: Add language identifier to fenced code block.The architecture diagram code block should specify a language (e.g.,
textorplaintext) to satisfy markdown linting rules.-``` +```text Transport (WebSocket/RPC)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ai_docs/provider_onboarding.md` around lines 9 - 19, The fenced architecture diagram block is missing a language identifier; update the triple-backtick fence that wraps the lines "Transport (WebSocket/RPC) | ProviderService (cross-provider facade) | ProviderAdapterRegistry (adapter lookup) | ProviderAdapter (provider-specific runtime) | Provider CLI/SDK (codex, claude, cursor, opencode, ...)" to include a language tag such as text or plaintext (e.g., change ``` to ```text) so markdown linters recognize it as a code block.apps/server/src/serverLayers.ts (1)
198-227: Path C: Consider usingProviderKindtype for consistency.The
getByProviderparameter is typed asstringbut the interface expectsProviderKind. While this works due to structural typing, using the correct type improves consistency.return { - getByProvider: (provider: string) => { + getByProvider: (provider) => { const adapter = byProvider.get(provider);Letting TypeScript infer the type from the interface is cleaner than an explicit
string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/serverLayers.ts` around lines 198 - 227, Change the getByProvider signature to accept the ProviderKind type instead of string for consistency: update the parameter type of getByProvider (the function returned in the layer) from string to ProviderKind and ensure any usages inside (e.g., the byProvider.get(provider) call and the ProviderUnsupportedError construction) still compile; letting TypeScript infer the parameter type from the interface that declares getByProvider is fine—replace the explicit string type with ProviderKind in that function's parameter so the implementation matches the declared interface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ai_docs/provider_onboarding.md`:
- Around line 92-101: Documentation for ProviderAdapterCapabilities is missing
five new fields; update the example/description to show the complete shape:
include resume, subagents, attachments, replay, and mcpConfig each typed as
ProviderCapabilityLevel, and ensure the existing fields (sessionModelSwitch,
supportsUserInput, supportsRollback, supportsFileChangeApproval) remain present;
reference the ProviderAdapterCapabilities / ProviderCapabilities type names and
ProviderCapabilityLevel enum so the doc matches the actual type definition used
by the codebase.
In `@apps/harness/lib/harness/providers/provider_behaviour.ex`:
- Around line 9-19: Update the ProviderBehaviour docs to match actual
SessionManager dispatch: add the missing `wait_for_ready/2` callback to the
required callbacks list (so implementers know to handle startup readiness), and
remove or reword the `stop/1` entry to reflect that SessionManager does not call
`stop/1` (shutdowns are handled by the supervisor); mention `wait_for_ready/2`
and `stop/1` by name in your change so future readers can find where
startup/shutdown responsibilities are defined in ProviderBehaviour.
In `@apps/server/integration/contract.integration.test.ts`:
- Around line 111-129: collectEventsDuring currently blocks indefinitely because
it calls Queue.take count times with no timeout; change it to fail-fast by
wrapping each Queue.take in a timed failure (e.g., use Effect.timeoutFail or
race with Effect.sleep) so that if the runtime stream under-emits the test fails
instead of hanging. Locate collectEventsDuring and replace the direct Queue.take
calls inside the Effect.forEach with a timed variant (e.g., timeoutFail(() =>
new Error("timed out waiting for provider event"), timeoutMs)(Queue.take(queue))
or an equivalent race), keeping Stream.runForEach and the queue setup unchanged
so missing events produce a deterministic test failure.
In `@apps/server/integration/TestProviderAdapter.integration.ts`:
- Around line 243-253: The hardcoded capabilities object in
TestProviderAdapter.integration.ts is out of sync with the centralized
definitions; replace the inline const capabilities with a lookup into the
canonical provider capabilities map (e.g. use the exported PROVIDER_CAPABILITIES
or providerCapabilities symbol) keyed by the local provider variable so tests
use the real capability set defined in providerCapabilities.ts (remove the
hand-rolled sessionModelSwitch/resume/subagents/attachments/mcpConfig logic and
return the canonical entry for the provider).
In `@apps/server/src/provider/Errors.ts`:
- Around line 171-183: ProviderRecoveryStrategy and ProviderErrorClassification
currently only tag errors for telemetry but never affect control flow; update
the handling in ProviderService (where classifications are recorded and the
cause rethrown) to enact the named strategies instead of just logging: implement
a retry/backoff loop for "retry-backoff", trigger config re-resolution for
"re-resolve-config", recreate auth/session state for "fresh-session" and
"restart-session" (with restart doing a more forceful refresh), perform graceful
degradation (return fallback/partial response) for "degrade-gracefully", and
immediately abort for "fail-fast"; alternatively, if behavioral changes are
unwanted, rename the union and documentation to make them explicit
telemetry-only labels and add TODOs where ProviderErrorClassification is used to
avoid misleading names.
In `@apps/server/src/provider/Layers/CodexAdapter.test.ts`:
- Around line 252-259: The assertions that check homePath and
generatedConfigPath run after the Effect scope (created by
ServerConfig.layerTest which uses fs.makeTempDirectoryScoped) is closed, so move
those assertions inside the Effect.gen so they execute before cleanup: inside
the generator passed to Effect.gen where you obtain the adapter (yield*
CodexAdapter) and call adapter.startSession, read/compute homePath and
generatedConfigPath there and perform fs.existsSync assertions before the
generator returns; this ensures the temp directory created by
ServerConfig.layerTest and cleaned up on scope exit remains available for the
checks.
In `@apps/server/src/provider/Layers/CodexAdapter.ts`:
- Around line 1413-1420: The current write logic appends
codexTomlFromResolved(resolvedMcp) to whatever is in configTomlPath
(existingConfig), causing duplicated [mcp_servers.*] tables or glued headers;
instead, load a clean base config when available (use baseHomePath's
config.toml) or strip/replace the previously generated MCP block before writing:
detect a unique marker or the start of the generated block from
codexTomlFromResolved and remove any existing matching block from
existingConfig, ensure a trailing newline between base content and the generated
block, then write the merged result to configTomlPath (update code around
configTomlPath, existingConfig, codexTomlFromResolved and generatedHomePath to
implement this).
In `@apps/server/src/provider/Layers/HarnessClientAdapter.ts`:
- Around line 1124-1131: The current write logic in HarnessClientAdapter.ts
concatenates existingConfig and codexTomlFromResolved(resolvedMcp) directly,
which can fuse TOML entries if existingConfig doesn't end with a newline; change
the write to ensure a separating newline by computing a prefix like const padded
= existingConfig === "" || existingConfig.endsWith("\n") ? existingConfig :
existingConfig + "\n" and then call fs.writeFileSync(configTomlPath, padded +
codexTomlFromResolved(resolvedMcp), "utf8") so the appended MCP block is always
on a new line.
In `@apps/server/src/provider/Layers/McpConfig.ts`:
- Around line 132-133: snapshotPath currently interpolates String(threadId)
directly into the file path which allows path traversal; modify snapshotPath to
derive a filesystem-safe filename (e.g. compute a SHA-256 hex hash or a URL-safe
Base64 of String(threadId)) and use that hash as the filename (e.g.
`${hash}.json`), and optionally validate the final resolved path stays inside
path.resolve(stateDir, "mcp", "snapshots") to guard against escapes; update the
snapshotPath function to import and use Node's crypto (or a safe encoder) and
replace String(threadId) with the computed safe identifier.
- Around line 182-184: The code uses Effect.catch(...) (invalid in modern
Effect-TS) when wrapping fileSystem.readFileString(targetPath) and two other
similar calls; replace each Effect.catch(...) with Effect.catchAll(...) so
errors are properly caught (e.g., change the assignment creating raw from
fileSystem.readFileString(targetPath).pipe(Effect.catch(() =>
Effect.succeed<string | null>(null))) to use Effect.catchAll instead), keeping
the same error handler functions and types.
In `@apps/server/src/provider/Layers/ProviderService.test.ts`:
- Around line 912-917: The test currently composes
ProviderSessionRuntimeRepositoryLive with SqlitePersistenceMemory causing
persisted rows to leak between tests; update the setup to use a fresh temp
sqlite DB for runtime by replacing Layer.provide(SqlitePersistenceMemory) with a
temp-isolated persistence layer (or create a new ephemeral
SqlitePersistenceMemory instance per test) when building runtimeRepositoryLayer,
or alternatively relax the assertion on recordedClearCalls in the test that
inspects ProviderSessionDirectoryLive to assert that "thread-mcp-runtime" is
included rather than matching the entire array; adjust the test referencing
recordedClearCalls and
ProviderSessionRuntimeRepositoryLive/ProviderSessionDirectoryLive accordingly so
each test uses an isolated runtime DB or checks inclusion of
"thread-mcp-runtime".
- Around line 402-415: The test currently sleeps 10ms then stops the session,
which races with the async analytics write; replace the fixed sleep with a
wait/poll that checks analyticsSpy.recorded for the "provider.turn.duration"
event (or otherwise await the lifecycle processing path) before asserting;
specifically, change the block that calls provider.stopSession({ threadId }) and
then inspects analyticsSpy.recorded to instead poll/wait until
analyticsSpy.recorded.find(entry => entry.event === "provider.turn.duration")
returns a value (or await a promise exposed by the provider/session lifecycle),
then assert that turnDuration.properties.durationMs is a number and adapterPath
is "direct".
In `@packages/contracts/src/provider.ts`:
- Around line 82-90: McpServerConfig must be changed from a single Schema.Struct
to a transport-discriminated Schema.Union so a stdio server cannot include
remote-only fields and vice versa: replace the current McpServerConfig with a
union of two structs — a "stdio" variant whose transport is the literal "stdio"
and which does not allow/omit command/url (only name, transport, enabled,
optional args/env as appropriate) and a "remote" variant whose transport is the
other MCP transports (use McpTransport or a literal union of those values) and
which requires the remote-specific shape (e.g., require command
(TrimmedNonEmptyString) or url as appropriate and allow args/env), keeping all
validation in packages/contracts and using only Schema.* constructors
(Schema.Union/Schema.Struct/Schema.literal/etc.) to enforce the discriminated
shapes for McpServerConfig.
---
Nitpick comments:
In `@ai_docs/provider_onboarding.md`:
- Around line 9-19: The fenced architecture diagram block is missing a language
identifier; update the triple-backtick fence that wraps the lines "Transport
(WebSocket/RPC) | ProviderService (cross-provider facade) |
ProviderAdapterRegistry (adapter lookup) | ProviderAdapter (provider-specific
runtime) | Provider CLI/SDK (codex, claude, cursor, opencode, ...)" to include a
language tag such as text or plaintext (e.g., change ``` to ```text) so markdown
linters recognize it as a code block.
In `@apps/harness/lib/harness/metrics.ex`:
- Around line 91-98: The lifecycle_metrics/1 function currently traverses
sessions multiple times to compute active_sessions, sessions_by_provider,
sessions_with_backlog and total_message_queue_len; change it to a single
Enum.reduce/3 pass that folds an accumulator map (keys: :active_sessions,
:sessions_by_provider, :sessions_with_backlog, :total_message_queue_len) and for
each session increments the active_sessions counter, updates
sessions_by_provider via Map.update/3 using session.provider, increments
sessions_with_backlog when session.message_queue_len > 0, and adds
message_queue_len to total_message_queue_len, then return that accumulator as
the result structure expected by lifecycle_metrics/1.
In `@apps/server/src/provider/Layers/codexHarnessCutover.test.ts`:
- Line 10: The import line in codexHarnessCutover.test.ts currently pulls in the
unused mock utility "vi"; remove "vi" from the named imports (i.e., change the
import of { describe, it, expect, vi, afterEach } to exclude vi) so the test
file only imports used symbols and eliminates the unused-import lint warning.
In `@apps/server/src/provider/Layers/HarnessClientAdapter.ts`:
- Around line 1112-1122: Add a short comment above the fs.cpSync call in
HarnessClientAdapter (around generatedHomePath/baseCodexHomePath logic) that
documents the intentional use of force: true — explicitly state that copying
from baseCodexHomePath will overwrite any existing files in generatedHomePath to
ensure a clean/consistent generated home state and that this behavior is
deliberate. Keep the comment concise and reference generatedHomePath,
baseCodexHomePath, and the fs.cpSync(force: true) usage.
In `@apps/server/src/provider/Layers/HarnessProvider.ts`:
- Line 33: The code uses redundant type assertions on settings.providers when
the provider parameter is already narrowed to "cursor" | "opencode"; remove the
unnecessary cast and directly index settings.providers[provider] and
settings.providers[provider] as HarnessProviderSettings where needed (reference
the usages in HarnessProvider, e.g., the expression currently written as
settings.providers[provider as "cursor" | "opencode"] and the similar occurrence
later around the HarnessProviderSettings access), and update both occurrences so
the compiler continues to infer the correct union type without the redundant
"as" cast.
In `@apps/server/src/provider/Layers/ProviderRegistry.ts`:
- Around line 119-130: Extract the capability-override into a small helper and
use it in both mapping chains to avoid in-line object spread: create a function
(e.g., applyCodexCapabilities or overrideWithCodexCapabilities) that accepts a
provider object and returns a new object with provider.capabilities set to
HARNESS_PROVIDER_CAPABILITIES.codex, then replace the inline map callbacks on
codexProviderBase.refresh (Effect.map) and codexProviderBase.streamChanges
(Stream.map) to call that helper instead of spreading inside the map.
In `@apps/server/src/serverLayers.ts`:
- Around line 198-227: Change the getByProvider signature to accept the
ProviderKind type instead of string for consistency: update the parameter type
of getByProvider (the function returned in the layer) from string to
ProviderKind and ensure any usages inside (e.g., the byProvider.get(provider)
call and the ProviderUnsupportedError construction) still compile; letting
TypeScript infer the parameter type from the interface that declares
getByProvider is fine—replace the explicit string type with ProviderKind in that
function's parameter so the implementation matches the declared interface.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a18cd076-d3f4-4c3d-b7b5-f5ded8920662
📒 Files selected for processing (45)
ai_docs/failure_matrix.mdai_docs/provider_onboarding.mdapps/harness/lib/harness/metrics.exapps/harness/lib/harness/provider_session.exapps/harness/lib/harness/providers/claude_session.exapps/harness/lib/harness/providers/codex_session.exapps/harness/lib/harness/providers/cursor_session.exapps/harness/lib/harness/providers/mock_session.exapps/harness/lib/harness/providers/opencode_session.exapps/harness/lib/harness/providers/provider_behaviour.exapps/harness/test/harness/providers/codex_session_test.exsapps/harness/test/harness/providers/cursor_session_test.exsapps/harness/test/harness/providers/opencode_session_test.exsapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/integration/TestProviderAdapter.integration.tsapps/server/integration/contract.integration.test.tsapps/server/integration/providerService.integration.test.tsapps/server/src/codexAppServerManager.tsapps/server/src/main.test.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/provider/Errors.test.tsapps/server/src/provider/Errors.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/CodexAdapter.test.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/HarnessClientAdapter.tsapps/server/src/provider/Layers/HarnessProvider.tsapps/server/src/provider/Layers/McpConfig.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.test.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/Layers/codexHarnessCutover.test.tsapps/server/src/provider/Services/CodexAdapter.tsapps/server/src/provider/Services/McpConfig.tsapps/server/src/provider/Services/ProviderAdapter.tsapps/server/src/provider/mcpTranslation.tsapps/server/src/provider/providerCapabilities.tsapps/server/src/provider/providerSnapshot.tsapps/server/src/serverLayers.tsapps/server/src/wsServer.test.tspackages/contracts/src/orchestration.tspackages/contracts/src/provider.ts
| export type ProviderRecoveryStrategy = | ||
| | "retry-backoff" | ||
| | "fail-fast" | ||
| | "re-resolve-config" | ||
| | "fresh-session" | ||
| | "degrade-gracefully" | ||
| | "restart-session"; | ||
|
|
||
| export interface ProviderErrorClassification { | ||
| readonly category: ProviderErrorCategory; | ||
| readonly recoveryStrategy: ProviderRecoveryStrategy; | ||
| readonly recoverable: boolean; | ||
| } |
There was a problem hiding this comment.
These “recovery strategies” never influence control flow.
The only consumer in apps/server/src/provider/Layers/ProviderService.ts (Lines 336-360, 555-562, and 813-820) records the classification to analytics and then immediately rethrows the cause. Right now retry-backoff, fresh-session, restart-session, etc. are telemetry labels only, so this new surface does not implement the recovery behavior the names imply.
Also applies to: 238-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Errors.ts` around lines 171 - 183,
ProviderRecoveryStrategy and ProviderErrorClassification currently only tag
errors for telemetry but never affect control flow; update the handling in
ProviderService (where classifications are recorded and the cause rethrown) to
enact the named strategies instead of just logging: implement a retry/backoff
loop for "retry-backoff", trigger config re-resolution for "re-resolve-config",
recreate auth/session state for "fresh-session" and "restart-session" (with
restart doing a more forceful refresh), perform graceful degradation (return
fallback/partial response) for "degrade-gracefully", and immediately abort for
"fail-fast"; alternatively, if behavioral changes are unwanted, rename the union
and documentation to make them explicit telemetry-only labels and add TODOs
where ProviderErrorClassification is used to avoid misleading names.
| const raw = yield* fileSystem | ||
| .readFileString(targetPath) | ||
| .pipe(Effect.catch(() => Effect.succeed<string | null>(null))); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the McpConfig.ts file
find . -name "McpConfig.ts" -type fRepository: Ranvier-Technologies/t3code-OTP
Length of output: 170
🏁 Script executed:
# Once located, check the file size and read the relevant sections
wc -l apps/server/src/provider/Layers/McpConfig.tsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 123
🏁 Script executed:
# Read the code around lines 182-184
sed -n '175,195p' apps/server/src/provider/Layers/McpConfig.tsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 781
🏁 Script executed:
# Read the code around lines 295-307
sed -n '290,315p' apps/server/src/provider/Layers/McpConfig.tsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 942
🏁 Script executed:
# Check Effect-TS version and look at its type definitions
cat apps/server/package.json | grep -A 2 -B 2 "effect"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 715
🌐 Web query:
Effect-TS Effect.catch Effect.catchAll type signature documentation
💡 Result:
In Effect-TS (effect package), the functions for catching errors are Effect.catchAll (catches all errors in the error channel) and related variants like Effect.catchTag, Effect.catchTags, Effect.catchAllCause (for Cause including defects/interrupts). There is no Effect.catch; instead, Effect.catchAll handles expected failures (E channel), while Effect.catchAllCause handles full causes. Effect.catchAll type signature (pipeable and data-last forms): export const catchAll: { <E, A2, E2, R2>(f: (e: E) => Effect<A2, E2, R2>): <A, R>(self: Effect<A, E, R>) => Effect<A | A2, E2, R | R2> <A, E, R, A2, E2, R2>(self: Effect<A, E, R>, f: (e: E) => Effect<A2, E2, R2>): Effect<A | A2, E2, R | R2> } Usage example: program.pipe( Effect.catchAll((error) => Effect.succeed("fallback")) ) Effect.catchAllCause (for Cause): Similar signature but with Cause in error position, to handle failures, defects, interrupts. Documentation: Effect provides type-safe error recovery. catchAll recovers from any error by providing a recovery Effect. For specific tagged errors, use catchTag/catchTags. Sources confirm these are standard since v2.0.0.
Citations:
- 1: https://effect-ts.github.io/effect/effect/Effect.ts.html
- 2: https://effect.website/docs/getting-started/creating-effects/
- 3: https://www.mintlify.com/Effect-TS/effect/core/error-handling
- 4: https://effect-ts.github.io/effect/platform/HttpClient.ts.html
- 5: https://fredcollman.co.uk/courses/daily-effect/effect.catchall/
🏁 Script executed:
# Check if the TypeScript compilation would work by looking for type definitions
# Search for Effect.catch in the node_modules or type definitions
cd apps/server && npm ls effect 2>/dev/null | head -10Repository: Ranvier-Technologies/t3code-OTP
Length of output: 125
🏁 Script executed:
# Look at import statements in McpConfig.ts to see what's being imported from effect
head -30 apps/server/src/provider/Layers/McpConfig.ts | grep -E "^import|^from"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 277
🏁 Script executed:
# Try to understand the exact Error type being caught - look at context around the Effect.catch calls
sed -n '1,50p' apps/server/src/provider/Layers/McpConfig.tsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 1737
🏁 Script executed:
# Check if there's a tsconfig to understand strict type checking
cat apps/server/tsconfig.jsonRepository: Ranvier-Technologies/t3code-OTP
Length of output: 837
🏁 Script executed:
# Search for Effect.catch usage patterns in the codebase to see if this is a pattern
rg "Effect\.catch" apps/server/src/ -A 2 -B 2Repository: Ranvier-Technologies/t3code-OTP
Length of output: 35491
🏁 Script executed:
# Check the Effect package exports to understand what's available
# Look at node_modules to find the actual Effect types (or search for effect.d.ts)
find . -path "*/node_modules/effect/dist/types*" -name "*.d.ts" 2>/dev/null | head -5Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🌐 Web query:
Effect-TS v3 Effect.catch function signature discriminator
💡 Result:
In Effect-TS v3, there is no function named Effect.catch. The primary error handling functions are Effect.catchAll, Effect.catchTag, and Effect.catchTags, which use tagged/discriminated errors via the _tag field for type-safe handling. Effect.catchAll signature (from docs): export const catchAll: { <E, A2, E2, R2>(f: (e: E) => Effect<A2, E2, R2>): <A, R>(self: Effect<A, E, R>) => Effect<A2 | A, E2, R2 | R> <A, E, R, A2, E2, R2>(self: Effect<A, E, R>, f: (e: E) => Effect<A2, E2, R2>): Effect<A2 | A, E2, R2 | R> } Effect.catchTag signature (handles specific _tag discriminator): export const catchTag: { <E, const K extends E extends { _tag: string } ? E["_tag"] : never, A1, E1, R1>( ...args: [...tags: K, f: (e: Extract<E, { _tag: K }>) => Effect<A1, E1, R1>] ): <A, R>(self: Effect<A, E, R>) => Effect<A | A1, Exclude<E, { _tag: K }> | E1, R | R1> } Effect.catchTags (for multiple tags): program.pipe( Effect.catchTags({ HttpError: (error) => Effect.succeed(HTTP Error ${error.status}), ValidationError: (error) => Effect.succeed(Invalid field: ${error.field}) }) ) The "discriminator" refers to the _tag field in tagged errors (e.g., class MyError { readonly _tag = "MyError" }), enabling discriminated union pattern matching in catchTag/catchTags. Recent updates (PR #4796, merged Apr 2025) allow Effect.catchTag to handle multiple tags.
Citations:
- 1: https://effect-ts.github.io/effect/effect/Effect.ts.html
- 2: https://www.mintlify.com/Effect-TS/effect/core/error-handling
- 3: https://effect.website/docs/error-management/matching/
- 4: https://www.trycatch.fi
- 5: allow catching multiple different tags in Effect.catchTag Effect-TS/effect#4796
🏁 Script executed:
# Check if there's version information in the monorepo catalog
cat pnpm-workspace.yaml 2>/dev/null || cat pnpm-lock.yaml 2>/dev/null | head -30Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Look for the effect version in the root package.json
cat package.json | grep -A 10 "catalogs"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Check if Effect.catch is actually available - search type definitions or imports
rg "export.*catch" --type typescript | grep -i effect | head -20Repository: Ranvier-Technologies/t3code-OTP
Length of output: 109
🏁 Script executed:
# Find the root package.json and check for catalog configuration
find . -maxdepth 1 -name "pnpm-workspace.yaml" -o -name ".npmrc"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Check the monorepo structure
ls -la | grep -E "pnpm|package"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 200
🏁 Script executed:
# Look for any Effect.catch usage to see if it compiles
rg "Effect\.catch" apps/server/src --type ts --type tsx | head -5Repository: Ranvier-Technologies/t3code-OTP
Length of output: 102
🏁 Script executed:
# Check if there's a catalog reference for effect version
cat pnpm-workspace.yaml 2>/dev/null || trueRepository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Look in root for version info
head -50 ../../package.json 2>/dev/null | grep -E "version|catalog" || head -50 pnpm-workspace.yaml 2>/dev/null | grep -E "effect"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Check package.json in root for catalog definitions
cat package.json | grep -A 20 "catalogs"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Search for Effect.catch in the codebase without type specification
rg "Effect\.catch\(" apps/server/src/ | head -5Repository: Ranvier-Technologies/t3code-OTP
Length of output: 480
🏁 Script executed:
# Check what's in the effect package exports
find node_modules/effect -name "*.d.ts" 2>/dev/null | head -5Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Get direct export information from effect if available
cat node_modules/effect/package.json 2>/dev/null | grep '"version"'Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Try to understand if this code compiles - look for TypeScript errors or compile logs
find . -name "tsconfig*.json" -o -name "tsup.config.ts" | head -5Repository: Ranvier-Technologies/t3code-OTP
Length of output: 222
🏁 Script executed:
# Try to run typecheck to see if there are compilation errors
cd apps/server && npm run typecheck 2>&1 | grep -i "catch\|error" | head -20Repository: Ranvier-Technologies/t3code-OTP
Length of output: 454
🏁 Script executed:
# Check if there's a build output or if compilation passes
cd apps/server && npm run build 2>&1 | head -50 || echo "Build not available"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 1348
🏁 Script executed:
# Let's look at the exact error if any from Effect's type definitions
grep -r "export.*catch" node_modules/effect/dist/types 2>/dev/null | head -10 || echo "Type defs not accessible"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Try to understand the actual Effect version being used
cat pnpm-lock.yaml 2>/dev/null | grep -A 3 '"effect"' | head -20 || echo "Lock file not readable"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Check if Effect.catch is a discriminator-based overload by examining actual usages
rg "Effect\.catch\(" apps/server/src/provider/Layers/McpConfig.ts -B 2 -A 2Repository: Ranvier-Technologies/t3code-OTP
Length of output: 701
🏁 Script executed:
# Let's look at what's imported from effect in McpConfig.ts
head -20 apps/server/src/provider/Layers/McpConfig.tsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 533
🏁 Script executed:
# Search for Effect.catch definition or type in the codebase
rg "Effect\.catch\s*[=:]" apps/server/src --type tsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Check if there's any custom Effect utilities being used
rg "const catch|export.*catch" apps/server/src --type tsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Let's check the Effect package version more directly from pnpm-lock
head -100 pnpm-lock.yaml | grep -E "version|packages" | head -20Repository: Ranvier-Technologies/t3code-OTP
Length of output: 148
🏁 Script executed:
# Search pnpm-lock for effect package version
rg '"effect"@' pnpm-lock.yaml --max-count 5Repository: Ranvier-Technologies/t3code-OTP
Length of output: 129
🏁 Script executed:
# Check if Effect has any custom extensions or if catch is defined locally
rg "catch.*=" apps/server/src --type ts -A 2 | grep -E "catch\s*[=:|]" | head -10Repository: Ranvier-Technologies/t3code-OTP
Length of output: 1133
🏁 Script executed:
# Look at all imports from "effect" package
rg 'from "effect"' apps/server/src --type ts -A 2Repository: Ranvier-Technologies/t3code-OTP
Length of output: 45201
🏁 Script executed:
# Check the actual effect package that's available - list its exports
ls -la node_modules/effect/ 2>/dev/null | head -20 || echo "Node modules not installed in sandbox"Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
# Let's try a Python script to analyze the codebase and understand the pattern
python3 << 'EOF'
import os
import re
# Find all Effect.catch usages
pattern = r'Effect\.catch\([^)]*\)'
catch_usages = []
for root, dirs, files in os.walk('apps/server/src'):
for file in files:
if file.endswith('.ts'):
filepath = os.path.join(root, file)
with open(filepath, 'r') as f:
content = f.read()
matches = re.finditer(pattern, content)
for match in matches:
# Get context
start = max(0, match.start() - 50)
end = min(len(content), match.end() + 50)
context = content[start:end]
catch_usages.append({
'file': filepath,
'match': match.group(0),
'context': context
})
print(f"Found {len(catch_usages)} usages of Effect.catch")
for i, usage in enumerate(catch_usages[:3]):
print(f"\n{i+1}. {usage['file']}")
print(f" Pattern: {usage['match']}")
EOFRepository: Ranvier-Technologies/t3code-OTP
Length of output: 284
Replace Effect.catch with Effect.catchAll for proper error handling.
In modern Effect-TS, Effect.catch is not a valid function; the correct error handling combinators are Effect.catchAll (catches any error) or Effect.catchTags (for discriminated errors). All three instances in this file use the single-argument pattern which should use Effect.catchAll.
🛠️ Proposed fix
const raw = yield* fileSystem
.readFileString(targetPath)
- .pipe(Effect.catch(() => Effect.succeed<string | null>(null)));
+ .pipe(Effect.catchAll(() => Effect.succeed<string | null>(null)));
@@
const setSnapshot = (threadId: ThreadId, config: ResolvedMcpConfig) =>
- persistSnapshot(threadId, config).pipe(Effect.catch(() => cacheSnapshot(threadId, config)));
+ persistSnapshot(threadId, config).pipe(
+ Effect.catchAll(() => cacheSnapshot(threadId, config)),
+ );
@@
const exists = yield* fileSystem.exists(targetPath).pipe(Effect.orElseSucceed(() => false));
if (exists) {
- yield* fileSystem.remove(targetPath).pipe(Effect.catch(() => Effect.void));
+ yield* fileSystem.remove(targetPath).pipe(Effect.catchAll(() => Effect.void));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/McpConfig.ts` around lines 182 - 184, The
code uses Effect.catch(...) (invalid in modern Effect-TS) when wrapping
fileSystem.readFileString(targetPath) and two other similar calls; replace each
Effect.catch(...) with Effect.catchAll(...) so errors are properly caught (e.g.,
change the assignment creating raw from
fileSystem.readFileString(targetPath).pipe(Effect.catch(() =>
Effect.succeed<string | null>(null))) to use Effect.catchAll instead), keeping
the same error handler functions and types.
| {Registry, keys: :unique, name: Harness.SessionRegistry}, | ||
| {DynamicSupervisor, name: Harness.SessionSupervisor, strategy: :one_for_one}, | ||
| Harness.Storage, | ||
| Harness.Metrics, |
There was a problem hiding this comment.
🟡 Non-critical Metrics GenServer placed before critical services in rest_for_one supervision tree
In application.ex:16, Harness.Metrics is inserted between Harness.Storage and Harness.SnapshotServer in the children list. The supervisor uses strategy: :rest_for_one (line 20), which means if Metrics crashes, both Harness.SnapshotServer and HarnessWeb.Endpoint are terminated and restarted. Metrics is a non-critical observability service — its crash should not cascade-restart the web endpoint and snapshot server. Per AGENTS.md: "Reliability first. Keep behavior predictable under load and during failures."
Prompt for agents
In apps/harness/lib/harness/application.ex, move Harness.Metrics to AFTER HarnessWeb.Endpoint in the children list (making it the last child), so that under the rest_for_one strategy, a Metrics crash does not cascade-restart the critical SnapshotServer and Endpoint. Alternatively, consider placing Metrics in a separate supervisor or using a one_for_one strategy for the non-critical service.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/serverLayers.ts (1)
301-317:⚠️ Potential issue | 🟠 MajorAdd
T3CODE_CODEX_LEGACYcheck tomakeProviderRegistryLayer()to avoid registry/adapter drift.
makeProviderRegistryLayer()always returnsProviderRegistryLivewhen harness is disabled, but that registry includes Codex. However, whenT3CODE_CODEX_LEGACYis not set and harness is disabled (Path C),makeServerProviderLayer()errors and provides only Claude adapters. This causes the registry to advertise Codex while adapters do not provide it, leading to discovery/lookup failures.
makeProviderRegistryLayer()must also checkT3CODE_CODEX_LEGACYto exclude Codex from the registry in Path C, or raise an equivalent error before the registry is used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/serverLayers.ts` around lines 301 - 317, makeProviderRegistryLayer currently returns ProviderRegistryLive whenever harness is disabled, which can advertise Codex even when adapters (from makeServerProviderLayer) exclude it; update makeProviderRegistryLayer to consult the T3CODE_CODEX_LEGACY flag on ServerConfig and, when harnessEnabled is false and T3CODE_CODEX_LEGACY is falsy, return a registry that excludes Codex (or throw a clear error) instead of ProviderRegistryLive; locate the check around ServerConfig and harnessEnabled in makeProviderRegistryLayer and branch to either ProviderRegistryWithHarnessLive, ProviderRegistryLive, or a Codex-excluded registry (or raise) based on serverConfig.T3CODE_CODEX_LEGACY so registry and adapters remain consistent.
♻️ Duplicate comments (1)
apps/server/src/provider/Layers/McpConfig.ts (1)
192-195:⚠️ Potential issue | 🔴 CriticalVerify these
Effect.catch(...)calls against the pinned Effect API.If this repo is still on the standard Effect API surface, these recovery sites need
catchAll/catchTag-style combinators instead. LeavingEffect.catch(...)here can turn the snapshot fallback paths into a typecheck failure instead of a best-effort read/write fallback.In the Effect version used by this repository, is `Effect.catch(...)` a supported combinator? If not, what is the correct replacement for: 1. `effect.pipe(Effect.catch(() => Effect.succeed(...)))` 2. `persistSnapshot(...).pipe(Effect.catch(() => ...))` 3. `fileSystem.remove(...).pipe(Effect.catch(() => Effect.void))`Also applies to: 304-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/McpConfig.ts` around lines 192 - 195, The code is using the unsupported Effect.catch combinator; replace these with the correct catchAll-style combinator to perform recovery without causing type errors: change the fileSystem.readFileString(...).pipe(Effect.catch(...)) to fileSystem.readFileString(...).pipe(Effect.catchAll(() => Effect.succeed<string | null>(null))) in the McpConfig read path, change persistSnapshot(...).pipe(Effect.catch(...)) to persistSnapshot(...).pipe(Effect.catchAll(() => Effect.unit or Effect.succeedVoid())) for the snapshot persistence fallback, and change fileSystem.remove(...).pipe(Effect.catch(...)) to fileSystem.remove(...).pipe(Effect.catchAll(() => Effect.unit)) so all recoveries use the correct Effect.catchAll combinator (or the appropriate catchTag variant if you need to handle specific error tags).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 922-933: The current teardown unconditionally emits
analytics.record("provider.session.end", ...) even when
takeSessionTelemetry(input.threadId) returned undefined (causing duplicate end
events if processRuntimeEvent already handled the session.exited); change the
logic so provider.session.end is only recorded when sessionTelemetry is non-null
(i.e., when takeSessionTelemetry returned a telemetry object), and do not emit a
second end event otherwise; additionally, ensure any outstanding turn telemetry
for the thread is cleared during teardown by invoking the existing
turn-telemetry cleanup routine (e.g., clearTurnTelemetryForThread or similar)
before/after calling clearSnapshot and directory.remove so telemetry state is
fully cleared.
In `@apps/server/src/provider/Layers/ProviderSessionDirectory.ts`:
- Around line 102-109: The migration only updates the value returned by
getBinding but upsert still writes the legacy key via binding.provider /
existingRuntime?.adapterKey, so persist the migrated adapter key to complete the
cutover: in the upsert logic (function upsert) replace the fallback that uses
binding.provider or existingRuntime?.adapterKey with the adapterKey produced by
migrateAdapterKey (or use the migrated value from getBinding), and ensure
migrateAdapterKey's output is threaded into the runtime object you persist
instead of allowing the old "codex" value to be written; verify getBinding and
upsert both consume the migrated adapterKey so future reads no longer rely on
the shim.
In `@apps/server/src/provider/mcpTranslation.ts`:
- Around line 13-15: sanitizeName currently maps different server names to the
same TOML key (e.g., "foo bar" -> "foo_bar"), causing overwritten/duplicate
sections in codexTomlFromResolved; fix by making the mapping bijective or by
detecting collisions: either (A) change sanitizeName to produce a reversible,
collision-free identifier (e.g., URL-encode or use base64url of the original
name) so each original name maps to a unique TOML key, or (B) keep the current
sanitization but in codexTomlFromResolved build a map of sanitized -> original
names and throw a clear error if multiple originals produce the same sanitized
key (listing conflicts) before emitting TOML; update references to sanitizeName
and the collision check in codexTomlFromResolved accordingly.
- Around line 89-95: generatedMcpDir currently uses String(threadId) directly
which allows path-traversal via `..` or separators; replace that by computing a
safe, deterministic filename from the threadId (e.g., hex or base64url of a
cryptographic hash) and use that hashed/encoded value in path.join instead of
the raw threadId; update the generatedMcpDir function to import/use Node's
crypto hashing (or a strict encoder) and return path.join(stateDir, "mcp",
provider, safeId) where safeId is the hashed/encoded threadId to prevent
escaping the intended directory.
In `@apps/server/src/serverLayers.ts`:
- Around line 205-210: The error message created in the provider check (see
provider variable and the new Error thrown) incorrectly suggests setting
T3CODE_CODEX_LEGACY=1 for 'cursor' and 'opencode'; that flag only restores the
direct Codex adapter. Update the thrown Error so it conditionally includes the
T3CODE_CODEX_LEGACY suggestion only when provider === "codex", and for provider
=== "cursor" or "opencode" remove that suggestion and instead instruct operators
to configure harnessPort or use the Elixir harness (keep the existing
harnessPort guidance). Ensure the change is only in the error text constructed
where the harnessPort error is thrown.
- Around line 137-145: The code eagerly resolves CodexAdapter and constructs
codexAdapterLayer even when useLegacyCodex is false, coupling the harness path
to direct-Codex startup; change the logic so CodexAdapter is only yielded and
codexAdapterLayer only created/added to byProvider when useLegacyCodex is true:
move the yield* CodexAdapter and any codexAdapterLayer construction inside the
if (useLegacyCodex) block and only call byProvider.set("codex", codexAdapter)
from that block, leaving claudeAdapter, HarnessClientAdapter and byProvider
creation untouched for the non-legacy path.
---
Outside diff comments:
In `@apps/server/src/serverLayers.ts`:
- Around line 301-317: makeProviderRegistryLayer currently returns
ProviderRegistryLive whenever harness is disabled, which can advertise Codex
even when adapters (from makeServerProviderLayer) exclude it; update
makeProviderRegistryLayer to consult the T3CODE_CODEX_LEGACY flag on
ServerConfig and, when harnessEnabled is false and T3CODE_CODEX_LEGACY is falsy,
return a registry that excludes Codex (or throw a clear error) instead of
ProviderRegistryLive; locate the check around ServerConfig and harnessEnabled in
makeProviderRegistryLayer and branch to either ProviderRegistryWithHarnessLive,
ProviderRegistryLive, or a Codex-excluded registry (or raise) based on
serverConfig.T3CODE_CODEX_LEGACY so registry and adapters remain consistent.
---
Duplicate comments:
In `@apps/server/src/provider/Layers/McpConfig.ts`:
- Around line 192-195: The code is using the unsupported Effect.catch
combinator; replace these with the correct catchAll-style combinator to perform
recovery without causing type errors: change the
fileSystem.readFileString(...).pipe(Effect.catch(...)) to
fileSystem.readFileString(...).pipe(Effect.catchAll(() => Effect.succeed<string
| null>(null))) in the McpConfig read path, change
persistSnapshot(...).pipe(Effect.catch(...)) to
persistSnapshot(...).pipe(Effect.catchAll(() => Effect.unit or
Effect.succeedVoid())) for the snapshot persistence fallback, and change
fileSystem.remove(...).pipe(Effect.catch(...)) to
fileSystem.remove(...).pipe(Effect.catchAll(() => Effect.unit)) so all
recoveries use the correct Effect.catchAll combinator (or the appropriate
catchTag variant if you need to handle specific error tags).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ffe3afa1-bece-47a2-a9e5-d591f3192a0e
📒 Files selected for processing (28)
ai_docs/provider_onboarding.mdapps/harness/lib/harness/application.exapps/harness/lib/harness/metrics.exapps/harness/lib/harness/providers/claude_session.exapps/harness/lib/harness/providers/codex_session.exapps/harness/lib/harness/providers/cursor_session.exapps/harness/lib/harness/providers/opencode_session.exapps/harness/lib/harness/providers/provider_behaviour.exapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/integration/TestProviderAdapter.integration.tsapps/server/integration/contract.integration.test.tsapps/server/integration/providerService.integration.test.tsapps/server/src/codexAppServerManager.tsapps/server/src/provider/Errors.tsapps/server/src/provider/Layers/CodexAdapter.test.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/HarnessClientAdapter.tsapps/server/src/provider/Layers/HarnessProvider.tsapps/server/src/provider/Layers/McpConfig.tsapps/server/src/provider/Layers/ProviderRegistry.tsapps/server/src/provider/Layers/ProviderService.test.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/Layers/ProviderSessionDirectory.tsapps/server/src/provider/Layers/codexHarnessCutover.test.tsapps/server/src/provider/Services/CodexAdapter.tsapps/server/src/provider/mcpTranslation.tsapps/server/src/serverLayers.tspackages/contracts/src/provider.ts
✅ Files skipped from review due to trivial changes (6)
- apps/harness/lib/harness/application.ex
- apps/server/src/provider/Services/CodexAdapter.ts
- apps/server/src/provider/Layers/HarnessProvider.ts
- ai_docs/provider_onboarding.md
- apps/server/src/codexAppServerManager.ts
- apps/harness/lib/harness/providers/provider_behaviour.ex
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/server/integration/OrchestrationEngineHarness.integration.ts
- apps/server/src/provider/Layers/ProviderRegistry.ts
- apps/harness/lib/harness/providers/opencode_session.ex
- apps/server/integration/providerService.integration.test.ts
- apps/harness/lib/harness/providers/cursor_session.ex
- apps/harness/lib/harness/providers/codex_session.ex
- apps/harness/lib/harness/providers/claude_session.ex
- apps/server/src/provider/Layers/CodexAdapter.ts
- apps/server/integration/TestProviderAdapter.integration.ts
- apps/server/src/provider/Layers/codexHarnessCutover.test.ts
- apps/server/integration/contract.integration.test.ts
- apps/harness/lib/harness/metrics.ex
- apps/server/src/provider/Errors.ts
| yield* mcpConfig.clearSnapshot(input.threadId); | ||
| yield* directory.remove(input.threadId); | ||
| const sessionTelemetry = yield* takeSessionTelemetry(input.threadId); | ||
| yield* analytics.record("provider.session.stopped", { | ||
| provider: routed.adapter.provider, | ||
| }); | ||
| yield* analytics.record("provider.session.end", { | ||
| provider: routed.adapter.provider, | ||
| adapterPath: sessionTelemetry?.adapterPath ?? routed.adapterPath, | ||
| durationMs: sessionTelemetry ? Date.now() - sessionTelemetry.startedAtMs : null, | ||
| endReason: "explicit", | ||
| }); |
There was a problem hiding this comment.
Don't emit provider.session.end twice on explicit stops.
processRuntimeEvent() already consumes the session telemetry and records provider.session.end when a session.exited event arrives. This block records another end event even when takeSessionTelemetry() returned undefined, so adapters that emit a shutdown event during stopSession() will double-count the stop and lose the real duration. This teardown should also clear any outstanding turn telemetry.
🛠️ Suggested fix
yield* mcpConfig.clearSnapshot(input.threadId);
yield* directory.remove(input.threadId);
const sessionTelemetry = yield* takeSessionTelemetry(input.threadId);
+ yield* clearTurnTelemetry(input.threadId);
yield* analytics.record("provider.session.stopped", {
provider: routed.adapter.provider,
});
- yield* analytics.record("provider.session.end", {
- provider: routed.adapter.provider,
- adapterPath: sessionTelemetry?.adapterPath ?? routed.adapterPath,
- durationMs: sessionTelemetry ? Date.now() - sessionTelemetry.startedAtMs : null,
- endReason: "explicit",
- });
+ if (sessionTelemetry) {
+ yield* analytics.record("provider.session.end", {
+ provider: routed.adapter.provider,
+ adapterPath: sessionTelemetry.adapterPath,
+ durationMs: Date.now() - sessionTelemetry.startedAtMs,
+ endReason: "explicit",
+ });
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ProviderService.ts` around lines 922 - 933,
The current teardown unconditionally emits
analytics.record("provider.session.end", ...) even when
takeSessionTelemetry(input.threadId) returned undefined (causing duplicate end
events if processRuntimeEvent already handled the session.exited); change the
logic so provider.session.end is only recorded when sessionTelemetry is non-null
(i.e., when takeSessionTelemetry returned a telemetry object), and do not emit a
second end event otherwise; additionally, ensure any outstanding turn telemetry
for the thread is cleared during teardown by invoking the existing
turn-telemetry cleanup routine (e.g., clearTurnTelemetryForThread or similar)
before/after calling clearSnapshot and directory.remove so telemetry state is
fully cleared.
| Effect.map((provider) => { | ||
| // Migrate legacy adapter_key values (Task 007) | ||
| const { key: adapterKey } = migrateAdapterKey(value.adapterKey, value.threadId); | ||
| return Option.some({ | ||
| threadId: value.threadId, | ||
| provider, | ||
| adapterKey: value.adapterKey, | ||
| adapterKey, | ||
| runtimeMode: value.runtimeMode, |
There was a problem hiding this comment.
This migration never becomes durable while upsert() keeps writing the legacy key.
migrateAdapterKey() only rewrites the value returned from getBinding(). But upsert() still falls back to binding.provider / existingRuntime?.adapterKey at Lines 141-143, so new and updated Codex rows continue to persist "codex" instead of "harness:codex". That means every future read still depends on this shim instead of completing the cutover in storage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ProviderSessionDirectory.ts` around lines 102
- 109, The migration only updates the value returned by getBinding but upsert
still writes the legacy key via binding.provider / existingRuntime?.adapterKey,
so persist the migrated adapter key to complete the cutover: in the upsert logic
(function upsert) replace the fallback that uses binding.provider or
existingRuntime?.adapterKey with the adapterKey produced by migrateAdapterKey
(or use the migrated value from getBinding), and ensure migrateAdapterKey's
output is threaded into the runtime object you persist instead of allowing the
old "codex" value to be written; verify getBinding and upsert both consume the
migrated adapterKey so future reads no longer rely on the shim.
| function sanitizeName(name: string): string { | ||
| return name.replace(/[^a-zA-Z0-9_-]+/g, "_"); | ||
| } |
There was a problem hiding this comment.
sanitizeName() can collapse distinct servers into the same TOML table.
This mapping is not one-to-one: names like foo bar and foo_bar, or a/b and a.b, both render to the same [mcp_servers.*] key. codexTomlFromResolved() will then emit duplicate/overwritten sections for otherwise valid ResolvedMcpConfig input. Preserve a bijection here, or reject collisions before rendering.
Also applies to: 24-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/mcpTranslation.ts` around lines 13 - 15,
sanitizeName currently maps different server names to the same TOML key (e.g.,
"foo bar" -> "foo_bar"), causing overwritten/duplicate sections in
codexTomlFromResolved; fix by making the mapping bijective or by detecting
collisions: either (A) change sanitizeName to produce a reversible,
collision-free identifier (e.g., URL-encode or use base64url of the original
name) so each original name maps to a unique TOML key, or (B) keep the current
sanitization but in codexTomlFromResolved build a map of sanitized -> original
names and throw a clear error if multiple originals produce the same sanitized
key (listing conflicts) before emitting TOML; update references to sanitizeName
and the collision check in codexTomlFromResolved accordingly.
| export function generatedMcpDir( | ||
| stateDir: string, | ||
| provider: "codex" | "cursor" | "opencode", | ||
| threadId: ThreadId, | ||
| ): string { | ||
| return path.join(stateDir, "mcp", provider, String(threadId)); | ||
| } |
There was a problem hiding this comment.
Hash or encode threadId before joining it into the MCP directory.
String(threadId) is used as a raw path segment here, and the harness path later deletes this directory recursively before recreating it. A crafted id containing .. or path separators can escape stateDir/mcp/<provider> and turn session startup into arbitrary deletion or overwrite on disk.
🔒 Suggested fix
+import { createHash } from "node:crypto";
import path from "node:path";
@@
export function generatedMcpDir(
stateDir: string,
provider: "codex" | "cursor" | "opencode",
threadId: ThreadId,
): string {
- return path.join(stateDir, "mcp", provider, String(threadId));
+ const safeThreadId = createHash("sha256").update(String(threadId)).digest("hex");
+ return path.join(stateDir, "mcp", provider, safeThreadId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function generatedMcpDir( | |
| stateDir: string, | |
| provider: "codex" | "cursor" | "opencode", | |
| threadId: ThreadId, | |
| ): string { | |
| return path.join(stateDir, "mcp", provider, String(threadId)); | |
| } | |
| import { createHash } from "node:crypto"; | |
| import path from "node:path"; | |
| export function generatedMcpDir( | |
| stateDir: string, | |
| provider: "codex" | "cursor" | "opencode", | |
| threadId: ThreadId, | |
| ): string { | |
| const safeThreadId = createHash("sha256").update(String(threadId)).digest("hex"); | |
| return path.join(stateDir, "mcp", provider, safeThreadId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/mcpTranslation.ts` around lines 89 - 95,
generatedMcpDir currently uses String(threadId) directly which allows
path-traversal via `..` or separators; replace that by computing a safe,
deterministic filename from the threadId (e.g., hex or base64url of a
cryptographic hash) and use that hashed/encoded value in path.join instead of
the raw threadId; update the generatedMcpDir function to import/use Node's
crypto hashing (or a strict encoder) and return path.join(stateDir, "mcp",
provider, safeId) where safeId is the hashed/encoded threadId to prevent
escaping the intended directory.
| const codexAdapter = yield* CodexAdapter; | ||
| const harnessBaseAdapter = yield* HarnessClientAdapter; | ||
|
|
||
| type Adapter = ProviderAdapterShape<ProviderAdapterError>; | ||
| const byProvider = new Map<string, Adapter>(); | ||
|
|
||
| byProvider.set("claudeAgent", claudeAdapter); | ||
| if (useLegacyCodex) { | ||
| byProvider.set("codex", codexAdapter); |
There was a problem hiding this comment.
Don’t build the direct Codex adapter on the non-legacy harness path.
Line 137 still resolves CodexAdapter, and Line 172 still provides codexAdapterLayer, even when useLegacyCodex is false. That keeps the harness-only path coupled to direct Codex startup, so a direct-adapter init failure can still take down a harness-enabled server.
♻️ Possible fix
- const adapterRegistryLayer = harnessEnabled
- ? // Path A: harness available — route harness providers through it
- Layer.effect(
+ const harnessRegistryBaseLayer = Layer.effect(
ProviderAdapterRegistry,
Effect.gen(function* () {
const claudeAdapter = yield* ClaudeAdapter;
- const codexAdapter = yield* CodexAdapter;
const harnessBaseAdapter = yield* HarnessClientAdapter;
@@
byProvider.set("claudeAgent", claudeAdapter);
if (useLegacyCodex) {
- byProvider.set("codex", codexAdapter);
+ byProvider.set("codex", yield* CodexAdapter);
}
@@
}),
).pipe(
- Layer.provide(codexAdapterLayer),
Layer.provide(claudeAdapterLayer),
Layer.provideMerge(harnessAdapterLayer.pipe(Layer.provideMerge(McpConfigServiceLive))),
Layer.provideMerge(providerSessionDirectoryLayer),
- )
+ );
+
+ const adapterRegistryLayer = harnessEnabled
+ ? useLegacyCodex
+ ? harnessRegistryBaseLayer.pipe(Layer.provide(codexAdapterLayer))
+ : harnessRegistryBaseLayerAlso applies to: 172-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/serverLayers.ts` around lines 137 - 145, The code eagerly
resolves CodexAdapter and constructs codexAdapterLayer even when useLegacyCodex
is false, coupling the harness path to direct-Codex startup; change the logic so
CodexAdapter is only yielded and codexAdapterLayer only created/added to
byProvider when useLegacyCodex is true: move the yield* CodexAdapter and any
codexAdapterLayer construction inside the if (useLegacyCodex) block and only
call byProvider.set("codex", codexAdapter) from that block, leaving
claudeAdapter, HarnessClientAdapter and byProvider creation untouched for the
non-legacy path.
| ...(provider === "codex" || provider === "cursor" || provider === "opencode" | ||
| ? { | ||
| cause: new Error( | ||
| `Harness port is not configured. Provider '${provider}' requires the Elixir harness. ` + | ||
| `Set T3CODE_CODEX_LEGACY=1 to use the legacy direct adapter, or configure harnessPort.`, | ||
| ), |
There was a problem hiding this comment.
Fix the Path C remediation text for Cursor/OpenCode.
T3CODE_CODEX_LEGACY=1 only restores direct Codex. For cursor and opencode, the current error points operators to a fallback that does not exist.
✏️ Possible fix
- cause: new Error(
- `Harness port is not configured. Provider '${provider}' requires the Elixir harness. ` +
- `Set T3CODE_CODEX_LEGACY=1 to use the legacy direct adapter, or configure harnessPort.`,
- ),
+ cause: new Error(
+ provider === "codex"
+ ? "Harness port is not configured. Provider 'codex' requires the Elixir harness by default. Set T3CODE_CODEX_LEGACY=1 to use the legacy direct adapter, or configure harnessPort."
+ : `Harness port is not configured. Provider '${provider}' requires the Elixir harness. Configure harnessPort.`,
+ ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/serverLayers.ts` around lines 205 - 210, The error message
created in the provider check (see provider variable and the new Error thrown)
incorrectly suggests setting T3CODE_CODEX_LEGACY=1 for 'cursor' and 'opencode';
that flag only restores the direct Codex adapter. Update the thrown Error so it
conditionally includes the T3CODE_CODEX_LEGACY suggestion only when provider ===
"codex", and for provider === "cursor" or "opencode" remove that suggestion and
instead instruct operators to configure harnessPort or use the Elixir harness
(keep the existing harnessPort guidance). Ensure the change is only in the error
text constructed where the harnessPort error is thrown.
|
@ranvier2d2 please teach your AI bot to ping the correct Bastian and not me. Thx.
|
2 participants
What Changed
Merges PR #28 as base and cherry-picks 7 targeted additions from PR #27 to consolidate the provider architecture work that was developed in parallel on two branches.
From PR #28 (base): Full MCP config service with disk snapshots and config resolution, MCP TOML/JSON materialization for Codex/OpenCode, graduated
ProviderCapabilityLevelfields ("none" | "basic" | "full"), error classification with recovery strategies, session lifecycle telemetry viaRef-based duration tracking, contract integration tests, and provider session schemas.Cherry-picked from PR #27:
ai_docs/failure_matrix.md— 22-row operation × provider × error matrixai_docs/provider_onboarding.md— 9-step playbook for adding new providersvalidateResumeCursor()— cursor validation integrated into both recovery and start session pathscodexHarnessCutover.test.ts— 21 unit tests for cutover logic (adapter_key migration, cursor validation, feature flag, registry resolution)provider_behaviour.ex— Elixir behaviour module with 8 callbacks and@implannotationsserverLayers.ts— graceful error when harness is required but not configuredProviderSessionStatusUpdates since last revision
Bug fixes applied on top of the consolidated merge:
CodexAdapter.tsandHarnessClientAdapter.tsnow read the existingconfig.toml(copied from the user's base home) and append the MCP overlay, instead of overwriting user settings with MCP-only content.codexTomlFromResolved()now ends with\n(POSIX text file compliance).fs.rmSync(generatedDir, …)beforefs.mkdirSyncso eachstartSessionbegins with a clean generated directory. This prevents MCP[mcp_servers.X]table blocks from accumulating across session restarts for the samethreadId.Why
PRs #27 and #28 were developed in parallel and implemented overlapping functionality with different depth. PR #28 had superior implementation (real MCP config vs stub, richer error taxonomy, comprehensive telemetry), but PR #27 contributed valuable documentation, test coverage, and defensive patterns that PR #28 lacked. This PR combines the strengths of both.
Human Review Checklist
CodexAdapter.test.ts,ProviderService.test.ts). Confirmed pre-existing from PR Add provider capability model and MCP runtime support #28 — they reproduce on the pristine PR Add provider capability model and MCP runtime support #28 branch without any cherry-picks or bug fixes applied. The cherry-pickedcodexHarnessCutover.test.tspasses all 21 tests.startSession, the generated MCP dir is wiped (rmSync) → base home is copied (cpSync) → existingconfig.tomlis read → MCP overlay is appended. Verify this append-on-fresh-copy approach preserves user TOML sections correctly (e.g., no duplicate keys if user already has[mcp_servers]).recoverSessionForThread, an invalid cursor returns a hardProviderValidationError. InstartSession, an invalid cursor is silently discarded (starts fresh). This is intentional—recovery requires a valid cursor, but start can fall back—but verify this is the desired behavior.sessionModelSwitchchanged from"in-session"to"restart-session"inDEFAULT_PROVIDER_CAPABILITIES(from PR Add provider capability model and MCP runtime support #28). This is a behavioral change for OpenCode session management.provider === "codex" || ...) rather than theHARNESS_PROVIDERSconstant. Minor drift risk.provider_behaviour.excallbacks should align with whatSessionManagerdispatches.Checklist
Link to Devin session: https://app.devin.ai/sessions/b74d31c8638c421fb6cd6ece03bf3856
Requested by: @ranvier2d2
Summary by CodeRabbit
New Features
Documentation
Improvements