Skip to content

[workspace-migration] fix: install-claude-hooks + verify-setup + 2 test fixtures resolve workspace correctly - #1331

Closed
liususan091219 wants to merge 1 commit into
mainfrom
fix/p2-test-fixtures-and-install-paths
Closed

liususan091219 wants to merge 1 commit into
mainfrom
fix/p2-test-fixtures-and-install-paths

Conversation

@liususan091219

Copy link
Copy Markdown
Collaborator

Summary

4 sites where install/verify scripts and test fixtures were either repo-anchored (so failed for users whose checkout isn't ~/Desktop/sutando) or read/wrote paths that didn't match the production resolver.

Bugs fixed

1. src/install-claude-hooks.sh:58-60 (audit bug 6)

Hook command strings hardcoded $HOME/Desktop/sutando/src/session-handoff.sh and .../check-pending-tasks.sh. Users with checkouts under ~/Documents/sutando/sutando had the hook silently never fire — the shell would try to bash a path that didn't exist.

Fix: use ${SUTANDO_REPO_DIR:-$HOME/Desktop/sutando} (resolved at hook-fire time). This matches the convention from CLAUDE.md ("$SUTANDO_REPO_DIR … names the public-repo checkout for scripts … that need the source tree") and the auto-detect pattern in src/session-handoff.sh.

Added two DEPRECATED_HOOKS entries so re-running the installer auto-migrates existing installs with hardcoded paths — the substring-match cleanup machinery added in #1083 follow-up already does the heavy lifting.

2. src/verify-setup.sh:123-128 (audit bug 10)

for d in tasks results notes; do mkdir -p "$d" — relative paths, silently created in cwd. Run from ~/Documents/sutando/sutando → dirs land in the repo root. Run from anywhere else → dirs land there. Neither matches $SUTANDO_WORKSPACE/{tasks,results,notes} where production reads.

Fix: explicit WORKSPACE="${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}" at top of script; loop uses "$WORKSPACE/$d". Output now shows the absolute resolved path so the user can verify.

3. scripts/test-event-log.sh:23,26 (audit bug 28)

Test wrote LOG = REPO / "logs" / f"events-{date.today()}.jsonl", but production src/event_log.py:57 writes to LOGS_DIR = WORKSPACE_DIR / "logs". The test would silently false-pass or false-fail depending on whether stale $REPO/logs/events-*.jsonl existed.

Fix: test reads LOG = WORKSPACE / "logs" / ... matching the production writer.

4. scripts/test-migrate-bundle.sh:56 (audit bug 29)

[ -d "$REPO/notes" ] for bundle-required-files check. Post-#769 notes/ moved to $SUTANDO_WORKSPACE/notes, so the test asserted on a path that no longer exists for workspace-only setups.

Fix: check NOTES_DIR="${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}/notes" instead.

Empirical verification — all four PoCs

# 1. verify-setup with SUTANDO_WORKSPACE=/tmp/vsetup-test-NNNN
  ✓ tasks/ (created) → /tmp/vsetup-test-NNNN/tasks
  ✓ results/ (created) → /tmp/vsetup-test-NNNN/results
  ✓ notes/ (created) → /tmp/vsetup-test-NNNN/notes
# 2. install-claude-hooks on empty settings.json
install-claude-hooks: added=3 skipped=0 removed=0
→ hook command "bash \"${SUTANDO_REPO_DIR:-$HOME/Desktop/sutando}/src/session-handoff.sh\" \"$TRANSCRIPT_PATH\"" lands verbatim
# 3. install-claude-hooks on settings.json seeded with the OLD hardcoded paths
install-claude-hooks: added=3 skipped=0 removed=2
→ final settings.json contains only the new ${SUTANDO_REPO_DIR:-...} form; old hardcoded entries are gone
# 4. test-event-log resolved path matches event_log.LOGS_DIR
Production:  /Users/<user>/.sutando/workspace/logs/events-2026-05-28.jsonl
Test reader: /Users/<user>/.sutando/workspace/logs/events-2026-05-28.jsonl

What is NOT in this PR

scripts/test-results-health.sh:25,27 (audit bugs 30, 31) is the obvious sibling fix to ship together, but it depends on scripts/results-health.sh itself being workspace-anchored (PR #1276). Bundling the test fix with #1276 instead — test + script stay together, no temporal coupling between this PR and #1276's merge order.

Branched off current upstream/main (0 commits behind at branch time).

Refs v3 workspace-contract audit, bugs 6, 10, 28, 29 (master log: workspace/notes/cwd-audit-v3-2026-05-27.md).

🤖 Generated with Claude Code

… workspace correctly

[workspace-migration] 4 sites across install + verify scripts and test fixtures that
were either repo-anchored (so failed for users with non-Desktop checkouts) or read/wrote
paths that didn't match the production resolver.

Fixes:

1. src/install-claude-hooks.sh:59-60 — PreCompact + Stop hook command strings
   hardcoded $HOME/Desktop/sutando/... — users with checkouts under
   ~/Documents/sutando/sutando never had the hooks fire. Replaced with
   ${SUTANDO_REPO_DIR:-$HOME/Desktop/sutando} (resolved at hook-fire time,
   matches session-handoff.sh's auto-detect pattern). Added DEPRECATED_HOOKS
   entries so existing installs with hardcoded paths get auto-migrated on
   next re-run of the installer (already a pattern in this file post-#1083).

2. src/verify-setup.sh:123-128 — `for d in tasks results notes; do mkdir -p "$d"`
   created dirs in cwd (silently anchored to wherever the user ran the script).
   Replaced with explicit $WORKSPACE prefix from $SUTANDO_WORKSPACE, default
   ~/.sutando/workspace. Output now shows the absolute resolved path.

3. scripts/test-event-log.sh:23,26 — test read LOG from $REPO/logs but
   production src/event_log.py:57 (LOGS_DIR = WORKSPACE_DIR / "logs") writes
   to $WORKSPACE/logs. The test produced false PASS / false FAIL depending
   on stale state. Now reads from the workspace path the writer actually targets.

4. scripts/test-migrate-bundle.sh:56 — `[ -d "$REPO/notes" ]` for bundle
   assertion. Post-#769 notes/ lives under $SUTANDO_WORKSPACE not $REPO.
   Updated to read NOTES_DIR=${SUTANDO_WORKSPACE:-~/.sutando/workspace}/notes.

Empirical verification (all four PoCs pass):
- verify-setup with SUTANDO_WORKSPACE=/tmp/test → creates /tmp/test/{tasks,results,notes}, not cwd
- install-claude-hooks on empty settings.json: ${SUTANDO_REPO_DIR:-...} string lands verbatim
- install-claude-hooks on seeded settings.json with old hardcoded paths: added=3 removed=2 — auto-migration works
- test-event-log resolved path matches event_log.LOGS_DIR/events-YYYY-MM-DD.jsonl

Not included in this PR:
- scripts/test-results-health.sh:25,27 (audit bugs 30, 31) — depends on
  scripts/results-health.sh fix landing first (PR #1276). Folded back into
  #1276 instead so the test + script stay together.

Refs v3 workspace-contract audit (bugs 6, 10, 28, 29).
@liususan091219
liususan091219 marked this pull request as draft May 29, 2026 12:42
@amkunte

amkunte commented May 29, 2026

Copy link
Copy Markdown
Contributor

LGTM — 4 distinct fixes, but they share one root cause (workspace/repo confusion in setup-time and test-time code paths), so bundling here is justified.

Strongest part: the deprecated-hook cleanup. Adding PreCompact|$HOME/Desktop/sutando/src/session-handoff.sh to DEPRECATED_HOOKS so a re-run uninstalls the old hardcoded entry is the kind of detail that gets skipped 9 times out of 10 — without it, users with old hooks would carry both forever. Nice catch.

${SUTANDO_REPO_DIR:-$HOME/Desktop/sutando} pattern is correct. Resolves at hook-fire time (when the env is present), not at install time (when it might not be). Matches the session-handoff.sh auto-detect convention quoted in CLAUDE.md.

Test fixture fixes are surgical:

One minor observation (non-blocking):
The new "warn for *.local author email" in verify-setup.sh would catch the noreply@anthropic.com case the #1338 docs also call out. Mirror the check from #1338 here? Won't block — feels like #1338's job to ship the docs and #1331's job to ship the resolver fixes. Separate concerns.

Closes 4 audit-flagged sites cleanly. 👍

@sonichi

sonichi commented May 29, 2026

Copy link
Copy Markdown
Owner

LGTM. Reviewed all four sites — each fixes a real reader/writer path mismatch, and the inline comments explain the why well:

  • test-event-log.sh — now reads WORKSPACE/logs/ to match event_log.py's actual writer (LOGS_DIR = WORKSPACE_DIR / "logs"). The old REPO/logs/ read was the classic false-pass/false-fail-on-stale-state bug.
  • test-migrate-bundle.sh — NOTES_DIR from $SUTANDO_WORKSPACE; correct per the post-feat(workspace): notes/ honors SUTANDO_WORKSPACE + auto-migration for in-repo stragglers #769 contract.
  • install-claude-hooks.sh — hook commands now use ${SUTANDO_REPO_DIR:-$HOME/Desktop/sutando}. Strictly an improvement: the embedded literal resolves at hook-fire time, default is unchanged, and the new DEPRECATED_HOOKS substring entries clean up the old hardcoded-path hooks on re-run. Good catch that hardcoding broke ~/Documents/sutando/sutando checkouts.
  • verify-setup.sh — dir checks now resolve under $WORKSPACE, with the → $full echo making the resolved path visible.

No blockers. One optional follow-up: if verify-setup.sh checks any other workspace-state files later in the script (build_log.md / pending-questions.md / core-status.json) with bare cwd-relative paths, fold them into the same $WORKSPACE anchoring for consistency — couldn't confirm from the diff alone since the branch is on a fork. (Reviewing as Sutando/Pro — no merge authority; flagging for Chi.)

@liususan091219

Copy link
Copy Markdown
Collaborator Author

Closing per Susan 2026-05-29 — Qingyun is redesigning the workspace migration approach. The 15-PR set will be re-applied (or rebuilt to match the new design) after that lands. Branch is preserved; can be reopened or cherry-picked when ready.

@liususan091219
liususan091219 deleted the fix/p2-test-fixtures-and-install-paths branch June 22, 2026 13:04
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.

3 participants