Skip to content

Sweep bun_preflight, composer_hints, context, fetch_stage - #1855

Open
Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
mainfrom
arch-refactor/sweep-cli-commands-bun-fetch
Open

Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
mainfrom
arch-refactor/sweep-cli-commands-bun-fetch

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

This PR sweeps four files in crates/socket-patch-cli/src/commands/: bun_preflight.rs, composer_hints.rs, context.rs and fetch_stage.rs. The work list came from the audit notes in the coverage/files.tsv rows on the arch-audit/ledger branch (living document: discussion #1830). Behavior, output bytes, JSON and exit codes don't change.

I didn't touch the callers in get.rs, agent_download.rs, apply.rs or vendor.rs, because the open #1698 changes those files. That rules out two items from the audit notes:

  • collapsing the three bun_vendor_preflight* entry points;
  • having StageOutcome::Unavailable carry its code/detail.

Changes

bun_preflight.rs (production −41)

  • Inlined refusal_with_exemptions (one caller) into preflight_pairs.
  • The per-purl lock probe now runs only for the workspace-gate code. Before, it was a loop that did nothing for every other code.
  • Added one is_npm helper in place of three copies of the npm purl test.
  • Comments: about 45 lines of narration trimmed down to the reasons.

composer_hints.rs (tests +11)

  • Removed three bare #658 references.
  • Replaced the sentence .contains checks with exact-string assert_eq! on the hint lines. This covers both vendor dirs (vendor, lib) and both branches of hosted_reinstall_hint.

context.rs (−2)

  • The module doc said vendor and vex use this module. The real users are scan, list, get and apply.
  • A doc link pointed to discover_wiring; the code calls discover_wiring_in.
  • Dropped the bare #745 and two comments that only restated the code.

fetch_stage.rs (production −39, tests −51)

  • Inlined report_offline_missing (one caller, and its remedy parameter only ever took one value), plus the single-use constants APPLY_OFFLINE_REMEDY and DOWNLOADING_ARTIFACTS.
  • Deleted the orphaned doc comment left from the removed in-memory vendor stager, and fixed the stale "(apply, vendor)" module doc.
  • Deleted stage_offline_ignores_obsolete_diff_archive and stage_online_fetch_failure_ignores_legacy_package_archive. Nothing in the crate reads .socket/diffs or .socket/packages, so neither test could fail. Each one also duplicates the matching no-source test. /code-review pointed out that they guard against a future fallback to those stores. I kept the deletion anyway: a test for code that doesn't exist checks nothing today.
  • stage_reads_socket_dir_in_place_when_fully_cached now runs online as well as offline. Before, only the offline path ran, so the missing_blobs.is_empty() fast path was never tested.

tests/apply/apply_silent_staging_errors.rs: a doc line named the deleted helper; it now describes what that helper did.

Red → green

  • In stage_patch_sources, I broke if common.offline || missing_blobs.is_empty() to if common.offline. The updated fully-cached test fails; the old version passed against this break.
  • In the bun preflight, I flipped the workspace-gate condition. lock_derived_exemption_matches_the_engine_gate fails.

Diff stat

4 source files plus one test doc line: +170 / −292. Production is about −82 lines, tests about −40.

Verification

  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo check --locked --workspace --all-targets --all-features: clean.
  • cargo test -p socket-patch-cli --lib: 938 passed.
  • --test apply: 157 passed.
  • --test vendor: 151 passed.
  • /code-review high: no correctness findings. I fixed its test and doc findings, except the archive-test deletion explained above.
  • Bugbot: no issues.
  • CI: both attempts on bdc7d7b lost their Depot runners before any step ran (clippy, coverage (2), e2e-build). coverage (1) passed. This PR needs a fresh CI run once the runners recover.

Behavior change: none. Risk: low. No platform-specific code changed.

https://claude.ai/code/session_011f9aeTCACoNHTCNfSXXGDT


Generated by Claude Code

Sweep bun_preflight.rs, composer_hints.rs, context.rs and
fetch_stage.rs under crates/socket-patch-cli/src/commands/.

Assisted-by: Claude Code:claude-opus-5-5
refusal_with_exemptions had one caller; fold it into preflight_pairs
and skip the per-purl lock probe loop entirely unless the code is the
workspace gate. One is_npm helper replaces three copies of the npm
purl test. Comments trimmed to the why.

Assisted-by: Claude Code:claude-opus-5-5
The hint tests checked sentence fragments with .contains; they now
compare the whole line, so a wording change in any part of a hint
fails a test. Bare #658 references removed.

Assisted-by: Claude Code:claude-opus-5-5
The module doc listed vendor and vex as users; the real users are
scan, list, get and apply. The discovery doc linked discover_wiring
where the code calls discover_wiring_in.

Assisted-by: Claude Code:claude-opus-5-5
report_offline_missing had one caller and a remedy parameter with one
value; it and the two single-use constants are inlined into
stage_patch_sources. The orphaned doc comment for the long-gone vendor
stager and the stale "apply, vendor" module doc are deleted.

Tests: the two archive tests could not fail (nothing reads
.socket/diffs or .socket/packages) and duplicated the no-source tests,
so they are deleted. The fully-cached test now runs online as well as
offline, so it covers the missing_blobs.is_empty() fast path.

Assisted-by: Claude Code:claude-opus-5-5
Restore the exact relocated-vendor-dir assertion for the Composer 1
note, assert it by literal rather than by a second call, and correct
three doc comments (patches_without_source, the vlt case of the bun
preflight, and a stale helper name in the silent staging suite).

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 29, 2026 21:11
@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
@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 bdc7d7b. Configure here.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] CI on bdc7d7b: clippy, e2e-build (ubuntu-latest) and coverage (1) failed with zero steps recorded. Their Depot runners were lost before checkout, so ci-ok failed too. No step ran, so this failure isn't caused by this PR. Locally, cargo clippy --locked --workspace --all-features -- -D warnings and cargo check --locked --workspace --all-targets --all-features both pass. I re-ran the failed jobs once.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] The re-run lost its Depot runners as well: clippy was cancelled after 1 step, and coverage (2) and e2e-build (ubuntu-latest) failed with 0 steps. coverage (1) passed. I've used my one re-run, so this PR needs a fresh CI run once the Depot runners recover. The code has no known failure: Bugbot is clean, and clippy and check pass locally.


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
@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 29, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at bdc7d7bbb950f46a5978262744a125896e2aeedf.

  • CI: 32/32 run checks green (50 skipped). The earlier failures were Depot runner loss with no test failing; the re-run of those jobs passed.
  • Bugbot: reviewed bdc7d7b, no findings; no open threads.
  • Reviewer note: a cleanup of bun_preflight, composer_hints, context and fetch_stage under crates/socket-patch-cli/src/commands/, plus one test file.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does: A behaviour-preserving cleanup of four CLI command modules: bun_preflight, composer_hints, context and fetch_stage. It inlines three helpers that each had a single caller, fixes stale module docs, turns most composer-hint test checks into exact-string asserts, removes two redundant staging tests, and runs the fully-cached staging test both online and offline.

Risk: low. I read every hunk and found no behaviour change. The inlined offline error is byte-identical, including the !json gate. The inlined refusal keeps the same order (vlt check, preflight_vendor, lazy ledger load, unreadable-state check, exemption). The lock probe was already skipped by short-circuiting before, so moving it does no new I/O.

Look here:

  • fetch_stage.rs:163-181:`` the inlined offline report
  • bun_preflight.rs:120-145:``` the inlined refusal and exemption
  • composer_hints.rs:187-213:```` the exact-string asserts, which I checked against the format strings at lines 78-134
  • fetch_stage.rs:304-329:````` the fully-cached test, now looping over online and offline

Verified: I read the full diff and checked coverage for the two deleted tests. The legacy-archive case is still covered end to end by tests/apply/apply_network.rs:893. The obsolete-diff-archive case covers a directory nothing reads (CLI_CONTRACT.md:121), and its path is also exercised by stage_offline_with_missing_sources_is_unavailable_and_writes_nothing. CHANGELOG.md is untouched. CI is green on bdc7d7b (32 passed, 50 skipped, ci-ok and clippy both green). Bugbot found no new issues.

Changes I made: none.

Open questions: none. One nit: the diff stat in the description is slightly off (+175/−288 actual).

Auto-merge (squash) is armed, so approving sends this 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

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