refactor: thread ctx through Compile and GetReader by robbyt · Pull Request #144 · robbyt/go-polyscript

robbyt · 2026-05-23T18:30:00Z

Summary

Compiler.Compile and Loader.GetReader now take a context.Context as the first argument. NewExecutableUnit gains a leading ctx too, threading the same context into the loader's IO and the compiler's parser/compile.

The driving goal: stop storing external context.Context objects as struct fields. The Extism compiler's ctx context.Context field is removed; ctx is now a method parameter that flows through and is never retained.

What's removed

Extism compiler's WithContext option and the ctx field on its struct. (The anti-pattern this PR was about.)
Risor inner compile's hard-coded context.Background() — caller cancellation now actually reaches the parser.
FromHTTP loader's GetReaderWithContext helper — now redundant, the body folds into GetReader(ctx).

What's added

A ctx context.Context parameter on:

script.Compiler.Compile (and all three engine implementations)
script/loader.Loader.GetReader (and all five loader implementations)
script.NewExecutableUnit (threads to both)

The three public FromXxxLoader constructors keep their signatures and pass context.Background() to NewExecutableUnit inline at the call site — they're one-shot startup constructors where that's the right default. Callers needing cancellable compile drive script.NewExecutableUnit directly.

Files

7 production files: 2 interfaces, 3 engine compilers, 1 Risor inner compile, ExecutableUnit
5 loader implementations
3 engine new.go constructors (pass context.Background() inline)
~25 test files: mocks updated, ~90 call sites updated, removed WithContext test cases
CHANGELOG.md — [Unreleased] > Changed entry

Net diff: -80 lines (261 insertions, 341 deletions across 44 files).

Closes #89.

Test plan

go build ./... clean
go vet ./... clean
go test -race -count=1 ./... full suite green
No ctx context.Context struct fields in production code (only method/interface params)
No context.Background() hidden in the compile/loader path — only in the three public from-loader constructors, with comments
GetReaderWithContext and Extism's WithContext are fully removed
CI: SonarQube, dependency-review, Copilot review

https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL

Generated by Claude Code

Compiler.Compile and Loader.GetReader now take a context.Context as the first argument. NewExecutableUnit gains a leading ctx too, threading the same context into the loader's IO and the compiler's parser/compile. Removes: - Extism compiler's WithContext option and the ctx field on its struct (the user's stated anti-pattern: storing external ctx as a struct field) - Risor inner compile's hard-coded context.Background() - FromHTTP loader's GetReaderWithContext helper (now redundant) The three public from-loader constructors (FromExtismLoader, FromRisorLoader, FromStarlarkLoader) keep their signatures and pass context.Background() to NewExecutableUnit inline; they're one-shot startup constructors where that's the right default. Callers needing cancellable compile drive script.NewExecutableUnit directly. Net diff: -80 lines (261 insertions, 341 deletions across 44 files). Closes #89. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL

github-actions · 2026-05-23T18:30:09Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

…it call CI lint failures on PR #144: - engines/extism/compiler/options_test.go: three "unnecessary trailing newline" warnings from cleanup blocks left behind when WithContext tests were removed - engines/starlark/evaluator/evaluator_test.go: gci complaint on the NewExecutableUnit call — gofumpt prefers each arg on its own line when wrapping https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL

Copilot

Pull request overview

This PR threads context.Context through the core script compilation and loading pipeline (script.Compiler.Compile, loader.Loader.GetReader, and script.NewExecutableUnit) to enable cancellation/timeouts and to remove the anti-pattern of retaining external contexts on long-lived structs (notably the Extism compiler).

Changes:

Add ctx context.Context as the first parameter to Compiler.Compile, Loader.GetReader, and NewExecutableUnit, and plumb it through engine compilers/loaders.
Remove Extism compiler WithContext option + stored ctx field; remove HTTP loader GetReaderWithContext as redundant.
Update constructors/tests/call sites and add a changelog entry documenting the breaking API changes.

Reviewed changes

