refactor(script): unexport ExecutableUnit fields#139
Merged
Conversation
Unexports all six fields on platform/script.ExecutableUnit (ID, CreatedAt,
ScriptLoader, Compiler, Content, DataProvider). The existing getters
(GetID, GetCreatedAt, GetLoader, GetCompiler, GetContent, GetDataProvider)
cover all external read needs.
Construction must now route through NewExecutableUnit; mid-flight mutation
of fields like DataProvider by external callers is no longer possible.
Test migration: the 31 external `&script.ExecutableUnit{...}` literals across
the three engine evaluator test packages now route through a per-package
`newExe(t, id, content, provider)` helper that wires a mock loader+compiler
to NewExecutableUnit, preserving the ability to inject pre-built content.
One risor test case ("empty execution id") is dropped — that scenario is
unreachable now that NewExecutableUnit auto-derives an ID from content.
Closes #91.
https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors platform/script.ExecutableUnit to make its core fields unexported, enforcing construction via NewExecutableUnit and access via getters, and updates engine evaluator tests accordingly. This supports the broader goal of preventing external callers from mutating execution-critical state (notably DataProvider) after construction.
Changes:
- Unexported
ExecutableUnitfields (ID,CreatedAt,ScriptLoader,Compiler,Content,DataProvider) and updated getters /String(). - Migrated engine evaluator tests away from
&script.ExecutableUnit{...}literals by introducing per-packagenewExe(...)helpers. - Added a breaking-change entry to
CHANGELOG.mdunder[Unreleased] > Changed.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| platform/script/executableUnit.go | Unexports fields and updates constructor return literal, String(), and getters. |
| platform/script/executableUnit_test.go | Updates in-package tests to use the new unexported field names. |
| engines/extism/evaluator/exec_helpers_test.go | Adds helper mocks and newExe(...) to build units via script.NewExecutableUnit. |
| engines/extism/evaluator/evaluator_test.go | Replaces ExecutableUnit{...} literals with newExe(...). |
| engines/risor/evaluator/exec_helpers_test.go | Adds helper mocks and newExe(...) (with naming to avoid local clashes). |
| engines/risor/evaluator/evaluator_test.go | Replaces literals with newExe(...); removes now-unreachable “empty execution id” case. |
| engines/starlark/evaluator/exec_helpers_test.go | Adds helper mocks and newExe(...) to build units via script.NewExecutableUnit. |
| engines/starlark/evaluator/evaluator_test.go | Replaces ExecutableUnit{...} literals with newExe(...) and updates provider access. |
| CHANGELOG.md | Documents the breaking change for ExecutableUnit field visibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…cutableUnit Addresses Copilot review on #139: NewExecutableUnit now rejects a nil scriptLoader (parallel to the existing nil-compiler check) and a nil ExecutableContent returned by Compile (which would otherwise nil-deref exe.GetSource() on the auto-fill path). Adds two new subtests covering both error branches. Updates the per-package newExe test helper to substitute a tiny stubContent when callers pass nil, since the new post-Compile guard makes nil content unreachable through NewExecutableUnit. Drops the now-unreachable starlark "content nil" subtest accordingly. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Trailing whitespace on the stubContent method signatures tripped gci on CI. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Reverts the scriptLoader and post-Compile content nil checks per the project's "don't validate scenarios that can't happen" guideline: - scriptLoader == nil: panics immediately with a clear nil-pointer message at scriptLoader.GetReader(), which is sufficient feedback at the boundary. - exe == nil from Compile: would be a Compiler-interface contract violation, not a state real compilers produce. Also restores the starlark "content nil" subtest (now reachable again) and drops the stubContent shim from each newExe helper. https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Unexports all six fields on
platform/script.ExecutableUnit(ID,CreatedAt,ScriptLoader,Compiler,Content,DataProvider). The existing getters cover every external read; construction routes throughNewExecutableUnitexclusively, so external callers can no longer mutateexe.DataProvider(or any other field) mid-flight.Breaking change (pre-1.0, landed under
[Unreleased] > Changed).Test migration
The 31 external
&script.ExecutableUnit{...}literals across the three engine evaluator test packages now route through a small per-package helpernewExe(t, id, content, provider)that wires a mock loader + compiler toNewExecutableUnit. This preserves the ability to inject pre-built mock content while routing through the public constructor.One risor test case (
"empty execution id") was dropped — that scenario is unreachable now thatNewExecutableUnitauto-derives a non-empty ID fromcontent.GetSource(). The corresponding evaluator-level guard (exeID == "" → "exeID is empty") is left in place across all three engines as a defensive check.Files
platform/script/executableUnit.go— six fields lowercased, constructor + String + getters updatedplatform/script/executableUnit_test.go— in-package literals/reads use lowercase fieldsengines/{extism,risor,starlark}/evaluator/exec_helpers_test.go(new) — per-packageloaderMock,compilerMock,newExehelperengines/{extism,risor,starlark}/evaluator/evaluator_test.go— 31 literals replaced withnewExe(...); 6exe.DataProviderreads switched toexe.GetDataProvider(); one untestable case droppedCHANGELOG.md—[Unreleased] > ChangedentryCloses #91.
Test plan
go build ./...cleango vet ./...cleango test -race -count=1 ./...full suite greengrep -rn 'script\.ExecutableUnit{' --include='*.go' .returns zero matches (no external literal constructions remain)https://claude.ai/code/session_01C61VEAmjxSnX5Xhbab8NvL
Generated by Claude Code