Skip to content

Build tests/apply fixtures through apply_fixture (#1941) - #1944

Merged
Mikola Lysenko (mikolalysenko) merged 3 commits into
mainfrom
arch-refactor/1941-apply-fixture
Nov 2, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 3 commits into
mainfrom
arch-refactor/1941-apply-fixture

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator

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

Fixes #1941

Summary

Nine suites of the apply test binary each wrote their own manifest, blob, npm-package, gem and event-lookup helpers beside apply_fixture's (register row T51, Part 5 of the living document). They now build every fixture through apply_fixture, and the event-by-purl lookup lives in common/envelope.rs. The PR changes only tests; there is no product code in it.

Changes

  • apply_fixture.rs
    • New hashed_file (one_file from hashes) and index_js_patches (one package/index.js record per purl).
    • link_dir moved here from the two global-store suites.
    • run_gem: run_with_env's env plus the BUNDLE_* scrub. run_gem_apply is built on it.
    • stage_blob writes through common::write_blob.
  • common/envelope.rs: new event_for(envelope, purl), replacing 4 identical private event fns.
  • Local copies deleted:
    • apply_network.rs: write_npm_package, write_root_package_json, write_manifest_with_patch and 8 hand-staged blobs.
    • apply_invariants.rs: MANIFEST_JSON, two hand-formatted manifests, dir_hash + collect_files (the file already had snapshot_dir), and its private run_apply.
    • bun_global_store.rs, pnpm_global_virtual_store.rs: link_dir, write_manifest, event.
    • cargo_lock_source.rs, check_verifies_installed_tree.rs, lockfile_only_skip.rs: write_manifest/record/project writers, install_npm_pkg, run_apply, run_cargo_apply, store_after_blob, event.
    • gem_stale_bundler_plugin.rs, in_process_gem_config_warning.rs, in_process_gem_fallback_home.rs: their hand-formatted rack manifests and runners.
  • Moved: the fallback-home human-warning test goes from apply_render.rs into in_process_gem_fallback_home.rs, where it reuses that file's fixture. Its assertions are unchanged.
  • gem_stale_bundler_plugin's child now gets the Extra::Bundle scrub, as the issue asks.
  • 13 explicit SOCKET_TELEMETRY_DISABLED=1 pairs dropped: hermetic::command forces that value already. apply_network's =0 opt-in stays.
  • Tests changed:
    • apply_does_not_mutate_socket_dir_offline and apply_does_not_mutate_socket_dir_when_no_packages_match now compare .socket/ path→bytes snapshots. They used to compare a digest and then re-check two files ("belt-and-suspenders against a dir_hash blind spot"); the snapshot has no such blind spot. Red→green: writing one byte into the sentinel after the run fails the test at the snapshot assert.
    • All other assertions are unchanged.
    • Test count is 157 before and after.

Size

git diff --stat: 13 files, +410 / −1,045, all of it test code. Production lines: 0.

Behavior change

None in the product. In the tests:

  • Gem children now also get jvm_env::isolate_cli (stand-in HOME), as every run_with_env child already does.
  • apply_invariants' children get the same isolation through run_apply.
  • Record descriptions and exportedAt are now the fixture's.
  • Fixtures that had no .socket/blobs/ dir now get an empty one.

/code-review high

It reported 10 findings; I fixed 9:

  • The gem suites that pin --cwd (config_warning, fallback_home) run from the runner's dir again.
  • apply_invariants uses the shared run_apply.
  • store_after_blob was replaced by stage_blob.
  • The specifications dir is now derived from install_gem's path.
  • write_patches copies were merged into index_js_patches.
  • apply_network uses one_file.
  • Two stale docs were fixed.

I did not fix one: "run_gem copies run_bin_with_env's body to insert the scrub." The fix would add an extras parameter to common::run_bin_with_env, which has 100+ callers across binaries. That belongs to T55/#1089, not this PR.

Not done from the issue: step 3's single manifest writer (write_manifest on top of common::write_minimal_manifest). That writer takes one purl and has 100+ callers in other binaries, so giving it a records parameter goes beyond this binary. Only stage_blob was unified.

Verification

  • cargo test -p socket-patch-cli --test apply: 157 passed.
  • Clippy reports nothing in the touched files. The existing test-target lints in vlt_hosted_common/prebuilt_common are untouched, and CI's clippy does not lint tests.

CI

79 of 81 checks and Bugbot are green. hosted-e2e (and the ci-ok aggregate) is red: e2e_hosted_production::bun_hosted_install_proof gets a 403 from the live patch.socket.dev for minimist-1.2.2.tgz, on 3 attempts in each of two runs. That test binary is not in this diff, and the same check passed on #1939 at 15:12Z.

Risk

Low: tests only.

https://claude.ai/code/session_011f9aeSfw2PKstyBdKzjUFz


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko Mikola Lysenko (mikolalysenko) added refactor Structural change: duplicated code or logic, missing abstraction, layering, dead code arch-refactor PR opened by the scheduled architecture refactor routine labels Nov 1, 2026
Nine suites of the apply test binary wrote their own manifest, blob,
npm package, gem and event-lookup helpers beside apply_fixture's. They
now call apply_fixture (one_file/hashed_file, write_manifest,
stage_blob, install_npm_pkg, install_gem, write_gem_patch, link_dir)
and common::envelope::event_for. stage_blob writes through
common::write_blob.

The four gem runners share apply_fixture::run_gem, so
gem_stale_bundler_plugin's child now gets the BUNDLE_* scrub the
others had. The fallback-home human test moves next to its --json
twin and reuses its fixture. apply_invariants compares .socket/
snapshots instead of a digest plus two byte checks. The explicit
SOCKET_TELEMETRY_DISABLED=1 pairs, which hermetic::command forces,
are gone.

Assisted-by: Claude Code:claude-opus-5-5
Gem suites that pin --cwd run their child from the runner's directory
again, so a crawler reading the process cwd still fails them.
apply_invariants runs apply through apply_fixture::run_apply, the
--cwd remedy test stages its blob with stage_blob, and the gem
--check fixture derives its specifications dir from install_gem's
path. bun_global_store and lockfile_only_skip build their records
through one index_js_patches helper. apply_network passes bytes to
one_file instead of precomputed hashes. Stale module docs updated.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review November 1, 2026 17:11
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Nov 1, 2026
Assisted-by: Claude Code:claude-opus-5-5

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

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] hosted-e2e (and the ci-ok aggregate) failed on e89dde25. Two tests in e2e_hosted_production.rs failed: bun_hosted_install_proof on all 3 attempts, and gem_bundler_hosted_install_proof on the first only. The bun failure is bun install --frozen-lockfile getting GET https://patch.socket.dev/patch/npm/minimist/1.2.2/…/minimist-1.2.2.tgz - 403 from the live hosted service.

