Skip to content

Commit 1668e4f

Browse files
authored
Fix data overwrite issues in status hub refresh (#46)
- Add data-safety-guidelines.md documenting merge patterns - Fix refresh-prs.sh to preserve non-PR foreground items (Slack, Calendar) - Add daemon lockfile ownership check to fix startup race condition - Clarify hub.md with explicit merge instructions for PR tracking - Add data preservation reference in hub-refresh.md - Add tests for non-PR preservation and daemon self-eviction - Bump version to 1.5.2
1 parent 859915d commit 1668e4f

9 files changed

Lines changed: 200 additions & 4 deletions

File tree

.claude-plugin/marketplace.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
{
1919
"name": "claude-status-hub",
2020
"description": "Monitor what matters - PRs, music, custom alerts - right in your Claude Code statusline",
21-
"version": "1.5.1",
21+
"version": "1.5.2",
2222
"author": {
2323
"name": "Pavel Fadeev",
2424
"email": "pavel.fadeev@gmail.com"

plugins/claude-status-hub/.claude-plugin/plugin.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "claude-status-hub",
3-
"version": "1.5.1",
3+
"version": "1.5.2",
44
"description": "Monitor and act - PRs, calendar, music, custom alerts with contextual actions via /hub-ack",
55
"author": {
66
"name": "Pavel Fadeev",

plugins/claude-status-hub/bin/refresh-daemon.sh

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,20 @@ while true; do
126126
exit 0
127127
fi
128128

129+
# Self-check: exit if another daemon took over the lockfile
130+
# This handles the startup race condition: multiple sessions starting simultaneously
131+
# all pass the initial lockfile check before any writes its PID. After one sleep cycle,
132+
# only the last writer survives because all others see a mismatched lockfile.
133+
# See docs/data-safety-guidelines.md for race condition prevention patterns.
134+
if [ -f "$LOCKFILE" ]; then
135+
CURRENT_LOCK=$(cat "$LOCKFILE" 2>/dev/null)
136+
if [ "$CURRENT_LOCK" != "${PLUGIN_VERSION}:$$" ]; then
137+
exit 0 # Another daemon owns the lock, gracefully exit
138+
fi
139+
else
140+
exit 0 # Lockfile gone, exit
141+
fi
142+
129143
[ -f "$CONFIG" ] || continue
130144

131145
# Check if bridge needs refresh

plugins/claude-status-hub/bin/refresh-prs.sh

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,14 @@ build_foreground() {
116116
done < <(jq -c '.foreground[] | select(.owner)' "$CONFIG" 2>/dev/null)
117117

118118
[ -n "$updates" ] && jq "$updates" "$CONFIG" > "${CONFIG}.tmp" && mv "${CONFIG}.tmp" "$CONFIG"
119-
echo "${result}]"
119+
120+
# Preserve non-PR foreground items (Slack, Calendar, etc.) - see docs/data-safety-guidelines.md
121+
NON_PR_ITEMS=$(jq -c '[.foreground[] | select(.owner | not)]' "$CONFIG" 2>/dev/null || echo '[]')
122+
if [ "$NON_PR_ITEMS" != "[]" ] && [ "$NON_PR_ITEMS" != "null" ]; then
123+
echo "${result}]" | jq --argjson nonpr "$NON_PR_ITEMS" '. + $nonpr'
124+
else
125+
echo "${result}]"
126+
fi
120127
}
121128

122129
FOREGROUND=$(build_foreground)

plugins/claude-status-hub/commands/hub.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,11 @@ gh pr view <number> --repo <owner>/<repo> --json state,isDraft,reviewDecision,st
6262
- D = Draft
6363
- ✓ = Approved + all checks pass
6464
4. Read `~/.claude/status-config.json` for lastSeen state
65-
5. Update config with new PR(s) in `foreground` array
65+
5. Update config with new PR(s) in `foreground` array:
66+
- Check if PR already exists (match by owner/repo/number)
67+
- If exists: update in place using jq `|=` operator
68+
- If new: append to existing array with `+= [$new_item]`
69+
- **NEVER replace the entire foreground array - always merge** (see `docs/data-safety-guidelines.md`)
6670
6. Write bridge file `/tmp/status-hub.json` - set timestamp at root, preserve `background`, update `foreground` array:
6771

6872
**CRITICAL:** Generate current timestamp in milliseconds: `$(($(date +%s) * 1000))`
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
# Data Safety Guidelines
2+
3+
This document establishes patterns for safely updating the status hub's data files without data loss.
4+
5+
## Core Principle
6+
7+
**Never overwrite, always merge.** When updating a subset of data (e.g., just PRs or just calendar), preserve items from other services.
8+
9+
## Bridge File Updates (`/tmp/status-hub.json`)
10+
11+
The bridge file has three main sections:
12+
- `timestamp`: Always update on write
13+
- `background`: Music/service status
14+
- `foreground`: Array of tracked items (PRs, calendar, Slack, etc.)
15+
16+
### Correct Pattern: Merge Foreground Items
17+
18+
When refreshing one service, preserve items from other services:
19+
20+
```bash
21+
# Get existing non-PR items before rebuilding PR list
22+
NON_PR_ITEMS=$(jq -c '[.foreground[] | select(.owner | not)]' "$CONFIG" 2>/dev/null || echo '[]')
23+
24+
# After building PR array, merge with non-PR items
25+
echo "$pr_array" | jq --argjson nonpr "$NON_PR_ITEMS" '. + $nonpr'
26+
```
27+
28+
### Wrong Pattern: Full Replacement
29+
30+
```bash
31+
# BAD: This loses Slack, calendar, and other items!
32+
jq '.foreground = $prs' --argjson prs "$PR_ONLY_ARRAY" "$BRIDGE"
33+
```
34+
35+
## Config File Updates (`~/.claude/status-config.json`)
36+
37+
### Correct Pattern: Targeted Updates with `|=`
38+
39+
```bash
40+
# Update specific item by matching criteria
41+
jq '(.foreground[] | select(.number == 123)) |= . + {lastSeen: {...}}' "$CONFIG"
42+
43+
# Update background without touching foreground
44+
jq '.background |= {service: "spotify", tabId: 123}' "$CONFIG"
45+
```
46+
47+
### Wrong Pattern: Full Object Replacement
48+
49+
```bash
50+
# BAD: Replaces entire config, losing other sections!
51+
echo '{"foreground": [...]}' > "$CONFIG"
52+
```
53+
54+
## Adding New Items
55+
56+
When adding a new tracked item:
57+
58+
1. Read existing foreground array
59+
2. Check if item already exists (match by unique key like `owner/repo/number`)
60+
3. If exists: update in place with `|=`
61+
4. If new: append with `+= [$new_item]`
62+
63+
```bash
64+
# Check if PR exists, then add or update
65+
if jq -e ".foreground[] | select(.owner == \"$owner\" and .repo == \"$repo\" and .number == $number)" "$CONFIG" >/dev/null 2>&1; then
66+
# Update existing
67+
jq "(.foreground[] | select(.number == $number)) |= . + {lastSeen: {...}}" "$CONFIG"
68+
else
69+
# Append new
70+
jq ".foreground += [{owner: \"$owner\", repo: \"$repo\", number: $number}]" "$CONFIG"
71+
fi
72+
```
73+
74+
## Checklist for Skill Authors
75+
76+
Before submitting a skill that modifies hub data:
77+
78+
- [ ] Does it preserve items from other services when updating foreground?
79+
- [ ] Does it use `|=` for targeted updates instead of full replacement?
80+
- [ ] Does it check for existing items before adding new ones?
81+
- [ ] Does it preserve background when only updating foreground (and vice versa)?
82+
- [ ] Does it handle missing files gracefully (create with sensible defaults)?
83+
84+
## Race Condition Prevention
85+
86+
When multiple processes might update the same file:
87+
88+
1. Use atomic writes: write to `.tmp` file first, then `mv` to final location
89+
2. For daemons: use lockfile with ownership verification (version:PID)
90+
3. Check file staleness before modifying
91+
92+
```bash
93+
# Atomic write pattern
94+
jq '...' "$CONFIG" > "${CONFIG}.tmp" && mv "${CONFIG}.tmp" "$CONFIG"
95+
```
96+
97+
## Related Files
98+
99+
- `bin/refresh-prs.sh` - PR-only refresh (preserves non-PR items)
100+
- `bin/update-bridge.sh` - Bridge writer (preserves background/foreground appropriately)
101+
- `bin/refresh-daemon.sh` - Background daemon with lockfile management
102+
- `skills/hub-refresh.md` - Full refresh skill

plugins/claude-status-hub/skills/hub-refresh.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,15 @@ function sanitize(str, maxLen = 30) {
2121
```
2222
Limits: title 30 chars, detail 25 chars, artist 20 chars.
2323

24+
## Data Preservation
25+
26+
**See `docs/data-safety-guidelines.md` for merge patterns.**
27+
28+
Key rules:
29+
- Never overwrite entire foreground array
30+
- When refreshing one service, preserve items from other services
31+
- Use `jq` merge operators (`+`, `|=`) not full replacement
32+
2433
## Step 1: Read Config
2534

2635
```bash

plugins/claude-status-hub/tests/test-daemon-version.sh

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,27 @@ fi
210210
# Cleanup daemon
211211
kill "$DAEMON_PID" 2>/dev/null || true
212212

213+
# --- Test 9: Self-eviction when lockfile ownership changes ---
214+
# This tests the race condition fix: if another daemon takes over the lockfile,
215+
# the current daemon should exit gracefully (see docs/data-safety-guidelines.md)
216+
echo ""
217+
echo "Test: Daemon self-evicts when lockfile ownership changes"
218+
rm -f "$LOCKFILE"
219+
CURRENT_V=$(jq -r '.version' "$PLUGIN_JSON")
220+
221+
# Simulate the lockfile ownership check logic from refresh-daemon.sh
222+
# A daemon with PID 12345 wrote the lockfile
223+
echo "${CURRENT_V}:12345" > "$LOCKFILE"
224+
225+
# Another daemon (PID 99999) checks if it owns the lockfile
226+
CURRENT_LOCK=$(cat "$LOCKFILE" 2>/dev/null)
227+
MY_EXPECTED="${CURRENT_V}:99999"
228+
if [ "$CURRENT_LOCK" != "$MY_EXPECTED" ]; then
229+
pass "Lockfile ownership mismatch detected (self-eviction trigger)"
230+
else
231+
fail "Lockfile ownership check" "mismatch detected" "false match"
232+
fi
233+
213234
echo ""
214235
echo "=== Results ==="
215236
echo "Passed: $TESTS_PASSED"

plugins/claude-status-hub/tests/test-refresh-prs.sh

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,45 @@ else
505505
fail "Bridge structure" "spotify bg + 1 fg" "$bg_site + $fg_count fg"
506506
fi
507507

508+
# Test non-PR foreground items are preserved (see docs/data-safety-guidelines.md)
509+
TESTS_RUN=$((TESTS_RUN + 1))
510+
# Setup config with both PR and non-PR items
511+
cat > "$CONFIG" << EOF
512+
{
513+
"foreground": [
514+
{
515+
"owner": "test",
516+
"repo": "repo",
517+
"number": 1,
518+
"lastSeen": {}
519+
},
520+
{
521+
"service": "slack",
522+
"icon": "S",
523+
"title": "3 unread",
524+
"detail": "#general"
525+
},
526+
{
527+
"service": "calendar",
528+
"icon": "📅",
529+
"title": "Meeting",
530+
"detail": "in 10m"
531+
}
532+
]
533+
}
534+
EOF
535+
create_mock_gh "OPEN" "false" "APPROVED" "MERGEABLE" 0 0 0
536+
run_refresh
537+
fg_count=$(jq -r '.foreground | length' "$BRIDGE")
538+
slack_exists=$(jq -r '.foreground[] | select(.service == "slack") | .title' "$BRIDGE")
539+
calendar_exists=$(jq -r '.foreground[] | select(.service == "calendar") | .title' "$BRIDGE")
540+
pr_exists=$(jq -r '.foreground[] | select(.site == "github-pr") | .title' "$BRIDGE")
541+
if [ "$fg_count" = "3" ] && [ "$slack_exists" = "3 unread" ] && [ "$calendar_exists" = "Meeting" ] && [ -n "$pr_exists" ]; then
542+
pass "Non-PR foreground items preserved (Slack, Calendar)"
543+
else
544+
fail "Non-PR preservation" "3 items (PR + Slack + Calendar)" "$fg_count items, slack='$slack_exists', calendar='$calendar_exists'"
545+
fi
546+
508547
echo ""
509548
echo "Testing auto-merge functionality..."
510549

0 commit comments

Comments
 (0)