Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: luvs01/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
|
✅ Deterministic PR hygiene checks passed. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…e on CLI opt-out observeClaudeDesktopMode suppresses an owned shared env while cliFirstParty is set, so an opt-out that pinned from the pre-mutation observation resolved gateway and removed a proxy a legacy Desktop install could still be using. The pin now observes with the flag cleared — the same state Desktop inference sees after the write — so an owned env pins first-party and survives, while an env that is not ours still pins gateway. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A CLI-only env established outside the flag flow (hand-configured cliFirstParty plus an env written by ocx ensure) is indistinguishable from a legacy Desktop first-party install — the shared env carries no client marker. Retaining it would silently keep interception on after opt-out, so the success response now warns 'shared_proxy_retained' when the retained env's ownership was ambiguous, telling the operator to pin claudeCode.desktopMode explicitly to release it. Co-Authored-By: Epinephrine <luvs01@hanmail.net>
| warnings: [ | ||
| ...(committed.retainedAmbiguous ? ["shared_proxy_retained"] : []), | ||
| ...(residual ? ["settings_residual"] : []), |
There was a problem hiding this comment.
🟡 CLI users never see the retained-proxy warning
When CLI opt-out retains an ambiguous proxy, the API returns a warning. The CLI command prints only a fixed success message, so operators never see the remedy.
Learn more
The ocx claude config set --first-party off command calls this API, but handleClaudeConfigCommand passes a fixed success line to printData for non-JSON output. That line hides shared_proxy_retained, although the warning is meant to tell operators their shared proxy is still active. The dashboard also discards the successful PUT body in toggleFirstParty; neither interactive client presents the warning.
Example: With cliFirstParty: true, no Desktop mode marker, and an owned shared proxy, opt-out pins Desktop to first-party. The API responds with warnings: ["shared_proxy_retained"], but the CLI displays only “Claude Code settings updated.”
Recommended fix: Render recognized warnings from successful responses in handleClaudeConfigCommand and toggleFirstParty, with localized dashboard copy explaining how to select Desktop gateway mode. Preserve the structured warnings for JSON callers.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // An opt-out that pins first-party from the shared env cannot tell whether that env was | ||
| // Desktop's or a hand-configured CLI-only one; the env is retained (Desktop keeps its | ||
| // route) and the caller is warned so it can pin gateway explicitly to release it. | ||
| const retainedAmbiguous = !body.cliFirstParty && previous.value && pinnedMode === "first-party"; | ||
| return { changed: true, value: { claudeCode: structuredClone(persisted.claudeCode), previous, pinnedMode, retainedAmbiguous } }; |
There was a problem hiding this comment.
🟡 Removed proxy is reported as retained when Desktop integration is off
With Desktop integration disabled, CLI opt-out removes the shared proxy through reconciliation. The response still warns that the proxy was retained, misleading callers about its actual state.
Learn more
The warning is computed from the previous CLI flag and pinned Desktop mode, not from whether Desktop integration is enabled or the final reconciliation result. desktopFirstPartyDesired returns false when the integration is disabled; reconcileClaudeFirstPartySettings then removes the owned env because neither Desktop nor CLI wants it. The successful response nevertheless claims it was retained.
Example: Set clientIntegrations['claude-desktop'] = false, cliFirstParty = true, leave desktopMode absent, and install an owned proxy env. PUT cliFirstParty: false pins first-party but removes the env; the response still includes shared_proxy_retained.
Recommended fix: Only emit shared_proxy_retained when the post-mutation Desktop integration actually desires first-party and the proxy remains present after reconciliation; add a focused test covering disabled Desktop integration.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
이관됨: lidge-jun#6046 |
|
동일 수정이 상류 저장소에 제출되어 이 포크 PR의 목적은 달성됐습니다. |
1 participant
Motivation
Description
cliFirstPartyis changed so an absentdesktopModeis pinned before writing or removing the shared settings env. (change insrc/server/management/agent-settings-routes.ts).tests/claude-integration/claude-management-api.test.ts).structure/config.md).Testing
bun test tests/claude-integration/claude-management-api.test.tswhich passed (55 tests in that suite passed).bun run typecheckwhich completed successfully.bun run structure:checkwhich completed successfully.bun run privacy:scanwhich completed successfully.Codex Task