diff --git a/src/ucode/agents/claude.py b/src/ucode/agents/claude.py index 58eea4c..422e763 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 @@ -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 `` + 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: + """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 ``. 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]: diff --git a/tests/test_agent_claude.py b/tests/test_agent_claude.py index f267085..b2d3a27 100644 --- a/tests/test_agent_claude.py +++ b/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)])