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/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/developers-guide.md b/docs/developers-guide.md index f556a917..80c573c5 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`. @@ -203,10 +209,15 @@ 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. It always includes the workspace root `Cargo.toml`, -validates configured nested manifests before invoking Cargo, and de-duplicates -resolved manifest paths. +`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`, +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` @@ -258,14 +269,16 @@ 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 -`lockfile.LockfileInspectionRepository` port (see the Lockfile helpers section -below), so neither the bump nor the publish lockfile domain holds a raw -`CommandRunner` (issue #82). +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). Bump-time crate-set derivation is centralized in the bump context: the `excluded` and `updated_crate_names` sets are computed exactly once in @@ -369,8 +382,11 @@ 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 configured manifest. The two cargo strategies +`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`, 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. @@ -623,6 +639,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 new file mode 100644 index 00000000..db0620db --- /dev/null +++ b/docs/execplans/regenerate-lockfiles.md @@ -0,0 +1,654 @@ +# 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: COMPLETE + +## 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/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` + 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). 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) + +- 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 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 + 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. +- [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 (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: + `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. +- [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`. +- [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 + +- 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` 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 + "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: 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. +- 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 + +- 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 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`); + 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. +- 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. +- 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 + +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, 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 +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 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. + +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. + +## 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. 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` 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. + `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 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 + 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, historical delivery): implement the minimal feature change. + +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 + 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 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): + +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 + 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 + 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). + +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 +(`/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. + +## 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 +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. The final public interface remains available from +`lading/commands/bump_lockfiles.py`, whose implementation imports the following +definition from `bump_lockfile_manifests.py`: + +Publicly re-exported from `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. + """ +``` + +`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 + +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 +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 +`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. + +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/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/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. diff --git a/docs/users-guide.md b/docs/users-guide.md index 988741bc..bcb7df24 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -86,14 +86,17 @@ 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 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 @@ -115,15 +118,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 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 @@ -306,25 +311,31 @@ 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. 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 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/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..ddbd9ada --- /dev/null +++ b/lading/commands/bump_lockfile_paths.py @@ -0,0 +1,94 @@ +"""Validate Cargo manifest paths and project their lockfile targets. + +This module keeps workspace-boundary and ``Cargo.toml`` validation shared +between dry-run projection and live regeneration. Dry runs use +:func:`resolve_lockfile_paths` to report the root and configured lockfiles +without invoking Cargo. + +Examples +-------- +Project the lockfiles that a configured nested manifest would update: + +```python +resolve_lockfile_paths(workspace_root, ("fixtures/example/Cargo.toml",)) +``` +""" + +from __future__ import annotations + +import collections.abc as cabc +from pathlib import Path + +from lading.exceptions import LadingError + + +class LockfileRegenerationError(LadingError): + """Report that Cargo lockfile regeneration cannot be prepared or completed. + + Notes + ----- + This exception derives from :class:`lading.exceptions.LadingError`. It is + raised when configured lockfile-regeneration manifests cannot be validated + or when regeneration cannot proceed. + + """ + + +def resolve_lockfile_paths( + workspace_root: Path, + lockfile_manifests: cabc.Sequence[str], +) -> tuple[Path, ...]: + """Return lockfile paths implied by configured manifest paths. + + Parameters + ---------- + workspace_root : Path + Absolute path to the Cargo workspace root. + lockfile_manifests : Sequence[str] + Configured manifest paths relative to *workspace_root*. + + Returns + ------- + tuple[Path, ...] + Execution-ordered, de-duplicated paths to the implied ``Cargo.lock`` + files, with the workspace-root lockfile first. + + Raises + ------ + LockfileRegenerationError + If a configured manifest escapes the workspace or is not named + ``Cargo.toml``. + + """ + 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..d84ff752 --- /dev/null +++ b/lading/commands/bump_lockfile_regeneration.py @@ -0,0 +1,212 @@ +"""Regenerate Cargo lockfiles and report failures across all manifests. + +This module invokes ``cargo update --workspace`` for each validated manifest, +records partial successes, and aggregates multiple failures after every target +has been attempted. It reuses path validation and the shared regeneration error +from :mod:`lading.commands.bump_lockfile_paths`. + +Examples +-------- +Regenerate the root and one configured nested lockfile with an injected runner: + +```python +regenerate_lockfiles( + workspace_root, + ("fixtures/example/Cargo.toml",), + runner=runner, +) +``` +""" + +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, CommandSpawnError, 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 (CommandSpawnError, ValueError) 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 7f518df9..1d84ede0 100644 --- a/lading/commands/bump_lockfiles.py +++ b/lading/commands/bump_lockfiles.py @@ -1,242 +1,27 @@ -"""Lockfile regeneration helpers for ``lading bump``. +"""Public lockfile regeneration façade 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 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.exceptions import LadingError -from lading.runtime import CommandRunner, subprocess_runner -from lading.utils.process import with_detail +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 -_LOGGER = logging.getLogger(__name__) - - -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 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) +if typ.TYPE_CHECKING: + from lading.runtime import CommandRunner @dc.dataclass(frozen=True, slots=True) @@ -257,9 +42,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 ------- @@ -274,7 +59,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, @@ -288,9 +76,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 ------- @@ -308,9 +96,12 @@ 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..e3c32f13 100644 --- a/lading/commands/lockfile.py +++ b/lading/commands/lockfile.py @@ -13,10 +13,13 @@ 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 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 ``cargo metadata --locked`` purely as a read-only freshness probe. The publish pre-flight domain reaches these operations through the diff --git a/lading/commands/publish_index_check.py b/lading/commands/publish_index_check.py index 32e170eb..cfd05882 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) } @@ -366,9 +366,23 @@ def _handle_index_missing_version( ) missing_name = failure.missing_dependency_name - if missing_name is None: - _raise_name_extraction_failure(context) + if missing_name is not None: + _downgrade_or_raise( + failure, context=context, handling=handling, missing_name=missing_name + ) + return + _raise_name_extraction_failure(context) + return + +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( @@ -381,14 +395,13 @@ def _handle_index_missing_version( 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) + return 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/bdd/features/cli.feature b/tests/bdd/features/cli.feature index b83c49bb..644d23f9 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 workspace and nested 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..73c57c94 100644 --- a/tests/bdd/steps/test_bump_steps.py +++ b/tests/bdd/steps/test_bump_steps.py @@ -40,6 +40,55 @@ 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. + + 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 + + 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 + # 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="" + ) + + @when( parsers.parse("I invoke lading bump {version} with that workspace"), target_fixture="cli_run", @@ -173,6 +222,54 @@ 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, ( + f"stdout:\n{cli_run['stdout']}\nstderr:\n{cli_run['stderr']}" + ) + 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 7de50610..a6237a72 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -77,9 +77,44 @@ 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.""" + """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. + + Examples + -------- + For a test module listed in ``_LOCKFILE_STUB_MODULES``, pytest applies this + fixture automatically: + + ```python + manifests = ("crates/example/Cargo.toml",) + merged = bump.bump_lockfiles.merge_discovered_manifests(tmp_path, manifests) + assert merged == manifests + assert bump.bump_lockfiles.regenerate_lockfiles(tmp_path, merged) == () + ``` + + The scoped replacements mean neither Git discovery nor Cargo regeneration + runs in those manifest-focused test modules. + + """ 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/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 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..98f78a22 --- /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( + 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 + ) + ) + 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_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..50b510c5 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,16 +7,16 @@ 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 +from lading.runtime import CommandSpawnError if typ.TYPE_CHECKING: from syrupy.assertion import SnapshotAssertion @@ -135,39 +135,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"), [ @@ -251,8 +218,9 @@ def partial_runner( ) -def test_regenerate_lockfiles_wraps_runner_exceptions(tmp_path: Path) -> None: - """Runner exceptions should retain their cause for diagnostics.""" +def test_regenerate_lockfiles_wraps_command_spawn_errors(tmp_path: Path) -> None: + """Command spawn failures should retain context and their cause.""" + spawn_error = CommandSpawnError("cargo", FileNotFoundError("cargo")) def failing_runner( command: cabc.Sequence[str], @@ -260,165 +228,45 @@ def failing_runner( cwd: Path | None = None, ) -> tuple[int, str, str]: del command, cwd - message = "cargo executable not found" - raise OSError(message) + raise spawn_error - with pytest.raises( - bump_lockfiles.LockfileRegenerationError, - match="cargo executable not found", - ) as exc_info: + with pytest.raises(bump_lockfiles.LockfileRegenerationError) as exc_info: bump_lockfiles.regenerate_lockfiles( tmp_path, (), runner=failing_runner, ) - assert isinstance(exc_info.value.__cause__, OSError) - - -# --------------------------------------------------------------------------- -# 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" - ) + root_manifest = (tmp_path / "Cargo.toml").resolve() + expected_message = ( + f"Cargo lockfile regeneration failed for {root_manifest}: {spawn_error}" + ) + assert str(exc_info.value) == expected_message + assert exc_info.value.__cause__ is spawn_error -@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], +def test_regenerate_lockfiles_propagates_unexpected_runner_defects( + tmp_path: Path, ) -> 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) + """Unexpected injected-runner defects should escape unchanged.""" + defect = RuntimeError("runner invariant violated") - lockfiles = bump_lockfiles.resolve_lockfile_paths(workspace_root, spellings) - - assert lockfiles == (workspace_root.resolve() / "Cargo.lock",) + def defective_runner( + command: cabc.Sequence[str], + *, + cwd: Path | None = None, + ) -> tuple[int, str, str]: + del command, cwd + raise defect + with pytest.raises(RuntimeError) as exc_info: + bump_lockfiles.regenerate_lockfiles(tmp_path, (), runner=defective_runner) -@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,)) + assert exc_info.value is defect # --------------------------------------------------------------------------- -# Aggregated failure handling (issue #84) +# Hypothesis property tests for manifest-path resolution (issue #93) # --------------------------------------------------------------------------- @@ -558,7 +406,7 @@ def test_regenerate_lockfiles_aggregate_chains_first_underlying_cause( ab_workspace: Path, ) -> None: """The aggregated error chains from the first failure's underlying cause.""" - boom = OSError("cargo executable not found") + boom = CommandSpawnError("cargo", FileNotFoundError("cargo")) def failing_runner( command: cabc.Sequence[str],