From 651294bfde2c19301c8b61bd22cda1dd0e45f352 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Wed, 15 Jul 2026 12:04:11 +0200 Subject: [PATCH 1/2] refactor(resolvers): resolve TextMarshaler via a source-checked interface Replace the go/types builder tower (NewInterfaceType/NewFunc/NewSignatureType) introduced by #60 with mustIfaceFromSource, which type-checks a one-line `type T interface{ MarshalText() ([]byte, error) }` snippet under a nil importer. The snippet imports nothing (its result types []byte and error are Universe types), so the type-checker never consults an importer: same zero-runtime- resolution guarantee as the synthesized interface, but readable and extensible to other stdlib interfaces (json.Marshaler, fmt.Stringer) by editing a string. Guard the scope lookup so a snippet that type-checks but declares no interface T panics wrapping ErrInternal instead of nil-dereferencing, matching the sibling Must* assertions. Cover the helper contract with TestMustIfaceFromSource. Also drop a stray empty doc-comment line before `package godoclink` (gofmt). Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Frederic BIDON --- internal/builders/godoclink/godoclink.go | 1 - internal/builders/resolvers/assertions.go | 69 +++++++++++++------ internal/builders/resolvers/resolvers_test.go | 46 +++++++++++++ 3 files changed, 93 insertions(+), 23 deletions(-) diff --git a/internal/builders/godoclink/godoclink.go b/internal/builders/godoclink/godoclink.go index 9736de8..4c22521 100644 --- a/internal/builders/godoclink/godoclink.go +++ b/internal/builders/godoclink/godoclink.go @@ -28,7 +28,6 @@ // github.com/fredbi/go-fred-mcp/pkg/doc-filters/godoc-filter; the key difference is that this // package *rewrites* the prose whereas that tool *redacts* (length-preserving blanking) for // masking. -// package godoclink import ( diff --git a/internal/builders/resolvers/assertions.go b/internal/builders/resolvers/assertions.go index 3f7d5e1..0df6e57 100644 --- a/internal/builders/resolvers/assertions.go +++ b/internal/builders/resolvers/assertions.go @@ -6,6 +6,7 @@ package resolvers import ( "fmt" "go/ast" + "go/parser" "go/token" "go/types" @@ -75,31 +76,55 @@ func IsFieldStringable(tpe ast.Expr) bool { return false } -// textMarshalerIface is the encoding.TextMarshaler interface, synthesized with go/types: +// textMarshalerIface is the encoding.TextMarshaler interface: // // interface{ MarshalText() (text []byte, err error) } // -// It is built structurally rather than by importing the real "encoding" package through -// go/importer's default importer. That importer resolves stdlib export data by running -// "go list -export" against the GOROOT the binary was built against. When GOTOOLCHAIN -// selects a different toolchain at runtime — or the binary simply runs on a machine whose Go -// installation lives at a different path than the build machine's — that invocation fails -// ("cannot find main module"), the error is silently swallowed, and every TextMarshaler check -// returns false. types.Implements compares method sets structurally, so a synthesized, -// structurally identical interface is equivalent to the imported one and removes the runtime -// dependency on a working "go list". -var textMarshalerIface = types.NewInterfaceType([]*types.Func{ //nolint:gochecknoglobals // immutable synthesized interface, built once, read-only - types.NewFunc(token.NoPos, nil, "MarshalText", - types.NewSignatureType(nil, nil, nil, - nil, - types.NewTuple( - types.NewVar(token.NoPos, nil, "text", types.NewSlice(types.Typ[types.Byte])), - types.NewVar(token.NoPos, nil, "err", types.Universe.Lookup("error").Type()), - ), - false, - ), - ), -}, nil).Complete() +// It is resolved by type-checking a one-line source snippet rather than by importing the real +// "encoding" package through go/importer's default importer. +// +// The importer reads stdlib export data out of the GOROOT the binary was built against: +// when GOTOOLCHAIN selects a different toolchain at runtime — or the callgin binary simply runs on a machine where +// Go installation lives at a different path than the build machine's — the lookup fails, and the error is silently +// swallowed: every TextMarshaler check returns false. +// +// This snippet imports nothing (the result types []byte and error are Universe types), so the type-checker never +// consults an importer. The construction has no runtime dependency on a working toolchain or GOROOT. +// +// types.Implements compares method sets structurally: the resulting interface is equivalent to the one the "encoding" package supplies. +var textMarshalerIface = mustIfaceFromSource( //nolint:gochecknoglobals // immutable, built once, read-only + `package p; type T interface{ MarshalText() (text []byte, err error) }`, +) + +// mustIfaceFromSource type-checks src: it must declare a single interface type T that imports nothing. +// +// It returns its underlying *types.Interface. It panics on malformed input: src is a compile-time constant +// (a failure is a programming error never a runtime condition). +func mustIfaceFromSource(src string) *types.Interface { + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "iface.go", src, 0) + if err != nil { + panic(fmt.Errorf("parsing synthetic interface source %q: %w: %w", src, err, ErrInternal)) + } + + // nil importer: the snippet imports nothing, so the type-checker never invokes it. + pkg, err := new(types.Config).Check("p", fset, []*ast.File{f}, nil) + if err != nil { + panic(fmt.Errorf("type-checking synthetic interface source %q: %w: %w", src, err, ErrInternal)) + } + + obj := pkg.Scope().Lookup("T") + if obj == nil { + panic(fmt.Errorf("synthetic interface source %q does not declare a type T: %w", src, ErrInternal)) + } + + iface, ok := obj.Type().Underlying().(*types.Interface) + if !ok { + panic(fmt.Errorf("synthetic interface source %q did not declare an interface T: %w", src, ErrInternal)) + } + + return iface +} func IsTextMarshaler(tpe types.Type) bool { return types.Implements(tpe, textMarshalerIface) diff --git a/internal/builders/resolvers/resolvers_test.go b/internal/builders/resolvers/resolvers_test.go index 38d5a48..e8d7f9c 100644 --- a/internal/builders/resolvers/resolvers_test.go +++ b/internal/builders/resolvers/resolvers_test.go @@ -480,5 +480,51 @@ func TestInternalError_Error(t *testing.T) { assert.EqualT(t, string(ErrInternal), ErrInternal.Error()) } +func TestMustIfaceFromSource(t *testing.T) { + // mustIfaceFromSource is the toolchain-independent replacement for go/importer used to + // materialize the encoding.TextMarshaler interface. Its contract: a valid one-line source + // declaring an interface type T returns that interface; any deviation is a programming error + // (src is a compile-time constant) and must panic wrapping ErrInternal — never resolve an + // importer or nil-deref. + assertPanicsWrappingErrInternal := func(t *testing.T, src string) { + t.Helper() + defer func() { + r := recover() + require.NotNil(t, r, "expected a panic") + err, ok := r.(error) + require.TrueT(t, ok, "panic value should be an error") + require.TrueT(t, errors.Is(err, ErrInternal), "panic should wrap ErrInternal") + }() + _ = mustIfaceFromSource(src) + } + + t.Run("valid interface source returns the interface", func(t *testing.T) { + iface := mustIfaceFromSource(`package p; type T interface{ MarshalText() (text []byte, err error) }`) + require.NotNil(t, iface) + require.EqualT(t, 1, iface.NumMethods()) + require.EqualT(t, "MarshalText", iface.Method(0).Name()) + }) + + t.Run("unparseable source panics wrapping ErrInternal", func(t *testing.T) { + assertPanicsWrappingErrInternal(t, `%% this is not go %%`) + }) + + t.Run("type-check failure panics wrapping ErrInternal", func(t *testing.T) { + // References an undefined type: with a nil importer nothing can resolve it, so the + // type-checker errors rather than reaching out to a toolchain. + assertPanicsWrappingErrInternal(t, `package p; type T interface{ Foo() Undefined }`) + }) + + t.Run("type T that is not an interface panics wrapping ErrInternal", func(t *testing.T) { + assertPanicsWrappingErrInternal(t, `package p; type T struct{}`) + }) + + t.Run("missing type T panics wrapping ErrInternal", func(t *testing.T) { + // Valid, type-checkable source that simply never declares T: the nil scope lookup must be + // guarded into an ErrInternal panic, not an opaque nil dereference. + assertPanicsWrappingErrInternal(t, `package p; type U interface{ Foo() }`) + }) +} + // Assert at compile time that our typable fixture satisfies the interface. var _ ifaces.SwaggerTypable = (*mocks.MockSwaggerTypable)(nil) From d420f3fda10bb89ca50f91c8afeff147a005f206 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Wed, 15 Jul 2026 12:04:22 +0200 Subject: [PATCH 2/2] test(ci): guard TextMarshaler toolchain-independence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a bespoke build-once / run-in-a-foreign-environment CI matrix (.github/workflows/toolchain-independence.yml), separate from the shared go-test workflow, that locks the property #60 fixed: IsTextMarshaler must not depend on a working build-time toolchain/GOROOT at runtime. - job A builds the resolvers + integration test binaries on `stable`. - job B (pinpoint) runs the resolvers binary under `env -i` — no Go present at all — proving the interface resolution resolves nothing at runtime. - job C (full workflow) runs the integration binary on `oldstable` with the build-time GOROOT removed and GOROOT unset, so the old go/importer path would fail while packages.Load still self-locates on PATH. TestToolchainIndependence_FullScan backs job C: a self-check that the baked runtime.GOROOT() is absent (fails loudly on masking) plus a full-scan symptom assertion that TextMarshaler fields still render as strings. It skips unless CODESCAN_TC_JOBC=1, so a normal `go test ./...` is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Frederic BIDON --- .github/workflows/toolchain-independence.yml | 113 ++++++++++++++++++ .../coverage_toolchain_independence_test.go | 83 +++++++++++++ 2 files changed, 196 insertions(+) create mode 100644 .github/workflows/toolchain-independence.yml create mode 100644 internal/integration/coverage_toolchain_independence_test.go diff --git a/.github/workflows/toolchain-independence.yml b/.github/workflows/toolchain-independence.yml new file mode 100644 index 0000000..0ae5713 --- /dev/null +++ b/.github/workflows/toolchain-independence.yml @@ -0,0 +1,113 @@ +# Guards the toolchain-independence of stdlib type detection (encoding.TextMarshaler). +# +# codescan is embedded as a compiled binary in downstream tools (e.g. go-swagger) and then run in a +# possibly-different Go environment than it was built in. resolvers.IsTextMarshaler must NOT depend +# on a working build-time toolchain/GOROOT at runtime. See +# .claude/plans/textmarshaler-toolchain-independence.md. +# +# This is a bespoke build-once / run-in-a-foreign-environment matrix; it is intentionally NOT part +# of the shared reusable go-test workflow. codescan makes no library-wide "runs without Go" pledge — +# the go-less property is local to job B only. +name: toolchain independence + +on: + push: + branches: + - master + pull_request: + +permissions: + contents: read + +jobs: + # Job A — build the test binaries against the STABLE toolchain and publish them as artifacts. + # Nothing is asserted here; the binaries bake in stable's GOROOT (runtime.GOROOT()), which jobs + # B and C then run against a different / absent GOROOT. + build: + runs-on: ubuntu-latest + outputs: + build_goroot: ${{ steps.goroot.outputs.build_goroot }} + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 + with: + go-version: stable + cache: false + - id: goroot + name: Record the build-time GOROOT (baked into the binaries as runtime.GOROOT()) + run: echo "build_goroot=$(go env GOROOT)" >> "$GITHUB_OUTPUT" + - name: Compile the pinpoint (resolvers) and full-workflow (integration) test binaries + run: | + go test -c -o resolvers.test ./internal/builders/resolvers/ + go test -c -o integration.test ./internal/integration/ + - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: toolchain-test-binaries + path: | + resolvers.test + integration.test + if-no-files-found: error + retention-days: 1 + + # Job B — pinpoint, hermetic. Claim: the TextMarshaler *interface resolution* depends on nothing at runtime. + # Deliberately installs NO Go and runs under a scrubbed environment (env -i): no PATH, + # no GOROOT, no toolchain. + # The absence of setup-go IS the test. + # Proven discriminator: this binary passes on the fixed code and fails on the pre-fix go/importer implementation in this same env. + pinpoint-no-go: + needs: build + runs-on: ubuntu-latest + steps: + - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 + with: + name: toolchain-test-binaries + - name: Run resolvers tests with no Go present at all + run: | + chmod +x resolvers.test # upload-artifact does not preserve the executable bit + env -i HOME="$HOME" ./resolvers.test \ + -test.run 'TestIsTextMarshaler|TestMustIfaceFromSource' -test.v + + # Job C — wide, full-workflow. Claim: a complete scan survives a build!=run toolchain divergence and still emits correct output. + # Runs the integration binary (built on stable) against an OLDER toolchain, + # with the build-time GOROOT removed and GOROOT unset, so the old go/importer path would fail + # — while packages.Load still works via the older `go` self-locating on PATH. + full-workflow-divergent: + needs: build + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 # fixtures source (FixturesDir resolves the build-time checkout path, identical across jobs) + # oldstable is a DIFFERENT minor than job A's stable, so its GOROOT lives at a different path. + # + # It must satisfy the fixtures module's `go` directive (currently go 1.25.0); oldstable only ever increases, so this holds going forward. + - uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 + with: + go-version: oldstable + cache: false + - name: Warm the fixtures module cache (normal toolchain, before divergence) + working-directory: fixtures + run: go mod download + - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 + with: + name: toolchain-test-binaries + - name: Remove the build-time GOROOT so the divergence is deterministic + # Hosted-runner images may pre-cache the stable toolchain at the same path the binary baked in. + # + # Deleting it guarantees runtime.GOROOT() points at an absent directory (the condition + # the self-check enforces). Different major than oldstable, so this never touches the + # toolchain packages.Load uses. + run: | + build_goroot='${{ needs.build.outputs.build_goroot }}' + echo "removing build-time GOROOT: $build_goroot" + sudo rm -rf "$build_goroot" + - name: Run the full scan under the divergent environment + # env -u GOROOT: NEVER set GOROOT to a bogus path — that makes the `go` command itself refuse + # to start ("cannot find GOROOT directory") and packages.Load fails for the wrong reason. + # + # Unsetting it lets `go` self-locate from its own binary while the old importer falls back to + # the (now absent) baked GOROOT. GOTOOLCHAIN=local forbids any surprise toolchain download. + run: | + chmod +x integration.test # upload-artifact does not preserve the executable bit + env -u GOROOT \ + GOTOOLCHAIN=local \ + CODESCAN_TC_JOBC=1 \ + ./integration.test -test.run 'TestToolchainIndependence_FullScan' -test.v diff --git a/internal/integration/coverage_toolchain_independence_test.go b/internal/integration/coverage_toolchain_independence_test.go new file mode 100644 index 0000000..c1f2e7b --- /dev/null +++ b/internal/integration/coverage_toolchain_independence_test.go @@ -0,0 +1,83 @@ +// SPDX-FileCopyrightText: Copyright 2015-2025 go-swagger maintainers +// SPDX-License-Identifier: Apache-2.0 + +package integration_test + +import ( + "os" + "runtime" + "testing" + + "github.com/go-openapi/codescan" + "github.com/go-openapi/codescan/internal/scantest" + "github.com/go-openapi/testify/v2/require" +) + +// jobCEnvMarker is set by the bespoke `toolchain-independence` CI job (job C). +// +// It signals that this process is a binary built against one Go toolchain and run against another, +// with GOROOT deliberately pointed away from the build-time install. +// +// See .github/workflows/toolchain-independence.yml. +const jobCEnvMarker = "CODESCAN_TC_JOBC" + +// TestToolchainIndependence_FullScan is the wide, full-workflow guard (job C) for the +// TextMarshaler toolchain-independence fix. +// +// It only does real work when jobCEnvMarker=="1"; under a normal `go test ./...` run it skips, +// because the pre-condition it needs (build-time GOROOT absent at runtime) cannot be arranged +// in-process. When the marker is set it enforces two things in order: +// +// 1. Self-check: the GOROOT baked into this binary at build time (runtime.GOROOT()) does NOT +// exist at runtime. This is what makes a green result meaningful — under the old go/importer +// implementation, a scan in exactly this environment produced {type: object} for every +// TextMarshaler type. If the baked path still exists (e.g. the CI runner pre-cached the build +// toolchain at the same path), the divergence never happened and the test would be a false +// green, so we fail loudly rather than skip. +// +// 2. Symptom: a full codescan.Run over the text-marshal fixture — which drives +// packages.Load -> schema.go -> resolvers.IsTextMarshaler end to end — still renders the +// TextMarshaler-bearing fields as strings. +func TestToolchainIndependence_FullScan(t *testing.T) { + if os.Getenv(jobCEnvMarker) != "1" { + t.Skipf("%s!=1: skipping the toolchain-divergence guard (only meaningful under CI job C)", jobCEnvMarker) + } + + t.Run("self-check: build-time GOROOT is absent at runtime", func(t *testing.T) { + // runtime.GOROOT() is deprecated exactly because it returns the (stale) build-time root when GOROOT is unset. + // This is "not meaningful if the binary is copied to another machine". + // That stale value is precisely the signal this guard needs: it is the path the old go/importer consulted, + // and its absence at runtime is what reproduces the bug's environment. + baked := runtime.GOROOT() //nolint:staticcheck // intentionally reading the stale build-time GOROOT to prove build!=run divergence + t.Logf("baked runtime.GOROOT()=%q; runtime GOROOT env=%q", baked, os.Getenv("GOROOT")) + require.NotEmpty(t, baked, "binary has no baked GOROOT — cannot assert divergence") + + _, err := os.Stat(baked) + require.TrueT(t, os.IsNotExist(err), + "build-time GOROOT %q still exists at runtime: the build!=run divergence did not happen, "+ + "so this job cannot distinguish the fix from the bug (masking). Ensure job C removes the "+ + "build toolchain and unsets GOROOT before running.", baked) + }) + + t.Run("symptom: TextMarshaler fields still render as strings", func(t *testing.T) { + doc, err := codescan.Run(&codescan.Options{ + Packages: []string{"./enhancements/text-marshal/..."}, + WorkDir: scantest.FixturesDir(), + ScanModels: true, + }) + require.NoError(t, err, "full scan must still succeed (packages.Load uses the runtime toolchain)") + require.NotNil(t, doc) + + device, ok := doc.Definitions["Device"] + require.TrueT(t, ok, "Device definition should be discovered") + + // All three fields are TextMarshaler-implementing types; each must be a string, not object. + for _, field := range []string{"id", "mac", "data"} { + prop, ok := device.Properties[field] + require.TrueT(t, ok, "Device.%s should be present", field) + require.TrueT(t, prop.Type.Contains("string"), + "Device.%s must be type:string (TextMarshaler), got %v — the toolchain-independent "+ + "IsTextMarshaler regressed", field, prop.Type) + } + }) +}