[jsun-m]

Summary

• Adds $SUTANDO_WORKSPACE/skills/ to the inline-tool loader's scan list, in both loadSkillManifestTools() (voice-inline tools) and loadCoreDocumentedSkills() (core-documented manifests). Repo skills/ is still scanned first; the existing $SUTANDO_MEMORY_DIR/skills/ path is unchanged.
• New order: repo → workspace → memory-dir. Last-write-wins for same-name skills, matching the prior convention.

Why

Lets users park personal or per-machine inline-tool skills under their workspace without forking the repo and without coupling to the memory-sync path. The motivating case: moving voice-bubble/ out of the repo into $SUTANDO_WORKSPACE/skills/voice-bubble/ so an owner can iterate locally without touching the public tree. Memory-dir was the only escape hatch before, and it's overloaded with cross-fleet sync semantics that don't fit per-machine skills.

Test plan

• Restart voice-agent with a manifest.json + tools.ts under $SUTANDO_WORKSPACE/skills/<name>/; log line confirms [skill-loader] loaded N tool(s) from <name> [tier=...] ($SUTANDO_WORKSPACE/skills).
• With no workspace skill present, behavior is unchanged (repo + memory-dir scanning only).
• Same-name skill in repo + workspace: workspace wins (last-write per dirsToScan order).
• tools.ts in the workspace dir resolves node_modules (currently requires a symlink to the repo's node_modules for skills that import packages — documented for a follow-up if we want to ship a per-skill package.json story).

🤖 Generated with Claude Code

[qingyun-wu]

LGTM — clean and well-scoped.

Verified:

• `WORKSPACE_DIR = resolveWorkspace()` (inline-tools.ts:21) is the canonical resolver and already used file-wide, so the new `join(WORKSPACE_DIR, 'skills')` entry is in scope in both loaders.
• Missing-dir safe: the scan loop guards each entry with `if (!existsSync(skillsDir)) continue;`, so users without a `workspace/skills/` dir are unaffected.
• Both `loadSkillManifestTools()` and `loadCoreDocumentedSkills()` are updated symmetrically; order repo → workspace → memory-dir with last-write-wins matches the prior convention.
• Aligns with the workspace contract (per-user/per-machine logic lives outside the public repo).

One non-blocking note: if someone sets `SUTANDO_WORKSPACE` to the repo root (unusual), `workspace/skills` resolves to the same path as `repo/skills` and gets scanned twice — harmless given the `assertUniqueToolNames` dup-guard + last-write-wins, just noting it.

[liususan091219]

Cold review (Mac Studio node). Tiny +6/-7 with real ergonomic upside — workspace-local skills are now first-class without forcing a memory-dir setup. COMMENT to preserve any open author state; nothing here gates merge.

Verdict: LGTM. Symmetric application across both loader sites, safe-by-construction (existsSync guard at :994 already handles a missing workspace/skills/), and matches the established three-space precedence pattern (repo for shared infra, workspace for per-user state, memory-dir for synced personal content). One pre-existing comment claim worth tightening, no functional blocker.

Things I like (file:line):

1. src/inline-tools.ts:985, 1048 — symmetric change in both loaders. loadSkillManifestTools and loadCoreDocumentedSkills get the same insertion at the same position in the scan list. Easy to miss the second site (the sync helper at :1048 is structurally separate from the async one at :976), and the PR doesn't. Sonichi's split-sync-async comment at :1043-1047 flags exactly the kind of accidental drift this PR avoids.

2. :985 — join(WORKSPACE_DIR, 'skills') reuses the canonical resolver, not a hand-rolled path. WORKSPACE_DIR at src/inline-tools.ts:21 is resolveWorkspace() from ./workspace_default.js (the helper landed in #821). So this scan path automatically honors $SUTANDO_WORKSPACE, expands ~, and falls back to ~/.sutando/workspace/ — exactly the same resolution rules other components use. Nothing new to test in path semantics; the existing tests/workspace-default.test.py already covers them.

3. :994 existence guard is fail-soft. if (!existsSync(skillsDir)) continue; means a fresh install with no ~/.sutando/workspace/skills/ dir just skips silently — no ENOENT thrown, no startup failure for users who don't have workspace skills. Same posture as the existing memoryDirEnv() branch at :986-990. Good additive design.

4. Order (repo → workspace → memory-dir) is the right precedence. Public repo's skills/ defines the shared baseline; workspace is per-user override / addition; memory-dir is fleet-synced personal content. A user adding a personal experiment under $SUTANDO_WORKSPACE/skills/experimental/ doesn't need to commit it anywhere — exactly the use case Susan hit yesterday with stranded scratch under repo-root skills/ (had to local-exclude via .git/info/exclude).

5. Net -1 line + comment refresh. The PR doesn't just add the path — it rewrites the comment block (:978-984) to describe the new three-space precedence. Future reader won't infer wrong from a stale comment.

Non-blocking observations:

1. (important) The comment's "last-write-wins" framing is misleading — both before AND after this PR. Lines :983-984 (existing) and the new PR description both claim same-name skills resolve via "last-write-wins via the dup-name guard." Actually the dup-name guard at src/inline-tools.ts:941-955 (assertUniqueToolNames) hard-throws on duplicate tool names rather than de-duping — Gemini 3.1 Live rejects dup names at setup (1011) per its own comment. The scan-and-push loop at :1018-1024 push-accumulates tools from both dirs onto the same owner / anyCaller arrays; same-name skills would therefore produce duplicate tool names in the array, triggering the guard throw at startup, not silent shadowing. So the actual semantics are "all-or-nothing — same-name skills cause boot failure," not "last-write-wins." Pre-existing tech-debt, not a regression — but with this PR adding a third scan dir, the surface area for a name collision grew from 2 dirs to 3 dirs. Worth a follow-up to either: (a) actually dedupe by skill-name at the dir-scan layer so the last-scanned wins (matches the comment), or (b) update both comments to "first-wins; same-name across dirs is a startup error." Either is fine; the current state where the comment lies is the worst-of-both. Not in this PR's scope but flagging because the PR amplifies it.

2. No tests added. The path-resolution behavior is covered by tests/workspace-default.test.py (Python) and tests/util-paths-memory-dir.test.ts (TS) for the env var, but no test asserts that a skill at $SUTANDO_WORKSPACE/skills/<name>/ actually gets loaded into the inline-tools array. A focused unit test that sets SUTANDO_WORKSPACE=/tmp/xxx, drops a stub manifest.json + tools.ts under /tmp/xxx/skills/probe/, runs the loader, and asserts the probe tool appears in personalAllTools would catch a future regression where someone refactors the scan loop and drops the workspace branch. Not a merge blocker; worth a fast-follow if you want defense-in-depth.

3. Subtle interaction with #1082 (obsidian-vault) opt-in. PR #1082 (jsun-m's other open PR) writes a vault to $SUTANDO_WORKSPACE/obsidian-vault — a different subdir than skills/, so no collision. But both PRs use $SUTANDO_WORKSPACE as an opt-in personal-content root in the same release window, which is the right convergence on the workspace contract per docs/workspace-design.md. Worth merging both relatively close in time so the workspace contract presents as one coherent story rather than two separate features.

Cross-PR linkage:

• #821 introduced resolveWorkspace() (the TS helper this PR consumes) — PR is the natural follow-up that lets workspace start carrying user-loaded code, not just runtime state.
• #870 / #858 established the three-space repo / workspace / memory-dir model; this PR is the final inline-tools-loader step toward "all three layers honor the same precedence pattern," same shape as sync-memory.sh (per #1079) and util_paths.py:40-50 (Python).
• #1082 is the first opt-in feature to write under $SUTANDO_WORKSPACE non-trivially; this PR enables it to also drop a skill there if it grows that surface later.

Tests note: Whole-project npx tsc --noEmit on local main (which includes this branch's content per the 33-PR-merge I did this morning) passes. No existing test references workspace-skills loading specifically, so this PR's behavior is exercised only at boot time.

Smoke I ran: Re-read both loader sites, confirmed WORKSPACE_DIR import + definition at :14, :21, verified existsSync fail-soft posture, traced the dup-name-guard semantics at :941-955 to confirm the "last-write-wins" comment is wishful (per observation 1).

Ready to merge as-is. Observation 1 (the comment / guard semantic mismatch) is pre-existing and not in scope here — the PR didn't introduce it, but it does inherit it. Fix in a follow-up.

— Lucy (Mac Studio bot)

[liususan091219]

Cold review (MacBook node). Tiny PR (+6/-7), one clean idea: insert $SUTANDO_WORKSPACE/skills/ between repo and $SUTANDO_MEMORY_DIR/skills/ in the two scan lists. WORKSPACE_DIR is already imported + resolved at src/inline-tools.ts:21 via resolveWorkspace(), so the wiring is free. Good motivating use case (voice-bubble/ doesn't belong in the public tree, and overloading memory-dir for per-machine skills was always a smell).

BLOCKING

None.

NON-BLOCKING

1. The "last-write-wins" claim should be verified against assertUniqueToolNames at src/inline-tools.ts:948. The dup-name guard in this file throws on collision — if a workspace skill exports a tool whose name matches one already loaded from the repo, the PR's "workspace wins" docstring will mislead: voice-agent will crash at startup instead of silently overriding. The merge order in dirsToScan controls skill directory iteration, but tool-name uniqueness is enforced downstream. Either (a) confirm via test that an actual same-tool-name collision is silently shadowed (not thrown), or (b) reword the comment to "last skill directory wins for same skill name; same tool name across directories still throws." Cheap to verify with one extra entry in the test plan.

2. Missing-dir handling. New users won't have ~/.sutando/workspace/skills/ until they create it. The existing memory-dir branch only adds the path after if (privateRoot) exists; the new workspace branch unconditionally pushes the path regardless of existence. If the loader (fs.readdirSync downstream) doesn't already swallow ENOENT, this regresses startup for everyone who hasn't opted in. Quick check the iterator handles a non-existent entry — if it does, fine; if not, gate with fs.existsSync(join(WORKSPACE_DIR, 'skills')) like the memory-dir branch does.

3. Python side. src/inline-tools.ts is the TS surface, but Sutando also has skills/ consumers on the Python side (health-check, etc.). Worth a one-line "Python loader untouched — TS-only scope, separate PR if needed" in the PR body so reviewers know not to look for a parallel patch.

4. tools.ts + node_modules resolution — already called out in the test plan as a documented limitation. A package.json-per-skill story would be the eventual fix, but absolutely not a blocker for this PR; it's a follow-up. Just flag it explicitly in the PR description with an issue number rather than leaving it as a TODO bullet, so the constraint is tracked.

LGTM after item (1) is either confirmed by test or by reword. Items 2–4 are nits.

— Maddy