Commit 0b4e645
Fix four vendored/hosted correctness bugs found while profiling (#268)
* fix(get): order the narrowed patch selection by purl, not by HashMap bucket
`filter_to_installed_releases` buckets every release-variant purl (PyPI
`?artifact_id=`, RubyGems `?platform=`, Maven `?classifier=`) into a
`HashMap` keyed by base purl and then drains it. That drain is the
function's OUTPUT order, which is the order the download loop walks — so
`download.patches` (and `apply.patches`, and the per-patch stderr lines)
came out in `HashMap` bucket order: two identical runs of the same
project emitted the same records in different orders.
Sort the multi-variant bases before resolving them (stable warnings) and
sort the kept selection by purl before returning it, matching how every
sibling collection in the same envelope is ordered (scan's `packages`,
the agent flow's `skip_records`). The `--all-releases` pass-through gets
the same order so both arms of the function share one contract.
Two tests: one on the narrowing itself and one on the emitted
`download.patches` array.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(vendor): report an unstageable patch per package, not as a dead run
The patch view serves `blobContent` only for the files a patch CHANGES. A
zero-delta file — `beforeHash == afterHash` — comes back with hashes and
no content, which the in-memory vendor stager counts as a failed fetch.
One such patch made the WHOLE run bail `no_local_source`: exit 1,
`status: error`, zero events, and every other package in the manifest
left unvendored without a word.
Live example: `pkg:npm/tar-fs@2.1.1`, patch
`8ff3e0c7-6855-4224-924b-3e1151744ed4` — seven zero-delta fixture files
plus one changed `package/index.js`. A three-package project (tar-fs,
braces, minimist) downloaded all three records and then vendored none.
A package whose patch content cannot be obtained is an unsatisfiable
package like any other (`vendor_fetch_failed`, `redirect_revert_failed`,
the Bun refusals …): it gets its own `failed` event and the run carries
on. `stage_vendor_sources_in_memory` now hands those purls back in
`MemStagedSources::unavailable()`; `vendor`, `scan --vendor` /
`get --mode vendored` and `repair` report them one by one and run the
engine over the rest. The pre-event `no_local_source` bail stays for the
case it was written for — NOTHING in the manifest can be staged, so
there are no events to report — which is the shape every existing test
pins. The stager's own stderr summary is unchanged, so `--silent` still
gets exactly one error line per arm.
Repro on the live API (same project, built binaries): before, exit 1 /
`status: error` / `no_local_source` / 0 events; after, exit 1 /
`partial_failure` with `failed pkg:npm/tar-fs@2.1.1 no_local_source` plus
`applied` braces and minimist, both in the ledger.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(vendor): keep an already-redirected requirements.txt line in the inventory
The hosted rewriter turns `name==X` into the PEP 508 direct reference
`name @ <patch-server url> --hash=sha256:…`. The requirements.txt
inventory only reads exact `==` pins, so every line a hosted run had
already rewritten vanished from it — and the second hosted run over a wet
requirements.txt reported `packagesWithPatches: 1` instead of 12, having
"lost" the eleven packages it had just wired. uv.lock keeps its
`[[package]]` name/version through the same rewrite, and Pipfile.lock's
reader already keeps a Socket-written reference as the package it
replaces (`socket_reference_coords`); requirements.txt now does too,
through that same reader and for both shapes it writes (the hosted url
and the vendored `.socket/vendor/pypi/…` path).
The recovered entry is discovery-only — `resolved: None`,
`integrity: None`, exactly what a `==` pin beside it yields — so the
PATCHED artifact the line points at can never be fetched as a pristine
source, and VEX ledger liveness keeps reading it as the "proves nothing"
entry it reads a hosted uv.lock/Cargo.lock entry as. A user's own
file/url reference is still not ours to resolve and stays out; a
reference whose coordinates contradict the requirement's own name is
skipped fail-closed.
Live repro on the phase-3 `req-big` fixture (391 pins, 11 redirected):
before, run 1 reported 12 packages with patches and run 2 reported 1 with
0 redirected; after, both runs report 12 / 11 with the same single
pre-existing warning and a byte-identical requirements.txt.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(vendor): refuse a lockfile-only gem in build mode before downloading it
A bundler path source will not load without the eval-able stub gemspec
rubygems writes into `<gem home>/specifications/` when the gem is
INSTALLED, and a downloaded `.gem` carries its gemspec only as YAML in
`metadata.gz` — which is precisely why the vendoring service converts it
and serves a separate `gem-stub-gemspec` artifact. So a local build can
never vendor a fetched gem. The auto-fetch rung downloaded the `.gem`
anyway and only then hit the backend's `gem_spec_missing`: a wasted
registry round trip on every `--vendor-source build` run, ending in a
message whose remedy ("use --vendor-source=service") did not name the
mode that actually works from here.
Refuse before the fetch, for gem purls only, only when the run cannot use
the patch service at all (`--vendor-source build`, or no service config),
and only for the purls a fetch would actually be attempted for — the ones
`fetch_pristine_package` resolves from the lockfile or recovers from the
ledger. A gem that resolves from nowhere has nothing to say about
gemspecs and keeps its calm `package_not_installed` skip. The refusal is
the same `gem_spec_missing` code and the same `failed` event, with a
detail that says why a fetched gem is unusable and points at
`bundle install` or `--vendor-source=auto`. `auto` and `service` still
fetch: the service path needs the staged pristine copy, and that is the
mode that CAN vendor this gem. The backend keeps its own refusal as the
backstop for every other route into it (including the security case where
a staging dir must never yield a stub).
Repro: a lockfile-only gem against a mock rubygems host — before, the
`.gem` was downloaded and the run then failed `gem_spec_missing`; after,
the host sees no request at all and the same failure arrives with the
real remedy. Two guard twins pin the scope: `auto` still downloads, and a
gem no lockfile resolves still reports `package_not_installed`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(vendor): scope the gem build-mode refusal to gems a fetch would download
The pre-fetch `gem_spec_missing` gate keyed off "the lockfile resolves this
purl, or the ledger knows it" — mere RESOLVABILITY. The download it exists
to save needs VERIFIABILITY, and the already-vendored case needs no local
build at all, so the gate fired in two cases where `main` never fetched
anything and never failed:
* A bundler < 2.6 `Gemfile.lock` (no `CHECKSUMS` section — the majority of
real locks) resolves the gem but records no verifier.
`registry_fetch::fetch_and_stage` refuses a `LockIntegrity::None` entry
before any network I/O, so CLI_CONTRACT's documented pair fires instead
(`vendor_fetch_unverifiable` warning + the calm `package_not_installed`
skip). The gate replaced that with `failed`/`gem_spec_missing` — for a
download that never existed, and with a remedy that cannot work: the same
fixture under `--vendor-source auto` still yields the skip pair, because
the purl never reaches the gem backend in any mode.
* An already-vendored gem on a fresh clone (committed `.socket/vendor/gem/`
copy + wired lock, `bundle install` not yet run) has a ledger entry, which
is exactly the case `fetch_pristine_package`'s ledger-recovery rung exists
for ("an already-vendored lock-only checkout re-scans green"). The
recovered fetch feeds the gem backend's idempotent hot path, which
re-confirms the wired lock and returns `already_vendored` without ever
needing a stub gemspec. Measured on `main`: exit 0, `status: success`,
events `[skipped/vendor_fetched_missing, skipped/already_vendored]`. With
the gate: exit 1, `partialFailure`, `failed`/`gem_spec_missing` — a green
idempotent re-run turned into a failure, with nothing wrong with the
project. `scan --vendor` / `get --mode vendored` under
`--vendor-source build` broke the same way.
Mirror `fetch_pristine_package`'s own `fetchable` filter instead: an
inventory entry whose integrity is not `LockIntegrity::None`, and no ledger
entry. GEM-4's real case — a not-installed gem a bundler >= 2.6 lock CAN
verify — still refuses before the download, unchanged.
Two regression tests, both green on `origin/main` and red on the gate as
written: the unverifiable lock keeps its documented skip pair with zero
registry requests, and a vendor-then-`rm -rf vendor/` re-run stays exit 0
with `already_vendored`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(vendor): keep an already-vendored requirements.txt line in the inventory
The hosted half of this fix landed in 11de805; the vendored half did not,
although both docstrings claimed it. The hosted rewriter emits a PEP 508
direct reference (`name @ <patch-server url>`), but the VENDORED
requirements writer emits something else entirely — a bare path line,
`./<rel wheel>[ ; marker] --hash=sha256:<hex> # socket-patch vendor:
<name>==<ver>` (`vendor::pypi_requirements::vendor_line`) — with no
`name @` at all. `direct_reference` splits on `@` and so never matched it,
and `inventory_requirements_txt` discarded the comment part where the
`socket-patch vendor:` tag lives, so a vendored requirements.txt kept the
exact symptom the hosted arm fixed: its packages drop out of
`lock_inventory`, which is what `scan/discovery.rs::lockfile_supplement`
counts, so a re-scan of an already-vendored lockfile-only checkout
under-reports them. The `.socket/vendor/pypi/` arm of
`socket_reference_coords` was only ever reachable from Pipfile.lock.
Read the vendored shape too: keep the logical line's comment, and when the
code part is a bare path `socket_reference_coords` recognizes, take the
requirement name from the `socket-patch vendor:` tag — the same
`utils::requirements::vendor_tag` reader `vex::discover::pypi_other`
already uses — and cross-check it against the path's own coordinates, the
same fail-closed rule the hosted arm applies to its url. The recovered
entry stays discovery-only (`resolved: None`, `integrity: None`), so the
PATCHED wheel it points at can never be fetched as a pristine source. A
user's own wheel path is still not ours to resolve and stays out.
Two tests, both red before: the vendored shape (with and without an env
marker, beside a `==` pin and a user's own wheel path), and a round trip
through the writer's own `vendor_line` formatter — the twin of the hosted
`the_hosted_rewriters_own_output_reinventories` guard. Both docstrings now
describe the two shapes they actually read.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(vendor): a zero-delta patch file needs no blob content to vendor
The JS-7 package is still unvendorable. a34bb9d stopped one contentless
view from killing a whole run, but the run's own example package —
`pkg:npm/tar-fs@2.1.1`, patch `8ff3e0c7-…`, one changed file plus seven
zero-delta fixture files — still fails on every run, and nothing in it
actually needs the bytes the view withholds.
`covered()` demanded the after-blob for EVERY file of a record. A zero-delta
file (`beforeHash == afterHash`) is already at its patched content in the
pristine copy: `verify_file_patch` answers `AlreadyPatched` as soon as the
on-disk hash equals `afterHash` (`patch/apply.rs`), which is exactly why the
view serves such a file with hashes and no `blobContent`. Requiring it made
every patch that carries one permanently unvendorable — measured on
origin/main as the original JS-7 symptom (exit 1, `status: error`,
`no_local_source`, zero events) and on a34bb9d as a per-package
`failed`/`no_local_source` on every run.
`needs_blob()` is now the one rule, used by `covered()` (which decides what
to fetch) and by the fetch loop (which decides whether a view came back
complete), so the two can never disagree about which files a fetch must
bring back. The loop also collects every genuinely contentless file instead
of breaking at the first one: `patch.files` is a `HashMap`, so "the first
file with no content" was bucket order, and a partial view abandoned its
remaining files at random.
Tests: `a_view_whose_only_contentless_files_are_zero_delta_vendors` is the
live JS-7 shape — red on unmodified main (`status: error`) and on a34bb9d,
green now, with the vendored tarball asserted to carry the changed file at
its patched bytes AND the zero-delta file at the bytes it always had.
`contentless_patch_view_fails_only_its_own_package` and
`every_patch_unstageable_keeps_the_run_level_error` (both added by a34bb9d)
encoded the wrong classification: they made their package unstageable with a
zero-delta file, i.e. they asserted that the JS-7 package must fail. Every
assertion in both is unchanged; only the fixture's view changed, so the
package they exercise is now unsatisfiable for a reason that really is
unsatisfiable — the file the patch CHANGES is served with no content, so its
patched bytes exist nowhere.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(vendor): name the real reason in a per-package staging failure
`drop_unstageable` recorded every dropped purl with one verbatim run-level
string, "patch artifacts unavailable (offline or download failure)". For the
case the per-package report was written for — the view is served fine, 200,
but a file the patch changes carries no `blobContent` — the run is neither
offline nor a download failure, so the one machine-readable explanation
named two causes that are both false.
The stager knew the real one (it formats `[error] <purl>: no blob content
served for <file>`), but every human channel in that block is gated on
`if !common.json`, so a `--json` consumer — depscan, CI — saw only the
misleading detail and never learned which file was contentless. The whole
point of the per-package report is per-package diagnostics, and the
per-package slot was the one place the specific reason was dropped.
Carry the reason out of the fetch loop with its purl and put it in that
package's `failed` event: which file was served without content (and how
many others), which file carried a malformed or undecodable blob, that no
view is served for the uuid at all, or the transport error. `no_local_source`
stays the stable `errorCode`; only the free-text `error` changes. The
`[error]`/summary stderr lines are untouched.
Pinned by an added assertion on the existing e2e: the failed event's `error`
must read "the patch view served no blob content for package/index.js".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(get): the --all-releases arm is purl-ordered, not a pass-through
acae5db made `filter_to_installed_releases` sort both arms by (purl, uuid)
but left the contract sentence "With `--all-releases` set this is a verbatim
pass-through" in place. Harmless today — `select_patches` hands this function
one patch per purl, so the uuid tiebreak never decides which record survives
the purl-keyed `records` map in `download_patch_records_preflighted` — but a
future caller would read a sentence that is no longer true. Say what the arm
does now: nothing is narrowed away and no view is fetched, and the output
order is the same one the narrowed arm returns.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(vendor): cover the vendored scan's per-package staging drop
`drop_unstageable` has three callers — `vendor`, `scan`/`get --mode
vendored` (`scan::vendor_flow`) and `repair` — and every test that reached
it drove `vendor`. The suites that touch the vendored scan
(`scan_vendor_step_error_e2e`, `covgap_commands_scan_vendor_flow`,
`covgap_commands_get`) all mount single-patch manifests, so they only ever
exercised the preserved whole-run bail: a regression in the vendored scan's
`Ok(staging_errors || engine_errors)` fold — dropping the staging-error bit,
or reporting a stuck package twice — passed the whole suite.
One mixed-selection case for `scan --mode vendored`, manifest-free the way
vendored mode really runs (discovery + the download phase's blob seed, no
`.socket/` on disk): one package whose view serves the file it changes
without content and one it serves complete. The stuck package is reported
exactly once as `failed`/`no_local_source` with the reason naming the file,
the other still vendors, and the envelope is `partialFailure`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs: record the vendored-envelope, ordering and gem changes
Four user- and consumer-visible behavior changes shipped on this branch
with no `[Unreleased]` entry, although the file's own header states the
Release workflow refuses to publish a version that does not appear in it
(`scripts/release-lint.sh`) and every recent merge to main updates it.
CHANGELOG `[Unreleased]` → `Fixed` now carries one entry per fix, each
calling out what a JSON consumer sees change: the vendor envelope's shift
from `status: "error"` + top-level `no_local_source` to `partialFailure` +
per-package `failed` events, the purl ordering of `download.patches` /
`apply.patches`, the requirements.txt inventory recovery, the zero-delta
staging fix, and the gem build-mode refusal — including the
`vendor_fetched_missing` warning that disappears from a build-mode run
that no longer downloads.
CLI_CONTRACT's error-code table gains the two facts a consumer needs and
could not previously read anywhere:
* `no_local_source` is now reported at TWO levels, and which one arrives
depends on whether anything else in the manifest staged (so a one-patch
manifest still gets the run-level shape). The table says so explicitly
rather than leaving the inconsistency undocumented, and `json_envelope`'s
`error_code` doc points at it.
* `gem_spec_missing` was never in the table at all. Its row states the
pre-fetch refusal, its exact scope (the lock both resolves AND verifies
the gem, and no ledger entry already vendors it), what is deliberately
unaffected (`vendor_fetch_unverifiable`'s documented pair, an
already-vendored re-run, `auto`/`service`), and the dropped warning.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(vendor): count the extra contentless files once, not twice
`plural()` already carries the count ("2 files"), so the multi-file arm of
the staging reason read "(and 2 other 2 files)". Say "(and 2 more files)",
and pin all three arms — none, one, several — with a unit test, since that
string is the only machine-readable explanation a `--json` consumer gets
for a per-package `no_local_source`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>1 parent afa7253 commit 0b4e645
15 files changed
Lines changed: 1871 additions & 74 deletions
File tree
- crates
- socket-patch-cli
- src
- commands
- scan
- tests
- socket-patch-core/src
- utils
- vendor
- lock_inventory
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
744 | 744 | | |
745 | 745 | | |
746 | 746 | | |
| 747 | + | |
| 748 | + | |
| 749 | + | |
| 750 | + | |
| 751 | + | |
| 752 | + | |
| 753 | + | |
| 754 | + | |
| 755 | + | |
| 756 | + | |
| 757 | + | |
| 758 | + | |
| 759 | + | |
| 760 | + | |
| 761 | + | |
| 762 | + | |
| 763 | + | |
| 764 | + | |
| 765 | + | |
| 766 | + | |
| 767 | + | |
| 768 | + | |
| 769 | + | |
| 770 | + | |
| 771 | + | |
| 772 | + | |
| 773 | + | |
| 774 | + | |
| 775 | + | |
| 776 | + | |
| 777 | + | |
| 778 | + | |
| 779 | + | |
| 780 | + | |
747 | 781 | | |
748 | 782 | | |
749 | 783 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1204 | 1204 | | |
1205 | 1205 | | |
1206 | 1206 | | |
1207 | | - | |
| 1207 | + | |
1208 | 1208 | | |
1209 | 1209 | | |
1210 | 1210 | | |
| |||
1268 | 1268 | | |
1269 | 1269 | | |
1270 | 1270 | | |
1271 | | - | |
| 1271 | + | |
1272 | 1272 | | |
1273 | 1273 | | |
1274 | 1274 | | |
| |||
1278 | 1278 | | |
1279 | 1279 | | |
1280 | 1280 | | |
| 1281 | + | |
1281 | 1282 | | |
1282 | 1283 | | |
1283 | 1284 | | |
| |||
0 commit comments