[Mikola Lysenko (mikolalysenko)]

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

Architecture refactor sweep of crates/socket-patch-cli/src/commands/remove.rs and repair.rs. It applies the local cleanups from those files' audit notes in coverage/files.tsv (discussion #1830, Part 4 structure and Part 5 tests).

Changes

remove.rs (production −62, tests +4)

• Inlined:
  • emit_not_found, at its one caller;
  • hosted_unwind_error, into emit_hosted_unwind_error.
• Deleted: the HostedUnwindError newtype. unwind_hosted now returns Result<_, String>.
• Built once instead of twice:
  • the hosted_reverted event (hosted_reverted_event);
  • the kept-entries exit tail, now kept_exit_code, used by both the manifest-backed path and the ledger-only path.
• Renamed print_hosted_leg_warnings to print_run_warnings. It also prints the nested rollback's warnings.
• Output plumbing: the 6 println!("{}", env.to_pretty_json()) calls now go through ui::print_json (byte-identical, because serde_json is built with preserve_order), and the 2 RunWarning literals now use RunWarning::new.
• Removed:
  • the DRY_RUN_FOOTER constant, inlined at its one use;
  • about 15 comment lines: bare #NNN references, history narration, and the parse_bool_flag story.
• Tests:
  • Deleted dry_run_footer_matches_repair, which compared a constant to its own literal and couldn't fail.
  • not_installed_warning_claims_kept_blobs_only_when_present now asserts the exact warning lines instead of .contains fragments.

repair.rs (production −52, tests −108)

