GeezerVison is showing github.com — Remove --download-mode and the diff download path (#792) by mikolalysenko · Pull Request #1049 · SocketDev/socket-patch
Conversation
Removes the diff download path and the --download-mode flag. Patch content is now always fetched as per-file blobs.
Why
On #792 the maintainer said: "We should try to clean up older and deprecated options we don't need any more." The decision on the issue is option A, done fully in v5. v5 is not stable yet, so we skip the one-major diff alias and remove the diff path and the flag in one PR. With diff gone, file would be the only value, so the flag would do nothing.
What changed
Core
- Deleted
patch/diff.rs, the diff branch of apply, AppliedVia::Diff and PatchSources::diffs_path.
- Deleted
DownloadMode, fetch_missing_sources and get_missing_archives from api/blob_fetcher.rs. The download loop fetches only blobs, and repair calls fetch_missing_blobs directly.
- Deleted
ApiClient::fetch_diff and package::read_archive_filtered. The rest of package.rs stays, because vendor, hosted npm and vlt heal use it.
- Removed
qbsdiff and its profile blocks (qbsdiff, bzip2, libbz2-rs-sys, suffix_array) from every Cargo.toml and from Cargo.lock.
.socket/diffs is swept whole, like .socket/packages. cleanup_unused_archives is gone; only its own tests used it.apply_package_patch and apply_go_redirect no longer take a uuid argument. It only fed the diff lookup (about 40 call sites).- Removed apply's mismatch-blob top-up. Staging now fetches every afterHash blob before apply runs, and apply stops before writing if any download fails, so that code could never fetch anything.
CLI
--download-mode and SOCKET_DOWNLOAD_MODE are removed with no alias.- Staging in
fetch_stage.rs fetches blobs only, and repair has no second download pass.
appliedVia is always "blob".
User-visible changes
- Passing
--download-mode to any command is now clap's unknown-argument error (exit 2). A leftover SOCKET_DOWNLOAD_MODE is ignored.
- A cold-cache
apply/get/scan/repair makes no /diffs/ requests.
.socket/diffs/ is no longer read. repair, scan --prune, remove and rollback sweep every archive in it. The cleanup output still says "N unused diff archives" when it removes old ones.--json appliedVia no longer has the "diff" value.- Kept for shape stability: repair's
Downloaded/Verified event keeps details.mode, and the patch_fetched telemetry event keeps download_mode. Both are now always "file". Dropping them is the alternative if reviewers prefer it.
Docs
CLI_CONTRACT.md: removed the flag and env rows, added a "removed in v5.0" note, made appliedVia "blob" only, updated the GC, prune and repair-event text, and changed the defaults example to name --vendor-source.docs/migrating-to-v5.md: added a Retired spellings row and rewrote the closing note about .socket/diffs/.CHANGELOG.md is unchanged.
Tests
- Deleted
diff_e2e.rs, diff_created_file_e2e.rs and every diff and download-mode case.
- Added parse tests that
apply --download-mode file|diff and repair --download-mode file exit 2.
- Added cold-cache checks to
apply_network, the fetch-stage covgap test and the repair lifecycle test: no request path contains /diff and no .socket/diffs is created. The repair test also checks that a stale .socket/diffs/<referenced-uuid>.tar.gz is swept.
cargo test -p socket-patch-core --no-fail-fast: 6119 passed, 1 failed. The failure is utils::digest::tests::production_digests_go_through_the_helpers, which also fails on main and names files this PR doesn't touch.cargo test -p socket-patch-cli: the lib, the 35 integration targets this PR touches, rollback, vendor, spawn_env_hygiene, in_process_remove_repair_lifecycle and the coverage_fix_*_silent_mute_exit targets all pass. The lib, rollback, spawn_env_hygiene, in_process_python_envs and the core api::client tests were re-run after rebasing onto main.cargo tree shows no qbsdiff.- Not run locally: docker e2e and a full
cargo test --workspace. CI covers those.
- Clippy with
-D warnings reports one warning, an unused unix_default in python_crawler.rs. It comes from main, not this PR.
Review findings fixed
tests/spawn_env_hygiene.rs still listed the deleted diff_created_file_e2e.rs in PENDING_RAW_SPAWNS, which would have failed no_new_bare_binary_spawns. Removed it.- Repair's download-message unit tests used the diff-archive noun for output repair can no longer produce. They now use the blob noun.
- Removed stale comments that pointed to deleted code in
rollback.rs, rollback_multicopy_blob_gate.rs, in_process_remove_repair_lifecycle.rs, golang_local.rs and args.rs.
- rustfmt also reformatted a few unrelated lines in
vendor.rs and telemetry.rs. They are harmless.
Coordination
#966 touches the same files (args.rs, get.rs, scan/mod.rs, repair.rs, both docs, the cli_parse_* tests). Whichever PR lands second rebases, and both add rows to the Retired spellings table. Blob retry (#676) now covers one artifact kind, because diffs are gone.
Closes #792
Closes #791 (there is no --download-mode value left to validate)
🤖 Generated with Claude Code
Note Medium Risk
This changes the core patch fetch/apply path and is a breaking CLI removal; behavior depends on blob staging succeeding where diff archives previously covered gaps.
Overview
v5 removes diff-based patch downloads and the global --download-mode / SOCKET_DOWNLOAD_MODE controls. Passing the flag is a parse error; patch bytes are always loaded as per-file blobs under .socket/blobs/.
The apply/repair/staging pipeline is blob-only: fetch_stage no longer fetches or honors .socket/diffs/ archives, repair drops the secondary “created-file blob” pass, and apply removes the on-demand mismatch blob top-up (staging must fetch all needed afterHash blobs up front). qbsdiff and related lockfile deps are dropped. JSON appliedVia is always "blob"; repair telemetry still reports details.mode: "file" for compatibility.
Obsolete storage is swept, not read: leftover .socket/diffs/ and package archives are treated as junk and removed by repair, remove, scan --prune, and related GC—matching updated CLI_CONTRACT.md and e2e tests that assert no /diff API calls on cold cache.
Reviewed by Cursor Bugbot for commit 36e534e. Configure here.
Generated by Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Patch content is now always fetched as per-file blobs. v5 has not
shipped stable, so the deprecated diff path goes before it does.
Core:
- delete patch/diff.rs (bspatch) and the qbsdiff dependency
- apply: drop the diff strategy, AppliedVia::Diff,
PatchSources::diffs_path and the now-unused `uuid` parameter of
apply_package_patch / apply_go_redirect
- blob_fetcher: drop DownloadMode, get_missing_archives and
fetch_missing_sources; download_entries is blob-only. DIFF_ARCHIVE
stays as the noun for the obsolete-archive sweep output
- client: drop fetch_diff
- package.rs: drop read_archive_filtered only; the other readers are
used by the vendored and hosted backends
- cleanup: .socket/diffs is obsolete like .socket/packages and every
file in it is swept
- telemetry: patch_fetched keeps download_mode, always "file"
CLI:
- remove --download-mode / SOCKET_DOWNLOAD_MODE outright (no alias,
no warning), matching how #966 removes deprecated spellings
- fetch_stage stages blobs only; with every afterHash blob staged up
front, apply's mismatch-blob top-up is dead and is removed
- repair downloads blobs only; the Downloaded event keeps
details.mode, always "file"
- appliedVia is always "blob"
Tests: delete the diff e2e suites and diff/mode parse tests, add
removal parse tests, a cold-cache no-/diff-request assertion and a
stale referenced .socket/diffs archive sweep check.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
CLI_CONTRACT.md: drop the flag and env rows, note the removal, make
appliedVia "blob" only, and mark .socket/diffs obsolete in the GC
notes. migrating-to-v5.md: add the Retired spellings row and say
.socket/diffs archives are no longer read.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Remove the deleted diff_created_file_e2e.rs from the bare-spawn ratchet
(its stale-entry check failed), point leftover comments at the blob-only
pipeline, and test repair's download messages with the blob noun.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.
CodeQL flags the uuid as sensitive (cleartext logging); label the
entries A/B in the assert message instead.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
[agent] CI status: all green on 1357bcf (552 pass, 6 skipped).
- Fixed: CodeQL
rust/cleartext-logging alert in crates/socket-patch-cli/tests/remove/remove_duality_invariants.rs. The new assert message interpolated the archive uuid, so it now labels the entries A/B instead (3be03b0).
- Infra, not this PR:
hosted-e2e failed when corepack couldn't fetch yarn from registry.npmjs.org. The Windows Gradle multi-project e2e failed on a Maven Central 403. The CodeQL Analyze (rust) SARIF upload also failed, and default-setup runs can't be re-run, so an empty commit (1357bcf) retriggered the checks. All of them passed on the new head.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Oct 8, 2026
Keep this PR's removal of the diff download path: drop main's updated
mismatch-blob prefetch (ensure_blobs_for_mismatches / mismatch_blob_gaps)
and its tests, and main's --download-mode parse tests. Keep main's new
CLI_CONTRACT wording about added files and rollback, minus the diff text.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
[agent] Merged main, CI green; ready for review.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Ready for review (burn-down agent).
- Head:
9fc3cd30571fa4e08089138f0272e167e966ff3d
- CI: 520/520 check runs green (success/skipped/neutral) on this head, mergeable, no conflicts.
- Bugbot: reviewed this head (Cursor Bugbot check: success), no unresolved review threads.
- Changelog: untouched.
Nothing specific flagged for the reviewer beyond the PR description.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Resolve docs/migrating-to-v5.md: keep this PR's --download-mode removal
row and main's new SOCKET_FORCE row (#1021) in the removed-spellings table.
Co-Authored-By: Claude <[email protected]>
cursor[bot] reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
cursor
Bot
left a comment
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale Bugbot comment from a previous run.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Main (#1043) moved the agent download engine (DownloadParams,
DownloadRun, filter_to_installed_releases, nested_apply_args_from_params,
...) out of commands/get.rs into commands/agent_download.rs and moved
is_local_go into args.rs. This branch had edited that code in get.rs to
drop the --download-mode plumbing, so the conflicts were "deleted on
main, modified here".
Resolution:
- get.rs: take main's removal of the moved block, then re-apply this
PR's change at the new home: agent_download.rs loses the
DownloadParams::download_mode field, its forwarding into the nested
apply's GlobalArgs, and the --download-mode mention in the
nested-apply doc comment (GlobalArgs no longer has the field).
- commands/apply.rs imports: keep main's `is_local_go` from crate::args;
keep this PR's removal of `StagedSources` (only the deleted diff path
used it).
Also gate the core apply.rs `make_fixture` test helper on macOS: with
the diff tests gone, its only remaining caller is the macOS-only chflags
test, so `clippy --all-targets -D warnings` failed on Linux/Windows with
dead_code.
No docs still list --download-mode as a live flag. It appears only in the
v5.0 removal note in CLI_CONTRACT.md and in the Retired spellings row of
migrating-to-v5.md, next to main's SOCKET_FORCE row.
Co-Authored-By: Claude <[email protected]>
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
cursor
Bot
left a comment
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stale Bugbot comment from a previous run.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Ready for review at 024c78b4d.
- CI: every check on this head green; merges cleanly into current main (
88d5c598).
- Bugbot: reviewed this head, no new issues; no open review threads.
- Large deletion (+506/-5096 over 84 files): worth checking nothing outside the
--download-mode / diff-download path went with it.
- Slack: not announced (this run's Slack connector has no send tool); the next run should retry.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
main renamed is_valid_uuid to is_uuid_shaped, including inside
ApiClient::fetch_diff, which this branch deletes. Keep the deletion and
main's rename everywhere else.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
[final reviewer] Merged main at d06ce6f6 to clear a conflict in crates/socket-patch-core/src/api/client.rs. main had renamed is_valid_uuid to is_uuid_shaped, including inside fetch_diff, which this PR deletes. I kept the deletion. No new non-merge commits since the re-look request for 6773eaa and df96c49e. cargo build (core and cli) and the api::client tests (125) pass. clippy and rustfmt report nothing in the file.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
main's new in_process_target_ambiguity test (#1034) sets
GlobalArgs::download_mode, which this PR deletes, so the merge at
d06ce6f does not compile the cli test targets.
Co-Authored-By: Claude <[email protected]>
|
Burn-down agent: the main merge at d06ce6f didn't compile the cli test targets. main's new tests/in_process_target_ambiguity.rs (#1034) sets GlobalArgs::download_mode, and this PR deletes that field. Pushed 36e534e, which drops that one line; cargo check -p socket-patch-cli --all-targets, workspace clippy, and the core api:: + target-ambiguity/get/apply-parse tests pass locally. I removed Ready for review until CI is green on the new head.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
cursor[bot] reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 36e534e. Configure here.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Ready for review (burn-down agent).
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
[final reviewer] I disarmed auto-merge. Tanmay Singla (@Tanmay182003), three non-merge commits landed after your approval at 9fc3cd30, head now b9a36289:
6773eaa6 (product code, commands/fetch_stage.rs +7/−1 with a test): under --silent, a fatal staging failure now prints the per-blob hash and reason lines before the error, because the removed diff path used to print them. This addresses Bugbot 4223475496. --json stays silent on stderr.df96c49 drops --download-mode from the repair org-fallback test.36e534e drops the removed download_mode field from main's new tests/in_process_target_ambiguity.rs, so the test targets compile after the main merge.
Please take another look at 6773eaa6 in particular and re-approve when you're happy.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Mikola Lysenko (mikolalysenko)
added a commit
that referenced
this pull request
Oct 9, 2026
Resolve conflicts with #1049 (diff download path removed):
- ArtifactReferences::active keeps the afterHash and beforeHash blobs of
every active patch (#1316); the patch_uuids/diff-archive retention is
dropped because #1049 sweeps every diff and package archive as obsolete.
- after_removal builds on active() plus the originals of
removed-but-not-installed patches.
- cleanup_unused_blobs / format_cleanup_result / cleanup_archives stay
removed (no callers left); the archive-retention unit tests go with them.
- CLI_CONTRACT.md: repair row and scan --prune paragraph describe the
combined policy.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Mikola Lysenko (mikolalysenko)
added a commit
that referenced
this pull request
Oct 9, 2026
The merge group failed `cargo check --all-targets` (lib test target):
the read-only module-cache test added by this PR called
apply_go_redirect with 11 arguments, but #1049 ("Remove --download-mode
and the diff download path") removed the `uuid: Option<&str>` parameter
on main. Pass the current 10-argument signature; behavior and test
coverage are unchanged.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Mikola Lysenko (mikolalysenko)
added a commit
that referenced
this pull request
Oct 10, 2026
Resolve against main's removal of the diff download path (#1049) and
the restore-blob GC fix (#1316): repair drops the created-file blob
pass and keeps the GcReport carrier; remove keeps the archive noun
loop; the contract keeps "update" and drops the removed paidRequired
status; the envelope contract test uses AppliedVia::Blob.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Reviewers
+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
arch-refactor
PR opened by the scheduled architecture refactor routine
Ready for review
Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review
Milestone
No milestone
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.
|