Skip to content

tech-debt: extend review noise-reduction to the escalate / manual-rework path (one final remediation for the whole cycle) #373

Description

@rucka

Story Statement

As a maintainer driving Pair stories through implement-batch
I want the "one review + one final remediation" noise policy to hold across the WHOLE review↔remediation cycle of a PR — through escalations, resume runs, and manual out-of-band rounds
So that a PR that took several runs/escalations to converge still shows exactly one review + one final remediation, not a per-round pile of comments

Where: the implement-batch workflow (.claude/workflows/implement-batch.js) review↔fix loop and its test suite.

Epic Context

Parent Epic: workflow-tooling / review noise-reduction (extends #367, the #314 bundle)
Status: Refined
Priority: P2 (Could-Have) — tech-debt, never blocks a PR (R7.2)

Classification

risk:yellow · cost:green · coupling: not assessed — no domain artifacts

Matrix — per dimension
Dimension Tier Source Note
Service/domain criticality yellow KB default implement-batch is the delivery orchestrator (supporting tooling, central to the workflow).
Change/diff risk yellow story scope Edits the central review-loop control flow (first gate, convergence/synthesis, comment minimize) + prompts; small surface (1 file + test) but behavior-sensitive.
Business impact green subdomain class Internal DX / comment noise; tech-debt, no user-facing impact.
Security relevance green path heuristic No secrets/auth/PII; prompt + control-flow only.
Coupling balance not assessed absent Dev-tooling — no product bounded-context/domain artifacts.

Overall risk:yellow — touches central review-gate control flow; a regression could silence a legitimate review comment (or re-introduce noise). Not red: no production runtime, no data, human still gates merge; worst case is cosmetic noise or a re-run. cost computed but not projected as a tag (risk-matrix Tag Projection: Active: risk).

Acceptance Criteria

Functional Requirements

  1. Given a PR whose review cycle already produced a first-review comment and left a persisted working log .pair/working/reviews/<id>.md (a prior run escalated or was interrupted)
    When the batch is re-run on that story (resume)
    Then the round-0 review runs SILENT (as a re-review) and appends to the SAME working log — no second full review report is posted on the PR.

  2. Given a resumed/escalated cycle that converges (zero actionable findings remaining)
    When the loop finishes
    Then exactly ONE synthesized remediation comment is posted — mapping every finding across ALL runs (read from the persisted log) to its resolution + accepted/non-actionable dispositions + final verdict — AND the working log is then deleted, so the PR shows exactly one review + one final remediation.

  3. Given a prior escalate-flush comment (or manual intermediate comments) exist on the PR
    When the cycle converges on a later run
    Then those intermediate comments are minimized / marked outdated so only the first review + the one final remediation remain as the visible current state.

  4. Given a human/orchestrator takes over rework/re-review out-of-band after an escalate
    When they act on the PR
    Then the documented convention (in the implement-batch.js PR-COMMENT POLICY block) directs them to funnel into the same working log and let the next orchestrated run synthesize; any intermediate manual comments are minimized at convergence.

  5. Given non-convergence after MAX_FIX_ROUNDS on a run
    When it escalates
    Then the working log is KEPT as the continuation anchor and NO synthesis/delete happens (regression guard on existing escalate behavior).

  6. Given a brand-new story with no prior PR/log
    When it is driven end-to-end
    Then behavior is unchanged vs the [#314] feat: wire template-override resolution into skills #367 policy — first review posted, re-reviews silent, convergence synthesizes exactly once (no regression).

Business Rules

  • The persisted working log .pair/working/reviews/<id>.md is the single source of truth for cycle state across runs; its existence == an in-flight cycle to CONTINUE (not restart).
  • Across the ENTIRE cycle of a PR — regardless of run/escalation/manual-round count — at most one first-review comment + at most one final remediation comment remain visible.
  • The orchestrator runs sandboxed (no FS / no gh): the log existence-probe, comment posting, and comment minimizing are all delegated to agents in the worktree.
  • The human merge gate is unchanged; the workflow never merges.

Edge Cases and Error Handling

  • prNumber resume with no prior review: PR opened but review never ran (no log yet) → treated as a FRESH review (round-0 posts the first review), not silenced.
  • failed-fix mid-round: a partial log exists and its path is surfaced in the return; the next resume continues from it (log kept) — no orphaned log.
  • Immediate convergence on resume: round-0 re-review of a resume finds zero actionable → still posts the ONE final synthesis + deletes the log (never leaves the escalate-flush comment as the last word).
  • No comment to minimize: no prior flush/manual comment exists (e.g. resume of a clean interrupt) → minimize is a no-op; the single synthesis is still posted.

Definition of Done Checklist

  • AC1–AC6 implemented and verified
  • Code follows project conventions (one-line opts style; .claude/workflows/ is outside the prettier gate)
  • Test-first: failing tests added for continuation-silent-review + single-synthesis-across-cycle before the code change
  • implement-batch.test.mjs green (existing [#314] feat: wire template-override resolution into skills #367 cases + new continuation/regression cases)
  • Code review completed and approved
  • No new files; change confined to implement-batch.js + implement-batch.test.mjs

Story Sizing and Sprint Readiness

Final Story Points: S (2)
Confidence Level: Medium
Sizing Justification: One file + its test; bounded control-flow change (a continuation flag, seeding remediated, a first gate tweak) + prompt/comment edits. The only unknown is the sandbox-safe existence-probe for the log.
Sprint Fit: Yes — fits a single sprint comfortably.

Dependencies and Coordination

Prerequisite Stories: #367 (in-loop noise-reduction) — MERGED. Builds directly on its reviewLog / synthesis machinery.
Dependent Stories: none.
Mutex note: touches ONLY implement-batch.js (+ test) — must not be batched with any other story editing that file.

Validation and Testing Strategy

  • Unit tests via implement-batch.test.mjs dispatch harness (mock agent calls, assert on prompt content + step labels), mirroring the existing [#314] feat: wire template-override resolution into skills #367 noise-policy tests.
  • New cases: (a) resume/continuation → round-0 review is SILENT, appends to same log; (b) continuation convergence → single synth: step posts one remediation + minimizes prior flush + deletes log; (c) escalate keeps log, no synthesis (regression); (d) fresh cycle unchanged (regression).

Technical Analysis

Implementation Approach

Technical Strategy: Make the review↔fix loop resumable across runs by keying continuation off the persisted working log, all within implement-batch.js:

  1. Continuation detection at review-loop entry — delegate a cheap existence-probe of .pair/working/reviews/<id>.md (sandbox has no FS) to derive isContinuation. Seed remediated = true when continuation, so convergence synthesizes and cleans the log even if round-0 converges immediately.
  2. Silent round-0 on continuationconst first = round === 0 && !isContinuation; the round-0 review then uses the existing RE-REVIEW prompt (no PR comment, append to log).
  3. Whole-cycle synthesis — extend the convergence synthesis prompt to (a) map findings across ALL runs from the persisted log, and (b) minimize / mark-outdated any prior escalate-flush or manual intermediate comments so only first-review + final remediation remain visible.
  4. Escalate = continuation anchor — confirm escalate keeps the log; refresh the escalate-flush prompt + the PR-COMMENT POLICY comment block to document the manual out-of-band convention (append to same log; next run synthesizes; intermediates minimized).

Key Components: the driveStory review↔fix while loop; the reviewer/fix/flush/synth agent prompts; the PR-COMMENT POLICY comment block.
Design: not required.

Explicitly Out of Scope

  • Changing standalone reviewer/fix agent defaults (.claude/agents/*), pair-process-review, templates, pair-next, pair-cli. The soft "Consider: standalone agents default to log-not-comment" item is captured as a documented CONVENTION only (code comments in implement-batch.js), NOT enforced by editing those files. Files touched by this story = implement-batch.js + implement-batch.test.mjs ONLY.

Technical Risks and Mitigation

Risk Impact Probability Mitigation
Over-silencing — a legitimate first review gets suppressed High Low Explicit isContinuation gate + fresh-cycle regression test (AC6); prNumber-with-no-log edge case posts fresh.
Extra delegated probe adds an agent call/latency Low Medium Cheap model/effort for the probe; only on resume runs.

Refinement Date: 2026-07-24

Metadata

Metadata

Assignees

No one assigned

    Labels

    risk:yellowClassification: medium risk tiertech-debtTracked technical debt (living backlog, R7.2 — never blocks a PR)

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions