Skip to content

Remove dead lockfile.refresh_lockfile; fix module docstring (#99) - #124

Merged
leynos merged 1 commit into
mainfrom
issue-99-remove-dead-refresh-lockfile
Jul 7, 2026
Merged

Remove dead lockfile.refresh_lockfile; fix module docstring (#99)#124
leynos merged 1 commit into
mainfrom
issue-99-remove-dead-refresh-lockfile

Conversation

@leynos

@leynos leynos commented Jun 10, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #99

  • Delete refresh_lockfile (ran cargo generate-lockfile, called only by its own unit tests) and its LockfileRefreshError class — the bump path regenerates lockfiles via bump_lockfiles.regenerate_lockfiles (cargo update --workspace).
  • Correct the lockfile.py module docstring, which described a call graph that does not exist (claimed bump calls discover_tracked_lockfiles + refresh_lockfile). It now documents the real call graph: publish uses discover_tracked_lockfiles + validate_lockfile_freshness; bump uses bump_lockfiles.regenerate_lockfiles.
  • One-line rationale recorded for the two cargo strategies: bump refreshes pinned versions in place after manifest rewrites; publish probes freshness read-only under --locked.
  • docs/developers-guide.md and the exceptions catalogue updated to match.

Rebase onto main

Since this branch was opened, main grew a metrics surface around the (still dead) refresh_lockfile via issue #91 / #134. Completing the deletion on top of current main therefore also removes that now-orphaned surface:

  • Drop the lockfile.refresh / lockfile.refresh.duration metric constants and the unused with_detail import from lockfile.py; lockfile.discovered and lockfile.validate (and their durations) remain.
  • Remove the refresh_lockfile metrics tests and the test_lockfile_refresh_failure_message snapshot test (plus its .ambr entry), and delete the two lockfile.refresh* rows from the developers-guide metrics table.
  • Repair a rebase-weave artifact that duplicated the "Workspace path normalization" section in docs/developers-guide.md.

Testing

  • The orphaned refresh_lockfile tests are removed with the function.
  • make check-fmt, make lint, make typecheck, and make test (676 passed, 60 snapshots) all green after rebasing onto current main; make markdownlint and make nixie also green.

🤖 Generated with Claude Code

Summary by Sourcery

Remove unused lockfile regeneration helper and align documentation with the current lockfile management workflow.

Enhancements:

  • Remove the obsolete refresh_lockfile helper and its associated LockfileRefreshError exception from the lockfile command module.

Documentation:

  • Update the lockfile module docstring and developer guide to describe the current bump and publish lockfile workflows and cargo strategies.
  • Adjust exception catalogue and guidance to reference LockfileRegenerationError instead of the removed LockfileRefreshError.

Tests:

  • Delete unit tests that covered the removed refresh_lockfile helper.

References

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0174686d-9b1e-4de6-bf5c-86385b295ba5

📥 Commits

Reviewing files that changed from the base of the PR and between 6bd4bd5 and a6022c5.

📒 Files selected for processing (6)
  • docs/developers-guide.md
  • lading/commands/lockfile.py
  • lading/exceptions.py
  • tests/unit/__snapshots__/test_command_failure_messages.ambr
  • tests/unit/test_command_failure_messages.py
  • tests/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)
💤 Files with no reviewable changes (2)
  • tests/unit/snapshots/test_command_failure_messages.ambr
  • tests/unit/test_lockfile.py

Removed the dead lockfile.refresh_lockfile path and its LockfileRefreshError, along with the related tests, snapshot, and refresh metrics.

Updated lading/commands/lockfile.py to match the real workflow:

  • publish discovers tracked lockfiles and checks freshness with --locked
  • bump regenerates lockfiles via bump_lockfiles.regenerate_lockfiles

Also clarified the rationale for the two Cargo strategies, noting that bump refreshes pinned versions in place after manifest rewrites, while publish performs read-only freshness checks.

Documentation was brought in line with the implementation in docs/developers-guide.md and lading/exceptions.py, including canonical lockfile ownership, the regeneration vs validation split, and the updated exception naming. A duplicated docs section introduced during the rebase was removed.

Walkthrough

This PR removes the unused refresh_lockfile helper and its metric constants from lading/commands/lockfile.py, updates the module docstring to reflect the actual bump/publish call graph, renames exception references from LockfileRefreshError to LockfileRegenerationError, and deletes the corresponding tests, snapshots, and documentation entries.

Changes

Lockfile regeneration removal

Layer / File(s) Summary
Remove refresh_lockfile helper and metrics
lading/commands/lockfile.py
Deletes the refresh_lockfile function and REFRESH_METRIC/REFRESH_DURATION_METRIC constants, switches process imports to append_detail/command_detail, and rewrites the module docstring to describe discovery, freshness validation, and bump-side regeneration via bump_lockfiles.regenerate_lockfiles.
Exception docstring update
lading/exceptions.py
Updates the docstring example to reference LockfileRegenerationError instead of LockfileRefreshError.
Remove refresh-related tests and snapshots
tests/unit/test_lockfile.py, tests/unit/test_command_failure_messages.py, tests/unit/__snapshots__/test_command_failure_messages.ambr
Removes success/error/metric tests for refresh_lockfile, the corresponding snapshot fixture, and the unused lockfile import.
Developers guide documentation updates
docs/developers-guide.md
Documents the cargo update --workspace bump flow, corrects the exception hierarchy table and usage guidance, and swaps the lockfile.refresh* metrics for lockfile.discovered.

Possibly related PRs

  • leynos/lading#85: Introduces the LockfileRegenerationError/regeneration flow that this PR completes by removing the older refresh_lockfile/LockfileRefreshError path.
  • leynos/lading#115: Touches the same lockfile.py error-detail formatting (with_detail/append_detail) and refresh_lockfile pathway that this PR removes entirely.

Suggested labels: Issue

Suggested reviewers: codescene-delta-analysis

Poem

A rabbit hopped through lockfile lore,
Found refresh_lockfile — dead code, no more!
Snipped the stale metrics, tidied the docs,
regenerate_lockfiles now runs the block. 🥕
Hop on, reviewer, the guide's aligned once more.

🚥 Pre-merge checks | ✅ 20
✅ Passed checks (20 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the PR’s main change and includes the linked issue reference (#99).
Description check ✅ Passed The description clearly describes the lockfile removal, doc updates, and test cleanup.
Linked Issues check ✅ Passed The PR deletes the dead helper, fixes the docstring, adds the strategy rationale, and updates the developers' guide as required.
Out of Scope Changes check ✅ Passed No clearly unrelated changes stand out beyond the documented doc, metrics, and test cleanup tied to the lockfile refactor.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Testing (Overall) ✅ Passed PASS: the bump and publish paths are covered by real CLI and unit tests that assert command wiring, outputs, and failure text; the removed helper was dead code.
User-Facing Documentation ✅ Passed PASS: docs/users-guide.md already documents bump lockfile refresh and publish freshness checks; this PR only removes dead internals, so no new user-facing behaviour needs surfacing.
Developer Documentation ✅ Passed docs/developers-guide.md now covers lockfile ownership, Cargo strategy rationale, exception hierarchy, and metrics; no execplan artefact or unchecked roadmap item was found.
Module-Level Documentation ✅ Passed Confirm all 95 Python modules carry top-level docstrings, including the touched lockfile, exceptions and test modules.
Testing (Unit And Behavioural) ✅ Passed Keep the unit tests on discovery/validation edge cases and the CLI/BDD/E2E coverage at the public bump/publish boundaries, including stale-lockfile handling.
Testing (Property / Proof) ✅ Passed PASS: This PR deletes dead refresh code and updates docs/tests only; it introduces no new range-dependent invariant or proof assumption. Existing Hypothesis coverage already handles lockfile path-f...
Testing (Compile-Time / Ui) ✅ Passed Treat this as non-applicable: the PR only removes dead Python helpers/docs, and the remaining failure-message snapshot stays the right fit for text output.
Unit Architecture ✅ Passed PASS: refresh_lockfile was deleted; bump owns injectable regeneration, publish keeps read-only freshness checks, and tests/docs match the split.
Domain Architecture ✅ Passed PASS: keep the lockfile work in the command layer; it removes dead infrastructure code and leaves domain code free of transport, storage, or CLI concerns.
Observability ✅ Passed The PR deletes dead refresh code and its orphaned metrics/tests; live lockfile discovery, bump regeneration, and freshness validation still emit logs and bounded metrics.
Security And Privacy ✅ Passed PASS: the patch only deletes dead lockfile refresh code and updates docs/tests; it adds no secrets, auth changes, permissions, or unsafe input sinks.
Performance And Resource Use ✅ Passed PASS: Remove-only change; no new loops or repeated I/O. Runtime paths stay bounded, and failure-detail formatting now derives command detail once.
Concurrency And State ✅ Passed No new shared mutable state, locks, async tasks, or ordering guarantees were introduced; the patch only deletes dead lockfile code and updates docs/tests.
Architectural Complexity And Maintainability ✅ Passed PASS: The patch deletes dead lockfile regeneration code, keeps explicit bump/publish call sites, and adds no new layers, registries, or hidden orchestration.
Rust Compiler Lint Integrity ✅ Passed No Rust source changed, and the patch removes stale helpers/tests without adding lint suppressions, artificial references, or suspicious clones.
📋 Issue Planner

Built with CodeRabbit's Coding Plans for faster development and fewer bugs.

View plan used: #99

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-99-remove-dead-refresh-lockfile

Comment @coderabbitai help to get the list of available commands.

@sourcery-ai

sourcery-ai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Reviewer's Guide

Removes the unused lockfile regeneration helper and its exception type in favor of the existing bump-side regeneration path, and updates documentation and exception references to describe the real publish/bump lockfile flows and cargo strategies.

Flow diagram for updated publish vs bump lockfile handling

flowchart LR
    subgraph Publish_flow
        P[lading_publish]
        D[discover_tracked_lockfiles]
        V[validate_lockfile_freshness]
        C1[cargo metadata --locked]

        P --> D
        D --> V
        V --> C1
    end

    subgraph Bump_flow
        B[lading_bump]
        R[regenerate_lockfiles]
        C2[cargo update --workspace]

        B --> R
        R --> C2
    end
Loading

File-Level Changes

Change Details Files
Remove the unused lockfile refresh helper and its dedicated exception from the lockfile command module and unit tests.
  • Update the lockfile module docstring to describe only discovery and freshness validation responsibilities and the actual publish/bump call graph.
  • Delete the LockfileRefreshError exception class from the lockfile module.
  • Delete the refresh_lockfile helper implementation that wrapped cargo generate-lockfile.
  • Remove unit tests that exercised refresh_lockfile success and failure behaviour.
lading/commands/lockfile.py
tests/unit/test_lockfile.py
Align developer documentation and exception catalogue with the new lockfile responsibilities and exception types.
  • Document that lockfile regeneration for bump is handled by lading.commands.bump_lockfiles.regenerate_lockfiles using cargo update --workspace, contrasting it with publish’s read-only cargo metadata --locked probe.
  • Remove references to refresh_lockfile and LockfileRefreshError from the developers guide, replacing them with LockfileRegenerationError where appropriate.
  • Update the exceptions table to remove LockfileRefreshError and ensure LockfileRegenerationError is listed as the bump-side regeneration failure type.
  • Adjust narrative guidance on catching narrow LadingError subclasses to reference LockfileRegenerationError instead of LockfileRefreshError.
  • Update the top-level exceptions module docstring to reference LockfileRegenerationError instead of LockfileRefreshError.
docs/developers-guide.md
lading/exceptions.py

Assessment against linked issues

Issue Objective Addressed Explanation
#99 Ensure refresh_lockfile is no longer dead code by either wiring it into a real call path or deleting it (including its dedicated tests and error type).
#99 Update lockfile.py’s module docstring so it accurately describes the actual call graph and includes a one-line rationale for the two Cargo strategies (generate-lockfile vs update --workspace).
#99 Update docs/developers-guide.md (and related exception documentation) to clearly document canonical lockfile ownership and which workflows use which regeneration/freshness functions.

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

refresh_lockfile ran cargo generate-lockfile but was called only by
its own unit tests: the bump path regenerates lockfiles via
bump_lockfiles.regenerate_lockfiles (cargo update --workspace). The
lockfile.py module docstring also described a call graph that does not
exist, claiming lading bump calls discover_tracked_lockfiles followed
by refresh_lockfile.

Delete refresh_lockfile, its LockfileRefreshError class, and their
tests. Correct the module docstring to describe the real call graph
and explain why the two cargo strategies differ: bump refreshes pinned
versions in place after manifest rewrites, while publish only probes
freshness read-only under --locked. Update the developers' guide and
the exceptions catalogue to match.

Closes #99
@lodyai
lodyai Bot force-pushed the issue-99-remove-dead-refresh-lockfile branch from b15004a to a6022c5 Compare July 7, 2026 17:46
@leynos
leynos marked this pull request as ready for review July 7, 2026 18:48

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @leynos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@coderabbitai coderabbitai Bot added the Issue label Jul 7, 2026
@leynos
leynos merged commit bf3cfc2 into main Jul 7, 2026
9 checks passed
@leynos
leynos deleted the issue-99-remove-dead-refresh-lockfile branch July 7, 2026 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove or wire up dead lockfile.refresh_lockfile; fix inaccurate module docstring

1 participant