Merge upstream main and preserve Copilot provider support#51
Merge upstream main and preserve Copilot provider support#51
Conversation
|
Important Review skippedToo many files! This PR contains 249 files, which is 99 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (249)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
…aults, and canonicalize Claude-
| ); | ||
| const checkedAt = new Date().toISOString(); | ||
| const models = providerModelsFromSettings( | ||
| const allModels = providerModelsFromSettings( |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
checkClaudeProviderStatus builds the model list for the pre-probe / probe-failed branches from the raw built-in set, and makePendingClaudeProvider still does the same. That leaves claude-opus-4-7 visible before a successful claude --version check even though the adapter now hard-fails that model whenever installedVersion is unknown.
// apps/server/src/provider/Layers/ClaudeProvider.ts
const allModels = providerModelsFromSettings(
BUILT_IN_MODELS,
PROVIDER,
normalizeClaudeCustomModelsForVersion(claudeSettings.customModels, null),
DEFAULT_CLAUDE_MODEL_CAPABILITIES,
);I verified that the web consumes provider.models directly in @apps/web/src/providerModels.ts and falls back to the first built-in model in @apps/web/src/modelSelection.ts, so on the initial pending snapshot or after a transient --version timeout/non-zero exit the UI can default Claude to Opus 4.7 and then @apps/server/src/provider/Layers/ClaudeAdapter.ts rejects startSession/sendTurn with the new validation error. Filter built-ins through getBuiltInClaudeModelsForVersion(null) in the unknown-version and pending-snapshot branches as well, so the UI never offers a model the runtime cannot accept.
| ? parsed.updateChannel | ||
| : null; | ||
| const isLegacySettings = parsed.updateChannelConfiguredByUser === undefined; | ||
| const updateChannelConfiguredByUser = parsed.updateChannelConfiguredByUser === true; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
Older desktop builds already exposed a user-facing stable/nightly selector and persisted only updateChannel; after this migration, any legacy nightly install with a saved {"updateChannel":"latest"} is treated as not user-configured and is rewritten to the nightly default on startup. That silently changes the user’s update track during upgrade even though they had previously opted back to stable. ```ts
// apps/desktop/src/desktopSettings.ts
const isLegacySettings = parsed.updateChannelConfiguredByUser === undefined;
const updateChannelConfiguredByUser =
parsed.updateChannelConfiguredByUser === true ||
(isLegacySettings && parsedUpdateChannel === "nightly");
updateChannel:
updateChannelConfiguredByUser && parsedUpdateChannel !== null
? parsedUpdateChannel
: defaultSettings.updateChannel,
``` Preserve the stored legacy updateChannel when it is present, or add a versioned migration that can distinguish “unset” from “explicit stable” instead of forcing all legacy `latest` values to `defaultSettings.updateChannel`.
| // Handle --key=value syntax | ||
| const eqIndex = rest.indexOf("="); | ||
| if (eqIndex !== -1) { | ||
| flags[rest.slice(0, eqIndex)] = rest.slice(eqIndex + 1); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
parseCliArgs drops the value for any non-boolean flag when the next token begins with --, even if that token came from a quoted string. The new desktop settings UI stores Claude launchArgs as free-form text and ClaudeAdapter passes parseCliArgs(claudeSettings.launchArgs).flags directly into the SDK, so inputs like --append-system-prompt "--project foo" are misparsed as two flags instead of one flag/value pair. That makes a documented launch-arg use case impossible and silently changes the CLI invocation. Preserve quote intent during tokenization or otherwise allow explicitly quoted --... tokens to remain values for the preceding flag.
// packages/shared/src/cliArgs.ts
const next = tokens[i + 1];
if (next !== undefined && !next.startsWith("--")) {
flags[rest] = next;
i++;
} else {
| type ChatAttachment, | ||
| type ModelSelection, | ||
| type ProviderKind, | ||
| } from "@t3tools/contracts"; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This PR now makes textGenerationModelSelection.provider = "copilot" a real persisted/runtime path, but the server text-generation implementation still hard-fails any non-"codex" selection in CodexTextGeneration (generateCommitMessage/generatePrContent/generateBranchName/generateThreadTitle all return TextGenerationError("Invalid model selection.") when provider !== "codex"). The changed contract here advertises all ProviderKind values as valid for git text generation:
// apps/server/src/git/Services/TextGeneration.ts
import type { ChatAttachment, ModelSelection, ProviderKind } from "@t3tools/contracts";
/** Providers that support git text generation (commit messages, PR content, branch names). */
export type TextGenerationProvider = ProviderKind;I verified that GitManager and ProviderCommandReactor both read settings.textGenerationModelSelection and pass it through unchanged, while the PR also adds Copilot persistence/selection plumbing in packages/shared/src/serverSettings.ts and SettingsPanels.tsx. As a result, selecting Copilot for git text generation now breaks commit message generation, PR title/body generation, worktree branch naming, and first-turn thread title generation. Fix by either routing provider: "copilot" to an implementation that accepts Copilot selections, or normalizing Copilot text-generation selections to a Codex-compatible selection before these server call sites invoke TextGeneration.
| } | ||
| return mapping; | ||
| }, [orderedProjects]); | ||
| return buildPhysicalToLogicalProjectKeyMap({ |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
projectOrder is now written with physical project keys in the runtime sync path (derivePhysicalProjectKey(project)) and drag-reorder path (member.physicalProjectKey), but the read/replay path still compares those saved ids against scoped project refs here:
// apps/web/src/components/Sidebar.tsx
return orderItemsByPreferredIds({
items: projects,
preferredIds: projectOrder,
getId: (project) => scopedProjectKey(scopeProjectRef(project.environmentId, project.id)),
});orderItemsByPreferredIds() only matches exact ids, so the preferred order is silently dropped after sync and the sidebar falls back to raw project enumeration. useHandleNewThread() still has the same scoped-key lookup, so defaultProjectRef for chat.new / command-palette new-thread actions also ignores the user’s manual sidebar order. Replaying the order with derivePhysicalProjectKey(project) in both read sites (or switching the writers back to scoped keys consistently) fixes the regression.
| }); | ||
| } | ||
|
|
||
| const existingContext = sessions.get(input.threadId); |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
startSession() now tears down an existing thread session before any of the new-session failure points run (getSettings, CLI version probe, Opus 4.7 validation, createQuery). If any of those steps fail, the thread is left with no active Claude session even though the old one was still usable, which regresses the repo's stated reliability requirement around session restarts/failures. Swap only after the replacement session has been successfully validated and created, or keep the old context alive until the new one is ready.
// apps/server/src/provider/Layers/ClaudeAdapter.ts
const existingContext = sessions.get(input.threadId);
if (existingContext) {
yield* stopSessionInternal(existingContext, {
emitExitEvent: false,
}).pipe(
| Effect.map((result) => | ||
| Option.isSome(result) | ||
| ? parseGenericCliVersion(`${result.value.stdout}\n${result.value.stderr}`) | ||
| : null, |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The adapter's private CLI probe parses a version string from stdout/stderr without checking CommandResult.code, so startSession()/sendTurn() can treat Opus 4.7 support as verified even when claude --version exited non-zero. checkClaudeProviderStatus() handles the same probe as an error, so the runtime and provider snapshot can disagree about whether 4.7 is allowed. Require a zero exit code before accepting the parsed version.
// apps/server/src/provider/Layers/ClaudeAdapter.ts
Effect.map((result) =>
Option.isSome(result)
? parseGenericCliVersion(`${result.value.stdout}\n${result.value.stderr}`)
: null,
),| Effect.map((result) => | |
| Option.isSome(result) | |
| ? parseGenericCliVersion(`${result.value.stdout}\n${result.value.stderr}`) | |
| : null, | |
| Effect.map((result) => | |
| Option.isSome(result) && result.value.code === 0 | |
| ? parseGenericCliVersion(`${result.value.stdout}\n${result.value.stderr}`) | |
| : null, | |
| ), |
| parsed.serverExposureMode === "network-accessible" ? "network-accessible" : "local-only", | ||
| updateChannel: parsed.updateChannel === "nightly" ? "nightly" : "latest", | ||
| updateChannel: | ||
| (!isLegacySettings && updateChannelConfiguredByUser && parsedUpdateChannel !== null) || |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
Legacy settings files only persist updateChannel, so an older nightly user who explicitly switched to stable is serialized the same way as any other legacy file with "latest". This migration treats every legacy file whose stored channel differs from the current build default as implicit and falls back to defaultSettings.updateChannel, which flips that user back to nightly the first time a nightly build reads the file. That reintroduces the preference-loss bug instead of preserving user intent; the migration needs to carry forward the stored legacy channel or persist an explicit migration marker before rewriting it.
// apps/desktop/src/desktopSettings.ts
updateChannel:
(!isLegacySettings && updateChannelConfiguredByUser && parsedUpdateChannel !== null) ||
(isLegacySettings &&
parsedUpdateChannel !== null &&
parsedUpdateChannel === defaultSettings.updateChannel)
| enabled: Schema.Boolean.pipe(Schema.withDecodingDefault(Effect.succeed(true))), | ||
| binaryPath: makeBinaryPathSetting("claude"), | ||
| customModels: Schema.Array(Schema.String).pipe(Schema.withDecodingDefault(Effect.succeed([]))), | ||
| launchArgs: Schema.String.pipe(Schema.withDecodingDefault(Effect.succeed(""))), |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
This diff adds launchArgs to the authoritative ClaudeSettings schema, but the patch contract later in the same file still defines ClaudeSettingsPatch with only enabled, binaryPath, and customModels. The new settings UI at @apps/web/src/components/settings/SettingsPanels.tsx:1327 now sends providers.claudeAgent.launchArgs through updateSettings(), and @apps/web/src/hooks/useSettings.ts:165-168 optimistically applies that patch before sending it over the server.updateSettings RPC, whose payload is validated against ServerSettingsPatch. As a result, Claude launch-arg edits cannot round-trip through the authoritative server settings path and will be dropped or revert on the next config push.
// packages/contracts/src/settings.ts
export const ClaudeSettings = Schema.Struct({
enabled: Schema.Boolean.pipe(Schema.withDecodingDefault(Effect.succeed(true))),
binaryPath: makeBinaryPathSetting("claude"),
customModels: Schema.Array(Schema.String).pipe(Schema.withDecodingDefault(Effect.succeed([]))),
launchArgs: Schema.String.pipe(Schema.withDecodingDefault(Effect.succeed(""))),
});
|
|
||
| const trimmed = input.trim(); | ||
| for (let index = 0; index < trimmed.length; index++) { | ||
| const char = trimmed[index]!; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
parseCliArgs() now sits on the production path from the Claude settings textbox to ClaudeQueryOptions.extraArgs in @apps/server/src/provider/Layers/ClaudeAdapter.ts, but the tokenizer treats every backslash as an escape prefix instead of preserving non-whitespace backslashes. That means a value like --mcp-config C:\Users\alice\.claude.json is rewritten to C:Usersalice.claude.json before Claude is spawned, breaking Windows paths and any other flag values that legitimately contain backslashes. The new tests only cover escaped spaces, so this regression is currently unchecked. ts // packages/shared/src/cliArgs.ts if (char === "\\") { tokenStarted = true; escaping = true; continue; } Preserve \ unless it is actually escaping whitespace or a quote, and add a regression test for Windows-style path arguments passed through launchArgs.
| for (let index = 0; index < trimmed.length; index++) { | ||
| const char = trimmed[index]!; | ||
| const nextChar = trimmed[index + 1]; | ||
|
|
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
@packages/shared/src/cliArgs.ts now tokenizes a raw settings string with a plain whitespace split before @apps/server/src/provider/Layers/ClaudeAdapter.ts forwards the parsed flags to the Claude SDK as extraArgs. That breaks any valid Claude flag whose value contains spaces. For example, entering --append-system-prompt "Prefer numbered lists" in the settings UI becomes ['--append-system-prompt', '"Prefer', 'numbered', 'lists"'], so the SDK receives only "Prefer and silently drops the rest of the prompt text. This turns a supported launch-argument configuration into corrupted runtime behavior. Use a shell-aware tokenizer here, or persist launch args as a pre-split argv array instead of reparsing a free-form command string.
// packages/shared/src/cliArgs.ts
const tokens =
typeof args === "string" ? args.trim().split(/\s+/).filter(Boolean) : Array.from(args);
| include: | ||
| - label: macOS arm64 | ||
| runner: macos-14 | ||
| runner: macos-26 |
There was a problem hiding this comment.
[🟠 High] [🔵 Bug]
The new release matrix hard-codes Blacksmith runner labels for the macOS and Linux jobs, but those labels are not standard GitHub-hosted runners. Blacksmith requires separate runner provisioning/app installation, and this fork is a personal repository (zortos293/t3code-copilot), so these jobs will sit in Waiting for a runner / fail to start and the release pipeline never reaches packaging. Use GitHub-hosted labels here or make the runner labels configurable for forks.
# .github/workflows/release.yml
- label: macOS arm64
runner: blacksmith-12vcpu-macos-26
...
- label: Linux x64
runner: blacksmith-32vcpu-ubuntu-2404
| const selectionPatch = patch.textGenerationModelSelection; | ||
| const next = deepMerge(current, patch); | ||
| if (!selectionPatch || !shouldReplaceTextGenerationModelSelection(selectionPatch)) { | ||
| return next; |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
When textGenerationModelSelection.provider changes without an explicit model, this helper reuses the old model unconditionally:
// packages/shared/src/serverSettings.ts
const provider = selectionPatch.provider ?? current.textGenerationModelSelection.provider;
const model = selectionPatch.model ?? current.textGenerationModelSelection.model;
if (provider === "codex") {ServerSettingsPatch explicitly allows provider-only patches, so switching from a Claude selection to { provider: "codex" } now produces { provider: "codex", model: "claude-sonnet-4-6" }. That survives ServerSettings decoding because model is only validated as a non-empty string, and the server later passes it straight through to git text generation (codex exec --model <modelSelection.model>). The result is a persisted settings state that routes to the Codex generator with a Claude-only slug and fails at runtime. Fix by defaulting model to DEFAULT_GIT_TEXT_GENERATION_MODEL_BY_PROVIDER[nextProvider] whenever the provider changes and no model is supplied, or by rejecting provider-only patches for this field.
| ), | ||
| ); | ||
| const resolvedNext = resolveTextGenerationProvider(next); | ||
| yield* writeSettingsAtomically(next); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
resolveTextGenerationProvider() is documented as a read-time fallback so the stored preference survives temporary provider changes, but updateSettings() now persists the resolved fallback instead of the validated raw settings:
// apps/server/src/serverSettings.ts
const resolvedNext = resolveTextGenerationProvider(next);
yield* writeSettingsAtomically(resolvedNext);
yield* Cache.set(settingsCache, cacheKey, resolvedNext);
yield* emitChange(resolvedNext);With this path, disabling the currently selected provider (or selecting Copilot, which is intentionally excluded from GIT_TEXT_GENERATION_PROVIDERS) permanently overwrites textGenerationModelSelection on disk/cache, so the previous provider/model/options cannot come back when the provider is re-enabled. Persist next and only apply resolveTextGenerationProvider() on the returned/read path; otherwise the service loses the preference it claims to preserve. Also mirror that fix in the layerTest implementation so tests keep the same invariant.
| export function buildProjectUiSyncInputs( | ||
| projects: ReturnType<typeof selectProjectsAcrossEnvironments>, | ||
| ) { | ||
| const projectGroupingSettings = getUnifiedSettingsSnapshot(); |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
buildProjectUiSyncInputs() now derives sidebar IDs from the current grouping settings, but the runtime service only calls syncProjects() during shell snapshot/project events. Changing sidebarProjectGroupingMode/overrides (or hydrating persisted client settings) immediately makes Sidebar.tsx rebuild rows under new project.projectKey values, while useUiStateStore.projectOrder and projectExpandedById stay keyed by the old IDs until some later project event happens. That drops manual ordering and collapsed/expanded state for the current session, which is exactly the state this sync is supposed to preserve. The new dependency is introduced here:
// apps/web/src/environments/runtime/service.ts
const projectGroupingSettings = getUnifiedSettingsSnapshot();
const logicalProjectInputs = projects.map((project, index) => ({
key: deriveLogicalProjectKeyFromSettings(project, projectGroupingSettings),
logicalId: deriveLogicalProjectKeyFromSettings(project, projectGroupingSettings),Add a settings-change resync path (for grouping mode and overrides) so project UI state is rebuilt as soon as the grouping key function changes, not only after the next orchestration update.
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
readBrowserClientSettings() now turns any schema decode failure into null, which breaks upgrade compatibility for existing browser-only users who still have older partial snapshots in localStorage. hydrateClientSettings() only applies persisted values when getClientSettings() returns a truthy object, so these users now silently fall back to defaults and lose preferences such as sort/grouping/timestamp settings after upgrading. Restore the legacy fallback that re-parses the raw JSON and decodes it with defaults (or add an explicit migration path) instead of treating every decode miss as "no settings".
// apps/web/src/clientPersistenceStorage.ts
try {
return getLocalStorageItem(CLIENT_SETTINGS_STORAGE_KEY, ClientSettingsSchema);
} catch {
return null;
}1 participant
This PR merges the latest upstream
pingdotgg/t3codemaininto the fork while preserving all Copilot-specific provider integration, web UX, and fork-safe CI/release behavior.Server Runtime & Provider Layer
ProviderAdapterRegistry.tsdefault adapters arrayProviderRegistry.tswith refresh/streamChangesproviderStatusCache.tsprovider cache IDs and quotaSnapshots hydrationProviderLayerLiveinserver.tsPROVIDER_ORDERinserverSettings.tsto include copilotContracts & Shared Types
orchestration.tsProviderKind and added CopilotModelSelection unionmodel.tsquotaSnapshotsand ServerProviderModelbillingMultiplierinserver.tssettings.tswith upstream sidebar groupingshared/model.tswith Copilot normalization helpers compatible with upstreamshared/serverSettings.tsapplyServerSettingsPatch for discriminated model selectionWeb Provider Model Picker & Copilot UX
modelSelection.tswith copilot custom model config and createModelSelection helpercomposerProviderRegistry.tsxwith copilot trait handlingProviderModelPicker.tsxwith Copilot quota summary and billing multiplier UIstore.tsProviderKind import and Schema.is usagequotaSnapshotsto ServerProvider test mocks in browser/test filesRelease & CI Workflow Preservation
release.ymlwith GitHub-hostedubuntu-24.04runners and removed npm CLIpublish_clici.ymlto GitHub-hostedubuntu-24.04with upstream expectationsType System & Discriminated Unions
composerDraftStore.tsCopilot reasoningEffort null/undefined castingcreateModelSelection()inChatComposer.tsx,ChatView.tsx,SettingsPanels.tsxstore.tsnormalizeModelSelection to handle Copilot provider in union typesinput.tsxstyle function call for updated InputState shapeTest Compatibility & Import Fixes
resolveimport indev-runner.test.tslaunchArgsfrom codex settings inKeybindingsToast.browser.tsxMessagesTimeline.tsxand restored itemType fieldClaudeAgentEfforttype-only import inClaudeAdapter.ts