feat: add F4 linter provisioning framework#125
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a provider-neutral F4 Full linter provisioning contract: new dataclasses and registry rules, planning and evidence generation/verification logic, two CLI scripts, per-linter contract metadata updates, documentation, and tests. Two unrelated scripts receive cosmetic reformatting. ChangesF4 Linter Provisioning Feature
Unrelated Formatting Tweaks
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 6
🤖 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/release/release-checklist.md`:
- Around line 81-84: The checklist examples for provision_f4_linters.py and
verify_f4_linter_provisioning.py are missing required path flags, so update the
release checklist entry to include the needed arguments for both commands. Add
--target-dir and --evidence-dir to the provision_f4_linters.py example, and add
--evidence-dir to the verify_f4_linter_provisioning.py example, keeping the
commands in docs/release/release-checklist.md runnable as written.
In `@scripts/provision_f4_linters.py`:
- Around line 153-213: main() is too complex because it mixes the list-selection
branch and the provisioning branch; split those paths into dedicated helpers.
Extract the --list-selection-options handling into a helper that builds and
prints the models, and extract the provisioning loop into another helper that
writes evidence, collects output, and computes failure state. Keep main()
focused on argument setup plus dispatching to these helpers while preserving the
existing behavior of build_provisioning_plan(), write_evidence(),
_print_human(), and plan_has_release_blocking_failure().
In `@src/ecli/extensions/linters/core/provisioning_contract.py`:
- Line 194: `ProvisioningEvidence` still exposes a mutable `os_context` even
though the dataclass is frozen, so callers can mutate evidence after
construction. Update the `ProvisioningEvidence` definition to make `os_context`
immutable by changing the type to `Mapping[str, str]` and/or normalizing it in
`__post_init__` to a read-only mapping, using the `os_context` field as the
location to fix. Ensure any construction or serialization paths in
`ProvisioningEvidence` continue to work with the immutable representation.
In `@src/ecli/extensions/linters/core/provisioning_registry.py`:
- Around line 40-59: The first two ArtifactContractEntry definitions in
provisioning_registry.py are passing the constrained-artifact fields
positionally, which makes the intent unclear and fragile. Update those entries
to use the named parameters full_provisioning_supported= and minimal_reason=,
matching the contract style used elsewhere and keeping the ArtifactContractEntry
calls in core/provisioning_registry.py explicit and stable if the constructor
changes.
In `@src/ecli/extensions/linters/core/provisioning.py`:
- Around line 683-750: `verify_evidence_payload` is over the
cognitive-complexity threshold, so split its validation logic into smaller
helpers and keep the main function as a simple orchestrator. Extract the
header/payload field checks and the per-required-tool validation loop into
dedicated functions near `verify_evidence_payload`, reusing symbols like
`artifact_entry_id`, `loaded`, `required`, and `_strategy_errors` so the logic
stays easy to locate. Apply the same refactor pattern to `_selection_json`,
`resolve_profile`, `_action_for_contract`, and `verify_evidence_dir` by moving
branching-heavy chunks into focused helper functions until each method is below
the SonarCloud limit.
In `@src/ecli/extensions/linters/core/registry.py`:
- Around line 167-180: Derive the runtime allowlists from the existing Literal
types instead of hardcoding them, so `ALLOWED_INSTALL_MECHANISMS` and the
corresponding `ALLOWED_*` frozensets stay in sync automatically. Update the
registry logic around `InstallMechanism` and `ProvenanceRequirement` to build
the frozensets with `typing.get_args()` from the Literal definitions, and keep
the surrounding validation code using those derived sets.
🪄 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: 45d646fe-3c61-43c6-9003-fdfd239eab37
📒 Files selected for processing (41)
docs/extensions/diagnostics-linter-layer.mddocs/release/README-release.mddocs/release/artifact-contract.mddocs/release/artifact-verification.mddocs/release/build-matrix.mddocs/release/packaging-flows.mddocs/release/release-checklist.mdscripts/build_and_package_arch.pyscripts/provision_f4_linters.pyscripts/verify_f4_linter_provisioning.pyscripts/verify_release_assets.pysrc/ecli/extensions/linters/actionlint/package_contract.pysrc/ecli/extensions/linters/biome/package_contract.pysrc/ecli/extensions/linters/cargo_clippy/package_contract.pysrc/ecli/extensions/linters/clang_format/package_contract.pysrc/ecli/extensions/linters/clang_tidy/package_contract.pysrc/ecli/extensions/linters/core/provisioning.pysrc/ecli/extensions/linters/core/provisioning_contract.pysrc/ecli/extensions/linters/core/provisioning_registry.pysrc/ecli/extensions/linters/core/registry.pysrc/ecli/extensions/linters/cppcheck/package_contract.pysrc/ecli/extensions/linters/eslint/package_contract.pysrc/ecli/extensions/linters/golangci_lint/package_contract.pysrc/ecli/extensions/linters/hadolint/package_contract.pysrc/ecli/extensions/linters/java_checkstyle/package_contract.pysrc/ecli/extensions/linters/java_pmd/package_contract.pysrc/ecli/extensions/linters/java_spotbugs/package_contract.pysrc/ecli/extensions/linters/markdownlint/package_contract.pysrc/ecli/extensions/linters/oxlint/package_contract.pysrc/ecli/extensions/linters/pylint/package_contract.pysrc/ecli/extensions/linters/ruff/package_contract.pysrc/ecli/extensions/linters/shellcheck/package_contract.pysrc/ecli/extensions/linters/sqlfluff/package_contract.pysrc/ecli/extensions/linters/stylelint/package_contract.pysrc/ecli/extensions/linters/taplo/package_contract.pysrc/ecli/extensions/linters/tflint/package_contract.pysrc/ecli/extensions/linters/yamllint/package_contract.pysrc/ecli/extensions/linters/zig/package_contract.pytests/extensions/linters/test_provisioning_contract.pytests/packaging/test_f4_linter_provisioning_scripts.pytests/packaging/test_packaging_release_contract.py
|
|
||
| artifact_entry_id: str | ||
| run_mode: ProvisioningMode | ||
| os_context: dict[str, str] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
os_context is a mutable dict inside a frozen dataclass.
ProvisioningEvidence is frozen=True, but os_context: dict[str, str] remains mutable — callers can alter it after construction, potentially corrupting evidence before serialization. Consider using Mapping[str, str] for the type hint or copying into a types.MappingProxyType in __post_init__ to enforce immutability.
🤖 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/core/provisioning_contract.py` at line 194,
`ProvisioningEvidence` still exposes a mutable `os_context` even though the
dataclass is frozen, so callers can mutate evidence after construction. Update
the `ProvisioningEvidence` definition to make `os_context` immutable by changing
the type to `Mapping[str, str]` and/or normalizing it in `__post_init__` to a
read-only mapping, using the `os_context` field as the location to fix. Ensure
any construction or serialization paths in `ProvisioningEvidence` continue to
work with the immutable representation.
| ArtifactContractEntry( | ||
| 1, | ||
| "pypi-wheel", | ||
| "PyPI wheel", | ||
| "PyPI / Python", | ||
| "ecli_editor-<version>-py3-none-any.whl", | ||
| False, | ||
| "Python wheel metadata cannot reliably provision Node, Rust, Go, Zig, " | ||
| "Java, or system binaries.", | ||
| ), | ||
| ArtifactContractEntry( | ||
| 2, | ||
| "pypi-sdist", | ||
| "PyPI source distribution", | ||
| "PyPI / Python", | ||
| "ecli_editor-<version>.tar.gz", | ||
| False, | ||
| "Source distribution installs are developer/source installs and may be " | ||
| "minimal for non-Python F4 toolchains.", | ||
| ), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f provisioning_contract.py src | xargs -I{} ast-grep run --pattern 'class ArtifactContractEntry:
$$$' --lang python {}
rg -nP 'full_provisioning_supported|minimal_reason' src/ecli/extensions/linters/core/provisioning_contract.pyRepository: SSobol77/ecli
Length of output: 1137
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,240p' src/ecli/extensions/linters/core/provisioning_registry.pyRepository: SSobol77/ecli
Length of output: 6323
Prefer keyword arguments for the constrained-artifact flag.
Entries 1–2 pass False and the reason as bare positionals, while the rest of the contract omits both. Using full_provisioning_supported= and minimal_reason= here makes the intent explicit and avoids accidental argument-order mistakes if ArtifactContractEntry changes.
🤖 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/core/provisioning_registry.py` around lines 40 -
59, The first two ArtifactContractEntry definitions in provisioning_registry.py
are passing the constrained-artifact fields positionally, which makes the intent
unclear and fragile. Update those entries to use the named parameters
full_provisioning_supported= and minimal_reason=, matching the contract style
used elsewhere and keeping the ArtifactContractEntry calls in
core/provisioning_registry.py explicit and stable if the constructor changes.
| ALLOWED_INSTALL_MECHANISMS: frozenset[str] = frozenset( | ||
| { | ||
| "artifact-policy", | ||
| "bundled-binary", | ||
| "bundled-internal", | ||
| "ecli-managed-tools", | ||
| "jar-shim", | ||
| "language-package-manager", | ||
| "nix-derivation", | ||
| "os-package-manager", | ||
| "toolchain-component", | ||
| "verified-upstream-download", | ||
| } | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Derive ALLOWED_* frozensets from their Literal types to eliminate manual sync.
The InstallMechanism/ProvenanceRequirement Literal types and their corresponding ALLOWED_* frozensets are manually synchronized. Adding a new mechanism or provenance value requires updating both the Literal and the frozenset — if one is forgotten, either the type checker or the runtime validator will be silently inconsistent. Use typing.get_args() to derive the frozensets from the Literals automatically.
♻️ Proposed refactor
+from typing import get_args
+
ALLOWED_INSTALL_MECHANISMS: frozenset[str] = frozenset(
- {
- "artifact-policy",
- "bundled-binary",
- "bundled-internal",
- "ecli-managed-tools",
- "jar-shim",
- "language-package-manager",
- "nix-derivation",
- "os-package-manager",
- "toolchain-component",
- "verified-upstream-download",
- }
+ get_args(InstallMechanism)
)
ALLOWED_PROVENANCE_REQUIREMENTS: frozenset[str] = frozenset(
- {
- "artifact-entry-id",
- "checksum-or-ecli-provenance-when-downloaded",
- "deterministic-install-log",
- "ecli-version",
- "executable-permission",
- "pinned-version-when-downloaded",
- "source-url-when-downloaded",
- "version-probe",
- }
+ get_args(ProvenanceRequirement)
)Also applies to: 193-204
🤖 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/core/registry.py` around lines 167 - 180, Derive
the runtime allowlists from the existing Literal types instead of hardcoding
them, so `ALLOWED_INSTALL_MECHANISMS` and the corresponding `ALLOWED_*`
frozensets stay in sync automatically. Update the registry logic around
`InstallMechanism` and `ProvenanceRequirement` to build the frozensets with
`typing.get_args()` from the Literal definitions, and keep the surrounding
validation code using those derived sets.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/extensions/linters/test_provisioning_contract.py`:
- Around line 191-194: The
`test_evidence_filename_rejects_pathlike_artifact_ids` loop should be replaced
with `@pytest.mark.parametrize` so each path-like `artifact_id` is tested
independently. Update the `evidence_filename` test to keep the same invalid
inputs but run them as separate cases, which ensures one failure does not skip
the rest and gives clearer per-input diagnostics.
🪄 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: ac3bce6e-0dac-4645-bf7d-5708a454bec5
📒 Files selected for processing (4)
scripts/provision_f4_linters.pysrc/ecli/extensions/linters/core/provisioning.pytests/extensions/linters/test_provisioning_contract.pytests/packaging/test_f4_linter_provisioning_scripts.py
| def test_evidence_filename_rejects_pathlike_artifact_ids() -> None: | ||
| for artifact_id in ("../deb", "/tmp/deb", "deb/../rpm", r"C:\tmp\deb"): | ||
| with pytest.raises(ValueError): | ||
| evidence_filename(artifact_id) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider using @pytest.mark.parametrize for individual input isolation.
The for-loop tests four path-like inputs, but if the first one fails to raise ValueError, the remaining three are never exercised. Parametrizing would test each independently and produce clearer failure diagnostics.
♻️ Optional refactor
+@pytest.mark.parametrize(
+ "artifact_id",
+ ["../deb", "/tmp/deb", "deb/../rpm", r"C:\tmp\deb"],
+)
-def test_evidence_filename_rejects_pathlike_artifact_ids() -> None:
- for artifact_id in ("../deb", "/tmp/deb", "deb/../rpm", r"C:\tmp\deb"):
- with pytest.raises(ValueError):
- evidence_filename(artifact_id)
+def test_evidence_filename_rejects_pathlike_artifact_ids(artifact_id: str) -> None:
+ with pytest.raises(ValueError):
+ evidence_filename(artifact_id)📝 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.
| def test_evidence_filename_rejects_pathlike_artifact_ids() -> None: | |
| for artifact_id in ("../deb", "/tmp/deb", "deb/../rpm", r"C:\tmp\deb"): | |
| with pytest.raises(ValueError): | |
| evidence_filename(artifact_id) | |
| `@pytest.mark.parametrize`( | |
| "artifact_id", | |
| ["../deb", "/tmp/deb", "deb/../rpm", r"C:\tmp\deb"], | |
| ) | |
| def test_evidence_filename_rejects_pathlike_artifact_ids(artifact_id: str) -> None: | |
| with pytest.raises(ValueError): | |
| evidence_filename(artifact_id) |
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 191-191: Do not hardcode temporary file or directory names
Context: "/tmp/deb"
Note: [CWE-377] Insecure Temporary File.
(hardcoded-tmp-file)
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 192-192: Make sure publicly writable directories are used safely here.
[failure] 192-192: Make sure publicly writable directories are used safely here.
🤖 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/linters/test_provisioning_contract.py` around lines 191 -
194, The `test_evidence_filename_rejects_pathlike_artifact_ids` loop should be
replaced with `@pytest.mark.parametrize` so each path-like `artifact_id` is
tested independently. Update the `evidence_filename` test to keep the same
invalid inputs but run them as separate cases, which ensures one failure does
not skip the rest and gives clearer per-input diagnostics.
|
SSobol77
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
Dismiss stale CodeRabbit change request after follow-up fixes and all required checks passing.
1 participant
Summary
Add the first provider-neutral F4 linter provisioning framework for ECLI.
This PR introduces the contract, planner, installer-selection model, dry-run evidence generation, and verification scripts needed for OS/artifact-aware F4 linter provisioning.
This is not yet the final artifact-specific installer/bundling implementation for every release package. It creates the framework and release evidence gates required before wiring concrete packaging actions.
Key changes
Add provider-neutral provisioning core under the F4 linter extension layer:
src/ecli/extensions/linters/core/provisioning_contract.pysrc/ecli/extensions/linters/core/provisioning_registry.pysrc/ecli/extensions/linters/core/provisioning.pyAdd release/packaging CLI entrypoints:
scripts/provision_f4_linters.pyscripts/verify_f4_linter_provisioning.pyExtend linter package contract metadata with provisioning information:
Add deterministic JSON evidence for provisioning plans.
Add installer-selection model:
Preserve the existing ECLI artifact contract:
Treat current docs and
package_contract.pyfiles as authoritative for Full-required linters.Full-required F4 baseline
The Full-required baseline is derived from current package contracts and includes:
Optional / legacy / specialist tools remain not selected by default:
Artifact behavior
The provisioning evidence model covers exactly 21 artifact entries:
pypi-wheelpypi-sdistlinux-pyinstallerlinux-tarballdebrpmopensuse-rpmarch-pkgbuildslackware-txzappimagefreebsd-pkgfreebsd-ports-chrootmacos-appmacos-dmgwindows-portable-exewindows-nsis-installernix-flakenixos-packagedocker-deb-helperdocker-rpm-helpergha-release-contractpypi-wheelandpypi-sdistare explicitly represented as constrained/minimal artifacts because Python packaging metadata cannot reliably provision Node, Rust, Go, Zig, Java, or system binaries. The evidence records this througheffective_profile,full_profile_complete, andcustom_profile_reason.Validation
Passed locally:
Results:
Manual smoke passed:
Additional evidence checks confirmed:
Boundaries
This PR does not change:
Remaining gap before v0.2.4
Concrete artifact-specific installation/bundling actions still need to be wired into the packaging flows before v0.2.4 can claim real Full provisioning across release artifacts.
This PR provides the provisioning framework, component-selection model, dry-run planner, evidence format, and verifier needed for that next step.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests