Repository navigation
Fix infinite reconnect loop in web client - #7
Merged
Merged
Conversation
Shows troubleshooting steps (startup.sh, Claude Code terminal) instead of looping forever when voice agent is unreachable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
liususan091219
pushed a commit
that referenced
this pull request
Apr 18, 2026
Per Susan's 'general edit to correct outdated descriptions' ask. Applied Maddy's review hits that I could verify locally: - pkill pattern: `-f "src/Sutando"` → `-x Sutando` (hit #8: safer, matches the actual process name regardless of launch path). - Uninstall step 5: softened from `brew uninstall imsg wacli` to a generic 'remove via the package manager you used' (hit #3: wacli isn't necessarily brew). - Capabilities table: - Task delegation: `task-bridge.ts` → `task-bridge.ts + watch-tasks.sh + tasks/` (hit #4: reflects the actual file-based flow). - Notes: 'via CLAUDE.md' → '`notes/` directory (YAML-frontmatter markdown)' (hit #5: notes/ is the real store). - Added rows for cross-node sync, info-radar, menu-bar avatar states (hit #6: recent features missing from the matrix). - Services table: added 7845 (screen-capture), 3100 (conversation-server), 4040 (ngrok admin UI) (hit #7). - Removed stray duplicate `---` separator after the services table (hit #9). Skipped: - Hit #1 (L77 subscription line) — already removed earlier. - Hit #2 (screencapture note) — Maddy confirmed still accurate. - Hit #10 (swiftc missing `-framework AVFoundation`) — main.swift does NOT import AVFoundation (verified: import Cocoa, Carbon, UserNotifications only). Current compile line is correct. - L101 permission rewrite (Maddy's 4-bullet proposal) — substantive rewrite, deferred to Susan's specific approval before applying. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
added a commit
that referenced
this pull request
Apr 19, 2026
* docs(README): update 'Summon' voice trigger example to 'Summon my computer to zoom' The short form 'Summon' is false-positive-prone in voice transcripts. Owner uses the longer 'Summon my computer to zoom' phrase in practice. This docs update matches what actually triggers the Zoom summon reliably. Code-side prompts are NOT changed in this PR (per CLAUDE.md's 'prompts tuned through testing' rule) — the underlying summonTool already matches both phrases via the 'summon' keyword. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): add two troubleshooting entries (Claude context drift + phone-server error) Per Susan's Discord msg: two common failure modes worth surfacing in Troubleshooting: 1. Claude hallucinates more as context fills → tell users to restart the Claude Code session periodically. 2. Phone call answers 'We are sorry, an error has occurred' → the conversation-server on port 3100 isn't running; relaunch via startup.sh or restart.sh. Verified the correct server path: skills/phone-conversation/scripts/ conversation-server.ts (not a standalone start-phone-server.sh, which doesn't exist). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): apply Maddy's verified hits — general cleanup Per Susan's 'general edit to correct outdated descriptions' ask. Applied Maddy's review hits that I could verify locally: - pkill pattern: `-f "src/Sutando"` → `-x Sutando` (hit #8: safer, matches the actual process name regardless of launch path). - Uninstall step 5: softened from `brew uninstall imsg wacli` to a generic 'remove via the package manager you used' (hit #3: wacli isn't necessarily brew). - Capabilities table: - Task delegation: `task-bridge.ts` → `task-bridge.ts + watch-tasks.sh + tasks/` (hit #4: reflects the actual file-based flow). - Notes: 'via CLAUDE.md' → '`notes/` directory (YAML-frontmatter markdown)' (hit #5: notes/ is the real store). - Added rows for cross-node sync, info-radar, menu-bar avatar states (hit #6: recent features missing from the matrix). - Services table: added 7845 (screen-capture), 3100 (conversation-server), 4040 (ngrok admin UI) (hit #7). - Removed stray duplicate `---` separator after the services table (hit #9). Skipped: - Hit #1 (L77 subscription line) — already removed earlier. - Hit #2 (screencapture note) — Maddy confirmed still accurate. - Hit #10 (swiftc missing `-framework AVFoundation`) — main.swift does NOT import AVFoundation (verified: import Cocoa, Carbon, UserNotifications only). Current compile line is correct. - L101 permission rewrite (Maddy's 4-bullet proposal) — substantive rewrite, deferred to Susan's specific approval before applying. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): revise 'How it works' diagram — phone agent + accurate process topology Per Susan's ask ('diagram isn't quite right', 'phone agent rarely covered') and Maddy's accuracy verification: Diagram changes: - Web client → Voice agent is now WebSocket on :9900 (was implicit file bridge) - Phone (Twilio) → Conversation server added as peer entry path - results/ flow annotated as per-channel (speak via voice/phone vs text via Telegram/Discord), not one unified speak path Process topology changes: - "Two processes work together" → "Three processes" (voice-agent, conversation-server, core-agent) — verified against conversation-server.ts by Maddy (writes to tasks/ directly at lines 289, 964, 1315; not hierarchically downstream of voice-agent). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): surface phone-agent in prereqs + services list Per Susan's ask ('phone agent is rarely covered in the README, it should be mentioned when appropriate'): - Prereqs: added optional Twilio + ngrok bullet so readers know the phone path exists and what they need to enable it. - 'This starts all services' sentence: added 'phone conversation server' to the list so readers see it's a default-started daemon, not an opt-in. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): rewrite permission section (4-bullet 'why elevated perms') per Maddy's proposal Replaces the conservative CYA note ('you should review what it does. All actions are logged') with Maddy's framing that tells the reader: 1. It's local (no remote control plane) 2. You control the audience (3-tier access gating + VERIFIED_CALLERS) 3. Actions are auditable (build_log + tasks/ + results/ + logs/) 4. Hooks are your brake pedal (git-rules-guard.sh Discord 👍/👎) Susan's direction was 'lead the work and Maddy please review iteratively'. Applying Maddy's already-drafted proposal; she reviews next. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): add inline-tools path to 'How it works' diagram Per Susan's ask — voice agent and conversation server have two response paths (inline tools for instant in-process calls; tasks/ for anything the core agent needs to execute), while Telegram/Discord bridges only use tasks/. — Lucy (Mac Studio bot) * docs(README): swap 'mute' for real inline tool per Maddy's review mute is a menu-bar ⌃M shortcut, not an inline tool. Replace with get_current_time (src/inline-tools.ts:376) and broaden the prose sentence to list only genuine inline tools. — Lucy (Mac Studio bot) * docs(README): add running-costs section + price-aware prereqs Per Susan's in-#dev review (consolidated 4-edit bundle, 👍'd): - Line 96: drop unqualified "free" on API key — usage-billed per Running costs - Line 97: add ~$0.01–0.03/min Twilio cost reference - New "## Running costs" section: Gemini Live $0.30–$1.30/hr, Twilio $0.0125/min (US local), free for browser/Telegram/Discord - New ffmpeg bullet in prereqs (optional, for video/audio) Sources: Gemini pricing docs, Twilio voice pricing docs. — Lucy (Mac Studio bot) * docs(README): bundle 4 follow-ups from #susan iteration Per Susan's 4× 👍 iterative review on PR #450: - Diagram: add `↻` on Core agent + legend explaining the cron-driven proactive loop (5-min fires for tasks, health, build-log work). - Running costs table: Twilio row now bidirectional — ~$0.013–$0.018/min covers inbound ($0.0085) + outbound ($0.014) + Media Streams ($0.004). - Prereq bullet: rename "Google AI Studio API key" → "Gemini API key" and inline the ~$0.30–$1.30/hour voice cost. - macOS permissions: moved from inline Quick start bullets to the Security section with a per-permission "why it's needed" expansion; Quick start keeps a one-line pointer. — Lucy (Mac Studio bot) * docs(README): hedge Gemini price claims — free tier vs paid unclear Susan and Chi are still sorting out the free-tier vs paid-tier picture; pull back the specific $0.30-$1.30/hr claim since it misrepresents the free-tier case: - Prereq bullet: "may be free on the free tier or usage-billed depending on your Google Cloud account" instead of a rate - Running costs table Gemini row: "Free tier available - paid usage varies"; notes keep the published paid rates as reference but point readers at their billing dashboard for ground truth - Drop the personal $13/602-min datapoint from the typical-call example (may conflate with non-voice Gemini spend) Twilio row unchanged - that math is from published rates. — Lucy (Mac Studio bot) * docs(README): Option B — remove Running costs section + price mentions Per Susan's "B" direction in #dev: the free-tier vs paid-tier picture for Gemini isn't settled, and leaving any price language in the PR risks misleading first-time readers. - Delete the entire "## Running costs" section (table + example + sources line). - Revert the Gemini API key prereq bullet to `(click "Get API key")` — drop the usage-billed caveat entirely. - Revert the Twilio prereq bullet to drop the `~$0.01–0.03/min` price reference. ffmpeg bullet, diagram rewrite, macOS permissions in Security section, `↻` cron legend, phone-agent surfacing — all stay. — Lucy (Mac Studio bot) --------- Co-authored-by: Xueqing Liu <xueqingliu@Xueqings-Mac-Studio.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sonichi
added a commit
that referenced
this pull request
Apr 20, 2026
-23) (#488) 9 open CodeQL path-injection alerts across 3 sites in src/agent-api.py. All three had strong existing sanitization (regex whitelist + is_relative_to confinement), but CodeQL doesn't recognize is_relative_to as a taint sanitizer. Apply the Path.name pattern matching PR #487's dashboard.py fix: 1. _safe_path(base_dir, filename) — the task/result lookup helper (alerts #16-19). Rebuild filename via Path(name + ".txt").name. 2. /media/{rel} handler (alerts #7, #20-21). Multi-segment; split on "/" and rebuild each component via Path(p).name, then joinpath. 3. Inline task-file build for "answer-{qid}-{ts}.txt" (alerts #22-23). Apply Path(...).name to the full basename. Path.name is functionally a no-op after the regex whitelist (no separators possible), but it's a CodeQL-recognized path-injection sanitizer that breaks the taint flow CodeQL tracks through f-string path construction. Same pattern Mini validated on #487. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
pushed a commit
that referenced
this pull request
Apr 29, 2026
… from path hallucination Two regressions from phone summon test #7 (CA5c9c0a711cfcacc2cc5458c1d5eca04d 21:09 ET): 1. **Model hallucinated "I can't add subtitles"** — record_screen_with_narration's tool description didn't mention that subtitle burn happens automatically when the live transcript exists. User said "add subtitle", model refused, even though the auto-stop callback ALWAYS calls burnLiveTranscriptSubtitles when liveTranscriptRecordingStart > 0. Description now states subtitles ARE always burned in — never refuse a "with subtitles" request. 2. **Model hallucinated path `/tmp/screen_recording_with_narration.mov`** when asked to open the recording — invented a generic name instead of using the `subtitled_path` returned by start. Two-pronged nudge: - Recording-tools description: "Do NOT invent file paths — only use the exact paths returned by this tool" + clarifies subtitled_path is the right one for "with subtitles". - open_file's `File not found` error explicitly says "Do not invent paths" and routes to play_video for unknown-path recording-open. `open_file` itself remains generic — only its error string text changed, no recording-specific code. play_video already has the findRecording fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
added a commit
that referenced
this pull request
Apr 30, 2026
…sweep (#546) * fix(open-file): move openFileTool to inline-tools, decouple from recording `openFileTool` lived in `src/recording-tools.ts` since #274 with recording-flavored helpers (`findRecording()` fallback, `sutando-recording` substring check, `/tmp/sutando-playback-path` write, `demoStateRef.value = 'idle'` reset, ffprobe duration probe) coupled into the function body. PR #353 introduced most of those; PR #392 cleaned the worst (made `find_recording=true` opt-in), but kept the helpers as deliberate residuals. This commit removes them entirely and moves the function to a generic home. Changes: 1. **Move `openFileTool` to `src/inline-tools.ts`.** Required `path` only. Stripped: `find_recording`, `findRecording()` fallback, `sutando-recording` substring write, `demoStateRef` reset, `recPath` rename → `filePath`, "play_video" instruction string, ffprobe duration extraction. 2. **Keep `fullscreen=true` flow in `openFileTool`** (per Chi's confirmation in #dev — fullscreen-on-open is general video-fullscreen UX, not recording-specific). The QuickTime force-open + present-mode AppleScript + Zoom-survives keystroke routing all stay. The runtime guard ("don't fullscreen=true when QT already has docs") stays. 3. **Re-export hygiene:** - `inline-tools.ts` line-17 re-export and line-18 import: dropped `openFileTool` (now defined locally in this file). - `browser-tools.ts` recording-tools re-export block: dropped `openFileTool`. - `recording-tools.ts`: removed definition + the misleading "polymorphic playRecordingTool" comment block above it. One-line marker pointing to the new location. 4. **Recording-flavored side effects relocated to recording-tools:** - `/tmp/sutando-playback-path` write moved to `screenRecordTool` stop handler AND its auto-stop `setTimeout` callback. Recording tool now stamps the playback hint itself when the recording finishes — no model courier through `open_file` needed. - ffprobe duration extraction moved to `screenRecordTool` stop result (`duration_seconds` field). - Stop handler instruction updated to mention both `open_file(path)` and `open_file(path, fullscreen=true)` so the model knows how to invoke each path of the demo flow. README demo flow (`screen_record → open_file(path) → play_video`) continues to work end-to-end: `screen_record stop` writes `/tmp/sutando-playback-path` proactively, and `play_video`'s existing read of that path (`startPlayback()` line ~454) is unchanged. Refs: #274 (initial split), #315 (rename), #353 (introduced coupling), #392 (partial cleanup, kept residuals), #408/#516/#520/#524/#526 (fullscreen + QuickTime fixes — fullscreen=true preserved as general UX). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tools): deduplicate open_url/capture_screen/type_text across browser-tools/inline-tools Three tool names had two registered definitions (one in src/browser-tools.ts and one in src/inline-tools.ts), which both inlineTools and ownerOnlyTools arrays then picked up. Gemini Live rejects setup payloads with duplicate function names (close 1011 "Internal error") — see reference_gemini_1011_tool_name_conflict.md. Canonical placement per layer responsibility: - open_url → browser-tools.ts (browser-side action) - capture_screen → inline-tools.ts (inline screen capture) - type_text → inline-tools.ts (inline keystroke / clipboard paste) inline-tools.ts now re-exports openUrlTool from browser-tools so consumers keep importing from inline-tools unchanged. Removed the duplicate definitions on the non-canonical side; left a one-line marker comment so greps for the old name still land somewhere meaningful. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tools): remove dead cancel_task duplicate from task-bridge `cancelTask` was defined in src/task-bridge.ts:171 (Chi, commit bbc2631 2026-03-28) AND `cancelTaskTool` in src/inline-tools.ts:404. The inline-tools version is the one registered with Gemini; the task-bridge export was imported by src/voice-agent.ts:39 but never referenced anywhere — dead import. Confirmed safe to delete via independent grep: - 0 call sites in src/ - 0 test references - only consumer was the dead import Removes the third source of the duplicate-tool-name 1011 setup error documented in reference_gemini_1011_tool_name_conflict.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(summon): use zoommtg:// deeplink + tighten meeting-window readiness check Two bugs caused phone summon on Studio to log "Screen share started" while nothing actually shared: 1. **HTTPS join URL unreliable on Zoom Workplace.** The `zoomRunning=true` branch was using `https://zoom.us/j/{id}?pwd=...`, which routes through the browser and doesn't reliably hand off to the in-app meeting on Zoom Workplace (Studio). Result: Zoom stayed on its home dashboard, no meeting joined. Replaced with the `zoommtg://` deeplink for both running-and-not-running cases — direct to the Zoom app, no browser hop. 2. **Readiness check counted any window.** The old check waited for `(count of windows of process "zoom.us") > 0`, which the always-on "Zoom Workplace" home dashboard already satisfies. So even with no meeting ever opened, the check passed, the share-screen menu fired into an inapplicable context, the AppleScript silently no-op'd, and the catch block still logged success. Now checks window names for `Zoom Meeting` / `zoom share` / `floating video` — matches `alreadyInMeeting`'s detection. Verified 2026-04-29 phone summon on Studio (`/tmp/conversation-server.log` 15:54:27–15:54:36) showed the false-positive log; instrumented a11y dump confirmed only "Zoom Workplace" was open after the supposed join. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(summon): click Join by a11y description; tighten host-joined check Phone summon on Studio kept missing the actual Join button: the cliclick coordinate (`preview.x + 0.80*w, preview.y + 0.93*h`) didn't land on the button on Zoom Workplace's preview layout. The meeting never opened, the host-joined check then matched "Personal Meeting Room" (a preview window!) on `winNames.includes('Meeting')` and reported `Host joined: true` falsely, so the phone dial-in path fired anyway, and the new strict share-readiness gate timed out 30s later. Net: `summon` returned error 30002ms after a chain of internal false positives. Two fixes: 1. **Join click via a11y description, not coordinates.** Iterate `buttons of w whose description is "Join"` across all zoom.us windows, click the first one. The Join button's a11y description is stable across Zoom Workplace layouts (verified by direct dump 2026-04-29). 2. **Host-joined check uses the strict meeting-window set.** Same `Zoom Meeting | zoom share | floating video` set as the share-readiness gate. Plain `Meeting` was matching preview windows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(summon): bump tool timeout to 60s — Studio Zoom takes ~30s to open meeting Phone summon test #4 (2026-04-29 16:06): a11y Join click landed correctly, but the meeting window took 31s to appear after the click on Studio Zoom Workplace. Bodhi's default tool timeout is 30s, so the tool returned error 30001ms BEFORE the share-menu fired. The share itself eventually fired (logged 16:06:57) but by then the assistant had already told the caller "I'm having some trouble summoning." `60_000` covers the worst-case meeting-open + share-menu + mute AppleScripts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(record-with-narration): write playback-path on auto-stop so play_video can find the file After PR #546 made open_file generic (no findRecording fallback), there was a behavioral gap: record_screen_with_narration's auto-stop didn't persist a playback-path hint, while screen_record stop did. Result on 2026-04-29 phone test: user said "open the recording" → model called open_file with no path → got generic "no path provided" error → user gave up. This commit closes the gap WITHOUT re-coupling open_file to recording: - record_screen_with_narration auto-stop now writes /tmp/sutando-playback-path with the best-available file (subtitled > narrated > raw), matching screen_record stop's existing pattern. - Tool description appends "After auto-stop, to play back or open the recording, call play_video — it auto-finds the file. Do NOT call open_file unless you have the absolute path." to nudge the model toward the correct tool. open_file remains general-purpose; recording-tools owns the playback-hint write. play_video already reads /tmp/sutando-playback-path with a findRecording fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(record-with-narration): return predicted recording paths so model can pass to open_file The 2026-04-29 phone-summon test still failed on "open the recorded video" even after `19ecfb8` wrote `/tmp/sutando-playback-path` on auto-stop — because the model called `open_file` (the natural verb match for "open"), not `play_video`, and `open_file` is generic and needs a path arg. Now `record_screen_with_narration` captures `record.py start`'s JSON output to extract the absolute `recording_path` and computes the `narrated_path = path.replace('.mov', '-narrated.mov')`. Both paths are returned in the start result + spelled out in the `message` field ("After auto-stop, the file will be at ... — pass that exact path to open_file when the user asks to open the recording."). The model now has the path inline from the moment recording starts and can pass it to open_file (or any other path-taking tool) without guessing. No recording-specific code in open_file. No reliance on the model picking up the prior tool description hint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(record-with-narration): return subtitled_path so model can open the burned-in version After 35cce47 returned `recording_path` + `narrated_path`, the model still couldn't open the subtitled version because it didn't know that path. The 2026-04-29 21:00 phone test had subtitle burn succeed (file at /tmp/sutando-recording-X-narrated-subtitled.mov) but the model called open_file three times with narrated/raw paths only — Susan kept saying "this is not the one with the subtitle". Now the start result includes `subtitled_path` and the message string spells out the priority order: pass subtitled_path to open_file when the user asks for "the recording" / "with subtitles". Fall back to narrated_path only when subtitled burn failed (e.g., libass missing, which is now known-fixed via the homebrew-ffmpeg/ffmpeg tap). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(recording/open-file): clarify subtitle support + nudge model away from path hallucination Two regressions from phone summon test #7 (CA5c9c0a711cfcacc2cc5458c1d5eca04d 21:09 ET): 1. **Model hallucinated "I can't add subtitles"** — record_screen_with_narration's tool description didn't mention that subtitle burn happens automatically when the live transcript exists. User said "add subtitle", model refused, even though the auto-stop callback ALWAYS calls burnLiveTranscriptSubtitles when liveTranscriptRecordingStart > 0. Description now states subtitles ARE always burned in — never refuse a "with subtitles" request. 2. **Model hallucinated path `/tmp/screen_recording_with_narration.mov`** when asked to open the recording — invented a generic name instead of using the `subtitled_path` returned by start. Two-pronged nudge: - Recording-tools description: "Do NOT invent file paths — only use the exact paths returned by this tool" + clarifies subtitled_path is the right one for "with subtitles". - open_file's `File not found` error explicitly says "Do not invent paths" and routes to play_video for unknown-path recording-open. `open_file` itself remains generic — only its error string text changed, no recording-specific code. play_video already has the findRecording fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(open-file): strip ALL video-related logic — back to truly generic open Per Susan: open_file should not contain any video-specific behavior. The fullscreen=true flow forced QuickTime, ran a present-mode AppleScript with 30-iteration polling, and had a guard against re-opening when QT already had documents. All video-specific. Removed: - `fullscreen` parameter - QT force-open via `open -a "QuickTime Player"` - Path-matched present-mode AppleScript (Cmd+Ctrl+F) - QT-already-has-doc guard - Description text about video presentation, .mp4/.mov, "fullscreen on a video that is already open" Now: `execFileSync('open', [filePath])` — LaunchServices picks the user's default app for that file type. Mirrors what a human running `open foo.x` in a terminal would get. For fullscreen on a just-opened video, the model should call the existing `fullscreen` tool as a separate step. Two-tool sequence is simpler than the polled AppleScript embedded in open_file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address Mini PR #546 review — summon retry/return, generic fullscreen, soften subtitle claim Mini's review on PR #546: **1. summon Join click + return values (Medium)** — the prior split (one-shot Join + 30s readiness) let a late preview-window race leave the meeting un-joined while readiness ran out the clock; the tool then still returned "summoned" / "joined with screen sharing". Now: combined 60s loop where each iteration first checks for a real meeting window (Zoom Meeting / zoom share / floating video), then if not present retries the a11y Join click. Track `zoomReady` and `shareStarted` accurately and surface them in the return payload + status string (`summoned` | `joined_share_failed` | `meeting_window_not_detected`). instruction text reflects what actually happened. **2. summon timeout (Medium)** — 60s was still tight per Mini's worst-case calc (Join 10s + readiness 60s + share 15s + mute/cleanup 10s + dial-in host wait 20s ≈ 115s). Bumped to 120s. **3. record_screen_with_narration subtitle claim (Low)** — softened "IS automatically burned" to "are attempted automatically when both transcript text and a libass-capable ffmpeg are present". Burn can silently skip on missing transcript / unreadable narrated / missing libass; honesty matters more than over-promising. Description still says "never refuse a 'with subtitles' request" so the model doesn't gate refusal on apparent failure. **Bonus (Susan):** open_file gets a *generic* `fullscreen` parameter back — but app-agnostic this time. Just `open <path>` (LaunchServices routes to default app) → 1.5s settle → System Events Cmd+Ctrl+F. Works for video in QuickTime, PDF in Preview, web pages in Chrome — no QT hardcoding, no path-match polling. Replaces the QT-specific present-mode flow with a one-keystroke generic toggle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(summon): address Mini PR #546 round-2 — dialIn order + 1-line instruction fix Medium #1 (code change): dialIn previously ran a separate host-joined wait BEFORE the readiness/Join-retry loop. Preview-still-up race made dialIn fire against an unjoined meeting. Restructured: readiness/Join-retry loop runs FIRST and gates the dialIn block on zoomReady. Removed redundant standalone host-joined wait. Low #2 (text only): instruction in phoneJoined branch falsely claimed "Screen is shared..." when shareStarted=false. One-line fix: `(shareStarted ? 'Screen is shared and ' : '')` prefix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Xueqing Liu <xueqingliu@Xueqings-Mac-Studio.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 of 6 tasks
liususan091219
pushed a commit
that referenced
this pull request
May 18, 2026
Moved from the private sonichi/sutando-skills repo to live alongside OSS source. OSS contributors install Sutando from this repo; the skill needs to be co-located so they can actually invoke it. In one invocation: 1. Opens a `use-case-submission` issue on sonichi/sutando — lowest friction, no fork needed. 2. Opens a PR adding `community-use-cases/<slug>.md` with YAML frontmatter mirroring the `UseCase` TypeScript type from AG2Platform/agent-universe. CLA-gated; submitter gets credit on merge. Both default-on; `--issue-only` / `--pr-only` opt out. Reuses the framing-check from the internal `add-use-case` skill (capability-vs-outcome gate, reject patterns, suggest_reframes). Critically, **leaves the submitter's git identity alone** so the CLA signs under THEIR email. Chi-fleet override only kicks in when `/Users/wangchi/.sutando/workspace/` exists or `SUTANDO_FLEET_OWNER=chi`. Companion infra: - Issue template + community-use-cases/ dir convention land in #781 - Sync skill `promote-community-use-case` stays in private sutando-skills (it's a maintainer tool, not OSS-facing) — see sutando-skills #7 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks done
2 of 6 tasks
sonichi
added a commit
that referenced
this pull request
May 20, 2026
* feat(skills): submit-use-case — OSS-facing use-case submission Moved from the private sonichi/sutando-skills repo to live alongside OSS source. OSS contributors install Sutando from this repo; the skill needs to be co-located so they can actually invoke it. In one invocation: 1. Opens a `use-case-submission` issue on sonichi/sutando — lowest friction, no fork needed. 2. Opens a PR adding `community-use-cases/<slug>.md` with YAML frontmatter mirroring the `UseCase` TypeScript type from AG2Platform/agent-universe. CLA-gated; submitter gets credit on merge. Both default-on; `--issue-only` / `--pr-only` opt out. Reuses the framing-check from the internal `add-use-case` skill (capability-vs-outcome gate, reject patterns, suggest_reframes). Critically, **leaves the submitter's git identity alone** so the CLA signs under THEIR email. Chi-fleet override only kicks in when `/Users/wangchi/.sutando/workspace/` exists or `SUTANDO_FLEET_OWNER=chi`. Companion infra: - Issue template + community-use-cases/ dir convention land in #781 - Sync skill `promote-community-use-case` stays in private sutando-skills (it's a maintainer tool, not OSS-facing) — see sutando-skills #7 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(submit-use-case): bullets-extend-summary + rename --image-url to --media-url + datetime.now(UTC) * fix(submit-use-case): SKILL.md flag rename to match script --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
pushed a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
…onichi#7) Shows troubleshooting steps (startup.sh, Claude Code terminal) instead of looping forever when voice agent is unreachable. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
liususan091219
added a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
* docs(README): update 'Summon' voice trigger example to 'Summon my computer to zoom' The short form 'Summon' is false-positive-prone in voice transcripts. Owner uses the longer 'Summon my computer to zoom' phrase in practice. This docs update matches what actually triggers the Zoom summon reliably. Code-side prompts are NOT changed in this PR (per CLAUDE.md's 'prompts tuned through testing' rule) — the underlying summonTool already matches both phrases via the 'summon' keyword. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): add two troubleshooting entries (Claude context drift + phone-server error) Per Susan's Discord msg: two common failure modes worth surfacing in Troubleshooting: 1. Claude hallucinates more as context fills → tell users to restart the Claude Code session periodically. 2. Phone call answers 'We are sorry, an error has occurred' → the conversation-server on port 3100 isn't running; relaunch via startup.sh or restart.sh. Verified the correct server path: skills/phone-conversation/scripts/ conversation-server.ts (not a standalone start-phone-server.sh, which doesn't exist). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): apply Maddy's verified hits — general cleanup Per Susan's 'general edit to correct outdated descriptions' ask. Applied Maddy's review hits that I could verify locally: - pkill pattern: `-f "src/Sutando"` → `-x Sutando` (hit sonichi#8: safer, matches the actual process name regardless of launch path). - Uninstall step 5: softened from `brew uninstall imsg wacli` to a generic 'remove via the package manager you used' (hit sonichi#3: wacli isn't necessarily brew). - Capabilities table: - Task delegation: `task-bridge.ts` → `task-bridge.ts + watch-tasks.sh + tasks/` (hit sonichi#4: reflects the actual file-based flow). - Notes: 'via CLAUDE.md' → '`notes/` directory (YAML-frontmatter markdown)' (hit sonichi#5: notes/ is the real store). - Added rows for cross-node sync, info-radar, menu-bar avatar states (hit sonichi#6: recent features missing from the matrix). - Services table: added 7845 (screen-capture), 3100 (conversation-server), 4040 (ngrok admin UI) (hit sonichi#7). - Removed stray duplicate `---` separator after the services table (hit sonichi#9). Skipped: - Hit sonichi#1 (L77 subscription line) — already removed earlier. - Hit sonichi#2 (screencapture note) — Maddy confirmed still accurate. - Hit sonichi#10 (swiftc missing `-framework AVFoundation`) — main.swift does NOT import AVFoundation (verified: import Cocoa, Carbon, UserNotifications only). Current compile line is correct. - L101 permission rewrite (Maddy's 4-bullet proposal) — substantive rewrite, deferred to Susan's specific approval before applying. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): revise 'How it works' diagram — phone agent + accurate process topology Per Susan's ask ('diagram isn't quite right', 'phone agent rarely covered') and Maddy's accuracy verification: Diagram changes: - Web client → Voice agent is now WebSocket on :9900 (was implicit file bridge) - Phone (Twilio) → Conversation server added as peer entry path - results/ flow annotated as per-channel (speak via voice/phone vs text via Telegram/Discord), not one unified speak path Process topology changes: - "Two processes work together" → "Three processes" (voice-agent, conversation-server, core-agent) — verified against conversation-server.ts by Maddy (writes to tasks/ directly at lines 289, 964, 1315; not hierarchically downstream of voice-agent). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): surface phone-agent in prereqs + services list Per Susan's ask ('phone agent is rarely covered in the README, it should be mentioned when appropriate'): - Prereqs: added optional Twilio + ngrok bullet so readers know the phone path exists and what they need to enable it. - 'This starts all services' sentence: added 'phone conversation server' to the list so readers see it's a default-started daemon, not an opt-in. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): rewrite permission section (4-bullet 'why elevated perms') per Maddy's proposal Replaces the conservative CYA note ('you should review what it does. All actions are logged') with Maddy's framing that tells the reader: 1. It's local (no remote control plane) 2. You control the audience (3-tier access gating + VERIFIED_CALLERS) 3. Actions are auditable (build_log + tasks/ + results/ + logs/) 4. Hooks are your brake pedal (git-rules-guard.sh Discord 👍/👎) Susan's direction was 'lead the work and Maddy please review iteratively'. Applying Maddy's already-drafted proposal; she reviews next. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(README): add inline-tools path to 'How it works' diagram Per Susan's ask — voice agent and conversation server have two response paths (inline tools for instant in-process calls; tasks/ for anything the core agent needs to execute), while Telegram/Discord bridges only use tasks/. — Lucy (Mac Studio bot) * docs(README): swap 'mute' for real inline tool per Maddy's review mute is a menu-bar ⌃M shortcut, not an inline tool. Replace with get_current_time (src/inline-tools.ts:376) and broaden the prose sentence to list only genuine inline tools. — Lucy (Mac Studio bot) * docs(README): add running-costs section + price-aware prereqs Per Susan's in-#dev review (consolidated 4-edit bundle, 👍'd): - Line 96: drop unqualified "free" on API key — usage-billed per Running costs - Line 97: add ~$0.01–0.03/min Twilio cost reference - New "## Running costs" section: Gemini Live $0.30–$1.30/hr, Twilio $0.0125/min (US local), free for browser/Telegram/Discord - New ffmpeg bullet in prereqs (optional, for video/audio) Sources: Gemini pricing docs, Twilio voice pricing docs. — Lucy (Mac Studio bot) * docs(README): bundle 4 follow-ups from #susan iteration Per Susan's 4× 👍 iterative review on PR sonichi#450: - Diagram: add `↻` on Core agent + legend explaining the cron-driven proactive loop (5-min fires for tasks, health, build-log work). - Running costs table: Twilio row now bidirectional — ~$0.013–$0.018/min covers inbound ($0.0085) + outbound ($0.014) + Media Streams ($0.004). - Prereq bullet: rename "Google AI Studio API key" → "Gemini API key" and inline the ~$0.30–$1.30/hour voice cost. - macOS permissions: moved from inline Quick start bullets to the Security section with a per-permission "why it's needed" expansion; Quick start keeps a one-line pointer. — Lucy (Mac Studio bot) * docs(README): hedge Gemini price claims — free tier vs paid unclear Susan and Chi are still sorting out the free-tier vs paid-tier picture; pull back the specific $0.30-$1.30/hr claim since it misrepresents the free-tier case: - Prereq bullet: "may be free on the free tier or usage-billed depending on your Google Cloud account" instead of a rate - Running costs table Gemini row: "Free tier available - paid usage varies"; notes keep the published paid rates as reference but point readers at their billing dashboard for ground truth - Drop the personal $13/602-min datapoint from the typical-call example (may conflate with non-voice Gemini spend) Twilio row unchanged - that math is from published rates. — Lucy (Mac Studio bot) * docs(README): Option B — remove Running costs section + price mentions Per Susan's "B" direction in #dev: the free-tier vs paid-tier picture for Gemini isn't settled, and leaving any price language in the PR risks misleading first-time readers. - Delete the entire "## Running costs" section (table + example + sources line). - Revert the Gemini API key prereq bullet to `(click "Get API key")` — drop the usage-billed caveat entirely. - Revert the Twilio prereq bullet to drop the `~$0.01–0.03/min` price reference. ffmpeg bullet, diagram rewrite, macOS permissions in Security section, `↻` cron legend, phone-agent surfacing — all stay. — Lucy (Mac Studio bot) --------- Co-authored-by: Xueqing Liu <xueqingliu@Xueqings-Mac-Studio.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
pushed a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
…chi#7, sonichi#16-23) (sonichi#488) 9 open CodeQL path-injection alerts across 3 sites in src/agent-api.py. All three had strong existing sanitization (regex whitelist + is_relative_to confinement), but CodeQL doesn't recognize is_relative_to as a taint sanitizer. Apply the Path.name pattern matching PR sonichi#487's dashboard.py fix: 1. _safe_path(base_dir, filename) — the task/result lookup helper (alerts sonichi#16-19). Rebuild filename via Path(name + ".txt").name. 2. /media/{rel} handler (alerts sonichi#7, sonichi#20-21). Multi-segment; split on "/" and rebuild each component via Path(p).name, then joinpath. 3. Inline task-file build for "answer-{qid}-{ts}.txt" (alerts sonichi#22-23). Apply Path(...).name to the full basename. Path.name is functionally a no-op after the regex whitelist (no separators possible), but it's a CodeQL-recognized path-injection sanitizer that breaks the taint flow CodeQL tracks through f-string path construction. Same pattern Mini validated on sonichi#487. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
added a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
…sweep (sonichi#546) * fix(open-file): move openFileTool to inline-tools, decouple from recording `openFileTool` lived in `src/recording-tools.ts` since sonichi#274 with recording-flavored helpers (`findRecording()` fallback, `sutando-recording` substring check, `/tmp/sutando-playback-path` write, `demoStateRef.value = 'idle'` reset, ffprobe duration probe) coupled into the function body. PR sonichi#353 introduced most of those; PR sonichi#392 cleaned the worst (made `find_recording=true` opt-in), but kept the helpers as deliberate residuals. This commit removes them entirely and moves the function to a generic home. Changes: 1. **Move `openFileTool` to `src/inline-tools.ts`.** Required `path` only. Stripped: `find_recording`, `findRecording()` fallback, `sutando-recording` substring write, `demoStateRef` reset, `recPath` rename → `filePath`, "play_video" instruction string, ffprobe duration extraction. 2. **Keep `fullscreen=true` flow in `openFileTool`** (per Chi's confirmation in #dev — fullscreen-on-open is general video-fullscreen UX, not recording-specific). The QuickTime force-open + present-mode AppleScript + Zoom-survives keystroke routing all stay. The runtime guard ("don't fullscreen=true when QT already has docs") stays. 3. **Re-export hygiene:** - `inline-tools.ts` line-17 re-export and line-18 import: dropped `openFileTool` (now defined locally in this file). - `browser-tools.ts` recording-tools re-export block: dropped `openFileTool`. - `recording-tools.ts`: removed definition + the misleading "polymorphic playRecordingTool" comment block above it. One-line marker pointing to the new location. 4. **Recording-flavored side effects relocated to recording-tools:** - `/tmp/sutando-playback-path` write moved to `screenRecordTool` stop handler AND its auto-stop `setTimeout` callback. Recording tool now stamps the playback hint itself when the recording finishes — no model courier through `open_file` needed. - ffprobe duration extraction moved to `screenRecordTool` stop result (`duration_seconds` field). - Stop handler instruction updated to mention both `open_file(path)` and `open_file(path, fullscreen=true)` so the model knows how to invoke each path of the demo flow. README demo flow (`screen_record → open_file(path) → play_video`) continues to work end-to-end: `screen_record stop` writes `/tmp/sutando-playback-path` proactively, and `play_video`'s existing read of that path (`startPlayback()` line ~454) is unchanged. Refs: sonichi#274 (initial split), sonichi#315 (rename), sonichi#353 (introduced coupling), sonichi#392 (partial cleanup, kept residuals), sonichi#408/sonichi#516/sonichi#520/sonichi#524/sonichi#526 (fullscreen + QuickTime fixes — fullscreen=true preserved as general UX). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tools): deduplicate open_url/capture_screen/type_text across browser-tools/inline-tools Three tool names had two registered definitions (one in src/browser-tools.ts and one in src/inline-tools.ts), which both inlineTools and ownerOnlyTools arrays then picked up. Gemini Live rejects setup payloads with duplicate function names (close 1011 "Internal error") — see reference_gemini_1011_tool_name_conflict.md. Canonical placement per layer responsibility: - open_url → browser-tools.ts (browser-side action) - capture_screen → inline-tools.ts (inline screen capture) - type_text → inline-tools.ts (inline keystroke / clipboard paste) inline-tools.ts now re-exports openUrlTool from browser-tools so consumers keep importing from inline-tools unchanged. Removed the duplicate definitions on the non-canonical side; left a one-line marker comment so greps for the old name still land somewhere meaningful. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(tools): remove dead cancel_task duplicate from task-bridge `cancelTask` was defined in src/task-bridge.ts:171 (Chi, commit bbc2631 2026-03-28) AND `cancelTaskTool` in src/inline-tools.ts:404. The inline-tools version is the one registered with Gemini; the task-bridge export was imported by src/voice-agent.ts:39 but never referenced anywhere — dead import. Confirmed safe to delete via independent grep: - 0 call sites in src/ - 0 test references - only consumer was the dead import Removes the third source of the duplicate-tool-name 1011 setup error documented in reference_gemini_1011_tool_name_conflict.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(summon): use zoommtg:// deeplink + tighten meeting-window readiness check Two bugs caused phone summon on Studio to log "Screen share started" while nothing actually shared: 1. **HTTPS join URL unreliable on Zoom Workplace.** The `zoomRunning=true` branch was using `https://zoom.us/j/{id}?pwd=...`, which routes through the browser and doesn't reliably hand off to the in-app meeting on Zoom Workplace (Studio). Result: Zoom stayed on its home dashboard, no meeting joined. Replaced with the `zoommtg://` deeplink for both running-and-not-running cases — direct to the Zoom app, no browser hop. 2. **Readiness check counted any window.** The old check waited for `(count of windows of process "zoom.us") > 0`, which the always-on "Zoom Workplace" home dashboard already satisfies. So even with no meeting ever opened, the check passed, the share-screen menu fired into an inapplicable context, the AppleScript silently no-op'd, and the catch block still logged success. Now checks window names for `Zoom Meeting` / `zoom share` / `floating video` — matches `alreadyInMeeting`'s detection. Verified 2026-04-29 phone summon on Studio (`/tmp/conversation-server.log` 15:54:27–15:54:36) showed the false-positive log; instrumented a11y dump confirmed only "Zoom Workplace" was open after the supposed join. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(summon): click Join by a11y description; tighten host-joined check Phone summon on Studio kept missing the actual Join button: the cliclick coordinate (`preview.x + 0.80*w, preview.y + 0.93*h`) didn't land on the button on Zoom Workplace's preview layout. The meeting never opened, the host-joined check then matched "Personal Meeting Room" (a preview window!) on `winNames.includes('Meeting')` and reported `Host joined: true` falsely, so the phone dial-in path fired anyway, and the new strict share-readiness gate timed out 30s later. Net: `summon` returned error 30002ms after a chain of internal false positives. Two fixes: 1. **Join click via a11y description, not coordinates.** Iterate `buttons of w whose description is "Join"` across all zoom.us windows, click the first one. The Join button's a11y description is stable across Zoom Workplace layouts (verified by direct dump 2026-04-29). 2. **Host-joined check uses the strict meeting-window set.** Same `Zoom Meeting | zoom share | floating video` set as the share-readiness gate. Plain `Meeting` was matching preview windows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(summon): bump tool timeout to 60s — Studio Zoom takes ~30s to open meeting Phone summon test sonichi#4 (2026-04-29 16:06): a11y Join click landed correctly, but the meeting window took 31s to appear after the click on Studio Zoom Workplace. Bodhi's default tool timeout is 30s, so the tool returned error 30001ms BEFORE the share-menu fired. The share itself eventually fired (logged 16:06:57) but by then the assistant had already told the caller "I'm having some trouble summoning." `60_000` covers the worst-case meeting-open + share-menu + mute AppleScripts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(record-with-narration): write playback-path on auto-stop so play_video can find the file After PR sonichi#546 made open_file generic (no findRecording fallback), there was a behavioral gap: record_screen_with_narration's auto-stop didn't persist a playback-path hint, while screen_record stop did. Result on 2026-04-29 phone test: user said "open the recording" → model called open_file with no path → got generic "no path provided" error → user gave up. This commit closes the gap WITHOUT re-coupling open_file to recording: - record_screen_with_narration auto-stop now writes /tmp/sutando-playback-path with the best-available file (subtitled > narrated > raw), matching screen_record stop's existing pattern. - Tool description appends "After auto-stop, to play back or open the recording, call play_video — it auto-finds the file. Do NOT call open_file unless you have the absolute path." to nudge the model toward the correct tool. open_file remains general-purpose; recording-tools owns the playback-hint write. play_video already reads /tmp/sutando-playback-path with a findRecording fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(record-with-narration): return predicted recording paths so model can pass to open_file The 2026-04-29 phone-summon test still failed on "open the recorded video" even after `19ecfb8` wrote `/tmp/sutando-playback-path` on auto-stop — because the model called `open_file` (the natural verb match for "open"), not `play_video`, and `open_file` is generic and needs a path arg. Now `record_screen_with_narration` captures `record.py start`'s JSON output to extract the absolute `recording_path` and computes the `narrated_path = path.replace('.mov', '-narrated.mov')`. Both paths are returned in the start result + spelled out in the `message` field ("After auto-stop, the file will be at ... — pass that exact path to open_file when the user asks to open the recording."). The model now has the path inline from the moment recording starts and can pass it to open_file (or any other path-taking tool) without guessing. No recording-specific code in open_file. No reliance on the model picking up the prior tool description hint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(record-with-narration): return subtitled_path so model can open the burned-in version After 35cce47 returned `recording_path` + `narrated_path`, the model still couldn't open the subtitled version because it didn't know that path. The 2026-04-29 21:00 phone test had subtitle burn succeed (file at /tmp/sutando-recording-X-narrated-subtitled.mov) but the model called open_file three times with narrated/raw paths only — Susan kept saying "this is not the one with the subtitle". Now the start result includes `subtitled_path` and the message string spells out the priority order: pass subtitled_path to open_file when the user asks for "the recording" / "with subtitles". Fall back to narrated_path only when subtitled burn failed (e.g., libass missing, which is now known-fixed via the homebrew-ffmpeg/ffmpeg tap). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(recording/open-file): clarify subtitle support + nudge model away from path hallucination Two regressions from phone summon test sonichi#7 (CA5c9c0a711cfcacc2cc5458c1d5eca04d 21:09 ET): 1. **Model hallucinated "I can't add subtitles"** — record_screen_with_narration's tool description didn't mention that subtitle burn happens automatically when the live transcript exists. User said "add subtitle", model refused, even though the auto-stop callback ALWAYS calls burnLiveTranscriptSubtitles when liveTranscriptRecordingStart > 0. Description now states subtitles ARE always burned in — never refuse a "with subtitles" request. 2. **Model hallucinated path `/tmp/screen_recording_with_narration.mov`** when asked to open the recording — invented a generic name instead of using the `subtitled_path` returned by start. Two-pronged nudge: - Recording-tools description: "Do NOT invent file paths — only use the exact paths returned by this tool" + clarifies subtitled_path is the right one for "with subtitles". - open_file's `File not found` error explicitly says "Do not invent paths" and routes to play_video for unknown-path recording-open. `open_file` itself remains generic — only its error string text changed, no recording-specific code. play_video already has the findRecording fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(open-file): strip ALL video-related logic — back to truly generic open Per Susan: open_file should not contain any video-specific behavior. The fullscreen=true flow forced QuickTime, ran a present-mode AppleScript with 30-iteration polling, and had a guard against re-opening when QT already had documents. All video-specific. Removed: - `fullscreen` parameter - QT force-open via `open -a "QuickTime Player"` - Path-matched present-mode AppleScript (Cmd+Ctrl+F) - QT-already-has-doc guard - Description text about video presentation, .mp4/.mov, "fullscreen on a video that is already open" Now: `execFileSync('open', [filePath])` — LaunchServices picks the user's default app for that file type. Mirrors what a human running `open foo.x` in a terminal would get. For fullscreen on a just-opened video, the model should call the existing `fullscreen` tool as a separate step. Two-tool sequence is simpler than the polled AppleScript embedded in open_file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address Mini PR sonichi#546 review — summon retry/return, generic fullscreen, soften subtitle claim Mini's review on PR sonichi#546: **1. summon Join click + return values (Medium)** — the prior split (one-shot Join + 30s readiness) let a late preview-window race leave the meeting un-joined while readiness ran out the clock; the tool then still returned "summoned" / "joined with screen sharing". Now: combined 60s loop where each iteration first checks for a real meeting window (Zoom Meeting / zoom share / floating video), then if not present retries the a11y Join click. Track `zoomReady` and `shareStarted` accurately and surface them in the return payload + status string (`summoned` | `joined_share_failed` | `meeting_window_not_detected`). instruction text reflects what actually happened. **2. summon timeout (Medium)** — 60s was still tight per Mini's worst-case calc (Join 10s + readiness 60s + share 15s + mute/cleanup 10s + dial-in host wait 20s ≈ 115s). Bumped to 120s. **3. record_screen_with_narration subtitle claim (Low)** — softened "IS automatically burned" to "are attempted automatically when both transcript text and a libass-capable ffmpeg are present". Burn can silently skip on missing transcript / unreadable narrated / missing libass; honesty matters more than over-promising. Description still says "never refuse a 'with subtitles' request" so the model doesn't gate refusal on apparent failure. **Bonus (Susan):** open_file gets a *generic* `fullscreen` parameter back — but app-agnostic this time. Just `open <path>` (LaunchServices routes to default app) → 1.5s settle → System Events Cmd+Ctrl+F. Works for video in QuickTime, PDF in Preview, web pages in Chrome — no QT hardcoding, no path-match polling. Replaces the QT-specific present-mode flow with a one-keystroke generic toggle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(summon): address Mini PR sonichi#546 round-2 — dialIn order + 1-line instruction fix Medium sonichi#1 (code change): dialIn previously ran a separate host-joined wait BEFORE the readiness/Join-retry loop. Preview-still-up race made dialIn fire against an unjoined meeting. Restructured: readiness/Join-retry loop runs FIRST and gates the dialIn block on zoomReady. Removed redundant standalone host-joined wait. Low sonichi#2 (text only): instruction in phoneJoined branch falsely claimed "Screen is shared..." when shareStarted=false. One-line fix: `(shareStarted ? 'Screen is shared and ' : '')` prefix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Xueqing Liu <xueqingliu@Xueqings-Mac-Studio.local> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219
pushed a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
…hi#782) * feat(skills): submit-use-case — OSS-facing use-case submission Moved from the private sonichi/sutando-skills repo to live alongside OSS source. OSS contributors install Sutando from this repo; the skill needs to be co-located so they can actually invoke it. In one invocation: 1. Opens a `use-case-submission` issue on sonichi/sutando — lowest friction, no fork needed. 2. Opens a PR adding `community-use-cases/<slug>.md` with YAML frontmatter mirroring the `UseCase` TypeScript type from AG2Platform/agent-universe. CLA-gated; submitter gets credit on merge. Both default-on; `--issue-only` / `--pr-only` opt out. Reuses the framing-check from the internal `add-use-case` skill (capability-vs-outcome gate, reject patterns, suggest_reframes). Critically, **leaves the submitter's git identity alone** so the CLA signs under THEIR email. Chi-fleet override only kicks in when `/Users/wangchi/.sutando/workspace/` exists or `SUTANDO_FLEET_OWNER=chi`. Companion infra: - Issue template + community-use-cases/ dir convention land in sonichi#781 - Sync skill `promote-community-use-case` stays in private sutando-skills (it's a maintainer tool, not OSS-facing) — see sutando-skills sonichi#7 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(submit-use-case): bullets-extend-summary + rename --image-url to --media-url + datetime.now(UTC) * fix(submit-use-case): SKILL.md flag rename to match script --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9 of 10 tasks
qingyun-wu
added a commit
that referenced
this pull request
Jun 2, 2026
…t-inbox Mini #7's defensive surface adds (agents/, docs/, email-drafts/, agent-inbox/ → structural) were applied uniformly to all sources A/B/C. But Source A IS the sutando repo root, where docs/ contains the project's own documentation (code-side, tracked in git, NOT workspace data). The walk picked them up → migrate copied to <workspace>/docs/ → --delete-source sha-matched and rm'd them from <repo>/docs/. Caught 2026-06-02 07:24 PT after owner ran repo-root cleanup; restored via git checkout HEAD -- docs/ (18 tracked files). Fix: explicit SOURCE_A_EXCLUDE constant lists the dirs in Mini's #7 defensive set that EXIST at the sutando repo root as code rather than workspace data. scan_source + commit_source both skip those when tag=A. Sources B and C (real workspace dirs) still get the full WORKSPACE_SURFACE_DIRS treatment. The principle (per new memory feedback_per_source_surface_lists): - Source A walks ONLY M0-canonical workspace surface dirs - Sources B+C walk those PLUS Mini #7 defensive non-canonical adds - A's surface is a strict subset of B+C's surface — never the other way - When adding to WORKSPACE_SURFACE_DIRS, ask "does Source A have a same-named dir that contains REPO CODE?" — if yes, add to SOURCE_A_EXCLUDE too Verified: real scan against Source A now reports 0 structural files (was previously walking docs/ + agents/). E2E + per-row tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
qingyun-wu
added a commit
that referenced
this pull request
Jun 2, 2026
* fix(workspace): M1 phase 1 — resolver fixes + helper bootstrap subcommands Part 1 of M1 (#1395 follow-up). Closes the resolver-leftover class identified in the Phase A audit (workspace/notes/m1-resolver-audit-2026-06-01.md). The runtime-data-recovery CLI (`bash scripts/sutando-migrate.sh`) is the remaining M1 deliverable and ships in a follow-up. ## Why M0 (#1395 + #1397) changed the default workspace from `~/.sutando/workspace/` to `<repo>/workspace/` via a config-driven loader and migrated the helper-aware src/ files. The CI lint (`scripts/lint-workspace-resolution.sh`) runs in `--diff` mode and explicitly defers existing-offender cleanup to a follow-up. This PR is that follow-up for the resolver class. Without it, ~5 scripts silently land at the pre-M0 location on hosts where `$SUTANDO_WORKSPACE` is unset (intended post-M0 state), producing split-brain state (`quota-state.json` at one path, `core-status.json` at another, etc.). ## What changes **Resolver fixes (route through M0 helper):** - `src/session-handoff.sh` — quota-state.json + proactive-loop sentinel - `src/init.sh` — workspace seeder; preserves .env-drift detection on top of helper - `src/install-credential-proxy-launchd.sh` - `src/install-health-check-launchd.sh` - `src/install-sutando-app-launchd.sh` - `skills/proactive-loop/SKILL.md` — adds path-resolution preamble; updates build_log.md + core-status.json sites to `$WORKSPACE/...` **Doc / template corrections (stale legacy paths):** - `src/launchd/com.sutando.menubar.plist` — comment refresh - `src/voice-agent.config.json.example` — `_comment` refresh **New helper subcommands (M1.3 — workspace init / sub-folder bootstrap):** - `scripts/sutando-config.sh subdirs` — print canonical workspace subdir list - `scripts/sutando-config.sh bootstrap` — mkdir -p the canonical subdirs in the resolved workspace (idempotent; covers the case where a workspace path change without service restart skips init.sh's tier1 bootstrap) ## What does NOT change - `src/startup.sh`, `src/watch-tasks-stream.sh`, `scripts/sync-memory.sh` — initial audit flagged these but closer read shows they already use the M0 helper as primary path; the legacy idiom is a defensive fallback only. - The migration CLI (`scripts/sutando-migrate.sh`) — separate PR. ## What is still left for M1 - `bash scripts/sutando-migrate.sh` — recovery skill for existing-user state in `~/.sutando/workspace/`, `<repo>/` root (pre-#911 legacy), and any env-pinned location. Per #1395 PR body. Tracked in audit Phase C. - Live-writer restart matrix — pre-M0 services holding stale env-pinned workspace resolution. slack-bridge was the lone caught case on this host (restarted out-of-band); other long-running services need the same. ## Test plan - [ ] `bash scripts/sutando-config.sh workspace` resolves to `<repo>/workspace/` - [ ] `bash scripts/sutando-config.sh bootstrap` creates the canonical subdirs - [ ] `bash scripts/sutando-config.sh subdirs` prints the 10-entry list - [ ] `bash src/init.sh --auto` resolves via helper (no .env drift on this host) - [ ] `bash src/install-credential-proxy-launchd.sh install` substitutes the M0 path - [ ] proactive-loop fires write core-status.json + build_log.md to `$WORKSPACE/` - [ ] CI workspace-leak-check + lint-workspace-resolution pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(workspace): drop legacy literal fallbacks; helper is the contract (CI lint fix) CI lint-workspace-resolution flagged the defensive fallbacks added in the prior commit (`${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}`) as "new direct workspace-path resolution." The lint was right — the defensive form perpetuates exactly the pattern M0 set out to retire. Replace the 5 sites with the canonical pattern: - helper present → use it - helper absent but $SUTANDO_WORKSPACE set → honor it (legacy escape hatch) - neither → fail loud with explanatory error Also: move init.sh's mode-arg validation BEFORE workspace resolution so the "Usage:" error path isn't masked by the workspace-resolve error (caught by tests/init-script.test.ts argument-parsing case). Affected: - src/init.sh - src/install-credential-proxy-launchd.sh - src/install-health-check-launchd.sh - src/install-sutando-app-launchd.sh - src/session-handoff.sh All 19 tests in tests/init-script.test.ts pass locally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(workspace): address Mini's 3 blockers + Lucy's nits on #1399 ## Must-fix (Mini's blockers from PR #1399 review) - `skills/proactive-loop/SKILL.md:21` activation step still used the legacy `${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}` sentinel path. But `src/session-handoff.sh` clears the helper-resolved sentinel. Mismatch → catchup could be skipped incorrectly. Now activation resolves via the helper and uses `"$WORKSPACE/state/proactive-loop-started.sentinel"`. - `src/session-handoff.sh:82` read in-flight tasks from `$REPO/tasks/*.txt`. Tasks live in the workspace, not the repo. Handoff would say "None pending" while real tasks sat in the M0 workspace. Now reads `"$WORKSPACE_DIR/tasks/*.txt"`. - `src/session-handoff.sh` was not actually fail-loud — no `set -e`, so an empty `WORKSPACE_DIR` would silently make downstream paths absolute-roots (`/state/...`). Now wraps the helper call with an explicit empty-check and exits 1 on either empty resolve or non-zero helper exit. ## Should-fix (convergent — Mini note 2 + Lucy nit #2) - `src/init.sh` tier1 hardcoded a separate subdir list parallel to `scripts/sutando-config.sh subdirs`. Init.sh now consumes the helper's list (with a defensive fallback to the historical hardcoded set for non-checkout installs). Single source of truth across init.sh + the `bootstrap` subcommand + docs. ## Optional follow-ups (Lucy nit #1 + Lucy "smoke test" + Mini wording) - New `src/workspace_resolve.sh` factoring out the `if helper; elif env; else fail` 3-way block previously duplicated across the 5 scripts (init.sh, install-credential-proxy-launchd.sh, install-sutando-app- launchd.sh, install-health-check-launchd.sh, session-handoff.sh). Each caller now sources it and calls `resolve_workspace_or_die`. Defensive fallback retained per-caller for the rare missing-helper case. - New `tests/sutando-config-shell.test.ts` smoke test (3 cases) — asserts `sutando-config.sh workspace` matches the py loader, `subdirs` returns the canonical list, and `bootstrap` creates exactly that set idempotently. Locks the wrapper against drift. - `skills/proactive-loop/SKILL.md` preamble updated: emphasises resolve-once-per-pass (avoid re-spawning python subprocess per write), uses "read or write" wording, and quotes `"$WORKSPACE/..."` examples so spaces in workspace paths don't tokenize. ## Reflection Why missed in round 1 (per owner's ask): - B1: I scanned SKILL.md for `build_log`/`core-status` writes (the obvious sites) but didn't grep the same file for `SUTANDO_WORKSPACE` — would have shown 2 hits, only 1 fixed. - B2: Focused on the lint-flagged resolution lines in session-handoff.sh (77 + 104) and considered the file done. Didn't audit the rest of the file for other `$REPO/{tasks,results,...}` workspace-state references. - B3: Assumed `set -e` would catch downstream issues. Other touched scripts have it; session-handoff.sh is the outlier — didn't verify per-file. The fail-loud pattern requires either `set -e` or an explicit empty-check; without one of those, it's not actually fail-loud. Process fix recorded as `feedback_per_file_pattern_audit.md`: when fixing a class of bugs in a file, (1) grep that file for ALL members of the class, not just the trigger; (2) whole-file skim post-edit; (3) cross-check invariants the fix-pattern depends on (`set -e`, etc.); (4) self-cold-review the diff before push. All 22 tests pass locally: 19 init-script + 3 new sutando-config-shell. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(workspace): cross-checkout helper resolution + tighten bootstrap idempotency test ## Cross-checkout helper resolution (E2E-found regression) Caught by an E2E pass against #1399 round 2: `bash src/session-handoff.sh` crashed on the owner's host because `$SUTANDO_REPO_DIR` pointed to a sibling checkout (a private submodule pin) that didn't yet contain the new `src/workspace_resolve.sh` introduced by 9eae682. session-handoff.sh's REPO resolution prefers env over script-parent, so the source path `$REPO/src/workspace_resolve.sh` resolved to a path that doesn't exist on the older pin → exit 1 → SessionStop hook silently no-ops. Two layers of fix: 1. **Each caller** (`src/init.sh`, 3× `install-*-launchd.sh`, `session-handoff.sh`) now tries `$REPO/src/workspace_resolve.sh` first and falls back to `$(dirname "$0")/workspace_resolve.sh` (script sibling). The script-sibling path is always reachable regardless of $REPO resolution. 2. **`src/workspace_resolve.sh` itself** now self-locates `scripts/sutando-config.sh` via `${BASH_SOURCE[0]}` instead of $REPO. Without this, even after the source path resolves correctly (via #1), the function would still call `$REPO/scripts/sutando-config.sh` and hit the same cross-checkout drift one layer deeper. E2E re-verified: `SUTANDO_REPO_DIR=<sibling-checkout> bash src/session-handoff.sh` now runs to completion (exit 0) where it previously exited 1. ## Bootstrap idempotency test tightened (Mini's overclaim catch) `tests/sutando-config-shell.test.ts`'s second-run check previously verified only "dirs exist after second run" — not "no extra dirs / no mutation." Now snapshots the recursive top-level dir set + mtimes before/after the second run and asserts `deepEqual` on the sorted key set. Any mutation (extra dir, touched mtime) makes the second run diverge from the snapshot and fails the test. 22 tests still pass locally (19 init-script + 3 sutando-config-shell with the tightened assertion). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(workspace): tighten bootstrap idempotency assertion to deep-equal full snapshot The snapshot helper collects mtimeMs per path, and the surrounding comment claims "strict idempotency / nothing was created or touched", but the assertion only compared Object.keys(after).sort() vs Object.keys(before).sort() — throwing away every mtime value. A bootstrap re-run that silently chmod'd, touched a marker, or otherwise mutated state inside existing dirs would still pass. Switch to assert.deepEqual(after, before) so the assertion enforces the claim. Also rewrite the explanatory comment to match what the assertion now actually checks. Convergent feedback from Sutando-Mini (#design 02:58Z) and Sutando-Pro (03:00Z) on the round-3 commit (0b017c2) of PR #1399. Verified locally: npx tsx --test tests/sutando-config-shell.test.ts — all 3 cases pass (bootstrap is a true FS-level no-op on APFS; mkdir -p on existing dirs doesn't touch mtime). * feat(workspace): M1 Part 2 — workspace migration engine + tests scripts/sutando-migrate.sh is the recovery CLI promised in #1395 and called out in PR #1399's "What is still left for M1 (Part 2)". Helps existing users fold their pre-M0 workspace state into the M0 canonical <repo>/workspace/. Modes: - scan (default) / --dry-run — read-only audit; per-source + cross-source collision report with size-mismatch / mtime-only / identical-content classification. JSON output via --json for tooling. - commit — rsync -a (mtime preserved) per-source in order C→A→B; per-class dispatch (structural / collision-keep-both / newest-mtime / append / rehome-state / rehome-narrative-log / rehome-dated-snapshot / inflight-guard). Backup tarball before any write. - verify — post-migration source-empty + dest-complete check. - rollback --backup-id <id> — restore dest from backup tarball + clean added-since-backup files + drop sentinels. - explain <path> — print which CLASS_RULES row matches + computed dest. Safety: - No-delete-source by default (matches workspace_m1_no_auto_commit). - --delete-source requires --backup-id (refuses without; Mini's polish). - Per-source idempotency sentinels (state/.migrated-from-<tag>-<ts>). - In-flight task/result protection: <60s old skip+warn; >60s old routes to tasks/archive/<src-tag>/ (NOT live tasks/, prevents watcher re-fire on migrated stale tasks). - Refuses if realpath(dest) == realpath(any source). - Per-file SHA-256 verify post-rsync. Test coverage: - tests/sutando-migrate.test.sh — synthetic fixture E2E covering scan → commit → idempotency-re-run → rollback. 10/10 assertions. - tests/sutando-migrate-rules.test.sh — per-row CLASS_RULES fixture asserting class + computed dest for ~41 representative paths. Catches glob-order regressions (e.g. state/auth/* BEFORE state/*.json). Per-class destinations follow Mini's #design 2026-06-02 per-file recommendation: state JSONs → state/, cloud-auth + device → state/auth/ (durable per-host install state — see CLAUDE.md addition in companion commit), root conversation.log → logs/workspace-narrative.log (renamed to dodge per-pid logs/conversation.log collision), dated snapshots → notes/archive/. Honors workspace_m1_no_auto_commit: never runs git commit against any source/dest repo; mutates workspace DATA only. The β rehome of source's .git → dest/.git (vault sync continuity for sutando-plus users) is intentionally a SEPARATE script (sutando-plus/scripts/sutando-migrate-sync.sh) — out of scope for this OSS PR. OSS users don't have a vault remote; sutando-plus users invoke the sync rehome explicitly after this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(workspace): /sutando-migrate skill — agent UX wrapper skills/sutando-migrate/SKILL.md is the agent-invoked walkthrough that wraps scripts/sutando-migrate.sh into a guided 5-phase flow (scan + summary → commit → verify → sync re-route → report). The skill adds value over the raw bash entry path: - Owner-aware surfacing: collision triage + phase boundaries delivered via Discord, macOS notification, voice (if connected) — not raw stdout - Auto-detects sutando-plus + sync configuration; skips the vault re-route phase for OSS users - Wraps the failure-mode fallbacks (verify mismatch → rollback path; partial commit → backup-id pointer) into operator-friendly prose Invocation: - /sutando-migrate — interactive, greenlights at each phase - /sutando-migrate --auto — non-interactive The bash entry path (scripts/sutando-migrate.sh) stays available for operators who prefer shell. The legacy-state-detected nag in init.sh + health-check.py already discoverably points users at the bash form; this skill is the second (agent-aware) path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * ci(workspace): allow sutando-migrate.sh to reference legacy paths The lint at scripts/lint-workspace-resolution.sh forbids direct workspace-path resolution (~/.sutando/workspace literals + direct SUTANDO_WORKSPACE env reads) outside the canonical loader. New scripts/sutando-migrate.sh trips this — but legitimately: its WHOLE PURPOSE is to source FROM the legacy paths (A=repo root, B=pre-M0 ~/.sutando/workspace/, C=env-override) and rsync them into the M0 canonical dest. There's no way to express Source B's location without naming the literal. Same pattern as src/startup.sh / scripts/sync-memory.sh / etc. already in the ALLOWED list per the script's comment block: bash scripts that retain the legacy fallback idiom as a defensive branch. This unblocks PR #1403 CI without weakening the lint for other code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sutando-migrate): surface PERSONAL_CLAUDE.md + per-host status JSONs Per Lucy's #design 2026-06-02 PR review — biggest blind-spot was PERSONAL_CLAUDE.md silently skipping (it's contract-defined in CLAUDE.md but not in WORKSPACE_SURFACE_FILES → falls through to skip-unknown → the user's personal override rules get lost on migration). Adds to WORKSPACE_SURFACE_FILES + CLASS_RULES: - PERSONAL_CLAUDE.md → newest-mtime (singleton-canonical; one user, one file; newest edit wins) - quota-state.json, dynamic-content.json, stand-identity.json → rehome-state (per-host status JSONs that Lucy found unruled on her workspace; re-home to state/ for consistency with the existing rehome-state class) Owner's already-completed migration (2026-06-01 23:17 PT, backup-id 20260602T061757Z) didn't lose any data — none of the newly-surfaced files were present on the owner's sources. This fix prevents data loss for other users (Lucy + future operators) running the migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sutando-migrate): address Mini's 7 blockers + Lucy's per-host carve-out + owner-directed quarantine catchall PR #1403 review batch — substantive corrections per Mini's 5:27 UTC #design Not-LGTM + Lucy's empirical follow-up + owner's "fix all" directive. Mini's blockers: 1. tests/sutando-migrate.test.sh idempotency re-run now uses RUN_MIGRATE helper (was calling bash directly, dropping SUTANDO_MIGRATE_SRC_A/B/C hooks + --respect-env; could mutate real workspace during test). 2. verify_main now sha-compares source → dest content per-file (was only checking path existence; sha_match() defined but unused). Wires sha_match for every indexed source file, fails on mismatch. 3. 3-way collision sidecar uniqueness: when a newer source replaces dest, prior dest goes to <path>.legacy-prior-from-<src_tag>-<ts>; when src loses to dest, src goes to <path>.legacy-<src_tag>-<ts>. 3-source increasing-mtime test added to E2E (B oldest → C → A newest, all 3 versions preserved uniquely). 4. --delete-source actual deletion implemented: per-file rm AFTER sha verify against dest (canonical OR sidecar OR quarantine OR timestamped-sidecar). Conservative: keeps source if no sha-matching dest landing found ("kept unsafe" counter). E2E asserts both the require-backup-id refusal AND the actual deletion. 5. archived-stale outcome now counted in n_copied (was reporting as "other"; commit_one emits archived-stale but commit_source counted only copied-stale). 6. skills/sutando-migrate/SKILL.md phase 5 corrected: health-check legacy warnings WILL still fire after default commit (sources preserved); cleared only after --delete-source phase 2. 7. CLAUDE.md adds "Migration transition window (30-day reader-fallback)" section documenting the no-delete-default + reader-side fallback contract that pairs with the migration script. Lucy's carve-out: - state/{core-status,quota-state,dynamic-content,voice-state,contextual-chips}.json → structural (was state/*.json|newest-mtime which would drop a host's data on multi-host scan; same per-host preservation pattern as state/auth/*). Owner-directed quarantine catchall (Lucy #design 2026-06-02 + "shall we expand?" directive): - WALK_FULL_TREE for sources B + C: tops-level entries NOT in the canonical surface AND not in QUARANTINE_EXCLUDES (.git, node_modules, .cache, dist, etc.) are routed through the catchall. - New CLASS_RULES catchall: *|quarantine-unknown → routes file to <dest>/legacy/<src-tag>/quarantine/<rel>. Net: user content (experiments/, obsidian-vault/, personal-src/, repro-*.ts) is preserved namespaced rather than silently skipped. - Source A (repo root) stays surface-restricted; walking its full tree would migrate sutando code. - New commit_one outcome `quarantined` + per-source `n_quarantined` counter. - E2E asserts 4 fixture-quarantined paths (experiments/note.md, obsidian-vault/daily.md, repro-bug.ts, personal-src/lib.py). Per-row tests updated: state/* per-host JSONs assert structural; novel root file asserts catchall quarantine-unknown class. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sutando-migrate): address Mini's 4 follow-up blockers on b62b9a0 PR #1403 re-review batch — Mini's diff-only post-b62b9a0 review found 4 new blockers; this fixes all of them + the test-flakiness Mini's E2E mention prompted. Mini's 4 new blockers: 1. verify_main + --delete-source lookups for legacy sidecars were only globbing `legacy-<src_tag>-*` and `legacy-prior-from-<src_tag>-*`. But Mini #3's timestamped-sidecar fix names the sidecar after the OVERWRITING source (e.g. C's content lives at `<path>.legacy-prior-from-A-<ts>` because A overwrote C). Source C's lookup would miss its own content there. Now both lookups walk ALL `<path>.legacy-*` siblings + sha-compare to find content regardless of tag attribution. 2. --delete-source replayed the full commit walk for sources that already had a sentinel (my sentinel guard `[ DELETE_SOURCE = 0 ]` was too permissive). Phase 2 would duplicate append-class content + create extra collision sidecars before the rm pass. Now: when sentinel exists AND DELETE_SOURCE=1, skip the commit walk entirely and only do the per-file sha-verified delete pass. 3. Quarantine class was treated as pass-no-check in verify + skipped in --delete-source. Contradicts the documented "deletion verifies canonical OR sidecar OR quarantine" claim. Removed quarantine-unknown from both early-skip lists so the standard candidate walk (dst_canonical / dst_sidecar / dst_quarantine + timestamped sidecars) considers it via sha_match. 4. QUARANTINE_EXCLUDES only filtered top-level entries; a custom dir like experiments/node_modules/ would still get walked + quarantined recursively (undercutting "don't hoover runtime/build artifacts"). Now: after computing rel, walk its segments and skip if ANY matches QUARANTINE_EXCLUDES. Test-suite stability (caught via 10-run reproduction): - Backup tarball was self-including when two backup_dest calls landed in the same second: BACKUP_ID was `date +%s` resolution → first commit + idempotency re-run could collide. Second backup's tar then included the first backup as a state/ file, and rollback's tar -xzf refused to overwrite the existing archive. Now BACKUP_ID appends -p$$r$RANDOM so every backup_dest call has a globally unique ID + path. - Rollback wraps cleanup walks in `set +e` so per-file failures (rm of non-existent paths, grep with empty listing, BSD-tar idiosyncrasies) don't shortcut the entire rollback. - Empty-backup case (dest was bootstrap-only at backup time) now skips tar -xzf (BSD tar errors on 0-byte input) and only runs the cleanup walk — exactly what we want for rolling back to a known-empty state. - Backup writes to mktemp outside dest/state/ then mv's atomically in. Tar creation no longer races with its own destination. - The no-sources-detected early exit now bypasses rollback + verify modes (they don't need sources, just dest). - Rollback's surface walk uses process substitution (no subshell rm). - Source detection inside rollback_main: removed; rollback only needs dest. E2E adjustments: - Rollback test now runs BEFORE --delete-source (mutations from delete could interfere with rollback's removed-since-backup walk). - 3-way collision E2E: A oldest → C → B newest produces unique timestamped+tagged sidecars (covered by the per-run RANDOM suffix). - --delete-source --backup-id test uses COMMIT2_BACKUP_ID (post-rollback fresh commit), not INITIAL. - All asserts pass 10/10 across stability runs (was 3/10 pre-fix). Owner's prior migration (backup-id 20260602T061757Z) unaffected — uses the older naming pattern. The fix is forward-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sutando-migrate): Source A excludes docs/agents/email-drafts/agent-inbox Mini #7's defensive surface adds (agents/, docs/, email-drafts/, agent-inbox/ → structural) were applied uniformly to all sources A/B/C. But Source A IS the sutando repo root, where docs/ contains the project's own documentation (code-side, tracked in git, NOT workspace data). The walk picked them up → migrate copied to <workspace>/docs/ → --delete-source sha-matched and rm'd them from <repo>/docs/. Caught 2026-06-02 07:24 PT after owner ran repo-root cleanup; restored via git checkout HEAD -- docs/ (18 tracked files). Fix: explicit SOURCE_A_EXCLUDE constant lists the dirs in Mini's #7 defensive set that EXIST at the sutando repo root as code rather than workspace data. scan_source + commit_source both skip those when tag=A. Sources B and C (real workspace dirs) still get the full WORKSPACE_SURFACE_DIRS treatment. The principle (per new memory feedback_per_source_surface_lists): - Source A walks ONLY M0-canonical workspace surface dirs - Sources B+C walk those PLUS Mini #7 defensive non-canonical adds - A's surface is a strict subset of B+C's surface — never the other way - When adding to WORKSPACE_SURFACE_DIRS, ask "does Source A have a same-named dir that contains REPO CODE?" — if yes, add to SOURCE_A_EXCLUDE too Verified: real scan against Source A now reports 0 structural files (was previously walking docs/ + agents/). E2E + per-row tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sutando-migrate): exclude code-side dirs by sutando-repo CONTENT, not tag Owner clarified 2026-06-02 07:29: "We should only EXCLUDE them when they are in the sutando repo root." Previous commit 5c64d42 used the per-source tag (A/B/C) which equates Source A === repo root. But a custom workspace path that happens to live inside another sutando checkout (or anyone running migrate from a non-standard A) would slip past the tag check. Now: detect "is this source path a sutando repo checkout?" by content — presence of `src/sutando_config.py` OR `scripts/sutando-config.sh` at source root. If yes, skip dirs in SOURCE_A_EXCLUDE (docs/, agents/, email-drafts/, agent-inbox/) regardless of which tag (A/B/C) the source got. Content-based detection generalizes cleanly: if a future M-N moves the sutando-config helper, the detection check moves with it. Tests still pass (10/10 lifecycle + 41/41 per-row). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sonichi
added a commit
that referenced
this pull request
Jun 22, 2026
* feat(workspace): M0 — in-repo workspace default + config-driven resolution
Milestone 0 of the workspace-revamp series. Replaces the per-user env-var
contract (`$SUTANDO_WORKSPACE` → `~/.sutando/workspace/`) with a config-file
contract that defaults to `<repo>/workspace/`. Subsequent milestones land
on the same staging branch (M1 = runtime data recovery, M2 = sync engine,
M3 = documentation polish + resolver migration).
## Contract
Resolution order (highest wins):
1. `$SUTANDO_WORKSPACE` env var — legacy escape hatch, one-time
deprecation warning at startup.
2. `sutando.config.local.json` → `workspace.path` — per-clone override,
gitignored.
3. `sutando.config.json` → `workspace.path` — tracked defaults.
4. Baked-in fallback `${REPO_DIR}/workspace`.
Deep-merge semantics: `.local.json` is merged over the tracked defaults;
dicts deep-merge, arrays REPLACE wholesale. `${REPO_DIR}` in any string
expands to the directory containing the config file. Keys starting with
`_` are stripped before validation so the `.example` file can carry
inline documentation.
## Loaders + cutover
Four-language loader matching byte-for-byte resolution:
• `src/sutando_config.py` — Python loader (stdlib-only)
• `src/sutando_config.ts` — TypeScript loader
• `src/Sutando/SutandoConfig.swift` — Swift loader (Foundation-only)
• `scripts/sutando-config.sh` — bash wrapper around the Python loader
Cutover (one-line delegations preserve every existing caller):
• `src/workspace_default.{py,ts}` now delegate to the loader
• `src/Sutando/main.swift` AppDelegate.workspace uses SutandoConfig
• 7 bash scripts route through `scripts/sutando-config.sh` with the
legacy `${SUTANDO_WORKSPACE:-...}` pattern retained as fallback for
non-checkout installs
• `src/startup.sh` exercises the loader as a startup banner (surfaces
drift warnings + parse errors at boot)
• Build commands updated to compile `SutandoConfig.swift` alongside
`main.swift` (startup.sh + migrate.sh)
## Protection
• `.gitignore` — `workspace/*` with `!workspace/.gitkeep` exception
• `.githooks/pre-commit` — refuses commits with workspace/ content,
auto-installed at startup via `scripts/install-git-hooks.sh`
• `.github/workflows/workspace-leak-check.yml` — CI mirror of the
pre-commit hook
• `.github/workflows/lint-workspace-resolution.yml` — CI lint refuses
NEW code that bypasses the loader; existing legacy offenders are
unaffected (uses --diff mode)
## Docs
• `docs/workspace-config.md` — new operational reference (resolution
order, override patterns, protection layers, migration story)
• `docs/sutando-config.schema.json` — JSON Schema for editor
autocomplete + validation
• `CLAUDE.md`, `CONTRIBUTING.md`, `KNOWN_ISSUES.md`, `docs/memory-sync.md`,
`docs/workspace-design.md`, `docs/workspace-contract.md` —
updated to reflect the in-repo default and config-first framing
• `.env.example` — `SUTANDO_WORKSPACE=` line marked as legacy escape
hatch only
## Compatibility
Existing installs with `$SUTANDO_WORKSPACE` set continue to work; the
loader honors the value and emits a one-time deprecation warning naming
the migration path. M1 will ship a recovery skill for users who want to
move data from the legacy default location into the in-repo workspace.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(workspace): unit tests for sutando_config loader (follow-up to #1395) (#1397)
* test(workspace): unit tests for sutando_config loader (follow-up to #1395)
Follow-up to #1395 (M0). Addresses Mini's cold-review items #1, #2, #3.
## #1 — Loader unit tests (blocking)
New: `tests/sutando-config.test.py` (21 tests) + `tests/sutando-config.test.ts`
(18 tests). Covers the eight invariants Mini called out, in both languages,
so cross-language drift is caught at PR time:
1. $SUTANDO_WORKSPACE env precedence over .local.json
2. .local.json deep-merge — dicts merge, arrays REPLACE wholesale
3. ${REPO_DIR} expansion in string values, NOT in keys
4. _-prefixed comment keys stripped at every depth
5. Malformed JSON → clear error naming the file (+ line/col in Python)
6. Empty / whitespace-only / missing .local.json all treated as `{}`
7. Per-process cache invalidates across repo_root changes; cache hit
when repo_root unchanged
8. resolve_vault() / resolveVault() returns safe defaults when vault
subtree absent; configured overrides propagate
Bonus coverage: detect_env_workspace_in_dotenv() (quoted values, absent
values), the internal helpers (_strip_comments, _deep_merge, _expand_vars).
All 39 tests pass; tsc clean.
## #2 — CI wiring
The three workflows under .github/workflows/ that gate on `pull_request`
+ `push` previously only triggered on `branches: [main]`. PRs targeting
`staging-workspace-revamp` (the integration branch for the M0–M3 milestone
series) thus only fired license/cla — Mini's #2 observation.
Updated trigger lists to include `staging-workspace-revamp`:
- `.github/workflows/ci.yml` (tsc + tests + UTC-timestamp check)
- `.github/workflows/workspace-leak-check.yml` (refuse workspace/ commits)
- `.github/workflows/lint-workspace-resolution.yml` (refuse new direct
workspace-path resolution)
This PR is the first follow-up that exercises the wider CI surface. Future
milestone PRs on the same staging branch will get the same coverage.
## #3 — _find_repo_root debug diagnostic
When the upward walk exits without finding sutando.config.json, the loader
now emits a one-line stderr line — gated by `SUTANDO_DEBUG=1` so normal
runs stay quiet. Helps users diagnose "why is Sutando using the baked-in
default" without strace.
Mirrored in both src/sutando_config.py and src/sutando_config.ts.
## Not addressed
- Mini's #8 (JSON Schema completeness + optional schema validation in
the loader) — separate follow-up PR; needs a design call on whether
to make schema rejection strict-by-default. Will file as #1397 or
similar after this lands.
- Mini's #4-7 — agreed nits / already-correct / already-decided per
the review.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(workspace): green CI + add Mini #8 (unknown-key warning) to #1397
CI on #1397 hit two failures once the workflows started running on PRs to
staging-workspace-revamp (which was Mini's #2 fix that this PR enabled):
1. tsc + tests (clean install) — Failing after 1m27s
2. lint-workspace-resolution — Failing after 6s
Plus owner asked to include Mini's #8 (schema completeness) in this PR.
## Fix 1: tsc + tests
`tests/workspace-default-relative-env.test.py` loads `src/workspace_default.py`
via `importlib.util.spec_from_file_location` without adding `src/` to
sys.path. Post-M0 cutover, workspace_default's inline `import sutando_config`
couldn't find the sibling module. Made workspace_default self-sufficient:
adds its own dir to sys.path defensively before the delegate import.
Same test also had `test_absolute_env_path_unchanged` checking string
equality against the input path. The new loader was calling `.resolve()`
on absolute paths too, which on macOS rewrites `/tmp/...` → `/private/tmp/...`
(symlink follow). Preserved pre-cutover semantic: only `.resolve()`-anchor
RELATIVE env values; absolute env values pass through `.expanduser()` and
return as-is.
## Fix 2: lint-workspace-resolution
The lint correctly flagged the legacy fallback idiom
`WS="${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}"` that the M0 cutover
kept in 7 bash scripts as a defensive branch for non-checkout installs.
Owner approved the fallback during M0; the lint just didn't see it (CI
gated on main only at the time).
Added the 7 scripts + the wrapper itself to the lint's ALLOWED list with
a clear comment explaining the policy. Tests + the lint script + the
hook installer remain in ALLOWED too (set up sys.path, exercise the
loader, or document the patterns).
## Mini #8: warn on unknown top-level keys
The schema (docs/sutando-config.schema.json) declares
`additionalProperties: false` for IDE strictness, but the loader stayed
lenient. New: `_warn_unknown_top_level_keys` / `warnUnknownTopLevelKeys`
emit a one-time stderr warning when the resolved config has keys outside
{workspace, vault}. Catches typos like `workspce` while leaving
experimental keys harmlessly ignored.
Tests added in both languages (`test_unknown_top_level_keys_warn_on_load`
+ `test_known_keys_only_does_not_warn`). Bringing the totals to 23 Python
+ 20 TypeScript = 43 unit tests on the loader.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(cwd-lint): allowlist src/sutando_config.py for Path.cwd() use
Follow-up to be31575 on #1397. CI's cwd-lint flagged
`src/sutando_config.py:293` for using `Path.cwd()` outside the canonical
resolver. But sutando_config IS the canonical resolver post-M0 (it's
where resolve_workspace lives now; workspace_default delegates to it).
The Path.cwd() call there anchors a relative `$SUTANDO_WORKSPACE` env
value to absolute, which is the same pre-cutover semantic
src/workspace_default.py used (and that file is already allowlisted for
the same reason). Adding sutando_config.py to the allowlist with a
clarifying comment.
TS side unaffected — src/sutando_config.ts doesn't use process.cwd().
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(workspace-default): update fallback assertions for M0 in-repo default
Follow-up to 4bfe80b on #1397. CI surfaced three more test failures —
`tests/workspace-default.test.py`'s `test_resolve_falls_back_*` cases
asserted equality against `default_workspace_dir()` (which still returns
the legacy `~/.sutando/workspace/`), but post-M0 the canonical fallback
for git-clone installs is `<repo>/workspace`.
Updated the three assertions to expect `ROOT / "workspace"` (= the
in-repo default the loader computes for checkouts). Added a docstring
note clarifying that `default_workspace_dir()` is kept as a fallback for
NON-checkout installs (extracted tarball etc.) but is no longer the
canonical first-choice target.
Local: all 4 loader-adjacent test files pass — workspace-default (43),
sutando-config (23), workspace-default-relative-env (3), cwd-lint
(clean). This is the third CI-driven follow-up; the workflows weren't
firing on staging-workspace-revamp PRs pre-this-series.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(workspace): address Mini's two blockers on #1397
Follow-up to e5c59e2 on #1397. Mini's review identified two small
blockers; both fixed here, both gain new test coverage.
## #1 — SUTANDO_DEBUG strict "1" gating
Both loader twins were using Python/JS env truthiness:
Python: if os.environ.get("SUTANDO_DEBUG"): ...
TS: if (process.env.SUTANDO_DEBUG) ...
Non-empty strings — including "0", "false", "" — are truthy in both
languages, so common "disable" patterns would silently emit the diagnostic.
The PR docs were precise about `SUTANDO_DEBUG=1` so the check should be
exact:
Python: if os.environ.get("SUTANDO_DEBUG") == "1": ...
TS: if (process.env.SUTANDO_DEBUG === '1') ...
Added parametric tests covering all five cases (unset / "0" / "false" /
"" / "1") in both languages. The Python side uses `_find_repo_root`
through its private export; the TS side gets a new `@internal` re-export
of `findRepoRoot` so the test can exercise the not-found branch with an
explicit `start` outside the repo (the function is otherwise impossible
to reach without a config-less workspace, since `import.meta.url`-anchored
default discovery always finds the M0 config).
## #2 — TS malformed-JSON test strengthened
The TS test for invariant 5a only asserted filename + "failed to parse".
It would pass even if the loader stripped V8's position info from the
JSON.parse exception. Tightened to also require `position N | line N |
column N` (V8's standard format), matching the Python assertion's
behavior of checking the line/col detail.
## Local audit
Per owner directive, ran the full test suite locally before pushing:
Python loader tests 24 OK (was 23)
TS loader tests 21 pass (was 20)
workspace-default 43 OK
workspace-default-relative-env 3 OK
cwd-lint 0 violations
lint-workspace-resolution no new patterns
tsc --noEmit clean (pre-existing ag2space unrelated)
`npm test`: 340/341 pass. The one failure (`screen-companion-work-retained`)
is `Cannot find package 'yaml'` — pre-existing dep issue unrelated to this
PR; passes under CI's `npm ci` install.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(workspace): M1 phase 1 — resolver fixes + helper bootstrap subcommands (#1399)
* fix(workspace): M1 phase 1 — resolver fixes + helper bootstrap subcommands
Part 1 of M1 (#1395 follow-up). Closes the resolver-leftover class identified
in the Phase A audit (workspace/notes/m1-resolver-audit-2026-06-01.md). The
runtime-data-recovery CLI (`bash scripts/sutando-migrate.sh`) is the remaining
M1 deliverable and ships in a follow-up.
## Why
M0 (#1395 + #1397) changed the default workspace from `~/.sutando/workspace/`
to `<repo>/workspace/` via a config-driven loader and migrated the
helper-aware src/ files. The CI lint (`scripts/lint-workspace-resolution.sh`)
runs in `--diff` mode and explicitly defers existing-offender cleanup to a
follow-up. This PR is that follow-up for the resolver class.
Without it, ~5 scripts silently land at the pre-M0 location on hosts where
`$SUTANDO_WORKSPACE` is unset (intended post-M0 state), producing split-brain
state (`quota-state.json` at one path, `core-status.json` at another, etc.).
## What changes
**Resolver fixes (route through M0 helper):**
- `src/session-handoff.sh` — quota-state.json + proactive-loop sentinel
- `src/init.sh` — workspace seeder; preserves .env-drift detection on top of helper
- `src/install-credential-proxy-launchd.sh`
- `src/install-health-check-launchd.sh`
- `src/install-sutando-app-launchd.sh`
- `skills/proactive-loop/SKILL.md` — adds path-resolution preamble; updates
build_log.md + core-status.json sites to `$WORKSPACE/...`
**Doc / template corrections (stale legacy paths):**
- `src/launchd/com.sutando.menubar.plist` — comment refresh
- `src/voice-agent.config.json.example` — `_comment` refresh
**New helper subcommands (M1.3 — workspace init / sub-folder bootstrap):**
- `scripts/sutando-config.sh subdirs` — print canonical workspace subdir list
- `scripts/sutando-config.sh bootstrap` — mkdir -p the canonical subdirs in
the resolved workspace (idempotent; covers the case where a workspace path
change without service restart skips init.sh's tier1 bootstrap)
## What does NOT change
- `src/startup.sh`, `src/watch-tasks-stream.sh`, `scripts/sync-memory.sh` —
initial audit flagged these but closer read shows they already use the M0
helper as primary path; the legacy idiom is a defensive fallback only.
- The migration CLI (`scripts/sutando-migrate.sh`) — separate PR.
## What is still left for M1
- `bash scripts/sutando-migrate.sh` — recovery skill for existing-user state
in `~/.sutando/workspace/`, `<repo>/` root (pre-#911 legacy), and any
env-pinned location. Per #1395 PR body. Tracked in audit Phase C.
- Live-writer restart matrix — pre-M0 services holding stale env-pinned
workspace resolution. slack-bridge was the lone caught case on this host
(restarted out-of-band); other long-running services need the same.
## Test plan
- [ ] `bash scripts/sutando-config.sh workspace` resolves to `<repo>/workspace/`
- [ ] `bash scripts/sutando-config.sh bootstrap` creates the canonical subdirs
- [ ] `bash scripts/sutando-config.sh subdirs` prints the 10-entry list
- [ ] `bash src/init.sh --auto` resolves via helper (no .env drift on this host)
- [ ] `bash src/install-credential-proxy-launchd.sh install` substitutes the M0 path
- [ ] proactive-loop fires write core-status.json + build_log.md to `$WORKSPACE/`
- [ ] CI workspace-leak-check + lint-workspace-resolution pass
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(workspace): drop legacy literal fallbacks; helper is the contract (CI lint fix)
CI lint-workspace-resolution flagged the defensive fallbacks added in the
prior commit (`${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}`) as "new
direct workspace-path resolution." The lint was right — the defensive form
perpetuates exactly the pattern M0 set out to retire.
Replace the 5 sites with the canonical pattern:
- helper present → use it
- helper absent but $SUTANDO_WORKSPACE set → honor it (legacy escape hatch)
- neither → fail loud with explanatory error
Also: move init.sh's mode-arg validation BEFORE workspace resolution so the
"Usage:" error path isn't masked by the workspace-resolve error (caught by
tests/init-script.test.ts argument-parsing case).
Affected:
- src/init.sh
- src/install-credential-proxy-launchd.sh
- src/install-health-check-launchd.sh
- src/install-sutando-app-launchd.sh
- src/session-handoff.sh
All 19 tests in tests/init-script.test.ts pass locally.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(workspace): address Mini's 3 blockers + Lucy's nits on #1399
## Must-fix (Mini's blockers from PR #1399 review)
- `skills/proactive-loop/SKILL.md:21` activation step still used the legacy
`${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}` sentinel path. But
`src/session-handoff.sh` clears the helper-resolved sentinel. Mismatch →
catchup could be skipped incorrectly. Now activation resolves via the
helper and uses `"$WORKSPACE/state/proactive-loop-started.sentinel"`.
- `src/session-handoff.sh:82` read in-flight tasks from `$REPO/tasks/*.txt`.
Tasks live in the workspace, not the repo. Handoff would say "None pending"
while real tasks sat in the M0 workspace. Now reads `"$WORKSPACE_DIR/tasks/*.txt"`.
- `src/session-handoff.sh` was not actually fail-loud — no `set -e`, so an
empty `WORKSPACE_DIR` would silently make downstream paths absolute-roots
(`/state/...`). Now wraps the helper call with an explicit empty-check
and exits 1 on either empty resolve or non-zero helper exit.
## Should-fix (convergent — Mini note 2 + Lucy nit #2)
- `src/init.sh` tier1 hardcoded a separate subdir list parallel to
`scripts/sutando-config.sh subdirs`. Init.sh now consumes the helper's
list (with a defensive fallback to the historical hardcoded set for
non-checkout installs). Single source of truth across init.sh + the
`bootstrap` subcommand + docs.
## Optional follow-ups (Lucy nit #1 + Lucy "smoke test" + Mini wording)
- New `src/workspace_resolve.sh` factoring out the `if helper; elif env;
else fail` 3-way block previously duplicated across the 5 scripts
(init.sh, install-credential-proxy-launchd.sh, install-sutando-app-
launchd.sh, install-health-check-launchd.sh, session-handoff.sh). Each
caller now sources it and calls `resolve_workspace_or_die`. Defensive
fallback retained per-caller for the rare missing-helper case.
- New `tests/sutando-config-shell.test.ts` smoke test (3 cases) — asserts
`sutando-config.sh workspace` matches the py loader, `subdirs` returns
the canonical list, and `bootstrap` creates exactly that set
idempotently. Locks the wrapper against drift.
- `skills/proactive-loop/SKILL.md` preamble updated: emphasises
resolve-once-per-pass (avoid re-spawning python subprocess per write),
uses "read or write" wording, and quotes `"$WORKSPACE/..."` examples so
spaces in workspace paths don't tokenize.
## Reflection
Why missed in round 1 (per owner's ask):
- B1: I scanned SKILL.md for `build_log`/`core-status` writes (the obvious
sites) but didn't grep the same file for `SUTANDO_WORKSPACE` — would
have shown 2 hits, only 1 fixed.
- B2: Focused on the lint-flagged resolution lines in session-handoff.sh
(77 + 104) and considered the file done. Didn't audit the rest of the
file for other `$REPO/{tasks,results,...}` workspace-state references.
- B3: Assumed `set -e` would catch downstream issues. Other touched
scripts have it; session-handoff.sh is the outlier — didn't verify
per-file. The fail-loud pattern requires either `set -e` or an explicit
empty-check; without one of those, it's not actually fail-loud.
Process fix recorded as `feedback_per_file_pattern_audit.md`:
when fixing a class of bugs in a file, (1) grep that file for ALL members
of the class, not just the trigger; (2) whole-file skim post-edit;
(3) cross-check invariants the fix-pattern depends on (`set -e`, etc.);
(4) self-cold-review the diff before push.
All 22 tests pass locally: 19 init-script + 3 new sutando-config-shell.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(workspace): cross-checkout helper resolution + tighten bootstrap idempotency test
## Cross-checkout helper resolution (E2E-found regression)
Caught by an E2E pass against #1399 round 2: `bash src/session-handoff.sh`
crashed on the owner's host because `$SUTANDO_REPO_DIR` pointed to a
sibling checkout (a private submodule pin) that didn't yet contain the
new `src/workspace_resolve.sh` introduced by 9eae682. session-handoff.sh's
REPO resolution prefers env over script-parent, so the source path
`$REPO/src/workspace_resolve.sh` resolved to a path that doesn't exist
on the older pin → exit 1 → SessionStop hook silently no-ops.
Two layers of fix:
1. **Each caller** (`src/init.sh`, 3× `install-*-launchd.sh`,
`session-handoff.sh`) now tries `$REPO/src/workspace_resolve.sh` first
and falls back to `$(dirname "$0")/workspace_resolve.sh` (script
sibling). The script-sibling path is always reachable regardless of
$REPO resolution.
2. **`src/workspace_resolve.sh` itself** now self-locates
`scripts/sutando-config.sh` via `${BASH_SOURCE[0]}` instead of $REPO.
Without this, even after the source path resolves correctly (via #1),
the function would still call `$REPO/scripts/sutando-config.sh` and
hit the same cross-checkout drift one layer deeper.
E2E re-verified: `SUTANDO_REPO_DIR=<sibling-checkout> bash src/session-handoff.sh`
now runs to completion (exit 0) where it previously exited 1.
## Bootstrap idempotency test tightened (Mini's overclaim catch)
`tests/sutando-config-shell.test.ts`'s second-run check previously
verified only "dirs exist after second run" — not "no extra dirs / no
mutation." Now snapshots the recursive top-level dir set + mtimes
before/after the second run and asserts `deepEqual` on the sorted key set.
Any mutation (extra dir, touched mtime) makes the second run diverge from
the snapshot and fails the test.
22 tests still pass locally (19 init-script + 3 sutando-config-shell with
the tightened assertion).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(workspace): tighten bootstrap idempotency assertion to deep-equal full snapshot
The snapshot helper collects mtimeMs per path, and the surrounding comment
claims "strict idempotency / nothing was created or touched", but the
assertion only compared Object.keys(after).sort() vs Object.keys(before).sort()
— throwing away every mtime value. A bootstrap re-run that silently chmod'd,
touched a marker, or otherwise mutated state inside existing dirs would
still pass.
Switch to assert.deepEqual(after, before) so the assertion enforces the
claim. Also rewrite the explanatory comment to match what the assertion
now actually checks.
Convergent feedback from Sutando-Mini (#design 02:58Z) and Sutando-Pro
(03:00Z) on the round-3 commit (0b017c2) of PR #1399.
Verified locally: npx tsx --test tests/sutando-config-shell.test.ts —
all 3 cases pass (bootstrap is a true FS-level no-op on APFS; mkdir -p
on existing dirs doesn't touch mtime).
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(workspace): M1 Part 2 — workspace migration script + skill (#1403)
* fix(workspace): M1 phase 1 — resolver fixes + helper bootstrap subcommands
Part 1 of M1 (#1395 follow-up). Closes the resolver-leftover class identified
in the Phase A audit (workspace/notes/m1-resolver-audit-2026-06-01.md). The
runtime-data-recovery CLI (`bash scripts/sutando-migrate.sh`) is the remaining
M1 deliverable and ships in a follow-up.
## Why
M0 (#1395 + #1397) changed the default workspace from `~/.sutando/workspace/`
to `<repo>/workspace/` via a config-driven loader and migrated the
helper-aware src/ files. The CI lint (`scripts/lint-workspace-resolution.sh`)
runs in `--diff` mode and explicitly defers existing-offender cleanup to a
follow-up. This PR is that follow-up for the resolver class.
Without it, ~5 scripts silently land at the pre-M0 location on hosts where
`$SUTANDO_WORKSPACE` is unset (intended post-M0 state), producing split-brain
state (`quota-state.json` at one path, `core-status.json` at another, etc.).
## What changes
**Resolver fixes (route through M0 helper):**
- `src/session-handoff.sh` — quota-state.json + proactive-loop sentinel
- `src/init.sh` — workspace seeder; preserves .env-drift detection on top of helper
- `src/install-credential-proxy-launchd.sh`
- `src/install-health-check-launchd.sh`
- `src/install-sutando-app-launchd.sh`
- `skills/proactive-loop/SKILL.md` — adds path-resolution preamble; updates
build_log.md + core-status.json sites to `$WORKSPACE/...`
**Doc / template corrections (stale legacy paths):**
- `src/launchd/com.sutando.menubar.plist` — comment refresh
- `src/voice-agent.config.json.example` — `_comment` refresh
**New helper subcommands (M1.3 — workspace init / sub-folder bootstrap):**
- `scripts/sutando-config.sh subdirs` — print canonical workspace subdir list
- `scripts/sutando-config.sh bootstrap` — mkdir -p the canonical subdirs in
the resolved workspace (idempotent; covers the case where a workspace path
change without service restart skips init.sh's tier1 bootstrap)
## What does NOT change
- `src/startup.sh`, `src/watch-tasks-stream.sh`, `scripts/sync-memory.sh` —
initial audit flagged these but closer read shows they already use the M0
helper as primary path; the legacy idiom is a defensive fallback only.
- The migration CLI (`scripts/sutando-migrate.sh`) — separate PR.
## What is still left for M1
- `bash scripts/sutando-migrate.sh` — recovery skill for existing-user state
in `~/.sutando/workspace/`, `<repo>/` root (pre-#911 legacy), and any
env-pinned location. Per #1395 PR body. Tracked in audit Phase C.
- Live-writer restart matrix — pre-M0 services holding stale env-pinned
workspace resolution. slack-bridge was the lone caught case on this host
(restarted out-of-band); other long-running services need the same.
## Test plan
- [ ] `bash scripts/sutando-config.sh workspace` resolves to `<repo>/workspace/`
- [ ] `bash scripts/sutando-config.sh bootstrap` creates the canonical subdirs
- [ ] `bash scripts/sutando-config.sh subdirs` prints the 10-entry list
- [ ] `bash src/init.sh --auto` resolves via helper (no .env drift on this host)
- [ ] `bash src/install-credential-proxy-launchd.sh install` substitutes the M0 path
- [ ] proactive-loop fires write core-status.json + build_log.md to `$WORKSPACE/`
- [ ] CI workspace-leak-check + lint-workspace-resolution pass
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(workspace): drop legacy literal fallbacks; helper is the contract (CI lint fix)
CI lint-workspace-resolution flagged the defensive fallbacks added in the
prior commit (`${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}`) as "new
direct workspace-path resolution." The lint was right — the defensive form
perpetuates exactly the pattern M0 set out to retire.
Replace the 5 sites with the canonical pattern:
- helper present → use it
- helper absent but $SUTANDO_WORKSPACE set → honor it (legacy escape hatch)
- neither → fail loud with explanatory error
Also: move init.sh's mode-arg validation BEFORE workspace resolution so the
"Usage:" error path isn't masked by the workspace-resolve error (caught by
tests/init-script.test.ts argument-parsing case).
Affected:
- src/init.sh
- src/install-credential-proxy-launchd.sh
- src/install-health-check-launchd.sh
- src/install-sutando-app-launchd.sh
- src/session-handoff.sh
All 19 tests in tests/init-script.test.ts pass locally.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(workspace): address Mini's 3 blockers + Lucy's nits on #1399
## Must-fix (Mini's blockers from PR #1399 review)
- `skills/proactive-loop/SKILL.md:21` activation step still used the legacy
`${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}` sentinel path. But
`src/session-handoff.sh` clears the helper-resolved sentinel. Mismatch →
catchup could be skipped incorrectly. Now activation resolves via the
helper and uses `"$WORKSPACE/state/proactive-loop-started.sentinel"`.
- `src/session-handoff.sh:82` read in-flight tasks from `$REPO/tasks/*.txt`.
Tasks live in the workspace, not the repo. Handoff would say "None pending"
while real tasks sat in the M0 workspace. Now reads `"$WORKSPACE_DIR/tasks/*.txt"`.
- `src/session-handoff.sh` was not actually fail-loud — no `set -e`, so an
empty `WORKSPACE_DIR` would silently make downstream paths absolute-roots
(`/state/...`). Now wraps the helper call with an explicit empty-check
and exits 1 on either empty resolve or non-zero helper exit.
## Should-fix (convergent — Mini note 2 + Lucy nit #2)
- `src/init.sh` tier1 hardcoded a separate subdir list parallel to
`scripts/sutando-config.sh subdirs`. Init.sh now consumes the helper's
list (with a defensive fallback to the historical hardcoded set for
non-checkout installs). Single source of truth across init.sh + the
`bootstrap` subcommand + docs.
## Optional follow-ups (Lucy nit #1 + Lucy "smoke test" + Mini wording)
- New `src/workspace_resolve.sh` factoring out the `if helper; elif env;
else fail` 3-way block previously duplicated across the 5 scripts
(init.sh, install-credential-proxy-launchd.sh, install-sutando-app-
launchd.sh, install-health-check-launchd.sh, session-handoff.sh). Each
caller now sources it and calls `resolve_workspace_or_die`. Defensive
fallback retained per-caller for the rare missing-helper case.
- New `tests/sutando-config-shell.test.ts` smoke test (3 cases) — asserts
`sutando-config.sh workspace` matches the py loader, `subdirs` returns
the canonical list, and `bootstrap` creates exactly that set
idempotently. Locks the wrapper against drift.
- `skills/proactive-loop/SKILL.md` preamble updated: emphasises
resolve-once-per-pass (avoid re-spawning python subprocess per write),
uses "read or write" wording, and quotes `"$WORKSPACE/..."` examples so
spaces in workspace paths don't tokenize.
## Reflection
Why missed in round 1 (per owner's ask):
- B1: I scanned SKILL.md for `build_log`/`core-status` writes (the obvious
sites) but didn't grep the same file for `SUTANDO_WORKSPACE` — would
have shown 2 hits, only 1 fixed.
- B2: Focused on the lint-flagged resolution lines in session-handoff.sh
(77 + 104) and considered the file done. Didn't audit the rest of the
file for other `$REPO/{tasks,results,...}` workspace-state references.
- B3: Assumed `set -e` would catch downstream issues. Other touched
scripts have it; session-handoff.sh is the outlier — didn't verify
per-file. The fail-loud pattern requires either `set -e` or an explicit
empty-check; without one of those, it's not actually fail-loud.
Process fix recorded as `feedback_per_file_pattern_audit.md`:
when fixing a class of bugs in a file, (1) grep that file for ALL members
of the class, not just the trigger; (2) whole-file skim post-edit;
(3) cross-check invariants the fix-pattern depends on (`set -e`, etc.);
(4) self-cold-review the diff before push.
All 22 tests pass locally: 19 init-script + 3 new sutando-config-shell.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(workspace): cross-checkout helper resolution + tighten bootstrap idempotency test
## Cross-checkout helper resolution (E2E-found regression)
Caught by an E2E pass against #1399 round 2: `bash src/session-handoff.sh`
crashed on the owner's host because `$SUTANDO_REPO_DIR` pointed to a
sibling checkout (a private submodule pin) that didn't yet contain the
new `src/workspace_resolve.sh` introduced by 9eae682. session-handoff.sh's
REPO resolution prefers env over script-parent, so the source path
`$REPO/src/workspace_resolve.sh` resolved to a path that doesn't exist
on the older pin → exit 1 → SessionStop hook silently no-ops.
Two layers of fix:
1. **Each caller** (`src/init.sh`, 3× `install-*-launchd.sh`,
`session-handoff.sh`) now tries `$REPO/src/workspace_resolve.sh` first
and falls back to `$(dirname "$0")/workspace_resolve.sh` (script
sibling). The script-sibling path is always reachable regardless of
$REPO resolution.
2. **`src/workspace_resolve.sh` itself** now self-locates
`scripts/sutando-config.sh` via `${BASH_SOURCE[0]}` instead of $REPO.
Without this, even after the source path resolves correctly (via #1),
the function would still call `$REPO/scripts/sutando-config.sh` and
hit the same cross-checkout drift one layer deeper.
E2E re-verified: `SUTANDO_REPO_DIR=<sibling-checkout> bash src/session-handoff.sh`
now runs to completion (exit 0) where it previously exited 1.
## Bootstrap idempotency test tightened (Mini's overclaim catch)
`tests/sutando-config-shell.test.ts`'s second-run check previously
verified only "dirs exist after second run" — not "no extra dirs / no
mutation." Now snapshots the recursive top-level dir set + mtimes
before/after the second run and asserts `deepEqual` on the sorted key set.
Any mutation (extra dir, touched mtime) makes the second run diverge from
the snapshot and fails the test.
22 tests still pass locally (19 init-script + 3 sutando-config-shell with
the tightened assertion).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(workspace): tighten bootstrap idempotency assertion to deep-equal full snapshot
The snapshot helper collects mtimeMs per path, and the surrounding comment
claims "strict idempotency / nothing was created or touched", but the
assertion only compared Object.keys(after).sort() vs Object.keys(before).sort()
— throwing away every mtime value. A bootstrap re-run that silently chmod'd,
touched a marker, or otherwise mutated state inside existing dirs would
still pass.
Switch to assert.deepEqual(after, before) so the assertion enforces the
claim. Also rewrite the explanatory comment to match what the assertion
now actually checks.
Convergent feedback from Sutando-Mini (#design 02:58Z) and Sutando-Pro
(03:00Z) on the round-3 commit (0b017c2) of PR #1399.
Verified locally: npx tsx --test tests/sutando-config-shell.test.ts —
all 3 cases pass (bootstrap is a true FS-level no-op on APFS; mkdir -p
on existing dirs doesn't touch mtime).
* feat(workspace): M1 Part 2 — workspace migration engine + tests
scripts/sutando-migrate.sh is the recovery CLI promised in #1395
and called out in PR #1399's "What is still left for M1 (Part 2)".
Helps existing users fold their pre-M0 workspace state into the
M0 canonical <repo>/workspace/.
Modes:
- scan (default) / --dry-run — read-only audit; per-source + cross-source
collision report with size-mismatch / mtime-only / identical-content
classification. JSON output via --json for tooling.
- commit — rsync -a (mtime preserved) per-source in order C→A→B;
per-class dispatch (structural / collision-keep-both / newest-mtime /
append / rehome-state / rehome-narrative-log / rehome-dated-snapshot /
inflight-guard). Backup tarball before any write.
- verify — post-migration source-empty + dest-complete check.
- rollback --backup-id <id> — restore dest from backup tarball + clean
added-since-backup files + drop sentinels.
- explain <path> — print which CLASS_RULES row matches + computed dest.
Safety:
- No-delete-source by default (matches workspace_m1_no_auto_commit).
- --delete-source requires --backup-id (refuses without; Mini's polish).
- Per-source idempotency sentinels (state/.migrated-from-<tag>-<ts>).
- In-flight task/result protection: <60s old skip+warn; >60s old
routes to tasks/archive/<src-tag>/ (NOT live tasks/, prevents
watcher re-fire on migrated stale tasks).
- Refuses if realpath(dest) == realpath(any source).
- Per-file SHA-256 verify post-rsync.
Test coverage:
- tests/sutando-migrate.test.sh — synthetic fixture E2E covering
scan → commit → idempotency-re-run → rollback. 10/10 assertions.
- tests/sutando-migrate-rules.test.sh — per-row CLASS_RULES fixture
asserting class + computed dest for ~41 representative paths.
Catches glob-order regressions (e.g. state/auth/* BEFORE state/*.json).
Per-class destinations follow Mini's #design 2026-06-02 per-file
recommendation: state JSONs → state/, cloud-auth + device →
state/auth/ (durable per-host install state — see CLAUDE.md addition
in companion commit), root conversation.log → logs/workspace-narrative.log
(renamed to dodge per-pid logs/conversation.log collision), dated
snapshots → notes/archive/.
Honors workspace_m1_no_auto_commit: never runs git commit against
any source/dest repo; mutates workspace DATA only.
The β rehome of source's .git → dest/.git (vault sync continuity for
sutando-plus users) is intentionally a SEPARATE script
(sutando-plus/scripts/sutando-migrate-sync.sh) — out of scope for
this OSS PR. OSS users don't have a vault remote; sutando-plus
users invoke the sync rehome explicitly after this commit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(workspace): /sutando-migrate skill — agent UX wrapper
skills/sutando-migrate/SKILL.md is the agent-invoked walkthrough
that wraps scripts/sutando-migrate.sh into a guided 5-phase flow
(scan + summary → commit → verify → sync re-route → report).
The skill adds value over the raw bash entry path:
- Owner-aware surfacing: collision triage + phase boundaries delivered
via Discord, macOS notification, voice (if connected) — not raw stdout
- Auto-detects sutando-plus + sync configuration; skips the vault
re-route phase for OSS users
- Wraps the failure-mode fallbacks (verify mismatch → rollback path;
partial commit → backup-id pointer) into operator-friendly prose
Invocation:
- /sutando-migrate — interactive, greenlights at each phase
- /sutando-migrate --auto — non-interactive
The bash entry path (scripts/sutando-migrate.sh) stays available
for operators who prefer shell. The legacy-state-detected nag in
init.sh + health-check.py already discoverably points users at the
bash form; this skill is the second (agent-aware) path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ci(workspace): allow sutando-migrate.sh to reference legacy paths
The lint at scripts/lint-workspace-resolution.sh forbids direct
workspace-path resolution (~/.sutando/workspace literals + direct
SUTANDO_WORKSPACE env reads) outside the canonical loader. New
scripts/sutando-migrate.sh trips this — but legitimately: its WHOLE
PURPOSE is to source FROM the legacy paths (A=repo root, B=pre-M0
~/.sutando/workspace/, C=env-override) and rsync them into the M0
canonical dest. There's no way to express Source B's location
without naming the literal.
Same pattern as src/startup.sh / scripts/sync-memory.sh / etc. already
in the ALLOWED list per the script's comment block: bash scripts that
retain the legacy fallback idiom as a defensive branch.
This unblocks PR #1403 CI without weakening the lint for other code.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sutando-migrate): surface PERSONAL_CLAUDE.md + per-host status JSONs
Per Lucy's #design 2026-06-02 PR review — biggest blind-spot was
PERSONAL_CLAUDE.md silently skipping (it's contract-defined in CLAUDE.md
but not in WORKSPACE_SURFACE_FILES → falls through to skip-unknown →
the user's personal override rules get lost on migration).
Adds to WORKSPACE_SURFACE_FILES + CLASS_RULES:
- PERSONAL_CLAUDE.md → newest-mtime (singleton-canonical;
one user, one file; newest edit wins)
- quota-state.json, dynamic-content.json, stand-identity.json
→ rehome-state (per-host status JSONs that Lucy found unruled on
her workspace; re-home to state/ for consistency with the existing
rehome-state class)
Owner's already-completed migration (2026-06-01 23:17 PT, backup-id
20260602T061757Z) didn't lose any data — none of the newly-surfaced
files were present on the owner's sources. This fix prevents data
loss for other users (Lucy + future operators) running the migration.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sutando-migrate): address Mini's 7 blockers + Lucy's per-host carve-out + owner-directed quarantine catchall
PR #1403 review batch — substantive corrections per Mini's 5:27 UTC
#design Not-LGTM + Lucy's empirical follow-up + owner's "fix all" directive.
Mini's blockers:
1. tests/sutando-migrate.test.sh idempotency re-run now uses RUN_MIGRATE
helper (was calling bash directly, dropping SUTANDO_MIGRATE_SRC_A/B/C
hooks + --respect-env; could mutate real workspace during test).
2. verify_main now sha-compares source → dest content per-file (was
only checking path existence; sha_match() defined but unused).
Wires sha_match for every indexed source file, fails on mismatch.
3. 3-way collision sidecar uniqueness: when a newer source replaces
dest, prior dest goes to <path>.legacy-prior-from-<src_tag>-<ts>;
when src loses to dest, src goes to <path>.legacy-<src_tag>-<ts>.
3-source increasing-mtime test added to E2E (B oldest → C → A
newest, all 3 versions preserved uniquely).
4. --delete-source actual deletion implemented: per-file rm AFTER
sha verify against dest (canonical OR sidecar OR quarantine OR
timestamped-sidecar). Conservative: keeps source if no
sha-matching dest landing found ("kept unsafe" counter). E2E
asserts both the require-backup-id refusal AND the actual deletion.
5. archived-stale outcome now counted in n_copied (was reporting as
"other"; commit_one emits archived-stale but commit_source counted
only copied-stale).
6. skills/sutando-migrate/SKILL.md phase 5 corrected: health-check
legacy warnings WILL still fire after default commit (sources
preserved); cleared only after --delete-source phase 2.
7. CLAUDE.md adds "Migration transition window (30-day reader-fallback)"
section documenting the no-delete-default + reader-side fallback
contract that pairs with the migration script.
Lucy's carve-out:
- state/{core-status,quota-state,dynamic-content,voice-state,contextual-chips}.json
→ structural (was state/*.json|newest-mtime which would drop a host's
data on multi-host scan; same per-host preservation pattern as
state/auth/*).
Owner-directed quarantine catchall (Lucy #design 2026-06-02 +
"shall we expand?" directive):
- WALK_FULL_TREE for sources B + C: tops-level entries NOT in the
canonical surface AND not in QUARANTINE_EXCLUDES (.git, node_modules,
.cache, dist, etc.) are routed through the catchall.
- New CLASS_RULES catchall: *|quarantine-unknown → routes file to
<dest>/legacy/<src-tag>/quarantine/<rel>. Net: user content
(experiments/, obsidian-vault/, personal-src/, repro-*.ts) is
preserved namespaced rather than silently skipped.
- Source A (repo root) stays surface-restricted; walking its full
tree would migrate sutando code.
- New commit_one outcome `quarantined` + per-source `n_quarantined` counter.
- E2E asserts 4 fixture-quarantined paths (experiments/note.md,
obsidian-vault/daily.md, repro-bug.ts, personal-src/lib.py).
Per-row tests updated: state/* per-host JSONs assert structural;
novel root file asserts catchall quarantine-unknown class.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sutando-migrate): address Mini's 4 follow-up blockers on b62b9a0
PR #1403 re-review batch — Mini's diff-only post-b62b9a0 review found
4 new blockers; this fixes all of them + the test-flakiness Mini's E2E
mention prompted.
Mini's 4 new blockers:
1. verify_main + --delete-source lookups for legacy sidecars were only
globbing `legacy-<src_tag>-*` and `legacy-prior-from-<src_tag>-*`.
But Mini #3's timestamped-sidecar fix names the sidecar after the
OVERWRITING source (e.g. C's content lives at
`<path>.legacy-prior-from-A-<ts>` because A overwrote C). Source C's
lookup would miss its own content there. Now both lookups walk ALL
`<path>.legacy-*` siblings + sha-compare to find content regardless
of tag attribution.
2. --delete-source replayed the full commit walk for sources that
already had a sentinel (my sentinel guard `[ DELETE_SOURCE = 0 ]`
was too permissive). Phase 2 would duplicate append-class content
+ create extra collision sidecars before the rm pass. Now: when
sentinel exists AND DELETE_SOURCE=1, skip the commit walk entirely
and only do the per-file sha-verified delete pass.
3. Quarantine class was treated as pass-no-check in verify + skipped
in --delete-source. Contradicts the documented "deletion verifies
canonical OR sidecar OR quarantine" claim. Removed quarantine-unknown
from both early-skip lists so the standard candidate walk
(dst_canonical / dst_sidecar / dst_quarantine + timestamped sidecars)
considers it via sha_match.
4. QUARANTINE_EXCLUDES only filtered top-level entries; a custom dir
like experiments/node_modules/ would still get walked + quarantined
recursively (undercutting "don't hoover runtime/build artifacts").
Now: after computing rel, walk its segments and skip if ANY matches
QUARANTINE_EXCLUDES.
Test-suite stability (caught via 10-run reproduction):
- Backup tarball was self-including when two backup_dest calls landed
in the same second: BACKUP_ID was `date +%s` resolution → first commit
+ idempotency re-run could collide. Second backup's tar then included
the first backup as a state/ file, and rollback's tar -xzf refused to
overwrite the existing archive. Now BACKUP_ID appends -p$$r$RANDOM so
every backup_dest call has a globally unique ID + path.
- Rollback wraps cleanup walks in `set +e` so per-file failures (rm of
non-existent paths, grep with empty listing, BSD-tar idiosyncrasies)
don't shortcut the entire rollback.
- Empty-backup case (dest was bootstrap-only at backup time) now skips
tar -xzf (BSD tar errors on 0-byte input) and only runs the cleanup
walk — exactly what we want for rolling back to a known-empty state.
- Backup writes to mktemp outside dest/state/ then mv's atomically in.
Tar creation no longer races with its own destination.
- The no-sources-detected early exit now bypasses rollback + verify
modes (they don't need sources, just dest).
- Rollback's surface walk uses process substitution (no subshell rm).
- Source detection inside rollback_main: removed; rollback only needs
dest.
E2E adjustments:
- Rollback test now runs BEFORE --delete-source (mutations from delete
could interfere with rollback's removed-since-backup walk).
- 3-way collision E2E: A oldest → C → B newest produces unique
timestamped+tagged sidecars (covered by the per-run RANDOM suffix).
- --delete-source --backup-id test uses COMMIT2_BACKUP_ID (post-rollback
fresh commit), not INITIAL.
- All asserts pass 10/10 across stability runs (was 3/10 pre-fix).
Owner's prior migration (backup-id 20260602T061757Z) unaffected — uses
the older naming pattern. The fix is forward-only.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sutando-migrate): Source A excludes docs/agents/email-drafts/agent-inbox
Mini #7's defensive surface adds (agents/, docs/, email-drafts/,
agent-inbox/ → structural) were applied uniformly to all sources A/B/C.
But Source A IS the sutando repo root, where docs/ contains the project's
own documentation (code-side, tracked in git, NOT workspace data). The
walk picked them up → migrate copied to <workspace>/docs/ →
--delete-source sha-matched and rm'd them from <repo>/docs/.
Caught 2026-06-02 07:24 PT after owner ran repo-root cleanup; restored
via git checkout HEAD -- docs/ (18 tracked files).
Fix: explicit SOURCE_A_EXCLUDE constant lists the dirs in Mini's #7
defensive set that EXIST at the sutando repo root as code rather than
workspace data. scan_source + commit_source both skip those when
tag=A. Sources B and C (real workspace dirs) still get the full
WORKSPACE_SURFACE_DIRS treatment.
The principle (per new memory feedback_per_source_surface_lists):
- Source A walks ONLY M0-canonical workspace surface dirs
- Sources B+C walk those PLUS Mini #7 defensive non-canonical adds
- A's surface is a strict subset of B+C's surface — never the other way
- When adding to WORKSPACE_SURFACE_DIRS, ask "does Source A have a
same-named dir that contains REPO CODE?" — if yes, add to
SOURCE_A_EXCLUDE too
Verified: real scan against Source A now reports 0 structural files
(was previously walking docs/ + agents/). E2E + per-row tests still pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sutando-migrate): exclude code-side dirs by sutando-repo CONTENT, not tag
Owner clarified 2026-06-02 07:29: "We should only EXCLUDE them when they
are in the sutando repo root." Previous commit 5c64d42 used the per-source
tag (A/B/C) which equates Source A === repo root. But a custom workspace
path that happens to live inside another sutando checkout (or anyone
running migrate from a non-standard A) would slip past the tag check.
Now: detect "is this source path a sutando repo checkout?" by content —
presence of `src/sutando_config.py` OR `scripts/sutando-config.sh` at
source root. If yes, skip dirs in SOURCE_A_EXCLUDE (docs/, agents/,
email-drafts/, agent-inbox/) regardless of which tag (A/B/C) the source
got.
Content-based detection generalizes cleanly: if a future M-N moves the
sutando-config helper, the detection check moves with it.
Tests still pass (10/10 lifecycle + 41/41 per-row).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sutando-migrate): M1 Part 2 follow-ups (bucket/DEST/narrative-log) (#1406)
* fix(sutando-migrate): M1 Part 2 follow-ups — scan bucket + DEST hook + narrative-log append
Three follow-up fixes for `scripts/sutando-migrate.sh` surfaced during PR
#1403's E2E gates on owner's MBP migration. All approved-out-of-band by
Mini + Lucy in #design (2026-06-02) as clean follow-ups, not approval
blockers.
1) scan report missing `quarantine-unknown` bucket. Synthetic B fixture
with 7 files (4 canonical + 3 non-canonical) showed only 4 in the
action breakdown; the 3 quarantined files committed correctly but
were invisible in scan's summary. UX gap — operator reads silent
quarantine as "didn't migrate" and may rm wrongly. Adds `n_quarantine`
counter + scan-bucket display line; `rehome-narrative-log` and
`rehome-dated-snapshot` also fold into the `rehome` bucket (previously
fell through the case statement uncounted).
2) no `SUTANDO_MIGRATE_DEST` env hook. Script derived dest from
`REPO_DIR` (= script's `SCRIPT_DIR/..`); to run against a custom dest
you had to clone the whole repo and call the script from inside the
clone. Asymmetric with the existing `SUTANDO_MIGRATE_SRC_{A,B,C}`
E2E hooks. Adds pre-arg-parsing override symmetric with src hooks;
takes precedence over the resolver AND `--respect-env`.
3) `rehome-narrative-log` class is data-lossy. Shared the rehome-state /
rehome-dated-snapshot newest-mtime handler, which silently DROPPED
the larger file when the newer one was smaller. Caught on owner's
MBP: Source A had 261K of voice-agent transcript history (mtime
May 17); Source C had a 2K phone-call snippet (mtime May 20, newer);
newest-mtime kept C's 2K and discarded A's 261K. Only `<repo>/
conversation.log` preserved A's content; was sidecar'd manually
before `--delete-source` would have erased it.
General rule baked in: **snapshot → newest-mtime; append-only /
accumulator → append**. Same failure class as
`state/*.json|newest-mtime` previously carved out for per-host
state (#design 2026-06-02). Narrative-log now appends each source's
content to `<dest>/logs/workspace-narrative.log` with a divider
header carrying src-tag + mtime + size, mirroring Mini's spec.
Also removes a dead-code duplicate block (post-`return 0`) that was
unreachable post-shared-handler.
`find -prune` perf nit (Mini, #1403 review) acknowledged but deferred
to a separate perf-only PR — non-blocking for correctness.
Test plan
- Existing `tests/sutando-migrate-rules.test.sh` — all rule assertions pass
- Existing `tests/sutando-migrate.test.sh` — full lifecycle (scan / commit /
verify / rollback / --delete-source) pass
- Manual smoke: 2-source narrative-log fixture (B old+larger, C newer+smaller)
→ dest contains BOTH contents with divider headers; A's 261K-equivalent
preserved alongside C's 2K-equivalent (vs. pre-fix where A's was dropped)
- Synthetic B fixture with quarantine: scan now shows `quarantine: 3` for
obsidian-vault/experiments/personal-src; commit still routes them to
`<workspace>/legacy/B/quarantine/*` (sha-verified)
- DEST env hook: scan + commit against `/tmp/dest` without cloning the
repo
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sutando-migrate): rehome-narrative-log append must trip set -e on failure (Mini #1406)
Mini's #1406 blocker (2026-06-02 08:10Z #design):
> The append write uses `{ ... } > "$tmp" && mv ...` but then unconditionally
> echoes `appended` / `appended-fresh`. In bash, `set -e` does not exit for
> failures on the left side of `&&`; a failed `cat`/write can be reported as
> success, and if the caller deletes source on success this becomes data-lossy.
> Split into simple commands or explicitly fail before echoing.
Root cause: `{ ... } > tmp && mv ...` is an `&&` compound. `set -e` has an
explicit carve-out for `&&`/`||` chains — left-side failures don't trip the
shell exit. When the redirect failed (read-only dest, ENOSPC, EACCES, etc.),
execution silently fell through to `echo "appended"`, propagating success up
to commit_one()'s caller. A subsequent `--delete-source` against the source
file would then erase the unmigrated content — same data-loss class as the
narrative-log newest-mtime bug this PR fixed in the first place.
Fix: split each commit_one branch into separate statements that each have
explicit error returns:
- Redirect captures rc into `_redirect_rc`; non-zero → rm tmp + return 1.
- `mv` failure → rm tmp + return 1.
- Only success path echoes `appended` / `appended-fresh` + returns 0.
Atomicity preserved: dest is untouched until `mv` completes; tmp is cleaned
up on every failure path.
Test plan
- Existing rules + lifecycle tests still green.
- New manual smoke: chmod 555 on dest/logs/ to force the redirect to ENOENT.
Pre-fix: would echo "appended" and report success.
Post-fix: writes "append-failed-redirect" to stderr + returns 1; dest's
pre-existing content is preserved unchanged ("pre-existing", not the
partially-written or empty file).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sutando-migrate): rehome-narrative-log idempotency grep-guard (folds #1407 into #1406)
Lucy's #1407 nit folded in per owner direction 2026-06-02 08:18Z.
Before this commit, re-running `commit` against the same surviving source
(e.g. accidentally, or after an aborted --delete-source) would re-append the
same source content again to <workspace>/logs/workspace-narrative.log,
growing it linearly with each run. The dest narrative log doubled in length
with no warning.
Fix: before appending, `grep -qF "migrated from source $tag "` against the
canonical narrative log. If the source-tag's divider header is already there,
skip the append + return "append-skip-idempotent". Cheap single-line check;
the tag substring is sufficient because only one append per source-tag
should ever land in the canonical log.
Trade-off: a malicious or corrupted divider line that happens to contain
"migrated from source <tag>" would suppress the legitimate append. Acceptable
because (a) the canonical narrative log is owned by this script, not user-
editable in normal flow, and (b) the worst case is a silent skip, not data
loss.
Test plan
- Existing rules + lifecycle tests still green.
- E2E-A (3-source narrative integration): commit C+A+B → dest contains all 3
sections with correct divider headers, no clobbering. Pass.
- E2E-B (re-run idempotency): commit again with --force, same sources → dest
divider count stays at 3 (was 3 after first run). Grep-guard prevents
duplicate sections. Pass.
- E2E-C (smoke scan against real MBP workspace): scan runs cleanly with the
new code; bucket display includes quarantine counter when relevant; no
regressions vs. pre-fold-in behavior. Pass.
Closes #1407.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(claude-config): M0+M1 for claude-config-dir — claude-sutando shell + --migrate engine + post-copy path rewriting (#1415)
* feat(workspace): claude_sutando_config_dir contract
Foundation for M2: define and resolve the workspace-scoped
CLAUDE_CONFIG_DIR target used by the `claude-sutando` shell function
(shipped in the follow-up commit).
The path is constrained to a sub-folder of the workspace so the M2
vault sync engine can include it via a single workspace-relative glob.
Escaping the workspace via absolute paths or `..` segments breaks
sync coherence — schema regex rejects both at load, and the loader
re-asserts the invariant at runtime (defense in depth, catches symlink
escapes the regex can't see).
Resolution returns the un-canonicalized `<workspace>/<subdir>` form so
callers get a string-prefix consistent with resolve_workspace() — on
macOS, `/tmp/...` doesn't become `/private/tmp/...` just because we
passed through realpath.
Adds:
- docs/sutando-config.schema.json — claude_sutando_config_dir block,
adjacent to workspace (related concerns grouped). subdir pattern
rejects absolute paths and `..` escapes.
- src/sutando_config.py — resolve_claude_sutando_config_dir() with
runtime invariant assertions and known-keys-set update so users
providing the (documented) override don't trigger the misleading
"unknown key — loader will ignore" warn.
- src/sutando_config.ts — TS port. Same logic, ENOENT-tolerant
realpath for first-run case.
- scripts/sutando-config.sh — `claude-sutando-config-dir` subcommand
alongside the existing `workspace` / `vault-*` siblings.
- tests/sutando-config.test.py — +6 cases (default, override, absolute
reject, parent escape, deep parent escape, env-workspace override
regression) + extend test_known_keys_only_does_not_warn to cover the
new key. 30/30 OK.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(workspace): claude-sutando shell function + startup wire-up
Builds on the claude_sutando_config_dir contract (parent commit) by
shipping the consumer side — a `claude-sutando` shell function that
multiple Sutando checkouts on the same machine can each invoke from
their own working tree, mapping to that checkout's workspace-scoped
CLAUDE_CONFIG_DIR.
Why a shell FUNCTION and not an alias: a static alias bakes in one
absolute path, breaking the multi-instance case (owner runs both
~/Documents/github/sutando and a second checkout). The function
resolves the right `.claude-sutando` per-invocation via `git rev-parse`
on the caller's cwd, so each checkout self-routes.
Why a managed marker BLOCK and not a single line: lets us evolve the
body (alias↔function, future tweaks) idempotently without bricking
existing rc files. The block also enables clean migration of a
pre-marker legacy alias — `--commit` detects the old line, strips it,
and installs the managed block.
Adds:
- scripts/sutando-shell-setup.sh — dry-run / --commit / --auto /
--check / --migrate modes. Detects user's shell (zsh / bash on
macOS+Linux), drift-detects an existing alias or block, atomic
rewrites via tmp+mv. --migrate rsyncs ~/.claude → workspace's
.claude-sutando (non-destructive, idempotent).
- src/startup.sh — calls the helper with --auto early in the bootstrap
chain. Per-host sentinel prevents re-pestering after the user's
initial yes/no.
- tests/sutando-shell-setup.test.sh — 11 cases covering --check (4
states), --commit (4 states), --migrate (2 states), dry-run. Caught
5 real bugs in the helper during development (awk -v multi-line,
stderr-into-path, rsync|head SIGPIPE, /dev/tty openability, macOS
rsync 2.6.9 `--info=stats1` incompat) — all fixed.
- tests/startup-shell-setup-wireup.test.sh — 4 cases. Tests 1-3 stub
the helper to absent / non-zero / zero exit and assert startup.sh
continues past the wire-up. Test 4 is a source-tied guard: greps
startup.sh for the actual wire-up block so future refactors that
drop the `|| true` swallow or `--auto` flag get caught.
All 15 tests pass. Test isolation: each test runs in a fresh HOME +
SUTANDO_WORKSPACE tmpdir; no real user state is touched.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(sutando-app): start-cli sets CLAUDE_CONFIG_DIR per workspace
Completes the M2 surgery sketched in
notes/sutando-app-claude-config-dir-design-2026-06-02.md: make Sutando
itself (not just the user's interactive shell) launch claude with the
workspace-scoped CLAUDE_CONFIG_DIR. Sessions, auto-memory, settings
land under <workspace>/.claude-sutando/ where the M2 vault sync engine
can include them.
Single surgical point: `scripts/start-cli.sh` (the canonical launcher
that both `startup.sh` and Sutando.app's "Restart Core" call). Adding
a one-block export near the top means all three claude spawn sites in
the script (no-tmux fallback, TTY exec, no-TTY detached) inherit the
env — no per-site edits.
Defense in depth on failure modes:
- M0 helper missing → silent fallback to default ~/.claude/.
Legacy / extracted-tarball installs keep working.
- Helper present + config violates the workspace-sub-folder invariant
→ refuse to start (exit 1) with the loader's error message. Silently
falling back to ~/.claude/ would hide a real config error AND
scatter state into a location the M2 vault sync engine can't see —
worse than failing visibly.
Adds tests/start-cli-claude-config-dir.test.sh — 4 cases:
1. helper missing → silent fallback, claude spawns w/o env
2. helper + valid config → CLAUDE_CONFIG_DIR exported to claude
3. helper + invariant-violating config → exit 1, claude never spawns
4. source-tied guard: grep start-cli.sh for the block, catches a
future refactor that drops it without intent.
Test isolation: each case runs in a fresh temp REPO + fresh HOME +
fresh SUTANDO_WORKSPACE. Stub `claude` records env to a file; stubs
`tmux` to exec claude directly (bypass tmux); stubs `pgrep` to return
no-matches so the test runner's own claude session doesn't trigger
start-cli's "already running" short-circuit. 4/4 OK.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(sutando-shell-setup): scope --migrate to true project sub-folders
The shipped --migrate (in commit 9676687) copied ALL of ~/.claude into
the new workspace-scoped tree. Owner asked it to scope to THIS project
only — exact slug + any subdir-cd variants — but skip sibling repos
with similar-prefix names (sutando-plus, sutando-v07, sutando-resources).
The slug encoding `/`→`-` is lossy, so we can't tell from the slug
alone whether `-Users-...-sutando-workspace` came from:
- cd /Users/.../sutando/workspace/ (true subdir of this checkout)
- cd /Users/.../sutando-workspace/ (sibling repo with similar name)
Resolution: filesystem disambiguation. For each candidate slug under
`~/.claude/projects/`:
1. Match `<THIS_SLUG>` exact OR `<THIS_SLUG>-*` prefix.
2. Decode the `-*` suffix back to a path:
- `--` → `/-` (preserves leading-dash dirnames like
notes/-project-board-meeting)
- remai…
john-the-dev
added a commit
to john-the-dev/sutando
that referenced
this pull request
Sep 3, 2026
…mber `recorded` held only PR numbers, so a full URL to one repo suppressed the refusal for a DIFFERENT repo's same-numbered shorthand. Reproduced at the parent: allow "…/ag2-space/backend/pull/12 and sonichi#12" WRONG That records ag2-space/backend#12 and nothing for sonichi#12, which pr-unattended then reads as NOBODY_EVER_ASKED -- this guard's own failure mode, arriving through the guard. It needs two repos and a colliding number in one message, which is ordinary: low PR numbers collide across repos constantly and cross-repo asks are normal here. Keys on the pair when the shorthand carries a repo, and falls back to the number alone for bare `#n`, which is genuinely repo-ambiguous. Keying on the pair unconditionally would have started refusing the legitimate same-repo case, so that case is now a named control test rather than an assumption. Also states the two-digit minimum as the deliberate trade it is: `sonichi#7` is a real PR and passes silently, which costs nothing in a four-digit repo and turns the guard off for a younger repo's first 90 PRs. Said out loud so the next reader does not "fix" it into matching prose. Found by yixuan-ag2 in review, with the fix form and an 8-case table. Evidence: parent (fix reverted, both new tests retained) FAIL: test_a_url_to_ANOTHER_repo_does_not_cover_a_colliding_number Ran 13 tests FAILED (failures=1) HEAD Ran 13 tests OK The same-repo control passes at BOTH heads by design: it guards against over-refusal, so it must not depend on this fix. siblings at HEAD, all rc=0: sci-notify-reviewers (29), -kind (8), -inproc (57), -refusal-basis (9). ruff clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
john-the-dev
added a commit
that referenced
this pull request
Sep 3, 2026
…3771) * fix(notify-reviewers): refuse a shorthand PR ask before it is sent record_asks() logs only full github.com/<owner>/<repo>/pull/<n> URLs. `#3760` and `#3622` deliver cleanly and record nothing, so pr-unattended then reads a correctly-asked PR as NOBODY_EVER_ASKED. #3562 made that loud, and the warning is correctly worded but prints after the send: it names a loss it structurally cannot prevent. The remaining guard was a note at the point of use, which failed at its own job -- the same note was filed twice in three days by an agent who did not remember filing it the first time. So the check moves ahead of the send. It refuses only SHORTHAND, never absence: an ask need not concern a PR, and a guard that also blocks the legitimate no-PR ask gets routed around, leaving neither guard nor warning. The refusal names the token it refused and the URL that would satisfy it, constructing the exact URL when the shorthand carries the repo, so a tripped send can be completed rather than abandoned. Requested by yixuan-ag2, who authored #3562 and owns the "loud, not fatal" choice this changes. Evidence: parent (fix reverted, new test retained) Ran 11 tests FAILED (failures=2, errors=6) HEAD Ran 11 tests OK siblings at HEAD, all rc=0: sci-notify-reviewers.test.py Ran 29 tests OK sci-notify-reviewers-kind.test.py Ran 8 tests OK sci-notify-reviewers-refusal-basis.test.py Ran 9 tests OK sci-notify-reviewers-inproc.test.py Ran 57 tests OK notify-reviewers-human-shapes.test.py Ran 8 tests OK Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(notify-reviewers): key the recorded set on (repo, number), not number `recorded` held only PR numbers, so a full URL to one repo suppressed the refusal for a DIFFERENT repo's same-numbered shorthand. Reproduced at the parent: allow "…/ag2-space/backend/pull/12 and #12" WRONG That records ag2-space/backend#12 and nothing for #12, which pr-unattended then reads as NOBODY_EVER_ASKED -- this guard's own failure mode, arriving through the guard. It needs two repos and a colliding number in one message, which is ordinary: low PR numbers collide across repos constantly and cross-repo asks are normal here. Keys on the pair when the shorthand carries a repo, and falls back to the number alone for bare `#n`, which is genuinely repo-ambiguous. Keying on the pair unconditionally would have started refusing the legitimate same-repo case, so that case is now a named control test rather than an assumption. Also states the two-digit minimum as the deliberate trade it is: `#7` is a real PR and passes silently, which costs nothing in a four-digit repo and turns the guard off for a younger repo's first 90 PRs. Said out loud so the next reader does not "fix" it into matching prose. Found by yixuan-ag2 in review, with the fix form and an 8-case table. Evidence: parent (fix reverted, both new tests retained) FAIL: test_a_url_to_ANOTHER_repo_does_not_cover_a_colliding_number Ran 13 tests FAILED (failures=1) HEAD Ran 13 tests OK The same-repo control passes at BOTH heads by design: it guards against over-refusal, so it must not depend on this fix. siblings at HEAD, all rc=0: sci-notify-reviewers (29), -kind (8), -inproc (57), -refusal-basis (9). ruff clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
bassilkhilo-ag2
added a commit
that referenced
this pull request
Sep 23, 2026
…ce gate's notice names the card user feedback's "core got stuck for a long time with no visible reason" (#7 on the tracker) turned out to be two things, and both were one sentence away. The watcher "keeps dying". Every startup skill armed the session watcher with `persistent: true`. The Monitor tool has no such option (its schema is command, description, timeout_ms, ws), so the flag was ignored and the watcher was killed at the tool's 5-minute default. The external standby supervisor re-armed ~50 s later and the next loop pass re-armed the session watcher, so tasks kept flowing — but the owner saw a handoff every few minutes and a health probe that said "watcher not running", which read as the watcher dying over and over. The skills now pass `timeout_ms: 1800000` (the tool's maximum) and say what the expiry notice is and that the watcher is re-armed on it; the durable-Monitor entries in schedule-crons step 5.4 get the same treatment; CLAUDE.md carries the rule in the same paragraph, at the cost of one historical sentence, because the file sits 17 bytes under its 40 KiB always-loaded budget (AGENTS.md is regenerated from it). The health-check hint says the standby announces tasks through the pane meanwhile, so the warning no longer implies an outage. The prompt that waits for a click. A numbered picker (the model prompt she hit) or a permission dialog is already escalated as blocked-human AND projected to the owner's DM as a HITL choice card whose buttons type the answer (src/hitl/tui_gate.py). The relay's text notice, though, only named the tmux attach command — useless to a desktop owner. For selection and permission gates the notice now names the card first and the terminal second; a typed chat reply still cannot answer it, and that sentence stays. Not changed: the standby/supervisor design (#4477/#4585/#4602) is the recovery owner and stays so; no third watcher is added (UX-002: a second deliverer on one inbox would process every task twice). Tests: core-supervisor-relay (card sentence for selection/permission, not for unknown), docs-watcher-launch-is-tagged, proactive-loop-skill-budget, agents-md-sync. tests/health-check-task-watcher.test.py case n ("the subshell must be grouped under its script root") fails on origin/main before this change too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bassilkhilo-ag2
added a commit
that referenced
this pull request
Sep 23, 2026
…ce gate's notice names the card user feedback's "core got stuck for a long time with no visible reason" (#7 on the tracker) turned out to be two things, and both were one sentence away. The watcher "keeps dying". Every startup skill armed the session watcher with `persistent: true`. The Monitor tool has no such option (its schema is command, description, timeout_ms, ws), so the flag was ignored and the watcher was killed at the tool's 5-minute default. The external standby supervisor re-armed ~50 s later and the next loop pass re-armed the session watcher, so tasks kept flowing — but the owner saw a handoff every few minutes and a health probe that said "watcher not running", which read as the watcher dying over and over. The skills now pass `timeout_ms: 1800000` (the tool's maximum) and say what the expiry notice is and that the watcher is re-armed on it; the durable-Monitor entries in schedule-crons step 5.4 get the same treatment; CLAUDE.md carries the rule in the same paragraph, at the cost of one historical sentence, because the file sits 17 bytes under its 40 KiB always-loaded budget (AGENTS.md is regenerated from it). The health-check hint says the standby announces tasks through the pane meanwhile, so the warning no longer implies an outage. The prompt that waits for a click. A numbered picker (the model prompt she hit) or a permission dialog is already escalated as blocked-human AND projected to the owner's DM as a HITL choice card whose buttons type the answer (src/hitl/tui_gate.py). The relay's text notice, though, only named the tmux attach command — useless to a desktop owner. For selection and permission gates the notice now names the card first and the terminal second; a typed chat reply still cannot answer it, and that sentence stays. Not changed: the standby/supervisor design (#4477/#4585/#4602) is the recovery owner and stays so; no third watcher is added (UX-002: a second deliverer on one inbox would process every task twice). Tests: core-supervisor-relay (card sentence for selection/permission, not for unknown), docs-watcher-launch-is-tagged, proactive-loop-skill-budget, agents-md-sync. tests/health-check-task-watcher.test.py case n ("the subshell must be grouped under its script root") fails on origin/main before this change too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
bassilkhilo-ag2
added a commit
that referenced
this pull request
Sep 26, 2026
…ce gate's notice names the card (#4662) * fix(core): the task watcher's Monitor names its real lifetime; a choice gate's notice names the card user feedback's "core got stuck for a long time with no visible reason" (#7 on the tracker) turned out to be two things, and both were one sentence away. The watcher "keeps dying". Every startup skill armed the session watcher with `persistent: true`. The Monitor tool has no such option (its schema is command, description, timeout_ms, ws), so the flag was ignored and the watcher was killed at the tool's 5-minute default. The external standby supervisor re-armed ~50 s later and the next loop pass re-armed the session watcher, so tasks kept flowing — but the owner saw a handoff every few minutes and a health probe that said "watcher not running", which read as the watcher dying over and over. The skills now pass `timeout_ms: 1800000` (the tool's maximum) and say what the expiry notice is and that the watcher is re-armed on it; the durable-Monitor entries in schedule-crons step 5.4 get the same treatment; CLAUDE.md carries the rule in the same paragraph, at the cost of one historical sentence, because the file sits 17 bytes under its 40 KiB always-loaded budget (AGENTS.md is regenerated from it). The health-check hint says the standby announces tasks through the pane meanwhile, so the warning no longer implies an outage. The prompt that waits for a click. A numbered picker (the model prompt she hit) or a permission dialog is already escalated as blocked-human AND projected to the owner's DM as a HITL choice card whose buttons type the answer (src/hitl/tui_gate.py). The relay's text notice, though, only named the tmux attach command — useless to a desktop owner. For selection and permission gates the notice now names the card first and the terminal second; a typed chat reply still cannot answer it, and that sentence stays. Not changed: the standby/supervisor design (#4477/#4585/#4602) is the recovery owner and stays so; no third watcher is added (UX-002: a second deliverer on one inbox would process every task twice). Tests: core-supervisor-relay (card sentence for selection/permission, not for unknown), docs-watcher-launch-is-tagged, proactive-loop-skill-budget, agents-md-sync. tests/health-check-task-watcher.test.py case n ("the subshell must be grouped under its script root") fails on origin/main before this change too. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(core): name the choice card only where one with buttons is projected; trim the relay comment Review findings on the previous revision. Blocker: `typecheck + node & shell tests` was red because the new block in src/core-supervisor-relay.py compose_message was five comment lines with incident history (prose-cap allows two). The comment now states only why the card comes before the terminal. Blocker: the selection/permission branch always told the owner to "tap an option on the choice card in our DM", which is false whenever no such card exists: a prompt matching tui_gate's never-one-click set (trust the files, Bypass Permissions, usage/weekly limit) or one whose options cannot be parsed gets the blocked card with "Open terminal" only, and the card is projected into AG2 Space rooms only, while this relay also delivers to discord, slack and telegram (and macOS-only). compose_message now takes the delivery `surface` (run_cycle passes its `source`) and names the card only when `_card_projected` says so: the surface is an AG2 Space one and tui_gate.requirement_for on the signal's state/kind/prompt returns at least one action other than open_terminal. Every other case keeps the terminal wording. A failed lookup also falls back to the terminal wording, since the notice is the owner's only channel. Tests (tests/core-supervisor-relay.test.py, 77 -> 80): a parseable picker and a Yes/No permission dialog on ag2space name the card first; the same picker on discord, slack, telegram and macOS-only does not; the three never-one-click prompts and an unparseable prompt on ag2space do not; a configured homeserver bridge surface does; an unimportable hitl package falls back; run_cycle hands the source through. Nits: the dated narrative in docs/proactive-loop-rationale.md and skills/schedule-crons/SKILL.md is reduced to the rule, and "the tool ignored persistent: true" (not verified against the tool's schema) is now "passing one does not keep the command alive". The "45 s grace" in the health-check hint matches GRACE_PERIOD's default in src/agent/codex/cli/task-notifier-supervisor.sh, which the Claude CLI launcher also runs. Verification: python3 tests/core-supervisor-relay.test.py (80 OK); scripts/review-checks.sh --diff PASS; diff coverage of changed Python lines 100%; lint-workspace-resolution --diff clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Oct 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Caps reconnect at 5 attempts, shows actionable troubleshooting message.