Skip to content

Fix gitignored ledger/manifest silently dropped (#1638) - #1640

Open
Mikola Lysenko (mikolalysenko) wants to merge 7 commits into
mainfrom
agent/fix-committed-state-gitignored
Open

Mikola Lysenko (mikolalysenko) wants to merge 7 commits into
mainfrom
agent/fix-committed-state-gitignored

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #1638

Root cause

The CLI tells users to commit two state files, .socket/vendor/state.json (the vendor ledger) and .socket/manifest.json (the agent-mode manifest). It never asked git whether it would actually commit them. The only gitignore checks (npm_dir::gitignored) looked at the vendored artifact directories, and only in some backends. So a .gitignore rule like state.json, *.json or manifest.json silently dropped the file from the commit. The scan still exited 0, and every other checkout then broke:

  • vendored: the lock wiring is there but the ledger is missing, so rollback hits vendor_ledger_missing and list shows no patches.
  • agent: the blobs are there but the manifest is missing, so CI's socket-patch apply reports noManifest, exits 0, and the package installs unpatched.

Changes

  1. Vendored runs refuse (exit 1, nothing written, dry run included). vendor::ledger_gitignore_refusal lives in vendor/state.rs, next to the ledger. It makes one git check-ignore call over .socket/vendor/ and the ledger and honours the index, so a ledger that is already tracked is never refused. It runs at the shared boundary vendor_records_reusing, which covers vendor and scan/get --mode vendored for every backend. It also runs at the start of the hosted→vendored eject, before any pin is restored.
    • A file rule that only matches the ledger gives vendor_ledger_gitignored, and the message names the rule.
    • A .socket/ or .socket/vendor/ directory rule is left to the per-package vendor_artifact_gitignored refusals when every ecosystem in the run checks that itself (npm family, NuGet, JVM), so that behaviour is unchanged. Otherwise (gem, PyPI, Cargo, Go, Composer) it is now a run-level vendor_artifact_gitignored. Before this PR, those projects lost the ledger and the artifacts silently.
  2. Agent-mode runs warn (manifest_gitignored, exit code unchanged). Both the search/scan path (download_and_apply_patches_into) and get <uuid> (save_and_apply_patch) warn whenever the run leaves patches recorded in a manifest git would not commit. This is a warning and not a refusal, because running scan and apply in the same CI job without ever committing .socket/ is a legitimate workflow.
  3. CLI_CONTRACT.md documents both codes.

Tests (red → green)

Each new test fails on main and passes here. The ones I checked by reverting just the fix are marked ✔.

  • in_process_vendor::a_gitignored_ledger_refuses_before_any_write ✔ covers state.json, *.json + !package*.json and **/state.json, each with and without --dry-run: exit 1, error code, nothing written, lock unchanged.
  • in_process_vendor::scan_vendor_gem_refuses_a_gitignored_ledger ✔ runs scan --mode vendored on gem (a non-npm backend) with a state.json rule, which gives vendor_ledger_gitignored, and with a .socket/ rule, which gives vendor_artifact_gitignored.
  • in_process_vendor::hosted_to_vendor_conversion::hosted_then_vendor_with_a_gitignored_ledger_keeps_the_hosted_wiring ✔ covers the eject (no manifest) and the manifest-driven takeover. The hosted lock and .npmrc stay untouched.
  • get::get_failure_paths::agent_record_warns_when_git_ignores_the_manifest ✔ covers manifest.json, *.json + !package.json and .socket/, which warn, and a re-included manifest, which doesn't.
  • get::get_failure_paths::get_uuid_warns_when_git_ignores_the_manifest ✔ covers the get <uuid> save path.
  • Controls: a_re_included_ledger_vendors, a_tracked_ledger_is_not_refused, agent_record_does_not_warn_for_a_tracked_manifest. The existing hosted_then_vendor_under_a_gitignored_socket_dir_keeps_the_hosted_wiring and vlt::vendor_vlt_refuses_gitignored_payload still pass.

Local: cargo fmt --all -- --check and cargo clippy --workspace --all-features -- -D warnings are clean. Tests: socket-patch-core lib has 6542 passing and 4 failing. All 4 are read-only-directory tests (copy_tree::relax_loop_must_not_traverse_symlinked_root, vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry, pypi_poetry::wire_write_failure_maps_error_and_leaves_lock_untouched, pypi_requirements::wire_failure_rolls_back_already_written_files). They fail only because this container runs as root, which ignores chmod, and none of their files are in this diff. socket-patch-cli lib, in_process_vendor and get all pass (924, 139 and 170 tests). A full cargo test --workspace run ran out of disk while linking, so CI runs the full matrix. No wrapper changes are needed: the npm/, pypi/ and gem/ wrappers only dispatch to the binary.

/code-review high

The review found 10 issues. I fixed 1–5, 7, 8 and 10 in 8caf8d6: tracked files were falsely reported as ignored (--no-index), the directory-rule gap for backends that don't check it, get <uuid> not warning, remedy text that couldn't work under an ignored .socket/, four git spawns per check, a misplaced doc comment, and the helpers living in the npm module. Not fixed:

  • Add packageManager field to specify pnpm version #6, git failing to answer is treated as "not ignored". This matches npm_dir::gitignored's documented fail-open. The npm-family artifact check already warns vendor_artifact_gitignore_unchecked for that same broken-git case, so a second warning would only duplicate it.
  • Add support for public patch API proxy #9, a small git_project_ignoring test helper is copied in two test binaries. The two binaries include different shared-helper modules (common/hermetic.rs vs common/mod.rs), so sharing it would mean wiring a new module into one of them just for a 10-line helper.

CI

1a25bb3 ports #1639's setup-uv 0.13.1 pin. uv 0.13.2 broke the Pipenv pylock e2e legs on every PR, and the pin no-ops once #1639 merges. Bugbot found no issues on 8caf8d6.

https://claude.ai/code/session_01QvLvEUfidP565pLJTJUJLK


Note

Medium Risk
Changes vendor exit behavior and adds git subprocess checks on common code paths; low security impact but can block vendoring in repos with broad *.json or state.json ignore rules until fixed.

Overview
Fixes #1638 by detecting .gitignore rules that would keep critical Socket state out of git before users commit broken trees.

Vendored workflows now fail early (vendor_ledger_gitignored, exit 1, no writes, including --dry-run) when .socket/vendor/state.json would not be committed. The check runs on normal vendor / scan --mode vendored paths and on hosted→vendored eject before any lock changes. For ecosystems that do not already enforce per-artifact ignores (e.g. gem), a run-level refusal also covers directory rules like .socket/ that would drop the whole vendor tree.

Agent mode (get, batch download/apply) emits a manifest_gitignored warning (exit unchanged) when .socket/manifest.json would be ignored—so CI that never commits .socket/ still works, but teams that do commit get a clear signal. Already-tracked ledger/manifest files are not flagged.

Git probing was extended so check-ignore can honor the index (tracked files) vs --no-index for new paths. CI pins uv 0.13.1 in Pipenv/pylock legs because 0.13.2 rejects Pipenv’s empty environments pylock.

Reviewed by Cursor Bugbot for commit 6094254. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
A .gitignore rule such as state.json or *.json kept
.socket/vendor/state.json out of the commit a vendored run asks for.
The scan exited 0, but every other checkout then had the lock wiring
and no ledger, so rollback failed with vendor_ledger_missing and list
showed no patches.

Vendored runs (vendor, scan/get --mode vendored, every backend) now
ask git before writing anything and refuse with
vendor_ledger_gitignored, naming the rule, dry run included.

Refs #1638

Assisted-by: Claude Code:claude-opus-5-5
A .gitignore rule such as manifest.json kept .socket/manifest.json
out of the commit while its blobs went in. In CI or a fresh clone,
socket-patch apply then found no manifest, applied nothing and exited
0, so the package stayed unpatched.

Agent-mode scan and get now warn with manifest_gitignored, naming
the rule, whenever the run leaves patches recorded in an ignored
manifest. It stays a warning: a pipeline that scans and applies in
one job never commits .socket/.

Refs #1638

Assisted-by: Claude Code:claude-opus-5-5
The standalone vendor eject restores every hosted pin upstream before
the vendored engine runs. With a .gitignore rule matching
.socket/vendor/state.json it now refuses with vendor_ledger_gitignored
before fetching or restoring anything, so the project stays hosted.

Refs #1638

Assisted-by: Claude Code:claude-opus-5-5
Review follow-ups for the #1638 checks:

- Ask git with the index, so a ledger or manifest already tracked
  (force-added, or committed before the rule) is not reported as
  ignored: git keeps committing changes to it.
- A .socket/ or .socket/vendor/ directory rule now refuses the run as
  vendor_artifact_gitignored when it includes a backend that does not
  check that rule per package (gem, PyPI, Cargo, Go, Composer), so
  those projects no longer lose the ledger and artifacts silently.
- One git check-ignore call per run instead of two.
- get <uuid> warns about an ignored manifest like the search path.
- The manifest warning no longer suggests a !rule that cannot work
  under an ignored parent directory.
- Move the helpers next to the ledger in vendor/state.rs.

Refs #1638

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 20, 2026 17:00
@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.

uv 0.13.2 refuses the pylock.toml Pipenv writes (environments = []),
so the Pipenv e2e legs fail on every PR. Same change as #1639; it
no-ops once that lands on main.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] e2e (ubuntu-latest, e2e_vex_build, pipenv:: --ignored, 2026.8.0) failed on 8caf8d6. This PR does not cause it: uv 0.13.2 refuses the environments = [] pylock that Pipenv writes, and the same leg fails on other PRs (#1591). I ported #1639's setup-uv 0.13.1 pin in 1a25bb3. It no-ops once #1639 lands. This PR touches no Pipenv code.


Generated by Claude Code

@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.

@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.

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

Copy link
Copy Markdown
Collaborator Author

Labeled Ready for review (burn-down agent).


Generated by Claude Code

artifacts_gitignore_checked was inserted between emit_eject_refusal and
its doc comment, so the refusal helper lost its doc and the new helper's
doc opened with an unrelated sentence. The PR description already claims
this was fixed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Pu6ZWFiGyCEMEvfpnZrnVa
@mikolalysenko Mikola Lysenko (mikolalysenko) removed the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 20, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[final reviewer] I reviewed 1a25bb3 and found nothing that blocks merging. I pushed one small fix, 6094254: artifacts_gitignore_checked had been inserted between emit_eject_refusal and its doc comment (vendor.rs:1400), so I moved that doc comment back onto emit_eject_refusal. The change is comments only, and cargo clippy -p socket-patch-cli -- -D warnings is clean. I removed Ready for review until CI is green on the new head, and I'll post the approver brief at that head.

What I checked on 1a25bb3

  • in_process_vendor: 139 passed. get: 170 passed. Core vendor::npm_dir / vendor::state: 42 passed.
  • The new ledger-refusal and manifest-warning tests fail when crates/*/src is reverted to main, so they exercise the change.
  • I probed git 2.43 check-ignore by hand: the path is echoed back as given, tracked ledgers report nothing, !-negations are filtered, and a git error (exit 128) fails open.

Non-blocking notes and questions for the approver

  1. Existing projects already in the Vendored scan exits 0 when .gitignore covers .socket/vendor/state.json, so the commit silently drops the ledger and every other checkout's rollback fails with vendor_ledger_missing #1638 state (an untracked ledger plus a state.json or *.json rule) will now exit 1 on every vendored run until the rule is fixed. That is intended, but it may deserve a release-note line.
  2. Once anything under .socket/vendor/ is tracked, git (without --no-index) no longer reports a .socket/ directory rule. In that case gem, PyPI, Cargo, Go and Composer runs still get no run-level vendor_artifact_gitignored. main behaves the same, so this is not a regression.
  3. If git is broken on a non-npm backend, the ledger check is skipped without any *_unchecked warning.
  4. The new tests call the real git init without isolating HOME or core.excludesFile, so an unusual global excludes file on a dev machine could affect them. CI is fine.

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 6094254. Configure here.

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

Copy link
Copy Markdown
Collaborator Author

Labeled Ready for review again (burn-down agent) after the final reviewer's push.

  • Head: 6094254. Mergeable, no conflicts; CHANGELOG.md not touched.
  • CI: 55/55 checks green (36 success, 19 skipped). No main-wide failures.
  • Bugbot: reviewed 6094254, no findings; no open review threads.
  • Slack: not announced this run (no Slack send tool available); the next run will retry.

Generated by Claude Code

@mikolalysenko

Mikola Lysenko (mikolalysenko) commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does. Vendored runs (vendor, scan/get --mode vendored, and the hosted-to-vendored eject) now ask git check-ignore once, index honoured, before writing anything (dry run included). If a file rule such as *.json or state.json ignores .socket/vendor/state.json and git doesn't already track it, the run is refused with vendor_ledger_gitignored. A .socket/ or .socket/vendor/ directory rule refuses the run as vendor_artifact_gitignored when it includes a backend with no per-package check of its own (gem, PyPI, Cargo, Go, Composer). Agent-mode scan/get also warn with manifest_gitignored; this is a warning only and doesn't change the exit code.

Risk: low to medium. The checks fail open: no git, not a work tree, or a git error mean no refusal. They cost one rev-parse and one check-ignore per run. The medium part is a deliberate behaviour change, documented in CLI_CONTRACT.md. A gem/PyPI/Cargo/Go/Composer vendored run in a repo that ignores .socket/ used to exit 0 and now exits 1. A mixed run (npm plus gem) under that rule now fails as a whole instead of per package.

Look here

Verified

  • cargo clippy --locked -p socket-patch-core -p socket-patch-cli --all-features -- -D warnings: clean.
  • cargo test -p socket-patch-cli --test in_process_vendor: the new ledger, re-include, tracked-ledger, gem and eject tests pass, and so do the existing Vendored yarn classic exits 0 when .gitignore covers the vendored tarball (*.tgz, vendor/, .socket/), so the commit drops it and every fresh checkout's install fails #831 tests.
  • cargo test -p socket-patch-cli --test get -- manifest: 16/16.
  • cargo test -p socket-patch-core --lib gitignore: 9/9.
  • The new tests assert codes that don't exist on main, so they would fail without the change.
  • Checked check-ignore behaviour by hand on git 2.43.
  • The uv-pin commit 1a25bb3 is identical to main (git diff origin/main -- .github/ is empty).
  • CHANGELOG.md is untouched.
  • CI 55/55 green on 6094254 (36 success, 19 skipped). Bugbot: success. 0 open threads. Mergeable.

Changes I made: none.

Open questions (non-blocking, follow-up material)

  1. If any file under .socket/vendor/ is already tracked and the repo has a .socket/ rule, git doesn't report the directory, so the run is refused as vendor_ledger_gitignored and told to add !.socket/vendor/state.json, which can't work under an ignored parent. The refusal is right; only the code and hint are wrong, and the case is narrow.
  2. A gem/PyPI/Cargo/Go/Composer project that already committed its ledger and artifacts and then adds .socket/ to .gitignore is still not caught: the probe honours the index and reports nothing. That matches main today, but commit 8caf8d6's message overstates the coverage.
  3. Are you OK with the run-level failure for mixed npm+gem runs under a .socket/ rule (see Risk)?

Auto-merge (squash) is armed, so approving sends this straight to the merge queue.


Generated by Claude Code

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

2 participants