feat(unic-spec-review): shared context (existing comments + Landscape Brief)

[orioltf]

What

Implements unic-spec-review S3 (#204): the two shared-context inputs the review reasons from, computed once per run and exposed for injection into the review agents.

1. Existing Confluence comments. Extend the vendored atlassian-fetch with fetchConfluenceComments (footer + inline) via the v1 REST API with injected fetch, paginated through _links.next. Read-only, no writes. Inline comments carry the original selection text as anchor.
2. Landscape Brief. New scripts/lib/landscape-detector.mjs derives a LandscapeBrief (stack, test runner, test frameworks, tooling, reachable-prod flag, adjacent systems) from repo manifests + file listing + user-declared adjacent systems. The technology stack is never hardcoded; everything is read from the repo. Computed once and exported for injection (consumed fully in S4).

Why

Review agents (Gaps, Testability, Feasibility, Non-functional, Spec-versus-Live) need prior page discussion (so resolved points are not re-raised) and the actual tech landscape (so feasibility/testability is judged against the real stack, not assumptions). Neither was available from S1+S2. Mirrors how unic-pr-review computes Doc Context once and injects it.

Scope / constraints honoured

• No reads or imports from apps/claude-code/pr-review/ (deprecated v1, hook-blocked); no cross-plugin runtime imports.
• All new modules use injectable deps; node:test unit tests touch no live services.
• Comment WRITE path and agent consumption of the Brief are explicitly out of scope (S4+).

Tests

• fetchConfluenceComments: footer, inline+anchor, empty page, _links.next pagination, HTML strip, author fallback (accountId / null), 401, 404, bad URL.
• detectLandscape: empty repo, Node+TS, React, jest/vitest/node:test runner selection, Playwright + playwright.config.js reachableProd, Biome/ESLint tooling, Python/pytest, requirements.txt, Rust, Go, Ruby/Rails, Java, adjacentSystems passthrough + copy, malformed JSON, readdir failure.

Verification

Check	Result
typecheck	pass
test	132 passed, 0 failed
verify:changelog	ok → 0.1.3
ci:check	pass

Closes #204

🤖 Generated with Claude Code

[orioltf]

🔍 Comprehensive PR Review

PR: #224 — feat(unic-spec-review): shared context (existing comments + Landscape Brief)
Reviewed by: 5 specialized agents (code-review · error-handling · test-coverage · comment-quality · docs-impact)
Date: 2026-06-08

---

Summary

Both new modules (fetchConfluenceComments and landscape-detector.mjs) are well-structured, follow all monorepo conventions, and use the established injectable-deps testability pattern. All 5 agents recommend APPROVE. Zero CRITICAL or HIGH issues found.

Verdict: ✅ APPROVE — 2 MEDIUM items worth addressing before/alongside merge

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

---

🟡 Medium Issues (Recommended to Fix)

M1: Unbounded Pagination Loop in fetchConfluenceComments

📍 scripts/atlassian-fetch.mjs:542

The while (nextUrl) loop follows _links.next with no page cap. A Confluence server bug returning a cyclic next-link chain would cause the function to hang indefinitely — caller times out or OOMs, user sees no error.

View recommended fix (3 lines)

const MAX_PAGES = 50
let page = 0
while (nextUrl) {
    if (++page > MAX_PAGES) break          // guard against runaway pagination
    const json = await fetchJson(nextUrl, creds, fetchImpl)
    // ... rest of loop unchanged ...
}

50 pages × 100 comments = 5 000 comments — well above any real spec page. Silent truncation at that ceiling is safe in practice.

---

M2: Playwright Reported in Both testRunner and testFrameworks

📍 scripts/lib/landscape-detector.mjs — detectTestRunner + detectTestFrameworks

When @playwright/test is installed, both testRunner: 'playwright' and testFrameworks: [..., 'Playwright'] are set simultaneously. The LandscapeBrief is the single source of truth injected into multiple S4 agents — redundant signals increase prompt noise.

View recommended fix

Remove @playwright/test / playwright from the detectTestFrameworks map — the testRunner field already carries this information precisely.

function detectTestFrameworks(deps) {
  const frameworks = []
  const map = [
    [['@testing-library/react', '@testing-library/dom', '@testing-library/vue'], 'Testing Library'],
    [['cypress'], 'Cypress'],
    // '@playwright/test' removed — covered by testRunner field
    [['msw'], 'MSW'],
    [['supertest'], 'Supertest'],
    [['nock'], 'Nock'],
  ]
  for (const [keys, label] of map) {
    if (keys.some((k) => deps.has(k))) frameworks.push(label)
  }
  return frameworks
}

---

🟢 Low Issues

View 10 low-priority findings

#	Issue	Location	Agent	Suggestion
L1	@vue/core is a non-existent npm package	landscape-detector.mjs frameworkMap	code-review	Remove dead key — vue already covers all real Vue projects
L2	node:test detection loses to Playwright dep check in mixed projects	landscape-detector.mjs detectTestRunner	code-review	Move node --test script check to the top (before dep-presence checks)
L3	Empty id string on malformed API response	atlassian-fetch.mjs:553	error-handling	Add JSDoc note: always non-empty for real API responses
L4	Broad catch {} swallows EACCES/EMFILE as if ENOENT	landscape-detector.mjs:48-89	error-handling	Intentional design — leave as-is; future upgrade: injectable onError
L5	reachableProd false-negative when readdirSync fails	landscape-detector.mjs:222	error-handling	Replace readdirSync glob with 4× existsSync per config variant
L6	Secondary detection branches untested (build.gradle, Django, Cypress, @jest/core)	landscape-detector.mjs:185-230	test-coverage	Add ~4 targeted it blocks matching existing test style
L7	No network-error test for fetchConfluenceComments	tests/atlassian-fetch.test.mjs	test-coverage	Add one fetchThrows test mirroring the fetchConfluencePage pattern
L8	"Pure function" overstates FP purity in detectLandscape JSDoc	landscape-detector.mjs:230	comment-quality	Stateless, injectable — all I/O goes through deps; never throws.
L9	Status note in AGENTS.md still describes S1	CLAUDE.md (symlinked AGENTS.md)	docs-impact	Extend to S3: URL classify → Confluence fetch → comments read → LandscapeBrief
L10	Status note in CONTEXT.md still describes S1	CONTEXT.md	docs-impact	Same — mention fetchConfluenceComments + landscape-detector as implemented

---

✅ What's Good

• detectLandscape never throws — defensive helpers mean no broken manifest crashes the pipeline
• adjacentSystems defensively copied — caller array mutation can't corrupt LandscapeBrief; tested
• Pagination _links.next handled correctly — confluenceBase prefix applied; while-loop terminates on falsy
• Author fallback chain (displayName ?? accountId ?? '') correct and tested including createdBy: null
• Array.isArray(json?.results) guard — prevents crash on unexpected Confluence top-level shape
• Separator-insensitive stubFs — genuine cross-platform tests (macOS/Linux/Windows CI all pass)
• Pagination call-count assertion — asserts both call count and returned comment identity, not just "no error"
• Comment quality high — cross-platform rationale, pagination mechanics, and typedef properties all documented correctly; no comment rot
• Typed FetchError surface — FetchErrorKind union lets callers pattern-match on kind, not strings
• No primitive duplication — all 4 new types fill genuine gaps in the existing type surface
• No new ADR needed — all design decisions already covered by ADRs 0001–0004

---

📋 Suggested Follow-up Issues

Issue	Priority
Fix node:test runner priority over Playwright dep (L2)	P2
Update AGENTS.md + CONTEXT.md status notes to S3 (L9, L10)	P2
Add missing landscape-detector tests: build.gradle, Django, Cypress, @jest/core (L6)	P3
Add network-error test for fetchConfluenceComments (L7)	P3
Fix reachableProd to use existsSync per config variant (L5)	P3

---

Reviewed by Archon comprehensive-pr-review workflow (5 agents)
Full artifacts: /artifacts/runs/96b0b5f2b66d64a7b5f313b5643d51d4/review/

[orioltf]

✅ Review findings addressed — commit aa91f5d

Resolved the actionable findings from the comprehensive review. All 141 unit tests pass, tsc --noEmit is clean, verify:changelog is ok, and all 8 CI checks are green. Folded into the unreleased 0.1.3 CHANGELOG entry (no re-bump — 0.1.3 has no tag yet).

#	Finding	Resolution
Recommended	Playwright double-reported in testFrameworks	Removed — testRunner carries it precisely
L1	Non-existent @vue/core key	Removed; vue covers all real Vue projects
L2	node --test script loses to Playwright dep	Script check moved above dep checks (most authoritative signal)
L3	Empty id on malformed API response	JSDoc note added: always non-empty for real API responses
L5	reachableProd false-negative on readdir failure	Replaced glob with per-variant existsSync (4 config names)
L6	Secondary landscape branches untested	Added tests: build.gradle, Django, FastAPI, Flask, Cypress, @jest/core, TS-without-tsconfig, node:test-over-Playwright
L7	No network-error test for fetchConfluenceComments	Added a fetchThrows transport-error test (asserts kind === 'unreachable')
L8	"Pure function" overstates FP purity	JSDoc → Stateless, injectable — all I/O goes through deps; never throws
L9	AGENTS.md status note still S1	Updated to S1–S3
L10	CONTEXT.md status note still S1	Updated to S1–S3

Plus: capped fetchConfluenceComments pagination at 50 pages as an infinite-loop guard against a misbehaving _links.next.

Intentionally left as-is (locked design)

• L4 — broad catch {} in the manifest readers treating EACCES/EMFILE like ENOENT is the intended never-throws contract for detectLandscape. A future injectable onError is a reasonable follow-up but out of scope for this slice.
• Vendoring / code duplication of the Atlassian helpers is required by ADR-0001 self-containment, not a defect.

Read-only/write-safety invariant untouched: this slice only adds read paths (fetchConfluenceComments) and the LandscapeBrief detector.

[orioltf]

🔁 Full re-review complete — commit 9ea627c

Re-ran the 5-agent comprehensive review against the patched diff to confirm no regressions. The five original fixes were confirmed behaviorally correct. The re-review caught one regression my own fix introduced plus one latent contract gap — both now fixed. 145 tests pass, typecheck clean, Biome clean, all 8 CI checks green.

Severity	Finding (agent)	Resolution
Regression	Dead tryListDir + readdirSync seam, orphaned by the existsSync reachableProd fix → new Biome noUnusedVariables warning (code-reviewer, test-analyzer, type-analyzer)	Removed the function, the readdirSync import and the LandscapeDetectorDeps.readdirSync field; repointed its stale test
HIGH	MAX_PAGES=50 cap silently truncated comments — partial result indistinguishable from complete (silent-failure-hunter, test-analyzer)	Added a truncated flag to ConfluenceCommentsResult, set when the cap fires; new runaway-_links.next test asserts the cap at exactly 50 pages
(latent)	detectLandscape "never throws" contract not upheld — raw existsSync calls for tsconfig/pyproject/Cargo/go.mod/Gemfile/pom/gradle ran outside the try/catch readers, so a throwing existsSync (EACCES) escaped	Routed all existence checks through a new tryExists helper; new "does not throw when existsSync fails" test
Medium	testRunner typed as bare string vs repo's string-literal-union convention (type-analyzer)	Introduced TestRunner union typedef; applied to the field and detectTestRunner
Low	Only playwright.config.js tested of 4 variants (test-analyzer)	Parametrized test over .js/.ts/.mjs/.cjs
Low	created JSDoc didn't note the '' fallback (type-analyzer)	Hedged to "ISO creation timestamp, or '' when unavailable"

Confirmed correct / no action (per locked decisions)

• Vendoring duplication (ADR-0001), broad ENOENT-style catch {} in the manifest readers (intentional never-throws degradation), Six-Hats layering — all upheld.
• All work stayed within issue unic-spec-review S3: shared context — read existing comments + Landscape Brief #204 scope (comments read path + Landscape Brief); no later-slice leakage.