chore(lint): clear EXE002 findings - #70
Conversation
Signed-off-by: hariomloharapps <hariomlohar.new@gmail.com>
MSKazemi
left a comment
MSKazemi
left a comment
There was a problem hiding this comment.
Thank you — this is a genuinely well-executed contribution, and the care shows in the parts that are easy to get wrong.
Three things I want to call out specifically:
You fixed the cause, not the symptom. EXE002 can be silenced two ways: chmod -x, or by adding a shebang. Adding shebangs would have made the rule pass while leaving 94 library modules falsely marked as executable programs. I verified every one of the 94 files: none carries a shebang, so stripping the bit is the only correct reading. That was the actual decision in this issue and you got it right.
The diff is provably behaviour-free. I checked it mechanically rather than by eye — the patch contains 94 diff --git headers, 94 old mode/new mode pairs, and zero content lines:
$ gh pr diff 70 | grep -cE '^[+-][^+-]'
0
That is the strongest possible evidence for a "no code changed" claim, and it made this reviewable in minutes instead of hours.
You scoped it deliberately and said so. Issue #64 asked for one rule family per PR precisely so the 438-line sweep wouldn't become unreviewable. You took EXE002, left the pin and the import-style rules alone, and stated that in the PR body. That is exactly the right call, and the note explaining that mypy src didn't apply because there is no src/ directory was a useful catch on our checklist rather than a box ticked untruthfully.
Verification
I re-ran every gate against the merge result (your branch merged into current main, not the branch alone):
| Check | Result |
|---|---|
ruff check (pinned, CI scope) |
All checks passed |
mypy |
Success: no issues found in 171 source files |
pytest tests/ (server) |
990 passed |
pytest tests/ (kube-q) |
312 passed |
ruff@0.16.1 --select EXE002 (CI scope) |
All checks passed — fully cleared |
ruff@0.16.1 (CI scope, all rules) |
436 → 342 |
Your numbers reproduce exactly, including the 436 → 342 reduction being attributable entirely to the 94 EXE002 findings. All 8 CI checks are green on 79b7c5d.
Merging as-is.
What I'm doing as follow-up (not a request for you)
Your fix is correct but the repo can silently undo it: with ruff pinned <0.16, CI cannot enforce EXE002 at all, so the next stray +x bit lands unnoticed — which is how these 94 accumulated. I'm landing a maintainer commit that (a) extends the same mode-only sweep to the remaining 100 EXE002 files elsewhere in v4/ that fall outside the CI-linted path, and (b) adds a lint-independent CI guard so the bit cannot come back regardless of the ruff version.
That is maintainer hardening around your change, not a gap in it — I'm keeping it separate so your PR stays exactly what it says it is.
EXE002 is now closed on #64; the import-style families remain open if you'd like another.
…ible PR #70 cleared 94 stray executable bits, but nothing stops them returning: ruff is pinned <0.16 (#64) and EXE002 only became a default rule in 0.16, so the lint gate is structurally blind to this class. That blind spot is how 94 library modules acquired a +x bit in the first place - mode-preserving copies drift in silently and no reviewer spots a mode change in a diff. Adds scripts/check-file-modes.sh and a File modes CI job enforcing: a tracked file is executable if and only if it starts with a shebang. Deliberately dependency-free (git + coreutils, no uv sync) so it stays correct whichever way the ruff upgrade in #64 lands, and it finishes in seconds. It also covers the inverse defect (EXE001, a shebang'd script that cannot be run), which ruff only reports for Python. Sweeps the remaining 282 offenders outside the CI-linted path (the rest of v4, deploy, three root files) and restores +x on four shebang'd scripts that had lost it. Mode-only: 282 files changed, 0 insertions, 0 deletions. The frozen v1-v3 generations are excluded by design (ADR-001/002) - closed to changes and not built by CI, so rewriting ~500 of their modes would be churn against immutable history for no gate benefit. Gates on the final tree: ruff clean, mypy 0 errors/171 files, 990 server tests, 312 kq tests, file modes OK.
Two blind spots, both structural rather than one-off. 1. We shipped an interpreter we never tested. v4/Dockerfile's runtime stage is python:3.13-slim and all three distributions declare requires-python >=3.12, so every container and every pip install on a current machine already ran 3.13 -- while every CI job pinned 3.12. kube-q even claimed the 3.13 classifier with no job behind it. Both suites pass on 3.13 unchanged (990 server + 312 kq), so this adds coverage rather than fixing a break; the point is that a future 3.13-only regression now fails a gate instead of reaching users. Closes #62. Added as a separate job, not a python-version matrix axis: the axis would rename "Tests (server)", and branch protection matches required checks by name, so every open PR would block on a check that never reports again. 2. Nothing in the pipeline could fail on a SyntaxWarning. The pinned ruff does not report an invalid escape sequence in the linted scope, mypy never compiles source, and pytest raises it only on a cold .pyc cache -- so a green suite was not evidence, and #63 reached an outside contributor. That defect was not cosmetic either: the same non-raw string was corrupting the jsonpath examples in the coordinator prompt. scripts/check-syntax-warnings.py compiles every tracked .py outside the frozen v1-v3 trees with SyntaxWarning promoted to an error. Like the file- modes gate it is dependency-free, so it stays correct however the ruff pin is eventually lifted, and it uses compile() rather than compileall so it leaves no .pyc behind. Verified red-green by re-introducing #63 verbatim. make setup now runs six gates instead of four, and every contributor-facing surface says so. Also repairs five stale pointers to #64: it was closed when #70 cleared EXE002, but 342 findings and the <0.16 pin remain, so that work is now untracked and the docs said otherwise. Names the trap in it: UP045 (95 of the 342) rewrites Optional[X] to X | None, which on an injected RunnableConfig parameter is exactly what safety invariant #6 forbids -- the config stops being injected and RBAC and the HITL gate silently stop being enforced while every test still passes. Gates: ruff clean; mypy 0 errors / 171 files; 995 server + 312 kq on both 3.12 and 3.13; make check-modes clean; make check-syntax clean; make setup exit 0.
|
@hariomlohardev — I'd like to invite you to the Reviewer rung for GOVERNANCE.md defines Reviewers as contributors trusted with review authority in one area — review requests routed to you via CODEOWNERS, plus triage and labelling. No merge rights, no expectation that you watch the whole project. Why you. This PR is the reason. $ gh pr diff 70 | grep -cE '^[+-][^+-]'
0That PR is also what prompted the What it would mean in practice
Interested? Say so here and I'll add you to CODEOWNERS. If you'd rather keep contributing without the label, that's completely fine — #79 is a live 30-minute one in your area. |
|
Hi! Thank you for the invitation, Please let me know if it is okay to reach out to you if I need some help or have questions as I get started. I have noted the critical warning regarding ruff --fix and UP045 to keep the security gates safe. Please go ahead and add me to CODEOWNERS. Excited to help out! |
|
Thank you — genuinely. You are the project's first Reviewer, and that matters more than it may sound: until today one person reviewed everything, which is the single biggest thing limiting how many contributions this project can take. You just changed that. Yes — please reach out any time, about anything. That is not a courtesy sentence. Questions from a reviewer are the cheapest thing in this project; a wrong merge is the expensive one. "I don't understand why this is safe" is a complete and welcome review comment, and if you ever say "I'm not sure, can you look?" the answer will always be yes. And please don't worry about being new to open source. You are already doing the part most people never learn: on #70 you checked that all 94 files had no shebang before running What being Reviewer means here, concretely
One honest thing about CODEOWNERS. I checked before adding you rather than after: GitHub only routes review requests to code owners who have write access to the repository, and it silently ignores entries for anyone who does not. If I added So that is one step on me: granting repository access is a permissions decision the owner makes deliberately, and it is queued. Nothing about your role waits on it — please start reviewing whenever you like by commenting on any open PR in Two live ones you may find interesting: #106 (a real bug your fellow contributor's honest test report uncovered) and #75, the Thank you again for saying yes. If the project is useful to you, a ⭐ helps other people find it — but you have already given it something worth much more. |
2 participants
What & why
Closes #64
Clears the Ruff
EXE002findings introduced by executable file mode bits on source files.+x) bit from 94 source files.100755 → 100644.Type of change
Scope
v4/v4/, or docs/typos in older versions).Checklist
uv run pytestpasses locallyuv run ruff check .passes locallyuv run mypy srcpasses locallygit commit -s— DCO)Notes for reviewers
Verification:
EXE002: fully cleared.EXE002findings.git diff --stat: 94 files changed, 0 insertions(+), 0 deletions(-).100755 → 100644.mypy srcchecklist command is not applicable because this repository does not contain asrc/directory.