Skip to content

lint: adopt the Sonar/Checkstyle/FindBugs ruff bar, fix the src/ baseline - #104

Merged
open-coder-ai merged 8 commits into
mainfrom
w51/ruff-lint-adoption
Sep 2, 2026
Merged

lint: adopt the Sonar/Checkstyle/FindBugs ruff bar, fix the src/ baseline#104
open-coder-ai merged 8 commits into
mainfrom
w51/ruff-lint-adoption

Conversation

@open-coder-ai

@open-coder-ai open-coder-ai commented Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

What this changes

Adopts the owner's Sonar/Checkstyle/FindBugs static-analysis bar (plan/coding-standards.md §3) via ruff's C90/N/PLR/PLW/PLC/ERA/T20/ARG/RET/SIM/PIE/FBT/A/B/S/BLE/TRY/RUF rule families, with the mccabe/pylint thresholds the standard specifies (max-complexity 10; max-args 5, max-branches 12, max-returns 6, max-statements 50), and fixes the measured src/agentseam baseline (101 findings at this ruff version — close to the plan's independently-measured 100; a per-rule count drift is noted in the worker report) category by category in seven bisectable commits:

  1. config adoption ([tool.ruff.lint] select + thresholds + documented per-file-ignores)
  2. mechanical/safe (__all__/__slots__ sort, exception chaining, contextlib.suppress, a safe import hoist)
  3. magic values + a new literal-duplication guard (tools/literal_duplication.py, seeded per the standard, no ruff rule covers this)
  4. exception hygiene + correctness (message constants, narrowed/justified blind excepts, Error-suffixed exception names, named dead parameters)
  5. boolean traps (keyword-only bool params, keyword bool call sites)
  6. complexity splits (every C901/PLR091x function split into named helpers, pure extraction)
  7. AGENTS.md records the new static_analysis standard

No behavior changes anywhere — every commit is a pure refactor (renames, extractions, keyword-ification, constant extraction). tests/, tools/, examples/, and docs/'s two asset scripts are out of scope for this wave (never measured against the new rule set) and are per-file-ignored with a TODO(lint-adoption) reason for a follow-up wave. contract.py/install.py/install_identity.py's PLR0913 (DTO/config-object constructor shape) is a deliberate, documented exception, not deferred work. No version bump; runtime stays stdlib-only; target-version stays py39.

CI already runs ruff check . and ruff format --check . unconditionally, so no workflow change was needed — it now enforces the expanded rule set automatically.

Claim check

Not applicable — this PR touches adapter internals only for lint/refactor purposes (pure extraction, renames, keyword-ification). No capability claim, MATRIX row, or payload-shape handling changes.

Checks

  • pytest -q passes (1448 passed, 4 skipped)
  • ruff check . and ruff format --check . pass
  • Runtime path is still stdlib-only (no new imports outside the standard library)
  • Commits are signed off (git commit -s)
  • Golden fixture replay, bundle equivalence, and the 12-agent subprocess replay all pass unchanged (behaviour-preservation gate for this wave)

Notes for the reviewer

  • Two spots deliberately keep a rule violation rather than "fixing" it, each with an inline noqa or per-file-ignore and a reason: vscode_copilot.py's function-local import json as _json (hoisting it would edit bundle-emitted text for no behavioral gain — tests/test_bundler.py::test_function_local_imports_are_left_alone exists specifically to pin this), and dispatch.py's exit parameter (shadows the builtin on purpose, mirrors sys.exit and the bundled runtime.py.tmpl's own main(exit=...)).
  • The literal-duplication check (tools/literal_duplication.py + tests/test_literal_duplication.py) found three genuine repeats in src/ before this PR ("permissionDecisionReason", "escalate_from_transform", "transform_missing_input"); all three are now named constants, so the allowlist ships empty.
  • Full detail (per-category rationale, the baseline-count drift, deviations from the brief) is in the worker report: plan/spine-a/reports/w51.md on open-coder-ai/org-plan.

🤖 Generated with Claude Code

@claude
lint: adopt the Sonar/Checkstyle/FindBugs rule families in ruff
eec9d6f 
Owner standard (plan/coding-standards.md §3): what Sonar, Checkstyle and
FindBugs would flag on a Java codebase is now flagged on src/agentseam via
ruff's C90/N/PLR/PLW/PLC/ERA/T20/ARG/RET/SIM/PIE/FBT/A/B/S/BLE/TRY/RUF
families, with the mccabe/pylint thresholds the standard specifies
(max-complexity 10; max-args 5, max-branches 12, max-returns 6,
max-statements 50).

Every per-file-ignore carries a one-line reason. Two carve-outs are
scope, not exemptions: tests/, tools/, examples/ and docs/'s two asset
scripts were never measured against this rule set (the baseline below is
src/-only, per the brief) and are marked TODO(lint-adoption) for a
follow-up wave; contract.py/install.py/install_identity.py's PLR0913 is a
deliberate DTO/config-object constructor shape, not deferred work.

Baseline measured 2026-09-02 on src/, this ruff version (101 findings,
close to the plan's independently-measured 100 -- the small drift is
noted in the report): fixed category by category in the commits that
follow, ending with `ruff check .` clean and CI (already running `ruff
check .`) enforcing the new bar with no workflow change needed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@claude
lint(mechanical): sort __all__/__slots__, exception chaining, hoist a…
61fe2b3 
… safe import

RUF022/RUF023 (ruff --fix, reviewed): __all__ and __slots__ tuples sorted;
order has no behavioral meaning for either.

B904: two bare `raise ... from within except` sites now chain explicitly
(`from exc`) so the original cause survives in the traceback.

SIM105: cli.py's try/except/pass around the BrokenPipeError dup2 cleanup
is now contextlib.suppress(OSError).

PLC0415: cli.py's lazy `from datetime import date` had no reason to be
lazy (no circular import, cli.py is never bundled) and is hoisted to the
top. vscode_copilot.py's function-local `import json as _json` is left
alone and noqa'd instead: it is deliberately preserved, per
tests/test_bundler.py::test_function_local_imports_are_left_alone --
bundler.py only hoists a source's *top-level* imports, and this file is
spliced into a bundle whole, so hoisting it would only relocate the
import while touching every call site in respond() for no behavioral
gain. Confirmed by diffing bundler.bundle("vscode_copilot") before/after
attempting the hoist.

RUF005: instructions.py's `[SHARED_FILE] + paths(agent)` is now
`[SHARED_FILE, *paths(agent)]`.

RUF100 and the mechanical S110/B905 targets from the standard were
already clean or resolved by the previous commit's `select` expansion
(the E402 rule going live made two `# noqa: E402` directives genuinely
used, not unused).

Full suite green (1446 passed, 4 skipped); golden fixtures and bundle
equivalence re-run explicitly for the two touched adapter modules.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@claude
lint(magic-values): named constants for cli.py, a literal-duplication…
0488956 
… guard

PLR2004: cli.py's three magic-value comparisons (rule field counts 2/3,
the 90-day staleness window) become named module constants beside the
code that reads them.

New: tools/literal_duplication.py, seeded from the AST-survey approach
plan/coding-standards.md §3 calls for (no ruff rule covers this). It
walks .py files under a root, skipping data/templates/tests/__pycache__,
and reports any non-docstring string constant >= 20 chars that repeats
>= 3 times. tests/test_literal_duplication.py runs it against src/agentseam
and fails on anything not in tools/literal_duplication_allowlist.json
(each entry there needs a non-empty reason; currently empty).

Running it against src/ found three genuine repeats, now named
constants instead: "permissionDecisionReason" (the G2/vscode_copilot
dialects' one reason key, 4x each in _hook_json.py and vscode_copilot.py
-- two separate local constants, since the two modules don't share code)
and "escalate_from_transform"/"transform_missing_input" (degrade_notes
keys shared by _hook_json.py and _cursor.py -- one constant each in
_hook_json.py, imported into _cursor.py the same way it already imports
_refusal_text from there; bundler.py's _extract_with_deps already walks
cross-module name references this way for the engine composition, so
the bundle picks up the constant's definition same as any other symbol).

Pure renames -- same string values, same JSON keys emitted. Full suite
green (1448 passed, 4 skipped, two more than before for the new check).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@claude
lint(exception-hygiene, correctness): message constants, narrowed/jus…
c8f9dcd 
…tified

catches, Error-suffixed exceptions, named dead parameters

TRY003: install_identity.py's and permissions.py's two vanilla-args
raises now read their message from a module constant beside the
function, matching contract.py's existing raise style elsewhere.

BLE001: contract.py's tool_input_of() narrows to json.JSONDecodeError --
the only exception json.loads can raise here, since the input is already
known to be a str. dispatch.py's run() keeps its blind except: it is the
outermost boundary before any handler runs, deliberately fails open on
any stdin/parse failure (not just malformed JSON), and mirrors the
frozen runtime.py.tmpl's identical catch one section down -- noqa'd with
that reasoning rather than narrowed.

N818: UnsupportedDecision -> UnsupportedDecisionError (dispatch.py, never
raised elsewhere, so no other reference to update) and ConfigUnreadable
-> ConfigUnreadableError (install_config.py, install.py,
install_identity.py, tests/test_install.py -- an exported, raised, and
caught name, all four updated together).

ARG001: cli.py's _cmd_agents/_cmd_packaging and _antigravity.py's
antigravity_claims take their unused argument positionally only (argparse's
`args.fn(args)` dispatch; _family.py's ConfigAdapter calling
antigravity_claims(self.CONFIG, raw)), so the parameter is safely renamed
to `_`-prefixed. vscode_copilot.py's hook_config(..., matcher=) and
instructions.py's plan(..., repo_root=...) are both called by keyword at
existing call sites (install.py/install_identity.py's **extra unpack;
tests/test_instructions.py's repo_root="."), so renaming would break
them; both get a noqa naming the interface-parity reason instead.

Full suite green (1448 passed, 4 skipped).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@claude
lint(boolean-traps): keyword-only bool params, keyword bool call sites
a9b8c2f 
FBT002 (boolean default positional argument): every flagged bool
parameter's only callers already pass it by keyword or via a **dict
unpack (verified per site before touching the signature), so each
becomes keyword-only rather than renamed:
- _family.py's _CursorAdapter.hook_config and _hook_entry.py's
  hook_entry_config: `fail_closed` after `*`; the one positional caller
  (_family.py's own hook_config -> hook_entry_config call) updated to
  pass it by keyword too.
- matrix_terms.py's _cap: both bools after `*` -- its only two callers
  (matrix.py) pass zero arguments.
- packaging.py's Part.__init__: `executable` after `*` -- every
  constructor call site (src and tests) already names it.
- instructions.py's write: `dry_run` after `*` -- cli.py and the one
  test call already name it.
- dispatch.py's run: `exit` after `*`; every caller already passes it by
  keyword. Left named `exit` (see A002 below) rather than renamed.

runtime.py.tmpl's `exit=True` is the one FBT002 site left alone: it is
frozen, bundle-emitted text (per-file-ignore from the config commit), and
converting it to keyword-only would edit that emitted text for a lint
rule with no behavioral upside.

FBT003 (boolean positional value in call): the five call sites passing
True/False positionally into `_default_for`/`_refusal_text`
(_hook_json.py x4) and `_refusal_text` (_windsurf.py) now name
`at_gate=`/`wire=` at the call site. Pure call-site rewrite, same
arguments, same order.

A002 (builtin-argument-shadowing): dispatch.py's `exit` parameter is a
deliberate stdlib-flavored name mirroring sys.exit, shared by every
caller and by the bundled runtime.py.tmpl's main(); noqa'd with that
reasoning rather than renamed, which would be the actual breaking change.

Full suite green (1448 passed, 4 skipped).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@claude
lint(complexity): split every C901/PLR091x function, behaviour-preser…
9815282 
…ving

Pure extraction throughout -- same branches, same order, same return
values, moved into named helpers so no single function carries the
whole shape:

- _hook_entry.py hook_entry_config (17/16 -> dispatch over four
  per-wrapper helpers: _flat_list_wrapper, _cursor_wrapper,
  _flat_entries_wrapper, _default_wrapper).
- _cursor.py cursor_respond (13/15 -> _flag_payload,
  _prompt_submit_payload, _gate_payload split out; the trailing
  user_message/agent_message rule stays in cursor_respond since it
  applies across every gate outcome).
- _hook_json.py _g1 (13/9 returns -> _g1_allow, _g1_transform split out;
  _g1_transform returns None for "not representable here" so _g1 falls
  through to the shared block path exactly as the original nested ifs did).
- _payload.py hj_claims (14/10 returns/13 branches -> _rejected_by_markers
  and _accepted_by_markers, both pure predicates over the entry's claims
  config).
- vscode_copilot.py respond (13/7 returns -> _top_level_block_body,
  _nested_block_body, _pre_tool_out split out; the local `import json as
  _json` stays put per the previous commit's reasoning, so the three
  helpers build plain dicts and respond() still does every _json.dumps()
  itself).
- bundler.py _extract_with_deps (12 -> _owner_map + _transitive_deps).
- install_identity.py installed (11/9 returns -> _installed_toml/
  _installed_json split by CONFIG_FORMAT; the shared hook_config()
  fragment is now built once in installed() and passed down rather than
  rebuilt in each branch, so PLR0913 doesn't just reappear on the split).

Two splits (_hook_entry.py's _default_wrapper, _hook_json.py's
_g1_allow) needed a second pass: the first cut left them at 6 params,
one over the standard's max-args=5. Both dropped a param that was
cheaply re-derivable inside the helper (hook_entry from cfg,
at_context_event from wire/v) rather than growing the file's PLR0913
per-file-ignore to cover them.

install_config.py picks up one `ruff format` wrap the previous commit's
ConfigUnreadable -> ConfigUnreadableError rename left needed (a message
string pushed one call over the line-length wrap point) -- caught by
`ruff format --check .` while verifying this commit, not new content of
its own.

`ruff check .` and `ruff format --check .` both clean repo-wide. Full
suite green (1448 passed, 4 skipped): golden fixture replay, bundle
equivalence, and hook_config-both-matcher-paths tests all exercise every
touched function's new call graph.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@claude
docs: record static_analysis in AGENTS.md; CI already enforces
1e345f3 
The Sonar/Checkstyle/FindBugs bar (plan/coding-standards.md §3) is now
in the conventions block beside code_comments/externalized_text: the
rule select, mccabe/pylint thresholds, the magic-value remedy order, and
the literal-duplication tool/allowlist.

No CI workflow change needed -- .github/workflows/ci.yml already runs
`ruff check .` and `ruff format --check .` unconditionally, so it picks
up pyproject.toml's expanded select and now enforces it on every push.

`ruff check .` and `ruff format --check .` both clean repo-wide. Full
suite green (1448 passed, 4 skipped).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@claude
lint: cover PLR0917 on install()/installed(), CI's pinned ruff catche…
c21de9c 
…s it

CI's lint job pins ruff==0.16.5 (.github/requirements/lint.txt); this
session's ruff (0.15.8) never fires PLR0917 (too-many-positional-
arguments) at all, so it went unseen while fixing the baseline locally
-- flagged as a risk in the worker report before this PR's own CI proved
it. 0.16.5 flags install.py's install() and install_identity.py's
installed() (7 positional args each, no keyword-only split) alongside
the PLR0913 both already carry a documented ignore for; same DTO/
config-knobs reasoning, so PLR0917 joins that ignore rather than getting
its own.

Reproduced the CI failure first: installed ruff==0.16.5 locally
(alongside the session's 0.15.8, which stayed first on PATH -- ran the
pinned binary explicitly), confirmed the same two findings CI reported,
fixed, then confirmed `ruff check .` and `ruff format --check .` clean
under BOTH versions and the full suite still green (1448 passed, 4
skipped).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Claude <noreply@anthropic.com>
@open-coder-ai
open-coder-ai marked this pull request as ready for review September 2, 2026 02:22
@open-coder-ai
open-coder-ai merged commit 617441d into main Sep 2, 2026
14 checks passed
@open-coder-ai open-coder-ai mentioned this pull request Sep 2, 2026
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@open-coder-ai @claude