feat(mcp): report_duplicate tool + request_review CAS generalization - #308
Merged
Conversation
No end-user-facing UX applies (pure CI/infra change) — lightweight DX pass instead, comparing failure surfacing to golangci-lint annotations vs plain-log precedent (ESLint/gofmt) and flagging --ci flag as a zero-cost quick win. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Documents the BacklogStatus enum pattern, mark3labs/mcp-go tool registration/handler shape, and confirms the ent status column needs no migration for a new terminal value.
Evaluates TOML parsing (go-toml v2), regex engine (stdlib RE2), and hot-reload (fsnotify) choices, plus config-aggregation frameworks (Viper/koanf) and existing plugin-system prior art (Caddy/Vector/Grafana), for the user-extensible agent detector plugin loader. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers RE2/ReDoS guarantees and limits, fsnotify hot-reload gotchas (temp-file-rename, debounce, inotify limits, macOS/Linux differences), TOML/config pitfalls, the atomic-swap concurrency pattern to reuse from worktree_git.go, and fail2ban/ESLint/VS Code prior art. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Documents existing table/sort patterns to mirror (SessionDetailDrawer's Tools Breakdown, backlog page's aria-sort), the proto gaps blocking AC-1 (per-turn) and AC-6 (cache split), and concrete labeling/empty-state/ sort-order decisions for AC-1 through AC-3.
Research pass on what commonly breaks when adding to an already-live analytics feature: the #280 silent-$0.00 precedent, TokenStore RWMutex contention, jumping-list risk for async-sorted cost data, stale registry schema.json vs real frontend entry shape, and the backlogItemEventSender pattern needed before WatchInsights is testable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Documents edge cases and unstated scope for the 6 gap-closing ACs: AC-1 requires a proto change (TurnTimeline isn't exposed over the wire, not just unwired in the frontend); AC-2's SessionList.tsx has zero token data wired in today (TokenBadge is unused dead code, contradicting requirements.md); AC-3 needs no backend change (ListSessionTokens sort_by already implemented, SessionsTable.tsx's sort is just hardcoded); AC-4 needs the same narrow-interface refactor WatchBacklogItems already established for testing connect-go streaming RPCs; AC-5's 5 target files share one +feature marker with an existing aggregated registry entry, creating a collision risk; AC-6 is free (client-side formula, data already on the wire).
Phase 3 planning artifacts: creative pass, domain glossary, pattern decisions, task breakdown (3 epics / 4 stories / 9 tasks), and ADR-001 justifying -p 1 over an isolated second -race invocation for the CI contention mitigation. Also commits the earlier requirements.md and research/*.md that were still uncommitted from phases 1-2.
Phase 3 architecture/task-breakdown plan (7 verified pipeline hops from regex capture through proto to the SubStatusChip badge) plus ADR-001 documenting the statusCacheEntry cache-coherence requirement between GetCurrentStatus and GetStatusAndIdleInfo. Also commits the requirements and research docs from earlier phases that were still untracked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 3 planning artifacts: plan.md (3 phases, 6 epics, 14 stories, 41 tasks, 30-term glossary) plus four ADRs covering the go-toml/v2 dependency, the registry-level copy-on-write snapshot (vs. the existing unused StatusDetector YAML loader), TOML schema v1, and the RE2 trust boundary. Also picks up the architecture/features research files that were still untracked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Phase 3 planning artifact for the token-cost-tracking gap-closure project:
domain glossary, pattern decisions (new-RPC vs bolt-on for AC-1, interface
extraction for AC-4's WatchInsights test, client-side derivation for AC-6),
risk control, and a 5-phase task breakdown sequenced by risk (AC-4/AC-5
low-risk first, AC-2's new SessionList data join last). Also commits the
requirements.md and research/{build-vs-buy,stack}.md artifacts from earlier
phases that were left uncommitted.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Maps each requirements.md scope item and plan.md task to concrete verification: regression checks for the two affected integration tests, a stress/flake-verification repro (-count=10 under artificial CPU contention), the Task 1.2.4 non-fatal-teardown check, the Task 2.1.1 coverage-artifact/-race-scope check, and the Task 2.1.2 averaged (>=3-run) wall-clock measurement. No user-facing surface, so UX Acceptance Tests section is N/A; no schema change, so Migration Test is N/A.
Wireframes, interaction flows, error/empty states, and UX acceptance criteria for the 4 user-facing surfaces in Phases 2-4 of the implementation plan (SessionsTable click-to-sort, ModelBreakdownChart cache-hit-rate label, SessionDetailDrawer per-turn table, SessionList Sort: Cost option). Flags a concrete contrast/layout risk in reusing TokenBadge.css.ts's badgeVariant.warning for outlier-turn highlighting in a table-cell context. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adversarial/architecture review, UX design, validation plan, and pre-mortem for the subagent-spawn-tracking feature; plan.md patched to resolve 2 architecture-review CONCERNS, 1 adversarial-review safety concern, 2 cross-artifact-consistency BLOCKERs (title-attribute contradiction with ux.md, missing NaN/negative test coverage), and the pre-mortem's P1 (corrected a false "no debounce precedent" claim). Triad review (Product/UX/Engineering) verdict: READY TO BUILD.
…-url-tests Adds architecture review, adversarial review, and pre-mortem docs from SDD Phase 3/4, and patches plan.md to resolve the cross-artifact consistency blocker (missing runner-concurrency check task) and the pre-mortem P1 item (testSocketOnce misdiagnosis prevention).
Maps all 6 requirements.md acceptance criteria plus plan.md's Phase 1-2 story-level Given/When/Then scenarios to concrete Go test names, organized as unit vs. integration, so implementation starts test-first.
Research and planning surfaced that this backlog item duplicates the already-planned, already-reviewed project_plans/flaky-hook-url-tests/ (same root cause, same two tests, ADR-001 + full task breakdown never implemented). implementation/plan.md consolidates rather than re-derives: it adopts that plan/ADR wholesale, reproduces the full Epic/Story/Task breakdown here for self-containment, and re-verifies every file:line reference against the current tree (all accurate, no drift found). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…or detector-plugins Phase 4 (sdd:4-validate): validation.md test suite, pre-mortem.md failure analysis, and cross-artifact consistency check. The pre-mortem's P1 finding (DetectForProgram/the plugin registry has zero production call sites, so a loaded plugin would never change what a user's session actually shows) was independently confirmed by adversarial-review.md's own Blocker and by direct repo verification (idle.go's live detection path uses getDefaultPatterns(), not the registry this plan builds). Resolved by adding Epic 2.4 to plan.md, wiring ClaudeController.Start to resolve its detector via the new registry snapshot instead of the always-generic default. Also folds in a Hardening Addendum (compile-time budget, total-file-count cap, rebuildSnapshot context cancellation, InitPlugins re-entrancy guard, non-fatal seed-file-write) closing 5 previously-open adversarial-review Concerns, and requirements.md amendments (Target User, Success Metric, Risky Assumption sections; corrected plugin-directory and version-field wording) from a 3-round product/engineering triad review.
project_plans/backlog-status-transitions/research/architecture.md belongs to an unrelated, concurrently in-progress backlog item and was swept into the previous commit because it was already staged in the shared index at commit time. Untracking (not deleting) it here so it returns to the working tree for its own session to commit under its own message.
…rl-race-flake Phase 4 (validate) for the flaky hook-URL/MCP-URL CI test backlog item: validation.md, pre-mortem.md, architecture-review.md, adversarial-review.md. Cross-artifact consistency review surfaced 3 blockers (AC #1's N/method never decided, a citation to a "Success Metrics" section that didn't exist in this project's own requirements.md, and a stale file path) — fixed by adding this project's own Success Metrics section (N=20, distinct decision from the sibling project) and correcting the path. Pre-mortem's 3 P1 items (unfolded review concerns, un-rebutted build-vs-buy divergence, unenforced AC#4 evidence gate) are folded directly into plan.md and research/build-vs-buy.md rather than left as unread siblings for Phase 5. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Evaluates GitHub verification (extend existing github/ + session.ParseGitHubURL, no google/go-github), duplicate-marking helpers (none exist, build new), and CAS precondition mechanism (reuse TransitionBacklogItemStatus as-is). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
Phase 1-2 planning artifacts for backlog item da58b867 (report_duplicate MCP tool + request_review CAS generalization). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
Phase 3 planning output for the report_duplicate MCP tool + request_review CAS-precondition generalization (item da58b867). 11 epics / 26 stories / 51 tasks, one Given-When-Then example per FR1-FR10, and 4 ADRs covering the GitHub-verification dispatcher/auth/error-classification design, the TriggeredByAgent audit-attribution scope, report_duplicate's idempotency rule, and the (flagged, owner-confirm-pending) decision not to extend FR2's active-reviewer refusal to report_duplicate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
…session A second, independent SDD planning pass turned out to be running on this same self-referential backlog item concurrently — its ADR-001/ADR-002 files appeared in decisions/ between this plan's mkdir and git add. Renumber this plan's four ADRs to ADR-005-008 to stop clobbering the other session's numbering, and flag the one substantive disagreement found (github.GetPR HTTP addition vs. keeping the gh-CLI GetPRInfoCtx path) as unresolved in both plan.md and ADR-005 for whoever picks up implementation to reconcile. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
…ite bug in backlog-self-resolve plan Resolves a duplicate-execution artifact from planning: two independently-produced ADRs disagreed on GitHub PR-verification (gh CLI vs HTTP). Adopted the HTTP-only approach (ADR-002) per this repo's subshell-avoidance convention and the auth-consistency risk pitfalls.md flagged; deleted the superseded draft and renumbered ADR-001..005 sequentially. Also fixes a real bug a second research pass surfaced: report_duplicate's verification-notes write must append rather than overwrite, or it silently discards evidence from an earlier request_review call on the same ItemSession. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
Architecture review (1 blocker, 3 concerns) and adversarial review (1 blocker, 9 concerns, 5 minors) findings recorded and repaired: - Fixed FR10's stuck-item detector citation: pr_pending_no_pr only covers items with NO PrNumber (wrong shape for this feature); the correct existing coverage is ReconcilePRPending's PRReadyUnmerged/PRNeedsFix family, which operates on pr_pending items that DO have a PR reference (this item's actual scenario). No new detector needed, just the right citation. - Fixed verifyGitHubRefExists's Domain Glossary/GWT wording, which claimed a (bool, error) contract matching verifyPR when the actual designed signature is single-error-return. - Rewrote Task 4.2.6a's garbled concurrency test to actually call both handlers, and corrected Story 4.2.6's acceptance criteria (the "exactly one status-event row" premise was false for two legitimately sequential transitions). - Ratified ADR-003/ADR-004 to Accepted; tightened ADR-005's argument so it doesn't contradict Story 3.1.2's own independently-justified 4th refusal condition. - (Already applied by a prior pass: hasActiveReviewSession export instead of a 4th duplicate copy — confirmed no import cycle; idempotency substring- match fix; verifyGitHubRef injectable test seam; corrected test-file citations to server/services/backlog_github_rpc_test.go's resetGhBaseURL.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
…ve plan Scoped re-review confirmed both prior blockers resolved. Cleaned up two leftover contradictions the re-review flagged: Story 3.2.2's narrative line still described verifyGitHubRefExists as verifyPR's (bool, error) shape, and Task 3.1.2b still offered a non-functional plain-== alternative that can never match the actual persisted notes format. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
…licate) Maps plan.md's Phase 4 test tasks (~22 functions across Epics 4.1-4.3) to FR1-FR10, confirms 10/10 requirement coverage, and identifies 8 genuine test gaps (G1-G8) found by cross-referencing architecture-review.md and adversarial-review.md concerns that aren't yet reflected as Phase 4 tasks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
A concurrent re-run pre-mortem verified directly against session/review_gate.go that ADR-005's justification for not refusing report_duplicate when a reviewer is already active -- "TriggerReviewForSession's own idempotency" -- was false: ReviewGateRunner.Run has no active-reviewer dedup check anywhere and would unconditionally spawn a second, concurrent review session for the same item. Fixed with the narrowest correct scope: Task 3.3.3b's TriggerReviewForSession call is now conditional on the same activeReview boolean Task 3.3.3a already computes for message branching, instead of reversing ADR-005 (which would make FR5's literal "still succeeds while a reviewer is active" text unreachable) or patching the shared spawnReviewGate/Run infrastructure (broader blast radius, affects every review-gate spawn path in the codebase, not just this feature). report_duplicate still transitions the item and persists evidence either way; only the redundant immediate spawn is skipped. Extended Task 4.2.5a's test to assert the trigger is/isn't called in each branch. Also accepted: a GHES-support gap (F2) explicitly triaged as an accepted P2 for v1 (github.com only, not blocking); a trust-boundary disclosure line added to the tool description (F5); and a re-derived validation.md superseding a draft written against an intermediate plan.md revision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
…er guard Epic 2.1 (FR1/FR9): add allowedSelfResolveSourceStatuses + the validateSelfResolveSource chokepoint (server/mcp/tools_backlog.go) so request_review's CAS precondition is pinned to the validated observed status (in_progress or pr_pending), never a hardcoded constant or raw item.Status. Populate BacklogItemPrecondition.Note, switch TriggeredBy to TriggeredByAgent on both source paths (ADR-003), and give a CAS-race loser a distinct non-retry message instead of the generic transition- failed text. Epic 2.2 (FR2): export HasActiveReviewSession from server/services/backlog_service_triage.go (was unexported) and reuse it in request_review's new pr_pending-only active-reviewer guard. Fails closed on a ListItemSessions storage error (INTERNAL_ERROR, never a silent pass-through) via a new listItemSessionsFn test seam on backlogHandlers, mirroring the existing verifyPRMatchesBranch/ resolveSessionBranch shape — session.Storage.ListItemSessions has no swappable-repository seam of its own (hard type-asserts to *EntRepository), so this was needed to make the fail-closed path testable without a real DB failure. Epic 4.1: the 5 pre-existing TestRequestReview_* tests pass unmodified (regression), plus 6 new tests covering the pr_pending success path, whitelist rejection (table-driven), both active-reviewer-guard branches, the fail-closed storage-error path, and the CAS-race-loser message (genuine goroutine concurrency, stable across 20 runs under -race). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
…ess-text assertion Architecture review's residual finding: ReconcilePRPending's IsPRMerged/GetPRStatus calls can themselves fail (revoked token, GitHub outage) and continue past the tick without marking anything stuck -- a pre-existing characteristic unrelated to this feature, but "runs unconditionally" overstated it. Also extends Task 4.2.1b to assert the affirmative "Reviewer notified" success text, closing a gap validation.md flagged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
Implements Epics 3.1-3.4 of the backlog-self-resolve plan: a new report_duplicate MCP tool letting a work session route a backlog item to review when it discovers the work duplicates an already-shipped GitHub PR/issue/commit, plus Epic 4.2's full test suite (24 tests). - Handler skeleton + FR6 refusal checks (SkipReviewGate, role, link, disallowed source status), reusing validateSelfResolveSource and services.HasActiveReviewSession rather than duplicating either chokepoint. - GitHub verification dispatcher (verifyGitHubRefExists) behind an injectable verifyGitHubRef seam, with the 3-channel error split (ErrNotAuthenticated / ErrGitHubRefNotFound / ErrGitHubAccessDenied vs. plain transient) per ADR-002. - CAS transition to review (never done/archived, ADR-001) with an append-not-overwrite VerificationNotes fix and ADR-004 idempotency (exact-retry no-op, differing second ref rejected). - FR5 success messaging + the safety-critical fix from Task 3.3.3b: the review-gate trigger is now conditional on !activeReview (TriggerReviewForSession has no dedup check against an already-active reviewer, so calling it unconditionally would spawn a second, concurrent review session). - MCP registration with explicit FR10 retry guidance. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
…teraction test Spec compliance sweep caught the mismatch: the fixture correctly seeds the item at "review" (matching SetBacklogItemPRAndTransition's real precondition, per the adjacent inline comment already explaining this), but the summary and step comments still described the plan's original, inaccurate premise. No behavior change -- comment only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
repos.go's SearchUserRepos/ListRepoIssues/GetIssue/GetPR and commits.go's GetCommit each repeated an identical ~25-line 401/403/429/default classification block. Extracted classifyGHResponse (github/http_client.go) parameterized by notFoundMsg (empty for the two list/search endpoints, which have no 404 semantics) and a sentinels flag (false for SearchUserRepos/ListRepoIssues to preserve their existing plain-error behavior, true for GetIssue/GetPR/GetCommit to keep wrapping ErrGitHubAccessDenied/ErrGitHubRefNotFound). Also swapped bare int literals for http.Status* constants and fmt.Errorf (no verbs) for errors.New (staticcheck S1028) in the consolidated logic. Also, two unrelated nits flagged in the same review pass: - tools_backlog.go: renamed checkGitHubRef -> verifyRef to match the file's field->shortened-method naming convention (verifyGitHubRef field, verifyPRMatchesBranch -> verifyPR, resolveSessionBranch -> sessionBranch). - tools_backlog_test.go: renamed TestReportDuplicate_LoserGetsDistinctMessage_WhenRacingReportPRCreated, which is misleadingly named "Racing" despite testing sequential state-machine composition on one goroutine, to TestReportDuplicate_RejectsThirdCall_AfterSequentialReportPRCreatedThenReportDuplicate. Verified SearchUserRepos/ListRepoIssues behavior is unchanged: no existing tests cover them directly (confirmed via repo-wide grep), so verification is by diff review — classifyGHResponse(resp, "", false) produces byte-identical error strings to the removed inline blocks for every status code path, and go build/vet/test -race stay green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces backlog self-resolution capabilities by adding a new report_duplicate MCP tool and widening request_review’s CAS precondition to allow re-requesting review from pr_pending, supported by GitHub-side reference verification helpers and extensive new tests.
Changes:
- Added planning/ADR documentation for multiple SDD efforts (backlog self-resolve, detector plugins, CI flake investigations, etc.).
- Added GitHub REST helpers (
GetPR,GetCommit) + shared HTTP error classification with sentinel errors for not-found/access-denied cases. - Added unit tests covering the new GitHub helpers’ status-code classification behavior.
Reviewed changes
Copilot reviewed 67 out of 87 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| project_plans/subagent-spawn-tracking/implementation/architecture-review.md | Adds architecture review notes/concerns for subagent spawn tracking plan. |
| project_plans/subagent-spawn-tracking/implementation/adversarial-review.md | Adds adversarial review notes for subagent spawn tracking plan. |
| project_plans/subagent-spawn-tracking/decisions/ADR-001-subagent-count-cache-coherence.md | Documents cache-coherence decision for subagent count. |
| project_plans/jest-ci-wiring/research/ux.md | Adds DX/UX research for wiring Jest into CI. |
| project_plans/flaky-hook-url-tests/research/ux.md | Adds UX note for CI/test-only flake work. |
| project_plans/flaky-hook-url-tests/research/features.md | Adds feature landscape research for race-induced CI flakiness. |
| project_plans/flaky-hook-url-tests/research/build-vs-buy.md | Adds build-vs-buy analysis for stabilizing flaky integration tests. |
| project_plans/flaky-hook-url-tests/research/architecture.md | Adds architecture analysis of async hook-injection path + test waiters. |
| project_plans/flaky-hook-url-tests/requirements.md | Adds requirements for CI flake mitigation. |
| project_plans/flaky-hook-url-tests/implementation/pre-mortem.md | Adds pre-mortem for flaky hook URL test stabilization. |
| project_plans/flaky-hook-url-tests/implementation/architecture-review.md | Adds architecture review for flaky hook URL test plan. |
| project_plans/flaky-hook-url-tests/implementation/adversarial-review.md | Adds adversarial review for flaky hook URL test plan. |
| project_plans/flaky-hook-url-tests/decisions/ADR-001-p1-flag-over-isolated-invocation.md | Records decision to use -p 1 vs isolated coverage merge. |
| project_plans/detector-plugins/research/ux.md | Adds UX research for TOML detector plugins. |
| project_plans/detector-plugins/research/stack.md | Adds stack research and design implications for TOML plugins + hot reload. |
| project_plans/detector-plugins/requirements.md | Adds requirements for detector plugins. |
| project_plans/detector-plugins/implementation/pre-mortem.md | Adds pre-mortem for detector plugins implementation. |
| project_plans/detector-plugins/implementation/architecture-review.md | Adds architecture review for detector plugins plan. |
| project_plans/detector-plugins/implementation/adversarial-review.md | Adds adversarial review for detector plugins plan. |
| project_plans/detector-plugins/decisions/ADR-004-plugin-trust-boundary-and-resource-caps.md | Documents plugin trust boundary + resource caps. |
| project_plans/detector-plugins/decisions/ADR-003-plugin-toml-schema-v1.md | Documents TOML schema v1 decisions. |
| project_plans/detector-plugins/decisions/ADR-002-registry-level-snapshot-not-statusdetector-yaml-path.md | Documents snapshot-based design choice over unused YAML loader. |
| project_plans/detector-plugins/decisions/ADR-001-go-toml-v2-for-plugin-parsing.md | Documents choosing go-toml/v2 for parsing. |
| project_plans/ci-hookurl-race-flake/requirements.md | Adds consolidated requirements for hook URL CI flake. |
| project_plans/ci-hookurl-race-flake/implementation/pre-mortem.md | Adds pre-mortem for consolidated CI flake plan. |
| project_plans/ci-hookurl-race-flake/implementation/architecture-review.md | Adds architecture review for consolidated CI flake plan. |
| project_plans/ci-hookurl-race-flake/implementation/adversarial-review.md | Adds adversarial review for consolidated CI flake plan. |
| project_plans/backlog-self-resolve/research/ux.md | Adds UX research focused on MCP tool messaging + retry guidance. |
| project_plans/backlog-self-resolve/requirements.md | Adds requirements for backlog self-resolve tooling. |
| project_plans/backlog-self-resolve/implementation/pre-mortem.md | Adds pre-mortem identifying/reporting key failure modes for self-resolve. |
| project_plans/backlog-self-resolve/implementation/architecture-review.md | Adds architecture review and residual concerns for self-resolve plan. |
| project_plans/backlog-self-resolve/decisions/ADR-005-report-duplicate-no-active-reviewer-refusal.md | Documents decision to avoid active-review hard refusal for report_duplicate. |
| project_plans/backlog-self-resolve/decisions/ADR-004-report-duplicate-idempotency.md | Documents idempotency behavior for report_duplicate. |
| project_plans/backlog-self-resolve/decisions/ADR-003-triggeredby-agent-scope.md | Documents switching audit attribution to TriggeredByAgent. |
| project_plans/backlog-self-resolve/decisions/ADR-002-github-ref-verification-dispatcher.md | Documents GitHub verification dispatcher + error classification approach. |
| project_plans/backlog-self-resolve/decisions/ADR-001-no-new-backlog-status-for-duplicates.md | Documents representing duplicates via existing statuses + notes. |
| github/repos_pr_test.go | Adds REST-based GetPR tests including sentinel classification. |
| github/repos.go | Adds sentinel errors, GetPR REST helper, and refactors error handling to classify helper. |
| github/http_client.go | Adds shared classifyGHResponse helper for HTTP status classification. |
| github/commits_test.go | Adds GetCommit tests + in-package GhBaseURL reset helper. |
| github/commits.go | Adds REST-based GetCommit helper using shared classification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fixes 5 confirmed MAJOR findings from the Gate 2 review pass:
1. TestRequestReview_ReportsDistinctMessage_WhenCASPreconditionFails
called require.NoError from inside a spawned goroutine — t.FailNow()
only unwinds that goroutine, not the test goroutine. Both racers now
send {result, err} through the channel and every require/assert call
happens in the main test goroutine. The same channel type is reused
for the new report_duplicate CAS test (fix 3).
2. Added TestReportDuplicate_VerifyGitHubRefExists_DispatchesPRTypeToRealGetPR,
the first test in the package that leaves verifyGitHubRef nil so the
real verifyGitHubRefExists dispatch switch (PR/Issue/Commit -> GetPR/
GetIssue/GetCommit) actually runs, pointed at an httptest.Server via
githubpkg.GhBaseURL (mirrors github/repos_pr_test.go's pattern).
3. Added TestReportDuplicate_ReportsDistinctMessage_WhenCASPreconditionFails,
mirroring the request_review CAS regression test for report_duplicate's
identical errors.Is(transErr, session.ErrPreconditionFailed) branch.
4. Added TestReportDuplicate_DoesNotTreatPrefixRefAsIdempotentMatch,
proving a shorter ref that is a literal string-prefix of an
already-recorded longer ref (.../pull/27 vs .../pull/272) is not
misclassified as the idempotent no-op retry.
5. requestReview's verification-notes persistence overwrote
VerificationNotes via UpdateItemSessionVerificationNotes, silently
erasing prior evidence (e.g. from an earlier report_duplicate call)
on the same ItemSession. reportDuplicate already had this fixed with
an append pattern; applied the same fix to requestReview and added
TestRequestReview_AppendsToExistingVerificationNotes_RatherThanOverwriting
as a new test, leaving the existing
TestRequestReview_PersistsVerificationNotesOnWorkSession assertions
unmodified per AC9/FR9.
All 5 pre-existing TestRequestReview_* tests still pass with their
original assertions intact. go build/vet/gofmt/golangci-lint clean on
server/mcp and github packages; go test ./server/mcp/... ./github/...
-race all green.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XxSH3xUu3fyFnprsybnRiN
Two real findings from the Copilot review, fixed: - classifyGHResponse's notFoundMsg 404 branch didn't drain resp.Body, preventing HTTP connection reuse on keep-alive transports. Drain it, matching the pattern already used in the 403 rate-limit branches. - resetGhBaseURL restored GhBaseURL to a hardcoded default instead of the captured prior value -- fragile if a future test changes GhBaseURL before calling this helper. Capture and restore the actual prior value. Three other comments (claiming a missing encoding/json import x2, and claiming server/mcp files are absent from the diff) were declined as factually incorrect -- verified against the actual file contents, a clean go build, and gh pr diff's file list. The review's own banner noted it "was unable to run its full agentic suite" and only reviewed 67/87 changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
Owner
Author
|
Reopening to retrigger CI — no workflow runs registered on this PR despite two pushes, while other concurrent PRs on this repo triggered normally in the same window. |
…cts)
Branch had diverged 21+ commits from main. All feature files
(server/mcp/tools_backlog.go, server/mcp/tools_backlog_test.go,
server/services/backlog_service_triage.go, github/*.go, session/backlog.go)
auto-merged cleanly with zero conflicts. The only real conflict was on two
unrelated planning docs (project_plans/token-cost-tracking/{implementation/plan.md,requirements.md})
that both branches independently added -- resolved by taking main's version,
removing them from this PR's diff entirely since they're not part of this
feature.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
Contributor
✅ Registry ValidationTest Coverage: 24/181 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
Contributor
Frontend Terminal Throughput |
Contributor
🎬 E2E Feature Demos2 shard(s) recorded feature flows for this PR. recordings shard 1 Demo preview opens directly in browser (single-file HTML). Raw WebM recordings in ZIP. Expires after 30 days. |
Root cause: TestRequestReview_ReportsDistinctMessage_WhenCASPreconditionFails synchronized only the *start* of its two racing goroutines via startBarrier.Wait(). That doesn't guarantee both goroutines' fresh GetBacklogItem reads (which feed validateSelfResolveSource before the CAS write) complete before either write does. Locally this ordering held 20/20 under -race, but on CI's runners (more cores, different scheduling) one goroutine could finish its full read->whitelist->write sequence before the other's first read even executed — so the "loser" observed the post-write Status: "review" and failed the whitelist check (ErrInvalidArgument) instead of racing the actual CAS write (ErrPreconditionFailed / ErrInternalError), which is the behavior this test exists to exercise. This is a test-harness determinism bug, not a production bug: the CAS write itself is already proven race-safe by TestTransitionBacklogItemStatus_should_letExactlyOneWinnerThrough_When_TwoWritersRaceConcurrently. Fix: add an injectable getBacklogItemFn seam on backlogHandlers (mirrors the existing listItemSessionsFn/itemSessionsFor pattern — nil falls back to h.storage.GetBacklogItem, so all existing callers are behavior-preserving) and wrap it in the test with a sync.WaitGroup(2) read-barrier so both goroutines' reads must complete (both observing "in_progress") before either can proceed to its write. Verified deterministic with -race -count=50 (was flaky under real CI scheduling); full server/mcp package remains green.
Contributor
✅ Registry ValidationTest Coverage: 24/181 features have
|
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
…al merge) Branch had drifted again since the prior Gate 5 merge -- main is very active. Only conflict was another unrelated planning doc (project_plans/flaky-hook-url-tests/research/features.md), resolved by taking main's version (rerere auto-applied, verified identical). Feature files (server/mcp/*, github/*, server/services/*, session/backlog.go) merged cleanly with zero conflicts. go build ./... succeeds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UDFVDKhxFquFRroHL7mDCW
Contributor
✅ Registry ValidationTest Coverage: 25/181 features have
|
4 tasks
tstapler
added a commit
that referenced
this pull request
Aug 4, 2026
…d seam (#332) TestReportDuplicate_ReportsDistinctMessage_WhenCASPreconditionFails failed deterministically under GOMAXPROCS=1 (successes=2 instead of 1): reportDuplicate read the backlog item via h.storage.GetBacklogItem directly instead of the overridable h.getBacklogItemFor seam requestReview already uses, so the test had no way to force both racers' pre-transition reads to land before either's write. Under scheduling delay, the loser could observe the winner's already- committed status + VerificationNotes and take the idempotency short-circuit instead of racing the CAS write. This is the identical race shape already found and fixed for request_review in #308 (a4793c1), which added the getBacklogItemFn hook + readBarrier — reportDuplicate was never migrated onto that same seam. Fix: route reportDuplicate through h.getBacklogItemFor and add the matching readBarrier to its test, mirroring TestRequestReview_ReportsDistinctMessage_WhenCASPreconditionFails. Verified: 200 runs at -cpu=1,2,4 -race, previously failing reliably at cpu=1, now pass; server/mcp package clean at -count=10 -race. Claude-Session: https://claude.ai/code/session_01W3683CH7Fs9zYR2yP3Dpba Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
Adds a
report_duplicateMCP tool so a work-role backlog session that discovers its work duplicates an already-shipped PR/issue/commit can self-resolve instead of getting stuck, and generalizesrequest_review's CAS precondition so apr_pendingitem can re-request review. Fixes the exact scenario backlog itemda58b867describes: a session whose own PR (#281) was superseded by an independently-merged PR (#272) had no tool to close out its item —request_reviewonly acceptedin_progress, and no "mark duplicate" tool existed at all.Context
Real incident: while working item
fc63d55b, a session discovered — only after implementing, reviewing, and opening PR #281 — that a parallel session had already shipped the identical fix as PR #272.request_review's CAS precondition was hardcoded toin_progress, so oncereport_pr_createdmoved the item topr_pendingthere was no way back.submit_review_verdictis review-role-only. No self-service resolution path existed.Changes
request_review(generalized): CAS precondition now validates the observed source status against an explicit whitelist (in_progress,pr_pending) through a singlevalidateSelfResolveSourcechokepoint — never a hardcoded constant, and neveritem.Statusechoed back unchecked (that would make the compare-and-swap vacuous). Adds an active-reviewer guard on thepr_pendingpath (fails closed on a storage error).TriggeredByswitches to a newTriggeredByAgentaudit value on both paths. CAS-race losers get a distinct, non-retry message instead of a generic one.report_duplicate(new tool): verifies a PR/issue/commitduplicate_refagainst GitHub before any mutation, then routes the item toreview(never directly todone/archived). Three-channel error classification (definitively-not-found / access-denied / transient-retry / no-credentials-non-retryable). Refuses onSkipReviewGate, wrong role, unlinked session, or disallowed source status — zero mutation on every refusal path, and refusal happens before any GitHub network call (enforced by tests, not just code ordering). Idempotent on exact retry; appends (never overwrites) priorVerificationNotes. Skips spawning a redundant review session when one is already active (a real double-spawn bug found and fixed during planning — see Reviewer Notes).githubpackage: newGetPR/GetCommit(native HTTP, notghCLI, for auth-mechanism consistency with the existingGetIssue), new sentinel errorsErrGitHubRefNotFound/ErrGitHubAccessDenied, and a sharedclassifyGHResponsehelper (extracted after this diff pushed 5 near-identical status-classification blocks past the point where duplication was worth factoring out).server/mcp/tools_backlog_test.go,github/{commits,repos_pr}_test.go) covering both the happy paths and the safety-critical fixes below. All 5 pre-existingTestRequestReview_*tests pass unmodified (zero edits).Impact
server/mcp/tools_backlog.go(new tool + generalized existing tool),github/{repos,commits,http_client}.go(new GitHub calls),server/services/backlog_service_triage.go(one export rename, no behavior change),session/backlog.go(one new constant).request_review'sin_progressbehavior is unchanged for all 5 existing tests (verified, not assumed —item.Statusis alwaysin_progressin those fixtures, so the generalized whitelist-validated precondition computes the identical value the old hardcoded constant did).ListItemSessionsquery onrequest_review'spr_pendingpath only;report_duplicatemakes one GitHub API call per invocation, same shape as the existingreport_pr_created.session/ent/diff — no newBacklogStatus, no schema/migration (per ADR-001;report_duplicate's "duplicate" outcome is represented via the existingreviewstatus + free-textVerificationNotes/BacklogStatusEvent.Notefields).Reviewer Notes
TriggerReviewForSessioncall inreport_duplicate's success path (server/mcp/tools_backlog.go, searchactiveReview) — this exists becauseTriggerReviewForSession → spawnReviewGate → ReviewGateRunner.Runhas no dedup against an already-active review-role session (verified directly againstsession/review_gate.go); calling it unconditionally would spawn a genuine second concurrent reviewer. This was found by an independent pre-mortem pass late in planning, not by the original design — seeproject_plans/backlog-self-resolve/implementation/pre-mortem.mdF1 for the full writeup.report_duplicate's GitHub verification is existence-only (confirms the ref is real, not that it's actually related to this item's work — that judgment is the calling agent's) and github.com-only for v1 (no GitHub Enterprise Server host support, though this codebase has GHES plumbing elsewhere) — both are documented, deliberate v1 scope decisions, not oversights (seepre-mortem.mdF2/F5). Areport_duplicatecall and arequest_reviewcall from the same session can theoretically race onVerificationNotes(last-write-wins, no CAS on that field) — accepted as low-probability given the single-work-session-per-item-at-a-time tool-call model this codebase already assumes elsewhere.tools_backlog_test.gointo a shared helper (noted duringsdd:6-verify, left out of this diff as out of scope); consider whetherFR10's "eventually surfaces" guarantee should get its ownStuckReasonrather than relying onReconcilePRPending's incidental coverage, if this workflow proves common.featureDisabledResultkill switch every backlog MCP tool in this file already checks.Process note
This PR was produced by an unattended SDD (Stapler-Driven Development) backlog session: requirements → parallel research (6 dimensions) → plan with adversarial + architecture review repair cycles → validation (test design + pre-mortem + cross-artifact consistency) → implementation (3 dependency-ordered waves of worker subagents) →
sdd:6-verify(idiom + architecture review, security review, full test suite — verdict PASS, zero blockers). Planning artifacts are committed underproject_plans/backlog-self-resolve/.request_reviewcould not be called before opening this PR because thestapler-squadMCP server was disconnected for the backlog session's remaining duration despite repeated retries — it will be called (andreport_pr_createdsubmitted) as soon as the connection is restored, per the item's own documented fallback ("continue your work using the criteria from/backlog/status... record completed criteria in commit messages").Related
da58b867-bf4e-4720-8fe4-9cfcfa5b6eed