fix(provider): skip config rewrite when provider entry already current

_provider.py

@@ -112,24 +112,52 @@ 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
     except Exception as exc:  # noqa: BLE001
         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

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)
 

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
 

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, "<setup_block>", "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