[codex] preserve fsmonitor for worktree Git reads#26880
Conversation
cfac85b to
43b580f
Compare
Codex forced core.fsmonitor=false on every internal Git command to prevent repository-selected fsmonitor helpers from running. That also disabled Git's built-in fsmonitor for status, diff, and ls-files, turning those worktree reads into full scans in large repositories. Probe effective core.fsmonitor only for commands that can benefit. Preserve canonical boolean true while mapping false, unset, helper paths, malformed values, and probe failures to false. Current Git boolean semantics start the built-in daemon on demand and fall back to a full scan when it cannot be queried. Share the original five-second deadline between the probe and requested command so the extra process cannot extend the operation's timeout. Do not cache the decision because layered and conditional configuration can change while Codex is running.
Git 2.35.1 and older interpret the boolean-looking value "true" as a filesystem monitor hook path. Before Git 2.26, a successful empty hook response can incorrectly hide tracked changes. The first commit could therefore trade correctness for acceleration on older Git installations. Before preserving true, query `git version --build-options` for the `feature: fsmonitor--daemon` line, which Git exposes specifically for capability checks. Fall back to false when the feature or probe is unavailable, avoiding version parsing and preserving the previous behavior on unsupported builds. Keep both probes within the existing five-second deadline. Cover the unsupported and supported command sequences with a deterministic fake Git executable.
The helper-suppression coverage invoked the ambient Git binary from a core integration test, while the capability coverage exercised the command wrapper directly. This split made the related behavior harder to review and discarded useful subprocess diagnostics. Move the helper case alongside the capability test in git-utils. Drive both through a fake Git executable whose stdout, stderr, status, and arguments are asserted exactly. Record the commands and upstream Git sources used to regenerate the reduced response fixtures.
eac630a to
0294cf2
Compare
|
CI failure looks unrelated? |
jif-oai
left a comment
There was a problem hiding this comment.
Ok after my comments AND extensive testing
The git-utils wrapper now preserves built-in fsmonitor, but TUI /diff uses the workspace command executor and continued forcing it off for its tracked diff and untracked-file scan. Share the config and capability output checks while leaving process execution with each caller. Probe once per /diff and reuse the result for tracked diff and ls-files. Keep no-index diffs disabled because they do not inspect the index and cannot benefit from fsmonitor. Also query core.fsmonitor without typed conversion. Git converts every matching value before --get selects the last, so a shadowed global helper could mask a repository-local true. Validate only the raw effective value and cover that layered configuration.
|
In this case, once fsmonitor is turned on for these Git commands, it can be used to execute malicious artifacts even when the Git commands seem harmless. I think keeping fsmonitor on here is the wrong security thing to do. We can think about ways to improve usability without making this computationally expensive, similar to the Codex app change in https://github.com/openai/openai/pull/948875/changes. |
b8047d1 to
ae24603
Compare
02762ff to
0cb701e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cb701e423
ℹ️ 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".
The local Git wrapper and TUI /diff use different command runners, but both need to reject configured fsmonitor helpers without discarding the built-in daemon's worktree-scan acceleration. Move config and capability interpretation into git-utils. Read the raw effective value before normalizing it so an invalid value from a lower configuration scope cannot poison the selected value. Accept every boolean form Git accepts, delegating uncommon forms back to Git with an exact-value query, and require the daemon capability before preserving the setting. Probe once at each worktree workflow boundary and reuse the result for all of its commands. Keep metadata-only commands disabled without guessing from argv, and bound each local probe and requested command independently with the existing command timeout. Keep probe execution in the local and workspace adapters. This preserves their existing process and transport behavior without introducing a generic command abstraction.
0cb701e to
89096fe
Compare
Fsmonitor detection runs before status and diff, but giving the requested command a fresh timeout lets a stalled probe double the existing five-second command budget. Create one deadline at each local worktree workflow boundary and pass it through detection and command execution. This preserves the latency bound while still reusing one fsmonitor decision across the workflow. Inline the probe arguments at their only call sites.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d893fd1ae
ℹ️ 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".
Sharing one deadline between fsmonitor detection and subsequent Git commands changes the existing timeout behavior for diff collection. Later ls-files and no-index diffs inherit time spent by earlier commands. The fsmonitor probes only query configuration and build options and do not inspect the worktree or index. Keep them independently bounded and preserve the existing five-second timeout for each requested Git command.
evawong-oai
left a comment
There was a problem hiding this comment.
Validated against BUGB 16290. This PR is now enforcing that CLI only uses fsmonitor when it is built in, and disables it everywhere else.
Codex forces
core.fsmonitor=falseon internal Git commands so arepository cannot select an executable fsmonitor helper. This also
disables Git's built-in daemon for
status,diff, andls-files,turning those worktree reads into full scans in large repositories.
Read the raw effective
core.fsmonitorvalue and preserve it only whenGit interprets it as true and advertises built-in daemon support through
git version --build-options. Query uncommon boolean spellings backthrough Git using the exact effective value. Unset, false, helper paths,
malformed values, probe failures, and unsupported Git builds continue to
force
core.fsmonitor=false.Centralize this policy in
git-utilswhile keeping process execution inthe existing local and workspace-command adapters. Probe once per
worktree workflow and reuse the result for its Git commands, including
the TUI
/diffpath. Metadata-only commands and repository discoveryremain disabled without probing. Each probe and requested Git process
keeps its own existing timeout, and the decision is not cached because
layered and conditional Git configuration can change while Codex runs.