Repository navigation
Decide: make --download-mode file the default and retire the diff download path #792
Description
Activity
I think this seems fine. We should try to clean up older and deprecated options we don't need any more.
[agent] Decision: option A, done fully in v5. This follows the maintainer's comment above and the cleanup direction in #966. v5 is not stable yet, so we skip the "keep
diffas an alias for one major" step and remove the diff path and the flag in one PR.What changes
- Patch content is always fetched as per-file blobs. A cold-cache
apply/get/scan --mode agent/repairmakes no/diffs/requests. --download-modeandSOCKET_DOWNLOAD_MODEare removed. Withdiffgone,filewould be the only value, so the flag would do nothing. Scripts that pass--download-mode …will get clap's unknown-argument error (exit 2). A leftoverSOCKET_DOWNLOAD_MODEenv var is ignored. This also makes An invalid --download-mode fails apply and repair with exit 1 but is accepted by every other command #791 moot, since there is no longer a value to validate..socket/diffs/is no longer read.repair,scanGC,removeandrollbacktreat it like.socket/packages/: every archive in it is obsolete and gets swept.--jsonappliedViais always"blob". The"diff"value is removed from the contract.
Code removed (main @
8cf19108)- core:
patch/diff.rsandpub mod diff;resolve_from_diff, the diff-first branch ofapply.rs(~L1011-1060) andAppliedVia::Diff;PatchSources::diffs_path;DownloadMode,DIFF_ARCHIVE,get_missing_archives,fetch_missing_diff_archivesand the mode switch infetch_missing_sources(api/blob_fetcher.rs);ApiClient::fetch_diff;package::read_archive_filtered; andqbsdiffin both crates'Cargo.tomland in the workspaceCargo.toml(including the[profile.dev.package.qbsdiff]block). - cli: the
download_modefield inargs.rs(L153-162, theDefaultimpl and the empty-env list); the diff branches offetch_stage.rs(patches_without_source,files_diffs_cannot_cover, the top-up at L377-398, deferred failures); theDownloadModematch and the created-files second pass inrepair.rs;download_modeforwarding inget.rs/scan/mod.rs; andDiffinjson_envelope.rs. - telemetry:
track_patch_fetchedloses itsdownload_modeparameter. It sends"download_mode": "file"as a constant so the event shape stays stable.
One correction to the issue:
patch/package.rsis not diff-only on current main. Its archive readers are used byvendor/*,hosted/npm_manifest.rsandredirect/vlt_heal.rs, so it stays. Onlyread_archive_filtered(used only by the diff path) is deleted. The saving is therefore smaller than the issue's ~700 lines, but the dependency and the extra round-trip per patch still go away.Docs
CLI_CONTRACT.md: remove the--download-moderow (L56) and theSOCKET_DOWNLOAD_MODEenv row (L1026); reword L652 to say vendored runs stage no blobs; changeappliedVia(L1176) to"blob"; extend the GC note (L822) to say.socket/diffsarchives are obsolete and swept; drop--download-modefrom the defaults example in L1679.docs/migrating-to-v5.md: add--download-mode/SOCKET_DOWNLOAD_MODEto "Retired spellings" (replacement: none, blobs are always used), and change the closing note to say.socket/diffs/and.socket/packages/are no longer read and are removed by cleanup.- No CHANGELOG edit (release agent).
Tests
- Delete
core/tests/diff_e2e.rs,cli/tests/diff_created_file_e2e.rs, the diff tests inapply.rs/fetch_stage.rs/repair.rs/blob_fetcher.rs, thefetch_diffcases inapi_timeout_e2e.rs, and the--download-modeparse tests. - Rewrite the ~50 test files that set
download_modeor mock/diffs/so they use blobs. - Add a cold-cache
applytest that asserts no/diffs/request is made, and a parse test that asserts--download-modeis rejected with exit 2. - Acceptance:
cargo tree -p socket-patch-clishows noqbsdiff, andcargo test --workspaceis green.
Coordination: #966 removes other
get/scanflags and touches the same files (args.rs,get.rs,scan/mod.rs,CLI_CONTRACT.md,migrating-to-v5.md,cli_parse_*). Whichever PR lands second rebases, and both add rows to the same "Retired spellings" table. Blob retry (#676) gets simpler because it now covers one artifact kind.A PR implementing this will follow.
Generated by Claude Code
- Patch content is always fetched as per-file blobs. A cold-cache
- added a commit that references this issue
on Oct 7, 2026 - added and removed
on Oct 9, 2026 v5 release blocker (P1). Finish the already accepted v5 removal in PR #1049 before publishing the interface. Include the removed flag/env spelling and legacy diff-only-state migration in the v5 migration contract.
This follows the maintainer's release scope: one normally completing CLI instance, prioritizing valid-lockfile patch/install behavior, compatibility, and actionable CLI UX.
[agent] Filed by the scheduled architecture audit routine (CLI and core). Register: discussion #560 register.
Kind: decision. Source: review Part 7.4 and R10; register C25.
Question
--download-mode diffis the default, but on a cold cache it downloads every diff archive and then every blob anyway. Shouldfilebecome the default, and should the diff download path be retired?Options:
filethe default now (a MAJOR perCLI_CONTRACT.md#L1578). Keepdiffas an accepted value that behaves likefilefor one major, then delete the diff machinery. Recommended.diffas the default, but stop the redundant blob top-up. Fetch blobs only for the files whose diff is missing or for created files, as is already done when every archive is cached. That keeps all the diff code and fixes only the bytes.Problem (verified at
045d7ec)default_value = "diff"(args.rs#L153-L162).fetch_stage.rs#L314-L319,`` diff mode downloads whenever any archive is missing. Then the top-up infetch_stage.rs#L377-L398sets `blob_scope = manifest` whenever `missing_diff_archives` was non-empty, and fetches every blob still missing from the stage. The diff fetch writes no blobs, so that is all of them..socket/diffsbut not.socket/blobs. Then just the blobs of created files are fetched (files_diffs_cannot_cover).patch/diff.rs(99 production lines, bspatch);patch/package.rs(332 production lines, diff archive tarball reader);fetch_missing_diff_archivesandget_missing_archivesinapi/blob_fetcher.rs;resolve_from_diff/AppliedVia::Diffinpatch/apply.rs;fetch_stage.rs(patches_without_source,files_diffs_cannot_cover, the top-up, deferred failures) andrepair.rs;qbsdiffdependency in both crates.Impact: every first
applyon a fresh CI checkout pays an extra round-trip per patch for archives it then backs up with blobs. Removing the path deletes roughly 700 production lines and a dependency.Proposed change (option A)
file(clapdefault_value, theGlobalArgs::default(),CLI_CONTRACT.mdline 56 and the env table, and the changelog as a MAJOR).diffstays accepted.diffan alias offile, and deletepatch/diff.rs,patch/package.rs, the diff fetch and coverage code inblob_fetcher.rs/fetch_stage.rs/repair.rs,AppliedVia::Diff,PatchSources::diffs_pathandqbsdiff.repairkeeps sweeping orphaned.socket/diffs/*once, then the directory is ignored.Size and scope
Acceptance criteria
socket-patch applyon a cold cache issues blob GETs only; a test asserts that no/diffs/request is made with the default.cargo tree -p socket-patch-clilists noqbsdiff; the apply/repair suites stay green with diff fixtures removed.Dependencies
--download-mode), Stream patch blob and diff downloads to disk (#571) #607 and Tracking: one retry primitive for the patch API client (JSON, vendor service, blob and diff) #676.Consolidated work — backlog review, 2026-10-08
The following standalone issues are now tracked here. Their closure consolidates scheduling; it does not mean their implementation is complete. Original reports and discussion remain linked below.
#791: An invalid --download-mode fails apply and repair with exit 1 but is accepted by every other command
The accepted removal in #792 / PR #1049 supersedes the old proposal to preserve and validate --download-mode. Finish that removal consistently across CLI arguments, environment configuration and core call sites, and test the resulting removal/unsupported-input behavior. Do not restore the option to satisfy its historical acceptance criteria.
Preserved scope and acceptance criteria from #791
Historical, superseded: Proposed change
pub download_mode: DownloadMode, withvalue_parser = DownloadMode::parse(keep it case-insensitive and keep theblobalias), so clap rejects a bad value with exit 2 for every command.DownloadMode::parse(..).map_err(..)?calls infetch_stage.rsandrepair.rs, and pass the enum throughget'sDownloadParams/nested-apply plumbing instead of theString.DownloadMode::as_tag().Historical, superseded: Size and scope
args.rs,fetch_stage.rs,repair.rs,get.rs(thedownload_mode: Stringfields),scan/mod.rs, and thetelemetry.rssignature.download_mode: "diff".to_string()→DownloadMode::Diff).Historical, superseded: Acceptance criteria
socket-patch <cmd> --download-mode bogusexits 2 with a clap usage error for every subcommand, and so doesSOCKET_DOWNLOAD_MODE=bogus.--download-mode packagestays rejected, with the "was removed; use diff or file" text.FILE,Diffandblobstill parse, as they do today.DownloadMode::parsecall remains outside the clap value parser.--download-mode bogusfor each subcommand inargs.rstests.test_download_mode_parseand the fetch_stage/repair tests stay green.