Skip to content

Fix vendored dry-run counting in-sync patches (#1850) - #1856

Open
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
mainfrom
agent/fix-vendored-dry-run-already-vendored
Open

Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
mainfrom
agent/fix-vendored-dry-run-already-vendored

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1850

Summary

On a project that is already vendored, scan --mode vendored --dry-run and
get <uuid> --mode vendored --dry-run said "Would download and vendor N
patches", while the same preview's --json (skipped /
already_vendored) and the wet run ("already vendored; nothing to do")
both said there was nothing to do.

Now the human preview matches them:

[dry-run] Would download and vendor 0 of 1 patch (1 already vendored). No changes made.
  [skip] pkg:npm/left-pad@1.3.0 (already vendored)      <- stderr, the wet run's line

Mixed runs list only the non-zero clauses, e.g. 1 of 4 patches (2 already vendored, 1 would be refused).

Root cause

preview_vendor already classified each row (PreviewVerdict::AlreadyVendored)
and the JSON arm recorded it, but the human renderer ignored the verdict:
render::dry_run_line subtracted only would-refuse rows, print_refusals
printed nothing for in-sync rows, and get used its own
format_dry_run(…, selected.len()) that subtracted nothing at all.

Fix

  • render::Plan::Vendor { total, refused, in_sync }: the headline leaves
    out in-sync and refused rows.
  • VendorPreview::print_human() prints the headline, the [skip] … (already vendored) lines (via the wet run's format_record_skip, on
    stderr like the wet run), the [would-refuse] lines and the symlink
    advisories. scan and get both call it, so they can't drift. Side
    effect: get --mode vendored --dry-run now also leaves refused patches
    out of its count, as scan already did.
  • CLI_CONTRACT.md documents the headline.

No wrapper changes needed (npm/, pypi/, gem/ only dispatch to the binary).

Tests (red → green)

Issue Test Without fix With fix
#1850 (scan) tests/scan/scan_render.rs::scan_human_vendored_dry_run_skips_already_vendored FAILED (Would download and vendor 1 patch) ok
#1850 (get) tests/get/get_render.rs::human_vendored_uuid_dry_run_skips_already_vendored FAILED ok
#1850 (shared headline) scan::vendor_flow::preview_tests::preview_headline_leaves_out_already_vendored, scan::render::tests::dry_run_line_counts_already_vendored n/a (new API) ok

Updated tests/vendor/in_process_vendor_bun.rs::dry_run_human_names_would_refuse_records
to expect the shared headline (0 of 1 patch (1 would be refused)).

Commands run locally:

  • cargo fmt --all -- --check: clean
  • cargo clippy --workspace --all-features -- -D warnings: clean
  • cargo test -p socket-patch-cli --all-features --lib: 942 passed
  • cargo test -p socket-patch-cli --all-features --test scan --test get --test vendor: 248 / 174 / 151 passed
  • The full cargo test --workspace --all-features build ran out of this
    container's disk while linking; CI covers the rest.

Review notes (/code-review high)

  • Fixed: the stale Bun dry-run expectation; folded the counts into
    Plan::Vendor and the two print calls into print_human().
  • Not changed: the preview marks a row AlreadyVendored whenever the
    ledger holds the purl at the same uuid, even for a legacy non-detached
    entry or one whose artifact was deleted, which the wet run would
    re-download or rebuild. That classification predates this PR and the
    JSON preview already reports those rows as already_vendored. This PR
    only makes the human output agree with it. Making the classifier check
    artifact and wiring would be a separate change.

Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
`scan --mode vendored --dry-run` and `get --mode vendored --dry-run`
told users they "Would download and vendor N patches" even when every
patch was already vendored, while the same preview's JSON and the wet
run both said there was nothing to do.

The human headline now leaves out patches the vendor ledger already
holds at this uuid ("Would download and vendor 0 of 1 patch (1 already
vendored)"), and each one gets the wet run's
"[skip] <purl> (already vendored)" line. `get` now shares scan's
headline, so it also stops counting patches the preflight would refuse.

Fixes #1850

Assisted-by: Claude Code:claude-opus-5-5
`scan` and `get` now print the vendored dry-run preview through one
call, so the headline and its skip and refusal lines cannot drift
between the two commands again. The Bun dry-run test now expects the
shared headline, which leaves a refused patch out of the count.

Refs #1850

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 29, 2026 21:44
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 015ca58. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 29, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at 015ca581d4f2578fbf10c529f9cfddb9be9efe96.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does: Before this PR, scan --mode vendored --dry-run and get --mode vendored --dry-run counted patches that were already vendored as work, so an in-sync project was told it would "vendor N patches". Both commands now share one VendorPreview::print_human(). Its headline counts only real work (Would download and vendor K of N (A already vendored, R would be refused)), and it prints the wet run's [skip] <purl> (already vendored) line for each in-sync row.

Risk: low. Only the human dry-run output changes. JSON output, exit codes and the wet path are unchanged. A row gets exactly one verdict, and refusal is checked before already-vendored, so total - refused - in_sync can't double-count. The intended behaviour change is that get's headline now also leaves refused patches out, and the Bun test was updated to match.

Look here:

Verified: I read the full diff against #1850. The tests would fail without the fix. There is no debug code or scope creep, CHANGELOG.md is untouched, and there are no path or CRLF concerns because the change is formatting only. CI is green on 015ca58 (32 passed, 50 skipped, ci-ok and clippy both green). Bugbot found no new issues.

Changes I made: none.

Open questions (non-blocking, follow-up material): The human scan "Patches to vendor:" table above the headline still lists in-sync patches (scan/mod.rs around line 3366). The [skip] lines go to stderr while the headline goes to stdout, which matches the wet run.

Auto-merge (squash) is armed, so approving sends this straight to the merge queue.


Generated by Claude Code

This was referenced Oct 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

2 participants