Put scripts/ under black and ruff (#381) - #412
Merged
Merged
Conversation
`just lint` ran black, ruff and mypy over `src/` and `tests/` only — so every "lint clean" claim in this run's PRs excluded `scripts/gtdb_ground.py`, the file most of them changed. Reviews kept noting it. black now formats `scripts/` and ruff checks it. 495 auto-fixable findings applied across 46 files; 184 remain and are listed by rule in `per-file-ignores` rather than blanket-suppressed, so new code is held to the full bar and the debt is greppable. Details and a suggested order in #411. **The vendored files nearly went with it.** `validate_id_label_correspondence.py` and `chem_formula.py` are byte-identical across the Mech repos and checked by the `vendored-sync` gate; black and ruff had rewritten both (138 insertions in one). pyproject already excluded the *test* files for exactly this reason and I read that note only after running the formatter. Reverted, both excluded, and `check_vendored_sync.sh` confirms all 6 files still match CultureMech@6be694f3. `scripts/` has **no test coverage**, so the reformatting is verified by what is available rather than by behaviour: all 65 scripts parse, the four exercised by tests still produce their expected output (`gtdb_ground --name Acetobacter`, `validate_strict`, `validate_yaml_scalars`, `validate_gtdb_coherence`), and the full suite plus every gate is green. That gap is the same one that let #410 — six scripts importing a module that does not exist — go unnoticed. mypy stays on `src/`: `mypy scripts/` reports 144 errors in 47 files, 44 of them missing third-party stubs. Adding it would mean either mass suppression or an annotation pass over untested code, neither of which belongs in a lint-scope change. Filed as #411. New: #411. just validate-all, validate-strict, validate-gtdb-all, validate-scalars, vendored-sync clean; 1226 passed; lint clean over src/ tests/ scripts/.
**The change had zero CI effect.** `.github/workflows/lint.yaml` inlines `black --check src/ tests/` rather than calling `just lint`, so widening the justfile did nothing where it matters. #381 names that file explicitly: the workflow already triggers on `scripts/**/*.py` and then checked nothing in them. A new script with unsorted imports and 120-char lines would have landed green. I widened the recipe and never opened the workflow it exists to mirror. **`just format` was left behind too**, so `just lint` could reject a file the repo's documented fix command would not touch — a contributor editing `scripts/gtdb_ground.py` gets a black failure and no way to fix it. **One ignore was over-broad.** `E741` has zero occurrences in linted scripts; its only two live in a vendored file ruff excludes entirely, so the entry could never apply to existing debt and would have silently permitted a rule the rest of the repo enforces. Removed, and the comment now says every remaining code is backed by a real finding. `S110` is 9, not 10 — the tenth is in that same vendored file. **`extend-exclude` did not survive a named invocation.** `black scripts/chem_formula.py` reformatted a vendored byte-identical file; only the directory form was protected. `force-exclude` in both tools now, which is the guard I should have reached for when I caught this the first time — reverting the file fixed the instance and left the mechanism open. **My verification claim was true and empty.** All four "test-exercised" scripts are AST-identical or untouched by this diff, so citing them said nothing about the 35 files whose ASTs actually changed. What does cover them, from the review: re-running the tools over main reproduces this branch byte-for-byte; an identifier census across all 65 scripts differs in exactly 4 expected fix sites; no docstring, prompt, regex or CLI string changed; no binary open mode changed; every removed import is genuinely unused with no side-effect imports anywhere in `scripts/`; no import crossed a `sys.path.insert`; and `mypy scripts/` shows only removals — zero new errors. Counts corrected throughout: 500 fixed (not 495), 180 remaining (not 184), 128 E501 (not 129), 143 mypy errors (not 144 — the delta is a removed `import duckdb`). Those numbers are the reference for #411's cleanup, so they matter. just validate-all, validate-strict, validate-gtdb-all, validate-scalars, vendored-sync clean; 1226 passed; lint clean over src/ tests/ scripts/ in both the justfile and the workflow.
realmarcin
added a commit
that referenced
this pull request
Aug 20, 2026
…ments guard, ambiguous issue refs Addresses the remaining findings from this PR's review (the gate failure it also reported — test_a_script_imports[deep_research_provider.py] under "Python 3.14" — does not reproduce: verified directly with the real Python 3.14.6 interpreter installed on this machine, both in isolation and via `uv run --python 3.14 pytest`, both pass. CI itself pins Python 3.10 (.github/workflows/validate-strict.yaml). The fix from this PR's first commit — registering the probe module in sys.modules before exec_module — already covers this; the review's static-gate step likely ran against a stale checkout, a known hazard documented for this fleet's review tooling): - Medium: stage capabilities keys weren't validated against the known capability set, unlike provider_adjustments. A typo'd capability key silently contributed 0 to every provider's score instead of erroring. - Medium: bare "#487"/"#412" review-attribution comments collided with this repo's own unrelated PR numbers (CommunityMech#487 and #412 are both real, different PRs) — qualified as "proteintraitsmech#487" and "mediaingredientmech#412". - Low: `high = max(raw.values()) or 1.0` only guarded an exact-zero max; a large negative provider_adjustments value pushes every score negative, leaving `high` negative too, clamping every fit to 0 and collapsing the ranking to alphabetical order. Guards the sign now, not just falsiness. Reproduced live with all-negative adjustments — fixed ranking keeps a real relative order instead of degenerating. High finding (missing KNOWN_BLOCKED provider check + not registered for vendored-sync) is real but out of scope for this PR — it requires porting CultureMech's evolved implementation and/or deciding the fleet's sharing model, which is what CultureMech#287/#290/#298 already track. Filed CommunityMech#658 to carry it forward rather than expanding this PR indefinitely. Verified: full `uv run pytest tests/` — 2540 passed, 75 skipped, 8 deselected, 0 failed. `black --check`/`ruff check`/`mypy src/` clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
realmarcin
added a commit
that referenced
this pull request
Aug 21, 2026
…search (#657) * Add deep-research provider triage for ecological mechanism/dataset research Ports the fleet's generic deep-research provider-scoring implementation to CommunityMech, completing work the justfile already assumed: `deep-research-providers`/`deep-research-provider` recipes referenced scripts/deep_research_provider.py and conf/deep_research_provider.yaml, but neither file was ever committed, so both recipes were broken on main. research_community.py (the entity runner) was already in place and untouched. Two domain-specific focuses in conf/deep_research_provider.yaml: - ecological_mechanism — exact composition, directional interaction evidence between community members. - datasets_environment — repository-native accessions, ENVO context, cultivation and perturbation metadata. Bumps deep-research-client to 0.2.10, matching the version pinned across CultureMech/TraitMech/MediaIngredientMech/ProteinTraitsMech. Carries two fixes from review rounds on the sibling ports (proteintraitsmech#487, MediaIngredientMech#412): - provider_adjustments keys in the YAML profile are canonicalized and validated against known providers at config-load time, so a typo'd or aliased key no longer silently no-ops instead of applying its intended score bonus. - `--json --provider X` recomputes recommended_available/ fallback_available from the filtered ranking instead of leaving stale values computed from the unfiltered one. Also fixes a latent bug in tests/test_scripts_import.py (#410's import sweep, unrelated to deep-research otherwise): its subprocess probe built a module via importlib.util.module_from_spec() but never registered it in sys.modules before exec_module(). Any script defining a `from __future__ import annotations` frozen dataclass crashes there — dataclasses._is_type does `sys.modules.get(cls.__module__).__dict__` while checking for ClassVar/InitVar, and cls.__module__ ('_probe') was never actually stored in sys.modules, so .get() returns None. The real invocation path (`python scripts/foo.py`) doesn't hit this: running as __main__ registers sys.modules['__main__'] for free. Reproduced in isolation and via mutation test (reverting the one-line fix turns the probe red again on deep_research_provider.py specifically). Verified: full `uv run pytest tests/` — 2523 passed, 89 skipped, 8 deselected (0 failed). `black --check src/ tests/ scripts/`, `ruff check src/ tests/ scripts/`, `mypy src/` — all clean, matching .github/workflows/lint.yaml exactly. `uv sync --frozen --all-extras` — consistent. `scripts/check_vendored_sync.sh` — OK, unaffected. Mutation-tested provider_adjustments and --json filtering fixes independently; both turn their new tests red when reverted. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix findings ported from MediaIngredientMech#412's round-2 review Same generic file, same two gaps found there, fixed identically here before CommunityMech's own review round even landed: - Medium: provider_adjustments canonicalization didn't check for two raw keys resolving to the same provider (e.g. edison: 3 and falcon: 5 in the same focus) — the second silently overwrote the first. Now raises a clear ValueError. - Medium: main()'s CLI-level rejection of an unknown --provider/--focus argument had no test coverage. Verified: mutation test on the duplicate-key guard turns the new test red. Full `uv run pytest tests/` — 2540 passed (+3 from the new tests minus the earlier no-longer-relevant skip count changes), 75 skipped, 8 deselected, 0 failed. `black --check` / `ruff check` / `mypy src/` all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix round-1 review findings: capabilities validation, negative-adjustments guard, ambiguous issue refs Addresses the remaining findings from this PR's review (the gate failure it also reported — test_a_script_imports[deep_research_provider.py] under "Python 3.14" — does not reproduce: verified directly with the real Python 3.14.6 interpreter installed on this machine, both in isolation and via `uv run --python 3.14 pytest`, both pass. CI itself pins Python 3.10 (.github/workflows/validate-strict.yaml). The fix from this PR's first commit — registering the probe module in sys.modules before exec_module — already covers this; the review's static-gate step likely ran against a stale checkout, a known hazard documented for this fleet's review tooling): - Medium: stage capabilities keys weren't validated against the known capability set, unlike provider_adjustments. A typo'd capability key silently contributed 0 to every provider's score instead of erroring. - Medium: bare "#487"/"#412" review-attribution comments collided with this repo's own unrelated PR numbers (CommunityMech#487 and #412 are both real, different PRs) — qualified as "proteintraitsmech#487" and "mediaingredientmech#412". - Low: `high = max(raw.values()) or 1.0` only guarded an exact-zero max; a large negative provider_adjustments value pushes every score negative, leaving `high` negative too, clamping every fit to 0 and collapsing the ranking to alphabetical order. Guards the sign now, not just falsiness. Reproduced live with all-negative adjustments — fixed ranking keeps a real relative order instead of degenerating. High finding (missing KNOWN_BLOCKED provider check + not registered for vendored-sync) is real but out of scope for this PR — it requires porting CultureMech's evolved implementation and/or deciding the fleet's sharing model, which is what CultureMech#287/#290/#298 already track. Filed CommunityMech#658 to carry it forward rather than expanding this PR indefinitely. Verified: full `uv run pytest tests/` — 2540 passed, 75 skipped, 8 deselected, 0 failed. `black --check`/`ruff check`/`mypy src/` clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <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
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 #381.
just lintran black, ruff and mypy oversrc/andtests/only — so every "lint clean" claim in this run's PRs excludedscripts/gtdb_ground.py, the file most of them changed. Reviews kept pointing at it.What landed
black now formats
scripts/and ruff checks it. 495 auto-fixable findings applied across 46 files. 184 remain, listed by rule inper-file-ignoresrather than blanket-suppressed, so new code is held to the full bar and the debt is greppable:The vendored files nearly went with it
scripts/validate_id_label_correspondence.pyandscripts/chem_formula.pyare byte-identical across the Mech repos and checked by thevendored-syncgate. black and ruff had rewritten both — 138 insertions in one — which would have broken that gate.pyproject.tomlalready excluded the corresponding test files with a comment explaining exactly this, and I read it only after running the formatter. Reverted, both now excluded, andcheck_vendored_sync.shconfirms all 6 files still matchCultureMech@6be694f3.How the reformatting is verified
scripts/has no test coverage, so this rests on what is actually available rather than on behaviour:gtdb_ground --name Acetobacter→GTDB:g__Acetobacter,validate_strict→ 0 errors,validate_yaml_scalars→ 0,validate_gtdb_coherence→ 0);That coverage gap is the same one that let #410 — six scripts importing a module that does not exist — go unnoticed.
mypy stays on
src/mypy scripts/reports 144 errors in 47 files, 44 of them missing third-party stubs. Adding it would mean either mass suppression or an annotation pass over untested code, neither of which belongs in a lint-scope change. Filed with a suggested order as #411.just validate-all,validate-strict,validate-gtdb-all,validate-scalars,vendored-syncclean; 1226 passed; lint clean oversrc/ tests/ scripts/.Review round 2 — the CI gate was never widened
The change had zero CI effect.
.github/workflows/lint.yamlinlinesblack --check src/ tests/rather than callingjust lint, so widening the justfile did nothing where it matters. #381 names that file explicitly: the workflow already triggers onscripts/**/*.pyand then checked nothing in them. A new script with unsorted imports and 120-char lines would have landed green.I widened the recipe and never opened the workflow it exists to mirror.
just formatwas left behind too, sojust lintcould reject a file the repo's documented fix command would not touch.One ignore was over-broad.
E741has zero occurrences in linted scripts — its only two live in a vendored file ruff excludes entirely, so the entry could never apply to existing debt and would have silently permitted a rule the rest of the repo enforces. Removed; the comment now states that every remaining code is backed by a real finding. (S110is 9, not 10 — the tenth is in that same vendored file.)extend-excludedid not survive a named invocation.black scripts/chem_formula.pyreformatted a vendored byte-identical file; only the directory form was protected.force-excludeis set in both tools now — the guard I should have reached for when I first caught this, instead of reverting the file and leaving the mechanism open.My verification claim was true and empty
All four "test-exercised" scripts are AST-identical or untouched by this diff, so citing them said nothing about the 35 files whose ASTs actually changed. What does cover them, established in review:
mainreproduces this branch byte-for-byte;scripts/contains no side-effect imports;sys.path.insertboundary in either direction;mypy scripts/main vs branch shows only removals — zero new type errors;Counts corrected
500 fixed (not 495), 180 remaining (not 184), 128 E501 (not 129), 143 mypy errors (not 144 — the delta is a removed
import duckdb). These are the reference for #411's cleanup, so they matter.1226 passed; lint clean over
src/ tests/ scripts/in both the justfile and the workflow.