Skip to content

Fix store-copy fold dropping copy writes (#756, #772) - #774

Merged
Mikola Lysenko (mikolalysenko) merged 6 commits into
mainfrom
agent/fix-store-copy-fold-records
Oct 5, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 6 commits into
mainfrom
agent/fix-store-copy-fold-records

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #756
Fixes #772

Summary

When a package has more than one pnpm or vlt peer-variant store copy, agent-mode apply and rollback already patch or restore every copy, but they only reported what happened to the copy the package resolved to. A run that fixed only a twin copy said already_patched / applied: 0. A rollback that restored only a twin said already_original / rolledBack: 0. Both now report what they actually did.

Root cause

apply_package_patch and rollback_package_patch each had a private copy of the store-copy fan-out and of fold_copy_result. Both folds kept only success/error state and dropped the copy's per-file records (files_patched / files_rolled_back, files_verified, applied_via). The CLI classifies events and tallies from those records, so a write that landed only in a twin was invisible. The two folds had also drifted: apply carried only the ownership advisory from a copy, rollback carried any advisory.

Change

  • New patch/store_copies.rs: one fan_out (primary, then every find_store_peer_variant_copies copy for pkg:npm/ purls) and one fold over a small CopyFold trait implemented by ApplyResult and RollbackResult.
  • The fold merges each copy's verify records and changed files into the result under the copy's on-disk path (<copy>/index.js), with applied_via carried for apply. A failed copy still fails the result, with the same store copy … failed to <verb> note.
  • A successful apply copy's --force skips (NotFound records) are not carried, for the same reason its all-skipped note is not: they describe that copy alone (Bugbot finding, fixed in 9114533).
  • One advisory rule for both directions: only the ownership advisory is carried from a successful copy. In practice rollback's success advisories were already only ownership notes, so behavior is unchanged there.
  • Both private fold_copy_result functions and duplicated loops are deleted (grep -rn fold_copy_result crates/ is empty).
  • CLI_CONTRACT documents the copy-qualified files[].path (and filesRolledBack / filesVerified).

No wrapper changes are needed: npm/, pypi/ and gem/ only dispatch to the binary.

Per-issue tests

Issue Test Without fix With fix
#756 (apply) patch::store_copies::regression_tests::apply_reports_a_write_to_an_unpatched_twin_copy FAIL (files_patched is []) pass
#756 (apply, dry run) …::apply_dry_run_carries_an_unpatched_twin_verify_record FAIL pass
#756 (rollback, from the follow-up comment) …::rollback_reports_a_restore_of_a_patched_twin_copy FAIL (files_rolled_back is []) pass
#756 (end-to-end through the binary, pnpm and vlt layouts) apply test binary: in_process_npm_multicopy::apply_and_rollback_report_a_write_to_only_a_store_twin FAIL: "applied":0,"skipped":1, event skipped / already_patched (the exact symptom in the issue) pass: applied: 1, event applied; rollback rolledBack: 1, alreadyOriginal: 0
#772 (one fold, one advisory rule, both directions) …::apply_over_patched_twins_stays_already_patched, …::rollback_over_original_twins_stays_already_original, …::apply_force_skip_in_a_twin_keeps_an_already_patched_primary, patch::apply::tests::store_copy_fold_carries_ownership_advisories_and_failures, patch::rollback::tests::store_copy_fold_carries_advisories_and_failures, test_rollback_package_patch_new_file_deleted_in_every_pnpm_peer_variant_copy (now also asserts that the heal is reported) — pass

Evidence

  • CI on 9114533: 356/356 check runs complete, 350 success and 6 skipped, none failed. Bugbot's review of 9114533 found no new issues. Its one finding on fd90c89 was fixed and the thread resolved.
  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • rustfmt --check on every touched file: clean. Running cargo fmt --all on main already reformats ~130 unrelated files, so I formatted only the touched code.
  • Locally, cargo test -p socket-patch-core --all-features gives 4847 passed and 4 failed. All 4 are permission-based tests that cannot fail a write when running as root, which the sandbox does. They are in untouched files and are green in CI.
  • cargo test -p socket-patch-cli --all-features --lib --bins --test apply --test rollback --test cli --test scan --test get: all green locally.

🤖 Generated with Claude Code


Note

Medium Risk
Touches core npm apply/rollback result merging and CLI-reported outcomes; wrong fold rules could mis-tally or mis-label events while still writing files.

Overview
Fixes #756 / #772: when only a non-primary pnpm or vlt peer-variant store copy was patched or rolled back, the CLI could still report already_patched / applied: 0 or already_original / rolledBack: 0 even though disk work happened on the twin.

Apply and rollback still fan out to every store copy, but folding no longer drops each copy's files_verified, files_patched / files_rolled_back, and applied_via. Those entries are merged with copy-qualified on-disk paths so event classification and summaries match reality. Shared logic lives in new patch/store_copies (fan_out, fold, CopyFold for ApplyResult and RollbackResult), replacing duplicated private folds with one advisory rule (ownership notes only; apply --force NotFound skips stay local to the copy).

CLI_CONTRACT now states that files[].path (and rollback verify lists) may use full twin paths, not just manifest keys. Regression coverage spans core unit tests, an in-process CLI multicopy test for pnpm and vlt, and small resolver test adjustments after #605.

Reviewed by Cursor Bugbot for commit 4727a77. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
When a package has more than one pnpm or vlt peer-variant store copy,
apply and rollback already patch (or restore) every copy, but they only
reported what happened to the first one. A run that fixed only a twin
copy said "already patched" (applied: 0), and a rollback that restored
only a twin said "already original" (rolledBack: 0).

Apply and rollback now share one store-copy fan-out and one fold, which
merges each copy's per-file records into the result under the copy's
on-disk path. The two private folds, which had drifted on which
advisories they kept, are gone; both directions now carry only the
ownership advisory from a copy.

Fixes #756, #772.

Assisted-by: Claude Code:claude-opus-5-5
End-to-end regression for #756 through the real binary, on hand-built
pnpm and vlt store layouts: apply that patches only a twin copy reports
it as applied, and rollback that restores only a twin counts it as
rolled back. Documents the copy-qualified file paths in CLI_CONTRACT.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 4, 2026 10:58
@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.

Stale Bugbot comment from a previous run.

Comment thread crates/socket-patch-core/src/patch/store_copies.rs
Under --force, a store twin missing a patched file skips it and still
succeeds. The fold already dropped that copy's "all files skipped" note,
but it carried the skipped file's NotFound record, so a package whose
primary copy was already patched was reported as "applied" with no
files instead of "already patched". Those records now stay with the
copy, like its note.

Assisted-by: Claude Code:claude-opus-5-5
@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.

Stale Bugbot comment from a previous run.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 4, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review. Head is 9114533.

  • CI: all 356 check runs finished on 9114533: 350 passed, 6 skipped, none failed. The branch is up to date with main and has no conflicts.
  • Bugbot: reviewed 9114533 and found no new issues. Its one earlier finding on fd90c89 (copy-level --force skips breaking the already_patched status) was fixed in 9114533, and that thread is resolved.
  • Where to look: the new patch/store_copies.rs replaces the two private fold_copy_result copies in apply.rs and rollback.rs. files[].path values in CLI JSON output are now prefixed with the store-copy path, as documented in CLI_CONTRACT.md.

Generated by Claude Code

Conflict in tests/apply/in_process_npm_multicopy.rs: both sides appended
new tests at the end of the file (this branch's pnpm twin-reporting test,
main's #601 bundled-copy and #626 first-party-link tests). Kept both.

Co-Authored-By: Claude <noreply@anthropic.com>
#605 taught the name-keyed npm resolver to probe bundled store
trees, so it now finds aliased copies (node_modules/lp) and a nested
host's store peers itself. Two vex_consumed tests from #738 assumed
that set never held aliases, so main's CI went red after both merged.

The tests now feed the alias-free set explicitly to keep covering
alias expansion, and also check the resolver's own set reaches the
same copies with no duplicates. No production code changes.

Assisted-by: Claude Code:claude-opus-5-5
(cherry picked from commit 40dac07)
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


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 4727a77. Configure here.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] CI note for whoever is driving this PR now (the burn-down agent's heartbeat at 14:22 is fresh, so I'm not pushing). PDM patch compatibility / native (ubuntu-latest, 2.10.4) failed on 4727a77. Only one of its 36 cells failed: 2.10.4 extras hosted FAIL rescanIdempotent. The other 35 are PASS, REF or UNS, and the other PDM legs that have finished are green.

Why I don't think this PR causes it: this PR only changes the npm pnpm/vlt store-copy fold used by agent-mode apply and rollback, and PDM hosted mode never runs that code. The same workflow passed on this PR's previous head 9114533, before main was merged in. Its last main run (792e836, 11:25 UTC) also passed, but main has gained ~10 commits since then with no PDM run. Those include PyPI and hosted-restore changes (#818, #766, #822, #841). So this is either a flake or a regression already on main that this merge picked up.

Next steps: re-run that one job once. If it fails again, compare it against a PDM run on current main (99f61d2). The case artifact is pdm-results-ubuntu-latest-2.10.4 (run 37319921529). I can't re-run jobs from this session because the gh token here is invalid.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] Second CI failure on 4727a77: e2e (ubuntu-latest, e2e_vendor_jvm_build, 9.8.0, 17, --ignored gradle_multi_project). This one is infrastructure and happened before any test ran. Downloading the Gradle 9.8.0 distribution from services.gradle.org failed with curl: (56) Recv failure: Connection reset by peer (exit 56) during job setup. Re-running that job once should clear it. I can't re-run it from this session because the gh token is invalid.


Generated by Claude Code

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 5, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review.

  • Head: 4727a773c1e59c070eda25ec0376edfb7b8ac7f6
  • CI: 458/458 check runs green (success/skipped) on this head; mergeable clean
  • Bugbot: reviewed 4727a77, no new issues; no open threads
  • Reviewer focus: apply/rollback outcome reporting across pnpm/vlt peer-variant store copies; two flaky jobs (Gradle install, PDM native) passed on one re-run

Generated by Claude Code

@mikolalysenko
Mikola Lysenko (mikolalysenko) merged commit 75ba9e1 into main Oct 5, 2026
701 of 703 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-store-copy-fold-records branch October 5, 2026 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

3 participants