From cb549e9714ca4325f9aa58a6bd9fa9e7bc2d5fbd Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Tue, 7 Jul 2026 23:42:16 +0100 Subject: [PATCH 01/16] Add ExecPlan for discovery-driven bump lockfile regeneration `lading bump` regenerates only the root lockfile plus manifests listed in `bump.lockfile_manifests`, while `lading publish` pre-flight validates every git-tracked `Cargo.lock`, so unconfigured nested lockfiles go stale and publish fails with a manual repair message. Plan the fix: reuse `discover_tracked_lockfiles` in bump, merging discovered manifests with the configured list. Record the Stage A prototype evidence that `cargo update --workspace` refreshes path-dependency entries in nested fixture lockfiles. --- docs/execplans/regenerate-lockfiles.md | 475 +++++++++++++++++++++++++ 1 file changed, 475 insertions(+) create mode 100644 docs/execplans/regenerate-lockfiles.md diff --git a/docs/execplans/regenerate-lockfiles.md b/docs/execplans/regenerate-lockfiles.md new file mode 100644 index 00000000..227b5adb --- /dev/null +++ b/docs/execplans/regenerate-lockfiles.md @@ -0,0 +1,475 @@ +# Regenerate discovered nested lockfiles during `lading bump` + +This ExecPlan (execution plan) is a living document. The sections +`Constraints`, `Tolerances`, `Risks`, `Progress`, `Surprises & Discoveries`, +`Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work +proceeds. + +Status: IN PROGRESS + +## Purpose / big picture + +`lading bump` rewrites crate versions across a Cargo workspace and then +regenerates lockfiles. Today it only regenerates the workspace root +`Cargo.lock` plus lockfiles whose manifests are explicitly listed in the +`bump.lockfile_manifests` configuration array. `lading publish`, however, +validates *every* git-tracked `Cargo.lock` in the repository (excluding files +under `target/` directories) and aborts when any of them is stale. The result +is a frustrating failure mode observed in real use: a user runs `lading bump`, +then `lading publish` fails with: + +```plaintext +Tracked Cargo.lock files are stale after manifest version changes. +This commonly happens after running `lading bump`; repair each stale lockfile +directly: +- /crates/cargo-bdd/tests/fixtures/minimal/Cargo.lock + cargo generate-lockfile --manifest-path /crates/cargo-bdd/tests/fixtures/minimal/Cargo.toml +- /crates/rstest-bdd/tests/ui_lints/Cargo.lock + cargo generate-lockfile --manifest-path /crates/rstest-bdd/tests/ui_lints/Cargo.toml +``` + +The user is left wondering why bump could not do this itself. Worse, the +users' guide (`docs/users-guide.md`, "After updating manifest versions" in the +bump section) already claims that bump "automatically refreshes any +git-tracked `Cargo.lock` files (excluding those under `target/`) that have an +adjacent `Cargo.toml`" — a promise the code does not keep. + +After this change, `lading bump` discovers every git-tracked `Cargo.lock` +using the same discovery helper the publish pre-flight uses +(`lading.commands.lockfile.discover_tracked_lockfiles`), merges the discovered +manifests with the configured `bump.lockfile_manifests` list, and regenerates +all of them. A subsequent `lading publish` in the same repository then passes +its lockfile freshness check without manual repair. Success is observable by +running `lading bump ` in a workspace containing a tracked nested +fixture lockfile and seeing that lockfile listed in the bump output with a +`(lockfile)` suffix and refreshed on disk. + +## Constraints + +- Do not change the `CommandRunner` protocol + (`lading/runtime.py`) or the signatures of + `lading.commands.lockfile.discover_tracked_lockfiles` and + `lading.commands.lockfile.validate_lockfile_freshness`. +- The existing `bump.lockfile_manifests` and `bump.rebuild_lockfiles` + configuration keys must keep working with their current meanings. + `--no-rebuild-lockfiles` must continue to skip all regeneration. +- Regeneration order must keep the workspace root `Cargo.lock` first so + existing output snapshots and user expectations hold. +- Workspaces that are not git repositories must keep working: discovery + degrades to a warning plus the configured list, exactly as the publish + pre-flight degrades today. +- All code comments and documentation use en-GB-oxendict spelling. +- No new external dependencies. +- No single code file may exceed 400 lines (repository rule); adding to + `lading/commands/bump_lockfiles.py` (155 lines) and + `lading/commands/bump.py` must respect this. + +## Tolerances (exception triggers) + +- Scope: if the implementation (excluding tests, snapshots, and docs) requires + changes to more than 5 files or more than 250 net lines, stop and escalate. +- Interface: if `regenerate_lockfiles` or `resolve_lockfile_paths` must change + their public signatures in a way that breaks existing callers, stop and + escalate. (Adding a new public function to `bump_lockfiles` is within + tolerance.) +- Behaviour: if the Stage A prototype shows that + `cargo update --workspace --manifest-path ` does *not* freshen a + nested fixture lockfile that `cargo metadata --locked` reports as stale, + stop and escalate with the evidence before choosing a different cargo + command (candidate fallback: `cargo generate-lockfile`, which is what the + publish pre-flight already recommends to users). +- Iterations: if the focused test suite still fails after 3 fix attempts on + any milestone, stop and escalate. +- Ambiguity: if any repository under test relies on tracked lockfiles that + must deliberately stay stale (none are known; the publish pre-flight already + hard-fails on them), stop and escalate rather than adding an exclusion + mechanism unilaterally. + +## Risks + +- Risk: `cargo update --workspace` on a nested manifest may not refresh the + recorded version of a *path* dependency on a bumped workspace crate, because + `--workspace` restricts updates to packages defined in that (nested) + workspace. + Severity: high. Likelihood: low. + Mitigation: Stage A is a throwaway prototype that reproduces the reported + layout (root workspace crate + nested fixture package with a path + dependency on it) and proves which cargo command restores freshness under + `cargo metadata --locked`. The command choice is decided by evidence before + any production code changes. +- Risk: newly discovered nested manifests may fail under cargo (for example, + deliberately broken lint fixtures), making bump fail where it previously + succeeded. + Severity: medium. Likelihood: low. + Mitigation: any tracked lockfile that cargo cannot read already fails the + publish pre-flight today, so such repositories are already broken for + lading's workflow. The failure message from + `LockfileRegenerationError` names the manifest and exit code; + `--no-rebuild-lockfiles` remains as an escape hatch. Documented in the + users' guide update. +- Risk: dry-run currently makes no subprocess calls in `_process_lockfiles`; + after this change it runs `git ls-files` (read-only) to list what would be + regenerated. + Severity: low. Likelihood: certain. + Mitigation: `git ls-files` mutates nothing. Tests pass a stub runner, so no + real subprocess runs in the unit suite. The behaviour is documented. +- Risk: bump output snapshots (`syrupy` `.ambr` files) change because more + lockfiles are listed. + Severity: low. Likelihood: high. + Mitigation: regenerate snapshots deliberately with `--snapshot-update` only + after eyeballing the new output, and commit them with the code change. + +## Progress + +- [x] (2026-07-07 00:00Z) Investigated current behaviour: bump regenerates + root + configured manifests only; publish pre-flight discovers all tracked + lockfiles; users' guide already promises discovery-based refresh. +- [x] (2026-07-07 12:20Z) Stage A: prototype proved + `cargo update --workspace --manifest-path ` restores freshness for + a nested fixture package with a path dependency on a bumped workspace + crate. See `Artifacts and notes`. No fallback command needed. +- [ ] Stage B: red tests for discovery-driven regeneration (unit + BDD). +- [ ] Stage C: implementation (discovery helper in `bump_lockfiles`, wiring in + `bump._process_lockfiles`). +- [ ] Stage D: documentation, pre-flight message wording, snapshot updates, + full quality gates. + +## Surprises & discoveries + +- Observation: `docs/users-guide.md` already documents the desired behaviour + ("automatically refreshes any git-tracked `Cargo.lock` files") even though + the code only regenerates configured manifests. + Evidence: `docs/users-guide.md` bump section versus + `lading/commands/bump.py::_process_lockfiles`, which reads only + `context.configuration.bump.lockfile_manifests`. + Impact: this change closes a documented behaviour gap rather than adding new + surface; the `lockfile_manifests` doc text must be re-scoped to "manifests + discovery cannot see" (for example, lockfiles not tracked by git). + +## Decision log + +- Decision: make discovery always-on for bump rather than adding a + `bump.discover_lockfiles` toggle. + Rationale: the publish pre-flight already treats every tracked lockfile as + required-fresh, so there is no coherent use case for bumping versions while + leaving a tracked lockfile stale. `rebuild_lockfiles = false` and + `--no-rebuild-lockfiles` already exist as global escape hatches. Fewer + configuration keys, and bump/publish stay consistent. + Date/Author: 2026-07-07, planning session. +- Decision: keep `bump.lockfile_manifests` rather than deprecating it. + Rationale: it remains the only way to regenerate a lockfile that git does + not track (for example, generated fixtures ignored by `.gitignore`), and + removing a configuration key is a breaking change out of scope here. + Date/Author: 2026-07-07, planning session. +- Decision: perform the union of configured and discovered manifests inside + `lading/commands/bump_lockfiles.py` via a new public helper, and keep + `bump._process_lockfiles` a thin caller. + Rationale: `bump_lockfiles` already owns manifest validation and + de-duplication (`_resolve_manifest_paths`); colocating discovery keeps the + feature testable without driving the whole bump command. + Date/Author: 2026-07-07, planning session. + +## Outcomes & retrospective + +To be completed as milestones land. + +## Context and orientation + +lading is a Python (3.13, `uv`-managed) command-line tool that automates +version bumps and crates.io publication for Rust Cargo workspaces. The +relevant pieces: + +- `lading/commands/bump.py` orchestrates `lading bump`. After rewriting + manifest versions it calls `_process_lockfiles(context, changed_manifests)`, + which returns the tuple of lockfile paths that were (or, in dry-run, would + be) regenerated. It currently passes only + `context.configuration.bump.lockfile_manifests` (a tuple of + workspace-relative `Cargo.toml` path strings) to the helpers below. + `context.base_options.command_runner` is an optional `CommandRunner` — a + callable taking a command sequence and returning + `(exit_code, stdout, stderr)` — used for dependency injection in tests; + `None` means "use the real subprocess runner". +- `lading/commands/bump_lockfiles.py` owns regeneration. + `resolve_lockfile_paths(workspace_root, lockfile_manifests)` maps configured + manifests to lockfile paths for dry-run reporting. + `regenerate_lockfiles(workspace_root, lockfile_manifests, *, runner=None)` + validates each manifest path (must stay inside the workspace and be named + `Cargo.toml`), always prepends the workspace root manifest, de-duplicates, + and runs `cargo update --workspace --manifest-path ` per entry. +- `lading/commands/lockfile.py` owns discovery and freshness validation, + currently used only by the publish pre-flight. + `discover_tracked_lockfiles(workspace_root, runner, *, manifest_exists=...)` + runs `git ls-files "**/Cargo.lock" "Cargo.lock"`, filters out paths with a + `target` component, keeps only lockfiles with an adjacent `Cargo.toml`, and + returns absolute lockfile paths. In a non-git directory it logs a warning + and returns an empty tuple. +- `lading/commands/publish_preflight.py` contains + `_validate_lockfile_freshness`, which discovers tracked lockfiles, probes + each with `cargo metadata --locked`, and raises `PublishPreflightError` with + the "Tracked Cargo.lock files are stale after manifest version changes" + message quoted above. +- `lading/config.py` defines `BumpConfig` with `exclude`, + `lockfile_manifests`, `rebuild_lockfiles`, and `documentation` fields. No + configuration change is needed for this plan. +- Tests live under `tests/unit/` (pytest, with `syrupy` snapshots under + `tests/unit/__snapshots__/`) and `tests/bdd/` (pytest-bdd; feature files in + `tests/bdd/features/`, notably `cli.feature` scenarios "Bump rebuilds + tracked lockfiles" and "Publish pre-flight aborts when lockfile is stale"). + `tests/helpers/workspace_builders.py` provides `_make_workspace` and + `_make_config` used by `tests/unit/test_bump_lockfile_rebuild.py`. +- Quality gates are Makefile targets, run sequentially (never in parallel): + `make test`, `make lint`, `make check-fmt` (`make fmt` to fix), + `make typecheck`, and for Markdown `make markdownlint` and `make nixie`. + +"Stale" throughout means: `cargo metadata --locked` fails for the adjacent +manifest with cargo's "needs to be updated" / "cannot update the lock file" +wording, i.e. the lockfile no longer matches the manifests it locks. + +## Plan of work + +Stage A (prototyping, no production code): prove the cargo command. In the +scratchpad directory, build a miniature repository imitating the failure +report: a git-initialized Cargo workspace with one member crate `alpha` +(version 0.1.0), plus a nested, tracked, non-member package at +`fixtures/minimal/` whose `Cargo.toml` declares +`alpha = { path = "../..", version = "0.1.0" }` (adjust the path to point at +the `alpha` crate) and which has a committed `Cargo.lock`. Then simulate a +bump by editing versions to 0.2.0 (both `alpha`'s version and the fixture's +version requirement), confirm `cargo metadata --locked --manifest-path +fixtures/minimal/Cargo.toml` fails as stale, run `cargo update --workspace +--manifest-path fixtures/minimal/Cargo.toml`, and confirm the freshness probe +now passes. If it does not, try `cargo generate-lockfile` and record the +outcome in `Decision Log` before proceeding (see Tolerances). The prototype is +throwaway; its findings are recorded here, not committed. + +Stage B (red): specify the behaviour with failing tests. + +1. In `tests/unit/test_bump_lockfiles.py`, add tests for a new function + `lading.commands.bump_lockfiles.merge_discovered_manifests` (final name at + implementer's discretion, recorded in `Decision Log` if it differs): + given a workspace root, a configured manifest tuple, and a stub runner + whose `git ls-files` output lists nested lockfiles, it returns the + configured entries followed by workspace-relative POSIX manifest strings + for each discovered lockfile, without duplicates (a manifest both + configured and discovered appears once, in its configured position). A + second test drives the non-git fallback: the stub runner returns exit code + 128 with "not a git repository" on stderr and the function returns just + the configured tuple. Mark these `@pytest.mark.xfail(strict=True, + reason="discovery merge not implemented")` until red is observed, then + remove the marker during Stage C. +2. In `tests/unit/test_bump_lockfile_rebuild.py`, extend the existing + monkeypatch-style tests: patch discovery (not the real git) so that + `bump.run` passes the union of configured and discovered manifests to + `regenerate_lockfiles`, and assert the dry-run path lists discovered + lockfile paths in its output. Existing assertions such as + `"lockfile_manifests": ()` will need the new expected union values. +3. In `tests/bdd/features/cli.feature`, extend the "Bump rebuilds tracked + lockfiles" scenario (or add a sibling scenario "Bump refreshes discovered + nested lockfiles") so the workspace contains a tracked nested lockfile + that is *not* configured in `lading.toml`, and assert the CLI output lists + it with the `(lockfile)` suffix and that it was refreshed. Reuse the + existing step vocabulary in `tests/bdd/steps/` where possible; add a Given + step for the nested fixture only if none fits. + +Run the focused tests and confirm each fails for the expected reason before +Stage C. + +Stage C (green): implement the minimal change. + +1. In `lading/commands/bump_lockfiles.py`, add the discovery merge function. + It imports `discover_tracked_lockfiles` from `lading.commands.lockfile`, + defaults its runner to `lading.runtime.subprocess_runner` when `None` + (matching `regenerate_lockfiles`), converts each discovered lockfile path + to `(lockfile.parent / "Cargo.toml").relative_to(workspace_root)` rendered + as a POSIX string, sorts the discovered entries for determinism, and + returns configured entries first followed by unseen discovered entries. + The existing `_resolve_manifest_paths` continues to prepend the root + manifest and de-duplicate resolved paths, so the root lockfile stays + first regardless of what discovery returns. +2. In `lading/commands/bump.py::_process_lockfiles`, call the merge function + once (passing `context.base_options.command_runner`), and feed its result + to both the dry-run `resolve_lockfile_paths` branch and the live + `regenerate_lockfiles` branch, so dry-run and live runs report the same + set. +3. Update module docstrings that describe the old contract: the header of + `bump_lockfiles.py` ("any configured nested lockfiles") and the call-graph + paragraph in `lockfile.py` (which currently says discovery is publish-only). + +Stage D (refactor, documentation, cleanup): + +1. Regenerate affected `syrupy` snapshots deliberately and review the diffs. +2. `docs/users-guide.md`: the bump section's discovery claim becomes true — + tighten it to mention that configured `lockfile_manifests` are additionally + regenerated; re-scope the `lockfile_manifests` key description to + "manifests whose lockfiles git does not track (discovery finds tracked + ones automatically)"; extend the "Lockfile regeneration runs…" paragraph to + say the command runs for the workspace root, each configured manifest, and + each discovered tracked lockfile's manifest. +3. `lading/commands/publish_preflight.py::_build_stale_lockfile_message`: + soften "This commonly happens after running `lading bump`" to reflect that + bump now repairs tracked lockfiles itself — for example, "This can happen + after manifest edits outside `lading bump`, or after running bump with + `--no-rebuild-lockfiles`; repair each stale lockfile directly:". Update the + snapshot `tests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambr`, + the assertion in `tests/bdd/features/cli.feature` ("Publish pre-flight + aborts when lockfile is stale"), and the quoted message in + `docs/users-guide.md` to match. +4. Update `docs/developers-guide.md` if it documents the bump lockfile flow + (search for "lockfile" there before editing). +5. Run the full gate suite sequentially and commit. + +Commit after each stage that leaves the tree green (AGENTS.md requires small, +gated commits): Stage B red tests are committed together with Stage C so the +suite never lands red; Stage D lands as one or two focused commits (behaviour +message + docs may be separate). + +## Concrete steps + +All commands run from the repository root +(`/data/leynos/Projects/lading.worktrees/regenerate-lockfiles`), except the +Stage A prototype, which runs in the session scratchpad directory. + +Stage A prototype sketch (scratchpad): + +```bash +mkdir proto && cd proto && git init -q +cargo new --lib alpha +# Write a workspace Cargo.toml with members = ["alpha"], version 0.1.0 in alpha +mkdir -p fixtures/minimal +# Write fixtures/minimal/Cargo.toml: [package] name = "minimal" ... +# [dependencies] alpha = { path = "../../alpha", version = "0.1.0" } +cargo generate-lockfile --manifest-path fixtures/minimal/Cargo.toml +git add -A && git commit -qm seed +# Simulate the bump: +sed -i 's/0\.1\.0/0.2.0/' alpha/Cargo.toml fixtures/minimal/Cargo.toml +cargo metadata --locked --manifest-path fixtures/minimal/Cargo.toml \ + --format-version=1 >/dev/null; echo "stale probe exit: $?" # expect non-zero +cargo update --workspace --manifest-path fixtures/minimal/Cargo.toml +cargo metadata --locked --manifest-path fixtures/minimal/Cargo.toml \ + --format-version=1 >/dev/null; echo "fresh probe exit: $?" # expect 0 +``` + +Focused test runs during Stages B and C (adjust node IDs to the tests +actually written): + +```bash +uv run pytest tests/unit/test_bump_lockfiles.py -q +uv run pytest tests/unit/test_bump_lockfile_rebuild.py -q +uv run pytest tests/bdd -q -k lockfile +``` + +Expected red transcript shape for Stage B (strict xfail proves the intended +failure): the new unit tests report `XFAIL`, and the extended existing tests +fail on the changed expectations (for example, an assertion diff showing +`lockfile_manifests` missing the discovered entry). + +Snapshot regeneration when output changes (Stage D, after reviewing why): + +```bash +uv run pytest tests/unit -q --snapshot-update +git diff -- '*.ambr' # review before staging +``` + +Full gates before each commit, sequentially: + +```bash +make test +make lint +make check-fmt +make typecheck +make markdownlint +make nixie +``` + +## Validation and acceptance + +Acceptance behaviour: in a git-tracked Cargo workspace containing a nested, +tracked, non-member fixture package with its own `Cargo.lock` and *no* +`bump.lockfile_manifests` configuration, running `lading bump 0.2.0` lists the +nested lockfile in the output with a `(lockfile)` suffix and leaves +`cargo metadata --locked` passing for the nested manifest; `lading bump 0.2.0 +--dry-run` lists the same lockfile without modifying it; and +`lading publish` (in that repository, after a real bump) no longer fails its +lockfile freshness pre-flight for lockfiles that bump could regenerate. + +Red-Green-Refactor evidence to record here as work proceeds: + +- Red: the Stage B commands above fail — new unit tests as strict `xfail`, + extended tests with assertion diffs naming the missing discovered manifest. +- Green: after Stage C, the same commands pass with the xfail markers + removed; `uv run pytest tests/unit tests/bdd -q` reports no failures. +- Refactor: after Stage D docstring/doc/message changes, the full gate suite + passes: `make test`, `make lint`, `make check-fmt`, `make typecheck`, + `make markdownlint`, `make nixie` all exit 0. + +Quality criteria: all six gates pass; the BDD scenario for discovered nested +lockfiles passes; no snapshot changes land unreviewed; users' guide text +matches actual CLI messages verbatim where it quotes them (there is a +`tests/unit/test_users_guide.py` that may enforce parts of this — keep it +green). + +## Idempotence and recovery + +Every step is re-runnable. Discovery (`git ls-files`) and the freshness probe +(`cargo metadata --locked`) are read-only. `cargo update --workspace` is +idempotent: rerunning it on a fresh lockfile is a no-op. Regeneration is not +atomic across manifests (documented in `regenerate_lockfiles`): if cargo fails +partway, earlier lockfiles stay updated; the recovery path is to fix the +cargo error and rerun `lading bump`, which converges. The Stage A prototype +lives entirely in the scratchpad and is deleted afterwards. If a milestone +must be abandoned mid-way, `git status` plus `git checkout -- ` restores +the tree; committed milestones are individually revertable. + +## Artifacts and notes + +Stage A prototype transcript (2026-07-07, scratchpad `proto/`): a +git-initialized workspace with member crate `alpha` 0.1.0 and a standalone +fixture package `fixtures/minimal` (empty `[workspace]` table, path dependency +`alpha = { path = "../../alpha", version = "0.1.0" }`, committed +`Cargo.lock`). After editing both versions to 0.2.0: + +```plaintext +$ cargo metadata --locked --manifest-path fixtures/minimal/Cargo.toml ... +stale probe exit: 101 +error: cannot update the lock file .../fixtures/minimal/Cargo.lock because +--locked was passed to prevent this +$ cargo update --workspace --manifest-path fixtures/minimal/Cargo.toml + Updating alpha v0.1.0 (...) -> v0.2.0 + Updating minimal v0.1.0 (...) -> v0.2.0 +$ cargo metadata --locked --manifest-path fixtures/minimal/Cargo.toml ... +fresh probe exit: 0 +``` + +Conclusion: `--workspace` does refresh path-dependency entries whose manifest +versions changed (the update is "necessary to satisfy other dependency +requirements"), so bump can reuse its existing cargo invocation unchanged for +discovered manifests. One incidental finding: a standalone nested package +needs an empty `[workspace]` table to avoid being claimed by the outer +workspace — real fixture packages with their own lockfiles already have this. + +## Interfaces and dependencies + +No new dependencies. At the end of Stage C the following must exist: + +In `lading/commands/bump_lockfiles.py`: + +```python +def merge_discovered_manifests( + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], + *, + runner: CommandRunner | None = None, +) -> tuple[str, ...]: + """Return configured manifests plus discovered tracked-lockfile manifests. + + Configured entries keep their order and come first; manifests implied by + git-tracked ``Cargo.lock`` files (via + :func:`lading.commands.lockfile.discover_tracked_lockfiles`) follow in + sorted order, skipping any already configured. In a non-git workspace the + configured tuple is returned unchanged. + """ +``` + +`lading/commands/bump.py::_process_lockfiles` calls it once and passes the +result to the existing `resolve_lockfile_paths` (dry-run) and +`regenerate_lockfiles` (live) helpers, whose signatures do not change. From 4e614ef7dd69a376b8f259c2da57fdfeb6aba6e8 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Tue, 7 Jul 2026 23:59:21 +0100 Subject: [PATCH 02/16] Restore a passing typecheck gate under ty 0.0.8 `uv tool install ty` (used by CI and locally) now resolves to ty 0.0.8, which does not narrow bindings after calls to `NoReturn` helpers and flags tomlkit's `OutOfOrderTableProxy` indexing. Six pre-existing diagnostics broke `make typecheck` on a clean tree. Restructure the guard cascades in `publish_index_check.py` as if/elif/else chains so narrowing flows from the branch conditions, extract `_downgrade_or_raise` to keep the named-dependency path typed as `str`, and mirror the existing `type: ignore[index]` comment for proxy indexing in `bump_toml.py`. No behaviour change. --- lading/commands/bump_toml.py | 2 +- lading/commands/publish_index_check.py | 30 ++++++++++++++++++++++---- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/lading/commands/bump_toml.py b/lading/commands/bump_toml.py index 43260614..d738b69b 100644 --- a/lading/commands/bump_toml.py +++ b/lading/commands/bump_toml.py @@ -116,7 +116,7 @@ def update_dependency_table( for name in dependency_names: if name not in table: continue - entry = table[name] # OutOfOrderTableProxy supports indexing + entry = table[name] # type: ignore[index] # OutOfOrderTableProxy supports indexing if update_dependency_entry(table, name, entry, target_version): changed = True return changed diff --git a/lading/commands/publish_index_check.py b/lading/commands/publish_index_check.py index 32e170eb..87f529fd 100644 --- a/lading/commands/publish_index_check.py +++ b/lading/commands/publish_index_check.py @@ -258,7 +258,7 @@ def _validate_dependency_placement( the plan and is ordered before the current crate. Raises the context exception class for every other case. """ - publishable_name_indexes = { + publishable_name_indexes: dict[str, int] = { _canonical_crate_name(entry.name): index for index, entry in enumerate(handling.plan.publishable) } @@ -287,13 +287,19 @@ def _validate_dependency_placement( missing_name, missing_index if missing_index is not None else "", ) + # An if/elif chain (rather than sequential guards) narrows missing_index + # to int in later branches: ty 0.0.8 does not narrow bindings after calls + # to NoReturn helpers, only via the branch conditions themselves. if missing_index is None: _raise_out_of_plan_dependency(context, missing_name=missing_name) - if missing_index == current_index: + elif missing_index == current_index: _raise_self_dependency(context, missing_name=missing_name) - if missing_index > current_index: + elif missing_index > current_index: _raise_out_of_order_dependency(context, missing_name=missing_name) - return _DependencyPlacement(current_index, missing_index, missing_canonical_name) + else: + return _DependencyPlacement( + current_index, missing_index, missing_canonical_name + ) def _emit_downgrade_success( @@ -366,9 +372,25 @@ def _handle_index_missing_version( ) missing_name = failure.missing_dependency_name + # if/else (rather than a guard clause) narrows missing_name to str for + # the downgrade path: ty 0.0.8 does not narrow bindings after calls to + # NoReturn helpers, only via the branch conditions themselves. if missing_name is None: _raise_name_extraction_failure(context) + else: + _downgrade_or_raise( + failure, context=context, handling=handling, missing_name=missing_name + ) + +def _downgrade_or_raise( + failure: CargoIndexLookupFailure, + *, + context: _IndexMissingVersionFailure, + handling: _IndexMissingVersionHandling, + missing_name: str, +) -> None: + """Downgrade a planned-dependency failure or raise when overrides forbid it.""" placement = _validate_dependency_placement(context, handling, missing_name) handling.logger.debug( From fe8558c595e1dabad592e201e01b75d91ff2aeb9 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Wed, 8 Jul 2026 00:00:49 +0100 Subject: [PATCH 03/16] Discover tracked nested lockfiles during bump regeneration `lading bump` regenerated only the workspace root `Cargo.lock` plus manifests listed in `bump.lockfile_manifests`, while the publish pre-flight validates every git-tracked `Cargo.lock`. A tracked nested fixture lockfile therefore went stale after a bump and `lading publish` failed with a manual repair message. Add `bump_lockfiles.merge_discovered_manifests`, which unions the configured manifests with manifests implied by git-tracked lockfiles (via `discover_tracked_lockfiles`, the same helper publish uses), and feed the merged tuple to both the dry-run and live regeneration paths in `bump._process_lockfiles`. Configured entries keep their order; discovered entries follow sorted, de-duplicated by resolved path. In a non-git workspace discovery degrades to a warning and the configured list, matching the publish pre-flight. A Stage A prototype (recorded in the ExecPlan) confirmed that `cargo update --workspace --manifest-path ` refreshes path-dependency entries in a nested standalone package's lockfile, so the existing cargo invocation is reused unchanged. The autouse `stub_lockfile_regeneration` fixture now also stubs the merge so manifest-focused unit suites never shell out to git. --- docs/execplans/regenerate-lockfiles.md | 312 +++++++++++++---------- lading/commands/bump_lockfiles.py | 71 +++++- lading/commands/lockfile.py | 10 +- tests/bdd/features/cli.feature | 9 + tests/bdd/steps/test_bump_steps.py | 34 +++ tests/unit/conftest.py | 7 +- tests/unit/test_bump_lockfile_rebuild.py | 44 +++- tests/unit/test_bump_lockfiles.py | 74 ++++++ 8 files changed, 414 insertions(+), 147 deletions(-) diff --git a/docs/execplans/regenerate-lockfiles.md b/docs/execplans/regenerate-lockfiles.md index 227b5adb..f97c960b 100644 --- a/docs/execplans/regenerate-lockfiles.md +++ b/docs/execplans/regenerate-lockfiles.md @@ -1,9 +1,8 @@ # Regenerate discovered nested lockfiles during `lading bump` -This ExecPlan (execution plan) is a living document. The sections -`Constraints`, `Tolerances`, `Risks`, `Progress`, `Surprises & Discoveries`, -`Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work -proceeds. +This ExecPlan (execution plan) is a living document. The sections `Constraints`, +`Tolerances`, `Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, +and `Outcomes & Retrospective` must be kept up to date as work proceeds. Status: IN PROGRESS @@ -28,14 +27,14 @@ directly: cargo generate-lockfile --manifest-path /crates/rstest-bdd/tests/ui_lints/Cargo.toml ``` -The user is left wondering why bump could not do this itself. Worse, the -users' guide (`docs/users-guide.md`, "After updating manifest versions" in the -bump section) already claims that bump "automatically refreshes any -git-tracked `Cargo.lock` files (excluding those under `target/`) that have an -adjacent `Cargo.toml`" — a promise the code does not keep. +The user is left wondering why bump could not do this itself. Worse, the users' +guide (`docs/users-guide.md`, "After updating manifest versions" in the bump +section) already claims that bump "automatically refreshes any git-tracked +`Cargo.lock` files (excluding those under `target/`) that have an adjacent +`Cargo.toml`" — a promise the code does not keep. -After this change, `lading bump` discovers every git-tracked `Cargo.lock` -using the same discovery helper the publish pre-flight uses +After this change, `lading bump` discovers every git-tracked `Cargo.lock` using +the same discovery helper the publish pre-flight uses (`lading.commands.lockfile.discover_tracked_lockfiles`), merges the discovered manifests with the configured `bump.lockfile_manifests` list, and regenerates all of them. A subsequent `lading publish` in the same repository then passes @@ -61,8 +60,8 @@ fixture lockfile and seeing that lockfile listed in the bump output with a - All code comments and documentation use en-GB-oxendict spelling. - No new external dependencies. - No single code file may exceed 400 lines (repository rule); adding to - `lading/commands/bump_lockfiles.py` (155 lines) and - `lading/commands/bump.py` must respect this. + `lading/commands/bump_lockfiles.py` (155 lines) and `lading/commands/bump.py` + must respect this. ## Tolerances (exception triggers) @@ -74,10 +73,10 @@ fixture lockfile and seeing that lockfile listed in the bump output with a tolerance.) - Behaviour: if the Stage A prototype shows that `cargo update --workspace --manifest-path ` does *not* freshen a - nested fixture lockfile that `cargo metadata --locked` reports as stale, - stop and escalate with the evidence before choosing a different cargo - command (candidate fallback: `cargo generate-lockfile`, which is what the - publish pre-flight already recommends to users). + nested fixture lockfile that `cargo metadata --locked` reports as stale, stop + and escalate with the evidence before choosing a different cargo command + (candidate fallback: `cargo generate-lockfile`, which is what the publish + pre-flight already recommends to users). - Iterations: if the focused test suite still fails after 3 fix attempts on any milestone, stop and escalate. - Ambiguity: if any repository under test relies on tracked lockfiles that @@ -90,34 +89,28 @@ fixture lockfile and seeing that lockfile listed in the bump output with a - Risk: `cargo update --workspace` on a nested manifest may not refresh the recorded version of a *path* dependency on a bumped workspace crate, because `--workspace` restricts updates to packages defined in that (nested) - workspace. - Severity: high. Likelihood: low. - Mitigation: Stage A is a throwaway prototype that reproduces the reported - layout (root workspace crate + nested fixture package with a path - dependency on it) and proves which cargo command restores freshness under - `cargo metadata --locked`. The command choice is decided by evidence before - any production code changes. + workspace. Severity: high. Likelihood: low. Mitigation: Stage A is a + throwaway prototype that reproduces the reported layout (root workspace crate + - nested fixture package with a path dependency on it) and proves which cargo + command restores freshness under `cargo metadata --locked`. The command + choice is decided by evidence before any production code changes. - Risk: newly discovered nested manifests may fail under cargo (for example, deliberately broken lint fixtures), making bump fail where it previously - succeeded. - Severity: medium. Likelihood: low. - Mitigation: any tracked lockfile that cargo cannot read already fails the - publish pre-flight today, so such repositories are already broken for - lading's workflow. The failure message from - `LockfileRegenerationError` names the manifest and exit code; - `--no-rebuild-lockfiles` remains as an escape hatch. Documented in the - users' guide update. + succeeded. Severity: medium. Likelihood: low. Mitigation: any tracked + lockfile that cargo cannot read already fails the publish pre-flight today, + so such repositories are already broken for lading's workflow. The failure + message from `LockfileRegenerationError` names the manifest and exit code; + `--no-rebuild-lockfiles` remains as an escape hatch. Documented in the users' + guide update. - Risk: dry-run currently makes no subprocess calls in `_process_lockfiles`; after this change it runs `git ls-files` (read-only) to list what would be - regenerated. - Severity: low. Likelihood: certain. - Mitigation: `git ls-files` mutates nothing. Tests pass a stub runner, so no - real subprocess runs in the unit suite. The behaviour is documented. + regenerated. Severity: low. Likelihood: certain. Mitigation: `git ls-files` + mutates nothing. Tests pass a stub runner, so no real subprocess runs in the + unit suite. The behaviour is documented. - Risk: bump output snapshots (`syrupy` `.ambr` files) change because more - lockfiles are listed. - Severity: low. Likelihood: high. - Mitigation: regenerate snapshots deliberately with `--snapshot-update` only - after eyeballing the new output, and commit them with the code change. + lockfiles are listed. Severity: low. Likelihood: high. Mitigation: regenerate + snapshots deliberately with `--snapshot-update` only after eyeballing the new + output, and commit them with the code change. ## Progress @@ -125,49 +118,100 @@ fixture lockfile and seeing that lockfile listed in the bump output with a root + configured manifests only; publish pre-flight discovers all tracked lockfiles; users' guide already promises discovery-based refresh. - [x] (2026-07-07 12:20Z) Stage A: prototype proved - `cargo update --workspace --manifest-path ` restores freshness for - a nested fixture package with a path dependency on a bumped workspace - crate. See `Artifacts and notes`. No fallback command needed. -- [ ] Stage B: red tests for discovery-driven regeneration (unit + BDD). -- [ ] Stage C: implementation (discovery helper in `bump_lockfiles`, wiring in - `bump._process_lockfiles`). + `cargo update --workspace --manifest-path ` restores freshness for a + nested fixture package with a path dependency on a bumped workspace crate. See + `Artifacts and notes`. No fallback command needed. +- [x] (2026-07-07 13:10Z) Stage B: red tests landed and observed failing for + the expected reasons — three new unit tests plus the two extended wiring + tests failed with `AttributeError: ... has no attribute + 'merge_discovered_manifests'`, and the new BDD scenario failed with + `'- fixtures/minimal/Cargo.lock (lockfile)'` absent from the CLI output. +- [x] (2026-07-07 13:40Z) Stage C: implemented `merge_discovered_manifests` + in `lading/commands/bump_lockfiles.py`, wired it into + `bump._process_lockfiles` for both dry-run and live paths, extended the + autouse `stub_lockfile_regeneration` fixture, and updated the + `lockfile.py` call-graph docstring. Full suite green: 683 passed, 62 + snapshots passed (no snapshot content changed). +- [x] (2026-07-07 13:50Z) Side quest: restored the `make typecheck` gate — + ty 0.0.8 (unpinned, installed by CI and locally via `uv tool install ty`) + flagged six pre-existing diagnostics on the clean tree. Committed + separately as "Restore a passing typecheck gate under ty 0.0.8". - [ ] Stage D: documentation, pre-flight message wording, snapshot updates, - full quality gates. + full quality gates, CodeRabbit review. ## Surprises & discoveries - Observation: `docs/users-guide.md` already documents the desired behaviour ("automatically refreshes any git-tracked `Cargo.lock` files") even though - the code only regenerates configured manifests. - Evidence: `docs/users-guide.md` bump section versus + the code only regenerates configured manifests. Evidence: + `docs/users-guide.md` bump section versus `lading/commands/bump.py::_process_lockfiles`, which reads only - `context.configuration.bump.lockfile_manifests`. - Impact: this change closes a documented behaviour gap rather than adding new - surface; the `lockfile_manifests` doc text must be re-scoped to "manifests - discovery cannot see" (for example, lockfiles not tracked by git). + `context.configuration.bump.lockfile_manifests`. Impact: this change closes a + documented behaviour gap rather than adding new surface; the + `lockfile_manifests` doc text must be re-scoped to "manifests discovery + cannot see" (for example, lockfiles not tracked by git). + +- Observation: the BDD infrastructure already anticipated discovery in bump — + `_mock_cargo_metadata` in `tests/bdd/steps/metadata_fixtures.py` and the + "workspace has tracked Cargo.lock files" given step both stub + `git ls-files "**/Cargo.lock" "Cargo.lock"` even though nothing in bump + invoked it. Evidence: the stubs existed before this change and cmd-mox + stubs are non-strict, so they sat unused. Impact: the existing lockfile + scenarios worked unchanged once discovery was wired in; only the new + nested-lockfile scenario needed a new given step. +- Observation: cmd-mox registers one `CommandDouble` per command name and + dispatches stubs by name alone (`controller._make_response`), so a second + `stub("cargo::update")` re-configures the first rather than adding an + argument-matched alternative. Evidence: `cmd_mox/controller.py::_get_double` + returns the existing double. Impact: the nested-lockfile given step + registers `cargo::update` without `with_args` so one response serves both + the root and nested manifest invocations. +- Observation: `uv tool install ty` now resolves to ty 0.0.8, which fails + `make typecheck` on the clean tree with six diagnostics in + `lading/commands/bump_toml.py` and `lading/commands/publish_index_check.py`. + Evidence: `git stash && make typecheck` reproduced all six without this + branch's changes; a scratch probe confirmed ty 0.0.8 does not narrow + bindings after calls to `NoReturn` helpers but does honour `NoReturn` for + reachability. Impact: fixed ahead of the feature commit (if/elif/else + restructure plus one `type: ignore[index]` mirroring an existing comment) + so the gate is green before CodeRabbit review. ## Decision log - Decision: make discovery always-on for bump rather than adding a - `bump.discover_lockfiles` toggle. - Rationale: the publish pre-flight already treats every tracked lockfile as - required-fresh, so there is no coherent use case for bumping versions while - leaving a tracked lockfile stale. `rebuild_lockfiles = false` and - `--no-rebuild-lockfiles` already exist as global escape hatches. Fewer - configuration keys, and bump/publish stay consistent. - Date/Author: 2026-07-07, planning session. + `bump.discover_lockfiles` toggle. Rationale: the publish pre-flight already + treats every tracked lockfile as required-fresh, so there is no coherent use + case for bumping versions while leaving a tracked lockfile stale. + `rebuild_lockfiles = false` and `--no-rebuild-lockfiles` already exist as + global escape hatches. Fewer configuration keys, and bump/publish stay + consistent. Date/Author: 2026-07-07, planning session. - Decision: keep `bump.lockfile_manifests` rather than deprecating it. - Rationale: it remains the only way to regenerate a lockfile that git does - not track (for example, generated fixtures ignored by `.gitignore`), and - removing a configuration key is a breaking change out of scope here. - Date/Author: 2026-07-07, planning session. + Rationale: it remains the only way to regenerate a lockfile that git does not + track (for example, generated fixtures ignored by `.gitignore`), and removing + a configuration key is a breaking change out of scope here. Date/Author: + 2026-07-07, planning session. - Decision: perform the union of configured and discovered manifests inside `lading/commands/bump_lockfiles.py` via a new public helper, and keep - `bump._process_lockfiles` a thin caller. - Rationale: `bump_lockfiles` already owns manifest validation and - de-duplication (`_resolve_manifest_paths`); colocating discovery keeps the - feature testable without driving the whole bump command. - Date/Author: 2026-07-07, planning session. + `bump._process_lockfiles` a thin caller. Rationale: `bump_lockfiles` already + owns manifest validation and de-duplication (`_resolve_manifest_paths`); + colocating discovery keeps the feature testable without driving the whole + bump command. Date/Author: 2026-07-07, planning session. + +- Decision: land the Stage B red tests without interim strict-xfail markers, + committing them together with the Stage C implementation. + Rationale: the red failures were observed and transcribed directly (the + plan's validation evidence), and the combined commit keeps the suite green + at every commit boundary as required by the repository's gating rules; a + strict-xfail round-trip would have added churn without extra proof. + Date/Author: 2026-07-07, implementation session. +- Decision: fix the pre-existing ty 0.0.8 typecheck failures in a separate + commit on this branch rather than ignoring them or pinning ty. + Rationale: CI installs ty unpinned (`uv tool install ty` in + `.github/workflows/ci.yml`), so main's next CI run would fail regardless; + the gates must pass before CodeRabbit review; and the fixes are small, + behaviour-preserving restructures. Pinning would hide the drift rather + than resolve it. + Date/Author: 2026-07-07, implementation session. ## Outcomes & retrospective @@ -176,15 +220,15 @@ To be completed as milestones land. ## Context and orientation lading is a Python (3.13, `uv`-managed) command-line tool that automates -version bumps and crates.io publication for Rust Cargo workspaces. The -relevant pieces: +version bumps and crates.io publication for Rust Cargo workspaces. The relevant +pieces: - `lading/commands/bump.py` orchestrates `lading bump`. After rewriting manifest versions it calls `_process_lockfiles(context, changed_manifests)`, which returns the tuple of lockfile paths that were (or, in dry-run, would be) regenerated. It currently passes only - `context.configuration.bump.lockfile_manifests` (a tuple of - workspace-relative `Cargo.toml` path strings) to the helpers below. + `context.configuration.bump.lockfile_manifests` (a tuple of workspace-relative + `Cargo.toml` path strings) to the helpers below. `context.base_options.command_runner` is an optional `CommandRunner` — a callable taking a command sequence and returning `(exit_code, stdout, stderr)` — used for dependency injection in tests; @@ -201,8 +245,8 @@ relevant pieces: `discover_tracked_lockfiles(workspace_root, runner, *, manifest_exists=...)` runs `git ls-files "**/Cargo.lock" "Cargo.lock"`, filters out paths with a `target` component, keeps only lockfiles with an adjacent `Cargo.toml`, and - returns absolute lockfile paths. In a non-git directory it logs a warning - and returns an empty tuple. + returns absolute lockfile paths. In a non-git directory it logs a warning and + returns an empty tuple. - `lading/commands/publish_preflight.py` contains `_validate_lockfile_freshness`, which discovers tracked lockfiles, probes each with `cargo metadata --locked`, and raises `PublishPreflightError` with @@ -213,8 +257,8 @@ relevant pieces: configuration change is needed for this plan. - Tests live under `tests/unit/` (pytest, with `syrupy` snapshots under `tests/unit/__snapshots__/`) and `tests/bdd/` (pytest-bdd; feature files in - `tests/bdd/features/`, notably `cli.feature` scenarios "Bump rebuilds - tracked lockfiles" and "Publish pre-flight aborts when lockfile is stale"). + `tests/bdd/features/`, notably `cli.feature` scenarios "Bump rebuilds tracked + lockfiles" and "Publish pre-flight aborts when lockfile is stale"). `tests/helpers/workspace_builders.py` provides `_make_workspace` and `_make_config` used by `tests/unit/test_bump_lockfile_rebuild.py`. - Quality gates are Makefile targets, run sequentially (never in parallel): @@ -232,31 +276,33 @@ scratchpad directory, build a miniature repository imitating the failure report: a git-initialized Cargo workspace with one member crate `alpha` (version 0.1.0), plus a nested, tracked, non-member package at `fixtures/minimal/` whose `Cargo.toml` declares -`alpha = { path = "../..", version = "0.1.0" }` (adjust the path to point at -the `alpha` crate) and which has a committed `Cargo.lock`. Then simulate a -bump by editing versions to 0.2.0 (both `alpha`'s version and the fixture's -version requirement), confirm `cargo metadata --locked --manifest-path -fixtures/minimal/Cargo.toml` fails as stale, run `cargo update --workspace ---manifest-path fixtures/minimal/Cargo.toml`, and confirm the freshness probe -now passes. If it does not, try `cargo generate-lockfile` and record the -outcome in `Decision Log` before proceeding (see Tolerances). The prototype is -throwaway; its findings are recorded here, not committed. +`alpha = { path = "../..", version = "0.1.0" }` (adjust the path to point at the +`alpha` crate) and which has a committed `Cargo.lock`. Then simulate a bump by +editing versions to 0.2.0 (both `alpha`'s version and the fixture's version +requirement), confirm +`cargo metadata --locked --manifest-path fixtures/minimal/Cargo.toml` fails as +stale, run +`cargo update --workspace --manifest-path fixtures/minimal/Cargo.toml`, and +confirm the freshness probe now passes. If it does not, try +`cargo generate-lockfile` and record the outcome in `Decision Log` before +proceeding (see Tolerances). The prototype is throwaway; its findings are +recorded here, not committed. Stage B (red): specify the behaviour with failing tests. 1. In `tests/unit/test_bump_lockfiles.py`, add tests for a new function `lading.commands.bump_lockfiles.merge_discovered_manifests` (final name at - implementer's discretion, recorded in `Decision Log` if it differs): - given a workspace root, a configured manifest tuple, and a stub runner - whose `git ls-files` output lists nested lockfiles, it returns the - configured entries followed by workspace-relative POSIX manifest strings - for each discovered lockfile, without duplicates (a manifest both - configured and discovered appears once, in its configured position). A - second test drives the non-git fallback: the stub runner returns exit code - 128 with "not a git repository" on stderr and the function returns just - the configured tuple. Mark these `@pytest.mark.xfail(strict=True, - reason="discovery merge not implemented")` until red is observed, then - remove the marker during Stage C. + implementer's discretion, recorded in `Decision Log` if it differs): given a + workspace root, a configured manifest tuple, and a stub runner whose + `git ls-files` output lists nested lockfiles, it returns the configured + entries followed by workspace-relative POSIX manifest strings for each + discovered lockfile, without duplicates (a manifest both configured and + discovered appears once, in its configured position). A second test drives + the non-git fallback: the stub runner returns exit code 128 with "not a git + repository" on stderr and the function returns just the configured tuple. + Mark these + `@pytest.mark.xfail(strict=True, reason="discovery merge not implemented")` + until red is observed, then remove the marker during Stage C. 2. In `tests/unit/test_bump_lockfile_rebuild.py`, extend the existing monkeypatch-style tests: patch discovery (not the real git) so that `bump.run` passes the union of configured and discovered manifests to @@ -265,11 +311,11 @@ Stage B (red): specify the behaviour with failing tests. `"lockfile_manifests": ()` will need the new expected union values. 3. In `tests/bdd/features/cli.feature`, extend the "Bump rebuilds tracked lockfiles" scenario (or add a sibling scenario "Bump refreshes discovered - nested lockfiles") so the workspace contains a tracked nested lockfile - that is *not* configured in `lading.toml`, and assert the CLI output lists - it with the `(lockfile)` suffix and that it was refreshed. Reuse the - existing step vocabulary in `tests/bdd/steps/` where possible; add a Given - step for the nested fixture only if none fits. + nested lockfiles") so the workspace contains a tracked nested lockfile that + is *not* configured in `lading.toml`, and assert the CLI output lists it + with the `(lockfile)` suffix and that it was refreshed. Reuse the existing + step vocabulary in `tests/bdd/steps/` where possible; add a Given step for + the nested fixture only if none fits. Run the focused tests and confirm each fails for the expected reason before Stage C. @@ -279,18 +325,17 @@ Stage C (green): implement the minimal change. 1. In `lading/commands/bump_lockfiles.py`, add the discovery merge function. It imports `discover_tracked_lockfiles` from `lading.commands.lockfile`, defaults its runner to `lading.runtime.subprocess_runner` when `None` - (matching `regenerate_lockfiles`), converts each discovered lockfile path - to `(lockfile.parent / "Cargo.toml").relative_to(workspace_root)` rendered - as a POSIX string, sorts the discovered entries for determinism, and - returns configured entries first followed by unseen discovered entries. - The existing `_resolve_manifest_paths` continues to prepend the root - manifest and de-duplicate resolved paths, so the root lockfile stays - first regardless of what discovery returns. + (matching `regenerate_lockfiles`), converts each discovered lockfile path to + `(lockfile.parent / "Cargo.toml").relative_to(workspace_root)` rendered as a + POSIX string, sorts the discovered entries for determinism, and returns + configured entries first followed by unseen discovered entries. The existing + `_resolve_manifest_paths` continues to prepend the root manifest and + de-duplicate resolved paths, so the root lockfile stays first regardless of + what discovery returns. 2. In `lading/commands/bump.py::_process_lockfiles`, call the merge function - once (passing `context.base_options.command_runner`), and feed its result - to both the dry-run `resolve_lockfile_paths` branch and the live - `regenerate_lockfiles` branch, so dry-run and live runs report the same - set. + once (passing `context.base_options.command_runner`), and feed its result to + both the dry-run `resolve_lockfile_paths` branch and the live + `regenerate_lockfiles` branch, so dry-run and live runs report the same set. 3. Update module docstrings that describe the old contract: the header of `bump_lockfiles.py` ("any configured nested lockfiles") and the call-graph paragraph in `lockfile.py` (which currently says discovery is publish-only). @@ -300,17 +345,18 @@ Stage D (refactor, documentation, cleanup): 1. Regenerate affected `syrupy` snapshots deliberately and review the diffs. 2. `docs/users-guide.md`: the bump section's discovery claim becomes true — tighten it to mention that configured `lockfile_manifests` are additionally - regenerated; re-scope the `lockfile_manifests` key description to - "manifests whose lockfiles git does not track (discovery finds tracked - ones automatically)"; extend the "Lockfile regeneration runs…" paragraph to - say the command runs for the workspace root, each configured manifest, and - each discovered tracked lockfile's manifest. + regenerated; re-scope the `lockfile_manifests` key description to "manifests + whose lockfiles git does not track (discovery finds tracked ones + automatically)"; extend the "Lockfile regeneration runs…" paragraph to say + the command runs for the workspace root, each configured manifest, and each + discovered tracked lockfile's manifest. 3. `lading/commands/publish_preflight.py::_build_stale_lockfile_message`: soften "This commonly happens after running `lading bump`" to reflect that bump now repairs tracked lockfiles itself — for example, "This can happen after manifest edits outside `lading bump`, or after running bump with `--no-rebuild-lockfiles`; repair each stale lockfile directly:". Update the - snapshot `tests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambr`, + snapshot + `tests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambr`, the assertion in `tests/bdd/features/cli.feature` ("Publish pre-flight aborts when lockfile is stale"), and the quoted message in `docs/users-guide.md` to match. @@ -349,8 +395,8 @@ cargo metadata --locked --manifest-path fixtures/minimal/Cargo.toml \ --format-version=1 >/dev/null; echo "fresh probe exit: $?" # expect 0 ``` -Focused test runs during Stages B and C (adjust node IDs to the tests -actually written): +Focused test runs during Stages B and C (adjust node IDs to the tests actually +written): ```bash uv run pytest tests/unit/test_bump_lockfiles.py -q @@ -387,8 +433,8 @@ Acceptance behaviour: in a git-tracked Cargo workspace containing a nested, tracked, non-member fixture package with its own `Cargo.lock` and *no* `bump.lockfile_manifests` configuration, running `lading bump 0.2.0` lists the nested lockfile in the output with a `(lockfile)` suffix and leaves -`cargo metadata --locked` passing for the nested manifest; `lading bump 0.2.0 ---dry-run` lists the same lockfile without modifying it; and +`cargo metadata --locked` passing for the nested manifest; +`lading bump 0.2.0 --dry-run` lists the same lockfile without modifying it; and `lading publish` (in that repository, after a real bump) no longer fails its lockfile freshness pre-flight for lockfiles that bump could regenerate. @@ -414,19 +460,19 @@ Every step is re-runnable. Discovery (`git ls-files`) and the freshness probe (`cargo metadata --locked`) are read-only. `cargo update --workspace` is idempotent: rerunning it on a fresh lockfile is a no-op. Regeneration is not atomic across manifests (documented in `regenerate_lockfiles`): if cargo fails -partway, earlier lockfiles stay updated; the recovery path is to fix the -cargo error and rerun `lading bump`, which converges. The Stage A prototype -lives entirely in the scratchpad and is deleted afterwards. If a milestone -must be abandoned mid-way, `git status` plus `git checkout -- ` restores -the tree; committed milestones are individually revertable. +partway, earlier lockfiles stay updated; the recovery path is to fix the cargo +error and rerun `lading bump`, which converges. The Stage A prototype lives +entirely in the scratchpad and is deleted afterwards. If a milestone must be +abandoned mid-way, `git status` plus `git checkout -- ` restores the +tree; committed milestones are individually revertable. ## Artifacts and notes Stage A prototype transcript (2026-07-07, scratchpad `proto/`): a git-initialized workspace with member crate `alpha` 0.1.0 and a standalone fixture package `fixtures/minimal` (empty `[workspace]` table, path dependency -`alpha = { path = "../../alpha", version = "0.1.0" }`, committed -`Cargo.lock`). After editing both versions to 0.2.0: +`alpha = { path = "../../alpha", version = "0.1.0" }`, committed `Cargo.lock`). +After editing both versions to 0.2.0: ```plaintext $ cargo metadata --locked --manifest-path fixtures/minimal/Cargo.toml ... @@ -443,9 +489,9 @@ fresh probe exit: 0 Conclusion: `--workspace` does refresh path-dependency entries whose manifest versions changed (the update is "necessary to satisfy other dependency requirements"), so bump can reuse its existing cargo invocation unchanged for -discovered manifests. One incidental finding: a standalone nested package -needs an empty `[workspace]` table to avoid being claimed by the outer -workspace — real fixture packages with their own lockfiles already have this. +discovered manifests. One incidental finding: a standalone nested package needs +an empty `[workspace]` table to avoid being claimed by the outer workspace — +real fixture packages with their own lockfiles already have this. ## Interfaces and dependencies diff --git a/lading/commands/bump_lockfiles.py b/lading/commands/bump_lockfiles.py index 7f518df9..e4fc73cb 100644 --- a/lading/commands/bump_lockfiles.py +++ b/lading/commands/bump_lockfiles.py @@ -1,8 +1,10 @@ """Lockfile regeneration helpers for ``lading bump``. -The public entry point is :func:`regenerate_lockfiles`, which rebuilds the -workspace root lockfile and any configured nested lockfiles after manifest -versions change. +The public entry points are :func:`merge_discovered_manifests`, which unions +the configured ``bump.lockfile_manifests`` entries with manifests implied by +git-tracked ``Cargo.lock`` files, and :func:`regenerate_lockfiles`, which +rebuilds the workspace root lockfile and every merged nested lockfile after +manifest versions change. """ from __future__ import annotations @@ -15,6 +17,7 @@ import typing as typ from pathlib import Path +from lading.commands.lockfile import discover_tracked_lockfiles from lading.exceptions import LadingError from lading.runtime import CommandRunner, subprocess_runner from lading.utils.process import with_detail @@ -25,7 +28,61 @@ class LockfileRegenerationError(LadingError): """Raise when lockfile regeneration cannot validate or execute.""" +def merge_discovered_manifests( + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], + *, + runner: CommandRunner | None = None, +) -> tuple[str, ...]: + """Return configured manifests plus discovered tracked-lockfile manifests. + + Parameters + ---------- + workspace_root : Path + Absolute path to the Cargo workspace root. + lockfile_manifests : Sequence[str] + Configured manifest paths relative to *workspace_root*. + runner : CommandRunner or None, optional + Callable used to invoke ``git``. Defaults to + :func:`lading.runtime.subprocess_runner` when ``None``. + Returns + ------- + tuple[str, ...] + The configured entries in their original order, followed by + workspace-relative POSIX manifest paths implied by git-tracked + ``Cargo.lock`` files (via + :func:`lading.commands.lockfile.discover_tracked_lockfiles`) in + sorted order, skipping any manifest already configured under an + equivalent spelling. In a non-git workspace the configured tuple is + returned unchanged. + + Examples + -------- + With ``fixtures/minimal/Cargo.lock`` tracked in git: + + ```python + merge_discovered_manifests(workspace_root, ("crates/ui/Cargo.toml",)) + # ("crates/ui/Cargo.toml", "Cargo.toml", "fixtures/minimal/Cargo.toml") + ``` + """ + command_runner = subprocess_runner if runner is None else runner + discovered = discover_tracked_lockfiles(workspace_root, command_runner) + seen_manifests = { + (workspace_root / manifest).resolve() for manifest in lockfile_manifests + } + merged = list(lockfile_manifests) + discovered_relative = sorted( + (lockfile_path.parent / "Cargo.toml").relative_to(workspace_root).as_posix() + for lockfile_path in discovered + ) + for relative_manifest in discovered_relative: + resolved = (workspace_root / relative_manifest).resolve() + if resolved in seen_manifests: + continue + seen_manifests.add(resolved) + merged.append(relative_manifest) + return tuple(merged) def resolve_lockfile_paths( workspace_root: Path, lockfile_manifests: cabc.Sequence[str], @@ -274,7 +331,10 @@ def resolve_lockfile_paths( workspace or not named ``Cargo.toml``), propagated from :func:`_resolve_manifest_paths`. """ - return resolve_lockfile_paths(workspace_root, lockfile_manifests) + merged_manifests = merge_discovered_manifests( + workspace_root, lockfile_manifests, runner=self.runner + ) + return resolve_lockfile_paths(workspace_root, merged_manifests) def regenerate_lockfiles( self, @@ -308,9 +368,10 @@ def regenerate_lockfiles( cargo error unchanged; multiple failures raise one aggregated error listing each failed manifest with a repair command. """ - return regenerate_lockfiles( + merged_manifests = merge_discovered_manifests( workspace_root, lockfile_manifests, runner=self.runner ) + return regenerate_lockfiles(workspace_root, merged_manifests, runner=self.runner) class LockfileRepository(typ.Protocol): diff --git a/lading/commands/lockfile.py b/lading/commands/lockfile.py index 2b309018..b94cd1ba 100644 --- a/lading/commands/lockfile.py +++ b/lading/commands/lockfile.py @@ -13,10 +13,12 @@ Call graph: ``lading publish`` uses :func:`discover_tracked_lockfiles` and :func:`validate_lockfile_freshness` before the cargo check/test pre-flight, so stale lockfiles fail early with an actionable repair command. -``lading bump`` regenerates lockfiles via -:func:`lading.commands.bump_lockfiles.regenerate_lockfiles` instead, which -runs ``cargo update --workspace``: bump wants existing pinned versions -refreshed in place after manifest rewrites, whereas validation here uses +``lading bump`` uses :func:`discover_tracked_lockfiles` too — via +:func:`lading.commands.bump_lockfiles.merge_discovered_manifests` — but then +regenerates the discovered and configured lockfiles through +:func:`lading.commands.bump_lockfiles.regenerate_lockfiles`, which runs +``cargo update --workspace``: bump wants existing pinned versions refreshed +in place after manifest rewrites, whereas validation here uses ``cargo metadata --locked`` purely as a read-only freshness probe. The publish pre-flight domain reaches these operations through the diff --git a/tests/bdd/features/cli.feature b/tests/bdd/features/cli.feature index b83c49bb..7fd6dd8b 100644 --- a/tests/bdd/features/cli.feature +++ b/tests/bdd/features/cli.feature @@ -35,6 +35,15 @@ Feature: Lading CLI scaffolding And the CLI output lists lockfile path "- nested/Cargo.lock (lockfile)" And the bump command refreshed tracked lockfiles + Scenario: Bump refreshes a discovered nested Cargo.lock file + Given a workspace directory with configuration + And cargo metadata describes a sample workspace + And the workspace has a tracked nested Cargo.lock file + When I invoke lading bump 1.2.3 with that workspace + Then the CLI output lists lockfile path "- Cargo.lock (lockfile)" + And the CLI output lists lockfile path "- fixtures/minimal/Cargo.lock (lockfile)" + And the bump command refreshed tracked lockfiles + Scenario: Bump dry-run does not modify lockfiles Given a workspace directory with configuration And cargo metadata describes a sample workspace diff --git a/tests/bdd/steps/test_bump_steps.py b/tests/bdd/steps/test_bump_steps.py index f429332a..490ef711 100644 --- a/tests/bdd/steps/test_bump_steps.py +++ b/tests/bdd/steps/test_bump_steps.py @@ -40,6 +40,40 @@ def given_workspace_has_tracked_lockfiles( ).returns(exit_code=0, stdout="cargo update --workspace\n", stderr="") +@given("the workspace has a tracked nested Cargo.lock file") +def given_workspace_has_nested_tracked_lockfile( + cmd_mox: CmdMox, + monkeypatch: pytest.MonkeyPatch, + workspace_directory: Path, +) -> None: + """Track a nested standalone package lockfile alongside the root lockfile. + + The nested package is not listed in ``bump.lockfile_manifests``; bump must + discover it from the git index and refresh it anyway. + """ + from tests.helpers.workspace_helpers import install_cargo_stub + + install_cargo_stub(cmd_mox, monkeypatch) + (workspace_directory / "Cargo.lock").write_text("# root lock\n", encoding="utf-8") + nested_dir = workspace_directory / "fixtures" / "minimal" + nested_dir.mkdir(parents=True) + (nested_dir / "Cargo.toml").write_text( + '[package]\nname = "minimal"\nversion = "0.1.0"\n\n[workspace]\n', + encoding="utf-8", + ) + (nested_dir / "Cargo.lock").write_text("# nested lock\n", encoding="utf-8") + cmd_mox.stub("git").with_args("ls-files", "**/Cargo.lock", "Cargo.lock").returns( + exit_code=0, + stdout="Cargo.lock\nfixtures/minimal/Cargo.lock\n", + stderr="", + ) + # Stubs dispatch by command name alone, so one argless registration covers + # the root and nested manifest invocations alike. + cmd_mox.stub("cargo::update").returns( + exit_code=0, stdout="cargo update --workspace\n", stderr="" + ) + + @when( parsers.parse("I invoke lading bump {version} with that workspace"), target_fixture="cli_run", diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 7de50610..8e1d5bd8 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -77,9 +77,14 @@ def disable_publish_preflight(monkeypatch: pytest.MonkeyPatch) -> None: def stub_lockfile_regeneration( request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch ) -> None: - """Avoid invoking Cargo from manifest-focused bump tests.""" + """Avoid invoking Cargo or git from manifest-focused bump tests.""" if request.module.__name__.rsplit(".", 1)[-1] not in _LOCKFILE_STUB_MODULES: return + monkeypatch.setattr( + bump.bump_lockfiles, + "merge_discovered_manifests", + lambda _root, manifests, **_kwargs: tuple(manifests), + ) monkeypatch.setattr( bump.bump_lockfiles, "regenerate_lockfiles", diff --git a/tests/unit/test_bump_lockfile_rebuild.py b/tests/unit/test_bump_lockfile_rebuild.py index b4445a8d..0dbe9c61 100644 --- a/tests/unit/test_bump_lockfile_rebuild.py +++ b/tests/unit/test_bump_lockfile_rebuild.py @@ -37,6 +37,18 @@ def test_run_rebuilds_lockfiles_when_enabled( configuration = _make_config() nested_lockfile = tmp_path / "crates/ui/Cargo.lock" captured: dict[str, object] = {} + merged_manifests = ("crates/ui/Cargo.toml",) + + def fake_merge_discovered_manifests( + workspace_root: pathlib.Path, + lockfile_manifests: tuple[str, ...], + *, + runner: object | None = None, + ) -> tuple[str, ...]: + captured["merge_workspace_root"] = workspace_root + captured["merge_lockfile_manifests"] = lockfile_manifests + captured["merge_runner"] = runner + return merged_manifests def fake_regenerate_lockfiles( workspace_root: pathlib.Path, @@ -50,6 +62,11 @@ def fake_regenerate_lockfiles( captured["runner"] = runner return (tmp_path / "Cargo.lock", nested_lockfile) + monkeypatch.setattr( + bump.bump_lockfiles, + "merge_discovered_manifests", + fake_merge_discovered_manifests, + ) monkeypatch.setattr( bump.bump_lockfiles, "regenerate_lockfiles", @@ -67,11 +84,14 @@ def fake_regenerate_lockfiles( ) assert captured == { + "merge_workspace_root": tmp_path, + "merge_lockfile_manifests": (), + "merge_runner": None, "calls": 1, "workspace_root": tmp_path, - "lockfile_manifests": (), + "lockfile_manifests": merged_manifests, "runner": None, - }, "expected a single regenerate_lockfiles call for the workspace root" + }, "expected regenerate_lockfiles to receive the merged manifest tuple" assert message == snapshot @@ -145,6 +165,16 @@ def test_run_reports_lockfiles_in_dry_run( ) nested_lockfile = tmp_path / "crates/ui/Cargo.lock" captured: dict[str, object] = {} + merged_manifests = ("crates/ui/Cargo.toml", "fixtures/minimal/Cargo.toml") + + def fake_merge_discovered_manifests( + workspace_root: pathlib.Path, + lockfile_manifests: tuple[str, ...], + *, + runner: object | None = None, + ) -> tuple[str, ...]: + captured["merge_lockfile_manifests"] = lockfile_manifests + return merged_manifests def fake_resolve_lockfile_paths( workspace_root: pathlib.Path, @@ -157,6 +187,11 @@ def fake_resolve_lockfile_paths( def fail_regeneration(*args: object, **kwargs: object) -> typ.NoReturn: pytest.fail("dry-run lockfile reporting should not invoke Cargo") + monkeypatch.setattr( + bump.bump_lockfiles, + "merge_discovered_manifests", + fake_merge_discovered_manifests, + ) monkeypatch.setattr( bump.bump_lockfiles, "resolve_lockfile_paths", @@ -180,9 +215,10 @@ def fail_regeneration(*args: object, **kwargs: object) -> typ.NoReturn: ) assert captured == { + "merge_lockfile_manifests": ("crates/ui/Cargo.toml",), "workspace_root": tmp_path, - "lockfile_manifests": ("crates/ui/Cargo.toml",), - }, "expected dry-run lockfile path resolution for the configured manifest" + "lockfile_manifests": merged_manifests, + }, "expected dry-run lockfile resolution for the merged manifest tuple" assert message == snapshot diff --git a/tests/unit/test_bump_lockfiles.py b/tests/unit/test_bump_lockfiles.py index 894b9332..6e8982f3 100644 --- a/tests/unit/test_bump_lockfiles.py +++ b/tests/unit/test_bump_lockfiles.py @@ -50,7 +50,81 @@ def __call__( self.invocations.append(_Invocation(command=tuple(command), cwd=cwd)) return self.result +def _write_manifest(directory: Path) -> None: + """Create ``directory`` with a placeholder ``Cargo.toml`` inside it.""" + directory.mkdir(parents=True, exist_ok=True) + (directory / "Cargo.toml").write_text( + '[package]\nname = "placeholder"\nversion = "0.1.0"\n', + encoding="utf-8", + ) + +def test_merge_discovered_manifests_appends_tracked_manifests( + tmp_path: Path, +) -> None: + """Discovered tracked-lockfile manifests follow configured entries, sorted.""" + _write_manifest(tmp_path) + _write_manifest(tmp_path / "crates/ui") + _write_manifest(tmp_path / "fixtures/minimal") + runner = _RecordingRunner( + result=( + 0, + "fixtures/minimal/Cargo.lock\nCargo.lock\ncrates/ui/Cargo.lock\n", + "", + ) + ) + + merged = bump_lockfiles.merge_discovered_manifests( + tmp_path, + ("crates/ui/Cargo.toml",), + runner=runner, + ) + + assert merged == ( + "crates/ui/Cargo.toml", + "Cargo.toml", + "fixtures/minimal/Cargo.toml", + ) + assert runner.invocations == [ + _Invocation( + command=("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), + cwd=tmp_path, + ) + ] + +def test_merge_discovered_manifests_deduplicates_configured_forms( + tmp_path: Path, +) -> None: + """A manifest configured under an equivalent spelling is not re-appended.""" + _write_manifest(tmp_path / "fixtures/minimal") + runner = _RecordingRunner(result=(0, "fixtures/minimal/Cargo.lock\n", "")) + + merged = bump_lockfiles.merge_discovered_manifests( + tmp_path, + ("./fixtures/minimal/Cargo.toml",), + runner=runner, + ) + + assert merged == ("./fixtures/minimal/Cargo.toml",) + +def test_merge_discovered_manifests_returns_configured_outside_git( + tmp_path: Path, +) -> None: + """A non-git workspace degrades to the configured manifests unchanged.""" + runner = _RecordingRunner( + result=( + 128, + "", + "fatal: not a git repository (or any of the parent directories): .git", + ) + ) + + merged = bump_lockfiles.merge_discovered_manifests( + tmp_path, + ("crates/ui/Cargo.toml",), + runner=runner, + ) + assert merged == ("crates/ui/Cargo.toml",) def test_regenerate_lockfiles_includes_workspace_manifest(tmp_path: Path) -> None: """The workspace root manifest should always be regenerated.""" runner = _RecordingRunner() From 8f4e2871072df52c70eeb37a6cc33fbb7a869f8a Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Wed, 8 Jul 2026 00:08:19 +0100 Subject: [PATCH 04/16] Document bump lockfile discovery and reword the stale pre-flight hint Now that `lading bump` discovers and regenerates git-tracked lockfiles itself, the publish pre-flight's "This commonly happens after running `lading bump`" hint would mislead. Point it at the remaining causes instead: manifest edits made without bump, or bump runs with `--no-rebuild-lockfiles`. Regenerate the affected snapshot. Update the users' guide (bump discovery paragraph, quoted pre-flight message, `lockfile_manifests` and `rebuild_lockfiles` key descriptions, and the regeneration paragraph) and the developers' guide (`bump_lockfiles` module description and the lockfile helpers section) to describe the merged configured-plus-discovered manifest set. --- docs/developers-guide.md | 22 ++++-- docs/execplans/regenerate-lockfiles.md | 74 ++++++++++--------- docs/users-guide.md | 70 ++++++++++-------- lading/commands/publish_preflight.py | 3 +- .../test_preflight_lockfile_validation.ambr | 2 +- 5 files changed, 98 insertions(+), 73 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 0002b3f3..e8f48323 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -204,9 +204,14 @@ Markdown links in fenced code blocks, indented code blocks, and inline code spans are preserved verbatim. `lading.commands.bump_lockfiles` owns Cargo lockfile discovery and regeneration -after manifest changes. It always includes the workspace root `Cargo.toml`, -validates configured nested manifests before invoking Cargo, and de-duplicates -resolved manifest paths. +after manifest changes. `merge_discovered_manifests` unions the configured +`bump.lockfile_manifests` entries with manifests implied by git-tracked +`Cargo.lock` files (reusing +`lading.commands.lockfile .discover_tracked_lockfiles`); configured entries +keep their order and discovered entries follow in sorted order. +`regenerate_lockfiles` always includes the workspace root `Cargo.toml`, +validates nested manifests before invoking Cargo, and de-duplicates resolved +manifest paths. For screen readers: the following flowchart traces `regenerate_lockfiles`. It resolves the manifest list, then initializes empty `lockfiles` and `failures` @@ -370,10 +375,13 @@ perform the error-handling and path-filtering passes respectively. Lockfile regeneration after `lading bump` is owned by `lading.commands.bump_lockfiles.regenerate_lockfiles`, which runs -`cargo update --workspace` per configured manifest. The two cargo strategies -differ deliberately: bump refreshes existing pinned versions in place after -manifest rewrites, while publish only probes freshness read-only via -`cargo metadata --locked` and never regenerates. +`cargo update --workspace` per merged manifest — +`bump_lockfiles.merge_discovered_manifests` unions the configured +`bump.lockfile_manifests` entries with manifests implied by +`discover_tracked_lockfiles`. The two cargo strategies differ deliberately: +bump refreshes existing pinned versions in place after manifest rewrites, while +publish only probes freshness read-only via `cargo metadata --locked` and never +regenerates. `validate_lockfile_freshness(manifest_path, runner)` runs `cargo metadata --locked --manifest-path ... --format-version=1`. It returns a diff --git a/docs/execplans/regenerate-lockfiles.md b/docs/execplans/regenerate-lockfiles.md index f97c960b..eddbacdc 100644 --- a/docs/execplans/regenerate-lockfiles.md +++ b/docs/execplans/regenerate-lockfiles.md @@ -123,21 +123,30 @@ fixture lockfile and seeing that lockfile listed in the bump output with a `Artifacts and notes`. No fallback command needed. - [x] (2026-07-07 13:10Z) Stage B: red tests landed and observed failing for the expected reasons — three new unit tests plus the two extended wiring - tests failed with `AttributeError: ... has no attribute - 'merge_discovered_manifests'`, and the new BDD scenario failed with - `'- fixtures/minimal/Cargo.lock (lockfile)'` absent from the CLI output. + tests failed with + `AttributeError: ... has no attribute 'merge_discovered_manifests'`, and the + new BDD scenario failed with `'- fixtures/minimal/Cargo.lock (lockfile)'` + absent from the CLI output. - [x] (2026-07-07 13:40Z) Stage C: implemented `merge_discovered_manifests` in `lading/commands/bump_lockfiles.py`, wired it into `bump._process_lockfiles` for both dry-run and live paths, extended the - autouse `stub_lockfile_regeneration` fixture, and updated the - `lockfile.py` call-graph docstring. Full suite green: 683 passed, 62 - snapshots passed (no snapshot content changed). + autouse `stub_lockfile_regeneration` fixture, and updated the `lockfile.py` + call-graph docstring. Full suite green: 683 passed, 62 snapshots passed (no + snapshot content changed). - [x] (2026-07-07 13:50Z) Side quest: restored the `make typecheck` gate — ty 0.0.8 (unpinned, installed by CI and locally via `uv tool install ty`) - flagged six pre-existing diagnostics on the clean tree. Committed - separately as "Restore a passing typecheck gate under ty 0.0.8". -- [ ] Stage D: documentation, pre-flight message wording, snapshot updates, - full quality gates, CodeRabbit review. + flagged six pre-existing diagnostics on the clean tree. Committed separately + as "Restore a passing typecheck gate under ty 0.0.8". +- [x] (2026-07-08 00:20Z) CodeRabbit review of Stages B+C: `coderabbit + review --agent` completed with zero findings. +- [x] (2026-07-08 00:40Z) Stage D: reworded the publish pre-flight stale + message (it no longer blames `lading bump`), regenerated the affected + syrupy snapshot after reviewing the diff, and updated + `docs/users-guide.md` (bump discovery paragraph, quoted pre-flight + message, `lockfile_manifests` and `rebuild_lockfiles` key descriptions, + regeneration paragraph) and `docs/developers-guide.md` (bump_lockfiles + module description, lockfile helpers section). All gates green: 683 + tests, lint 10.00/10, formatting, typecheck, markdown, nixie. ## Surprises & discoveries @@ -155,26 +164,26 @@ fixture lockfile and seeing that lockfile listed in the bump output with a `_mock_cargo_metadata` in `tests/bdd/steps/metadata_fixtures.py` and the "workspace has tracked Cargo.lock files" given step both stub `git ls-files "**/Cargo.lock" "Cargo.lock"` even though nothing in bump - invoked it. Evidence: the stubs existed before this change and cmd-mox - stubs are non-strict, so they sat unused. Impact: the existing lockfile - scenarios worked unchanged once discovery was wired in; only the new - nested-lockfile scenario needed a new given step. + invoked it. Evidence: the stubs existed before this change and cmd-mox stubs + are non-strict, so they sat unused. Impact: the existing lockfile scenarios + worked unchanged once discovery was wired in; only the new nested-lockfile + scenario needed a new given step. - Observation: cmd-mox registers one `CommandDouble` per command name and dispatches stubs by name alone (`controller._make_response`), so a second `stub("cargo::update")` re-configures the first rather than adding an argument-matched alternative. Evidence: `cmd_mox/controller.py::_get_double` - returns the existing double. Impact: the nested-lockfile given step - registers `cargo::update` without `with_args` so one response serves both - the root and nested manifest invocations. + returns the existing double. Impact: the nested-lockfile given step registers + `cargo::update` without `with_args` so one response serves both the root and + nested manifest invocations. - Observation: `uv tool install ty` now resolves to ty 0.0.8, which fails `make typecheck` on the clean tree with six diagnostics in `lading/commands/bump_toml.py` and `lading/commands/publish_index_check.py`. Evidence: `git stash && make typecheck` reproduced all six without this - branch's changes; a scratch probe confirmed ty 0.0.8 does not narrow - bindings after calls to `NoReturn` helpers but does honour `NoReturn` for + branch's changes; a scratch probe confirmed ty 0.0.8 does not narrow bindings + after calls to `NoReturn` helpers but does honour `NoReturn` for reachability. Impact: fixed ahead of the feature commit (if/elif/else - restructure plus one `type: ignore[index]` mirroring an existing comment) - so the gate is green before CodeRabbit review. + restructure plus one `type: ignore[index]` mirroring an existing comment) so + the gate is green before CodeRabbit review. ## Decision log @@ -198,19 +207,18 @@ fixture lockfile and seeing that lockfile listed in the bump output with a bump command. Date/Author: 2026-07-07, planning session. - Decision: land the Stage B red tests without interim strict-xfail markers, - committing them together with the Stage C implementation. - Rationale: the red failures were observed and transcribed directly (the - plan's validation evidence), and the combined commit keeps the suite green - at every commit boundary as required by the repository's gating rules; a - strict-xfail round-trip would have added churn without extra proof. - Date/Author: 2026-07-07, implementation session. + committing them together with the Stage C implementation. Rationale: the red + failures were observed and transcribed directly (the plan's validation + evidence), and the combined commit keeps the suite green at every commit + boundary as required by the repository's gating rules; a strict-xfail + round-trip would have added churn without extra proof. Date/Author: + 2026-07-07, implementation session. - Decision: fix the pre-existing ty 0.0.8 typecheck failures in a separate - commit on this branch rather than ignoring them or pinning ty. - Rationale: CI installs ty unpinned (`uv tool install ty` in - `.github/workflows/ci.yml`), so main's next CI run would fail regardless; - the gates must pass before CodeRabbit review; and the fixes are small, - behaviour-preserving restructures. Pinning would hide the drift rather - than resolve it. + commit on this branch rather than ignoring them or pinning ty. Rationale: CI + installs ty unpinned (`uv tool install ty` in `.github/workflows/ci.yml`), so + main's next CI run would fail regardless; the gates must pass before + CodeRabbit review; and the fixes are small, behaviour-preserving + restructures. Pinning would hide the drift rather than resolve it. Date/Author: 2026-07-07, implementation session. ## Outcomes & retrospective diff --git a/docs/users-guide.md b/docs/users-guide.md index a6235fef..53ed671e 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -86,14 +86,16 @@ lading bump 1.2.3 --dry-run After updating manifest versions, `lading bump` automatically refreshes any git-tracked `Cargo.lock` files (excluding those under `target/`) that have an -adjacent `Cargo.toml`. The result header counts each changed category, e.g. "N -manifest(s)", "N documentation file(s)", "N readme file(s)", "N lockfile(s)", -joined with Oxford-comma grammar. In the per-file body list, manifest paths -carry no suffix; the other categories carry a parenthetical suffix: -`(documentation)` for Markdown docs whose TOML code fences were updated, -`(readme)` for crate READMEs adopted from the workspace README, and -`(lockfile)` for regenerated `Cargo.lock` files. In dry-run mode, every file is -listed, but none are modified. +adjacent `Cargo.toml`, plus any additional manifests listed in +`bump.lockfile_manifests` for lockfiles that git does not track. The result +header counts each changed category, e.g. "N manifest(s)", "N documentation +file(s)", "N readme file(s)", "N lockfile(s)", joined with Oxford-comma +grammar. In the per-file body list, manifest paths carry no suffix; the other +categories carry a parenthetical suffix: `(documentation)` for Markdown docs +whose TOML code fences were updated, `(readme)` for crate READMEs adopted from +the workspace README, and `(lockfile)` for regenerated `Cargo.lock` files. In +dry-run mode, every file is listed, but none are modified (lockfile discovery +still runs `git ls-files`, which is read-only). Where a member crate sets `readme.workspace = true`, `lading bump` also adopts the workspace `README.md` into that crate's directory and rewrites relative @@ -115,15 +117,17 @@ lading publish Before running `cargo check` and `cargo test`, `lading publish` validates that all git-tracked `Cargo.lock` files are fresh under `--locked` mode. It probes -every tracked lockfile rather than stopping at the first stale one — for -example after a `lading bump` that regenerated one or more nested workspace -lockfiles — so a single run reports the whole workspace. If any lockfile is -stale, the command exits with code 1 and lists each stale lockfile alongside -its own repair command: +every tracked lockfile rather than stopping at the first stale one, so a single +run reports the whole workspace. If any lockfile is stale — for example after +manifest edits made without `lading bump`, or after a bump run with +`--no-rebuild-lockfiles` — the command exits with code 1 and lists each stale +lockfile alongside its own repair command: -```plaintext +```text Tracked Cargo.lock files are stale after manifest version changes. -This commonly happens after running `lading bump`; repair each stale lockfile directly: +This can happen after manifest edits made without `lading bump`, or after +running bump with `--no-rebuild-lockfiles`; repair each stale lockfile +directly: - /Cargo.lock cargo generate-lockfile --manifest-path /Cargo.toml - /Cargo.lock @@ -278,25 +282,29 @@ stderr_tail_lines = 40 manifest updates. - `lockfile_manifests`: array of strings, default `[]`. Additional `Cargo.toml` manifests whose adjacent `Cargo.lock` files should be - regenerated after `lading bump`. The workspace root `Cargo.toml` is always - included and should not be listed. + regenerated after `lading bump`. Git-tracked lockfiles are discovered and + regenerated automatically, so this key is only needed for lockfiles that git + does not track (for example, generated fixtures listed in `.gitignore`). The + workspace root `Cargo.toml` is always included and should not be listed. - `rebuild_lockfiles`: boolean, default `true`. Controls whether `lading bump` - regenerates the workspace lockfile and configured nested lockfiles after - manifest updates. Pass `--no-rebuild-lockfiles` to skip regeneration for a - single run, or `--rebuild-lockfiles` to force regeneration when - `rebuild_lockfiles` is configured as `false`. + regenerates the workspace lockfile, discovered tracked lockfiles, and + configured nested lockfiles after manifest updates. Pass + `--no-rebuild-lockfiles` to skip regeneration for a single run, or + `--rebuild-lockfiles` to force regeneration when `rebuild_lockfiles` is + configured as `false`. Lockfile regeneration runs -`cargo update --workspace --manifest-path ` for the workspace root -and each configured nested manifest. This updates workspace package entries -while avoiding a full transitive dependency refresh. - -Regeneration is not atomic, and `lading bump` attempts every configured -manifest rather than stopping at the first Cargo failure. Manifest versions are -written before any lockfile is refreshed, so a failure leaves the workspace -inconsistent: the manifests carry the new version but the affected lockfiles do -not. When several lockfiles are regenerated, `lading` raises one aggregated -error that lists every failed manifest with the exact repair command to run: +`cargo update --workspace --manifest-path ` for the workspace root, +each configured nested manifest, and each manifest adjacent to a discovered +git-tracked lockfile. This updates workspace package entries while avoiding a +full transitive dependency refresh. + +Regeneration is not atomic, and `lading bump` attempts every manifest rather +than stopping at the first Cargo failure. Manifest versions are written before +any lockfile is refreshed, so a failure leaves the workspace inconsistent: the +manifests carry the new version but the affected lockfiles do not. When several +lockfiles are regenerated, `lading` raises one aggregated error that lists every +failed manifest with the exact repair command to run: ```plaintext Cargo lockfile regeneration failed for 2 manifest(s). Manifests already carry the new version, so the workspace is inconsistent until each lockfile below is repaired: diff --git a/lading/commands/publish_preflight.py b/lading/commands/publish_preflight.py index 3ccb862d..4f3a5ed5 100644 --- a/lading/commands/publish_preflight.py +++ b/lading/commands/publish_preflight.py @@ -240,7 +240,8 @@ def _build_stale_lockfile_message(stale_lockfiles: list[Path]) -> str: lines = [ "Tracked Cargo.lock files are stale after manifest version changes.", ( - "This commonly happens after running `lading bump`; repair each " + "This can happen after manifest edits made without `lading bump`, " + "or after running bump with `--no-rebuild-lockfiles`; repair each " "stale lockfile directly:" ), ] diff --git a/tests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambr b/tests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambr index 7536f7b0..361fa8bb 100644 --- a/tests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambr +++ b/tests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambr @@ -2,7 +2,7 @@ # name: test_validate_lockfile_freshness_error_snapshot ''' Tracked Cargo.lock files are stale after manifest version changes. - This commonly happens after running `lading bump`; repair each stale lockfile directly: + This can happen after manifest edits made without `lading bump`, or after running bump with `--no-rebuild-lockfiles`; repair each stale lockfile directly: - /workspace root/Cargo.lock cargo generate-lockfile --manifest-path '/workspace root/Cargo.toml' - /workspace root/tests/ui_lints/Cargo.lock From 7e1d7f63b7e360ec40a118b15152b90d426875d6 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Wed, 8 Jul 2026 00:16:46 +0100 Subject: [PATCH 05/16] Complete the regenerate-lockfiles ExecPlan Record the end-to-end acceptance run (real git and cargo against the prototype workspace: nested lockfile discovered, refreshed, and fresh under `--locked`), the versioned-path-dependency edge case surfaced by that run, both CodeRabbit reviews (zero findings), the retrospective, and the follow-up candidates. Mark the plan COMPLETE. --- docs/execplans/regenerate-lockfiles.md | 76 ++++++++++++++++++++++---- 1 file changed, 65 insertions(+), 11 deletions(-) diff --git a/docs/execplans/regenerate-lockfiles.md b/docs/execplans/regenerate-lockfiles.md index eddbacdc..6e8f94e6 100644 --- a/docs/execplans/regenerate-lockfiles.md +++ b/docs/execplans/regenerate-lockfiles.md @@ -4,7 +4,7 @@ This ExecPlan (execution plan) is a living document. The sections `Constraints`, `Tolerances`, `Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work proceeds. -Status: IN PROGRESS +Status: COMPLETE ## Purpose / big picture @@ -137,16 +137,23 @@ fixture lockfile and seeing that lockfile listed in the bump output with a ty 0.0.8 (unpinned, installed by CI and locally via `uv tool install ty`) flagged six pre-existing diagnostics on the clean tree. Committed separately as "Restore a passing typecheck gate under ty 0.0.8". -- [x] (2026-07-08 00:20Z) CodeRabbit review of Stages B+C: `coderabbit - review --agent` completed with zero findings. +- [x] (2026-07-08 00:20Z) CodeRabbit review of Stages B+C: + `coderabbit review --agent` completed with zero findings. - [x] (2026-07-08 00:40Z) Stage D: reworded the publish pre-flight stale - message (it no longer blames `lading bump`), regenerated the affected - syrupy snapshot after reviewing the diff, and updated - `docs/users-guide.md` (bump discovery paragraph, quoted pre-flight - message, `lockfile_manifests` and `rebuild_lockfiles` key descriptions, - regeneration paragraph) and `docs/developers-guide.md` (bump_lockfiles - module description, lockfile helpers section). All gates green: 683 - tests, lint 10.00/10, formatting, typecheck, markdown, nixie. + message (it no longer blames `lading bump`), regenerated the affected syrupy + snapshot after reviewing the diff, and updated `docs/users-guide.md` (bump + discovery paragraph, quoted pre-flight message, `lockfile_manifests` and + `rebuild_lockfiles` key descriptions, regeneration paragraph) and + `docs/developers-guide.md` (bump_lockfiles module description, lockfile + helpers section). All gates green: 683 tests, lint 10.00/10, formatting, + typecheck, markdown, nixie. +- [x] (2026-07-08 01:00Z) CodeRabbit review of Stage D: zero findings. +- [x] (2026-07-08 01:20Z) End-to-end acceptance run against the Stage A + prototype with the real CLI, git, and cargo: discovery reported two tracked + lockfiles, bump output listed `- fixtures/minimal/Cargo.lock (lockfile)`, the + nested lockfile recorded the new version, and `cargo metadata --locked` + exited 0 afterwards. Also surfaced the versioned-path-dependency edge + recorded under `Surprises & discoveries`. ## Surprises & discoveries @@ -175,6 +182,20 @@ fixture lockfile and seeing that lockfile listed in the bump output with a returns the existing double. Impact: the nested-lockfile given step registers `cargo::update` without `with_args` so one response serves both the root and nested manifest invocations. +- Observation: a real end-to-end run surfaced an edge the mocked tests cannot: + `lading bump` does not rewrite manifests of non-member nested packages, so a + nested fixture that pins a *versioned* path dependency on a bumped crate + (`alpha = { path = ..., version = "0.1.0" }`) makes + `cargo update --workspace` fail at bump time with cargo's clear "failed to + select a version for the requirement" error. Evidence: prototype run of the + real CLI against the Stage A repository with a versioned path dependency; the + same repository with a path-only dependency (the common fixture pattern) + bumps cleanly end-to-end. Impact: acceptable — such a repository was already + broken at publish time (the freshness probe fails with the same cargo error), + and the failure now surfaces earlier with an actionable message; + `--no-rebuild-lockfiles` remains the escape hatch. Rewriting dependency + requirements in non-member manifests is a possible future enhancement, out of + scope here. - Observation: `uv tool install ty` now resolves to ty 0.0.8, which fails `make typecheck` on the clean tree with six diagnostics in `lading/commands/bump_toml.py` and `lading/commands/publish_index_check.py`. @@ -223,7 +244,32 @@ fixture lockfile and seeing that lockfile listed in the bump output with a ## Outcomes & retrospective -To be completed as milestones land. +Delivered as planned. `lading bump` now discovers git-tracked `Cargo.lock` +files with the same helper the publish pre-flight uses and regenerates them +alongside the root and configured lockfiles, in both live and dry-run modes. +The publish pre-flight's stale-lockfile message no longer blames bump. The +acceptance behaviour was verified twice: through the new BDD scenario (mocked +git/cargo) and through a real CLI run against the Stage A prototype repository, +where the nested lockfile was discovered, refreshed to the new version, and +passed `cargo metadata --locked` afterwards (exit 0). + +Deviations from the original plan, all recorded in the decision log: the +strict-xfail round-trip was replaced by directly observed red failures; six +pre-existing ty 0.0.8 typecheck diagnostics were fixed in a separate commit so +the gates could pass; and the interim mdformat reflow of the developers' guide +landed as its own commit. + +Lessons: (1) mocked BDD scenarios validated the wiring but only the real +end-to-end run exposed the versioned-path-dependency edge — keep a live +prototype exercise in plans that change subprocess behaviour; (2) the test +infrastructure had anticipated this feature (unused `git ls-files` stubs), +which suggests checking fixture stubs for "future intent" during orientation; +(3) unpinned typecheckers (`uv tool install ty`) make gate drift a recurring +cost — pinning with a scheduled bump would be a worthwhile follow-up for the +repository. + +Follow-up candidates (not in scope): rewrite version requirements in non-member +nested manifests that depend on bumped crates; pin `ty` in CI and the Makefile. ## Context and orientation @@ -527,3 +573,11 @@ def merge_discovered_manifests( `lading/commands/bump.py::_process_lockfiles` calls it once and passes the result to the existing `resolve_lockfile_paths` (dry-run) and `regenerate_lockfiles` (live) helpers, whose signatures do not change. + +## Revision note + +2026-07-08: implementation complete. Progress, surprises, decisions, and the +retrospective were updated throughout Stages A–D; the final revision records +the end-to-end acceptance run, the versioned-path-dependency edge case, and the +follow-up candidates (non-member manifest rewriting; pinning ty). Status moved +to COMPLETE. No further work remains on this plan. From b9687e1f7f7181a3d3d4d124e1f0885dae7d5ff0 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Wed, 8 Jul 2026 00:27:32 +0100 Subject: [PATCH 06/16] Pin ty 0.0.56 in the Makefile and stop installing it in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `make typecheck` previously ran whichever ty happened to be on PATH, and CI installed ty unpinned with `uv tool install`. ty is pre-1.0 and its diagnostics shift between releases, so the typecheck gate could break (or silently loosen) without any code change — locally, a stale ty 0.0.8 flagged six diagnostics that the current release does not. Adopt the latest release by pinning `TY_VERSION ?= 0.0.56` in the Makefile, mirroring the existing `RUFF_VERSION` pattern: the `typecheck` target now invokes `uv tool run --from ty==$(TY_VERSION)`, so no separate installation is needed and CI's tool-install step drops ty. Document the variable (and the bump-deliberately policy) in the developers' guide. ty 0.0.56 passes with no new diagnostics. --- .github/workflows/ci.yml | 4 ++-- Makefile | 11 ++++++----- docs/developers-guide.md | 6 ++++++ docs/execplans/regenerate-lockfiles.md | 6 ++++++ 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9fa282a6..d2a458f3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,8 +28,8 @@ jobs: run: | for tool in mbake; do uv tool install ${tool}; done # ty is not installed here: `make typecheck` runs it via - # `uv tool run ty@$(TY_VERSION)`. TY_VERSION in the Makefile is the - # sole ty version declaration; update it there. + # `uv tool run --from ty==$(TY_VERSION) ty`. TY_VERSION in the + # Makefile is the sole ty version declaration; update it there. # Pin ruff to the version the Makefile invokes (RUFF_VERSION) so the # linter behaves identically across the uv tool, the synced # virtualenv, and developer PATH; bump both sites together. diff --git a/Makefile b/Makefile index 4f5ec024..a0e468e7 100644 --- a/Makefile +++ b/Makefile @@ -10,11 +10,12 @@ UV ?= $(shell command -v uv 2>/dev/null || printf '%s/.local/bin/uv' "$$HOME") RUFF_VERSION ?= 0.15.12 RUFF ?= $(UV) tool run --from ruff==$(RUFF_VERSION) ruff TYPOS_VERSION ?= 1.48.0 -# Pin ty. `make typecheck` invokes it via `uv tool run ty@$(TY_VERSION)`, so -# TY_VERSION is the sole ty version declaration (CI runs `make typecheck` and -# installs no separate ty). Bump it here to move local and CI checks together. -TY_VERSION ?= 0.0.32 -TY ?= $(UV) tool run ty@$(TY_VERSION) +# Pin ty so `make` and CI invoke the same typechecker release. ty is +# pre-1.0 and diagnostics shift between releases, so an unpinned install +# breaks the typecheck gate without any code change. Bump deliberately and +# fix new diagnostics in the same commit. +TY_VERSION ?= 0.0.56 +TY ?= $(UV) tool run --from ty==$(TY_VERSION) ty UV_ENV = UV_CACHE_DIR=.uv-cache UV_TOOL_DIR=.uv-tools TOOLS = $(MDFORMAT_ALL) $(MDLINT) $(NIXIE) $(UV) PY_SOURCES := $(sort $(shell find lading scripts -type f -name '*.py' -print)) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index e8f48323..37b2a3b4 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -66,6 +66,12 @@ The relevant Makefile variables are: - `RUFF` — the pinned Ruff command (`uv tool run --from ruff==$(RUFF_VERSION) ruff`) that the `fmt`, `check-fmt`, and `lint` targets invoke. +- `TY_VERSION` — pinned ty version used by `make typecheck`; defaults to + `0.0.56`. ty is pre-1.0 and diagnostics shift between releases, so bump it + deliberately and fix any new diagnostics in the same commit. CI does not + install ty separately; it runs whatever `TY_VERSION` pins. +- `TY` — the pinned ty command (`uv tool run --from ty==$(TY_VERSION) ty`) + that the `typecheck` target invokes. - `PYLINT_PYTHON` — Python executable used by `uv tool run`; defaults to `pypy`. - `PYLINT_TARGETS` — directories passed to Pylint; defaults to `lading scripts tests`. diff --git a/docs/execplans/regenerate-lockfiles.md b/docs/execplans/regenerate-lockfiles.md index 6e8f94e6..25196c52 100644 --- a/docs/execplans/regenerate-lockfiles.md +++ b/docs/execplans/regenerate-lockfiles.md @@ -581,3 +581,9 @@ retrospective were updated throughout Stages A–D; the final revision records the end-to-end acceptance run, the versioned-path-dependency edge case, and the follow-up candidates (non-member manifest rewriting; pinning ty). Status moved to COMPLETE. No further work remains on this plan. + +2026-07-08 (later): the ty-pinning follow-up was completed on this branch at +the user's request — `TY_VERSION ?= 0.0.56` in the Makefile (invoked via +`uv tool run --from ty==$(TY_VERSION)`), CI no longer installs ty separately, +and ty 0.0.56 passes with no new diagnostics. The non-member manifest rewriting +follow-up remains open. From 2f350f5e51d697e6047b59773e60448c0fbb5c55 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Wed, 8 Jul 2026 18:30:30 +0100 Subject: [PATCH 07/16] Index the execution plan in the documentation contents `docs/contents.md` is the documented entry point for the documentation set, but the new plan under `docs/execplans/` was not listed, so readers following the navigation path would not discover it. Add an "Execution plans" section to the index and record the `docs/execplans/` placement convention in the repository layout guide. --- docs/contents.md | 9 +++++++++ docs/repository-layout.md | 2 ++ 2 files changed, 11 insertions(+) diff --git a/docs/contents.md b/docs/contents.md index 082a095f..4893edff 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -20,6 +20,14 @@ looking for project guidance, then follow the link that matches the task. - [Roadmap](roadmap.md) - phased delivery plan and tracked implementation tasks. +## Execution plans + +Living delivery documents under `execplans/`, one per branch, recording the +plan, progress, decisions, and retrospective for a change. + +- [Regenerate discovered nested lockfiles][execplan-regenerate-lockfiles] - + completed plan for discovery-driven lockfile regeneration in the bump command. + ## Decision records - [ADR-003: Use three-tier Python linting][adr-003] - accepted linting policy @@ -38,3 +46,4 @@ looking for project guidance, then follow the link that matches the task. [adr-003]: adr/003-three-tier-python-linting.md [adr-004]: adr/004-in-process-metrics-backend.md +[execplan-regenerate-lockfiles]: execplans/regenerate-lockfiles.md diff --git a/docs/repository-layout.md b/docs/repository-layout.md index 9a027314..75ef76ad 100644 --- a/docs/repository-layout.md +++ b/docs/repository-layout.md @@ -97,6 +97,8 @@ Documentation belongs in `docs/` unless it is a short entry point such as - Use `docs/lading-design.md` for architecture, constraints, and design rationale. - Use `docs/roadmap.md` for delivery planning and task tracking. +- Use `docs/execplans/` for per-branch execution plans (living + design-and-delivery documents), named after the branch they guide. - Use `docs/documentation-style-guide.md` as the formatting and document-type authority. From 79f6f15d7f3a50448dddc21c8f95d226d61c26bb Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 14 Jul 2026 00:19:51 +0200 Subject: [PATCH 08/16] Address lockfile regeneration review feedback Clarify live and dry-run lockfile behaviour and document non-Git configuration requirements. Simplify ty narrowing now supported by the pinned checker, make successful None returns explicit, and add property coverage for merge ordering, resolved-path de-duplication, and non-Git fallback behaviour. Refresh the generated spelling dictionary entries carried in the working tree. --- docs/developers-guide.md | 4 +- docs/execplans/regenerate-lockfiles.md | 19 ++--- docs/users-guide.md | 12 +-- lading/commands/publish_index_check.py | 39 ++++----- tests/unit/test_bump_lockfiles.py | 106 ++++++++++++++++++++++++- 5 files changed, 135 insertions(+), 45 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 37b2a3b4..eff93181 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -213,8 +213,8 @@ spans are preserved verbatim. after manifest changes. `merge_discovered_manifests` unions the configured `bump.lockfile_manifests` entries with manifests implied by git-tracked `Cargo.lock` files (reusing -`lading.commands.lockfile .discover_tracked_lockfiles`); configured entries -keep their order and discovered entries follow in sorted order. +`lading.commands.lockfile.discover_tracked_lockfiles`); configured entries keep +their order and discovered entries follow in sorted order. `regenerate_lockfiles` always includes the workspace root `Cargo.toml`, validates nested manifests before invoking Cargo, and de-duplicates resolved manifest paths. diff --git a/docs/execplans/regenerate-lockfiles.md b/docs/execplans/regenerate-lockfiles.md index 25196c52..936ba713 100644 --- a/docs/execplans/regenerate-lockfiles.md +++ b/docs/execplans/regenerate-lockfiles.md @@ -46,7 +46,7 @@ fixture lockfile and seeing that lockfile listed in the bump output with a ## Constraints - Do not change the `CommandRunner` protocol - (`lading/runtime.py`) or the signatures of + (`lading/runtime/runner.py`) or the signatures of `lading.commands.lockfile.discover_tracked_lockfiles` and `lading.commands.lockfile.validate_lockfile_freshness`. - The existing `bump.lockfile_manifests` and `bump.rebuild_lockfiles` @@ -245,13 +245,14 @@ fixture lockfile and seeing that lockfile listed in the bump output with a ## Outcomes & retrospective Delivered as planned. `lading bump` now discovers git-tracked `Cargo.lock` -files with the same helper the publish pre-flight uses and regenerates them -alongside the root and configured lockfiles, in both live and dry-run modes. -The publish pre-flight's stale-lockfile message no longer blames bump. The -acceptance behaviour was verified twice: through the new BDD scenario (mocked -git/cargo) and through a real CLI run against the Stage A prototype repository, -where the nested lockfile was discovered, refreshed to the new version, and -passed `cargo metadata --locked` afterwards (exit 0). +files with the same helper the publish pre-flight uses. Live mode regenerates +the discovered lockfiles alongside the root and configured lockfiles; dry-run +discovers and reports that same set without modifying files. The publish +pre-flight's stale-lockfile message no longer blames bump. The acceptance +behaviour was verified twice: through the new BDD scenario (mocked git/cargo) +and through a real CLI run against the Stage A prototype repository, where the +nested lockfile was discovered, refreshed to the new version, and passed +`cargo metadata --locked` afterwards (exit 0). Deviations from the original plan, all recorded in the decision log: the strict-xfail round-trip was replaced by directly observed red failures; six @@ -269,7 +270,7 @@ cost — pinning with a scheduled bump would be a worthwhile follow-up for the repository. Follow-up candidates (not in scope): rewrite version requirements in non-member -nested manifests that depend on bumped crates; pin `ty` in CI and the Makefile. +nested manifests that depend on bumped crates. ## Context and orientation diff --git a/docs/users-guide.md b/docs/users-guide.md index 53ed671e..48e76978 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -283,9 +283,11 @@ stderr_tail_lines = 40 - `lockfile_manifests`: array of strings, default `[]`. Additional `Cargo.toml` manifests whose adjacent `Cargo.lock` files should be regenerated after `lading bump`. Git-tracked lockfiles are discovered and - regenerated automatically, so this key is only needed for lockfiles that git - does not track (for example, generated fixtures listed in `.gitignore`). The - workspace root `Cargo.toml` is always included and should not be listed. + regenerated automatically. Configured manifests are needed for lockfiles that + git does not track (for example, generated fixtures listed in `.gitignore`) + and for nested lockfiles when the workspace is outside a Git repository, + where discovery returns no tracked manifests. The workspace root `Cargo.toml` + is always included and should not be listed. - `rebuild_lockfiles`: boolean, default `true`. Controls whether `lading bump` regenerates the workspace lockfile, discovered tracked lockfiles, and configured nested lockfiles after manifest updates. Pass @@ -303,8 +305,8 @@ Regeneration is not atomic, and `lading bump` attempts every manifest rather than stopping at the first Cargo failure. Manifest versions are written before any lockfile is refreshed, so a failure leaves the workspace inconsistent: the manifests carry the new version but the affected lockfiles do not. When several -lockfiles are regenerated, `lading` raises one aggregated error that lists every -failed manifest with the exact repair command to run: +lockfiles are regenerated, `lading` raises one aggregated error that lists +every failed manifest with the exact repair command to run: ```plaintext Cargo lockfile regeneration failed for 2 manifest(s). Manifests already carry the new version, so the workspace is inconsistent until each lockfile below is repaired: diff --git a/lading/commands/publish_index_check.py b/lading/commands/publish_index_check.py index 87f529fd..5e6fa350 100644 --- a/lading/commands/publish_index_check.py +++ b/lading/commands/publish_index_check.py @@ -287,19 +287,13 @@ def _validate_dependency_placement( missing_name, missing_index if missing_index is not None else "", ) - # An if/elif chain (rather than sequential guards) narrows missing_index - # to int in later branches: ty 0.0.8 does not narrow bindings after calls - # to NoReturn helpers, only via the branch conditions themselves. if missing_index is None: _raise_out_of_plan_dependency(context, missing_name=missing_name) - elif missing_index == current_index: + if missing_index == current_index: _raise_self_dependency(context, missing_name=missing_name) - elif missing_index > current_index: + if missing_index > current_index: _raise_out_of_order_dependency(context, missing_name=missing_name) - else: - return _DependencyPlacement( - current_index, missing_index, missing_canonical_name - ) + return _DependencyPlacement(current_index, missing_index, missing_canonical_name) def _emit_downgrade_success( @@ -372,15 +366,12 @@ def _handle_index_missing_version( ) missing_name = failure.missing_dependency_name - # if/else (rather than a guard clause) narrows missing_name to str for - # the downgrade path: ty 0.0.8 does not narrow bindings after calls to - # NoReturn helpers, only via the branch conditions themselves. - if missing_name is None: - _raise_name_extraction_failure(context) - else: + if missing_name is not None: _downgrade_or_raise( failure, context=context, handling=handling, missing_name=missing_name ) + return + _raise_name_extraction_failure(context) def _downgrade_or_raise( @@ -403,14 +394,12 @@ def _downgrade_or_raise( else "will downgrade to warning", ) - if not handling.options.allow_unpublished_workspace_deps: - _raise_unpublished_dependency_override_required( - context, missing_name=missing_name + if handling.options.allow_unpublished_workspace_deps: + _emit_downgrade_success( + handling, + failure, + missing_name=missing_name, + placement=placement, ) - - _emit_downgrade_success( - handling, - failure, - missing_name=missing_name, - placement=placement, - ) + return + _raise_unpublished_dependency_override_required(context, missing_name=missing_name) diff --git a/tests/unit/test_bump_lockfiles.py b/tests/unit/test_bump_lockfiles.py index 6e8982f3..b85b26c4 100644 --- a/tests/unit/test_bump_lockfiles.py +++ b/tests/unit/test_bump_lockfiles.py @@ -58,6 +58,39 @@ def _write_manifest(directory: Path) -> None: encoding="utf-8", ) +@st.composite +def _manifest_merge_cases( + draw: st.DrawFn, +) -> tuple[tuple[str, ...], tuple[str, ...]]: + """Generate configured manifests and arbitrarily ordered tracked lockfiles.""" + directories = draw( + st.lists( + st.from_regex(r"[a-z][a-z0-9]{0,7}", fullmatch=True), + min_size=1, + max_size=8, + unique=True, + ) + ) + discovered_directories = draw(st.permutations(directories)) + configured_directories = draw(st.lists(st.sampled_from(directories), unique=True)) + configured_directories = draw(st.permutations(configured_directories)) + dot_prefixes = draw( + st.lists( + st.booleans(), + min_size=len(configured_directories), + max_size=len(configured_directories), + ) + ) + configured = tuple( + f"{'./' if has_dot_prefix else ''}{directory}/Cargo.toml" + for directory, has_dot_prefix in zip( + configured_directories, dot_prefixes, strict=True + ) + ) + discovered = tuple( + f"{directory}/Cargo.lock" for directory in discovered_directories + ) + return configured, discovered def test_merge_discovered_manifests_appends_tracked_manifests( tmp_path: Path, ) -> None: @@ -83,13 +116,13 @@ def test_merge_discovered_manifests_appends_tracked_manifests( "crates/ui/Cargo.toml", "Cargo.toml", "fixtures/minimal/Cargo.toml", - ) + ), f"unexpected merged manifests: {merged!r}" assert runner.invocations == [ _Invocation( command=("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), cwd=tmp_path, ) - ] + ], f"unexpected git invocations: {runner.invocations!r}" def test_merge_discovered_manifests_deduplicates_configured_forms( tmp_path: Path, @@ -104,7 +137,9 @@ def test_merge_discovered_manifests_deduplicates_configured_forms( runner=runner, ) - assert merged == ("./fixtures/minimal/Cargo.toml",) + assert merged == ("./fixtures/minimal/Cargo.toml",), ( + f"equivalent configured manifest was re-appended: {merged!r}" + ) def test_merge_discovered_manifests_returns_configured_outside_git( tmp_path: Path, @@ -124,7 +159,70 @@ def test_merge_discovered_manifests_returns_configured_outside_git( runner=runner, ) - assert merged == ("crates/ui/Cargo.toml",) + assert merged == ("crates/ui/Cargo.toml",), ( + f"non-git fallback changed configured manifests: {merged!r}" + ) + +@given(case=_manifest_merge_cases()) +@settings(max_examples=80, deadline=None) +def test_merge_discovered_manifests_preserves_order_and_deduplicates( + case: tuple[tuple[str, ...], tuple[str, ...]], +) -> None: + """Configured order wins while unique discoveries are appended sorted.""" + configured, discovered = case + with tempfile.TemporaryDirectory() as temporary_directory: + workspace_root = Path(temporary_directory) + for lockfile in discovered: + _write_manifest(workspace_root / Path(lockfile).parent) + runner = _RecordingRunner(result=(0, "\n".join(discovered) + "\n", "")) + + merged = bump_lockfiles.merge_discovered_manifests( + workspace_root, + configured, + runner=runner, + ) + + configured_paths = { + (workspace_root / manifest).resolve() for manifest in configured + } + expected_discovered = tuple( + sorted( + str(Path(lockfile).with_name("Cargo.toml")) + for lockfile in discovered + if (workspace_root / Path(lockfile).with_name("Cargo.toml")).resolve() + not in configured_paths + ) + ) + assert merged == configured + expected_discovered, ( + f"merge did not preserve configured order and sorted discovery: {merged!r}" + ) + assert len({ + (workspace_root / manifest).resolve() for manifest in merged + }) == len(merged), f"merge retained equivalent manifest paths: {merged!r}" + +@given( + configured=st.lists( + st.from_regex(r"(?:\./)?[a-z][a-z0-9]{0,7}/Cargo\.toml", fullmatch=True), + max_size=8, + ).map(tuple) +) +@settings(max_examples=50, deadline=None) +def test_merge_discovered_manifests_non_git_fallback_is_identity( + configured: tuple[str, ...], +) -> None: + """Non-git discovery preserves every configured entry and its order.""" + runner = _RecordingRunner(result=(128, "", "fatal: not a git repository")) + with tempfile.TemporaryDirectory() as temporary_directory: + merged = bump_lockfiles.merge_discovered_manifests( + Path(temporary_directory), + configured, + runner=runner, + ) + + assert merged == configured, f"non-git fallback changed input: {merged!r}" + assert len(runner.invocations) == 1, ( + f"non-git discovery should invoke git once: {runner.invocations!r}" + ) def test_regenerate_lockfiles_includes_workspace_manifest(tmp_path: Path) -> None: """The workspace root manifest should always be regenerated.""" runner = _RecordingRunner() From c2aaed3140c4bfc4f069564b1ef07a755ead3efd Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 14 Jul 2026 00:29:14 +0200 Subject: [PATCH 09/16] Preserve lockfile discovery behind repository port Adapt the rebased lockfile discovery behaviour to main's `LockfileRepository` boundary. Keep the bump domain independent of Git and Cargo execution while ensuring the concrete adapter discovers the same manifest set for dry-run projection and live regeneration. Record the post-plan architectural adjustment in the developer and design documentation. --- docs/developers-guide.md | 27 ++++++++++++++------------ docs/execplans/regenerate-lockfiles.md | 7 +++++++ docs/lading-design.md | 14 +++++++------ lading/commands/bump_lockfiles.py | 19 +++++++++++------- 4 files changed, 42 insertions(+), 25 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index eff93181..99ebef7e 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -269,11 +269,14 @@ the original cargo error rather than wrapped in the aggregate message. `lockfile_repository` field, a `bump_lockfiles.LockfileRepository` port introduced by issue #82: the bump domain never holds a raw command runner. When the field is `None`, bump uses `bump_lockfiles.CargoLockfileRepository`, the -cargo-backed adapter bound to the default subprocess runner; the CLI binds the -adapter to its selected runner. Tests inject a repository (or bind the adapter -to a recording runner) so lockfile commands can be observed without invoking -real Cargo processes. The port's scope is bump-side lockfile projection and -regeneration; publish-side discovery and validation go through the sibling +cargo- and git-backed adapter bound to the default subprocess runner; the CLI +binds the adapter to its selected runner. The adapter merges configured +manifests with manifests discovered from tracked lockfiles before either +projecting dry-run paths or regenerating live lockfiles. Tests inject a +repository (or bind the adapter to a recording runner) so lockfile commands can +be observed without invoking real processes. The port's scope is bump-side +lockfile projection and regeneration; publish-side discovery and validation go +through the sibling `lockfile.LockfileInspectionRepository` port (see the Lockfile helpers section below), so neither the bump nor the publish lockfile domain holds a raw `CommandRunner` (issue #82). @@ -380,14 +383,14 @@ Private helpers `_handle_git_ls_files_failure` and `_lockfiles_with_manifests` perform the error-handling and path-filtering passes respectively. Lockfile regeneration after `lading bump` is owned by -`lading.commands.bump_lockfiles.regenerate_lockfiles`, which runs -`cargo update --workspace` per merged manifest — -`bump_lockfiles.merge_discovered_manifests` unions the configured +`lading.commands.bump_lockfiles.CargoLockfileRepository`. The adapter uses +`bump_lockfiles.merge_discovered_manifests` to union the configured `bump.lockfile_manifests` entries with manifests implied by -`discover_tracked_lockfiles`. The two cargo strategies differ deliberately: -bump refreshes existing pinned versions in place after manifest rewrites, while -publish only probes freshness read-only via `cargo metadata --locked` and never -regenerates. +`discover_tracked_lockfiles`, then delegates to `regenerate_lockfiles`, which +runs `cargo update --workspace` per merged manifest. The two cargo strategies +differ deliberately: bump refreshes existing pinned versions in place after +manifest rewrites, while publish only probes freshness read-only via +`cargo metadata --locked` and never regenerates. `validate_lockfile_freshness(manifest_path, runner)` runs `cargo metadata --locked --manifest-path ... --format-version=1`. It returns a diff --git a/docs/execplans/regenerate-lockfiles.md b/docs/execplans/regenerate-lockfiles.md index 936ba713..0b31a064 100644 --- a/docs/execplans/regenerate-lockfiles.md +++ b/docs/execplans/regenerate-lockfiles.md @@ -588,3 +588,10 @@ the user's request — `TY_VERSION ?= 0.0.56` in the Makefile (invoked via `uv tool run --from ty==$(TY_VERSION)`), CI no longer installs ty separately, and ty 0.0.56 passes with no new diagnostics. The non-member manifest rewriting follow-up remains open. + +2026-07-14: rebasing onto `origin/main` incorporated issue #82's +`LockfileRepository` port. Discovery now occurs inside the +`CargoLockfileRepository` adapter before its dry-run projection or live +regeneration operation; `_process_lockfiles` remains dependent only on the +port. This preserves the discovery behaviour delivered by the plan while +keeping Git and Cargo execution outside the bump domain. diff --git a/docs/lading-design.md b/docs/lading-design.md index 55fb9300..cb782adc 100644 --- a/docs/lading-design.md +++ b/docs/lading-design.md @@ -378,12 +378,14 @@ lading bump [--dry-run] The bump domain reaches Cargo lockfile projection and regeneration through a `LockfileRepository` port defined in `lading.commands.bump_lockfiles`. -`CargoLockfileRepository` is the cargo-backed adapter; it is constructed with -an optional `CommandRunner` and delegates to the module-level helpers. -`BumpOptions.lockfile_repository` is the injection point; when `None`, bump -substitutes `CargoLockfileRepository` bound to the default subprocess runner, -and the CLI binds the adapter at the composition root. This keeps the bump -domain free of a raw `CommandRunner` (issue #82). +`CargoLockfileRepository` is the cargo- and git-backed adapter; it is +constructed with an optional `CommandRunner`, merges configured manifests with +manifests discovered from tracked lockfiles, and delegates projection and +regeneration to the module-level helpers. `BumpOptions.lockfile_repository` is +the injection point; when `None`, bump substitutes `CargoLockfileRepository` +bound to the default subprocess runner, and the CLI binds the adapter at the +composition root. This keeps the bump domain free of a raw `CommandRunner` +(issue #82). ## 4. `publish` Subcommand Design diff --git a/lading/commands/bump_lockfiles.py b/lading/commands/bump_lockfiles.py index e4fc73cb..e7664ce9 100644 --- a/lading/commands/bump_lockfiles.py +++ b/lading/commands/bump_lockfiles.py @@ -28,6 +28,7 @@ class LockfileRegenerationError(LadingError): """Raise when lockfile regeneration cannot validate or execute.""" + def merge_discovered_manifests( workspace_root: Path, lockfile_manifests: cabc.Sequence[str], @@ -83,6 +84,8 @@ def merge_discovered_manifests( seen_manifests.add(resolved) merged.append(relative_manifest) return tuple(merged) + + def resolve_lockfile_paths( workspace_root: Path, lockfile_manifests: cabc.Sequence[str], @@ -314,9 +317,9 @@ def resolve_lockfile_paths( workspace_root : Absolute path to the Cargo workspace root. lockfile_manifests : - Configured manifest paths relative to *workspace_root*. The - workspace-root ``Cargo.toml`` is always prepended and - de-duplicated. + Configured manifest paths relative to *workspace_root*. Manifests + implied by git-tracked lockfiles are merged before the + workspace-root ``Cargo.toml`` is prepended and de-duplicated. Returns ------- @@ -348,9 +351,9 @@ def regenerate_lockfiles( workspace_root : Absolute path to the Cargo workspace root. lockfile_manifests : - Configured manifest paths relative to *workspace_root*. The - workspace-root ``Cargo.toml`` is always prepended and - de-duplicated. + Configured manifest paths relative to *workspace_root*. Manifests + implied by git-tracked lockfiles are merged before the + workspace-root ``Cargo.toml`` is prepended and de-duplicated. Returns ------- @@ -371,7 +374,9 @@ def regenerate_lockfiles( merged_manifests = merge_discovered_manifests( workspace_root, lockfile_manifests, runner=self.runner ) - return regenerate_lockfiles(workspace_root, merged_manifests, runner=self.runner) + return regenerate_lockfiles( + workspace_root, merged_manifests, runner=self.runner + ) class LockfileRepository(typ.Protocol): From dedf53e91d2b7657f204a24d94b18a7eb75b2a14 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 11:25:32 +0200 Subject: [PATCH 10/16] Align lockfile ExecPlan with repository port Describe the current `LockfileRepository` boundary, shared lockfile discovery, and live versus dry-run behaviour. Label pre-pin ty evidence as historical and record the completed 0.0.56 pin unambiguously. Correct the remaining Oxford-spelling blocker surfaced by the documentation gate. --- docs/execplans/regenerate-lockfiles.md | 173 +++++++++++++------------ 1 file changed, 90 insertions(+), 83 deletions(-) diff --git a/docs/execplans/regenerate-lockfiles.md b/docs/execplans/regenerate-lockfiles.md index 0b31a064..937683d9 100644 --- a/docs/execplans/regenerate-lockfiles.md +++ b/docs/execplans/regenerate-lockfiles.md @@ -102,11 +102,11 @@ fixture lockfile and seeing that lockfile listed in the bump output with a message from `LockfileRegenerationError` names the manifest and exit code; `--no-rebuild-lockfiles` remains as an escape hatch. Documented in the users' guide update. -- Risk: dry-run currently makes no subprocess calls in `_process_lockfiles`; - after this change it runs `git ls-files` (read-only) to list what would be - regenerated. Severity: low. Likelihood: certain. Mitigation: `git ls-files` - mutates nothing. Tests pass a stub runner, so no real subprocess runs in the - unit suite. The behaviour is documented. +- Risk identified during planning: dry-run made no subprocess calls in + `_process_lockfiles`; the delivered adapter runs `git ls-files` (read-only) + to list what would be regenerated. Severity: low. Likelihood: certain. + Mitigation: `git ls-files` mutates nothing. Tests pass a stub runner, so no + real subprocess runs in the unit suite. The behaviour is documented. - Risk: bump output snapshots (`syrupy` `.ambr` files) change because more lockfiles are listed. Severity: low. Likelihood: high. Mitigation: regenerate snapshots deliberately with `--snapshot-update` only after eyeballing the new @@ -134,7 +134,7 @@ fixture lockfile and seeing that lockfile listed in the bump output with a call-graph docstring. Full suite green: 683 passed, 62 snapshots passed (no snapshot content changed). - [x] (2026-07-07 13:50Z) Side quest: restored the `make typecheck` gate — - ty 0.0.8 (unpinned, installed by CI and locally via `uv tool install ty`) + ty 0.0.8 (then installed without a version constraint by CI and locally) flagged six pre-existing diagnostics on the clean tree. Committed separately as "Restore a passing typecheck gate under ty 0.0.8". - [x] (2026-07-08 00:20Z) CodeRabbit review of Stages B+C: @@ -157,15 +157,15 @@ fixture lockfile and seeing that lockfile listed in the bump output with a ## Surprises & discoveries -- Observation: `docs/users-guide.md` already documents the desired behaviour - ("automatically refreshes any git-tracked `Cargo.lock` files") even though - the code only regenerates configured manifests. Evidence: - `docs/users-guide.md` bump section versus - `lading/commands/bump.py::_process_lockfiles`, which reads only - `context.configuration.bump.lockfile_manifests`. Impact: this change closes a +- Historical observation from before implementation: `docs/users-guide.md` + already documented the desired behaviour ("automatically refreshes any + git-tracked `Cargo.lock` files"), but the code regenerated only configured + manifests. Evidence at the time: `docs/users-guide.md` bump section versus + the pre-change `lading/commands/bump.py::_process_lockfiles`, which read only + `context.configuration.bump.lockfile_manifests`. Impact: this change closed a documented behaviour gap rather than adding new surface; the - `lockfile_manifests` doc text must be re-scoped to "manifests discovery - cannot see" (for example, lockfiles not tracked by git). + `lockfile_manifests` documentation was re-scoped to manifests that discovery + cannot see. - Observation: the BDD infrastructure already anticipated discovery in bump — `_mock_cargo_metadata` in `tests/bdd/steps/metadata_fixtures.py` and the @@ -196,15 +196,16 @@ fixture lockfile and seeing that lockfile listed in the bump output with a `--no-rebuild-lockfiles` remains the escape hatch. Rewriting dependency requirements in non-member manifests is a possible future enhancement, out of scope here. -- Observation: `uv tool install ty` now resolves to ty 0.0.8, which fails - `make typecheck` on the clean tree with six diagnostics in - `lading/commands/bump_toml.py` and `lading/commands/publish_index_check.py`. - Evidence: `git stash && make typecheck` reproduced all six without this - branch's changes; a scratch probe confirmed ty 0.0.8 does not narrow bindings - after calls to `NoReturn` helpers but does honour `NoReturn` for - reachability. Impact: fixed ahead of the feature commit (if/elif/else - restructure plus one `type: ignore[index]` mirroring an existing comment) so - the gate is green before CodeRabbit review. +- Historical observation from 2026-07-07: the then-unpinned ty installation + resolved to ty 0.0.8, which failed `make typecheck` on the clean tree with + six diagnostics in `lading/commands/bump_toml.py` and + `lading/commands/publish_index_check.py`. Evidence: + `git stash && make typecheck` reproduced all six without this branch's + changes; a scratch probe confirmed ty 0.0.8 does not narrow bindings after + calls to `NoReturn` helpers but does honour `NoReturn` for reachability. + Impact: fixed ahead of the feature commit (if/elif/else restructure plus one + `type: ignore[index]` mirroring an existing comment) so the gate is green + before CodeRabbit review. ## Decision log @@ -216,11 +217,12 @@ fixture lockfile and seeing that lockfile listed in the bump output with a global escape hatches. Fewer configuration keys, and bump/publish stay consistent. Date/Author: 2026-07-07, planning session. - Decision: keep `bump.lockfile_manifests` rather than deprecating it. - Rationale: it remains the only way to regenerate a lockfile that git does not - track (for example, generated fixtures ignored by `.gitignore`), and removing - a configuration key is a breaking change out of scope here. Date/Author: - 2026-07-07, planning session. -- Decision: perform the union of configured and discovered manifests inside + Rationale: it remains the only way to regenerate a lockfile that discovery + cannot find, including untracked lockfiles and nested lockfiles in a + workspace outside a Git repository. Removing a configuration key is a + breaking change out of scope here. Date/Author: 2026-07-07, planning session. +- Original implementation decision, later adapted during the 2026-07-14 + rebase: perform the union of configured and discovered manifests inside `lading/commands/bump_lockfiles.py` via a new public helper, and keep `bump._process_lockfiles` a thin caller. Rationale: `bump_lockfiles` already owns manifest validation and de-duplication (`_resolve_manifest_paths`); @@ -234,24 +236,24 @@ fixture lockfile and seeing that lockfile listed in the bump output with a boundary as required by the repository's gating rules; a strict-xfail round-trip would have added churn without extra proof. Date/Author: 2026-07-07, implementation session. -- Decision: fix the pre-existing ty 0.0.8 typecheck failures in a separate - commit on this branch rather than ignoring them or pinning ty. Rationale: CI - installs ty unpinned (`uv tool install ty` in `.github/workflows/ci.yml`), so - main's next CI run would fail regardless; the gates must pass before - CodeRabbit review; and the fixes are small, behaviour-preserving - restructures. Pinning would hide the drift rather than resolve it. - Date/Author: 2026-07-07, implementation session. +- Historical decision from 2026-07-07: fix the pre-existing ty 0.0.8 typecheck + failures in a separate commit before introducing a version pin. Rationale: CI + then installed ty without a version constraint, so main's next CI run would + fail regardless; the gates had to pass before CodeRabbit review; and the + fixes were small, behaviour-preserving restructures. A later revision on this + completed branch also pinned ty as described below. Date/Author: 2026-07-07, + implementation session. ## Outcomes & retrospective Delivered as planned. `lading bump` now discovers git-tracked `Cargo.lock` files with the same helper the publish pre-flight uses. Live mode regenerates -the discovered lockfiles alongside the root and configured lockfiles; dry-run -discovers and reports that same set without modifying files. The publish -pre-flight's stale-lockfile message no longer blames bump. The acceptance -behaviour was verified twice: through the new BDD scenario (mocked git/cargo) -and through a real CLI run against the Stage A prototype repository, where the -nested lockfile was discovered, refreshed to the new version, and passed +the discovered, workspace-root, and configured lockfiles; dry-run discovers and +reports that same set without modifying files. The publish pre-flight's +stale-lockfile message no longer blames bump. The acceptance behaviour was +verified twice: through the new BDD scenario (mocked git/cargo) and through a +real CLI run against the Stage A prototype repository, where the nested +lockfile was discovered, refreshed to the new version, and passed `cargo metadata --locked` afterwards (exit 0). Deviations from the original plan, all recorded in the decision log: the @@ -265,9 +267,10 @@ end-to-end run exposed the versioned-path-dependency edge — keep a live prototype exercise in plans that change subprocess behaviour; (2) the test infrastructure had anticipated this feature (unused `git ls-files` stubs), which suggests checking fixture stubs for "future intent" during orientation; -(3) unpinned typecheckers (`uv tool install ty`) make gate drift a recurring -cost — pinning with a scheduled bump would be a worthwhile follow-up for the -repository. +(3) unpinned typecheckers caused avoidable gate drift. The completed branch +addresses that lesson by defining `TY_VERSION ?= 0.0.56` and invoking the +checker through `uv tool run --from ty==$(TY_VERSION) ty`; CI calls +`make typecheck` and does not install ty separately. Follow-up candidates (not in scope): rewrite version requirements in non-member nested manifests that depend on bumped crates. @@ -281,22 +284,22 @@ pieces: - `lading/commands/bump.py` orchestrates `lading bump`. After rewriting manifest versions it calls `_process_lockfiles(context, changed_manifests)`, which returns the tuple of lockfile paths that were (or, in dry-run, would - be) regenerated. It currently passes only - `context.configuration.bump.lockfile_manifests` (a tuple of workspace-relative - `Cargo.toml` path strings) to the helpers below. - `context.base_options.command_runner` is an optional `CommandRunner` — a - callable taking a command sequence and returning - `(exit_code, stdout, stderr)` — used for dependency injection in tests; - `None` means "use the real subprocess runner". -- `lading/commands/bump_lockfiles.py` owns regeneration. - `resolve_lockfile_paths(workspace_root, lockfile_manifests)` maps configured - manifests to lockfile paths for dry-run reporting. + be) regenerated. The bump domain depends on the `LockfileRepository` port and + passes the configured manifest tuple to that boundary. The default + `CargoLockfileRepository` adapter performs discovery and merges configured + manifests with manifests implied by tracked `Cargo.lock` files before either + dry-run projection or live regeneration. +- `lading/commands/bump_lockfiles.py` defines the `LockfileRepository` port, + its `CargoLockfileRepository` adapter, and the regeneration helpers. + `resolve_lockfile_paths(workspace_root, lockfile_manifests)` maps the merged + manifest set to lockfile paths for dry-run reporting. `regenerate_lockfiles(workspace_root, lockfile_manifests, *, runner=None)` validates each manifest path (must stay inside the workspace and be named `Cargo.toml`), always prepends the workspace root manifest, de-duplicates, and runs `cargo update --workspace --manifest-path ` per entry. -- `lading/commands/lockfile.py` owns discovery and freshness validation, - currently used only by the publish pre-flight. +- `lading/commands/lockfile.py` owns discovery and freshness validation. + Publish uses discovery for freshness validation, while the bump-side + `CargoLockfileRepository` uses it to construct the merged manifest set. `discover_tracked_lockfiles(workspace_root, runner, *, manifest_exists=...)` runs `git ls-files "**/Cargo.lock" "Cargo.lock"`, filters out paths with a `target` component, keeps only lockfiles with an adjacent `Cargo.toml`, and @@ -359,11 +362,11 @@ Stage B (red): specify the behaviour with failing tests. `@pytest.mark.xfail(strict=True, reason="discovery merge not implemented")` until red is observed, then remove the marker during Stage C. 2. In `tests/unit/test_bump_lockfile_rebuild.py`, extend the existing - monkeypatch-style tests: patch discovery (not the real git) so that - `bump.run` passes the union of configured and discovered manifests to - `regenerate_lockfiles`, and assert the dry-run path lists discovered - lockfile paths in its output. Existing assertions such as - `"lockfile_manifests": ()` will need the new expected union values. + monkeypatch-style tests: patch the lockfile repository so that `bump.run` + projects or regenerates the union of configured and discovered manifests, + and assert the dry-run path lists discovered lockfile paths in its output. + Existing assertions such as `"lockfile_manifests": ()` will need the new + expected union values. 3. In `tests/bdd/features/cli.feature`, extend the "Bump rebuilds tracked lockfiles" scenario (or add a sibling scenario "Bump refreshes discovered nested lockfiles") so the workspace contains a tracked nested lockfile that @@ -387,24 +390,27 @@ Stage C (green): implement the minimal change. `_resolve_manifest_paths` continues to prepend the root manifest and de-duplicate resolved paths, so the root lockfile stays first regardless of what discovery returns. -2. In `lading/commands/bump.py::_process_lockfiles`, call the merge function - once (passing `context.base_options.command_runner`), and feed its result to - both the dry-run `resolve_lockfile_paths` branch and the live - `regenerate_lockfiles` branch, so dry-run and live runs report the same set. +2. In `CargoLockfileRepository`, call the merge function before delegating to + either the dry-run `resolve_lockfile_paths` helper or the live + `regenerate_lockfiles` helper. Keep + `lading/commands/bump.py::_process_lockfiles` dependent only on the + `LockfileRepository` port, so dry-run and live runs use the same set without + introducing Git or Cargo execution into the bump domain. 3. Update module docstrings that describe the old contract: the header of `bump_lockfiles.py` ("any configured nested lockfiles") and the call-graph - paragraph in `lockfile.py` (which currently says discovery is publish-only). + paragraph in `lockfile.py` (which said before this change that discovery was + publish-only). Stage D (refactor, documentation, cleanup): 1. Regenerate affected `syrupy` snapshots deliberately and review the diffs. 2. `docs/users-guide.md`: the bump section's discovery claim becomes true — tighten it to mention that configured `lockfile_manifests` are additionally - regenerated; re-scope the `lockfile_manifests` key description to "manifests - whose lockfiles git does not track (discovery finds tracked ones - automatically)"; extend the "Lockfile regeneration runs…" paragraph to say - the command runs for the workspace root, each configured manifest, and each - discovered tracked lockfile's manifest. + regenerated; re-scope the `lockfile_manifests` key description to manifests + discovery cannot find, including nested manifests outside a Git repository; + extend the "Lockfile regeneration runs…" paragraph to say the command runs + for the workspace root, each configured manifest, and each discovered + tracked lockfile's manifest. 3. `lading/commands/publish_preflight.py::_build_stale_lockfile_message`: soften "This commonly happens after running `lading bump`" to reflect that bump now repairs tracked lockfiles itself — for example, "This can happen @@ -521,7 +527,7 @@ entirely in the scratchpad and is deleted afterwards. If a milestone must be abandoned mid-way, `git status` plus `git checkout -- ` restores the tree; committed milestones are individually revertable. -## Artifacts and notes +## Artefacts and notes Stage A prototype transcript (2026-07-07, scratchpad `proto/`): a git-initialized workspace with member crate `alpha` 0.1.0 and a standalone @@ -571,23 +577,24 @@ def merge_discovered_manifests( """ ``` -`lading/commands/bump.py::_process_lockfiles` calls it once and passes the -result to the existing `resolve_lockfile_paths` (dry-run) and -`regenerate_lockfiles` (live) helpers, whose signatures do not change. +`CargoLockfileRepository` calls it before delegating the merged result to the +existing `resolve_lockfile_paths` helper for dry-run projection or +`regenerate_lockfiles` for live regeneration. The bump-side +`_process_lockfiles` function depends only on the `LockfileRepository` port. ## Revision note 2026-07-08: implementation complete. Progress, surprises, decisions, and the retrospective were updated throughout Stages A–D; the final revision records the end-to-end acceptance run, the versioned-path-dependency edge case, and the -follow-up candidates (non-member manifest rewriting; pinning ty). Status moved -to COMPLETE. No further work remains on this plan. - -2026-07-08 (later): the ty-pinning follow-up was completed on this branch at -the user's request — `TY_VERSION ?= 0.0.56` in the Makefile (invoked via -`uv tool run --from ty==$(TY_VERSION)`), CI no longer installs ty separately, -and ty 0.0.56 passes with no new diagnostics. The non-member manifest rewriting -follow-up remains open. +then-pending follow-up candidates. Status moved to COMPLETE. No further work +remained in the original feature scope. + +2026-07-08 (later): the ty version-pin follow-up was completed on this branch +at the user's request. The Makefile defines `TY_VERSION ?= 0.0.56` and invokes +the checker through `uv tool run --from ty==$(TY_VERSION) ty`; CI runs +`make typecheck` and no longer installs ty separately. Ty 0.0.56 passes with no +new diagnostics. The non-member manifest rewriting follow-up remains open. 2026-07-14: rebasing onto `origin/main` incorporated issue #82's `LockfileRepository` port. Discovery now occurs inside the From acdf5cbef5cbbb1e32659f6abd2ad3fa7026a001 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 11:31:24 +0200 Subject: [PATCH 11/16] Make index failure returns explicit Add bare returns after the terminal raising helpers while preserving the existing branch structure and type narrowing. --- lading/commands/publish_index_check.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lading/commands/publish_index_check.py b/lading/commands/publish_index_check.py index 5e6fa350..cfd05882 100644 --- a/lading/commands/publish_index_check.py +++ b/lading/commands/publish_index_check.py @@ -372,6 +372,7 @@ def _handle_index_missing_version( ) return _raise_name_extraction_failure(context) + return def _downgrade_or_raise( @@ -403,3 +404,4 @@ def _downgrade_or_raise( ) return _raise_unpublished_dependency_override_required(context, missing_name=missing_name) + return From 2ff364f2302f9f34ac980579828c8be049891bf9 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 27 Jul 2026 14:55:32 +0200 Subject: [PATCH 12/16] Format replayed lockfile tests Restore Ruff's required module-level spacing after the rebase combined the example and property tests. --- tests/unit/test_bump_lockfiles.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/unit/test_bump_lockfiles.py b/tests/unit/test_bump_lockfiles.py index b85b26c4..8252a68c 100644 --- a/tests/unit/test_bump_lockfiles.py +++ b/tests/unit/test_bump_lockfiles.py @@ -50,6 +50,7 @@ def __call__( self.invocations.append(_Invocation(command=tuple(command), cwd=cwd)) return self.result + def _write_manifest(directory: Path) -> None: """Create ``directory`` with a placeholder ``Cargo.toml`` inside it.""" directory.mkdir(parents=True, exist_ok=True) @@ -58,6 +59,7 @@ def _write_manifest(directory: Path) -> None: encoding="utf-8", ) + @st.composite def _manifest_merge_cases( draw: st.DrawFn, @@ -91,6 +93,8 @@ def _manifest_merge_cases( f"{directory}/Cargo.lock" for directory in discovered_directories ) return configured, discovered + + def test_merge_discovered_manifests_appends_tracked_manifests( tmp_path: Path, ) -> None: @@ -124,6 +128,7 @@ def test_merge_discovered_manifests_appends_tracked_manifests( ) ], f"unexpected git invocations: {runner.invocations!r}" + def test_merge_discovered_manifests_deduplicates_configured_forms( tmp_path: Path, ) -> None: @@ -141,6 +146,7 @@ def test_merge_discovered_manifests_deduplicates_configured_forms( f"equivalent configured manifest was re-appended: {merged!r}" ) + def test_merge_discovered_manifests_returns_configured_outside_git( tmp_path: Path, ) -> None: @@ -163,6 +169,7 @@ def test_merge_discovered_manifests_returns_configured_outside_git( f"non-git fallback changed configured manifests: {merged!r}" ) + @given(case=_manifest_merge_cases()) @settings(max_examples=80, deadline=None) def test_merge_discovered_manifests_preserves_order_and_deduplicates( @@ -200,6 +207,7 @@ def test_merge_discovered_manifests_preserves_order_and_deduplicates( (workspace_root / manifest).resolve() for manifest in merged }) == len(merged), f"merge retained equivalent manifest paths: {merged!r}" + @given( configured=st.lists( st.from_regex(r"(?:\./)?[a-z][a-z0-9]{0,7}/Cargo\.toml", fullmatch=True), @@ -223,6 +231,8 @@ def test_merge_discovered_manifests_non_git_fallback_is_identity( assert len(runner.invocations) == 1, ( f"non-git discovery should invoke git once: {runner.invocations!r}" ) + + def test_regenerate_lockfiles_includes_workspace_manifest(tmp_path: Path) -> None: """The workspace root manifest should always be regenerated.""" runner = _RecordingRunner() From 29962d990be1a4bc731d2329073aab00feeb2261 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 28 Jul 2026 01:42:10 +0200 Subject: [PATCH 13/16] Verify discovered lockfile update calls Assert that the discovery BDD scenario invokes Cargo separately for the workspace and nested manifests. Expand the affected fixture docstrings to record their inputs, module gating, side effects, and return contract. --- tests/bdd/features/cli.feature | 2 +- tests/bdd/steps/test_bump_steps.py | 63 +++++++++++++++++++++++++++++- tests/unit/conftest.py | 17 +++++++- 3 files changed, 79 insertions(+), 3 deletions(-) diff --git a/tests/bdd/features/cli.feature b/tests/bdd/features/cli.feature index 7fd6dd8b..644d23f9 100644 --- a/tests/bdd/features/cli.feature +++ b/tests/bdd/features/cli.feature @@ -42,7 +42,7 @@ Feature: Lading CLI scaffolding When I invoke lading bump 1.2.3 with that workspace Then the CLI output lists lockfile path "- Cargo.lock (lockfile)" And the CLI output lists lockfile path "- fixtures/minimal/Cargo.lock (lockfile)" - And the bump command refreshed tracked lockfiles + And the bump command refreshed workspace and nested tracked lockfiles Scenario: Bump dry-run does not modify lockfiles Given a workspace directory with configuration diff --git a/tests/bdd/steps/test_bump_steps.py b/tests/bdd/steps/test_bump_steps.py index 490ef711..c9ab8373 100644 --- a/tests/bdd/steps/test_bump_steps.py +++ b/tests/bdd/steps/test_bump_steps.py @@ -50,6 +50,21 @@ def given_workspace_has_nested_tracked_lockfile( The nested package is not listed in ``bump.lockfile_manifests``; bump must discover it from the git index and refresh it anyway. + + Parameters + ---------- + cmd_mox : CmdMox + Command-double fixture used to stub Git, Cargo, and metadata commands. + monkeypatch : pytest.MonkeyPatch + Fixture used to install the Cargo command shim for the scenario. + workspace_directory : Path + Temporary workspace containing the root and nested lockfiles. + + Returns + ------- + None + This step configures the workspace and command doubles in place. + """ from tests.helpers.workspace_helpers import install_cargo_stub @@ -68,7 +83,7 @@ def given_workspace_has_nested_tracked_lockfile( stderr="", ) # Stubs dispatch by command name alone, so one argless registration covers - # the root and nested manifest invocations alike. + # both invocations. The scenario's Then step verifies their exact arguments. cmd_mox.stub("cargo::update").returns( exit_code=0, stdout="cargo update --workspace\n", stderr="" ) @@ -207,6 +222,52 @@ def then_bump_refreshed_lockfiles(cli_run: dict[str, typ.Any]) -> None: assert "cargo update --workspace" in output +@then("the bump command refreshed workspace and nested tracked lockfiles") +def then_bump_refreshed_workspace_and_nested_lockfiles( + cli_run: dict[str, typ.Any], + cmd_mox: CmdMox, + workspace_directory: Path, +) -> None: + """Assert Cargo refreshed the root and discovered nested lockfiles. + + Parameters + ---------- + cli_run : dict[str, Any] + Captured result of the completed ``lading bump`` invocation. + cmd_mox : CmdMox + Command-double fixture whose journal records Cargo update calls. + workspace_directory : Path + Temporary workspace used to derive the expected manifest paths. + + Returns + ------- + None + This step only asserts the recorded command invocations. + + """ + assert cli_run["returncode"] == 0 + update_args = [ + tuple(invocation.args) + for invocation in cmd_mox.journal + if invocation.command == "cargo::update" + ] + expected_args = [ + ( + "--workspace", + "--manifest-path", + str(workspace_directory / "Cargo.toml"), + ), + ( + "--workspace", + "--manifest-path", + str(workspace_directory / "fixtures/minimal/Cargo.toml"), + ), + ] + assert update_args == expected_args, ( + f"expected distinct root and nested Cargo updates; got {update_args!r}" + ) + + @then("the bump command did not refresh tracked lockfiles") def then_bump_did_not_refresh_lockfiles(cli_run: dict[str, typ.Any]) -> None: """Assert the dry-run lockfile scenario completed without refresh.""" diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 8e1d5bd8..fae3baa4 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -77,7 +77,22 @@ def disable_publish_preflight(monkeypatch: pytest.MonkeyPatch) -> None: def stub_lockfile_regeneration( request: pytest.FixtureRequest, monkeypatch: pytest.MonkeyPatch ) -> None: - """Avoid invoking Cargo or git from manifest-focused bump tests.""" + """Stub lockfile operations for manifest-focused bump test modules. + + Parameters + ---------- + request : pytest.FixtureRequest + Active test request used to identify the containing test module. + monkeypatch : pytest.MonkeyPatch + Fixture used to replace lockfile discovery and regeneration helpers. + + Returns + ------- + None + Modules outside ``_LOCKFILE_STUB_MODULES`` are left unchanged; selected + modules receive deterministic helpers that avoid invoking Git or Cargo. + + """ if request.module.__name__.rsplit(".", 1)[-1] not in _LOCKFILE_STUB_MODULES: return monkeypatch.setattr( From c11299645f6c2910a5c2fca8021cc36568eee41b Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 28 Jul 2026 13:56:58 +0200 Subject: [PATCH 14/16] Split lockfile tests by subject Separate manifest merging and path projection from lockfile regeneration so each test module exercises one cohesive production API. Keep private support code local and preserve the regeneration snapshot under its existing module. --- .../unit/test_bump_lockfile_manifest_merge.py | 224 +++++++++++ .../test_bump_lockfile_path_resolution.py | 176 +++++++++ tests/unit/test_bump_lockfiles.py | 361 +----------------- 3 files changed, 402 insertions(+), 359 deletions(-) create mode 100644 tests/unit/test_bump_lockfile_manifest_merge.py create mode 100644 tests/unit/test_bump_lockfile_path_resolution.py diff --git a/tests/unit/test_bump_lockfile_manifest_merge.py b/tests/unit/test_bump_lockfile_manifest_merge.py new file mode 100644 index 00000000..5e7724d8 --- /dev/null +++ b/tests/unit/test_bump_lockfile_manifest_merge.py @@ -0,0 +1,224 @@ +"""Tests for merging configured and discovered lockfile manifests.""" + +from __future__ import annotations + +import collections.abc as cabc +import dataclasses as dc +import tempfile +from pathlib import Path + +from hypothesis import given, settings +from hypothesis import strategies as st + +from lading.commands import bump_lockfiles + + +@dc.dataclass(frozen=True, slots=True) +class _Invocation: + """Recorded command invocation.""" + + command: tuple[str, ...] + cwd: Path | None + + +class _RecordingRunner: + """Record command invocations and return a configured result.""" + + def __init__( + self, + result: tuple[int, str, str] = (0, "", ""), + ) -> None: + self.result = result + self.invocations: list[_Invocation] = [] + + def __call__( + self, + command: cabc.Sequence[str], + *, + cwd: Path | None = None, + ) -> tuple[int, str, str]: + """Record one command invocation.""" + self.invocations.append(_Invocation(command=tuple(command), cwd=cwd)) + return self.result + + +def _write_manifest(directory: Path) -> None: + """Create ``directory`` with a placeholder ``Cargo.toml`` inside it.""" + directory.mkdir(parents=True, exist_ok=True) + (directory / "Cargo.toml").write_text( + '[package]\nname = "placeholder"\nversion = "0.1.0"\n', + encoding="utf-8", + ) + + +@st.composite +def _manifest_merge_cases( + draw: st.DrawFn, +) -> tuple[tuple[str, ...], tuple[str, ...]]: + """Generate configured manifests and arbitrarily ordered tracked lockfiles.""" + directories = draw( + st.lists( + st.from_regex(r"[a-z][a-z0-9]{0,7}", fullmatch=True), + min_size=1, + max_size=8, + unique=True, + ) + ) + discovered_directories = draw(st.permutations(directories)) + configured_directories = draw(st.lists(st.sampled_from(directories), unique=True)) + configured_directories = draw(st.permutations(configured_directories)) + dot_prefixes = draw( + st.lists( + st.booleans(), + min_size=len(configured_directories), + max_size=len(configured_directories), + ) + ) + configured = tuple( + f"{'./' if has_dot_prefix else ''}{directory}/Cargo.toml" + for directory, has_dot_prefix in zip( + configured_directories, dot_prefixes, strict=True + ) + ) + discovered = tuple( + f"{directory}/Cargo.lock" for directory in discovered_directories + ) + return configured, discovered + + +def test_merge_discovered_manifests_appends_tracked_manifests( + tmp_path: Path, +) -> None: + """Discovered tracked-lockfile manifests follow configured entries, sorted.""" + _write_manifest(tmp_path) + _write_manifest(tmp_path / "crates/ui") + _write_manifest(tmp_path / "fixtures/minimal") + runner = _RecordingRunner( + result=( + 0, + "fixtures/minimal/Cargo.lock\nCargo.lock\ncrates/ui/Cargo.lock\n", + "", + ) + ) + + merged = bump_lockfiles.merge_discovered_manifests( + tmp_path, + ("crates/ui/Cargo.toml",), + runner=runner, + ) + + assert merged == ( + "crates/ui/Cargo.toml", + "Cargo.toml", + "fixtures/minimal/Cargo.toml", + ), f"unexpected merged manifests: {merged!r}" + assert runner.invocations == [ + _Invocation( + command=("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), + cwd=tmp_path, + ) + ], f"unexpected git invocations: {runner.invocations!r}" + + +def test_merge_discovered_manifests_deduplicates_configured_forms( + tmp_path: Path, +) -> None: + """A manifest configured under an equivalent spelling is not re-appended.""" + _write_manifest(tmp_path / "fixtures/minimal") + runner = _RecordingRunner(result=(0, "fixtures/minimal/Cargo.lock\n", "")) + + merged = bump_lockfiles.merge_discovered_manifests( + tmp_path, + ("./fixtures/minimal/Cargo.toml",), + runner=runner, + ) + + assert merged == ("./fixtures/minimal/Cargo.toml",), ( + f"equivalent configured manifest was re-appended: {merged!r}" + ) + + +def test_merge_discovered_manifests_returns_configured_outside_git( + tmp_path: Path, +) -> None: + """A non-git workspace degrades to the configured manifests unchanged.""" + runner = _RecordingRunner( + result=( + 128, + "", + "fatal: not a git repository (or any of the parent directories): .git", + ) + ) + + merged = bump_lockfiles.merge_discovered_manifests( + tmp_path, + ("crates/ui/Cargo.toml",), + runner=runner, + ) + + assert merged == ("crates/ui/Cargo.toml",), ( + f"non-git fallback changed configured manifests: {merged!r}" + ) + + +@given(case=_manifest_merge_cases()) +@settings(max_examples=80, deadline=None) +def test_merge_discovered_manifests_preserves_order_and_deduplicates( + case: tuple[tuple[str, ...], tuple[str, ...]], +) -> None: + """Configured order wins while unique discoveries are appended sorted.""" + configured, discovered = case + with tempfile.TemporaryDirectory() as temporary_directory: + workspace_root = Path(temporary_directory) + for lockfile in discovered: + _write_manifest(workspace_root / Path(lockfile).parent) + runner = _RecordingRunner(result=(0, "\n".join(discovered) + "\n", "")) + + merged = bump_lockfiles.merge_discovered_manifests( + workspace_root, + configured, + runner=runner, + ) + + configured_paths = { + (workspace_root / manifest).resolve() for manifest in configured + } + expected_discovered = tuple( + sorted( + str(Path(lockfile).with_name("Cargo.toml")) + for lockfile in discovered + if (workspace_root / Path(lockfile).with_name("Cargo.toml")).resolve() + not in configured_paths + ) + ) + assert merged == configured + expected_discovered, ( + f"merge did not preserve configured order and sorted discovery: {merged!r}" + ) + assert len({ + (workspace_root / manifest).resolve() for manifest in merged + }) == len(merged), f"merge retained equivalent manifest paths: {merged!r}" + + +@given( + configured=st.lists( + st.from_regex(r"(?:\./)?[a-z][a-z0-9]{0,7}/Cargo\.toml", fullmatch=True), + max_size=8, + ).map(tuple) +) +@settings(max_examples=50, deadline=None) +def test_merge_discovered_manifests_non_git_fallback_is_identity( + configured: tuple[str, ...], +) -> None: + """Non-git discovery preserves every configured entry and its order.""" + runner = _RecordingRunner(result=(128, "", "fatal: not a git repository")) + with tempfile.TemporaryDirectory() as temporary_directory: + merged = bump_lockfiles.merge_discovered_manifests( + Path(temporary_directory), + configured, + runner=runner, + ) + + assert merged == configured, f"non-git fallback changed input: {merged!r}" + assert len(runner.invocations) == 1, ( + f"non-git discovery should invoke git once: {runner.invocations!r}" + ) diff --git a/tests/unit/test_bump_lockfile_path_resolution.py b/tests/unit/test_bump_lockfile_path_resolution.py new file mode 100644 index 00000000..1327b007 --- /dev/null +++ b/tests/unit/test_bump_lockfile_path_resolution.py @@ -0,0 +1,176 @@ +"""Tests for projecting Cargo manifests to lockfile paths.""" + +from __future__ import annotations + +import string +import tempfile +from pathlib import Path + +import pytest +from hypothesis import assume, given, settings +from hypothesis import strategies as st + +from lading.commands import bump_lockfiles + + +def test_resolve_lockfile_paths_reports_dry_run_targets(tmp_path: Path) -> None: + """Dry-run reporting can resolve lockfiles without invoking Cargo.""" + lockfiles = bump_lockfiles.resolve_lockfile_paths( + tmp_path, + ("Cargo.toml", "crates/nested/Cargo.toml"), + ) + + assert lockfiles == ( + tmp_path / "Cargo.lock", + tmp_path / "crates/nested/Cargo.lock", + ) + + +@pytest.mark.parametrize( + ("manifest", "expected_message"), + [ + ("../outside/Cargo.toml", "within the workspace"), + ("Cargo.lock", "Cargo.toml file"), + ("crates/nested/foo.toml", "Cargo.toml file"), + ], +) +def test_resolve_lockfile_paths_rejects_invalid_targets( + tmp_path: Path, + manifest: str, + expected_message: str, +) -> None: + """Configured manifests must stay in-workspace and name Cargo.toml.""" + with pytest.raises( + bump_lockfiles.LockfileRegenerationError, match=expected_message + ): + bump_lockfiles.resolve_lockfile_paths(tmp_path, (manifest,)) + + +_SEGMENT = st.text( + alphabet=string.ascii_lowercase + string.digits + "_-", + min_size=1, + max_size=10, +) + + +@st.composite +def _inside_dir(draw: st.DrawFn) -> list[str]: + """Draw a relative directory path that stays within the workspace. + + Alongside real segments and redundant ``.`` segments, this emits safe + ``..`` parent-traversal segments that never ascend above the workspace + root after normalisation: a ``..`` is only produced while a prior real + segment remains to cancel it (the running segment balance never drops + below zero). Cases such as ``crate/../Cargo.toml`` are therefore + exercised without allowing traversal above the root. + """ + length = draw(st.integers(min_value=0, max_value=4)) + components: list[str] = [] + depth = 0 + for _ in range(length): + options = [_SEGMENT, st.just(".")] + if depth > 0: + options.append(st.just("..")) + segment = draw(st.one_of(*options)) + if segment == "..": + depth -= 1 + elif segment != ".": + depth += 1 + components.append(segment) + return components + + +# Relative directory paths that stay inside the workspace, optionally with +# redundant "." segments and safe ".." traversals which normalise away. +_INSIDE_DIR = _inside_dir() + + +def _manifest_string(components: list[str]) -> str: + """Render a manifest path string from directory ``components``.""" + return "/".join((*components, "Cargo.toml")) + + +@given(dirs=st.lists(_INSIDE_DIR, max_size=6)) +@settings(max_examples=60, deadline=None) +def test_inside_manifests_resolve_to_sibling_lockfiles( + dirs: list[list[str]], +) -> None: + """Any in-workspace manifest string yields a sibling Cargo.lock path. + + Also pins the ordering and deduplication invariants: the workspace root + lockfile is always first, and resolved paths are unique. + """ + with tempfile.TemporaryDirectory(prefix="lading-bump-lockfiles-") as tmp: + workspace_root = Path(tmp) + manifests = [_manifest_string(components) for components in dirs] + + lockfiles = bump_lockfiles.resolve_lockfile_paths(workspace_root, manifests) + + resolved_root = workspace_root.resolve() + assert lockfiles[0] == resolved_root / "Cargo.lock", ( + "workspace root Cargo.lock must be the first resolved lockfile" + ) + assert len(set(lockfiles)) == len(lockfiles), ( + "resolved lockfiles must be unique" + ) + for lockfile_path in lockfiles: + assert lockfile_path.name == "Cargo.lock", ( + f"resolved path must be a sibling Cargo.lock: {lockfile_path}" + ) + assert lockfile_path.parent == lockfile_path.parent.resolve(), ( + f"lockfile parent must be a normalised path: {lockfile_path}" + ) + assert lockfile_path.is_relative_to(resolved_root), ( + f"lockfile must stay within the workspace root: {lockfile_path}" + ) + expected = tuple( + dict.fromkeys( + [resolved_root / "Cargo.lock"] + + [ + workspace_root.joinpath(*components, "Cargo.toml").resolve().parent + / "Cargo.lock" + for components in dirs + ] + ) + ) + assert lockfiles == expected, ( + "resolved lockfiles must preserve manifest execution order " + "without duplicates" + ) + + +@given(spellings=st.lists(st.sampled_from(["Cargo.toml", "./Cargo.toml"]), max_size=4)) +@settings(max_examples=20, deadline=None) +def test_root_manifest_spellings_deduplicate_to_one_invocation( + spellings: list[str], +) -> None: + """Every spelling of the root manifest produces exactly one root entry.""" + with tempfile.TemporaryDirectory(prefix="lading-bump-lockfiles-") as tmp: + workspace_root = Path(tmp) + + lockfiles = bump_lockfiles.resolve_lockfile_paths(workspace_root, spellings) + + assert lockfiles == (workspace_root.resolve() / "Cargo.lock",) + + +@given( + escape_depth=st.integers(min_value=1, max_value=3), + suffix=st.lists(_SEGMENT, max_size=2), +) +@settings(max_examples=30, deadline=None) +def test_escaping_manifests_are_rejected( + escape_depth: int, + suffix: list[str], +) -> None: + """Any manifest path escaping the workspace root raises an error.""" + with tempfile.TemporaryDirectory(prefix="lading-bump-lockfiles-") as tmp: + workspace_root = Path(tmp) / "workspace" + workspace_root.mkdir() + assume(suffix[:1] != ["workspace"]) + escaping = "/".join(([".."] * escape_depth) + [*suffix, "Cargo.toml"]) + + with pytest.raises( + bump_lockfiles.LockfileRegenerationError, + match="must stay within the workspace", + ): + bump_lockfiles.resolve_lockfile_paths(workspace_root, (escaping,)) diff --git a/tests/unit/test_bump_lockfiles.py b/tests/unit/test_bump_lockfiles.py index 8252a68c..b4a3e0f4 100644 --- a/tests/unit/test_bump_lockfiles.py +++ b/tests/unit/test_bump_lockfiles.py @@ -1,4 +1,4 @@ -"""Tests for lockfile regeneration after bump operations.""" +"""Tests for Cargo lockfile regeneration after bump operations.""" from __future__ import annotations @@ -7,13 +7,12 @@ import operator import pathlib import shlex -import string import tempfile import typing as typ from pathlib import Path import pytest -from hypothesis import assume, given, settings +from hypothesis import given, settings from hypothesis import strategies as st from lading.commands import bump_lockfiles @@ -51,188 +50,6 @@ def __call__( return self.result -def _write_manifest(directory: Path) -> None: - """Create ``directory`` with a placeholder ``Cargo.toml`` inside it.""" - directory.mkdir(parents=True, exist_ok=True) - (directory / "Cargo.toml").write_text( - '[package]\nname = "placeholder"\nversion = "0.1.0"\n', - encoding="utf-8", - ) - - -@st.composite -def _manifest_merge_cases( - draw: st.DrawFn, -) -> tuple[tuple[str, ...], tuple[str, ...]]: - """Generate configured manifests and arbitrarily ordered tracked lockfiles.""" - directories = draw( - st.lists( - st.from_regex(r"[a-z][a-z0-9]{0,7}", fullmatch=True), - min_size=1, - max_size=8, - unique=True, - ) - ) - discovered_directories = draw(st.permutations(directories)) - configured_directories = draw(st.lists(st.sampled_from(directories), unique=True)) - configured_directories = draw(st.permutations(configured_directories)) - dot_prefixes = draw( - st.lists( - st.booleans(), - min_size=len(configured_directories), - max_size=len(configured_directories), - ) - ) - configured = tuple( - f"{'./' if has_dot_prefix else ''}{directory}/Cargo.toml" - for directory, has_dot_prefix in zip( - configured_directories, dot_prefixes, strict=True - ) - ) - discovered = tuple( - f"{directory}/Cargo.lock" for directory in discovered_directories - ) - return configured, discovered - - -def test_merge_discovered_manifests_appends_tracked_manifests( - tmp_path: Path, -) -> None: - """Discovered tracked-lockfile manifests follow configured entries, sorted.""" - _write_manifest(tmp_path) - _write_manifest(tmp_path / "crates/ui") - _write_manifest(tmp_path / "fixtures/minimal") - runner = _RecordingRunner( - result=( - 0, - "fixtures/minimal/Cargo.lock\nCargo.lock\ncrates/ui/Cargo.lock\n", - "", - ) - ) - - merged = bump_lockfiles.merge_discovered_manifests( - tmp_path, - ("crates/ui/Cargo.toml",), - runner=runner, - ) - - assert merged == ( - "crates/ui/Cargo.toml", - "Cargo.toml", - "fixtures/minimal/Cargo.toml", - ), f"unexpected merged manifests: {merged!r}" - assert runner.invocations == [ - _Invocation( - command=("git", "ls-files", "**/Cargo.lock", "Cargo.lock"), - cwd=tmp_path, - ) - ], f"unexpected git invocations: {runner.invocations!r}" - - -def test_merge_discovered_manifests_deduplicates_configured_forms( - tmp_path: Path, -) -> None: - """A manifest configured under an equivalent spelling is not re-appended.""" - _write_manifest(tmp_path / "fixtures/minimal") - runner = _RecordingRunner(result=(0, "fixtures/minimal/Cargo.lock\n", "")) - - merged = bump_lockfiles.merge_discovered_manifests( - tmp_path, - ("./fixtures/minimal/Cargo.toml",), - runner=runner, - ) - - assert merged == ("./fixtures/minimal/Cargo.toml",), ( - f"equivalent configured manifest was re-appended: {merged!r}" - ) - - -def test_merge_discovered_manifests_returns_configured_outside_git( - tmp_path: Path, -) -> None: - """A non-git workspace degrades to the configured manifests unchanged.""" - runner = _RecordingRunner( - result=( - 128, - "", - "fatal: not a git repository (or any of the parent directories): .git", - ) - ) - - merged = bump_lockfiles.merge_discovered_manifests( - tmp_path, - ("crates/ui/Cargo.toml",), - runner=runner, - ) - - assert merged == ("crates/ui/Cargo.toml",), ( - f"non-git fallback changed configured manifests: {merged!r}" - ) - - -@given(case=_manifest_merge_cases()) -@settings(max_examples=80, deadline=None) -def test_merge_discovered_manifests_preserves_order_and_deduplicates( - case: tuple[tuple[str, ...], tuple[str, ...]], -) -> None: - """Configured order wins while unique discoveries are appended sorted.""" - configured, discovered = case - with tempfile.TemporaryDirectory() as temporary_directory: - workspace_root = Path(temporary_directory) - for lockfile in discovered: - _write_manifest(workspace_root / Path(lockfile).parent) - runner = _RecordingRunner(result=(0, "\n".join(discovered) + "\n", "")) - - merged = bump_lockfiles.merge_discovered_manifests( - workspace_root, - configured, - runner=runner, - ) - - configured_paths = { - (workspace_root / manifest).resolve() for manifest in configured - } - expected_discovered = tuple( - sorted( - str(Path(lockfile).with_name("Cargo.toml")) - for lockfile in discovered - if (workspace_root / Path(lockfile).with_name("Cargo.toml")).resolve() - not in configured_paths - ) - ) - assert merged == configured + expected_discovered, ( - f"merge did not preserve configured order and sorted discovery: {merged!r}" - ) - assert len({ - (workspace_root / manifest).resolve() for manifest in merged - }) == len(merged), f"merge retained equivalent manifest paths: {merged!r}" - - -@given( - configured=st.lists( - st.from_regex(r"(?:\./)?[a-z][a-z0-9]{0,7}/Cargo\.toml", fullmatch=True), - max_size=8, - ).map(tuple) -) -@settings(max_examples=50, deadline=None) -def test_merge_discovered_manifests_non_git_fallback_is_identity( - configured: tuple[str, ...], -) -> None: - """Non-git discovery preserves every configured entry and its order.""" - runner = _RecordingRunner(result=(128, "", "fatal: not a git repository")) - with tempfile.TemporaryDirectory() as temporary_directory: - merged = bump_lockfiles.merge_discovered_manifests( - Path(temporary_directory), - configured, - runner=runner, - ) - - assert merged == configured, f"non-git fallback changed input: {merged!r}" - assert len(runner.invocations) == 1, ( - f"non-git discovery should invoke git once: {runner.invocations!r}" - ) - - def test_regenerate_lockfiles_includes_workspace_manifest(tmp_path: Path) -> None: """The workspace root manifest should always be regenerated.""" runner = _RecordingRunner() @@ -317,39 +134,6 @@ def test_regenerate_lockfiles_deduplicates_root_manifest(tmp_path: Path) -> None ] -def test_resolve_lockfile_paths_reports_dry_run_targets(tmp_path: Path) -> None: - """Dry-run reporting can resolve lockfiles without invoking Cargo.""" - lockfiles = bump_lockfiles.resolve_lockfile_paths( - tmp_path, - ("Cargo.toml", "crates/nested/Cargo.toml"), - ) - - assert lockfiles == ( - tmp_path / "Cargo.lock", - tmp_path / "crates/nested/Cargo.lock", - ) - - -@pytest.mark.parametrize( - ("manifest", "expected_message"), - [ - ("../outside/Cargo.toml", "within the workspace"), - ("Cargo.lock", "Cargo.toml file"), - ("crates/nested/foo.toml", "Cargo.toml file"), - ], -) -def test_resolve_lockfile_paths_rejects_invalid_targets( - tmp_path: Path, - manifest: str, - expected_message: str, -) -> None: - """Configured manifests must stay in-workspace and name Cargo.toml.""" - with pytest.raises( - bump_lockfiles.LockfileRegenerationError, match=expected_message - ): - bump_lockfiles.resolve_lockfile_paths(tmp_path, (manifest,)) - - @pytest.mark.parametrize( ("manifest", "expected_message"), [ @@ -462,147 +246,6 @@ def failing_runner( # Hypothesis property tests for manifest-path resolution (issue #93) # --------------------------------------------------------------------------- -_SEGMENT = st.text( - alphabet=string.ascii_lowercase + string.digits + "_-", - min_size=1, - max_size=10, -) - - -@st.composite -def _inside_dir(draw: st.DrawFn) -> list[str]: - """Draw a relative directory path that stays within the workspace. - - Alongside real segments and redundant ``.`` segments, this emits safe - ``..`` parent-traversal segments that never ascend above the workspace - root after normalisation: a ``..`` is only produced while a prior real - segment remains to cancel it (the running segment balance never drops - below zero). Cases such as ``crate/../Cargo.toml`` are therefore - exercised without allowing traversal above the root. - """ - length = draw(st.integers(min_value=0, max_value=4)) - components: list[str] = [] - depth = 0 - for _ in range(length): - options = [_SEGMENT, st.just(".")] - if depth > 0: - options.append(st.just("..")) - segment = draw(st.one_of(*options)) - if segment == "..": - depth -= 1 - elif segment != ".": - depth += 1 - components.append(segment) - return components - - -# Relative directory paths that stay inside the workspace, optionally with -# redundant "." segments and safe ".." traversals which normalise away. -_INSIDE_DIR = _inside_dir() - - -def _manifest_string(components: list[str]) -> str: - """Render a manifest path string from directory ``components``.""" - return "/".join((*components, "Cargo.toml")) - - -@given(dirs=st.lists(_INSIDE_DIR, max_size=6)) -@settings(max_examples=60, deadline=None) -def test_inside_manifests_resolve_to_sibling_lockfiles( - dirs: list[list[str]], -) -> None: - """Any in-workspace manifest string yields a sibling Cargo.lock path. - - Also pins the ordering and deduplication invariants: the workspace root - lockfile is always first, and resolved paths are unique. - """ - with tempfile.TemporaryDirectory(prefix="lading-bump-lockfiles-") as tmp: - workspace_root = Path(tmp) - manifests = [_manifest_string(components) for components in dirs] - - lockfiles = bump_lockfiles.resolve_lockfile_paths(workspace_root, manifests) - - resolved_root = workspace_root.resolve() - assert lockfiles[0] == resolved_root / "Cargo.lock", ( - "workspace root Cargo.lock must be the first resolved lockfile" - ) - assert len(set(lockfiles)) == len(lockfiles), ( - "resolved lockfiles must be unique" - ) - for lockfile_path in lockfiles: - assert lockfile_path.name == "Cargo.lock", ( - f"resolved path must be a sibling Cargo.lock: {lockfile_path}" - ) - assert lockfile_path.parent == lockfile_path.parent.resolve(), ( - f"lockfile parent must be a normalised path: {lockfile_path}" - ) - assert lockfile_path.is_relative_to(resolved_root), ( - f"lockfile must stay within the workspace root: {lockfile_path}" - ) - # Build the expected ordered tuple: the workspace root Cargo.lock first, - # then the sibling lockfile for each manifest in execution order, with - # duplicates removed while preserving that order. - expected = tuple( - dict.fromkeys( - [resolved_root / "Cargo.lock"] - + [ - workspace_root.joinpath(*components, "Cargo.toml").resolve().parent - / "Cargo.lock" - for components in dirs - ] - ) - ) - assert lockfiles == expected, ( - "resolved lockfiles must preserve manifest execution order " - "without duplicates" - ) - - -@given(spellings=st.lists(st.sampled_from(["Cargo.toml", "./Cargo.toml"]), max_size=4)) -@settings(max_examples=20, deadline=None) -def test_root_manifest_spellings_deduplicate_to_one_invocation( - spellings: list[str], -) -> None: - """Every spelling of the root manifest produces exactly one root entry.""" - with tempfile.TemporaryDirectory(prefix="lading-bump-lockfiles-") as tmp: - workspace_root = Path(tmp) - - lockfiles = bump_lockfiles.resolve_lockfile_paths(workspace_root, spellings) - - assert lockfiles == (workspace_root.resolve() / "Cargo.lock",) - - -@given( - escape_depth=st.integers(min_value=1, max_value=3), - suffix=st.lists(_SEGMENT, max_size=2), -) -@settings(max_examples=30, deadline=None) -def test_escaping_manifests_are_rejected( - escape_depth: int, - suffix: list[str], -) -> None: - """Any manifest path escaping the workspace root raises an error.""" - with tempfile.TemporaryDirectory(prefix="lading-bump-lockfiles-") as tmp: - # Anchor inside a subdirectory so ".." segments cannot accidentally - # resolve back inside the temporary root. - workspace_root = Path(tmp) / "workspace" - workspace_root.mkdir() - # A suffix re-entering the workspace directory would resolve back - # inside and legitimately pass validation; exclude that case. - assume(suffix[:1] != ["workspace"]) - escaping = "/".join(([".."] * escape_depth) + [*suffix, "Cargo.toml"]) - - with pytest.raises( - bump_lockfiles.LockfileRegenerationError, - match="must stay within the workspace", - ): - bump_lockfiles.resolve_lockfile_paths(workspace_root, (escaping,)) - - -# --------------------------------------------------------------------------- -# Aggregated failure handling (issue #84) -# --------------------------------------------------------------------------- - def _selective_failure_runner( failing_manifests: set[Path], From 1581d877178cecf3a6e28480ab6fe0a8144c98f5 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 28 Jul 2026 19:11:10 +0200 Subject: [PATCH 15/16] Clarify lockfile ownership documentation Attribute discovery to its defining module, document unconditional root regeneration and non-Git configuration, and align the manifest-merge property with the POSIX path contract. --- docs/developers-guide.md | 17 +++++++++-------- docs/execplans/regenerate-lockfiles.md | 6 +++--- docs/users-guide.md | 19 ++++++++++--------- lading/commands/lockfile.py | 3 ++- .../unit/test_bump_lockfile_manifest_merge.py | 2 +- 5 files changed, 25 insertions(+), 22 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 99ebef7e..85710834 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -209,10 +209,10 @@ resolve from the crate directory, and leaves absolute URI targets unchanged. Markdown links in fenced code blocks, indented code blocks, and inline code spans are preserved verbatim. -`lading.commands.bump_lockfiles` owns Cargo lockfile discovery and regeneration -after manifest changes. `merge_discovered_manifests` unions the configured -`bump.lockfile_manifests` entries with manifests implied by git-tracked -`Cargo.lock` files (reusing +`lading.commands.bump_lockfiles` owns bump-side manifest merging and Cargo +lockfile regeneration after manifest changes. `merge_discovered_manifests` +unions the configured `bump.lockfile_manifests` entries with manifests implied +by git-tracked `Cargo.lock` files (reusing `lading.commands.lockfile.discover_tracked_lockfiles`); configured entries keep their order and discovered entries follow in sorted order. `regenerate_lockfiles` always includes the workspace root `Cargo.toml`, @@ -276,10 +276,9 @@ projecting dry-run paths or regenerating live lockfiles. Tests inject a repository (or bind the adapter to a recording runner) so lockfile commands can be observed without invoking real processes. The port's scope is bump-side lockfile projection and regeneration; publish-side discovery and validation go -through the sibling -`lockfile.LockfileInspectionRepository` port (see the Lockfile helpers section -below), so neither the bump nor the publish lockfile domain holds a raw -`CommandRunner` (issue #82). +through the sibling `lockfile.LockfileInspectionRepository` port (see the +Lockfile helpers section below), so neither the bump nor the publish lockfile +domain holds a raw `CommandRunner` (issue #82). Bump-time crate-set derivation is centralized in the bump context: the `excluded` and `updated_crate_names` sets are computed exactly once in @@ -610,6 +609,8 @@ canonical replacement callers and tests now use directly: | Private `_append_section` / `_format_plan` | `publish_plan.py` | renamed to public `append_section` / `format_plan` | | `split_command` / `_split_command` wrapper | `publish_execution.py` | `lading.runtime.subprocess_runner.split_command` | +_Table 1: Compatibility shims removed by the issue #163 sweep._ + #### Retained boundaries Not every module-level indirection is a compatibility shim. The following diff --git a/docs/execplans/regenerate-lockfiles.md b/docs/execplans/regenerate-lockfiles.md index 937683d9..61597637 100644 --- a/docs/execplans/regenerate-lockfiles.md +++ b/docs/execplans/regenerate-lockfiles.md @@ -59,9 +59,9 @@ fixture lockfile and seeing that lockfile listed in the bump output with a pre-flight degrades today. - All code comments and documentation use en-GB-oxendict spelling. - No new external dependencies. -- No single code file may exceed 400 lines (repository rule); adding to - `lading/commands/bump_lockfiles.py` (155 lines) and `lading/commands/bump.py` - must respect this. +- No single code file may exceed 400 lines (repository rule); at planning time, + `lading/commands/bump_lockfiles.py` was 155 lines, and additions to it and + `lading/commands/bump.py` had to respect this. ## Tolerances (exception triggers) diff --git a/docs/users-guide.md b/docs/users-guide.md index 48e76978..272afb56 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -87,15 +87,16 @@ lading bump 1.2.3 --dry-run After updating manifest versions, `lading bump` automatically refreshes any git-tracked `Cargo.lock` files (excluding those under `target/`) that have an adjacent `Cargo.toml`, plus any additional manifests listed in -`bump.lockfile_manifests` for lockfiles that git does not track. The result -header counts each changed category, e.g. "N manifest(s)", "N documentation -file(s)", "N readme file(s)", "N lockfile(s)", joined with Oxford-comma -grammar. In the per-file body list, manifest paths carry no suffix; the other -categories carry a parenthetical suffix: `(documentation)` for Markdown docs -whose TOML code fences were updated, `(readme)` for crate READMEs adopted from -the workspace README, and `(lockfile)` for regenerated `Cargo.lock` files. In -dry-run mode, every file is listed, but none are modified (lockfile discovery -still runs `git ls-files`, which is read-only). +`bump.lockfile_manifests` for lockfiles that git does not track or for nested +lockfiles when the workspace is outside a Git repository. The result header +counts each changed category, e.g. "N manifest(s)", "N documentation file(s)", +"N readme file(s)", "N lockfile(s)", joined with Oxford-comma grammar. In the +per-file body list, manifest paths carry no suffix; the other categories carry +a parenthetical suffix: `(documentation)` for Markdown docs whose TOML code +fences were updated, `(readme)` for crate READMEs adopted from the workspace +README, and `(lockfile)` for regenerated `Cargo.lock` files. In dry-run mode, +every file is listed, but none are modified (lockfile discovery still runs +`git ls-files`, which is read-only). Where a member crate sets `readme.workspace = true`, `lading bump` also adopts the workspace `README.md` into that crate's directory and rewrites relative diff --git a/lading/commands/lockfile.py b/lading/commands/lockfile.py index b94cd1ba..e3c32f13 100644 --- a/lading/commands/lockfile.py +++ b/lading/commands/lockfile.py @@ -15,7 +15,8 @@ so stale lockfiles fail early with an actionable repair command. ``lading bump`` uses :func:`discover_tracked_lockfiles` too — via :func:`lading.commands.bump_lockfiles.merge_discovered_manifests` — but then -regenerates the discovered and configured lockfiles through +regenerates the workspace-root lockfile from ``Cargo.toml``, together with the +discovered and configured lockfiles, through :func:`lading.commands.bump_lockfiles.regenerate_lockfiles`, which runs ``cargo update --workspace``: bump wants existing pinned versions refreshed in place after manifest rewrites, whereas validation here uses diff --git a/tests/unit/test_bump_lockfile_manifest_merge.py b/tests/unit/test_bump_lockfile_manifest_merge.py index 5e7724d8..98f78a22 100644 --- a/tests/unit/test_bump_lockfile_manifest_merge.py +++ b/tests/unit/test_bump_lockfile_manifest_merge.py @@ -185,7 +185,7 @@ def test_merge_discovered_manifests_preserves_order_and_deduplicates( } expected_discovered = tuple( sorted( - str(Path(lockfile).with_name("Cargo.toml")) + Path(lockfile).with_name("Cargo.toml").as_posix() for lockfile in discovered if (workspace_root / Path(lockfile).with_name("Cargo.toml")).resolve() not in configured_paths From ec1d7bc476b481e354724ec9dac33ba6394b5fd8 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 29 Jul 2026 12:10:59 +0200 Subject: [PATCH 16/16] Split lockfile regeneration by responsibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract manifest merging, path validation, and Cargo execution into cohesive modules while retaining `bump_lockfiles` as the stable public façade and repository boundary. Record the completed size refactor and final module measurements in the living ExecPlan. --- docs/execplans/regenerate-lockfiles.md | 108 +++++-- lading/commands/bump_lockfile_manifests.py | 66 ++++ lading/commands/bump_lockfile_paths.py | 51 +++ lading/commands/bump_lockfile_regeneration.py | 194 +++++++++++ lading/commands/bump_lockfiles.py | 303 +----------------- 5 files changed, 404 insertions(+), 318 deletions(-) create mode 100644 lading/commands/bump_lockfile_manifests.py create mode 100644 lading/commands/bump_lockfile_paths.py create mode 100644 lading/commands/bump_lockfile_regeneration.py diff --git a/docs/execplans/regenerate-lockfiles.md b/docs/execplans/regenerate-lockfiles.md index 61597637..db0620db 100644 --- a/docs/execplans/regenerate-lockfiles.md +++ b/docs/execplans/regenerate-lockfiles.md @@ -59,9 +59,10 @@ fixture lockfile and seeing that lockfile listed in the bump output with a pre-flight degrades today. - All code comments and documentation use en-GB-oxendict spelling. - No new external dependencies. -- No single code file may exceed 400 lines (repository rule); at planning time, - `lading/commands/bump_lockfiles.py` was 155 lines, and additions to it and - `lading/commands/bump.py` had to respect this. +- No single code file may exceed 400 lines (repository rule). The final + lockfile implementation is split into the 176-line compatibility façade + `bump_lockfiles.py`, the 66-line `bump_lockfile_manifests.py`, the 51-line + `bump_lockfile_paths.py`, and the 194-line `bump_lockfile_regeneration.py`. ## Tolerances (exception triggers) @@ -154,6 +155,10 @@ fixture lockfile and seeing that lockfile listed in the bump output with a nested lockfile recorded the new version, and `cargo metadata --locked` exited 0 afterwards. Also surfaced the versioned-path-dependency edge recorded under `Surprises & discoveries`. +- [x] (2026-07-29 10:07Z) Split the 451-line `bump_lockfiles.py` by + responsibility while retaining it as a 176-line compatibility façade. The + focused lockfile suite passed with 34 tests and 7 snapshots; all four + affected production modules measure at most 194 lines. ## Surprises & discoveries @@ -243,6 +248,15 @@ fixture lockfile and seeing that lockfile listed in the bump output with a fixes were small, behaviour-preserving restructures. A later revision on this completed branch also pinned ty as described below. Date/Author: 2026-07-07, implementation session. +- Decision: split manifest merging, path validation, and Cargo regeneration + into `bump_lockfile_manifests.py`, `bump_lockfile_paths.py`, and + `bump_lockfile_regeneration.py`, while retaining `bump_lockfiles.py` as the + public compatibility façade and home of the repository port and adapter. + Rationale: the combined module reached 451 lines and mixed four distinct + responsibilities. Re-exporting the established functions and exception + preserves public imports, while resolving those façade names from + `CargoLockfileRepository` preserves the monkeypatch seam used by bump tests. + Date/Author: 2026-07-29, implementation session. ## Outcomes & retrospective @@ -272,6 +286,12 @@ addresses that lesson by defining `TY_VERSION ?= 0.0.56` and invoking the checker through `uv tool run --from ty==$(TY_VERSION) ty`; CI calls `make typecheck` and does not install ty separately. +The later size refactor preserved that behaviour while making each production +module cohesive and compliant with the 400-line limit. The compatibility façade +retains the public function and exception imports and the `LockfileRepository` +port, while three implementation modules now separate manifest merging, path +projection, and Cargo execution. + Follow-up candidates (not in scope): rewrite version requirements in non-member nested manifests that depend on bumped crates. @@ -289,14 +309,22 @@ pieces: `CargoLockfileRepository` adapter performs discovery and merges configured manifests with manifests implied by tracked `Cargo.lock` files before either dry-run projection or live regeneration. -- `lading/commands/bump_lockfiles.py` defines the `LockfileRepository` port, - its `CargoLockfileRepository` adapter, and the regeneration helpers. - `resolve_lockfile_paths(workspace_root, lockfile_manifests)` maps the merged - manifest set to lockfile paths for dry-run reporting. - `regenerate_lockfiles(workspace_root, lockfile_manifests, *, runner=None)` - validates each manifest path (must stay inside the workspace and be named - `Cargo.toml`), always prepends the workspace root manifest, de-duplicates, - and runs `cargo update --workspace --manifest-path ` per entry. +- `lading/commands/bump_lockfiles.py` is the public compatibility façade. It + re-exports the established lockfile functions and exception, defines the + `LockfileRepository` port, and implements its `CargoLockfileRepository` + adapter. The adapter calls the façade-level names so tests and callers can + continue to replace them at that established seam. +- `lading/commands/bump_lockfile_manifests.py` implements + `merge_discovered_manifests`, preserving configured order before sorted, + de-duplicated discovery results. +- `lading/commands/bump_lockfile_paths.py` defines + `LockfileRegenerationError`, validates manifest paths, prepends the workspace + root, de-duplicates resolved paths, and projects them to lockfile paths for + dry-run reporting. +- `lading/commands/bump_lockfile_regeneration.py` implements + `regenerate_lockfiles` and its Cargo execution and failure-aggregation + helpers. It uses the path module's shared validation and error type, then runs + `cargo update --workspace --manifest-path ` per entry. - `lading/commands/lockfile.py` owns discovery and freshness validation. Publish uses discovery for freshness validation, while the bump-side `CargoLockfileRepository` uses it to construct the merged manifest set. @@ -378,28 +406,30 @@ Stage B (red): specify the behaviour with failing tests. Run the focused tests and confirm each fails for the expected reason before Stage C. -Stage C (green): implement the minimal change. +Stage C (green, historical delivery): implement the minimal feature change. -1. In `lading/commands/bump_lockfiles.py`, add the discovery merge function. - It imports `discover_tracked_lockfiles` from `lading.commands.lockfile`, - defaults its runner to `lading.runtime.subprocess_runner` when `None` - (matching `regenerate_lockfiles`), converts each discovered lockfile path to +1. Add the discovery merge function at the public + `lading.commands.bump_lockfiles` import path. The completed size refactor + implements it in `bump_lockfile_manifests.py` and re-exports it from the + façade. It imports `discover_tracked_lockfiles` from + `lading.commands.lockfile`, defaults its runner to + `lading.runtime.subprocess_runner` when `None` (matching + `regenerate_lockfiles`), converts each discovered lockfile path to `(lockfile.parent / "Cargo.toml").relative_to(workspace_root)` rendered as a POSIX string, sorts the discovered entries for determinism, and returns configured entries first followed by unseen discovered entries. The existing - `_resolve_manifest_paths` continues to prepend the root manifest and - de-duplicate resolved paths, so the root lockfile stays first regardless of - what discovery returns. + The path implementation in `bump_lockfile_paths.py` continues to prepend the + root manifest and de-duplicate resolved paths, so the root lockfile stays + first regardless of what discovery returns. 2. In `CargoLockfileRepository`, call the merge function before delegating to either the dry-run `resolve_lockfile_paths` helper or the live `regenerate_lockfiles` helper. Keep `lading/commands/bump.py::_process_lockfiles` dependent only on the `LockfileRepository` port, so dry-run and live runs use the same set without introducing Git or Cargo execution into the bump domain. -3. Update module docstrings that describe the old contract: the header of - `bump_lockfiles.py` ("any configured nested lockfiles") and the call-graph - paragraph in `lockfile.py` (which said before this change that discovery was - publish-only). +3. Update module docstrings that describe the old contract: the public façade + header and the call-graph paragraph in `lockfile.py` (which said before this + change that discovery was publish-only). Stage D (refactor, documentation, cleanup): @@ -430,6 +460,14 @@ gated commits): Stage B red tests are committed together with Stage C so the suite never lands red; Stage D lands as one or two focused commits (behaviour message + docs may be separate). +Stage E (completed size refactor): extract manifest merging into +`bump_lockfile_manifests.py`, path validation and projection into +`bump_lockfile_paths.py`, and Cargo execution into +`bump_lockfile_regeneration.py`. Keep `bump_lockfiles.py` as the compatibility +façade and repository boundary. Re-export the public functions and exception +without changing signatures, and keep the adapter calling façade-level names to +preserve runner injection and monkeypatch behaviour. + ## Concrete steps All commands run from the repository root @@ -556,9 +594,11 @@ real fixture packages with their own lockfiles already have this. ## Interfaces and dependencies -No new dependencies. At the end of Stage C the following must exist: +No new dependencies. The final public interface remains available from +`lading/commands/bump_lockfiles.py`, whose implementation imports the following +definition from `bump_lockfile_manifests.py`: -In `lading/commands/bump_lockfiles.py`: +Publicly re-exported from `lading/commands/bump_lockfiles.py`: ```python def merge_discovered_manifests( @@ -577,10 +617,14 @@ def merge_discovered_manifests( """ ``` -`CargoLockfileRepository` calls it before delegating the merged result to the -existing `resolve_lockfile_paths` helper for dry-run projection or -`regenerate_lockfiles` for live regeneration. The bump-side -`_process_lockfiles` function depends only on the `LockfileRepository` port. +`bump_lockfile_paths.py` defines and the façade re-exports +`LockfileRegenerationError` and `resolve_lockfile_paths`. +`bump_lockfile_regeneration.py` defines and the façade re-exports +`regenerate_lockfiles`; it imports the shared exception and private manifest +resolver from the path module. `CargoLockfileRepository` calls the façade-level +merge function before delegating the merged result through the façade-level +projection or regeneration function. The bump-side `_process_lockfiles` +function depends only on the `LockfileRepository` port. ## Revision note @@ -602,3 +646,9 @@ new diagnostics. The non-member manifest rewriting follow-up remains open. regeneration operation; `_process_lockfiles` remains dependent only on the port. This preserves the discovery behaviour delivered by the plan while keeping Git and Cargo execution outside the bump domain. + +2026-07-29: split the oversized lockfile implementation into cohesive manifest, +path, and regeneration modules. `bump_lockfiles.py` remains the compatibility +façade and repository boundary, so public imports and monkeypatch seams remain +stable. The refactor changes no behaviour and leaves no production module over +400 lines. diff --git a/lading/commands/bump_lockfile_manifests.py b/lading/commands/bump_lockfile_manifests.py new file mode 100644 index 00000000..d5d6f232 --- /dev/null +++ b/lading/commands/bump_lockfile_manifests.py @@ -0,0 +1,66 @@ +"""Manifest discovery and merging for Cargo lockfile regeneration.""" + +from __future__ import annotations + +import collections.abc as cabc +from pathlib import Path + +from lading.commands.lockfile import discover_tracked_lockfiles +from lading.runtime import CommandRunner, subprocess_runner + + +def merge_discovered_manifests( + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], + *, + runner: CommandRunner | None = None, +) -> tuple[str, ...]: + """Return configured manifests plus discovered tracked-lockfile manifests. + + Parameters + ---------- + workspace_root : Path + Absolute path to the Cargo workspace root. + lockfile_manifests : Sequence[str] + Configured manifest paths relative to *workspace_root*. + runner : CommandRunner or None, optional + Callable used to invoke ``git``. Defaults to + :func:`lading.runtime.subprocess_runner` when ``None``. + + Returns + ------- + tuple[str, ...] + The configured entries in their original order, followed by + workspace-relative POSIX manifest paths implied by git-tracked + ``Cargo.lock`` files (via + :func:`lading.commands.lockfile.discover_tracked_lockfiles`) in + sorted order, skipping any manifest already configured under an + equivalent spelling. In a non-git workspace the configured tuple is + returned unchanged. + + Examples + -------- + With ``fixtures/minimal/Cargo.lock`` tracked in git: + + ```python + merge_discovered_manifests(workspace_root, ("crates/ui/Cargo.toml",)) + # ("crates/ui/Cargo.toml", "Cargo.toml", "fixtures/minimal/Cargo.toml") + ``` + """ + command_runner = subprocess_runner if runner is None else runner + discovered = discover_tracked_lockfiles(workspace_root, command_runner) + seen_manifests = { + (workspace_root / manifest).resolve() for manifest in lockfile_manifests + } + merged = list(lockfile_manifests) + discovered_relative = sorted( + (lockfile_path.parent / "Cargo.toml").relative_to(workspace_root).as_posix() + for lockfile_path in discovered + ) + for relative_manifest in discovered_relative: + resolved = (workspace_root / relative_manifest).resolve() + if resolved in seen_manifests: + continue + seen_manifests.add(resolved) + merged.append(relative_manifest) + return tuple(merged) diff --git a/lading/commands/bump_lockfile_paths.py b/lading/commands/bump_lockfile_paths.py new file mode 100644 index 00000000..a1f7c59c --- /dev/null +++ b/lading/commands/bump_lockfile_paths.py @@ -0,0 +1,51 @@ +"""Path validation and projection for Cargo lockfile regeneration.""" + +from __future__ import annotations + +import collections.abc as cabc +from pathlib import Path + +from lading.exceptions import LadingError + + +class LockfileRegenerationError(LadingError): + """Raise when lockfile regeneration cannot validate or execute.""" + + +def resolve_lockfile_paths( + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], +) -> tuple[Path, ...]: + """Return lockfile paths implied by configured manifest paths.""" + manifests = _resolve_manifest_paths(workspace_root, lockfile_manifests) + return tuple(manifest.parent / "Cargo.lock" for manifest in manifests) + + +def _resolve_manifest_paths( + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], +) -> tuple[Path, ...]: + """Return validated root and configured manifest paths in execution order.""" + resolved_root = workspace_root.resolve() + root_manifest = (workspace_root / "Cargo.toml").resolve() + seen_manifests: set[Path] = {root_manifest} + manifests = [root_manifest] + for manifest in lockfile_manifests: + candidate = (workspace_root / manifest).resolve() + try: + candidate.relative_to(resolved_root) + except ValueError as exc: + message = ( + f"Lockfile manifest path must stay within the workspace: {manifest}" + ) + raise LockfileRegenerationError(message) from exc + if candidate.name != "Cargo.toml": + message = ( + f"Lockfile manifest path must point to a Cargo.toml file: {manifest}" + ) + raise LockfileRegenerationError(message) + if candidate in seen_manifests: + continue + seen_manifests.add(candidate) + manifests.append(candidate) + return tuple(manifests) diff --git a/lading/commands/bump_lockfile_regeneration.py b/lading/commands/bump_lockfile_regeneration.py new file mode 100644 index 00000000..c123ebba --- /dev/null +++ b/lading/commands/bump_lockfile_regeneration.py @@ -0,0 +1,194 @@ +"""Cargo invocation and failure handling for lockfile regeneration.""" + +from __future__ import annotations + +import collections.abc as cabc +import dataclasses as dc +import logging +import shlex +import time +import typing as typ +from pathlib import Path + +from lading.commands.bump_lockfile_paths import ( + LockfileRegenerationError, + _resolve_manifest_paths, +) +from lading.runtime import CommandRunner, subprocess_runner +from lading.utils.process import with_detail + +_LOGGER = logging.getLogger(__name__) + + +def regenerate_lockfiles( + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], + *, + runner: CommandRunner | None = None, +) -> tuple[Path, ...]: + """Regenerate Cargo lockfiles for root and configured manifests. + + Parameters + ---------- + workspace_root : Path + Absolute path to the Cargo workspace root. + lockfile_manifests : Sequence[str] + Configured manifest paths relative to *workspace_root*. The workspace + root ``Cargo.toml`` is always prepended and de-duplicated. + runner : CommandRunner or None, optional + Callable used to invoke ``cargo``. Defaults to + :func:`lading.runtime.subprocess_runner` when ``None``. + + Returns + ------- + tuple[Path, ...] + Paths to every ``Cargo.lock`` file that was regenerated, in manifest + execution order. + + Raises + ------ + LockfileRegenerationError + If any configured manifest path is invalid (outside the workspace or + 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. + + Notes + ----- + **Partial-update semantics:** regeneration is not atomic and successful + updates are not rolled back when a later manifest fails. Every manifest + is attempted (issue #84), so a single cargo failure does not leave + unrelated lockfiles silently stale. When several lockfiles are + regenerated, the aggregated error tells the operator exactly which + lockfiles still need repair and how; a lone root-lockfile failure needs no + such disambiguation and surfaces the plain cargo error. + """ + command_runner = subprocess_runner if runner is None else runner + manifests = _resolve_manifest_paths(workspace_root, lockfile_manifests) + started_at = time.perf_counter() + _LOGGER.info("Regenerating %d Cargo lockfile(s)", len(manifests)) + lockfiles, failures = _collect_lockfile_results( + manifests, workspace_root, command_runner + ) + if failures: + _raise_aggregated_failure(manifests, failures) + _LOGGER.info( + "Regenerated %d Cargo lockfile(s) in %.3fs", + len(lockfiles), + time.perf_counter() - started_at, + ) + return tuple(lockfiles) + + +def _collect_lockfile_results( + manifests: tuple[Path, ...], + workspace_root: Path, + command_runner: CommandRunner, +) -> tuple[list[Path], list[tuple[Path, LockfileRegenerationError]]]: + """Attempt every manifest and partition the outcomes into a pair.""" + lockfiles: list[Path] = [] + failures: list[tuple[Path, LockfileRegenerationError]] = [] + for manifest in manifests: + result = _attempt_single_lockfile_update( + workspace_root, manifest, command_runner + ) + if result.error is not None: + failures.append((manifest, result.error)) + elif result.lockfile is not None: + lockfiles.append(result.lockfile) + return lockfiles, failures + + +def _raise_aggregated_failure( + manifests: tuple[Path, ...], + failures: cabc.Sequence[tuple[Path, LockfileRegenerationError]], +) -> typ.NoReturn: + """Raise the appropriate error for one or more failed manifests.""" + if len(manifests) == 1: + raise failures[0][1] + cause = failures[0][1].__cause__ or failures[0][1] + raise LockfileRegenerationError( + _build_aggregate_failure_message(failures) + ) from cause + + +def _build_aggregate_failure_message( + failures: cabc.Sequence[tuple[Path, LockfileRegenerationError]], +) -> str: + """Return the aggregated operator-facing regeneration failure message.""" + header = ( + f"Cargo lockfile regeneration failed for {len(failures)} manifest(s). " + "Manifests already carry the new version, so the workspace is " + "inconsistent until each lockfile below is repaired:" + ) + lines = [header] + for manifest, error in failures: + lines.append(f"- {error}") + quoted_manifest = shlex.quote(str(manifest)) + lines.append(f" cargo update --workspace --manifest-path {quoted_manifest}") + return "\n".join(lines) + + +@dc.dataclass(frozen=True, slots=True) +class _LockfileUpdateResult: + """Outcome of attempting one manifest's lockfile update.""" + + lockfile: Path | None + error: LockfileRegenerationError | None + + +def _attempt_single_lockfile_update( + workspace_root: Path, + manifest: Path, + command_runner: CommandRunner, +) -> _LockfileUpdateResult: + """Attempt one manifest's lockfile update and return the outcome.""" + manifest_started_at = time.perf_counter() + _LOGGER.info("Regenerating Cargo lockfile for %s", manifest) + try: + _run_workspace_lockfile_update(workspace_root, manifest, command_runner) + except LockfileRegenerationError as exc: + # Keep going: attempting the remaining manifests gives the + # operator one aggregated repair list instead of a re-run per + # failure (issue #84). + _LOGGER.exception("Cargo lockfile regeneration failed") + return _LockfileUpdateResult(lockfile=None, error=exc) + _LOGGER.info( + "Regenerated Cargo lockfile for %s in %.3fs", + manifest, + time.perf_counter() - manifest_started_at, + ) + return _LockfileUpdateResult(lockfile=manifest.parent / "Cargo.lock", error=None) + + +def _run_workspace_lockfile_update( + workspace_root: Path, + manifest_path: Path, + runner: CommandRunner, +) -> None: + """Invoke cargo for one manifest and surface failures consistently.""" + try: + exit_code, stdout, stderr = runner( + ( + "cargo", + "update", + "--workspace", + "--manifest-path", + str(manifest_path), + ), + cwd=workspace_root, + ) + except Exception as exc: + message = f"Cargo lockfile regeneration failed for {manifest_path}: {exc}" + raise LockfileRegenerationError(message) from exc + if exit_code != 0: + message = with_detail( + "Cargo lockfile regeneration failed for " + f"{manifest_path} with exit code {exit_code}", + stdout, + stderr, + ) + raise LockfileRegenerationError(message) diff --git a/lading/commands/bump_lockfiles.py b/lading/commands/bump_lockfiles.py index e7664ce9..1d84ede0 100644 --- a/lading/commands/bump_lockfiles.py +++ b/lading/commands/bump_lockfiles.py @@ -1,302 +1,27 @@ -"""Lockfile regeneration helpers for ``lading bump``. +"""Public lockfile regeneration façade for ``lading bump``. -The public entry points are :func:`merge_discovered_manifests`, which unions -the configured ``bump.lockfile_manifests`` entries with manifests implied by -git-tracked ``Cargo.lock`` files, and :func:`regenerate_lockfiles`, which -rebuilds the workspace root lockfile and every merged nested lockfile after -manifest versions change. +The implementation is split by responsibility while this module retains the +established public imports and the bump-side repository port and adapter. """ from __future__ import annotations import collections.abc as cabc import dataclasses as dc -import logging -import shlex -import time import typing as typ from pathlib import Path -from lading.commands.lockfile import discover_tracked_lockfiles -from lading.exceptions import LadingError -from lading.runtime import CommandRunner, subprocess_runner -from lading.utils.process import with_detail - -_LOGGER = logging.getLogger(__name__) - - -class LockfileRegenerationError(LadingError): - """Raise when lockfile regeneration cannot validate or execute.""" - - -def merge_discovered_manifests( - workspace_root: Path, - lockfile_manifests: cabc.Sequence[str], - *, - runner: CommandRunner | None = None, -) -> tuple[str, ...]: - """Return configured manifests plus discovered tracked-lockfile manifests. - - Parameters - ---------- - workspace_root : Path - Absolute path to the Cargo workspace root. - lockfile_manifests : Sequence[str] - Configured manifest paths relative to *workspace_root*. - runner : CommandRunner or None, optional - Callable used to invoke ``git``. Defaults to - :func:`lading.runtime.subprocess_runner` when ``None``. - - Returns - ------- - tuple[str, ...] - The configured entries in their original order, followed by - workspace-relative POSIX manifest paths implied by git-tracked - ``Cargo.lock`` files (via - :func:`lading.commands.lockfile.discover_tracked_lockfiles`) in - sorted order, skipping any manifest already configured under an - equivalent spelling. In a non-git workspace the configured tuple is - returned unchanged. - - Examples - -------- - With ``fixtures/minimal/Cargo.lock`` tracked in git: - - ```python - merge_discovered_manifests(workspace_root, ("crates/ui/Cargo.toml",)) - # ("crates/ui/Cargo.toml", "Cargo.toml", "fixtures/minimal/Cargo.toml") - ``` - """ - command_runner = subprocess_runner if runner is None else runner - discovered = discover_tracked_lockfiles(workspace_root, command_runner) - seen_manifests = { - (workspace_root / manifest).resolve() for manifest in lockfile_manifests - } - merged = list(lockfile_manifests) - discovered_relative = sorted( - (lockfile_path.parent / "Cargo.toml").relative_to(workspace_root).as_posix() - for lockfile_path in discovered - ) - for relative_manifest in discovered_relative: - resolved = (workspace_root / relative_manifest).resolve() - if resolved in seen_manifests: - continue - seen_manifests.add(resolved) - merged.append(relative_manifest) - return tuple(merged) - - -def resolve_lockfile_paths( - workspace_root: Path, - lockfile_manifests: cabc.Sequence[str], -) -> tuple[Path, ...]: - """Return lockfile paths implied by configured manifest paths.""" - manifests = _resolve_manifest_paths(workspace_root, lockfile_manifests) - return tuple(manifest.parent / "Cargo.lock" for manifest in manifests) - - -def regenerate_lockfiles( - workspace_root: Path, - lockfile_manifests: cabc.Sequence[str], - *, - runner: CommandRunner | None = None, -) -> tuple[Path, ...]: - """Regenerate Cargo lockfiles for root and configured manifests. - - Parameters - ---------- - workspace_root : Path - Absolute path to the Cargo workspace root. - lockfile_manifests : Sequence[str] - Configured manifest paths relative to *workspace_root*. The workspace - root ``Cargo.toml`` is always prepended and de-duplicated. - runner : CommandRunner or None, optional - Callable used to invoke ``cargo``. Defaults to - :func:`lading.runtime.subprocess_runner` when ``None``. - - Returns - ------- - tuple[Path, ...] - Paths to every ``Cargo.lock`` file that was regenerated, in manifest - execution order. - - Raises - ------ - LockfileRegenerationError - If any configured manifest path is invalid (outside the workspace or - 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. - - Notes - ----- - **Partial-update semantics:** regeneration is not atomic and successful - updates are not rolled back when a later manifest fails. Every manifest - is attempted (issue #84), so a single cargo failure does not leave - unrelated lockfiles silently stale. When several lockfiles are - regenerated, the aggregated error tells the operator exactly which - lockfiles still need repair and how; a lone root-lockfile failure needs no - such disambiguation and surfaces the plain cargo error. - """ - command_runner = subprocess_runner if runner is None else runner - manifests = _resolve_manifest_paths(workspace_root, lockfile_manifests) - started_at = time.perf_counter() - _LOGGER.info("Regenerating %d Cargo lockfile(s)", len(manifests)) - lockfiles, failures = _collect_lockfile_results( - manifests, workspace_root, command_runner - ) - if failures: - _raise_aggregated_failure(manifests, failures) - _LOGGER.info( - "Regenerated %d Cargo lockfile(s) in %.3fs", - len(lockfiles), - time.perf_counter() - started_at, - ) - return tuple(lockfiles) - - -def _collect_lockfile_results( - manifests: tuple[Path, ...], - workspace_root: Path, - command_runner: CommandRunner, -) -> tuple[list[Path], list[tuple[Path, LockfileRegenerationError]]]: - """Attempt every manifest and partition the outcomes into a pair.""" - lockfiles: list[Path] = [] - failures: list[tuple[Path, LockfileRegenerationError]] = [] - for manifest in manifests: - result = _attempt_single_lockfile_update( - workspace_root, manifest, command_runner - ) - if result.error is not None: - failures.append((manifest, result.error)) - elif result.lockfile is not None: - lockfiles.append(result.lockfile) - return lockfiles, failures - - -def _raise_aggregated_failure( - manifests: tuple[Path, ...], - failures: cabc.Sequence[tuple[Path, LockfileRegenerationError]], -) -> typ.NoReturn: - """Raise the appropriate error for one or more failed manifests.""" - if len(manifests) == 1: - raise failures[0][1] - cause = failures[0][1].__cause__ or failures[0][1] - raise LockfileRegenerationError( - _build_aggregate_failure_message(failures) - ) from cause - - -def _build_aggregate_failure_message( - failures: cabc.Sequence[tuple[Path, LockfileRegenerationError]], -) -> str: - """Return the aggregated operator-facing regeneration failure message.""" - header = ( - f"Cargo lockfile regeneration failed for {len(failures)} manifest(s). " - "Manifests already carry the new version, so the workspace is " - "inconsistent until each lockfile below is repaired:" - ) - lines = [header] - for manifest, error in failures: - lines.append(f"- {error}") - quoted_manifest = shlex.quote(str(manifest)) - lines.append(f" cargo update --workspace --manifest-path {quoted_manifest}") - return "\n".join(lines) - - -def _resolve_manifest_paths( - workspace_root: Path, - lockfile_manifests: cabc.Sequence[str], -) -> tuple[Path, ...]: - """Return validated root and configured manifest paths in execution order.""" - resolved_root = workspace_root.resolve() - root_manifest = (workspace_root / "Cargo.toml").resolve() - seen_manifests: set[Path] = {root_manifest} - manifests = [root_manifest] - for manifest in lockfile_manifests: - candidate = (workspace_root / manifest).resolve() - try: - candidate.relative_to(resolved_root) - except ValueError as exc: - message = ( - f"Lockfile manifest path must stay within the workspace: {manifest}" - ) - raise LockfileRegenerationError(message) from exc - if candidate.name != "Cargo.toml": - message = ( - f"Lockfile manifest path must point to a Cargo.toml file: {manifest}" - ) - raise LockfileRegenerationError(message) - if candidate in seen_manifests: - continue - seen_manifests.add(candidate) - manifests.append(candidate) - return tuple(manifests) - - -@dc.dataclass(frozen=True, slots=True) -class _LockfileUpdateResult: - """Outcome of attempting one manifest's lockfile update.""" - - lockfile: Path | None - error: LockfileRegenerationError | None - - -def _attempt_single_lockfile_update( - workspace_root: Path, - manifest: Path, - command_runner: CommandRunner, -) -> _LockfileUpdateResult: - """Attempt one manifest's lockfile update and return the outcome.""" - manifest_started_at = time.perf_counter() - _LOGGER.info("Regenerating Cargo lockfile for %s", manifest) - try: - _run_workspace_lockfile_update(workspace_root, manifest, command_runner) - except LockfileRegenerationError as exc: - # Keep going: attempting the remaining manifests gives the - # operator one aggregated repair list instead of a re-run per - # failure (issue #84). - _LOGGER.exception("Cargo lockfile regeneration failed") - return _LockfileUpdateResult(lockfile=None, error=exc) - _LOGGER.info( - "Regenerated Cargo lockfile for %s in %.3fs", - manifest, - time.perf_counter() - manifest_started_at, - ) - return _LockfileUpdateResult(lockfile=manifest.parent / "Cargo.lock", error=None) - - -def _run_workspace_lockfile_update( - workspace_root: Path, - manifest_path: Path, - runner: CommandRunner, -) -> None: - """Invoke cargo for one manifest and surface failures consistently.""" - try: - exit_code, stdout, stderr = runner( - ( - "cargo", - "update", - "--workspace", - "--manifest-path", - str(manifest_path), - ), - cwd=workspace_root, - ) - except Exception as exc: - message = f"Cargo lockfile regeneration failed for {manifest_path}: {exc}" - raise LockfileRegenerationError(message) from exc - if exit_code != 0: - message = with_detail( - "Cargo lockfile regeneration failed for " - f"{manifest_path} with exit code {exit_code}", - stdout, - stderr, - ) - raise LockfileRegenerationError(message) +from lading.commands.bump_lockfile_manifests import merge_discovered_manifests +from lading.commands.bump_lockfile_paths import ( + LockfileRegenerationError as LockfileRegenerationError, +) +from lading.commands.bump_lockfile_paths import ( + resolve_lockfile_paths, +) +from lading.commands.bump_lockfile_regeneration import regenerate_lockfiles + +if typ.TYPE_CHECKING: + from lading.runtime import CommandRunner @dc.dataclass(frozen=True, slots=True)