feat(unic-spec-review): gate dedup posts when comparison is incomplete

[orioltf]

Why

The dedup-matcher CLI dropped the truncated flag from the comments object, emitting a bare DedupResult[]. A Finding that duplicated a comment living beyond the pagination cap was therefore decided post with the same apparent authority as a fully-checked Finding — silently breaking ADR-0002's premise (similarity against all existing comments). This was the silent-failure-hunter CRITICAL finding from PR #237, triaged to #238.

What changed

Threads comparison completeness structurally, per the locked design in ADR-0005 (grilled 2026-06-09):

• scripts/lib/dedup-matcher.mjs — CLI main() reads truncated from the comments object and emits a run-level { truncated, results } envelope. matchDedup's signature and purity are unchanged — the completeness axis lives in the CLI and command, not in the pure matcher.
• commands/review-spec.md — Step 10b parses .results off the envelope (failure fallback still treats every decision as post); Step 10a computes COMPARISON_INCOMPLETE = truncated OR (non-auth errors), converging two advisory warnings into one gate; Step 10c shows [?incomplete] on clean posts + a warning preamble; Step 10d adds a single run-level confirmation before the first clean-post write. skip/flag keep their existing per-Finding gates.
• tests/dedup-matcher.test.mjs — 5 new node:test CLI envelope tests with injected { comments, truncated } (no live services).
• CHANGELOG.md + patch bump → 0.1.9.

Write-safety invariant preserved: bare /review-spec stays read-only; --post cancellable at every step. MAX_PAGES left at 50 (out of scope per ADR-0005 §5).

Verification

Check	Result
pnpm --filter unic-spec-review typecheck	✅
pnpm --filter unic-spec-review test	✅ 372 pass
pnpm --filter unic-spec-review verify:changelog	✅ → 0.1.9
pnpm check	✅ (2 pre-existing infos)

Closes #238.

🤖 Generated with Claude Code

[orioltf]

🔍 Comprehensive PR Review

PR: #241 — feat(unic-spec-review): gate dedup posts when comparison is incomplete
Reviewed by: 4 specialized agents (code-review, error-handling, comment-quality, docs-impact)
Date: 2026-06-09

---

Summary

Implementation is sound. matchDedup signature is preserved, the { truncated, results } envelope is backward-compatible, CI passes on all 6 platform/Node combinations, and the CHANGELOG is correctly formatted. Two MEDIUM findings exist in command prose; no blocking bugs in source code.

Verdict: ✅ APPROVE

Severity	Count
🔴 CRITICAL	0
🟠 HIGH	0
🟡 MEDIUM	2
🟢 LOW	5

---

🟡 Medium Issues (Need Decision)

1. SKIP_CLEAN_POSTS silently cancels approved skip/flag overrides

📍 apps/claude-code/unic-spec-review/commands/review-spec.md:513-515

The prose at line 479 promises: "skip and flag Findings keep their existing per-Finding gates and are unaffected." But SKIP_CLEAN_POSTS is checked at the top of the #### If decision is 'post' (or an override was approved above): block — so an approved skip/flag override is also blocked when the run-level confirm was declined.

Reproduced flow: User selects a 'post' finding (A) + a 'skip' near-dup (B), COMPARISON_INCOMPLETE=true. Declines run-level confirm → SKIP_CLEAN_POSTS=true. Per-Finding gate for B fires → user says y. Falls through to the combined branch → "Skipped (incomplete comparison)" despite explicit override consent.

Two fix options

Option A — Fix the behaviour (code-review recommendation): split the override branch so SKIP_CLEAN_POSTS only applies to clean posts:

#### If an override was approved above (skip or flag, user said y):

Post the Finding using steps 1–3 below. The run-level `SKIP_CLEAN_POSTS` flag does not apply — the reviewer explicitly consented to post despite the near-duplicate.

#### If decision is `'post'` (clean post, no duplicate found):

When `SKIP_CLEAN_POSTS` is true …

Option B — Fix the prose (error-handling recommendation): update line 479 to accurately state that overrides are also held:

- If the user answers anything else: set `SKIP_CLEAN_POSTS = true`.
  Clean-post Findings, and approved override posts (`skip`/`flag` whose per-Finding gate was
  accepted), will both be skipped. The per-Finding prompts are still presented; only the final
  write step is bypassed by `SKIP_CLEAN_POSTS`.

---

2. ADR README marks ADR-0005 as pending after implementation

📍 apps/claude-code/unic-spec-review/docs/adr/README.md

Current text: "ADR-0005 … is pending — scoped in #238." This PR completes the implementation. Future agents reading this will think ADR-0005 is unimplemented.

Suggested one-line fix (matches existing pattern):

ADR-0002 (similarity de-dup) is implemented as of S8 (`dedup-matcher.mjs`). ADR-0005 (gate incomplete comparisons) refines ADR-0002 and is implemented as of v0.1.9 (`dedup-matcher.mjs` envelope + `review-spec.md` Steps 10a–10d).

---

🟢 Low Issues

View 5 low-priority suggestions

