Skip to content

Sweep hosted_unwind, lock_cli, list, commands/mod (+ rollback dead branch) - #1864

Open
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
mainfrom
arch-refactor/sweep-cli-commands-hosted-list-lock
Open

Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
mainfrom
arch-refactor/sweep-cli-commands-hosted-list-lock

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Sweep of the next audited, unswept rows of coverage/files.tsv in crates/socket-patch-cli/src/commands/: hosted_bundle.rs, hosted_unwind.rs, list.rs, lock_cli.rs, mod.rs and pypi_reinstall.rs (clean). No other open PR touches these files. This follows the architecture audit's per-file notes (discussion #1830, living document Part 4).

Changes

hosted_unwind.rs (+ rollback.rs)

  • Deleted the dead HostedLegOutcome::unsupported field. Nothing ever wrote to it. Removing it also removes rollback's hosted_unsupported event loop and its exit-code conjunct (−11 lines in rollback.rs). The hosted_unsupported code appears nowhere else: not in the contract and not in any test.
  • Merged the two flush_error.is_none() blocks into one. Warning order is unchanged.
  • A dry run now skips the bun live_user_patch_keys read, since only the wet run uses it.
  • The module doc now names scan's withdrawn-patch caller. Removed 3 bare issue references.

hosted_bundle.rs

  • Bundle option fields are now moved with take() instead of cloned.
  • The stdin schema in the module doc was missing policySha256; it is now listed.

list.rs

  • Removed the Store as Source alias and its 6-line comment.
  • Replaced the PatchRecord literal with ledgers::uuid_only_record.
  • Replaced 3 RunWarning literals with RunWarning::new.
  • Removed a stale comment about read_manifest.
  • Inlined vendor_state_lenient, which had one caller and lived in mod.rs.
  • Moved a misplaced fixture doc onto vendor_entry.
  • Deleted ordering_is_deterministic_across_builds. It built the envelope twice from the same HashMap, so both builds iterate in the same order and the test can't fail.

mod.rs

  • Moved mod user_patches up with the other module declarations.
  • hosted_state_from_pins now uses uuid_only_record.
  • Tests fixed (3):
    • an_uncovered_manifest_record_beside_a_ledger_is_ambiguous asserted only .contains("--mode").
    • a_malformed_manifest_is_not_a_hosted_project asserted only is_err().
    • a_hosted_pin_over_a_ledger_entry_is_ambiguous asserted only is_err().
    • All three now compare the whole usage error, including the named purl. The expected strings are written out as literals.
    • hosted_pins_beside_a_ledger_are_ambiguous now does the same.
    • Red→green: I temporarily changed the (true, true) arm to return "<root> --mode". The old asserts still passed. The new tests failed (an_uncovered_… and a_malformed_…), and they pass again after the revert.

lock_cli.rs

  • Inlined emit, held_hint and held_message into their only callers.
  • The human branch of acquire_or_emit now prints through format_lock_error, so it shares one formatter with the sites that build their own envelope.
  • Removed the 24-line lifecycle essay; core apply_lock already documents the lifecycle.
  • Removed history notes about the "historical" try-once, --break-lock and the acquire-mkdirs change.
  • Fixed the doc overclaim on error_envelope_propagates_dry_run_and_command.
  • Tests:
    • The 3 held_message tests became one, lock_failure_renders_the_waited_budget.
    • Deleted held_hint_depends_on_whether_we_already_waited, a duplicate of format_lock_error_matches_acquire_or_emit.
    • waiting_message is still a separate function, because its status-line text is tested only through it.

Not done here, because each would reach files that #1698 touches or would change output:

Size

git diff --stat: 6 files, +160 / −300.

  • Production: about −105
  • Tests: about −35

Behavior change

None. Output bytes, JSON, exit codes and error codes are unchanged. Removing the dead hosted_unsupported branch changes nothing, because that list was always empty.

Verification

  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test -p socket-patch-cli --lib: 936 passed.
  • --test rollback, remove, in_process_rollback_hosted, in_process_rollback_vendored, scan_get_mode_from_project_state, e2e_safety_lock, cli_parse_list, command_module_layering and hosted_memory_parity: all pass.
  • No platform-specific code was touched.

/code-review high

Six findings. I fixed three:

  • acquire_or_emit now reuses format_lock_error.
  • The lock_failure doc no longer overclaims.
  • The merged test is renamed.

I did not fix three:

  • list --json doesn't report an unreadable vendor ledger in warnings[]. This is pre-existing; I only moved the code. Fixing it would change JSON output, so it belongs in its own issue.
  • The deleted determinism test "wasn't a tautology." It was: it reused one manifest instance, so both builds iterate the same HashMap.
  • The mode tests copy the production message. That is deliberate: expected values are written independently of the code under test.

Risk

Low. The changes are mechanical inlining and deletions, and the touched paths are covered by the suites above.

🤖 Generated with Claude Code

https://claude.ai/code/session_011f9aeQq8rjUYApktQhf6WR


Note

Low Risk
Mechanical refactors and deletion of dead rollback paths in CLI listing, lock, and hosted helpers; PR asserts output and exit codes stay the same aside from never-fired hosted_unsupported.

Overview
Architecture-audit cleanup across several socket-patch-cli command modules: mostly doc trims, shared helpers, and dead-code removal with no intended user-visible behavior change.

Hosted unwind and rollback drop the never-populated HostedLegOutcome::unsupported field and rollback’s hosted_unsupported JSON events / exit-code check. Hosted restore dry runs skip the bun live_user_patch_keys read; post-restore warning logic is merged into one flush_error-gated block.

List and mod centralize uuid-only patch stubs via ledgers::uuid_only_record, use RunWarning::new, rename ledger Store usage (no Source alias), and inline vendor-ledger lenient loading into list (stderr warning unchanged; --json still omits that warning). Mode-resolution tests now assert full --mode usage strings instead of partial checks.

Lock CLI inlines lock-failure formatting so human-mode acquire_or_emit goes through format_lock_error with lock_failure; redundant tests and long historical comments are removed.

Hosted bundle documents policySha256 on stdin and moves optional bundle fields into scan options with take() instead of cloning.

Reviewed by Cursor Bugbot for commit 1d557d4. Configure here.


Generated by Claude Code

Sweep hosted_bundle, hosted_unwind, list, lock_cli, commands/mod
and pypi_reinstall per the coverage notes.

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 Oct 29, 2026
HostedLegOutcome::unsupported was never written, so rollback's
hosted_unsupported events and its exit-code conjunct were dead.
run_hosted_leg now merges its two post-flush blocks and skips the
bun patch-key read on a dry run, which never used it.
hosted-bundle moves the bundle's option fields instead of cloning
them, and its module doc lists policySha256.

Assisted-by: Claude Code:claude-opus-5-5
list uses ledgers::Store directly, builds uuid-only records and run
warnings through their helpers, and reads the vendor ledger leniently
inline (vendor_state_lenient had one caller). The can't-fail
determinism test is gone. user_patches is declared with the other
modules, and the mode tests compare the whole usage error, so a
swapped agent/hosted variant fails them.

Assisted-by: Claude Code:claude-opus-5-5
emit, held_hint and held_message fold into their only callers, and
the lifecycle essay that core apply_lock already documents is gone.
The three held_message tests become one on lock_failure, and the
held_hint test duplicated format_lock_error's. Output is unchanged.

Assisted-by: Claude Code:claude-opus-5-5
The human branch now calls the formatter the self-enveloping sites
use, so the two cannot drift. The budget test is renamed after the
function it exercises.

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

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Oct 29, 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.

✅ 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 1d557d4. Configure here.

Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Oct 29, 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 Oct 30, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at 1d557d4. CI 82/82 green on this head; mergeable, no conflicts. Bugbot reviewed 1d557d4: no findings; no open review threads. Slack announcement not sent this run (Slack posting unavailable).


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at 1d557d417a6c10181fe2e9606eb9e330e3a17ec1.

  • CI: 32/32 run checks green (50 skipped by path filters); no main-wide failures.
  • Bugbot: reviewed 1d557d417a, no new issues; no unresolved review threads.
  • Mergeable: yes, no conflicts; waiting only on human approval.
  • Slack: not announced (Slack send unavailable in this run); the next run retries.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief: approver-ready

What it does: It is a behavior-preserving cleanup of five CLI command modules: it inlines single-use helpers, dedupes tests and trims docs. It also deletes the HostedLegOutcome::unsupported field. Nothing on main ever writes that field, so the hosted_unsupported event loop and its exit-code check in rollback.rs could never run.

Risk: low. Every user-visible string, error code, exit code and JSON field compares identical to main. The only runtime difference is that a dry-run unwind skips one read-only lockfile scan whose result it never used.

Look here

  • hosted_unwind.rs:77: the dry run now skips live_before. :123-165:`` the two post-flush blocks are merged, and their warnings still come out in the same order.
  • rollback.rs:2226: the dead hosted_unsupported field and its exit-code check are removed.
  • lock_cli.rs:19-41: human output now goes through format_lock_error, which prints byte-identical text. The trimmed lifecycle docs are still in socket_patch_core::patch::apply_lock.
  • list.rs:411-423: the lenient vendor-ledger read is inlined, with the same warning and the same silent gating.
  • hosted_bundle.rs:125-148:``` .clone() became `.take()` on fields `build_input` never reads.

Verified

  • I read the diff line by line against main.
  • cargo test -p socket-patch-cli --lib: 936/936 pass.
  • Integration suites: in_process_rollback_hosted 41, in_process_rollback_all_ecosystems 28, e2e_safety_lock 34, cli_parse_list 69, command_module_layering 3, hosted_memory_engine 49, all passing.
  • Deleted tests: what they checked is still covered by remaining tests, or the test could never fail.
  • CI: 32 success, 50 skipped, 0 failing.
  • Bugbot found no issues on 1d557d4. There are no open threads, and CHANGELOG.md is untouched.

Changes I made: none.

Open questions (non-blocking)

  • Commit 09a66fa is empty and names pypi_reinstall, which the PR doesn't touch. The squash merge hides this.
  • The PR title doesn't mention the rollback.rs edit.

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


Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) changed the title Sweep hosted_bundle, hosted_unwind, list, lock_cli, commands/mod Sweep hosted_unwind, lock_cli, list, commands/mod (+ rollback dead branch) Oct 30, 2026
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.

2 participants