feat: CI/CD status in diff viewer - #326
Closed
tstapler wants to merge 2 commits into
Closed
Conversation
Show GitHub Actions CI status alongside the per-session diff viewer, block manual approval on failing CI (with an audited override), and let the auto-approve rule engine require green CI. ## What Changed - CIStatusBadge in the diff-viewer header: 4 states (passing/failing/ pending/no-checks), links to the PR's Checks page, renders nothing when the session has no PR. Purely presentational over the already-delivered Session proto — no new fetch code. - Fixed PRStatusPoller.applyPRUpdate to publish a WatchSessions update when only the CI conclusion changes (previously gated on priority changes only), so the badge can't go stale between poll ticks. - ApprovalService.ResolveApproval blocks manual Approve when review:block-approval-on-ci-failure (default off) is on and the session's branch has failing CI, with a visible inline explanation in NotificationPanel (not a silent no-op). An audited OverrideCiBlock lets a reviewer bypass the block per-approval with a distinct log line. - pkg/classifier's Rule.RequireCIPassing ANDs with existing conditions (regex, tool name, etc.) via a new ClassificationContext.CIStatus field, persisted through RuleSpec, an ent schema column, and a new proto field. Exposed via a rule-builder UI checkbox. - CI status (GitHubCheckConclusion/GitHubPRNumber/LastPRStatusCheck) is not persisted — it only lives on the poller's in-memory Instance — so both gates read it through the existing LiveInstanceFinder interface (reused from WorkspaceService) rather than *session.Storage. Reads go through Instance.Snapshot(), not raw struct fields, since PRStatusPoller mutates those fields on its own goroutine. - A bounded-staleness guard (2x the poller's constructor-injected PollInterval) treats CIStatus as unknown for the auto-approve path; the manual-approve path's staleness gap is a documented P2 follow-up (see project_plans/ci-status-diff-viewer/implementation/pre-mortem.md Failure #2). - Feature registry entries for the new RPC field coverage and the CIStatusBadge UI feature. ## Test plan - [x] go build ./... clean - [x] go test ./pkg/classifier/... ./session/... ./server/services/... — all pass - [x] npx tsc --noEmit — clean - [x] npx jest CIStatusBadge — 12/12 passed - [x] npx playwright test ci-status-badge.spec.ts rule-builder-ci-passing.spec.ts — 10/10 passed (chromium + chromium-dom), run against an isolated test server built from this exact branch - [x] make registry-diff — 0.0% divergence ## Known gap approval-ci-block.spec.ts (frontend e2e for the block/override UI) was not delivered — the notification-panel toggle button this spec would need does not render in the isolated test server's DOM in any configuration tried; no existing spec in this repo's e2e suite exercises that surface either. The underlying block/override behavior is covered by 7 Go integration tests in approval_service_test.go. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qutTQcLJtXZhFiMCaD9Dm
Contributor
✅ Registry ValidationTest Coverage: 27/181 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
Contributor
Frontend Terminal Throughput |
Contributor
UX Analysis
|
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
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. |
…entry make registry-generate (via encoding/json) escapes > as >; my hand- authored testIds used a literal >, which made `git diff --exit-code docs/registry/features/` fail in CI after a real make registry-generate run. Regenerated the file with the actual tool instead of hand-editing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qutTQcLJtXZhFiMCaD9Dm
Contributor
✅ Registry ValidationTest Coverage: 27/181 features have
|
tstapler
marked this pull request as ready for review
August 4, 2026 07:10
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds GitHub Actions CI status visibility and enforcement across the diff viewer and approval/rule flows, including a manual-approval CI-failure block (with audited override) and an auto-approve rule condition that requires green CI.
Changes:
- Render a session-scoped CI status badge in the diff viewer header (with unit + e2e coverage).
- Add
require_ci_passingto approval-rule persistence (proto/ent/JSON/UI) and classifier evaluation context. - Enforce (flagged) manual-approval blocking on failing CI via live-instance CI state, plus poller/event updates to prevent staleness.
Reviewed changes
Copilot reviewed 40 out of 50 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web-app/src/lib/ruleTemplates.ts | Extends rule template shape with requireCiPassing. |
| web-app/src/lib/hooks/useApprovalRules.ts | Threads requireCiPassing through rule hydration/mapping. |
| web-app/src/gen/session/v1/types_pb.ts | Regenerates TS proto types to include require_ci_passing. |
| web-app/src/components/ui/NotificationPanel.tsx | Adds CI-blocked inline UI + override path for ResolveApproval. |
| web-app/src/components/ui/NotificationPanel.css.ts | Styles for CI-blocked row/text. |
| web-app/src/components/sessions/tests/CIStatusBadge.test.tsx | Unit tests for CI status badge rendering/linking/staleness tooltip. |
| web-app/src/components/sessions/SessionDetailView.tsx | Passes session into DiffViewer for badge rendering. |
| web-app/src/components/sessions/DiffViewer.tsx | Renders CIStatusBadge in diff header when session has a PR. |
| web-app/src/components/sessions/DiffViewer.css.ts | Adds header layout styling for diff viewer. |
| web-app/src/components/sessions/CIStatusBadge.tsx | Implements presentational CI status badge with link to PR checks. |
| web-app/src/components/rules/RuleBuilderForm.tsx | Adds “Require CI passing” checkbox and includes it in payload. |
| web-app/src/app/settings/features/page.tsx | Adds feature-flag label for CI-failure approval block. |
| tests/e2e/rule-builder-ci-passing.spec.ts | E2E coverage for rule-builder checkbox + persistence via RPC. |
| tests/e2e/pages/SessionDetailPage.ts | Adds page object accessor for CI status badge. |
| tests/e2e/ci-status-badge.spec.ts | E2E coverage for diff-viewer CI badge (via response injection). |
| session/repository.go | Adds RequireCIPassing to rule data model. |
| session/pr_status_poller_test.go | Regression test for conclusion-only updates firing onUpdated. |
| session/pr_status_poller.go | Emits updates/logs on CI conclusion changes; adds PollInterval accessor. |
| session/instance_terminal.go | Tracks conclusion-change flag in PR status update result. |
| session/ent_repository.go | Persists RequireCIPassing via ent repository mapping. |
| session/ent/schema/approvalrule.go | Adds require_ci_passing bool column (default false). |
| server/services/session_service.go | Wires live instance finder into ApprovalService. |
| server/services/rules_store.go | Adds require_ci_passing to RuleSpec and classifier rule creation. |
| server/services/rules_service.go | Threads require_ci_passing between proto/spec/classifier. |
| server/services/feature_flag_service.go | Registers CI-failure approval-block flag and constant. |
| server/services/approval_service_test.go | Adds integration tests for CI-failure block + override behavior. |
| server/services/approval_service.go | Implements CI-failure block with override (using live instance snapshot). |
| server/services/approval_handler_integration_test.go | Tests CI status staleness guard and CIStatus context propagation. |
| server/services/approval_handler.go | Adds CIStatus to classifier context with staleness bounding. |
| server/server.go | Wires poll interval + live finder into ApprovalHandler. |
| server/dependencies.go | Expands WatchSessions update fields to include CI conclusion. |
| proto/session/v1/types.proto | Adds require_ci_passing to ApprovalRuleProto. |
| proto/session/v1/session.proto | Adds override_ci_block to ResolveApprovalRequest. |
| project_plans/ci-status-diff-viewer/implementation/plan.md | Documents discovered persistence assumptions + wiring deviations. |
| pkg/classifier/classifier_test.go | Adds classifier tests for RequireCIPassing behavior and AND semantics. |
| pkg/classifier/classifier.go | Adds CIStatus context + RequireCIPassing rule condition. |
| docs/registry/features/frontend/ci-status-badge.json | Feature registry entry for CI badge + tests. |
| docs/registry/features/backend/approval/upsert-rule.json | Marks UpsertApprovalRule as tested w/ new e2e tests. |
| docs/registry/features/backend/approval/resolve.json | Marks ResolveApproval as tested w/ new CI-block tests. |
Files not reviewed (10)
- gen/proto/go/session/v1/session.pb.go: Generated file
- gen/proto/go/session/v1/types.pb.go: Generated file
- session/ent/approvalrule.go: Generated file
- session/ent/approvalrule/approvalrule.go: Generated file
- session/ent/approvalrule/where.go: Generated file
- session/ent/approvalrule_create.go: Generated file
- session/ent/approvalrule_update.go: Generated file
- session/ent/migrate/schema.go: Generated file
- session/ent/mutation.go: Generated file
- session/ent/runtime.go: Generated file
Suppressed comments (3)
web-app/src/components/sessions/CIStatusBadge.tsx:1
- Rendering the badge as an
<a>withrole=\"status\"is semantically conflicting (an interactive element presented as a live-region status), and whenhrefis undefined it stops being keyboard-focusable while still being a ‘status’. A more accessible approach is to render a non-interactive status element (e.g., a<span role=\"status\">…</span>) when there is no PR URL, and render a normal link (default link semantics, norole=\"status\") whenhrefis present; if you keep a single element, also avoidtarget/relwhenhrefis absent.
web-app/src/components/sessions/tests/CIStatusBadge.test.tsx:1 - This test mutates
global.fetchbut doesn’t restore it in atry/finally. If an assertion throws before line 102, subsequent tests can be affected by the mocked fetch; wrap the render/assert intryand restoreglobal.fetchinfinally.
tests/e2e/ci-status-badge.spec.ts:1 - This loop calls
mockCIStatus(page, …)repeatedly, which registers newpage.route()handlers each time. Playwright routes are additive unless explicitly removed, so handlers can accumulate across iterations and make the spec flaky. Consider registering a single route once (using a mutable map keyed by sessionId), or callpage.unroute()before re-registering routes in each iteration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| warren.SetAlways(w2, "PRStatusPoller.Instances", svc.PRStatusPoller.SetInstances, instances) | ||
| warren.SetAlways(w2, "PRStatusPoller.OnUpdated", svc.PRStatusPoller.SetOnUpdated, func(inst *session.Instance) { | ||
| eventBus.Publish(events.NewSessionUpdatedEvent(inst, []string{"github_pr_priority", "github_pr_state"})) | ||
| eventBus.Publish(events.NewSessionUpdatedEvent(inst, []string{"github_pr_priority", "github_pr_state", "github_check_conclusion"})) |
This was referenced Aug 6, 2026
Closed
Owner
Author
|
Superseded — item 3065ecfb (CI/CD status in diff viewer) shipped via PR #353 (merged 2026-08-06). Closing this earlier duplicate attempt. |
tstapler
added a commit
that referenced
this pull request
Aug 6, 2026
… an audited override (#351) * fix(session): guard reconciliation automation against override-linked PR mismatches Story 5/6 of report-pr-created-branch-mismatch: adds a prByNumberFinder seam and verifyPRHeadBranchMatchesTracked guard that re-verifies, via a live GitHub lookup, that a backlog item's PR still has the head branch this item is tracking before three automated reconciliation call sites (closeIfSupersededByMain's auto-close, ReconcilePRPending's merge-detected done transition, reconcileBouncingItems' IsPRMerged-driven done transition) treat item.PrNumber as ground truth. Also prepends an "unverified association" disclaimer to spawned fix-session context when the guard can't confirm a match (Task 6.3a), and documents reconcileOrphanedAgentPRs' identical pre-existing blind spot (Task 5.1, no functional change). Fixing 7 pre-existing tests that exercised the now-guarded mutation paths without any tracked-branch/worktree fixture required adding real work sessions (via SaveInstances) and a matching prByNumberFinder stub so the guard verifies true, preserving their original assertions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpCssZgc5WeLQ5B45q7XV5 * fix(mcp): allow report_pr_created to accept a self-authored PR on a fallback branch Rewrite VerifyPRMatchesBranch to look PRs up by number (GetPRByNumber) instead of by head branch, returning a PRVerification value (via the invariant-enforcing NewPRVerification constructor) instead of a bare bool so reportPRCreated can distinguish "no such PR" from "PR exists but head branch differs." Add decideOverridePolicy, a pure decision function gating a new override_reason argument: a mismatched branch is now accepted only when the caller supplies a reason and the PR was authored by this server's own resolved GitHub identity (existence still can never be overridden). Accepted overrides are audited via a structured log.Warn line. Covers Stories 2-4 of project_plans/report-pr-created-branch-mismatch's implementation plan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpCssZgc5WeLQ5B45q7XV5 * fix(github): add GetPRByNumber for root-cause PR lookup by number Adds PRStateOpen/PRStateClosed/PRStateMerged constants and GetPRByNumber, which looks a PR up by its immutable number via the REST single-PR endpoint instead of by head branch name, closing the root cause of PR lookups matching the wrong PR when a branch is reused or renamed. Validates base.repo.full_name against the requested owner/repo before returning success, and parses the REST response's user.login (not author.login, a different endpoint shape) into PRInfo.Author. Covers Story 1 / Task 1.1-1.3 of the report-pr-created-branch-mismatch implementation plan: unit tests against httptest fixtures, plus a live_github-tagged manual test against real, public PR #326. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpCssZgc5WeLQ5B45q7XV5 * fix(github): don't pin live test to PR #326's mutable merged state The live-github test asserted State == PRStateMerged, but PR #326 is currently open on real GitHub, not merged, contradicting the bug report's "shipped and merged cleanly" framing. HeadRef/Author/BaseRef all parsed correctly against the real payload, proving the fix's parsing logic is sound — only the hardcoded lifecycle-state assertion was wrong, since an external, mutable PR's current state isn't what this test exists to pin. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpCssZgc5WeLQ5B45q7XV5 * chore(sdd): implementation plan for report-pr-created-branch-mismatch Plan resolves the architecture.md-vs-pitfalls.md tension by rejecting both the ancestry/compare-API fallback (mathematically can't distinguish a history-severed recovery branch from an unrelated PR) and a separate operator-override tool (no human/operator auth primitive exists in this codebase), in favor of a root-cause fix (number-keyed GitHub PR lookup) plus a same-tool relaxed check gated by a mandatory, audited override_reason argument. Includes ADR-001 documenting that override_reason is a role-gated + audit-trail control, not a human-gated one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpCssZgc5WeLQ5B45q7XV5 * chore(sdd): verify/review artifacts for report-pr-created-branch-mismatch Adds pre-mortem, architecture review, adversarial review, and validation docs, and updates ADR-001 and plan.md with fourth plan-repair pass fixes, produced during sdd:4-validate/6-verify passes on this bugfix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(mcp,session): resolve fallout from rebasing onto post-#346 main Cherry-picking this fix's commits onto origin/main (cut clean of this branch's unrelated worktree-shared history, per this same bug's own documented recovery) surfaced three silent (non-conflicting) merge issues go vet/test caught but git's line-based merge did not: - server/mcp/tools_github_test.go and tools_backlog_test.go each independently declared resetGhBaseURL; drop the duplicate. - Two pre-existing tools_backlog_test.go tests still constructed verifyPRMatchesBranch mocks against the pre-fix (bool, error) signature; update to PRVerification. - PR #346 (merged same day, unrelated to this item) added TestReconcilePRPending_should_StillCloseSupersededPR_When_SessionsRealTipIsOnMain, which this fix's new closeIfSupersededByMain branch-verification guard (Story 6) now fails closed against since its fixture never stubs a PRByNumberFinder. Stub one via the existing stubMatchingPRByNumberFinder helper so the guard confirms the match instead. go build ./... && go test ./session/... ./server/mcp/... ./github/... all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- 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
Show GitHub Actions CI status alongside the per-session diff viewer, block manual approval on failing CI (with an audited override), and let the auto-approve rule engine require green CI. Closes the backlog item migrated from issue #37.
What Changed
Sessionproto — no new fetch code.PRStatusPoller.applyPRUpdateto publish aWatchSessionsupdate when only the CI conclusion changes (previously gated on priority changes only), so the badge can't go stale between poll ticks.ApprovalService.ResolveApprovalblocks manual Approve whenreview:block-approval-on-ci-failure(default off) is on and the session's branch has failing CI, with a visible inline explanation inNotificationPanel(not a silent no-op). An auditedOverrideCiBlocklets a reviewer bypass the block per-approval with a distinct log line.pkg/classifier'sRule.RequireCIPassingANDs with existing conditions (regex, tool name, etc.) via a newClassificationContext.CIStatusfield, persisted throughRuleSpec, an ent schema column, and a new proto field. Exposed via a rule-builder UI checkbox.GitHubCheckConclusion/GitHubPRNumber/LastPRStatusCheck) is not persisted — it only lives on the poller's in-memoryInstance— so both gates read it through the existingLiveInstanceFinderinterface (reused fromWorkspaceService) rather than*session.Storage. Reads go throughInstance.Snapshot(), not raw struct fields, sincePRStatusPollermutates those fields on its own goroutine (a real race caught by this PR's own architecture review — fixed and verified withgo test -race).PollInterval) treatsCIStatusas unknown for the auto-approve path; the manual-approve path's staleness gap is a documented P2 follow-up (project_plans/ci-status-diff-viewer/implementation/pre-mortem.mdFailure chore: remove CLA and rename Claude Squad to Stapler Squad #2).CIStatusBadgeUI feature.Known Gap
approval-ci-block.spec.ts(a frontend e2e spec for the block/override UI, named in the original acceptance criteria) was not delivered — the notification-panel toggle button it would need to interact with does not render in the isolated e2e test server's DOM in any configuration tried, and no existing spec in this repo's ~100-file e2e suite exercises that surface either. The underlying block/override behavior is still covered by 7 real Go integration tests inapproval_service_test.go.Test Plan
go build ./...— cleango test ./pkg/classifier/... ./session/... ./server/services/...— all passgo test -race ./server/services/... -run "TestResolveApproval|TestHandlePermissionRequest"— clean (confirms the Snapshot() race fix)npx tsc --noEmit— cleannpx jest CIStatusBadge— 12/12 passednpx playwright test ci-status-badge.spec.ts rule-builder-ci-passing.spec.ts— 10/10 passed (chromium + chromium-dom), run against an isolated test server built from this exact branchmake registry-diff— 0.0% divergence🤖 Generated with Claude Code
https://claude.ai/code/session_014qutTQcLJtXZhFiMCaD9Dm