fix(unic-pr-review): remove identity matching, detect re-reviews by hidden Iteration Marker#195
Merged
orioltf merged 7 commits intoJun 4, 2026
Conversation
…teration Marker `az devops user show` resolves via the admin-only `_apis/UserEntitlements` API (HTTP 404 for normal reviewers), making the doctor identity check and the `author.id`-based re-review detection permanently broken for the typical user. Replace identity matching end-to-end with the hidden Iteration Marker (`<!-- unic-pr-review:iteration=N -->`) that `renderFooter` now embeds in every bot-authored comment. `parseSignature` keys on `ITERATION_MARKER_REGEX` only; `checkAzIdentity` is deleted from `doctor.mjs`; agent prompts and commands are updated to remove the `identity` / `identityId` fields from every contract. Resolves #194. Conforms to ADR-0006 (revised 2026-06). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
Comprehensive PR ReviewPR: #195 — SummaryClean, targeted cleanup PR. Identity-matching removal is complete end-to-end: Verdict:
MEDIUM — Fix Before MergeREADME.md says "six checks" but doctor now runs fiveLocation: # Current (wrong)
It runs six checks and tells you exactly what is missing before any Review is attempted.
# Recommended
It runs all prerequisite checks and tells you exactly what is missing before any Review is attempted.Removing the hard-coded count avoids the same drift if a check is added/removed again. LOW — Defer or Follow-upView 4 low-priority suggestions1. Duplicate
2. Two describe blocks cover same credential-failure scenarios
3.
4.
What's Good
Suggested Follow-up Issues
Reviewed by Archon comprehensive-pr-review workflow |
- README: "six checks" → "all prerequisite checks" (avoids count drift) - signature.mjs: add NaN guard after parseInt (mirrors existing !match guard pattern) - doctor.test.mjs: remove duplicate allOkExec shadow inside credential-errors block - doctor.test.mjs: merge overlapping "missing credentials" + "credential errors" describe groups; consolidated "credential errors" is the canonical home; added "One or more checks failed" assertion to both tests - doctor.test.mjs: assert "All checks passed" in all-ok Jira-silence test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
⚡ Self-Fix Report (Aggressive)Status: COMPLETE Fixes Applied (5 total)
View all fixes
Tests Added(none — assertions added to existing tests) Skipped (0)(none — all findings addressed) Suggested Follow-up Issues(none) Validation✅ Type check | ✅ Lint | ✅ Tests (481 passed) Self-fix by Archon · aggressive mode · fixes pushed to |
`highest-N-wins` and `picks the highest iteration when multiple signatures exist` both pass [1,5,2]-equivalent inputs and assert priorRevisionId===5. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The archon self-fix left the new test file unformatted, which made `biome ci` emit one error and failed Root checks on PR #195. Format fix only; no logic change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the unic-pr-review plugin to remove all Azure DevOps identity resolution (which fails for non-admin users) and makes re-review detection rely solely on a hidden Iteration Marker embedded in the bot signature, aligning implementation with ADR-0006 / issue #194.
Changes:
- Replace signature detection from visible footer/author identity to the hidden
<!-- unic-pr-review:iteration=N -->Iteration Marker. - Remove the
az devops user showidentity check fromdoctorand update related docs/tests. - Update agent contracts/docs to drop identity plumbing; bump plugin version to
2.1.1.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/claude-code/unic-pr-review/scripts/lib/signature.mjs | Emit Iteration Marker in footer; parse iteration from marker only; drop priorAuthorUserId. |
| apps/claude-code/unic-pr-review/tests/signature.test.mjs | Update tests for marker-based detection and new footer format. |
| apps/claude-code/unic-pr-review/scripts/doctor.mjs | Remove identity check; cascade now stops at session validity before Atlassian checks. |
| apps/claude-code/unic-pr-review/tests/doctor.test.mjs | Remove identity stubs/tests; update waterfall and output assertions. |
| apps/claude-code/unic-pr-review/scripts/parse-prior-signature.mjs | Update docstring to reflect marker-based pre-filtering. |
| apps/claude-code/unic-pr-review/agents/ado-fetcher.md | Remove identity cache step; filter bot threads by marker; renumber steps/output contract. |
| apps/claude-code/unic-pr-review/agents/ado-writer.md | Update summary-thread detection to key on marker, not author.id. |
| apps/claude-code/unic-pr-review/agents/re-review-coordinator.md | Update classification rules to treat marker presence as “bot comment”. |
| apps/claude-code/unic-pr-review/commands/review-pr.md | Remove identity-related error path and coordinator/writer inputs. |
| apps/claude-code/unic-pr-review/commands/doctor.md | Update checklist and remediation guidance to remove identity step. |
| apps/claude-code/unic-pr-review/README.md | Generalize doctor description now that check count changed. |
| apps/claude-code/unic-pr-review/AGENTS.md | Update ADR-0006 doctrine bullet to marker-based detection + doctor behavior. |
| apps/claude-code/unic-pr-review/CHANGELOG.md | Add 2.1.1 entry describing identity removal and marker detection. |
| apps/claude-code/unic-pr-review/package.json | Bump version to 2.1.1. |
| apps/claude-code/unic-pr-review/.claude-plugin/plugin.json | Bump plugin version to 2.1.1. |
| apps/claude-code/unic-pr-review/.claude-plugin/marketplace.json | Bump marketplace version to 2.1.1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…umber (#195) This PR renamed ado-fetcher.md "Step 4a" to "Step 3a" but left two cross-references pointing at the old name, so a maintainer following them would not find the mode-detection logic. Why: keep internal cross-references consistent with the renumbered ADO Fetcher steps introduced in the same change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ceholders (#195) Addresses two Copilot review comments on agents/ado-fetcher.md: - Frontmatter claimed `rawDiff` is "returned empty" unconditionally, but Step 5 populates it with the git delta diff in re-review mode. Now states it is empty in first-review modes and carries the delta diff in re-review mode. - The emit-result JSON template quotes `prMetadata`/`revisions`/`threads` as string placeholders. Clarified above the block that each `<…>` token is replaced with the real value it names and these three are objects, not strings — keeping the template shape intact per maintainer preference. Why: the agent spec is the contract downstream consumers reason about; internal contradictions mislead them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…0006 (#195) US-34 and the "Identity caching" / "Signature module" implementation bullets in PRD.md still describe the `az devops user show` identity mechanism this PR removed. Added inline "Superseded by ADR-0006" notes pointing at the live source of truth rather than rewriting the frozen spec. Why: US-34 directly contradicted the shipped doctor checks; leaving the drift unmarked is how a future change reintroduces the identity probe and the non-admin 404 bug. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
orioltf
deleted the
archon/task-feature-unic-pr-review-194-remove-identity-matchin
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
az devops user showresolves identity via_apis/UserEntitlements— an org-administration surface readable only by Project Collection Administrators. Normal reviewers get HTTP 404, so the doctor's identity check andauthor.id-based re-review detection were permanently broken for the typical user (issue #194).The fix removes identity end-to-end and replaces it with the hidden Iteration Marker
<!-- unic-pr-review:iteration=N -->that the Bot Signature already embeds in every bot-authored comment, making re-review detection self-sufficient (ADR-0006, revised 2026-06).What changed
scripts/lib/signature.mjs—renderFooter(n)now appends the Iteration Marker on its own line;parseSignaturematchesITERATION_MARKER_REGEXonly (not the visible footer text);priorAuthorUserIdremoved fromParsedSignaturescripts/doctor.mjs—checkAzIdentitydeleted;runDoctornow runs five checks (was six)scripts/parse-prior-signature.mjs— docstring updated: caller pre-filters by Iteration Marker, not author identityagents/ado-fetcher.md— Step 1 (identity cache) deleted; Step 4a filters threads by Iteration Marker presence incomments[0].content;identityfield removed from output JSON contract; steps renumbered 1-6agents/ado-writer.md— Step 7a summary lookup now matches by Iteration Marker (<!-- unic-pr-review:iteration=), notauthor.id === identity.id;identityremoved from re-review inputagents/re-review-coordinator.md—identityIdremoved from input; human replies identified by absence of Iteration Marker; persistent-unaddressed logic counts marker-bearing commentscommands/review-pr.md—identity-cache-failederror path removed;identityId/signaturePrefixremoved from Coordinator input;identityremoved from ADO Writer re-review inputcommands/doctor.md— check Develop #4 (az devops user show) removed; checks renumbered to 5AGENTS.md(plugin) — ADR-0006 doctrine bullet updated to reflect marker-based detectionTests
tests/doctor.test.mjs—checkAzIdentitydescribe block deleted; identity stubs removed fromallOkExec; waterfall assertions updatedtests/signature.test.mjs—priorAuthorUserIdassertions removed; 5 new tests added: Iteration Marker inrenderFooter, marker on its own line, visible-footer-only rejection, quote-reply rejection, marker round-trippnpm --filter unic-pr-review test— 483/483 passpnpm --filter unic-pr-review typecheck— cleanNon-goals (per issue #194)
connectionDataHTTP call or alternative identity source~/.unic-azure.json/loadAzureCredswiringCloses #194
🤖 Generated with Claude Code