feat(unic-spec-review): Figma + live sources via MCP, fail loud if absent

[orioltf]

What

Wires the two non-Confluence input sources into /review-spec (issue #209, epic #200), completing the Spec-versus-Design and Spec-versus-Live dimensions that previously ran in degraded text-only mode.

• Figma (Dev Mode MCP): reads designs + annotations for pasted Figma page/frame links and feeds them to spec-versus-design-agent.
• Live system (Playwright MCP): inspects pasted live URLs (title + visible content) and feeds observations to spec-versus-live-agent.
• Fail-loud invariant: if a required MCP is absent when a pasted link demands it, the run stops for that source with explicit remediation guidance — it never silently skips or degrades the source. Complements the up-front /spec-doctor check.
• Read-only: Figma and the live system are inputs only; nothing is posted to either.

Why

spec-versus-design-agent and spec-versus-live-agent already existed (S4) but reasoned from spec text alone, with inline notes acknowledging no real Figma/live access. The classifier (S1) already identified figma-page/figma-frame/live link kinds, so pasted Figma/live URLs were recognised but silently ignored — a user could believe the design/live system was checked when it was not. This slice closes that gap with real MCP gathering and a loud failure when the MCP is missing.

How

• Two new pure helpers with full node:test unit tests (injected data, no live MCPs):
  • figma-gatherer.mjs — extractAnnotations (defensive recursive annotation pull from an unknown Figma node tree), formatFigmaNodeSummary, buildFigmaContext.
  • live-gatherer.mjs — formatLivePageSummary (content capped at 2000 chars), buildLiveContext.
• commands/review-spec.md: Step 1.5 classifies all pasted URLs; Step 1.6 fails loud on absent MCPs; Steps 3.5/3.6 gather via the discovered MCPs and format via the helpers; Step 5 injects figmaContext/liveContext into the two agents.
• Both agents accept the new optional field and fall back to inference when it is null.
• Status lines in AGENTS.md and CONTEXT.md no longer list Figma/live-system as deferred.

The MCP-driven gathering orchestration and agent prompts are deliberately not unit-tested (exercised via manual runs), per the issue's scope; only the pure shaping helpers are.

Validation

• pnpm --filter unic-spec-review typecheck — ✅
• pnpm --filter unic-spec-review test — ✅ 345 pass, 0 fail (37 new)
• pnpm --filter unic-spec-review verify:changelog — ✅ → 0.1.8
• pnpm check — ✅ (2 pre-existing infos, none in new files)

🤖 Generated with Claude Code

[orioltf]

🔍 Comprehensive PR Review

PR: #240 — feat(unic-spec-review): Figma + live sources via MCP, fail loud if absent
Reviewed by: 5 specialized agents (code-review, error-handling, test-coverage, comment-quality, docs-impact)
Date: 2026-06-09

---

Summary

Final S9 slice delivering Figma Dev Mode MCP and Playwright MCP gathering. Implementation is well-structured: zero external deps, pure-helper extraction, full injected-dep unit tests, cross-platform, all 9 CI matrix jobs green. The sole CLAUDE.md violation is three /** @type {any} */ casts in one function; all other findings are LOW (diagnostic quality, test symmetry, pre-existing stale doc note). Nothing blocks merge.

Verdict: ✅ APPROVE

Severity	Count
🔴 CRITICAL	0
🟠 HIGH	0
🟡 MEDIUM	1
🟢 LOW	6

(7 unique findings after deduplication across 5 agents)

---

🟡 Medium Issues

any type casts in formatFigmaNodeSummary

📍 apps/claude-code/unic-spec-review/scripts/lib/figma-gatherer.mjs:90-92

Three /** @type {any} */ casts access .node and .document on an unknown argument — violates CLAUDE.md "no any without explicit approval". The correct Record<string, unknown> pattern is already used in extractAnnotations (line 54) of the same file.

View fix

Current:

const name =
    readString(data, 'name') ??
    readString(/** @type {any} */ (data)?.node, 'name') ??
    readString(/** @type {any} */ (data)?.document, 'name')
const description = readString(data, 'description') ?? readString(/** @type {any} */ (data)?.node, 'description')

Fix (mirrors established extractAnnotations pattern):

export function formatFigmaNodeSummary(url, data) {
    const obj = typeof data === 'object' && data !== null ? /** @type {Record<string, unknown>} */ (data) : null
    const name =
        readString(data, 'name') ??
        readString(obj?.node, 'name') ??
        readString(obj?.document, 'name')
    const description = readString(data, 'description') ?? readString(obj?.node, 'description')
    // ...rest unchanged
}

---

🟢 Low Issues

View 6 low-priority findings

1. Unguarded JSON.parse / readFileSync in CLI entries (both files)

📍 scripts/lib/figma-gatherer.mjs:132, scripts/lib/live-gatherer.mjs:73

ENOENT or SyntaxError emits a raw Node.js stack trace. Not a silent failure (orchestrator handles non-zero exit gracefully) but poor debuggability. Inconsistent with the clean --input required validation directly above.

Fix (same pattern for both files):

let raw
try {
    raw = JSON.parse(readFileSync(process.argv[inputFlag + 1], 'utf8'))
} catch (err) {
    process.stderr.write(`figma-gatherer: failed to read/parse input: ${err.message}\n`)
    process.exit(1)
}

---

2. Missing test for data.document wrapper in formatFigmaNodeSummary

📍 tests/figma-gatherer.test.mjs

The data.node name fallback is tested; the data.document fallback (line 91 of source) is not. A future removal of the branch would go undetected.

it('shows the name from a nested document wrapper', () => {
    const out = formatFigmaNodeSummary('url', { document: { name: 'Doc Frame' } })
    assert.ok(out.includes('Frame/Page: Doc Frame'))
})

---

3. main() --comments dispatch branch untested

📍 scripts/atlassian-fetch.mjs:1012

parseCommentsArg and collectComments are unit-tested; only the dispatch branch in main() is missing. An else if reorder or flag rename would not be caught. One test mirroring the collectChildPages success path suffices.

---

4. Empty-string edge cases not tested in formatLivePageSummary

📍 tests/live-gatherer.test.mjs

length > 0 guard treats '' as absent. Tests cover null and missing properties but not ''. Two lines to add:

it('shows (untitled) when title is an empty string', () => { ... })
it('shows (no content captured) when content is an empty string', () => { ... })

---

5. formatFigmaNodeSummary JSDoc omits wrapper fallback chain

📍 scripts/lib/figma-gatherer.mjs (JSDoc block)

JSDoc says "Defensive against missing names, descriptions, and annotations" but doesn't mention the data.node / data.document wrapper fallback. Non-obvious Figma API knowledge worth one sentence.

---

6. docs/adr/README.md status note is stale (pre-existing)

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

Note reads "ADR-0002 remains pending, landing with S8" — ADR-0002 landed in the prior S8 PR. This PR declaring all slices complete makes the contradiction more visible. Pre-existing issue.

---

✅ What's Good

• Fail-loud invariant fully realized: Step 1.6 hard-stops on missing MCPs with actionable remediation. Clean separation from graceful degradation in Steps 3.5/3.6.
• Defensive data access: readString + extractAnnotations never throw on null/non-object/unexpected MCP output.
• Zero external deps: both gatherers import only node:fs and node:url — the auto-format bar, met.
• Test quality: boundary testing at exact CONTENT_LIMIT, null-safety sweep, deduplication invariant, injected-dep pattern throughout. All 6 CI matrix jobs pass.
• No scope creep: all 6 new symbols are genuinely new — no duplication with existing lib primitives.
• Comments: module docstrings capture design decisions (why pure, why CLI entry, why defensive) not derivable from the code alone. The // Descend into common Figma MCP wrapper shapes. inline comment earns its place.
• Stale stubs removed: temporary "Note: no X available" placeholders in both agent prompts were cleanly removed rather than updated to new placeholders.
• CHANGELOG 0.1.8: thorough — all 6 exports, both modules, all 4 command steps, and both agent prompt updates described accurately.

---

📋 Suggested Follow-up Issues (if deferring LOWs)

Issue Title	Priority
"Add try-catch to gatherer CLI entries for parse/read errors"	P3
"Update ADR README to reflect all ADRs implemented in S8+S9"	P3

---

Reviewed by Archon comprehensive-pr-review workflow · 5 agents · 2026-06-09
Full artifact: artifacts/runs/2115326f5f4944acce94f48608056e69/review/consolidated-review.md

[orioltf]

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to `archon/task-feature-unic-spec-review-209-figma-live-sources`
Commit: befcec2
Philosophy: Fix everything unless clearly a new concern

---

Fixes Applied (7 total)

Severity	Count
🟡 MEDIUM	1
🟢 LOW	6

View all fixes

• ✅ any type casts in formatFigmaNodeSummary (figma-gatherer.mjs:88-93) — Replaced 3× /** @type {any} */ casts with const obj = /** @type {Record<string, unknown>} */ (data) pattern; name chain uses obj?.node / obj?.document
• ✅ Unguarded JSON.parse in CLI entry (figma-gatherer.mjs) — Wrapped in try/catch; prints figma-gatherer: failed to read/parse input: <message> to stderr and exits 1
• ✅ Unguarded JSON.parse in CLI entry (live-gatherer.mjs) — Same pattern with live-gatherer: prefix
• ✅ Missing test for data.document name fallback (figma-gatherer.test.mjs) — Added document-wrapper test case
• ✅ Empty-string edge cases not tested (live-gatherer.test.mjs) — Added 2 tests: empty title → (untitled), empty content → (no content captured)
• ✅ main() --comments dispatch untested (atlassian-fetch.test.mjs) — Added dispatch integration test; imported main
• ✅ formatFigmaNodeSummary JSDoc incomplete (figma-gatherer.mjs) — Added wrapper fallback documentation to JSDoc
• ✅ Stale ADR README status note (docs/adr/README.md) — Updated to "all four ADRs are implemented as of 0.1.8"

---

Tests Added

• figma-gatherer.test.mjs — shows the name from a nested document wrapper
• live-gatherer.test.mjs — shows (untitled) when title is an empty string, shows (no content captured) when content is an empty string
• atlassian-fetch.test.mjs — routes --comments to collectComments and returns a CommentsOutput

---

Skipped (0)

(none — all findings addressed)

---

Suggested Follow-up Issues

(none)

---

Validation

✅ Type check | ✅ Lint | ✅ Tests (349/349 passed, +4 new)

---

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-feature-unic-spec-review-209-figma-live-sources

[orioltf]

✅ Independent review (post Archon self-fix)

Re-reviewed PR #240 independently against issue #209's acceptance criteria and the locked ADRs (fail-loud MCP, read-only inputs, vendoring, no-em-dash). Verified Archon's 7 self-fixes actually landed in the source (not just self-reported), and ran two fresh scoped agents (correctness/style + silent-failure-hunter).

Verdict: solid. No CRITICAL/HIGH. The fail-loud invariant is correctly enforced at the right layer (Step 1.6, pre-gather), the read-only invariant holds, and the parseJsonResponse extraction + --comments dispatch in atlassian-fetch.mjs preserve error propagation.

One additional fix applied (c23f443)

Both reviewers independently flagged that the gatherer CLI entries did Array.isArray(raw) ? raw : [], which silently coerced a parseable-but-non-array MCP payload (e.g. {"error":"rate limited"}) into [] and exited 0 — laundering an MCP error into "no data" and bypassing the orchestrator's non-zero-exit warning. Directly in tension with #209's "never silently degrades" requirement.

Fixed: added pure exported asResultsArray / asObservationsArray helpers that throw on non-array input, folded into the existing CLI try/catch so a malformed payload now fails loud (stderr + exit 1). +18 unit tests (349 → 367), CHANGELOG bullet under [0.1.8], verify:changelog passes, all 9 CI jobs green.

Findings about prompt-driven gating being model-followed (not code-enforced) are inherent to command orchestration — noted as maintainer judgment calls, not defects, and out of scope for this slice.

Not merging — handing back for human merge.