From d0910a2ed7780c5110aef8f6b9b1b4b66e4a7e75 Mon Sep 17 00:00:00 2001 From: NI0317 Date: Wed, 15 Jul 2026 21:05:18 +0800 Subject: [PATCH 1/6] docs(skill): distill adopted review feedback --- .agents/skills/dsh-code-review/SKILL.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.agents/skills/dsh-code-review/SKILL.md b/.agents/skills/dsh-code-review/SKILL.md index 3502e4454a..03a0189227 100644 --- a/.agents/skills/dsh-code-review/SKILL.md +++ b/.agents/skills/dsh-code-review/SKILL.md @@ -27,11 +27,17 @@ description: Use when reviewing a pull request in the deepseek-harness repo — ## 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, and quiescent 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. +- **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:** snapshot caller-owned values retained after return or crossing queues, logs, processes, workers, and 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. +- **Final artifact bounds:** verify byte, token, item, and time limits against the complete emitted or retained artifact, including wrappers and metadata. Probe tiny limits, threshold edges, oversized single chunks, and multibyte text. - **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 or changed automated check includes a deliberately invalid fixture that fails through the real top-level runner; a green happy path does not prove the check is wired. +- **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. From 9a4efada5d53928c6b5d8c7fbf482d58c8cac229 Mon Sep 17 00:00:00 2001 From: NI0317 Date: Wed, 15 Jul 2026 21:05:43 +0800 Subject: [PATCH 2/6] docs(rfc): propose periodic human-review maintenance for dsh-code-review A private out-of-repo maintainer tool turns adopted human review feedback into candidate skill updates. Only the resulting SKILL.md diff enters this repository through normal PR review; the tool source, adapter binaries, provider credentials, and scheduler stay on the maintainer's machine to keep this repository focused on shipped product code. Kept in proposed/ until end-to-end verification against master lands a candidate PR; the acceptance criteria list the exact observations that unlock promotion. --- docs/rfc/INDEX.md | 1 + ...26-07-13-human-review-skill-maintenance.md | 77 +++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md diff --git a/docs/rfc/INDEX.md b/docs/rfc/INDEX.md index a374e795bc..c53f0e5bd5 100644 --- a/docs/rfc/INDEX.md +++ b/docs/rfc/INDEX.md @@ -37,6 +37,7 @@ Generated by `pnpm run gen-rfc-index` from the RFC tree — never edit by hand; | [Architectural conformance — dependency rules and the adapter kit](proposed/process/2026-06-11-architectural-conformance.md) | 2026-06-11 | | [Supply chain checks and vendor drift verification](proposed/process/2026-06-11-supply-chain-and-vendor-drift.md) | 2026-06-11 | | [Discover package inventories instead of maintaining static lists](proposed/process/2026-06-20-discover-package-inventory.md) | 2026-06-20 | +| [Periodic human-review maintenance for dsh-code-review](proposed/process/2026-07-13-human-review-skill-maintenance.md) | 2026-07-13 | ### Testing diff --git a/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md b/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md new file mode 100644 index 0000000000..6a760286ff --- /dev/null +++ b/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md @@ -0,0 +1,77 @@ +# RFC: Periodic human-review maintenance for dsh-code-review + +Status: proposed + +## Problem + +The `dsh-code-review` skill records failure modes that require reviewer judgment, but one-off audits are expensive to repeat and easy to scope inconsistently. Treating every comment as a lesson produces checklist bloat; treating merge, thread resolution, or an author's “fixed” reply as proof of adoption promotes feedback that the final code may not implement. The maintenance process needs enough evidence and independent review to fail closed without requiring a webhook service, durable event state, or automatic repository promotion before the workflow has proven useful. + +## Proposal + +Periodic out-of-repo maintenance. A private tool, kept on the skill maintainer's machine rather than committed to this repository, runs against a clean full-history checkout at refreshed `origin/master` on an operator-chosen cadence — daily and weekly are both safe because the scan is idempotent against the current skill, and the `--since` window is a `--since 2d` overlap for daily or `--since 7d` for weekly. Repeated `--pr` arguments inspect an explicit set. The tool stores no repository cursor. The only artifact that reaches this repository is a working-tree diff to [.agents/skills/dsh-code-review/SKILL.md](../../../../.agents/skills/dsh-code-review/SKILL.md) that the maintainer inspects and, if useful, promotes through the repository's normal PR review. + +```mermaid +flowchart TD + A["Maintainer runs the weekly tool on origin/master"] --> B["List PRs merged in the seven-day window"] + B --> C["Collect pre-merge User feedback and final PR evidence"] + C --> D["Two reviewers verify provenance and adoption"] + D --> E{"Both confirm human-authored and adopted?"} + E -- "No" --> F["Exclude or retain as unresolved"] + E -- "Yes" --> G["Two reviewers classify against the current skill"] + G --> H["Draft a complete candidate from agreed guidance"] + H --> I["Two reviewers inspect the same skill diff"] + I -- "Blocking finding" --> J["Bounded revision loop"] + J --> I + I -- "Both approve" --> K["Run documentation and lint checks"] + K --> L["Leave a reviewed local working-tree diff"] +``` + +### Acquisition contract + +Each selected PR is filtered before any feedback is retrieved: its merge commit must be an ancestor of `origin/master`. Merge-commit reachability is the sole eligibility check — a stacked PR whose direct base is a feature branch is admitted whenever the base has since reached master, because the code the reviewer commented on is now on master regardless of the intermediate stack. A single PR that fails preflight, acquisition, or evidence collection is logged to `skipped-pulls.json` and skipped rather than aborting the whole weekly run. The search stage also fails loud when the window would exceed GitHub's 1,000-result search cap so no merged PR is silently omitted. The acquisition stage then reads complete paginated connections for inline review comments, review submissions, PR conversation comments, and PR commits. It admits feedback only when GitHub reports the actor `type` as `User`, and only when both creation and last-edit timestamps strictly predate the PR merge (an equal-timestamp edit is treated as post-merge); review submissions use GraphQL `lastEditedAt` because the REST representation omits edit time. + +### Adoption evidence + +Each feedback item carries a stable source ID and bounded change evidence. When the reviewer's `commit_id` still belongs to the PR (force-push fail-closed), the tool selects the latest PR commit whose committer timestamp strictly predates the feedback as the baseline — not the reviewer's clicked commit, which may be an older commit — and compares that baseline with the merge commit that actually landed on master. Conversation feedback, force-pushed reviews, and any feedback that predates every PR commit fall to the whole-PR baseline and are deterministically classified `unclear` before any reviewer sees them, because the base-to-head diff cannot prove that a change is causally after the feedback; only feedback-commit baselines reach the adapter for adoption. Merge status, a resolved thread, an author's “fixed” reply, or a same-file edit is context rather than adoption proof; the PR author's own comments never reach the adapter as they cannot be adoption of themselves. + +### Dual-reviewer classification and drafting + +Two independently configured reviewer adapters classify every item by provenance (`human-authored`, `forwarded-automation`, or `unclear`) and adoption (`adopted`, `rejected`, or `unclear`). Only matching `human-authored` plus `adopted` verdicts proceed. The adopted set then receives a second independent classification against the current skill: candidate, already covered, implementation-specific, or not feedback. A singleton may qualify; recurrence is not required. Disagreement receives one bounded re-evaluation and remains visible in run artifacts if unresolved. A single batch whose adapter output fails schema or id validation is failed closed at the batch level — every feedback item in it is marked unclear and routed to `excluded` — rather than aborting the whole run; the offending raw output is preserved under the run's private artifacts for debugging. + +The primary adapter drafts from structured agreed guidance, never raw review text. It remains tool-free and read-only by adapter-author contract: it returns complete candidate file content, which the tool validates before writing the sole target. Both adapters then review the same complete skill diff; blocking findings return to a bounded revision loop, and both must approve the same revision. The tool rejects staged changes and edits outside the target skill both before running the documentation and lint gates and again before reporting success, so a gate or concurrent process that adds another path cannot slip through. It restores its own write on failure using best-effort compare-and-swap so a concurrent maintainer edit is not overwritten, produces a reviewed local diff and private run artifacts on success, and never commits, pushes, opens, or merges a PR. + +### Reviewer adapter protocol + +Each private executable receives a byte-bounded, versioned JSON request on stdin and returns byte-bounded, schema-conforming JSON on stdout. The tool refuses to run when the two reviewer commands resolve to byte-identical executables — a minimum-bar mechanical check; guaranteeing that primary and secondary are backed by independent providers or models is the deployment operator's responsibility. The `access` and `tools` fields are contract markers on the adapter author, not an OS sandbox: reviewer subprocesses spawn with a scrubbed environment, `cwd` set to a private run directory rather than the repository root, and feedback wrapped in a nonce-tagged `` block that every prompt instructs the model to treat as data; the 128-bit nonce prevents an untrusted body from forging the closing tag. Every subprocess uses bounded, abort-aware process-tree cleanup. Adapter authors implement each operation as pure read-only inference — even the `edit` operation returns complete candidate content in JSON, which the tool validates and writes to the sole target. Every production `git`/`gh`/gate spawn also uses the scrubbed environment so a pre-push hook's routing variables cannot silently redirect the maintainer. Candidate writes and the failure rollback use best-effort compare-and-swap against the last written content; the rollback also unstages the target so an adapter- or gate-staged candidate cannot survive a failed run into a later commit. + +### Where the mechanism lives + +The tool source, adapter binaries, provider credentials, and the seven-day scheduler are kept private to the maintainer's machine rather than committed to this repository. This document specifies the protocol; the reference implementation is private infrastructure. The mechanism serves a single skill maintained by a single operator, so the ongoing cost of vetting mechanism edits through repository review outweighs any provenance benefit. If the mechanism is ever handed off to a second maintainer, that handoff is a follow-up RFC that revises this decision — the operator doc at [docs/cookbook/maintaining-dsh-code-review.md](../../../cookbook/maintaining-dsh-code-review.md) is the entry point for anyone taking over. + +## Alternatives considered + +- **Ship the tool inside this repository.** Rejected for a single-maintainer scope: repository maintenance overhead (typecheck, lint, coverage, cross-cutting refactors) would exceed the value of committed provenance. Retained option for a later handoff. +- **Record every feedback-time PR head** — rejected: it improves causal isolation but requires a continuously running observer, durable event state, retries, and force-push reconciliation. Periodic maintenance uses reviewed-commit evidence where available and fails closed on broader whole-PR evidence. +- **Persist a processed-PR cursor** — rejected: an overlapping seven-day scan is cheap and naturally idempotent against the current skill, while cursor state creates recovery and missed-event problems. +- **Run on every new comment** — rejected: review waves produce many related comments and lack the final artifact needed to judge adoption. +- **Treat merge or thread resolution as adoption** — rejected: a PR can merge with rejected, superseded, or intentionally unresolved feedback. +- **Create or merge repository changes automatically** — rejected: the tool first needs a track record of useful periodic output. The maintainer inspects and promotes the local diff through normal repository review. +- **Learn from bot findings that were fixed** — rejected: the source contract is human review feedback. Actor type is filtered before analysis, and human accounts forwarding automated findings are excluded by provenance review. +- **Use one reviewer as author and final judge** — rejected: independent verdicts expose unsupported generalization before it reaches the skill. + +## Acceptance criteria + +Promotion from `proposed/` to `implemented/` requires all of the following to be observed in a real end-to-end run against this repository: + +- The private tool runs from a clean detached checkout at refreshed `origin/master` and either reports "no candidate" or produces a working-tree diff limited to `.agents/skills/dsh-code-review/SKILL.md`. **Observed on 2026-07-15:** 62 merged PRs scanned, 5 skipped (unreachable merge commit or >250-commit acquisition cap), 426 human feedback items considered, 0 candidates surfaced. +- Both reviewer adapters are independently configured (distinct providers or models) and complete an analyze / adopt / review pass without user intervention. **Observed on 2026-07-15:** distinct primary/secondary adapters completed adoption + analysis in ~8 minutes; batch fail-closed handled one adapter id-hallucination without aborting the run. +- A scheduler triggers the tool without an interactive terminal, and a candidate diff (or a "no candidate" record) reaches the operator through a durable notification channel. +- At least one candidate diff produced by this workflow is inspected by the operator and promoted to `master` through a normal repository PR review. That PR is the evidence that the workflow can turn adopted feedback into shipped skill guidance. + +## Risks + +- **Causality inferred from committer timestamps.** The feedback-commit baseline is selected by comparing GitHub commit timestamps with feedback creation timestamps; committer clock skew and rewrites still leave a residual false-adoption window. Cross-referencing GitHub's PR event stream would tighten this but requires event acquisition beyond the scope of the periodic tool. +- **Two-non-candidate classifications routed to `excluded` without a dispute round.** When both classifiers say "not a candidate" but disagree on which non-candidate reason applies (for example `covered` vs `specific`), the item is excluded rather than re-evaluated. Both classifiers agree the item does not become new reviewer behavior, so a dispute round would not change the outcome. +- **Dual-reviewer independence beyond byte-hash distinctness is a deployment contract.** The tool refuses to run when the two commands resolve to byte-identical executables, but cannot verify that two distinct wrappers back different providers or models. Operators must configure independent primary and secondary adapters. +- **Best-effort compare-and-swap for candidate writes and rollback.** File-based CAS on POSIX is not truly atomic; the window is one event-loop tick. The tool targets single-user weekly maintenance and a truly concurrent editor is out of scope. +- **Single-maintainer bus factor.** Because the mechanism lives on one machine, its interruption stops skill maintenance entirely until the operator restores service or hands off to a new maintainer through a follow-up RFC. From 3735f8c955c4804828a81ccfd33fef9ce9a35e66 Mon Sep 17 00:00:00 2001 From: NI0317 Date: Wed, 15 Jul 2026 21:05:52 +0800 Subject: [PATCH 3/6] docs(cookbook): add operator guide for dsh-code-review maintenance Entry point for the skill's operator and for any future handoff: what the private periodic tool produces, where the saved candidate diff and SKILL.md land on the operator's machine, and how the promote helper turns the operator's decision into a draft PR from a clean master checkout. Companion to the maintenance RFC. --- docs/cookbook/maintaining-dsh-code-review.md | 61 ++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 docs/cookbook/maintaining-dsh-code-review.md diff --git a/docs/cookbook/maintaining-dsh-code-review.md b/docs/cookbook/maintaining-dsh-code-review.md new file mode 100644 index 0000000000..6158af5850 --- /dev/null +++ b/docs/cookbook/maintaining-dsh-code-review.md @@ -0,0 +1,61 @@ +# Maintaining the dsh-code-review skill + +The [`dsh-code-review`](../../.agents/skills/dsh-code-review/SKILL.md) skill is kept current by a single designated operator running a private weekly maintenance tool. This cookbook is the entry point for that operator — and for anyone taking over the role — and for repo contributors who want to understand why skill updates arrive as small periodic PRs rather than one-off audits. The workflow itself is specified in the [human-review skill-maintenance RFC](../rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md). + +## What the maintainer receives + +On the operator's chosen cadence (currently daily at 10:00 local time, with a two-day overlap window) a private tool runs the workflow the RFC describes: + +1. It selects PRs merged in the chosen window (default two UTC days for the daily cadence, seven for weekly) whose merge commit is reachable from `origin/master`. PRs whose merge commit is not reachable (stacked branches whose parent was squashed) or that exceed a 250-commit acquisition cap are logged to `skipped-pulls.json` and skipped rather than aborting the run. +2. It collects pre-merge human review feedback (inline comments, review submissions, PR conversation comments) and post-feedback diff evidence. +3. Two independently configured reviewer adapters classify provenance and adoption, then classify agreed-adopted items against the current skill. +4. The primary adapter drafts a complete revised `SKILL.md`; both adapters review the same diff; blocking findings loop until both approve. +5. `pnpm run doc-sync` and `pnpm run lint` run against the candidate before the tool declares success. + +Each run stores its artifacts on the operator's machine. The saved diff and candidate `SKILL.md` land under `~/dsh-code-review-outputs/` named by timestamp; the raw per-adapter I/O, adopted evidence, and consensus/dispute JSON stay in a private temp directory whose path is written to the notification and to the daily log under `~/Library/Logs/dsh-code-review-maintainer/`. The maintenance worktree itself is restored clean after every run so the operator is never tempted to edit the maintenance copy in place. + +## What the operator does with a candidate diff + +When a run produces a candidate, a macOS notification arrives with a `dsh-code-review-promote ` hint. + +1. **Read the diff on its own merits.** Do not defer to "the reviewers approved" — the maintainer contract is that the operator is the final judgment. Look for checklist bloat, historical prose, unsupported extrapolation from a single incident, and duplicated coverage with existing skill or authoritative-doc content. + + ```sh + ls ~/dsh-code-review-outputs/ # every candidate ever produced + less ~/dsh-code-review-outputs/2026-07-16T02-00-00Z.diff + less ~/dsh-code-review-outputs/2026-07-16T02-00-00Z.SKILL.md + ``` + +2. **Cross-check against the run artifacts.** Each candidate's per-adapter I/O, consensus, and adopted evidence live under the run's private temp directory (path shown in the log). Spot-check at least one candidate: does the linked human comment actually support the added rule? Does the linked PR actually adopt it? + +3. **Decide one of three:** + - **Discard.** Delete the saved candidate. The tool re-considers the same feedback on the next run under whatever the current skill then says. + + ```sh + rm ~/dsh-code-review-outputs/2026-07-16T02-00-00Z.{diff,SKILL.md} + ``` + - **Batch.** Keep the candidate aside if the update is small and could combine with a future one. + - **Promote.** From a clean `master` checkout of the repo, run the promote helper. It creates a branch, copies the saved candidate over the current skill, commits, pushes, and opens a draft PR — the operator still reviews the PR on GitHub and either merges it or closes it. + + ```sh + cd ~/path/to/deepseek-harness # clean master + dsh-code-review-promote 2026-07-16 + ``` + +4. **Do not commit adapter output verbatim.** Small edits during promotion — tightening wording, removing an example that only makes sense with the source PR's context, folding a rule into an existing one — are expected and preserve the "reviewer judgment" the workflow depends on. Amend the branch before merging. + +## When a run produces no candidate + +That is the common case. The tool records "no candidate" in its daily log, sends no notification (to avoid alert fatigue), and moves on. Days without a skill update are the workflow behaving correctly, not a stall. + +## Interruptions and handoff + +The mechanism lives on one machine. Interruptions the operator handles as they arise: + +- **Daily run missed.** The two-day overlap window catches one skipped day automatically; longer gaps recover by running the wrapper manually with `DSH_CODE_REVIEW_SINCE=`. Overlapping windows are idempotent: guidance already in the current skill is classified `covered` and does not re-enter as a candidate. +- **Adapter provider outage.** The tool refuses to run when the two reviewer commands resolve to byte-identical executables. A single batch whose adapter response fails schema or id validation is failed closed at the batch level (every item in the batch marked unclear) and the run continues; the raw output is preserved for debugging. A total-provider outage that fails every batch produces a "no candidate" result — retry after the provider is restored. +- **Handoff to another maintainer.** Open a follow-up RFC that supersedes the current one: either move the mechanism into the repository or record the new operator's private setup. Do not silently transfer the tool — the "single-maintainer bus factor" in the RFC's Risks section is the reason the handoff needs a documented decision. + +## Where the operator's private setup lives + +The tool source, reviewer adapters, provider credentials, and scheduler are the operator's private infrastructure and are outside this repository by design (see the RFC's "Where the mechanism lives" section). This cookbook and the RFC describe **what the workflow guarantees**; **how** those guarantees are implemented is a private-infrastructure concern. If you are the new operator, the RFC's `## Proposal` sections are the specification you build against. From dffeac037d6b98649ce38dbdc8a42e7289d316af Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 15 Jul 2026 21:39:20 +0800 Subject: [PATCH 4/6] docs(review): fail closed across skill maintenance A saved complete-file candidate could overwrite newer skill guidance, while a provider-wide outage was indistinguishable from a healthy no-op run. Record the source blob and provenance, reject drift during promotion, and make total adapter failure visible. Align cadence and feedback acquisition with the evidence contract, and narrow the borrowed-state and negative-control checks so they do not contradict the package and testing standards. --- .agents/skills/dsh-code-review/SKILL.md | 6 ++--- docs/cookbook/maintaining-dsh-code-review.md | 23 ++++++++++--------- ...26-07-13-human-review-skill-maintenance.md | 23 +++++++++++-------- 3 files changed, 29 insertions(+), 23 deletions(-) diff --git a/.agents/skills/dsh-code-review/SKILL.md b/.agents/skills/dsh-code-review/SKILL.md index 03a0189227..8059950d8f 100644 --- a/.agents/skills/dsh-code-review/SKILL.md +++ b/.agents/skills/dsh-code-review/SKILL.md @@ -32,11 +32,11 @@ description: Use when reviewing a pull request in the deepseek-harness repo — - **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. - **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:** snapshot caller-owned values retained after return or crossing queues, logs, processes, workers, and 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. -- **Final artifact bounds:** verify byte, token, item, and time limits against the complete emitted or retained artifact, including wrappers and metadata. Probe tiny limits, threshold edges, oversized single chunks, and multibyte text. +- **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 or changed automated check includes a deliberately invalid fixture that fails through the real top-level runner; a green happy path does not prove the check is wired. +- **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. - **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/docs/cookbook/maintaining-dsh-code-review.md b/docs/cookbook/maintaining-dsh-code-review.md index 6158af5850..3d83d20bd9 100644 --- a/docs/cookbook/maintaining-dsh-code-review.md +++ b/docs/cookbook/maintaining-dsh-code-review.md @@ -1,18 +1,18 @@ # Maintaining the dsh-code-review skill -The [`dsh-code-review`](../../.agents/skills/dsh-code-review/SKILL.md) skill is kept current by a single designated operator running a private weekly maintenance tool. This cookbook is the entry point for that operator — and for anyone taking over the role — and for repo contributors who want to understand why skill updates arrive as small periodic PRs rather than one-off audits. The workflow itself is specified in the [human-review skill-maintenance RFC](../rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md). +The [`dsh-code-review`](../../.agents/skills/dsh-code-review/SKILL.md) skill is kept current by a single designated operator running a private periodic maintenance tool. This cookbook is the entry point for that operator — and for anyone taking over the role — and for repo contributors who want to understand why skill updates arrive as small periodic PRs rather than one-off audits. The workflow itself is specified in the [human-review skill-maintenance RFC](../rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md). ## What the maintainer receives -On the operator's chosen cadence (currently daily at 10:00 local time, with a two-day overlap window) a private tool runs the workflow the RFC describes: +Run the private tool daily with a two-UTC-day overlap; until the proposed scheduler has completed its acceptance run, the operator invokes the wrapper manually at the same cadence. A manual weekly recovery run uses a seven-day window. The workflow: 1. It selects PRs merged in the chosen window (default two UTC days for the daily cadence, seven for weekly) whose merge commit is reachable from `origin/master`. PRs whose merge commit is not reachable (stacked branches whose parent was squashed) or that exceed a 250-commit acquisition cap are logged to `skipped-pulls.json` and skipped rather than aborting the run. -2. It collects pre-merge human review feedback (inline comments, review submissions, PR conversation comments) and post-feedback diff evidence. +2. It collects pre-merge human review feedback with commit anchors (inline comments and review submissions) plus post-feedback diff evidence. It does not acquire PR conversation comments because current GitHub state cannot give them a force-push-safe feedback-time baseline. 3. Two independently configured reviewer adapters classify provenance and adoption, then classify agreed-adopted items against the current skill. 4. The primary adapter drafts a complete revised `SKILL.md`; both adapters review the same diff; blocking findings loop until both approve. 5. `pnpm run doc-sync` and `pnpm run lint` run against the candidate before the tool declares success. -Each run stores its artifacts on the operator's machine. The saved diff and candidate `SKILL.md` land under `~/dsh-code-review-outputs/` named by timestamp; the raw per-adapter I/O, adopted evidence, and consensus/dispute JSON stay in a private temp directory whose path is written to the notification and to the daily log under `~/Library/Logs/dsh-code-review-maintainer/`. The maintenance worktree itself is restored clean after every run so the operator is never tempted to edit the maintenance copy in place. +Each run stores its artifacts on the operator's machine. The saved diff, candidate `SKILL.md`, and promotion manifest land under `~/dsh-code-review-outputs/` named by timestamp. The manifest records the source master commit and skill blob, source feedback IDs and URLs, landed evidence ranges, adapter verdicts, and gate results; raw per-adapter I/O stays in a private temp directory whose path is written to the notification and to the daily log under `~/Library/Logs/dsh-code-review-maintainer/`. The maintenance worktree itself is restored clean after every run so the operator is never tempted to edit the maintenance copy in place. ## What the operator does with a candidate diff @@ -24,36 +24,37 @@ When a run produces a candidate, a macOS notification arrives with a `dsh-code-r ls ~/dsh-code-review-outputs/ # every candidate ever produced less ~/dsh-code-review-outputs/2026-07-16T02-00-00Z.diff less ~/dsh-code-review-outputs/2026-07-16T02-00-00Z.SKILL.md + less ~/dsh-code-review-outputs/2026-07-16T02-00-00Z.manifest.json ``` -2. **Cross-check against the run artifacts.** Each candidate's per-adapter I/O, consensus, and adopted evidence live under the run's private temp directory (path shown in the log). Spot-check at least one candidate: does the linked human comment actually support the added rule? Does the linked PR actually adopt it? +2. **Cross-check against the run artifacts.** The promotion manifest maps each proposed rule to source feedback and landed evidence; detailed per-adapter I/O, consensus, and adopted evidence live under the run's private temp directory (path shown in the log). Spot-check at least one candidate: does the linked human comment actually support the added rule? Does the linked PR actually adopt it? 3. **Decide one of three:** - **Discard.** Delete the saved candidate. The tool re-considers the same feedback on the next run under whatever the current skill then says. ```sh - rm ~/dsh-code-review-outputs/2026-07-16T02-00-00Z.{diff,SKILL.md} + rm ~/dsh-code-review-outputs/2026-07-16T02-00-00Z.{diff,SKILL.md,manifest.json} ``` - - **Batch.** Keep the candidate aside if the update is small and could combine with a future one. - - **Promote.** From a clean `master` checkout of the repo, run the promote helper. It creates a branch, copies the saved candidate over the current skill, commits, pushes, and opens a draft PR — the operator still reviews the PR on GitHub and either merges it or closes it. + - **Batch.** Keep the candidate aside if the update is small and could combine with a future one. The source-skill check still applies; rerun the analysis or manually rebase and re-review the diff if `master` changes first. + - **Promote.** From a clean `master` checkout of the repo, run the promote helper. It refreshes `master`, verifies that the current skill matches the recorded source blob, applies the saved diff, and opens a draft PR whose body carries the manifest's provenance summary. It stops on skill drift rather than overwriting newer guidance; the operator still reviews the PR on GitHub and either merges it or closes it. ```sh cd ~/path/to/deepseek-harness # clean master - dsh-code-review-promote 2026-07-16 + dsh-code-review-promote 2026-07-16T02-00-00Z ``` 4. **Do not commit adapter output verbatim.** Small edits during promotion — tightening wording, removing an example that only makes sense with the source PR's context, folding a rule into an existing one — are expected and preserve the "reviewer judgment" the workflow depends on. Amend the branch before merging. ## When a run produces no candidate -That is the common case. The tool records "no candidate" in its daily log, sends no notification (to avoid alert fatigue), and moves on. Days without a skill update are the workflow behaving correctly, not a stall. +That is the common case after every nonempty classification stage has produced at least one valid adapter result. The tool records "no candidate" in its daily log, sends no notification (to avoid alert fatigue), and moves on. Days without a skill update are the workflow behaving correctly, not a stall. ## Interruptions and handoff The mechanism lives on one machine. Interruptions the operator handles as they arise: - **Daily run missed.** The two-day overlap window catches one skipped day automatically; longer gaps recover by running the wrapper manually with `DSH_CODE_REVIEW_SINCE=`. Overlapping windows are idempotent: guidance already in the current skill is classified `covered` and does not re-enter as a candidate. -- **Adapter provider outage.** The tool refuses to run when the two reviewer commands resolve to byte-identical executables. A single batch whose adapter response fails schema or id validation is failed closed at the batch level (every item in the batch marked unclear) and the run continues; the raw output is preserved for debugging. A total-provider outage that fails every batch produces a "no candidate" result — retry after the provider is restored. +- **Adapter provider outage.** The tool refuses to run when the two reviewer commands resolve to byte-identical executables. A single batch whose adapter response fails schema or id validation is failed closed at the batch level (every item in the batch marked unclear) and the run continues; the raw output is preserved for debugging. If either adapter produces no valid result for any nonempty batch in an operation, the run fails, writes a failure record, and notifies the operator; it never collapses a total-provider outage into "no candidate." - **Handoff to another maintainer.** Open a follow-up RFC that supersedes the current one: either move the mechanism into the repository or record the new operator's private setup. Do not silently transfer the tool — the "single-maintainer bus factor" in the RFC's Risks section is the reason the handoff needs a documented decision. ## Where the operator's private setup lives diff --git a/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md b/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md index 6a760286ff..9629a832e8 100644 --- a/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md +++ b/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md @@ -8,11 +8,11 @@ The `dsh-code-review` skill records failure modes that require reviewer judgment ## Proposal -Periodic out-of-repo maintenance. A private tool, kept on the skill maintainer's machine rather than committed to this repository, runs against a clean full-history checkout at refreshed `origin/master` on an operator-chosen cadence — daily and weekly are both safe because the scan is idempotent against the current skill, and the `--since` window is a `--since 2d` overlap for daily or `--since 7d` for weekly. Repeated `--pr` arguments inspect an explicit set. The tool stores no repository cursor. The only artifact that reaches this repository is a working-tree diff to [.agents/skills/dsh-code-review/SKILL.md](../../../../.agents/skills/dsh-code-review/SKILL.md) that the maintainer inspects and, if useful, promotes through the repository's normal PR review. +Periodic out-of-repo maintenance. A private tool, kept on the skill maintainer's machine rather than committed to this repository, runs against a clean full-history checkout at refreshed `origin/master`. The intended scheduler runs daily with a two-UTC-day overlap; manual runs accept another `--since` duration or repeated `--pr` arguments for an explicit set. The scan is idempotent against the current skill and stores no repository cursor. The only repository file changed by promotion is [.agents/skills/dsh-code-review/SKILL.md](../../../../.agents/skills/dsh-code-review/SKILL.md); the draft PR carries a provenance summary so reviewers can audit the source feedback and adoption evidence without the private adapter logs. ```mermaid flowchart TD - A["Maintainer runs the weekly tool on origin/master"] --> B["List PRs merged in the seven-day window"] + A["Maintainer or scheduler runs the tool on origin/master"] --> B["List PRs merged in the overlap window"] B --> C["Collect pre-merge User feedback and final PR evidence"] C --> D["Two reviewers verify provenance and adoption"] D --> E{"Both confirm human-authored and adopted?"} @@ -28,31 +28,35 @@ flowchart TD ### Acquisition contract -Each selected PR is filtered before any feedback is retrieved: its merge commit must be an ancestor of `origin/master`. Merge-commit reachability is the sole eligibility check — a stacked PR whose direct base is a feature branch is admitted whenever the base has since reached master, because the code the reviewer commented on is now on master regardless of the intermediate stack. A single PR that fails preflight, acquisition, or evidence collection is logged to `skipped-pulls.json` and skipped rather than aborting the whole weekly run. The search stage also fails loud when the window would exceed GitHub's 1,000-result search cap so no merged PR is silently omitted. The acquisition stage then reads complete paginated connections for inline review comments, review submissions, PR conversation comments, and PR commits. It admits feedback only when GitHub reports the actor `type` as `User`, and only when both creation and last-edit timestamps strictly predate the PR merge (an equal-timestamp edit is treated as post-merge); review submissions use GraphQL `lastEditedAt` because the REST representation omits edit time. +Each selected PR is filtered before any feedback is retrieved: its merge commit must be an ancestor of `origin/master`. Merge-commit reachability is the sole eligibility check — a stacked PR whose direct base is a feature branch is admitted whenever the base has since reached master, because the code the reviewer commented on is now on master regardless of the intermediate stack. A single PR that fails preflight, acquisition, or evidence collection is logged to `skipped-pulls.json` and skipped rather than aborting the whole run. The search stage also fails loud when the window would exceed GitHub's 1,000-result search cap so no merged PR is silently omitted. The acquisition stage reads complete paginated connections for inline review comments, review submissions, and PR commits. PR conversation comments are not acquired because current GitHub state cannot prove which surviving commit preceded them after a force-push, so the adoption contract would exclude them unconditionally. The workflow admits acquired feedback only when GitHub reports the actor `type` as `User`, and only when both creation and last-edit timestamps strictly predate the PR merge (an equal-timestamp edit is treated as post-merge); review submissions use GraphQL `lastEditedAt` because the REST representation omits edit time. ### Adoption evidence -Each feedback item carries a stable source ID and bounded change evidence. When the reviewer's `commit_id` still belongs to the PR (force-push fail-closed), the tool selects the latest PR commit whose committer timestamp strictly predates the feedback as the baseline — not the reviewer's clicked commit, which may be an older commit — and compares that baseline with the merge commit that actually landed on master. Conversation feedback, force-pushed reviews, and any feedback that predates every PR commit fall to the whole-PR baseline and are deterministically classified `unclear` before any reviewer sees them, because the base-to-head diff cannot prove that a change is causally after the feedback; only feedback-commit baselines reach the adapter for adoption. Merge status, a resolved thread, an author's “fixed” reply, or a same-file edit is context rather than adoption proof; the PR author's own comments never reach the adapter as they cannot be adoption of themselves. +Each feedback item carries a stable source ID and bounded change evidence. When the reviewer's `commit_id` still belongs to the PR (force-push fail-closed), the tool selects the latest PR commit whose committer timestamp strictly predates the feedback as the baseline — not the reviewer's clicked commit, which may be an older commit — and compares that baseline with the merge commit that actually landed on master. Force-pushed reviews and feedback that predates every surviving PR commit fall to the whole-PR baseline and are deterministically classified `unclear` before any reviewer sees them, because the base-to-head diff cannot prove that a change is causally after the feedback; only feedback-commit baselines reach the adapter for adoption. Merge status, a resolved thread, an author's “fixed” reply, or a same-file edit is context rather than adoption proof; the PR author's own comments never reach the adapter as they cannot be adoption of themselves. ### Dual-reviewer classification and drafting -Two independently configured reviewer adapters classify every item by provenance (`human-authored`, `forwarded-automation`, or `unclear`) and adoption (`adopted`, `rejected`, or `unclear`). Only matching `human-authored` plus `adopted` verdicts proceed. The adopted set then receives a second independent classification against the current skill: candidate, already covered, implementation-specific, or not feedback. A singleton may qualify; recurrence is not required. Disagreement receives one bounded re-evaluation and remains visible in run artifacts if unresolved. A single batch whose adapter output fails schema or id validation is failed closed at the batch level — every feedback item in it is marked unclear and routed to `excluded` — rather than aborting the whole run; the offending raw output is preserved under the run's private artifacts for debugging. +Two independently configured reviewer adapters classify every eligible item by provenance (`human-authored`, `forwarded-automation`, or `unclear`) and adoption (`adopted`, `rejected`, or `unclear`). Only matching `human-authored` plus `adopted` verdicts proceed. The adopted set then receives a second independent classification against the current skill: candidate, already covered, implementation-specific, or not feedback. A singleton may qualify; recurrence is not required. Disagreement receives one bounded re-evaluation and remains visible in run artifacts if unresolved. A single batch whose adapter output fails schema or id validation is failed closed at the batch level — every feedback item in it is marked unclear and routed to `excluded` — rather than aborting the whole run; the offending raw output is preserved under the run's private artifacts for debugging. If either adapter returns no valid result for any nonempty batch in an operation, the run exits non-zero and emits a failure record instead of reporting “no candidate.” -The primary adapter drafts from structured agreed guidance, never raw review text. It remains tool-free and read-only by adapter-author contract: it returns complete candidate file content, which the tool validates before writing the sole target. Both adapters then review the same complete skill diff; blocking findings return to a bounded revision loop, and both must approve the same revision. The tool rejects staged changes and edits outside the target skill both before running the documentation and lint gates and again before reporting success, so a gate or concurrent process that adds another path cannot slip through. It restores its own write on failure using best-effort compare-and-swap so a concurrent maintainer edit is not overwritten, produces a reviewed local diff and private run artifacts on success, and never commits, pushes, opens, or merges a PR. +The primary adapter drafts from structured agreed guidance, never raw review text. It remains tool-free and read-only by adapter-author contract: it returns complete candidate file content, which the tool validates before writing the sole target. Both adapters then review the same complete skill diff; blocking findings return to a bounded revision loop, and both must approve the same revision. The tool rejects staged changes and edits outside the target skill both before running the documentation and lint gates and again before reporting success, so a gate or concurrent process that adds another path cannot slip through. It restores its own write on failure using best-effort compare-and-swap so a concurrent maintainer edit is not overwritten. On success it saves a candidate bundle containing the source `origin/master` commit, source skill blob ID, reviewed diff, complete candidate, source feedback IDs and URLs, landed evidence ranges, adapter verdicts, and gate results; it never commits, pushes, opens, or merges a PR. ### Reviewer adapter protocol Each private executable receives a byte-bounded, versioned JSON request on stdin and returns byte-bounded, schema-conforming JSON on stdout. The tool refuses to run when the two reviewer commands resolve to byte-identical executables — a minimum-bar mechanical check; guaranteeing that primary and secondary are backed by independent providers or models is the deployment operator's responsibility. The `access` and `tools` fields are contract markers on the adapter author, not an OS sandbox: reviewer subprocesses spawn with a scrubbed environment, `cwd` set to a private run directory rather than the repository root, and feedback wrapped in a nonce-tagged `` block that every prompt instructs the model to treat as data; the 128-bit nonce prevents an untrusted body from forging the closing tag. Every subprocess uses bounded, abort-aware process-tree cleanup. Adapter authors implement each operation as pure read-only inference — even the `edit` operation returns complete candidate content in JSON, which the tool validates and writes to the sole target. Every production `git`/`gh`/gate spawn also uses the scrubbed environment so a pre-push hook's routing variables cannot silently redirect the maintainer. Candidate writes and the failure rollback use best-effort compare-and-swap against the last written content; the rollback also unstages the target so an adapter- or gate-staged candidate cannot survive a failed run into a later commit. +### Promotion contract + +The promote helper starts from a clean checkout at refreshed `origin/master` and refuses to apply a candidate when the current skill blob differs from the bundle's recorded source blob. The operator then reruns the maintenance analysis or manually rebases the diff and repeats the candidate review; the helper never replaces a newer `SKILL.md` with stale complete-file output. After applying a current candidate, it opens a draft PR whose body lists the source feedback URLs or IDs, the landed commit range used as adoption evidence, the originating run, gate results, and any operator edits. Raw adapter prompts and responses remain private, but repository reviewers receive enough provenance to judge whether each proposed rule follows from adopted human feedback. + ### Where the mechanism lives -The tool source, adapter binaries, provider credentials, and the seven-day scheduler are kept private to the maintainer's machine rather than committed to this repository. This document specifies the protocol; the reference implementation is private infrastructure. The mechanism serves a single skill maintained by a single operator, so the ongoing cost of vetting mechanism edits through repository review outweighs any provenance benefit. If the mechanism is ever handed off to a second maintainer, that handoff is a follow-up RFC that revises this decision — the operator doc at [docs/cookbook/maintaining-dsh-code-review.md](../../../cookbook/maintaining-dsh-code-review.md) is the entry point for anyone taking over. +The tool source, adapter binaries, provider credentials, and intended daily scheduler are kept private to the maintainer's machine rather than committed to this repository. This document specifies the protocol; the reference implementation is private infrastructure. The mechanism serves a single skill maintained by a single operator, so the ongoing cost of vetting mechanism edits through repository review outweighs any provenance benefit. If the mechanism is ever handed off to a second maintainer, that handoff is a follow-up RFC that revises this decision — the operator doc at [docs/cookbook/maintaining-dsh-code-review.md](../../../cookbook/maintaining-dsh-code-review.md) is the entry point for anyone taking over. ## Alternatives considered - **Ship the tool inside this repository.** Rejected for a single-maintainer scope: repository maintenance overhead (typecheck, lint, coverage, cross-cutting refactors) would exceed the value of committed provenance. Retained option for a later handoff. - **Record every feedback-time PR head** — rejected: it improves causal isolation but requires a continuously running observer, durable event state, retries, and force-push reconciliation. Periodic maintenance uses reviewed-commit evidence where available and fails closed on broader whole-PR evidence. -- **Persist a processed-PR cursor** — rejected: an overlapping seven-day scan is cheap and naturally idempotent against the current skill, while cursor state creates recovery and missed-event problems. +- **Persist a processed-PR cursor** — rejected: an overlapping time-window scan is cheap and naturally idempotent against the current skill, while cursor state creates recovery and missed-event problems. - **Run on every new comment** — rejected: review waves produce many related comments and lack the final artifact needed to judge adoption. - **Treat merge or thread resolution as adoption** — rejected: a PR can merge with rejected, superseded, or intentionally unresolved feedback. - **Create or merge repository changes automatically** — rejected: the tool first needs a track record of useful periodic output. The maintainer inspects and promotes the local diff through normal repository review. @@ -66,6 +70,7 @@ Promotion from `proposed/` to `implemented/` requires all of the following to be - The private tool runs from a clean detached checkout at refreshed `origin/master` and either reports "no candidate" or produces a working-tree diff limited to `.agents/skills/dsh-code-review/SKILL.md`. **Observed on 2026-07-15:** 62 merged PRs scanned, 5 skipped (unreachable merge commit or >250-commit acquisition cap), 426 human feedback items considered, 0 candidates surfaced. - Both reviewer adapters are independently configured (distinct providers or models) and complete an analyze / adopt / review pass without user intervention. **Observed on 2026-07-15:** distinct primary/secondary adapters completed adoption + analysis in ~8 minutes; batch fail-closed handled one adapter id-hallucination without aborting the run. - A scheduler triggers the tool without an interactive terminal, and a candidate diff (or a "no candidate" record) reaches the operator through a durable notification channel. +- The promote helper rejects a candidate after the source skill changes, and a current candidate opens a draft PR with the provenance summary defined above. - At least one candidate diff produced by this workflow is inspected by the operator and promoted to `master` through a normal repository PR review. That PR is the evidence that the workflow can turn adopted feedback into shipped skill guidance. ## Risks @@ -73,5 +78,5 @@ Promotion from `proposed/` to `implemented/` requires all of the following to be - **Causality inferred from committer timestamps.** The feedback-commit baseline is selected by comparing GitHub commit timestamps with feedback creation timestamps; committer clock skew and rewrites still leave a residual false-adoption window. Cross-referencing GitHub's PR event stream would tighten this but requires event acquisition beyond the scope of the periodic tool. - **Two-non-candidate classifications routed to `excluded` without a dispute round.** When both classifiers say "not a candidate" but disagree on which non-candidate reason applies (for example `covered` vs `specific`), the item is excluded rather than re-evaluated. Both classifiers agree the item does not become new reviewer behavior, so a dispute round would not change the outcome. - **Dual-reviewer independence beyond byte-hash distinctness is a deployment contract.** The tool refuses to run when the two commands resolve to byte-identical executables, but cannot verify that two distinct wrappers back different providers or models. Operators must configure independent primary and secondary adapters. -- **Best-effort compare-and-swap for candidate writes and rollback.** File-based CAS on POSIX is not truly atomic; the window is one event-loop tick. The tool targets single-user weekly maintenance and a truly concurrent editor is out of scope. +- **Best-effort compare-and-swap for candidate writes and rollback.** File-based CAS on POSIX is not truly atomic; the window is one event-loop tick. The tool targets single-user periodic maintenance and a truly concurrent editor is out of scope. - **Single-maintainer bus factor.** Because the mechanism lives on one machine, its interruption stops skill maintenance entirely until the operator restores service or hands off to a new maintainer through a follow-up RFC. From d340fb2999690d881093a033e56b2cbbe3d26229 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 15 Jul 2026 21:43:32 +0800 Subject: [PATCH 5/6] docs(rfc): isolate PR-owned adoption evidence Comparing a feedback-time PR commit directly with the landing merge lets unrelated target-branch changes satisfy a review comment and creates false adoption evidence. Define feedback-time and final PR-specific patch snapshots, fail closed when the landing shape cannot be reconstructed, and require a target-only negative control before implementation. --- docs/cookbook/maintaining-dsh-code-review.md | 2 +- .../process/2026-07-13-human-review-skill-maintenance.md | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/cookbook/maintaining-dsh-code-review.md b/docs/cookbook/maintaining-dsh-code-review.md index 3d83d20bd9..5a6c2df120 100644 --- a/docs/cookbook/maintaining-dsh-code-review.md +++ b/docs/cookbook/maintaining-dsh-code-review.md @@ -7,7 +7,7 @@ The [`dsh-code-review`](../../.agents/skills/dsh-code-review/SKILL.md) skill is Run the private tool daily with a two-UTC-day overlap; until the proposed scheduler has completed its acceptance run, the operator invokes the wrapper manually at the same cadence. A manual weekly recovery run uses a seven-day window. The workflow: 1. It selects PRs merged in the chosen window (default two UTC days for the daily cadence, seven for weekly) whose merge commit is reachable from `origin/master`. PRs whose merge commit is not reachable (stacked branches whose parent was squashed) or that exceed a 250-commit acquisition cap are logged to `skipped-pulls.json` and skipped rather than aborting the run. -2. It collects pre-merge human review feedback with commit anchors (inline comments and review submissions) plus post-feedback diff evidence. It does not acquire PR conversation comments because current GitHub state cannot give them a force-push-safe feedback-time baseline. +2. It collects pre-merge human review feedback with commit anchors (inline comments and review submissions), then compares feedback-time and final landed PR patches. It does not acquire PR conversation comments because current GitHub state cannot give them a force-push-safe feedback-time baseline, and it excludes target-branch-only changes from adoption evidence. 3. Two independently configured reviewer adapters classify provenance and adoption, then classify agreed-adopted items against the current skill. 4. The primary adapter drafts a complete revised `SKILL.md`; both adapters review the same diff; blocking findings loop until both approve. 5. `pnpm run doc-sync` and `pnpm run lint` run against the candidate before the tool declares success. diff --git a/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md b/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md index 9629a832e8..8098247fdf 100644 --- a/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md +++ b/docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md @@ -28,11 +28,11 @@ flowchart TD ### Acquisition contract -Each selected PR is filtered before any feedback is retrieved: its merge commit must be an ancestor of `origin/master`. Merge-commit reachability is the sole eligibility check — a stacked PR whose direct base is a feature branch is admitted whenever the base has since reached master, because the code the reviewer commented on is now on master regardless of the intermediate stack. A single PR that fails preflight, acquisition, or evidence collection is logged to `skipped-pulls.json` and skipped rather than aborting the whole run. The search stage also fails loud when the window would exceed GitHub's 1,000-result search cap so no merged PR is silently omitted. The acquisition stage reads complete paginated connections for inline review comments, review submissions, and PR commits. PR conversation comments are not acquired because current GitHub state cannot prove which surviving commit preceded them after a force-push, so the adoption contract would exclude them unconditionally. The workflow admits acquired feedback only when GitHub reports the actor `type` as `User`, and only when both creation and last-edit timestamps strictly predate the PR merge (an equal-timestamp edit is treated as post-merge); review submissions use GraphQL `lastEditedAt` because the REST representation omits edit time. +Each selected PR is filtered before any feedback is retrieved: its merge commit must be an ancestor of `origin/master`. Merge-commit reachability is the sole eligibility check — a stacked PR whose direct base is a feature branch is admitted whenever the base has since reached master, because the code the reviewer commented on is now on master regardless of the intermediate stack. The tool also resolves the landing merge's target parent; a landing shape it cannot reconstruct is logged to `skipped-pulls.json` and skipped. A single PR that fails preflight, acquisition, or evidence collection is skipped rather than aborting the whole run. The search stage fails loud when the window would exceed GitHub's 1,000-result search cap so no merged PR is silently omitted. The acquisition stage reads complete paginated connections for inline review comments, review submissions, and PR commits. PR conversation comments are not acquired because current GitHub state cannot prove which surviving commit preceded them after a force-push, so the adoption contract would exclude them unconditionally. The workflow admits acquired feedback only when GitHub reports the actor `type` as `User`, and only when both creation and last-edit timestamps strictly predate the PR merge (an equal-timestamp edit is treated as post-merge); review submissions use GraphQL `lastEditedAt` because the REST representation omits edit time. ### Adoption evidence -Each feedback item carries a stable source ID and bounded change evidence. When the reviewer's `commit_id` still belongs to the PR (force-push fail-closed), the tool selects the latest PR commit whose committer timestamp strictly predates the feedback as the baseline — not the reviewer's clicked commit, which may be an older commit — and compares that baseline with the merge commit that actually landed on master. Force-pushed reviews and feedback that predates every surviving PR commit fall to the whole-PR baseline and are deterministically classified `unclear` before any reviewer sees them, because the base-to-head diff cannot prove that a change is causally after the feedback; only feedback-commit baselines reach the adapter for adoption. Merge status, a resolved thread, an author's “fixed” reply, or a same-file edit is context rather than adoption proof; the PR author's own comments never reach the adapter as they cannot be adoption of themselves. +Each feedback item carries a stable source ID and bounded change evidence. When the reviewer's `commit_id` still belongs to the PR (force-push fail-closed), the tool selects the latest PR commit whose committer timestamp strictly predates the feedback as the baseline — not the reviewer's clicked commit, which may be an older commit. It never compares that baseline directly with the landing merge: such a diff includes unrelated changes from an advancing target branch. Instead, it gives the adoption reviewers two PR-specific patch snapshots. Let `B` be the feedback baseline, `T` the landing merge's target parent, and `M` the landing merge. The feedback-time snapshot is the tree diff from `merge-base(B, T)` to `B`; the final snapshot is the tree diff from `T` to `M`. A target-only change therefore appears in neither PR patch, while a change added to the PR after feedback appears only in the final snapshot. Force-pushed reviews, feedback that predates every surviving PR commit, and landing shapes whose target parent cannot be reconstructed are deterministically classified `unclear` before any reviewer sees them. Merge status, a resolved thread, an author's “fixed” reply, or a same-file edit is context rather than adoption proof; the PR author's own comments never reach the adapter as they cannot be adoption of themselves. ### Dual-reviewer classification and drafting @@ -70,6 +70,7 @@ Promotion from `proposed/` to `implemented/` requires all of the following to be - The private tool runs from a clean detached checkout at refreshed `origin/master` and either reports "no candidate" or produces a working-tree diff limited to `.agents/skills/dsh-code-review/SKILL.md`. **Observed on 2026-07-15:** 62 merged PRs scanned, 5 skipped (unreachable merge commit or >250-commit acquisition cap), 426 human feedback items considered, 0 candidates surfaced. - Both reviewer adapters are independently configured (distinct providers or models) and complete an analyze / adopt / review pass without user intervention. **Observed on 2026-07-15:** distinct primary/secondary adapters completed adoption + analysis in ~8 minutes; batch fail-closed handled one adapter id-hallucination without aborting the run. - A scheduler triggers the tool without an interactive terminal, and a candidate diff (or a "no candidate" record) reaches the operator through a durable notification channel. +- A controlled acquisition case advances the target branch with a feedback-matching change after the feedback baseline; the reviewer evidence excludes that target-only change while retaining a later PR-owned change. - The promote helper rejects a candidate after the source skill changes, and a current candidate opens a draft PR with the provenance summary defined above. - At least one candidate diff produced by this workflow is inspected by the operator and promoted to `master` through a normal repository PR review. That PR is the evidence that the workflow can turn adopted feedback into shipped skill guidance. From cd092e70ad096cdf1ab5d183f45d70b8712eb421 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 15 Jul 2026 21:56:46 +0800 Subject: [PATCH 6/6] docs(skill): require critical prose review A source-of-truth link did not make the review obligation explicit, so reviewers could treat green documentation checks as sufficient. Make dsh-prose-standard a blocking requirement for every added or changed prose form and require semantic comparison with the owning code or behavior. --- .agents/skills/dsh-code-review/SKILL.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.agents/skills/dsh-code-review/SKILL.md b/.agents/skills/dsh-code-review/SKILL.md index 8059950d8f..2b562cc321 100644 --- a/.agents/skills/dsh-code-review/SKILL.md +++ b/.agents/skills/dsh-code-review/SKILL.md @@ -19,10 +19,11 @@ description: Use when reviewing a pull request in the deepseek-harness repo — ## Blocking requirements -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 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. +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. +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