Skip to content

Batch git hash-object when fingerprinting a working-tree review target - #110

Merged
patriyang merged 4 commits into
mainfrom
fix/102-batch-working-tree-hash-object
Aug 11, 2026
Merged

Batch git hash-object when fingerprinting a working-tree review target#110
patriyang merged 4 commits into
mainfrom
fix/102-batch-working-tree-hash-object

Conversation

@patriyang

@patriyang patriyang commented Aug 11, 2026

Copy link
Copy Markdown
Owner

What

captureRepoStateIdentity fingerprints a working-tree review target so a queued background review can refuse to run once the repository has moved under it. Building that fingerprint hashed content one path at a time: every dirty tracked path and every untracked regular file went through its own git hash-object --no-filters -- <path> spawnSync.

That cost is O(dirty paths) process startups, paid inline on the --background enqueue path — the interactive call the user waits on before a job ID comes back.

Measured before the change on a synthetic repo (N dirty tracked + N untracked files), with git instrumented through a PATH shim:

N (each kind) git hash-object processes wall clock
50 100 —
400 800 6639 ms
800 1600 10227 ms

Exactly 2N process startups, ~6.4 ms per path, linear.

How

captureWorkingTreeDigest is now classify → batch → assemble:

  • classify — one lstat per tracked and untracked path decides whether it is a regular file (needs an OID) or a non-regular path (symlink, directory/gitlink, missing, other), which keeps its existing token logic untouched.
  • batch — hashRegularFilesBatched hashes the regular files from both sets in chunks bounded by path count (256) and cumulative UTF-8 path bytes (8 KiB, conservative enough to stay well under Windows' ~32767-character command line including the fixed arguments and quoting). git hash-object accepts multiple paths and emits one OID per line in argument order; a chunk's output is accepted only when the process succeeded and the non-empty line count equals the chunk length.
  • assemble — the digest is built in the original order with byte-identical token strings.

A chunk that fails for any reason falls back to hashing that chunk's paths individually, so one bad path (deleted mid-run, unreadable) cannot poison its neighbours. The per-path failure result is carried through, so a tracked file that fails to hash still throws with the real spawn error or the actual status/signal/stderr — the same fidelity as the gitChecked call it replaced — while untracked files keep their tolerant skipped:(...) behavior and never throw.

Same tree, after:

N (each kind) git hash-object processes wall clock
400 4 130 ms

51× faster at N=400, and the digest is byte-identical to what the old code produced for the same tree (e4c0b753…c299a1 before and after).

Semantics preserved

Every per-path behavior is unchanged: missing, symlink:<sha256 of readlink target>, submodule:<head>:<status>:<staged>:<unstaged>, other:<mode>:<size>, and the untracked inspectUntrackedFile branch — including that the MAX_UNTRACKED_BYTES / binary / directory display limits still apply only to non-regular untracked paths and never affect regular-file identity. The digest composition (status output, then \0tracked\0…, then \0untracked\0…) is untouched.

Tests

Four new tests in tests/git.test.mjs:

  1. Process count — a 300 + 300 tree, with a git shim on PATH logging each subcommand, asserts hash-object invocations stay a small constant. Verified red against the pre-change implementation: expected at most 8 hash-object calls, got 600.
  2. Chunk-to-OID alignment — 300 files span more than one 256-path chunk; editing a file in the second chunk is still detected.
  3. Awkward paths — spaces, non-ASCII, and a leading - in both the tracked and untracked sets, guarding the -- separator and the argv batching.
  4. Mixed tree — dirty tracked file, tracked symlink, untracked symlink pointing outside the repo, untracked binary, and an untracked file over MAX_UNTRACKED_BYTES: no drift right after capture, and each mutation still detected.

Full suite: 352 tests, 0 failures (npm test -- --test-concurrency=1; the parallel default spawns enough app-servers to produce spurious broker/runtime failures in this repo).

Notes

Closes #102

@patriyang
patriyang merged commit fc2d32c into main Aug 11, 2026
3 checks passed
@patriyang
patriyang deleted the fix/102-batch-working-tree-hash-object branch August 11, 2026 22:43
@patriyang patriyang mentioned this pull request Aug 11, 2026
Merged
patriyang added a commit that referenced this pull request Aug 11, 2026
@patriyang
Let plugin threads actually use MCP servers, and let an operator drop…
f58c8ad 
… the ones they should not be offered (#111)

Closes #109
Closes #105

Two changes on one surface: whether a Codex thread this plugin starts
can *use* the MCP servers it
is offered, and whether the operator can decide which ones it is offered
at all.

#105 was filed as an open policy question with three options and a
prerequisite nobody had tested.
Probing a live app-server to settle that prerequisite turned up #109,
which changes the premise —
so the two land together.

## #109 — every MCP tool call was failing as a phantom user rejection

Reproduced through the shipped entry point, before the fix:

```
node plugins/codex/scripts/codex-companion.mjs task --json \
  "Call the codegraph MCP status tool once and report the indexed file count..."
→ "failureMessage": "user rejected MCP tool call"
```

No user was asked. Tracing the JSONL against a raw `codex app-server`,
the app-server sends a
server→client **request** `mcpServer/elicitation/request` carrying
`_meta.codex_approval_kind: "mcp_tool_call"`, and
`AppServerClientBase.handleServerRequest` answered
*every* server request with `-32601`. The app-server reads that refusal
as a denial rather than as
"this client does not implement that method", and reports it to the
model as a user rejection.

`handleServerRequest` now answers elicitations:

- **approval-kind** (`_meta.codex_approval_kind` present) → `{ action:
"accept", content: {}, _meta: null }`.
These threads run `approvalPolicy: "never"` with nobody at the keyboard;
an approval gate has no
  one to answer it.
- **anything else** — a real data-collection form, or `mode: "url"` → `{
action: "decline", content: null, _meta: null }`.
Fabricating form values would be worse than declining, and a
protocol-valid decline still avoids
  the phantom rejection.
- **every other server method** keeps `-32601`, which is the right
answer for a method the client
  genuinely does not implement.

Both transports inherit this from the base class, so the broker path is
covered too — and in shared
mode the broker is the process that answers, which is worth knowing when
testing: a broker started
before this change keeps rejecting until it restarts.

This means MCP tool calls from plugin threads are now approved
unattended. That is deliberate rather
than incidental, it is the direction the operator picked, and it is
stated plainly in the README —
an MCP tool is not confined by the thread's sandbox the way a shell
command is. The knob below is
the control.

## #105 — `CODEX_DISABLED_MCP_SERVERS`

Comma-separated server names to disable on threads this plugin starts.
Unset by default, so nothing
changes for anyone who does not opt in — issue option 1 stays the
default, option 2 becomes
available per-server.

Two things worth recording, both established against a live app-server
(codex-cli 0.147.0) rather
than inferred from the generated types:

- **The mechanism the issue guessed does not work.** `config: {
mcp_servers: {} }` on `thread/start`
leaves every server attached — same at the CLI (`codex exec -c
'mcp_servers={}'`).
- **`mcp_servers.<name>.enabled = false` does work**, as a bare dotted
key, in both the `thread/start`
  `config` map and `-c`. That is the lever used here.

Names must match `[A-Za-z0-9_-]+`; anything else is skipped with a
warning. That is not fussiness:
`-c 'mcp_servers."hermes-vault".enabled=false'` fails config loading
outright
(`invalid transport in mcp_servers."hermes-vault"`) because the quoted
segment is read as a *new*
server table, so emitting a quoted key would break `thread/start` for
the whole run instead of just
leaving one server enabled. Hyphens are fine unquoted.

Also fixed while in those two functions: `buildThreadParams` /
`buildResumeParams` only built
`params.config` when `writableRoots` was non-empty, silently dropping
`options.config` otherwise.
The map is now built unconditionally and attached only when non-empty —
with nothing to say, the
params are byte-identical to before.

## Verification

`node --test --test-concurrency=1 tests/*.test.mjs` — **354 pass, 0
fail** (346 on `main`; +8 here).

Tests were written first and confirmed red: reverting only
`app-server.mjs` reds the three
elicitation tests, and reverting only `codex.mjs` reds the disable
tests. New coverage asserts the
recorded client reply on the wire (accept / decline / `-32601`) and the
recorded `thread/start` and
`thread/resume` config maps, via the fake app-server fixture.

End-to-end against a live Codex and the shipped entry point, same
workspace, only the env var
differing:

| Run | Result |
| --- | --- |
| before the fix | `user rejected MCP tool call` |
| after, env unset | `CALLED 42` — the tool actually ran |
| after, `CODEX_DISABLED_MCP_SERVERS=codegraph` | `NO_CODEGRAPH` — not
offered |

## Left alone

- **Probing servers for workspace fitness** (issue #105 option 3). It
depends on servers answering a
health question promptly, which is the exact thing the server behind
#100 failed to do.
- **`mcp_servers.<name>.tool_timeout_sec` / `startup_timeout_sec`**,
which `codex mcp list --json`
shows exist per server. A more targeted lever than the global budgets
from #104 for the original
  stall, but a separate change — worth its own issue.
- **A wildcard "disable everything" value.** Named servers only until
something asks for more.

## Collision note

`#102` was in flight in a parallel worktree while this was being built
and landed first as #110.
This branch is rebased on top of it (version bumped to 1.0.46 rather
than 1.0.45), and the full
suite was re-run after the rebase: **361 pass, 0 fail**. No overlap
between the two changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Background review enqueue spawns one git hash-object process per dirty path, so a large dirty tree makes enqueue slow

1 participant

@patriyang