From 56f9119e98ee3278b36b33c37855777555463f91 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 9 Jun 2026 14:57:31 +0200 Subject: [PATCH 1/5] fix(unic-pr-review): Write Retry completes a partial --post Iteration (no silently dropped Findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A --post run that partially fails left a surviving Approval Loop state directory (ADR-0014), but a re-run routed to re-review (a prior Iteration Marker exists), where an unchanged HEAD yields an empty delta diff, zero Findings, and the Approval Loop is skipped — so every Finding that failed to post was silently dropped. The Step 1.12 warning promised a retry that was never implemented. Implements the locked Write Retry design (ADR-0015): - scripts/lib/write-outcomes.mjs — tested helper with checkWriteRetry (none/retry/stale classification + staleness guard), recordOutcomes (persist per-Finding postedMap + sticky summaryPosted, atomic write, before the success-gated cleanup), and filterUnposted (first-attempt no-op, retry subtracts posted-ok). Exposed via a real-script CLI dispatcher (env-before-node, no inline-eval ESM import of an absolute path) so it works on Windows. - tests/write-outcomes.test.mjs — node:test coverage of all three exports incl. the stale-HEAD discard path and first-attempt no-op. - commands/review-pr.md — Step 1.2a Write Retry check (short-circuits the Fetcher/mode-detection/fan-out and resumes the Approval Loop at the same Iteration), Step 1.11 deltas, Step 1.12 Writer input + Step 1.12a persist, and a rewritten Step 1.12 warning incl. cross-machine and HEAD-moved caveats. - agents/ado-writer.md — first-review input accepts summaryAlreadyPosted; Step 3 skips when true. No other Writer behaviour changes. Fixes #236 Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/claude-code/unic-pr-review/CHANGELOG.md | 2 +- .../unic-pr-review/agents/ado-writer.md | 7 +- .../unic-pr-review/commands/review-pr.md | 86 ++++++- .../scripts/lib/write-outcomes.mjs | 223 ++++++++++++++++++ .../tests/write-outcomes.test.mjs | 174 ++++++++++++++ 5 files changed, 489 insertions(+), 3 deletions(-) create mode 100644 apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs create mode 100644 apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs diff --git a/apps/claude-code/unic-pr-review/CHANGELOG.md b/apps/claude-code/unic-pr-review/CHANGELOG.md index ae2df8d4..d54d1395 100644 --- a/apps/claude-code/unic-pr-review/CHANGELOG.md +++ b/apps/claude-code/unic-pr-review/CHANGELOG.md @@ -17,7 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - (none) ### Fixed -- (none) +- Write Retry completes a partial `--post` Iteration — a re-run with a surviving state directory and unchanged HEAD now resumes the saved Approval Loop and posts only the inline Threads that failed, skips the Summary when it already landed, and never increments the Iteration. Prior behaviour silently dropped all failed Findings by routing the retry to re-review with an empty delta. Adds the tested `scripts/lib/write-outcomes.mjs` helper (`checkWriteRetry` / `recordOutcomes` / `filterUnposted`) and a `summaryAlreadyPosted` input on the ADO Writer. (#236, ADR-0015) ## [2.1.8] — 2026-06-09 diff --git a/apps/claude-code/unic-pr-review/agents/ado-writer.md b/apps/claude-code/unic-pr-review/agents/ado-writer.md index 91d4289e..9a7dc1bf 100644 --- a/apps/claude-code/unic-pr-review/agents/ado-writer.md +++ b/apps/claude-code/unic-pr-review/agents/ado-writer.md @@ -23,12 +23,15 @@ You run in one of two modes. In **first-review** mode (default) you consume appr "repo": "myrepo", "prId": 42, "approvedPath": "/tmp/unic-pr-review-approved-abc123.json", - "iteration": 1 + "iteration": 1, + "summaryAlreadyPosted": false } ``` `mode` absent or `"first-review"` → run Steps 1–4 (existing path). +`summaryAlreadyPosted` — optional boolean (default: absent / false). When `true`, the Review Summary thread already landed in a prior partially-successful `--post` attempt (the Write Retry path, ADR-0015); skip Steps 3a–3d entirely. No other behaviour changes. + ### Re-review mode ```json @@ -158,6 +161,8 @@ Failure here is silent — continue with the next Finding regardless. ### Step 3 — Post the Review Summary +**Write Retry guard:** if `summaryAlreadyPosted` is `true`, skip Steps 3a–3d entirely and set `summaryResult = { "success": true, "threadId": null, "error": null }` — the Summary already landed in a prior attempt, so treat it as a success and let the top-level `success` (Step 4) be `true` when every inline Finding also posted. Then proceed to Step 4. + #### 3a — Render the Review Summary Build `FINDINGS_JSON` from **all** approved Findings — include the full Finding shape required by `parseFinding` inside `render-summary.mjs`: at minimum `confidence`, `filePath`, `startLine`, `title`, `body`, and optionally `suggestion`. Include `severity` too (already on the approved Finding). Pass an empty `positiveObservations` array: diff --git a/apps/claude-code/unic-pr-review/commands/review-pr.md b/apps/claude-code/unic-pr-review/commands/review-pr.md index 6a3f9de4..52992c9c 100644 --- a/apps/claude-code/unic-pr-review/commands/review-pr.md +++ b/apps/claude-code/unic-pr-review/commands/review-pr.md @@ -53,6 +53,38 @@ process.stdout.write(createHash('sha256').update('','utf8').digest('hex').s Store the output as `PR_KEY`. +#### Step 1.2a — Check for Write Retry (ADR-0015) + +Default `IS_WRITE_RETRY = false`. This step only applies when `IS_POST` is true and the provider is ADO (Write Retry has no meaning in a preview or Pre-PR run); when `IS_POST` is false, skip directly to Step 1.3. + +A surviving Approval Loop state directory means the prior `--post` attempt did not complete (it is deleted only on a fully-successful write — ADR-0014). Before invoking the Fetcher, classify the re-run by comparing the saved `headSha` to the current HEAD. + +Get the current HEAD: + +```sh +git rev-parse HEAD +``` + +Capture as `HEAD_SHA`, then classify: + +```sh +HEAD_SHA="" node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/write-outcomes.mjs" check "" +``` + +stdout is a JSON object. Route on its `mode`: + +- **`{ "mode": "none" }`** → no prior state (or it was unreadable); proceed to Step 1.3 (normal review). +- **`{ "mode": "retry", "state": … }`** → **Write Retry**: set `IS_WRITE_RETRY = true`, store `state` as `WRITE_RETRY_STATE`, and set `CURRENT_ITERATION = WRITE_RETRY_STATE.iteration`. Skip Steps 1.3–1.10 entirely (no Fetcher, no mode detection, no aspect fan-out) and go straight to Step 1.11 — the saved approval decisions are reused, nothing is re-prompted. +- **`{ "mode": "stale" }`** → the partial attempt is from a different HEAD (force-push / rebase). Print the Notice, discard the stale state directory, then proceed to Step 1.3 (normal review against the new HEAD): + + ``` + Notice: Prior --post state is stale (saved HEAD differs from current HEAD). Discarding saved state and running a normal review against the new HEAD. + ``` + + ```sh + node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/clear-state-dir.mjs" "" + ``` + #### Step 1.3 — Invoke the ADO Fetcher agent Use the Agent tool to launch the agent identified by `FETCHER_AGENT` (e.g. `unic-pr-review:ado-fetcher`). Provide: @@ -234,6 +266,17 @@ process.stdout.write(f) Capture the output path as `FINDINGS_FILE`. +**Write Retry delta (`IS_WRITE_RETRY` is true):** `FINDINGS_JSON` was never computed (Steps 1.3–1.10 were skipped). Write an empty JSON array to `FINDINGS_FILE` instead — the Approval Loop ignores it and reuses `state.json` because the saved `headSha` matches the current HEAD and `--reset` is absent: + +```sh +PR_KEY="" node -e " +const fs=require('node:fs'),os=require('node:os'),path=require('node:path') +const f=path.join(os.tmpdir(),'unic-pr-review-findings-'+process.env.PR_KEY+'.json') +fs.writeFileSync(f,'[]') +process.stdout.write(f) +" +``` + **If the `node -e` script exits non-zero**, print the stderr verbatim and stop. Do not proceed with an empty or invalid findings path. **2. Determine the approved-Findings path.** @@ -281,6 +324,14 @@ node "${CLAUDE_PLUGIN_ROOT}/scripts/approval-loop.mjs" \ - **Exit 2** (non-TTY without --yes): print `"approval-loop: --post requires an interactive terminal or --yes."` and stop. - **Any other non-zero exit**: relay stderr verbatim and stop. +**Write Retry post-loop (`IS_WRITE_RETRY` is true):** after the Approval Loop writes `APPROVED_FILE`, filter it down to only the Findings that were **not** already successfully posted, using the `postedMap` persisted in `state.json`: + +```sh +APPROVED_FILE="" node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/write-outcomes.mjs" filter "" +``` + +Then set `SUMMARY_ALREADY_POSTED = WRITE_RETRY_STATE.summaryPosted === true` for the Writer input in Step 1.12. + **6. Clean up the findings temp file.** ```sh @@ -302,6 +353,20 @@ Use the Agent tool to launch `unic-pr-review:ado-writer`. Provide: } ``` +**Write Retry delta (`IS_WRITE_RETRY` is true):** use `CURRENT_ITERATION` (from `WRITE_RETRY_STATE.iteration`) instead of `1`, and add `summaryAlreadyPosted` so the Writer skips the Summary when it already landed: + +```json +{ + "orgUrl": "", + "project": "", + "repo": "", + "prId": , + "approvedPath": "", + "iteration": , + "summaryAlreadyPosted": +} +``` + Wait for the agent to complete. It returns: ```json @@ -317,9 +382,28 @@ Print the summary: how many inline threads were posted, how many failed, and the If `success` is `false` (any thread failed), warn the user: ``` -⚠ Some threads could not be posted. Check the errors above and re-run with --post (not --post --yes) once the issues are resolved — the Approval Loop resumes from saved state and re-posts only the threads that failed. Using --yes would re-post the threads that already succeeded, creating duplicate comments. +⚠ Some threads could not be posted. The failed Findings are recorded in the local state directory — re-run with --post (not --post --yes) from the same machine and checkout to trigger Write Retry: the review is skipped and only the threads that failed are re-posted; the Summary is skipped if it already landed. + +Caveats: +- Cross-machine: Write Retry is local. A retry from a different clone has no state directory and falls back to re-review, which sees an empty delta and produces zero Findings. +- HEAD moved: if the branch is force-pushed or rebased between the failed attempt and the retry, the stale state is discarded and a normal re-review runs instead. +- Do not use --yes: --post --yes bypasses the Approval Loop entirely and re-posts all approved Findings, creating duplicate comments for the ones that already succeeded. +``` + +#### Step 1.12a — Persist post outcomes (ADR-0015) + +After the ADO Writer returns, and **before** the success-gated state-directory cleanup in Step 1.13, persist each Finding's post outcome and the Summary-posted flag into `state.json`. This is what lets a subsequent `--post` re-run use Write Retry (Step 1.2a) instead of silently dropping the failed Findings. + +Pass the Writer's returned JSON object through `WRITER_RESULT`: + +```sh +WRITER_RESULT='' node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/write-outcomes.mjs" record "" ``` +If the script exits non-zero, print a warning (`outcomes not persisted — a retry will re-post all Findings`) and continue to Step 1.13. Do not stop the run. + +**This step applies to both the first-review path and the Write Retry path.** On a first attempt the resulting `postedMap` simply seeds the dedup state; on a fully-successful write Step 1.13 then deletes the whole state directory anyway, so the persisted outcomes are discarded together with it. + #### Step 1.13 — Cleanup Delete the approved-Findings temp file (always — it is not needed for retries): diff --git a/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs b/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs new file mode 100644 index 00000000..b22b4c16 --- /dev/null +++ b/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs @@ -0,0 +1,223 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +// @ts-check +// Copyright © 2026 Unic + +/** + * write-outcomes.mjs — Write Retry bookkeeping for the Approval Loop state dir. + * + * Implements the local-dedup half of ADR-0015 (Write Retry completes a + * partially-written Iteration). Three pure-ish helpers, all path-based so they + * are unit-testable without touching the module system: + * + * - `checkWriteRetry` — classify a re-run as none / retry / stale by comparing + * the saved `headSha` to the current HEAD. + * - `recordOutcomes` — after a write, persist each Finding's post outcome and + * a `summaryPosted` flag into `state.json` (atomic write), + * run BEFORE the success-gated cleanup (ADR-0014). + * - `filterUnposted` — drop Findings that already posted so a retry re-posts + * only the ones that failed (first attempt → no-op). + * + * Owned by the `review-pr` orchestrator (Steps 1.2a, 1.11, 1.12a). The CLI + * dispatcher at the bottom exposes the three helpers as a real script file so + * the command-prompt one-liners pass data via env (`VAR=… node script.mjs sub`) + * and never embed an absolute path in an inline ESM `import` specifier — the + * Windows-breaking pattern that issue #227 / clear-state-dir.mjs already retired. + */ + +import { + existsSync as realExistsSync, + readFileSync as realReadFile, + renameSync as realRenameSync, + writeFileSync as realWriteFile, +} from 'node:fs' +import { join } from 'node:path' +import { fileURLToPath } from 'node:url' +import { approvalStateDirPath } from './cache-paths.mjs' + +/** + * @typedef {Object} InlineResult + * @property {string} findingId + * @property {boolean} success + * @property {number|null} threadId + * @property {string|null} [error] + */ + +/** + * @typedef {Object} SummaryResult + * @property {boolean} success + * @property {number|null} threadId + * @property {string|null} [error] + */ + +/** + * @typedef {Record} PostedMap + */ + +/** + * @typedef {Object} WriteRetryState + * @property {string} headSha + * @property {number} [iteration] + * @property {PostedMap} [postedMap] + * @property {boolean} [summaryPosted] + */ + +/** + * @typedef {Object} WriteOutcomeDeps + * @property {(path: string) => boolean} [existsSync] + * @property {(path: string, encoding: BufferEncoding) => string} [readFile] + * @property {(path: string, data: string, encoding: BufferEncoding) => void} [writeFile] + * @property {(from: string, to: string) => void} [renameSync] + */ + +/** + * Classify a `--post` re-run by inspecting the surviving Approval Loop state. + * + * The state directory is deleted only on a fully-successful write (ADR-0014), so + * its presence means the prior `--post` did not complete. The short-circuit fires + * only when the saved `headSha` still matches the current HEAD; otherwise the + * partial attempt is stale and the caller discards it (staleness guard, ADR-0015). + * + * @param {string} statePath - absolute path to `/state.json` + * @param {string} currentHead - current `git rev-parse HEAD` + * @param {WriteOutcomeDeps} [deps] + * @returns {{ mode: 'none' } | { mode: 'retry', state: WriteRetryState } | { mode: 'stale' }} + */ +export function checkWriteRetry(statePath, currentHead, deps = {}) { + const existsSync = deps.existsSync ?? realExistsSync + const readFile = deps.readFile ?? realReadFile + + if (!existsSync(statePath)) return { mode: 'none' } + + let state + try { + state = JSON.parse(readFile(statePath, 'utf8')) + } catch { + return { mode: 'none' } + } + + if (!state || typeof state !== 'object' || typeof state.headSha !== 'string') { + return { mode: 'none' } + } + + if (state.headSha === currentHead) return { mode: 'retry', state } + return { mode: 'stale' } +} + +/** + * Persist post outcomes into `state.json`, run after the Writer returns and + * BEFORE the success-gated cleanup (ADR-0014). Merges into any existing state so + * `headSha`, `iteration`, and `findings` are preserved. `summaryPosted` is sticky: + * once true it never resets, so a later partial retry cannot un-set it. + * + * A missing or corrupt state file starts from `{}` — the outcomes are still + * written. Written atomically (tmp + rename) like `approval-loop.mjs`. + * + * @param {string} statePath - absolute path to `/state.json` + * @param {InlineResult[]} inlineResults + * @param {SummaryResult|null} summaryResult + * @param {WriteOutcomeDeps} [deps] + * @returns {void} + */ +export function recordOutcomes(statePath, inlineResults, summaryResult, deps = {}) { + const readFile = deps.readFile ?? realReadFile + const writeFile = deps.writeFile ?? realWriteFile + const renameSync = deps.renameSync ?? realRenameSync + + let state = /** @type {Record} */ ({}) + try { + const parsed = JSON.parse(readFile(statePath, 'utf8')) + if (parsed && typeof parsed === 'object') state = parsed + } catch { + // start fresh if the state file is missing or corrupt + } + + const postedMap = /** @type {PostedMap} */ (state.postedMap ?? {}) + for (const r of inlineResults) { + postedMap[r.findingId] = { success: r.success, threadId: r.threadId } + } + + const updated = { + ...state, + postedMap, + summaryPosted: Boolean(state.summaryPosted) || summaryResult?.success === true, + } + + const tmp = `${statePath}.tmp` + writeFile(tmp, JSON.stringify(updated, null, 2), 'utf8') + renameSync(tmp, statePath) +} + +/** + * Drop Findings that already posted successfully so a retry re-posts only the + * failures. An empty or absent posted-map returns the input unchanged — the + * first-attempt no-op that keeps normal behaviour intact. + * + * @template {{ id?: string }} F + * @param {F[]} approvedFindings + * @param {PostedMap} [postedMap] + * @returns {F[]} + */ +export function filterUnposted(approvedFindings, postedMap) { + if (!postedMap || Object.keys(postedMap).length === 0) return approvedFindings + return approvedFindings.filter((f) => postedMap[/** @type {string} */ (f.id)]?.success !== true) +} + +/* ------------------------------------------------------------------ CLI ---- */ + +/** + * Read a required env var or exit non-zero with a usage message. + * @param {string} name + * @returns {string} + */ +function requireEnv(name) { + const value = process.env[name] + if (typeof value !== 'string' || value === '') { + process.stderr.write(`write-outcomes: missing required env var ${name}\n`) + process.exit(1) + } + return value +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) { + const [subcommand, key] = process.argv.slice(2) + + if (!subcommand || !key) { + process.stderr.write('write-outcomes: usage: write-outcomes.mjs \n') + process.exit(1) + } + + const statePath = join(approvalStateDirPath(key), 'state.json') + + if (subcommand === 'check') { + // env HEAD_SHA → stdout: JSON { mode, state? } + const result = checkWriteRetry(statePath, requireEnv('HEAD_SHA')) + process.stdout.write(JSON.stringify(result)) + } else if (subcommand === 'record') { + // env WRITER_RESULT (JSON) → persist outcomes into state.json + let writer + try { + writer = JSON.parse(requireEnv('WRITER_RESULT')) + } catch (err) { + process.stderr.write(`write-outcomes: WRITER_RESULT is not valid JSON: ${String(err)}\n`) + process.exit(1) + } + recordOutcomes(statePath, writer.inlineResults ?? [], writer.summaryResult ?? null) + } else if (subcommand === 'filter') { + // env APPROVED_FILE (path) → rewrite it in place to the un-posted Findings + const approvedFile = requireEnv('APPROVED_FILE') + /** @type {PostedMap} */ + let postedMap = {} + try { + const state = JSON.parse(realReadFile(statePath, 'utf8')) + if (state && typeof state === 'object' && state.postedMap) postedMap = state.postedMap + } catch { + // no readable state → empty posted-map → filter is a no-op + } + const approved = JSON.parse(realReadFile(approvedFile, 'utf8')) + const unposted = filterUnposted(approved, postedMap) + realWriteFile(approvedFile, JSON.stringify(unposted), 'utf8') + } else { + process.stderr.write(`write-outcomes: unknown subcommand '${subcommand}'\n`) + process.exit(1) + } +} diff --git a/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs b/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs new file mode 100644 index 00000000..51c64a82 --- /dev/null +++ b/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs @@ -0,0 +1,174 @@ +// @ts-check +// SPDX-License-Identifier: LGPL-3.0-or-later +// Copyright © 2026 Unic + +import assert from 'node:assert/strict' +import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, it } from 'node:test' +import { checkWriteRetry, filterUnposted, recordOutcomes } from '../scripts/lib/write-outcomes.mjs' + +const HEAD = 'a'.repeat(40) +const OTHER = 'b'.repeat(40) + +function tempDir() { + return mkdtempSync(join(tmpdir(), 'write-outcomes-')) +} + +/** @param {Record} value */ +function writeState(value) { + const path = join(tempDir(), 'state.json') + writeFileSync(path, JSON.stringify(value), 'utf8') + return path +} + +describe('checkWriteRetry', () => { + it('returns {mode:"none"} when state file does not exist', () => { + const path = join(tempDir(), 'state.json') // never written + assert.deepEqual(checkWriteRetry(path, HEAD), { mode: 'none' }) + }) + + it('returns {mode:"retry", state} when headSha matches current HEAD', () => { + const path = writeState({ headSha: HEAD, iteration: 1 }) + const result = checkWriteRetry(path, HEAD) + assert.equal(result.mode, 'retry') + assert.equal(/** @type {any} */ (result).state.headSha, HEAD) + assert.equal(/** @type {any} */ (result).state.iteration, 1) + }) + + it('returns {mode:"stale"} when headSha differs from current HEAD', () => { + const path = writeState({ headSha: OTHER }) + assert.deepEqual(checkWriteRetry(path, HEAD), { mode: 'stale' }) + }) + + it('returns {mode:"stale"} when headSha differs — caller should discard state dir', () => { + // Stale-HEAD discard path: the orchestrator deletes the state dir and runs a normal review. + const path = writeState({ headSha: OTHER, postedMap: { x: { success: true, threadId: 1 } } }) + assert.deepEqual(checkWriteRetry(path, HEAD), { mode: 'stale' }) + }) + + it('returns {mode:"none"} when state file exists but is malformed JSON', () => { + const path = join(tempDir(), 'state.json') + writeFileSync(path, '{not json', 'utf8') + assert.deepEqual(checkWriteRetry(path, HEAD), { mode: 'none' }) + }) + + it('returns {mode:"none"} when state file exists but headSha is not a string', () => { + const path = writeState({ iteration: 2 }) // no headSha + assert.deepEqual(checkWriteRetry(path, HEAD), { mode: 'none' }) + }) + + it('uses injected deps without touching the real filesystem', () => { + const result = checkWriteRetry('/nowhere/state.json', HEAD, { + existsSync: () => true, + readFile: () => JSON.stringify({ headSha: HEAD }), + }) + assert.equal(result.mode, 'retry') + }) +}) + +describe('recordOutcomes', () => { + const inlineResults = [ + { findingId: 'f1', success: true, threadId: 101, error: null }, + { findingId: 'f2', success: false, threadId: null, error: 'boom' }, + ] + + it('writes postedMap with success/threadId from inlineResults', () => { + const path = writeState({ headSha: HEAD }) + recordOutcomes(path, inlineResults, { success: false, threadId: null, error: null }) + const state = JSON.parse(readFileSync(path, 'utf8')) + assert.deepEqual(state.postedMap, { + f1: { success: true, threadId: 101 }, + f2: { success: false, threadId: null }, + }) + }) + + it('sets summaryPosted:true when summaryResult.success is true', () => { + const path = writeState({ headSha: HEAD }) + recordOutcomes(path, [], { success: true, threadId: 200, error: null }) + assert.equal(JSON.parse(readFileSync(path, 'utf8')).summaryPosted, true) + }) + + it('leaves summaryPosted falsey when summaryResult failed', () => { + const path = writeState({ headSha: HEAD }) + recordOutcomes(path, [], { success: false, threadId: null, error: 'x' }) + assert.equal(JSON.parse(readFileSync(path, 'utf8')).summaryPosted, false) + }) + + it('preserves summaryPosted:true when already set (does not reset to false)', () => { + const path = writeState({ headSha: HEAD, summaryPosted: true }) + recordOutcomes(path, [], { success: false, threadId: null, error: 'x' }) + assert.equal(JSON.parse(readFileSync(path, 'utf8')).summaryPosted, true) + }) + + it('merges with existing state fields (headSha, iteration, findings)', () => { + const path = writeState({ headSha: HEAD, iteration: 3, findings: [{ id: 'f1' }] }) + recordOutcomes(path, inlineResults, null) + const state = JSON.parse(readFileSync(path, 'utf8')) + assert.equal(state.headSha, HEAD) + assert.equal(state.iteration, 3) + assert.deepEqual(state.findings, [{ id: 'f1' }]) + assert.equal(state.postedMap.f1.success, true) + }) + + it('merges new outcomes into a pre-existing postedMap', () => { + const path = writeState({ headSha: HEAD, postedMap: { f0: { success: true, threadId: 1 } } }) + recordOutcomes(path, [{ findingId: 'f1', success: true, threadId: 2, error: null }], null) + const state = JSON.parse(readFileSync(path, 'utf8')) + assert.equal(state.postedMap.f0.success, true) + assert.equal(state.postedMap.f1.threadId, 2) + }) + + it('creates state.json when it does not exist (catches parse error)', () => { + const path = join(tempDir(), 'state.json') // never written + recordOutcomes(path, inlineResults, { success: true, threadId: 9, error: null }) + const state = JSON.parse(readFileSync(path, 'utf8')) + assert.equal(state.postedMap.f1.threadId, 101) + assert.equal(state.summaryPosted, true) + }) + + it('writes atomically via tmp + rename', () => { + const writes = /** @type {string[]} */ ([]) + const renames = /** @type {[string, string][]} */ ([]) + recordOutcomes( + '/state/state.json', + [], + { success: true, threadId: 1, error: null }, + { + readFile: () => JSON.stringify({ headSha: HEAD }), + writeFile: (p) => writes.push(p), + renameSync: (from, to) => renames.push([from, to]), + } + ) + assert.deepEqual(writes, ['/state/state.json.tmp']) + assert.deepEqual(renames, [['/state/state.json.tmp', '/state/state.json']]) + }) +}) + +describe('filterUnposted', () => { + const findings = [{ id: 'f1' }, { id: 'f2' }, { id: 'f3' }] + + it('returns all Findings when postedMap is empty (first-attempt no-op)', () => { + assert.deepEqual(filterUnposted(findings, {}), findings) + }) + + it('returns all Findings when postedMap is undefined', () => { + assert.deepEqual(filterUnposted(findings, undefined), findings) + }) + + it('filters out Findings with postedMap[id].success === true (retry reduces set)', () => { + const postedMap = { f1: { success: true, threadId: 1 }, f2: { success: true, threadId: 2 } } + assert.deepEqual(filterUnposted(findings, postedMap), [{ id: 'f3' }]) + }) + + it('keeps Findings where postedMap[id].success === false (failed → retry)', () => { + const postedMap = { f1: { success: false, threadId: null } } + assert.deepEqual(filterUnposted(findings, postedMap), findings) + }) + + it('keeps Findings with no entry in postedMap (missing entry → un-posted)', () => { + const postedMap = { f1: { success: true, threadId: 1 } } + assert.deepEqual(filterUnposted(findings, postedMap), [{ id: 'f2' }, { id: 'f3' }]) + }) +}) From 7b947e7f38a33f0d6042116951a3a91a09f91640 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 9 Jun 2026 14:57:48 +0200 Subject: [PATCH 2/5] =?UTF-8?q?chore(unic-pr-review):=20bump=202.1.8=20?= =?UTF-8?q?=E2=86=92=202.1.9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../unic-pr-review/.claude-plugin/marketplace.json | 2 +- .../unic-pr-review/.claude-plugin/plugin.json | 2 +- apps/claude-code/unic-pr-review/CHANGELOG.md | 11 +++++++++++ apps/claude-code/unic-pr-review/package.json | 2 +- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json b/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json index 9bf8302b..281c8749 100644 --- a/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json +++ b/apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json @@ -21,7 +21,7 @@ "name": "unic-pr-review", "source": "./", "tags": ["productivity", "code-review", "azure-devops"], - "version": "2.1.8" + "version": "2.1.9" } ] } diff --git a/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json b/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json index f5b53201..dbe8bc73 100644 --- a/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json +++ b/apps/claude-code/unic-pr-review/.claude-plugin/plugin.json @@ -15,5 +15,5 @@ "keywords": ["pr-review", "azure-devops", "jira", "confluence", "code-review", "unic"], "license": "LGPL-3.0-or-later", "name": "unic-pr-review", - "version": "2.1.8" + "version": "2.1.9" } diff --git a/apps/claude-code/unic-pr-review/CHANGELOG.md b/apps/claude-code/unic-pr-review/CHANGELOG.md index d54d1395..bdc210b0 100644 --- a/apps/claude-code/unic-pr-review/CHANGELOG.md +++ b/apps/claude-code/unic-pr-review/CHANGELOG.md @@ -13,6 +13,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - (none) +### Fixed +- (none) + +## [2.1.9] — 2026-06-09 + +### Breaking +- (none) + +### Added +- (none) + ### Changed - (none) diff --git a/apps/claude-code/unic-pr-review/package.json b/apps/claude-code/unic-pr-review/package.json index d11f9037..bcde4233 100644 --- a/apps/claude-code/unic-pr-review/package.json +++ b/apps/claude-code/unic-pr-review/package.json @@ -22,5 +22,5 @@ "verify:changelog": "unic-verify-changelog" }, "type": "module", - "version": "2.1.8" + "version": "2.1.9" } From 8ba90a778ba5570e554271fb46672c707cfaa951 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 9 Jun 2026 15:11:34 +0200 Subject: [PATCH 3/5] fix(unic-pr-review): address review findings for Write Retry helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `filter` subcommand: use atomic tmp+rename write for APPROVED_FILE (prevents corruption on partial write / disk-full) - `filter` subcommand: wrap APPROVED_FILE read in try-catch with clean error - `record` subcommand: wrap recordOutcomes() call in try-catch with clean error - review-pr.md: `CURRENT_ITERATION = WRITE_RETRY_STATE.iteration ?? 1` fallback (defensive against older state files missing the iteration field) - write-outcomes.mjs: fix two JSDoc inaccuracies ("usage message" → "error message", "without touching" → "without mocking" the module system) - README.md: update "without any local state" claim to acknowledge Write Retry state directory (ADR-0015) Co-Authored-By: Claude Sonnet 4.6 --- apps/claude-code/unic-pr-review/README.md | 2 +- .../unic-pr-review/commands/review-pr.md | 2 +- .../scripts/lib/write-outcomes.mjs | 26 ++++++++++++++----- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/apps/claude-code/unic-pr-review/README.md b/apps/claude-code/unic-pr-review/README.md index 2b1d9c7e..c6da7461 100644 --- a/apps/claude-code/unic-pr-review/README.md +++ b/apps/claude-code/unic-pr-review/README.md @@ -124,7 +124,7 @@ flowchart TD w2 --> posted ``` -Read-only by default. The `--post` path writes only what you accept in the Approval Loop, and the Bot Signature in every comment lets the next run recognise its own prior work without any local state ([ADR-0006](docs/adr/0006-iteration-state-in-pr.md)). +Read-only by default. The `--post` path writes only what you accept in the Approval Loop. Bot Signature detection lets the next run recognise its own prior work and increment the Iteration ([ADR-0006](docs/adr/0006-iteration-state-in-pr.md)); if a `--post` partially fails, the local state directory (`/.unic-pr-review//`) lets a re-run on the same machine finish the partial Iteration instead of starting a new one ([ADR-0015](docs/adr/0015-write-retry-completes-partial-iteration.md)). ## Commands diff --git a/apps/claude-code/unic-pr-review/commands/review-pr.md b/apps/claude-code/unic-pr-review/commands/review-pr.md index 52992c9c..52cece67 100644 --- a/apps/claude-code/unic-pr-review/commands/review-pr.md +++ b/apps/claude-code/unic-pr-review/commands/review-pr.md @@ -74,7 +74,7 @@ HEAD_SHA="" node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/write-outcomes.mjs stdout is a JSON object. Route on its `mode`: - **`{ "mode": "none" }`** → no prior state (or it was unreadable); proceed to Step 1.3 (normal review). -- **`{ "mode": "retry", "state": … }`** → **Write Retry**: set `IS_WRITE_RETRY = true`, store `state` as `WRITE_RETRY_STATE`, and set `CURRENT_ITERATION = WRITE_RETRY_STATE.iteration`. Skip Steps 1.3–1.10 entirely (no Fetcher, no mode detection, no aspect fan-out) and go straight to Step 1.11 — the saved approval decisions are reused, nothing is re-prompted. +- **`{ "mode": "retry", "state": … }`** → **Write Retry**: set `IS_WRITE_RETRY = true`, store `state` as `WRITE_RETRY_STATE`, and set `CURRENT_ITERATION = WRITE_RETRY_STATE.iteration ?? 1`. Skip Steps 1.3–1.10 entirely (no Fetcher, no mode detection, no aspect fan-out) and go straight to Step 1.11 — the saved approval decisions are reused, nothing is re-prompted. - **`{ "mode": "stale" }`** → the partial attempt is from a different HEAD (force-push / rebase). Print the Notice, discard the stale state directory, then proceed to Step 1.3 (normal review against the new HEAD): ``` diff --git a/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs b/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs index b22b4c16..ffaf8a46 100644 --- a/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs +++ b/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs @@ -7,7 +7,7 @@ * * Implements the local-dedup half of ADR-0015 (Write Retry completes a * partially-written Iteration). Three pure-ish helpers, all path-based so they - * are unit-testable without touching the module system: + * are unit-testable without mocking the module system: * * - `checkWriteRetry` — classify a re-run as none / retry / stale by comparing * the saved `headSha` to the current HEAD. @@ -165,7 +165,7 @@ export function filterUnposted(approvedFindings, postedMap) { /* ------------------------------------------------------------------ CLI ---- */ /** - * Read a required env var or exit non-zero with a usage message. + * Read a required env var or exit non-zero with an error message. * @param {string} name * @returns {string} */ @@ -201,7 +201,12 @@ if (process.argv[1] === fileURLToPath(import.meta.url)) { process.stderr.write(`write-outcomes: WRITER_RESULT is not valid JSON: ${String(err)}\n`) process.exit(1) } - recordOutcomes(statePath, writer.inlineResults ?? [], writer.summaryResult ?? null) + try { + recordOutcomes(statePath, writer.inlineResults ?? [], writer.summaryResult ?? null) + } catch (err) { + process.stderr.write(`write-outcomes: outcomes not persisted: ${String(err)}\n`) + process.exit(1) + } } else if (subcommand === 'filter') { // env APPROVED_FILE (path) → rewrite it in place to the un-posted Findings const approvedFile = requireEnv('APPROVED_FILE') @@ -213,9 +218,18 @@ if (process.argv[1] === fileURLToPath(import.meta.url)) { } catch { // no readable state → empty posted-map → filter is a no-op } - const approved = JSON.parse(realReadFile(approvedFile, 'utf8')) - const unposted = filterUnposted(approved, postedMap) - realWriteFile(approvedFile, JSON.stringify(unposted), 'utf8') + /** @type {unknown} */ + let approved + try { + approved = JSON.parse(realReadFile(approvedFile, 'utf8')) + } catch (err) { + process.stderr.write(`write-outcomes: could not read APPROVED_FILE: ${String(err)}\n`) + process.exit(1) + } + const unposted = filterUnposted(/** @type {any[]} */ (approved), postedMap) + const tmp = `${approvedFile}.tmp` + realWriteFile(tmp, JSON.stringify(unposted), 'utf8') + realRenameSync(tmp, approvedFile) } else { process.stderr.write(`write-outcomes: unknown subcommand '${subcommand}'\n`) process.exit(1) From 3a5c87b4d7fe783b0ef200cef7530ecbbcc70c26 Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 9 Jun 2026 15:45:24 +0200 Subject: [PATCH 4/5] fix(unic-pr-review): Write Retry re-posts a failed Summary in full MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A --post retry where the Review Summary failed in the prior attempt could silently drop it (all inline Threads already posted → pruned approved set is empty → Writer's zero-Findings short-circuit skips Step 3) or render it from the reduced re-post subset (some inline failed → incomplete Summary). Both violate ADR-0015 AC#1 ("posts the Summary only if it didn't already land"). Why: a single approvedPath fed both the inline Threads (Step 2) and the Summary (Step 3a), so pruning it for inline dedup corrupted the Summary. Fix: stop pruning the approved set. The orchestrator now keeps APPROVED_FILE full and passes the Writer an alreadyPostedFindingIds inline-skip list; the Writer skips those inline Threads but still renders the Summary from every approved Finding. Replaces filterUnposted/the `filter` subcommand with postedFindingIds/`posted-ids`. Amends ADR-0015 decision #4 accordingly. Co-Authored-By: Claude Opus 4.8 --- apps/claude-code/unic-pr-review/CHANGELOG.md | 2 +- .../unic-pr-review/agents/ado-writer.md | 9 ++-- .../unic-pr-review/commands/review-pr.md | 23 +++++----- ...write-retry-completes-partial-iteration.md | 6 ++- .../scripts/lib/write-outcomes.mjs | 43 +++++++------------ .../tests/write-outcomes.test.mjs | 30 ++++++------- 6 files changed, 53 insertions(+), 60 deletions(-) diff --git a/apps/claude-code/unic-pr-review/CHANGELOG.md b/apps/claude-code/unic-pr-review/CHANGELOG.md index bdc210b0..38656549 100644 --- a/apps/claude-code/unic-pr-review/CHANGELOG.md +++ b/apps/claude-code/unic-pr-review/CHANGELOG.md @@ -28,7 +28,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - (none) ### Fixed -- Write Retry completes a partial `--post` Iteration — a re-run with a surviving state directory and unchanged HEAD now resumes the saved Approval Loop and posts only the inline Threads that failed, skips the Summary when it already landed, and never increments the Iteration. Prior behaviour silently dropped all failed Findings by routing the retry to re-review with an empty delta. Adds the tested `scripts/lib/write-outcomes.mjs` helper (`checkWriteRetry` / `recordOutcomes` / `filterUnposted`) and a `summaryAlreadyPosted` input on the ADO Writer. (#236, ADR-0015) +- Write Retry completes a partial `--post` Iteration — a re-run with a surviving state directory and unchanged HEAD now resumes the saved Approval Loop and posts only the inline Threads that failed, re-posts the Review Summary (rendered from the full approved set) only when it did not already land, and never increments the Iteration. Prior behaviour silently dropped all failed Findings by routing the retry to re-review with an empty delta. Adds the tested `scripts/lib/write-outcomes.mjs` helper (`checkWriteRetry` / `recordOutcomes` / `postedFindingIds`) and `alreadyPostedFindingIds` + `summaryAlreadyPosted` inputs on the ADO Writer. (#236, ADR-0015) ## [2.1.8] — 2026-06-09 diff --git a/apps/claude-code/unic-pr-review/agents/ado-writer.md b/apps/claude-code/unic-pr-review/agents/ado-writer.md index 9a7dc1bf..fffd4341 100644 --- a/apps/claude-code/unic-pr-review/agents/ado-writer.md +++ b/apps/claude-code/unic-pr-review/agents/ado-writer.md @@ -24,12 +24,15 @@ You run in one of two modes. In **first-review** mode (default) you consume appr "prId": 42, "approvedPath": "/tmp/unic-pr-review-approved-abc123.json", "iteration": 1, + "alreadyPostedFindingIds": [], "summaryAlreadyPosted": false } ``` `mode` absent or `"first-review"` → run Steps 1–4 (existing path). +`alreadyPostedFindingIds` — optional array of Finding ids (default: absent / `[]`). Each id named here already posted its inline Review Thread in a prior partially-successful `--post` attempt (the Write Retry path, ADR-0015); skip Step 2 for those Findings so they are not duplicated. `approvedPath` still carries the **full** approved set, so the Review Summary (Step 3a) is unaffected. When absent or empty (the default / first-review path) every approved Finding is posted. + `summaryAlreadyPosted` — optional boolean (default: absent / false). When `true`, the Review Summary thread already landed in a prior partially-successful `--post` attempt (the Write Retry path, ADR-0015); skip Steps 3a–3d entirely. No other behaviour changes. ### Re-review mode @@ -80,11 +83,11 @@ Read and parse the JSON array at `approvedPath`. Each element carries: **If the file cannot be read or does not parse to a JSON array**, emit `{ "inlineResults": [], "summaryResult": null, "success": false, "error": "approved-read-failed: " }` and stop — do not proceed to Steps 2–3. Reporting `success: true` here would trigger the state-directory cleanup in the calling command and silently drop every Finding the user just approved. -If the array is empty, skip Steps 2 and 3a–3d; emit a success result with an empty `inlineResults` array and `summaryResult: null`. (An empty array is the legitimate "user approved zero Findings" case, distinct from the read-failure case above.) +If the array is empty, skip Steps 2 and 3a–3d; emit a success result with an empty `inlineResults` array and `summaryResult: null`. (An empty array is the legitimate "user approved zero Findings" case, distinct from the read-failure case above. On a Write Retry the array is always the **full** approved set — never empty — so this short-circuit is a first-review-only path; a Write Retry where every Finding already posted inline reaches Step 2, skips them all via `alreadyPostedFindingIds`, and still runs Step 3 to post a not-yet-landed Summary.) ### Step 2 — Post inline Review Threads -For **each** approved Finding, execute Steps 2a–2d in order. +For **each** approved Finding whose `id` is **not** listed in `alreadyPostedFindingIds`, execute Steps 2a–2d in order. Skip any Finding whose `id` **is** in `alreadyPostedFindingIds` — do not post it and do not add it to `inlineResults`; it already landed in a prior attempt (Write Retry, ADR-0015), and its prior outcome is preserved in `state.json`. When `alreadyPostedFindingIds` is absent or empty, every approved Finding is posted. #### 2a — Render the Inline Comment @@ -165,7 +168,7 @@ Failure here is silent — continue with the next Finding regardless. #### 3a — Render the Review Summary -Build `FINDINGS_JSON` from **all** approved Findings — include the full Finding shape required by `parseFinding` inside `render-summary.mjs`: at minimum `confidence`, `filePath`, `startLine`, `title`, `body`, and optionally `suggestion`. Include `severity` too (already on the approved Finding). Pass an empty `positiveObservations` array: +Build `FINDINGS_JSON` from **all** approved Findings — the entire `approvedPath` array, including any whose inline Thread was skipped via `alreadyPostedFindingIds`, so the Summary reflects every Finding regardless of which inline Threads were (re)posted this attempt. Include the full Finding shape required by `parseFinding` inside `render-summary.mjs`: at minimum `confidence`, `filePath`, `startLine`, `title`, `body`, and optionally `suggestion`. Include `severity` too (already on the approved Finding). Pass an empty `positiveObservations` array: ```sh FINDINGS_JSON='{"findings":[],"positiveObservations":[]}' \ diff --git a/apps/claude-code/unic-pr-review/commands/review-pr.md b/apps/claude-code/unic-pr-review/commands/review-pr.md index 52cece67..ea0a6f65 100644 --- a/apps/claude-code/unic-pr-review/commands/review-pr.md +++ b/apps/claude-code/unic-pr-review/commands/review-pr.md @@ -324,13 +324,13 @@ node "${CLAUDE_PLUGIN_ROOT}/scripts/approval-loop.mjs" \ - **Exit 2** (non-TTY without --yes): print `"approval-loop: --post requires an interactive terminal or --yes."` and stop. - **Any other non-zero exit**: relay stderr verbatim and stop. -**Write Retry post-loop (`IS_WRITE_RETRY` is true):** after the Approval Loop writes `APPROVED_FILE`, filter it down to only the Findings that were **not** already successfully posted, using the `postedMap` persisted in `state.json`: +**Write Retry post-loop (`IS_WRITE_RETRY` is true):** after the Approval Loop writes `APPROVED_FILE`, leave that file **intact** — the Writer renders the Review Summary from the **full** approved set (`ado-writer.md` Step 3a), so pruning it would drop already-posted Findings from the Summary. Instead, collect the ids of the Findings that already posted successfully in the prior attempt, from the `postedMap` persisted in `state.json`: ```sh -APPROVED_FILE="" node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/write-outcomes.mjs" filter "" +node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/write-outcomes.mjs" posted-ids "" ``` -Then set `SUMMARY_ALREADY_POSTED = WRITE_RETRY_STATE.summaryPosted === true` for the Writer input in Step 1.12. +stdout is a JSON array of Finding ids. Capture it as `ALREADY_POSTED_IDS` (default `[]`); the Writer skips the inline Thread for each of these so only the Findings that failed are re-posted, while the Summary still reflects every approved Finding. Then set `SUMMARY_ALREADY_POSTED = WRITE_RETRY_STATE.summaryPosted === true` for the Writer input in Step 1.12. **6. Clean up the findings temp file.** @@ -353,17 +353,18 @@ Use the Agent tool to launch `unic-pr-review:ado-writer`. Provide: } ``` -**Write Retry delta (`IS_WRITE_RETRY` is true):** use `CURRENT_ITERATION` (from `WRITE_RETRY_STATE.iteration`) instead of `1`, and add `summaryAlreadyPosted` so the Writer skips the Summary when it already landed: +**Write Retry delta (`IS_WRITE_RETRY` is true):** use `CURRENT_ITERATION` (from `WRITE_RETRY_STATE.iteration`) instead of `1`, add `alreadyPostedFindingIds` so the Writer skips the inline Threads that already landed, and add `summaryAlreadyPosted` so the Writer skips the Summary when it already landed. `approvedPath` stays the **full** approved set so the Summary still reflects every Finding: ```json { - "orgUrl": "", - "project": "", - "repo": "", - "prId": , - "approvedPath": "", - "iteration": , - "summaryAlreadyPosted": + "orgUrl": "", + "project": "", + "repo": "", + "prId": , + "approvedPath": "", + "iteration": , + "alreadyPostedFindingIds": , + "summaryAlreadyPosted": } ``` diff --git a/apps/claude-code/unic-pr-review/docs/adr/0015-write-retry-completes-partial-iteration.md b/apps/claude-code/unic-pr-review/docs/adr/0015-write-retry-completes-partial-iteration.md index 2700f243..5d92aa55 100644 --- a/apps/claude-code/unic-pr-review/docs/adr/0015-write-retry-completes-partial-iteration.md +++ b/apps/claude-code/unic-pr-review/docs/adr/0015-write-retry-completes-partial-iteration.md @@ -26,7 +26,9 @@ Introduce **Write Retry**: a re-run of `--post` that finishes a partially-writte 3. **Short-circuit routing.** When the state directory is present and HEAD matches, the orchestrator skips the Fetcher, mode detection, and the aspect fan-out entirely. It resumes the Approval Loop (which re-prompts nothing — all decisions are already saved) at the **same** Iteration number stored in `state.json`. -4. **Dedup is local; the Writer stays mostly dumb.** After a write, each Finding's post outcome (`threadId` / success) and a `summaryPosted` flag are persisted into `state.json`. On retry the orchestrator passes the Writer only the Findings that did not already post, and sets `summaryAlreadyPosted` so the Writer skips the Summary (Step 3) when it already landed. On a first attempt the posted-map is empty, so behaviour is unchanged. A fully-successful retry then deletes the state directory per ADR-0014. +4. **Dedup is local; the Writer stays mostly dumb.** After a write, each Finding's post outcome (`threadId` / success) and a `summaryPosted` flag are persisted into `state.json`. On retry the orchestrator passes the Writer the **full** approved set plus two skip signals: `alreadyPostedFindingIds` (the ids that already posted inline) and `summaryAlreadyPosted`. The Writer skips the inline Thread for each listed id and skips the Summary (Step 3) when it already landed — but the Summary (Step 3a) is always rendered from the full approved set, so a Summary that failed in the prior attempt is re-posted complete, never from the reduced re-post subset. On a first attempt both signals are empty/false, so behaviour is unchanged. A fully-successful retry then deletes the state directory per ADR-0014. + + > The skip-list is passed to the Writer rather than pruning its input file because a single `approvedPath` feeds both the inline Threads (Step 2) and the Review Summary (Step 3a). Pruning that file to the un-posted Findings — the original design here — would have rendered an incomplete Summary, or, when every inline Thread had already landed, dropped a still-failed Summary entirely (the array would be empty and the Writer's zero-Findings short-circuit would skip Step 3). Keeping the file full and skipping by id keeps the inline-dedup local while letting the Summary stay complete. ## Considered alternatives @@ -37,7 +39,7 @@ Introduce **Write Retry**: a re-run of `--post` that finishes a partially-writte ## Consequences - `state.json` gains a per-Finding post outcome and a `summaryPosted` flag (written by the orchestrator after the Writer returns, via a small tested helper). -- The ADO Writer's first-review input gains an optional `summaryAlreadyPosted` flag; everything else is unchanged. +- The ADO Writer's first-review input gains an optional `alreadyPostedFindingIds` array (inline-skip list) and an optional `summaryAlreadyPosted` flag; everything else is unchanged. The Writer never prunes its own input, so the Summary always reflects the full approved set. - The orchestrator gains a top-of-flow Write Retry check (state directory present + `headSha` match) before the Fetcher. - Cross-machine retry is **not** covered: without the local state directory the re-run still routes to re-review. Acceptable given the resume contract was already local (ADR-0014). - The Step 1.12 warning is rewritten to describe Write Retry accurately and to state the cross-machine and HEAD-moved caveats. diff --git a/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs b/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs index ffaf8a46..141d372f 100644 --- a/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs +++ b/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs @@ -14,8 +14,8 @@ * - `recordOutcomes` — after a write, persist each Finding's post outcome and * a `summaryPosted` flag into `state.json` (atomic write), * run BEFORE the success-gated cleanup (ADR-0014). - * - `filterUnposted` — drop Findings that already posted so a retry re-posts - * only the ones that failed (first attempt → no-op). + * - `postedFindingIds` — list the Finding ids that already posted so a retry can + * skip re-posting them inline (first attempt → empty list). * * Owned by the `review-pr` orchestrator (Steps 1.2a, 1.11, 1.12a). The CLI * dispatcher at the bottom exposes the three helpers as a real script file so @@ -148,18 +148,17 @@ export function recordOutcomes(statePath, inlineResults, summaryResult, deps = { } /** - * Drop Findings that already posted successfully so a retry re-posts only the - * failures. An empty or absent posted-map returns the input unchanged — the - * first-attempt no-op that keeps normal behaviour intact. + * List the Finding ids that already posted successfully, so a retry can skip + * re-posting them as inline Threads while the Review Summary is still rendered + * from the full approved set (ADR-0015). An empty or absent posted-map returns + * an empty list — the first-attempt no-op that keeps normal behaviour intact. * - * @template {{ id?: string }} F - * @param {F[]} approvedFindings * @param {PostedMap} [postedMap] - * @returns {F[]} + * @returns {string[]} */ -export function filterUnposted(approvedFindings, postedMap) { - if (!postedMap || Object.keys(postedMap).length === 0) return approvedFindings - return approvedFindings.filter((f) => postedMap[/** @type {string} */ (f.id)]?.success !== true) +export function postedFindingIds(postedMap) { + if (!postedMap) return [] + return Object.keys(postedMap).filter((id) => postedMap[id]?.success === true) } /* ------------------------------------------------------------------ CLI ---- */ @@ -207,29 +206,19 @@ if (process.argv[1] === fileURLToPath(import.meta.url)) { process.stderr.write(`write-outcomes: outcomes not persisted: ${String(err)}\n`) process.exit(1) } - } else if (subcommand === 'filter') { - // env APPROVED_FILE (path) → rewrite it in place to the un-posted Findings - const approvedFile = requireEnv('APPROVED_FILE') + } else if (subcommand === 'posted-ids') { + // reads state.postedMap → stdout: JSON array of already-posted Finding ids. + // The orchestrator passes these to the Writer as an inline-skip list while + // the APPROVED_FILE stays full so the Summary still renders every Finding. /** @type {PostedMap} */ let postedMap = {} try { const state = JSON.parse(realReadFile(statePath, 'utf8')) if (state && typeof state === 'object' && state.postedMap) postedMap = state.postedMap } catch { - // no readable state → empty posted-map → filter is a no-op + // no readable state → empty posted-map → empty id list (retry re-posts all) } - /** @type {unknown} */ - let approved - try { - approved = JSON.parse(realReadFile(approvedFile, 'utf8')) - } catch (err) { - process.stderr.write(`write-outcomes: could not read APPROVED_FILE: ${String(err)}\n`) - process.exit(1) - } - const unposted = filterUnposted(/** @type {any[]} */ (approved), postedMap) - const tmp = `${approvedFile}.tmp` - realWriteFile(tmp, JSON.stringify(unposted), 'utf8') - realRenameSync(tmp, approvedFile) + process.stdout.write(JSON.stringify(postedFindingIds(postedMap))) } else { process.stderr.write(`write-outcomes: unknown subcommand '${subcommand}'\n`) process.exit(1) diff --git a/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs b/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs index 51c64a82..c6d864fa 100644 --- a/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs +++ b/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs @@ -7,7 +7,7 @@ import { mkdtempSync, readFileSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { describe, it } from 'node:test' -import { checkWriteRetry, filterUnposted, recordOutcomes } from '../scripts/lib/write-outcomes.mjs' +import { checkWriteRetry, postedFindingIds, recordOutcomes } from '../scripts/lib/write-outcomes.mjs' const HEAD = 'a'.repeat(40) const OTHER = 'b'.repeat(40) @@ -146,29 +146,27 @@ describe('recordOutcomes', () => { }) }) -describe('filterUnposted', () => { - const findings = [{ id: 'f1' }, { id: 'f2' }, { id: 'f3' }] - - it('returns all Findings when postedMap is empty (first-attempt no-op)', () => { - assert.deepEqual(filterUnposted(findings, {}), findings) +describe('postedFindingIds', () => { + it('returns [] when postedMap is empty (first-attempt no-op)', () => { + assert.deepEqual(postedFindingIds({}), []) }) - it('returns all Findings when postedMap is undefined', () => { - assert.deepEqual(filterUnposted(findings, undefined), findings) + it('returns [] when postedMap is undefined', () => { + assert.deepEqual(postedFindingIds(undefined), []) }) - it('filters out Findings with postedMap[id].success === true (retry reduces set)', () => { + it('returns ids with success === true (retry skips these inline)', () => { const postedMap = { f1: { success: true, threadId: 1 }, f2: { success: true, threadId: 2 } } - assert.deepEqual(filterUnposted(findings, postedMap), [{ id: 'f3' }]) + assert.deepEqual(postedFindingIds(postedMap).sort(), ['f1', 'f2']) }) - it('keeps Findings where postedMap[id].success === false (failed → retry)', () => { - const postedMap = { f1: { success: false, threadId: null } } - assert.deepEqual(filterUnposted(findings, postedMap), findings) + it('omits ids with success === false (failed → still re-posted inline)', () => { + const postedMap = { f1: { success: false, threadId: null }, f2: { success: true, threadId: 2 } } + assert.deepEqual(postedFindingIds(postedMap), ['f2']) }) - it('keeps Findings with no entry in postedMap (missing entry → un-posted)', () => { - const postedMap = { f1: { success: true, threadId: 1 } } - assert.deepEqual(filterUnposted(findings, postedMap), [{ id: 'f2' }, { id: 'f3' }]) + it('returns [] when every entry failed (nothing posted yet)', () => { + const postedMap = { f1: { success: false, threadId: null } } + assert.deepEqual(postedFindingIds(postedMap), []) }) }) From fb6bcb3b227ce7dc44ea569544ad72309b95c16e Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Tue, 9 Jun 2026 16:01:23 +0200 Subject: [PATCH 5/5] fix(unic-pr-review): surface corrupt Write Retry state instead of silent re-review Three review follow-ups on the Write Retry path: - checkWriteRetry now returns `corrupt` (not `none`) when a state directory survived but its state.json is unreadable/invalid. A surviving dir means the prior --post did not complete, so treating it as "no prior attempt" silently re-entered the empty-delta re-review path that drops Findings (#236). The orchestrator prints a Notice and runs a normal review, symmetric with `stale`. - recordOutcomes throws a clear TypeError on a non-array inlineResults so a malformed Writer result reports its real cause, not a generic persistence miss. - Step 1.12 HEAD-moved caveat: a discarded stale state runs a normal review (re-review only if a prior comment already carried the Iteration Marker), not always a re-review. Co-Authored-By: Claude Opus 4.8 --- .../unic-pr-review/commands/review-pr.md | 14 ++++++++++++-- ...-write-retry-completes-partial-iteration.md | 1 + .../scripts/lib/write-outcomes.mjs | 18 +++++++++++++----- .../tests/write-outcomes.test.mjs | 18 ++++++++++++++---- 4 files changed, 40 insertions(+), 11 deletions(-) diff --git a/apps/claude-code/unic-pr-review/commands/review-pr.md b/apps/claude-code/unic-pr-review/commands/review-pr.md index ea0a6f65..8cad1802 100644 --- a/apps/claude-code/unic-pr-review/commands/review-pr.md +++ b/apps/claude-code/unic-pr-review/commands/review-pr.md @@ -73,7 +73,7 @@ HEAD_SHA="" node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/write-outcomes.mjs stdout is a JSON object. Route on its `mode`: -- **`{ "mode": "none" }`** → no prior state (or it was unreadable); proceed to Step 1.3 (normal review). +- **`{ "mode": "none" }`** → no state directory at all (no prior `--post` attempt); proceed to Step 1.3 (normal review). - **`{ "mode": "retry", "state": … }`** → **Write Retry**: set `IS_WRITE_RETRY = true`, store `state` as `WRITE_RETRY_STATE`, and set `CURRENT_ITERATION = WRITE_RETRY_STATE.iteration ?? 1`. Skip Steps 1.3–1.10 entirely (no Fetcher, no mode detection, no aspect fan-out) and go straight to Step 1.11 — the saved approval decisions are reused, nothing is re-prompted. - **`{ "mode": "stale" }`** → the partial attempt is from a different HEAD (force-push / rebase). Print the Notice, discard the stale state directory, then proceed to Step 1.3 (normal review against the new HEAD): @@ -85,6 +85,16 @@ stdout is a JSON object. Route on its `mode`: node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/clear-state-dir.mjs" "" ``` +- **`{ "mode": "corrupt" }`** → a state directory survived (the prior `--post` did **not** complete) but its `state.json` is unreadable or invalid, so the failed Findings cannot be auto-resumed. Print the Notice, discard the unusable state directory, then proceed to Step 1.3 (normal review). Unlike `none`, this is **not** silent — the user must know a prior attempt's failed comments will not be re-posted automatically: + + ``` + Notice: A prior --post state directory exists but its state.json is unreadable or invalid. Discarding it and running a normal review against the current HEAD — any Findings that failed to post in the prior attempt are NOT auto-resumed, so re-check the PR for missing comments. + ``` + + ```sh + node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/clear-state-dir.mjs" "" + ``` + #### Step 1.3 — Invoke the ADO Fetcher agent Use the Agent tool to launch the agent identified by `FETCHER_AGENT` (e.g. `unic-pr-review:ado-fetcher`). Provide: @@ -387,7 +397,7 @@ If `success` is `false` (any thread failed), warn the user: Caveats: - Cross-machine: Write Retry is local. A retry from a different clone has no state directory and falls back to re-review, which sees an empty delta and produces zero Findings. -- HEAD moved: if the branch is force-pushed or rebased between the failed attempt and the retry, the stale state is discarded and a normal re-review runs instead. +- HEAD moved: if the branch is force-pushed or rebased between the failed attempt and the retry, the stale state is discarded and a normal review runs instead (re-review if a prior comment already carried the Iteration Marker, first-review otherwise). - Do not use --yes: --post --yes bypasses the Approval Loop entirely and re-posts all approved Findings, creating duplicate comments for the ones that already succeeded. ``` diff --git a/apps/claude-code/unic-pr-review/docs/adr/0015-write-retry-completes-partial-iteration.md b/apps/claude-code/unic-pr-review/docs/adr/0015-write-retry-completes-partial-iteration.md index 5d92aa55..f9b0371c 100644 --- a/apps/claude-code/unic-pr-review/docs/adr/0015-write-retry-completes-partial-iteration.md +++ b/apps/claude-code/unic-pr-review/docs/adr/0015-write-retry-completes-partial-iteration.md @@ -42,4 +42,5 @@ Introduce **Write Retry**: a re-run of `--post` that finishes a partially-writte - The ADO Writer's first-review input gains an optional `alreadyPostedFindingIds` array (inline-skip list) and an optional `summaryAlreadyPosted` flag; everything else is unchanged. The Writer never prunes its own input, so the Summary always reflects the full approved set. - The orchestrator gains a top-of-flow Write Retry check (state directory present + `headSha` match) before the Fetcher. - Cross-machine retry is **not** covered: without the local state directory the re-run still routes to re-review. Acceptable given the resume contract was already local (ADR-0014). +- A surviving-but-unreadable `state.json` is classified `corrupt` (distinct from `none`): the orchestrator prints a Notice and runs a normal review, rather than silently re-entering the empty-delta re-review path that drops Findings. This keeps the staleness guard's "discard + Notice" posture symmetric across the stale and corrupt cases. - The Step 1.12 warning is rewritten to describe Write Retry accurately and to state the cross-machine and HEAD-moved caveats. diff --git a/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs b/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs index 141d372f..8e52df30 100644 --- a/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs +++ b/apps/claude-code/unic-pr-review/scripts/lib/write-outcomes.mjs @@ -9,8 +9,8 @@ * partially-written Iteration). Three pure-ish helpers, all path-based so they * are unit-testable without mocking the module system: * - * - `checkWriteRetry` — classify a re-run as none / retry / stale by comparing - * the saved `headSha` to the current HEAD. + * - `checkWriteRetry` — classify a re-run as none / retry / stale / corrupt by + * comparing the saved `headSha` to the current HEAD. * - `recordOutcomes` — after a write, persist each Finding's post outcome and * a `summaryPosted` flag into `state.json` (atomic write), * run BEFORE the success-gated cleanup (ADR-0014). @@ -80,7 +80,7 @@ import { approvalStateDirPath } from './cache-paths.mjs' * @param {string} statePath - absolute path to `/state.json` * @param {string} currentHead - current `git rev-parse HEAD` * @param {WriteOutcomeDeps} [deps] - * @returns {{ mode: 'none' } | { mode: 'retry', state: WriteRetryState } | { mode: 'stale' }} + * @returns {{ mode: 'none' } | { mode: 'retry', state: WriteRetryState } | { mode: 'stale' } | { mode: 'corrupt' }} */ export function checkWriteRetry(statePath, currentHead, deps = {}) { const existsSync = deps.existsSync ?? realExistsSync @@ -88,15 +88,19 @@ export function checkWriteRetry(statePath, currentHead, deps = {}) { if (!existsSync(statePath)) return { mode: 'none' } + // The state file exists, so a prior --post left resumable state behind. If it + // is unreadable or invalid we cannot resume it — but that is NOT "no prior + // attempt". Surface `corrupt` so the orchestrator can warn the user instead of + // silently re-entering the empty-delta re-review path that drops Findings (#236). let state try { state = JSON.parse(readFile(statePath, 'utf8')) } catch { - return { mode: 'none' } + return { mode: 'corrupt' } } if (!state || typeof state !== 'object' || typeof state.headSha !== 'string') { - return { mode: 'none' } + return { mode: 'corrupt' } } if (state.headSha === currentHead) return { mode: 'retry', state } @@ -123,6 +127,10 @@ export function recordOutcomes(statePath, inlineResults, summaryResult, deps = { const writeFile = deps.writeFile ?? realWriteFile const renameSync = deps.renameSync ?? realRenameSync + if (!Array.isArray(inlineResults)) { + throw new TypeError('recordOutcomes: inlineResults must be an array') + } + let state = /** @type {Record} */ ({}) try { const parsed = JSON.parse(readFile(statePath, 'utf8')) diff --git a/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs b/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs index c6d864fa..9e63afc4 100644 --- a/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs +++ b/apps/claude-code/unic-pr-review/tests/write-outcomes.test.mjs @@ -48,15 +48,17 @@ describe('checkWriteRetry', () => { assert.deepEqual(checkWriteRetry(path, HEAD), { mode: 'stale' }) }) - it('returns {mode:"none"} when state file exists but is malformed JSON', () => { + it('returns {mode:"corrupt"} when state file exists but is malformed JSON', () => { + // The dir survived (prior --post did not complete) but state.json is garbage: + // not "no prior attempt" — the orchestrator must warn, not silently re-review. const path = join(tempDir(), 'state.json') writeFileSync(path, '{not json', 'utf8') - assert.deepEqual(checkWriteRetry(path, HEAD), { mode: 'none' }) + assert.deepEqual(checkWriteRetry(path, HEAD), { mode: 'corrupt' }) }) - it('returns {mode:"none"} when state file exists but headSha is not a string', () => { + it('returns {mode:"corrupt"} when state file exists but headSha is not a string', () => { const path = writeState({ iteration: 2 }) // no headSha - assert.deepEqual(checkWriteRetry(path, HEAD), { mode: 'none' }) + assert.deepEqual(checkWriteRetry(path, HEAD), { mode: 'corrupt' }) }) it('uses injected deps without touching the real filesystem', () => { @@ -128,6 +130,14 @@ describe('recordOutcomes', () => { assert.equal(state.summaryPosted, true) }) + it('throws a clear TypeError when inlineResults is not an array (malformed Writer result)', () => { + const path = writeState({ headSha: HEAD }) + assert.throws( + () => recordOutcomes(path, /** @type {any} */ ({ not: 'an array' }), null), + /inlineResults must be an array/ + ) + }) + it('writes atomically via tmp + rename', () => { const writes = /** @type {string[]} */ ([]) const renames = /** @type {[string, string][]} */ ([])