Copilot reviewed 44 out of 44 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
platform/script/compiler.go	Update `Compiler` interface to accept `context.Context`.
platform/script/compiler_test.go	Update compiler mock call sites for new `Compile(ctx, r)` signature.
platform/script/executableUnit.go	Add `ctx` to `NewExecutableUnit` and thread into loader/compiler.
platform/script/executableUnit_test.go	Update tests/mocks for context-threaded load/compile path.
platform/script/mocks_test.go	Update `MockCompiler` to accept `ctx` parameter.
platform/script/loader/loader.go	Update `Loader` interface to accept `context.Context` in `GetReader`.
platform/script/loader/loader_test.go	Update loader tests to pass a context to `GetReader`.
platform/script/loader/mock_helpers_test.go	Update loader mocks/helpers for `GetReader(ctx)` signature.
platform/script/loader/inference_test.go	Update inference tests to use `GetReader(t.Context())`.
platform/script/loader/fromBytes.go	Accept (ignored) `ctx` for in-memory byte loader.
platform/script/loader/fromBytes_test.go	Update FromBytes tests to pass context into `GetReader`.
platform/script/loader/fromString.go	Accept (ignored) `ctx` for in-memory string loader.
platform/script/loader/fromString_test.go	Update FromString tests to pass context into `GetReader`.
platform/script/loader/fromIoReader.go	Accept (ignored) `ctx` for buffered io.Reader loader.
platform/script/loader/fromIoReader_test.go	Update FromIoReader tests to pass context into `GetReader`.
platform/script/loader/fromDisk.go	Update disk loader `GetReader(ctx)` signature; adjust `String()` to avoid fabricating a ctx.
platform/script/loader/fromDisk_test.go	Update FromDisk tests to pass context into `GetReader`.
platform/script/loader/fromHTTP.go	Fold `GetReaderWithContext` into `GetReader(ctx)` and propagate ctx into HTTP request/auth.
platform/script/loader/fromHTTP_test.go	Update HTTP loader tests; rename/add cancellation-focused test.
engines/extism/new.go	Update engine constructor to call `NewExecutableUnit(context.Background(), ...)` with guidance comment.
engines/extism/new_test.go	Update extism engine constructor tests/mocks for `GetReader(ctx)`.
engines/extism/mock_loader_test.go	Update extism loader mock for `GetReader(ctx)` signature.
engines/extism/evaluator/exec_helpers_test.go	Update helper mocks/constructors to pass context through `NewExecutableUnit`.
engines/extism/compiler/compiler.go	Update Extism compiler to accept ctx parameter and pass it into SDK calls.
engines/extism/compiler/compiler_test.go	Update compiler tests for `Compile(ctx, r)` signature.
engines/extism/compiler/options.go	Remove `WithContext` and any default/validation around stored ctx.
engines/extism/compiler/options_test.go	Remove `WithContext` test cases and ctx-related validation expectations.
engines/risor/new.go	Update engine constructor to call `NewExecutableUnit(context.Background(), ...)` with guidance comment.
engines/risor/new_test.go	Update risor engine constructor tests/mocks for `GetReader(ctx)`.
engines/risor/mock_loader_test.go	Update risor loader mock for `GetReader(ctx)` signature.
engines/risor/evaluator/exec_helpers_test.go	Update helper mocks/constructors to pass context through `NewExecutableUnit`.
engines/risor/evaluator/evaluator_test.go	Update helper to pass ctx into `NewExecutableUnit`.
engines/risor/compiler/compiler.go	Update Risor compiler to accept ctx and pass into inner compile.
engines/risor/compiler/compiler_test.go	Update compiler tests for `Compile(ctx, r)` signature.
engines/risor/compiler/internal/compile/compile.go	Thread ctx into Risor parser (`Parse(ctx, ...)`) and update APIs accordingly.
engines/risor/compiler/internal/compile/compile_test.go	Update internal compile tests to pass context.
engines/starlark/new.go	Update engine constructor to call `NewExecutableUnit(context.Background(), ...)` with guidance comment.
engines/starlark/new_test.go	Update starlark engine constructor tests/mocks for `GetReader(ctx)`.
engines/starlark/mock_loader_test.go	Update starlark loader mock for `GetReader(ctx)` signature.
engines/starlark/evaluator/exec_helpers_test.go	Update helper mocks/constructors to pass context through `NewExecutableUnit`.
engines/starlark/evaluator/evaluator_test.go	Update evaluator test setup to pass context into `NewExecutableUnit`.
engines/starlark/compiler/compiler.go	Update Starlark compiler to accept ctx parameter (documented as not honored).
engines/starlark/compiler/compiler_test.go	Update compiler tests for `Compile(ctx, r)` signature.
CHANGELOG.md	Document the breaking interface changes and removed APIs.

Comments suppressed due to low confidence (2)

engines/extism/compiler/compiler.go:75

If io.ReadAll(scriptReader) returns an error, the reader is never closed, which can leak file descriptors / HTTP connections (and is especially likely when ctx cancellation interrupts a read). Consider ensuring scriptReader.Close() runs on all paths (e.g., defer close right after the nil check and handle/merge close errors appropriately).

func (c *Compiler) Compile(ctx context.Context, scriptReader io.ReadCloser) (script.ExecutableContent, error) {
	logger := c.logger.WithGroup("compile")

	if scriptReader == nil {
		return nil, ErrContentNil
	}

	scriptBytes, err := io.ReadAll(scriptReader)
	if err != nil {
		return nil, fmt.Errorf("failed to read script: %w", err)
	}

	err = scriptReader.Close()
	if err != nil {
		return nil, fmt.Errorf("failed to close reader: %w", err)
	}

engines/starlark/compiler/compiler.go:66

If io.ReadAll(scriptReader) returns an error, the reader is never closed, which can leak resources. Consider ensuring Close is always called (e.g., defer right after validating scriptReader != nil), including on early returns due to read errors.

func (c *Compiler) Compile(_ context.Context, scriptReader io.ReadCloser) (script.ExecutableContent, error) {
	if scriptReader == nil {
		return nil, ErrContentNil
	}

	scriptBodyBytes, err := io.ReadAll(scriptReader)
	if err != nil {
		return nil, fmt.Errorf("failed to read script: %w", err)
	}

	err = scriptReader.Close()
	if err != nil {
		return nil, fmt.Errorf("failed to close reader: %w", err)
	}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Per Copilot review on #144 — the previous shape called Close *after* io.ReadAll, so a read failure would return early and leave the reader open (FDs / HTTP connections leaked). Applies to all three engines (extism, risor, starlark). Each Compile now defers reader.Close() right after the nil check. Close errors are logged via the engine's logger rather than propagated; a successful read+compile shouldn't sink on a cleanup failure. Three "close error" tests updated to match the new behavior: Compile succeeds despite a failing Close (instead of returning the close error). https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL

Copilot

Pull request overview

Copilot reviewed 44 out of 44 changed files in this pull request and generated 4 comments.

robbyt · 2026-05-23T20:36:38Z

-	//   - ExecutableContent: The validated script
-	//   - error: Details about validation failures (syntax errors, undefined globals)
-	Compile(scriptReader io.ReadCloser) (ExecutableContent, error)
+	// script ready for execution. Cancellation of ctx halts the parser/compiler.


Fixed in 2c024eb — softened the doc to be honest about the three impls: Risor and Extism honor ctx for their parser/SDK cancellation; Starlark's parser is synchronous and ignores ctx (accepts it for interface conformance only).

Generated by Claude Code

robbyt · 2026-05-23T20:36:47Z

 	// Create a temporary instance to verify the entry point exists
-	instance, err := plugin.Instance(c.ctx, extismSDK.PluginInstanceConfig{})
+	instance, err := plugin.Instance(ctx, extismSDK.PluginInstanceConfig{})
 	if err != nil {
 		return nil, fmt.Errorf("%w: failed to create test instance: %w", ErrValidationFailed, err)
 	}
 	defer func() {
-		if err := instance.Close(c.ctx); err != nil {
+		if err := instance.Close(ctx); err != nil {
 			logger.Warn("Failed to close Extism plugin instance in compiler", "error", err)
 		}
 	}()


Good catch — fixed in 2c024eb. The deferred instance.Close now wraps the caller ctx with context.WithoutCancel(ctx), so a cancelled compile ctx still gets a chance to release the plugin instance cleanly without the WithoutCancel-introduced detachment masking real Close failures.

Generated by Claude Code

robbyt · 2026-05-23T20:37:00Z

 	t.Run("GetReaderError", func(t *testing.T) {
 		mockReader := new(mockReadCloser)

 		mockLoader := new(mockLoader)
-		mockLoader.On("GetReader").Return(mockReader, errors.New("get reader error")).Once()
+		mockLoader.On("GetReader", mock.Anything).Return(mockReader, errors.New("get reader error")).Once()



Fixed in 2c024eb — the test now models the standard Go convention (nil reader on error), matching what every real loader does. Also added a nil-guard to the mockLoader.GetReader method so the type assertion no longer panics when nil is the configured return.

Skipped adding defensive Close logic in NewExecutableUnit itself — a well-behaved Loader.GetReader following the (T, error) convention never produces a non-nil reader alongside a non-nil error, so the leak isn't reachable in practice. Treating it as a real possibility would convert a "loader contract violation" into a defensive guard, which goes against the project's "don't validate scenarios that can't happen" guideline.

Generated by Claude Code

robbyt · 2026-05-23T20:37:10Z

 // Loader is an interface used by the engines to load scripts or binaries.
+// GetReader accepts a context so loaders performing real I/O (HTTP, disk)
+// can honor caller cancellation; in-memory loaders ignore it.
 type Loader interface {
-	GetReader() (io.ReadCloser, error)
+	GetReader(ctx context.Context) (io.ReadCloser, error)


Fixed in 2c024eb — the docstring now says: HTTP truly honors ctx; FromDisk accepts ctx for interface conformance but os.Open is synchronous and doesn't observe it; in-memory loaders ignore ctx. Honest contract for each impl.

Generated by Claude Code

- platform/script/compiler.go: soften interface doc so it doesn't promise cancellation across all impls (Starlark's parser is synchronous and ignores ctx). - platform/script/loader/loader.go: same treatment — only HTTP truly honors ctx; FromDisk's os.Open is synchronous; in-memory loaders ignore ctx. - engines/extism/compiler/compiler.go: deferred instance.Close uses context.WithoutCancel(ctx) so a cancelled compile ctx doesn't abort cleanup and leak the plugin instance. - platform/script/executableUnit_test.go: the GetReaderError subtest used to return (mockReader, err), modeling an unrealistic loader. Real loaders follow the Go convention (nil reader on error); the mock now matches. Added nil-guard to mockLoader.GetReader so the type assertion no longer panics when nil is returned. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL

PR #144 review thread: the inlined os.Open in FromDisk.String() was overcautious avoidance of context.Background(). String() is a synchronous debug helper with no cancellation contract; using context.Background() inside a Stringer is benign and matches the pattern stdlib uses (e.g., sql.DB.Exec calling ExecContext with Background). Reverts to l.GetReader(context.Background()), restoring DRY (single os.Open site) so future changes to GetReader's body flow into String() automatically. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL

sonarqubecloud · 2026-05-23T20:37:45Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

* refactor: propagate ctx into the four public constructors Finishes the work started in #144. The four public consumer constructors now take ctx as the first argument: - polyscript.New[E](ctx, src, opts...) - extism.FromExtismLoader(ctx, ldr, opts...) - risor.FromRisorLoader(ctx, ldr, opts...) - starlark.FromStarlarkLoader(ctx, ldr, opts...) The three inline context.Background() injections in engines/*/new.go go away; cancellation now actually reaches the loader's I/O and the compiler's parse path. deprecated.go keeps its twelve legacy FromXxx* signatures unchanged and injects context.Background() inline at each downstream call. They are slated for removal in #104; churning their signatures during the deprecation window adds caller pain without value. New cancellation tests: - engines/risor/new_test.go: TestFromRisorLoader_CompileCancelled — pre-cancelled ctx aborts the Risor parser - engines/extism/new_test.go: TestFromExtismLoader_LoaderCancelled — pre-cancelled ctx aborts the loader's GetReader (the wazero compile of the small embedded test module completes too fast to demonstrate parser-level cancellation) - polyscript_test.go: TestNew_CompileCancelled (top-level Risor cancel) and TestNew_HTTPLoaderCancelled (end-to-end: slow httptest server, ctx deadline, verify the fetch aborts near the deadline rather than waiting for the full server stall) Closes #145. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL * fix(test): gofumpt-format the three engine new_test.go files CI gci lint flagged formatting on the new ctx-injected From*Loader calls. gofumpt prefers each arg on its own line when wrapping; the mechanical sed I used to inject t.Context() left some calls with two args on the wrap line. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL --------- Co-authored-by: Claude <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings May 23, 2026 18:30

Copilot started reviewing on behalf of robbyt May 23, 2026 18:30 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

Comment thread platform/script/executableUnit.go

Comment thread engines/risor/compiler/compiler.go Outdated

robbyt mentioned this pull request May 23, 2026

[v1][breaking] Propagate ctx into polyscript.New[E] and the three FromXxxLoader constructors #145

Closed

robbyt requested a review from Copilot May 23, 2026 20:25

Copilot started reviewing on behalf of robbyt May 23, 2026 20:26 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

claude added 2 commits May 23, 2026 20:35

robbyt merged commit 0a2e8be into main May 23, 2026
3 checks passed

robbyt deleted the claude/review-repo-improvements-NDaxm branch May 23, 2026 20:40

robbyt mentioned this pull request May 23, 2026

refactor: propagate ctx into the four public constructors #146

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: thread ctx through Compile and GetReader#144

refactor: thread ctx through Compile and GetReader#144
robbyt merged 5 commits into
mainfrom
claude/review-repo-improvements-NDaxm

robbyt commented May 23, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 23, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

robbyt May 23, 2026

Uh oh!

robbyt May 23, 2026

Uh oh!

robbyt May 23, 2026

Uh oh!

robbyt May 23, 2026

Uh oh!

sonarqubecloud Bot commented May 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

robbyt commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What's removed

What's added

Files

Test plan

Uh oh!

github-actions Bot commented May 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

Scanned Files

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

robbyt May 23, 2026

Choose a reason for hiding this comment

Uh oh!

robbyt May 23, 2026

Choose a reason for hiding this comment

Uh oh!

robbyt May 23, 2026

Choose a reason for hiding this comment

Uh oh!

robbyt May 23, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented May 23, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robbyt commented May 23, 2026 •

edited

Loading

github-actions Bot commented May 23, 2026 •

edited

Loading