Skip to content

Refactor: address codebase audit findings#38

Open
aaditagrawal wants to merge 1 commit intomainfrom
refactor/codebase-audit-fixes
Open

Refactor: address codebase audit findings#38
aaditagrawal wants to merge 1 commit intomainfrom
refactor/codebase-audit-fixes

Conversation

@aaditagrawal
Copy link
Copy Markdown
Owner

@aaditagrawal aaditagrawal commented Mar 28, 2026

Summary

Addresses findings from a comprehensive codebase audit across contracts, server, and web packages.

Contracts

  • Standardize ModelSelection.options to Schema.optionalKey() for all 8 providers (was inconsistent mix of optional vs optionalKey)
  • Add binaryPath to GenericProviderSettings schema

Server

  • Extract shared adapter utils into ProviderAdapterUtils.ts — deduplicates ~257 lines of toMessage, toSessionError, toRequestError, asObject, asString, asNumber across 6 adapters
  • Add ServerSettingsService to all 6 fork-added adapters (Copilot, Cursor, OpenCode, GeminiCli, Amp, Kilo): read binary paths from server settings, check enabled flag before session start
  • Add missing tests: AmpAdapter.test.ts and GeminiCliAdapter.test.ts (3 tests each)
  • Add conformance tests for OpenCode and Kilo adapters (8/8 providers now covered)

Web

  • Remove modelSelection.ts pure re-export indirection — imports point directly to customModels.ts
  • Fix type-unsafe Record<string, unknown> casts in Sidebar.tsx for fork-only settings (providerAccentColors, grayscaleProviderLogos)
  • Rename duplicate getCustomModelOptionsByProvider to buildModelOptionsByProvider in ProviderModelPicker.tsx
  • Remove dead providers prop from ProviderModelPicker

Test plan

  • bun typecheck passes (7/7 packages, 0 errors)
  • bun lint passes (0 errors, 40 pre-existing warnings)
  • bunx oxfmt --check passes
  • AmpAdapter.test.ts: 3/3 pass
  • GeminiCliAdapter.test.ts: 3/3 pass
  • ProviderAdapterConformance.test.ts: 8/8 pass
  • Verify provider sessions start correctly with settings-derived binary paths

Summary by CodeRabbit

Release Notes

  • Improvements
    • Provider adapters now validate enablement status during session startup and reject sessions for disabled providers.
    • Configurable binary paths for AI providers—binaryPath settings now control where providers locate their executables, with sensible defaults when unspecified.
    • Enhanced error handling with consistent validation errors for provider configuration issues across all adapters.

Contracts:
- Standardize ModelSelection options to Schema.optionalKey() for all
  8 providers (was inconsistent: optional vs optionalKey)
- Add binaryPath to GenericProviderSettings schema

Server:
- Extract shared error handling (toMessage, toSessionError,
  toRequestError) and type helpers (asObject, asString, asNumber)
  into ProviderAdapterUtils.ts, deduplicating ~257 lines across
  6 adapters
- Add ServerSettingsService integration to all 6 fork-added
  adapters (Copilot, Cursor, OpenCode, GeminiCli, Amp, Kilo):
  read binary paths from settings, check enabled flag
- Add missing test files for AmpAdapter and GeminiCliAdapter
- Add OpenCode and Kilo to ProviderAdapterConformance tests

Web:
- Remove modelSelection.ts indirection (was pure re-exports)
- Fix type-unsafe Record<string, unknown> casts in Sidebar.tsx
  for fork-only settings (providerAccentColors, grayscaleProviderLogos)
- Rename duplicate getCustomModelOptionsByProvider to
  buildModelOptionsByProvider in ProviderModelPicker
- Remove dead providers prop from ProviderModelPicker
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 28, 2026

📝 Walkthrough

Walkthrough

Multiple provider adapter layers (AMP, Copilot, Cursor, Gemini CLI, Kilo, OpenCode, Codex) were updated to integrate ServerSettingsService for settings-based validation and configuration. A new shared utility module provides centralized error handling. Web components were refactored to import from customModels instead of modelSelection. Contract schemas were updated for optional field handling and provider configuration.

