From e47f7c59f75d4e951e98bd90b98f68e894f39ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Mon, 10 Aug 2026 00:48:44 +0200 Subject: [PATCH] fix(git): ignore inherited indexed Git config beyond GIT_CONFIG_COUNT (#2475) Co-authored-by: Kandev Agent --- apps/backend/AGENTS.md | 8 +-- .../agentctl/server/config/config_test.go | 34 +++++++++++ .../internal/gitconfigenv/environment.go | 42 ++------------ .../internal/gitconfigenv/environment_test.go | 56 +++++++++++++++++++ 4 files changed, 100 insertions(+), 40 deletions(-) diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 7561de25d5..f3aab55ff3 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -225,10 +225,10 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update - **Path/security tests:** Avoid using the real filesystem root as a fixture root. Build fake absolute roots under `t.TempDir()` with `filepath.Join`; this keeps tests portable across Windows, POSIX, and privileged cloud executors. - **Filesystem permission tests:** Assert permission-denied behavior only after probing that the current executor enforces the permission bit change. Root-like Sprite executors may bypass `chmod` restrictions. - **Filesystem safety checks:** Carry the original `os.Lstat` `FileInfo` (or opened handle) through every decision; do not re-stat a validated path, which reopens a TOCTOU window before side effects. Test mismatches before writes or manager commands. - - **Git indexed-environment tests:** When a test supplies `GIT_CONFIG_COUNT`, - `GIT_CONFIG_KEY_n`, and `GIT_CONFIG_VALUE_n`, clear every inherited indexed - key first and restore it with `t.Cleanup`. Changing only the count can leave - higher parent indexes behind and create a malformed Git config block. + - **Git indexed-environment tests:** When a test supplies `GIT_CONFIG_COUNT`, `GIT_CONFIG_KEY_n`, + and `GIT_CONFIG_VALUE_n`, clear every inherited indexed key first and restore it with + `t.Cleanup`. `internal/gitconfigenv` ignores indexes past the count the way Git does, so an + uncleared parent silently changes what the block contains instead of failing loudly. - **Full test output:** For local full-suite pass/fail validation, prefer plain `go test -race ./...`. `go test -json ./...` can emit very large JSONL streams; if a wrapper or tracing tool truncates the stream mid-record, downstream JSON parsing may fail even when Go tests passed. Use JSON output mainly for CI artifacts or test-report tooling that explicitly requires it. - **Private executable helpers:** When library code starts `os.Executable()` in a private helper mode, do not rely only on a command package's dispatch or diff --git a/apps/backend/internal/agentctl/server/config/config_test.go b/apps/backend/internal/agentctl/server/config/config_test.go index 02b375c3f1..fb34218e92 100644 --- a/apps/backend/internal/agentctl/server/config/config_test.go +++ b/apps/backend/internal/agentctl/server/config/config_test.go @@ -282,6 +282,40 @@ func TestCollectAgentEnvPreservesParentIndexedGitConfig(t *testing.T) { } } +func TestCollectAgentEnvIgnoresParentIndexedGitConfigBeyondCount(t *testing.T) { + // Hosts inherit indexed entries from a parent that later lowered + // GIT_CONFIG_COUNT. Git ignores the leftovers, so instance creation must + // too instead of failing every task start. + clearParentIndexedGitConfig(t) + t.Setenv("GIT_CONFIG_COUNT", "1") + t.Setenv("GIT_CONFIG_KEY_0", "core.hooksPath") + t.Setenv("GIT_CONFIG_VALUE_0", "/opt/locstat/hooks") + t.Setenv("GIT_CONFIG_KEY_1", "notes.augment.mergeStrategy") + t.Setenv("GIT_CONFIG_VALUE_1", "cat_sort_uniq") + + env, err := CollectAgentEnvWithError(map[string]string{ + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "credential.https://github.com.helper", + "GIT_CONFIG_VALUE_0": "!agentctl git-credential", + }) + if err != nil { + t.Fatalf("CollectAgentEnvWithError() error = %v", err) + } + + if got := envSliceValue(env, "GIT_CONFIG_COUNT"); got != "2" { + t.Fatalf("GIT_CONFIG_COUNT = %q, want 2", got) + } + if got := envSliceValue(env, "GIT_CONFIG_KEY_0"); got != "core.hooksPath" { + t.Fatalf("GIT_CONFIG_KEY_0 = %q, want core.hooksPath", got) + } + if got := envSliceValue(env, "GIT_CONFIG_KEY_1"); got != "credential.https://github.com.helper" { + t.Fatalf("GIT_CONFIG_KEY_1 = %q, want managed helper", got) + } + if got := envSliceValue(env, "GIT_CONFIG_KEY_2"); got != "" { + t.Fatalf("GIT_CONFIG_KEY_2 = %q, want stray parent entry dropped", got) + } +} + func TestCollectAgentEnvPreservesParentGitConfigHook(t *testing.T) { clearParentIndexedGitConfig(t) repository := t.TempDir() diff --git a/apps/backend/internal/gitconfigenv/environment.go b/apps/backend/internal/gitconfigenv/environment.go index 87046e9cad..fcea513a90 100644 --- a/apps/backend/internal/gitconfigenv/environment.go +++ b/apps/backend/internal/gitconfigenv/environment.go @@ -123,9 +123,9 @@ func entriesFrom(env map[string]string) ([]Entry, error) { } countValue, hasCount := env[countKey] if !hasCount { - if hasIndexedEntry(env) { - return nil, fmt.Errorf("%s is required when indexed entries are set", countKey) - } + // Git reads indexed entries only when GIT_CONFIG_COUNT is set, so any + // leftover key/value pair configures nothing. Ignore them the way Git + // does rather than rejecting an inherited environment we do not own. return nil, nil } count, err := strconv.Atoi(countValue) @@ -141,42 +141,12 @@ func entriesFrom(env map[string]string) ([]Entry, error) { } entries = append(entries, Entry{Key: key, Value: value}) } - if hasOutOfRangeIndexedEntry(env, count) { - return nil, fmt.Errorf("indexed entry exceeds %s", countKey) - } + // Entries at or past the count are invisible to Git — a parent process that + // lowered GIT_CONFIG_COUNT leaves its higher indexes behind — so they are + // dropped here instead of failing the whole block. return entries, nil } -func hasIndexedEntry(env map[string]string) bool { - for key := range env { - if strings.HasPrefix(key, keyPrefix) || strings.HasPrefix(key, valuePrefix) { - return true - } - } - return false -} - -func hasOutOfRangeIndexedEntry(env map[string]string, count int) bool { - for key := range env { - index, ok := indexedEntryIndex(key) - if ok && index >= count { - return true - } - } - return false -} - -func indexedEntryIndex(key string) (int, bool) { - prefix := keyPrefix - if strings.HasPrefix(key, valuePrefix) { - prefix = valuePrefix - } else if !strings.HasPrefix(key, keyPrefix) { - return 0, false - } - index, err := strconv.Atoi(strings.TrimPrefix(key, prefix)) - return index, err == nil && index >= 0 -} - func removeBoundaryOverlap(base, overlay []Entry) []Entry { maxOverlap := min(len(base), len(overlay)) for overlap := maxOverlap; overlap > 0; overlap-- { diff --git a/apps/backend/internal/gitconfigenv/environment_test.go b/apps/backend/internal/gitconfigenv/environment_test.go index dba04b71e5..4b6890429e 100644 --- a/apps/backend/internal/gitconfigenv/environment_test.go +++ b/apps/backend/internal/gitconfigenv/environment_test.go @@ -77,6 +77,62 @@ func TestIndexedGitConfigMergeRejectsMalformedBlock(t *testing.T) { } } +func TestIndexedGitConfigMergeIgnoresEntriesBeyondCount(t *testing.T) { + // Observed on a host whose shell hook re-initialized the block at count 2 + // without clearing the higher indexes a nested shell had appended. Git + // reads only entries 0 and 1, so the leftovers must not fail agent startup. + merged, err := Merge(map[string]string{ + "GIT_CONFIG_COUNT": "2", + "GIT_CONFIG_KEY_0": "notes.augment.mergeStrategy", + "GIT_CONFIG_VALUE_0": "union", + "GIT_CONFIG_KEY_1": "core.hooksPath", + "GIT_CONFIG_VALUE_1": "/Users/example/.locstat/git/hooks", + "GIT_CONFIG_KEY_2": "credential.useHttpPath", + "GIT_CONFIG_VALUE_2": "true", + "GIT_CONFIG_KEY_3": "notes.augment.mergeStrategy", + "GIT_CONFIG_VALUE_3": "union", + "GIT_CONFIG_KEY_4": "core.hooksPath", + "GIT_CONFIG_VALUE_4": "/Users/example/.locstat/git/hooks", + }, environmentWithEntries( + Entry{Key: "credential.https://github.com.helper", Value: "!agentctl git-credential"}, + )) + if err != nil { + t.Fatalf("Merge() error = %v", err) + } + + if got := merged["GIT_CONFIG_COUNT"]; got != "3" { + t.Fatalf("GIT_CONFIG_COUNT = %q, want 3", got) + } + if got := merged["GIT_CONFIG_KEY_1"]; got != "core.hooksPath" { + t.Fatalf("GIT_CONFIG_KEY_1 = %q, want core.hooksPath", got) + } + if got := merged["GIT_CONFIG_KEY_2"]; got != "credential.https://github.com.helper" { + t.Fatalf("GIT_CONFIG_KEY_2 = %q, want managed helper", got) + } + if got, exists := merged["GIT_CONFIG_KEY_3"]; exists { + t.Fatalf("GIT_CONFIG_KEY_3 = %q, want stray entry dropped", got) + } +} + +func TestIndexedGitConfigMergeIgnoresIndexedEntriesWithoutCount(t *testing.T) { + // Git reads indexed entries only when GIT_CONFIG_COUNT is set. + merged, err := Merge(map[string]string{ + "GIT_CONFIG_KEY_0": "core.hooksPath", + "GIT_CONFIG_VALUE_0": "/opt/locstat/hooks", + "HOME": "/home/kandev", + }, nil) + if err != nil { + t.Fatalf("Merge() error = %v", err) + } + + if got, exists := merged["GIT_CONFIG_KEY_0"]; exists { + t.Fatalf("GIT_CONFIG_KEY_0 = %q, want orphaned entry dropped", got) + } + if got := merged["HOME"]; got != "/home/kandev" { + t.Fatalf("HOME = %q, want ordinary variables preserved", got) + } +} + func TestFilterRemovesOnlyRequestedIndexedEntries(t *testing.T) { env := map[string]string{ "GIT_CONFIG_COUNT": "2",