feat: add benchmark tool-isolation guard (task 0023) #24

alexion · 2026-07-15T22:27:28-04:00

alexion commented

2026-07-15 22:27:28 -04:00

Task: .claude/tasks/0023-bench-tool-isolation-guard.md

Summary

Adds the guard that confines each benchmark arm's agent to exactly one tool, so a result measures the tool rather than the agent's choice between tools (ADR 0016).

guardCommand(arm, command) — the authoritative guard. It inspects every binary a proposed shell command would reach (not just the leading token) via a hand-rolled shell parser handling pipelines, ;/&&/|| sequences, subshells, $(...) and backtick substitutions, process substitutions, redirections (incl. 2>&1, &>), and leading NAME=value assignments. It permits only the active arm's one allow-listed binary plus a curated set of harmless read-only utilities; foreign binaries, absolute-path evasions (even of the arm's own binary), and interpreter-based fetch tricks are denied. The gitea-mcp arm runs with the shell disabled entirely.
provisionArmBin(arm, binDir, locate?) — produces a curated per-arm bin directory exposing only that arm's binary (empty for gitea-mcp), as the convenience layer behind the authoritative guard. locate is injectable so tests stay host-independent.
ARM_BINARY / HARMLESS_BINARIES — the per-arm allow-listed binary and the curated harmless-utility set.

31 colocated tests in bench/guard.test.ts, run via npm run test:bench (kept out of the src/ coverage tier).

Deviations from the plan

Depth beyond the literal acceptance criteria, invited by ADR 0016 ("isolation strength rests on the completeness of the guard's deny rules"): the full command parser closes pipe-hiding and substitution-hiding evasions in addition to the named absolute-path and interpreter-fetch cases, and path-qualified invocations are refused even for the arm's own binary. No criteria were dropped.

Review

Risk

Overall: Medium

Blast radius: Low — two self-contained files in bench/ plus one README line; only in-directory imports, no external callers.
Reversibility: Low — new dev-only files, no persistence/schema/published API; deletion fully reverts.
Test coverage: Low — guard.test.ts thoroughly exercises both exports (own/foreign/abs-path/pipe/substitution/redirection/interpreter/mcp-off, provisioning success/empty/throw).
Sensitive domain: Medium — a security-flavored isolation guard (hand-rolled allow-list parser), but it gates only the benchmark harness, not production auth.
Size & complexity: Medium — ~540 lines; extractCommands is an intricate hand-rolled shell tokenizer.
Runtime criticality: Low — bench/ is dev-only, excluded from the published package.

Standards findings (unaddressed)

Primitive Obsession (judgement call) — binary/command names are bare string rather than a dedicated type. Left as-is: this matches the sibling files' de-facto convention (result.ts also uses bare string for taskId).
extractCommands size/complexity (judgement call) — ~185 lines of nested cursor-sharing closures. Left as-is: the complexity is deliberate and justified by the security intent, and extracting the closures wouldn't help since they share one cursor.

Spec findings (unaddressed)

None. The Spec axis reported the diff faithfully implements every acceptance criterion, with the extra hardening explicitly invited by ADR 0016 — not scope creep.

Task: `.claude/tasks/0023-bench-tool-isolation-guard.md` ## Summary Adds the guard that confines each benchmark arm's agent to exactly one tool, so a result measures the tool rather than the agent's choice between tools (ADR 0016). - `guardCommand(arm, command)` — the authoritative guard. It inspects **every** binary a proposed shell command would reach (not just the leading token) via a hand-rolled shell parser handling pipelines, `;`/`&&`/`||` sequences, subshells, `$(...)` and backtick substitutions, process substitutions, redirections (incl. `2>&1`, `&>`), and leading `NAME=value` assignments. It permits only the active arm's one allow-listed binary plus a curated set of harmless read-only utilities; foreign binaries, absolute-path evasions (even of the arm's own binary), and interpreter-based fetch tricks are denied. The `gitea-mcp` arm runs with the shell disabled entirely. - `provisionArmBin(arm, binDir, locate?)` — produces a curated per-arm bin directory exposing only that arm's binary (empty for `gitea-mcp`), as the convenience layer behind the authoritative guard. `locate` is injectable so tests stay host-independent. - `ARM_BINARY` / `HARMLESS_BINARIES` — the per-arm allow-listed binary and the curated harmless-utility set. 31 colocated tests in `bench/guard.test.ts`, run via `npm run test:bench` (kept out of the `src/` coverage tier). ### Deviations from the plan Depth beyond the literal acceptance criteria, invited by ADR 0016 ("isolation strength rests on the completeness of the guard's deny rules"): the full command parser closes pipe-hiding and substitution-hiding evasions in addition to the named absolute-path and interpreter-fetch cases, and path-qualified invocations are refused even for the arm's own binary. No criteria were dropped. ## Review ### Risk **Overall: Medium** - Blast radius: Low — two self-contained files in `bench/` plus one README line; only in-directory imports, no external callers. - Reversibility: Low — new dev-only files, no persistence/schema/published API; deletion fully reverts. - Test coverage: Low — `guard.test.ts` thoroughly exercises both exports (own/foreign/abs-path/pipe/substitution/redirection/interpreter/mcp-off, provisioning success/empty/throw). - Sensitive domain: Medium — a security-flavored isolation guard (hand-rolled allow-list parser), but it gates only the benchmark harness, not production auth. - Size & complexity: Medium — ~540 lines; `extractCommands` is an intricate hand-rolled shell tokenizer. - Runtime criticality: Low — `bench/` is dev-only, excluded from the published package. ### Standards findings (unaddressed) - **Primitive Obsession (judgement call)** — binary/command names are bare `string` rather than a dedicated type. Left as-is: this matches the sibling files' de-facto convention (`result.ts` also uses bare `string` for `taskId`). - **`extractCommands` size/complexity (judgement call)** — ~185 lines of nested cursor-sharing closures. Left as-is: the complexity is deliberate and justified by the security intent, and extracting the closures wouldn't help since they share one cursor. ### Spec findings (unaddressed) - None. The Spec axis reported the diff faithfully implements every acceptance criterion, with the extra hardening explicitly invited by ADR 0016 — not scope creep.

alexion added 1 commit 2026-07-15 22:27:28 -04:00

feat: add benchmark tool-isolation guard (task 0023)

CI / test (pull_request) Successful in 50s

Details

CI / test (push) Successful in 49s

Details

0436dc25fd

Add the guard that confines each benchmark arm's agent to exactly one
tool, so a result measures the tool rather than the agent's choice
between tools.

`guardCommand` inspects every binary a proposed shell command would
reach — across pipelines, sequences, subshells, command and process
substitutions, redirections, and leading environment assignments — and
permits only the active arm's one allow-listed binary plus a curated set
of harmless read-only utilities. Foreign binaries, absolute-path
evasions (even of the arm's own binary), and interpreter-based fetch
tricks are denied; the gitea-mcp arm runs with the shell disabled
entirely. `provisionArmBin` produces a curated per-arm bin directory
exposing only that arm's binary as the convenience layer behind the
authoritative guard.

Tests are colocated in bench/guard.test.ts and run via `npm run
test:bench`.

alexion merged commit 0436dc25fd into main

2026-07-16 06:58:47 -04:00

alexion deleted branch task-0023-bench-tool-isolation-guard

2026-07-16 06:58:47 -04:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: alexion/gitea-axi#24