Skip to content

test(install-hooks+watcher): 20 structural tests for deprecated-hook auto-uninstall (#1083/#1085) and watcher PID file (#1065) - #1161

Closed
bassilkhilo-ag2 wants to merge 2 commits into
sonichi:mainfrom
bassilkhilo-ag2:test/hooks-watcher-1083-1065
Closed

bassilkhilo-ag2 wants to merge 2 commits into
sonichi:mainfrom
bassilkhilo-ag2:test/hooks-watcher-1083-1065

Conversation

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator

What

Structural tests covering two merged PRs:

Background

PR #1083 / #1085 — auto-uninstall deprecated hooks

The Stop|watch-tasks-stream.pid hook was removed from HOOKS=() in #1083 because Turn-end firing killed the live Monitor watcher every turn. But existing installs still had the deprecated entry in their settings.json. PR #1085 added DEPRECATED_HOOKS + Phase 2 removal so the old entry is automatically removed on re-run — no manual jq needed.

PR #1065 — PID file

Adds <workspace>/state/watch-tasks-stream.pid so the Stop hook can kill the exact fswatch PID on session end. trap ... EXIT removes the file on graceful shutdown; dirty exits (SIGKILL) are covered by the Stop hook + startup reaper.

Tests (20 assertions)

install-claude-hooks.sh (11)

# Assertion
1 DEPRECATED_HOOKS array defined
2 watch-tasks-stream.pid appears in DEPRECATED_HOOKS
3 Entry uses Stop|watch-tasks-stream.pid format
4 Deprecated entry NOT in active HOOKS=()
5 Phase 2 for loop over DEPRECATED_HOOKS exists
6 Removal uses contains() substring match (no exact-string fragility)
7 Atomic jq → mktemp → mv pattern
8 Skip (continue) when no deprecated match (idempotent)
9 Empty hook groups dropped after removal
10 Summary reports removed=N
11 ADDED and SKIPPED still tracked (Phase 1 preserved)

watch-tasks-stream.sh (9)

# Assertion
12 PID_FILE defined → watch-tasks-stream.pid
13 PID file under state/ dir
14 Respects SUTANDO_WORKSPACE env var
15 Falls back to ~/.sutando/workspace/state
16 $$ (current PID) written to PID_FILE
17 trap 'rm -f "$PID_FILE"' EXIT cleanup
18 mkdir -p before writing PID file
19 No repo-root fallback for TASKS_DIR
20 TASKS_DIR defaults to ~/.sutando/workspace/tasks

Run

python3 tests/install-hooks-watcher-pid.test.py

🤖 Generated with Claude Code

@VasiliyRad VasiliyRad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good structural tests. Three issues before approving:

  1. Non-test changes in a test PR. src/health-check.py has a production code change (+14/-2) and build_log.md adds 114 lines of unrelated operational notes (LinkedIn posts, WWDC threads, outreach signals). The PR title says "test" but ships a feature and a build-log dump. Split the health-check change into its own PR or at minimum update the PR title/description to reflect it. build_log.md should not be committed here.

  2. Redundant condition in test #9. "length > 0" in hooks or "length > 0" in hooks — both sides are identical. One branch is likely a typo (maybe "length == 0" or "select(length" was intended as the alternate pattern).

  3. Minor: health-check-bridge-multi-pid.test.py is undocumented in the PR body. The PR describes 20 tests across two scripts but ships a third test file (5 additional tests for the health-check change) without mentioning it in the description.

Tests themselves are reasonable structural greps. No correctness bugs in the assertions I checked.

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

Status update (2026-05-26): The build_log.md and src/health-check.py entries in this diff are a housekeeping artifact — local main has 3 commits ahead of upstream main (two build_log.md chore commits + the health-check multi-PID fix from d9a384c), and this branch was branched from local main before the divergence was noticed.

The health-check change ships cleanly in PR #1157 (already APPROVED by @VasiliyRad). The build_log.md chore commits should never have been pushed and will be dropped.

A rebase onto upstream main (dropping build_log.md, letting health-check land via #1157) is ready — pending owner go-ahead to force-push. The actual test content here is unchanged and passes against current main.

@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

T9 fixed (commit 4b801e0). The condition was indeed a tautology — both sides of or were "length > 0" in hooks, making it always pass regardless of whether select( is actually in the source. Updated to "select(" in hooks and "length > 0" in hooks, which matches the real jq pattern in install-claude-hooks.sh line 136: map(select((.hooks // []) | length > 0)).

Regarding the other two issues (non-test changes + build_log.md): those are scope-creep from local main being 3 commits ahead of upstream main. A clean rebase onto origin/main removes them entirely — all verified conflict-free. Pending owner go-ahead to force-push.

bassilkhilo-ag2 and others added 2 commits May 27, 2026 06:57
…auto-uninstall (sonichi#1085/sonichi#1083) and watcher PID file (sonichi#1065)

sonichi#1085/sonichi#1083: DEPRECATED_HOOKS array, Stop|watch-tasks-stream.pid entry,
not in active HOOKS=(), Phase 2 removal loop, contains() substring match,
atomic jq→tmp→mv, idempotent skip, empty-group cleanup, removed= summary.

sonichi#1065: PID_FILE at state/watch-tasks-stream.pid, SUTANDO_WORKSPACE
resolution, ~/.sutando/workspace/state fallback, $$ PID written,
trap EXIT cleanup, mkdir -p before write, workspace-default TASKS_DIR.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
T9 checked `"length > 0" in hooks or "length > 0"` — both sides
identical (tautology). Now checks `"select(" in hooks and "length > 0"
in hooks`, matching the actual jq pattern in install-claude-hooks.sh.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bassilkhilo-ag2
bassilkhilo-ag2 force-pushed the test/hooks-watcher-1083-1065 branch from 4b801e0 to 7e19948 Compare May 27, 2026 02:58
@bassilkhilo-ag2

Copy link
Copy Markdown
Collaborator Author

Branch rebased onto origin/main — bundled junk (build_log.md, health-check.py, health-check-bridge-multi-pid.test.py) stripped. Diff is now clean: only the intended test file(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants