Repository navigation
Finish every vendored revert through one shared helper with an explicit keep policy instead of 12 copied finish blocks #990
Copy link
Copy link
Closed as not planned
Closed as not planned
Copy link
Labels
agent:triagedarch-auditFiled by a scheduled architecture audit routine (see the architecture review discussion)Filed by a scheduled architecture audit routine (see the architecture review discussion)priority:p3refactorStructural change: duplicated code or logic, missing abstraction, layering, dead codeStructural change: duplicated code or logic, missing abstraction, layering, dead code
Description
[agent] Filed by the scheduled architecture audit routine (ecosystems and formats). Register: register comment.
Kind: refactor. Source: Part 5.3; register E24, child 1 of #989.
Problem
Twelve revert_*_opts functions end with the same hand-written finish:
- Build
RevertOutcome { success: true, kept_artifact: false, .. }. - Return on
dry_run. - Drift-keep (
outcome.keep_artifact(&uuid_dir_rel)). - Return on
keep_artifact(--preserve-state). remove_tree_and_prune(&uuid_dir, &project_root.join(SOCKET_DIR)), mapping the error.
The 12 permalinks are in #989. The copies differ only in their keep policy, and CLI_CONTRACT.md documents each policy:
| Policy | Backends |
|---|---|
DriftKeeps: keep on any drift |
gem, npm, pnpm, bun, bun binary |
KeepWhileReferenced(files): keep on drift only while any_live_file_references |
composer (composer.lock), maven legacy (pom.xml), nuget (wired files) |
RefuseIfStillWired(files): lock_text_mentions_uuid makes it an error, vendor_lock_still_wired_revert_blocked |
yarn classic (yarn.lock), berry (yarn.lock, package.json) |
DriftOrResidualKeeps, where a removal failure is a warning |
pypi |
They also differ in small ways nobody chose:
- The removal error text is "failed to remove {}" in some backends and "cannot remove {}" in others.
- composer nests the finish step under
if !dry_runinstead of returning early.
Proposed change
- Add
vendor::revert::finish(outcome, project_root, uuid_dir_rel, opts, KeepPolicy) -> RevertOutcomenext toRevertOutcomeinvendor/mod.rsor a newvendor/revert.rs. - Replace the 12 finish blocks with one call each, passing the backend's current policy.
- Keep each backend's error text byte-identical, or pick one text and update the tests that assert it in the same PR.
- Leave cargo, golang and uv alone, because their removal paths differ. List them in the PR as follow-ups.
Size and scope
- About −300 / +90 production lines across the 12 backend files plus the new helper.
- This is mechanical: no policy change, and no change to warning codes or the JSON shape.
- Out of scope: the record loop (checklist step 2 of Tracking: revert every vendored backend through one record-revert engine instead of nine hand-written mechanisms #989), unifying the policies (step 6), and Vendored gem revert restores Gemfile.lock but leaves the Gemfile path: line when that line carries a trailing comment, and no re-run can finish it #988's gem fix.
Acceptance criteria
-
grep -n "remove_tree_and_prune(" crates/socket-patch-core/src/vendor/{bun_binary,bun_lock,composer_lock,gem,maven_repo,npm_lock,nuget_feed,pnpm_lock,pypi,vlt_lock,yarn_berry_lock,yarn_classic_lock}.rsfinds no call left in a revert function. - One unit test per
KeepPolicyvariant in the new module covers dry run, drift,--preserve-stateand removal. - The existing per-backend revert tests stay green unchanged. That includes every
drift_skipped/kept_artifactassertion, the yarnvendor_lock_still_wired_revert_blockedtests and the pypivendor_artifact_remove_failedtest. -
cargo test -p socket-patch-core vendor::and the CLIin_process_vendorsuites pass.
Dependencies
- Blocks Tracking: revert every vendored backend through one record-revert engine instead of nine hand-written mechanisms #989 step 2.
- It touches the same files as Build npm-family vendor ledger entries through one constructor instead of seven literal VendorEntry blocks #922 (npm
VendorEntryconstructor) and Route vendored revert and in-use dispatch through one core VendorBackend enum instead of CLI string matches #960 (dispatch), but different functions, so either order works.
Backlog review — 2026-10-08
Consolidated into #989. The retained tracker(s) preserve this issue’s implementation scope and acceptance criteria. Closing this separate scheduling item as not planned, not as completed.
Explicit shared revert-finish child; retain its per-backend keep-policy table under the parent rather than another issue.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
agent:triagedarch-auditFiled by a scheduled architecture audit routine (see the architecture review discussion)Filed by a scheduled architecture audit routine (see the architecture review discussion)priority:p3refactorStructural change: duplicated code or logic, missing abstraction, layering, dead codeStructural change: duplicated code or logic, missing abstraction, layering, dead code
Activity
audit-ecosystems: file E24 (#989, #990), add E68 (#988)
mikolalysenko commentedon Oct 7, 2026
[agent] Triaged as
priority:p3(cross-cutting refactor, no behavior change). It's actionable as written: there are 12 finish blocks with the 4 keep policies listed, and the acceptance criteria can be tested. It's the first child slice of #989.Generated by Claude Code
refactor: re-rank queue, add #990 (E24)