Skip to content

Fix stale docs and comments, and the npm wrapper's schema and exit code - #1022

Open
Mikola Lysenko (mikolalysenko) wants to merge 8 commits into
mainfrom
arch-fix/repo-docs
Open

Mikola Lysenko (mikolalysenko) wants to merge 8 commits into
mainfrom
arch-fix/repo-docs

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

Problem

The architecture audit (report §6.3, §7 item 17, and B71) found docs and code comments that no longer match the code, plus three defects in the npm wrapper package:

  • CLI_CONTRACT.md:907 says a declined rollback prompt prints Rollback cancelled.. The code prints the shared ui::CANCELLED text, Cancelled; no changes made. (rollback.rs:1430).
  • CLI_CONTRACT.md:1136 says tests/cli_parse_*.rs are "snapshot tests that lock the shape" of the JSON envelope. Those tests pin the parsed clap *Args structs. The envelope shape is pinned by json_envelope.rs's unit tests and by each command's e2e tests.
  • Comments about the deleted hosted ledger and replay: CLI scan/hosted.rs, core hosted/engine.rs and patch/redirect/staged.rs still describe a redirect-ledger load, a merge-then-persist, a ledger-record fallback, replay.rs, super::ledger, super::takeover, super::replay, and edit ordering for a "whole-ledger replay". None of these exist in v5.
  • vex/discover/pypi_other.rs:20 names vendor::pypi_pipenv::is_socket_hosted_reference as an any-host twin. Recognize hosted Pipenv references through one shared grammar (#563) #572 deleted that function.
  • update/release.rs:15-17 says the API client has a "no-timeout posture". That has been false since Bound patch API connects and stalled reads (#570) #581 added ApiTimeouts.
  • scan/policy.rs:70-72 says "manifests are not markers". dir_markers falls back to manifests whenever a directory has no lock marker.
  • B71 (npm wrapper):
    • The zod PatchManifestSchema lacks the legacy setup block that the Rust manifest still parses and keeps, so validating a pre-v5 manifest strips it.
    • The schema tests never run in CI, and the test script calls pnpm in an npm-locked package.
    • A binary killed by a signal is reported as exit 1.
    • With no files list, the tarball ships src/, the tsconfig and the compiled tests.

Change

  • Fixed both CLI_CONTRACT lines.
  • Rewrote the stale comments to describe what the code does now:
    • Hosted mode keeps no ledger. The vendored ledger is the only one the hosted flow loads.
    • The gem stale-install probe judges only this run's fetched records, and its test helper and uuid-matching test now say so.
    • staged.rs serves the hosted → upstream restore.
    • Self-update uses whole-request deadlines, while ApiTimeouts uses connect and per-read bounds.
    • The pypi twin is hosted_pypi_reference.
    • dir_markers falls back to manifests, and the in-memory engine does not.
  • npm wrapper:
    • Added SetupConfigSchema (optional exclude/manual string lists, mirroring SetupConfig in core manifest/schema.rs) to PatchManifestSchema.
    • A signal death now exits with 128 + the signal number (SIGINT 130, SIGTERM 143), as a shell reports it.
    • Added a files list: the wrapper, any bundled bin/socket-patch-* binaries, and the compiled schema .js/.d.ts.
    • The test script now uses npm, and the dispatch-tests CI job runs it.

Every change is to docs, comments or the npm/socket-patch package, plus one added step in the CI dispatch-tests job. No Rust behavior changes. Duplicate copies deleted: none (this workstream consolidates no code).

Testing

  • node --test npm/socket-patch/bin/socket-patch.test.mjs: 23 pass, 1 skipped (the Linux-only musl fallback). Four of these are new and failed first: three signal-exit tests, and the npm pack --dry-run contents test. Once dist/schema is built, the contents test also requires both dist/schema/manifest-schema.js and .d.ts in the pack list. CI re-runs it with --test-name-pattern="npm package contents" right after npm test builds dist/.
  • npm test in npm/socket-patch (tsc + zod tests): 10 pass. The three new setup tests failed first, at compile time.
  • cargo build -p socket-patch-cli: OK.
  • cargo test -p socket-patch-core --lib filtered to hosted::engine, update::release, vex::discover::pypi_other and patch::redirect: 166 pass.
  • cargo test -p socket-patch-cli --lib -- scan::hosted scan::policy: 61 pass, re-run after the review fixes. That includes the renamed gem_stale_probe_matches_records_by_uuid_not_purl_key.
  • cargo clippy -p socket-patch-cli --all-targets -- -D warnings on macOS:
    • One pre-existing error on main: unused unix_default at core crawlers/python_crawler.rs:2734, a cfg-dependent macOS-only warning in a file this PR does not touch.
    • With that lint allowed (-A unused_variables), every remaining lint is in crates/socket-patch-cli/tests/** integration-test files this PR does not touch. There are none in src/.
    • Linux clippy is left to CI.
  • I checked every --flag, subcommand and --mode/--vendor-source value in README.md and docs/*.md against the built binary's --help. I found no drift: every unmatched flag belongs to another tool (cargo, git, gh, gradle, dotnet).
  • All commands ran through the shared heavy-job.sh limiter with -j4 and CARGO_INCREMENTAL=0.

Deferred

🤖 Generated with Claude Code


Note

Low Risk
Documentation and npm wrapper packaging/exit-code behavior only; Rust CLI logic is unchanged aside from comment edits.

Overview
Aligns CLI_CONTRACT and scattered Rust comments with v5 behavior (no hosted redirect ledger, shared cancel text, where JSON envelope tests live, dir_markers manifest fallback, self-update timeouts, PyPI hosted URL parsing). No Rust runtime changes.

The @socketsecurity/socket-patch npm package gets substantive fixes: optional legacy setup block on the Zod manifest schema (parity with Rust), shell-style exit codes on signal death, an explicit files whitelist so publishes ship only the wrapper and compiled schema, npm test instead of pnpm, and new tests (signals, npm pack contents, setup round-trip). CI dispatch-tests now runs schema build/tests and re-runs the pack-contents check after dist/ exists.

Reviewed by Cursor Bugbot for commit 6af73d3. Configure here.

The rollback prompt's decline line is the shared "Cancelled; no changes
made." (ui::CANCELLED), not "Rollback cancelled.". The JSON envelope
shape is pinned by json_envelope.rs's unit tests and the commands' e2e
tests; tests/cli_parse_*.rs pin the parsed clap arguments, not the
envelope.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
v5 hosted mode keeps no ledger, and patch/redirect has no ledger,
replay or takeover module any more. The hosted engine, the CLI hosted
flow and the staged I/O layer still described a ledger load, a
merge-then-persist, a ledger-record fallback for the gem stale-install
probe and edit ordering for a whole-ledger replay. Say what the code
does now: the vendored ledger is the only one the hosted flow loads,
the gem probe judges only this run's fetched records, and staged.rs
serves the hosted -> upstream restore.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- update/release.rs: the API client is no longer timeout-free; contrast
  self-update's whole-request deadlines with ApiTimeouts' connect and
  per-read bounds instead.
- vex/discover/pypi_other.rs: is_socket_hosted_reference was deleted by
  #572; name hosted_pypi_reference, which shares the path parser and
  the origin allowlist.
- scan/policy.rs: dir_markers falls back to manifests when a directory
  has no lock marker, which the in-memory engine does not do.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
spawnSync gives status null when the binary dies by a signal, and the
wrapper turned that into exit 1, so Ctrl-C read as an ordinary failure.
Return 128 + the signal number, as a shell does (SIGINT 130, SIGTERM
143).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- The zod PatchManifestSchema lacked the legacy setup block the Rust
  manifest still parses and keeps, so validating a pre-v5 manifest
  stripped it. Add SetupConfigSchema (optional exclude/manual lists).
- package.json had no files list, so the tarball shipped src/, the
  tsconfig and the compiled tests. Publish only the wrapper, bundled
  binaries and the compiled schema; a pack test guards the list.
- The test script called pnpm in an npm-locked package and no workflow
  ran it. Use npm, and run it in the dispatch-tests job.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@mikolalysenko Mikola Lysenko (mikolalysenko) added the arch-refactor PR opened by the scheduled architecture refactor routine label Oct 7, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Filtered Rust tests for the touched modules pass on macOS:

  • cargo test -p socket-patch-core --lib -- hosted::engine update::release vex::discover::pypi_other patch::redirect::upstream: 166 passed
  • cargo test -p socket-patch-cli --lib -- scan::policy scan::hosted: 61 passed

The test helper and one test still described the merged ledger map the
deleted hosted ledger used to feed. Production now hands over only this
run's fetched records; the renamed test pins what it really checks: a
record keyed under another purl spelling still matches by uuid.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The pack-contents test only allowlisted dist/schema files, so a files
list that dropped them would still pass. When dist/schema is built,
both manifest-schema.js and .d.ts must be in the pack list.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 7, 2026 15:37

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

arch-refactor PR opened by the scheduled architecture refactor routine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant