.plans/17-claude-code.md-98-115 (1)

98-115: ⚠️ Potential issue | 🟠 Major

Add credential management and security considerations.

The plan lists Claude SDK configuration options (binary path, permission mode, max thinking tokens) but doesn't address how API keys or authentication credentials for the Claude SDK will be managed, stored, or rotated.

For a production multi-provider system, the plan should specify:

1. Where provider credentials are stored (environment variables, secure settings, keychain)
2. How credentials are scoped (per-user, per-workspace, global)
3. Whether credentials are encrypted at rest
4. Rotation and expiration policies
5. How credential failures are surfaced to users

This is especially critical since the PR introduces 6 different provider integrations, each likely requiring separate authentication.

Additionally, consider documenting:

• Rate limiting strategies per provider to avoid quota exhaustion
• Concurrent session limits (memory/resource constraints when multiple providers are active)
• Observability patterns (structured logging with provider context, per-provider metrics)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.plans/17-claude-code.md around lines 98 - 115, Update the plan to include a
clear credential and security policy for the new Claude/provider integrations:
specify where credentials are stored (env vars, secrets manager, keychain),
scoping rules (per-user, per-workspace, global), encryption-at-rest
requirements, rotation/expiration policies and how failures surface to users
(errors/alerts), and map these to the adapter options like
providerOptions.claudeCode (pathToClaudeCodeExecutable, permissionMode,
maxThinkingTokens) plus top-level options (env, additionalDirectories, hooks,
resume/resumeSessionAt, includePartialMessages, canUseTool); also add production
guidelines for per-provider rate limiting, concurrent session limits, and
observability (structured logging/metrics with provider context) so each of the
six providers has documented auth, rotation, and failure-handling expectations.

.plans/17-claude-code.md-374-417 (1)

374-417: 🛠️ Refactor suggestion | 🟠 Major

Expand testing strategy to cover multi-provider scenarios.

The testing strategy is thorough for single-provider integration but doesn't address critical multi-provider scenarios that are central to this PR's scope:

Missing test coverage areas:

1. Provider switching: What happens when a user switches from Codex to Claude mid-thread?
2. Concurrent sessions: Multiple providers active simultaneously (memory, performance, state isolation)
3. Provider fallback: Behavior when a selected provider becomes unavailable
4. Cross-provider resume: Whether resume cursors are strictly scoped to prevent leakage
5. Registry edge cases: Provider discovery when some adapters fail to initialize
6. Health monitoring: Provider-specific health checks and degradation scenarios
7. Event ordering: Race conditions when multiple providers emit runtime events simultaneously

📋 Suggested additional test coverage

Add to Phase 6.4 Orchestration integration tests:

5. **Multi-provider scenarios**:
   - Provider switching mid-conversation
   - Concurrent active sessions across different providers
   - Provider availability changes during active sessions
   - Resume cursor isolation (Claude cursor cannot start Codex session)
   - Health monitoring and fallback behavior
   - Event ordering and concurrency under multi-provider load

Add to Phase 6.3 Provider service integration tests:

7. **Provider registry and health**:
   - All providers report correct availability status
   - Provider initialization failures don't block healthy providers
   - Health probes execute correctly for each provider type

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.plans/17-claude-code.md around lines 374 - 417, The Phase 6 testing plan
misses multi-provider scenarios; add explicit tests for provider switching
mid-conversation, concurrent active sessions across different providers,
provider fallback when a provider becomes unavailable, resume cursor isolation
(ensure Claude cursors cannot resume Codex and vice versa), provider registry
initialization failures not blocking healthy providers, per-provider health
probes and degradation behavior, and event ordering/race conditions when
multiple providers emit runtime events. Implement these as: (1) additions to
ClaudeCodeAdapter.test.ts for adapter-level concurrency, resume/interrupt and
rollback edge cases; (2) extensions to Phase 6.3 ProviderService integration
tests to validate registry/health, start/send/restart flows across multiple
providers; and (3) Phase 6.4 orchestration integration tests to cover provider
switching, concurrent sessions, fallback/recovery, checkpoint revert under
multi-provider load, and event ordering under concurrency. Ensure tests use
SDK-facing boundaries only and assert strict provider scoping for resume cursors
and session isolation.

.plans/17-claude-code.md-1-16 (1)

1-16: ⚠️ Potential issue | 🟠 Major

Plan scope significantly misaligned with implementation: covers Claude only while PR implements 6+ providers.

The plan file is titled and scoped exclusively for Claude Code integration, with its introduction specifically stating "Claude integration must plug into this path." However, verification confirms the PR implements a full multi-provider adapter infrastructure including ClaudeCodeAdapter, CopilotAdapter, OpenCodeAdapter, GeminiCliAdapter, KiloAdapter, and AmpAdapter—each with corresponding server managers and orchestration routing.

This creates a documentation maintenance problem: future readers will find a plan that doesn't capture the multi-provider patterns, abstractions, or design decisions actually implemented across 6+ providers.

Update the plan to either:

• Generalize it as a multi-provider integration guide (rename and restructure to show how new providers plug into the orchestration path)
• Or add a prominent note explaining this documents the pattern applicable to all providers, not just Claude

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.plans/17-claude-code.md around lines 1 - 16, The plan is scoped to "Claude
Code" but the PR implements a multi-provider adapter infrastructure
(ClaudeCodeAdapter, CopilotAdapter, OpenCodeAdapter, GeminiCliAdapter,
KiloAdapter, AmpAdapter) and shared orchestration plumbing
(orchestration.dispatchCommand, OrchestrationEngine, ProviderCommandReactor,
ProviderService, ProviderRuntimeIngestion, orchestration.domainEvent); update
the document to either (A) generalize and rename it to a multi-provider
integration guide that explains how new adapters plug into the orchestration
path and the common abstractions used across all adapters, or (B) add a
prominent note at the top stating this plan describes a pattern applicable to
all providers and then add sections showing how each adapter (list the six
adapters) fits into the orchestration steps so readers can map implementation to
design.

packages/contracts/src/orchestration.ts-30-39 (1)

30-39: ⚠️ Potential issue | 🟠 Major

Fix Schema.Union to use variadic arguments instead of an array.

Schema.Union expects variadic arguments, not an array. Correct the call by spreading the members:

export const ProviderKind = Schema.Union(
  Schema.Literal("codex"),
  Schema.Literal("copilot"),
  Schema.Literal("claudeCode"),
  Schema.Literal("cursor"),
  Schema.Literal("opencode"),
  Schema.Literal("geminiCli"),
  Schema.Literal("amp"),
  Schema.Literal("kilo"),
);

Or use the spread operator if the array is already defined. The Schema.Literals pattern below (line 41+) is correct for its array-based API.

