fix(provider): skip config rewrite when provider entry already current - #99
Merged
Conversation
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.
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.
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.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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).
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens the install/uninstall flow so plugin load no longer rewrites
config.yaml(and strips its comments), removes two small redundancies, and fixes a
default-model mismatch Greptile caught.
1.
register_provider()no longer rewrites config on every load (H1)The previous implementation always ran
load_config()+save_config()on pluginload, which round-trips the YAML and strips all user comments even when nothing
changed. This is now idempotent:
_entries_equivalent()compares the rebuilt entry against what is already stored,treating the
"***"redaction sentinel and"no-key-required"as equivalent.save_config()is skipped when the entry is already current and no legacymigration ran. A
changedflag ensures the legacyaws-build->aws-builderslug migration is still persisted even when the entry is otherwise a no-op.
model-catalog refresh still write when there is an actual difference.
scripts/setup.shnow emitsdiscover_models: false, matching the runtime path.test_provider_register_noop_when_already_current.2. Single source of truth for the model catalog (M3)
register_provider()hardcoded a third copy of the model list as anor [...]fallback. That fallback is unreachable in normal operation —_declared_models()->backend.list_models()->STATIC_MODELSalready covers the"no plugin.yaml" case. The empty-catalog case (backend import failure) now skips
registration instead of duplicating the list.
test_provider_register_skips_when_no_models.3. Preserve declared model order in setup.sh (L7)
yaml.dumpalphabetizes mapping keys by default. Passsort_keys=Falseso thegenerated
models:block mirrors theplugin.yamldeclaration order.4. Derive default model from the declared catalog (Greptile P1)
setup.shhardcodedmodel: "auto"in the generated block, so a customplugin.yamlcatalog withoutautoadvertised a default the provider does notactually offer. The default now comes from
models[0]— the same valueregister_provider()uses at runtime — so the selected default is always present inthe provider's own
models:block.tests/test_setup.pyexercises the real block-generation heredoc(custom-catalog regression + shipped-default sanity check).
Notes / corrections from the flow analysis
api_key: "***"is not a bug (H2 was misdiagnosed): it is the redactionsentinel that
hermes_clinormalizes to"no-key-required"on save. No changeneeded.
register_provider()" was too blunt — it would break intentional,tested behavior (adoption, migration, foreign-entry protection). Change-detection
achieves the same comment-preservation outcome without that collateral.
Deferred (documented, not in this PR)
ruamel.yamlcomment-preserving round-trip to replace all three hand-rolled configwriters (bigger, riskier refactor).
aws-buildvsbuilderreconcile (murky history, low value).Tests
Full suite: 225 passed, 1 skipped.
ruffclean (3 pre-existing N999 module-nameexclusions).