Skip to content

fix(vendor): count skipped packages, not advisory warnings - #166

Merged
Mikola Lysenko (mikolalysenko) merged 1 commit into
mainfrom
fix/vendor-honest-event-counts
Aug 13, 2026
Merged

fix(vendor): count skipped packages, not advisory warnings#166
Mikola Lysenko (mikolalysenko) merged 1 commit into
mainfrom
fix/vendor-honest-event-counts

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Problem

The vendor command's skipped count is untrustworthy because per-package advisory warnings are recorded as Skipped events that bump summary.skipped.

record_warning (crates/socket-patch-cli/src/commands/vendor.rs:242) routed every backend VendorWarning through Envelope::record as a Skipped event. But a vendor warning is an advisory ABOUT how a package was vendored — a successful vendor_prebuilt_downloaded service fetch (crates/socket-patch-core/src/vendor/cargo.rs:164, gem.rs:707, composer_lock.rs:566, ...), an artifact rebuild, a content-mismatch overwrite — not a package that was skipped. The package's genuine Applied/Skipped/Failed outcome is recorded separately, so the advisory double-counted.

Observed against real production data (2026-08-12):

  1. A SUCCESSFUL service download surfaced as action:"skipped" (vendor_prebuilt_downloaded), so a single service vendor reported applied:1 skipped:1.
  2. summary.skipped counted warning EVENTS, not skipped PACKAGES — "2 skipped" on a 1-package project, "1 skipped" on a refresh that skipped nothing.

Fix

crates/socket-patch-cli/src/commands/vendor.rs:242 (record_warning): push the advisory Skipped event directly onto env.events instead of through Envelope::record. The advisory stays visible to JSON consumers (unchanged Skipped action + errorCode/reason; Skipped never flips run status, so no status signal is lost) but no longer bumps summary.skipped. That counter now reflects only packages that were genuinely skipped — those recorded via Envelope::record (already_vendored, package_not_installed, vendor_unsupported_ecosystem, benign refusals). A successful service vendor is now counted applied:1 skipped:0.

Test

crates/socket-patch-cli/src/commands/vendor.rs new hermetic module warning_counting_tests:

  • advisory_warning_does_not_bump_skipped_summary — an applied package + a vendor_prebuilt_downloaded advisory yields applied:1 skipped:0, with the advisory still present in events[].
  • multiple_advisories_do_not_accumulate_skips — two advisories on one package keep skipped:0 (reproduces the "2 skipped" report).
  • genuine_package_skip_still_counts — an already_vendored skip still bumps summary.skipped (the fix narrows the counter, it does not zero it out).

RED before / GREEN after (verified by reverting the one-line change: the first two assertions fail with skipped:1). Also re-ran the hermetic vendor suites unchanged: --lib vendor (19), in_process_vendor (27, incl. mismatched_baseline_vendors_with_warning_event which asserts a warning is still a skipped event), scan_vendor_e2e (18), e2e_vex_vendor (8), json_envelope (23), full --lib (350). Build clean.

Scope

Kills the count/label sweep findings: vendor-skip-count-inflation (P2), vendor-envelope-service-download-as-skip (P3), vendor-event-applied-plus-skipped-info (P3), service-success-recorded-as-skipped (P3), vendor-unverifiable-skip-noise (P2).

Deferred (not in this focused PR):

  • vendor-noop-redownload (P2) — the write side. The covered backends already short-circuit an idempotent re-vendor without rewriting when the installed copy is present (e.g. cargo's wiring_in_sync + copy_matches_after_hashes hot path at crates/socket-patch-core/src/vendor/cargo.rs:396; the docker_e2e_vendor_* suites assert byte-stable re-vendor). This PR already makes the no-op REPORT honestly (Vendored 0; 1 skipped for already_vendored, no advisory inflation). The residual re-fetch on a not-installed / fresh-clone re-vendor routes through find_packages_for_purls at crates/socket-patch-cli/src/commands/vendor.rs:699 — the qualified-purl "not installed" lookup owned by a separate PR — so the write-avoidance for that path is intentionally left to it and not touched here.

🤖 Generated with Claude Code


Note

Low Risk
CLI reporting and JSON envelope counting only; no change to vendoring behavior, lockfile wiring, or run exit semantics for successful vendors.

Overview
Fixes vendor reporting where summary.skipped and human “N skipped” lines counted advisory warnings (e.g. successful vendor_prebuilt_downloaded service downloads) as skipped packages, so one applied package could show applied:1 skipped:1 or “2 skipped” on a single-package run.

record_warning still emits a Skipped-shaped event with stable errorCode/reason for JSON, but pushes it onto events directly instead of Envelope::record, so summary.skipped only reflects genuine package skips (already_vendored, package_not_installed, etc.). Run status is unchanged because advisory Skipped events never drove failure status.

Adds warning_counting_tests to lock in applied + advisory → skipped:0, multiple advisories on one package, and real skips still incrementing the counter.

Reviewed by Cursor Bugbot for commit 52a950f. Configure here.

`record_warning` routed every backend advisory through `Envelope::record`
as a `Skipped` event, so each one bumped `summary.skipped`. A successful
`vendor_prebuilt_downloaded` service fetch then reported `applied:1
skipped:1`, a 1-package project could print "2 skipped", and a refresh
that skipped nothing still counted "1 skipped".

Push the advisory event directly onto `events` instead: it stays visible
to JSON consumers (same `Skipped` action + code/detail, which never flips
run status) but no longer inflates `summary.skipped`, which now counts
only packages that were genuinely skipped (recorded via `Envelope::record`
— `already_vendored`, `package_not_installed`, benign refusals, ...).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mikolalysenko
Mikola Lysenko (mikolalysenko) merged commit cd8b53b into main Aug 13, 2026
62 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the fix/vendor-honest-event-counts branch August 13, 2026 21:23
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Aug 19, 2026
…ck restore (#205)

* test(vendor): RED — drift-skipped revert must keep artifacts + ledger (residual #131)

Flip the deliberate #131 pinning test
classic_vendor_revert_after_hosted_overlay_is_drift_skipped_and_lossy into
the fixed contract (..._keeps_blob): when every wiring record is
drift-skipped ("left alone"), the blob dir must SURVIVE the revert and the
keep must be surfaced (vendor_artifact_kept) — plus a recovery leg proving
a second revert completes once the drift is undone.

Same contract asserted across the npm-family twins' existing drift tests
(npm_lock / pnpm_lock / yarn_berry_lock / bun_lock / yarn_classic_lock:
revert_leaves_drifted_* now require the artifact dir kept + the keep
warning), two new CLI orchestration tests in in_process_vendor.rs
(run_revert and reconcile must retain the state.json entry, record a
COUNTED Skipped vendor_revert_kept — summary.skipped was 0 even when the
skip event was emitted — and never a Removed), and a new yarn-classic e2e
capstone leg (vendored -> hosted overlay -> vendor --revert keeps the
tarball, the kept tarball still installs after the drift is undone, and
the follow-up revert completes to pristine).

RED: 6 core + 2 CLI + 1 e2e tests fail on current main behavior
(remove_tree runs unconditionally after the restore loop; success prunes
the ledger entry).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(vendor): keep artifacts and ledger when revert drift-skips the lock restore

Residual #131 (strapi campaign, adversarially re-confirmed on main): when
vendor --revert found every recorded lock entry re-resolved (a hosted
patch.socket.dev overlay, a registry re-resolve — anything failing the
uuid-dir ownership gate), revert_recorded_block correctly left the lock
alone with vendor_lock_entry_drifted warnings, but the backend still ran
remove_tree(.socket/vendor/npm/<uuid>/) unconditionally after the restore
loop, and run_revert/reconcile_dropped treated the warning-only success as
"removed": PatchAction::Removed + state.entries.remove + save_state. End
state: the lock still pointed at the drifted target while the artifacts,
the ledger entry holding the only pre-vendor originals, and the tarball
the redirect ledger's recorded `original` fragments reference were all
destroyed — a silent-success data loss with a delayed broken-install
hazard.

Fix, per the orphan sweep's invariant (never delete what something still
references):

- RevertOutcome grows `kept_artifact` plus drift_skipped()/keep_artifact()
  helpers (vendor/mod.rs). keep_artifact() pushes an honest
  `vendor_artifact_kept` warning naming the kept dir and the remediation.
- All five npm-family backends with the unconditional remove shape
  (yarn_classic_lock, npm_lock, pnpm_lock, yarn_berry_lock, bun_lock) now
  skip the remove_tree and mark the keep whenever any wiring record was
  drift-skipped.
- run_revert and reconcile_dropped (cli vendor.rs) honor the signal: the
  state.json entry is retained and the outcome is a COUNTED Skipped event
  (`vendor_revert_kept`) recorded via env.record — fixing the sub-bug
  where summary.skipped stayed 0 while skip events were emitted — never a
  Removed; counts stay uninflated (#166 precedent: advisory warnings keep
  bypassing the counters). Human output gains a "Kept N drifted
  package(s)" line.
- Other RevertOutcome construction sites gain `kept_artifact: false`
  (no behavior change outside the npm family).

Once the drift is undone (vendored lock restored — e.g. a future hosted
revert replaying its recorded originals), the next `vendor --revert`
completes exactly as before: lock restored byte-for-byte, artifacts and
ledger pruned.

GREEN: the 6 flipped core tests, the 2 new CLI orchestration tests, and
the new yarn-classic E1 e2e capstone (drift-skip keep -> kept tarball
still installs -> follow-up revert completes to pristine) all pass;
cargo test -p socket-patch-core clean; clippy --workspace --all-features
clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(vendor): drift-skip keep gate converges — already-reverted records are not drift

The keep gate (drift_skipped) keyed on vendor_lock_entry_drifted, but the
npm-family backends emitted that same code for records a PREVIOUS partial
revert had already restored or removed: a restored fragment's registry
value fails the uuid ownership gate ("re-resolved since vendoring"), a
removed berry resolutions entry hits "no longer exists; nothing to
remove", and a rekeyed pnpm/berry block restored under its pre-vendor key
misses the recorded-key lookup ("no longer exists; nothing to restore").
After any partial restore, every later `vendor --revert` therefore
re-classified the converged records as drift and kept the artifact dir +
ledger entry forever — the CLI's own "undo the drift and re-run `vendor
--revert` to finish" remediation could never be satisfied.

Teach every left-alone branch in the five keep-gate backends (npm, bun,
pnpm, yarn classic, yarn berry) to recognize the ALREADY-CONVERGED state
as a silent no-op before warning:

- a live fragment equal to the recorded pre-vendor `original` (restored
  in place: npm/bun entries, classic/berry blocks, pnpm overrides /
  importer deps / snapshot refs, berry takeover resolutions);
- for Added records with no original, the key being absent (berry
  resolutions entry/table, pnpm overrides in package.json /
  pnpm-workspace.yaml / the lock's overrides section);
- for the rekeying restores (berry lock entries, pnpm packages/snapshots
  blocks), the recorded original block being live verbatim under its own
  pre-vendor key when the recorded key no longer matches.

drift_skipped() now fires only on genuine third-party drift the user can
still undo, so the kept artifacts + ledger entry are released the moment
the drift is undone — run 2 completes the revert and prunes everything.

Tests: each backend's drift unit test gains a second-run leg (undo ONLY
the drift; already-reverted records must be silent and the revert must
complete), the CLI keep tests now drift to a value that is neither ours
nor the original, and a new CLI test pins that a lock manually restored
to the pre-vendor original converges (counted Removed, ledger pruned) on
the first revert.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants