Skip to content

Fix rollback over unledgered vendor wiring (#991) - #1739

Merged
Mikola Lysenko (mikolalysenko) merged 7 commits into
mainfrom
agent/fix-rollback-unledgered-vendor-wiring
Oct 27, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 7 commits into
mainfrom
agent/fix-rollback-unledgered-vendor-wiring

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-opus-5-5

Fixes #991

What changes for users

Two branches each vendor a different package. Git merges yarn.lock cleanly, but .socket/vendor/state.json conflicts and the user keeps one side. The lockfile still wires the other side's package into .socket/vendor/npm/<uuid>/, and no ledger entry owns that dir.

  • Before: a bare rollback reverted the ledgered package and exited 0 with only a vendor_orphan_still_wired advisory. It then deleted state.json, so the other package stayed patched and committed. Every later rollback / scan --mode vendored failed with "restore state.json from version control". The advisory told users to run repair, which no longer rebuilds ledger entries in v5.
  • After: an unscoped rollback (wet, --dry-run and --preserve-state) refuses before writing anything. It exits 1 with vendor_ledger_missing and names each unowned dir. The remedy: restore the missing entries, and after a merge that kept one side, merge both sides' entries; or restore the lockfile. This is the same refusal rollback already gives when the ledger is missing entirely. Once the entries are merged, rollback unwinds both packages byte-exact (checked with real yarn 1.22.22).
  • The vendor_orphan_still_wired advisory (vendor --revert, scan --prune, scoped rollback, remove) now names the ledger restore instead of repair. It does not suggest checking out the lockfile, because it follows a revert that already rewrote it (a Bugbot finding).

The vendor --check half of #991 was already fixed on main (re-verified in the issue on 10-14). vendor --check and rollback now share one helper, unledgered_vendor_references, for "which lockfile references does the ledger not own".

Root cause

rollback's ledger guard (rollback.rs, "Ledger-less but still wired?") ran only when state.json was absent. With a partial ledger, the unowned wiring reached the #1610 orphan sweep. That sweep treats a wired orphan as an advisory, so the run reverted every ledgered entry, saved an empty ledger (which deletes the file) and exited 0.

Changes

  • commands/rollback.rs: after scope resolution and before the prompt or any write, an unscoped run with project state checks the lockfile references against the loaded ledger. Any in---ecosystems reference whose uuid no entry carries gets a vendor_ledger_missing refusal, recorded in telemetry like the other post-lock errors. An entry whose ecosystem has no backend still owns its uuid, so its own revert reports vendor_revert_failed (the existing vendored_unknown_ecosystem_* tests).
  • vendored_backend/repair.rs: a new shared unledgered_vendor_references. vendor --check (vendor.rs) now uses it.
  • vendor_orphans.rs: a shared UNLEDGERED_WIRING_REMEDY, and the advisory text no longer points at repair.
  • CLI_CONTRACT.md: rollback State discovery, the vendored-leg note and the vendor_ledger_missing row.

Tests (red on main, green here)

Issue Test Red on main Green
#991 (yarn classic, real yarn) e2e_vendor_yarn_classic_build::yarn_classic_rollback_refuses_a_lock_reference_the_ledger_lost: merge flow from the issue, dry and wet refusal, nothing written, then the merged-entries remedy unwinds both byte-exact rollback (dry=true) must refuse … left: 0 right: 1 ok
#991 (npm lock) in_process_rollback_vendored::unscoped_rollback_refuses_a_still_wired_orphan (replaces …_keeps_a_still_wired_orphan, which asserted the exit-0 behavior): dry, wet and --preserve-state all refuse; lock, ledger and manifest are byte-identical afterwards; a scoped rollback still reverts its target rollback ["--dry-run"] refuses … left: 0 right: 1 ok
#991 (remedy text) vendor_orphans::tests::orphan_still_wired_names_a_working_remedy and in_process_rollback_vendored::scoped_rollback_keeps_a_still_wired_orphan_of_its_target FAILED (advisory said socket-patch repair) ok

Commands run locally (Linux, root container):

  • cargo fmt --all -- --check: ok. cargo clippy --workspace --all-features -- -D warnings: ok.
  • cargo test --workspace --all-features --no-fail-fast: everything this PR touches passes. That includes the rollback suite (122), in_process_rollback_vendored (23), in_process_vendor, repair, vendor_jvm_cli, e2e_vendor_composer_build, e2e_sbt_vendor and e2e_vendor_yarn_classic_build (41, with SOCKET_PATCH_YARN_E2E_REQUIRED=1). Two kinds of failure are environmental and fail identically with origin/main's src/. (1) Tests that inject write failures with chmod don't fail as root: repair_cleanup_failure…, repair_exits_zero…lock_file_unremovable, 3 in_process_redirect write-failure tests and 4 socket-patch-core unit tests (core is untouched). (2) Two e2e_bun_lockb tests need patches-api.socket.dev, which the sandbox proxy blocks.
  • node --test npm/socket-patch/bin/socket-patch.test.mjs: 24 pass. The wrappers only dispatch to the binary, so no wrapper changes are needed.

CI note

Commit 274386a ports #1738's unearth<0.19 pin (test harness only) because unearth 0.19 broke the PDM e2e leg on main. It is a no-op once #1738 merges.

/code-review high outcomes

Fixed: the misplaced emit_rollback_error doc comment, the missing telemetry on the refusal, the duplicated reference-vs-ledger comparison (now shared with vendor --check), singular/plural remedy grammar, a redundant dedup, and the lost rollback coverage of vendor_orphan_still_wired (new scoped test).

Not fixed, with reasons (follow-ups, not claimed by this PR):

  • Scoped / --ecosystems-narrowed rollback can still revert the last ledger entry and delete state.json while another package's wiring is unowned. A scoped run doesn't promise to unwind that package, and the ledger it deletes held nothing for it. The next unscoped rollback, or vendor --check (the CI gate), names the wiring. Refusing a targeted rollback over an unrelated package would block a legitimate request.
  • Unscoped vendor --revert keeps the still-wired orphan with an advisory and exits 0. e2e_vendor_composer_build asserts that on purpose ("sweeping is not a failure"), so changing it is a separate decision for that command.
  • JVM vendor trees (maven2 / gradle / coursier) are not uuid-keyed, so scan_vendor_references can't see them. A merge that loses a jvm entry is not covered here. vendor --check handles JVM through its own LEDGER_OWNED_PATHS path.

https://claude.ai/code/session_011f9aePVxAgoVYdQqvzxbPm


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
Regression tests for #991. After a git merge keeps one side of
.socket/vendor/state.json, the lockfile still wires the other side's
package into .socket/vendor/ with no ledger entry. An unscoped
rollback should refuse before writing anything instead of reporting
success, deleting the ledger and leaving that package patched.

- in_process_rollback_vendored: the still-wired orphan case now
  expects a vendor_ledger_missing refusal (dry run, wet run and
  --preserve-state), with the lock, ledger and manifest untouched.
- e2e_vendor_yarn_classic_build: the merge flow from the issue with
  real yarn classic, ending with the merged-entries remedy unwinding
  both packages byte-exact.
- vendor_orphans: the vendor_orphan_still_wired advisory must not
  send users to `repair`, which no longer rebuilds ledger entries.

These fail on main.

Assisted-by: Claude Code:claude-opus-5-5
When a git merge keeps one side of .socket/vendor/state.json, the
lockfile can still wire the other side's package into .socket/vendor/
with no ledger entry. A bare rollback reverted the ledgered packages,
exited 0 and deleted the ledger, leaving that package patched and
every later run asking for a ledger no commit had (#991).

An unscoped rollback now refuses up front with vendor_ledger_missing,
naming each unowned dir and how to recover (merge both sides' ledger
entries, or restore the lockfile), and writes nothing. This matches
the existing refusal when the ledger is missing entirely. A scoped
rollback does not own that wiring and proceeds as before.

The vendor_orphan_still_wired advisory no longer tells users to run
`repair`, which stopped rebuilding ledger entries in v5.

Assisted-by: Claude Code:claude-opus-5-5
Review follow-ups for the #991 refusal:

- vendor --check and rollback now read lockfile references the ledger
  does not own through one helper, so a fix to that comparison reaches
  both commands.
- The rollback refusal is recorded as a failed rollback in telemetry,
  like the other errors raised after the lock is taken.
- The remedy reads correctly when several entries are missing, and
  emit_rollback_error keeps its own doc comment.
- A test covers a scoped rollback that keeps its target's still-wired
  orphan, checking the advisory's remedy text.

Assisted-by: Claude Code:claude-opus-5-5
A ledger entry whose ecosystem has no backend in this build (or was
hand-edited) still owns its uuid dir. The new rollback refusal matched
on ecosystem and uuid together, so it refused such a project as
"unledgered wiring" instead of letting that entry's own revert fail
with vendor_revert_failed. Rollback now treats any entry carrying the
uuid as the owner.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 27, 2026 09:12
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

Ported from #1738 so this PR's PDM e2e leg can go green. unearth 0.19
moved to httpx2, and PDM 2.x's unbounded unearth now resolves it, so
`pdm lock` crashes with "PDMPyPIClient is not an instance or subtype
of type (Client)". This is a no-op once #1738 lands on main.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] CI on 2120d22: the only failure was e2e (ubuntu-latest, e2e_vex_build, pdm:: --ignored, 2.29.2). pdm lock crashed with "PDMPyPIClient is not an instance or subtype of type (Client)" because unearth 0.19 moved to httpx2. This PR doesn't cause it: it breaks the PDM leg on main and every PR. I ported the existing fix from #1738 (pin unearth<0.19, 2 files, test harness only) as a separate commit. It becomes a no-op once #1738 merges.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

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.

Comment thread crates/socket-patch-cli/src/commands/vendor_orphans.rs
The vendor_orphan_still_wired advisory follows a revert that has
already rewritten the lockfile, so telling users to run
`git checkout -- <lockfile>` would bring back the wiring that revert
just removed. The advisory now names only the ledger restore. The
up-front rollback refusal, which writes nothing, still offers
returning the lockfiles to their pre-vendor versions.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

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 dcd5127. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 27, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at head dcd5127.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at head dcd5127.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does: An unscoped rollback now refuses up front (vendor_ledger_missing, exit 1, nothing written, also on --dry-run/--preserve-state) when a lockfile is still wired into a .socket/vendor/<eco>/<uuid>/ dir that the vendor ledger has no entry for, which is what a git merge that kept one side of state.json leaves behind. Before, it reverted everything else, exited 0 and deleted the ledger, leaving that package patched with no way to unwind it (#991). A scoped rollback still proceeds, and the vendor_orphan_still_wired advisory stops pointing at repair, which can't rebuild ledger entries in v5.

Risk: low–medium. The change is small and fails closed, but it's a new hard stop on the unscoped path: one unledgered reference now blocks the agent and hosted legs too, the same way the existing "ledger missing" refusal does.

Look here:

Verified:

  • Read the full diff.
  • cargo test -p socket-patch-cli --test in_process_rollback_vendored: 23/23 pass.
  • With rollback.rs reverted to main, unscoped_rollback_refuses_a_still_wired_orphan fails, so the test covers the fix.
  • The tests check that the lockfile, ledger and manifest are byte-identical after the refusal, on all three flag variants, and that a scoped run still reverts its target.
  • CI 37/37 non-skipped green (ci-ok and clippy included), Bugbot success, 0 unresolved threads, CHANGELOG untouched.
  • Also carries Pin unearth<0.19 in the PDM e2e bootstrap #1738's unearth<0.19 PDM pin. It's identical to Pin unearth<0.19 in the PDM e2e bootstrap #1738, so it merges cleanly either way.

Changes I made: none.

Open questions (non-blocking):

  • Is blocking the agent and hosted legs on an unscoped run the intended trade-off? It mirrors the existing missing-ledger refusal, and a scoped rollback is the escape hatch.
  • Nit: the doc comment on orphan_still_wired_names_a_working_remedy says the advisory names "or the lockfile restore", but the text (correctly) doesn't.

Auto-merge is armed: approving sends it straight to the merge queue.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

3 participants