Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# 0001. Multi-source intent gathering with shared Atlassian credentials

**Status:** Accepted (2026-05)

## Context

The Plugin fetches business intent from three Platforms — Azure Boards work items, Jira issues, and Confluence pages — to build the Intent Brief that seeds every Review Aspect agent. Azure Boards uses `~/.unic-azure.json` (PAT); Confluence and Jira share `~/.unic-confluence.json` because Atlassian Cloud authenticates both products with the same email plus API token on the same tenant.

Two alternatives were considered:

- **Separate `~/.unic-jira.json` file mirroring the Confluence schema.** Rejected — duplicates one credential pair that always points at the same tenant in practice. Would force users to run two near-identical wizards and store the same token twice.
- **One unified `~/.unic-atlassian.json`.** Rejected for v2 — would break the existing `unic-confluence` Plugin which already ships with `~/.unic-confluence.json` and is depended on outside Claude Code. Reusing the existing file is a strict superset that preserves backward compatibility.

## Decision

Reuse `~/.unic-confluence.json` as the shared Atlassian Credential File for both Confluence and Jira. The file gains an optional `jiraUrl` field (defaulting to the same tenant); the `JIRA_URL` env var overrides it. Azure Boards keeps its own `~/.unic-azure.json` file because its auth model (PAT against an ADO organisation) is unrelated to Atlassian Cloud.

## Consequences

- The Confluence Setup Wizard becomes the de-facto Atlassian wizard; the Jira wizard is a thin overlay that only writes the `jiraUrl` field if absent.
- The Doctor command pings Jira only when `jiraUrl` is configured, keeping projects that don't use Jira free of noise.
- If Atlassian ever splits Cloud auth across products, the schema can be extended additively (`jiraToken`, `jiraUsername`) without breaking existing files.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# 0002. Confidence-scored Findings with explicit Severity thresholds

**Status:** Accepted (2026-05)

## Context

Review Aspect agents emit Findings that must be filtered (to drop low-quality output) and grouped into Severity buckets (to drive the Review Summary). The team needed a rubric that produced consistent, low-noise output across six independent agent prompts.

Two alternatives were considered:

- **Descriptive severity only (Critical / Important / Minor as agent choice).** Rejected — what we observed in the Anthropic `pr-review-toolkit` is that explicit numeric confidence with a hard cutoff produces far fewer false positives than asking the model to pick a severity word. The number forces the agent to commit and gives a clean filter knob.
- **Score with no Minor bucket (Anthropic's exact rubric: 80+ floor).** Rejected — our Review Summary template has a Minor section that's useful for low-impact-but-correct observations. Dropping everything below 80 would empty that bucket.

## Decision

Every Review Aspect agent attaches a 0-100 Confidence Score to each candidate Finding and drops anything below 60. The Score deterministically maps to a Severity bucket in the Review Summary: 90-100 Critical, 80-89 Important, 60-79 Minor. The Intent Checker is exempt — it emits qualitative per-Acceptance Criterion verdicts (addressed / partially addressed / unaddressed) instead.

## Consequences

- Every agent prompt embeds the rubric verbatim so all six aspect agents stay consistent.
- The threshold knobs (60 floor, 80 Important boundary, 90 Critical boundary) are policy, not implementation detail — changing them changes how a Review reads and would surprise long-time users. Any future tuning warrants a superseding ADR.
- The Intent Checker's exemption is intentional: AC verdicts (addressed / partially addressed / unaddressed) are categorical, not confidence-bearing; forcing a 0-100 score on intent matching would be noise.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# 0003. Interactive Approval Loop as the default write path

**Status:** Accepted (2026-05)

## Context

The Plugin produces Findings that may or may not be worth posting to a shared PR thread in ADO. We needed a default behaviour for the write path that balanced developer trust (don't dump unfiltered LLM output) against ergonomics (don't make the common case painful).

Two alternatives were considered:

- **Bulk-post by default, like a CI bot.** Rejected — this Plugin runs from a developer's terminal, not a CI pipeline. Posting unfiltered LLM output to a shared PR thread is a noise-and-trust hazard the invoker hasn't opted into.
- **Preview-then-confirm-once (single y/N for the whole batch).** Rejected — invokers consistently want to drop the one weak Finding without re-running the whole Review. Per-Finding choice costs a few seconds and earns trust.

## Decision

The Plugin previews a Review in the terminal by default and writes nothing to ADO. `--post` enters the Approval Loop — each Finding shown one at a time with accept / edit / skip choices. `--post --yes` bulk-accepts and posts every Finding without prompting, for CI use.

## Consequences

- The Plugin must render Findings in a stable order and emit a resumable state log so an interrupted Approval Loop can be picked up without re-running the agents.
- The default behaviour (no `--post`) makes every Review effectively a dry-run, which removes the need for a separate `--dry-run` flag.
- The `--yes` escape hatch lets CI use the Plugin without prompts; the Plugin must detect non-TTY stdin and abort cleanly when `--post` is given without `--yes` in a non-interactive context.
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# 0004. Hard-stop when intent sources are unreachable

**Status:** Accepted (2026-05)

## Context

A fetched Work Item often links to a Confluence page or Jira issue that carries the actual intent. When that linked source cannot be retrieved (missing Credential File, wrong tenant, page deleted), the Review Aspect agents have an incomplete Intent Brief and the Intent Check verdicts come out wrong. We needed to decide whether to soft-fail or hard-fail.

Two alternatives were considered:

- **Best-effort: skip unreachable sources, post a Notice, continue.** Rejected — a Work Item that links to a Confluence page is making a promise that the page carries the intent. Skipping the page means every aspect agent reviews against an incomplete Intent Brief, the Intent Check verdicts come out wrong, and the Reviewer cannot tell from the output that intent was missing. The failure is silent and load-bearing.
- **Degrade with a top-of-summary banner.** Rejected — banners are routinely ignored when the rest of the output looks complete. A hard stop forces the invoker to set up credentials once, then never see this failure mode again.

## Decision

If a fetched Work Item links to a Confluence page and the Confluence Credential File is missing or the page is unreachable, the Plugin halts with a one-line instruction to run `setup-confluence`. Same rule for Jira issues whose tenant is unreachable. The Plugin does not silently skip an intent source.

## Consequences

- The Doctor command becomes load-bearing: invokers run `doctor` before their first Review to surface missing credentials before they're in the middle of a PR review.
- Empty intent (no Work Items linked, none pasted) is NOT a failure — it's an empty Intent Brief and the Review proceeds without a preamble. The hard stop fires only when intent is promised by a link but cannot be retrieved.
22 changes: 22 additions & 0 deletions apps/claude-code/unic-pr-review/docs/adr/0005-az-cli-over-rest.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# 0005. `az` CLI for Azure DevOps, custom HTTP for Atlassian

**Status:** Accepted (2026-05)

## Context

The Plugin talks to two Source Platforms — Azure DevOps (for PR data, threads, write-back) and Atlassian Cloud (Confluence pages, Jira issues, for intent). Each has a different mix of CLI vs REST options, and the team had to pick the right transport for each without applying a global doctrine.

Two alternatives were considered:

- **REST everywhere via Node's global `fetch`.** Rejected for ADO — the ADO REST surface is broad, evolves per-org, and requires resolving organisation, project, repository, and PR IDs through several round-trips. The `az` CLI handles all of that, ships with the user's existing auth context (`az login`), and is already trusted by the developer community.
- **`az` CLI for Atlassian too (via a community wrapper).** Rejected — no first-party or widely-trusted CLI exists for Confluence v2 or Jira Cloud; pinning a community wrapper would add a runtime dep with weaker stability guarantees than calling the REST API directly.

## Decision

ADO Fetcher and ADO Writer shell out to `az devops invoke` for every Source Platform operation against Azure DevOps. Confluence and Jira are accessed via direct HTTPS calls in a Node script because Atlassian's Cloud REST endpoints are stable, JSON-native, and authenticated with a static token — no first-party CLI is worth the dependency.

## Consequences

- The Plugin's external dependency footprint is `az` CLI + the `azure-devops` extension. The Doctor command verifies both.
- Atlassian fetching lives in a small in-repo Node script using the global `fetch` (built into Node 22+) and the shared Credential File loader — no `marked`-class runtime deps.
- Future GitHub or GitLab support will pick the right CLI/REST balance per-Platform; there is no global doctrine that all Source Platforms must use a CLI.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# 0006. Iteration state lives in the PR, not locally

**Status:** Accepted (2026-05)

## Context

The Re-review machinery needs to know which Revision of a PR was last reviewed so it can compute a delta diff. That state has to live somewhere durable that survives machine swaps, CI runs, and shared laptops — and that the Plugin can recover deterministically at the start of every Review.

Two alternatives were considered:

- **Local state file under `~/.unic-pr-review/state.json` keyed by PR URL.** Rejected — invokers swap machines, share laptops, and run the Plugin from CI. A local cache silently desyncs and would force every Review to re-detect mode from the PR anyway. (Note: a different, repo-local cache at `<cwd>/.unic-pr-review/<key>/` is used by the Approval Loop for Ctrl-C-resumable per-Finding decisions — see PRD §81. That cache is scoped to one Review's lifetime, not to long-term iteration state, and the two are not interchangeable.)
- **A custom ADO PR property (`pullRequest.properties`).** Rejected — properties are not visible in the PR UI, can be modified out-of-band, and require an extra round-trip. The Bot Signature is human-readable, lives in the same place as the Findings, and survives every ADO change short of comment deletion.

## Decision

The Plugin stores no local state about which Revisions it has reviewed. The prior reviewed Revision is recovered by finding the most recent comment authored by the Plugin in the PR's Threads (matched by the authenticated `az devops` user ID, cached at startup) and parsing the literal `Iteration N` suffix of its Bot Signature. First Review is detected when no such comment exists.

## Consequences

- The Bot Signature wording is load-bearing — `🤖 Reviewed by Claude Code — Iteration N` — and changing it breaks detection on any PR with an older Review. Any change requires a migration ADR.
- The Plugin must cache the authenticated user identity at startup so it never mistakes a human comment for its own. The Doctor command verifies this lookup succeeds.
- A user deleting the Plugin's prior comments is a legitimate way to force a fresh First Review; this is documented behaviour, not a bug.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# 0007. Re-review uses a delta diff, not a full PR diff

**Status:** Accepted (2026-05)

## Context

When an author pushes new commits to a PR that has already been reviewed, the Plugin needs to decide what scope of code to re-analyse and how to handle Findings raised in the previous Review. The naive option (re-run the full PR diff) is expensive and produces duplicate Findings; the right option needs to preserve cross-iteration context so the Plugin can reply to prior Threads rather than re-raise them.

Two alternatives were considered:

- **Always review the full base diff.** Rejected — invokers Re-review primarily to check that author changes since the last pass are correct. A full re-analysis would re-surface every prior Finding as a duplicate candidate and inflate token cost on every iteration.
- **Delta diff with no prior-Finding context.** Rejected — without prior Findings the Re-review Coordinator cannot do Thread Classification or auto-resolve `addressed` threads. The context is what enables the Reply-not-duplicate doctrine.

## Decision

A First Review analyses the diff from the PR's base branch to its current Revision. A Re-review analyses only the diff between the prior reviewed Revision and the current Revision. Prior Findings from earlier Reviews are passed as context so each Review Aspect agent can verify whether they're fixed, still pending, or obsolete — without re-raising them as new Findings.

## Consequences

- The ADO Fetcher must compute the delta from the Revision identified by the Bot Signature parser; if that Revision no longer exists (force-push that overwrote history), the Plugin falls back to First-Review mode and posts a Notice.
- Prior Findings injected into agent context are tagged with their original Severity and Thread state so agents can produce a structured verdict (fixed / partial / ignored) that the Re-review Coordinator consumes for Thread Classification.
- The cost of a Re-review scales with the size of the delta, not the size of the PR — which is the property invokers actually want.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# 0008. Conditional sub-agent spawning over per-file chunking

**Status:** Accepted (2026-05)

## Context

A PR review needs to cover several aspects (code quality, tests, error handling, security, etc.) without losing cross-file context and without burning tokens on aspects that don't apply to the changed files. We had to choose between splitting work by file or splitting it by review lens, and between running every lens unconditionally or only when relevant.

Two alternatives were considered:

- **Per-file chunking with a single reviewer agent.** Rejected — chunking loses cross-file context (a Finding in `service.ts` often depends on a type defined in `types.ts`), and the orchestration overhead of merging per-file passes into a coherent Review Summary is large.
- **Unconditional fan-out (every aspect agent every Review).** Rejected — most PRs touch only a few aspect categories; running all six aspect agents on every Review (plus the Intent Checker, which always runs first) wastes tokens and produces empty result blocks that clutter the output. Conditional spawning is the same pattern the Anthropic `pr-review-toolkit` uses and we adopt it deliberately.

## Decision

The Plugin reviews a PR by fanning out to specialised Review Aspect sub-agents in parallel, each handling the whole diff under one lens. Spawning is conditional on what the diff contains — `code-reviewer` always runs; `pr-test-analyzer` runs only when test files changed; `silent-failure-hunter` only when error handling changed; etc. The Plugin does NOT split the diff into per-file chunks.

## Consequences

- The Plugin needs a changed-file analyser that classifies the diff once and decides which aspect agents to spawn. This decision is made before any agent runs.
- The Intent Checker is the one exception — it always runs first (regardless of file types) because its output seeds every other agent's context. Its result is broadcast to every spawned aspect agent.
- Adding a new Review Aspect is additive: define the agent, write its spawn predicate, no orchestration rewrite needed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# 0009. Pre-PR mode is a peer operating mode, not a flag

**Status:** Accepted (2026-05)

## Context

Developers often want to run the Plugin against a working branch before opening a PR — to catch issues earlier, while there's no ADO PR URL yet. We had to decide whether to express that as a flag layered on top of the existing PR-URL workflow, or as a distinct operating mode driven by argument shape.

Two alternatives were considered:

- **Add a `--pre-pr` flag to the PR-URL mode.** Rejected — the flag would need to live alongside a now-meaningless PR URL argument and either ignore it or error. A peer mode driven by argument shape (URL present vs absent) is unambiguous and has no flag combinations to document.
- **Auto-promote Pre-PR to First-review by opening a PR mid-run.** Rejected — opening a PR is a user-owned action with side effects (notifications, CI runs, reviewer assignment). The Plugin never opens a PR.

## Decision

Running the Plugin without a PR URL enters Pre-PR mode: the Plugin resolves the base branch via `git symbolic-ref refs/remotes/origin/HEAD` (falling back to `develop`, then `main`, then `master`), computes the local diff, prompts the invoker for optional Work Item URLs (ADO or Jira, comma-separated) and Confluence URLs, then runs the full sub-agent fan-out and renders the Review in the terminal. No ADO write-back, no Approval Loop, no Re-review state machine.

## Consequences

- The base-branch fallback chain is policy: `origin/HEAD` → `develop` → `main` → `master`, hard-error otherwise. Repos with non-standard default branches must set `origin/HEAD` correctly via `git remote set-head`.
- Pre-PR mode bypasses the ADO Fetcher entirely; the Intent Checker still runs (against the URLs the invoker pasted) and the aspect agents still fan out. The Approval Loop is skipped because there is nothing to write back.
- Pre-PR mode with no pasted URLs and no `intent` to check is the closest the Plugin gets to a "pure linter" mode — explicitly supported, but the absence of intent is reflected in the rendered Summary so the invoker knows what was and wasn't checked.
20 changes: 20 additions & 0 deletions apps/claude-code/unic-pr-review/docs/adr/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# unic-pr-review — Architecture Decision Records

Plugin-scoped ADRs. Monorepo-wide decisions live in `../../../../../docs/adr/`.

- [0001](0001-multi-source-intent-with-shared-atlassian-credentials.md) — Multi-source intent gathering with shared Atlassian credentials
- [0002](0002-confidence-scored-findings.md) — Confidence-scored Findings with explicit Severity thresholds
- [0003](0003-interactive-approval-as-default.md) — Interactive Approval Loop as the default write path
- [0004](0004-hard-stop-on-missing-doc-credentials.md) — Hard-stop when intent sources are unreachable
- [0005](0005-az-cli-over-rest.md) — `az` CLI for Azure DevOps, custom HTTP for Atlassian
- [0006](0006-iteration-state-in-pr.md) — Iteration state lives in the PR, not locally
- [0007](0007-delta-diff-for-re-review.md) — Re-review uses a delta diff, not a full PR diff
- [0008](0008-conditional-sub-agent-spawning.md) — Conditional sub-agent spawning over per-file chunking
- [0009](0009-pre-pr-mode-as-peer-of-pr-modes.md) — Pre-PR mode is a peer operating mode, not a flag

## Planned

The following ADRs are referenced from the PRD but have not landed yet. Each is scheduled to ship with the implementation slice that first exercises the decision.

- **ADR-0010 — Provider as a folder bundle.** Captures the `providers/<name>/` shape over a single-file alternative. Lands with issue [#148](https://github.com/unic/unic-agents-plugins/issues/148) (ADO first-review preview).
- **Amendment to ADR-0001 — provider-owned work-item discovery.** Adds a note that each Provider owns `discoverWorkItems(prMetadata)` and the Intent Checker consumes the normalised list. Lands with issue [#148](https://github.com/unic/unic-agents-plugins/issues/148).
Loading
Loading