Skip to content

Fix Go replace target missed in go 1.16 graphs (#1840) - #1843

Open
Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
mainfrom
agent/fix-go-unpruned-replace-target
Open

Mikola Lysenko (mikolalysenko) wants to merge 5 commits into
mainfrom
agent/fix-go-unpruned-replace-target

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 #1840

What was wrong

Take a go 1.16 module (or one with no go directive) where a dependency imports other.com/fork, and the root go.mod has replace other.com/fork => example.com/upstream v1.0.0. A patch for example.com/upstream v1.0.0 was reported as applied. apply --check said it was in sync, and vex attested not_affected. Meanwhile go build linked the unpatched code, because go doesn't chain replaces.

Root cause

replaced_module (crates/socket-patch-core/src/vendor/go_build_list.rs) decides whether a module-to-module replace explains the upstream's go.sum zip line. It only counted the replace when the root go.mod's require listed the fork. In an unpruned graph (go < 1.17), the root go.mod leaves out modules that only a dependency requires. The root's replace still applies to them. Its require versions are also only lower bounds. So go.mod alone can't tell whether the fork is in the build, or at which version.

Fix

  • Read the module graph. When go.mod is unpruned and has a user-authored module-to-module replace, ModuleGraph reads it from GOMODCACHE. It reuses GoWorkspace::read_graph_from, the walker go.work projects already use (as a one-member workspace). The fork's MVS-selected version then decides whether the replace is in effect. That covers a fork only a dependency requires, a fork a dependency raises past go.mod's require, a versioned replace shadowing a version-less one, and a stale replace that go mod tidy kept but nothing uses.
  • One rule for every caller. select_in / selection_in take the graph. All callers use it: project_selection (agent apply, apply --check, CLI vex), the hosted rewriter (through HostedGoWorkspace.module_graph, read by the hosted engine), and VEX lockfile discovery. The graph is only read when the go.mod needs it. The files it reads in the project (root go.mod, directory replacements' go.mod) are declared to the snapshot view, so cached discovery stays valid.
  • Safe fallback. Without a complete graph (the module cache lacks a needed go.mod, or an in-memory view), a replace that may be in effect counts as in effect, so no patch is reported built that isn't. The one exception is when go.mod requires module@version itself: go refuses one module@version under two paths. That guess only refuses a new hosted pin. It never removes an existing one (proves_replace_target). Only a pruned go.mod or a complete graph removes a stale pin.

Tests (red → green)

Issue / case Test Without fix With fix
#1840 repro: fork required only by a dep, versionless and versioned replace e2e_golang_build::fork_required_only_by_a_dependency_is_a_replace_target apply --check exit 0 "Patches are in sync" (build prints PRISTINE) refuses / exit 1 / not attested
#1840 unit: transitive fork in go 1.16 / no directive go_build_list::tests::unpruned_replace_of_a_transitive_fork_is_a_replace_target fails (Selected) passes
Stale fork replace must not block a working patch (review finding) e2e_golang_build::stale_fork_replace_keeps_a_transitive_patch fails on the first (guess-only) commit: apply refused apply 0, build PATCHED, check 0
Graph settles raised/shadowed/stale forks; incomplete graph falls back safely go_build_list::tests::unpruned_module_graph_settles_the_fork_replace, unpruned_root_require_of_another_version_keeps_the_fork_replace, only_a_pruned_go_mod_or_a_complete_graph_proves_a_replace_target, module_graph_is_needed_only_for_an_unpruned_fork_replace n/a (new API) pass
Graph walk reaches a dep-only fork go_workspace::tests::module_graph_reaches_a_fork_only_a_dependency_requires n/a pass
Hosted rewriter uses the graph; an unproven guess keeps an existing pin redirect::tests::golang_go116_module_graph_decides_the_fork_replace, golang_go116_suspected_replace_target_keeps_the_hosted_pin n/a pass

One existing assertion changed meaning. In inactive_replace_target_keeps_the_zip_as_evidence, the go 1.16 cases "fork not required" and "version-less replace shadowed at go.mod's required version" encoded the bug: go.mod alone can't show the fork is absent, or that a dependency didn't raise it. They now run only for go 1.21. The go 1.16 behavior is covered with and without a graph in unpruned_module_graph_settles_the_fork_replace.

Commands run locally (Linux, go 1.24.7):

  • cargo fmt --all -- --check: clean
  • cargo clippy --workspace --all-features -- -D warnings: clean
  • cargo test -p socket-patch-core --all-features: 6999 passed, 4 failed. The 4 failures are chmod-based write-failure tests (copy_tree, vlt_heal, pypi_poetry, pypi_requirements) that can't fail as root (this container runs as uid 0). They're unrelated to Go, and none of their modules is touched by this PR.
  • cargo test -p socket-patch-cli --all-features --lib plus every Go-touching suite with SOCKET_PATCH_GO_E2E_REQUIRED=1: e2e_golang_build (33), e2e_golang_hosted_build (25), e2e_golang_workspace_build (31), e2e_vendor_golang_build (44), e2e_golang, e2e_golang_hosted_state, e2e_vex, e2e_vex_redirect, e2e_vex_vendor, ecosystem_dispatch_e2e, in_process_*, cli_apply_silent: all pass.
  • The full cargo test --workspace (189 CLI test binaries) doesn't fit this container's disk, so CI covers the rest.

/code-review high findings not fixed

  • Module-cache reads aren't in the snapshot read set. If the cache gains a missing go.mod between two passes of one command, a reused discovery keeps its safe verdict ("not built") until the next run. Cached module versions are immutable, and the stale answer is always the conservative one.
  • exclude directives aren't applied by the graph walker. That's the same pre-existing limitation as go.work mode. An excluded fork version can only make a patch refused, never falsely attested.
  • "Graph rebuilt per patched module" in project_selection: the graph is read only for a module that an unpruned go.mod's fork replace targets (needs_module_graph), not for every patch.
  • let in_effect = …; in_effect in replaced_module is required: the iterator borrows entries, a local, so it can't be the tail expression (E0597). Clippy is clean.

Out of scope: GoWorkspace::selection never returns ReplaceTarget, so the #1823/#1840 shapes inside a go.work workspace aren't detected. That's pre-existing and not part of #1840.

https://claude.ai/code/session_011f9aeNyyce3TLFE4vQrpsJ


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
In a go 1.16 module, when a dependency imports a fork that the root
go.mod replaces with the patched upstream module, apply reported the
patch applied, apply --check said in sync and vex attested
not_affected, while go build linked the unpatched code.

The root go.mod of an unpruned graph does not list modules that only
a dependency requires, but its replace still applies to them. The
replace-target check only trusted a fork listed in the root require,
so it missed this shape. Now, in an unpruned graph, such a replace
counts as in effect unless go.mod requires the patched module itself.

Fixes #1840

Assisted-by: Claude Code:claude-opus-5-5
The first fix guessed that a fork replace in a go 1.16 module is in
effect whenever go.mod doesn't list the fork. That refused working
patches when go mod tidy kept a replace nothing uses, and the hosted
rewriter then removed a working Socket replace. It also missed a fork
a dependency raises to the replaced version.

Now, when an unpruned go.mod has a module-to-module replace, the
module graph is read from the module cache, the same walk go.work
projects use, and the fork's selected version decides. apply,
apply --check, the hosted rewriter and VEX discovery all use it. When
the cache lacks go.mod files, the safe answer stays: the patch is not
reported built.

Refs #1840

Assisted-by: Claude Code:claude-opus-5-5
Reading the go 1.16 module graph went around the project view, which
ended read recording for every project: VEX discovery asked for the
raw disk root even with no Go files, so discovery could no longer be
reused between passes. Now the graph is read only when go.mod needs
it, and the root go.mod and any directory replacement's go.mod are
declared as reads, so a change to them still invalidates the cache.

Refs #1840

Assisted-by: Claude Code:claude-opus-5-5
Without every go.mod of a go 1.16 module graph in the module cache,
a fork replace is only suspected of bypassing the Socket pin. That
guess now refuses a new pin but no longer removes an existing one
that may be working. A partial graph is no longer trusted as proof
that the fork is at a lower version. Socket's own hosted replaces no
longer trigger a graph walk.

Refs #1840

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

Burn-down agent: labeled Ready for review at 4e9eebf6171c934fc22bd3f3672e94156ff33cd4.

  • CI: 32/32 run checks green (50 skipped), no failures.
  • Bugbot: reviewed 4e9eebf with no findings; no open review threads.
  • CHANGELOG.md: untouched.
  • For the reviewer: the core change is replaced_module / ModuleGraph in crates/socket-patch-core/src/vendor/go_build_list.rs — unpruned (go < 1.17) go.mods now read the module graph from GOMODCACHE to decide whether a fork replace is in effect, falling back to "in effect" when the graph is incomplete. One existing test's go 1.16 cases changed meaning (see the PR body).

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does. In a go 1.16 (unpruned) module, a root replace other.com/fork => example.com/upstream v1.0.0 applies even when only a dependency requires the fork, so go build links the fork path and our patch for example.com/upstream never lands. We used to report that patch as applied/in sync/not_affected. Now, for unpruned go.mods with a module-to-module replace, we read the module graph from GOMODCACHE to see whether the fork is really selected. If the graph is incomplete, we treat the replace as in effect, so we refuse rather than falsely attest.

Risk: medium. It changes the selection rule that apply, apply --check, vex and the hosted rewriter share. The only behavior change is for go < 1.17 projects with a fork replace, and every fallback errs toward "not built". The graph is only read in that case, so other projects pay nothing.

Look here

Verified

  • Read the full non-test diff (go_build_list, go_workspace, redirect, hosted engine, vex discovery). All three callers go through select_in with the same graph.
  • Checked that Loaded::None plus module_graph is handled downstream like the existing modfile-only case (selection and host_conflict are no-ops).
  • Checked that a missing GOMODCACHE gives an incomplete graph whenever the root requires anything, so the fallback is the conservative one.
  • Windows: module_graph_reads normalizes \\ to / and joins onto the root.
  • CI 32/32 green on 4e9eebf. Bugbot reviewed 4e9eebf with no findings. No open threads. CHANGELOG.md untouched.

Changes I made: none.

Open questions: none blocking. The PR body lists known non-goals: exclude directives, module-cache reads outside the snapshot read set, and go.work workspaces not detecting this shape.

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


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

[agent] Heads-up: #1893 (fix for #1862) is stacked on this branch; it reuses the ModuleGraph reader added here. Please merge this one first.


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