Merge remote-tracking branch 'origin/master' into codex/pr106-master-merge-review-20260715

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

@@ -19,19 +19,26 @@ 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
 
 - **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:** 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.
+- **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.
 

docs/cookbook/maintaining-dsh-code-review.md

@@ -0,0 +1,62 @@
+# 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 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
+
+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), 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.
+
+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
+
+When a run produces a candidate, a macOS notification arrives with a `dsh-code-review-promote <timestamp>` 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
+   less ~/dsh-code-review-outputs/2026-07-16T02-00-00Z.manifest.json
+   ```
+
+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,manifest.json}
+     ```
+   - **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-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 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=<Nd>`. 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. 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
+
+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.

docs/rfc/INDEX.md

@@ -36,6 +36,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
 

docs/rfc/proposed/process/2026-07-13-human-review-skill-maintenance.md

@@ -0,0 +1,83 @@
+# 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`. 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 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?"}
+  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. 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. 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
+
+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. 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 `<untrusted-feedback nonce="…">` 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 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 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.
+- **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.
+- 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.
+
+## 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 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.