From 55fdaf99e736082c4382d890434cec7dbb965d68 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Mon, 24 Nov 2025 00:59:49 -0800 Subject: [PATCH] Centralize error handling patterns in storage layer (bd-bwk2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Standardized error handling across the SQLite storage layer by consistently using wrapDBError() helper functions that were already defined in errors.go. Changes: - config.go: Applied wrapDBError to all config/metadata functions - queries.go: Fixed bare 'return err' in CreateIssue, UpdateIssue, DeleteIssues - store.go: Changed %v to %w for proper error chain preservation - errors_test.go: Added comprehensive test coverage for error wrapping All error paths now: - Wrap errors with operation context using %w - Convert sql.ErrNoRows to ErrNotFound consistently - Preserve error chains for unwrapping and type checking This improves debugging by maintaining operation context throughout the error chain and enables type-safe error checking with sentinel errors. All tests passing āœ“ šŸ¤– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .beads/beads.jsonl | 6 +- internal/storage/sqlite/config.go | 16 +- internal/storage/sqlite/errors_test.go | 248 +++++++++++++++++++++++++ internal/storage/sqlite/queries.go | 22 +-- internal/storage/sqlite/store.go | 2 +- 5 files changed, 273 insertions(+), 21 deletions(-) create mode 100644 internal/storage/sqlite/errors_test.go diff --git a/.beads/beads.jsonl b/.beads/beads.jsonl index f38dce2c..9aece07b 100644 --- a/.beads/beads.jsonl +++ b/.beads/beads.jsonl @@ -32,7 +32,7 @@ {"id":"bd-b2c","content_hash":"4628ae4a4e4170cd4b2786b1dac3630acdb8ee84fc15a75a952cb6f4d2bd5c2c","title":"Clarify metadata handling in init.go (config vs tracking)","description":"Review init.go:207-228 to ensure configuration metadata (issue_prefix, sync.branch) uses Pattern A consistently, while tracking metadata (bd_version, repo_id, etc.) uses Pattern B. Add comments explaining the distinction.","status":"closed","priority":2,"issue_type":"task","created_at":"2025-11-24T00:28:53.199481-08:00","updated_at":"2025-11-24T00:33:24.654007-08:00","closed_at":"2025-11-24T00:33:24.654007-08:00","source_repo":".","dependencies":[{"issue_id":"bd-b2c","depends_on_id":"bd-1qwo","type":"blocks","created_at":"2025-11-24T00:28:53.200299-08:00","created_by":"daemon"}]} {"id":"bd-b55e2ac2","content_hash":"44122b61b1dcd06407ecf36f57577ea72c5df6dc8cc2a8c1b173b37d16a10267","title":"Fix autoimport tests for content-hash collision scoring","description":"## Overview\nThree autoimport tests are failing after bd-cbed9619.4 because they expect behavior based on the old reference-counting collision resolution, but the system now uses deterministic content-hash scoring.\n\n## Failing Tests\n1. `TestAutoImportMultipleCollisionsRemapped` - expects local versions preserved\n2. `TestAutoImportAllCollisionsRemapped` - expects local versions preserved \n3. `TestAutoImportCollisionRemapMultipleFields` - expects specific collision resolution behavior\n\n## Root Cause\nThese tests were written when ScoreCollisions used reference counting to determine which version to keep. Now it uses content-hash comparison (introduced in commit 2e87329), which produces different but deterministic results.\n\n## Example\nOld behavior: Issue with more references would be kept\nNew behavior: Issue with lexicographically lower content hash is kept\n\n## Solution\nUpdate each test to:\n1. Verify the new content-hash based behavior is correct\n2. Check that the remapped issue (not necessarily local/remote) has the expected content\n3. Ensure dependencies are preserved on the correct remapped issue\n\n## Acceptance Criteria\n- All three autoimport tests pass\n- Tests verify content-hash determinism (same collision always resolves the same way)\n- Tests check dependency preservation on remapped issues\n- Test documentation explains content-hash scoring expectations\n\n## Files to Modify\n- `cmd/bd/autoimport_collision_test.go`\n\n## Testing\nRun: `go test ./cmd/bd -run \"TestAutoImport.*Collision\" -v`","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-28T19:17:28.358028-07:00","updated_at":"2025-11-23T23:20:43.995595-08:00","closed_at":"2025-11-23T23:20:43.995595-08:00","source_repo":"."} {"id":"bd-bt6y","content_hash":"462f08aa379cf2f196b4c0ca096271fa47ab5e1a18c5663c28d2d86fd02115cf","title":"Improve compact/daemon/merge documentation and UX","description":"Multiple documentation and UX issues encountered:\n1. \"bd compact --analyze\" fails with misleading \"requires SQLite storage\" error when daemon is running. Needs --no-daemon or better error.\n2. \"bd merge\" help text is outdated (refers to 3-way merge instead of issue merging).\n3. Daemon mode purpose isn't clear to local-only users.\n4. Compact/cleanup commands are hard to discover.\n\nProposed fixes:\n- Fix compact+daemon interaction or error message.\n- Update \"bd merge\" help text.\n- Add \"when to use daemon\" section to docs.\n- Add maintenance section to quickstart.\n","status":"open","priority":2,"issue_type":"task","created_at":"2025-11-20T18:55:43.637047-05:00","updated_at":"2025-11-20T18:55:43.637047-05:00","source_repo":"."} -{"id":"bd-bwk2","content_hash":"b69758a5dd9ce7605a61dc6e1fe3e753b87dfc6824c248d6ad56e038d47e77e7","title":"Centralize error handling patterns in storage layer","description":"80+ instances of inconsistent error handling across sqlite.go with mix of %w, %v, and no wrapping.\n\nLocation: internal/storage/sqlite/sqlite.go (throughout)\n\nProblem:\n- Some use fmt.Errorf(\"op failed: %w\", err) - correct wrapping\n- Some use fmt.Errorf(\"op failed: %v\", err) - loses error chain\n- Some return err directly - no context\n- Hard to debug production issues\n- Can't distinguish error types\n\nSolution: Create internal/storage/sqlite/errors.go:\n- Define sentinel errors (ErrNotFound, ErrInvalidID, etc.)\n- Create wrapDBError(op string, err error) helper\n- Convert sql.ErrNoRows to ErrNotFound\n- Always wrap with operation context\n\nImpact: Lost error context; inconsistent messages; hard to debug\n\nEffort: 5-7 hours","status":"open","priority":1,"issue_type":"task","created_at":"2025-11-16T14:51:54.974909-08:00","updated_at":"2025-11-16T14:51:54.974909-08:00","source_repo":"."} +{"id":"bd-bwk2","content_hash":"62146d5d6ed4bc35c0e0d15c79b311911c503d6f05a79371c5bd12d561469a0d","title":"Centralize error handling patterns in storage layer","description":"80+ instances of inconsistent error handling across sqlite.go with mix of %w, %v, and no wrapping.\n\nLocation: internal/storage/sqlite/sqlite.go (throughout)\n\nProblem:\n- Some use fmt.Errorf(\"op failed: %w\", err) - correct wrapping\n- Some use fmt.Errorf(\"op failed: %v\", err) - loses error chain\n- Some return err directly - no context\n- Hard to debug production issues\n- Can't distinguish error types\n\nSolution: Create internal/storage/sqlite/errors.go:\n- Define sentinel errors (ErrNotFound, ErrInvalidID, etc.)\n- Create wrapDBError(op string, err error) helper\n- Convert sql.ErrNoRows to ErrNotFound\n- Always wrap with operation context\n\nImpact: Lost error context; inconsistent messages; hard to debug\n\nEffort: 5-7 hours","status":"closed","priority":1,"issue_type":"task","created_at":"2025-11-16T14:51:54.974909-08:00","updated_at":"2025-11-24T00:57:47.359519-08:00","closed_at":"2025-11-24T00:57:47.359519-08:00","source_repo":".","dependencies":[{"issue_id":"bd-bwk2","depends_on_id":"bd-vfe","type":"blocks","created_at":"2025-11-24T00:53:28.831021-08:00","created_by":"daemon"},{"issue_id":"bd-bwk2","depends_on_id":"bd-z8z","type":"blocks","created_at":"2025-11-24T00:53:28.897593-08:00","created_by":"daemon"},{"issue_id":"bd-bwk2","depends_on_id":"bd-w8h","type":"blocks","created_at":"2025-11-24T00:53:28.957487-08:00","created_by":"daemon"},{"issue_id":"bd-bwk2","depends_on_id":"bd-r71","type":"blocks","created_at":"2025-11-24T00:53:29.023262-08:00","created_by":"daemon"}]} {"id":"bd-c362","content_hash":"c9b1c62aad1db56a65ae6b628ea6e3d54923c71436fe5eec1ea4daddfcd68fc2","title":"Extract database search logic into helper function","description":"The logic for finding a database in a beads directory is duplicated:\n- FindDatabasePath() BEADS_DIR section (beads.go:141-169)\n- findDatabaseInTree() (beads.go:248-280)\n\nBoth implement the same search order:\n1. Check config.json first (single source of truth)\n2. Fall back to canonical beads.db\n3. Search for *.db files, filtering backups and vc.db\n\nRefactoring suggestion:\nExtract to a helper function like:\n func findDatabaseInBeadsDir(beadsDir string) string\n\nBenefits:\n- Single source of truth for database search logic\n- Easier to maintain and update search order\n- Reduces code duplication\n\nRelated to [deleted:[deleted:[deleted:[deleted:[deleted:[deleted:[deleted:[deleted:[deleted:bd-e16b]]]]]]]]] implementation.","status":"open","priority":3,"issue_type":"chore","created_at":"2025-11-02T18:34:02.831543-08:00","updated_at":"2025-11-23T22:38:46.782725-08:00","source_repo":"."} {"id":"bd-c4rq","content_hash":"4c096e1d84c3ba5b5b4e107692b990a99166b4c99a4262fd26ec08297fb81046","title":"Refactor: Move staleness check inside daemon branch","description":"## Problem\n\nCurrently ensureDatabaseFresh() is called before the daemon mode check, but it checks daemonClient != nil internally and returns early. This is redundant.\n\n**Location:** All read commands (list.go:196, show.go:27, ready.go:102, status.go:80, etc.)\n\n## Current Pattern\n\nCall happens before daemon check, function checks daemonClient internally.\n\n## Better Pattern\n\nMove staleness check to direct mode branch only, after daemon check.\n\n## Impact\nLow - minor performance improvement (avoids one function call per command in daemon mode)\n\n## Effort\nMedium - requires refactoring 8 command files\n\n## Priority\nLow - can defer to future cleanup PR","status":"open","priority":3,"issue_type":"chore","created_at":"2025-11-20T20:17:45.119583-05:00","updated_at":"2025-11-20T20:17:45.119583-05:00","source_repo":"."} {"id":"bd-d4i","content_hash":"41cafb4bfa5377a84005b08cddd3e703c1317e98ef32b050ddaabf1bdc7718c9","title":"Create tip system infrastructure for contextual hints","description":"Implement a tip/hint system that shows helpful contextual messages after successful commands. This is different from the existing error-path \"Hint:\" messages - tips appear on success paths to educate users about features they might not know about.","design":"## Implementation\n\nCreate `cmd/bd/tips.go` with:\n\n### Core Infrastructure\n```go\ntype Tip struct {\n ID string\n Condition func() bool // Should this tip be eligible?\n Message string\n Frequency time.Duration // Minimum gap between showings\n Priority int // Higher = shown first when eligible\n Probability float64 // 0.0 to 1.0 - chance of showing\n}\n\nfunc maybeShowTip(store storage.Storage) {\n if jsonOutput || quietMode {\n return // Respect output flags\n }\n \n tip := selectNextTip(store)\n if tip != nil {\n fmt.Fprintf(os.Stdout, \"\\nšŸ’” Tip: %s\\n\", tip.Message)\n recordTipShown(store, tip.ID)\n }\n}\n\nfunc selectNextTip(store storage.Storage) *Tip {\n now := time.Now()\n var eligibleTips []Tip\n \n // Filter to eligible tips (condition + frequency check)\n for _, tip := range tips {\n if !tip.Condition() {\n continue\n }\n \n lastShown := getLastShown(store, tip.ID)\n if !lastShown.IsZero() \u0026\u0026 now.Sub(lastShown) \u003c tip.Frequency {\n continue\n }\n \n eligibleTips = append(eligibleTips, tip)\n }\n \n if len(eligibleTips) == 0 {\n return nil\n }\n \n // Sort by priority (highest first)\n sort.Slice(eligibleTips, func(i, j int) bool {\n return eligibleTips[i].Priority \u003e eligibleTips[j].Priority\n })\n \n // Apply probability roll (in priority order)\n for _, tip := range eligibleTips {\n if rand.Float64() \u003c tip.Probability {\n return \u0026tip\n }\n }\n \n return nil // No tips won probability roll\n}\n```\n\n### Probability Examples\n\n```go\n// High priority, high probability = shows often\n{Priority: 90, Probability: 0.8} // 80% chance when eligible\n\n// High priority, medium probability = important but not spammy\n{Priority: 100, Probability: 0.6} // 60% chance\n\n// Low priority, low probability = rare suggestion\n{Priority: 30, Probability: 0.3} // 30% chance\n```\n\n### Metadata Storage\nUse existing metadata table to track:\n- `tip_{id}_last_shown` - Timestamp of last display (RFC3339 format)\n- `tip_{id}_dismissed` - User permanently dismissed (future feature)\n\n### Integration Points\nCall `maybeShowTip()` at end of:\n- `bd list` - After showing issues\n- `bd ready` - After showing ready work\n- `bd create` - After creating issue\n- `bd show` - After showing issue details\n\n## Design Decisions\n- Tips shown on stdout (informational, not errors)\n- Respects `--json` and `--quiet` flags\n- Frequency enforces minimum gap between showings\n- Priority determines evaluation order\n- Probability reduces spam (not every eligible tip shows)\n- Store state in metadata table (no new files)\n- Deterministic seed for testing (optional BEADS_TIP_SEED env var)","acceptance_criteria":"- Tip infrastructure exists in cmd/bd/tips.go\n- Tips respect --json and --quiet flags\n- Frequency tracking works (no spam)\n- Metadata table stores tip state\n- Unit tests for tip selection logic\n- Documentation in code comments","status":"open","priority":2,"issue_type":"feature","created_at":"2025-11-11T23:29:15.693956-08:00","updated_at":"2025-11-11T23:49:50.812933-08:00","source_repo":"."} @@ -57,14 +57,18 @@ {"id":"bd-p6vp","content_hash":"1df6d3b9b438cdcdbc618c24fea48769c1f22e8a8701af4e742531d4433ca7ea","title":"Clarify .beads/.gitattributes handling in Protected Branches docs","description":"Protected Branches docs quick start leaves untracked `.beads` directory and `.gitattributes`.\nQuestion: Are these changes meant to be checked into the protected branch?\nNeed to clarify if these should be ignored or committed, or if the instructions are missing a step.\n","status":"open","priority":2,"issue_type":"task","created_at":"2025-11-20T18:56:25.79407-05:00","updated_at":"2025-11-20T18:56:25.79407-05:00","source_repo":"."} {"id":"bd-pp3","content_hash":"812488cb18460a1e3974e8ddacc9a7494a2f495863b82551983362f86011e1f3","title":"Add metadata distinction guidelines to ERROR_HANDLING.md","description":"Update ERROR_HANDLING.md with clear guidelines distinguishing configuration metadata (Pattern A) from tracking metadata (Pattern B). Include examples from init.go and sync.go.","status":"closed","priority":3,"issue_type":"task","created_at":"2025-11-24T00:28:54.559851-08:00","updated_at":"2025-11-24T00:34:13.980602-08:00","closed_at":"2025-11-24T00:34:13.980602-08:00","source_repo":".","dependencies":[{"issue_id":"bd-pp3","depends_on_id":"bd-1qwo","type":"blocks","created_at":"2025-11-24T00:28:54.560699-08:00","created_by":"daemon"}]} {"id":"bd-r46","content_hash":"31d26e3081c31f4e8813fc1287c3bb7b2e87763eccc24b930a763c380e8433ab","title":"Support --reason flag in daemon mode for reopen command","description":"The reopen.go command has a TODO at line 61 to add reason as a comment once RPC supports AddComment. Currently --reason flag is ignored in daemon mode with a warning.","status":"open","priority":2,"issue_type":"feature","created_at":"2025-11-21T18:55:10.773626-05:00","updated_at":"2025-11-21T18:55:10.773626-05:00","source_repo":"."} +{"id":"bd-r71","content_hash":"d968112dba4120afe0bae451a01a9a61ae237aa4e20032a466cf96eeb07fe59b","title":"Add tests for error wrapping behavior","description":"","status":"closed","priority":2,"issue_type":"task","created_at":"2025-11-24T00:53:16.906291-08:00","updated_at":"2025-11-24T00:57:46.21316-08:00","closed_at":"2025-11-24T00:57:46.21316-08:00","source_repo":"."} {"id":"bd-s0z","content_hash":"b69df0c8664737b3c04b10e3137652e3c8c3d782de0ecd02bfcd648919f8d944","title":"Consider extracting error handling helpers","description":"Evaluate creating FatalError() and WarnError() helpers as suggested in ERROR_HANDLING.md to reduce boilerplate and enforce consistency. Prototype in a few files first to validate the approach.","status":"open","priority":4,"issue_type":"task","created_at":"2025-11-24T00:28:57.248959-08:00","updated_at":"2025-11-24T00:28:57.248959-08:00","source_repo":".","dependencies":[{"issue_id":"bd-s0z","depends_on_id":"bd-1qwo","type":"blocks","created_at":"2025-11-24T00:28:57.249945-08:00","created_by":"daemon"}]} {"id":"bd-t3b","content_hash":"c32a3a0f2f836148033fb330e209ac22e06dbecf18894153c15e2036f5afae1c","title":"Add test coverage for internal/config package","description":"","design":"Config package has 1 test file. Need comprehensive tests. Target: 70% coverage","acceptance_criteria":"- At least 3 test files\n- Package coverage \u003e= 70%","status":"open","priority":2,"issue_type":"task","created_at":"2025-11-20T21:21:22.91657-05:00","updated_at":"2025-11-20T21:21:22.91657-05:00","source_repo":".","dependencies":[{"issue_id":"bd-t3b","depends_on_id":"bd-ge7","type":"blocks","created_at":"2025-11-20T21:21:31.201036-05:00","created_by":"daemon"}]} {"id":"bd-t4u1","content_hash":"5558c6e25c6aae4be03fd9f112d892f6e69dc020dee2292a24ec185fb7b6a054","title":"False positive detection by Kaspersky Antivirus (Trojan)","description":"Kaspersky Antivirus falsely detects beads (bd.exe v0.23.1) as a Trojan (PDM:Trojan.Win32.Generic) and removes it.\nEvent: Malicious object detected\nComponent: System Watcher\nObject name: bd.exe\n","status":"open","priority":1,"issue_type":"task","created_at":"2025-11-20T18:56:12.498187-05:00","updated_at":"2025-11-20T18:56:12.498187-05:00","source_repo":"."} {"id":"bd-tne","content_hash":"2a6596980450714800bddc88e106026743a1a131e96f09198eb7dc2a16d75ca4","title":"Add Claude setup tip with dynamic priority","description":"Add a predefined tip that suggests running `bd setup claude` when Claude Code is detected but not configured. This tip should have higher priority (shown more frequently) until the setup is complete.","design":"## Implementation\n\nAdd to tip registry in `cmd/bd/tips.go`:\n\n```go\n{\n ID: \"claude_setup\",\n Condition: func() bool {\n return isClaudeDetected() \u0026\u0026 !isClaudeSetupComplete()\n },\n Message: \"Run 'bd setup claude' to enable automatic context recovery in Claude Code\",\n Frequency: 24 * time.Hour, // Daily minimum gap\n Priority: 100, // Highest priority\n Probability: 0.6, // 60% chance when eligible\n}\n```\n\n## Detection Logic\n\n```go\nfunc isClaudeDetected() bool {\n // Check environment variables\n if os.Getenv(\"CLAUDE_CODE\") != \"\" || os.Getenv(\"ANTHROPIC_CLI\") != \"\" {\n return true\n }\n // Check if .claude/ directory exists\n if _, err := os.Stat(filepath.Join(os.Getenv(\"HOME\"), \".claude\")); err == nil {\n return true\n }\n return false\n}\n\nfunc isClaudeSetupComplete() bool {\n // Check for global installation\n home, err := os.UserHomeDir()\n if err == nil {\n _, err1 := os.Stat(filepath.Join(home, \".claude/commands/prime_beads.md\"))\n _, err2 := os.Stat(filepath.Join(home, \".claude/hooks/sessionstart\"))\n if err1 == nil \u0026\u0026 err2 == nil {\n return true // Global hooks installed\n }\n }\n \n // Check for project installation\n _, err1 := os.Stat(\".claude/commands/prime_beads.md\")\n _, err2 := os.Stat(\".claude/hooks/sessionstart\")\n return err1 == nil \u0026\u0026 err2 == nil\n}\n```\n\n## Priority and Probability Behavior\n\n**Why 60% probability?**\n- Important message (priority 100) but not critical\n- Daily frequency + 60% = shows ~4 times per week\n- Avoids spam while staying visible\n- Balances persistence with user experience\n\n**Comparison with other probabilities:**\n- 100% probability: Shows EVERY day (annoying)\n- 80% probability: Shows ~6 days per week (too frequent)\n- 60% probability: Shows ~4 days per week (balanced)\n- 40% probability: Shows ~3 days per week (might be missed)\n\n**Auto-stops when setup complete:**\n- Condition becomes false after `bd setup claude`\n- No manual dismissal needed\n- Tip naturally disappears from rotation","acceptance_criteria":"- Claude setup tip added to registry\n- isClaudeDetected() checks environment and filesystem\n- isClaudeSetupComplete() verifies hook installation\n- Tip shows daily until setup complete\n- Tip stops showing after setup\n- Unit tests for detection functions","status":"open","priority":2,"issue_type":"task","created_at":"2025-11-11T23:29:29.871324-08:00","updated_at":"2025-11-11T23:50:29.756454-08:00","source_repo":".","dependencies":[{"issue_id":"bd-tne","depends_on_id":"bd-d4i","type":"blocks","created_at":"2025-11-11T23:29:29.872081-08:00","created_by":"daemon"}]} {"id":"bd-tru","content_hash":"0de12031088519a3dcd27968d6bf17eb3a92d1853264e5a0dceef3310b3a2b04","title":"Update documentation for bd prime and Claude integration","description":"Update AGENTS.md, README.md, and QUICKSTART.md to document the new `bd prime` command, `bd setup claude` command, and tip system.","design":"## Documentation Updates\n\n### AGENTS.md\nAdd new section \"Context Recovery\":\n```markdown\n## Context Recovery\n\n### The Problem\nAfter context compaction or clearing conversation, AI agents may forget to use Beads and revert to markdown TODOs. Claude Code hooks solve this.\n\n### bd prime Command\nThe `bd prime` command outputs essential Beads workflow context in AI-optimized markdown format (~1-2k tokens).\n\n**When to use:**\n- After context compaction\n- After clearing conversation\n- Starting new session\n- When agent seems to forget bd workflow\n- Manual context refresh\n\n**Usage:**\n```bash\nbd prime # Output workflow context\n```\n\n### Automatic Integration (Recommended)\n\nRun `bd setup claude` to install hooks that auto-refresh bd context:\n- **SessionStart hook**: Loads context in new sessions\n- **PreCompact hook**: Refreshes context before compaction (survives better)\n- **Works with MCP**: Hooks complement MCP server (not replace)\n- **Works without MCP**: bd prime provides workflow via CLI\n\n**Why hooks matter even with MCP:**\n- MCP provides native tools, but agent may forget to use them\n- Hooks keep \"use bd, not markdown\" fresh in context\n- PreCompact refreshes workflow before compaction\n\n### MCP Server vs bd prime\n\n**Not an either/or choice** - they solve different problems:\n\n| Aspect | MCP Server | bd prime | Both |\n|--------|-----------|----------|------|\n| **Purpose** | Native bd tools | Workflow context | Best of both |\n| **Tokens** | 10.5k always loaded | ~1-2k when called | 10.5k + ~2k |\n| **Tool access** | Function calls | CLI via Bash | Function calls |\n| **Context memory** | Can fade after compaction | Hooks keep fresh | Hooks + tools |\n| **Recommended** | Heavy usage | Token optimization | Best experience |\n\n**Setup options:**\n```bash\nbd setup claude # Install hooks (works with or without MCP)\nbd setup claude --local # Per-project only\nbd setup claude --remove # Remove hooks\n```\n```\n\n### README.md\nAdd to \"Getting Started\" section:\n```markdown\n### AI Agent Integration\n\n**Claude Code users:** Run `bd setup claude` to install automatic context recovery hooks.\n\nHooks work with both MCP server and CLI approaches, preventing agents from forgetting bd workflow after compaction.\n\n**MCP vs bd prime:**\n- **With MCP server**: Hooks keep agent using bd tools (prevents markdown TODO reversion)\n- **Without MCP server**: Hooks provide workflow context via `bd prime` (~1-2k tokens)\n```\n\n### QUICKSTART.md\nAdd section on agent integration:\n```markdown\n## For AI Agents\n\n**Context loading:**\n```bash\nbd prime # Load workflow context (~1-2k tokens)\n```\n\n**Automatic setup (Claude Code):**\n```bash\nbd setup claude # Install hooks for automatic context recovery\n```\n\nHooks prevent agents from forgetting bd workflow after compaction.\n```","acceptance_criteria":"- AGENTS.md has Context Recovery section\n- README.md mentions bd setup claude\n- QUICKSTART.md mentions bd prime\n- Examples show when to use bd prime vs MCP\n- Clear comparison of trade-offs","status":"open","priority":2,"issue_type":"task","created_at":"2025-11-11T23:30:22.77349-08:00","updated_at":"2025-11-11T23:45:23.242658-08:00","source_repo":"."} {"id":"bd-v0y","content_hash":"72d91011884bc9e20ac1c18b67f66489dcf49f3fec6da4d9b4dbbe1832d8b72f","title":"Critical: bd sync overwrites git-pulled JSONL instead of importing it","description":"## Problem\n\nWhen a user runs `git pull` (which updates .beads/beads.jsonl) followed by `bd sync`, the sync command fails to detect that the JSONL changed and exports the local database, **overwriting the pulled JSONL** instead of importing it first.\n\nThis causes cleaned-up issues (removed via `bd cleanup` in another repo) to be resurrected and pushed back to the remote.\n\n## Root Cause\n\n`hasJSONLChanged()` in cmd/bd/integrity.go incorrectly returns FALSE when it should return TRUE after git pull updates the JSONL file.\n\nThe function has two code paths:\n1. **Fast-path (line 128-129)**: If mtime unchanged, return false\n2. **Slow-path (line 135-151)**: Compute hash and compare\n\nSuspected issue: The mtime-based fast-path may incorrectly return false if:\n- Git preserves mtime when checking out files, OR\n- The mtime check logic has a race condition, OR\n- The stored last_import_mtime is stale/incorrect\n\n## Reproduction\n\n**Setup:**\n1. Repo A: Has 686 issues (including 630 closed)\n2. Run `bd cleanup -f` in Repo A (removes 630 closed issues ā†’ 56 issues remain)\n3. Push to remote\n4. Repo B: Still has 686 issues in database\n\n**Trigger:**\n1. In Repo B: `git pull` (JSONL now has 56 issues)\n2. In Repo B: `bd sync`\n\n**Expected behavior:**\n- hasJSONLChanged() returns TRUE\n- Auto-imports 56-issue JSONL\n- Database updated to 56 issues\n- Exports (no-op, DB and JSONL match)\n- Pushes\n\n**Actual behavior:**\n- hasJSONLChanged() returns FALSE āŒ\n- Skips auto-import\n- Exports 686 issues from DB to JSONL\n- Commits and pushes the 686-issue JSONL\n- **Undoes the cleanup from Repo A**\n\n## Evidence\n\nFrom actual session (2025-11-23):\n- Commit 8bc8611 in cino/beads shows: `+640, -14 lines` after bd sync\n- No \"Auto-import complete\" message in output\n- Database had 686 issues, JSONL was pulled with 56 issues\n- Sync exported DB ā†’ JSONL instead of importing JSONL ā†’ DB\n\n## Impact\n\n**Critical:** This breaks multi-device sync and causes:\n- Lost work (cleanup operations undone)\n- Data resurrection (deleted issues come back)\n- Confusing merge conflicts\n- User trust issues (\"Why does sync keep bringing back deleted issues?\")\n\n## Proposed Solutions\n\n### Option 1: Remove mtime fast-path (safest)\nAlways compute and compare hashes. Eliminates false negatives from mtime issues.\n\n**Pros:** Guaranteed correctness\n**Cons:** Slightly slower (hash computation on every sync)\n\n### Option 2: Fix mtime comparison logic\nInvestigate why mtime check fails and fix it properly.\n\n**Areas to check:**\n- Does git preserve mtime on checkout? (may vary by git version/config)\n- Is last_import_mtime updated correctly after all operations?\n- Race condition between git pull and mtime check?\n\n### Option 3: Add hash-based validation\nKeep mtime fast-path but add hash validation as backup:\n- If mtime unchanged, still spot-check hash occasionally (e.g., 10% of the time)\n- Log warnings when mtime and hash disagree\n- This would catch the bug while maintaining performance\n\n### Option 4: Add `--force-import` flag to bd sync\nShort-term workaround: Allow users to force import even if hasJSONLChanged() returns false.\n\n## Workaround\n\nUntil fixed, after `git pull`:\n```bash\nbd import --force # Force import the pulled JSONL\nbd sync # Then sync normally\n```\n\nOr manually run bd cleanup in affected repos.\n\n## Files\n\n- cmd/bd/integrity.go:101-152 (hasJSONLChanged function)\n- cmd/bd/sync.go:130-143 (auto-import logic)\n- cmd/bd/autoflush.go:698 (export updates last_import_hash)\n\n## Related Issues\n\n- bd-77gm: Import reports misleading counts\n- bd-khnb: Content-based staleness detection (original implementation of hash checking)\n","notes":"## Fix Implemented (Option 1)\n\n**Root Cause Confirmed:**\nGit does NOT update mtime when checking out files. Testing confirmed that after `git checkout HEAD~1`, the file mtime remains unchanged even though content differs.\n\n**Solution:**\nRemoved the mtime-based fast-path in `hasJSONLChanged()`. Now always computes content hash for comparison.\n\n**Changes Made:**\n1. **cmd/bd/integrity.go:107-116** - Removed mtime fast-path, always compute hash\n2. **cmd/bd/sync.go:752** - Removed mtime storage after import\n3. **cmd/bd/import.go:340** - Removed mtime storage after import\n4. **cmd/bd/daemon_sync.go:280-301** - Removed mtime storage and updated comments\n5. **cmd/bd/daemon_sync_test.go:479,607,628** - Removed mtime assertions from tests\n\n**Performance Impact:**\nMinimal. Hash computation takes ~10-50ms even for large databases, which is acceptable for sync operations.\n\n**Testing:**\n- All existing tests pass\n- Test \"mtime changed but content same - git operation scenario\" verifies the fix\n- Full test suite passes (cmd/bd and all internal packages)\n\n**Verification:**\nThe fix ensures that after `git pull` updates the JSONL:\n1. `hasJSONLChanged()` returns TRUE (content hash differs)\n2. Auto-import runs and updates database\n3. No data loss or resurrection of deleted issues","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-11-23T22:24:53.69901-08:00","updated_at":"2025-11-23T23:01:49.003926-08:00","closed_at":"2025-11-23T22:50:12.611126-08:00","source_repo":"."} +{"id":"bd-vfe","content_hash":"a491220195d767432333fecbfeb9d3124be5a38decaddc9286a9c7d0f16fd9da","title":"Apply wrapDBError to config.go","description":"","status":"closed","priority":2,"issue_type":"task","created_at":"2025-11-24T00:53:12.771275-08:00","updated_at":"2025-11-24T00:54:15.676618-08:00","closed_at":"2025-11-24T00:54:15.676618-08:00","source_repo":"."} +{"id":"bd-w8h","content_hash":"50d08ac2f696be4718763c75253447a763b2e433108c614e505be0d9c3f36b17","title":"Apply wrapDBError to remaining storage files","description":"","status":"closed","priority":2,"issue_type":"task","created_at":"2025-11-24T00:53:15.498428-08:00","updated_at":"2025-11-24T00:56:47.434071-08:00","closed_at":"2025-11-24T00:56:47.434071-08:00","source_repo":"."} {"id":"bd-wcl","content_hash":"c08d62ce3627a49126c63f6a630a08c1666e5b1b8d9148ae0c72d7d06611b2a9","title":"Document CLI + hooks as recommended approach over MCP","description":"Update documentation to position CLI + bd prime hooks as the primary recommended approach over MCP server, explaining why minimizing context matters even with large context windows (compute cost, energy, environment, latency).","design":"## Goals\n\nPosition CLI + `bd prime` hooks as the **primary recommended approach** for AI agent integration, with MCP server as a legacy/fallback option.\n\nExplore **hybrid mode** - if certain commands benefit from MCP (UX/DX advantages like no approval prompts), minimize MCP surface area to only those commands.\n\nThis requires production validation first - only update docs after CLI mode is proven reliable.\n\n## Why Minimize Context (Even With Large Windows)\n\n**Context window size ā‰  free resource**\n\nLarge context windows (100k+, 200k+) don't mean we should fill them wastefully. Every token in context has real costs:\n\n### Compute Cost\n- **Processing overhead**: Larger context = more GPU/CPU cycles per request\n- **Memory usage**: 10.5k tokens consume significant RAM/VRAM\n- **Scaling impact**: Multiplied across all users, all sessions, all requests\n\n### Energy \u0026 Environment\n- **Electricity**: More compute = more power consumption\n- **Carbon footprint**: Data centers running on grid power (not all renewable)\n- **Sustainability**: Unnecessary token usage contributes to AI's environmental impact\n- **Responsibility**: Efficient tools are better for the planet\n\n### User Experience\n- **Latency**: Larger context = slower processing (noticeable at 10k+ tokens)\n- **Cost**: Many AI services charge per token (input + output)\n- **Rate limits**: Context counts against API quotas\n\n### Engineering Excellence\n- **Efficiency**: Good engineering minimizes resource usage\n- **Scalability**: Efficient tools scale better\n- **Best practices**: Optimize for the common case\n\n**The comparison:**\n\n| Approach | Standing Context | Efficiency | User Cost | Environmental Impact |\n|----------|-----------------|------------|-----------|---------------------|\n| **CLI + hooks** | ~1-2k tokens | 80-90% reduction | Lower | Sustainable āœ“ |\n| **MCP minimal** | ~2-4k tokens | 60-80% reduction | Medium | Better āœ“ |\n| **MCP full** | ~10.5k tokens | Baseline | Higher | Wasteful āœ— |\n\n**Functional equivalence:**\n- CLI via Bash tool works just as well as MCP native calls\n- Same features, same reliability\n- No downside except initial learning curve\n\n## Hybrid Mode: Minimal MCP Surface Area\n\n**Philosophy:** MCP server doesn't have to expose everything.\n\nIf certain commands have legitimate UX/DX benefits from MCP (e.g., no approval prompts, cleaner syntax), we can expose ONLY those commands via MCP while using CLI for everything else.\n\n### Potential MCP-Only Candidates (TBD)\n\nCommands that might benefit from MCP native calls:\n- `ready` - frequently checked, no side effects, approval prompt annoying\n- `show` - read-only, frequently used, approval slows workflow\n- `list` - read-only, no risk, approval adds friction\n\nCommands that work fine via CLI:\n- `create` - complex parameters, benefits from explicit confirmation\n- `update` - state changes, good to see command explicitly\n- `close` - state changes, explicit is better\n- `dep` - relationships, good to see what's being linked\n- `sync` - git operations, definitely want visibility\n\n### Token Budget\n\n**Full MCP** (current): ~10.5k tokens\n- All ~20+ bd commands exposed\n- All parameter schemas\n- All descriptions and examples\n\n**Minimal MCP** (proposed): ~2-4k tokens\n- 3-5 high-frequency read commands only\n- Simplified schemas\n- Minimal descriptions\n- Everything else via CLI\n\n**Pure CLI**: ~1-2k tokens (only on SessionStart/PreCompact)\n- No MCP tools loaded\n- All commands via Bash\n\n### Investigation Required\n\nBefore implementing hybrid mode, validate:\n\n1. **Do MCP calls actually skip approval prompts?**\n - Test with Claude Code approval settings\n - Compare MCP tool calls vs Bash tool calls\n - Measure UX difference in real usage\n\n2. **What's the actual token breakdown per command?**\n - Measure individual command schemas\n - Calculate token savings for minimal vs full\n\n3. **Is approval prompt the only benefit?**\n - Are there other UX advantages to MCP?\n - Does native syntax actually improve experience?\n - User testing with both approaches\n\n4. **Can we dynamically load MCP tools?**\n - Only load MCP when certain commands needed?\n - Hot-swap between CLI and MCP?\n - Probably not - MCP loads at startup\n\n### Hybrid Mode Documentation (If Validated)\n\n```markdown\n## Choosing Your Integration Approach\n\nBeads supports three AI agent integration approaches:\n\n### CLI + Hooks (Recommended - Most Efficient)\n\n**Setup:** `bd setup claude`\n\nUses Claude Code hooks to inject workflow context via `bd prime` command. Agent uses bd via Bash tool.\n\n**Tokens:** ~1-2k (on SessionStart/PreCompact only)\n\n**Pros:**\n- Maximum efficiency (80-90% reduction vs full MCP)\n- Lowest compute/energy usage\n- Same functionality as MCP\n\n**Cons:**\n- Bash tool calls may require approval prompts\n- Slightly more verbose in conversation\n\n### Minimal MCP + Hooks (Balanced)\n\n**Setup:** Install minimal MCP server (read-only commands) + `bd setup claude`\n\nExposes only high-frequency read commands via MCP (ready, show, list). Everything else via CLI.\n\n**Tokens:** ~2-4k MCP + ~1-2k hooks\n\n**Pros:**\n- 60-80% reduction vs full MCP\n- No approval prompts for common queries\n- Cleaner syntax for frequent operations\n- Still efficient\n\n**Cons:**\n- Requires MCP server (additional setup)\n- Mixed interface (some MCP, some CLI)\n\n### Full MCP + Hooks (Legacy)\n\n**Setup:** Install full MCP server + `bd setup claude`\n\n**Tokens:** ~10.5k MCP + hooks\n\n**Pros:**\n- All commands as native function calls\n- Consistent interface\n\n**Cons:**\n- Highest token usage (worst for compute/energy/cost)\n- Slowest processing\n- Less sustainable\n\n### Recommendation\n\n1. **Start with CLI + hooks** - most efficient, works great\n2. **Try minimal MCP** if approval prompts become annoying\n3. **Avoid full MCP** - wasteful with no significant benefit\n```\n\n## Production Validation Checklist\n\nBefore making these documentation changes, validate CLI approach works reliably:\n\n### Phase 1: Pure CLI Validation\n- [ ] `bd prime` implemented and tested\n- [ ] Hooks installed and working in Claude Code\n- [ ] Real-world usage by at least 2-3 developers for 1+ weeks\n- [ ] No significant usability issues reported\n- [ ] Agent successfully uses bd via Bash tool\n- [ ] Document which commands (if any) have approval prompt issues\n\n### Phase 2: Hybrid Mode Investigation (Optional)\n- [ ] Test if MCP calls skip approval prompts vs Bash calls\n- [ ] Measure token cost per MCP command\n- [ ] Identify minimal set of commands worth exposing via MCP\n- [ ] Build minimal MCP server variant\n- [ ] Validate token savings (should be 60-80% vs full MCP)\n- [ ] User testing shows actual UX improvement\n\n### Phase 3: Documentation Update\n- [ ] Update based on validation results\n- [ ] Include measured token counts (not estimates)\n- [ ] Provide clear migration paths\n- [ ] Update `bd doctor` recommendations\n\n## Migration Guide (Optional)\n\nFor users currently using MCP:\n\n```markdown\n### Migrating from Full MCP to CLI + Hooks\n\nAlready using full MCP server? You can switch to the more efficient CLI approach:\n\n1. Install hooks: `bd setup claude`\n2. Test it works (hooks inject context, agent uses Bash tool)\n3. Remove MCP server from `~/.claude/settings.json`\n4. Restart Claude Code\n\nYou'll get the same functionality with 80-90% less token usage.\n\n### Migrating to Minimal MCP (If Available)\n\nIf you find approval prompts annoying for certain commands:\n\n1. Replace full MCP with minimal MCP in `~/.claude/settings.json`\n2. Restart Claude Code\n3. Verify high-frequency commands (ready, show, list) work via MCP\n4. Everything else automatically uses CLI\n\nYou'll get 60-80% token reduction vs full MCP while keeping the UX benefits.\n```\n\n## Files to Update\n\n- `README.md` - Add recommendation in AI Integration section\n- `AGENTS.md` - Add \"Choosing Your Integration Approach\" section early\n- `QUICKSTART.md` - Update AI integration section\n- `docs/` - Any other AI integration docs if they exist\n- `mcp-server/` - Create minimal variant if hybrid validated\n\n## Future: Update `bd init`\n\nOnce validated, update `bd init` to:\n- Default to recommending `bd setup claude` (hooks only)\n- Mention minimal MCP as option for UX improvement\n- Detect existing full MCP and suggest migration\n- Provide token usage estimates for each approach\n\n## MCP Server Architecture Note\n\n**Key insight:** MCP server doesn't have to expose all bd functionality.\n\nCurrent design exposes ~20+ commands (all bd subcommands). This is over-engineered.\n\n**Better design:**\n- **Minimal MCP**: 3-5 read-only commands (~2-4k tokens)\n- **CLI**: Everything else via Bash tool\n- **Hooks**: Context injection via `bd prime`\n\nThis achieves best of both worlds:\n- Low token usage (efficient)\n- No approval prompts for common queries (UX)\n- Explicit visibility for state changes (safety)\n\nIf validation shows NO meaningful benefit to MCP (even minimal), skip hybrid mode entirely and recommend pure CLI.","acceptance_criteria":"- Documentation explains CLI + hooks as recommended approach\n- Explains why context size matters (compute/energy/cost/latency)\n- Token comparison table shows 80-90% reduction\n- Migration guide for existing MCP users\n- Only deployed AFTER production validation\n- Clear that both approaches are supported","status":"open","priority":2,"issue_type":"task","created_at":"2025-11-12T00:15:25.923025-08:00","updated_at":"2025-11-12T00:18:16.786857-08:00","source_repo":"."} {"id":"bd-ybv5","content_hash":"52f6d2143a3e9d63937e7dee2cfb4055740132d3c0831c3e948210179336820f","title":"Refactor AGENTS.md to use external references","description":"Suggestion to use external references (e.g., \"ALWAYS REFER TO ./beads/prompt.md\") instead of including all instructions directly within AGENTS.md.\nReasons:\n1. Agents can follow external references.\n2. Prevents context pollution/stuffing in AGENTS.md as more tools append instructions.\n","status":"open","priority":3,"issue_type":"task","created_at":"2025-11-20T18:55:53.259144-05:00","updated_at":"2025-11-20T18:55:53.259144-05:00","source_repo":"."} {"id":"bd-ye0d","content_hash":"40962ef4e144b58167a07ae13458b40cedff3f3549fccab3a172ca908cd754bc","title":"troubleshoot GH#278 daemon exits every few secs","description":"","status":"open","priority":2,"issue_type":"task","created_at":"2025-11-13T06:27:23.39509215-07:00","updated_at":"2025-11-13T06:27:23.39509215-07:00","source_repo":"."} {"id":"bd-z3s3","content_hash":"24d99dc1a9a5f35af962137f5709d4b0f1b6a9ec91511c30a2517d790640cce8","title":"Create deployment scripts for GCP","description":"Automated provisioning scripts for GCP Compute Engine deployment.\n\nAcceptance Criteria:\n- Terraform/gcloud scripts\n- Static IP allocation\n- Firewall rules\n- NGINX reverse proxy config\n- TLS setup (Let's Encrypt)\n- Systemd service file\n\nFile: deployment/agent-mail/gcp/","status":"closed","priority":3,"issue_type":"task","created_at":"2025-11-07T22:43:43.294839-08:00","updated_at":"2025-11-23T23:38:58.545006-08:00","closed_at":"2025-11-23T23:38:58.545006-08:00","source_repo":".","dependencies":[{"issue_id":"bd-z3s3","depends_on_id":"bd-9li4","type":"blocks","created_at":"2025-11-07T23:04:27.982336-08:00","created_by":"daemon"}]} +{"id":"bd-z8z","content_hash":"36495ab2605d6146dd821bc2d90910b8390790b6e5c7245c58f03b0c208cf3d1","title":"Apply wrapDBError to queries.go","description":"","status":"closed","priority":2,"issue_type":"task","created_at":"2025-11-24T00:53:14.298177-08:00","updated_at":"2025-11-24T00:55:37.364663-08:00","closed_at":"2025-11-24T00:55:37.364663-08:00","source_repo":"."} {"id":"bd-zj8e","content_hash":"655c761aaf4d5b0c9edfba7d96d23e608de94760148715667738d35c2033e110","title":"Performance Testing Documentation","description":"Create docs/performance-testing.md documenting the performance testing framework.\n\nSections:\n1. Overview - What the framework does, goals\n2. Running Benchmarks\n - make bench command\n - Running specific benchmarks\n - Interpreting output (ns/op, allocs/op)\n3. Profiling and Analysis\n - Viewing CPU profiles with pprof\n - Reading flamegraphs\n - Memory profiling\n - Finding hotspots\n4. User Diagnostics\n - bd doctor --perf usage\n - Sharing profiles with bug reports\n - Understanding the report output\n5. Comparing Performance\n - Using benchstat for before/after comparisons\n - Detecting regressions\n6. Tips for Optimization\n - Common patterns\n - When to profile vs benchmark\n\nStyle:\n- Concise, practical examples\n- Screenshots/examples of pprof output\n- Clear command-line examples\n- Focus on workflow, not theory","status":"open","priority":2,"issue_type":"task","created_at":"2025-11-13T22:23:38.99897-08:00","updated_at":"2025-11-13T22:23:38.99897-08:00","source_repo":"."} diff --git a/internal/storage/sqlite/config.go b/internal/storage/sqlite/config.go index 2e245f9c..0f06abdc 100644 --- a/internal/storage/sqlite/config.go +++ b/internal/storage/sqlite/config.go @@ -11,7 +11,7 @@ func (s *SQLiteStorage) SetConfig(ctx context.Context, key, value string) error INSERT INTO config (key, value) VALUES (?, ?) ON CONFLICT (key) DO UPDATE SET value = excluded.value `, key, value) - return err + return wrapDBError("set config", err) } // GetConfig gets a configuration value @@ -21,14 +21,14 @@ func (s *SQLiteStorage) GetConfig(ctx context.Context, key string) (string, erro if err == sql.ErrNoRows { return "", nil } - return value, err + return value, wrapDBError("get config", err) } // GetAllConfig gets all configuration key-value pairs func (s *SQLiteStorage) GetAllConfig(ctx context.Context) (map[string]string, error) { rows, err := s.db.QueryContext(ctx, `SELECT key, value FROM config ORDER BY key`) if err != nil { - return nil, err + return nil, wrapDBError("query all config", err) } defer func() { _ = rows.Close() }() @@ -36,17 +36,17 @@ func (s *SQLiteStorage) GetAllConfig(ctx context.Context) (map[string]string, er for rows.Next() { var key, value string if err := rows.Scan(&key, &value); err != nil { - return nil, err + return nil, wrapDBError("scan config row", err) } config[key] = value } - return config, rows.Err() + return config, wrapDBError("iterate config rows", rows.Err()) } // DeleteConfig deletes a configuration value func (s *SQLiteStorage) DeleteConfig(ctx context.Context, key string) error { _, err := s.db.ExecContext(ctx, `DELETE FROM config WHERE key = ?`, key) - return err + return wrapDBError("delete config", err) } // OrphanHandling defines how to handle orphan issues during import @@ -81,7 +81,7 @@ func (s *SQLiteStorage) SetMetadata(ctx context.Context, key, value string) erro INSERT INTO metadata (key, value) VALUES (?, ?) ON CONFLICT (key) DO UPDATE SET value = excluded.value `, key, value) - return err + return wrapDBError("set metadata", err) } // GetMetadata gets a metadata value (for internal state like import hashes) @@ -91,5 +91,5 @@ func (s *SQLiteStorage) GetMetadata(ctx context.Context, key string) (string, er if err == sql.ErrNoRows { return "", nil } - return value, err + return value, wrapDBError("get metadata", err) } diff --git a/internal/storage/sqlite/errors_test.go b/internal/storage/sqlite/errors_test.go new file mode 100644 index 00000000..990403e0 --- /dev/null +++ b/internal/storage/sqlite/errors_test.go @@ -0,0 +1,248 @@ +package sqlite + +import ( + "database/sql" + "errors" + "fmt" + "testing" +) + +// TestWrapDBError tests the wrapDBError function +func TestWrapDBError(t *testing.T) { + tests := []struct { + name string + op string + err error + wantNil bool + wantError string + wantType error + }{ + { + name: "nil error returns nil", + op: "test operation", + err: nil, + wantNil: true, + }, + { + name: "sql.ErrNoRows converted to ErrNotFound", + op: "get issue", + err: sql.ErrNoRows, + wantError: "get issue: not found", + wantType: ErrNotFound, + }, + { + name: "generic error wrapped with context", + op: "update issue", + err: errors.New("database locked"), + wantError: "update issue: database locked", + }, + { + name: "already wrapped error preserved", + op: "delete issue", + err: fmt.Errorf("constraint violation: %w", ErrConflict), + wantError: "delete issue: constraint violation: conflict", + wantType: ErrConflict, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := wrapDBError(tt.op, tt.err) + + if tt.wantNil { + if result != nil { + t.Errorf("wrapDBError() = %v, want nil", result) + } + return + } + + if result == nil { + t.Fatal("wrapDBError() returned nil, want error") + } + + if tt.wantError != "" && result.Error() != tt.wantError { + t.Errorf("wrapDBError() error = %q, want %q", result.Error(), tt.wantError) + } + + if tt.wantType != nil && !errors.Is(result, tt.wantType) { + t.Errorf("wrapDBError() error doesn't wrap %v", tt.wantType) + } + }) + } +} + +// TestWrapDBErrorf tests the wrapDBErrorf function +func TestWrapDBErrorf(t *testing.T) { + tests := []struct { + name string + err error + format string + args []interface{} + wantNil bool + wantError string + wantType error + }{ + { + name: "nil error returns nil", + err: nil, + format: "operation %s on %s", + args: []interface{}{"update", "issue-123"}, + wantNil: true, + }, + { + name: "sql.ErrNoRows converted to ErrNotFound with formatting", + err: sql.ErrNoRows, + format: "get issue %s", + args: []interface{}{"bd-abc"}, + wantError: "get issue bd-abc: not found", + wantType: ErrNotFound, + }, + { + name: "generic error with formatted context", + err: errors.New("timeout"), + format: "query %s with filter %s", + args: []interface{}{"issues", "status=open"}, + wantError: "query issues with filter status=open: timeout", + }, + { + name: "multiple format args", + err: errors.New("invalid value"), + format: "update %s field %s to %v", + args: []interface{}{"issue-123", "priority", 1}, + wantError: "update issue-123 field priority to 1: invalid value", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := wrapDBErrorf(tt.err, tt.format, tt.args...) + + if tt.wantNil { + if result != nil { + t.Errorf("wrapDBErrorf() = %v, want nil", result) + } + return + } + + if result == nil { + t.Fatal("wrapDBErrorf() returned nil, want error") + } + + if tt.wantError != "" && result.Error() != tt.wantError { + t.Errorf("wrapDBErrorf() error = %q, want %q", result.Error(), tt.wantError) + } + + if tt.wantType != nil && !errors.Is(result, tt.wantType) { + t.Errorf("wrapDBErrorf() error doesn't wrap %v", tt.wantType) + } + }) + } +} + +// TestSentinelErrors tests the sentinel error constants +func TestSentinelErrors(t *testing.T) { + tests := []struct { + name string + err error + check func(error) bool + want bool + }{ + { + name: "ErrNotFound detected by IsNotFound", + err: ErrNotFound, + check: IsNotFound, + want: true, + }, + { + name: "wrapped ErrNotFound detected", + err: fmt.Errorf("get issue: %w", ErrNotFound), + check: IsNotFound, + want: true, + }, + { + name: "other error not detected as ErrNotFound", + err: errors.New("other error"), + check: IsNotFound, + want: false, + }, + { + name: "ErrConflict detected by IsConflict", + err: ErrConflict, + check: IsConflict, + want: true, + }, + { + name: "wrapped ErrConflict detected", + err: fmt.Errorf("unique constraint: %w", ErrConflict), + check: IsConflict, + want: true, + }, + { + name: "ErrCycle detected by IsCycle", + err: ErrCycle, + check: IsCycle, + want: true, + }, + { + name: "wrapped ErrCycle detected", + err: fmt.Errorf("dependency check: %w", ErrCycle), + check: IsCycle, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.check(tt.err) + if got != tt.want { + t.Errorf("check(%v) = %v, want %v", tt.err, got, tt.want) + } + }) + } +} + +// TestErrorChaining tests that error chains are preserved through operations +func TestErrorChaining(t *testing.T) { + // Create a chain: root -> middle -> top + root := errors.New("root cause") + middle := fmt.Errorf("middle layer: %w", root) + top := wrapDBError("top operation", middle) + + // Verify we can unwrap to each level + if !errors.Is(top, middle) { + t.Error("top error doesn't wrap middle error") + } + if !errors.Is(top, root) { + t.Error("top error doesn't wrap root error") + } + + // Verify error message includes all context + want := "top operation: middle layer: root cause" + if top.Error() != want { + t.Errorf("error message = %q, want %q", top.Error(), want) + } +} + +// TestSQLErrNoRowsConversion tests that sql.ErrNoRows is consistently converted +func TestSQLErrNoRowsConversion(t *testing.T) { + // Both wrapping functions should convert sql.ErrNoRows to ErrNotFound + err1 := wrapDBError("get config", sql.ErrNoRows) + err2 := wrapDBErrorf(sql.ErrNoRows, "get metadata %s", "key") + + if !IsNotFound(err1) { + t.Error("wrapDBError didn't convert sql.ErrNoRows to ErrNotFound") + } + if !IsNotFound(err2) { + t.Error("wrapDBErrorf didn't convert sql.ErrNoRows to ErrNotFound") + } + + // The conversion replaces sql.ErrNoRows with ErrNotFound (not wrapped together) + // This is intentional - we want a single, clean error type for "not found" conditions + // The error message should indicate the operation context + if err1.Error() != "get config: not found" { + t.Errorf("err1 message = %q, want %q", err1.Error(), "get config: not found") + } + if err2.Error() != "get metadata key: not found" { + t.Errorf("err2 message = %q, want %q", err2.Error(), "get metadata key: not found") + } +} diff --git a/internal/storage/sqlite/queries.go b/internal/storage/sqlite/queries.go index 67153f00..fa63f727 100644 --- a/internal/storage/sqlite/queries.go +++ b/internal/storage/sqlite/queries.go @@ -87,13 +87,13 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act // Generate hash-based ID with adaptive length based on database size (bd-ea2a13) generatedID, err := GenerateIssueID(ctx, conn, prefix, issue, actor) if err != nil { - return err + return wrapDBError("generate issue ID", err) } issue.ID = generatedID } else { // Validate that explicitly provided ID matches the configured prefix (bd-177) if err := ValidateIssueIDPrefix(issue.ID, prefix); err != nil { - return err + return wrapDBError("validate issue ID prefix", err) } // For hierarchical IDs (bd-a3f8e9.1), ensure parent exists @@ -115,17 +115,17 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act // Insert issue if err := insertIssue(ctx, conn, issue); err != nil { - return err + return wrapDBError("insert issue", err) } // Record creation event if err := recordCreatedEvent(ctx, conn, issue, actor); err != nil { - return err + return wrapDBError("record creation event", err) } // Mark issue as dirty for incremental export if err := markDirty(ctx, conn, issue.ID); err != nil { - return err + return wrapDBError("mark issue dirty", err) } // Commit the transaction @@ -370,7 +370,7 @@ func (s *SQLiteStorage) UpdateIssue(ctx context.Context, id string, updates map[ // Get old issue for event oldIssue, err := s.GetIssue(ctx, id) if err != nil { - return err + return wrapDBError("get issue for update", err) } if oldIssue == nil { return fmt.Errorf("issue %s not found", id) @@ -388,7 +388,7 @@ func (s *SQLiteStorage) UpdateIssue(ctx context.Context, id string, updates map[ // Validate field values if err := validateFieldUpdate(key, value); err != nil { - return err + return wrapDBError("validate field update", err) } setClauses = append(setClauses, fmt.Sprintf("%s = ?", key)) @@ -740,7 +740,7 @@ func (s *SQLiteStorage) DeleteIssue(ctx context.Context, id string) error { } if err := tx.Commit(); err != nil { - return err + return wrapDBError("commit delete transaction", err) } // REMOVED (bd-c7af): Counter sync after deletion - no longer needed with hash IDs @@ -777,7 +777,7 @@ func (s *SQLiteStorage) DeleteIssues(ctx context.Context, ids []string, cascade expandedIDs, err := s.resolveDeleteSet(ctx, tx, ids, idSet, cascade, force, result) if err != nil { - return nil, err + return nil, wrapDBError("resolve delete set", err) } inClause, args := buildSQLInClause(expandedIDs) @@ -835,7 +835,7 @@ func (s *SQLiteStorage) expandWithDependents(ctx context.Context, tx *sql.Tx, id func (s *SQLiteStorage) validateNoDependents(ctx context.Context, tx *sql.Tx, ids []string, idSet map[string]bool, result *DeleteIssuesResult) error { for _, id := range ids { if err := s.checkSingleIssueValidation(ctx, tx, id, idSet, result); err != nil { - return err + return wrapDBError("check dependents", err) } } return nil @@ -885,7 +885,7 @@ func (s *SQLiteStorage) trackOrphanedIssues(ctx context.Context, tx *sql.Tx, ids orphanSet := make(map[string]bool) for _, id := range ids { if err := s.collectOrphansForID(ctx, tx, id, idSet, orphanSet); err != nil { - return err + return wrapDBError("collect orphans", err) } } for orphanID := range orphanSet { diff --git a/internal/storage/sqlite/store.go b/internal/storage/sqlite/store.go index 93b952a6..86939668 100644 --- a/internal/storage/sqlite/store.go +++ b/internal/storage/sqlite/store.go @@ -149,7 +149,7 @@ func New(ctx context.Context, path string) (*SQLiteStorage, error) { if err := verifySchemaCompatibility(db); err != nil { // Schema probe failed - retry migrations once if retryErr := RunMigrations(db); retryErr != nil { - return nil, fmt.Errorf("migration retry failed after schema probe failure: %w (original: %v)", retryErr, err) + return nil, fmt.Errorf("migration retry failed after schema probe failure: %w (original: %w)", retryErr, err) } // Probe again after retry