fix(sandbox): answer the open review threads on the runtime root

Resolve the stamp reader before the stamp is opened. FILE_OVERWRITE_IF
truncates at open time, so a reader that could not be established returned
an error with the previous stamp already emptied, or a new empty one left
behind. The pathname entry point now delegates to the handle writer instead
of carrying its own copy of the sequence.

Take the Windows user scope for the fallback runtime root from the token's
user SID rather than USERNAME. Setup and a later command derive that root in
two processes with independent environments, and a USERNAME that differed
between them made the command reject the root setup had recorded.

On Unix, report a created runtime directory that exists but cannot be
identified instead of treating every lookup failure as a clean undo, which
is the line the Windows build already draws.

Move runtimeTailNotOwned and windowsSameRuntimeRootPath beside their only,
Windows-only callers so they are no longer dead code on other platforms,
replace a deprecated filepath.HasPrefix in a Unix test, and bind the
junction test's planted flag to mklink's result so a failed mklink skips
instead of failing a correct acquisition.

Author: Vasanthdev2004

internal/sandbox/runtime_compensation_other.go

@@ -3,6 +3,7 @@
 package sandbox
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -42,7 +43,15 @@ func compensateRuntimeStampBound(root string, identity string, name string, prio
 func removeCreatedRuntimeDirBound(path string, identity string) error {
 	current, ok := runtimeDirIdentity(path)
 	if !ok {
-		return nil
+		// ONLY ABSENCE MEANS THERE IS NOTHING TO UNDO. Any lookup failure used to
+		// return here, so a directory that exists but could not be identified was
+		// reported as cleanly removed while it stayed on disk. The Windows build
+		// already draws this line, and the rollback it serves promises to report
+		// what it could not remove. Reported by CodeRabbit.
+		if _, err := os.Lstat(path); errors.Is(err, os.ErrNotExist) {
+			return nil
+		}
+		return fmt.Errorf("sandbox runtime root %s was created by this run but could not be identified for removal; leaving it in place", path)
 	}
 	if current != identity {
 		return fmt.Errorf("sandbox runtime root %s is no longer the directory this run created; "+

internal/sandbox/runtime_compensation_other_test.go

@@ -0,0 +1,74 @@
+//go:build !windows
+
+package sandbox
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// ONLY ABSENCE IS NOTHING TO UNDO.
+//
+// removeCreatedRuntimeDirBound returned success on any failure to identify the
+// directory, so one that was still there, but could not be looked at, was
+// reported as removed. Here the parent loses search permission, which makes the
+// directory exist and be unidentifiable at the same time.
+func TestRemovingAnUnidentifiableCreatedDirectoryIsReported(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root bypasses directory search permission, so the lookup cannot be made to fail")
+	}
+	parent := filepath.Join(t.TempDir(), "zero")
+	created := filepath.Join(parent, "runtime")
+	if err := os.MkdirAll(created, 0o700); err != nil {
+		t.Fatalf("create the directory: %v", err)
+	}
+	identity, ok := runtimeDirIdentity(created)
+	if !ok {
+		t.Fatal("identify the created directory")
+	}
+	if err := os.Chmod(parent, 0o600); err != nil {
+		t.Fatalf("withdraw search permission: %v", err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(parent, 0o700) })
+	if _, err := os.Lstat(created); err == nil || errors.Is(err, os.ErrNotExist) {
+		t.Fatalf("SETUP INVALID: the directory should exist and be unidentifiable, Lstat said %v", err)
+	}
+
+	err := removeCreatedRuntimeDirBound(created, identity)
+	if err == nil {
+		t.Fatal("a created directory that could not be identified was reported as removed")
+	}
+	if !strings.Contains(err.Error(), "could not be identified") {
+		t.Errorf("the error does not say why the directory was left: %v", err)
+	}
+
+	// And it is still there, which is why the error is owed.
+	if err := os.Chmod(parent, 0o700); err != nil {
+		t.Fatalf("restore search permission: %v", err)
+	}
+	if _, err := os.Lstat(created); err != nil {
+		t.Fatalf("the directory was gone after all, so the error was not owed: %v", err)
+	}
+}
+
+// Absence stays a clean undo, or the test above would be satisfied by a function
+// that reports every lookup failure.
+func TestRemovingACreatedDirectoryThatIsAlreadyGoneSucceeds(t *testing.T) {
+	created := filepath.Join(t.TempDir(), "runtime")
+	if err := os.Mkdir(created, 0o700); err != nil {
+		t.Fatalf("create the directory: %v", err)
+	}
+	identity, ok := runtimeDirIdentity(created)
+	if !ok {
+		t.Fatal("identify the created directory")
+	}
+	if err := os.Remove(created); err != nil {
+		t.Fatalf("remove the directory: %v", err)
+	}
+	if err := removeCreatedRuntimeDirBound(created, identity); err != nil {
+		t.Fatalf("a created directory that is already gone was reported as a failed undo: %v", err)
+	}
+}

internal/sandbox/runtime_fallback_scope_windows_test.go

@@ -0,0 +1,41 @@
+//go:build windows
+
+package sandbox
+
+import "testing"
+
+// THE COMMAND HAS TO RECOGNISE THE ROOT SETUP RECORDED, WHATEVER ITS
+// ENVIRONMENT SAYS.
+//
+// Setup derives the fallback root in an elevated terminal and records it; a
+// command re-derives it in an ordinary one to decide whether the record belongs
+// to this workspace. The user scope in that derivation used to come from
+// USERNAME, so a value that differed between the two terminals made the command
+// reject the record and select a root with no capability ACE on it.
+func TestRecordedFallbackRootSurvivesADifferentUsernameVariable(t *testing.T) {
+	workspace := t.TempDir()
+	tempHome := t.TempDir()
+	t.Setenv("TMP", tempHome)
+	t.Setenv("TEMP", tempHome)
+
+	t.Setenv("USERNAME", "setup-terminal")
+	recorded, err := fallbackSandboxRuntimeRoot(workspace)
+	if err != nil {
+		t.Fatalf("derive the fallback root as setup: %v", err)
+	}
+	if !ownedFallbackRuntimeRecord(workspace, recorded) {
+		t.Fatalf("SETUP INVALID: the fallback root is not recognised even in the environment that derived it: %s", recorded)
+	}
+
+	t.Setenv("USERNAME", "command-terminal")
+	if !ownedFallbackRuntimeRecord(workspace, recorded) {
+		t.Fatalf("once USERNAME changed, the command no longer recognises the fallback root setup recorded: %s", recorded)
+	}
+	derived, err := fallbackSandboxRuntimeRoot(workspace)
+	if err != nil {
+		t.Fatalf("derive the fallback root as the command: %v", err)
+	}
+	if !sameWindowsRuntimeRootPath(derived, recorded) {
+		t.Fatalf("setup and the command derived different fallback roots for one workspace:\n  setup:   %s\n  command: %s", recorded, derived)
+	}
+}

internal/sandbox/runtime_lease_junction_windows_test.go

@@ -54,24 +54,32 @@ func TestRuntimeLeaseRefusesAJunctionPlantedAfterTheCheck(t *testing.T) {
 	// the pathname lease open used to follow it.
 	previous := runtimeLeasePreCreateBarrier
 	t.Cleanup(func() { runtimeLeasePreCreateBarrier = previous })
-	planted := false
+	attempted, planted := false, false
 	runtimeLeasePreCreateBarrier = func() {
-		if planted {
+		if attempted {
 			return
 		}
-		planted = true
+		attempted = true
 		if out, err := exec.Command("cmd", "/c", "mklink", "/J", owned, target).CombinedOutput(); err != nil {
 			t.Logf("mklink /J unavailable: %v: %s", err, out)
+			return
 		}
+		planted = true
 	}
 
 	lease, _, err := prepareSandboxRuntimeLeaseRecording(root)
 	if lease != nil {
 		lease.release()
 	}
-	if !planted {
+	if !attempted {
 		t.Skip("the pre-create barrier never ran, so the race was not reproduced")
 	}
+	// Bound to mklink's result, not to the attempt. When it fails, acquisition
+	// goes on to create an ordinary directory at the same name, the check below
+	// finds it, and a correct acquisition would be failed as a junction bypass.
+	if !planted {
+		t.Skip("mklink /J could not create the junction here, so the race was not reproduced")
+	}
 	if _, statErr := os.Lstat(owned); statErr != nil {
 		t.Skipf("the junction could not be created here: %v", statErr)
 	}

internal/sandbox/runtime_lease_link_unix_test.go

@@ -65,7 +65,7 @@ func TestRuntimeLeaseRefusesALinkPlantedAfterTheCheck(t *testing.T) {
 	root := unixLeaseRootUnder(t, base)
 
 	owned := filepath.Join(canonicalSandboxWorkspaceRoot(base), "zero")
-	if !filepath.HasPrefix(root, owned) {
+	if !pathWithinRoot(owned, root) {
 		t.Fatalf("SETUP INVALID: %s does not sit under the owned component %s", root, owned)
 	}
 

internal/sandbox/runtime_root_guard_windows.go

@@ -22,12 +22,22 @@ func refuseForeignRuntimeComponent(string, os.FileInfo) error {
 // Windows TEMP is already per-user, so this is belt and braces rather than the
 // load-bearing separation it is on Unix. Kept so the derived path has the same
 // shape on every platform and one code path builds it.
+//
+// FROM THE TOKEN, NOT THE ENVIRONMENT. Setup derives the fallback root in the
+// elevated terminal and records it; a command later re-derives it in an
+// ordinary one to decide whether that record belongs to this workspace. Those
+// are two processes with independent environments, so a USERNAME that differed
+// between them made the command reject the root setup had provisioned and
+// select one that carries no capability ACE. The token's user SID is the same
+// in both, because setup refuses to run as any account other than the one that
+// will use Zero. The Unix build reads the uid for the same reason. Reported by
+// CodeRabbit.
 func sandboxRuntimeUserScope() string {
-	name := strings.TrimSpace(os.Getenv("USERNAME"))
-	if name == "" {
+	sid, err := currentProcessSID()
+	if err != nil {
 		return "u"
 	}
-	return "u" + strings.ToLower(name)
+	return "u" + strings.ToLower(sid)
 }
 
 // sandboxRuntimeFallbackOwnedNames are the components the temp-derived runtime

internal/sandbox/windows_acl_apply_windows.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
 	"sort"
 	"strings"
 	"unsafe"
@@ -147,6 +148,19 @@ func verifyStampRootIdentity(handle windows.Handle, path string, stamp *windowsA
 // Nil in production.
 var windowsACLStampIdentitySwapHook func(path string)
 
+// windowsSameRuntimeRootPath compares two runtime roots the way the filesystem
+// does, so the stamp rides along with the right target regardless of spelling.
+// Beside its only caller: in the shared file it was dead code on every other
+// platform.
+func windowsSameRuntimeRootPath(left, right string) bool {
+	left = filepath.Clean(strings.TrimSpace(left))
+	right = filepath.Clean(strings.TrimSpace(right))
+	if left == "" || right == "" {
+		return false
+	}
+	return strings.EqualFold(left, right)
+}
+
 // writeRidingStamp writes the stamp through the handle the capability ACE was
 // applied on, or through the test hook when one is installed.
 func writeRidingStamp(handle windows.Handle, path string, planHash string) error {

internal/sandbox/windows_runtime_tail.go

@@ -2,9 +2,7 @@ package sandbox
 
 import (
 	"errors"
-	"fmt"
 	"path/filepath"
-	"runtime"
 	"strings"
 )
 
@@ -65,24 +63,6 @@ func windowsSandboxRuntimeOwnedTail(root string) (string, []string, bool) {
 // Callers must fail rather than quietly opening it by name.
 var errRuntimeTailNotOwned = errors.New("path is not a sandbox runtime root")
 
-// windowsSameRuntimeRootPath compares two runtime roots the way the filesystem
-// does, so the stamp rides along with the right target regardless of spelling.
-func windowsSameRuntimeRootPath(left, right string) bool {
-	left = filepath.Clean(strings.TrimSpace(left))
-	right = filepath.Clean(strings.TrimSpace(right))
-	if left == "" || right == "" {
-		return false
-	}
-	if runtime.GOOS == "windows" {
-		return strings.EqualFold(left, right)
-	}
-	return left == right
-}
-
-func runtimeTailNotOwned(root string) error {
-	return fmt.Errorf("%w: %s", errRuntimeTailNotOwned, root)
-}
-
 // windowsRuntimeOwnedNameMatches reports whether one component of a candidate
 // tail is a name Zero owns at that position.
 //