Refresh stale Cargo lockfiles before publish preflight (#61) - #75
Merged
leynos merged 24 commits intoJun 8, 2026
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (35)
SummaryThis pull request implements automatic discovery, refresh, and validation of git-tracked Cargo lockfiles in the lading tool, addressing issue Key ChangesCore FunctionalityNew
Exception Hierarchy ConsolidationEstablishes
Testing and ValidationNew test suite (
Integration tests (
Publish pre-flight tests (new
BDD scenarios (
DocumentationUser-facing guidance (
Developer documentation (
Roadmap updates (
Design Decisions
Known Follow-upsThe PR defers several architectural improvements to tracked issues:
Closes issue WalkthroughThis PR implements Cargo lockfile discovery, refresh, and freshness validation across ChangesUnified exception hierarchy
Lockfile support
Test infrastructure and documentation updates
Sequence Diagram(s)sequenceDiagram
participant Bump as lading bump
participant Discover as discover_tracked_lockfiles
participant Git as git ls-files
participant Refresh as refresh_lockfile
participant Cargo as cargo generate-lockfile
Bump->>Discover: workspace_root, runner
Discover->>Git: ls-files Cargo.lock **/Cargo.lock
Git-->>Discover: stdout
Discover-->>Bump: sorted lockfile paths (filtered by target/ and manifest adjacency)
Bump->>Refresh: each manifest_path, runner
Refresh->>Cargo: --manifest-path manifest_dir/Cargo.toml
Cargo-->>Refresh: exit code, stderr
Refresh-->>Bump: Cargo.lock path or LockfileRefreshError
Bump-->>CLI: BumpChanges with lockfiles for output rendering
sequenceDiagram
participant Preflight as _run_preflight_checks
participant Validate as _validate_lockfile_freshness
participant Discover as discover_tracked_lockfiles
participant Freshness as validate_lockfile_freshness
participant CargoMeta as cargo metadata --locked
Preflight->>Validate: workspace_root, runner, env
Validate->>Discover: workspace_root, runner
Discover-->>Validate: lockfile paths
loop each lockfile
Validate->>Freshness: manifest_path, runner with env
Freshness->>CargoMeta: --locked --manifest-path manifest_dir/Cargo.toml
CargoMeta-->>Freshness: exit code
Freshness-->>Validate: boolean (fresh or stale)
end
alt any stale
Validate-->>Preflight: raise PublishPreflightError with cargo generate-lockfile commands
else all fresh
Validate-->>Preflight: return (continue to cargo check/test)
end
Possibly related issues
Possibly related PRs
Suggested labels
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +19 to +58 def discover_tracked_lockfiles(
workspace_root: Path,
runner: _CommandRunner,
) -> tuple[Path, ...]:
"""Return tracked Cargo.lock files with adjacent manifests."""
candidate_lockfiles = tuple(
path
for path in workspace_root.rglob("Cargo.lock")
if "target" not in path.relative_to(workspace_root).parts
)
if not candidate_lockfiles:
return ()
exit_code, stdout, stderr = runner(
("git", "ls-files", "*/Cargo.lock", "Cargo.lock"),
cwd=workspace_root,
)
if exit_code != 0:
detail = (stderr or stdout).strip()
if "not a git repository" in detail.lower():
LOGGER.warning(
"Skipping Cargo.lock discovery because %s is not a git repository",
workspace_root,
)
return ()
LOGGER.warning(
"Skipping Cargo.lock discovery after git ls-files failed: %s", detail
)
return ()
lockfiles: list[Path] = []
for line in stdout.splitlines():
relative_path = line.strip()
if not relative_path:
continue
lockfile_path = workspace_root / relative_path
manifest_path = lockfile_path.parent / "Cargo.toml"
if manifest_path.exists():
lockfiles.append(lockfile_path)
return tuple(lockfiles)❌ New issue: Complex Method |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Comment on lines +19 to +58 def discover_tracked_lockfiles(
workspace_root: Path,
runner: _CommandRunner,
) -> tuple[Path, ...]:
"""Return tracked Cargo.lock files with adjacent manifests."""
candidate_lockfiles = tuple(
path
for path in workspace_root.rglob("Cargo.lock")
if "target" not in path.relative_to(workspace_root).parts
)
if not candidate_lockfiles:
return ()
exit_code, stdout, stderr = runner(
("git", "ls-files", "*/Cargo.lock", "Cargo.lock"),
cwd=workspace_root,
)
if exit_code != 0:
detail = (stderr or stdout).strip()
if "not a git repository" in detail.lower():
LOGGER.warning(
"Skipping Cargo.lock discovery because %s is not a git repository",
workspace_root,
)
return ()
LOGGER.warning(
"Skipping Cargo.lock discovery after git ls-files failed: %s", detail
)
return ()
lockfiles: list[Path] = []
for line in stdout.splitlines():
relative_path = line.strip()
if not relative_path:
continue
lockfile_path = workspace_root / relative_path
manifest_path = lockfile_path.parent / "Cargo.toml"
if manifest_path.exists():
lockfiles.append(lockfile_path)
return tuple(lockfiles)❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
June 6, 2026 23:34
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This was referenced Jun 7, 2026
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@coderabbitai review |
✅ Action performedReview finished.
|
Update partial-refresh recovery guidance, log successful lockfile freshness validation, and exercise the real validation helper in preflight tests. Keep the runner environment propagation assertion intact.
Document that rerunning lading bump after a partial lockfile refresh failure will not refresh lockfiles unless manifests are rewritten, and point users to cargo generate-lockfile for repair.
Require dry_run to be passed by keyword in bump output formatting helpers and remove the positional-boolean lint suppressions.
Assert the bump CLI leaves the injectable command runner unset so the command layer uses its production default while lower-level tests can still inject stubs.
Use a recursive git pathspec for tracked Cargo.lock discovery and align test stubs with that command. Update stale-lockfile recovery text to point at direct cargo generate-lockfile repair.
Inject the manifest-existence probe used during lockfile discovery so filesystem checks are explicit and replaceable. Add observability for lockfile discovery, refresh, and validation, plus regression coverage for partial refresh failures. Lock bump and publish remediation formatting with syrupy snapshots.
Expand the developer guide exception hierarchy section with package-level LadingError guidance, a reuse plan for new domain exceptions, and caller/test usage rules.
Move command utility, cargo runner, metadata decoding, and lockfile validation tests out of `test_preflight_checks.py` so the remaining file covers only pre-flight orchestration. Relocate the syrupy snapshot with the moved lockfile-validation test module while preserving the snapshot key and content.
Replace four structurally identical cargo preflight command tests with a single parametrized scenario table. Keep the failure and diagnostic tests unchanged while preserving the same option combinations and expected command arguments.
Expand the package exception module documentation so `LadingError` is clearly identified as the common root for lading domain failures. Add bump command internals guidance covering the `BumpOptions.runner` injection point and the `BumpChanges.lockfiles` summary field.
Add the missing docstring to the lockfile discovery runner closure so the project docstring coverage check stays above the required threshold.
Move the shared `_CommandRunner` protocol behind a neutral `_runners` module and point bump, publish, preflight, and lockfile typing at that boundary. Leave the concrete subprocess and cmd-mox implementation in `publish_execution`, with `_runners._invoke` delegating there at call time. Document the boundary in the developer guide so bump no longer appears to depend on publish-specific execution internals for lockfile refreshes.
Add Hypothesis as a development dependency and cover the lockfile filtering helper with generated git stdout cases. Check that returned lockfiles never include `target` components, only pass through approved adjacent manifests, and always originate from non-empty git stdout lines.
Keep the issue 61 lockfile behaviour aligned with the runtime runner boundary now present on `origin/main`. Pass the selected CLI runner into `lading bump`, keep publish preflight runner wrappers compatible with the shared protocol, and update tests to use the current helper modules. Adjust BDD cmd-mox preflight registration so metadata, lockfile, package, and publish command stubs can coexist after the rebase. Update developer documentation to describe `lading.runtime.CommandRunner` instead of the removed command-local runner module.
lodyai
Bot
force-pushed
the
issue-61-refresh-stale-nested-cargo-lock-files-before-publish-preflight
branch
from
June 8, 2026 17:56
cfaa044 to
527c7a8
Compare
There was a problem hiding this comment.
(1 files improve in Code Health)
Our agent can fix these. Install it.
Gates Passed
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| test_preflight_checks.py | 9.39 → 10.00 | Code Duplication |
Absence of Expected Change Pattern
- lading/lading/commands/publish.py is usually changed with: lading/tests/bdd/steps/test_publish_steps.py
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
This comment was marked as resolved.
This comment was marked as resolved.
1 similar comment
Owner
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lading/commands/lockfile.py`:
- Around line 55-68: The current handler returns an empty tuple for any non-zero
git exit_code, which silently skips lockfile discovery; change it so only the
explicit "not a git repository" case returns an empty tuple, and for all other
non-zero exit_code paths raise an exception (e.g., RuntimeError) containing the
git output details so failures are surfaced. Locate the block that examines
exit_code and builds detail from (stderr or stdout). Keep the LOGGER.warning for
the "not a git repository" branch (return ()), but replace the other
LOGGER.warning branch so it raises a RuntimeError (or a domain-specific
exception if one exists in the module) with a message including detail and
workspace context instead of returning ().
In `@lading/commands/publish_preflight.py`:
- Around line 220-246: The current loop treats any non-success from
validate_lockfile_freshness as a stale lockfile; change
validate_lockfile_freshness (or its caller) to distinguish a genuine "locked
needs update" Cargo response from other cargo failures: have
validate_lockfile_freshness return a tri-state or (bool, optional_error) so the
caller can detect the specific Cargo stderr/message that indicates the lockfile
must be regenerated (e.g. the stderr text stating the lockfile needs updating)
and only append to stale_lockfiles in that case; if validate_lockfile_freshness
returns an actual cargo error (other stderr/exit), surface that error
immediately via LOGGER.error with cargo details and raise PublishPreflightError
rather than suggesting repair commands; update usages of
validate_lockfile_freshness, stale_lockfiles, runner_with_env, LOGGER and
PublishPreflightError accordingly.
In `@tests/unit/publish/test_command_helpers.py`:
- Around line 85-109: The test duplicates implementation logic; update
test_normalise_cmd_mox_command_forwards_non_cargo_commands to accept explicit
expected_program and expected_args instead of deriving them: change the
`@pytest.mark.parametrize` signature to ("command", "expected_program",
"expected_args") and supply pytest.param entries for each case (e.g.
("cargo","check") -> expected_program "cargo::check", expected_args [],
("cargo","test","--workspace") -> "cargo::test", ["--workspace"], and
("git","status","--porcelain") -> "git", ["status","--porcelain"]); then remove
the conditional block that reconstructs expectations and assert
rewritten_program == expected_program and rewritten_args == expected_args,
leaving normalise_cmd_mox_command as the function under test.
In `@tests/unit/publish/test_preflight_cargo_runner.py`:
- Around line 53-85: Update the docstring of _run_and_record_cargo_preflight to
explain the recording-runner pattern: describe that the helper injects a closure
named recording_runner into publish._run_cargo_preflight to capture subprocess
arguments instead of executing them, so tests can assert the constructed cargo
command (returned as a tuple) without spawning real processes; include a short
note that recorded is a list of commands and recording_runner appends the
intercepted command and returns a dummy success tuple.
- Around line 87-143: Add a parametrized case to
test_run_cargo_preflight_command_arguments that verifies composition of
test_excludes and unit_tests_only: create a _RunCargoPreflightCase using
publish._CargoPreflightOptions with extra_args=("--workspace","--all-targets"),
test_excludes=["slow-integration"], and unit_tests_only=True, and set
expected_tail to ("--exclude","slow-integration","--lib","--bins"); give the
case id "test_excludes_with_unit_tests_only" and append it to the existing param
list so the test asserts both options combine correctly.
In `@tests/unit/publish/test_preflight_lockfile_validation.py`:
- Around line 75-77: Update the two pytest.raises blocks that assert
PublishPreflightError in test_preflight_lockfile_validation to include explicit
message constraints via the match= parameter so the test only accepts the
intended failure text; locate the asserts that call
publish_preflight._validate_lockfile_freshness (the block using `with
pytest.raises(publish.PublishPreflightError) as excinfo:` around the
tmp_path/runner call) and the second similar block around lines 119-123, and
replace them to use `with pytest.raises(publish.PublishPreflightError,
match="...")` with a regex or exact substring matching the expected preflight
error message for lockfile freshness.
In `@tests/unit/test_cargo_metadata_loading.py`:
- Around line 68-86: Rename the boolean-capturing list recorded_echo_stdout in
test_load_cargo_metadata_suppresses_stdout_echo to a clearer name (e.g.,
echo_flags) and update all references inside the test and its nested runner
(where recorded_echo_stdout.append(echo_stdout) and the final assertion
recorded_echo_stdout == [False]) to the new name so the variable clearly
represents a list of echo_stdout booleans; the test function name, nested runner
signature, and call to load_cargo_metadata should remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f31ad427-c5ac-4f43-aeca-9f9bc893036a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
docs/developers-guide.mddocs/roadmap.mddocs/users-guide.mdlading/__init__.pylading/cli.pylading/commands/bump.pylading/commands/lockfile.pylading/commands/publish.pylading/commands/publish_errors.pylading/commands/publish_manifest.pylading/commands/publish_plan.pylading/commands/publish_preflight.pylading/config.pylading/exceptions.pylading/workspace/metadata.pylading/workspace/models.pypyproject.tomltests/bdd/features/cli.featuretests/bdd/steps/metadata_fixtures.pytests/bdd/steps/test_bump_steps.pytests/bdd/steps/test_publish_given_steps.pytests/bdd/steps/test_publish_infrastructure.pytests/e2e/steps/test_e2e_steps.pytests/unit/__snapshots__/test_bump_command_internals.ambrtests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambrtests/unit/publish/test_command_helpers.pytests/unit/publish/test_preflight_cargo_runner.pytests/unit/publish/test_preflight_checks.pytests/unit/publish/test_preflight_lockfile_validation.pytests/unit/publish/test_run_integration.pytests/unit/test_bump_command_integration.pytests/unit/test_bump_command_internals.pytests/unit/test_cargo_metadata_loading.pytests/unit/test_cli.pytests/unit/test_lockfile.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lading/commands/lockfile.py`:
- Around line 55-68: The current handler returns an empty tuple for any non-zero
git exit_code, which silently skips lockfile discovery; change it so only the
explicit "not a git repository" case returns an empty tuple, and for all other
non-zero exit_code paths raise an exception (e.g., RuntimeError) containing the
git output details so failures are surfaced. Locate the block that examines
exit_code and builds detail from (stderr or stdout). Keep the LOGGER.warning for
the "not a git repository" branch (return ()), but replace the other
LOGGER.warning branch so it raises a RuntimeError (or a domain-specific
exception if one exists in the module) with a message including detail and
workspace context instead of returning ().
In `@lading/commands/publish_preflight.py`:
- Around line 220-246: The current loop treats any non-success from
validate_lockfile_freshness as a stale lockfile; change
validate_lockfile_freshness (or its caller) to distinguish a genuine "locked
needs update" Cargo response from other cargo failures: have
validate_lockfile_freshness return a tri-state or (bool, optional_error) so the
caller can detect the specific Cargo stderr/message that indicates the lockfile
must be regenerated (e.g. the stderr text stating the lockfile needs updating)
and only append to stale_lockfiles in that case; if validate_lockfile_freshness
returns an actual cargo error (other stderr/exit), surface that error
immediately via LOGGER.error with cargo details and raise PublishPreflightError
rather than suggesting repair commands; update usages of
validate_lockfile_freshness, stale_lockfiles, runner_with_env, LOGGER and
PublishPreflightError accordingly.
In `@tests/unit/publish/test_command_helpers.py`:
- Around line 85-109: The test duplicates implementation logic; update
test_normalise_cmd_mox_command_forwards_non_cargo_commands to accept explicit
expected_program and expected_args instead of deriving them: change the
`@pytest.mark.parametrize` signature to ("command", "expected_program",
"expected_args") and supply pytest.param entries for each case (e.g.
("cargo","check") -> expected_program "cargo::check", expected_args [],
("cargo","test","--workspace") -> "cargo::test", ["--workspace"], and
("git","status","--porcelain") -> "git", ["status","--porcelain"]); then remove
the conditional block that reconstructs expectations and assert
rewritten_program == expected_program and rewritten_args == expected_args,
leaving normalise_cmd_mox_command as the function under test.
In `@tests/unit/publish/test_preflight_cargo_runner.py`:
- Around line 53-85: Update the docstring of _run_and_record_cargo_preflight to
explain the recording-runner pattern: describe that the helper injects a closure
named recording_runner into publish._run_cargo_preflight to capture subprocess
arguments instead of executing them, so tests can assert the constructed cargo
command (returned as a tuple) without spawning real processes; include a short
note that recorded is a list of commands and recording_runner appends the
intercepted command and returns a dummy success tuple.
- Around line 87-143: Add a parametrized case to
test_run_cargo_preflight_command_arguments that verifies composition of
test_excludes and unit_tests_only: create a _RunCargoPreflightCase using
publish._CargoPreflightOptions with extra_args=("--workspace","--all-targets"),
test_excludes=["slow-integration"], and unit_tests_only=True, and set
expected_tail to ("--exclude","slow-integration","--lib","--bins"); give the
case id "test_excludes_with_unit_tests_only" and append it to the existing param
list so the test asserts both options combine correctly.
In `@tests/unit/publish/test_preflight_lockfile_validation.py`:
- Around line 75-77: Update the two pytest.raises blocks that assert
PublishPreflightError in test_preflight_lockfile_validation to include explicit
message constraints via the match= parameter so the test only accepts the
intended failure text; locate the asserts that call
publish_preflight._validate_lockfile_freshness (the block using `with
pytest.raises(publish.PublishPreflightError) as excinfo:` around the
tmp_path/runner call) and the second similar block around lines 119-123, and
replace them to use `with pytest.raises(publish.PublishPreflightError,
match="...")` with a regex or exact substring matching the expected preflight
error message for lockfile freshness.
In `@tests/unit/test_cargo_metadata_loading.py`:
- Around line 68-86: Rename the boolean-capturing list recorded_echo_stdout in
test_load_cargo_metadata_suppresses_stdout_echo to a clearer name (e.g.,
echo_flags) and update all references inside the test and its nested runner
(where recorded_echo_stdout.append(echo_stdout) and the final assertion
recorded_echo_stdout == [False]) to the new name so the variable clearly
represents a list of echo_stdout booleans; the test function name, nested runner
signature, and call to load_cargo_metadata should remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f31ad427-c5ac-4f43-aeca-9f9bc893036a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
docs/developers-guide.mddocs/roadmap.mddocs/users-guide.mdlading/__init__.pylading/cli.pylading/commands/bump.pylading/commands/lockfile.pylading/commands/publish.pylading/commands/publish_errors.pylading/commands/publish_manifest.pylading/commands/publish_plan.pylading/commands/publish_preflight.pylading/config.pylading/exceptions.pylading/workspace/metadata.pylading/workspace/models.pypyproject.tomltests/bdd/features/cli.featuretests/bdd/steps/metadata_fixtures.pytests/bdd/steps/test_bump_steps.pytests/bdd/steps/test_publish_given_steps.pytests/bdd/steps/test_publish_infrastructure.pytests/e2e/steps/test_e2e_steps.pytests/unit/__snapshots__/test_bump_command_internals.ambrtests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambrtests/unit/publish/test_command_helpers.pytests/unit/publish/test_preflight_cargo_runner.pytests/unit/publish/test_preflight_checks.pytests/unit/publish/test_preflight_lockfile_validation.pytests/unit/publish/test_run_integration.pytests/unit/test_bump_command_integration.pytests/unit/test_bump_command_internals.pytests/unit/test_cargo_metadata_loading.pytests/unit/test_cli.pytests/unit/test_lockfile.py
🛑 Comments failed to post (7)
lading/commands/lockfile.py (1)
55-68:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on unexpected
git ls-filesfailures.At Line 55, stop returning an empty tuple for all non-zero exits. Preserve the empty-tuple path only for the explicit “not a git repository” case, and raise a domain error for all other failures so lockfile refresh/validation cannot be silently skipped.
Patch sketch
+class LockfileDiscoveryError(LadingError): + """Raised when tracked Cargo.lock files cannot be discovered.""" + def _handle_git_ls_files_failure( @@ ) -> tuple[Path, ...] | None: @@ if "not a git repository" in detail.lower(): LOGGER.warning( "Skipping Cargo.lock discovery because %s is not a git repository", workspace_root, ) - else: - LOGGER.warning( - "Skipping Cargo.lock discovery after git ls-files failed: %s", detail - ) - return () + return () + + message = "git ls-files failed during Cargo.lock discovery" + if detail: + message = f"{message}: {detail}" + LOGGER.error(message) + raise LockfileDiscoveryError(message)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lading/commands/lockfile.py` around lines 55 - 68, The current handler returns an empty tuple for any non-zero git exit_code, which silently skips lockfile discovery; change it so only the explicit "not a git repository" case returns an empty tuple, and for all other non-zero exit_code paths raise an exception (e.g., RuntimeError) containing the git output details so failures are surfaced. Locate the block that examines exit_code and builds detail from (stderr or stdout). Keep the LOGGER.warning for the "not a git repository" branch (return ()), but replace the other LOGGER.warning branch so it raises a RuntimeError (or a domain-specific exception if one exists in the module) with a message including detail and workspace context instead of returning ().lading/commands/publish_preflight.py (1)
220-246:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDifferentiate stale lockfiles from generic
cargo metadatafailures.Line 222 treats every non-zero
cargo metadata --lockedresult as a stale lockfile, then Lines 229-242 emit lockfile-repair commands. Preserve non-lockfile failures as direct preflight errors with Cargo detail, and only emit stale-lock remediation when Cargo reports the--lockedlockfile-update condition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lading/commands/publish_preflight.py` around lines 220 - 246, The current loop treats any non-success from validate_lockfile_freshness as a stale lockfile; change validate_lockfile_freshness (or its caller) to distinguish a genuine "locked needs update" Cargo response from other cargo failures: have validate_lockfile_freshness return a tri-state or (bool, optional_error) so the caller can detect the specific Cargo stderr/message that indicates the lockfile must be regenerated (e.g. the stderr text stating the lockfile needs updating) and only append to stale_lockfiles in that case; if validate_lockfile_freshness returns an actual cargo error (other stderr/exit), surface that error immediately via LOGGER.error with cargo details and raise PublishPreflightError rather than suggesting repair commands; update usages of validate_lockfile_freshness, stale_lockfiles, runner_with_env, LOGGER and PublishPreflightError accordingly.tests/unit/publish/test_command_helpers.py (1)
85-109: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Simplify the parametrized test by removing redundant conditional logic.
Lines 101-108 reconstruct the expected output by re-implementing the same transformation logic tested in
normalise_cmd_mox_command. This duplicates the implementation rather than verifying it against known outputs.Replace the conditional block with explicit expected values in each parametrize case:
`@pytest.mark.parametrize`( - "command", + ("command", "expected_program", "expected_args"), [ - ("cargo", "check"), - ("cargo", "test", "--workspace"), - ("git", "status", "--porcelain"), + pytest.param( + ("cargo", "check"), + "cargo::check", + [], + id="cargo_check", + ), + pytest.param( + ("cargo", "test", "--workspace"), + "cargo::test", + ["--workspace"], + id="cargo_test_with_args", + ), + pytest.param( + ("git", "status", "--porcelain"), + "git", + ["status", "--porcelain"], + id="git_command", + ), ], ) def test_normalise_cmd_mox_command_forwards_non_cargo_commands( - command: tuple[str, ...], + command: tuple[str, ...], + expected_program: str, + expected_args: list[str], ) -> None: """cmd-mox normalisation preserves non-cargo commands and arguments.""" program, args = command[0], tuple(command[1:]) rewritten_program, rewritten_args = normalise_cmd_mox_command(program, args) - if program == "cargo" and args: - expected_program = f"cargo::{args[0]}" - expected_args = list(args[1:]) - else: - expected_program = program - expected_args = list(args) - assert rewritten_program == expected_program assert rewritten_args == expected_argsThis separates test data from test logic, making failures easier to diagnose and extending the test matrix simpler.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/publish/test_command_helpers.py` around lines 85 - 109, The test duplicates implementation logic; update test_normalise_cmd_mox_command_forwards_non_cargo_commands to accept explicit expected_program and expected_args instead of deriving them: change the `@pytest.mark.parametrize` signature to ("command", "expected_program", "expected_args") and supply pytest.param entries for each case (e.g. ("cargo","check") -> expected_program "cargo::check", expected_args [], ("cargo","test","--workspace") -> "cargo::test", ["--workspace"], and ("git","status","--porcelain") -> "git", ["status","--porcelain"]); then remove the conditional block that reconstructs expectations and assert rewritten_program == expected_program and rewritten_args == expected_args, leaving normalise_cmd_mox_command as the function under test.tests/unit/publish/test_preflight_cargo_runner.py (2)
53-85: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Document the recording runner pattern in the helper docstring.
The
_run_and_record_cargo_preflighthelper uses a closure to record commands, but the docstring only describes the return value. Expand the docstring to explain the recording pattern so future test authors understand why this abstraction exists.📝 Proposed docstring enhancement
def _run_and_record_cargo_preflight( workspace_root: Path, subcommand: typ.Literal["check", "test"], options: publish._CargoPreflightOptions, ) -> tuple[str, ...]: - """Run cargo preflight with a recording runner and return the command. + """Run cargo preflight with a recording runner and return the command. + + This helper isolates command construction from execution by injecting a + recording runner that captures the exact arguments passed to the subprocess + layer. It allows tests to verify option-to-argument transformations without + spawning real processes. Returns ------- The recorded cargo command as a tuple of strings. """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/publish/test_preflight_cargo_runner.py` around lines 53 - 85, Update the docstring of _run_and_record_cargo_preflight to explain the recording-runner pattern: describe that the helper injects a closure named recording_runner into publish._run_cargo_preflight to capture subprocess arguments instead of executing them, so tests can assert the constructed cargo command (returned as a tuple) without spawning real processes; include a short note that recorded is a list of commands and recording_runner appends the intercepted command and returns a dummy success tuple.
87-143: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add parametrize case for combined test_excludes and unit_tests_only.
The test matrix covers
test_excludesandunit_tests_onlyindependently, but not their combination. Add a case verifying that both options compose correctly:➕ Proposed additional test case
pytest.param( _RunCargoPreflightCase( options=publish._CargoPreflightOptions( extra_args=("--workspace", "--all-targets"), unit_tests_only=False, ), expected_tail=(), ), id="unit_tests_only_false", ), + pytest.param( + _RunCargoPreflightCase( + options=publish._CargoPreflightOptions( + extra_args=("--workspace", "--all-targets"), + test_excludes=["slow-integration"], + unit_tests_only=True, + ), + expected_tail=("--exclude", "slow-integration", "--lib", "--bins"), + ), + id="test_excludes_with_unit_tests_only", + ), ], )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/publish/test_preflight_cargo_runner.py` around lines 87 - 143, Add a parametrized case to test_run_cargo_preflight_command_arguments that verifies composition of test_excludes and unit_tests_only: create a _RunCargoPreflightCase using publish._CargoPreflightOptions with extra_args=("--workspace","--all-targets"), test_excludes=["slow-integration"], and unit_tests_only=True, and set expected_tail to ("--exclude","slow-integration","--lib","--bins"); give the case id "test_excludes_with_unit_tests_only" and append it to the existing param list so the test asserts both options combine correctly.tests/unit/publish/test_preflight_lockfile_validation.py (1)
75-77:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd message constraints to exception assertions.
Assert the failure contract directly with
match=...in bothpytest.raisesblocks, so tests fail only for the intended preflight error text instead of anyPublishPreflightError.Patch
- with pytest.raises(publish.PublishPreflightError) as excinfo: + with pytest.raises( + publish.PublishPreflightError, + match=r"Tracked Cargo\.lock files are stale", + ) as excinfo: @@ - with pytest.raises(publish.PublishPreflightError) as excinfo: + with pytest.raises( + publish.PublishPreflightError, + match=r"cargo generate-lockfile --manifest-path", + ) as excinfo:As per coding guidelines, “In tests, use specific exception types and message constraints with
pytest.raises(SpecificException, match=pattern)instead of generic exceptions (B017)”.Also applies to: 119-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/publish/test_preflight_lockfile_validation.py` around lines 75 - 77, Update the two pytest.raises blocks that assert PublishPreflightError in test_preflight_lockfile_validation to include explicit message constraints via the match= parameter so the test only accepts the intended failure text; locate the asserts that call publish_preflight._validate_lockfile_freshness (the block using `with pytest.raises(publish.PublishPreflightError) as excinfo:` around the tmp_path/runner call) and the second similar block around lines 119-123, and replace them to use `with pytest.raises(publish.PublishPreflightError, match="...")` with a regex or exact substring matching the expected preflight error message for lockfile freshness.Source: Coding guidelines
tests/unit/test_cargo_metadata_loading.py (1)
68-86: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Use a more descriptive variable name for the echo_stdout capture.
The variable
recorded_echo_stdoutcollects boolean values, but the name suggests it might contain stdout content. Rename it to clarify intent:♻️ Proposed refactor
def test_load_cargo_metadata_suppresses_stdout_echo(tmp_path: Path) -> None: """Cargo metadata captures JSON without echoing it to the terminal.""" - recorded_echo_stdout: list[bool] = [] + echo_flags: list[bool] = [] def runner( command: tuple[str, ...], *, cwd: Path | None = None, env: cabc.Mapping[str, str] | None = None, echo_stdout: bool = True, ) -> tuple[int, str, str]: del command, cwd, env - recorded_echo_stdout.append(echo_stdout) + echo_flags.append(echo_stdout) return 0, json.dumps(_METADATA_PAYLOAD), "" result = load_cargo_metadata(tmp_path, runner=runner) assert result == _METADATA_PAYLOAD - assert recorded_echo_stdout == [False] + assert echo_flags == [False]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_cargo_metadata_loading.py` around lines 68 - 86, Rename the boolean-capturing list recorded_echo_stdout in test_load_cargo_metadata_suppresses_stdout_echo to a clearer name (e.g., echo_flags) and update all references inside the test and its nested runner (where recorded_echo_stdout.append(echo_stdout) and the final assertion recorded_echo_stdout == [False]) to the new name so the variable clearly represents a list of echo_stdout booleans; the test function name, nested runner signature, and call to load_cargo_metadata should remain unchanged.
leynos
deleted the
issue-61-refresh-stale-nested-cargo-lock-files-before-publish-preflight
branch
June 8, 2026 19:47
4 tasks
This was referenced Jun 10, 2026
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch refreshes tracked
Cargo.lockfiles afterlading bumprewrites manifest versions, then validates tracked lockfiles beforelading publishenters the full cargo preflight. It addresses stale nested lockfiles such as therstest-bddtests/ui_lints/Cargo.lockcase described in issue #61.Closes #61.
Review walkthrough
--lockedvalidation.Validation
make check-fmt: passedmake lint: passedmake test: passed, 490 testsmake typecheck: passedcoderabbit review --agent: attempted twice; both runs reachedtools_completedand then hung without returning findings, so each was terminated after an extended waitNotes
The branch implements source-checkout lockfile refresh and source preflight validation. It does not restructure publish staging so that all publish preflight runs against the staged tree; that remains the larger follow-up investigation called out in issue #61.