feat(config-schema): versioned plugin config schemas + doctor extension (phase 1 of #80)#86
Open
rolling-codes wants to merge 3 commits into
Open
feat(config-schema): versioned plugin config schemas + doctor extension (phase 1 of #80)#86rolling-codes wants to merge 3 commits into
rolling-codes wants to merge 3 commits into
Conversation
…on (phase 1 of #80) - Add ConfigSchema class (easycord/config_schema.py): pure apply() with migration chain, _v version stamping, missing-key backfill, non-dict replacement, and unknown-key preservation. - Add PluginConfigManager.get_schema(): fast path (pure read) when section is already valid; heal path uses atomic store.mutate() under the per-guild lock. - Pilot on SuggestionsPlugin: _get_config() now uses get_schema(SCHEMA) instead of get() + manual defaults, guaranteeing all keys are present on every read. - Extend easycord doctor: generic .tmp and corrupt-JSON detection (always); plugin config schema drift detection (with bot target); --fix-configs flag applies sync healing via atomic JSON write (temp + rename). - 11 new tests in tests/test_config_schema.py; full suite: 1524 tests.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideIntroduces a declarative, versioned plugin configuration schema system with healing and migration support, wires it into the plugin config manager and SuggestionsPlugin, extends Sequence diagram for PluginConfigManager.get_schema healing flowsequenceDiagram
participant Plugin as SuggestionsPlugin
participant PCM as PluginConfigManager
participant Store as ServerConfigStore
participant Schema as ConfigSchema
Plugin->>PCM: get_schema(guild_id, SCHEMA)
PCM->>Store: load(guild_id)
Store-->>PCM: cfg_obj
PCM->>Schema: apply(cfg_obj.get_other(schema.key))
Schema-->>PCM: healed_section, changes
alt [changes is empty]
PCM-->>Plugin: section (pure read)
else [changes not empty]
PCM->>Store: mutate(guild_id, _heal)
Store-->>PCM: healed_section
PCM-->>Plugin: healed_section
end
Sequence diagram for easycord doctor schema drift detection and healingsequenceDiagram
actor Dev as Developer
participant CLI as cmd_doctor
participant Doctor as _doctor_report
participant Bot as bot
participant Plugin as Plugin
participant Schema as ConfigSchema
Dev->>CLI: easycord doctor --fix-configs target
CLI->>Doctor: _doctor_report(target, fix_configs=True)
Doctor->>Bot: iterate _plugins
Doctor->>Plugin: inspect config.store._base
Doctor->>Schema: apply(section)
Schema-->>Doctor: healed_section, changes
alt [changes present]
Doctor->>Doctor: _apply_schema_fixes(schema_plugins)
Doctor-->>CLI: report with healed count
else [no changes]
Doctor-->>CLI: report schemas healthy
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
documentation
3 tasks
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The schema drift detection in
_doctor_reportand the healing logic in_apply_schema_fixesboth walk plugin stores and JSON files separately; consider reusing the same discovered guild paths or passing the detected issues into the fixer to avoid redundant file I/O. - In
_doctor_report,schema_pluginsincludes plugins even if theirconfig.store._baseisNone, whereas_apply_schema_fixesiterates over all of them again and skips those without a base; you could pre-filterschema_pluginsto only those with a valid store path to simplify both loops. - The
.easycordroot used for generic config health checks is hard-coded asPath('.easycord'); if there is already a single source of truth for the config root elsewhere, consider using that to avoid divergence if the root location changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The schema drift detection in `_doctor_report` and the healing logic in `_apply_schema_fixes` both walk plugin stores and JSON files separately; consider reusing the same discovered guild paths or passing the detected issues into the fixer to avoid redundant file I/O.
- In `_doctor_report`, `schema_plugins` includes plugins even if their `config.store._base` is `None`, whereas `_apply_schema_fixes` iterates over all of them again and skips those without a base; you could pre-filter `schema_plugins` to only those with a valid store path to simplify both loops.
- The `.easycord` root used for generic config health checks is hard-coded as `Path('.easycord')`; if there is already a single source of truth for the config root elsewhere, consider using that to avoid divergence if the root location changes.
## Individual Comments
### Comment 1
<location path="easycord/cli.py" line_range="423-424" />
<code_context>
+ tmp.write_text(json.dumps(data, indent=2), encoding="utf-8")
+ tmp.replace(guild_json)
+ fixed += 1
+ except (json.JSONDecodeError, OSError, ValueError):
+ pass
+ return fixed
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid silently swallowing all JSON/IO/schema errors during schema heal
Catching `JSONDecodeError`, `OSError`, and `ValueError` and then `pass`-ing hides config/schema and filesystem problems, which makes failures in `--fix-configs` hard to understand. Please log at least a debug or warning message with the file path and exception, or gate logging behind a verbosity flag / once-per-file mechanism to avoid noise, rather than silently swallowing these errors.
</issue_to_address>
### Comment 2
<location path="easycord/cli.py" line_range="545-556" />
<code_context>
+ pass
+
+ if schema_issues:
+ _fixed = _apply_schema_fixes(schema_plugins) if fix_configs else 0
+ add(
+ "config.schema_health",
+ "Plugin config schema drift",
+ fix_configs and _fixed >= 0,
+ f"{len(schema_issues)} guild config(s) need healing"
+ if not fix_configs
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Success flag for schema heal always reports OK when --fix-configs is used
Because `ok` is computed as `fix_configs and _fixed >= 0`, it always evaluates to `True` whenever `--fix-configs` is used, even if no configs were actually healed. This can mask remaining schema drift after a supposed repair. Consider basing `ok` on the relationship between `_fixed` and `schema_issues` (e.g. `(_fixed == 0 and not schema_issues)` or `(_fixed >= len(schema_issues))`), or at least on `schema_issues` when `fix_configs` is true, so the result reflects the actual post-heal state.
```suggestion
if schema_issues:
_fixed = _apply_schema_fixes(schema_plugins) if fix_configs else 0
_issue_count = len(schema_issues)
add(
"config.schema_health",
"Plugin config schema drift",
(_fixed >= _issue_count) if fix_configs else False,
f"{_issue_count} guild config(s) need healing"
if not fix_configs
else f"{_fixed} of {_issue_count} guild config(s) healed",
severity="warning",
fix="" if fix_configs else f"Run: easycord doctor --fix-configs {target}",
)
```
</issue_to_address>
### Comment 3
<location path="easycord/config_schema.py" line_range="90-96" />
<code_context>
+ result = dict(section)
+
+ # --- migrations ---------------------------------------------------------
+ current_v = result.get("_v")
+ if current_v is None:
+ current_v = 1 # pre-schema; treat as version 1
+ if self._version > 1:
+ changes.append("no _v stamp — treating as pre-schema v1")
+
+ if isinstance(current_v, int) and current_v < self._version:
+ v = current_v
+ while v < self._version:
</code_context>
<issue_to_address>
**issue:** Non-integer _v values skip migrations silently and remain inconsistent
When `_v` exists but isn’t an `int`, `isinstance(current_v, int)` causes all migrations to be skipped and leaves `_v` in a bad type, so future heals will also skip migrations. Consider treating non-int `_v` as invalid and either resetting it to a baseline (e.g. pre-schema v1) or overwriting it with `self._version` while recording that correction, so the version stamp and migration path stay consistent.
</issue_to_address>
### Comment 4
<location path="tests/test_config_schema.py" line_range="92-101" />
<code_context>
+ assert result["_v"] == 2
+
+
+def test_apply_runs_migration_chain() -> None:
+ schema = ConfigSchema(
+ key="test", version=3, defaults={**_DEFAULTS, "new_v3": "hi"}
+ )
+
+ @schema.migration(from_version=1)
+ def _v1_to_v2(s: dict) -> dict:
+ return {**s, "_migrated_v1": True}
+
+ @schema.migration(from_version=2)
+ def _v2_to_v3(s: dict) -> dict:
+ return {**s, "_migrated_v2": True}
+
+ result, changes = schema.apply({**_DEFAULTS, "_v": 1})
+ assert result["_migrated_v1"] is True
+ assert result["_migrated_v2"] is True
+ assert result["_v"] == 3
+ migration_changes = [c for c in changes if "migrated" in c]
+ assert len(migration_changes) == 2
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for edge cases around `_v` handling and migration gaps in `ConfigSchema.apply`
The current tests only cover a linear 1→2→3 migration. Please also add coverage for:
- A section with `_v` greater than `schema.version` (forward/incompatible configs) to define whether `apply` leaves it unchanged and records no changes.
- A section with a non-int `_v` (e.g. string/float) to pin how `current_v` is interpreted (treated as pre-schema vs ignored).
- A section with `_v` < `schema.version` where a migration step is missing (e.g. `_v = 1`, `version = 3`, only `from_version=2` registered) to ensure we don’t silently produce a partially migrated config or inconsistent `_v`.
These tests will lock in the intended behavior for `_v` handling and guard against regressions in migration logic.
Suggested implementation:
```python
import copy
from pathlib import Path
import pytest
from easycord.config_schema import ConfigSchema
```
```python
_DEFAULTS: dict = {"enabled": True, "count": 0, "name": "default"}
def test_apply_ignores_forward_version_section() -> None:
schema = ConfigSchema(key="test", version=2, defaults=_DEFAULTS)
@schema.migration(from_version=1)
def _v1_to_v2(s: dict) -> dict:
return {**s, "extra": True}
original = {**_DEFAULTS, "_v": 5, "marker": "future"}
result, changes = schema.apply(copy.deepcopy(original))
# Forward/incompatible configs should be left unchanged and produce no changes
assert result == original
assert changes == []
def test_apply_treats_non_int_version_as_unversioned() -> None:
schema = ConfigSchema(key="test", version=2, defaults=_DEFAULTS)
@schema.migration(from_version=1)
def _v1_to_v2(s: dict) -> dict:
return {**s, "extra": True}
base_section = dict(_DEFAULTS)
non_int_version_section = {**_DEFAULTS, "_v": "not-an-int"}
result_base, changes_base = schema.apply(base_section)
result_non_int, changes_non_int = schema.apply(non_int_version_section)
# Non-int _v should behave the same as an unversioned section
assert result_non_int == result_base
assert changes_non_int == changes_base
def test_apply_raises_on_migration_gap() -> None:
schema = ConfigSchema(key="test", version=3, defaults=_DEFAULTS)
@schema.migration(from_version=2)
def _v2_to_v3(s: dict) -> dict:
return {**s, "extra": True}
# Current version is 1, but there is no migration registered from 1 to 2
section_v1 = {**_DEFAULTS, "_v": 1}
with pytest.raises(ValueError):
schema.apply(section_v1)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+545
to
+556
| if schema_issues: | ||
| _fixed = _apply_schema_fixes(schema_plugins) if fix_configs else 0 | ||
| add( | ||
| "config.schema_health", | ||
| "Plugin config schema drift", | ||
| fix_configs and _fixed >= 0, | ||
| f"{len(schema_issues)} guild config(s) need healing" | ||
| if not fix_configs | ||
| else f"{_fixed} guild config(s) healed", | ||
| severity="warning", | ||
| fix="" if fix_configs else f"Run: easycord doctor --fix-configs {target}", | ||
| ) |
There was a problem hiding this comment.
suggestion (bug_risk): Success flag for schema heal always reports OK when --fix-configs is used
Because ok is computed as fix_configs and _fixed >= 0, it always evaluates to True whenever --fix-configs is used, even if no configs were actually healed. This can mask remaining schema drift after a supposed repair. Consider basing ok on the relationship between _fixed and schema_issues (e.g. (_fixed == 0 and not schema_issues) or (_fixed >= len(schema_issues))), or at least on schema_issues when fix_configs is true, so the result reflects the actual post-heal state.
Suggested change
| if schema_issues: | |
| _fixed = _apply_schema_fixes(schema_plugins) if fix_configs else 0 | |
| add( | |
| "config.schema_health", | |
| "Plugin config schema drift", | |
| fix_configs and _fixed >= 0, | |
| f"{len(schema_issues)} guild config(s) need healing" | |
| if not fix_configs | |
| else f"{_fixed} guild config(s) healed", | |
| severity="warning", | |
| fix="" if fix_configs else f"Run: easycord doctor --fix-configs {target}", | |
| ) | |
| if schema_issues: | |
| _fixed = _apply_schema_fixes(schema_plugins) if fix_configs else 0 | |
| _issue_count = len(schema_issues) | |
| add( | |
| "config.schema_health", | |
| "Plugin config schema drift", | |
| (_fixed >= _issue_count) if fix_configs else False, | |
| f"{_issue_count} guild config(s) need healing" | |
| if not fix_configs | |
| else f"{_fixed} of {_issue_count} guild config(s) healed", | |
| severity="warning", | |
| fix="" if fix_configs else f"Run: easycord doctor --fix-configs {target}", | |
| ) |
Comment on lines
+90
to
+96
| current_v = result.get("_v") | ||
| if current_v is None: | ||
| current_v = 1 # pre-schema; treat as version 1 | ||
| if self._version > 1: | ||
| changes.append("no _v stamp — treating as pre-schema v1") | ||
|
|
||
| if isinstance(current_v, int) and current_v < self._version: |
There was a problem hiding this comment.
issue: Non-integer _v values skip migrations silently and remain inconsistent
When _v exists but isn’t an int, isinstance(current_v, int) causes all migrations to be skipped and leaves _v in a bad type, so future heals will also skip migrations. Consider treating non-int _v as invalid and either resetting it to a baseline (e.g. pre-schema v1) or overwriting it with self._version while recording that correction, so the version stamp and migration path stay consistent.
Comment on lines
+92
to
+101
| def test_apply_runs_migration_chain() -> None: | ||
| schema = ConfigSchema( | ||
| key="test", version=3, defaults={**_DEFAULTS, "new_v3": "hi"} | ||
| ) | ||
|
|
||
| @schema.migration(from_version=1) | ||
| def _v1_to_v2(s: dict) -> dict: | ||
| return {**s, "_migrated_v1": True} | ||
|
|
||
| @schema.migration(from_version=2) |
There was a problem hiding this comment.
suggestion (testing): Add tests for edge cases around _v handling and migration gaps in ConfigSchema.apply
The current tests only cover a linear 1→2→3 migration. Please also add coverage for:
- A section with
_vgreater thanschema.version(forward/incompatible configs) to define whetherapplyleaves it unchanged and records no changes. - A section with a non-int
_v(e.g. string/float) to pin howcurrent_vis interpreted (treated as pre-schema vs ignored). - A section with
_v<schema.versionwhere a migration step is missing (e.g._v = 1,version = 3, onlyfrom_version=2registered) to ensure we don’t silently produce a partially migrated config or inconsistent_v.
These tests will lock in the intended behavior for _v handling and guard against regressions in migration logic.
Suggested implementation:
import copy
from pathlib import Path
import pytest
from easycord.config_schema import ConfigSchema_DEFAULTS: dict = {"enabled": True, "count": 0, "name": "default"}
def test_apply_ignores_forward_version_section() -> None:
schema = ConfigSchema(key="test", version=2, defaults=_DEFAULTS)
@schema.migration(from_version=1)
def _v1_to_v2(s: dict) -> dict:
return {**s, "extra": True}
original = {**_DEFAULTS, "_v": 5, "marker": "future"}
result, changes = schema.apply(copy.deepcopy(original))
# Forward/incompatible configs should be left unchanged and produce no changes
assert result == original
assert changes == []
def test_apply_treats_non_int_version_as_unversioned() -> None:
schema = ConfigSchema(key="test", version=2, defaults=_DEFAULTS)
@schema.migration(from_version=1)
def _v1_to_v2(s: dict) -> dict:
return {**s, "extra": True}
base_section = dict(_DEFAULTS)
non_int_version_section = {**_DEFAULTS, "_v": "not-an-int"}
result_base, changes_base = schema.apply(base_section)
result_non_int, changes_non_int = schema.apply(non_int_version_section)
# Non-int _v should behave the same as an unversioned section
assert result_non_int == result_base
assert changes_non_int == changes_base
def test_apply_raises_on_migration_gap() -> None:
schema = ConfigSchema(key="test", version=3, defaults=_DEFAULTS)
@schema.migration(from_version=2)
def _v2_to_v3(s: dict) -> dict:
return {**s, "extra": True}
# Current version is 1, but there is no migration registered from 1 to 2
section_v1 = {**_DEFAULTS, "_v": 1}
with pytest.raises(ValueError):
schema.apply(section_v1)
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
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
3 participants
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
Phase 1 of #80 — versioned plugin config schemas.
easycord/config_schema.py— newConfigSchemaclass withapply(): pure function, no I/O. Handles absent/empty/non-dict sections, migration chains, missing-key backfill,_vversion stamping, and unknown-key preservation.PluginConfigManager.get_schema()— fast path (pure read) when section is already valid; heal path usesstore.mutate()under the per-guild lock for atomic persist.SuggestionsPluginpilot —_get_config()switches fromget()toget_schema(SCHEMA), guaranteeing all default keys are present on every read. A missingenabledkey is now impossible to observe.easycord doctorextension — generic.tmpand corrupt-JSON detection (always); plugin config schema drift detection and--fix-configshealing (requires bot target).tests/test_config_schema.py(9 unit, 2 integration). Full suite: 1524 tests.Test plan
pytest tests/test_config_schema.py -v— all 11 passpytest -q— full suite 1524 green, no regressionsruff check easycord tests --select E9,F63,F7,F82— cleanpython scripts/verify_plugin_tests.py— all thresholds meteasycord doctor --help—--fix-configsflag visibleSummary by Sourcery
Introduce versioned plugin configuration schemas with migration support and integrate them into plugin config management and diagnostics.
New Features:
Enhancements:
Documentation:
Tests: