Abstract bump lockfile operations behind a repository port (#82) - #135
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Summary
References: issue WalkthroughReplace direct ChangesLockfile repository ports and bump wiring
Publish lockfile inspection
Sequence Diagram(s)sequenceDiagram
participant CLI
participant bump as bump.run
participant repository as CargoLockfileRepository
participant runner as CommandRunner
CLI->>bump: BumpOptions(lockfile_repository)
bump->>repository: resolve_lockfile_paths() or regenerate_lockfiles()
repository->>runner: Cargo command
runner-->>repository: command result
repository-->>bump: lockfile paths
sequenceDiagram
participant preflight as publish_preflight
participant repository as CargoLockfileInspectionRepository
participant runner as CommandRunner
preflight->>repository: discover_tracked_lockfiles()
repository->>runner: git command
runner-->>repository: tracked paths
preflight->>repository: validate_lockfile_freshness()
repository->>runner: cargo metadata
runner-->>repository: freshness result
repository-->>preflight: LockfileFreshness
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideRefactors bump lockfile handling to depend on a LockfileRepository port instead of a raw CommandRunner, introduces a Cargo-backed repository adapter, wires the CLI and bump options to use it, and adds tests and docs covering the new abstraction and dry-run behavior. Sequence diagram for bump lockfile processing via LockfileRepositorysequenceDiagram
actor User
participant CLI as lading.cli.bump
participant Bump as bump.run
participant Process as _process_lockfiles
participant Repo as CargoLockfileRepository
User->>CLI: invoke bump(...)
CLI->>Repo: CargoLockfileRepository(runner=command_runner)
CLI->>Bump: run(options.with(lockfile_repository=Repo))
Bump->>Process: _process_lockfiles(context)
alt dry_run
Process->>Repo: resolve_lockfile_paths(root_path, lockfile_manifests)
Repo-->>Process: lockfile_paths
else live_run
Process->>Repo: regenerate_lockfiles(root_path, lockfile_manifests)
Repo-->>Process: rewritten_lockfile_paths
end
Process-->>Bump: lockfile_paths
Bump-->>CLI: BumpChanges
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Failed
(1 file with Low Cohesion)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| test_bump_command_integration.py | 1 critical rule | 10.00 → 8.03 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
cc47439 to
bbb546b
Compare
bbb546b to
30933bf
Compare
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
30933bf to
a078db3
Compare
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. tests/unit/test_bump_command_integration.py Comment on file from __future__ import annotations
import collections.abc as cabc❌ New issue: Low Cohesion |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
d7a7786 to
b2a7d9a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
b2a7d9a to
9774d03
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/developers-guide.md`:
- Around line 213-224: The new issue callout in the developers guide uses bare
“issue 82” text instead of the document’s existing GitHub-linked convention.
Update the prose around lading.commands.bump.run and the lockfile_repository /
bump_lockfiles.LockfileRepository discussion to use the same issue-reference
style as the rest of the file (for example, `#82` formatting) wherever that
reference appears, including the parenthetical at the end.
In `@lading/commands/bump_lockfiles.py`:
- Around line 240-287: The public contract on LockfileRepository and
CargoLockfileRepository is under-documented: their method docstrings only give
brief summaries and do not describe inputs, outputs, or the fact that
regenerate_lockfiles can raise LockfileRegenerationError via the delegated
regenerate_lockfiles function. Expand the docstrings on resolve_lockfile_paths
and regenerate_lockfiles in both the Protocol and CargoLockfileRepository to
include clear Parameters/Returns/Raises-style details, especially the error
behavior, so callers can rely on the port without inspecting the implementation.
In `@lading/commands/lockfile.py`:
- Around line 266-286: The environment-bound wrapper in `_bound_runner` is
ignoring the caller’s `echo_stdout` argument whenever it returns
`runner_with_env`, so preserve and forward that parameter instead of discarding
it. Update the `runner_with_env` closure to accept `echo_stdout` and pass it
through to `base_runner`, while still applying `base_env` only when `env` is not
provided.
In `@tests/unit/test_bump_lockfile_repository.py`:
- Line 1: The module docstring in test_bump_lockfile_repository is triggering an
E501 suppression without justification; either shorten the docstring so the line
no longer exceeds the limit or replace the bare # noqa: E501 on that docstring
with an inline reason. Update the top-level docstring in this test module so the
suppression, if kept, includes a clear justification rather than only the code.
In `@tests/unit/test_lockfile.py`:
- Around line 440-459: Consolidate the new _recording_runner test double with
the existing CallTrackingRunner instead of keeping two near-identical helpers.
Update CallTrackingRunner in tests/unit/publish/conftest.py to also capture env
if needed, then refactor the lockfile tests to use that shared runner for
command, cwd, and env recording. Keep the behavior of _recording_runner callers
intact while removing the duplicated ad-hoc implementation.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 21c9d48e-6912-4cb8-9dbd-2a1f2fee9006
📒 Files selected for processing (11)
docs/developers-guide.mdlading/cli.pylading/commands/__init__.pylading/commands/bump.pylading/commands/bump_lockfiles.pylading/commands/lockfile.pylading/commands/publish_preflight.pytests/unit/publish/test_preflight_lockfile_validation.pytests/unit/test_bump_lockfile_repository.pytests/unit/test_cli.pytests/unit/test_lockfile.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/shared-actions(auto-detected)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (1 error, 1 warning)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git fetch origin pull/135/head:pr-135 2>&1 | tail -5
git checkout pr-135 2>&1 | tail -5
echo "---_bound_runner---"
rg -n "_bound_runner" lading/commands/lockfile.py -A 20
echo "---echo_stdout in lockfile.py---"
rg -n "echo_stdout" lading/commands/lockfile.py
echo "---echo_stdout in tests---"
rg -n "echo_stdout" tests/unit/test_lockfile.py
echo "---design doc check---"
fd -i "design" docs
rg -n "LockfileRepository|LockfileInspectionRepository|lockfile" docs/lading-design.md 2>/dev/null
echo "---issue 82 mentions---"
rg -n "issue 82|`#82`" docs/developers-guide.md
echo "---git log recent---"
git log --oneline -10Length of output: 2574 🏁 Script executed: #!/bin/bash
echo "---_bound_runner---"
rg -n "_bound_runner" lading/commands/lockfile.py -A 20
echo "---echo_stdout in lockfile.py---"
rg -n "echo_stdout" lading/commands/lockfile.py
echo "---echo_stdout in tests---"
rg -n "echo_stdout" tests/unit/test_lockfile.py
echo "---design doc files---"
fd -i "design" docs
echo "---design doc lockfile mentions---"
rg -n "LockfileRepository|LockfileInspectionRepository|lockfile" docs/lading-design.md 2>/dev/null
echo "---issue 82 mentions in developers-guide---"
rg -n "issue 82|`#82`" docs/developers-guide.md
echo "---git log recent---"
git log --oneline -10Length of output: 4932
Testing (Overall) — resolved. Developer Documentation — resolved. No further action is required for either check. Do not open a follow-up issue; there is no remaining scope gap here. |
BumpOptions, a public domain dataclass, exposed a CommandRunner field, leaking execution infrastructure into the domain boundary. Introduce the LockfileRepository protocol in bump_lockfiles as the port through which the bump domain projects and regenerates Cargo lockfiles, with CargoLockfileRepository as the cargo-backed adapter bound to a command runner. BumpOptions now carries lockfile_repository instead of command_runner; the CLI binds the adapter to its selected runner at the composition root, and bump falls back to the default adapter when no repository is injected. The port's documented scope is bump-side lockfile projection and regeneration; publish-side discovery and validation continue to take a CommandRunner directly. Tests inject a recording repository to verify live runs regenerate and dry runs only project, without touching Cargo. Closes #82
Extend the issue #82 repository/VCS port abstraction to the publish side. Previously `lockfile.py` discovery and freshness validation, and the `publish_preflight` domain that drives them, took a `CommandRunner` directly, mixing git VCS, filesystem, and cargo execution with the freshness-classification logic. Add a `LockfileInspectionRepository` port and a git/cargo-backed `CargoLockfileInspectionRepository` adapter (binding a runner and the optional pre-flight environment). `_validate_lockfile_freshness` and `_collect_stale_lockfiles` now depend only on the port; `_run_preflight_checks` is the composition root that binds the adapter, and tests inject a port double at the `_validate_lockfile_freshness` seam. This is the publish-side counterpart to the bump-side `bump_lockfiles.LockfileRepository`, so neither lockfile domain holds a raw command runner. Rewrite the pre-flight validation tests to inject a recording port double and add adapter tests covering env binding, delegation, and the `manifest_exists` predicate. Update the developer guide to document the publish-side port and drop the bump-side-only scope caveat. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve the CodeScene Low Cohesion finding on `tests/unit/test_bump_lockfile_rebuild.py` by moving the injected-port integration tests into a focused module. `_RecordingLockfileRepository`, `test_run_uses_injected_lockfile_repository`, and `test_dry_run_projects_through_lockfile_repository` move verbatim into the new `tests/unit/test_bump_lockfile_repository.py`; the original file keeps the monkeypatch-based rebuild scenarios and `_LockfileSkipScenario`. Imports are trimmed to what each file needs: the new module imports only `cabc`, `pathlib`, `bump`, and the workspace builders, and the original drops the now-unused `collections.abc` import. No production code changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Resolve reviewer findings on the issue #82 lockfile port work: - lockfile.py: `CargoLockfileInspectionRepository._bound_runner` now forwards `echo_stdout` (and any other runner keyword) to the bound runner instead of discarding it; only `env` is defaulted to the bound base environment. Implemented as a transparent `**kwargs` proxy so callers that omit `echo_stdout` leave the underlying runner's default intact and existing runner doubles need no change. - test_lockfile.py: `_recording_runner` now records `echo_stdout`; adds `test_adapter_bound_runner_forwards_echo_stdout`, which drives the env-bound runner with `echo_stdout=False` and asserts it is forwarded unchanged. A `_RecordedCall` type alias keeps the annotations tidy. - bump_lockfiles.py: expand the `LockfileRepository` Protocol and `CargoLockfileRepository` adapter method docstrings to full NumPy-style Parameters/Returns/Raises, documenting the `LockfileRegenerationError` behaviour. (The port methods keep docstring-only bodies; no `...`.) - test_bump_lockfile_repository.py: justify the module-docstring `# noqa: E501` (the one-line summary is fixed; wrapping trips D205/D209). - docs/developers-guide.md: use the file's `issue #82` reference style. - docs/lading-design.md: document the bump- and publish-side lockfile repository ports. The "consolidate the recording runner doubles" suggestion is skipped: the publish `CallTrackingRunner` (a class recording 2-tuples with env discarded, in a non-importable publish conftest) and the lockfile `_recording_runner` are materially different, and merging them would churn three unrelated publish tests for no behavioural gain. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
5cf208b to
269f4e9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- lockfile.py: expand the discover_tracked_lockfiles and validate_lockfile_freshness docstrings on both the CargoLockfileInspectionRepository adapter and the LockfileInspectionRepository protocol to full NumPy Parameters/Returns sections (protocol stubs stay docstring-only, no ellipsis). - test_lockfile.py: attach failure messages to the bare asserts in the five CargoLockfileInspectionRepository adapter tests. The suggestion to group the adapter tests under a test class is skipped: the entire test suite is module-level functions (no test classes in any file), so a lone class here would be an inconsistency island; CodeRabbit itself rated it a poor tradeoff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@coderabbitai Have the following now been resolved? tests/unit/test_lockfile.py (2)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions 🧪 Example fix for one assertion- assert result == (tmp_path / "Cargo.lock",)
- assert calls == [
- (("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), tmp_path, base_env, True)
- ]
+ assert result == (tmp_path / "Cargo.lock",), "expected discovered lockfile"
+ assert calls == [
+ (("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), tmp_path, base_env, True)
+ ], "git ls-files call did not receive the bound env"🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions lading/commands/lockfile.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git fetch origin pull/135/head:pr-135 2>&1 | tail -5
git checkout pr-135 2>&1 | tail -5
git log --oneline -5
echo "---class grouping check---"
rg -n "^class |^def test_adapter" tests/unit/test_lockfile.py
echo "---bare asserts around adapter tests---"
sed -n '460,570p' tests/unit/test_lockfile.py
echo "---lockfile.py docstrings---"
sed -n '255,320p' lading/commands/lockfile.pyLength of output: 8656
Bare asserts — resolved. Every assertion in Docstrings — resolved. Class grouping — not resolved. The five [resolve] |
Move the five test_adapter_* functions into a new TestCargoLockfileInspectionRepositoryAdapter class as methods, keeping their names, docstrings, assertions, and helper usage (_recording_runner, _RecordedCall) unchanged. The _recording_runner factory and _RecordedCall alias stay at module scope since they are shared across the file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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/unit/test_lockfile.py`:
- Around line 471-475: The three adapter tests repeat the same
Cargo.toml/Cargo.lock setup, so extract that shared manifest/lockfile
preparation into a pytest fixture instead of duplicating the write_text calls.
Add the fixture in the shared test setup area (for example conftest.py or a
fixtures module) and update test_adapter_discovers_lockfiles_binding_env,
test_adapter_without_env_leaves_runner_env_untouched, and
test_adapter_honours_injected_manifest_exists to use it while keeping each
test’s specific assertions intact.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 606682bc-b3f3-45ed-a7bc-e0e0ba3ba1f3
📒 Files selected for processing (2)
lading/commands/lockfile.pytests/unit/test_lockfile.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/shared-actions(auto-detected)
Three CargoLockfileInspectionRepository adapter tests wrote the same root Cargo.toml/Cargo.lock pair inline. Introduce a module-local _cargo_workspace pytest fixture (mirroring the existing _metrics_registry pattern) and apply it via @pytest.mark.usefixtures on the three tests that need the pair, dropping the duplicated write_text calls. Each test keeps its tmp_path parameter and assertions unchanged; the two adapter tests that do not need a workspace on disk are left untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
Closes #82
BumpOptions, a public domain dataclass, no longer exposes aCommandRunnerfield. It now carrieslockfile_repository, abump_lockfiles.LockfileRepositoryprotocol.LockfileRepositoryis the port through which the bump domain projects (resolve_lockfile_paths) and regenerates (regenerate_lockfiles) Cargo lockfiles;CargoLockfileRepositoryis the cargo-backed adapter bound to a command runner.lockfile.pynow go through a siblingLockfileInspectionRepositoryport, withCargoLockfileInspectionRepositoryas the git/cargo-backed adapter. The publish pre-flight domain (_validate_lockfile_freshness,_collect_stale_lockfiles) depends only on the port;publish_preflight._run_preflight_checksis the composition root that binds the adapter to the selected runner and pre-flight environment, and tests inject a port double at the_validate_lockfile_freshnessseam. This fully closes Remove infrastructure _CommandRunner from public BumpOptions and abstract VCS/filesystem behind a repository port #82: neither the bump nor the publish lockfile domain holds a rawCommandRunner.Testing
bump.runand verify live runs regenerate while dry runs only project, without touching Cargo.LockfileInspectionRepositorydouble; new adapter tests cover env binding, delegation, and themanifest_existspredicate.make check-fmt,make lint,make typecheck,make test(701 passed),make markdownlint, andmake nixieall green.🤖 Generated with Claude Code
Summary by Sourcery
Introduce a lockfile repository port for bump operations to decouple lockfile handling from direct command runner usage and wire it through the CLI and tests.
New Features:
Enhancements:
Tests:
References