feat(backlog): two-way GitHub issue sync — provenance, forward/backward status sync, loop prevention - #336
Merged
tstapler merged 18 commits intoAug 4, 2026
Conversation
…og-github-two-way-sync - validation.md: 52 test cases (Go unit/integration/migration, Jest, Playwright) covering all 11 acceptance criteria. - pre-mortem.md: 3 P1 failure modes identified and resolved directly in plan.md (watermark clock-skew in CloseIssue's signature, UserModifiedFields presence-check vs value-diff, forward-sync failures wired into the row-level-warning store). - plan.md: fixed a real correctness bug found by cross-artifact consistency review (Labels backward-sync was missing its BackwardSyncEnabled gate, unlike the status blocks in Epic 2.1/2.2), and added Epic 4.4 (PreviewBackwardSyncImpact + confirm dialog) to close a Product Triad Review UX blocker: enabling backward sync could previously bulk-archive already-imported items with no preview or confirmation. Readiness gate: PASS. Triad review: READY TO BUILD (verified via a fresh, independent UX re-check after the blocker fix).
…owed, SyncLoop.workflowEngine Adds the audit-trail marker and read-only guard-evaluation helper the backward sync (GitHub -> backlog) work needs, without creating an import cycle (session cannot import server/services): - TriggeredByGitHubSync = "github_sync" constant alongside TriggeredByUser/TriggeredBySystem (session/backlog.go). - GuardedTransitionAllowed(engine, item, to) evaluates CanTransition + ValidateGates without executing the transition — the read-only counterpart to transitionWithGuard for callers in package session that can't import server/services (session/workflow_engine.go). - SyncLoop gains a workflowEngine field, defaulted to NewDefaultWorkflowEngine() in both constructors so existing NewSyncLoop(...)/NewSyncLoopWithKeyProvider(...) call sites compile unchanged (session/backlog_sync.go). Consumed by later Phase 2/3 work, not by this change. Ref: project_plans/backlog-github-two-way-sync/implementation/plan.md Epic 0.4. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NYt425C3izWQRqKgYLoj74
Add ForwardSyncEnabled, BackwardSyncEnabled, ForwardSyncCloseLabel as first-class ItemSource fields end-to-end (ent schema -> generated code -> repository -> UpdateItemSource RPC handler -> proto), mirroring the existing Enabled field's shape. Proto field numbers verified free against the live .proto before assigning (ItemSource highest was 8, UpdateItemSourceRequest highest was 4 - matches plan.md's projected 9/10/11 and 5/6/7). Also fixes a latent bug found while adding the UpdateItemSource not-found test: EntRepository.UpdateItemSource wraps ent's *ent.NotFoundError as session.ErrNotFound before returning, so the handler's `ent.IsNotFound(err)` check never matched and unknown source IDs fell through to CodeInternal instead of CodeNotFound. Scoped the fix to UpdateItemSource only (per assigned scope). Ref: project_plans/backlog-github-two-way-sync/implementation/plan.md Epic 0.5. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NYt425C3izWQRqKgYLoj74
…bSyncedIssueUpdatedAt watermark (Epics 0.1/0.2/0.6) Lands the ent schema, repository, and GitHub-plugin plumbing that both sync directions depend on: - Epic 0.1: `labels []string` + `external_url string` (backlog-github-issue-link had not landed external_url yet, so this adds it defensively per Task 0.1.1a) added to the BacklogItem ent schema, threaded through BacklogItemData/ BacklogItemUpdate and the ent create/update/read mapping, and populated by GitHubIssuesPlugin.MapToBacklogItem instead of being dropped. - Epic 0.2: GitHubIssuesPlugin.Fetch now queries state=all instead of state=open so closed/reopened issues are observed; ExternalItem gains State and IssueUpdatedAt (parsed from the issue's updated_at, reusing the same value already used to compute the Fetch cursor). - Epic 0.6: GitHubSyncedIssueUpdatedAt *time.Time loop-prevention watermark added to the ent schema/BacklogItemData/BacklogItemUpdate, mirroring PrFeedbackAddressedAt's exact shape (Set.../Clear... pair). Single `go generate ./session/ent` pass covers all new backlog_item fields across the three epics, per plan.md's Phase 0 instruction. Note: DecryptConfigToken (Epic 0.6, Story 0.6.2) was already renamed and committed as an incidental part of an earlier concurrent commit (58ded38) in this shared worktree — no separate change needed here. TestDecryptConfigToken is kept as a thin forwarding wrapper rather than deleted, since server/services/backlog_service_encryption_test.go (outside this task's file-ownership scope) still calls it directly. Ref: project_plans/backlog-github-two-way-sync/implementation/plan.md Epics 0.1, 0.2, 0.6. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NYt425C3izWQRqKgYLoj74
…ic 0.3) Stories 0.3.1/0.3.2 of backlog-github-two-way-sync: export ParseUserModifiedFields/ContainsModifiedField and add MergeUserModifiedFields in package session; thread UserModifiedFields through BacklogItemUpdate and UpdateBacklogItem (repo + ent layers); populate it in the UpdateBacklogItem RPC handler via a value-diff against the existing item (not a presence check), per the pre-mortem P1 #2 correction — the only frontend edit form always resubmits Title verbatim, so a presence-only check would falsely mark it user-modified on nearly every edit. This makes the pre-existing local-wins gate in SyncOne reachable in production for the first time.
Epic 0.1's ent/repository layer already persisted ExternalURL/Labels, but nothing surfaced them through BacklogItem's proto message or the list-view BacklogItemSummary struct — the frontend had no way to read them. Adds external_url (30) and labels (31) to the proto, wires both proto-conversion functions, and adds the fields to BacklogItemSummary (populated directly from the ent entity, no new query). Prerequisite for Epic 4.1/4.2 (card badge, detail Source section).
…k (Phase 2-3) Implements Epics 2.1-2.4 (closed-issue -> archived status mapping per ADR-002, reopened-issue log-only no-op, gated Labels backward sync, ExternalURL/Labels backfill for pre-existing items) plus Phase 3's GitHubSyncedIssueUpdatedAt watermark read-and-skip check in SyncOne (ADR-003), all in the same gated-block style as the existing title/description/priority local-wins blocks. Includes the validation-pass correction from Task 2.3.1a: the Labels backward-sync block gates on source.BackwardSyncEnabled (previously missing from the plan draft), matching the closed/reopened status blocks' existing gate. Adds 15 new tests covering the ADR-002 decision table, the BackwardSyncEnabled/UserModifiedFields gates (including their deliberate asymmetry for ExternalURL vs Labels), and the two AC7 loop-prevention regressions (Risk A: done->done is structurally impossible; Risk B: a manual reopen after forward-sync-close is not re-closed by an exact-echo watermark comparison, while a genuinely newer external change is still processed).
… (Epics 4.1-4.3)
Card badge (Epic 4.1) and detail-view Source section (Epic 4.2) show an
item's GitHub provenance (issue link + labels) when ExternalURL/Labels are
present, per ux.md's icon+identifier+link recommendation. lucide-react
1.14 ships no brand "Github" glyph, so CircleDot substitutes for it.
Settings (Epic 4.3) adds two role="switch" toggles per source ("Close
GitHub issues when I finish here" / "Reflect GitHub status back here"),
a close-label input, a both-directions loop-risk warning, and a
row-level warning for a non-transient (401/403/revoked) sync failure —
sourced from eagerly-fetched sync history so it's visible without
expanding it. The three new setForwardSyncEnabled/setBackwardSyncEnabled/
setForwardSyncCloseLabel hook functions (and the existing
setItemSourceEnabled) now round-trip the full current ItemSource through
UpdateItemSource, since its fields are unconditionally overwritten, not
partial-update.
Backward-sync-enable currently flips directly on click; Epic 4.4's
confirm-with-preview gate (depends on Epic 2.1's
determineBackwardSyncTarget, in flight concurrently) lands in a later
wave — noted in code, not implemented here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NYt425C3izWQRqKgYLoj74
…e 1, Epics 1.1-1.3) Implements AC3: transitioning a backlog item to done closes its linked GitHub issue (merging in a configured close label) and leaves an explanatory bot comment, gated by ItemSource.ForwardSyncEnabled. - Epic 1.1: GitHubIssuesPlugin.CloseIssue/PostIssueComment (session/backlog_plugin_github.go). CloseIssue returns the PATCH response's own updated_at (not wall-clock time), per ADR-003's loop-prevention watermark design (pre-mortem P1 #1). - Epic 1.2/1.3: externalIssueCloser interface + a new EventBus subscriber, StartBacklogGitHubForwardSyncSubscriber (server/services/backlog_github_forward_sync.go), wired in server.go alongside the other Start*Subscriber calls. On CloseIssue failure, records a queryable RecordSourceSyncFailure row instead of only logging (pre-mortem P1 #3; new EntRepository/Storage method, session/ent_repository_backlog.go + storage.go). Plan deviations discovered while implementing: - deps.SyncLoop is always nil (the live periodic SyncLoop is owned internally by session.BacklogController) and *session.SyncLoop has no exported registry accessor, so the subscriber takes the plugin registry and a SyncLoop as separate params, sourced from two new BacklogService accessors (Registry, SyncLoopForForwardSync) added in server/services/backlog_service_sync.go, mirroring TriggerSync's own inline SyncLoop construction — rather than session/backlog_sync.go, which a concurrent worker owns for Phase 2. - EntRepository.TransitionBacklogItemStatus reloads the item via a plain BacklogItem.Get (no .WithSource()), so the EventBus payload's Item can have an empty SourceID even for a source-linked item. handleForwardSyncClose re-fetches via storage.GetBacklogItem (which does eager-load Source) instead of trusting the payload snapshot. Deferred (explicitly non-blocking per plan.md pre-mortem P2 #5): skipping the close+comment when the issue is already known closed — BacklogItemData has no stored external-state field, so this would need a schema change or an extra GitHub call; left as a follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NYt425C3izWQRqKgYLoj74
…dialog (Epic 4.4) Closes Unresolved Question #3 (Product Triad Review UX blocker): turning on backward sync no longer silently bulk-archives already-imported items in the same tick the toggle flips. - New PreviewBackwardSyncImpact RPC (proto/session/v1/backlog.proto): server/services/backlog_sources_preview.go loads the source, decrypts its token, calls the plugin's Fetch once, and reuses determineBackwardSyncTarget (session/backlog_sync.go's new SyncLoop.PreviewBackwardSyncImpact) to count only items in idea/refining/ready/queued whose linked issue is closed. - BacklogSourcesSettings' backward-sync toggle now calls the preview RPC on enable; itemCount 0 flips immediately, itemCount > 0 shows a new BackwardSyncConfirmDialog (informed-consent copy, focus trap, Escape-to- cancel, focus-return) before calling setBackwardSyncEnabled. Toggle shows a pending state during the preview call; a preview failure shows an inline error with no dialog and no toggle flip. - tools/scanner/backend/proto_scanner.go: registered the new RPC's methodToID mapping so registry-generate produces the kebab-case feature id instead of falling back to the raw method name. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NYt425C3izWQRqKgYLoj74
…ition session/backlog_sync.go's closed-issue backward-sync block advanced GitHubSyncedIssueUpdatedAt unconditionally, including when TransitionBacklogItemStatus itself failed — a failed write would permanently mark the item as "already reconciled" and never be retried on a later sync tick. Gate the watermark advance on the transition having actually succeeded (or deliberately skipped), matching this codebase's established retry-on-next-tick convention (see backlog_lifecycle.go's ReconcilePRPending precedent). Also resolves the make lint silenttransition finding on this line — the errored++ counter and next-tick retry are the existing notify mechanisms, per the //nolint justification added.
Per .claude/rules/feature-registry.md — sweep found several stale entries and one missing marker from the preceding waves: - update-item.json/update-source.json: testIds were missing the new UserModifiedFields/sync-direction round-trip tests. - backlog-item-card.json/backlog-item-detail.json: testIds were missing the new provenance-badge/SourceSection tests. - settings-backlog-sources.json: testIds hadn't been touched since before Epic 4.3/4.4 landed (11 new tests added). - SourceSection.tsx had no // +feature: marker despite this repo's own precedent for detail sub-sections (LifecycleSummary.tsx, SessionDiagnosticPanel.tsx) — added the marker + a new registry entry.
MUST FIX: - WCAG AA contrast: provenanceBadge / subHeading / previewPendingLabel rendered textMuted on surfaceMuted, failing 4.5:1 in the dark theme (2.99:1) and clean theme (3.10:1). Switch to textSecondary, and bump clean theme's textSecondary token (4.02:1 -> 4.57:1 against surfaceMuted) since it was itself marginal. - Keyboard nav: BacklogItemCard's onKeyDown fired on any bubbled Enter/Space, so Enter on the nested provenance-badge <a> both preventDefault()'d the anchor's navigation and opened the item detail. Guard on e.target === e.currentTarget. - Stale-closure double-click: handleToggleEnabled/handleToggleForwardSync had no in-flight guard, unlike handleToggleBackwardSync's backwardSyncPreviewPendingId pattern, so a rapid double-click could send the same target value twice. Added matching per-source pending-id guards for both. Cheap follow-ups: - PreviewBackwardSyncImpact was missing TriggerSync's syncFeatureEnabled gate — added it, and switched to SyncLoopForForwardSync() instead of reimplementing its branch inline. - Deleted TestDecryptConfigToken, a single-caller forwarding wrapper; the one caller now calls DecryptConfigToken directly. - closeLabelDrafts never cleared after a successful commit, permanently pinning the input to the locally-typed value. Clear the draft entry once refresh() succeeds. - The closed-issue backward-sync "no valid target" skip branch (item is in_progress/review/pr_pending) left advanceWatermark true even though nothing changed locally, which could permanently suppress a later legitimate auto-archive after a manual status revert. Mirrors the transition-failure branch's existing fix (0fa219f). Added regression tests for the keyboard-nav guard, the double-click guards (both toggles), the close-label reconciliation, and the watermark fix (including an end-to-end two-tick reprocessing test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWAXmtpHQXXZTKmnvLSSWb
CRITICAL: - UpdateItemSource guarded ForwardSyncCloseLabel on non-empty, so a user clearing the close-label input via blur got a 200 response but the label silently reappeared unchanged. Write it unconditionally like the sibling full-state-overwrite fields (Enabled, ForwardSyncEnabled, BackwardSyncEnabled). - PreviewBackwardSyncImpact called plugin.Fetch's single page (50 issues, sorted by created desc), silently missing older closed issues on repos with >50 total issues — could report "0 items affected" when more exist, undermining Epic 4.4's entire purpose. Added GitHubIssuesPlugin.FetchAll (a PaginatedFetcher the preview path type-asserts for) which paginates up to maxPreviewFetchPages (20 pages / 1000 issues), and a possibly_incomplete response field + UI caveat when the cap is hit — went with pagination (approach a) since it stayed contained to Fetch/FetchAll/PreviewBackwardSyncImpact. MAJOR: - Batched PreviewBackwardSyncImpact's N+1 per-issue GetBacklogItemByExternalID loop into one GetBacklogItemsByExternalIDs query. - Added regression tests for previously-untested guard paths: watermark persists when PostIssueComment fails after a successful CloseIssue; the GuardedTransitionAllowed-denied branch in SyncOne's closed-issue block; locally-created items (no SourceID/ExternalID) never trigger CloseIssue. NIT: - Fixed a stale e2e helper comment claiming the Epic 4.4 confirm-with-preview gate was a later wave — it ships in this PR; the fixture just has zero linked items so the dialog auto-skips. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWAXmtpHQXXZTKmnvLSSWb
tstapler
marked this pull request as ready for review
August 4, 2026 17:14
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Implements an opt-in, two-way GitHub Issues ↔ backlog integration with provenance (URL/labels), forward/backward status sync, and loop-prevention via a GitHub-side timestamp watermark.
Changes:
- Persist and surface provenance (
external_url,labels) end-to-end; render on card + detail “Source” section. - Add per-source sync-direction settings (forward close-on-done + optional close label; backward reflect-back) including a preview + confirm gate for first-time backward sync enable.
- Add forward-sync EventBus subscriber, backward-sync logic in
SyncOne, and supporting storage/proto/schema changes (watermark, sync failure recording, pagination preview).
Reviewed changes
Copilot reviewed 69 out of 71 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web-app/src/styles/theme.css.ts | Adjusts clean theme text contrast token to meet WCAG AA on muted surfaces. |
| web-app/src/lib/hooks/useBacklogSourcesService.ts | Adds new per-source sync-direction fields and RPC helpers, plus backward-sync impact preview call. |
| web-app/src/lib/hooks/useBacklogService.ts | Maps new backlog item provenance fields from proto (externalId, externalUrl, labels). |
| web-app/src/components/settings/BackwardSyncConfirmDialog.tsx | New accessible confirm dialog with focus trap/restore for backward sync enable preview. |
| web-app/src/components/settings/BackwardSyncConfirmDialog.css.ts | Styles for the new confirm dialog. |
| web-app/src/components/settings/BacklogSourcesSettings.tsx | Adds sync-direction toggles, preview/confirm flow, close-label input, and row-level warnings. |
| web-app/src/components/settings/BacklogSourcesSettings.css.ts | Adds styles for toggle pending state, sync-direction panel, warnings, and inline preview error. |
| web-app/src/components/backlog/detail/SourceSection.tsx | New backlog item detail “Source” section showing external link + labels. |
| web-app/src/components/backlog/detail/SourceSection.test.tsx | RTL tests for SourceSection rendering and collapse behavior. |
| web-app/src/components/backlog/detail/SourceSection.css.ts | Styles for SourceSection and label badges. |
| web-app/src/components/backlog/BacklogItemDetail.tsx | Wires SourceSection into detail view with expansion state. |
| web-app/src/components/backlog/BacklogItemDetail.test.tsx | Tests for conditional rendering of Source section. |
| web-app/src/components/backlog/BacklogItemCard.tsx | Adds provenance badge link + keyboard handling fix for nested focusables. |
| web-app/src/components/backlog/BacklogItemCard.test.tsx | Adds tests for provenance badge rendering and event propagation/keyboard behavior. |
| web-app/src/components/backlog/BacklogItemCard.css.ts | Styles for provenance badge with WCAG-conscious tokens. |
| tools/scanner/backend/proto_scanner.go | Registers new RPC method ID for preview backward sync impact. |
| tests/e2e/pages/BacklogSourcesSettingsPage.ts | Adds page-object helpers for forward/backward sync toggles. |
| tests/e2e/backlog-github-sync-settings.spec.ts | Adds Playwright coverage for sync-direction settings persistence and loop-risk warning. |
| session/workflow_engine_test.go | Adds unit test coverage for new GuardedTransitionAllowed helper. |
| session/workflow_engine.go | Adds GuardedTransitionAllowed to evaluate transitions without executing them. |
| session/storage.go | Adds GetItemSourceByID and RecordSourceSyncFailure helpers for forward sync and UI warnings. |
| session/repository.go | Extends domain models for provenance, watermark, user-modified fields, and sync-direction settings. |
| session/ent_repository_backlog_test.go | Adds tests for labels NULL-safety and GitHub watermark round-tripping. |
| session/ent_repository_backlog.go | Persists new fields and adds batched lookup for preview (avoids N+1). |
| session/ent/schema/item_source.go | Adds per-source sync-direction fields to schema. |
| session/ent/schema/backlog_item.go | Adds provenance, labels, and watermark fields to schema. |
| session/ent/runtime.go | Updates generated runtime defaults/indexes for new schema fields. |
| session/ent/migrate/schema.go | Updates migration schema to include new columns and indices. |
| session/ent/itemsource_update.go | Generated setters/sql save wiring for new ItemSource fields. |
| session/ent/itemsource_create.go | Generated defaults/check/spec wiring for new ItemSource fields. |
| session/ent/itemsource/where.go | Generated predicates for new ItemSource fields. |
| session/ent/itemsource/itemsource.go | Generated column constants/orderers/defaults for new ItemSource fields. |
| session/ent/itemsource.go | Generated scan/assign/string wiring for new ItemSource fields. |
| session/ent/backlogitem_update.go | Generated setters/sql save wiring for new BacklogItem fields (labels, external_url, watermark). |
| session/ent/backlogitem_create.go | Generated create spec/upsert wiring for new BacklogItem fields. |
| session/ent/backlogitem/where.go | Generated predicates for new BacklogItem fields. |
| session/ent/backlogitem/backlogitem.go | Generated column constants/orderers for new BacklogItem fields. |
| session/ent/backlogitem.go | Generated scan/assign/string wiring for provenance/labels/watermark. |
| session/backlog_sync.go | Adds backward sync impact preview, watermark handling, label/url sync, and exports token decrypt helper. |
| session/backlog_plugin_github_test.go | Adds extensive tests for state=all fetch, pagination, close-issue semantics, comments, and watermark timestamps. |
| session/backlog_plugin_github.go | Adds closed/open state parsing, FetchAll pagination, CloseIssue + PostIssueComment, labels/url mapping. |
| session/backlog_plugin.go | Extends ExternalItem with state/timestamp and introduces PaginatedFetcher capability. |
| session/backlog_lifecycle_test.go | Adds integration round-trip test for labels + external URL through storage. |
| session/backlog.go | Adds TriggeredByGitHubSync for status event provenance. |
| server/services/backlog_sources_preview_test.go | Adds test coverage for PreviewBackwardSyncImpact behavior and error handling. |
| server/services/backlog_sources_preview.go | Implements PreviewBackwardSyncImpact RPC (read-only, gated, timeout-bounded). |
| server/services/backlog_service_test.go | Adds tests for UserModifiedFields diffing and sync-direction settings round-trips/clears/not-found. |
| server/services/backlog_service_sync.go | Exposes plugin registry + sync loop helper for forward-sync subscriber wiring. |
| server/services/backlog_service_lifecycle.go | Implements value-diff user-modified fields merge; updates UpdateItemSource error mapping and new fields. |
| server/services/backlog_service_encryption_test.go | Updates test to use exported DecryptConfigToken. |
| server/services/backlog_service.go | Maps provenance fields to proto and maps new ItemSource fields to proto. |
| server/services/backlog_github_forward_sync_test.go | Adds integration tests for forward sync close/comment/watermark/failure persistence/no-op guards. |
| server/services/backlog_github_forward_sync.go | Adds EventBus subscriber to close issues on done transitions with loop-prevention watermark. |
| server/server.go | Wires forward-sync subscriber at server startup. |
| proto/session/v1/backlog.proto | Adds new fields/RPCs for provenance, sync-direction settings, and backward sync impact preview. |
| project_plans/backlog-github-two-way-sync/implementation/validation.md | Documents validation plan and requirement→test mapping for the feature. |
| project_plans/backlog-github-two-way-sync/implementation/pre-mortem.md | Documents pre-mortem risks and mitigations; reflects fixes included in PR. |
| docs/registry/features/frontend/ui/settings-backlog-sources.json | Updates frontend feature registry with new tests and lastModified. |
| docs/registry/features/frontend/ui/backlog-item-detail.json | Updates feature registry to include Source section tests. |
| docs/registry/features/frontend/ui/backlog-item-detail-source-section.json | Registers the new SourceSection component and tests. |
| docs/registry/features/frontend/ui/backlog-item-card.json | Updates feature registry to include provenance badge tests. |
| docs/registry/features/backend/backlog/update-source.json | Marks update-source backend feature as tested with new test IDs. |
| docs/registry/features/backend/backlog/update-item.json | Adds test IDs for UserModifiedFields diffing to backend registry. |
| docs/registry/features/backend/backlog/preview-backward-sync-impact.json | Registers and marks new preview RPC as tested. |
Files not reviewed (1)
- gen/proto/go/session/v1/sessionv1connect/backlog.connect.go: Generated file
Suppressed comments (7)
session/backlog_sync.go:1
updated/skippedcounters can double-count the same item: a backward-sync status transition can incrementupdated++, butanyFieldremains false so the code later also incrementsskipped++. This will make the recorded SourceSyncEvent aggregates misleading. Consider tracking ananyChange(orstatusChanged/watermarkChanged) flag that’s set when a status transition or watermark write happens, and only incrementskippedwhen no change of any kind occurred.
package session
session/backlog_sync.go:480
updated/skippedcounters can double-count the same item: a backward-sync status transition can incrementupdated++, butanyFieldremains false so the code later also incrementsskipped++. This will make the recorded SourceSyncEvent aggregates misleading. Consider tracking ananyChange(orstatusChanged/watermarkChanged) flag that’s set when a status transition or watermark write happens, and only incrementskippedwhen no change of any kind occurred.
if !anyField {
skipped++
continue
}
web-app/src/components/backlog/detail/SourceSection.tsx:1
externalIdis optional but is rendered unconditionally (Issue #{externalId}), which can produce user-visibleIssue #undefinedwhenexternalUrlis present butexternalIdis missing. Either makeexternalIdrequired whenever this component is rendered, or render a fallback (e.g., omit the number / derive it from the URL) whenexternalIdis not provided.
web-app/src/components/backlog/detail/SourceSection.tsx:1externalIdis optional but is rendered unconditionally (Issue #{externalId}), which can produce user-visibleIssue #undefinedwhenexternalUrlis present butexternalIdis missing. Either makeexternalIdrequired whenever this component is rendered, or render a fallback (e.g., omit the number / derive it from the URL) whenexternalIdis not provided.
web-app/src/components/backlog/BacklogItemCard.tsx:1- The provenance badge is guarded only by
item.externalUrl, but it renders and announcesitem.externalId. IfexternalIdis absent, the badge will show/announce#undefined. Consider guarding on bothexternalUrl && externalId(or computing a safe label) so the link text/aria-label never contains an undefined identifier.
web-app/src/components/settings/BacklogSourcesSettings.tsx:1 isAuthFailuretreats any403as an auth failure, but GitHub rate limiting is commonly surfaced as HTTP 403 (and your own code produces errors containingrate limited+ status 403). This will incorrectly show a persistent “check credentials” warning for transient rate-limit failures. Consider removing the generic403check and instead matching more specific auth signals (e.g.401, 'bad credentials', 'revoked', 'forbidden: requires authentication') while explicitly excluding 'rate limited'.
web-app/src/components/backlog/detail/SourceSection.test.tsx:1- This test name doesn’t follow the more descriptive naming convention used by the other new tests in this file (and elsewhere in the PR). Consider renaming it to match the established pattern (e.g.
SourceSection_should_BeCollapsed_When_DefaultExpandedFalse) so failures are easier to interpret in CI output.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…-backlog-github-two-way-sync # Conflicts: # gen/proto/go/session/v1/backlog.pb.go # session/backlog.go # web-app/src/gen/session/v1/backlog_pb.ts
Contributor
✅ Registry ValidationTest Coverage: 27/182 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
Contributor
UX Analysis
|
Contributor
Frontend Terminal Throughput |
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. |
- session/backlog_sync.go: guard both closed/reopened-issue backward-sync blocks against a zero (unparsed) IssueUpdatedAt, which would otherwise either false-short-circuit as already-reconciled against a real watermark or persist a garbage zero watermark. - session/backlog_sync.go: track a per-item anyChange flag so a status transition/watermark write in the closed/reopened blocks isn't also double-counted by the generic `!anyField` skipped++ fallback, restoring the SourceSyncEvent aggregate's partition-of-item-count invariant. - server/services/backlog_service.go: only set the optional ExternalUrl proto field when ExternalURL is non-empty, in both backlogItemToProto and backlogItemSummaryToProto, instead of always setting a non-nil pointer to an empty string. - web-app SourceSection.tsx / BacklogItemCard.tsx: guard the "Issue #<id>" rendering so a present externalUrl with a missing externalId can't render a literal "Issue #undefined". - web-app BacklogSourcesSettings.tsx: isAuthFailure no longer treats every 403 uniformly — GitHub's rate-limit response is also a 403, so rate-limited messages are now explicitly excluded before matching on 401/403/bad credentials/revoked/requires authentication. - Test naming nit: renamed a SourceSection test to the file's established Subject_should_ExpectedBehavior_When_Condition convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HWAXmtpHQXXZTKmnvLSSWb
Contributor
✅ Registry ValidationTest Coverage: 27/182 features have
|
…-backlog-github-two-way-sync # Conflicts: # session/backlog_plugin_github.go
Contributor
✅ Registry ValidationTest Coverage: 27/182 features have
|
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
The GitHub Issues backlog source was pull-only and lossy:
Fetchonly queriedopen issues,
MapToBacklogItemdropped the issue's URL/labels on the floor,and
SyncOnenever wrote status back in either direction — so a backlog itemcould sit around long after its source issue closed, and shipping a backlog
item never closed the linked issue. This PR builds a real two-way
integration, gated entirely behind opt-in per-source settings.
Closes backlog item
be676dab-6798-410e-a3ec-708fc03758e0.What Changed
Fetchnow queriesstate=all(observes closed/reopenedissues);
ExternalURL/Labelspersist end-to-end (ent → repository →proto) and render as a card badge + detail "Source" section.
donetransition closes thelinked GitHub issue via a new
EventBussubscriber, optionally applies aconfigured label, and posts one explanatory bot comment.
SyncOneapplies the issue'sclosed/open state and label changes onto the backlog item, respecting the
existing
UserModifiedFieldslocal-wins guard — closed issues in pre-workstatuses (
idea/refining/ready/queued) auto-archive; issues closedwhile an item is mid-flight are left alone (see ADR-002).
GitHubSyncedIssueUpdatedAtwatermark (GitHub's owntimestamp, not local wall-clock — see ADR-003) prevents forward/backward
sync from re-processing each other's writes, including on a failed
transition (retried next tick, not silently dropped).
source runs a
PreviewBackwardSyncImpactcheck and shows a confirm dialogwith the exact item count before anything changes — added after triad
review flagged silent bulk-archiving as a launch blocker.
Settings > Backlog Sources, with a persistent row-level warning for
non-transient sync failures (auth/401, from either the read or write side).
UpdateBacklogItem's RPC handler nowactually populates
UserModifiedFields(via value-diff, not a naivepresence check) — previously the local-wins guard existed in code but was
unreachable in production for any item, GitHub-linked or not.
Test plan
go build ./...— cleango test ./session/... ./server/...— all pass, 0 failuresgo test -racescoped to every touched package (forward sync, backwardsync, plugin, preview RPC, UserModifiedFields) — clean, no races
make lint— 0 issuesmake build— full production build succeedscd web-app && npx tsc --noEmit— cleancd web-app && npx jest --testPathPatterns="Backlog"— 3720/3727passing (2 unrelated pre-existing failures confirmed via empty diff
against the pre-feature commit)
inbound GitHub data) — no HIGH/MEDIUM findings
sdd:6-verify(idiom + architecture + correctness gates) — PASS afterone fix-loop iteration resolving 3 real bugs found by review (WCAG AA
contrast failure, a keyboard-nav bug, a stale-closure double-click bug)
a dedicated spec-compliance sweep