Aggregate lockfile regeneration failures in bump (#84) - #127
Conversation
Walkthrough
ChangesLockfile regeneration aggregation
Sequence Diagram(s)sequenceDiagram
participant regenerate_lockfiles
participant _attempt_single_lockfile_update
participant _run_workspace_lockfile_update
participant _build_aggregate_failure_message
loop each manifest
regenerate_lockfiles->>_attempt_single_lockfile_update: attempt update
_attempt_single_lockfile_update->>_run_workspace_lockfile_update: run cargo update
_run_workspace_lockfile_update-->>_attempt_single_lockfile_update: success or error
_attempt_single_lockfile_update-->>regenerate_lockfiles: path or failure
end
alt multiple failures
regenerate_lockfiles->>_build_aggregate_failure_message: format aggregated message
_build_aggregate_failure_message-->>regenerate_lockfiles: operator-facing details
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideImplements aggregated failure handling in regenerate_lockfiles so all manifests are attempted, failures are collected into a single LockfileRegenerationError with an operator-focused message, and adds tests (including snapshot tests) to verify the new behavior and messaging. Flow diagram for aggregated failure handling in regenerate_lockfilesflowchart TD
A[Start regenerate_lockfiles] --> B[Resolve manifests]
B --> C[Init lockfiles and failures]
C --> D{For each manifest}
D -->|Next manifest| E[Log regeneration start]
E --> F[Call _run_workspace_lockfile_update]
F --> G{LockfileRegenerationError?}
G -->|No| H[Log success and add Cargo.lock to lockfiles]
H --> D
G -->|Yes| I[Log exception and append manifest error to failures]
I --> D
D -->|No more manifests| J{Any failures?}
J -->|No| K[Log overall success and return lockfiles]
J -->|Yes| L[Select first failure and its cause]
L --> M[Call _build_aggregate_failure_message]
M --> N[Raise LockfileRegenerationError with aggregated message chained from cause]
N --> O[End]
K --> O
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Gates Failed
Enforce critical code health rules
(1 file with Bumpy Road Ahead)
Enforce advisory code health rules
(1 file with Large Method)
Our agent can fix these. Install it.
Gates Passed
4 Quality Gates Passed
Reason for failure
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| bump_lockfiles.py | 1 critical rule | 10.00 → 9.45 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| bump_lockfiles.py | 1 advisory rule | 10.00 → 9.45 | 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.
|
@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. lading/commands/bump_lockfiles.py Comment on lines +64 to +67 not named ``Cargo.toml``), or — after every manifest has been
attempted — if ``cargo update --workspace`` failed for one or more
manifests. The aggregated message lists each failed manifest with a
repair command.❌ New issue: Large Method |
|
@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. lading/commands/bump_lockfiles.py Comment on lines +64 to +67 not named ``Cargo.toml``), or — after every manifest has been
attempted — if ``cargo update --workspace`` failed for one or more
manifests. The aggregated message lists each failed manifest with a
repair command.❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
eae8151 to
9356db7
Compare
9356db7 to
9aa5c72
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. lading/commands/bump_lockfiles.py Comment on lines +65 to +70 not named ``Cargo.toml``), or — after every manifest has been
attempted — if ``cargo update --workspace`` failed. When only the
workspace-root lockfile is regenerated, the original cargo error is
re-raised unchanged. When several lockfiles are regenerated, one
aggregated error is raised whose message lists each failed manifest
with a repair command.❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@lading/commands/bump_lockfiles.py`:
- Around line 166-193: The return value of _attempt_single_lockfile_update is a
position-based union tuple that hides intent at call sites. Replace the tuple in
this function with a small self-documenting result type, such as a dataclass
result object with lockfile and error fields, and update the success/failure
returns to populate that type consistently. Then adjust any callers that unpack
the current tuple so they read the named fields instead.
- Around line 166-193: The private helper _attempt_single_lockfile_update has an
overlong multi-line docstring; trim it to a single-line summary only. Keep the
function’s purpose concise at the top of the docstring and remove the extended
returns/details text, leaving the fuller explanation for public APIs instead.
In `@tests/unit/test_bump_lockfiles.py`:
- Around line 299-354: Both new lockfile regeneration tests duplicate the same
`a`/`b` Cargo manifest directory setup, so extract that shared workspace
preparation into a reusable `pytest` fixture. Move the repeated creation of the
`a`, `b`, and root `Cargo.toml` files into a fixture (in `conftest.py` or a
shared fixtures module), then have
`test_regenerate_lockfiles_attempts_all_manifests_after_failure` and
`test_regenerate_lockfiles_aggregates_multiple_failures` depend on it instead of
building the layout inline.
- Around line 317-326: The new bare asserts in the unit tests should include
explicit failure messages so failures are self-explanatory. Update the key
checks in the affected test cases in test_bump_lockfiles.py to use assert ... ,
message instead of bare assert, especially around the attempted-manifest list
and the error-message content checks. Keep the assertions themselves the same,
but add short messages that identify what condition failed for easier debugging.
🪄 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: 7b184364-9252-456f-ad1f-567fba849adb
📒 Files selected for processing (5)
.gitignoredocs/developers-guide.mdlading/commands/bump_lockfiles.pytests/unit/__snapshots__/test_bump_lockfiles.ambrtests/unit/test_bump_lockfiles.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)
|
@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_lockfiles.py Comment on lines +395 to +442 def test_regenerate_lockfiles_attempts_all_and_reports_each_failure_once(
spec: tuple[bool, list[tuple[str, bool]]],
) -> None:
"""Every manifest is attempted and each failure is reported exactly once."""
fail_root, crates = spec
with tempfile.TemporaryDirectory() as tmp:
root = Path(tmp)
(root / "Cargo.toml").write_text("", encoding="utf-8")
expected_order = [(root / "Cargo.toml").resolve()]
failing: set[Path] = set()
if fail_root:
failing.add((root / "Cargo.toml").resolve())
for name, should_fail in crates:
(root / name).mkdir()
(root / name / "Cargo.toml").write_text("", encoding="utf-8")
manifest = (root / name / "Cargo.toml").resolve()
expected_order.append(manifest)
if should_fail:
failing.add(manifest)
runner, attempted = _selective_failure_runner(failing)
configured = tuple(f"{name}/Cargo.toml" for name, _ in crates)
if failing:
with pytest.raises(bump_lockfiles.LockfileRegenerationError) as excinfo:
bump_lockfiles.regenerate_lockfiles(root, configured, runner=runner)
message = str(excinfo.value)
repair_lines = message.count("cargo update --workspace --manifest-path")
assert repair_lines == len(failing), (
f"expected one repair line per failure; got {repair_lines} for "
f"{len(failing)} failure(s): {message}"
)
for manifest in failing:
assert shlex.quote(str(manifest)) in message, (
f"failed manifest {manifest} should appear in: {message}"
)
else:
regenerated = bump_lockfiles.regenerate_lockfiles(
root, configured, runner=runner
)
assert len(regenerated) == len(expected_order), (
"all lockfiles should be regenerated when nothing fails"
)
assert attempted == expected_order, (
f"every manifest should be attempted once, in order; "
f"expected {expected_order}, got {attempted}"
)❌ New issue: Bumpy Road Ahead |
regenerate_lockfiles aborted on the first cargo failure, leaving the workspace inconsistent with no inventory of which lockfiles were refreshed: manifests carry the new version while later lockfiles were never attempted. Attempt every manifest, collect the failures, and raise one aggregated LockfileRegenerationError after the loop. The message states that the workspace is inconsistent and lists each failed manifest with its repair command (cargo update --workspace --manifest-path ...). The exception chains from the first underlying failure so diagnostics such as a missing cargo executable survive aggregation. Manifest-path validation errors still raise immediately, as those are configuration mistakes rather than partial-update hazards. Tests cover the mid-list failure (remaining manifests still attempted; successes excluded from the repair list), multi-failure aggregation with exactly one repair command per failed manifest, and a syrupy snapshot of the aggregated message. Closes #84
Reduce regenerate_lockfiles below the 70-line limit by moving the per-manifest iteration into a new private _attempt_lockfile_updates helper, placed immediately before _build_aggregate_failure_message. The helper owns its lockfiles and failures lists and returns them as a pair; regenerate_lockfiles now calls it in one line and keeps the aggregation and raise logic unchanged. Behaviour is identical: every manifest is still attempted and failures are aggregated into one LockfileRegenerationError (issue #84). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The memtrace agent tooling writes .memdb/daemon.pid and .memdb/daemon-state.json into the worktree. These are per-machine runtime artifacts and must not be tracked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the whole-loop _attempt_lockfile_updates helper with a per-manifest _attempt_single_lockfile_update, keeping the iteration loop in regenerate_lockfiles. The helper logs, runs the cargo update, and returns (lockfile, None) on success or (None, error) on failure, so the loop stays flat: one call per manifest, then branch on the result. The success branch narrows lockfile to Path via `elif lockfile is not None`, so no type suppression is needed (ty does not honour mypy's `# type: ignore[arg-type]`). Simplify the cause selection to `failures[0][1].__cause__ or failures[0][1]`. Observable behaviour of regenerate_lockfiles is unchanged: every manifest is attempted and failures aggregate into one LockfileRegenerationError (issue #84). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The aggregate failure message exists to disambiguate which of several regenerated lockfiles is now inconsistent and how to repair each. When only the workspace-root lockfile is regenerated there is nothing to disambiguate, so wrapping the lone cargo error in the aggregate header plus repair list adds noise without information. Re-raise the original LockfileRegenerationError unchanged when exactly one manifest was processed; keep the aggregate path for multiple manifests (issue #84 still attempts every manifest and lists each failure with its repair command). This restores the canonical single-line detail message that test_lockfile_regeneration_failure_message pins, and needs no snapshot change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a Mermaid flowchart of regenerate_lockfiles to the Bump command internals section, with a screen-reader description before it and an accessible Figure 1 caption after, per the documentation style guide. A following note records that a lone workspace-root failure re-raises the plain cargo error rather than the aggregate message. Normalize the one asterisk emphasis (_already-derived_) in the section to underscore so the document is MD049-consistent with the new caption. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeScene flagged two nested-conditional blocks in regenerate_lockfiles. Extract each into a private helper: - _collect_lockfile_results attempts every manifest and partitions the outcomes into (lockfiles, failures). - _raise_aggregated_failure raises the appropriate error — the plain cargo error for a lone workspace-root failure, or one aggregated error for several regenerated lockfiles. It is annotated typ.NoReturn since it is only reached when failures is non-empty. regenerate_lockfiles now reads as a flat sequence: collect, branch on failures, log, return. Behaviour is unchanged. Use the repo's aliased typing import (typ.NoReturn); `from typing import NoReturn` is banned by ruff ICN003. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Replace the position-based union tuple returned by _attempt_single_lockfile_update with a self-documenting _LockfileUpdateResult dataclass (lockfile/error fields); the caller now reads named fields. Trim the helper's docstring to a one-line summary. - Extract the duplicated a/b workspace setup in the two aggregate tests into an `ab_workspace` fixture, and give the key assertions explicit failure messages. - Add a Hypothesis property test over arbitrary manifest lists and failing subsets, asserting every manifest is attempted in order and each failure is reported exactly once. - Document the attempt-all-and-aggregate bump behaviour and the operator-facing repair list in docs/users-guide.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
03bf3d9 to
3c4d65a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Extract the two nested blocks of the Hypothesis property test into module-level helpers: - _prepare_manifest_fixture creates the manifest files and returns the (expected_order, failing) pair. - _assert_failure_message_reports_each_once checks each failing manifest has exactly one repair line. The test body now reads as a flat arrange/act/assert. `fail_root` is keyword-only on the helper to satisfy ruff FBT001 (no bool positional args). Decorators, docstring, and the other tests are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 167-205: Update the `regenerate_lockfiles` Mermaid flowchart to
include the single-manifest branch handled by `_raise_aggregated_failure`, since
it does not always go through `_build_aggregate_failure_message`. Add a separate
outcome from the failures path that shows the workspace-root-only case re-raises
the original cargo error unchanged, while keeping the existing
aggregated-failure branch for multi-manifest runs. Make sure the diagram in
`docs/developers-guide.md` matches the behavior described by
`_raise_aggregated_failure` and `_build_aggregate_failure_message`.
In `@docs/users-guide.md`:
- Around line 282-303: The new fenced example in the users guide uses the wrong
code-block language identifier; update the aggregated error-output example to
use plaintext instead of text. Locate the fenced block in the section describing
lading bump’s aggregated lockfile failures, and make it consistent with the
existing plaintext example elsewhere in the file so the markdown style guide is
followed.
In `@lading/commands/bump_lockfiles.py`:
- Around line 101-140: Trim the docstrings for _collect_lockfile_results and
_raise_aggregated_failure to single-line summaries only, matching the style
already used for _attempt_single_lockfile_update. Keep the brief purpose
statement in each helper, but remove the extended explanatory paragraphs so
these private functions follow the one-line docstring rule for internal helpers.
In `@tests/unit/test_bump_lockfiles.py`:
- Around line 276-327: The private helper `_prepare_manifest_fixture` has a
multi-line docstring that should be reduced to a single-line summary to match
the other private helpers in this test block. Update only the docstring in
`_prepare_manifest_fixture` so it stays brief and omits the separate Returns
section, keeping the style consistent with `_selective_failure_runner` and
`_assert_failure_message_reports_each_once`.
🪄 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: 8b1f4bfb-193d-466c-bd24-d4098bbc2c63
📒 Files selected for processing (6)
.gitignoredocs/developers-guide.mddocs/users-guide.mdlading/commands/bump_lockfiles.pytests/unit/__snapshots__/test_bump_lockfiles.ambrtests/unit/test_bump_lockfiles.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)
- developers-guide.md: extend the regenerate_lockfiles flowchart with the single-manifest branch (workspace-root-only failure re-raises the plain cargo error) alongside the multi-manifest aggregate branch, matching _raise_aggregated_failure; update the screen-reader description to match. - bump_lockfiles.py: trim _collect_lockfile_results and _raise_aggregated_failure docstrings to single-line summaries, matching the other private helpers. - test_bump_lockfiles.py: trim _prepare_manifest_fixture's docstring; add test_regenerate_lockfiles_aggregate_chains_first_underlying_cause (asserts the aggregated error chains from the first failure's real __cause__) and test_regenerate_lockfiles_single_manifest_reraises_unchanged (proves a lone-root failure surfaces the plain error, not the aggregate). The users-guide fenced-block finding was skipped: `text` is the file's dominant convention (3 blocks vs a single `plaintext` outlier), so the block already matches; switching to `plaintext` would reduce consistency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd test_bump_lockfiles.py
echo "---"
rg -n "cause|__cause__|lone|single.*root|root.*single|re-?raise" tests/unit/test_bump_lockfiles.py -C2
echo "---helper/aggregate function---"
rg -n "def _raise_aggregated_failure|def regenerate_lockfiles|def _collect_lockfile_results|def _attempt_single_lockfile_update|raise .* from" lading/commands/bump_lockfiles.py -A15Length of output: 2312 🏁 Script executed: #!/bin/bash
fd test_bump_lockfiles.py
echo "---"
rg -n "cause|__cause__|lone|single.*root|root.*single|re-?raise" tests/unit/test_bump_lockfiles.py -C2
echo "---helper/aggregate function---"
rg -n "def _raise_aggregated_failure|def regenerate_lockfiles|def _collect_lockfile_results|def _attempt_single_lockfile_update|raise .* from" lading/commands/bump_lockfiles.py -A15Length of output: 5606
Both match the implementation in (=^・ω・^=) ノ⌒●~* |
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes.
Mark this check resolved; nothing further outstanding on this item. (=^・ω・^=)ノ |
Summary
Closes #84
regenerate_lockfilespreviously aborted on the first cargo failure, leaving the workspace inconsistent (manifests carry the new version; later lockfiles never attempted) with no inventory of what needs repair.LockfileRegenerationErroris raised after the loop. The message states the workspace is inconsistent and lists each failed manifest with its repair command (cargo update --workspace --manifest-path …).regenerate_lockfilesdocstring documents the partial-update semantics.Testing
make check-fmt,make lint,make typecheck, andmake test(563 passed) all green after rebasing onto currentmain.coderabbit review --agent: 0 findings.🤖 Generated with Claude Code
Summary by Sourcery
Aggregate Cargo lockfile regeneration failures so all manifests are attempted and a single error reports required repairs.
Enhancements:
Tests:
References