fix(session): resolve workspaces without git - #716
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 090f28bf28
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Fixed in e965ca9 by following the |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Regular .git files are still too broad on this head. statSync fixes the symlink case, but now any ordinary file named .git marks the directory as a repo root, even if it isn't a valid Git gitfile. The old git rev-parse path rejected that, so a stray/junk .git file can now change workspace/state identity. Could the file case validate gitdir: (and ideally the target) before accepting it?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2667b207af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| const markerPath = path.join(current, ".git"); | ||
| const markerStats = fs.statSync(markerPath); | ||
| const validGitFile = markerStats.isFile() && /^gitdir:\s*\S/m.test(fs.readFileSync(markerPath, "utf8")); |
There was a problem hiding this comment.
When a non-repository child directory within an outer checkout contains an ordinary .git file such as gitdir:/tmp/metadata (or one with a later gitdir: line), this regex accepts it because \s* permits no separator and /m searches every line. Git rejects those as invalid gitfiles—the valid header must begin the file as gitdir: <path>—so resolveWorkspaceRoot incorrectly selects the child instead of the real outer workspace, splitting job state and narrowing the app-server workspace for commands run below it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0106a64f04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39c6c2a85c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const markerPath = path.join(current, ".git"); | ||
| const markerStats = fs.statSync(markerPath); |
There was a problem hiding this comment.
When a checkout is configured with GIT_DIR and GIT_WORK_TREE but has no .git marker in the work tree, Git still resolves rev-parse --show-toplevel to the configured work-tree root, whereas this walk returns each caller's own directory. A task launched from work/sub is therefore stored and sandboxed under sub, while /codex:status or task resume run from work uses a separate state directory; preserve an environment-aware workspace-root path before falling back to marker discovery.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Fixed in fe4f2a4. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe4f2a4d6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function resolveWorkspaceRoot(cwd, env = process.env) { | ||
| try { | ||
| return ensureGitRepository(cwd); | ||
| if (env?.GIT_DIR && env?.GIT_WORK_TREE) { |
There was a problem hiding this comment.
When invoked inside a real repository with only GIT_WORK_TREE set (for example, from repo/packages/app with GIT_WORK_TREE=/tmp/wt), Git treats that variable as the working-tree root—its docs say it sets “the path to the root of the working tree” (Git environment docs)—but this pair-only guard skips it and the marker walk returns repo. Before this change, ensureGitRepository delegated to git rev-parse --show-toplevel, so task/status state and the app-server sandbox used the configured worktree; now they are incorrectly scoped to the metadata checkout. Handle the valid work-tree-only case after confirming the repository marker.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe4f2a4d6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| const configuredGitDirectory = path.resolve(cwd, env.GIT_DIR); | ||
| const configuredWorkTree = path.resolve(cwd, env.GIT_WORK_TREE); | ||
| if (fs.statSync(configuredGitDirectory).isDirectory() && fs.statSync(configuredWorkTree).isDirectory()) { |
There was a problem hiding this comment.
When GIT_DIR names a gitfile (for example, a gitdir: ... pointer copied from a linked worktree) and GIT_WORK_TREE names a work tree without its own .git marker, Git resolves that pair successfully, but this directory-only check rejects the valid GIT_DIR. The ancestor walk then returns the caller's subdirectory instead of the configured work-tree root, splitting task/state storage and narrowing the app-server workspace for commands launched below that root. Resolve gitfile-form GIT_DIR values before falling back to marker discovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4718eda3a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return false; | ||
| } | ||
|
|
||
| const match = /^gitdir: (.+?)(?:\r?\n|$)/.exec(fs.readFileSync(candidatePath, "utf8")); |
There was a problem hiding this comment.
When a nested .git file contains an otherwise valid header followed by extra content (for example, gitdir: /valid/.git\ntrailing-data), Git rejects the gitfile, but this expression accepts only its first line and findMarkerWorkspace selects the nested directory as a workspace. That creates a bogus state/job namespace and scopes task sandboxing to the child instead of continuing to the enclosing valid workspace; require the match to consume the complete gitfile.
Useful? React with 👍 / 👎.
| const canonicalCwd = fs.realpathSync.native(cwd); | ||
| const markerWorkspace = findMarkerWorkspace(canonicalCwd); | ||
|
|
||
| if (env?.GIT_WORK_TREE) { |
There was a problem hiding this comment.
When a process sets a valid GIT_DIR but leaves GIT_WORK_TREE unset, Git treats the invocation directory as the working-tree root (git rev-parse --show-toplevel returns that directory). This guard ignores that valid configuration, so a task started from a subdirectory that also has an enclosing .git marker is instead rooted at the enclosing checkout, changing the prior state identity and giving Codex a broader, incorrect workspace sandbox.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| return markerWorkspace ?? cwd; |
There was a problem hiding this comment.
When a checkout's .git/config sets core.worktree to an external working tree, the previous ensureGitRepository call returned that configured directory via git rev-parse --show-toplevel. The marker-only fallback returns the directory containing .git instead, so executeTaskRun stores jobs and launches the app server under the metadata checkout rather than the actual working tree; workspace-write tasks may consequently be unable to reach the configured worktree files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80236169be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| const valueMatch = /^worktree\s*=\s*(.+)$/i.exec(line); | ||
| if (valueMatch) { | ||
| const value = valueMatch[1].trim().replace(/^(?:"(.*)"|'(.*)')$/, "$1$2"); |
There was a problem hiding this comment.
When a valid repository config uses an inline comment after a core.worktree value (for example, worktree = "../work tree" # external tree), Git parses the value as ../work tree, but this code keeps the comment and therefore cannot unquote or stat the intended directory. configuredWorkTree then returns null and resolution falls back to the metadata checkout rather than the configured work tree, splitting task state and giving workspace-write app-server turns the wrong sandbox root.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e6a9988cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const valueMatch = /^worktree\s*=\s*(.+)$/i.exec(line); | ||
| if (valueMatch) { | ||
| const value = parseConfigValue(valueMatch[1]); | ||
| return value ? path.resolve(gitDirectory, value) : null; |
There was a problem hiding this comment.
When a valid config contains more than one core.worktree entry, Git's effective value is the last entry, but this returns as soon as it sees the first. For such a checkout, resolveWorkspaceRoot selects a stale worktree, placing task state and the workspace-write app-server sandbox in the wrong root even though git rev-parse --show-toplevel resolves the final entry; continue scanning and retain the final value.
Useful? React with 👍 / 👎.
| if (!line || line.startsWith("#") || line.startsWith(";")) { | ||
| continue; | ||
| } | ||
| const sectionMatch = /^\[([^\]]+)\]$/.exec(line); |
There was a problem hiding this comment.
When a valid config uses [core] # ... or [core] ; ... after another section, this anchored header regexp does not recognize the header and leaves section set to the preceding section. Git treats those as core headers, so the following worktree setting is ignored here and task state plus the workspace-write sandbox fall back to the metadata checkout instead of the configured worktree.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f45241476
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (candidateStats.isDirectory()) { | ||
| return fs.realpathSync.native(candidatePath); |
There was a problem hiding this comment.
.git directories before accepting them as repositories
When a non-repository child inside a real checkout contains an empty (or otherwise invalid) .git directory, this branch immediately accepts it. Git ignores such a directory and continues discovery to the enclosing repository, so a task invoked below the child now receives a separate job/state namespace and runAppServerTurn is confined to that child; validate directory markers as actual Git directories before returning them.
Useful? React with 👍 / 👎.
| const configPath = path.join(gitDirectory, "config"); | ||
| let section = ""; | ||
| let workTree = null; | ||
| for (const rawLine of fs.readFileSync(configPath, "utf8").split(/\r?\n/)) { |
There was a problem hiding this comment.
core.worktree values
Git config permits a value to continue across a backslash-newline. For example, a valid core.worktree = ../work\ followed by tree resolves in Git to ../work tree, but splitting the config before parsing reads the incomplete first line, cannot stat that path, and falls back to the metadata checkout. This gives tasks and their app-server sandbox the wrong workspace whenever the configured external worktree uses a continued value.
Useful? React with 👍 / 👎.
2 participants
Summary
git rev-parse --show-toplevelworkspace discovery with a filesystem-only ancestor walk.gitdirectories and linked-worktree gitfilesVerification
git diff --checkpassFixes #403