Global settings claude codex - #319
Conversation
|
btw @lilly-luo we can only support claude/codex for global settings right now because the other agents don't have auth self-refresh capabilities ( the token would expire within 1 hr) so the user needs to continue using ucode for the other ones |
When an admin marks an agent machine-wide in `ucode setup`, also write the agent's own native config file (~/.claude/settings.json, ~/.codex/config.toml) so a bare `claude`/`codex` hits the gateway, not just `ucode <agent>`. Only claude and codex qualify — their auth self-refreshes — so the machine-wide prompt is now only asked for those two. Revert surgically prunes just ucode's keys from the native file, and hydrate_state no longer drops the tracking. Co-authored-by: Isaac
Addresses Isaac Review findings on this PR: - mark_tool_managed dropped the persisted native descriptor whenever a re-launch wrote no native file (use_as_global_settings unset, a relayed Claude launch, or a legacy-layout Codex launch). ucode's keys stayed in the user's shared file but revert could no longer find them. Preserve the prior descriptor when native is None (revert wipes state wholesale, so no stale descriptor lingers). - claude revert_native_config path-pruned whole hook-event arrays (hooks.Stop, hooks.PreToolUse/SessionStart/SubagentStart), deleting the user's own hooks. Route hook-event keys through the marker-matched removers (remove_smart_routing_hooks / _remove_tracing_stop_hook), symmetric with the write path; only plain keys go to prune_key_paths. Co-authored-by: Isaac
c617547 to
cb8f9ce
Compare
Under use_as_global_settings, write the agent's OS-level managed settings file so a bare `claude`/`codex` picks up the gateway, not just `ucode <agent>`: - claude: /etc/claude-code/managed-settings.json (mac: /Library/…) — JSON - codex: /etc/codex/managed_config.toml — TOML New managed_files.py mirrors isaac (devtools/ai/llm_lib/core/config.py): a drift check reads the world-readable file with no sudo and no-ops when unchanged (so no password prompt on the common launch), else temp-file -> `sudo cp` with chattr/chflags immutable handling and actionable errors. Revert surgically prunes only ucode's keys via the same sudo path. Renames the transient flag write_native_config -> write_managed_config. Co-authored-by: Isaac
cb8f9ce to
712c8d7
Compare
…dirs Manual testing on an enterprise-managed box surfaced three issues: - read_json_safe/read_toml_safe did an unguarded path.exists(), which raises PermissionError (not a clean "absent") when the file sits under a root-locked dir like a 750 /etc/codex — crashing the whole launch. Guard it -> treat as empty/absent. - managed_files._clear_immutable had the same unguarded path.exists() inside the sudo write, so the write aborted with a misleading "cannot write without root" before the chmod that opens the dir. Guard it; root's `cp` overwrites anyway. - The "enterprise managed settings may override your admin's config" warning fired even when ucode itself authored that managed file under use_as_global_settings. Suppress it (both sites) when ucode owns the file. Co-authored-by: Isaac
Name the exact file each answer writes (Claude Code's managed-settings.json / Codex's managed_config.toml) and spell out the payoff: "yes" means a bare `claude`/`codex` reaches the gateway on its own (no ucode needed), "no" keeps a ucode-only settings file. Co-authored-by: Isaac
Co-authored-by: Isaac
The provider-launch e2e tests only prove routing reaches the Model Provider Service; a real "Credit balance is too low" from the provider account is an environmental condition, not a ucode bug, so skip it like the existing no-permission case rather than failing CI. Generalize _skip_if_no_permission -> _skip_if_provider_unusable to cover both. Co-authored-by: Isaac
| Runs only under use_as_global_settings. The managed file is root-owned and the highest-precedence | ||
| scope, so it applies whether or not `ucode` launches `claude`. The same compose (merge overlay + | ||
| prune stale keys) that produced the private file is applied to the existing managed file, so any | ||
| real IT-authored keys already there survive. The write goes through the isaac-style sudo path |
There was a problem hiding this comment.
can u rm isaac from the comments
| if is_dry_run(): | ||
| console.print(f"\n[bold]\\[dry run] {path} (via sudo)[/bold]\n{desired_text}") | ||
| return "written" |
There was a problem hiding this comment.
i thought we're removing dry run for now?
| write_json_file(CLAUDE_SETTINGS_PATH, _compose(read_json_safe(CLAUDE_SETTINGS_PATH))) | ||
|
|
||
| managed_descriptors = None | ||
| if state.get("write_managed_config"): | ||
| managed_descriptors = _write_managed_settings(_compose, managed_keys, relayed) |
There was a problem hiding this comment.
so does this mean we write to both ~/.claude/settings.json AND /etc/claude-code/managed-settings.json for managed config? i think we should only write to the /etc/claude-code/managed-settings.json if it's managed config
| managed_reverts = { | ||
| "claude": claude_agent.revert_managed_config(state), | ||
| "codex": codex_agent.revert_managed_config(state), | ||
| } |
There was a problem hiding this comment.
do we have to support reverts?
| if len(key) == 2 and key[0] == "hooks" and key[1] in CLAUDE_ROUTING_HOOK_EVENTS: | ||
| touches_routing_hooks = True | ||
| elif len(key) == 2 and key[0] == "hooks" and key[1] == "Stop": | ||
| touches_tracing_stop_hook = True |
There was a problem hiding this comment.
im pretty worried about maintaining these...it feels pretty brittle to rely on the order of hook events. can we just save a copy of the managed config beforehand and upsert it instead of selectively pruning?
| managed_descriptors = None | ||
| if state.get("write_managed_config"): | ||
| managed_descriptors = _write_managed_config( | ||
| workspace, chosen_model, databricks_profile, bool(state.get("use_pat")), provider | ||
| ) | ||
| state = mark_tool_managed(state, "codex", MANAGED_KEYS, native=managed_descriptors) |
There was a problem hiding this comment.
same comment about only updating the managed config vs the local settings instead of both
| """ | ||
| if sys.platform == "darwin" or sys.platform.startswith("linux"): | ||
| return Path("/etc/codex/managed_config.toml") | ||
| if sys.platform.startswith("win"): |
There was a problem hiding this comment.
can we just match on windows. also can we lowercase sys.platform?
| return changed | ||
|
|
||
|
|
||
| def revert_managed_config(state: dict) -> str | None: |
There was a problem hiding this comment.
same comment on just saving what it was before and then upserting that, instead of stripping + pruning
| """Replace ``path`` with ``desired_text`` via sudo (temp file → ``sudo cp``), handling immutability. | ||
|
|
||
| Writes the payload to a user-owned temp file first (no sudo), then copies it into place with | ||
| ``sudo`` and makes it world-readable — the same sequence isaac uses so the file it lays down is |
|
|
||
|
|
||
| def _sudo_replace(path: Path, desired_text: str) -> None: | ||
| """Replace ``path`` with ``desired_text`` via sudo (temp file → ``sudo cp``), handling immutability. |
There was a problem hiding this comment.
you're saying sudo cp doesn't need me to enter my password? 😂
| return False | ||
| except OSError: | ||
| return False | ||
| if sys.platform == "darwin": |
There was a problem hiding this comment.
can we have a set of enums for darwin, linux, etc?
| other agent — a hand-written ``--from-file`` config can't turn it on for an agent that has no | ||
| managed settings path. | ||
| """ | ||
| from ucode.agents import GLOBAL_SETTINGS_AGENTS |
| """Record which config keys ucode manages for ``tool``. | ||
|
|
||
| ``native`` optionally describes the agent's own native config file(s) ucode also wrote under | ||
| ``use_as_global_settings`` — each ``{"path": str, "format": "json"|"toml", "keys": [...]}`` — | ||
| so ``ucode revert`` can surgically prune only ucode's keys from the user's shared file. | ||
|
|
||
| ``native=None`` means "this launch wrote no native file", not "clear the tracking": a later | ||
| launch that skips the native write (the admin unset ``use_as_global_settings``, a relayed Claude | ||
| launch, or a legacy-layout Codex launch) leaves ucode's keys sitting in the user's shared file, so | ||
| the descriptor from the launch that *did* write them must be preserved or ``ucode revert`` can no | ||
| longer find and prune them. ``ucode revert`` wipes state wholesale (``clear_state``), so a stale | ||
| descriptor never lingers past a revert. | ||
| """ |
There was a problem hiding this comment.
can you make this comment shorter
When an admin marks an agent machine-wide in ucode setup (use_as_global_settings), ucode now also writes the agent's
native config file so a bare claude/codex hits the gateway — not just ucode . It keeps writing the existing
ucode-private file too, so ucode is unchanged.
Only claude and codex qualify: their gateway auth self-refreshes, so the native file keeps working indefinitely. Other
agents bake a short-lived token with no bare-launch refresher (copilot has no native config file at all), so the
machine-wide prompt in ucode setup is now only asked for claude and codex.
ucode revert surgically prunes only ucode's keys from the native file — never deletes it, never touches the user's own
settings. Also fixes a latent hydrate_state bug that dropped the native tracking.
31 new tests; full suite green except one pre-existing environmental e2e failure. ruff + ty clean.
globalsettings.mov