Skip to content

Sweep vendored_backend mod.rs and repair.rs - #1939

Merged
Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
arch-refactor/sweep-cli-vendored-backend
Nov 2, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
arch-refactor/sweep-cli-vendored-backend

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

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

Sweep of crates/socket-patch-cli/src/commands/vendored_backend/{mod,repair}.rs, from the audit's coverage notes for these rows (living document: discussion #1830, Part 4 structure). Callers in vendor.rs (#1698), rollback.rs (#1864), remove.rs and commands/repair.rs (#1875) are held by open PRs, so the crate-visible API is unchanged and only file-local cleanups land here.

Changes

mod.rs

  • Inlined revert_vendor_entry and deleted VendorRevertResult (its only caller was VendoredBackend::revert). The entry is now cloned once, so the separate flavor/gem pre-captures are gone.
  • Added VendorRevertStep::unwired() in place of two hand-written match lists (the pypi advisory's list is unwired() || WouldRevert).
  • Removed 7 bare issue references from comments.

repair.rs

  • Inlined workspace_copy_issue (a forwarder to ArtifactHealth::is_workspace_copy_issue), rebuild_reason_label and format_ledger_missing.
  • Candidate: dropped the unused Clone and the detached field, which copied entry.detached.
  • One ensure_client replaces the two copies of the build-once API client code, and drops the expect.
  • Tests: merged ui_format_tests into tests; renamed repair_returns_with_fifo_script → scan_skips_fifo_wiring_files (it tests the scan, not repair); removed the rebuild_reason_label passthrough assert along with the function. Removed 6 bare issue references from comments.

Size

git diff --stat: 2 files, +115 / −165. About +106 / −149 production and +9 / −16 test lines.

Merge with main

Merged main after #1954 to clear a merge-queue conflict: revert now passes the entry's wiring files (read from the cloned entry before the revert) to push_vendor_advisory, as main requires. Lib, in-process vendor/remove/rollback suites pass.

Behavior

None. Output bytes, JSON, exit codes and error codes are unchanged.

Verification

  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test -p socket-patch-cli --lib: 940 passed.
  • --test in_process_remove_repair_lifecycle, in_process_rollback_vendored, in_process_vendor, remove_rollback_api_overrides, remove, rollback: all passed.
  • --test repair: 127 passed, 2 failed. The failures are the known root-sandbox chmod tests (repair_cleanup_failure_is_reported_in_json_and_silent_modes, repair_exits_zero_and_stays_quiet_when_lock_file_unremovable), which pass in CI.
  • No cfg(windows)/cfg(unix) production code was changed.

/code-review high

It found no correctness bugs. I fixed 2 of its 8 low-severity findings: ensure_client no longer ends in an unwrap, and the scan tests' notes on the FIFO-safe reader and the one shared scan are restored. I left the other 6:

  • remove.rs L1327's doc still names revert_vendor_entry, and commands/repair.rs has its own build-once client copy. Both files are held by Sweep remove.rs and repair.rs #1875; recorded in the register for the next sweep.
  • The other => other arm in the preview label is unreachable, because a candidate's reason is always missing or corrupt. Making it an enum touches the event JSON reason field's source; it stays total.
  • Inlining puts the step classification in the loop. That is acceptable: it had one caller, and the rules are documented on VendorRevertStep.
  • The bare issue numbers were removed on purpose (the refactor procedure deletes unexplained issue numbers and keeps the why text).

Risk

Low. Mechanical inlining, covered by the in-process revert/repair suites.

🤖 Generated with Claude Code

https://claude.ai/code/session_011f9aePYVxtNghHHt1yrb1D


Note

Low Risk
Mechanical inlining and comment cleanup in vendored revert/repair paths; PR asserts unchanged behavior with broad CLI test coverage.

Overview
Refactors vendored_backend/mod.rs and repair.rs with no intended behavior change (same CLI output, JSON, and exit codes).

In mod.rs, revert_vendor_entry and VendorRevertResult are removed; their logic lives inline in VendoredBackend::revert, with one ledger entry clone up front instead of repeated lookups. VendorRevertStep::unwired() centralizes which revert outcomes count as “unwired” for PyPI reinstall advisories, gem stale-cache warnings, and go_undo_warnings. Bare GitHub issue numbers are stripped from comments while keeping explanatory text.

In repair.rs, small one-line helpers (workspace_copy_issue, rebuild_reason_label, format_ledger_missing) are inlined at call sites. Repair Candidate drops an unused Clone and redundant detached field (uses entry.detached). Shared lazy API client setup is factored into ensure_client, used by the redownload phase and fetch_record_by_uuid. Tests consolidate ui_format_tests into tests and rename the FIFO test to reflect that it exercises scan_vendor_references, not repair.

Reviewed by Cursor Bugbot for commit 0e72166. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko Mikola Lysenko (mikolalysenko) added refactor Structural change: duplicated code or logic, missing abstraction, layering, dead code arch-refactor PR opened by the scheduled architecture refactor routine labels Nov 1, 2026
Inline revert_vendor_entry and its result struct into
VendoredBackend::revert, and give VendorRevertStep one unwired()
predicate for the two lists that spelled it out. In repair.rs,
inline workspace_copy_issue, rebuild_reason_label and
format_ledger_missing, drop Candidate's Clone and its copy of
entry.detached, and build the lazy API client in one place, which
removes an expect. Merge the ui_format_tests module into tests,
rename the FIFO test after what it checks, and cut bare issue
numbers from comments. Behavior is unchanged.

Assisted-by: Claude Code:claude-opus-5-5
Take the slot and insert the client back, so ensure_client returns
the reference with no panic path. Restore the scan tests' notes on
the FIFO-safe reader and the one shared scan.

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

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Nov 1, 2026
Assisted-by: Claude Code:claude-opus-5-5

@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.

Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Nov 1, 2026
Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Nov 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review — burn-down agent.

  • Head: 823cfbf6ee313f0ab593d3c29a77eb7619c65feb
  • CI: all 82 check runs on this head succeeded or were skipped. None failed.
  • Bugbot reviewed 823cfbf6 and found no issues.
  • Mergeable with no conflicts. The PR doesn't touch CHANGELOG.md.
  • For the reviewer: this is a refactor sweep of vendored_backend/mod.rs and repair.rs only. Not announced in Slack yet: the send tool wasn't available in this run, so a later run will retry.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does: A behavior-preserving cleanup of vendored_backend/mod.rs and repair.rs (+115/-165). It inlines single-caller helpers (revert_vendor_entry, rebuild_reason_label, format_ledger_missing, workspace_copy_issue), adds VendorRevertStep::unwired() in place of two hand-written variant lists, merges the two build-once API client blocks into ensure_client (dropping an expect), and strips bare issue numbers from comments.

Risk: low. It is a mechanical refactor, and every variant list, user-facing string and side-effect order matched the old code (checked below).

Look here:

  • mod.rs:146-216:`` the inlined revert loop. Step branches, then entries.remove, then `save_state`, in the same order as before, with the same warning gating
  • mod.rs:323-333:``` unwired() = `Reverted | Preserved | LedgerWriteFailed`. Its uses at :186 (`dry || unwired()`, the same as the old PyPI list) and :235 (the same as the old Go list)
  • repair.rs:774-783:```` reads candidate.entry.detached in place of the removed field. `repair_workspace_copies` only changes `wiring`, so the value is the same
  • repair.rs:886-911:````` ensure_client keeps build-once behavior, and the token advisory still prints at most once

Verified:

  • Compared the old and new code for each inlined helper. The vendor_ledger_missing detail text is byte-identical after line continuation, and the offline guards are unchanged.
  • cargo test -p socket-patch-cli --lib vendored_backend: 10 passed. cargo clippy -p socket-patch-cli --lib: clean.
  • CHANGELOG.md is untouched and there is no debug code.
  • CI: 31 checks passed, 20 skipped, 0 failed (ci-ok and clippy green). Cursor Bugbot passed. No open review threads.

Changes I made: none.

Nits (not blocking): remove.rs:1327 still names the deleted revert_vendor_entry (that file is held by #1875, and the PR description already defers this). The fall-through other => other reason label lost its one-line test.

Auto-merge (squash) is armed, so approving sends this straight to the merge queue.


Generated by Claude Code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[agent] Reviewed 823cfbf6ee313f0ab593d3c29a77eb7619c65feb. Checked vendored revert step transitions, advisory timing, ledger failures and lazy repair-client reuse. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.

@mikolalysenko
Mikola Lysenko (mikolalysenko) added this pull request to the merge queue Nov 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 2, 2026
Resolve the conflict with #1954 in VendoredBackend::revert: the
entry's wiring files, read from the cloned entry before the revert
drops it, go to push_vendor_advisory as main now requires.

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 0e72166. Configure here.

@mikolalysenko
Mikola Lysenko (mikolalysenko) added this pull request to the merge queue Nov 2, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[final reviewer] Re-enqueued at 0e721669e: approved (approval stays valid after the post-approval pushes), ci-ok and clippy green, mergeable, no unresolved threads. Armed squash auto-merge, which put it back into the merge queue after the earlier dequeue.


Generated by Claude Code

Merged via the queue into main with commit e0023e5 Nov 2, 2026
50 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the arch-refactor/sweep-cli-vendored-backend branch November 2, 2026 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-refactor PR opened by the scheduled architecture refactor routine Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review refactor Structural change: duplicated code or logic, missing abstraction, layering, dead code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants