Implement F4 Linter Microservices#123
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR migrates F4 diagnostics from a Ruff-only setup into an ECLI-owned multi-language linter microservices architecture under ChangesF4 Linter Microservices
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture/ecli-f4-linter-microservices-design.md`:
- Around line 213-216: The path-correction sentence is self-referential because
it maps src/ecli/extensions/linters/... to the same path. Update the wording in
this document section so the “old” path uses the incorrect legacy location
referenced elsewhere in the draft, and keep the corrected target as
src/ecli/extensions/linters/...; make sure the surrounding paragraph in this
architecture doc consistently reflects the intended source-to-destination path
change.
In `@docs/extensions/diagnostics-linter-layer.md`:
- Line 117: The diagnostics-linter-layer docs are inconsistent because
`biome.manifest.MANIFEST.supersedes` includes stylelint while the tier table
still marks stylelint as `optional`; update the tier definition around the
`MANIFEST.supersedes` and tier entries so they agree, either by changing
stylelint to `legacy` to match the supersedes claim or by removing stylelint
from the supersedes tuple if that relationship is not intended.
- Around line 37-55: The diagnostics-linter-layer doc still reflects an outdated
Ruff-only architecture, so update the sections around the core layout and the
out-of-scope/next-steps guidance to match the current tree. In the overview that
names core symbols like Diagnostic, DiagnosticsService, DiagnosticProvider, and
LinterDefinition, also list command_runner.py and provider_utils.py, and remove
the claims that only Ruff has a working provider or that non-Ruff tools have no
output parsing. Rewrite the out-of-scope and next-steps text so it no longer
frames non-Ruff providers and parsers as pending work, and instead describes the
current supported structure consistently with
src/ecli/extensions/linters/__init__.py and the microservice directories.
In `@src/ecli/core/Ecli.py`:
- Around line 5611-5628: The scroll-centering logic is duplicated between
_center_scroll_on_cursor and _goto_match, so extract the shared tentative scroll
calculation into a helper like _center_scroll_on_line(self, line: int) -> None.
Move the desired_scroll_top/max-scroll clamp logic there, have
_center_scroll_on_cursor delegate to it with self.cursor_y, and update
_goto_match to call the same helper instead of keeping its own inline centering
block.
In `@src/ecli/extensions/linters/biome/manifest.py`:
- Around line 32-41: The Biome manifest and provider are out of sync because
`manifest.py` omits `.gql` from `file_extensions` while `provider.py` already
advertises it in `_EXTENSIONS`. Update the `BiomeManifest` `file_extensions`
tuple to include `.gql` so catalog queries like `linters_for_language` and other
extension-based lookups resolve `.gql` files consistently with `supports()`.
In `@src/ecli/extensions/linters/cppcheck/manifest.py`:
- Around line 27-30: The cppcheck manifest’s argv_template is inconsistent with
the runtime command and parser contract because it uses --template=json while
provider.py’s cppcheck invocation uses --template=gcc and parser.py expects
gcc-formatted text. Update the manifest in manifest.py so argv_template matches
the provider’s template setting, keeping parse_cppcheck_output compatible and
avoiding silent loss of diagnostics.
In `@src/ecli/extensions/linters/cppcheck/parser.py`:
- Around line 62-66: The ValueError handler in parser.py is unreachable because
the regex groups in the parsing logic already guarantee numeric input. Simplify
the parsing in the parser function by removing the unnecessary try/except around
int(match.group("line")) and int(match.group("column")), while keeping the
max(1, ...) bounds checks and the surrounding match-processing flow intact.
In `@src/ecli/extensions/linters/hadolint/provider.py`:
- Around line 65-71: The supports() method in HadolintProvider currently does a
separate has_basename check before calling supports_by_language_or_extension;
simplify it by passing _BASENAMES into supports_by_language_or_extension
directly and removing the redundant import. Update the supports() implementation
in HadolintProvider to rely on the shared helper for languages, extensions, and
basenames so the logic stays centralized and the unused has_basename reference
can be dropped.
In `@src/ecli/extensions/linters/java_spotbugs/__init__.py`:
- Around line 14-20: Update the module docstring in the SpotBugs microservice
package to remove the stale claim that only Ruff has a fully working provider;
the comment in __init__.py should reflect the current set of implemented
providers. Edit the top-level docstring in this package so it accurately
describes the existing provider state without mentioning only Ruff, and keep the
rest of the metadata-only/migration wording consistent with the current
architecture.
In `@src/ecli/extensions/linters/oxlint/__init__.py`:
- Around line 14-20: The docstring in the Oxlint package skeleton is stale
because it still claims only Ruff has a fully working provider. Update the
module-level documentation in __init__.py to describe Oxlint as metadata-only
without referencing Ruff as the sole implemented provider, and align the wording
with the current state of the other provider modules (for example, the existing
provider implementations such as Biome, Zig, Clang-Tidy, Cppcheck, Checkstyle,
PMD, ShellCheck, Markdownlint, Yamllint, Actionlint, Hadolint, Taplo, and Cargo
Clippy).
In `@src/ecli/extensions/linters/stylelint/__init__.py`:
- Around line 14-20: The module docstring in Stylelint’s __init__ is stale
because it makes a global claim that only Ruff has a fully working provider.
Update the docstring in the stylelint microservice skeleton to describe only
this microservice’s status, keeping the note that there is no provider.py in
this migration and that it is metadata-only for now, without referencing Ruff or
the broader set of providers. Use the existing __init__ module text as the place
to revise.
- Around line 22-31: Remove the stale Ruff-only docstring from the metadata-only
linter package skeletons and keep these modules as simple re-export/manifest
files. Update the affected __init__.py modules for stylelint, clang_format,
eslint, golangci_lint, java_spotbugs, oxlint, pylint, sqlfluff, and tflint by
deleting the outdated docstring text and leaving the manifest/package_contract
imports and __all__ definitions intact.
In `@tests/extensions/test_linters_namespace_migration.py`:
- Around line 318-338: The normalization comment in
test_only_implemented_providers_are_registered_with_linter_bridge is misleading
because the code only special-cases Shellcheck/ShellCheck rather than doing a
general case-insensitive comparison. Update the nearby inline comment to
accurately describe the actual expected-name normalization in the referenced
test, or remove the case-insensitive wording so it matches the implementation
and avoids confusion.
In `@tests/extensions/test_linters_registry_contract.py`:
- Around line 93-98: The helper _default_or_recommended is missing a return type
annotation despite having a typed parameter. Update the function signature for
_default_or_recommended in the linters registry contract test to explicitly
annotate the tuple return type that matches the filtered LinterDefinition
entries, keeping it consistent with the file’s typed style.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 450e99bf-22fb-469d-9dbb-fc800f9ffdf4
📒 Files selected for processing (124)
docs/architecture/ecli-f4-linter-microservices-design.mddocs/architecture/extensions-layer.mddocs/extensions/diagnostics-linter-layer.mdscripts/check_runtime_imports.pysrc/ecli/core/Ecli.pysrc/ecli/diagnostics/__init__.pysrc/ecli/extensions/linters/__init__.pysrc/ecli/extensions/linters/actionlint/__init__.pysrc/ecli/extensions/linters/actionlint/manifest.pysrc/ecli/extensions/linters/actionlint/package_contract.pysrc/ecli/extensions/linters/actionlint/parser.pysrc/ecli/extensions/linters/actionlint/provider.pysrc/ecli/extensions/linters/biome/__init__.pysrc/ecli/extensions/linters/biome/manifest.pysrc/ecli/extensions/linters/biome/package_contract.pysrc/ecli/extensions/linters/biome/parser.pysrc/ecli/extensions/linters/biome/provider.pysrc/ecli/extensions/linters/cargo_clippy/__init__.pysrc/ecli/extensions/linters/cargo_clippy/manifest.pysrc/ecli/extensions/linters/cargo_clippy/package_contract.pysrc/ecli/extensions/linters/cargo_clippy/parser.pysrc/ecli/extensions/linters/cargo_clippy/provider.pysrc/ecli/extensions/linters/clang_format/__init__.pysrc/ecli/extensions/linters/clang_format/manifest.pysrc/ecli/extensions/linters/clang_format/package_contract.pysrc/ecli/extensions/linters/clang_tidy/__init__.pysrc/ecli/extensions/linters/clang_tidy/manifest.pysrc/ecli/extensions/linters/clang_tidy/package_contract.pysrc/ecli/extensions/linters/clang_tidy/parser.pysrc/ecli/extensions/linters/clang_tidy/provider.pysrc/ecli/extensions/linters/core/__init__.pysrc/ecli/extensions/linters/core/command_runner.pysrc/ecli/extensions/linters/core/display.pysrc/ecli/extensions/linters/core/models.pysrc/ecli/extensions/linters/core/provider_protocol.pysrc/ecli/extensions/linters/core/provider_utils.pysrc/ecli/extensions/linters/core/registry.pysrc/ecli/extensions/linters/core/service.pysrc/ecli/extensions/linters/cppcheck/__init__.pysrc/ecli/extensions/linters/cppcheck/manifest.pysrc/ecli/extensions/linters/cppcheck/package_contract.pysrc/ecli/extensions/linters/cppcheck/parser.pysrc/ecli/extensions/linters/cppcheck/provider.pysrc/ecli/extensions/linters/eslint/__init__.pysrc/ecli/extensions/linters/eslint/manifest.pysrc/ecli/extensions/linters/eslint/package_contract.pysrc/ecli/extensions/linters/golangci_lint/__init__.pysrc/ecli/extensions/linters/golangci_lint/manifest.pysrc/ecli/extensions/linters/golangci_lint/package_contract.pysrc/ecli/extensions/linters/hadolint/__init__.pysrc/ecli/extensions/linters/hadolint/manifest.pysrc/ecli/extensions/linters/hadolint/package_contract.pysrc/ecli/extensions/linters/hadolint/parser.pysrc/ecli/extensions/linters/hadolint/provider.pysrc/ecli/extensions/linters/java_checkstyle/__init__.pysrc/ecli/extensions/linters/java_checkstyle/manifest.pysrc/ecli/extensions/linters/java_checkstyle/package_contract.pysrc/ecli/extensions/linters/java_checkstyle/parser.pysrc/ecli/extensions/linters/java_checkstyle/provider.pysrc/ecli/extensions/linters/java_pmd/__init__.pysrc/ecli/extensions/linters/java_pmd/manifest.pysrc/ecli/extensions/linters/java_pmd/package_contract.pysrc/ecli/extensions/linters/java_pmd/parser.pysrc/ecli/extensions/linters/java_pmd/provider.pysrc/ecli/extensions/linters/java_spotbugs/__init__.pysrc/ecli/extensions/linters/java_spotbugs/manifest.pysrc/ecli/extensions/linters/java_spotbugs/package_contract.pysrc/ecli/extensions/linters/markdownlint/__init__.pysrc/ecli/extensions/linters/markdownlint/manifest.pysrc/ecli/extensions/linters/markdownlint/package_contract.pysrc/ecli/extensions/linters/markdownlint/parser.pysrc/ecli/extensions/linters/markdownlint/provider.pysrc/ecli/extensions/linters/oxlint/__init__.pysrc/ecli/extensions/linters/oxlint/manifest.pysrc/ecli/extensions/linters/oxlint/package_contract.pysrc/ecli/extensions/linters/pylint/__init__.pysrc/ecli/extensions/linters/pylint/manifest.pysrc/ecli/extensions/linters/pylint/package_contract.pysrc/ecli/extensions/linters/ruff/__init__.pysrc/ecli/extensions/linters/ruff/manifest.pysrc/ecli/extensions/linters/ruff/package_contract.pysrc/ecli/extensions/linters/ruff/provider.pysrc/ecli/extensions/linters/shellcheck/__init__.pysrc/ecli/extensions/linters/shellcheck/manifest.pysrc/ecli/extensions/linters/shellcheck/package_contract.pysrc/ecli/extensions/linters/shellcheck/parser.pysrc/ecli/extensions/linters/shellcheck/provider.pysrc/ecli/extensions/linters/sqlfluff/__init__.pysrc/ecli/extensions/linters/sqlfluff/manifest.pysrc/ecli/extensions/linters/sqlfluff/package_contract.pysrc/ecli/extensions/linters/stylelint/__init__.pysrc/ecli/extensions/linters/stylelint/manifest.pysrc/ecli/extensions/linters/stylelint/package_contract.pysrc/ecli/extensions/linters/taplo/__init__.pysrc/ecli/extensions/linters/taplo/manifest.pysrc/ecli/extensions/linters/taplo/package_contract.pysrc/ecli/extensions/linters/taplo/parser.pysrc/ecli/extensions/linters/taplo/provider.pysrc/ecli/extensions/linters/tflint/__init__.pysrc/ecli/extensions/linters/tflint/manifest.pysrc/ecli/extensions/linters/tflint/package_contract.pysrc/ecli/extensions/linters/yamllint/__init__.pysrc/ecli/extensions/linters/yamllint/manifest.pysrc/ecli/extensions/linters/yamllint/package_contract.pysrc/ecli/extensions/linters/yamllint/parser.pysrc/ecli/extensions/linters/yamllint/provider.pysrc/ecli/extensions/linters/zig/__init__.pysrc/ecli/extensions/linters/zig/manifest.pysrc/ecli/extensions/linters/zig/package_contract.pysrc/ecli/extensions/linters/zig/parser.pysrc/ecli/extensions/linters/zig/provider.pysrc/ecli/integrations/LinterBridge.pysrc/ecli/ui/panels.pytests/core/test_diagnostics_service.pytests/docs/test_f4_linter_microservices_contract.pytests/extensions/linters/test_parsers.pytests/extensions/linters/test_provider_contracts.pytests/extensions/linters/test_provider_utils.pytests/extensions/linters/test_service_provider_selection.pytests/extensions/test_extensions_tree_contract.pytests/extensions/test_linters_namespace_migration.pytests/extensions/test_linters_registry_contract.pytests/packaging/test_runtime_import_guard_scope.pytests/ui/test_diagnostics_panel.py
💤 Files with no reviewable changes (1)
- src/ecli/diagnostics/init.py
Comment on lines
+213
to
+216
| environment health checks) and must not contain F4 linter code. Every | ||
| `src/ecli/extensions/linters/...` path in earlier drafts of this document | ||
| is corrected to `src/ecli/extensions/linters/...` below and throughout this | ||
| document. This is a path correction only; every other invariant in this |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Path correction text is self-referential.
The sentence states "Every src/ecli/extensions/linters/... path in earlier drafts of this document is corrected to src/ecli/extensions/linters/...," but both paths are identical, making the correction meaningless. The "from" path should reference the old incorrect location — likely src/ecli/diagnostics/linters/... based on the preceding sentence ("not under src/ecli/diagnostics/").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/architecture/ecli-f4-linter-microservices-design.md` around lines 213 -
216, The path-correction sentence is self-referential because it maps
src/ecli/extensions/linters/... to the same path. Update the wording in this
document section so the “old” path uses the incorrect legacy location referenced
elsewhere in the draft, and keep the corrected target as
src/ecli/extensions/linters/...; make sure the surrounding paragraph in this
architecture doc consistently reflects the intended source-to-destination path
change.
Comment on lines
+37
to
+55
| - `src/ecli/extensions/linters/core/` holds the linter-agnostic contract: | ||
| `models.py` (`Diagnostic`, `DiagnosticRequest`, `DiagnosticResult`, | ||
| `DiagnosticsSnapshot`, `ProviderState`), `display.py`, `service.py` | ||
| (`DiagnosticsService`), `provider_protocol.py` (`DiagnosticProvider`), | ||
| and `registry.py` (`LinterDefinition`, `PackageContract`, and the | ||
| generic catalog-lookup helpers). | ||
| - `src/ecli/extensions/linters/ruff/` has a fully working `provider.py` | ||
| (moved unchanged from the retired `ruff_provider.py`) plus `manifest.py` | ||
| and `package_contract.py`. Ruff is the only linter with a working | ||
| provider; it remains the F4 linter microservices reference | ||
| implementation and its Python behavior is unchanged by this migration. | ||
| - Every other first-class and legacy/optional linter (22 in total) has its | ||
| own microservice directory with `manifest.py` and `package_contract.py` | ||
| only -- no `provider.py`, no execution, no output parsing. | ||
| - `src/ecli/extensions/linters/__init__.py` aggregates every | ||
| microservice's `manifest.MANIFEST` into `LINTER_CATALOG` and exposes | ||
| `get_linter(name)`, `iter_linters()`, `linters_for_language(language)`. | ||
| - No new wiring into `DiagnosticsService` beyond Ruff, no output parsing | ||
| for non-Ruff tools, and no F4 UI change of any kind. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target doc and nearby sections.
git ls-files docs/extensions/diagnostics-linter-layer.md
wc -l docs/extensions/diagnostics-linter-layer.md
sed -n '1,120p' docs/extensions/diagnostics-linter-layer.md
printf '\n---\n'
sed -n '340,420p' docs/extensions/diagnostics-linter-layer.md
printf '\n=== Search for mentioned core files and provider files ===\n'
git ls-files 'src/ecli/extensions/linters/**' | sed -n '1,220p'
printf '\n--- specific matches ---\n'
rg -n "command_runner|provider_utils|parser.py|provider.py|LinterBridge|working provider|out of scope|Next steps|Not yet implemented" src/ecli/extensions/linters docs/extensions/diagnostics-linter-layer.mdRepository: SSobol77/ecli
Length of output: 25374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant sections of the documentation.
nl -ba docs/extensions/diagnostics-linter-layer.md | sed -n '30,70p'
printf '\n---\n'
nl -ba docs/extensions/diagnostics-linter-layer.md | sed -n '360,405p'
printf '\n=== Repository files matching cited modules ===\n'
git ls-files 'src/ecli/extensions/linters/**' | rg '(^|/)(command_runner|provider_utils|parser|provider)\.py$|LinterBridge\.py$|__init__\.py$|manifest\.py$|package_contract\.py$'
printf '\n=== Cross references in docs and source ===\n'
rg -n "working provider|out of scope|Next steps|Not yet implemented|command_runner|provider_utils|registers Ruff|thirteen|13" docs/extensions/diagnostics-linter-layer.md src/ecli/extensions/lintersRepository: SSobol77/ecli
Length of output: 191
Refresh the linter-layer docs to match the current tree
docs/extensions/diagnostics-linter-layer.md:37-55, 363-404 still describes a Ruff-only future state. Update the core listing to include command_runner.py and provider_utils.py, replace the “only Ruff has a working provider” / “no output parsing” claims, and rewrite the out-of-scope + next-steps sections so they no longer describe the non-Ruff providers and parsers as pending work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/extensions/diagnostics-linter-layer.md` around lines 37 - 55, The
diagnostics-linter-layer doc still reflects an outdated Ruff-only architecture,
so update the sections around the core layout and the out-of-scope/next-steps
guidance to match the current tree. In the overview that names core symbols like
Diagnostic, DiagnosticsService, DiagnosticProvider, and LinterDefinition, also
list command_runner.py and provider_utils.py, and remove the claims that only
Ruff has a working provider or that non-Ruff tools have no output parsing.
Rewrite the out-of-scope and next-steps text so it no longer frames non-Ruff
providers and parsers as pending work, and instead describes the current
supported structure consistently with src/ecli/extensions/linters/__init__.py
and the microservice directories.
| | Systems | (none) | **Zig toolchain**, **Cargo Clippy**, **Clang-Tidy/Cppcheck/Clang-Format** | Zig, Rust, and C/C++ are core to ECLI's systems-programming audience, not afterthoughts | | ||
| | Java | (none) | **Checkstyle, PMD, SpotBugs** | first-class language profile spanning style, static rules, and bytecode bug detection | | ||
|
|
||
| `biome.manifest.MANIFEST.supersedes == ("eslint", "stylelint")` records |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
stylelint tier is inconsistent with Biome's supersedes claim.
Line 117 states biome.manifest.MANIFEST.supersedes == ("eslint", "stylelint"), implying Biome supersedes stylelint. Per the doc's own tier definition (line 130), a superseded entry should be tier "legacy" — and indeed eslint is "legacy" (line 205). But stylelint is "optional" (line 206), not "legacy". Either stylelint should be "legacy" to match the supersedes relationship, or it should be removed from Biome's supersedes tuple if it's only partially superseded.
Also applies to: 206-206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/extensions/diagnostics-linter-layer.md` at line 117, The
diagnostics-linter-layer docs are inconsistent because
`biome.manifest.MANIFEST.supersedes` includes stylelint while the tier table
still marks stylelint as `optional`; update the tier definition around the
`MANIFEST.supersedes` and tier entries so they agree, either by changing
stylelint to `legacy` to match the supersedes claim or by removing stylelint
from the supersedes tuple if that relationship is not intended.
Comment on lines
+5611
to
+5628
| def _center_scroll_on_cursor(self) -> None: | ||
| """Pre-adjust scroll_top so a deliberate jump lands with context. | ||
|
|
||
| ``_clamp_scroll()`` alone only nudges the viewport by the minimum | ||
| amount needed to keep the cursor on-screen, which pins a jump | ||
| target to the very last visible row when scrolling down. Diagnostic | ||
| jumps (like search jumps via ``_goto_match``) are deliberate, | ||
| far-reaching navigation, so pre-seed ``scroll_top`` to place the | ||
| target roughly a third of the way down the viewport before the | ||
| final clamp runs. | ||
| """ | ||
| if self.visible_lines <= 0: | ||
| return | ||
| text_area_height = self.visible_lines | ||
| desired_scroll_top = self.cursor_y - (text_area_height // 3) | ||
| max_scroll_possible = max(0, len(self.text) - text_area_height) | ||
| self.scroll_top = max(0, min(desired_scroll_top, max_scroll_possible)) | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate scroll-centering algorithm vs. _goto_match.
_center_scroll_on_cursor re-implements the same tentative-scroll-top centering logic already inlined in _goto_match (desired_scroll_top = cursor_y - height//3, clamp to [0, len(text)-height]). Extracting a shared helper (e.g. _center_scroll_on_line(self, line: int) -> None) would avoid the two copies drifting apart if the centering ratio or clamp is ever tuned in only one place.
♻️ Proposed refactor
- def _center_scroll_on_cursor(self) -> None:
- """Pre-adjust scroll_top so a deliberate jump lands with context."""
- if self.visible_lines <= 0:
- return
- text_area_height = self.visible_lines
- desired_scroll_top = self.cursor_y - (text_area_height // 3)
- max_scroll_possible = max(0, len(self.text) - text_area_height)
- self.scroll_top = max(0, min(desired_scroll_top, max_scroll_possible))
+ def _center_scroll_on_cursor(self) -> None:
+ """Pre-adjust scroll_top so a deliberate jump lands with context."""
+ self._center_scroll_on_line(self.cursor_y)
+
+ def _center_scroll_on_line(self, line: int) -> None:
+ """Shared centering helper used by both search jumps and diagnostic jumps."""
+ if self.visible_lines <= 0:
+ return
+ text_area_height = self.visible_lines
+ desired_scroll_top = line - (text_area_height // 3)
+ max_scroll_possible = max(0, len(self.text) - text_area_height)
+ self.scroll_top = max(0, min(desired_scroll_top, max_scroll_possible))Then update _goto_match to call self._center_scroll_on_line(self.cursor_y) instead of its inline block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/core/Ecli.py` around lines 5611 - 5628, The scroll-centering logic
is duplicated between _center_scroll_on_cursor and _goto_match, so extract the
shared tentative scroll calculation into a helper like
_center_scroll_on_line(self, line: int) -> None. Move the
desired_scroll_top/max-scroll clamp logic there, have _center_scroll_on_cursor
delegate to it with self.cursor_y, and update _goto_match to call the same
helper instead of keeping its own inline centering block.
Comment on lines
+32
to
+41
| file_extensions=(".js", | ||
| ".jsx", | ||
| ".mjs", | ||
| ".cjs", | ||
| ".ts", | ||
| ".tsx", | ||
| ".json", | ||
| ".jsonc", | ||
| ".css", | ||
| ".graphql"), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add .gql to manifest file_extensions to match provider.
The provider's _EXTENSIONS in provider.py includes .gql, but the manifest's file_extensions does not. This means catalog-based extension lookups (e.g., linters_for_language or any catalog query using file_extensions) will not match .gql files to Biome, even though the runtime supports() check will. This contract mismatch can cause inconsistent provider selection behavior between the catalog layer and the runtime layer.
🔧 Proposed fix
file_extensions=(".js",
".jsx",
".mjs",
".cjs",
".ts",
".tsx",
".json",
".jsonc",
".css",
".graphql",
+ ".gql"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| file_extensions=(".js", | |
| ".jsx", | |
| ".mjs", | |
| ".cjs", | |
| ".ts", | |
| ".tsx", | |
| ".json", | |
| ".jsonc", | |
| ".css", | |
| ".graphql"), | |
| file_extensions=(".js", | |
| ".jsx", | |
| ".mjs", | |
| ".cjs", | |
| ".ts", | |
| ".tsx", | |
| ".json", | |
| ".jsonc", | |
| ".css", | |
| ".graphql", | |
| ".gql"), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/extensions/linters/biome/manifest.py` around lines 32 - 41, The
Biome manifest and provider are out of sync because `manifest.py` omits `.gql`
from `file_extensions` while `provider.py` already advertises it in
`_EXTENSIONS`. Update the `BiomeManifest` `file_extensions` tuple to include
`.gql` so catalog queries like `linters_for_language` and other extension-based
lookups resolve `.gql` files consistently with `supports()`.
Comment on lines
+14
to
+20
| """Oxlint linter microservice (manifest/package-contract skeleton only). | ||
|
|
||
| No provider.py in this migration: only Ruff has a fully working | ||
| provider. This microservice is metadata-only until a future stage | ||
| implements its runtime provider. See | ||
| docs/architecture/ecli-f4-linter-microservices-design.md. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale docstring: "only Ruff has a fully working provider" is incorrect.
This PR adds 13 working providers (Biome, Zig, Clang-Tidy, Cppcheck, Checkstyle, PMD, ShellCheck, Markdownlint, Yamllint, Actionlint, Hadolint, Taplo, Cargo Clippy). The statement "only Ruff has a fully working provider" is factually incorrect and likely copied from an earlier migration step. Update to reflect that oxlint is metadata-only while other providers are already implemented.
📝 Proposed fix
"""Oxlint linter microservice (manifest/package-contract skeleton only).
-No provider.py in this migration: only Ruff has a fully working
-provider. This microservice is metadata-only until a future stage
-implements its runtime provider. See
+No provider.py in this migration: this microservice is metadata-only
+until a future stage implements its runtime provider. See
docs/architecture/ecli-f4-linter-microservices-design.md.
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| """Oxlint linter microservice (manifest/package-contract skeleton only). | |
| No provider.py in this migration: only Ruff has a fully working | |
| provider. This microservice is metadata-only until a future stage | |
| implements its runtime provider. See | |
| docs/architecture/ecli-f4-linter-microservices-design.md. | |
| """ | |
| """Oxlint linter microservice (manifest/package-contract skeleton only). | |
| No provider.py in this migration: this microservice is metadata-only | |
| until a future stage implements its runtime provider. See | |
| docs/architecture/ecli-f4-linter-microservices-design.md. | |
| """ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/extensions/linters/oxlint/__init__.py` around lines 14 - 20, The
docstring in the Oxlint package skeleton is stale because it still claims only
Ruff has a fully working provider. Update the module-level documentation in
__init__.py to describe Oxlint as metadata-only without referencing Ruff as the
sole implemented provider, and align the wording with the current state of the
other provider modules (for example, the existing provider implementations such
as Biome, Zig, Clang-Tidy, Cppcheck, Checkstyle, PMD, ShellCheck, Markdownlint,
Yamllint, Actionlint, Hadolint, Taplo, and Cargo Clippy).
Comment on lines
+14
to
+20
| """Stylelint linter microservice (manifest/package-contract skeleton only). | ||
|
|
||
| No provider.py in this migration: only Ruff has a fully working | ||
| provider. This microservice is metadata-only until a future stage | ||
| implements its runtime provider. See | ||
| docs/architecture/ecli-f4-linter-microservices-design.md. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale docstring claims "only Ruff has a fully working provider."
This PR adds 13 working providers (Biome, Zig, Clang-Tidy, Cppcheck, Checkstyle, PMD, ShellCheck, Markdownlint, Yamllint, Actionlint, Hadolint, Taplo, Cargo Clippy). The docstring should say something like "No provider.py in this migration for this microservice" rather than making a global claim that is already inaccurate.
📝 Proposed fix
"""Stylelint linter microservice (manifest/package-contract skeleton only).
-No provider.py in this migration: only Ruff has a fully working
-provider. This microservice is metadata-only until a future stage
-implements its runtime provider. See
+No provider.py for this microservice in this migration. It is
+metadata-only until a future stage implements its runtime provider. See
docs/architecture/ecli-f4-linter-microservices-design.md.
"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| """Stylelint linter microservice (manifest/package-contract skeleton only). | |
| No provider.py in this migration: only Ruff has a fully working | |
| provider. This microservice is metadata-only until a future stage | |
| implements its runtime provider. See | |
| docs/architecture/ecli-f4-linter-microservices-design.md. | |
| """ | |
| """Stylelint linter microservice (manifest/package-contract skeleton only). | |
| No provider.py for this microservice in this migration. It is | |
| metadata-only until a future stage implements its runtime provider. See | |
| docs/architecture/ecli-f4-linter-microservices-design.md. | |
| """ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/extensions/linters/stylelint/__init__.py` around lines 14 - 20, The
module docstring in Stylelint’s __init__ is stale because it makes a global
claim that only Ruff has a fully working provider. Update the docstring in the
stylelint microservice skeleton to describe only this microservice’s status,
keeping the note that there is no provider.py in this migration and that it is
metadata-only for now, without referencing Ruff or the broader set of providers.
Use the existing __init__ module text as the place to revise.
Comment on lines
+22
to
+31
| from ecli.extensions.linters.stylelint import ( | ||
| manifest, | ||
| package_contract, | ||
| ) | ||
|
|
||
|
|
||
| __all__ = [ | ||
| "manifest", | ||
| "package_contract", | ||
| ] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the stale "only Ruff" docstring across all metadata-only skeleton __init__.py files
rg -n "only Ruff has a fully working" src/ecli/extensions/linters/*/__init__.pyRepository: SSobol77/ecli
Length of output: 1202
Remove the stale Ruff-only docstring here and in the other metadata-only skeletons — src/ecli/extensions/linters/stylelint/__init__.py still has the claim, and the same text is duplicated in src/ecli/extensions/linters/clang_format/__init__.py, src/ecli/extensions/linters/eslint/__init__.py, src/ecli/extensions/linters/golangci_lint/__init__.py, src/ecli/extensions/linters/java_spotbugs/__init__.py, src/ecli/extensions/linters/oxlint/__init__.py, src/ecli/extensions/linters/pylint/__init__.py, src/ecli/extensions/linters/sqlfluff/__init__.py, and src/ecli/extensions/linters/tflint/__init__.py.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/extensions/linters/stylelint/__init__.py` around lines 22 - 31,
Remove the stale Ruff-only docstring from the metadata-only linter package
skeletons and keep these modules as simple re-export/manifest files. Update the
affected __init__.py modules for stylelint, clang_format, eslint, golangci_lint,
java_spotbugs, oxlint, pylint, sqlfluff, and tflint by deleting the outdated
docstring text and leaving the manifest/package_contract imports and __all__
definitions intact.
Comment on lines
+318
to
+338
| def test_only_implemented_providers_are_registered_with_linter_bridge() -> None: | ||
| """LinterBridge.py -- the F4 provider-registration site -- must register | ||
| exactly the providers that have a real provider.py, and nothing else | ||
| (no legacy/optional/not-yet-safe provider is registered). | ||
| """ | ||
| bridge_source = (SRC_ROOT / "integrations" / "LinterBridge.py").read_text( | ||
| encoding="utf-8" | ||
| ) | ||
| assert "register_provider" in bridge_source | ||
| referenced_providers = set(re.findall(r"(\w+)DiagnosticProvider\(", bridge_source)) | ||
| expected = { | ||
| "".join(part.title() for part in name.split("_")) | ||
| for name in IMPLEMENTED_PROVIDER_DIRECTORIES | ||
| } | ||
| # ShellCheck's class name capitalizes "Check" mid-word; normalize both | ||
| # sides by comparing case-insensitively against the known class names. | ||
| expected = {"ShellCheck" if name == "Shellcheck" else name for name in expected} | ||
| assert referenced_providers == expected, ( | ||
| f"LinterBridge provider registration drifted from the implemented " | ||
| f"set: {referenced_providers.symmetric_difference(expected)}" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Comment says "case-insensitively" but code does a targeted replacement.
Line 333-334 comment states "normalize both sides by comparing case-insensitively against the known class names," but the code replaces only "Shellcheck" with "ShellCheck" on the expected side, then does a case-sensitive == comparison. The code is correct; the comment is misleading.
📝 Proposed comment fix
- # ShellCheck's class name capitalizes "Check" mid-word; normalize both
- # sides by comparing case-insensitively against the known class names.
+ # ShellCheck's class name capitalizes "Check" mid-word; correct the
+ # expected set so the case-sensitive set equality check matches.
expected = {"ShellCheck" if name == "Shellcheck" else name for name in expected}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_only_implemented_providers_are_registered_with_linter_bridge() -> None: | |
| """LinterBridge.py -- the F4 provider-registration site -- must register | |
| exactly the providers that have a real provider.py, and nothing else | |
| (no legacy/optional/not-yet-safe provider is registered). | |
| """ | |
| bridge_source = (SRC_ROOT / "integrations" / "LinterBridge.py").read_text( | |
| encoding="utf-8" | |
| ) | |
| assert "register_provider" in bridge_source | |
| referenced_providers = set(re.findall(r"(\w+)DiagnosticProvider\(", bridge_source)) | |
| expected = { | |
| "".join(part.title() for part in name.split("_")) | |
| for name in IMPLEMENTED_PROVIDER_DIRECTORIES | |
| } | |
| # ShellCheck's class name capitalizes "Check" mid-word; normalize both | |
| # sides by comparing case-insensitively against the known class names. | |
| expected = {"ShellCheck" if name == "Shellcheck" else name for name in expected} | |
| assert referenced_providers == expected, ( | |
| f"LinterBridge provider registration drifted from the implemented " | |
| f"set: {referenced_providers.symmetric_difference(expected)}" | |
| ) | |
| def test_only_implemented_providers_are_registered_with_linter_bridge() -> None: | |
| """LinterBridge.py -- the F4 provider-registration site -- must register | |
| exactly the providers that have a real provider.py, and nothing else | |
| (no legacy/optional/not-yet-safe provider is registered). | |
| """ | |
| bridge_source = (SRC_ROOT / "integrations" / "LinterBridge.py").read_text( | |
| encoding="utf-8" | |
| ) | |
| assert "register_provider" in bridge_source | |
| referenced_providers = set(re.findall(r"(\w+)DiagnosticProvider\(", bridge_source)) | |
| expected = { | |
| "".join(part.title() for part in name.split("_")) | |
| for name in IMPLEMENTED_PROVIDER_DIRECTORIES | |
| } | |
| # ShellCheck's class name capitalizes "Check" mid-word; correct the | |
| # expected set so the case-sensitive set equality check matches. | |
| expected = {"ShellCheck" if name == "Shellcheck" else name for name in expected} | |
| assert referenced_providers == expected, ( | |
| f"LinterBridge provider registration drifted from the implemented " | |
| f"set: {referenced_providers.symmetric_difference(expected)}" | |
| ) |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 327-327: Simplify this regular expression to reduce its runtime, as it has super-linear performance due to backtracking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/extensions/test_linters_namespace_migration.py` around lines 318 - 338,
The normalization comment in
test_only_implemented_providers_are_registered_with_linter_bridge is misleading
because the code only special-cases Shellcheck/ShellCheck rather than doing a
general case-insensitive comparison. Update the nearby inline comment to
accurately describe the actual expected-name normalization in the referenced
test, or remove the case-insensitive wording so it matches the implementation
and avoids confusion.
Comment on lines
+93
to
+98
| def _default_or_recommended(entries: tuple[LinterDefinition, ...]): | ||
| return tuple( | ||
| entry | ||
| for entry in entries | ||
| if entry.enabled_by_default or entry.tier in ("core", "recommended") | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Add return type annotation to _default_or_recommended.
The parameter type is annotated but the return type is missing. Adding it improves readability and consistency with the rest of the file's typed style.
📝 Proposed fix
-def _default_or_recommended(entries: tuple[LinterDefinition, ...]):
+def _default_or_recommended(
+ entries: tuple[LinterDefinition, ...],
+) -> tuple[LinterDefinition, ...]:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _default_or_recommended(entries: tuple[LinterDefinition, ...]): | |
| return tuple( | |
| entry | |
| for entry in entries | |
| if entry.enabled_by_default or entry.tier in ("core", "recommended") | |
| ) | |
| def _default_or_recommended( | |
| entries: tuple[LinterDefinition, ...], | |
| ) -> tuple[LinterDefinition, ...]: | |
| return tuple( | |
| entry | |
| for entry in entries | |
| if entry.enabled_by_default or entry.tier in ("core", "recommended") | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/extensions/test_linters_registry_contract.py` around lines 93 - 98, The
helper _default_or_recommended is missing a return type annotation despite
having a typed parameter. Update the function signature for
_default_or_recommended in the linters registry contract test to explicitly
annotate the tuple return type that matches the filtered LinterDefinition
entries, keeping it consistent with the file’s typed style.
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
1 participant
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.
Implements F4 linter microservices under the Extensions Layer, preserving the existing F4 panel UI while moving linter diagnostics into src/ecli/extensions/linters. Ruff remains the reference provider; Biome, markdownlint, yamllint, shellcheck, Zig, Hadolint, Taplo, Actionlint, C/C++, Java and Cargo Clippy provider paths are added with provider/parser/package-contract structure. This also restores diagnostics selection readability, jump visibility, path normalization, and EOF diagnostic navigation.
Summary by CodeRabbit
New Features
Bug Fixes
Tests