Skip to content

Report artifact GC in one JSON shape and count every rollback leg - #1273

Merged
Mikola Lysenko (mikolalysenko) merged 14 commits into
mainfrom
fix/gc-report-json-1257
Oct 10, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 14 commits into
mainfrom
fix/gc-report-json-1257

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

Closes #1257. Closes #1066.

Both are JSON-contract drift that should be fixed before v5.0.0 ships (#1194).

Artifact GC: one shape (#1257)

Command Before After
repair --json removed event, details: {count, checked}; no bytes; bumped summary.removed by 1 top-level gc + summary.bytesFreed; carrier event keeps count/checked, gains bytes, bumps no counter
remove --json carrier details: {blobsRemoved, rolledBack, archivesRemoved}; no bytes same carrier + bytes; top-level gc + summary.bytesFreed
rollback --json hand-built gc json same keys, built from GcReport
scan --prune --json hand-built gc json same keys, built from GcReport (plus scan's manifest/vendored keys)
  • New json_envelope::GcReport {removedBlobs, removedDiffArchives, removedPackageArchives, bytesFreed}, folded from the three sweep passes.
  • summary.bytesFreed is always present (0 when no GC ran). events[].bytes is set on the GC carrier and on --update's downloaded event. summary.bytesDownloaded was never emitted, so it is removed from the contract.
  • remove's human output now prints the diff/package archives it sweeps, like repair does.
  • scan's --dry-run GC preview uses the same keys too (MAJOR): prunableManifestEntries/orphanBlobs/orphanDiffArchives/orphanPackageArchives/revertableVendoredEntries/vendorOrphanDirs/bytesReclaimable became prunedManifestEntries/removedBlobs/removedDiffArchives/removedPackageArchives/revertedVendoredEntries/removedVendorOrphanDirs/bytesFreed. The preview leaves out only the keys a real pass alone can fill (keptVendoredEntries, failedVendoredEntries, skipped, warnings).

Rollback counters span every leg (#1066)

  • rolledBack = agent restores + vendoredReverted + vendoredPreserved + hosted.reverted.
  • failed = agent failures + vendoredKept + vendoredFailed + hosted.failed + hosted.unsupported.
  • If something failed and nothing was rolled back, already original or not installed, the run now reports status: "error" with error.code: "rollback_failed" instead of partial_failure. Exit codes are unchanged (1).

Contract

  • Envelope schema: adds update to command, plus rebuilt, bytesFreed and gc; drops bytesDownloaded.
  • PatchEvent action enum now includes rebuilt.
  • Fixed the "Emitted by" column and the per-command action matrix. Examples: apply never emits updated, discovered is list-only, vex was missing.
  • The GC jq recipe now reads .gc.* and .summary.bytesFreed.
  • New unit tests pin the documented summary, gc, top-level and PatchEvent key sets, and the action vocabulary rows, against what json_envelope serializes.

Tests

  • --lib (910), plus the rollback, repair, remove, scan, in_process_scan, covgap rollback/scan-hosted and in-process rollback hosted/vendored suites, all pass locally.
  • cargo clippy --locked --workspace --all-features -- -D warnings is clean.
  • Updated tests that pinned the old summary.removed: 1 and the old partial_failure on all-failed rollbacks. The mixed hosted case now asserts rolledBack: 1, failed: 1 (was 0/0).
  • e2e_bun_lockb (needs the real toolchain) is updated to the new status, but CI is the first to run it.

v5 blocker agent update (bc9c9be)

  • e2e_bun_lockb was red on 9714d570: native_binary_hosted_vendored_takeover_roundtrip expected status: "error", but a staged manifest makes the agent leg report the copy alreadyOriginal: 1, so the run is partial_failure, as the contract says. The per-caller fix from 113c0177 was lost in the branch rewrite.
  • rollback_refuses_binary_hosted_pin_then_checkout now takes a copy_already_original flag from each caller: takeover and shared-bundled-record legs (manifest staged) expect partial_failure + alreadyOriginal: 1; hosted-only alias/transitive shapes expect error / rollback_failed + alreadyOriginal: 0. All expect failed: 1.
  • Merged origin/main (merge, not rebase) at 1e69b30.
  • Local (bun 1.4.2): takeover roundtrip red -> green; 21/23 e2e_bun_lockb tests pass. The other 2 fail only because this sandbox can't reach patches-api.socket.dev / GitHub tarballs (proxy 403), not because of the change.
  • cargo fmt --all -- --check clean.

🤖 Generated with Claude Code


Note

Medium Risk
MAJOR JSON shape changes (scan dry-run GC keys, rollback total-failure status) affect automation consumers; rollback counter semantics changed across vendored/hosted legs.

Overview
v5 JSON contract: artifact GC is reported through one shared gc object (removedBlobs, removedDiffArchives, removedPackageArchives, bytesFreed) on envelope commands (repair, remove) and aligned with rollback / scan --prune. summary.bytesFreed mirrors gc.bytesFreed; GC carrier events carry bytes but no longer inflate summary.removed. summary.bytesDownloaded is dropped from the contract.

Breaking (scan dry-run): GC preview keys are unified with the wet pass (prunedManifestEntries, removedBlobs, …) instead of the old prunable* / orphan* / bytesReclaimable names.

Rollback (#1066): top-level rolledBack / failed count agent, vendored, and hosted legs. When every targeted package fails and nothing is restored or already original, JSON is status: "error" with error.code: "rollback_failed" (still exit 1), not partial_failure.

Docs & tests: CLI_CONTRACT.md updated (envelope, update command, action matrix, jq recipes); contract tests pin serialized keys; broad test updates for GC shape and rollback status.

Reviewed by Cursor Bugbot for commit bc9c9be. Configure here.


Generated by Claude Code

Comment thread crates/socket-patch-cli/tests/e2e_bun_lockb.rs Outdated
@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.

Stale Bugbot comment from a previous run.

Comment thread crates/socket-patch-cli/tests/e2e_bun_lockb.rs Outdated
@Tanmay182003

Copy link
Copy Markdown

[agent] Follow-up on my earlier comment: I had the diagnosis half wrong, and the flip in 3e6153c trades one red for two others.

  1. rollback_refuses_binary_hosted_pin_then_checkout (tests/e2e_bun_lockb.rs:125) is shared by two callers that produce different envelopes:

    • native_binary_hosted_vendored_takeover_roundtrip (:1081) gets alreadyOriginal: 1 → partial_failure (now passes)
    • native_binary_alias_and_transitive (:973) gets alreadyOriginal: 0 → {"status":"error",...} (now fails at :135)

    The total_failure rule (rollback.rs:1604-1608) is right; the helper should take the expected status as a parameter (or derive it from env["alreadyOriginal"]) and assert error.code == "rollback_failed" when it's error. That's what keeps all 14 bun binary backtest cells and both e2e_bun_lockb legs red.

  2. CI's merge with main also fails tests/apply/bun_global_store.rs:191 (rollback_refuses_bun_global_store_packages, from Fix open bun issues (#992, #861, #784, #764, #735, #635, #599, #578, #497, #443, #371) #1009). It expects partial_failure when every package is refused, but under the new rule the result is {"status":"error","rolledBack":0,"alreadyOriginal":0,"failed":2}. That's what fails test (windows-latest, 2), test-release (1) and coverage. A rebase plus updating that test to expect error / rollback_failed fixes it.

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

bugbot run


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: pushed 113c017 (with a merge of main at 2990e48), following Tanmay Singla (@Tanmay182003)'s diagnosis.

  • e2e_bun_lockb's rollback_refuses_binary_hosted_pin_then_checkout now reads the expected status from alreadyOriginal. When it is above 0 it expects partial_failure (the takeover caller). Otherwise it expects error with error.code == rollback_failed (the alias/transitive caller).
  • apply/bun_global_store rollback_refuses_bun_global_store_packages now expects error/rollback_failed. That matches this PR's total-failure rule: every package is refused.

These pass locally: apply, rollback, in_process_rollback_hosted, covgap_commands_rollback, e2e_bun_lockb. Note that hosted-e2e, e2e_safety_pnpm and the Bun native legs are red on main too (production no longer serves the free [email protected] patch), so they don't block this PR.


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 9, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review (burn-down agent).


Generated by Claude Code

, #1066)

GC was reported four ways. repair and remove buried the sweep in
artifact-level event details with no byte count, while rollback and
scan --prune printed a hand-built `gc` object. The contract documented
summary.bytesFreed, summary.bytesDownloaded and events[].bytes, but no
command emitted any of them, so its GC jq recipe returned null.

- json_envelope::GcReport {removedBlobs, removedDiffArchives,
  removedPackageArchives, bytesFreed} is built from the three sweep
  passes and serialized identically everywhere: the envelope's new
  top-level `gc` (repair, remove), rollback's `gc` and scan's `gc`. The
  hand-written json! blocks are gone.
- summary.bytesFreed is always present and mirrors gc.bytesFreed.
  events[].bytes is set on the GC carrier event and on --update's
  downloaded event. summary.bytesDownloaded is dropped from the contract.
- repair's GC carrier event no longer bumps summary.removed/verified,
  matching remove: summary counters count patch entries, and the sweep
  totals live in `gc`.
- remove's human output now names the diff/package archives it sweeps.
- rollback --json: rolledBack and failed now span the agent, vendored
  and hosted legs (#1066). A run where something failed and nothing was
  rolled back, already original or not installed now reports
  status "error" with error.code rollback_failed instead of
  partial_failure. Exit codes are unchanged.
- CLI_CONTRACT.md: the envelope and PatchEvent schemas, the PatchAction
  "Emitted by" column, the per-command action matrix and the GC jq
  recipe now match the emitters. New unit tests pin the documented
  summary/gc/PatchEvent key sets against what json_envelope serializes.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
The --dry-run preview of scan --prune/--sync used its own vocabulary
(prunableManifestEntries, orphanBlobs, orphanDiffArchives,
orphanPackageArchives, revertableVendoredEntries, vendorOrphanDirs,
bytesReclaimable). It now prints the same keys as the wet pass and every
other GC-running command, counting what the pass would remove, and leaves
out only the keys a real pass alone can fill (keptVendoredEntries,
failedVendoredEntries, skipped, warnings). v5.0 MAJOR.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
All packages refused and nothing rolled back is a failed run since #1066.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[final reviewer] Not enqueuing. Tanmay Singla (@Tanmay182003), the commit you approved (113c0177) is no longer on this branch: the branch was rewritten afterwards, and the current head 9714d570 diverges from it (5 ahead, 4 behind). Non-merge commits on the new history are 7efdf798 (the main GC-report change), f7977faa (scan's GC dry-run preview in the shared gc shape) and ecbdd1ea (expect rollback_failed when every Bun global-store package is refused). ci-ok is red on this head. Please take another look once it's green.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
The shared bun.lockb rollback helper expected status "error" for every
caller, so the hosted -> vendored -> hosted takeover leg went red: there
a manifest record makes the agent leg report the copy already original,
and the run is correctly a partial_failure. A branch rewrite had dropped
the earlier fix for this. Each caller now states which outcome it
expects, so the takeover leg checks partial_failure and the hosted-only
alias/transitive shapes keep checking rollback_failed.

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.

✅ 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 bc9c9be. Configure here.

Comment thread crates/socket-patch-cli/src/json_envelope.rs Outdated
Resolve against main's removal of the diff download path (#1049) and
the restore-blob GC fix (#1316): repair drops the created-file blob
pass and keeps the GcReport carrier; remove keeps the archive noun
loop; the contract keeps "update" and drops the removed paidRequired
status; the envelope contract test uses AppliedVia::Blob.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
main's new test read gc.prunableManifestEntries, which this branch
renamed to prunedManifestEntries for the dry-run preview.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 10, 2026
Brings in main through #1277 via #1273. Resolutions:
- get keeps the envelope's paidRequired status and drops main's legacy
  {"status": "paid_required"} emitter; contract_paid_required.rs now pins
  the envelope row instead of the legacy one.
- repair: main removed the diff download path, so the created-file blob
  pass is gone; the download event keeps details.downloadMode, now
  always "file".
- scan: the envelope arms read main's lockfile_only_count; main's
  hoisted release-variant narrowing replaces the hosted-only copy in get.
- CLI_CONTRACT.md: three-way merged per paragraph; main's new hosted
  warning rows point at the top-level warnings[] like their neighbours.
- tests: main's new tests (cargo takeover refusal, #1127 human prune,
  bun.lockb already-original rollback) read the envelope shapes.
- json_envelope contract tests normalize CRLF so they pass on a Windows
  checkout.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
….lockb e2e

- json_envelope's contract tests read CLI_CONTRACT.md through one
  LF-normalized copy, so a Windows checkout (CRLF) no longer misses the
  ```jsonc fence (review thread on #1273; full-scope test windows).
- e2e_bun_lockb's shared-bundled case also has an agent copy that fails
  (hash_mismatch on a bundled copy the patch never touched); with #1066's
  all-leg counter, `failed` is the hosted refusal plus those agent failures.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 10, 2026
Takes #1273's LF-normalized contract_doc(); the bun.lockb shared-bundled
rollback counts agent failed events (no details.mode) on top of the
refused hosted pin.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Resolve remove.rs imports: keep main's KeepCause and this branch's GcReport.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 10, 2026
Brings in main's latest via #1273. Conflicts:
- CLI_CONTRACT.md: keep the envelope wording and port main's additions
  (cargo_build_cache_stale, cargo_cache_patch_kept, the Cargo shared-cache
  exemption in GC and rollback, redirect_cargo_dep_overridden,
  vendor_pypi_reinstall_required, vendor_revert_residual_reference, the
  pnpm keyless-yaml restore) onto it; GC warnings are top-level
  warnings[], a kept entry gets no preview event, and rollback's residual
  keep is its failed vendor_revert_kept event.
- rollback.rs tests: keep both the envelope tests and the PyPI reinstall
  note test.

Port main's new tests to the envelope: e2e_cargo reads pruned manifest
entries from details.manifest events and cargo_cache_patch_kept from the
top-level warnings; in_process_redirect and mode_migration_pypi count
hosted pins from events and read redirect_vendored_revert_failed from
the top-level warnings.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
main's #1278 e2e (sync_keeps_entry_whose_shared_cache_copy_is_still_patched)
reads the preview's prunableManifestEntries, which the one GC shape
renamed to prunedManifestEntries; the contract text it brought gets the
same rename.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 10, 2026
Keep the envelope ports of e2e_cargo and the Cargo GC contract text
(pruned entries are details.manifest events, so the preview key rename
on #1273 does not apply here).

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
#1305's sync_keeps_entry_whose_shared_cache_copy_is_still_patched
asserted the scan --sync --dry-run preview via gc.prunableManifestEntries,
a key #1273 retires: under the one GC shape a dry run reports would-be
prunes as gc.prunedManifestEntries. The keep logic itself merged intact
(preview and wet pass both keep the still-patched itoa entry, and the wet
pass reports cargo_cache_patch_kept in gc.warnings[] with the purl and the
rollback remedy), so only the test read the wrong key. Assert the same
ryu-only list under the new key.

CLI_CONTRACT.md's scan --prune paragraph still named the retired preview
keys (prunableManifestEntries, revertableVendoredEntries,
vendorOrphanDirs); point it at the unified keys and note that the
preview, like every warnings[] entry, carries no cargo_cache_patch_kept.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
# Conflicts:
#	crates/socket-patch-cli/CLI_CONTRACT.md
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 10, 2026
Keep the envelope wording for the Cargo shared-cache GC keep (no event
and no warning in the preview) and take #1273's vendor_artifact_gitignored
NuGet/JVM contract updates with the envelope's top-level warnings[].
e2e_cargo keeps the events-based prune read with #1273's message.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Merged via the queue into main with commit e3eaa0c Oct 10, 2026
53 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the fix/gc-report-json-1257 branch October 10, 2026 16:42
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 10, 2026
#1273 landed as squash e3eaa0c, whose tree is fix/gc-report-json-1257
(ff9704a, already merged here) plus #1342. Conflicts take this branch's
side throughout (the squash adds nothing beyond ff9704a); #1342's
nuget_feed.rs merges cleanly and its CLI_CONTRACT.md nuget drift note is
ported.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Mikola Lysenko (mikolalysenko) added a commit that referenced this pull request Oct 10, 2026
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