fix(cli): sync proxy env to ~/.claude/settings.json for agent workers - #881
fix(cli): sync proxy env to ~/.claude/settings.json for agent workers#881SebasSotoA wants to merge 8 commits into
Conversation
fcc-claude only injected ANTHROPIC_* env into the launched process. claude agents spawns daemon workers as plain claude with empty env, causing /login prompts. Auto-sync FCC proxy settings into the Claude Code settings.json env block on fcc-claude launch and admin apply.
- Strip stale ANTHROPIC_* env keys before writing FCC proxy values - Make sync best-effort: skip on invalid paths/content instead of raising - Restrict settings file permissions on Unix (0o600) - Defer admin apply sync when PORT changes until proxy restart - Patch sync in launcher tests to avoid mutating real ~/.claude
|
Addressed all Greptile review annotations in bbe28e4:
Added tests for stale-key stripping, skip-on-invalid-json, directory path, PORT deferral, and admin sync gating. |
- Defer sync on HOST changes in addition to PORT - Use NamedTemporaryFile for atomic writes to avoid temp-file collisions - Harden file permissions on no-op (already up-to-date) sync path - Always call sync_claude_settings in admin apply (remove deferral logic) - Update and add tests for all four changes
Addressing Greptile 1/5 review (commit 03b8acb)All five issues from the latest review have been fixed: 1. Mixed applies skip sync ( 2. Defer HOST changes too ( 3. Unique temp files ( 4. Sync after restart ( 5. Harden existing files ( Test results: 58 passed, 2 skipped (Unix chmod tests correctly skipped on Windows). @greptileai please re-review. |
…eferral, env types
Addressing Greptile 4/5 review (commit 9ca3f20)All four issues from the latest review have been fixed: 1. Preserve symlinked settings ( 2. Clean failed temp files ( 3. Avoid pre-restart sync for PORT-only changes (
4. Preserve non-string env entries ( Test results: 37 passed, 3 skipped (Unix-only tests correctly skipped on Windows). @greptileai please re-review. |
Addressing the 2 remaining considerations (commit bfd99ec)Both considerations were rooted in the settings-sync deferral guard being too narrow. They are now fixed together, plus a supervisor-side sync that closes the loop so deferred changes are never left stale. 1. Check pending restarts across applies ( pending_now = set(result["pending_fields"])
pending_before = set(getattr(request.app.state, "admin_pending_fields", []))
restart_pending = (pending_now | pending_before) & {"HOST", "PORT"}A token-only apply while a 2. Defer on actual changed fields, not submitted keys ( Closing the loop: sync on every server start ( settings = get_settings()
_sync_claude_settings_on_start(settings)This means deferred Updated behavior matrix:
Test results: 63 passed, 3 skipped (Unix-only tests skipped on Windows). @greptileai please re-review. |
Revised approach: always sync the live address + current token (commit 2fbb799)The previous "defer on pending HOST/PORT restart" approach had two flaws (found in local review):
Both stem from the URL and the token living in the same
live_proxy_root_url = getattr(request.app.state, "live_proxy_root_url", None) or local_proxy_root_url(fresh_settings)
sync_claude_settings(proxy_root_url=live_proxy_root_url, auth_token=fresh_settings.anthropic_auth_token)
Behavior matrix:
Tests: the three deferral tests were replaced with live-URL assertions ( @greptileai please re-review. |
Addressing Greptile P1: avoid persisting shell tokens (commit e0b2870)A process-environment Fix: guard every Three sites guarded:
Normal flow unchanged: when New tests:
Test results: 187 passed, 3 skipped (Unix-only tests skipped on Windows). @greptileai please re-review. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Addressing Bugbot medium: sync deferred until server is reachable (commit 7fcb5e8)The previous approach called Fix: the settings sync is now deferred into the existing "server ready" daemon thread that already gates on
The old pre-bind call in Updated tests:
Test results: 66 passed, 3 skipped on affected files; 372 passed across @greptileai please re-review. |
| merged_env: dict[str, object] = { | ||
| key: value | ||
| for key, value in existing_env.items() | ||
| if isinstance(key, str) | ||
| and not key.startswith("ANTHROPIC_") | ||
| and key not in FCC_CLAUDE_ENV_KEYS | ||
| } |
There was a problem hiding this comment.
This merge removes every existing ANTHROPIC_* setting, not just the FCC-managed proxy keys or stale auth endpoint keys. If a user stores another Claude/Anthropic option in ~/.claude/settings.json, such as a model or feature flag env var, running fcc-claude silently deletes it and later plain claude agent workers start with different settings. Limit the removal to the keys this sync owns or known conflicting auth/base-url keys.
Summary
Test plan
Fixes #880
Greptile Summary
This PR defers
~/.claude/settings.jsonwrites until the proxy server is confirmed reachable, so a failed bind no longer corrupts agent worker config. It also unifies the admin-browser-open and settings-sync paths into a single_schedule_on_server_readydaemon thread._schedule_on_server_readyreplaces_schedule_open_admin_browser; it polls/healthfor up to 30 s before syncing or opening the browser, so a port-in-use error leavessettings.jsonintact.app.state.live_proxy_root_url) and skips sync when the token comes only from the process environment, preventing shell-only secrets from being persisted.Confidence Score: 5/5
The core correctness goal — never writing settings.json before the server is confirmed live — is achieved cleanly; the only finding is an unused helper that should be removed.
The pre-bind sync problem is correctly solved: the daemon thread gates all writes on a successful /health poll, admin apply uses the captured live URL rather than the pending new address, and the process-token guard prevents shell secrets from being persisted. Test coverage is thorough across the new sync paths. The one leftover is should_defer_claude_settings_sync, which is defined, exported, and tested but no longer called from any production code — a dead-code cleanup, not a correctness issue.
cli/claude_settings_sync.py — should_defer_claude_settings_sync and its test in tests/cli/test_claude_settings_sync.py can be removed since the production call site was eliminated in this PR.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant serve as serve() loop participant rss as _run_supervised_server participant thread as fcc-server-ready thread participant health as /health participant sync as sync_claude_settings participant fs as ~/.claude/settings.json serve->>rss: "open_admin_browser=True" rss->>rss: set app.state.live_proxy_root_url rss->>thread: _schedule_on_server_ready(settings) rss->>rss: server.run() [blocks] thread->>health: preflight_proxy() poll (30s) health-->>thread: None (reachable) thread->>sync: proxy_root_url, auth_token sync->>fs: atomic write (temp + replace) Note over serve,fs: Admin apply path participant admin as /admin/api/config/apply admin->>admin: write_managed_env() admin->>admin: cache_clear() + get fresh settings admin->>sync: live_proxy_root_url (not pending port) sync->>fs: atomic write%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant serve as serve() loop participant rss as _run_supervised_server participant thread as fcc-server-ready thread participant health as /health participant sync as sync_claude_settings participant fs as ~/.claude/settings.json serve->>rss: "open_admin_browser=True" rss->>rss: set app.state.live_proxy_root_url rss->>thread: _schedule_on_server_ready(settings) rss->>rss: server.run() [blocks] thread->>health: preflight_proxy() poll (30s) health-->>thread: None (reachable) thread->>sync: proxy_root_url, auth_token sync->>fs: atomic write (temp + replace) Note over serve,fs: Admin apply path participant admin as /admin/api/config/apply admin->>admin: write_managed_env() admin->>admin: cache_clear() + get fresh settings admin->>sync: live_proxy_root_url (not pending port) sync->>fs: atomic writeComments Outside Diff (4)
tests/cli/test_entrypoints.py, line 510-524 (link)launch([])calls the realsync_claude_settings()before reaching the patchedPopenpath, and the command-resolution failure test also writes settings beforeresolve_client_binary()returnsNone. These tests do not redirectHOMEor patch the sync function, so running them can mutate the test runner's real~/.claude/settings.jsonwhile they are only trying to assert launch behavior. Patchcli.launchers.claude.sync_claude_settingsor redirectHOMEfor these test cases.Artifacts
Repro: focused launcher harness mirroring the affected test without patching settings sync
Repro: harness output showing fake HOME settings.json creation and synced env contents
api/admin_routes.py, line 144 (link)admin_pending_fields. If a HOST/PORT apply is followed by another apply while the old server is still running, or if the callback fails/cannot bind the new address, later token-only applies see no pending restart and can sync~/.claude/settings.jsonto the not-yet-serving address. Keep the pending restart state until the supervisor has confirmed the restart, or track a restart-in-progress state that still blocks settings sync.api/admin_routes.py, line 141-145 (link)admin_pending_fieldsimmediately after scheduling the background callback, before_invoke_admin_restart_callback()runs. If that callback raises or fails to make the supervisor restart, or if another Admin apply is processed during that window, the sync guard no longer sees the pendingHOST/PORTrestart and can write~/.claude/settings.jsonwith the new address even though the old server is still the one serving traffic. Keep the pending fields until the restart callback completes and the new server is live, or track a separate restart-in-progress state for the sync guard.Artifacts
Repro: executable admin apply harness with failing restart callback and isolated HOME
Repro: command output showing callback failure, cleared pending fields, and settings.json written with new port
api/app.py, line 101-105 (link)Avoid guessed live URL. This stores a proxy URL derived from
Settings, but the supported direct ASGI path can bind uvicorn with CLI overrides, for exampleuvicorn server:app --port 9090while the config still says8082. Admin apply later trustsapp.state.live_proxy_root_urland syncs~/.claude/settings.jsonto the configured port, so agent workers can be pointed at a proxy URL that is not actually serving. Use an address captured from the serving path/request context, or skip this fallback when the supervisor did not set the live URL.Artifacts
Repro: focused runtime harness for direct ASGI admin apply live URL mismatch
Repro: runtime output showing HTTP 200 OK and synced configured-port URL despite actual request port 9090
Reviews (14): Last reviewed commit: "fix: defer settings sync until server is..." | Re-trigger Greptile