From 2f8c397d1621d676bc5643766f4575b0ceb30806 Mon Sep 17 00:00:00 2001 From: Iko Date: Sat, 22 Aug 2026 10:55:09 -0700 Subject: [PATCH 1/4] fix(provider): skip config rewrite when provider entry already current MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit register_provider() rewrote config.yaml via load_config()/save_config() on every plugin load — a full YAML round-trip that strips every comment in the file, defeating the comment-preserving design of setup.sh/uninstall.sh. Add change-detection: compare the rebuilt entry to the existing one and skip save_config when they are equivalent and no legacy-slug migration happened. Also align setup.sh's provider block with register_provider by emitting discover_models: false (previously only register_provider set it). Adds a regression test asserting a no-op load does not rewrite the config. --- _provider.py | 44 +++++++++++++++++++++++++++++++++---- scripts/setup.sh | 1 + tests/test_coverage_gaps.py | 29 ++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 4 deletions(-) diff --git a/_provider.py b/_provider.py index da058cb..b9bc376 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 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). - 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. + 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 @@ -164,6 +188,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 +197,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 +249,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..ce20d41 100755 --- a/scripts/setup.sh +++ b/scripts/setup.sh @@ -119,6 +119,7 @@ lines = [ f" base_url: http://localhost:{port}/v1", " api_key: no-key-required", " model: \"auto\"", + " discover_models: false", ] lines.extend(model_lines) diff --git a/tests/test_coverage_gaps.py b/tests/test_coverage_gaps.py index 758e9e8..8b6b9fb 100644 --- a/tests/test_coverage_gaps.py +++ b/tests/test_coverage_gaps.py @@ -232,6 +232,35 @@ 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_unregister_removes_our_entry(monkeypatch): import sys From 721f9dce080601f4e43215f4439900c75ddbdb05 Mon Sep 17 00:00:00 2001 From: Iko Date: Sat, 22 Aug 2026 11:11:21 -0700 Subject: [PATCH 2/4] refactor(provider): single source of truth for model catalog register_provider() hardcoded a third copy of the model list as an `or [...]` fallback when _declared_models() returned empty. That case is already covered: _declared_models() -> backend.list_models() -> STATIC_MODELS, so the only way to get [] is backend import failure, where advertising a provider is pointless anyway. Skip registration (return False, no save) when the catalog is empty instead of duplicating the catalog. Adds regression test for the skip path. --- _provider.py | 16 ++++++++++------ tests/test_coverage_gaps.py | 16 ++++++++++++++++ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/_provider.py b/_provider.py index b9bc376..190fd3e 100644 --- a/_provider.py +++ b/_provider.py @@ -148,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 diff --git a/tests/test_coverage_gaps.py b/tests/test_coverage_gaps.py index 8b6b9fb..3d6ca21 100644 --- a/tests/test_coverage_gaps.py +++ b/tests/test_coverage_gaps.py @@ -261,6 +261,22 @@ def test_provider_register_noop_when_already_current(monkeypatch): 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 From 22b4c3136019099c4edc6ca8dc76d9aa2b37f6c8 Mon Sep 17 00:00:00 2001 From: Iko Date: Sat, 22 Aug 2026 11:11:43 -0700 Subject: [PATCH 3/4] fix(setup): preserve declared model order in generated block yaml.dump sorts mapping keys alphabetically by default, reordering the models: block away from the plugin.yaml declaration order. Pass sort_keys=False so the generated provider block mirrors the source catalog. --- scripts/setup.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/setup.sh b/scripts/setup.sh index ce20d41..6343cb7 100755 --- a/scripts/setup.sh +++ b/scripts/setup.sh @@ -108,7 +108,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] From fee6c92f54dae76b3c93b96e08dec715c67ebf7a Mon Sep 17 00:00:00 2001 From: Iko Date: Sat, 22 Aug 2026 11:29:06 -0700 Subject: [PATCH 4/4] fix(setup): derive default model from declared catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setup.sh hardcoded `model: "auto"` in the generated provider block, which selects a model outside the advertised catalog when a custom plugin.yaml declares a catalog without `auto`. Derive the default from models[0] — the same value register_provider() uses at runtime — so the selected default is always present in the provider's own models block. Adds tests/test_setup.py exercising the real block-generation heredoc (custom-catalog regression + shipped-default sanity check). --- scripts/setup.sh | 8 +++++++- tests/test_setup.py | 49 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 tests/test_setup.py diff --git a/scripts/setup.sh b/scripts/setup.sh index 6343cb7..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 @@ -118,7 +124,7 @@ 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_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