johno/beads
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix P0 bug: JSONL lookup now returns LAST match (bd-58c0)

Browse Source 
Addresses code review feedback:

✅ P0 (Must Fix):
- Fix JSONL lookup to return LAST match, not FIRST (resurrection.go:160-162)
  - Changed from early return to scan all matches and keep last
  - Respects JSONL append-only semantics

✅ P1 (Should Fix):
- Add test for multiple JSONL versions
  - TestTryResurrectParent_MultipleVersionsInJSONL verifies correct behavior
- Document error message change in CHANGELOG.md
  - Old: "parent issue X does not exist"
  - New: "parent issue X does not exist and could not be resurrected from JSONL history"
  - Marked as breaking change for script parsers

✅ P2/P3 (Nice to Have):
- Add documentation to AGENTS.md explaining auto-resurrection behavior
- Document best-effort dependency resurrection

⏸️ Deferred (P1 - Optimize batch resurrection):
- Caching optimization deferred (no batch use cases currently)

All tests pass:
- Unit tests: internal/storage/sqlite/
- Integration test: TestImportWithDeletedParent
This commit is contained in:
Steve Yegge
2025-11-04 22:07:36 -08:00
parent 4c77631e48
commit 3af353ade3
6 changed files with 121 additions and 390 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
.beads/beads.jsonl
View File

File diff suppressed because one or more lines are too long

7
AGENTS.md
View File

@@ -213,6 +213,13 @@ bd import -i .beads/issues.jsonl --dry-run # Preview changes
bd import -i .beads/issues.jsonl # Import and update issues
bd import -i .beads/issues.jsonl --dedupe-after # Import + detect duplicates
# Note: Import automatically handles missing parents!
# - If a hierarchical child's parent is missing (e.g., bd-abc.1 but no bd-abc)
# - bd will search the JSONL history for the parent
# - If found, creates a tombstone placeholder (Status=Closed, Priority=4)
# - Dependencies are also resurrected on best-effort basis
# - This prevents import failures after parent deletion
# Find and merge duplicate issues
bd duplicates # Show all duplicates
bd duplicates --auto-merge # Automatically merge all

18
CHANGELOG.md
View File

@@ -7,6 +7,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]
### Added
- **Parent Resurrection** (bd-58c0): Automatic resurrection of deleted parent issues from JSONL history
- Prevents import failures when parent issues have been deleted
- Creates tombstone placeholders for missing hierarchical parents
- Best-effort dependency resurrection from JSONL
### Changed
- **Error Messages**: Improved error messages for missing parent issues
- Old: `"parent issue X does not exist"`
- New: `"parent issue X does not exist and could not be resurrected from JSONL history"`
- **Breaking**: Scripts parsing exact error messages may need updates
### Fixed
- **JSONL Resurrection Logic**: Fixed to use LAST occurrence instead of FIRST (append-only semantics)
## [0.21.7] - 2025-11-04
### Fixed

381
CODE_REVIEW_GUIDE.md
View File

