Regenerate discovered nested lockfiles during lading bump - #160
Open
leynos wants to merge 15 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesNested lockfile regeneration
Pinned typechecking toolchain
Publish validation control flow
Sequence Diagram(s)sequenceDiagram
participant BumpCommand
participant CargoLockfileRepository
participant Git
participant Cargo
BumpCommand->>CargoLockfileRepository: resolve or regenerate lockfiles
CargoLockfileRepository->>Git: discover tracked Cargo.lock files
Git-->>CargoLockfileRepository: return tracked lockfile paths
CargoLockfileRepository-->>BumpCommand: return merged manifest paths
BumpCommand->>Cargo: run cargo update --workspace
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
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
July 8, 2026 09:48
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d8c9c57a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
lodyai
Bot
force-pushed
the
regenerate-lockfiles
branch
from
July 9, 2026 09:09
fbc0a6d to
2509734
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
regenerate-lockfiles
branch
from
July 13, 2026 19:29
2509734 to
1198e46
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Around line 177-184: Fix the API reference in the merge_discovered_manifests
documentation by removing the space between the module and member, so it
resolves as lading.commands.lockfile.discover_tracked_lockfiles.
In `@docs/execplans/regenerate-lockfiles.md`:
- Around line 48-51: Update the CommandRunner constraint in
regenerate-lockfiles.md to reference its defining module,
lading/runtime/runner.py, instead of lading/runtime.py; leave the lockfile
function signature constraints unchanged.
- Around line 271-272: Update the “Follow-up candidates” section in
regenerate-lockfiles.md to remove the completed “pin ty in CI and the Makefile”
item, keeping only genuinely pending candidates and preserving the completed
pinning record in the revision note.
- Around line 247-250: Update the “Delivered as planned” statement in the
lockfile regeneration plan to distinguish modes: live mode regenerates the root,
discovered, and configured lockfiles, while dry-run only discovers and reports
that same set without modifying files. Keep the existing discovery and publish
pre-flight behavior description unchanged.
In `@docs/users-guide.md`:
- Around line 279-284: Update the lockfile_manifests documentation to state that
configured manifests are required for nested lockfiles when the workspace is
outside a Git repository, because discovery returns no Git-tracked manifests
there and the configured list is the only inclusion mechanism. Replace the
wording that limits this option to untracked lockfiles, while preserving the
notes about automatic discovery and excluding the workspace-root manifest.
In `@lading/commands/publish_index_check.py`:
- Around line 375-393: The None-returning paths in _handle_index_missing_version
and _downgrade_or_raise currently rely on implicit returns. Add bare return
statements at the ends of both functions, using return rather than return None,
while preserving their existing control flow.
- Around line 290-292: Update the comments around the missing_index if/elif
chain to reference ty 0.0.56 instead of ty 0.0.8. Verify whether the pinned
checker still requires branch-based narrowing; if not, remove the workaround and
retain equivalent behavior.
In `@tests/unit/test_bump_lockfiles.py`:
- Around line 62-131: Update the assertions in all three
merge_discovered_manifests tests to include descriptive failure messages,
covering the merged result and runner.invocations checks in
test_merge_discovered_manifests_appends_tracked_manifests and the merged-result
checks in the other two tests. Follow the existing assert-with-message
convention used elsewhere in the test suite.
🪄 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: 0c8bcc34-50a7-4e4b-b434-a3b0d2fd11bb
📒 Files selected for processing (19)
.github/workflows/ci.ymlMakefiledocs/contents.mddocs/developers-guide.mddocs/execplans/regenerate-lockfiles.mddocs/repository-layout.mddocs/users-guide.mdlading/commands/bump.pylading/commands/bump_lockfiles.pylading/commands/bump_toml.pylading/commands/lockfile.pylading/commands/publish_index_check.pylading/commands/publish_preflight.pytests/bdd/features/cli.featuretests/bdd/steps/test_bump_steps.pytests/unit/conftest.pytests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambrtests/unit/test_bump_lockfile_rebuild.pytests/unit/test_bump_lockfiles.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/shared-actions(auto-detected)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
regenerate-lockfiles
branch
from
July 13, 2026 22:29
84f36e7 to
27ca243
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (4 warnings)
|
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.
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.
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope). ❌ Failed checks (4 warnings)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata ==='
gh pr view 160 --repo leynos/lading --json title,url,state,headRefName,updatedAt \
--jq '{title, url, state, headRefName, updatedAt}'
echo '=== PR checks ==='
gh pr checks 160 --repo leynos/lading || true
echo '=== Relevant Makefile validation targets ==='
rg -n -A12 -B3 '^(docs|format|lint|typecheck|test|check|TOOLS|TY_VERSION|TY)\b' Makefile || true
echo '=== Documentation: stale/current wording and ty status ==='
rg -n -i -C3 \
'currently passes only|currently used only|dry.run|dry run|configured manifests|merged manifest|uv tool install ty|pinning.*(follow.up|worthwhile)|TY_VERSION|ty==0\.0\.56|CargoLockfileRepository|LockfileRepository|discover_tracked_lockfiles' \
docs/developers-guide.md docs/execplans/regenerate-lockfiles.md || true
echo '=== Source and property-test structure ==='
ast-grep outline lading/commands/bump_lockfiles.py --items all --view expanded
ast-grep outline tests/unit/test_bump_lockfiles.py --items all --view expanded
rg -n -A70 -B10 \
'\bmerge_discovered_manifests\b|`@given`\b|hypothesis|non.git|not.a.git|fallback|dedup|deduplication|order' \
tests/unit/test_bump_lockfiles.py || true
echo '=== Public functions lacking docstrings (source only) ==='
python3 - <<'PY'
from pathlib import Path
import ast
missing = []
for filename in Path("lading").rglob("*.py"):
tree = ast.parse(filename.read_text(encoding="utf-8"), filename=str(filename))
for node in ast.walk(tree):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
if not node.name.startswith("_") and ast.get_docstring(node) is None:
missing.append(f"{filename}:{node.lineno}: {node.name}")
print("\n".join(sorted(missing)) or "None")
PYLength of output: 59462
Do not open a GitHub issue. Update the PR metadata in scope: The current |
lodyai
Bot
force-pushed
the
regenerate-lockfiles
branch
from
July 27, 2026 12:55
58bb2a7 to
37be73c
Compare
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.
`lading bump` regenerates only the root lockfile plus manifests listed in `bump.lockfile_manifests`, while `lading publish` pre-flight validates every git-tracked `Cargo.lock`, so unconfigured nested lockfiles go stale and publish fails with a manual repair message. Plan the fix: reuse `discover_tracked_lockfiles` in bump, merging discovered manifests with the configured list. Record the Stage A prototype evidence that `cargo update --workspace` refreshes path-dependency entries in nested fixture lockfiles.
`uv tool install ty` (used by CI and locally) now resolves to ty 0.0.8, which does not narrow bindings after calls to `NoReturn` helpers and flags tomlkit's `OutOfOrderTableProxy` indexing. Six pre-existing diagnostics broke `make typecheck` on a clean tree. Restructure the guard cascades in `publish_index_check.py` as if/elif/else chains so narrowing flows from the branch conditions, extract `_downgrade_or_raise` to keep the named-dependency path typed as `str`, and mirror the existing `type: ignore[index]` comment for proxy indexing in `bump_toml.py`. No behaviour change.
`lading bump` regenerated only the workspace root `Cargo.lock` plus manifests listed in `bump.lockfile_manifests`, while the publish pre-flight validates every git-tracked `Cargo.lock`. A tracked nested fixture lockfile therefore went stale after a bump and `lading publish` failed with a manual repair message. Add `bump_lockfiles.merge_discovered_manifests`, which unions the configured manifests with manifests implied by git-tracked lockfiles (via `discover_tracked_lockfiles`, the same helper publish uses), and feed the merged tuple to both the dry-run and live regeneration paths in `bump._process_lockfiles`. Configured entries keep their order; discovered entries follow sorted, de-duplicated by resolved path. In a non-git workspace discovery degrades to a warning and the configured list, matching the publish pre-flight. A Stage A prototype (recorded in the ExecPlan) confirmed that `cargo update --workspace --manifest-path <nested>` refreshes path-dependency entries in a nested standalone package's lockfile, so the existing cargo invocation is reused unchanged. The autouse `stub_lockfile_regeneration` fixture now also stubs the merge so manifest-focused unit suites never shell out to git.
Now that `lading bump` discovers and regenerates git-tracked lockfiles itself, the publish pre-flight's "This commonly happens after running `lading bump`" hint would mislead. Point it at the remaining causes instead: manifest edits made without bump, or bump runs with `--no-rebuild-lockfiles`. Regenerate the affected snapshot. Update the users' guide (bump discovery paragraph, quoted pre-flight message, `lockfile_manifests` and `rebuild_lockfiles` key descriptions, and the regeneration paragraph) and the developers' guide (`bump_lockfiles` module description and the lockfile helpers section) to describe the merged configured-plus-discovered manifest set.
Record the end-to-end acceptance run (real git and cargo against the prototype workspace: nested lockfile discovered, refreshed, and fresh under `--locked`), the versioned-path-dependency edge case surfaced by that run, both CodeRabbit reviews (zero findings), the retrospective, and the follow-up candidates. Mark the plan COMPLETE.
`make typecheck` previously ran whichever ty happened to be on PATH, and CI installed ty unpinned with `uv tool install`. ty is pre-1.0 and its diagnostics shift between releases, so the typecheck gate could break (or silently loosen) without any code change — locally, a stale ty 0.0.8 flagged six diagnostics that the current release does not. Adopt the latest release by pinning `TY_VERSION ?= 0.0.56` in the Makefile, mirroring the existing `RUFF_VERSION` pattern: the `typecheck` target now invokes `uv tool run --from ty==$(TY_VERSION)`, so no separate installation is needed and CI's tool-install step drops ty. Document the variable (and the bump-deliberately policy) in the developers' guide. ty 0.0.56 passes with no new diagnostics.
`docs/contents.md` is the documented entry point for the documentation set, but the new plan under `docs/execplans/` was not listed, so readers following the navigation path would not discover it. Add an "Execution plans" section to the index and record the `docs/execplans/` placement convention in the repository layout guide.
Clarify live and dry-run lockfile behaviour and document non-Git configuration requirements. Simplify ty narrowing now supported by the pinned checker, make successful None returns explicit, and add property coverage for merge ordering, resolved-path de-duplication, and non-Git fallback behaviour. Refresh the generated spelling dictionary entries carried in the working tree.
Adapt the rebased lockfile discovery behaviour to main's `LockfileRepository` boundary. Keep the bump domain independent of Git and Cargo execution while ensuring the concrete adapter discovers the same manifest set for dry-run projection and live regeneration. Record the post-plan architectural adjustment in the developer and design documentation.
Describe the current `LockfileRepository` boundary, shared lockfile discovery, and live versus dry-run behaviour. Label pre-pin ty evidence as historical and record the completed 0.0.56 pin unambiguously. Correct the remaining Oxford-spelling blocker surfaced by the documentation gate.
Add bare returns after the terminal raising helpers while preserving the existing branch structure and type narrowing.
Restore Ruff's required module-level spacing after the rebase combined the example and property tests.
Assert that the discovery BDD scenario invokes Cargo separately for the workspace and nested manifests. Expand the affected fixture docstrings to record their inputs, module gating, side effects, and return contract.
Separate manifest merging and path projection from lockfile regeneration so each test module exercises one cohesive production API. Keep private support code local and preserve the regeneration snapshot under its existing module.
lodyai
Bot
force-pushed
the
regenerate-lockfiles
branch
from
July 28, 2026 14:14
2696b26 to
c112996
Compare
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.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/developers-guide.md (1)
601-612: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCaption the shim inventory table.
Add a concise numbered caption immediately before or after the table.
Triage:
[type:docstyle]Proposed fix
+_Table 1: Removed compatibility shims and their canonical replacements._ + | Removed shim | Location | Canonical replacement |As per coding guidelines, “Caption every table and every diagram in documentation.” As per path instructions, doc-style comments require a Triage paragraph.
🤖 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 `@docs/developers-guide.md` around lines 601 - 612, Add a concise numbered caption immediately before or after the shim inventory table in the developers guide, identifying it as the table of removed shims and canonical replacements. Include the required doc-style Triage paragraph near the caption, without changing the table contents.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/users-guide.md`:
- Around line 87-98: Update the `lading bump` documentation to state that
configured `bump.lockfile_manifests` entries also include nested lockfiles in
non-Git workspaces, rather than limiting them to lockfiles Git does not track.
Retain the existing Git-tracked discovery behavior and add the required Triage
paragraph for this doc-style change.
In `@tests/unit/conftest.py`:
- Around line 80-95: Update the fixture docstring near the lockfile-stubbing
fixture to include an Examples section showing how the fixture is applied in a
test and that selected modules use deterministic helpers without invoking Git or
Cargo.
In `@tests/unit/test_bump_lockfile_path_resolution.py`:
- Around line 56-66: Change the _inside_dir helper’s multi-line docstring to a
concise single-line docstring. If the rationale about safe parent traversal is
still needed, preserve it as an inline comment within the helper without
changing generation behavior.
---
Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 601-612: Add a concise numbered caption immediately before or
after the shim inventory table in the developers guide, identifying it as the
table of removed shims and canonical replacements. Include the required
doc-style Triage paragraph near the caption, without changing the table
contents.
🪄 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: 205c31ec-7fe6-4e25-b345-a3939996d152
📒 Files selected for processing (21)
.github/workflows/ci.ymlMakefiledocs/contents.mddocs/developers-guide.mddocs/execplans/regenerate-lockfiles.mddocs/lading-design.mddocs/repository-layout.mddocs/users-guide.mdlading/commands/bump_lockfiles.pylading/commands/bump_toml.pylading/commands/lockfile.pylading/commands/publish_index_check.pylading/commands/publish_preflight.pytests/bdd/features/cli.featuretests/bdd/steps/test_bump_steps.pytests/unit/conftest.pytests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambrtests/unit/test_bump_lockfile_manifest_merge.pytests/unit/test_bump_lockfile_path_resolution.pytests/unit/test_bump_lockfile_rebuild.pytests/unit/test_bump_lockfiles.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/cuprum(auto-detected)leynos/shared-actions(auto-detected)
Comment on lines
+80
to
+95
| """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. | ||
|
|
||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a usage example to the fixture docstring.
Add an Examples section showing fixture application and the resulting stubbed behaviour.
Triage: [type:docstyle]
As per path instructions, public function documentation must include clear examples demonstrating usage and outcome.
🤖 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/conftest.py` around lines 80 - 95, Update the fixture docstring
near the lockfile-stubbing fixture to include an Examples section showing how
the fixture is applied in a test and that selected modules use deterministic
helpers without invoking Git or Cargo.
Source: Path instructions
Comment on lines
+56
to
+66
| @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. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a single-line docstring for this private helper.
Move the generation rationale into an inline comment if it remains necessary.
Proposed fix
`@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.
- """
+ """Draw a relative directory path that stays within the workspace."""
+ # Permit redundant components while preventing traversal above the root.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @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. | |
| """ | |
| `@st.composite` | |
| def _inside_dir(draw: st.DrawFn) -> list[str]: | |
| """Draw a relative directory path that stays within the workspace.""" | |
| # Permit redundant components while preventing traversal above the root. |
🤖 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_bump_lockfile_path_resolution.py` around lines 56 - 66,
Change the _inside_dir helper’s multi-line docstring to a concise single-line
docstring. If the rationale about safe parent traversal is still needed,
preserve it as an inline comment within the helper without changing generation
behavior.
Sources: Coding guidelines, Path instructions
Collaborator
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/developers-guide.md (1)
601-612: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCaption the shim-inventory table.
Add a table caption immediately after the table, consistent with the captioned table and figure conventions elsewhere in this document.
Triage:
[type:docstyle]As per coding guidelines, every table and diagram requires a caption.
🤖 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 `@docs/developers-guide.md` around lines 601 - 612, Add a descriptive caption immediately after the shim-inventory table in the developers guide, following the existing caption syntax and conventions used by other tables and figures in the document. Keep the caption specific to the removed compatibility shims and do not alter the table contents.Sources: Coding guidelines, Path instructions
♻️ Duplicate comments (1)
docs/users-guide.md (1)
87-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCarry the non-Git fallback wording into the tutorial too.
The tutorial still reads "for lockfiles that git does not track", but the configuration reference two hundred lines down (Lines 285-290) now correctly adds "and for nested lockfiles when the workspace is outside a Git repository". Sync the tutorial with the reference so readers scanning only the tutorial do not skip configuring
bump.lockfile_manifestsin a non-Git workspace.📝 Proposed fix
-adjacent `Cargo.toml`, plus any additional manifests listed in -`bump.lockfile_manifests` for lockfiles that git does not track. The result -header counts each changed category, e.g. "N manifest(s)", "N documentation +adjacent `Cargo.toml`. It also refreshes manifests listed in +`bump.lockfile_manifests`, including nested lockfiles in non-Git workspaces, +where Git discovery returns no manifests. The result header counts each +changed category, e.g. "N manifest(s)", "N documentation🤖 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 `@docs/users-guide.md` around lines 87 - 98, Update the tutorial’s lockfile description in the `lading bump` documentation to include nested lockfiles when the workspace is outside a Git repository, matching the wording and behavior described in the `bump.lockfile_manifests` configuration reference. Preserve the existing tracked-lockfile and additional-manifest details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Around line 212-220: Update the bump_lockfiles description to identify it as
owning bump-side manifest merging and regeneration, rather than Cargo lockfile
discovery. Preserve the explicit attribution of discovery to
lading.commands.lockfile.discover_tracked_lockfiles and keep the existing
details about merge ordering and regeneration behavior unchanged.
In `@docs/execplans/regenerate-lockfiles.md`:
- Around line 62-64: Update the line-count reference for
lading/commands/bump_lockfiles.py in the execplan from 155 to 379, while
preserving the existing 400-line repository limit and surrounding guidance.
In `@lading/commands/lockfile.py`:
- Around line 16-21: Update the documentation describing lading bump’s
regenerate_lockfiles behavior to explicitly state that it also regenerates the
workspace-root lockfile from Cargo.toml, even when that file is not discovered
or configured; keep the existing distinction between bump and validation
behavior intact.
In `@tests/unit/test_bump_lockfile_manifest_merge.py`:
- Around line 186-193: Update the expected_discovered helper in the test to
format each generated Cargo.toml path with the same POSIX conversion used by
merge_discovered_manifests, replacing plain str() while preserving the existing
filtering and sorting behavior.
---
Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 601-612: Add a descriptive caption immediately after the
shim-inventory table in the developers guide, following the existing caption
syntax and conventions used by other tables and figures in the document. Keep
the caption specific to the removed compatibility shims and do not alter the
table contents.
---
Duplicate comments:
In `@docs/users-guide.md`:
- Around line 87-98: Update the tutorial’s lockfile description in the `lading
bump` documentation to include nested lockfiles when the workspace is outside a
Git repository, matching the wording and behavior described in the
`bump.lockfile_manifests` configuration reference. Preserve the existing
tracked-lockfile and additional-manifest details.
🪄 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: a78be84e-1910-456e-8d7b-041babf8adf2
📒 Files selected for processing (21)
.github/workflows/ci.ymlMakefiledocs/contents.mddocs/developers-guide.mddocs/execplans/regenerate-lockfiles.mddocs/lading-design.mddocs/repository-layout.mddocs/users-guide.mdlading/commands/bump_lockfiles.pylading/commands/bump_toml.pylading/commands/lockfile.pylading/commands/publish_index_check.pylading/commands/publish_preflight.pytests/bdd/features/cli.featuretests/bdd/steps/test_bump_steps.pytests/unit/conftest.pytests/unit/publish/__snapshots__/test_preflight_lockfile_validation.ambrtests/unit/test_bump_lockfile_manifest_merge.pytests/unit/test_bump_lockfile_path_resolution.pytests/unit/test_bump_lockfile_rebuild.pytests/unit/test_bump_lockfiles.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/cuprum(auto-detected)leynos/shared-actions(auto-detected)
Comment on lines
+62
to
+64
| - No single code file may exceed 400 lines (repository rule); adding to | ||
| `lading/commands/bump_lockfiles.py` (155 lines) and `lading/commands/bump.py` | ||
| must respect this. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the stale capacity figure.
Update the stated bump_lockfiles.py line count. The supplied implementation reaches Line 379, not 155, leaving materially less headroom under the 400-line limit.
Triage: [type:docstyle]
As per coding guidelines, execplans must be kept up to date with the progress of changes.
🤖 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 `@docs/execplans/regenerate-lockfiles.md` around lines 62 - 64, Update the
line-count reference for lading/commands/bump_lockfiles.py in the execplan from
155 to 379, while preserving the existing 400-line repository limit and
surrounding guidance.
Sources: Coding guidelines, Path instructions
Attribute discovery to its defining module, document unconditional root regeneration and non-Git configuration, and align the manifest-merge property with the POSIX path contract.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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
2 participants
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 makes
lading bumpregenerate every git-trackedCargo.lockin the repository, not just the workspace root plus manifests listed inbump.lockfile_manifests. Previously,lading publishvalidated all tracked lockfiles under--locked, so any nested fixture lockfile that bump had not been configured to refresh went stale and publish aborted with a manual repair message — leaving the user wondering why bump had not done the repair itself. The users' guide already promised discovery-based refresh; this branch makes the code keep that promise.Execplan: docs/execplans/regenerate-lockfiles.md (implemented in full; status COMPLETE, including the Stage A prototype evidence, red/green test transcripts, an end-to-end acceptance run, and the retrospective).
The branch also carries three supporting changes:
lading bump; it now names the remaining causes (manifest edits made without bump, or bump runs with--no-rebuild-lockfiles).make typecheckgate is restored and future-proofed: six pre-existing diagnostics under the drifting unpinned ty toolchain are fixed, and ty is now pinned (TY_VERSION ?= 0.0.56) in the Makefile with CI running the pinned version viamake typecheckrather than installing ty separately.make fmtruns clean.Review walkthrough
merge_discovered_manifests, which unions configuredbump.lockfile_manifestsentries (order preserved) with manifests implied by git-tracked lockfiles (sorted, de-duplicated by resolved path). Discovery reusesdiscover_tracked_lockfiles, the same helper the publish pre-flight uses; a non-git workspace degrades to the configured list with a warning._process_lockfilesnow feeds the merged tuple to both the dry-run and live regeneration paths, so both report the same set.NoReturncalls) and lading/commands/bump_toml.py; the pinning is in the Makefile and .github/workflows/ci.yml.lockfile_manifestskey) and docs/developers-guide.md (module descriptions,TY_VERSIONvariable).Validation
make test: 683 passed, 62 snapshots passed.make lint: Ruff clean; interrogate 100%; Pylint 10.00/10.make check-fmt: 132 files already formatted.make typecheck(ty 0.0.56, pinned): all checks passed.make markdownlintandmake nixie: clean.uv tool run mbake validate Makefile: valid syntax.coderabbit review --agent: run after each milestone (feature, docs, ty pinning); zero findings each time.- fixtures/minimal/Cargo.lock (lockfile)in its output, refreshed the nested lockfile to the new version, andcargo metadata --lockedsubsequently exited 0.Notes
alpha = { path = ..., version = "0.1.0" }) now fails at bump time with cargo's version-selection error, because bump does not rewrite non-member manifests. Such a repository already failed at publish time, so the error merely surfaces earlier and more actionably; rewriting those requirements is recorded in the execplan as an open follow-up.bump.lockfile_manifestsremains supported for lockfiles that git does not track;--no-rebuild-lockfilesremains the global escape hatch.References