Skip to content

Commit 6f89566

Browse files
committed
chore: Sync review-rounds / review-respond skills with documents repo
Mirrors four upstream changes: - Specify build-invocation forms and accept Windows PowerShell in review-helper. Pin pwsh ./build.ps1 / powershell ./build.ps1 forms on Windows, mandate `>` redirection for log capture, and forbid `-Command` and `tee` / `Tee-Object`. - Bundle the final-report compile prompt next to the sequencer .py (mirror of final-report-format.md), and exclude future-recommendation candidates whose same-location finding ended up Fixed in a later round. - Pass previous-round doc paths to the triage sub-agent so findings matching prior-round Fixed / Won't Fix / Downgrade verdicts are treated as Won't Fix. - Render future-recommendation findings as full-text subsections (one subsection per finding, full text quoted from the source review document) in place of the previous table.
1 parent d0a5477 commit 6f89566

10 files changed

Lines changed: 179 additions & 27 deletions

File tree

.claude/agents/review-helper.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
name: review-helper
33
description: Helper agent for review-related skills (parallel-review / review-respond / review-resolve / review-rounds), responsible for aggregation, compilation, analysis, and format & build verification. Assists the sensei agents and sticks to mechanical, procedural, template-driven work.
44
model: sonnet
5-
allowed-tools: Read, Write, Glob, Grep, Bash(grep:*), Bash(ls:*), Bash(find:*), Bash(mkdir:*), Bash(git diff:*), Bash(git log:*), Bash(git show:*), Bash(git status:*), Bash(clang-format:*), Bash(cmake-format:*), Bash(cmake:*), Bash(make:*), Bash(pwsh ./build.ps1:*), Bash(.claude/scripts/rm-tmp.sh:*), Bash(python .claude/scripts/render-review.py:*)
5+
allowed-tools: Read, Write, Glob, Grep, Bash(grep:*), Bash(ls:*), Bash(find:*), Bash(mkdir:*), Bash(git diff:*), Bash(git log:*), Bash(git show:*), Bash(git status:*), Bash(clang-format:*), Bash(cmake-format:*), Bash(cmake:*), Bash(make:*), Bash(pwsh ./build.ps1:*), Bash(powershell ./build.ps1:*), Bash(.claude/scripts/rm-tmp.sh:*), Bash(python .claude/scripts/render-review.py:*)
66
---
77

88
You are **review-helper**, a helper agent that assists the specialist (sensei) agents (cpp-sensei / qt-sensei / obs-sensei, etc.) in the review-related skills.

.claude/sequencer/programs/review_rounds.py

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,11 @@
9393
Path(__file__).resolve().parent / "review_rounds" / "final-report-format.md"
9494
).as_posix()
9595

