docs: trim generated prose
This commit is contained in:
Tianyi Cui
@@ -5,53 +5,35 @@ description: Use when reviewing a pull request in the deepseek-harness repo —
|
||||
|
||||
# Reviewing a DeepSeek-Harness PR
|
||||
|
||||
**This skill is guidance, not a complete checklist.** It is a where-to-look map that lowers your startup cost on an unfamiliar PR — clearing every item here does not mean the PR is good. You are the reviewer: reason independently from the code in front of you, and think broadly across every dimension a change can fail on. The items below are the failure modes this repo has already paid for; a real review also catches the ones nobody has written down yet.
|
||||
Read the diff and enough surrounding code to understand the design, then verify suspected defects before reporting them. Prioritize correctness, lifecycle, security, and contract failures over style; a short review with one substantiated blocker is better than a list of nits.
|
||||
|
||||
Independent judgment governs *what to look at* and *how to apply a rule to this case* — not whether the repo's documented requirements still hold. AGENTS.md, packages/AGENTS.md, and the [quality gates](../../../docs/rfc/implemented/process/2026-06-11-quality-gates.md) remain authoritative; a missing HMR-safety test or out-of-sync docs is a blocking gap regardless of your judgment, not a suggestion you can waive. Use your own reasoning to go *beyond* these checks and to weigh genuine edge cases against an RFC (raise it as a discussion, don't silently override) — never to demote a documented blocker to optional.
|
||||
## Sources of truth
|
||||
|
||||
## How to think about a review
|
||||
- [AGENTS.md](../../../AGENTS.md) and [packages/AGENTS.md](../../../packages/AGENTS.md): repository and package rules.
|
||||
- [docs/defensive-patterns.md](../../../docs/defensive-patterns.md): subprocess, callback, async-state, and disposal bug classes.
|
||||
- [docs/AGENTS.md](../../../docs/AGENTS.md): documentation placement and prose discipline.
|
||||
- [docs/testing.md](../../../docs/testing.md) and the [quality-gates RFC](../../../docs/rfc/implemented/process/2026-06-11-quality-gates.md): required test tiers and gates.
|
||||
- [RFC index](../../../docs/rfc/README.md): design rationale. Treat disagreement with an RFC as a design discussion, not an automatic veto.
|
||||
- For bilingual changes, read [translation-rules.md](../../../docs/i18n/translation-rules.md), [terminology.md](../../../docs/i18n/terminology.md), and [dsh-translate-docs](../dsh-translate-docs/SKILL.md).
|
||||
|
||||
- **Reason from the code, not from this list.** Read the diff and enough surrounding context to understand what the change actually does, then ask what could go wrong — independently of whether this skill names it. The named patterns are a floor, not a ceiling.
|
||||
- **Think broadly, across many aspects.** A change can be wrong in correctness, concurrency/lifecycle, error handling, security, performance, API/contract design, type safety, test quality, docs sync, naming, readability, or backward compatibility. Also challenge the *approach itself*: is this the right design, are its assumptions sound, where does it fail under real-world conditions? Don't tunnel on the first defect you spot or stop at the checklists below — sweep all of them.
|
||||
- **Verify before you flag.** Check a suspected issue against the actual codebase (grep the symbol, read the caller, confirm the path is reachable) before raising it. An unverified claim wastes the author's time and erodes trust in the review.
|
||||
- **Calibrate confidence; suppress noise.** Distinguish a blocking bug from a nitpick and say which is which. Don't raise things a gate already enforces (typecheck, lint, formatting, type errors, broken tests), pre-existing issues on lines the PR didn't touch, or pedantic style a senior engineer would let slide. When unsure whether something is real, investigate or frame it explicitly as a question rather than a finding.
|
||||
- **Severity, not volume.** Lead with what blocks merge. A short review that names the one real bug beats a long one that buries it under nits.
|
||||
## Blocking requirements
|
||||
|
||||
## Sources of truth (read, don't re-summarize)
|
||||
1. **Docs match the code.** Config, defaults, errors, wire fields, events, and public behavior update the package README and JSDoc in the same diff. Comments state non-obvious contracts; flag implementation narration, test walkthroughs, review history, and duplicated rationale for deletion or a link to their one home.
|
||||
2. **Core type docs match.** Changes to spine or seam vocabulary update the appropriate [core-data-structures](../../../docs/core-data-structures/core.md) page and any `type-equiv` entry. Internal types need no catalog entry.
|
||||
3. **Registrations clean up.** A new registry contribution has a test that disposes its owner and observes removal.
|
||||
4. **Required gates pass.** Trust typecheck, lint, coverage, build, hygiene, doc-sync, and module-graph checks for what they enforce; review the semantic gaps they cannot detect.
|
||||
|
||||
These define the conventions and gates this repo is checked against, and they are authoritative. Read them at the source so this skill never drifts out of sync — and apply judgment in *interpreting* them for the case at hand, not in deciding whether they apply.
|
||||
## Manual checks
|
||||
|
||||
- **[AGENTS.md](../../../AGENTS.md) § Conventions** — effect-based registrations, declaration-merging for events/ctx keys, waterfall `next()` discipline, discriminated-union match-don't-chain, explicit-over-implicit at seams, no hardcoded tunables in plugins, the empty-`catch` rule, symmetry.
|
||||
- **[docs/defensive-patterns.md](../../../docs/defensive-patterns.md)** — each section is a bug class that bit us. Reviewing anything touching process lifecycle, async/await, disposal, or adapter error paths? Re-read this first — then look for the *adjacent* mistake it doesn't name.
|
||||
- **AGENTS.md § Type safety and documentation + [docs/AGENTS.md](../../../docs/AGENTS.md)** — the doc-sync rule (code change ⇒ update README + JSDoc in the SAME commit) and the writing rules (current-state-never-history, one line per paragraph, one home per fact, the word-budget gate).
|
||||
- **[packages/AGENTS.md](../../../packages/AGENTS.md)** — per-package conventions (file layout, the HMR-safety test requirement).
|
||||
- **[docs/i18n/translation-rules.md](../../../docs/i18n/translation-rules.md) and [docs/i18n/terminology.md](../../../docs/i18n/terminology.md)** — the authoritative standard for bilingual-doc review: faithfulness, structure, typography, and the binding terminology table. For PRs touching translated docs or pending terms, read these before judging the translation; [dsh-translate-docs](../dsh-translate-docs/SKILL.md) is the translator workflow.
|
||||
- **[RFC index](../../../docs/rfc/README.md)** — the *why* behind the architecture. Especially [quality gates](../../../docs/rfc/implemented/process/2026-06-11-quality-gates.md) (what a PR must pass) and [capability seams](../../../docs/rfc/implemented/architecture/2026-06-13-capability-seams.md) (the three-package split). If a change seems to fight an RFC, that's a discussion, not a silent override — and not an automatic veto either: an RFC can be wrong for this case, so reason about it.
|
||||
- **Intent and seam contracts:** trace both sides of every changed interface. Confirm the implementation matches the PR and any RFC, including errors, cancellation, ownership, and disposal.
|
||||
- **Lifecycle and concurrency:** for async setup, callbacks, processes, or teardown, apply [defensive-patterns.md](../../../docs/defensive-patterns.md). Check races before publication, cancellation during awaits, independent error reporting, callback containment, and quiescent disposal.
|
||||
- **Capability shape:** a swappable capability follows the interface / implementation / consumer split. Consumers depend on the interface, not a backend.
|
||||
- **Configuration:** deployment-varying timeouts, caps, models, URLs, paths, and retry counts are validated `Config` fields, not literals or `DEFAULT_*` constants.
|
||||
- **Real entry path:** tests exercise the shipped Loader, bin, worker, ACP bridge, or subprocess where relevant. A hand-mounted plugin does not catch Loader export-shape failures; a function plugin must named-export its namespace and have no default export.
|
||||
- **Test strength:** assertions fail on the intended regression and verify external state, logs, events, or disposal rather than restating the implementation or trusting an agent's report. Coverage is necessary but not evidence that the scenario is correct.
|
||||
- **Transcript changes:** editor-visible or model-visible changes update snapshots or explain why no snapshot applies. Review golden diffs as behavior changes, not formatting noise.
|
||||
- **Bilingual changes:** compare meaning and terminology on both sides; a green pairing hash does not prove translation quality.
|
||||
|
||||
## Hard blockers (documented requirements — missing one blocks merge)
|
||||
## Reporting findings
|
||||
|
||||
These come straight from the source docs above. They are not discretionary; absence is a blocking gap.
|
||||
|
||||
1. **Docs in sync.** If the PR changes a config key, default, error code, wire field, or event name, it must update the package README + module/JSDoc in the same diff. The `doc-sync` gate (check #4) does not catch prose drift in config keys, defaults, error codes, or wire fields — that is on the reviewer, but it is still required, not optional.
|
||||
2. **Core-data-structures catalog in sync.** If the PR adds, removes, or reshapes a type the [core-data-structures catalog](../../../docs/core-data-structures/core.md) documents — a new `…Map` variant, a new content-block/session-event type, a field on `GenerateOptions`/`Agent`/`ToolDefinition`/a bash type, or a whole new core/seam type — it must update that catalog in the same diff (prose + any verbatim ` ```ts type-equiv ` block + the 1:1 `scripts/type-equiv.manifest.json`). The `verify-type-equiv` gate (part of `doc-sync`) catches a *drifted paste* of an already-documented type, but it cannot tell you a brand-new core type went undocumented — that judgment is yours. Confirm a genuinely spine-level type landed in core.md and a new capability's vocabulary on a sub-page, per the spine-vs-seam line in [core.md § What counts as "core"](../../../docs/core-data-structures/core.md#what-counts-as-core). A pure internal type with no cross-package reach needs no catalog entry — say so if it's a judgment call.
|
||||
3. **HMR-safety test.** Any new registry/registration needs a test that disposes the contributing fiber and asserts cleanup (packages/AGENTS.md). Its absence blocks merge.
|
||||
4. **Quality gates pass.** typecheck, lint, test, test:coverage (100% per-file on `packages/*/src`), knip, build, publint, constraints, `doc-sync` (the full gate list is the `doc-sync` script in the root `package.json`), module-graph freshness (the quality-gates RFC). Don't re-review what a gate already enforces — trust the gate and spend attention on what it can't check. Note that `doc-sync` only covers compilable `ts` blocks, generated-catalog freshness, markdown wrapping/links/refs, verbatim type-equiv blocks, word budgets, and the bilingual pairing contract ([docs/i18n/README.md](../../../docs/i18n/README.md)); prose drift (checks #1 and #2) and translation *quality* (the [dsh-translate-docs](../dsh-translate-docs/SKILL.md) rules) are *additional* manual review on top of it, not covered by it.
|
||||
|
||||
## Reviewer-only checks (gates can't catch these — judgment required)
|
||||
|
||||
Where your independent reasoning earns its keep. Start here, then keep going across the broader aspects above.
|
||||
|
||||
- **e2e verifies the world, not the agent's self-report.** For real-API tests, confirm the assertion re-runs the command/checks the file externally — a keyword probe lets a cheating agent pass. For a behavior change to the agent's real flows, a no-key/mock test alone is usually insufficient: a with-key e2e (especially a smoke test that boots the real example and checks the world) is cheap here and catches "green units, broken product" — encourage it rather than treating real-API tests as expensive (see [docs/testing.md](../../../docs/testing.md)).
|
||||
- **Plugin export shape + real-loader coverage.** A new/changed `cordis.yml`-loaded plugin: is it a function/namespace plugin (`name`/`inject`/`Config`/`apply` named exports) with NO `export default`? A stray default export makes the Loader's `unwrapExports` drop `inject` and the plugin crashes at load with `cannot get property … without inject` — invisible to hand-built `ctx.plugin({...})` tests and to line coverage. Confirm there's a test driving it through the REAL loader path (the no-key subprocess e2e for ACP is the model). And any opportunistic read of a service NOT in `static inject` should use `ctx.get(name)`, not `ctx.<name>` (the property proxy throws through a foreign shadow). See [packages/AGENTS.md](../../../packages/AGENTS.md) and [docs/postmortem/0001](../../../docs/postmortem/0001-acp-default-export-drops-inject.md).
|
||||
- **Seam discipline.** New swappable capability? Check it's split per the capability-seams RFC (interface / impl / consumer), and that the consumer injects the interface key, never an implementation type.
|
||||
- **Hardcoded tunables that should be plugin config.** A literal timeout, grace period, output/truncation cap, result-count limit, retry count, buffer size, model name, API base URL, user agent, or filesystem path introduced inside a plugin belongs on the plugin's schemastery `Config` with the shipped value as its default (AGENTS.md § Conventions "No hardcoded tunables in plugins"). A named `DEFAULT_*` constant or a test-only injection seam is not configurability — the question to ask is whether a `cordis.yml` deployment can change the value without a code edit. Protocol/wire constants, semantic constants, values pinned by an external spec, and security invariants are exempt; a new `Config` field also needs its README row and range validation. No gate detects a hardcoded tunable — this check is entirely on the reviewer.
|
||||
- **Test quality — sufficiency, not just coverage.** 100% per-file coverage and a green suite are necessary, not sufficient: they prove the lines *ran*, not that the feature *works the way it ships*. Judge whether the tests are sufficient on two axes. (1) **Would they fail if the behavior regressed?** A test that passes but asserts the wrong thing — or restates the implementation instead of the contract (events fired, disposal reached, the world changed) — is worse than none. (2) **Do they exercise the REAL thing, the way it's actually used?** Prefer the genuine collaborator over a fake, drive the change through its real entry path (the cordis Loader, the ACP bridge, a booted subprocess — not a hand-built `ctx.plugin({...})` that bypasses `unwrapExports`), and verify the WORLD (re-read the file/log/registry externally), not the agent's self-report. A test that fakes the inputs just enough to cover every line will agree with whatever the author assumed; the real thing won't. When a test sets up a *clean/happy* path to reach a line, ask whether the line's PURPOSE is exercised — e.g. a durability/teardown path "tested" by a fully-completed turn never proves the mid-flight teardown it exists for; a torn-tail recovery branch covered by a well-formed log never proves recovery. Flag tests that hit the line but not the scenario. See [docs/testing.md](../../../docs/testing.md) § "Test the real entry path" and § "Prefer the real implementation over a mock".
|
||||
- **Snapshot coverage for transcript/UX changes.** If the PR changes the editor-facing transcript or end-to-end agent UX — the ACP bridge's event→update translation, the agent loop's observable output, tool presentation, or anything an editor renders — it must add or update a snapshot scenario (`examples/*/tests/**/*.snapshot.ts`, goldens under `examples/acp-agent/tests/snapshots/`) or note explicitly why none applies (AGENTS.md § Conventions). Review the golden diff itself: a changed `stdout.golden.txt` / `session.golden.txt` is a behavior change in disguise — confirm it's intended, not an accidental regression someone re-recorded away. A pure internal refactor with no observable-output change is exempt, but the PR should say so. See [docs/rfc/implemented/testing/2026-06-19-acp-snapshot-tests.md](../../../docs/rfc/implemented/testing/2026-06-19-acp-snapshot-tests.md).
|
||||
- **Bilingual docs: review translation quality, not just pairing.** If the PR adds or edits a doc pair, read the changed English and Chinese sides and compare the meaning, not only the mechanical diff. Verify terms against [terminology.md](../../../docs/i18n/terminology.md), including first-occurrence annotations and "do not translate as" prohibitions; if a new term has no established precedent, the PR should keep it in English, list it under `待定术语`, and update the terminology table once the rendering is decided. A green `verify-translation-pairing` only proves hashes, switchers, and structure were recorded — it does not prove the translation is faithful, natural, or correctly termed. Treat [translation-rules.md](../../../docs/i18n/translation-rules.md) MUST/MUST NOT violations as blocking.
|
||||
- **Intent and contracts.** Does the change do what the PR says, and honor the documented contract on *both* sides of every seam it touches (see [docs/defensive-patterns.md](../../../docs/defensive-patterns.md) "Honor cross-seam contracts on BOTH sides")?
|
||||
|
||||
## How to respond
|
||||
|
||||
Technical, specific, non-performative — no "great catch", no "you're absolutely right". State the issue, where it is, and why it matters; cite the AGENTS.md bullet or ADR when one applies, but don't manufacture a citation for a finding that stands on its own reasoning. Separate blocking issues from suggestions so the author knows what gates merge. When replying to inline threads on GitHub, reply in the thread (`gh api repos/{owner}/{repo}/pulls/{n}/comments/{id}/replies`), not as a top-level comment. If a suggestion would fight an ADR or an established convention, say so and link it rather than relitigating in the thread.
|
||||
|
||||
If you are the author *receiving* this review, evaluate each point on its technical merits before acting — verify against the codebase, push back with reasoning where the reviewer lacks context or is wrong, and fix what's correct without performative agreement. A review is a set of claims to evaluate, not orders to follow.
|
||||
State the defect, location, impact, and evidence. Separate blockers from suggestions and omit issues already enforced by a green gate. Use the existing GitHub review thread for replies. When receiving review, verify each claim and fix or rebut it on technical grounds without performative agreement.
|
||||
|
||||
@@ -5,7 +5,7 @@ description: 'Use when writing, moving, reviewing, or auditing documentation in
|
||||
|
||||
# Applying the DeepSeek Harness Documentation Standard
|
||||
|
||||
The contract lives in [docs/AGENTS.md](../../../docs/AGENTS.md) — the tier taxonomy, the word budgets, and the slop checklist. This skill is the workflow for applying it: placing content, auditing the corpus, and handling a red budget gate. It is guidance, not a script; keep judgment active and prefer a few well-proven fixes over a mass rewording pass.
|
||||
The contract lives in [docs/AGENTS.md](../../../docs/AGENTS.md). This workflow covers Markdown, JSDoc, and code comments; use judgment rather than treating length alone as a defect.
|
||||
|
||||
## Sources of truth (read, don't re-summarize)
|
||||
|
||||
@@ -28,13 +28,14 @@ Run the placement test in the standard's taxonomy table, then check the constrai
|
||||
The audit is a hunt for the standard's slop checklist, cheapest probes first:
|
||||
|
||||
1. Measure: `pnpm run verify-doc-budgets --list`, then `git ls-files '*.md' | grep -v '^vendor/' | xargs wc -w | sort -rn | head -30` to spot unbudgeted outliers.
|
||||
2. Hunt narrated history: `rg -n -g '!vendor' -t md "no longer|used to|previously|was moved|renamed"` — judge each hit; some are legitimate (quoting a contrast against a live alternative), most are drift.
|
||||
3. Hunt duplication: take each standing-doc rule, grep one distinctive phrase from it across all Markdown; more than one home means all but one become links.
|
||||
4. Hunt catalog restatement: compare README event/tool tables against the generated catalogs and JSDoc; hand copies get replaced by links.
|
||||
5. Hunt spec-speak in `implemented/` RFCs: migration plans, test checklists, future-tense "should" — an implemented RFC describes what is. The heading-level cases (`## Plan`, `## Acceptance criteria`, …) are mechanically gated by `verify-rfc-format`; the prose-level "should" hunt remains manual.
|
||||
6. Classify each finding: a mechanical trim lands as a small PR; a restructure or removal that changes what a doc promises gets a proposed RFC first (follow [dsh-find-simplifications](../dsh-find-simplifications/SKILL.md) for the RFC shape).
|
||||
2. Hunt narrated history: `rg -n -g '!vendor' "no longer|used to|previously|was moved|renamed" --glob '*.md' --glob '*.ts'` and keep only contrasts against a live alternative.
|
||||
3. Inspect long comments for reasoning transcripts: control-flow narration, test walkthroughs, proof of obvious branches, review findings, and rejected local alternatives. Preserve only a non-obvious contract or durable rationale; otherwise delete the comment.
|
||||
4. Hunt duplication by grepping distinctive phrases. Keep one home and replace other copies with links.
|
||||
5. Replace hand-written catalog or JSDoc restatements with links to generated references.
|
||||
6. In `implemented/` RFCs, remove migration plans, test checklists, and future-tense spec language; keep the decision, rationale, and shipped constraints.
|
||||
7. If removing prose changes a promised behavior rather than its explanation, use a proposed RFC first (follow [dsh-find-simplifications](../dsh-find-simplifications/SKILL.md)).
|
||||
|
||||
Compression discipline: every load-bearing rule survives — as one to three lines plus a link to the home that carries its why. Cut stories, duplicates, and status annotations; never silently drop a rule. If a cut rule has no durable home to link, create it (usually an RFC or postmortem) in the same change.
|
||||
Keep every load-bearing rule, preferably as one to three lines plus a link to its rationale. Cut stories, duplicates, status notes, and the path used to derive the rule. Do not create a new explanation merely to relocate disposable reasoning.
|
||||
|
||||
## When verify-doc-budgets goes red
|
||||
|
||||
@@ -42,4 +43,4 @@ Apply the ordered relocate-condense-raise policy in [docs/AGENTS.md](../../../do
|
||||
|
||||
## Validation and PR hygiene
|
||||
|
||||
For docs-only changes run at least `pnpm run doc-sync`, `pnpm run lint`, and `git diff --check`; if a paired doc was touched, update the counterpart (see [dsh-translate-docs](../dsh-translate-docs/SKILL.md)) and re-record with `pnpm run verify-translation-pairing --write`. Open a draft PR while the audit is still expanding; in the PR body, list what was trimmed/moved with word deltas, what was deliberately kept long and why, and which checks ran. The first audit cycle's deferred work list lives in [the doc-tiers-and-budgets RFC](../../../docs/rfc/implemented/process/2026-07-04-doc-tiers-and-budgets.md) § Deferred work.
|
||||
Run at least `pnpm run doc-sync`, `pnpm run lint`, and `git diff --check`; JSDoc changes may regenerate catalogs. If a paired doc changed, follow [dsh-translate-docs](../dsh-translate-docs/SKILL.md) and run `pnpm run verify-translation-pairing --write`. The PR body should give word deltas, explain any deliberately long exception, and list checks.
|
||||
|
||||
Reference in New Issue
Block a user