@@ -1,381 +0,0 @@
# Code Review Guide: Parent Resurrection Feature (bd-d19a)
**Branch**: `fix/import-missing-parents`
**Epic**: bd-d19a - Fix import failure on missing parent issues
**Reviewer Instructions**: Follow this guide to perform a systematic code review, filing issues for any problems found.
---
## Overview
This branch implements a **parent resurrection** feature that allows beads to gracefully handle deleted parent issues during import. When a child issue references a missing parent, the system:
1. Searches JSONL history for the deleted parent
2. Creates a tombstone (status=closed) to preserve referential integrity
3. Allows the import to proceed instead of failing atomically
**Key Files Changed**:
- `internal/storage/sqlite/resurrection.go` (NEW)
- `internal/storage/sqlite/sqlite.go` (CreateIssue modifications)
- `internal/storage/sqlite/ids.go` (EnsureIDs modifications)
- `internal/storage/sqlite/child_id_test.go` (test updates)
---
## Critical Review Areas
### 1. **BACKWARDS COMPATIBILITY** ⚠️ HIGHEST PRIORITY
**Why Critical**: Existing beads installations must continue to work without migration, data loss, or behavior changes.
#### 1.1 Database Schema
- [ ] **Review**: Check if any new columns/tables were added to schema
- [ ] **Test**: Confirm old databases work without migration
- [ ] **Verify**: No schema version bump required for resurrection feature
- [ ] **Edge Case**: What happens if user downgrades bd after using resurrection?
- [ ] **Issue**: File bug if resurrection creates tombstones that old versions can't handle
**Files**: `internal/storage/sqlite/schema.go`, `internal/storage/sqlite/resurrection.go`
#### 1.2 JSONL Format
- [ ] **Review**: Confirm JSONL export/import format unchanged
- [ ] **Test**: Old JSONL files can be imported by new version
- [ ] **Test**: New JSONL files (with resurrected parents) can be read by old versions
- [ ] **Verify**: No new fields added to Issue struct that break old parsers
- [ ] **Issue**: File bug if format is incompatible
**Files**: `internal/types/types.go`, `internal/importer/importer.go`
#### 1.3 API/CLI Behavior Changes
- [ ] **Review**: Check if any existing commands have different behavior
- [ ] **Test**: `bd create` with hierarchical IDs still works as before
- [ ] **Test**: `bd import` still works for normal (non-deleted-parent) cases
- [ ] **Verify**: Error messages for truly invalid cases unchanged
- [ ] **Issue**: File bug if existing workflows break
**Files**: `cmd/bd/*.go`
#### 1.4 Error Message Changes
- [ ] **Review**: Document all error message changes (breaking for scripts/tests)
- [ ] **Check**: `internal/storage/sqlite/child_id_test.go:200` - error message updated, why?
- [ ] **Verify**: All tests updated to match new error messages
- [ ] **Issue**: File bug if error messages are worse/less informative than before
**Key Question**: Did we change `"parent issue X does not exist"` to `"parent issue X does not exist and could not be resurrected from JSONL history"`? Is this acceptable?
---
### 2. **Transaction Safety** ⚠️ HIGH PRIORITY
**Why Critical**: SQLite is sensitive to transaction conflicts. The resurrection feature must participate in existing transactions correctly.
#### 2.1 Connection/Transaction Handling
- [ ] **Review**: `resurrection.go` - verify all DB operations use the provided connection
- [ ] **Check**: `tryResurrectParentWithConn()` uses `conn` parameter, not `s.db`
- [ ] **Verify**: No calls to `s.db.Conn()` inside transaction-aware functions
- [ ] **Test**: Run `TestImportWithDeletedParent` to confirm no "database is locked" errors
- [ ] **Edge Case**: What if resurrection is called outside a transaction?
- [ ] **Issue**: File bug if any transaction conflict scenarios remain
**Files**: `internal/storage/sqlite/resurrection.go:38-104`
#### 2.2 Rollback Behavior
- [ ] **Review**: If resurrection fails mid-chain (bd-abc.1.2 → bd-abc fails), does it rollback?
- [ ] **Check**: `tryResurrectParentChainWithConn()` error handling
- [ ] **Verify**: Failed resurrection doesn't leave partial tombstones in DB
- [ ] **Test**: Create test case for partial resurrection failure
- [ ] **Issue**: File bug if rollback behavior is incorrect
**Files**: `internal/storage/sqlite/resurrection.go:189-206`
---
### 3. **Resurrection Logic Correctness**
#### 3.1 Parent Chain Resurrection
- [ ] **Review**: `extractParentChain()` correctly extracts all parent IDs
- [ ] **Test**: Multi-level hierarchy (bd-abc.1.2.3) resurrects bd-abc, bd-abc.1, bd-abc.1.2 in order
- [ ] **Edge Case**: What if JSONL has bd-abc.1.2 but not bd-abc.1? Does it fail gracefully?
- [ ] **Verify**: Root-to-leaf ordering (depth 0 → 1 → 2 → 3)
- [ ] **Issue**: File bug if chain resurrection can fail silently
**Files**: `internal/storage/sqlite/resurrection.go:189-206`, `resurrection.go:207-221`
#### 3.2 JSONL Search
- [ ] **Review**: `findIssueInJSONL()` - can it handle large JSONL files (>10MB)?
- [ ] **Performance**: Line-by-line scanning is O(n) - acceptable?
- [ ] **Edge Case**: What if JSONL is malformed/corrupted?
- [ ] **Edge Case**: What if issue appears multiple times in JSONL (updates)?
- [ ] **Verify**: Uses latest version if issue appears multiple times
- [ ] **Issue**: File bug if JSONL search has correctness or performance issues
**Files**: `internal/storage/sqlite/resurrection.go:116-179`
**Key Question**: Does it pick the FIRST or LAST occurrence of an issue in JSONL? (JSONL may have updates)
#### 3.3 Tombstone Creation
- [ ] **Review**: Tombstone fields - are they correct?
- Status: `closed` ✓
- Priority: `4` (lowest) ✓
- Description: `[RESURRECTED]` prefix ✓
- Timestamps: CreatedAt from original, UpdatedAt/ClosedAt = now ✓
- [ ] **Edge Case**: What if original issue had dependencies? Are they resurrected?
- [ ] **Check**: Lines 79-95 - dependency resurrection is "best effort", acceptable?
- [ ] **Verify**: Tombstone doesn't trigger export/sync loops
- [ ] **Issue**: File bug if tombstone creation causes side effects
**Files**: `internal/storage/sqlite/resurrection.go:48-95`
---
### 4. **Integration Points**
#### 4.1 CreateIssue Integration
- [ ] **Review**: `sqlite.go:182-196` - resurrection called before `insertIssue()`
- [ ] **Verify**: Resurrection only happens for hierarchical IDs (contains ".")
- [ ] **Edge Case**: What if user manually creates issue with ID "bd-abc.1" but parent exists?
- [ ] **Performance**: Does resurrection check happen on EVERY CreateIssue call?
- [ ] **Issue**: File bug if resurrection adds unnecessary overhead
**Files**: `internal/storage/sqlite/sqlite.go:182-196`
#### 4.2 EnsureIDs Integration
- [ ] **Review**: `ids.go:189-202` - resurrection in batch ID validation
- [ ] **Verify**: Works correctly during import (batch operations)
- [ ] **Edge Case**: What if 100 issues all reference same missing parent?
- [ ] **Performance**: Is parent resurrected once or 100 times?
- [ ] **Issue**: File bug if resurrection is inefficient in batch scenarios
**Files**: `internal/storage/sqlite/ids.go:189-202`
#### 4.3 Import Flow
- [ ] **Review**: Does topological sort + resurrection work together correctly?
- [ ] **Check**: `importer.go:540-558` - depth sorting happens BEFORE EnsureIDs
- [ ] **Verify**: Resurrection is a fallback, not the primary mechanism
- [ ] **Test**: Import batch with mix of new hierarchical issues + deleted parent refs
- [ ] **Issue**: File bug if import flow has race conditions or ordering issues
**Files**: `internal/importer/importer.go:540-558`
---
### 5. **Testing Coverage**
#### 5.1 Existing Tests Updated
- [ ] **Review**: `child_id_test.go:200` - why was error message changed?
- [ ] **Verify**: All tests pass with `go test ./internal/storage/sqlite/...`
- [ ] **Check**: Integration test `TestImportWithDeletedParent` passes
- [ ] **Issue**: File bug for any test failures or inadequate test updates
#### 5.2 Missing Test Cases (File issues for these!)
- [ ] **Test Case**: Multi-level resurrection (bd-abc.1.2 with missing bd-abc and bd-abc.1)
- [ ] **Test Case**: Resurrection when JSONL doesn't exist at all
- [ ] **Test Case**: Resurrection when JSONL exists but parent never existed
- [ ] **Test Case**: Resurrection with corrupted JSONL file
- [ ] **Test Case**: Resurrection during concurrent operations (race conditions)
- [ ] **Test Case**: Backwards compat - old DB with new code, new DB with old code
- [ ] **Test Case**: Performance test - resurrect parent with 1000 children
- [ ] **Test Case**: Dependency resurrection - parent had dependencies, are they restored?
**Files**: `beads_integration_test.go`, `internal/storage/sqlite/*_test.go`
---
### 6. **Edge Cases & Error Handling**
#### 6.1 Error Handling
- [ ] **Review**: All error paths return meaningful messages
- [ ] **Check**: JSONL file doesn't exist - returns `nil, nil` (acceptable?)
- [ ] **Check**: Parent not found in JSONL - returns `false, nil` (acceptable?)
- [ ] **Check**: Database locked - returns error (handled by transaction refactor)
- [ ] **Verify**: No silent failures that leave DB in inconsistent state
- [ ] **Issue**: File bug for any unclear or missing error messages
#### 6.2 Pathological Cases
- [ ] **Edge Case**: Circular references (bd-abc.1 parent is bd-abc.1.2)?
- [ ] **Edge Case**: ID with 10+ levels of nesting (bd-abc.1.2.3.4.5.6.7.8.9.10)?
- [ ] **Edge Case**: Issue ID contains multiple dots in hash (bd-abc.def.1)?
- [ ] **Edge Case**: JSONL has 1 million issues - how long does resurrection take?
- [ ] **Edge Case**: Resurrection triggered during daemon auto-sync - conflicts?
- [ ] **Issue**: File bug for any unhandled pathological cases
---
### 7. **Documentation & User Impact**
#### 7.1 User-Facing Documentation
- [ ] **Review**: Is resurrection behavior documented in README.md?
- [ ] **Review**: Is it documented in AGENTS.md (for AI users)?
- [ ] **Review**: Should there be a tombstone cleanup command (bd compact-tombstones)?
- [ ] **Verify**: Error messages guide users to solutions
- [ ] **Issue**: File doc bug if feature is undocumented
#### 7.2 Migration Path
- [ ] **Review**: Do users need to do anything when upgrading?
- [ ] **Review**: What happens to existing orphaned children in old DBs?
- [ ] **Verify**: Feature is opt-in or backwards compatible by default?
- [ ] **Issue**: File bug if migration path is unclear
---
## Review Workflow
### Step 1: Read the Code
```bash
# Review core resurrection logic
cat internal/storage/sqlite/resurrection.go
# Review integration points
cat internal/storage/sqlite/sqlite.go
cat internal/storage/sqlite/ids.go
# Review tests
cat internal/storage/sqlite/child_id_test.go
cat beads_integration_test.go | grep -A 30 "TestImportWithDeletedParent"
```
### Step 2: Run Tests
```bash
# Unit tests
go test ./internal/storage/sqlite/... -v
# Integration tests
go test -v ./beads_integration_test.go -run TestImportWithDeletedParent
# Full test suite
go test ./...
```
### Step 3: Test Backwards Compatibility Manually
```bash
# Create old-style database
git checkout main
./bd init --prefix test
./bd create "Parent" --id test-parent
./bd create "Child" --id test-parent.1
# Switch to new branch and verify it works
git checkout fix/import-missing-parents
./bd show test-parent test-parent.1 # Should work
# Test resurrection
./bd delete test-parent
./bd sync
# Edit JSONL to add back child reference, then import
./bd import -i .beads/issues.jsonl
./bd show test-parent # Should show tombstone
```
### Step 4: File Issues
For each problem found, create an issue:
```bash
./bd create "BUG: [description]" -t bug -p 0 --deps discovered-from:bd-d19a
```
---
## Specific Code Snippets to Review
### Critical: Transaction Handling
**File**: `internal/storage/sqlite/resurrection.go:67-76` (BEFORE fix)
```go
// Get a connection for the transaction
conn, err := s.db.Conn(ctx)
if err != nil {
return false, fmt.Errorf("failed to get database connection: %w", err)
}
defer conn.Close()
// Insert tombstone into database
if err := insertIssue(ctx, conn, tombstone); err != nil {
return false, fmt.Errorf("failed to create tombstone for parent %s: %w", parentID, err)
}
```
**Question**: Does this create a NEW connection inside an existing transaction? (YES - this was bd-58c0 bug, fixed by refactor)
### Critical: Error Message Change
**File**: `internal/storage/sqlite/child_id_test.go:200`
```go
// OLD:
if err != nil && err.Error() != "parent issue bd-nonexistent does not exist" {
// NEW:
expectedErr := "parent issue bd-nonexistent does not exist and could not be resurrected from JSONL history"
if err != nil && err.Error() != expectedErr {
```
**Question**: Is this error message change acceptable for backwards compatibility? (Scripts parsing errors may break)
### Critical: JSONL Lookup Logic
**File**: `internal/storage/sqlite/resurrection.go:138-156`
```go
for scanner.Scan() {
lineNum++
line := scanner.Text()
// Quick check: does this line contain our issue ID?
if !strings.Contains(line, `"`+issueID+`"`) {
continue
}
// Parse JSON
var issue types.Issue
if err := json.Unmarshal([]byte(line), &issue); err != nil {
fmt.Fprintf(os.Stderr, "Warning: skipping malformed JSONL line %d: %v\n", lineNum, err)
continue
}
if issue.ID == issueID {
return &issue, nil // Returns FIRST match
}
}
```
**Question**: Should this return the LAST match instead (latest version of issue)? JSONL may have updates.
---
## Sign-Off Checklist
Before approving this branch for merge, confirm:
- [ ] All backwards compatibility concerns addressed
- [ ] No schema migration required
- [ ] No JSONL format changes
- [ ] Transaction safety verified (no "database is locked" errors)
- [ ] Error messages are informative and backwards-compatible
- [ ] Integration tests pass (`TestImportWithDeletedParent`)
- [ ] Unit tests pass (`go test ./internal/storage/sqlite/...`)
- [ ] Documentation updated (README.md, AGENTS.md)
- [ ] Edge cases have test coverage or filed issues
- [ ] Performance acceptable for common cases (resurrect 1 parent with 100 children)
---
## Expected Issues to File
Based on this review, you should file issues for:
1. **JSONL lookup returns first match, not last** - should return latest version
2. **No test for multi-level resurrection** (bd-abc.1.2 with missing bd-abc and bd-abc.1)
3. **No test for resurrection when JSONL doesn't exist**
4. **No test for backwards compat** (old DB → new code, new DB → old code)
5. **Error message change may break user scripts** - document or add deprecation warning
6. **Performance concern**: Resurrection in EnsureIDs may resurrect same parent N times for N children
7. **Missing documentation** - README.md doesn't mention resurrection feature
8. **No cleanup mechanism** for tombstones (consider `bd compact-tombstones` command)
---
## Questions for Original Implementer
1. Why does `findIssueInJSONL()` return the FIRST match instead of LAST?
2. Is the error message change acceptable for backwards compatibility?
3. Should resurrection be opt-in via config flag?
4. What's the performance impact of resurrection on large JSONL files (1M+ issues)?
5. Should tombstones be marked with a special flag (`is_tombstone=true`) in the database?
---
**Good luck with the review!** Be thorough, file issues for everything you find, and don't hesitate to ask questions.

