chore(distribution): record claude-code-plugins as a locally-owned ruff consumer - #336
Merged
Merged
Conversation
…ff consumer The repo was absent from the ruff component entirely, which is how it ended up with an implicit rule set nobody chose — ruff 0.16.0 moved the default from 59 rules to 413 and turned two of its contract suites red on an unmodified main. It now owns a ruff.toml selecting E4, E7, E9, and F. Registering that as locally-owned is what keeps it from being a second unregistered lint policy, and records the trigger for re-adopting the canonical payload as managed. Closes #335 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMBm1Ae29KhZmjFxw2wdZ7
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 2m 30s —— View job Reviewing PR #336
SummarySingle-file change: adds
Finding (Suggestion / nit)Confusing double use of "against" in the new rationale comment. distribution/sync-manifest.yml#L331-L332: "against" is used twice with two different meanings in the same clause — first as "run against [the tree]" (the scan target), then as "compared against 166" (the contrast). A reader has to parse it twice to disambiguate. Since this comment is the durable record of why
No other issues found. The change is small, internally consistent, matches established patterns elsewhere in this file, and is inert to the synchronizer per the documented ownership model. |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
kyle-sexton
added a commit
to melodic-software/claude-code-plugins
that referenced
this pull request
Aug 8, 2026
…k the repo (#1991) Closes #1972 ## Summary The repo had no ruff configuration, so every ruff invocation in it linted against whatever upstream's default selection happened to be that release. Ruff 0.16.0 moved that default from 59 rules to 413 and removed eighteen `E`/`F` rules from it — *"E401, E402, E701, E702, E703, E711, E712, E713, E714, E721, E731, E741, E742, E743, F403, F405, F406, and F722"* ([0.16.0 release notes](https://github.com/astral-sh/ruff/releases/tag/0.16.0)). That single change is behind both failures in #1972: the expansion brought the `TRY` category into the default set, and the removals took `E712` out of it. ## Fix - **`ruff.toml`** selects `E4`, `E7`, `E9`, and `F` — the pre-0.16 default set. The rule set is now a decision recorded in the repo rather than one inherited from upstream. It is deliberately a floor: raise it toward the managed policy with `extend-select` as each category is cleared. Registered as `locally-owned` for this repo in melodic-software/standards#336 (merged), which is what keeps it from being a second unregistered lint policy. - **Eval fixtures are excluded.** An audit skill's fixture holds the defect its eval asserts on, so linting it fails by construction. The comment records that exclusion governs *traversal*: Ruff's settings reference states *"Typically, Ruff will lint any paths passed in directly, even if they would typically be excluded"*, and that `force-exclude` *"will cause Ruff to respect these exclusions unequivocally"*. Every invocation here passes directories; the ruff-format hook passes a file explicitly and already sets `--force-exclude`. Verified both ways against the fixture path. - **`ruff-format.test.sh` case 4e selects `E712` in its fixture** instead of relying on it being a default rule. A case asserting on one rule's fix safety must name that rule; this is version-agnostic rather than re-pinned. - **Drops an unused `import stat`** in `python3_alias_probe.py` — a real `F401` the pinned set surfaces (line 90 calls `path.stat()`, the method, not the module). ## The TRY004 decision, recorded #1972 asked for this on its merits rather than by config, so both halves: - **On merits: the rule does not fit these sites.** All six raise on a decoded payload or a persisted state file failing an `isinstance` guard — `babysit_gh.py:603,699,728` on a GraphQL response, `babysit_lease.py:114`, `manage_feedback_ledger.py:176`, and `refresh_pr_branch.py:112` on state files this code wrote itself. None is a caller passing the wrong type to a function. That distinction is the whole argument, so it is worth being explicit that it is a judgment rather than something upstream states. TRY004's own rationale is "the Python documentation states that `TypeError` should be raised upon encountering an inappropriate type"; the Python docs' operative sentence is *"Passing arguments of the wrong type … should result in a `TypeError`"*, which is a caller-contract rule, and they define `RuntimeError` as "an error is detected that doesn't fall in any of the other categories." The broader opening sentence ("an operation or function is applied to an object of inappropriate type") could be read to cover these; the reading taken here is that data-integrity validation of a payload is not argument passing. Anything catching `RuntimeError` at these sites would change behavior if they were retyped. No `noqa` is needed; the sites stand as written. - **Separately: `TRY` is not in the selected set**, so they are not findings today regardless. Recorded so a future `extend-select = ["TRY"]` knows the category was examined and rejected on merits, not merely left unselected. ## What the prefix selection does not buy Review raised that `E4`/`E7`/`E9`/`F` are prefixes, so a later release adding a rule inside one of those categories enables it here automatically — the original wording of this PR overstated the invariant, and the file now says so at the selection. That exposure is real and is closed by version parity rather than by config. The pin in `.github/requirements-ci.txt` is held equal to the fleet inventory (melodic-software/dotfiles `.chezmoidata/uv-tools.yaml`), so a newly-added rule cannot reach CI before a developer or the reverse; melodic-software/dotfiles#416 adds the check that keeps those two equal, and #1953 is the bump that makes them equal today. Enumerating the exact pre-0.16 codes was the alternative. It trades a reviewable statement of intent for a list no reader can evaluate, freezes the policy against genuinely useful new correctness rules inside `F`, and still needs the version pin to be trustworthy — so it buys the appearance of the invariant rather than the invariant. ## Verification - `ruff check --no-cache --statistics plugins/ scripts/` → **0 findings** (166 under bare 0.16.0 defaults before this change). - `bash plugins/ruff-format/hooks/ruff-format.test.sh` → `PASS=52 FAIL=0` (was `PASS=51 FAIL=1`). - `bash plugins/source-control/skills/babysit-prs/scripts/engine.test.sh` → exit 0; `Ran 597 tests ... OK`, ruff section clean (83 findings under bare 0.16.0 defaults before this change). - `bash scripts/run-plugin-tests.sh` → complete sweep of all 170 suites, exit 0, `All plugin tests passed or were skipped.` Those two are also the only suites that invoke ruff at all (`grep -rl 'ruff check\|ruff format' --include=*.sh` finds `plugins/ruff-format/hooks/ruff-format.sh` and `engine.test.sh`), so no other suite could be affected by a lint config. - The ruff-format fixtures build their own repos under `mktemp -d`, which resolves outside the worktree, so they do not inherit this config — case 4e's pass is real evidence about the fixture's own `select`, not about this file. - Local ruff is 0.16.0, matching the fleet pin in `dotfiles` `.chezmoidata/uv-tools.yaml`, so these runs reproduce what a developer sees. CI still installs 0.15.22; #1953 is what closes that half. ## Related - melodic-software/standards#336 — registers the `ruff` component as `locally-owned` for this repo; closes melodic-software/standards#335. - Refs #1953 — the Dependabot bump to ruff 0.16.0. This makes it safe against the failure that happened: with `select` explicit, the bump cannot change which CATEGORIES run. - Refs #1871 — resolves ruff from the pin file rather than PATH; it reads the pin dynamically, so it needs no value change once #1953 lands. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.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
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 #335
Summary
Registers
ruffformelodic-software/claude-code-plugins— aslocally-owned, notmanaged.What changed relative to the issue
#335 proposed adopting the managed component. This lands the other branch of the
same decision, deliberately: that repo now owns a
ruff.tomlselectingE4,E7,E9, andF, and the manifest records that ownership.The issue's own measurement is the reason. The canonical policy reports 385
findings against that tree; upstream's bare 0.16.0 defaults report 166 (the
issue's 130 predates ~2.8k lines of merged work). Adoption is a migration —
mostly
S,ERA,FIX,PTH,ARG,RET, and theTIDrelative-import banmeeting a codebase never held to them — and blocking that repo's ruff pin bump
behind a 385-finding migration is what has kept the pin stale in the first
place.
What #335 actually argued for is preserved: the rule set stops being an implicit
default inherited from whatever upstream shipped that week. An explicit
selectdelivers that property at either size, which is why the sequencing note in the
issue ("adoption should land before, or together with, raising that repo's CI
ruff pin") is satisfied by this shape too.
The
locally-ownedentry is what keeps the repo-local file from being a second,unregistered lint policy — the exact objection #335 raises against a repo-local
ruff.toml. The manifest comment records the migration cost and the removaltrigger, so moving to
managedlater restores the canonical payload with noarchaeology.
Verification
distribution/sync-manifest.sh validate --source-root . --manifest distribution/sync-manifest.yml→Manifest valid: 34 components, 8 targets.locally-ownedis never read, changed, or deleted by the synchronizer(
distribution/README.md, "Ownership model"), so this entry cannot cause thecanonical
ruff.tomlto overwrite the repo's own.Related
that becomes safe once
selectis explicit.