feat(toggles): Codex restore truth, Claude Desktop switch, composed acceptance (WP13 resume) - #1106
feat(toggles): Codex restore truth, Claude Desktop switch, composed acceptance (WP13 resume)#1106lidge-jun wants to merge 52 commits into
Conversation
…tating today, full sync-caller gating, workstation-only 030 leaves #1048 open)
…hantom coordinator; OFF path stays async
…alog joins the restore envelope
The commit-path OFF check runs under the first catalog permit; the models_cache rewrite reacquires K after release, so a disable landing in the gap could still publish a routed cache. Re-read intent under the second permit too.
…ee-client contract
…set, transport-liveness test restored
…status The inspector now carries ownedProfileActive (ID-match tri-state, null when metadata or appliedId is absent/unreadable) so the status route stops flattening undeterminable into false.
The toggle field names the API wire id (claude-desktop) which is not an OverviewClientId; widen the union explicitly. The status parser narrowed on fields absent from its readOptional shape.
…ontract for scenario 6, deferred-ID ledger
Re-read persisted integration intent and runtime port for POST /api/sync, and refuse foreign service-home convergence before catalog/cache writes.
Skip clean repeated restores and cache serialization while Codex is disabled, and avoid native-main startup leases when the proxy starts with Codex OFF.
Expose the canonical lock id on typed busy outcomes so callers can prove which same-home coordinator is contended.
Expose the native restore artifact envelope through `ocx restore --json` so child-process callers can distinguish history contention from success.
Do not start native-main ownership or run explicit restore when a recorded foreign service home owns the Codex artifacts.
Exercise real CLI children, management HTTP, persisted desired state, foreign-home refusal, cross-home lock contention, Grok startup, and busy history restore recovery in one isolated workstation suite.
… service claim B-reduced held the startup discovery flight instead of its own request; enable discovery only after the server is up and race the sentinel against completion. The fixture now records itself as the active install so P08 exercises the admit path rather than refusing on the workstation claim.
Persist fresh intent inputs and explicitly claim temp service homes so tests exercise their intended contracts without ambient service state.
Drive POST /api/sync from a persisted, local held provider so the race remains valid after the route refreshes configuration at its boundary.
Early exits (persist failure, already-OFF clean no-op) printed human text on a machine-readable invocation; each now emits one JSON envelope with skipped artifact states and never enters restore machinery.
Early outcomes now flow through skippedRestoreEnvelope(), keeping every artifact member schema-complete, with a CLI test on the no-op path.
# Conflicts: # src/cli/index.ts # src/codex/desired-state.ts # src/server/index.ts
|
✅ Deterministic PR hygiene checks passed. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe PR adds durable desired-state controls for Codex and Claude Desktop. It gates synchronization and restoration, adds structured outcomes and native routes, updates GUI state and localization, and adds isolated CLI, HTTP, race, ownership, and cleanup tests. ChangesIntegration toggle behavior
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/codex/inject.ts (1)
990-997: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA
desired_disabledhistory block is reported as "the history DB is locked" — a wrong diagnosis with an unactionable instruction.Line 983 passes
expectedDesiredEnabled: true. If the user disables the Codex integration during the window between the config write and the history job,runHistoryUnitUnderLockreturns{ kind: "blocked", reason: "desired_disabled" }(src/codex/history-worker.tslines 147-153). Line 997 collapses every non-converged, non-skippedoutcome into{ failed: true }, and line 1022 then prints:
⚠️ Codex resume history sync SKIPPED: the history DB is locked (Codex app/IDE open?). Close it and rerun 'ocx start'.Nothing was locked. The user turned the integration off, and the message tells them to close Codex and rerun the command that would turn it back on.
restoreNativeCodexAsyncalready handles this correctly at lines 1479-1485, with dedicated text for both desired-state reasons. Mirror that discrimination here so the two directions report the same class of event the same way.🐛 Proposed fix: carry the desired-state block through to the message
// A blocked or failed unit is reported, not silently counted as zero work: // `failed` is what makes the caller's message say so. - const history: { rows: number; files: number; failed?: true } = + const historyDesiredDisabled = historyOutcome.kind === "blocked" + && historyOutcome.reason === "desired_disabled"; + const history: { rows: number; files: number; failed?: true } = historyOutcome.kind === "converged" ? { rows: historyOutcome.rows, files: historyOutcome.files } - : historyOutcome.kind === "skipped" + : historyOutcome.kind === "skipped" || historyDesiredDisabled ? { rows: 0, files: 0 } : { rows: 0, files: 0, failed: true };Then add the explicit branch to the message chain:
const historyMessage = config?.syncResumeHistory === false ? ` Codex resume history: left unchanged (syncResumeHistory=false).\n` - : history.failed + : historyDesiredDisabled + ? ` Codex resume history: left unchanged — the Codex integration was turned OFF while this sync was running.\n` + : history.failedAlso applies to: 1017-1025
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/codex/inject.ts` around lines 990 - 997, Update the history outcome handling in the block around historyOutcome and the message chain near the history sync report to distinguish blocked desired-state reasons from lock failures. Preserve the existing failed behavior for lock or other failures, but carry desired_disabled (and the corresponding desired-state reason) through so the message gives the same dedicated guidance used by restoreNativeCodexAsync instead of claiming the history database is locked.src/cli/claude-desktop.ts (1)
75-94: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrite from a re-read config, as the two sibling writers now do.
The desired-state re-read at Line 79 is correct and correctly placed — no await separates it from the writer at Line 87. The values passed to the writer are not re-read though.
configcomes from Line 50, before thefetchAllModelsawait at Line 74, soconfig.port,config.apiKeys?.[0]?.key, and thedesktopVisibleNativeSlugs(config)/filterCatalogVisibleModels(..., config)inputs are all pre-await snapshots.This PR fixed exactly that staleness in both siblings.
handleClaudeDesktopTogglere-reads intolatestand writes from it (src/server/management/native-integration-routes.tsLines 647-663). The/api/claude-desktop/applyroute does the same with its ownlatest(src/server/management/agent-settings-routes.tsLines 767-782), and its added comment states the rule: "never write from the stale config captured before that await".Exposure here is smaller than on the routes, because this branch only runs when
findLiveProxyreturned nothing at Line 54. A rotated API key or a changed model selection written by another process during the catalog fetch would still be lost.♻️ Proposed refactor to read the same freshness the routes read
const { claudeDesktopIntegrationEnabledNow } = await import("../codex/desired-state"); if (!claudeDesktopIntegrationEnabledNow()) { return { ok: false, path: "", reason: "desired_state_changed" }; } - const routed = filterCatalogVisibleModels(allModels, config).map(model => ({ + // Same rule the management writers follow: the catalog await can admit any + // config change, so the writer's inputs come from a post-await read. + const latest = loadConfig(); + const routed = filterCatalogVisibleModels(allModels, latest).map(model => ({ provider: model.provider, id: model.id, contextWindow: model.contextWindow, })); const result = writeDesktop3pConfig( - config.port ?? 10100, - [...desktopVisibleNativeSlugs(config)], + latest.port ?? 10100, + [...desktopVisibleNativeSlugs(latest)], routed, - config.apiKeys?.[0]?.key, + latest.apiKeys?.[0]?.key, mode, state.profile, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/claude-desktop.ts` around lines 75 - 94, Re-read the current configuration after fetchAllModels and use that latest snapshot for the desktop writer. Update the inputs to filterCatalogVisibleModels, desktopVisibleNativeSlugs, the port, API key, and profile in writeDesktop3pConfig; preserve the existing desired-state check and skip behavior.gui/src/pages/integrations/IntegrationsOverview.tsx (3)
127-139: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the desired state for the switch label.
The switch state at Line [127] and the next-state calculation at Line [542] use
row.toggleOn ?? row.applied. The label still usesrow.applied.If desired state and applied state differ, the switch can be off while its accessible label says “Disable”, or on while it says “Apply”. Compute one
toggleOnvalue and use it for both the switch state and label.Proposed fix
const t = useT(); + const toggleOn = row.toggleOn ?? row.applied; const detail = row.detail ?? (row.detailKey ? t(row.detailKey, row.detailVars ?? undefined) : null); <Switch - on={row.toggleOn ?? row.applied} + on={toggleOn} onClick={onToggle} - label={row.applied + label={toggleOn ? t("integrations.action.disable") : t("integrations.action.apply")}As per path instructions, GUI state and user-visible action feedback must remain consistent with management API responses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/pages/integrations/IntegrationsOverview.tsx` around lines 127 - 139, Compute a single toggleOn value from row.toggleOn ?? row.applied in the integration row rendering, then use it for both the switch’s on state and the label selection. Replace the label’s direct row.applied check while preserving the existing apply/disable translation keys and next-state behavior.Source: Path instructions
422-435: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow refusal details for Claude Desktop.
OverviewCarddisables a switch whentoggleBlockedis set, butblockedTextis generated only forclaudeandgrokat Lines [94-102]. The new Claude Desktop path can receivemetadata_unreadableorcleanup_incomplete, yet it will show a disabled switch without the localized reason or residual paths.Include
"claude-desktop"in theblockedTextcondition.Proposed fix
- && (row.toggle === "claude" || row.toggle === "grok") + && ( + row.toggle === "claude" + || row.toggle === "grok" + || row.toggle === "claude-desktop" + )As per path instructions, user-visible refusal text must remain localized and actionable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/pages/integrations/IntegrationsOverview.tsx` around lines 422 - 435, Update the blockedText generation near the OverviewCard setup to include "claude-desktop" alongside "claude" and "grok" in its condition, preserving the existing localized and actionable refusal details for metadata_unreadable and cleanup_incomplete cases.Source: Path instructions
422-441: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRefresh the Claude Desktop status resource after its native toggle.
The Claude Desktop overview row is built from
claudeDesktopResourceingui/src/pages/integrations/overview-clients.ts.refreshNativeDetails()refreshesnativeResource,claudeResource, andgrokResource, but notclaudeDesktopResource.The new
claude-desktopbranch calls this helper after success and after failure. The switch, applied count, and status detail therefore keep the old response until another full refresh.Proposed fix
const refreshNativeDetails = () => { nativeResource.refresh(); claudeResource.refresh(); + claudeDesktopResource.refresh(); grokResource.refresh(); };As per path instructions, GUI state changes must stay consistent with management API responses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gui/src/pages/integrations/IntegrationsOverview.tsx` around lines 422 - 441, Update the refreshNativeDetails helper used by the native integration toggle flow to also refresh claudeDesktopResource, alongside nativeResource, claudeResource, and grokResource. Ensure both successful and failed claude-desktop toggle paths through the existing helper update the switch, applied count, and status detail from the management API response.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/pages/ClaudeDesktop.tsx`:
- Line 46: Update the statusResource cache hydration around
readSessionListCache<DesktopStatus> and useDataSurface so entries created before
desiredEnabled existed cannot be used; either bump the
ocx.claude-desktop.status.v1:${apiBase} cache key or validate cached data and
discard invalid entries while keeping hydration in useDataSurface.initialData.
Preserve management API response semantics so unavailable desiredEnabled values
do not incorrectly appear disabled or prevent the enable action.
In `@gui/tests/integrations-overview-rows.test.ts`:
- Around line 84-109: Extend the integration overview tests around
buildOverviewRows and claudeDesktopRow to assert toggleOn and detailKey for
desiredEnabled false and desiredEnabled true with applied false, covering the
desktopDesiredOff and desktopDesiredOnNotApplied branches. Add an indeterminate
case with desiredEnabled absent and verify it produces unknown state, a null
toggle, and no toggleOn value.
In `@src/cli/claude-desktop.ts`:
- Around line 46-49: Allow a missing persisted configuration to proceed through
all three Claude Desktop enable paths, preserving failure handling for other
reasons: in src/cli/claude-desktop.ts:46-49 update the setIntegrationEnabled
guard so applyProfile continues; in
src/server/management/agent-settings-routes.ts:718-720 let reason missing
continue to the writer; and in
src/server/management/native-integration-routes.ts:620-622 match
handleCodexToggle and handleGrokToggle, including reason not_durable in the
success response. If verification confirms missing is unreachable due to
bootstrap behavior, make no changes at these sites.
In `@src/cli/index.ts`:
- Around line 886-895: Update the catch path surrounding restoreNativeCodexAsync
in the restore command so its failed result includes the complete artifact
envelope, including config, catalog, and history, alongside success and message.
Ensure restoreJson’s JSON.stringify(r) emits the same schema as successful
results while preserving the captured error message.
- Around line 413-418: Handle every CodexSyncResult state at all listed CLI
callers: in src/cli/index.ts lines 413-418 and 458-462, log refused and
unsuccessful applied results in addition to skipped; in src/cli/models.ts lines
105-111, report refusal or failure after saving the custom model; and in
src/cli/provider.ts lines 232-259, mark synchronization successful only when
status is applied and ok is true, otherwise display the applicable refusal or
failure message.
In `@src/codex/inject.ts`:
- Around line 1491-1499: Update restoreNativeCodexAsync and restoreNativeCodex
so their aggregate message includes failure details from catalog.message when
catalog.state is "failed", alongside any existing history failure message. Apply
the change at src/codex/inject.ts lines 1491-1499 and 1546-1552; ensure both
async and sync paths surface failed catalog and history artifacts while
preserving existing success and normal catalog messaging.
- Around line 1300-1318: Update removeCodexConfig to return an explicit removed
boolean based on its existing had/write determination, including all return
paths, while preserving its message. In restoreCodexConfigInline, derive changed
from journal restoration/profile changes or restored.removed, and remove the
startsWith("Removed") message parsing.
- Around line 1437-1466: In the restore flow around the coordinated status
handling, return the failed restore result immediately when coordination is not
acquired, including busy or refused outcomes. Ensure restoreCodexCatalogArtifact
and runCodexHistoryJob execute only after a successful config restore, while
preserving the skipped and legacy paths.
In `@src/codex/sync.ts`:
- Line 31: Remove the duplicate CodexSyncAdmission declarations in
src/codex/sync.ts at lines 31-31, retaining exactly one declaration, and remove
the duplicate r declaration in src/cli/index.ts at lines 885-885, retaining a
single let r in that scope.
- Around line 16-17: Update the sync admission flow around admitCodexWrite() so
every refusal, including config and generation refusals, returns a refused
result before refreshCodexModelCatalog() or any artifact writes occur. If
external-provider courtesy is still required, identify and handle that case
before admission, while ensuring no refused admission reaches catalog refresh.
In `@src/server/index.ts`:
- Around line 543-555: Update the nativeMainLifecycle condition to call
inspectNativeCodexOwnership() only after shouldSyncCodexOnStart(config) is true,
and wrap that probe in best-effort error handling so exceptions cannot prevent
startup. On inspection failure, use an ownership fallback whose value is not
"foreign", preserving the normal lifecycle startup path while retaining the
disabled-Codex short circuit.
In `@src/server/management/native-integration-routes.ts`:
- Around line 623-625: Update the surrounding route logic to call loadConfig()
once and store its result in a shared snapshot, then derive both desiredEnabled
and fingerprint from that snapshot. Remove the separate loadConfig() call while
preserving the existing defaults and property access behavior.
- Around line 119-140: Update desktopStatus so disableBlocked also includes the
gateway_drifted state handled by removeDesktop3pStandardPivot. Preserve the
existing blocked states and assign gateway_drifted its distinct refusal reason
while keeping the current write_failed mapping for foreign and other applicable
states.
- Around line 330-338: Update the skipped-enable branch in the Codex toggle
handler to report the observed persisted desired state, not the requested
`enabled` value. Re-read or reuse the post-sync configuration value as the
Desktop sibling’s `handleClaudeDesktopToggle` does, and set `desiredEnabled` to
that observed OFF state while preserving the existing response message and
status fields.
In `@tests/cli-restore-back.test.ts`:
- Around line 95-103: Update the no-write assertion in the sync test to capture
configPath contents with readFileSync(configPath, "utf8") before spawning the
CLI, then compare the post-run contents against that captured value. Remove the
now-unused statSync import while preserving the existing status and output
assertions.
In `@tests/codex-composed-acceptance.test.ts`:
- Around line 278-284: Update the toggle assertion in the client loop to require
status 200 for every covered client id, including codex, grok, and
claude-desktop; remove 404 from the accepted statuses while preserving the
desiredEnabled false assertion.
- Around line 495-505: Update the JSON parsing in the blocked and converged
restore assertions to parse the last non-empty stdout line, matching the
defensive approach used by the sibling history-job test. Apply this to both
JSON.parse calls while preserving the existing success and failure envelope
assertions.
- Around line 179-204: Update spawnCli and the StartedServer lifecycle around
start() so every piped child stdout/stderr stream is drained from spawn time,
retaining accumulated output for callers that assert on it; rows without output
assertions may use stdout: "ignore". Update the Grok row to consume the
already-drained stdout result instead of attaching a late Response, and ensure
runCli reuses that drain rather than reading the same stream twice. Apply the
same handling to the first Grok server and A-, B-, and D-reduced servers.
In `@tests/codex-history-job.test.ts`:
- Around line 189-198: Update the restore-child assertions around runRestore in
the skipped and restored cases to include the child’s stderr (and relevant
stdout when useful) in the failure message, so nonzero exits surface the
underlying error. Replace the JSON.parse fallback that masks missing output with
assertions or diagnostics that preserve the actual child output while retaining
the existing expected status and response checks.
In `@tests/codex-retained-root-serialization.test.ts`:
- Around line 84-100: Extract the duplicated service-state and macOS LaunchAgent
ownership setup into a shared tests/helpers helper named
seedAdmittedServiceOwnership({ home, codexHome, opencodexHome }). Preserve the
existing version 2, scheduler backend, plist contents, permissions, and
Darwin-only behavior, then replace both inline constructions in the affected
test files with calls to this helper.
- Around line 304-319: The sync test’s Promise.race must require the fixture
provider request before accepting child completion. Update the flow around
waitForPath(requested) and sync.exited so /models is observed before sync.exited
can resolve, or otherwise assert requested is set before proceeding to
runPublisher; preserve the existing diagnostics for premature child exits.
In `@tests/codex-sync-api.test.ts`:
- Around line 35-53: The three test fixtures duplicate and diverge in how they
claim a temporary home; extract one shared helper under tests/helpers/ that
writes the service-state and Darwin plist artifacts and returns the claimed home
directories. Replace claimTempHome in tests/codex-sync-api.test.ts#L35-L53,
remove the local claimTempHome in tests/shutdown-launcher.test.ts#L28-L51 while
deciding whether HOME and USERPROFILE share one directory or remain split, and
replace ownedEnvironment in tests/cli-restore-back.test.ts#L9-L30 while
preserving its { HOME, USERPROFILE } shape for existing environment spreads.
- Around line 211-222: Update both the nested flip spawnSync and the main child
spawnSync around the child IIFE to include a finite timeout, and make each
status assertion surface the corresponding child.stderr when the exit status is
nonzero. Add a catch handler to the main async IIFE so rejected promises log the
error and exit with status 1, while preserving the existing cross-process timing
behavior.
In `@tests/desktop-3p-removal.test.ts`:
- Around line 52-58: Strengthen the traversal test by creating a sentinel file
at the sibling path resolved from library’s parent using the unsafe appliedId,
then assert it still exists after inspectDesktop3pConfigLibrary and
removeDesktop3pStandardPivot run. Add basename to the node:path imports as
needed, and preserve the existing unsafe-result assertions.
In `@tests/native-claude-desktop-toggle.test.ts`:
- Around line 126-128: Remove the redundant config.json write at
tests/native-claude-desktop-toggle.test.ts:97-97 because beforeEach already
seeds identical config() content. In
tests/native-claude-desktop-toggle.test.ts:126-128, retain the first persisted
write as the desktop-profile seed, keep the config.json.bak write, and delete
the duplicate final config.json write.
In `@tests/native-codex-toggle.test.ts`:
- Around line 111-115: Strengthen the artifact assertions in the native Codex
OFF test by replacing generic state-string checks with the expected skipped
restore envelope for config, catalog, and history. Assert success is true and
each artifact has state "skipped", changed false, action
"owned-fields-stripped", and an empty message, preserving the schema-complete
outcome for the fixture.
---
Outside diff comments:
In `@gui/src/pages/integrations/IntegrationsOverview.tsx`:
- Around line 127-139: Compute a single toggleOn value from row.toggleOn ??
row.applied in the integration row rendering, then use it for both the switch’s
on state and the label selection. Replace the label’s direct row.applied check
while preserving the existing apply/disable translation keys and next-state
behavior.
- Around line 422-435: Update the blockedText generation near the OverviewCard
setup to include "claude-desktop" alongside "claude" and "grok" in its
condition, preserving the existing localized and actionable refusal details for
metadata_unreadable and cleanup_incomplete cases.
- Around line 422-441: Update the refreshNativeDetails helper used by the native
integration toggle flow to also refresh claudeDesktopResource, alongside
nativeResource, claudeResource, and grokResource. Ensure both successful and
failed claude-desktop toggle paths through the existing helper update the
switch, applied count, and status detail from the management API response.
In `@src/cli/claude-desktop.ts`:
- Around line 75-94: Re-read the current configuration after fetchAllModels and
use that latest snapshot for the desktop writer. Update the inputs to
filterCatalogVisibleModels, desktopVisibleNativeSlugs, the port, API key, and
profile in writeDesktop3pConfig; preserve the existing desired-state check and
skip behavior.
In `@src/codex/inject.ts`:
- Around line 990-997: Update the history outcome handling in the block around
historyOutcome and the message chain near the history sync report to distinguish
blocked desired-state reasons from lock failures. Preserve the existing failed
behavior for lock or other failures, but carry desired_disabled (and the
corresponding desired-state reason) through so the message gives the same
dedicated guidance used by restoreNativeCodexAsync instead of claiming the
history database is locked.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 52b1c9d7-4355-4d11-be34-eca1216d8c6a
⛔ Files ignored due to path filters (1)
devlog/_plan/260806_wp13_toggles_resume/assets/integrations-overview.pngis excluded by!**/*.png
📒 Files selected for processing (58)
devlog/_plan/260806_wp13_toggles_resume/000_plan.mddevlog/_plan/260806_wp13_toggles_resume/010_codex_toggle_amendments.mddevlog/_plan/260806_wp13_toggles_resume/020_desktop_toggle_amendments.mddevlog/_plan/260806_wp13_toggles_resume/030_composed_acceptance_amendments.mdgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/ClaudeDesktop.tsxgui/src/pages/integrations/IntegrationsOverview.tsxgui/src/pages/integrations/integration-api.tsgui/src/pages/integrations/native-api.tsgui/src/pages/integrations/overview-clients.tsgui/src/pages/integrations/refusal-copy.tsgui/tests/integrations-overview-rows.test.tsgui/tests/integrations-surfaces.test.tsxsrc/claude/desktop-3p.tssrc/cli/claude-desktop.tssrc/cli/index.tssrc/cli/models.tssrc/cli/provider.tssrc/codex/catalog/sync.tssrc/codex/codex-write-lock.tssrc/codex/desired-state.tssrc/codex/history-job.tssrc/codex/history-provider.tssrc/codex/history-worker.tssrc/codex/inject-coordination.tssrc/codex/inject.tssrc/codex/internal/history-writer.tssrc/codex/refresh.tssrc/codex/sync.tssrc/config.tssrc/server/index.tssrc/server/management/agent-settings-routes.tssrc/server/management/config-routes.tssrc/server/management/context.tssrc/server/management/native-integration-routes.tssrc/types.tstests/claude-messages-endpoint.test.tstests/cli-restore-back.test.tstests/codex-composed-acceptance.test.tstests/codex-desired-state.test.tstests/codex-history-job.test.tstests/codex-history-provider.test.tstests/codex-inject-write-lock.test.tstests/codex-models-cache-invalidate.test.tstests/codex-retained-root-serialization.test.tstests/codex-sync-api.test.tstests/desktop-3p-removal.test.tstests/helpers/codex-inject-race-child.tstests/helpers/codex-write-lock-child.tstests/native-claude-desktop-toggle.test.tstests/native-codex-toggle.test.tstests/native-grok-toggle.test.tstests/shutdown-launcher.test.ts
| } | ||
|
|
||
| interface DesktopStatus { | ||
| desiredEnabled: boolean; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Invalidate old cached status entries when adding desiredEnabled.
statusResource seeds from readSessionListCache<DesktopStatus> using the unchanged ocx.claude-desktop.status.v1:${apiBase} key. An entry written before desiredEnabled existed can bypass the new response validation. If refresh fails, the status bar treats undefined as disabled, while Line [467] does not select the enable action because it checks === false.
Bump the cache key or validate cached data before passing it to useDataSurface.
Proposed cache-key fix
- const statusCacheKey = `ocx.claude-desktop.status.v1:${apiBase}`;
+ const statusCacheKey = `ocx.claude-desktop.status.v2:${apiBase}`;As per path instructions, GUI state changes must stay consistent with management API responses.
Based on learnings, keep cache hydration in useDataSurface.initialData; invalidate or validate the cached shape instead of adding a page-level seed.
Also applies to: 467-467
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/pages/ClaudeDesktop.tsx` at line 46, Update the statusResource cache
hydration around readSessionListCache<DesktopStatus> and useDataSurface so
entries created before desiredEnabled existed cannot be used; either bump the
ocx.claude-desktop.status.v1:${apiBase} cache key or validate cached data and
discard invalid entries while keeping hydration in useDataSurface.initialData.
Preserve management API response semantics so unavailable desiredEnabled values
do not incorrectly appear disabled or prevent the enable action.
Sources: Path instructions, Learnings
| const desktopNative = [{ | ||
| clientId: "claude-desktop" as const, | ||
| state: "current" as const, | ||
| installed: true, | ||
| configPath: "/tmp/desktop", | ||
| desiredEnabled: true, | ||
| disableBlocked: null, | ||
| }]; | ||
| const served = buildOverviewRows( | ||
| sources({ claudeDesktop: { applied: true, stale: false, activeProfile: true } }), | ||
| sources({ native: desktopNative, claudeDesktop: { desiredEnabled: true, installed: true, applied: true, stale: false, activeProfile: true } }), | ||
| ); | ||
| expect(rowById(served, "claudeDesktop").state).toBe("current"); | ||
|
|
||
| const notServed = buildOverviewRows( | ||
| sources({ claudeDesktop: { applied: true, stale: false, activeProfile: false } }), | ||
| sources({ native: desktopNative, claudeDesktop: { desiredEnabled: true, installed: true, applied: true, stale: false, activeProfile: false } }), | ||
| ); | ||
| expect(rowById(notServed, "claudeDesktop").state).toBe("stale"); | ||
|
|
||
| const drifted = buildOverviewRows( | ||
| sources({ claudeDesktop: { applied: true, stale: true, activeProfile: true } }), | ||
| sources({ native: desktopNative, claudeDesktop: { desiredEnabled: true, installed: true, applied: true, stale: true, activeProfile: true } }), | ||
| ); | ||
| expect(rowById(drifted, "claudeDesktop").state).toBe("stale"); | ||
|
|
||
| // Undeterminable must not downgrade a healthy applied profile. | ||
| const unknownProfile = buildOverviewRows( | ||
| sources({ claudeDesktop: { applied: true, stale: false, activeProfile: null } }), | ||
| sources({ native: desktopNative, claudeDesktop: { desiredEnabled: true, installed: true, applied: true, stale: false, activeProfile: null } }), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the desired-state axis these fixtures now enable.
The four scenarios cover the applied/drift axis well, and each correctly supplies desiredEnabled: true — without it the Line 299 guard in claudeDesktopRow would collapse every case to unknown and the state assertions would silently stop testing drift.
The new behavior in this change is the desired-vs-applied split, and none of it is asserted:
toggleOnis never checked, on any row.desiredEnabled: falseis never exercised, sointegrations.detail.desktopDesiredOff(gui/src/pages/integrations/overview-clients.tsLine 310) has no test.- The desired-on-but-not-applied branch and its
integrations.detail.desktopDesiredOnNotAppliedkey are untested. - The guard itself is untested: a payload with
desiredEnabledabsent must yieldtoggle: null, which is what stops the GUI rendering a switch in a guessed position.
💚 Proposed additional cases
test("claude desktop row separates desired state from applied state", () => {
const desktopNative = [{
clientId: "claude-desktop" as const,
state: "absent" as const,
installed: true,
configPath: "/tmp/desktop",
desiredEnabled: false,
disableBlocked: null,
}];
const desiredOff = rowById(buildOverviewRows(sources({
native: desktopNative,
claudeDesktop: { desiredEnabled: false, installed: true, applied: false },
})), "claudeDesktop");
expect(desiredOff.toggleOn).toBe(false);
expect(desiredOff.state).toBe("absent");
expect(desiredOff.detailKey).toBe("integrations.detail.desktopDesiredOff");
const desiredOnNotApplied = rowById(buildOverviewRows(sources({
native: desktopNative,
claudeDesktop: { desiredEnabled: true, installed: true, applied: false },
})), "claudeDesktop");
expect(desiredOnNotApplied.toggleOn).toBe(true);
expect(desiredOnNotApplied.detailKey).toBe("integrations.detail.desktopDesiredOnNotApplied");
// No boolean desired state means no switch, not a guessed position.
const indeterminate = rowById(buildOverviewRows(sources({
native: desktopNative,
claudeDesktop: { installed: true, applied: true },
})), "claudeDesktop");
expect(indeterminate.state).toBe("unknown");
expect(indeterminate.toggle).toBeNull();
expect(indeterminate.toggleOn).toBeUndefined();
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/tests/integrations-overview-rows.test.ts` around lines 84 - 109, Extend
the integration overview tests around buildOverviewRows and claudeDesktopRow to
assert toggleOn and detailKey for desiredEnabled false and desiredEnabled true
with applied false, covering the desktopDesiredOff and
desktopDesiredOnNotApplied branches. Add an indeterminate case with
desiredEnabled absent and verify it produces unknown state, a null toggle, and
no toggleOn value.
| // Explicit apply is an enable action. Persist intent before any Desktop write | ||
| // so a process crash cannot leave a gateway profile that startup immediately removes. | ||
| const desired = setIntegrationEnabled("claude-desktop", true); | ||
| if (!desired.ok) return { ok: false, path: "", reason: desired.message }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
All three Claude Desktop enable paths treat a missing config as fatal; the Codex and Grok paths in this same PR deliberately do not. setIntegrationEnabled returns ok:false, reason:"missing" when no config file exists yet (src/codex/desired-state.ts Lines 127-138), which is the normal state for a user who has never saved settings. The two sibling toggles gate with if (!persisted.ok && persisted.reason !== "missing") and document the reasoning: the artifact change is still worth performing, and what the user loses is durability across a restart, not the operation. The three Desktop enable paths omit that exception, so a first-run user gets a refusal from all of them.
src/cli/claude-desktop.ts#L46-L49: change the guard toif (!desired.ok && desired.reason !== "missing")soapplyProfileproceeds to save the profile and write the Desktop config. The current early return also makes the caller print "프로필은 저장되었지만..." at Line 122 when no profile was saved at all.src/server/management/agent-settings-routes.ts#L718-L720: skip the 409/500 return whendesired.reason === "missing", and let the apply continue to the writer at Line 778.src/server/management/native-integration-routes.ts#L620-L622: matchhandleCodexToggleLine 312 andhandleGrokToggleLine 443 — allowmissingthrough, and report the reduced durability on the success envelope withreason: "not_durable"as the Codex path does at Line 347.
If missing is in fact unreachable on these paths because an earlier bootstrap always creates config.json, close this without change; the verification script on the CLI site resolves that question for all three.
📍 Affects 3 files
src/cli/claude-desktop.ts#L46-L49(this comment)src/server/management/agent-settings-routes.ts#L718-L720src/server/management/native-integration-routes.ts#L620-L622
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/claude-desktop.ts` around lines 46 - 49, Allow a missing persisted
configuration to proceed through all three Claude Desktop enable paths,
preserving failure handling for other reasons: in
src/cli/claude-desktop.ts:46-49 update the setIntegrationEnabled guard so
applyProfile continues; in
src/server/management/agent-settings-routes.ts:718-720 let reason missing
continue to the writer; and in
src/server/management/native-integration-routes.ts:620-622 match
handleCodexToggle and handleGrokToggle, including reason not_durable in the
success response. If verification confirms missing is unreachable due to
bootstrap behavior, make no changes at these sites.
| if (live) { | ||
| const synced = await syncModelsToCodex(live.port).catch(e => { | ||
| console.error(`⚠️ Model sync skipped: ${e instanceof Error ? e.message : String(e)}`); | ||
| return null; | ||
| }); | ||
| if (synced?.status === "skipped") console.log(" Codex integration OFF; startup left Codex native."); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle all CodexSyncResult states at every CLI caller.
These callers only recognize skipped. A refused or unsuccessful applied result is silent, and provider add --sync can print “Models synced to Codex” when no sync occurred.
src/cli/index.ts#L413-L418: logrefusedand unsuccessful applied results before reporting the live proxy.src/cli/index.ts#L458-L462: logrefusedand unsuccessful applied results after startup.src/cli/models.ts#L105-L111: report refused and unsuccessful synchronization after saving a custom model.src/cli/provider.ts#L232-L259: set the success flag only forstatus === "applied" && ok === true; otherwise show the applicable refusal or failure message.
📍 Affects 3 files
src/cli/index.ts#L413-L418(this comment)src/cli/index.ts#L458-L462src/cli/models.ts#L105-L111src/cli/provider.ts#L232-L259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/index.ts` around lines 413 - 418, Handle every CodexSyncResult state
at all listed CLI callers: in src/cli/index.ts lines 413-418 and 458-462, log
refused and unsuccessful applied results in addition to skipped; in
src/cli/models.ts lines 105-111, report refusal or failure after saving the
custom model; and in src/cli/provider.ts lines 232-259, mark synchronization
successful only when status is applied and ok is true, otherwise display the
applicable refusal or failure message.
| try { | ||
| r = await restoreNativeCodexAsync(); | ||
| r = await restoreNativeCodexAsync({ revalidateDesiredState: true }); | ||
| } catch (err) { | ||
| r = { success: false, message: err instanceof Error ? err.message : String(err) }; | ||
| } | ||
| if (restoreJson) { | ||
| // Spawned callers need the artifact-level result to distinguish a busy | ||
| // history worker from a successful native restore. Keep stdout machine | ||
| // readable; human framing remains the default command contract. | ||
| console.log(JSON.stringify(r)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep restore --json schema-complete after an exception.
restoreNativeCodexAsync() returns artifacts, but the catch path assigns only success and message. JSON.stringify(r) then omits config, catalog, and history.
Return a failed artifact envelope from the catch path. This keeps machine consumers able to classify every restore result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/index.ts` around lines 886 - 895, Update the catch path surrounding
restoreNativeCodexAsync in the restore command so its failed result includes the
complete artifact envelope, including config, catalog, and history, alongside
success and message. Ensure restoreJson’s JSON.stringify(r) emits the same
schema as successful results while preserving the captured error message.
| const child = spawnSync(process.execPath, ["--eval", script], { | ||
| cwd: repoRoot, | ||
| env: { | ||
| ...process.env, | ||
| HOME: raceHome, | ||
| USERPROFILE: raceHome, | ||
| CODEX_HOME: raceCodexHome, | ||
| OPENCODEX_HOME: raceOcxHome, | ||
| }, | ||
| encoding: "utf8", | ||
| }); | ||
| expect(child.status).toBe(0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Give the race child a spawn timeout, and surface its stderr when the status assertion fails.
Two concrete problems with this spawnSync call:
- No
timeout. The child runs the realinjectCodexConfig, which acquires the Codex write lock, and it passes nolockTimeoutMs. If any sibling process still holds that lock —tests/codex-inject-write-lock.test.tsdeliberately parks a holder for 3 seconds — this call blocks with no upper bound and hangs the whole suite instead of failing the test. The nested flipspawnSyncon line 197 has the same gap. expect(child.status).toBe(0)on line 222 discards the diagnosis. The child's(async () => { … })()IIFE on line 192 has no.catch, so a module-resolution error or a rejected promise surfaces only on stderr. When this test fails in CI, the report readsexpected 0, received 1and nothing else — for a test whose whole point is a cross-process timing window.
🐛 Proposed fix: bound the child and keep its output in the failure message
const child = spawnSync(process.execPath, ["--eval", script], {
cwd: repoRoot,
env: {
...process.env,
HOME: raceHome,
USERPROFILE: raceHome,
CODEX_HOME: raceCodexHome,
OPENCODEX_HOME: raceOcxHome,
},
encoding: "utf8",
+ timeout: 30_000,
});
- expect(child.status).toBe(0);
+ expect({ status: child.status, stderr: child.stderr }).toMatchObject({ status: 0 });Also consider adding .catch(e => { console.error(e); process.exit(1); }) to the child IIFE on line 208 so a rejection becomes a deterministic non-zero exit rather than a runtime-dependent one.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const child = spawnSync(process.execPath, ["--eval", script], { | |
| cwd: repoRoot, | |
| env: { | |
| ...process.env, | |
| HOME: raceHome, | |
| USERPROFILE: raceHome, | |
| CODEX_HOME: raceCodexHome, | |
| OPENCODEX_HOME: raceOcxHome, | |
| }, | |
| encoding: "utf8", | |
| }); | |
| expect(child.status).toBe(0); | |
| const child = spawnSync(process.execPath, ["--eval", script], { | |
| cwd: repoRoot, | |
| env: { | |
| ...process.env, | |
| HOME: raceHome, | |
| USERPROFILE: raceHome, | |
| CODEX_HOME: raceCodexHome, | |
| OPENCODEX_HOME: raceOcxHome, | |
| }, | |
| encoding: "utf8", | |
| timeout: 30_000, | |
| }); | |
| expect({ status: child.status, stderr: child.stderr }).toMatchObject({ status: 0 }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/codex-sync-api.test.ts` around lines 211 - 222, Update both the nested
flip spawnSync and the main child spawnSync around the child IIFE to include a
finite timeout, and make each status assertion surface the corresponding
child.stderr when the exit status is nonzero. Add a catch handler to the main
async IIFE so rejected promises log the error and exit with status 1, while
preserving the existing cross-process timing behavior.
| test("a selected path traversal id is refused without following it", () => { | ||
| const library = mkdtempSync(join(tmpdir(), "ocx-desktop-remove-")); | ||
| writeFileSync(join(library, "_meta.json"), JSON.stringify({ appliedId: "../outside", entries: [] })); | ||
| const result = inspectDesktop3pConfigLibrary({ env: envFor(library) }); | ||
| expect(result).toMatchObject({ kind: "unsafe", reason: "unsafe_applied_id" }); | ||
| expect(removeDesktop3pStandardPivot({ env: envFor(library) }).kind).toBe("unsafe"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The traversal test asserts the refusal but not the "without following it" half of its own name.
Lines 56-57 prove inspectDesktop3pConfigLibrary and removeDesktop3pStandardPivot both classify "../outside" as unsafe. Neither assertion proves that no path outside library was touched. If a future refactor moved the SAFE_DESKTOP_PROFILE_ID check in src/claude/desktop-3p.ts to after profilePath() resolution and an existsSync/unlinkSync on the resolved path, this test would still pass while the guard had stopped guarding.
library comes from mkdtempSync, so its parent is tmpdir() and a sentinel is cheap to place.
♻️ Proposed fix: assert the sibling target survives
test("a selected path traversal id is refused without following it", () => {
const library = mkdtempSync(join(tmpdir(), "ocx-desktop-remove-"));
+ const outside = join(library, "..", `${basename(library)}-outside.json`);
+ writeFileSync(outside, "{}\n");
writeFileSync(join(library, "_meta.json"), JSON.stringify({ appliedId: "../outside", entries: [] }));
const result = inspectDesktop3pConfigLibrary({ env: envFor(library) });
expect(result).toMatchObject({ kind: "unsafe", reason: "unsafe_applied_id" });
expect(removeDesktop3pStandardPivot({ env: envFor(library) }).kind).toBe("unsafe");
+ expect(existsSync(outside)).toBe(true);
});This needs basename added to the node:path import on line 4.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/desktop-3p-removal.test.ts` around lines 52 - 58, Strengthen the
traversal test by creating a sentinel file at the sibling path resolved from
library’s parent using the unsafe appliedId, then assert it still exists after
inspectDesktop3pConfigLibrary and removeDesktop3pStandardPivot run. Add basename
to the node:path imports as needed, and preserve the existing unsafe-result
assertions.
| writeFileSync(join(root, "config.json"), JSON.stringify(persisted)); | ||
| writeFileSync(join(root, "config.json.bak"), JSON.stringify(persisted)); | ||
| writeFileSync(join(root, "config.json"), JSON.stringify(persisted)); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Redundant config.json writes across rows in tests/native-claude-desktop-toggle.test.ts. beforeEach at Line 52 already writes config.json with the config() content before every row. Two rows write it again, and one of them writes the same bytes twice within three lines. A reader cannot tell which write is load-bearing, and a future change to beforeEach will be masked by the local rewrites.
tests/native-claude-desktop-toggle.test.ts#L126-L128: Line 126 and Line 128 write byte-identicalpersistedcontent, separated only by the.bakwrite at Line 127. Delete Line 128 and keep the single seed at Line 126, which is load-bearing because it overrides thebeforeEachcontent with the desktop profile.tests/native-claude-desktop-toggle.test.ts#L97-L97: this write reproduces thebeforeEachcontent exactly and changes nothing. Delete it.
📍 Affects 1 file
tests/native-claude-desktop-toggle.test.ts#L126-L128(this comment)tests/native-claude-desktop-toggle.test.ts#L97-L97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/native-claude-desktop-toggle.test.ts` around lines 126 - 128, Remove
the redundant config.json write at
tests/native-claude-desktop-toggle.test.ts:97-97 because beforeEach already
seeds identical config() content. In
tests/native-claude-desktop-toggle.test.ts:126-128, retain the first persisted
write as the desktop-profile seed, keep the config.json.bak write, and delete
the duplicate final config.json write.
| expect(result.body.artifacts).toMatchObject({ | ||
| config: { state: expect.any(String) }, | ||
| catalog: { state: expect.any(String) }, | ||
| history: { state: expect.any(String) }, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the disable test fixture and the artifact states the route can emit.
fd -t f 'native-codex-toggle.test.ts' tests --exec sed -n '60,120p'
rg -nP --type=ts -C4 'artifacts' src/server/management/native-integration-routes.ts | head -80
rg -nP --type=ts -C2 'state:\s*"' src/codex/ | head -60Repository: lidge-jun/opencodex
Length of output: 8862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- candidate native-codex-toggle.test.ts sections ---'
fd -t f 'native-codex-toggle.test.ts' tests --exec sh -c 'echo "FILE: $1"; wc -l "$1"; sed -n "1,140p" "$1"' sh {}
echo '--- native-integration-routes handleCodexToggle envelope section ---'
fd -t f 'native-integration-routes.ts' src --exec sh -c 'wc -l "$1"; sed -n "280,380p" "$1"' sh {}
echo '--- restore result definitions/usages ---'
rg -n --type=ts -C4 'CodexNativeRestoreResult|artifacts:|history|catalog|config.*state|state: "skipped"|changed|rows|removed' src | head -240
echo '--- CLI restore test reference ---'
fd -t f 'cli-restore-back.test.ts' tests --exec sh -c 'echo "FILE: $1"; sed -n "60,95p" "$1" | cat -n' sh {}Repository: lidge-jun/opencodex
Length of output: 27133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate restoreNativeCodexAsync and artifact definitions ---'
rg -n --type=ts -C5 'restoreNativeCodexAsync|CodexNativeRestoreResult|interface .*Artifact|type .*Artifact|changed:|removed:|rows:' src/codex src/server | head -320
echo '--- inject.ts relevant restore result construction ---'
fd -t f 'inject.ts' src/codex --exec sh -c 'echo "FILE: $1"; wc -l "$1"; rg -n -C10 'restoreNativeCodexAsync|artifacts|changed|removed|rows|state:' "$1" | head -260' sh {}Repository: lidge-jun/opencodex
Length of output: 23761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- files named inject.ts ---'
fd -i '^inject\.ts$' src
echo '--- searches exact symbols with grep ---'
grep -RIn --include='*.ts' 'restoreNativeCodexAsync\|CodexNativeRestoreResult\|NativeToggleEnvelope\|changed:\s*false\|removed:\s*0\|rows:\s*0' src tests | head -320Repository: lidge-jun/opencodex
Length of output: 21289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/codex/inject.ts: artifact envelope section ---'
sed -n '1218,1515p' src/codex/inject.ts | cat -n
echo '--- tests/native-codex-toggle.test.ts: test shape ---'
sed -n '1,145p' tests/native-codex-toggle.test.ts | cat -n
echo '--- behavioral shape probe from source literals ---'
python3 - <<'PY'
from pathlib import Path
inject = Path('src/codex/inject.ts').read_text()
for name in ['skippedRestoreEnvelope', 'restoreNativeCodexAsync']:
idx = inject.find(f'export function {name}')
print(f'\n{name}: {idx}')
# Print compact artifact payloads found in this section
PYRepository: lidge-jun/opencodex
Length of output: 22093
Pin the artifact outcome in tests/native-codex-toggle.test.ts:111-115.
This fixture creates only config.json and service-state.json, so the no-restore Codex OFF path should produce the schema-complete skipped envelope. The current assertions accept any state string, including "failed", so a restore regression can pass on a broken outcome. Assert skippedRestoreEnvelope(success: true, ...) values, or at least { state: "skipped", changed: false, action: "owned-fields-stripped", message }, with success: true and message: "" on the top-level envelope if needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/native-codex-toggle.test.ts` around lines 111 - 115, Strengthen the
artifact assertions in the native Codex OFF test by replacing generic
state-string checks with the expected skipped restore envelope for config,
catalog, and history. Assert success is true and each artifact has state
"skipped", changed false, action "owned-fields-stripped", and an empty message,
preserving the schema-complete outcome for the fixture.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a875c2eb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const nativeMainLifecycle: NativeMainStartupLifecycle = shouldSyncCodexOnStart(config) | ||
| && nativeOwnership.ownership !== "foreign" | ||
| ? startNativeMainStartupLifecycle(deps.nativeMainStartup) |
There was a problem hiding this comment.
When the proxy starts with clientIntegrations.codex: false, this substitutes an always-ready lifecycle and never probes or recovers a pending native-main credential transaction. That switch disables Codex client injection, not native OpenAI main-account routing for Claude, Grok, or direct data-plane requests, so those requests can proceed while the credential store is in an interrupted transition. Keep the real recovery/admission lifecycle active independently of Codex injection, or explicitly block all native-main traffic until a lifecycle is initialized.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| if (!result.written) return postCommitRefusal(500, "claude-desktop", "write_failed", "Claude Desktop apply failed.", { desiredEnabled: latestDesiredEnabled }); | ||
| return jsonResponse({ | ||
| ok: true, clientId: "claude-desktop", changed: true, state: "current", desiredEnabled: latestDesiredEnabled, | ||
| message: "Claude Desktop integration enabled.", | ||
| } satisfies NativeToggleEnvelope); |
There was a problem hiding this comment.
When the overview enables Claude Desktop without an already matching appliedFingerprint—for example, the first enable from an existing standard library—the writer returns the new fingerprint but this success path discards it. The next inspection therefore classifies the just-written profile as gateway_drifted, and removeDesktop3pStandardPivot refuses that state, so the newly added switch cannot turn the integration back off. Persist the returned fingerprint and apply timestamp through a fresh config mutation before reporting success.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| if (url.pathname === "/api/claude-desktop/apply" && req.method === "POST") { | ||
| try { | ||
| const { setIntegrationEnabled, claudeDesktopIntegrationEnabled } = await import("../../codex/desired-state"); | ||
| const desired = setIntegrationEnabled("claude-desktop", true); |
There was a problem hiding this comment.
When Desktop is currently disabled, this field-scoped mutation removes the persisted false, but the handler later saves the request's older ctx.config snapshot, which still contains clientIntegrations["claude-desktop"] = false; saveConfigPreservingClaudeCode does not merge that field. The subsequent loadConfig() consequently sees OFF and returns desired_state_changed, so the Claude Desktop page's “Enable & Apply” action cannot re-enable the integration. Reload or mutate the fresh persisted config when saving the profile rather than writing the pre-toggle snapshot.
Useful? React with 👍 / 👎.
| // Spawned callers need the artifact-level result to distinguish a busy | ||
| // history worker from a successful native restore. Keep stdout machine | ||
| // readable; human framing remains the default command contract. | ||
| console.log(JSON.stringify(r)); |
There was a problem hiding this comment.
If restoreNativeCodexAsync throws before returning its artifact envelope—for example, on an unexpected filesystem or coordinator failure—the catch constructs only { success, message }, and this line emits that incomplete object for ocx restore --json. Consumers that rely on the newly documented artifacts.config/catalog/history contract then fail precisely on exceptional restores. Convert the catch result to the same full failure envelope used by the other early exits before serializing it.
Useful? React with 👍 / 👎.
| const preImages = captureCodexPreImages(); | ||
| let restored: CodexRestoreConfigResult; | ||
| try { | ||
| restored = restoreCodexConfigInline(); |
There was a problem hiding this comment.
restoreCodexConfigInline() catches its own exceptions and returns { state: "failed" }, so the surrounding catch never runs. If journal restoration writes config.toml and then fails while restoring the profile, this callback returns normally, the coordinator transaction commits its remove generation, and the captured pre-images are never restored even though the filesystem is only partially restored. Detect the failed result inside the lock and compensate or abort the transition before allowing the callback to return.
Useful? React with 👍 / 👎.
| if (appliedId === null) { | ||
| return { kind: "no_owned_state", libraryPath, selectedProfilePath: null, appliedId: null, residualPaths: [], ownedProfileActive: null }; |
There was a problem hiding this comment.
If readable _meta.json has no appliedId but still contains an opencodex entry and its profile or backup—for example after Desktop deselects the profile or an external metadata update is interrupted—this returns no_owned_state without examining those rows. The OFF path then reports an idempotent no-op while credential-bearing owned files remain on disk. Scan for owned entries before classifying this state and either clean their safe paths or return an explicit unsafe/incomplete result.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| <div className="integration-toggle-control"> | ||
| <Switch | ||
| on={row.applied} | ||
| on={row.toggleOn ?? row.applied} |
There was a problem hiding this comment.
When a client has desired state ON but observed application is absent—such as after a failed Desktop or Codex apply—the switch renders in its ON position through toggleOn, but its accessible label is still selected from row.applied and says “Apply.” Activating that control actually sends OFF, so screen-reader users are told the opposite action. Use the same row.toggleOn ?? row.applied value for both the switch position and its action label.
AGENTS.md reference: gui/AGENTS.md:L33-L33
Useful? React with 👍 / 👎.
| const { getConfigPath } = await import("../../config"); | ||
| return jsonResponse({ | ||
| clients: [claudeStatus(config, getConfigPath()), grokStatus()], | ||
| clients: [claudeStatus(config, getConfigPath()), grokStatus(config), codexStatus(config, getConfigPath()), desktopStatus(config)], |
There was a problem hiding this comment.
setIntegrationEnabled() updates config.json through a separate fresh mutation and does not modify the long-lived server config object passed here. After toggling Codex, Grok, or Claude Desktop, subsequent GET /api/native-integrations calls therefore keep returning the startup value in desiredEnabled—and Desktop inspection also receives a stale fingerprint—until the proxy restarts. Load the persisted config once in this GET handler and use that snapshot for all durable status fields.
Useful? React with 👍 / 👎.
| const selected = metadata.entries.find(entry => entry?.id === metadata.appliedId && isOwnedDesktopGatewayEntry(entry)); | ||
| const existing = selected ?? metadata.entries.find(entry => isOwnedDesktopGatewayEntry(entry) && typeof entry.id === "string"); | ||
| const id = existing?.id ?? randomUUID(); |
There was a problem hiding this comment.
Every successful OFF pivot creates and selects a new opencodex-standard profile, but the enable writer only searches for rows named opencodex. It consequently creates a new gateway row while leaving the selected standard row and file behind; each ON/OFF cycle adds another permanent standard entry and JSON file. Reuse the selected owned standard row on enable, or remove superseded owned standard rows after the new gateway is safely selected.
Useful? React with 👍 / 👎.
| if (url.pathname === "/api/native-integrations/claude-desktop" && req.method === "PUT") { | ||
| return handleClaudeDesktopToggle(ctx); |
There was a problem hiding this comment.
This adds a user-facing management endpoint and durable ON/OFF behavior, but the commit does not update docs-site/; the management API reference still documents only the older profile GET/PUT and apply routes, with no native Desktop toggle, desired-state field, standard-profile pivot, or refusal behavior. Add the endpoint and lifecycle semantics to the English source and keep the translated references synchronized.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
…sts:write Pre-existing red on origin/dev (run 31078958589 and a clean dev worktree both fail it); the permissions gate now matches the migrated workflow.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
This campaign is high-value, but the current head must be rebased before I can approve it.
The important boundaries are in the right place: OFF intent is persisted before mutation; config/history/catalog/cache writers re-read intent inside their serialization boundary; restore returns artifact-level truth instead of folding history failure into success; Claude Desktop removal pivots to a credential-free standard profile and refuses owned-but-drifted state; and the composed suite drives real CLI/HTTP entry points rather than only helpers. Those are meaningful correctness improvements, not cosmetic toggle work.
Current blocker: head 39f452ac is 8 commits behind dev and GitHub reports it conflicting. I reproduced the merge against a5b37827; the only conflict is tests/ci-workflows.test.ts. Current dev intentionally accepts either copilot-requests: write or the migration-era models: read, while this branch re-tightens the assertion to Copilot only. Please rebase onto current dev and keep the current dev assertion unless the workflow itself is being changed in this PR. The source/runtime changes merge without conflict.
Local current-dev merge validation after resolving that test conflict with the dev version:
- 12 focused Codex/Claude Desktop toggle, restore, sync, lock, and composed suites: 100 passed; one
native-codex-togglecase exceeded its 5 s timeout under the concurrent CPU-limited run and caused 3 cascading failures - isolated rerun of
tests/native-codex-toggle.test.ts: 6 passed, 0 failed (the timed case completed in 180 ms) bun run typecheck: passed
Because this touches credential-bearing Desktop profiles, persistent desired state, cross-process locks, management writers, and GUI controls, the rebased exact head needs the full required CI and GUI gates before approval. Please rebase rather than merge dev into the branch again, rerun the suite, and request review on the new SHA.
# Conflicts: # tests/ci-workflows.test.ts
The route persisted desired ON, then saved the whole long-lived server snapshot -- whose clientIntegrations still said OFF -- back over it, so its post-await guard refused the apply it had just been asked to perform. Persist only the desktopProfile field under the config-mutation lock, and route the writer through the deps seam its sibling path already uses.
setIntegrationEnabled writes disk only, and the server reuses one config object per request, so the native GET reported OFF right after a successful apply and the profile PUT wrote that stale OFF back. Mirror the transition onto the snapshot, and surface an unavailable profile mutation instead of dropping it.
…ccess The route already reported saved:false with a warning when the applied marker failed to persist, but both front ends discarded it and announced a clean save+apply. GUI shows a warn-tone notice (six locales), CLI prints the warning after a successful apply.
2 participants
Summary
Resumes the paused WP13/WP14 tail of the Codex write-substrate campaign: the Codex CLI toggle gets artifact-level restore truth, Claude Desktop gets a real ON/OFF toggle with a documented standard-mode pivot, and a composed acceptance suite proves the production entry points reach the substrate instead of writing around it.
devlog/_plan/260803_codex_desktop_toggle/040as amended by260806_wp13_toggles_resume/010):ocx restore/ejectpersist desired OFF before mutating (crash-durable),restore back/eject backpersist ON;restoreNativeCodexAsyncreturns a per-artifact{config, catalog, history}envelope where history failure carries abusy | permissiondiscriminator instead of being folded into success; everysyncModelsToCodexcaller (startup, ensure, sync, restore dispatch, models, provider,POST /api/sync, toggle enable) branches on a discriminatedapplied/skipped/refusedstatus; desired state is re-read inside each artifact serializer (write lock, history serialization, catalog commit, cache reacquisition) so a lost race becomes a typed skip, never a stale write.050as amended by020):claude-desktopjoinsclientIntegrationsand the native route union; a read-only inspector classifiesnot_installed/standard/gateway_ours/gateway_drifted/foreign/no_owned_state/broken/unsafewith an ownership boundary (owned-but-drifted refuses as unsafe; reads never create files); OFF pivots to a credential-free{}standard profile before removing the opencodex profile, its backup, and its metadata row; all four writer paths (auto-apply, native enable,POST /apply, no-daemon CLI apply) re-read persisted intent after their awaits;desiredEnabledis required on post-commit refusals for every client; GUI switch with six-locale copy.030):tests/codex-composed-acceptance.test.tsruns six scenarios through real spawned CLI children and a real HTTP server on temp homes — entry-to-funnel manifests, lost transition via HTTP, foreign-home zero-artifact refusal, same-user cross-home single lock, Grok OFF surviving a real restart, and restore truth via the newocx restore --json. Building it exposed four real production holes (OFF paths creating artifacts inCODEX_HOME,POST /api/synctrusting the server-captured config, foreign homes accepted over HTTP, opaque busy results), all fixed here. The disposable-host service class (P09/P10/P18/P34-P36) and remaining census rows stay deferred; WP13: composed acceptance at the production boundary #1048 remains open (referenced, not closed).Verification
bun x tsc --noEmit— clean at every commit.bun scripts/test.ts(full suite) — 9242 pass / 8 skip / 1 fail; the single failure (issue-quality workflow rejects workflow_dispatch pull request numbers before mutationintests/ci-workflows.test.ts) is pre-existing onorigin/dev(reproduced on a clean detached worktree ate496e9bfe) and untouched by this branch (git diff origin/dev...HEADtouches no workflow files).bun run lint:gui,bun run privacy:scan,bun run build:gui— green.PUT /api/native-integrations/claude-desktopOFF on anot_installedhome returns an idempotent no-op with zero filesystem footprint and persistedclientIntegrations["claude-desktop"]: false; GET lists all four clients.GUI change screenshot (Integrations overview with the Claude Desktop switch and the four native rows):
Related: #1048 (stays open — workstation subset landed here, service-class scenarios still deferred).
Checklist
260806_wp13_toggles_resume/documents the roadmap, amendments, and deferred rows).Summary by CodeRabbit
New Features
Bug Fixes
Tests