feat(unic-pr-review): surface existing Human Threads as read-only review context

[orioltf]

Why

unic-pr-review already classified Human Threads (ADO Fetcher Step 3b) and emitted them as humanThreads, and the matcher (human-thread-matcher.mjs) + Notice renderer (notices.mjs) were implemented and tested — but the orchestrator never consumed any of it. Reviewer discussion was invisible to a Review in every Mode. Found comparing legacy pr-review vs unic-pr-review on ADO PR #5570: the legacy plugin cross-referenced a Finding to a still-open Human Thread and caught a thread marked fixed whose issue was still present. v2 surfaced none of it.

This wires the feature together per ADR-0016 (Human Threads as read-only context). Closes #248.

What changed

• commands/review-pr.md
  • New Step 1.8b (first-review / first-review-fallback) and Step 1.8b extension (re-review): run human-thread-matcher.mjs after the fan-out, annotate matched Findings, and add humanThreadsNotice to NOTICES_CONTEXT. Non-zero exit logs to stderr and continues — Human Threads are never a hard stop.
  • Pass read-only humanThreads to the Re-review Coordinator (Step 1.8a input).
• README.md: "How it works" prose + mermaid flowchart — three-way Thread classify, Step 7b matcher node, Coordinator humanThreads read-only input, Human Thread Notice at Step 8, and w2 writer box clarified to bot threads only — never human.
• CHANGELOG.md + version bump 2.1.10 → 2.1.11.

Invariants held

• No Finding is suppressed or down-ranked — Confidence < 60 (ADR-0002) stays the sole filter; the matcher only annotates.
• The ADO Writer never posts to a Human Thread in any Mode; the Coordinator emits zero threadActions for them.
• Aspect agents unchanged — no prompt injection, no SPAWN_TABLE / ADR-0008 change.

Verification

• pnpm --filter unic-pr-review test — 687 pass / 0 fail
• pnpm --filter unic-pr-review typecheck — clean
• pnpm --filter unic-pr-review verify:changelog — ok → 2.1.11
• pnpm ci:check — clean

🤖 Generated with Claude Code

[orioltf]

🔍 Comprehensive PR Review

PR: #250 — feat(unic-pr-review): surface existing Human Threads as read-only review context
Reviewed by: 4 specialized agents (code-review, error-handling, test-coverage, docs-impact)
Date: 2026-06-16

---

Summary

Clean, well-scoped ADR-0016 implementation. The new matchHumanThreadsToFindings pure function has excellent test coverage (19 cases), ADR invariants are airtight (no Findings suppressed, no writes to Human Threads), and CI passes all 9 matrix checks. One HIGH finding: the CLI entry-point in human-thread-matcher.mjs has no subprocess tests — a gap relative to every comparable CLI script in this plugin. All other findings are LOW/MEDIUM style issues, none blocking.

Verdict: APPROVE (with suggestions)

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

---

🟠 High Issue

CLI entry-point has no subprocess tests

📍 scripts/lib/human-thread-matcher.mjs:103–133

human-thread-matcher.mjs has a CLI entry-point invoked by the orchestrator, but unlike every other CLI script in this plugin (render-inline-comment.mjs, render-summary.mjs, intent-check-merger.mjs) it has no spawnSync subprocess tests. Untested paths: invalid JSON in env vars, type-error exits, default empty-array fallback, happy-path round-trip.

View recommended test file: tests/human-thread-matcher-cli.test.mjs

// @ts-check
// SPDX-License-Identifier: LGPL-3.0-or-later
// Copyright © 2026 Unic

import assert from 'node:assert/strict'
import { spawnSync } from 'node:child_process'
import { describe, it } from 'node:test'
import { fileURLToPath } from 'node:url'

const SCRIPT = fileURLToPath(new URL('../scripts/lib/human-thread-matcher.mjs', import.meta.url))

