Close diagnostic and restore coverage gaps

docs/roadmap/implementation.md

@@ -56,7 +56,7 @@ the full audit or any audit line range.
 | 32 | Reconciled lifecycle/analyze documentation and closed the original audit traceability inventory. | COM-002, COM-005 | Completed |
 | 33 | Confine diagnostic log destinations and eliminate pathname-based tail reads. | Follow-up review | Completed |
 | 34 | Dispose of owned subprocess descendants after natural leader exit. | Follow-up review | Completed |
-| 35 | Bound remote current-state and lock control-plane reads. | Follow-up review | Pending |
+| 35 | Bound remote current-state and lock control-plane reads. | Follow-up review | Completed |
 
 Completed stages must not be reimplemented wholesale. A pending stage may adjust
 their code only where its stated remediation requires it.

internal/app/restore_execution_test.go

@@ -357,7 +357,7 @@ func TestExecuteRestoreLockConflictFailsAndWritesNothing(t *testing.T) {
 	}
 }
 
-func TestExecuteRestoreInvalidManifestDoesNotCorruptExistingManifest(t *testing.T) {
+func TestExecuteRestorePlanInvalidManifestDoesNotCorruptExistingManifest(t *testing.T) {
 	cfg := restorePlanConfig(t)
 	store := &storage.FakeBackend{}
 	current := seedCommittedRestoreSnapshot(t, cfg, store, "20260519T010203Z-a1b2c3d4", map[string][]byte{
@@ -417,6 +417,87 @@ func TestExecuteRestoreInvalidManifestDoesNotCorruptExistingManifest(t *testing.
 	}
 }
 
+func TestExecuteRestoreInvalidManifestReportsFailureAndRetainsRecoveryMarker(t *testing.T) {
+	workspaceRoot := t.TempDir()
+	pipelinePath, campaignPath, sessionPath := writeValidConfigFiles(t, workspaceRoot)
+	cfg, err := config.LoadWithSessionOptions(pipelinePath, campaignPath, sessionPath, config.SessionLoadOptions{})
+	if err != nil {
+		t.Fatalf("LoadWithSessionOptions() error = %v", err)
+	}
+	store := &storage.FakeBackend{}
+	seedCommittedRestoreSnapshot(t, cfg, store, "20260519T010203Z-a1b2c3d4", map[string][]byte{
+		"transcripts/full.json": []byte("remote-transcript"),
+	})
+
+	sessionRoot := artifacts.SessionWorkDirForCampaign(workspaceRoot, cfg.Session.Campaign, cfg.Session.SessionID)
+	existing := manifest.New(cfg.Session.SessionID, nowUTC())
+	existing.Campaign = cfg.Session.Campaign
+	existingPath := filepath.Join(sessionRoot, "manifest.json")
+	manifestStore := &manifest.LocalStore{}
+	if err := manifestStore.Save(context.Background(), existingPath, existing); err != nil {
+		t.Fatalf("save existing local manifest: %v", err)
+	}
+	existingData, err := os.ReadFile(existingPath)
+	if err != nil {
+		t.Fatalf("read existing local manifest: %v", err)
+	}
+
+	restoreWithStoreAndRealPhases(t, store)
+	realBuildRestorePlan := buildRestorePlanFn
+	buildRestorePlanFn = func(ctx context.Context, cfg *config.Config, current *RemoteCurrentState, objectStore storage.ObjectStore, opts RestorePlanOptions) (*RestorePlan, error) {
+		plan, err := realBuildRestorePlan(ctx, cfg, current, objectStore, opts)
+		if err != nil {
+			return nil, err
+		}
+		invalidManifest := []byte("{invalid json")
+		for index := range plan.Actions {
+			if plan.Actions[index].LocalRelativePath != config.PathManifestFile {
+				continue
+			}
+			plan.Actions[index].VerifiedContent = invalidManifest
+			plan.Actions[index].SHA256 = restoreCommitSHA256(invalidManifest)
+			plan.Actions[index].Size = int64(len(invalidManifest))
+			store.SeedObject(storage.FakeObject{
+				Key:  plan.Actions[index].RemoteKey,
+				Data: invalidManifest,
+				ETag: plan.Actions[index].Generation,
+			})
+			return plan, nil
+		}
+		t.Fatal("restore plan has no manifest action")
+		return nil, nil
+	}
+
+	var stdout bytes.Buffer
+	var stderr bytes.Buffer
+	code := Execute([]string{"session", "restore", cfg.Session.SessionID, "--config", pipelinePath, "--campaign-file", campaignPath, "--session", sessionPath, "--force"}, &stdout, &stderr)
+	if code == 0 {
+		t.Fatal("exit code = 0, want non-zero")
+	}
+	if !strings.Contains(stderr.String(), "validate manifest decode") {
+		t.Fatalf("stderr = %q, want manifest validation failure", stderr.String())
+	}
+	mustReadEquals(t, filepath.Join(sessionRoot, "transcripts", "full.json"), "remote-transcript")
+	report := mustReadRestoreReport(t, filepath.Join(sessionRoot, "reports", "restore-latest.json"))
+	if report.Status != "failed" {
+		t.Fatalf("report status = %q, want failed", report.Status)
+	}
+	if strings.TrimSpace(report.Error) == "" {
+		t.Fatal("report error is empty, want failure context")
+	}
+	afterData, err := os.ReadFile(existingPath)
+	if err != nil {
+		t.Fatalf("read local manifest after failure: %v", err)
+	}
+	if string(afterData) != string(existingData) {
+		t.Fatalf("local manifest changed after failed restore; before=%q after=%q", string(existingData), string(afterData))
+	}
+	markerPath := artifacts.SessionRestoreMarkerPathForCampaign(workspaceRoot, cfg.Session.Campaign, cfg.Session.SessionID)
+	if _, err := os.Stat(markerPath); err != nil {
+		t.Fatalf("incomplete restore marker should remain after failed forced restore: %v", err)
+	}
+}
+
 func TestExecuteRestorePlanPathMismatchFails(t *testing.T) {
 	cfg := restorePlanConfig(t)
 	current := restorePlanCurrentState(t, cfg)

internal/fileops/confined.go

@@ -156,13 +156,17 @@ func setOpenedDirectoryMode(root *os.Root, mode os.FileMode) error {
 }
 
 func createSiblingTemp(parent *os.Root, base string) (*os.File, string, error) {
+	return createSiblingTempWithFlags(parent, base, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600)
+}
+
+func createSiblingTempWithFlags(parent *os.Root, base string, flags int, mode os.FileMode) (*os.File, string, error) {
 	for attempt := 0; attempt < 100; attempt++ {
 		var randomBytes [16]byte
 		if _, err := rand.Read(randomBytes[:]); err != nil {
 			return nil, "", fmt.Errorf("generate temporary file name: %w", err)
 		}
 		name := "." + base + ".tmp-" + hex.EncodeToString(randomBytes[:])
-		file, err := parent.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600)
+		file, err := parent.OpenFile(name, flags|os.O_CREATE|os.O_EXCL, mode)
 		if errors.Is(err, os.ErrExist) {
 			continue
 		}
@@ -184,28 +188,128 @@ func syncOpenedDirectory(parent *os.Root) error {
 }
 
 // OpenFileConfined opens a file after verifying its parent hierarchy without
-// following symbolic links. Existing symbolic-link and non-regular leaves are
+// mutating through symbolic links. Truncating opens atomically replace the leaf
+// with a newly created sibling, while other creating opens never carry O_CREATE
+// into an existing-leaf open. Existing symbolic-link and non-regular leaves are
 // rejected.
 func OpenFileConfined(path string, flags int, mode os.FileMode) (*os.File, error) {
+	return openFileConfined(path, flags, mode, nil)
+}
+
+func openFileConfined(path string, flags int, mode os.FileMode, beforeOpen func() error) (*os.File, error) {
 	parent, name, err := openConfinedParent(path, false, 0)
 	if err != nil {
 		return nil, err
 	}
 	defer func() { _ = parent.Close() }()
-	if info, err := parent.Lstat(name); err == nil {
-		if info.Mode()&os.ModeSymlink != 0 {
-			return nil, fmt.Errorf("destination file %q is a symbolic link", name)
+
+	if flags&(os.O_CREATE|os.O_EXCL) == os.O_CREATE|os.O_EXCL {
+		if err := runBeforeConfinedOpen(beforeOpen); err != nil {
+			return nil, err
 		}
-		if !info.Mode().IsRegular() {
-			return nil, fmt.Errorf("destination file %q is not a regular file", name)
+		file, err := parent.OpenFile(name, flags, mode)
+		if err != nil {
+			return nil, err
 		}
-	} else if err != nil && !errors.Is(err, os.ErrNotExist) {
-		return nil, fmt.Errorf("inspect destination file %q: %w", name, err)
+		return validateOpenedConfinedFile(parent, name, file)
 	}
-	file, err := parent.OpenFile(name, flags, mode)
+
+	exists, err := validateConfinedLeaf(parent, name)
 	if err != nil {
 		return nil, err
 	}
+	if flags&os.O_TRUNC != 0 {
+		if !exists && flags&os.O_CREATE == 0 {
+			return nil, fmt.Errorf("open destination file %q: %w", name, os.ErrNotExist)
+		}
+		return replaceAndOpenConfinedFile(parent, name, flags, mode, beforeOpen)
+	}
+
+	if err := runBeforeConfinedOpen(beforeOpen); err != nil {
+		return nil, err
+	}
+	if !exists && flags&os.O_CREATE != 0 {
+		file, err := parent.OpenFile(name, flags|os.O_EXCL, mode)
+		if err == nil {
+			return validateOpenedConfinedFile(parent, name, file)
+		}
+		if !errors.Is(err, os.ErrExist) {
+			return nil, err
+		}
+		if _, validationErr := validateConfinedLeaf(parent, name); validationErr != nil {
+			return nil, validationErr
+		}
+	}
+
+	// The leaf now exists, so removing O_CREATE prevents a raced dangling
+	// symlink from creating its target. Opening an existing symlink can expose a
+	// handle, but no caller can mutate through it before the identity check below.
+	file, err := parent.OpenFile(name, flags&^os.O_CREATE, mode)
+	if err != nil {
+		return nil, err
+	}
+	return validateOpenedConfinedFile(parent, name, file)
+}
+
+func validateConfinedLeaf(parent *os.Root, name string) (bool, error) {
+	info, err := parent.Lstat(name)
+	if errors.Is(err, os.ErrNotExist) {
+		return false, nil
+	}
+	if err != nil {
+		return false, fmt.Errorf("inspect destination file %q: %w", name, err)
+	}
+	if info.Mode()&os.ModeSymlink != 0 {
+		return false, fmt.Errorf("destination file %q is a symbolic link", name)
+	}
+	if !info.Mode().IsRegular() {
+		return false, fmt.Errorf("destination file %q is not a regular file", name)
+	}
+	return true, nil
+}
+
+func replaceAndOpenConfinedFile(parent *os.Root, name string, flags int, mode os.FileMode, beforeOpen func() error) (_ *os.File, resultErr error) {
+	temporary, temporaryName, err := createSiblingTempWithFlags(parent, name, flags&^os.O_TRUNC, mode)
+	if err != nil {
+		return nil, fmt.Errorf("create replacement for destination file %q: %w", name, err)
+	}
+	installed := false
+	defer func() {
+		if installed {
+			return
+		}
+		closeErr := temporary.Close()
+		removeErr := parent.Remove(temporaryName)
+		if errors.Is(removeErr, os.ErrNotExist) {
+			removeErr = nil
+		}
+		resultErr = errors.Join(resultErr, closeErr, removeErr)
+	}()
+
+	if err := runBeforeConfinedOpen(beforeOpen); err != nil {
+		return nil, err
+	}
+	// Rename replaces the directory entry itself rather than following it. A
+	// leaf changed to a symlink after validation therefore cannot redirect the
+	// truncation to its target.
+	if err := parent.Rename(temporaryName, name); err != nil {
+		return nil, fmt.Errorf("install replacement for destination file %q: %w", name, err)
+	}
+	installed = true
+	return validateOpenedConfinedFile(parent, name, temporary)
+}
+
+func runBeforeConfinedOpen(beforeOpen func() error) error {
+	if beforeOpen == nil {
+		return nil
+	}
+	if err := beforeOpen(); err != nil {
+		return fmt.Errorf("check before confined file open: %w", err)
+	}
+	return nil
+}
+
+func validateOpenedConfinedFile(parent *os.Root, name string, file *os.File) (*os.File, error) {
 	opened, err := file.Stat()
 	if err != nil {
 		_ = file.Close()

internal/fileops/confined_open_test.go

@@ -54,3 +54,95 @@ func TestOpenFileConfinedRejectsSymlinkWithoutTruncatingTarget(t *testing.T) {
 		t.Fatalf("target content = %q, want %q", data, original)
 	}
 }
+
+func TestOpenFileConfinedDoesNotFollowLeafReplacedBeforeTruncate(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("creating symlinks requires privileges that are not available on every Windows runner")
+	}
+	dir := t.TempDir()
+	target := filepath.Join(dir, "target.log")
+	const original = "unrelated content"
+	if err := os.WriteFile(target, []byte(original), 0o600); err != nil {
+		t.Fatalf("WriteFile(target) error = %v", err)
+	}
+	leaf := filepath.Join(dir, "diagnostic.log")
+	if err := os.WriteFile(leaf, []byte("old diagnostic"), 0o600); err != nil {
+		t.Fatalf("WriteFile(leaf) error = %v", err)
+	}
+
+	file, err := openFileConfined(leaf, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, WorkspaceFileMode, func() error {
+		if err := os.Remove(leaf); err != nil {
+			return err
+		}
+		return os.Symlink(filepath.Base(target), leaf)
+	})
+	if err != nil {
+		t.Fatalf("openFileConfined() error = %v", err)
+	}
+	if _, err := file.WriteString("new diagnostic"); err != nil {
+		_ = file.Close()
+		t.Fatalf("WriteString() error = %v", err)
+	}
+	if err := file.Close(); err != nil {
+		t.Fatalf("Close() error = %v", err)
+	}
+
+	targetData, err := os.ReadFile(target)
+	if err != nil {
+		t.Fatalf("ReadFile(target) error = %v", err)
+	}
+	if string(targetData) != original {
+		t.Fatalf("target content = %q, want %q", targetData, original)
+	}
+	leafData, err := os.ReadFile(leaf)
+	if err != nil {
+		t.Fatalf("ReadFile(leaf) error = %v", err)
+	}
+	if string(leafData) != "new diagnostic" {
+		t.Fatalf("leaf content = %q, want replacement diagnostic", leafData)
+	}
+	info, err := os.Lstat(leaf)
+	if err != nil {
+		t.Fatalf("Lstat(leaf) error = %v", err)
+	}
+	if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() {
+		t.Fatalf("leaf mode = %v, want regular file", info.Mode())
+	}
+}
+
+func TestOpenFileConfinedRejectsLeafReplacedBeforeExistingFileOpen(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("creating symlinks requires privileges that are not available on every Windows runner")
+	}
+	dir := t.TempDir()
+	target := filepath.Join(dir, "target.lock")
+	const original = "unrelated lock content"
+	if err := os.WriteFile(target, []byte(original), 0o600); err != nil {
+		t.Fatalf("WriteFile(target) error = %v", err)
+	}
+	leaf := filepath.Join(dir, "session.lock")
+	if err := os.WriteFile(leaf, []byte("old lock"), 0o600); err != nil {
+		t.Fatalf("WriteFile(leaf) error = %v", err)
+	}
+
+	file, err := openFileConfined(leaf, os.O_RDWR|os.O_CREATE, WorkspaceFileMode, func() error {
+		if err := os.Remove(leaf); err != nil {
+			return err
+		}
+		return os.Symlink(filepath.Base(target), leaf)
+	})
+	if file != nil {
+		_ = file.Close()
+		t.Fatal("openFileConfined() returned a raced symbolic-link target")
+	}
+	if err == nil || !strings.Contains(err.Error(), "changed while being opened") {
+		t.Fatalf("openFileConfined() error = %v, want identity-change rejection", err)
+	}
+	targetData, readErr := os.ReadFile(target)
+	if readErr != nil {
+		t.Fatalf("ReadFile(target) error = %v", readErr)
+	}
+	if string(targetData) != original {
+		t.Fatalf("target content = %q, want %q", targetData, original)
+	}
+}