Skip to content

fix(provider): harden no-op regression test and setup.sh model fallback - #100

Merged
iap merged 1 commit into
mainfrom
fix/review-round2
Aug 23, 2026
Merged

fix(provider): harden no-op regression test and setup.sh model fallback#100
iap merged 1 commit into
mainfrom
fix/review-round2

Conversation

@iap

@iap iap commented Aug 22, 2026

Copy link
Copy Markdown
Owner

Summary

Follow-up to #99. An adversarial review of the merged changes surfaced four
quality issues (no production bugs). This PR fixes all four.

1. No-op regression test was hollow (P2)

test_provider_register_noop_when_already_current asserted only
saved[0] == cfg, which holds whether or not save_config ran — the
rebuilt entry is deep-equal to the stored one. So the test did not lock in the
PR's headline behavior ("no rewrite on load"). The mock now records
save_config calls, and the test asserts the call count is zero.

2. "***""no-key-required" normalization untested (P3)

The new test used "***" on both sides, so the cross-writer normalization
branch (the whole point of _entries_equivalent) was never exercised. Added
test_provider_register_noop_cross_writer_key: a config written by setup.sh
(api_key: no-key-required) vs the runtime sentinel ("***") must still skip
the save.

3. setup.sh model fallback diverged from backend.STATIC_MODELS (P3)

When plugin.yaml omitted models:, setup.sh wrote {auto: {}} while the
runtime rebuilt 4 models — causing a one-time rewrite (comment strip) on first
load. setup.sh now falls back to the same built-in catalog as
backend.list_models() (documented duplication, since setup.sh cannot import
backend.py — it depends on requests).

4. Scalar models: iterated character-by-character (P3)

models: auto (a scalar) made [str(m) for m in models] iterate the string's
characters, yielding model: "a". Added an isinstance(models, list) guard,
matching backend._load_model_override().

Tests

  • tests/test_coverage_gaps.py: mock tracks save calls; no-op asserts zero
    saves; new cross-writer key test.
  • tests/test_setup.py: new fallback-to-STATIC_MODELS and scalar-guard tests.

Full suite: 228 passed, 1 skipped. ruff clean (3 pre-existing N999).

…h model fallback

Adversarial review of the merged #99 surfaced four issues:

- The no-op regression test only asserted the stored config was equal, which
  holds whether or not save_config ran (the rebuilt entry is deep-equal to the
  stored one). Track save_config calls and assert zero on the no-op path.
- The "***" <-> "no-key-required" normalization branch was untested (both
  sides used "***"). Add a cross-writer test: setup.sh's "no-key-required" vs
  runtime's "***" must still skip the save.
- setup.sh fell back to ["auto"] when plugin.yaml omitted `models:`, diverging
  from backend.STATIC_MODELS (4 models) and forcing a one-time rewrite on
  first load. Fall back to the same built-in catalog.
- setup.sh iterated a scalar `models:` value character-by-character
  (e.g. `models: auto` -> model "a"). Guard with isinstance(list), matching
  backend._load_model_override().

Full suite: 228 passed, 1 skipped.
@github-actions github-actions Bot added the bug Something isn't working label Aug 22, 2026
@iap
iap merged commit 6f132e2 into main Aug 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant