diff --git a/internal/agent/agentenv_test.go b/internal/agent/agentenv_test.go new file mode 100644 index 00000000..04a51995 --- /dev/null +++ b/internal/agent/agentenv_test.go @@ -0,0 +1,107 @@ +package agent + +import ( + "os" + "path/filepath" + "regexp" + "slices" + "strings" + "testing" + + "github.com/yetone/magpie/internal/agentenv" +) + +// readsEnv matches the two ways these packages name a variable they read to +// find an agent's folder: as the argument of a Getenv, a LookupEnv or a +// place's getenv, and as the env of a table entry, which is how Qoder's two +// builds are named (read through at.getenv(b.env)). +var readsEnv = regexp.MustCompile(`(?:Getenv|LookupEnv|getenv)\("([A-Z][A-Z_0-9]*)"\)|env:\s*"([A-Z][A-Z_0-9]*)"`) + +// notAnAgent are the variables the same sources read that are not an agent's +// folder, so no sandbox clears them through agentenv.Vars. +var notAnAgent = map[string]string{ + "APPDATA": "Windows' own folder, which a sandbox sets to one of its own rather than clears", + "LOCALAPPDATA": "the same", + "PATH": "the process' own", + "TZ": "the process' own", + "USER": "the process' own", + "XDG_CONFIG_HOME": "magpie's own folder's, which appdir decides", + "XDG_DATA_HOME": "the same", + "MAGPIE_ADDR": "magpie's own", +} + +// TestFolderVarsAreListed says every variable the packages that find an +// agent's folder read is one a sandbox clears. Nothing said so before, so a +// new agent could take a variable of its own and every sandbox would go on +// meeting the agent a developer really has installed — which is how #522 came +// about, four sandboxes clearing lists of their own that had each drifted. +// +// The four packages are this one, sessions, provider and library: every +// agent's folder variable magpie reads is read in one of them, and the rest +// of the repository reads only magpie's own variables, the process' and the +// desktop's. Their sources are read as files rather than imported, since a +// test in sessions or provider cannot import this package, which imports +// them; what the walk looks at is what they say, not what they do. +// +// What this cannot see is a variable read through a name the sources build at +// runtime, as omoDir reads OMO_CODING_AGENT_DIR and SENPI_CODING_AGENT_DIR +// out of a slice. Both are in agentenv.Vars; a third taken the same way would +// not be caught here. +func TestFolderVarsAreListed(t *testing.T) { + listed := make(map[string]bool, len(agentenv.Vars)) + for _, v := range agentenv.Vars { + listed[v] = true + } + dirs := []string{".", filepath.Join("..", "sessions"), filepath.Join("..", "provider"), filepath.Join("..", "library")} + found := map[string]string{} + for _, dir := range dirs { + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + n := e.Name() + if e.IsDir() || !strings.HasSuffix(n, ".go") || strings.HasSuffix(n, "_test.go") { + continue + } + b, err := os.ReadFile(filepath.Join(dir, n)) + if err != nil { + t.Fatal(err) + } + for _, m := range readsEnv.FindAllStringSubmatch(string(b), -1) { + name := m[1] + if name == "" { + name = m[2] + } + if _, ok := found[name]; !ok { + found[name] = filepath.Join(dir, n) + } + } + } + } + // One name read each way the pattern matches, so that a change in how a + // variable is read empties the scan loudly instead of leaving the test + // nothing to complain of. + for _, canary := range []string{"CODEX_HOME", "QODER_CONFIG_DIR"} { + if _, ok := found[canary]; !ok { + t.Errorf("%s was not found in these sources, so readsEnv no longer matches the way a variable is read", canary) + } + } + // And no excuse outlives the variable it was for. + for n := range notAnAgent { + if _, ok := found[n]; !ok { + t.Errorf("%s is in notAnAgent, but nothing in these sources reads it any more", n) + } + } + names := make([]string, 0, len(found)) + for n := range found { + names = append(names, n) + } + slices.Sort(names) + for _, n := range names { + if listed[n] || notAnAgent[n] != "" { + continue + } + t.Errorf("%s, read by %s, is not in agentenv.Vars, so no sandbox clears it and a test meets the agent a developer really has installed: add it to Vars, or to notAnAgent here with a reason if it is not an agent's folder", n, found[n]) + } +} diff --git a/internal/agentenv/agentenv.go b/internal/agentenv/agentenv.go index bc443b40..9b939945 100644 --- a/internal/agentenv/agentenv.go +++ b/internal/agentenv/agentenv.go @@ -15,10 +15,10 @@ // Each package keeps its own sandbox, because a test in internal/sessions // cannot import internal/agent, which imports it, to ask which variables // there are; this is the list they all clear. A name here is one magpie -// reads, and VarsTest says so, so a variable magpie stopped reading does not -// linger — that is how GEMINI_CLI_HOME and OPENCODE_CONFIG came to be -// cleared by a sandbox while OPENCODE_CONFIG_DIR, the one magpie reads, was -// not. What is deliberately not here: APPDATA and LOCALAPPDATA, Windows' +// reads, and TestVarsAreRead says so, so a variable magpie stopped reading +// does not linger — that is how GEMINI_CLI_HOME and OPENCODE_CONFIG came to +// be cleared by a sandbox while OPENCODE_CONFIG_DIR, the one magpie reads, +// was not. What is deliberately not here: APPDATA and LOCALAPPDATA, Windows' // folders rather than an agent's, which a sandbox sets to a folder of its // own instead of clearing; and CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC, a // behaviour flag whose one test sets it on purpose to read what magpie then diff --git a/internal/provider/kiro_main_test.go b/internal/provider/kiro_main_test.go index a17a654d..4e852423 100644 --- a/internal/provider/kiro_main_test.go +++ b/internal/provider/kiro_main_test.go @@ -4,6 +4,8 @@ import ( "os" "path/filepath" "testing" + + "github.com/yetone/magpie/internal/agentenv" ) // TestMain keeps the package's tests off the Kiro sign-ins of whoever runs @@ -25,6 +27,11 @@ func TestMain(m *testing.M) { } { os.Setenv(k, v) } + // Accounts here read what an agent keeps on this machine: a variable left + // in the shell would point a test at the runner's real one (#522). + for _, k := range agentenv.Vars { + os.Unsetenv(k) + } kiroCLIDB = func() string { return filepath.Join(dir, "kiro-cli", "data.sqlite3") } kiroIDEDir = func() string { return filepath.Join(dir, "sso") } askKiroIdentity = func(string, string) (string, string) { return "", "" } diff --git a/internal/usage/main_test.go b/internal/usage/main_test.go index 11136261..74828905 100644 --- a/internal/usage/main_test.go +++ b/internal/usage/main_test.go @@ -4,6 +4,8 @@ import ( "os" "path/filepath" "testing" + + "github.com/yetone/magpie/internal/agentenv" ) // TestMain gives the package a home of its own: a test that doesn't set one @@ -21,6 +23,12 @@ func TestMain(m *testing.M) { } { os.Setenv(k, v) } + // The package's ledger reads what sessions finds, which asks each agent's + // own variable before its folder in the home: one left in the shell would + // point a test at the runner's real agent (#522). + for _, k := range agentenv.Vars { + os.Unsetenv(k) + } code := m.Run() os.RemoveAll(home) os.Exit(code)