96+
# Adjacent bundle: prompt template for the final report aggregator sub-agent.
97+
_FINAL_REPORT_COMPILE_PATH = (
98+
Path(__file__).resolve().parent / "review_rounds" / "final-report-compile.md"
99+
).as_posix()
100+
96101
_SEVERITY_COUNTS_SCHEMA = {
97102
"type": "object",
98103
"properties": {
@@ -262,7 +267,12 @@
262267
{commit_clause}
263268
264269
Additional constraints:
265-
- Do not reference the previous round's review document (bias avoidance)
270+
- When launching the triage sub-agent, pass the following as the
271+
previous_round_doc_paths variable (`(none)` in Round 1; in Round N, the
272+
doc_paths of Round 1..N-1). For decision behavior, see the Won't Fix
273+
guideline in templates/triage.md:
274+
{previous_round_doc_paths_block}
275+
- Do not pass the previous round's review document to the estimate sub-agent (bias avoidance)
266276
- Confirm Will Fix count via the triage sub-agent's return value (state explicitly even if 0)
267277
- When judging diffusion signal e (Will Fix originating from FIXME), confirm
268278
whether the target finding originates from FIXME: / TODO: in the review body
@@ -317,6 +327,10 @@
317327
Run {respond_skill} Step 1 (triage).
318328
Append to triage prompt: prioritize triage of findings whose stage is "feedback"
319329
(Feedback details are in current_meta.verification).
330+
When launching triage, pass the following as the previous_round_doc_paths
331+
variable (for decision behavior, see the Won't Fix guideline in
332+
templates/triage.md):
333+
{previous_round_doc_paths_block}
320334
321335
Step 2.4.{attempt}.2 Feedback estimate
322336
Run {respond_skill} Step 2 (parallel estimate).
@@ -375,7 +389,7 @@
375389
the leader). The launch prompt is as follows:
376390
377391
```
378-
As your first action, you MUST Read `.claude/skills/review-rounds/templates/final-report-compile.md`. Do not perform any other judgment, action, or tool call before the Read completes. After reading, follow its instructions.
392+
As your first action, you MUST Read `{compile_path}`. Do not perform any other judgment, action, or tool call before the Read completes. After reading, follow its instructions.
379393
380394
Variables (substitute into the template's {{{{...}}}} placeholders):
381395
- round_doc_paths: |
@@ -533,6 +547,8 @@ def run(ctx):
533547
break
534548

535549
# ----- Step 2.2: review-respond -----
550+
# round_records has not yet been appended for the current round, so
551+
# its contents at this point form the "list of past-round documents".
536552
respond_result = yield Instruction(
537553
text=_TPL_RESPOND.format(
538554
round_num=round_num,
@@ -541,6 +557,9 @@ def run(ctx):
541557
doc_path=doc_path,
542558
confirm_clause=confirm_clause,
543559
commit_clause=commit_clause,
560+
previous_round_doc_paths_block=_format_round_docs_block(
561+
round_records
562+
),
544563
),
545564
expect_schema=_RESPOND_SCHEMA,
546565
timeout_minutes=180,
@@ -603,6 +622,9 @@ def run(ctx):
603622
doc_path=doc_path,
604623
respond_skill=_REVIEW_RESPOND_SKILL,
605624
resolve_skill=_REVIEW_RESOLVE_SKILL,
625+
previous_round_doc_paths_block=_format_round_docs_block(
626+
round_records
627+
),
606628
),
607629
expect_schema=_FEEDBACK_SCHEMA,
608630
timeout_minutes=120,
@@ -657,6 +679,7 @@ def run(ctx):
657679
branch_dir=branch_dir,
658680
termination_reason=termination_reason,
659681
format_path=_FINAL_REPORT_FORMAT_PATH,
682+
compile_path=_FINAL_REPORT_COMPILE_PATH,
660683
round_docs_block=_format_round_docs_block(round_records),
661684
per_round_stats_block=_format_per_round_stats_block(round_records),
662685
),
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
---
2+
name: final-report-compile
3+
description: Prompt for the final report aggregator sub-agent that generates the final report from all rounds' review documents in review-rounds Step 3
4+
template_id: 4f8a2d1c-9b35-4e67-a2c1-8b5d3f9e7a16
5+
---
6+
7+
<!--
8+
This file is kept in sync with .claude/skills/review-rounds/templates/final-report-compile.md.
9+
-->
10+
11+
Generate the final report from all rounds' review documents. Read `.claude/rules/sub-agent.md` and observe the common prohibitions.
12+
13+
Input:
14+
15+
- Each round's review document: `{{round_doc_paths}}` (e.g., `Round 1 → {round1_doc_path}, Round 2 → {round2_doc_path}, ...`)
16+
- Each round's statistics (reference information): `{{round_stats}}` (e.g., `Round 1: findings=N, will_fix=N, maintain=N, alternative=N, downgrade=N, fixed=N, wontfix=N, feedback_attempts=N, unresolved=N, code_changed=<bool>, ...`)
17+
- Report template: `{{template_path}}`
18+
- Output path: `{{report_path}}`
19+
- Language: `{{language}}`
20+
21+
What to do:
22+
23+
1. Read the template markdown to grasp the structure (`<...>` placeholders, table structure, sample subsections under future recommendations).
24+
2. From each round's md `<!-- METADATA(id) --> ... <!-- /METADATA(id) -->`, extract Triage / Estimate / Status / Verification values to obtain per-finding details (severity / location / summary / response / whether a separate-PR recommendation is attached, etc.).
25+
3. Fill the template's statistics summary, full findings list, future recommendations, and review document list, and Write to `{{report_path}}`.
26+
- Aggregation rules for the "Recommended future actions" section:
27+
- Candidates: findings with Triage: 🚫 Won't Fix / Estimate: 🔻 Downgrade / Estimate: 🚧 Alternative whose reason field explicitly states a separate-PR recommendation.
28+
- Exclusion: among the candidates, do not include findings at the same location with the same content that were resolved in a later round as Status: 🟢 Fixed (already fixed, so no need to keep on the roadmap). Identity is determined by matching `file:line` and finding gist. When the determination is difficult, do not exclude — instead append a note in the recommendation reason field stating that the determination is deferred.
29+
- Format: produce one subsection per finding, following the template example. Heading format: `### R{source round number}-{source ID} — `file:line`` (always prefix with the round number to avoid ID collisions across multiple rounds). Within the subsection, list severity / source round / source ID / source reviewer / decision as bullets, then **transcribe the entire body** of the finding from the source review document (the range from immediately below `### {id} — ...` up to just before `---`, excluding the `<!-- METADATA(id) --> ... <!-- /METADATA(id) -->` block) after a `**Finding:**` label, **without omission**. Separate subsections with `---`.
30+
31+
Return value: `{report_path, template_id}`. Include `template_id` exactly as Read from this template's frontmatter.

.claude/sequencer/programs/review_rounds/final-report-format.md

Lines changed: 49 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -54,19 +54,59 @@ List findings with `Triage: 🚫 Won't Fix` and `Estimate: 🔻 Downgrade` that
5454

5555
## Recommended future actions
5656

57-
Aggregate the following (findings expected to be addressed later in a separate PR):
57+
List findings expected to be addressed later in a separate PR, in full text using the same section format as the review document.
5858

59-
- `Triage: 🚫 Won't Fix` items where a separate-PR recommendation is explicitly stated in the reason field
60-
- `Estimate: 🔻 Downgrade` items where a separate-PR recommendation is explicitly stated in the reason field
61-
- `Estimate: 🚧 Alternative` (FIXME already attached) items where a separate-PR recommendation is explicitly stated in the reason field
59+
Aggregation targets (any of the following with a separate-PR recommendation explicitly stated in the reason field):
60+
61+
- `Triage: 🚫 Won't Fix`
62+
- `Estimate: 🔻 Downgrade`
63+
- `Estimate: 🚧 Alternative` (FIXME already attached)
64+
65+
Exclusion rule: among the candidates above, exclude from this section any finding at the same location with the same content that was resolved as `Status: 🟢 Fixed` in a later round (already fixed, so there is no need to keep it on the roadmap). Identity is determined by matching `file:line` and finding gist.
6266

6367
This is mutually exclusive with the "Judged not to require action" section. Alternative FIXME attachments are also listed in the "Resolved" section, but when accompanied by a separate-PR recommendation, they are additionally listed in this section (for roadmap purposes).
6468

65-
| # | Severity | Location | Summary | Recommendation reason |
66-
|---|----------|----------|---------|------------------------|
67-
| 1 | Minor | `file:line` | Summary | Triage: 🚫 Won't Fix — bug in existing code. Recommended to fix in a separate PR. |
68-
| 2 | Major | `file:line` | Summary | Estimate: 🔻 Downgrade — Cost L, Signals a,b,c. This response is recommended to be performed in a separate PR. |
69-
| 3 | Major | `file:line` | Summary | Estimate: 🚧 Alternative — FIXME already attached (`output.cpp:200`). Full response recommended in a separate PR. |
69+
### R1-C-1 — `file.cpp:42`
70+
71+
- **Severity:** Critical
72+
- **Source round:** Round 1
73+
- **Source ID:** C-1
74+
- **Source reviewers:** cpp-sensei, obs-sensei
75+
- **Decision:** Triage: 🚫 Won't Fix — bug in existing code. Recommended to fix in a separate PR.
76+
77+
**Finding:**
78+
79+
{Transcribe the entire body of the finding from the source review document (do not include METADATA markers)}
80+
81+
---
82+
83+
### R2-M-3 — `output.cpp:200`
84+
85+
- **Severity:** Major
86+
- **Source round:** Round 2
87+
- **Source ID:** M-3
88+
- **Source reviewers:** cpp-sensei
89+
- **Decision:** Estimate: 🔻 Downgrade — Cost: L, Future: M, Signals: a,b,c — This response is recommended to be performed in a separate PR
90+
91+
**Finding:**
92+
93+
{Transcribe the entire body of the finding from the source review document}
94+
95+
---
96+
97+
### R1-M-2 — `output.cpp:300`
98+
99+
- **Severity:** Major
100+
- **Source round:** Round 1
101+
- **Source ID:** M-2
102+
- **Source reviewers:** qt-sensei
103+
- **Decision:** Estimate: 🚧 Alternative — Cost: M, Future: S, Signals: b,d — FIXME already attached (`output.cpp:200`). Full response recommended in a separate PR.
104+
105+
**Finding:**
106+
107+
{Transcribe the entire body of the finding from the source review document}
108+
109+
---
70110

71111
## Review document list
72112

.claude/skills/review-respond/SKILL.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ As your first action, you MUST Read `.claude/skills/review-respond/templates/tri
138138
Variables (substitute into the template's {{...}} placeholders):
139139
- document_path: {document_path}
140140
- tmp_dir: {tmp_dir}
141+
- previous_round_doc_paths: {previous_round_doc_paths} (in the standard flow, "(none)". Non-empty only when an upper flow such as review-rounds passes a list of past-round doc_paths.)
141142
142143
Round-specific overrides (apply after following the template's instructions):
143144
- (none)

.claude/skills/review-respond/templates/format-build-verify.md

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,13 @@ What to do:
1515
- Verification commands: `clang-format -style=file -fallback-style=none --dry-run -Werror <file>`; on violation, auto-fix with `clang-format -i -style=file -fallback-style=none <file>`. CMake is the same.
1616

1717
2. Build verification:
18-
- Choose the command for the current platform from build.ps1 / Makefile / build script, or from the CMake presets listed in CLAUDE.md.
19-
- Run the build, save stdout/stderr to `{{tmp_dir}}/build.log`. Determine success/failure by exit code.
18+
- Choose one of the following for the current platform:
19+
- Windows: `pwsh ./build.ps1` / `powershell ./build.ps1` (positional script-argument form).
20+
- Linux / macOS: a direct command such as `cmake --preset <preset>` or `make` (use the preset listed in CLAUDE.md).
21+
- Capture output with the `>` redirection only: `<command> > {{tmp_dir}}/build.log 2>&1`.
22+
- Do not use PowerShell's `-Command` flag (limit invocation to positional or `-File` form to avoid arbitrary-expression execution).
23+
- Do not pipe through `tee` / `Tee-Object` (avoids shrinking the permission boundary and unintended external-command invocation).
24+
- Determine success/failure by the exit code.
2025

2126
3. Specialist identification on failure:
2227
- Read build.log and the error-source files to analyze the cause and concisely organize the fix direction (fix_guidance).

.claude/skills/review-respond/templates/triage.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ template_id: 1e9c4f7a-5b82-4d63-a1c8-3f7d2e9b4a15
66

77
As the initial-triage owner of the review document, Read `{{document_path}}`, perform stage classification and the triage decision for each finding, and Write the result to `{{tmp_dir}}/triage.json`. Read `.claude/rules/sub-agent.md` and observe the common prohibitions.
88

9+
If `{{previous_round_doc_paths}}` is provided (empty in the standard flow that runs in Round 1), Read each file to extract past-round decision information (id / location / description / METADATA's triage / estimate / status / verification) and reference it during triage. No reference is needed when the value is empty or `(none)`.
10+
911
Extraction targets: Critical / Major / Minor sections (skip Info). For each finding, obtain id (C-1, M-1, mi-1, etc.) / severity / location / description (the body up to the marker) / current_meta (the current values of triage / estimate / status / verification; when the same field appears multiple times, use the last value).
1012

1113
Stage classification (based on current_meta):
@@ -34,6 +36,10 @@ Won't Fix guideline (when any of the following applies):
3436
4. Inferable as acceptable from the project's purpose, use case, or assumed users.
3537
5. Preference-based refactoring (no rationale grounded in correctness, safety, performance, or maintainability).
3638
6. Reproducibility unclear; e2e verification needed.
39+
7. A finding at the same location with the same content was already processed in a past round (only judgable when `{{previous_round_doc_paths}}` is provided). Identity is determined by matching `file:line` and finding gist. Applicable patterns:
40+
- Already `status: 🟢 Fixed` in a past round (an edge case that does not happen in the normal flow; since it has been rediscovered, explicitly state in the reason field "Already Fixed in a previous round").
41+
- `triage: 🚫 Won't Fix` in a past round (state in the reason field "Same finding as previous-round Won't Fix" and concisely transcribe the past decision's reason as well).
42+
- `estimate: 🔻 Downgrade` in a past round (state in the reason field "Same finding as previous-round Downgrade" and concisely transcribe the past decision's reason as well).
3743

3844
High-severity exception: For Critical / Major Won't Fix, explicitly state "recommend separate PR" in the reason field (e.g. "Won't Fix — Existing-code bug. Recommend fixing in a separate PR.").
3945

.claude/skills/review-rounds/SKILL.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,12 @@ Round-specific overrides (apply after following the template's instructions):
142142
- At triage start: `## Round {N} — Step 2: Triage`
143143
- At estimate start: `## Round {N} — Step 2.5: Estimate`
144144
- At fix / verify / update / commit start: `## Round {N} — Step 3: Review Respond (Fix & Verify)`
145-
- Additional constraints for the triage / estimate sub-agents:
146-
- Do not reference the previous round's review document.
145+
- Additional constraints for the triage sub-agent:
146+
- At launch, pass the full list of past-round doc_paths as the `previous_round_doc_paths` variable (`(none)` in Round 1, the doc_paths of Round 1..N-1 in Round N). For decision behavior, see the Won't Fix guideline in `.claude/skills/review-respond/templates/triage.md`.
147147
- State the Will Fix count explicitly in the triage report (also state explicitly when 0).
148-
- When determining the diffusion signal e (Will Fix originating from FIXME) during estimate, verify whether the finding originates from a `FIXME:` / `TODO:` in the review body or target file.
148+
- Additional constraints for the estimate sub-agent:
149+
- Do not reference the previous round's review document (bias avoidance).
150+
- When determining the diffusion signal e (Will Fix originating from FIXME), verify whether the finding originates from a `FIXME:` / `TODO:` in the review body or target file.
149151
- Round loop control after triage:
150152
- Will Fix == 0: skip Steps 2.3–2.4 and proceed to Step 2.5 (round end).
151153
- Will Fix >= 1: proceed to the estimate phase.

.claude/skills/review-rounds/templates/final-report-compile.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,12 @@ Input:
1616

1717
What to do:
1818

19-
1. Read the template markdown to grasp the structure (`<...>` placeholders, table structure).
19+
1. Read the template markdown to grasp the structure (`<...>` placeholders, table structure, sample subsections under future recommendations).
2020
2. From each round's md `<!-- METADATA(id) --> ... <!-- /METADATA(id) -->`, extract Triage / Estimate / Status / Verification values to obtain per-finding details (severity / location / summary / response / whether a separate-PR recommendation is attached, etc.).
2121
3. Fill the template's statistics summary, full findings list, future recommendations, and review document list, and Write to `{{report_path}}`.
22+
- Aggregation rules for the "Recommended future actions" section:
23+
- Candidates: findings with Triage: 🚫 Won't Fix / Estimate: 🔻 Downgrade / Estimate: 🚧 Alternative whose reason field explicitly states a separate-PR recommendation.
24+
- Exclusion: among the candidates, do not include findings at the same location with the same content that were resolved in a later round as Status: 🟢 Fixed (already fixed, so no need to keep on the roadmap). Identity is determined by matching `file:line` and finding gist. When the determination is difficult, do not exclude — instead append a note in the recommendation reason field stating that the determination is deferred.
25+
- Format: produce one subsection per finding, following the template example. Heading format: `### R{source round number}-{source ID} — `file:line`` (always prefix with the round number to avoid ID collisions across multiple rounds). Within the subsection, list severity / source round / source ID / source reviewer / decision as bullets, then **transcribe the entire body** of the finding from the source review document (the range from immediately below `### {id} — ...` up to just before `---`, excluding the `<!-- METADATA(id) --> ... <!-- /METADATA(id) -->` block) after a `**Finding:**` label, **without omission**. Separate subsections with `---`.
2226

2327
Return value: `{report_path, template_id}`. Include `template_id` exactly as Read from this template's frontmatter.

0 commit comments

Comments
 (0)