mirror of
https://github.com/MoonshotAI/kimi-code.git
synced 2026-07-09 17:29:12 +00:00
chore(skills): bundle review and test lenses into kc-review
- add agent-core-review umbrella skill with slop and test sub-skills - move write-tests rules into agent-core-review/test and drop the standalone skill
This commit is contained in:
parent
14626234a4
commit
f85dbdbf03
4 changed files with 267 additions and 100 deletions
19
.agents/skills/agent-core-review/SKILL.md
Normal file
19
.agents/skills/agent-core-review/SKILL.md
Normal file
|
|
@ -0,0 +1,19 @@
|
|||
---
|
||||
name: kc-review
|
||||
description: Use for Kimi Code code review and test write/review guidance. Groups the review and testing lenses used in this repo — `slop` (single-level-of-abstraction / layered error-handling review, invoked only on explicit request) and `test` (contract-driven per-test rules for both authoring and reviewing tests). Apply the sub-skill that matches the task; do not apply `slop` unprompted.
|
||||
has-sub-skill: true
|
||||
---
|
||||
|
||||
# kc-review
|
||||
|
||||
A bundle of the lenses used when reviewing or testing Kimi Code. Each sub-skill is self-contained; invoke the one that matches the task.
|
||||
|
||||
## Sub-skills
|
||||
|
||||
- **`slop/`** — Single Level of Abstraction & layered error handling. A *review dimension*: a function should read as a straight-line description of its own layer, with errors handled above or below. The agent reports detections and measurements, not severity grades. **Invoke only when the user explicitly asks for this lens** — do not apply it unprompted to general reviews or refactors.
|
||||
- **`test/`** — Per-test rules behind "test the contract / responsibility, not the implementation," serving two modes. **Write mode:** author a test — one behavior per `it`, drive through the public surface, stub only the true external boundary, control time/config via documented knobs, keep tests clear, isolated, and refactor-resilient (CCCR). **Review mode:** audit existing tests against the same rules and report findings with `file:line`. Use when writing, modifying, or reviewing tests, or when asked how to write a good single test.
|
||||
|
||||
## Routing
|
||||
|
||||
- Reviewing code structure / abstraction layers / where error handling belongs → `slop` (only on explicit request).
|
||||
- Writing or modifying tests, reviewing test quality, or advising on a single test → `test`.
|
||||
133
.agents/skills/agent-core-review/slop/SKILL.md
Normal file
133
.agents/skills/agent-core-review/slop/SKILL.md
Normal file
|
|
@ -0,0 +1,133 @@
|
|||
---
|
||||
name: slop
|
||||
description: Invoke only when the user explicitly asks to review code through the "single level of abstraction / layered error handling" lens — a function does only its own layer's business logic while errors are handled above or below. The agent reports detections, raw-count measurements, and move directions. Apply only when the user explicitly requests this lens.
|
||||
---
|
||||
|
||||
# Single Level of Abstraction & Layered Error Handling
|
||||
|
||||
North star: **a function should read as a straight-line description of what its own layer does. Anything that is not that — input validation, error handling, error-to-response translation, logging, retries, low-level mechanics — belongs to a layer above or below, not inline.**
|
||||
|
||||
This is a review dimension, not a hard rule. See "Exemption checklist" at the end.
|
||||
|
||||
## Scope of this skill — detect and measure
|
||||
|
||||
The agent applying this lens is a **sensor**. Its one job is to report *whether* a function mixes levels and *by how much*; deciding *how serious* it is belongs downstream. Severity labels (`Block` / `Request changes` / `Nit`) compress a continuous quantity into an uncalibrated three-point scale and are the main source of review-to-review variance, so they are produced downstream — by a deterministic rubric, anchored examples, or a human — from the facts the agent reports.
|
||||
|
||||
The agent's output is exactly these four things:
|
||||
|
||||
- **Detection (yes/no):** does this statement / block / function violate a rule of the lens?
|
||||
- **Measurement (raw factual counts only):** mechanically countable quantities — body size, control-flow keywords, named syntactic shapes (see "Quantify"). Anything that first requires classifying a line (core/foreign, happy/error, high/low level) is recorded under detection, not here.
|
||||
- **Direction (where it moves):** for each foreign concern, the destination layer — push **down** into a value / parser / infra helper, or push **up** into the edge handler.
|
||||
- **Exemption flags:** which items, if any, hit the exemption checklist — recorded, not weighed.
|
||||
|
||||
Severity grades, merge/block verdicts, and "is splitting worth it" calls live downstream, derived from the four items above.
|
||||
|
||||
## When to use
|
||||
|
||||
Apply this lens only when the user asks for it explicitly (for example "用单一抽象层次审视一下", "check whether this function does too much", "errors should be handled above/below, right?"). Leave general reviews and refactors to other lenses unless the user names this one.
|
||||
|
||||
## The principle
|
||||
|
||||
One function, one level of abstraction, one responsibility. Three mutually reinforcing rules:
|
||||
|
||||
1. **Single Level of Abstraction (SLAP).** Every statement inside a function sits at the same conceptual level. High-level intent ("reserve inventory, charge payment, create the order") must not be interleaved with low-level mechanics (building headers, escaping strings, opening sockets, parsing bytes). If some lines read as "what" and others as "how", they belong in different functions.
|
||||
2. **Error handling is its own concern (Clean Code).** A function either does the work or handles the error — not both. Business logic describes the happy path and *signals* failure (throw or return a result); the catch, mapping, logging, and recovery live in a dedicated handler, usually one layer up. Prefer exceptions / result types over threaded check-and-return ladders that interrupt the main flow.
|
||||
3. **Separation of concerns by layer.** Each layer owns exactly one kind of knowledge: low-level code knows formats and protocols; mid-level code knows business rules; edge code knows the outside world (HTTP / CLI / UI). A function that knows two of these at once is leaking a layer.
|
||||
|
||||
The combined test: **could you explain this function to someone without using the word "and"?** If the explanation is "it reserves stock AND validates the email format AND maps the error to a status code AND logs to metrics", it is doing more than its layer's job.
|
||||
|
||||
Concerns that usually do **not** belong in a business function:
|
||||
|
||||
- Format / range / null validation that a lower value or parser could guarantee once.
|
||||
- Mapping domain failures to an external protocol (status code, exit code, UI message) — that is the edge layer's job.
|
||||
- Catch-and-swallow, retry loops, backoff, timeout, circuit breaking around a single call — infrastructure, push down.
|
||||
- Cross-cutting telemetry / log / metric noise woven through every step — extract or push to a wrapper.
|
||||
- Check-and-return ladders that occupy more space than the business core — replace with signal + a handler above.
|
||||
|
||||
## Methodology — fixing a function that violates it
|
||||
|
||||
Work top-down. Never start by shuffling lines.
|
||||
|
||||
1. **Name the level.** In one sentence, write what this function is for at its own layer. If you cannot, the function has no clear level — split before polishing.
|
||||
2. **Classify every statement.** Tag each line or block as: **core** (this layer's business), **down** (a detail a lower abstraction should own), **up** (a concern an upper / edge layer should own), or **cross-cutting** (log / metric / retry). Unlabeled lines are where the mess hides — do not "just leave them".
|
||||
3. **Decide down vs. up for each foreign item.**
|
||||
- Push **down** when it is a guarantee a lower building block can provide: a value that can only be constructed valid, a parser that returns a typed result, an infra helper that already retries. The business function then assumes validity and stays clean.
|
||||
- Push **up** when it is about translating or reacting to failure for the outside world: status codes, messages, exit codes, aggregation of many errors. The edge layer catches once and maps; business code just signals.
|
||||
- Rule of thumb: if removing it would change what the business rule says, it is core and stays; if removing it only changes how a failure is reported or a detail is computed, it moves.
|
||||
4. **Extract, do not interleave.** Pull each foreign concern into its own named function or layer. Keep the original function as a readable sequence of same-level calls. For error handling specifically, separate the work body from the recovery body into distinct functions so neither clutters the other.
|
||||
5. **Signal, do not handle, in the middle.** Mid-layer business functions throw / return and let the right layer react. Do not catch-and-log-and-continue in business code unless continuing is itself the business rule.
|
||||
6. **Re-read for level.** After the moves, every remaining line should be explainable at the same altitude. If not, repeat from step 1.
|
||||
|
||||
Keep the change minimal: move the smallest thing that restores the level. Do not invent abstractions, frameworks, or generic "handler" machinery beyond what the function actually needs. Three straight-line, same-level calls beat a premature pipeline.
|
||||
|
||||
## Review method — applying the lens to a diff
|
||||
|
||||
Read each changed or touched function and, for each check, record only: **the hit (yes/no) plus evidence (`file:line`)**, and — where the check points at a construct — a raw factual count from "Quantify".
|
||||
|
||||
1. **Altitude check.** Are all lines at the same level of abstraction? Record each place where a "what" line is immediately followed by a "how" block (or vice versa) inside the same function, with `file:line`.
|
||||
2. **Happy-path check.** Can you read the business intent top to bottom without stepping through error branches? Record whether error handling sits inline between business steps (yes/no + `file:line`), supported by raw counts from "Quantify" (e.g. number of `catch` clauses, `continue` statements).
|
||||
3. **Ownership check.** For each validation, catch, mapping, log, retry: is this layer the rightful owner, or is it borrowed from above / below? Record each borrowed item with `file:line` and its destination (down / up), using the rules from the methodology.
|
||||
4. **Layer-leak check.** Does a business function mention an external protocol (status code, exit code, UI text, wire field)? Does an edge function contain a business rule? Record each leak candidate with `file:line` and whether it names an *external* protocol or an *internal* domain shape.
|
||||
5. **Explanation test.** Describe the function in one sentence with no "and". Record whether "and" was needed; if so, list the proposed split as candidate moves (down / up).
|
||||
|
||||
### Quantify — report only raw factual counts
|
||||
|
||||
Report only quantities that can be counted **mechanically from the text**. Anything that first requires classifying a line (core vs foreign, happy-path vs error-handling, high-level vs low-level) is recorded under detection (the five checks above) as evidence, not as a number here.
|
||||
|
||||
Report, per function:
|
||||
|
||||
- **Body size** — lines and/or statements of the function body; state the basis (e.g. "statements, excluding lone braces").
|
||||
- **Control-flow keywords (raw counts)** — `if`, `continue`, early `return`, `throw`, `try` / `catch` / `finally`, `await`, loops (`for` / `while` / `.forEach`).
|
||||
- **Named syntactic shapes a check points at** — when a check cites a construct, count it verbatim and name the exact token: e.g. number of object literals, string literals, `.trim()` calls, `.length` reads, `origin.` property reads, spread `[...x]` operations.
|
||||
- **Recovery presence (raw)** — number of `catch` clauses, and number of log / metric calls inside them.
|
||||
|
||||
Quantities that embed a prior classification — out-of-level vs core counts, guard-to-core ratios, happy-path vs error-handling volume, "repeated boundary checks a lower layer could guarantee once", "low-level literals in a high-level flow" — are captured as evidence under the relevant check (`file:line` + the verbatim tokens). A downstream rubric derives any ratio from those raw facts.
|
||||
|
||||
### Red flags
|
||||
|
||||
Record each as evidence (yes/no + `file:line`); these are candidates, not verdicts:
|
||||
|
||||
- A body that is mostly check-and-return / check-and-throw ladders around a thin core.
|
||||
- A recovery block that logs, maps, and returns inline, sitting next to business steps.
|
||||
- A function that both computes a value and decides how that value's failure is shown to the user.
|
||||
- Low-level literals (byte offsets, header strings, format codes) inside a high-level workflow.
|
||||
- A name that needs "And" / "Or" / "With" to be honest, or a name so vague ("handle", "process", "do") that it hides multiple levels.
|
||||
- Catch-and-swallow that hides a failure the caller needed to see.
|
||||
- Defensive null / format checks repeated at every call site instead of guaranteed once at the boundary.
|
||||
|
||||
### Severity grading belongs downstream
|
||||
|
||||
The agent's facts (detections, raw counts, directions, exemptions) feed a downstream grade; the agent reports those facts and stops there. Grades compress a continuous quantity into an uncalibrated three-point scale and are exactly where identical evidence gets labeled differently across runs. Grading happens above the agent:
|
||||
|
||||
- A **deterministic rubric** — a versioned threshold table over the raw counts from "Quantify"; or
|
||||
- **Anchored examples** — the reviewer judges relative to repo-known reference functions rather than against an absolute adjective like "materially"; or
|
||||
- A **human**, for items that land near a threshold boundary.
|
||||
|
||||
If a downstream consumer still asks the agent for a grade, the agent returns the underlying facts and the threshold band it would fall under, with `confidence: low` on boundary cases; the grade itself is produced downstream.
|
||||
|
||||
### How to report findings
|
||||
|
||||
Report **evidence + direction**. Lead with the location and the level, then the proposed move. Prefer "this block is one level lower than the rest of the function (`file:line`) — move it **down** into X" over "this is ugly" or "this is a request-changes". The destination layer (down into a value / parser / infra helper, or up into the edge handler) is the actionable output and the deliverable. Attach the "Quantify" numbers and any exemption flags to each finding.
|
||||
|
||||
## Exemption checklist
|
||||
|
||||
This is a lens, not a law. For each foreign concern, check whether any exemption below applies and **record the hit (yes/no) plus the reason**. The agent records exemptions as facts; a recorded exemption is then used downstream to cap the grade (e.g. to `Nit`) deterministically.
|
||||
|
||||
- **Tiny function:** the function is small enough that splitting would add indirection with no reader benefit.
|
||||
- **Foreign concern is the single job:** the "foreign" concern is in fact the function's one purpose — a dedicated error mapper, a validator, an infra wrapper, or an index-bookkeeping helper whose low-level arithmetic *is* its level.
|
||||
- **Atomicity / correctness / performance:** the steps genuinely must stay together (e.g. a re-check after an `await` to guard state that may have changed).
|
||||
- **Edge-translator role:** an edge / handler function whose job is to translate an external event into internal indices; naming the wire fields is its job.
|
||||
|
||||
Keep a split that would make the code harder to read as a recorded candidate for downstream review. When the evidence lands on an exemption boundary, record both sides and set `confidence: low`.
|
||||
|
||||
## Output contract
|
||||
|
||||
Return, per function, items 1–5 only:
|
||||
|
||||
1. **Level statement** — one sentence: what the function is for at its own layer.
|
||||
2. **Per-check results** — for each of the five review checks: `hit: yes/no`, evidence `file:line`, and (only where the check points at a construct) a raw factual count.
|
||||
3. **Measurements** — the raw factual counts from "Quantify".
|
||||
4. **Exemptions** — checklist hits (yes/no + reason).
|
||||
5. **Proposed moves** — for each foreign concern: `file:line` → destination (down into X / up into Y). This is the actionable deliverable.
|
||||
|
||||
Severity grades, block/merge verdicts, and "worth splitting" calls live downstream, derived from items 1–4. When a consumer asks for a label, hand back items 1–4 and the threshold band, with `confidence: low` on boundary cases.
|
||||
115
.agents/skills/agent-core-review/test/SKILL.md
Normal file
115
.agents/skills/agent-core-review/test/SKILL.md
Normal file
|
|
@ -0,0 +1,115 @@
|
|||
---
|
||||
name: test
|
||||
description: Use when writing or reviewing tests, or when asked how to write a good single test. Encodes the per-test rules behind the "test the contract / responsibility, not the implementation" principle — name and structure one behavior per `it`, drive through the public surface, stub only true external boundaries, control time and config via documented knobs, and keep tests clear, isolated, and refactor-resilient. The same rules drive both authoring (write mode) and auditing existing tests (review mode).
|
||||
---
|
||||
|
||||
# Tests — write & review
|
||||
|
||||
Per-test rules that operationalize one principle: **test the contract / responsibility, not the implementation**. This is the how-to for a single `it`, and the lens for reviewing one.
|
||||
|
||||
## Two modes, one rule set
|
||||
|
||||
- **Write mode** — authoring a test. Apply the rules below to produce it.
|
||||
- **Review mode** — auditing an existing test or test diff. Apply the same rules as a checklist; report each violation with `file:line`, the rule it breaks, and the fix. See "Review mode" near the end.
|
||||
|
||||
The rules are identical in both modes — only the posture changes (produce vs. audit).
|
||||
|
||||
## Test contract, not implementation
|
||||
|
||||
- Drive the system through its **public control plane** and assert on **observable effects** (returned values, persisted state, emitted events, injected messages), never on source details.
|
||||
- Resolve collaborators through their contract — the interface plus its identifier — not the module that binds a concrete implementation.
|
||||
- Do not reach into private fields or add backdoors "for testing". If you feel the need, the seam is wrong — fix the design, not the test.
|
||||
|
||||
## One behavior per `it`
|
||||
|
||||
Each `it` covers exactly one responsibility / scenario. If the name needs "and", split it.
|
||||
|
||||
```ts
|
||||
it('returns 401 when the caller is unauthorized', ...);
|
||||
it('does not double-fire when the same tick repeats', ...);
|
||||
```
|
||||
|
||||
## Name and structure
|
||||
|
||||
- `describe('<slice> (<responsibilities>)'` — name the **responsibility**, not the class.
|
||||
- An `it(...)` reads as a sentence, but it must still encode three things — the **behavior / method**, the **state or condition**, and the **expected outcome**: `it('<behavior> when <condition>, <outcome>')`. A name like `does X when Y` with no result is too vague to fail usefully.
|
||||
- Use spaces, not the Java-style `method_state_outcome` underscores — that convention exists only because Java test methods cannot contain spaces. A string-named test reads fine as a sentence.
|
||||
- Good: `it('returns 401 when the caller is unauthorized')` · `it('advances the cursor and does not double-fire on a repeat tick')`
|
||||
- Bad: `it('works')` · `it('handles auth correctly')` — no condition, no outcome
|
||||
- Arrange / Act / Assert. A short `// Given` `// When` `// Then` is fine when it aids reading; do not paste it mechanically on trivial tests.
|
||||
|
||||
## Build a small rig
|
||||
|
||||
When several tests share setup, write a factory (`rig()`, `createHost()`, whatever fits the codebase) that returns the **smallest surface the test needs**. Tests reach into the rig; they do not rebuild the world each time. Keep the rig dumb: wiring only, no assertions.
|
||||
|
||||
## Stub only the real external boundary
|
||||
|
||||
Default to real collaborators wired the way production wires them. Stub the **minimum seam** that is genuinely external:
|
||||
|
||||
- A remote / model / service boundary — spy on the contract method (the interface), and capture what the system sends across it. Do not stand up the real external thing.
|
||||
- Network / other-process boundaries — stub at the boundary, not the internals.
|
||||
- Time, timers, jitter — use the documented control knobs the system exposes (env, an injected clock, a manual tick). Do **not** use fake timers or real `setTimeout` to drive time.
|
||||
- Env / config knobs are usually snapshotted at bootstrap — set them **before** building the system under test, and restore them in `afterEach`.
|
||||
|
||||
## Keep tests DAMP and keep cause next to effect
|
||||
|
||||
- DAMP over DRY: use **literal expected values** in assertions; do not compute the expectation with the same logic as the code under test.
|
||||
- Keep the key preconditions inside the `it` (or its rig), where the reader can see cause next to effect. Reserve `beforeEach` for cross-cutting plumbing (env snapshot, cleanup), not for hiding the scenario's setup.
|
||||
|
||||
```ts
|
||||
// Good — the expected value is a literal the reader can check.
|
||||
expect(discount).toBe(15);
|
||||
// Bad — re-derives the expectation; mirrors the implementation.
|
||||
expect(discount).toBe(price * rate);
|
||||
```
|
||||
|
||||
## Assert only what is relevant
|
||||
|
||||
Assert the effect that proves the contract. Use matchers / partial-object matching to ignore incidental fields. Do not assert internal counters, call orders, or shapes the user cannot rely on.
|
||||
|
||||
## Isolate and clean up (no flakes)
|
||||
|
||||
Every test must be hermetic and order-independent. In `afterEach`:
|
||||
|
||||
- restore every mock / spy
|
||||
- restore every env var you touched (snapshot in `beforeEach`)
|
||||
- dispose the host / container and reset its reference
|
||||
|
||||
No dependence on wall-clock time, run order, or leftover on-disk state — give each scenario its own isolated identity / workspace when state persists.
|
||||
|
||||
## Quality bar: CCCR
|
||||
|
||||
Before finishing, check each test against:
|
||||
|
||||
- **Clarity** — a stranger can tell what broke from the failure message alone.
|
||||
- **Completeness** — covers the responsibility's success, error, and boundary paths.
|
||||
- **Conciseness** — no duplicate or speculative cases; one scenario per `it`.
|
||||
- **Resilience** — survives an internal refactor with no test change (because it asserts contract, not implementation).
|
||||
|
||||
## Per-file scenario header
|
||||
|
||||
Start each test file with a short header comment: the **scenario**, the **responsibilities** asserted, the **wiring** (which collaborators are real vs. the single stubbed boundary), and how to run it.
|
||||
|
||||
## Review mode — auditing existing tests
|
||||
|
||||
Apply the rules above as a checklist against each test in scope (a file, a diff, or a named `it`). For every hit, report `file:line` + the rule it breaks + the fix; do not rewrite unless asked. Lead with the contract question: *what observable behavior does this test prove, and would it survive a refactor?*
|
||||
|
||||
Check, in order:
|
||||
|
||||
1. **Contract, not implementation** — asserts observable effects, not private fields, call order, or internal shapes the user cannot rely on.
|
||||
2. **One behavior per `it`** — the name carries behavior + condition + outcome; "and" in the name means a split is owed.
|
||||
3. **Boundary discipline** — only the true external seam is stubbed; time is driven by documented knobs, not fake timers / real `setTimeout`.
|
||||
4. **DAMP expectations** — expected values are literals, not re-derived by the code under test's logic.
|
||||
5. **Isolation** — mocks / spies / env / host restored in `afterEach`; no wall-clock, run-order, or leftover on-disk dependence.
|
||||
6. **CCCR read-through** — Clarity, Completeness (success / error / boundary), Conciseness, Resilience.
|
||||
|
||||
Report findings as evidence + fix, e.g. "`foo.test.ts:42` asserts on `service.internalMap` (contract) — assert the returned value instead." If a test passes the lens, say so briefly; silence on a rule means it held.
|
||||
|
||||
## Quick checklist (write & review)
|
||||
|
||||
- Resolved through the contract; no concrete-impl import
|
||||
- One behavior per `it`; name carries behavior + condition + outcome; AAA
|
||||
- Stubbed only the true external seam; time via knobs, not fake timers
|
||||
- Literal expectations; relevant assertions only
|
||||
- Mocks / env / host restored in `afterEach`; hermetic, no flakes
|
||||
- CCCR read-through done
|
||||
|
|
@ -1,100 +0,0 @@
|
|||
---
|
||||
name: write-tests
|
||||
description: Use when writing or modifying the black-box example tests in kimi-code-mini-bench (`examples/*.example.ts`, run with `pnpm example`), or when asked how to write a good single test. Encodes the per-test writing rules that sit under AGENTS.md's "test contract, not implementation" principle — name and structure one behavior per `it`, drive through the public surface, stub only true external boundaries, control time/config via documented knobs, and keep tests clear, isolated, and refactor-resilient.
|
||||
---
|
||||
|
||||
# Write Tests (kimi-code-mini-bench)
|
||||
|
||||
Per-test writing rules for the `examples/*.example.ts` black-box tests. They operationalize the top-level rule in `AGENTS.md` (**test contract / responsibility, not implementation**) — read that first; this skill is the how-to for a single `it`.
|
||||
|
||||
## File placement & run
|
||||
|
||||
- Tests live under `examples/` and MUST be named `*.example.ts` — `vitest.config.ts` only includes `examples/**/*.example.ts`.
|
||||
- Run one file: `pnpm example -- examples/<name>.example.ts`. Run all: `pnpm test`.
|
||||
- `pnpm check:blackbox` MUST pass before you submit. It fails any test that imports an **impl module** (a source file with a top-level `registerScopedService(...)`). Resolve services through `accessor.get(IX)` instead.
|
||||
|
||||
## Test contract, not implementation
|
||||
|
||||
- Drive the system through its **public control plane** and assert on **observable effects** (returned values, persisted state, injected messages), never on source details.
|
||||
- Resolve services via the contract: `host.session.accessor.get(IX)` / `host.app.accessor.get(IX)`. Import only the interface + its `ServiceIdentifier`, never the module that binds the concrete impl.
|
||||
- Do not reach into private fields or add backdoors "for testing". If you feel the need, the seam is wrong — fix the design, not the test.
|
||||
|
||||
## One behavior per `it`
|
||||
|
||||
Each `it` covers exactly one responsibility / scenario. If the name needs "and", split it.
|
||||
|
||||
```ts
|
||||
it('fires a one-shot task by steering the main agent, then auto-deletes it', ...);
|
||||
it('does not fire while the agent is busy, then fires once it goes idle', ...);
|
||||
```
|
||||
|
||||
## Name and structure
|
||||
|
||||
- `describe('<slice> (<responsibilities>)'` — name the **responsibility**, not the class.
|
||||
- An `it(...)` reads as a sentence, but it must still encode three things — the **behavior / method**, the **state or condition**, and the **expected outcome**: `it('<behavior> when <condition>, <outcome>')`. A name like `does X when Y` with no result is too vague to fail usefully.
|
||||
- Use spaces, not the Java-style `method_state_outcome` underscores — that convention exists only because Java test methods cannot contain spaces. vitest `it()` takes a string, and the repo already reads this way, e.g. `it('fires a one-shot task by steering the main agent, then auto-deletes it')`.
|
||||
- Good: `it('returns 401 when the caller is unauthorized')` · `it('advances the cursor and does not double-fire on a repeat tick')`
|
||||
- Bad: `it('works')` · `it('handles auth correctly')` — no condition, no outcome
|
||||
- Arrange / Act / Assert. A short `// Given` `// When` `// Then` is fine when it aids reading; do not paste it mechanically on trivial tests.
|
||||
|
||||
## Build a small rig
|
||||
|
||||
When several tests share setup, write a `rig()` (or use `createSliceHost`) that returns the **smallest surface the test needs** — e.g. `{ cron, steered, setNow, setActiveTurn }`. Tests reach into the rig; they do not rebuild the world each time. Keep the rig dumb: wiring only, no assertions.
|
||||
|
||||
## Stub only the real external boundary
|
||||
|
||||
Default to real collaborators wired by `_harness`. Stub the **minimum seam** that is genuinely external:
|
||||
|
||||
- LLM — spy on the contract method, e.g. `vi.spyOn(main.accessor.get(IAgentPromptService), 'steer').mockImplementation(...)`, and capture the injected message. Do not spin up a real turn.
|
||||
- Network / other process boundaries — stub at the boundary, not the internals.
|
||||
- Time, timers, jitter — use the documented control knobs (`KIMI_CRON_CLOCK=file:<path>` + rewrite the file to advance time; `KIMI_CRON_MANUAL_TICK=1`; `KIMI_CRON_NO_JITTER=1`). Do **not** use `vi.useFakeTimers()` or real `setTimeout` to drive time.
|
||||
- Env knobs are snapshotted at bootstrap — set them **before** `createSliceHost(...)`, and restore them in `afterEach`.
|
||||
|
||||
## Keep tests DAMP and keep cause next to effect
|
||||
|
||||
- DAMP over DRY: use **literal expected values** in assertions; do not compute the expectation with the same logic as the code under test.
|
||||
- Keep the key preconditions inside the `it` (or its rig), where the reader can see cause next to effect. Reserve `beforeEach` for cross-cutting plumbing (env snapshot, cleanup), not for hiding the scenario's setup.
|
||||
|
||||
```ts
|
||||
// Good — the expected value is a literal the reader can check.
|
||||
expect(cron.getNextFireTime()).toBe(BASE + MINUTE);
|
||||
// Bad — re-derives the expectation; mirrors the implementation.
|
||||
expect(cron.getNextFireTime()).toBe(computeNextSlot(BASE, '* * * * *'));
|
||||
```
|
||||
|
||||
## Assert only what is relevant
|
||||
|
||||
Assert the effect that proves the contract. Use matchers / `expect.objectContaining` to ignore incidental fields. Do not assert internal counters, call orders, or shapes the user cannot rely on.
|
||||
|
||||
## Isolate and clean up (no flakes)
|
||||
|
||||
Every test must be hermetic and order-independent. In `afterEach`:
|
||||
|
||||
- `vi.restoreAllMocks()`
|
||||
- restore every env var you touched (snapshot in `beforeEach`)
|
||||
- `host?.dispose()` and reset the `host` reference
|
||||
|
||||
No dependence on wall-clock time, run order, or leftover on-disk state — give each scenario its own `workspaceId` / home when state persists.
|
||||
|
||||
## Quality bar: CCCR
|
||||
|
||||
Before finishing, check each test against:
|
||||
|
||||
- **Clarity** — a stranger can tell what broke from the failure message alone.
|
||||
- **Completeness** — covers the responsibility's success, error, and boundary paths.
|
||||
- **Conciseness** — no duplicate or speculative cases; one scenario per `it`.
|
||||
- **Resilience** — survives an internal refactor with no test change (because it asserts contract, not implementation).
|
||||
|
||||
## Per-file scenario header
|
||||
|
||||
Start each `*.example.ts` with a short header comment: the **scenario**, the **responsibilities** asserted, the **wiring** (which collaborators are real vs. the single stubbed boundary), and the **Run** line. Match the existing files (e.g. `cron.example.ts`).
|
||||
|
||||
## Quick checklist
|
||||
|
||||
- `*.example.ts` under `examples/`; `pnpm check:blackbox` passes
|
||||
- Resolved through `accessor.get(IX)`; no impl-module import
|
||||
- One behavior per `it`; name carries behavior + condition + outcome; AAA
|
||||
- Stubbed only the true external seam; time via knobs, not `useFakeTimers`
|
||||
- Literal expectations; relevant assertions only
|
||||
- Env/mocks/host restored in `afterEach`; hermetic, no flakes
|
||||
- CCCR read-through done
|
||||
Loading…
Add table
Add a link
Reference in a new issue