From d5350cd42e9c3c352567accefff480d2453c590d Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 19 Jul 2026 17:08:00 +0800 Subject: [PATCH 1/2] docs(review): distill human review principles --- .agents/skills/dsh-code-review/SKILL.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/.agents/skills/dsh-code-review/SKILL.md b/.agents/skills/dsh-code-review/SKILL.md index 2b562cc321..ca42b97323 100644 --- a/.agents/skills/dsh-code-review/SKILL.md +++ b/.agents/skills/dsh-code-review/SKILL.md @@ -29,19 +29,20 @@ description: Use when reviewing a pull request in the deepseek-harness repo — - **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, ownership before reentry, complete detach cleanup, and quiescent disposal. -- **Capability shape:** a swappable capability follows the interface / implementation / consumer split. Consumers depend on the interface, not a backend. -- **Scope, ownership, and necessity:** tie each abstraction, state machine, option, defensive copy, and compatibility path to a current contract or production consumer. Challenge unrelated features, speculative generality, and behavior placed outside its owning plugin or service. -- **Configuration:** deployment-varying timeouts, caps, models, URLs, paths, and retry counts are validated `Config` fields, not literals or `DEFAULT_*` constants. +- **Capability and consumer fit:** a swappable capability follows the interface / implementation / consumer split. Shape the interface for all current consumers, not one tool schema, loader, UI, transport, or backend; keep consumer-specific behavior in the consumer or adapter. +- **Scope, ownership, and necessity:** tie each abstraction, state machine, option, defensive copy, and compatibility path to a current contract or production consumer. Challenge unrelated features, speculative generality, and behavior placed outside its owning plugin or service. Review whether the PR is one coherent change; move an independently meaningful feature or design decision into a separate or stacked PR when combining it obscures ownership, intent, or verification. +- **Configuration and public choices:** deployment-varying tunables are validated `Config` fields. Do not invent a default, public operation set, format, or borrowed external concept merely because it can be configurable; require evidence from a current consumer or relevant prior art, otherwise keep the choice explicit or defer it. +- **Model perspective:** inspect the exact prompts, tool schemas, results, and diagnostics the model receives across affected modes. Flag UI, transport, or implementation concepts irrelevant to the model's task. Verify stable model-visible text verbatim and dynamic behavior through snapshots or end-to-end coverage. - **Enforcement boundaries:** hidden schema fields, filtered prompts, facades, wrappers, and listener ordering are not authoritative enforcement when direct or alternate callers can bypass them. Exercise denial paths at the boundary that actually executes the operation. - **Borrowed and derived state:** determine whether retained caller-owned values are borrowed or snapshotted by contract; do not demand copies at typed same-process seams. Materialize mutable values that cross queues, model/tool JSON, durable logs or files, workers, processes, or wire boundaries. Commit notifications and derived state only at the documented success boundary, and trace caches, prompts, UI echoes, replay, and query views to one authoritative source. - **Bounds cover the final operation:** verify byte, token, item, and time limits at the boundary that owns the complete emitted or retained result, including wrappers and metadata. Probe tiny limits, exact thresholds, oversized single chunks, and multibyte text for byte limits. - **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. -- **Changed checks have a negative control:** a new automated check, or a changed acceptance path in one, has a deliberately invalid case that reaches the real top-level runner and fails for the intended rule; a green happy path does not prove the check is wired. +- **Mechanized invariants and negative controls:** when a repeatable invariant is mechanically checkable, require it to be wired into an executed top-level gate. A new check, or a changed acceptance path in one, also needs a deliberately invalid case that reaches the real runner and fails for the intended rule. - **Implemented RFCs match shipped reality:** when a PR implements a proposed RFC, move and rewrite it as present-tense shipped state in the same diff, then verify paths, names, and mechanisms against the implementation. - **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. ## Reporting findings -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. +State the defect, location, impact, and evidence. Place a localized defect inline on the tightest relevant diff range; use a PR-level comment for cross-cutting architecture, scope, or review-wide synthesis. 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. From 4ab5a76317f6763e682f7525d913e08fb205fdd4 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Sun, 19 Jul 2026 17:35:43 +0800 Subject: [PATCH 2/2] docs(agents): promote review principles --- .agents/skills/dsh-code-review/SKILL.md | 20 ++++++++++---------- AGENTS.md | 6 +++--- docs/AGENTS.md | 2 +- packages/AGENTS.md | 8 ++++++++ scripts/doc-budgets.manifest.json | 6 +++--- 5 files changed, 25 insertions(+), 17 deletions(-) diff --git a/.agents/skills/dsh-code-review/SKILL.md b/.agents/skills/dsh-code-review/SKILL.md index ca42b97323..cabc6211ff 100644 --- a/.agents/skills/dsh-code-review/SKILL.md +++ b/.agents/skills/dsh-code-review/SKILL.md @@ -9,7 +9,7 @@ description: Use when reviewing a pull request in the deepseek-harness repo — ## Sources of truth -- [AGENTS.md](../../../AGENTS.md) and [packages/AGENTS.md](../../../packages/AGENTS.md): repository and package rules. +- [AGENTS.md](../../../AGENTS.md) and [packages/AGENTS.md](../../../packages/AGENTS.md): standing repository and package authoring contracts. - [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. - [dsh-prose-standard](../dsh-prose-standard/SKILL.md): required coverage and editorial judgment for comments, docs, prompts, and visible strings. @@ -22,23 +22,23 @@ description: Use when reviewing a pull request in the deepseek-harness repo — 1. **New prose receives semantic review.** Use [dsh-prose-standard](../dsh-prose-standard/SKILL.md) to critically review every added or changed Markdown passage, JSDoc, comment, prompt, description, diagnostic, and visible string. Verify required coverage, accuracy, placement, and editorial quality against the owning code or behavior; automated checks do not establish those properties. 2. **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. 3. **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. -4. **Registrations clean up.** A new registry contribution has a test that disposes its owner and observes removal. +4. **Registrations clean up.** Verify each new registry contribution satisfies the disposal-test contract in [packages/AGENTS.md](../../../packages/AGENTS.md). 5. **Required gates pass.** Trust the [current readiness sequence](../../../AGENTS.md#run-the-ci-gates-locally-before-marking-a-pr-ready) and `pnpm run check:pre-push` for their enforced inventory; review the semantic gaps they cannot detect. ## Manual checks - **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, ownership before reentry, complete detach cleanup, and quiescent disposal. -- **Capability and consumer fit:** a swappable capability follows the interface / implementation / consumer split. Shape the interface for all current consumers, not one tool schema, loader, UI, transport, or backend; keep consumer-specific behavior in the consumer or adapter. -- **Scope, ownership, and necessity:** tie each abstraction, state machine, option, defensive copy, and compatibility path to a current contract or production consumer. Challenge unrelated features, speculative generality, and behavior placed outside its owning plugin or service. Review whether the PR is one coherent change; move an independently meaningful feature or design decision into a separate or stacked PR when combining it obscures ownership, intent, or verification. -- **Configuration and public choices:** deployment-varying tunables are validated `Config` fields. Do not invent a default, public operation set, format, or borrowed external concept merely because it can be configurable; require evidence from a current consumer or relevant prior art, otherwise keep the choice explicit or defer it. -- **Model perspective:** inspect the exact prompts, tool schemas, results, and diagnostics the model receives across affected modes. Flag UI, transport, or implementation concepts irrelevant to the model's task. Verify stable model-visible text verbatim and dynamic behavior through snapshots or end-to-end coverage. -- **Enforcement boundaries:** hidden schema fields, filtered prompts, facades, wrappers, and listener ordering are not authoritative enforcement when direct or alternate callers can bypass them. Exercise denial paths at the boundary that actually executes the operation. -- **Borrowed and derived state:** determine whether retained caller-owned values are borrowed or snapshotted by contract; do not demand copies at typed same-process seams. Materialize mutable values that cross queues, model/tool JSON, durable logs or files, workers, processes, or wire boundaries. Commit notifications and derived state only at the documented success boundary, and trace caches, prompts, UI echoes, replay, and query views to one authoritative source. -- **Bounds cover the final operation:** verify byte, token, item, and time limits at the boundary that owns the complete emitted or retained result, including wrappers and metadata. Probe tiny limits, exact thresholds, oversized single chunks, and multibyte text for byte limits. +- **Capability and consumer fit:** trace every current consumer, then flag consumer-specific behavior leaking into the interface under [the package contract](../../../packages/AGENTS.md). +- **Scope, ownership, and necessity:** map each abstraction, state machine, option, defensive copy, and compatibility path to its current contract, production consumer, and owning plugin or service. Challenge unrelated features and speculative generality, then test the PR's coherence against [the root contract](../../../AGENTS.md#conventions). +- **Configuration and public choices:** ask what current-consumer evidence or prior art supports each default, public operation set, format, or imported external concept. Require an explicit choice or deferral when that evidence is absent. +- **Model perspective:** inspect the exact prompts, tool schemas, results, and diagnostics the model receives across affected modes. Flag concepts outside the model's task, then verify stable text verbatim and dynamic behavior through snapshots or end-to-end coverage. +- **Enforcement boundaries:** follow every denial path to the operation that executes it; exercise direct and alternate callers that can bypass schemas, prompts, facades, wrappers, or listener ordering. +- **Borrowed and derived state:** classify each retained value under the package boundary contract, then trace notifications and every cache, prompt, UI echo, replay, and query view to the documented success point and authoritative source. +- **Bounds cover the final operation:** locate the owner of the complete emitted or retained result, including wrappers and metadata. Probe tiny and exact limits, oversized single chunks, and multibyte text for byte limits. - **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. -- **Mechanized invariants and negative controls:** when a repeatable invariant is mechanically checkable, require it to be wired into an executed top-level gate. A new check, or a changed acceptance path in one, also needs a deliberately invalid case that reaches the real runner and fails for the intended rule. +- **Mechanized invariants and negative controls:** trace each new or changed check through the executed top-level gate and its deliberately invalid case; confirm the real runner fails for the intended rule. - **Implemented RFCs match shipped reality:** when a PR implements a proposed RFC, move and rewrite it as present-tense shipped state in the same diff, then verify paths, names, and mechanisms against the implementation. - **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. diff --git a/AGENTS.md b/AGENTS.md index efaa29bbac..27b5a655e2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -110,7 +110,7 @@ Real-API tests and demos read `DEEPSEEK_API_KEY`, optional `DEEPSEEK_BASE_URL`, - **Plugins, not loop changes**: new behavior goes on the documented extension seams; changing `agent-loop` requires updating docs/architecture.md. - **Capability seams are three packages** — interface / implementation / consumer; don't split preemptively. - **Explicit > implicit at package seams**: defaulting is an explicit `resolve(request): Spec` step in the owning implementation, never a hidden `?? default` inside `run()` (the `dsh-bash` request/spec split is the template). -- **No hardcoded tunables in plugins**: deployment choices are defaulted, validated `Config` fields changeable from cordis.yml; a `DEFAULT_*` constant or test seam is not configurability. Protocol constants, external specs, and security invariants stay fixed. +- **No hardcoded tunables in plugins**: deployment-varying choices are validated `Config` fields changeable from cordis.yml; a `DEFAULT_*` constant or test seam is not configurability. Protocol constants, external specs, and security invariants stay fixed. - **Misconfiguration fails loud** at load when self-contained, otherwise at the earliest resolvable point; never silently skip a missing referent. - **Opaque cross-boundary ids are branded** (`Branded` from `dsh-brand`), never bare `string`. - **An empty `catch` names what it swallows** and why nothing else can reach it; keep the `try` to one statement. @@ -120,7 +120,7 @@ Real-API tests and demos read `DEEPSEEK_API_KEY`, optional `DEEPSEEK_BASE_URL`, - **Testing policy** — [docs/testing.md](docs/testing.md). Transcript changes need snapshots or a PR note. Fixtures must replay on macOS/Linux; fix fixtures, not normalizers. - **A tool's ACP render intent is part of its design**, decided up front (`generic`/`terminal`/`diff`, `locations`); presentation methods are pure functions of `args` ([cookbook](docs/cookbook/adding-a-tool.md)). - **Plan unit, e2e, and snapshot coverage** for new seams, lifecycle shapes, and transcript surfaces, and schedule any missing harness support before implementation. -- **Merge PRs with merge commits**, never squash/rebase or rewrite pushed branches. Put a review fix on its introducing PR, then merge down the stack ([guide](docs/cookbook/responding-to-pr-review-on-a-stack.md)). +- **Keep PRs coherent and merge with merge commits.** Split an independently meaningful feature or design decision into a separate or stacked PR when combining it obscures ownership, intent, or verification. Never squash/rebase or rewrite pushed branches; put a review fix on its introducing PR, then merge down the stack ([guide](docs/cookbook/responding-to-pr-review-on-a-stack.md)). - TODO markers: `FIXME`/`TODO`/`XXX` by urgency ([semantics](docs/development.md)). - Files end with exactly one trailing newline; `git diff --check` (pre-push) gates it. @@ -132,7 +132,7 @@ Read [docs/defensive-patterns.md](docs/defensive-patterns.md) before lifecycle, Everything compiles under `strict: true` with `noImplicitAny`; every remaining `any` explains why a narrower type is infeasible. Every module and export has concise JSDoc for its non-obvious contract; function-like exports include `@param`/`@returns`, as enforced by `verify-export-jsdoc`. Heritage-declared members, plugin-protocol slots, and constructors keep their docs at the declaring seam, protocol, or class. -Comments and docs preserve complete contracts and non-obvious orientation, not reasoning transcripts. Do not narrate control flow or tests, preserve review history, or restate code. Keep factual clauses affecting behavior, failure, timing, ownership, or safe use; link aggressively to owning rationale. Use [dsh-prose-standard](.agents/skills/dsh-prose-standard/SKILL.md) for prose decisions. Encode enforceable invariants in checks, using narrow justified exceptions rather than disabling a rule globally. +Comments and docs preserve complete contracts and non-obvious orientation, not reasoning transcripts. Do not narrate control flow or tests, preserve review history, or restate code. Keep factual clauses affecting behavior, failure, timing, ownership, or safe use; link aggressively to owning rationale. Use [dsh-prose-standard](.agents/skills/dsh-prose-standard/SKILL.md) for prose decisions. Wire mechanically checkable invariants into an executed top-level gate and prove each new or changed acceptance path rejects an invalid case. Use narrow justified exceptions instead of disabling a rule globally. Docs are part of every change: code changes update their README and JSDoc in the SAME change; a bilingual-pair edit updates the counterpart and re-records ([i18n contract](docs/i18n/README.md)). The writing rules — document the current state never the history, one physical line per paragraph, one home per fact — and the word-budget gate live in [docs/AGENTS.md](docs/AGENTS.md). diff --git a/docs/AGENTS.md b/docs/AGENTS.md index f7635c659b..a0c30c5545 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -43,7 +43,7 @@ When the gate goes red: 2. **Condense** content that belongs here but can be shorter. 3. **Raise** the ceiling only when the words truly need the space; justify the manifest diff in the PR. A too-low ceiling is a budget bug. -Ceilings are guardrails, not reduction targets. Retain at least 5% headroom; lower a ceiling only when the document's durable contract still has room, and raise it when necessary content would otherwise be deleted. Targets: root `AGENTS.md` ≤ 1,500 words; `architecture.md` ≤ 1,800; each subtree `AGENTS.md` ≤ 600, except this file ≤ 1,250; `packages/README.md` ≤ 600. Review and the slop checklist govern unbudgeted tiers. +Ceilings are guardrails, not reduction targets. Retain at least 5% headroom; lower a ceiling only when the document's durable contract still has room, and raise it when necessary content would otherwise be deleted. Targets: root `AGENTS.md` ≤ 1,600 words; `architecture.md` ≤ 1,800; each subtree `AGENTS.md` ≤ 600, except `packages/AGENTS.md` ≤ 650 and this file ≤ 1,250; `packages/README.md` ≤ 600. Review and the slop checklist govern unbudgeted tiers. ## The slop checklist diff --git a/packages/AGENTS.md b/packages/AGENTS.md index 9ab0327cf9..e4b5ded6a3 100644 --- a/packages/AGENTS.md +++ b/packages/AGENTS.md @@ -8,6 +8,14 @@ These package-specific rules supplement the repo-wide [conventions](../AGENTS.md - **Typed same-process service and plugin calls are contracts, not serialization boundaries.** Prefer readonly borrowed values; materialize or defensively validate only at parser/config, queued, model/tool JSON, durable/file, worker, process, or wire boundaries. - **Initiator-owned private chains derive, then capture.** Under `ctx.agents.withInitiator()`, recover the Agent at each orchestration entry, derive `agent.session`, and let operation-local helpers close over it. Keep `Agent` and `Session` explicit at lifecycle, session-log, service, authority, worker/process, persistence, and wire interfaces; do not widen a leaf helper from `Session` to `Context` merely to hide a parameter ([rationale](../docs/rfc/implemented/architecture/2026-07-15-agent-initiator-scope.md)). - **Represent one asynchronous operation with one lifecycle controller or transaction.** Separate readiness, cancellation, disposal, reservation, or sentinel state requires an independent owner or settlement boundary; otherwise fold it while preserving rollback, callback containment, and quiescence. +- **Shape capability interfaces around all current consumers.** Keep tool-schema, Loader, UI, transport, and backend-specific behavior in the consumer or adapter; do not let one consumer dictate the interface ([capability-seam rationale](../docs/rfc/implemented/architecture/2026-06-13-capability-seams.md)). +- **Require a current owner and need.** Tie each abstraction, state machine, option, defensive copy, and compatibility path to a current contract or production consumer, and keep behavior in its owning plugin or service. +- **Require evidence for public choices.** Configurability does not justify an unsupported default, public operation set, format, or imported external concept. Use current-consumer evidence or relevant prior art; otherwise require an explicit value or defer the choice. +- **Write model-facing contracts from the model's perspective.** Prompts, tool schemas, results, and diagnostics contain only task-relevant concepts, not UI, transport, or implementation vocabulary. Pin stable model-visible text verbatim and dynamic behavior through snapshots or end-to-end coverage. +- **Enforce at the operation boundary that owns the decision.** Schema omission, prompt filtering, facades, wrappers, and listener order are not enforcement when direct or alternate callers can bypass them; test denial through the executor. +- **Publish state only at its commit point.** Emit each notification and update derived state only after the success boundary that makes it true; derive caches, prompts, UI echoes, replay, and query views from one authoritative source. +- **Apply bounds to the complete result.** Enforce byte, token, item, and time limits where the complete emitted or retained value, including wrappers and metadata, is known; test tiny and exact limits, oversized single chunks, and multibyte byte limits. +- **Registry contributions prove disposal.** Add the HMR-safety test required by the [testing policy](../docs/testing.md): dispose the contributing fiber and observe removal. Naming notes: diff --git a/scripts/doc-budgets.manifest.json b/scripts/doc-budgets.manifest.json index b4d2bca159..d849248189 100644 --- a/scripts/doc-budgets.manifest.json +++ b/scripts/doc-budgets.manifest.json @@ -1,11 +1,11 @@ { - "AGENTS.md": 1500, - "docs/AGENTS.md": 1100, + "AGENTS.md": 1600, + "docs/AGENTS.md": 1150, "docs/architecture.md": 1790, "docs/cordis-primer.md": 600, "docs/defensive-patterns.md": 550, "docs/testing.md": 960, "examples/AGENTS.md": 310, - "packages/AGENTS.md": 370, + "packages/AGENTS.md": 650, "packages/README.md": 760 }