diff --git a/apps/backend/internal/github/store.go b/apps/backend/internal/github/store.go index 8809f03289..b25a33ce3c 100644 --- a/apps/backend/internal/github/store.go +++ b/apps/backend/internal/github/store.go @@ -1466,11 +1466,35 @@ func (s *Store) CreateTaskPR(ctx context.Context, tp *TaskPR) error { return err } +// taskPRColumns is the explicit column list for every github_task_prs read. +// Using `SELECT *` here is unsafe: sqlx's StructScan errors out +// ("missing destination name X in *github.TaskPR") the moment the table has +// a column the current TaskPR struct doesn't declare a field for. That drift +// is not hypothetical — a self-update that applies a newer release's +// migration (adding a column) followed by a rollback to an older binary +// leaves exactly this mismatch permanently on disk, since SQLite migrations +// are additive and never reverted. Projecting the known columns keeps every +// read working regardless of what the table has picked up beyond them. +const taskPRColumns = `id, workspace_id, task_id, repository_id, owner, repo, pr_number, pr_url, + pr_title, head_branch, base_branch, author_login, state, review_state, checks_state, + mergeable_state, review_count, pending_review_count, required_reviews, comment_count, + unresolved_review_threads, checks_total, checks_passing, additions, deletions, + created_at, merged_at, closed_at, last_synced_at, updated_at` + +// taskPRColumnsQualified is taskPRColumns with each column qualified by the +// `gtp` alias, for queries that join github_task_prs against another table. +const taskPRColumnsQualified = `gtp.id, gtp.workspace_id, gtp.task_id, gtp.repository_id, gtp.owner, gtp.repo, + gtp.pr_number, gtp.pr_url, gtp.pr_title, gtp.head_branch, gtp.base_branch, gtp.author_login, + gtp.state, gtp.review_state, gtp.checks_state, gtp.mergeable_state, gtp.review_count, + gtp.pending_review_count, gtp.required_reviews, gtp.comment_count, gtp.unresolved_review_threads, + gtp.checks_total, gtp.checks_passing, gtp.additions, gtp.deletions, + gtp.created_at, gtp.merged_at, gtp.closed_at, gtp.last_synced_at, gtp.updated_at` + // GetTaskPR returns the first PR association for a task. For multi-repo tasks // the result is non-deterministic across repos — use ListTaskPRsByTask instead. func (s *Store) GetTaskPR(ctx context.Context, taskID string) (*TaskPR, error) { var tp TaskPR - err := s.ro.GetContext(ctx, &tp, `SELECT * FROM github_task_prs WHERE task_id = ? LIMIT 1`, taskID) + err := s.ro.GetContext(ctx, &tp, `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? LIMIT 1`, taskID) if errors.Is(err, sql.ErrNoRows) { return nil, nil } @@ -1487,7 +1511,7 @@ func (s *Store) GetTaskPR(ctx context.Context, taskID string) (*TaskPR, error) { func (s *Store) GetTaskPRByRepository(ctx context.Context, taskID, repositoryID string) (*TaskPR, error) { var tp TaskPR err := s.ro.GetContext(ctx, &tp, - `SELECT * FROM github_task_prs WHERE task_id = ? AND repository_id = ? + `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? AND repository_id = ? ORDER BY updated_at DESC LIMIT 1`, taskID, repositoryID) if errors.Is(err, sql.ErrNoRows) { @@ -1504,7 +1528,7 @@ func (s *Store) GetTaskPRByRepository(ctx context.Context, taskID, repositoryID func (s *Store) GetTaskPRByRepoAndNumber(ctx context.Context, taskID, repositoryID string, prNumber int) (*TaskPR, error) { var tp TaskPR err := s.ro.GetContext(ctx, &tp, - `SELECT * FROM github_task_prs + `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? AND repository_id = ? AND pr_number = ? LIMIT 1`, taskID, repositoryID, prNumber) if errors.Is(err, sql.ErrNoRows) { @@ -1518,7 +1542,7 @@ func (s *Store) GetTaskPRByRepoAndNumber(ctx context.Context, taskID, repository func (s *Store) ListTaskPRsByTask(ctx context.Context, taskID string) ([]*TaskPR, error) { var prs []TaskPR if err := s.ro.SelectContext(ctx, &prs, - `SELECT * FROM github_task_prs WHERE task_id = ? ORDER BY created_at ASC`, taskID); err != nil { + `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id = ? ORDER BY created_at ASC`, taskID); err != nil { return nil, err } out := make([]*TaskPR, 0, len(prs)) @@ -1536,7 +1560,7 @@ func (s *Store) ListTaskPRsByTaskIDs(ctx context.Context, taskIDs []string) (map return make(map[string][]*TaskPR), nil } query, args, err := sqlx.In( - `SELECT * FROM github_task_prs WHERE task_id IN (?) ORDER BY created_at ASC`, + `SELECT `+taskPRColumns+` FROM github_task_prs WHERE task_id IN (?) ORDER BY created_at ASC`, taskIDs, ) if err != nil { @@ -1556,7 +1580,7 @@ func (s *Store) ListTaskPRsByTaskIDs(ctx context.Context, taskIDs []string) (map func (s *Store) ListTaskPRsByWorkspaceID(ctx context.Context, workspaceID string) (map[string][]*TaskPR, error) { var prs []TaskPR if err := s.ro.SelectContext(ctx, &prs, - `SELECT gtp.* FROM github_task_prs gtp + `SELECT `+taskPRColumnsQualified+` FROM github_task_prs gtp INNER JOIN tasks t ON gtp.task_id = t.id WHERE t.workspace_id = ? ORDER BY gtp.created_at ASC`, workspaceID); err != nil { diff --git a/apps/backend/internal/github/store_taskpr_schema_drift_test.go b/apps/backend/internal/github/store_taskpr_schema_drift_test.go new file mode 100644 index 0000000000..6e15dc5d30 --- /dev/null +++ b/apps/backend/internal/github/store_taskpr_schema_drift_test.go @@ -0,0 +1,96 @@ +package github + +import ( + "context" + "testing" + "time" +) + +// TestTaskPRReads_ToleratesExtraColumn locks in resilience against a +// specific production incident: the "Link GitHub pull request" dialog +// failed with "associate PR with task: missing destination name workspace_id +// in *github.TaskPR" for a task in a workspace whose github_task_prs table +// had picked up a column (via a schema migration from a newer release, e.g. +// a self-update that was later rolled back) that the running binary's +// TaskPR struct doesn't declare. sqlx's StructScan errors out on any SELECT +// * column with no matching destination field, so every TaskPR read query +// must project an explicit column list rather than `SELECT *` / +// `SELECT gtp.*` — otherwise ANY future schema drift ahead of the binary +// breaks every read path, not just the one column that happened to trigger +// the original report. +func TestTaskPRReads_ToleratesExtraColumn(t *testing.T) { + store := newTestStore(t) + ctx := context.Background() + + // Simulate schema drift: a column present in the table that the + // current TaskPR struct has no field for. + if _, err := store.db.Exec( + `ALTER TABLE github_task_prs ADD COLUMN future_only_column TEXT NOT NULL DEFAULT ''`, + ); err != nil { + t.Fatalf("simulate schema drift: %v", err) + } + if _, err := store.db.Exec( + `INSERT INTO tasks (id, workspace_id) VALUES ('task-drift', 'ws-drift')`, + ); err != nil { + t.Fatalf("seed task: %v", err) + } + + tp := &TaskPR{ + WorkspaceID: "ws-drift", + TaskID: "task-drift", + Owner: "kdlbs", + Repo: "kandev", + PRNumber: 1978, + PRURL: "https://github.com/kdlbs/kandev/pull/1978", + PRTitle: "drifted schema", + State: "open", + CreatedAt: time.Now().UTC(), + } + if err := store.ReplaceTaskPR(ctx, tp); err != nil { + t.Fatalf("ReplaceTaskPR: %v", err) + } + + fatalOnScanError := func(name string, err error) { + t.Helper() + if err == nil { + return + } + t.Fatalf("%s: SELECT scan broke on schema drift: %v", name, err) + } + + got, err := store.GetTaskPR(ctx, "task-drift") + fatalOnScanError("GetTaskPR", err) + if got == nil || got.PRNumber != 1978 || got.WorkspaceID != "ws-drift" { + t.Fatalf("GetTaskPR: expected PR #1978 in ws-drift, got %+v", got) + } + + gotByRepo, err := store.GetTaskPRByRepository(ctx, "task-drift", "") + fatalOnScanError("GetTaskPRByRepository", err) + if gotByRepo == nil || gotByRepo.PRNumber != 1978 { + t.Fatalf("GetTaskPRByRepository: expected PR #1978, got %+v", gotByRepo) + } + + gotByNumber, err := store.GetTaskPRByRepoAndNumber(ctx, "task-drift", "", 1978) + fatalOnScanError("GetTaskPRByRepoAndNumber", err) + if gotByNumber == nil || gotByNumber.PRTitle != "drifted schema" { + t.Fatalf("GetTaskPRByRepoAndNumber: expected the drifted-schema PR, got %+v", gotByNumber) + } + + list, err := store.ListTaskPRsByTask(ctx, "task-drift") + fatalOnScanError("ListTaskPRsByTask", err) + if len(list) != 1 || list[0].PRNumber != 1978 { + t.Fatalf("ListTaskPRsByTask: expected 1 PR (#1978), got %+v", list) + } + + byIDs, err := store.ListTaskPRsByTaskIDs(ctx, []string{"task-drift"}) + fatalOnScanError("ListTaskPRsByTaskIDs", err) + if len(byIDs["task-drift"]) != 1 || byIDs["task-drift"][0].PRNumber != 1978 { + t.Fatalf("ListTaskPRsByTaskIDs: expected 1 PR (#1978) for task-drift, got %+v", byIDs) + } + + byWorkspace, err := store.ListTaskPRsByWorkspaceID(ctx, "ws-drift") + fatalOnScanError("ListTaskPRsByWorkspaceID", err) + if len(byWorkspace["task-drift"]) != 1 || byWorkspace["task-drift"][0].PRNumber != 1978 { + t.Fatalf("ListTaskPRsByWorkspaceID: expected 1 PR (#1978) for task-drift, got %+v", byWorkspace) + } +}