Skip to content

Fix scan/get --json dropping apply failures (#424) - #955

Open
Mikola Lysenko (mikolalysenko) wants to merge 7 commits into
mainfrom
agent/fix-nested-apply-json-failures
Open

Mikola Lysenko (mikolalysenko) wants to merge 7 commits into
mainfrom
agent/fix-nested-apply-json-failures

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #424

Summary

When scan --mode agent or get (agent mode) downloads a patch and the in-place apply then fails, --json now says what failed. Before this change the envelope said failed: 0 and applied: 0, listed the patch as added, and had no error text. Only the exit code and status: "partial_failure" hinted at the failure. The human output already printed the error.

Now the failed patch's record is {purl, uuid, action: "failed", errorCode, error}, using the same code/text pair the standalone apply --json emits (apply_failed, or package_not_installed when nothing is installed and the lockfiles don't resolve it). failed counts it, and applied counts only patches that really applied. A failure that no single patch explains (unreadable manifest, yarn PnP refusal, unavailable patch sources) is reported as top-level errorCode / error on the same object (apply in scan's envelope).

Root cause

run_nested_apply in crates/socket-patch-cli/src/commands/get.rs returned only a bool. The nested apply never prints JSON (one envelope per command), so its per-patch events were thrown away. download_and_apply_patches_with (behind both get and scan --mode agent) and the single-uuid get path then filled failed from download failures only. The code is shared by every ecosystem, which is why the report reproduced with Maven, PDM, Pipenv, npm and vlt.

Fix

  • apply::run_locked now returns an ApplyRunReport (exit code, per-patch failures, optional run-level error) instead of a bare exit code. apply itself still uses only the code, so its output is unchanged.
  • get.rs fold_apply_failures folds the report into the get/scan envelope. Records are matched by normalized purl, falling back to the base purl for qualified PyPI variants. A failing manifest patch that this run didn't select is added as its own failed record, because the nested apply covers the whole --ecosystems-scoped manifest.
  • CLI_CONTRACT.md documents the new failed records and counters.
  • No wrapper changes are needed: the npm, PyPI and gem wrappers only dispatch the binary.

Tests (red → green)

The regression tests were committed before the fix (9951d28) and failed on that commit with exactly the reported shape: "failed":0,...,"action":"added" and no error. All of them pass with the fix (94c993c and later).

Issue Test Before After
#424 scan --mode agent --json covgap_commands_scan_mod::scan_agent_json_nested_apply_failure_reaches_the_apply_block FAIL (apply.failed 0) pass
#424 get <uuid> --json covgap_commands_get::get_uuid_json_nested_apply_failure_names_the_patch FAIL pass
#424 engine (get search path / scan) covgap_commands_get::engine_nested_apply_failure_reaches_the_json_envelope FAIL pass
#424 not-installed variant covgap_commands_get::engine_nested_apply_not_installed_reaches_the_json_envelope FAIL pass
control: clean apply unchanged covgap_commands_get::engine_nested_apply_success_keeps_added_and_counts_applied pass pass
#424 uninstalled patches stay warnings beside a real failure apply::tests::collect_apply_failures_names_unresolved_purls_only_when_nothing_else_failed new pass
fold/collect units get::tests::fold_apply_failures_* (4), apply::tests::collect_apply_failures_* (2) new pass

The apply-failure tests use the first-party-link refusal from the issue thread (node_modules/<pkg> symlinked to packages/<pkg>), so they're deterministic and don't need a read-only filesystem, which root would bypass anyway. Because they use symlinks they're #[cfg(unix)].

Existing test updated: the composer and gem docker e2e verifiers (docker_e2e_composer.rs, docker_e2e_gem.rs) used to check that scan's JSON said "action": "added". In those fixtures, scan's own in-place apply fails on a hash mismatch and the later apply --force patches the file, so added was only there because of this bug. CI's coverage-docker (composer) caught the new failed/apply_failed record. The verifiers now check what "synced" actually means: the purl is recorded in .socket/manifest.json.

Local checks:

  • cargo clippy --workspace --all-features -- -D warnings: clean.
  • cargo test --workspace --all-features: not run in full locally: building every test binary ran this container out of disk. Run instead and all green: the socket-patch-cli lib tests (853), the get, scan and apply targets, in_process_get* (6 targets), covgap_commands_get (89), covgap_commands_scan_mod (52), and cli_{get,scan,apply}_silent. CI runs the full suite.
  • Formatting: CI has no rustfmt step, and cargo fmt --all -- --check already reports diffs in about 120 untouched files on main. The files this PR touches are clean under rustfmt --edition 2021 --check.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HjNH36TbmyXCpJPw3EyBZB


Note

Medium Risk
Changes JSON contract and exit/partial-failure semantics for agent-mode get/scan; apply internals now expose structured failures but standalone apply output is unchanged.

Overview
Fixes #424: when get or scan --mode agent runs download then nested apply, --json now reports apply failures instead of leaving patches as added with failed: 0.

apply::run_locked returns an ApplyRunReport (exit code, per-patch ApplyFailure list, optional run-level errorCode/error, and which purls actually applied). Standalone apply still only uses the exit code. collect_apply_failures maps apply results to apply_failed or package_not_installed, matching standalone apply --json.

get.rs folds that report via fold_apply_failures: selected patch rows become action: "failed" with metadata stripped; unselected manifest failures are appended; counters and top-level errors align with CLI_CONTRACT.md. PURL matching uses normalization and base-purl rules for qualified variants.

Tests cover engine, get <uuid> --json, and scan agent JSON; composer/gem docker e2e now treat “synced” as manifest presence when scan’s inline apply can show failed. Unrelated: digest.rs pending-inline list extended.

Reviewed by Cursor Bugbot for commit c51938b. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
scan --mode agent --json and get --json report a failed nested apply
as failed: 0 with the patch listed as added and no error text. These
tests pin the expected envelope: the patch record carries
action: failed, errorCode and error, and failed counts it.

Refs #424

Assisted-by: Claude Code:claude-opus-5-5
When scan --mode agent or get downloads a patch and the in-place apply
then fails, the --json output said failed: 0, listed the patch as
added and carried no error, so automation reading the JSON could not
tell what went wrong. Only the exit code and status hinted at it.

The nested apply now hands its failures back to the caller instead of
just a pass/fail flag. Each patch that failed to apply is reported as
action: failed with the same errorCode/error pair that apply --json
prints (apply_failed or package_not_installed), failed counts it, and
applied counts only patches that really applied. A failure that no
single patch explains (unreadable manifest, yarn PnP refusal, missing
patch sources) is reported as a top-level errorCode/error.

Fixes #424

Assisted-by: Claude Code:claude-opus-5-5
When one patch fails to apply, apply only warns about other patches
that have no installed copy. The JSON report now matches that: those
patches are reported as package_not_installed failures only when
nothing else failed the run. Adds unit tests for the failure
collection.

Refs #424

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 6, 2026 18:57
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] CI on d2e44f3: the Bun and vlt patch-compatibility runs are red, but in cells this PR doesn't touch.

  • Bun patch compatibility (native ubuntu 1.0.0 / 1.1.0 / 1.1.38, macOS 0.8.1 / 1.0.0 / 1.0.36): every failing cell is hosted mode. Three cells failed with HTTP Error 503: Service Unavailable from the patch service. The rest failed *PatchedBytes checks with no error text, which means bun install didn't land the hosted bytes. The failures land on different shapes on each runner (direct, dev, optional, alias, peer, space-unicode), which doesn't look like a code regression.
  • vlt patch compatibility: native (ubuntu-latest, 1.2.0) had one cell, 1.2.0-vendored-dev, end in safe-refusal instead of patched. lock-diff follows from that: the Linux vlt-lock.json for that same cell differs from macOS and Windows.

