Skip to content

Add plugin cli-agent-bridge - #16

Closed
Hylouis233 wants to merge 40 commits into
hetaoBackend:mainfrom
Hylouis233:plugin/cli-agent-bridge
Closed

Add plugin cli-agent-bridge#16
Hylouis233 wants to merge 40 commits into
hetaoBackend:mainfrom
Hylouis233:plugin/cli-agent-bridge

Conversation

@Hylouis233

@Hylouis233 Hylouis233 commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

What changes

Adds the hosted Plugin plugins/Hylouis233/cli-agent-bridge: a dependency-free stdio MCP server
and same-name Skill that let MiniMax Code delegate bounded coding tasks to locally installed
Claude Code, Codex CLI, Kimi Code, ZCode, or DSH processes inside a Git repository, then review
the resulting workspace and commit changes.

The server exposes three tools:

  • list_backends reports configured CLIs, availability, versions, and experimental notes.
  • workspace_status returns a bounded Git snapshot: status, staged/unstaged diff stats, changed
    files (including untracked paths), HEAD and symbolic HEAD, and refs.
  • delegate_task runs one backend and returns the exit code, bounded stdout/stderr tails,
    before/after Git snapshots, moved refs, and an attributed commits block. Dirty worktrees are
    rejected unless allowDirty=true; unsupported resume requests fail instead of silently starting
    a fresh session.

Safety and concurrency behavior

  • Calls targeting the same Git common directory are serialized across subdirectory aliases,
    symlinks, linked worktrees, independent MCP clients, and bridge processes. Independent clones
    can still run in parallel.
  • Coordination state lives in a private bare lock store under the Git common directory; bridge
    refs never enter the target repository and therefore do not leak through git push --mirror.
    The lock store inherits core.sharedRepository and uses compare-and-swap owner records,
    cross-process failed-release recovery, read-only ownership probes, and safe automatic Git
    maintenance.
  • Cancellation and the overall deadline cover discovery, lock waits, snapshots, the backend, and
    post-run attribution. Backend process trees, plus descendants spawned by Git hooks/helpers, are
    contained and confirmed terminated before the repository lease is released. Uncertain worker
    termination fails closed behind a shared quarantine marker.
  • Git attribution handles unborn HEADs, same-commit symbolic-HEAD changes, force-moved and
    non-commit refs, fetched remote/tag/prefetch history, more than 256 baseline tips, linked
    worktrees, submodules, unusual path bytes, and staged/untracked changes without presenting a
    truncated snapshot as complete.
  • The server makes no network calls and stores no credentials or session logs. The selected backend
    CLI uses the user's own authentication and may send task/workspace data to its provider. Private
    coordination records contain process/lease metadata; a quarantine marker persists only when
    manual recovery is required.

User value

MiniMax Code remains the orchestrator while specialized coding CLIs perform self-contained work.
Every result includes reviewable Git evidence instead of relying on copied terminal output.

Example prompt:

Use the cli-agent-bridge skill, then delegate the login-form refactor in this repository to codex.

For independent comparison runs, create separate clean clones at the same starting commit; linked
worktrees intentionally share the repository lock and queue instead of running concurrently.

Requirements and disclosure

  • Node.js 20 or newer and Git on PATH; no npm runtime dependencies.
  • Windows, Linux, and macOS are supported. Windows shim commands use the bundled PowerShell 5.1
    runner. The implementation is exercised on Windows and Linux; the macOS POSIX path is supported
    but has not been machine-verified in this PR.
  • Backend CLIs must already be installed, authenticated, and configured by the user. Claude,
    Codex, and Kimi templates are non-experimental; ZCode and DSH remain explicitly experimental.
  • No credentials, installers, native binaries, private endpoints, or telemetry are bundled.

Verification

Verified on current head 6f158b2:

  • Linux container npm run check: 107 tests, 104 passed, 3 platform-specific skips, 0 failed.
  • Original plugin integration suite (test/server.test.mjs): 11/11 passed, including protocol
    negotiation, tool discovery, untracked files, dirty-tree refusal, unknown backends,
    cancellation, before/after snapshots, same-workspace serialization, queued cancellation,
    unborn HEAD, and Codex option delimiters.
  • Targeted Linux fixture proves an escaped fsmonitor-hook descendant is terminated before return.
  • Targeted recovery fixture proves a failed release is recoverable through a separately loaded
    module instance with no shared in-memory recovery state; ownership probes do not manufacture
    heartbeat blobs.
  • git diff --check is clean.
  • GitHub validate, analyze, and CodeQL checks all pass.

Submission checklist

  • Plugin path, manifest name, license, examples, requirements, platforms, accounts, network/data
    behavior, and experimental backends are disclosed.
  • No scaffold TODOs, credentials, symlinks, installers, or native binaries are included.
  • Repository validation and test suites pass on the current head.

Delegate coding tasks from MiniMax Code to locally installed coding CLIs (Claude Code, Codex, Kimi Code, ZCode, DSH) through a dependency-free stdio MCP server with git-diff review. See NOTICE for upstream credits.
Copilot AI lite review requested due to automatic review settings August 15, 2026 09:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Hylouis233

Copy link
Copy Markdown
Contributor Author

@codex review

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

Copy link
Copy Markdown

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: 02a992490a

ℹ️ 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 plugins/Hylouis233/cli-agent-bridge/server.mjs
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
- claude template gains --permission-mode acceptEdits so headless runs can
  actually edit files (verified end-to-end with Claude Code 2.1.226)
- kimi keeps plain -p prompt mode: --auto/-y are mutually exclusive with -p
  on current Kimi Code versions
- git snapshot now lists untracked files (git ls-files --others) so new files
  created by workers appear in changed files
- README/SKILL document permission defaults and honest backend verification
  status

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

Copy link
Copy Markdown

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: 9cd9011c67

ℹ️ 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 plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/README.md
npm-style .ps1/.cmd shims cannot be spawned directly on Windows, so runCommand retries them through the bundled ps1-runner.ps1 (Windows PowerShell 5.1) with verbatim argument forwarding. README documents the fallback and the custom-wrapper caveat.
- per-workspace serialization of delegate_task
- SIGTERM then SIGKILL force kill after timeout grace
- snapshots include staged, untracked files and committed deltas
- delegate_task marked destructiveHint, isError on failures
- notifications/cancelled kills the worker
- honest protocol version negotiation
- fail closed when git snapshot commands fail
- ring-buffer output capture without repeated large copies
- README discloses supported operating systems

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

