Address PR review comments: fix ref counting, dedup resolveAuth, remove unused CacheStatus

- Fix Release() ref counting bug: avoid calling getRef() which increments
  count, instead look up ref directly and clean up when count reaches zero
- Remove unpopulated CacheStatus struct and fields from types, deepcopy,
  and CRD manifest since nothing writes to them
- Extract duplicated resolveAuth into shared pkg/gitauth package with
  explicit key validation for missing "username"/"password" keys
- Fix GC() race condition by skipping paths with active in-process refs
- Remove misleading WorkspaceCacheMiss metric increment for in-memory
  (non-cache) code paths
- Change push controller from shallow to full clones to avoid silent
  failures when transactions reference historical commits
- Add tests for ref count cleanup, ref leak prevention, GC active ref
  skipping, and gitauth key validation

https://claude.ai/code/session_01WJE6ozYXZMn1CD6d4vy8b6

pkg/workspace/workspace.go

@@ -107,10 +107,8 @@ func (m *Manager) acquireInMemory(ctx context.Context, repoURL string, auth *htt
 	})
 	metrics.WorkspaceAcquireDuration.WithLabelValues("memory").Observe(time.Since(start).Seconds())
 	if err != nil {
-		metrics.WorkspaceCacheMiss.Inc()
 		return nil, fmt.Errorf("in-memory clone: %w", err)
 	}
-	metrics.WorkspaceCacheMiss.Inc()
 
 	return &Workspace{
 		Repo: repo,
@@ -210,8 +208,18 @@ func (m *Manager) Release(ws *Workspace) {
 	if ws == nil || ws.manager == nil {
 		return
 	}
-	r := m.getRef(ws.path)
+	m.mu.Lock()
+	r, ok := m.active[ws.path]
+	m.mu.Unlock()
+	if !ok {
+		return
+	}
 	r.count--
+	if r.count == 0 {
+		m.mu.Lock()
+		delete(m.active, ws.path)
+		m.mu.Unlock()
+	}
 	r.mu.Unlock()
 }
 
@@ -229,6 +237,8 @@ func (m *Manager) getRef(path string) *ref {
 }
 
 // GC removes cached directories that are not in the activeRepos set.
+// It skips paths that have active in-process references to avoid racing
+// with ongoing reconciles.
 func (m *Manager) GC(ctx context.Context, activeRepos map[string]bool) {
 	if m.basePath == "" {
 		return
@@ -246,11 +256,20 @@ func (m *Manager) GC(ctx context.Context, activeRepos map[string]bool) {
 			continue
 		}
 		dirPath := filepath.Join(m.basePath, entry.Name())
-		if !activeRepos[dirPath] {
-			logger.Infof("GC: removing stale cache dir %s", dirPath)
-			if err := os.RemoveAll(dirPath); err != nil {
-				logger.Warnf("GC: removing %s: %v", dirPath, err)
-			}
+		if activeRepos[dirPath] {
+			continue
+		}
+		// Skip paths with active in-process references.
+		m.mu.Lock()
+		r, hasRef := m.active[dirPath]
+		m.mu.Unlock()
+		if hasRef && r.count > 0 {
+			logger.Infof("GC: skipping %s (active references)", dirPath)
+			continue
+		}
+		logger.Infof("GC: removing stale cache dir %s", dirPath)
+		if err := os.RemoveAll(dirPath); err != nil {
+			logger.Warnf("GC: removing %s: %v", dirPath, err)
 		}
 	}
 }

pkg/workspace/workspace_test.go

@@ -424,3 +424,91 @@ func TestInMemoryStorerCompatibility(t *testing.T) {
 		t.Fatal("repo is nil")
 	}
 }
+
+// Test that Release properly decrements ref count and cleans up the active map.
+func TestRelease_RefCountCleanup(t *testing.T) {
+	sourceDir := t.TempDir()
+	barePath, _ := initBareRepo(t, sourceDir)
+
+	cacheDir := t.TempDir()
+	m := NewManager(cacheDir, false)
+	ctx := testCtx(t)
+
+	ws, err := m.Acquire(ctx, barePath, nil, true)
+	if err != nil {
+		t.Fatalf("Acquire: %v", err)
+	}
+
+	// After acquire, ref should exist with count 1.
+	m.mu.Lock()
+	r, ok := m.active[ws.path]
+	m.mu.Unlock()
+	if !ok {
+		t.Fatal("expected active ref after acquire")
+	}
+	if r.count != 1 {
+		t.Errorf("ref count after acquire = %d, want 1", r.count)
+	}
+
+	m.Release(ws)
+
+	// After release, the ref should be cleaned up from the active map.
+	m.mu.Lock()
+	_, ok = m.active[ws.path]
+	m.mu.Unlock()
+	if ok {
+		t.Error("expected active ref to be cleaned up after release")
+	}
+}
+
+// Test that multiple acquire/release cycles don't leak refs.
+func TestRelease_NoRefLeak(t *testing.T) {
+	sourceDir := t.TempDir()
+	barePath, _ := initBareRepo(t, sourceDir)
+
+	cacheDir := t.TempDir()
+	m := NewManager(cacheDir, false)
+	ctx := testCtx(t)
+
+	for i := 0; i < 5; i++ {
+		ws, err := m.Acquire(ctx, barePath, nil, true)
+		if err != nil {
+			t.Fatalf("Acquire #%d: %v", i, err)
+		}
+		m.Release(ws)
+	}
+
+	// After all cycles, active map should be empty.
+	m.mu.Lock()
+	activeCount := len(m.active)
+	m.mu.Unlock()
+	if activeCount != 0 {
+		t.Errorf("active map has %d entries, want 0 after all releases", activeCount)
+	}
+}
+
+// Test that GC skips directories with active references.
+func TestGC_SkipsActiveRefs(t *testing.T) {
+	sourceDir := t.TempDir()
+	barePath, _ := initBareRepo(t, sourceDir)
+
+	cacheDir := t.TempDir()
+	m := NewManager(cacheDir, false)
+	ctx := testCtx(t)
+
+	// Acquire a workspace (creates a cached dir and holds a ref).
+	ws, err := m.Acquire(ctx, barePath, nil, true)
+	if err != nil {
+		t.Fatalf("Acquire: %v", err)
+	}
+
+	// Run GC with empty activeRepos — the cache dir should survive because
+	// there's an active in-process reference.
+	m.GC(ctx, map[string]bool{})
+
+	if _, err := os.Stat(ws.path); os.IsNotExist(err) {
+		t.Error("GC should not remove dir with active references")
+	}
+
+	m.Release(ws)
+}