Reserve legacy aliases and advisory IDs against plugin rule collisions - #485
Conversation
A plugin rule claiming a legacy alias (e.g. plugin-readme) could never be addressed under its own name — config keys, --rule/--skip-rule, and suppression directives all resolve the alias to the builtin, so its settings would silently land on the wrong rule. A plugin rule claiming the reserved 'deprecated-rule' ID would have its violations excluded from the exit code by the advisory filter. Treat both ID sets as taken during plugin loading: the colliding rule is skipped with a plugin-load-error warning, matching the existing builtin-collision behavior. Follow-up to Codex review on #484. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 58 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPlugin and custom rule loading now rejects IDs that conflict with builtin legacy aliases or reserved advisory IDs. Baseline generation and suppression exclude repository-path and deprecated-rule violations. Missing-frontmatter autofix output removes a trailing space. ChangesRule validation and baseline handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
PR Summary by QodoReserve legacy alias/advisory rule IDs during plugin & custom rule loading
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_plugins.py`:
- Around line 396-407: Update both collision tests,
test_rule_id_collision_with_legacy_alias_is_skipped and the additional test
around the second collision case, to assert the emitted plugin-load-error
violation has Severity.WARNING. Import Severity from skillsaw.rule if it is not
already available, while preserving the existing rule ID, count, and message
assertions.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ded82583-fe36-4ee3-8cc5-5ea98cacb13c
📒 Files selected for processing (2)
src/skillsaw/linter.pytests/test_plugins.py
Code Review by Qodo
1.
|
Pre-release audit findings: - skillsaw baseline baked 'deprecated-rule' notices into the baseline, permanently hiding the removal warning on later runs. Advisory IDs are now unbaselinable (never written, and baked entries from earlier builds no longer suppress). - The alias/advisory ID reservation now also covers custom rule files, which could previously squat a legacy alias and become unaddressable. - The agentskill-valid frontmatter fix no longer writes a trailing space after 'description:'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f070d2917
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ument reservations - Reword the alias-squatting test comment that read as jargon. - Assert Severity.WARNING on both new collision warnings (CodeRabbit). - Document the reserved rule IDs in docs/plugins.md and the advisory-notice baseline behavior in docs/baseline.md (Codex). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/skillsaw/rules/builtin/agentskills/valid.py`:
- Line 87: Update the exact expected autofix output in the relevant test within
tests/test_autofix.py to match the formatting emitted by the fixed string in
valid.py: expect description: immediately followed by a newline, without a
trailing space. Preserve all other expected output unchanged.
In `@tests/test_rule_aliases.py`:
- Around line 242-270: Update test_custom_rule_cannot_claim_legacy_alias to
assert that the single plugin-load-error warning has Severity.WARNING in
addition to its existing rule ID and message assertions.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a94d86e8-2603-4077-a71e-4fb70e685fa2
📒 Files selected for processing (7)
docs/baseline.mddocs/plugins.mdsrc/skillsaw/baseline.pysrc/skillsaw/linter.pysrc/skillsaw/rules/builtin/agentskills/valid.pytests/test_plugins.pytests/test_rule_aliases.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_plugins.py
- src/skillsaw/linter.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c953bfa711
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Pull request was closed
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit c953bfa |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #485 +/- ##
==========================================
+ Coverage 94.00% 94.06% +0.06%
==========================================
Files 162 170 +8
Lines 13418 14244 +826
==========================================
+ Hits 12613 13398 +785
- Misses 805 846 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The integration job asserts `summary.warnings == 0` against real repos. openshift-eng/ai-helpers' .skillsaw.yaml still configures `content-critical-position`, which 0.18.0 deprecated, so skillsaw now emits an advisory `deprecated-rule` notice for it. skillsaw itself exits 0 on that repo — advisory notices are excluded from the exit code, the grade, and baselines — but `summary` counts them, so the gate failed on a clean run. Assert over the violation list instead, filtering advisories, so the gate uses the same "is anything actually wrong" semantics as the exit code. It also now reports which warnings fired instead of just a count. Pre-existing on main since #484; main's post-merge run never executed during the Actions outage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Add test_custom_rule_cannot_claim_advisory_id. The custom-rule reservation guards both halves, but only the legacy-alias half was covered for custom rules and only the advisory half for plugin rules (Codex, Qodo). Verified by reverting the guard to `rid in RULE_ALIASES` — the new test fails. - Assert Severity.WARNING on the custom-rule collision warning, matching the plugin-side asserts (CodeRabbit). - Document reserved rule IDs in docs/custom-rules.md; only docs/plugins.md had the note, and custom-rules.md is the entry point for repository-local rules the same guard applies to (Codex). - Reword the baked-baseline test docstring to describe the invariant rather than the PR's fix history (Codex). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae5f287f1d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| from .rules.builtin import RULE_ALIASES | ||
|
|
||
| rid = rule_instance.rule_id | ||
| if rid in RULE_ALIASES or rid in ADVISORY_RULE_IDS: |
There was a problem hiding this comment.
When a repository-local custom rule returns a canonical builtin ID such as claude-plugin-readme, this new guard only checks RULE_ALIASES and ADVISORY_RULE_IDS; _known_rule_ids already contains the builtins but custom loading never consults it, so the custom rule can still be appended under the builtin's identity and share its config, suppressions, and baseline fingerprints. Please mirror the plugin loader's existing-ID check here, or narrow the new docs that promise builtin collisions are skipped.
AGENTS.md reference: AGENTS.md:L27-L29
Useful? React with 👍 / 👎.
2 participants
Follow-up to the Codex review on #484 (the findings landed as the PR merged), plus fixes from the 0.18.0 pre-release audit.
ID reservation — two edge cases in the new alias/advisory machinery:
plugin-readme) could never be addressed under its own name — config keys,--rule/--skip-rule, and suppression directives all resolve the alias to the builtin, so its settings would silently land on the wrong rule.deprecated-ruleID would have its violations excluded from the exit code by the advisory filter.Both ID sets are now treated as taken during plugin and custom-rule loading: the colliding rule is skipped with a
plugin-load-errorwarning, matching the existing builtin-collision behavior.Baseline —
skillsaw baselinebakeddeprecated-rulenotices into the baseline, permanently hiding the removal warning on later runs. Advisory IDs are now in_UNBASELINABLE_RULE_IDS: never written to new baselines, and baked entries from earlier builds no longer suppress the notice (pinned by a test asserting the set coversADVISORY_RULE_IDS).Nit — the
agentskill-validadd-frontmatter fix no longer writes a trailing space afterdescription:.CI — the third-party integration gate asserted
summary.warnings == 0, butsummarycounts advisory notices even though they never affect the exit code. openshift-eng/ai-helpers still configures the now-deprecatedcontent-critical-position, so the gate failed on a run skillsaw itself exits 0 on. The gate now asserts over the violation list with advisories filtered out — the same "is anything actually wrong" semantics as the exit code — and reports which warnings fired instead of just a count. This was pre-existing on main since #484; main's post-merge run never executed during the Actions outage.Seven regression tests added; full suite green (3870 passed).
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation