fix(dev-env): resolve ruff from the declared pin instead of PATH - #1871
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @kyle-sexton's task in 1m 50s —— View job Security review
No security issues found. This PR is a dev-tooling change (aligns local Scope reviewed: What I checked in
|
![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: 4bc89095a5
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 3m 14s —— View job Code review
Reviewed the single commit 🔴 Important —
|
|
Claude has reviewed this PR 4 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
The prior commit documented 0.45.1 and wired engine.test.sh through scripts/run-ruff.sh, but left the manifest at 0.45.0. Also drop the bare PATH-ruff fallback so a drifted global cannot false-red the suite when the wrapper is present, and mark run-ruff.sh executable.
|
Claude finished @kyle-sexton's task in 1m 35s —— View job Code review
Reviewed commit 🔴 Executable bit — fixed. 🟡 Manifest/CHANGELOG version desync — fixed. 🟡 Redundant Also in this commit: the bare PATH- No new issues found. This PR is ready from a code-review standpoint; security was already cleared in the earlier pass on this PR (no workflow/permission/secret-handling surface touched). |
|
Claude finished @kyle-sexton's task in 1m 30s —— View job Security review
No security issues found. Scope reviewed: the full two-commit diff ( What I checked:
Confidence: high (both commits fully diffed against |
Invoke ruff_reported_version outside || so set -e stays intact under ShellCheck.
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
…#1983) ## Summary `fleet-state.sh` excludes `$HOME` from project context, because that directory is USER scope and reading `$HOME/.claude/settings.json` as the project map duplicates the user map. The exclusion did not hold. It compared the native path `pwd -W` reports against `$HOME` exactly as the environment carried it — one directory, two spellings. An MSYS mount alias has no drive letter for the normalizer to reconcile, so `$HOME=/tmp/x` never equalled the `C:/Users/…/Temp/x` reported for the same place. `PROJECT_ROOT` was then set to `$HOME` and the duplication happened silently. Both sides now go through `pwd -W` before normalization. Normalizing harder could not have fixed it: the two inputs disagreed before the normalizer saw them. ## Changes - `fleet-state.sh`: derive the `$HOME` side of the comparison through `pwd -W` (falling back to `pwd`, then to `$HOME`), with the reasoning recorded at the site. - `fleet-state.test.sh`: record that the existing case is load-bearing only on Git Bash — on a POSIX runner both spellings already agree, so it passes there whether or not the exclusion works. - `claude-ops` **0.27.1 → 0.27.2** with a matching CHANGELOG entry. Closes #1982 ## Why CI never caught it `plugin-gate` runs on `ubuntu-24.04`, where `$HOME` and `pwd` agree, so the case passes regardless. It was found by a local full-suite sweep and split out of #1972, which flagged it as a different root cause from that ticket's two ruff failures. ## Related - #1972 — the sweep that found this; its two remaining failures are ruff-default churn with a separate root cause and are not touched here. - #1856, #1871, #1953 — the ruff pin divergence behind those two, not this one. ## Test plan - [x] Standalone reproduction of the test's exact fixture: `currentProject` was `false`, is now `null` - [x] `fleet-state.test.sh` — 34 cases, 0 failed (was 34 / 1 failed) - [x] `shellcheck -x` on the changed script — clean - [x] `scripts/check-shell-portability.sh` — no unexcused GNU-only constructs - [x] `scripts/check-changelog-parity.sh --check-bump` — PASS - [x] `scripts/validate-plugins.sh` — PASS - [x] `markdownlint-cli2` on the CHANGELOG — 0 issues - [ ] CI green on the pushed head --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…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>
Resolves the source-control manifest and changelog against 0.47.1, and re-frames the entry: the wrapper resolves ruff from the declared pin at run time, so it follows the repository's version rather than freezing 0.15.22. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMBm1Ae29KhZmjFxw2wdZ7
…s lookup The wrapper's value was framed as pinning local ruff DOWN to CI's 0.15.22. The fleet moved to 0.16.0 and CI follows it, so that framing is now backwards: what the wrapper is for is resolving the tool from the declared pin instead of from PATH, in whichever direction the pin points. It reads the pin at run time, so no value in it changes. Also fixes the wrapper lookup itself. engine.test.sh re-derived the repository root from BASH_SOURCE AFTER cd-ing to its own directory, so a relative invocation resolved against the new cwd and landed outside the repo. The lint pass then printed "SKIP: scripts/run-ruff.sh not found" and the suite passed without linting anything — running it from the repository root, which is how scripts/run-plugin-tests.sh runs it, took that path every time. The script directory is now captured once, before the cd. 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 3m 58s —— View job Code review
Note on scope: the two "encountered an error" runs at 22:39/22:40 and the rate-limit run at 23:15 posted no findings (infrastructure failures, not review verdicts, per the automated warning comment). This pass reviews everything pushed since the last completed review ( This environment's sandbox blocked Two inline findings posted, both non-blocking:
What checked out: |
|
Claude finished @kyle-sexton's task in 1m 6s —— View job Security review
No security issues found. Scope reviewed: the full diff against This PR has been reviewed on this security lane three times already (commits
What I checked in this incremental change:
Confidence: high — this is a narrow, mechanical follow-up to a script already fully vetted across three prior review passes on this PR. |
kyle-sexton
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review found the SC2310 workaround had duplicated a twelve-line probe rather than using this repo's established disable-comment convention. Restores the single ruff_reported_version helper with a scoped disable at each call site. The 0.47.2 changelog entry also described only the resolution order, not the lookup bug the previous commit fixed — the entry now covers both, since the feature it describes was inert on a relative invocation until that fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMBm1Ae29KhZmjFxw2wdZ7
|
Claude finished @kyle-sexton's task in 1m 10s —— View job Security review
No security issues found. Scope reviewed: the full diff against This PR has now been reviewed on this security lane four times across its history (commits
What I re-confirmed holds across the full diff:
|
|
Claude finished @kyle-sexton's task in 1m 32s —— View job Code review
Reviewed commit SC2310 duplication — fixed. Changelog gap — fixed. Verification performed this pass: read the full current No new findings. This PR is ready from a code-review standpoint. Security remains out of scope for this lane (dedicated security lane already posted clean findings across all prior commits on this PR, including this one's predecessor). |
1 participant
Closes #1856
Summary
A bare
ruffonPATHand the one CI installs can be different versions, andthen they disagree about what a finding is. This adds
scripts/run-ruff.sh,which resolves ruff from the pin declared in
.github/requirements-ci.txtrather than from
PATH, and points thebabysit-prslint pass at it.The direction has been re-targeted since this PR was opened, deliberately.
It was written as "align local ruff with CI pin 0.15.22" — pinning workstations
down to CI. CI has since moved to
ruff==0.16.1(#1953) and the repo's ruleset is pinned explicitly (#1991), so that framing would read as the fleet moving
backwards. No value in this PR changes: the wrapper parses the pin at run
time, so it follows the repository's version wherever it goes and carries no
copy of its own. What changed is the title, the docs, and the changelog entry,
so a merged commit does not assert the opposite of what the fleet does.
Fix
scripts/run-ruff.sh— uses a PATHruffonly when it already reportsthe pinned version (the CI install path), otherwise
uvx ruff==<pin>; exit 2when a drifted PATH ruff is all there is and
uvxis unavailable, exit 127when neither exists.
engine.test.shlints through the wrapper, skipping visibly rather thansilently when the pin cannot be resolved.
docs/CI-RUNNER-ROUTING.mdrecords the local rule, and now also recordswhich way the pin moves: it is held equal to the fleet inventory in
melodic-software/dotfiles
.chezmoidata/uv-tools.yaml, because that is whatinstalls a developer's toolchain.
source-control0.47.2 with a matching changelog entry (was 0.45.1against a 0.45.0 baseline; rebased onto 0.47.1).
A real defect found while re-verifying
engine.test.shre-derived the repository root fromBASH_SOURCEaftercd-ing to its own directory.BASH_SOURCEholds the path as invoked, so arelative invocation resolved against the new cwd and landed outside the
repository. The lint pass then printed
and the suite exited 0 having linted nothing. Running the suite from the
repository root — which is exactly how
scripts/run-plugin-tests.shruns it —took that path every time, so the wrapper this PR exists to introduce would have
been inert in CI. The script directory is now captured once, before the
cd.Verification
bash scripts/run-ruff.sh --version→ruff 0.15.22before merging main andruff 0.16.1after, with an unchanged 0.16.0 ruff onPATHthroughout and noedit to this branch in between. That is the design demonstrated rather than
asserted: chore(deps): bump ruff from 0.15.22 to 0.16.1 in /.github #1953 landed at 0.16.1 rather than the 0.16.0 its title advertised,
and the wrapper followed a version this PR never named.
bash plugins/source-control/skills/babysit-prs/scripts/engine.test.shfromthe repository root and from the script's own directory → exit 0, both
printing
== ruff (CI pin via scripts/run-ruff.sh) ==. Before the lookup fix,the repository-root invocation printed the SKIP line instead.
shellcheck scripts/run-ruff.sh plugins/source-control/skills/babysit-prs/scripts/engine.test.sh→ clean.bash scripts/check-changelog-parity.sh --check-bump origin/main→Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry.origin/main(100 commits behind at the start); the only conflictswere the
source-controlmanifest version and changelog, resolved onto0.47.1.
Related
categories run; that and this are the two halves of local-equals-CI.
ruff==0.16.1, which this wrapperpicked up with no change here.
fleet inventory equal, so the value this wrapper reads stays the one
developers run.