claude: merge a caller's --settings instead of clobbering ucode's (#189)

* claude: merge a caller's --settings instead of clobbering ucode's

`ucode claude` prepends its own `--settings <ucode-file>` 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

* claude: fail loudly on an unresolvable --settings value

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

Author: Edwinhe03

src/ucode/agents/claude.py

@@ -2,6 +2,8 @@
 
 from __future__ import annotations
 
+import copy
+import json
 import os
 import re
 import shutil
@@ -503,12 +505,141 @@ 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 <value>``
+    and ``--settings=<value>`` 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:
+    """Resolve a ``--settings`` value (inline JSON or file path) to a dict.
+
+    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:
+            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:
+    """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 <ucode-file>``. 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. 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:
+        # 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 = {}
+    for value in caller_values:
+        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))
+    return [binary, "--settings", json.dumps(merged, separators=(",", ":")), *remaining]
+
+
 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]:

tests/test_agent_claude.py

@@ -2,8 +2,11 @@
 
 from __future__ import annotations
 
+import json
 import os
 
+import pytest
+
 from ucode.agents import claude
 
 WS = "https://example.databricks.com"
@@ -525,3 +528,103 @@ 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"}]}]}}
+            )
+        )
+        # 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_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"})
+        with pytest.raises(RuntimeError, match="not valid JSON"):
+            claude._build_claude_argv("claude", ["--settings", str(bad_file)])