fix(server): a raising tuning provider no longer takes down get_merged() for everyone (#899) - #904
Merged
Merged
Conversation
…d() for everyone (#899) One word. server.py's TuningProviderRegistry.get_merged(): except Exception: - logger.exception("tuning provider %r raised during get_merged()", provider_id) + log.exception("tuning provider %r raised during get_merged()", provider_id) There is no `logger` in server.py — the module logger is `log`. So the handler written to swallow-and-report a bad provider instead raised NameError from inside the except, and that NameError propagated out of get_merged(). The effect was the exact OPPOSITE of what the handler is for: one misbehaving plugin took the whole merged-tunings call down for every other provider, AND the traceback named the wrong problem ("name 'logger' is not defined" rather than the provider that actually blew up). Doubly silent: nothing was ever logged either, because the logging call was the thing that crashed. Found by pyflakes while carving server.py (R3b). It survived because NOTHING exercised the failure path — no test ever had a provider raise. That is the whole reason this class of bug is invisible: it lives only on error paths, so the suite is green and the feature is broken exactly when it matters. tests/test_tuning_provider_isolation.py is that path: * a raising provider must not lose the HEALTHY providers' tunings, nor the defaults * and the failure must actually be LOGGED — swallowing is only acceptable if it reports Bite-tested: restoring `logger` fails both. (The log assertion attaches caplog's handler to the feedBack logger directly. It sets propagate=False, so pytest's root capture sees nothing from it — test_plugins.py has a capture_logger() for this, but it is not importable here: pyproject pins pythonpath to [".", "lib"], so `tests` is not a package. Three lines beat churning 21 call sites in an unrelated file to convert that helper into a fixture.) pytest 2410, pyflakes 0 undefined names in server.py, Codex 0. Closes #899 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughChangesTuning provider error isolation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_tuning_provider_isolation.py (1)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep test docstrings minimal.
These docstrings include issue history and implementation details that can become stale. Replace them with concise purpose statements.
As per coding guidelines:
*.py: Use type hints sparingly, such asPath | None,dict, andlist; keep docstrings minimal.Also applies to: 30-30, 51-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tuning_provider_isolation.py` around lines 1 - 12, Shorten the test docstrings in tests/test_tuning_provider_isolation.py, including the locations noted at lines 30–30 and 51–52, to concise statements describing each test’s purpose. Remove issue history, implementation details, and stale explanatory context while preserving the tests’ behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_tuning_provider_isolation.py`:
- Around line 21-26: Update the registry fixture’s module-isolation teardown:
after importing and yielding the registry, restore the prior
sys.modules["server"] entry or remove the fixture-imported module via
monkeypatch so later tests do not retain the temporary CONFIG_DIR
initialization. Keep the existing TuningProviderRegistry setup unchanged.
- Around line 62-68: Restore the original level of the feedBack logger after the
registry.get_merged() assertion completes. Capture the logger’s level before
setting it to ERROR, then restore that saved level in the existing finally block
alongside removing caplog.handler.
- Around line 42-47: Update the default-preservation assertion in the tuning
provider isolation test to check for a specific known default tuning value,
rather than only asserting that merged["guitar"] is non-empty. Keep the existing
healthy-provider assertion unchanged and use the repository’s established
default tuning name.
---
Nitpick comments:
In `@tests/test_tuning_provider_isolation.py`:
- Around line 1-12: Shorten the test docstrings in
tests/test_tuning_provider_isolation.py, including the locations noted at lines
30–30 and 51–52, to concise statements describing each test’s purpose. Remove
issue history, implementation details, and stale explanatory context while
preserving the tests’ behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a3be0194-b694-4adc-987a-1cc21f7ee8e1
📒 Files selected for processing (2)
server.pytests/test_tuning_provider_isolation.py
Comment on lines
+21
to
+26
| @pytest.fixture() | ||
| def registry(monkeypatch, tmp_path): | ||
| monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) | ||
| sys.modules.pop("server", None) | ||
| mod = importlib.import_module("server") | ||
| yield mod.TuningProviderRegistry() |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the cached server module after the fixture.
Removing server from sys.modules and leaving the newly imported module cached can contaminate later tests: they may see a module initialized with the temporary CONFIG_DIR. Use monkeypatch.delitem(sys.modules, "server", raising=False) or explicitly restore the previous module during teardown.
Proposed fix
- sys.modules.pop("server", None)
+ monkeypatch.delitem(sys.modules, "server", raising=False)
mod = importlib.import_module("server")
yield mod.TuningProviderRegistry()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.fixture() | |
| def registry(monkeypatch, tmp_path): | |
| monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) | |
| sys.modules.pop("server", None) | |
| mod = importlib.import_module("server") | |
| yield mod.TuningProviderRegistry() | |
| `@pytest.fixture`() | |
| def registry(monkeypatch, tmp_path): | |
| monkeypatch.setenv("CONFIG_DIR", str(tmp_path)) | |
| monkeypatch.delitem(sys.modules, "server", raising=False) | |
| mod = importlib.import_module("server") | |
| yield mod.TuningProviderRegistry() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_tuning_provider_isolation.py` around lines 21 - 26, Update the
registry fixture’s module-isolation teardown: after importing and yielding the
registry, restore the prior sys.modules["server"] entry or remove the
fixture-imported module via monkeypatch so later tests do not retain the
temporary CONFIG_DIR initialization. Keep the existing TuningProviderRegistry
setup unchanged.
Comment on lines
+42
to
+47
| assert "My Tuning" in merged["guitar"], ( | ||
| "the healthy provider's tuning is missing — one raising provider took down the " | ||
| "merged result for everyone" | ||
| ) | ||
| # and the default tunings survive | ||
| assert merged["guitar"], "default tunings were lost" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that a known default tuning survives.
assert merged["guitar"] is vacuous because the healthy provider already adds "My Tuning". The test would pass even if all defaults were lost, so it does not cover the stated default-preservation objective.
Proposed fix
+ default_guitar_names = set(registry.get_merged()["guitar"])
registry.register("bad-plugin", boom)
registry.register("good-plugin", good)
merged = registry.get_merged()
...
- # and the default tunings survive
- assert merged["guitar"], "default tunings were lost"
+ assert default_guitar_names <= merged["guitar"].keys(), (
+ "default tunings were lost"
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert "My Tuning" in merged["guitar"], ( | |
| "the healthy provider's tuning is missing — one raising provider took down the " | |
| "merged result for everyone" | |
| ) | |
| # and the default tunings survive | |
| assert merged["guitar"], "default tunings were lost" | |
| default_guitar_names = set(registry.get_merged()["guitar"]) | |
| registry.register("bad-plugin", boom) | |
| registry.register("good-plugin", good) | |
| merged = registry.get_merged() | |
| assert "My Tuning" in merged["guitar"], ( | |
| "the healthy provider's tuning is missing — one raising provider took down the " | |
| "merged result for everyone" | |
| ) | |
| assert default_guitar_names <= merged["guitar"].keys(), ( | |
| "default tunings were lost" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_tuning_provider_isolation.py` around lines 42 - 47, Update the
default-preservation assertion in the tuning provider isolation test to check
for a specific known default tuning value, rather than only asserting that
merged["guitar"] is non-empty. Keep the existing healthy-provider assertion
unchanged and use the repository’s established default tuning name.
Comment on lines
+62
to
+68
| lg = logging.getLogger("feedBack") | ||
| lg.addHandler(caplog.handler) | ||
| lg.setLevel(logging.ERROR) | ||
| try: | ||
| registry.get_merged() | ||
| finally: | ||
| lg.removeHandler(caplog.handler) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the feedBack logger level.
The test sets a process-global logger level to ERROR and never restores it, which can make later tests order-dependent.
Proposed fix
+ previous_level = lg.level
lg.addHandler(caplog.handler)
lg.setLevel(logging.ERROR)
try:
registry.get_merged()
finally:
lg.removeHandler(caplog.handler)
+ lg.setLevel(previous_level)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lg = logging.getLogger("feedBack") | |
| lg.addHandler(caplog.handler) | |
| lg.setLevel(logging.ERROR) | |
| try: | |
| registry.get_merged() | |
| finally: | |
| lg.removeHandler(caplog.handler) | |
| previous_level = lg.level | |
| lg.addHandler(caplog.handler) | |
| lg.setLevel(logging.ERROR) | |
| try: | |
| registry.get_merged() | |
| finally: | |
| lg.removeHandler(caplog.handler) | |
| lg.setLevel(previous_level) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_tuning_provider_isolation.py` around lines 62 - 68, Restore the
original level of the feedBack logger after the registry.get_merged() assertion
completes. Capture the logger’s level before setting it to ERROR, then restore
that saved level in the existing finally block alongside removing
caplog.handler.
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.
Closes #899. One word.
There is no
loggerinserver.py— the module logger islog.So the handler written to swallow-and-report a bad provider instead raised
NameErrorfrom inside theexcept, and thatNameErrorpropagated straight out ofget_merged().The effect was the exact opposite of what the handler is for:
name 'logger' is not defined, not the provider that actually blew up)Why it survived
Found by
pyflakeswhile carving server.py (R3b). It lasted because nothing exercised the failure path — no test ever had a provider raise.That's the whole reason this class of bug is invisible: it lives only on error paths, so the suite stays green and the feature is broken exactly when it matters.
The test
tests/test_tuning_provider_isolation.pyis that path:Bite-tested: restoring
loggerfails both.The log assertion attaches caplog's handler to the
feedBacklogger directly — it setspropagate=False, so pytest's root capture sees nothing from it.test_plugins.pyhas acapture_logger()for this, but it isn't importable here (pyproject pinspythonpath = [".", "lib"], sotestsisn't a package). Three lines beat churning 21 call sites in an unrelated file to convert that helper into a fixture.pytest 2410 · pyflakes: 0 undefined names in server.py · Codex 0.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests