test: add regression test for exec store identity propagation (#391)#700
Conversation
github-actions
Bot
added
the
status/needs-triage
There was a problem hiding this comment.
Pull request overview
Adds a Go regression test to prevent recurrence of #391 by asserting that buildStores correctly propagates per-rig identity environment variables to exec-based beads stores (GC_BEADS=exec:<script>), ensuring multi-prefix rigs don’t accidentally share the same exec-store identity.
Changes:
- Add
TestBuildStores_ExecProviderSetsPerRigEnvto validate per-rigGC_BEADS_PREFIX,BEADS_DIR,GC_RIG_ROOT, andGC_RIGpropagation for exec beads providers. - Add
stringsimport to support env-contents assertions in the new test.
| // Cross-rig assertion: the two rigs must have received different prefixes. | ||
| // This is the exact regression from #391 — before PR #421, both stores | ||
| // got identical env, so the last rig's prefix silently won. | ||
| alphaEnv, _ := os.ReadFile(filepath.Join(envDir, "alpha.env")) | ||
| bravoEnv, _ := os.ReadFile(filepath.Join(envDir, "bravo.env")) | ||
| if string(alphaEnv) == string(bravoEnv) { | ||
| t.Errorf("regression: alpha and bravo exec stores received identical env — "+ | ||
| "store identity is not being propagated per rig:\n%s", string(alphaEnv)) |
There was a problem hiding this comment.
The cross-rig assertion compares the entire captured env output as a raw string. env line ordering is not guaranteed, so two stores could have identical key/value pairs but different line order and this check would miss the regression (false negative). Consider parsing the file into a map (split lines on =) and comparing the relevant keys (or at least compare GC_BEADS_PREFIX values directly) instead of comparing the full string dump.
|
Addressed Copilot's inline feedback on the cross-rig assertion (606d753): now extracts and compares |
julianknutsen
added
priority/p2
sjarmak
force-pushed
the
fix/391-exec-store-identity-regression-test
branch
from
606d753 to
d98267d
Compare
…nhall#391) Add TestBuildStores_ExecProviderSetsPerRigEnv verifying that buildStores passes distinct GC_BEADS_PREFIX, BEADS_DIR, GC_RIG_ROOT, and GC_RIG env vars to each rig's exec store. Before PR gastownhall#421, all exec stores shared identical env — the last rig's prefix silently won, causing a create→orphan loop in K8s multi-prefix deployments. Closes gastownhall#391
Address Copilot review feedback: env line ordering is non-deterministic due to Go map iteration in exec.Store.run(), so comparing raw env output could produce false negatives. Extract and compare GC_BEADS_PREFIX values directly instead.
…moved, BEADS_DIR intentionally empty) PR gastownhall#790 rewrote the exec store env contract: - writeTempScript helper was removed (commit c6081e5) — inline the tempdir write - exec stores now set BEADS_DIR="" by design (store_target_exec.go:55); scope is communicated via GC_RIG_ROOT / GC_STORE_ROOT Update the regression test to match the new contract: - inline t.TempDir + os.WriteFile for the provider script - drop the BEADS_DIR=<rig>/.beads positive assertion - add a negative assertion that BEADS_DIR does NOT get a rig-specific path (would indicate regression back to the pre-gastownhall#790 projection model) The original gastownhall#391 regression (all exec stores sharing identical env) is still covered by the per-rig GC_BEADS_PREFIX / GC_RIG_ROOT / GC_RIG assertions and the cross-rig prefix-differ check.
sjarmak
force-pushed
the
fix/391-exec-store-identity-regression-test
branch
from
d98267d to
6e42099
Compare
Summary
TestBuildStores_ExecProviderSetsPerRigEnvregression test for bug: exec beads provider ignores store identity on CRUD operations, breaking K8s multi-prefix #391buildStoreswithGC_BEADS=exec:<script>passes distinctGC_BEADS_PREFIX,BEADS_DIR,GC_RIG_ROOT, andGC_RIGenv vars to each rig's exec storeThe maintainer's investigation comment identified this as the remaining work to close #391:
Closes #391
Test plan
go test -run TestBuildStores_ExecProviderSetsPerRigEnv -v ./cmd/gc/— PASSgo test -race -run TestBuildStores_ExecProviderSetsPerRigEnv ./cmd/gc/— PASSgo vet ./cmd/gc/— cleanmake build— PASSmake check— baseline-only failure (TestHandleProviderReadinessReturnsNotInstalledWhenBinaryMissing, env-dependent, fails identically on clean main)🤖 Generated with Claude Code