Skip to content

[codex] preserve fsmonitor for worktree Git reads#26880

Merged
tamird merged 8 commits into
mainfrom
fix-fsmonitor-usage
Jun 9, 2026
Merged

[codex] preserve fsmonitor for worktree Git reads#26880
tamird merged 8 commits into
mainfrom
fix-fsmonitor-usage

Conversation

@tamird

@tamird tamird commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Codex forces core.fsmonitor=false on internal Git commands so a
repository cannot select an executable fsmonitor helper. This also
disables Git's built-in daemon for status, diff, and ls-files,
turning those worktree reads into full scans in large repositories.

Read the raw effective core.fsmonitor value and preserve it only when
Git interprets it as true and advertises built-in daemon support through
git version --build-options. Query uncommon boolean spellings back
through 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-utils while keeping process execution in
the existing local and workspace-command adapters. Probe once per
worktree workflow and reuse the result for its Git commands, including
the TUI /diff path. Metadata-only commands and repository discovery
remain 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.

Comment thread codex-rs/core/src/git_info_tests.rs Outdated
Comment thread codex-rs/git-utils/src/info.rs Outdated
tamird added 3 commits June 8, 2026 05:35
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.
@tamird tamird force-pushed the fix-fsmonitor-usage branch from eac630a to 0294cf2 Compare June 8, 2026 12:35
@tamird

tamird commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

CI failure looks unrelated?

failures:

---- program_resolver::tests::test_resolved_program_executes_successfully stdout ----

thread 'program_resolver::tests::test_resolved_program_executes_successfully' (69) panicked at rmcp-client/src/program_resolver.rs:152:9:
Resolved program should execute successfully
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    program_resolver::tests::test_resolved_program_executes_successfully

test result: FAILED. 51 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.14s

@jif-oai jif-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok after my comments AND extensive testing

Comment thread codex-rs/git-utils/src/info.rs Outdated
Comment thread codex-rs/git-utils/src/info.rs Outdated
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.
@evawong-oai

Copy link
Copy Markdown
Contributor

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.

@tamird tamird force-pushed the fix-fsmonitor-usage branch 3 times, most recently from b8047d1 to ae24603 Compare June 8, 2026 23:58
@tamird tamird disabled auto-merge June 9, 2026 00:15
@tamird tamird force-pushed the fix-fsmonitor-usage branch 2 times, most recently from 02762ff to 0cb701e Compare June 9, 2026 02:23

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread codex-rs/git-utils/src/info.rs Outdated
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.
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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread codex-rs/git-utils/src/fsmonitor.rs
Comment thread codex-rs/git-utils/src/info.rs Outdated
tamird and others added 2 commits June 8, 2026 20:11
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 evawong-oai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated against BUGB 16290. This PR is now enforcing that CLI only uses fsmonitor when it is built in, and disables it everywhere else.

@tamird tamird merged commit dffc4bf into main Jun 9, 2026
31 checks passed
@tamird tamird deleted the fix-fsmonitor-usage branch June 9, 2026 04:32
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 9, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants