diff --git a/AGENTS.md b/AGENTS.md index e81d80f86..e14e37a89 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,7 +4,7 @@ ## Before You Start -Load the `style` and `philosophy` skills. Confirm working-tree status (`git status`) and run `git log --oneline -5`. When the task touches the agent loop, directors, tools, or prompts, read the relevant doc in `/docs` before writing code. +Confirm working-tree status (`git status`) and run `git log --oneline -5`. When the task touches the agent loop, directors, tools, or prompts, read the relevant doc in `/docs` before writing code. New contributors: configure git hooks and verify the environment before the first commit. diff --git a/docs/PLUGINS.md b/docs/PLUGINS.md index dc0c5227e..5ddfe255e 100644 --- a/docs/PLUGINS.md +++ b/docs/PLUGINS.md @@ -270,11 +270,11 @@ shape. and optional `skills//SKILL.md` needs no `index.ts`; `loadDataOnlyAgentPlugin` synthesizes the same `agentPlugin` shape after frontmatter validation. - **Skill refs on agent frontmatter / body.** A skill name is either: - - **Bare** — `style`, `philosophy`, or a namespaced `plugin:style`. Resolved by + - **Bare** — `typescript`, `plan`, or a namespaced `plugin:typescript`. Resolved by searching the plugin's `skills/` dir first, then project-local fallbacks (`.agents/skills`, `.claude/skills`, `.codex/skills`). Prefer bare names for co-located skills; they are the portable, discoverable form. - - **Path-like** — `./skills/style`, `skills/style`, `../sibling-skill`, or any + - **Path-like** — `./skills/typescript`, `skills/typescript`, `../sibling-skill`, or any ref containing `/` (including a trailing `SKILL.md`). Resolved only under the plugin root (`pluginRoot`), with lexical containment plus a realpath check so a symlink under the root cannot escape. Absolute paths, bare `.` / `..`, and @@ -331,7 +331,7 @@ shape. `resolveSkillBody`), so the model does not auto-suggest background libraries. First-party recipes that are not operator slashes remain listed for `skill_search` / `use_skill` when they only set `user-invocable: false` - (`style`, `philosophy`, `typescript`). Background libs such as `git-worktrees` set both flags: not a + (`typescript`). Background libs such as `git-worktrees` set both flags: not a slash and not listed by `skill_search`/`discoverSkills`, but still loadable by explicit `use_skill`/`resolveSkillBody` name. The slash command is a direct user entry point on top. diff --git a/docs/PRODUCT.md b/docs/PRODUCT.md index f1e9a23b6..3c97a2049 100644 --- a/docs/PRODUCT.md +++ b/docs/PRODUCT.md @@ -107,7 +107,7 @@ the file path and parse details. The TUI has an extensible slash-command framework. Built-ins include `/help` (shortcut + command overlay), `/model` (models-only picker for connected accounts; **Alt+A** or `/connect` adds a provider), `/settings`, `/permissions`, `/plugins`, `/clear`, `/new`, `/compact` (fold conversation context now, optional trailing instructions to the summarizer; does not wait for the 60% occupancy governor; idle success shows the fold and does not start a new turn), `/mcp` (enable, disable, or remove servers), `/handoff [optional instructions]` (folds context through the shared operator pipeline, then immediately starts the next turn with the instructions as the inbound content — default copy when omitted; unlike `/compact`, which stops after the fold, handoff always re-infers, so the operator can pivot goals without `/clear`; a handoff issued mid-tool-batch queues behind the in-flight batch and whichever boundary fires first runs the single fold), and `/yolo` (`/yolo [on|off|toggle]`, bare `/yolo` toggles), plus a `/` command per available workflow. `/yolo` persists skip-permissions to the active settings file. That file is the user-global `~/.corbits/settings.json` by default, making the setting machine-wide; explicit `--config ` selects a different active file, and `/yolo` writes that file. An ordinary TUI launch without the same `--config` returns to the user-global source and does not modify the custom file. `--dangerously-skip-permissions` and `--yolo` are process-only aliases. Secret-guard and authz still apply. When a session starts with its active persisted setting already on, the TUI and `corbits exec` warn that permission prompts are disabled by saved settings at the active settings path and direct the operator to edit that file to re-enable them. Plugins can register additional commands. -**Default skills** exist out of the gate as first-party slash **actions**, not director names: `/implement`, `/plan`, `/refactor`, `/review`, `/pull-request`, `/issue`, `/docs`, `/interview`. Each one is a how-to playbook — the slash sends the skill body to the primary, which follows the steps. Skills do not assign identity or route the fleet; that stays on director system prompts. `/review` classifies the target first, then dispatches a selected fleet; `/pull-request` finds or opens the PR for the current branch, and `/review` takes a PR target and reviews it from a worktree; `/docs` is how to maintain PRODUCT / ARCHITECTURE / IMPLEMENTATION; `/implement` takes a plan and a ticket to a pushed branch (worktree, per-commit planner/build/reviewer loop, whole-branch review, push, hand off to `/pull-request`) — it does not steal planning from `/plan`. Substantial Builder work consumes a counsel / `/plan` plan first; tiny parent-DIY stays plan-optional. `/plan` authors an eng change plan (files, AC, non-goals, risks, ordered steps) and does not implement. `/issue` finds or creates the tracker issue: Linear MCP when available; otherwise it `ask_operator`s for the platform (GitHub etc.) and persists `Preferred issue tracker` in `.corbits/MEMORY.md` (GitHub via `gh issue create`). There is no first-party dispatch skill — Skywalker orchestrates natively. `style`, `philosophy`, and `typescript` stay `use_skill` only (`user-invocable: false`); `git-worktrees` is a background library that is not listed for `use_skill`. Draper and emil are not slashes; they remain closed directors via `spawn_agent(agent=…)`. There is no catch-all worker. Slash names are also available to the model via `skill_search` (descriptions) then `use_skill` (body). Disable the catalog in `/plugins` (`corbits-skills`) if you want them gone. +**Default skills** exist out of the gate as first-party slash **actions**, not director names: `/implement`, `/plan`, `/refactor`, `/review`, `/pull-request`, `/issue`, `/docs`, `/interview`. Each one is a how-to playbook — the slash sends the skill body to the primary, which follows the steps. Skills do not assign identity or route the fleet; that stays on director system prompts. `/review` classifies the target first, then dispatches a selected fleet; `/pull-request` finds or opens the PR for the current branch, and `/review` takes a PR target and reviews it from a worktree; `/docs` is how to maintain PRODUCT / ARCHITECTURE / IMPLEMENTATION; `/implement` takes a plan and a ticket to a pushed branch (worktree, per-commit planner/build/reviewer loop, whole-branch review, push, hand off to `/pull-request`) — it does not steal planning from `/plan`. Substantial Builder work consumes a counsel / `/plan` plan first; tiny parent-DIY stays plan-optional. `/plan` authors an eng change plan (files, AC, non-goals, risks, ordered steps) and does not implement. `/issue` finds or creates the tracker issue: Linear MCP when available; otherwise it `ask_operator`s for the platform (GitHub etc.) and persists `Preferred issue tracker` in `.corbits/MEMORY.md` (GitHub via `gh issue create`). There is no first-party dispatch skill — Skywalker orchestrates natively. `typescript` stays `use_skill` only (`user-invocable: false`); `git-worktrees` is a background library that is not listed for `use_skill`. Draper and emil are not slashes; they remain closed directors via `spawn_agent(agent=…)`. There is no catch-all worker. Slash names are also available to the model via `skill_search` (descriptions) then `use_skill` (body). Disable the catalog in `/plugins` (`corbits-skills`) if you want them gone. Providers are **models-first**: there is no standalone `/login` command. `/model` opens a **models-only list** (Recent, Favorites, then connected provider/model rows) — type-to-filter owns printable keys, so Connect is never a bare letter. **Alt+A** or `/connect` opens a dedicated add-provider selector over every first-class kind (OpenAI dual-path ChatGPT OAuth or API key, xAI, OpenCode Zen, Anthropic, Google, OpenCode Go, Z.AI Coding Plan, Ollama, Custom), each annotated with its live account count and never filtered out for “already connected.” **Alt+F** toggles favorite on the highlighted model. **Alt+D** persists the highlighted pair as the default without switching the live session. Advanced provider drill-down (edit/delete/tiers) stays on the advanced surface, not a bare printable key while the model list is filtering. OAuth providers open their existing browser login with a named account step so multiple accounts per kind coexist (`codex/work`, …). API-key providers use the same named-instance step before the key (auth-only form: instance name + key + fixed catalog base URL), so personal and team keys land as distinct catalog rows (`openai/default`, `anthropic/work`, …); reusing a name re-keys that instance after confirm. Custom remains a free-form single endpoint (full manual form). Successful connect refreshes the catalog and reopens the model list focused on the new account’s default model. OpenCode Go lists models from the live `/zen/go/v1/models` catalog (packaged seed on fetch failure), routes each by its protocol metadata (chat completions, OpenAI responses, or Anthropic messages) and can show subscription usage in the status bar when active (rolling 5h / weekly / monthly windows when the usage API responds; omitted on auth or network failure). When Go returns a quota or rate-limit error — including some HTTP 400 responses that carry limit payloads — Corbits classifies them so quota aborts cleanly and short provider rate limits remain retryable. On a free-tier or subscription quota hit, wait for the window to reset or use OpenCode Zen free models. diff --git a/docs/TELEMETRY.md b/docs/TELEMETRY.md index 1a21cf8bb..a31d120c3 100644 --- a/docs/TELEMETRY.md +++ b/docs/TELEMETRY.md @@ -81,8 +81,7 @@ director ids from `DIRECTOR_IDS` (and the legacy `worker` alias) are reported by id; project-defined or marketplace profile ids become `custom`. `skill_used` carries `skill_name`: a first-party skill name reportable by name from the closed `corbits-skills` allowlist in `src/telemetry/classify.ts` -(the slash workflows plus `git-worktrees`, `philosophy`, `style`, -`typescript`), or `custom` for anything else, including bundled background +(the slash workflows plus `git-worktrees`, `typescript`), or `custom` for anything else, including bundled background skills outside the list. Unknown, project-local, and plugin-authored skill names are never transmitted; `skill_name` is the only identifying-adjacent property the event can carry. diff --git a/plugins/corbits-skills/skills/philosophy/SKILL.md b/plugins/corbits-skills/skills/philosophy/SKILL.md deleted file mode 100644 index 7c47dfaff..000000000 --- a/plugins/corbits-skills/skills/philosophy/SKILL.md +++ /dev/null @@ -1,114 +0,0 @@ ---- -name: philosophy -description: Engineering principles. Load for architectural decisions. -user-invocable: false ---- - -# Philosophy - -Engineering philosophy and work culture principles. This skill is meant to be loaded alongside the `style` skill to provide broader context for decision-making and collaboration. - -## Guiding Principles - -**Pragmatic over idealistic.** - -Don't get fixed on details that don't matter. If you're unsure if a detail matters, ask. - -**Simple is usually harder than easy, but it pays off in the long run.** - -**Engineer Hippocratic Oath** - Do no harm to our customers and their data. - -**Benevolent Dictatorship** - All ideas are welcome, but not all will be acted upon. We've got work to do. - -**"The map is not the territory"** - Documentation is there to guide you to the code, which is the source of truth. - -## Collaboration & Communication - -Don't be afraid to ask questions. - -**Direct Messages are for secrets.** Unless it's private, keep talking to people in public. It helps the rest of the engineers learn. - -Don't be offended when people ask you why you implemented something a certain way; if it's not your strongest solution, "it was the best solution I could put together with the information I had" is a fine answer. - -**Respect and learn from your fellow engineer.** - -Be careful of how much you judge other people's engineering decisions; there's a profound moment as an engineer when you look at something, think that it's totally insane that it was implemented that way, and then realize you're the one who implemented it but you've since forgotten. - -## Code & Git Practices - -For specific guidelines on commits, comments, and external code attribution, see the `style` skill. - -Key philosophical points: - -- **Commits should read like a story** - They're there for others and future-you to understand why a change was made -- **Keep your commit summaries clear and short** - Use the body if the change warrants further explanation -- **Don't intermix refactors and feature additions** - Keep them separate for clarity -- **Comments shouldn't describe what code is doing** - They should describe why you're doing it - -See the `style` skill for detailed formatting rules and technical specifications. - -## Constraint Ownership - -Every system has layers. Constraints belong in exactly one layer — the one that has enough information to enforce them correctly. - -When a downstream function re-checks conditions that an upstream function already guarantees, you get duplication that eventually conflicts. When callers pre-process inputs to satisfy invariants the callee already enforces, you get unnecessary complexity. When three layers all enforce the same rule, two of them are unnecessary and one of them is probably wrong. - -Find the layer that owns the constraint. Fix it there. Trust it everywhere else. - -**Before fixing a bug, answer these questions:** - -1. What invariant is being violated? -2. Which layer is responsible for enforcing that invariant? -3. Does that layer already attempt to enforce it? - -If the answer to (3) is yes, fix that layer — not a downstream consumer. If your fix requires changes in more than one module, stop and explain which layer owns the constraint and why. - -If you have made two or more fix commits to the same subsystem without resolving the issue, you are symptom-chasing. Describe the constraint violation and ask where it should be fixed. - -**It's almost never a bug in the compiler — until it is.** Exhaust every possibility in your own code before blaming the toolchain. But never fully dismiss the possibility; sometimes it actually is. - -## Backwards Compatibility - -Backwards compatibility is not inherently virtuous. It depends entirely on context. - -**Public interfaces deserve backwards compatibility.** If external consumers depend on your API, CLI, wire format, or SDK, breaking them has real cost. Maintain compatibility there, deprecate gracefully, and version when you must break. - -**Internal code does not.** When backwards compatibility in internal code means keeping dead parameters, maintaining two paths through the same logic, or wrapping new code around old assumptions just to avoid updating callers — that's not compatibility, that's tech debt with a noble-sounding name. If you own all the callers, update all the callers. - -The instinct to "keep the old way working just in case" creates code that is harder to read, harder to change, and harder to trust. Every shim, adapter, and fallback you leave behind is a lie about how the system actually works. Kill the old path when the new one is proven. Don't leave both alive. - -**Ask yourself:** who breaks if I remove this? If the answer is "nobody external," remove it. - -## Testing Philosophy - -**Tests are primarily there to verify required behavior is being followed. They're your friend.** - -Refactoring without them is a disconcerting nightmare filled with uncertainty and strife. - -## Automation & Tools - -**Automate when it's appropriate:** the first time might be too soon to understand the problem, by the third time might be when you should stop doing the same thing manually. - -**Solving problems is so much easier with the right tools.** Don't be afraid of building tools. - -## Business Context - -**Without engineering, sales has nothing to sell. Without sales, engineering can't pay rent.** - -This symbiotic relationship informs our prioritization and decision-making. - -## Issue & Project Management - -**Issues and tickets represent actual work to get done; not a hope or a dream.** - -An issue should be self-contained enough that it can be handed off at any moment. - -An issue shouldn't take longer than 2-3 days to implement. - -A single feature can have many tickets; they're cheap, so use as many as makes things clear. - -**The more status updates you put in your tickets, the less you'll be bugged by people asking you for status** (see TPS reports). - -## Acknowledgment - -After reviewing this skill, state: "I have reviewed the philosophy skill." diff --git a/plugins/corbits-skills/skills/refactor/SKILL.md b/plugins/corbits-skills/skills/refactor/SKILL.md index 214f59440..da262dc18 100644 --- a/plugins/corbits-skills/skills/refactor/SKILL.md +++ b/plugins/corbits-skills/skills/refactor/SKILL.md @@ -12,10 +12,6 @@ Use this skill to analyze existing code, produce a structured design document, a Requires a git repo. Run `git rev-parse --show-toplevel`; if it fails, stop and tell the operator to `git init` first. Work in a worktree from `origin/`, never in the main checkout: `use_skill("git-worktrees")` for the commands. -## Initialization - -Before doing anything else, load the `philosophy` skill. The principles in that skill guide how you evaluate design decisions. - ## Workflow ### Step 1: Understand the Scope @@ -60,7 +56,7 @@ Document structure: After documenting the current state: 1. Present your observations and ask the user about their priorities -2. Propose specific improvements with rationale grounded in philosophy principles (pragmatic, simple over easy, etc.) +2. Propose specific improvements with rationale grounded in the principles below (pragmatic, simple over easy, etc.) 3. Let the user accept, reject, or modify proposals 4. Ask follow-up questions to refine the approach 5. Iterate until alignment is reached @@ -81,7 +77,6 @@ A single markdown file in the user's current working directory containing both t ## Guiding Principles -From the philosophy skill: - **Pragmatic over idealistic** - Don't propose changes for theoretical purity - **Simple is usually harder than easy** - Favor designs that are genuinely simple, not just quick - **Do no harm** - Consider risks to stability and correctness diff --git a/plugins/corbits-skills/skills/style/SKILL.md b/plugins/corbits-skills/skills/style/SKILL.md deleted file mode 100644 index d8f2c75fa..000000000 --- a/plugins/corbits-skills/skills/style/SKILL.md +++ /dev/null @@ -1,350 +0,0 @@ ---- -name: style -description: General coding conventions. Load when writing or reviewing code. -user-invocable: false ---- - -# Style - -General guidelines for writing clean, maintainable code. - -## Git Repository Requirement - -Agents must only operate within git repositories. Before performing any work: - -1. Verify the current working directory is inside a git repository -2. If not in a git repository, refuse to proceed - -Without a git repository, it's too hard to succeed with agents - changes can't be tracked, reviewed, or safely reverted. - -## Documentation - -### Avoiding Redundant Comments - -Code should be self-documenting. Do not add comments that describe what the code obviously does: - -``` -// Bad - obvious comments -// Base configuration type for all backends -BaseConfigArgs = { level: LogLevel } - -// Good - let code speak for itself -BaseConfigArgs = { level: LogLevel } -``` - -Decorative comment blocks (ASCII art dividers, section headers) add visual noise without providing meaningful information. - -**When comments ARE useful:** - -- Complex algorithms that aren't immediately obvious -- Non-obvious workarounds or edge cases -- TODO/FIXME/XXX markers for work that is genuinely blocked (see below) -- Business logic that requires explanation - -``` -// XXX - Temporary workaround until upstream fix -// TODO - Switch to newMethod when minimum version is bumped -result = await legacyMethod() -``` - -**TODO/FIXME/XXX markers are not a deferral mechanism.** They are reserved for work that is _genuinely blocked_ by something outside your control — waiting on an upstream library fix, an unreleased API version, missing access or credentials, a dependency in another team's queue. The marker must name the blocker, so a reader knows what would unblock it. - -Do not use these markers for: - -- Work you could do now but would prefer not to ("TODO: clean up this function") -- Work you ran out of patience for ("FIXME: this should probably handle the error case") -- Work you're hoping someone else will pick up ("TODO: add tests") -- Decisions you didn't want to make ("TODO: figure out the right default") - -If you could do it now, do it now. A TODO is a promise to the reader that the work cannot be done yet; abusing the marker for work you simply chose not to do is dishonest and accumulates as dead weight in the codebase. - -### Comments describe the current code - -Code comments speak for the commit they appear in. Do not write comments that refer to other commits — neither what an earlier commit changed nor what a planned follow-up commit will do. A comment like `// stub; next commit fills this in` is wrong the moment that follow-up is reordered, dropped, or read by someone who reverted past it. If the code is intentionally a stub now, say _why it is a stub now_, not what is supposed to replace it. - -This holds even when you have a multi-commit plan in context — a planned commit does not exist until it lands, and the comment must be accurate for the commit it lives in, standing alone. - -## Git Workflow - -### Commit Messages - -Commits should read like a story, allowing others and future-you to understand why changes were made. - -**Commit Organization:** - -- Separate refactoring from feature additions (distinct commits) -- Separate formatting/whitespace fixes from logical changes -- Each commit should represent one logical unit of work -- **Amend** (`git commit --amend`) to refine the most recent commit (e.g., critique fixes, wording changes, missed files) -- **Edit-in-place** to fix an earlier unpushed commit when a later review reveals a problem that belongs on that commit, not HEAD. Mark the target `edit` in the rebase todo, make the fix at the stop, amend, and continue. The fix is authored against the target commit's historical tree, which keeps the change intent-correct against the right baseline — downstream commits may still produce a replay conflict if they touch the same lines, but resolving that conflict is straightforward because both sides of it are coherent diffs. - -``` -git rebase -i origin/main # substitute your project's base branch -# In the editor, change "pick abc1234 ..." to "edit abc1234 ..." -# git stops with the target commit checked out -# ... make the fix ... -git add -git commit --amend --no-edit -git rebase --continue -``` - -For more elaborate history surgery — scripted plans, multiple targets, splits, per-commit validation — search your available skills for one whose description covers git rebase or branch-history cleanup, and load it when the simple form above is not enough. - -**Message Format:** - -- **Summary line**: Max 72 characters, non-empty -- **Blank line**: Required between summary and body (if body exists) -- **Body lines**: Max 72 characters each - -**Before drafting a subject, sample the project's existing log:** - -```bash -git log origin/main --format='%s' | head -20 -``` - -The existing commits document the project's actual subject convention — verb tense, level of detail, voice, capitalization. Match what is there. - -**Conventional Commits.** Summary lines follow -[Conventional Commits 1.0.0](https://www.conventionalcommits.org/en/v1.0.0/): - -```text -(): -``` - -- **Type** — one of `feat`, `fix`, `perf`, `refactor`, `test`, `docs`, `build`, - `ci`, `chore`, `style` -- **Scope** — the component the change lives in. Omit only when the change - genuinely spans the whole project -- **Description** — plain English, imperative, starts with a verb, describes - the change directly -- **Breaking changes** — `!` after the type/scope, or a `BREAKING CHANGE:` - footer - -Everything after the colon still obeys the rules below: no abbreviations, no -trailing punctuation, no filenames, self-contained. - -**Still banned as subject prefixes**, before or instead of the type: - -- Ticket IDs: `INTR-79:`, `JIRA-1234:`, `#456:` -- Status or severity tags: `WIP:`, `[urgent]`, `(security):` -- Bare component prefixes with no type: `Anthropic adapter:`, `mm:`, `[X86]`, - `drivers/net:`, `frontend:` — the component belongs in the scope, so - `mm: fix leak` becomes `fix(mm): fix leak` - -Summary lines also use no abbreviations and do not end with punctuation. - -**Good examples:** - -``` -feat(executor): add retry logic for failed network requests -fix(inference): resolve race condition in transaction verification -docs(api): document response format -perf(glob): stop rescanning ignored directories -``` - -**Bad examples:** - -``` -Add retry logic (no type or scope) -feat: add retry logic (no scope, and says nothing specific) -Anthropic adapter: handle 429s (bare component, no type) -INTR-79: add retry logic (ticket-ID prefix) -[WIP] refactor the parser (status tag) -chore: update code (too vague) -fix: bug in server.ts (filename in subject) -``` - -**Self-contained:** - -A commit message must stand alone. Do not reference: - -- File paths or filenames — the diff already lists what changed -- External tracking systems (Linear, Jira, GitHub issues) — they may move, be renamed, or be inaccessible to future readers; the commit must explain _itself_, not point to an explanation elsewhere -- PR review comments, prior conversations, or other ephemeral discussions -- The commit's position in a branch or series, in either direction — neither prior commits ("as discussed in the previous commit") nor upcoming ones ("the next commit wires this up"). A commit describes the state of the repo at that commit, not the branch's trajectory. This holds even when you know exactly which commits are planned to land next: a follow-up commit you intend to write does not yet exist, and a reader landing on this commit (or reverting past the planned one) will not see it. - -Someone reading `git log` years from now, with only the repo in hand, should understand the change without leaving the message. - -**Body content — what belongs in a commit message:** - -**Write for a stranger reading `git log` years from now, not for the person reviewing this PR.** The reviewer has the conversation, the ticket, the prior state of the code; the future reader has only the message and the diff. Most length problems dissolve once the audience is right: anything you would write _because the reviewer would appreciate seeing your reasoning_ almost certainly does not belong. - -**Most commits do not need a body.** A clear subject and a coherent diff are usually enough. Add a body only when the diff would leave a future reader genuinely unable to answer _why_ this change. If you are reaching for a body to demonstrate the change was considered, or to preempt questions from the reviewer, that is not the body's job. - -When a body is warranted, it carries one thing: the motivation that would otherwise leave the diff looking arbitrary — why this change, why now, why not the obvious alternative. Information about the _code's behavior_, even non-obvious behavior, does not belong here: future callers do not read `git log`, they read the code, so a comment on the affected function or a line in the relevant documentation file is the right home. Surrounding context — the alternatives explored, the work that led here, the broader trade-off landscape — does not belong either, even when it feels load-bearing in the moment. Before writing a line of body, ask where that information actually lives: - -- **Describes what the code does** → the code already says this. Cut. -- **Describes how the system works in general** → belongs in repo documentation. If the docs are wrong, fix them in this commit; don't smuggle the explanation into the message. -- **Describes why a specific line exists, or how a specific block behaves** → if it meets the bar in "Avoiding Redundant Comments," it goes in a code comment at that location, or in the documentation file describing the behavior. Future callers read the code, not the commit log. If it doesn't meet that bar, it goes nowhere. -- **Walks through the diff file-by-file** → cut. The diff is right there. -- **Recaps the conversation, review, retrospective, or planning that led to the change** → cut. This is the single most common source of bloat. That the work was hard, that three alternatives were considered, that the change came out of an incident review, is not load-bearing for the future reader. - -What remains is the body. It should be short — typically one short paragraph, rarely more than two. If your draft is materially longer, you are almost certainly violating one of the bullets above (most often the conversation-recap one). The fix is to cut, not to justify. - -**Good body:** - -``` -Switch retries to exponential backoff with full jitter. - -Fixed-interval retries were producing synchronized thundering -herds against the upstream rate limiter during partial outages, -making recovery slower than no retries at all. Full jitter is the -AWS-recommended variant and the only one that decorrelates retries -across clients without losing the backoff guarantee. -``` - -**Bad body (same change):** - -``` -Switch retries to exponential backoff with full jitter. - -The original retry implementation used a fixed interval. After -last quarter's rate-limiter incident we spent a few sessions -working through the right replacement. We discussed whether to -gate the change behind a feature flag and decided against it -since the new behavior is strictly better. Full jitter -decorrelates retries across clients without losing the backoff -guarantee. Unit tests have been updated. See the PR discussion -for the full reasoning. -``` - -The bad version is not paraphrasing the diff — it is recapping the work session: the history of the prior code, the incident-and-session framing, the feature-flag discussion, the existence of tests, the pointer to the PR. None of it is load-bearing for a future reader; it is the agent demonstrating to the immediate reviewer that the change was carefully considered. Strip it and the substantive sentence — "full jitter decorrelates retries across clients without losing the backoff guarantee" — is what survives. That is what the good version already says. - -## Naming - -### Acronyms - -Acronyms are not words. Do not reshape them to fit camelCase or PascalCase word boundaries. Preserve the acronym's natural capitalization regardless of position in the name. - -``` -// Good -JSONSchema, HTTPClient, parseJSON, requestURL - -// Bad - treating acronyms as regular words -JsonSchema, HttpClient, parseJson, requestUrl -``` - -## Documentation Maintenance - -When making changes to code, check whether related documentation needs updating: - -- README files that reference changed functionality -- API documentation for modified interfaces -- Inline comments that describe changed behavior -- Configuration examples that no longer apply - -Update documentation in the same commit as the code change, not as a separate task. - -## Scope Discipline - -Only touch code that is directly related to the task at hand. Do not make drive-by changes to surrounding code, even if they look like improvements. Common violations: - -- Reformatting lines you didn't otherwise need to change -- Adding or removing comments on unrelated code -- Renaming variables or functions outside the scope of your task -- Adjusting whitespace, import order, or style in files you're passing through -- "While I'm here" refactors that aren't part of the assignment - -These changes pollute diffs, make review harder, and risk introducing unintended breakage. - -### Scope is not "the narrowest possible reading of the task" - -Scope discipline exists to prevent unrelated drive-bys, not to license deferral of work that is genuinely part of the task you accepted. If you read the task narrowly enough, almost anything can be called "out of scope" — that is a failure mode, not a virtue. - -A change is **in scope** if it is: - -- Part of what the task or issue explicitly asked for -- Required to make the requested change correct, safe, or coherent -- Necessary follow-through to the change you just made (updating callers of a renamed function, adjusting tests that now fail, updating docs that now lie) - -A change is **out of scope** only if it has no causal relationship to the work you are doing — a tangential improvement you noticed while passing through. - -### Deferral has a cost. Do the work now when you can. - -When something is in scope but inconvenient — a refactor your change makes obvious, a test you should add, a docstring that's now wrong, a helper that should be extracted — the default is to **do it now, in a properly-scoped commit on this branch**. Not a TODO. Not a follow-up ticket. Not a "we should clean this up someday." Those mechanisms exist for genuinely blocked work; using them as a release valve for work you'd rather not do creates debt the team has to carry. - -If you genuinely cannot do it on this branch, "raise it as a separate piece of work" means one of: - -1. A separate commit on the same branch, with a clear message explaining why it stands alone. -2. A follow-up PR that you commit to opening in this same working session, not "later". -3. A tracked issue with concrete acceptance criteria and a named owner — not a vague reminder. - -If none of those are happening, you are not deferring the work, you are dropping it. Don't pretend otherwise. - -## Code Reuse and Refactoring - -Do not reimplement functionality that already exists in the codebase. Before writing new code: - -1. Search for existing implementations that could serve the same purpose -2. If similar functionality exists, prefer refactoring it to meet the new requirements -3. Look for unexported functions in other packages that could be promoted to a shared location - -When a refactor might be necessary, prompt the user with specific options: - -- Refactor the existing implementation -- Promote an unexported function to a shared package -- Create a new implementation - -Allow the user to provide their own answer if none of the options fit. - -## Removing Dead Code - -When refactoring replaces an old implementation, delete the old one. Do not leave backwards-compatibility shims, re-exports, renamed `_unused` variables, or `// removed` comments for code that no longer serves a purpose. If all callers are internal and have been updated, the old path should not survive. See the `philosophy` skill for the reasoning behind this. - -## External Code Attribution - -Any code from outside the organization requires careful attribution and licensing compliance: - -1. **License verification**: Check that the license is compatible with your project -2. **Isolated commit**: Place external code in its own commit without any modifications -3. **Complete attribution**: Include in the commit message: - - Original source URL or reference - - Author/copyright information - - License type - - Date retrieved - - Any other details required for audit compliance - -If modifications to external code are needed, make them in a separate follow-up commit with clear explanation of what changed and why. - -## Data Validation - -Never trust data from outside the program. All external input — user submissions, API responses, file contents, environment variables, query parameters, message payloads — must be validated at the boundary where it enters the system. Parse it, check it, and reject it if it's wrong. Once data has crossed the boundary and been validated, internal code can trust it without re-checking. - -This means validation logic lives at the edge: HTTP handlers, CLI argument parsers, message consumers, file readers, and configuration loaders. It does not live deep inside business logic, scattered across internal functions, or deferred until the data happens to cause a failure somewhere downstream. - -If invalid data can travel through multiple layers before something finally breaks, the validation boundary is in the wrong place. - -## Defaults - -Defaults live at the edge, alongside validation. The boundary that accepts user input — CLI argument parser, config loader, HTTP handler, public API entry point — is the one layer that knows what was supplied and what was omitted. That layer resolves omissions into concrete values and hands a fully-populated argument inward. Internal code receives required parameters and acts on them; it does not invent values the caller did not supply. This is "Constraint Ownership" from the `philosophy` skill applied to a specific question: who decides what an absent value means. - -The rule targets **read-site defaults** — code that asks "did I get a value?" and silently substitutes one when the answer is no. Concretely: no `getattr(obj, "key", default)`, no `dict.get(k, default)`, no `value || fallback` or `value ?? fallback` scattered through business logic. Each of these is a defaulting decision smuggled into a layer that does not own the input contract, and each colludes with swallowed errors — a missing value that should have raised at the boundary instead becomes a silent fallback three layers deep, indistinguishable from a value the user actually passed. - -Default parameter values on a function signature are a different shape and are fine _when the function is itself a boundary_: a config loader, a dataclass constructor that receives values crossing from edge to interior, the entry point of a recursion (its own first call is the edge for the accumulator). What is not fine is an internal helper deep in the call graph that papers over a caller forgetting to pass something. Optional configuration fields get resolved once, at load time, into a concrete config object with no optionals; inner code sees a fully-specified value and trusts it. - -To locate the edge in a multi-layer system, ask which single function or file decides what an absent value means. That layer is the edge. Anything deeper that re-decides is wrong. The exception is genuinely public library code where no single layer owns the contract — every caller is the edge. "Public" here means consumed across organization or API boundaries, not "shared across two internal modules"; the latter still has an edge, and the rule still applies one layer in. - -## Build Verification - -Always run the full build command before declaring any task complete. - -- Individual package builds do not guarantee the full tree will build -- Do not work around a failing build by running individual targets and treating their success as equivalent -- If the build fails, report the failure to the user and identify the cause -- If the failure is pre-existing and unrelated to your changes, say so explicitly and let the user decide how to proceed - -Never silently skip a failing step or substitute a partial build. - -## Configuration Files - -Do not modify configuration files (e.g. eslint, prettier, tsconfig) unless explicitly asked. Focus on writing working software, not changing the conventions that are being used. - -Keep consistent even if we disagree; if we decide to change a style, make it an explicit decision and discussion, not a side effect of other work. - -## Personality - -Do not use emojis in code or documentation. Act professionally. - -## Acknowledgment - -At the start of a session, after reviewing this skill, state: "I have reviewed the style skill, and I am ready to proceed in good taste." diff --git a/plugins/corbits-skills/skills/typescript/SKILL.md b/plugins/corbits-skills/skills/typescript/SKILL.md index 83f3294ca..169e68ab8 100644 --- a/plugins/corbits-skills/skills/typescript/SKILL.md +++ b/plugins/corbits-skills/skills/typescript/SKILL.md @@ -1,548 +1,36 @@ --- name: typescript -description: TypeScript conventions and type patterns. Load when writing TypeScript. +description: How we write TypeScript and verify it. Load when writing TypeScript. user-invocable: false --- # TypeScript -TypeScript-specific guidelines for type safety and code organization. +Five rules. Everything else follows the repo's existing code and `AGENTS.md`. -## Quick Reference +## 1. Types are real -### Do +Use `unknown` and narrow, never `any`. No `as Type` and no `x!`: both hide an interface problem and give no runtime safety. Let the compiler infer what it can, and use `import type` for type-only imports. -- Use `import type` for type-only imports -- Use `{ cause }` when re-throwing errors -- Let TypeScript infer types when obvious -- Create factory functions with `create*` prefix -- Prefer factory functions over classes -- Return `null` from handlers when request doesn't match -- Use a logger instead of `console.log` -- Validate external data at runtime (fetch, filesystem, env vars, user input) with an existing validation library +## 2. Validate at the boundary -### Don't - -- Use default exports -- Use `any` type (use `unknown` and narrow) -- Use type assertions (`as Type`) - they indicate interface problems -- Use non-null assertions (`x!`) - they hide nullability bugs -- Assume type assertions provide runtime safety - they don't -- Over-type code with explicit annotations the compiler can infer -- Include file extensions in imports (unless required by runtime) - -## Naming Conventions - -### Files - -| Type | Convention | Example | -| ------------------- | --------------------------------- | ------------------------------- | -| Regular modules | Lowercase, hyphens for multi-word | `token-payment.ts`, `server.ts` | -| Single-word modules | Lowercase | `cache.ts`, `common.ts` | -| Test files | `{name}.test.ts` | `cache.test.ts` | - -### Types and Interfaces - -| Pattern | Use Case | Example | -| ----------------- | ------------------------ | --------------------------------- | -| `PascalCase` | Interfaces, type aliases | `PaymentHandler`, `RequestConfig` | -| `*Args` / `*Opts` | Function arguments | `CreateHandlerOpts` | -| `*Response` | API responses | `SettleResponse` | -| `*Info` | Data structures | `ChainInfo`, `TokenInfo` | -| `*Handler` | Handler interfaces | `PaymentHandler` | - -### Functions - -| Pattern | Use Case | Example | -| ----------- | ------------------------------ | ----------------------------------- | -| `camelCase` | All functions | `handleRequest` | -| `create*` | Factory functions | `createHandler`, `createClient` | -| `is*` | Boolean predicates | `isValidationError`, `isKnownType` | -| `get*` | Retrieval without side effects | `getBalance`, `getConfig` | -| `lookup*` | Search/lookup operations | `lookupToken`, `lookupNetwork` | -| `generate*` | Builder/generator functions | `generateMatcher`, `generateConfig` | -| `handle*` | Event/request handlers | `handleSettle`, `handleVerify` | - -### Variables - -| Pattern | Use Case | Example | -| ---------------------- | --------------------------- | -------------------------------- | -| `camelCase` | Regular variables | `paymentResponse`, `blockNumber` | -| `SCREAMING_SNAKE_CASE` | Constants, environment vars | `API_BASE_URL`, `MAX_RETRIES` | -| `_` prefix | Unused parameters | `_ctx`, `_unused` | - -### Acronyms in Names - -Acronyms are not words. Do not conform them to camelCase or PascalCase word boundaries. Preserve the acronym's natural capitalization: - -``` -// Good - types preserve acronyms -type JSONSchema = { ... } -type HTTPResponse = { ... } -type APIClient = { ... } -type XMLParser = { ... } - -// Bad - don't camelCase acronyms in types -type JsonSchema = { ... } // Should be JSONSchema -type HttpResponse = { ... } // Should be HTTPResponse -type ApiClient = { ... } // Should be APIClient - -// Good - functions and variables preserve acronyms too -getURLFromRequest -requestURL -parseHTTPHeaders -parseJSON - -// Bad -getUrlFromRequest // Should be getURLFromRequest -requestUrl // Should be requestURL -parseJson // Should be parseJSON -``` - -Common acronyms: URL, HTTP, HTTPS, JSON, API, RPC, HTML, XML - -Note: "ID" is an abbreviation, not an acronym, so use standard camelCase: `userId`, `requestId`, `getId()`. - -## Type System Patterns - -### Runtime Validation - -Use a validation library (e.g., arktype, zod, typebox) for runtime type validation. Define the validator and TypeScript type together: - -```typescript -import { type } from "arktype"; - -// Define runtime validator -export const PaymentRequest = type({ - scheme: "string", - network: "string", - amount: "string.numeric", - resource: "string.url", -}); - -// Derive TypeScript type from validator -export type PaymentRequest = typeof PaymentRequest.infer; -``` - -If no existing validation library is installed, install arktype and use it. - -This pattern should be used for all external data: API responses from `fetch`, file system reads, environment variables, user input, and third-party API responses. - -### Type Guards - -Create type guards using validation functions: - -```typescript -export function isAddress(maybe: unknown): maybe is Address { - return !isValidationError(Address(maybe)); -} - -export function isKnownNetwork(n: string): n is KnownNetwork { - return knownNetworks.includes(n as KnownNetwork); -} -``` - -### Interfaces vs Types - -- **`type`**: Use for data structures, unions, and validator-derived types -- **`interface`**: Use for behavioral contracts (objects with methods) - -```typescript -// Type for data structure -export type RequestContext = { - request: RequestInfo | URL; -}; - -// Interface for behavioral contract -export interface PaymentHandler { - getSupported?: () => Promise[]; - handleSettle: (requirements, payment) => Promise; -} -``` - -### Const Assertions for Exhaustive Types - -Use `as const` for exhaustive literal types: - -```typescript -const PaymentMode = { - Direct: "direct", - Deferred: "deferred", -} as const; - -type PaymentMode = (typeof PaymentMode)[keyof typeof PaymentMode]; - -// TypeScript ensures all cases handled in switch -switch (mode) { - case PaymentMode.Direct: - // ... - break; - case PaymentMode.Deferred: - // ... - break; -} -``` - -### Type-Only Imports - -Use `import type` for type-only imports: - -```typescript -import type { PaymentRequest } from "./types"; -import type { Hex, Account } from "viem"; - -// Mixed imports -import { - type Transaction, - createTransaction, // value import -} from "./transactions"; -``` - -### Avoid Over-Typing - -Let TypeScript infer types when obvious: - -```typescript -// Good - return type is obvious -const createHandler = async (network: string) => { - const config = { network, enabled: true }; - return { - getConfig: () => config, - isEnabled: () => config.enabled, - }; -}; - -// Unnecessary - the return type is obvious -const createHandler = async (network: string): Promise<{ - getConfig: () => { network: string; enabled: boolean }; - isEnabled: () => boolean; -}> => { ... }; -``` - -**When to add explicit types:** - -- Public API boundaries where the type serves as documentation -- When the inferred type would be too wide -- When TypeScript cannot infer the type correctly -- Complex return types that benefit from explicit documentation - -**When NOT to add explicit types:** - -- Variable assignments with obvious literal values -- Return types that match a simple expression -- Loop variables and intermediate calculations -- Arrow function parameters in callbacks where context provides types - -### Avoiding `any` and Type Assertions - -Type assertions (`as Type`) only affect compile-time types. They provide **zero runtime safety**. A type assertion tells TypeScript "trust me, this is the shape" but does nothing at runtime. - -This is especially critical for external data. Data from `fetch`, the filesystem, environment variables, user input, and third-party APIs **always needs runtime validation** because: - -1. The TypeScript type is just a guess about the actual data shape -2. The network/file/env can return anything, not what you expected -3. External data can be malformed, malicious, or changed without warning - -Use `unknown` instead of `any` when the type is truly unknown, then narrow with validation: - -```typescript -// Bad -function processData(data: any) { - return data.value; -} - -// Good -function processData(data: unknown) { - const validated = MyDataType(data); - if (isValidationError(validated)) { - throw new Error(`Invalid data: ${validated.summary}`); - } - return validated.value; -} -``` - -Type assertions bypass type checking and often indicate interface problems. Prefer runtime validation: - -```typescript -// Bad -const data = (await response.json()) as UserData; - -// Good -const raw = await response.json(); -const data = UserData(raw); -if (isValidationError(data)) { - throw new Error(`Invalid response: ${data.summary}`); -} -``` - -### Avoiding Non-Null Assertions - -The non-null assertion operator (`x!`) has the same problem as `as Type`: it's a compile-time lie. It tells TypeScript "trust me, this isn't null or undefined" when the compiler thinks it could be. If the compiler thinks a value might be null, there's usually a reason. - -Instead of silencing the compiler, restructure the code so the value is provably non-null: - -```typescript -// Bad - hiding a potential bug -const user = users.find((u) => u.id === id)!; -processUser(user); - -// Good - handle the null case -const user = users.find((u) => u.id === id); -if (!user) { - throw new Error(`User not found: ${id}`); -} -processUser(user); -``` - -```typescript -// Bad - asserting map result exists -const handler = handlers.get(name)!; - -// Good - check and provide a meaningful error -const handler = handlers.get(name); -if (!handler) { - throw new Error(`No handler registered for: ${name}`); -} -``` - -If you find yourself reaching for `!`, it means one of: - -- The code doesn't properly guarantee the value exists (fix the code) -- The type is too wide for the context (narrow it with a guard or restructure) -- An upstream function returns `T | null` when it shouldn't (fix the upstream function) - -### Generic Constraints vs Index Signatures - -Prefer generic type parameters with constraints over index signatures: - -```typescript -// Bad - index signature (too permissive) -export interface LoggingBackend { - configureApp(args: { - level: LogLevel; - [key: string]: unknown; - }): Promise; -} - -// Good - generic with constraint (type-safe) -export type BaseConfigArgs = { level: LogLevel }; - -export interface LoggingBackend< - TConfig extends BaseConfigArgs = BaseConfigArgs, -> { - configureApp(args: TConfig): Promise; -} -``` - -## Import/Export Patterns - -### Barrel Exports - -Use `index.ts` files to re-export from modules: - -```typescript -// packages/types/src/index.ts - -// Namespaced exports for grouped functionality -export * as payments from "./payments"; -export * as client from "./client"; - -// Flat exports for utilities -export * from "./validation"; -export * from "./helpers"; -``` - -### Named Exports (Preferred) - -```typescript -// Good -export function createMiddleware(args: CreateMiddlewareArgs) { ... } -export const MAX_RETRIES = 3; - -// Avoid -export default function createMiddleware(args: CreateMiddlewareArgs) { ... } -``` - -### Import Ordering - -Order imports by category: - -1. External library imports -2. Internal package imports -3. Relative imports - -```typescript -// External libraries -import { type } from "arktype"; -import { Hono } from "hono"; - -// Internal packages -import { isValidationError } from "@myorg/types"; -import type { Handler } from "@myorg/types/handler"; - -// Relative imports -import { isValidTransaction } from "./verify"; -import { logger } from "./logger"; -``` - -### Import Paths - -Omit file extensions in import paths when the module resolver can infer them: - -```typescript -// Good - no extension needed -import { createHandler } from "./handler"; -import type { Config } from "../types"; - -// Bad - unnecessary extension -import { createHandler } from "./handler.ts"; -import type { Config } from "../types.ts"; -``` - -Note: Some environments (like Deno or Node.js with `"type": "module"`) require explicit extensions. Follow project conventions when extensions are mandated by the runtime. - -### Dynamic Imports - -Dynamic `import()` expressions should be used sparingly. They exist for genuinely dynamic scenarios where the module to load is not known at authoring time (e.g., plugin systems where the module path is constructed from a variable) or where a module must be conditionally loaded at runtime (e.g., optional dependencies that may not be installed). - -If you know which module you need, use a static `import` at the top of the file. Do not use `await import()` inline next to your code change because it is convenient — that is a static dependency with worse type safety and unnecessary indirection. Add the import statement to the top of the file where it belongs. - -```typescript -// Bad - lazy inline import of a known module -const { createHandler } = await import("./handler"); - -// Good - static import at the top of the file -import { createHandler } from "./handler"; - -// Good - genuinely dynamic: the module path is not known at authoring time -const plugin = await import(`./plugins/${pluginName}`); - -// Good - conditional loading of an optional dependency -let sharp: typeof import("sharp") | undefined; -try { - sharp = await import("sharp"); -} catch (err) { - logger.warn("sharp not installed, falling back to basic image handling", { - cause: err, - }); -} -``` - -## Async Patterns - -### Factory Functions - -Use async factory functions that return objects with async methods: - -```typescript -const createHandler = async ( - network: string, - rpc: RpcClient, - config?: HandlerOptions, -) => { - // Async initialization - const networkInfo = await fetchNetworkInfo(rpc); - - // Return object with async methods - return { - getSupported, - handleVerify, - handleSettle, - }; -}; -``` - -### Parallel Execution - -Use `Promise.all` for independent parallel operations: - -```typescript -const [tokenName, tokenVersion] = await Promise.all([ - client.readContract({ functionName: "name" }), - client.readContract({ functionName: "version" }), -]); -``` - -### Timeouts - -Use `Promise.race` for operations that need timeouts: - -```typescript -function timeout(timeoutMs: number, msg?: string) { - return new Promise((_, reject) => - setTimeout(() => reject(new Error(msg ?? "timed out")), timeoutMs), - ); -} - -const result = await Promise.race([ - fetchData(), - timeout(5000, "fetch timed out"), -]); -``` - -### Retry Logic - -Implement retries with exponential backoff: - -```typescript -let attempt = (options.retryCount ?? 2) + 1; -let backoff = options.initialRetryDelay ?? 100; -let response; - -do { - response = await makeRequest(); - - if (response.ok) { - return response; - } - - await new Promise((resolve) => setTimeout(resolve, backoff)); - backoff *= 2; -} while (--attempt > 0); -``` - -## Error Handling - -### Validation Errors - -Check validation errors before proceeding: +Validate external data (fetch, filesystem, env, user input) once, where it enters, with the repo's validation library. Check the result before using it, and trust the typed value everywhere inside: ```typescript const payload = parsePayload(input); - if (isValidationError(payload)) { logger.debug(`couldn't validate payload: ${payload.summary}`); return sendBadRequest(); } - -// payload is now typed correctly ``` -### Local Error Response Factories - -Create local helpers for consistent error responses: - -```typescript -const handleSettle = async (requirements, payment) => { - const errorResponse = (msg: string): SettleResponse => { - logger.error(msg); - return { - success: false, - error: msg, - txHash: null, - }; - }; +## 3. Functions, named exports, real names - if (someConditionFails) { - return errorResponse("Invalid transaction"); - } - // ... -}; -``` +Factory functions (`create*`) over classes. Named exports only, no default exports, no file extensions in imports unless the runtime requires them. Modules are lowercase with hyphens, tests are `{name}.test.ts`. Acronyms keep their case (`URL`, `JSONSchema`, `parseHTTPHeaders`); `ID` is an abbreviation (`userId`). -### Error Chaining +## 4. Errors keep their cause -Use `{ cause }` when re-throwing errors: +Re-throw with `{ cause }`. A handler that does not own a request returns `null` so another can try. Use the logger, not `console.log`. ```typescript try { @@ -552,85 +40,18 @@ try { } ``` -### Return `null` for "Not My Responsibility" +## 5. Tests prove behavior -Handlers should return `null` when a request doesn't match their criteria: - -```typescript -const handleVerify = async (requirements, payment) => { - if (!isMatchingRequirement(requirements)) { - return null; // Let another handler try - } - // Handle the request... -}; -``` - -## Testing - -### Philosophy - -Focus test coverage on logic specific to your codebase: - -- Business logic and domain-specific validation -- Integration points between components -- Error handling paths and edge cases -- Custom algorithms and data transformations - -Do not write tests that merely verify functionality provided by external libraries. Trust well-maintained libraries to do their job. - -### Test Structure +Use `bun:test`. Test the logic that is yours (domain rules, error paths, edge cases), not the libraries you call. Inject time instead of sleeping, and land the test in the same commit as the code: ```typescript import { expect, test } from "bun:test"; -test("descriptiveTestName", () => { - // Setup - const cache = new Cache({ capacity: 3 }); - - // Assertions - expect(cache.size).toBe(0); +test("entry expires after maxAge", () => { + let now = 0; + const cache = createCache({ maxAge: 1000, now: () => now }); + cache.set("key", 42); + now += 1500; expect(cache.get("key")).toBeUndefined(); }); ``` - -### Time-Based Testing - -Inject time functions for deterministic time-based tests: - -```typescript -let theTime = 0; -const now = () => theTime; - -const cache = new Cache({ - maxAge: 1000, - now, // Inject time function -}); - -theTime += 500; -expect(cache.get("key")).toBe(42); // Still valid - -theTime += 1000; -expect(cache.get("key")).toBeUndefined(); // Expired -``` - -## Documentation - -### TSDoc Comments - -Document public APIs with TSDoc: - -```typescript -/** - * Creates a handler for the payment scheme. - * - * @param network - The network identifier (e.g., "mainnet", "testnet") - * @param rpc - RPC client - * @param config - Optional configuration options - * @returns Promise resolving to a Handler - */ -export const createHandler = async ( - network: string, - rpc: RpcClient, - config?: HandlerOptions, -): Promise => { ... }; -``` diff --git a/src/agent/prompts.ts b/src/agent/prompts.ts index 84a9ff49f..76e66f711 100644 --- a/src/agent/prompts.ts +++ b/src/agent/prompts.ts @@ -184,13 +184,16 @@ const GUIDELINE_SUB_BLOCKS: Record< "- Unexpected changes in files you did not touch: stop and ask_operator.", ]), ], - scopeConventions: (ctx) => [ + scopeConventions: () => [ "Scope and conventions:", "- Touch only code required for the task; no drive-by refactors, formatting sweeps, or unrelated fixes.", - ctx.subAgent - ? "- Follow AGENTS.md and /docs for architecture." - : "- Follow AGENTS.md and /docs for architecture; use_skill style and philosophy when starting repo work.", + "- Follow AGENTS.md and /docs for architecture.", "- Match existing project patterns (functional style, arktype at boundaries, small focused diffs).", + "- Keep refactors out of feature changes, and do work now rather than deferring it. A TODO marks only work blocked by something outside your control, and names the blocker.", + "- Comments explain why, never what.", + "- Replacing a path deletes the old one: no shims, re-exports, or compatibility wrappers for callers you own (public interfaces excepted).", + "- Fix at the layer that owns the invariant. Two fixes to one subsystem without resolving it means you are chasing symptoms: stop and report where the constraint belongs.", + "- Validate external input once at the boundary and trust it inside. Tests assert required behavior and land with the change.", "- Before finishing implementation work, run the repository-defined typecheck command, relevant tests, and every defined full verification command; these checks are mandatory.", "- If the repository defines no typecheck command, do not invent a typecheck command: report its absence as an explicit Blocker with evidence from AGENTS.md and package scripts (or equivalent project configuration).", "- In Findings, report every exact verification command and its outcome, including exit status. A bare `pass` without command evidence is an incomplete report.", diff --git a/src/extensions/skills.ts b/src/extensions/skills.ts index 13ff1d3de..d989d671a 100644 --- a/src/extensions/skills.ts +++ b/src/extensions/skills.ts @@ -23,7 +23,7 @@ export interface ResolveSkillBodyOptions { pluginRoot?: string; /** * Skip project-local `.agents/.claude/.codex/skills` fallbacks. Attached - * product skills (style/philosophy) use this so a repo SKILL.md cannot + * bundled product skills use this so a repo SKILL.md cannot * become system-prompt constraints. */ pluginDirsOnly?: boolean; diff --git a/src/subagent/run.ts b/src/subagent/run.ts index 6f4f72050..3d8db699e 100644 --- a/src/subagent/run.ts +++ b/src/subagent/run.ts @@ -768,7 +768,7 @@ async function runSubAgentInner( // tool; the scope cannot widen — use_skill refuses names outside the // allowlist and refuses attached/already-loaded names without dumping the // body again. Plugin skill dirs match the primary so bundled - // corbits-skills (style/philosophy) resolve. + // corbits-skills resolve. const modelFamilyPolicy = resolveModelFamilyPolicy({ providerName: params.provider.providerName, model: params.provider.model, diff --git a/src/telemetry/classify.ts b/src/telemetry/classify.ts index d13fcec92..faf49c072 100644 --- a/src/telemetry/classify.ts +++ b/src/telemetry/classify.ts @@ -88,12 +88,10 @@ const FIRST_PARTY_SKILL_NAMES: ReadonlySet = new Set([ "implement", "interview", "issue", - "philosophy", "plan", "pull-request", "refactor", "review", - "style", "typescript", ]);