Fix lock rewrites changing file owner (#1635)

[Mikola Lysenko (mikolalysenko)]

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

Fixes #1635

Summary

A hosted or vendored scan run as root (a Docker bind mount, sudo) used to leave the project's poetry.lock owned by root, so the owner's next poetry add / poetry lock failed with Permission denied. A run in a shared group checkout moved the lock to the runner's primary group. Rewritten lockfiles, manifests and .npmrc now keep their owner and group, the same as their mode.

Root cause

Every mode-preserving rewrite of a project file (poetry.lock, package.json, .npmrc, Gemfile.lock, …, plus the rollback / remove / vendor --revert restores and the group-commit replay) runs through stage_and_rename_blocking in crates/socket-patch-core/src/utils/fs.rs. It stages a fresh file, copies only the old file's permission bits onto it, and renames it over the old path. The fresh inode belongs to the process's euid/egid. So the defect isn't Poetry-specific: every ecosystem's lock and manifest rewrite had it.

Fix

keep_owner fchowns the stage to the old file's uid/gid before the fsync and before the chmod (chown(2) clears setuid/setgid), best-effort like npm's write-file-atomic. An unprivileged caller that can't give the file away (EPERM) still keeps the group when it belongs to it; otherwise the file keeps the runner's ownership, as before. CLI_CONTRACT.md now states the ownership rule.

Test evidence

Test	Without fix	With fix
utils::fs::tests::preserving_writes_keep_the_destination_owner (root; every preserving writer, setuid mode kept)	FAIL (0, 0, 2540) vs (1000, 1001, 2540)	pass
utils::fs::tests::preserving_write_keeps_a_supplementary_group (run as a non-root user in a devs group)	FAIL gid 30033 vs 30002	pass
in_process_redirect::root_hosted_scan_keeps_the_lock_owner (end-to-end hosted scan as root)	FAIL (0, 0) vs (1000, 1001)	pass

Per-issue checklist:

• Hosted and vendored rewrites replace poetry.lock with a file owned by the user running socket-patch, so after a root scan (e.g. a Docker bind mount) the lock's owner gets "Permission denied" from poetry add / poetry lock #1635: root run keeps the lock's owner (preserving_writes_keep_the_destination_owner, root_hosted_scan_keeps_the_lock_owner); a group-checkout run keeps the group (preserving_write_keeps_a_supplementary_group).

Commands run locally:

• cargo fmt --all -- --check: clean.
• cargo clippy --workspace --all-features -- -D warnings: clean.
• cargo test -p socket-patch-core --all-features --lib: 6536 passed, 4 failed. cargo test -p socket-patch-cli --all-features --lib --test in_process_redirect --test in_process_redirect_pipenv: 3 failed in in_process_redirect. All 7 failures are read-only-directory tests that root ignores. They fail the same way on main as root, and they pass as a non-root user against this branch. The sandbox runs as root.
• The full cargo test --workspace build ran out of the sandbox's disk allowance, so CI runs the rest.

Review notes (/code-review high)

Fixed: the WriteOpts::preserve_mode doc, a root-only test that assumed a new file's group, and the contract's scope wording. Not fixed, on purpose:

• .socket/manifest.json and the vendored ledgers use the plain (non-preserving) writer, and files a root run creates (a new .npmrc, .socket/vendor/ dirs) are root-owned. The issue is about existing project files, and those writers don't preserve the mode either. Possible follow-up: give new files their parent directory's owner, as restore_file_permissions does for patched files.
• A symlinked destination gets its target's owner, the same source the mode already came from. The content comes from that same target, so nothing is exposed to a new owner.
• Failed fchown calls stay silent. Reporting them like OWNERSHIP_NOT_RESTORED_MARKER would need a warning channel through every writer.
• The mode is still set after the fsync. That ordering is older than this PR.

https://claude.ai/code/session_01XwH2o4SRLPwRpbu8QBzz2T

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ 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 1e945c5. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Labeled Ready for review (burn-down agent).

• Head: 1e945c50
• CI: 86/86 green (36 success, 50 skipped); mergeable, no conflicts; no CHANGELOG.md change.
• Bugbot: reviewed 1e945c50, no findings; 0 open review threads.
• Worth a look: keep_owner in crates/socket-patch-core/src/utils/fs.rs chowns the stage before the fsync and before the chmod (chown clears setuid/setgid); the new root-only test root_hosted_scan_keeps_the_lock_owner skips when not run as root.

---

Generated by Claude Code