Changes

Cohort / File(s) Summary
Provider Adapter Utilities
apps/server/src/provider/Layers/ProviderAdapterUtils.ts
New shared module exporting error helpers (toSessionError, toRequestError, makeErrorHelpers) and type-narrowing utilities (asObject, asString, asArray, asNumber) for centralized provider-adapter error normalization across multiple adapters.
Server Adapter Layer Updates
apps/server/src/provider/Layers/AmpAdapter.ts, apps/server/src/provider/Layers/CopilotAdapter.ts, apps/server/src/provider/Layers/CursorAdapter.ts, apps/server/src/provider/Layers/GeminiCliAdapter.ts, apps/server/src/provider/Layers/KiloAdapter.ts, apps/server/src/provider/Layers/OpenCodeAdapter.ts, apps/server/src/provider/Layers/CodexAdapter.ts
Updated to inject ServerSettingsService, validate provider enabled status at startSession, read and normalize binaryPath from settings with provider-specific defaults, replace local error helpers with shared makeErrorHelpers, and map settings fetch failures to ProviderAdapterProcessError.
Server Adapter Tests
apps/server/src/provider/Layers/AmpAdapter.test.ts, apps/server/src/provider/Layers/GeminiCliAdapter.test.ts, apps/server/src/provider/Layers/CopilotAdapter.test.ts, apps/server/src/provider/Layers/CursorAdapter.test.ts, apps/server/src/provider/Layers/KiloAdapter.test.ts, apps/server/src/provider/Layers/OpenCodeAdapter.test.ts, apps/server/src/provider/Layers/ProviderAdapterConformance.test.ts
Test files updated to merge ServerSettingsService.layerTest() into adapter layers via Layer.provideMerge(...) for proper dependency injection. New test files added for AmpAdapter and GeminiCliAdapter with FakeManager implementations and Effect-based assertions.
Web Component Refactoring
apps/web/src/components/ChatView.tsx, apps/web/src/components/Sidebar.tsx, apps/web/src/components/chat/ProviderModelPicker.tsx, apps/web/src/components/chat/ProviderModelPicker.browser.tsx
Removed ServerProvider-based provider status tracking from ProviderModelPicker, renamed model-options builder (getCustomModelOptionsByProviderbuildModelOptionsByProvider), updated settings imports to use useAppSettings(), and removed disabled-provider rendering test case.
Web Module Consolidation
apps/web/src/modelSelection.ts, apps/web/src/hooks/useSettings.ts
Deleted re-export module modelSelection.ts; updated useSettings.ts to import normalizeCustomModelSlugs directly from customModels instead of through modelSelection.
Contract Schema Updates
packages/contracts/src/orchestration.ts, packages/contracts/src/settings.ts
Updated six model-selection schema options fields to use Schema.optionalKey instead of Schema.optional. Added binaryPath: TrimmedString field to GenericProviderSettings with empty-string default for provider binary path configuration.
Test Configuration
apps/web/src/components/KeybindingsToast.browser.tsx
Updated test server configuration to include explicit binaryPath: "" fields for all provider entries in settings.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Suggested labels

vouch:trusted, size:L

Poem

🐰 Hoppy settings dance through providers so fine,
Error helpers unified, schemas align,
Binary paths trim, validation's in place,
Web components refactored with elegance and grace,
Adapters now coordinated, the codebase takes flight!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Refactor: address codebase audit findings' clearly describes the main objective of the changeset—addressing audit findings across multiple packages. It is concise, relevant, and accurately reflects the scope of refactoring work.
Description check ✅ Passed The PR description comprehensively covers all changes across contracts, server, and web packages with clear sections for what changed and why. It includes a detailed test plan with pending status markers, addressing the template's 'What Changed' and 'Why' sections, though formatted differently.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/codebase-audit-fixes

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added size:XL 500-999 effective changed lines (test files excluded in mixed PRs). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Mar 28, 2026
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
apps/server/src/provider/Layers/ProviderAdapterConformance.test.ts (1)

44-94: ⚠️ Potential issue | 🟡 Minor

The conformance matrix still omits Kilo and OpenCode.

These layer additions only cover six adapters, and the cases array below still stops at amp. If this PR is meant to bring provider conformance to 8/8 after the settings refactor, makeKiloAdapterLive and makeOpenCodeAdapterLive still need to be added here.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/ProviderAdapterConformance.test.ts` around
lines 44 - 94, Add the missing Kilo and OpenCode adapters to the conformance
test by creating layers via makeKiloAdapterLive(...) and
makeOpenCodeAdapterLive(...) (mirroring how copilot/claude/cursor/amp/gemini
layers are created) and pipe the appropriate test layers (e.g.,
Layer.provideMerge(ServerSettingsService.layerTest()) plus NodeServices.layer
and ServerConfig.layerTest(...) where applicable). Then add those new layer
variables to the test's cases array (the same array that currently ends at amp)
so the matrix covers makeKiloAdapterLive and makeOpenCodeAdapterLive as well.
🧹 Nitpick comments (4)
packages/contracts/src/settings.ts (1)

79-84: Consider using makeBinaryPathSetting for consistency.

CodexSettings and ClaudeSettings (lines 66, 74) use makeBinaryPathSetting(fallback) which provides automatic fallback to the provider's default binary name during decoding. GenericProviderSettings.binaryPath defaults to "" instead, pushing the fallback responsibility to each adapter.

This works since adapters already handle the empty-string fallback (e.g., binaryPath.trim() || "kilo"), but using a consistent schema-level approach would reduce adapter boilerplate. Given this is a cross-cutting change affecting multiple adapters, the current approach is acceptable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/contracts/src/settings.ts` around lines 79 - 84,
GenericProviderSettings currently defines binaryPath with
TrimmedString.pipe(Schema.withDecodingDefault(() => "")) which forces each
adapter to implement empty-string fallback; replace that with the shared helper
by using makeBinaryPathSetting(fallback) for binaryPath to provide automatic
decoding fallback (as used by CodexSettings and ClaudeSettings), updating
GenericProviderSettings (and its type) to reference makeBinaryPathSetting so
adapters no longer need to handle empty-string fallbacks themselves.
apps/server/src/provider/Layers/AmpAdapter.test.ts (1)

109-126: Add coverage for the settings-backed AMP startup path.

