fix(shellwrap): unify ResolveDockerSource with ResolveDockerPath cache path (Codex P2 r4)

ResolveDockerSource honored a live cached negative by returning 'absent'
WITHOUT running the well-known-path stat probe that ResolveDockerPath uses
during the negative-TTL window — so docker_cli_source reported 'absent' while
ResolveDockerPath returned the bundled path (schema-v5 telemetry mismatch).

Root-cause the recurring divergence (this is the 2nd source-tracking drift
between the two functions) by extracting a single shared resolveDockerPathLocked
helper that is the only writer of the cache + dockerPathSource. ResolveDockerPath
and ResolveDockerSource now drive the identical cache path, including the
MCP-2744 override, so they cannot disagree on the same cache state.

Regression test asserts ResolveDockerSource reports 'bundled' (not 'absent')
when a well-known docker appears during the negative-TTL window — verified
red->green against the shared-resolver refactor.

Related #696

Co-Authored-By: Paperclip <noreply@paperclip.ing>

Author: Dumbris

internal/shellwrap/shellwrap.go

@@ -228,7 +228,16 @@ func probeWellKnownDocker(logger *zap.Logger) string {
 func ResolveDockerPath(logger *zap.Logger) (string, error) {
 	dockerPathMu.Lock()
 	defer dockerPathMu.Unlock()
+	return resolveDockerPathLocked(logger)
+}
 
+// resolveDockerPathLocked is the single cache-aware resolver. Callers MUST hold
+// dockerPathMu. It is the one place the docker-path cache (path, err, expiry)
+// AND the parallel dockerPathSource enum are written, so ResolveDockerPath and
+// ResolveDockerSource can never diverge on the same cache state — including the
+// MCP-2744 stat-probe override. (Earlier the two functions duplicated this
+// logic and the source-tracking drifted between them.)
+func resolveDockerPathLocked(logger *zap.Logger) (string, error) {
 	// Honor cache: keep successful resolutions forever.
 	if dockerPathHasResult && dockerPathErr == nil {
 		return dockerPath, nil
@@ -273,33 +282,18 @@ func ResolveDockerPath(logger *zap.Logger) (string, error) {
 // ResolveDockerSource returns the coarse, fixed-enum label describing how the
 // docker CLI was resolved (DockerSourcePath / DockerSourceBundled /
 // DockerSourceLoginShell), or DockerSourceAbsent when docker cannot be found.
-// It shares the same cache as ResolveDockerPath, so a prior successful or
-// failed resolution is reused (honoring the negative TTL). Never returns the
-// resolved path — only the branch — so callers (telemetry) cannot leak it.
+// It drives the SAME cache path as ResolveDockerPath (via resolveDockerPathLocked),
+// so the reported source always matches the resolution ResolveDockerPath would
+// give for the current cache state — including the MCP-2744 stat-probe override
+// during the negative-TTL window. Never returns the resolved path — only the
+// branch — so callers (telemetry) cannot leak it.
 func ResolveDockerSource(logger *zap.Logger) string {
 	dockerPathMu.Lock()
 	defer dockerPathMu.Unlock()
-
-	if dockerPathHasResult {
-		if dockerPathErr == nil {
-			return sourceOrAbsent(dockerPathSource)
-		}
-		if !dockerPathExpires.IsZero() && time.Now().Before(dockerPathExpires) {
-			return DockerSourceAbsent
-		}
-	}
-
-	path, source, err := resolveDockerPathUncached(logger)
-	dockerPath = path
-	dockerPathSource = source
-	dockerPathErr = err
-	dockerPathHasResult = true
-	if err != nil {
-		dockerPathExpires = time.Now().Add(dockerPathNegativeTTL)
+	if _, err := resolveDockerPathLocked(logger); err != nil {
 		return DockerSourceAbsent
 	}
-	dockerPathExpires = time.Time{}
-	return sourceOrAbsent(source)
+	return sourceOrAbsent(dockerPathSource)
 }
 
 // sourceOrAbsent normalizes an empty source string to DockerSourceAbsent so the

internal/shellwrap/shellwrap_test.go

@@ -393,6 +393,46 @@ func TestResolveDockerSource_StatProbeUpgradeReportsBundled(t *testing.T) {
 		"source must upgrade to bundled when the stat probe overrides the cached negative")
 }
 
+// TestResolveDockerSource_NegativeTTLProbeOverride is the Codex round-4
+// regression guard: ResolveDockerSource must apply the same well-known-path
+// stat-probe override that ResolveDockerPath does during the negative-TTL
+// window. Before the shared-resolver refactor, ResolveDockerSource returned
+// "absent" off the cached negative without ever probing, so docker_cli_source
+// reported the wrong source while ResolveDockerPath returned the bundled path.
+func TestResolveDockerSource_NegativeTTLProbeOverride(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("unix-only fixture")
+	}
+	resetDockerPathCacheForTest()
+	t.Cleanup(resetDockerPathCacheForTest)
+
+	// FIRST resolution fails everywhere → caches a LIVE negative (default TTL).
+	prevPaths := wellKnownDockerPathsFn
+	wellKnownDockerPathsFn = func() []string { return nil }
+	t.Cleanup(func() { wellKnownDockerPathsFn = prevPaths })
+
+	t.Setenv("PATH", t.TempDir())
+	shellDir := t.TempDir()
+	t.Setenv("SHELL", writeFakeShell(t, shellDir, ""))
+
+	_, err := ResolveDockerPath(nil)
+	require.Error(t, err, "first call should cache a live negative")
+
+	// Docker now appears at a well-known path. The very next ResolveDockerSource
+	// call must run the stat probe (not honor the cached "absent") and report
+	// bundled — without any TTL expiry.
+	dockerDir := t.TempDir()
+	want := writeFakeDocker(t, dockerDir)
+	wellKnownDockerPathsFn = func() []string { return []string{want} }
+
+	require.Equal(t, DockerSourceBundled, ResolveDockerSource(nil),
+		"ResolveDockerSource must apply the stat-probe override during the negative-TTL window")
+	// And ResolveDockerPath agrees (cache was upgraded to a permanent success).
+	got, err := ResolveDockerPath(nil)
+	require.NoError(t, err)
+	require.Equal(t, want, got)
+}
+
 func TestMinimalEnv_DropsSecrets(t *testing.T) {
 	t.Setenv("AWS_ACCESS_KEY_ID", "AKIA_test_dummy_value_00000000")
 	t.Setenv("GITHUB_TOKEN", "ghp_dummy_test_token_1234567890abcdef")