🤖 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 30 - 39, ProviderKind
is currently created by passing an array to Schema.Union but Schema.Union
requires variadic arguments; fix by calling Schema.Union with each
Schema.Literal(...) as separate arguments (i.e., replace Schema.Union([...])
with Schema.Union(Schema.Literal("codex"), Schema.Literal("copilot"),
Schema.Literal("claudeCode"), Schema.Literal("cursor"),
Schema.Literal("opencode"), Schema.Literal("geminiCli"), Schema.Literal("amp"),
Schema.Literal("kilo")) so the ProviderKind declaration uses variadic literals
rather than an array.

apps/web/src/components/ThreadTerminalDrawer.tsx-56-61 (1)

56-61: ⚠️ Potential issue | 🟠 Major

Keep terminal accent colors contrast-safe.

Using the raw accent for cursor, blue, and brightBlue can make the cursor and ANSI blue text effectively disappear for light/pastel accents on the light terminal theme. Since accent color is user-configurable now, derive these terminal colors against the current background instead of applying the accent verbatim. Reusing the existing contrast logic from apps/web/src/accentColor.ts would avoid that regression.

Also applies to: 67-68, 94-95, 103-111

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ThreadTerminalDrawer.tsx` around lines 56 - 61, The
terminal currently uses the raw normalized accentColor for terminalBlue,
terminalBrightBlue and selection/cursor, which can make blue/cursor invisible
for light/pastel accents; update the logic to compute contrast-safe terminal
colors by reusing the contrast helpers from apps/web/src/accentColor.ts (e.g.,
the function that derives a contrast-safe accent against a background) instead
of using accentColor verbatim: call that helper passing the resolved background
(based on isDark) to produce terminalBlue and terminalBrightBlue and use a
contrast-safe color for the cursor/selection (replacing resolveAccentColorRgba
usage where appropriate); apply the same change to the other occurrences
mentioned (lines around 67-68, 94-95, 103-111) so all terminal accent usages are
derived against the current background.

apps/web/src/accentColor.ts-63-85 (1)

63-85: ⚠️ Potential issue | 🟠 Major

Reject accents whose best foreground still fails contrast.

This only picks the better of white and #111827; it never checks whether that best ratio is actually acceptable. Mid-tone inputs such as #777777 can still fail both choices, so applyAccentColorToDocument can make primary text unreadable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/accentColor.ts` around lines 63 - 85, The current logic in
resolveAccentForegroundColor only picks the better of white and "#111827"
without ensuring that choice meets an accessibility contrast threshold; update
resolveAccentForegroundColor to compute both contrast ratios (whiteContrast and
darkContrast), determine the best choice and then compare that best ratio
against a defined WCAG threshold (e.g., 4.5 for normal text); if the best ratio
is below the threshold return a sentinel (null or undefined) instead of a color.
Then update applyAccentColorToDocument to call normalizeAccentColor and
resolveAccentForegroundColor and, if the resolver returns no valid foreground
(or the best contrast is below threshold), avoid setting the --accent-color and
--accent-color-foreground variables (and optionally log or fallback to a safe
default accent), ensuring primary text color is never made unreadable.

apps/server/src/provider/Layers/KiloAdapter.ts-135-149 (1)

135-149: ⚠️ Potential issue | 🟠 Major

Forward numTurns to the manager.

Line 147 validates numTurns and then drops it. rollbackThread(threadId, 5) currently delegates the same call as rollbackThread(threadId, 1).

🐛 Minimal fix

-            try: () => manager.rollbackThread(threadId),
+            try: () => manager.rollbackThread(threadId, numTurns),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/KiloAdapter.ts` around lines 135 - 149, The
rollbackThread implementation validates numTurns but never forwards it to the
manager; change the Effect.tryPromise call in rollbackThread to call
manager.rollbackThread(threadId, numTurns) (keeping the existing validation,
ProviderAdapterValidationError usage, and the toRequestError catch wrapper) so
the manager receives the requested number of turns instead of always using the
default.

apps/server/src/provider/Layers/OpenCodeAdapter.ts-135-149 (1)

135-149: ⚠️ Potential issue | 🟠 Major

Pass the validated rollback count through.

Line 147 ignores numTurns after validating it, so multi-turn rollback requests collapse to the manager's default behavior.

🐛 Minimal fix

-            try: () => manager.rollbackThread(threadId),
+            try: () => manager.rollbackThread(threadId, numTurns),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/OpenCodeAdapter.ts` around lines 135 - 149,
rollbackThread validates numTurns but never passes it to the manager, causing
multi-turn rollbacks to be ignored; update the rollbackThread implementation to
call manager.rollbackThread(threadId, numTurns) (and propagate the same args
into the Effect.tryPromise try/catch) so the validated rollback count is
actually used; keep existing error handling with toRequestError(threadId,
"session/revert", cause) and ProviderAdapterValidationError/PROVIDER unchanged.

apps/server/src/provider/Layers/GeminiCliAdapter.ts-135-149 (1)

135-149: ⚠️ Potential issue | 🟠 Major

rollbackThread currently ignores its count parameter.

Line 147 drops numTurns, so callers cannot request a rollback depth larger than the manager default.

🐛 Minimal fix

-            try: () => manager.rollbackThread(threadId),
+            try: () => manager.rollbackThread(threadId, numTurns),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/GeminiCliAdapter.ts` around lines 135 - 149,
The rollbackThread implementation validates numTurns but then ignores it when
calling manager.rollbackThread(threadId); update the call to pass the validated
count (e.g., manager.rollbackThread(threadId, numTurns)) so the manager uses the
requested rollback depth, keeping the existing Effect.tryPromise/catch and
toRequestError(threadId, "session/revert", cause) behavior intact.

apps/server/src/opencodeServerManager.ts-39-43 (1)

39-43: ⚠️ Potential issue | 🟠 Major

Scope the cached server by connection options.

providerOptions are passed per session, but ensureServer() memoizes a single SharedServerState for the whole manager. The first thread that starts wins, and later threads silently reuse its serverUrl/auth/binary path, which can route sessions to the wrong backend.

Also applies to: 1183-1200, 1270-1285

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/opencodeServerManager.ts` around lines 39 - 43, The manager
currently memoizes a single SharedServerState in ensureServer(), causing
different sessions to share the first server's serverUrl/auth/binary path;
change ensureServer() to cache per-connection options by including
providerOptions (specifically OpenCode providerOptions within
OpenCodeSessionStartInput) in the cache key (e.g., maintain a Map keyed by a
stable serialization of providerOptions or relevant fields) so each distinct set
of providerOptions produces its own SharedServerState; update any other places
that rely on the single memo (see the same pattern referenced around lines
1183-1200 and 1270-1285) to use the per-options cache lookup/creation instead of
a single shared instance.

apps/server/src/opencodeServerManager.ts-602-611 (1)

602-611: ⚠️ Potential issue | 🟠 Major

Keep cancel distinct from decline.

The shared decision enum and the UI expose both Decline and Cancel turn, but this manager collapses both to reply: "reject". That removes any behavioral distinction for OpenCode approval prompts.

Also applies to: 1039-1051

apps/web/src/components/ChatView.tsx-886-888 (1)

886-888: ⚠️ Potential issue | 🟠 Major

Avoid eager provider discovery on every chat mount.

These queries run unconditionally, and providerListModelsQueryOptions() immediately calls api.provider.listModels(). For providers like OpenCode, model discovery can probe or spawn a local server, so opening a chat tab now starts unrelated provider work in the background even if the picker is never opened.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ChatView.tsx` around lines 886 - 888, The three
queries (copilotModelsQuery, opencodeModelsQuery, kiloModelsQuery) call
providerListModelsQueryOptions(), which immediately invokes
api.provider.listModels() and causes eager provider discovery on every ChatView
mount; change them to be lazy by passing an enabled flag or deferring creation
until the model picker is opened so api.provider.listModels() only runs when
needed. Update the hooks that create copilotModelsQuery, opencodeModelsQuery and
kiloModelsQuery to use useQuery(..., { enabled: isPickerOpen || isManualTrigger
}) or construct the query options only when the picker is opened/triggered,
referencing providerListModelsQueryOptions and api.provider.listModels so
discovery is gated. Ensure any UI that expects these queries handles the
disabled state and triggers a refetch when the picker opens.

apps/web/src/components/ChatView.tsx-5342-5343 (1)

5342-5343: ⚠️ Potential issue | 🟠 Major

Don't suppress diff-only assistant turns.

This early return runs before the changed-files block below is computed. Any assistant message with empty text but a valid turnDiffSummaryByAssistantMessageId entry will disappear completely, taking its diff summary with it.

💡 Only hide rows that truly have nothing to render

-          if (!row.message.text && !row.message.streaming) return null;
-          const messageText = row.message.text || "";
+          const turnSummary = turnDiffSummaryByAssistantMessageId.get(row.message.id);
+          if (!row.message.text && !row.message.streaming && !turnSummary) return null;
+          const messageText = row.message.text || "";
@@
-                  const turnSummary = turnDiffSummaryByAssistantMessageId.get(row.message.id);
                   if (!turnSummary) return null;

Also applies to: 5362-5415

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/ChatView.tsx` around lines 5342 - 5343, The current
early return in ChatView that checks if (!row.message.text &&
!row.message.streaming) must be tightened so assistant turns that have no text
but do have a diff summary are not dropped: update the condition to also check
the diff-summary map (turnDiffSummaryByAssistantMessageId) for an entry keyed by
the assistant message id (e.g., row.message.id or the specific
assistantMessageId used elsewhere) and only return null when text is empty,
streaming is false, and there is no diff summary for that message; adjust the
render logic that computes the "changed-files" block to rely on the same key so
valid diff-only assistant messages remain rendered.

apps/server/src/opencodeServerManager.ts-504-507 (1)

504-507: ⚠️ Potential issue | 🟠 Major

Read workspace back out of resumeCursor.

Lines 833-836 persist both sessionId and workspace, but reattach only reads sessionId. A resumed thread that omits providerOptions.workspace will call session.get() against the default workspace and fail to reconnect to workspace-scoped sessions.

💡 Minimal fix

-function readResumeSessionId(resumeCursor: unknown): string | undefined {
-  const record = asRecord(resumeCursor);
-  return asString(record?.sessionId);
+function readResumeSessionInfo(resumeCursor: unknown): {
+  readonly sessionId?: string;
+  readonly workspace?: string;
+} {
+  const record = asRecord(resumeCursor);
+  return {
+    sessionId: asString(record?.sessionId),
+    workspace: asString(record?.workspace),
+  };
 }
@@
-    const workspace = options?.workspace;
+    const resume = readResumeSessionInfo(openCodeInput.resumeCursor);
+    const workspace = options?.workspace ?? resume.workspace;
@@
-    const resumedSessionId = readResumeSessionId(openCodeInput.resumeCursor);
+    const resumedSessionId = resume.sessionId;

Also applies to: 783-809, 833-836

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/opencodeServerManager.ts` around lines 504 - 507,
readResumeSessionId only extracts sessionId from resumeCursor but the resume
flow also persists workspace; update the resume parsing to also read workspace
(use asString(record?.workspace)) and return both values (e.g., change
readResumeSessionId to return { sessionId?: string, workspace?: string } or add
a new readResumeWorkspace function), then update callers (including the reattach
logic that calls session.get() and any places handling
providerOptions.workspace) to use the parsed workspace when reconnecting so
workspace-scoped sessions reconnect correctly. Ensure you update function
signature and all call sites (e.g., reattach/resume handlers referenced around
readResumeSessionId and session.get()) to accept and pass the workspace.

apps/server/src/opencodeServerManager.ts-900-965 (1)

900-965: ⚠️ Potential issue | 🟠 Major

Don't silently drop attachments on OpenCode turns.

The shared turn contract already supports attachments, and the web composer sends them, but this path always forwards a single text part. Image/file turns will look sent in the UI and arrive as text-only prompts here.

🛡️ Minimal guard until attachment parts are implemented

   const variant =
     openCodeInput.modelOptions?.opencode?.variant ??
     openCodeInput.modelOptions?.opencode?.reasoningEffort ??
     parsedModel?.variant;
   const startedAt = nowIso();
+
+  if ((openCodeInput.attachments?.length ?? 0) > 0) {
+    throw new Error("OpenCode attachments are not supported yet.");
+  }
 
   context.activeTurnId = turnId;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/opencodeServerManager.ts` around lines 900 - 965, The
sendTurn function currently builds parts as a single textPart (parts:
[textPart(openCodeInput.input ?? "")]) which silently drops any attachments;
change the parts construction so it forwards the incoming attachment/part data
instead of overwriting it: locate sendTurn / openCodeInput and replace the
static parts array with one derived from the incoming input (e.g. use
openCodeInput.parts if present, otherwise build parts by including a textPart
for openCodeInput.input when present plus mapped attachment parts from
openCodeInput.attachments), and pass that parts array into
context.client.session.promptAsync so attachments are preserved and not dropped.

apps/server/src/provider/Layers/CopilotAdapter.ts-527-560 (1)

527-560: ⚠️ Potential issue | 🟠 Major

Terminal turn bookkeeping is incomplete.

sendTurn() queues an orchestration turnId immediately, but terminal handling only clears currentTurnId/currentProviderTurnId. If Copilot terminates a turn before assistant.turn_start, that stale ID stays in pendingTurnIds, session.idle can still emit turn.completed, and the next assistant.turn_start can be matched to the wrong turn.

Also applies to: 1052-1080, 1300-1303

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/CopilotAdapter.ts` around lines 527 - 560,
The terminal handling in the CopilotAdapter's event cases (e.g., the
"session.idle" branch) only clears currentTurnId/currentProviderTurnId but
leaves the corresponding orchestration turn queued in pendingTurnIds, causing
stale IDs to persist and mis-match future assistant.turn_start; update the
terminal handlers (including the "session.idle" case and the other terminal
ranges noted) to also remove/advance the corresponding entry from pendingTurnIds
(and any related mappings) when clearing currentTurnId/currentProviderTurnId so
that sendTurn()'s queued orchestration turn is consumed or discarded
consistently and no stale turnId remains.

apps/server/src/provider/Layers/ClaudeCodeAdapter.ts-1509-1513 (1)

1509-1513: ⚠️ Potential issue | 🟠 Major

Guard startSession() against duplicate threadIds.

This path always creates a new query and then overwrites the existing sessions entry without stopping it first. A retry on the same thread will orphan the old Claude stream/process, and its events can still leak into the runtime with no remaining handle to stop it.

Also applies to: 1728-1729

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/ClaudeCodeAdapter.ts` around lines 1509 -
1513, The startSession flow (using readClaudeResumeState and the derived
threadId) currently creates a new PromptQueue and then unconditionally
overwrites the sessions entry, which can orphan an existing Claude stream;
update startSession (and the analogous block around the other occurrence at the
lines noted) to guard against duplicate threadId by checking
sessions.get(threadId) before creating a new session: if an active session
exists, either return that existing session/handle or explicitly stop/cleanup
the old session (call its shutdown/stop/abort method or clear its promptQueue
and stream) before replacing it; ensure you reference the threadId derived from
readClaudeResumeState(input.resumeCursor) and handle both branches (resume vs
new) so no orphaned streams remain.

apps/server/src/provider/Layers/ClaudeCodeAdapter.ts-393-411 (1)

393-411: ⚠️ Potential issue | 🟠 Major

Don't silently drop attachments.

Images are reduced to a text note, every non-image attachment is ignored, and a file-only turn can collapse to an empty text message. The turn still succeeds, but Claude never receives the attached data. If this adapter can't forward attachments, it should reject unsupported attachments instead of degrading silently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/ClaudeCodeAdapter.ts` around lines 393 - 411,
The adapter currently converts attachments into plain text notes and silently
drops non-image files; update the method in ClaudeCodeAdapter that builds the
SDKUserMessage (the block iterating over input.attachments and returning the
message) to validate attachments up front: if any attachment.type is not "image"
reject/throw an error (do not degrade silently), and if there are attachments
but the resulting text would be empty also reject/throw so we don't send an
empty turn; keep the existing behavior for images (produce the descriptive text
fragments) but ensure unsupported attachments cause the call to fail instead of
being ignored.

apps/server/src/provider/Layers/ClaudeCodeAdapter.ts-1719-1725 (1)

1719-1725: ⚠️ Potential issue | 🟠 Major

rollbackThread() only edits local bookkeeping.

context.turns is reinitialized to [] on resume, and rollbackThread() only splices that local array before rewriting the cursor. The live Claude session still retains the removed turns, and lastAssistantUuid keeps pointing at the discarded tip, so the next send/resume continues from the supposedly rolled-back history.

Safer interim fix

-    const rollbackThread: ClaudeCodeAdapterShape["rollbackThread"] = (threadId, numTurns) =>
+    const rollbackThread: ClaudeCodeAdapterShape["rollbackThread"] = (threadId, _numTurns) =>
       Effect.gen(function* () {
-        const context = yield* requireSession(threadId);
-        const nextLength = Math.max(0, context.turns.length - numTurns);
-        context.turns.splice(nextLength);
-        yield* updateResumeCursor(context);
-        return yield* snapshotThread(context);
+        yield* requireSession(threadId);
+        return yield* new ProviderAdapterRequestError({
+          provider: PROVIDER,
+          method: "thread.rollback",
+          detail: "Claude Code rollback is not supported without rewinding the underlying provider session.",
+        });
       });

Also applies to: 1866-1873

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/ClaudeCodeAdapter.ts` around lines 1719 -
1725, rollbackThread currently only mutates the local context.turns array and
cursor, leaving the remote Claude session and lastAssistantUuid pointing at the
discarded tip; update rollbackThread to also remove the corresponding turns from
the live Claude session (via whatever session API/delete or edit method your
adapter uses) and set lastAssistantUuid to the new tip's uuid (not
resumeState?.resumeSessionAt), and ensure resume logic does not blindly set
turns: [] but instead syncs context.turns with the live session state after
resume; touch symbols: rollbackThread(), context.turns, lastAssistantUuid, and
the resume initialization that reads resumeState?.resume/resumeSessionAt so the
in-memory and remote session are consistent.

apps/server/src/provider/Layers/CopilotAdapter.ts-1091-1110 (1)

1091-1110: ⚠️ Potential issue | 🟠 Major

Emit resolution/exit events when a Copilot session is torn down.

stopRecord() resolves pending approvals and user-input promises, but it never emits request.resolved, user-input.resolved, or session.exited. Stream consumers can end up with dangling modal state and no canonical signal that the session actually ended.

Also applies to: 1416-1431

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/CopilotAdapter.ts` around lines 1091 - 1110,
stopRecord currently resolves pendingApprovalResolvers and
pendingUserInputResolvers but does not notify stream consumers; update
stopRecord (and the analogous teardown block around the 1416-1431 region) to
emit the proper resolution/exit events after resolving each pending: emit
"request.resolved" for each approval resolved, "user-input.resolved" for each
user-input resolver, and emit a single "session.exited" for the
ActiveCopilotSession when teardown finishes (do this before calling
sessions.delete(record.threadId)); make these emissions on the session/event
emitter associated with the ActiveCopilotSession so consumers receive canonical
end signals.

apps/server/src/provider/Layers/CopilotAdapter.ts-415-420 (1)

415-420: ⚠️ Potential issue | 🟠 Major

Add layer cleanup for active Copilot sessions and the event queue.

This layer creates an unbounded queue and maintains a sessions map but never registers a finalizer. When the scope releases (in tests, hot reloads, or layer replacement), these resources can leak. Other adapters in the same directory (CursorAdapter, ClaudeCodeAdapter) use Effect.addFinalizer to clean up sessions on scope release—apply the same pattern here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/CopilotAdapter.ts` around lines 415 - 420,
The makeCopilotAdapter layer creates runtimeEventQueue via Queue.unbounded and a
sessions Map (Map<ThreadId, ActiveCopilotSession>) but never registers a
finalizer; add an Effect.addFinalizer inside makeCopilotAdapter to run when the
scope releases that (1) iterates sessions.values() and invokes each
ActiveCopilotSession cleanup method (use the same method name used in
CursorAdapter/ClaudeCodeAdapter — e.g., session.close()/dispose()/stop()) to
terminate active sessions and clear the sessions Map, and (2) shuts down the
runtimeEventQueue by calling the Queue shutdown method (e.g.,
runtimeEventQueue.shutdown()/offerShutdown()) so both the sessions map and the
unbounded queue are cleaned up on scope release.

apps/server/src/provider/Layers/CopilotAdapter.ts-1266-1284 (1)

1266-1284: ⚠️ Potential issue | 🟠 Major

Handle attachment validation errors through Effect error channel.

Throwing ProviderAdapterRequestError from the synchronous .map() callback escapes the Effect context and becomes an unhandled defect instead of a normal session.send failure. Within this Effect.gen generator, use Effect.forEach and propagate the error with yield* to keep callers on the typed request-error path, consistent with other error handling in this function (see lines 1363 and 1395).

Suggested shape

-        const attachments = (input.attachments ?? [])
-          .map((attachment) => {
+        const attachments = yield* Effect.forEach(input.attachments ?? [], (attachment) =>
+          Effect.gen(function* () {
             const attachmentPath = resolveAttachmentPath({
               stateDir: serverConfig.stateDir,
               attachment,
             });
             if (!attachmentPath) {
-              throw new ProviderAdapterRequestError({
+              return yield* new ProviderAdapterRequestError({
                 provider: PROVIDER,
                 method: "session.send",
                 detail: `Invalid attachment id '${attachment.id}'.`,
               });
             }
             return {
               type: "file" as const,
               path: attachmentPath,
               displayName: attachment.name,
             };
-          });
+          }),
+        );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/CopilotAdapter.ts` around lines 1266 - 1284,
The synchronous throw inside the attachments .map escapes the surrounding
Effect.gen and causes an unhandled defect; replace the inline .map with an
Effect.forEach that resolves each attachment via resolveAttachmentPath and,
inside the generator, use yield* to run the Effect so any
ProviderAdapterRequestError is returned on the Effect error channel; update the
attachments construction in CopilotAdapter (the block that currently maps
attachments to {type: "file", path, displayName}) to use Effect.forEach and
yield* so invalid attachment ids produce a typed session.send failure instead of
a defect.

README.md-3-3 (1)

3-3: ⚠️ Potential issue | 🟡 Minor

Document Kilo support here too.

The PR adds Kilo alongside the other provider adapters, but this README’s support summary and agent list omit it. That leaves the user-facing docs out of sync with the feature set.

Also applies to: 20-28

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 3, Update the README to include Kilo in the support
summary and the agent/provider list: add "Kilo" to the sentence that currently
lists Codex, Claude Code, Cursor, Copilot, Gemini CLI, Amp, and OpenCode, and
also add Kilo to the agent list block referenced around lines 20-28 so the
documentation matches the new provider adapter added in the PR.

README.md-1-1 (1)

1-1: ⚠️ Potential issue | 🟡 Minor

Remove the leading spaces before these headings.

markdownlint is flagging every heading here with MD023, so the README will keep failing docs lint until the headings start at column 1.

Suggested fix

- # T3 Code
+# T3 Code
...
- ## Getting started
+## Getting started
...
- ### CLI
+### CLI
...
- ### Desktop app
+### Desktop app
...
- ## Supported agents
+## Supported agents
...
- ## Notes
+## Notes
...
- ## Need help?
+## Need help?

Also applies to: 5-5, 7-7, 16-16, 20-20, 30-30, 36-36

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 1, Several README headings have leading spaces causing
markdownlint MD023; update any lines that start with whitespace followed by
hashes (e.g., "  # T3 Code") so the first character of the line is the first "#"
character. Fix by trimming leading whitespace for all heading lines (pattern:
/^\s+#+/), and ensure exactly one space between the final "#" and the heading
text (e.g., "# T3 Code", "## ...") for the headings mentioned.

apps/server/src/provider/Layers/ProviderAdapterRegistry.ts-36-46 (1)

36-46: ⚠️ Potential issue | 🟡 Minor

Fail fast on duplicate provider registrations.

new Map(adapters.map(...)) silently keeps the last adapter for a provider. With a larger built-in set and customizable options.adapters, that can hide misconfiguration and route requests to the wrong backend. Validate uniqueness while building byProvider instead of overwriting implicitly.

🛠️ Suggested guard

-    const byProvider = new Map(adapters.map((adapter) => [adapter.provider, adapter]));
+    const byProvider = new Map();
+    for (const adapter of adapters) {
+      if (byProvider.has(adapter.provider)) {
+        throw new Error(`Duplicate provider adapter registered for "${adapter.provider}"`);
+      }
+      byProvider.set(adapter.provider, adapter);
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/ProviderAdapterRegistry.ts` around lines 36 -
46, The current creation of byProvider using new Map(adapters.map(...)) silently
overwrites duplicates; modify the code that builds byProvider (the adapters
array -> byProvider logic) to iterate adapters and check for duplicates: for
each adapter (use adapter.provider) if byProvider.has(adapter.provider) throw an
error (or otherwise fail fast) with a clear message identifying the duplicate
provider and the conflicting adapter entries; otherwise set the map entry. This
ensures ProviderAdapterRegistry (the adapters array and byProvider) validates
uniqueness instead of keeping the last registration.

scripts/sync-upstream-pr-tracks.mjs-96-97 (1)

96-97: ⚠️ Potential issue | 🟡 Minor

Hardcoded repository URL should use configuration.

The PR URL is hardcoded to pingdotgg/t3code, but the script reads upstreamRemote and forkRemote from configuration. Consider deriving the URL from the git remote or adding a repoUrl field to the config.

🔧 Suggested fix

In loadConfig(), add repo URL extraction or config field:

 function loadConfig() {
   const raw = fs.readFileSync(configPath, "utf8");
   const parsed = JSON.parse(raw);
   if (!parsed || typeof parsed !== "object") {
     throw new Error("Invalid upstream PR tracking config.");
   }
   if (!Array.isArray(parsed.trackedPrs) || parsed.trackedPrs.length === 0) {
     throw new Error("No tracked PRs configured.");
   }
+  // Derive repo URL from git remote or require in config
+  if (!parsed.repoUrl) {
+    const remoteUrl = tryRunGit(["remote", "get-url", parsed.upstreamRemote]);
+    parsed.repoUrl = remoteUrl?.replace(/\.git$/, "").replace(/^git@github\.com:/, "https://github.com/") ?? "https://github.com/pingdotgg/t3code";
+  }
   return parsed;
 }

Then update line 97:

-    console.log(`URL: https://github.com/pingdotgg/t3code/pull/${pr.number}`);
+    console.log(`URL: ${config.repoUrl}/pull/${pr.number}`);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/sync-upstream-pr-tracks.mjs` around lines 96 - 97, The PR URL is
hardcoded to pingdotgg/t3code; update script to derive the repo host/owner/name
from configuration instead: add a repoUrl (or owner/repo) field when reading
config in loadConfig() (or parse upstreamRemote/forkRemote) and then build the
URL using that value instead of the literal string in the console.log call that
uses pr.number; locate usages around formatSection and the console.log with
pr.number/pr.title and replace the hardcoded URL construction with the derived
repoUrl.

apps/server/src/geminiCliServerManager.test.ts-97-108 (1)

97-108: ⚠️ Potential issue | 🟡 Minor

This test never proves lazy spawning.

Lines 106-108 only confirm that a ready session exists. A regression that eagerly calls spawn during startSession would still pass here unless the test asserts on spawn directly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/geminiCliServerManager.test.ts` around lines 97 - 108, The
test currently doesn't prove lazy spawning because it only checks session
readiness; update the test for GeminiCliServerManager to assert that spawn is
not invoked during startSession by either spying/mocking the spawn method on
GeminiCliServerManager (or the underlying process launcher) and expecting it not
to have been called, or by checking that no child/process entry exists before
calling sendTurn; then verify that spawn is called (or a process appears) after
sendTurn is invoked to confirm lazy per-turn spawning.

apps/web/src/appSettings.ts-161-165 (1)

161-165: ⚠️ Potential issue | 🟡 Minor

Drop invalid provider overrides instead of normalizing them to blue.

Line 164 uses normalizeAccentColor, which substitutes DEFAULT_ACCENT_COLOR for any non-empty invalid value. That turns a bad/stale provider override into a sticky #2563eb override instead of letting the provider inherit the global accent again.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/appSettings.ts` around lines 161 - 165, The current mapping in
providerAccentColors uses normalizeAccentColor which replaces
invalid-but-nonempty values with DEFAULT_ACCENT_COLOR causing stale overrides to
stick; instead validate each value from settings.providerAccentColors and drop
entries that are not valid accents so providers inherit the global accent.
Update the logic around providerAccentColors to call a validation helper (or add
an isValidAccentColor check) and only map entries where isValidAccentColor(v) is
true, then apply normalizeAccentColor(v) (or keep as-is) for those valid
entries, removing any reference that forces DEFAULT_ACCENT_COLOR for invalid
inputs.

apps/server/src/geminiCliServerManager.test.ts-552-563 (1)

552-563: ⚠️ Potential issue | 🟡 Minor

Make the live Gemini suite explicitly opt-in.

The current guard only proves that the gemini binary exists. On machines with the CLI installed but no auth/config/network, this suite will still run and fail unexpectedly.

♻️ Safer gate

-const hasGemini = await (async () => {
-  try {
-    const { execSync } = await import("node:child_process");
-    execSync("gemini --version", { stdio: "pipe" });
-    return true;
-  } catch {
-    return false;
-  }
-})();
+const hasGemini =
+  process.env.RUN_GEMINI_LIVE_TESTS === "1" &&
+  (await (async () => {
+    try {
+      const { execSync } = await import("node:child_process");
+      execSync("gemini --version", { stdio: "pipe" });
+      return true;
+    } catch {
+      return false;
+    }
+  })());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/geminiCliServerManager.test.ts` around lines 552 - 563, The
test guard currently uses only hasGemini (which checks for the binary) and
should require an explicit opt-in; update the condition used with
describe.skipIf so the suite runs only when both hasGemini is true and an
explicit env flag (e.g., process.env.RUN_GEMINI_LIVE === '1' or 'true') is
present — change the hasGemini computation or the describe.skipIf argument to
require the env var alongside hasGemini (referencing hasGemini and
describe.skipIf and the "GeminiCliServerManager live integration" suite) so the
integration tests are opt-in rather than running whenever the CLI binary exists.

apps/server/src/provider/Layers/AmpAdapter.ts-135-150 (1)

135-150: ⚠️ Potential issue | 🟡 Minor

Validated numTurns parameter is not passed to the manager.

The numTurns parameter is validated (must be integer >= 1) but never passed to manager.rollbackThread(). The manager's rollbackThread method doesn't accept this parameter either. If rollback functionality is intended to respect numTurns, both the adapter and manager need updating.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/AmpAdapter.ts` around lines 135 - 150, The
adapter validates numTurns but never forwards it; update the rollback flow to
accept and propagate numTurns: change AmpAdapter.rollbackThread to call
manager.rollbackThread(threadId, numTurns) instead of
manager.rollbackThread(threadId), and update the manager.rollbackThread method
signature/implementation to accept (threadId, numTurns) and honor numTurns when
reverting turns (adjust types, callers, and tests accordingly); keep the
existing validation (ProviderAdapterValidationError) and preserve error mapping
via toRequestError.

apps/server/src/ampServerManager.ts-738-742 (1)

738-742: ⚠️ Potential issue | 🟡 Minor

Content deltas receive unique itemIds, preventing event aggregation.

For content.delta events, a new random itemId is generated each time if not explicitly provided. This means consecutive text deltas for the same assistant message will have different itemIds, which may complicate downstream aggregation or rendering logic that groups events by itemId.

Consider tracking a stable itemId for assistant messages across deltas (similar to how GeminiCliServerManager uses activeAssistantItemId).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/ampServerManager.ts` around lines 738 - 742, The current
logic generates a new random itemId for partial.type === "content.delta" when
itemId isn't provided, causing deltas for the same assistant message to get
different ids; add a stable activeAssistantItemId field to AmpServerManager
(like GeminiCliServerManager's activeAssistantItemId), set it when an assistant
message is created/started (e.g., on message.create or the first chunk) and then
use that activeAssistantItemId instead of RuntimeItemId.makeUnsafe(randomUUID())
for subsequent content.delta events when itemId is absent, and clear/reset the
activeAssistantItemId when the message completes or is finalized.

apps/server/src/geminiCliServerManager.ts-385-400 (1)

385-400: ⚠️ Potential issue | 🟡 Minor

Hardcoded runtimeMode in listSessions ignores session's actual mode.

Line 391 uses hardcoded "full-access" instead of session.runtimeMode. This discards the actual runtime mode that was set during session creation.

🐛 Fix

     for (const session of this.sessions.values()) {
       sessions.push({
         provider: PROVIDER,
         status: session.status === "closed" ? "closed" : "ready",
-        runtimeMode: "full-access",
+        runtimeMode: session.runtimeMode as ProviderSession["runtimeMode"],
         threadId: session.threadId,
         cwd: session.cwd,
         model: session.model,
         createdAt: session.createdAt,
         updatedAt: session.updatedAt,
       });
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/geminiCliServerManager.ts` around lines 385 - 400, The
listSessions() method is hardcoding runtimeMode to "full-access" which discards
the session's actual mode; update listSessions() to set runtimeMode from the
stored session (use session.runtimeMode) when building each ProviderSession
object so the returned array reflects the real mode for each session (refer to
listSessions, this.sessions, ProviderSession, and session.runtimeMode).

apps/server/src/kiloServerManager.ts-1202-1245 (1)

1202-1245: ⚠️ Potential issue | 🟡 Minor

Process cleanup on timeout may leave orphaned child process.

When the server start times out (line 1203-1209), the promise rejects but the spawned child process isn't explicitly killed. This could leave orphaned processes if the Kilo server takes longer than SERVER_START_TIMEOUT_MS to start.

🐛 Suggested fix

       const startedBaseUrl = await new Promise<string>((resolve, reject) => {
+        let resolved = false;
         const timeout = setTimeout(() => {
+          if (!resolved) {
+            child.kill();
+          }
           reject(
             new Error(
               `Timed out waiting for Kilo server to start after ${SERVER_START_TIMEOUT_MS}ms`,
             ),
           );
         }, SERVER_START_TIMEOUT_MS);
         let output = "";

         const onChunk = (chunk: Buffer) => {
           output += chunk.toString();
           const url = parseServerUrl(output);
           if (!url) {
             return;
           }
           clearTimeout(timeout);
+          resolved = true;
           resolve(url);
         };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/kiloServerManager.ts` around lines 1202 - 1245, The startup
promise (startedBaseUrl) currently rejects on timeout but does not terminate the
spawned child process, which can leave an orphan; update the timeout handler in
the Promise that uses SERVER_START_TIMEOUT_MS so it explicitly kills the child
process (e.g., call child.kill() or child.kill('SIGKILL') as appropriate),
remove/cleanup the stdout/stderr/error/exit listeners, then reject with the
timeout error; also ensure other rejection paths (child.once('error') and the
exit path where probeServer determines failure) similarly perform child cleanup
and listener removal before rejecting to avoid leaked processes.

apps/server/src/provider/claude-agent-sdk.d.ts (1)

63-88: Consider using unknown instead of any for consistency.

The SDKMessage interface uses any in several places (lines 72, 78, 82, 87) while other interfaces like SDKResultMessage use unknown for similar index signatures. Using unknown provides better type safety by requiring explicit type narrowing before use.

♻️ Suggested change for stricter typing

   export interface SDKMessage {
     readonly type?: string;
     readonly subtype?: string;
     readonly role?: string;
     readonly message?: {
       readonly id?: string;
       readonly content?: ReadonlyArray<unknown>;
       readonly [key: string]: unknown;
     };
-    readonly content?: ReadonlyArray<Record<string, any>>;
+    readonly content?: ReadonlyArray<Record<string, unknown>>;
     readonly uuid?: string;
     readonly session_id?: string;
     readonly parent_tool_use_id?: string;
     readonly tool_use_id?: string;
     readonly tool_name?: string;
-    readonly input?: Record<string, any>;
+    readonly input?: Record<string, unknown>;
     readonly result?: string;
     readonly error?: string;
     readonly errors?: ReadonlyArray<unknown>;
-    readonly content_block?: Record<string, any>;
+    readonly content_block?: Record<string, unknown>;
     readonly index?: number;
     readonly preceding_tool_use_ids?: ReadonlyArray<string>;
     readonly is_error?: boolean;
     readonly suggestions?: ReadonlyArray<PermissionUpdate>;
-    readonly [key: string]: any;
+    readonly [key: string]: unknown;
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/claude-agent-sdk.d.ts` around lines 63 - 88, The
SDKMessage interface uses permissive any types; change all occurrences to
unknown for stricter typing: replace Record<string, any> in the fields content,
input, and content_block with Record<string, unknown>, and replace the index
signature [key: string]: any with [key: string]: unknown (and any other any
usages inside SDKMessage) so callers must narrow types before use; update
SDKMessage accordingly to mirror the stricter pattern used in SDKResultMessage.

apps/server/src/kiloServerManager.ts (2)

1813-1815: Type cast through unknown may hide type mismatches.

The as unknown as ProviderRuntimeEvent double cast bypasses TypeScript's type checking. Since KiloProviderRuntimeEvent is designed to extend ProviderRuntimeEvent with "kilo" as an additional provider value, consider refining the type relationship or adding a runtime assertion in development to catch mismatches.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/kiloServerManager.ts` around lines 1813 - 1815, The
double-cast from KiloProviderRuntimeEvent to ProviderRuntimeEvent in
emitRuntimeEvent hides type errors; update the types or add a runtime check
instead of using "as unknown as". Either make KiloProviderRuntimeEvent
explicitly extend ProviderRuntimeEvent so the cast is unnecessary, change the
emit signature to accept KiloProviderRuntimeEvent, or add a development-only
type guard/assertion inside emitRuntimeEvent that validates the event shape
before calling this.emit("event", event) to fail fast on mismatches (refer to
emitRuntimeEvent, KiloProviderRuntimeEvent, and ProviderRuntimeEvent).

---

701-703: Remove or document the purpose of this pass-through function.

readJsonData simply returns the promise unchanged. This appears to be dead code or a placeholder. If it was intended for response parsing or transformation, that logic is missing. Otherwise, consider removing it and calling the promises directly.

♻️ If the function has no purpose, remove it and replace usages

-async function readJsonData<T>(promise: Promise<T>): Promise<T> {
-  return promise;
-}

Then replace all readJsonData(client.session.get(...)) calls with direct client.session.get(...) calls.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/kiloServerManager.ts` around lines 701 - 703, The helper
readJsonData currently just returns its input promise and is effectively dead
code; either remove this pass-through and replace all calls like
readJsonData(client.session.get(...)) with direct client.session.get(...) (or
await the promise where appropriate), or implement the intended
parsing/transformation inside readJsonData (e.g., await the response,
validate/parse JSON and return typed T) and update callsites to await the
function. Locate usages by the symbol readJsonData and the call pattern
readJsonData(client.session.get(...)) and apply the chosen fix consistently.

.plans/17-claude-code.md (3)

118-125: Expand security guidance for credential handling.

The credential management section correctly delegates authentication to provider-native mechanisms, but should explicitly address security concerns that span multiple providers:

1. Preventing credential leakage - Ensure error messages, logs, and debug output never expose API keys or tokens
2. Secure environment propagation - When spawning child processes for CLI-based providers, explicitly document which environment variables are inherited and how to sanitize the environment
3. Secret rotation - Note that credential rotation requires restarting affected provider sessions
4. Audit logging - Consider logging authentication attempts (success/failure) without logging the credentials themselves

📋 Suggested additions to credential management section

 ### 2.1.b Credential management and resource limits

 Each provider manages its own authentication externally:

 1. **Environment variables and CLI auth** -- Credentials are resolved via provider-native mechanisms (e.g. `ANTHROPIC_API_KEY` for Claude, `OPENAI_API_KEY` for Codex, `gh auth` for Copilot). The adapter layer never stores or brokers credentials directly; it relies on the underlying CLI/SDK picking them up from the environment.
+   - **Security**: Adapters must sanitize error messages and logs to prevent credential leakage. When spawning child processes, explicitly allow-list environment variables rather than inheriting the full parent environment.
+   - **Rotation**: Credential rotation requires stopping and restarting affected provider sessions. Document the restart procedure for operators.
 2. **Per-provider rate limiting** -- Each server manager (`codexAppServerManager`, `claudeCodeServerManager`, etc.) is responsible for honoring its provider's rate limits. Adapters should surface rate-limit errors as `ProviderAdapterProcessError` so orchestration can report them cleanly.
 3. **Concurrent session limits** -- The number of simultaneous provider sessions is bounded by system resources (open processes, file descriptors, memory). `ProviderSessionDirectory` tracks active sessions but does not enforce hard caps; operators should monitor resource usage when running multiple providers concurrently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.plans/17-claude-code.md around lines 118 - 125, Update the "Credential
management and resource limits" section to explicitly cover cross-provider
security: add guidance to prevent credential leakage by ensuring error messages,
logs, and debug output never print or interpolate API keys/tokens (sanitize
messages in adapters and providers like codexAppServerManager and
claudeCodeServerManager); document secure environment propagation when spawning
child processes (use an explicit env whitelist/denylist and sanitize inherited
variables rather than inheriting process.env blindly in adapter launch code);
note that secret rotation requires restarting affected provider sessions and
reference ProviderSessionDirectory as the registry of active sessions that must
be restarted; and recommend audit logging of authentication attempts (record
success/failure, timestamp, provider identifier) without storing the credentials
themselves and surface rate-limit/auth errors as ProviderAdapterProcessError for
orchestration to report.

---

429-437: Consider adding performance and chaos testing scenarios.

The multi-provider test scenarios (lines 429-437) provide excellent coverage of correctness and isolation. To further harden the multi-provider system, consider adding:

1. Performance under load - Simulate multiple concurrent sessions across different providers with realistic turn rates to identify resource bottlenecks
2. Graceful degradation - Verify that when one provider is unavailable, other providers continue to function normally
3. Resume under failure - Test resume behavior after ungraceful shutdowns (killed processes, network interruptions)

🧪 Suggested additional test scenarios

5. **Load and performance testing** -- Run 10+ concurrent sessions across multiple providers with realistic turn rates (1 turn every 5-30 seconds). Monitor memory usage, file descriptor counts, and response latency to identify resource exhaustion or contention issues.
6. **Chaos scenarios** -- Simulate provider binary crashes, network timeouts, and disk full conditions. Verify that the orchestration layer surfaces clear errors and that unaffected providers continue operating.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.plans/17-claude-code.md around lines 429 - 437, Add performance and chaos
testing to the "6.5 Multi-provider test scenarios" section: extend the list to
include "Performance under load" (simulate 10+ concurrent sessions across
providers with realistic turn rates, monitor memory, FD counts, and latency) and
"Chaos scenarios / Graceful degradation" (simulate provider crashes, network
timeouts, disk-full, and verify listProviderStatuses()/orchestration surfaces
errors and other providers continue functioning), and also add "Resume under
failure" to validate resume cursor behavior after ungraceful shutdowns; update
the numbered list and the suggested additional test scenarios block so these
cases are described with expected monitors and pass/fail criteria.

---

316-342: Add decision criteria for rollback strategy selection.

Phase 4 presents two rollback options (provider-native rewind vs. session restart) but doesn't specify how to choose between them or when the decision should be made. This ambiguity could delay implementation.

🎯 Suggested decision framework

Add a subsection after line 319:

### 4.0 Decision criteria

Choose rollback strategy during adapter implementation based on SDK capabilities:

1. **If Claude SDK exposes a rewind/undo API** → use Option A (provider-native rewind)
   - Verify: SDK documentation confirms conversation history can be truncated
   - Verify: Rewind operation is atomic and does not corrupt session state
   
2. **If no native rewind exists** → use Option B (session restart)
   - Document: Users will see "conversation reset to checkpoint" behavior
   - Document: Performance implications of session recreation
   
3. **Fallback**: If investigation timeline exceeds 2 days, proceed with Option B and file a follow-up issue to migrate to Option A if native rewind becomes available.

**Decision deadline**: Before starting Phase 4 implementation (after Phase 3 is complete).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.plans/17-claude-code.md around lines 316 - 342, Add a "Decision criteria"
subsection after the Phase 4 description that specifies how to choose between
provider-native rewind and session-restart: state that adapter implementers must
check the Claude SDK for a rewind/undo API and atomicity (if present, implement
provider-native rewind and keep CheckpointReactor unchanged), otherwise
implement the session-restart + state-truncation shim in the Claude adapter
(stop session, rewrite resume cursor, force session recreation) and document
user-visible behavior; include a fallback policy (if investigation >2 days,
default to session-restart and file an issue to migrate), require the decision
before Phase 4 work begins, and ensure providerService.rollbackConversation()
behavior is documented and deterministic for checkpoint revert tests.

apps/server/src/provider/Layers/ClaudeCodeAdapter.ts (1)

64-99: Module-level mutable state accumulates usage across all sessions.

The _claudeUsageAccumulator aggregates usage data globally across all Claude Code sessions. This design means:

1. fetchClaudeCodeUsage() returns combined totals from all sessions
2. State persists across session restarts
3. No way to reset or separate per-session usage

If per-session usage tracking is needed, consider moving this state into ClaudeSessionContext and aggregating on demand.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/ClaudeCodeAdapter.ts` around lines 64 - 99,
The module currently uses a module-level mutable _claudeUsageAccumulator with
accumulateClaudeUsage and storeClaudeRateLimits, causing global aggregation
across sessions; move this state into the per-session ClaudeSessionContext (add
a usage field and methods like addClaudeUsage and setClaudeRateLimits on the
context), update accumulateClaudeUsage and storeClaudeRateLimits to operate on
the session's context instead of the module variable, ensure
fetchClaudeCodeUsage reads/aggregates from the session context (or from all
session contexts only when intentional), and reset the session usage on session
restart/close so usage is isolated per session.

apps/server/src/provider/Layers/ProviderAdapterRegistry.ts (1)

48-55: Consider using Effect.die for idiomatic Effect error handling.

Using throw inside Effect.gen produces a Cause.Die (defect), which is appropriate for invariant violations. However, Effect.die() is more idiomatic and makes the intent explicit.

♻️ Suggested idiomatic Effect approach

     for (const adapter of adapters) {
       if (byProvider.has(adapter.provider)) {
-        throw new Error(
-          `Duplicate provider adapter registration for provider "${adapter.provider}"`,
-        );
+        return yield* Effect.die(
+          new Error(
+            `Duplicate provider adapter registration for provider "${adapter.provider}"`,
+          ),
+        );
       }
       byProvider.set(adapter.provider, adapter);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/ProviderAdapterRegistry.ts` around lines 48 -
55, Replace the thrown Error inside the adapter registration loop with an
explicit Effect.die to signal a defect idiomatically: when detecting a duplicate
key (the check using byProvider.has(adapter.provider) inside the for (const
adapter of adapters) loop), call Effect.die with a descriptive error (including
adapter.provider) instead of throw, ensuring Effect is imported/used in this
module and the defect intent is explicit for the ProviderAdapterRegistry
registration logic.

apps/server/src/provider/Layers/CopilotAdapter.ts (1)

1215-1246: ⚠️ Potential issue | 🟠 Major

Validation failures still leak the started Copilot client.

validateSessionConfiguration() can call client.start() and fail before the inner client.stop() catch around createSession() / resumeSession() runs. Invalid model or reasoning-effort inputs still leave an orphaned Copilot process behind.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/CopilotAdapter.ts` around lines 1215 - 1246,
validateSessionConfiguration may start the Copilot client and throw before the
later try/catch that calls client.stop(), leaking a started client; ensure the
client is stopped if validation fails by wrapping the
validateSessionConfiguration call (or the whole block that may start the client)
in a try/finally or catch that calls await client.stop(). Specifically, update
the flow around validateSessionConfiguration, client.start(), and the subsequent
createSession()/resumeSession() logic so that any thrown error after
client.start() triggers await client.stop().catch(() => {}) before rethrowing;
reference validateSessionConfiguration, client.start, client.stop,
resumeSessionId, createSession to locate the code to modify.

apps/server/src/geminiCliServerManager.ts (1)

242-243: ⚠️ Potential issue | 🟠 Major

Don't reopen or delete the session before the owning child exits.

Lines 532-533 mark the thread ready before close, and Line 394 deletes the session immediately after kill(). Because the later close/error handlers re-fetch by threadId and mutate state unconditionally, the old child can either lose its terminal event or clobber whichever turn/session has since reused that thread.

Also applies to: 304-347, 383-395, 531-533

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/geminiCliServerManager.ts` around lines 242 - 243, The
session lifecycle currently mutates or deletes a session (e.g., the block that
throws when session.status === "running", the code that calls kill(), and the
close/error handlers that re-fetch by threadId) while the original child process
may still be exiting; to fix, ensure you do not reopen or delete a session until
the owning child has fully exited by associating the session with an immutable
owner identifier (e.g., capture and store session.process.pid or a unique
ownerId when the child is spawned) and then, in the close/error handlers and
anywhere you call kill(), verify that the live session still matches that
ownerId before mutating/deleting or marking ready; update the handlers that
currently unconditionally mutate state (the handlers referenced around the
kill()/close/error regions) to early-return unless the ownerId/pid matches the
session fetched by threadId.

apps/server/src/kiloServerManager.ts (1)

1845-1852: Consider caching or server reuse for frequent model discovery.

The current implementation creates a new KiloServerManager instance for each call, which may spawn a new Kilo server if none exists. For scenarios where fetchKiloModels is called frequently, this could be inefficient.

If model discovery is expected to be called repeatedly, consider:

1. Caching results with a TTL
2. Accepting an existing manager instance as an optional parameter

This is acceptable for infrequent calls but worth considering for performance-sensitive use cases.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/kiloServerManager.ts` around lines 1845 - 1852,
fetchKiloModels currently creates a new KiloServerManager for every call (via
new KiloServerManager()), which may spawn servers repeatedly; modify
fetchKiloModels to accept an optional KiloServerManager parameter and reuse it
when provided, and/or add an in-memory cache keyed by the options with a TTL to
return cached results from listModels without instantiating a new manager;
ensure you still call stopAll() only for managers you created internally (track
ownership) and invalidate the cache after TTL or when listModels signals
changes.