Skip to content

Fix hosted scan from a workspace member pinning nothing or the wrong files (#590, #417) - #598

Open
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
mainfrom
agent/fix-hosted-ancestor-workspace-lock
Open

Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
mainfrom
agent/fix-hosted-ancestor-workspace-lock

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

Final-head CI is complete: 479 successful checks, 6 skipped; no failures or pending checks. Bugbot passed, there are no unresolved review threads, and the PR is mergeable.

Fixes #590 and #417. Hosted scan and get run from a pnpm or Cargo workspace member can otherwise report success while reading only the member directory: pnpm pins nothing, and Cargo may rewrite member manifests as a lockless project, breaking workspace builds.

The hosted path now refuses these layouts before takeover or writes, including dry runs. Cargo shares the existing vendored workspace-root check and reports cargo_manifest_not_workspace_root. A pnpm candidate with no local npm-family lock reports redirect_pnpm_lockfile_elsewhere when its governing lock exists elsewhere, naming that lock and returning exit 1.

pnpm resolution follows native configuration controls: the nearest workspace YAML takes precedence over member .npmrc, which takes precedence over root .npmrc. Configured relative lockfileDir/lockfile-dir paths resolve from the invocation directory; without an override, the lock is sought at the workspace root. Existing local locks and Rush bypass this ancestor check, and the nearest workspace bounds lookup. The disk check is shared by hosted scan/get; in-memory projects have no ancestor directory to inspect.

Validation:

  • Eight governing-root unit tests, all 16 pnpm CLI tests, and the Cargo member refusal test passed after the correction.
  • Three CLI regressions fail on the original PR head and pass with the fix: inherited root .npmrc, inherited relative YAML directory, and YAML precedence over member .npmrc. Refused runs preserve project and lock bytes.
  • Native pnpm 10.34.5 offline installs verified both setting sources, relative-path behavior, and the precedence combinations.
  • Independent review checked the final correction against the native evidence. Diff and targeted lint checks passed (with the existing macOS unused_variables allowance), and the fixed commit merges cleanly with current main.
  • Full CI, compatibility workflows, benchmarks, and Bugbot completed successfully on the corrected commit 93c3e32b.

Other package managers' workspace-member rules remain outside this pnpm/Cargo change.


Note

Medium Risk
Changes hosted-mode entry preconditions and ancestor filesystem inspection for pnpm lock resolution; incorrect detection could block valid runs or still miss edge layouts, but refused runs are non-destructive.

Overview
Hosted scan and get --mode hosted now fail closed when --cwd is a workspace member whose real lock or Cargo workspace root lives elsewhere, instead of exiting success with no pin or rewriting the wrong manifests.

A new governing_root pre-check runs on disk projects before takeover or any writes (including --dry-run). pnpm npm candidates with no local npm-family lock get redirect_pnpm_lockfile_elsewhere, resolving the governing pnpm-lock.yaml using pnpm-style precedence (pnpm-workspace.yaml lockfileDir, then member/root .npmrc, relative paths from invocation cwd). Cargo candidates reuse the vendored cargo_manifest_not_workspace_root check with a hosted-specific message. Runs exit 1, write nothing, and tell the user which directory to use; workspace root runs still pin normally.

Docs (CHANGELOG, CLI_CONTRACT) and broad unit/CLI regression tests cover workspace members, lockfile-dir, inherited config, and the Cargo member case (#590, #417).

Reviewed by Cursor Bugbot for commit 93c3e32. Configure here.

Assisted-by: Claude Code:claude-opus-5-5
Hosted scan and get read locks only in --cwd. Run from a pnpm
workspace member (or a project whose lockfile-dir puts pnpm-lock.yaml
elsewhere), they pinned nothing and still reported success, so pnpm
kept installing the unpatched package (#590). Run from a cargo
workspace member, they rewrote the member as a lockless project and
broke every build of the workspace (#417).

Both layouts are now refused before any takeover or write, exit 1,
naming the directory to run from: redirect_pnpm_lockfile_elsewhere
for pnpm, and the vendored cargo_manifest_not_workspace_root check,
now shared, for cargo.

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

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] CI note: native (ubuntu-latest, 1.4.2) (Bun compatibility) failed 51/52 on 70f46f8.

  • The failing cell: 1.4.2 crlf hosted, on the rollbackSucceeded / rollbackOriginalFiles / rollbackOriginalBytes checks.
  • Why it isn't this PR's: this PR only adds a pre-check to hosted scan / get for a cwd that is a pnpm or cargo workspace member. It doesn't touch rollback, Bun locks, or line-ending handling. The crlf cell runs from a project root with its own bun.lock, so the new check is a no-op there. In the same job, 12 other cells failed on patches-api.socket.dev Connection reset by peer and passed on their transport retry. The rollback step re-resolves upstream over the network, and its failure text doesn't match the harness's retry pattern, so that cell wasn't retried.
  • Fix to port: none exists. Fix npm/Bun VEX attesting a patch a same-lock copy skips (#588) #589 hit the same job, failing on the same patches-api connection resets.
  • Next step: I'll re-run the failed job once the workflow finishes; the API refuses a re-run while other jobs are still running. If it fails again I'll treat it as real and dig into the rollback artifact.

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-core/src/hosted/governing_root.rs
Comment thread crates/socket-patch-core/src/hosted/governing_root.rs
A workspace root can move pnpm-lock.yaml with lockfileDir, and the
key may be written quoted in pnpm-workspace.yaml. Hosted runs from a
member of such a workspace, or of one with a quoted key, still
reported success while pinning nothing. Both are now refused like
any other member whose lock lives elsewhere.

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.

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

Copy link
Copy Markdown
Collaborator Author

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

  • CI: 469/469 completed check runs on the head pass (463 success, 6 skipped by matrix), no failing commit statuses.
  • Bugbot: 2 findings on 70f46f8 (quoted lockfileDir keys; a workspace root that relocates its lock) were fixed in afce769. Bugbot's re-review of afce769 found no new issues. 0 unresolved review threads.
  • Mergeability: no conflicts. The branch is 1 commit behind main (045d7ec, Bound patch API connects and stalled reads (#570) #581 patch API timeouts). That commit only overlaps this PR in CHANGELOG.md, and GitHub still reports the PR as cleanly mergeable.
  • Reviewer focus: hosted/governing_root.rs, which decides pnpm lock discovery precedence (lockfile-dir / lockfileDir, then the nearest ancestor pnpm-workspace.yaml), and the new pub(crate) workspace_root_refusal reuse in vendor/cargo.rs.

Generated by Claude Code

@mikolalysenko

Mikola Lysenko (mikolalysenko) commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator Author

Review updated for 93c3e32bee97ae4041b627cee3a295b1d1a6e249: Ready to merge as-is from this review. Final-head CI is complete: 479 successful checks, 6 skipped; no failures or pending checks. Bugbot passed, there are no unresolved review threads, and the PR is mergeable.

The original guard missed root .npmrc settings and resolved inherited YAML paths from the wrong directory. Full CLI regressions reproduced exit 0, status: success, and zero redirects while native pnpm used an external lock.

The correction reads the nearest workspace's settings with native precedence (YAML, then member .npmrc, then root .npmrc) and resolves configured relative paths from the invocation directory. Ordinary workspace locks are still located at the workspace root when no override applies.

Validation passed: eight governing-root tests, all 16 pnpm CLI tests, and the Cargo member refusal test. Three new CLI regressions fail on the original head and pass with the fix. Native pnpm 10.34.5 offline installs verify inherited settings, path resolution, and precedence. Independent review of the final correction found no remaining issue; it merges cleanly with current main.

No remaining code finding from this review. The Ready label has been restored after all checks completed on the corrected commit.

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

Copy link
Copy Markdown
Collaborator Author

bugbot run

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

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

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hosted scan/get run from a pnpm workspace member (or with lockfile-dir=..) ignores the parent pnpm-lock.yaml and reports success while pinning nothing

2 participants