[Auto] Add rule-set profiles: one-line curated rule bundles (profile: claude-5) - #495
[Auto] Add rule-set profiles: one-line curated rule bundles (profile: claude-5)#495stbenjam wants to merge 8 commits into
Conversation
Introduce a 'profile' config key selecting a named bundle of rule overrides applied between the builtin defaults and the user's own rules: entries (defaults < profile < user). Two profiles ship: - default: the builtin defaults, unchanged (and what runs when the key is absent) - claude-5: tuned to Anthropic's Claude 5 context-engineering guidance — elevates content-repeated-directive to error and content-instruction-drift / content-tautological / content-section-length to warning, tightens context-budget limits for claude-md/agents-md/gemini-md and skills, enables the opt-in content-missing-stop-condition rule, and disables content-weak-language Semantics: any explicit user 'enabled' (including auto) replaces the profile's decision; a severity-only user override does not re-enable a profile-disabled rule; profile 'enabled' decisions bypass the config version gate (profiles ship with the installed skillsaw) while severity/parameter-only entries never change activation; deprecated rules cannot be resurrected by a profile. skillsaw explain reports when a rule's state comes from the active profile. The registry in skillsaw/profiles.py is data-only so future profiles are a single Profile entry; TestProfileRegistry pins that every profile names live, canonical rules with valid values and declared config parameters. Closes the P9 proposal from #444. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JW4e6YDBtnqNNM1F7Hy1gg
|
ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing |
PR Summary by QodoAdd config
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
|
I am not sure about the profile mechanism in the YAML config file -- I was imagining this just being We should add a gpt-5.6 profile as well https://developers.openai.com/api/docs/guides/latest-model Also perhaps we should alias "claude-latest" and "gpt-latest" to the newest one, as a moving target. |
stbenjam
changed the title
![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: 30d35f9efe
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.profile and self.profile != DEFAULT_PROFILE: | ||
| f.write(f"profile: {self._yaml_value(self.profile)}\n\n") | ||
| else: | ||
| f.write("# profile: claude-5\n\n") |
There was a problem hiding this comment.
When a user follows this generated comment—or edits .skillsaw.yaml.example—by uncommenting profile: claude-5, the file already contains explicit enabled and severity entries for every rule from LinterConfig.default(). Since user rule entries take precedence, these generated defaults suppress the profile's severity changes and its enable/disable decisions; only the parameter override survives. Consequently, skillsaw init users cannot activate the advertised one-line bundle without deleting the relevant generated rule entries, so the generated configuration needs to preserve profile applicability rather than advertising this uncomment as sufficient.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — addressed in e418d17 by making the advertisement honest rather than restructuring generated output: the generated header (and a new docs note) now says the explicit per-rule entries below count as user overrides and win over the profile, so adopting one also means deleting the entries the profile should manage. Changing skillsaw init to stop materializing per-rule defaults, or making profiles beat default-identical user entries, would change long-standing generated-config semantics — left as a maintainer call.
Generated by Claude Code
| raw_profile = data.get("profile") | ||
| if raw_profile is None: | ||
| profile = DEFAULT_PROFILE |
There was a problem hiding this comment.
When the config contains profile: with no value, YAML parses it as null and this branch silently selects the default profile. That makes an incomplete or accidentally blank profile declaration run a different rule set without warning, even though every other non-string profile value is rejected as invalid and the error states that the value must be one of the named profiles. Distinguish an absent key from an explicitly null value and reject the latter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Keeping null-as-absent deliberately: it matches this codebase's convention for every other key — fail-on: (null) behaves like the default (pinned by test_null_fail_on_does_not_crash), version: (null) falls back with a warning, and null rule entries coerce to {}. Rejecting null only for profile would be the inconsistency. e418d17 pins the behavior with test_null_profile_behaves_like_default so it's a documented choice rather than an accident.
Generated by Claude Code
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe PR adds named rule profiles, including Rule profile support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LinterConfig
participant PROFILES
participant RuleResolver
User->>LinterConfig: Select profile
LinterConfig->>PROFILES: Validate and load profile
LinterConfig->>RuleResolver: Resolve rule settings
RuleResolver->>PROFILES: Read profile overrides
RuleResolver-->>LinterConfig: Return merged settings
LinterConfig-->>User: Report effective rule severity
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #495 +/- ##
==========================================
+ Coverage 93.88% 94.00% +0.11%
==========================================
Files 173 174 +1
Lines 14715 14771 +56
==========================================
+ Hits 13815 13885 +70
+ Misses 900 886 -14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Another question: if a user omits skillsaw version but specifies profile, should they opt-in automatically to future updates (allowing breaking changes, but also getting the latest guidance enforced immediately). Typically omitting version forces config back to 0.6.0, but with profile we know they have the latest |
…n, init caveat - Deep-merge nested rule parameters across config layers (defaults < profile < user) so a user overriding one entry of a mapping like context-budget 'limits' keeps the profile's other entries instead of silently reverting them to rule defaults (Qodo). - A config that selects a curated profile but omits 'version' now follows the installed skillsaw version instead of the 0.6.0 fallback — choosing a profile opts into the current rule set; 'profile: default' keeps the legacy fallback (stbenjam). - Clarify the generated-config profile hint and docs: the explicit per-rule entries skillsaw init writes count as user overrides and shadow the profile, so adopting one there also means deleting the entries the profile should manage (Codex P1). - Pin 'profile:' (null) behaving like an absent key, matching the fail-on/version convention (Codex P2), and cover the defensive unknown-profile branch (codecov). - Trim review-history references from the claude-5 profile comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JW4e6YDBtnqNNM1F7Hy1gg
|
Implemented in e418d17: a config that selects a curated profile but omits One scoping choice: The same push also addresses the bot findings: nested rule parameters now deep-merge across config layers (a user overriding one Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e418d17201
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| For the same reason, a config that selects a curated profile but omits | ||
| `version` follows the **installed** skillsaw version rather than falling | ||
| back to `0.6.0`: new rules apply immediately after upgrades. Set `version` |
There was a problem hiding this comment.
This exception contradicts the prominent warning above that every config without a version is treated as 0.6.0. With profile: claude-5, LinterConfig.from_file() instead assigns the installed version, so readers may incorrectly believe an unversioned profile remains pinned across upgrades; qualify the earlier warning to exclude curated profiles or cross-reference this exception.
AGENTS.md reference: AGENTS.md:L27-L31
Useful? React with 👍 / 👎.
The Version Pinning warning stated every unversioned config falls back to 0.6.0, contradicting the curated-profile exception documented in the Profiles section. Cross-reference the exception where the blanket claim is made. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JW4e6YDBtnqNNM1F7Hy1gg
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87f7ab4a39
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.profile and self.profile != DEFAULT_PROFILE: | ||
| f.write(f"profile: {self._yaml_value(self.profile)}\n\n") |
There was a problem hiding this comment.
When an API caller constructs LinterConfig(profile="not-a-profile"), the new test explicitly treats that value as a supported fallback to the default profile, but save() writes profile: not-a-profile; subsequently loading the generated file raises ValueError because from_file() rejects unknown profiles. This breaks the config save/load round trip, so either validate programmatic profile assignments before serialization or serialize their effective default-profile value.
Useful? React with 👍 / 👎.
| profile_enabled = self._profile_rules(rule_id).get("enabled") | ||
| if profile_enabled is True: | ||
| return True, f"enabled: true set by profile '{self.profile}'" | ||
| if profile_enabled is False: | ||
| return False, f"disabled by profile '{self.profile}'" |
There was a problem hiding this comment.
When a current or future profile sets a rule to enabled: auto—a value explicitly accepted by the new profile-registry validation—this branch handles neither boolean case and later applies the config version gate because there are no user overrides. That contradicts the documented rule that profile enabled decisions bypass version pinning, so an older pinned config can silently skip a rule that its selected profile opted into; preserve the profile-origin flag while falling through to repository/format detection and use it to bypass the version check.
Useful? React with 👍 / 👎.
…ile auto - save()/to_dict() no longer serialize a programmatically-assigned profile name that isn't in the registry — it contributes no overrides, and writing it produced a file from_file() rejects. - A profile-set 'enabled: auto' now bypasses the config version gate on its way to repo-type/format detection, matching the documented rule that profile enabled decisions are version-independent (true/false already behaved this way; no shipped profile uses auto yet, but the registry validation accepts it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JW4e6YDBtnqNNM1F7Hy1gg
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93ce60d8df
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| version: str = "" | ||
| # Named rule-set profile (see skillsaw.profiles): a curated bundle of | ||
| # rule overrides applied under the user's own ``rules:`` entries. | ||
| profile: str = DEFAULT_PROFILE |
There was a problem hiding this comment.
Adding profile before rules silently changes the generated dataclass constructor: an existing call such as LinterConfig("0.19.0", {"my-rule": {"enabled": true}}) previously populated rules, but now stores the dict in profile; a subsequent get_rule_config() then passes that unhashable dict to PROFILES.get() and raises TypeError. Append the new field after the existing constructor fields or otherwise preserve the prior positional signature.
Useful? React with 👍 / 👎.
| `enabled` setting. `skillsaw explain <rule-id>` reports when a rule's | ||
| state comes from the active profile. |
There was a problem hiding this comment.
This promises profile attribution for a rule's general state, but rule_enabled_reason() only names the profile for profile-supplied enabled: true or enabled: false. For current severity- or parameter-only entries such as content-repeated-directive and context-budget, skillsaw explain displays the effective value without saying it came from claude-5, so the documented diagnostic behavior is unavailable for most profile overrides. Describe this as enablement attribution or extend explain to attribute the other profile layers.
Useful? React with 👍 / 👎.
…verity in explain
- Declare the profile dataclass field after every pre-profile field so
LinterConfig keeps the positional constructor signature of earlier
releases — positional callers passing (version, rules) no longer put
the rules dict into profile.
- skillsaw explain now annotates a profile-supplied severity ('severity:
error (set by profile claude-5)'), dropped when a user rules: entry
overrides it, and the docs claim is scoped to enablement and severity
attribution.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JW4e6YDBtnqNNM1F7Hy1gg
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_config.py (1)
1525-1530: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate profile parameter values.
config_schemaonly declares"limits"as a"dict". The test does not validate its nested shape or values. A malformedcontext-budget.limitsvalue can reach_get_limits()and fail during rule execution. Add schema-based validation or assert the expectedwarnanderrormappings explicitly.🤖 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_config.py` around lines 1525 - 1530, Extend the profile parameter validation in the test’s config_schema branch to validate the nested shape and values of context-budget.limits, not just that the key exists. Assert that limits is a mapping containing the expected warn and error entries with valid values, or reuse the project’s schema-based validator if available, so malformed limits are rejected before rule execution.
🤖 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.
Outside diff comments:
In `@tests/test_config.py`:
- Around line 1525-1530: Extend the profile parameter validation in the test’s
config_schema branch to validate the nested shape and values of
context-budget.limits, not just that the key exists. Assert that limits is a
mapping containing the expected warn and error entries with valid values, or
reuse the project’s schema-based validator if available, so malformed limits are
rejected before rule execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 13d08fcd-3890-4f6b-977b-5d5f7316b059
📒 Files selected for processing (6)
docs/configuration.mdsrc/skillsaw/cli/_explain.pysrc/skillsaw/config.pytests/test_config.pytests/test_explain.pytests/test_integration.py
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/configuration.md
- src/skillsaw/config.py
- tests/test_integration.py
The registry test checked that profile parameters exist in the rule's
config_schema but not that nested values are well-formed — a malformed
context-budget limits entry in a future profile would only fail at lint
time. Assert each category is an int or a non-empty {warn, error}
mapping of positive ints.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JW4e6YDBtnqNNM1F7Hy1gg
2 participants
Implements the P9 proposal from #444: a
profileconfig key that selects a named, curated bundle of rule overrides with one line instead of a hand-maintainedrules:block.Design
src/skillsaw/profiles.py. AProfileis a name, description, and arule_id → {enabled, severity, params}mapping — adding a future profile is a single dict entry with a rationale comment per rule.rules:entries. Merging happens inLinterConfig.get_rule_config()/rule_enabled_reason(), so lint, fix, andskillsaw explainall pick it up;explainreports profile-driven state ("enabled: true set by profile 'claude-5'").TestProfileRegistrypins the registry's integrity: every profile must name live, canonical (non-aliased, non-deprecated) rules with valid severities/enabledvalues, and any parameter it sets must exist in that rule'sconfig_schema. A bad entry fails CI instead of being silently ignored at lint time.Profiles
default— empty by definition; identical to omitting the key (pinned by a byte-identical-output integration test).claude-5— encodes Anthropic's Claude 5 context-engineering guidance as deltas over the defaults, per the #444 gap analysis:content-repeated-directivecontent-instruction-driftcontent-tautologicalcontent-section-lengthcontext-budgetcontent-missing-stop-conditioncontent-weak-languageSemantics
enabled— includingauto— replaces the profile's decision for that rule.enabled.enableddecisions bypass the configversiongate (profiles ship with the installed skillsaw, so selecting one is an explicit opt-in); severity/parameter-only entries never change activation.profile:degrade to the existing unknown-key warning.Docs
docs/configuration.md: new Profiles section with precedence rules and the full claude-5 delta table..skillsaw.yaml.examplenow advertises the key (# profile: claude-5).Validation
make test: 3960 passed (26 new unit tests incl.TestProfileRegistry; 5 new integration tests over a new realistictests/fixtures/config/profile-claude-5fixture whose one CLAUDE.md exercises an elevation, a disable, and an opt-in enable under both profiles)make lint,make updatecleanopenshift-eng/ai-helpers: output byte-identical to main (the exit-1 there is pre-existing and self-inflicted — their own customplugins-doc-up-to-daterule flags their docs/ as stale on unmodified main too; grade and violation counts match main exactly, confirming the default profile changes nothing)Closes #444's P9. The remaining proposals in #444 (new rules P1–P8) are untouched; when they land, adding them to the claude-5 profile is a one-line registry edit each.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JW4e6YDBtnqNNM1F7Hy1gg
Generated by Claude Code
Summary by CodeRabbit
New Features
claude-5profile.skillsaw explainidentifies profile-provided severity values.Documentation
Bug Fixes