This is an automated email from the ASF dual-hosted git repository.
morningman pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/doris-skills.git
The following commit(s) were added to refs/heads/main by this push:
new 028ba92 feat(doris-repo-review): carry review state across runs, add
coverage report and premise checks (#10)
028ba92 is described below
commit 028ba927e1d9052c987156e6db1b03c6ed8856cc
Author: Mingyu Chen (Rayner) <[email protected]>
AuthorDate: Thu Aug 20 12:54:36 2026 +0800
feat(doris-repo-review): carry review state across runs, add coverage
report and premise checks (#10)
A PR is normally reviewed more than once, but every run used to start from
zero:
the dismissals were re-derived, the same files were re-read, and "was this
considered last time?" was unanswerable. This teaches the skill to inherit
from
its own earlier runs, and closes three gaps that made a round cost more
than it
returned.
Scripts:
- `coverage-report.sh` (new, step 6a): mechanical check of which changed
files
no ledger file has mentioned yet. Exits 1 when gaps remain and appends to
`coverage_history.tsv`. The risk scan and the merged ledger are excluded
--
neither is evidence that a file was read.
- `save-run-state.sh` (new, step 11): persists the merged ledger under a
stable
per-PR state directory (`$DORIS_REVIEW_STATE`, default
`~/.cache/doris-repo-review`) and maintains an `index.tsv` history. Runs
for
both verdicts.
- `prepare-review-context.sh`: emits `coverage_checklist.tsv`, loads earlier
runs of the same PR into `$CTX/prior_runs/`, and records `STATE_DIR` /
`PRIOR_RUNS` in `meta.env`.
SKILL.md:
- 0.4a: evidence may live outside the repository -- open the pinned jar, the
sources jar, the vendored definition, and name the artifact and its
version.
- 2.1 / 3.3a: prior runs are input, not history. Dismissals carry forward
with
their evidence; a rebased head is compared by content, not by commit hash.
- 4.1: every risk item carries a premise and a one-command premise check,
run
before dispatch. A false premise becomes a dismissal, not a subagent.
- 5.1: round 1 goes out in two waves, because the ledger is empty when it
starts and the deduplication rule cannot work against it.
- 7: a round converges on no new Blocker/Major plus clean coverage, not on
"no
new candidates at all" -- a run still yielding Minor/Nit has converged.
- 9: documents are named by reviewed head
(`pr-<N>-review.<head7>.{en,zh}.md`)
with symlinks to the newest pair; an earlier run's pair is never
overwritten.
- 13: six new traps drawn from the above.
References:
- `prompts.md`: `{TECHNIQUE}` placeholder plus section D, a catalogue of the
eight techniques that actually produce findings, so a subagent is aimed
rather than left to invent "read the diff and look for mistakes".
- `doc-templates.md`: `Builds on` header row, convergence stated separately
from the verdict, and three additions to "Coverage and Limits" (outside
evidence, inherited runs, convergence) in both languages.
README: the skill's one-line description now names the new mechanics.
Claude-Session: https://claude.ai/code/session_01GWPzxwYKXgPKrsXVSazJnr
Co-authored-by: morningman <[email protected]>
Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
---
README.md | 2 +-
skills/doris-repo-review/SKILL.md | 235 +++++++++++++++++++--
.../doris-repo-review/references/doc-templates.md | 32 ++-
skills/doris-repo-review/references/prompts.md | 76 +++++++
.../doris-repo-review/scripts/coverage-report.sh | 115 ++++++++++
.../scripts/prepare-review-context.sh | 42 ++++
skills/doris-repo-review/scripts/save-run-state.sh | 86 ++++++++
7 files changed, 562 insertions(+), 26 deletions(-)
diff --git a/README.md b/README.md
index 0ff7565..5c23ff6 100644
--- a/README.md
+++ b/README.md
@@ -34,7 +34,7 @@ skill from its `description`, so in practice you describe the
problem and the ri
| Skill | What it does | Use it when |
|---|---|---|
-| [`doris-repo-review`](skills/doris-repo-review/) | Contributor-side PR
review of a local `apache/doris` clone, running the same flow as the CI Code
Review Runner: worktree alignment, risk scan, subagent fan-out, shared ledger,
anchored EN + ZH review documents, and — on a passing review — one
machine-readable PASS comment on the PR | Reviewing an `apache/doris` pull
request locally, the way the pipeline would |
+| [`doris-repo-review`](skills/doris-repo-review/) | Contributor-side PR
review of a local `apache/doris` clone, running the same flow as the CI Code
Review Runner: worktree alignment, risk scan with premise checks, subagent
fan-out, shared ledger, mechanical coverage report, anchored EN + ZH review
documents kept per reviewed head, dismissals carried forward from earlier
reviews of the same PR, and — on a passing review — one machine-readable PASS
comment on the PR | Reviewing an `apach [...]
These skills read a Doris **source** checkout and never touch a cluster; the
cluster-side skills
never touch repository source. Nothing here builds Doris or runs tests. The
one GitHub write in
diff --git a/skills/doris-repo-review/SKILL.md
b/skills/doris-repo-review/SKILL.md
index c460e38..3a1a02f 100644
--- a/skills/doris-repo-review/SKILL.md
+++ b/skills/doris-repo-review/SKILL.md
@@ -34,16 +34,20 @@ Let `$S` be this skill's own `scripts/` directory (with a
default Claude Code in
$S/align-to-pr.sh <PR> --check # diagnosis only, changes
nothing
$S/align-to-pr.sh <PR> --out "$CTX/align.env" # step 1: align the
current directory
$S/prepare-review-context.sh --ctx "$CTX" --align "$CTX/align.env" # step 2:
gather context
+$S/coverage-report.sh --ctx "$CTX" # step 6a: what has nobody
read yet
python3 $S/verify-anchors.py --ctx "$CTX" --doc <en> --doc <zh> # step 9:
verify anchors
$S/post-pass-comment.sh --ctx "$CTX" --model <id> ... --dry-run # step
10: PASS comment
+$S/save-run-state.sh --ctx "$CTX" --verdict ... --findings b,m,mi,n # step
11: leave state for next time
```
| File | Purpose |
|---|---|
| `scripts/align-to-pr.sh` | Resolve the PR, diagnose how the current
directory relates to it, align it to the PR head |
| `scripts/prepare-review-context.sh` | Produce the authoritative diff,
new-side line ranges, required AGENTS.md list, existing comments, ledger
skeleton |
+| `scripts/coverage-report.sh` | Mechanical check of which changed files no
ledger file has mentioned yet |
| `scripts/verify-anchors.py` | Check that every `path:line` anchor really
exists and that both documents expose the same finding IDs |
| `scripts/post-pass-comment.sh` | Render and post the machine-readable PASS
comment; refuses everything that is not a pass |
+| `scripts/save-run-state.sh` | Persist this run's merged ledger under the
stable per-PR state directory, so the next review inherits its dismissals |
| `references/prompts.md` | Subagent prompt templates (CI wording, carried
over verbatim) |
| `references/doc-templates.md` | Templates for both documents, anchor format,
verdict rule |
| `references/pr-comment-format.md` | The `doris-repo-review/v1` comment
schema, field meanings, and how a program reads it back |
@@ -75,6 +79,15 @@ diff from the merge base.
4. **Confirm a path before reading it.** If a path is not already confirmed by
`pr_changed_files.txt`, `pr.diff`, or the output of an earlier successful
command, run
`rg --files` to confirm it first.
+4a. **Evidence may live outside the repository, and you are expected to go and
get it.** Rule 3
+ fixes where the *change list* comes from; it says nothing about where
*evidence* comes from.
+ When a dependency's own content is what decides the behaviour under review,
read that
+ dependency: unzip the jar in `~/.m2/repository/...` that the build actually
pins and read the
+ resource inside it, read the `-sources.jar` of the library whose semantics
the change relies on,
+ read the vendored service definition, read the JDK class whose contract a
comment claims.
+ A review that never leaves the diff cannot find a defect whose two halves
are "the changed line"
+ and "what the changed line now reaches" - which is exactly where the worst
findings live.
+ Always name the artifact and its version in the finding, so the evidence is
reproducible.
5. **Do not stop at the first blocking issue.** Work through the changed
files, the related
control flow, the tests, and the parallel or special-case paths.
6. **Every suspicion must reach a conclusion**: it becomes a finding, is
excluded as "already
@@ -157,10 +170,45 @@ Output (under `$CTX`):
| `review_focus.txt` | The user's focus points |
| `worktree_status.txt` | Uncommitted changes in the review tree (**out of
review scope**; call them out in the documents) |
| `ledger/` | Shared-ledger skeleton |
+| `coverage_checklist.tsv` | One row per changed file, for the mechanical
coverage report of step 6a |
+| `prior_runs/` | What earlier reviews of **this same PR** concluded - see
below |
If `BASE_SOURCE` is not `PR base sha (matches CI)`, the baseline differs from
CI's and the
documents must say so.
+### 2.1 Prior runs of the same PR are input, not history
+
+A PR is usually reviewed more than once - the author pushes, you re-run. Each
run used to start
+from zero, re-deriving the same dismissals and re-reading the same files, and
none of it survived.
+That is the single largest source of wasted budget, and it is why "why did the
last review not
+find this?" used to be unanswerable.
+
+`prepare-review-context.sh` therefore keeps every run's merged ledger under a
**stable per-PR
+state directory**, outside the session scratchpad and outside the repository:
+
+```
+${DORIS_REVIEW_STATE:-${XDG_CACHE_HOME:-$HOME/.cache}/doris-repo-review}/<owner>-<repo>/pr-<N>/
+ runs/<head7>/main-merged.md one per reviewed head
+ runs/<head7>/meta.env
+ index.tsv head sha, date, verdict, finding counts
+```
+
+and copies whatever it finds into `$CTX/prior_runs/`. **Read it as part of
step 3**, right after
+`pr_review_threads.md`, and treat it exactly the way you treat an existing
review thread:
+
+- Its **"Considered and Dismissed" table is the expensive part.** A dismissal
that still holds does
+ not need re-deriving; carry it forward into this run's table with its
original evidence and a
+ note that it was re-confirmed (or say why it no longer holds). Only
re-derive a dismissal whose
+ premise the new head could have changed.
+- Its accepted findings tell you what the author has already been told. One
that is now fixed is
+ worth one line in the closing report ("fixed at this head"), not a finding.
+- **If a prior run reviewed a different head, say so and check what moved.**
The branch may have
+ been rebased, so comparing by commit hash is unsafe: compare content
+ (`git show <old head>:<path>`) before claiming a finding is new.
+
+An empty `prior_runs/` means this is the first review of this PR - not that
nothing was ever
+reviewed. Say which it is in "Coverage and Limits".
+
---
## 3. Required reading (in this order)
@@ -176,6 +224,10 @@ Before looking at any code, the main agent reads, in order:
raise** the same or a substantially similar issue again, even phrased
differently. Raise a
similar concern only when this PR introduces a genuinely different instance
somewhere else that
the existing comments do not cover, and say why it is distinct.
+3a. Everything under `$CTX/prior_runs/` - what earlier reviews of this same PR
concluded. See
+ section 2.1: dismissals carry forward with their evidence, accepted
findings that are now fixed
+ become one line in the closing report, and a prior run on a different head
means comparing
+ content rather than commit hashes.
4. `$CTX/review_focus.txt` - do the full review as usual and pay extra
attention to these points;
the final documents must respond to each one, even if the conclusion is "no
additional issue
found for this point".
@@ -193,12 +245,41 @@ points look risky to you?
Write the result into `$CTX/ledger/00-main-risk-scan.md`, each entry carrying:
an ID, the changed
files/lines involved, the related mechanism to inspect, why it is suspicious,
the upstream or
-downstream files that must be read alongside it, and the **specific question**
a risk-focused
-subagent has to answer.
+downstream files that must be read alongside it, the **specific question** a
risk-focused
+subagent has to answer, and - see below - a **premise check**.
An empty risk scan means you have not understood the PR yet - go back and read
it; do not skip
this step.
+### 4.1 Every risk item carries a premise check, and you run it before
spawning anything
+
+A risk item always rests on a premise: "this used to be X and is now Y",
"nothing validates Z",
+"only one caller does W". If the premise is false the subagent spends its
entire budget proving
+you wrong, and you learn nothing you could not have learned in thirty seconds.
+
+So each entry gets one more field:
+
+```
+ Premise: <the one factual claim the whole item rests on>
+ Premise check: <a single command that confirms or kills it>
+ Premise result: <confirmed | FALSE - item dismissed | cannot be checked
cheaply>
+```
+
+Run every one of them **before** step 5. The check is nearly always a
one-liner against the base:
+
+```bash
+git show "$BASE_SHA:path/to/File.java" | grep -n 'thing I think is new'
+git show "$BASE_SHA:path/to/File.java" | sed -n '120,140p'
+rg -n 'symbol' --files-with-matches # "only one caller" claims
+```
+
+A premise that comes back FALSE does not become a subagent - it becomes a row
in "Considered and
+Dismissed" with the command as its evidence. That row is worth as much as a
finding: it is the
+part of the review that says "this was checked", and it costs one command
instead of one agent.
+
+A premise you genuinely cannot check cheaply is fine - dispatch it, and say in
the prompt that the
+premise is unverified so the subagent checks it first and stops early if it
fails.
+
---
## 5. Spawn the subagents
@@ -219,13 +300,41 @@ see the bug. Give every subagent its own ledger file,
`$CTX/ledger/sub-<round>-<
Take the prompts from `references/prompts.md`: section A (shared preamble)
plus section B
(full review) or section C (risk-focused), substituting `{CTX}` /
`{REPO_ROOT}` (= `$WORKDIR`) /
`{BASE_SHA}` / `{HEAD_SHA}` / `{ROUND}` / `{AGENT_ID}` / `{FOCUS}`. **Every
subagent prompt must
-state the ledger directory and the path of that subagent's own file.**
+state the ledger directory and the path of that subagent's own file.** Section
D of that file is a
+catalogue of the techniques that actually find things - name the one you want
in each prompt
+rather than leaving the subagent to invent a method.
> **One implementation difference from CI**: CI uses a single
> `subagent_review_findings.md` with
> sections; locally, concurrent writes to one file collide on patch, so this
> becomes **one file per
> owner** under `ledger/`. The semantics are unchanged - a single shared
> source of truth, everyone
> reads all of it, each writes only their own, the main agent merges.
+### 5.1 Round 1 goes out in two waves, because the ledger is empty when it
starts
+
+The deduplication rule ("read every file in `ledger/` before reviewing")
cannot work in round 1:
+every subagent starts at the same instant against an empty directory. In
practice three agents
+independently rediscover the same defect and three budgets buy one finding.
+
+So round 1 - and only round 1 - is dispatched in two waves:
+
+- **Wave A: the full-review subagents.** All of them concurrently, as before.
They own the slices,
+ so their coverage is what the union has to span.
+- **Wave B: the risk-focused subagents.** Dispatched once wave A returns,
concurrently among
+ themselves. They read wave A's ledger files first, so a mechanism wave A
already settled becomes
+ a duplicate note instead of a second investigation, and a risk item wave A
has already answered
+ is dropped before it costs anything.
+
+Two things make this cheap rather than slow. Wave A is the long pole either
way. And the main
+agent is not idle in between - it merges wave A (step 6) while wave B runs.
+
+Where two waves genuinely will not fit - a small PR, or a risk item so
specific that no
+full-review slice touches it - dispatch everything at once and **pre-seed the
ledger instead**:
+before spawning, write into `00-main-risk-scan.md` the defects you already
expect each slice to
+surface, so a concurrent agent can recognise one of yours and mark it
duplicate rather than
+writing it up from scratch.
+
+From round 2 onward the ledger is populated, so all subagents of a round go
out together.
+
---
## 6. Main agent merges, verifies, deduplicates
@@ -245,19 +354,53 @@ candidate into `$CTX/ledger/main-merged.md`:
When this step ends, **no candidate may be left without a status**.
+### 6a. Run the coverage report at the end of every round
+
+```bash
+$S/coverage-report.sh --ctx "$CTX"
+```
+
+It is mechanical, not a judgement: it walks `pr_changed_files.txt` and reports
which changed files
+no ledger file has so much as mentioned. Finding "13 changed files nobody
opened" is a job for
+`grep`, not for a subagent in round 3 - and knowing it after round 1 is what
lets round 2 be
+aimed instead of guessed.
+
+Its output is an input to the next round's slicing, and to the verdict: **a
file nobody read is
+not a file with no findings.** Either cover it in the next round or read it
yourself in step 8.
+
---
## 7. Convergence loop (at most 3 rounds)
-One round = step 5 + step 6. Record the outcome in the `Convergence Rounds`
table of
-`main-merged.md`.
+One round = step 5 + step 6 + step 6a. Record the outcome in the `Convergence
Rounds` table of
+`main-merged.md`, one row per round with: subagents, new candidates **by
severity**, coverage gaps
+remaining, verdict.
+
+**Convergence is about the verdict, not about the count.** The rounds exist to
make you confident
+in the answer you are about to give - which findings block the merge - not to
empty the well of
+Nits. A large PR will yield another Minor for as long as you keep looking, and
a rule that waits
+for that to stop combined with the re-slicing rule below can never terminate.
+
+So a round **converges** when both hold:
+
+- it produced **no new `Blocker` and no new `Major`**, and
+- the step-6a coverage report is clean - every changed file has been read by
somebody.
+
+Then go to step 8, and record in the documents that the verdict has been
stable since round N.
-- Every subagent returned `NO_NEW_VALUABLE_FINDINGS` → go to step 8.
-- New valuable candidates remain → start another round: **re-slice the
coverage** based on what
- this round taught you (do not re-dispatch the same split unchanged), and add
risk-focused
- subagents for any newly suspicious mechanism.
-- **Cap of 3 rounds.** If the cap is reached and new candidates still appear,
finish normally but
- state in the verdict and in "Coverage and Limits" that **this review did not
converge**.
+Otherwise start another round: **re-slice the coverage** based on what this
round taught you (do
+not re-dispatch the same split unchanged), and add risk-focused subagents for
any newly suspicious
+mechanism. Aim the new round at where severity actually came from, not at what
is left over.
+
+- **Cap of 3 rounds.** If the cap is reached while `Blocker`/`Major`
candidates are still
+ appearing, or while coverage gaps remain, finish normally but state in the
verdict and in
+ "Coverage and Limits" that **this review did not converge**, and say which
of the two conditions
+ failed.
+- If the cap is reached with only `Minor`/`Nit` still trickling in, that is
**converged**, and the
+ documents should say so plainly: "the verdict was settled at round N; later
rounds added only
+ Minor and Nit findings". Do not report a converged review as a failed one.
+- **A round that returns only Nits is a stop signal, not a reason for another
round.** Record the
+ yield (agents spent, findings by severity) so the next person can see where
the returns fell off.
---
@@ -266,8 +409,11 @@ One round = step 5 + step 6. Record the outcome in the
`Convergence Rounds` tabl
Before writing the documents, walk explicitly through the changed-file list
and the open-candidate
list:
-- Was every changed file covered by at least one subagent? Cover the rest
yourself.
+- Run `$S/coverage-report.sh --ctx "$CTX"` one last time. Was every changed
file covered by at
+ least one subagent? Cover the rest yourself, and say in "Coverage and
Limits" which files you
+ read only here.
- Does every suspicion have a conclusion?
+- Did every dismissal carried forward from `prior_runs/` get re-confirmed or
re-opened?
- Does every applicable item in Part 1.3 of the code-review skill have an
explicit conclusion?
- Is there anywhere you are still unsure about, or that may not have been
investigated deeply
enough? Investigate it now.
@@ -278,13 +424,25 @@ Only after the sweep may you write the documents.
## 9. Produce the two documents
-Write them into `review-docs/` in the **current directory** (`mkdir -p` it if
needed):
+Write them into `review-docs/` in the **current directory** (`mkdir -p` it if
needed), named by the
+head that was actually reviewed:
```
-review-docs/pr-<N>-review.en.md
-review-docs/pr-<N>-review.zh.md
+review-docs/pr-<N>-review.<head7>.en.md e.g. pr-66770-review.3f45815.en.md
+review-docs/pr-<N>-review.<head7>.zh.md
+review-docs/pr-<N>-review.en.md symlink -> the newest of the above
+review-docs/pr-<N>-review.zh.md symlink -> the newest of the above
```
+**Never overwrite an earlier run's document.** The head sha is in the name
precisely so a re-review
+cannot destroy what the last one concluded: that record is the only way to
answer "was this
+considered last time, and dismissed with what evidence?" - and once it is
gone, it is gone, because
+`review-docs/` is untracked. The two unsuffixed names are convenience
symlinks, so anything that
+links to them keeps working while the history accumulates behind them.
+
+Point `verify-anchors.py` at the real files, not the symlinks. If a document
for this exact head
+already exists, you are re-running against an unchanged head: overwrite that
one, and only that one.
+
`references/doc-templates.md` holds the templates, the anchor format, and the
verdict rule
(`Blocker`/`Major` → REQUEST_CHANGES; only `Minor`/`Nit` → APPROVE). The
essentials:
@@ -302,8 +460,11 @@ review-docs/pr-<N>-review.zh.md
- "Response to Review Focus" answers each of the user's focus points.
- "Considered and Dismissed" lists every excluded suspicion together with its
evidence.
- "Coverage and Limits" states: what was read in depth versus skimmed, how the
subagents were
- split, what was not verified locally (builds, tests, anything needing a real
cluster), and the
- uncommitted changes from `worktree_status.txt` that were excluded.
+ split, what was not verified locally (builds, tests, anything needing a real
cluster), the
+ uncommitted changes from `worktree_status.txt` that were excluded, **which
earlier runs of this
+ PR this one builds on** (heads and dates from `prior_runs/`, or "first
review of this PR"), and
+ **whether the review converged** in the sense of step 7 - naming the round
after which the
+ verdict stopped moving, and, if it did not converge, which of the two
conditions failed.
When they are written, running the verifier is **mandatory**; if it fails, fix
the documents and
re-run until it passes:
@@ -368,7 +529,12 @@ $S/post-pass-comment.sh --ctx "$CTX" \
Tell the user:
1. The paths of both documents, the verdict (REQUEST_CHANGES / APPROVE), the
finding count per
- severity, and whether the rounds converged.
+ severity, and whether the rounds converged in the sense of step 7 - "the
verdict was settled at
+ round N" if they did, and which condition failed if they did not. A run
that ended with only
+ Minor/Nit still arriving **converged**; do not report it as a failure.
+1a. **What this run inherited**, when `prior_runs/` was not empty: which heads
were reviewed
+ before, how many of their dismissals were carried forward, and which of
their accepted findings
+ are fixed at this head. If no prior run existed, say that this is the first
review of this PR.
2. The `branch check` / `commit check` results - especially `ahead:N`
(unpushed commits that were
not reviewed).
3. **What happened to the PASS comment**: the URL when one was posted or
updated, or the reason
@@ -378,6 +544,19 @@ Tell the user:
may still want to read the code.
5. `review-docs/` **is not gitignored in the doris repository**, so **do not
commit it
automatically**; leave that to the user.
+6. **Save the run state before you finish** - this is what makes the next
review cheaper and makes
+ this one auditable:
+
+ ```bash
+ $S/save-run-state.sh --ctx "$CTX" \
+ --verdict REQUEST_CHANGES --findings <blocker>,<major>,<minor>,<nit> \
+ --rounds <r> --converged <true|false> \
+ --docs "review-docs/pr-<N>-review.<head7>.en.md" --note "<one line: the
headline finding>"
+ ```
+
+ It runs for **both** verdicts - a REQUEST_CHANGES review has just as much
to hand forward as a
+ pass, and rather more. Tell the user where it was saved and print the
per-PR history it echoes,
+ so the sequence of reviews of this PR is visible in one place.
---
@@ -393,7 +572,8 @@ Tell the user:
| Fetch existing inline threads (30 threads / 1200 chars) | Same jq |
| Text after `/review` = review focus | Free text after the PR URL → `--focus`
|
| Single-file ledger with sections | A `ledger/` directory, one file per owner
|
-| Main risk scan → 1-3 full-review subagents + risk-focused → merge → ≤3
rounds | Identical |
+| Main risk scan → 1-3 full-review subagents + risk-focused → merge → ≤3
rounds | Same shape, three local additions: every risk item carries a premise
check the main agent runs before dispatch (4.1); round 1 goes out in two waves
so the ledger can deduplicate (5.1); a round converges on **severity plus
coverage**, not on "no new candidates at all" (7) |
+| CI reviews one push in isolation | Earlier runs of the same PR are loaded
from a stable state directory and read as input (2.1); documents are named by
head sha and never overwritten (9) |
| `gh pr review` / Reviews API posting inline comments | **Two `review-docs/`
documents (EN + ZH) with `path:line` anchors** |
| CI's review verdict is visible on the PR itself | On a pass, one
`doris-repo-review/v1` comment from the local account (commit sha, timestamp,
model, findings, notes); on REQUEST_CHANGES, nothing - the documents stay local
|
| 60-minute timeout | No hard timeout, but likewise do not let one round turn
into unbounded digging |
@@ -402,6 +582,23 @@ Tell the user:
## 13. Common traps
+- **Treating "another Minor appeared" as non-convergence.** A PR of any size
yields another Minor
+ for as long as you keep looking. What has to stop moving is the *verdict* -
see step 7. Two runs
+ in a row reporting "did not converge" while the blocking findings were
settled in round 1 is a
+ broken criterion, not a deep PR.
+- **Dispatching a subagent on a premise you never checked.** The premise check
of step 4.1 costs
+ one command; skipping it costs a whole agent, and the agent comes back
having proved you wrong
+ rather than having reviewed anything.
+- **Believing the round-1 ledger deduplicates anything.** It is empty when
round 1 starts. Use the
+ two waves of step 5.1, or pre-seed it.
+- **Overwriting the previous run's documents.** They are untracked, so an
overwrite is permanent,
+ and it destroys the only record of what was already considered and
dismissed. Write
+ `pr-<N>-review.<head7>.*` and let the symlinks move.
+- **Comparing runs by commit hash after a rebase.** A re-reviewed branch is
often rebased, so
+ `git log old..new` lists the whole PR again and tells you nothing. Compare
content with
+ `git show <old head>:<path>` before claiming a finding is newly introduced.
+- **Staying inside the diff when the evidence is in a dependency.** See ground
rule 4a. If a
+ finding's mechanism ends in "…and the library does X", open the library.
- **The local checkout is ahead of the PR head.** `commit check: ahead:N`
means there are unpushed
commits and **they are not part of the review**. This is the easiest thing
for a reader to
misread, so state it both in the document header and in the closing report.
diff --git a/skills/doris-repo-review/references/doc-templates.md
b/skills/doris-repo-review/references/doc-templates.md
index c1a1c1d..17b0c0f 100644
--- a/skills/doris-repo-review/references/doc-templates.md
+++ b/skills/doris-repo-review/references/doc-templates.md
@@ -1,11 +1,16 @@
# Review document templates
-Two documents, same content, same finding IDs, same order:
+Two documents, same content, same finding IDs, same order, named by the head
that was reviewed:
-- `review-docs/pr-<N>-review.en.md`
-- `review-docs/pr-<N>-review.zh.md`
+- `review-docs/pr-<N>-review.<head7>.en.md`
+- `review-docs/pr-<N>-review.<head7>.zh.md`
+- `review-docs/pr-<N>-review.en.md` / `.zh.md` — symlinks to the newest pair
-When the branch has no PR, use `review-docs/<branch-slug>-review.en.md` /
`.zh.md`.
+When the branch has no PR, use
`review-docs/<branch-slug>-review.<head7>.en.md` / `.zh.md`.
+
+**Never overwrite an earlier run's pair.** `review-docs/` is untracked, so
overwriting is
+permanent, and what it destroys is the only record of what an earlier review
already considered
+and dismissed. Re-running against the *same* head overwrites that head's pair,
and only that one.
The ZH document is a real Chinese review, not a machine translation of the EN
one: same facts,
same anchors, same IDs, but idiomatic Chinese. Identifiers, file paths, log
messages, config
@@ -33,6 +38,13 @@ Local review has no inline comments, so an anchor is the
only way a reader finds
| `Blocker` or `Major` | `REQUEST_CHANGES` |
| only `Minor` / `Nit` / none | `APPROVE` |
+The verdict is about severity; **convergence is a separate statement and must
not be confused with
+it.** A round converges when it produced no new `Blocker`/`Major` *and* the
coverage report is
+clean (SKILL.md step 7). A run that ended with Minor and Nit findings still
arriving **converged**
+— report it as such, and name the round after which the verdict stopped
moving. Only report "did
+not converge" when blocking findings were still appearing at the cap, or when
changed files
+remained unread; say which of the two it was.
+
Severity meanings: `Blocker` = correctness, data loss, deadlock, crash,
incompatible change
without a compat path, or a broken build/test contract. `Major` = real defect
or a missing
guarantee that will bite in production or during upgrade. `Minor` = worth
fixing, not urgent.
@@ -54,7 +66,8 @@ guarantee that will bite in production or during upgrade.
`Minor` = worth fixing
| Reviewed in | `<WORKDIR>` (<ALIGN_MODE>), on <YYYY-MM-DD> |
| Local alignment | branch check: <…>; commit check: <same / ahead:N /
behind:N / diverged:a/b> |
| Verdict | **REQUEST_CHANGES** / **APPROVE** |
-| Rounds | <r> of max 3, converged / not converged |
+| Rounds | <r> of max 3; converged (verdict stable since round <n>) / did not
converge (<which condition failed>) |
+| Builds on | <prior heads reviewed, from prior_runs/, or "first review of
this PR"> |
<!-- Only when commit check was ahead:N before aligning: -->
> `<N>` local commit(s) on `<branch>` are ahead of the PR head and are **not**
> covered by
@@ -134,6 +147,9 @@ out. This is what makes the review auditable — do not
silently drop a concern.
- Files skimmed and why: <...>
- Subagents: <round/id/coverage table>
- Not verified locally: <builds, tests, and anything that needs a running
cluster>
+- Evidence taken from outside the repository: <dependency artifacts opened,
with versions, or "none">
+- Inherited from earlier runs: <heads, and how many dismissals were carried
forward — or "first review of this PR">
+- Convergence: <converged, verdict stable since round N / did not converge
because <condition>>
- Uncommitted worktree paths excluded from this review: <from
worktree_status.txt, or "none">
```
@@ -153,7 +169,8 @@ out. This is what makes the review auditable — do not
silently drop a concern.
| 评审位置 | `<WORKDIR>`（<ALIGN_MODE>），<YYYY-MM-DD> |
| 本地一致性 | 分支：<…>；commit：<一致 / 领先 N / 落后 N / 已分叉 a/b> |
| 结论 | **REQUEST_CHANGES** / **APPROVE** |
-| 轮次 | 共 <r> 轮（上限 3），已收敛 / 未收敛 |
+| 轮次 | 共 <r> 轮（上限 3）；已收敛（结论自第 <n> 轮起稳定）/ 未收敛（哪个条件没满足） |
+| 承接自 | <prior_runs/ 里更早评审过的 head，若没有则写"本 PR 的首次评审"> |
<!-- 仅当对齐前 commit check 是领先 N 时保留： -->
> `<branch>` 上有 `<N>` 个本地提交领先于 PR head，**不在**本次评审范围内。
@@ -223,5 +240,8 @@ out. This is what makes the review auditable — do not
silently drop a concern.
- 只扫了一眼的文件及原因：<...>
- 子 agent 分工：<轮次 / id / 覆盖面 表格>
- 本地未验证：<编译、用例，以及任何需要真实集群才能确认的部分>
+- 仓库之外取到的证据：<打开过的依赖产物及版本，没有就写“无”>
+- 承接自更早的评审：<更早的 head，以及沿用了多少条已排除结论——首次评审就写“本 PR 的首次评审”>
+- 收敛情况：<已收敛，结论自第 N 轮起稳定 / 未收敛，因为 <哪个条件>>
- 未纳入本次评审的未提交改动：<取自 worktree_status.txt，没有就写“无”>
```
diff --git a/skills/doris-repo-review/references/prompts.md
b/skills/doris-repo-review/references/prompts.md
index 4c15f6a..d3ae4b8 100644
--- a/skills/doris-repo-review/references/prompts.md
+++ b/skills/doris-repo-review/references/prompts.md
@@ -15,6 +15,7 @@ Substitute before use:
| `{ROUND}` | 1-based round number |
| `{AGENT_ID}` | short slug, e.g. `r1-fe-spi` |
| `{FOCUS}` | this subagent's assigned coverage |
+| `{TECHNIQUE}` | the section-D technique this agent must apply, named and
aimed |
---
@@ -36,6 +37,8 @@ Authoritative PR context (do not obtain the diff or the
changed-path list any ot
- User review focus : {CTX}/review_focus.txt
- Required AGENTS.md files : {CTX}/required_agents.txt
- Shared review ledger directory : {CTX}/ledger/
+- Earlier reviews of this PR : {CTX}/prior_runs/ (may be absent - then
this is the first)
+- Coverage checklist : {CTX}/coverage_checklist.tsv
- Diff range : {BASE_SHA}...{HEAD_SHA} (three-dot, from
the merge base)
These were generated with `git diff {BASE_SHA}...{HEAD_SHA}` in this worktree.
The base SHA
@@ -75,6 +78,19 @@ Line-number rule (this run has no GitHub inline comments, so
anchors are the onl
Evidence rule: for any claimed error you must give the concrete path or logic
where it occurs.
"If A then B" is only acceptable when you name a concrete scenario in which A
actually happens.
+Evidence may live outside the repository, and you are expected to go and get
it. The change list
+comes only from the files above - but evidence does not. When a dependency's
own content is what
+decides the behaviour under review, open that dependency: unzip the jar under
`~/.m2/repository`
+that the build actually pins and read the resource inside it, read its
`-sources.jar`, read the
+vendored service definition, read the JDK class whose contract a comment
claims. Name the artifact
+and its version in the finding so the evidence is reproducible. A review that
never leaves the diff
+cannot find a defect whose two halves are "the changed line" and "what the
changed line now
+reaches".
+
+If `{CTX}/prior_runs/` exists it holds what earlier reviews of this same PR
concluded. Read it with
+`pr_review_threads.md`: a dismissal recorded there with evidence does not need
re-deriving, and an
+accepted finding recorded there is something the author has already been told.
+
Do NOT stop after finding the first blocking issue. Keep reviewing changed
files, related
control flow, tests, and parallel or special-case paths until all plausible
correctness,
lifecycle, configuration, compatibility, performance, and coverage bugs have
been investigated
@@ -109,6 +125,10 @@ coverage. For optimizer/Nereids changes, follow the
plan-tree output style in Pa
Read the actual surrounding code, not only the diff hunks: a change is only
correct with
respect to its real call chain, its concurrency, and its lifecycle.
+
+Technique to apply: {TECHNIQUE}
+See section D of this file. Use it deliberately rather than falling back on
"read the diff and
+look for mistakes" - that finds what a careful author already found.
```
---
@@ -139,4 +159,60 @@ until you can answer the question with concrete code
evidence. Then record in yo
Report a candidate finding only when the evidence is concrete. "Not a bug" is
a valid and
useful answer, but it must come with the code evidence that rules the concern
out — the main
agent records that evidence as a dismissal.
+
+Technique to apply: {TECHNIQUE}
+
+Premise: {PREMISE} — status: {PREMISE_RESULT}
+If that status is "cannot be checked cheaply", check the premise FIRST and
stop early if it is
+false: say so, with the command that shows it, and do not spend the rest of
your budget.
```
+
+---
+
+## D. Technique catalogue (name one in every prompt)
+
+A subagent told only "review your slice" invents a method, and the method it
invents is usually
+"read the diff and look for mistakes" - which finds what a careful author
already found. The
+techniques below are the ones that actually produced findings. Name the one
you want, and say what
+it should be applied to.
+
+**D1. Differential against the base.** *Do not read the diff. Reconstruct both
sides.* Extract the
+pre-change files with `git show {BASE_SHA}:<path>`, build the old and the new
behaviour tables
+yourself - one row per (input, condition) the code distinguishes - and list
every cell that
+differs. Then classify each differing cell as intended (name the commit that
says so) or as a
+regression. This is what catches a defect whose changed line is *correct*: the
line is right, the
+comment explaining it is right, and the consequence two modules away is wrong.
+
+**D2. What did this switch turn on?** When a change fixes something that was
silently not working,
+the path it revives has never been exercised. Ask: what else is on that path,
what has never run,
+what does the newly-live path now hand to a consumer that was never built to
receive it?
+
+**D3. Contract-boundary and dialect check.** At every boundary where one
component hands text,
+names, or payloads to another - SPI, plugin, RPC, SQL text, config - ask who
owns the format, who
+validates it, and what happens to a value that parses but means something
different on the other
+side. Silent widening lives here.
+
+**D4. Doc versus code.** Read the PR body, the release note, the module
README, the AGENTS.md
+obligations and the javadoc as *claims*, and check each against the code. A
claim that the code
+contradicts is a finding even when the code is right, because the claim is
what the next person
+acts on. Machine-checked obligations that a comment says are review-only are
your job by definition.
+
+**D5. Parallel paths.** For every fix, find its siblings: the other call sites
of the same shape,
+the other plugin family, the other branch of the same method, the other error
code with the same
+arity. A fix applied to one of N identical sites is N-1 findings.
+
+**D6. Failure-mode enumeration.** For each new interface method, enumerate
`{null, empty,
+exception, wrong type}` as the answer and trace what the engine does with each
on every consuming
+path. State for each whether the outcome is a correct refusal, a hard error,
or a silent grant.
+
+**D7. Coverage-of-the-gate.** For every test that is claimed to be a gate, ask
what change would
+leave it green. A frozen baseline that records less than the obligation it
enforces, an assertion
+that restates what the code computes, a mock that stubs out the very default
under test, a negative
+control that is not negative.
+
+**D8. Completeness critic (round 3, or whenever a round disappoints).** Not a
slice. Build the
+coverage map from the ledger, list the changed files nobody demonstrably
opened, read them. Then
+find the two or three most load-bearing "not a bug" conclusions in the ledger
and re-derive them
+from primary sources. Then ask which *kinds* of check were never run at all -
persistence and
+replay, upgrade *and downgrade*, CI gates, licence obligations, the thrift
surface - and run the
+ones that apply.
diff --git a/skills/doris-repo-review/scripts/coverage-report.sh
b/skills/doris-repo-review/scripts/coverage-report.sh
new file mode 100755
index 0000000..f68f396
--- /dev/null
+++ b/skills/doris-repo-review/scripts/coverage-report.sh
@@ -0,0 +1,115 @@
+#!/usr/bin/env bash
+# Mechanical coverage report for the `doris-repo-review` skill (step 6a).
+#
+# Answers one question, with grep rather than judgement: which of this PR's
+# changed files has no ledger file so much as mentioned yet?
+#
+# A file nobody read is not a file with no findings. Running this after every
+# round is what lets the next round be aimed instead of guessed, and what keeps
+# "13 changed files nobody opened" from being a round-3 discovery.
+#
+# Usage:
+# coverage-report.sh --ctx <dir> [--round <n>] [--quiet]
+#
+# Reads : <ctx>/pr_changed_files.txt, <ctx>/ledger/*.md
+# Writes : <ctx>/coverage_report.txt (and appends one line to
<ctx>/coverage_history.tsv)
+# Exit : 0 when every changed file is mentioned, 1 when some are not.
+set -euo pipefail
+
+CTX=""
+ROUND=""
+QUIET=0
+while [ $# -gt 0 ]; do
+ case "$1" in
+ --ctx) CTX="$2"; shift 2 ;;
+ --round) ROUND="$2"; shift 2 ;;
+ --quiet) QUIET=1; shift ;;
+ -h|--help)
+ sed -n '2,18p' "$0" | sed 's/^# \{0,1\}//'
+ exit 0 ;;
+ *) echo "unknown argument: $1" >&2; exit 2 ;;
+ esac
+done
+
+[ -n "$CTX" ] || { echo "ERROR: --ctx is required" >&2; exit 2; }
+[ -d "$CTX" ] || { echo "ERROR: no such context directory: $CTX" >&2; exit 2; }
+CHANGED="$CTX/pr_changed_files.txt"
+[ -f "$CHANGED" ] || { echo "ERROR: $CHANGED not found - run
prepare-review-context.sh first" >&2; exit 2; }
+
+python3 - "$CTX" "${ROUND:-}" "$QUIET" <<'PY'
+import os, sys, glob, datetime
+
+ctx, round_label, quiet = sys.argv[1], sys.argv[2], sys.argv[3] == "1"
+changed = [l.strip() for l in open(os.path.join(ctx, "pr_changed_files.txt"))
if l.strip()]
+
+ledger_files = sorted(
+ p for p in glob.glob(os.path.join(ctx, "ledger", "*.md"))
+ # main-merged is the main agent's own summary; the risk scan is written
+ # before anyone reads anything. Neither is evidence that a file was read.
+ if os.path.basename(p) not in ("README.md", "main-merged.md",
"00-main-risk-scan.md")
+)
+
+blobs = {}
+for p in ledger_files:
+ try:
+ blobs[os.path.basename(p)] = open(p, errors="replace").read()
+ except OSError:
+ pass
+
+def mentions(path, text):
+ """A file counts as mentioned when its full path or its basename
appears."""
+ if path in text:
+ return True
+ base = os.path.basename(path)
+ # A bare basename is weak evidence for a common name (pom.xml, README.md),
+ # so require the full path for those.
+ return len(base) > 12 and base in text
+
+unread, by_owner = [], {}
+for path in changed:
+ owners = [name for name, text in blobs.items() if mentions(path, text)]
+ if owners:
+ for o in owners:
+ by_owner.setdefault(o, 0)
+ by_owner[o] += 1
+ else:
+ unread.append(path)
+
+lines = []
+lines.append("=== coverage report ===")
+lines.append(f"round : {round_label or '(unspecified)'}")
+lines.append(f"changed files : {len(changed)}")
+lines.append(f"mentioned : {len(changed) - len(unread)}")
+lines.append(f"NOT mentioned : {len(unread)}")
+lines.append(f"ledger files : {len(blobs)}")
+if by_owner:
+ lines.append("")
+ lines.append("files mentioned per ledger owner:")
+ for name in sorted(by_owner):
+ lines.append(f" {by_owner[name]:4d} {name}")
+if unread:
+ lines.append("")
+ lines.append("changed files no ledger file mentions yet:")
+ lines.extend(" " + p for p in unread)
+ lines.append("")
+ lines.append("These are not files with no findings - they are files nobody
has read.")
+ lines.append("Cover them in the next round's slicing, or read them
yourself in step 8.")
+else:
+ lines.append("")
+ lines.append("Every changed file is mentioned by at least one ledger
file.")
+
+report = "\n".join(lines) + "\n"
+open(os.path.join(ctx, "coverage_report.txt"), "w").write(report)
+
+hist = os.path.join(ctx, "coverage_history.tsv")
+if not os.path.exists(hist):
+ open(hist, "w").write("date\tround\tchanged\tmentioned\tunread\n")
+with open(hist, "a") as f:
+ f.write("%s\t%s\t%d\t%d\t%d\n" % (
+ datetime.date.today().isoformat(), round_label or "-",
+ len(changed), len(changed) - len(unread), len(unread)))
+
+if not quiet:
+ sys.stdout.write(report)
+sys.exit(1 if unread else 0)
+PY
diff --git a/skills/doris-repo-review/scripts/prepare-review-context.sh
b/skills/doris-repo-review/scripts/prepare-review-context.sh
index e37d764..b75ff36 100755
--- a/skills/doris-repo-review/scripts/prepare-review-context.sh
+++ b/skills/doris-repo-review/scripts/prepare-review-context.sh
@@ -166,6 +166,12 @@ GIT_DIFF=(git -c diff.renameLimit=32768 diff --no-ext-diff
--no-color)
"${GIT_DIFF[@]}" "$DIFF_RANGE" >
"$CTX/pr.diff"
"${GIT_DIFF[@]}" --name-only "$DIFF_RANGE" >
"$CTX/pr_changed_files.txt"
+# One row per changed file, for the mechanical coverage report of step 6a.
+# A file nobody read is not a file with no findings.
+{
+ printf 'path\tstatus\n'
+ "${GIT_DIFF[@]}" --name-status "$DIFF_RANGE" | awk -F'\t' 'NF>=2 {print
$NF "\t" $1}'
+} > "$CTX/coverage_checklist.tsv"
"${GIT_DIFF[@]}" --name-status -M "$DIFF_RANGE" >
"$CTX/pr_changed_files_status.txt"
"${GIT_DIFF[@]}" --stat "$DIFF_RANGE" >
"$CTX/pr_diffstat.txt"
git log --oneline --no-decorate "${MERGE_BASE}..${HEAD_SHA}" >
"$CTX/pr_commits.txt"
@@ -399,6 +405,32 @@ fi
# ------------------------------------------------------------------ meta +
report
[ -n "$DOCS_ROOT" ] || DOCS_ROOT="$REPO_ROOT"
+# --------------------------------------------------- prior runs of this same
PR
+# A PR is normally reviewed more than once. Each run used to start from zero
and
+# leave nothing behind, so the expensive half of a review - the dismissals,
with
+# their evidence - was re-derived every time, and "was this considered last
+# time?" was unanswerable. Runs are kept under a stable per-PR directory that
is
+# outside both the session scratchpad and the git worktree.
+STATE_ROOT="${DORIS_REVIEW_STATE:-${XDG_CACHE_HOME:-$HOME/.cache}/doris-repo-review}"
+STATE_DIR=""
+PRIOR_RUNS=0
+if [ -n "${PR_NUMBER:-}" ]; then
+ STATE_DIR="${STATE_ROOT}/$(printf '%s' "${UPSTREAM_REPO}" | tr '/'
'-')/pr-${PR_NUMBER}"
+ mkdir -p "$STATE_DIR/runs"
+ mkdir -p "$CTX/prior_runs"
+ for run in "$STATE_DIR"/runs/*/; do
+ [ -d "$run" ] || continue
+ head7="$(basename "$run")"
+ # The run for this very head is this run, not a prior one.
+ [ "$head7" = "${HEAD_SHA:0:7}" ] && continue
+ [ -f "$run/main-merged.md" ] || continue
+ cp "$run/main-merged.md" "$CTX/prior_runs/${head7}-main-merged.md"
+ [ -f "$run/meta.env" ] && cp "$run/meta.env"
"$CTX/prior_runs/${head7}-meta.env"
+ PRIOR_RUNS=$((PRIOR_RUNS + 1))
+ done
+ [ -f "$STATE_DIR/index.tsv" ] && cp "$STATE_DIR/index.tsv"
"$CTX/prior_runs/index.tsv"
+fi
+
{
echo "REPO_ROOT=$REPO_ROOT"
echo "DOCS_ROOT=$DOCS_ROOT"
@@ -417,6 +449,8 @@ fi
echo "MERGE_BASE=$MERGE_BASE"
echo "DIFF_RANGE=$DIFF_RANGE"
echo "DIRTY_FILES=$DIRTY_COUNT"
+ echo "STATE_DIR=${STATE_DIR:-}"
+ echo "PRIOR_RUNS=${PRIOR_RUNS:-0}"
echo "REVIEW_DATE=$(date +%Y-%m-%d)"
} > "$CTX/meta.env"
@@ -428,6 +462,14 @@ echo "commits : $(grep -c . "$CTX/pr_commits.txt" ||
true)"
echo "diff lines : $(wc -l < "$CTX/pr.diff" | tr -d ' ')"
echo "diffstat : $(tail -n 1 "$CTX/pr_diffstat.txt")"
echo
+if [ "${PRIOR_RUNS:-0}" -gt 0 ]; then
+ echo "prior runs of this PR (read them, see SKILL.md 2.1): $PRIOR_RUNS"
+ ls -1 "$CTX/prior_runs" | sed 's/^/ /'
+ echo
+else
+ echo "prior runs of this PR: none - this is the first review"
+ echo
+fi
echo "required AGENTS.md:"
sed 's/^/ /' "$CTX/required_agents.txt"
if [ "${DIRTY_COUNT:-0}" -gt 0 ]; then
diff --git a/skills/doris-repo-review/scripts/save-run-state.sh
b/skills/doris-repo-review/scripts/save-run-state.sh
new file mode 100755
index 0000000..d9294e8
--- /dev/null
+++ b/skills/doris-repo-review/scripts/save-run-state.sh
@@ -0,0 +1,86 @@
+#!/usr/bin/env bash
+# Persist what this run concluded, so the next review of the same PR starts
from
+# it instead of from zero (`doris-repo-review` skill, step 11).
+#
+# What gets kept is the merged ledger - and the expensive part of that is the
+# "Considered and Dismissed" set, with its evidence. A later run reads it back
+# through <ctx>/prior_runs/ and carries the dismissals forward rather than
+# re-deriving them, which is also what makes "was this considered last time?"
+# an answerable question.
+#
+# Usage:
+# save-run-state.sh --ctx <dir> --verdict <APPROVE|REQUEST_CHANGES> \
+# --findings <blocker>,<major>,<minor>,<nit> \
+# --rounds <n> --converged <true|false> \
+# [--docs <path>[,<path>]] [--note <text>]
+#
+# Writes <state>/runs/<head7>/{main-merged.md,meta.env} and appends
<state>/index.tsv,
+# where <state> is the STATE_DIR recorded in <ctx>/meta.env by
prepare-review-context.sh.
+set -euo pipefail
+
+CTX=""; VERDICT=""; FINDINGS=""; ROUNDS=""; CONVERGED=""; DOCS=""; NOTE=""
+while [ $# -gt 0 ]; do
+ case "$1" in
+ --ctx) CTX="$2"; shift 2 ;;
+ --verdict) VERDICT="$2"; shift 2 ;;
+ --findings) FINDINGS="$2"; shift 2 ;;
+ --rounds) ROUNDS="$2"; shift 2 ;;
+ --converged) CONVERGED="$2"; shift 2 ;;
+ --docs) DOCS="$2"; shift 2 ;;
+ --note) NOTE="$2"; shift 2 ;;
+ -h|--help) sed -n '2,20p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;;
+ *) echo "unknown argument: $1" >&2; exit 2 ;;
+ esac
+done
+
+[ -n "$CTX" ] || { echo "ERROR: --ctx is required" >&2; exit 2; }
+[ -f "$CTX/meta.env" ] || { echo "ERROR: $CTX/meta.env not found" >&2; exit 2;
}
+
+read_meta() { sed -n "s/^$1=//p" "$CTX/meta.env" | head -n 1; }
+STATE_DIR="$(read_meta STATE_DIR)"
+HEAD_SHA="$(read_meta HEAD_SHA)"
+PR_NUMBER="$(read_meta PR_NUMBER)"
+REVIEW_DATE="$(read_meta REVIEW_DATE)"
+
+if [ -z "$STATE_DIR" ]; then
+ echo "No STATE_DIR in meta.env (the PR number was not resolved), so this
run cannot be saved." >&2
+ echo "That is not fatal - it only means the next review starts without
this one's dismissals." >&2
+ exit 0
+fi
+[ -n "$VERDICT" ] || { echo "ERROR: --verdict is required" >&2; exit 2; }
+[ -n "$FINDINGS" ] || { echo "ERROR: --findings is required" >&2; exit 2; }
+[ -n "$ROUNDS" ] || { echo "ERROR: --rounds is required" >&2; exit 2; }
+[ -n "$CONVERGED" ] || { echo "ERROR: --converged is required" >&2; exit 2; }
+case "$VERDICT" in APPROVE|REQUEST_CHANGES) ;; *) echo "ERROR: bad --verdict:
$VERDICT" >&2; exit 2 ;; esac
+case "$CONVERGED" in true|false) ;; *) echo "ERROR: --converged must be true
or false" >&2; exit 2 ;; esac
+echo "$FINDINGS" | grep -Eq '^[0-9]+,[0-9]+,[0-9]+,[0-9]+$' \
+ || { echo "ERROR: --findings must be <blocker>,<major>,<minor>,<nit>" >&2;
exit 2; }
+
+HEAD7="${HEAD_SHA:0:7}"
+RUN_DIR="$STATE_DIR/runs/$HEAD7"
+mkdir -p "$RUN_DIR"
+
+if [ -f "$CTX/ledger/main-merged.md" ]; then
+ cp "$CTX/ledger/main-merged.md" "$RUN_DIR/main-merged.md"
+else
+ echo "WARNING: no $CTX/ledger/main-merged.md to save - the next run
inherits nothing." >&2
+fi
+cp "$CTX/meta.env" "$RUN_DIR/meta.env"
+[ -f "$CTX/coverage_report.txt" ] && cp "$CTX/coverage_report.txt"
"$RUN_DIR/coverage_report.txt"
+
+INDEX="$STATE_DIR/index.tsv"
+[ -f "$INDEX" ] || printf
'date\thead\tverdict\tblocker,major,minor,nit\trounds\tconverged\tdocs\tnote\n'
> "$INDEX"
+# One row per head. Re-running against an unchanged head replaces its row.
+if grep -q " ${HEAD7} " "$INDEX" 2>/dev/null; then
+ grep -v " ${HEAD7} " "$INDEX" > "$INDEX.tmp" && mv "$INDEX.tmp"
"$INDEX"
+fi
+printf '%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n' \
+ "${REVIEW_DATE:-$(date +%Y-%m-%d)}" "$HEAD7" "$VERDICT" "$FINDINGS" \
+ "$ROUNDS" "$CONVERGED" "${DOCS:-}" "${NOTE:-}" >> "$INDEX"
+
+echo "saved run state for PR #${PR_NUMBER:-?} head $HEAD7"
+echo " $RUN_DIR"
+echo " $INDEX"
+echo
+echo "history for this PR:"
+column -t -s ' ' "$INDEX" 2>/dev/null || cat "$INDEX"
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
