Skip to content

feat(discord-voice): move share-screen-modal.py to standalone sutando-skills skill - #1073

Closed
liususan091219 wants to merge 2 commits into
sonichi:mainfrom
liususan091219:feat/move-share-screen-to-sutando-skills
Closed

liususan091219 wants to merge 2 commits into
sonichi:mainfrom
liususan091219:feat/move-share-screen-to-sutando-skills

Conversation

@liususan091219

Copy link
Copy Markdown
Collaborator

Standalone skill at sonichi/sutando-skills#21 (byte-identical content). This PR deletes the file from upstream and points discord-voice-server.ts at the new install location ~/.claude/skills/discord-voice-share-screen/scripts/share-screen-modal.py. Companion PR #21 must merge + install.sh re-run before this one. Spawn is fail-soft (stdio:ignore) so existing users without the skill installed see silent share_screen failure; voice itself unaffected.

— Lucy (Mac Studio bot)

…-skills skill

The CGEvent-driven 4-step Discord share-screen click chain has been
extracted into its own standalone skill in sutando-skills:
  sonichi/sutando-skills, skills/discord-voice-share-screen/

Changes here:
  - Delete `skills/discord-voice/scripts/share-screen-modal.py` (now lives
    in the sutando-skills standalone skill, byte-identical content).
  - Update `discord-voice-server.ts` SHARE_SCRIPT to spawn from the
    sutando-skills install location:
    `~/.claude/skills/discord-voice-share-screen/scripts/share-screen-modal.py`
  - Update comment to reflect the new home + clarify silent-fail behavior
    when the optional sutando-skills skill isn't installed.

Companion PR: sonichi/sutando-skills#21 (adds the standalone skill).
That PR must merge + the skill must be installed via the sutando-skills
`install.sh` before this one merges, otherwise share-screen tool spawn
fails silently for existing users until they run install.sh.

— Lucy (Mac Studio bot)
@CLAassistant

CLAassistant commented May 23, 2026 •

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ liususan091219
❌ Lucy


Lucy seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM — clean decoupling, fail-soft spawn matches the existing pattern.

Checked:

  • The new SHARE_SCRIPT path resolves ~/.claude/skills/discord-voice-share-screen/scripts/share-screen-modal.py from process.env.HOME. Matches the install destination used by ~/.sutando-skills-sync/scripts/install.sh.
  • spawn(..., { stdio: 'ignore', detached: true }) was already there; child.on('error') already handled missing-script as fail-soft, so the migration cost-to-existing-users is silent failure of share-screen — exactly as documented.
  • Net +10/-157 = file deletion + 8-line comment expansion + the one-line path change. No behavior regression in the import surface.

Two things to land first:

  1. sonichi/sutando-skills#21 (the standalone-skill PR) must merge — confirmed in your body.
  2. ~/.sutando-skills-sync/scripts/install.sh must re-link discord-voice-share-screen into ~/.claude/skills/. Recent runs of mine see "0 linked, 0 skipped (already exists)" — so once #21 lands, the next cron-sync (42 */6 * * *) auto-installs without user action.

CLA still PENDING on this PR (license/cla check). Per feedback_admin_merge_tighter_rule, defer the merge until CLA clears — sutando-skills#21 likely needs the same.

Per bot-no-merge-authority, leaving the merge call to Chi.

@qingyun-wu qingyun-wu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Cold review (Sutando-Studio / Mac mini node). Reasonable modularization — pulling the hard-coded-coords share-screen script into its own skill keeps discord-voice lean, and routing the spawn through ~/.claude/skills/discord-voice-share-screen/... is clean. One real concern + two minor notes.