Copy link
Copy Markdown

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: 77b9cb252d

ℹ️ 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 plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/README.md Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/skills/cli-agent-bridge/SKILL.md Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated

@hetaoBackend hetaoBackend left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@Hylouis233 感谢集中修复上一轮问题。最新 head 77b9cb2 的 CI/CodeQL 全绿,但仍有几个高风险生命周期问题:

  1. workspace mutex 以用户输入路径为 key。同一 Git worktree 通过仓库根目录、子目录或 symlink 会获得不同锁,写入型 worker 仍能并发污染 checkout;请按 canonical realpath 的 Git worktree root 加锁。
  2. 请求在排队等锁时取消,只设置标志;拿到锁后没有再次检查,仍会启动 CLI 并修改文件。
  3. timeout/cancel 只 kill 顶层 child 就 settle 并释放锁,后代进程可能继续写文件,同时下一个 worker 已启动。需要可靠的 process-tree 终止契约。
  4. same-workspace “两个 backend 独立比较”示例实际上第二次会遇到 dirty tree;应要求独立 worktree。
  5. unborn HEAD、resumeSessionId 不可获得、Codex prompt 缺 -- delimiter 等 thread 仍有效。
  6. 663 行高风险 MCP 没有把描述中的 fake-backend 测试提交进 PR,维护者无法复现并发/取消/git 状态行为。

请先修复前三个 P1,并提交可运行的自动化测试后再复审。

SKILL workflow and README tool description now cover before/after snapshots, committed deltas, same-workspace serialization, isError semantics, force-kill timeouts, and cancellation.
Seven self-contained tests cover protocol negotiation, tool list, snapshots, dirty-tree guard, unknown backends, cancellation, and before/after snapshots. SKILL wording now notes same-workspace serialization.
- workspace lock is now keyed by the canonical realpath of the git worktree
  root (git rev-parse --show-toplevel + fs.realpath), so the same checkout
  reached via a subdirectory, casing, or symlink shares one mutex (P1)
- a delegation cancelled while queued for the lock re-checks the cancel flag
  after acquiring it and returns before spawning the worker (P1)
- timeout and cancellation now terminate the whole process tree: taskkill
  /PID /T /F on Windows, signal to the detached process group on POSIX,
  instead of only killing the top-level child (P1)
- git snapshots tolerate an unborn HEAD (fresh git init) and committedDelta
  reports the first commits when the worker started from no commits (P2)
- codex templates delimit the prompt with -- so option-like tasks are not
  parsed as CLI flags (P2)
- README comparison example now requires two independent git worktrees; the
  queued same-checkout second run is documented as follow-up work (P2)
- SKILL resume guidance no longer implies results carry backend session ids

Tests extended to 11 (protocol, tools, snapshots, dirty guard, unknown
backend, in-flight cancel, before/after, worktree-root lock serialization,
cancel-while-queued, unborn HEAD, codex -- delimiter); 3 consecutive runs
green locally
@Hylouis233

Copy link
Copy Markdown
Contributor Author

Round-3 lifecycle feedback is addressed in a42a84d (plus the previously unpushed fd9c432 test suite and db2553e doc alignment):

  1. Lock keyed by worktree root (P1) — mutex key is now the canonical realpath of git rev-parse --show-toplevel output; subdir/casing/symlink paths to one checkout share a single lock.
  2. Cancel-while-queued (P1) — after acquiring the lock the delegation re-checks the cancel flag and returns before any spawn; the result reports cancelled=true and the worker never started.
  3. Process-tree termination (P1) — timeout and cancellation kill the tree: taskkill /PID <pid> /T /F on Windows, group signal to the detached process group on POSIX; descendants can no longer outlive the kill while the next worker starts.
  4. Unborn HEAD (P2) — snapshots tolerate a fresh git init (empty head, other failures still fail closed); committedDelta reports the worker's first commits.
  5. Codex -- delimiter (P2) — templates are exec -- <task> / exec resume <session> -- <task>.
  6. Comparison example (P2) — README now requires two independent worktrees; same-checkout queueing is documented as follow-up semantics.
  7. SKILL resume wording (P2) — no implication that results carry backend session ids.
  8. Runnable tests (your item 6)test/server.test.mjs now has 11 self-contained tests (protocol negotiation, tool list, snapshots incl. untracked, dirty guard + isError, unknown backend, in-flight cancellation ~1.2s settle, before/after + committed delta, worktree-root lock serialization via concurrent root+subdir delegations, cancel-while-queued, unborn HEAD end-to-end, codex template shape). node --test test/server.test.mjs passed 3 consecutive runs locally on Windows; no network needed (fake slow backends).

CI on the new head is running; validate was green pre-push.

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

Copy link
Copy Markdown

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: a42a84dba2

ℹ️ 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 plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/ps1-runner.ps1 Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
- gitSnapshot/committedDelta executed their git commands via Promise.all;
  git status and git diff both refresh the index, so concurrent processes
  raced for .git/index.lock and intermittently failed with exit code 128,
  surfacing as random 'git snapshot unreliable' errors (reproduced 1-in-15
  on Windows; the fail-closed guard turned it into a JSON-RPC error)
- commands now run serially per repository; 10/10 Windows runs and a full
  npm run check in a Linux node:22 container pass
- test harness: every server now stops in a finally block (a leaked server
  kept stdin/stdout pipes open and hung node --test when an assertion
  failed - the cause of the stalled CI validate job), in-flight cancel test
  bounds its worker with an explicit timeoutMs
@Hylouis233

Copy link
Copy Markdown
Contributor Author

Follow-up in d27f2e6 — root cause of the stalled validate job on a42a84d, found and fixed:

Why validate hung: the CI npm run check run of a42a84d hung because (a) gitSnapshot ran its seven git commands through Promise.allgit status and git diff both refresh the index, and concurrent git processes intermittently race for .git/index.lock, failing a command with exit code 128 (reproduced 1-in-15 locally; the fail-closed guard turned it into a JSON-RPC error), which made a test assertion fail; and (b) the test harness only stopped its server after the assertions, so the failed test leaked a server process holding stdin/stdout pipes and node --test never exited.

Fixes: git snapshot commands now run serially per repository (same for committedDelta); the test harness stops every server in a finally block regardless of assertion outcome; the in-flight-cancel test bounds its worker with an explicit timeoutMs. The stalled run for a42a84d can be cancelled — it is running the old head.

