Skip to content

refactor(cli): sweep vex_consumed.rs and vex_sources.rs - #1948

Open
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
mainfrom
arch-refactor/sweep-cli-vex-consumed-sources
Open

Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
mainfrom
arch-refactor/sweep-cli-vex-consumed-sources

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

Architecture sweep of crates/socket-patch-cli/src/commands/vex_consumed.rs and vex_sources.rs. These are the next free rows in coverage/files.tsv, and their notes are tracked in the architecture discussion #1830. No open PR touches either file. vex.rs (held by #1698 and #1893) is not changed.

Changes

vex_consumed.rs

  • Deleted the not_installed() wrapper (9 call sites now use HostedCopies::default()).
  • Deleted the cfg(test) forwarder npm_identity_fallback. The _reusing variant takes its name, and the two docs are merged.
  • Removed comments: 3 section banners and 5 bare issue refs (#599, #1098, #1494 ×2, #856).
  • Tests:
    • Fixed the stale doc on npm_identity_fallback_fills_only_misses. It said the targeted lookup probes only node_modules/left-pad, which is no longer true; it now says what the asserts check.
    • Rewrote the garbled vlt_alias_is_consumed_through_its_store_copy doc ("no importer dir is an alias nothing").
    • Deleted the write_pkg wrapper and reused local() for an inline CrawlerOptions.
    • Moved npm_alias_copies_finds_only_alias_installs and npm_alias_copies_walks_every_workspace_members_tree to ecosystem_dispatch tests. They test find_manifest_package_copies_reusing, and are renamed manifest_copies_take_npm_alias_installs_only and manifest_copies_walk_every_npm_workspace_members_tree. Their new write_package_json helper is now also behind write_npm_package.

vex_sources.rs

  • Inlined recognized_dead_note into its only caller, dead_claim_notes, which is now one loop.
  • Deleted the note() forwarder. The 10 call sites now build PlanNote { code, detail } directly.
  • Made 12 NOTE_* consts private, since nothing outside the module uses them. NOTE_API_AUTH_FALLBACK stays pub(crate).
  • Moved the 3 unattested-note consts (NOTE_NPM_SHRINKWRAP_ONLY, NOTE_MAVEN_REACTOR_ROOT, NOTE_MAVEN_CLASSIFIER_UNPATCHED) up beside their siblings, above unattested_note.
  • Removed comments: 3 section banners and 3 bare refs (#745, #899, #261).

Audit notes not applied

/code-review high

There were no correctness findings and 7 cleanup or test-coverage notes. 6 are fixed:

  • restored fetch_concurrency and its test;
  • restored build_candidates;
  • inlined the re-created manifest_copies wrapper;
  • kept the "no second alias walk" reason;
  • renamed the moved tests;
  • write_npm_package now shares the package.json writer.

1 is left: a sorted helper now exists in two different test modules. A shared test-util module for one three-line helper isn't worth it.

Stats

git diff --stat: 3 files, +247 / −285.

  • Production: +104 / −141 (net −37). Most of the + lines in vex_sources.rs come from the PlanNote literals that replace note().
  • Tests: +146 / −144. Most of that is the two tests moving to ecosystem_dispatch.rs.

Verification

  • cargo clippy --workspace --all-features -- -D warnings is clean.
  • cargo test -p socket-patch-cli --lib: 940 passed.
  • The vex, e2e_vex, e2e_embedded_vex, e2e_vex_redirect and e2e_vex_vendor suites all pass (234 tests).
  • No platform-specific code changed.
  • CI: ci-ok green on 14ba988; Bugbot found no issues.

Behavior change

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

Risk

Low: local inlining and comment and visibility changes only.

https://claude.ai/code/session_011f9aeQ8265a5x29iqeryUU


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
vex_consumed.rs: drop the not_installed() wrapper and the cfg(test)
npm_identity_fallback forwarder (the reusing variant takes its name),
drop banner and bare issue-ref comments, fix the stale and garbled
test docs, and drop the write_pkg test wrapper. The two
npm_alias_copies_* tests exercise ecosystem_dispatch's every-copy
lookup, so they move there.

vex_sources.rs: inline fetch_concurrency, recognized_dead_note and
build_candidates, drop the note() forwarder for PlanNote literals,
make the module-only NOTE_* consts private and group the unattested
ones together, and drop banners and bare issue refs. The deleted
fetch_concurrency test only re-checked core's api_concurrency.

No behavior change.

Assisted-by: Claude Code:claude-opus-5-5
/code-review: fetch_concurrency's test is the only check of this
window's own FETCH_CONCURRENCY ceiling, and build_candidates names a
step plan() is better off without inlined, so both stay. The moved
ecosystem_dispatch tests are renamed after the lookup they test, call
it directly, keep the "no second alias walk" reason, and share the
package.json writer with write_npm_package.

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

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

[burn-down agent] Ready for review at 14ba988.

  • CI: 82/82 check runs on 14ba988 green (success/skipped/neutral); no main-wide failures.
  • Bugbot: reviewed 14ba988, no findings; no open review threads.
  • Mergeable, no conflicts with main; no CHANGELOG.md change.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does: A behavior-preserving cleanup of vex_consumed.rs and vex_sources.rs. It inlines the not_installed(), note() and recognized_dead_note wrappers and the test-only npm_identity_fallback forwarder, and makes 12 NOTE_* consts private. It also moves two alias-install tests into ecosystem_dispatch tests, where the function they exercise lives.

Risk: low. Every production change is a mechanical inline. Note codes, message text and ordering are unchanged. Narrowing the consts to private compiles, so nothing else used them.

Look here:

  • vex_sources.rs:894-923:`` dead_claim_notes, the only control-flow rewrite (filter_map/map became a for/continue loop).
  • vex_sources.rs:179-234:``` the visibility narrowing and the moved consts.
  • vex_consumed.rs:145-171:```` npm_identity_fallback (formerly `_reusing`), with the production call at :99.
  • ecosystem_dispatch.rs:2259-2380:````` the moved tests and their new helpers.

Verified:

  • Read the full diff against main.
  • Assertions in the moved tests are byte-identical to the originals.
  • cargo test -p socket-patch-cli --lib -- vex_consumed vex_sources ecosystem_dispatch: 71 passed.
  • cargo test -p socket-patch-cli --test e2e_vex: 45 passed.
  • clippy (lib/bins, test profile): clean.
  • CHANGELOG.md is untouched.
  • CI: ci-ok and clippy are green on the head.

Changes I made: In the PR description, corrected "12 call sites" to 10 for note().

Open questions: None. A small sorted() test helper is now duplicated in two test modules; the description acknowledges this.

Auto-merge is armed: 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