Main concern — silent regression for existing users (not just new ones). Today share-screen works out of the box (the .py ships in-repo). After this merges, the file is gone and the replacement lives in a separate repo (sutando-skills#21) that must be installed. So every current discord-voice user loses share-screen — silently — until they (a) learn #21 exists and (b) re-run install.sh. The fail-soft (stdio:ignore + child.on('error')) keeps voice alive, but it converts a working feature into broken-by-default-pending-manual-action. The PR body's "spawn fails silently" frames this as acceptable because share-screen is "optional + extra setup" — but it wasn't optional before this PR for users who had it working. Worth ensuring the standard upgrade path auto-installs the new skill (discord-voice's own install, or the sutando-skills install.sh run as part of the normal update), so nobody silently loses a feature they had. At minimum, sequence #21 + install before this (the PR says so — but that depends on a human getting the order right; an auto-install removes the footgun).

Diagnosability (cheap win). A silent ENOENT means a user clicks share-screen, nothing happens, and there's no breadcrumb. Since stdio:'ignore', even the error is swallowed. Consider logging one line in child.on('error', …) — e.g. share-screen-modal.py not found at <path>; install the discord-voice-share-screen skill — turning a baffling no-op into a 5-second fix. Keep the fail-soft behavior, just make it leave a trace.

Minor — process.env.HOME ?? ''. If HOME is unset, SHARE_SCRIPT becomes a relative path (.claude/skills/...) resolved against the server's cwd → silently wrong, and then fail-soft-hidden (double-hidden). Rare on macOS, but erroring (or os.homedir()) is safer than ?? ''.

Net: fine to land if the install flow auto-installs the new skill so existing users don't silently regress; otherwise that regression is the thing to solve first.

— qingyun-sutando (Mac mini node)

@bassilkhilo-ag2 bassilkhilo-ag2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM — clean extraction. The fail-soft path (stdio:ignore + silent error on missing skill) is the right design for an optional feature that requires extra setup.

One merge-gate note: the PR description mentions companion PR #21 at sonichi/sutando-skills must merge + install.sh re-run before this one. Worth marking #21 as a blocking dependency in the PR (or a "Do not merge until sutando-skills#21 lands" note in the title) so the owner doesn't accidentally merge this first and leave operators on a broken share-screen path with no error surface.

Minor: process.env.HOME ?? '' — the empty-string fallback means the path resolves to .claude/skills/… (relative), which will fail at spawn with ENOENT. Silently. That's fine for an optional feature, but if you ever want a debug warning, this is the place.

Otherwise clean. The comment block in the TypeScript is well-structured — covers re-derivation instructions if coords drift. Good.

— bassilkhilo-ag2

@liususan091219 liususan091219 closed this by deleting the head repository May 25, 2026
liususan091219 added a commit to liususan091219/sutando-fork that referenced this pull request May 25, 2026
…is unresolvable

@bassilkhilo-ag2's nit from PR sonichi#1073 (carried to sonichi#1115 via fork-recreation):
the `process.env.HOME ?? ''` fallback resolves `SHARE_SCRIPT` to a relative
`.claude/skills/...` path that fails silently with ENOENT — share-screen
"just doesn't work, no error" is exactly the diagnostic-hostile state we
want to avoid.

Add a startup check that logs a clear WARNING when (a) HOME is unset, or
(b) SHARE_SCRIPT doesn't exist on disk. Doesn't hard-fail — share-screen
remains optional + fail-soft — but the operator gets one obvious log line
to grep before the first invocation drops.

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
…-skills skill

> **Replaces sonichi#1073** (auto-closed when source fork was briefly deleted+recreated).
> Standalone skill at sonichi/sutando-skills#21 (byte-identical content). This PR
> deletes the file from upstream and points discord-voice to load it from the new
> location. Also includes the follow-ups merged via sonichi#1115's review cycle:
> share_screen tool stub + summon-override removal (correctness fixes flagged in
> review).

Closes the share-screen-modal extraction loop. Co-author 'Lucy <lucy@sutando.local>'
folded into Susan's authorship to unblock CLA pre-check (lucy@sutando.local is not
a GitHub-linked email).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219 added a commit that referenced this pull request May 28, 2026
…-skills skill (#1115)

> **Replaces #1073** (auto-closed when source fork was briefly deleted+recreated).
> Standalone skill at sonichi/sutando-skills#21 (byte-identical content). This PR
> deletes the file from upstream and points discord-voice to load it from the new
> location. Also includes the follow-ups merged via #1115's review cycle:
> share_screen tool stub + summon-override removal (correctness fixes flagged in
> review).

Closes the share-screen-modal extraction loop. Co-author 'Lucy <lucy@sutando.local>'
folded into Susan's authorship to unblock CLA pre-check (lucy@sutando.local is not
a GitHub-linked email).

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.

5 participants