fix(unic-pr-review): Write Retry completes a partial --post Iteration (no silently dropped Findings)

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"
     }
   ]
 }

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"
 }

apps/claude-code/unic-pr-review/CHANGELOG.md

@@ -13,11 +13,22 @@ 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)
 
 ### 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, 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
 

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 (`<cwd>/.unic-pr-review/<key>/`) 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
 

apps/claude-code/unic-pr-review/agents/ado-writer.md

@@ -23,12 +23,18 @@ 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,
+  "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
 
 ```json
@@ -77,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: <message>" }` 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
 
@@ -158,9 +164,11 @@ 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:
+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":[<full approved Finding objects>],"positiveObservations":[]}' \

apps/claude-code/unic-pr-review/commands/review-pr.md

@@ -53,6 +53,48 @@ process.stdout.write(createHash('sha256').update('<URL>','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="<HEAD_SHA>" node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/write-outcomes.mjs" check "<PR_KEY>"
+```
+
+stdout is a JSON object. Route on its `mode`:
+
+- **`{ "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):
+
+  ```
+  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" "<PR_KEY>"
+  ```
+
+- **`{ "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" "<PR_KEY>"
+  ```
+
 #### 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 +276,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="<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 +334,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`, 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
+node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/write-outcomes.mjs" posted-ids "<PR_KEY>"
+```
+
+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.**
 
 ```sh
@@ -302,6 +363,21 @@ 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`, 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":                  "<PR_REF.orgUrl>",
+  "project":                 "<PR_REF.project>",
+  "repo":                    "<PR_REF.repo>",
+  "prId":                    <PR_REF.prId>,
+  "approvedPath":            "<APPROVED_FILE>",
+  "iteration":               <CURRENT_ITERATION>,
+  "alreadyPostedFindingIds": <ALREADY_POSTED_IDS>,
+  "summaryAlreadyPosted":    <SUMMARY_ALREADY_POSTED>
+}
+```
+
 Wait for the agent to complete. It returns:
 
 ```json
@@ -317,9 +393,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 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.
 ```
 
+#### 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='<writerResult JSON>' node "${CLAUDE_PLUGIN_ROOT}/scripts/lib/write-outcomes.mjs" record "<PR_KEY>"
+```
+
+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):

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,8 @@ 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).
+- 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.

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"
 }