-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(settings): apply the Desktop switches and report effective state #4899
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2f75a79
3b6c693
90035f3
c1378dd
4d0ee75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| import type { OcxConfig } from "../types"; | ||
| import { shouldSyncCodexOnStart } from "./desired-state"; | ||
| import { | ||
| isEffectiveCodexClientCompaction, | ||
| isEffectiveCodexDesktopAuthless, | ||
| } from "./loopback-target"; | ||
|
|
||
| export type CodexDesktopSwitchInertReason = | ||
| | "client_role" | ||
| | "non_loopback_bind_requires_admission_token"; | ||
|
|
||
| export interface CodexDesktopSwitchState { | ||
| stored: boolean; | ||
| effective: boolean; | ||
| inertReason?: CodexDesktopSwitchInertReason; | ||
| } | ||
|
|
||
| export type CodexDesktopSwitchApplyReason = | ||
| | "not_requested" | ||
| | "proxy_not_running" | ||
| | "integration_disabled" | ||
| | "write_lock_busy" | ||
| | "injection_refused"; | ||
|
|
||
| export type CodexDesktopSwitchApply = | ||
| | { applied: true } | ||
| | { | ||
| applied: false; | ||
| reason: CodexDesktopSwitchApplyReason; | ||
| retryable: boolean; | ||
| detail?: string; | ||
| }; | ||
|
|
||
| export interface CodexDesktopSwitchReport { | ||
| codexDesktopAuthless: CodexDesktopSwitchState; | ||
| codexClientCompaction: CodexDesktopSwitchState; | ||
| apply: CodexDesktopSwitchApply; | ||
| authSource: { presentsCodexAccount: boolean; summary: string }; | ||
| } | ||
|
|
||
| type DesktopSwitchConfig = Pick< | ||
| OcxConfig, | ||
| | "clientIntegrations" | ||
| | "runtimeRole" | ||
| | "hostname" | ||
| | "unauthenticatedLoopbackListener" | ||
| | "codexDesktopAuthless" | ||
| | "codexClientCompaction" | ||
| >; | ||
|
|
||
| function describeSwitch( | ||
| stored: boolean, | ||
| effective: boolean, | ||
| config: Pick<OcxConfig, "runtimeRole">, | ||
| ): CodexDesktopSwitchState { | ||
| if (!stored || effective) return { stored, effective }; | ||
| return { | ||
| stored, | ||
| effective, | ||
| inertReason: config.runtimeRole === "client" | ||
| ? "client_role" | ||
| : "non_loopback_bind_requires_admission_token", | ||
| }; | ||
| } | ||
|
|
||
| export function describeCodexDesktopSwitches( | ||
| config: DesktopSwitchConfig, | ||
| apply: CodexDesktopSwitchApply, | ||
| ): CodexDesktopSwitchReport { | ||
| const authlessStored = config.codexDesktopAuthless === true; | ||
| const authlessEffective = isEffectiveCodexDesktopAuthless(config); | ||
| const compactionStored = config.codexClientCompaction === true; | ||
| const compactionEffective = isEffectiveCodexClientCompaction(config); | ||
|
Comment on lines
+71
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For a hub bound to loopback without Useful? React with 👍 / 👎. |
||
|
|
||
| return { | ||
| codexDesktopAuthless: describeSwitch(authlessStored, authlessEffective, config), | ||
| codexClientCompaction: describeSwitch(compactionStored, compactionEffective, config), | ||
| apply, | ||
| authSource: authlessEffective | ||
| ? { | ||
| presentsCodexAccount: false, | ||
| summary: "The Codex app will not require its own account sign-in.", | ||
| } | ||
| : { | ||
| presentsCodexAccount: true, | ||
| summary: "The Codex app will require its own account sign-in.", | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| export async function applyCodexDesktopSwitches( | ||
| config: OcxConfig, | ||
| ): Promise<CodexDesktopSwitchApply> { | ||
| if (!shouldSyncCodexOnStart(config)) { | ||
| return { applied: false, reason: "integration_disabled", retryable: false }; | ||
| } | ||
|
|
||
| const { readRuntimePort } = await import("../config/process-state"); | ||
| const runtime = readRuntimePort(process.pid); | ||
| if (!runtime) { | ||
| return { applied: false, reason: "proxy_not_running", retryable: true }; | ||
| } | ||
|
|
||
| try { | ||
| // Imported at call time, not module load. The settings route reaches this module on | ||
| // every GET, and pulling the whole injection graph in just to report stored-versus- | ||
| // effective state would put it on a read path that never writes anything. | ||
| const { injectCodexConfig } = await import("./inject"); | ||
| const result = await injectCodexConfig(runtime.port, config); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a running remote client calls Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This call changes the user-visible workflow so AGENTS.md reference: AGENTS.md:L380-L381 Useful? React with 👍 / 👎. |
||
| if (result.status === "skipped") { | ||
| return { | ||
| applied: false, | ||
| reason: "integration_disabled", | ||
| retryable: false, | ||
| detail: result.message, | ||
| }; | ||
| } | ||
| if (result.success) { | ||
| // history_paginated_requires_native_writer stands down only the legacy relabel; | ||
| // apply still writes the routing and catalog half for paginated Codex homes. | ||
| return { applied: true }; | ||
|
Comment on lines
+118
to
+121
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| } | ||
| if (result.retryable === true) { | ||
| return { | ||
| applied: false, | ||
| reason: "write_lock_busy", | ||
| retryable: true, | ||
| detail: result.message, | ||
| }; | ||
| } | ||
| return { | ||
| applied: false, | ||
| reason: "injection_refused", | ||
| retryable: false, | ||
| detail: result.message, | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| applied: false, | ||
| reason: "injection_refused", | ||
| retryable: false, | ||
| detail: error instanceof Error ? error.message : "Codex config injection failed.", | ||
| }; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,11 @@ import { randomUUID } from "node:crypto"; | |
| import { readFileSync } from "node:fs"; | ||
| import type { CatalogModel } from "../../codex/catalog"; | ||
| import { catalogModelSlug, invalidateCodexModelsCache, nativeContextLimits, nativeModelRows, uniqueCatalogModelsForPublicList } from "../../codex/catalog"; | ||
| import { | ||
| applyCodexDesktopSwitches, | ||
| describeCodexDesktopSwitches, | ||
| type CodexDesktopSwitchApply, | ||
| } from "../../codex/desktop-switches"; | ||
| import { | ||
| DEFAULT_SUBAGENT_MODELS, | ||
| codexAutoStartEnabled, | ||
|
|
@@ -328,6 +333,11 @@ export async function handleConfigRoutes(ctx: ManagementContext): Promise<Respon | |
| codexDesktopAuthless: config.codexDesktopAuthless === true, | ||
| // Absent keeps Design B remote compaction; true selects the dedicated provider identity. | ||
| codexClientCompaction: config.codexClientCompaction === true, | ||
| codexDesktopSwitches: describeCodexDesktopSwitches(config, { | ||
| applied: false, | ||
| reason: "not_requested", | ||
| retryable: false, | ||
| }), | ||
| startupHealth: await readStartupHealth(config), | ||
| codexRuntime: { | ||
| path: displayCodexRuntimePath(resolved.runtime.command), | ||
|
|
@@ -597,15 +607,26 @@ export async function handleConfigRoutes(ctx: ManagementContext): Promise<Respon | |
| configureAppOwnedMemoryBudget(resolveAppOwnedMemoryBudgetBytes(body.appOwnedMemoryBudgetMb)); | ||
| enforceAppOwnedMemoryBudget(); | ||
| } | ||
| // Both Desktop compatibility switches change the injected config.toml shape, so converge now | ||
| // rather than waiting for the next start; the injector re-reads config and rewrites the form. | ||
| const authlessIsEnabled = config.codexDesktopAuthless === true; | ||
| const clientCompactionIsEnabled = config.codexClientCompaction === true; | ||
| const catalogRefresh = pickerWasEnabled !== pickerIsEnabled | ||
| || authlessWasEnabled !== authlessIsEnabled | ||
| || clientCompactionWasEnabled !== clientCompactionIsEnabled | ||
| const desktopSwitchesChanged = authlessWasEnabled !== authlessIsEnabled | ||
| || clientCompactionWasEnabled !== clientCompactionIsEnabled; | ||
| // Catalog convergence is not config injection, and the comment that used to sit here said | ||
| // it was. `convergeCodexCatalog` rejects any scope but `catalog` and never reaches the | ||
| // injector, which is why flipping either switch left `config.toml` in its old shape until | ||
| // a separate `ocx sync` (#4809). Both halves are needed when a Desktop switch changes; a | ||
| // picker-only update still refreshes just the catalog. | ||
| const catalogRefresh = pickerWasEnabled !== pickerIsEnabled || desktopSwitchesChanged | ||
| ? await convergeCodexCatalog() | ||
| : undefined; | ||
| // Injection second, matching `syncModelsToCodex`: the injected `model_catalog_json` should | ||
| // point at a catalog that has already settled. And it runs here rather than inside the save | ||
| // because coordinated Codex writes acquire the Codex write lock N before the config mutation | ||
| // lock C — awaiting N while still holding C would invert that order. | ||
| const desktopSwitchApply: CodexDesktopSwitchApply = desktopSwitchesChanged | ||
| ? await applyCodexDesktopSwitches(config) | ||
| : { applied: false, reason: "not_requested", retryable: false }; | ||
|
Comment on lines
620
to
+628
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift 🔎 Supported by static analysis🏁 Script executed: sed -n '580,660p' src/server/management/config-routes.ts
rg -n "type CatalogDisposition|interface CatalogDisposition|CatalogDisposition|convergeCodexCatalog|catalogRefreshPending" src/server tests
sed -n '180,255p' src/server/management-api.ts
rg -n "failed.*catalog|CatalogDisposition.*failed|convergeCodexCatalog" tests/config tests/helpersRepository: lidge-jun/opencodex Length of output: 24705 🏁 Script executed: set -e
printf '%s\n' '--- convergence types/status ---'
rg -n -A80 -B10 'export type CatalogDisposition|type CatalogDisposition|interface CatalogDisposition|function catalogRefreshIsPending|const catalogRefreshIsPending|catalogRefreshIsPending' src/codex src/server
printf '%s\n' '--- injector definition/usages ---'
rg -n -A100 -B20 'applyCodexDesktopSwitches|CodexDesktopSwitchApply' src tests
printf '%s\n' '--- relevant settings tests ---'
sed -n '360,760p' tests/config/settings-stream-mode.test.ts
printf '%s\n' '--- convergence contract tests around settings ---'
sed -n '360,440p' tests/codex-integration/codex-convergence-contract.test.tsRepository: lidge-jun/opencodex Length of output: 50375 🏁 Script executed: set -e
rg -n -A80 -B10 'export type CatalogDisposition|type CatalogDisposition|interface CatalogDisposition|function catalogRefreshIsPending|const catalogRefreshIsPending|catalogRefreshIsPending' src/codex src/server
rg -n -A100 -B20 'applyCodexDesktopSwitches|CodexDesktopSwitchApply' src tests
sed -n '360,760p' tests/config/settings-stream-mode.test.tsRepository: lidge-jun/opencodex Length of output: 50375 🏁 Script executed: set -e
printf '%s\n' '--- desktop switch module ---'
rg -n -A120 -B20 'export (async )?function applyCodexDesktopSwitches|function applyCodexDesktopSwitches|type CodexDesktopSwitchApply|interface CodexDesktopSwitchApply|describeCodexDesktopSwitches' src/codex/desktop-switches.ts src
printf '%s\n' '--- settings route test helpers and assertions ---'
rg -n -A35 -B20 'codexDesktopSwitches|catalogRefreshPending|applyCodexDesktopSwitches|createManagementConvergeCodex|PUT.*settings|/api/settings' tests/config tests/codex-integration src/server/management/config-routes.ts
printf '%s\n' '--- exact route setup and response projection ---'
sed -n '240,285p' src/server/management/config-routes.ts
sed -n '580,650p' src/server/management/config-routes.tsRepository: lidge-jun/opencodex Length of output: 50376 🏁 Script executed: set -e
rg -n -A120 -B20 'export (async )?function applyCodexDesktopSwitches|function applyCodexDesktopSwitches|type CodexDesktopSwitchApply|interface CodexDesktopSwitchApply|describeCodexDesktopSwitches' src/codex/desktop-switches.ts src
rg -n -A35 -B20 'codexDesktopSwitches|catalogRefreshPending|applyCodexDesktopSwitches|createManagementConvergeCodex|/api/settings' tests/config tests/codex-integration src/server/management/config-routes.ts
sed -n '580,650p' src/server/management/config-routes.tsRepository: lidge-jun/opencodex Length of output: 50376 🏁 Script executed: set -e
printf '%s\n' '--- desktop-switches.ts outline and implementation ---'
rg -n 'CodexDesktopSwitchApply|applyCodexDesktopSwitches|describeCodexDesktopSwitches' src/codex/desktop-switches.ts
sed -n '1,260p' src/codex/desktop-switches.ts
printf '%s\n' '--- catalog convergence test helper ---'
sed -n '1,100p' tests/helpers/catalog-convergence.ts
printf '%s\n' '--- relevant settings test helper ---'
rg -n 'function putDesktopSwitchInIsolatedHome|function putSettings|catalogConvergenceFactory' tests/config/settings-stream-mode.test.ts
sed -n '1,180p' tests/config/settings-stream-mode.test.tsRepository: lidge-jun/opencodex Length of output: 14299 🏁 Script executed: set -e
rg -n 'CodexDesktopSwitchApply|applyCodexDesktopSwitches|describeCodexDesktopSwitches' src/codex/desktop-switches.ts
sed -n '1,260p' src/codex/desktop-switches.ts
sed -n '1,100p' tests/helpers/catalog-convergence.ts
rg -n 'function putDesktopSwitchInIsolatedHome|function putSettings|catalogConvergenceFactory' tests/config/settings-stream-mode.test.ts
sed -n '1,180p' tests/config/settings-stream-mode.test.tsRepository: lidge-jun/opencodex Length of output: 14166 🏁 Script executed: set -e
rg -n 'model_catalog_json|export (async )?function injectCodexConfig|function injectCodexConfig|type.*Inject|interface.*Inject' src/codex/inject.ts src/codex
sed -n '1,260p' src/codex/inject.tsRepository: lidge-jun/opencodex Length of output: 14350 🏁 Script executed: set -e
sed -n '260,490p' src/codex/inject.ts
rg -n -A35 -B20 'chooseCatalogPathForInjection|setRootModelCatalogPath|catalogPath' src/codex/inject.ts src/codex/inject/config-toml.tsRepository: lidge-jun/opencodex Length of output: 50375 Gate Codex injection on successful catalog convergence.
Require 🤖 Prompt for AI Agents |
||
| const codexDesktopSwitches = describeCodexDesktopSwitches(config, desktopSwitchApply); | ||
| const catalogRefreshPending = catalogRefresh | ||
| ? catalogRefreshIsPending(catalogRefresh) | ||
| : false; | ||
|
|
@@ -622,6 +643,7 @@ export async function handleConfigRoutes(ctx: ManagementContext): Promise<Respon | |
| catalogRefreshPending, | ||
| codexDesktopAuthless: authlessIsEnabled, | ||
| codexClientCompaction: clientCompactionIsEnabled, | ||
| codexDesktopSwitches, | ||
| codexMainAccountHardLock: config.codexMainAccountHardLock === true, | ||
| mainAccountHardLock: getMainAccountHardLockStatus(config), | ||
| startupHealth: await readStartupHealth(config), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 50375
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 22785
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 46191
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 46064
Use
apply.retryablebefore recommendingocx sync.settingsUpdateLinesadds the sync instruction for every unsuccessful application.integration_disabledis explicitly non-retryable, andsyncModelsToCodexalso skips config writes while the integration remains disabled. Forinjection_refused,ocx syncruns the same injector preflight and returns the refusal before catalog or cache mutation.The current message therefore tells users to repeat a command that cannot resolve these failures without changing the underlying state. Append the instruction only when
apply.retryable === true. Add a CLI test for a non-retryable result and assert that the instruction is absent.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents