Enforce builtin command structure and import allowlist#11
Merged
Conversation
- Introduced self-registration via init() and a Register() function, replacing the manually maintained central registry in builtins.go; each command implementation now owns its own registration - Moved all command implementations (cat, echo, exit, true/false, break/continue) from interp/builtins/ into a new interp/builtins/cmds/ package, keeping the framework (CallContext, Result, Register, Lookup) isolated in interp/builtins/ - Changed CallContext.Stdin from *os.File to io.Reader to eliminate the dangerous *os.File method surface from command implementations; guarded the assignment in runner_exec.go to avoid typed-nil interface issues - Added tests/import_allowlist_test.go with a symbol-level import allowlist for interp/builtins/cmds/; every package and pkg.Symbol used by a command implementation must be explicitly listed, including internal packages; reflect and unsafe are permanently banned Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Renamed Register → register (unexported); the Go compiler now enforces that only files within package builtins can call it, making the AST-based TestBuiltinRegisterOnlyFromCmds test redundant — removed it - Moved command implementations back from interp/builtins/cmds/ into interp/builtins/ (same package), removing the need for the blank import in runner_exec.go and the internal-package allowlist entries - Restored the builtins.go exclusion in TestBuiltinImportAllowlist; this is safe because Go's per-file import system ensures that imports in builtins.go do not bleed into command implementation files Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AlexandreYang
approved these changes
Mar 9, 2026
This was referenced Mar 12, 2026
Closed
Merged
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
2 participants
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.
Introduced self-registration via init() and a Register() function, replacing the manually maintained central registry in builtins.go; each command implementation now owns its own registration
Moved all command implementations (cat, echo, exit, true/false, break/continue) from interp/builtins/ into a new interp/builtins/cmds/ package, keeping the framework (CallContext, Result, Register, Lookup) isolated in interp/builtins/
Changed CallContext.Stdin from *os.File to io.Reader to eliminate the dangerous *os.File method surface from command implementations; guarded the assignment in runner_exec.go to avoid typed-nil interface issues
Added tests/import_allowlist_test.go with a symbol-level import allowlist for interp/builtins/cmds/; every package and pkg.Symbol used by a command implementation must be explicitly listed, including internal packages; reflect and unsafe are permanently banned