deepseek-harness/.agents/skills/dsh-code-review/SKILL.md

name, description

name	description
dsh-code-review	Use when reviewing a pull request in the deepseek-harness repo — orients the reviewer to this codebase's standards (AGENTS.md conventions, defensive patterns, ADRs, quality gates) and the review-specific checks that code alone can't show

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.

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 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.

How to think about a review

• 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.

Sources of truth (read, don't re-summarize)

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.

• 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 — 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 — 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 — per-package conventions (file layout, the HMR-safety test requirement).
• docs/i18n/translation-rules.md and 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 is the translator workflow.
• RFC index — the why behind the architecture. Especially quality gates (what a PR must pass) and capability seams (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.

Hard blockers (documented requirements — missing one blocks merge)

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 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". 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); prose drift (checks #1 and #2) and translation quality (the dsh-translate-docs 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).
• 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 and docs/postmortem/0001.
• 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 § "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.
• 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, 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 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 "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.