Why these aren't this PR's: the diff only changes the agent-mode nested apply (apply::run_locked and get.rs run_nested_apply/fold_apply_failures) and how its result is reported in --json. Hosted and vendored runs never call either. Both workflows passed on other agent branches all afternoon (latest 18:34 UTC), and the patch service returned 503s during this run.

No code fix exists or is needed in this PR. I'll re-run the failed jobs once when each run completes. A second failure would be treated as real and investigated.


Generated by Claude Code

The composer and gem docker e2e scripts checked that scan's JSON said
"action": "added". In these fixtures scan's own in-place apply fails
(the later apply --force patches the file), and scan --json now
reports that failure on the patch record (#424). So "added" was only
there because of the bug. Check instead that the patch was recorded in
.socket/manifest.json, which is what "synced" means here.

Refs #424

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.

Comment thread crates/socket-patch-cli/src/commands/get.rs
Comment thread crates/socket-patch-cli/src/commands/get.rs Outdated
Comment thread crates/socket-patch-cli/src/commands/get.rs
The --json apply failure report could blame the wrong patch and miscount
applied:
- a failure on one PyPI release variant was pinned on a selected
  sibling variant that applied fine, via a base-purl fallback;
- applied was "selected minus failed", so a selected patch that was
  never installed (only a warning next to a real failure) still counted
  as applied;
- get <uuid> zeroed applied whenever any other manifest patch failed,
  and its extra failure records had no uuid.

The nested apply now also reports which package keys it patched, and the
envelope counts applied from that. A failure only marks records it
covers: the same purl, or an unqualified key covering its variants.

Refs #424

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.

The digest guard test (#865) fails on main. Gradle support landed with
inline sha256/sha1 computations in crawlers/gradle_cache.rs,
patch/jvm_jar.rs and patch/sidecars/maven.rs, and the guard's pending
list doesn't name them. List them as pending so CI is green until they
move onto the utils::digest helpers. Open PRs #876 and #889 add only
gradle_cache.rs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HjNH36TbmyXCpJPw3EyBZB
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] test (macos-latest) on 9df5fab failed in socket-patch-core --lib: utils::digest::tests::production_digests_go_through_the_helpers. This isn't this PR's failure. It's red on main too (CI on 9c43dfc failed). The digest guard from #865 doesn't list the inline digests that Gradle support added in crawlers/gradle_cache.rs, patch/jvm_jar.rs and patch/sidecars/maven.rs. I reproduced it locally.

I added a minimal fix in c51938b: those three files are added to PENDING_INLINE_DIGESTS, and the test passes locally. This change does nothing once main lists them itself. #876 and #889 each add only gradle_cache.rs, so they'd still fail on the other two files.


Generated by Claude Code

@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 c51938b. Configure here.

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

Copy link
Copy Markdown
Collaborator Author

[agent] Ready for review at c51938b.

  • CI: every check suite on c51938b is green.
  • Bugbot: reviewed c51938b, no new issues. No open review threads.
  • Reviewer note: c51938b only ports main's digest-guard fix (adds the Gradle/Maven inline digests to the guard's pending list) so socket-patch-core --lib passes; it no-ops once main carries the same change. Approved earlier on this same head.

Generated by Claude Code

This branch has not been deployed

No deployments
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