feat(unic-pr-review): Intent Check overlay-merge helper

[orioltf]

What & why

Closes #164 (parent: #160 — Intent Check verdicts are static, always unaddressed).

The Intent Checker emits an intentCheck skeleton with every AC verdict hardcoded to unaddressed. To render live verdicts, the Intent Assessor's output must be overlaid onto that skeleton deterministically — so the Assessor can only colour in verdicts, never add, drop, rename, or reorder ACs, and a malformed/missing response degrades gracefully to today's behaviour. This PR adds that pure, isolated, fully unit-tested helper (US 13 / ADR-0011). No agent or orchestrator wiring lands here — those are separate slices.

Changes

• scripts/lib/intent-check-merger.mjs — pure mergeIntentCheck(skeleton, assessed):
  • Skeleton is the structural source of truth; verdicts overlay only on matching item id + AC key.
  • A verdict applies only when present and valid; valid-verdict checking reuses the renderer's isAcVerdict (single source of truth — verdict set not re-defined).
  • Assessor items/AC keys absent from the skeleton are ignored; absent assessed ACs stay unaddressed.
  • Note-bearing skeleton items pass through verbatim (ADR-0004 hard-stop signal preserved).
  • Total-failure inputs (null, non-array, []) return the skeleton unchanged — free graceful degradation.
• tests/intent-check-merger.test.mjs — 10 node:test cases covering all 10 acceptance criteria + edge subcases.

Clean-slate doctrine respected: every module re-derived from the PRD + ADR-0011; zero reference to apps/claude-code/pr-review/.

Validation

Check	Result
pnpm typecheck	✅
node --test tests/intent-check-merger.test.mjs	✅ 10/10
pnpm test (full suite)	✅ 300/300
pnpm check (Biome + Prettier)	✅

🤖 Generated with Claude Code

[orioltf]

🔍 Comprehensive PR Review

PR: #168
Reviewed by: 3 specialized agents (code-review, error-handling, test-coverage)
Date: 2026-05-29

---

Summary

PR #168 adds a focused, pure mergeIntentCheck helper that overlays Intent Assessor verdicts onto the Intent Checker skeleton, along with 10 well-structured tests covering all acceptance criteria. All three review agents independently approved the PR. The implementation is clean, defensively designed, and correctly reuses isAcVerdict as the single source of truth.

Verdict: APPROVE ✅

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

---

🟢 Low Issues (All optional)

All 5 LOW findings recommend either keeping current behaviour or adding small test cases. No functional changes are needed.

L1: Type cast on assessed elements overstates guarantee

📍 scripts/lib/intent-check-merger.mjs:36

The assessed parameter is cast to IntentCheckItem[] immediately after entering the function, without per-element validation. Safe in practice (downstream ?. and isAcVerdict absorb structural deficiency), but the cast could mislead a future maintainer.

Recommendation: Keep as-is — module docstring already states "untrusted at this boundary".

---

L2: Assessed item with no verdicts property not explicitly tested (also reported by test-coverage agent)

📍 scripts/lib/intent-check-merger.mjs:52

assessedItem.verdicts?. safely handles absent verdicts, but no test explicitly pins this path.

Suggested test (7 lines)

it('keeps skeleton verdicts when the assessed item has no verdicts property', () => {
    /** @type {IntentCheckItem[]} */
    const skeleton = [{ id: 'PROJ-1', title: 'Login', verdicts: { 'AC 1': 'unaddressed' } }]
    // @ts-expect-error — intentionally malformed assessed output
    const assessed = [{ id: 'PROJ-1', title: 'Login' }]

    const merged = mergeIntentCheck(skeleton, assessed)

    assert.equal(merged[0].verdicts['AC 1'], 'unaddressed')
})

---

L3: Silent no-op when assessed items lack an id property

📍 scripts/lib/intent-check-merger.mjs:37

Items without a string id map to key undefined in the Map and never match any skeleton item — silently dropped. Safe outcome, but invisible during debugging.

Recommendation: Keep as-is — graceful-degradation outcome is correct; adding side effects to a pure function would break testability.

---

L4: No debug signal when verdicts are silently rejected

📍 scripts/lib/intent-check-merger.mjs:52–53

When isAcVerdict(candidate) fails, skeleton's unaddressed stands with no signal. Systematic assessor hallucination would look identical to a genuine all-unaddressed result.

Recommendation: Keep as-is now. Before first integration point, consider returning { items, rejectedVerdicts } so the caller can log — decision belongs at the integration layer, not here.

---

L5: Mixed note + non-note skeleton items not tested together

📍 scripts/lib/intent-check-merger.mjs:39–47

Tests cover note-bearing and non-note items in isolation but not in the same call. A future refactor that inverts the note-check could pass isolated tests while silently breaking the compound case.

Suggested test (12 lines)

it('passes note-bearing items through while still merging adjacent normal items', () => {
    /** @type {IntentCheckItem[]} */
    const skeleton = [
        { id: 'PROJ-1', title: 'Login', verdicts: { 'AC 1': 'unaddressed' }, note: 'Could not fetch' },
        { id: 'PROJ-2', title: 'Logout', verdicts: { 'AC 1': 'unaddressed' } },
    ]
    const assessed = [
        { id: 'PROJ-1', title: 'Login', verdicts: { 'AC 1': 'addressed' } },
        { id: 'PROJ-2', title: 'Logout', verdicts: { 'AC 1': 'addressed' } },
    ]

    const merged = mergeIntentCheck(skeleton, assessed)

    assert.equal(merged[0].verdicts['AC 1'], 'unaddressed') // note-bearing: unchanged
    assert.equal(merged[0].note, 'Could not fetch')
    assert.equal(merged[1].verdicts['AC 1'], 'addressed')  // normal: merged
})

---

✅ What's Good

• Pure function: No side effects, no I/O, no module-level state — maximally testable and composable
• Graceful degradation by default: Total-failure inputs (null, non-array, []) return the exact skeleton reference unchanged
• Identity preservation: Early-return paths return the original skeleton reference (not a copy) — assert.equal tests correctly pin this contract
• isAcVerdict reuse: Single source of truth honoured per ADR-0011; no re-definition of valid verdict set
• ADR-0004 note passthrough: Note-bearing items returned verbatim; asserts both verdict AND note property
• Spread-based immutability: { ...skeletonItem, verdicts: mergedVerdicts } — no skeleton mutation
• 10 tests for 58-line module: Exceptional ratio; all tests behaviour-oriented and resilient to refactoring
• CLAUDE.md compliance: All rules pass (SPDX, @ts-check, copyright, tabs, single quotes, no semicolons, node:test, no external deps)

---

📋 Suggested Follow-up Issue

• "Return rejected verdict count from mergeIntentCheck at first integration point" (P3) — relates to Finding L4

---

Reviewed by Archon comprehensive-pr-review workflow
Artifacts: artifacts/runs/4b3940cb6740131ca081991e06a08da8/review/

[orioltf]

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-fix-issue-164 (4198f0a)
Philosophy: Fix everything unless clearly a new concern

---

Fixes Applied (2 total)

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

View all fixes

• ✅ L2: assessed item missing verdicts property (tests/intent-check-merger.test.mjs) — added test pinning ?. guard; skeleton stands when assessed item lacks verdicts
• ✅ L5: Mixed note + non-note items not tested together (tests/intent-check-merger.test.mjs) — added compound scenario test verifying note-bearing item passes through while adjacent normal item merges

---

Tests Added

• keeps skeleton verdicts when the assessed item has no verdicts property
• passes note-bearing items through while still merging adjacent normal items

Test suite: 302 tests, 0 failures.

---

Skipped (3)

All three were explicitly recommended as "keep as-is" by the review agents:

Finding	Reason
L1: Type cast on assessed overstates guarantee	Option C: existing ?. + isAcVerdict guards sufficient; runtime validator would add noise
L3: Silent no-op when assessed items lack id	Option A: graceful-degradation is safe; side effects in a pure function break testability
L4: No debug signal for rejected verdicts	Option A: belongs at integration layer when wiring into skill orchestrator

---

Suggested Follow-up Issues

1. "Return rejected verdict count from mergeIntentCheck at first integration point" — P3 — add logging at the integration layer, not in the pure function

---

Validation

✅ Type check | ✅ Lint | ✅ Tests (302 passed)

---

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-fix-issue-164

[Copilot]

Pull request overview

Adds a pure, deterministic helper to overlay the Intent Assessor’s AC verdicts onto the Intent Checker’s intentCheck skeleton (skeleton remains the structural source of truth), plus unit tests to validate the merge semantics. This supports the upcoming “live” Intent Check rendering while preserving graceful degradation to today’s all-unaddressed behavior.

Changes:

• Introduces mergeIntentCheck(skeleton, assessed) helper that overlays only valid verdicts on matching id + AC key and preserves note-bearing skeleton items.
• Adds a dedicated node:test suite covering the intended acceptance criteria and edge cases.
• Updates Prettier ignore rules to exclude artifacts/.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
apps/claude-code/unic-pr-review/scripts/lib/intent-check-merger.mjs	New merge helper that overlays assessed verdicts onto the skeleton using renderer-sourced verdict validation.
apps/claude-code/unic-pr-review/tests/intent-check-merger.test.mjs	Unit tests for the merge helper across happy path + failure/ignore scenarios.
.prettierignore	Ignores artifacts/ from Prettier formatting.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.