8
internal/storage/sqlite/resurrection.go
View File

@@ -133,6 +133,7 @@ func (s *SQLiteStorage) findIssueInJSONL(issueID string) (*types.Issue, error) {
scanner.Buffer(buf, maxCapacity)
lineNum := 0
var lastMatch *types.Issue
for scanner.Scan() {
lineNum++
line := scanner.Text()
@@ -156,9 +157,10 @@ func (s *SQLiteStorage) findIssueInJSONL(issueID string) (*types.Issue, error) {
continue
}
// Check if this is the issue we're looking for
// Keep the last occurrence (JSONL append-only semantics)
if issue.ID == issueID {
return &issue, nil
issueCopy := issue
lastMatch = &issueCopy
}
}
@@ -166,7 +168,7 @@ func (s *SQLiteStorage) findIssueInJSONL(issueID string) (*types.Issue, error) {
return nil, fmt.Errorf("error reading JSONL file: %w", err)
}
return nil, nil // Not found
return lastMatch, nil // Returns last match or nil if not found
}
// TryResurrectParentChain recursively resurrects all missing parents in a hierarchical ID chain.

84
internal/storage/sqlite/resurrection_test.go
View File

@@ -539,6 +539,90 @@ func writeIssuesToJSONL(path string, issues []types.Issue) error {
return nil
}
// TestTryResurrectParent_MultipleVersionsInJSONL verifies that the LAST occurrence is used
func TestTryResurrectParent_MultipleVersionsInJSONL(t *testing.T) {
s := newTestStore(t, "")
ctx := context.Background()
// Create JSONL with multiple versions of the same issue (append-only semantics)
dbDir := filepath.Dir(s.dbPath)
jsonlPath := filepath.Join(dbDir, "issues.jsonl")
// First version: priority 3, title "Old Version"
v1 := &types.Issue{
ID: "bd-multi",
Title: "Old Version",
Status: types.StatusOpen,
Priority: 3,
IssueType: types.TypeTask,
CreatedAt: time.Now(),
UpdatedAt: time.Now(),
}
v1JSON, _ := json.Marshal(v1)
// Second version: priority 2, title "Updated Version"
time.Sleep(10 * time.Millisecond) // Ensure different timestamp
v2 := &types.Issue{
ID: "bd-multi",
Title: "Updated Version",
Status: types.StatusInProgress,
Priority: 2,
IssueType: types.TypeTask,
CreatedAt: v1.CreatedAt, // Same creation time
UpdatedAt: time.Now(),
}
v2JSON, _ := json.Marshal(v2)
// Third version: priority 1, title "Latest Version"
time.Sleep(10 * time.Millisecond)
v3 := &types.Issue{
ID: "bd-multi",
Title: "Latest Version",
Status: types.StatusClosed,
Priority: 1,
IssueType: types.TypeTask,
CreatedAt: v1.CreatedAt,
UpdatedAt: time.Now(),
}
v3JSON, _ := json.Marshal(v3)
// Write all three versions (append-only)
content := string(v1JSON) + "\n" + string(v2JSON) + "\n" + string(v3JSON) + "\n"
if err := os.WriteFile(jsonlPath, []byte(content), 0644); err != nil {
t.Fatalf("Failed to create JSONL: %v", err)
}
// Resurrect - should get the LAST version (v3)
resurrected, err := s.TryResurrectParent(ctx, "bd-multi")
if err != nil {
t.Fatalf("TryResurrectParent failed: %v", err)
}
if !resurrected {
t.Error("Expected successful resurrection")
}
// Verify we got the latest version's data
retrieved, err := s.GetIssue(ctx, "bd-multi")
if err != nil {
t.Fatalf("Failed to retrieve resurrected issue: %v", err)
}
// Most important: title should be from LAST occurrence (v3)
if retrieved.Title != "Latest Version" {
t.Errorf("Expected title 'Latest Version', got '%s' (should use LAST occurrence in JSONL)", retrieved.Title)
}
// CreatedAt should be preserved from original (all versions share this)
if !retrieved.CreatedAt.Equal(v1.CreatedAt) {
t.Errorf("Expected CreatedAt %v, got %v", v1.CreatedAt, retrieved.CreatedAt)
}
// Note: Priority, Status, and Description are modified by tombstone logic
// (Priority=4, Status=Closed, Description="[RESURRECTED]...")
// This is expected behavior - the test verifies we read the LAST occurrence
// before creating the tombstone.
}
// Helper function to check if string contains substring
func contains(s, substr string) bool {
return len(s) > 0 && len(substr) > 0 && (s == substr || len(s) >= len(substr) && (s[:len(substr)] == substr || s[len(s)-len(substr):] == substr || containsMiddle(s, substr)))