Repository navigation
Improve reconnect error message - #8
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
liususan091219
added a commit
that referenced
this pull request
Mar 31, 2026
agent-api.py: - Add _safe_id() to sanitize task IDs against path traversal (#2-4, #8) - Reject path traversal in /media/ before resolve (#5-7) - Sanitize mime type to prevent HTTP response splitting (#1) inline-tools.ts: - Escape backslashes in shell args for openUrl and switchApp (#9-11) conversation-server.ts: - Mask phone number in startup log (#12) - Escape meetingId/passcode in TwiML Play digits (#13) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sonichi
pushed a commit
that referenced
this pull request
Mar 31, 2026
* Fix 13 CodeQL security alerts agent-api.py: - Add _safe_id() to sanitize task IDs against path traversal (#2-4, #8) - Reject path traversal in /media/ before resolve (#5-7) - Sanitize mime type to prevent HTTP response splitting (#1) inline-tools.ts: - Escape backslashes in shell args for openUrl and switchApp (#9-11) conversation-server.ts: - Mask phone number in startup log (#12) - Escape meetingId/passcode in TwiML Play digits (#13) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add security comments to shell escaping in inline-tools Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
sonichi
pushed a commit
that referenced
this pull request
Apr 17, 2026
Followup to #396. The find in section 7 used `-newer "$OUT/meta.txt"`, but meta.txt is created at the start of gather.sh. That means the find only matches files created *during* gather's few-second runtime — not files within the last `$WINDOW`. `results-recent-paths.txt` was empty in every real run. Switch to `-mmin -$((SECONDS_AGO/60))` so "recent" actually means recent. Also add test #8 to test-gather.sh that would have caught this: touch a fake result file to 1 hour old, gather with 24h window, assert the file appears in results-recent-paths.txt. Full suite now 15 cases, all pass. macOS BSD date + linux GNU date both handled for the backdate step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sonichi
added a commit
that referenced
this pull request
Apr 17, 2026
Followup to #396. The find in section 7 used `-newer "$OUT/meta.txt"`, but meta.txt is created at the start of gather.sh. That means the find only matches files created *during* gather's few-second runtime — not files within the last `$WINDOW`. `results-recent-paths.txt` was empty in every real run. Switch to `-mmin -$((SECONDS_AGO/60))` so "recent" actually means recent. Also add test #8 to test-gather.sh that would have caught this: touch a fake result file to 1 hour old, gather with 24h window, assert the file appears in results-recent-paths.txt. Full suite now 15 cases, all pass. macOS BSD date + linux GNU date both handled for the backdate step. Co-authored-by: Chi <wangchi@Chis-Mac-mini.local> Co-authored-by: Claude Opus 4.7 (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>
4 of 5 tasks
qingyun-wu
added a commit
that referenced
this pull request
May 18, 2026
…#5, #6, #8) From sonichi cold-review on PR #794 — uncontroversial nice-to-haves that don't depend on the scope decisions for findings #1–#3. - #5: validateConfig now checks tools_allow is a string array. A YAML with `tools_allow: "vision_query"` (string instead of list) used to pass validation and crash later at `.length` access with a confusing error. Now fails fast at load with a clear message. - #6: vision_cadence_ms bounded to 100–5000ms. A typo like 70000 (70s) would silently produce useless frame cadence; 50ms would saturate the network. The 100–5000 range covers every plausible mode today. - #8: SKILL.md typo — "bash scripts/activate.ts --list" → "npx tsx ..." (matches the smoke-test block above; bash can't execute the .ts). No behavior change for the live guided-setup config (verified by --list and tsc clean).
qingyun-wu
added a commit
that referenced
this pull request
May 18, 2026
* feat(screen-companion): skeleton skill (M1) + pair-read-paper config
v0 skeleton for the screen-companion skill. Closes nothing yet — this
PR ships the *primitive*, not the working feature. Followup PR wires
the loaded config into voice-agent.ts.
What's in this PR:
- skills/screen-companion/SKILL.md — describes the skill, the M1 model
(master skill + N declarative configs), how to add a new use case,
trust + scope boundary.
- skills/screen-companion/configs/pair-read-paper.yaml — the first
use-case config. Demonstrates the full schema: activation triggers
(voice phrases + button label + CLI alias), vision_mode + cadence,
system_prompt_overlay with domain priors + don't-list, tools_allow
whitelist, goal_template for optional activation-time fill-in.
- skills/screen-companion/scripts/activate.ts — loader + activation
printer. v0 scope: discover configs in configs/*.yaml, validate
required fields, load one by name, print what *would* happen when
it's activated. No voice-agent wiring yet — intentional, so the
config schema can crystallize against a real concrete config
(pair-read-paper) before the integration shape is committed.
CLI:
npx tsx skills/screen-companion/scripts/activate.ts --list
npx tsx skills/screen-companion/scripts/activate.ts --config pair-read-paper
npx tsx skills/screen-companion/scripts/activate.ts --config pair-read-paper --goal "..."
Picks M1 ("master skill + declarative configs") from screen_companion.md.
Discussion + tradeoffs vs M2 (per-use-case sub-skills) + M3 (SDK) live
in that doc.
YAML parsing via python3 subprocess (same pattern as src/oc-profile-catalog.ts);
avoids adding js-yaml as an npm dep.
Next PR (followup):
- Wire system_prompt_overlay onto the active VoiceSession.
- Restrict the tool surface to tools_allow at activation time.
- Hook vision push/pull cadence.
- Add pair-debug + pair-review-code configs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(screen-companion): swap v0 config from pair-read-paper to guided-setup
Owner-picked 2026-05-17: the v0 first-shipped config is `guided-setup`
(proactive + goal-directed — Sutando narrates next steps for a UI
config task), not `pair-read-paper`. More visceral demo angle: "Sutando
helps me set up a Discord bot I've never configured before" vs. "Sutando
reads a paper with me."
Changes in this commit:
- Add skills/screen-companion/configs/guided-setup.yaml — the new v0
config. PROACTIVE shape: tighter vision cadence (700ms vs 1000ms),
prompt overlay narrates next steps continuously, explicit DO NOT
click/type guidance (Sutando narrates; user keeps the action).
- Drop skills/screen-companion/configs/pair-read-paper.yaml — was
shipping as the v0 reference config but the demo angle is weaker;
owner-clarified the framing should center on "help me set up
something I've never done before."
- Update SKILL.md table + run example + roadmap to reflect guided-setup
as the shipped config; pair-read-paper dropped (siblings pair-debug
+ pair-review-code still tracked in screen_companion.md as future).
- Update activate.ts doc header + CLI example to use guided-setup.
- Add Privacy + data handling section to SKILL.md: default DOES NOT
take screenshots, record video, or persist frames; frames are sent
to Gemini Live for real-time understanding and discarded; text
transcripts are persisted (conversation-server precedent); user
has to explicitly opt-in per request to save screenshots/recordings.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(screen-companion): wire guided-setup end-to-end via skill manifest
Closes the wiring gap left by the v0 skeleton — voice can now activate
guided-setup mode directly. Earlier passes shipped the YAML config + CLI
loader; this pass adds the missing piece:
- manifest.json — skill is enabled, points to tools.ts
- tools.ts — exports activate_screen_companion tool (loads config, returns
prompt overlay + activation_message + vision hint as a structured payload)
- scripts/load-config.ts — shared loader used by both the CLI activate.ts
and the inline tool (was duplicated; now single source of truth)
- scripts/activate.ts — refactored to import from load-config.ts (-80 lines)
- SKILL.md — drops the "v0 skeleton — load + print only" caveat, adds the
voice activation phrases + the wire-through explanation
Smoke test (matches what voice will see):
npx tsx -e "import('./skills/screen-companion/tools.ts').then(...)"
→ status: 'activated', mode: 'guided-setup', goal/instructions/tools_allow
all populated, activation_message ready to speak.
Architecture: tool lives in the skill (per CLAUDE.md decision guide —
self-contained feature). Voice-agent picks it up via the existing skill
manifest loader (same pattern as gws-gmail-voice). No changes to
src/inline-tools.ts or src/voice-agent.ts.
Voice-agent restart required to register the new tool. Configs themselves
are read fresh per activation, so adding more configs in later PRs does
not require another restart.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(screen-companion): tolerate LaunchAgent python3 without PyYAML
Voice-agent runs under a LaunchAgent whose PATH is
`/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin`. Homebrew's python3 wins
that lookup but does not ship PyYAML, so `python3 -c 'import yaml; ...'`
in parseYaml() failed every activation with ModuleNotFoundError. The
tool returned an error and Gemini fell back to default behavior — silent
to the owner aside from "screen companion failed."
Try a list of python3 candidates (system → homebrew → PATH default), use
the first that can run the yaml import. macOS's `/usr/bin/python3` ships
PyYAML, so the system Python is the usual winner under the LaunchAgent
PATH. Aggregate per-binary errors into the thrown message for diagnosis
when all candidates miss.
Verified under the LaunchAgent's PATH env: activation now succeeds.
* feat(vision): single-line nudge for screen-companion at share-start
The existing screen-share-started system note tells the model to say "I
can see your screen now" but stops there — so users who don't already
know the activation phrases (e.g. "help me set this up" for
guided-setup) share their screen and get no signal that pre-built
companion modes exist. Add a one-line, mode-agnostic nudge: "Want me
to help you through something?" If they describe a goal that fits any
configured mode, the model routes via `activate_screen_companion`.
Mode-agnostic on purpose — the prompt does not list configs by name, so
it won't age as new ones land (pair-debug, pair-review-code planned per
screen_companion.md). The model already knows the catalog from the
`activate_screen_companion` tool's description (which discovers configs
at boot), so listing them in the nudge would be redundant.
* feat(vision): share-start nudge names screen-companion + configs explicitly
First attempt (626a18f) shipped a mode-agnostic nudge ("Want me to help
you through something?") that did not surface the screen-companion
catalog. Owner test 2026-05-17 17:28 PT: routing worked (goal → mode)
but discoverability didn't — owner reported "I do not hear the nudge
about screen companion mode."
Updated injected prompt instructs Gemini to deliver ONE sentence that
(a) acknowledges the screen, (b) names "screen-companion mode"
explicitly, (c) names the available configs by name (Gemini reads them
from the activate_screen_companion tool description, which is
discoverConfigs() at boot — so new YAMLs surface automatically), (d)
asks the user's goal.
Example shape: "I can see your screen — screen-companion mode is
available with guided-setup for UI walkthroughs. What are you trying
to do?"
* fix(screen-companion): force ground-then-step in guided-setup overlay
Owner test 2026-05-17 17:34 PT: in a FRESH voice session with no
chat history, Gemini activated guided-setup for "set up a Discord bot"
and asserted "you're on the Discord Developer Portal" + "I see your
cursor hovering over those existing apps" — when the user was
demonstrably NOT on the dev portal. Goal-priming overrode actual
vision input. (Verified via fresh-session diagnostic — chat-history
anchoring was ruled out.)
Add a GROUNDING RULE section at the top of the overlay:
- Name what you ACTUALLY see (page title/URL/breadcrumb, dominant UI
elements, overall page kind) BEFORE naming any step.
- If the visible page doesn't match the goal's destination, do NOT
assert the user is on the right page — say what you see and ask them
to navigate.
- Never fabricate UI elements; if a label is illegible, ask the user
to read it.
- Goal text is a hint about WHERE the user is going, NOT proof of
where they ARE. Reconcile goal vs visible-screen as two separate
facts every turn.
Configs are loaded fresh on each activation — next re-activation picks
this up without a voice-agent restart.
* docs(screen-companion): downgrade tools_allow claim from 'enforced' to advisory
sonichi cold-review on PR #794 flagged: SKILL.md asserts the tool allow-
list is "enforced at activation time; configs cannot grant tools the
active VoiceSession doesn't already expose" — but tools.ts:67 just
returns the field in the activation payload as a string list. The
_note instructs Gemini to self-restrict; there is no path in the
bodhi-realtime-agent layer that scopes the live tool surface.
Update the Trust + scope section to match reality: advisory in v0,
hard enforcement deferred to a follow-up PR. The risk surface
("worst case is Gemini calls a tool you didn't intend") is named
explicitly so readers can calibrate.
Code unchanged — the field still functions as the documented intent
and Gemini does self-restrict in observed sessions; only the doc claim
was overstating.
* chore(screen-companion): defensive validation + SKILL.md typo (sonichi #5, #6, #8)
From sonichi cold-review on PR #794 — uncontroversial nice-to-haves
that don't depend on the scope decisions for findings #1–#3.
- #5: validateConfig now checks tools_allow is a string array. A YAML
with `tools_allow: "vision_query"` (string instead of list) used to
pass validation and crash later at `.length` access with a confusing
error. Now fails fast at load with a clear message.
- #6: vision_cadence_ms bounded to 100–5000ms. A typo like 70000 (70s)
would silently produce useless frame cadence; 50ms would saturate the
network. The 100–5000 range covers every plausible mode today.
- #8: SKILL.md typo — "bash scripts/activate.ts --list" → "npx tsx ..."
(matches the smoke-test block above; bash can't execute the .ts).
No behavior change for the live guided-setup config (verified by
--list and tsc clean).
* docs(screen-companion): mark guided-setup tools_allow as PLANNED (sonichi #1)
PR #794 review finding #1 — tools_allow lists three tools that don't
exist anywhere in the codebase (zero grep matches outside this skill).
sonichi's three suggested fixes were: rename to real tools, land the
missing tools, or document as schema-only. Going with the documentation
path for v0 — least-risky of the three, reversible, and consistent
with the truthfulness fix in eff1286 that already downgrades the
allow-list claim in SKILL.md from "enforced" to "advisory".
guided-setup as shipped relies only on push-mode vision frames + Gemini
Live's conversational capability — no tool calls are actually required
to operate. The PLANNED comment captures design intent without
fabricating wiring that doesn't exist; a follow-up PR registers real
tools and Gemini gets a real allow-list to follow.
Code unchanged. CI green expected — pure YAML comments.
* refactor(vision): contributor registry — remove screen-companion knowledge from core (sonichi #3)
PR #794 review finding #3: src/vision-tools.ts:254-256 hardcoded the
strings "screen-companion mode" and "guided-setup" in the screen-share-
started system note. Per CLAUDE.md decision guide, core services must
not contain feature-specific logic; if a user removed the screen-
companion skill, vision-tools.ts would lie to Gemini about modes that
no longer exist.
Refactor:
- src/vision-tools.ts now exposes `registerVisionOnContributor(fn)`.
When push mode starts, the injected note is built from a generic base
note + each contributor's output. With zero contributors registered,
the note is generic and mentions no modes.
- skills/screen-companion/tools.ts registers a contributor at module-
load time. The contributor calls discoverConfigs() at injection time,
so new YAMLs in configs/ surface automatically without code changes.
- If the skill is disabled (manifest.enabled=false) or removed, the
skill-loader at src/inline-tools.ts:937 never imports tools.ts, the
contributor never registers, and vision-tools.ts emits only the base
note — exactly matches CLAUDE.md "skills are optional."
Behavioral equivalence: with screen-companion enabled (default), the
injected note now contains the same essential content as before — just
sourced from the contributor instead of hardcoded.
Verified: tsc clean. Smoke test confirms contributor count goes 0→1
when the skill module is imported. Voice-agent restarted to pick up
the new module.
---------
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
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
* Fix 13 CodeQL security alerts agent-api.py: - Add _safe_id() to sanitize task IDs against path traversal (sonichi#2-4, sonichi#8) - Reject path traversal in /media/ before resolve (sonichi#5-7) - Sanitize mime type to prevent HTTP response splitting (sonichi#1) inline-tools.ts: - Escape backslashes in shell args for openUrl and switchApp (sonichi#9-11) conversation-server.ts: - Mask phone number in startup log (sonichi#12) - Escape meetingId/passcode in TwiML Play digits (sonichi#13) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add security comments to shell escaping in inline-tools Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
liususan091219
pushed a commit
to liususan091219/sutando-fork
that referenced
this pull request
May 28, 2026
…chi#401) Followup to sonichi#396. The find in section 7 used `-newer "$OUT/meta.txt"`, but meta.txt is created at the start of gather.sh. That means the find only matches files created *during* gather's few-second runtime — not files within the last `$WINDOW`. `results-recent-paths.txt` was empty in every real run. Switch to `-mmin -$((SECONDS_AGO/60))` so "recent" actually means recent. Also add test sonichi#8 to test-gather.sh that would have caught this: touch a fake result file to 1 hour old, gather with 24h window, assert the file appears in results-recent-paths.txt. Full suite now 15 cases, all pass. macOS BSD date + linux GNU date both handled for the backdate step. Co-authored-by: Chi <wangchi@Chis-Mac-mini.local> 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
* 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
…nichi#794) * feat(screen-companion): skeleton skill (M1) + pair-read-paper config v0 skeleton for the screen-companion skill. Closes nothing yet — this PR ships the *primitive*, not the working feature. Followup PR wires the loaded config into voice-agent.ts. What's in this PR: - skills/screen-companion/SKILL.md — describes the skill, the M1 model (master skill + N declarative configs), how to add a new use case, trust + scope boundary. - skills/screen-companion/configs/pair-read-paper.yaml — the first use-case config. Demonstrates the full schema: activation triggers (voice phrases + button label + CLI alias), vision_mode + cadence, system_prompt_overlay with domain priors + don't-list, tools_allow whitelist, goal_template for optional activation-time fill-in. - skills/screen-companion/scripts/activate.ts — loader + activation printer. v0 scope: discover configs in configs/*.yaml, validate required fields, load one by name, print what *would* happen when it's activated. No voice-agent wiring yet — intentional, so the config schema can crystallize against a real concrete config (pair-read-paper) before the integration shape is committed. CLI: npx tsx skills/screen-companion/scripts/activate.ts --list npx tsx skills/screen-companion/scripts/activate.ts --config pair-read-paper npx tsx skills/screen-companion/scripts/activate.ts --config pair-read-paper --goal "..." Picks M1 ("master skill + declarative configs") from screen_companion.md. Discussion + tradeoffs vs M2 (per-use-case sub-skills) + M3 (SDK) live in that doc. YAML parsing via python3 subprocess (same pattern as src/oc-profile-catalog.ts); avoids adding js-yaml as an npm dep. Next PR (followup): - Wire system_prompt_overlay onto the active VoiceSession. - Restrict the tool surface to tools_allow at activation time. - Hook vision push/pull cadence. - Add pair-debug + pair-review-code configs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(screen-companion): swap v0 config from pair-read-paper to guided-setup Owner-picked 2026-05-17: the v0 first-shipped config is `guided-setup` (proactive + goal-directed — Sutando narrates next steps for a UI config task), not `pair-read-paper`. More visceral demo angle: "Sutando helps me set up a Discord bot I've never configured before" vs. "Sutando reads a paper with me." Changes in this commit: - Add skills/screen-companion/configs/guided-setup.yaml — the new v0 config. PROACTIVE shape: tighter vision cadence (700ms vs 1000ms), prompt overlay narrates next steps continuously, explicit DO NOT click/type guidance (Sutando narrates; user keeps the action). - Drop skills/screen-companion/configs/pair-read-paper.yaml — was shipping as the v0 reference config but the demo angle is weaker; owner-clarified the framing should center on "help me set up something I've never done before." - Update SKILL.md table + run example + roadmap to reflect guided-setup as the shipped config; pair-read-paper dropped (siblings pair-debug + pair-review-code still tracked in screen_companion.md as future). - Update activate.ts doc header + CLI example to use guided-setup. - Add Privacy + data handling section to SKILL.md: default DOES NOT take screenshots, record video, or persist frames; frames are sent to Gemini Live for real-time understanding and discarded; text transcripts are persisted (conversation-server precedent); user has to explicitly opt-in per request to save screenshots/recordings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(screen-companion): wire guided-setup end-to-end via skill manifest Closes the wiring gap left by the v0 skeleton — voice can now activate guided-setup mode directly. Earlier passes shipped the YAML config + CLI loader; this pass adds the missing piece: - manifest.json — skill is enabled, points to tools.ts - tools.ts — exports activate_screen_companion tool (loads config, returns prompt overlay + activation_message + vision hint as a structured payload) - scripts/load-config.ts — shared loader used by both the CLI activate.ts and the inline tool (was duplicated; now single source of truth) - scripts/activate.ts — refactored to import from load-config.ts (-80 lines) - SKILL.md — drops the "v0 skeleton — load + print only" caveat, adds the voice activation phrases + the wire-through explanation Smoke test (matches what voice will see): npx tsx -e "import('./skills/screen-companion/tools.ts').then(...)" → status: 'activated', mode: 'guided-setup', goal/instructions/tools_allow all populated, activation_message ready to speak. Architecture: tool lives in the skill (per CLAUDE.md decision guide — self-contained feature). Voice-agent picks it up via the existing skill manifest loader (same pattern as gws-gmail-voice). No changes to src/inline-tools.ts or src/voice-agent.ts. Voice-agent restart required to register the new tool. Configs themselves are read fresh per activation, so adding more configs in later PRs does not require another restart. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(screen-companion): tolerate LaunchAgent python3 without PyYAML Voice-agent runs under a LaunchAgent whose PATH is `/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin`. Homebrew's python3 wins that lookup but does not ship PyYAML, so `python3 -c 'import yaml; ...'` in parseYaml() failed every activation with ModuleNotFoundError. The tool returned an error and Gemini fell back to default behavior — silent to the owner aside from "screen companion failed." Try a list of python3 candidates (system → homebrew → PATH default), use the first that can run the yaml import. macOS's `/usr/bin/python3` ships PyYAML, so the system Python is the usual winner under the LaunchAgent PATH. Aggregate per-binary errors into the thrown message for diagnosis when all candidates miss. Verified under the LaunchAgent's PATH env: activation now succeeds. * feat(vision): single-line nudge for screen-companion at share-start The existing screen-share-started system note tells the model to say "I can see your screen now" but stops there — so users who don't already know the activation phrases (e.g. "help me set this up" for guided-setup) share their screen and get no signal that pre-built companion modes exist. Add a one-line, mode-agnostic nudge: "Want me to help you through something?" If they describe a goal that fits any configured mode, the model routes via `activate_screen_companion`. Mode-agnostic on purpose — the prompt does not list configs by name, so it won't age as new ones land (pair-debug, pair-review-code planned per screen_companion.md). The model already knows the catalog from the `activate_screen_companion` tool's description (which discovers configs at boot), so listing them in the nudge would be redundant. * feat(vision): share-start nudge names screen-companion + configs explicitly First attempt (626a18f) shipped a mode-agnostic nudge ("Want me to help you through something?") that did not surface the screen-companion catalog. Owner test 2026-05-17 17:28 PT: routing worked (goal → mode) but discoverability didn't — owner reported "I do not hear the nudge about screen companion mode." Updated injected prompt instructs Gemini to deliver ONE sentence that (a) acknowledges the screen, (b) names "screen-companion mode" explicitly, (c) names the available configs by name (Gemini reads them from the activate_screen_companion tool description, which is discoverConfigs() at boot — so new YAMLs surface automatically), (d) asks the user's goal. Example shape: "I can see your screen — screen-companion mode is available with guided-setup for UI walkthroughs. What are you trying to do?" * fix(screen-companion): force ground-then-step in guided-setup overlay Owner test 2026-05-17 17:34 PT: in a FRESH voice session with no chat history, Gemini activated guided-setup for "set up a Discord bot" and asserted "you're on the Discord Developer Portal" + "I see your cursor hovering over those existing apps" — when the user was demonstrably NOT on the dev portal. Goal-priming overrode actual vision input. (Verified via fresh-session diagnostic — chat-history anchoring was ruled out.) Add a GROUNDING RULE section at the top of the overlay: - Name what you ACTUALLY see (page title/URL/breadcrumb, dominant UI elements, overall page kind) BEFORE naming any step. - If the visible page doesn't match the goal's destination, do NOT assert the user is on the right page — say what you see and ask them to navigate. - Never fabricate UI elements; if a label is illegible, ask the user to read it. - Goal text is a hint about WHERE the user is going, NOT proof of where they ARE. Reconcile goal vs visible-screen as two separate facts every turn. Configs are loaded fresh on each activation — next re-activation picks this up without a voice-agent restart. * docs(screen-companion): downgrade tools_allow claim from 'enforced' to advisory sonichi cold-review on PR sonichi#794 flagged: SKILL.md asserts the tool allow- list is "enforced at activation time; configs cannot grant tools the active VoiceSession doesn't already expose" — but tools.ts:67 just returns the field in the activation payload as a string list. The _note instructs Gemini to self-restrict; there is no path in the bodhi-realtime-agent layer that scopes the live tool surface. Update the Trust + scope section to match reality: advisory in v0, hard enforcement deferred to a follow-up PR. The risk surface ("worst case is Gemini calls a tool you didn't intend") is named explicitly so readers can calibrate. Code unchanged — the field still functions as the documented intent and Gemini does self-restrict in observed sessions; only the doc claim was overstating. * chore(screen-companion): defensive validation + SKILL.md typo (sonichi sonichi#5, sonichi#6, sonichi#8) From sonichi cold-review on PR sonichi#794 — uncontroversial nice-to-haves that don't depend on the scope decisions for findings sonichi#1–sonichi#3. - sonichi#5: validateConfig now checks tools_allow is a string array. A YAML with `tools_allow: "vision_query"` (string instead of list) used to pass validation and crash later at `.length` access with a confusing error. Now fails fast at load with a clear message. - sonichi#6: vision_cadence_ms bounded to 100–5000ms. A typo like 70000 (70s) would silently produce useless frame cadence; 50ms would saturate the network. The 100–5000 range covers every plausible mode today. - sonichi#8: SKILL.md typo — "bash scripts/activate.ts --list" → "npx tsx ..." (matches the smoke-test block above; bash can't execute the .ts). No behavior change for the live guided-setup config (verified by --list and tsc clean). * docs(screen-companion): mark guided-setup tools_allow as PLANNED (sonichi sonichi#1) PR sonichi#794 review finding sonichi#1 — tools_allow lists three tools that don't exist anywhere in the codebase (zero grep matches outside this skill). sonichi's three suggested fixes were: rename to real tools, land the missing tools, or document as schema-only. Going with the documentation path for v0 — least-risky of the three, reversible, and consistent with the truthfulness fix in eff1286 that already downgrades the allow-list claim in SKILL.md from "enforced" to "advisory". guided-setup as shipped relies only on push-mode vision frames + Gemini Live's conversational capability — no tool calls are actually required to operate. The PLANNED comment captures design intent without fabricating wiring that doesn't exist; a follow-up PR registers real tools and Gemini gets a real allow-list to follow. Code unchanged. CI green expected — pure YAML comments. * refactor(vision): contributor registry — remove screen-companion knowledge from core (sonichi sonichi#3) PR sonichi#794 review finding sonichi#3: src/vision-tools.ts:254-256 hardcoded the strings "screen-companion mode" and "guided-setup" in the screen-share- started system note. Per CLAUDE.md decision guide, core services must not contain feature-specific logic; if a user removed the screen- companion skill, vision-tools.ts would lie to Gemini about modes that no longer exist. Refactor: - src/vision-tools.ts now exposes `registerVisionOnContributor(fn)`. When push mode starts, the injected note is built from a generic base note + each contributor's output. With zero contributors registered, the note is generic and mentions no modes. - skills/screen-companion/tools.ts registers a contributor at module- load time. The contributor calls discoverConfigs() at injection time, so new YAMLs in configs/ surface automatically without code changes. - If the skill is disabled (manifest.enabled=false) or removed, the skill-loader at src/inline-tools.ts:937 never imports tools.ts, the contributor never registers, and vision-tools.ts emits only the base note — exactly matches CLAUDE.md "skills are optional." Behavioral equivalence: with screen-companion enabled (default), the injected note now contains the same essential content as before — just sourced from the contributor instead of hardcoded. Verified: tsc clean. Smoke test confirms contributor count goes 0→1 when the skill module is imported. Voice-agent restarted to pick up the new module. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
qingyun-wu
added a commit
that referenced
this pull request
Jun 2, 2026
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>
qingyun-wu
added a commit
that referenced
this pull request
Jun 2, 2026
…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>
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…
5 tasks
john-the-dev
added a commit
to john-the-dev/sutando
that referenced
this pull request
Jul 7, 2026
…token Adds a shared secret token (generated at screen-capture-server startup, stored 0600 at ~/.config/sutando/screen-capture-token) and checks it in every /capture request via the X-Sutando-Capture-Token header. A browser page on loopback cannot set custom headers on a no-cors fetch or read a local 0600 file, so this closes the CSRF vector identified in the 2026-07-06 security audit (Finding sonichi#8). All callers (inline-tools, vision-tools, recording-tools, browser-tools, pointer-teacher-tracer/resolver.py) updated to read and forward the token. readCaptureToken() helper added to util_paths.ts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh
liususan091219
added a commit
that referenced
this pull request
Jul 17, 2026
…token (#1957) * fix(screen-capture): authenticate /capture endpoint with per-startup token Adds a shared secret token (generated at screen-capture-server startup, stored 0600 at ~/.config/sutando/screen-capture-token) and checks it in every /capture request via the X-Sutando-Capture-Token header. A browser page on loopback cannot set custom headers on a no-cors fetch or read a local 0600 file, so this closes the CSRF vector identified in the 2026-07-06 security audit (Finding #8). All callers (inline-tools, vision-tools, recording-tools, browser-tools, pointer-teacher-tracer/resolver.py) updated to read and forward the token. readCaptureToken() helper added to util_paths.ts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh * test(screen-capture): auth handler regression tests for PR #1957 12 test cases covering the X-Sutando-Capture-Token gate and _load_or_create_capture_token(): - Missing/empty/wrong/partial-prefix header → 403 - Correct token → auth passes (not 403) - CAPTURE_TOKEN=None → no auth check (token loading failed path) - Correct token with query params → not 403 - Non-/capture path (/health) → no auth gate - _load_or_create: creates new 0600 token when file missing - _load_or_create: reuses existing valid token - _load_or_create: rejects world-readable (0644) token file Addresses the coverage gap noted in the #1865 review. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh * fix(screen-capture): add from __future__ import annotations and fix unclosed file PEP-604 `str | None` return annotation crashed on Python 3.9 (the stock macOS interpreter). Add `from __future__ import annotations` so the annotation is evaluated lazily. Also use a `with` statement for the token file read to avoid a ResourceWarning in test runs under coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh * fix(test): update video-routing test for renamed CAPTURE_TOKEN + /capture token gate CAPTURE_VIDEO_TOKEN was renamed CAPTURE_TOKEN in PR #1957 (the token now covers /capture too, not just /capture-video). Two test adjustments: 1. setUp: set mod.CAPTURE_TOKEN instead of mod.CAPTURE_VIDEO_TOKEN. 2. test_capture_still_returns_png: pass token= so the new /capture auth gate doesn't return 403 before the screenshot logic runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh * fix(util_paths): use module-scope readFileSync instead of require() in readCaptureToken `require` is not defined in ES modules (this repo has "type": "module" + Node16 moduleResolution). The dynamic-require wrapper worked at tsc time but would throw `ReferenceError: require is not defined` at runtime when any inline tool called readCaptureToken(). Fix: add readFileSync to the existing node:fs import at module scope and use it directly. Addresses qingyun-wu's P1 inline comment on PR #1957. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QB3qZQrwponfum2JFNvHeh * fix(screen-capture): fail-closed when CAPTURE_TOKEN is None on /capture do_GET /capture used `if CAPTURE_TOKEN:` which is falsy when the token file fails to load, silently bypassing auth. Mirror the sibling gate on /capture-video (already fail-closed) — drop the `if CAPTURE_TOKEN:` wrapper and lead with `not CAPTURE_TOKEN` so a load failure → 403. Updated test 5 to assert the corrected fail-closed behaviour. Addresses qingyun-wu's re-review (2026-07-08). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RUA5TZgDa7VmFcNtPka38t * fix(screen-capture): send auth token in dropScreenshot() — mirrors dropVideoClip dropVideoClip() has read ~/.config/sutando/screen-capture-token and set X-Sutando-Capture-Token since the token-gate was introduced, but dropScreenshot() still called /capture without the header. After PR #1957 lands the server rejects all headerless /capture requests with 403, breaking the ⌥C hotkey. Apply the identical three-line read-and-set block used by dropVideoClip(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EL2XBcV3RsWhevzTPtW4jw * fix(ci): os._exit after morning-briefing-calendar test to dodge ubuntu teardown segfault Co-Authored-By: Sutando-Maddy-MBP <maddy@noreply.sutando.local> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Susan Xueqing Liu <xliu127@stevens.edu> Co-authored-by: Sutando-Maddy-MBP <maddy@noreply.sutando.local>
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.
Suggests running startup.sh to restart services.