forked from Octopus/octopus-workflow
v0.1.5: standalone-readability pass — delink --strip provenance mode ([TT-NNNN]), Tier-2 dangling refs neutralized, verify/review-code giant skills split into reference/ on-demand files; guards now runtime-neutral (bun|node)
This commit is contained in:
@@ -1,7 +1,7 @@
|
||||
# Code Review Output Artifacts
|
||||
|
||||
> Output tree for code review artifacts. Read on-demand for reference.
|
||||
> Output model ([org-internal #1988] Tier 1 migration): Tier 1 local files (raw dimension findings) + Tier 2 wiki pages (task prompts, revision summaries, final reports) + Tier 2 synthesis comment. Status tracking via commit status `pipeline/review-code`.
|
||||
> Output model ([TT-1988] Tier 1 migration): Tier 1 local files (raw dimension findings) + Tier 2 wiki pages (task prompts, revision summaries, final reports) + Tier 2 synthesis comment. Status tracking via commit status `pipeline/review-code`.
|
||||
|
||||
```
|
||||
<runs-root>/{slug}/reviews/code/
|
||||
|
||||
@@ -0,0 +1,240 @@
|
||||
> Extracted from review-code/SKILL.md (Phase C — Developer Revision +
|
||||
> Phase D — Re-review Loop) — moved verbatim; read on-demand when
|
||||
> `overall_verdict` is not PASS (skipped entirely on PASS, which jumps
|
||||
> straight to Phase E).
|
||||
|
||||
<!-- section:phase-c -->
|
||||
## Phase C — Developer Revision
|
||||
|
||||
If `overall_verdict` is PASS, skip to Phase E.
|
||||
|
||||
Otherwise:
|
||||
|
||||
**Orchestrator actions**:
|
||||
|
||||
1. Prepare the Developer's input as a structured task file,
|
||||
created as a wiki page via `wiki 读写 API（见 TERMINOLOGY）` （`_shared/gitea-write-patterns.md` Pattern 1）with page_name `{slug}/reviews/code/round{N}/task-developer`.
|
||||
|
||||
This file must contain:
|
||||
- Paths to all code files that need revision.
|
||||
- Reference to the synthesis comment on the ticket (posted via `工单评论 API（见 TERMINOLOGY）` in Phase B — Tier 2).
|
||||
- References to the Tier 1 finding files for dimensions that have FAIL or WARN
|
||||
verdicts (`<runs-root>/{slug}/reviews/code/round{N}/findings-{DIMENSION_CODE}.json`).
|
||||
- The Developer MUST NOT receive the full text of all reviewer findings
|
||||
inline — use finding-file path references only to avoid context explosion.
|
||||
- Convergence status: blockers/majors remaining vs. previous round.
|
||||
- **Revision survival ([TT-2601]) — primary path**: the task tool's completion
|
||||
output includes a `task_id`; passing it back resumes the SAME Developer
|
||||
subagent session instead of creating a fresh one. Round 1: persist the
|
||||
returned `task_id` with the round's working-notes entry (it must survive
|
||||
round-boundary compaction). Round ≥ 2: dispatch with `task_id` set to the
|
||||
previous revision round's — the Developer continues in-session with its
|
||||
prior diff/context; the round's task page then carries only the findings
|
||||
delta, not a full re-derivation. Reviewers are NEVER resumed — per-round
|
||||
freshness is their independence.
|
||||
- **Revision survival — fallback** (task_id lost to compaction before it
|
||||
was persisted): prepend the current `<runs-root>/{slug}/working-notes.md`
|
||||
content (run narrative memory — prior decisions, dead ends, reviewer
|
||||
feedback from earlier rounds) and reference the previous round's revision
|
||||
summary (`{slug}/reviews/code/round{N-1}/revision-summary`), stating
|
||||
explicitly that this revises code a prior round already wrote — do NOT
|
||||
re-read the full design/acceptance artifacts unless a finding requires
|
||||
them.
|
||||
|
||||
2. Spawn the Developer (Worker) with the prompt
|
||||
template at `core/skills/_shared/review-revision-prompt.md`, substituting:
|
||||
|
||||
- `{ROLE}`: Developer
|
||||
- `{TASK_FILE}`: task-developer
|
||||
- `{REVISION_SCOPE}`: Apply the Developer Revision Protocol (see Protocol
|
||||
subsection below) — evaluate each finding against codebase reality:
|
||||
Explorer reviewers are automated LLM sub-agents that can hallucinate
|
||||
citations, misapply rules, or miss design context. Verify cited code
|
||||
exists, the rule applies, and the fix does not break existing
|
||||
functionality. WAIVE technically-incorrect findings with justification
|
||||
(`WAIVED-{finding-id}` → one-line reason; canonical definition of the
|
||||
`WAIVED-{id}` convention, incl. the round-2+ cosmetic-MINOR waiver
|
||||
rule: `_shared/review-revision-prompt.md`). Revise code minimally and
|
||||
surgically — do NOT refactor unrelated code or introduce new behavior.
|
||||
- `{RESOLUTION_RULE}`: Do NOT silently deviate from the approved baseline
|
||||
(DAG task mode: the node spec + frozen cross-session contracts in
|
||||
`{epic-slug}/dag`; the legacy `{slug}/03-design-*` baseline is archived
|
||||
historical, [TT-3072] phase 3). If a finding exposes a design flaw, mark it
|
||||
as deferred with justification and suggest a design amendment — do not
|
||||
change the code against the baseline.
|
||||
- `{COSMETIC_MINOR_EXAMPLES}`: naming, comments, dead code, import
|
||||
ordering, style
|
||||
- `{WAIVED_EXAMPLE}`: `WAIVED-COR-R2-001`
|
||||
- `{CHECKLIST_REF}`: `core/checklists/code-review.md`
|
||||
- `{FILES_MODIFIED_LINE}`: {list}
|
||||
- `{RETURN_FILE}`:
|
||||
`<runs-root>/{slug}/reviews/code/round{N}/revision-return.md`
|
||||
(Tier 1 return persistence — [TT-2847],
|
||||
`_shared/worker-report-persistence.md`)
|
||||
|
||||
3. The Developer revises the code files in place. The Developer creates a
|
||||
revision summary as a wiki page via `wiki 读写 API（见 TERMINOLOGY）` （`_shared/gitea-write-patterns.md` Pattern 1）with page_name `{slug}/reviews/code/round{N}/revision-summary`,
|
||||
and persists its final return summary to
|
||||
`<runs-root>/{slug}/reviews/code/round{N}/revision-return.md` (Tier 1,
|
||||
worker-report persistence [TT-2847] — see
|
||||
`_shared/worker-report-persistence.md`) BEFORE returning. If the
|
||||
completion notification is lost, the Orchestrator recovers the revision
|
||||
outcome from the persisted return (wiki page for the full summary,
|
||||
Tier 1 file for the conclusion of record).
|
||||
|
||||
4. **Pre-handoff gate** — before reporting completion, the Developer MUST run
|
||||
the same mechanical gate the Orchestrator will re-run at Phase D re-entry
|
||||
(the tree-fingerprint cache turns the re-run into a cache hit — one real
|
||||
execution per tree state, no double-paying):
|
||||
- Run `bun core/skills/review-code/scripts/precondition-gate.ts {slug} --round {N}`
|
||||
from the worktree root (oxlint + typecheck + `test:changed`, CI-aligned —
|
||||
`bun run test:parallel` is verify's job, not a per-revision
|
||||
requirement ([TT-2598])).
|
||||
- Exit 0 required; a new test failure, lint error, or type error introduced
|
||||
by the revision must be fixed before the Orchestrator proceeds to Phase D.
|
||||
- Report the gate's per-check results in the revision summary.
|
||||
|
||||
---
|
||||
|
||||
### Phase C — Developer Revision: Protocol
|
||||
|
||||
The Developer applies this cognitive protocol before implementing any finding.
|
||||
**Core principle:** reviewer findings are claims to evaluate, not orders to
|
||||
execute. Verify before implementing. Technical correctness over performative
|
||||
agreement.
|
||||
|
||||
#### Response Pattern
|
||||
|
||||
1. **READ** — Load every FAIL/WARN dimension finding file
|
||||
(`<runs-root>/{slug}/reviews/code/round{N}/findings-{DIMENSION_CODE}.json`,
|
||||
Tier 1 local). Do not react item-by-item.
|
||||
2. **UNDERSTAND** — Restate each finding's requirement in your own words.
|
||||
If you cannot, the finding is unclear — see STOP rule below.
|
||||
3. **VERIFY** — Check the finding against codebase reality: does the cited
|
||||
code exist at the given file:line? Does the cited rule apply?
|
||||
4. **EVALUATE** — Is the finding technically correct for THIS codebase?
|
||||
Does the proposed fix break existing functionality? Does it conflict
|
||||
with the approved baseline (DAG task mode: the node spec + contracts in
|
||||
`{epic-slug}/dag`)?
|
||||
5. **RESPOND** — Fix it (correct), WAIVE it (`WAIVED-{finding-id}` →
|
||||
one-line reason), or flag it (unclear / design-level question).
|
||||
6. **IMPLEMENT** — One verified finding at a time, test each individually.
|
||||
|
||||
#### Evaluating Findings — 5-Check Gate
|
||||
|
||||
Before implementing any finding, run these checks:
|
||||
|
||||
1. **Cited code is real** — Open the file:line. Does the code exist and say
|
||||
what the finding claims?
|
||||
2. **Rule applies** — Does the cited checklist rule actually match this issue?
|
||||
3. **Reason for current implementation** — Is the flagged behavior
|
||||
intentional? Check the baseline (node spec + contracts in
|
||||
`{epic-slug}/dag` for DAG task mode) for rationale.
|
||||
4. **Fix does not break existing functionality** — Will the proposed change
|
||||
cause a regression?
|
||||
5. **Reviewer has full context** — Did the reviewer see the baseline (node
|
||||
spec + acceptance criteria) and the diff context? An isolated diff review
|
||||
can miss architectural intent.
|
||||
|
||||
```
|
||||
IF a finding seems wrong:
|
||||
WAIVE it: WAIVED-{finding-id} → <one-line technical reason>
|
||||
|
||||
IF you cannot easily verify a finding:
|
||||
State the limitation in revision_summary.md and request direction.
|
||||
|
||||
IF a finding conflicts with the approved design:
|
||||
Flag as a design-level question — do NOT silently change code against the design.
|
||||
```
|
||||
|
||||
#### Unclear Findings — STOP Rule
|
||||
|
||||
```
|
||||
IF any finding is unclear:
|
||||
STOP — do not implement ANY finding yet.
|
||||
Flag the unclear items in revision_summary.md and request clarification.
|
||||
|
||||
WHY: findings may be related. Partial understanding produces wrong
|
||||
implementation across multiple items.
|
||||
```
|
||||
|
||||
**Example:** The synthesis lists findings F1–F6. You understand F1, F2, F3, F6.
|
||||
F4 and F5 are ambiguous about what change is expected.
|
||||
- **WRONG:** implement F1, F2, F3, F6 now; ask about F4, F5 later.
|
||||
- **RIGHT:** "Understand F1, F2, F3, F6. Need clarification on F4, F5 before
|
||||
proceeding — they may interact with the same module."
|
||||
|
||||
#### YAGNI Check
|
||||
|
||||
```
|
||||
IF a reviewer suggests "implementing this properly" (full abstraction,
|
||||
generic API, configurable layer):
|
||||
Check actual usage (codegraph callers / grep) BEFORE expanding scope.
|
||||
|
||||
IF unused: WAIVE — "No call sites found. Leaving as-is (YAGNI)."
|
||||
IF used: Then implement properly.
|
||||
```
|
||||
|
||||
#### Forbidden Responses in revision_summary.md
|
||||
|
||||
**NEVER** write:
|
||||
- "You're absolutely right!" / "Great point!" / "Excellent catch!" — performative.
|
||||
- "Thanks for catching that!" / any gratitude expression.
|
||||
- "Let me implement that now" — before verification.
|
||||
|
||||
**INSTEAD:**
|
||||
- Restate the technical requirement concisely.
|
||||
- State the fix factually: `Fixed {finding-id}: <what changed> at <file:line>`.
|
||||
- Just fix it and show the result in the diff.
|
||||
|
||||
#### Gracefully Correcting a WAIVE
|
||||
|
||||
If you WAIVEd a finding and were wrong:
|
||||
```
|
||||
"Verified {finding-id} — the reviewer is correct. My initial read missed
|
||||
<reason>. Implementing now."
|
||||
```
|
||||
State the correction factually. No lengthy apology.
|
||||
|
||||
#### Implementation Order
|
||||
|
||||
For multi-finding revisions, after clarifying all unclear items:
|
||||
1. **Blocking issues** — security, data loss, breaks existing functionality.
|
||||
2. **Simple fixes** — typos, imports, naming, dead code.
|
||||
3. **Complex fixes** — logic changes, refactoring within minimal-surgical scope.
|
||||
Test each fix individually.
|
||||
|
||||
#### Rationalization-Anticipation Quick Reference
|
||||
|
||||
| Trap | Hold to |
|
||||
|------|---------|
|
||||
| "The reviewer flagged it as BLOCKER, so it must be real." | VERIFY cited code exists and rule applies before any edit. |
|
||||
| "Just fix everything to avoid a re-review round." | EVALUATE each finding; WAIVE the wrong ones with reasoning. |
|
||||
| "This finding looks plausible — I'll fix it without checking." | Open the file:line, confirm the behavior. |
|
||||
| "I'll implement the clear findings and ask about unclear ones later." | STOP. Clarify ALL unclear items before implementing any. |
|
||||
| "The reviewer wants it done 'properly' — full abstraction." | Check call sites first. If unused, WAIVE (YAGNI). |
|
||||
| "I shouldn't push back — the reviewer is probably right." | Push back with technical reasoning. The diff is the acknowledgment. |
|
||||
|
||||
---
|
||||
|
||||
<!-- section:phase-d -->
|
||||
## Phase D — Re-review Loop
|
||||
|
||||
After the Developer completes revision:
|
||||
|
||||
1. Increment the round counter (the Synthesizer updates the commit status in Phase B
|
||||
with the new round's results).
|
||||
2. Re-enter Phase A at step 2.5 — the mechanical-green gate re-runs on the
|
||||
revised tree before any reviewer spawn (a red gate blocks dispatch exactly
|
||||
as on round-1 entry) — then spawn the new round of reviewers against the
|
||||
revised files.
|
||||
|
||||
**Convergence criteria**: see `core/skills/_shared/review-pipeline-phases.md` Phase D for the canonical table (stop conditions). `{MAX_ROUNDS}` is per-skill — code review binds the routing budget: **2** rounds default, **3** for high risk (a BLOCKER survived a round, or the diff touches a frozen cross-session contract) — `<instance-root>/workflow-routing.yaml` `review.max_rounds` ([TT-3567]). The code-review-specific addition is inheritance of the shared INFO-disposition rule. The shared Round-3 Escalation Guardrail is scoped to the `review-artifact` skill targets only and does not apply here.
|
||||
|
||||
**Round budget (mandatory, [TT-3567])**: when `current_round` reaches the budget (2; 3 for high risk) without convergence, do NOT enter another round. STOP: register every remaining BLOCKER/MAJOR finding as TD registry rows (`rules/ticket-lifecycle.md`), record the verdict with the shared three-tier ladder (`core/schemas/synthesis.schema.json` `overall_verdict` + `core/skills/_shared/review-orchestrator-rules.md` rule 3): FAIL if any BLOCKER or FAIL dimension remains, else WARN, PASS only if every remaining dimension is PASS ([TT-4786]: the prior binary "else PASS" wording silently upgraded WARN-eligible outcomes to PASS), and close the loop. The debt is carried by the TD registry rows just registered, not by the recorded verdict. A review that cannot converge within budget is a design-level stall wearing review clothes (retro 2026-08-21 / [TT-3050]): in DAG task mode, map it to the DAG oversize signals (`<instance-root>/workflow-routing.yaml` `dag.size_derivation.oversize_signals`) and let the aggregator agent drive re-derivation + a `review-dag` re-run; otherwise record the design-level finding on the Epic, or open `{slug}/03-design-amendments` via `wiki 读写 API（见 TERMINOLOGY）` (`_shared/gitea-write-patterns.md` Pattern 1).
|
||||
|
||||
**Efficiency optimization**: see `core/skills/_shared/review-pipeline-phases.md` Phase D (same rule — from round 2 onward ([TT-2598]), only spawn reviewers for FAIL/WARN/UNRESOLVED dimensions; full sweep every 2 rounds). Round ≥2 reviewer input is delta-scoped to the revised files + their neighbors.
|
||||
|
||||
<!-- inject: ../_shared/compaction-round-boundary.md -->
|
||||
|
||||
---
|
||||
Reference in New Issue
Block a user