Address preserved path review feedback

2026-10-25 18:28:38 +00:00 · 2026-04-27 10:51:25 -07:00
parent 90d5f4304f
commit c5ddf213fe
12 changed files with 1768 additions and 357 deletions
@@ -19,6 +19,61 @@ use crate::protocol::NetworkAccess;
 use crate::protocol::SandboxPolicy;
 use crate::protocol::WritableRoot;

+const PRESERVED_GIT_PATH_NAME: &str = ".git";
+const PRESERVED_AGENTS_PATH_NAME: &str = ".agents";
+const PRESERVED_CODEX_PATH_NAME: &str = ".codex";
+
+/// Top-level workspace metadata paths that stay protected under writable roots.
+pub const PRESERVED_PATH_NAMES: &[&str] = &[
+    PRESERVED_GIT_PATH_NAME,
+    PRESERVED_AGENTS_PATH_NAME,
+    PRESERVED_CODEX_PATH_NAME,
+];
+
+/// Returns true when a path basename is one of the preserved workspace metadata names.
+pub fn is_preserved_path_name(name: &OsStr) -> bool {
+    PRESERVED_PATH_NAMES
+        .iter()
+        .any(|preserved| name == OsStr::new(preserved))
+}
+
+/// Returns the preserved workspace metadata name when an agent write to `path`
+/// should be blocked before execution.
+pub fn forbidden_agent_preserved_path_write(
+    path: &Path,
+    cwd: &Path,
+    file_system_sandbox_policy: &FileSystemSandboxPolicy,
+) -> Option<&'static str> {
+    if !matches!(
+        file_system_sandbox_policy.kind,
+        FileSystemSandboxKind::Restricted
+    ) {
+        return None;
+    }
+
+    let target = resolve_candidate_path(path, cwd)?;
+    let (preserved_path, preserved_name) = first_preserved_component(target.as_path())?;
+    if has_explicit_write_entry_for_path(file_system_sandbox_policy, &preserved_path, cwd) {
+        return None;
+    }
+
+    if !file_system_sandbox_policy.can_write_path_with_cwd(preserved_path.as_path(), cwd) {
+        return Some(preserved_name);
+    }
+
+    // A child directory of an existing Git repository must keep normal Git
+    // discovery working, so the sandbox layer intentionally does not
+    // materialize a missing child `.git`. Block that creation at command time.
+    if preserved_name == PRESERVED_GIT_PATH_NAME
+        && !preserved_path.as_path().exists()
+        && has_ancestor_git_metadata(preserved_path.as_path())
+    {
+        return Some(preserved_name);
+    }
+
+    None
+}
+
 #[derive(
    Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Display, Default, JsonSchema, TS,
 )]
