diff --git a/_provider.py b/_provider.py index da058cb..190fd3e 100644 --- a/_provider.py +++ b/_provider.py @@ -112,11 +112,35 @@ def _is_our_entry(entry: Any) -> bool: return _is_our_base_url(base) +def _entries_equivalent(a: Any, b: Any) -> bool: + """True if two provider entries are semantically equivalent. + + ``register_provider`` rebuilds an entry in memory and only saves it when + it differs from what's already in config, so a no-op plugin load doesn't + rewrite config.yaml (which would strip its comments). Treats the ``"***"`` + redaction sentinel and the canonical ``"no-key-required"`` keyless value + as equal (``hermes_cli`` normalises the former to the latter on save). + """ + if not isinstance(a, dict) or not isinstance(b, dict): + return False + a_key = a.get("api_key") + b_key = b.get("api_key") + if a_key in ("***", "no-key-required") and b_key in ("***", "no-key-required"): + a = {**a, "api_key": "no-key-required"} + b = {**b, "api_key": "no-key-required"} + return a == b + + def register_provider(port: int) -> bool: - """Write a custom provider entry for the builder adapter. + """Ensure the builder adapter's provider entry exists in config.yaml. - Returns True if an entry was written, False if it was skipped (e.g. - config module unavailable). Does not change the user's current model. + Returns True when the entry is present and correct (written now or + already current), False when it is skipped (config unavailable, or a + user-managed entry at our slug that must not be clobbered). + + The write is a full ``load_config()``/``save_config()`` round-trip that + strips comments, so it is skipped when the entry is already equivalent — + ``setup.sh`` (line-based, comment-preserving) is the primary writer. """ try: from hermes_cli.config import load_config, save_config @@ -124,12 +148,16 @@ def register_provider(port: int) -> bool: logger.warning("builder: cannot import hermes_cli.config (%s)", exc) return False - models = _declared_models() or [ - "auto", - "claude-sonnet-4.5", - "claude-sonnet-4", - "claude-haiku-4.5", - ] + models = _declared_models() + if not models: + # _declared_models() already falls back to backend.STATIC_MODELS via + # backend.list_models(); an empty result means backend is unavailable, + # so there is nothing to advertise. Avoid a third hardcoded copy of the + # catalog here — the single fallback source is STATIC_MODELS. + logger.warning( + "builder: no models available (backend unavailable); skipping provider registration" + ) + return False default_model = models[0] # Best-effort: never let a malformed/unreadable config abort plugin @@ -164,6 +192,7 @@ def register_provider(port: int) -> bool: # wrote under the old slug so existing config isn't orphaned and # unregister_provider still finds it. _LEGACY_SLUG = "aws-build" + changed = False legacy = providers.get(_LEGACY_SLUG) if isinstance(legacy, dict) and _is_our_entry(legacy): logger.info( @@ -172,6 +201,7 @@ def register_provider(port: int) -> bool: providers.pop(_LEGACY_SLUG, None) if not isinstance(providers.get(PROVIDER_SLUG), dict): providers[PROVIDER_SLUG] = legacy + changed = True existing = providers.get(PROVIDER_SLUG) @@ -223,8 +253,18 @@ def _is_user_managed(entry: Any) -> bool: } ) entry["models"] = {m: {} for m in models} - providers[PROVIDER_SLUG] = entry + # Idempotency (comment preservation): skip the save when the rebuilt + # entry is already equivalent to config AND no legacy migration happened. + # load_config()/save_config() is a full YAML round-trip that strips every + # comment, so rewriting on every plugin load would destroy user comments. + if _entries_equivalent(entry, existing) and not changed: + logger.info( + "builder: provider '%s' already current; skipping write", PROVIDER_SLUG + ) + return True + + providers[PROVIDER_SLUG] = entry try: save_config(config) except Exception as exc: # noqa: BLE001 diff --git a/scripts/setup.sh b/scripts/setup.sh index b00f03f..bba0dcc 100755 --- a/scripts/setup.sh +++ b/scripts/setup.sh @@ -100,6 +100,12 @@ models = manifest.get("models") or ["auto"] # exact strings; backend.list_models() already does this coercion for the # same reason. models = [str(m) for m in models] +# Derive the default model from the first declared model, matching +# register_provider() at runtime. A custom catalog without "auto" must still +# advertise a default that is present in its own models block — hardcoding +# "auto" would select a model the provider does not actually offer. +model = models[0] +model_scalar = yaml.dump(model, default_style='"').splitlines()[0] # Use yaml.dump for serialized model identifiers to ensure proper scalar # serialization. This handles edge cases like embedded quotes, newlines, # and YAML-special characters that would break the config or silently alter @@ -108,7 +114,7 @@ models = [str(m) for m in models] # We include the "models:" wrapper key in the dump so the indentation is # handled correctly by yaml itself. model_mapping = {m: {} for m in models} -model_yaml = yaml.dump({"models": model_mapping}, default_flow_style=False) +model_yaml = yaml.dump({"models": model_mapping}, default_flow_style=False, sort_keys=False) model_lines = model_yaml.rstrip("\n").splitlines() # Indent each line by 4 spaces so the block sits correctly under aws-builder model_lines = [" " + ln for ln in model_lines] @@ -118,7 +124,8 @@ lines = [ " transport: openai_chat", f" base_url: http://localhost:{port}/v1", " api_key: no-key-required", - " model: \"auto\"", + f" model: {model_scalar}", + " discover_models: false", ] lines.extend(model_lines) diff --git a/tests/test_coverage_gaps.py b/tests/test_coverage_gaps.py index 758e9e8..3d6ca21 100644 --- a/tests/test_coverage_gaps.py +++ b/tests/test_coverage_gaps.py @@ -232,6 +232,51 @@ def test_provider_register_skips_user_managed(monkeypatch): assert result is False +def test_provider_register_noop_when_already_current(monkeypatch): + """register_provider must NOT rewrite config.yaml when the entry already + matches what it would write — a no-op load must not strip comments.""" + import sys + + import _provider + from _provider import _declared_models + + models = [str(m) for m in _declared_models()] + existing = { + "name": "AWS Builder", + "transport": "openai_chat", + "base_url": "http://localhost:8088/v1", + "model": models[0], + "discover_models": False, + "api_key": "***", + "models": {m: {} for m in models}, + } + cfg = {"providers": {_provider.PROVIDER_SLUG: existing}} + fake_hermes, fake_cfg, saved = _make_hermes_cli_mock(cfg) + monkeypatch.setitem(sys.modules, "hermes_cli", fake_hermes) + monkeypatch.setitem(sys.modules, "hermes_cli.config", fake_cfg) + result = _provider.register_provider(8088) + assert result is True + # No write: save_config must not have been called, so the stored config + # is unchanged (comment-preserving idempotency). + assert saved[0] == cfg + + +def test_provider_register_skips_when_no_models(monkeypatch): + """register_provider must skip (no write) when the model catalog is empty + — there is no hardcoded fallback list duplicated here.""" + import sys + + import _provider + + monkeypatch.setattr(_provider, "_declared_models", list) + fake_hermes, fake_cfg, saved = _make_hermes_cli_mock({}) + monkeypatch.setitem(sys.modules, "hermes_cli", fake_hermes) + monkeypatch.setitem(sys.modules, "hermes_cli.config", fake_cfg) + result = _provider.register_provider(8088) + assert result is False + assert saved[0] == {} + + def test_provider_unregister_removes_our_entry(monkeypatch): import sys diff --git a/tests/test_setup.py b/tests/test_setup.py new file mode 100644 index 0000000..484e717 --- /dev/null +++ b/tests/test_setup.py @@ -0,0 +1,49 @@ +"""Regression tests for scripts/setup.sh provider-block generation. + +These exercise the REAL block-generation heredoc embedded in setup.sh (not a +mirror of it), so the `model:` default stays consistent with register_provider(). + +Motivated by a Greptile review round: + * setup.sh hardcoded `model: "auto"` even when a custom catalog did not + declare `auto`, selecting a model outside the advertised catalog. +""" + +import re +import sys +from pathlib import Path + +_SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "setup.sh" +_HEREDOC = re.search( + r"<<'PY'\nimport sys, yaml\n(.*?)\nPY\n", + _SCRIPT.read_text(encoding="utf-8"), + re.DOTALL, +) +assert _HEREDOC, "setup.sh block-generation heredoc not found" +_BLOCK_CODE = "import sys, yaml\n" + _HEREDOC.group(1) + + +def _generate(manifest_text, tmp_path, monkeypatch): + plugin_yaml = tmp_path / "plugin.yaml" + plugin_yaml.write_text(manifest_text, encoding="utf-8") + blockfile = tmp_path / "block.txt" + monkeypatch.setattr( + sys, "argv", ["setup.py", str(blockfile), str(plugin_yaml), "8088"] + ) + exec(compile(_BLOCK_CODE, "", "exec"), {}) # noqa: S102 + return blockfile.read_text(encoding="utf-8") + + +def test_setup_default_model_matches_runtime_catalog(tmp_path, monkeypatch): + """Custom catalog without `auto` must set model to the first declared + model, matching register_provider() -> models[0].""" + out = _generate( + "models:\n - custom-model\n - other-model\n", tmp_path, monkeypatch + ) + assert 'model: "custom-model"' in out + assert 'model: "auto"' not in out + + +def test_setup_default_model_keeps_auto_when_declared(tmp_path, monkeypatch): + """When `auto` is declared first (the shipped default), model stays auto.""" + out = _generate("models:\n - auto\n - claude-sonnet-4.5\n", tmp_path, monkeypatch) + assert 'model: "auto"' in out