• Dropped the noun: ArtifactNoun parameter from format_id_list, format_found_missing, format_nothing_missing, format_offline_warning and format_final_line. Production always passed BLOB. format_cleanup_summary keeps its nouns because it really covers three kinds.
• Removed: about 35 comment lines (history at the arg, the lock and telemetry narratives, bare #NNN references). The had_failure exit became i32::from.
• Output plumbing: 5 envelope prints now go through ui::print_json.
• Tests:
  • Deleted the tautological HOSTED_ONLY_REASON == "<literal>" assert. tests/repair checks the JSON reason by substring only, so its full text is no longer pinned (noted in the final review).
  • Deleted offline_warning_survives_multibyte_manifest_hash and dry_run_preview_survives_multibyte_manifest_hash, plus their fixture. The multibyte id case is asserted directly in id_list_is_sorted_capped_and_honest_about_truncation (and in core's display_id test).
  • Merged offline_dry_run_does_not_record_download_event and offline_missing_artifact_warns_without_failure into offline_missing_artifact_records_no_download_event, which runs both a wet run and a dry run. Red→green: dropping the !args.common.offline && conjunct from the download-event gate fails it with dry_run=true: events=[… "downloadMode": "file" …].

Totals

git diff --stat: 2 files, +188 / −406. Production −114, tests −104.

Not done

• Splitting remove's ~900-line run: too big for a sweep; it needs its own issue.
• The structural items (T16, T17, T19, T20) aren't in scope here.
• Typing repair's (String, String, String) vendor references: the type belongs to vendored_backend/repair.rs.
• /code-review high found no bugs. I applied three of its cleanups and declined four:
  • The Dry run: no changes made. literal now appears in both remove and repair. The old test only compared remove's constant with itself, so the two were never kept in sync anyway.
  • The not_found envelope stays hand-built, because emit_error_envelope sets a different status. Unifying the error emit is T20.
  • print_json is the canonical --json mechanism in Part 2.
  • kept/kept_causes are always pushed together.

Behavior change

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

Verification

• cargo clippy --workspace --all-features -- -D warnings is clean.
• cargo test -p socket-patch-cli --lib (remove and repair) passes: 25 tests.
• Integration suites remove, cli_parse_remove, cli_parse_repair, cli_remove_silent, in_process_remove_repair_lifecycle and remove_rollback_api_overrides all pass.
• repair: 127 pass and 2 fail, both because the sandbox runs as root and the tests rely on chmod (repair_cleanup_failure_is_reported_in_json_and_silent_modes and repair_exits_zero_and_stays_quiet_when_lock_file_unremovable). They pass in CI.
• No platform-specific code was touched.

Risk

Low. The changes are mechanical inlining, parameter removal and comment and test cleanup.

🤖 Generated with Claude Code

https://claude.ai/code/session_011f9aeSLfu6JCwzAqH82nG5

---

Note

Low Risk
Refactor-only changes to remove/repair with integration tests asserting unchanged behavior; no auth, security, or data-path logic changes.

Overview
Mechanical cleanup of remove and repair in socket-patch-cli: dedupe helpers, route --json through ui::print_json, and trim comments/tests without changing CLI behavior.

remove.rs inlines the old emit_not_found path and drops the HostedUnwindError wrapper (unwind_hosted returns Result<_, String>). Shared pieces are hosted_reverted_event, kept_exit_code, and print_run_warnings (renamed from hosted-only naming). Envelope printing and RunWarning::new replace ad-hoc JSON prints and struct literals.

repair.rs removes the redundant ArtifactNoun argument from blob-only formatters (always BLOB), uses print_json for envelopes, and tightens unit tests (merged offline download-event cases; multibyte ID coverage lives in format_id_list tests).

Net diff is mostly deleted lines (~400) with no intended change to stdout/stderr bytes, JSON envelopes, exit codes, or error codes.

^{Reviewed by Cursor Bugbot for commit b2e8bc9. Configure here.}

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ 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 b2e8bc9. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Ready for review at b2e8bc9 (burn-down agent).

• CI: 82/82 checks green on the head (32 success, 50 skipped, no failures). No main-wide failures.
• Bugbot: Cursor Bugbot check passed on this head; no open review threads.
• Mergeable with no conflicts; no CHANGELOG.md change.

Slack announcement not sent this run (the Slack send tool was unavailable); a later run will retry.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief: b2e8bc9

What it does. This is a refactor sweep of remove.rs and repair.rs, and it changes nothing the CLI prints or returns. It inlines single-caller wrappers and drops the HostedUnwindError newtype. The hosted_reverted event and the kept-entries exit tail were each written twice and now come from one helper each. It also deletes tautological tests and merges two offline repair tests into one.

Risk: low. I compared every merged block with the code it replaces:

• Telemetry still runs before the print.
• Error messages and codes are unchanged (hosted_revert_failed, "The manifest was not modified.").
• Event actions and reasons are unchanged.
• JSON bytes are unchanged: print_json serializes to_value(), and serde_json keeps key order (preserve_order).
• kept_exit_code now tests kept_causes where the old code tested kept. Both are pushed only together, at one push site, so the exit code is the same.

Look here

• remove.rs:516: the inlined not-found envelope
• remove.rs:1510: emit_hosted_unwind_error, which absorbs the old wrapper
• remove.rs:1525: hosted_reverted_event
• remove.rs:1540: kept_exit_code, used at :1200 and :1772
• repair.rs:253: format_id_list without its noun parameter; the merged offline test is at :814

Verified

• socket-patch-cli lib tests for remove and repair: 25 passed.
• Integration suites remove (104), in_process_remove_repair_lifecycle (41), cli_remove_silent (30), remove_rollback_api_overrides (21), cli_parse_remove and cli_parse_repair: all passed.
• repair suite: 127 passed, 2 failed. The 2 rely on chmod denying a write, which a root sandbox doesn't enforce; the PR names the same 2, and they pass in CI.
• CI: ci-ok and clippy are green. Cursor Bugbot found no issues. There are no open review threads, and CHANGELOG.md is untouched.

Open questions (non-blocking nits)

• The description says the JSON reason "stays pinned by tests/repair". The tests only check substrings (repair_invariants.rs:153 and :235), so the full message text isn't pinned.
• The repair lock comment (around :148) lost the line "a live holder makes repair refuse with lock_held; it never steals the lock", which was worth keeping.
• Two deleted multibyte-hash tests leave the formatter covered only by unit tests in CLI and core. That's acceptable.

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

---

Generated by Claude Code

[Wenxin Jiang (Wenxin-Jiang)]

[agent] Reviewed b2e8bc9742e81aa9cf8856096b66acb61a06a77a. Compared remove/repair output and error paths before and after the refactor, including kept-record exit codes, dry runs and hosted unwind failures. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.