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

Fix bd-191: bd sync --dry-run should not modify database

Browse Source 
Skip auto-import when sync command is run with --dry-run flag to prevent
database modifications during dry-run mode. Previously, autoImportIfNewer()
would run in PersistentPreRun hook and modify the database even in dry-run,
causing the JSONL file to become dirty.

Amp-Thread-ID: https://ampcode.com/threads/T-1bc29344-0c59-4127-855d-860d1579ba0b
Co-authored-by: Amp <amp@ampcode.com>
This commit is contained in:
Steve Yegge
2025-10-27 18:40:58 -07:00
parent 897edc6ce7
commit 6b5d26d7d1
2 changed files with 26 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
.beads/beads.jsonl
View File

@@ -76,9 +76,21 @@
{"id":"bd-167","title":"Final validation test","description":"","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-27T18:19:58.016545-07:00","updated_at":"2025-10-27T18:19:58.016545-07:00"}
{"id":"bd-17","title":"Update EXTENDING.md with UnderlyingDB() usage and best practices","description":"EXTENDING.md currently shows how to use direct sql.Open() to access the database, but doesn't mention the new UnderlyingDB() method that's the recommended way for extensions.\n\n**Update needed:**\n1. Add section showing UnderlyingDB() usage:\n ```go\n store, err := beads.NewSQLiteStorage(dbPath)\n db := store.UnderlyingDB()\n // Create extension tables using db\n ```\n\n2. Document when to use UnderlyingDB() vs direct sql.Open():\n - Use UnderlyingDB() when you want to share the storage connection\n - Use sql.Open() when you need independent connection management\n\n3. Add safety warnings (cross-reference from UnderlyingDB() docs):\n - Don't close the DB\n - Don't modify pool settings\n - Keep transactions short\n\n4. Update the VC example to show UnderlyingDB() pattern\n\n5. Explain beads.Storage.UnderlyingDB() in the API section","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-22T17:07:56.820056-07:00","updated_at":"2025-10-25T23:15:33.478579-07:00","closed_at":"2025-10-22T19:41:19.895847-07:00","dependencies":[{"issue_id":"bd-17","depends_on_id":"bd-10","type":"discovered-from","created_at":"2025-10-24T13:17:40.32522-07:00","created_by":"renumber"}]}
{"id":"bd-18","title":"Consider adding UnderlyingConn(ctx) for safer scoped DB access","description":"Currently UnderlyingDB() returns *sql.DB which is correct for most uses, but for extension migrations/DDL, a scoped connection might be safer.\n\n**Proposal:** Add optional UnderlyingConn(ctx) (*sql.Conn, error) method that:\n- Returns a scoped connection via s.db.Conn(ctx)\n- Encourages lifetime-bounded usage\n- Reduces temptation to tune global pool settings\n- Better for one-time DDL operations like CREATE TABLE\n\n**Implementation:**\n```go\n// UnderlyingConn returns a single connection from the pool for scoped use\n// Useful for migrations and DDL. Close the connection when done.\nfunc (s *SQLiteStorage) UnderlyingConn(ctx context.Context) (*sql.Conn, error) {\n return s.db.Conn(ctx)\n}\n```\n\n**Benefits:**\n- Safer for migrations (explicit scope)\n- Complements UnderlyingDB() for different use cases\n- Low implementation cost\n\n**Trade-off:** Adds another method to maintain, but Oracle considers this balanced compromise between safety and flexibility.\n\n**Decision:** This is optional - evaluate based on VC's actual usage patterns.","status":"closed","priority":3,"issue_type":"feature","created_at":"2025-10-22T17:07:56.832638-07:00","updated_at":"2025-10-25T23:15:33.479496-07:00","closed_at":"2025-10-22T22:02:18.479512-07:00","dependencies":[{"issue_id":"bd-18","depends_on_id":"bd-10","type":"related","created_at":"2025-10-24T13:17:40.325463-07:00","created_by":"renumber"}]}
{"id":"bd-188","title":"Add migration tooling for database upgrades","description":"Create bd migrate command and auto-migration logic for version upgrades.\n\nImplementation:\n- bd migrate command (or bd init --migrate)\n- Auto-run on first command after daemon version upgrade\n- Detection logic:\n - Find all .db files in .beads/\n - Check schema version in each\n - Prompt to migrate/rename/delete\n- Migration operations:\n - Rename old database to beads.db\n - Update schema version metadata\n - Remove stale databases (with confirmation)\n- Could be part of daemon auto-start logic\n\nUser experience:\n$ bd ready\nDatabase schema mismatch detected.\n Found: vc.db (schema v0.16.0)\n Expected: beads.db (schema v0.17.5)\n \nRun 'bd migrate' to migrate automatically.\n\nBenefits:\n- Smooth upgrade path\n- Prevents confusion on version changes\n- Clean up stale databases\n\nDepends on:\n- Canonical naming (bd-160)\n- Schema versioning (bd-198)","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-27T18:27:28.312163-07:00","updated_at":"2025-10-27T18:34:51.565471-07:00","closed_at":"2025-10-26T19:04:02.023089-07:00"}
{"id":"bd-19","title":"MCP close tool method signature error - takes 1 positional argument but 2 were given","description":"The close approval routing fix in beads-mcp v0.11.0 works correctly and successfully routes update(status=\"closed\") calls to close() tool. However, the close() tool has a Python method signature bug that prevents execution.\n\nImpact: All MCP-based close operations are broken. Workaround: Use bd CLI directly.\n\nError: BdDaemonClient.close() takes 1 positional argument but 2 were given\n\nRoot cause: BdDaemonClient.close() only accepts self, but MCP tool passes issue_id and reason.\n\nAdditional issue: CLI close has FOREIGN KEY constraint error when recording reason parameter.\n\nSee GitHub issue #107 for full details.","status":"closed","priority":0,"issue_type":"bug","created_at":"2025-10-22T17:25:34.67056-07:00","updated_at":"2025-10-25T23:15:33.480292-07:00","closed_at":"2025-10-22T17:36:55.463445-07:00"}
{"id":"bd-191","title":"bd sync --dry-run should not modify database during auto-import collision resolution","description":"When running 'bd sync --dry-run', the auto-import step detects colliding issues and remaps them in the database, then exports the modified DB to JSONL. This makes the JSONL file dirty and modifies the database state, which violates the dry-run contract.\n\nExpected: Dry-run should only preview what would happen without making any changes to the database or JSONL.\n\nActual: Auto-import collision resolution runs and modifies the DB, causing the JSONL to become dirty after a supposedly no-op dry-run.\n\nReproduction:\n1. Have colliding issues between DB and JSONL (e.g., after git pull)\n2. Run 'bd sync --dry-run'\n3. Observe that .beads/beads.jsonl becomes modified\n4. Run 'bd sync --dry-run' again - no collisions found (already resolved)\n\nThe dry-run flag needs to be propagated to the auto-import step so it previews collision resolution without applying it.","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-10-27T18:32:48.522521-07:00","updated_at":"2025-10-27T18:36:50.950009-07:00","closed_at":"2025-10-27T18:36:50.950009-07:00"}
{"id":"bd-192","title":"Test database naming","description":"","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-27T18:34:51.543439-07:00","updated_at":"2025-10-27T18:34:51.543439-07:00"}
{"id":"bd-193","title":"Final validation test","description":"","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-27T18:34:51.543831-07:00","updated_at":"2025-10-27T18:34:51.543831-07:00"}
{"id":"bd-194","title":"Update AGENTS.md and README.md with \"bd daemons\" documentation","description":"Document the new \"bd daemons\" command and all subcommands in AGENTS.md and README.md. Include examples and troubleshooting guidance.","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-27T18:34:51.544152-07:00","updated_at":"2025-10-27T18:34:51.544152-07:00"}
{"id":"bd-195","title":"Implement \"bd daemons logs\" subcommand","description":"Add command to view daemon logs for a specific workspace. Requires daemon logging to file (may need separate issue for log infrastructure).","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-27T18:34:51.54455-07:00","updated_at":"2025-10-27T18:34:51.54455-07:00"}
{"id":"bd-196","title":"Enforce canonical database name (beads.db)","description":"Always use beads.db as the canonical database name. Never auto-detect from multiple .db files.\n\nImplementation:\n- bd init always creates/uses beads.db\n- bd init detects and migrates old databases (vc.db → beads.db, bd.db → beads.db)\n- Daemon refuses to start if multiple .db files exist in .beads/ (exit with ambiguity error)\n- Update database discovery logic to prefer beads.db, error on ambiguity\n\nBenefits:\n- Prevents accidental use of stale databases\n- Clear single source of truth\n- Migration path for existing users","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-27T18:34:51.544928-07:00","updated_at":"2025-10-27T18:34:51.544928-07:00","closed_at":"2025-10-26T18:16:56.23769-07:00"}
{"id":"bd-197","title":"Add .beads/config.json for database path configuration","description":"Create config file to eliminate ambiguity about which database is active.\n\nConfig file format (.beads/config.json):\n{\n \"database\": \"beads.db\",\n \"version\": \"0.17.5\",\n \"jsonl_export\": \"beads.jsonl\" // Allow user to rename\n}\n\nImplementation:\n- bd init creates config.json\n- Daemon and clients read config first (single source of truth)\n- Fall back to beads.db if config missing (backward compat)\n- bd init --jsonl-name allows customizing export filename\n- Gitignore: do NOT ignore config.json (part of repo state)\n\nBenefits:\n- Explicit configuration over convention\n- Allows JSONL renaming for git history hygiene\n- Single source of truth for file paths","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-27T18:34:51.545261-07:00","updated_at":"2025-10-27T18:34:51.545261-07:00","closed_at":"2025-10-26T18:44:16.133085-07:00"}
{"id":"bd-198","title":"Stricter daemon lock file validation","description":"Enhance daemon.lock to include database path and version, validate on client connection.\n\nCurrent: daemon.lock has PID\nProposed: JSON format with database path and version\n\nLock file format:\n{\n \"pid\": 12345,\n \"database\": \"/full/path/to/.beads/beads.db\",\n \"version\": \"0.17.5\",\n \"started_at\": \"2025-10-26T18:00:00Z\"\n}\n\nImplementation:\n- Daemon writes enhanced lock on startup\n- Client reads lock and validates:\n - Database path matches expected\n - Version compatible\n - Fail hard (not just warn) on mismatch\n- Update existing lock validation code (already partially implemented)\n\nBenefits:\n- Catch daemon/database mismatches early\n- Better error messages\n- More robust multi-workspace scenarios","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-27T18:34:51.545623-07:00","updated_at":"2025-10-27T18:34:51.545623-07:00","closed_at":"2025-10-26T18:36:09.975648-07:00"}
{"id":"bd-199","title":"Add migration tooling for database upgrades","description":"Create bd migrate command and auto-migration logic for version upgrades.\n\nImplementation:\n- bd migrate command (or bd init --migrate)\n- Auto-run on first command after daemon version upgrade\n- Detection logic:\n - Find all .db files in .beads/\n - Check schema version in each\n - Prompt to migrate/rename/delete\n- Migration operations:\n - Rename old database to beads.db\n - Update schema version metadata\n - Remove stale databases (with confirmation)\n- Could be part of daemon auto-start logic\n\nUser experience:\n$ bd ready\nDatabase schema mismatch detected.\n Found: vc.db (schema v0.16.0)\n Expected: beads.db (schema v0.17.5)\n \nRun 'bd migrate' to migrate automatically.\n\nBenefits:\n- Smooth upgrade path\n- Prevents confusion on version changes\n- Clean up stale databases\n\nDepends on:\n- Canonical naming (bd-160)\n- Schema versioning (bd-161)","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-27T18:34:51.546-07:00","updated_at":"2025-10-27T18:34:51.576772-07:00","closed_at":"2025-10-26T19:04:02.023089-07:00"}
{"id":"bd-2","title":"Improve error handling in dependency removal during remapping","description":"In updateDependencyReferences(), RemoveDependency errors are caught and ignored with continue (line 392). Comment says 'if dependency doesn't exist' but this catches ALL errors including real failures. Should check error type with errors.Is(err, ErrDependencyNotFound) and only ignore not-found errors, returning other errors properly.","status":"closed","priority":3,"issue_type":"bug","created_at":"2025-10-21T23:53:44.31362-07:00","updated_at":"2025-10-25T23:15:33.462194-07:00","closed_at":"2025-10-18T09:41:18.209717-07:00"}
{"id":"bd-20","title":"Fix pre-existing MCP test failures - show/update return arrays not dicts","description":"9 tests fail in beads-mcp because bd CLI commands return arrays but MCP client expects dicts:\n\nFailing tests:\n- test_create_and_show_issue: show returns array, expects dict\n- test_update_issue: update returns array, expects dict \n- test_add_dependency: show returns array, expects dict\n- test_invalid_issue_id: show returns empty dict instead of error\n- test_dependency_types: show returns array, expects dict\n- test_show_issue_tool: show returns array, expects dict\n- test_update_issue_tool: update returns array, expects dict\n- test_update_partial_fields: update returns array, expects dict\n- test_client_lazy_initialization: BdClient import issue\n\nRoot cause: bd CLI commands like 'bd show' and 'bd update' output JSON arrays, but BdCliClient.show() and BdCliClient.update() expect single dict objects.\n\nExample:\n```bash\nbd show test-1 --json\n[{\"id\":\"test-1\",...}] # Array, not dict\n```\n\nFix needed: Update bd_client.py to handle array responses and extract first element, or change CLI to return single object for single-ID operations.","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-10-22T17:43:23.29302-07:00","updated_at":"2025-10-25T23:15:33.481154-07:00","closed_at":"2025-10-22T20:05:49.3826-07:00"}
{"id":"bd-200","title":"Improve database naming and version management robustness","description":"Make beads architecture more robust to prevent issues like accidental vc.db usage after version upgrades.\n\nKey improvements:\n\n1. **Canonical database name enforcement**\n - Always use beads.db, never auto-detect from multiple .db files\n - bd init migrates/renames any old databases (vc.db → beads.db, bd.db → beads.db)\n - Daemon refuses to start if multiple .db files exist (ambiguity error)\n\n2. **Database schema versioning**\n - Store beads version in SQLite (PRAGMA user_version or metadata table)\n - Daemon checks on startup: validate schema version matches\n - Auto-migrate or fail with clear instructions on version mismatch\n\n3. **Config file with database path**\n - .beads/config.json specifies {\"database\": \"beads.db\", \"version\": \"0.17.5\"}\n - Daemon and clients read config first (single source of truth)\n - No ambiguity about which file is active\n\n4. **Stricter daemon lock validation**\n - daemon.lock includes database path and beads version (JSON)\n - Client validates: lock says beads.db but I expect bd.db → hard error\n - Already partially implemented, make it stricter\n\n5. **Migration tooling**\n - bd init --migrate or auto-run on first command after upgrade\n - Detects old databases, prompts to migrate/clean up\n - Could be part of daemon auto-start logic\n\n**IMPORTANT**: Allow issues.jsonl to be renamed (users cycle through new names to avoid polluted git history). Only enforce database naming, not JSONL naming.","status":"closed","priority":1,"issue_type":"epic","created_at":"2025-10-27T18:34:51.546639-07:00","updated_at":"2025-10-27T18:34:51.546639-07:00","closed_at":"2025-10-26T19:04:07.843634-07:00"}
{"id":"bd-201","title":"Add \"bd daemons\" command for multi-daemon management","description":"Add a new \"bd daemons\" command with subcommands to manage daemon processes across all beads repositories/worktrees. Should show all running daemons with metadata (version, workspace, uptime, last sync), allow stopping/restarting individual daemons, auto-clean stale processes, view logs, and show exclusive lock status.","design":"Subcommands:\n- list: Show all running daemons with metadata (workspace, PID, version, socket path, uptime, last activity, exclusive lock status)\n- stop \u003cpath|pid\u003e: Gracefully stop a specific daemon\n- restart \u003cpath|pid\u003e: Stop and restart daemon\n- killall: Emergency stop all daemons\n- health: Verify each daemon responds to ping\n- logs \u003cpath\u003e: View daemon logs\n\nFeatures:\n- Auto-clean stale sockets/dead processes\n- Discovery: Scan for .beads/bd.sock files + running processes\n- Communication: Use existing socket protocol, add GET /status endpoint for metadata","status":"closed","priority":1,"issue_type":"epic","created_at":"2025-10-27T18:34:51.546979-07:00","updated_at":"2025-10-27T18:34:51.546979-07:00","closed_at":"2025-10-26T19:26:29.045738-07:00"}
{"id":"bd-21","title":"Fix bd sync prefix mismatch error message suggesting non-existent flag","description":"GH #103: bd sync suggests using --rename-on-import flag that doesn't exist. Need to either implement the flag or fix the error message to suggest the correct workflow.","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-10-22T17:54:24.473508-07:00","updated_at":"2025-10-25T23:15:33.481941-07:00","closed_at":"2025-10-22T17:57:46.973029-07:00"}
{"id":"bd-22","title":"Fix MCP close tool method signature error","description":"GH #107: MCP close() tool fails with \"BdDaemonClient.close() takes 1 positional argument but 2 were given\". Need to fix method signature in beads-mcp server.","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-10-22T19:17:05.429429-07:00","updated_at":"2025-10-25T23:15:33.482758-07:00","closed_at":"2025-10-22T19:19:54.601153-07:00"}
{"id":"bd-23","title":"Update Claude Code marketplace plugin","description":"Update the beads plugin in the Claude Code marketplace to the latest version. This may help resolve some of the open GitHub issues related to marketplace installation and compatibility (#54, #112).\n\nShould include:\n- Latest beads version\n- Updated documentation\n- Any new features or bug fixes","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-22T22:29:11.293161-07:00","updated_at":"2025-10-25T23:15:33.483625-07:00","closed_at":"2025-10-23T22:27:37.671065-07:00"}

15
cmd/bd/main.go
View File

@@ -428,8 +428,21 @@ var rootCmd = &cobra.Command{
// Auto-import if JSONL is newer than DB (e.g., after git pull)
// Skip for import command itself to avoid recursion
// Skip if sync --dry-run to avoid modifying DB in dry-run mode (bd-191)
if cmd.Name() != "import" && autoImportEnabled {
autoImportIfNewer()
// Check if this is sync command with --dry-run flag
if cmd.Name() == "sync" {
if dryRun, _ := cmd.Flags().GetBool("dry-run"); dryRun {
// Skip auto-import in dry-run mode
if os.Getenv("BD_DEBUG") != "" {
fmt.Fprintf(os.Stderr, "Debug: auto-import skipped for sync --dry-run\n")
}
} else {
autoImportIfNewer()
}
} else {
autoImportIfNewer()
}
}
},
PersistentPostRun: func(cmd *cobra.Command, args []string) {