bd sync: 2025-10-22 17:37:45

.beads/beads.jsonl

@@ -61,6 +61,8 @@
 {"id":"bd-64","title":"Add lifecycle safety docs and tests for UnderlyingDB() method","description":"The new UnderlyingDB() method exposes the raw *sql.DB connection for extensions like VC to create their own tables. While database/sql is concurrency-safe, there are lifecycle and misuse risks that need documentation and testing.\n\n**What needs to be done:**\n\n1. **Enhanced documentation** - Expand UnderlyingDB() comments to warn:\n   - Callers MUST NOT call Close() on returned DB\n   - Do NOT change pool/driver settings (SetMaxOpenConns, SetConnMaxIdleTime)\n   - Do NOT modify SQLite PRAGMAs (WAL mode, journal, etc.)\n   - Expect errors after Storage.Close() - use contexts\n   - Keep write transactions short to avoid blocking core storage\n\n2. **Add lifecycle tracking** - Implement closed flag:\n   - Add atomic.Bool closed field to SQLiteStorage\n   - Set flag in Close(), clear in New()\n   - Optional: Add IsClosed() bool method\n\n3. **Add safety tests** (run with -race):\n   - TestUnderlyingDB_ConcurrentAccess - N goroutines using UnderlyingDB() during normal storage ops\n   - TestUnderlyingDB_AfterClose - Verify operations fail cleanly after storage closed\n   - TestUnderlyingDB_CreateExtensionTables - Create VC table with FK to issues, verify FK enforcement\n   - TestUnderlyingDB_LongTxDoesNotCorrupt - Ensure long read tx doesn't block writes indefinitely\n\n**Why this matters:**\nVC will use this to create tables in the same database. Need to ensure production-ready safety without over-engineering.\n\n**Estimated effort:** S+S+S = M total (1-3h)","design":"Oracle recommends \"simple path\": enhanced docs + minimal guardrails + focused tests. See oracle output for detailed rationale on concurrency safety, lifecycle risks, and when to consider advanced path (wrapping interface).","status":"open","priority":1,"issue_type":"task","created_at":"2025-10-22T17:07:56.812983-07:00","updated_at":"2025-10-22T17:07:56.812983-07:00"}
 {"id":"bd-65","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":"open","priority":1,"issue_type":"task","created_at":"2025-10-22T17:07:56.820056-07:00","updated_at":"2025-10-22T17:07:56.820056-07:00","dependencies":[{"issue_id":"bd-65","depends_on_id":"bd-57","type":"discovered-from","created_at":"2025-10-22T17:07:56.822413-07:00","created_by":"daemon"}]}
 {"id":"bd-66","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":"open","priority":3,"issue_type":"feature","created_at":"2025-10-22T17:07:56.832638-07:00","updated_at":"2025-10-22T17:07:56.832638-07:00","dependencies":[{"issue_id":"bd-66","depends_on_id":"bd-57","type":"related","created_at":"2025-10-22T17:07:56.835844-07:00","created_by":"daemon"}]}
+{"id":"bd-67","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-22T17:36:55.463445-07:00","closed_at":"2025-10-22T17:36:55.463445-07:00"}
+{"id":"bd-68","title":"Test close issue","description":"","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-22T17:27:56.89475-07:00","updated_at":"2025-10-22T17:28:00.795511-07:00","closed_at":"2025-10-22T17:28:00.795511-07:00"}
 {"id":"bd-7","title":"Write tests for merge functionality","description":"Unit tests: validation, merge logic, data integrity. Integration tests: end-to-end workflow, export/import. Edge case tests: chains, circular refs, epics.","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-21T23:53:44.31362-07:00","updated_at":"2025-10-22T11:56:36.523309-07:00","closed_at":"2025-10-22T01:07:04.72062-07:00"}
 {"id":"bd-8","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-22T11:56:36.523529-07:00","closed_at":"2025-10-18T09:41:18.209717-07:00"}
 {"id":"bd-9","title":"Test issue 2","description":"","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-21T23:53:44.31362-07:00","updated_at":"2025-10-22T11:56:36.523753-07:00","closed_at":"2025-10-21T22:06:41.257019-07:00","labels":["test-label"]}