feat: provider architecture consolidation (Phase 0-3) - #27
Merged
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>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR expands the provider adapter system with new capability models, MCP configuration management, a provider behavior contract for Elixir harness providers, error classification logic, and a Codex legacy-to-harness migration feature flag. It adds harness metrics tracking, integration tests for provider contracts, and updates multiple adapter implementations across TypeScript and Elixir layers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
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 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
ai_docs/provider_onboarding.md (1)
90-103: Capabilities section missing graduatedCapabilityLevelfields.The capabilities documentation only shows the boolean flags but omits the new graduated capability fields introduced in this PR:
resume,subagents,attachments,replay, andmcpConfig. These useCapabilityLevel("none" | "basic" | "full") and are important for the contract test suite's capability-gating behavior.📝 Proposed update to include graduated capabilities
Set `ProviderAdapterCapabilities` for your provider: ```typescript { sessionModelSwitch: "in-session" | "restart-session" | "unsupported", supportsUserInput: boolean, supportsRollback: boolean, supportsFileChangeApproval: boolean, + // Graduated capabilities (CapabilityLevel: "none" | "basic" | "full") + resume: CapabilityLevel, + subagents: CapabilityLevel, + attachments: CapabilityLevel, + replay: CapabilityLevel, + mcpConfig: CapabilityLevel, }</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@ai_docs/provider_onboarding.mdaround lines 90 - 103, Update the
ProviderAdapterCapabilities example in the provider_onboarding.md docs to
include the new graduated CapabilityLevel fields (CapabilityLevel = "none" |
"basic" | "full") so the contract tests can gate correctly; specifically add
resume, subagents, attachments, replay, and mcpConfig as typed CapabilityLevel
entries alongside the existing sessionModelSwitch, supportsUserInput,
supportsRollback, and supportsFileChangeApproval (the contract test suite
referenced is apps/server/integration/contract.integration.test.ts). Ensure you
mention CapabilityLevel and show the new fields in the example object so
implementers know to provide those values.</details> </blockquote></details> <details> <summary>apps/harness/lib/harness/providers/cursor_session.ex (1)</summary><blockquote> `29-29`: **`mcp_config` is stored but not used.** The `:mcp_config` field is added to the struct (Line 29) and populated from params during `init/1` (Lines 109-110), but it's not referenced in `build_cursor_args/3` or `spawn_cursor_process/3`. If Cursor CLI supports MCP server configuration via command-line args or environment variables, consider wiring it through. If this is intentional scaffolding for future use (similar to the stub `McpConfigServiceLive`), a brief comment would clarify intent. Also applies to: 109-110 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@apps/harness/lib/harness/providers/cursor_session.exat line 29, The
:mcp_config struct field is set in init/1 but never used; either wire it into
the CLI invocation or mark it as intentional scaffolding: if MCP configuration
should be passed to the Cursor process, update build_cursor_args/3 and/or
spawn_cursor_process/3 to read the struct's :mcp_config and append the
appropriate CLI flags or environment variables (use the same flag names the
Cursor CLI expects), otherwise remove :mcp_config from the struct and init/1 or
add a one-line comment near the :mcp_config field referencing
McpConfigServiceLive to indicate it is a deliberate placeholder for future MCP
wiring.</details> </blockquote></details> <details> <summary>apps/harness/lib/harness/providers/codex_session.ex (1)</summary><blockquote> `96-96`: **MCP config stored but unused — intentional placeholder.** The `:mcp_config` field is populated from params but not referenced elsewhere in this file. This appears intentional for future MCP support. Consider adding a brief comment indicating this is reserved for future use. Also applies to: 172-173 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@apps/harness/lib/harness/providers/codex_session.exat line 96, The
:mcp_config field is being set from params but never used; add a concise inline
comment next to the :mcp_config occurrence(s) (the struct/param list entries
where :mcp_config appears and the other occurrence around lines referenced)
indicating it is intentionally reserved for future MCP support so reviewers know
it is not an accidental leftover; update both occurrences (the one at
:mcp_config and the related entries at the other occurrence) with the same brief
comment.</details> </blockquote></details> <details> <summary>apps/server/src/provider/Layers/ProviderService.ts (1)</summary><blockquote> `80-93`: **Consider structured logging over console.info for metrics.** Using `console.info` with JSON works but may get interleaved with other logs. Consider using Effect's logging with a dedicated metrics tag for better filtering: ```typescript Effect.logInfo("metric", { _t: "metric", name, ts: Date.now(), ...attributes })This would integrate better with Effect's logging infrastructure and allow filtering by log level.
🤖 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 80 - 93, Replace the raw console output in emitMetric with Effect's structured logging: change the implementation of emitMetric to call Effect.logInfo (or the project's equivalent) with a clear "metric" tag and the same payload ({ _t: "metric", name, ts: Date.now(), ...attributes }) so metrics are routed through Effect's logging pipeline and can be filtered by level/tag; update any imports or wrappers used by ProviderService to use Effect.logInfo and ensure emitMetric remains synchronous in signature or adjust callers if log calls become asynchronous.apps/server/integration/contract.integration.test.ts (1)
48-54: Local capabilities object duplicates test adapter capabilities.This capabilities object is used for
describe.skipIfconditions but isn't connected to the actual test adapter's declared capabilities. If the test adapter's capabilities change, these skip conditions won't update.Consider importing capabilities from the test adapter or querying them at test setup time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/integration/contract.integration.test.ts` around lines 48 - 54, Replace the hardcoded capabilities object used for describe.skipIf with the actual adapter-declared capabilities: remove or stop using the local const capabilities and instead import or read the capabilities from the test adapter (e.g., adapter.capabilities or await testAdapter.getCapabilities() during setup) and pass that into describe.skipIf so skip conditions reflect the adapter's real abilities; update any references to the local capabilities symbol to use the adapter-derived capabilities.apps/server/src/provider/Layers/HarnessClientAdapter.ts (1)
1347-1361: Consider: Empty array after filter should also return null.If all servers exist but none are enabled, the current implementation returns
{ mcp_config: { version, servers: [] } }rather thannull. This may be intentional, but could cause the Elixir side to receive an empty config.♻️ Optional fix to return null for empty enabled servers
translateMcpConfig: (config) => Effect.succeed( - config.servers.length > 0 + config.servers.filter((s) => s.enabled).length > 0 ? { mcp_config: { version: config.version, servers: config.servers .filter((s) => s.enabled) .map((s) => ({ name: s.name, command: s.command, args: s.args, ...(s.env ? { env: s.env } : {}), })), }, } : null, ),🤖 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 1347 - 1361, The code constructs mcp_config even when config.servers exists but no server is enabled, producing an empty servers array; update the logic in HarnessClientAdapter (the block building mcp_config) to compute enabledServers = config.servers?.filter(s => s.enabled) and only return the mcp_config object when enabledServers.length > 0 (otherwise return null), and use enabledServers.map(...) when building the servers list so an all-disabled list yields null instead of { mcp_config: { ..., servers: [] } }.apps/harness/lib/harness/metrics.ex (1)
111-117:safe_castreturns:okunconditionally — consider returning cast result.The function always returns
:okregardless of whether the cast was sent. This is fine for fire-and-forget semantics, but callers can't distinguish between "server running, cast sent" and "server not running, cast dropped."If observability of dropped casts matters, consider returning
{:ok, :sent}vs{:ok, :dropped}.🤖 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 111 - 117, safe_cast currently always returns :ok even when the GenServer isn't running; change it to detect Process.whereis(__MODULE__) and return {:ok, :sent} after calling GenServer.cast(__MODULE__, {msg, payload}) and {:ok, :dropped} when the process is missing (or propagate GenServer.cast result if you prefer), and update any callers that rely on the return value to handle the new {:ok, :sent} / {:ok, :dropped} results; keep the core logic in safe_cast (referencing safe_cast, Process.whereis/1, GenServer.cast/2 and __MODULE__) and ensure tests/checks cover both running and not-running cases.apps/server/src/serverLayers.ts (1)
81-86: ReadT3CODE_CODEX_LEGACYinside the factory.Line 86 caches the flag before
makeServerProviderLayer()runs, so later calls in the same loaded module cannot observe a changed env value. Moving this read into the factory, or intoServerConfig, keeps the cutover switch scoped to layer construction and easier to exercise in tests.♻️ Suggested change
-const useLegacyCodex = process.env.T3CODE_CODEX_LEGACY === "1"; - export function makeServerProviderLayer(options?: { harnessAdapterLayer?: ReturnType<typeof makeHarnessClientAdapterLive>; }): Layer.Layer< @@ > { return Effect.gen(function* () { + const useLegacyCodex = process.env.T3CODE_CODEX_LEGACY === "1"; const serverConfig = yield* ServerConfig;🤖 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 81 - 86, Currently the T3CODE_CODEX_LEGACY flag is read once into the top-level const useLegacyCodex, which freezes its value for the whole module; change this by moving the environment read into the factory so the flag is evaluated at layer construction time: remove the top-level useLegacyCodex reference and instead read process.env.T3CODE_CODEX_LEGACY === "1" inside makeServerProviderLayer() (or inside ServerConfig if you prefer), then use that local value to choose the Codex path so tests and later calls can observe env changes.
🤖 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 9-19: The fenced ASCII diagram block (the code fence containing
the Transport/ProviderService/ProviderAdapter diagram) lacks a language
specifier; update that code fence to use a neutral specifier such as text or
plaintext (e.g., change ``` to ```text) so markdownlint MD040 is satisfied,
leaving the diagram contents unchanged; locate the ASCII diagram block in the
Provider onboarding documentation and add the specifier to the opening fence.
In `@apps/server/src/provider/Errors.ts`:
- Around line 28-95: The classifyProviderError signature and switch need to
include McpConfigError: add McpConfigError to the union type parameter of
classifyProviderError and add a switch case for the tag "McpConfigError" that
returns "configuration" (mirror how ProviderUnsupportedError maps to
configuration); ensure any type import/definition for McpConfigError is
referenced so the compiler recognizes it and update the switch default behavior
unchanged.
In `@apps/server/src/provider/Layers/CodexAdapter.ts`:
- Around line 1610-1614: The CodexAdapter declares mcpConfig: "none" but its
translateMcpConfig function (translateMcpConfig) can return a non-null MCP
config when enabled servers exist, creating an inconsistency; either change the
capability value to "basic" to reflect that translateMcpConfig may return a
config, or change translateMcpConfig to always return null (matching other
adapters) — update the mcpConfig property in the adapter's capability object or
alter translateMcpConfig to unconditionally return null so both the capability
and translator behavior are consistent.
In `@apps/server/src/provider/Layers/codexHarnessCutover.test.ts`:
- Around line 156-162: Update the test so the assertion matches the actual
reason string returned by validateResumeCursor; locate the test using
validateResumeCursor in codexHarnessCutover.test.ts and replace the
expect(result.reason).toContain("parses to null/undefined") check with the exact
substring used by the validateResumeCursor implementation (e.g., the production
message it sets when parsing yields null/undefined).
- Around line 74-104: The test helper validateResumeCursor diverges from
production; update it to exactly match ProviderService.ts by (1) changing the
empty-string reason to "cursor string is empty after trimming", (2) only
treating parsed === null as invalid (remove the parsed === undefined check), and
(3) remove the explicit typeof cursor === "object" branch so non-string inputs
are treated as already-deserialized like production (i.e., return valid with the
original cursor for any non-string path); keep the JSON parsing and
error-message behavior identical to production.
In `@apps/server/src/provider/Layers/ProviderService.test.ts`:
- Around line 1190-1202: The capability suite layer composition for "layer"
created with it.layer and Layer.mergeAll is missing McpConfigServiceLive; update
the Layer.mergeAll composition used with makeProviderServiceLive() to also
provide/merge McpConfigServiceLive (same way other ProviderServiceLive
compositions do) so ProviderServiceLive's dependency on McpConfigService for
session start is satisfied; locate the composition that references
makeProviderServiceLive(), Layer.provide(providerAdapterLayer), directoryLayer,
runtimeRepositoryLayer, and NodeServices.layer and add McpConfigServiceLive into
that merge/provide list.
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 107-123: The function logClassifiedError is defined but unused;
either delete this dead helper or retain it with a clear TODO explaining why
it’s kept and when it will be used. If removing, delete the entire
logClassifiedError function (which references classifyProviderError,
ProviderErrorCategory, and Effect.logWarning) and run tests/lint to ensure no
references remain. If keeping, add a one-line TODO comment above
logClassifiedError stating its intended future use (e.g., for Effect.tapError
pipelines) and why it must remain, ensuring the comment includes the function
name for discoverability.
In `@apps/server/src/provider/Layers/ProviderSessionDirectory.ts`:
- Around line 95-102: The migration of adapterKey via migrateAdapterKey(...) is
being stored but not used for resolving adapters during recovery; update the
recovery/path-resolution logic (used by getBinding and wherever binding.provider
is read) to resolve adapters using the migrated adapterKey (binding.adapterKey)
instead of relying solely on binding.provider, or alternatively update
getBinding to replace binding.provider with the provider derived from the
migrated adapterKey; change references in ProviderSessionDirectory/getBinding
and any session recovery code to consult the migrated adapterKey and derive the
correct provider before dispatch so adapter-path ambiguity is eliminated.
In `@apps/server/src/serverLayers.ts`:
- Around line 255-261: The current ProviderUnsupportedError construction uses a
hardcoded Codex-specific hint for multiple providers; update the cause/message
generation in the ProviderUnsupportedError block (where ProviderUnsupportedError
is instantiated using the provider variable) to produce provider-specific
remediation text: if provider === "codex" include the Elixir harness hint and
T3CODE_CODEX_LEGACY=1 suggestion, and for provider === "cursor" or provider ===
"opencode" emit a different, accurate hint (or a generic “configure harnessPort
or use supported adapter” message) so callers for cursor/opencode are not
directed to Codex-only fixes; implement this by branching on provider or mapping
provider->hint before passing the cause to ProviderUnsupportedError.
---
Nitpick comments:
In `@ai_docs/provider_onboarding.md`:
- Around line 90-103: Update the ProviderAdapterCapabilities example in the
provider_onboarding.md docs to include the new graduated CapabilityLevel fields
(CapabilityLevel = "none" | "basic" | "full") so the contract tests can gate
correctly; specifically add resume, subagents, attachments, replay, and
mcpConfig as typed CapabilityLevel entries alongside the existing
sessionModelSwitch, supportsUserInput, supportsRollback, and
supportsFileChangeApproval (the contract test suite referenced is
apps/server/integration/contract.integration.test.ts). Ensure you mention
CapabilityLevel and show the new fields in the example object so implementers
know to provide those values.
In `@apps/harness/lib/harness/metrics.ex`:
- Around line 111-117: safe_cast currently always returns :ok even when the
GenServer isn't running; change it to detect Process.whereis(__MODULE__) and
return {:ok, :sent} after calling GenServer.cast(__MODULE__, {msg, payload}) and
{:ok, :dropped} when the process is missing (or propagate GenServer.cast result
if you prefer), and update any callers that rely on the return value to handle
the new {:ok, :sent} / {:ok, :dropped} results; keep the core logic in safe_cast
(referencing safe_cast, Process.whereis/1, GenServer.cast/2 and __MODULE__) and
ensure tests/checks cover both running and not-running cases.
In `@apps/harness/lib/harness/providers/codex_session.ex`:
- Line 96: The :mcp_config field is being set from params but never used; add a
concise inline comment next to the :mcp_config occurrence(s) (the struct/param
list entries where :mcp_config appears and the other occurrence around lines
referenced) indicating it is intentionally reserved for future MCP support so
reviewers know it is not an accidental leftover; update both occurrences (the
one at :mcp_config and the related entries at the other occurrence) with the
same brief comment.
In `@apps/harness/lib/harness/providers/cursor_session.ex`:
- Line 29: The :mcp_config struct field is set in init/1 but never used; either
wire it into the CLI invocation or mark it as intentional scaffolding: if MCP
configuration should be passed to the Cursor process, update build_cursor_args/3
and/or spawn_cursor_process/3 to read the struct's :mcp_config and append the
appropriate CLI flags or environment variables (use the same flag names the
Cursor CLI expects), otherwise remove :mcp_config from the struct and init/1 or
add a one-line comment near the :mcp_config field referencing
McpConfigServiceLive to indicate it is a deliberate placeholder for future MCP
wiring.
In `@apps/server/integration/contract.integration.test.ts`:
- Around line 48-54: Replace the hardcoded capabilities object used for
describe.skipIf with the actual adapter-declared capabilities: remove or stop
using the local const capabilities and instead import or read the capabilities
from the test adapter (e.g., adapter.capabilities or await
testAdapter.getCapabilities() during setup) and pass that into describe.skipIf
so skip conditions reflect the adapter's real abilities; update any references
to the local capabilities symbol to use the adapter-derived capabilities.
In `@apps/server/src/provider/Layers/HarnessClientAdapter.ts`:
- Around line 1347-1361: The code constructs mcp_config even when config.servers
exists but no server is enabled, producing an empty servers array; update the
logic in HarnessClientAdapter (the block building mcp_config) to compute
enabledServers = config.servers?.filter(s => s.enabled) and only return the
mcp_config object when enabledServers.length > 0 (otherwise return null), and
use enabledServers.map(...) when building the servers list so an all-disabled
list yields null instead of { mcp_config: { ..., servers: [] } }.
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 80-93: Replace the raw console output in emitMetric with Effect's
structured logging: change the implementation of emitMetric to call
Effect.logInfo (or the project's equivalent) with a clear "metric" tag and the
same payload ({ _t: "metric", name, ts: Date.now(), ...attributes }) so metrics
are routed through Effect's logging pipeline and can be filtered by level/tag;
update any imports or wrappers used by ProviderService to use Effect.logInfo and
ensure emitMetric remains synchronous in signature or adjust callers if log
calls become asynchronous.
In `@apps/server/src/serverLayers.ts`:
- Around line 81-86: Currently the T3CODE_CODEX_LEGACY flag is read once into
the top-level const useLegacyCodex, which freezes its value for the whole
module; change this by moving the environment read into the factory so the flag
is evaluated at layer construction time: remove the top-level useLegacyCodex
reference and instead read process.env.T3CODE_CODEX_LEGACY === "1" inside
makeServerProviderLayer() (or inside ServerConfig if you prefer), then use that
local value to choose the Codex path so tests and later calls can observe env
changes.
🪄 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: b2ee4f88-9743-4621-8f74-60ff5e132ca4
📒 Files selected for processing (33)
ai_docs/failure_matrix.mdai_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/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/orchestration/Layers/ProviderRuntimeIngestion.test.tsapps/server/src/provider/Errors.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/HarnessClientAdapter.tsapps/server/src/provider/Layers/McpConfig.tsapps/server/src/provider/Layers/ProviderAdapterRegistry.test.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/Services/McpConfig.tsapps/server/src/provider/Services/ProviderAdapter.tsapps/server/src/serverLayers.tsapps/server/src/wsServer.test.tspackages/contracts/src/provider.ts
Comment on lines
+9
to
+19
| ``` | ||
| Transport (WebSocket/RPC) | ||
| | | ||
| ProviderService (cross-provider facade) | ||
| | | ||
| ProviderAdapterRegistry (adapter lookup) | ||
| | | ||
| ProviderAdapter (provider-specific runtime) | ||
| | | ||
| Provider CLI/SDK (codex, claude, cursor, opencode, ...) | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The ASCII diagram code block is missing a language specifier, triggering markdownlint MD040. Use text or plaintext for compatibility.
📝 Proposed fix
-```
+```text
Transport (WebSocket/RPC)
|
ProviderService (cross-provider facade)📝 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.
Suggested change
| ``` | |
| Transport (WebSocket/RPC) | |
| | | |
| ProviderService (cross-provider facade) | |
| | | |
| ProviderAdapterRegistry (adapter lookup) | |
| | | |
| ProviderAdapter (provider-specific runtime) | |
| | | |
| Provider CLI/SDK (codex, claude, cursor, opencode, ...) | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 ASCII diagram
block (the code fence containing the Transport/ProviderService/ProviderAdapter
diagram) lacks a language specifier; update that code fence to use a neutral
specifier such as text or plaintext (e.g., change ``` to ```text) so
markdownlint MD040 is satisfied, leaving the diagram contents unchanged; locate
the ASCII diagram block in the Provider onboarding documentation and add the
specifier to the opening fence.
Comment on lines
+28
to
+95
| export function classifyProviderError( | ||
| error: | ||
| | ProviderAdapterError | ||
| | ProviderValidationError | ||
| | ProviderUnsupportedError | ||
| | ProviderSessionNotFoundError | ||
| | ProviderSessionDirectoryPersistenceError, | ||
| ): ProviderErrorCategory { | ||
| const tag = (error as { readonly _tag: string })._tag; | ||
|
|
||
| switch (tag) { | ||
| case "ProviderAdapterRequestError": { | ||
| // Request errors are generally transient (timeout, network) unless the | ||
| // detail indicates a permanent issue. | ||
| const detail = ((error as ProviderAdapterRequestError).detail ?? "").toLowerCase(); | ||
| if ( | ||
| detail.includes("timeout") || | ||
| detail.includes("rate limit") || | ||
| detail.includes("econnreset") || | ||
| detail.includes("econnrefused") || | ||
| detail.includes("socket hang up") | ||
| ) { | ||
| return "transient"; | ||
| } | ||
| if ( | ||
| detail.includes("not found") || | ||
| detail.includes("unauthorized") || | ||
| detail.includes("forbidden") | ||
| ) { | ||
| return "permanent"; | ||
| } | ||
| // Default request errors to transient — safer to retry. | ||
| return "transient"; | ||
| } | ||
|
|
||
| case "ProviderAdapterProcessError": { | ||
| const detail = ((error as ProviderAdapterProcessError).detail ?? "").toLowerCase(); | ||
| if ( | ||
| detail.includes("not found") || | ||
| detail.includes("enoent") || | ||
| detail.includes("permission denied") | ||
| ) { | ||
| return "configuration"; | ||
| } | ||
| if (detail.includes("crashed") || detail.includes("signal")) { | ||
| return "unavailable"; | ||
| } | ||
| return "transient"; | ||
| } | ||
|
|
||
| case "ProviderAdapterValidationError": | ||
| case "ProviderAdapterSessionNotFoundError": | ||
| case "ProviderAdapterSessionClosedError": | ||
| case "ProviderValidationError": | ||
| case "ProviderSessionNotFoundError": | ||
| return "permanent"; | ||
|
|
||
| case "ProviderUnsupportedError": | ||
| return "configuration"; | ||
|
|
||
| case "ProviderSessionDirectoryPersistenceError": | ||
| return "transient"; | ||
|
|
||
| default: | ||
| // Fallback — treat unknown errors as transient to allow retry. | ||
| return "transient"; | ||
| } | ||
| } |
There was a problem hiding this comment.
classifyProviderError does not handle McpConfigError.
The classifyProviderError function's parameter union (Lines 29-34) excludes McpConfigError, and there's no case for "McpConfigError" in the switch statement. Per the failure matrix documentation (ai_docs/failure_matrix.md), MCP configuration errors should map to the "configuration" category with recovery strategy "Re-resolve config, prompt user to fix".
🐛 Proposed fix to include McpConfigError
export function classifyProviderError(
error:
| ProviderAdapterError
| ProviderValidationError
| ProviderUnsupportedError
| ProviderSessionNotFoundError
- | ProviderSessionDirectoryPersistenceError,
+ | ProviderSessionDirectoryPersistenceError
+ | McpConfigError,
): ProviderErrorCategory {
const tag = (error as { readonly _tag: string })._tag;
switch (tag) {
// ... existing cases ...
+ case "McpConfigError":
+ return "configuration";
+
case "ProviderSessionDirectoryPersistenceError":
return "transient";
default:🤖 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 28 - 95, The
classifyProviderError signature and switch need to include McpConfigError: add
McpConfigError to the union type parameter of classifyProviderError and add a
switch case for the tag "McpConfigError" that returns "configuration" (mirror
how ProviderUnsupportedError maps to configuration); ensure any type
import/definition for McpConfigError is referenced so the compiler recognizes it
and update the switch default behavior unchanged.
Comment on lines
+1610
to
+1614
| resume: "full", | ||
| subagents: "none", | ||
| attachments: "basic", | ||
| replay: "full", | ||
| mcpConfig: "none", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how other adapters handle mcpConfig capability vs translateMcpConfig behavior
echo "=== ClaudeAdapter mcpConfig capability and translateMcpConfig ==="
rg -n "mcpConfig:" apps/server/src/provider/Layers/ClaudeAdapter.ts -A1
rg -n "translateMcpConfig" apps/server/src/provider/Layers/ClaudeAdapter.ts -A5
echo ""
echo "=== HarnessClientAdapter mcpConfig capability ==="
rg -n "mcpConfig:" apps/server/src/provider/Layers/HarnessClientAdapter.ts -A1Repository: Ranvier-Technologies/t3code-OTP
Length of output: 593
🏁 Script executed:
rg -n "translateMcpConfig" apps/server/src/provider/Layers/CodexAdapter.ts -A15Repository: Ranvier-Technologies/t3code-OTP
Length of output: 677
🏁 Script executed:
# Check if CodexAdapter is expected to support MCP or if it's intentionally disabled
# Look for any comments or context around mcpConfig in CodexAdapter
rg -n "mcpConfig" apps/server/src/provider/Layers/CodexAdapter.ts -B3 -A3
# Also check if there's a pattern in how adapters handle translateMcpConfig when mcpConfig is "none"
echo "=== Checking other adapters for translateMcpConfig with mcpConfig: none ==="
rg -n "mcpConfig:.*none" apps/server/src/provider/Layers/ -A15 | head -60Repository: Ranvier-Technologies/t3code-OTP
Length of output: 5565
Inconsistency between mcpConfig: "none" capability and translateMcpConfig behavior.
The adapter declares mcpConfig: "none" (Line 1614), implying no MCP configuration support. However, translateMcpConfig (Lines 1627-1641) returns a non-null object when enabled servers exist. This contradicts the established pattern in other adapters (e.g., ClaudeAdapter) where mcpConfig: "none" consistently pairs with translateMcpConfig returning null.
Resolve by either:
- Updating the capability to
"basic"to match the translator's behavior, or - Modifying
translateMcpConfigto returnnullunconditionally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/CodexAdapter.ts` around lines 1610 - 1614,
The CodexAdapter declares mcpConfig: "none" but its translateMcpConfig function
(translateMcpConfig) can return a non-null MCP config when enabled servers
exist, creating an inconsistency; either change the capability value to "basic"
to reflect that translateMcpConfig may return a config, or change
translateMcpConfig to always return null (matching other adapters) — update the
mcpConfig property in the adapter's capability object or alter
translateMcpConfig to unconditionally return null so both the capability and
translator behavior are consistent.
Comment on lines
+74
to
+104
| function validateResumeCursor( | ||
| cursor: unknown, | ||
| ): | ||
| | { readonly valid: true; readonly cursor: unknown } | ||
| | { readonly valid: false; readonly reason: string } { | ||
| if (cursor === null || cursor === undefined) { | ||
| return { valid: false, reason: "cursor is null or undefined" }; | ||
| } | ||
| if (typeof cursor === "string") { | ||
| const trimmed = cursor.trim(); | ||
| if (trimmed.length === 0) { | ||
| return { valid: false, reason: "cursor is an empty string" }; | ||
| } | ||
| try { | ||
| const parsed = JSON.parse(trimmed); | ||
| if (parsed === null || parsed === undefined) { | ||
| return { valid: false, reason: "cursor JSON parses to null/undefined" }; | ||
| } | ||
| return { valid: true, cursor: parsed }; | ||
| } catch (err) { | ||
| return { | ||
| valid: false, | ||
| reason: `cursor string is not valid JSON: ${err instanceof Error ? err.message : String(err)}`, | ||
| }; | ||
| } | ||
| } | ||
| if (typeof cursor === "object") { | ||
| return { valid: true, cursor }; | ||
| } | ||
| return { valid: false, reason: `unexpected cursor type: ${typeof cursor}` }; | ||
| } |
There was a problem hiding this comment.
Test helper diverges from production implementation.
The local validateResumeCursor helper has subtle differences from the production code in ProviderService.ts:
- Line 85: Returns
"cursor is an empty string"vs production's"cursor string is empty after trimming" - Line 89: Checks
parsed === null || parsed === undefinedvs production'sparsed === nullonly - Lines 100-103: Adds an explicit
typeof cursor === "object"branch and handles unexpected types, which production code doesn't have (production accepts any non-string as already-deserialized)
These inconsistencies mean tests may pass but not accurately validate production behavior.
🔧 Suggested fix to align with production
function validateResumeCursor(
cursor: unknown,
):
| { readonly valid: true; readonly cursor: unknown }
| { readonly valid: false; readonly reason: string } {
if (cursor === null || cursor === undefined) {
return { valid: false, reason: "cursor is null or undefined" };
}
if (typeof cursor === "string") {
const trimmed = cursor.trim();
if (trimmed.length === 0) {
- return { valid: false, reason: "cursor is an empty string" };
+ return { valid: false, reason: "cursor string is empty after trimming" };
}
try {
const parsed = JSON.parse(trimmed);
- if (parsed === null || parsed === undefined) {
- return { valid: false, reason: "cursor JSON parses to null/undefined" };
+ if (parsed === null) {
+ return { valid: false, reason: "cursor JSON parses to null" };
}
return { valid: true, cursor: parsed };
} catch (err) {
return {
valid: false,
reason: `cursor string is not valid JSON: ${err instanceof Error ? err.message : String(err)}`,
};
}
}
- if (typeof cursor === "object") {
- return { valid: true, cursor };
- }
- return { valid: false, reason: `unexpected cursor type: ${typeof cursor}` };
+ // Assume it's an object that was already deserialized by persistence layer
+ return { valid: true, cursor };
}📝 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.
Suggested change
| function validateResumeCursor( | |
| cursor: unknown, | |
| ): | |
| | { readonly valid: true; readonly cursor: unknown } | |
| | { readonly valid: false; readonly reason: string } { | |
| if (cursor === null || cursor === undefined) { | |
| return { valid: false, reason: "cursor is null or undefined" }; | |
| } | |
| if (typeof cursor === "string") { | |
| const trimmed = cursor.trim(); | |
| if (trimmed.length === 0) { | |
| return { valid: false, reason: "cursor is an empty string" }; | |
| } | |
| try { | |
| const parsed = JSON.parse(trimmed); | |
| if (parsed === null || parsed === undefined) { | |
| return { valid: false, reason: "cursor JSON parses to null/undefined" }; | |
| } | |
| return { valid: true, cursor: parsed }; | |
| } catch (err) { | |
| return { | |
| valid: false, | |
| reason: `cursor string is not valid JSON: ${err instanceof Error ? err.message : String(err)}`, | |
| }; | |
| } | |
| } | |
| if (typeof cursor === "object") { | |
| return { valid: true, cursor }; | |
| } | |
| return { valid: false, reason: `unexpected cursor type: ${typeof cursor}` }; | |
| } | |
| function validateResumeCursor( | |
| cursor: unknown, | |
| ): | |
| | { readonly valid: true; readonly cursor: unknown } | |
| | { readonly valid: false; readonly reason: string } { | |
| if (cursor === null || cursor === undefined) { | |
| return { valid: false, reason: "cursor is null or undefined" }; | |
| } | |
| if (typeof cursor === "string") { | |
| const trimmed = cursor.trim(); | |
| if (trimmed.length === 0) { | |
| return { valid: false, reason: "cursor string is empty after trimming" }; | |
| } | |
| try { | |
| const parsed = JSON.parse(trimmed); | |
| if (parsed === null) { | |
| return { valid: false, reason: "cursor JSON parses to null" }; | |
| } | |
| return { valid: true, cursor: parsed }; | |
| } catch (err) { | |
| return { | |
| valid: false, | |
| reason: `cursor string is not valid JSON: ${err instanceof Error ? err.message : String(err)}`, | |
| }; | |
| } | |
| } | |
| // Assume it's an object that was already deserialized by persistence layer | |
| return { valid: true, cursor }; | |
| } |
🤖 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` around lines 74
- 104, The test helper validateResumeCursor diverges from production; update it
to exactly match ProviderService.ts by (1) changing the empty-string reason to
"cursor string is empty after trimming", (2) only treating parsed === null as
invalid (remove the parsed === undefined check), and (3) remove the explicit
typeof cursor === "object" branch so non-string inputs are treated as
already-deserialized like production (i.e., return valid with the original
cursor for any non-string path); keep the JSON parsing and error-message
behavior identical to production.
Comment on lines
+156
to
+162
| it("rejects JSON string that parses to null", () => { | ||
| const result = validateResumeCursor("null"); | ||
| expect(result.valid).toBe(false); | ||
| if (!result.valid) { | ||
| expect(result.reason).toContain("parses to null/undefined"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Test assertion needs update to match production reason string.
If the helper is updated per the previous comment, update this test's assertion accordingly.
it("rejects JSON string that parses to null", () => {
const result = validateResumeCursor("null");
expect(result.valid).toBe(false);
if (!result.valid) {
- expect(result.reason).toContain("parses to null/undefined");
+ expect(result.reason).toContain("parses to null");
}
});📝 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.
Suggested change
| it("rejects JSON string that parses to null", () => { | |
| const result = validateResumeCursor("null"); | |
| expect(result.valid).toBe(false); | |
| if (!result.valid) { | |
| expect(result.reason).toContain("parses to null/undefined"); | |
| } | |
| }); | |
| it("rejects JSON string that parses to null", () => { | |
| const result = validateResumeCursor("null"); | |
| expect(result.valid).toBe(false); | |
| if (!result.valid) { | |
| expect(result.reason).toContain("parses to null"); | |
| } | |
| }); |
🤖 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` around lines 156
- 162, Update the test so the assertion matches the actual reason string
returned by validateResumeCursor; locate the test using validateResumeCursor in
codexHarnessCutover.test.ts and replace the
expect(result.reason).toContain("parses to null/undefined") check with the exact
substring used by the validateResumeCursor implementation (e.g., the production
message it sets when parsing yields null/undefined).
Comment on lines
+107
to
+123
| /** | ||
| * Effect that classifies a provider error and logs the category for observability. | ||
| * | ||
| * Designed for use in `Effect.tapError` pipelines. | ||
| */ | ||
| function logClassifiedError( | ||
| operation: string, | ||
| error: Parameters<typeof classifyProviderError>[0], | ||
| ): Effect.Effect<void> { | ||
| const category: ProviderErrorCategory = classifyProviderError(error); | ||
| return Effect.logWarning("provider error classified", { | ||
| operation, | ||
| errorTag: (error as { readonly _tag?: string })._tag ?? "unknown", | ||
| category, | ||
| message: error.message, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify logClassifiedError is not used
rg -n "logClassifiedError" apps/server/src/provider/Layers/ProviderService.tsRepository: Ranvier-Technologies/t3code-OTP
Length of output: 107
🏁 Script executed:
# Search entire codebase for logClassifiedError usage
rg "logClassifiedError" --no-headingRepository: Ranvier-Technologies/t3code-OTP
Length of output: 154
Remove logClassifiedError function or add a TODO comment if intended for future use.
The helper function is defined but never called anywhere in the codebase. Either remove it to eliminate dead code, or add a TODO comment explaining the intended future use.
🤖 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 107 - 123,
The function logClassifiedError is defined but unused; either delete this dead
helper or retain it with a clear TODO explaining why it’s kept and when it will
be used. If removing, delete the entire logClassifiedError function (which
references classifyProviderError, ProviderErrorCategory, and Effect.logWarning)
and run tests/lint to ensure no references remain. If keeping, add a one-line
TODO comment above logClassifiedError stating its intended future use (e.g., for
Effect.tapError pipelines) and why it must remain, ensuring the comment includes
the function name for discoverability.
Comment on lines
+95
to
102
| 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.
adapterKey migration is currently non-functional for adapter resolution.
getBinding now returns a migrated adapterKey, but session recovery resolves adapters by binding.provider (not binding.adapterKey), so this migration does not affect the actual dispatch path and won’t eliminate adapter-path ambiguity during recovery.
🤖 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 95
- 102, The migration of adapterKey via migrateAdapterKey(...) is being stored
but not used for resolving adapters during recovery; update the
recovery/path-resolution logic (used by getBinding and wherever binding.provider
is read) to resolve adapters using the migrated adapterKey (binding.adapterKey)
instead of relying solely on binding.provider, or alternatively update
getBinding to replace binding.provider with the provider derived from the
migrated adapterKey; change references in ProviderSessionDirectory/getBinding
and any session recovery code to consult the migrated adapterKey and derive the
correct provider before dispatch so adapter-path ambiguity is eliminated.
Comment on lines
+180
to
+279
| // Determine the adapter registry layer based on configuration. | ||
| // | ||
| // Three paths: | ||
| // A) harnessPort configured + codex via harness (default) | ||
| // B) legacy codex (T3CODE_CODEX_LEGACY=1) — with or without harness | ||
| // C) no harness port + harness required (error gracefully) | ||
| const adapterRegistryLayer = harnessPortConfigured | ||
| ? codexViaHarness | ||
| ? // Path A: harness for all (codex, cursor, opencode) | ||
| Layer.effect( | ||
| ProviderAdapterRegistry, | ||
| Effect.gen(function* () { | ||
| const claudeAdapter = yield* ClaudeAdapter; | ||
| const harnessBaseAdapter = yield* HarnessClientAdapter; | ||
| return makeRegistryFromMap(makeHarnessProviderMap(harnessBaseAdapter, claudeAdapter)); | ||
| }), | ||
| ).pipe( | ||
| Layer.provide(claudeAdapterLayer), | ||
| Layer.provideMerge(harnessAdapterLayer), | ||
| Layer.provideMerge(providerSessionDirectoryLayer), | ||
| ) | ||
| : // Path B-1: legacy codex + harness for cursor/opencode | ||
| Layer.effect( | ||
| ProviderAdapterRegistry, | ||
| Effect.gen(function* () { | ||
| const claudeAdapter = yield* ClaudeAdapter; | ||
| const codexAdapter = yield* CodexAdapter; | ||
| const harnessBaseAdapter = yield* HarnessClientAdapter; | ||
|
|
||
| type Adapter = ProviderAdapterShape<ProviderAdapterError>; | ||
| const byProvider = new Map<string, Adapter>(); | ||
| byProvider.set("claudeAgent", claudeAdapter); | ||
| byProvider.set("codex", codexAdapter); | ||
| for (const providerKind of HARNESS_ONLY_PROVIDERS) { | ||
| byProvider.set(providerKind, { | ||
| ...harnessBaseAdapter, | ||
| provider: providerKind, | ||
| capabilities: | ||
| HARNESS_PROVIDER_CAPABILITIES[providerKind] ?? harnessBaseAdapter.capabilities, | ||
| } as Adapter); | ||
| } | ||
| return makeRegistryFromMap(byProvider); | ||
| }), | ||
| ).pipe( | ||
| Layer.provide(claudeAdapterLayer), | ||
| Layer.provide(codexAdapterLayer), | ||
| Layer.provideMerge(harnessAdapterLayer), | ||
| Layer.provideMerge(providerSessionDirectoryLayer), | ||
| ) | ||
| : useLegacyCodex | ||
| ? // Path B-2: legacy codex, no harness — codex + claude only | ||
| ProviderAdapterRegistryLive.pipe( | ||
| Layer.provide(codexAdapterLayer), | ||
| Layer.provide(claudeAdapterLayer), | ||
| Layer.provideMerge(providerSessionDirectoryLayer), | ||
| ) | ||
| : // Path C: harness required but not configured — error gracefully | ||
| Layer.effect( | ||
| ProviderAdapterRegistry, | ||
| Effect.gen(function* () { | ||
| yield* Effect.logError( | ||
| "[codex-harness-cutover] Harness port is not configured but Codex requires " + | ||
| "the harness (default path). Set T3CODE_CODEX_LEGACY=1 to use the legacy " + | ||
| "direct adapter, or configure harnessPort.", | ||
| ); | ||
| const claudeAdapter = yield* ClaudeAdapter; | ||
| type Adapter = ProviderAdapterShape<ProviderAdapterError>; | ||
| const byProvider = new Map<string, Adapter>(); | ||
| byProvider.set("claudeAgent", claudeAdapter); | ||
|
|
||
| return { | ||
| getByProvider: (provider: string) => { | ||
| const adapter = byProvider.get(provider); | ||
| if (!adapter) { | ||
| return Effect.fail( | ||
| new ProviderUnsupportedError({ | ||
| provider, | ||
| ...(provider === "codex" || provider === "cursor" || provider === "opencode" | ||
| ? { | ||
| cause: new Error( | ||
| `Harness port is not configured. Codex requires the Elixir harness. ` + | ||
| `Set T3CODE_CODEX_LEGACY=1 to use the legacy direct adapter, or configure harnessPort.`, | ||
| ), | ||
| } | ||
| : {}), | ||
| }), | ||
| ); | ||
| } | ||
| return Effect.succeed(adapter); | ||
| }, | ||
| listProviders: () => | ||
| Effect.sync( | ||
| () => Array.from(byProvider.keys()) as unknown as readonly ProviderKind[], | ||
| ), | ||
| }; | ||
| }), | ||
| ).pipe( | ||
| Layer.provide(claudeAdapterLayer), | ||
| Layer.provideMerge(providerSessionDirectoryLayer), | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep the Codex/harness routing matrix in providerManager.ts.
This branch now encodes provider-selection policy in serverLayers.ts, which splits dispatch behavior across bootstrap wiring and the required coordination point. Please move this branching behind apps/server/src/providerManager.ts and keep this file focused on layer assembly.
As per coding guidelines, "Provider dispatch and thread event logging must be coordinated in apps/server/src/providerManager.ts."
Comment on lines
+255
to
+261
| new ProviderUnsupportedError({ | ||
| provider, | ||
| ...(provider === "codex" || provider === "cursor" || provider === "opencode" | ||
| ? { | ||
| cause: new Error( | ||
| `Harness port is not configured. Codex 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.
Make the unsupported-provider hint provider-specific.
Lines 257-261 tell cursor and opencode callers that “Codex requires the Elixir harness” and suggest T3CODE_CODEX_LEGACY=1, but that flag only helps Codex. Emit provider-specific remediation here so operators do not chase a fix that can never enable those providers.
🩹 Suggested change
- cause: new Error(
- `Harness port is not configured. Codex 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. 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} requires the Elixir harness. Configure harnessPort.`,
+ ),📝 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.
Suggested change
| new ProviderUnsupportedError({ | |
| provider, | |
| ...(provider === "codex" || provider === "cursor" || provider === "opencode" | |
| ? { | |
| cause: new Error( | |
| `Harness port is not configured. Codex requires the Elixir harness. ` + | |
| `Set T3CODE_CODEX_LEGACY=1 to use the legacy direct adapter, or configure harnessPort.`, | |
| new ProviderUnsupportedError({ | |
| provider, | |
| ...(provider === "codex" || provider === "cursor" || provider === "opencode" | |
| ? { | |
| cause: new Error( | |
| provider === "codex" | |
| ? `Harness port is not configured. 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} 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 255 - 261, The current
ProviderUnsupportedError construction uses a hardcoded Codex-specific hint for
multiple providers; update the cause/message generation in the
ProviderUnsupportedError block (where ProviderUnsupportedError is instantiated
using the provider variable) to produce provider-specific remediation text: if
provider === "codex" include the Elixir harness hint and T3CODE_CODEX_LEGACY=1
suggestion, and for provider === "cursor" or provider === "opencode" emit a
different, accurate hint (or a generic “configure harnessPort or use supported
adapter” message) so callers for cursor/opencode are not directed to Codex-only
fixes; implement this by branching on provider or mapping provider->hint before
passing the cause to ProviderUnsupportedError.
- 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>
Comment on lines
+27
to
+36
| function migrateAdapterKey( | ||
| adapterKey: string, | ||
| threadId: string, | ||
| ): { readonly key: string; readonly migrated: boolean } { | ||
| const mapped = LEGACY_ADAPTER_KEY_MAP[adapterKey]; | ||
| if (mapped !== undefined) { | ||
| return { key: mapped, migrated: true }; | ||
| } | ||
| return { key: adapterKey, migrated: false }; | ||
| } |
There was a problem hiding this comment.
🔴 Duplicate logic in test file violates AGENTS.md maintainability rule
The codexHarnessCutover.test.ts file re-implements both migrateAdapterKey (lines 27-36, mirroring apps/server/src/provider/Layers/ProviderSessionDirectory.ts:33-48) and validateResumeCursor (lines 74-104, mirroring apps/server/src/provider/Layers/ProviderService.ts:214-246). The AGENTS.md explicitly states: "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 proper fix is to extract these functions as named exports from their respective modules and import them in the test file.
Prompt for agents
Extract the migrateAdapterKey function from apps/server/src/provider/Layers/ProviderSessionDirectory.ts (lines 33-48) and the validateResumeCursor function from apps/server/src/provider/Layers/ProviderService.ts (lines 214-246) as named exports. Then, in apps/server/src/provider/Layers/codexHarnessCutover.test.ts, replace the local re-implementations (lines 22-36 for migrateAdapterKey and lines 74-104 for validateResumeCursor) with imports from those modules. This eliminates the duplicate logic and ensures the tests validate the actual production code rather than a copy.
Was this helpful? React with 👍 or 👎 to provide feedback.
| {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.
🟡 Harness.Metrics placed before critical services under rest_for_one supervisor causes cascading restarts
In the supervision tree, Harness.Metrics is inserted at position 5 (before Harness.SnapshotServer at 6 and HarnessWeb.Endpoint at 7) under a :rest_for_one strategy. If Harness.Metrics crashes, the supervisor terminates and restarts all children started after it — including the SnapshotServer and the WebSocket Endpoint — causing a service interruption for all connected clients. The Metrics module is a non-critical counter server designed to be crash-tolerant (its safe_cast/2 already handles the case where the GenServer is down). Placing it before critical services contradicts the root AGENTS.md priority: "Reliability first" and "Keep behavior predictable under load and during failures." The fix is to either move Harness.Metrics to the end of the children list, or supervise it under a separate supervisor so its failure doesn't cascade.
Suggested change
| Harness.Metrics, | |
| Harness.SnapshotServer, |
Was this helpful? React with 👍 or 👎 to provide feedback.
devin-ai-integration Bot
added a commit
that referenced
this pull request
Mar 28, 2026
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>
9 tasks
This was referenced Mar 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates the provider architecture across 4 phases (7 commits), eliminating Codex path ambiguity and establishing foundational contracts for multi-provider support.
HarnessClientAdapterthe default path for Codex sessions. DirectCodexAdapterpreserved behindT3CODE_CODEX_LEGACY=1feature flag for rollback safety. Includes adapter_key migration (read-time) and resume_cursor validation.ProviderAdapterCapabilitieswith graduatedCapabilityLevelfields (resume,subagents,attachments,replay,mcpConfig). AddsclassifyProviderError()taxonomy and structured metric emission (session.start/end/resume).McpConfigService(resolve + snapshot) with per-adapter translators. MCP policy defined once server-side; adapters only translate. Elixir sessions accept and storemcp_configin state."none"). Elixir@behaviour ProviderBehaviourwith 8 callbacks +@implannotations on all 4 session modules. Provider onboarding playbook.Commits (chronological)
feat: Codex harness-only cutover with T3CODE_CODEX_LEGACY flagfeat: expand capability model with graduated CapabilityLevel fieldsfeat: error taxonomy with category classification on provider errorsfeat: telemetry baseline with structured metrics for session lifecyclefeat: McpConfigService with reference + snapshot persistence modelfeat: adapter MCP translators with per-provider config translationfeat: contract test suite + provider behaviour + onboarding playbookKey artifacts
ai_docs/failure_matrix.mdai_docs/provider_onboarding.mdapps/harness/lib/harness/providers/provider_behaviour.exapps/server/integration/contract.integration.test.tsapps/server/src/provider/Services/McpConfig.tsTest plan
bun run test— 666 tests passing (54 files), 0 type errors, 0 lint errorsmix test— 88 tests passing (run in worktree, excluded from final phase due to port conflicts)T3CODE_CODEX_LEGACY=1→ verify routes through direct path🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Deprecations