Verification: node --test test/server.test.mjs 10/10 consecutive Windows runs green; full npm ci && npm run check in a Linux node:22 container: 23/23 tests pass, exit 0, completes in well under 5 minutes.

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

Copy link
Copy Markdown

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: d27f2e6035

ℹ️ 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 plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated

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

Copy link
Copy Markdown

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: 6ee0a0b8c9

ℹ️ 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 plugins/Hylouis233/cli-agent-bridge/process-tree.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs

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

Copy link
Copy Markdown

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: 1612d21f54

ℹ️ 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 plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated

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

Copy link
Copy Markdown

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: f46431a5bb

ℹ️ 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 plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/process-tree.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs

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

Copy link
Copy Markdown

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: c7a4a15ba8

ℹ️ 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 plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated

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

Copy link
Copy Markdown

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: 8f599d2bb2

ℹ️ 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 plugins/Hylouis233/cli-agent-bridge/server.mjs
Comment thread plugins/Hylouis233/cli-agent-bridge/process-tree.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated
Comment thread plugins/Hylouis233/cli-agent-bridge/server.mjs Outdated

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

Copy link
Copy Markdown

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: dd7d4b4c14

ℹ️ 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 on lines +383 to +384
allowLocalRecovery() {
if (stopped && !retained && !released) locallyAbandonedRefs.set(localRefKey, currentOid);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Share failed-release recovery across bridge processes

When lease.release() exhausts its delete retries after a transient failure, this records the abandoned OID only in the current server's in-memory map and then releases its local FIFO gate. A request handled by another bridge process cannot see that recovery authorization; because the leftover owner PID is still alive, normal stale-idle reclamation also refuses the lease, so that process waits indefinitely even after the underlying Git failure clears. Persist or otherwise expose the abandoned-lease state to every server sharing the lock store, or continue cleanup until the ref is removed.

Useful? React with 👍 / 👎.

if (error.code !== "ENOENT") throw error;
}
if (!initialized) {
const result = await runGitCommand(["init", "--bare", "--quiet", storeRoot], {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve shared-repository permissions in the lock store

When the target repository is configured for group or multi-user sharing, the first caller initializes this nested bare repository with ordinary umask-derived permissions rather than the parent repository's sharing mode. Subsequent OS users who can write the target Git directory can therefore be unable to create lock objects or update refs in this store, making both workspace_status and delegation fail for them. git init -h documents --shared as the option that makes a repository shared among several users; initialize the store with permissions compatible with the enclosing repository.

Useful? React with 👍 / 👎.

Comment on lines +323 to +324
if (manageProcessTree) await signalProcessTree(child, "SIGTERM", treeState);
else try { child.kill("SIGTERM"); } catch { /* already gone */ }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Contain descendants spawned by Git commands

When a repository-configured Git helper such as core.fsmonitor forks a long-lived process, these non-worker commands run with manageProcessTree=false, so timeout or cancellation signals only the Git parent. A descendant that inherited the command pipes can remain alive after runCommand() settles and after the workspace lease is released, and may continue accessing or modifying the repository without serialization; a simple fsmonitor hook that backgrounds sleep reproduces the surviving process. Run snapshot/discovery Git commands in containment or explicitly terminate and drain their descendant processes before releasing the lease.

Useful? React with 👍 / 👎.

Comment on lines +352 to +356
const timer = setInterval(() => {
if (stopped || heartbeatPending || lostError || interruptedError) return;
heartbeatPending = true;
void queueUpdate({}).catch(() => {}).finally(() => { heartbeatPending = false; });
}, heartbeatMs);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Garbage-collect superseded lock-owner blobs

Every five-second heartbeat hashes a new owner blob because heartbeatAt changes and then moves the ref away from the previous blob. Those old blobs, along with superseded history records, become unreachable in the private bare store, but no code ever runs maintenance or garbage collection against that store; garbage collection of the enclosing target repository does not recurse into this nested repository. Long-running and repeated delegations therefore accumulate thousands of loose objects indefinitely, increasing disk usage and eventually degrading lock operations. Schedule safe maintenance for the lock store or avoid creating a new Git object for every heartbeat.

Useful? React with 👍 / 👎.

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

Copy link
Copy Markdown

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: a1ecd0a21b

ℹ️ 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 on lines +441 to +443
if (platform === "linux" && treeState.runMarker &&
!processes.some((item) => item.pid === child.pid) &&
treeState.markerObservationComplete !== true) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Recover escaped children on non-Linux POSIX

On macOS, if a backend forks a child that creates a new session and the parent exits before the first 250 ms ps snapshot, reparenting removes the only ancestry link and the old process group can already be gone, so this function reports the tree as terminated and releases the repository lock while the child keeps editing. Fresh evidence beyond the earlier Linux marker issue is that the new observation grace is explicitly gated to platform === "linux"; add equivalent escaped-child discovery or kernel-backed containment for the advertised macOS path.

Useful? React with 👍 / 👎.

Comment on lines +889 to +891
if (change.ref.startsWith("refs/remotes/") || change.ref.startsWith("refs/prefetch/")) {
externalRefChanges.push(change);
continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Exclude fetches directed into local refs

When a worker runs git fetch origin topic:refs/heads/imported, the downloaded tip appears as a changed refs/heads/ target rather than one of these recognized external namespaces, so its entire upstream history is reported as worker-created commits. Fresh evidence after the remote/tag/prefetch fixes is Git's arbitrary destination refspec: git fetch -h documents the [<refspec>...] form and --force as “force overwrite of local reference”; capture those fetch-updated destinations as external baselines rather than inferring provenance solely from the final namespace.

Useful? React with 👍 / 👎.

// realpath() has already canonicalized ordinary aliases and path casing.
// Preserve the result: NTFS directories can opt into case sensitivity and
// may legally contain distinct repositories whose names differ only by case.
return "git-common-dir:" + normalized;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep the repository lock stable across directory renames

On POSIX, if the repository or its Git common directory is renamed during an active delegation, the existing worker retains its current directory inode and can continue editing, but a request opened through the new pathname computes a different lock key and can acquire a second ref in the moved lock store. The two workers then run concurrently until the original holder's next heartbeat notices that its old absolute store path vanished, creating a several-second window for conflicting edits; derive the key from a rename-stable filesystem or repository identity rather than the canonical pathname alone.

Useful? React with 👍 / 👎.

Comment on lines +709 to +711
if (ref.startsWith(WORKSPACE_LOCK_REF_PREFIX)) {
if (ref !== ownLockRef) lockRefs.push(line.slice(separator + 1));
continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve similarly named refs from the target repository

When the target repository legitimately contains a ref below refs/cli-agent-bridge/workspace-locks/, this branch removes it from every snapshot even though coordination refs now live in the separate private lock store. A worker can therefore create, move, or delete such a ref without it appearing in refsChanged, and commits reachable only through that ref are omitted from worker attribution entirely; retain all target-repository refs and read concurrency metadata directly from the private store instead.

Useful? React with 👍 / 👎.

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

Copy link
Copy Markdown

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: 6f158b2d9d

ℹ️ 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 on lines +588 to +592
const result = await runGitCommand(initArgs, {
cwd: gitCommonDir, ...options,
});
const failure = snapshotFailure("git init --bare workspace lock store", result);
if (failure) throw new Error("cannot initialize workspace lock store: " + failure);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Initialize the shared lock store atomically

When two bridge processes first access a repository before the private lock store exists, both can pass the HEAD check and run git init --bare against the same directory concurrently. Git initialization is not reliably concurrency-safe—one initializer can fail on template copies or config locks—so an otherwise valid workspace_status or delegation returns an internal initialization error precisely in the cross-process scenario this lock is intended to support. Initialize into a uniquely named temporary directory and atomically install it, or tolerate a losing initializer after validating the completed store.

Useful? React with 👍 / 👎.

Comment on lines +294 to +297
if (operatorCleared) {
try {
if (await operatorCleared()) return true;
} catch { /* treat a failed check as not cleared */ }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Require explicit recovery instead of marker absence

When the quarantine file under the OS temporary directory disappears because of routine temporary-file cleanup rather than an operator inspection, operatorCleared() returns true and this branch immediately authorizes reclamation without checking the lease age or whether its owner is still alive. If the process tree that caused quarantine remains active, the next delegation can therefore acquire the repository lock and run concurrently with that escaped worker; record an explicit durable recovery action instead of treating ENOENT as proof of operator approval.

Useful? React with 👍 / 👎.

Comment on lines +819 to +828
for (const candidate of baselineCommits) {
const ancestor = await runGitCommand(["merge-base", "--is-ancestor", candidate, target], {
cwd: worktreeRoot, ...options,
});
if (ancestor.timedOut || ancestor.stdoutTruncated || ancestor.stderrTruncated ||
![0, 1].includes(ancestor.exitCode)) {
throw new Error("cannot select committed-delta baseline: git merge-base --is-ancestor failed");
}
if (ancestor.exitCode !== 0) continue;
const distance = await distanceFromTarget(candidate);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Batch committed-delta baseline selection

When a repository has thousands of branches or tags, each target runs at least one separate git merge-base process per pre-run tip and another git rev-list process for every ancestral tip; the fallback loop can add another process per tip. This makes a simple post-run attribution require tens of thousands of serial Git invocations while holding the repository lock, commonly exhausting the overall delegation deadline and blocking queued requests; compute the nearest baseline with batched revision input or a bounded number of graph walks.

Useful? React with 👍 / 👎.

continue;
}
handleMessage(message).then((response) => {
if (response) stdout.write(JSON.stringify(response) + "\n");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Handle broken stdout through the shutdown path

When the MCP host disconnects or crashes while a delegation is active, writing the eventual response to the closed stdout pipe emits an error event, but no stdout error handler is installed. Node treats that as an uncaught stream error and exits immediately; the process exit hook cannot await asynchronous process-tree termination, so a detached worker can survive while its running lease remains permanently retained. Route stdout errors through the same awaited terminateActiveRequests() shutdown path used for stdin closure.

Useful? React with 👍 / 👎.

Add cross-process repository locking, fail-closed process containment, safe Git attribution, explicit quarantine recovery, Windows Job Object execution, and regression coverage for reviewed lifecycle and documentable safety boundaries.

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

Copy link
Copy Markdown

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: bf51a027cb

ℹ️ 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 (process.platform !== "win32" || options.refreshProcessTree) {
const refreshIntervalMs = process.platform === "linux" ? 25 : 250;
treeRefreshTimer = setInterval(() => { void refreshTree(); }, refreshIntervalMs);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Start polling only after process-tree initialization

On Linux when the initial /proc inspection takes longer than 25 ms, this interval calls refreshTree() concurrently with treeState.initialRefresh; both mutate knownPids, knownStarts, and the sticky uncertainty flags, while the periodic refresh is not allowed to capture the root identity. In this environment that race consistently marks the tree uncertain, causes initializeProcessTree() to reject, kills the containment runner, and makes the first lock-store initialization return -32603, so both workspace_status and delegate_task are unusable. Start periodic polling only after initialRefresh has completed successfully, or otherwise serialize the two inspections.

Useful? React with 👍 / 👎.

// the periodic heartbeat only needs to prove this exact ref is still ours.
// A read-only probe avoids creating an unreachable content-addressed blob
// every few seconds during long delegations.
void queueOwnershipProbe().catch(() => {}).finally(() => { heartbeatPending = false; });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep active-lease timestamps fresh for attribution

When a worker runs for more than CONCURRENT_LEASE_STALE_MS (30 seconds), this read-only ownership probe leaves the owner blob's heartbeatAt at its last state transition. gitSnapshot() only counts starting/running leases whose timestamp is under that threshold, so a delegation in a linked worktree can stop detecting the still-active worker; if that worker moves shared refs during the second delegation's snapshot window, its commits can be reported as the second worker's changes without repositoryConcurrency being set. Refresh the timestamp or make concurrency detection verify the live lease independently of this stale field.

Useful? React with 👍 / 👎.

try {
await interruptibleFilesystemOperation(chmod(candidatePath, mode), options);
try {
await interruptibleFilesystemOperation(link(candidatePath, idPath), options);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid requiring hard-link support for repository IDs

When the Git common directory is on a filesystem that permits ordinary Git repositories but does not support hard links, such as some network shares or removable filesystems, this link() fails with EPERM, ENOTSUP, or EOPNOTSUPP; only EEXIST is tolerated, so repository identity initialization fails and every workspace_status or delegate_task call returns an internal error. Publish the identity with an atomic primitive available on those filesystems, or provide a safe fallback when hard links are unsupported.

Useful? React with 👍 / 👎.

Comment on lines +453 to +455
if (isLinuxProcessGone(error)) {
taskChangedWhileReading = true;
continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Handle Linux procfs without task children files

On Linux systems where /proc/<pid>/task/<tid>/children is unavailable—for example when the kernel lacks the procfs children feature—every read here returns ENOENT even though the runner and its threads remain alive. This branch treats that permanent capability absence as repeated task-list churn, so no stable sample is ever recorded, processIdentityUncertain becomes sticky, and process-tree initialization kills the runner before any contained Git command or backend can start; the current container reproduces this for every task directory. Fall back to a complete PID/PPID snapshot or reject the platform based on capability detection rather than treating persistent missing files as a transient exit race.

Useful? React with 👍 / 👎.

Comment on lines +76 to +79
if (!pathCommandPromises.has(command)) {
pathCommandPromises.set(command, resolvePathCommandUncached(command));
}
return pathCommandPromises.get(command);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Retry failed backend command resolution

When list_backends or delegate_task first probes a command before that CLI is installed, the resolved promise contains null and is retained in this process-wide map forever. Installing the executable later into an already-present PATH directory therefore leaves every subsequent probe and delegation reporting it unavailable until the MCP server is restarted; avoid caching negative resolutions indefinitely or invalidate them before a later availability check.

Useful? React with 👍 / 👎.

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

Copy link
Copy Markdown

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: 95af068ee8

ℹ️ 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 on lines +99 to +101
async function disabledHooksRoot() {
hooksRootPromise ??= mkdir(DISABLED_HOOKS_ROOT, { recursive: true, mode: 0o700 })
.then(() => DISABLED_HOOKS_ROOT)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Secure the disabled-hooks directory before trusting it

When another local user pre-creates this predictable temp path (the suffix is derived from the platform and UID), mkdir({ recursive: true }) accepts the attacker-controlled directory without verifying its owner, permissions, or whether a component is a symlink. The attacker can place an executable reference-transaction hook there, and every safeGitInvocation() then selects it through core.hooksPath; Git's githooks documentation says this hook runs during ref updates, so lock-store update-ref operations execute attacker-controlled code with the bridge user's privileges. Create the directory with exclusive, user-owned semantics and verify every reused path before treating it as an empty hooks directory.

Useful? React with 👍 / 👎.

Comment on lines +1412 to +1417
// FETCH_HEAD records the actual objects downloaded by fetch independently
// of its arbitrary destination refspec. A fetch can write directly into
// refs/heads or any custom namespace, so namespace alone cannot establish
// provenance. Exclude every recorded fetched tip while retaining the local
// destination movement as an attribution target for any later worker commit.
for (const oid of after.fetchHeads ?? []) await addBaseline(oid);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve fetched tips from every fetch in the run

Fresh evidence beyond the prior arbitrary-refspec case is that this baseline contains only the final contents of FETCH_HEAD. If a worker fetches divergent branch A into a local/custom ref and then performs another fetch for branch B, the second command overwrites A's entry while its destination ref remains; the changed A ref is consequently reported as worker-created history. git fetch -h documents --append as appending instead of overwriting and also exposes --no-write-fetch-head, so the final file cannot be a complete provenance log; retain fetched tips across the run or otherwise identify every fetch-updated destination.

Useful? React with 👍 / 👎.

Comment on lines +1682 to +1685
await writeFile(temporaryPath, JSON.stringify(record), { flag: "wx", mode: 0o600 });
try {
try {
await link(temporaryPath, quarantinePath);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Publish quarantine markers without hard-link support

When the per-user temporary directory is on a filesystem without hard links, link() fails with errors such as EPERM or ENOTSUP during the exact process-tree failure that requires quarantine. The caller has already invoked workspaceLease.retain() and added the key to quarantinedWorkspaces, so this exception leaves a non-reclaimable running lease and subsequent requests report quarantine without the marker path that operators are instructed to rename. Use an atomic publication mechanism supported on the advertised filesystems or preserve and return a usable recovery record when publication fails.

Useful? React with 👍 / 👎.

Comment on lines +124 to +127
for (const name of [
"GIT_ALTERNATE_OBJECT_DIRECTORIES", "GIT_CONFIG_COUNT", "GIT_DIR", "GIT_DIFF_OPTS",
"GIT_EXTERNAL_DIFF", "GIT_INDEX_FILE", "GIT_OBJECT_DIRECTORY", "GIT_WORK_TREE",
]) delete env[name];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Clear GIT_COMMON_DIR before repository discovery

When the MCP server inherits GIT_COMMON_DIR—for example because it was launched from Git-related tooling—this environment sanitization leaves it active while clearing the related GIT_DIR. Git documents GIT_COMMON_DIR as overriding the common administrative directory, and git rev-parse --git-common-dir consequently returns that inherited path rather than the requested workspace's common directory. The bridge then creates/acquires its lock in the wrong repository while snapshotting and launching the worker in the requested one, so a normally launched bridge can concurrently edit the same workspace under a different lock; remove GIT_COMMON_DIR along with the other repository-routing variables.

Useful? React with 👍 / 👎.

Comment on lines +1167 to +1169
const diffStat = [String(out.diffStat ?? "").trim(), String(out.cachedDiffStat ?? "").trim()]
.filter(Boolean)
.map((s, i) => (i === 0 ? s : s.split(/\r?\n/).map((l) => "staged: " + l).join("\n")))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep the staged label when no unstaged diff exists

When a workspace contains only staged changes, the empty unstaged entry is removed by filter(Boolean), so the cached diff becomes element zero and bypasses the staged: prefix. The returned git.diffStat therefore presents a staged-only change in the same form used for an unstaged diff, even though the snapshot contract distinguishes those states; associate each stat with its source before filtering rather than deriving the source from the filtered index.

Useful? React with 👍 / 👎.

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

Copy link
Copy Markdown

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: 68bbb1457c

ℹ️ 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 on lines +2240 to +2244
const result = await runCommand(backendCommand, args, {
cwd: workspacePath,
timeoutMs: remaining,
manageProcessTree: true,
shouldCancel: () => Boolean(cancel && cancel.cancelled),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Sanitize Git routing variables for delegated workers

When the MCP server inherits repository-routing variables such as GIT_DIR, GIT_WORK_TREE, or GIT_INDEX_FILE from a hook or other Git tooling, the bridge's own preflight succeeds because safeGitInvocation() removes them, but this backend launch supplies no sanitized env, so runCommand() passes the original process.env through the containment runner. Git commands executed by the delegated agent can consequently modify or commit in the inherited repository instead of workspacePath, while the returned snapshot examines the requested workspace and may show no changes; launch workers with those routing variables removed.

Useful? React with 👍 / 👎.

}

async function markWorkspaceQuarantined(key, details) {
await mkdir(WORKSPACE_LOCK_ROOT, { recursive: true, mode: 0o700 });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Verify ownership of the quarantine directory

When another local account pre-creates the predictable WORKSPACE_LOCK_ROOT path, this recursive mkdir accepts the attacker-owned directory without checking its owner, permissions, or path components. After a quarantine marker is published, the directory owner can rename that exact file to the .recovery-approved suffix; because its contents already contain the matching random quarantine ID, quarantineRecoveryApproved() authorizes reclamation while the escaped worker may still be editing the repository. Create this user-scoped directory exclusively or verify every reused component before trusting recovery files beneath it.

Useful? React with 👍 / 👎.

experimental: Boolean(spec.experimental),
};
}
const backendCommand = await resolvePathCommand(spec.command);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Bound backend command resolution by the request deadline

When the configured backend or an earlier absolute PATH entry is on a stalled network/FUSE filesystem, resolvePathCommand() can remain pending in access(), stat(), or realpath() indefinitely. This await happens before deadline is constructed and does not observe cancel, so neither timeoutMs nor client cancellation can finish delegate_task; clean MCP shutdown also waits forever for the tracked request to unwind. Make command resolution interruptible and include it in the advertised overall deadline.

Useful? React with 👍 / 👎.

Comment on lines +1405 to +1409
// A newly arriving tag without a moved local HEAD/branch at the same commit
// is conservatively treated as fetch-sourced. Existing tags may be moved by
// the worker (including from a non-commit object) and remain attribution
// labels because a before/after snapshot cannot prove such a move was fetch.
if (tagCommit && !movedLocalTargets.has(tagCommit)) externalRefChanges.push(change);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve commits reachable only through a new tag

When a worker creates a commit on a temporary branch, tags it, returns to the original branch, and deletes the temporary branch, the new tag is the only after-state ref that reaches the worker-created commit. Because no moved HEAD or local branch target matches it, this condition classifies the tag as fetch-sourced, excludes it from targets, and returns newCommitCount: 0 with an empty diff even though the commit persists in the repository. Do not infer fetched provenance solely from a tag being newly created without a matching moved branch.

Useful? React with 👍 / 👎.

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

Copy link
Copy Markdown

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: 7ac1c50f40

ℹ️ 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 on lines +205 to +207
} catch {
// fall through to the next candidate
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Fail closed when the backend override cannot load

When CLI_AGENT_BRIDGE_BACKENDS is explicitly set but the file is missing, unreadable, malformed, or has an empty/invalid backends object, this fallback silently loads the bundled configuration instead. A delegation can therefore execute a different command or omit wrapper/sandbox flags that the operator intended to require; treat an explicit override as authoritative and report its load or validation error rather than falling through to defaults.

Useful? React with 👍 / 👎.

Comment on lines +64 to +65
`.recovery-approved` suffix. Mere marker absence never authorizes recovery. The workspace may
still contain edits made before cancellation, so still review the returned snapshot.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Require a fresh status after cancellation

When an in-flight delegation is cancelled after making edits, delegateTask passes the already-cancelled token to the post-run snapshot, catches OperationCancelledError, and returns git: null; the retained gitBefore cannot show those edits. This instruction therefore tells the orchestrator to review a returned snapshot that does not exist, risking cancellation-time changes going unreviewed; direct it to call workspace_status after cleanup instead.

Useful? React with 👍 / 👎.

Comment on lines +1182 to +1186
const traceStat = await stat(provenance.tracePath);
if (traceStat.size > MAX_CAPTURE_CHARS) {
throw new Error("Git fetch provenance trace exceeded the capture limit");
}
const trace = await readFile(provenance.tracePath, "utf8");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Read the worker-controlled trace through a bounded regular file

A delegated backend receives GIT_TRACE2_EVENT and runs as the same user, so before exiting it can unlink this trace and replace it with a FIFO or a symlink to a non-terminating device such as /dev/zero. stat() follows that replacement and its initial size passes the limit, after which this unbounded readFile() can hang forever outside the request deadline and cancellation path while the workspace lease and shutdown remain blocked. Retain and read the originally created file descriptor, or verify a no-follow regular file and perform a bounded, interruptible read.

Useful? React with 👍 / 👎.

GIT_PAGER: "",
PAGER: "",
});
return { command: await trustedGitExecutable(), args: safeArgs, env };

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Bound Git executable resolution by request interruption

When an absolute PATH entry before the real Git executable is on a stalled network or FUSE filesystem, trustedGitExecutable() can remain pending in access(), stat(), or realpath() indefinitely. This await does not receive the delegation deadline or cancellation token, so timeoutMs, workspace_status cancellation, and awaited shutdown cannot unwind the request; resolve Git through the same detachable deadline-aware subscription used for backend commands.

Useful? React with 👍 / 👎.

Comment on lines +1781 to +1784
available: check.exitCode === 0,
experimental: Boolean(spec.experimental),
version: check.exitCode === 0 ? tail(check.stdout, 200).trim() : null,
error: check.exitCode === 0 ? "" : (check.errorMessage || "command not found or not executable"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject backend probes with unconfirmed process cleanup

When a backend's --version process exits with code 0 but leaves a descendant whose termination cannot be confirmed, runCommand() returns treeTerminated: false and a termination error, yet this entry still reports the backend as available with no error. The list_backends call may already have left that process running, and the orchestrator is encouraged to delegate to a backend whose containment probe failed; treat treeTerminated === false as an unavailable probe and surface terminationError.

Useful? React with 👍 / 👎.

Comment on lines +2204 to +2206
const timeoutMs = Number.isInteger(rawArgs?.timeoutMs)
? Math.min(MAX_TIMEOUT_MS, Math.max(MIN_TIMEOUT_MS, rawArgs.timeoutMs))
: DEFAULT_TIMEOUT_MS;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject invalid timeout values instead of extending them

When a client sends a non-integer timeoutMs, such as the common JSON mistake "5000", this silently substitutes the 20-minute default rather than rejecting the request. The advertised input schema is not enforcement for raw MCP clients, so a malformed five-second safety limit can unexpectedly allow a worker to run for 1,200,000 ms; validate the value and return an invalid-arguments error instead of defaulting or clamping explicit invalid input.

Useful? React with 👍 / 👎.

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

Copy link
Copy Markdown

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: 50664e4bd7

ℹ️ 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".

for (const line of String(walk.stdout ?? "").split(/\r?\n/u)) {
if (!line.startsWith("-")) continue;
const boundary = line.slice(1).trim();
if (/^[0-9a-f]{40,64}$/u.test(boundary)) return boundary;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Choose the moved ref's baseline for merge diffs

When a worker merges an existing side branch into the checked-out branch, this walk yields both pre-run parents as boundary commits, but returning the first one does not ensure it is the moved ref's prior tip. In a Git 2.43 reproduction with main at A, side at B, and merge M, rev-list --topo-order --boundary M emitted B before A, so the later stat used B..M: it reported main's pre-existing changes and omitted the side changes introduced into main. Prefer beforeOid when it is ancestral to the target, or otherwise select a boundary according to explicit per-ref semantics.

Useful? React with 👍 / 👎.

Comment on lines +796 to +798
if (platform === "linux" && treeState.runMarker &&
!processes.some((item) => item.pid === child.pid) &&
treeState.markerObservationComplete !== true) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Contain Linux children that clear the run marker

On supported Linux, a backend can spawn a detached child with an environment that omits CLI_AGENT_BRIDGE_RUN_ID and exit before the 25 ms ancestry poll observes that child; once it is reparented, this grace loop can see neither ancestry nor the marker and declares the tree terminated. I reproduced this with a Node backend that spawned such a detached child and exited immediately: runCommand() returned treeTerminated: true while the child wrote a file 1.5 seconds later. The workspace lease is therefore released while a daemonized child can still modify it; use kernel-backed containment or fail closed when this escape cannot be ruled out.

Useful? React with 👍 / 👎.

async function readWorkspaceQuarantine(quarantineRoot, key) {
const quarantinePath = workspaceQuarantinePath(quarantineRoot, key);
try {
const marker = await stat(quarantinePath);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Make quarantine reads interruptible

When the lock store is on a stalled network or FUSE filesystem, this stat()—and the following readFile()/realpath() calls—can remain pending indefinitely because readWorkspaceQuarantine() accepts neither the request cancellation token nor its deadline. The function is awaited both before acquisition and again while holding the workspace lease, so timeoutMs, client cancellation, and awaited shutdown can all fail to unwind the request. Pass the interruption options through and use the same bounded filesystem-operation path used during workspace discovery.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector

Copy link
Copy Markdown

💡 Codex Review

const recoveryAuthorization = await writeRecoveryAuthorization(
cwd, ref, currentOid, ownerToken,
);

P1 Badge Retry recovery authorization before abandoning release

When the recovery-ref update-ref fails transiently—for example because its .lock file briefly exists—this await throws before the deletion retry loop starts, leaving the workspace owner ref behind. Because that owner belongs to the still-running bridge process, canReclaim() continues to see it as alive, so subsequent status and delegation requests cannot acquire the workspace until the server exits; retry the authorization write or otherwise retain a cleanup path for this exact owner.


async function readBackendConfiguration(file) {
return validateBackendConfiguration(JSON.parse(await readFile(file, "utf8")), file);

P2 Badge Make backend configuration reads interruptible

When CLI_AGENT_BRIDGE_BACKENDS points to a stalled UNC or other network filesystem, this unbounded readFile() ignores both the delegation deadline and MCP cancellation. delegate_task starts its advertised overall deadline before calling loadBackends(), but neither that timeout nor shutdown can unwind this read, and shutdown waits indefinitely for the active request to finish; pass cancellation/deadline handling into configuration loading or use an abortable read.



P1 Badge Reconcile acquisition after an interrupted ref update

When cancellation or the deadline fires just after the acquisition update-ref commits but before its Git process closes, this unconditional interruption check discards the successful command result, so tryAcquireGitWorkspaceLock() never creates the lease that would compensate by releasing the new owner ref. The ref then names the still-running bridge process and blocks every later request until that process exits; after an interrupted CAS, re-read the ref and clean up when it contains this request's newOid.


} catch (error) {
throw new Error("cannot canonicalize Git worktree root: " + error.message);

P2 Badge Preserve interruptions while canonicalizing Git paths

When cancellation or the delegation deadline fires while realpath() is canonicalizing the discovered worktree, this catch wraps OperationCancelledError or DeadlineExceededError in a generic Error. The request therefore returns a -32603 internal error instead of the documented cancelled or timed-out tool result; rethrow interruption errors unchanged here and in the analogous Git-common-directory catch.

ℹ️ 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".

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

Copy link
Copy Markdown

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: 51437d9e32

ℹ️ 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".

uncertain = true;
continue;
}
if (session.worktree === targetWorktree && session.exitCode === 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Treat nonzero fetches as provenance-uncertain

When one refspec is updated but another is rejected, git fetch can exit nonzero after changing refs; git fetch -h exposes --atomic as “use atomic transaction to update references,” so partial updates are permitted by default. This condition ignores that traced fetch because its exit code is not zero, leaving uncertain false and allowing the successfully fetched ref's upstream commits to be reported as worker-created history. Mark every completed fetch targeting this worktree as uncertain regardless of its final exit code.

Useful? React with 👍 / 👎.

Comment on lines +1783 to +1786
if (Number.isFinite(record?.endedAt)) {
const acquiredAt = Number.isFinite(record.acquiredAt) ? record.acquiredAt : record.endedAt;
if (acquiredAt <= Date.now() && record.endedAt >= windowStart) {
concurrentDelegations += 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Avoid cross-host clocks when detecting overlapping runs

When linked worktrees sharing this repository are delegated from hosts whose clocks differ, acquiredAt and endedAt were generated using the other host's Date.now() and cannot be compared reliably with this host's windowStart or current time. A remote delegation that starts after the before-snapshot and finishes before this scan can therefore be skipped by both the active-lease signal and this history test; if it moved shared refs, its commits may be attributed to the current worker with repositoryConcurrency left false. Use lock-store ordering or another clock-independent overlap marker instead of comparing wall-clock timestamps across hosts.

Useful? React with 👍 / 👎.

let buffer = "";
stdin.on("data", (chunk) => {
if (shutdownRequested) return;
buffer += chunk;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Bound the pending stdio request buffer

When a buggy or malicious MCP host writes an unterminated request line continuously, every chunk is appended here with no size limit, so the server's memory grows until Node is terminated. An OOM during an active delegation can also leave a non-reclaimable starting or running workspace lease behind. Enforce a maximum JSON-RPC line size and reject or close the connection once the buffered input exceeds it.

Useful? React with 👍 / 👎.

for (const [name, spec] of Object.entries(backends)) {
// A hung `--version` probe must not pin the request: the client can cancel
// the discovery call, terminating the current probe and skipping the rest.
if (cancel?.cancelled) break;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Report list_backends cancellation instead of partial success

When cancellation arrives after one or more backend probes, this break returns the entries collected so far and the handler wraps them as a normal successful list_backends result; cancellation during configuration loading similarly returns an empty success. A caller that still receives the response cannot distinguish cancellation from an authoritative availability list and may incorrectly conclude that unprobed backends are absent. Propagate a cancelled result or JSON-RPC cancellation error rather than returning a partial list.

Useful? React with 👍 / 👎.

Comment on lines +2131 to +2133
const probeError = check.treeTerminated !== true
? (check.terminationError || "backend version probe process tree could not be confirmed terminated")
: (check.errorMessage || "command not found or not executable");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve the actual backend probe failure

When an installed backend's --version probe times out or exits nonzero because of configuration, authentication, or an unsupported flag, treeTerminated can still be true and errorMessage empty, so this fallback reports the command as “not found or not executable.” That sends users toward the wrong remediation and hides the timeout, exit code, and captured stderr already present in check; construct the error from those fields before using the missing-command fallback.

Useful? React with 👍 / 👎.

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

Copy link
Copy Markdown

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: b2f78116b0

ℹ️ 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".

worktreeRoot, String(out.fetchHeadPath ?? "").replace(/\r?\n$/u, ""),
);
try {
const rawFetchHead = await interruptibleFilesystemOperation(readFile(fetchHeadPath, "utf8"), options);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Bound FETCH_HEAD before reading it into memory

When the repository or delegated worker leaves a very large FETCH_HEAD (or replaces it with a link to a large file), this unbounded readFile() loads the entire contents before validating any records. A worker can therefore exhaust the MCP server's memory during either snapshot, and the surrounding deadline/cancellation race does not stop the underlying filesystem read or its allocation. Read through a size-limited regular-file handle, as is already done for the worker-controlled provenance trace.

Useful? React with 👍 / 👎.

Comment on lines +2755 to +2756
if (!rawArgs || typeof rawArgs.backend !== "string" || !rawArgs.backend.trim()) {
throw new Error("backend must be a non-empty string");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Classify malformed tool arguments as invalid requests

When a raw MCP client omits backend, supplies an empty task, or provides an invalid workspacePath, these validators throw a generic Error; the outer handler maps only InvalidArgumentsError to -32602, so these client mistakes are reported as -32603 internal server failures. Clients may consequently retry or diagnose a bridge outage instead of correcting their input; use the invalid-arguments error type consistently for request-field validation.

Useful? React with 👍 / 👎.

Comment on lines +964 to +967
if (process.platform === "linux" && repositoryId) {
return "git-common-dir-id:" + repositoryId;
}
const normalized = path.normalize(gitCommonDir);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Key Windows locks by repository identity

On the supported Windows path, renaming a repository changes this pathname-derived key even though the private lock store and its persistent repositoryId move with the same repository. A second bridge process opening the new path therefore acquires a different coordination ref and can start another worker before the original holder's next heartbeat notices that its old path disappeared, allowing both workers to edit the same repository concurrently and invalidating their snapshots. Use the persistent repository identity for Windows lock keys as well, or establish an equivalent rename-stable identity.

Useful? React with 👍 / 👎.

Comment on lines +1504 to +1506
const normalizeWorktree = (value) => {
const normalized = path.resolve(String(value ?? ""));
return process.platform === "win32" ? normalized.toLowerCase() : normalized;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve case-sensitive Windows repository identities

On Windows directories with per-directory case sensitivity enabled, sibling repositories whose paths differ only by case are distinct, but this normalization collapses their Trace2 worktree names. If a delegated worker fetches in such a sibling repository, the bridge treats that fetch as targeting the delegated workspace and returns attributionUnavailable, discarding otherwise valid commit attribution. Compare canonical path or filesystem identities without unconditional lowercasing, consistent with the case-sensitive handling already used for repository lock keys.

Useful? React with 👍 / 👎.

Comment on lines +141 to +142
worker = spawn(process.execPath, [entry, ...payload.args], {
cwd: process.cwd(), env: workerEnvironment(payload), windowsHide: true,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Honor npm shims' bundled Node executable

When a standard Windows npm shim sits beside its own node.exe, the shim deliberately selects that executable, but this direct-launch path always substitutes the bridge's process.execPath. This changes normal shim semantics and can run the CLI under an incompatible Node version or architecture even though invoking the configured .cmd manually succeeds; retain the adjacent node.exe choice detected by the validated shim or reject the shim rather than silently changing runtimes.

Useful? React with 👍 / 👎.

@Hylouis233

Copy link
Copy Markdown
Contributor Author

本 PR 已迁移至 MiniMax-AI/MiniMax-Code-Plugins#7(head 8f87ae4,内容一致)。上一轮反馈后的修复提交均已包含,后续 review 请在新 PR 进行。

@DeepPattern-AI

Copy link
Copy Markdown

Hi Hylouis233, sorry to bother you. This PR is very relevant to a workflow problem I’m trying to understand: delegating bounded coding tasks across local CLI agents, then bringing the resulting workspace back for review and commit instead of treating each tool as an isolated chat surface.

The list_backends, workspace_status, and review-before-commit flow are especially interesting because they make tool coordination and handoff visible in the engineering loop.

We’re inviting a small group of developers with hands-on AI dev tool / coding-agent workflow experience to an external testing program. For now I’m only checking interest, and details can be shared privately if relevant. If this sounds interesting, could you reply and let me know?

@Hylouis233

Copy link
Copy Markdown
Contributor Author

关闭说明:本 PR 已完成迁移,后续在 MiniMax-AI/MiniMax-Code-Plugins#7 继续 review。当前 head 8f87ae4 与新 PR 内容一致。

@Hylouis233 Hylouis233 closed this Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants