diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 09e2e4b..744e15e 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -18,7 +18,7 @@ { "name": "claude-status-hub", "description": "Monitor what matters - PRs, music, custom alerts - right in your Claude Code statusline", - "version": "1.5.1", + "version": "1.5.2", "author": { "name": "Pavel Fadeev", "email": "pavel.fadeev@gmail.com" diff --git a/plugins/claude-status-hub/.claude-plugin/plugin.json b/plugins/claude-status-hub/.claude-plugin/plugin.json index eca4a2d..b2c67c9 100644 --- a/plugins/claude-status-hub/.claude-plugin/plugin.json +++ b/plugins/claude-status-hub/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "claude-status-hub", - "version": "1.5.1", + "version": "1.5.2", "description": "Monitor and act - PRs, calendar, music, custom alerts with contextual actions via /hub-ack", "author": { "name": "Pavel Fadeev", diff --git a/plugins/claude-status-hub/bin/refresh-daemon.sh b/plugins/claude-status-hub/bin/refresh-daemon.sh index 4ccd8a8..c580ae9 100755 --- a/plugins/claude-status-hub/bin/refresh-daemon.sh +++ b/plugins/claude-status-hub/bin/refresh-daemon.sh @@ -126,6 +126,20 @@ while true; do exit 0 fi + # Self-check: exit if another daemon took over the lockfile + # This handles the startup race condition: multiple sessions starting simultaneously + # all pass the initial lockfile check before any writes its PID. After one sleep cycle, + # only the last writer survives because all others see a mismatched lockfile. + # See docs/data-safety-guidelines.md for race condition prevention patterns. + if [ -f "$LOCKFILE" ]; then + CURRENT_LOCK=$(cat "$LOCKFILE" 2>/dev/null) + if [ "$CURRENT_LOCK" != "${PLUGIN_VERSION}:$$" ]; then + exit 0 # Another daemon owns the lock, gracefully exit + fi + else + exit 0 # Lockfile gone, exit + fi + [ -f "$CONFIG" ] || continue # Check if bridge needs refresh diff --git a/plugins/claude-status-hub/bin/refresh-prs.sh b/plugins/claude-status-hub/bin/refresh-prs.sh index 53761ca..1ce8d19 100755 --- a/plugins/claude-status-hub/bin/refresh-prs.sh +++ b/plugins/claude-status-hub/bin/refresh-prs.sh @@ -116,7 +116,14 @@ build_foreground() { done < <(jq -c '.foreground[] | select(.owner)' "$CONFIG" 2>/dev/null) [ -n "$updates" ] && jq "$updates" "$CONFIG" > "${CONFIG}.tmp" && mv "${CONFIG}.tmp" "$CONFIG" - echo "${result}]" + + # Preserve non-PR foreground items (Slack, Calendar, etc.) - see docs/data-safety-guidelines.md + NON_PR_ITEMS=$(jq -c '[.foreground[] | select(.owner | not)]' "$CONFIG" 2>/dev/null || echo '[]') + if [ "$NON_PR_ITEMS" != "[]" ] && [ "$NON_PR_ITEMS" != "null" ]; then + echo "${result}]" | jq --argjson nonpr "$NON_PR_ITEMS" '. + $nonpr' + else + echo "${result}]" + fi } FOREGROUND=$(build_foreground) diff --git a/plugins/claude-status-hub/commands/hub.md b/plugins/claude-status-hub/commands/hub.md index 4826aaa..f352295 100644 --- a/plugins/claude-status-hub/commands/hub.md +++ b/plugins/claude-status-hub/commands/hub.md @@ -62,7 +62,11 @@ gh pr view --repo / --json state,isDraft,reviewDecision,st - D = Draft - ✓ = Approved + all checks pass 4. Read `~/.claude/status-config.json` for lastSeen state -5. Update config with new PR(s) in `foreground` array +5. Update config with new PR(s) in `foreground` array: + - Check if PR already exists (match by owner/repo/number) + - If exists: update in place using jq `|=` operator + - If new: append to existing array with `+= [$new_item]` + - **NEVER replace the entire foreground array - always merge** (see `docs/data-safety-guidelines.md`) 6. Write bridge file `/tmp/status-hub.json` - set timestamp at root, preserve `background`, update `foreground` array: **CRITICAL:** Generate current timestamp in milliseconds: `$(($(date +%s) * 1000))` diff --git a/plugins/claude-status-hub/docs/data-safety-guidelines.md b/plugins/claude-status-hub/docs/data-safety-guidelines.md new file mode 100644 index 0000000..ba242d1 --- /dev/null +++ b/plugins/claude-status-hub/docs/data-safety-guidelines.md @@ -0,0 +1,102 @@ +# Data Safety Guidelines + +This document establishes patterns for safely updating the status hub's data files without data loss. + +## Core Principle + +**Never overwrite, always merge.** When updating a subset of data (e.g., just PRs or just calendar), preserve items from other services. + +## Bridge File Updates (`/tmp/status-hub.json`) + +The bridge file has three main sections: +- `timestamp`: Always update on write +- `background`: Music/service status +- `foreground`: Array of tracked items (PRs, calendar, Slack, etc.) + +### Correct Pattern: Merge Foreground Items + +When refreshing one service, preserve items from other services: + +```bash +# Get existing non-PR items before rebuilding PR list +NON_PR_ITEMS=$(jq -c '[.foreground[] | select(.owner | not)]' "$CONFIG" 2>/dev/null || echo '[]') + +# After building PR array, merge with non-PR items +echo "$pr_array" | jq --argjson nonpr "$NON_PR_ITEMS" '. + $nonpr' +``` + +### Wrong Pattern: Full Replacement + +```bash +# BAD: This loses Slack, calendar, and other items! +jq '.foreground = $prs' --argjson prs "$PR_ONLY_ARRAY" "$BRIDGE" +``` + +## Config File Updates (`~/.claude/status-config.json`) + +### Correct Pattern: Targeted Updates with `|=` + +```bash +# Update specific item by matching criteria +jq '(.foreground[] | select(.number == 123)) |= . + {lastSeen: {...}}' "$CONFIG" + +# Update background without touching foreground +jq '.background |= {service: "spotify", tabId: 123}' "$CONFIG" +``` + +### Wrong Pattern: Full Object Replacement + +```bash +# BAD: Replaces entire config, losing other sections! +echo '{"foreground": [...]}' > "$CONFIG" +``` + +## Adding New Items + +When adding a new tracked item: + +1. Read existing foreground array +2. Check if item already exists (match by unique key like `owner/repo/number`) +3. If exists: update in place with `|=` +4. If new: append with `+= [$new_item]` + +```bash +# Check if PR exists, then add or update +if jq -e ".foreground[] | select(.owner == \"$owner\" and .repo == \"$repo\" and .number == $number)" "$CONFIG" >/dev/null 2>&1; then + # Update existing + jq "(.foreground[] | select(.number == $number)) |= . + {lastSeen: {...}}" "$CONFIG" +else + # Append new + jq ".foreground += [{owner: \"$owner\", repo: \"$repo\", number: $number}]" "$CONFIG" +fi +``` + +## Checklist for Skill Authors + +Before submitting a skill that modifies hub data: + +- [ ] Does it preserve items from other services when updating foreground? +- [ ] Does it use `|=` for targeted updates instead of full replacement? +- [ ] Does it check for existing items before adding new ones? +- [ ] Does it preserve background when only updating foreground (and vice versa)? +- [ ] Does it handle missing files gracefully (create with sensible defaults)? + +## Race Condition Prevention + +When multiple processes might update the same file: + +1. Use atomic writes: write to `.tmp` file first, then `mv` to final location +2. For daemons: use lockfile with ownership verification (version:PID) +3. Check file staleness before modifying + +```bash +# Atomic write pattern +jq '...' "$CONFIG" > "${CONFIG}.tmp" && mv "${CONFIG}.tmp" "$CONFIG" +``` + +## Related Files + +- `bin/refresh-prs.sh` - PR-only refresh (preserves non-PR items) +- `bin/update-bridge.sh` - Bridge writer (preserves background/foreground appropriately) +- `bin/refresh-daemon.sh` - Background daemon with lockfile management +- `skills/hub-refresh.md` - Full refresh skill diff --git a/plugins/claude-status-hub/skills/hub-refresh.md b/plugins/claude-status-hub/skills/hub-refresh.md index 7615f80..736e3be 100644 --- a/plugins/claude-status-hub/skills/hub-refresh.md +++ b/plugins/claude-status-hub/skills/hub-refresh.md @@ -21,6 +21,15 @@ function sanitize(str, maxLen = 30) { ``` Limits: title 30 chars, detail 25 chars, artist 20 chars. +## Data Preservation + +**See `docs/data-safety-guidelines.md` for merge patterns.** + +Key rules: +- Never overwrite entire foreground array +- When refreshing one service, preserve items from other services +- Use `jq` merge operators (`+`, `|=`) not full replacement + ## Step 1: Read Config ```bash diff --git a/plugins/claude-status-hub/tests/test-daemon-version.sh b/plugins/claude-status-hub/tests/test-daemon-version.sh index 1d3f06f..18ac840 100755 --- a/plugins/claude-status-hub/tests/test-daemon-version.sh +++ b/plugins/claude-status-hub/tests/test-daemon-version.sh @@ -210,6 +210,27 @@ fi # Cleanup daemon kill "$DAEMON_PID" 2>/dev/null || true +# --- Test 9: Self-eviction when lockfile ownership changes --- +# This tests the race condition fix: if another daemon takes over the lockfile, +# the current daemon should exit gracefully (see docs/data-safety-guidelines.md) +echo "" +echo "Test: Daemon self-evicts when lockfile ownership changes" +rm -f "$LOCKFILE" +CURRENT_V=$(jq -r '.version' "$PLUGIN_JSON") + +# Simulate the lockfile ownership check logic from refresh-daemon.sh +# A daemon with PID 12345 wrote the lockfile +echo "${CURRENT_V}:12345" > "$LOCKFILE" + +# Another daemon (PID 99999) checks if it owns the lockfile +CURRENT_LOCK=$(cat "$LOCKFILE" 2>/dev/null) +MY_EXPECTED="${CURRENT_V}:99999" +if [ "$CURRENT_LOCK" != "$MY_EXPECTED" ]; then + pass "Lockfile ownership mismatch detected (self-eviction trigger)" +else + fail "Lockfile ownership check" "mismatch detected" "false match" +fi + echo "" echo "=== Results ===" echo "Passed: $TESTS_PASSED" diff --git a/plugins/claude-status-hub/tests/test-refresh-prs.sh b/plugins/claude-status-hub/tests/test-refresh-prs.sh index eea9452..33b6fb1 100755 --- a/plugins/claude-status-hub/tests/test-refresh-prs.sh +++ b/plugins/claude-status-hub/tests/test-refresh-prs.sh @@ -505,6 +505,45 @@ else fail "Bridge structure" "spotify bg + 1 fg" "$bg_site + $fg_count fg" fi +# Test non-PR foreground items are preserved (see docs/data-safety-guidelines.md) +TESTS_RUN=$((TESTS_RUN + 1)) +# Setup config with both PR and non-PR items +cat > "$CONFIG" << EOF +{ + "foreground": [ + { + "owner": "test", + "repo": "repo", + "number": 1, + "lastSeen": {} + }, + { + "service": "slack", + "icon": "S", + "title": "3 unread", + "detail": "#general" + }, + { + "service": "calendar", + "icon": "📅", + "title": "Meeting", + "detail": "in 10m" + } + ] +} +EOF +create_mock_gh "OPEN" "false" "APPROVED" "MERGEABLE" 0 0 0 +run_refresh +fg_count=$(jq -r '.foreground | length' "$BRIDGE") +slack_exists=$(jq -r '.foreground[] | select(.service == "slack") | .title' "$BRIDGE") +calendar_exists=$(jq -r '.foreground[] | select(.service == "calendar") | .title' "$BRIDGE") +pr_exists=$(jq -r '.foreground[] | select(.site == "github-pr") | .title' "$BRIDGE") +if [ "$fg_count" = "3" ] && [ "$slack_exists" = "3 unread" ] && [ "$calendar_exists" = "Meeting" ] && [ -n "$pr_exists" ]; then + pass "Non-PR foreground items preserved (Slack, Calendar)" +else + fail "Non-PR preservation" "3 items (PR + Slack + Calendar)" "$fg_count items, slack='$slack_exists', calendar='$calendar_exists'" +fi + echo "" echo "Testing auto-merge functionality..."