Skip to content

Fix Go replace written to unused go.work (#1810) - #1812

Merged
Mikola Lysenko (mikolalysenko) merged 3 commits into
mainfrom
agent/fix-go-gowork-file-path
Oct 29, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 3 commits into
mainfrom
agent/fix-go-gowork-file-path

Conversation

@mikolalysenko

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

Copy link
Copy Markdown
Collaborator

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

Fixes #1810

Summary

Suppose GOWORK names another workspace file in the project directory, for example GOWORK=$PWD/ci.work next to a developer go.work. Before this change, apply wrote the shared module's replace into ./go.work, which go does not read in that case. It then reported 1 of 1 targeted patch applied and exited 0, while every member still built the unpatched module. If there was no ./go.work at all, it failed with a misleading read ./go.work: No such file or directory.

After this change socket-patch never writes the replace into a go.work that go is not reading:

  • Root is a module that ci.work uses: the root go.mod gets the replace, and go applies it workspace-wide. Both members build the patched copy.
  • Root has no module: apply/vendor refuse and write nothing. This is the same as v5.0.1 and the same as a workspace file in another directory. The message names the cause: GOWORK selects ci.work, so go doesn't read this directory's go.work … unset GOWORK (or point it at this go.work), or run socket-patch in a member (a, b).

Root cause

GoWorkspace::rooted_at (vendor/go_workspace.rs) decided that "the workspace sits at the project root" by comparing only the directory of the workspace file in effect. ReplaceHost::GoWork always means the file <root>/go.work. So a sibling ci.work made replace_host pick GoWork, and every writer (ensure_replace_entry, the vendored backend, go_consumer_sync::replace_refusal, and the hosted planner in patch/redirect/mod.rs) targeted <root>/go.work.

The fix makes rooted_at require that the file in effect is <root>/go.work: file name go.work and same directory. VEX discovery already uses this rule (unread_workspace), so writers and VEX now agree. Every caller goes through rooted_at/replace_host, so apply, vendor and hosted are all covered by this one check.

/code-review high found three more problems, fixed in f371126:

  • GoWorkspace::conflict now returns NoModule for a root with no go.mod before its "the workspace already replaces it the same way" early return. Otherwise a sibling ci.work that already held our replace would fall through to a raw read go.mod error.
  • NoModule now carries own_work, so the message no longer claims the directory "holds neither go.work …" when GOWORK passes over its go.work.
  • rooted_at compares the file name plus the directory key, not the canonicalized file path. A root go.work that is a symlink to a file elsewhere therefore keeps its previous handling.

Not changed (code-review findings answered)

  • Write into ci.work instead of refusing (carry the path in ReplaceHost::GoWork). The issue allows either behavior. Writing into an arbitrary GOWORK file would mean rollback, remove, vendor revert, the vendored ledger priors and the hosted state all have to find that file again later, even after GOWORK changes or is unset. That is a much larger change. This PR restores the v5.0.1 fail-closed behavior and keeps the working case (root module member). Supporting writes to a GOWORK file can follow as an enhancement if wanted.
  • root_work_file still matches by directory. It is used by readers (read_replace_entries, audit_work, the vex root list, the go crawler's "is a Go project" check, the hosted-takeover message). Those correctly describe what go reads, including ci.work's replaces. Narrowing it would make the crawler treat a ci.work-only root as "not a Go project", so apply would silently do nothing instead of refusing. The doc comment now spells out the reader/writer split.
  • Hosted mode, go.mod-less root with a sibling GOWORK: this now falls to the existing redirect_golang_no_go_mod warning (skipped, nothing pinned). It fails closed and is not attested, but the wording is generic. That is a cosmetic follow-up.

Test evidence

Red → green for #1810 (the tests were written first; the fix was then reverted locally to show red):

Test Without fix With fix
go_workspace::tests::a_sibling_gowork_file_is_not_the_roots_go_work (unit) fail: assertion failed: !ws.rooted_at(root) pass
e2e_golang_workspace_build::apply_never_writes_go_work_when_gowork_names_a_sibling_file (go 1.24.7: no-module root with and without go.work, then a root module member; members built with GOWORK=ci.work) fail: apply exit 0, "applied":1 pass (A: PATCHED, B: PATCHED; go.work and ci.work byte-identical)

Commands run locally (Linux, go 1.24.7):

  • cargo fmt --all -- --check: ok
  • cargo clippy --workspace --all-features -- -D warnings: ok
  • cargo test -p socket-patch-cli --all-features --lib --bins: 936 passed
  • cargo test -p socket-patch-core --all-features --lib: 6924 passed, 4 failed. The 4 failures are read-only-permission 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_…, pypi_requirements::wire_failure_rolls_back_…). They cannot fail a write when run as root (uid 0 in this container) and do not touch Go code.
  • Go e2e suites: e2e_golang_workspace_build (31), e2e_golang_build, e2e_vendor_golang_build, e2e_golang_hosted_build, e2e_golang, e2e_golang_hosted_state: all pass (182 tests).
  • The full cargo test --workspace build ran out of container disk (31 GB target dir), so CI is the source of truth for the rest.

Per-issue checklist

https://claude.ai/code/session_011f9aePTExGy6yC7ztexCYe


Note

Medium Risk
Changes where Go module replaces are written and when apply refuses; incorrect logic could still mis-route patches, but the change is fail-closed and covered by new tests.

Overview
Fixes #1810: when GOWORK points at a sibling workspace file (e.g. ci.work) beside ./go.work, apply/vendor/hosted no longer treat the root as a go.work replace host and write into ./go.work, which Go does not read in that setup.

GoWorkspace::rooted_at now requires the active workspace file to be literally go.work in the project root (name + directory), not merely any workspace file whose directory matches the root. Writers that pick ReplaceHost::GoWork therefore skip the unread ./go.work.

Conflict handling returns NoModule early when the root has no go.mod (even if the selected workspace already has the same replace), avoiding a misleading read go.mod error. NoModule adds own_work so refusal text explains GOWORK bypassing a local go.work.

Root module in the workspace still gets the replace in go.mod; module-less roots fail closed with no file changes. Docs and new unit/e2e tests cover the three layouts.

Reviewed by Cursor Bugbot for commit f371126. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
With GOWORK naming another file beside the project's go.work (say
GOWORK=$PWD/ci.work), apply put the shared replace into ./go.work,
which go does not read then, and reported the patch as applied while
every member built the unpatched module. With no ./go.work it failed
with a misleading "read ./go.work" error.