@@ -474,27 +529,17 @@ impl FileSystemSandboxPolicy {
    /// into split filesystem policy.
    pub fn from_legacy_sandbox_policy_for_cwd(sandbox_policy: &SandboxPolicy, cwd: &Path) -> Self {
        let mut file_system_policy = Self::from(sandbox_policy);
-        if let SandboxPolicy::WorkspaceWrite { writable_roots, .. } = sandbox_policy {
+        if let SandboxPolicy::WorkspaceWrite { .. } = sandbox_policy {
            let legacy_writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd);
            prune_read_entries_under_writable_roots(
                &mut file_system_policy.entries,
                &legacy_writable_roots,
            );

-            if let Ok(cwd_root) = AbsolutePathBuf::from_absolute_path(cwd) {
+            for writable_root in legacy_writable_roots {
                for protected_path in default_read_only_subpaths_for_writable_root(
-                    &cwd_root, /*protect_missing_preserved_paths*/ true,
-                ) {
-                    append_default_read_only_path_if_no_explicit_rule(
-                        &mut file_system_policy.entries,
-                        protected_path,
-                    );
-                }
-            }
-            for writable_root in writable_roots {
-                for protected_path in default_read_only_subpaths_for_writable_root(
-                    writable_root,
-                    /*protect_missing_preserved_paths*/ false,
+                    &writable_root.root,
+                    /*protect_missing_preserved_paths*/ true,
                ) {
                    append_default_read_only_path_if_no_explicit_rule(
                        &mut file_system_policy.entries,
@@ -687,13 +732,9 @@ impl FileSystemSandboxPolicy {
                .iter()
                .filter(|path| normalize_effective_absolute_path((*path).clone()) == root)
                .collect();
-            let protect_missing_preserved_paths = AbsolutePathBuf::from_absolute_path(cwd)
-                .ok()
-                .is_some_and(|cwd| normalize_effective_absolute_path(cwd) == root);
            let mut read_only_subpaths: Vec<AbsolutePathBuf> =
                default_read_only_subpaths_for_writable_root(
-                    &root,
-                    protect_missing_preserved_paths,
+                    &root, /*protect_missing_preserved_paths*/ true,
                )
                .into_iter()
                .filter(|path| !has_explicit_resolved_path_entry(&resolved_entries, path))
@@ -1301,18 +1342,21 @@ fn normalize_effective_absolute_path(path: AbsolutePathBuf) -> AbsolutePathBuf {
    path
 }

-fn default_read_only_subpaths_for_writable_root(
+pub(crate) fn default_read_only_subpaths_for_writable_root(
    writable_root: &AbsolutePathBuf,
    protect_missing_preserved_paths: bool,
 ) -> Vec<AbsolutePathBuf> {
    let mut subpaths: Vec<AbsolutePathBuf> = Vec::new();
-    let top_level_git = writable_root.join(".git");
+    let top_level_git = writable_root.join(PRESERVED_GIT_PATH_NAME);
    // This applies to typical repos (directory .git), worktrees/submodules
    // (file .git with gitdir pointer), and bare repos when the gitdir is the
    // writable root itself.
    let top_level_git_is_file = top_level_git.as_path().is_file();
    let top_level_git_is_dir = top_level_git.as_path().is_dir();
-    if top_level_git_is_dir || top_level_git_is_file || protect_missing_preserved_paths {
+    let should_protect_top_level_git = top_level_git_is_dir
+        || top_level_git_is_file
+        || (protect_missing_preserved_paths && !has_ancestor_git_metadata(writable_root.as_path()));
+    if should_protect_top_level_git {
        if top_level_git_is_file
            && is_git_pointer_file(&top_level_git)
            && let Some(gitdir) = resolve_gitdir_from_file(&top_level_git)
@@ -1322,7 +1366,7 @@ fn default_read_only_subpaths_for_writable_root(
        subpaths.push(top_level_git);
    }

-    let top_level_agents = writable_root.join(".agents");
+    let top_level_agents = writable_root.join(PRESERVED_AGENTS_PATH_NAME);
    if protect_missing_preserved_paths || top_level_agents.as_path().is_dir() {
        subpaths.push(top_level_agents);
    }
@@ -1331,7 +1375,7 @@ fn default_read_only_subpaths_for_writable_root(
    // default. For the workspace root itself, protect it even before the
    // directory exists so first-time creation still goes through the
    // preserved path approval flow.
-    let top_level_codex = writable_root.join(".codex");
+    let top_level_codex = writable_root.join(PRESERVED_CODEX_PATH_NAME);
    if protect_missing_preserved_paths || top_level_codex.as_path().is_dir() {
        subpaths.push(top_level_codex);
    }
@@ -1434,8 +1478,67 @@ fn has_explicit_resolved_path_entry(
    entries.iter().any(|entry| &entry.path == path)
 }

+fn first_preserved_component(path: &Path) -> Option<(AbsolutePathBuf, &'static str)> {
+    let mut candidate = PathBuf::new();
+    for component in path.components() {
+        candidate.push(component.as_os_str());
+        if let Some(preserved_name) = preserved_path_name(component.as_os_str()) {
+            let absolute = AbsolutePathBuf::from_absolute_path(candidate).ok()?;
+            return Some((absolute, preserved_name));
+        }
+    }
+    None
+}
+
+fn preserved_path_name(name: &OsStr) -> Option<&'static str> {
+    PRESERVED_PATH_NAMES
+        .iter()
+        .copied()
+        .find(|preserved| name == OsStr::new(preserved))
+}
+
+fn has_explicit_write_entry_for_path(
+    policy: &FileSystemSandboxPolicy,
+    path: &AbsolutePathBuf,
+    cwd: &Path,
+) -> bool {
+    policy
+        .resolved_entries_with_cwd(cwd)
+        .iter()
+        .any(|entry| entry.access.can_write() && &entry.path == path)
+}
+
+fn has_ancestor_git_metadata(path: &Path) -> bool {
+    let Some(parent) = path.parent() else {
+        return false;
+    };
+    parent
+        .ancestors()
+        .any(|ancestor| git_metadata_dir(&ancestor.join(PRESERVED_GIT_PATH_NAME)).is_some())
+}
+
+fn git_metadata_dir(path: &Path) -> Option<AbsolutePathBuf> {
+    let Ok(metadata) = std::fs::symlink_metadata(path) else {
+        return None;
+    };
+    if metadata.is_dir() {
+        return AbsolutePathBuf::from_absolute_path(path).ok();
+    }
+    if !metadata.is_file() {
+        return None;
+    }
+    let Ok(dot_git) = AbsolutePathBuf::from_absolute_path(path) else {
+        return None;
+    };
+    if !is_git_pointer_file(&dot_git) {
+        return None;
+    }
+    resolve_gitdir_from_file(&dot_git)
+}
+
 fn is_git_pointer_file(path: &AbsolutePathBuf) -> bool {
-    path.as_path().is_file() && path.as_path().file_name() == Some(OsStr::new(".git"))
+    path.as_path().is_file()
+        && path.as_path().file_name() == Some(OsStr::new(PRESERVED_GIT_PATH_NAME))
 }

 fn resolve_gitdir_from_file(dot_git: &AbsolutePathBuf) -> Option<AbsolutePathBuf> {
@@ -1452,7 +1555,14 @@ fn resolve_gitdir_from_file(dot_git: &AbsolutePathBuf) -> Option<AbsolutePathBuf

    let trimmed = contents.trim();
    let (_, gitdir_raw) = match trimmed.split_once(':') {
-        Some(parts) => parts,
+        Some((prefix, gitdir_raw)) if prefix.trim() == "gitdir" => (prefix, gitdir_raw),
+        Some(_) => {
+            error!(
+                "Expected {path} to contain a gitdir pointer, but it did not match `gitdir: <path>`.",
+                path = dot_git.as_path().display()
+            );
+            return None;
+        }
        None => {
            error!(
                "Expected {path} to contain a gitdir pointer, but it did not match `gitdir: <path>`.",
@@ -1626,6 +1736,71 @@ mod tests {
        );
    }

+    #[cfg(unix)]
+    #[test]
+    fn writable_roots_keep_missing_git_absent_under_parent_git_repo() {
+        let repo = TempDir::new().expect("tempdir");
+        fs::create_dir(repo.path().join(".git")).expect("create parent .git");
+        let cwd = repo.path().join("sub");
+        fs::create_dir(&cwd).expect("create subdir");
+        let expected_root =
+            AbsolutePathBuf::from_absolute_path(cwd.canonicalize().expect("canonicalize cwd"))
+                .expect("absolute canonical root");
+        let expected_dot_git = expected_root.join(".git");
+        let expected_dot_agents = expected_root.join(".agents");
+        let expected_dot_codex = expected_root.join(".codex");
+
+        let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry {
+            path: FileSystemPath::Special {
+                value: FileSystemSpecialPath::CurrentWorkingDirectory,
+            },
+            access: FileSystemAccessMode::Write,
+        }]);
+
+        let writable_roots = policy.get_writable_roots_with_cwd(&cwd);
+        assert_eq!(writable_roots.len(), 1);
+        assert_eq!(writable_roots[0].root, expected_root);
+        assert!(
+            !writable_roots[0]
+                .read_only_subpaths
+                .contains(&expected_dot_git),
+            "missing child .git under an existing parent repo is enforced by command policy so Git discovery still works"
+        );
+        assert!(
+            writable_roots[0]
+                .read_only_subpaths
+                .contains(&expected_dot_agents)
+        );
+        assert!(
+            writable_roots[0]
+                .read_only_subpaths
+                .contains(&expected_dot_codex)
+        );
+    }
+
+    #[cfg(unix)]
+    #[test]
+    fn writable_roots_protect_missing_git_when_parent_git_metadata_is_invalid() {
+        let repo = TempDir::new().expect("tempdir");
+        fs::create_dir(repo.path().join("real_git_dir")).expect("create real git dir");
+        fs::write(repo.path().join(".git"), "notgitdir: real_git_dir").expect("write parent .git");
+        let cwd = repo.path().join("sub");
+        fs::create_dir(&cwd).expect("create subdir");
+        let expected_root =
+            AbsolutePathBuf::from_absolute_path(cwd.canonicalize().expect("canonicalize cwd"))
+                .expect("absolute canonical root");
+        let expected_dot_git = expected_root.join(".git");
+
+        assert!(
+            default_read_only_subpaths_for_writable_root(
+                &expected_root,
+                /*protect_missing_preserved_paths*/ true,
+            )
+            .contains(&expected_dot_git),
+            "invalid parent .git metadata should not suppress child .git protection"
+        );
+    }
+
    #[cfg(unix)]
    #[test]
    fn writable_roots_skip_default_preserved_paths_when_explicit_user_rule_exists() {
@@ -1700,6 +1875,7 @@ mod tests {
    fn legacy_workspace_write_projection_blocks_missing_preserved_path_writes() {
        let cwd = TempDir::new().expect("tempdir");
        let dot_git_config = cwd.path().join(".git").join("config");
+        let dot_agents_config = cwd.path().join(".agents").join("config");
        let dot_codex_config = cwd.path().join(".codex").join("config.toml");
        let policy = SandboxPolicy::WorkspaceWrite {
            writable_roots: vec![],
@@ -1712,33 +1888,53 @@ mod tests {
            FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, cwd.path());

        assert!(!file_system_policy.can_write_path_with_cwd(&dot_git_config, cwd.path()));
+        assert!(!file_system_policy.can_write_path_with_cwd(&dot_agents_config, cwd.path()));
        assert!(!file_system_policy.can_write_path_with_cwd(&dot_codex_config, cwd.path()));
    }

+    #[test]
+    fn legacy_workspace_write_projection_blocks_missing_preserved_paths_under_extra_writable_root()
+    {
+        let cwd = TempDir::new().expect("tempdir");
+        let extra = TempDir::new().expect("extra writable root");
+        let extra_root = AbsolutePathBuf::from_absolute_path(extra.path()).expect("absolute extra");
+        let policy = SandboxPolicy::WorkspaceWrite {
+            writable_roots: vec![extra_root],
+            read_only_access: ReadOnlyAccess::Restricted {
+                include_platform_defaults: false,
+                readable_roots: vec![],
+            },
+            network_access: false,
+            exclude_tmpdir_env_var: true,
+            exclude_slash_tmp: true,
+        };
+
+        let file_system_policy =
+            FileSystemSandboxPolicy::from_legacy_sandbox_policy(&policy, cwd.path());
+
+        assert!(
+            !file_system_policy
+                .can_write_path_with_cwd(extra.path().join(".git/config").as_path(), cwd.path())
+        );
+        assert!(
+            !file_system_policy
+                .can_write_path_with_cwd(extra.path().join(".agents/config").as_path(), cwd.path())
+        );
+        assert!(!file_system_policy.can_write_path_with_cwd(
+            extra.path().join(".codex/config.toml").as_path(),
+            cwd.path()
+        ));
+    }
+
    #[test]
    fn legacy_workspace_write_projection_accepts_relative_cwd() {
        let relative_cwd = Path::new("workspace");
-        let expected_dot_git = AbsolutePathBuf::from_absolute_path(
+        let expected_root = AbsolutePathBuf::from_absolute_path(
            std::env::current_dir()
                .expect("current dir")
-                .join(relative_cwd)
-                .join(".git"),
+                .join(relative_cwd),
        )
-        .expect("absolute dot git");
-        let expected_dot_agents = AbsolutePathBuf::from_absolute_path(
-            std::env::current_dir()
-                .expect("current dir")
-                .join(relative_cwd)
-                .join(".agents"),
-        )
-        .expect("absolute dot agents");
-        let expected_dot_codex = AbsolutePathBuf::from_absolute_path(
-            std::env::current_dir()
-                .expect("current dir")
-                .join(relative_cwd)
-                .join(".codex"),
-        )
-        .expect("absolute dot codex");
+        .expect("absolute root");
        let policy = SandboxPolicy::WorkspaceWrite {
            writable_roots: vec![],
            network_access: false,
@@ -1749,43 +1945,35 @@ mod tests {
        let file_system_policy =
            FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(&policy, relative_cwd);

+        let mut expected_entries = vec![FileSystemSandboxEntry {
+            path: FileSystemPath::Special {
+                value: FileSystemSpecialPath::CurrentWorkingDirectory,
+            },
+            access: FileSystemAccessMode::Write,
+        }];
+        expected_entries.extend(
+            default_read_only_subpaths_for_writable_root(
+                &expected_root,
+                /*protect_missing_preserved_paths*/ true,
+            )
+            .into_iter()
+            .map(|path| FileSystemSandboxEntry {
+                path: FileSystemPath::Path { path },
+                access: FileSystemAccessMode::Read,
+            }),
+        );
+
        assert_eq!(
            file_system_policy,
-            FileSystemSandboxPolicy::restricted(vec![
-                FileSystemSandboxEntry {
-                    path: FileSystemPath::Special {
-                        value: FileSystemSpecialPath::Root,
-                    },
-                    access: FileSystemAccessMode::Read,
-                },
-                FileSystemSandboxEntry {
-                    path: FileSystemPath::Special {
-                        value: FileSystemSpecialPath::CurrentWorkingDirectory,
-                    },
-                    access: FileSystemAccessMode::Write,
-                },
-                FileSystemSandboxEntry {
-                    path: FileSystemPath::Path {
-                        path: expected_dot_git,
-                    },
-                    access: FileSystemAccessMode::Read,
-                },
-                FileSystemSandboxEntry {
-                    path: FileSystemPath::Path {
-                        path: expected_dot_agents,
-                    },
-                    access: FileSystemAccessMode::Read,
-                },
-                FileSystemSandboxEntry {
-                    path: FileSystemPath::Path {
-                        path: expected_dot_codex,
-                    },
-                    access: FileSystemAccessMode::Read,
-                },
-            ])
+            FileSystemSandboxPolicy::restricted(expected_entries)
        );
-        assert!(
-            !file_system_policy.can_write_path_with_cwd(Path::new(".git/config"), relative_cwd,)
+        assert_eq!(
+            forbidden_agent_preserved_path_write(
+                Path::new(".git/config"),
+                relative_cwd,
+                &file_system_policy,
+            ),
+            Some(".git")
        );
        assert!(
            !file_system_policy
@@ -1939,6 +2127,20 @@ mod tests {
                .join(".codex"),
        )
        .expect("absolute .codex symlink");
+        let expected_dot_git = AbsolutePathBuf::from_absolute_path(
+            root.as_path()
+                .canonicalize()
+                .expect("canonicalize root")
+                .join(".git"),
+        )
+        .expect("absolute .git");
+        let expected_dot_agents = AbsolutePathBuf::from_absolute_path(
+            root.as_path()
+                .canonicalize()
+                .expect("canonicalize root")
+                .join(".agents"),
+        )
+        .expect("absolute .agents");
        let unexpected_decoy =
            AbsolutePathBuf::from_absolute_path(decoy.canonicalize().expect("canonicalize decoy"))
                .expect("absolute canonical decoy");
@@ -1952,7 +2154,7 @@ mod tests {
        assert_eq!(writable_roots.len(), 1);
        assert_eq!(
            writable_roots[0].read_only_subpaths,
-            vec![expected_dot_codex]
+            vec![expected_dot_git, expected_dot_agents, expected_dot_codex]
        );
        assert!(
            !writable_roots[0]
@@ -1977,6 +2179,9 @@ mod tests {
            AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root");
        let link_private = link_root.join("linked-private");
        let expected_root = link_root.clone();
+        let expected_dot_git = expected_root.join(".git");
+        let expected_dot_agents = expected_root.join(".agents");
+        let expected_dot_codex = expected_root.join(".codex");
        let expected_linked_private = link_private.clone();
        let unexpected_decoy =
            AbsolutePathBuf::from_absolute_path(decoy.canonicalize().expect("canonicalize decoy"))
@@ -1998,7 +2203,12 @@ mod tests {
        assert_eq!(writable_roots[0].root, expected_root);
        assert_eq!(
            writable_roots[0].read_only_subpaths,
-            vec![expected_linked_private]
+            vec![
+                expected_dot_git,
+                expected_dot_agents,
+                expected_dot_codex,
+                expected_linked_private
+            ]
        );
        assert!(
            !writable_roots[0]
@@ -2024,6 +2234,9 @@ mod tests {
            AbsolutePathBuf::from_absolute_path(&link_root).expect("absolute symlinked root");
        let link_private = link_root.join("linked-private");
        let expected_root = link_root.clone();
+        let expected_dot_git = expected_root.join(".git");
+        let expected_dot_agents = expected_root.join(".agents");
+        let expected_dot_codex = expected_root.join(".codex");
        let expected_linked_private = link_private.clone();
        let unexpected_decoy =
            AbsolutePathBuf::from_absolute_path(decoy.canonicalize().expect("canonicalize decoy"))
@@ -2045,7 +2258,12 @@ mod tests {
        assert_eq!(writable_roots[0].root, expected_root);
        assert_eq!(
            writable_roots[0].read_only_subpaths,
-            vec![expected_linked_private]
+            vec![
+                expected_dot_git,
+                expected_dot_agents,
+                expected_dot_codex,
+                expected_linked_private
+            ]
        );
        assert!(
            !writable_roots[0]
@@ -2069,6 +2287,9 @@ mod tests {
            root.as_path().canonicalize().expect("canonicalize root"),
        )
        .expect("absolute canonical root");
+        let expected_dot_git = expected_root.join(".git");
+        let expected_dot_agents = expected_root.join(".agents");
+        let expected_dot_codex = expected_root.join(".codex");
        let expected_alias = expected_root.join("alias-root");

        let policy = FileSystemSandboxPolicy::restricted(vec![
@@ -2085,7 +2306,15 @@ mod tests {
        let writable_roots = policy.get_writable_roots_with_cwd(cwd.path());
        assert_eq!(writable_roots.len(), 1);
        assert_eq!(writable_roots[0].root, expected_root);
-        assert_eq!(writable_roots[0].read_only_subpaths, vec![expected_alias]);
+        assert_eq!(
+            writable_roots[0].read_only_subpaths,
+            vec![
+                expected_dot_git,
+                expected_dot_agents,
+                expected_dot_codex,
+                expected_alias
+            ]
+        );
    }

    #[cfg(unix)]
@@ -5,7 +5,6 @@

 use std::collections::HashMap;
 use std::collections::HashSet;
-use std::ffi::OsStr;
 use std::fmt;
 use std::ops::Mul;
 use std::path::Path;
@@ -84,6 +83,7 @@ pub use crate::permissions::FileSystemSandboxKind;
 pub use crate::permissions::FileSystemSandboxPolicy;
 pub use crate::permissions::FileSystemSpecialPath;
 pub use crate::permissions::NetworkSandboxPolicy;
+use crate::permissions::default_read_only_subpaths_for_writable_root;
 pub use crate::request_permissions::RequestPermissionsArgs;
 pub use crate::request_user_input::RequestUserInputEvent;

@@ -1249,17 +1249,13 @@ impl SandboxPolicy {
                }

                // For each root, compute subpaths that should remain read-only.
-                let cwd_root = AbsolutePathBuf::from_absolute_path(cwd).ok();
                roots
                    .into_iter()
                    .map(|writable_root| {
-                        let protect_missing_preserved_paths = cwd_root
-                            .as_ref()
-                            .is_some_and(|cwd_root| cwd_root == &writable_root);
                        WritableRoot {
                            read_only_subpaths: default_read_only_subpaths_for_writable_root(
                                &writable_root,
-                                protect_missing_preserved_paths,
+                                /*protect_missing_preserved_paths*/ true,
                            ),
                            root: writable_root,
                        }
@@ -1270,107 +1266,6 @@ impl SandboxPolicy {
    }
 }

-fn default_read_only_subpaths_for_writable_root(
-    writable_root: &AbsolutePathBuf,
-    protect_missing_preserved_paths: bool,
-) -> Vec<AbsolutePathBuf> {
-    let mut subpaths: Vec<AbsolutePathBuf> = Vec::new();
-    let top_level_git = writable_root.join(".git");
-    // This applies to typical repos (directory .git), worktrees/submodules
-    // (file .git with gitdir pointer), and bare repos when the gitdir is the
-    // writable root itself.
-    let top_level_git_is_file = top_level_git.as_path().is_file();
-    let top_level_git_is_dir = top_level_git.as_path().is_dir();
-    if top_level_git_is_dir || top_level_git_is_file || protect_missing_preserved_paths {
-        if top_level_git_is_file
-            && is_git_pointer_file(&top_level_git)
-            && let Some(gitdir) = resolve_gitdir_from_file(&top_level_git)
-        {
-            subpaths.push(gitdir);
-        }
-        subpaths.push(top_level_git);
-    }
-
-    let top_level_agents = writable_root.join(".agents");
-    if protect_missing_preserved_paths || top_level_agents.as_path().is_dir() {
-        subpaths.push(top_level_agents);
-    }
-
-    // Keep top-level preserved paths under .codex read-only to the agent by
-    // default. For the workspace root itself, protect it even before the
-    // directory exists so first-time creation still goes through the
-    // preserved path approval flow.
-    let top_level_codex = writable_root.join(".codex");
-    if protect_missing_preserved_paths || top_level_codex.as_path().is_dir() {
-        subpaths.push(top_level_codex);
-    }
-
-    let mut deduped = Vec::with_capacity(subpaths.len());
-    let mut seen = HashSet::new();
-    for path in subpaths {
-        if seen.insert(path.to_path_buf()) {
-            deduped.push(path);
-        }
-    }
-    deduped
-}
-
-fn is_git_pointer_file(path: &AbsolutePathBuf) -> bool {
-    path.as_path().is_file() && path.as_path().file_name() == Some(OsStr::new(".git"))
-}
-
-fn resolve_gitdir_from_file(dot_git: &AbsolutePathBuf) -> Option<AbsolutePathBuf> {
-    let contents = match std::fs::read_to_string(dot_git.as_path()) {
-        Ok(contents) => contents,
-        Err(err) => {
-            error!(
-                "Failed to read {path} for gitdir pointer: {err}",
-                path = dot_git.as_path().display()
-            );
-            return None;
-        }
-    };
-
-    let trimmed = contents.trim();
-    let (_, gitdir_raw) = match trimmed.split_once(':') {
-        Some(parts) => parts,
-        None => {
-            error!(
-                "Expected {path} to contain a gitdir pointer, but it did not match `gitdir: <path>`.",
-                path = dot_git.as_path().display()
-            );
-            return None;
-        }
-    };
-    let gitdir_raw = gitdir_raw.trim();
-    if gitdir_raw.is_empty() {
-        error!(
-            "Expected {path} to contain a gitdir pointer, but it was empty.",
-            path = dot_git.as_path().display()
-        );
-        return None;
-    }
-    let base = match dot_git.as_path().parent() {
-        Some(base) => base,
-        None => {
-            error!(
-                "Unable to resolve parent directory for {path}.",
-                path = dot_git.as_path().display()
-            );
-            return None;
-        }
-    };
-    let gitdir_path = AbsolutePathBuf::resolve_path_against_base(gitdir_raw, base);
-    if !gitdir_path.as_path().exists() {
-        error!(
-            "Resolved gitdir path {path} does not exist.",
-            path = gitdir_path.as_path().display()
-        );
-        return None;
-    }
-    Some(gitdir_path)
-}
-
 /// Event Queue Entry - events from agent
 #[derive(Debug, Clone, Deserialize, Serialize)]
 pub struct Event {
@@ -4453,6 +4348,15 @@ mod tests {
        let expected_docs_public =
            AbsolutePathBuf::from_absolute_path(canonical_cwd.join("docs/public"))
                .expect("canonical docs/public");
+        let expected_docs_public_dot_agents =
+            AbsolutePathBuf::from_absolute_path(canonical_cwd.join("docs/public/.agents"))
+                .expect("canonical docs/public/.agents");
+        let expected_docs_public_dot_codex =
+            AbsolutePathBuf::from_absolute_path(canonical_cwd.join("docs/public/.codex"))
+                .expect("canonical docs/public/.codex");
+        let expected_docs_public_dot_git =
+            AbsolutePathBuf::from_absolute_path(canonical_cwd.join("docs/public/.git"))
+                .expect("canonical docs/public/.git");
        let expected_dot_codex = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".codex"))
            .expect("canonical .codex");
        let expected_dot_git = AbsolutePathBuf::from_absolute_path(canonical_cwd.join(".git"))
@@ -4490,7 +4394,92 @@ mod tests {
                        expected_docs.to_path_buf()
                    ],
                ),
-                (expected_docs_public.to_path_buf(), Vec::new()),
+                (
+                    expected_docs_public.to_path_buf(),
+                    vec![
+                        expected_docs_public_dot_agents.to_path_buf(),
+                        expected_docs_public_dot_codex.to_path_buf(),
+                        expected_docs_public_dot_git.to_path_buf(),
+                    ],
+                ),
+            ]
+        );
+    }
+
+    #[test]
+    fn workspace_write_keeps_missing_git_absent_under_parent_repo() {
+        let repo = TempDir::new().expect("tempdir");
+        std::fs::create_dir(repo.path().join(".git")).expect("create parent .git");
+        let cwd = repo.path().join("sub");
+        std::fs::create_dir(&cwd).expect("create subdir");
+        let expected_root = AbsolutePathBuf::from_absolute_path(&cwd).expect("absolute cwd");
+        let expected_dot_codex =
+            AbsolutePathBuf::from_absolute_path(cwd.join(".codex")).expect("canonical .codex");
+        let expected_dot_agents =
+            AbsolutePathBuf::from_absolute_path(cwd.join(".agents")).expect("canonical .agents");
+        let policy = SandboxPolicy::WorkspaceWrite {
+            writable_roots: vec![],
+            read_only_access: ReadOnlyAccess::Restricted {
+                include_platform_defaults: false,
+                readable_roots: vec![],
+            },
+            network_access: false,
+            exclude_tmpdir_env_var: true,
+            exclude_slash_tmp: true,
+        };
+
+        assert_eq!(
+            sorted_writable_roots(policy.get_writable_roots_with_cwd(&cwd)),
+            vec![(
+                expected_root.to_path_buf(),
+                vec![
+                    expected_dot_agents.to_path_buf(),
+                    expected_dot_codex.to_path_buf()
+                ],
+            )]
+        );
+    }
+
+    #[test]
+    fn workspace_write_reserves_missing_preserved_paths_under_configured_writable_roots() {
+        let root = TempDir::new().expect("tempdir");
+        let cwd = root.path().join("cwd");
+        let extra = root.path().join("extra");
+        std::fs::create_dir_all(&cwd).expect("create cwd");
+        std::fs::create_dir_all(&extra).expect("create extra writable root");
+        let expected_cwd = AbsolutePathBuf::from_absolute_path(&cwd).expect("absolute cwd");
+        let expected_extra =
+            AbsolutePathBuf::from_absolute_path(&extra).expect("absolute extra root");
+        let policy = SandboxPolicy::WorkspaceWrite {
+            writable_roots: vec![expected_extra.clone()],
+            read_only_access: ReadOnlyAccess::Restricted {
+                include_platform_defaults: false,
+                readable_roots: vec![],
+            },
+            network_access: false,
+            exclude_tmpdir_env_var: true,
+            exclude_slash_tmp: true,
+        };
+
+        assert_eq!(
+            sorted_writable_roots(policy.get_writable_roots_with_cwd(&cwd)),
+            vec![
+                (
+                    expected_cwd.to_path_buf(),
+                    vec![
+                        expected_cwd.join(".agents").to_path_buf(),
+                        expected_cwd.join(".codex").to_path_buf(),
+                        expected_cwd.join(".git").to_path_buf()
+                    ],
+                ),
+                (
+                    expected_extra.to_path_buf(),
+                    vec![
+                        expected_extra.join(".agents").to_path_buf(),
+                        expected_extra.join(".codex").to_path_buf(),
+                        expected_extra.join(".git").to_path_buf()
+                    ],
+                ),
            ]
        );
    }