Pin lockfile discovery and validation I/O decisions (#83) - #125
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
SummaryCompletes issue Changes
ValidationFormatting, linting, type checking, and tests pass, with 696 tests passing. WalkthroughClarify that publish pre-flight classifies every tracked ChangesLockfile validation clarification
Tooling maintenance
Sequence Diagram(s)sequenceDiagram
participant Publish as lading publish
participant Discovery as discover_tracked_lockfiles
participant Preflight as _validate_lockfile_freshness
participant Cargo as cargo metadata --locked
participant Error as PublishPreflightError
Publish->>Discovery: Enumerate git-tracked Cargo.lock files
Discovery-->>Preflight: Return ordered lockfile paths
loop Each tracked lockfile
Preflight->>Cargo: Check freshness for adjacent Cargo.toml
Cargo-->>Preflight: Fresh, stale, or unexpected error
end
Preflight->>Error: Aggregate stale paths and repair commands
Error-->>Publish: Exit with diagnostic
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideEnsure lockfile freshness validation always classifies all tracked lockfiles (rather than short-circuiting on the first stale one), document this I/O decision, and pin the behavior with a regression test. Flow diagram for lockfile freshness validation classification behaviorflowchart TD
A[Start lockfile freshness validation] --> B[discover_tracked_lockfiles]
B --> C{Next tracked lockfile?}
C -->|No| D{Any stale lockfiles recorded?}
C -->|Yes| E[cargo metadata --locked]
E --> F{Result for lockfile}
F -->|Stale| G[Record lockfile as stale]
F -->|Fresh| C
F -->|Unexpected failure| H[Raise PublishPreflightError immediately]
G --> C
D -->|Yes| I[Raise aggregated error listing all stale lockfiles and repair commands]
D -->|No| J[Proceed with cargo check/test pre-flight]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
8dbe8ec to
96e5ace
Compare
![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.
|
@coderabbitai looking at the codebase, is this concern still relevant or is it made redundant by the PR's findings? Assessment against linked issues
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
rg -n "discover_tracked_lockfiles" -A 30 | head -100Length of output: 229 🏁 Script executed: #!/bin/bash
fd -e py | xargs grep -l "discover_tracked_lockfiles" 2>/dev/null
echo "---"
rg -n "rglob" --type py
echo "---"
rg -n "def discover_tracked_lockfiles" -A 40 --type pyLength of output: 666 🏁 Script executed: #!/bin/bash
cat -n lading/commands/lockfile.pyLength of output: 9679
Treat the earlier assessment table as stale on this point. The rglob redundancy from |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
48655ba to
3aef8d2
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
3aef8d2 to
8cbe18d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/unit/publish/test_preflight_lockfile_validation.py`:
- Around line 223-226: The test in test_preflight_lockfile_validation uses bare
asserts, so update each assertion in the preflight lockfile validation test to
include a clear failure message. Add messages to the assertions comparing probed
paths and checking each lockfile path in the exception message, using the
existing test variables like probed, lockfiles, and excinfo to keep the failure
output actionable.
In `@tests/unit/test_lockfile.py`:
- Around line 133-142: Add failure messages to the stub runner’s bare assertions
in the test helper so mismatches are diagnosable. In runner, update the checks
on command and cwd to use assertion messages instead of plain asserts, keeping
the existing behavior while making failures clearer.
🪄 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: 4d68175f-6ca7-42b1-bca1-f59f48e601dd
📒 Files selected for processing (4)
docs/developers-guide.mdlading/commands/publish_preflight.pytests/unit/publish/test_preflight_lockfile_validation.pytests/unit/test_lockfile.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/shared-actions(auto-detected)
8cbe18d to
731e458
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. tests/unit/publish/test_preflight_lockfile_validation.py Comment on file match="failed to download registry index",
):
publish_preflight._validate_lockfile_freshness(tmp_path, repository=repository)❌ New issue: Code Duplication |
|
@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). #83 Update unit tests to reflect the simpler lockfile discovery/validation path. ❌ The new regression test covers the decision to classify every lockfile (validation behavior), but there are no changes to tests specifically reflecting the simpler discovery path or the removal of the rglob pass. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@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 (1 warning)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== lockfile.py ==="
cat -n lading/commands/lockfile.py
echo "=== test_lockfile.py ==="
cat -n tests/unit/test_lockfile.py
echo "=== rglob search ==="
rg -n "rglob" --type py
echo "=== issue 83 ==="
gh issue view 83 --repo leynos/lading --json title,body,state,comments 2>/dev/null || trueLength of output: 45267
This sits alongside the pre-existing So the earlier finding — "no changes to tests specifically reflecting the simpler discovery path" — no longer holds. Treat that check as satisfied; no further work is required for issue [resolve] |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@coderabbitai Have the following now been resolved? ❌ Failed checks (1 warning)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== production symbols ==="
ast-grep outline lading/commands/publish_preflight.py \
--items all \
--match '_build_stale_lockfile_message|_validate_lockfile_freshness' \
--view expanded
echo "=== production implementations ==="
rg -n -A75 -B8 \
'^def _build_stale_lockfile_message|^def _validate_lockfile_freshness' \
lading/commands/publish_preflight.py
echo "=== design-document references ==="
rg -n -i -C5 \
'_build_stale_lockfile_message|_validate_lockfile_freshness|PublishPreflightError|stale lockfile' \
docs/lading-design.mdLength of output: 7322
No further documentation change or follow-up issue is required. [resolve] |
The property test test_validate_lockfile_freshness_probes_every_lockfile_ until_error carried a nested if/else with two comprehensions and two assertion loops, tripping the cyclomatic-complexity review check. Extract the no-error branch into _assert_no_error_outcomes and the error-abort branch into _assert_error_aborts_classification, leaving the test to locate the first error and dispatch. Assertions, diagnostic messages, and the Hypothesis strategy/settings are unchanged, so fresh-only, stale-only, mixed, and first-error-abort sequences keep their coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
`_string_list` returned `tuple(sorted(set(value)))` where `value` narrows only to `list[object]` after the `isinstance(value, list)` guard; the runtime `all(isinstance(item, str) ...)` check does not narrow the element type, so `set(value)` is `set[object]` and `sorted` rejects it under a newer `ty`. Build a `list[str]` via an isinstance-narrowing comprehension so the sort key type is provable. The diagnostic slipped in because both the Makefile and CI installed `ty` unpinned (latest). Pin it: add `TY_VERSION` and invoke the checker via `uv tool run ty@$(TY_VERSION)` (matching the ruff/typos pattern, so local runs are pinned too), drop the now-unused PATH `ty` prerequisite, and pin `uv tool install ty==0.0.32` in CI with a sync comment. Makefile and CI now agree on the ty version and must be bumped together. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Sorry, something went wrong.
- CI: drop the duplicate `uv tool install ty==0.0.32`. `make typecheck` now runs `uv tool run ty@$(TY_VERSION)`, so the Makefile's TY_VERSION is the single source of truth and a separate CI pin could only drift. - typos_rollout._string_list: the isinstance guard already proves every element is a str, so cast the validated list directly instead of filtering it a second time; keeps the sort key type provable under ty without the redundant pass. TypeError behaviour is unchanged. - Pin the remaining tag-referenced actions to immutable commit SHAs (verified against upstream tags): astral-sh/setup-uv@37802adc (v7, = v7.6.0) and actions/upload-artifact@ea165f8d (v4), each retaining its version comment. - Correct the pkg-index-collision comment to reference the list strategy's max_size, not max_examples. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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: 2
🤖 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 @.github/workflows/ci.yml:
- Around line 25-35: Update the “Install CLI tools” comment in the workflow to
state that TY_VERSION in the Makefile is the sole ty version declaration and
should be updated there. Remove wording that implies a separate CI pin or
requires synchronizing two locations; leave the installation commands and ruff
guidance unchanged.
In `@scripts/typos_rollout.py`:
- Around line 57-60: Inline the single-use strings assignment in the return
expression of the surrounding function, passing typ.cast("list[str]", value)
directly to set and preserving the existing sorting and tuple conversion.
🪄 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: 3dcdef0d-c402-4413-8158-5c1154696fc4
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/typos_rollout.pytests/unit/publish/test_preflight_lockfile_validation.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)
|
@coderabbitai resume@coderabbitai Have the following now been resolved? .github/workflows/ci.yml (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In @.github/workflows/ci.yml at line 25, Pin the workflow actions in the CI Source: Linters/SAST tools |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline .github/workflows/ci.yml --items all --view expanded || true
echo "=== Relevant workflow action references ==="
rg -n -C 3 '^\s*uses:\s*(astral-sh/setup-uv|actions/upload-artifact)@' .github/workflows/ci.yml
echo "=== All external action references ==="
rg -n '^\s*uses:\s*[^[:space:]]+@' .github/workflows/ci.ymlLength of output: 1562
No remaining work is required for this specific SAST finding. The workflow also contains other version-tagged actions, but they are outside the explicitly reported replacements and must be tracked by a separate finding if the security policy requires pinning every third-party action. ✅ Action performedReviews resumed. |
- ci.yml: reword the "Install CLI tools" comment so it names the Makefile's TY_VERSION as the sole ty version declaration to update, dropping the reference to a (now non-existent) duplicate CI pin. Install commands and the ruff guidance are unchanged. - typos_rollout._string_list: inline the single-use `strings` cast directly into the return expression; behaviour and the type-narrowing cast are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
30-32: 📐 Maintainability & Code Quality | 🟡 MinorSynchronize the TY pin documentation.
Update the Makefile comment referenced here: it still describes a separate
uv tool install ty==...pin and instructs maintainers to update both sites, contradicting this workflow’s claim thatTY_VERSIONis the sole declaration. Remove the stale wording and documentuv tool run ty@$(TY_VERSION)as the only source.🤖 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 @.github/workflows/ci.yml around lines 30 - 32, Update the referenced TY pin comment in the Makefile to remove the stale separate-installation and dual-update instructions. Document that make typecheck invokes ty via uv tool run ty@$(TY_VERSION) and that TY_VERSION is the sole version declaration.
🤖 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.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 30-32: Update the referenced TY pin comment in the Makefile to
remove the stale separate-installation and dual-update instructions. Document
that make typecheck invokes ty via uv tool run ty@$(TY_VERSION) and that
TY_VERSION is the sole version declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d7427885-0801-4b95-8d3b-78a3b91f6e72
📒 Files selected for processing (3)
.github/workflows/ci.ymlscripts/typos_rollout.pytests/unit/publish/test_preflight_lockfile_validation.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)
|
@coderabbitai review |
✅ Action performedReview finished.
|
The comment still referenced the `uv tool install ty==` CI step (removed when ty moved to `uv tool run ty@$(TY_VERSION)`) and instructed bumping two sites. Restate it: `make typecheck` invokes ty via the pinned uv tool run, TY_VERSION is the sole declaration, and CI installs no separate ty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
2 participants
Summary
Closes #83
rglobpass flagged in the issue no longer exists:discover_tracked_lockfilesalready relies solely ongit ls-filesplus thetarget/filter andCargo.tomladjacency check (landed with the lockfile-refresh rework in PR Refresh stale Cargo lockfiles before publish preflight (#61) #75's follow-ups). This PR completes the issue's remaining item: the evaluation of short-circuiting_validate_lockfile_freshnesson the first stale lockfile.cargo metadata --lockedprobes are cheap compared with replaying the pre-flight once per lockfile. Unexpected failures still raise immediately._collect_stale_lockfilesanddocs/developers-guide.md.Testing
test_discover_tracked_lockfiles_ignores_untracked_on_disk) pins the simpler, git-driven discovery path: an untrackedCargo.locksitting on disk beside a valid manifest is never returned, proving discovery consultsgit ls-filesalone and does not glob the tree (the redundantrglobpass is gone).make check-fmt,make lint,make typecheck, andmake test(696 passed) all green after rebasing onto currentmain.coderabbit review --agent: 0 findings.🤖 Generated with Claude Code
Summary by Sourcery
Document and test the decision to always classify all tracked lockfiles during publish preflight validation so that errors report every stale lockfile at once.
Documentation:
Tests:
References