fix(vendor): count skipped packages, not advisory warnings - #166
Merged
Mikola Lysenko (mikolalysenko) merged 1 commit intoAug 13, 2026
Merged
Conversation
Mikola Lysenko (mikolalysenko)
enabled auto-merge (squash)
August 12, 2026 23:47
`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>
Mikola Lysenko (mikolalysenko)
force-pushed
the
fix/vendor-honest-event-counts
branch
from
August 13, 2026 18:44
52a950f to
51f668d
Compare
Wenxin Jiang (Wenxin-Jiang)
approved these changes
Aug 13, 2026
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>
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
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.
Problem
The
vendorcommand'sskippedcount is untrustworthy because per-package advisory warnings are recorded asSkippedevents that bumpsummary.skipped.record_warning(crates/socket-patch-cli/src/commands/vendor.rs:242) routed every backendVendorWarningthroughEnvelope::recordas aSkippedevent. But a vendor warning is an advisory ABOUT how a package was vendored — a successfulvendor_prebuilt_downloadedservice 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 genuineApplied/Skipped/Failedoutcome is recorded separately, so the advisory double-counted.Observed against real production data (2026-08-12):
action:"skipped"(vendor_prebuilt_downloaded), so a single service vendor reportedapplied:1 skipped:1.summary.skippedcounted 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 advisorySkippedevent directly ontoenv.eventsinstead of throughEnvelope::record. The advisory stays visible to JSON consumers (unchangedSkippedaction +errorCode/reason;Skippednever flips run status, so no status signal is lost) but no longer bumpssummary.skipped. That counter now reflects only packages that were genuinely skipped — those recorded viaEnvelope::record(already_vendored,package_not_installed,vendor_unsupported_ecosystem, benign refusals). A successful service vendor is now countedapplied:1 skipped:0.Test
crates/socket-patch-cli/src/commands/vendor.rsnew hermetic modulewarning_counting_tests:advisory_warning_does_not_bump_skipped_summary— anappliedpackage + avendor_prebuilt_downloadedadvisory yieldsapplied:1 skipped:0, with the advisory still present inevents[].multiple_advisories_do_not_accumulate_skips— two advisories on one package keepskipped:0(reproduces the "2 skipped" report).genuine_package_skip_still_counts— analready_vendoredskip still bumpssummary.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_eventwhich asserts a warning is still askippedevent),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):
wiring_in_sync+copy_matches_after_hasheshot path atcrates/socket-patch-core/src/vendor/cargo.rs:396; thedocker_e2e_vendor_*suites assert byte-stable re-vendor). This PR already makes the no-op REPORT honestly (Vendored 0; 1 skippedforalready_vendored, no advisory inflation). The residual re-fetch on a not-installed / fresh-clone re-vendor routes throughfind_packages_for_purlsatcrates/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
vendorreporting wheresummary.skippedand human “N skipped” lines counted advisory warnings (e.g. successfulvendor_prebuilt_downloadedservice downloads) as skipped packages, so one applied package could showapplied:1 skipped:1or “2 skipped” on a single-package run.record_warningstill emits aSkipped-shaped event with stableerrorCode/reasonfor JSON, but pushes it ontoeventsdirectly instead ofEnvelope::record, sosummary.skippedonly reflects genuine package skips (already_vendored,package_not_installed, etc.). Run status is unchanged because advisorySkippedevents never drove failure status.Adds
warning_counting_teststo 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.