From 9225114c0b3ee7e04e055a7fbc44273b5c6e18aa Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Fri, 31 Oct 2025 01:25:05 -0700 Subject: [PATCH] Fix tests after --resolve-collisions removal Remove obsolete test file and update remaining tests to not use the removed --resolve-collisions flag. Hash-based IDs make collision resolution unnecessary. Changes: - Delete internal/rpc/server_autoimport_test.go (obsolete) - Remove --resolve-collisions from beads_nway_test.go - Remove --resolve-collisions from beads_twoclone_test.go - Remove --resolve-collisions from cmd/bd/sync.go All tests now pass. --- .beads/beads.jsonl | 1 + beads_nway_test.go | 7 ++-- beads_twoclone_test.go | 6 +-- cmd/bd/sync.go | 4 +- internal/rpc/server_autoimport_test.go | 56 -------------------------- 5 files changed, 9 insertions(+), 65 deletions(-) delete mode 100644 internal/rpc/server_autoimport_test.go diff --git a/.beads/beads.jsonl b/.beads/beads.jsonl index 4674cda3..af872365 100644 --- a/.beads/beads.jsonl +++ b/.beads/beads.jsonl @@ -33,6 +33,7 @@ {"id":"bd-3240cd79","content_hash":"3e8b654c9dd6a631db40adc23bf6b7e291c13f977dceaf08d447963f99b3124d","title":"Another hash test","description":"","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-30T14:27:28.658778-07:00","updated_at":"2025-10-30T17:05:26.02898-07:00","closed_at":"2025-10-30T14:27:33.073977-07:00"} {"id":"bd-325da116","content_hash":"d7c5637527778c5c835f5e4b6e15fbd51a3476d6749ab3155b8aeac08a8ef339","title":"Fix N-way collision convergence","description":"Epic to fix the N-way collision convergence problem documented in n-way-collision-convergence.md.\n\n## Problem Summary\nThe current collision resolution implementation works correctly for 2-way collisions but does not converge for 3-way (and by extension N-way) collisions. TestThreeCloneCollision demonstrates this with reproducible failures.\n\n## Root Causes Identified\n1. Pairwise resolution doesn't scale - each clone makes local decisions without global context\n2. DetectCollisions modifies state during detection (line 83-86 in collision.go)\n3. No remapping history - can't track transitive remap chains (test-1 → test-2 → test-3)\n4. Import-time resolution is too late - happens after git merge\n\n## Solution Architecture\nReplace pairwise resolution with deterministic global N-way resolution using:\n- Content-addressable identity (content hashing)\n- Global collision resolution (sort all versions by hash)\n- Read-only detection phase (separate from modification)\n- Idempotent imports (content-first matching)\n\n## Success Criteria\n- TestThreeCloneCollision passes without skipping\n- All clones converge to identical content after final pull\n- No data loss (all issues present in all clones)\n- Works for N workers (test with 5+ clones)\n- Idempotent imports (importing same JSONL multiple times is safe)\n\n## Implementation Phases\nSee child issues for detailed breakdown of each phase.","status":"closed","priority":0,"issue_type":"epic","created_at":"2025-10-29T23:05:13.889079-07:00","updated_at":"2025-10-30T20:02:31.539548-07:00","closed_at":"2025-10-30T20:02:31.539548-07:00"} {"id":"bd-3262e055","content_hash":"0c150383d4c2ce7aeecf70fe53f2599e9720eccffc7ab717a2abfef8e37f9dcc","title":"Deep nested","description":"","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-30T15:46:53.570315-07:00","updated_at":"2025-10-30T17:05:26.031513-07:00","closed_at":"2025-10-30T15:46:59.619236-07:00"} +{"id":"bd-34e79e","content_hash":"29d56b87e8a862dfff904eea51d5893fd757989229b9b40dd1acaa8c8cd93748","title":"Rebase and integrate PR #161: Mermaid.js dependency tree visualization","description":"PR #161 by @mrdavidlaing adds `bd dep tree --format mermaid` for visualizing dependency trees as Mermaid.js diagrams.\n\n**Status:** 186 commits behind main (from Oct 27), JSONL merge conflict, all CI checks failed\n\n**What needs doing:**\n1. Rebase onto current main (handle JSONL conflict - our database is wholly different now)\n2. Review code changes - may need reimplementation due to storage layer changes\n3. Update tests to work with current codebase\n4. Ensure mermaid output logic still valid\n5. Get CI passing\n6. Merge\n\n**Original features (worth preserving):**\n- Status indicators: ☐ open, ◧ in_progress, ⚠ blocked, ☑ closed\n- Theme-agnostic design\n- Works with --reverse flag\n- Well-tested with TDD\n- Clean implementation per steveyegge review","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-31T01:16:58.185175-07:00","updated_at":"2025-10-31T01:16:58.185175-07:00","external_ref":"github:161"} {"id":"bd-36320a04","content_hash":"883eb385fa9eded3826008fa6db3b842cabb2ce0e93a23293449f65024303fb7","title":"Add mutation channel to internal/rpc/server.go","description":"Add mutationChan chan MutationEvent to Server struct. Emit events on CreateIssue, UpdateIssue, DeleteIssue, AddComment. Non-blocking send with default case for full channel.","status":"open","priority":1,"issue_type":"task","created_at":"2025-10-29T19:42:29.860173-07:00","updated_at":"2025-10-30T17:05:26.040943-07:00"} {"id":"bd-36870264","content_hash":"e88e5d98a2a5bebc38b3ac505b00687bfe78bd72654bd0c756bceee4a01e15f5","title":"Enforce daemon singleton per workspace with file locking","description":"Agent in ~/src/wyvern discovered 4 simultaneous daemon processes running, causing infinite directory recursion (.beads/.beads/.beads/...). Each daemon used relative paths and created nested .beads/ directories.\n\nRoot cause: No singleton enforcement. Multiple `bd daemon` processes can start in same workspace.\n\nExpected: One daemon per workspace (each workspace = separate .beads/ dir with bd.sock)\nActual: Multiple daemons can run simultaneously in same workspace\n\nNote: Separate git clones = separate workspaces = separate daemons (correct). Git worktrees share .beads/ and have known limitations (documented, use --no-daemon).","design":"Use flock (file locking) on daemon socket or database file to enforce singleton:\n\n1. On daemon start, attempt exclusive lock on .beads/bd.sock or .beads/daemon.lock\n2. If lock held by another process, refuse to start (exit with clear error)\n3. Hold lock for lifetime of daemon process\n4. Release lock on daemon shutdown\n\nAlternative: Use PID file with stale detection (check if PID is still running)\n\nImplementation location: Daemon startup code in cmd/bd/ or internal/daemon/","acceptance_criteria":"1. Starting second daemon process in same workspace fails with clear error\n2. Test: Start daemon, attempt second start, verify failure\n3. Killing daemon releases lock, allowing new daemon to start\n4. No infinite .beads/ directory recursion possible\n5. Works correctly with auto-start mechanism","status":"in_progress","priority":0,"issue_type":"bug","created_at":"2025-10-25T23:13:12.269549-07:00","updated_at":"2025-10-30T17:05:26.038883-07:00"} {"id":"bd-37f7","content_hash":"72cb1f58d7ad6a91d3a02233bf4e78d0992834c15fbd8c6d9dc56df7095dc0a3","title":"Remove or archive collision resolution documentation","description":"Clean up documentation that's specific to sequential ID collision resolution:\n- docs/collision-resolution-failure-analysis.md (can archive or remove)\n- Remove collision resolution sections from AGENTS.md, FAQ.md, TROUBLESHOOTING.md\n- Update HASH_ID_DESIGN.md to reflect that collision resolution is fully removed\n- Update CHANGELOG.md to note the removal\n- Keep only minimal docs about hash ID accidental collisions","status":"open","priority":2,"issue_type":"chore","created_at":"2025-10-30T22:16:48.499529-07:00","updated_at":"2025-10-30T22:16:48.499529-07:00","labels":["cleanup","documentation","hash-ids"]} diff --git a/beads_nway_test.go b/beads_nway_test.go index 31cd55f4..fbd1c744 100644 --- a/beads_nway_test.go +++ b/beads_nway_test.go @@ -191,8 +191,8 @@ func syncCloneWithConflictResolution(t *testing.T, cloneDir, name string, isFirs runCmd(t, cloneDir, "git", "commit", "-m", "Resolve merge conflict") } - // Import with collision resolution - runCmdWithEnv(t, cloneDir, map[string]string{"BEADS_NO_DAEMON": "1"}, "./bd", "import", "-i", ".beads/issues.jsonl", "--resolve-collisions") + // Import (no collision resolution needed with hash IDs) + runCmdWithEnv(t, cloneDir, map[string]string{"BEADS_NO_DAEMON": "1"}, "./bd", "import", "-i", ".beads/issues.jsonl") runCmd(t, cloneDir, "git", "push", "origin", "master") } } @@ -216,8 +216,7 @@ func finalPullForClone(t *testing.T, cloneDir, name string) { } // Import JSONL to update database - // Use --resolve-collisions to handle any remaining ID conflicts - runCmdOutputWithEnvAllowError(t, cloneDir, map[string]string{"BEADS_NO_DAEMON": "1"}, true, "./bd", "import", "-i", ".beads/issues.jsonl", "--resolve-collisions") + runCmdOutputWithEnvAllowError(t, cloneDir, map[string]string{"BEADS_NO_DAEMON": "1"}, true, "./bd", "import", "-i", ".beads/issues.jsonl") } // getTitlesFromClone extracts all issue titles from a clone's database diff --git a/beads_twoclone_test.go b/beads_twoclone_test.go index 0578cad7..9689636b 100644 --- a/beads_twoclone_test.go +++ b/beads_twoclone_test.go @@ -551,7 +551,7 @@ func testThreeCloneCollisionWithSyncOrder(t *testing.T, first, second, third str } // Import with collision resolution - runCmd(t, secondDir, "./bd", "import", "-i", ".beads/issues.jsonl", "--resolve-collisions") + runCmd(t, secondDir, "./bd", "import", "-i", ".beads/issues.jsonl") runCmd(t, secondDir, "git", "push", "origin", "master") _ = pullOut } @@ -590,7 +590,7 @@ func testThreeCloneCollisionWithSyncOrder(t *testing.T, first, second, third str } // Import with collision resolution - runCmd(t, thirdDir, "./bd", "import", "-i", ".beads/issues.jsonl", "--resolve-collisions") + runCmd(t, thirdDir, "./bd", "import", "-i", ".beads/issues.jsonl") runCmd(t, thirdDir, "git", "push", "origin", "master") _ = pullOut } @@ -623,7 +623,7 @@ func testThreeCloneCollisionWithSyncOrder(t *testing.T, first, second, third str } } - // Import JSONL to update database (but don't use --resolve-collisions to avoid creating duplicates) + // Import JSONL to update database runCmdOutputAllowError(t, clone, "./bd", "import", "-i", ".beads/issues.jsonl") } diff --git a/cmd/bd/sync.go b/cmd/bd/sync.go index a5be4d31..9a1bf865 100644 --- a/cmd/bd/sync.go +++ b/cmd/bd/sync.go @@ -483,12 +483,12 @@ func importFromJSONL(ctx context.Context, jsonlPath string, renameOnImport bool) } // Build args for import command - args := []string{"import", "-i", jsonlPath, "--resolve-collisions"} + args := []string{"import", "-i", jsonlPath} if renameOnImport { args = append(args, "--rename-on-import") } - // Run import command with --resolve-collisions to automatically handle conflicts + // Run import command cmd := exec.CommandContext(ctx, exe, args...) // #nosec G204 - bd import command from trusted binary output, err := cmd.CombinedOutput() if err != nil { diff --git a/internal/rpc/server_autoimport_test.go b/internal/rpc/server_autoimport_test.go deleted file mode 100644 index e9ef7788..00000000 --- a/internal/rpc/server_autoimport_test.go +++ /dev/null @@ -1,56 +0,0 @@ -package rpc - -import ( - "testing" - - "github.com/steveyegge/beads/internal/importer" -) - -// TestAutoImportDoesNotUseResolveCollisions ensures auto-import NEVER uses -// ResolveCollisions flag. That flag is ONLY for explicit user-driven imports -// (bd import --resolve-collisions) when merging branches. -// -// Auto-import should update existing issues by ID, not create duplicates. -// Using ResolveCollisions in auto-import causes catastrophic duplicate creation -// where every git pull creates new duplicate issues, leading to endless ping-pong -// between agents (bd-247). -// -// This test enforces that checkAndAutoImportIfStale uses ResolveCollisions: false. -func TestAutoImportDoesNotUseResolveCollisions(t *testing.T) { - // This is a compile-time and code inspection test. - // We verify the Options struct used in checkAndAutoImportIfStale. - - // The correct options for auto-import - correctOpts := importer.Options{ - ResolveCollisions: false, // MUST be false for auto-import - RenameOnImport: true, // Safe: handles prefix mismatches - // SkipPrefixValidation is false by default - } - - // Verify ResolveCollisions is false - if correctOpts.ResolveCollisions { - t.Fatal("Auto-import MUST NOT use ResolveCollisions=true. This causes duplicate creation (bd-247).") - } - - // This test will fail if someone changes the auto-import code to use ResolveCollisions. - // To fix this test, you need to: - // 1. Change server_export_import_auto.go line ~221 to ResolveCollisions: false - // 2. Add a comment explaining why it must be false - // 3. Update AGENTS.md to document the auto-import behavior -} - -// TestResolveCollisionsOnlyForExplicitImport documents when ResolveCollisions -// should be used: ONLY for explicit user-driven imports from CLI. -func TestResolveCollisionsOnlyForExplicitImport(t *testing.T) { - t.Log("ResolveCollisions should ONLY be used for:") - t.Log(" - bd import --resolve-collisions (user explicitly requested)") - t.Log(" - Branch merge scenarios (different issues with same ID)") - t.Log("") - t.Log("ResolveCollisions should NEVER be used for:") - t.Log(" - Auto-import after git pull (daemon/auto-sync)") - t.Log(" - Background JSONL updates") - t.Log(" - Normal agent synchronization") - t.Log("") - t.Log("Violation causes: Endless duplicate creation, database pollution, ping-pong commits") - t.Log("See: bd-247 for catastrophic failure caused by this bug") -}