fix(unic-pr-review): Write Retry completes a partial --post Iteration (no silently dropped Findings)#242
Merged
orioltf merged 5 commits intoJun 9, 2026
Conversation
… (no silently dropped Findings) 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) <noreply@anthropic.com>
- `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 <noreply@anthropic.com>
Member
Author
⚡ Self-Fix Report (Aggressive)Status: COMPLETE Fixes Applied (7 total)
View all fixes
Tests Added(none) — 653 existing tests all pass; no new coverage gaps required fixes. Skipped (3)
Suggested Follow-up Issues(none) Validation✅ Type check | ✅ Lint | ✅ Tests (653 passed) Self-fix by Archon · aggressive mode · fixes pushed to |
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 <noreply@anthropic.com>
…ent 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 <noreply@anthropic.com>
orioltf
deleted the
archon/task-feature-unic-pr-review-236-write-retry-partial-ite
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A
--postrun that partially fails (some inline Review Threads post, others hit a transient error) left a surviving Approval Loop state directory (ADR-0014) so the run was meant to be resumable. But the resume was never implemented, and the actual behaviour was worse than a no-op: on re-run the ADO Fetcher detects a prior Iteration Marker → routes to re-review (ADR-0009) → re-review uses a delta diff against the prior Revision (ADR-0007) → on an immediate retry HEAD is unchanged → the delta is empty → aspect agents produce zero Findings → the Approval Loop is skipped → every Finding that failed to post is silently dropped. The Step 1.12 warning promised "re-posts only the threads that failed" — a promise that didn't exist in code.What (locked Write Retry design — ADR-0015)
--postre-run ⇒ the prior write didn't complete.state.json.headSha == current HEAD. If HEAD moved, discard the stale state dir, print a Notice, and run a normal Review against the new HEAD.state.json— nothing is re-prompted (decisions are saved).recordOutcomespersists each Finding's post outcome (postedMap) + a stickysummaryPostedflag intostate.jsonbefore the success-gated cleanup. On retry the orchestrator passes the Writer only un-posted Findings (filterUnposted) and setssummaryAlreadyPostedso the Summary (Step 3) is skipped when it already landed. First attempt ⇒ empty posted-map ⇒ unchanged behaviour; fully-successful retry ⇒ state dir deleted (ADR-0014).summaryAlreadyPosted; Step 3 skipped when true. No other Writer behaviour changes.Implementation note (cross-platform)
write-outcomes.mjsis invoked through a real-script CLI dispatcher (check/record/filter, env-before-node, key passed positionally), not an inlinenode -e "import … from '<abs path>'". Embedding an absolute path in an ESM import specifier breaks on Windows — the same defect that retired the oldclear-state-dirone-liner (#227). The three exports stay pure and are fully unit-tested.Tests
tests/write-outcomes.test.mjs— node:test coverage of the persist/filter helper: record outcomes, subtract posted-ok, sticky summary flag, atomic write, first-attempt no-op (empty posted-map), retry filters to un-posted, and the stale-HEAD discard path.Out of scope (untouched)
Re-review classification / delta-diff contract (ADR-0007); cross-machine retry (documented limitation, caveat in the Step 1.12 warning); a fully idempotent server-side-matching Writer (rejected in ADR-0015). No code read or copied from
apps/claude-code/pr-review/(Clean-Slate Doctrine).Fixes #236
🤖 Generated with Claude Code