From 7586df4400aacdf1f2592af9fb891530ebfa0448 Mon Sep 17 00:00:00 2001 From: Lin Huazhong <69783678+LHHHHHZ@users.noreply.github.com> Date: Fri, 2 Oct 2026 18:41:27 +0800 Subject: [PATCH] =?UTF-8?q?test:=20usage's=20and=20provider's=20TestMains?= =?UTF-8?q?=20clear=20the=20agents'=20variables=20too,=20and=20a=20new=20a?= =?UTF-8?q?gent's=20variable=20left=20out=20of=20the=20list=20now=20fails?= =?UTF-8?q?=20(yetone,=20reviewing=20#543:=20internal/usage/main=5Ftest.go?= =?UTF-8?q?'s=20TestMain=20still=20clears=20no=20agent=20variables,=20so?= =?UTF-8?q?=20a=20CLAUDE=5FCONFIG=5FDIR=20left=20in=20the=20shell=20reache?= =?UTF-8?q?s=20the=20real=20~/.claude=20there=20too;=20nothing=20fails=20y?= =?UTF-8?q?et=20when=20a=20new=20agent's=20variable=20is=20left=20out=20of?= =?UTF-8?q?=20Vars)=20=E2=80=94=20both=20TestMains=20gave=20their=20packag?= =?UTF-8?q?e=20a=20home=20of=20its=20own=20and=20cleared=20no=20agent=20va?= =?UTF-8?q?riable,=20the=20provider=20one=20as=20much=20as=20the=20usage?= =?UTF-8?q?=20one=20that=20was=20named.=20A=20CLAUDE=5FCONFIG=5FDIR=20left?= =?UTF-8?q?=20in=20the=20shell=20reached=20the=20real=20~/.claude=20throug?= =?UTF-8?q?h=20the=20ledger's=20sessions.Calls=20(usage/ledger.go:122):=20?= =?UTF-8?q?with=20a=20canary=20Claude=20folder=20holding=20three=20of=20th?= =?UTF-8?q?e=20sessions=20package's=20fixtures,=20six=20usage=20tests=20fa?= =?UTF-8?q?il=20=E2=80=94=20TestLedger,=20TestSubscriptionListPrice,=20Tes?= =?UTF-8?q?tLedgerUsesTheStatedPrice,=20TestLedgerConfiguredPricesAndRefre?= =?UTF-8?q?sh=20and=20the=20two=20Antigravity=20ledger=20ones=20=E2=80=94?= =?UTF-8?q?=20and=20pass=20with=20the=20clearing;=20provider=20passes=20wi?= =?UTF-8?q?th=20the=20same=20canary=20either=20way,=20but=20its=20Accounts?= =?UTF-8?q?=20read=20what=20an=20agent=20keeps=20on=20the=20machine=20too.?= =?UTF-8?q?=20#543=20missed=20these=20by=20how=20it=20looked=20for=20its?= =?UTF-8?q?=20sites:=20by=20the=20clearing=20loops=20a=20sandbox=20already?= =?UTF-8?q?=20had,=20which=20one=20with=20no=20loop=20at=20all=20has=20no?= =?UTF-8?q?=20way=20to=20answer.=20TestFolderVarsAreListed=20reads=20the?= =?UTF-8?q?=20sources=20of=20the=20four=20packages=20an=20agent's=20folder?= =?UTF-8?q?=20variable=20is=20read=20in=20=E2=80=94=20this=20one,=20sessio?= =?UTF-8?q?ns,=20provider=20and=20library,=20the=20rest=20of=20the=20repos?= =?UTF-8?q?itory=20reading=20only=20magpie's=20own=20variables,=20the=20pr?= =?UTF-8?q?ocess'=20and=20the=20desktop's=20=E2=80=94=20and=20fails=20on?= =?UTF-8?q?=20a=20name=20that=20is=20neither=20in=20agentenv.Vars=20nor=20?= =?UTF-8?q?excused=20here=20with=20a=20reason,=20so=20a=20new=20agent's=20?= =?UTF-8?q?variable=20no=20longer=20waits=20for=20a=20developer's=20shell?= =?UTF-8?q?=20to=20meet=20one.=20It=20reads=20them=20as=20files,=20since?= =?UTF-8?q?=20a=20test=20in=20sessions=20or=20provider=20cannot=20import?= =?UTF-8?q?=20this=20package,=20which=20imports=20them;=20it=20asks=20for?= =?UTF-8?q?=20one=20name=20each=20way=20its=20pattern=20matches,=20so=20a?= =?UTF-8?q?=20change=20in=20how=20a=20variable=20is=20read=20empties=20the?= =?UTF-8?q?=20scan=20loudly=20instead=20of=20passing=20it,=20and=20for=20e?= =?UTF-8?q?very=20name=20excused,=20so=20no=20excuse=20outlives=20its=20va?= =?UTF-8?q?riable.=20What=20it=20cannot=20see=20is=20a=20name=20the=20sour?= =?UTF-8?q?ces=20build=20at=20runtime,=20as=20omoDir=20reads=20its=20two?= =?UTF-8?q?=20out=20of=20a=20slice=20(omo.go:33);=20both=20are=20listed,?= =?UTF-8?q?=20and=20a=20third=20taken=20the=20same=20way=20would=20not=20b?= =?UTF-8?q?e=20caught.=20The=20package=20doc's=20VarsTest=20is=20TestVarsA?= =?UTF-8?q?reRead.=20TestFolderVarsAreListed=20fails=20without=20the=20cha?= =?UTF-8?q?nge=20when=20KIMI=5FCODE=5FHOME=20is=20taken=20out=20of=20Vars,?= =?UTF-8?q?=20or=20when=20a=20pretend=20NEWAGENT=5FHOME=20is=20read=20in?= =?UTF-8?q?=20agents.go,=20and=20names=20the=20file=20that=20reads=20it;?= =?UTF-8?q?=20the=20six=20usage=20tests=20fail=20without=20theirs.=20On=20?= =?UTF-8?q?Windows=2011=20the=20whole=20suite=20answers=20as=20it=20did=20?= =?UTF-8?q?before=20the=20change:=2031=20packages=20pass,=20and=20the=20sa?= =?UTF-8?q?me=2015=20tests=20in=20internal/library,=20internal/gateway=20a?= =?UTF-8?q?nd=20internal/agent=20fail=20for=20what=20they=20failed=20for?= =?UTF-8?q?=20before.=20go=20vet=20./...,=20go=20build=20-tags=20nogui=20(?= =?UTF-8?q?windows,=20GOOS=3Dlinux,=20GOOS=3Ddarwin),=20go=20vet=20-tags?= =?UTF-8?q?=20nogui=20for=20linux=20and=20darwin=20over=20every=20package?= =?UTF-8?q?=20touched,=20and=20gofmt=20pass.=20(#549)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- internal/agent/agentenv_test.go | 107 ++++++++++++++++++++++++++++ internal/agentenv/agentenv.go | 8 +-- internal/provider/kiro_main_test.go | 7 ++ internal/usage/main_test.go | 8 +++ 4 files changed, 126 insertions(+), 4 deletions(-) create mode 100644 internal/agent/agentenv_test.go 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)