From 3d8f985de726106d78922e0264e866913b6c3207 Mon Sep 17 00:00:00 2001 From: Edwin He Date: Mon, 6 Jul 2026 22:07:45 +0000 Subject: [PATCH 1/2] claude: merge a caller's --settings instead of clobbering ucode's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ucode claude` prepends its own `--settings ` to every launch. Claude Code honors only ONE --settings flag, so a caller that also passes --settings (e.g. an integration injecting hooks) has exactly one of the two silently dropped — either ucode's gateway config or the caller's hooks. This broke omnigent's managed Claude sessions: its transcript/permission hooks were dropped and the web UI rendered empty. Compose instead of clobber: when a caller supplies --settings, deep-merge it with ucode's (ucode's gateway keys win; hooks from both are unioned) and hand Claude a single merged --settings (inline JSON). The merge is per-launch and is never written back to the shared ucode settings file, so concurrent launches cannot accumulate one another's hooks. No caller --settings → unchanged (pass ucode's settings file directly). Co-authored-by: Isaac --- src/ucode/agents/claude.py | 122 ++++++++++++++++++++++++++++++++++++- tests/test_agent_claude.py | 82 +++++++++++++++++++++++++ 2 files changed, 203 insertions(+), 1 deletion(-) diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index 1508b87..7603190 100644 --- a/src/ucode/agents/claude.py +++ b/src/ucode/agents/claude.py @@ -2,6 +2,8 @@ from __future__ import annotations +import copy +import json import os import re import shutil @@ -495,12 +497,130 @@ def default_model(state: dict) -> str | None: return claude_models.get("opus") or claude_models.get("sonnet") or claude_models.get("haiku") +def _extract_caller_settings(tool_args: list[str]) -> tuple[list[str], list[str]]: + """Split caller-supplied ``--settings`` values out of *tool_args*. + + Returns ``(values, remaining_args)``, handling both ``--settings `` + and ``--settings=`` spellings. Each value is either a JSON string or + a path to a settings file — Claude Code accepts either. + """ + values: list[str] = [] + remaining: list[str] = [] + i = 0 + while i < len(tool_args): + arg = tool_args[i] + if arg == "--settings" and i + 1 < len(tool_args): + values.append(tool_args[i + 1]) + i += 2 + continue + if arg.startswith("--settings="): + values.append(arg[len("--settings=") :]) + i += 1 + continue + remaining.append(arg) + i += 1 + return values, remaining + + +def _load_caller_settings(value: str) -> dict | None: + """Resolve a ``--settings`` value (inline JSON or file path) to a dict. + + Returns ``None`` when it is neither parseable JSON nor an existing file, so + the caller can pass such a value through untouched instead of dropping it. + """ + text = value.strip() + if text.startswith("{"): + try: + parsed = json.loads(text) + except json.JSONDecodeError: + return None + return parsed if isinstance(parsed, dict) else None + path = Path(text) + if path.exists(): + return read_json_safe(path) + return None + + +def _union_claude_hooks(base: dict, overlay: dict) -> dict: + """Union two Claude Code ``hooks`` maps. + + ``hooks`` is ``{event: [entry, ...]}``. Per event we concatenate the entry + lists so hooks from BOTH settings sources fire, rather than one replacing + the other (which is what a plain deep-merge does to lists). This is what + lets ucode's tracing Stop hook and a caller's own hooks coexist. + """ + result: dict = {} + for event in [*base, *(e for e in overlay if e not in base)]: + entries: list = [] + for src in (base, overlay): + val = src.get(event) + if isinstance(val, list): + entries.extend(val) + result[event] = entries + return result + + +def _merge_claude_settings(base: dict, overlay: dict) -> dict: + """Deep-merge *overlay* onto *base* (overlay wins on conflicting leaves), + but UNION the ``hooks`` so neither side's hooks are dropped. Inputs are not + mutated. + """ + merged = deep_merge_dict(copy.deepcopy(base), overlay) + base_hooks = base.get("hooks") + overlay_hooks = overlay.get("hooks") + if isinstance(base_hooks, dict) or isinstance(overlay_hooks, dict): + merged["hooks"] = _union_claude_hooks( + base_hooks if isinstance(base_hooks, dict) else {}, + overlay_hooks if isinstance(overlay_hooks, dict) else {}, + ) + return merged + + +def _build_claude_argv(binary: str, tool_args: list[str]) -> list[str]: + """Build the ``claude`` argv, composing any caller ``--settings`` with + ucode's managed settings. + + ucode needs its own settings (gateway ``apiKeyHelper`` + env) to reach + Claude, and normally passes ``--settings ``. But Claude Code + honors only ONE ``--settings`` flag, so a caller that ALSO passes + ``--settings`` (e.g. an integration injecting hooks) would have exactly one + of the two silently dropped. To let ucode compose with any prior command, + we merge a caller-supplied ``--settings`` with ucode's — ucode's gateway + keys win, hooks from both are unioned — and hand Claude a single merged + ``--settings`` (inline JSON). The merge is per-launch and is never written + back to the shared ucode settings file, so concurrent launches cannot + accumulate one another's hooks. + """ + caller_values, remaining = _extract_caller_settings(tool_args) + if not caller_values: + # No caller --settings: hand Claude ucode's settings file directly (the + # common path; behavior unchanged). + return [binary, "--settings", str(CLAUDE_SETTINGS_PATH), *tool_args] + caller_settings: dict = {} + unparsed: list[str] = [] + for value in caller_values: + parsed = _load_caller_settings(value) + if parsed is None: + unparsed.append(value) + continue + caller_settings = _merge_claude_settings(caller_settings, parsed) + # ucode wins over the caller for conflicting keys (protects gateway auth); + # hooks from both sides survive. + merged = _merge_claude_settings(caller_settings, read_json_safe(CLAUDE_SETTINGS_PATH)) + argv = [binary, "--settings", json.dumps(merged, separators=(",", ":")), *remaining] + # Anything we could not parse (not JSON, not an existing file) is passed + # through untouched rather than silently dropped. + for value in unparsed: + argv.extend(["--settings", value]) + return argv + + def launch(state: dict, tool_args: list[str]) -> None: binary = SPEC["binary"] workspace = state.get("workspace") if workspace: os.environ["OAUTH_TOKEN"] = get_databricks_token(workspace, state.get("profile")) - exec_or_spawn([binary, "--settings", str(CLAUDE_SETTINGS_PATH), *tool_args]) + exec_or_spawn(_build_claude_argv(binary, tool_args)) def validate_cmd(binary: str) -> list[str]: diff --git a/tests/test_agent_claude.py b/tests/test_agent_claude.py index 838cf36..ad525d0 100644 --- a/tests/test_agent_claude.py +++ b/tests/test_agent_claude.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json import os from ucode.agents import claude @@ -523,3 +524,84 @@ def test_prunes_stale_name_companion_keys_from_older_ucode(self, monkeypatch): assert "ANTHROPIC_DEFAULT_OPUS_MODEL_NAME" not in env assert "ANTHROPIC_DEFAULT_SONNET_MODEL_NAME" not in env assert "ANTHROPIC_DEFAULT_HAIKU_MODEL_NAME" not in env + + +class TestBuildClaudeArgv: + def test_no_caller_settings_uses_ucode_file(self, monkeypatch): + monkeypatch.setattr(claude, "read_json_safe", lambda p: {"apiKeyHelper": "u"}) + argv = claude._build_claude_argv("claude", ["-p", "hi"]) + assert argv == ["claude", "--settings", str(claude.CLAUDE_SETTINGS_PATH), "-p", "hi"] + + def test_inline_caller_settings_merged_into_single_flag(self, monkeypatch): + ucode_settings = { + "apiKeyHelper": "ucode-helper", + "env": {"ANTHROPIC_BASE_URL": "https://gw"}, + "hooks": {"Stop": [{"hooks": [{"type": "command", "command": "ucode-stop"}]}]}, + } + monkeypatch.setattr(claude, "read_json_safe", lambda p: ucode_settings) + caller = json.dumps( + { + "hooks": {"Stop": [{"hooks": [{"type": "command", "command": "caller-stop"}]}]}, + "statusLine": {"type": "command", "command": "sl"}, + } + ) + argv = claude._build_claude_argv("claude", ["--settings", caller, "-p", "hi"]) + # Exactly one --settings reaches Claude, and the caller's raw flag is gone. + assert argv.count("--settings") == 1 + assert argv[:2] == ["claude", "--settings"] + assert argv[3:] == ["-p", "hi"] + merged = json.loads(argv[2]) + # ucode's gateway config survives. + assert merged["apiKeyHelper"] == "ucode-helper" + assert merged["env"]["ANTHROPIC_BASE_URL"] == "https://gw" + # The caller's own (non-hook) settings pass through. + assert merged["statusLine"] == {"type": "command", "command": "sl"} + # Hooks from BOTH sides fire (unioned, not clobbered). + stop_cmds = [h["command"] for e in merged["hooks"]["Stop"] for h in e["hooks"]] + assert "ucode-stop" in stop_cmds + assert "caller-stop" in stop_cmds + + def test_equals_form_is_handled(self, monkeypatch): + monkeypatch.setattr(claude, "read_json_safe", lambda p: {"apiKeyHelper": "u"}) + caller = json.dumps({"hooks": {"Stop": [{"hooks": [{"type": "command", "command": "c"}]}]}}) + argv = claude._build_claude_argv("claude", [f"--settings={caller}"]) + assert argv.count("--settings") == 1 + merged = json.loads(argv[2]) + assert merged["apiKeyHelper"] == "u" + assert merged["hooks"]["Stop"][0]["hooks"][0]["command"] == "c" + + def test_ucode_wins_on_conflicting_env(self, monkeypatch): + monkeypatch.setattr( + claude, "read_json_safe", lambda p: {"env": {"ANTHROPIC_BASE_URL": "https://ucode"}} + ) + caller = json.dumps({"env": {"ANTHROPIC_BASE_URL": "https://caller", "FOO": "bar"}}) + argv = claude._build_claude_argv("claude", ["--settings", caller]) + merged = json.loads(argv[2]) + assert merged["env"]["ANTHROPIC_BASE_URL"] == "https://ucode" # ucode wins + assert merged["env"]["FOO"] == "bar" # caller's non-conflicting key kept + + def test_file_path_caller_settings(self, tmp_path, monkeypatch): + caller_file = tmp_path / "caller.json" + caller_file.write_text( + json.dumps( + {"hooks": {"SessionStart": [{"hooks": [{"type": "command", "command": "cs"}]}]}} + ) + ) + + def fake_read(p): + if str(p) == str(claude.CLAUDE_SETTINGS_PATH): + return {"apiKeyHelper": "u"} + return json.loads(p.read_text()) + + monkeypatch.setattr(claude, "read_json_safe", fake_read) + argv = claude._build_claude_argv("claude", ["--settings", str(caller_file)]) + assert argv.count("--settings") == 1 + merged = json.loads(argv[2]) + assert merged["apiKeyHelper"] == "u" + assert merged["hooks"]["SessionStart"][0]["hooks"][0]["command"] == "cs" + + def test_unparseable_value_passed_through(self, monkeypatch): + monkeypatch.setattr(claude, "read_json_safe", lambda p: {"apiKeyHelper": "u"}) + argv = claude._build_claude_argv("claude", ["--settings", "not-json-not-a-file"]) + # Could not parse → passed through untouched rather than dropped. + assert "not-json-not-a-file" in argv From 83f172d29b6caf1b2d00b658e9b97c29c57ec2ce Mon Sep 17 00:00:00 2001 From: Edwin He Date: Wed, 8 Jul 2026 16:29:50 +0000 Subject: [PATCH 2/2] claude: fail loudly on an unresolvable --settings value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review on #189: a caller --settings value that ucode could not parse was silently swallowed (returned None) and then passed through as a second --settings flag. Claude Code honors only one --settings, so that collision silently dropped either the caller's settings or ucode's gateway config (breaking auth). Raise an actionable RuntimeError instead — malformed inline JSON, a malformed/unreadable settings file, a nonexistent file path, or non-object JSON — surfaced by the CLI as a clean error. Removes the pass-through branch, which never helped (the colliding flag always loses). Co-authored-by: Isaac --- src/ucode/agents/claude.py | 59 ++++++++++++++++++++++---------------- tests/test_agent_claude.py | 43 ++++++++++++++++++++------- 2 files changed, 67 insertions(+), 35 deletions(-) diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index 7603190..69b0168 100644 --- a/src/ucode/agents/claude.py +++ b/src/ucode/agents/claude.py @@ -522,23 +522,42 @@ def _extract_caller_settings(tool_args: list[str]) -> tuple[list[str], list[str] return values, remaining -def _load_caller_settings(value: str) -> dict | None: +def _load_caller_settings(value: str) -> dict: """Resolve a ``--settings`` value (inline JSON or file path) to a dict. - Returns ``None`` when it is neither parseable JSON nor an existing file, so - the caller can pass such a value through untouched instead of dropping it. + Claude Code accepts either inline JSON or a path to a JSON file. Raises + ``RuntimeError`` (surfaced by the CLI as an actionable error) when the value + is neither, rather than silently dropping it: a dropped value would also be + passed through as a second ``--settings`` flag, and Claude Code honors only + one — so either the caller's settings or ucode's gateway config would be + silently ignored. Failing loudly lets the caller fix their input. """ text = value.strip() if text.startswith("{"): + source, malformed = text, "value is not valid JSON" + else: + path = Path(text) + if not path.exists(): + raise RuntimeError( + f"--settings file not found: {value!r}. " + "Pass inline JSON or a path to an existing JSON file." + ) try: - parsed = json.loads(text) - except json.JSONDecodeError: - return None - return parsed if isinstance(parsed, dict) else None - path = Path(text) - if path.exists(): - return read_json_safe(path) - return None + source = path.read_text(encoding="utf-8") + except OSError as exc: + raise RuntimeError(f"--settings file could not be read: {value!r} ({exc}).") from exc + malformed = "file is not valid JSON" + try: + parsed = json.loads(source) + except json.JSONDecodeError as exc: + raise RuntimeError( + f"--settings {malformed} ({exc}): {value!r}. Pass inline JSON or a path to a JSON file." + ) from exc + if not isinstance(parsed, dict): + raise RuntimeError( + f"--settings must be a JSON object, got {type(parsed).__name__}: {value!r}." + ) + return parsed def _union_claude_hooks(base: dict, overlay: dict) -> dict: @@ -589,7 +608,9 @@ def _build_claude_argv(binary: str, tool_args: list[str]) -> list[str]: keys win, hooks from both are unioned — and hand Claude a single merged ``--settings`` (inline JSON). The merge is per-launch and is never written back to the shared ucode settings file, so concurrent launches cannot - accumulate one another's hooks. + accumulate one another's hooks. A caller ``--settings`` value ucode cannot + resolve raises (see :func:`_load_caller_settings`) rather than being passed + through as a second, colliding flag. """ caller_values, remaining = _extract_caller_settings(tool_args) if not caller_values: @@ -597,22 +618,12 @@ def _build_claude_argv(binary: str, tool_args: list[str]) -> list[str]: # common path; behavior unchanged). return [binary, "--settings", str(CLAUDE_SETTINGS_PATH), *tool_args] caller_settings: dict = {} - unparsed: list[str] = [] for value in caller_values: - parsed = _load_caller_settings(value) - if parsed is None: - unparsed.append(value) - continue - caller_settings = _merge_claude_settings(caller_settings, parsed) + caller_settings = _merge_claude_settings(caller_settings, _load_caller_settings(value)) # ucode wins over the caller for conflicting keys (protects gateway auth); # hooks from both sides survive. merged = _merge_claude_settings(caller_settings, read_json_safe(CLAUDE_SETTINGS_PATH)) - argv = [binary, "--settings", json.dumps(merged, separators=(",", ":")), *remaining] - # Anything we could not parse (not JSON, not an existing file) is passed - # through untouched rather than silently dropped. - for value in unparsed: - argv.extend(["--settings", value]) - return argv + return [binary, "--settings", json.dumps(merged, separators=(",", ":")), *remaining] def launch(state: dict, tool_args: list[str]) -> None: diff --git a/tests/test_agent_claude.py b/tests/test_agent_claude.py index ad525d0..46151bf 100644 --- a/tests/test_agent_claude.py +++ b/tests/test_agent_claude.py @@ -5,6 +5,8 @@ import json import os +import pytest + from ucode.agents import claude WS = "https://example.databricks.com" @@ -587,21 +589,40 @@ def test_file_path_caller_settings(self, tmp_path, monkeypatch): {"hooks": {"SessionStart": [{"hooks": [{"type": "command", "command": "cs"}]}]}} ) ) - - def fake_read(p): - if str(p) == str(claude.CLAUDE_SETTINGS_PATH): - return {"apiKeyHelper": "u"} - return json.loads(p.read_text()) - - monkeypatch.setattr(claude, "read_json_safe", fake_read) + # The caller file is read directly; read_json_safe is only used for + # ucode's own settings file. + monkeypatch.setattr(claude, "read_json_safe", lambda p: {"apiKeyHelper": "u"}) argv = claude._build_claude_argv("claude", ["--settings", str(caller_file)]) assert argv.count("--settings") == 1 merged = json.loads(argv[2]) assert merged["apiKeyHelper"] == "u" assert merged["hooks"]["SessionStart"][0]["hooks"][0]["command"] == "cs" - def test_unparseable_value_passed_through(self, monkeypatch): + def test_malformed_inline_json_raises(self, monkeypatch): + monkeypatch.setattr(claude, "read_json_safe", lambda p: {"apiKeyHelper": "u"}) + # Clearly-intended-as-JSON but broken: fail loudly rather than pass it + # through as a second, colliding --settings flag. + with pytest.raises(RuntimeError, match="not valid JSON"): + claude._build_claude_argv("claude", ["--settings", '{"hooks": ']) + + def test_nonexistent_file_raises(self, monkeypatch): + monkeypatch.setattr(claude, "read_json_safe", lambda p: {"apiKeyHelper": "u"}) + with pytest.raises(RuntimeError, match="file not found"): + claude._build_claude_argv("claude", ["--settings", "/no/such/settings.json"]) + + def test_non_object_file_json_raises(self, tmp_path, monkeypatch): + # A --settings file whose JSON is not an object (e.g. an array) can't be + # merged; fail loudly. (An inline value only enters the JSON branch when + # it starts with "{", so the non-object case is reachable via a file.) + bad_file = tmp_path / "bad.json" + bad_file.write_text("[1, 2, 3]") + monkeypatch.setattr(claude, "read_json_safe", lambda p: {"apiKeyHelper": "u"}) + with pytest.raises(RuntimeError, match="must be a JSON object"): + claude._build_claude_argv("claude", ["--settings", str(bad_file)]) + + def test_malformed_file_json_raises(self, tmp_path, monkeypatch): + bad_file = tmp_path / "bad.json" + bad_file.write_text('{"hooks": ') monkeypatch.setattr(claude, "read_json_safe", lambda p: {"apiKeyHelper": "u"}) - argv = claude._build_claude_argv("claude", ["--settings", "not-json-not-a-file"]) - # Could not parse → passed through untouched rather than dropped. - assert "not-json-not-a-file" in argv + with pytest.raises(RuntimeError, match="not valid JSON"): + claude._build_claude_argv("claude", ["--settings", str(bad_file)])