diff --git a/CHANGELOG.md b/CHANGELOG.md index d3919ee5..6db19eaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,17 @@ ## [Unreleased] +## 0.24.4 — 2026-09-29 + ### Added +- **A review of content this deployment already reviewed is replayed from a durable store instead of paying for the reviewer again.** `src/automation/reviewVerdictStore.ts` keys a verdict on `(kind, base, sha256(content digest))`, the digest being every changed path paired with its content hash, and replays the stored outcome on a hit — from both the plain `openswarm review` path (`reviewCommand.ts`) and the publication hook (`publicationReviewHook.ts`). Measured over the recorded history (530 records, 512 carrying a verdict): 68 byte-identical pairs, 15 of them same-mode (8 direct→direct, 7 pr→pr) and 10 of those repeating the same verdict — on the order of 10 of 512 reviews, at a measured ~$0.26 and 93s p50 per reviewer call. Proven across two separate processes: a cold 27.0s / 6-model-call review became a 1.58s replay with **zero** model calls, same verdict. The key carries the base ref and the review mode deliberately, because 15 of the 53 cross-mode repeats in that history flipped their verdict — a CLI review of a tree and a publication review of the same tree are different reviews. What is skipped is the reviewer call; the advisor pass still runs over the reused verdict, so a replay can tighten (`approve`→`revise`/`reject`) exactly as a live run would and the cache cannot fail open past an escalation. +- **The reuse survives a daemon restart.** The verdict lives in its own `publication_reviews` table in the automation database rather than in the in-process `reviewedPublications` Set it replaces, which was empty after every restart — the failure census records 141 `owner_process_exited` and 157 `shutdown_cancelled`, each of which threw away a verdict already paid for. A store that cannot be opened does not suppress a review; the hook reviews instead. - **`openswarm review --max --harness-only` runs a deterministic quality harness with no LLM cost.** The new harness (`src/verify/qualityHarness.ts`) enumerates every **tracked** source file via the git index and scans each one, then runs the discovered typecheck/lint/test/build commands inside the existing isolated verify sandbox. It is fail-closed by construction: a file over the 512 KiB ceiling, one containing NUL bytes, an unreadable path, a symlinked source, or a path that escapes the repository root each become an explicit error finding rather than a silent skip, and a listing that scanned nothing is itself an error — so the gate cannot report "passed" over a subset it never read. Findings are folded into the audit run as a synthetic `.openswarm/quality-harness` area, which means the markdown report and the exit-code contract carry the evidence on every `--max` run, not only when an LLM area happened to notice something. ### Fixed +- **`openswarm review --max` no longer changes its verdict with `--concurrency`.** The audit re-partitioned the source into reviewer areas until the fan-out saturated the pool (INT-2249), so the same files and the same reviewer produced 2 areas at concurrency 1 and 10 at concurrency 8 — and because `aggregateAuditResults` is worst-wins, the finer split could only turn an `approve` into a `revise`/`reject`. The audit's units of judgement now come from `planAuditAreas`, which is derived from `--max-files-per-area` alone (`--concurrency` is purely how many reviewers run at once), so a given file set gets one verdict. The finer, more parallel fan-out is still available, deterministically, by lowering `--max-files-per-area`; the `--fix` path keeps the pool-filling `balanceAreasToConcurrency`, where more areas only mean more parallel fix workers. The cost gate now prints the real agent-run count (the area count) and points at the granularity flag when the pool is under-filled. - **A workflow execution can no longer be persisted in a state its DAG forbids.** `saveExecution` now rejects a step marked `completed` without a `completedAt`, `failed` without an `error`, or advanced past a dependency that is still pending, running or failed — states that previously reached disk and were then read back as if the pipeline had progressed. It also gains a `definitionStamp` fence: a definition replaced underneath a live execution makes the next save refuse rather than record a snapshot that never ran against it. - **The local issue store no longer serves a stale snapshot after a same-size replacement.** The cache stamp was `mtimeMs:size`, which is unchanged when another process replaces the file by atomic rename within the same mtime tick and writes the same number of bytes — the process then kept serving the old contents indefinitely. The stamp now includes the inode. - **A status transition's event log records the status the write actually saw.** `changeStatus` and `updateIssue` read the current status inside the write transaction instead of from a pre-transaction read, so a concurrent transition can no longer stamp a stale `oldValue` into the audit trail. @@ -22,6 +27,8 @@ - **The `dev.ts` close handler always releases its task.** Reporting ran before `onComplete` and `activeTasks.delete`, so a throw while formatting cost/output left the task registered forever; the reporting is now contained and the cleanup runs in `finally`. - **`memoryBridge` emits one `memory_linked` event per link**, not two (`linkMemory` already emits one). - **A failed Linear SDK load is retryable.** The rejected init promise stayed cached, so every later `initLinearBridge` call awaited the same rejection and the bridge never recovered within the process. +- **`advisor` role: a second, independent review of the same diff.** Ported from the harness agent patterns. A separate, independently-prompted model is asked one narrow question — *what concrete defects did the reviewer miss?* — and its only permitted effect is to make the gate **more** cautious: it may append concrete findings the reviewer did not report and raise severity, but the merged decision is the max rank of the two (`approve < revise < reject`), so an advisor `approve` can never soften a reviewer `revise`/`reject`, and a raised severity with no concrete finding is discarded. Any error, timeout, empty, or unparseable output fails **open** — `ran: false`, the reviewer's result untouched, no exit code changed. `review`, `review --max` (per area), and the `--fix` re-review loop all run it before dedupe, so its findings are deduped against history like any other. Disabled by default (a second paid call per review), configured under `autonomous.defaultRoles.advisor`; its model must come from a different family than the reviewer's or it is a second identical opinion. +- **Declarative per-role subagent settings: `tools` and `effort`.** Each role (`worker`/`reviewer`/`advisor`/…) may declare `tools.allow` / `tools.deny` and `effort`. An allow-list can only **narrow** the role's default tool set — it can never grant a tool the role would not otherwise have, so a misconfiguration cannot hand a read-only reviewer `bash`. `deny` is applied after `allow` (deny wins) and supports a trailing `*` (`scratch_*`). `effort` selects the native-loop reasoning level for the stage. ## 0.24.3 — 2026-09-28 diff --git a/README.md b/README.md index fda16215..f47c8763 100644 --- a/README.md +++ b/README.md @@ -95,7 +95,8 @@ openswarm review --max --fix # after the audit, dependency-related findings # a PR is published only after every re-review and trusted # deterministic repository check passes # add --in-place to edit the current working tree instead -openswarm review --max --concurrency 8 # widen the fan-out — areas auto-split to fill the pool +openswarm review --max --max-files-per-area 6 # finer fan-out — the partition is this knob + # alone; --concurrency only sets how many run at once # more --max flags: --no-linear (report only) · --issues-per-area # (legacy spray) · --issues (set parent) · --fallback # · --out · --dry-run (print the plan) @@ -754,7 +755,7 @@ Agent-oriented module map, admission gates, and landmines: **[ARCHITECTURE.md](A - **BS Detector** — Built-in static analysis engine that detects bad code patterns (empty catch, hardcoded secrets, `as any`, etc.) with pipeline guard integration - **Autonomous Pipeline** — Cron-driven heartbeat fetches Linear issues, runs Worker/Reviewer pair loops, and updates issue state automatically - **Worker/Reviewer Pairs** — Multi-iteration code generation with automated review, testing, and documentation stages -- **Codebase Audit (`review --max`)** — fans reviewer subagents out over directory-shaped areas (auto-split to fill `--concurrency`), aggregates a deduped verdict into a markdown report, and synthesizes ≤10 cohesive Linear issues via a PM agent. Both `review` modes consult repository-local prior review logs; resolved/stale findings are not repeated, and byte-identical duplicate follow-ups are suppressed while unresolved issues remain visible. `--fix` groups findings by repository dependency closure, injects the package manager/manifests/verification contract and repo knowledge, runs only independent fix units concurrently in isolated sandboxes, and promotes disjoint in-scope diffs into an audit worktree. It publishes the PR only when every area re-approves and trusted deterministic verification passes; unavailable dependencies/checks fail closed. `--in-place` keeps edits in the current working tree but uses the same gates. Language-agnostic; codex usage-limit aware with automatic `claude` fallback +- **Codebase Audit (`review --max`)** — fans reviewer subagents out over directory-shaped areas (partitioned by `--max-files-per-area`; `--concurrency` only sets how many run at once, so one file set gets one verdict), aggregates a deduped verdict into a markdown report, and synthesizes ≤10 cohesive Linear issues via a PM agent. Both `review` modes consult repository-local prior review logs; resolved/stale findings are not repeated, and byte-identical duplicate follow-ups are suppressed while unresolved issues remain visible. A review of content this deployment already reviewed is replayed from a durable `(kind, base, content digest)` store instead of re-paying for the reviewer, and that store survives a daemon restart. `--fix` groups findings by repository dependency closure, injects the package manager/manifests/verification contract and repo knowledge, runs only independent fix units concurrently in isolated sandboxes, and promotes disjoint in-scope diffs into an audit worktree. It publishes the PR only when every area re-approves and trusted deterministic verification passes; unavailable dependencies/checks fail closed. `--in-place` keeps edits in the current working tree but uses the same gates. Language-agnostic; codex usage-limit aware with automatic `claude` fallback - **CI / test gate auto-fix (`openswarm fix`)** — runs the project's objective checks (lint / typecheck / build / test), groups the failures by file into areas, fans a fix-worker out over each, then **re-runs the checks and repeats until green** (or the round budget). Deterministic convergence — unlike the review fix pass, it verifies its own work. Multi-language: auto-detects npm scripts, `Cargo.toml` (`cargo check`/`test`, clippy on request), and Python tooling (`ruff`/`mypy`/`pytest`, gated on the repo's config); any other toolchain via a `"checks"` map in `openswarm.json` - **PR autopilot (`openswarm pr`)** — on-demand surface over the daemon's PRProcessor + `commitAndCreatePR`. `status` reports conflicts / review feedback / CI; `fix` runs one autopilot pass (conflict → comments → CI); `review` re-applies reviewer feedback only, skipping conflict/CI work — recognizes Claude, Codex, and any formal `CHANGES_REQUESTED` review; `review --fresh` instead runs a brand-new code review of the PR's current diff (the same reviewer `openswarm review` uses) and posts the verdict as a PR comment, independent of any existing feedback; `review --all` reviews every open PR in the repo sequentially (combine with `--fresh`) instead of just the current branch's PR or `--number`; `watch` loops until merge-ready; `create` publishes the current feature branch (local fix → commit → push → `gh pr create`). Never merges or enables auto-merge. - **Decision Engine** — Scope validation, priority-based task selection, and workflow mapping diff --git a/config.example.yaml b/config.example.yaml index 9605cd64..ddd39532 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -183,6 +183,18 @@ autonomous: # run ends on completion, the repeated-tool-call guard, or timeoutMs — a turn # count is not a property of the task. Set a number only to cap cost hard. # maxTurns: 0 + # + # Declarative tool scoping per role. `allow` can only NARROW the role's + # default tools — it can never grant one the role would not otherwise have, + # so an allow-list naming `bash` on a read-only role stays withheld. `deny` + # is applied after `allow` (deny wins), and supports a trailing `*` + # (`scratch_*`). Use this to keep a stage's blast radius explicit instead + # of relying on the stage's defaults. + # tools: + # allow: [read_file, write_file, edit_file, bash] + # deny: [web_fetch, web_search] + # reasoning effort for this role's native-loop adapter (low|medium|high) + # effort: medium reviewer: enabled: true model: gpt-5.6-sol # Correctness gate; light profile lowers this to Terra @@ -192,6 +204,23 @@ autonomous: # diff-scaled default (300s base); a slow model needs more — measured # 2026-09-17: half the reviews died at 300s. maxTurns 0 = no ceiling. timeoutMs: 600000 + # Second, independently-prompted review of the SAME diff. Its only permitted + # effect is to add findings the reviewer missed and to raise severity — it + # can never soften the reviewer's verdict, and any raised severity without a + # concrete finding is discarded. DISABLED by default: it is a second paid + # call on every review. + # + # Its model must come from a DIFFERENT family than the reviewer's. On the + # reviewer's own model the advisor is a second identical opinion — the same + # weights re-deriving the same blind spots (the trap modelCompat.ts + # documents for `escalate`). The shipped reviewer default is + # deepseek/deepseek-v4-flash; z-ai/glm-5.2 is the family-independent + # alternative measured at 100% detect / 6% false-reject (6s avg) on the + # planted-defect fixtures where the reviewer scored 0% false-reject at 36s. + advisor: + enabled: false + model: z-ai/glm-5.2 + timeoutMs: 45000 # 45s single-turn ceiling, as the guard arbiter uses tester: enabled: false model: gpt-5.6-terra # Used only if deterministic verify cannot run diff --git a/package-lock.json b/package-lock.json index 0d9dca52..9eebab0e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@intrect/openswarm", - "version": "0.24.3", + "version": "0.24.4", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@intrect/openswarm", - "version": "0.24.3", + "version": "0.24.4", "license": "MIT", "dependencies": { "@anthropic-ai/sdk": "^0.72.1", diff --git a/package.json b/package.json index 4bb6985f..9c99d659 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@intrect/openswarm", - "version": "0.24.3", + "version": "0.24.4", "description": "Autonomous AI agent orchestrator — Claude, GPT, Codex, and local models (Ollama/LMStudio/llama.cpp)", "license": "MIT", "type": "module", diff --git a/src/adapters/agenticLoop.test.ts b/src/adapters/agenticLoop.test.ts index 0fbcd15e..549649fa 100644 --- a/src/adapters/agenticLoop.test.ts +++ b/src/adapters/agenticLoop.test.ts @@ -534,6 +534,135 @@ describe('runAgenticLoop tool exposure options', () => { expect(toolNames).not.toContain('search_memory'); }); + it('exposes only the allow-listed tools, and a deny entry removes what allow kept', async () => { + let toolNames: string[] = []; + + await runAgenticLoop({ + prompt: 'x', + cwd: process.cwd(), + model: 'test', + webTools: false, + memoryTools: false, + // `search_files` is allow-listed and then denied: allow can never add a + // tool back that deny removed, not even a member of `allow` itself. + toolAllow: ['read_file', 'search_files', 'write_file'], + toolDeny: ['search_files'], + maxTurns: 1, + callApi: async (_messages, tools) => { + toolNames = tools.map((tool) => tool.function.name); + return finalResp('done'); + }, + }); + + expect(toolNames).toEqual(['read_file', 'write_file']); + }); + + it('a deny wildcard withholds the whole scratch family', async () => { + let toolNames: string[] = []; + + await runAgenticLoop({ + prompt: 'x', + cwd: process.cwd(), + model: 'test', + webTools: false, + memoryTools: false, + scratchpadRunId: 'AGT-0000', + toolDeny: ['scratch_*'], + maxTurns: 1, + callApi: async (_messages, tools) => { + toolNames = tools.map((tool) => tool.function.name); + return finalResp('done'); + }, + }); + + expect(toolNames).not.toContain('scratch_write'); + expect(toolNames).not.toContain('scratch_read'); + expect(toolNames).toContain('read_file'); + }); + + it('an allow-list cannot resurrect bash on a read-only run', async () => { + // The narrowing-only invariant: `allow` intersects with the composition, so + // naming a withheld tool does not expose it. A read-only run (the reviewer's + // shape) keeps bash hidden no matter what the role declared. + let toolNames: string[] = []; + + await runAgenticLoop({ + prompt: 'x', + cwd: process.cwd(), + model: 'test', + readOnly: true, + webTools: false, + memoryTools: false, + toolAllow: ['bash', 'read_file', 'write_file'], + maxTurns: 1, + callApi: async (_messages, tools) => { + toolNames = tools.map((tool) => tool.function.name); + return finalResp('done'); + }, + }); + + expect(toolNames).toEqual(['read_file']); + }); + + it('refuses a tool the role scope narrowed away, at dispatch as well as in the schema', async () => { + // The schema above and the dispatch set below are the same narrowed array, + // so a provider that emits a withheld name anyway is answered, not obeyed. + let turn = 0; + let deniedResult = ''; + + await runAgenticLoop({ + prompt: 'x', + cwd: process.cwd(), + model: 'test', + webTools: false, + memoryTools: false, + toolAllow: ['read_file'], + maxTurns: 2, + callApi: async (messages, tools) => { + if (turn++ === 0) { + expect(tools.map((tool) => tool.function.name)).toEqual(['read_file']); + return toolCallResp('hidden-write', 'write_file', { path: 'out.txt', content: 'x' }); + } + deniedResult = messages.at(-1)?.content ?? ''; + return finalResp('done'); + }, + }); + + expect(deniedResult).toContain('TOOL_NOT_ALLOWED'); + expect(deniedResult).toContain('write_file'); + }); + + it('leaves MCP and coordination tools to their own flags, not the role list', async () => { + // RoleConfig.tools names built-ins (see its doc): a role narrowing to the + // file tools must not silently lose its board access, which the MCP and + // coordination flags govern. + let toolNames: string[] = []; + + await runAgenticLoop({ + prompt: 'x', + cwd: process.cwd(), + model: 'test', + webTools: false, + memoryTools: false, + toolAllow: ['read_file'], + maxTurns: 1, + mcpTools: [{ + type: 'function', + function: { name: 'linear__get_issue', description: '', parameters: { type: 'object' } }, + }], + coordinationContext: { repository: '/repo', taskId: 'supervisor', actor: 'orchestrator' }, + callApi: async (_messages, tools) => { + toolNames = tools.map((tool) => tool.function.name); + return finalResp('done'); + }, + }); + + expect(toolNames).toContain('read_file'); + expect(toolNames).toContain('linear__get_issue'); + expect(toolNames).toContain('coordination_read'); + expect(toolNames).not.toContain('write_file'); + }); + it('withholds the scratch tools when the run has no scratchpad', async () => { let toolNames: string[] = []; await runAgenticLoop({ diff --git a/src/adapters/agenticLoop.ts b/src/adapters/agenticLoop.ts index 91226a41..d38a6a96 100644 --- a/src/adapters/agenticLoop.ts +++ b/src/adapters/agenticLoop.ts @@ -202,6 +202,16 @@ export interface AgenticLoopOptions { webTools?: boolean; /** Expose search_memory (default true). Disabled for isolated/temp repo benchmarks. */ memoryTools?: boolean; + /** + * Declarative per-role tool scope (RoleConfig.tools), naming BUILT-IN tools. + * `toolAllow` keeps only the names it lists; `toolDeny` then removes names from + * what remains, a trailing `*` standing for a prefix (`scratch_*`). Both run over + * the built-ins left by the readOnly/shell/web rules, so they only ever NARROW — + * `allow` cannot resurrect `bash` on a readOnly run. MCP and coordination tools + * keep their own flags; the narrowed set is also the dispatch allow-list. + */ + toolAllow?: string[]; + toolDeny?: string[]; /** * Run whose scratchpad `scratch_write`/`scratch_read` address (AGT-4459). * Absent means no scratchpad: the two tools are withheld from the model and @@ -287,6 +297,33 @@ export interface AgenticLoopResult { // ============ 에이전틱 루프 ============ +/** + * Apply a role's declarative `tools.allow` / `tools.deny` (RoleConfig) to the + * built-in tools the readOnly / scratch / shell / web filters have already shaped. + * + * It runs LAST and only removes entries — an allow-list cannot resurrect a tool an + * earlier rule withheld (a `bash` in `allow` stays hidden on a readOnly run), and + * `deny` follows `allow`, so the two cannot contradict each other. A `deny` entry + * ending in `*` matches by prefix (`scratch_*`), how the scratch tools are + * addressed as a family. + */ +function applyRoleToolScope( + tools: ToolDefinition[], + toolAllow?: string[], + toolDeny?: string[], +): ToolDefinition[] { + let scoped = tools; + if (toolAllow && toolAllow.length > 0) { + const allowed = new Set(toolAllow); + scoped = scoped.filter((tool) => allowed.has(tool.function.name)); + } + return toolDeny && toolDeny.length > 0 + ? scoped.filter((tool) => !toolDeny.some((denied) => denied.endsWith('*') + ? tool.function.name.startsWith(denied.slice(0, -1)) + : tool.function.name === denied)) + : scoped; +} + /** * 에이전틱 도구 루프 실행 * @@ -359,6 +396,8 @@ async function runAgenticLoopInner( bashTimeoutMs, webTools = true, memoryTools = true, + toolAllow, + toolDeny, scratchpadRunId, shellTools: requestedShellTools = true, sandboxExecutorSessionFactory, @@ -444,23 +483,31 @@ async function runAgenticLoopInner( const visibleBaseTools = readOnly ? shellFilteredTools.filter((t) => !['write_file', 'edit_file', 'bash', 'remember'].includes(t.function.name)) : shellFilteredTools; - const tools = enableTools + const builtinTools = enableTools ? [ ...visibleBaseTools, ...(filesystemTools && applyPatch && editFormat === 'json' && !readOnly ? [APPLY_PATCH_TOOL] : []), // Not in readOnly: it spawns compiler subprocesses, matching bash's exclusion. ...(filesystemTools && diagnosticsTool && !readOnly && shellTools ? [DIAGNOSTICS_TOOL] : []), - // Both are withheld in readOnly. A read-only run exists because the - // material under inspection is untrusted, and a fetch is an outbound - // channel for anything the agent can read — the provider credential - // included. MCP servers are withheld for the mirror reason: OpenSwarm's - // own memory server exposes writes, so injected content could leave - // something behind for a later run. (INT-3189) + // Also withheld in readOnly: the material under inspection is untrusted, + // and a fetch is an outbound channel for anything the agent can read — + // the provider credential included. (INT-3189) ...(webTools && !readOnly ? WEB_TOOL_DEFINITIONS : []), + ] + : []; + // MCP and coordination tools keep their own flags; the role list names built-ins. + // MCP is withheld in readOnly for the mirror reason: our memory server exposes + // writes, so injected content could leave something behind. (INT-3189) + const externalTools = enableTools + ? [ ...(readOnly ? [] : humanSurfaceFilteredMcp.tools), ...(readOnly || !coordinationContext ? [] : COORDINATION_TOOL_DEFINITIONS), ] : []; + // The role's declared scope goes LAST, over the built-ins every rule above has + // already shaped, so it intersects with them instead of overriding one — see + // applyRoleToolScope. `allowedToolNames` below is built from the same result. + const tools = [...applyRoleToolScope(builtinTools, toolAllow, toolDeny), ...externalTools]; // The provider-visible schema is not an enforcement boundary. Carry the // exact same set into dispatch so a hidden tool call cannot reach a globally // registered MCP route (or another built-in withheld for this run). diff --git a/src/adapters/atlascloud.ts b/src/adapters/atlascloud.ts index 49ec8623..f968bcf8 100644 --- a/src/adapters/atlascloud.ts +++ b/src/adapters/atlascloud.ts @@ -164,6 +164,8 @@ export class AtlasCloudCliAdapter implements CliAdapter { webTools: options.webTools, memoryTools: options.memoryTools, shellTools: options.shellTools, + toolAllow: options.toolAllow, + toolDeny: options.toolDeny, filesystemTools: options.filesystemTools, diagnosticsTool: options.diagnosticsTool, readOnly: options.readOnly, diff --git a/src/adapters/base.spawn.test.ts b/src/adapters/base.spawn.test.ts index dc69230a..4c77c9f8 100644 --- a/src/adapters/base.spawn.test.ts +++ b/src/adapters/base.spawn.test.ts @@ -533,6 +533,71 @@ describe('delegated-CLI capability guards', () => { ).rejects.toThrow(/cannot withhold shell access/); }); + it('warns that a role tool allow/deny list is inert on a delegated CLI, without failing the run', async () => { + // The delegated CLI owns its own tools, so the list cannot be enforced here. + // Warning (not throwing) keeps existing claude/codex configs running while + // making sure a fence nobody applies is never silent. (AGT-4444 class) + const warns: string[] = []; + const warn = vi.spyOn(console, 'warn').mockImplementation((line: unknown) => { warns.push(String(line)); }); + const proc = Object.assign(new EventEmitter(), { + pid: 311, + stdout: new PassThrough(), + stderr: new PassThrough(), + stdin: Object.assign(new EventEmitter(), { end: vi.fn() }), + kill: vi.fn(), + }); + spawnMock.mockImplementationOnce(() => { + queueMicrotask(() => { + proc.stdout.end('ok'); + proc.emit('close', 0); + }); + return proc; + }); + try { + await expect(spawnCli(delegated(), { + prompt: 'p', cwd: process.cwd(), toolAllow: ['read_file'], toolDeny: ['scratch_*'], + })).resolves.toMatchObject({ stdout: 'ok' }); + } finally { + warn.mockRestore(); + } + expect(warns.join('\n')).toContain('the role tool allow/deny list'); + }); + + it('warns that protectedFiles and forbidPublication are inert on a delegated CLI (AGT-4444)', async () => { + // Same failure class as the tool list: these fences live in the in-process + // tool executor, so a role routed to a delegated CLI silently loses them. + // The warning is the defect's whole point — silence is what made a dead + // fence look like a working one. + const warns: string[] = []; + const warn = vi.spyOn(console, 'warn').mockImplementation((line: unknown) => { warns.push(String(line)); }); + const proc = Object.assign(new EventEmitter(), { + pid: 312, + stdout: new PassThrough(), + stderr: new PassThrough(), + stdin: Object.assign(new EventEmitter(), { end: vi.fn() }), + kill: vi.fn(), + }); + spawnMock.mockImplementationOnce(() => { + queueMicrotask(() => { + proc.stdout.end('ok'); + proc.emit('close', 0); + }); + return proc; + }); + try { + await expect(spawnCli(delegated(), { + prompt: 'p', cwd: process.cwd(), + protectedFiles: ['secrets.env'], + forbidPublication: true, + })).resolves.toMatchObject({ stdout: 'ok' }); + } finally { + warn.mockRestore(); + } + const text = warns.join('\n'); + expect(text).toContain('1 protected path(s)'); + expect(text).toContain('the publication fence'); + }); + it('does not construct or spawn a delegated fake CLI in strict mode, even with HOME credentials', async () => { const buildCommand = vi.fn(() => ({ command: 'fake-codex', args: [] })); const adapter = { ...delegated(), name: 'fake-codex', buildCommand } satisfies CliAdapter; diff --git a/src/adapters/base.ts b/src/adapters/base.ts index c33a73cf..406a8b78 100644 --- a/src/adapters/base.ts +++ b/src/adapters/base.ts @@ -98,11 +98,24 @@ export async function spawnCli( // Below this line the adapter runs its own tool loop inside its own CLI, so // anything OpenSwarm assembles for *our* loop is dropped. Silence there is // how a configured MCP grant or an `ask_human` escape hatch turns into an - // agent that quietly never had it — say it out loud instead. - if (options.mcpTools?.length || options.coordinationContext) { + // agent that quietly never had it — say it out loud instead. The same goes for + // a role's `tools.allow`/`tools.deny`, and for `protectedFiles` / + // `forbidPublication`: this path cannot honor any of them (the CLI owns its + // tools), and a silently inert fence is worse than none. (AGT-4444) + if ( + options.mcpTools?.length + || options.coordinationContext + || options.toolAllow?.length + || options.toolDeny?.length + || options.protectedFiles?.length + || options.forbidPublication + ) { const dropped = [ options.mcpTools?.length ? `${options.mcpTools.length} MCP tool(s)` : '', options.coordinationContext ? 'coordination tools' : '', + options.toolAllow?.length || options.toolDeny?.length ? 'the role tool allow/deny list' : '', + options.protectedFiles?.length ? `${options.protectedFiles.length} protected path(s)` : '', + options.forbidPublication ? 'the publication fence' : '', ].filter(Boolean).join(' and '); console.warn( `[Adapter] '${adapter.name}' delegates to its own CLI tool loop; ${dropped} will not be available to this run. ` diff --git a/src/adapters/codexResponses.ts b/src/adapters/codexResponses.ts index 4ac31610..2e78a403 100644 --- a/src/adapters/codexResponses.ts +++ b/src/adapters/codexResponses.ts @@ -455,6 +455,8 @@ export class CodexResponsesAdapter implements CliAdapter { webTools: options.webTools, memoryTools: options.memoryTools, shellTools: options.shellTools, + toolAllow: options.toolAllow, + toolDeny: options.toolDeny, filesystemTools: options.filesystemTools, diagnosticsTool: options.diagnosticsTool, mcpTools: options.mcpTools, diff --git a/src/adapters/gpt.ts b/src/adapters/gpt.ts index 165928f5..c8c891ce 100644 --- a/src/adapters/gpt.ts +++ b/src/adapters/gpt.ts @@ -164,6 +164,8 @@ export class GptCliAdapter implements CliAdapter { webTools: options.webTools, memoryTools: options.memoryTools, shellTools: options.shellTools, + toolAllow: options.toolAllow, + toolDeny: options.toolDeny, filesystemTools: options.filesystemTools, diagnosticsTool: options.diagnosticsTool, readOnly: options.readOnly, diff --git a/src/adapters/local.ts b/src/adapters/local.ts index 83d7388e..160c8215 100644 --- a/src/adapters/local.ts +++ b/src/adapters/local.ts @@ -202,6 +202,8 @@ export class LocalModelAdapter implements CliAdapter { webTools: options.webTools, memoryTools: options.memoryTools, shellTools: options.shellTools, + toolAllow: options.toolAllow, + toolDeny: options.toolDeny, filesystemTools: options.filesystemTools, diagnosticsTool: options.diagnosticsTool, readOnly: options.readOnly, diff --git a/src/adapters/modelCompat.ts b/src/adapters/modelCompat.ts index 377e99e8..bcffc317 100644 --- a/src/adapters/modelCompat.ts +++ b/src/adapters/modelCompat.ts @@ -33,7 +33,7 @@ const CLAUDE_ALIASES = new Set(['sonnet', 'opus', 'haiku']); * without a home here is a compile error, not a silent fall to the bulk model. */ export type ModelRole = - | 'worker' | 'reviewer' | 'tester' | 'documenter' | 'auditor' | 'skill-documenter' + | 'worker' | 'reviewer' | 'advisor' | 'tester' | 'documenter' | 'auditor' | 'skill-documenter' | 'planner' | 'orchestrator' | 'escalate'; // Verified against `cursor-agent --list-models` (2026.09.08, vela, 2026-09-10): @@ -45,6 +45,13 @@ const CURSOR_JUDGE_MODEL = 'cursor-grok-4.6-high'; // `reviewer` because a tier that resolves to the same model as the tier it // escalates FROM is a log line claiming work that did not happen. const CURSOR_ESCALATE_MODEL = 'cursor-grok-4.6-xhigh'; +// The advisor must not resolve to the reviewer's model either — same reason as +// the escalation above, one tier over: an advisor on `cursor-grok-4.6-high` +// would be the reviewer's own weights asked the same question twice. cursor's +// catalogue carries exactly two judge-grade ids, so the advisor takes the other +// one (`-xhigh`); the collision with escalate is the catalogue's limit, not a +// routing decision, and the two never run as the same role. +const CURSOR_ADVISOR_MODEL = CURSOR_ESCALATE_MODEL; const ADAPTER_DEFAULT_MODEL: Partial> = { 'codex-responses': 'gpt-5.6-terra', @@ -98,6 +105,7 @@ const CURSOR_ROLE_MODEL: Readonly> = { documenter: CURSOR_BULK_MODEL, 'skill-documenter': CURSOR_BULK_MODEL, reviewer: CURSOR_JUDGE_MODEL, + advisor: CURSOR_ADVISOR_MODEL, auditor: CURSOR_JUDGE_MODEL, planner: CURSOR_JUDGE_MODEL, orchestrator: CURSOR_JUDGE_MODEL, diff --git a/src/adapters/ollamaCloud.ts b/src/adapters/ollamaCloud.ts index b82f7877..111c522b 100644 --- a/src/adapters/ollamaCloud.ts +++ b/src/adapters/ollamaCloud.ts @@ -406,6 +406,8 @@ export class OllamaCloudAdapter extends LocalModelAdapter implements CliAdapter webTools: options.webTools, memoryTools: options.memoryTools, shellTools: options.shellTools, + toolAllow: options.toolAllow, + toolDeny: options.toolDeny, filesystemTools: options.filesystemTools, diagnosticsTool: options.diagnosticsTool, readOnly: options.readOnly, diff --git a/src/adapters/openrouter.ts b/src/adapters/openrouter.ts index 3afb415d..c915f08f 100644 --- a/src/adapters/openrouter.ts +++ b/src/adapters/openrouter.ts @@ -198,6 +198,8 @@ export class OpenRouterCliAdapter implements CliAdapter { webTools: options.webTools, memoryTools: options.memoryTools, shellTools: options.shellTools, + toolAllow: options.toolAllow, + toolDeny: options.toolDeny, filesystemTools: options.filesystemTools, diagnosticsTool: options.diagnosticsTool, readOnly: options.readOnly, diff --git a/src/adapters/resultParsing.test.ts b/src/adapters/resultParsing.test.ts index 3bfe09b0..5ce304d0 100644 --- a/src/adapters/resultParsing.test.ts +++ b/src/adapters/resultParsing.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { parseReviewerResult, parseWorkerResult } from './resultParsing.js'; +import { findStringAwareJsonObject, parseReviewerResult, parseWorkerResult } from './resultParsing.js'; import { t } from '../locale/index.js'; const wrap = (obj: unknown) => '```json\n' + JSON.stringify(obj) + '\n```'; @@ -321,3 +321,54 @@ describe('a limitation report is not a failure declaration (AGT-4534)', () => { expect(isExplicitFailure('Failed to apply the patch.')).toBe(true); }); }); + +describe('findStringAwareJsonObject (AGT-3466)', () => { + const find = (text: string) => findStringAwareJsonObject(text, '"success"'); + + it('does not end the object at a brace inside a quoted string', () => { + // The unscanned variant sliced to the `}` inside the summary, so + // JSON.parse failed on an unterminated string and every field was lost. + expect(find('{"success": true, "summary": "Use `{}` here"}')) + .toBe('{"success": true, "summary": "Use `{}` here"}'); + expect(find('{"success": true, "summary": "trailing } here"}')) + .toBe('{"success": true, "summary": "trailing } here"}'); + expect(find('{"success": true, "summary": "the { never closes"}')) + .toBe('{"success": true, "summary": "the { never closes"}'); + }); + + it('honors backslash escapes when deciding string boundaries', () => { + // Without escape state the quote inside \"}\" closes the string early, and + // the brace that follows is then read as structure. + expect(find('{"success": true, "summary": "say \\"}\\" here"}')) + .toBe('{"success": true, "summary": "say \\"}\\" here"}'); + // An escaped backslash is a literal, so the next quote really does close. + expect(find('{"success": true, "summary": "path C:\\\\ then } here"}')) + .toBe('{"success": true, "summary": "path C:\\\\ then } here"}'); + }); + + it('returns the object enclosing the marker, not a nested object before it', () => { + // lastIndexOf('{') would settle on the inner object and hand back a + // fragment with no `success` field in it. + expect(find('{"a": {"b": 1}, "success": true}')).toBe('{"a": {"b": 1}, "success": true}'); + }); + + it('skips prose that names the marker before the object appears', () => { + expect(find('The "success" flag was set. Result: {"success":true,"summary":"done"}')) + .toBe('{"success":true,"summary":"done"}'); + }); + + it('skips stray braces in the prose before the object', () => { + // A `{` in prose opens a candidate that never balances; a `}` closes one + // that never opened. Neither may abort the scan or become the result. + expect(find('noise } then {"success":true}')).toBe('{"success":true}'); + expect(find('noise { then {"success":true}')).toBe('{"success":true}'); + expect(find('oops { {"success":true}')).toBe('{"success":true}'); + expect(find('sibling {"x":1} then {"success":true}')).toBe('{"success":true}'); + }); + + it('returns null when there is no enclosing object', () => { + expect(find('no json at all')).toBeNull(); + expect(find('{"success": true')).toBeNull(); + expect(find('"success" with no object')).toBeNull(); + }); +}); diff --git a/src/adapters/resultParsing.ts b/src/adapters/resultParsing.ts index cd6344f5..a4f70ea2 100644 --- a/src/adapters/resultParsing.ts +++ b/src/adapters/resultParsing.ts @@ -166,7 +166,13 @@ function extractBulletsAfter(text: string, heading: RegExp): string[] { return items; } -/** Brace-balanced scan for the JSON object containing `marker`. */ +/** + * Brace-balanced scan for the JSON object containing `marker`, counting braces + * blindly. Kept as the worker/reviewer adapters' path (their output is a + * structured completion, not prose); `findStringAwareJsonObject` below is the + * variant for result JSON that carries free text. Fixing brace handling in one + * is not fixing the other — check both call sites. (AGT-3466) + */ function findJsonObject(text: string, marker: string): string | null { const idx = text.indexOf(marker); if (idx < 0) return null; @@ -187,6 +193,66 @@ function findJsonObject(text: string, marker: string): string | null { return null; } +/** + * String-aware brace-balanced scan for the top-level JSON object containing + * `marker` — the variant for callers whose JSON carries prose. Counting braces + * without tracking quoted strings reads a brace in a value as structure: + * `"summary": "Use \`{}\` here"` sliced to the brace inside the string, so + * JSON.parse threw on an unterminated string and the caller fell back to its + * lossy text heuristic, losing every structured field. Escape state is tracked + * for the same reason — the quote in `\"}\"` would otherwise close the string + * early and expose the brace as structure. + * + * Returns the object that ENCLOSES the marker, not the nearest `{` before it, so + * a nested object earlier in the text (`{"a": {"b": 1}, "success": true}`) is + * not mistaken for the result. Every occurrence of the marker is tried, because + * prose can name the field before the object appears. (AGT-3466) + */ +export function findStringAwareJsonObject(text: string, marker: string): string | null { + for (let idx = text.indexOf(marker); idx >= 0; idx = text.indexOf(marker, idx + 1)) { + const found = enclosingObject(text, idx); + if (found) return found; + } + return null; +} + +/** The outermost brace-balanced object spanning `idx`, or null. */ +function enclosingObject(text: string, idx: number): string | null { + // Each `{` at or before the marker is a candidate, tried in position order so + // the outermost one wins. A candidate that closes before the marker is a + // sibling object, and one that never closes is an unbalanced `{` in prose; + // both are skipped rather than aborting the scan — prose before the result + // routinely carries stray braces of either kind. + for (let start = text.indexOf('{'); start >= 0 && start <= idx; start = text.indexOf('{', start + 1)) { + const end = endOfBalancedObject(text, start); + if (end !== null && end > idx) return text.slice(start, end); + } + return null; +} + +/** Index just past the `}` matching the `{` at `start`, tracking quoted strings. */ +function endOfBalancedObject(text: string, start: number): number | null { + let depth = 0; + let inString = false; + let escaped = false; + + for (let i = start; i < text.length; i++) { + const ch = text[i]; + + if (escaped) { escaped = false; continue; } + if (ch === '\\') { escaped = true; continue; } + if (ch === '"') { inString = !inString; continue; } + if (inString) continue; + + if (ch === '{') depth++; + if (ch === '}') { + depth--; + if (depth === 0) return i + 1; + } + } + return null; +} + /** * A statement of what the agent could not check, which the worker prompt * requires as a "Could not verify" section (locale/prompts/*.ts). It names a diff --git a/src/adapters/shellCommandGuard.test.ts b/src/adapters/shellCommandGuard.test.ts new file mode 100644 index 00000000..7e04b11a --- /dev/null +++ b/src/adapters/shellCommandGuard.test.ts @@ -0,0 +1,160 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import fs from 'node:fs/promises'; +import { executeTool, ToolCall } from './tools.js'; +import { isCommandBlocked } from './shellCommandGuard.js'; + +/** Helper to build a ToolCall object */ +function makeCall(name: string, args: Record): ToolCall { + return { id: 'tc-1', function: { name, arguments: JSON.stringify(args) } }; +} + +const TMP_DIR = await fs.mkdtemp('/tmp/openswarm-guard-test-'); + +beforeAll(async () => { + await fs.mkdir(TMP_DIR, { recursive: true }); +}); + +afterAll(async () => { + await fs.rm(TMP_DIR, { recursive: true, force: true }); +}); + +// ────────────────────────────────────────────── +// AGT-3436 — destructive commands that never contain the literal +// ────────────────────────────────────────────── + +/** + * Each of these executes a destructive command while containing no literal any + * of the old patterns looked for. Bash rewrites the line before running it: + * quote removal (`r"m"`), backslash escapes (`\rm`), empty-quote splicing + * (`g''it`), and brace expansion (`r{m,}`) all reconstruct the verb. + */ +const rewriteBypasses = [ + 'r"m" -rf /foo', + '\\rm -rf /foo', + "g''it clean -fdx", + 'r{m,} -rf /foo', + "$'\\x72\\x6d' -rf /foo", + 'rm -r -f /foo', + 'rm -fr /foo', + 'git clean -fd', + 'git clean -f -d', +]; + +describe('destructive-command guard sees what the shell will run (AGT-3436)', () => { + it.each(rewriteBypasses)('blocks the shell-rewritten form: %s', (command) => { + expect(isCommandBlocked(command)).toBe(true); + }); + + it.each(rewriteBypasses)('refuses it through the bash tool too: %s', async (command) => { + const result = await executeTool(makeCall('bash', { command }), TMP_DIR); + expect(result.is_error).toBe(true); + expect(result.content).toContain('BLOCKED'); + // A refused command ran nothing, so it is no evidence of anything. + expect(result.executed).toBeUndefined(); + }); + + // Destructive verbs reached through a launcher or a nested shell are still + // that verb; the guard follows both. + it.each([ + "sh -c 'rm -rf /foo'", + 'bash -c "git reset --hard"', + 'sudo -u root rm -rf /foo', + 'env FOO=1 rm -rf /foo', + 'echo "x" | xargs rm -rf', + 'FOO=bar rm -rf /foo', + 'cd /tmp && rm -rf /foo', + 'true; rm -rf /foo', + 'rm -rf /foo > /dev/sda', + ])('blocks a destructive command reached indirectly: %s', (command) => { + expect(isCommandBlocked(command)).toBe(true); + }); + + /** + * The other half of AGT-3436: text that merely MENTIONS a destructive command + * is data, not a command. Refusing it teaches the model to route around the + * guard instead of respecting it. + */ + it.each([ + '# rm -rf /tmp/x', + 'echo "rm -rf is blocked"', + 'echo "run rm -rf only when you mean it"', + 'git status', + 'git log --oneline -5', + 'grep -rn "rm -rf" docs', + 'chmod 755 script.sh', + "python -c 'print(1)'", + 'VERSION=$(cat package.json)', + 'for f in $(ls); do echo "$f"; done', + 'echo `date`', + 'git commit -m "fix: rename variable"', + // `rm` without the recursive flag, and `git clean` without force, are + // ordinary parts of a build loop — flagging them would make the guard noise. + 'rm -f ./dist/bundle.js', + 'rm build/output.txt', + 'git clean -n', + 'git clean -nd', + 'chown user:group file.txt', + 'kill -0 1234', + 'pkill -f local-server', + // Ordinary build/verification commands, the guard's main traffic. + 'npm test', + 'npx vitest run src/adapters/tools.test.ts', + 'npx tsc --noEmit', + 'git status --porcelain', + 'git diff HEAD~1', + 'git log --oneline -5 | head -20', + 'rg -n "pattern" src | head -30', + 'sed -n \'1,50p\' src/adapters/tools.ts', + 'python3 -m pytest tests/ -q', + 'cat package.json | jq .version', + 'mkdir -p a/b && touch a/b/c', + 'echo "hello" > out.txt', + 'ls nonexistent 2>&1 | head -3', + 'node -e "console.log(1+1)"', + 'for f in src/*.ts; do echo "$f"; done', + 'git add -A && git commit -m "fix: thing"', + 'git stash', + "curl -sS https://example.com -o /tmp/out.html", + 'find src -name "*.ts" -type f | wc -l', + 'timeout 30 npm test', + 'env NODE_ENV=test npm test', + "bash -c 'echo hello'", + 'sudo -n true 2>/dev/null || echo nope', + "awk '{print $1}' file.txt", + "printf 'a\\nb\\n' > f.txt", + 'npx oxlint src/adapters/tools.ts', + ])('allows a command that only mentions one: %s', (command) => { + expect(isCommandBlocked(command)).toBe(false); + }); + + it('does not refuse a mention that actually runs', async () => { + const result = await executeTool(makeCall('bash', { command: 'echo "rm -rf is blocked"' }), TMP_DIR); + expect(result.is_error).toBe(false); + expect(result.content).toContain('rm -rf is blocked'); + expect(result.content).not.toContain('BLOCKED'); + }); + + // Text the guard cannot resolve is refused rather than guessed at: a false + // positive costs a retry, a false negative costs the working tree. + it.each([ + 'echo "unterminated', + 'echo "r$(true)m -rf /foo"', + 'rm -rf /{a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v}', + ])('refuses what it cannot resolve: %s', (command) => { + expect(isCommandBlocked(command)).toBe(true); + }); + + // A brace that is not an expansion group must not send the scan looking for + // the previous one forever: `awk '{print $1}'` is an ordinary command, and a + // guard that never returns is a denial of service on every bash call. + it.each([ + "awk '{print $1}' file.txt", + "awk '{print}' f.txt", + 'echo "{a,b}"', + 'grep -E "{2,3}" file', + 'echo "}"', + 'echo "{unclosed"', + ])('returns promptly for a brace that is not a group: %s', (command) => { + expect(isCommandBlocked(command)).toBe(false); + }); +}); diff --git a/src/adapters/shellCommandGuard.ts b/src/adapters/shellCommandGuard.ts new file mode 100644 index 00000000..9c148619 --- /dev/null +++ b/src/adapters/shellCommandGuard.ts @@ -0,0 +1,407 @@ +// ============================================ +// OpenSwarm - Destructive shell-command guard +// Split out of tools.ts, which sits near the 1500-line pre-commit cap. +// Purpose: decide whether a bash tool command would run something destructive, +// after the shell's own rewriting (quotes, escapes, braces) is applied. +// ============================================ + +import path from 'node:path'; + +/** + * Destructive-command guard (AGT-3436). + * + * This was a regex sweep over the raw command text, and that shape was wrong in + * both directions: + * + * - It missed what bash does before running anything. Quote removal, backslash + * escapes, `$'...'` decoding and brace expansion all rewrite the command + * first, so `r"m" -rf /`, `\rm -rf /`, `$'\x72\x6d' -rf /`, `r{m,} -rf /` + * and `git clean -fdx` each execute a destructive command while containing + * no literal those patterns looked for. + * - It fired on text that is only data. `echo "rm -rf stays blocked"` and + * `# rm -rf /tmp/x` were refused, which is how a model learns to route + * around a guard rather than respect it. + * + * So the command is now resolved the way bash resolves it — quotes and escapes + * removed, `$'...'` decoded, comments dropped, braces expanded — and matched by + * WORD: the first word of a simple command is the program that runs, so a + * destructive verb is one only where a program name sits. Anything that cannot + * be resolved (an unclosed quote or substitution, a substitution spliced into a + * word, a brace expansion past its cap) is refused rather than guessed at: a + * false positive costs a retry, a false negative costs the working tree. + */ + +/** How far the guard follows `$(...)`, backticks and `sh -c` scripts before refusing. */ +const GUARD_MAX_DEPTH = 4; +/** Candidates `{a,b}` expansion may produce before the command is refused. */ +const GUARD_MAX_EXPANSIONS = 32; +/** Words scanned for a launcher's real command (`sudo -u root rm -rf /`). */ +const GUARD_MAX_WORDS = 64; + +/** One simple command, split out of a `;`/`&&`/`||`/`|`/newline chain. */ +interface ResolvedCommand { + /** Words after quote removal, backslash escapes and brace expansion. */ + words: string[]; + /** Targets of `>`/`<` redirections, kept apart from arguments. */ + redirects: string[]; + /** Bodies of `$(...)`/backtick substitutions — each runs a command of its own. */ + nested: string[]; +} + +/** Programs that only launch another command: the real one is in the arguments. */ +const COMMAND_LAUNCHERS: Record = { + sudo: true, doas: true, su: true, command: true, builtin: true, env: true, + nohup: true, nice: true, ionice: true, stdbuf: true, setsid: true, time: true, + timeout: true, watch: true, flock: true, chroot: true, exec: true, xargs: true, + find: true, +}; + +/** Programs whose `-c` argument is a script the shell runs. */ +const SCRIPT_HOSTS: Record = { sh: true, bash: true, zsh: true, dash: true, ksh: true, su: true }; + +function isWordChar(char: string | undefined): boolean { + return char !== undefined && /[A-Za-z0-9_]/.test(char); +} + +/** Innermost `{a,b}` group bash would expand, or null when the word has none. */ +function innermostBraceGroup(word: string): { start: number; end: number; alternatives: string[] } | null { + let start = word.lastIndexOf('{'); + while (start >= 0) { + const close = word.indexOf('}', start + 1); + if (close >= 0) { + const body = word.slice(start + 1, close); + // Not innermost (the inner group expands first) and no alternative list + // (`{x}` is literal to bash) both mean this brace is not a group. + if (!body.includes('{') && body.includes(',')) { + return { start, end: close + 1, alternatives: body.split(',') }; + } + } + // NB: `lastIndexOf('{', -1)` clamps to 0 and would rescan index 0 forever, + // so the walk stops explicitly rather than relying on a negative fromIndex. + if (start === 0) break; + start = word.lastIndexOf('{', start - 1); + } + return null; +} + +/** + * Every word `{a,b}` expansion can produce, or null when the count explodes. + * Each candidate is a word bash may run, so an unresolvable expansion is + * refused instead of being matched as its own literal text. + */ +function expandBraces(word: string): string[] | null { + let candidates = [word]; + for (;;) { + const next: string[] = []; + let expanded = false; + for (const candidate of candidates) { + const group = innermostBraceGroup(candidate); + if (!group) { + next.push(candidate); + continue; + } + expanded = true; + for (const alternative of group.alternatives) { + next.push(candidate.slice(0, group.start) + alternative + candidate.slice(group.end)); + } + } + if (!expanded) return next; + if (next.length > GUARD_MAX_EXPANSIONS) return null; + candidates = next; + } +} + +/** Index of the `'` closing the `$'...'` quote that starts at `start`, or -1. */ +function closingAnsiCQuote(command: string, start: number): number { + for (let i = start + 1; i < command.length; i++) { + if (command[i] === '\\') { i++; continue; } + if (command[i] === "'") return i; + } + return -1; +} + +/** + * What `$'...'` resolves to: bash decodes backslash escapes there, so + * `$'\x72\x6d' -rf /` runs `rm -rf /`. Decoding keeps the guard looking at the + * characters the process will actually see. + */ +function decodeAnsiCQuote(body: string): string { + return body.replace( + /\\(x[0-9a-fA-F]{1,2}|[0-7]{1,3}|u[0-9a-fA-F]{4}|U[0-9a-fA-F]{8}|[\s\S])/g, + (_all, escape: string) => { + const kind = escape[0]; + const code = kind === 'x' || kind === 'u' || kind === 'U' + ? parseInt(escape.slice(1), 16) + : kind >= '0' && kind <= '7' ? parseInt(escape, 8) : -1; + if (code >= 0 && code <= 0x10ffff) return String.fromCodePoint(code); + if (escape === 'n') return '\n'; + if (escape === 't') return '\t'; + if (escape === 'r') return '\r'; + return escape; + }, + ); +} + +/** Index just past the `$(...)`, `${...}` or `` `...` `` span at `start`, or -1 when it never closes. */ +function endOfSubstitution(command: string, start: number): number { + if (command[start] === '`') { + for (let i = start + 1; i < command.length; i++) { + if (command[i] === '\\') { i++; continue; } + if (command[i] === '`') return i + 1; + } + return -1; + } + const opens = command[start + 1]; + const closes = opens === '(' ? ')' : '}'; + let depth = 0; + let quote: '"' | "'" | null = null; + for (let i = start + 1; i < command.length; i++) { + const char = command[i]; + if (quote) { + if (char === '\\' && quote === '"') { i++; continue; } + if (char === quote) quote = null; + continue; + } + if (char === '\\') { i++; continue; } + if (char === '"' || char === "'") { quote = char; continue; } + if (char === opens) depth++; + else if (char === closes && --depth === 0) return i + 1; + } + return -1; +} + +interface SubstitutionSpan { + /** The span as written, which is all the guard can know about its output. */ + text: string; + /** The command inside `$(...)`/backticks, for recursive inspection. */ + body: string; + /** Index just past the span. */ + end: number; + runsCommand: boolean; +} + +/** + * The `$(...)`/`${...}`/`` `...` `` span at `i`: `'none'` when there is none, + * `'unclosed'` when it never terminates (the caller refuses). + */ +function substitutionSpanAt(command: string, i: number): SubstitutionSpan | 'none' | 'unclosed' { + const char = command[i]; + const dollar = char === '$' && (command[i + 1] === '(' || command[i + 1] === '{'); + if (char !== '`' && !dollar) return 'none'; + const end = endOfSubstitution(command, i); + if (end < 0) return 'unclosed'; + const runsCommand = char === '`' || command[i + 1] === '('; + const text = command.slice(i, end); + return { text, body: runsCommand ? text.slice(char === '`' ? 1 : 2, -1) : text, end, runsCommand }; +} + +/** + * The simple commands bash will run for `command`, each word resolved the way + * bash resolves it. Returns null when the text cannot be resolved — see the + * block comment above for why that is refused rather than matched. + */ +function resolveCommands(command: string): ResolvedCommand[] | null { + const commands: ResolvedCommand[] = []; + let words: string[] = []; + let redirects: string[] = []; + let nested: string[] = []; + let word = ''; + let wordStarted = false; + let atWordStart = true; + let redirectTarget = false; + let unresolvable = false; + let quote: '"' | "'" | null = null; + + const endWord = (): void => { + if (!wordStarted) return; + if (redirectTarget) redirects.push(word); + else { + const expanded = expandBraces(word); + if (expanded === null) unresolvable = true; + else words.push(...expanded); + } + word = ''; + wordStarted = false; + redirectTarget = false; + }; + const endCommand = (): void => { + endWord(); + if (words.length || redirects.length || nested.length) commands.push({ words, redirects, nested }); + words = []; + redirects = []; + nested = []; + atWordStart = true; + }; + + for (let i = 0; i < command.length; i++) { + const char = command[i]; + + if (quote === "'") { + if (char === "'") quote = null; + else word += char; + continue; + } + + if (quote === '"') { + if (char === '"') { quote = null; continue; } + if (char === '\\') { + const next = command[i + 1]; + if (next === undefined) return null; + if (next === '"' || next === '\\' || next === '$' || next === '`') { word += next; i++; } + else if (next === '\n') i++; + else word += char; + continue; + } + const span = substitutionSpanAt(command, i); + if (span === 'unclosed') return null; + if (span !== 'none') { + // `r"$(true)"m` is `rm` once the quotes come off: a splice is refused + // because the guard cannot know what the substitution yields. + if (isWordChar(word[word.length - 1]) || isWordChar(command[span.end])) return null; + word += span.text; + if (span.runsCommand) nested.push(span.body); + i = span.end - 1; + continue; + } + word += char; + continue; + } + + const ifs = /^\$\{IFS\}|\$IFS(?![A-Za-z0-9_])/.exec(command.slice(i, i + 6)); + if (ifs) { + // `rm$IFS-rf` is `rm -rf`: bash splits the word where IFS expands. + endWord(); + atWordStart = true; + i += ifs[0].length - 1; + continue; + } + if (char === '\\') { + const next = command[i + 1]; + if (next === undefined) return null; + if (next !== '\n') { word += next; wordStarted = true; atWordStart = false; } + i++; + continue; + } + if (char === "'" || char === '"') { quote = char; wordStarted = true; atWordStart = false; continue; } + if (char === '$' && command[i + 1] === "'") { + const close = closingAnsiCQuote(command, i + 1); + if (close < 0) return null; + word += decodeAnsiCQuote(command.slice(i + 2, close)); + wordStarted = true; + atWordStart = false; + i = close; + continue; + } + if (char === ' ' || char === '\t') { endWord(); atWordStart = true; continue; } + if (char === '\n' || char === ';' || char === '&' || char === '|') { + endCommand(); + if (char !== '\n' && command[i + 1] === char) i++; + continue; + } + if (char === '>' || char === '<') { + endWord(); + atWordStart = true; + redirectTarget = false; + if (command[i + 1] === char) { i++; redirectTarget = char === '>'; } // `>> device` + else if (command[i + 1] === '&') i++; // `>&2` duplicates a descriptor + else redirectTarget = char === '>'; + continue; + } + if (char === '#' && atWordStart) { + const newline = command.indexOf('\n', i); + endCommand(); + if (newline < 0) break; + i = newline; + continue; + } + const span = substitutionSpanAt(command, i); + if (span === 'unclosed') return null; + if (span !== 'none') { + if (isWordChar(word[word.length - 1]) || isWordChar(command[span.end])) return null; + word += span.text; + wordStarted = true; + atWordStart = false; + if (span.runsCommand) nested.push(span.body); + i = span.end - 1; + continue; + } + word += char; + wordStarted = true; + atWordStart = false; + } + + if (unresolvable) return null; + if (quote !== null) return null; // an unclosed quote swallows the rest of the line + endCommand(); + return commands; +} + +/** + * The destructive shapes, as predicates over (program, arguments). Kept apart + * from the resolution above so the two questions stay separable: what will run, + * and is that thing destructive. + */ +const DESTRUCTIVE_RULES: ReadonlyArray<(name: string, args: string[]) => boolean> = [ + // `rm -rf`, `rm -fr`, `rm -R`, `rm --recursive` — the recursive flag is the destructive part. + (name, args) => name === 'rm' && args.some((arg) => arg === '--recursive' || /^-[A-Za-z]*[rR][A-Za-z]*$/.test(arg)), + (name, args) => name === 'git' && args.includes('reset') && args.includes('--hard'), + // `git clean` deletes untracked files once force meets directories: `-fd`, `-fdx`, `-d -f`. + (name, args) => name === 'git' && args.includes('clean') + && args.some((arg) => arg === '--force' || /^-[A-Za-z]*f[A-Za-z]*$/.test(arg)) + && args.some((arg) => /^-[A-Za-z]*d[A-Za-z]*$/.test(arg)), + (name, args) => name === 'chmod' && args.some((arg) => /^[0-7]*777[0-7]*$/.test(arg)), + (name, args) => name === 'chown' + && args.some((arg) => arg === '--recursive' || /^-[A-Za-z]*R[A-Za-z]*$/.test(arg)), + (name, args) => name === 'dd' && args.some((arg) => arg.startsWith('if=')), + (name, args) => (name === 'kill' || name === 'pkill') + && args.some((arg) => /^-[A-Za-z]*9$/.test(arg) || /^-(SIG)?KILL$/i.test(arg)), +]; + +/** Is one resolved simple command destructive? */ +function resolvedCommandIsBlocked(command: ResolvedCommand, depth: number): boolean { + if (command.redirects.some((target) => target.startsWith('/dev/sd'))) return true; + // SQL verbs are destructive wherever they sit in the line: a client reads + // them as a statement, so position cannot separate `psql -c "drop database + // app"` from a word that merely mentions one. + const joined = command.words.join(' '); + if (/\bdrop\s+database\b/i.test(joined) || /\btruncate\s+table\b/i.test(joined)) return true; + + if (depth < GUARD_MAX_DEPTH) { + for (const script of command.nested) { + if (isCommandBlocked(script, depth + 1)) return true; + } + } + + let start = 0; + while (command.words[start] !== undefined && /^[A-Za-z_][A-Za-z0-9_]*=/.test(command.words[start])) start++; // `FOO=bar rm -rf /` + const name = path.basename(command.words[start] ?? ''); + const args = command.words.slice(start + 1); + + if (depth < GUARD_MAX_DEPTH) { + // `sh -c '