Skip to content

Add unmute hint and YouTube link - #6

Merged
sonichi merged 1 commit into
mainfrom
video-audio-hint
Mar 28, 2026
Merged

sonichi merged 1 commit into
mainfrom
video-audio-hint

Conversation

@sonichi

@sonichi sonichi commented Mar 28, 2026

Copy link
Copy Markdown
Owner

Adds audio hint below inline demo video.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@sonichi
sonichi merged commit d40cd70 into main Mar 28, 2026
@sonichi
sonichi deleted the video-audio-hint branch March 28, 2026 17:16
liususan091219 added a commit that referenced this pull request Mar 29, 2026
- startup.sh: add phone conversation server (port 3100) as service #6
- restart.sh: kill conversation-server and credential-proxy on restart
- agent-api.py: fix dict | None syntax for Python 3.9 compatibility

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
liususan091219 added a commit that referenced this pull request Mar 29, 2026
- startup.sh: add phone conversation server (port 3100) as service #6
- restart.sh: kill conversation-server and credential-proxy on restart
- agent-api.py: fix dict | None syntax for Python 3.9 compatibility

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
liususan091219 pushed a commit that referenced this pull request Apr 18, 2026
Per Susan's 'general edit to correct outdated descriptions' ask. Applied
Maddy's review hits that I could verify locally:

- pkill pattern: `-f "src/Sutando"` → `-x Sutando` (hit #8: safer, matches
  the actual process name regardless of launch path).
- Uninstall step 5: softened from `brew uninstall imsg wacli` to a
  generic 'remove via the package manager you used' (hit #3: wacli isn't
  necessarily brew).
- Capabilities table:
  - Task delegation: `task-bridge.ts` → `task-bridge.ts + watch-tasks.sh + tasks/`
    (hit #4: reflects the actual file-based flow).
  - Notes: 'via CLAUDE.md' → '`notes/` directory (YAML-frontmatter markdown)'
    (hit #5: notes/ is the real store).
  - Added rows for cross-node sync, info-radar, menu-bar avatar states
    (hit #6: recent features missing from the matrix).
- Services table: added 7845 (screen-capture), 3100 (conversation-server),
  4040 (ngrok admin UI) (hit #7).
- Removed stray duplicate `---` separator after the services table
  (hit #9).

Skipped:
- Hit #1 (L77 subscription line) — already removed earlier.
- Hit #2 (screencapture note) — Maddy confirmed still accurate.
- Hit #10 (swiftc missing `-framework AVFoundation`) — main.swift does NOT
  import AVFoundation (verified: import Cocoa, Carbon, UserNotifications
  only). Current compile line is correct.
- L101 permission rewrite (Maddy's 4-bullet proposal) — substantive
  rewrite, deferred to Susan's specific approval before applying.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219 added a commit that referenced this pull request Apr 19, 2026
* docs(README): update 'Summon' voice trigger example to 'Summon my computer to zoom'

The short form 'Summon' is false-positive-prone in voice transcripts. Owner
uses the longer 'Summon my computer to zoom' phrase in practice. This docs
update matches what actually triggers the Zoom summon reliably.

Code-side prompts are NOT changed in this PR (per CLAUDE.md's 'prompts tuned
through testing' rule) — the underlying summonTool already matches both
phrases via the 'summon' keyword.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(README): add two troubleshooting entries (Claude context drift + phone-server error)

Per Susan's Discord msg: two common failure modes worth surfacing in
Troubleshooting:
1. Claude hallucinates more as context fills → tell users to restart the
   Claude Code session periodically.
2. Phone call answers 'We are sorry, an error has occurred' → the
   conversation-server on port 3100 isn't running; relaunch via
   startup.sh or restart.sh.

Verified the correct server path: skills/phone-conversation/scripts/
conversation-server.ts (not a standalone start-phone-server.sh, which
doesn't exist).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(README): apply Maddy's verified hits — general cleanup

Per Susan's 'general edit to correct outdated descriptions' ask. Applied
Maddy's review hits that I could verify locally:

- pkill pattern: `-f "src/Sutando"` → `-x Sutando` (hit #8: safer, matches
  the actual process name regardless of launch path).
- Uninstall step 5: softened from `brew uninstall imsg wacli` to a
  generic 'remove via the package manager you used' (hit #3: wacli isn't
  necessarily brew).
- Capabilities table:
  - Task delegation: `task-bridge.ts` → `task-bridge.ts + watch-tasks.sh + tasks/`
    (hit #4: reflects the actual file-based flow).
  - Notes: 'via CLAUDE.md' → '`notes/` directory (YAML-frontmatter markdown)'
    (hit #5: notes/ is the real store).
  - Added rows for cross-node sync, info-radar, menu-bar avatar states
    (hit #6: recent features missing from the matrix).
- Services table: added 7845 (screen-capture), 3100 (conversation-server),
  4040 (ngrok admin UI) (hit #7).
- Removed stray duplicate `---` separator after the services table
  (hit #9).

Skipped:
- Hit #1 (L77 subscription line) — already removed earlier.
- Hit #2 (screencapture note) — Maddy confirmed still accurate.
- Hit #10 (swiftc missing `-framework AVFoundation`) — main.swift does NOT
  import AVFoundation (verified: import Cocoa, Carbon, UserNotifications
  only). Current compile line is correct.
- L101 permission rewrite (Maddy's 4-bullet proposal) — substantive
  rewrite, deferred to Susan's specific approval before applying.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(README): revise 'How it works' diagram — phone agent + accurate process topology

Per Susan's ask ('diagram isn't quite right', 'phone agent rarely covered')
and Maddy's accuracy verification:

Diagram changes:
- Web client → Voice agent is now WebSocket on :9900 (was implicit file bridge)
- Phone (Twilio) → Conversation server added as peer entry path
- results/ flow annotated as per-channel (speak via voice/phone vs text
  via Telegram/Discord), not one unified speak path

Process topology changes:
- "Two processes work together" → "Three processes" (voice-agent,
  conversation-server, core-agent) — verified against
  conversation-server.ts by Maddy (writes to tasks/ directly at lines
  289, 964, 1315; not hierarchically downstream of voice-agent).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(README): surface phone-agent in prereqs + services list

Per Susan's ask ('phone agent is rarely covered in the README, it should be
mentioned when appropriate'):
- Prereqs: added optional Twilio + ngrok bullet so readers know the phone
  path exists and what they need to enable it.
- 'This starts all services' sentence: added 'phone conversation server' to
  the list so readers see it's a default-started daemon, not an opt-in.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(README): rewrite permission section (4-bullet 'why elevated perms') per Maddy's proposal

Replaces the conservative CYA note ('you should review what it does. All
actions are logged') with Maddy's framing that tells the reader:
1. It's local (no remote control plane)
2. You control the audience (3-tier access gating + VERIFIED_CALLERS)
3. Actions are auditable (build_log + tasks/ + results/ + logs/)
4. Hooks are your brake pedal (git-rules-guard.sh Discord 👍/👎)

Susan's direction was 'lead the work and Maddy please review iteratively'.
Applying Maddy's already-drafted proposal; she reviews next.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(README): add inline-tools path to 'How it works' diagram

Per Susan's ask — voice agent and conversation server have two
response paths (inline tools for instant in-process calls;
tasks/ for anything the core agent needs to execute), while
Telegram/Discord bridges only use tasks/.

— Lucy (Mac Studio bot)

* docs(README): swap 'mute' for real inline tool per Maddy's review

mute is a menu-bar ⌃M shortcut, not an inline tool. Replace with
get_current_time (src/inline-tools.ts:376) and broaden the prose
sentence to list only genuine inline tools.

— Lucy (Mac Studio bot)

* docs(README): add running-costs section + price-aware prereqs

Per Susan's in-#dev review (consolidated 4-edit bundle, 👍'd):
- Line 96: drop unqualified "free" on API key — usage-billed per
  Running costs
- Line 97: add ~$0.01–0.03/min Twilio cost reference
- New "## Running costs" section: Gemini Live $0.30–$1.30/hr,
  Twilio $0.0125/min (US local), free for browser/Telegram/Discord
- New ffmpeg bullet in prereqs (optional, for video/audio)

Sources: Gemini pricing docs, Twilio voice pricing docs.

— Lucy (Mac Studio bot)

* docs(README): bundle 4 follow-ups from #susan iteration

Per Susan's 4× 👍 iterative review on PR #450:
- Diagram: add `↻` on Core agent + legend explaining the cron-driven
  proactive loop (5-min fires for tasks, health, build-log work).
- Running costs table: Twilio row now bidirectional — ~$0.013–$0.018/min
  covers inbound ($0.0085) + outbound ($0.014) + Media Streams ($0.004).
- Prereq bullet: rename "Google AI Studio API key" → "Gemini API key"
  and inline the ~$0.30–$1.30/hour voice cost.
- macOS permissions: moved from inline Quick start bullets to the
  Security section with a per-permission "why it's needed" expansion;
  Quick start keeps a one-line pointer.

— Lucy (Mac Studio bot)

* docs(README): hedge Gemini price claims — free tier vs paid unclear

Susan and Chi are still sorting out the free-tier vs paid-tier
picture; pull back the specific $0.30-$1.30/hr claim since it
misrepresents the free-tier case:

- Prereq bullet: "may be free on the free tier or usage-billed
  depending on your Google Cloud account" instead of a rate
- Running costs table Gemini row: "Free tier available - paid
  usage varies"; notes keep the published paid rates as reference
  but point readers at their billing dashboard for ground truth
- Drop the personal $13/602-min datapoint from the typical-call
  example (may conflate with non-voice Gemini spend)

Twilio row unchanged - that math is from published rates.

— Lucy (Mac Studio bot)

* docs(README): Option B — remove Running costs section + price mentions

Per Susan's "B" direction in #dev: the free-tier vs paid-tier
picture for Gemini isn't settled, and leaving any price language
in the PR risks misleading first-time readers.

- Delete the entire "## Running costs" section (table + example +
  sources line).
- Revert the Gemini API key prereq bullet to `(click "Get API key")`
  — drop the usage-billed caveat entirely.
- Revert the Twilio prereq bullet to drop the `~$0.01–0.03/min`
  price reference.

ffmpeg bullet, diagram rewrite, macOS permissions in Security
section, `↻` cron legend, phone-agent surfacing — all stay.

— Lucy (Mac Studio bot)

---------

Co-authored-by: Xueqing Liu <xueqingliu@Xueqings-Mac-Studio.local>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sonichi pushed a commit that referenced this pull request May 4, 2026
…archive watch

Changes per Chi's review of v1 (PR #586 thread):

1. Pointer schema. Drop inline derivable fields (user_id, channel_id,
   access_tier, channel_source, size_bytes). Each event now carries only
   {task_id, path}; summarizer reads the file when it needs body. Cuts
   event size 3-4x.

2. Cursor-based restart safety. Track last-seen mtime per directory in
   state/learning-<instance>.cursor. On first run, bootstrap to NOW so
   history doesn't replay; on subsequent runs, pick up from cursor — no
   missed events across restarts.

3. Heartbeat. Emit collector.heartbeat every 60s (configurable via
   LEARN_COLLECTOR_HEARTBEAT_S). Lets the summarizer detect daemon
   death without inspecting file mtime.

4. Switch results watch to results/archive/<YYYY-MM>/ (recursive). The
   transient results/ dir gets emptied by the bridge in <5s, so a 5s
   poll missed most produced events. Watching the durable archive
   instead catches every result reliably.

5. Drop Set-based bootstrap-skip; use mtime > cursor instead. Simpler
   and integrates with cursor.

Smoke-tested: cursor reload across restart works; new tasks in tasks/
emit incoming + archived events; heartbeat fires on schedule.

Addresses self-audit issues #1, #5, #6 (count imbalance was downstream
of #1 and #6 — same root causes).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rickchen007 referenced this pull request in AG2Platform/stando May 15, 2026
Untrack .onboarding-step, .swiftpm/ workspace + xcuserstate so a fresh
clone doesn't inherit a stale wizard step or someone else's Xcode UI
state. Add .onboarding-complete + .firstrun-complete to .gitignore for
the same reason.

Sync DISTRIBUTION.md + PRODUCT.md to current behavior after commit
22e6cd7 (onboarding wizard + screen-capture supervisor). New Phase
4.13 section covers the wizard, the in-process screen-capture
supervisor, layered Screen Recording detection, and restartSelf() via
'open -n'. Superseded sections (Phase 4.7 stepper, Phase 4.9 bug #6,
Phase 5 item #6) tombstoned rather than rewritten.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
* 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>
qingyun-wu added a commit that referenced this pull request Jun 3, 2026
…ando() exists (closes #1418)

Surfaced by @Sutando-Mini-bot's cold-review on PR #1415 (review item #6),
filed as issue #1418. The script previously detected legacy ALIASES via
`legacy_alias_present()` but didn't detect a user-defined `claude-sutando()`
FUNCTION outside our managed marker block. --commit silently appended the
managed block alongside the user's function — bash's last-definition-wins
made their function dead code with no warning.

## What changes

1. **New helper `user_defined_function_present()`.** Awk strips the managed
   marker block from the rc file, then greps the remainder for
   `^claude-sutando[[:space:]]*\(\)`. Catches the common
   `claude-sutando()` / `claude-sutando ()` shapes. Doesn't catch
   `function claude-sutando { ... }` (less common; add if it bites).

2. **`--force` modifier flag.** Parsed separately from MODE flags (doesn't
   consume a `break;` so it can combine with --commit etc.). Default 0.

3. **Refuse logic in `commit | auto` case-arm.** Before any rc write, if
   `user_defined_function_present` returns true and FORCE != 1, exit 1
   with a clear stderr explanation listing 3 options:
     - Remove the existing user function + re-run.
     - Pass --force to commit anyway (function will be shadowed).
     - Leave as-is.

4. **Header docs updated** to mention --repair-paths (was missed in PR
   #1415) and the new --force modifier.

## Verification

- `bash -n scripts/sutando-shell-setup.sh` passes.
- Smoke test: sourced helper against a temp rc containing
  `claude-sutando() { echo my version }` → helper correctly returns true.

## Out of scope

- Doesn't catch `function claude-sutando { ... }` syntax. Could add a second
  regex alternation. Holding for follow-up unless someone hits it.
- Doesn't detect a user-defined ALIAS (`alias claude-sutando=...`) — those
  are caught by the existing `legacy_alias_present()` which we already
  handle in state-3 ("Legacy alias present → remove + append fresh block").
  Same end-result; might be worth refusing-without-force on those too. Out
  of scope here.

Closes #1418.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
qingyun-wu added a commit that referenced this pull request Jun 3, 2026
) (#1424)

* fix(sutando-migrate): --merge-append compound-redirect breaks set -e (data-loss class, closes #1422)

Surfaced by @Sutando-Pro's cold-review on PR #1415 (review item #5), filed as
issue #1422. Same bug class as the `feedback_bash_and_compound_breaks_set_e`
memory (originally caught by Mini on PR #1406 2026-06-02) — but in a sibling
code site that the original fix missed.

## The bug

Lines 1075-1082 (pre-patch):
```bash
{
    cat "$dst_path"
    echo ""
    echo "=== migrated from source $tag at $BACKUP_ID ==="
    echo ""
    cat "$src_file"
} > "$dst_path.merge.$$" && mv -f "$dst_path.merge.$$" "$dst_path"
echo "merged"
```

If the `{ ... } > "$dst_path.merge.$$"` write fails (disk full, permission
denied, broken pipe, quota exceeded), the `&&` short-circuits — but because
the compound is left-of-&&, `set -e` does NOT exit. Execution continues to
`echo "merged"` while the actual data write failed AND dst_path is unchanged.

Net: caller sees "merged" success signal in the log, downstream code trusts
the dest is updated, dest is actually the old content. Silent data loss.

## The fix

Split into separate statements with explicit error returns. Mirrors the
pattern the append-fresh blocks above (L1027-1066) already use (PR #1406 fix
that missed this site):

- Capture redirect exit status into `_merge_rc` via `|| _merge_rc=$?`.
- If non-zero: `rm -f` the tmp + echo error + `return 1`.
- Otherwise: `mv -f` with explicit error handler.
- Only on success: `echo "merged"`.

`echo "merged"` is now gated on the actual write + rename succeeding.

## Verification

`bash -n scripts/sutando-migrate.sh` passes. No behavior change on the happy
path (write + mv succeed → still echoes "merged"). On failure path: caller
gets `return 1` + a specific stderr signal (`merge-append-failed-redirect`
vs `merge-append-failed-mv`) instead of a false success.

Same code shape as the existing `append-failed-redirect` / `append-failed-mv`
signals from the fresh-append blocks — semantically consistent.

Closes #1422.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(skills): replace pre-M0 \$SUTANDO_WORKSPACE snippets in operational prompts (closes #1421)

Surfaced by @Sutando-Pro's cold-review on PR #1415 (review item #3), filed
as issue #1421. Three skill SKILL.md files still carried
`${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}` snippets in their
operational prompts — these aren't docs, they're prompts agents execute, so
stale examples become real writes to the legacy path that no other reader
looks at.

Touchpoints:

- `skills/startup/SKILL.md:48-49` — step 1b bash block (mkdir + touch the
  sentinel). 2 occurrences in one block.
- `skills/schedule-crons/SKILL.md:22` — step 0 prose ("Check ... under
  `${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}/`"). 1 occurrence inline.
- `skills/task-orphan-check/SKILL.md:30` — step 1 bash block (list live
  tasks). 1 occurrence.

All three updated to use the M0 helper form:
```bash
WORKSPACE="$(bash scripts/sutando-config.sh workspace)"
```
…then `"$WORKSPACE/..."` for subsequent paths. Matches the canonical pattern
in `/proactive-loop` SKILL.md (which already uses the helper form).

Net: agents that execute these prompts read/write to the M0 canonical
location (`<repo>/workspace/` by default per PR #1395) instead of the legacy
`~/.sutando/workspace/` that the pre-M0 fallback resolved to.

Verified: `grep 'SUTANDO_WORKSPACE.*:-.*\.sutando/workspace'` on the three
files returns 0 matches post-edit.

Closes #1421.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(claude.md): chat-task-tracking examples use M0 helper for path resolution (closes #1419)

Surfaced by @Sutando-Pro's cold-review on PR #1415 (review item #1), filed as
issue #1419. The §Chat-path task tracking section's bash snippets used bare
`tasks/...` / `results/...` paths that resolve against the agent's cwd (the
repo root, since session cwd = repo) instead of the M0 canonical workspace.
Agents copy-pasting the snippet wrote tasks where readers don't look.

Fix:

- Prepend `WORKSPACE="$(bash scripts/sutando-config.sh workspace)"` to the
  "How:" block.
- Use `"$WORKSPACE/tasks/task-chat-${_ts}.txt"` instead of bare
  `"tasks/task-chat-${_ts}.txt"`.
- "When done:" block uses the same `$WORKSPACE` (reusing the resolved value
  from above — no re-spawn of the helper subprocess per the §Work Status
  block's "resolve once per pass" guidance).
- Same `"$WORKSPACE/results/..."` form for the result-file write.

Doc-only — no code change. The bridge / consumer code already resolves
through the helper (PR #1395 / #1399). This just aligns the example with
what the actual writers do.

Closes #1419.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(workspace-contract): strengthen pre-M0 banner + update worst stale examples (closes #1420)

Surfaced by @Sutando-Pro's cold-review on PR #1415 (review item #2), filed as
issue #1420. `docs/workspace-contract.md` still taught the pre-M0
`${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}` pattern in several examples
despite the top-of-doc note. Readers walked away thinking the old patterns
were canonical.

Scope of this fix (focused, not full rewrite):

1. **Strengthen the top banner.** The prior "Note: see workspace-config.md"
   was easy to skip. Replaced with a ⚠️ "Stale examples — read with care"
   banner that lists the CURRENT 3 resolution recipes (shell / Python / TS)
   inline so anyone copy-pasting from this file gets the right thing in the
   first paragraph.

2. **Update the §What goes where table.** WORKSPACE_DIR row now leads with
   post-M0 ("`<repo>/workspace/` default, resolved via M0 helper") instead
   of the pre-M0 fallback chain. Pre-M0 default mentioned parenthetically
   for historical context.

3. **Replace the worst stale bash example.** L68's
   `TASKS_DIR="${SUTANDO_WORKSPACE:-$HOME/.sutando/workspace}/tasks"` was
   exactly the copy-paste-poison pattern Pro flagged. Now uses
   `TASKS_DIR="$(bash scripts/sutando-config.sh workspace)/tasks"` with an
   inline comment that the helper handles the legacy escape hatch
   internally.

Out of scope for THIS commit (left as historical text — file is now clearly
labeled as such by the banner):
- §Existing-repo installs migration guidance still references pre-M0
  direction (legacy → in-repo MIGRATE is the new direction; old text
  describes the opposite). This is misleading for new readers but updating
  the section properly requires rewriting most of the file. Banner already
  warns; full rewrite can be follow-up.
- workspace-design.md + workspace-config.md untouched — they already use
  M0-correct framing (verified via grep). The 6 + 7 mentions in those files
  are legitimate legacy-context references, not stale examples.

Closes #1420.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(sutando-shell-setup): refuse --commit if user-defined claude-sutando() exists (closes #1418)

Surfaced by @Sutando-Mini-bot's cold-review on PR #1415 (review item #6),
filed as issue #1418. The script previously detected legacy ALIASES via
`legacy_alias_present()` but didn't detect a user-defined `claude-sutando()`
FUNCTION outside our managed marker block. --commit silently appended the
managed block alongside the user's function — bash's last-definition-wins
made their function dead code with no warning.

## What changes

1. **New helper `user_defined_function_present()`.** Awk strips the managed
   marker block from the rc file, then greps the remainder for
   `^claude-sutando[[:space:]]*\(\)`. Catches the common
   `claude-sutando()` / `claude-sutando ()` shapes. Doesn't catch
   `function claude-sutando { ... }` (less common; add if it bites).

2. **`--force` modifier flag.** Parsed separately from MODE flags (doesn't
   consume a `break;` so it can combine with --commit etc.). Default 0.

3. **Refuse logic in `commit | auto` case-arm.** Before any rc write, if
   `user_defined_function_present` returns true and FORCE != 1, exit 1
   with a clear stderr explanation listing 3 options:
     - Remove the existing user function + re-run.
     - Pass --force to commit anyway (function will be shadowed).
     - Leave as-is.

4. **Header docs updated** to mention --repair-paths (was missed in PR
   #1415) and the new --force modifier.

## Verification

- `bash -n scripts/sutando-shell-setup.sh` passes.
- Smoke test: sourced helper against a temp rc containing
  `claude-sutando() { echo my version }` → helper correctly returns true.

## Out of scope

- Doesn't catch `function claude-sutando { ... }` syntax. Could add a second
  regex alternation. Holding for follow-up unless someone hits it.
- Doesn't detect a user-defined ALIAS (`alias claude-sutando=...`) — those
  are caught by the existing `legacy_alias_present()` which we already
  handle in state-3 ("Legacy alias present → remove + append fresh block").
  Same end-result; might be worth refusing-without-force on those too. Out
  of scope here.

Closes #1418.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(sutando-shell-setup): expand _rewrite_runtime_paths() allowlist with globbed patterns (closes #1416)

Surfaced by @Sutando-Mini-bot's cold-review on PR #1415 (review item #2),
filed as issue #1416. The original allowlist was 4 explicit files:
`settings.json`, both `plugins/*.json` manifests, and
`commands/openacp:handoff.md`. New user-installed slash-commands and
third-party skills with hardcoded `~/.claude` paths could slip through
`--migrate` + `--repair-paths` silently.

## What changes

Two-layer allowlist:

**(a) Explicit-file list.** Files known by name at runtime:
- `settings.json`
- `plugins/installed_plugins.json`
- `plugins/known_marketplaces.json`

**(b) Globbed patterns.** Enumerated to absolute paths via find:
- `commands/*.md` — every slash-command body (was 1 explicit file, now glob
  to catch any future commands the user installs).
- `skills/**/SKILL.md` — skill prompts (often hardcode paths to scripts).
- `skills/**/*.{sh,py,ts,js}` — skill scripts (the actual executables).

`plugins/*/cache/` intentionally skipped — vendored plugin source already
covered indirectly by rewriting `installed_plugins.json` + `known_marketplaces.json`.

## Properties

- **Still allowlist-shaped** (not full recursive sed). The contract is
  grep'able + the immutable surfaces (`projects/`, `history.jsonl`,
  `*.before-migrate.*`, `shell-snapshots/`, `file-history/`) are
  un-touched.
- **No-op on candidates that don't contain source_dir.** Each file is
  filtered through `grep -q "${source_dir}/"` before sed fires.
- **Idempotent** unchanged from before — rewriting destpath → destpath is
  a no-op.
- **BSD-sed compatible** unchanged.
- **Summary output now includes candidate count** so operator sees the
  scope of the iteration.

## Design call (vs blocklist)

This PR keeps the allowlist shape per the explicit "allowlist vs blocklist"
tradeoff documented in PR #1415's body. Mini's review opened the case for
revisiting; my call after thinking it through is: stay allowlist + expand
patterns. Blocklist would force enumerating "things never to touch" which
drifts faster than the inclusion list (3 immutable-surface patterns vs N
inclusion-patterns growing with the ecosystem).

If reviewers lean blocklist, easy swap — replace the find-based enumeration
with a recursive find + an exclude list. Tradeoff documented in the
helper's docstring.

## Verification

`bash -n` passes. Live dry-run via `--repair-paths` on this checkout shows
the expanded candidate count + still correctly identifies the 4 unchanged
files from the prior pass (idempotent).

Closes #1416.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(sutando-shell-setup): --repair-paths --from=PATH for workspace-move scenarios (closes #1417)

Surfaced by @Sutando-Mini-bot's cold-review on PR #1415 (review item #3),
filed as issue #1417. `--repair-paths` only rewrote `$SOURCE_CLAUDE_CONFIG_DIR/`
(default `~/.claude/`) → `$CLAUDE_DIR/`. So after a workspace move:

1. User runs `claude-sutando --migrate` → `<workspace-A>/.claude-sutando/`
   set up. Runtime files re-pinned A.
2. User moves workspace: `mv <workspace-A> <workspace-B>`.
3. User runs `claude-sutando --repair-paths` expecting paths re-pinned.
4. Reality: no `~/.claude/` matches in runtime files (they're now
   `<workspace-A>` paths) → "unchanged" reported while paths point at
   stale A.

## What changes

New `--from=PATH` modifier. When set, `_rewrite_runtime_paths()` runs TWO
sed passes per file:
- Existing: `source_dir/` → `CLAUDE_DIR/`
- New: `${FROM_DIR%/}/` → `CLAUDE_DIR/`

A file matching EITHER pattern gets rewritten + counted as "rewrote".
Summary unchanged in shape; just shows both source-of-truth lines when
`--from` is set ("source : .../"  "move-from : .../  (also re-pinned)").

## Form

Uses `=`-joined form (`--from=/path/to/old/.claude-sutando`), NOT
space-separated. Keeps the existing single-pass for-loop arg parser
unchanged (which uses `case ... break` per-arg). Documented in the script
header. Space-separated form is a follow-up if `=`-joined UX bites people.

## Properties

- Idempotent: re-running with the same --from is a no-op since destpath
  → destpath rewrite has no effect.
- Default behavior unchanged: without `--from`, single-pass rewrite as
  before.
- BSD-sed compatible unchanged.

## Verification

`bash -n` passes. Smoke test: invoked `--repair-paths --from=/nonexistent/path`
→ correctly runs no-op on all 12 candidates (the source rewrite finds no
matches; the from rewrite also finds no matches since the path doesn't
appear anywhere).

## Out of scope

- **Tombstone auto-detect**: `.claude-sutando/.previous-locations` file
  that --migrate updates on each rehome, --repair-paths reads. Would cover
  the "user moved workspace twice" case without explicit `--from`. Issue
  #1417 documented (B) as the heavier alternative; this PR ships (A) only.

Closes #1417.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(sutando-shell-setup): two-pass argparse so --from=PATH parses after --repair-paths (follow-up to #1417)

The previous --from=PATH support relied on a single-pass for-loop arg
parser where MODE-setting flags `break;` out of the loop. So
`--repair-paths --from=/x` was eating --from on the loop break — FROM_DIR
stayed empty, smoke test caught it immediately.

Fix: split argparse into two passes.

- Pass 1: scan ALL args for modifiers (`--force`, `--from=PATH`). No break.
- Pass 2: find the FIRST MODE-setting flag, then break. Original "first
  mode wins" semantics preserved.

Modifiers silently consumed by pass 1's case fall through pass 2 via an
explicit `--force|--from=*) ;;` no-op arm (otherwise the unknown-arg
catch-all would exit 1 on them).

Verified with smoke tests:
- `--repair-paths --from=PATH` → FROM_DIR set, "move-from:" line in output.
- `--from=PATH --repair-paths` → same, modifier-first order also works.
- `--repair-paths` (no modifier) → unchanged behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(sutando-migrate): per-command RC capture inside merge-append brace (Mini PR #1424 review #1)

Surfaced by @Sutando-Mini-bot's deep review on PR #1424 (item #1). My
prior #1422 fix split `{ ... } > tmp && mv && echo` into separate
statements + captured the brace exit via `|| _merge_rc=$?` — but a brace
group's exit code equals its LAST command. Earlier failures inside the
brace get swallowed if the last `cat "$src_file"` returns 0.

Concrete failure scenario the prior fix missed:
- `cat "$dst_path"` fails (file disappears between [ -e $dst_path ] and
  read, or permission flip mid-merge).
- The header echos succeed (write to the already-open tmp fd).
- `cat "$src_file"` succeeds.
- Brace group exits 0.
- `|| _merge_rc=$?` doesn't fire → `_merge_rc` stays 0.
- `mv` runs + commits a PARTIAL merge (dst content silently missing).
- `echo "merged"` prints — invisible data loss.

## Fix

Per-command RC capture inside the brace via a bitmask. Each step `||`s
into a unique bit position so the stderr signal points at WHICH step
failed:

```
1=cat-dst  2=hdr-blank  4=hdr-line  8=trailer-blank  16=cat-src
```

stderr message becomes `merge-append-failed-inner <mask>` instead of the
prior `merge-append-failed-redirect` (which implied a whole-redirect
failure — misleading when only one inner cat fails).

## Verification

Smoke test: invoked the same brace shape against `/nonexistent/file` (cat
fail at step 1) → bitmask correctly returns 1 (only step 1 bit set).
The OTHER 4 steps succeed because once a brace group has its stdout
redirected to an open fd, individual writes only fail on quota/disk
boundaries, not on dst_path's disappearance.

Per Mini's "same template as the fix I posted on PR #1406" guidance —
this template is also worth back-applying to the append-fresh blocks at
L1027-1066 if they have the same gap. Quick audit: those blocks use
`|| _redirect_rc=$?` on the whole brace too. Same class of bug. SHOULD
land in a sibling commit; doing now to keep the PR coherent.

Closes Mini PR #1424 review #1.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(sutando-migrate): per-command RC capture for append-fresh + append blocks (per-file pattern audit, sibling to f74a110)

The previous commit fixed --merge-append with per-command RC capture inside
the brace group. Per `feedback_per_file_pattern_audit`: same bug class
exists in the two sibling blocks above (L1027-1066), originally fixed by
PR #1406 with the brace-overall capture pattern — but that's the same
"only catches last-command failure" gap. Mini's review caught the
--merge-append site; auditing the file caught these two siblings.

Applying the same per-command bitmask template:

- Append-with-divider block (when dst exists): 5-bit mask covering
  cat-dst, hdr-blank, hdr-line, trailer-blank, cat-src.
- Append-fresh block (when dst missing): 3-bit mask (4=hdr-line,
  8=trailer-blank, 16=cat-src — no cat-dst). Kept the same bit values
  as the 5-bit mask for grep-ability; bits 1 and 2 unused.

stderr signals:
- Existing `append-failed-redirect` → `append-failed-inner <mask>`.
- New `append-fresh-failed-inner <mask>` (was `append-failed-redirect`
  too — disambiguated to make per-class triage easier).

No behavior change on the happy path. Brace-internal failures now surface
with a specific bit pattern instead of being silently swallowed.

Per `feedback_per_file_pattern_audit`: when fixing a class of bugs in a
file, grep that file for ALL members of the class. Two more sibling sites
audited and fixed in this commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(sutando-shell-setup): validate --from= before sed (Mini PR #1424 review #2)

Surfaced by @Sutando-Mini-bot's deep review on PR #1424 (item #2). The
prior --from=PATH implementation built `s|${FROM_DIR%/}/|${CLAUDE_DIR}/|g`
unconditionally. Pathological inputs would catastrophically corrupt every
candidate file:

- `--from=/`        → `s|/|<CLAUDE_DIR>/|g` rewrites every `/` in
  settings.json, plugin manifests, slash-commands, skill SKILL.md +
  scripts. File-corruption-on-typo.
- `--from=`         (empty after `=`) — same as `/` after `%/` strips.
- `--from=/etc`     — silently rewrites unrelated paths if `/etc/` appears
  in any candidate.
- `--from=/typo/.claude-sutando-prev`  (doesn't exist) — silent no-op
  rather than explicit "you typo'd."

## Validation

Reject BEFORE building the sed script:

1. **Empty / root.** `[ -z "${FROM_DIR%/}" ]` catches both.
2. **Path shape.** Must end in `/.claude`, `/.claude-sutando`, or
   `/.claude.<suffix>` (catches the M0 canonical + the M0+M1 canonical +
   any future variant). Anything else rejected with explanation of the
   typo-guard rationale.
3. **Existence.** Directory must exist on disk. A typo'd `--from=...`
   that points nowhere is a likely typo, not a no-op situation.

Each rejection prints a clear `--from=<value> rejected — <reason>` line
to stderr + exits 1.

## Verification

Smoke tests:
- `--from=/`        → rejects with "cannot be empty or root" ✓
- `--from=/etc`     → rejects with shape mismatch + typo-guard
  explanation ✓
- `--from=/nonexistent/.claude-sutando` → rejects with "directory does
  not exist" ✓
- `--from=$HOME/.claude` → accepts (existing real shape) ✓

## Out of scope

- `--from=$HOME/.claude` (the SOURCE_CLAUDE_CONFIG_DIR default itself)
  is accepted but redundant — the existing source-rewrite already handles
  that. Not flagged as redundant; harmless.
- No path-canonicalization (symlinks not resolved). If user passes a
  symlink, sed matches the symlink path, not the resolved target.
  Probably fine; symlinks pointing at active .claude dirs are unusual.

Closes Mini PR #1424 review #2.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(sutando-shell-setup): expand user-function detection + add --force overwrite warning (Mini PR #1424 review #3)

Surfaced by @Sutando-Mini-bot's deep review on PR #1424 (item #3). Two
parts:

## Part 1: regex expansion

The original `user_defined_function_present()` regex
`'^claude-sutando[[:space:]]*\(\)'` only caught the column-0 POSIX
function shape. Misses:

- `function claude-sutando { ... }`         (keyword form)
- `function claude-sutando () { ... }`      (keyword + parens)
- `  claude-sutando()` / `  function claude-sutando` (leading whitespace,
  common in rc-files with conditional `if [ -n "$ZSH_VERSION" ]; then`
  blocks)
- Bash also accepts `claude-sutando ()` with a space before parens.

Expanded grep alternation now covers all of these via a single regex:
`'^[[:space:]]*(claude-sutando[[:space:]]*\(\)|function[[:space:]]+claude-sutando([[:space:]]|\(|\{|$))'`

The trailing `([[:space:]]|\(|\{|$)` is the word-boundary equivalent for
the keyword form — prevents false-positive on names like
`function claude-sutando-other`.

Smoke-tested with 10 fixtures (7 positive, 3 negative) — all classify
correctly:

| Fixture | Expected | Got |
|---|---|---|
| `claude-sutando() { … }`            | DETECTED | ✓ |
| `  claude-sutando() { … }`          | DETECTED | ✓ |
| `claude-sutando () { … }`           | DETECTED | ✓ |
| `function claude-sutando { … }`     | DETECTED | ✓ |
| `function claude-sutando\n{...`     | DETECTED | ✓ |
| `function claude-sutando () { … }`  | DETECTED | ✓ |
| `  function claude-sutando { … }`   | DETECTED | ✓ |
| `alias claude-sutando=…`            | miss     | ✓ (handled by legacy_alias_present) |
| `alias foo=bar`                     | miss     | ✓ |
| `claude-other() { … }`              | miss     | ✓ (no false-positive on similar names) |

## Part 2: --force overwrite warning

Previously `--force` silently proceeded with the managed-block append,
hiding the consequence (user's function becomes dead code).

Now: when `--force` is passed AND a user-defined function is detected,
print an explicit stderr warning:

> ⚠ --force — OVERWRITING user-defined claude-sutando function in
>   $RC_FILE. Your function will be DEAD CODE after this commit...
>   If this wasn't intended: Ctrl+C now, remove --force, and read the
>   refusal message for the 3 options. Continuing in 2s if no interrupt...

The 2-second grace window gives the user time to Ctrl+C if they realize
they shouldn't have used --force. `|| true` on the sleep so we don't
fail if the user pipes `< /dev/null` or runs from a non-interactive
context where sleep might behave oddly.

`sleep 2 2>/dev/null || true` — defensive against any sleep-not-available
weirdness; the warning's still on screen even without the grace window.

Closes Mini PR #1424 review #3.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(sutando-shell-setup): inline argparse + --from validation + user-function detect regression tests (Mini PR #1424 test-gap)

Surfaced by @Sutando-Mini-bot's deep review on PR #1424: "Parser two-pass
at L54-73 fixes modifier-after-mode ordering — but no smoke test landed
in this diff; regression-risk if argparse touched again."

This commit closes the test gap with a single `bash`-runnable test file
at `tests/sutando-shell-setup-argparse.test.sh`. Pattern matches the
existing test conventions (`tests/sutando-migrate.test.sh`, etc).

## Coverage (18 assertions)

**Argparse two-pass (3 assertions):**
- modifier-after-mode: `--repair-paths --from=PATH` parses --from ✓
- modifier-before-mode: `--from=PATH --repair-paths` parses --from ✓
- no-modifier regression: bare `--repair-paths` runs cleanly ✓

**--from= validation (4 assertions):**
- `--from=/` rejected with empty/root error ✓
- `--from=/etc` rejected with shape-mismatch error ✓
- `--from=/nonexistent/.claude-sutando` rejected with does-not-exist ✓
- `--from=/nonexistent/.claude` rejected with does-not-exist (different
  shape, same outcome path) ✓

**user_defined_function_present regex (11 assertions):**
- 7 positive fixtures: column-0 POSIX, leading ws, space-before-parens,
  function-keyword (+ brace), function-keyword (+ parens), leading-ws +
  function-kw, function-kw bare (brace on next line).
- 4 negative fixtures: alias-only, no-claude-sutando, similar-but-diff
  name, function-kw with diff name.

## Discovered + fixed during test development

`grep -qF` interprets needles starting with `--` (like `--from=/x`) as
flag options. macOS BSD grep needs `--` terminator before the pattern:
`grep -qF -- "$needle"`. Documented in the helper.

## How to run

```sh
bash tests/sutando-shell-setup-argparse.test.sh
```

Exit code: 0 on all-pass, 1 on any-fail. Each assertion prints
✓/✗ + summary line at end. Offline; no /tmp dir setup required (each
fixture writes its own).

Closes Mini PR #1424 test-gap callout.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(sutando-shell-setup): add brace-internal RC capture tests to argparse suite (Mini PR #1424 review #1 test-gap)

Extends `tests/sutando-shell-setup-argparse.test.sh` (the new file from
a6ee5bd) with 4 brace-pattern assertions. Mini's review #1 callout said
"no tmpfs/full repro test in the diff" — full move_file_local() repro
is hard to isolate, so this commit ships PATTERN-level regression tests
that exercise the brace-internal bitmask shape directly against
synthetic missing-file fixtures.

## New assertions (4)

- **Happy path:** all commands in the brace succeed → mask=0. ✓
- **Early-cat fail:** cat-dst missing, later-cat-src OK → mask=1. ✓
  This is the SPECIFIC gap Mini identified — brace-overall exit would
  be 0 (last command succeeds) and the partial-merge would silently
  commit.
- **Both cats fail:** mask=17 (1|16). Confirms the bitmask OR's
  multiple failures correctly without overwriting. ✓
- **Old-pattern proof:** invoke the brace WITHOUT per-command capture
  + assert brace-overall rc=0 even when cat-dst fails. Codifies the
  previously-broken behavior — a future regression to the old pattern
  would surface immediately because this assertion would flip. ✓

## File naming

Kept the existing `-argparse.test.sh` suffix even though the scope is
now broader than just argparse — there's already a
`tests/sutando-shell-setup.test.sh` (11-assertion suite from earlier
work). Renaming would clash. The `-argparse` file specifically targets
the post-PR-#1415 follow-up surfaces (argparse + --from + user-func +
brace pattern); the existing file covers --commit / --check / --auto /
--migrate at a different layer.

## Total coverage now

22 assertions across 4 sections:
- 3 argparse two-pass
- 4 --from= validation
- 11 user-function regex
- 4 brace-internal RC capture (NEW)

Closes Mini PR #1424 review #1 test-gap callout.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(tests): wrap (d) brace-overall comparison in set +e/-e (test self-test fix, follow-up to 6be675a)

The new brace-overall comparison test (item d in the brace-internal RC
capture section) intentionally invokes a brace WITHOUT per-command
`||` capture to prove the OLD pattern's gap. The brace fails (first
cat misses), and the script's top-level `set -euo pipefail` was
propagating that failure up — script exited 1 before the summary
block could print. Net: test passed silently but didn't show the
summary; CI would mark it failed on exit-code-1.

Fix: `set +e` before the comparison brace + `set -e` after. Standard
pattern for "I'm capturing this command's rc into a variable; tolerate
non-zero temporarily."

Verified: full run now prints all 22 ✓ + summary `22 passed, 0 failed`
+ exit 0.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
qingyun-wu added a commit that referenced this pull request Jun 6, 2026
…1467 thread + Lucy's Maddy report)

Part 3 of the v0.3.0 docs sweep (PR #1472).

Adds two substantive sections to `docs/workspace-sync.md`:

**1. Migration risks + pre-flight checklist** — distilled from:
- sonichi's "Migration" thread on 2026-06-05 (Mini + Pro + Sutando risk analyses)
- Lucy's Maddy-host migration run report 2026-06-06 #design

Covers:
- Pre-migration checks per host (vault URL set, $SUTANDO_WORKSPACE
  removed from shell rc AND .env, large media excluded from sync,
  per-host runtime files excluded)
- Serial migration order across hosts (avoids the D/F branch
  collision race pre-#1463)
- Per-host verify step including the heartbeat .alive cleanliness
  check Mini surfaced
- Known issues with workarounds (Claude memory not auto-moved,
  sync-workspace plain run silent-commits, $SUTANDO_WORKSPACE
  re-migrate loop)

**2. Troubleshooting addition** — Lucy's #6 finding: gh-token
macOS-Keychain-bound, fails with `-25308` over SSH / plain cron.
Documents the launchd-plist-scoped-to-GUI-session workaround.

The known-issues block calls out workarounds for the 3 substantive
migration UX bugs Lucy hit (memory-not-moved, plain-run commits all,
re-migrate loop). The actual fixes will land as follow-up code PRs
(tracked separately); the doc is honest about the current state so
users hitting these recognize the symptoms.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
qingyun-wu added a commit that referenced this pull request Jun 6, 2026
…1467 thread + Lucy's Maddy report)

Part 3 of the v0.3.0 docs sweep (PR #1472).

Adds two substantive sections to `docs/workspace-sync.md`:

**1. Migration risks + pre-flight checklist** — distilled from:
- sonichi's "Migration" thread on 2026-06-05 (Mini + Pro + Sutando risk analyses)
- Lucy's Maddy-host migration run report 2026-06-06 #design

Covers:
- Pre-migration checks per host (vault URL set, $SUTANDO_WORKSPACE
  removed from shell rc AND .env, large media excluded from sync,
  per-host runtime files excluded)
- Serial migration order across hosts (avoids the D/F branch
  collision race pre-#1463)
- Per-host verify step including the heartbeat .alive cleanliness
  check Mini surfaced
- Known issues with workarounds (Claude memory not auto-moved,
  sync-workspace plain run silent-commits, $SUTANDO_WORKSPACE
  re-migrate loop)

**2. Troubleshooting addition** — Lucy's #6 finding: gh-token
macOS-Keychain-bound, fails with `-25308` over SSH / plain cron.
Documents the launchd-plist-scoped-to-GUI-session workaround.

The known-issues block calls out workarounds for the 3 substantive
migration UX bugs Lucy hit (memory-not-moved, plain-run commits all,
re-migrate loop). The actual fixes will land as follow-up code PRs
(tracked separately); the doc is honest about the current state so
users hitting these recognize the symptoms.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant