From da8fa2dabd75abbc2d492d9780ad85452a86b763 Mon Sep 17 00:00:00 2001 From: webster Date: Fri, 22 May 2026 13:00:45 -0700 Subject: [PATCH 1/2] Hash session+branch for sidecar dedup (FACT-176) - Sidecar state files are now named sidecar.-.json where hash8 = sha256(sessionID+":"+branch)[:4], isolating concurrent sessions and branches without exposing raw branch names in filenames. - Auto-named sidecars use the same -- scheme; falls back to sanitised branch name when no session ID is present. - Export sidecar.CurrentBranch so cmd/validate.go can reuse it instead of duplicating the git invocation inline. - Pin hash assertions in tests with hashFor() instead of structural checks. Co-Authored-By: Claude Sonnet 4.6 --- acceptance/validate_test.go | 20 ++++- internal/cmd/validate.go | 48 +++++++++++- internal/cmd/validate_test.go | 135 ++++++++++++++++++++++++++++++++ internal/sidecar/active.go | 62 ++++++++++++--- internal/sidecar/active_test.go | 32 +++++++- 5 files changed, 283 insertions(+), 14 deletions(-) diff --git a/acceptance/validate_test.go b/acceptance/validate_test.go index da93d04b..6339bf80 100644 --- a/acceptance/validate_test.go +++ b/acceptance/validate_test.go @@ -17,6 +17,7 @@ import ( "golang.org/x/crypto/ssh" "gotest.tools/v3/assert" + "github.com/CircleCI-Public/chunk-cli/internal/sidecar" "github.com/CircleCI-Public/chunk-cli/internal/testing/binary" testenv "github.com/CircleCI-Public/chunk-cli/internal/testing/env" "github.com/CircleCI-Public/chunk-cli/internal/testing/fakes" @@ -476,11 +477,28 @@ func writeSidecarState(t *testing.T, e *testenv.TestEnv, projectRoot, sessionID, sum := sha256.Sum256([]byte(filepath.Clean(realRoot))) dir := filepath.Join(e.HomeDir, ".local", "share", "chunk", fmt.Sprintf("%x", sum)) assert.NilError(t, os.MkdirAll(dir, 0o755)) - filename := "sidecar." + sessionID + ".json" + // Detect the branch so the file name matches what the subprocess will look for. + branch := gitCurrentBranch(t, projectRoot) + filename := sidecar.StateFileName(sessionID, branch) data := []byte(`{"sidecar_id":"` + sidecarID + `"}`) assert.NilError(t, os.WriteFile(filepath.Join(dir, filename), data, 0o644)) } +// gitCurrentBranch returns the current branch of the git repo at dir, or "" +// on any error. +func gitCurrentBranch(t *testing.T, dir string) string { + t.Helper() + out, err := exec.Command("git", "-C", dir, "rev-parse", "--abbrev-ref", "HEAD").Output() + if err != nil { + return "" + } + b := strings.TrimSpace(string(out)) + if b == "HEAD" { + return "" + } + return b +} + // TestValidateHookMode_SessionIsolation verifies that two concurrent Claude // sessions each see their own sidecar state rather than sharing one file. func TestValidateHookMode_SessionIsolation(t *testing.T) { diff --git a/internal/cmd/validate.go b/internal/cmd/validate.go index cc55a2e2..10deae6c 100644 --- a/internal/cmd/validate.go +++ b/internal/cmd/validate.go @@ -3,12 +3,14 @@ package cmd import ( "bufio" "context" + "crypto/sha256" "encoding/json" "errors" "fmt" "io" "os" "path/filepath" + "regexp" "strings" "github.com/spf13/cobra" @@ -585,7 +587,7 @@ func resolveOrCreateSidecarID(ctx context.Context, client *circleci.Client, side if err != nil { return false, err } - sandboxName := filepath.Base(workDir) + "-validate" + sandboxName := sidecarAutoName(ctx, workDir) sc, err := sidecar.Create(ctx, client, resolvedOrgID, sandboxName, image) if err != nil { if authErr := notAuthorized("create sidecars", err); authErr != nil { @@ -616,6 +618,50 @@ func resolveOrCreateSidecarID(ctx context.Context, client *circleci.Client, side return true, nil } +// branchSanitizer is kept for the no-session fallback path. +var branchSanitizer = regexp.MustCompile(`[^a-z0-9-]+`) + +// sidecarAutoName builds a sidecar name from workDir, the Claude session ID, +// and the current git branch. +// +// When a session ID is present the branch is encoded as an 8-hex-char suffix +// (sha256(sessionID+":"+branch)[:4]) so the raw branch name is never exposed: +// - Both present → "--" +// - Session only → "-" +// +// Without a session ID the branch is sanitised and included directly (legacy +// fallback): +// - Branch only → "--validate" +// - Neither → "-validate" +func sidecarAutoName(ctx context.Context, workDir string) string { + base := filepath.Base(workDir) + sessionID := session.IDFromCtx(ctx) + branch := sidecar.CurrentBranch(workDir) + + if sessionID != "" { + if branch != "" { + sum := sha256.Sum256([]byte(sessionID + ":" + branch)) + hash8 := fmt.Sprintf("%x", sum[:4]) + return base + "-" + sessionID + "-" + hash8 + } + return base + "-" + sessionID + } + + // No session ID: fall back to sanitised branch name for human readability. + if branch != "" { + branch = strings.ReplaceAll(branch, "/", "-") + branch = strings.ToLower(branch) + branch = branchSanitizer.ReplaceAllString(branch, "") + if len(branch) > 30 { + branch = branch[:30] + } + if branch != "" { + return base + "-" + branch + "-validate" + } + } + return base + "-validate" +} + func mapValidateError(err error) error { if errors.Is(err, validate.ErrNotConfigured) { return &userError{ diff --git a/internal/cmd/validate_test.go b/internal/cmd/validate_test.go index 86440a0b..7f17ed9d 100644 --- a/internal/cmd/validate_test.go +++ b/internal/cmd/validate_test.go @@ -3,9 +3,12 @@ package cmd import ( "bytes" "context" + "crypto/sha256" "errors" + "fmt" "net/http/httptest" "os" + "os/exec" "path/filepath" "strings" "testing" @@ -14,6 +17,7 @@ import ( "github.com/CircleCI-Public/chunk-cli/internal/circleci" "github.com/CircleCI-Public/chunk-cli/internal/config" + "github.com/CircleCI-Public/chunk-cli/internal/session" "github.com/CircleCI-Public/chunk-cli/internal/testing/fakes" ) @@ -159,3 +163,134 @@ func TestValidateEnvFlagBadValue(t *testing.T) { assert.Assert(t, err != nil) assert.Assert(t, strings.Contains(err.Error(), "BADVALUE"), "got: %v", err) } + +// gitSetup initialises a minimal git repo at dir on the given branch name. +func gitSetup(t *testing.T, dir, branch string) { + t.Helper() + run := func(args ...string) { + t.Helper() + c := exec.Command("git", append([]string{"-C", dir}, args...)...) + out, err := c.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + run("init", "-b", branch) + run("config", "user.email", "test@example.com") + run("config", "user.name", "Test") + _ = os.WriteFile(filepath.Join(dir, "README"), []byte("init"), 0o644) + run("add", ".") + run("commit", "-m", "init") +} + +func hashFor(sessionID, branch string) string { + sum := sha256.Sum256([]byte(sessionID + ":" + branch)) + return fmt.Sprintf("%x", sum[:4]) +} + +// Tests with a session ID: branch must be hashed, never appear raw. + +func TestSidecarAutoNameWithSessionAndBranch(t *testing.T) { + dir := t.TempDir() + gitSetup(t, dir, "main") + ctx := session.WithID(context.Background(), "sess-1") + got := sidecarAutoName(ctx, dir) + want := filepath.Base(dir) + "-sess-1-" + hashFor("sess-1", "main") + assert.Equal(t, got, want) +} + +func TestSidecarAutoNameWithSessionBranchWithSlashes(t *testing.T) { + dir := t.TempDir() + gitSetup(t, dir, "main") + run := func(args ...string) { + t.Helper() + c := exec.Command("git", append([]string{"-C", dir}, args...)...) + out, err := c.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + run("checkout", "-b", "feature/my-branch") + ctx := session.WithID(context.Background(), "sess-2") + got := sidecarAutoName(ctx, dir) + want := filepath.Base(dir) + "-sess-2-" + hashFor("sess-2", "feature/my-branch") + assert.Equal(t, got, want) + assert.Assert(t, !strings.Contains(got, "feature"), "raw branch must not appear in name, got %q", got) + assert.Assert(t, !strings.Contains(got, "my-branch"), "raw branch must not appear in name, got %q", got) +} + +func TestSidecarAutoNameWithSessionNoBranch(t *testing.T) { + dir := t.TempDir() + // No git repo → no branch. + ctx := session.WithID(context.Background(), "sess-3") + got := sidecarAutoName(ctx, dir) + assert.Equal(t, got, filepath.Base(dir)+"-sess-3") +} + +func TestSidecarAutoNameDifferentBranchesDifferentNames(t *testing.T) { + dir := t.TempDir() + gitSetup(t, dir, "main") + run := func(args ...string) { + t.Helper() + c := exec.Command("git", append([]string{"-C", dir}, args...)...) + out, err := c.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + ctx := session.WithID(context.Background(), "sess-x") + n1 := sidecarAutoName(ctx, dir) + run("checkout", "-b", "other-branch") + n2 := sidecarAutoName(ctx, dir) + assert.Assert(t, n1 != n2, "different branches must produce different names: %q vs %q", n1, n2) +} + +// Tests without a session ID: legacy sanitised-branch fallback. + +func TestSidecarAutoNameNoSessionBranchPresent(t *testing.T) { + dir := t.TempDir() + gitSetup(t, dir, "main") + got := sidecarAutoName(context.Background(), dir) + assert.Equal(t, got, filepath.Base(dir)+"-main-validate") +} + +func TestSidecarAutoNameNoSessionBranchAbsent(t *testing.T) { + dir := t.TempDir() + // No git repo → falls back to old format. + got := sidecarAutoName(context.Background(), dir) + assert.Equal(t, got, filepath.Base(dir)+"-validate") +} + +func TestSidecarAutoNameNoSessionBranchWithSlashes(t *testing.T) { + dir := t.TempDir() + gitSetup(t, dir, "main") + run := func(args ...string) { + t.Helper() + c := exec.Command("git", append([]string{"-C", dir}, args...)...) + out, err := c.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + run("checkout", "-b", "feature/my-branch") + got := sidecarAutoName(context.Background(), dir) + assert.Equal(t, got, filepath.Base(dir)+"-feature-my-branch-validate") +} + +func TestSidecarAutoNameNoSessionLongBranch(t *testing.T) { + dir := t.TempDir() + long := "abcdefghijklmnopqrstuvwxyz012345" // 32 chars + gitSetup(t, dir, "main") + run := func(args ...string) { + t.Helper() + c := exec.Command("git", append([]string{"-C", dir}, args...)...) + out, err := c.CombinedOutput() + if err != nil { + t.Fatalf("git %v: %v\n%s", args, err, out) + } + } + run("checkout", "-b", long) + got := sidecarAutoName(context.Background(), dir) + // branch truncated to 30 chars + assert.Equal(t, got, filepath.Base(dir)+"-"+long[:30]+"-validate") +} diff --git a/internal/sidecar/active.go b/internal/sidecar/active.go index 7d48bb31..c776cea6 100644 --- a/internal/sidecar/active.go +++ b/internal/sidecar/active.go @@ -1,11 +1,16 @@ package sidecar import ( + "bytes" "context" + "crypto/sha256" "encoding/json" "errors" + "fmt" "os" + "os/exec" "path/filepath" + "strings" "github.com/CircleCI-Public/chunk-cli/internal/config" "github.com/CircleCI-Public/chunk-cli/internal/session" @@ -18,14 +23,43 @@ type ActiveSidecar struct { Workspace string `json:"workspace,omitempty"` } -// sidecarFileName returns the name of the sidecar state file. When sessionID -// is non-empty the file is keyed to that session so concurrent Claude sessions -// in the same repo each maintain their own active sidecar. -func sidecarFileName(sessionID string) string { - if sessionID != "" { +// CurrentBranch returns the current git branch for the repo rooted at root. +// Returns "" on any error (no git, detached HEAD, etc.). +func CurrentBranch(root string) string { + var out bytes.Buffer + cmd := exec.Command("git", "-C", root, "rev-parse", "--abbrev-ref", "HEAD") + cmd.Stdout = &out + if err := cmd.Run(); err != nil { + return "" + } + b := strings.TrimSpace(out.String()) + if b == "HEAD" { + return "" // detached HEAD + } + return b +} + +// sidecarFileName returns the name of the sidecar state file. +// - Both empty → "sidecar.json" (legacy fallback) +// - Session only → "sidecar..json" (unchanged behaviour) +// - Both present → "sidecar.-.json" where hash8 is the first +// 8 hex chars of sha256(sessionID + ":" + branch), encoding the branch uniquely. +func sidecarFileName(sessionID, branch string) string { + if sessionID == "" { + return "sidecar.json" + } + if branch == "" { return "sidecar." + sessionID + ".json" } - return "sidecar.json" + sum := sha256.Sum256([]byte(sessionID + ":" + branch)) + hash8 := fmt.Sprintf("%x", sum[:4]) + return "sidecar." + sessionID + "-" + hash8 + ".json" +} + +// StateFileName returns the sidecar state file name for the given session ID +// and git branch. Exposed so acceptance tests can construct expected paths. +func StateFileName(sessionID, branch string) string { + return sidecarFileName(sessionID, branch) } // StateDir returns the XDG_DATA_HOME directory for the current project. @@ -49,7 +83,9 @@ func LoadActive(ctx context.Context) (*ActiveSidecar, error) { // LoadActiveFrom reads the active sidecar from dir. func LoadActiveFrom(ctx context.Context, dir string) (*ActiveSidecar, error) { - path, err := findSidecarFile(dir, session.IDFromCtx(ctx)) + root, _ := projectRoot() + branch := CurrentBranch(root) + path, err := findSidecarFile(dir, session.IDFromCtx(ctx), branch) if err != nil { return nil, err } @@ -85,7 +121,9 @@ func SaveActiveTo(ctx context.Context, dir string, a ActiveSidecar) error { if err != nil { return err } - return os.WriteFile(filepath.Join(dir, sidecarFileName(session.IDFromCtx(ctx))), data, 0o644) + root, _ := projectRoot() + branch := CurrentBranch(root) + return os.WriteFile(filepath.Join(dir, sidecarFileName(session.IDFromCtx(ctx), branch)), data, 0o644) } // saveDir returns the XDG_DATA_HOME directory for the current project. @@ -135,7 +173,9 @@ func ClearActive(ctx context.Context) error { // ClearActiveFrom removes the active sidecar state file in dir. func ClearActiveFrom(ctx context.Context, dir string) error { - path, err := findSidecarFile(dir, session.IDFromCtx(ctx)) + root, _ := projectRoot() + branch := CurrentBranch(root) + path, err := findSidecarFile(dir, session.IDFromCtx(ctx), branch) if err != nil { return err } @@ -146,8 +186,8 @@ func ClearActiveFrom(ctx context.Context, dir string) error { } // findSidecarFile returns the sidecar state file path in dir, or "" if it doesn't exist. -func findSidecarFile(dir, sessionID string) (string, error) { - return statOrEmpty(filepath.Join(dir, sidecarFileName(sessionID))) +func findSidecarFile(dir, sessionID, branch string) (string, error) { + return statOrEmpty(filepath.Join(dir, sidecarFileName(sessionID, branch))) } // statOrEmpty returns path if it exists, "" if it does not, or an error for other failures. diff --git a/internal/sidecar/active_test.go b/internal/sidecar/active_test.go index 3ffd8627..df11dfbf 100644 --- a/internal/sidecar/active_test.go +++ b/internal/sidecar/active_test.go @@ -2,6 +2,8 @@ package sidecar import ( "context" + "crypto/sha256" + "fmt" "os" "path/filepath" "strings" @@ -13,6 +15,11 @@ import ( "github.com/CircleCI-Public/chunk-cli/internal/session" ) +func hashFor(sessionID, branch string) string { + sum := sha256.Sum256([]byte(sessionID + ":" + branch)) + return fmt.Sprintf("%x", sum[:4]) +} + func TestSaveActiveWritesToXDGDataPath(t *testing.T) { dataHome := t.TempDir() t.Setenv(config.EnvXDGDataHome, dataHome) @@ -203,7 +210,7 @@ func TestWorkspaceOmittedWhenEmpty(t *testing.T) { stateDir, err := saveDir() assert.NilError(t, err) - data, err := os.ReadFile(filepath.Join(stateDir, sidecarFileName(""))) + data, err := os.ReadFile(filepath.Join(stateDir, sidecarFileName("", ""))) assert.NilError(t, err) assert.Assert(t, !strings.Contains(string(data), "workspace"), "empty workspace should be omitted from JSON") } @@ -240,3 +247,26 @@ func TestResolveWorkspaceDefaultFallback(t *testing.T) { got := ResolveWorkspace(context.Background(), "", "myrepo") assert.Equal(t, got, "./workspace/myrepo") } + +func TestSidecarFileNameCases(t *testing.T) { + cases := []struct { + session string + branch string + want string + }{ + {"", "", "sidecar.json"}, + {"sess-1", "", "sidecar.sess-1.json"}, + {"", "main", "sidecar.json"}, + {"sess-1", "main", "sidecar.sess-1-" + hashFor("sess-1", "main") + ".json"}, + } + for _, tc := range cases { + got := sidecarFileName(tc.session, tc.branch) + assert.Equal(t, got, tc.want, "session=%q branch=%q", tc.session, tc.branch) + } +} + +func TestSidecarFileNameHashUniquenessAcrossBranches(t *testing.T) { + f1 := sidecarFileName("sess-abc", "main") + f2 := sidecarFileName("sess-abc", "feature/my-branch") + assert.Assert(t, f1 != f2, "different branches must produce different file names") +} From 169650ad96f7aa00855b1ef64a914fd0834b81fa Mon Sep 17 00:00:00 2001 From: webster Date: Fri, 22 May 2026 16:12:40 -0700 Subject: [PATCH 2/2] Fix goconst lint: extract defaultSidecarFile constant Co-Authored-By: Claude Sonnet 4.6 --- internal/sidecar/active.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/sidecar/active.go b/internal/sidecar/active.go index c776cea6..5f19a661 100644 --- a/internal/sidecar/active.go +++ b/internal/sidecar/active.go @@ -39,6 +39,8 @@ func CurrentBranch(root string) string { return b } +const defaultSidecarFile = "sidecar.json" + // sidecarFileName returns the name of the sidecar state file. // - Both empty → "sidecar.json" (legacy fallback) // - Session only → "sidecar..json" (unchanged behaviour) @@ -46,7 +48,7 @@ func CurrentBranch(root string) string { // 8 hex chars of sha256(sessionID + ":" + branch), encoding the branch uniquely. func sidecarFileName(sessionID, branch string) string { if sessionID == "" { - return "sidecar.json" + return defaultSidecarFile } if branch == "" { return "sidecar." + sessionID + ".json"