feat!: scan↔rollback duality — full-state rollback default, --preserve-state, path targeting - #231
Open
Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
Open
feat!: scan↔rollback duality — full-state rollback default, --preserve-state, path targeting#231Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
Conversation
…lback multi-leg orchestration Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ract doc, suite green (163/163) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s, suite 168/168 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- P0: line-aware fragment removal (gem DEPENDENCIES/CHECKSUMS indent corruption) - dry-run hosted previews compose like wet runs (per-purl reverts claim the in-memory ledger; replay never refuses their edits) - remove hosted legs persist the mutated ledger before partial-failure exits - all three state stores loaded under apply.lock (pre-lock = existence probes) - vendoredFailed envelope key; vendored reserved-empty; any drift-keep makes remove a partialFailure; wiring-unknown guards apply under --preserve-state - vacuous anchor-original probe refuses; gem section-move record fails closed - FIFO/regular-file guards on replay reads+writes; error-class prints escape --silent; leftover-edit replays are prompted - hosted-only remove path (manifest-less redirect projects) + contract sync Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Detached remove ignores preserve-state
- Updated remove_detached_only to use dispatch_revert_one_opts with keep_artifact flag, handle drift-keep case, and skip ledger deletion when preserve-state is active.
- ✅ Fixed: Hosted revert lacks FIFO guards
- Replaced bare tokio::fs operations in read_rel and write_rel with open_regular_file guards to prevent indefinite blocking on FIFO/device nodes.
Or push these changes by commenting:
@cursor push 36b4989020
Preview (36b4989020)
diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs
--- a/crates/socket-patch-cli/src/commands/remove.rs
+++ b/crates/socket-patch-cli/src/commands/remove.rs
@@ -13,7 +13,7 @@
use super::get::short_uuid;
use super::rollback::{all_files_already_original, pin_before_hash_blobs, rollback_patches};
-use super::vendor::{dispatch_revert_one, dispatch_revert_one_opts};
+use super::vendor::dispatch_revert_one_opts;
use crate::args::{apply_env_toggles, GlobalArgs};
use socket_patch_core::vendor::RevertOpts;
use crate::commands::lock_cli::acquire_or_emit;
@@ -1224,7 +1224,15 @@
let mut env = Envelope::new(Command::Remove);
env.dry_run = args.common.dry_run;
for (key, entry) in &detached {
- let outcome = dispatch_revert_one(entry, &args.common.cwd, args.common.dry_run).await;
+ let outcome = dispatch_revert_one_opts(
+ entry,
+ &args.common.cwd,
+ RevertOpts {
+ dry_run: args.common.dry_run,
+ keep_artifact: args.preserve_state,
+ },
+ )
+ .await;
for w in &outcome.warnings {
if !args.common.json && !args.common.silent {
eprintln!("Warning ({}): {}", w.code, w.detail);
@@ -1252,9 +1260,31 @@
);
return 1;
}
+ if outcome.kept_artifact {
+ // Drift-keep: the lock changed under us and the backend
+ // left both the wiring and the artifact alone. Per the
+ // RevertOutcome contract the ledger entry stays.
+ if !args.common.json && !args.common.silent {
+ eprintln!(
+ "Kept vendored state for {key}: lockfile wiring drifted; \
+ ledger entry kept too"
+ );
+ }
+ env.record(
+ PatchEvent::new(PatchAction::Skipped, key.clone()).with_reason(
+ "vendor_revert_kept",
+ "lockfile wiring drifted; vendored state and ledger entry kept",
+ ),
+ );
+ continue;
+ }
if args.common.dry_run {
if !args.common.json && !args.common.silent {
- println!("Would revert vendoring for {key}");
+ if args.preserve_state {
+ println!("Would unwire vendoring for {key} (artifact preserved)");
+ } else {
+ println!("Would revert vendoring for {key}");
+ }
}
// Verified preview (the dry-run convention); still recorded
// so `summary.verified` counts the would-be removals.
@@ -1266,6 +1296,22 @@
);
continue;
}
+ if args.preserve_state {
+ // Entry kept byte-identical: its already-reverted wiring
+ // records replay as silent no-ops later (the liveness
+ // contract) and a re-vendor re-wires from the live lock.
+ if !args.common.json && !args.common.silent {
+ println!("Unwired vendoring for {key} (artifact preserved)");
+ }
+ env.record(
+ PatchEvent::new(PatchAction::Skipped, key.clone()).with_reason(
+ "vendor_state_preserved",
+ "lockfile unwired; artifact and ledger entry preserved \
+ (--preserve-state)",
+ ),
+ );
+ continue;
+ }
state.entries.remove(key);
if let Err(e) = save_state(&args.common.cwd, &state).await {
emit_error_envelope(
diff --git a/crates/socket-patch-core/src/patch/redirect/takeover.rs b/crates/socket-patch-core/src/patch/redirect/takeover.rs
--- a/crates/socket-patch-core/src/patch/redirect/takeover.rs
+++ b/crates/socket-patch-core/src/patch/redirect/takeover.rs
@@ -86,17 +86,51 @@
}
/// Read a project file, distinguishing missing (`Ok(None)`) from unreadable.
+/// Guarded via [`crate::utils::fs::open_regular_file`]: a FIFO at the lock
+/// path would block forever on bare `read_to_string`.
async fn read_rel(project_root: &Path, rel: &str) -> Result<Option<String>, String> {
- match tokio::fs::read_to_string(project_root.join(rel)).await {
- Ok(c) => Ok(Some(c)),
+ use tokio::io::AsyncReadExt;
+ let path = project_root.join(rel);
+ match crate::utils::fs::open_regular_file(&path).await {
+ Ok((mut file, _)) => {
+ let mut content = String::new();
+ file.read_to_string(&mut content)
+ .await
+ .map_err(|e| format!("read {rel}: {e}"))?;
+ Ok(Some(content))
+ }
Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None),
+ Err(e) if e.kind() == std::io::ErrorKind::InvalidInput => {
+ Err(format!("read {rel}: not a regular file (FIFO/device/directory)"))
+ }
Err(e) => Err(format!("read {rel}: {e}")),
}
}
async fn write_rel(project_root: &Path, rel: &str, content: &str) -> Result<(), String> {
- tokio::fs::write(project_root.join(rel), content)
+ use tokio::io::AsyncWriteExt;
+ let path = project_root.join(rel);
+ // Guarded write: open with O_CREAT|O_WRONLY|O_TRUNC, then check the
+ // opened handle is a regular file before writing. A FIFO at the lock
+ // path would block forever on bare `write`.
+ let mut file = tokio::fs::OpenOptions::new()
+ .write(true)
+ .create(true)
+ .truncate(true)
+ .open(&path)
.await
+ .map_err(|e| format!("write {rel}: {e}"))?;
+ let metadata = file
+ .metadata()
+ .await
+ .map_err(|e| format!("write {rel}: {e}"))?;
+ if !metadata.is_file() {
+ return Err(format!(
+ "write {rel}: not a regular file (FIFO/device/directory)"
+ ));
+ }
+ file.write_all(content.as_bytes())
+ .await
.map_err(|e| format!("write {rel}: {e}"))
}You can send follow-ups to the cloud agent here.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5ceba4a. Configure here.
| async fn remove_patch_from_manifest( | ||
| identifier: &str, | ||
| manifest_path: &Path, | ||
| // Matching entries to KEEP anyway — drift-kept vendored purls whose |
There was a problem hiding this comment.
Detached remove ignores preserve-state
High Severity
remove_detached_only still calls dispatch_revert_one and always drops the ledger entry. It never honors --preserve-state (keep_artifact) and never handles kept_artifact drift-keeps the way the main remove path does. For detached patches the ledger entry holds the only local patch record, so this flag can delete preservable state instead of keeping it for re-apply.
Reviewed by Cursor Bugbot for commit 5ceba4a. Configure here.
| revert_cargo_redirect_purl(project_root, state, purl, dry_run).await | ||
| } else if purl.starts_with("pkg:npm/") { | ||
| revert_npm_redirect_purl(project_root, state, purl).await | ||
| revert_npm_redirect_purl(project_root, state, purl, dry_run).await |
There was a problem hiding this comment.
Hosted revert lacks FIFO guards
High Severity
Per-purl hosted reverts still read and write project lockfiles via bare tokio::fs::read_to_string / tokio::fs::write. A FIFO or device at a lockfile path can block forever on open. The new whole-ledger replay path already uses open_regular_file, but rollback/remove’s cargo and npm-family unwind still goes through these unguarded helpers.
Additional Locations (1)
Triggered by learned rule: Workspace/user-writable file reads must use open_regular_file guard, not bare fs::read
Reviewed by Cursor Bugbot for commit 5ceba4a. Configure here.
| refused_groups.insert(group); | ||
| continue 'group; | ||
| } | ||
| } |
There was a problem hiding this comment.
Replay writes are non-atomic
Medium Severity
The new whole-ledger replay flushes staged lockfile content with bare tokio::fs::write instead of atomic_write_bytes / atomic_write_bytes_preserving_mode. A crash mid-write can leave a torn lockfile. The flush loop already documents residual partial-landing risk across files in a group.
Reviewed by Cursor Bugbot for commit 5ceba4a. Configure here.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
… 6.0.3 patch The hosted-e2e pin drifted independently of this branch: production published a fifth free-tier activestorage@6.0.3 patch on 2026-08-24 (GHSA-xr9x-r78c-5hrm / CVE-2026-66066, the Active Storage libvips variant-processing advisory) and the server-ranked selection now wires it. Live-verified via the public proxy /patch/view before pinning. Suite green locally (16 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Path::is_absolute() is false on Windows for /global/store (rooted, no drive letter), so PathScope::parse classified it relative and it could never match an out-of-cwd path. A rooted pattern can never be cwd-relative — classify on has_root() (identical to is_absolute on Unix). Fixes path_scope::absolute_pattern_matches_paths_outside_cwd on the windows-latest runner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reorganizes the CLI around a duality:
scan↔rollbackare the batch primaries (toward fully patched / toward fully unpatched),get↔removethe single-patch duals. Rollback becomes a true full-state inverse with cleanup by default, both commands gain a preserve opt-out, and scan/rollback gain glob path targeting.What changes
rollback [TARGET]...— scan's batch dual (no--mode).socket/manifest.json(in-place),.socket/vendor/state.json(vendored),.socket/vendor/redirect-state.json(hosted). Runs manifest-less on hosted-only / detached-vendored projects..socket/vendorartifacts + ledger entries → unwind hosted redirects → remove the rolled-back entries from the manifest → GC unused blobs + diff/package archives. One confirmation prompt on wet runs (auto-accepted under--yes/--json/non-TTY;Rollback cancelled.exit 0 on decline).pkg:→ PURL, UUID-shaped → UUID, and only path-shaped tokens (separator, glob metachar,./, absolute) become globs —rollback lodashor a truncated UUID stays a safe exit-1 error, never a destructive scope.--preserve-state(rollback + remove,SOCKET_PRESERVE_STATE)Restore the system but keep local patch state for later re-apply: manifest entries kept, vendored artifacts + ledger entries kept byte-identical (only the lockfile wiring is reverted; a later
vendorrun re-wires — verified by test), all GC skipped. Hosted redirects have no preservable state (records dropped either way, surfaced viahosted_state_not_preservable). Conflicts with remove's--skip-rollback(exit 2 — the no-op quadrant).scan [PATHS]...+ rollback path targetsShared glob matcher (
path_scope.rs,glob = 0.3.4):require_literal_separator, ancestor matching (scan packages/appscopes the subtree), absolute patterns for out-of-tree stores, Windows case-insensitive. Purl-level scoping. The prune universe is never narrowed —scan PATHS --prunecannot delete out-of-scope manifest entries (pinned by test). Supplements (lockfile-only / ledger) are excluded from scoped scans with a counted warning. PATHS is rejected with--mode hosted|vendored(exit 2).Hosted unwind coverage
dry_runthat claims the in-memory ledger, so composed previews behave exactly like wet runs.core/patch/redirect/replay.rs: whole-ledger reverse replay with a per-kind inverse table and per-ecosystem all-or-nothing group staging — covers gem, golang, pypi, composer, bun, and the pnpmtrustLockfilerideshare edit. Runs when the scope covers every record.replacefolded into a block and refreshed,bun.lockbmigration (unrestorable by design — warning names git history).removeadditionsHosted leg (including manifest-less hosted-only projects — the unwind is the removal there), diff/package-archive GC parity, and the drift-keep fix: a
kept_artifactrevert now keeps the ledger entry and the manifest record, and any drift-keep makes the run apartialFailure(exit 1) instead of silently orphaning state.Safety properties (review-driven)
apply.lock(pre-lock work is existence probes only); mutated redirect ledgers are persisted even on partial-failure exits, so flushed lockfile writes are never stranded against a stale ledger.Gemfile.lockindentation — caught in review as a P0), anchor-shaped originals never read as already-reverted, and FIFO/regular-file guards cover the replay's reads and writes.Validation
cargo test -p socket-patch-cli --no-fail-fast,cargo test -p socket-patch-core --lib).rollback_duality_invariants,in_process_rollback_vendored,in_process_rollback_hosted(realscan --mode hostedround-trips, byte-identical lock restores),scan_paths_e2e(query-narrowing oracle + the prune fail-safe pin),remove_duality_invariants; plus updated parse pins and lifecycle suites. The real-API#[ignore]suites' mid-cycle rollbacks now use--preserve-state(they re-apply from the manifest).Reviewer notes
CLI_CONTRACT.mdandCHANGELOG.mdare updated in lockstep; the contract's new v5.0 section is the authoritative spec.docker-e2e/setup-e2efeature-gated suites (CI) and the real-API#[ignore]suites (manual).sweep_orphan_vendor_dirsinside rollback, depscan TS parity for the replay module.🤖 Generated with Claude Code
Note
High Risk
MAJOR breaking default rollback behavior touches manifest, vendor/redirect ledgers, lockfile rewiring, and GC across ecosystems; incorrect unwind or manifest cleanup could leave projects in inconsistent patched/unpatched states.
Overview
MAJOR (v5.0):
rollbackbecomes the batch inverse ofscan— by default it restores unpatched state across agent (in-place files), vendored (unwire + delete artifacts/ledger), and hosted (redirect unwind), then drops manifest entries and GCs blobs, diff, and package archives. State is inferred from the three stores; manifest-less runs work when a vendor or redirect ledger holds work.rollback --json'svendored: []narrows to vendor-owned purls the run did not act on; acted-on entries move to new envelope keys (vendoredReverted/Preserved/Kept,hosted,manifest,gc, populatedwarnings[],paths).Adds
scan [PATHS]...and variadicrollback [TARGET]...path globs (shared matcher via newglobdep): purl-level scoping, ancestor matching, prune universe never narrowed on scoped scan, supplements excluded with a warning; path-shaped tokens only become globs on rollback so mistyped identifiers stay safe exit-1 errors.Adds
--preserve-state(SOCKET_PRESERVE_STATE) onrollbackandremove: unpatch the tree/lockfiles but keep manifest, vendored artifacts + ledger (byte-identical), skip GC; hosted records still unwind (hosted_state_not_preservable).remove --preserve-state+--skip-rollbackis exit 2.Hosted unwind: per-purl reverts (cargo + npm family) plus whole-ledger reverse replay in core
patch/redirect/replay.rswhen scope covers all records; maven/nuget and several carve-outs fail closed with remedies.remove: hosted leg (manifest-less hosted-only path), blob + archive GC parity, and drift-keep fix —kept_artifactnow keeps the manifest entry too (vendor_revert_kept, partialFailure exit 1).Reviewed by Cursor Bugbot for commit 5ceba4a. Configure here.