GeezerVison is showing github.com — Fix apply failing when patched deps are skipped (#403) by mikolalysenko · Pull Request #555 · SocketDev/socket-patch
Conversation
LLM Description written by Claude Code:claude-opus-5-5
Fixes #403
Summary
socket-patch apply no longer exits 1 when the only manifest patches are for packages that the project's lockfile resolves but the package manager deliberately left uninstalled on this host. Examples are a platform-gated optional dependency (fsevents on Linux, @esbuild/<os>-<cpu>) and a devDependency after npm ci --omit=dev. Before this change, CI on "the other" platform and production deploys running npm ci --omit=dev && socket-patch apply failed even though the tree was already correct.
Root cause
Agent-mode apply (crates/socket-patch-cli/src/commands/apply.rs) failed the run when no targeted manifest patch matched an installed package. That happened both on the empty-crawl path (success: unmatched.is_empty()) and on the none_matched path. Neither path looked at the project's lockfiles. If any other patch matched, the same entry was only a warning (exit 0). scan --apply already treats lockfile-only packages as a calm skipped/package_not_installed, as CLI_CONTRACT.md says.
Fix
- New
lockfile_resolved() reads the project lock inventory (ProjectContext::locks, the same inventory behind scan's lockfile supplement) and marks the unmatched purls the lockfiles resolve. It uses the normalize_purl/qualifier-stripped comparison and Composer's purl_eq, mirroring lockfile_only_contains. Global runs have no project lock, so nothing is lockfile-resolved there.
- Lockfile-resolved purls never fail the run:
- JSON keeps the
skipped/package_not_installed event, with the detail "Resolved by the project lockfile but not installed on this host (lockfile-only)".
- The human path prints a
Note: block instead of the Error: block. --silent/--json mute it.
- An unmatched purl with no lock evidence still fails an all-miss run, and the error lists only those purls. The wrong-
--cwd guard is kept: no lock means behavior is unchanged.
- CLI_CONTRACT.md (
package_not_installed row, rollback asymmetry note) and the doc comment on the pinned unmatched_purl_exit_semantics_are_pinned invariant are updated. That invariant's ghost purl has no lock, so both of its halves are unchanged.
- I added no new async frame to
apply_patches_inner's poll state: the lock read is Box::pinned, per the Windows 1 MiB stack note.
- Wrappers (
npm/, pypi/, gem/) only dispatch to the binary, so they need no change.
Test evidence
New suite crates/socket-patch-cli/tests/apply/lockfile_only_skip.rs (cargo test -p socket-patch-cli --all-features --test apply lockfile_only_skip):
- Without the fix (main's
apply.rs): 4 failed, 1 passed. The no_lockfile_all_miss_still_fails control passes on both.
- With the fix: 5 passed.
Per-issue checklist (#403):
- Platform-skipped optional dependency (
fsevents, os: ["darwin"]): exit 0, success, calm skip event, human note and no Error:. Covered by platform_skipped_optional_dependency_is_a_calm_skip.
- Hook-style
apply --silent: exit 0 and silent. Covered by silent_apply_on_platform_skipped_optional_dependency_exits_zero.
-
npm ci --omit=dev devDependency (dev: true): exit 0. Covered by omitted_dev_dependency_is_a_calm_skip.
- A purl the lock doesn't resolve still fails, and the error lists only it. Covered by
unresolved_purl_still_fails_alongside_a_lock_resolved_one.
- No lockfile at all (wrong cwd): still exit 1. Covered by
no_lockfile_all_miss_still_fails.
- Real npm 10.9.4 / Node 22 repro from the issue (
[email protected], so [email protected] is in the lock but not installed on Linux, with a manifest holding only pkg:npm/[email protected]). apply --offline exits 0 with the note, and apply --offline --silent exits 0.
Local checks:
cargo clippy --workspace --all-features -- -D warnings: clean.
cargo fmt --all -- --check: main itself isn't rustfmt-clean (a cargo fmt --all rewrites 129 files on 61cfb9b), and CI doesn't run fmt. I checked that this PR's hunks and the new file are rustfmt-clean, and the only rustfmt --check diff left in apply.rs is a pre-existing one that main also has.
cargo test --workspace --all-features --no-fail-fast: 9473 passed, 12 failed, 246 ignored. All 12 failures are permission-based fixtures (chmod read-only dirs or unremovable files, e.g. covgap_commands_vendor::*_state_write_failure_*, vendor::pypi_poetry::tests::wire_write_failure_*, patch::copy_tree::tests::relax_loop_must_not_traverse_symlinked_root). They can't fail as root (the sandbox runs as uid 0), and none of them touch apply. CI runs as non-root.
node --test npm/socket-patch/bin/socket-patch.test.mjs: 4/4.
CI on 2b8466b: all 407 check runs finished, 401 success and 6 skipped. Bugbot reviewed 2b8466b and found no issues.
Note Medium Risk
Changes documented exit-code and failure semantics for apply, which CI and postinstall hooks may gate on, though behavior is narrowed to lockfile-evidenced misses only.
Overview
socket-patch apply no longer exits 1 when manifest patches only target packages the lockfile resolves but that are intentionally absent on disk (e.g. platform optional fsevents, devDependencies after npm ci --omit=dev). That aligns agent apply with scan --apply, which already treated those as benign skips.
The command now cross-checks unmatched manifest purls against ProjectContext lock inventory and splits them into lockfile-resolved vs truly unresolved. Only unresolved purls can trigger partialFailure, stderr Error: lines, or the all-miss failure path; lockfile-only entries still emit skipped / package_not_installed with an explicit lockfile-only detail and an optional human Note: (muted under --json / --silent). CLI_CONTRACT.md and integration tests in lockfile_only_skip.rs pin the behavior; wrong---cwd / no-lock cases stay exit 1.
Reviewed by Cursor Bugbot for commit 2b8466b. Configure here.
Generated by Claude Code
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Assisted-by: Claude Code:claude-opus-5-5
`socket-patch apply` exited 1 when every manifest patch targeted a
package the project's lockfile resolves but the package manager
deliberately left uninstalled on this host: a platform-gated optional
dependency like fsevents or @esbuild/<os>-<cpu>, or a devDependency
after `npm ci --omit=dev`. CI on other platforms and production
deploys failed although the tree was already correct.
Such purls are now a calm package_not_installed skip with a
lockfile-only detail and a human note, matching scan --apply. An
unmatched purl that no lock resolves still fails the all-miss run, so
the wrong --cwd guard is kept.
Fixes #403
Assisted-by: Claude Code:claude-opus-5-5
cursor[bot] reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 2b8466b. Configure here.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
[burn-down agent] Labeled Ready for review at 2b8466b0f71994b66cbcfbd26e217eb273238944.
- CI: 401/401 non-skipped check runs green on head (6 skipped); 0 commits behind
main, mergeable (blocked only on required review).
- Bugbot: reviewed
2b8466b — no issues found; no unresolved review threads.
- Reviewer focus: the new path where
apply treats unmatched purls as "deliberately not installed" when the project lock resolves them — check it can't mask a genuinely missing install.
Generated by Claude Code
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Reviewed 2b8466b0f71994b66cbcfbd26e217eb273238944. Recommendation: ready to merge as-is from code review.
No actionable correctness/security regressions found. Checked lockfile matching, scoped/qualified PURLs, global-scope exclusion, mixed unresolved/lock-resolved misses, and preservation of actual patch/variant failures.
Validation: cargo test -p socket-patch-cli --all-features --test apply — 93 passed, 1 pre-existing ignored test. The five new skip/control tests pass on this head; against merge-base apply.rs, four fail and the no-lock control passes. Full workspace/platform matrix not rerun.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Second review pass of 2b8466b0f71994b66cbcfbd26e217eb273238944: ready from code review. Lockfile-only misses still change only unmatched-package handling. The current-main PyPI multi-copy apply/fallback changes are in distinct paths; actual installed-copy patch failures remain errors. Global scope and no-lock all-miss runs retain their failure behavior.
The head is unchanged from the prior review, discussions introduce no unresolved finding, and it merges cleanly with current main 73b17db5. Prior exact-head apply suite: 93 passed, 1 pre-existing ignored test. Five new skip/control tests passed; with the earlier base implementation four failed and the no-lock control passed. No duplicate broad test run or code changes were needed for this pass.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Oct 2, 2026
Mikola Lysenko (mikolalysenko)
added a commit
that referenced
this pull request
Oct 9, 2026
Since #555, apply treated a manifest crate that Cargo.lock resolves
but that is not unpacked in $CARGO_HOME/registry/src as a calm
lockfile-only skip: exit 0, status success. On a fresh CI runner or
a pruned cache that crate was simply not fetched yet, so
'socket-patch apply && cargo build --locked' went green and shipped
the unpatched crate (#616).
Cargo crates are no longer lockfile-only: cargo fetch unpacks every
locked crate, target-gated ones included, so a missing one is a real
miss. An all-miss run exits 1 again, as in 4.x, and the JSON detail,
the human error and the warning tell the user to run cargo fetch
first. npm's platform-gated and --omit=dev skips are unchanged.
Fixes #616
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
Ready for review
Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review
Milestone
No milestone
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.
|