From 1ad4615fabe526a7d0f87327941d26107e8839df Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 00:56:06 +0800 Subject: [PATCH 1/3] docs(dsh-code-review): frame skill as guidance, emphasize independent broad reasoning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The skill read like an authoritative checklist; clearing its items could be mistaken for a complete review. Reframe it explicitly as guidance — a where-to-look map that lowers startup cost, not a definition of a sufficient review. Add a "How to think about a review" section drawing on the patterns from the /code-review, /review, requesting-code-review, and receiving-code-review skills: reason from the code independently, sweep all aspects (correctness, concurrency, security, design/approach, tests, docs, …), verify before flagging, calibrate confidence and suppress noise, and lead with severity. Add a receiving-review note so authors evaluate findings on merit rather than following them blindly. --- .agents/skills/dsh-code-review/SKILL.md | 26 +++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/.agents/skills/dsh-code-review/SKILL.md b/.agents/skills/dsh-code-review/SKILL.md index a8f740c305..0191f30e15 100644 --- a/.agents/skills/dsh-code-review/SKILL.md +++ b/.agents/skills/dsh-code-review/SKILL.md @@ -5,24 +5,38 @@ description: Use when reviewing a pull request in the deepseek-harness repo — # Reviewing a DeepSeek-Harness PR -This is a where-to-look map, not a rules list. The rules live in the docs below and are the source of truth — read them there so this skill never drifts out of sync with them. +**This skill is guidance, not authority.** It is a where-to-look map that lowers your startup cost on an unfamiliar PR — it is not a checklist that defines a complete review, and clearing every item here does not mean the PR is good. You are the reviewer. Reason independently from the code in front of you, think broadly across every dimension a change can fail on, and trust your own judgment over this document when they disagree. 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. + +## 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 the few categories listed under "Where to look first" — 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) -- **[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, the empty-`catch` rule, symmetry. Every PR is checked against these. -- **AGENTS.md § Defensive patterns (hard-won)** — each bullet is a bug class that bit us. Reviewing anything touching process lifecycle, async/await, disposal, or adapter error paths? Re-read this first. +These define many of the conventions this repo is checked against. Read them at the source so this skill never drifts out of sync — but treat them as inputs to your judgment, not a substitute for it. + +- **[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, the empty-`catch` rule, symmetry. +- **AGENTS.md § Defensive patterns (hard-won)** — each bullet 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** — the doc-sync rule (code change ⇒ update README + JSDoc in the SAME commit) and the no-hard-wrap markdown convention. - **[packages/AGENTS.md](../../../packages/AGENTS.md)** — per-package conventions (file layout, the HMR-safety test requirement). -- **[ADR index](../../../docs/adr/README.md)** — the *why* behind the architecture. Especially [0007 quality gates](../../../docs/adr/0007-quality-gates.md) (what a PR must pass) and [0009 capability seams](../../../docs/adr/0009-capability-seams.md) (the three-package split). If a change seems to fight an ADR, that's a discussion, not a silent override. +- **[ADR index](../../../docs/adr/README.md)** — the *why* behind the architecture. Especially [0007 quality gates](../../../docs/adr/0007-quality-gates.md) (what a PR must pass) and [0009 capability seams](../../../docs/adr/0009-capability-seams.md) (the three-package split). If a change seems to fight an ADR, that's a discussion, not a silent override — and not an automatic veto either: an ADR can be wrong for this case, so reason about it. ## Where to look first (review-specific, not in the docs) +A starting set of checks the docs don't spell out — not the whole job. After these, keep going on the broader aspects above. + 1. **Docs in sync?** If the PR changes a config key, default, error code, wire field, or event name, did it update the package README + module/JSDoc in the same diff? Stale docs are the most common miss — `pnpm run doc-sync` only gates compilable `ts` blocks and the event-taxonomy table, so prose drift (config keys, defaults, error codes, wire fields) has no gate and is on the reviewer to catch. 2. **HMR-safety test present?** Any new registry/registration needs a test that disposes the contributing fiber and asserts cleanup. Its absence is a blocking gap. -3. **Gates green?** typecheck, lint, test, test:coverage (100% per-file on `packages/*/src`), knip, build, publint, constraints. Don't re-review what a gate already enforces — trust the gate, spend attention on what gates can't check (intent, contracts, doc sync). +3. **Gates green — and trusted?** typecheck, lint, test, test:coverage (100% per-file on `packages/*/src`), knip, build, publint, constraints. Don't re-review what a gate already enforces; spend your attention on what gates can't check — intent, contracts, design, doc sync, test *quality* (a test that passes but asserts the wrong thing is worse than none). 4. **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 (see AGENTS.md e2e bullet). 5. **Seam discipline.** New swappable capability? Check it's split per ADR 0009 (interface / impl / consumer), and that the consumer injects the interface key, never an implementation type. ## How to respond -Technical, specific, non-performative — no "great catch", no "you're absolutely right". State the issue and where; cite the AGENTS.md bullet or ADR it relates to. 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. +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. From 5f6ccf91ecf659c3acaf91c11da6ebe64dad49e0 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 01:09:58 +0800 Subject: [PATCH 2/3] docs(dsh-code-review): scope judgment so documented blockers stay authoritative Codex adversarial review flagged that "guidance not authority" over-rotated: it could be read as licensing a reviewer to waive genuinely-blocking checks (HMR-safety test, doc-sync, ADR 0007 gates), and the "Where to look first" section claimed to list things "not in the docs" while its first two items ARE documented requirements. Fix both: scope independent judgment to *what to look at* and *how to apply a rule*, not whether documented requirements hold; state AGENTS.md / packages/AGENTS.md / ADR 0007 stay authoritative. Split the checks into "Hard blockers (documented requirements)" and "Reviewer-only checks (judgment required)", so doc-sync/HMR-safety/gates aren't mixed with reviewer-only heuristics. Keep the independent/broad-reasoning intent. --- .agents/skills/dsh-code-review/SKILL.md | 29 ++++++++++++++++--------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/.agents/skills/dsh-code-review/SKILL.md b/.agents/skills/dsh-code-review/SKILL.md index 0191f30e15..3d33d7bfac 100644 --- a/.agents/skills/dsh-code-review/SKILL.md +++ b/.agents/skills/dsh-code-review/SKILL.md @@ -5,19 +5,21 @@ description: Use when reviewing a pull request in the deepseek-harness repo — # Reviewing a DeepSeek-Harness PR -**This skill is guidance, not authority.** It is a where-to-look map that lowers your startup cost on an unfamiliar PR — it is not a checklist that defines a complete review, and clearing every item here does not mean the PR is good. You are the reviewer. Reason independently from the code in front of you, think broadly across every dimension a change can fail on, and trust your own judgment over this document when they disagree. 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. +**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 [ADR 0007 quality gates](../../../docs/adr/0007-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 ADR (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 the few categories listed under "Where to look first" — sweep all of them. +- **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 many of the conventions this repo is checked against. Read them at the source so this skill never drifts out of sync — but treat them as inputs to your judgment, not a substitute for it. +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](../../../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, the empty-`catch` rule, symmetry. - **AGENTS.md § Defensive patterns (hard-won)** — each bullet 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. @@ -25,15 +27,22 @@ These define many of the conventions this repo is checked against. Read them at - **[packages/AGENTS.md](../../../packages/AGENTS.md)** — per-package conventions (file layout, the HMR-safety test requirement). - **[ADR index](../../../docs/adr/README.md)** — the *why* behind the architecture. Especially [0007 quality gates](../../../docs/adr/0007-quality-gates.md) (what a PR must pass) and [0009 capability seams](../../../docs/adr/0009-capability-seams.md) (the three-package split). If a change seems to fight an ADR, that's a discussion, not a silent override — and not an automatic veto either: an ADR can be wrong for this case, so reason about it. -## Where to look first (review-specific, not in the docs) +## Hard blockers (documented requirements — missing one blocks merge) -A starting set of checks the docs don't spell out — not the whole job. After these, keep going on the broader aspects above. +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, did it update the package README + module/JSDoc in the same diff? Stale docs are the most common miss — `pnpm run doc-sync` only gates compilable `ts` blocks and the event-taxonomy table, so prose drift (config keys, defaults, error codes, wire fields) has no gate and is on the reviewer to catch. -2. **HMR-safety test present?** Any new registry/registration needs a test that disposes the contributing fiber and asserts cleanup. Its absence is a blocking gap. -3. **Gates green — and trusted?** typecheck, lint, test, test:coverage (100% per-file on `packages/*/src`), knip, build, publint, constraints. Don't re-review what a gate already enforces; spend your attention on what gates can't check — intent, contracts, design, doc sync, test *quality* (a test that passes but asserts the wrong thing is worse than none). -4. **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 (see AGENTS.md e2e bullet). -5. **Seam discipline.** New swappable capability? Check it's split per ADR 0009 (interface / impl / consumer), and that the consumer injects the interface key, never an implementation type. +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. `pnpm run doc-sync` only gates compilable `ts` blocks and the event-taxonomy table, so prose drift (config keys, defaults, error codes, wire fields) has no automated gate and is on the reviewer to catch — but it is still required, not optional. +2. **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. +3. **Quality gates pass.** typecheck, lint, test, test:coverage (100% per-file on `packages/*/src`), knip, build, publint, constraints (ADR 0007). Don't re-review what a gate already enforces — trust the gate and spend attention on what it can't check. + +## 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 (see AGENTS.md e2e bullet). +- **Seam discipline.** New swappable capability? Check it's split per ADR 0009 (interface / impl / consumer), and that the consumer injects the interface key, never an implementation type. +- **Test quality.** A test that passes but asserts the wrong thing is worse than none. Check that new tests would actually fail if the behavior regressed, and that they exercise the contract (events fired, disposal reached) rather than restating the implementation. +- **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 AGENTS.md "Honor cross-seam contracts on BOTH sides")? ## How to respond From c58bbf22eb55d0ce2440c02ec15473a1ecbc8e41 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Thu, 18 Jun 2026 01:14:10 +0800 Subject: [PATCH 3/3] docs(dsh-code-review): add doc-sync + module-graph to hard-blocker gate list Codex round 2 caught that the quality-gates hard blocker omitted the documented doc-sync gate (and module-graph freshness), so a docs-specific gate failure could be read as outside the blocking checklist. Add both, and note doc-sync's coverage so the manual prose-drift check (#1) reads as additional to it, not a duplicate. --- .agents/skills/dsh-code-review/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.agents/skills/dsh-code-review/SKILL.md b/.agents/skills/dsh-code-review/SKILL.md index 3d33d7bfac..fcd6443cce 100644 --- a/.agents/skills/dsh-code-review/SKILL.md +++ b/.agents/skills/dsh-code-review/SKILL.md @@ -31,9 +31,9 @@ These define the conventions and gates this repo is checked against, and they ar 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. `pnpm run doc-sync` only gates compilable `ts` blocks and the event-taxonomy table, so prose drift (config keys, defaults, error codes, wire fields) has no automated gate and is on the reviewer to catch — but it is still required, not optional. +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 #3) 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. **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. -3. **Quality gates pass.** typecheck, lint, test, test:coverage (100% per-file on `packages/*/src`), knip, build, publint, constraints (ADR 0007). Don't re-review what a gate already enforces — trust the gate and spend attention on what it can't check. +3. **Quality gates pass.** typecheck, lint, test, test:coverage (100% per-file on `packages/*/src`), knip, build, publint, constraints, `doc-sync` (doc-typecheck + verify-event-taxonomy + verify-md-wrap), module-graph freshness (ADR 0007). Don't re-review what a gate already enforces — trust the gate and spend attention on what it can't check. Note that the `doc-sync` gate only covers compilable `ts` blocks, the event-taxonomy table, and markdown wrapping; prose drift (check #1) is *additional* manual review on top of it, not covered by it. ## Reviewer-only checks (gates can't catch these — judgment required)