Merge remote-tracking branch 'origin/master' into codex/simp-session-log-representation
# Conflicts: # docs/tool-catalog.md # examples/acp-agent/tests/snapshots/escalation-approved/session.jsonl # examples/acp-agent/tests/snapshots/escalation-rejected/session.jsonl # examples/acp-agent/tests/snapshots/hook-cc-pretool-ask/session.jsonl # examples/acp-agent/tests/snapshots/permission-switching/session.jsonl # examples/acp-agent/tests/snapshots/permission-switching/system-prompt.golden.md
This commit is contained in:
Tianyi Cui
@@ -1,41 +0,0 @@
|
||||
# RFC: Extract a generic long-running tool runtime
|
||||
|
||||
Status: proposed
|
||||
|
||||
## Problem
|
||||
|
||||
The bash capability seam supports both foreground commands and long-running background tasks. Background support is large: the abstract executor exposes `start`, `get`, `ownerOf`, `list`, `readOutput`, `kill`, and `onTaskDone`; the local executor tracks tasks, incremental reads, owner tokens, process cleanup, and completion listeners; the model sees three tools (`bash`, `bash_output`, `bash_kill`); the tool plugin injects completion notices back into the owning agent's session. The local executor fences task access behind owner tokens because predictable global task ids are a cross-session read/kill hazard.
|
||||
|
||||
The [tool cookbook](../../../cookbook/adding-a-tool.md) already points at the real design smell: background bash is really generic long-running-tool infrastructure living inside one tool. If future tools need background execution, polling, kill, ownership, and completion notices, those semantics should not be hidden in `dsh-bash`.
|
||||
|
||||
## Proposal
|
||||
|
||||
Move long-running task semantics above bash into a tool-agnostic runtime. Bash remains able to run background commands, but it stops owning the general concepts of task ids, ownership tokens, polling, cancellation, completion notifications, and model-facing "read/kill this task" commands.
|
||||
|
||||
The runtime should own:
|
||||
|
||||
- Stable task ids and owner tokens keyed to the calling session/agent.
|
||||
- Registration of a long-running task with a producer for incremental output and a completion promise.
|
||||
- Generic read/cancel/list operations with the same cross-session authorization rule for every tool.
|
||||
- Completion notification injection into the owning session.
|
||||
- Presentation hooks for pending/running/completed task state, with bash supplying only command-specific labels and output formatting.
|
||||
|
||||
`dsh-bash` then keeps the bash-specific execution contract: resolve a request into a command spec, run a foreground command, or start a process and hand its streams/process handle to the generic runtime. `dsh-tool-bash` keeps the model-facing command tool, but the follow-up operations become generic long-running-tool operations or a shared utility that bash registers with, rather than bespoke `bash_output`/`bash_kill` plumbing.
|
||||
|
||||
## Current seam consumption
|
||||
|
||||
Current consumers split cleanly: `dsh-tool-bash` uses the full foreground/background seam, while hook bridges use only foreground `resolve` and `run` with trusted `stdin` and `env`. `get` and `list` are test-only; `BashTask.done` is implementation-only for disposal, while production completion uses `onTaskDone`. An extracted runtime should expose one public completion mechanism, preserve the simple foreground path for hooks, and decide whether background `timeoutMs` belongs on `start`. If it owns process spawning, it should also centralize the duplicated credential scrub.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- The bash-specific packages no longer define the generic task registry, owner-token authorization, polling, cancellation, or completion-notification machinery.
|
||||
- A shared long-running-task service or tool layer owns those semantics and is documented as the path for any future background-capable tool.
|
||||
- Bash background behavior remains available through the shared layer, with tests proving cross-session isolation still holds.
|
||||
- ACP and snapshot fixtures render background bash through the shared task vocabulary, not through bash-only lifecycle semantics.
|
||||
- The [tool cookbook](../../../cookbook/adding-a-tool.md) points long-running tools at the shared runtime instead of telling each tool to invent its own task protocol.
|
||||
|
||||
## Risks
|
||||
|
||||
The bash package loses local ownership of an already-working background-task implementation, and the implementing PR may temporarily churn model-facing tool names or transcript presentation. That churn is worthwhile if it leaves one background-task contract instead of making every future long-running tool clone bash's private protocol.
|
||||
|
||||
<!-- rfc-format: alternatives-not-recorded (pre-format RFC) -->
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user