feat(unic-pr-review): plugin scaffold + doctor command#155
Conversation
Scaffolds the v2.0.0 unic-pr-review Claude Code plugin from the PRD and
ADRs 0001-0009 already on develop. Lands the manifest, package layout,
CHANGELOG, CONTEXT vocabulary, README, AGENTS.md (with CLAUDE.md symlink),
the /unic-pr-review:doctor slash command, the doctor script with six
preflight checks (az CLI, azure-devops extension, az devops login, az
devops user identity for ADR-0006 caching, Confluence reachability, and
Jira reachability — silent when jiraUrl is unset per US 35), and the
shared credentials loader. Adds the unit-test suite covering all six
predicates with stubbed executors and fetchers. No code is taken from
the existing apps/claude-code/pr-review/ plugin — every module is
re-derived from first principles against the PRD and ADRs.
Changes:
- Add apps/claude-code/unic-pr-review/.claude-plugin/{plugin,marketplace}.json (v2.0.0)
- Add apps/claude-code/unic-pr-review/{package,tsconfig}.json
- Add apps/claude-code/unic-pr-review/{CHANGELOG,CONTEXT,README,AGENTS}.md
- Symlink apps/claude-code/unic-pr-review/CLAUDE.md -> AGENTS.md
- Add apps/claude-code/unic-pr-review/commands/doctor.md
- Add apps/claude-code/unic-pr-review/scripts/{doctor.mjs,lib/credentials.mjs}
- Add apps/claude-code/unic-pr-review/tests/doctor.test.mjs (15 cases, all green)
- Append unic-pr-review entry to root CONTEXT-MAP.md
🔍 Comprehensive PR ReviewPR: #155 —
|
| Severity | Count |
|---|---|
| 🔴 CRITICAL | 1 |
| 🟠 HIGH | 1 |
| 🟡 MEDIUM | 6 |
| 🟢 LOW | 5 |
🔴 Critical: credentials.mjs — untested + crashes on malformed JSON
Agents: test-coverage (CRITICAL) + error-handling (HIGH) + code-review (MEDIUM) — three angles on the same bug
📍 apps/claude-code/unic-pr-review/scripts/lib/credentials.mjs:63 and :90
Three problems:
- Neither
loadAtlassianCredsnorloadAzureCredshas a single test JSON.parse(raw)is called without try/catch — a malformed~/.unic-confluence.json(e.g. trailing comma) causes an uncaughtSyntaxError- The top-level
.catchemits only the raw JS error, with no file path context
What the user sees today:
doctor: unexpected error: Unexpected token '<', "..." is not valid JSON
View fix
Source fix (credentials.mjs) — wrap JSON.parse in both loaders:
// loadAtlassianCreds, replace lines 62-64:
let parsed
try {
const raw = readFileSync(path, 'utf8')
parsed = JSON.parse(raw)
} catch (err) {
throw new Error(`~/.unic-confluence.json is unreadable or contains invalid JSON: ${err instanceof Error ? err.message : String(err)}`)
}
// Same fix in loadAzureCreds at lines 88-91:
let parsed
try {
const raw = readFileSync(path, 'utf8')
parsed = JSON.parse(raw)
} catch (err) {
throw new Error(`~/.unic-azure.json is unreadable or contains invalid JSON: ${err instanceof Error ? err.message : String(err)}`)
}New test file — add tests/credentials.test.mjs with 10 tests (env-var path, file path, missing-fields, malformed-JSON for both loaders). Pattern: real tmpdir() + writeFileSync, same as unic-archon-dlc/test/config-loader.test.mjs. The injectable homedir and env parameters are already designed for this.
🟠 High: Windows-incompatible entry-point guard
Agent: code-review (HIGH)
📍 apps/claude-code/unic-pr-review/scripts/doctor.mjs:288
On Windows, process.argv[1] is a backslash path; import.meta.url is a percent-encoded forward-slash URL. The strings never match — main() is never called — node doctor.mjs silently does nothing on Windows.
View fix
// Add pathToFileURL to the existing node:url import at the top
import { pathToFileURL } from 'node:url'
// Replace line 288:
// Before:
if (import.meta.url === `file://${process.argv[1]}`) {
// After:
if (import.meta.url === pathToFileURL(process.argv[1]).href) {Pattern reference: apps/claude-code/pr-review/scripts/confluence-client.mjs:159–162 — the sibling plugin solves this identically.
🟡 Medium Issues (Needs Decision)
1. checkAzExtension exec failure path untested
📍 doctor.mjs:92-94 — if (!r.ok) { return { ok: false, detail: 'unable to list az extensions' } } has no test. Silently removable.
View suggested test
it('returns ok:false when extension list command exits non-zero', () => {
const exec = execReturning({ ok: false, stderr: 'permission denied' })
assert.equal(checkAzExtension(exec).ok, false)
})Options: Fix now | Create issue | Skip
2. checkAzIdentity invalid JSON path untested
📍 doctor.mjs:131-133 — catch block returning 'az devops user show returned invalid JSON' is never exercised.
View suggested test
it('returns ok:false when user show returns invalid JSON', () => {
const exec = execReturning({ ok: true, stdout: 'not-valid-json' })
assert.equal(checkAzIdentity(exec).ok, false)
})Options: Fix now | Create issue | Skip
3. Network error (status 0) paths for Confluence/Jira untested
📍 doctor.mjs:152-186 — status 0 (timeout/ENOTFOUND) not covered; only HTTP 401/403 tested.
Options: Fix now (2 it() blocks) | Create issue | Skip
4. realPing JSDoc says "HEAD/GET" — implementation is GET-only
📍 doctor.mjs:213 — "HEAD/GET via node:https" implies a fallback pattern that doesn't exist.
// Fix: change to
/** Default fetcher: GET via node:https with a 10 s timeout. */Options: Fix now (one-line change) | Skip
5. "identity caching" vs "Iteration caching" — inconsistent ADR-0006 terminology
📍 doctor.mjs:15 (file header) vs doctor.mjs:~143 (checkAzIdentity JSDoc) — doctor.md uses "identity caching"; the function JSDoc says "Iteration caching". These are different concepts.
View suggested fix
/**
* Predicate: `az devops user show --user me` resolves with a user id.
* ADR-0006 requires the caller's identity to be resolvable before
* iteration data can be cached against it.
*/Options: Fix now | Skip (clarify which term is canonical in ADR-0006 first)
6. Root AGENTS.md workspace layout tree missing unic-pr-review/
📍 AGENTS.md:~11-31 — lists 4 plugins; unic-pr-review is the 5th.
# Fix: add one line
│ ├── unic-archon-dlc/
│ └── unic-pr-review/Options: Fix now (1 line) | Skip (CONTEXT-MAP.md is already updated, which is the primary agent navigation artifact)
🟢 Low Issues
View 5 low-priority suggestions
| Issue | Location | Suggestion |
|---|---|---|
catch {} in checkAzExtension discards parse error position |
doctor.mjs:85 |
Bind: catch (err) → append err.message |
catch {} in checkAzIdentity discards parse error position |
doctor.mjs:125 |
Same |
Top-level .catch omits stack trace |
doctor.mjs:291 |
Use err?.stack ?? err?.message |
checkAzExtension non-array JSON guard untested |
doctor.mjs:100-102 |
One it() with JSON.stringify({ extensions: [] }) |
credentials.mjs module comment says "the function" (singular) |
credentials.mjs:6 |
→ "each loader returns null when neither source is configured" |
✅ What's Good
- Injectable executor/ping pattern — excellent testability design; all six predicates testable without module mocking
- Windows
az.cmdhandling already correct atdoctor.mjs:56 checkJirasilent-skip (US 35) correct and tested, including a "must not call ping" sentinel guardsafeHostdefensive URL parsing handles malformed credential URLs gracefully- Cascade abort for az checks prevents noise when az CLI is missing
- Credential env-var short-circuit skips file reads in CI
- JSDoc typedef coverage is thorough — 8 types fully declared
- CONTEXT.md — all 14 domain terms with preferred/avoid pairs; exemplary
- Plugin-local AGENTS.md sets a high bar for future plugin scaffolds
- Biome root checks pass ✅
📋 Suggested Follow-up Issues
| Issue Title | Priority |
|---|---|
| "Add missing edge-case tests to doctor.mjs" | P2 |
| "Add stack trace to doctor top-level error handler" | P3 |
Next Steps
- ⚡ Fix CRITICAL: wrap
JSON.parse+ addtests/credentials.test.mjs - ⚡ Fix HIGH: use
pathToFileURLfor entry-point guard - 📝 Decide on MEDIUM issues 1–6 (fix now / create issue / skip)
- 🔀 Change base branch:
gh pr edit 155 --base develop - 🎯 Merge when CI green on corrected base
Reviewed by Archon comprehensive-pr-review workflow — 5 specialized agents
Full artifacts: /Users/oriol.torrent/.archon/workspaces/unic/unic-agents-plugins/artifacts/runs/3e48542b2829405328a2d9954bbc61ac/review/
Fixed: - Windows-incompatible entry-point guard (pathToFileURL instead of string concat) - Unguarded JSON.parse in credentials.mjs now throws descriptive errors - Bare catch blocks now bind err and include parse error position in detail - Top-level catch now emits stack trace for better diagnosability - realPing JSDoc corrected HEAD/GET -> GET (implementation is GET-only) - checkAzIdentity JSDoc: align Iteration caching -> identity caching terminology - credentials.mjs module comment: the function -> each loader - AGENTS.md workspace tree: add missing unic-pr-review/ entry Tests added: - tests/credentials.test.mjs: 11 tests for loadAtlassianCreds + loadAzureCreds (env vars, file loading, missing fields, malformed JSON) - doctor.test.mjs: 5 new edge-case tests (extension exec failure, non-array JSON, identity invalid JSON, Confluence status-0, Jira status-0) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
⚡ Self-Fix Report (Aggressive)Status: COMPLETE Fixes Applied (13 total)
View all fixes
Tests Added
Test suite grew from 15 → 31 passing tests. Skipped (0)(none — all findings addressed) Suggested Follow-up Issues(none) Validation✅ Type check | ✅ Lint | ✅ Tests (31 passed) Self-fix by Archon · aggressive mode · fixes pushed to |
…loader Removes duplicated try-catch JSON parse logic between loadAtlassianCreds and loadAzureCreds. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds unic-pr-review to the changes-detection filter, the test job's trigger condition, and the matrix package list so its tests actually run across ubuntu/macos/windows on Node 22 and 24. Without this, the new plugin's tests are silently skipped in CI. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The test job's name template used ${{ matrix.package }} which
stringifies the package object to the literal "Object" in GitHub's UI.
Fixes the display for every package (auto-format, unic-confluence,
release-tools, unic-archon-dlc, pr-review, unic-pr-review) so failed
cells are identifiable at a glance.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a brand-new unic-pr-review Claude Code plugin package in the monorepo and adds an initial /unic-pr-review:doctor command to validate required Azure DevOps and Atlassian prerequisites before future review functionality lands.
Changes:
- Added
apps/claude-code/unic-pr-review/scaffold (manifest + marketplace listing + scripts + tests + docs). - Implemented
scripts/doctor.mjswith six preflight checks plus credential loading from env/files. - Wired the new plugin into repo indices and CI path filtering.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Registers the new workspace package’s devDependencies. |
| CONTEXT-MAP.md | Adds the new plugin context entry. |
| AGENTS.md | Updates workspace layout tree to include unic-pr-review. |
| .github/workflows/ci.yml | Adds unic-pr-review to the path filter + test matrix. |
| apps/claude-code/unic-pr-review/.claude-plugin/plugin.json | New plugin manifest referencing the doctor command. |
| apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json | New marketplace listing metadata for the plugin. |
| apps/claude-code/unic-pr-review/package.json | New package scripts (bump/sync/tag/test/typecheck/verify:changelog). |
| apps/claude-code/unic-pr-review/tsconfig.json | Enables tsc --checkJs over .mjs sources/tests. |
| apps/claude-code/unic-pr-review/CHANGELOG.md | Adds initial v2.0.0 changelog entry. |
| apps/claude-code/unic-pr-review/CONTEXT.md | Introduces the domain glossary for the plugin. |
| apps/claude-code/unic-pr-review/README.md | Documents install, prerequisites, credentials, and doctor usage. |
| apps/claude-code/unic-pr-review/AGENTS.md | Plugin-local agent guidance and conventions/constraints. |
| apps/claude-code/unic-pr-review/commands/doctor.md | Defines the /unic-pr-review:doctor slash command. |
| apps/claude-code/unic-pr-review/scripts/doctor.mjs | Implements the doctor checks and CLI output/exit code. |
| apps/claude-code/unic-pr-review/scripts/lib/credentials.mjs | Implements env-first, file-fallback credential loading helpers. |
| apps/claude-code/unic-pr-review/tests/doctor.test.mjs | Unit tests for doctor predicates. |
| apps/claude-code/unic-pr-review/tests/credentials.test.mjs | Unit tests for credential loading behavior. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Test script picks up tests/*.test.mjs so credentials.test.mjs runs - Doctor stays fully silent about Jira when jiraUrl is unset (US-35) - realPing catches synchronous URL construction errors - Credentials loader separates file-read from JSON-parse errors Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The `changes` job exported `confluence-publish` while the paths-filter definition and all downstream consumers used `unic-confluence`, so the output was always empty and the matrix exclude never fired for unic-confluence — its tests ran on every PR. Rename the output to match. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
unic-pr-reviewClaude Code plugin from scratch underapps/claude-code/unic-pr-review/using only the PRD and ADRs as design inputs (clean-slate doctrine — no code taken fromapps/claude-code/pr-review/)/unic-pr-review:doctorslash command that runs six environmental preflight checks (az CLI, azure-devops extension, ADO login, ADO identity, Confluence, Jira) so users can diagnose setup issues before running a reviewCONTEXT-MAP.mdChanges
apps/claude-code/unic-pr-review/.claude-plugin/plugin.jsonapps/claude-code/unic-pr-review/.claude-plugin/marketplace.jsonapps/claude-code/unic-pr-review/package.jsonapps/claude-code/unic-pr-review/tsconfig.json@unic/tsconfig/tsconfig.base.jsonapps/claude-code/unic-pr-review/CHANGELOG.mdapps/claude-code/unic-pr-review/CONTEXT.mdapps/claude-code/unic-pr-review/README.mdapps/claude-code/unic-pr-review/AGENTS.mdapps/claude-code/unic-pr-review/CLAUDE.mdapps/claude-code/unic-pr-review/commands/doctor.mdapps/claude-code/unic-pr-review/scripts/doctor.mjsmain()orchestratorapps/claude-code/unic-pr-review/scripts/lib/credentials.mjs~/.unic-confluence.jsonand~/.unic-azure.jsonapps/claude-code/unic-pr-review/tests/doctor.test.mjsCONTEXT-MAP.md## Plugin contextspnpm-lock.yamlpnpm installrecorded the new workspace packageMotivation
The
unic-pr-reviewplugin did not exist as a package in the monorepo. Nothing can ship until the manifest, package layout, and credential loader are in place. The/unic-pr-review:doctorcommand is functional end-to-end so first-time users can debug setup without running a full review.Validation
All checks pass:
pnpm --filter unic-pr-review run typecheckpnpm --filter unic-pr-review test(15/15)pnpm --filter unic-pr-review verify:changelogpnpm ci:check(Biome + Prettier)git diff origin/develop..HEAD --name-onlyscope gateDoctor checks covered (15 tests, 6 suites):
checkAzCli×2,checkAzExtension×3,checkAzLogin×2,checkAzIdentity×3,checkConfluence×2,checkJira×3.Note for maintainer:
apps/claude-code/unic-pr-review/LICENSEwas NOT created (per CLAUDE.md: LICENSE files are maintainer-managed). Please add LGPL-3.0-or-later manually before taggingunic-pr-review@2.0.0for release.Notes
ADR-0010 is intentionally absent — it is planned but not yet landed per
docs/adr/README.md. The issue body states "ADRs 0001–0010 are present" which is a typo; only 0001–0009 exist on develop (merged via PR #153).Fixes #143