#	Issue	Location	Suggestion
1	Badge table missing spaces around AND — `'post'`AND`COMPARISON_INCOMPLETE` is ambiguous for LLM parsers	review-spec.md:457	`'post'` AND `COMPARISON_INCOMPLETE` is false
2	runDedupCli never removes mkdtempSync temp dirs (accumulates on repeated runs)	dedup-matcher.test.mjs:280-292	Wrap spawnSync in try/finally + rmSync(dir, { recursive: true, force: true })
3	No symmetric test for --comments-file missing (only --findings-file has an exit-1 test)	dedup-matcher.test.mjs:350-357	Add it('exits 1 when --comments-file is missing', ...)
4	10b failure-fallback prose gap: exit-0 with absent results key is not covered	review-spec.md:429	Extend: "non-zero exit, parse error, or parsed.results is not an array"
5	Test description says "error JSON" but assertion is plain substring includes('Usage')	dedup-matcher.test.mjs:360	Strengthen: JSON.parse(res.stderr) + typeof err.error === 'string'

---

✅ What's Good

• matchDedup signature invariant: exported function byte-for-byte unchanged, purity preserved
• Backward compatibility: !Array.isArray(commentsRaw) && commentsRaw?.truncated === true handles legacy bare-array shape cleanly; undefined → false correctly
• COMPARISON_INCOMPLETE architecture: unifying truncation + partial read errors into a single boolean gate is the right abstraction
• Run-level confirm placement: fires once before per-Finding processing, not per-Finding
• 5 new CLI envelope tests: cover all envelope shapes + dedup + exit-1; no live services; pass on macOS/ubuntu/windows × Node 22/24
• JSDoc on main(): accurately describes input duality, strict === true check, and backward-compat rule
• "do not re-read" note in Step 10b: valuable "don't do the obvious thing" guard against dual-source confusion
• CHANGELOG format: ## [0.1.9] — 2026-06-09 ✅; all three version files consistent ✅
• Zero new runtime dependencies ✅

---

Reviewed by Archon comprehensive-pr-review workflow — 4 agents
Artifacts: .archon/workspaces/unic/unic-agents-plugins/artifacts/runs/181b1dd9865e5ad343ef34f2a67ebfb0/review/

[orioltf]

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-feature-unic-spec-review-238-gate-dedup-incomplete
Commit: e4a8bb9
Philosophy: Fix everything unless clearly a new concern

---

Fixes Applied (7 total)

Severity	Count
🟠 MEDIUM	2
🟢 LOW	5

View all fixes

• ✅ SKIP_CLEAN_POSTS silently cancels approved skip/flag overrides (review-spec.md:513-515) — Split into two branches: "If an override was approved above" bypasses SKIP_CLEAN_POSTS; "If decision is 'post'" respects it. Scope guarantee now structurally enforced.
• ✅ ADR README marks ADR-0005 as pending (docs/adr/README.md:11) — Updated to "implemented as of v0.1.9"
• ✅ Badge table missing spaces around AND (review-spec.md:457) — Added spaces around AND
• ✅ 10b failure-fallback missing exit-0/no-results case (review-spec.md:429) — Extended to "non-zero exit, parse error, or parsed.results is not an array"
• ✅ runDedupCli leaks temp dirs (dedup-matcher.test.mjs:280-292) — Added try/finally + rmSync cleanup
• ✅ Test description says "error JSON" but assertion is substring (dedup-matcher.test.mjs:360) — Strengthened to JSON.parse(res.stderr) + typeof err.error === 'string'; added cleanup
• ✅ No symmetric test for --comments-file missing (dedup-matcher.test.mjs:350-357) — Added symmetric test with full JSON assertion

---

Tests Added

• exits 1 with an error JSON on stderr when --comments-file is missing (dedup-matcher.test.mjs)

---

Skipped (0)

(none — all findings addressed)

---

Validation

✅ Type check | ✅ Lint | ✅ Tests (373 passed, 6 in CLI envelope suite)

---

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-feature-unic-spec-review-238-gate-dedup-incomplete

[orioltf]

Human-side review pass (review-spec workflow) — complete

Ran the full PR-review workflow on top of Archon's self-review. Confirmed the Archon run had finished (no active run, no unresolved inline threads) before touching the branch.

Verified against ISSUE #238 acceptance criteria + ADR-0005 (all met):

• CLI emits run-level { truncated, results } envelope; matchDedup signature/purity unchanged ✅
• Step 10b parses .results; failure-fallback treats all as post with COMPARISON_INCOMPLETE still in effect ✅
• COMPARISON_INCOMPLETE = truncated OR (non-auth errors) ✅
• [?incomplete] badge on clean posts + single run-level confirmation before first clean-post write; skip/flag keep per-Finding gates ✅
• Write-safety invariant preserved; tests inject { comments, truncated } (no live services) ✅
• CHANGELOG + 0.1.9 bump; verify:changelog passes ✅

Ran 4 review agents (code / tests / silent-failure / comments) — 0 critical, 0 blocking. Acted on three in-scope, low-risk findings:

1. unic-spec-review: gate dedup posts when the existing-comment set is truncated #238 AC violation Archon missed — an em dash in authored command text (10d override branch); unic-spec-review: gate dedup posts when the existing-comment set is truncated #238 forbids em dashes in authored text. Replaced with the file's - style. (5151796)
2. Empty dedup-badge bullet (10c) rendered ambiguously and fought Prettier — reworded to (no badge) with spaced operators. (664a74f)
3. Documented that the CLI envelope's truncated === true soft default is deliberate (Step 10a is the authoritative COMPARISON_INCOMPLETE source); added a test for the realistic { comments }-without-truncated payload on the gating path. (664a74f)

Rejected as out-of-scope (locked by ADR-0005, not #238): folding the errors-half into the envelope and adding a third self-describing envelope signal — ADR-0005 deliberately keeps completeness authority in Step 10a, not the envelope.

gh pr checks all green (374 tests, typecheck, verify:changelog). Not merging — handing back for human merge.