function run(findingsJson, threadsJson) {
	const env = { ...process.env }
	if (findingsJson === undefined) delete env.FINDINGS_JSON
	else env.FINDINGS_JSON = findingsJson
	if (threadsJson === undefined) delete env.HUMAN_THREADS_JSON
	else env.HUMAN_THREADS_JSON = threadsJson
	const r = spawnSync(process.execPath, [SCRIPT], { encoding: 'utf8', env })
	return { status: r.status ?? -1, stdout: r.stdout ?? '', stderr: r.stderr ?? '' }
}

describe('human-thread-matcher CLI', () => {
	it('exits 0 with valid JSON when both env vars omitted (defaults to [])', () => {
		const r = run(undefined, undefined)
		assert.equal(r.status, 0)
		const out = JSON.parse(r.stdout)
		assert.deepEqual(out.annotatedFindings, [])
		assert.deepEqual(out.unmatchedUnresolved, [])
	})

	it('exits 1 with stderr when FINDINGS_JSON is not valid JSON', () => {
		const r = run('{not json}', '[]')
		assert.equal(r.status, 1)
		assert.match(r.stderr, /JSON parse error/)
		assert.equal(r.stdout, '')
	})

	it('exits 1 with stderr when HUMAN_THREADS_JSON is not valid JSON', () => {
		const r = run('[]', '{bad}')
		assert.equal(r.status, 1)
		assert.match(r.stderr, /JSON parse error/)
	})

	it('exits 1 when FINDINGS_JSON is not an array', () => {
		const r = run('"a string"', '[]')
		assert.equal(r.status, 1)
		assert.match(r.stderr, /FINDINGS_JSON must be a JSON array/)
	})

	it('exits 1 when HUMAN_THREADS_JSON is not an array', () => {
		const r = run('[]', 'null')
		assert.equal(r.status, 1)
		assert.match(r.stderr, /HUMAN_THREADS_JSON must be a JSON array/)
	})

	it('exits 0 and writes annotated result for a matched finding + thread', () => {
		const findings = [{ severity: 'important', confidence: 80, filePath: 'src/a.ts', startLine: 10, title: 'T', body: 'B' }]
		const threads = [{ threadId: 1, filePath: 'src/a.ts', startLine: 10, status: 'active', excerpt: 'e' }]
		const r = run(JSON.stringify(findings), JSON.stringify(threads))
		assert.equal(r.status, 0)
		const out = JSON.parse(r.stdout)
		assert.ok(out.annotatedFindings[0].body.includes('Overlaps open Human Thread #1'))
		assert.equal(out.unmatchedUnresolved.length, 0)
	})
})

---

🟡 Medium Issue

CLI entry-point detection departs from codebase pattern

📍 scripts/lib/human-thread-matcher.mjs:104–107

The isMain guard uses .endsWith('human-thread-matcher.mjs') string matching instead of the canonical import.meta.url === pathToFileURL(process.argv[1]).href pattern used by all other CLI-capable lib scripts (changed-file-analyser.mjs:238, base-branch-resolver.mjs:67, intent-check-merger.mjs:138). Also carries a spurious typeof process !== 'undefined' guard.

View fix

// Add to imports (top of file):
import { pathToFileURL } from 'node:url'

// Replace lines 104-107:
const isMain = process.argv[1] != null && import.meta.url === pathToFileURL(process.argv[1]).href

---

🟢 Low Issues

View 6 low-priority suggestions

#	Issue	Location	Suggestion
1	Asymmetric array guard: findings not validated (only humanThreads is)	human-thread-matcher.mjs:67	Add if (!Array.isArray(findings)) return { annotatedFindings: [], unmatchedUnresolved: [] } before the humanThreads guard
2	filePath !== null misses undefined — garbles notice as `undefined:undefined`	notices.mjs:90	One-char: !== null → != null
3	No try/catch around matcher call in CLI — raw stack trace on unexpected throw	human-thread-matcher.mjs:131	Leave as-is (stack trace more useful than generic message; behavior is correct)
4	HumanThreadEntry typedef omits status field	notices.mjs:22-28	Leave as-is (intentionally narrow API for renderer)
5	excerpt: undefined entry not tested	notices.mjs:91	Add one unit test with excerpt: undefined
6	Exactly-80-char excerpt boundary not tested (tests cover 14 and 100, not 80)	notices.mjs:92	Add 'A'.repeat(80) boundary test

---

✅ What's Good

• Test suite is exemplary: 19 cases covering all four resolved statuses, ±10 proximity boundary (both sides), non-inline threads, no-mutation invariant, and PR #5570 golden regression scenario
• ADR-0016 invariants are airtight: unmatchedUnresolved filter combines !RESOLVED_STATUSES.has(t.status) && !matchedThreadIds.has(t.threadId) in a single pass; Coordinator explicitly forbids threadActions for humanThreads IDs
• Orchestrator Step 1.8b is non-blocking: non-zero exit → continue with original findings; never hard-stops the run
• ?? '[]' env-var defaults: both env vars fall back safely to empty arrays
• ADR-first implementation: ADR-0016 was accepted and merged before this PR; implementation matches consequences section point-for-point
• CHANGELOG entry is well-formed: correct ## [X.Y.Z] — YYYY-MM-DD em-dash format covering all three change areas

---

📋 Suggested Follow-up Issues (if not fixing in this PR)

Issue Title	Priority
Add CLI subprocess tests for human-thread-matcher.mjs	P1
Align isMain guard to import.meta.url pattern	P2

---

Reviewed by Archon comprehensive-pr-review workflow
Full artifacts: artifacts/runs/1e722a0fc56d0066498ee63c707874df/review/

[orioltf]

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-feature-unic-pr-review-248-surface-human-threads
Commit: dc0f4cd
Philosophy: Fix everything unless clearly a new concern

---

Fixes Applied (6 total)

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

View all fixes

• ✅ [HIGH] CLI subprocess tests (tests/human-thread-matcher-cli.test.mjs) — Created new file with 6 spawnSync tests covering: defaults to [], invalid JSON in both env vars, non-array inputs, and happy-path round-trip with matched finding+thread
• ✅ [MEDIUM] isMain guard pattern (scripts/lib/human-thread-matcher.mjs:104) — Replaced endsWith() + spurious typeof process guard with canonical import.meta.url === pathToFileURL(process.argv[1]).href; added import { pathToFileURL } from 'node:url'
• ✅ [LOW] Asymmetric array guard (scripts/lib/human-thread-matcher.mjs:67) — Added if (!Array.isArray(findings)) guard mirroring the existing humanThreads guard; returns { annotatedFindings: [], unmatchedUnresolved: [] } on bad input
• ✅ [LOW] filePath null check (scripts/lib/notices.mjs:90) — Changed !== null to != null to also handle undefined filePath from unexpected ADO response shapes
• ✅ [LOW] undefined excerpt test (tests/notices.test.mjs) — Added test verifying ?? '' guard: excerpt absent → renders "", no "undefined" in output
• ✅ [LOW] 80-char boundary test (tests/notices.test.mjs) — Added test verifying > 80 (not >= 80): exactly 80 chars → no truncation, no …

---

Tests Added

• tests/human-thread-matcher-cli.test.mjs — 6 new CLI subprocess tests
• tests/notices.test.mjs — 2 new edge-case tests (undefined excerpt, 80-char boundary)

Total: 695 tests passing (up from 689)

---

Skipped (2)

Finding	Reason
No try/catch around matchHumanThreadsToFindings()	Agent recommended leave-as-is: stack trace is more useful than a generic message for a bug in a pure, pre-validated function
HumanThreadEntry typedef omits status	Agent recommended leave-as-is: intentionally narrow — renderer must never branch on status

---

Suggested Follow-up Issues

(none — all actionable findings addressed)

---

Validation

✅ Type check | ✅ Lint (0 errors, 6 pre-existing infos) | ✅ Tests (695 passed)

---

Self-fix by Archon · aggressive mode · fixes pushed to archon/task-feature-unic-pr-review-248-surface-human-threads