From d86f359e63dadcb3d79d8f5662f0227a7622dc5a Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Sat, 1 Nov 2025 22:51:58 -0700 Subject: [PATCH] fix: DetectCycles SQL bug and add comprehensive tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix SQL query bug preventing cycle detection (bd-9f20) - Allow revisiting start node to complete cycle - Remove duplicate start_id concatenation in final SELECT - Add cycle_detection_test.go with comprehensive test coverage (bd-cdf7) - Simple 2-node cycles - Complex multi-node cycles (4-node, 10-node) - Self-loops - Multiple independent cycles - Acyclic graphs (diamond, chain) - Empty graph and single node edge cases - Mixed dependency types Improves sqlite package coverage: 68.2% → 69.1% --- .beads/beads.jsonl | 3 +- .../storage/sqlite/cycle_detection_test.go | 506 ++++++++++++++++++ internal/storage/sqlite/dependencies.go | 14 +- 3 files changed, 515 insertions(+), 8 deletions(-) create mode 100644 internal/storage/sqlite/cycle_detection_test.go diff --git a/.beads/beads.jsonl b/.beads/beads.jsonl index 914d705c..2a8203f3 100644 --- a/.beads/beads.jsonl +++ b/.beads/beads.jsonl @@ -117,6 +117,7 @@ {"id":"bd-9ae788be","content_hash":"19599f6bcc268e97438593e08eb6343b551ce765f0d91956591aa811cbb90690","title":"Implement clone-scoped ID allocation to prevent N-way collisions","description":"## Problem\nCurrent ID allocation uses per-clone atomic counters (issue_counters table) that sync based on local database state. In N-way collision scenarios:\n- Clone B sees {test-1} locally, allocates test-2\n- Clone D sees {test-1, test-2, test-3} locally, allocates test-4\n- When same content gets assigned test-2 and test-4, convergence fails\n\nRoot cause: Each clone independently allocates IDs without global coordination, leading to overlapping assignments for the same content.\n\n## Solution\nAdd clone UUID to ID allocation to make every ID globally unique:\n\n**Current format:** `test-1`, `test-2`, `test-3`\n**New format:** `test-1-a7b3`, `test-2-a7b3`, `test-3-c4d9`\n\nWhere suffix is first 4 chars of clone UUID.\n\n## Implementation\n\n### 1. Add clone_identity table\n```sql\nCREATE TABLE clone_identity (\n clone_uuid TEXT PRIMARY KEY,\n created_at DATETIME DEFAULT CURRENT_TIMESTAMP\n);\n```\n\n### 2. Modify getNextIDForPrefix()\n```go\nfunc (s *SQLiteStorage) getNextIDForPrefix(ctx context.Context, prefix string) (string, error) {\n cloneUUID := s.getOrCreateCloneUUID(ctx)\n shortUUID := cloneUUID[:4]\n \n nextNum := s.getNextCounterForPrefix(ctx, prefix)\n return fmt.Sprintf(\"%s-%d-%s\", prefix, nextNum, shortUUID), nil\n}\n```\n\n### 3. Update ID parsing logic\nAll places that parse IDs (utils.ExtractIssueNumber, etc.) need to handle new format.\n\n### 4. Migration strategy\n- Existing IDs remain unchanged (no suffix)\n- New IDs get clone suffix automatically\n- Display layer can hide suffix in UI: `bd-cb64c226.3-a7b3` → `#42`\n\n## Benefits\n- **Zero collision risk**: Same content in different clones gets different IDs\n- **Maintains readability**: Still sequential numbering within clone\n- **No coordination needed**: Works offline, no central authority\n- **Scales to 100+ clones**: 4-char hex = 65,536 unique clones\n\n## Concerns\n- ID format change may break existing integrations\n- Need migration path for existing databases\n- Display logic needs update to hide/show suffixes appropriately\n\n## Success Criteria\n- 10+ clone collision test passes without failures\n- Existing issues continue to work (backward compatibility)\n- Documentation updated with new ID format\n- Migration guide for v1.x → v2.x\n\n## Timeline\nMedium-term (v1.1-v1.2), 2-3 weeks implementation\n\n## References\n- Related to bd-e6d71828 (immediate fix)\n- See beads_nway_test.go for failing N-way tests","status":"open","priority":2,"issue_type":"feature","created_at":"2025-10-29T10:22:52.260524-07:00","updated_at":"2025-10-30T17:12:58.18193-07:00"} {"id":"bd-9e8d","content_hash":"8a2616a707f5ae932357049b0bc922716f4d729724fb8c38b256c91e292f851b","title":"Test Issue","description":"","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-10-31T21:41:11.107393-07:00","updated_at":"2025-11-01T20:02:28.292279-07:00","closed_at":"2025-11-01T20:02:28.292279-07:00"} {"id":"bd-9f1fce5d","content_hash":"14b0d330680e979e504043d2c560bd2eda204698f5622c3bdc6f91816f861d22","title":"Add internal/ai package for LLM integration","description":"Shared AI client for repair commands.\n\nProviders:\n- Anthropic (Claude)\n- OpenAI (GPT)\n- Ollama (local)\n\nEnv vars:\n- BEADS_AI_PROVIDER\n- BEADS_AI_API_KEY\n- BEADS_AI_MODEL\n\nFiles: internal/ai/client.go (new)","status":"open","priority":1,"issue_type":"task","created_at":"2025-10-28T14:48:29.072473-07:00","updated_at":"2025-10-30T17:12:58.219706-07:00"} +{"id":"bd-9f20","content_hash":"523ec11b4c3e82c8d5b36deeb284ebae19eee2090a7051b599da79366c642238","title":"DetectCycles SQL query has bug preventing cycle detection","description":"The DetectCycles function's SQL query has a bug in the LIKE filter that prevents it from detecting cycles.\n\nCurrent code (line 571):\n```sql\nAND p.path NOT LIKE '%' || d.depends_on_id || '→%'\n```\n\nThis prevents ANY revisit to nodes, including returning to the start node to complete a cycle.\n\nFix:\n```sql\nAND (d.depends_on_id = p.start_id OR p.path NOT LIKE '%' || d.depends_on_id || '→%')\n```\n\nThis allows revisiting the start node (to detect the cycle) while still preventing intermediate node revisits.\n\nImpact: Currently DetectCycles cannot detect any cycles, but this hasn't been noticed because AddDependency prevents cycles from being created. The function would only matter if cycles were manually inserted into the database.","status":"open","priority":3,"issue_type":"bug","created_at":"2025-11-01T22:50:32.552763-07:00","updated_at":"2025-11-01T22:50:32.552763-07:00"} {"id":"bd-a03d5e36","content_hash":"b4ee73e439a133a77e5df27e3e457674dbc9968fdbee0dc630175726960bb8cf","title":"Improve integration test coverage for stateful features","description":"","design":"## Context\n\nbd-70419816 revealed a critical gap: the export deduplication feature had unit tests but no integration tests simulating real-world git operations. This led to silent data loss in production.\n\n## Root Cause\n- Unit tests only tested functions in isolation\n- No integration tests for git operations (pull, reset, checkout) modifying JSONL\n- No tests validating export_hashes and JSONL stay in sync\n- Missing tests for stateful distributed system interactions (DB + JSONL + git)\n\n## Completed (bd-70419816)\n✓ TestJSONLIntegrityValidation - unit tests for validation logic\n✓ TestImportClearsExportHashes - tests import clears hashes\n✓ TestExportIntegrityAfterJSONLTruncation - simulates git reset (would have caught bd-70419816)\n✓ TestExportIntegrityAfterJSONLDeletion - tests recovery from file deletion\n✓ TestMultipleExportsStayConsistent - tests repeated exports\n\n## Still Needed (High Priority)\n1. Multi-repo sync test - two clones staying in sync after push/pull\n2. Auto-flush integration test - JSONL integrity preserved during auto-flush\n3. Daemon auto-sync integration test - complex state management\n4. Import after corruption test - recovery from partial data loss\n\n## Medium Priority\n- Partial export failure handling (disk full, network interruption)\n- Concurrent export/import race conditions\n- Large dataset performance tests (1000+ issues)\n- Export hash migration tests (version upgrades)\n\n## Testing Principles\n1. Test real-world scenarios: git ops, user errors, system failures, concurrent ops\n2. Integration tests for stateful systems (DB + files + git)\n3. Regression test for every bug fix\n4. Test invariants: JSONL count == DB count, hash consistency, etc.\n\n## Key Lesson\nStateful distributed systems need integration tests, not just unit tests.","acceptance_criteria":"- [ ] Multi-repo sync test implemented\n- [ ] Auto-flush integration test implemented \n- [ ] Daemon auto-sync integration test implemented\n- [ ] Testing guidelines added to CONTRIBUTING.md\n- [ ] CI runs integration tests\n- [ ] All critical workflows have integration test coverage","status":"open","priority":2,"issue_type":"epic","created_at":"2025-10-29T21:53:15.397137-07:00","updated_at":"2025-10-30T17:12:58.206063-07:00"} {"id":"bd-a1691807","content_hash":"23f0119ee9df98f1bf6d648dba06065c156963064ef1c7308dfb02c8bdd5bc58","title":"Integration test: mutation to export latency","description":"Measure time from bd create to JSONL update. Verify \u003c500ms latency. Test with multiple rapid mutations to verify batching.","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-29T20:49:49.105247-07:00","updated_at":"2025-10-31T12:00:43.198883-07:00","closed_at":"2025-10-31T12:00:43.198883-07:00"} {"id":"bd-a40f374f","content_hash":"599448515198700decd2494cf0cea3335b013c573bdcbda90a151564585cf845","title":"bd validate - Comprehensive health check","description":"Run all validation checks in one command.\n\nChecks:\n- Duplicates\n- Orphaned dependencies\n- Test pollution\n- Git conflicts\n\nSupports --fix-all for auto-repair.\n\nDepends on bd-cbed9619.1, bd-0dcea000, bd-31aab707, bd-9826b69a.\n\nFiles: cmd/bd/validate.go (new)","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-29T20:02:47.956664-07:00","updated_at":"2025-10-30T17:12:58.195108-07:00","closed_at":"2025-10-29T20:02:15.318966-07:00"} @@ -149,7 +150,7 @@ {"id":"bd-cbed9619.3","content_hash":"ecebc4a18d5355bafc88e778ee87365717f894d3590d325a97ecf8b3f763d54d","title":"Implement global N-way collision resolution algorithm","description":"## Overview\nPhase 3: Replace pairwise collision resolution with global N-way resolution that produces deterministic results regardless of sync order.\n\n## Current Problem\nScoreCollisions (collision.go:228) compares issues pairwise:\n```go\ncollision.RemapIncoming = existingHash \u003c incomingHash\n```\n\nThis works for 2-way but fails for 3+ way because:\n- Each clone makes local decisions without global context\n- No guarantee intermediate states are consistent\n- Remapping decisions depend on sync order\n- Can't detect transitive remap chains (test-1 → test-2 → test-3)\n\n## Solution\nImplement global resolution that:\n1. Collects ALL versions of same logical issue\n2. Sorts by content hash (deterministic)\n3. Assigns sequential IDs based on sorted order\n4. All clones converge to same assignments\n\n## Implementation Tasks\n\n### 1. Create ResolveNWayCollisions function\nFile: internal/storage/sqlite/collision.go\n\nReplace ScoreCollisions with:\n```go\n// ResolveNWayCollisions handles N-way collisions deterministically.\n// Groups all versions with same base ID, sorts by content hash,\n// assigns sequential IDs. Returns mapping of old ID → new ID.\nfunc ResolveNWayCollisions(ctx context.Context, s *SQLiteStorage,\n collisions []*CollisionDetail, incoming []*types.Issue) (map[string]string, error) {\n \n if len(collisions) == 0 {\n return make(map[string]string), nil\n }\n \n // Group by base ID pattern (e.g., test-1, test-2 → base \"test-1\")\n groups := groupCollisionsByBaseID(collisions)\n \n idMapping := make(map[string]string)\n \n for baseID, versions := range groups {\n // 1. Collect all unique versions by content hash\n uniqueVersions := deduplicateVersionsByContentHash(versions)\n \n // 2. Sort by content hash (deterministic!)\n sort.Slice(uniqueVersions, func(i, j int) bool {\n return uniqueVersions[i].ContentHash \u003c uniqueVersions[j].ContentHash\n })\n \n // 3. Assign sequential IDs based on sorted order\n prefix := extractPrefix(baseID)\n baseNum := extractNumber(baseID)\n \n for i, version := range uniqueVersions {\n targetID := fmt.Sprintf(\"%s-%d\", prefix, baseNum+i)\n \n // Map this version to its deterministic ID\n if version.ID != targetID {\n idMapping[version.ID] = targetID\n }\n }\n }\n \n return idMapping, nil\n}\n```\n\n### 2. Implement helper functions\n\n```go\n// groupCollisionsByBaseID groups collisions by their logical base ID\nfunc groupCollisionsByBaseID(collisions []*CollisionDetail) map[string][]*types.Issue {\n groups := make(map[string][]*types.Issue)\n for _, c := range collisions {\n baseID := c.ID // All share same ID (that's why they collide)\n groups[baseID] = append(groups[baseID], c.ExistingIssue, c.IncomingIssue)\n }\n return groups\n}\n\n// deduplicateVersionsByContentHash keeps one issue per unique content hash\nfunc deduplicateVersionsByContentHash(issues []*types.Issue) []*types.Issue {\n seen := make(map[string]*types.Issue)\n for _, issue := range issues {\n if _, found := seen[issue.ContentHash]; !found {\n seen[issue.ContentHash] = issue\n }\n }\n result := make([]*types.Issue, 0, len(seen))\n for _, issue := range seen {\n result = append(result, issue)\n }\n return result\n}\n```\n\n### 3. Update handleCollisions in importer\nFile: internal/importer/importer.go\n\nReplace ScoreCollisions call with:\n```go\n// OLD:\nif err := sqlite.ScoreCollisions(ctx, sqliteStore, collisionResult.Collisions, allExistingIssues); err != nil {\n return nil, fmt.Errorf(\"failed to score collisions: %w\", err)\n}\n\n// NEW:\nidMapping, err := sqlite.ResolveNWayCollisions(ctx, sqliteStore, \n collisionResult.Collisions, issues)\nif err != nil {\n return nil, fmt.Errorf(\"failed to resolve collisions: %w\", err)\n}\n```\n\n### 4. Update RemapCollisions\nRemapCollisions currently uses collision.RemapIncoming field. Update to use idMapping directly:\n- Remove RemapIncoming logic\n- Use idMapping to determine what to remap\n- Simplify to just apply the computed mapping\n\n### 5. Add comprehensive tests\n\nTest cases:\n1. 3-way collision with different content → 3 sequential IDs\n2. 3-way collision with 2 identical content → 2 IDs (dedupe works)\n3. Sync order independence (A→B→C vs C→A→B produce same result)\n4. Content hash ordering is respected\n5. Works with 5+ clones\n\n## Acceptance Criteria\n- ResolveNWayCollisions implemented and replaces ScoreCollisions\n- Groups all versions of same ID together\n- Deduplicates by content hash\n- Sorts by content hash deterministically\n- Assigns sequential IDs starting from base ID\n- Returns complete mapping (old ID → new ID)\n- All clones converge to same ID assignments\n- Works for arbitrary N-way collisions\n- TestThreeCloneCollision passes (or gets much closer)\n\n## Files to Modify\n- internal/storage/sqlite/collision.go (new function, helpers)\n- internal/importer/importer.go (call new function)\n- internal/storage/sqlite/collision_test.go (comprehensive tests)\n\n## Testing Strategy\n\n### Unit Tests\n- groupCollisionsByBaseID correctly groups\n- deduplicateVersionsByContentHash removes duplicates\n- Sorting by hash is stable and deterministic\n- Sequential ID assignment is correct\n\n### Integration Tests\n- 3-way collision resolves to 3 issues\n- Sync order doesn't affect final IDs\n- Content hash ordering determines winner\n\n### Property Tests\n- For any N clones with same content, all converge to same IDs\n- Idempotent: running resolution twice produces same result\n\n## Dependencies\n- Requires bd-cbed9619.5 (ContentHash field) to be completed first\n- Requires bd-cbed9619.4 (read-only detection) for clean integration\n\n## Notes\nThis is the core algorithm that enables convergence. The key insight:\n**Sort by content hash globally, not pairwise comparison.**","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-28T18:37:42.85616-07:00","updated_at":"2025-10-30T17:12:58.228707-07:00","closed_at":"2025-10-28T20:03:26.675257-07:00","dependencies":[{"issue_id":"bd-cbed9619.3","depends_on_id":"bd-325da116","type":"parent-child","created_at":"2025-10-28T18:39:20.593102-07:00","created_by":"daemon"},{"issue_id":"bd-cbed9619.3","depends_on_id":"bd-cbed9619.5","type":"blocks","created_at":"2025-10-28T18:39:28.30886-07:00","created_by":"daemon"},{"issue_id":"bd-cbed9619.3","depends_on_id":"bd-cbed9619.4","type":"blocks","created_at":"2025-10-28T18:39:28.336312-07:00","created_by":"daemon"}]} {"id":"bd-cbed9619.4","content_hash":"49aad5fa2497f7f88fb74d54553825b93c1021ed7db04cfb2e58682699d8dca9","title":"Make DetectCollisions read-only (separate detection from modification)","description":"## Overview\nPhase 2: Separate collision detection from state modification to enable safe, composable collision resolution.\n\n## Current Problem\nDetectCollisions (collision.go:38-111) modifies database state during detection:\n- Line 83-86: Deletes issues when content matches but ID differs\n- This violates separation of concerns\n- Causes race conditions when processing multiple issues\n- Makes contentToDBIssue map stale after first deletion\n- Partial failures leave DB in inconsistent state\n\n## Solution\nMake DetectCollisions purely read-only. Move all modifications to a separate ApplyCollisionResolution function.\n\n## Implementation Tasks\n\n### 1. Add RenameDetail to CollisionResult\nFile: internal/storage/sqlite/collision.go\n```go\ntype CollisionResult struct {\n ExactMatches []string\n Collisions []*CollisionDetail\n NewIssues []string\n Renames []*RenameDetail // NEW\n}\n\ntype RenameDetail struct {\n OldID string // ID in database\n NewID string // ID in incoming\n Issue *types.Issue // The issue with new ID\n}\n```\n\n### 2. Remove deletion from DetectCollisions\nReplace lines 83-86:\n```go\n// OLD (DELETE THIS):\nif err := s.DeleteIssue(ctx, dbMatch.ID); err != nil {\n return nil, fmt.Errorf(\"failed to delete renamed issue...\")\n}\n\n// NEW (ADD THIS):\nresult.Renames = append(result.Renames, \u0026RenameDetail{\n OldID: dbMatch.ID,\n NewID: incoming.ID,\n Issue: incoming,\n})\ncontinue // Don't mark as NewIssue yet\n```\n\n### 3. Create ApplyCollisionResolution function\nNew function to apply all modifications atomically:\n```go\nfunc ApplyCollisionResolution(ctx context.Context, s *SQLiteStorage,\n result *CollisionResult, mapping map[string]string) error {\n \n // Phase 1: Handle renames (delete old IDs)\n for _, rename := range result.Renames {\n if err := s.DeleteIssue(ctx, rename.OldID); err != nil {\n return fmt.Errorf(\"failed to delete renamed issue %s: %w\", \n rename.OldID, err)\n }\n }\n \n // Phase 2: Create new IDs (from mapping)\n // Phase 3: Update references\n return nil\n}\n```\n\n### 4. Update callers to use two-phase approach\nFile: internal/importer/importer.go (handleCollisions)\n```go\n// Phase 1: Detect (read-only)\ncollisionResult, err := sqlite.DetectCollisions(ctx, sqliteStore, issues)\n\n// Phase 2: Resolve (compute mapping)\nmapping, err := sqlite.ResolveNWayCollisions(ctx, sqliteStore, collisionResult)\n\n// Phase 3: Apply (modify DB)\nerr = sqlite.ApplyCollisionResolution(ctx, sqliteStore, collisionResult, mapping)\n```\n\n### 5. Update tests\n- Verify DetectCollisions doesn't modify DB\n- Test ApplyCollisionResolution separately\n- Add test for rename detection without modification\n\n## Acceptance Criteria\n- DetectCollisions performs zero writes to database\n- DetectCollisions returns RenameDetail entries for content matches\n- ApplyCollisionResolution handles all modifications\n- All existing tests still pass\n- New test verifies read-only detection\n- contentToDBIssue map stays consistent throughout detection\n\n## Files to Modify\n- internal/storage/sqlite/collision.go (DetectCollisions, new function)\n- internal/importer/importer.go (handleCollisions caller)\n- internal/storage/sqlite/collision_test.go (add tests)\n\n## Testing\n- Unit test: DetectCollisions with content match doesn't delete DB issue\n- Unit test: RenameDetail correctly populated\n- Unit test: ApplyCollisionResolution applies renames\n- Integration test: Full flow still works end-to-end\n\n## Risk Mitigation\nThis is a significant refactor of core collision logic. Recommend:\n1. Add comprehensive tests before modifying\n2. Use feature flag to enable/disable new behavior\n3. Test thoroughly with TestTwoCloneCollision first","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-28T18:37:09.652326-07:00","updated_at":"2025-10-30T17:12:58.228266-07:00","closed_at":"2025-10-28T19:08:17.715416-07:00","dependencies":[{"issue_id":"bd-cbed9619.4","depends_on_id":"bd-325da116","type":"parent-child","created_at":"2025-10-28T18:39:20.570276-07:00","created_by":"daemon"},{"issue_id":"bd-cbed9619.4","depends_on_id":"bd-cbed9619.5","type":"blocks","created_at":"2025-10-28T18:39:28.285653-07:00","created_by":"daemon"}]} {"id":"bd-cbed9619.5","content_hash":"12cd30dee3c08ba58d03e4468e6fe261a47d58c3b75397d9f14f38ee644fab6e","title":"Add content-addressable identity to Issue type","description":"## Overview\nPhase 1: Add content hashing to enable global identification of issues regardless of their assigned IDs.\n\n## Current Problem\nThe system identifies issues only by ID (e.g., test-1, test-2). When multiple clones create the same ID with different content, there's no way to identify that these are semantically different issues without comparing all fields.\n\n## Solution\nAdd a ContentHash field to the Issue type that represents the canonical content fingerprint.\n\n## Implementation Tasks\n\n### 1. Add ContentHash field to Issue type\nFile: internal/types/types.go\n```go\ntype Issue struct {\n ID string\n ContentHash string // SHA256 of canonical content\n // ... existing fields\n}\n```\n\n### 2. Add content hash computation method\nUse existing hashIssueContent from collision.go:186 as foundation:\n```go\nfunc (i *Issue) ComputeContentHash() string {\n return hashIssueContent(i)\n}\n```\n\n### 3. Compute hash at creation time\n- Modify CreateIssue to compute and store ContentHash\n- Modify CreateIssues (batch) to compute hashes\n\n### 4. Compute hash at import time \n- Modify ImportIssues to compute ContentHash for all incoming issues\n- Store hash in database\n\n### 5. Add database column\n- Add migration to add content_hash column to issues table\n- Update SELECT/INSERT statements to include content_hash\n- Index on content_hash for fast lookups\n\n### 6. Populate existing issues\n- Add migration step to compute ContentHash for all existing issues\n- Use hashIssueContent function\n\n## Acceptance Criteria\n- Issue type has ContentHash field\n- Hash is computed automatically at creation time\n- Hash is computed for imported issues\n- Database stores content_hash column\n- All existing issues have non-empty ContentHash\n- Hash is deterministic (same content → same hash)\n- Hash excludes ID, timestamps (only semantic content)\n\n## Files to Modify\n- internal/types/types.go\n- internal/storage/sqlite/sqlite.go (schema, CreateIssue, CreateIssues)\n- internal/storage/sqlite/migrations.go (new migration)\n- internal/importer/importer.go (compute hash during import)\n- cmd/bd/create.go (compute hash at creation)\n\n## Testing\n- Unit test: same content produces same hash\n- Unit test: different content produces different hash \n- Unit test: hash excludes ID and timestamps\n- Integration test: hash persists in database\n- Migration test: existing issues get hashes populated","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-28T18:36:44.914967-07:00","updated_at":"2025-10-30T17:12:58.2279-07:00","closed_at":"2025-10-28T18:57:10.985198-07:00","dependencies":[{"issue_id":"bd-cbed9619.5","depends_on_id":"bd-325da116","type":"parent-child","created_at":"2025-10-28T18:39:20.547325-07:00","created_by":"daemon"}]} -{"id":"bd-cdf7","content_hash":"c82db80a2cbf87f83503bbfd8f049594377d6193be5d93c9375a0505deda099c","title":"Add tests for DetectCycles to improve coverage from 29.6%","description":"DetectCycles currently has 29.6% coverage. Need comprehensive tests for:\n- Simple cycles (A-\u003eB-\u003eA)\n- Complex multi-node cycles\n- Acyclic graphs (should not detect cycles)\n- Self-loops\n- Multiple independent cycles\n- Edge cases (empty graph, single node)","status":"open","priority":2,"issue_type":"task","created_at":"2025-11-01T22:40:58.977156-07:00","updated_at":"2025-11-01T22:40:58.977156-07:00"} +{"id":"bd-cdf7","content_hash":"ebe962c7eb6dba6d112f7ccf59a8920e0354ea9cd8b039974a8fc4a58373809b","title":"Add tests for DetectCycles to improve coverage from 29.6%","description":"DetectCycles currently has 29.6% coverage. Need comprehensive tests for:\n- Simple cycles (A-\u003eB-\u003eA)\n- Complex multi-node cycles\n- Acyclic graphs (should not detect cycles)\n- Self-loops\n- Multiple independent cycles\n- Edge cases (empty graph, single node)","status":"in_progress","priority":2,"issue_type":"task","created_at":"2025-11-01T22:40:58.977156-07:00","updated_at":"2025-11-01T22:47:23.173432-07:00"} {"id":"bd-ce37850f","content_hash":"3ef2872c3fcb1e5acc90d33fd5a76291742cbcecfbf697b611aa5b4d8ce80078","title":"Add embedding generation for duplicate detection","description":"Use embeddings for scalable duplicate detection.\n\nModel: text-embedding-3-small (OpenAI) or all-MiniLM-L6-v2 (local)\nStorage: SQLite vector extension or in-memory\nCost: ~/bin/bash.0002 per 100 issues\n\nMuch cheaper than LLM comparisons for large databases.\n\nFiles: internal/embeddings/ (new package)","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-28T14:48:29.072913-07:00","updated_at":"2025-10-30T17:12:58.219921-07:00"} {"id":"bd-cf349eb3","content_hash":"1b42289a0cb1da0626a69c6f004bf62fc9ba6e3a0f8eb70159c5f1446497020b","title":"Update LINTING.md with current baseline","description":"After cleanup, document the remaining acceptable baseline in LINTING.md so we can track regression.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-27T23:20:10.39272-07:00","updated_at":"2025-10-30T17:12:58.215471-07:00","closed_at":"2025-10-27T23:05:31.945614-07:00"} {"id":"bd-d33c","content_hash":"0c3eb277be0ec16edae305156aa8824b6bc9c37fbd6151477f235e859e9b6181","title":"Separate process/lock/PID concerns into process.go","description":"Create internal/daemonrunner/process.go with: acquireDaemonLock, PID file read/write, stopDaemon, isDaemonRunning, getPIDFilePath, socket path helpers, version check.","status":"open","priority":1,"issue_type":"task","created_at":"2025-11-01T11:41:14.871122-07:00","updated_at":"2025-11-01T11:41:14.871122-07:00"} diff --git a/internal/storage/sqlite/cycle_detection_test.go b/internal/storage/sqlite/cycle_detection_test.go new file mode 100644 index 00000000..395ae67f --- /dev/null +++ b/internal/storage/sqlite/cycle_detection_test.go @@ -0,0 +1,506 @@ +package sqlite + +import ( + "context" + "testing" + + "github.com/steveyegge/beads/internal/types" +) + +// TestDetectCyclesSimple tests simple 2-node cycles +func TestDetectCyclesSimple(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create two issues + issue1 := &types.Issue{Title: "Issue 1", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + issue2 := &types.Issue{Title: "Issue 2", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + + if err := store.CreateIssue(ctx, issue1, "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + if err := store.CreateIssue(ctx, issue2, "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + // Manually create a cycle by inserting directly into dependencies table + // (bypassing AddDependency's cycle prevention) + _, err := store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issue1.ID, issue2.ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + + _, err = store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issue2.ID, issue1.ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + + // Detect cycles + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) == 0 { + t.Fatal("Expected to detect a cycle, but found none") + } + + // Verify the cycle contains both issues + cycle := cycles[0] + if len(cycle) != 2 { + t.Logf("Cycle issues: %v", cycle) + for i, iss := range cycle { + t.Logf(" [%d] ID=%s Title=%s", i, iss.ID, iss.Title) + } + t.Errorf("Expected cycle of length 2, got %d", len(cycle)) + } + + // Verify both issues are in the cycle + foundIDs := make(map[string]bool) + for _, issue := range cycle { + foundIDs[issue.ID] = true + } + + if !foundIDs[issue1.ID] || !foundIDs[issue2.ID] { + t.Errorf("Cycle missing expected issues. Got: %v", foundIDs) + } +} + +// TestDetectCyclesComplex tests a more complex multi-node cycle +func TestDetectCyclesComplex(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create a 4-node cycle: A → B → C → D → A + issues := make([]*types.Issue, 4) + for i := 0; i < 4; i++ { + issues[i] = &types.Issue{ + Title: "Issue " + string(rune('A'+i)), + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, issues[i], "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + } + + // Create cycle: 0→1→2→3→0 + for i := 0; i < 4; i++ { + nextIdx := (i + 1) % 4 + _, err := store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issues[i].ID, issues[nextIdx].ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + } + + // Detect cycles + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) == 0 { + t.Fatal("Expected to detect a cycle, but found none") + } + + // Verify the cycle contains all 4 issues + cycle := cycles[0] + if len(cycle) != 4 { + t.Errorf("Expected cycle of length 4, got %d", len(cycle)) + } + + // Verify all issues are in the cycle + foundIDs := make(map[string]bool) + for _, issue := range cycle { + foundIDs[issue.ID] = true + } + + for _, issue := range issues { + if !foundIDs[issue.ID] { + t.Errorf("Cycle missing issue %s", issue.ID) + } + } +} + +// TestDetectCyclesSelfLoop tests detection of self-loops (A → A) +func TestDetectCyclesSelfLoop(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + issue := &types.Issue{Title: "Self Loop", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + if err := store.CreateIssue(ctx, issue, "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + // Create self-loop + _, err := store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issue.ID, issue.ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + + // Detect cycles + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) == 0 { + t.Fatal("Expected to detect a self-loop cycle, but found none") + } + + // Verify the cycle contains the issue + cycle := cycles[0] + if len(cycle) != 1 { + t.Errorf("Expected self-loop cycle of length 1, got %d", len(cycle)) + } + + if cycle[0].ID != issue.ID { + t.Errorf("Expected cycle to contain issue %s, got %s", issue.ID, cycle[0].ID) + } +} + +// TestDetectCyclesMultipleIndependent tests detection of multiple independent cycles +func TestDetectCyclesMultipleIndependent(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create two independent cycles: + // Cycle 1: A → B → A + // Cycle 2: C → D → C + + cycle1 := make([]*types.Issue, 2) + cycle2 := make([]*types.Issue, 2) + + for i := 0; i < 2; i++ { + cycle1[i] = &types.Issue{ + Title: "Cycle1-" + string(rune('A'+i)), + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + cycle2[i] = &types.Issue{ + Title: "Cycle2-" + string(rune('A'+i)), + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, cycle1[i], "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + if err := store.CreateIssue(ctx, cycle2[i], "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + } + + // Create first cycle: 0→1→0 + _, err := store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, cycle1[0].ID, cycle1[1].ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + _, err = store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, cycle1[1].ID, cycle1[0].ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + + // Create second cycle: 0→1→0 + _, err = store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, cycle2[0].ID, cycle2[1].ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + _, err = store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, cycle2[1].ID, cycle2[0].ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + + // Detect cycles + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + // The SQL may detect the same cycle from different entry points, + // so we might get more than 2 cycles reported. Verify we have at least 2. + if len(cycles) < 2 { + t.Errorf("Expected to detect at least 2 independent cycles, got %d", len(cycles)) + } + + // Verify we found cycles involving all 4 issues + foundIssues := make(map[string]bool) + for _, cycle := range cycles { + for _, issue := range cycle { + foundIssues[issue.ID] = true + } + } + + allCycleIssues := append(cycle1, cycle2...) + for _, issue := range allCycleIssues { + if !foundIssues[issue.ID] { + t.Errorf("Cycle detection missing issue %s", issue.ID) + } + } +} + +// TestDetectCyclesAcyclicGraph tests that no cycles are detected in an acyclic graph +func TestDetectCyclesAcyclicGraph(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create a DAG: A → B → C → D (no cycles) + issues := make([]*types.Issue, 4) + for i := 0; i < 4; i++ { + issues[i] = &types.Issue{ + Title: "Issue " + string(rune('A'+i)), + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, issues[i], "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + } + + // Create chain: 0→1→2→3 (no cycle) + for i := 0; i < 3; i++ { + _, err := store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issues[i].ID, issues[i+1].ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + } + + // Detect cycles + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) != 0 { + t.Errorf("Expected no cycles in acyclic graph, but found %d", len(cycles)) + } +} + +// TestDetectCyclesEmptyGraph tests cycle detection on empty graph +func TestDetectCyclesEmptyGraph(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Detect cycles on empty database + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) != 0 { + t.Errorf("Expected no cycles in empty graph, but found %d", len(cycles)) + } +} + +// TestDetectCyclesSingleNode tests cycle detection with a single node (no dependencies) +func TestDetectCyclesSingleNode(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create a single issue with no dependencies + issue := &types.Issue{Title: "Lonely Issue", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask} + if err := store.CreateIssue(ctx, issue, "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + + // Detect cycles + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) != 0 { + t.Errorf("Expected no cycles for single node with no dependencies, but found %d", len(cycles)) + } +} + +// TestDetectCyclesDiamond tests cycle detection in a diamond pattern (no cycle) +func TestDetectCyclesDiamond(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create a diamond pattern: A → B → D, A → C → D (no cycle) + issues := make([]*types.Issue, 4) + names := []string{"A", "B", "C", "D"} + for i := 0; i < 4; i++ { + issues[i] = &types.Issue{ + Title: "Issue " + names[i], + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, issues[i], "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + } + + // Create dependencies: A→B, A→C, B→D, C→D + deps := [][2]int{{0, 1}, {0, 2}, {1, 3}, {2, 3}} + for _, dep := range deps { + _, err := store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issues[dep[0]].ID, issues[dep[1]].ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + } + + // Detect cycles + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) != 0 { + t.Errorf("Expected no cycles in diamond pattern, but found %d", len(cycles)) + } +} + +// TestDetectCyclesLongCycle tests detection of a long cycle (10 nodes) +func TestDetectCyclesLongCycle(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create a 10-node cycle + const cycleLength = 10 + issues := make([]*types.Issue, cycleLength) + for i := 0; i < cycleLength; i++ { + issues[i] = &types.Issue{ + Title: "Issue " + string(rune('0'+i)), + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, issues[i], "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + } + + // Create cycle: 0→1→2→...→9→0 + for i := 0; i < cycleLength; i++ { + nextIdx := (i + 1) % cycleLength + _, err := store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issues[i].ID, issues[nextIdx].ID, types.DepBlocks) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + } + + // Detect cycles + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) == 0 { + t.Fatal("Expected to detect a cycle, but found none") + } + + // Verify the cycle contains all 10 issues + cycle := cycles[0] + if len(cycle) != cycleLength { + t.Errorf("Expected cycle of length %d, got %d", cycleLength, len(cycle)) + } +} + +// TestDetectCyclesMixedTypes tests cycle detection with different dependency types +func TestDetectCyclesMixedTypes(t *testing.T) { + store, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + + // Create a cycle using different dependency types + issues := make([]*types.Issue, 3) + for i := 0; i < 3; i++ { + issues[i] = &types.Issue{ + Title: "Issue " + string(rune('A'+i)), + Status: types.StatusOpen, + Priority: 1, + IssueType: types.TypeTask, + } + if err := store.CreateIssue(ctx, issues[i], "test-user"); err != nil { + t.Fatalf("CreateIssue failed: %v", err) + } + } + + // Create cycle with mixed types: A -blocks-> B -related-> C -parent-child-> A + depTypes := []types.DependencyType{types.DepBlocks, types.DepRelated, types.DepParentChild} + for i := 0; i < 3; i++ { + nextIdx := (i + 1) % 3 + _, err := store.db.ExecContext(ctx, ` + INSERT INTO dependencies (issue_id, depends_on_id, type, created_by, created_at) + VALUES (?, ?, ?, 'test-user', CURRENT_TIMESTAMP) + `, issues[i].ID, issues[nextIdx].ID, depTypes[i]) + if err != nil { + t.Fatalf("Insert dependency failed: %v", err) + } + } + + // Detect cycles + cycles, err := store.DetectCycles(ctx) + if err != nil { + t.Fatalf("DetectCycles failed: %v", err) + } + + if len(cycles) == 0 { + t.Fatal("Expected to detect a cycle with mixed types, but found none") + } + + // Verify the cycle contains all 3 issues + cycle := cycles[0] + if len(cycle) != 3 { + t.Errorf("Expected cycle of length 3, got %d", len(cycle)) + } +} diff --git a/internal/storage/sqlite/dependencies.go b/internal/storage/sqlite/dependencies.go index a6336326..8aa66076 100644 --- a/internal/storage/sqlite/dependencies.go +++ b/internal/storage/sqlite/dependencies.go @@ -560,17 +560,17 @@ func (s *SQLiteStorage) DetectCycles(ctx context.Context) ([][]*types.Issue, err UNION ALL SELECT - d.issue_id, - d.depends_on_id, - p.start_id, - p.path || '→' || d.depends_on_id, - p.depth + 1 + d.issue_id, + d.depends_on_id, + p.start_id, + p.path || '→' || d.depends_on_id, + p.depth + 1 FROM dependencies d JOIN paths p ON d.issue_id = p.depends_on_id WHERE p.depth < ? - AND p.path NOT LIKE '%' || d.depends_on_id || '→%' + AND (d.depends_on_id = p.start_id OR p.path NOT LIKE '%' || d.depends_on_id || '→%') ) - SELECT DISTINCT path || '→' || start_id as cycle_path + SELECT DISTINCT path as cycle_path FROM paths WHERE depends_on_id = start_id ORDER BY cycle_path