Merge pull request #6456 from deepseek-harness/worktree/review-local-simplification

docs(review): test necessity before choosing feature ownership
This commit is contained in:
Tianyi Cui authored and GitHub committed 2026-10-18 14:14:42 +08:00
commit dfa1482c75
5 files changed
+14 -10

No files matched your search

+3 -3
View File
@@ -41,9 +41,9 @@ description: Use when reviewing a pull request in the deepseek-harness repo —
- **Intent and interface contracts:** trace both sides of every changed interface. Confirm the implementation matches the PR and any Agent Note, including errors, cancellation, ownership, and disposal.
- **Error handling:** apply [dsh-error-handling](../dsh-error-handling/SKILL.md) to changed failure paths, including their state guarantees and final user-visible reports. Reject catch-and-continue unless the state owner establishes that subsequent work's preconditions hold, including after recovery.
- **Lifecycle and concurrency:** for async setup, callbacks, processes, or teardown, apply [defensive-patterns/general.md](../../../docs/defensive-patterns/general.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:** trace every current consumer, then flag consumer-specific behavior leaking into the interface under [the package rules](../../../packages/AGENTS.md). Flag the inverse too: a new public method on a generic service (registry, session, agent) whose only caller is one internal consumer is an unnecessary API expansion — require a private capability closure handed to that consumer at construction instead.
- **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 against [the root rules](../../../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.
- **Scope, ownership, and necessity:** first identify the required behavior behind each new distinction, field, adapter, state machine, option, defensive copy, and compatibility path. A requirement, check, or Agent Note introduced by the PR is a design choice to assess, not independent evidence of necessity. Compare existing source fields, discriminants, and their consumers before accepting another representation; distinguish deleting a concept from relocating its declaration. Moving an unnecessary concept closer to its owner is not a simplification. Challenge unrelated features and speculative generality against [the root rules](../../../AGENTS.md#conventions).
- **Capability and consumer fit:** trace every current consumer, then flag consumer-specific behavior leaking into the interface under [the package rules](../../../packages/AGENTS.md). For required behavior, prefer an existing extension point near its producer or consumer when the local implementation adds little complexity, following [the ownership rule](../../../docs/defensive-patterns/general.md#a-business-capability-owns-itself); compare producer-owned declaration merging with changes to distant core or shared type declarations. Flag the inverse too: a new public method on a generic service (registry, session, agent) whose only caller is one internal consumer is an unnecessary API expansion — require a private capability closure handed to that consumer at construction instead.
- **Configuration and public choices:** trace authoritative reads and precedence before accepting another CLI or configuration pass-through; a forwarded value may be shadowed before use. 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:** 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:** determine whether each retained value is borrowed or owned under the package contract, then trace notifications and every cache, prompt, UI echo, replay, and query view to the documented success point and authoritative source.
@@ -17,14 +17,14 @@ For broad requests, divide independent domains among subagents: loop and persist
## Search for removable obligations
Use these questions to guide discovery. Read the relevant section of [historical patterns](references/historical-patterns.md) when a candidate needs calibration; the examples illustrate decisions, not a current deletion inventory.
Use these questions to guide discovery. Requirements, checks, and Agent Notes introduced by the code under review are design choices to assess, not independent evidence that its distinctions are necessary. Read the relevant section of [historical patterns](references/historical-patterns.md) when a candidate needs calibration; the examples illustrate decisions, not a current deletion inventory.
- **Does a declared feature have a complete effect path?** Trace producer, transformations, provider or endpoint support, and observable result. A field copied everywhere may still have no setter, no reader, or only implementations that reject it. Search constructors and discriminant emitters as well as callers.
- **Which distinctions change a consumer's action?** Several internal states may require one model-facing status or one usable identifier. Keep internal distinctions that control authorization, residency, durability, or ownership; remove public distinctions that only invite an unusable choice.
- **Does a declared feature have a complete effect path?** Trace producer, transformations, provider or endpoint support, and observable result. A field copied everywhere may still have no setter, no reader, or only implementations that reject it. For CLI and configuration pass-throughs, follow authoritative reads and precedence to find forwarded values that are shadowed before use. Search constructors and discriminant emitters as well as callers.
- **Which distinctions change a consumer's action?** First ask whether the distinction, field, or adapter is needed at all. Trace existing source fields and discriminants through their emitters and authorization or capability checks before adding a transport-specific marker; a broad envelope category need not have the source field's meaning. Consider deleting a distinction separately from relocating its declaration, and state which inputs or capabilities each alternative admits. Several internal states may require one model-facing status or one usable identifier. Preserve required authorization, residency, durability, and ownership behavior; remove public distinctions that only invite an unusable choice.
- **Could a smaller explicit behavior remove the subsystem?** Fixed intervals, caller-supplied time zones, or reported activation failure can eliminate generalized calendars, persisted defaults, or automatic rollback. Name the capability given up and the machinery deleted. A production caller makes this a behavior decision, not an automatic rejection; implementation must remain within the user's authorization.
- **Can a consumer read the authoritative value when needed?** Look for copied histories, promotion ledgers, invalidation events followed by reads, and feature caches beside a shared projection stream. Derivation or reuse may delete subscriptions, retained data, stale-response handling, and recovery paths together. Establish the required time of observation first.
- **Is composition being mistaken for policy?** Optional service presence, a provider method, or a sibling tool can express availability without deciding deployment behavior. Explicit configuration or a caller-intent operation may remove registries and dependency checks. Preserve distinct operations when their ownership or completion timing differs.
- **What owns the complete maintenance cost?** Trace pass-through configuration, duplicate application trees, demo launchers used by products, resolver-only workspaces, and installation helpers with their own upgrade lifecycle. Compare the entire removed system with the replacement, including residual glue; moving complexity or adding a gate to keep duplicate definitions equal does not remove it.
- **What owns the complete maintenance cost?** Trace duplicate application trees, demo launchers used by products, resolver-only workspaces, and installation helpers with their own upgrade lifecycle. For required behavior, compare existing producer/consumer extension points, including declaration merging, with changes to distant core or shared type declarations under [the ownership rule](../../../docs/defensive-patterns/general.md#a-business-capability-owns-itself). Compare the entire removed system with the replacement, including residual glue; moving an unnecessary concept closer to its owner or adding a gate to keep duplicate definitions equal does not remove its maintenance cost.
## Prove reachability and the trade-off
@@ -32,7 +32,7 @@ Start with `rg`, then read the matches. Search exact symbols, property reads and
Include `packages/*/*/src`, `apps/`, `python/`, runtime scripts, shipped profiles and overlays, package manifests and exports, loader resolution, generated runtime assets, and installed-package consumers. Classify examples and fixtures by their actual entry point rather than directory name. Follow generated catalogs to their consumers: a catalog used by model-written plugin mounts is a dynamic product API, while a documentation-only listing is not a fixed caller. Public third-party extension paths can be intentional despite zero repository call sites. A static unused-code checker must cover manifest-driven loading, generated outputs, and compiler faces without maintaining a parallel per-workspace exception inventory.
For each candidate, record the current owner, effective producer/consumer path, what disappears, what remains, and the strongest reason to retain it. Distinguish:
For each candidate, record the current owner, effective producer/consumer path, what disappears, what remains, capability and compatibility effects, and the strongest reason to retain it. Distinguish:
- removal of unreachable or unread behavior;
- a narrower public or product behavior with an explicit loss;
+2 -2
View File
@@ -30,8 +30,8 @@
en: 053e2c485eec2613
zh: 2817da00572c789f
/defensive-patterns/a-business-capability-owns-itself:
en: 7588b8dba9279dac
zh: 2760468ede60220c
en: f74f2467e507455f
zh: fe1bae04be5ad63a
/defensive-patterns/changes-carry-performance-evidence:
en: 0d6855d1ae0ef4ae
zh: 0f771999398ce052
+2
View File
@@ -125,6 +125,8 @@ The wrong form lets instances overwrite each other, makes reclamation timing unk
A business capability lives in its own package and attaches through a registration surface — a registry, `slots.inject`, a waterfall, or an event subscription. A runtime, controller, manager, or gateway carries generic orchestration: it may hold the registry structure and the disposal lifecycle, while the entries are written by the packages that own them, and it never hardcodes a central table of business rows.
Establish whether a new distinction, field, or adapter is needed before choosing its owner; moving an unnecessary concept does not remove its maintenance cost. For required behavior, prefer an existing extension point near its producer or consumer when the local implementation adds little complexity. A producer can declare a feature variant through [declaration merging](../subsystems/core.md#the-map--derived-union-pattern) in its own package, without editing the package that declares the shared type. Obligations common to all consumers belong in the shared owner.
The judged condition is whether the capability declares and registers itself, with the center layer only forwarding and managing lifetime rather than making business decisions. The boundary is that the center layer holds the registry structure and the disposal lifecycle while the entries come from the owning packages. One exception: pure infrastructure — logging, transport, storage — does not attach through a business registration surface. A new branch in a center layer must explain why it cannot live in the owning package.
```ts
+2
View File
@@ -143,6 +143,8 @@ globalThis.__dshCache = new Map() // shared across instances, reclaimed never
业务能力住在自己的包里,经注册面接入——registry、`slots.inject`、waterfall 或事件订阅。runtime、controller、manager、gateway 承载通用编排:它可以持有注册表结构与 disposal 生命周期,但条目由拥有它们的包写入,且绝不硬编码一张业务中心表。
先确定新增的区分、字段或适配器是否必要,再选择其所有者;移动不必要的概念并不能消除维护成本。对于必需的行为,如果局部实现只增加少量复杂度,就优先使用靠近生产者或消费方的现有扩展点。生产者可以在自己的包内通过[声明合并](../subsystems/core.zh.md#the-map--derived-union-pattern)声明功能专属变体,无需修改声明该共享类型的包。所有消费方共同依赖的义务由共享所有者负责。
判定条件是:该能力是否由业务包自己声明并注册,中心层只做转发与生命周期管理、不做业务判定。边界是:中心层持有注册表结构与 disposal 生命周期,条目来自拥有它们的包。一条例外:纯基础设施能力——日志、传输、存储——不经业务注册面接入。中心层新增分支必须说明它为何无法下沉到业务包。
```ts