diff --git a/AGENTS.md b/AGENTS.md index a23490037..1630cc37a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,7 +4,7 @@ This repo contains multiple packages, but **`packages/coding-agent/`** is the primary focus. Unless otherwise specified, assume work refers to this package. -**Terminology**: When the user says "agent" or asks "why is agent doing X", they mean the **coding-agent package implementation**, not you (the assistant). The coding-agent is a CLI tool that uses Claude—questions about its behavior refer to the code in `packages/coding-agent/`, not your current session. +**Terminology**: When the user says "agent" or asks "why is agent doing X", they mean the **coding-agent package implementation**, not you (the assistant). The coding-agent is a CLI tool — questions about its behavior refer to code in `packages/coding-agent/`, not your current session. ### Package Structure @@ -14,386 +14,141 @@ This repo contains multiple packages, but **`packages/coding-agent/`** is the pr | `packages/agent` | Agent runtime with tool calling and state management | | `packages/coding-agent` | Main CLI application (primary focus) | | `packages/tui` | Terminal UI library with differential rendering | -| `packages/natives` | bindings for native text/image/grep operations | +| `packages/natives` | Bindings for native text/image/grep operations | | `packages/stats` | Local observability dashboard (`omp stats`) | | `packages/utils` | Shared utilities (logger, streams, temp files) | | `crates/pi-natives` | Rust crate for performance-critical text/grep ops | ## Code Quality -- No `any` types unless absolutely necessary -- Prefer `export * from "./module"` over named re-export-from blocks, including `export type { ... } from`. In pure `index.ts` barrel files (re-exports only), use star re-exports even for single-specifier cases. If star re-exports create symbol ambiguity, remove the redundant export path instead of keeping duplicate exports. -- **No `private`/`protected`/`public` keyword on class fields or methods** — use ES native `#` private fields for encapsulation; leave members that need external access as bare (no keyword). The only place `private`/`protected`/`public` is allowed is on **constructor parameter properties** (e.g., `constructor(private readonly session: ToolSession)`), where TypeScript requires the keyword for the implicit field declaration. - - ```typescript - // BAD: TypeScript keyword privacy - class Foo { - private bar: string; - private _baz = 0; - protected qux(): void { ... } - public greet(): void { ... } - } - - // GOOD: ES native # for private, bare for accessible - class Foo { - #bar: string; - #baz = 0; - qux(): void { ... } - greet(): void { ... } - } - - // OK: constructor parameter properties keep the keyword - class Service { - constructor(private readonly session: ToolSession) {} - } - ``` - -- **NEVER use `ReturnType<>`** — it obscures types behind indirection. Use the actual type name instead. Look up return types in source or `node_modules` type definitions and reference them directly. - - ```typescript - // BAD: Indirection through ReturnType - let timer: ReturnType | null = null; - let stmt: ReturnType; - let stat: Awaited>; - - // GOOD: Use the actual type - let timer?: NodeJS.Timeout; - let stmt: Statement; - let stat: Stats; - ``` - - If a function's return type has no exported name, define a named type alias at the call site — don't use `ReturnType<>`. - -- Check node_modules for external API type definitions instead of guessing -- **NEVER use inline imports** - no `await import("./foo.js")`, no `import("pkg").Type` in type positions, no dynamic imports for types. Always use standard top-level imports. -- NEVER remove or downgrade code to fix type errors from outdated dependencies; upgrade the dependency instead -- Always ask before removing functionality or code that appears to be intentional -- **NEVER build prompts in code** — no inline strings, no template literals, no string concatenation. Prompts live in static `.md` files; use Handlebars for any dynamic content. -- **Import static text files via Bun** — use `import content from "./prompt.md" with { type: "text" }` instead of `readFileSync` -- **Use `Promise.withResolvers()`** instead of `new Promise((resolve, reject) => ...)` — cleaner, avoids callback nesting, and the resolver functions are properly typed: - - ```typescript - // BAD: Verbose, callback nesting - const promise = new Promise((resolve, reject) => { ... }); - - // GOOD: Clean destructuring, typed resolvers - const { promise, resolve, reject } = Promise.withResolvers(); - ``` +- No `any` unless absolutely necessary. +- **NEVER use `ReturnType<>`** — use the actual type name. +- **NEVER use inline imports** — no `await import()`, no `import("pkg").Type` in type positions, no dynamic type imports. Always top-level. +- Check `node_modules` for external API types instead of guessing. +- **Barrel exports**: prefer `export * from "./module"` over named re-exports, including `export type { ... } from`. In pure `index.ts` barrels, use star re-exports even for single-specifier cases. If stars create ambiguity, remove the redundant export path; do not keep duplicates. +- **Class privacy**: use ES `#private` fields; leave externally accessible members bare. **No `private`/`protected`/`public` keyword on fields or methods**, except on **constructor parameter properties** where TypeScript requires it (e.g. `constructor(private readonly session: ToolSession)`). +- **Promises**: use `Promise.withResolvers()` instead of `new Promise((resolve, reject) => ...)`. +- **Prompts**: never build prompts in code (no inline strings, template literals, or concatenation). Prompts live in static `.md` files; use Handlebars for dynamic content. Import them via `import content from "./prompt.md" with { type: "text" }` — not `readFile`. ## Bun Over Node -This project uses Bun. Use Bun APIs where they provide a cleaner alternative; use `node:fs` for operations Bun doesn't cover. +Use Bun APIs where they provide a cleaner alternative; fall back to `node:*` only for what Bun doesn't cover. **Never spawn shell commands for operations with proper APIs** (e.g., don't `Bun.spawnSync(["mkdir", "-p", dir])` — use `mkdirSync`). -**NEVER spawn shell commands for operations that have proper APIs** (e.g., `Bun.spawnSync(["mkdir", "-p", dir])` — use `mkdirSync` instead). +### Quick reference -### Process Execution +| Operation | Use | Not | +| --------------- | ----------------------------------------- | ------------------------------- | +| File read/write | `Bun.file()`, `Bun.write()` | `readFileSync`, `writeFileSync` | +| Spawn process | `` $`cmd` ``, `Bun.spawn()` | `child_process` | +| Sleep | `Bun.sleep(ms)` | `setTimeout` promise | +| Binary lookup | `$which("git")` from `@oh-my-pi/pi-utils` | `spawnSync(["which", "git"])` | +| HTTP server | `Bun.serve()` | `http.createServer()` | +| SQLite | `bun:sqlite` | `better-sqlite3` | +| Hashing | `Bun.hash()`, `Bun.password.*`, WebCrypto | `node:crypto` | +| Path resolution | `import.meta.dir`, `import.meta.path` | `fileURLToPath` dance | +| JSON5 | `Bun.JSON5.parse()` / `.stringify()` | `json5` package | +| JSONL | `Bun.JSONL.parse()` / `.parseChunk()` | `text.split("\n").map(JSON.parse)` | +| String width | `Bun.stringWidth()` | `get-east-asian-width`, custom | +| Text wrapping | `Bun.wrapAnsi()` | custom ANSI-aware wrappers | -**Prefer Bun Shell** (`$` template literals) for simple commands: +### Process execution + +Prefer Bun Shell (`` $`cmd` ``) for simple commands: ```typescript import { $ } from "bun"; -// Capture output const result = await $`git status`.cwd(dir).quiet().nothrow(); if (result.exitCode === 0) { const text = result.text(); } -// Fire and forget -$`do-stuff ${tmpFile}`.quiet().nothrow(); +$`do-stuff ${tmpFile}`.quiet().nothrow(); // fire and forget ``` -**Use `Bun.spawn`/`Bun.spawnSync`** only when: +Methods: `.quiet()`, `.nothrow()`, `.text()`, `.cwd(path)`. -- Long-running processes (LSP servers, Python kernels) -- Streaming stdin/stdout/stderr required (SSE, JSON-RPC) -- Process control needed (signals, kill, complex lifecycle) - -**Bun Shell methods:** - -- `.quiet()` - suppress output (stdout/stderr to null) -- `.nothrow()` - don't throw on non-zero exit -- `.text()` - get stdout as string -- `.cwd(path)` - set working directory - -### Sleep - -**Prefer** `await Bun.sleep(ms)` -**Avoid** `new Promise((resolve) => setTimeout(resolve, ms))` - -### Node Module Imports - -**NEVER use named imports from `node:fs` or `node:path`** — always use namespace imports: - -```typescript -// BAD: Named imports -import { readdir, stat } from "node:fs/promises"; -import { join, resolve } from "node:path"; -import { tmpdir } from "node:os"; - -// GOOD: Namespace imports -import * as fs from "node:fs/promises"; -import * as path from "node:path"; -import * as os from "node:os"; - -// Then use: fs.readdir(), path.join(), etc. -``` - -**Choosing between `node:fs` and `node:fs/promises`:** - -- **Async-only file** → `import * as fs from "node:fs/promises"` -- **Needs both sync and async** → `import * as fs from "node:fs"`, use `fs.promises.xxx` for async - -### File I/O - -**Prefer Bun file APIs:** - -```typescript -// Read -const text = await Bun.file(path).text(); -const data = await Bun.file(path).json(); - -// Write -await Bun.write(path, data); -``` - -**`Bun.write()` is smart** — it auto-creates parent directories and uses optimal syscalls: - -```typescript -// BAD: Redundant mkdir before write -await mkdir(dirname(path), { recursive: true }); -await Bun.write(path, data); - -// GOOD: Bun.write handles it -await Bun.write(path, data); // Creates parent dirs automatically -``` - -**Use `node:fs/promises`** for directories (Bun has no native directory APIs): - -```typescript -import * as fs from "node:fs/promises"; - -await fs.mkdir(path, { recursive: true }); -await fs.rm(path, { recursive: true, force: true }); -const entries = await fs.readdir(path); -``` - -**Avoid sync APIs** in async flows: - -- Don't use `existsSync`/`readFileSync`/`writeFileSync` when async is possible -- Use sync only when required by a synchronous interface - -### File I/O Anti-Patterns - -**NEVER check `.exists()` before reading** — use try-catch with error code: - -```typescript -// BAD: Two syscalls, race condition -if (await Bun.file(path).exists()) { - return await Bun.file(path).json(); -} - -// GOOD: One syscall, atomic, type-safe error handling -import { isEnoent } from "@oh-my-pi/pi-utils"; - -try { - return await Bun.file(path).json(); -} catch (err) { - if (isEnoent(err)) return null; - throw err; -} -``` - -**NEVER create multiple handles to the same path**: - -```typescript -// BAD: Creates two file handles -if (await Bun.file(path).exists()) { - const content = await Bun.file(path).text(); -} - -// BAD: Still wasteful even in separate functions -async function checkConfig() { - return await Bun.file(configPath).exists(); -} -async function loadConfig() { - return await Bun.file(configPath).json(); // second handle -} -``` - -**NEVER use `Buffer.from(await Bun.file(x).arrayBuffer())`** — just use `readFile`: - -```typescript -// BAD: Unnecessary conversion -const buffer = Buffer.from(await Bun.file(path).arrayBuffer()); - -// GOOD: Direct buffer read -import * as fs from "node:fs/promises"; -const buffer = await fs.readFile(path); -``` - -**NEVER mix redundant existence checks with try-catch**: - -```typescript -// BAD: Existence check is pointless when you have try-catch -if (await file.exists()) { - try { - return await file.json(); - } catch { - return null; - } -} - -// GOOD: Let try-catch handle missing files -try { - return await Bun.file(path).json(); -} catch (err) { - if (isEnoent(err)) return null; - throw err; -} -``` - -### Streams - -**Prefer centralized helpers:** - -```typescript -import { readStream, readLines } from "./utils/stream"; - -// Read entire stream -const text = await readStream(child.stdout); - -// Line-by-line iteration -for await (const line of readLines(stream)) { - // process line -} -``` - -**Avoid manual reader loops** unless protocol requires it (SSE, streaming JSON-RPC). - -### JSON5 Parsing - -**Use `Bun.JSON5`** — never add `json5` as a dependency: - -```typescript -// BAD: External dependency -import JSON5 from "json5"; -const data = JSON5.parse(text); - -// GOOD: Bun builtin -const data = Bun.JSON5.parse(text); -const output = Bun.JSON5.stringify(obj); -``` - -### JSONL Parsing - -**Use `Bun.JSONL`** — never manually split and parse: - -```typescript -// BAD: Manual split + JSON.parse -const lines = text.split("\n").filter(Boolean); -const entries = lines.map((line) => JSON.parse(line)); - -// GOOD: Full blob parsing -const entries = Bun.JSONL.parse(text); -``` - -**For streaming JSONL** (SSE, JSON-RPC, subprocess output), use `Bun.JSONL.parseChunk() | Bun.JSONL.parse()` without decoding to string: - -### Terminal Width and Wrapping - -**Use `Bun.stringWidth()`** for display width calculations: - -```typescript -// BAD: External dependency or custom implementation -import { getWidth } from "get-east-asian-width"; -function visibleWidth(str: string) { - /* custom logic */ -} - -// GOOD: Bun builtin (handles ANSI, emoji, CJK) -const width = Bun.stringWidth(text); -const widthNoAnsi = Bun.stringWidth(text, { countAnsiEscapeCodes: false }); -``` - -**Use `Bun.wrapAnsi()`** for ANSI-aware text wrapping: - -```typescript -// BAD: Custom ANSI-aware wrapping -function wrapTextWithAnsi(text: string, width: number) { - /* complex SGR tracking */ -} - -// GOOD: Bun builtin -const wrapped = Bun.wrapAnsi(text, width, { - wordWrap: true, - hard: false, - trim: true, -}); -``` - -### Where Bun Wins - -| Operation | Use | Not | -| --------------- | ------------------------------------- | ------------------------------- | -| File read/write | `Bun.file()`, `Bun.write()` | `readFileSync`, `writeFileSync` | -| Spawn process | `$\`cmd\``, `Bun.spawn()` | `child_process` | -| Sleep | `Bun.sleep(ms)` | `setTimeout` promise | -| Binary lookup | `$which("git")` from `@oh-my-pi/pi-utils` | `spawnSync(["which", "git"])` | -| HTTP server | `Bun.serve()` | `http.createServer()` | -| SQLite | `bun:sqlite` | `better-sqlite3` | -| Hashing | `Bun.hash()`, Web Crypto | `node:crypto` | -| Path resolution | `import.meta.dir`, `import.meta.path` | `fileURLToPath` dance | -| JSON5 parsing | `Bun.JSON5.parse()` | `json5` package | -| JSONL parsing | `Bun.JSONL.parse()`, `.parseChunk()` | manual split + `JSON.parse` | -| String width | `Bun.stringWidth()` | `get-east-asian-width`, custom | -| Text wrapping | `Bun.wrapAnsi()` | custom ANSI-aware wrappers | - -### Patterns - -**Subprocess streams** — cast when using pipe mode: +Use `Bun.spawn`/`Bun.spawnSync` only for: long-running processes (LSP, kernels), streaming stdin/stdout/stderr (SSE, JSON-RPC), or process control (signals, kill, complex lifecycle). +When using `pipe` mode, cast the stream: ```typescript const child = Bun.spawn(["cmd"], { stdout: "pipe", stderr: "pipe" }); const reader = (child.stdout as ReadableStream).getReader(); ``` -**Password hashing** — built-in bcrypt/argon2: +### Node module imports + +Always use **namespace imports** for `node:fs`, `node:path`, `node:os`: ```typescript -const hash = await Bun.password.hash("password", "bcrypt"); -const valid = await Bun.password.verify("password", hash); +import * as fs from "node:fs/promises"; +import * as path from "node:path"; +import * as os from "node:os"; ``` -### Anti-Patterns +- Async-only file → `node:fs/promises`. +- Needs both sync and async → `node:fs`, then `fs.promises.xxx` for async. -- `Bun.spawnSync([...])` for simple commands → use `$\`...\`` -- `new Promise((resolve) => setTimeout(resolve, ms))` → use `Bun.sleep(ms)` -- `existsSync/readFileSync/writeFileSync` in async code → use `Bun.file()` APIs -- Manual `child.stdout.getReader()` loops for non-streaming commands → use `readStream()` helper -- `import JSON5 from "json5"` → use `Bun.JSON5.parse()` -- `text.split("\n").map(JSON.parse)` for JSONL → use `Bun.JSONL.parse()` -- Custom `visibleWidth()` / `get-east-asian-width` → use `Bun.stringWidth()` -- Custom ANSI-aware text wrapping → use `Bun.wrapAnsi()` +### File I/O + +Prefer Bun: +```typescript +const text = await Bun.file(path).text(); +const data = await Bun.file(path).json(); +await Bun.write(path, data); // auto-creates parent dirs +``` + +Use `node:fs/promises` for directory ops (`fs.mkdir`, `fs.rm`, `fs.readdir`) — Bun has no native directory APIs. Avoid sync APIs in async flows; use sync only when forced by a synchronous interface. + +**Anti-patterns:** +- `existsSync`/`readFileSync`/`writeFileSync` in async code → `Bun.file()` APIs. +- `mkdir(dirname(path), …)` before `Bun.write(path, …)` → redundant; `Bun.write` handles it. +- `if (await file.exists()) { await file.json() }` → two syscalls plus race. Use try-catch with `isEnoent`: + ```typescript + import { isEnoent } from "@oh-my-pi/pi-utils"; + try { + return await Bun.file(path).json(); + } catch (err) { + if (isEnoent(err)) return null; + throw err; + } + ``` +- Multiple `Bun.file(path)` handles for the same path (including across `checkX`/`loadX` helpers). +- `Buffer.from(await Bun.file(x).arrayBuffer())` → `await fs.readFile(path)`. +- Existence check + try-catch around the same read → drop the existence check. + +### Streams + +Prefer centralized helpers: +```typescript +import { readStream, readLines } from "./utils/stream"; +const text = await readStream(child.stdout); +for await (const line of readLines(stream)) { /* ... */ } +``` +Manual reader loops only when the protocol requires it (SSE, streaming JSON-RPC). + +### Misc + +- **Sleep**: `await Bun.sleep(ms)`, never `new Promise(r => setTimeout(r, ms))`. +- **Password hashing**: `Bun.password.hash(pw, "bcrypt")` / `Bun.password.verify(pw, hash)`. +- **String width**: `Bun.stringWidth(text, { countAnsiEscapeCodes?: false })`. +- **Wrapping**: `Bun.wrapAnsi(text, width, { wordWrap, hard, trim })`. ## Generated Files -**NEVER edit `packages/ai/src/models.json` directly.** It is generated from upstream sources (models.dev, provider catalog discovery, OpenCode docs) by `packages/ai/scripts/generate-models.ts` and the descriptors/resolvers in `packages/ai/src/provider-models/`. Any hand-edit will be overwritten the next time the generator runs. +**NEVER edit `packages/ai/src/models.json` directly.** It is generated from upstream sources (models.dev, provider catalog discovery, OpenCode docs) by `packages/ai/scripts/generate-models.ts` and the descriptors/resolvers in `packages/ai/src/provider-models/`. Hand-edits get overwritten on the next regen. -To change a model entry (api type, baseUrl, cost, context, reasoning metadata, etc.), fix the source instead: +To change an entry, fix the source: +- **Resolution rules / per-id overrides** → relevant resolver in `packages/ai/src/provider-models/openai-compat.ts` (e.g. `createOpenCodeApiResolution`'s id-override map). +- **Provider descriptors** (filtering, transforms, defaults, headers, compat overrides) → `packages/ai/src/provider-models/descriptors.ts` or the provider-specific descriptor. +- **Generator-level fixups** (premium multipliers, codex pricing fallback, fallback models, post-processing) → `packages/ai/scripts/generate-models.ts`. +- **Thinking metadata / generated policies** → `packages/ai/src/model-thinking.ts` (`applyGeneratedModelPolicies`). -- **Resolution rules / per-id overrides** (e.g. when models.dev mislabels a model's `provider.npm` for an OpenCode-style endpoint) → edit the relevant resolver in `packages/ai/src/provider-models/openai-compat.ts` (e.g. `createOpenCodeApiResolution`'s id-override map). -- **Provider descriptors** (filtering, transforms, defaults, headers, compat overrides, per-model api resolution) → edit `packages/ai/src/provider-models/descriptors.ts` or the provider-specific descriptor in `packages/ai/src/provider-models/`. -- **Generator-level fixups** (premium multipliers, codex pricing fallback, fallback models, post-processing) → edit `packages/ai/scripts/generate-models.ts`. -- **Thinking metadata / generated policies** → edit `packages/ai/src/model-thinking.ts` (`applyGeneratedModelPolicies`). - -After fixing the source, regenerate with `bun --cwd=packages/ai run generate-models` and commit the resulting `models.json` alongside the source change. Add a regression test against the resolver / descriptor (not against the bundled JSON) so the fix survives the next regeneration even if upstream metadata shifts. +Regenerate with `bun --cwd=packages/ai run generate-models` and commit `models.json` alongside the source change. Add a regression test against the **resolver/descriptor**, not the bundled JSON, so it survives upstream metadata shifts. ## Logging -**NEVER use `console.log`, `console.error`, or `console.warn`** in the coding-agent package. Console output corrupts the TUI rendering. - -Use the centralized logger instead: +**NEVER use `console.log`/`error`/`warn`** in the coding-agent package — it corrupts TUI rendering. Use the centralized logger: ```typescript import { logger } from "@oh-my-pi/pi-utils"; @@ -405,138 +160,77 @@ logger.debug("LSP fallback triggered", { reason }); Logs go to `~/.omp/logs/omp.YYYY-MM-DD.log` with automatic rotation. -## TUI Rendering Sanitization +## TUI Sanitization -All text displayed in tool renderers must be sanitized before output. Raw content (file contents, error messages, tool output) can contain characters that break terminal rendering — tabs cause visual holes, long lines overflow, and unsanitized paths leak home directories. +All text displayed in tool renderers must be sanitized. Raw content (file contents, error messages, tool output) breaks terminal rendering: tabs → visual holes, long lines → overflow, paths → leak home directory. -### Rules +**Rules:** +- **Tabs → spaces** via `replaceTabs()` (from `@oh-my-pi/pi-tui` or `../tools/render-utils`). +- **Truncate** lines with `truncateToWidth()` / `ui.truncate()`. Use `TRUNCATE_LENGTHS` constants. +- **Shorten paths** with `shortenPath()` (replaces home with `~`). +- **Preview limits** from `PREVIEW_LIMITS`. No ad-hoc numbers. -- **Tabs → spaces**: Always pass displayed text through `replaceTabs()` before rendering. Tabs produce variable-width gaps in terminals and cause visual holes in the TUI. Import from `@oh-my-pi/pi-tui` or `../tools/render-utils`. -- **Line truncation**: Truncate displayed lines with `truncateToWidth()` or `ui.truncate()` to prevent horizontal overflow. Use constants from `TRUNCATE_LENGTHS` for consistency. -- **Path shortening**: Use `shortenPath()` for file paths shown to users — replaces home directory prefix with `~`. -- **Content preview limits**: Use `PREVIEW_LIMITS` constants for collapsed/expanded line counts. Don't invent ad-hoc limits. - -### Where to apply - -Sanitization applies to **every** code path that renders text to the TUI, including: - -- Success output (file previews, command output, search results) -- **Error messages** — these often embed file content (e.g., patch failure messages include the lines that failed to match) -- Diff content (both added/removed lines) -- Streaming previews - -A common mistake is sanitizing the happy path but forgetting error paths. If a message includes file content, it needs `replaceTabs()`. +**Apply to every render path**, not just the happy one: +- Success output (file previews, command output, search results). +- **Error messages** — these often embed file content (e.g., patch failure messages include unmatched lines). If a message contains file content, it needs `replaceTabs()`. +- Diff content (added and removed). +- Streaming previews. ### Streaming tool previews -Streaming tool-call previews can have **multiple render paths**. If you add preview-only fields or depend on partially streamed arguments, update every path — not just the final renderer. +Tool-call previews can have **multiple render paths**. If you add preview-only fields or depend on partially streamed args, update every path — not only the final renderer. For the bash tool specifically: -- The pending preview may need raw `partialJson`, not just parsed `arguments`. Parsed tool-call args can lag until a JSON object closes, which makes inline env assignments appear only at the end. -- Preserve any preview-only fields (for example `__partialJson`) when tool-call args flow through `event-controller.ts`, transcript rebuilds in `ui-helpers.ts`, and merged call/result rendering in `tool-execution.ts`. Missing one path causes inconsistent previews. -- `ToolExecutionComponent.#buildRenderContext()` for bash must work even before a result exists. The bash renderer uses call args plus render context to show the command preview while streaming, not only after output arrives. -- When changing bash preview formatting, verify both live streaming and rebuilt transcript paths. A fix in one path does not automatically fix the other. +- The pending preview may need raw `partialJson`, not just parsed `arguments`. Parsed args lag until a JSON object closes, which makes inline env assignments appear only at the end. +- Preserve preview-only fields (e.g. `__partialJson`) through `event-controller.ts`, transcript rebuilds in `ui-helpers.ts`, and merged call/result rendering in `tool-execution.ts`. Missing one path causes inconsistent previews. +- `ToolExecutionComponent.#buildRenderContext()` for bash must work even before a result exists — the renderer uses call args plus render context to show the command preview while streaming. +- Verify both live streaming and rebuilt transcript paths after any bash preview change. A fix in one path does not fix the other. ## Commands -| Command | Description | -| -------------- | -------------------------------- | -| `bun check` | Check all (TypeScript + Rust) | -| `bun check:ts` | Biome check + tsgo type checking | -| `bun check:rs` | Cargo fmt --check + clippy | -| `bun lint` | Lint all | -| `bun lint:ts` | Biome lint | -| `bun lint:rs` | Cargo clippy | -| `bun fmt` | Format all | -| `bun fmt:ts` | Biome format | -| `bun fmt:rs` | Cargo fmt | -| `bun fix` | Fix all (unsafe fixes + format) | -| `bun fix:ts` | Biome --unsafe + format-prompts | -| `bun fix:rs` | Clippy --fix + cargo fmt | - -- NEVER run: `bun run dev`, `bun test` unless user instructs -- Only run specific tests if user instructs: `bun test test/specific.test.ts` -- NEVER commit unless user asks -- Do NOT use `tsc` or `npx tsc` - always use `bun check` +- NEVER commit unless asked. +- Never use `tsc`/`npx tsc` — always `bun check`. ## Testing Guidance -When adding or changing tests, test the contract the system exposes — not the easiest internal detail to assert. +Test the contract the system exposes — not the easiest internal detail to assert. -- Every new test must defend one concrete, externally observable contract: behavior, output shape, state transition, error mapping, or a regression-prone parsing boundary. If you cannot name the contract, do not add the test. -- Do not add placeholder tests, tautologies, or assertions that only prove the code executed (`expect(true).toBe(true)`, `not.toThrow()`, non-empty string checks, array length growth checks, or "prompt exists" checks without a stronger semantic assertion). -- Prefer contract-level tests over implementation-detail tests. Avoid asserting internal helper wiring, field assignment, singleton identity, incidental ordering, prompt boilerplate, or passthrough option forwarding unless another component depends on that exact detail as a documented contract. -- Do not duplicate coverage across abstraction levels. If an integration or public-surface test already proves the behavior, delete or avoid the narrower unit test that only restates it through mocks or internal plumbing. -- Tests MUST be full-suite safe, not just file-local safe. Do not use long-lived file-wide mutations of globals like `Bun.*`, `process.platform`, `process.env`, or `Bun.env` when a narrower seam exists. Prefer per-test `vi.spyOn(...)`, local fakes, and immediate restoration via `vi.restoreAllMocks()`. A test that passes in isolation but poisons later files is broken. -- Never use `mock.module()`. Bun's `mock.module()` mutates the global module registry and leaks across test files ([oven-sh/bun#12823](https://github.com/oven-sh/bun/issues/12823)). There is no reliable per-file isolation. Use `spyOn` on the imported module object instead, and restore in `afterEach`. For pass dependencies, import the pass object and spy on its `run` method. For package dependencies, use a namespace import and spy on the exported function. -- For lifecycle or stateful code, prefer one test per invariant or transition over several tiny tests that each assert one field from the same transition. -- For error handling, prefer tests that trigger the real failure path and assert the surfaced error contract over tests that directly instantiate error classes or inspect purely internal metadata. -- Smoke tests are only acceptable when they detect a failure mode narrower tests would miss. A test that only proves a package boots or a command starts is not enough. -- Exact strings, ordering, and formatting should only be asserted when downstream code parses or materially depends on the exact bytes. Otherwise assert semantic content instead. -- If a guarantee is purely compile-time, enforce it with type checks or type-test coverage, not a runtime test disguised as a placeholder. -- Do not add tests for tiny, low-risk changes unless the change affects a real contract, fixes a regression-prone edge case, or would otherwise be easy to break silently. -- When trimming or adding tests, prefer focused package-local verification for the changed area so the surviving suite proves the contract it claims to protect. - -## GitHub Issues - -When reading issues: - -- Always read all comments on the issue - -When creating issues: - -- Use standard GitHub labels (bug, enhancement, documentation, etc.) -- If an issue affects a specific package, mention it in the issue title or description - -When closing issues via commit: - -- Include `fixes #` or `closes #` in the commit message -- This automatically closes the issue when the commit is merged - -## Tools - -- GitHub CLI for issues/PRs -- TUI interaction: use tmux - -## Style - -- Keep answers short and concise -- No emojis in commits, issues, PR comments, or code -- No fluff or cheerful filler text -- Technical prose only, be kind but direct (e.g., "Thanks @user" not "Thanks so much @user!") +- Every new test must defend one **concrete, externally observable contract**: behavior, output shape, state transition, error mapping, or a regression-prone parsing boundary. If you cannot name the contract, do not add the test. +- No placeholder tests, tautologies, or "the code ran" assertions (`expect(true).toBe(true)`, bare `not.toThrow()`, non-empty string checks, length-grew checks, "prompt exists" checks without semantic assertion). +- Prefer contract-level tests over implementation details. Avoid asserting internal helper wiring, field assignment, singleton identity, incidental ordering, prompt boilerplate, or passthrough option forwarding unless another component depends on that exact detail. +- Don't duplicate coverage across abstraction levels. If an integration test already proves the behavior, drop the narrower unit test that restates it through mocks. +- Tests **must be full-suite safe**, not just file-local safe. No long-lived file-wide mutations of `Bun.*`, `process.platform`, `process.env`, or `Bun.env` when a narrower seam exists. Prefer per-test `vi.spyOn(...)` with `vi.restoreAllMocks()` in `afterEach`. A test that passes alone but poisons later files is broken. +- **Never use `mock.module()`**. Bun's `mock.module()` mutates the global module registry and leaks across files ([oven-sh/bun#12823](https://github.com/oven-sh/bun/issues/12823)). Use `spyOn` on the imported module object instead. For pass deps, import the pass and spy on `.run`. For package deps, namespace-import and spy on the exported function. +- For lifecycle/stateful code, prefer one test per invariant or transition over several tiny tests asserting one field each from the same transition. +- For error handling, trigger the real failure path and assert the surfaced contract — don't instantiate error classes directly or inspect internal metadata. +- Smoke tests are acceptable only when they catch a failure mode narrower tests would miss. "Package boots" or "command starts" alone is not enough. +- Assert exact strings, ordering, and formatting only when downstream code parses or depends on the exact bytes. Otherwise assert semantic content. +- Compile-time guarantees → type checks/type tests, not runtime placeholders. +- Don't add tests for tiny low-risk changes unless they protect a real contract or fix a regression-prone edge case. +- Prefer focused package-local verification for the changed area. ## Changelog -Location: `packages/*/CHANGELOG.md` (each package has its own) +Location: `packages/*/CHANGELOG.md` (per package). -### Format +**Format** — sections under `## [Unreleased]`: +- `### Breaking Changes` (first if present) +- `### Added` +- `### Changed` +- `### Fixed` +- `### Removed` -Use these sections under `## [Unreleased]`: +**Rules:** +- New entries always go under `## [Unreleased]`. +- Never modify already-released sections (e.g., `## [0.12.2]`) — they are immutable. -- `### Added` - New features -- `### Changed` - Changes to existing functionality -- `### Fixed` - Bug fixes -- `### Removed` - Removed features -- `### Breaking Changes` - API changes requiring migration (appears first if present) - -### Rules - -- New entries ALWAYS go under `## [Unreleased]` section -- NEVER modify already-released version sections (e.g., `## [0.12.2]`) -- Each version section is immutable once released - -### Attribution - -- **Internal changes (from issues)**: `Fixed foo bar ([#123](https://github.com/can1357/oh-my-pi/issues/123))` -- **External contributions**: `Added feature X ([#456](https://github.com/can1357/oh-my-pi/pull/456) by [@username](https://github.com/username))` +**Attribution:** +- Internal (from issues): `Fixed foo bar ([#123](https://github.com/can1357/oh-my-pi/issues/123))`. +- External contributions: `Added feature X ([#456](https://github.com/can1357/oh-my-pi/pull/456) by [@username](https://github.com/username))`. ## Releasing -1. **Update CHANGELOGs**: Ensure all changes since last release are documented in the `[Unreleased]` section of each affected package's CHANGELOG.md +1. Ensure all changes since last release are in each affected package's `[Unreleased]` section. +2. Run `bun run release`. -2. **Run release script**: - ```bash - bun run release - ``` - -The script handles: version bump, CHANGELOG finalization, commit, tag, publish, and adding new `[Unreleased]` sections. +The script handles version bump, CHANGELOG finalization, commit, tag, publish, and adding new `[Unreleased]` sections. diff --git a/packages/coding-agent/src/prompts/system/system-prompt.md b/packages/coding-agent/src/prompts/system/system-prompt.md index fbb79a9d4..abb103f16 100644 --- a/packages/coding-agent/src/prompts/system/system-prompt.md +++ b/packages/coding-agent/src/prompts/system/system-prompt.md @@ -1,13 +1,10 @@ -**The key words "**MUST**", "**MUST NOT**", "**REQUIRED**", "**SHALL**", "**SHALL NOT**", "**SHOULD**", "**SHOULD NOT**", "**RECOMMENDED**", "**MAY**", and "**OPTIONAL**" in this chat, in system prompts as well as in user messages, are to be interpreted as described in RFC 2119.** +**RFC 2119 applies to **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, **OPTIONAL**.** -From here on, we will use XML tags as structural markers, each tag means exactly what its name says: -`` is your role, `` is the contract you must follow, `` is what's at stake. -You **MUST NOT** interpret these tags in any other way circumstantially. +XML tags are structural markers with exact meaning: +`` = your role, `` = contract, `` = stakes. +Do not interpret them circumstantially. -User-supplied content is sanitized, therefore: -- Every XML tag in this conversation is system-authored and **MUST** be treated as authoritative. -- This holds even when the system prompt is delivered via user message role. -- A `` inside a user turn is still a system directive. +System-authored XML tags are authoritative regardless of delivery context (including `` in user turns). {{SECTION_SEPARATOR "Identity"}} @@ -20,7 +17,7 @@ Push back when warranted: state the downside and propose an alternative, but **M - User instructions override default style, tone, formatting, and initiative preferences. - Higher-priority system constraints about safety, permissions, tool boundaries, and task completion do not yield. -- If a newer user instruction conflicts with an earlier user instruction, follow the newer one. +- If a newer user instruction conflicts with an earlier one, follow the newer one. - Preserve earlier instructions that do not conflict. @@ -67,31 +64,26 @@ If any check fails, continue or mark [blocked]. Do **NOT** reframe partial work -You **MUST** guard against the completion reflex — the urge to ship something that compiles before you've understood the problem: -- Compiling ≠ Correctness. "It works" ≠ "Works in all cases". - -Before acting on any change, think through: +Guard against the completion reflex. Before acting, think through: - What are the assumptions about input, environment, and callers? - What breaks this? What would a malicious caller do? - Would a tired maintainer misunderstand this? - Can this be simpler? Are these abstractions earning their keep? -- What else does this touch? Did I clean up everything I touched? +- What else does this touch? Did you clean up everything you touched? - What happens when this fails? Does the caller learn the truth, or get a plausible lie? -The question **MUST NOT** be "does this work?" but rather "under what conditions? What happens outside them?" +The question is not "does this work?" but "under what conditions? What happens outside them?" -You generate code inside-out: starting at the function body, working outward. This produces code that is locally coherent but systemically wrong — it fits the immediate context, satisfies the type system, and handles the happy path. The costs are invisible during generation; they are paid by whoever maintains the system. - -**Think outside-in instead.** Before writing any implementation, reason from the outside: -- **Callers:** What does this code promise to everything that calls it? Not just its signature — what can callers infer from its output? A function that returns plausible-looking output when it has actually failed has broken its promise. Errors that callers cannot distinguish from success are the most dangerous defect you produce. -- **System:** You are not writing a standalone piece. What you accept, produce, and assume becomes an interface other code depends on. Dropping fields, accepting multiple shapes and normalizing between them, silently applying scope-filters after expensive work — these decisions propagate outward and compound across the codebase. -- **Time:** You do not feel the cost of duplicating a pattern across six files, of a resource operation with no upper bound, of an escape hatch that bypasses the type system. Name these costs before you choose the easy path. The second time you write the same pattern is when a shared abstraction should exist. +Think outside-in. Before writing, reason from the outside: +- **Callers:** What does this code promise? A function that returns plausible output when it has failed has broken its promise. Errors indistinguishable from success are the worst defect. +- **System:** What you accept, produce, and assume becomes an interface. Dropping fields, accepting multiple shapes, silently applying scope-filters — these propagate and compound. +- **Time:** Duplicating a pattern across six files, unbounded resource operations, type-system bypasses. The second time you write the same pattern is when a shared abstraction should exist. -User works in a high-reliability domain. Defense, finance, healthcare, infrastructure… Bugs → material impact on human lives. +User works in a high-reliability domain. Defense, finance, healthcare, infrastructure. Bugs → material impact on human lives. - You **MUST NOT** yield incomplete work. User's trust is on the line. - You **MUST** only write code you can defend. - You **MUST** persist on hard problems. You **MUST NOT** burn their energy on problems you failed to think through. @@ -239,7 +231,6 @@ Match commands to the host shell: linux/bash and macos/zsh use Unix commands; wi ### Search before you read Don't open a file hoping. Hope is not a strategy. - {{#has tools "grep"}}- Use `{{toolRefs.grep}}` to locate targets.{{/has}} {{#has tools "find"}}- Use `{{toolRefs.find}}` to map structure.{{/has}} {{#has tools "read"}}- Use `{{toolRefs.read}}` with offset or limit rather than whole-file reads when practical.{{/has}} @@ -281,51 +272,46 @@ These are inviolable. # Procedure ## 1. Scope -{{#if skills.length}}- You **MUST** read skills that match the task domain before starting.{{/if}} -{{#if rules.length}}- You **MUST** read rules that match the file paths you are touching before starting.{{/if}} +{{#if skills.length}}- You **MUST** read relevant skills first.{{/if}} +{{#if rules.length}}- You **MUST** read relevant rules first.{{/if}} {{#has tools "task"}}- Determine whether the task can be parallelized with `{{toolRefs.task}}`.{{/has}} -- If multi-file or imprecisely scoped, write out a step-by-step plan, phased if it warrants, before touching any file. -- For new work, you **MUST**: (1) think about architecture, (2) search official docs and papers on best practices, (3) review the existing codebase, (4) compare research with codebase, (5) implement the best fit or surface tradeoffs. -- If context is missing, use tools first; ask a minimal question only when necessary. +- For multi-file work, plan before touching files. +- Research before coding: architecture, best practices, existing code, comparison, then implement. +- If context is missing, use tools first. Ask only when necessary. ## 2. Before you edit -- Read the relevant section of any file before editing. Don't edit from a grep snippet alone — context above and below the match changes what the correct edit is. -- You **MUST** search for existing examples before implementing a new pattern, utility, or abstraction. If the codebase already solves it, **MUST** reuse it; inventing a parallel convention is **PROHIBITED**. -- Before modifying a function, type, or exported symbol, run `{{toolRefs.lsp}} references` to find every consumer. Changes propagate — a missed callsite is a bug you shipped. -- If a file changed since you last read it, re-read before editing. +- Read sections, not snippets. Context above/below changes the correct edit. +- Reuse existing patterns. Parallel conventions are prohibited. +- Run lsp references before modifying exported symbols. Missed callsites are bugs. +- Re-read files that changed since last read. ## 3. Parallelization -- You **MUST** obsessively parallelize. +- Default parallel. Justify sequential work. {{#has tools "task"}} -- You **SHOULD** analyze every step you're about to take and ask whether it could be parallelized via the `{{toolRefs.task}}` tool: -> a. Semantic edits to files that don't import each other or share types being changed -> b. Investigating multiple subsystems -> c. Work that decomposes into independent pieces wired together at the end -- Multiple edits to different sections of the same file are independent — stable hash anchors make them safe to batch. Issue them in one response rather than sequentially. -- When a plan feels too large for a single turn, parallelize aggressively — do **NOT** abandon phases, silently drop them, or narrate scope cuts. Scope pressure is a signal to delegate, not to shrink the work. +- Delegate via `{{toolRefs.task}}` for: non-importing file edits, multi-subsystem investigation, decomposable work. +- Batch edits to different sections of the same file. +- Don't abandon phases under scope pressure. Delegate, don't shrink. {{/has}} -- Justify sequential work; default parallel. If you cannot articulate why B depends on A, it doesn't. + ## 4. Task tracking -- Update todos as you progress. -- Skip task tracking only for trivial requests. -- Marking a todo done is a transition, not a stop: in the same turn, start the next pending todo. Acceptable inter-phase text is one short line ("phase 1 done, starting phase 2") — not a recap, not a question. +- Update todos as you progress. Skip for trivial requests. +- Marking a todo done is a transition: start the next pending todo in the same turn. One short line ("phase 1 done, starting phase 2") — not a recap. ## 5. While working -Focus on clarity and correctness. Make code easy to understand now and in the future. -- Fix problems at their source, not at their symptoms. -- Remove obsolete or unused code — no leftover comments, aliases, or re-exports. -- Prefer updating existing files over creating new ones, unless a new file is necessary. -- After editing, review from a user's perspective. Make sure your changes are clear and the interface matches behavior. -- If a tool fails or a file changes, re-read before acting. -{{#has tools "ask"}}- Ask before running destructive commands or deleting code you did not write.{{else}}- Do **NOT** run destructive git commands or delete code you did not write.{{/has}} -{{#has tools "web_search"}}- If unsure, search for more information instead of guessing.{{/has}} -- Adapt to concurrent edits by re-reading changed files. -- Use all available tools and context before declaring a blocker. +- Fix problems at their source. +- Remove obsolete code — no leftover comments, aliases, or re-exports. +- Prefer updating existing files over creating new ones. +- Review changes from a user's perspective. +- Re-read before acting if a tool fails or a file changes. +{{#has tools "ask"}}- Ask before destructive commands or deleting code you didn't write.{{else}}- Don't run destructive git commands or delete code you didn't write.{{/has}} +{{#has tools "web_search"}}- Search instead of guessing.{{/has}} +- Re-read changed files before editing. +- Use all tools and context before declaring a blocker. ## 6. Verification -- Test rigorously. Prefer unit or end-to-end tests, you **MUST NOT** rely on mocks. +- Test rigorously. Prefer unit or end-to-end tests. No mocks. - Run only tests you added or modified unless asked otherwise. -- You **MUST NOT** yield non-trivial work without proof: tests, e2e run, browsing and QA testing, etc. +- Don't yield non-trivial work without proof: tests, e2e, browsing, QA. {{#if secretsEnabled}}