This PR doesn't cause it:

I've re-run the failed jobs once. If it is still red, the 403 is coming from the hosted service and needs someone with access to patch.socket.dev to look.


Generated by Claude Code

Mikola Lysenko (mikolalysenko) pushed a commit that referenced this pull request Nov 1, 2026
Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Nov 1, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[burn-down agent] Labeled Ready for review at e89dde2.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does. Test-only refactor of the apply test binary for #1941. Nine suites each had their own manifest, blob, npm-package, gem and event-lookup helpers. They now build every fixture through apply_fixture.rs, and one event_for in common/envelope.rs replaces four copies. The diff is +410 / −1,045 lines. Product code is untouched.

Risk: low. Only test code changes. The #[test] set is identical before and after: 121 fns in tests/apply/, matched name by name, and the binary runs 157 tests.

Look here

  • apply_invariants.rs#L196-L270 and #L506:``` the two no-mutation tests now compare a path→bytes snapshot of .socket/. It covers everything the old digest plus two re-checks did, and it panics on an unreadable entry instead of skipping it.
  • apply_fixture.rs#L284-L308:```` run_gem. The order is `isolate_cli` HOME, then the `BUNDLE_*` scrub, then the caller's env, so each test's own `BUNDLE_APP_CONFIG` still wins.
  • apply_fixture.rs#L72-L82:````` write_manifest now always creates `.socket/blobs/`. This makes the "blobs stays empty" checks in `apply_network` always run. The trade-off is that no test in this binary now covers apply on a tree that has no `blobs/` dir.
  • in_process_gem_fallback_home.rs#L333-L360:`````` the human-warning test moved here from apply_render.rs. Its assertions are unchanged.
  • common/hermetic.rs#L71-L120: forced opt-outs. Every call site whose explicit SOCKET_TELEMETRY_DISABLED=1 was dropped goes through this.

Verified

  • Full diff read by an independent review pass. It found no weakened or dropped assertions and no blocking findings.
  • Fixture byte constants, UUIDs and descriptions match wherever tests pin them.
  • link_dir, the Windows junction helper, moved verbatim. Manifests are now written as JSON, so there is no CRLF risk.
  • CHANGELOG.md is untouched, and there is no debug code.
  • cargo test -p socket-patch-cli --test apply on the head: 157 passed, 0 failed.
  • cargo clippy --all-targets fails, but only on lints already present in tests/prebuilt_common/mod.rs, a file this PR does not touch. None of the failures point at the touched files.
  • CI on the head: 32 checks green, 50 skipped, 0 failing. ci-ok and clippy are green. Bugbot is green.

Changes I made: none.

Open questions (non-blocking)

  • The description says 13 telemetry env pairs were dropped; the diff drops 12.
  • The new event_for panic no longer names the Shape in cargo_lock_source, so a missing event there is slightly harder to trace.

Auto-merge is armed, so approving sends this PR straight to the merge queue.


Generated by Claude Code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[agent] Reviewed e89dde25d2df9c54179e7fb3067a32ccbb00eecf. Checked common apply fixtures/runners and preserved offline, scope, per-copy and installed-tree assertions. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.

@mikolalysenko
Mikola Lysenko (mikolalysenko) added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit 55d0dae Nov 2, 2026
166 of 170 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the arch-refactor/1941-apply-fixture branch November 2, 2026 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-refactor PR opened by the scheduled architecture refactor routine Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review refactor Structural change: duplicated code or logic, missing abstraction, layering, dead code

Projects

None yet

3 participants