From a6263a3b4ac0226dd9f7c7c6978819a883ff1902 Mon Sep 17 00:00:00 2001 From: Alexandre Yang Date: Tue, 10 Mar 2026 23:39:03 +0100 Subject: [PATCH 1/3] Fail import allowlist test on unused symbols Add a check that every symbol in builtinAllowedSymbols is actually referenced by at least one builtin file. This keeps the allowlist minimal and prevents stale entries from accumulating. Remove io/fs.DirEntry which was the one unused symbol caught. Co-Authored-By: Claude Opus 4.6 --- tests/import_allowlist_test.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/import_allowlist_test.go b/tests/import_allowlist_test.go index 8c69085a..3c944c36 100644 --- a/tests/import_allowlist_test.go +++ b/tests/import_allowlist_test.go @@ -44,8 +44,6 @@ var builtinAllowedSymbols = []string{ "errors.New", // fmt.Sprintf — string formatting; pure function, no I/O. "fmt.Sprintf", - // io/fs.DirEntry — interface type for directory entries; no side effects. - "io/fs.DirEntry", // io/fs.FileInfo — interface type for file information; no side effects. "io/fs.FileInfo", // io/fs.ModeDir — file mode bit constant for directories; pure constant. @@ -144,6 +142,7 @@ var permanentlyBanned = map[string]string{ func TestBuiltinImportAllowlist(t *testing.T) { // Build lookup sets from the allowlist. allowedSymbols := make(map[string]bool, len(builtinAllowedSymbols)) + usedSymbols := make(map[string]bool, len(builtinAllowedSymbols)) allowedPackages := make(map[string]bool) for _, entry := range builtinAllowedSymbols { dot := strings.LastIndexByte(entry, '.') @@ -261,6 +260,8 @@ func TestBuiltinImportAllowlist(t *testing.T) { if !allowedSymbols[key] { pos := fset.Position(sel.Pos()) t.Errorf("%s:%d: %s is not in the allowlist", rel, pos.Line, key) + } else { + usedSymbols[key] = true } return true }) @@ -268,4 +269,12 @@ func TestBuiltinImportAllowlist(t *testing.T) { if checked == 0 { t.Fatal("no command implementation files found in interp/builtins/ sub-packages") } + + // Verify every symbol in the allowlist is actually used by at least one + // builtin. Unused entries should be removed to keep the allowlist minimal. + for _, entry := range builtinAllowedSymbols { + if !usedSymbols[entry] { + t.Errorf("allowlist symbol %q is not used by any builtin — remove it from builtinAllowedSymbols", entry) + } + } } From c6a4bac1771b1e43319f1a5adc1f42c52b9bcf2d Mon Sep 17 00:00:00 2001 From: Alexandre Yang Date: Tue, 10 Mar 2026 23:41:48 +0100 Subject: [PATCH 2/3] Rename import allowlist test to allowed symbols test Co-Authored-By: Claude Opus 4.6 --- tests/{import_allowlist_test.go => allowed_symbols_test.go} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename tests/{import_allowlist_test.go => allowed_symbols_test.go} (98%) diff --git a/tests/import_allowlist_test.go b/tests/allowed_symbols_test.go similarity index 98% rename from tests/import_allowlist_test.go rename to tests/allowed_symbols_test.go index 3c944c36..31b4150c 100644 --- a/tests/import_allowlist_test.go +++ b/tests/allowed_symbols_test.go @@ -135,11 +135,11 @@ var permanentlyBanned = map[string]string{ "unsafe": "bypasses Go's type and memory safety guarantees", } -// TestBuiltinImportAllowlist enforces symbol-level import restrictions on +// TestBuiltinAllowedSymbols enforces symbol-level import restrictions on // command implementation files in interp/builtins/. builtins.go is exempt as // the package framework. Every other file's imports and pkg.Symbol references // must be explicitly listed in builtinAllowedSymbols. -func TestBuiltinImportAllowlist(t *testing.T) { +func TestBuiltinAllowedSymbols(t *testing.T) { // Build lookup sets from the allowlist. allowedSymbols := make(map[string]bool, len(builtinAllowedSymbols)) usedSymbols := make(map[string]bool, len(builtinAllowedSymbols)) From 79c6f68a41e81168463e72dbf4ff55da23ad5e9d Mon Sep 17 00:00:00 2001 From: Alexandre Yang Date: Tue, 10 Mar 2026 23:48:28 +0100 Subject: [PATCH 3/3] Fix Windows CI: ls sandbox test expects wrong error for /etc On Windows, /etc is not an absolute path (no drive letter), so it resolves to /etc inside the sandbox, yielding "no such file or directory" instead of "permission denied". Add stderr_windows override. Co-Authored-By: Claude Opus 4.6 --- tests/scenarios/cmd/ls/sandbox/outside_allowed_paths.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/scenarios/cmd/ls/sandbox/outside_allowed_paths.yaml b/tests/scenarios/cmd/ls/sandbox/outside_allowed_paths.yaml index e199ee2b..a22beaae 100644 --- a/tests/scenarios/cmd/ls/sandbox/outside_allowed_paths.yaml +++ b/tests/scenarios/cmd/ls/sandbox/outside_allowed_paths.yaml @@ -11,4 +11,8 @@ input: expect: stdout: "" stderr: "ls: cannot access '/etc': permission denied\n" + # On Windows, /etc is not an absolute path (no drive letter), so it resolves + # to /etc inside the sandbox — yielding "no such file or directory" + # instead of "permission denied". + stderr_windows: "ls: cannot access '/etc': no such file or directory\n" exit_code: 1