Skip to content

Fix apply failing when patched deps are skipped (#403) - #555

Merged
Mikola Lysenko (mikolalysenko) merged 2 commits into
mainfrom
agent/fix-apply-lockfile-only-skip
Oct 2, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 2 commits into
mainfrom
agent/fix-apply-lockfile-only-skip

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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 (chokidar@3.6.0, so fsevents@2.3.3 is in the lock but not installed on Linux, with a manifest holding only pkg:npm/fsevents@2.3.3). 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

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
@mikolalysenko
Mikola Lysenko (mikolalysenko) force-pushed the agent/fix-apply-lockfile-only-skip branch from 325d73c to 2b8466b Compare October 2, 2026 12:35
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 2, 2026 12:52
@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 2b8466b. Configure here.

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

Copy link
Copy Markdown
Collaborator Author

[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

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

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.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

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.

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