Skip to content

lint(ruff): clean up 80 pre-existing violations + wire ruff into CI as required check (closes #33) - #34

Merged
scottconverse merged 3 commits into
masterfrom
bug/33-ruff-cleanup-and-ci-gate
Apr 25, 2026
Merged

scottconverse merged 3 commits into
masterfrom
bug/33-ruff-cleanup-and-ci-gate

Conversation

@scottconverse

Copy link
Copy Markdown
Collaborator

Closes #33

Pre-flight Bug fix #1 of the Phase 2 sprint. Dual scope per dev directive: ruff cleanup + wire ruff into CI as a required check + add ruff step to scripts/verify-release.sh.

What's in this PR

Cleanup — 80 violations addressed (was 82 in earlier discovery; host ruff 0.15.7 against on-disk source counted 80)

  • 70 auto-fixed via python -m ruff check . --fix. 47 files modified, mostly unused-import removals.
  • 6 manually fixed:
    • 4× E402 (import-order): app/exemptions/router.py L17 stray import re + L143 BaseModel hoisted; app/main.py L31-33 engine + User/UserRole + AdminUserCreate hoisted; tests/test_base_connector.py L29 get_connector hoisted.
    • 1× F841 (unused var): scripts/generate_pdf.py L218 cx removed (single-occurrence dead code, no shadowing risk).
  • 4 retained as inline # noqa: E402 with rule-ID + one-line justification:
    • app/connectors/file_system.py L16 — module logger configured at L14 before base imports trigger their own logging
    • tests/test_bootstrap_integration.py L65 — explicit T6/ENG-001 doc-block above mandates module-constant tie (_VALID_ENCRYPTION_KEY)
    • tests/test_manual_drop.py L250 — section-local import for "Pipeline Dispatch Tests" group
    • tests/test_rest_connector.py L497 — section-local import for "P7 adversarial Retry-After" group

No bare # noqa. Final state: python -m ruff check . from backend/ → exit 0, "All checks passed!"

CI gate (.github/workflows/ci.yml)

Adds new job ruff (lint). Mirrors the --no-deps pattern from the bootstrap-failure job. Runs on every push to master + every PR. 5 min timeout, no postgres needed.

⚠️ Branch-protection follow-up needed by repo admin: the workflow change here makes the job RUN on every PR. Making it a REQUIRED check (i.e. block merges, not just run) needs the protection setting (Settings → Branches → master → Require status checks → add ruff (lint)). After this PR merges and the job has at least one successful run on master, that flip should happen.

scripts/verify-release.sh

Adds step 4 (ruff lint) between section 3 (required docs) and the summary. Uses host-side ruff (pip install --user ruff) with python -m ruff fallback. Comment in the script explains why host ruff (scans on-disk source directly) over container ruff (would see image-baked source, potentially stale relative to current working tree). CI uses container ruff because CI always builds a fresh api image first.

CHANGELOG

[Unreleased] > Changed entry referencing this PR's dual scope.

Local verification

Diff summary

  • 56 files changed, 101 insertions, 80 deletions
  • 181 non-binary text lines total (well under Hard Rule 11's 800-line threshold; no [LARGE-CHANGE] tag needed)

Sprint context

  • Phase 2 sprint ledger: sprint-phase2-log.md at workspace root tracks all sprint work. This is pre-flight Step 1 of 8.
  • Audit tier: Tier 2 (per directive — single PR, real but bounded scope).
  • Live-fire test next: after merge, parent will open a throwaway PR with one synthetic violation to confirm the new ruff CI gate fails CI; then delete the throwaway. That's the real proof the gate works end-to-end.
  • No mid-PR scope creep. Three adjacent temptations resisted: did NOT touch the pre-existing telemetry-pattern sovereignty WARN (out of scope); did NOT bump versions (v1.3.0 stays); did NOT pre-stage anything for Phase 2.

Refs

  • Closes #33
  • Sprint directive: 2026-04-25 conversation
  • Sprint log: sprint-phase2-log.md (workspace root)
  • Sister deferrals (untouched in this PR): #30 (DataSources flake), #31 (binary regen)

🤖 Generated with Claude Code

scottconverse and others added 3 commits April 25, 2026 00:23
…s required check (closes #33)

Dual scope per dev directive: cleanup + CI gate + verify-release.sh step.

## Cleanup (80 violations addressed)

Original discovery enumerated 82; host ruff 0.15.7 against on-disk source
counted 80 (small drift likely from a ruff version difference between the
api container and host). All 80 addressed.

- 70 auto-fixed via `python -m ruff check . --fix`. 47 files modified,
  mostly unused-import removals.
- 6 manually fixed:
  - 4x E402 import-order: app/exemptions/router.py L17 stray `import re`
    + L143 `BaseModel` hoisted to top; app/main.py L31-33 `engine` +
    `User`/`UserRole` + `AdminUserCreate` hoisted alongside other app.*
    imports; tests/test_base_connector.py L29 `get_connector` hoisted.
  - 1x F841: scripts/generate_pdf.py L218 unused `cx` removed (grep
    confirmed single-occurrence dead code, no shadowing risk).
- 4 retained as inline `# noqa: E402  <justification>`:
  - app/connectors/file_system.py L16: module logger configured at L14
    before base imports trigger their own logging.
  - tests/test_bootstrap_integration.py L65: explicit T6/ENG-001
    doc-block above mandates module-constant tie (`_VALID_ENCRYPTION_KEY`).
  - tests/test_manual_drop.py L250: section-local import for the
    "Pipeline Dispatch Tests" group.
  - tests/test_rest_connector.py L497: section-local import for the
    "P7 adversarial Retry-After" group.

No bare `# noqa`; every suppression carries rule-ID + one-line justification.

Final: `python -m ruff check .` from backend/ → exit 0, "All checks passed!"

## CI gate (.github/workflows/ci.yml)

Adds new job `ruff (lint)`. Mirrors the `--no-deps` pattern from the
bootstrap-failure job. Runs on every push to master + every PR.
Lightweight: 5min timeout, no postgres needed.

CAVEAT: making this a REQUIRED check (i.e. blocks merges, not just runs)
needs the branch-protection setting (Settings -> Branches -> master ->
Require status checks -> add `ruff (lint)`). Workflow change in this PR
makes the job RUN; the protection setting makes it BLOCK. Repo admin
must flip the setting after this merges and the job has at least one
successful run on master.

## verify-release.sh

Adds step 4 ("ruff lint") between section 3 (required docs) and the
summary. Uses host-side ruff (`pip install --user ruff`) with
`python -m ruff` fallback. Comment in the script explains why host ruff
(scans on-disk source directly) over container ruff (would see image-baked
source, potentially stale relative to current working tree). CI uses
container ruff because CI always builds a fresh api image first.

## CHANGELOG.md

[Unreleased] > Changed entry referencing this PR's dual scope.

## Local verification

- `python -m ruff check .` from backend/: exit 0, "All checks passed!"
- `bash scripts/verify-release.sh`: exit 0, all 4 sections PASS:
  - sovereignty: 8 PASS / 1 WARN / 0 FAIL (pre-existing telemetry-pattern
    match, carried from prior runs)
  - version lockstep: `1.3.0` across all 4 surfaces (this PR doesn't
    bump version; v1.4.0 cut is Phase 2 Step 6)
  - required docs: all 6 present
  - ruff: 0 violations
- Evidence captured in `sprint-phase2-evidence/local/2026-04-25-*-{ruff,verify-release}*.log`

## Sprint context

This PR is pre-flight Bug fix #1 of the Phase 2 sprint. Closes #33.
After merge: live-fire test the new ruff CI gate via a throwaway PR
with one synthetic violation; confirm CI fails; delete the throwaway.
Then the sprint proceeds to #30 + #31 in parallel.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ruff job in PR #34 failed CI on first run because docker-compose.yml's
service-level `env_file: .env` directive requires the .env file to exist
even when `--no-deps` skips bringing up postgres/redis. The other CI jobs
(backend, bootstrap-failure) synthesize a per-job .env as their first step;
the ruff job missed that pattern.

Adds a hermetic .env synthesis step before `docker compose build api`,
matching the existing convention. Values are placeholders — ruff never
reads them; the file just needs to exist for compose to load.

Per dev directive's bug-discipline rule: bugs encountered mid-PR are
fixed in the same PR (no file-and-defer). Logged as a sprint surprise.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@scottconverse
scottconverse merged commit 1a19a9e into master Apr 25, 2026
4 checks passed
@scottconverse
scottconverse deleted the bug/33-ruff-cleanup-and-ci-gate branch April 25, 2026 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lint: ruff cleanup (82 pre-existing violations) + wire ruff into CI as required check

1 participant