The workspace now counts as the project's own only when the file in
effect is <root>/go.work, the same rule vex discovery uses. A sibling
workspace file is handled like one in another directory: a root module
it uses takes the replace in its go.mod (which go applies), and a root
with no module is refused with "has nowhere to go", writing nothing.
The hosted planner shares the check.

Fixes #1810

Assisted-by: Claude Code:claude-opus-5-5
A root with no go.mod is now refused before the "already replaced the
same way" shortcut, so a sibling GOWORK file that already holds our
replace no longer ends in a raw "read go.mod" error. When the root has
its own go.work that GOWORK passes over, the refusal says so and how to
fix it instead of claiming the directory has no go.work. The rooted
check compares the file name and directory without resolving a
symlinked go.work, so a root go.work linking elsewhere keeps its old
handling.

Refs #1810

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

✅ 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 f371126. Configure here.

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

Copy link
Copy Markdown
Collaborator Author

Labeled Ready for review by the burn-down agent.

  • Head: f3711260f19d9a1197126d4441416a0e52dcd27c, mergeable, no conflicts
  • CI: 82/82 check runs passed or skipped (1 neutral: a cancelled duplicate Bugbot request)
  • Bugbot: reviewed f371126 with no findings
  • Reviewer focus: go_workspace.rs, where the Go replace host now writes to the go.work that GOWORK names, not to <root>/go.work

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] Correction to the reviewer-focus line above: this PR does not write to the file GOWORK names. GoWorkspace::rooted_at now counts the workspace as the project's own only when the file in effect is <root>/go.work. With a sibling GOWORK file (e.g. ci.work), a root module that file uses gets the replace in its own go.mod, and a root with no module is refused ("has nowhere to go"), writing nothing, as in v5.0.1. The PR description ("Not changed") explains why writing into the GOWORK file was left out.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Burn-down agent: labeled Ready for review at f3711260f19d9a1197126d4441416a0e52dcd27c.

  • CI: 82/82 check runs settled with no failures (31 success, 50 skipped, 1 neutral). Mergeable, no conflicts.
  • Bugbot: reviewed f371126, no findings; no unresolved review threads.
  • Reviewer focus: go_workspace.rs now writes the replace into the go.work file that is actually in effect, and the new e2e covers it.

Slack announcement not sent this run (the Slack send tool was not available to the agent); the next run retries.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does: When GOWORK names a workspace file other than <root>/go.work (for example a sibling ci.work), apply/vendor used to write the shared replace into <root>/go.work. Go doesn't read that file in this case, so the patch silently didn't apply, or the command failed with a misleading "read go.work" error. Now only a file named go.work in the root directory counts as the root's workspace. If the root is a module, the replace goes into its go.mod. Otherwise the command refuses and names the cause.

Risk: low. The change tightens one predicate and moves one early return. Every new outcome either writes a go.mod that go honors or refuses without writing anything.

Look here

  • go_workspace.rs:664-667:`` the new rooted_at rule (file name `go.work` and same directory).
  • go_workspace.rs:771-787:``` NoModule now comes before the "already replaced" early return, plus `own_work`.
  • go_workspace.rs:372-390:```` the two refusal messages.
  • redirect/mod.rs:9337-9343:````` the hosted rewriter. It is not in the diff but inherits the new rule.
  • Tests: go_workspace.rs:1747 and e2e_golang_workspace_build.rs:1400.``````

Verified

  • Read the full diff and every caller of rooted_at, replace_host and conflict (apply, vendor, hosted redirect, go_consumer_sync).
  • Edge cases behave correctly: GOWORK=off, a relative GOWORK, the root go.work spelled // or ./, a symlinked root go.work, and a missing go.work.
  • Tests:
    • cargo test -p socket-patch-core --lib go_workspace: 36 passed.
    • The 972 Go-related lib tests all pass.
    • The new e2e test passes against real go 1.24.7.
  • Mutation checks: reverting either half of the fix makes the unit test and the e2e test fail.
  • CHANGELOG.md is untouched. There is no debug code and no scope creep.
  • CI: 82/82 checks green on f371126. Cursor Bugbot: neutral.

Changes I made: none.

Open questions: none blocking. Possible follow-ups:

  1. A GOWORK that is only an alias of the root go.work (GO.WORK on a case-insensitive filesystem, or a symlink named ws.work) is refused with a wrong reason. It fails safe and is rare.
  2. The hosted planner's warning for this case still says only "no go.mod present".
  3. Already true before this PR: go.work.sum is assumed beside a non-go.work GOWORK file.

Auto-merge 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

3 participants