Add provider capability model and MCP runtime support - #28
Merged
Conversation
📝 WalkthroughWalkthroughAdds a formal ProviderSession behaviour and lifecycle metrics to the harness; introduces MCP (Model Context Protocol) config types, a McpConfigService with snapshot persistence and translation to provider artifacts (Codex TOML, OpenCode JSON); enriches provider capabilities, wires MCP into adapters/provider service, and adds related tests and telemetry. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProviderService as ProviderService
participant McpCfg as McpConfigService
participant FS as FileSystem
participant Adapter as ProviderAdapter
participant Manager as SessionManager
Client->>ProviderService: startSession(threadId, provider, cwd)
ProviderService->>McpCfg: resolveConfig(provider, cwd, threadId)
McpCfg->>FS: read global/cwd config files
FS-->>McpCfg: config contents
McpCfg->>McpCfg: normalize & build ResolvedMcpConfig
McpCfg->>FS: persist snapshot -> mcp/snapshots/<threadId>.json
McpCfg-->>ProviderService: ResolvedMcpConfig
ProviderService->>Adapter: startSession(threadId, generatedPaths?)
alt MCP servers present
Adapter->>FS: mkdir and write generated artifacts (config.toml / opencode.json)
Adapter->>Manager: manager.startSession(generatedPath)
else
Adapter->>Manager: manager.startSession(basePath)
end
Manager-->>Adapter: sessionHandle
Adapter-->>ProviderService: sessionRuntime (mcpConfigRef included)
ProviderService-->>Client: sessionStarted + metadata
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/harness/lib/harness/metrics.ex (1)
13-20:⚠️ Potential issue | 🟡 MinorAvoid collecting session data twice in a single metrics snapshot.
collect/0andlifecycle_metrics/0each callsession_metrics/0. Under session churn,sessionsandlifecyclecan disagree in the same payload, and you also pay the collection cost twice.♻️ Suggested patch
def collect do + sessions = session_metrics() + %{ beam: beam_metrics(), - sessions: session_metrics(), - lifecycle: lifecycle_metrics(), + sessions: sessions, + lifecycle: lifecycle_metrics(sessions), snapshot_server: snapshot_server_metrics(), timestamp: System.system_time(:millisecond) } end -defp lifecycle_metrics do - sessions = session_metrics() - +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)) } endAlso applies to: 89-97
🤖 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 13 - 20, collect/0 currently calls session_metrics/0 and also calls lifecycle_metrics/0 which re-calls session_metrics/0, causing duplicate collection; change collect/0 to compute sessions = session_metrics() once and pass it into a new lifecycle_metrics/1 (or update existing lifecycle_metrics to accept a sessions argument) so lifecycle_metrics no longer calls session_metrics/0 internally; update any other callers of lifecycle_metrics/0 (e.g., places flagged around lines 89-97) to either call lifecycle_metrics(precomputed_sessions) or fall back to session_metrics() only when a sessions arg is not provided.
🧹 Nitpick comments (7)
apps/server/src/provider/Layers/HarnessProvider.ts (1)
25-34: Drop redundant provider casts after narrowing the function parameter.Now that
provideris already"cursor" | "opencode", the type assertions can be removed for cleaner typing.♻️ Suggested cleanup
- (settings) => - settings.providers[provider as "cursor" | "opencode"] as HarnessProviderSettings, + (settings) => settings.providers[provider] as HarnessProviderSettings, @@ - (settings) => - settings.providers[provider as "cursor" | "opencode"] as HarnessProviderSettings, + (settings) => settings.providers[provider] as HarnessProviderSettings,Also applies to: 92-93
🤖 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` around lines 25 - 34, In makeHarnessProviderLayer, remove the redundant type assertions that re-cast the already-narrowed provider parameter and settings entry: update the getProviderSettings mapping to directly index settings.providers[provider] and treat it as HarnessProviderSettings (or remove the unnecessary "cursor" | "opencode" cast), and likewise remove the duplicate casts present near the other usage (the second occurrence around the getProviderSettings consumer). Touch the symbols: makeHarnessProviderLayer, provider, getProviderSettings, ServerSettingsService, and HarnessProviderSettings to eliminate the unnecessary "as" assertions so TypeScript relies on the function parameter's narrowed union type.apps/server/src/provider/Layers/ProviderRegistry.ts (1)
111-131: Consider extracting a small capability-wrapping helper.
getSnapshot,refresh, andstreamChangeseach repeat the same codex capability override mapping. A tiny helper would reduce drift risk if the wrapper logic changes later.🤖 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 111 - 131, The three mappings in codexProvider repeat identical logic; extract a small helper (e.g., wrapWithCodexCapabilities or applyCodexCaps) that takes a stream/effect (used on CodexProvider.getSnapshot, .refresh, .streamChanges) and returns the mapped version that merges HARNESS_PROVIDER_CAPABILITIES.codex into the provider; replace the three inline Effect.map/Stream.map blocks with calls to this helper applied to codexProviderBase.getSnapshot, codexProviderBase.refresh, and codexProviderBase.streamChanges to centralize the capability-wrapping logic.apps/server/src/provider/providerCapabilities.ts (1)
17-19: Cursor harness capabilities spread without modification.The
cursorentry spreadsDEFAULT_PROVIDER_CAPABILITIES.cursorwithout any overrides. This is intentional for consistency, but consider adding a brief comment explaining that cursor's defaults are already suitable for harness mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/providerCapabilities.ts` around lines 17 - 19, The cursor entry currently spreads DEFAULT_PROVIDER_CAPABILITIES.cursor without changes; add a concise inline comment next to the cursor property explaining that the default cursor capabilities are intentionally used for harness mode (e.g., "using default cursor capabilities for harness mode — no overrides required") so future readers understand this is deliberate; update the cursor block where DEFAULT_PROVIDER_CAPABILITIES.cursor is spread to include that comment near the cursor symbol.apps/server/src/serverLayers.ts (1)
117-120: Consider documenting the legacy mode environment variable.The
T3CODE_CODEX_LEGACYenvironment variable controls whether Codex uses the Node SDK adapter directly or routes through the harness. This behavior should be documented for operators and developers.🤖 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 117 - 120, Document the T3CODE_CODEX_LEGACY environment variable and its effects: note that setting T3CODE_CODEX_LEGACY="1" causes useLegacyCodex to be true and changes HARNESS_PROVIDERS from ["codex","cursor","opencode"] to ["cursor","opencode"], meaning Codex will use the Node SDK adapter directly instead of routing through the harness; add its purpose, accepted values, default behavior, and operational impact (including how it affects useLegacyCodex and HARNESS_PROVIDERS) to the project's environment/configuration docs or README so operators and developers can discover and use it correctly.apps/server/src/provider/Layers/CodexAdapter.ts (1)
1393-1429: MCP config materialization uses synchronous filesystem operations.The use of synchronous
fs.mkdirSync,fs.cpSync, andfs.writeFileSyncinsideEffect.tryis acceptable since these are wrapped in an error-handling context. However, the directory copy on line 1411 could be slow for large home directories and block the event loop.Consider using async filesystem operations via
fileSystem(already available in scope) for consistency with the rest of the codebase, or document that this is intentional due to the need for synchronous materialization before session start.🤖 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 1393 - 1429, The materialization block that computes translatedHomePath (inside CodexAdapter where generatedMcpDir is used) currently uses synchronous fs calls (mkdirSync, cpSync, writeFileSync) which can block the event loop; replace these with the asynchronous fileSystem APIs available in scope (use await/promise style inside Effect.try or convert to Effect.async/Effect.attemptPromise) to perform directory creation, recursive copy, and file write (mirroring the current behavior of creating generatedHomePath, copying baseHomePath -> generatedHomePath when present, and writing config.toml from codexTomlFromResolved(resolvedMcp)); preserve the existing ProviderAdapterProcessError construction on failure and keep the returned generatedHomePath value and the surrounding logic (translatedHomePath ?? baseHomePath) unchanged so semantics remain identical.apps/server/src/provider/Layers/McpConfig.ts (1)
182-200: Potential issue:Effect.catchswallows all errors silently.On line 184,
Effect.catch(() => Effect.succeed<string | null>(null))swallows all filesystem read errors without logging. Similarly, JSON parse errors are silently converted tonull. This could make debugging MCP config issues difficult.Consider adding debug logging when config file reads or parses fail:
♻️ Add debug logging for silent failures
const raw = yield* fileSystem .readFileString(targetPath) - .pipe(Effect.catch(() => Effect.succeed<string | null>(null))); + .pipe( + Effect.catchAll((cause) => + Effect.logDebug("failed to read MCP snapshot", { targetPath, cause }).pipe( + Effect.as<string | null>(null), + ), + ), + ); if (raw === null) return null; const parsed = (() => { try { return JSON.parse(raw) as unknown; } catch { return null; } })(); - if (parsed === null || !Schema.is(ResolvedMcpConfig)(parsed)) { + if (parsed === null) { + yield* Effect.logDebug("MCP snapshot JSON parse failed", { targetPath }); + return null; + } + if (!Schema.is(ResolvedMcpConfig)(parsed)) { + yield* Effect.logDebug("MCP snapshot schema validation failed", { targetPath }); return null; }🤖 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 - 200, The code silently swallows filesystem and JSON parse errors when reading MCP config; update the read and parse paths to log failures (including error details, targetPath and threadId) before returning null: change the Effect.catch used with fileSystem.readFileString to capture the caught error and call the existing logger (e.g., processLogger or logger) with a descriptive message and the error, and wrap the JSON.parse catch to log the parse exception and a short preview of raw before returning null; keep behavior of returning null on failure but ensure errors are logged; references: fileSystem.readFileString, Effect.catch, JSON.parse, ResolvedMcpConfig, Schema.is, cacheSnapshot, threadId, targetPath.apps/server/src/provider/Services/McpConfig.ts (1)
35-54: MakelayerTest()snapshot APIs stateful by default.The no-op
setSnapshot/ always-nullgetSnapshotdefaults let tests miss the new snapshot lifecycle entirely. A tiny in-memory map here would keep fixtures terse and still exercise start/recovery/clear behavior.🧪 Back the default test implementation with an in-memory snapshot store
static readonly layerTest = (options?: { readonly resolveConfig?: McpConfigServiceShape["resolveConfig"]; readonly setSnapshot?: McpConfigServiceShape["setSnapshot"]; readonly getSnapshot?: McpConfigServiceShape["getSnapshot"]; readonly clearSnapshot?: McpConfigServiceShape["clearSnapshot"]; - }) => - Layer.succeed(McpConfigService, { + }) => { + const snapshots = new Map<string, ResolvedMcpConfig>(); + return Layer.succeed(McpConfigService, { resolveConfig: options?.resolveConfig ?? (() => Effect.succeed({ version: "empty", resolvedAt: new Date(0).toISOString(), sourcePaths: [], servers: [], })), - setSnapshot: options?.setSnapshot ?? (() => Effect.void), - getSnapshot: options?.getSnapshot ?? (() => Effect.succeed(null)), - clearSnapshot: options?.clearSnapshot ?? (() => Effect.void), + setSnapshot: + options?.setSnapshot ?? + ((threadId, config) => + Effect.sync(() => { + snapshots.set(String(threadId), config); + })), + getSnapshot: + options?.getSnapshot ?? + ((threadId) => Effect.sync(() => snapshots.get(String(threadId)) ?? null)), + clearSnapshot: + options?.clearSnapshot ?? + ((threadId) => + Effect.sync(() => { + snapshots.delete(String(threadId)); + })), } satisfies McpConfigServiceShape); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Services/McpConfig.ts` around lines 35 - 54, The test Layer default is currently no-op for snapshots so tests never exercise snapshot lifecycle; update layerTest to back setSnapshot/getSnapshot/clearSnapshot with a closure-scoped in-memory store: inside the layerTest factory create a local variable (e.g. let currentSnapshot: ReturnType<McpConfigServiceShape["getSnapshot"]> | null = null) and set the defaults to options?.setSnapshot ?? ((snap) => Effect.sync(() => { currentSnapshot = snap })) , options?.getSnapshot ?? (() => Effect.succeed(currentSnapshot)), and options?.clearSnapshot ?? (() => Effect.sync(() => { currentSnapshot = null })); keep the overrides from options and ensure the resulting object still satisfies McpConfigServiceShape.
🤖 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/Errors.ts`:
- Around line 238-315: classifyProviderError currently omits
CheckpointServiceError so checkpoint/store faults fall through as permanent; add
a branch that detects Schema.is(CheckpointServiceError)(error) in
classifyProviderError and return an appropriate recoverable classification
(e.g., category: "transient" or "provider-unavailable" depending on your fault
model, recoveryStrategy: "retry-persist" or "degrade-gracefully", recoverable:
true) so snapshot persistence errors are not marked fail-fast; reference the
Schema.is(CheckpointServiceError) check and ensure it appears before the final
fall-through return (or alternatively explicitly exclude CheckpointServiceError
from using this helper until a mapping exists).
- Around line 303-308: The current
Schema.is(ProviderSessionDirectoryPersistenceError)(error) branch classifies all
persistence errors as configuration issues; change it to distinguish transient
storage/contention errors (e.g. messages or nested codes like "sqlite busy",
"database is locked" or equivalent DB_LOCKED codes) from true configuration
failures: within the ProviderSessionDirectoryPersistenceError handling, inspect
error.message and any nested error.code/innerError to detect these transient
conditions and for them return { category: "transient", recoveryStrategy:
"retry-with-backoff", recoverable: true }, otherwise keep the configuration path
({ category: "configuration", recoveryStrategy: "re-resolve-config",
recoverable: true }). Use the same ProviderSessionDirectoryPersistenceError /
Schema.is(...) spot to implement this branching so retries/backoff are attempted
for storage contention instead of re-resolving config.
In `@apps/server/src/provider/Layers/HarnessClientAdapter.ts`:
- Around line 1106-1127: The generated MCP artifacts created under
generatedMcpDir (variable generatedDir / generatedHomePath) are created with
process-default permissions; tighten them by explicitly setting directory and
file modes: create generatedHomePath with mode 0o700 (use fs.mkdirSync with mode
or chmod after creation), ensure any copied content from baseCodexHomePath is
recursively restricted (after fs.cpSync set chmod 0o700 for directories and
0o600 for files), and write config.toml and opencode.json (the fs.writeFileSync
call and any similar writes around lines ~1135-1145) then set their mode to
0o600 (via fs.chmodSync) so secrets are not world-readable. Ensure these changes
reference the existing symbols generatedMcpDir, generatedHomePath,
baseCodexHomePath, fs.cpSync, fs.mkdirSync, and fs.writeFileSync.
In `@apps/server/src/provider/Layers/ProviderService.ts`:
- Around line 833-845: The explicit-stop block should only emit the
"provider.session.end" metric if this code still owns telemetry state; change
the logic around takeSessionTelemetry/analytics.record so you first call
takeSessionTelemetry(input.threadId) into sessionTelemetry and then only call
analytics.record("provider.session.end", ...) when sessionTelemetry is non-null
(use sessionTelemetry?.adapterPath and compute durationMs from
sessionTelemetry.startedAtMs); this prevents double-emitting when
processRuntimeEvent drained sessionTelemetry before stopSession() resumes.
- Around line 614-654: The resolved MCP config (resolvedMcp / mcpContext) must
be persisted to the MCP service before delegating to adapter.startSession;
update ProviderService.startSession to call the McpConfigService.setSnapshot (or
equivalent setSnapshot) with the resolvedMcp (and threadId/context) immediately
after computing toResolvedMcpContext(resolvedMcp) and recording analytics, but
before invoking adapter.startSession, so that
McpConfigService.getSnapshot(threadId) will return the materialized MCP during
adapter startup.
In `@apps/server/src/provider/mcpTranslation.ts`:
- Around line 13-18: sanitizeName collapses different server names into
identical TOML keys which can produce duplicate sections; update
codexTomlFromResolved (and codexServerBlock which uses sanitizeName) to detect
collisions after sanitization and reject (throw or return an error) when two
distinct McpServerConfig.name values map to the same sanitized key.
Specifically, build a map from sanitizeName(server.name) to original names while
generating sections in codexTomlFromResolved, check for existing keys and if a
collision occurs include both conflicting original names in the error message so
callers can surface it; ensure codexServerBlock continues to use sanitizeName
but only after the collision check.
- Around line 72-77: generatedMcpDir currently calls path.join(stateDir, "mcp",
provider, String(threadId)) with an unsanitized ThreadId, allowing path
traversal (e.g., "../", "/" or "\"), so update generatedMcpDir to
validate/sanitize threadId: either reject any threadId containing path
separators or dot-segments ("/", "\\", "..") and throw, or encode the threadId
(e.g., url-safe base64) before joining; alternatively tighten the ThreadId
schema to only allow UUID/alphanumeric-hyphen and ensure adapters use the
validated form—implement the chosen fix inside generatedMcpDir (and adjust
callers if encoding is used) and reference the ThreadId value consistently to
avoid accepting unsafe values.
---
Outside diff comments:
In `@apps/harness/lib/harness/metrics.ex`:
- Around line 13-20: collect/0 currently calls session_metrics/0 and also calls
lifecycle_metrics/0 which re-calls session_metrics/0, causing duplicate
collection; change collect/0 to compute sessions = session_metrics() once and
pass it into a new lifecycle_metrics/1 (or update existing lifecycle_metrics to
accept a sessions argument) so lifecycle_metrics no longer calls
session_metrics/0 internally; update any other callers of lifecycle_metrics/0
(e.g., places flagged around lines 89-97) to either call
lifecycle_metrics(precomputed_sessions) or fall back to session_metrics() only
when a sessions arg is not provided.
---
Nitpick comments:
In `@apps/server/src/provider/Layers/CodexAdapter.ts`:
- Around line 1393-1429: The materialization block that computes
translatedHomePath (inside CodexAdapter where generatedMcpDir is used) currently
uses synchronous fs calls (mkdirSync, cpSync, writeFileSync) which can block the
event loop; replace these with the asynchronous fileSystem APIs available in
scope (use await/promise style inside Effect.try or convert to
Effect.async/Effect.attemptPromise) to perform directory creation, recursive
copy, and file write (mirroring the current behavior of creating
generatedHomePath, copying baseHomePath -> generatedHomePath when present, and
writing config.toml from codexTomlFromResolved(resolvedMcp)); preserve the
existing ProviderAdapterProcessError construction on failure and keep the
returned generatedHomePath value and the surrounding logic (translatedHomePath
?? baseHomePath) unchanged so semantics remain identical.
In `@apps/server/src/provider/Layers/HarnessProvider.ts`:
- Around line 25-34: In makeHarnessProviderLayer, remove the redundant type
assertions that re-cast the already-narrowed provider parameter and settings
entry: update the getProviderSettings mapping to directly index
settings.providers[provider] and treat it as HarnessProviderSettings (or remove
the unnecessary "cursor" | "opencode" cast), and likewise remove the duplicate
casts present near the other usage (the second occurrence around the
getProviderSettings consumer). Touch the symbols: makeHarnessProviderLayer,
provider, getProviderSettings, ServerSettingsService, and
HarnessProviderSettings to eliminate the unnecessary "as" assertions so
TypeScript relies on the function parameter's narrowed union type.
In `@apps/server/src/provider/Layers/McpConfig.ts`:
- Around line 182-200: The code silently swallows filesystem and JSON parse
errors when reading MCP config; update the read and parse paths to log failures
(including error details, targetPath and threadId) before returning null: change
the Effect.catch used with fileSystem.readFileString to capture the caught error
and call the existing logger (e.g., processLogger or logger) with a descriptive
message and the error, and wrap the JSON.parse catch to log the parse exception
and a short preview of raw before returning null; keep behavior of returning
null on failure but ensure errors are logged; references:
fileSystem.readFileString, Effect.catch, JSON.parse, ResolvedMcpConfig,
Schema.is, cacheSnapshot, threadId, targetPath.
In `@apps/server/src/provider/Layers/ProviderRegistry.ts`:
- Around line 111-131: The three mappings in codexProvider repeat identical
logic; extract a small helper (e.g., wrapWithCodexCapabilities or
applyCodexCaps) that takes a stream/effect (used on CodexProvider.getSnapshot,
.refresh, .streamChanges) and returns the mapped version that merges
HARNESS_PROVIDER_CAPABILITIES.codex into the provider; replace the three inline
Effect.map/Stream.map blocks with calls to this helper applied to
codexProviderBase.getSnapshot, codexProviderBase.refresh, and
codexProviderBase.streamChanges to centralize the capability-wrapping logic.
In `@apps/server/src/provider/providerCapabilities.ts`:
- Around line 17-19: The cursor entry currently spreads
DEFAULT_PROVIDER_CAPABILITIES.cursor without changes; add a concise inline
comment next to the cursor property explaining that the default cursor
capabilities are intentionally used for harness mode (e.g., "using default
cursor capabilities for harness mode — no overrides required") so future readers
understand this is deliberate; update the cursor block where
DEFAULT_PROVIDER_CAPABILITIES.cursor is spread to include that comment near the
cursor symbol.
In `@apps/server/src/provider/Services/McpConfig.ts`:
- Around line 35-54: The test Layer default is currently no-op for snapshots so
tests never exercise snapshot lifecycle; update layerTest to back
setSnapshot/getSnapshot/clearSnapshot with a closure-scoped in-memory store:
inside the layerTest factory create a local variable (e.g. let currentSnapshot:
ReturnType<McpConfigServiceShape["getSnapshot"]> | null = null) and set the
defaults to options?.setSnapshot ?? ((snap) => Effect.sync(() => {
currentSnapshot = snap })) , options?.getSnapshot ?? (() =>
Effect.succeed(currentSnapshot)), and options?.clearSnapshot ?? (() =>
Effect.sync(() => { currentSnapshot = null })); keep the overrides from options
and ensure the resulting object still satisfies McpConfigServiceShape.
In `@apps/server/src/serverLayers.ts`:
- Around line 117-120: Document the T3CODE_CODEX_LEGACY environment variable and
its effects: note that setting T3CODE_CODEX_LEGACY="1" causes useLegacyCodex to
be true and changes HARNESS_PROVIDERS from ["codex","cursor","opencode"] to
["cursor","opencode"], meaning Codex will use the Node SDK adapter directly
instead of routing through the harness; add its purpose, accepted values,
default behavior, and operational impact (including how it affects
useLegacyCodex and HARNESS_PROVIDERS) to the project's environment/configuration
docs or README so operators and developers can discover and use it correctly.
🪄 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: b370b64e-f5b1-4315-b027-8a7421bec791
📒 Files selected for processing (41)
apps/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/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/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
Comment on lines
+238
to
+315
| export function classifyProviderError(error: unknown): ProviderErrorClassification { | ||
| if ( | ||
| Schema.is(ProviderAdapterValidationError)(error) || | ||
| Schema.is(ProviderValidationError)(error) | ||
| ) { | ||
| return { | ||
| category: "permanent", | ||
| recoveryStrategy: "fail-fast", | ||
| recoverable: false, | ||
| }; | ||
| } | ||
|
|
||
| if ( | ||
| Schema.is(ProviderAdapterSessionNotFoundError)(error) || | ||
| Schema.is(ProviderSessionNotFoundError)(error) | ||
| ) { | ||
| return { | ||
| category: "configuration", | ||
| recoveryStrategy: "fresh-session", | ||
| recoverable: true, | ||
| }; | ||
| } | ||
|
|
||
| if (Schema.is(ProviderAdapterSessionClosedError)(error)) { | ||
| return { | ||
| category: "transient", | ||
| recoveryStrategy: "restart-session", | ||
| recoverable: true, | ||
| }; | ||
| } | ||
|
|
||
| if (Schema.is(ProviderAdapterRequestError)(error)) { | ||
| return requestErrorClassification(error.detail); | ||
| } | ||
|
|
||
| if (Schema.is(ProviderAdapterProcessError)(error)) { | ||
| const normalized = error.detail.toLowerCase(); | ||
| if ( | ||
| normalized.includes("not installed") || | ||
| normalized.includes("enoent") || | ||
| normalized.includes("no such file") || | ||
| normalized.includes("binary") | ||
| ) { | ||
| return { | ||
| category: "provider-unavailable", | ||
| recoveryStrategy: "degrade-gracefully", | ||
| recoverable: true, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| category: "transient", | ||
| recoveryStrategy: "restart-session", | ||
| recoverable: true, | ||
| }; | ||
| } | ||
|
|
||
| if (Schema.is(ProviderUnsupportedError)(error)) { | ||
| return { | ||
| category: "provider-unavailable", | ||
| recoveryStrategy: "degrade-gracefully", | ||
| recoverable: true, | ||
| }; | ||
| } | ||
|
|
||
| if (Schema.is(ProviderSessionDirectoryPersistenceError)(error)) { | ||
| return { | ||
| category: "configuration", | ||
| recoveryStrategy: "re-resolve-config", | ||
| recoverable: true, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| category: "permanent", | ||
| recoveryStrategy: "fail-fast", | ||
| recoverable: false, | ||
| }; |
There was a problem hiding this comment.
CheckpointServiceError has no explicit mapping here.
classifyProviderError() never branches on checkpointing failures, so they fall through to permanent / fail-fast if they reach this helper. With snapshot persistence added in this PR, that is likely to mislabel recoverable store faults. Please either classify checkpoint errors here or keep them out of this helper until they have a mapping.
🤖 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 238 - 315,
classifyProviderError currently omits CheckpointServiceError so checkpoint/store
faults fall through as permanent; add a branch that detects
Schema.is(CheckpointServiceError)(error) in classifyProviderError and return an
appropriate recoverable classification (e.g., category: "transient" or
"provider-unavailable" depending on your fault model, recoveryStrategy:
"retry-persist" or "degrade-gracefully", recoverable: true) so snapshot
persistence errors are not marked fail-fast; reference the
Schema.is(CheckpointServiceError) check and ensure it appears before the final
fall-through return (or alternatively explicitly exclude CheckpointServiceError
from using this helper until a mapping exists).
Comment on lines
+303
to
+308
| if (Schema.is(ProviderSessionDirectoryPersistenceError)(error)) { | ||
| return { | ||
| category: "configuration", | ||
| recoveryStrategy: "re-resolve-config", | ||
| recoverable: true, | ||
| }; |
There was a problem hiding this comment.
Don’t route storage-contention errors through re-resolve-config.
This branch treats every ProviderSessionDirectoryPersistenceError as a configuration issue, but failures like sqlite busy or database is locked are transient operational errors. Classifying them as re-resolve-config pushes the wrong recovery path and skips retry/backoff for a recoverable condition.
Suggested fix
if (Schema.is(ProviderSessionDirectoryPersistenceError)(error)) {
+ const normalized = error.detail.toLowerCase();
+
+ if (
+ normalized.includes("busy") ||
+ normalized.includes("locked") ||
+ normalized.includes("timeout")
+ ) {
+ return {
+ category: "transient",
+ recoveryStrategy: "retry-backoff",
+ recoverable: true,
+ };
+ }
+
return {
category: "configuration",
recoveryStrategy: "re-resolve-config",
recoverable: true,
};
}📝 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
| if (Schema.is(ProviderSessionDirectoryPersistenceError)(error)) { | |
| return { | |
| category: "configuration", | |
| recoveryStrategy: "re-resolve-config", | |
| recoverable: true, | |
| }; | |
| if (Schema.is(ProviderSessionDirectoryPersistenceError)(error)) { | |
| const normalized = error.detail.toLowerCase(); | |
| if ( | |
| normalized.includes("busy") || | |
| normalized.includes("locked") || | |
| normalized.includes("timeout") | |
| ) { | |
| return { | |
| category: "transient", | |
| recoveryStrategy: "retry-backoff", | |
| recoverable: true, | |
| }; | |
| } | |
| return { | |
| category: "configuration", | |
| recoveryStrategy: "re-resolve-config", | |
| recoverable: true, | |
| }; | |
| } |
🤖 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 303 - 308, The current
Schema.is(ProviderSessionDirectoryPersistenceError)(error) branch classifies all
persistence errors as configuration issues; change it to distinguish transient
storage/contention errors (e.g. messages or nested codes like "sqlite busy",
"database is locked" or equivalent DB_LOCKED codes) from true configuration
failures: within the ProviderSessionDirectoryPersistenceError handling, inspect
error.message and any nested error.code/innerError to detect these transient
conditions and for them return { category: "transient", recoveryStrategy:
"retry-with-backoff", recoverable: true }, otherwise keep the configuration path
({ category: "configuration", recoveryStrategy: "re-resolve-config",
recoverable: true }). Use the same ProviderSessionDirectoryPersistenceError /
Schema.is(...) spot to implement this branching so retries/backoff are attempted
for storage contention instead of re-resolving config.
Comment on lines
+1106
to
+1127
| const generatedDir = generatedMcpDir( | ||
| serverConfig.stateDir, | ||
| "codex", | ||
| input.threadId, | ||
| ); | ||
| 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, | ||
| }); | ||
| } | ||
| fs.writeFileSync( | ||
| path.join(generatedHomePath, "config.toml"), | ||
| codexTomlFromResolved(resolvedMcp), | ||
| "utf8", | ||
| ); |
There was a problem hiding this comment.
Lock down the generated MCP artifact permissions.
These files can carry MCP env secrets, but the current mkdirSync / writeFileSync calls rely on process-default modes. On a typical umask 022 path, config.toml and opencode.json end up readable by other local users.
🔒 Tighten the generated directory and file modes
const generatedHomePath = path.join(generatedDir, "home");
fs.mkdirSync(generatedHomePath, { recursive: true });
+ fs.chmodSync(generatedDir, 0o700);
+ fs.chmodSync(generatedHomePath, 0o700);
if (
baseCodexHomePath &&
fs.existsSync(baseCodexHomePath) &&
baseCodexHomePath !== generatedHomePath
@@
fs.writeFileSync(
path.join(generatedHomePath, "config.toml"),
codexTomlFromResolved(resolvedMcp),
"utf8",
);
+ fs.chmodSync(path.join(generatedHomePath, "config.toml"), 0o600);
@@
const generatedDir = generatedMcpDir(
serverConfig.stateDir,
"opencode",
input.threadId,
);
fs.mkdirSync(generatedDir, { recursive: true });
+ fs.chmodSync(generatedDir, 0o700);
const configPath = path.join(generatedDir, "opencode.json");
fs.writeFileSync(
configPath,
openCodeConfigFromResolved(resolvedMcp),
"utf8",
);
+ fs.chmodSync(configPath, 0o600);Also applies to: 1135-1145
🤖 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 1106 -
1127, The generated MCP artifacts created under generatedMcpDir (variable
generatedDir / generatedHomePath) are created with process-default permissions;
tighten them by explicitly setting directory and file modes: create
generatedHomePath with mode 0o700 (use fs.mkdirSync with mode or chmod after
creation), ensure any copied content from baseCodexHomePath is recursively
restricted (after fs.cpSync set chmod 0o700 for directories and 0o600 for
files), and write config.toml and opencode.json (the fs.writeFileSync call and
any similar writes around lines ~1135-1145) then set their mode to 0o600 (via
fs.chmodSync) so secrets are not world-readable. Ensure these changes reference
the existing symbols generatedMcpDir, generatedHomePath, baseCodexHomePath,
fs.cpSync, fs.mkdirSync, and fs.writeFileSync.
Comment on lines
+614
to
654
| const resolvedMcp = yield* mcpConfig | ||
| .resolveConfig({ | ||
| provider: input.provider, | ||
| cwd: effectiveCwd, | ||
| threadId, | ||
| }) | ||
| .pipe( | ||
| Effect.mapError((error) => | ||
| toValidationError( | ||
| "ProviderService.startSession.resolveMcpConfig", | ||
| `Failed to resolve MCP config: ${error.detail}`, | ||
| error, | ||
| ), | ||
| ), | ||
| ); | ||
| const mcpContext = toResolvedMcpContext(resolvedMcp); | ||
| const mcpSupported = adapter.capabilities.mcpConfig !== "none"; | ||
| yield* analytics.record("mcp.config.resolved", { | ||
| provider: input.provider, | ||
| adapterPath, | ||
| version: mcpContext.version, | ||
| serverCount: mcpContext.serverCount, | ||
| sourceCount: mcpContext.sourceCount, | ||
| supported: mcpSupported, | ||
| }); | ||
| if (mcpContext.serverCount > 0) { | ||
| yield* analytics.record(mcpSupported ? "mcp.config.sent" : "mcp.config.deferred", { | ||
| provider: input.provider, | ||
| adapterPath, | ||
| version: mcpContext.version, | ||
| serverCount: mcpContext.serverCount, | ||
| sourceCount: mcpContext.sourceCount, | ||
| reason: mcpSupported ? undefined : "provider-capability-none", | ||
| phase: "session-start", | ||
| }); | ||
| } | ||
| const session = yield* adapter.startSession({ | ||
| ...input, | ||
| cwd: effectiveCwd, | ||
| ...(effectiveResumeCursor !== undefined ? { resumeCursor: effectiveResumeCursor } : {}), | ||
| }); |
There was a problem hiding this comment.
Persist the resolved MCP snapshot before starting the adapter.
This path resolves MCP config and records mcp.config.sent, but it never setSnapshot()s the result. The adapters now read McpConfigService.getSnapshot(threadId) during startup, so first-time session starts can miss MCP materialization entirely.
🧩 Store the snapshot before delegating to `adapter.startSession()`
const resolvedMcp = yield* mcpConfig
.resolveConfig({
provider: input.provider,
cwd: effectiveCwd,
threadId,
})
.pipe(
Effect.mapError((error) =>
toValidationError(
"ProviderService.startSession.resolveMcpConfig",
`Failed to resolve MCP config: ${error.detail}`,
error,
),
),
);
+ yield* mcpConfig.setSnapshot(threadId, resolvedMcp);
const mcpContext = toResolvedMcpContext(resolvedMcp);
const mcpSupported = adapter.capabilities.mcpConfig !== "none";🤖 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 614 - 654,
The resolved MCP config (resolvedMcp / mcpContext) must be persisted to the MCP
service before delegating to adapter.startSession; update
ProviderService.startSession to call the McpConfigService.setSnapshot (or
equivalent setSnapshot) with the resolvedMcp (and threadId/context) immediately
after computing toResolvedMcpContext(resolvedMcp) and recording analytics, but
before invoking adapter.startSession, so that
McpConfigService.getSnapshot(threadId) will return the materialized MCP during
adapter startup.
Comment on lines
+833
to
+845
| 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", | ||
| }); |
There was a problem hiding this comment.
Avoid double-emitting provider.session.end on explicit stop.
processRuntimeEvent() already drains sessionTelemetryRef on session.exited. If the provider emits that event before stopSession() resumes, this block records a second provider.session.end with durationMs: null.
📉 Only emit the explicit-stop metric when this code still owns the telemetry state
- 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",
+ });
+ }📝 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
| 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", | |
| }); | |
| 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, | |
| }); | |
| 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 833 - 845,
The explicit-stop block should only emit the "provider.session.end" metric if
this code still owns telemetry state; change the logic around
takeSessionTelemetry/analytics.record so you first call
takeSessionTelemetry(input.threadId) into sessionTelemetry and then only call
analytics.record("provider.session.end", ...) when sessionTelemetry is non-null
(use sessionTelemetry?.adapterPath and compute durationMs from
sessionTelemetry.startedAtMs); this prevents double-emitting when
processRuntimeEvent drained sessionTelemetry before stopSession() resumes.
Comment on lines
+13
to
+18
| function sanitizeName(name: string): string { | ||
| return name.replace(/[^a-zA-Z0-9_-]+/g, "_"); | ||
| } | ||
|
|
||
| function codexServerBlock(server: McpServerConfig): string { | ||
| const section = [`[mcp_servers.${sanitizeName(server.name)}]`]; |
There was a problem hiding this comment.
Reject collisions after sanitizing Codex server names.
sanitizeName() collapses distinct names like foo/bar and foo bar into the same TOML key. That makes codexTomlFromResolved() emit duplicate [mcp_servers.<name>] sections, so one server definition gets clobbered or the file becomes invalid.
🤖 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 - 18,
sanitizeName collapses different server names into identical TOML keys which can
produce duplicate sections; update codexTomlFromResolved (and codexServerBlock
which uses sanitizeName) to detect collisions after sanitization and reject
(throw or return an error) when two distinct McpServerConfig.name values map to
the same sanitized key. Specifically, build a map from sanitizeName(server.name)
to original names while generating sections in codexTomlFromResolved, check for
existing keys and if a collision occurs include both conflicting original names
in the error message so callers can surface it; ensure codexServerBlock
continues to use sanitizeName but only after the collision check.
Comment on lines
+72
to
+77
| 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 --glob '*.ts' 'export const ThreadId|export type ThreadId|ThreadId\s*=|ThreadId\.makeUnsafe|brand\("ThreadId"\)' packages/contracts apps/serverRepository: Ranvier-Technologies/t3code-OTP
Length of output: 50388
🏁 Script executed:
find packages/contracts -name '*.ts' -o -name '*.js' | head -20Repository: Ranvier-Technologies/t3code-OTP
Length of output: 811
🏁 Script executed:
rg -n 'type ThreadId|const ThreadId' packages/contracts --glob '*.ts' --max-count 5Repository: Ranvier-Technologies/t3code-OTP
Length of output: 251
🏁 Script executed:
rg -n 'brand.*ThreadId\|ThreadId.*=.*brand' packages/contracts --glob '*.ts'Repository: Ranvier-Technologies/t3code-OTP
Length of output: 57
🏁 Script executed:
cat -n packages/contracts/src/baseSchemas.ts | head -50Repository: Ranvier-Technologies/t3code-OTP
Length of output: 2405
🏁 Script executed:
rg -n 'makeEntityId|function makeEntityId' packages/contracts --glob '*.ts' -B2 -A10Repository: Ranvier-Technologies/t3code-OTP
Length of output: 2993
🏁 Script executed:
rg -n 'ThreadId\.makeUnsafe|ThreadId\.make' apps/server/src --glob '*.ts' -B2 -A2 | head -80Repository: Ranvier-Technologies/t3code-OTP
Length of output: 7960
🏁 Script executed:
cat -n apps/server/src/provider/mcpTranslation.ts | sed -n '65,80p'Repository: Ranvier-Technologies/t3code-OTP
Length of output: 461
Confirm ThreadId path traversal risk and sanitize generatedMcpDir().
The ThreadId schema (in packages/contracts/src/baseSchemas.ts) only enforces non-empty trimmed strings via TrimmedNonEmptyString.pipe(Schema.brand("ThreadId")). It does not restrict path separators (/, \) or dot-segments (..).
In generatedMcpDir() at line 77, the raw threadId is passed directly to path.join() without sanitization. While production code currently generates safe UUIDs via crypto.randomUUID(), provider adapters (e.g., ClaudeAdapter.ts:404) create ThreadIds from external provider responses using .makeUnsafe(), bypassing validation. A malicious provider response could inject ../../etc/passwd or absolute paths to escape stateDir.
Either:
- Sanitize the threadId in
generatedMcpDir()(e.g., reject/,\,..or encode via base64/url-safe encoding), or - Restrict ThreadId schema to forbid path traversal characters (recommended: enforce UUID or alphanumeric+hyphen only).
🤖 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 72 - 77,
generatedMcpDir currently calls path.join(stateDir, "mcp", provider,
String(threadId)) with an unsanitized ThreadId, allowing path traversal (e.g.,
"../", "/" or "\"), so update generatedMcpDir to validate/sanitize threadId:
either reject any threadId containing path separators or dot-segments ("/",
"\\", "..") and throw, or encode the threadId (e.g., url-safe base64) before
joining; alternatively tighten the ThreadId schema to only allow
UUID/alphanumeric-hyphen and ensure adapters use the validated form—implement
the chosen fix inside generatedMcpDir (and adjust callers if encoding is used)
and reference the ThreadId value consistently to avoid accepting unsafe values.
Collaborator
Author
|
Addressed the actionable Devin findings in
Validation rerun after the fixes:
I am intentionally leaving these as non-blocking follow-ups for a later pass:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/contracts/src/orchestration.ts (1)
125-169: 🛠️ Refactor suggestion | 🟠 MajorKeep provider default capability policy out of
packages/contracts.Adding these per-provider defaults here grows the runtime/provider-policy surface in a schema package.
apps/server/src/provider/providerCapabilities.ts:1-24already exists as the provider-layer home for capability constants, so this package should keep only the schema/type definitions and move the default map there.As per coding guidelines,
packages/contractsis schema-only — no runtime logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contracts/src/orchestration.ts` around lines 125 - 169, DEFAULT_PROVIDER_CAPABILITIES (the per-provider default map) is runtime/provider-policy data and should be removed from the schema package; move the DEFAULT_PROVIDER_CAPABILITIES constant into the provider-layer module that owns provider capability constants (the providerCapabilities module), keep only the type defs ProviderKind and ProviderCapabilities in this contracts module, export the moved constant from the provider-layer module, and update all call sites/imports to import DEFAULT_PROVIDER_CAPABILITIES from that provider-layer module instead of from packages/contracts.
🧹 Nitpick comments (1)
packages/contracts/src/orchestration.ts (1)
108-120: Define the meaning ofbasicandfullin the contract.
none/basic/fullis now part of the shared provider contract, but this schema does not say whatbasicorfullguarantees forresume,attachments,replay, ormcpConfig. That leaves adapter authors to interpret the same value differently. Add brief field-level docs here and mirror them inAGENTS.md.Based on learnings, document agent capabilities, interfaces, and communication protocols in
AGENTS.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/contracts/src/orchestration.ts` around lines 108 - 120, The ProviderCapabilityLevel schema (ProviderCapabilityLevel and ProviderCapabilities) uses literals "none", "basic", and "full" but lacks field-level docs explaining what "basic" and "full" guarantee for fields like resume, attachments, replay, and mcpConfig; update the Schema.Struct declaration for ProviderCapabilities to add concise inline comments/docstrings for each affected field (resume, attachments, replay, mcpConfig) that define the behavioral contract for "basic" vs "full" (e.g., what operations, data shapes, and failure modes each level must support), and mirror these same definitions in AGENTS.md under the agent capability section so adapter authors have explicit examples and protocols to implement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/contracts/src/orchestration.ts`:
- Around line 125-169: DEFAULT_PROVIDER_CAPABILITIES (the per-provider default
map) is runtime/provider-policy data and should be removed from the schema
package; move the DEFAULT_PROVIDER_CAPABILITIES constant into the provider-layer
module that owns provider capability constants (the providerCapabilities
module), keep only the type defs ProviderKind and ProviderCapabilities in this
contracts module, export the moved constant from the provider-layer module, and
update all call sites/imports to import DEFAULT_PROVIDER_CAPABILITIES from that
provider-layer module instead of from packages/contracts.
---
Nitpick comments:
In `@packages/contracts/src/orchestration.ts`:
- Around line 108-120: The ProviderCapabilityLevel schema
(ProviderCapabilityLevel and ProviderCapabilities) uses literals "none",
"basic", and "full" but lacks field-level docs explaining what "basic" and
"full" guarantee for fields like resume, attachments, replay, and mcpConfig;
update the Schema.Struct declaration for ProviderCapabilities to add concise
inline comments/docstrings for each affected field (resume, attachments, replay,
mcpConfig) that define the behavioral contract for "basic" vs "full" (e.g., what
operations, data shapes, and failure modes each level must support), and mirror
these same definitions in AGENTS.md under the agent capability section so
adapter authors have explicit examples and protocols to implement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58db03d0-2ba2-4c09-92e7-2af8c6d8088c
📒 Files selected for processing (5)
apps/harness/lib/harness/metrics.exapps/server/integration/TestProviderAdapter.integration.tsapps/server/src/provider/Layers/ProviderService.tsapps/server/src/provider/mcpTranslation.tspackages/contracts/src/orchestration.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/server/integration/TestProviderAdapter.integration.ts
- apps/server/src/provider/mcpTranslation.ts
- apps/harness/lib/harness/metrics.ex
- apps/server/src/provider/Layers/ProviderService.ts
Comment on lines
+1405
to
+1417
| fs.mkdirSync(generatedHomePath, { recursive: true }); | ||
| if ( | ||
| baseHomePath && | ||
| fs.existsSync(baseHomePath) && | ||
| baseHomePath !== generatedHomePath | ||
| ) { | ||
| fs.cpSync(baseHomePath, generatedHomePath, { recursive: true, force: true }); | ||
| } | ||
| fs.writeFileSync( | ||
| path.join(generatedHomePath, "config.toml"), | ||
| codexTomlFromResolved(resolvedMcp), | ||
| "utf8", | ||
| ); |
There was a problem hiding this comment.
🔴 Generated config.toml overwrites user's non-MCP Codex settings when materializing MCP configuration
When MCP servers are resolved for a Codex session, the code copies the user's base CODEX_HOME directory to a generated session-specific directory (fs.cpSync), then completely overwrites config.toml with a file containing only [mcp_servers.*] entries (codexTomlFromResolved). Any non-MCP settings the user had in their config.toml (e.g., [api], [model], custom sandbox settings) are silently destroyed in the session-specific copy. The codexTomlFromResolved function at apps/server/src/provider/mcpTranslation.ts:42-49 generates only a header comment and MCP server blocks — no other TOML sections are preserved.
Identical pattern in HarnessClientAdapter
The same bug exists in apps/server/src/provider/Layers/HarnessClientAdapter.ts:1112-1127 where the harness codex path also copies the base home and overwrites config.toml.
Prompt for agents
In apps/server/src/provider/Layers/CodexAdapter.ts (lines 1405-1417) and apps/server/src/provider/Layers/HarnessClientAdapter.ts (lines 1112-1127), the code copies the user's CODEX_HOME and then overwrites config.toml with only MCP entries. Instead of fully replacing config.toml, the generated MCP TOML should be merged with the existing config.toml content. One approach: read the existing config.toml from the copied directory (if it exists), strip any existing [mcp_servers.*] sections, then append the generated MCP blocks. This preserves non-MCP user settings while still injecting the resolved MCP servers. Extract this logic into a shared helper in mcpTranslation.ts (e.g. mergeCodexTomlMcpServers) to avoid the current code duplication between CodexAdapter and HarnessClientAdapter, consistent with the AGENTS.md maintainability rules.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+1105
to
+1132
| case "codex": { | ||
| const generatedDir = generatedMcpDir( | ||
| serverConfig.stateDir, | ||
| "codex", | ||
| input.threadId, | ||
| ); | ||
| 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, | ||
| }); | ||
| } | ||
| fs.writeFileSync( | ||
| path.join(generatedHomePath, "config.toml"), | ||
| codexTomlFromResolved(resolvedMcp), | ||
| "utf8", | ||
| ); | ||
| return { | ||
| codex: { | ||
| homePath: generatedHomePath, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🔴 Duplicate MCP config materialization logic across CodexAdapter and HarnessClientAdapter violates AGENTS.md maintainability rules
The Codex MCP home-directory materialization logic (create generated dir → copy base home → write config.toml) is nearly identical in apps/server/src/provider/Layers/CodexAdapter.ts:1397-1428 and apps/server/src/provider/Layers/HarnessClientAdapter.ts:1105-1132. The repository's AGENTS.md explicitly states: "Duplicate logic across multiple files is a code smell and should be avoided. Don't take shortcuts by just adding local logic to solve a problem." This logic should be extracted into a shared helper in mcpTranslation.ts (which already houses codexTomlFromResolved and generatedMcpDir).
Prompt for agents
Extract the Codex MCP home-directory materialization logic into a shared function in apps/server/src/provider/mcpTranslation.ts. This function should take (stateDir, threadId, baseHomePath, resolvedMcp) as parameters and return the generated home path. Then both CodexAdapter.ts (lines 1397-1428) and HarnessClientAdapter.ts (lines 1105-1132) should call this shared function instead of duplicating the fs.mkdirSync / fs.cpSync / fs.writeFileSync pattern.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
920
to
924
| yield* analytics.record("provider.conversation.rolled_back", { | ||
| provider: routed.adapter.provider, | ||
| adapterPath: routed.adapterPath, | ||
| turns: input.numTurns, | ||
| }); |
There was a problem hiding this comment.
🟡 rollbackConversation emits 'rolled_back' telemetry event even when rollback fails
In ProviderService.ts, the rollbackConversation method records a "provider.conversation.rolled_back" analytics event (line 920) before checking the rollback result for failure (line 931). This means the event is emitted even when rollbackResult._tag === "Failure". The event name "provider.conversation.rolled_back" strongly implies success, which is misleading for analytics consumers. The second event "provider.rollback.outcome" does include the correct success flag, but the first event creates a data integrity issue where dashboards counting rolled_back events would overcount successful rollbacks.
Suggested change
| yield* analytics.record("provider.conversation.rolled_back", { | |
| provider: routed.adapter.provider, | |
| adapterPath: routed.adapterPath, | |
| turns: input.numTurns, | |
| }); | |
| if (rollbackResult._tag === "Success") { | |
| yield* analytics.record("provider.conversation.rolled_back", { | |
| provider: routed.adapter.provider, | |
| adapterPath: routed.adapterPath, | |
| turns: input.numTurns, | |
| }); | |
| } |
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
Validation
Notes
bun run testwas not runmix testwas not runSummary by CodeRabbit
New Features
Bug Fixes
Chores