Repository navigation
refactor(cli): sweep vex_consumed.rs and vex_sources.rs - #1948
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
Conversation
Assisted-by: Claude Code:claude-opus-5-5
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
|
BugBot review Generated by Claude Code |
There was a problem hiding this comment.
✅ 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.
Assisted-by: Claude Code:claude-opus-5-5
|
[burn-down agent] Ready for review at
Generated by Claude Code |
Final review briefWhat it does: A behavior-preserving cleanup of 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:
Verified:
Changes I made: In the PR description, corrected "12 call sites" to 10 for Open questions: None. A small Auto-merge is armed: approving sends this straight to the merge queue. Generated by Claude Code |
LLM Description written by Claude Code:claude-opus-5-5
Architecture sweep of
crates/socket-patch-cli/src/commands/vex_consumed.rsandvex_sources.rs. These are the next free rows incoverage/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.rsnot_installed()wrapper (9 call sites now useHostedCopies::default()).cfg(test)forwardernpm_identity_fallback. The_reusingvariant takes its name, and the two docs are merged.#599,#1098,#1494×2,#856).npm_identity_fallback_fills_only_misses. It said the targeted lookup probes onlynode_modules/left-pad, which is no longer true; it now says what the asserts check.vlt_alias_is_consumed_through_its_store_copydoc ("no importer dir is an alias nothing").write_pkgwrapper and reusedlocal()for an inlineCrawlerOptions.npm_alias_copies_finds_only_alias_installsandnpm_alias_copies_walks_every_workspace_members_treetoecosystem_dispatchtests. They testfind_manifest_package_copies_reusing, and are renamedmanifest_copies_take_npm_alias_installs_onlyandmanifest_copies_walk_every_npm_workspace_members_tree. Their newwrite_package_jsonhelper is now also behindwrite_npm_package.vex_sources.rsrecognized_dead_noteinto its only caller,dead_claim_notes, which is now one loop.note()forwarder. The 10 call sites now buildPlanNote { code, detail }directly.NOTE_*consts private, since nothing outside the module uses them.NOTE_API_AUTH_FALLBACKstayspub(crate).NOTE_NPM_SHRINKWRAP_ONLY,NOTE_MAVEN_REACTOR_ROOT,NOTE_MAVEN_CLASSIFIER_UNPATCHED) up beside their siblings, aboveunattested_note.#745,#899,#261).Audit notes not applied
fetch_concurrencyandbuild_candidates:/code-review highobjected to both.fetch_concurrency's test is the only check of this window's ownFETCH_CONCURRENCYceiling.build_candidatesnames a step, and inlining it would add 20 lines toplan(), which is already about 350.failed()wrapper: it is kept. It does&str→Stringfor 19 call sites, and each 4-line literal would replace a one-liner.api_auth_fallback_warningtojson_envelope; mergePlanNoteintoRunWarning: both change callers invendor.rsandapply.rs(Fix Go patch delivery, audits, and reinstalls #1698),repair.rs(Sweep remove.rs and repair.rs #1875) andvex.rs(Fix Go patch delivery, audits, and reinstalls #1698/Fix Go stale go.sum line after a downgrade (#1862) #1893), so they wait.npm_crawler.rs: that file is held by Fix hidden npm workspace members left unpatched (#1827) #1829, Build: share one test env guard in core (#1889) #1891, Fix crawlers missing parent dirs under --cwd . (#1902) #1906 and Fix .yarnrc read by two drifted tokenizers (#1901) #1915.cfg(test)import swap that records store-variant expansion: kept. It is the only way the hosted tests can assert that already-expanded copies are not rescanned./code-review high
There were no correctness findings and 7 cleanup or test-coverage notes. 6 are fixed:
fetch_concurrencyand its test;build_candidates;manifest_copieswrapper;write_npm_packagenow shares the package.json writer.1 is left: a
sortedhelper 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.vex_sources.rscome from thePlanNoteliterals that replacenote().ecosystem_dispatch.rs.Verification
cargo clippy --workspace --all-features -- -D warningsis clean.cargo test -p socket-patch-cli --lib: 940 passed.vex,e2e_vex,e2e_embedded_vex,e2e_vex_redirectande2e_vex_vendorsuites all pass (234 tests).ci-okgreen 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