PR Review: Reject — Critical bugs from bad rebase

Recommendation: Reject (needs rework)

The feature concept (first-class spec linking) is reasonable, but this PR has multiple critical bugs that appear to result from a bad rebase or merge conflict resolution. It cannot be merged as-is and would break the build.

Critical Bugs

1. SQL syntax error in CloseIssue (2 locations)

Both queries.go and transaction.go change:

UPDATE issues SET ... closed_by_session = ?

to:

UPDATE issues SET ... closed_by_session = ? = NULL

= ? = NULL is invalid SQL. This breaks all issue closing, not just spec-related functionality.

2. Scan/SELECT column mismatches (3+ locations)

The diff removes scan arguments without replacing them, causing the number of Scan() args to not match the number of selected columns. This will panic at runtime.

• GetIssue (queries.go): &dueAt, &deferUntil removed from Scan, &specID never added. SELECT has due_at, defer_until, spec_id — 3 columns with 0 scan targets.
• GetIssueByExternalRef (queries.go): &awaitType, &awaitID, &timeoutNs, &waiters removed from Scan but spec_id added to SELECT — 5 columns with 0 scan targets.
• scanIssueRow (transaction.go): Same pattern — &dueAt, &deferUntil removed, &specID never added.

3. INSERT value/placeholder mismatches (4 functions)

In insertIssue, insertIssueStrict, insertIssues, and insertIssuesStrict:

• Column list adds spec_id (+1 column) and placeholder count goes from 40 to 42 (+2 placeholders)
• But issue.DueAt, issue.DeferUntil values are deleted and issue.SpecID is never added
• Result: 42 placeholders with ~38 values — will not compile or will crash

4. Non-existent spec_changed_at column scanned

scanIssues and scanIssuesWithDependencyType in dependencies.go declare and scan specChangedAt sql.NullTime, but:

• The SELECT does not include spec_changed_at
• The migration does not create a spec_changed_at column
• The schema does not define it

This will panic on every dependency query.

Medium Issues

5. Undeclared struct fields referenced in tests

list_filters_test.go references args.SpecChanged and filter.SpecChanged, but:

• SpecChanged is not defined on ListArgs
• SpecChanged is not defined on IssueFilter

This will not compile.

6. Phantom features documented but not implemented

CLI_REFERENCE.md documents several features with no backing code:

• bd spec scan, bd spec list, bd spec show, bd spec coverage — no spec.go command file exists
• --spec-changed flag on bd list — not registered in list.go init()

Minor Issues

7. Excessive whitespace reformatting

A large portion of the diff is alignment changes to unrelated struct fields, constants, and comments across protocol.go, types.go, etc. This creates unnecessary merge conflicts and makes the actual changes harder to review. Cosmetic reformatting should be in a separate commit or omitted entirely.

Summary

The core idea (linking issues to spec documents via spec_id) is fine and the migration is clean. But the implementation has at least 4 categories of bugs that would crash the application on basic operations (closing issues, querying issues, inserting issues, dependency queries). These look like artifacts of a bad rebase where conflict markers were resolved incorrectly.

Recommendation: Start fresh from current main, apply only the intended changes, and verify with go build ./... and go test ./... before resubmitting.

PR Sheriff: APPROVE (re-review)

All critical issues from the previous review have been addressed:

1. SQL syntax error = ? = NULL — Fixed ✅
2. Scan/SELECT column mismatches — Fixed, spec_id properly added to all SELECT and Scan calls ✅
3. INSERT value/placeholder mismatches — Fixed, column/placeholder/value counts all match ✅
4. Non-existent spec_changed_at — Removed ✅
5. Undeclared SpecChanged struct fields — Removed ✅
6. Phantom bd spec CLI commands — Removed from docs ✅

Feature implementation is clean and follows the existing external_ref pattern:

• SpecID field on Issue type with content hash inclusion
• SQLite migration 041 + Dolt schema update
• All storage backends (sqlite, dolt, memory) properly handle spec_id
• CLI: --spec-id on create/update, --spec filter on list, displayed in show
• Tests cover spec_id prefix filtering

Minor note: Excessive whitespace reformatting in protocol.go, types.go, etc. (cosmetic, not blocking).