diff --git a/apps/backend/internal/github/service_comparison_target_sync_test.go b/apps/backend/internal/github/service_comparison_target_sync_test.go new file mode 100644 index 0000000000..20c41e9496 --- /dev/null +++ b/apps/backend/internal/github/service_comparison_target_sync_test.go @@ -0,0 +1,87 @@ +package github + +import ( + "context" + "testing" + + taskmodels "github.com/kandev/kandev/internal/task/models" +) + +type syncComparisonTargetObserver struct { + store *Store + calls int + taskID string + candidate taskmodels.ComparisonTargetCandidate + persistedTitle string +} + +func (o *syncComparisonTargetObserver) ReconcileComparisonTarget( + context.Context, string, taskmodels.ComparisonTargetCandidate, +) (*taskmodels.ComparisonTargetReconciliation, error) { + return nil, nil +} + +func (o *syncComparisonTargetObserver) ReconcileComparisonTargetFromSync( + ctx context.Context, taskID string, candidate taskmodels.ComparisonTargetCandidate, +) (*taskmodels.ComparisonTargetReconciliation, error) { + persisted, err := o.store.GetTaskPR(ctx, taskID) + if err != nil { + return nil, err + } + o.calls++ + o.taskID = taskID + o.candidate = candidate + o.persistedTitle = persisted.PRTitle + return &taskmodels.ComparisonTargetReconciliation{ + Status: taskmodels.ComparisonTargetNoMatch, + }, nil +} + +func (o *syncComparisonTargetObserver) RemoveComparisonTargetForChange( + context.Context, string, string, string, string, int, +) error { + return nil +} + +func TestSyncTaskPR_PersistsBeforeComparisonTargetReconciliation(t *testing.T) { + svc, store, _ := setupSyncTest(t) + ctx := context.Background() + createOutcomeSyncTestTaskPR(t, store, &TaskPR{ + TaskID: "t1", Owner: "owner", Repo: "repo", PRNumber: 1, + PRURL: "https://github.com/owner/repo/pull/1", PRTitle: "Initial", + HeadBranch: "feat", BaseBranch: "main", State: "open", + }) + + observer := &syncComparisonTargetObserver{store: store} + svc.SetComparisonTargetObserver(observer) + status := &PRStatus{PR: &PR{ + Number: 1, + Title: "Updated title", + State: "open", + RepoOwner: "owner", + RepoName: "repo", + HeadRepoOwner: "contributor", + HeadRepoName: "repo", + BaseRepoOwner: "upstream", + BaseRepoName: "repo", + HeadBranch: "feat", + BaseBranch: "main", + }} + + if err := svc.SyncTaskPR(ctx, "t1", status); err != nil { + t.Fatalf("SyncTaskPR: %v", err) + } + + if observer.calls != 1 { + t.Fatalf("comparison reconciliation calls = %d, want 1", observer.calls) + } + if observer.taskID != "t1" { + t.Fatalf("comparison reconciliation task ID = %q, want t1", observer.taskID) + } + if observer.persistedTitle != "Updated title" { + t.Fatalf("persisted title during reconciliation = %q, want Updated title", observer.persistedTitle) + } + if observer.candidate.Number != 1 || observer.candidate.HeadRepository.Path != "contributor/repo" { + t.Fatalf("comparison candidate = %#v, want PR 1 from contributor/repo", observer.candidate) + } +} diff --git a/apps/backend/internal/github/service_pr_outcome_sync_test.go b/apps/backend/internal/github/service_pr_outcome_sync_test.go index f9be96c199..3ecfc89d61 100644 --- a/apps/backend/internal/github/service_pr_outcome_sync_test.go +++ b/apps/backend/internal/github/service_pr_outcome_sync_test.go @@ -338,7 +338,7 @@ func TestPersistAndPublishTaskPRSync_PublishesReReadValueNotStaleInMemoryOne(t * staleObservedAt := persistedAt.Add(-time.Minute) staleTP.AutoMergeObservedAt = &staleObservedAt - if err := svc.persistAndPublishTaskPRSync(ctx, &staleTP, true, true); err != nil { + if err := svc.persistAndPublishTaskPRSync(ctx, staleTP.TaskID, nil, &staleTP, true, true); err != nil { t.Fatalf("persistAndPublishTaskPRSync: %v", err) } diff --git a/apps/backend/internal/github/service_pr_unwatched.go b/apps/backend/internal/github/service_pr_unwatched.go index bc7f758b25..4accd0078b 100644 --- a/apps/backend/internal/github/service_pr_unwatched.go +++ b/apps/backend/internal/github/service_pr_unwatched.go @@ -158,7 +158,7 @@ func (s *Service) reconcileTaskPRLifecycle(ctx context.Context, tp *TaskPR, stat // this call's own resolved value. persistAndPublishTaskPRSync already // re-reads before publishing for exactly this reason (codex [P2] on the // SyncTaskPR path); reuse it here instead of duplicating the write. - return s.persistAndPublishTaskPRSync(ctx, tp, changed, status.OutcomeFieldsPopulated) + return s.persistAndPublishTaskPRSync(ctx, tp.TaskID, status.PR, tp, changed, status.OutcomeFieldsPopulated) } // fetchUnwatchedTaskPRs prefers the batched GraphQL query — one call for the diff --git a/apps/backend/internal/github/service_pr_watch.go b/apps/backend/internal/github/service_pr_watch.go index a11814beba..e5682be17c 100644 --- a/apps/backend/internal/github/service_pr_watch.go +++ b/apps/backend/internal/github/service_pr_watch.go @@ -994,7 +994,7 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu ) } - return s.persistAndPublishTaskPRSync(ctx, tp, changed, status.OutcomeFieldsPopulated) + return s.persistAndPublishTaskPRSync(ctx, taskID, status.PR, tp, changed, status.OutcomeFieldsPopulated) } // persistAndPublishTaskPRSync writes the reconciled sync state and, on @@ -1009,7 +1009,9 @@ func (s *Service) SyncTaskPR(ctx context.Context, taskID string, status *PRStatu // what a subsequent read of the row returns (codex [P2]). Split out of // SyncTaskPR to keep that function within the repo's complexity limits and // to make the re-read-before-publish behavior directly testable. -func (s *Service) persistAndPublishTaskPRSync(ctx context.Context, tp *TaskPR, changed, outcomeFieldsPopulated bool) error { +func (s *Service) persistAndPublishTaskPRSync( + ctx context.Context, taskID string, pr *PR, tp *TaskPR, changed, outcomeFieldsPopulated bool, +) error { // AC-38/AC-18c: the counter fires at the populated-ness decision point, // before the write is attempted, and survives write failure — it // measures what the sync observed, not whether the store call happened @@ -1021,7 +1023,7 @@ func (s *Service) persistAndPublishTaskPRSync(ctx context.Context, tp *TaskPR, c // Provider payloads carry the authoritative head/base repository identity // and branch. Reconcile after the TaskPR write so a malformed or // unmatchable payload never prevents the review association from persisting. - s.reconcileComparisonTargetFromSync(ctx, taskID, status.PR) + s.reconcileComparisonTargetFromSync(ctx, taskID, pr) if changed && s.eventBus != nil { published, err := s.store.GetTaskPRByID(ctx, tp.ID) diff --git a/docs/plans/github-pr-sync-context-regression/plan.md b/docs/plans/github-pr-sync-context-regression/plan.md new file mode 100644 index 0000000000..38cf017f40 --- /dev/null +++ b/docs/plans/github-pr-sync-context-regression/plan.md @@ -0,0 +1,110 @@ +--- +spec: docs/specs/pr-outcome-attribution/spec.md +created: 2026-08-22 +status: implemented +--- + +# Implementation Plan: Restore GitHub PR Sync Context + +## Overview + +The pull-request outcome change extracted the final sync write into a helper. +The helper still uses `taskID` and `status.PR`, but its signature does not +receive either value. As a result, the GitHub package does not compile. + +Pass the task and pull-request context into the helper. Keep the existing order: +persist the task PR, reconcile the comparison target, then publish the committed +row when semantic data changed. + +## Confirmed Root Cause + +Commit `9178780ba` extracted `persistAndPublishTaskPRSync` from `SyncTaskPR`. +The extracted body kept this call: + +```go +s.reconcileComparisonTargetFromSync(ctx, taskID, status.PR) +``` + +The new helper signature does not receive `taskID` or `status`. The focused +backend build therefore stops with these compiler errors: + +```text +internal/github/service_pr_watch.go:1024:43: undefined: taskID +internal/github/service_pr_watch.go:1024:51: undefined: status +``` + +Use this reproduction command when the default Go cache is read-only: + +```bash +cd apps/backend && GOCACHE=/tmp/kandev-go-build-cache make build-kandev +``` + +## Backend + +### Sync writer context + +Update `apps/backend/internal/github/service_pr_watch.go`: + +- Add the task ID and pull-request payload to + `persistAndPublishTaskPRSync`. +- Pass `taskID` and `status.PR` from `SyncTaskPR`. +- Pass `tp.TaskID` and `status.PR` from unwatched lifecycle reconciliation. +- Keep comparison-target reconciliation after `UpdateTaskPR` succeeds. +- Keep event publication after comparison-target reconciliation. +- Return immediately when `UpdateTaskPR` fails. + +### Regression coverage + +Add `apps/backend/internal/github/service_comparison_target_sync_test.go`. +Use a comparison-target observer that reads the stored row during its callback. +The test proves that the write occurs first and that the observer receives the +correct task and pull-request identity. + +Update the direct helper call in +`apps/backend/internal/github/service_pr_outcome_sync_test.go`. Pass explicit +task and pull-request context through the helper contract. + +Update the unwatched lifecycle caller in +`apps/backend/internal/github/service_pr_unwatched.go` with the same context. + +## Tests + +- **What:** the GitHub package compiles after the helper receives its required + context. + **File:** `apps/backend/internal/github/service_pr_watch.go`. + **How:** compile the package through the focused Go test command. +- **What:** a normal sync persists the task PR before comparison reconciliation. + **File:** `apps/backend/internal/github/service_comparison_target_sync_test.go`. + **How:** use a fake observer that reads the stored row in its callback. +- **What:** the helper still publishes the committed row after a stale write. + **File:** `apps/backend/internal/github/service_pr_outcome_sync_test.go`. + **How:** run the existing deterministic stale-writer regression test. + +## Verification Results + +- RED: `GOCACHE=/tmp/kandev-go-build-cache go test ./internal/github -run + TestSyncTaskPR_PersistsBeforeComparisonTargetReconciliation` failed at the + expected undefined `taskID` and `status` compiler errors. +- GREEN: the focused regression test passed. +- Targeted tests: the regression test and stale-writer test passed. +- Backend build: `GOCACHE=/tmp/kandev-go-build-cache make build` passed. +- Formatting: `gofmt -w` completed for all changed Go files. +- `git diff --check` passed. +- Build output was written to ignored files under `apps/backend/bin`. +- No temporary source files or E2E assets were created. + +## Implementation Waves And Parallel Candidates + +Wave 1: + +- [x] [task-01-restore-sync-context](task-01-restore-sync-context.md) (done) + +The task is sequential because the source and tests share one package contract. + +## Risks And Out Of Scope + +- The fix does not change pull-request field sourcing or storage schemas. +- The fix does not change comparison-target matching rules. +- The fix does not change event payloads or event ordering. +- The fix does not change the root Makefile output suppression. +- The read-only default Go cache remains an environment concern. diff --git a/docs/plans/github-pr-sync-context-regression/task-01-restore-sync-context.md b/docs/plans/github-pr-sync-context-regression/task-01-restore-sync-context.md new file mode 100644 index 0000000000..e54cd9486a --- /dev/null +++ b/docs/plans/github-pr-sync-context-regression/task-01-restore-sync-context.md @@ -0,0 +1,100 @@ +--- +id: "01-restore-sync-context" +title: "Restore GitHub PR sync context" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/pr-outcome-attribution/spec.md" +--- + +# Task 01: Restore GitHub PR Sync Context + +## Intent + +Restore the missing task and pull-request context in the final sync helper. +Preserve the existing persistence, reconciliation, and publication order. + +## Acceptance + +- `apps/backend/internal/github` compiles without undefined identifiers. +- `SyncTaskPR` persists the task PR before comparison-target reconciliation. +- The comparison observer receives the same task ID and PR identity as the sync. +- Existing stale-writer event behavior remains unchanged. + +## Red Test First + +Add `TestSyncTaskPR_PersistsBeforeComparisonTargetReconciliation` in +`service_comparison_target_sync_test.go`. The current package must fail to +compile because the helper does not receive `taskID` or `status.PR`. + +## Implementation + +- Extend `persistAndPublishTaskPRSync` with `taskID string` and `pr *PR`. +- Pass `taskID` and `status.PR` from `SyncTaskPR`. +- Pass `tp.TaskID` and `status.PR` from unwatched lifecycle reconciliation. +- Use those parameters for `reconcileComparisonTargetFromSync`. +- Update direct helper calls in existing tests. +- Do not change storage, reconciliation, or event behavior. + +## Files Likely Touched + +- `apps/backend/internal/github/service_pr_watch.go` +- `apps/backend/internal/github/service_pr_unwatched.go` +- `apps/backend/internal/github/service_comparison_target_sync_test.go` +- `apps/backend/internal/github/service_pr_outcome_sync_test.go` +- `docs/plans/github-pr-sync-context-regression/plan.md` +- `docs/plans/github-pr-sync-context-regression/task-01-restore-sync-context.md` + +## Dependencies + +None. + +## Parallelism + +`sequential`. The source and tests share one helper contract. + +## Inputs + +- `docs/specs/pr-outcome-attribution/spec.md`, section `Sync writer` +- `docs/specs/platform/workspace-git-status.md`, section + `Repository-qualified comparison targets` +- `docs/decisions/2026-08-19-repository-qualified-comparison-targets.md` + +## Verification + +Run these commands from the repository root: + +```bash +cd apps/backend && \ +GOCACHE=/tmp/kandev-go-build-cache go test ./internal/github -run 'TestSyncTaskPR_PersistsBeforeComparisonTargetReconciliation|TestPersistAndPublishTaskPRSync_PublishesReReadValueNotStaleInMemoryOne' && \ +GOCACHE=/tmp/kandev-go-build-cache make build +``` + +Then run this command from the repository root: + +```bash +git diff --check +``` + +## Output Contract + +Report the changed files, exact command results, blockers, and remaining risks. +Update this task and `plan.md` in the same conversation. + +## Results + +- RED: `GOCACHE=/tmp/kandev-go-build-cache go test ./internal/github -run + TestSyncTaskPR_PersistsBeforeComparisonTargetReconciliation` failed with the + expected undefined `taskID` and `status` compiler errors. +- GREEN: `GOCACHE=/tmp/kandev-go-build-cache go test ./internal/github -run + TestSyncTaskPR_PersistsBeforeComparisonTargetReconciliation` passed. +- Targeted regression command passed: + `GOCACHE=/tmp/kandev-go-build-cache go test ./internal/github -run + "TestSyncTaskPR_PersistsBeforeComparisonTargetReconciliation|TestPersistAndPublishTaskPRSync_PublishesReReadValueNotStaleInMemoryOne"`. +- Backend build passed: + `GOCACHE=/tmp/kandev-go-build-cache make build`. +- `gofmt -w` completed for all changed Go files. +- `git diff --check` passed. +- Build output was written to ignored files under `apps/backend/bin`. +- No temporary source files or E2E assets were created. diff --git a/docs/specs/pr-outcome-attribution/spec.md b/docs/specs/pr-outcome-attribution/spec.md index 9c5ff2f945..9f8dbfb391 100644 --- a/docs/specs/pr-outcome-attribution/spec.md +++ b/docs/specs/pr-outcome-attribution/spec.md @@ -2,6 +2,7 @@ title: Pull request outcome attribution status: shipped created: 2026-08-12 +updated: 2026-08-22 owner: kandev --- @@ -80,6 +81,11 @@ transaction while reading the outgoing row. This preserves values when an operation has no new upstream observation. The stored row is re-read before a sync event is published, so the event matches the committed state. +After the task-PR write succeeds, the normal sync reconciles the +repository-qualified comparison target. The reconciliation receives the same +task ID and pull-request payload as the sync. A reconciliation error does not +reverse the task-PR write. + ## Acceptance criteria - **AC-01 to AC-03:** schema creation and replay add nullable columns without @@ -99,6 +105,8 @@ sync event is published, so the event matches the committed state. - **AC-16 and AC-17:** the auto-merge timestamp is write-once. - **AC-18 to AC-18c:** unchanged syncs publish no update, changed syncs publish the committed row, and failed writes publish nothing. +- **AC-18d:** a successful normal sync keeps its task and pull-request context + through persistence, comparison-target reconciliation, and event publication. - **AC-19:** draft pull requests remain non-mergeable in the stored state. - **AC-30 and AC-30b:** the backend emits explicit nullable JSON keys and the frontend exposes types without adding a core screen. @@ -109,6 +117,14 @@ sync event is published, so the event matches the committed state. - **AC-43, AC-43a, and AC-43p:** replace and restore resolve and preserve outcome fields inside their own transaction using explicit observation flags. +## Regression scenarios + +- **GIVEN** a stored task PR and a comparison-target observer, **WHEN** a normal + sync applies an authoritative pull-request payload, **THEN** the write succeeds + before the observer receives the same task and pull-request identity. +- **GIVEN** comparison-target reconciliation returns an error, **WHEN** the + task-PR write succeeds, **THEN** the stored task-PR update remains committed. + ## Verification Backend unit and persistence tests cover migration replay, source decoding,