These tests only verify delegation with the default settings layer. They do not assert disabled-provider rejection or binaryPath propagation, so the behavior introduced in AmpAdapter.ts can break silently.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/AmpAdapter.test.ts` around lines 109 - 126,
Add tests exercising the settings-backed AMP startup path: create a layer that
provides makeAmpAdapterLive({ manager }) merged with
ServerSettingsService.layerTest() configured to disable the "amp" provider and
another config that sets a custom binaryPath; for the disabled case call
AmpAdapter.startSession and assert it rejects with the expected
disabled-provider error, and for the binaryPath case call
AmpAdapter.startSession and assert manager.startSessionImpl was called with the
threadId and that the passed options include the configured binaryPath (inspect
manager.startSessionImpl.mock.calls). Locate symbols: makeAmpAdapterLive,
ServerSettingsService.layerTest, AmpAdapter, manager.startSessionImpl, and
startSession to implement these assertions.
apps/web/src/components/chat/ProviderModelPicker.tsx (1)

55-82: Keep model-option construction in a shared non-UI module.

buildModelOptionsByProvider now overlaps with getCustomModelOptionsByProvider that still lives under customModels.ts, and ChatView/tests import this one from the picker component file. That leaves two sources of truth for provider options and couples pure model-selection logic to a UI module. Please centralize the shared builder in a model/settings utility and keep any picker-only cursor shaping as a thin wrapper here.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/chat/ProviderModelPicker.tsx` around lines 55 - 82,
buildModelOptionsByProvider duplicates logic in getCustomModelOptionsByProvider
and lives in a UI component file, coupling model-selection to the picker;
extract the shared model-options builder into a new non-UI module (e.g.,
model/settings utility) and have buildModelOptionsByProvider call that shared
function for codex/copilot/claudeAgent/opencode/geminiCli/amp/kilo, while
keeping only the picker-specific cursor shaping here (use
getCursorModelFamilyOptions plus a thin wrapper that merges family options with
the shared builder's "cursor" results). Update references where ChatView/tests
import buildModelOptionsByProvider to import the shared builder from the new
utility and ensure getCustomModelOptionsByProvider in customModels.ts delegates
to the same shared builder to avoid duplicate sources of truth.
apps/server/src/provider/Layers/GeminiCliAdapter.test.ts (1)

109-126: Cover the settings-aware startSession path as well.

This suite only checks the happy-path delegation with default test settings. It never asserts enabled=false rejection or that the configured binaryPath reaches the manager, so the new startup behavior can regress without a failing test.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/server/src/provider/Layers/GeminiCliAdapter.test.ts` around lines 109 -
126, Add tests for the settings-aware startSession behavior: extend the existing
GeminiCliAdapterLive test (using makeGeminiCliAdapterLive and
ServerSettingsService.layerTest()) to include two cases — one where
ServerSettingsService.layerTest() is configured with enabled=false and assert
adapter.startSession rejects/throws, and another where you provide a custom
binaryPath in the test settings and assert manager.startSessionImpl is called
with that binaryPath (inspect its mock call args). Keep using the
GeminiCliAdapter accessor and manager.startSessionImpl mockClear/assert pattern
to locate and verify the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/server/src/provider/Layers/AmpAdapter.ts`:
- Around line 47-72: The AMP binary path from server settings is read into
_binaryPath but never used, so providerSettings.binaryPath is ignored; update
the call to manager.startSession inside the Effect.tryPromise to pass the
resolved binary path (use providerSettings.binaryPath or the trimmed
_binaryPath) into the session start (e.g., include it on the input object or as
the explicit parameter expected by manager.startSession) so manager.startSession
receives the configured binary; locate the _binaryPath/ providerSettings usage
and the manager.startSession(...) call and wire the path through, keeping the
existing error handling (toRequestError) and ProviderAdapter errors intact.

In `@apps/server/src/provider/Layers/CopilotAdapter.ts`:
- Around line 1306-1307: The code uses the settings-derived CLI path only in
startSession (cliPath computed from copilotSettings.binaryPath and
resolveBundledCopilotCliPath()) but fetchCopilotModels and fetchCopilotUsage
still call resolveBundledCopilotCliPath() directly; change those functions to
obtain the same effective CLI path (use the same cliPath computation or
centralize into a helper like getCopilotCliPath that uses
copilotSettings.binaryPath || resolveBundledCopilotCliPath()) and replace direct
calls to resolveBundledCopilotCliPath() in fetchCopilotModels and
fetchCopilotUsage so model/usage discovery uses the configured binary path
consistently with startSession.
- Around line 1271-1289: The current startSession() flow checks
serverSettingsService.getSettings and rejects when copilotSettings.enabled is
false, which prevents returning already-running sessions stored in sessions;
move the existing-session fast-path that looks up and returns an active session
from sessions (the in-memory/session store used by startSession) to before the
copilotSettings.enabled guard so existing sessions remain reachable, or
alternatively add logic that when copilotSettings.enabled flips to false the
adapter explicitly tears down active sessions by invoking the same cleanup used
by stopSession() / sendTurn() for each session; specifically adjust the logic
around serverSettingsService.getSettings, copilotSettings.enabled,
startSession(), sessions, sendTurn(), and stopSession() so either the sessions
lookup/return occurs first or active sessions are cleaned up when settings
change.

In `@apps/server/src/provider/Layers/GeminiCliAdapter.ts`:
- Around line 49-74: The code reads providerSettings.binaryPath into _binaryPath
but never passes it to the session start, so user-supplied Gemini paths are
ignored; update the call to manager.startSession to include the resolved path
(use the trimmed _binaryPath or default "gemini") by either merging it into the
input object (e.g., pass {...input, binaryPath: _binaryPath}) or calling the
manager API that accepts an explicit binaryPath, ensuring the identifier
_binaryPath (from providerSettings.binaryPath) is used when invoking
manager.startSession so the configured path is honored.

---

Outside diff comments:
In `@apps/server/src/provider/Layers/ProviderAdapterConformance.test.ts`:
- Around line 44-94: Add the missing Kilo and OpenCode adapters to the
conformance test by creating layers via makeKiloAdapterLive(...) and
makeOpenCodeAdapterLive(...) (mirroring how copilot/claude/cursor/amp/gemini
layers are created) and pipe the appropriate test layers (e.g.,
Layer.provideMerge(ServerSettingsService.layerTest()) plus NodeServices.layer
and ServerConfig.layerTest(...) where applicable). Then add those new layer
variables to the test's cases array (the same array that currently ends at amp)
so the matrix covers makeKiloAdapterLive and makeOpenCodeAdapterLive as well.

---

Nitpick comments:
In `@apps/server/src/provider/Layers/AmpAdapter.test.ts`:
- Around line 109-126: Add tests exercising the settings-backed AMP startup
path: create a layer that provides makeAmpAdapterLive({ manager }) merged with
ServerSettingsService.layerTest() configured to disable the "amp" provider and
another config that sets a custom binaryPath; for the disabled case call
AmpAdapter.startSession and assert it rejects with the expected
disabled-provider error, and for the binaryPath case call
AmpAdapter.startSession and assert manager.startSessionImpl was called with the
threadId and that the passed options include the configured binaryPath (inspect
manager.startSessionImpl.mock.calls). Locate symbols: makeAmpAdapterLive,
ServerSettingsService.layerTest, AmpAdapter, manager.startSessionImpl, and
startSession to implement these assertions.

In `@apps/server/src/provider/Layers/GeminiCliAdapter.test.ts`:
- Around line 109-126: Add tests for the settings-aware startSession behavior:
extend the existing GeminiCliAdapterLive test (using makeGeminiCliAdapterLive
and ServerSettingsService.layerTest()) to include two cases — one where
ServerSettingsService.layerTest() is configured with enabled=false and assert
adapter.startSession rejects/throws, and another where you provide a custom
binaryPath in the test settings and assert manager.startSessionImpl is called
with that binaryPath (inspect its mock call args). Keep using the
GeminiCliAdapter accessor and manager.startSessionImpl mockClear/assert pattern
to locate and verify the behavior.

In `@apps/web/src/components/chat/ProviderModelPicker.tsx`:
- Around line 55-82: buildModelOptionsByProvider duplicates logic in
getCustomModelOptionsByProvider and lives in a UI component file, coupling
model-selection to the picker; extract the shared model-options builder into a
new non-UI module (e.g., model/settings utility) and have
buildModelOptionsByProvider call that shared function for
codex/copilot/claudeAgent/opencode/geminiCli/amp/kilo, while keeping only the
picker-specific cursor shaping here (use getCursorModelFamilyOptions plus a thin
wrapper that merges family options with the shared builder's "cursor" results).
Update references where ChatView/tests import buildModelOptionsByProvider to
import the shared builder from the new utility and ensure
getCustomModelOptionsByProvider in customModels.ts delegates to the same shared
builder to avoid duplicate sources of truth.

In `@packages/contracts/src/settings.ts`:
- Around line 79-84: GenericProviderSettings currently defines binaryPath with
TrimmedString.pipe(Schema.withDecodingDefault(() => "")) which forces each
adapter to implement empty-string fallback; replace that with the shared helper
by using makeBinaryPathSetting(fallback) for binaryPath to provide automatic
decoding fallback (as used by CodexSettings and ClaudeSettings), updating
GenericProviderSettings (and its type) to reference makeBinaryPathSetting so
adapters no longer need to handle empty-string fallbacks themselves.
🪄 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: 4621e98b-d1f1-4826-82d3-124f68c30c96

📥 Commits

Reviewing files that changed from the base of the PR and between f6c3eff and 9b2291d.

📒 Files selected for processing (24)
  • apps/server/src/provider/Layers/AmpAdapter.test.ts
  • apps/server/src/provider/Layers/AmpAdapter.ts
  • apps/server/src/provider/Layers/CodexAdapter.ts
  • apps/server/src/provider/Layers/CopilotAdapter.test.ts
  • apps/server/src/provider/Layers/CopilotAdapter.ts
  • apps/server/src/provider/Layers/CursorAdapter.test.ts
  • apps/server/src/provider/Layers/CursorAdapter.ts
  • apps/server/src/provider/Layers/GeminiCliAdapter.test.ts
  • apps/server/src/provider/Layers/GeminiCliAdapter.ts
  • apps/server/src/provider/Layers/KiloAdapter.test.ts
  • apps/server/src/provider/Layers/KiloAdapter.ts
  • apps/server/src/provider/Layers/OpenCodeAdapter.test.ts
  • apps/server/src/provider/Layers/OpenCodeAdapter.ts
  • apps/server/src/provider/Layers/ProviderAdapterConformance.test.ts
  • apps/server/src/provider/Layers/ProviderAdapterUtils.ts
  • apps/web/src/components/ChatView.tsx
  • apps/web/src/components/KeybindingsToast.browser.tsx
  • apps/web/src/components/Sidebar.tsx
  • apps/web/src/components/chat/ProviderModelPicker.browser.tsx
  • apps/web/src/components/chat/ProviderModelPicker.tsx
  • apps/web/src/hooks/useSettings.ts
  • apps/web/src/modelSelection.ts
  • packages/contracts/src/orchestration.ts
  • packages/contracts/src/settings.ts
💤 Files with no reviewable changes (1)
  • apps/web/src/modelSelection.ts

Comment on lines +47 to +72
Effect.gen(function* () {
const providerSettings = yield* serverSettingsService.getSettings.pipe(
Effect.map((s) => s.providers.amp),
Effect.mapError(
(error) =>
new ProviderAdapterProcessError({
provider: PROVIDER,
threadId: input.threadId,
detail: error.message,
cause: error,
}),
),
);
if (!providerSettings.enabled) {
return yield* new ProviderAdapterValidationError({
provider: PROVIDER,
operation: "startSession",
issue: "AMP provider is disabled in server settings.",
});
}
const _binaryPath = providerSettings.binaryPath.trim() || "amp";
void _binaryPath;
return yield* Effect.tryPromise({
try: () => manager.startSession(input),
catch: (cause) => toRequestError(input.threadId, "session/start", cause),
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Configured AMP path is read and then dropped.

Line 67 computes the AMP binary from server settings, but Line 70 still starts the session with the original input. That makes providers.amp.binaryPath ineffective, so users who configure a non-default AMP executable will still launch the default path.

🤖 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 47 - 72, The AMP
binary path from server settings is read into _binaryPath but never used, so
providerSettings.binaryPath is ignored; update the call to manager.startSession
inside the Effect.tryPromise to pass the resolved binary path (use
providerSettings.binaryPath or the trimmed _binaryPath) into the session start
(e.g., include it on the input object or as the explicit parameter expected by
manager.startSession) so manager.startSession receives the configured binary;
locate the _binaryPath/ providerSettings usage and the manager.startSession(...)
call and wire the path through, keeping the existing error handling
(toRequestError) and ProviderAdapter errors intact.

Comment on lines +1271 to +1289
const copilotSettings = yield* serverSettingsService.getSettings.pipe(
Effect.map((s) => s.providers.copilot),
Effect.mapError(
(error) =>
new ProviderAdapterProcessError({
provider: PROVIDER,
threadId: input.threadId,
detail: error.message,
cause: error,
}),
),
);
if (!copilotSettings.enabled) {
return yield* new ProviderAdapterValidationError({
provider: PROVIDER,
operation: "startSession",
issue: "Copilot provider is disabled in server settings.",
});
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Return existing sessions before enforcing the disabled-provider guard.

With the new settings check here, an already-running Copilot session becomes unreachable via startSession() as soon as enabled flips to false, even though the record is still live in sessions and sendTurn() / stopSession() continue to work. Move the existing-session fast path above this guard, or explicitly tear active sessions down when the setting changes.

🤖 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 1271 - 1289,
The current startSession() flow checks serverSettingsService.getSettings and
rejects when copilotSettings.enabled is false, which prevents returning
already-running sessions stored in sessions; move the existing-session fast-path
that looks up and returns an active session from sessions (the in-memory/session
store used by startSession) to before the copilotSettings.enabled guard so
existing sessions remain reachable, or alternatively add logic that when
copilotSettings.enabled flips to false the adapter explicitly tears down active
sessions by invoking the same cleanup used by stopSession() / sendTurn() for
each session; specifically adjust the logic around
serverSettingsService.getSettings, copilotSettings.enabled, startSession(),
sessions, sendTurn(), and stopSession() so either the sessions lookup/return
occurs first or active sessions are cleaned up when settings change.

Comment on lines +1306 to +1307
const settingsBinaryPath = copilotSettings.binaryPath.trim();
const cliPath = settingsBinaryPath || resolveBundledCopilotCliPath();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

The new binaryPath only affects session startup.

Line 1306 switches startSession() to the settings-derived CLI, but Line 1724 and Line 1762 still hardcode resolveBundledCopilotCliPath() in fetchCopilotModels() and fetchCopilotUsage(). A custom Copilot path can therefore start sessions against one runtime while model/usage discovery talks to another.

🤖 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 1306 - 1307,
The code uses the settings-derived CLI path only in startSession (cliPath
computed from copilotSettings.binaryPath and resolveBundledCopilotCliPath()) but
fetchCopilotModels and fetchCopilotUsage still call
resolveBundledCopilotCliPath() directly; change those functions to obtain the
same effective CLI path (use the same cliPath computation or centralize into a
helper like getCopilotCliPath that uses copilotSettings.binaryPath ||
resolveBundledCopilotCliPath()) and replace direct calls to
resolveBundledCopilotCliPath() in fetchCopilotModels and fetchCopilotUsage so
model/usage discovery uses the configured binary path consistently with
startSession.

Comment on lines +49 to +74
Effect.gen(function* () {
const providerSettings = yield* serverSettingsService.getSettings.pipe(
Effect.map((s) => s.providers.geminiCli),
Effect.mapError(
(error) =>
new ProviderAdapterProcessError({
provider: PROVIDER,
threadId: input.threadId,
detail: error.message,
cause: error,
}),
),
);
if (!providerSettings.enabled) {
return yield* new ProviderAdapterValidationError({
provider: PROVIDER,
operation: "startSession",
issue: "Gemini CLI provider is disabled in server settings.",
});
}
const _binaryPath = providerSettings.binaryPath.trim() || "gemini";
void _binaryPath;
return yield* Effect.tryPromise({
try: () => manager.startSession(input),
catch: (cause) => toRequestError(input.threadId, "session/start", cause),
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Configured Gemini CLI path is currently a no-op.

Line 69 resolves providerSettings.binaryPath, but Line 72 still calls manager.startSession(input) with the unmodified input. A user-supplied Gemini executable path will never be honored, so the new server-settings integration does not actually affect session startup.

🤖 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 49 - 74,
The code reads providerSettings.binaryPath into _binaryPath but never passes it
to the session start, so user-supplied Gemini paths are ignored; update the call
to manager.startSession to include the resolved path (use the trimmed
_binaryPath or default "gemini") by either merging it into the input object
(e.g., pass {...input, binaryPath: _binaryPath}) or calling the manager API that
accepts an explicit binaryPath, ensuring the identifier _binaryPath (from
providerSettings.binaryPath) is used when invoking manager.startSession so the
configured path is honored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XL 500-999 effective changed lines (test files excluded in mixed PRs). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant