Skip to content

feat(unic-spec-review): self-contained auth and preflight (/setup-confluence, /spec-doctor)#211

Merged
orioltf merged 4 commits into
developfrom
archon/task-feature-unic-spec-review-203-setup-confluence-spec
Jun 5, 2026
Merged

feat(unic-spec-review): self-contained auth and preflight (/setup-confluence, /spec-doctor)#211
orioltf merged 4 commits into
developfrom
archon/task-feature-unic-spec-review-203-setup-confluence-spec

Conversation

@orioltf
Copy link
Copy Markdown
Member

@orioltf orioltf commented Jun 5, 2026

Why

unic-spec-review had no way to set up credentials or verify prerequisites — both command stubs (/setup-confluence, /spec-doctor) were unimplemented and the script layer was absent. A user who installed only this plugin could not configure Confluence or preflight a review without first installing another plugin. This slice (S2, issue #203) makes the plugin self-contained for setup and preflight.

What

  • parseArgs added to scripts/lib/args.mjs (CLI parser the setup wizard imports); parseReviewSpecArgs untouched.
  • scripts/setup-confluence.mjs — vendored by copying from unic-pr-review (no cross-import; ./lib/args.mjs resolves locally). Writes ~/.unic-confluence.json atomically, chmod 600 on POSIX, icacls hint on Windows, preserves existing fields (e.g. jiraUrl).
  • scripts/spec-doctor.mjs — Confluence-only credential + connectivity preflight with injectable ping/loadCreds. No az CLI, no Jira (out of scope per AC).
  • commands/setup-confluence.md — full interactive wizard; routes the user to /unic-spec-review:spec-doctor (no Jira, no unic-pr-review references).
  • commands/spec-doctor.md — orchestrator: runs the script, then checks Figma Dev Mode MCP and Playwright MCP. Absent MCPs are reported as loud explicit ✗ failures with remediation, never a silent skip.
  • Unit tests for the wizard and the Confluence preflight logic with injected deps; no live services. Command orchestrators are not unit-tested (per AC).
  • CHANGELOG bullets under [Unreleased]; patch bump 0.1.1 → 0.1.2.

Self-containment

No runtime or setup dependency on any other plugin. Vendored by copying from unic-pr-review (the v2 plugin), never from the deprecated v1 pr-review/. ~/.unic-confluence.json is a shared credential store by convention, not a plugin coupling.

Validation

  • pnpm --filter unic-spec-review typecheck
  • pnpm --filter unic-spec-review test ✅ 93 pass / 0 fail
  • pnpm --filter unic-spec-review verify:changelog ✅ → 0.1.2
  • pnpm check

Refs #203.

🤖 Generated with Claude Code

…fluence, /spec-doctor)

The plugin had no way to set up credentials or verify prerequisites: both
command stubs were unimplemented and the script layer was absent. A user who
installed only unic-spec-review could not configure Confluence or preflight a
review without first installing another plugin.

Changes:
- Add parseArgs to scripts/lib/args.mjs (CLI parser for the setup wizard)
- Vendor scripts/setup-confluence.mjs by copying from unic-pr-review (no cross-import)
- Add scripts/spec-doctor.mjs: Confluence credential + connectivity preflight (injectable deps)
- Replace commands/setup-confluence.md stub with the full interactive wizard
- Replace commands/spec-doctor.md stub with the orchestrator (script + Figma/Playwright MCP checks; absent MCPs are loud explicit failures with remediation)
- Add unit tests for the wizard and the Confluence preflight logic; no live services
- Add CHANGELOG bullets under [Unreleased]

Refs #203

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@orioltf
Copy link
Copy Markdown
Member Author

orioltf commented Jun 5, 2026

🔍 Comprehensive PR Review

PR: #211 — feat(unic-spec-review): self-contained auth and preflight (/setup-confluence, /spec-doctor)
Reviewed by: 5 specialized agents (code-review, error-handling, test-coverage, comment-quality, docs-impact)
Date: 2026-06-05


Summary

Clean, self-contained implementation. Both new scripts use the injectable-deps pattern correctly, import only from local ./lib/* paths, and are backed by 93 passing tests across all CI platforms (macOS × Ubuntu × Windows × Node 22/24). All five agents independently recommend APPROVE. No blocking issues.

Verdict: ✅ APPROVE

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

🟡 Medium Issues (Needs Decision)

M1: parseArgs exported from args.mjs has no direct unit tests

📍 scripts/lib/args.mjs:57-82 / tests/args.test.mjs (not updated)

parseArgs is a new shared utility with 4 distinct behavioural branches — none covered by any test. args.test.mjs imports only parseReviewSpecArgs and was not extended. CLAUDE.md requires "Write tests for new functionality."

Risk: If this vendored copy diverges from the unic-pr-review origin, there's no regression guard — and a misparsed arg could silently write wrong credentials.

Options: Fix in this PR (preferred, ~15 lines) | Create follow-up issue | Skip

View recommended tests

Add to tests/args.test.mjs:

import { parseArgs, parseReviewSpecArgs } from '../scripts/lib/args.mjs'

describe('parseArgs', () => {
	it('parses --key=value inline form', () => {
		assert.deepEqual(parseArgs(['--url=https://x.atlassian.net', '--username=u']), {
			url: 'https://x.atlassian.net', username: 'u',
		})
	})
	it('parses --key value space-separated form', () => {
		assert.deepEqual(parseArgs(['--url', 'https://x.atlassian.net', '--username', 'u']), {
			url: 'https://x.atlassian.net', username: 'u',
		})
	})
	it('records boolean flags as empty string (presence-only)', () => {
		const r = parseArgs(['--dry-run'], { booleanFlags: new Set(['dry-run']) })
		assert.ok('dry-run' in r)
		assert.equal(r['dry-run'], '')
	})
	it('throws when --flag at end of args with no value', () => {
		assert.throws(() => parseArgs(['--url']), /--url requires a value/)
	})
	it('throws when --flag followed immediately by another --flag', () => {
		assert.throws(() => parseArgs(['--url', '--username']), /--url requires a value/)
	})
	it('ignores bare positional args', () => {
		assert.deepEqual(parseArgs(['positional', '--url', 'https://x.example']), { url: 'https://x.example' })
	})
	it('returns empty object for empty args', () => {
		assert.deepEqual(parseArgs([]), {})
	})
})

🟢 Low Issues (For Consideration)

View 5 low-priority suggestions
# Issue Location Fix
L1 Dead skipped branch in formatLineCheckResult.skipped never set spec-doctor.mjs:47,141 Remove branch (YAGNI) OR add — reserved for future optional checks comment to typedef
L2 process.env.HOME in inline snippet not cross-platform (Windows native) commands/setup-confluence.md:26 Change to require('os').homedir() — mirrors what the Node.js script already does
L3 setup-confluence.mjs top-level .catch omits stack trace (inconsistent with spec-doctor.mjs) scripts/setup-confluence.mjs:114 Change err?.message to err?.stack ?? err?.message ?? String(err)
L4 runSpecDoctorCredentials HTTP-error path (e.g. 401) not tested end-to-end scripts/spec-doctor.mjs:185 Add one it() with pingHttp(401)checkConfluence already covers it in isolation
L5 args.mjs module-level description only describes parseReviewSpecArgs, not parseArgs scripts/lib/args.mjs:6 Broaden to: args.mjs — CLI argument parsers for unic-spec-review with two bullet entries

✅ What's Good

  • Self-containment is airtight — all imports resolve to ./lib/* within the same plugin; vendor copy of parseArgs is character-for-character identical to the origin
  • Atomic credential writewrite(tmp) → chmod(tmp) → rename(tmp, target) prevents partial-write corruption on crash
  • Windows branch explicit — skips chmod and emits icacls remediation hint; never silent, never pretends chmod works
  • Discriminated-union PingResult — eliminates exception path for network ops entirely
  • Targeted SyntaxError catch in writeConfluenceCreds — only the known exception type caught; EACCES/ENOSPC propagate
  • Boundary condition tests — HTTP 199/200/299/300 all explicitly tested; 2xx predicate fully pinned
  • MCP failure loudnessspec-doctor.md specifies exact ✗ ... not connected output with remediation; "Missing MCPs are hard failures" documented
  • 93 pass / 0 fail across 18 suites on all CI platforms
  • All docs were anticipatorily accurate — AGENTS.md, README.md, CONTEXT.md already reflected the final design; no sync lag

Next Steps

  1. 📝 Decide on M1 (parseArgs tests): fix in this PR or create follow-up issue
  2. 🎯 Apply L1–L5 at discretion — none block merge
  3. ✅ Merge when ready

Reviewed by Archon comprehensive-pr-review workflow · 5 agents
Artifacts: /Users/oriol.torrent/.archon/workspaces/unic/unic-agents-plugins/artifacts/runs/f8fe1b4a1a348ed0d6cda23050c03da2/review/

- Add parseArgs test suite (7 cases) to args.test.mjs — covers all four
  behavioural branches including boolean flags and throw-on-missing-value
- Add runSpecDoctorCredentials HTTP-error test (401 via pingHttp)
- Remove dead CheckResult.skipped typedef property and formatLine skipped
  branch (YAGNI — nothing sets skipped: true in current code)
- Fix setup-confluence.mjs top-level catch to include err?.stack for
  better debugging parity with spec-doctor.mjs
- Fix process.env.HOME → require('os').homedir() in setup-confluence.md
  inline snippet for Windows-native portability
- Broaden args.mjs module description to cover both exported parsers

Tests: 101 pass / 0 fail (was 93)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@orioltf
Copy link
Copy Markdown
Member Author

orioltf commented Jun 5, 2026

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to `archon/task-feature-unic-spec-review-203-setup-confluence-spec`
Commit: ce92234
Philosophy: Fix everything unless clearly a new concern


Fixes Applied (6 total)

Severity Count
🔴 CRITICAL 0
🟠 HIGH 0
🟡 MEDIUM 1
🟢 LOW 5
View all fixes
  • parseArgs has no direct unit tests (tests/args.test.mjs) — Added 7-case parseArgs describe block covering all four branches (inline form, space-separated form, boolean flags, throw-on-missing-value)
  • Dead skipped branch in formatLine (scripts/spec-doctor.mjs:47,141) — Removed @property {boolean} [skipped] from CheckResult typedef; simplified formatLine to a ternary
  • process.env.HOME not cross-platform (commands/setup-confluence.md:26) — Changed to require('os').homedir() (Windows-native safe)
  • setup-confluence.mjs top-level catch omits stack (scripts/setup-confluence.mjs:114) — Changed to err?.stack ?? err?.message ?? String(err) — parity with spec-doctor.mjs
  • runSpecDoctorCredentials HTTP-error path not tested (tests/spec-doctor.test.mjs) — Added pingHttp(401) case asserting ok:false and 401 in output
  • args.mjs module description only covers parseReviewSpecArgs (scripts/lib/args.mjs:6) — Broadened to describe both exported parsers

Tests Added

File Cases
tests/args.test.mjs 7 new cases for parseArgs
tests/spec-doctor.test.mjs 1 new case for runSpecDoctorCredentials HTTP-error path

Total: 101 pass / 0 fail (was 93)


Skipped

(none — all findings addressed)


Validation

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


Self-fix by Archon · aggressive mode · fixes pushed to archon/task-feature-unic-spec-review-203-setup-confluence-spec

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements the previously stubbed /setup-confluence and /spec-doctor paths in unic-spec-review to make the plugin self-contained for Confluence credential setup and prerequisite preflight (per issue #203).

Changes:

  • Added a shared parseArgs CLI flag parser for setup scripts and covered it with tests.
  • Added scripts/setup-confluence.mjs (writes ~/.unic-confluence.json atomically, preserves existing fields) plus unit tests.
  • Added scripts/spec-doctor.mjs (credential + Confluence reachability preflight with injectable deps) plus unit tests; updated command docs and bumped plugin version to 0.1.2.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
apps/claude-code/unic-spec-review/tests/spec-doctor.test.mjs Unit tests for Confluence preflight and ping error mapping.
apps/claude-code/unic-spec-review/tests/setup-confluence.test.mjs Unit tests for credential writer + env detection.
apps/claude-code/unic-spec-review/tests/args.test.mjs Extends args tests to cover the new parseArgs.
apps/claude-code/unic-spec-review/scripts/spec-doctor.mjs New Confluence credential/connectivity preflight script.
apps/claude-code/unic-spec-review/scripts/setup-confluence.mjs New credential file writer script used by the wizard.
apps/claude-code/unic-spec-review/scripts/lib/args.mjs Adds parseArgs alongside existing parseReviewSpecArgs.
apps/claude-code/unic-spec-review/package.json Bumps package version to 0.1.2.
apps/claude-code/unic-spec-review/commands/spec-doctor.md Implements spec-doctor command orchestration instructions.
apps/claude-code/unic-spec-review/commands/setup-confluence.md Implements interactive setup wizard instructions.
apps/claude-code/unic-spec-review/CHANGELOG.md Adds 0.1.2 release notes and resets Unreleased placeholders.
apps/claude-code/unic-spec-review/.claude-plugin/plugin.json Bumps plugin manifest version to 0.1.2.
apps/claude-code/unic-spec-review/.claude-plugin/marketplace.json Bumps marketplace version to 0.1.2.

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

Comment thread apps/claude-code/unic-spec-review/scripts/spec-doctor.mjs
Comment thread apps/claude-code/unic-spec-review/scripts/spec-doctor.mjs
Comment thread apps/claude-code/unic-spec-review/scripts/setup-confluence.mjs
Comment thread apps/claude-code/unic-spec-review/scripts/setup-confluence.mjs
Comment thread apps/claude-code/unic-spec-review/commands/setup-confluence.md Outdated
Comment thread apps/claude-code/unic-spec-review/commands/setup-confluence.md Outdated
Comment thread apps/claude-code/unic-spec-review/commands/spec-doctor.md Outdated
orioltf and others added 2 commits June 5, 2026 17:11
Issue #203's acceptance criterion forbids em dashes in authored text
(except the mandated CHANGELOG version header). Copilot flagged the
user-facing strings; this sweeps the rest too - JSDoc comments, command
docs, spec-doctor/setup-confluence output, and test descriptions - so the
whole slice complies. Replaced every " — " separator with " - ".

The prior CHANGELOG note cited a now-removed org typography rule; reworded
to ground the change in this slice's own acceptance criterion.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Drop stale `:setup-jira` reference in writeConfluenceCreds JSDoc; that
  command exists in unic-pr-review (the vendoring source) but not in this
  plugin. Behavior unchanged.
- Add tests for two untested in-scope pure-logic branches: isEnvConfigured
  rejecting a present-but-empty env var, and checkConfluence's raw-string
  fallback when the configured url is unparseable.

Why: review surfaced a misleading vendored comment and two uncovered
branches in the pure logic the slice's acceptance criteria require tested.
No Critical/High findings; remaining suggestions are sub-threshold.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@orioltf orioltf merged commit d9882a2 into develop Jun 5, 2026
9 checks passed
@orioltf orioltf deleted the archon/task-feature-unic-spec-review-203-setup-confluence-spec branch June 5, 2026 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants