diff --git a/.agents/sessions/external-read-roots-2026-08/SPEC.md b/.agents/sessions/external-read-roots-2026-08/SPEC.md new file mode 100644 index 0000000000..8ed85103eb --- /dev/null +++ b/.agents/sessions/external-read-roots-2026-08/SPEC.md @@ -0,0 +1,151 @@ +# SPEC — Read-only external read roots + +## Overview + +Openbuff tools are contained to the project root, plus a narrow exception for the +openbuff-owned OS temp namespace (`owned-temp`). Users cannot read files outside +the project — including their own openbuff config directory (logs, harness +state) — even though those are the user's own files and reading them is a +legitimate, non-mutating action. + +This work adds a third containment scope, `external-read`: a **read-only**, +**default-closed**, **configure-once** allowlist of roots outside the project +that path-taking READ tools may reach. The write path is structurally excluded. + +Wave 1 (the `common` primitive) is COMPLETE, gate-approved, and committed. +Wave 2 (product wiring) is implemented in the working tree but NOT yet green. + +## Goals + +- A user can read files in their openbuff config dir (e.g. logs, harness state) + through `read_files` / `read_logs` / `read_image` / `list_directory`. +- A user can allowlist additional absolute roots via `openbuff.json` + → `readableRoots: string[]`. +- Mandatory-sensitive files stay blocked **inside** allowlisted roots + (`credentials.json`, `.env`, private keys, kubeconfig, tfstate, …). +- Writes can never reach an allowlisted root, enforced structurally rather than + by handler discipline. +- Default posture is closed: with nothing configured, behavior is byte-identical + to before this work. + +## Non-goals + +- `glob` stays project-only. It is a pattern-driven directory walk; widening it + would let a pattern enumerate an allowlisted root. Deliberately out of scope. +- No write, move, delete, or `cwd` access to external roots, ever. +- No relative entries in `readableRoots` (ambiguous in a global config file; + dropped rather than guessed). +- No mid-session reconfiguration. Changing `readableRoots` requires a restart. +- No UI/slash-command surface for managing roots in this iteration. + +## Requirements + +### R1 — Containment primitive (`common`) — DONE, committed + +- `ContainedProjectPath['scope']` is `'project' | 'owned-temp' | 'external-read'`. +- `configureExternalReadRoots(roots)` normalizes/dedupes/sorts; skips filesystem + roots and `..` entries; idempotent for an equivalent set; THROWS on a + differing set. +- `ensureExternalReadRootsConfigured(roots)` — non-throwing wrapper returning + `'configured' | 'unchanged' | 'refused-changed'`. +- `getExternalReadRoots()`, `resetExternalReadRootsForTesting()`, + `isExternalReadPath(input)`. +- `resolveProjectPathForRead` / `resolveProjectPathForFileSystemRead` — the ONLY + entry points that can produce `external-read`. They delegate to the existing + write resolvers first and only fall back to the external branch on `null`. +- `resolveProjectPath` / `resolveProjectPathForFileSystem` are UNCHANGED. +- The external resolver refuses mandatory-sensitive basenames on BOTH the + lexical and dereferenced path, so consumers inherit the refusal fail-closed. +- `credentials.json` / `.yaml` / `.yml` added to `SENSITIVE_BASENAMES`. + +### R2 — Config schema (`sdk/src/provider-config.ts`) — implemented, unverified + +- `readableRoots: z.array(z.string().min(1)).default([])`. +- `.default([])` is intentional: downstream code never handles `undefined`. + Consequence: `readableRoots: string[]` is REQUIRED in the schema OUTPUT type + and therefore in `LoadedProviderConfig['config']`. This is what breaks the + hand-built test fixtures (see PLAN task T1). + +### R3 — Read-only operation resolvers (`sdk/src/tools/path-utils.ts`) — implemented + +- `resolveFilePathForReadOperation`, + `resolveFilePathForFileSystemReadOperation`. +- Follow-symlink shape only; no `followFinalSymlink: false` (that option exists + for unlink-style mutations). +- The existing `resolveFilePathFor*Operation` write twins are unchanged. + +### R4 — Read handlers rewired — implemented + +`read-files.ts` (`authorizeReadTarget`), `read-logs.ts`, `read-image.ts`, +`list-directory.ts`. Plus: `read-files.ts` extends its owned-temp fileFilter +alias block to cover `external-read` (both scopes carry an ABSOLUTE +`relativePath`, so a host filter written against project-relative globs would +silently fail OPEN). Alias key: `external-read/`. + +### R5 — Run-start configuration (`sdk/src/run.ts`) — implemented + +In `runOnce`, before tool dispatch: `ensureExternalReadRootsConfigured([...])` +with the openbuff config dir (`getConfigDir(env)`) plus absolute-only entries +from `loadProviderConfigSync().config.readableRoots`. Wrapped in try/catch so a +malformed config cannot block a run. `'refused-changed'` logs a warn naming +counts, not paths (home-dir paths are mildly sensitive and logs get shared). + +### R6 — Agent-runtime read backstop (`tool-executor.ts`) — implemented + +The pre-dispatch scope check's `ownedTempRead` condition extended so an +allowlisted external READ is not hard-blocked. Writes there still hard-block. + +### R7 — Fixtures + validation — BLOCKED (this is the remaining work) + +Six typecheck failures, all "missing required `readableRoots`" in hand-built +`LoadedProviderConfig` fixtures. See PLAN task T1 for exact sites. + +### R8 — Security review — NOT STARTED + +`security-reviewer` on the permission-boundary widening. + +### R9 — Documentation — NOT STARTED + +`docs/configuration.md` + `openbuff.json.example`. + +## Acceptance criteria + +- AC1 — `bun run typecheck` exits 0 for all 11 workspace packages. +- AC2 — `bun test sdk/src/__tests__/` reports 0 fail AND 0 error. +- AC3 — `bun test common/src/util/__tests__/` and the agent-runtime tool tests + report 0 fail. +- AC4 — Test named for the write-path invariant still passes: `resolveProjectPath` + returns `null` for a path inside a configured allowlisted root. +- AC5 — `credentials.json` inside an allowlisted root is refused by BOTH + `isExternalReadPath` and the resolver. +- AC6 — With `readableRoots` unset and no config dir on the allowlist, external + paths are refused exactly as before (default-closed). +- AC7 — Full-directory SDK test run is order-independent: the read-logs external + suite passes both alone and after a suite that exercises the SDK run path. +- AC8 — `security-reviewer` returns no unresolved BLOCKING finding. +- AC9 — Docs state: reads only, absolute-only entries, sensitive files still + blocked, restart required to apply changes. + +## Relevant files + +Committed (wave 1): +- `common/src/util/project-path-containment.ts` +- `common/src/util/sensitive-paths.ts` +- `common/src/util/__tests__/{project-path-containment,sensitive-paths}.test.ts` +- `sdk/src/tools/filesystem-authority.ts` — `toAuthorizedPath` fails closed on + `external-read` with code `external_read_scope_unsupported` + +Dirty (wave 2, in working tree): +- `sdk/src/provider-config.ts`, `sdk/src/run.ts` +- `sdk/src/tools/{path-utils,read-files,read-logs,read-image,list-directory}.ts` +- `packages/agent-runtime/src/tools/tool-executor.ts` +- `sdk/src/__tests__/{path-utils,read-files,read-logs,model-provider}.test.ts` +- `packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts` +- `common/src/util/project-path-containment.ts` (+ its test) — `ensureExternalReadRootsConfigured` + +To touch in T1: +- `sdk/src/__tests__/model-provider.test.ts` +- `sdk/src/impl/__tests__/failover.test.ts` + +To touch in T4: +- `docs/configuration.md`, `openbuff.json.example` diff --git a/agents/base2/quality-prompt-section.ts b/agents/base2/quality-prompt-section.ts index 3303fa8889..fb95437487 100644 --- a/agents/base2/quality-prompt-section.ts +++ b/agents/base2/quality-prompt-section.ts @@ -74,7 +74,7 @@ Neither replaces the runtime hooks + automated code-reviewer path. ## Hard blocks while GATE: PENDING -- \`suggest_followups\` — rejected +- \`suggest_followups\` — rejected (end the turn instead; the rejection is agent-facing only and is not shown to the user) - \`git-committer\` — withheld until GATE: PASSED - Manual re-spawn of code-reviewer for the same pending set — do not; the automated gate owns that set. If phase is \`awaiting_validation\` / gate not yet passed, end the turn for the programmatic hooks→reviewer cycle. @@ -88,5 +88,5 @@ Dirty working-tree files are not the same as pending: only task-related **review - Write the final user-visible completion summary first - Spawn optional \`git-committer\` (with \`params.owned_paths\` for task-owned paths) before followups if committing this turn -- Call \`suggest_followups\` only as the absolute last tool after summary/commit; never mid-turn and never before remaining work +- Call \`suggest_followups\` only as the absolute last tool after summary/commit — it is the FINAL output of the turn, so emit nothing after it except \`end_turn\`/\`task_completed\`; never mid-turn and never before remaining work - The gate re-arms on every new edit (back to GATE: PENDING); one more clear cycle is required. Treat early withhold as normal ordering; do not tight-loop committer spawns — wait for GATE: PASSED, then spawn once.` diff --git a/cli/knowledge.md b/cli/knowledge.md index 299878a969..7c2dd6016b 100644 --- a/cli/knowledge.md +++ b/cli/knowledge.md @@ -907,3 +907,7 @@ Streaming markdown renders as plain text until the message or agent finishes. Th - _Knowledge refresh 2026-08-23: add `/memory` (alias `/mem`) slash command; staleness guard touch._ - _Knowledge refresh 2026-08-31: live compaction status rendering (`context_compaction_status` consumption, run-correlated pending/settled pairing, replayed-pending-as-interrupted) in `cli/src/utils/sdk-event-handlers.ts` and `cli/src/components/renderers/compaction-box.tsx`._ + +- _Knowledge refresh 2026-08-31 (followups): `handleRuntimeError` in `cli/src/utils/sdk-event-handlers.ts` now splits runtime error events by `autoRecovering` — auto-recovering notices log at `debug` (`'SDK auto-recovering runtime notice'`) with no visible error banner, while genuine failures still log at `error` (`'SDK runtime error event'`) and render. Tool-ordering rejections (the `suggest_followups` gate/ordering rejections and the pre-gate `git-committer withheld` chunk) arrive with a concise `userMessage` plus `autoRecovering: true`, so the model still receives the full `message` via the `TOOL_CALL_ERROR` path in `packages/agent-runtime/src/tools/stream-parser.ts`; `git-committer blocked by unvalidated dirty file(s)` deliberately stays user-visible because it asks the user to reply `COMMIT ANYWAY`._ + +- _Knowledge refresh 2026-08-31 (UI polish): `cli/src/components/status-bar.tsx` is now three regions — status label left, chip cluster left-aligned in the growing middle (`flexGrow: 1` + `flexBasis: 0`), and every width-varying control (scroll-to-bottom, then the `■ Esc` stop hint) in a `flexShrink: 0` right region with no `minWidth: 0`, so a hover cannot reflow the label or the chips. `cli/src/components/scroll-to-bottom-button.tsx` exports `SCROLL_HINT_LABEL`/`SCROLL_GLYPH` plus `string-width`-derived `SCROLL_BUTTON_WIDTH` (10) and `SCROLL_BUTTON_COMPACT_WIDTH` (3) and renders at a fixed width in both hover states; its `isScrollButtonCompact(width)` predicate is the single source `StatusBar` also passes as `scrollButtonCompact`, so `statusBarChipBudget`'s duplicated `SCROLL_BUTTON_RESERVATION`/`SCROLL_BUTTON_COMPACT_RESERVATION` in `cli/src/utils/status-bar-chips.ts` always reserve the columns actually rendered (test-enforced agreement, since the util must not import a component module). `cli/src/components/renderers/completion-summary-box.tsx` renders a titled `Run summary` `HarnessBox` (`gap={0}`, `paddingBottom={0}`) of aligned `Label value` rows built from `ROW_LABELS` + derived `LABEL_COLUMN_WIDTH`, with no status emoji — meaning lives in the value words, not color. Reconciler-level coverage for the status bar lives in `cli/src/components/__tests__/status-bar.test.tsx`, which reuses the dev-only `renderTest`/`renderFrame` convention from `text-nesting.test.tsx` (`@opentui/react/test-utils` cannot be imported under `NODE_ENV=production`)._ diff --git a/cli/src/components/__tests__/completion-summary-box.test.tsx b/cli/src/components/__tests__/completion-summary-box.test.tsx index 37b47584e5..29b3157510 100644 --- a/cli/src/components/__tests__/completion-summary-box.test.tsx +++ b/cli/src/components/__tests__/completion-summary-box.test.tsx @@ -13,6 +13,9 @@ initializeThemeStore() const theme = chatThemes.dark +/** Icons the tightened box must never render again. */ +const REMOVED_ICONS = ['✅', '❌', '⚠️', '⚠', '🔴', '🟡', '🟢'] + function makeSummary(overrides: Partial): CompletionSummary { return { filesEdited: 0, @@ -190,11 +193,13 @@ describe('CompletionSummaryBox deriveTone', () => { expect(markup).toContain('1 failed') }) - test('renders auxiliary section', () => { + test('renders auxiliary section under the Agents label', () => { const markup = renderSummary( makeSummary({ auxiliaryCompleted: 2, auxiliaryFailed: 1 }), ) - expect(markup).toContain('auxiliary') + expect(markup).toContain('Agents') + expect(markup).toContain('2 completed') + expect(markup).toContain('1 failed') }) test('renders errors section with error color', () => { @@ -203,21 +208,64 @@ describe('CompletionSummaryBox deriveTone', () => { expect(markup).toContain(theme.error) }) - test('renders BLOCKING with red icon', () => { + test('renders BLOCKING in the error tone, without an icon', () => { const markup = renderSummary(makeSummary({ reviewVerdict: 'BLOCKING' })) expect(markup).toContain('BLOCKING') - expect(markup).toContain('🔴') + expect(markup).toContain(theme.error) }) - test('renders NON_BLOCKING with yellow icon', () => { + test('renders NON_BLOCKING in the warning tone, without an icon', () => { const markup = renderSummary(makeSummary({ reviewVerdict: 'NON_BLOCKING' })) expect(markup).toContain('NON_BLOCKING') - expect(markup).toContain('🟡') + expect(markup).toContain(theme.warning) }) - test('renders LOOKS_GOOD with green icon', () => { + test('renders LOOKS_GOOD in the success tone, without an icon', () => { const markup = renderSummary(makeSummary({ reviewVerdict: 'LOOKS_GOOD' })) expect(markup).toContain('LOOKS_GOOD') - expect(markup).toContain('🟢') + expect(markup).toContain(theme.success) + }) + + test('titles the box and emits none of the removed status emoji', () => { + const markup = renderSummary( + makeSummary({ + filesEdited: 2, + filesRolledBack: 1, + hooksPassed: 1, + hooksSkipped: 2, + reviewVerdict: 'NON_BLOCKING', + testPassed: 5, + testFailed: 1, + auxiliaryCompleted: 3, + auxiliaryFailed: 1, + errors: 2, + }), + ) + + expect(markup).toContain('Run summary') + for (const icon of REMOVED_ICONS) { + expect(markup).not.toContain(icon) + } + // Meaning still survives without color: the state words are in the values. + expect(markup).toContain('rolled back') + expect(markup).toContain('2 skipped') + }) + + test('pads the label column so every row value starts at the same offset', () => { + const markup = renderSummary( + makeSummary({ filesEdited: 2, reviewVerdict: 'LOOKS_GOOD' }), + ) + // One row per ; strip the inline spans to get the rendered line. + const rows = [...markup.matchAll(/]*>(.*?)<\/text>/g)].map( + (match) => match[1].replace(/<[^>]*>/g, ''), + ) + const filesRow = rows.find((row) => row.startsWith('Files')) + const reviewRow = rows.find((row) => row.startsWith('Review')) + + expect(filesRow).toBeDefined() + expect(reviewRow).toBeDefined() + // 'Files' is a column shorter than 'Review', so equal value offsets can + // only come from the shared padded label column. + expect(filesRow?.indexOf('2 edited')).toBe(reviewRow?.indexOf('LOOKS_GOOD')) }) }) diff --git a/cli/src/components/__tests__/scroll-to-bottom-button.test.ts b/cli/src/components/__tests__/scroll-to-bottom-button.test.ts new file mode 100644 index 0000000000..964e76adf5 --- /dev/null +++ b/cli/src/components/__tests__/scroll-to-bottom-button.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, test } from 'bun:test' +import stringWidth from 'string-width' + +import { + SCROLL_BUTTON_COMPACT_RESERVATION, + SCROLL_BUTTON_RESERVATION, +} from '../../utils/status-bar-chips' +import { + SCROLL_BUTTON_COMPACT_WIDTH, + SCROLL_BUTTON_WIDTH, + SCROLL_GLYPH, + SCROLL_HINT_LABEL, +} from '../scroll-to-bottom-button' + +/** + * The button reserves a fixed width so hovering it cannot reflow the status + * bar. Asserted on the exported constants rather than through the reconciler: + * `@opentui/react/test-utils` is unavailable in this package's production-mode + * test run, so hover cannot be simulated here. + */ +describe('ScrollToBottomButton reserved width', () => { + test('the expanded width fits the hint label plus one column per side', () => { + expect(SCROLL_BUTTON_WIDTH).toBe(stringWidth(SCROLL_HINT_LABEL) + 2) + // Pinned: status-bar-chips.ts duplicates this as a literal reservation, so + // a change to the label must be a deliberate change to both. + expect(SCROLL_BUTTON_WIDTH).toBe(10) + }) + + test('the resting glyph occupies the same reserved width as the hover label', () => { + // Both expanded-mode states render inside the same fixed width, so the + // rendered button width is identical hovered and unhovered. + const restingWidth = stringWidth(SCROLL_GLYPH) + 2 + expect(restingWidth).toBeLessThanOrEqual(SCROLL_BUTTON_WIDTH) + expect(SCROLL_BUTTON_COMPACT_WIDTH).toBe(restingWidth) + // Same pinning as the expanded width, against the duplicated reservation. + expect(SCROLL_BUTTON_COMPACT_WIDTH).toBe(3) + }) + + test('the hint label and the compact form share the resting glyph', () => { + // The compact width is derived from SCROLL_GLYPH, so the glyph the button + // renders when resting is the one the width was measured from. + expect(SCROLL_HINT_LABEL.startsWith(SCROLL_GLYPH)).toBe(true) + // Non-ASCII, so rendered width rather than `.length` is what matters. + expect(stringWidth(SCROLL_GLYPH)).toBe(1) + }) + + test('the hint stays short enough to leave the chips room', () => { + // The old '↓ Scroll to bottom ↓' label was 20 columns; permanently + // reserving that much would crowd the chip cluster. + expect(SCROLL_BUTTON_WIDTH).toBeLessThan(20) + }) + + test('the chip budget reservation matches the button width', () => { + // status-bar-chips.ts duplicates the value rather than importing this + // component module, so the two must be asserted to agree. + expect(SCROLL_BUTTON_RESERVATION).toBe(SCROLL_BUTTON_WIDTH) + }) + + test('the compact chip budget reservation matches the compact button width', () => { + // Same duplication as above, for the narrow 'xs'/'sm' form. + expect(SCROLL_BUTTON_COMPACT_RESERVATION).toBe(SCROLL_BUTTON_COMPACT_WIDTH) + // Strictly cheaper than the expanded form, so reserving the expanded width + // at 'xs'/'sm' would cost the chips columns for nothing. + expect(SCROLL_BUTTON_COMPACT_RESERVATION).toBeLessThan( + SCROLL_BUTTON_RESERVATION, + ) + }) +}) diff --git a/cli/src/components/__tests__/status-bar.test.tsx b/cli/src/components/__tests__/status-bar.test.tsx new file mode 100644 index 0000000000..3979b8b5c3 --- /dev/null +++ b/cli/src/components/__tests__/status-bar.test.tsx @@ -0,0 +1,116 @@ +import { describe, expect, test } from 'bun:test' +import React from 'react' + +import { initializeThemeStore } from '../../hooks/use-theme' +import { SCROLL_GLYPH } from '../scroll-to-bottom-button' +import { StatusBar } from '../status-bar' + +import type { StatusIndicatorState } from '../../utils/status-indicator-state' + +initializeThemeStore() + +/** + * `@opentui/react/test-utils` imports `act` from react, which the production + * build does not export, so it cannot even be imported under NODE_ENV=production + * — which is how this package's `bun run test` script invokes bun test. Same + * convention as text-nesting.test.tsx. + */ +const renderTest = process.env.NODE_ENV === 'production' ? test.skip : test + +const renderFrame = async (node: React.ReactNode): Promise => { + const { testRender } = await import('@opentui/react/test-utils') + const setup = await testRender( + {node}, + { width: 100, height: 40 }, + ) + await setup.renderOnce() + const frame: string = setup.captureCharFrame() + setup.renderer.destroy() + return frame +} + +/** + * Asserts the renderer survived commit (see text-nesting.test.tsx: an OpenTUI + * nesting throw replaces the frame with the root error boundary's fallback) and + * that every expected substring made it into the frame. + */ +const expectRendered = (frame: string, expectedContent: string[]) => { + expect(frame).not.toContain('TextNodeRenderable') + for (const content of expectedContent) expect(frame).toContain(content) +} + +/** The status bar is a single row, so ordering is compared within one line. */ +const rowContaining = (frame: string, needle: string): string => { + const row = frame.split('\n').find((line) => line.includes(needle)) + expect(row).toBeDefined() + return row ?? '' +} + +const STREAMING: StatusIndicatorState = { + kind: 'streaming', + phaseLabel: 'working...', +} + +/** + * The context chip's percent survives every width the harness might report: + * 'ctx 48%', ' 48%' and the bare '48%' overflow fallback all contain it, + * and it is the highest-priority chip, so it is the one chip label that is safe + * to assert without controlling the rendered width. + */ +const CONTEXT_PERCENT = '48%' + +/** + * `timerStartTime` is null on purpose: a live elapsed timer would add a chip + * whose label changes with wall-clock time, and the timer itself is covered by + * utils/__tests__/status-bar-chips.test.ts. + */ +const baseProps = { + timerStartTime: null, + scrollToLatest: () => {}, + statusIndicatorState: STREAMING, + contextWindowUsage: { used: 48_000, max: 100_000 }, + modelName: 'anthropic/claude-sonnet', + diffStats: { modified: 2, added: 1, deleted: 0 }, +} + +describe('StatusBar through the real OpenTUI reconciler', () => { + renderTest( + 'renders the status label, the chip cluster and the scroll control together', + async () => { + const frame = await renderFrame( + , + ) + + expectRendered(frame, ['working...', CONTEXT_PERCENT, SCROLL_GLYPH]) + }, + ) + + renderTest( + 'renders the stop hint to the right of the chips while a run is active', + async () => { + const frame = await renderFrame( + {}} />, + ) + + expectRendered(frame, [CONTEXT_PERCENT, SCROLL_GLYPH, '■ Esc']) + + // Both controls live in the right-hand region, so they share the chips' + // row; compare offsets inside that one row rather than across the + // newline-joined frame. + const row = rowContaining(frame, '■ Esc') + expect(row).toContain(CONTEXT_PERCENT) + expect(row.indexOf(CONTEXT_PERCENT)).toBeLessThan(row.indexOf('■ Esc')) + expect(row.indexOf(SCROLL_GLYPH)).toBeLessThan(row.indexOf('■ Esc')) + }, + ) + + renderTest( + 'hides the scroll control at the bottom, keeping the chips', + async () => { + const frame = await renderFrame() + + expectRendered(frame, ['working...', CONTEXT_PERCENT]) + expect(frame).not.toContain(SCROLL_GLYPH) + }, + ) +}) diff --git a/cli/src/components/__tests__/text-nesting.test.tsx b/cli/src/components/__tests__/text-nesting.test.tsx index 0ed0ef1a6c..5e29e973e1 100644 --- a/cli/src/components/__tests__/text-nesting.test.tsx +++ b/cli/src/components/__tests__/text-nesting.test.tsx @@ -198,12 +198,18 @@ describe('harness box renderers survive the real OpenTUI reconciler', () => { ) expectRendered(frame, [ - '3 files edited', - 'Hooks: 1 passed', - 'Reviewed:', + 'Run summary', + 'Files', + '3 edited', + 'Hooks', + '1 passed', + 'Review', 'LOOKS_GOOD', - 'Tests: 2 passed', - 'auxiliary agent', + 'Tests', + '2 passed', + 'Agents', + '1 completed', + 'Errors', '1 error', ]) }) diff --git a/cli/src/components/renderers/completion-summary-box.tsx b/cli/src/components/renderers/completion-summary-box.tsx index 1112e2bc1e..feeeb18792 100644 --- a/cli/src/components/renderers/completion-summary-box.tsx +++ b/cli/src/components/renderers/completion-summary-box.tsx @@ -7,7 +7,30 @@ import type { CompletionSummaryContentBlock } from '../../types/chat' import type { ChatTheme } from '../../types/theme-system' import type { CompletionSummary } from '../../utils/completion-summary' -type Tone = 'secondary' | 'success' | 'error' | 'warning' | 'info' +type Tone = 'secondary' | 'success' | 'error' | 'warning' + +/** + * Row labels. The label column carries the noun, so the row values are bare + * descriptors ('2 edited') and no icon is needed to identify the row. + */ +const ROW_LABELS = { + files: 'Files', + hooks: 'Hooks', + review: 'Review', + tests: 'Tests', + agents: 'Agents', + errors: 'Errors', +} as const + +/** + * Shared label column: the longest label plus a two-column gutter, so every + * value starts at the same offset. Derived rather than hardcoded so a longer + * label added later cannot silently misalign the column. `.length` is correct + * only because these labels are plain ASCII — a label with wide or combining + * characters would need rendered-width math instead. + */ +const LABEL_COLUMN_WIDTH = + Math.max(...Object.values(ROW_LABELS).map((label) => label.length)) + 2 const deriveTone = (summary: CompletionSummary): Tone => { const isBlockingVerdict = @@ -54,18 +77,32 @@ const statusColorForTone = (tone: Tone, theme: ChatTheme): string => { return theme.error case 'warning': return theme.warning - case 'info': - return theme.info default: return theme.secondary } } -const reviewIcon = (verdict: string | null): string => { - if (verdict === 'BLOCKING' || verdict === 'NEEDS_WORK') return '🔴' - if (verdict === 'NON_BLOCKING') return '🟡' - if (verdict === 'LOOKS_GOOD' || verdict === 'APPROVED') return '🟢' - return '🟢' +interface SummaryRowProps { + label: string + value: string + tone: Tone +} + +/** + * One compact row: a muted, padded label column followed by the toned value. + * Exactly two inline spans inside a single `` — OpenTUI rejects nested + * block elements inside text. + */ +const SummaryRow = ({ label, value, tone }: SummaryRowProps) => { + const theme = useTheme() + return ( + + + {label.padEnd(LABEL_COLUMN_WIDTH)} + + {value} + + ) } interface CompletionSummaryBoxProps { @@ -74,7 +111,6 @@ interface CompletionSummaryBoxProps { export const CompletionSummaryBox = memo( ({ block }: CompletionSummaryBoxProps) => { - const theme = useTheme() const summary = block.summary const tone = deriveTone(summary) @@ -103,12 +139,11 @@ export const CompletionSummaryBox = memo( const testsTone: Tone = summary.testFailed > 0 ? 'error' : 'success' const auxTone: Tone = summary.auxiliaryFailed > 0 ? 'error' : 'success' - const filesText = (() => { + // Row values are built inside each guarded branch below, so a hidden row's + // string is never assembled. + const buildFilesText = (): string => { const parts: string[] = [] - if (summary.filesEdited > 0) - parts.push( - `${summary.filesEdited} file${summary.filesEdited !== 1 ? 's' : ''} edited`, - ) + if (summary.filesEdited > 0) parts.push(`${summary.filesEdited} edited`) if (summary.filesFailed > 0) parts.push(`${summary.filesFailed} failed`) if (summary.filesUnconfirmed > 0) parts.push(`${summary.filesUnconfirmed} unconfirmed`) @@ -117,26 +152,25 @@ export const CompletionSummaryBox = memo( if (summary.rollbackIncomplete > 0) parts.push(`${summary.rollbackIncomplete} rollback incomplete`) return parts.join(', ') - })() - const hooksText = (() => { + } + const buildHooksText = (): string => { const parts: string[] = [] if (summary.hooksPassed > 0) parts.push(`${summary.hooksPassed} passed`) if (summary.hooksFailed > 0) parts.push(`${summary.hooksFailed} failed`) if (summary.hooksSkipped > 0) parts.push(`${summary.hooksSkipped} skipped`) - return `Hooks: ${parts.join(', ')}` - })() - const testsText = (() => { - let part = 'Tests: ' - if (summary.testPassed > 0) part += `${summary.testPassed} passed` - if (summary.testFailed > 0) { - if (summary.testPassed > 0) part += ', ' - part += `${summary.testFailed} failed` - } - return part - })() - const auxText = `${summary.auxiliaryCompleted} auxiliary agent${summary.auxiliaryCompleted === 1 ? '' : 's'} completed${summary.auxiliaryFailed > 0 ? `, ${summary.auxiliaryFailed} failed` : ''}` - const errorsText = `${summary.errors} error${summary.errors !== 1 ? 's' : ''}` + return parts.join(', ') + } + const buildTestsText = (): string => { + const parts: string[] = [] + if (summary.testPassed > 0) parts.push(`${summary.testPassed} passed`) + if (summary.testFailed > 0) parts.push(`${summary.testFailed} failed`) + return parts.join(', ') + } + const buildAuxText = (): string => + `${summary.auxiliaryCompleted} completed${summary.auxiliaryFailed > 0 ? `, ${summary.auxiliaryFailed} failed` : ''}` + const buildErrorsText = (): string => + `${summary.errors} error${summary.errors !== 1 ? 's' : ''}` const reviewTone: Tone = summary.reviewVerdict === 'BLOCKING' || summary.reviewVerdict === 'NEEDS_WORK' @@ -146,59 +180,48 @@ export const CompletionSummaryBox = memo( : 'success' return ( - + {hasFiles ? ( - - - {filesTone === 'error' - ? '❌' - : filesTone === 'warning' - ? '⚠️' - : '✅'} - - {` ${filesText}`} - + ) : null} {hasHooks ? ( - - - {hooksTone === 'error' ? '❌' : '✅'} - - {` ${hooksText}`} - + ) : null} {summary.reviewVerdict ? ( - - Reviewed: - {` ${reviewIcon(summary.reviewVerdict)}`} - {` ${summary.reviewVerdict}`} - + ) : null} {hasTests ? ( - - - {testsTone === 'error' ? '❌' : '✅'} - - {` ${testsText}`} - + ) : null} {hasAux ? ( - - - {auxTone === 'error' ? '⚠️' : '✅'} - - {` ${auxText}`} - + ) : null} {summary.errors > 0 ? ( - - ❌ - {` ${errorsText}`} - + ) : null} ) diff --git a/cli/src/components/scroll-to-bottom-button.tsx b/cli/src/components/scroll-to-bottom-button.tsx index 9c3732b165..59bb484df0 100644 --- a/cli/src/components/scroll-to-bottom-button.tsx +++ b/cli/src/components/scroll-to-bottom-button.tsx @@ -1,30 +1,56 @@ import { TextAttributes } from '@opentui/core' import { useState } from 'react' +import stringWidth from 'string-width' import { Button } from './button' import { useTerminalLayout } from '../hooks/use-terminal-layout' import { useTheme } from '../hooks/use-theme' +import type { WidthLayoutHelper } from '../hooks/use-terminal-layout' + +/** Expanded (hovered) label. Short on purpose: the button reserves its width + * permanently, so a longer hint would crowd the status-bar chips. */ +export const SCROLL_HINT_LABEL = '↓ Bottom' + +/** + * Fixed width of the expanded button: the hint label plus one column of padding + * per side. Reserved in both states so hovering never reflows the status row. + * Rendered width rather than `.length`, because the label leads with a + * non-ASCII glyph. + */ +export const SCROLL_BUTTON_WIDTH = stringWidth(SCROLL_HINT_LABEL) + 2 + +/** Resting/compact glyph. */ +export const SCROLL_GLYPH = '↓' + +/** Fixed width of the compact (narrow-terminal) form: glyph plus padding. */ +export const SCROLL_BUTTON_COMPACT_WIDTH = stringWidth(SCROLL_GLYPH) + 2 + +/** Single source for which form the button renders; StatusBar uses the same + * predicate to size the chip-budget reservation, so the two cannot drift. */ +export const isScrollButtonCompact = (width: WidthLayoutHelper): boolean => + width.atMost('sm') + interface ScrollToBottomButtonProps { onClick: () => void - /** Keep the glyph-only label even on hover (narrow terminals). */ - compact?: boolean } export const ScrollToBottomButton = ({ onClick, - compact, }: ScrollToBottomButtonProps) => { const theme = useTheme() const { width } = useTerminalLayout() const [hovered, setHovered] = useState(false) - const isCompact = compact ?? width.atMost('sm') + const isCompact = isScrollButtonCompact(width) return ( diff --git a/cli/src/components/status-bar.tsx b/cli/src/components/status-bar.tsx index a445925929..5a02f318ca 100644 --- a/cli/src/components/status-bar.tsx +++ b/cli/src/components/status-bar.tsx @@ -2,7 +2,10 @@ import { TextAttributes } from '@opentui/core' import React, { useEffect, useState } from 'react' import { Button } from './button' -import { ScrollToBottomButton } from './scroll-to-bottom-button' +import { + isScrollButtonCompact, + ScrollToBottomButton, +} from './scroll-to-bottom-button' import { ShimmerText } from './shimmer-text' import { useTerminalLayout } from '../hooks/use-terminal-layout' @@ -153,6 +156,10 @@ export const StatusBar = ({ elapsedSeconds, showTimer: shouldShowTimer, showStop, + showScrollButton: !isAtBottom, + // ScrollToBottomButton renders its compact form on the same shared + // predicate, so the reserved columns match the width it actually renders. + scrollButtonCompact: isScrollButtonCompact(width), isActive, }) @@ -233,32 +240,28 @@ export const StatusBar = ({ backgroundColor: hasContent ? theme.surface : 'transparent', }} > + {/* Left: the working label, sized to its content. */} {statusIndicatorContent} + {/* Middle: the chip cluster, left-aligned inside the growing region so it + fills the empty middle instead of packing against the right edge. */} - {!isAtBottom && ( - - - - )} {chips.map((chip, index) => ( @@ -274,6 +277,20 @@ export const StatusBar = ({ ))} + + + {/* Right: every width-varying control lives here so a hover cannot reflow + the label or the chips. No minWidth: 0, which would collapse the + scroll button's reserved width. */} + + {!isAtBottom && } {showStop && onStop && ( ■ Esc )} diff --git a/cli/src/utils/__tests__/sdk-event-handlers.test.ts b/cli/src/utils/__tests__/sdk-event-handlers.test.ts index 91e12f8428..dc3e591985 100644 --- a/cli/src/utils/__tests__/sdk-event-handlers.test.ts +++ b/cli/src/utils/__tests__/sdk-event-handlers.test.ts @@ -35,6 +35,8 @@ const createTestContext = () => { }, ) + const loggerCalls: { level: string; message: unknown }[] = [] + const ctx: EventHandlerState = { streaming: { streamRefs: { @@ -80,10 +82,14 @@ const createTestContext = () => { setHasReceivedPlanResponse: () => {}, }, logger: { - info: () => {}, - warn: () => {}, - error: () => {}, - debug: () => {}, + info: (_obj: unknown, message?: unknown) => + loggerCalls.push({ level: 'info', message }), + warn: (_obj: unknown, message?: unknown) => + loggerCalls.push({ level: 'warn', message }), + error: (_obj: unknown, message?: unknown) => + loggerCalls.push({ level: 'error', message }), + debug: (_obj: unknown, message?: unknown) => + loggerCalls.push({ level: 'debug', message }), } as Logger, setIsRetrying: () => {}, } @@ -91,6 +97,7 @@ const createTestContext = () => { return { ctx, getMessages: () => messages, + getLoggerCalls: () => loggerCalls, } } @@ -187,6 +194,51 @@ describe('sdk-event-handlers', () => { expect(getMessages()[0].userError).toBeUndefined() }) + test('does not render an error banner for suggest_followups ordering rejections', () => { + const { ctx, getMessages } = createTestContext() + createEventHandler(ctx)({ + type: 'error', + message: + 'Tool `suggest_followups` is not available yet. GATE: PENDING (or final summary not written).', + userMessage: + 'The model called suggest_followups out of order and is correcting the ordering automatically. No action is needed.', + autoRecovering: true, + }) + expect(getMessages()[0].userError).toBeUndefined() + }) + + test('logs auto-recovering runtime errors at debug rather than error', () => { + const { ctx, getLoggerCalls } = createTestContext() + createEventHandler(ctx)({ + type: 'error', + message: + 'Tool `suggest_followups` is not available yet. GATE: PENDING (or final summary not written).', + userMessage: + 'The model called suggest_followups out of order and is correcting the ordering automatically. No action is needed.', + autoRecovering: true, + }) + const calls = getLoggerCalls() + expect(calls).toContainEqual({ + level: 'debug', + message: 'SDK auto-recovering runtime notice', + }) + expect(calls.some((call) => call.level === 'error')).toBe(false) + }) + + test('logs genuine runtime errors at error level', () => { + const { ctx, getLoggerCalls } = createTestContext() + createEventHandler(ctx)({ + type: 'error', + message: 'Provider failed\n at secret/path.ts:1:2', + }) + const calls = getLoggerCalls() + expect(calls).toContainEqual({ + level: 'error', + message: 'SDK runtime error event', + }) + expect(calls.some((call) => call.level === 'debug')).toBe(false) + }) + test('background agent cards remain running until polling reports settlement', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) @@ -796,13 +848,13 @@ describe('sdk-event-handlers', () => { test('tool_start flips a queued custom/unknown-path tool block back to running', () => { const { ctx, getMessages } = createTestContext() const handleEvent = createEventHandler(ctx) - // Pins RF-1: the `queued === true` branch in `executeCustomToolCall` that - // emits `tool_start` for a custom/MCP tool is genuinely reachable, not dead - // defensive code. The CLI handler treats any queued tool_call identically - // regardless of whether it was produced by the native (`executeToolCall`) or - // custom (`executeCustomToolCall`) path, so a custom/unknown-path tool name - // that lands queued must flip from 'queued' to 'running' on tool_start - // exactly like a native write_file. + // CLI-side coverage only: the queued→running flip is tool-name agnostic, so + // a custom/MCP tool name that lands queued must flip from 'queued' to + // 'running' on tool_start exactly like a native write_file. This does NOT + // exercise the runtime `queued === true` branch in `executeCustomToolCall`; + // that branch's reachability is pinned at the runtime level by 'emits + // tool_start for a custom/MCP tool queued behind an in-flight write (RF-1)' + // in packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts. dispatchValidEvent(handleEvent, { type: 'tool_call', toolCallId: 'custom-write-queued', diff --git a/cli/src/utils/__tests__/status-bar-chips.test.ts b/cli/src/utils/__tests__/status-bar-chips.test.ts index 21cd3d0664..dd8dc852ce 100644 --- a/cli/src/utils/__tests__/status-bar-chips.test.ts +++ b/cli/src/utils/__tests__/status-bar-chips.test.ts @@ -3,6 +3,8 @@ import stringWidth from 'string-width' import { formatStatusTokenCount, + SCROLL_BUTTON_COMPACT_RESERVATION, + SCROLL_BUTTON_RESERVATION, selectStatusBarChips, shortenStatusModelName, statusBarChipBudget, @@ -1052,6 +1054,59 @@ describe('selectStatusBarChips', () => { } }) + test('a visible scroll button tightens the chip budget', () => { + const withScroll = selectStatusBarChips({ + ...full, + widthSize: 'lg', + terminalWidth: 60, + showScrollButton: true, + }) + expect(statusBarClusterWidth(withScroll.chips)).toBeLessThanOrEqual( + statusBarChipBudget(60, full.showStop, true), + ) + + // Omitting the flag keeps the existing selection, so the two-argument + // budget path and the current call sites are unchanged. + expect( + selectStatusBarChips({ ...full, widthSize: 'lg', terminalWidth: 60 }) + .chips, + ).toEqual( + selectStatusBarChips({ + ...full, + widthSize: 'lg', + terminalWidth: 60, + showScrollButton: false, + }).chips, + ) + }) + + test('the compact scroll button form leaves the chips more room', () => { + const idsFor = (scrollButtonCompact: boolean) => + selectStatusBarChips({ + ...full, + widthSize: 'sm', + terminalWidth: 80, + showScrollButton: true, + scrollButtonCompact, + }).chips.map((chip) => chip.id) + + // At 'xs'/'sm' the button renders three columns, so the seven columns the + // expanded reservation would have taken keep the git chip instead. + expect(idsFor(true)).toEqual(['context', 'git', 'timer']) + expect(idsFor(false)).toEqual(['context', 'timer']) + + // Omitting the flag keeps the expanded reservation, so existing call sites + // are unaffected. + expect( + selectStatusBarChips({ + ...full, + widthSize: 'sm', + terminalWidth: 80, + showScrollButton: true, + }).chips.map((chip) => chip.id), + ).toEqual(idsFor(false)) + }) + test('never exits overflow handling with an over-budget cluster', () => { for (const terminalWidth of [1, 8, 12, 20, 39, 60]) { for (const showStop of [true, false]) { @@ -1101,6 +1156,66 @@ describe('statusBarChipBudget', () => { expect(statusBarChipBudget(1, true)).toBe(0) expect(statusBarChipBudget(1, false)).toBe(1) }) + + test('reserves the scroll button only when it is shown', () => { + // 0.4 * 200 = 80 columns, so neither budget hits the MIN_WIDTH_BUDGET floor + // and the difference is exactly the new reservation. + for (const showStop of [true, false]) { + const withoutScroll = statusBarChipBudget(200, showStop) + const withScroll = statusBarChipBudget(200, showStop, true) + + expect(withScroll).toBe(withoutScroll - SCROLL_BUTTON_RESERVATION) + expect(withScroll).toBeGreaterThan(0) + } + }) + + test('reserves the compact width when the narrow button form is rendered', () => { + // Strictly cheaper than the expanded reservation, so the narrow form can + // never reserve more columns than it renders. + expect(SCROLL_BUTTON_COMPACT_RESERVATION).toBe(3) + expect(SCROLL_BUTTON_COMPACT_RESERVATION).toBeLessThan( + SCROLL_BUTTON_RESERVATION, + ) + + // 0.4 * 200 = 80 columns, so no budget here hits the MIN_WIDTH_BUDGET floor + // and each difference is exactly the reservation under test. + for (const showStop of [true, false]) { + const withoutScroll = statusBarChipBudget(200, showStop) + const compact = statusBarChipBudget(200, showStop, true, true) + const expanded = statusBarChipBudget(200, showStop, true) + + expect(compact).toBe(withoutScroll - 3) + expect(compact).toBe(withoutScroll - SCROLL_BUTTON_COMPACT_RESERVATION) + expect(compact).toBeGreaterThan(expanded) + expect(compact - expanded).toBe( + SCROLL_BUTTON_RESERVATION - SCROLL_BUTTON_COMPACT_RESERVATION, + ) + } + }) + + test('the compact flag is ignored while the scroll button is hidden', () => { + for (const terminalWidth of [1, 8, 12, 20, 60, 200]) { + for (const showStop of [true, false]) { + expect(statusBarChipBudget(terminalWidth, showStop, false, true)).toBe( + statusBarChipBudget(terminalWidth, showStop), + ) + } + } + }) + + test('the two-argument form keeps its previous budgets', () => { + // The third parameter is optional and defaults to false, so existing call + // sites must be unaffected. + for (const terminalWidth of [1, 8, 12, 20, 60, 200]) { + for (const showStop of [true, false]) { + expect(statusBarChipBudget(terminalWidth, showStop)).toBe( + statusBarChipBudget(terminalWidth, showStop, false), + ) + } + } + expect(statusBarChipBudget(60, false)).toBe(24) + expect(statusBarChipBudget(60, true)).toBe(17) + }) }) describe('formatStatusTokenCount', () => { diff --git a/cli/src/utils/sdk-event-handlers.ts b/cli/src/utils/sdk-event-handlers.ts index d8803d648f..bbd87c067c 100644 --- a/cli/src/utils/sdk-event-handlers.ts +++ b/cli/src/utils/sdk-event-handlers.ts @@ -1609,13 +1609,16 @@ const handleRuntimeError = ( state: EventHandlerState, event: Extract, ) => { - state.logger.error({ event }, 'SDK runtime error event') // Auto-recoverable model errors (e.g. a malformed tool call the model is - // already correcting) are agent-facing diagnostics, not user-facing errors: - // skip the visible error banner entirely. + // already correcting, or a runtime-enforced tool-ordering rejection) are + // agent-facing control-flow diagnostics, not user-facing errors: skip the + // visible error banner entirely, and log at debug rather than error so the + // log level matches their non-failure nature. if (event.autoRecovering === true) { + state.logger.debug({ event }, 'SDK auto-recovering runtime notice') return } + state.logger.error({ event }, 'SDK runtime error event') const concise = event.userMessage?.trim() if (concise) { state.message.updater.setError(concise) diff --git a/cli/src/utils/status-bar-chips.ts b/cli/src/utils/status-bar-chips.ts index af6ae03845..57cf143bd6 100644 --- a/cli/src/utils/status-bar-chips.ts +++ b/cli/src/utils/status-bar-chips.ts @@ -35,6 +35,13 @@ export type SelectStatusBarChipsInput = { * '!', so the label should lead with its subject (e.g. 'idx failed: …'). */ indexChip?: { label: string; tone: 'secondary' | 'warning' | 'error' } | null + /** Whether the scroll-to-bottom button shares the row with the chips. */ + showScrollButton?: boolean + /** + * Whether that button renders its compact (glyph-only) form, which is three + * columns instead of ten. Only consulted while `showScrollButton` is set. + */ + scrollButtonCompact?: boolean /** * Accumulated context-compaction notice for the current turn, or null when * nothing has been compacted. `degraded` marks a compaction that did not fit @@ -60,6 +67,21 @@ const WIDTH_BUDGET_RATIO = 0.4 const MIN_WIDTH_BUDGET = 8 /** Columns reserved for the stop-button hint rendered beside the chips. */ export const STOP_BUTTON_WIDTH = 7 +/** + * Columns reserved for the scroll-to-bottom button rendered beside the chips. + * `SCROLL_BUTTON_WIDTH` in components/scroll-to-bottom-button.tsx is the source + * of truth ('↓ Bottom' plus one column of padding per side); duplicated here so + * this util does not import a component module. + */ +export const SCROLL_BUTTON_RESERVATION = 10 +/** + * Columns reserved for the compact (narrow-terminal) scroll-to-bottom button. + * `SCROLL_BUTTON_COMPACT_WIDTH` in components/scroll-to-bottom-button.tsx is the + * source of truth (the glyph plus one column of padding per side); duplicated + * here for the same reason as the expanded reservation, so reserving the wider + * form at 'xs'/'sm' cannot silently cost the chips seven columns. + */ +export const SCROLL_BUTTON_COMPACT_RESERVATION = 3 /** Columns rendered between two adjacent chips. */ const CHIP_SEPARATOR_WIDTH = 3 /** Suffix appended to a truncated status chip label. */ @@ -278,22 +300,34 @@ export function statusBarClusterWidth(chips: StatusBarChip[]): number { return chips.reduce((sum, chip) => sum + stringWidth(chip.label), separators) } -/** Columns available to the chip cluster for a given terminal width. */ +/** + * Columns available to the chip cluster for a given terminal width. + * `scrollButtonCompact` selects the narrow three-column reservation, matching + * the form the button actually renders at 'xs'/'sm'. + */ export function statusBarChipBudget( terminalWidth: number, showStop: boolean, + showScrollButton = false, + scrollButtonCompact = false, ): number { - const stopReservation = showStop ? STOP_BUTTON_WIDTH : 0 + const scrollReservation = !showScrollButton + ? 0 + : scrollButtonCompact + ? SCROLL_BUTTON_COMPACT_RESERVATION + : SCROLL_BUTTON_RESERVATION + const reservedColumns = (showStop ? STOP_BUTTON_WIDTH : 0) + scrollReservation const available = - Math.floor(terminalWidth * WIDTH_BUDGET_RATIO) - stopReservation - // The floor applies after the stop-hint reservation so one chip still fits in - // a narrow terminal, then the result is clamped to the columns actually left - // beside the stop hint so the cluster can never overflow the real row width. + Math.floor(terminalWidth * WIDTH_BUDGET_RATIO) - reservedColumns + // The floor applies after the stop-hint and scroll-button reservations so one + // chip still fits in a narrow terminal, then the result is clamped to the + // columns actually left beside those two controls so the cluster can never + // overflow the real row width. return Math.max( 0, Math.min( Math.max(MIN_WIDTH_BUDGET, available), - terminalWidth - stopReservation, + terminalWidth - reservedColumns, ), ) } @@ -320,6 +354,8 @@ export function selectStatusBarChips(input: SelectStatusBarChipsInput): { elapsedSeconds, showTimer, showStop, + showScrollButton, + scrollButtonCompact, isActive, } = input @@ -420,7 +456,12 @@ export function selectStatusBarChips(input: SelectStatusBarChipsInput): { }) } - const budget = statusBarChipBudget(terminalWidth, showStop) + const budget = statusBarChipBudget( + terminalWidth, + showStop, + showScrollButton, + scrollButtonCompact, + ) while (statusBarClusterWidth(chips) > budget) { if (removeChip(chips, 'cost')) continue diff --git a/common/knowledge.md b/common/knowledge.md index f9a2963dce..8185805bec 100644 --- a/common/knowledge.md +++ b/common/knowledge.md @@ -43,7 +43,8 @@ This package contains code shared across the Openbuff monorepo, especially the l - **Git discipline prompt (`common/src/constants/git-discipline.ts`)**: `gitCommitGuidePrompt` is the shared commit-workflow contract. It requires one-line `git commit` invocations using repeated `-m` flags for subject and body, forbids HEREDOC, command substitution, pipes, redirects, and raw newlines in commit messages, and forbids amend without explicit authorization as well as AI-attribution footers. - **Occurrence-scoped ranges (`common/src/tools/params/tool/replace-range.ts`)**: `replace_range` accepts `occurrence: { match, occurrence? }` as a mutually exclusive alternative to explicit `startLine`/`endLine`. Resolution happens only in the agent-runtime handler and is confined to the authorized capability window, so the schema never carries derived absolute bounds. - **Transaction bounds (`common/src/tools/params/tool/edit-transaction.ts`)**: the edit-count, unique-path, and aggregate-byte limits all live in one `superRefine` on `boundedTransactionEditListSchema`, which runs only after a successful array parse. This keeps a stringified/truncated `edits` payload reporting an array-shape `invalid_type` instead of a character-length "too many edits" diagnostic, while still emitting the `too_small`/`too_big` codes that chained `.min()`/`.max()` produced. Byte totals are summed per string field rather than by serializing the payload. -- **Path scope (`common/src/util/project-path-containment.ts`)**: `ContainedProjectPath` carries `scope: 'project' | 'owned-temp'`. Openbuff-owned OS temp paths (`openbuff-.log|.json`, extension-free `openbuff-` directories, `tmux-captures-`) resolve successfully but return an ABSOLUTE `relativePath`, so consumers must branch on `scope` rather than inferring from absoluteness. Raw `..` segments are refused for that exception at every entry point, and an owned-named symlink whose realpath escapes the temp roots is rejected. +- **Path scope (`common/src/util/project-path-containment.ts`)**: `ContainedProjectPath` carries `scope: 'project' | 'owned-temp' | 'external-read'`. Openbuff-owned OS temp paths (`openbuff-.log|.json`, extension-free `openbuff-` directories, `tmux-captures-`) resolve successfully but return an ABSOLUTE `relativePath`, so consumers must branch on `scope` rather than inferring from absoluteness. Raw `..` segments are refused for that exception at every entry point, and an owned-named symlink whose realpath escapes the temp roots is rejected. `external-read` is the READ-ONLY third scope: a default-closed, configure-once allowlist of roots outside the project (the openbuff config directory, plus `readableRoots` from `openbuff.json`) reachable ONLY through `resolveProjectPathForRead` / `resolveProjectPathForFileSystemRead`. `resolveProjectPath` / `resolveProjectPathForFileSystem` are deliberately left unchanged because they are the write path's resolvers, so a write cannot reach an allowlisted root even by mistake — a write handler would have to be edited to call a differently-named function — and `sdk/src/tools/filesystem-authority.ts` additionally fails closed on the scope. It carries an absolute `relativePath` for the same reason owned-temp does. The resolver dereferences EXACTLY ONCE (a second, independently resolved realpath reopens a TOCTOU window), requires both the lexical and dereferenced form to be STRICTLY inside a root via `path.relative`-based containment (so a sibling-prefix `-evil` is refused), and applies `isMandatorySensitiveReadPath` to both full paths so every current and future consumer inherits the credential refusal fail-closed rather than each handler remembering it. `configureExternalReadRoots` skips filesystem-root and `..` entries, throws on a differing set for the same owning project, and takes an optional `projectRoot` owner so a genuine project switch REPLACES the boundary instead of leaving the previous project's roots in force; `ensureExternalReadRootsConfigured` is the non-throwing wrapper for per-run callers and reports `'refused-changed'` rather than widening a boundary earlier reads were validated against. +- **Credential carriers (`common/src/util/sensitive-paths.ts`)**: `isMandatorySensitiveReadPath` is the ONE refusal check callers must remember, so both new rules are composed into it rather than exported separately. `credentials.json|.yaml|.yml` are exact basenames (not a `credentials.*` pattern) so a repository `credentials.md` stays readable, and a `credentials`-bearing basename with a structured-data extension (`application_default_credentials.json`, `gcloud_credentials.json`) is refused. Path-aware carriers are matched on their OWNING PARENT directory because their basenames are far too generic to blanket-block: `.kube/config`, `.docker/config.json`, `gh/hosts.yml`, `.aws/config` — blocking every `config.json` would make most repositories unreadable, and blocking every `hosts.yml` would break ansible inventories. - **Edit transport truncation (`common/src/tools/params/utils.ts` + `edit-transaction.ts`)**: tool-call argument payloads cut in transport are classified with machine code `payload_truncated` (`PAYLOAD_TRUNCATED_ERROR_CODE`), distinct from genuine syntax/preflight failures. `edit_transaction` abort results may set `errorCode`/`failureKind` to `payload_truncated` and can recover at a clean edit boundary when the truncated encoding is still provably complete; otherwise they fail closed without applying any edit. - **Content-search params**: `code_search`, `find_files_matching_content`, and `glob` accept a `cwd` that may resolve outside the project root (file-as-cwd is coerced/rejected consistently). Flag allowlists stay aligned with the SDK handlers so docs and runtime do not drift. - **Coercion helpers (`common/src/tools/params/utils.ts`)**: array/object coercion for tool args remains fail-closed for truncated or ambiguous encodings; gate specialist crash taxonomy and v3 snapshot attestation depend on these helpers not inventing structure from cut payloads. @@ -60,6 +61,10 @@ This package contains code shared across the Openbuff monorepo, especially the l - _Knowledge refresh 2026-08-31: additive `context_compaction_status` print-mode variant and the transient loop-owned `AgentState.suppressSemanticCompaction` anti-thrash advisory (bare-agent-id pruner matching) documented under the shared provider/message boundaries._ +- _Knowledge refresh 2026-08-31 (followups): `printModeErrorSchema` in `common/src/types/print-mode.ts` keeps `userMessage` and `autoRecovering` optional and additive; together they mark errors the runtime is already steering the agent out of — a malformed tool call being retried, or a control-flow/ordering rejection such as `suggest_followups` called before the gate passed or after the turn's final tool. UIs must treat those events as non-user-visible (log-only, no error banner) while the full `message` still flows to the agent, and consumers that ignore the optional fields keep the pre-existing visible-error behavior._ + +- _Knowledge refresh 2026-08-31 (external read roots): `common/src/util/project-path-containment.ts` gained the read-only `external-read` scope, its default-closed configure-once registry (`configureExternalReadRoots` / `ensureExternalReadRootsConfigured` / `getExternalReadRoots` / `resetExternalReadRootsForTesting`), the `isExternalReadPath` predicate, and the `resolveProjectPathForRead` / `resolveProjectPathForFileSystemRead` entry points; `common/src/util/sensitive-paths.ts` gained the openbuff credential basenames plus the path-aware credential carriers. Consumers: the four SDK read handlers (`read-files`, `read-logs`, `read-image`, `list-directory`) via `sdk/src/tools/path-utils.ts` read-only resolvers, the `readableRoots` config field and its provenance-based trust gate in `sdk/src/provider-config.ts` + `sdk/src/run.ts` (`selectTrustedReadableRoots`, gated by `OPENBUFF_TRUST_PROJECT_READABLE_ROOTS` because project config wins the config merge), and the tool-scoped backstop exemption in `packages/agent-runtime/src/tools/tool-executor.ts` (`EXTERNAL_READ_EXEMPT_TOOLS`)._ + ## Scope Notes Openbuff is CLI/SDK-focused and local/BYOK. Do not add new dependencies from `common/` to hosted web, billing, credit, subscription, or BigQuery product surfaces. Provider-owned billing, quota, token usage, and OAuth flows may still be documented when they refer to the user's configured provider rather than an Openbuff-hosted product. diff --git a/common/src/types/print-mode.ts b/common/src/types/print-mode.ts index 233e40e4c5..ca8de88e9f 100644 --- a/common/src/types/print-mode.ts +++ b/common/src/types/print-mode.ts @@ -13,13 +13,17 @@ export const printModeErrorSchema = z.object({ type: z.literal('error'), message: z.string(), // Concise, calm summary for agent-recoverable errors (e.g. a malformed tool - // call the agent will auto-correct). When present, UIs should show this to - // the user instead of the full `message`, which carries detailed recovery - // context intended for the agent's message history. + // call the agent will auto-correct, or a runtime-enforced tool-ordering + // rejection the agent corrects by reordering). When present, UIs should show + // this to the user instead of the full `message`, which carries detailed + // recovery context intended for the agent's message history. userMessage: z.string().optional(), - // True when the runtime is already auto-correcting this error (e.g. a - // malformed tool call the model is retrying). UIs should not surface these - // as user-visible errors; the full `message` still flows to the agent. + // True when the runtime is already steering the agent out of this error and + // no user action is possible: a malformed tool call the model is retrying, or + // a control-flow/ordering rejection (e.g. `suggest_followups` called before + // the gate passed or after the turn's final tool) that the model resolves on + // its own. UIs must not surface these as user-visible errors; the full + // `message` still flows to the agent so it can correct itself. autoRecovering: z.boolean().optional(), }) export type PrintModeError = z.infer diff --git a/common/src/util/__tests__/project-path-containment.test.ts b/common/src/util/__tests__/project-path-containment.test.ts index 977ba5ab96..fb04565d01 100644 --- a/common/src/util/__tests__/project-path-containment.test.ts +++ b/common/src/util/__tests__/project-path-containment.test.ts @@ -11,10 +11,17 @@ import { } from 'bun:test' import { + configureExternalReadRoots, + ensureExternalReadRootsConfigured, + getExternalReadRoots, getOwnedTempRoots, + isExternalReadPath, isOwnedTempPath, isPathInsideProject, + resetExternalReadRootsForTesting, resolveProjectPath, + resolveProjectPathForFileSystemRead, + resolveProjectPathForRead, } from '../project-path-containment' describe('isPathInsideProject', () => { @@ -485,3 +492,365 @@ describe('openbuff-owned OS temp namespace exception', () => { }) }) }) + +describe('external read root allowlist', () => { + const projectRoot = '/repo' + let allowedRoot: string + let siblingRoot: string + let outsideDir: string + let allowedFile: string + + const cleanupPaths: string[] = [] + const removeTracked = () => { + for (const target of cleanupPaths.splice(0)) { + // `rmSync` unlinks symlinks rather than following them, so a link that + // points outside the temp dirs can never be traversed during cleanup. + fs.rmSync(target, { force: true, recursive: true }) + } + } + + beforeEach(() => { + // Mandatory: the registry is module state, so an unreset configuration + // would leave an open read boundary for every later test in the process. + resetExternalReadRootsForTesting() + + allowedRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'external-read-')) + cleanupPaths.push(allowedRoot) + // Sibling directory sharing the allowlisted root's prefix. + siblingRoot = `${allowedRoot}-evil` + fs.mkdirSync(siblingRoot) + cleanupPaths.push(siblingRoot) + outsideDir = fs.mkdtempSync(path.join(os.tmpdir(), 'external-outside-')) + cleanupPaths.push(outsideDir) + + allowedFile = path.join(allowedRoot, 'notes.txt') + fs.writeFileSync(allowedFile, 'notes\n') + fs.writeFileSync(path.join(siblingRoot, 'file.txt'), 'sibling\n') + fs.writeFileSync(path.join(outsideDir, 'secret.txt'), 'secret\n') + }) + + afterEach(() => { + resetExternalReadRootsForTesting() + removeTracked() + }) + + describe('default-closed posture', () => { + test('is unconfigured and refuses outside paths', () => { + expect(getExternalReadRoots()).toEqual([]) + expect(isExternalReadPath(allowedFile)).toBe(false) + expect(resolveProjectPathForRead(projectRoot, allowedFile)).toBeNull() + }) + + test('read resolver still behaves exactly like the write resolver in-project', () => { + const readResult = resolveProjectPathForRead(projectRoot, 'src/a.ts') + const writeResult = resolveProjectPath(projectRoot, 'src/a.ts') + expect(readResult).toEqual(writeResult) + expect(readResult!.scope).toBe('project') + }) + }) + + describe('once configured', () => { + beforeEach(() => { + configureExternalReadRoots([allowedRoot]) + }) + + test('stores the resolved root and returns a defensive copy', () => { + expect(getExternalReadRoots()).toEqual([path.resolve(allowedRoot)]) + const copy = getExternalReadRoots() + copy.push('/mutated') + expect(getExternalReadRoots()).toEqual([path.resolve(allowedRoot)]) + }) + + test('resolves a file strictly inside the root with scope "external-read"', () => { + expect(isExternalReadPath(allowedFile)).toBe(true) + + const result = resolveProjectPathForRead(projectRoot, allowedFile) + expect(result).not.toBeNull() + expect(result!.scope).toBe('external-read') + // Outside the project, so `relativePath` is the absolute resolved path. + expect(result!.relativePath).toBe(path.resolve(allowedFile)) + expect(result!.fullPath).toBe(path.resolve(allowedFile)) + expect(path.isAbsolute(result!.relativePath)).toBe(true) + }) + + test('resolves a nested file inside the root', () => { + const nestedDir = path.join(allowedRoot, 'nested') + fs.mkdirSync(nestedDir) + const nestedFile = path.join(nestedDir, 'deep.txt') + fs.writeFileSync(nestedFile, 'deep\n') + + expect(isExternalReadPath(nestedFile)).toBe(true) + const nested = resolveProjectPathForRead(projectRoot, nestedFile) + expect(nested).not.toBeNull() + expect(nested!.scope).toBe('external-read') + }) + + test('rejects the allowlisted root itself (strictly-inside rule)', () => { + expect(isExternalReadPath(allowedRoot)).toBe(false) + expect(resolveProjectPathForRead(projectRoot, allowedRoot)).toBeNull() + }) + + test('rejects a sibling-prefix directory (naive startsWith would admit it)', () => { + const siblingFile = path.join(siblingRoot, 'file.txt') + expect(isExternalReadPath(siblingFile)).toBe(false) + expect(resolveProjectPathForRead(projectRoot, siblingFile)).toBeNull() + }) + + test('rejects a raw .. input even when it collapses back inside the root', () => { + // Built by concatenation so the `..` segment survives into the input. + const collapsing = `${allowedRoot}/nested/../notes.txt` + expect(isExternalReadPath(collapsing)).toBe(false) + expect(resolveProjectPathForRead(projectRoot, collapsing)).toBeNull() + + const escaping = `${allowedRoot}/../${path.basename(outsideDir)}/secret.txt` + expect(isExternalReadPath(escaping)).toBe(false) + expect(resolveProjectPathForRead(projectRoot, escaping)).toBeNull() + }) + + test('rejects a symlink inside the root that dereferences outside it', () => { + const link = path.join(allowedRoot, 'escape.txt') + let symlinkSupported = true + try { + fs.symlinkSync(path.join(outsideDir, 'secret.txt'), link) + } catch { + // Some platforms (e.g. Windows without privileges) refuse symlinks. + symlinkSupported = false + } + if (!symlinkSupported) return + + expect(isExternalReadPath(link)).toBe(false) + expect(resolveProjectPathForRead(projectRoot, link)).toBeNull() + }) + + test('refuses credentials.json inside the root at the resolver (fail-closed)', () => { + const credentials = path.join(allowedRoot, 'credentials.json') + fs.writeFileSync(credentials, '{"apiKey":"redacted"}\n') + + // Pinned on BOTH the predicate and the resolver, so a handler cannot be + // the only thing standing between an allowlisted config root and the + // stored OAuth tokens. + expect(isExternalReadPath(credentials)).toBe(false) + expect(resolveProjectPathForRead(projectRoot, credentials)).toBeNull() + + // A non-sensitive neighbour in the same root stays readable. + expect(isExternalReadPath(allowedFile)).toBe(true) + }) + + test('WRITE-PATH INVARIANT: resolveProjectPath never reaches an allowlisted external root', () => { + // The load-bearing separation: the resolvers used by change_file / + // replace_range / filesystem-authority must stay blind to the read + // allowlist even while it is configured. + expect(resolveProjectPath(projectRoot, allowedFile)).toBeNull() + expect(isPathInsideProject(projectRoot, allowedFile)).toBe(false) + // ...while the read-only entry point does reach it. + expect(resolveProjectPathForRead(projectRoot, allowedFile)).not.toBeNull() + }) + + test('async read resolver agrees with the sync resolver', async () => { + const allowed = await resolveProjectPathForFileSystemRead( + projectRoot, + allowedFile, + fs.promises, + ) + expect(allowed).not.toBeNull() + expect(allowed!.scope).toBe('external-read') + expect(allowed!.relativePath).toBe(path.resolve(allowedFile)) + + const denied = await resolveProjectPathForFileSystemRead( + projectRoot, + path.join(siblingRoot, 'file.txt'), + fs.promises, + ) + expect(denied).toBeNull() + }) + + test('async read resolver applies the sensitive refusal too', async () => { + const credentials = path.join(allowedRoot, 'credentials.json') + fs.writeFileSync(credentials, '{"apiKey":"redacted"}\n') + + expect( + await resolveProjectPathForFileSystemRead( + projectRoot, + credentials, + fs.promises, + ), + ).toBeNull() + }) + + test('re-configuring with an equivalent set is a no-op', () => { + configureExternalReadRoots([allowedRoot]) + // Same set, different spelling/order still resolves identically. + configureExternalReadRoots([`${allowedRoot}${path.sep}`, ' ']) + expect(getExternalReadRoots()).toEqual([path.resolve(allowedRoot)]) + }) + + test('re-configuring with a different set throws', () => { + expect(() => configureExternalReadRoots([siblingRoot])).toThrow(Error) + // The original boundary is untouched by the refused attempt. + expect(getExternalReadRoots()).toEqual([path.resolve(allowedRoot)]) + expect(isExternalReadPath(path.join(siblingRoot, 'file.txt'))).toBe(false) + }) + }) + + describe('configuration entry filtering', () => { + test('skips a filesystem-root entry', () => { + const filesystemRoot = path.resolve('/') + configureExternalReadRoots([filesystemRoot, allowedRoot]) + + expect(getExternalReadRoots()).toEqual([path.resolve(allowedRoot)]) + // Allowlisting `/` would have made the whole filesystem readable. + expect(isExternalReadPath('/etc/hosts')).toBe(false) + expect(isExternalReadPath(allowedFile)).toBe(true) + }) + + test('skips an entry containing a raw .. segment', () => { + configureExternalReadRoots([`${allowedRoot}/../elsewhere`, allowedRoot]) + + expect(getExternalReadRoots()).toEqual([path.resolve(allowedRoot)]) + const skipped = path.join(path.dirname(allowedRoot), 'elsewhere', 'x') + expect(isExternalReadPath(skipped)).toBe(false) + }) + + test('drops empty entries and stays closed when nothing survives', () => { + configureExternalReadRoots(['', ' ', path.resolve('/')]) + + expect(getExternalReadRoots()).toEqual([]) + expect(isExternalReadPath(allowedFile)).toBe(false) + expect(resolveProjectPathForRead(projectRoot, allowedFile)).toBeNull() + }) + }) + + describe('ensureExternalReadRootsConfigured', () => { + test('reports "configured" for the first successful configuration', () => { + const result = ensureExternalReadRootsConfigured([allowedRoot]) + + expect(result.status).toBe('configured') + expect(result.roots).toEqual([path.resolve(allowedRoot)]) + expect(isExternalReadPath(allowedFile)).toBe(true) + }) + + test('reports "unchanged" for an equivalent second call', () => { + expect(ensureExternalReadRootsConfigured([allowedRoot]).status).toBe( + 'configured', + ) + + // Same set, different spelling/order, plus a dropped blank entry. + const result = ensureExternalReadRootsConfigured([ + `${allowedRoot}${path.sep}`, + ' ', + ]) + expect(result.status).toBe('unchanged') + expect(result.roots).toEqual([path.resolve(allowedRoot)]) + }) + + test('reports "refused-changed" without throwing and keeps the earlier boundary', () => { + ensureExternalReadRootsConfigured([allowedRoot]) + + // A mid-session edit to openbuff.json must not crash the turn, and must + // not widen a boundary earlier reads were already validated against. + const result = ensureExternalReadRootsConfigured([siblingRoot]) + + expect(result.status).toBe('refused-changed') + expect(result.roots).toEqual([path.resolve(allowedRoot)]) + if (result.status === 'refused-changed') { + expect(result.attempted).toEqual([path.resolve(siblingRoot)]) + } + // The registry itself is untouched by the refused attempt. + expect(getExternalReadRoots()).toEqual([path.resolve(allowedRoot)]) + expect(isExternalReadPath(allowedFile)).toBe(true) + expect(isExternalReadPath(path.join(siblingRoot, 'file.txt'))).toBe(false) + }) + + test('a first configuration whose entries are all skipped still reports "configured"', () => { + // Every entry is refused by normalization, so the stored value is `[]` — + // which must not be misreported as an already-configured "unchanged". + const result = ensureExternalReadRootsConfigured([path.resolve('/'), ' ']) + + expect(result.status).toBe('configured') + expect(result.roots).toEqual([]) + // ...and a later differing set is refused rather than applied. + expect(ensureExternalReadRootsConfigured([allowedRoot]).status).toBe( + 'refused-changed', + ) + expect(isExternalReadPath(allowedFile)).toBe(false) + }) + }) + + describe('projectRoot owner (ER-5)', () => { + // The registry is process-global while `cwd` is per-run, so the boundary is + // tagged with the project it belongs to: a genuine project switch must + // REPLACE the boundary instead of being refused, which would otherwise + // leave project A's roots readable while project B's never applied. + const projectA = path.resolve('/project-a') + const projectB = path.resolve('/project-b') + + test('a different owner REPLACES the boundary', () => { + expect( + ensureExternalReadRootsConfigured([allowedRoot], projectA).status, + ).toBe('configured') + expect(isExternalReadPath(allowedFile)).toBe(true) + + const switched = ensureExternalReadRootsConfigured( + [siblingRoot], + projectB, + ) + + expect(switched.status).toBe('configured') + expect(switched.roots).toEqual([path.resolve(siblingRoot)]) + expect(getExternalReadRoots()).toEqual([path.resolve(siblingRoot)]) + // The new project's boundary is the one in force, not project A's. + expect(isExternalReadPath(path.join(siblingRoot, 'file.txt'))).toBe(true) + expect(isExternalReadPath(allowedFile)).toBe(false) + }) + + test('the SAME owner with a different set is still refused', () => { + configureExternalReadRoots([allowedRoot], projectA) + + // Same project mid-run: this is the configure-once violation the registry + // exists to prevent, owner or not. + expect(() => + configureExternalReadRoots([siblingRoot], projectA), + ).toThrow(Error) + expect(getExternalReadRoots()).toEqual([path.resolve(allowedRoot)]) + + const refused = ensureExternalReadRootsConfigured([siblingRoot], projectA) + expect(refused.status).toBe('refused-changed') + expect(refused.roots).toEqual([path.resolve(allowedRoot)]) + expect(isExternalReadPath(path.join(siblingRoot, 'file.txt'))).toBe(false) + }) + + test('an equivalent call adopts an owner the first call did not supply', () => { + // First call has no owner, so the registry has no project to compare a + // later switch against. + expect(ensureExternalReadRootsConfigured([allowedRoot]).status).toBe( + 'configured', + ) + + // Equivalent set WITH an owner: reported as unchanged, but the owner is + // adopted... + expect( + ensureExternalReadRootsConfigured([allowedRoot], projectA).status, + ).toBe('unchanged') + + // ...so this genuine switch is detected and replaces the boundary rather + // than being refused as an unknown-owner reconfiguration. + const switched = ensureExternalReadRootsConfigured( + [siblingRoot], + projectB, + ) + expect(switched.status).toBe('configured') + expect(getExternalReadRoots()).toEqual([path.resolve(siblingRoot)]) + }) + + test('without an adopted owner a later switch is refused', () => { + // Contrast case for the adoption above: no owner is ever recorded, so a + // second project cannot be distinguished from a mid-run re-point. + ensureExternalReadRootsConfigured([allowedRoot]) + + const refused = ensureExternalReadRootsConfigured([siblingRoot], projectB) + + expect(refused.status).toBe('refused-changed') + expect(getExternalReadRoots()).toEqual([path.resolve(allowedRoot)]) + }) + }) +}) diff --git a/common/src/util/__tests__/sensitive-paths.test.ts b/common/src/util/__tests__/sensitive-paths.test.ts index 4b816978a9..cfa9d451e0 100644 --- a/common/src/util/__tests__/sensitive-paths.test.ts +++ b/common/src/util/__tests__/sensitive-paths.test.ts @@ -40,6 +40,74 @@ describe('isMandatorySensitiveReadPath', () => { } }) + test('blocks openbuff/cloud-CLI credential files, case-normalized', () => { + for (const path of [ + 'credentials.json', + 'credentials.yaml', + 'credentials.yml', + // The openbuff global config directory location. + '/home/user/.config/openbuff/credentials.json', + // Basename matching is case-normalized. + 'Credentials.JSON', + 'CREDENTIALS.YML', + ]) { + expect(isMandatorySensitiveReadPath(path)).toBe(true) + } + }) + + test('blocks path-aware credential carriers under their owning tool directory', () => { + for (const path of [ + // kubeconfig, docker registry auth, gh CLI OAuth token store, AWS config. + '/home/user/.kube/config', + '/home/user/.docker/config.json', + '/home/user/.config/gh/hosts.yml', + '/home/user/.aws/config', + // The bare `credentials` half of the `.aws` pair. + '/home/user/.aws/credentials', + ]) { + expect(isMandatorySensitiveReadPath(path)).toBe(true) + } + }) + + test('keeps generic config and inventory files readable', () => { + // These basenames are far too generic to blanket-block: the carriers above + // are matched on their owning parent directory precisely so ordinary + // repository files stay readable. + for (const path of [ + 'src/config', + 'src/config.json', + // No `gh` ancestor, so this is an ansible inventory, not a token store. + 'inventory/hosts.yml', + ]) { + expect(isMandatorySensitiveReadPath(path)).toBe(false) + } + }) + + test('blocks credentials-bearing basenames with a structured-data extension', () => { + for (const path of [ + 'application_default_credentials.json', + '/home/user/.config/gcloud/application_default_credentials.json', + 'gcloud_credentials.json', + 'service_credentials.yaml', + 'service_credentials.yml', + ]) { + expect(isMandatorySensitiveReadPath(path)).toBe(true) + } + }) + + test('keeps credential documentation readable', () => { + // Deliberately narrow basenames instead of a `credentials.*` pattern. + for (const path of [ + 'credentials.md', + 'docs/credentials-guide.md', + 'docs/credentials.txt', + // A `credentials`-bearing doc, not a structured credential store. + 'docs/application_default_credentials.md', + ]) { + expect(isMandatorySensitiveReadPath(path)).toBe(false) + } + }) + test('allows env template files', () => { expect(isMandatorySensitiveReadPath('.env.example')).toBe(false) expect(isMandatorySensitiveReadPath('.env.sample')).toBe(false) diff --git a/common/src/util/project-path-containment.ts b/common/src/util/project-path-containment.ts index c3b6c53c7f..e4c24c226a 100644 --- a/common/src/util/project-path-containment.ts +++ b/common/src/util/project-path-containment.ts @@ -2,6 +2,8 @@ import fs from 'fs' import os from 'os' import path from 'path' +import { isMandatorySensitiveReadPath } from './sensitive-paths' + import type { CodebuffFileSystem } from '../types/filesystem' /** @@ -13,16 +15,23 @@ import type { CodebuffFileSystem } from '../types/filesystem' * - `relativePath` is the project-relative form of the path, with OS-native * separators (i.e. whatever `path.relative` produces). Callers can use it * as a lookup key into a project file tree built with the same - * convention. For the owned-temp exception it is the ABSOLUTE resolved - * path instead; branch on `scope` to tell the two apart rather than - * inferring from absoluteness. + * convention. For the owned-temp exception — and, identically, for the + * read-only `external-read` exception — it is the ABSOLUTE resolved path + * instead, because a project-relative form for a path outside the project + * would be a meaningless traversal string. Consumers must branch on `scope` + * rather than inferring from absoluteness. */ export type ContainedProjectPath = { fullPath: string realFullPath: string relativePath: string - /** 'project' for in-project paths; 'owned-temp' for the openbuff-owned OS temp namespace exception. */ - scope: 'project' | 'owned-temp' + /** + * 'project' for in-project paths; 'owned-temp' for the openbuff-owned OS + * temp namespace exception; 'external-read' for a path inside an explicitly + * allowlisted read-only root outside the project (reachable only through + * `resolveProjectPathForRead` / `resolveProjectPathForFileSystemRead`). + */ + scope: 'project' | 'owned-temp' | 'external-read' } /** @@ -391,6 +400,412 @@ async function ownedTempContainedPathForFileSystem( } } +// Read-only allowlist of roots outside the project that path-taking READ tools +// may reach. Deliberately module-level and configure-once: the agent runtime +// backstop and the SDK read handlers must agree on one identical set, and a +// mutable per-call parameter threaded through every read handler would let one +// caller widen the boundary the other enforces. +let externalReadRoots: string[] | undefined +let externalReadComparisonRoots: string[] | undefined +// The project root the stored boundary belongs to, when the configuring caller +// supplied one. WHY it is tracked: the registry is process-global, so without +// an owner a second project configured in the same process (e.g. after +// `switchProjectContext`) is indistinguishable from a mid-run attempt to +// re-point the boundary — the strict primitive would refuse it and project A's +// roots would stay in force while project B's never applied. +let externalReadRootsOwner: string | undefined + +/** + * Canonical normalization for external read root entries: `path.resolve`, + * empty/whitespace-only entries dropped, deduped and sorted so the stored value + * is order-independent. + * + * Two entry shapes are REFUSED (skipped, not thrown): + * - a filesystem root (`path.dirname(resolved) === resolved`): allowlisting + * `/` or `C:\` would make the entire filesystem readable, which is the exact + * opposite of an allowlist; + * - an entry containing a raw `..` segment: the same rule the owned-temp + * exception applies to inputs, applied to the boundary definition itself. + * + * Shared by `configureExternalReadRoots` and + * `ensureExternalReadRootsConfigured` so the `attempted` set the wrapper + * reports can never describe a different boundary than the one the strict + * primitive evaluated. + */ +function normalizeExternalReadRoots(roots: readonly string[]): string[] { + return [ + ...new Set( + roots + .filter((root) => root.trim() !== '') + .filter((root) => !hasTraversalSegment(root)) + .map((root) => path.resolve(root)) + .filter((resolved) => path.dirname(resolved) !== resolved), + ), + ].sort() +} + +/** + * Configure the read-only external root allowlist. + * + * Entries are normalized by `normalizeExternalReadRoots` (see there for the + * refused entry shapes). + * + * Idempotent for an equivalent set. Calling it with a DIFFERENT set for the + * SAME (or an unknown) owning project THROWS: silently re-pointing a security + * boundary mid-run is precisely the failure mode this registry exists to + * prevent, and a late widening would apply to reads already validated against + * the earlier set. + * + * `projectRoot` is the project the boundary belongs to. Supplying a DIFFERENT + * project root than the stored owner REPLACES the boundary instead of refusing + * it: that is a legitimate project switch, and replacing is strictly safer than + * keeping a boundary that belongs to another project (which would leave project + * A's roots readable while project B's own allowlist never applied). Omitting + * `projectRoot` keeps the historical configure-once-per-process behavior, so + * existing callers and tests are unaffected. + * + * Callers that run on EVERY run (rather than exactly once) must use + * `ensureExternalReadRootsConfigured` instead of catching this throw + * themselves. + */ +export function configureExternalReadRoots( + roots: readonly string[], + projectRoot?: string, +): void { + const normalized = normalizeExternalReadRoots(roots) + const owner = + projectRoot === undefined ? undefined : path.resolve(projectRoot) + + if (externalReadRoots) { + // A different KNOWN owner means a project switch, which replaces the + // boundary below. Anything else keeps the strict configure-once semantics. + const projectSwitched = + owner !== undefined && + externalReadRootsOwner !== undefined && + owner !== externalReadRootsOwner + if (!projectSwitched) { + const unchanged = + externalReadRoots.length === normalized.length && + externalReadRoots.every((root, index) => root === normalized[index]) + if (unchanged) { + // Adopt an owner the first caller did not supply, so a later genuine + // project switch is still detectable. + externalReadRootsOwner ??= owner + return + } + throw new Error( + `External read roots are already configured with ${externalReadRoots.length} root(s); ` + + `refusing to reconfigure with a different set of ${normalized.length} root(s). ` + + 'The external read boundary is configure-once per process.', + ) + } + } + + externalReadRoots = normalized + externalReadRootsOwner = owner ?? externalReadRootsOwner + // The comparison roots memoize a realpath-dereferenced view of the value + // being replaced here, so they must be invalidated whenever it changes — + // otherwise the first configuration wins for the life of the process. + externalReadComparisonRoots = undefined +} + +/** + * The configured external read roots, as a defensive copy. An empty array when + * unconfigured: the default posture is closed, so every external-read helper + * behaves as if the feature does not exist until someone configures it. + */ +export function getExternalReadRoots(): string[] { + return [...(externalReadRoots ?? [])] +} + +/** + * Outcome of `ensureExternalReadRootsConfigured`. + * + * - `'configured'`: this call performed the one configuration for the process, + * OR it replaced the boundary because `projectRoot` named a different project + * than the stored owner (a legitimate project switch — see + * `configureExternalReadRoots`). + * - `'unchanged'`: the registry was already set to an equivalent value. + * - `'refused-changed'`: the strict primitive rejected a DIFFERENT set. `roots` + * is the still-effective boundary; `attempted` is the normalized set that was + * refused. Callers MUST NOT treat this as success. + */ +export type ExternalReadConfigurationResult = + | { status: 'configured'; roots: string[] } + | { status: 'unchanged'; roots: string[] } + | { status: 'refused-changed'; roots: string[]; attempted: string[] } + +/** + * Non-throwing wrapper around `configureExternalReadRoots` for wiring that runs + * on EVERY run rather than exactly once (see `sdk/src/run.ts`). + * + * WHY this exists: the boundary stays configure-once per process, but the + * caller does not. A user who edits `openbuff.json` mid-session to add a + * `readableRoots` entry would make the next run call the strict primitive with + * a different set, and the raw throw would crash that turn. So a mid-session + * change is deliberately NOT applied: keeping the earlier (narrower or equal) + * boundary is the fail-safe choice, because reads already validated in this + * process were validated against that earlier boundary, and widening it + * retroactively would mean earlier decisions no longer describe the boundary in + * force. Callers surface a "restart to apply" warning instead. + * + * `'refused-changed'` is NOT success: nothing was applied, and the returned + * `roots` are the pre-existing boundary. + */ +export function ensureExternalReadRootsConfigured( + roots: readonly string[], + projectRoot?: string, +): ExternalReadConfigurationResult { + // Read the registry's own "has it been configured?" state rather than + // comparing before/after snapshots: a first configuration whose entries are + // all skipped (e.g. only a filesystem root) stores `[]`, which is + // indistinguishable from the unconfigured snapshot and would be misreported + // as `'unchanged'`. + const alreadyConfigured = externalReadRoots !== undefined + // Captured BEFORE configuring: a project switch replaces the boundary, so it + // is reported as `'configured'` rather than `'unchanged'`. + const owner = + projectRoot === undefined ? undefined : path.resolve(projectRoot) + const projectSwitched = + owner !== undefined && + externalReadRootsOwner !== undefined && + owner !== externalReadRootsOwner + try { + configureExternalReadRoots(roots, projectRoot) + } catch { + // The strict primitive only throws for a differing set, and it throws + // BEFORE mutating anything, so the earlier boundary is still in force. + return { + status: 'refused-changed', + roots: getExternalReadRoots(), + attempted: normalizeExternalReadRoots(roots), + } + } + return { + status: alreadyConfigured && !projectSwitched ? 'unchanged' : 'configured', + roots: getExternalReadRoots(), + } +} + +/** + * Clear the external read registry. Exists ONLY for test isolation — the + * registry is module state, so a test that configures it would otherwise leave + * an open boundary for every later test importing this module. Must not be + * called from production code paths; the configure-once throw is the intended + * production behavior. + */ +export function resetExternalReadRootsForTesting(): void { + externalReadRoots = undefined + externalReadComparisonRoots = undefined + // The owner must be cleared too, or a later suite configuring for a different + // project would be treated as a project switch against a stale owner. + externalReadRootsOwner = undefined +} + +/** + * Configured external read roots in both lexical and symlink-dereferenced + * form, mirroring `getOwnedTempComparisonRoots`: a configured root may itself + * be a symlink (e.g. a home directory on macOS), so a file's realpath only + * lands under the dereferenced root. + */ +function getExternalReadComparisonRoots(): string[] { + if (!externalReadComparisonRoots) { + const roots = getExternalReadRoots() + externalReadComparisonRoots = [ + ...new Set([...roots, ...roots.map(realpathCachedForRoot)]), + ] + } + return externalReadComparisonRoots +} + +/** + * Async counterpart of `getExternalReadComparisonRoots`. The dereferenced form + * comes from the injected filesystem and is memoized per filesystem in + * `projectRootFileSystemRealpathCache`, matching + * `getOwnedTempComparisonRootsForFileSystem`. + */ +async function getExternalReadComparisonRootsForFileSystem( + fileSystem: CodebuffFileSystem, +): Promise { + const roots = getExternalReadRoots() + const realRoots = await Promise.all( + roots.map((root) => realpathCachedForFileSystemRoot(root, fileSystem)), + ) + return [...new Set([...roots, ...realRoots])] +} + +/** + * True when `target` is STRICTLY inside one of `roots`. The root itself is + * refused: it is not a readable file, and admitting it would silently widen + * later directory-listing consumers to the root entry itself. + * + * Containment goes through `escapesRoot`, which is `path.relative`-based, so a + * sibling-prefix directory like `-evil` is correctly refused where a + * naive `startsWith` check would admit it. + */ +function isInsideExternalReadRoot(target: string, roots: string[]): boolean { + return roots.some((root) => { + const relative = path.relative(root, target) + return relative !== '' && !escapesRoot(root, target) + }) +} + +/** + * Resolve the ALREADY-RESOLVED absolute `fullPath` to the ONE real path that + * is both validated here and used by callers for the actual read. Returns + * `null` when the path is not strictly inside a configured external read root + * — including whenever the registry is unconfigured, since the comparison root + * list is then empty. + * + * The raw-input `..` policy is enforced by the entry points (see + * `hasTraversalSegment`), never here: this function only ever sees collapsed + * paths. + * + * The real path is dereferenced EXACTLY ONCE: validating one realpath and then + * handing callers a second, independently resolved one leaves a TOCTOU window + * where a symlink swapped in between the two resolutions redirects the read to + * an arbitrary target. Requiring the dereferenced path to satisfy containment + * too closes the symlink escape — a file inside an allowlisted root that + * points at `/etc/shadow`. + */ +function resolveExternalReadRealPath(fullPath: string): string | null { + const roots = getExternalReadComparisonRoots() + if (!isInsideExternalReadRoot(fullPath, roots)) return null + + const realFullPath = realpathOrLexical(fullPath) + if (!isInsideExternalReadRoot(realFullPath, roots)) return null + + // Fail-closed sensitive refusal, checked on BOTH the lexical and the + // dereferenced basename (a benign-looking name may link to `credentials.json` + // and vice versa). + // + // This lives in the resolver rather than in individual read handlers so every + // current and future external-read consumer inherits it: the primary thing an + // allowlisted config root would otherwise expose is + // `/credentials.json`, and a handler that forgot the check would + // leak provider OAuth tokens and the default API key. This is why + // `credentials.json` had to be added to the sensitive basenames first. + // + // The FULL paths are passed (not just the basenames) so the path-aware + // credential carriers `isMandatorySensitiveReadPath` recognizes — + // `.kube/config`, `.docker/config.json`, `gh/hosts.yml`, `.aws/config` — + // are refused too. Those basenames are far too generic to block on their own, + // so a basename-only call would silently expose them inside an allowlisted + // home-directory root. + if ( + isMandatorySensitiveReadPath(fullPath) || + isMandatorySensitiveReadPath(realFullPath) + ) { + return null + } + + return realFullPath +} + +/** Async counterpart of `resolveExternalReadRealPath` for injected filesystems. */ +async function resolveExternalReadRealPathForFileSystem( + fullPath: string, + fileSystem: CodebuffFileSystem, +): Promise { + const roots = await getExternalReadComparisonRootsForFileSystem(fileSystem) + if (!isInsideExternalReadRoot(fullPath, roots)) return null + + // Resolved once, exactly like the sync helper: the validated string is the + // string callers read from. + const realFullPath = await realpathOrLexicalForFileSystem( + fullPath, + fileSystem, + ) + if (!isInsideExternalReadRoot(realFullPath, roots)) return null + + // Identical fail-closed sensitive refusal as the sync resolver, on the same + // FULL paths; the two must never disagree about `credentials.json` or about a + // path-aware carrier like `.docker/config.json`. + if ( + isMandatorySensitiveReadPath(fullPath) || + isMandatorySensitiveReadPath(realFullPath) + ) { + return null + } + + return realFullPath +} + +/** + * Resolve a caller input to an absolute path the same way the containment + * resolvers do: absolute inputs as given, relative inputs against the project + * root (never `process.cwd()`). + */ +function resolveAgainstRoot(projectRoot: string, input: string): string { + return path.isAbsolute(input) + ? path.resolve(input) + : path.resolve(path.resolve(projectRoot), input) +} + +/** + * True when `input` resolves strictly inside a configured external read root + * and is not a mandatory-sensitive file. Returns `false` whenever the registry + * is unconfigured. + * + * Contract: a raw input containing a `..` segment is refused outright, even + * when it would collapse back inside an allowlisted root — the check must live + * here, above `path.resolve`. `resolveProjectPathForRead` and + * `resolveProjectPathForFileSystemRead` apply the same rule, so all three + * agree on any given input. + */ +export function isExternalReadPath(input: string): boolean { + if (!input || hasTraversalSegment(input)) return false + return resolveExternalReadRealPath(path.resolve(input)) !== null +} + +/** + * Build the containment result for an external read path. `relativePath` is the + * absolute resolved path: allowlisted roots live outside the project, so a + * project-relative form would be meaningless (and would look like a traversal + * escape). + * + * Takes the ALREADY-RESOLVED absolute path from the caller: re-resolving the + * raw input here would resolve a relative input against `process.cwd()` + * instead of the caller's project root. + * + * `realFullPath` is the exact string that `resolveExternalReadRealPath` + * validated — never a second, independently resolved realpath. + */ +function externalReadContainedPath( + fullPath: string, +): ContainedProjectPath | null { + const realFullPath = resolveExternalReadRealPath(fullPath) + if (realFullPath === null) return null + return { + fullPath, + realFullPath, + relativePath: fullPath, + scope: 'external-read', + } +} + +/** + * Async counterpart of `externalReadContainedPath`. Also takes the + * already-resolved absolute path, and like the sync variant uses the single + * validated resolution as `realFullPath`. + */ +async function externalReadContainedPathForFileSystem( + fullPath: string, + fileSystem: CodebuffFileSystem, +): Promise { + const realFullPath = await resolveExternalReadRealPathForFileSystem( + fullPath, + fileSystem, + ) + if (realFullPath === null) return null + return { + fullPath, + realFullPath, + relativePath: fullPath, + scope: 'external-read', + } +} + /** * Resolve `input` against `projectRoot` and verify it stays inside the * project. Returns `null` when: @@ -499,6 +914,61 @@ export async function resolveProjectPathForFileSystem( } } +/** + * READ-ONLY containment resolver: `resolveProjectPath` first, then the + * configured external read allowlist as a fallback. + * + * `resolveProjectPath` and `resolveProjectPathForFileSystem` are DELIBERATELY + * LEFT UNCHANGED. They are the resolvers the write path uses + * (`sdk/src/tools/change-file.ts`, `sdk/src/tools/replace-range.ts`, + * `sdk/src/tools/filesystem-authority.ts`), so keeping the external-read + * widening in separate, differently-named read-only entry points means a write + * cannot reach an allowlisted root even by mistake — a write handler would have + * to be edited to call this function instead. Any future caller of this + * resolver MUST be a read-only operation. + * + * All existing project and owned-temp behavior is identical by construction: + * a non-null delegate result is returned unchanged, and the external-read + * branch is only consulted when the delegate returns `null`. That branch + * refuses a raw `..` first, exactly like the owned-temp fallback, so + * `isExternalReadPath` and this resolver agree on every input. + */ +export function resolveProjectPathForRead( + projectRoot: string, + input: string, +): ContainedProjectPath | null { + const contained = resolveProjectPath(projectRoot, input) + if (contained !== null) return contained + if (!input || hasTraversalSegment(input)) return null + // Anchored on the project root exactly like the owned-temp fallback, so a + // relative input never depends on `process.cwd()`. + return externalReadContainedPath(resolveAgainstRoot(projectRoot, input)) +} + +/** + * Async counterpart of `resolveProjectPathForRead`, for operations executed + * through an injected filesystem. Same read-only contract, and the same reason + * `resolveProjectPathForFileSystem` is left unchanged: it is the write path's + * resolver. + */ +export async function resolveProjectPathForFileSystemRead( + projectRoot: string, + input: string, + fileSystem: CodebuffFileSystem, +): Promise { + const contained = await resolveProjectPathForFileSystem( + projectRoot, + input, + fileSystem, + ) + if (contained !== null) return contained + if (!input || hasTraversalSegment(input)) return null + return externalReadContainedPathForFileSystem( + resolveAgainstRoot(projectRoot, input), + fileSystem, + ) +} + /** * Boolean convenience wrapper for tools that only need to know "is this path * inside the project root?" without the resolved metadata. diff --git a/common/src/util/sensitive-paths.ts b/common/src/util/sensitive-paths.ts index ccdb3cb364..58a5c3e11c 100644 --- a/common/src/util/sensitive-paths.ts +++ b/common/src/util/sensitive-paths.ts @@ -12,6 +12,15 @@ const SENSITIVE_BASENAMES = new Set([ '.htpasswd', '.netrc', 'credentials', + // openbuff/cloud-CLI credential files: the openbuff global config directory + // stores OAuth access/refresh tokens and the default API key in + // `/credentials.json` (see `sdk/src/credentials.ts`). Listed as + // exact basenames rather than a `credentials.*` pattern so a repository doc + // named `credentials.md` stays readable. `isMandatorySensitiveReadPath` is + // basename-driven and case-normalized, so entries must be lowercase. + 'credentials.json', + 'credentials.yaml', + 'credentials.yml', '.npmrc', 'auth.json', '.pypirc', @@ -37,6 +46,77 @@ export function isEnvTemplatePath(filePath: string): boolean { return ENV_TEMPLATE_SUFFIXES.some((suffix) => basename.endsWith(suffix)) } +// Basename suffixes that make a `credentials`-bearing name a machine-readable +// credential store rather than documentation. `credentials.md` / +// `credentials.txt` deliberately stay readable. +const CREDENTIAL_BASENAME_SUFFIXES = ['.json', '.yaml', '.yml'] +// gcloud application default credentials. Subsumed by the general rule below, +// but pinned explicitly because it is the single most common cloud credential +// carrier an allowlisted home-directory root would expose. +const APPLICATION_DEFAULT_CREDENTIALS_PATTERN = + /^application_default_credentials\.json$/ + +/** + * True for a basename that carries `credentials` AND a structured-data + * extension (`application_default_credentials.json`, `gcloud_credentials.json`, + * `credentials.yml`, ...). Deliberately narrower than `credentials.*` so + * repository docs named `credentials.md` / `credentials.txt` stay readable. + */ +function isCredentialBasename(basename: string): boolean { + return ( + APPLICATION_DEFAULT_CREDENTIALS_PATTERN.test(basename) || + (basename.includes('credentials') && + CREDENTIAL_BASENAME_SUFFIXES.some((suffix) => basename.endsWith(suffix))) + ) +} + +/** + * Path-aware credential carriers: files whose BASENAME is far too generic to + * blanket-block (`config`, `config.json`, `hosts.yml`), but which are + * unambiguous credential stores when they sit under the owning tool's + * directory. + * + * WHY this is path-aware instead of another `SENSITIVE_BASENAMES` entry: + * blocking every `config` or `config.json` would make most repositories + * unreadable, and blocking every `hosts.yml` would break ansible inventories + * and docs. Composed into `isMandatorySensitiveReadPath` rather than exported + * as a second predicate so callers keep having exactly ONE refusal check to + * remember. + * + * Expects the already-portable, lowercased path form produced by + * `toPortablePath` + `toLowerCase`. + */ +function isCredentialDirectoryPath(portablePath: string): boolean { + const segments = portablePath.split('/').filter(Boolean) + const basename = segments.at(-1) + const parent = segments.at(-2) + if (!basename || !parent) return false + + // GitHub CLI OAuth token store, e.g. `~/.config/gh/hosts.yml`. Any `gh` + // ancestor qualifies so a nested layout is covered too. + if ( + (basename === 'hosts.yml' || basename === 'hosts.yaml') && + segments.slice(0, -1).includes('gh') + ) { + return true + } + // kubeconfig: `~/.kube/config` only, directly under `.kube`. + if (basename === 'config' && parent === '.kube') return true + // Docker registry auth (base64 registry credentials): `~/.docker/config.json`. + if (basename === 'config.json' && parent === '.docker') return true + // AWS shared credentials. `~/.aws/credentials` already matches the bare + // `credentials` basename in SENSITIVE_BASENAMES; it is listed here too so the + // pair stays visible in one place and neither half can be dropped silently. + if ( + (basename === 'config' || basename === 'credentials') && + parent === '.aws' + ) { + return true + } + + return false +} + /** Mandatory, case-normalized sensitive-file policy shared by discovery and reads. */ export function isMandatorySensitiveReadPath(filePath: string): boolean { const portable = toPortablePath(filePath).toLowerCase() @@ -52,6 +132,11 @@ export function isMandatorySensitiveReadPath(filePath: string): boolean { (/^id_(rsa|ed25519|dsa|ecdsa)/.test(basename) && !basename.endsWith('.pub')) || basename.endsWith('_credentials') || + isCredentialBasename(basename) || + // Path-aware credential carriers (`.kube/config`, `.docker/config.json`, + // `gh/hosts.yml`, `.aws/config`), matched on their parent directory because + // their basenames are far too generic to blanket-block. + isCredentialDirectoryPath(portable) || // kubeconfig: exact credential filenames, not docs/scripts that mention the word basename === 'kubeconfig' || basename.endsWith('.kubeconfig') || diff --git a/docs/agents-and-tools.md b/docs/agents-and-tools.md index 0876a20bc1..a6e44df9eb 100644 --- a/docs/agents-and-tools.md +++ b/docs/agents-and-tools.md @@ -728,6 +728,17 @@ is set on the allow path and cleared at the start of each base2 user turn. `GATE: PENDING` still rejects `suggest_followups`. Non-gated agents (`canSuggestFollowups` undefined) are unchanged. +These ordering/gate rejections are agent-facing control-flow diagnostics, not +user errors: the runtime emits them with a concise `userMessage` plus +`autoRecovering: true`, so `handleRuntimeError` in +`cli/src/utils/sdk-event-handlers.ts` returns early and no error banner is +shown to the user, while the full `message` still reaches the model through the +`TOOL_CALL_ERROR` path in `packages/agent-runtime/src/tools/stream-parser.ts`. +The rejection wording states plainly that `suggest_followups` is the FINAL +output of the turn: completion summary first, then `git-committer` if +committing, then `suggest_followups` with nothing after it except +`end_turn`/`task_completed`. + ### Background shell jobs (`check_job` / `read_logs` / `kill_job` / `list_jobs`) Background jobs are unified behind a single `JobRegistry` (in the `common` diff --git a/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts b/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts index d5fb03d955..2a80910a07 100644 --- a/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts +++ b/packages/agent-runtime/src/__tests__/run-agent-step-tools.test.ts @@ -7,6 +7,11 @@ import { TEST_AGENT_RUNTIME_IMPL } from '@codebuff/common/testing/impl/agent-run import { getInitialSessionState } from '@codebuff/common/types/session-state' import { promptSuccess } from '@codebuff/common/util/error' import { assistantMessage, userMessage } from '@codebuff/common/util/messages' +import { + configureExternalReadRoots, + getOwnedTempRoots, + resetExternalReadRootsForTesting, +} from '@codebuff/common/util/project-path-containment' import { handleWriteTodos } from '../tools/handlers/tool/write-todos' import { @@ -26,7 +31,12 @@ import { import { runAgentStep } from '../run-agent-step' import { clearAgentGeneratorCache } from '../run-programmatic-step' -import { createToolCallChunk } from './test-utils' +import { processStream } from '../tools/stream-parser' +import { + createMockStreamWithToolCalls, + createToolCallChunk, + mockFileContext as sharedMockFileContext, +} from './test-utils' import { asUserMessage } from '../util/messages' import type { AgentTemplate } from '../templates/types' @@ -503,6 +513,7 @@ describe('runAgentStep - set_output tool', () => { message: expect.stringContaining( 'Tool `suggest_followups` is not available yet', ), + autoRecovering: true, }), ) expect(chunks).not.toContainEqual( @@ -607,6 +618,7 @@ describe('runAgentStep - set_output tool', () => { expect.objectContaining({ type: 'error', message: expect.stringContaining('git-committer withheld'), + autoRecovering: true, }), ) // Pin the affirmative GATE vocabulary: withheld until GATE: PASSED / @@ -704,6 +716,7 @@ describe('runAgentStep - set_output tool', () => { expect.objectContaining({ type: 'error', message: expect.stringContaining('git-committer withheld'), + autoRecovering: true, }), ) // The spawn_agents tool_call proceeds with only the helper agent. @@ -1789,6 +1802,7 @@ describe('runAgentStep - set_output tool', () => { message: expect.stringContaining( 'Tool `suggest_followups` is not available yet', ), + autoRecovering: true, }), ) expect(chunks).not.toContainEqual( @@ -2079,6 +2093,580 @@ describe('runAgentStep - set_output tool', () => { ) }) + it('does not hard-block reads of an openbuff-owned temp path', async () => { + // The SDK deliberately allows reads under the openbuff-owned OS temp + // namespace (tmux capture evidence, background-job logs), so this runtime + // backstop must not refuse them. The path is built from getOwnedTempRoots() + // rather than a hardcoded '/tmp' because on macOS os.tmpdir() is a + // symlinked '/var/folders/...' path. + const ownedTempRead = path.join( + getOwnedTempRoots()[0], + 'tmux-captures-session-1', + 'capture-001.txt', + ) + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('read_files', { + paths: [ownedTempRead], + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['read_files', 'end_turn'], + filesystemScope: undefined, + } + + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Read back the tmux capture evidence', + }) + + // No read-scope error chunk... + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining('filesystem read scope'), + }), + ) + // ...and the read is published as a tool call. + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'read_files', + }), + ) + }) + + it('still hard-blocks writes to an openbuff-owned temp path', async () => { + // The owned-temp exception is read-only by construction: the SDK's + // filesystem-authority.ts owns the narrower owned-temp mutation policy + // (tmux captures are verification evidence a subagent must not forge), so + // this backstop must never pre-authorize a write there. + const ownedTempWrite = path.join( + getOwnedTempRoots()[0], + 'tmux-captures-session-1', + 'capture-001.txt', + ) + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('write_file', { + path: ownedTempWrite, + instructions: 'Forge tmux capture evidence', + content: 'export const blocked = true\n', + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['write_file', 'end_turn'], + filesystemScope: undefined, + } + + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Write into the owned temp namespace', + }) + + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining( + 'was blocked by the unscoped-agent filesystem write scope', + ), + }), + ) + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'write_file', + }), + ) + }) + + it('still hard-blocks reads of a non-owned absolute temp sibling', async () => { + // Attribution guard: the allow above must come from owned-temp SCOPE, not + // from "any absolute temp path". This first segment matches no + // OWNED_TEMP_SEGMENT_PATTERNS entry, so the read stays hard-blocked. + const nonOwnedTempRead = path.join( + getOwnedTempRoots()[0], + 'not-openbuff-owned', + 'file.txt', + ) + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('read_files', { + paths: [nonOwnedTempRead], + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['read_files', 'end_turn'], + filesystemScope: undefined, + } + + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Read an unowned absolute temp path', + }) + + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining( + 'was blocked by the unscoped-agent filesystem read scope', + ), + }), + ) + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'read_files', + }), + ) + }) + + it('does not hard-block reads of an allowlisted external path', async () => { + // The SDK read handlers deliberately allow reads strictly inside a root the + // user explicitly allowlisted (the openbuff config dir, plus openbuff.json + // `readableRoots`), so this runtime backstop must not refuse them. The SDK + // resolvers stay authoritative — including the fail-closed + // mandatory-sensitive refusal — this layer only stops pre-dispatch refusal. + const externalRoot = fs.mkdtempSync( + path.join(os.tmpdir(), 'external-read-backstop-'), + ) + const externalRead = path.join(externalRoot, 'notes.txt') + fs.writeFileSync(externalRead, 'notes\n') + // Module state: reset before configuring so a differing set from an earlier + // test can never make the configure-once primitive throw here. + resetExternalReadRootsForTesting() + configureExternalReadRoots([externalRoot]) + + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('read_files', { + paths: [externalRead], + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['read_files', 'end_turn'], + filesystemScope: undefined, + } + + try { + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Read a file inside an allowlisted external root', + }) + + // No read-scope error chunk... + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining('filesystem read scope'), + }), + ) + // ...and the read is published as a tool call. + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'read_files', + }), + ) + } finally { + // Unconditional reset in both the success and failure paths: an + // unreset registry would leave an open read boundary for every later + // test in this process. + resetExternalReadRootsForTesting() + fs.rmSync(externalRoot, { recursive: true, force: true }) + } + }) + + it('still hard-blocks writes to an allowlisted external path', async () => { + // The external allowlist is READ-only by construction (there is no + // external-write scope), so this backstop must never pre-authorize a write + // there — the exception stays gated on access === 'read'. + const externalRoot = fs.mkdtempSync( + path.join(os.tmpdir(), 'external-read-backstop-write-'), + ) + const externalWrite = path.join(externalRoot, 'notes.txt') + fs.writeFileSync(externalWrite, 'notes\n') + resetExternalReadRootsForTesting() + configureExternalReadRoots([externalRoot]) + + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('write_file', { + path: externalWrite, + instructions: 'Write into an allowlisted read-only root', + content: 'export const blocked = true\n', + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['write_file', 'end_turn'], + filesystemScope: undefined, + } + + try { + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Write into the allowlisted external root', + }) + + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining( + 'was blocked by the unscoped-agent filesystem write scope', + ), + }), + ) + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'write_file', + }), + ) + } finally { + resetExternalReadRootsForTesting() + fs.rmSync(externalRoot, { recursive: true, force: true }) + } + }) + + it('still hard-blocks reads of a non-allowlisted external sibling', async () => { + // Attribution guard: the allow above must come from the ALLOWLIST, not from + // "any absolute path outside the project". The sibling directory shares the + // allowlisted root's prefix, which a naive startsWith check would admit. + const externalRoot = fs.mkdtempSync( + path.join(os.tmpdir(), 'external-read-backstop-sibling-'), + ) + const siblingRoot = `${externalRoot}-evil` + fs.mkdirSync(siblingRoot) + const siblingRead = path.join(siblingRoot, 'notes.txt') + fs.writeFileSync(siblingRead, 'sibling\n') + resetExternalReadRootsForTesting() + configureExternalReadRoots([externalRoot]) + + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('read_files', { + paths: [siblingRead], + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['read_files', 'end_turn'], + filesystemScope: undefined, + } + + try { + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Read a non-allowlisted external sibling path', + }) + + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining( + 'was blocked by the unscoped-agent filesystem read scope', + ), + }), + ) + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'read_files', + }), + ) + } finally { + resetExternalReadRootsForTesting() + fs.rmSync(externalRoot, { recursive: true, force: true }) + fs.rmSync(siblingRoot, { recursive: true, force: true }) + } + }) + + it('still hard-blocks code_search with an absolute cwd inside an allowlisted external root', async () => { + // ER-3: the external-read relaxation is TOOL-scoped, not merely + // access-scoped. code_search's SDK handler performs NO containment (it + // realpaths the caller cwd and spawns ripgrep there), so it is absent from + // EXTERNAL_READ_EXEMPT_TOOLS and stays hard-blocked even for a configured + // allowlisted root — otherwise code_search({ cwd: '/projects' }) + // would recursively grep other projects' persisted transcripts. + const externalRoot = fs.mkdtempSync( + path.join(os.tmpdir(), 'external-read-code-search-'), + ) + fs.writeFileSync(path.join(externalRoot, 'notes.txt'), 'notes\n') + resetExternalReadRootsForTesting() + configureExternalReadRoots([externalRoot]) + + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('code_search', { + pattern: 'apiKey', + cwd: externalRoot, + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['code_search', 'end_turn'], + filesystemScope: undefined, + } + + try { + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Grep inside an allowlisted external root', + }) + + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining( + 'was blocked by the unscoped-agent filesystem read scope', + ), + }), + ) + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'code_search', + }), + ) + } finally { + resetExternalReadRootsForTesting() + fs.rmSync(externalRoot, { recursive: true, force: true }) + } + }) + + it('still allows read_files for an absolute path inside an allowlisted external root', async () => { + // Attribution guard for the ER-3 fix: gating the relaxation on + // EXTERNAL_READ_EXEMPT_TOOLS must not be a blanket revert. read_files IS a + // migrated tool (its SDK handler resolves through the read-only containment + // resolvers), so the same configured root that code_search cannot reach + // stays readable here. + const externalRoot = fs.mkdtempSync( + path.join(os.tmpdir(), 'external-read-exempt-read-files-'), + ) + const externalRead = path.join(externalRoot, 'notes.txt') + fs.writeFileSync(externalRead, 'notes\n') + resetExternalReadRootsForTesting() + configureExternalReadRoots([externalRoot]) + + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('read_files', { + paths: [externalRead], + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['read_files', 'end_turn'], + filesystemScope: undefined, + } + + try { + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Read a file inside an allowlisted external root', + }) + + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining('filesystem read scope'), + }), + ) + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'read_files', + }), + ) + } finally { + resetExternalReadRootsForTesting() + fs.rmSync(externalRoot, { recursive: true, force: true }) + } + }) + + it('hard-blocks find_files_matching_content with an absolute cwd outside the project', async () => { + // ER-3: find_files_matching_content used to fall through + // getFilesystemToolPaths and return undefined, so it got NO backstop at all + // while still resolving an arbitrary absolute cwd. It now has a backstop + // entry, and (like code_search) is not exempt, so an out-of-project cwd is + // hard-blocked. + const outsideRoot = fs.mkdtempSync( + path.join(os.tmpdir(), 'ffmc-outside-project-'), + ) + fs.writeFileSync(path.join(outsideRoot, 'notes.txt'), 'notes\n') + // The registry stays closed: nothing here is allowlisted. + resetExternalReadRootsForTesting() + + const chunks: unknown[] = [] + runAgentStepBaseParams = { + ...runAgentStepBaseParams, + onResponseChunk: (chunk) => chunks.push(chunk), + } + runAgentStepBaseParams.promptAiSdkStream = async function* ({}) { + yield createToolCallChunk('find_files_matching_content', { + pattern: 'apiKey', + cwd: outsideRoot, + }) + yield createToolCallChunk('end_turn', {}) + return promptSuccess('mock-message-id') + } + + const sessionState = getInitialSessionState(mockFileContext) + const agentState = sessionState.mainAgentState + const unscopedAgent: AgentTemplate = { + ...testAgent, + id: 'unscoped-agent', + toolNames: ['find_files_matching_content', 'end_turn'], + filesystemScope: undefined, + } + + try { + await runAgentStep({ + ...runAgentStepBaseParams, + agentType: 'unscoped-agent', + localAgentTemplates: { 'unscoped-agent': unscopedAgent }, + agentTemplate: unscopedAgent, + agentState, + prompt: 'Search content outside the project root', + }) + + expect(chunks).toContainEqual( + expect.objectContaining({ + type: 'error', + message: expect.stringContaining( + 'was blocked by the unscoped-agent filesystem read scope', + ), + }), + ) + expect(chunks).not.toContainEqual( + expect.objectContaining({ + type: 'tool_call', + toolName: 'find_files_matching_content', + }), + ) + } finally { + resetExternalReadRootsForTesting() + fs.rmSync(outsideRoot, { recursive: true, force: true }) + } + }) + it('blocks suggest_followups after same-step rewrite_symbol edits when the gate started open', async () => { const chunks: unknown[] = [] runAgentStepBaseParams = { @@ -2141,6 +2729,7 @@ describe('runAgentStep - set_output tool', () => { message: expect.stringContaining( 'Tool `suggest_followups` is not available yet', ), + autoRecovering: true, }), ) expect(chunks).not.toContainEqual( @@ -2202,6 +2791,7 @@ describe('runAgentStep - set_output tool', () => { message: expect.stringMatching( /No tools are available after suggest_followups|suggest_followups already ended the actionable work/, ), + autoRecovering: true, }), ) expect(chunks).not.toContainEqual( @@ -2260,6 +2850,7 @@ describe('runAgentStep - set_output tool', () => { message: expect.stringMatching( /No tools are available after suggest_followups|suggest_followups already ended the actionable work/, ), + autoRecovering: true, }), ) expect(chunks).not.toContainEqual( @@ -2319,6 +2910,7 @@ describe('runAgentStep - set_output tool', () => { message: expect.stringMatching( /No tools are available after suggest_followups|suggest_followups already ended the actionable work/, ), + autoRecovering: true, }), ) expect(chunks).not.toContainEqual( @@ -2381,6 +2973,7 @@ describe('runAgentStep - set_output tool', () => { message: expect.stringMatching( /No tools are available after suggest_followups|suggest_followups already ended the actionable work/, ), + autoRecovering: true, }), ) expect(chunks).not.toContainEqual( @@ -2483,6 +3076,7 @@ describe('runAgentStep - set_output tool', () => { message: expect.stringMatching( /No tools are available after suggest_followups|suggest_followups already ended the actionable work/, ), + autoRecovering: true, }), ) expect(chunks).not.toContainEqual( @@ -2592,6 +3186,7 @@ describe('runAgentStep - set_output tool', () => { message: expect.stringMatching( /No tools are available after suggest_followups|suggest_followups already ended the actionable work/, ), + autoRecovering: true, }), ) expect(chunks).not.toContainEqual( @@ -3014,3 +3609,195 @@ describe('runAgentStep - set_output tool', () => { ) }) }) + +describe('processStream queued custom/MCP tool tool_start', () => { + const customToolName = 'custom_queued_write' + const queuedFileContext: ProjectFileContext = { + ...sharedMockFileContext, + customToolDefinitions: { + [customToolName]: { + inputSchema: { + type: 'object', + properties: { + target: { type: 'string' }, + }, + required: ['target'], + additionalProperties: false, + }, + endsAgentStep: false, + description: 'Custom tool used to pin the queued tool_start branch', + }, + }, + } + const customToolAgent: AgentTemplate = { + id: 'queued-custom-tool-agent', + displayName: 'Queued Custom Tool Agent', + spawnerPrompt: 'Drives a queued custom tool through processStream', + model: 'claude-3-5-sonnet-20241022', + inputSchema: {}, + outputMode: 'last_message' as const, + includeMessageHistory: true, + inheritParentSystemPrompt: false, + mcpServers: {}, + toolNames: ['write_file', customToolName, 'end_turn'], + spawnableAgents: [], + systemPrompt: 'Test system prompt', + instructionsPrompt: 'Test instructions prompt', + stepPrompt: 'Test step prompt', + } + + type ToolEvent = { + type?: string + toolName?: string + toolCallId?: string + queued?: boolean + } + const asToolEvent = (chunk: unknown): ToolEvent => chunk as ToolEvent + + it('emits tool_start for a custom/MCP tool queued behind an in-flight write (RF-1)', async () => { + // RF-1 reachability, pinned at the RUNTIME level: a custom/MCP tool has no + // statically determinable target path, so it serializes behind every + // outstanding write barrier. While a prior named-path write_file is still + // in flight, the custom tool is dispatched with `queued === true`, which is + // exactly the branch in executeCustomToolCall that emits `tool_start` once + // that barrier resolves. Deleting that emission makes this test fail. + const writePath = 'queued-custom-write.txt' + const chunks: unknown[] = [] + + let releaseWrite!: () => void + const writeGate = new Promise((resolve) => { + releaseWrite = resolve + }) + let writeStarted!: () => void + const writeStart = new Promise((resolve) => { + writeStarted = resolve + }) + let customCallObserved!: () => void + const customCall = new Promise((resolve) => { + customCallObserved = resolve + }) + + const agentRuntimeImpl: AgentRuntimeDeps & AgentRuntimeScopedDeps = { + ...TEST_AGENT_RUNTIME_IMPL, + sendAction: () => {}, + requestFiles: async () => buildReadFilesResultV1([]), + requestOptionalFile: async () => null, + requestToolCall: async (toolCallParams) => { + if (toolCallParams.toolName === 'write_file') { + // Hold the per-path write barrier open so the following custom tool + // is dispatched while that write is still in flight. + writeStarted() + await writeGate + return { output: [] } + } + return { output: [{ type: 'json', value: { ok: true } }] } + }, + } + + const sessionState = getInitialSessionState(queuedFileContext) + // Pre-authorize the write path so write_file does not need a separate read. + sessionState.mainAgentState.readAuthorizationsByPath = { + [writePath]: true, + } + + const stream = createMockStreamWithToolCalls([ + { + toolName: 'write_file', + input: { + path: writePath, + instructions: 'hold the per-path write barrier', + content: 'first write', + }, + }, + { toolName: customToolName, input: { target: 'queued-custom-input' } }, + { toolName: 'end_turn', input: {} }, + ]) + + const processing = processStream({ + ...agentRuntimeImpl, + agentContext: {}, + agentState: sessionState.mainAgentState, + agentStepId: 'queued-custom-step-id', + agentTemplate: customToolAgent, + ancestorRunIds: [], + clientSessionId: 'test-session', + fileContext: queuedFileContext, + fingerprintId: 'test-fingerprint', + fullResponse: '', + localAgentTemplates: { [customToolAgent.id]: customToolAgent }, + messages: [], + prompt: 'Run a custom tool behind an in-flight write', + repoId: undefined, + repoUrl: undefined, + runId: 'test-run-id', + signal: new AbortController().signal, + stream, + system: 'test system', + tools: {}, + userId: TEST_USER_ID, + userInputId: 'test-input-id', + onCostCalculated: async () => {}, + onResponseChunk: (chunk) => { + chunks.push(chunk) + const event = asToolEvent(chunk) + if (event.type === 'tool_call' && event.toolName === customToolName) { + customCallObserved() + } + }, + }) + + await writeStart + await customCall + + // The custom tool's tool_call is published immediately and carries the + // runtime `queued` signal, because the prior write still holds a barrier. + const customCallChunk = chunks + .map(asToolEvent) + .find( + (event) => + event.type === 'tool_call' && event.toolName === customToolName, + ) + expect(customCallChunk).toBeDefined() + expect(customCallChunk!.queued).toBe(true) + const customToolCallId = customCallChunk!.toolCallId + expect(typeof customToolCallId).toBe('string') + + // No queued→running transition has fired yet for any call: every queued + // tool in this step is still waiting on the gated write. + expect( + chunks.map(asToolEvent).some((event) => event.type === 'tool_start'), + ).toBe(false) + + releaseWrite() + await processing + + // Once the write barrier resolves, executeCustomToolCall emits tool_start + // for the custom tool's own call id, ordered after its tool_call and before + // its tool_result. + const startIdx = chunks.findIndex((chunk) => { + const event = asToolEvent(chunk) + return ( + event.type === 'tool_start' && event.toolCallId === customToolCallId + ) + }) + expect(startIdx).toBeGreaterThan(-1) + expect(chunks[startIdx]).toMatchObject({ + type: 'tool_start', + toolCallId: customToolCallId, + }) + + const callIdx = chunks.findIndex((chunk) => { + const event = asToolEvent(chunk) + return event.type === 'tool_call' && event.toolCallId === customToolCallId + }) + const resultIdx = chunks.findIndex((chunk) => { + const event = asToolEvent(chunk) + return ( + event.type === 'tool_result' && event.toolCallId === customToolCallId + ) + }) + expect(callIdx).toBeGreaterThan(-1) + expect(callIdx).toBeLessThan(startIdx) + expect(resultIdx).toBeGreaterThan(startIdx) + }) +}) diff --git a/packages/agent-runtime/src/tools/tool-executor.ts b/packages/agent-runtime/src/tools/tool-executor.ts index c5851687b2..46ae358575 100644 --- a/packages/agent-runtime/src/tools/tool-executor.ts +++ b/packages/agent-runtime/src/tools/tool-executor.ts @@ -24,6 +24,10 @@ import { } from '@codebuff/common/tools/metadata' import { isAbortError } from '@codebuff/common/util/error' import { jsonToolResult } from '@codebuff/common/util/messages' +import { + isExternalReadPath, + isOwnedTempPath, +} from '@codebuff/common/util/project-path-containment' import { generateCompactId } from '@codebuff/common/util/string' import { cloneDeep } from 'lodash' import z from 'zod/v4' @@ -1499,9 +1503,26 @@ function isFileChangingTool(toolName: string): boolean { } const POST_FOLLOWUPS_ERROR_MESSAGE = - 'No tools are available after suggest_followups in the same step (except end_turn/task_completed). suggest_followups must be the absolute last actionable tool after the completion summary (and after git-committer if committing).' + 'No tools are available after suggest_followups in the same step (except end_turn/task_completed). suggest_followups is the FINAL output of the turn: emit it only after your user-visible completion summary (and after git-committer if committing), then emit nothing further. Reorder so suggest_followups is your last tool call.' const ALREADY_EMITTED_FOLLOWUPS_ERROR_MESSAGE = - 'suggest_followups already ended the actionable work for this turn. No more non-terminal tools are available after followups (except end_turn/task_completed).' + 'suggest_followups already ended the actionable work for this turn. It is the FINAL output of the turn, so no further non-terminal tools may run (only end_turn/task_completed). End the turn now instead of calling more tools.' +// Concise, calm summary for the followups ordering/gate rejections. These are +// agent-facing control-flow diagnostics the model corrects on its own, so the +// CLI suppresses the visible banner (see `autoRecovering` in +// `common/src/types/print-mode.ts`); the full `message` still reaches the model. +const FOLLOWUPS_ORDERING_USER_MESSAGE = + 'The model called suggest_followups out of order and is correcting the ordering automatically. No action is needed.' +// Concise, calm summary for the pre-gate git-committer withhold. Like the +// followups ordering rejections, this is normal harness ordering the model +// resolves by ending its turn, so the CLI suppresses the visible banner while +// the full `message` still reaches the model. +const GIT_COMMITTER_WITHHELD_USER_MESSAGE = + 'Commit deferred until the validation/reviewer gate passes. No action is needed.' +// Single source for the malformed-tool-call user summary shared by the native +// (`executeToolCall`) and custom/MCP (`executeCustomToolCall`) parse-failure +// paths, so the two stay in sync. +const malformedToolCallUserMessage = (toolName: string): string => + `The model sent a malformed \`${toolName}\` tool call and is correcting it automatically. No action is needed.` function isTerminalFollowupCompanion(name: string): boolean { return ( @@ -1596,6 +1617,20 @@ export function getFilesystemToolPaths( return { access: 'read', paths: strings(input.path) } } if (toolName === 'glob' || toolName === 'code_search') { + return { + access: 'read', + paths: [ + ...strings(input.cwd ?? '.'), + // code_search additionally accepts an explicit `paths` list whose + // entries may be absolute, so those must be backstopped too. `glob` + // has no `paths` input, so its behavior is unchanged. + ...(toolName === 'code_search' ? strings(input.paths) : []), + ], + } + } + if (toolName === 'find_files_matching_content') { + // Previously fell through and returned undefined, so this tool got NO + // backstop at all while still resolving an arbitrary absolute cwd. return { access: 'read', paths: strings(input.cwd ?? '.') } } if (toolName === 'edit_transaction') { @@ -1680,6 +1715,25 @@ function normalizedEscapesProject(normalized: string): boolean { ) } +// Tools whose SDK handler is the authoritative containment layer for the +// owned-temp / allowlisted-external READ relaxation in executeToolCall. +// +// INVARIANT: a tool belongs here ONLY if its SDK handler resolves every +// caller-supplied path through `resolveFilePathForRead*Operation` (the +// read-only containment resolvers in `sdk/src/tools/path-utils.ts`). This +// backstop is deliberately NOT the authoritative containment layer — it defers +// to the handler — so exempting a tool whose handler does not contain removes +// the only check that exists for it. `code_search` and +// `find_files_matching_content` are deliberately absent: they resolve an +// arbitrary caller `cwd` and spawn ripgrep there with no containment +// resolution at all. +const EXTERNAL_READ_EXEMPT_TOOLS = new Set([ + 'read_files', + 'read_logs', + 'read_image', + 'list_directory', +]) + const MAX_CUSTOM_INPUT_SCAN_DEPTH = 6 const MAX_CUSTOM_INPUT_SCAN_STRINGS = 1000 @@ -1966,7 +2020,7 @@ export async function executeToolCall( type: 'error', message: `${toolCall.error}\n\n${inputLabel}:\n${formattedInput}`, - userMessage: `The model sent a malformed \`${toolName}\` tool call and is correcting it automatically. No action is needed.`, + userMessage: malformedToolCallUserMessage(toolName), autoRecovering: true, }) logger.debug( @@ -2007,19 +2061,61 @@ export async function executeToolCall( // lexical scope for missing paths so create operations still work. } } + // READ-ONLY owned-temp and external-read exceptions. The SDK + // deliberately permits reads under the openbuff-owned OS temp namespace + // (see read-files.ts `authorizeReadTarget` and read-logs.ts): that is how + // a parent agent reads back tmux capture evidence and background-job + // logs. It equally permits reads strictly inside a root the user + // explicitly allowlisted (the openbuff config directory for logs/state, + // plus any `readableRoots` entry in openbuff.json). This backstop has no + // notion of either namespace, so without the exceptions it refuses reads + // the SDK is designed to allow. + // + // The SDK read handlers of the EXEMPT tools (see + // EXTERNAL_READ_EXEMPT_TOOLS) remain AUTHORITATIVE for both: they run + // the real containment resolution (symlink dereferencing, + // strictly-inside checks, and the fail-closed mandatory-sensitive + // refusal that keeps `credentials.json` unreadable inside an allowlisted + // config root). This layer only stops pre-dispatch refusal of paths + // those handlers will validate themselves, so a tool whose handler does + // NOT contain (e.g. code_search) is never exempted. + // + // Access-scoped AND tool-scoped. Access: a WRITE to an owned-temp or + // allowlisted-external path keeps hard-blocking here. The + // (narrower) owned-temp mutation policy is owned by the SDK's + // filesystem-authority.ts `ownedTempMutationRefusal` — tmux captures are + // verification evidence a subagent must not be able to forge — and the + // external allowlist is READ-only by construction (there is no + // `external-write` scope), so this layer must not pre-authorize any + // mutation of either. + // + // Both predicates get the RAW caller path: each resolves its own input + // and refuses any raw `..` segment itself, which is exactly the guard we + // want. The project-relative `normalized` form would be a meaningless + // `../..`-style string here. + const externalReadAllowed = + filesystemAccess.access === 'read' && + EXTERNAL_READ_EXEMPT_TOOLS.has(toolName) && + (isOwnedTempPath(rawPath) || isExternalReadPath(rawPath)) // A path "escapes" the project when it traverses above the root or is // absolute (either lexically or after canonicalization). Escapes are the // real containment boundary: an agent must never read or write outside - // the project, so these are always hard-blocked regardless of access. + // the project, so these are always hard-blocked regardless of access — + // except for the owned-temp / allowlisted-external reads above. const escapesProject = - normalizedEscapesProject(normalized) || - normalizedEscapesProject(canonical) + !externalReadAllowed && + (normalizedEscapesProject(normalized) || + normalizedEscapesProject(canonical)) // An in-project path is a scope mismatch when it stays inside the project // but does not match the agent's declared filesystemScope patterns. Only - // meaningful when the agent declared a scope for this access type. + // meaningful when the agent declared a scope for this access type. An + // owned-temp or allowlisted-external read is not in-project, so it is + // never pattern-matched against filesystemScope globs: it is neither + // hard-blocked above nor spuriously warned about below. const scopeMismatch = allowedPatterns !== undefined && !escapesProject && + !externalReadAllowed && !allowedPatterns.some( (pattern) => scopePatternMatches(normalized, pattern) && @@ -2090,6 +2186,8 @@ export async function executeToolCall( onResponseChunk({ type: 'error', message: postFollowupsBlockReason, + userMessage: FOLLOWUPS_ORDERING_USER_MESSAGE, + autoRecovering: true, }) return abortablePreviousToolCallFinished } @@ -2102,7 +2200,9 @@ export async function executeToolCall( onResponseChunk({ type: 'error', message: - 'Tool `suggest_followups` is not available yet. GATE: PENDING (or final summary not written). End your turn so the runtime gate can clear; call this only after GATE: PASSED and a user-visible completion summary.', + 'Tool `suggest_followups` is not available yet. GATE: PENDING (or final summary not written). End your turn so the runtime gate can clear. Call it only after GATE: PASSED, and only as the FINAL output of that turn: user-visible completion summary first, then git-committer if committing, then suggest_followups with nothing after it.', + userMessage: FOLLOWUPS_ORDERING_USER_MESSAGE, + autoRecovering: true, }) return abortablePreviousToolCallFinished } @@ -2169,6 +2269,8 @@ export async function executeToolCall( type: 'error', message: 'git-committer withheld: GATE: PENDING (need GATE: PASSED / phase=final_response_allowed). End your turn; do not retry or predict gate progress. Spawn git-committer once after GATE: PASSED.', + userMessage: GIT_COMMITTER_WITHHELD_USER_MESSAGE, + autoRecovering: true, }) if (filteredAgents.length === 0) { return abortablePreviousToolCallFinished @@ -3059,6 +3161,8 @@ export async function executeCustomToolCall( onResponseChunk({ type: 'error', message: postFollowupsBlockReason, + userMessage: FOLLOWUPS_ORDERING_USER_MESSAGE, + autoRecovering: true, }) return abortablePreviousToolCallFinished } @@ -3112,7 +3216,7 @@ export async function executeCustomToolCall( onResponseChunk({ type: 'error', message: `${toolCall.error}\n\n${inputLabel}:\n${formattedInput}`, - userMessage: `The model sent a malformed \`${toolName}\` tool call and is correcting it automatically. No action is needed.`, + userMessage: malformedToolCallUserMessage(toolName), autoRecovering: true, }) logger.debug( @@ -3169,14 +3273,16 @@ export async function executeCustomToolCall( // `previousToolCallFinished` here (the handler still awaits it internally). // // Reachability (RF-1): `queued` is threaded through `ExecuteToolCallParams` - // for any serialized same-path write, and custom/MCP tool paths can be - // queued when a per-path write barrier applies to a custom/unknown-path - // input — so this branch is genuinely reachable, not dead defensive code. - // It is rarer than the native write_file/edit_transaction path because most - // custom tools do not touch the project filesystem and therefore never hit - // the write barrier, but the runtime does not restrict `queued` to native - // tools. The downstream CLI flip is covered by the queued-block tool_start - // tests in sdk-event-handlers.test.ts (including the nested-agent case). + // for any serialized same-path write, and a custom/MCP tool has no statically + // determinable target path, so stream-parser.ts marks it `queued` whenever an + // outstanding write barrier or in-flight read exists — this branch is + // genuinely reachable, not dead defensive code. Pinned at the runtime level + // by 'emits tool_start for a custom/MCP tool queued behind an in-flight write + // (RF-1)' in __tests__/run-agent-step-tools.test.ts, which drives a custom + // tool through processStream behind a gated write_file and asserts the + // tool_start chunk for that call id. The downstream CLI flip is covered + // separately by the queued-block tool_start tests in + // sdk-event-handlers.test.ts (including the nested-agent case). if (queued === true) { abortablePreviousToolCallFinished.then( () => { diff --git a/sdk/src/__tests__/model-provider.test.ts b/sdk/src/__tests__/model-provider.test.ts index 206c1114bf..6e9e732152 100644 --- a/sdk/src/__tests__/model-provider.test.ts +++ b/sdk/src/__tests__/model-provider.test.ts @@ -19,6 +19,7 @@ import { import { PROVIDER_CONFIG_ENV_VAR, OPENBUFF_PROVIDER_PRESETS, + clearProviderConfigCacheForTest, createProviderPresetConfig, formatModelCapabilitiesSummary, getAncestorProviderConfigPaths, @@ -42,6 +43,7 @@ import { setModelDiscoveryCachePathForTest, } from '../model-discovery' import type { ModelDiscoveryFetch } from '../model-discovery' +import { selectTrustedReadableRoots } from '../run' const originalEnv = { ...process.env } const originalCwd = process.cwd() @@ -131,6 +133,26 @@ describe('model-provider', () => { } }) + test('defaults readableRoots to an empty array and flows entries through the transform', () => { + // `.default([])` means downstream code (the run-start external read + // registry wiring) never has to handle `undefined`. + expect(providerConfigFileSchema.parse({}).readableRoots).toEqual([]) + + const parsed = providerConfigFileSchema.parse({ + readableRoots: ['/opt/notes', 'relative/notes'], + }) + // The schema keeps entries verbatim; run.ts is what drops relative ones. + expect(parsed.readableRoots).toEqual(['/opt/notes', 'relative/notes']) + + expect( + providerConfigFileSchema.safeParse({ readableRoots: [''] }).success, + ).toBe(false) + expect( + providerConfigFileSchema.safeParse({ readableRoots: '/opt/notes' }) + .success, + ).toBe(false) + }) + test('rejects zero as an ambiguous maxAgentSteps value', () => { expect( providerConfigFileSchema.safeParse({ maxAgentSteps: 0 }).success, @@ -412,6 +434,7 @@ describe('model-provider', () => { semantic: { enabled: false }, }, fileChangeHooks: [], + readableRoots: [], providers: { 'opencode-go': { type: 'openai-compatible', @@ -558,6 +581,7 @@ describe('model-provider', () => { semantic: { enabled: false }, }, fileChangeHooks: [], + readableRoots: [], providers: { custom: { type: 'openai-compatible', @@ -606,6 +630,7 @@ describe('model-provider', () => { semantic: { enabled: false }, }, fileChangeHooks: [], + readableRoots: [], providers: {}, }, } @@ -650,6 +675,7 @@ describe('model-provider', () => { semantic: { enabled: false }, }, fileChangeHooks: [], + readableRoots: [], providers: {}, }, } @@ -687,6 +713,7 @@ describe('model-provider', () => { semantic: { enabled: false }, }, fileChangeHooks: [], + readableRoots: [], providers: { 'opencode-go': { type: 'openai-compatible', @@ -833,6 +860,7 @@ describe('model-provider', () => { semantic: { enabled: false }, }, fileChangeHooks: [], + readableRoots: [], providers: { freemodel: { type: 'anthropic-compatible', @@ -879,6 +907,7 @@ describe('model-provider', () => { semantic: { enabled: false }, }, fileChangeHooks: [], + readableRoots: [], providers: { freemodel: { type: 'anthropic-compatible', @@ -2788,3 +2817,168 @@ describe('getAncestorProviderConfigPaths — bounded ancestor walk (C1.3)', () = expect(path.dirname(lastDir)).toBe(lastDir) }) }) + +describe('readableRoots provenance (ER-1)', () => { + const tempDirs: string[] = [] + + const makeTempDir = (prefix: string) => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)) + tempDirs.push(dir) + return dir + } + + beforeEach(() => { + resetEnv() + delete process.env[PROVIDER_CONFIG_ENV_VAR] + clearProviderConfigCacheForTest() + }) + + afterEach(() => { + process.chdir(originalCwd) + resetEnv() + clearProviderConfigCacheForTest() + for (const dir of tempDirs.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + test('records the config-dir file that declared a readable root', () => { + const configDir = makeTempDir('openbuff-config-dir-') + process.env.OPENBUFF_CONFIG_DIR = configDir + const configFile = path.join(configDir, 'openbuff.json') + fs.writeFileSync( + configFile, + JSON.stringify({ readableRoots: ['/opt/global-notes'] }), + ) + process.chdir(makeTempDir('openbuff-project-')) + + const loaded = loadProviderConfigSync() + + expect(loaded.config.readableRoots).toEqual(['/opt/global-notes']) + // The trust gate in run.ts registers this root unconditionally precisely + // because its source file lives inside the openbuff config directory. + expect(loaded.readableRootsSources?.[path.resolve('/opt/global-notes')]).toBe( + configFile, + ) + }) + + test('records the project openbuff.json that declared a readable root', () => { + // Config dir exists but declares nothing, so the only readableRoots value + // comes from the project file a cloned repository can ship. + process.env.OPENBUFF_CONFIG_DIR = makeTempDir('openbuff-config-dir-') + process.chdir(makeTempDir('openbuff-project-')) + // Read back through process.cwd(): the config walk starts there, and on + // macOS the temp dir is reached through a symlink. + const projectDir = process.cwd() + const projectConfigFile = path.join(projectDir, 'openbuff.json') + fs.writeFileSync( + projectConfigFile, + JSON.stringify({ readableRoots: ['/opt/project-notes'] }), + ) + + const loaded = loadProviderConfigSync() + + expect(loaded.config.readableRoots).toEqual(['/opt/project-notes']) + // Untrusted provenance: run.ts drops this root unless + // OPENBUFF_TRUST_PROJECT_READABLE_ROOTS=1. + expect( + loaded.readableRootsSources?.[path.resolve('/opt/project-notes')], + ).toBe(projectConfigFile) + }) +}) + +describe('selectTrustedReadableRoots (ER-1 trust gate)', () => { + // `readableRoots` is the only config key that grants filesystem authority, so + // this gate decides whether a cloned repository can allowlist credential + // directories like `~/.config/gh` or `~/.docker`. Exercised directly on the + // pure helper `run.ts` uses, with provenance shaped exactly as + // `loadProviderConfigSync` records it above. + const configDir = path.resolve('/home/user/.config/openbuff') + const configDirFile = path.join(configDir, 'openbuff.json') + const projectFile = path.resolve('/repo/openbuff.json') + const globalRoot = path.resolve('/opt/global-notes') + const projectRoot = path.resolve('/opt/project-notes') + + test('registers a root declared by a config file inside the config dir', () => { + expect( + selectTrustedReadableRoots({ + roots: [globalRoot], + sources: { [globalRoot]: configDirFile }, + configDir, + trustProjectRoots: false, + }), + ).toEqual({ trusted: [globalRoot], untrustedCount: 0 }) + }) + + test('drops a root declared by a project openbuff.json', () => { + // The fail-closed case: a repository ships `readableRoots` and the project + // value wins the config merge, so without this gate a clone could expose + // credential directories outside the project. + expect( + selectTrustedReadableRoots({ + roots: [projectRoot], + sources: { [projectRoot]: projectFile }, + configDir, + trustProjectRoots: false, + }), + ).toEqual({ trusted: [], untrustedCount: 1 }) + }) + + test('registers a project-declared root with the explicit opt-in', () => { + // `run.ts` derives `trustProjectRoots` from + // OPENBUFF_TRUST_PROJECT_READABLE_ROOTS=1. + expect( + selectTrustedReadableRoots({ + roots: [projectRoot], + sources: { [projectRoot]: projectFile }, + configDir, + trustProjectRoots: true, + }), + ).toEqual({ trusted: [projectRoot], untrustedCount: 0 }) + }) + + test('drops a root with no recorded provenance', () => { + // Provenance is optional metadata, so "unknown source" must fail CLOSED + // rather than be treated as config-dir owned. + expect( + selectTrustedReadableRoots({ + roots: [globalRoot], + sources: {}, + configDir, + trustProjectRoots: false, + }), + ).toEqual({ trusted: [], untrustedCount: 1 }) + }) + + test('drops a relative entry without counting it as untrusted', () => { + // A relative entry is never anchored to process.cwd(): the declaring config + // may be a global file loaded from an unrelated directory. Nothing was + // refused on trust grounds, so it does not inflate the warning count. + expect( + selectTrustedReadableRoots({ + roots: ['notes', globalRoot], + sources: { + [path.resolve('notes')]: configDirFile, + [globalRoot]: configDirFile, + }, + configDir, + trustProjectRoots: false, + }), + ).toEqual({ trusted: [globalRoot], untrustedCount: 0 }) + }) + + test('keeps a sibling directory of the config dir untrusted', () => { + // `path.relative`-based containment: `-evil` shares the prefix + // but is not inside the config dir, so a naive startsWith would trust it. + expect( + selectTrustedReadableRoots({ + roots: [globalRoot], + sources: { + [globalRoot]: path.join(`${configDir}-evil`, 'openbuff.json'), + }, + configDir, + trustProjectRoots: false, + }), + ).toEqual({ trusted: [], untrustedCount: 1 }) + }) +}) diff --git a/sdk/src/__tests__/path-utils.test.ts b/sdk/src/__tests__/path-utils.test.ts index 8f2d67b3ce..4e0c5bad8d 100644 --- a/sdk/src/__tests__/path-utils.test.ts +++ b/sdk/src/__tests__/path-utils.test.ts @@ -3,11 +3,19 @@ import os from 'os' import path from 'path' import { afterEach, beforeEach, describe, expect, test } from 'bun:test' +import { + configureExternalReadRoots, + resetExternalReadRootsForTesting, +} from '@codebuff/common/util/project-path-containment' + import { getProjectPathLookupKeys, + getScopedReadPolicyAliases, isSafeProjectRelativePath, resolveFilePathForFileSystemOperation, + resolveFilePathForFileSystemReadOperation, resolveFilePathForOperation, + resolveFilePathForReadOperation, resolveFilePathWithinProject, } from '../tools/path-utils' @@ -182,3 +190,111 @@ test('filesystem operations resolve symlinks through the injected filesystem', a ), ).resolves.toBeNull() }) + +describe('read-only operation resolvers', () => { + let projectDir: string + let externalRoot: string + let externalFile: string + + /** Host-realpath-backed adapter for the async read resolver. */ + const hostFileSystem = { + realpath: async (input: string) => fs.realpathSync(input), + } as unknown as CodebuffFileSystem + + beforeEach(() => { + resetExternalReadRootsForTesting() + projectDir = fs.mkdtempSync(path.join(os.tmpdir(), 'read-resolver-proj-')) + externalRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'read-resolver-ext-')) + externalFile = path.join(externalRoot, 'notes.txt') + fs.writeFileSync(externalFile, 'notes\n') + fs.writeFileSync( + path.join(projectDir, 'in-project.ts'), + 'export const a = 1\n', + ) + }) + + afterEach(() => { + // The registry is module state: reset unconditionally so no later test + // file inherits an open external read boundary. + resetExternalReadRootsForTesting() + fs.rmSync(projectDir, { recursive: true, force: true }) + fs.rmSync(externalRoot, { recursive: true, force: true }) + }) + + test('an in-project path resolves identically to the write-path resolver', () => { + configureExternalReadRoots([externalRoot]) + + expect(resolveFilePathForReadOperation(projectDir, 'in-project.ts')).toEqual( + resolveFilePathForOperation(projectDir, 'in-project.ts'), + ) + }) + + test('an allowlisted external file resolves with scope external-read', () => { + configureExternalReadRoots([externalRoot]) + + const resolved = resolveFilePathForReadOperation(projectDir, externalFile) + expect(resolved).not.toBeNull() + expect(resolved!.scope).toBe('external-read') + expect(resolved!.operationPath).toBe(resolved!.realFullPath) + // Outside the project, so `relativePath` is the absolute resolved path. + expect(resolved!.relativePath).toBe(path.resolve(externalFile)) + + // The write twin stays blind to the read allowlist. + expect(resolveFilePathForOperation(projectDir, externalFile)).toBeNull() + }) + + test('the async read resolver agrees with the sync one', async () => { + configureExternalReadRoots([externalRoot]) + + const resolved = await resolveFilePathForFileSystemReadOperation( + projectDir, + externalFile, + hostFileSystem, + ) + expect(resolved).not.toBeNull() + expect(resolved!.scope).toBe('external-read') + expect(resolved!.operationPath).toBe(resolved!.realFullPath) + + // The write twin refuses it through the same injected filesystem. + await expect( + resolveFilePathForFileSystemOperation( + projectDir, + externalFile, + hostFileSystem, + ), + ).resolves.toBeNull() + }) + + test('refuses the external file while the registry is unconfigured', () => { + expect(resolveFilePathForReadOperation(projectDir, externalFile)).toBeNull() + }) +}) + +describe('getScopedReadPolicyAliases', () => { + test('returns no extra aliases for a project-scoped path', () => { + // The project-relative path is already the key a host policy targets, so + // adding a bare basename alias there would widen host filters. + expect(getScopedReadPolicyAliases('project', 'src/notes.png')).toEqual([]) + }) + + test('builds basename and / keys for non-project scopes', () => { + // The absolute relativePath of an external-read/owned-temp resolution never + // matches a project-relative glob, so these are the keys a host filter can + // actually target. Same alias shape read-files.ts builds. + expect( + getScopedReadPolicyAliases('external-read', '/external-root/notes.png'), + ).toEqual(['notes.png', 'external-read/notes.png']) + expect( + getScopedReadPolicyAliases('owned-temp', '/tmp/openbuff-x/job.log'), + ).toEqual(['job.log', 'owned-temp/job.log']) + }) + + test('normalizes backslash separators before taking the basename', () => { + // On a POSIX host a backslash is a legal filename character, so the + // normalization is what keeps the alias a bare basename rather than a + // whole path fragment. + expect( + getScopedReadPolicyAliases('external-read', '/external-root\\notes.png'), + ).toEqual(['notes.png', 'external-read/notes.png']) + }) +}) diff --git a/sdk/src/__tests__/read-files.test.ts b/sdk/src/__tests__/read-files.test.ts index e1a3eb7676..0755aeeb10 100644 --- a/sdk/src/__tests__/read-files.test.ts +++ b/sdk/src/__tests__/read-files.test.ts @@ -1,6 +1,10 @@ import * as projectFileTree from '@codebuff/common/project-file-tree' import { createNodeError } from '@codebuff/common/testing/errors' -import { getOwnedTempRoots } from '@codebuff/common/util/project-path-containment' +import { + configureExternalReadRoots, + getOwnedTempRoots, + resetExternalReadRootsForTesting, +} from '@codebuff/common/util/project-path-containment' import { decodeReadCapabilityToken, getContentHash, @@ -1053,3 +1057,83 @@ describe('getFilesStructured', () => { }) }) }) + +describe('getFilesStructured — allowlisted external read roots', () => { + // Synthetic absolute root: every filesystem call in these cases goes through + // the mock filesystem, and the name deliberately avoids the `openbuff-` + // owned-temp patterns so an allow here can only come from the external read + // allowlist. + const externalRoot = path.resolve('/external-read-root') + const externalFile = path.join(externalRoot, 'notes.txt') + const externalCredentials = path.join(externalRoot, 'credentials.json') + + beforeEach(() => { + resetExternalReadRootsForTesting() + configureExternalReadRoots([externalRoot]) + }) + + afterEach(() => { + // The registry is module state: reset unconditionally so no later test + // inherits an open external read boundary. + resetExternalReadRootsForTesting() + }) + + test('reads a file inside an allowlisted root', async () => { + const result = await getFilesStructured({ + filePaths: [externalFile], + cwd: '/project', + fs: createMockFs({ files: { [externalFile]: { content: 'notes\n' } } }), + }) + + expect(result.results[0]).toMatchObject({ + selector: 'file', + status: 'ok', + complete: true, + content: 'notes\n', + }) + }) + + test('refuses credentials.json inside an allowlisted root', async () => { + const result = await getFilesStructured({ + filePaths: [externalCredentials], + cwd: '/project', + fs: createMockFs({ + files: { [externalCredentials]: { content: '{"apiKey":"x"}\n' } }, + }), + // An allow-everything host filter proves the refusal comes from the + // resolver's fail-closed mandatory-sensitive check (which returns no + // resolution at all, hence `outside_project`), not from host policy. + fileFilter: () => ({ status: 'allow' }), + }) + + expect(result.results[0]).toMatchObject({ + status: 'error', + error: { code: 'outside_project' }, + }) + }) + + test('applies a host fileFilter to the external-read policy alias', async () => { + // external-read results carry an ABSOLUTE relativePath, exactly like + // owned-temp, so without the alias a host filter written against + // project-relative globs would silently stop applying — a fail-open. + const blockedAliases: string[] = [] + const result = await getFilesStructured({ + filePaths: [externalFile], + cwd: '/project', + fs: createMockFs({ files: { [externalFile]: { content: 'notes\n' } } }), + fileFilter: (candidate) => { + if (candidate === 'external-read/notes.txt') { + blockedAliases.push(candidate) + return { status: 'blocked' } + } + return { status: 'allow' } + }, + }) + + expect(blockedAliases).toEqual(['external-read/notes.txt']) + expect(result.results[0]).toMatchObject({ + status: 'error', + error: { code: 'blocked' }, + }) + }) +}) diff --git a/sdk/src/__tests__/read-image.test.ts b/sdk/src/__tests__/read-image.test.ts index 204c6a24b7..33fd3c5b96 100644 --- a/sdk/src/__tests__/read-image.test.ts +++ b/sdk/src/__tests__/read-image.test.ts @@ -4,7 +4,11 @@ import * as path from 'path' import { FILE_READ_STATUS } from '@codebuff/common/old-constants' import { createNodeError } from '@codebuff/common/testing/errors' -import { describe, expect, test } from 'bun:test' +import { + configureExternalReadRoots, + resetExternalReadRootsForTesting, +} from '@codebuff/common/util/project-path-containment' +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' import { readImages } from '../tools/read-image' @@ -176,3 +180,232 @@ describe('readImages', () => { }) }) }) + +describe('readImages — allowlisted external read roots', () => { + // Synthetic absolute root: every filesystem call in these cases goes through + // the mock filesystem, and the name deliberately avoids the `openbuff-` + // owned-temp patterns so an allow here can only come from the external read + // allowlist — i.e. the readableRoots support read_image documents. + const externalRoot = path.resolve('/external-read-root') + const externalImage = path.join(externalRoot, 'notes.png') + const externalPrivateKeyImage = path.join(externalRoot, 'id_rsa.png') + + beforeEach(() => { + resetExternalReadRootsForTesting() + }) + + afterEach(() => { + // The registry is module state: reset unconditionally so no later test + // inherits an open external read boundary. + resetExternalReadRootsForTesting() + }) + + test('attaches an image inside an allowlisted root', async () => { + configureExternalReadRoots([externalRoot]) + const image = Buffer.from('external-png-bytes') + + const output = await readImages({ + paths: [externalImage], + cwd: '/project', + fs: createMockFs({ [externalImage]: image }), + }) + + // Regression guard: read_image used to re-check the resolved realpath + // against the project root, which rejected every external-read resolution + // with OUTSIDE_PROJECT even though the resolver had already contained it + // inside the allowlisted root. + expect(output[0]).toEqual({ + type: 'json', + value: { + images: [ + { + path: externalImage, + status: 'attached', + mediaType: 'image/png', + sizeBytes: image.length, + message: 'Image attached as original media.', + }, + ], + }, + }) + expect(output[1]).toEqual({ + type: 'media', + data: image.toString('base64'), + mediaType: 'image/png', + }) + }) + + test('refuses the same image while the registry is unconfigured', async () => { + // No configureExternalReadRoots call: the default posture is closed, so the + // allow above is attributable to the allowlist rather than to paths outside + // the project having become generally readable. + const output = await readImages({ + paths: [externalImage], + cwd: '/project', + fs: createMockFs({ [externalImage]: Buffer.from('external-png-bytes') }), + }) + + expect(output).toHaveLength(1) + expect(output[0].type).toBe('json') + if (output[0].type !== 'json') throw new Error('Expected JSON output') + expect(output[0].value.images[0]).toEqual({ + path: externalImage, + status: 'error', + message: FILE_READ_STATUS.OUTSIDE_PROJECT, + }) + }) + + test('refuses a sensitive basename inside an allowlisted root', async () => { + configureExternalReadRoots([externalRoot]) + + const output = await readImages({ + paths: [externalPrivateKeyImage], + cwd: '/project', + fs: createMockFs({ + [externalPrivateKeyImage]: Buffer.from('private-key-bytes'), + }), + // An allow-everything host filter proves the refusal comes from the + // resolver's fail-closed mandatory-sensitive check (which returns no + // resolution at all, hence OUTSIDE_PROJECT), not from host policy. + fileFilter: () => ({ status: 'allow' }), + }) + + expect(output).toHaveLength(1) + expect(output[0].type).toBe('json') + if (output[0].type !== 'json') throw new Error('Expected JSON output') + expect(output[0].value.images[0]).toEqual({ + path: externalPrivateKeyImage, + status: 'error', + message: FILE_READ_STATUS.OUTSIDE_PROJECT, + }) + }) + + test('stats and reads only the resolver-validated operationPath, never a second realpath (TOCTOU)', async () => { + configureExternalReadRoots([externalRoot]) + // Chained realpath: the caller path dereferences to the target the resolver + // validates and returns as `operationPath`, and THAT target dereferences + // again to a redirect. So any second, unvalidated `fs.realpath` in the + // handler would silently move the stat/read to `redirectTarget` — the + // TOCTOU window this test pins closed. Only the resolver's single validated + // dereference may be touched. + const validatedImage = path.join(externalRoot, 'notes-real.png') + const redirectTarget = path.resolve( + '/external-read-secrets/credentials.json', + ) + const realpathRedirects: Record = { + [externalImage]: validatedImage, + [validatedImage]: redirectTarget, + } + const image = Buffer.from('validated-png-bytes') + const files: Record = { + [validatedImage]: image, + // Registered so a redirected read would SUCCEED (and be observable in the + // output) rather than failing for an unrelated ENOENT reason. + [redirectTarget]: Buffer.from('redirected-secret-bytes'), + } + const statCalls: string[] = [] + const readFileCalls: string[] = [] + const injectedFs = { + // `realpath` is the minimum member the read-only resolver calls. + realpath: async (filePath: PathLike) => + realpathRedirects[String(filePath)] ?? String(filePath), + stat: async (filePath: PathLike) => { + statCalls.push(String(filePath)) + const file = files[String(filePath)] + if (!file) { + throw createNodeError( + `ENOENT: no such file or directory: ${filePath}`, + 'ENOENT', + ) + } + return { + size: file.length, + isDirectory: () => false, + isFile: () => true, + atimeMs: Date.now(), + mtimeMs: Date.now(), + } + }, + readFile: async (filePath: PathLike) => { + readFileCalls.push(String(filePath)) + const file = files[String(filePath)] + if (!file) { + throw createNodeError( + `ENOENT: no such file or directory: ${filePath}`, + 'ENOENT', + ) + } + return file + }, + readdir: async () => [], + mkdir: async () => undefined, + unlink: async () => undefined, + writeFile: async () => undefined, + } as unknown as CodebuffFileSystem + + const output = await readImages({ + paths: [externalImage], + cwd: '/project', + fs: injectedFs, + }) + + // Exactly the resolver's validated operationPath is stat'd and read... + expect(statCalls).toEqual([validatedImage]) + expect(readFileCalls).toEqual([validatedImage]) + // ...and the redirect target is never touched. + expect(statCalls).not.toContain(redirectTarget) + expect(readFileCalls).not.toContain(redirectTarget) + + // The attached bytes come from the validated path, not the redirect. + expect(output[0]).toEqual({ + type: 'json', + value: { + images: [ + { + path: externalImage, + status: 'attached', + mediaType: 'image/png', + sizeBytes: image.length, + message: 'Image attached as original media.', + }, + ], + }, + }) + expect(output[1]).toEqual({ + type: 'media', + data: image.toString('base64'), + mediaType: 'image/png', + }) + }) + + test('applies a host fileFilter to the external-read policy alias', async () => { + configureExternalReadRoots([externalRoot]) + // external-read results carry an ABSOLUTE relativePath, so without the + // scoped alias a host filter written against project-relative globs would + // silently stop applying — a fail-open. + const blockedAliases: string[] = [] + + const output = await readImages({ + paths: [externalImage], + cwd: '/project', + fs: createMockFs({ [externalImage]: Buffer.from('external-png-bytes') }), + fileFilter: (candidate) => { + if (candidate === 'external-read/notes.png') { + blockedAliases.push(candidate) + return { status: 'blocked' } + } + return { status: 'allow' } + }, + }) + + expect(blockedAliases).toEqual(['external-read/notes.png']) + expect(output).toHaveLength(1) + expect(output[0].type).toBe('json') + if (output[0].type !== 'json') throw new Error('Expected JSON output') + expect(output[0].value.images[0]).toEqual({ + path: externalImage, + status: 'error', + message: FILE_READ_STATUS.IGNORED, + }) + }) +}) diff --git a/sdk/src/__tests__/read-logs.test.ts b/sdk/src/__tests__/read-logs.test.ts index 7212fe9471..cddbdf9de2 100644 --- a/sdk/src/__tests__/read-logs.test.ts +++ b/sdk/src/__tests__/read-logs.test.ts @@ -1,10 +1,12 @@ -import { afterEach, describe, expect, test } from 'bun:test' +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' import * as fs from 'fs' import * as os from 'os' import * as path from 'path' import { + configureExternalReadRoots, isOwnedTempPath, + resetExternalReadRootsForTesting, resolveProjectPath, resolveProjectPathForFileSystem, } from '@codebuff/common/util/project-path-containment' @@ -66,6 +68,8 @@ type ReadLogsValue = { status?: string resolvedPath?: string content?: string + lines?: number + truncated?: boolean errorMessage?: string } @@ -426,6 +430,134 @@ describe('readLogs', () => { expect(result.errorMessage).toContain('No background job found') expect(result.content).toBeUndefined() }) + + test('bounds the backward scan for a newline-free file larger than the byte ceiling', async () => { + const cwd = makeTempDir() + // One head line followed by a single newline-free run far larger than the + // byte ceiling. Before ER-6 the backward scan read the WHOLE file into one + // growing JS string, because `lineCount` never advanced past `lines` for + // newline-free input and the `lines`/`max_chars` caps only bound the + // returned slice. + fs.writeFileSync( + path.join(cwd, 'huge.log'), + `HEADMARKER\n${'x'.repeat(300_000)}`, + ) + + const result = value( + await readLogs({ cwd, path: 'huge.log', owner: TRUSTED_OWNER }), + ) + + expect(result.errorMessage).toBeUndefined() + expect(result.truncated).toBe(true) + // The default max_chars (20_000) bounds the returned slice... + expect(result.content?.length).toBe(20_000) + // ...and the byte-bounded scan never reached the head of the file, so the + // reported line count describes only the bounded region it actually read. + expect(result.content).not.toContain('HEADMARKER') + expect(result.lines).toBe(1) + }) +}) + +describe('readLogs — allowlisted external read roots', () => { + beforeEach(() => { + // The registry is configure-once per PROCESS, and `run.ts` legitimately + // configures it (with the openbuff config dir) as soon as any suite in this + // process exercises a run. Reset BEFORE configuring, or the strict + // `configureExternalReadRoots` throws on the differing set and this test + // passes in isolation while failing in a full-directory run. + resetExternalReadRootsForTesting() + }) + + afterEach(() => { + // Module state: reset unconditionally so no later test inherits an open + // external read boundary. + resetExternalReadRootsForTesting() + }) + + test('reads a log inside an allowlisted external root', async () => { + const cwd = makeTempDir() + const externalRoot = makeTempDir() + const externalLog = path.join(externalRoot, 'external.log') + fs.writeFileSync(externalLog, 'one\ntwo\nthree\n') + + configureExternalReadRoots([externalRoot]) + + const result = value( + await readLogs({ + cwd, + path: externalLog, + lines: 2, + max_chars: 1_000, + owner: TRUSTED_OWNER, + }), + ) + + expect(result.errorMessage).toBeUndefined() + // Compared against the realpath: on macOS `os.tmpdir()` is a symlinked + // `/var/folders/...` path. + expect(result.resolvedPath).toBe(fs.realpathSync(externalLog)) + expect(result.content).toBe('two\nthree\n') + }) + + test('refuses the same log while the registry is unconfigured', async () => { + const cwd = makeTempDir() + const externalRoot = makeTempDir() + const externalLog = path.join(externalRoot, 'external.log') + fs.writeFileSync(externalLog, 'one\ntwo\nthree\n') + + const result = value( + await readLogs({ cwd, path: externalLog, owner: TRUSTED_OWNER }), + ) + + expect(result.errorMessage).toContain('outside the project directory') + expect(result.content).toBeUndefined() + }) + + test('a host fileFilter blocks a log inside an allowlisted external root', async () => { + const cwd = makeTempDir() + const externalRoot = makeTempDir() + const externalLog = path.join(externalRoot, 'external.log') + fs.writeFileSync(externalLog, 'one\ntwo\nthree\n') + + configureExternalReadRoots([externalRoot]) + + // An 'external-read' resolution carries an ABSOLUTE `relativePath`, so the + // host policy is targeted through the scoped `external-read/` + // alias — the same alias shape read_files / read_image present. + const blocked = value( + await readLogs({ + cwd, + path: externalLog, + lines: 2, + max_chars: 1_000, + owner: TRUSTED_OWNER, + fileFilter: (filePath: string) => ({ + status: + filePath === 'external-read/external.log' + ? ('blocked' as const) + : ('allow' as const), + }), + }), + ) + + expect(blocked.errorMessage).toBe('[BLOCKED]') + expect(blocked.content).toBeUndefined() + + // Without a filter the very same log still reads, so the refusal above is + // the host policy and not the containment boundary. + const allowed = value( + await readLogs({ + cwd, + path: externalLog, + lines: 2, + max_chars: 1_000, + owner: TRUSTED_OWNER, + }), + ) + + expect(allowed.errorMessage).toBeUndefined() + expect(allowed.content).toBe('two\nthree\n') + }) }) describe('owned-temp containment through an injected filesystem', () => { diff --git a/sdk/src/__tests__/terminal-command-policy.test.ts b/sdk/src/__tests__/terminal-command-policy.test.ts index be721da00f..8696015df4 100644 --- a/sdk/src/__tests__/terminal-command-policy.test.ts +++ b/sdk/src/__tests__/terminal-command-policy.test.ts @@ -180,6 +180,9 @@ describe('terminal command permission policy', () => { 'export', 'export -p', 'pwd; printenv', + // Background `&` is a command separator too: the dump job must be seen. + 'pwd & printenv', + 'true & env', // Nested/wrapped/path dump forms must not slip past bare-leading checks. 'env printenv', 'env env', @@ -231,6 +234,46 @@ describe('terminal command permission policy', () => { } }) + it('tolerates the bare temp-root token while still refusing sibling-prefix paths', () => { + for (const command of [ + // Bare `/tmp` operand (no trailing slash). + "stat -c '%a %U' /tmp", + // The verbatim stale-capture sweep at the top of the tmux-cli setup script. + "find /tmp -maxdepth 1 -type d -name 'tmux-captures-*' -mmin +1440", + // Paths inside the temp root keep working. + 'cat /tmp/openbuff-job-1.log', + ]) { + expect( + evaluateTerminalCommandPolicy({ + command, + mode: 'assistant', + permissionProfile: 'workspace-write', + projectRoot, + }), + ).toEqual({ allowed: true }) + } + // Sibling-prefix regression: a naive `startsWith('/tmp')` relaxation would + // allow these, because `'/tmpfoo'.startsWith('/tmp')` is true. + for (const [command, token] of [ + ['cat /tmpfoo/secret', '/tmpfoo/secret'], + ['cat /tmpevil/x', '/tmpevil/x'], + // Unrelated outside paths stay refused. + ['cat /etc/passwd', '/etc/passwd'], + ] as const) { + expect( + evaluateTerminalCommandPolicy({ + command, + mode: 'assistant', + permissionProfile: 'workspace-write', + projectRoot, + }), + ).toEqual({ + allowed: false, + reason: `absolute path is outside the project: ${token}`, + }) + } + }) + it('denies process environment dumps under tmux-test without re-enabling workspace deny patterns', () => { for (const command of [ 'printenv', diff --git a/sdk/src/impl/__tests__/failover.test.ts b/sdk/src/impl/__tests__/failover.test.ts index 21807d7043..a6a5eb0b0b 100644 --- a/sdk/src/impl/__tests__/failover.test.ts +++ b/sdk/src/impl/__tests__/failover.test.ts @@ -41,6 +41,7 @@ function makeLoadedConfig( semantic: { enabled: false, model: undefined }, }, fileChangeHooks: [], + readableRoots: [], failoverModels, }, sourceFilePaths: [], diff --git a/sdk/src/provider-config.ts b/sdk/src/provider-config.ts index 9578127baa..633910725c 100644 --- a/sdk/src/provider-config.ts +++ b/sdk/src/provider-config.ts @@ -471,6 +471,21 @@ export const providerConfigFileSchema = z autoFileChangeHooks: z.boolean().optional(), /** Approval UX for classified terminal effects. */ approvalMode: z.enum(['balanced', 'strict', 'allow-all']).optional(), + /** + * Additional roots OUTSIDE the project that read-only tools may reach + * (read_files, read_logs, read_image, list_directory). + * + * Reads only: this never grants write access, and there is no + * external-write equivalent. Each entry must be an ABSOLUTE path or it is + * ignored (a relative entry in a possibly-global config file is ambiguous, + * so it is dropped rather than anchored to a guessed directory). + * Filesystem roots (`/`, `C:\`) and entries containing a `..` segment are + * refused, because allowlisting a filesystem root is the opposite of an + * allowlist. Mandatory-sensitive files (`.env`, `credentials.json`, private + * keys, ...) stay blocked inside an allowlisted root. The value is applied + * once per process, so changing it requires restarting openbuff. + */ + readableRoots: z.array(z.string().min(1)).default([]), /** * Optional fixed agent-step cap. Unset or -1 means unlimited productive * steps; a repeated-step watchdog still stops identical no-progress loops. @@ -555,6 +570,9 @@ export const providerConfigFileSchema = z ...(config.approvalMode !== undefined && { approvalMode: config.approvalMode, }), + // Always present thanks to `.default([])`, so downstream code (the + // run-start registry wiring) never has to handle `undefined`. + readableRoots: config.readableRoots, // Optional in the resolved config: omitted unless explicitly set, so // callers use the unlimited default plus the no-progress watchdog. ...(config.maxAgentSteps !== undefined && { @@ -595,6 +613,17 @@ export type ResolvedProviderModel = { export type LoadedProviderConfig = { config: ProviderConfigFile sourceFilePaths: string[] + /** + * ER-1 provenance metadata for the `readableRoots` trust gate: + * `path.resolve()` -> the absolute path of the config + * FILE that declared it. + * + * Deliberately kept OUTSIDE the parsed `config` object (and OPTIONAL) so the + * zod schema OUTPUT type is unchanged — adding a required field there caused + * wide test-fixture churn we already had to repair once. Consumers must fail + * CLOSED when an effective root has no recorded source here. + */ + readableRootsSources?: Record diagnostics?: Array<{ filePath: string message: string @@ -635,6 +664,7 @@ const emptyProviderConfig = (): ProviderConfigFile => ({ fileChangeHooks: [], autoFileChangeHooks: undefined, approvalMode: 'balanced', + readableRoots: [], failoverModels: undefined, maxAgentSteps: undefined, }) @@ -810,6 +840,7 @@ function readProviderConfigFile( let config = emptyProviderConfig() const sourceFilePaths: string[] = [] + const readableRootsSources: Record = {} let sourceFiles: NonNullable = { providers: {}, routes: { @@ -826,6 +857,13 @@ function readProviderConfigFile( sourceFiles, loadedFragment.sourceFiles ?? {}, ) + // ER-1 provenance follows the same last-declaration-wins rule as + // `mergeSourceFiles` above, so nested/extends fragments attribute their + // roots to the exact file that declared them. + Object.assign( + readableRootsSources, + loadedFragment.readableRootsSources ?? {}, + ) } const parseResult = providerConfigFileSchema.safeParse(rawConfig) @@ -836,6 +874,12 @@ function readProviderConfigFile( } config = mergeProviderConfigs(config, parseResult.data) + // This file's own declarations win over any fragment it included, matching + // the override-wins `readableRoots` merge in mergeProviderConfigs. + for (const entry of parseResult.data.readableRoots) { + readableRootsSources[path.resolve(entry)] = resolvedConfigPath + } + const currentSourceFiles = getSourceFilesFromRawConfig( rawConfig, resolvedConfigPath, @@ -848,6 +892,7 @@ function readProviderConfigFile( new Set([...sourceFilePaths, resolvedConfigPath]), ), sourceFiles, + readableRootsSources, } state.cache.set(resolvedConfigPath, result) return result @@ -936,6 +981,12 @@ function mergeProviderConfigs( autoFileChangeHooks: override.autoFileChangeHooks ?? base.autoFileChangeHooks, approvalMode: override.approvalMode ?? base.approvalMode, + // An override fragment that simply omits `readableRoots` parses to `[]` + // (schema default), so a plain override would silently erase a base + // fragment's allowlist. Only a non-empty override replaces it. + readableRoots: override.readableRoots?.length + ? override.readableRoots + : (base.readableRoots ?? []), failoverModels: override.failoverModels ?? base.failoverModels, maxAgentSteps: override.maxAgentSteps ?? base.maxAgentSteps, } @@ -1201,6 +1252,7 @@ export function loadProviderConfigSync( let config = emptyProviderConfig() const sourceFilePaths: string[] = [] + const readableRootsSources: Record = {} const diagnostics: NonNullable = [] let sourceFiles: NonNullable = { providers: {}, @@ -1223,6 +1275,18 @@ export function loadProviderConfigSync( sourceFiles, parsedConfig.sourceFiles ?? {}, ) + // ER-1 provenance: every fragment already attributes its own + // `readableRoots` entries to the exact file that declared them, so this + // merge just folds those maps together — later config paths overwrite + // earlier ones for the same root, matching the override-wins merge + // above. Deliberately NO fallback to `configPath` for an unattributed + // root: an unrecorded root must stay unrecorded so the trust gate fails + // CLOSED rather than inheriting this file's trust. Purely in-memory — no + // extra filesystem work on this hot path. + Object.assign( + readableRootsSources, + parsedConfig.readableRootsSources ?? {}, + ) } catch (error) { if (explicitConfigPath) { throw error @@ -1249,6 +1313,7 @@ export function loadProviderConfigSync( config, sourceFilePaths, sourceFiles, + readableRootsSources, diagnostics, } providerConfigCache = { key: cacheKey, config: result } @@ -2429,6 +2494,10 @@ export function writeProviderConfigFile(params: { existingConfig.autoFileChangeHooks ?? newConfig.autoFileChangeHooks, failoverModels: existingConfig.failoverModels ?? newConfig.failoverModels, maxAgentSteps: existingConfig.maxAgentSteps ?? newConfig.maxAgentSteps, + // Preserve the user's existing allowlist; /setup only adds providers. + readableRoots: existingConfig.readableRoots?.length + ? existingConfig.readableRoots + : (newConfig.readableRoots ?? []), } if (tryWriteFragmentedConfig(configPath, mergedConfig)) { diff --git a/sdk/src/run.ts b/sdk/src/run.ts index 0039b9ab8f..7e4ac95d79 100644 --- a/sdk/src/run.ts +++ b/sdk/src/run.ts @@ -29,14 +29,16 @@ import { advanceWorkspaceState } from '@codebuff/common/types/workspace-state' import type { WorkspaceStateV1 } from '@codebuff/common/types/workspace-state' import { extractApiErrorDetails } from '@codebuff/common/util/error' import { jobRegistry } from '@codebuff/common/util/job-registry' +import { ensureExternalReadRootsConfigured } from '@codebuff/common/util/project-path-containment' import { stableHash } from '@codebuff/common/util/stable-hash' import type { TaskMemoryV1 } from '@codebuff/common/types/task-memory' import { cloneDeep } from 'lodash' import { getErrorStatusCode } from './error-utils' import { createJobUpdateForwarder } from './job-update-forwarder' -import { getHarnessStateDir } from './credentials' +import { getHarnessStateDir, getConfigDir } from './credentials' import { getAgentRuntimeImpl } from './impl/agent-runtime' +import { loadProviderConfigSync } from './provider-config' import { initialSessionState, applyOverridesToSessionState } from './run-state' import { codebuffFsToNodePromises, @@ -133,6 +135,7 @@ import { listJobs } from './tools/list-jobs' import type { ListJobsViewRow } from '@codebuff/common/util/list-jobs-view' import { fingerprintListJobsRows } from '@codebuff/common/util/list-jobs-view' +import { getSystemProcessEnv } from './env' /** * Wraps content for user messages, ensuring text is wrapped in tags. @@ -338,6 +341,74 @@ const createAbortError = (signal?: AbortSignal) => { return error } +/** + * `path.relative`-based containment, the convention used throughout this + * codebase. `startsWith` is treated as a bug here: a sibling directory such as + * `-evil` shares the prefix but is NOT contained. + */ +function isPathInsideDirectory(directory: string, candidate: string): boolean { + const relative = path.relative( + path.resolve(directory), + path.resolve(candidate), + ) + return ( + relative !== '' && !relative.startsWith('..') && !path.isAbsolute(relative) + ) +} + +/** Outcome of the ER-1 `readableRoots` trust gate. */ +export type TrustedReadableRootsSelection = { + /** Roots that may be registered with the external read allowlist. */ + trusted: string[] + /** + * How many ABSOLUTE roots were dropped for untrusted provenance. Reported as + * a count only, never as paths: a user's home-directory paths are mildly + * sensitive and the warning that carries this may be shared. + */ + untrustedCount: number +} + +/** + * ER-1 trust gate for `readableRoots`, extracted as a PURE function so the + * decision that lets a cloned repository allowlist credential directories is + * directly testable (see `sdk/src/__tests__/model-provider.test.ts`). + * + * TRUSTED: the root's declaring config file lives inside `configDir` + * (user-owned). Always registered. + * UNTRUSTED: declared anywhere else (project or ancestor `openbuff.json`), or + * with no recorded provenance at all — fail CLOSED and drop it unless + * `trustProjectRoots` (OPENBUFF_TRUST_PROJECT_READABLE_ROOTS=1) is set. + * + * Relative entries are dropped rather than guessed — the declaring config may + * be a global file loaded from an unrelated directory, so anchoring to + * `process.cwd()` could allowlist something the user never intended — and they + * are NOT counted as untrusted, since nothing was refused on trust grounds. + */ +export function selectTrustedReadableRoots(params: { + roots: readonly string[] + sources: Record + configDir: string + trustProjectRoots: boolean +}): TrustedReadableRootsSelection { + const { roots, sources, configDir, trustProjectRoots } = params + const trusted: string[] = [] + let untrustedCount = 0 + for (const root of roots) { + if (!path.isAbsolute(root)) continue + const sourceFile = sources[path.resolve(root)] + if (sourceFile && isPathInsideDirectory(configDir, sourceFile)) { + trusted.push(root) + continue + } + if (trustProjectRoots) { + trusted.push(root) + continue + } + untrustedCount++ + } + return { trusted, untrustedCount } +} + async function executeOverride({ override, input, @@ -522,6 +593,109 @@ async function runOnce({ resumeInterruptedTurn, }: RunExecutionOptions): Promise { const resolvedHarnessStateDir = harnessStateDir ?? getHarnessStateDir(env) + // Read-only external root allowlist. Configured HERE, strictly before any + // tool can dispatch, so the first read of an allowlisted path in a process + // cannot fail confusingly while later ones succeed. Two sources: + // 1. the openbuff config directory, which is what makes "read my logs/state + // from the config dir" work by default (sensitive files there, notably + // credentials.json, stay refused by the resolver itself); + // 2. absolute `readableRoots` entries from openbuff.json, subject to the + // ER-1 trust gate below (only roots declared by a config file inside the + // config dir are registered without an explicit opt-in). Relative + // entries are dropped rather than guessed: that config may be a global + // file loaded from an unrelated directory, so anchoring to process.cwd() + // could allowlist something the user never intended. + // Best-effort: if this block throws (e.g. a malformed config), no external + // roots are configured and every read outside the project stays refused. + try { + // loadProviderConfigSync is module-cached but still on a hot path; called + // exactly once per run here, never per path or per tool. + const loadedProviderConfig = loadProviderConfigSync(env ? { env } : {}) + const loadedReadableRoots = loadedProviderConfig.config.readableRoots + const readableRootsSources = + loadedProviderConfig.readableRootsSources ?? {} + const configDir = getConfigDir(env) + // ER-1 trust gate. `readableRoots` is the only config key that grants + // filesystem authority rather than influencing model routing, so a value + // supplied by a cloned repository must not silently allowlist directories + // like `~/.config/gh` or `~/.docker`: config resolution includes + // `/openbuff.json` (the first ancestor entry), and a non-empty + // project value WINS the merge. A repo-supplied value therefore needs the + // same explicit acknowledgement this codebase already requires for + // ancestor `apiKeyEnv` providers. + // + // TRUSTED: the root's declaring config file lives inside the openbuff + // config dir (user-owned). Always registered. + // UNTRUSTED: declared anywhere else (project or ancestor `openbuff.json`), + // or with no recorded provenance at all — fail CLOSED and drop it unless + // the user opted in. + // + // The opt-in env var is read through the same accessor + // `getAncestorProviderConfigPaths` uses for + // OPENBUFF_TRUST_ANCESTOR_CONFIG, so both trust switches behave alike. + const trustProjectReadableRoots = + (getSystemProcessEnv().OPENBUFF_TRUST_PROJECT_READABLE_ROOTS ?? '') === + '1' + const { + trusted: trustedReadableRoots, + untrustedCount: untrustedRootCount, + } = selectTrustedReadableRoots({ + roots: loadedReadableRoots, + sources: readableRootsSources, + configDir, + trustProjectRoots: trustProjectReadableRoots, + }) + if (untrustedRootCount > 0) { + // Counts only, never the raw paths: a user's home-directory paths are + // mildly sensitive and this log may be shared. + logger?.warn( + { + untrustedRootCount, + trustedRootCount: trustedReadableRoots.length, + }, + 'Ignored readableRoots entries that were not declared by a config file ' + + 'inside the openbuff config directory (a project or ancestor ' + + 'openbuff.json, or an entry with no recorded provenance). A ' + + 'repository-supplied allowlist can expose credential directories ' + + 'outside the project. Set ' + + 'OPENBUFF_TRUST_PROJECT_READABLE_ROOTS=1 to acknowledge and register ' + + 'them.', + ) + } + const externalReadResult = ensureExternalReadRootsConfigured( + [ + // The config dir root is seeded by the runtime, not by config, so it is + // always trusted and stays unconditional. Narrowing it is out of scope. + configDir, + ...trustedReadableRoots, + ], + // ER-5: the registry is process-global but `cwd` is per-run, so the + // boundary is tagged with the project it belongs to. Without an owner, a + // second project configured in the same process (after + // `switchProjectContext`) looks identical to a mid-run attempt to + // re-point the boundary: the strict primitive would refuse it, leaving + // project A's roots readable while project B's own allowlist never + // applied. Supplying the owner makes a genuine switch REPLACE the + // boundary instead, which is strictly safer. + cwd, + ) + if (externalReadResult.status === 'refused-changed') { + // Counts only, never the raw paths: a user's home-directory paths are + // mildly sensitive and this log may be shared. + logger?.warn( + { + effectiveRootCount: externalReadResult.roots.length, + attemptedRootCount: externalReadResult.attempted.length, + }, + 'External read roots changed since process start; keeping the boundary configured earlier in this process. Restart openbuff to apply the new readableRoots.', + ) + } + } catch (error) { + logger?.warn( + { error }, + 'External read roots could not be configured; reads outside the project stay refused for this run', + ) + } let fs: CodebuffFileSystem if (fsSource !== undefined) { const fsSourceValue = typeof fsSource === 'function' ? fsSource() : fsSource @@ -2036,12 +2210,15 @@ export async function handleToolCall({ } else if (toolName === 'read_logs') { const readLogsInput = input as Omit< Parameters[0], - 'cwd' | 'owner' + 'cwd' | 'owner' | 'fileFilter' > result = await readLogs({ ...readLogsInput, cwd: requireCwd(cwd, 'read_logs'), owner: trustedJobOwner, + // ER-4: host read policy is runtime-injected, exactly like the + // read_image / list_directory branches; model input cannot supply it. + fileFilter, }) } else if (toolName === 'list_jobs') { // Any model-supplied `input.owner` is ignored entirely; scoping always diff --git a/sdk/src/tools/__tests__/list-directory.test.ts b/sdk/src/tools/__tests__/list-directory.test.ts index d40606214a..43f181b3e1 100644 --- a/sdk/src/tools/__tests__/list-directory.test.ts +++ b/sdk/src/tools/__tests__/list-directory.test.ts @@ -1,10 +1,14 @@ -import { describe, expect, test } from 'bun:test' +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' import { promises as nodeFs } from 'node:fs' import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' import os from 'os' import path from 'path' -import { OWNED_TEMP_SEGMENT_PATTERNS } from '@codebuff/common/util/project-path-containment' +import { + OWNED_TEMP_SEGMENT_PATTERNS, + configureExternalReadRoots, + resetExternalReadRootsForTesting, +} from '@codebuff/common/util/project-path-containment' import { listDirectory, @@ -796,6 +800,96 @@ describe('listDirectory listing behaviour', () => { }) }) +describe('listDirectory allowlisted external read roots', () => { + // Synthetic absolute root: every filesystem call goes through the stub + // filesystem, and the name deliberately avoids the `openbuff-` owned-temp + // patterns so an allow here can only come from the external read allowlist. + const externalRoot = path.resolve('/external-read-root') + // Strictly inside the root: the root itself is deliberately not readable. + const externalDir = path.join(externalRoot, 'logs') + + beforeEach(() => { + resetExternalReadRootsForTesting() + }) + + afterEach(() => { + // The registry is module state: reset unconditionally so no later test + // inherits an open external read boundary. + resetExternalReadRootsForTesting() + }) + + test('lists a directory strictly inside a configured external root', async () => { + configureExternalReadRoots([externalRoot]) + const readdir = makeReaddir([ + dirent('job.log'), + dirent('.env'), + dirent('nested', 'dir'), + ]) + const result = await listDirectory({ + directoryPath: externalDir, + projectPath: '/virtual/repo', + fs: makeFs({ readdir }), + }) + const value = expectListing(result) + expect(value.files).toEqual(['job.log']) + // The mandatory sensitive-path block still applies to external entries. + expect(value.directories).toEqual(['nested']) + expect(value.path).toBe(externalDir) + // Listing runs on the resolved real path inside the allowlisted root. + expect(readdir.calls).toEqual([externalDir]) + }) + + test('refuses the same directory while the registry is unconfigured', async () => { + // No configureExternalReadRoots call: the default posture is closed, so the + // allow above is attributable to the allowlist rather than to directories + // outside the project having become generally listable. + const result = await listDirectory({ + directoryPath: externalDir, + projectPath: '/virtual/repo', + fs: rejectingFs(), + }) + expectContainmentRejection(result) + }) + + test('applies a host fileFilter to the external-read entry alias', async () => { + configureExternalReadRoots([externalRoot]) + // An external-read resolution carries an ABSOLUTE relativePath, so the + // joined entry path is absolute too and a host filter written against + // project-relative globs would never match it — a fail-open. The scoped + // `external-read/` alias is what the host can target. + const seen: string[] = [] + const fileFilter: FileFilter = (filePath) => { + seen.push(filePath) + return { + status: + filePath === 'external-read/blocked.log' || + filePath === 'external-read/blocked-dir' + ? 'blocked' + : 'allow', + } + } + const result = await listDirectory({ + directoryPath: externalDir, + projectPath: '/virtual/repo', + fs: makeFs({ + readdir: makeReaddir([ + dirent('kept.log'), + dirent('blocked.log'), + dirent('nested', 'dir'), + dirent('blocked-dir', 'dir'), + ]), + }), + fileFilter, + }) + const value = expectListing(result) + expect(value.files).toEqual(['kept.log']) + expect(value.directories).toEqual(['nested']) + expect(seen).toContain('external-read/kept.log') + expect(seen).toContain('external-read/blocked.log') + expect(seen).toContain('external-read/blocked-dir') + }) +}) + describe('supportsStreamDirectory', () => { test('reports true only for a capability paired with the adapter readdir', async () => { const fs = streamingFs(makeStreamDirectory([dirent('kept.txt')])) diff --git a/sdk/src/tools/filesystem-authority.ts b/sdk/src/tools/filesystem-authority.ts index 9c2b567308..8c02d1cacd 100644 --- a/sdk/src/tools/filesystem-authority.ts +++ b/sdk/src/tools/filesystem-authority.ts @@ -701,6 +701,16 @@ export class FilesystemAuthority { operation: FilesystemOperationKind, phase: FilesystemPolicyPhase, ): Promise { + // Fail closed on the read-only `external-read` scope: this authority only + // covers the project tree and the openbuff-owned temp namespace, which are + // the only scopes the operation resolvers + // (`resolveFilePathFor*Operation`) can produce. The check keeps + // `AuthorizedFilesystemPath.scope` narrow instead of widening a + // mutation-side type with a read-only scope. + if (resolved.scope === 'external-read') { + return { allowed: false, code: 'external_read_scope_unsupported' } + } + const scope = resolved.scope const portablePath = toPortablePath(resolved.relativePath) const canonicalParentPath = path.dirname(resolved.operationPath) const decision = await this.policy.evaluate({ @@ -722,7 +732,7 @@ export class FilesystemAuthority { portablePath, operationPath: resolved.operationPath, redactPath: decision.redactPath === true, - scope: resolved.scope, + scope, }, } } diff --git a/sdk/src/tools/list-directory.ts b/sdk/src/tools/list-directory.ts index 37ee624e50..9a6c88a447 100644 --- a/sdk/src/tools/list-directory.ts +++ b/sdk/src/tools/list-directory.ts @@ -3,7 +3,10 @@ import * as path from 'path' import { MAX_LIST_DIRECTORY_ENTRIES } from '@codebuff/common/tools/params/tool/list-directory' import { readBoundedEntries } from './bounded-readdir' -import { resolveFilePathForFileSystemOperation } from './path-utils' +import { + getScopedReadPolicyAliases, + resolveFilePathForFileSystemReadOperation, +} from './path-utils' import { isReadPathBlocked } from './read-policy' import type { CodebuffToolOutput } from '@codebuff/common/tools/list' @@ -27,7 +30,8 @@ export async function listDirectory(params: { // Reuse the shared containment helper so list_directory gets the same // lexical + symlink-resolved protection as read_files; a bare // `startsWith(projectPath)` prefix check admits siblings like /project-evil. - const resolved = await resolveFilePathForFileSystemOperation( + // list_directory only reads, so it uses the read-only resolver. + const resolved = await resolveFilePathForFileSystemReadOperation( projectPath, directoryPath, fs, @@ -72,7 +76,16 @@ export async function listDirectory(params: { resolved.relativePath.replace(/\\/g, '/'), entry.name, ) - if (isReadPathBlocked(relativeEntryPath, fileFilter)) continue + // A non-'project' resolution has an ABSOLUTE relativePath (and therefore + // an absolute relativeEntryPath), so the scoped `/` + // aliases are added for the host fileFilter; without them a filter + // written against project-relative globs would silently fail open. + const policyAliases = [ + relativeEntryPath, + ...getScopedReadPolicyAliases(resolved.scope, entry.name), + ] + if (policyAliases.some((alias) => isReadPathBlocked(alias, fileFilter))) + continue if (entry.isDirectory()) { directories.push(entry.name) } else if (entry.isFile()) { diff --git a/sdk/src/tools/path-utils.ts b/sdk/src/tools/path-utils.ts index 97c8326be3..688a4c60fb 100644 --- a/sdk/src/tools/path-utils.ts +++ b/sdk/src/tools/path-utils.ts @@ -6,6 +6,8 @@ import { OWNED_TEMP_SEGMENT_PATTERNS, resolveProjectPath, resolveProjectPathForFileSystem, + resolveProjectPathForFileSystemRead, + resolveProjectPathForRead, type ContainedProjectPath, } from '@codebuff/common/util/project-path-containment' @@ -53,6 +55,29 @@ export type ResolvedOperationPath = ContainedProjectPath & { operationPath: string } +/** + * Extra host-policy alias keys for a read path resolved under a non-'project' + * scope. + * + * WHY: an 'owned-temp' or 'external-read' resolution carries an ABSOLUTE + * `relativePath`, so a host `fileFilter` written against project-relative globs + * never matches it and would silently fail OPEN. The basename and the stable + * `/` key (`owned-temp/job.log`, `external-read/notes.png`) + * are what a host policy can actually target for those paths — the same alias + * shape `read-files.ts` builds in `authorizeReadTarget`. + * + * Returns an empty list for `scope === 'project'`, where the project-relative + * path is already the key a host policy targets. + */ +export function getScopedReadPolicyAliases( + scope: ContainedProjectPath['scope'], + pathOrName: string, +): string[] { + if (scope === 'project') return [] + const basename = path.posix.basename(pathOrName.replace(/\\/g, '/')) + return [...new Set([basename, `${scope}/${basename}`])] +} + /** * Shared owned-temp fallback for unlink-style operations (followFinalSymlink: false). * @@ -259,3 +284,54 @@ export async function resolveFilePathForFileSystemOperation( if (!operationPath) return null return { ...resolved, operationPath } } + +/** + * READ-ONLY twin of `resolveFilePathForOperation`. + * + * Delegates to `resolveProjectPathForRead`, so in addition to project and + * owned-temp paths it also resolves a path strictly inside an explicitly + * allowlisted external read root (`scope: 'external-read'`, with an ABSOLUTE + * `relativePath` — consumers must branch on `scope`). + * + * ANY caller of this function MUST be a read-only operation. The write path + * (`change-file.ts`, `replace-range.ts`, `filesystem-authority.ts`, + * `3d-assets.ts`) keeps calling `resolveFilePathForOperation`, which is blind + * to the external read allowlist — so reaching an allowlisted root from a write + * would require someone to edit a write handler to call this differently-named + * read-only resolver. + * + * This is the FOLLOW-SYMLINK read shape ONLY: there is deliberately no + * `followFinalSymlink: false` option. That option exists for unlink-style + * operations (deleting the link rather than its target), which are mutations + * and must never reach an external root. + */ +export function resolveFilePathForReadOperation( + projectRoot: string, + input: string, +): ResolvedOperationPath | null { + const resolved = resolveProjectPathForRead(projectRoot, input) + if (!resolved) return null + return { ...resolved, operationPath: resolved.realFullPath } +} + +/** + * Filesystem-aware counterpart of `resolveFilePathForReadOperation`, used + * whenever the read itself runs through an injected CodebuffFileSystem. + * + * Same read-only contract, and the same deliberate omission of + * `followFinalSymlink: false`: unlink-style resolution is for mutations, which + * must never reach an allowlisted external root. + */ +export async function resolveFilePathForFileSystemReadOperation( + projectRoot: string, + input: string, + fileSystem: CodebuffFileSystem, +): Promise { + const resolved = await resolveProjectPathForFileSystemRead( + projectRoot, + input, + fileSystem, + ) + if (!resolved) return null + return { ...resolved, operationPath: resolved.realFullPath } +} diff --git a/sdk/src/tools/read-files.ts b/sdk/src/tools/read-files.ts index c293896b2c..e736ed82c2 100644 --- a/sdk/src/tools/read-files.ts +++ b/sdk/src/tools/read-files.ts @@ -21,8 +21,10 @@ import { } from '@codebuff/common/util/sensitive-paths' import { + getScopedReadPolicyAliases, isSafeProjectRelativePath, resolveFilePathForFileSystemOperation, + resolveFilePathForFileSystemReadOperation, } from './path-utils' import type { FileLineRange } from '@codebuff/common/types/contracts/client' @@ -173,7 +175,10 @@ async function authorizeReadTarget(params: { } } - const resolved = await resolveFilePathForFileSystemOperation( + // Read-only resolver: in-project and owned-temp paths behave exactly as + // before, plus paths strictly inside an explicitly allowlisted external read + // root. The write path keeps using resolveFilePathForFileSystemOperation. + const resolved = await resolveFilePathForFileSystemReadOperation( cwd, requestedPath, fs, @@ -195,19 +200,18 @@ async function authorizeReadTarget(params: { resolved.relativePath, canonicalRelative || resolved.relativePath, ) - if (resolved.scope === 'owned-temp') { - // Owned-temp results carry an ABSOLUTE `relativePath`, so a host filter - // written against project-relative globs never matches it and would - // silently fail open. The basename and the stable `owned-temp/` - // key are what a host policy can target for these paths. The mandatory - // sensitive-path blocklist below is basename-driven and unaffected. - const ownedTempBasename = path.basename(resolved.operationPath) - for (const alias of uniquePolicyAliases( - ownedTempBasename, - `owned-temp/${ownedTempBasename}`, - )) { - if (!aliases.includes(alias)) aliases.push(alias) - } + // Non-project scopes (owned-temp AND external-read) carry an ABSOLUTE + // `relativePath`, so a host filter written against project-relative globs + // never matches it and would silently fail OPEN. `getScopedReadPolicyAliases` + // is the CANONICAL builder for those keys (`owned-temp/notes.txt`, + // `external-read/notes.txt`), shared with read-logs / read-image / + // list-directory so the four read tools cannot drift on the key a host + // fileFilter must target; it returns nothing for scope 'project'. The + // mandatory sensitive-path blocklist below is basename-driven and unaffected. + for (const alias of uniquePolicyAliases( + ...getScopedReadPolicyAliases(resolved.scope, resolved.operationPath), + )) { + if (!aliases.includes(alias)) aliases.push(alias) } if (aliases.some(isMandatorySensitiveReadPath)) { return { diff --git a/sdk/src/tools/read-image.ts b/sdk/src/tools/read-image.ts index 262f92fb2b..5c608485b6 100644 --- a/sdk/src/tools/read-image.ts +++ b/sdk/src/tools/read-image.ts @@ -8,7 +8,10 @@ import { } from '@codebuff/common/constants/images' import { FILE_READ_STATUS } from '@codebuff/common/old-constants' -import { resolveFilePathForFileSystemOperation } from './path-utils' +import { + getScopedReadPolicyAliases, + resolveFilePathForFileSystemReadOperation, +} from './path-utils' import { isReadPathBlocked } from './read-policy' import type { CodebuffToolOutput } from '@codebuff/common/tools/list' @@ -85,7 +88,9 @@ export async function readImages(params: { }, } } - const resolvedPath = await resolveFilePathForFileSystemOperation( + // Read-only tool, so it resolves through the read-only containment + // resolver. + const resolvedPath = await resolveFilePathForFileSystemReadOperation( cwd, imagePath, fs, @@ -114,7 +119,17 @@ export async function readImages(params: { } } - if (isReadPathBlocked(relativePath, fileFilter)) { + // A non-'project' resolution carries an ABSOLUTE relativePath, so the + // scoped `/` aliases are added for the host fileFilter; + // without them a filter written against project-relative globs would + // silently fail open. The basename comes from the dereferenced + // `operationPath`, exactly like read-files.ts's `authorizeReadTarget`, so + // the two tools present the same key to a host policy. + const policyAliases = [ + relativePath, + ...getScopedReadPolicyAliases(resolvedPath.scope, fullPath), + ] + if (policyAliases.some((alias) => isReadPathBlocked(alias, fileFilter))) { return { kind: 'error', entry: { @@ -125,16 +140,29 @@ export async function readImages(params: { } } - // Realpath-based containment: if the resolved path (or any symlink it - // points through) escapes the project root, reject before reading. - let realResolved: string | null = null - try { - realResolved = await fs.realpath(fullPath) - } catch { - // File may not exist yet; fall through and let fs.stat below produce - // the normal DOES_NOT_EXIST error. - } - if (realResolved && !isInsideRoot(rootRealPath, realResolved)) { + // INVARIANT (single dereference): `fullPath` IS the resolver's + // `operationPath` — the ONE already-dereferenced path it validated, both + // lexically and after realpath, against the boundary that matches its + // scope, plus the fail-closed mandatory-sensitive refusal. Read handlers + // must operate on `operationPath` and must NEVER independently re-resolve + // it: a second `fs.realpath` here would reopen exactly the TOCTOU window + // the single-dereference contract exists to close (a symlink swapped + // between the resolver's realpath and ours would redirect the read to an + // arbitrary file, while the extension gate above only ever sees the + // pre-dereference `relativePath`). `read-files.ts`, `read-logs.ts` and + // `list-directory.ts` already follow this. + // + // The project-root check below is now a redundant ASSERTION on resolver + // output rather than the primary control: a 'project' resolution has + // already been contained against the real project root. An 'external-read' + // (allowlisted readableRoots) or 'owned-temp' resolution is legitimately + // outside the project root and was contained against its own boundary, so + // asserting it against the project root would reject every such read — + // which is exactly the documented read_image support for readableRoots. + if ( + resolvedPath.scope === 'project' && + !isInsideRoot(rootRealPath, fullPath) + ) { return { kind: 'error', entry: { @@ -144,7 +172,9 @@ export async function readImages(params: { }, } } - const safePath = realResolved ?? fullPath + // A non-existent file needs no special handling here: the fs.stat below + // produces the normal DOES_NOT_EXIST error (see its catch). + const safePath = fullPath try { const stats = await fs.stat(safePath) diff --git a/sdk/src/tools/read-logs.ts b/sdk/src/tools/read-logs.ts index b798768e3c..1aff08163b 100644 --- a/sdk/src/tools/read-logs.ts +++ b/sdk/src/tools/read-logs.ts @@ -1,17 +1,35 @@ import * as fs from 'fs' import * as path from 'path' +import { FILE_READ_STATUS } from '@codebuff/common/old-constants' import { jobRegistry } from '@codebuff/common/util/job-registry' import { getBackgroundJob, safeOpenJobLogForRead } from './background-jobs' -import { resolveFilePathForOperation } from './path-utils' +import { + getScopedReadPolicyAliases, + resolveFilePathForReadOperation, +} from './path-utils' +import { isReadPathBlocked } from './read-policy' import type { BackgroundJobOwner } from './background-jobs' +import type { FileFilter } from './read-files' import type { CodebuffToolOutput } from '../../../common/src/tools/list' const DEFAULT_LINES = 200 const DEFAULT_MAX_CHARS = 20_000 const READ_CHUNK_BYTES = 64 * 1024 +/** + * ER-6: multiplier bounding the backward tail scan by BYTES as well as lines. + * `lines` alone cannot bound it — a file with NO newlines never increments + * `lineCount`, so the loop would accumulate the entire file into one growing + * JS string regardless of the `lines` / `max_chars` caps (which only bound the + * returned slice). The scan therefore stops once it has accumulated a small + * multiple of `maxChars`, which is always more than enough to fill the + * returned tail (at most `maxChars`) while keeping a pathological single-line + * file bounded. Floored at one chunk so a tiny `max_chars` still reads the + * last chunk in a single pass. + */ +const MAX_TAIL_SCAN_CHARS_MULTIPLE = 8 /** * Exact file shape produced by `getBackgroundJobFilePath` * (`openbuff-.log` / `.json` in the OS temp dir). The path branch can @@ -36,6 +54,12 @@ type ReadLogsParams = { * (never from model/tool input). Only consulted on the jobId branch. */ owner: BackgroundJobOwner + /** + * ER-4: host read policy, injected by the run dispatch site exactly like + * read_files / read_image / list_directory. Only consulted on the `path` + * branch (see the filter block below). + */ + fileFilter?: FileFilter } export async function readLogs( @@ -122,9 +146,11 @@ export async function readLogs( const requested = params.path // Canonical containment check: in-project paths (including openbuff-owned - // OS temp namespaces such as background-job logs and tmux captures) pass; - // traversal, sibling-prefix and escaping-symlink paths are refused. - const resolved = resolveFilePathForOperation(params.cwd, requested) + // OS temp namespaces such as background-job logs and tmux captures, and + // paths strictly inside an explicitly allowlisted external read root) pass; + // traversal, sibling-prefix and escaping-symlink paths are refused. This is a + // read-only tool, so it uses the read-only resolver. + const resolved = resolveFilePathForReadOperation(params.cwd, requested) if (!resolved) { return [ { @@ -137,6 +163,34 @@ export async function readLogs( ] } + // ER-4: host read policy for the resolved target, applied before the + // ownership gate and before any file content is opened. A non-'project' + // resolution ('owned-temp' / 'external-read') carries an ABSOLUTE + // `relativePath`, so a fileFilter written against project-relative globs + // would never match it and would silently fail OPEN; the scoped + // `/` aliases are what a host policy can actually target. + // The basename comes from the dereferenced `operationPath`, exactly like + // read-image.ts, so both tools present the same key to a host policy. + // Only this `path` branch needs filtering: the jobId branch resolves an + // openbuff-owned artifact under its own ownership gate. + const policyAliases = [ + resolved.relativePath, + ...getScopedReadPolicyAliases(resolved.scope, resolved.operationPath), + ] + if ( + policyAliases.some((alias) => isReadPathBlocked(alias, params.fileFilter)) + ) { + return [ + { + type: 'json', + value: { + path: requested, + errorMessage: FILE_READ_STATUS.IGNORED, + }, + }, + ] + } + // Ownership gate for the owned-temp exception: a resolved path that names a // background-job log/metadata file exposes another session's output, owner // record and full command line, so it must clear the SAME authorization the @@ -208,9 +262,16 @@ function readTail( const { fd, size } = opened try { + const maxScanChars = Math.max( + READ_CHUNK_BYTES, + maxChars * MAX_TAIL_SCAN_CHARS_MULTIPLE, + ) let collected = '' let lineCount = 0 let offset = size + // Set when the scan stopped on the ER-6 byte bound rather than the line + // bound, so earlier file content was deliberately never read. + let stoppedOnByteBound = false while (offset > 0 && lineCount <= lines) { const length = Math.min(READ_CHUNK_BYTES, offset) offset -= length @@ -219,6 +280,11 @@ function readTail( const chunk = buf.toString('utf8') collected = chunk + collected lineCount = (collected.match(/\n/g) ?? []).length + if (collected.length >= maxScanChars) { + // Only a real truncation when bytes before this point stayed unread. + stoppedOnByteBound = offset > 0 + break + } } const endsWithNewline = collected.endsWith('\n') @@ -230,7 +296,7 @@ function readTail( const tail = selectedLines.join('\n') + (endsWithNewline && selectedLines.length > 0 ? '\n' : '') - let truncated = false + let truncated = stoppedOnByteBound let content = tail if (content.length > maxChars) { content = content.slice(content.length - maxChars) diff --git a/sdk/src/tools/terminal-command-policy.ts b/sdk/src/tools/terminal-command-policy.ts index e3d9e5f300..d0c36fe843 100644 --- a/sdk/src/tools/terminal-command-policy.ts +++ b/sdk/src/tools/terminal-command-policy.ts @@ -32,6 +32,12 @@ const WORKSPACE_ENV_DUMP_REASON = const READ_ONLY_ENV_DUMP_REASON = 'dumping or mutating the process environment is not allowed' +/** + * Dump-adjacent utilities. A fragment that cannot be classified structurally + * but still names one of these fails closed instead of being allowed. + */ +const ENV_DUMP_UTILITY_PATTERN = /\b(?:printenv|env|export|set)\b/i + /** * True when `set` arguments are only shell option toggles (`-e`, `+x`, * `-o pipefail`, `-euo pipefail`, …). Positional/`--` forms are not safe: @@ -403,7 +409,7 @@ function findProcessEnvironmentIssue( // untokenizable junk that still names a dumper). Non-dump segments that // merely tokenize poorly stay allowed so ordinary workspace commands are // not false-denied by the env-dump gate. - if (/\b(?:printenv|env|export|set)\b/i.test(trimmed)) { + if (ENV_DUMP_UTILITY_PATTERN.test(trimmed)) { return reason } return undefined @@ -589,11 +595,23 @@ function findProcessEnvironmentIssueInPieces( pieces: string[], style: 'workspace' | 'read-only', ): string | undefined { + const reason = + style === 'workspace' + ? WORKSPACE_ENV_DUMP_REASON + : READ_ONLY_ENV_DUMP_REASON for (const piece of pieces) { const trimmed = piece.trim() if (!trimmed) continue - const segments = splitReadOnlyShellSegments(trimmed) + // Background `&` is a real command separator for this scan: classify every + // job instead of handing `pwd & printenv` to the first-executable resolver. + const segments = splitReadOnlyShellSegments(trimmed, { + backgroundAmpersand: 'split', + }) if (!segments) { + // Still unparseable (unbalanced substitution, dangling separator): fail + // closed when the piece names a dump utility, the same way the + // `__unsafe-tmux-wrapper__` branch does. + if (ENV_DUMP_UTILITY_PATTERN.test(trimmed)) return reason const issue = findProcessEnvironmentIssue(trimmed, style) if (issue) return issue continue @@ -1053,7 +1071,18 @@ function findTraversalPath(command: string): string | undefined { return undefined } -function splitReadOnlyShellSegments(command: string): string[] | undefined { +/** + * Split a command on unquoted `|`, `;`, `&&`, and newlines. A single + * background `&` is rejected by default (read-only containment); callers that + * only need per-command classification pass `backgroundAmpersand: 'split'` to + * treat it as an ordinary separator. + */ +function splitReadOnlyShellSegments( + command: string, + options: { backgroundAmpersand: 'reject' | 'split' } = { + backgroundAmpersand: 'reject', + }, +): string[] | undefined { const segments: string[] = [] let quote: "'" | '"' | null = null let escaped = false @@ -1086,7 +1115,11 @@ function splitReadOnlyShellSegments(command: string): string[] | undefined { ) { continue } - if (char === '&' && command[index + 1] !== '&') { + if ( + char === '&' && + command[index + 1] !== '&' && + options.backgroundAmpersand === 'reject' + ) { return undefined } if ( @@ -1818,10 +1851,16 @@ function findOutsideAbsolutePath( const resolved = path.resolve(token) const tempRoot = path.resolve('/tmp') const relativeToTemp = path.relative(tempRoot, resolved) + // Exempt the temp root itself (`/tmp`, `/tmp/`) as well as anything + // strictly inside it, so bare-`/tmp` operands like `stat -c '%a %U' /tmp` + // and the tmux-cli stale-capture sweep (`find /tmp -maxdepth 1 ...`) are + // tolerated. The gate is the RESOLVED relationship, never a raw `/tmp` + // string prefix: `'/tmpfoo'.startsWith('/tmp')` is true, so a prefix test + // would silently admit siblings like `/tmpfoo` and `/tmpevil/x`, which + // resolve outside the temp root and must stay refused. if ( - token.startsWith('/tmp/') && - !relativeToTemp.startsWith('..') && - !path.isAbsolute(relativeToTemp) + relativeToTemp === '' || + (!relativeToTemp.startsWith('..') && !path.isAbsolute(relativeToTemp)) ) { continue }