diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 1fbfd8663..6c1414359 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -11,6 +11,7 @@ - Added `replace block N:` and `delete block N` operators to the `edit` tool: they resolve the syntactic block beginning on line N via tree-sitter (native `blockRangeAt`) and replace or delete its full line span, so a construct can be rewritten or removed without counting its closing line. Unresolvable blocks (unsupported language, blank/closing-delimiter line, or a parse error) are rejected with guidance to use an explicit `replace N..M:` / `delete N..M` range. - Added an animated pending border for `bash` and `eval` execution blocks: while a command/cell is running, a single dark segment glides clockwise around the block's outer edge (top → right → bottom → left), replacing the previous static accent border. Motion is eased per edge (decelerating into each corner) and timed against a fixed lap duration mapped onto the live perimeter, so streaming a new output line or resizing the terminal nudges the segment proportionally instead of resetting its position. Driven by the existing spinner cadence and gated on the `display.shimmer` setting (no motion when `disabled`). - Added a `PI_TINY_DTYPE` environment variable that overrides the ONNX quantization/precision used for local tiny models (session titles and Mnemosyne memory tasks), mirroring `PI_TINY_DEVICE`. Unset keeps each model's shipped dtype (currently `q4`); `PI_TINY_DTYPE=fp16` trades speed for fidelity and `PI_TINY_DTYPE=q8` is also accepted, alongside `auto`, `fp32`, `int8`, `uint8`, `bnb4`, `q4f16`, `q2`, `q2f16`, `q1`, and `q1f16`. An unrecognized value fails loudly at worker startup instead of silently loading a different precision. +- Added a bundled set of default rules shipped with the agent (TypeScript/Rust convention rules registered as TTSR conditions). They load via the new lowest-priority `builtin-defaults` discovery provider, so any user/project/tool rule of the same name overrides the bundled copy. Disable the whole set with `ttsr.builtinRules: false`, or drop individual rules (bundled or your own) by name via `ttsr.disabledRules`. ### Changed diff --git a/packages/coding-agent/src/capability/rule-buckets.ts b/packages/coding-agent/src/capability/rule-buckets.ts new file mode 100644 index 000000000..16afdc307 --- /dev/null +++ b/packages/coding-agent/src/capability/rule-buckets.ts @@ -0,0 +1,64 @@ +/** + * Rule bucketing + * + * Single funnel that every discovered rule passes through on its way into a + * session. It applies the user's disable levers, registers TTSR rules with the + * manager, and splits the rest into the always-apply and rulebook buckets. + * + * Bucket precedence (matches docs/rulebook-matching-pipeline.md §5): + * 1. TTSR — non-empty `condition` that `TtsrManager.addRule` accepts + * 2. always — `alwaysApply === true` + * 3. rulebook — has a `description` + */ +import type { TtsrManager } from "../export/ttsr"; +import { BUILTIN_DEFAULTS_PROVIDER_ID, type Rule } from "./rule"; + +export interface RuleBuckets { + rulebookRules: Rule[]; + alwaysApplyRules: Rule[]; +} + +export interface BucketRulesOptions { + /** Rule names to drop entirely (bundled defaults and user rules alike). */ + disabledRules?: readonly string[]; + /** When false, drop every rule from the bundled `builtin-defaults` provider. */ + builtinRules?: boolean; +} + +/** + * Filter and bucket rules, registering TTSR rules on `ttsrManager` as a side + * effect. Disabled rules are dropped before any bucket assignment, so a + * disabled rule is neither matched as TTSR nor surfaced via `rule://`. + */ +export function bucketRules( + rules: readonly Rule[], + ttsrManager: TtsrManager, + options: BucketRulesOptions = {}, +): RuleBuckets { + const includeBuiltin = options.builtinRules !== false; + const disabled = new Set(); + for (const raw of options.disabledRules ?? []) { + const name = raw.trim(); + if (name.length > 0) disabled.add(name); + } + + const rulebookRules: Rule[] = []; + const alwaysApplyRules: Rule[] = []; + + for (const rule of rules) { + if (disabled.has(rule.name)) continue; + if (!includeBuiltin && rule._source?.provider === BUILTIN_DEFAULTS_PROVIDER_ID) continue; + + const isTtsrRule = rule.condition && rule.condition.length > 0 ? ttsrManager.addRule(rule) : false; + if (isTtsrRule) continue; + if (rule.alwaysApply === true) { + alwaysApplyRules.push(rule); + continue; + } + if (rule.description) { + rulebookRules.push(rule); + } + } + + return { rulebookRules, alwaysApplyRules }; +} diff --git a/packages/coding-agent/src/capability/rule.ts b/packages/coding-agent/src/capability/rule.ts index 8b4623dd2..0b5d8d2b3 100644 --- a/packages/coding-agent/src/capability/rule.ts +++ b/packages/coding-agent/src/capability/rule.ts @@ -9,6 +9,14 @@ import type { SourceMeta } from "./types"; const CONDITION_GLOB_SCOPE_TOOLS = ["edit", "write"] as const; +/** + * Provider id for the bundled default rules shipped with the agent. + * Lowest priority, so any user/project/tool rule of the same name overrides + * a bundled default. Also used to gate the whole bundled set via + * `ttsr.builtinRules`. + */ +export const BUILTIN_DEFAULTS_PROVIDER_ID = "builtin-defaults"; + /** * Parsed frontmatter from rule files. */ diff --git a/packages/coding-agent/src/config/settings-schema.ts b/packages/coding-agent/src/config/settings-schema.ts index 2622b0797..fa1095fee 100644 --- a/packages/coding-agent/src/config/settings-schema.ts +++ b/packages/coding-agent/src/config/settings-schema.ts @@ -1705,6 +1705,26 @@ export const SETTINGS_SCHEMA = { }, }, + "ttsr.builtinRules": { + type: "boolean", + default: true, + ui: { + tab: "context", + label: "Builtin Rules", + description: "Load the default rules shipped with the agent (override individually with ttsr.disabledRules)", + }, + }, + + "ttsr.disabledRules": { + type: "array", + default: [] as string[], + ui: { + tab: "context", + label: "Disabled Rules", + description: "Rule names to ignore entirely (applies to bundled defaults and your own rules)", + }, + }, + // ──────────────────────────────────────────────────────────────────────── // Editing // ──────────────────────────────────────────────────────────────────────── @@ -3299,6 +3319,10 @@ export interface TtsrSettings { interruptMode: "never" | "prose-only" | "tool-only" | "always"; repeatMode: "once" | "after-gap"; repeatGap: number; + /** Bucketing-only (read by bucketRules, not the TtsrManager). */ + builtinRules?: boolean; + /** Bucketing-only (read by bucketRules, not the TtsrManager). */ + disabledRules?: string[]; } export interface ExaSettings { diff --git a/packages/coding-agent/src/discovery/builtin-defaults.ts b/packages/coding-agent/src/discovery/builtin-defaults.ts new file mode 100644 index 000000000..e4d3d6887 --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-defaults.ts @@ -0,0 +1,39 @@ +/** + * Builtin Defaults Provider + * + * Ships a curated set of default rules (mostly TTSR conventions) embedded into + * the binary. Registered at the lowest priority so any user/project/tool rule + * with the same `name` overrides the bundled copy (first-wins dedup by name). + * + * Users disable bundled rules three ways: + * - flip `ttsr.builtinRules` off (drops the whole set), + * - list a name in `ttsr.disabledRules` (drops one rule), + * - define a same-named rule in any higher-priority source (overrides it). + * The first two are enforced in `bucketRules` (see capability/rule-buckets.ts). + */ +import { registerProvider } from "../capability"; +import { BUILTIN_DEFAULTS_PROVIDER_ID, type Rule, ruleCapability } from "../capability/rule"; +import type { LoadContext, LoadResult } from "../capability/types"; +import { BUILTIN_RULE_SOURCES } from "./builtin-rules"; +import { buildRuleFromMarkdown, createSourceMeta } from "./helpers"; + +const DISPLAY_NAME = "Builtin Defaults"; +// Lowest priority: every other rule provider wins a name conflict. +const PRIORITY = 1; + +async function loadRules(_ctx: LoadContext): Promise> { + const items = BUILTIN_RULE_SOURCES.map(({ name, content }) => { + const virtualPath = `${BUILTIN_DEFAULTS_PROVIDER_ID}:${name}.md`; + const source = createSourceMeta(BUILTIN_DEFAULTS_PROVIDER_ID, virtualPath, "user"); + return buildRuleFromMarkdown(name, content, virtualPath, source, { ruleName: name }); + }); + return { items }; +} + +registerProvider(ruleCapability.id, { + id: BUILTIN_DEFAULTS_PROVIDER_ID, + displayName: DISPLAY_NAME, + description: "Default rules shipped with the agent (disable via ttsr.builtinRules / ttsr.disabledRules)", + priority: PRIORITY, + load: loadRules, +}); diff --git a/packages/coding-agent/src/discovery/builtin-rules/index.ts b/packages/coding-agent/src/discovery/builtin-rules/index.ts new file mode 100644 index 000000000..392908144 --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/index.ts @@ -0,0 +1,48 @@ +/** + * Bundled default rules shipped with the coding agent. + * + * Each markdown source is embedded via `with { type: "text" }` so it survives + * `bun build --compile` (the compiled binary ships no loose rule files; only + * the embedded text). The native source/tarball installs read the same modules. + * + * Registered by the lowest-priority `builtin-defaults` rule provider so any + * user/project/tool rule with the same name overrides the bundled copy. + */ +import rsBoxLeak from "./rs-box-leak.md" with { type: "text" }; +import rsFuturePrelude from "./rs-future-prelude.md" with { type: "text" }; +import rsLazylock from "./rs-lazylock.md" with { type: "text" }; +import rsMatchErgonomics from "./rs-match-ergonomics.md" with { type: "text" }; +import rsParkingLot from "./rs-parking-lot.md" with { type: "text" }; +import rsResultType from "./rs-result-type.md" with { type: "text" }; +import tsBareCatch from "./ts-bare-catch.md" with { type: "text" }; +import tsImportType from "./ts-import-type.md" with { type: "text" }; +import tsNoAny from "./ts-no-any.md" with { type: "text" }; +import tsNoDynamicImport from "./ts-no-dynamic-import.md" with { type: "text" }; +import tsNoReturnType from "./ts-no-return-type.md" with { type: "text" }; +import tsNoTinyFunctions from "./ts-no-tiny-functions.md" with { type: "text" }; +import tsPromiseWithResolvers from "./ts-promise-with-resolvers.md" with { type: "text" }; +import tsSetMap from "./ts-set-map.md" with { type: "text" }; + +/** A bundled rule's stable name and raw markdown (frontmatter + body). */ +export interface BuiltinRuleSource { + name: string; + content: string; +} + +/** All bundled default rules, ordered by name. */ +export const BUILTIN_RULE_SOURCES: readonly BuiltinRuleSource[] = [ + { name: "rs-box-leak", content: rsBoxLeak }, + { name: "rs-future-prelude", content: rsFuturePrelude }, + { name: "rs-lazylock", content: rsLazylock }, + { name: "rs-match-ergonomics", content: rsMatchErgonomics }, + { name: "rs-parking-lot", content: rsParkingLot }, + { name: "rs-result-type", content: rsResultType }, + { name: "ts-bare-catch", content: tsBareCatch }, + { name: "ts-import-type", content: tsImportType }, + { name: "ts-no-any", content: tsNoAny }, + { name: "ts-no-dynamic-import", content: tsNoDynamicImport }, + { name: "ts-no-return-type", content: tsNoReturnType }, + { name: "ts-no-tiny-functions", content: tsNoTinyFunctions }, + { name: "ts-promise-with-resolvers", content: tsPromiseWithResolvers }, + { name: "ts-set-map", content: tsSetMap }, +]; diff --git a/packages/coding-agent/src/discovery/builtin-rules/rs-box-leak.md b/packages/coding-agent/src/discovery/builtin-rules/rs-box-leak.md new file mode 100644 index 000000000..e6abd0e55 --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/rs-box-leak.md @@ -0,0 +1,48 @@ +--- +description: Never use Box::leak - it intentionally leaks memory +condition: "Box::leak" +scope: "tool:edit(*.rs), tool:write(*.rs)" +--- + +Never use `Box::leak` to satisfy a lifetime. It intentionally leaks the allocation for the rest of the process. + +## Why + +- The allocation is never freed. +- It hides ownership bugs. +- It turns lifetime errors into process lifetime growth. +- It makes tests pass while production memory grows. + +## Use instead + +| Need | Use | +| --- | --- | +| Shared async/thread data | `Arc` or owned values | +| Global lazy state | `LazyLock` or `OnceLock` | +| Text escaping a scope | `String` / `Arc` | +| `'static` callback | `move` closure with owned captures | +| FFI pointer | Explicit owner that frees on drop | + +## Examples + +```rust +// Bad — leaking to manufacture 'static. +fn label(id: u64) -> &'static str { + Box::leak(Box::new(format!("item_{id}"))) +} + +// Good — return owned data. +fn label(id: u64) -> String { + format!("item_{id}") +} + +// Bad — leaking before spawn. +let state = Box::leak(Box::new(state)); +tokio::spawn(async move { use_state(state) }); + +// Good — share owned state. +let state = Arc::new(state); +tokio::spawn(async move { use_state(&state) }); +``` + +If `Box::leak` looks necessary, fix ownership instead. diff --git a/packages/coding-agent/src/discovery/builtin-rules/rs-future-prelude.md b/packages/coding-agent/src/discovery/builtin-rules/rs-future-prelude.md new file mode 100644 index 000000000..4ffd17618 --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/rs-future-prelude.md @@ -0,0 +1,23 @@ +--- +description: Use Future not std::future::Future - it's in the prelude +condition: "std::future::Future" +scope: "tool:edit(*.rs), tool:write(*.rs)" +--- + +Use `Future` directly instead of `std::future::Future` in type positions. + +Rust 2024 includes `Future` in the standard prelude. Older editions can import it once with `use std::future::Future;`. Repeating the fully qualified path makes signatures harder to read without adding safety. + +## Examples + +```rust +// Bad — fully qualified in every signature. +fn fetch() -> impl std::future::Future> { ... } +fn poll(fut: Pin<&mut dyn std::future::Future>) { ... } + +// Good — use the prelude or one import. +fn fetch() -> impl Future> { ... } +fn poll(fut: Pin<&mut dyn Future>) { ... } +``` + +Pre-2024 edition? Add `use std::future::Future;` at the top. diff --git a/packages/coding-agent/src/discovery/builtin-rules/rs-lazylock.md b/packages/coding-agent/src/discovery/builtin-rules/rs-lazylock.md new file mode 100644 index 000000000..c82a9e0af --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/rs-lazylock.md @@ -0,0 +1,51 @@ +--- +description: Prefer std::sync::LazyLock over OnceLock and once_cell +condition: + - "once_cell::" + - "OnceLock::new" +scope: "tool:edit(*.rs), tool:write(*.rs)" +--- + +Prefer `std::sync::LazyLock` over `OnceLock` and the `once_cell` crate when the initializer is known at declaration time. + +`LazyLock` stores the cell and initializer together. There is no separate `init()` function, no repeated `get_or_init`, and no missing initialization path. + +## once_cell → std + +```rust +// Before +use once_cell::sync::Lazy; +static CONFIG: Lazy = Lazy::new(|| "value".to_string()); + +// After +use std::sync::LazyLock; +static CONFIG: LazyLock = LazyLock::new(|| "value".to_string()); +``` + +## OnceLock → LazyLock + +```rust +// Before — fixed initializer hidden in accessor. +use std::sync::OnceLock; +static SETTINGS: OnceLock = OnceLock::new(); +fn settings() -> &'static Settings { + SETTINGS.get_or_init(Settings::load) +} + +// After — initializer lives with the static. +use std::sync::LazyLock; +static SETTINGS: LazyLock = LazyLock::new(Settings::load); +``` + +## Keep OnceLock when runtime input is required + +```rust +use std::sync::OnceLock; +static DATABASE: OnceLock = OnceLock::new(); + +fn init_database(url: &str) { + let _ = DATABASE.set(Database::connect(url)); +} +``` + +Do not add `once_cell` for new code. Use the standard library equivalent. diff --git a/packages/coding-agent/src/discovery/builtin-rules/rs-match-ergonomics.md b/packages/coding-agent/src/discovery/builtin-rules/rs-match-ergonomics.md new file mode 100644 index 000000000..8f4d34280 --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/rs-match-ergonomics.md @@ -0,0 +1,67 @@ +--- +description: Use match ergonomics instead of ref/ref mut patterns +condition: + - "\\(ref mut " + - "\\(ref [a-z_]" +scope: "tool:edit(*.rs), tool:write(*.rs)" +--- + +Use match ergonomics instead of explicit `ref` / `ref mut` patterns. Borrow the scrutinee and let bindings receive references. + +## Shared references + +```rust +// Before +match value { + Some(ref item) => println!("{item}"), + None => {} +} + +// After +match &value { + Some(item) => println!("{item}"), + None => {} +} + +if let Some(item) = &value { + println!("{item}"); +} +``` + +## Mutable references + +```rust +// Before +match value { + Some(ref mut item) => *item += 1, + None => {} +} + +// After +match &mut value { + Some(item) => *item += 1, + None => {} +} + +if let Some(item) = &mut value { + *item += 1; +} +``` + +## Result + +```rust +// Before +match result { + Ok(ref data) => println!("{data}"), + Err(ref err) => eprintln!("{err}"), +} + +// After +match &result { + Ok(data) => println!("{data}"), + Err(err) => eprintln!("{err}"), +} +``` + +Modern Rust rarely needs `ref` in patterns. Borrow the value being matched. diff --git a/packages/coding-agent/src/discovery/builtin-rules/rs-parking-lot.md b/packages/coding-agent/src/discovery/builtin-rules/rs-parking-lot.md new file mode 100644 index 000000000..3a6d18b22 --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/rs-parking-lot.md @@ -0,0 +1,44 @@ +--- +description: Use parking_lot instead of std::sync for Mutex/RwLock +condition: + - "\\.lock\\(\\)\\.unwrap\\(\\)" + - "\\.read\\(\\)\\.unwrap\\(\\)" + - "\\.write\\(\\)\\.unwrap\\(\\)" +scope: "tool:edit(*.rs), tool:write(*.rs)" +--- + +Use `parking_lot::{Mutex, RwLock}` instead of `std::sync::{Mutex, RwLock}` when code immediately unwraps lock results. + +## Why + +- `lock()`, `read()`, and `write()` return guards directly. +- No poisoning error path to unwrap. +- Guards are smaller and faster in common contention cases. +- The call site shows locking, not error handling boilerplate. + +## Migration + +```rust +// Before +use std::sync::Mutex; +let data = Mutex::new(Vec::new()); +let guard = data.lock().unwrap(); + +// After +use parking_lot::Mutex; +let data = Mutex::new(Vec::new()); +let guard = data.lock(); +``` + +## Equivalents + +| std::sync | parking_lot | +| --- | --- | +| `Mutex` | `Mutex` | +| `RwLock` | `RwLock` | +| `Condvar` | `Condvar` | +| `Once` | `Once` | + +## Keep async locks async + +Use `tokio::sync::Mutex` / `tokio::sync::RwLock` when a guard is held across `.await` or the lock belongs to async coordination. diff --git a/packages/coding-agent/src/discovery/builtin-rules/rs-result-type.md b/packages/coding-agent/src/discovery/builtin-rules/rs-result-type.md new file mode 100644 index 000000000..6515e0736 --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/rs-result-type.md @@ -0,0 +1,19 @@ +--- +description: Result type aliases must include a defaulted error type parameter +condition: "type\\s+Result<[A-Za-z_]\\w*>\\s*=" +scope: "tool:edit(*.rs), tool:write(*.rs)" +--- + +`Result` aliases must expose the error type as a defaulted parameter. + +```rust +pub type Result = std::result::Result; +``` + +Never write: + +```rust +type Result = std::result::Result; +``` + +The default keeps common call sites short while preserving escape hatches for precise errors. diff --git a/packages/coding-agent/src/discovery/builtin-rules/ts-bare-catch.md b/packages/coding-agent/src/discovery/builtin-rules/ts-bare-catch.md new file mode 100644 index 000000000..accc6a95d --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/ts-bare-catch.md @@ -0,0 +1,38 @@ +--- +description: Use bare `catch {` when the error binding is unused +condition: "catch \\(_" +scope: "tool:edit(*.ts), tool:edit(*.tsx), tool:write(*.ts), tool:write(*.tsx)" +--- + +Use bare `catch {}` when the caught value is unused. An underscore-prefixed binding adds noise and still allocates a local name. + +## Replace + +```typescript +// Bad +try { + await loadConfig(); +} catch (_err) { + return null; +} + +// Good +try { + await loadConfig(); +} catch { + return null; +} +``` + +## Keep a real name when used + +```typescript +try { + await saveConfig(); +} catch (err) { + logger.error("save failed", { err }); + throw err; +} +``` + +Unused error? Bare `catch`. Used error? Name it for what it carries. diff --git a/packages/coding-agent/src/discovery/builtin-rules/ts-import-type.md b/packages/coding-agent/src/discovery/builtin-rules/ts-import-type.md new file mode 100644 index 000000000..5bf88d830 --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/ts-import-type.md @@ -0,0 +1,42 @@ +--- +description: "Use `import type`, not `import('pkg').Type` in type positions" +condition: "import\\(" +scope: "tool:edit(*.ts), tool:edit(*.tsx), tool:write(*.ts), tool:write(*.tsx)" +--- + +Use top-level `import type` declarations for type-only dependencies. NEVER write `import("pkg").Type` inside source annotations. + +## Why + +- Top-level imports expose dependencies immediately. +- Import sorting and deduplication can manage them. +- Signatures stay readable and reviewable. +- Re-exports do not inherit noisy inline paths. + +## Avoid + +```typescript +// Bad — inline imports hide dependencies in signatures. +function run(client: import("some-sdk").Client, input: import("zod/v4").infer): Promise; + +// Bad — annotations become path dumps. +const options: import("some-sdk/config").ClientOptions = { ... }; +``` + +## Use + +```typescript +import type { Client } from "some-sdk"; +import type { ClientOptions } from "some-sdk/config"; +import type { infer as Infer } from "zod/v4"; + +function run(client: Client, input: Infer): Promise; +const options: ClientOptions = { ... }; +``` + +## Exceptions + +- Ambient `.d.ts` globals that must not become modules. +- Generated files whose generator owns import management. + +In normal `.ts` / `.tsx` source, use `import type`. diff --git a/packages/coding-agent/src/discovery/builtin-rules/ts-no-any.md b/packages/coding-agent/src/discovery/builtin-rules/ts-no-any.md new file mode 100644 index 000000000..d2df70b96 --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/ts-no-any.md @@ -0,0 +1,56 @@ +--- +description: "Never use `any` in TypeScript annotations or assertions — use `unknown`, generics, or the actual type" +condition: ": any|as any" +scope: "tool:edit(*.ts), tool:edit(*.tsx), tool:write(*.ts), tool:write(*.tsx)" +--- + +Never use `: any` or `as any`. They disable type checking exactly where the boundary needs precision. + +## Use instead + +- `unknown` for unvalidated input. +- A domain type when the shape is known. +- A generic when the caller supplies the shape. +- A type guard when runtime checks establish shape. +- `satisfies` for object literals that must match a contract. + +## Parameters and returns + +```typescript +// Bad +function readId(value: any): any { + return value.id; +} + +// Good — validate unknown input. +function readId(value: unknown): string | undefined { + if (value && typeof value === "object" && "id" in value) { + const candidate = (value as { id: unknown }).id; + return typeof candidate === "string" ? candidate : undefined; + } +} +``` + +## Assertions + +```typescript +// Bad +const root = document.getElementById("root") as any; +root.innerText = "ready"; + +// Good +const root = document.getElementById("root") as HTMLElement | null; +root?.innerText = "ready"; +``` + +## Object literals + +```typescript +// Bad +const config = { port: 3000 } as any as ServerConfig; + +// Good +const config = { port: 3000 } satisfies ServerConfig; +``` + +If a library boundary truly requires an unchecked cast, use `as unknown as T` with a short reason. Never leave a bare `any`. diff --git a/packages/coding-agent/src/discovery/builtin-rules/ts-no-dynamic-import.md b/packages/coding-agent/src/discovery/builtin-rules/ts-no-dynamic-import.md new file mode 100644 index 000000000..831aed215 --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/ts-no-dynamic-import.md @@ -0,0 +1,39 @@ +--- +description: "Do not use `await import()` — use static imports unless dynamic loading is unavoidable" +condition: "await import\\(" +scope: "tool:edit(*.ts), tool:edit(*.tsx), tool:write(*.ts), tool:write(*.tsx)" +--- + +Use static imports for modules known at author time. Reach for `await import()` only when the module specifier is genuinely runtime-selected. + +## Why + +- Static imports fail during build, not under load. +- Bundlers, type checkers, and tree shakers see them. +- The dependency graph remains reviewable. +- Consumers keep precise module types without casts. + +## Avoid + +```typescript +// Bad — the module path is a literal. +const { createClient } = await import("some-sdk"); + +// Bad — dynamic import followed by a shape assertion. +const mod = (await import("./known-module")) as { run?: unknown }; +``` + +## Use + +```typescript +import { createClient } from "some-sdk"; +import { run } from "./known-module"; +``` + +## Exceptions + +- Plugin loading from a runtime registry. +- Platform-specific modules that do not exist everywhere. +- Test cases that intentionally exercise module loading boundaries. + +Exception? Add a short comment naming why static import cannot work. diff --git a/packages/coding-agent/src/discovery/builtin-rules/ts-no-return-type.md b/packages/coding-agent/src/discovery/builtin-rules/ts-no-return-type.md new file mode 100644 index 000000000..cbba659af --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/ts-no-return-type.md @@ -0,0 +1,45 @@ +--- +description: "Do not use `ReturnType` — name the type explicitly" +condition: "ReturnType<" +scope: "tool:edit(*.ts), tool:edit(*.tsx), tool:write(*.ts), tool:write(*.tsx)" +--- + +Do not publish contracts through `ReturnType`. Name the type at the module that owns the value and import that name at consumers. + +## Why + +- Named types document the contract directly. +- Consumers stop coupling to implementation helpers. +- JSDoc and changelog notes attach to the exported type. +- Type errors point at the intended API boundary. + +## Avoid + +```typescript +// Bad — opaque and coupled to implementation names. +type Config = Awaited>; +type Message = ReturnType["message"]; +let service: ReturnType | undefined; +``` + +## Use + +```typescript +// In the module that owns the function: +export interface LoadedConfig { + path: string; + values: Record; +} + +export function loadConfig(path: string): Promise { ... } + +// At the consumer: +import type { LoadedConfig } from "./config"; +``` + +## Exceptions + +- Timer handles: `ReturnType` / `setInterval`. +- Generic type utilities where the function is a type parameter. + +Concrete function? Export a concrete type. diff --git a/packages/coding-agent/src/discovery/builtin-rules/ts-no-tiny-functions.md b/packages/coding-agent/src/discovery/builtin-rules/ts-no-tiny-functions.md new file mode 100644 index 000000000..a359fc4dc --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/ts-no-tiny-functions.md @@ -0,0 +1,50 @@ +--- +description: "Do not extract 1-2 line functions that only wrap an expression — inline them" +condition: "\\{\\s*return [^;{}\\n]+;?\\s*\\}|\\b(?:const|let|var)\\s+[\\w$]+\\s*=\\s*(\\([^)]*\\)|[a-zA-Z_$][\\w$]*)\\s*=>\\s*[^{\\n]+$" +scope: "tool:edit(*.ts), tool:edit(*.tsx), tool:write(*.ts), tool:write(*.tsx)" +interruptMode: never +--- + +Do not extract a function whose whole body is one expression or one `return`. Inline it unless the name creates a durable contract. + +## Why + +- One-line wrappers hide no real behavior. +- Readers must jump to verify trivial code. +- The signature freezes a shape too early. +- Search and type flow work better with inline expressions. + +## Avoid + +```typescript +// Bad — pure rename, no behavior added. +function isEmpty(value: string): boolean { + return value.length === 0; +} + +const getDisplayName = (user: User) => user.profile.displayName; + +function double(value: number) { + return value * 2; +} + +if (isEmpty(name)) { ... } +``` + +## Use + +```typescript +if (name.length === 0) { ... } +const displayName = user.profile.displayName; +const doubled = value * 2; +``` + +## Allowed tiny functions + +- Three or more call sites need lockstep behavior. +- Exported name represents a stable domain concept. +- Callback identity matters. +- Type guard preserves narrowing. +- Public API, test seam, or DI boundary needs indirection. + +If none apply, inline it. diff --git a/packages/coding-agent/src/discovery/builtin-rules/ts-promise-with-resolvers.md b/packages/coding-agent/src/discovery/builtin-rules/ts-promise-with-resolvers.md new file mode 100644 index 000000000..27d14a62f --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/ts-promise-with-resolvers.md @@ -0,0 +1,65 @@ +--- +description: Use Promise.withResolvers() instead of new Promise() constructor +condition: "new Promise\\(" +scope: "tool:edit(*.ts), tool:edit(*.tsx), tool:write(*.ts), tool:write(*.tsx)" +--- + +Use `Promise.withResolvers()` instead of `new Promise((resolve, reject) => ...)`. It keeps control flow linear and exposes typed resolver functions without callback nesting. + +## Basic operation + +```typescript +// Bad +function delay(ms: number): Promise { + return new Promise(resolve => { + setTimeout(resolve, ms); + }); +} + +// Good +function delay(ms: number): Promise { + const { promise, resolve } = Promise.withResolvers(); + setTimeout(resolve, ms); + return promise; +} +``` + +## Event-based completion + +```typescript +// Bad +function waitForEvent(emitter: EventEmitter, event: string): Promise { + return new Promise((resolve, reject) => { + emitter.once(event, resolve); + emitter.once("error", reject); + }); +} + +// Good +function waitForEvent(emitter: EventEmitter, event: string): Promise { + const { promise, resolve, reject } = Promise.withResolvers(); + emitter.once(event, resolve); + emitter.once("error", reject); + return promise; +} +``` + +## Stored resolver + +```typescript +class Gate { + #promise: Promise; + #resolve: () => void; + + constructor() { + const { promise, resolve } = Promise.withResolvers(); + this.#promise = promise; + this.#resolve = resolve; + } + + open(): void { this.#resolve(); } + wait(): Promise { return this.#promise; } +} +``` + +Use the constructor only when an API specifically requires the executor form. diff --git a/packages/coding-agent/src/discovery/builtin-rules/ts-set-map.md b/packages/coding-agent/src/discovery/builtin-rules/ts-set-map.md new file mode 100644 index 000000000..7cca8e11f --- /dev/null +++ b/packages/coding-agent/src/discovery/builtin-rules/ts-set-map.md @@ -0,0 +1,28 @@ +--- +description: Prefer Record for small static literals; use Set/Map for anything dynamic +condition: "\\bnew\\s+(Set|Map)\\b" +scope: "tool:edit(**/*.{ts,tsx}), tool:write(**/*.{ts,tsx})" +interruptMode: never +--- + +Use `Record` / `Record` for small, static string-keyed lookup tables. + +Use `Set` / `Map` when keys are dynamic, non-string, inserted or deleted at runtime, or when code needs `.size`, `.clear()`, stable insertion order, or iterator APIs. + +```typescript +// Static literal → Record +const LABEL_BY_KIND: Record = { + text: "Text", + json: "JSON", + binary: "Binary", +}; + +// Dynamic membership → Set +const seen = new Set(); +for (const item of items) { + if (seen.has(item.id)) continue; + seen.add(item.id); +} +``` + +Small fixed table? `Record`. Runtime collection? `Set` / `Map`. diff --git a/packages/coding-agent/src/discovery/index.ts b/packages/coding-agent/src/discovery/index.ts index 5b9c16847..c5ed5fd00 100644 --- a/packages/coding-agent/src/discovery/index.ts +++ b/packages/coding-agent/src/discovery/index.ts @@ -22,6 +22,7 @@ import "../capability/tool"; // Import providers (each registers itself on import) import "./agents-md"; import "./builtin"; +import "./builtin-defaults"; import "./claude"; import "./claude-plugins"; import "./cline"; diff --git a/packages/coding-agent/src/export/ttsr.ts b/packages/coding-agent/src/export/ttsr.ts index bdc0f47e9..ae0710bdf 100644 --- a/packages/coding-agent/src/export/ttsr.ts +++ b/packages/coding-agent/src/export/ttsr.ts @@ -54,6 +54,8 @@ const DEFAULT_SETTINGS: Required = { interruptMode: "always", repeatMode: "once", repeatGap: 10, + builtinRules: true, + disabledRules: [], }; const DEFAULT_SCOPE: TtsrScope = { diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index 315e413bf..86ad1297e 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -38,6 +38,7 @@ import { type AsyncJob, AsyncJobManager, isBackgroundJobSupportEnabled } from ". import { createAutoresearchExtension } from "./autoresearch"; import { loadCapability } from "./capability"; import { type Rule, ruleCapability, setActiveRules } from "./capability/rule"; +import { bucketRules } from "./capability/rule-buckets"; import { ModelRegistry } from "./config/model-registry"; import { formatModelString, @@ -1045,21 +1046,10 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} options.rules !== undefined ? { items: options.rules, warnings: undefined } : await loadCapability(ruleCapability.id, { cwd }); - const rulebookRules: Rule[] = []; - const alwaysApplyRules: Rule[] = []; - for (const rule of rulesResult.items) { - const isTtsrRule = rule.condition && rule.condition.length > 0 ? ttsrManager.addRule(rule) : false; - if (isTtsrRule) { - continue; - } - if (rule.alwaysApply === true) { - alwaysApplyRules.push(rule); - continue; - } - if (rule.description) { - rulebookRules.push(rule); - } - } + const { rulebookRules, alwaysApplyRules } = bucketRules(rulesResult.items, ttsrManager, { + builtinRules: ttsrSettings.builtinRules, + disabledRules: ttsrSettings.disabledRules, + }); if (existingSession.injectedTtsrRules.length > 0) { ttsrManager.restoreInjected(existingSession.injectedTtsrRules); } diff --git a/packages/coding-agent/test/capability/rule-buckets.test.ts b/packages/coding-agent/test/capability/rule-buckets.test.ts new file mode 100644 index 000000000..96bbfa273 --- /dev/null +++ b/packages/coding-agent/test/capability/rule-buckets.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, it } from "bun:test"; +import { BUILTIN_DEFAULTS_PROVIDER_ID, type Rule } from "@oh-my-pi/pi-coding-agent/capability/rule"; +import { bucketRules } from "@oh-my-pi/pi-coding-agent/capability/rule-buckets"; +import { TtsrManager } from "@oh-my-pi/pi-coding-agent/export/ttsr"; + +function source(provider: string): Rule["_source"] { + return { provider, providerName: provider, path: "/tmp/rule.md", level: "user" }; +} + +function makeRule(partial: Partial): Rule { + return { + name: partial.name ?? "rule", + path: partial.path ?? "/tmp/rule.md", + content: partial.content ?? "body", + globs: partial.globs, + alwaysApply: partial.alwaysApply, + description: partial.description, + condition: partial.condition, + scope: partial.scope, + interruptMode: partial.interruptMode, + _source: partial._source ?? source("native"), + }; +} + +describe("bucketRules", () => { + it("registers a condition rule as TTSR and excludes it from rulebook/always buckets", () => { + const mgr = new TtsrManager(); + const ttsr = makeRule({ name: "no-foo", condition: ["FORBIDDEN"], description: "blocks foo" }); + + const { rulebookRules, alwaysApplyRules } = bucketRules([ttsr], mgr); + + expect(rulebookRules).toHaveLength(0); + expect(alwaysApplyRules).toHaveLength(0); + expect(mgr.checkDelta("contains FORBIDDEN token", { source: "text" }).map(r => r.name)).toEqual(["no-foo"]); + }); + + it("splits non-TTSR rules into always-apply and rulebook by metadata", () => { + const mgr = new TtsrManager(); + const sticky = makeRule({ name: "sticky", alwaysApply: true, description: "sticky desc" }); + const book = makeRule({ name: "book", description: "rulebook desc" }); + const orphan = makeRule({ name: "orphan" }); + + const { rulebookRules, alwaysApplyRules } = bucketRules([sticky, book, orphan], mgr); + + expect(alwaysApplyRules.map(r => r.name)).toEqual(["sticky"]); + expect(rulebookRules.map(r => r.name)).toEqual(["book"]); + expect(mgr.hasRules()).toBe(false); + }); + + it("disabledRules drops a rule from every bucket and from TTSR registration", () => { + const mgr = new TtsrManager(); + const ttsr = makeRule({ name: "no-foo", condition: ["FORBIDDEN"], description: "blocks foo" }); + const book = makeRule({ name: "book", description: "rulebook desc" }); + + const { rulebookRules } = bucketRules([ttsr, book], mgr, { disabledRules: ["no-foo", "book"] }); + + expect(rulebookRules).toHaveLength(0); + expect(mgr.hasRules()).toBe(false); + expect(mgr.checkDelta("contains FORBIDDEN token", { source: "text" })).toHaveLength(0); + }); + + it("disabledRules trims entries and ignores blanks", () => { + const mgr = new TtsrManager(); + const ttsr = makeRule({ name: "no-foo", condition: ["FORBIDDEN"] }); + + bucketRules([ttsr], mgr, { disabledRules: [" no-foo ", "", " "] }); + + expect(mgr.hasRules()).toBe(false); + }); + + it("builtinRules:false drops builtin-defaults rules but keeps the rest", () => { + const mgr = new TtsrManager(); + const builtin = makeRule({ + name: "builtin-foo", + condition: ["FORBIDDEN"], + _source: source(BUILTIN_DEFAULTS_PROVIDER_ID), + }); + const userRule = makeRule({ name: "user-foo", condition: ["BANNED"], _source: source("native") }); + + bucketRules([builtin, userRule], mgr, { builtinRules: false }); + + expect(mgr.checkDelta("contains FORBIDDEN token", { source: "text" })).toHaveLength(0); + mgr.resetBuffer(); + expect(mgr.checkDelta("contains BANNED token", { source: "text" }).map(r => r.name)).toEqual(["user-foo"]); + }); + + it("includes builtin-defaults rules when builtinRules is unset (default on)", () => { + const mgr = new TtsrManager(); + const builtin = makeRule({ + name: "builtin-foo", + condition: ["FORBIDDEN"], + _source: source(BUILTIN_DEFAULTS_PROVIDER_ID), + }); + + bucketRules([builtin], mgr); + + expect(mgr.checkDelta("contains FORBIDDEN token", { source: "text" }).map(r => r.name)).toEqual(["builtin-foo"]); + }); +}); diff --git a/packages/coding-agent/test/discovery/builtin-defaults.test.ts b/packages/coding-agent/test/discovery/builtin-defaults.test.ts new file mode 100644 index 000000000..0664c5fe0 --- /dev/null +++ b/packages/coding-agent/test/discovery/builtin-defaults.test.ts @@ -0,0 +1,80 @@ +/** + * The bundled `builtin-defaults` rule provider ships a curated default rule set + * embedded into the binary. These tests defend that the whole set loads and + * parses, and that the provider sits at the lowest priority so any user/project + * rule of the same name overrides a bundled default (first-wins dedup). + */ +import { describe, expect, it } from "bun:test"; +import { getCapability } from "@oh-my-pi/pi-coding-agent/capability"; +import { BUILTIN_DEFAULTS_PROVIDER_ID, type Rule, ruleCapability } from "@oh-my-pi/pi-coding-agent/capability/rule"; +import type { LoadContext } from "@oh-my-pi/pi-coding-agent/capability/types"; +// Register all discovery providers as a side effect. +import "@oh-my-pi/pi-coding-agent/discovery"; + +const EXPECTED_RULE_NAMES = [ + "rs-box-leak", + "rs-future-prelude", + "rs-lazylock", + "rs-match-ergonomics", + "rs-parking-lot", + "rs-result-type", + "ts-bare-catch", + "ts-import-type", + "ts-no-any", + "ts-no-dynamic-import", + "ts-no-return-type", + "ts-no-tiny-functions", + "ts-promise-with-resolvers", + "ts-set-map", +].sort(); + +function ruleProvider() { + const cap = getCapability(ruleCapability.id); + if (!cap) throw new Error("rules capability missing"); + const provider = cap.providers.find(p => p.id === BUILTIN_DEFAULTS_PROVIDER_ID); + if (!provider) throw new Error("builtin-defaults provider missing"); + return { cap, provider }; +} + +async function loadBuiltinRules(): Promise { + const { provider } = ruleProvider(); + const ctx: LoadContext = { cwd: "/tmp", home: "/tmp/home", repoRoot: null }; + const result = await (provider.load as (ctx: LoadContext) => Promise<{ items: Rule[] }>)(ctx); + return result.items; +} + +describe("builtin-defaults rule provider", () => { + it("loads exactly the bundled default rule set, all attributed to the provider", async () => { + const rules = await loadBuiltinRules(); + const names = rules.map(r => r.name).sort(); + expect(names).toEqual(EXPECTED_RULE_NAMES); + expect(rules.every(r => r._source.provider === BUILTIN_DEFAULTS_PROVIDER_ID)).toBe(true); + }); + + it("parses every bundled rule as a TTSR rule (non-empty condition and scope)", async () => { + const rules = await loadBuiltinRules(); + for (const rule of rules) { + expect(rule.condition?.length, `${rule.name} condition`).toBeGreaterThan(0); + expect(rule.scope?.length, `${rule.name} scope`).toBeGreaterThan(0); + } + }); + + it("parses YAML list-form conditions from the embedded text", async () => { + const rules = await loadBuiltinRules(); + const lazylock = rules.find(r => r.name === "rs-lazylock"); + // Frontmatter declares two condition patterns as a YAML sequence. + expect(lazylock?.condition).toHaveLength(2); + }); + + it("preserves a per-rule interruptMode override from frontmatter", async () => { + const rules = await loadBuiltinRules(); + expect(rules.find(r => r.name === "ts-set-map")?.interruptMode).toBe("never"); + }); + + it("is the lowest-priority rule provider so user/project rules override defaults", () => { + const { cap, provider } = ruleProvider(); + const others = cap.providers.filter(p => p.id !== BUILTIN_DEFAULTS_PROVIDER_ID); + expect(others.length).toBeGreaterThan(0); + expect(others.every(p => p.priority > provider.priority)).toBe(true); + }); +}); diff --git a/packages/coding-agent/test/tiny-device.test.ts b/packages/coding-agent/test/tiny-device.test.ts index 33337c9b6..6757070c7 100644 --- a/packages/coding-agent/test/tiny-device.test.ts +++ b/packages/coding-agent/test/tiny-device.test.ts @@ -2,8 +2,8 @@ import { describe, expect, it } from "bun:test"; import { normalizeTinyModelDevice, resolveTinyModelDevicePreference, - tinyModelDeviceLoadOrder, type TinyModelDevice, + tinyModelDeviceLoadOrder, } from "../src/tiny/device"; function expectedDefaultDevice(): TinyModelDevice {