From 0b0d9a43d102893ed2e45e9a8284c3d8571b7c39 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Thu, 6 Nov 2025 19:01:28 -0800 Subject: [PATCH] Improve bd-my64 fix based on oracle review Oracle identified a critical race condition in the initial fix: - Pre-push hook checked for changes but didn't flush first - Pending 5s-debounced flushes could land after the check - Result: stale JSONL could still be pushed Improvements: 1. Pre-push now flushes pending changes FIRST (bd sync --flush-only) 2. Uses git status --porcelain to catch ALL change types: - Staged, unstaged, untracked, deleted, renamed, conflicts 3. Handles both beads.jsonl and issues.jsonl (backward compat) 4. Works even without bd installed (git-only check) 5. Pre-commit stages both JSONL files (simpler loop) This completely eliminates the race condition. --- .beads/beads.jsonl | 2 +- examples/git-hooks/README.md | 14 +++++---- examples/git-hooks/pre-commit | 17 ++++------- examples/git-hooks/pre-push | 54 +++++++++++++++++++---------------- 4 files changed, 44 insertions(+), 43 deletions(-) diff --git a/.beads/beads.jsonl b/.beads/beads.jsonl index 58e23c99..1f5d3493 100644 --- a/.beads/beads.jsonl +++ b/.beads/beads.jsonl @@ -243,7 +243,7 @@ {"id":"bd-lwnt","content_hash":"ddfa247870eb3734ffa7a4d0da6fcd4a359d2b48e02d70aad8560ec4bc13afdc","title":"Test P1 priority","description":"","status":"closed","priority":1,"issue_type":"task","created_at":"2025-11-05T12:58:38.074112-08:00","updated_at":"2025-11-05T12:58:44.711763-08:00","closed_at":"2025-11-05T12:58:44.711763-08:00","source_repo":"."} {"id":"bd-mlcz","content_hash":"35434fed7e20e8b67f7b598c46ce5dc65c111de237ea38b36a88a671b023b58b","title":"Implement bd migrate command","description":"Add bd migrate command to move issues between repos with filtering. Should support: filtering by status/priority/labels, dry-run mode, preserving dependencies, handling source_repo field updates.","status":"closed","priority":1,"issue_type":"task","created_at":"2025-11-05T18:04:29.902151-08:00","updated_at":"2025-11-05T18:42:52.536951-08:00","closed_at":"2025-11-05T18:42:52.536951-08:00","source_repo":".","dependencies":[{"issue_id":"bd-mlcz","depends_on_id":"bd-8rd","type":"parent-child","created_at":"2025-11-05T18:04:39.072312-08:00","created_by":"daemon"}]} {"id":"bd-mn9p","content_hash":"89752e08d5a278ed301c655753e07bc2b564e710ce6d9e7e535bbd9ec48af182","title":"bd-hv01: Brittle string comparison breaks with JSON field reordering","description":"## Problem\ndeletion_tracking.go:125 uses string comparison to detect unchanged issues:\n\n```go\nif leftLine, existsInLeft := leftIndex[id]; existsInLeft \u0026\u0026 leftLine == baseLine {\n deletions = append(deletions, id)\n}\n```\n\nThis breaks if:\n- JSON field order changes (legal in JSON)\n- Timestamps updated by import/export\n- Whitespace/formatting changes\n- Floating point precision varies\n\n## Example Failure\n```json\n// baseLine\n{\"id\":\"bd-1\",\"priority\":1,\"status\":\"open\"}\n// leftLine (same data, different order)\n{\"id\":\"bd-1\",\"status\":\"open\",\"priority\":1}\n```\nThese are semantically identical but string comparison fails.\n\n## Fix\nParse and compare JSON semantically:\n```go\nfunc jsonEquals(a, b string) bool {\n var objA, objB map[string]interface{}\n json.Unmarshal([]byte(a), \u0026objA)\n json.Unmarshal([]byte(b), \u0026objB)\n return reflect.DeepEqual(objA, objB)\n}\n```\n\n## Files Affected\n- cmd/bd/deletion_tracking.go:125\n- cmd/bd/deletion_tracking.go:134-170 (buildIDToLineMap)","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-11-06T18:15:35.090716-08:00","updated_at":"2025-11-06T18:46:55.889888-08:00","closed_at":"2025-11-06T18:46:55.889888-08:00","source_repo":".","dependencies":[{"issue_id":"bd-mn9p","depends_on_id":"bd-rbxi","type":"parent-child","created_at":"2025-11-06T18:19:14.790898-08:00","created_by":"daemon"}]} -{"id":"bd-my64","content_hash":"5ce1e4594cd5eede3f8eb0dbc1c903d90fd46ae7805179688d9425472b711311","title":"Pre-push hook and daemon export produce different JSONL","description":"After committing and pushing, git status shows .beads/beads.jsonl as dirty. Investigation shows:\n\n1. Pre-push hook ran successfully and exported DB → JSONL\n2. Push completed\n3. Shortly after, daemon exported DB → JSONL again with different content\n4. Diff shows comments added to old issues (bd-23a8, bd-6049, bd-87a0)\n\nTimeline:\n- Commit c731c45 \"Update beads JSONL\"\n- Pre-push hook exported JSONL\n- Push succeeded\n- Daemon PID 33314 exported again with different content\n\nQuestions:\n1. Did someone run a command between commit and daemon export?\n2. Is there a timing issue where pre-push hook doesn't capture all DB changes?\n3. Should pre-commit hook flush daemon changes before committing?\n\nThe comments appear to be from Nov 5 (created_at: 2025-11-05T08:38:46Z) but are only appearing in JSONL now. This suggests the DB had these comments but they weren't exported during pre-push.\n\nPossible causes:\n- Pre-push hook uses BEADS_NO_DAEMON=1 which might skip pending writes\n- Daemon has unflushed changes in memory\n- Race condition between pre-push export and daemon's periodic export","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-11-06T18:49:54.570993-08:00","updated_at":"2025-11-06T18:57:42.710282-08:00","closed_at":"2025-11-06T18:57:42.710282-08:00","source_repo":"."} +{"id":"bd-my64","content_hash":"8f4eb8056f81096e7090813f319b3aa996ada6dc5809d81305271d0584c2f364","title":"Pre-push hook and daemon export produce different JSONL","description":"After committing and pushing, git status shows .beads/beads.jsonl as dirty. Investigation shows:\n\n1. Pre-push hook ran successfully and exported DB → JSONL\n2. Push completed\n3. Shortly after, daemon exported DB → JSONL again with different content\n4. Diff shows comments added to old issues (bd-23a8, bd-6049, bd-87a0)\n\nTimeline:\n- Commit c731c45 \"Update beads JSONL\"\n- Pre-push hook exported JSONL\n- Push succeeded\n- Daemon PID 33314 exported again with different content\n\nQuestions:\n1. Did someone run a command between commit and daemon export?\n2. Is there a timing issue where pre-push hook doesn't capture all DB changes?\n3. Should pre-commit hook flush daemon changes before committing?\n\nThe comments appear to be from Nov 5 (created_at: 2025-11-05T08:38:46Z) but are only appearing in JSONL now. This suggests the DB had these comments but they weren't exported during pre-push.\n\nPossible causes:\n- Pre-push hook uses BEADS_NO_DAEMON=1 which might skip pending writes\n- Daemon has unflushed changes in memory\n- Race condition between pre-push export and daemon's periodic export","notes":"Improved fix based on oracle code review:\n1. Pre-push now flushes pending changes first (prevents debounce race)\n2. Uses git status --porcelain to catch all change types\n3. Handles both beads.jsonl and issues.jsonl\n4. Works even if bd not installed (git-only check)","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-11-06T18:49:54.570993-08:00","updated_at":"2025-11-06T19:01:14.549032-08:00","closed_at":"2025-11-06T18:57:42.710282-08:00","source_repo":"."} {"id":"bd-ng56","content_hash":"82c7d4d6f8a2fa73193de02e52de172f7325f38c02e4b9d397aec4833234ff5f","title":"bd-hv01: Three full JSONL reads on every sync (performance)","description":"Problem: computeAcceptedDeletions reads three JSONL files completely into memory (base, left, merged). For 1000 issues at 1KB each, this is 3MB read and 3000 JSON parse operations.\n\nImpact: Acceptable now (~20-35ms overhead) but will be slow for large repos (10k+ issues).\n\nPossible optimizations: single-pass streaming, memory-mapped files, binary format, incremental snapshots.\n\nFiles: cmd/bd/deletion_tracking.go:101-208","status":"open","priority":3,"issue_type":"task","created_at":"2025-11-06T18:16:25.653076-08:00","updated_at":"2025-11-06T18:16:25.653076-08:00","source_repo":".","dependencies":[{"issue_id":"bd-ng56","depends_on_id":"bd-rbxi","type":"parent-child","created_at":"2025-11-06T18:19:15.148149-08:00","created_by":"daemon"}]} {"id":"bd-nqes","content_hash":"d7ca10b54b92c464a0c4c1c932f2918a6e0e952a5fd7337acb9160cd524dc59a","title":"bd-hv01: Non-atomic snapshot operations can cause data loss","description":"## Problem\nIn sync.go:146-155 and daemon_sync.go:502-505, snapshot capture failures are logged as warnings but sync continues:\n\n```go\nif err := exportToJSONL(ctx, jsonlPath); err != nil { ... }\nif err := captureLeftSnapshot(jsonlPath); err != nil {\n fmt.Fprintf(os.Stderr, \"Warning: failed to capture snapshot...\")\n}\n```\n\nIf export succeeds but snapshot capture fails, the merge uses stale snapshot data, potentially deleting wrong issues.\n\n## Impact\n- Critical data integrity issue\n- Could delete issues incorrectly during multi-workspace sync\n\n## Fix\nMake snapshot capture mandatory:\n```go\nif err := captureLeftSnapshot(jsonlPath); err != nil {\n return fmt.Errorf(\"failed to capture snapshot (required for deletion tracking): %w\", err)\n}\n```\n\n## Files Affected\n- cmd/bd/sync.go:146-155\n- cmd/bd/daemon_sync.go:502-505","status":"closed","priority":1,"issue_type":"bug","created_at":"2025-11-06T18:15:33.574158-08:00","updated_at":"2025-11-06T18:46:55.874814-08:00","closed_at":"2025-11-06T18:46:55.874814-08:00","source_repo":".","dependencies":[{"issue_id":"bd-nqes","depends_on_id":"bd-rbxi","type":"parent-child","created_at":"2025-11-06T18:19:14.749153-08:00","created_by":"daemon"}]} {"id":"bd-o43","content_hash":"4caa0f14a58127378a533362ec0292833b6d59195e503fab7505180c9c5c0438","title":"Add richer query capabilities to bd list","description":"Current bd list filters are limited to basic field matching (status, priority, type, assignee, label). This forces users to resort to piping through jq for common queries.\n\nMissing query capabilities:\n- Pattern matching: --title-contains, --desc-contains\n- Date ranges: --created-after, --updated-before, --closed-after\n- Empty/null checks: --empty-description, --no-assignee, --no-labels\n- Numeric ranges: --priority-min, --priority-max\n- Complex boolean logic: --and, --or operators\n- Full-text search: --search across all text fields\n- Negation: --not-status, --exclude-label\n\nExample use cases:\n- Find issues with empty descriptions\n- Find stale issues not updated in 30 days\n- Find high-priority bugs with no assignee\n- Search for keyword across title/description/notes\n\nImplementation approach:\n- Add query builder pattern to storage layer\n- Support --query DSL for complex queries\n- Keep simple flags for common cases\n- Add --json output for programmatic use","notes":"## Progress Update\n\n**Completed:**\n- ✅ Extended IssueFilter struct with new fields (pattern matching, date ranges, empty/null checks, priority ranges)\n- ✅ Updated SQLite SearchIssues implementation \n- ✅ Added CLI flags to list.go\n- ✅ Added parseTimeFlag helper\n- ✅ Comprehensive tests added - all passing\n\n**Remaining:**\n- ⚠️ RPC layer needs updating (internal/rpc/protocol.go ListArgs)\n- ⚠️ Daemon handler needs to forward new filters\n- ⚠️ End-to-end testing with daemon mode\n- 📝 Documentation updates\n\n**Files Modified:**\n- internal/types/types.go\n- internal/storage/sqlite/sqlite.go \n- cmd/bd/list.go\n- cmd/bd/list_test.go\n\n**Next Steps:**\n1. Update RPC protocol\n2. Update daemon handler \n3. Test with daemon mode\n4. Update docs","status":"closed","priority":2,"issue_type":"feature","created_at":"2025-11-05T00:17:48.677493-08:00","updated_at":"2025-11-05T00:33:38.998433-08:00","closed_at":"2025-11-05T00:33:38.998433-08:00","source_repo":"."} diff --git a/examples/git-hooks/README.md b/examples/git-hooks/README.md index 13b3c4c7..76a6d875 100644 --- a/examples/git-hooks/README.md +++ b/examples/git-hooks/README.md @@ -69,18 +69,20 @@ The hook is silent on success, fast (no git operations), and safe (fails commit ### pre-push -Before each push, the hook checks: +Before each push, the hook: ```bash -git diff --quiet .beads/beads.jsonl +bd sync --flush-only # Flush pending changes (if bd available) +git status --porcelain .beads/*.jsonl # Check for uncommitted changes ``` This prevents pushing stale JSONL by: -1. Checking if JSONL has uncommitted changes (working tree or staging) -2. Failing the push with clear error message if changes exist -3. Instructing user to commit JSONL before pushing again +1. Flushing pending in-memory changes from daemon's 5s debounce +2. Checking for uncommitted changes (staged, unstaged, untracked, deleted) +3. Failing the push with clear error message if changes exist +4. Instructing user to commit JSONL before pushing again -This solves bd-my64: changes made between commit and push are caught before reaching remote. +This solves bd-my64: changes made between commit and push (or pending debounced flushes) are caught before reaching remote. ### post-merge diff --git a/examples/git-hooks/pre-commit b/examples/git-hooks/pre-commit index 2d8fe905..556c5f2a 100755 --- a/examples/git-hooks/pre-commit +++ b/examples/git-hooks/pre-commit @@ -35,17 +35,10 @@ if ! bd sync --flush-only >/dev/null 2>&1; then exit 1 fi -# Find the JSONL file (could be issues.jsonl or beads.jsonl) -JSONL_FILE="" -if [ -f .beads/beads.jsonl ]; then - JSONL_FILE=".beads/beads.jsonl" -elif [ -f .beads/issues.jsonl ]; then - JSONL_FILE=".beads/issues.jsonl" -fi - -# If the JSONL file was modified, stage it -if [ -n "$JSONL_FILE" ] && [ -f "$JSONL_FILE" ]; then - git add "$JSONL_FILE" 2>/dev/null || true -fi +# Stage both possible JSONL files (backward compatibility) +# git add is harmless if file doesn't exist +for f in .beads/beads.jsonl .beads/issues.jsonl; do + [ -f "$f" ] && git add "$f" 2>/dev/null || true +done exit 0 diff --git a/examples/git-hooks/pre-push b/examples/git-hooks/pre-push index 11780169..fa61e0a4 100755 --- a/examples/git-hooks/pre-push +++ b/examples/git-hooks/pre-push @@ -3,11 +3,14 @@ # # bd (beads) pre-push hook # -# This hook checks if the database has uncommitted changes and prevents -# pushing stale JSONL by failing with a clear error message. +# This hook prevents pushing stale JSONL by: +# 1. Flushing any pending in-memory changes to JSONL (if bd available) +# 2. Checking for uncommitted changes (staged, unstaged, untracked, deleted) +# 3. Failing the push with clear instructions if changes found # -# The pre-commit hook should have already exported changes, but if new -# changes were made between commit and push, this hook catches that. +# The pre-commit hook already exports changes, but this catches: +# - Changes made between commit and push +# - Pending debounced flushes (5s daemon delay) # # Installation: # cp examples/git-hooks/pre-push .git/hooks/pre-push @@ -16,37 +19,40 @@ # Or use the install script: # examples/git-hooks/install.sh -# Check if bd is available -if ! command -v bd >/dev/null 2>&1; then - # If bd is not available, we can't check - allow push - exit 0 -fi - # Check if we're in a bd workspace if [ ! -d .beads ]; then # Not a bd workspace, nothing to do exit 0 fi -# Find the JSONL file (could be issues.jsonl or beads.jsonl) -JSONL_FILE="" -if [ -f .beads/beads.jsonl ]; then - JSONL_FILE=".beads/beads.jsonl" -elif [ -f .beads/issues.jsonl ]; then - JSONL_FILE=".beads/issues.jsonl" +# Optionally flush pending bd changes so they surface in JSONL +# This prevents the race where a debounced flush lands after the check +if command -v bd >/dev/null 2>&1; then + bd sync --flush-only >/dev/null 2>&1 || true fi -# Check if JSONL file has uncommitted changes -if [ -n "$JSONL_FILE" ] && [ -f "$JSONL_FILE" ]; then - # Check git status for the JSONL file - if ! git diff --quiet "$JSONL_FILE" 2>/dev/null || ! git diff --cached --quiet "$JSONL_FILE" 2>/dev/null; then - echo "❌ Error: $JSONL_FILE has uncommitted changes" >&2 +# Collect all tracked or existing JSONL files (supports both old and new names) +FILES="" +for f in .beads/beads.jsonl .beads/issues.jsonl; do + # Include file if it exists in working tree OR is tracked by git (even if deleted) + if git ls-files --error-unmatch "$f" >/dev/null 2>&1 || [ -f "$f" ]; then + FILES="$FILES $f" + fi +done + +# Check for any uncommitted changes using porcelain status +# This catches: staged, unstaged, untracked, deleted, renamed, and conflicts +if [ -n "$FILES" ]; then + # shellcheck disable=SC2086 + if [ -n "$(git status --porcelain -- $FILES 2>/dev/null)" ]; then + echo "❌ Error: Beads JSONL has uncommitted changes" >&2 echo "" >&2 echo "You made changes to bd issues between your last commit and this push." >&2 - echo "Please commit the updated JSONL file before pushing:" >&2 + echo "Please commit the updated JSONL before pushing:" >&2 echo "" >&2 - echo " git add $JSONL_FILE" >&2 - echo " git commit -m \"Update beads JSONL\"" >&2 + # shellcheck disable=SC2086 + echo " git add $FILES" >&2 + echo ' git commit -m "Update bd JSONL"' >&2 echo " git push" >&2 echo "" >&2 exit 1