[sonichi]

Priority

Level: medium
Reason: owner decision: skills marked developer-only must be kept out of ordinary installs; a developer-only skill is already in review and needs this to land first.

Closes

N/A (owner decision, no issue).

Summary

Adds a manifest field audience ("user", the default, or "developer") and a host opt-in skills.developer in sutando config. Every path that links repo or workspace skills into the core's skills dirs now asks one helper, src/skill_audience.py, and skips developer-only skills unless the host opted in.

What changed

• src/skill_audience.py (new, stdlib + sutando_config): the one decision. audience(skill_dir), installable(skill_dir, developer), excluded(dirs, developer), and a CLI skill_audience.py excluded <dir>... printing name<TAB>reason for shell installers.
• Unknown values fail closed. Any audience other than "user"/"developer" (typo, wrong case, empty, non-string), and a manifest.json that is unreadable or not an object, count as developer-only. A typo in a developer skill must keep it away from users, not ship it. No manifest, or no field, means user, so every existing skill is unaffected. scripts/lint-skill.py rejects an unknown value, so a repo skill cannot merge with one.
• Opt-in: skills.developer (new top-level config object, JSON boolean, default false), read by resolve_skills_developer() in src/sutando_config.py from the merged repo / repo-local / workspace-local config. Only JSON true opts in; "true", 1, or an unreadable config are false with a stderr note. Added to both loaders' known/object key sets and to docs/sutando-config.schema.json. No env var or CLI flag: this is a host setting, not skill config, so the CLI > env > manifest > config-file > state precedence does not apply (documented in skills/MANIFEST.md).
• Installers, each delegating to the helper:
  • skills/install.sh: one excluded call per root (<repo>/skills, <workspace>/skills); the same skip applies to the Claude target and, on a Codex core, the Codex target. If no runnable python is found, or the helper fails, it links nothing and exits 1 (fail closed; startup.sh already runs it with || true, so existing links are untouched).
  • skills/install.ps1 (Windows twin): same excluded call via scripts/python-binary.ps1; fails closed the same way.
  • src/health-check.py skill-symlinks: skips skills the helper excludes, so it no longer reports them as "unlinked" and --fix no longer links them.
• Already installed on a host without the opt-in: left in place, never deleted (it may be user state); install.sh/install.ps1 print <name> (developer-only; already installed, left in place — remove it yourself, or set skills.developer).
• Runtime scans: tools, hooks, claude_plugin, supervised_worker, pending_questions_store and documented_for_core are activated by loaders that read <repo>/skills directly, not through an installer. Rather than add the audience check to each of those loaders (TS and shell included), lint-skill.py refuses those fields on a developer-only skill.
• Docs: skills/MANIFEST.md "Audience" section + field-table row; schemas/skill-manifest.schema.json gains audience; docs/src-map.md regenerated.

Paths reviewed that need no change

• skills/refresh-skill.sh: only refreshes links that already exist ("NOT INSTALLED — nothing to refresh" otherwise), so it never installs; an already-installed developer skill is treated like any other existing link.
• src/init.sh: copies crons.example.json only. src/startup.sh, src/migrate.sh, src/startup.ps1 call install.sh/install.ps1.
• src/skill_install.py (marketplace, trusted-capabilities): installs a third-party bundle the owner names explicitly, not repo skills; an explicit named install is itself the opt-in.

Stated gaps

• Desktop bundle: which skills the packaged engine copy ships, and how it materializes them, is decided in the desktop app's repository, not here. That packaging needs the same skill_audience.py excluded call (or must run skills/install.sh); not done in this PR.
• process-hygiene: the developer-only skill in feat(process-hygiene): reap orphaned test processes left in agent scratchpads; fixtures die with their test #5724 is still open, so its manifest is not touched here. One-line follow-up after feat(process-hygiene): reap orphaned test processes left in agent scratchpads; fixtures die with their test #5724 merges: "audience": "developer" in skills/process-hygiene/manifest.json.

Files to read first

src/skill_audience.py, then skills/install.sh, then the skill-symlinks hunk in src/health-check.py.

Before / after evidence

Same throwaway repo (copies of skills/install.sh and its helpers at each revision), two skills: plain (no manifest) and dev-tool ({"audience": "developer"}), minimal env, temp Claude home.

At origin/main, the developer skill is installed:

  ✓ dev-tool
  ✓ plain
Installed. Skills available in any Claude Code session.
linked: dev-tool plain

At HEAD, default install:

  - dev-tool (developer-only; not installed — set skills.developer to opt in)
  ✓ plain
Installed. Skills available in any Claude Code session.
linked: plain

At HEAD, with {"skills": {"developer": true}} in the workspace's sutando.config.local.json:

  ✓ dev-tool
  ✓ plain
Installed. Skills available in any Claude Code session.
linked: dev-tool plain

Mutation check: removing the continue after the repo-root skip in install.sh fails 4 of the end-to-end tests.

Test plan

New tests/skill-audience-developer-only.test.py (16 tests, hermetic: temp dirs, minimal env, SUTANDO_* cleared, temp CLAUDE_CONFIG_DIR/CODEX_HOME/HOME):

• rule: absent manifest/field and user → user; developer, typo, wrong case, empty, null, list, broken JSON, non-object → developer; opt-in lifts exclusion.
• opt-in: only JSON true; string/int/broken config → false; the workspace layer opts in; skills is a known object key.
• install.sh end to end in a throwaway repo: developer and typo'd skills not linked by default (both roots); Codex target gated; opt-in links all; string opt-in ignored; an already-linked developer skill left in place with the note.
• health-check skill-symlinks + fix_skill_symlinks: developer skill not linked without opt-in, linked with it.
• lint: unknown audience rejected; developer skill with tools / documented_for_core rejected.
• contract: install.sh and install.ps1 call src/skill_audience.py excluded; health-check.py imports the helper; no other code under src/, scripts/, skills/ reads audience alongside a manifest.

Existing tests run one file at a time, all green: skills-install-codex (copy list gains the helper), skills-install-workspace-skills, health-check-skill-symlinks, health-check-skill-symlinks-live, health-check-dangling-symlinks, health-check-skill-junctions, skill-symlink-ephemeral-target, config-known-keys-parity, sutando-config-twin-contract, lint-skill-supervised-worker, skill-hooks-discovery, skill-roots, src-map, ci-covers-every-python-test, test-discovery-has-one-owner, installer-python-probe.test.sh. Also: python3 scripts/lint-skill.py --all (63 manifests, 0 errors), lint-hermetic-bridge-tests.py ok, review-checks.sh PASS, git diff --check clean, shellcheck -S error skills/install.sh clean. install.ps1 was not executed (no PowerShell on the test host); its delegation is pinned by the contract test only.

Live path: N/A for delivery; install.sh runs at startup, and the before/after above is that script run end to end.

Checklist

• Confirmed no other open PR covers this
• Git author email is GH-mapped
• Single concern
• Feature not already covered on main
• Test added
• Before/after evidence pasted
• Every claim checkable from the diff or the output above
• Live path: N/A (see above)
• Stacked PR: N/A
• Scanned added lines for hardcoded host paths
• No CI workflow edits
• No new workspace/home-path resolution

Docs: skills/MANIFEST.md is the canonical doc for manifest fields (not in docs/catalog.json). CHANGELOG-PENDING entry added.

🤖 Generated with Claude Code

[liususan091219]

Reviewed at 08d86560. Verdict: the code is ready from my side, no blockers. This is a COMMENT, not an approval, only because nothing on this PR counts toward the approval bar yet: there are no reviews at this head, and CI was still running when I checked.

What I checked, in a scratch worktree at the head (merge-base is current main, 0 behind):

• One owner for the rule. src/skill_audience.py decides. install.sh (repo and workspace roots, Claude and Codex targets), install.ps1 and health-check skill-symlinks all call it, and none of them reads audience on its own. The negative scan test_no_other_code_reads_the_audience_field can actually fire: I added a control file in src/ that does m.get("audience") next to a manifest, and the test failed.

• Unknown values fail closed. In the helper, a typo, wrong case, empty, null, a list, broken JSON or a non-object manifest all count as developer. install.sh exits 1 and links nothing when no python runs or the helper fails. install.ps1 does the same: Resolve-SutandoPython throws when no python is found, and a non-zero $LASTEXITCODE is thrown too, and both land in the catch that runs exit 1. Existing links are untouched in every one of these cases.

• Tests fail when the code is reverted. I ran 16/16 green at the head. Then I reverted one thing at a time:

  • install.sh back to main: 5 failures.
  • the health-check hunk back to main: 2 failures, including the behavioural test_without_opt_in.
  • unknown audience mapped to user: 4 failures.
  • unreadable manifest mapped to user: 1 failure.
  • a truthy skills.developer (so "true" would count): 2 failures.
  • the Codex target with the gate removed: 1 failure.

  The delegation pins (test_installers_call_the_helper) read source text. That is the structural exception in REVIEW.md #14, and here it sits beside behavioural tests of the helper, install.sh end to end, and health-check --fix.

• Health-check. With the opt-in off, --fix no longer adds a developer skill. With it on, --fix links it. This is tested by behaviour, not by reading the source.

• Config key. skills.developer is a host setting rather than skill config, so I agree that the manifest-config precedence does not apply. No env var was invented, and the key was added to both loaders and to the schema. Only a real JSON true opts in; I verified that.

• Which skills are marked. None in this PR (grep '"audience"' skills/*/manifest.json is empty), so no skill that users rely on is affected. I checked #5724's process-hygiene manifest with "audience": "developer" added: it lints clean (it declares no tools, hooks or other fields that runtime scans pick up), and the helper excludes it.

• Hardcoded paths. review-checks.sh --diff PASS. The only added SUTANDO_WORKSPACE lines set test environment variables to temp dirs, not a live default.

• Related suites green: skills-install-codex, skills-install-workspace-skills, health-check-skill-symlinks (+live, +dangling), config-known-keys-parity, sutando-config-twin-contract, lint-skill-supervised-worker, skill-roots, src-map.

Non-blocking notes:

1. The "left in place" policy combined with #5724's order is the real worst case. #5724 already has two approvals at its head and does not yet carry the audience line. If it merges first, startup.sh links process-hygiene on every host. When the marking lands later, it stays installed for good, and the only notice is a stdout line from an install that startup.sh runs as 2>/dev/null || true. The simplest mitigation is to put "audience": "developer" into #5724 itself, rebased after this PR lands, rather than as a follow-up. A health-check line for "installed but excluded" would also make any leftover visible.
2. Two fail-closed paths are not pinned by tests. First, the "no runnable python, so link nothing" branch in install.sh: if I drop the exit 1 there, the suite still passes. Second, install.ps1's behaviour is pinned only by the source-text check, and the repo already has windows-*.test.ps1 suites it could join. The code is correct as written; only the coverage is missing.
3. Nit: scripts/lint-skill.py keeps its own AUDIENCES = {"user", "developer"}. Importing it from skill_audience would leave a single list of values.
4. The desktop bundle gap is named, not blocking. On a desktop install the skills dir is linked by startup.sh → install.sh (I checked: the links on this host point into the engine checkout), so the gate already applies there at link time. What is still open is only whether the bundle should ship the files at all.

Missing before merge: two approvals at this head from people outside the author's party, and CI finishing green: at submit time 32 checks pass, and "tsc + tests (clean install)" and "diff coverage >= 95% (python)" are still pending. CLA: license/cla=success (1 context). Unresolved threads: 0.

— Sutando (susans-MacBook-Pro)

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 92%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• scripts/lint-skill.py (100%)
• src/health-check.py (100%)
• src/skill_audience.py (100%)
• src/sutando_config.py (100%)

Summary

• Total: 77 lines
• Missing: 0 lines
• Coverage: 100%

[liususan091219]

Re-review at b9f0efea. Verdict: still ready from my side, no blockers. This is a COMMENT, not an approval, because nothing at this head counts toward the approval bar yet: there are no approvals, and CI is still running.

1. What changed since 08d86560. Two commits: a clean merge of main (21a569c0, which has no conflict-resolution hunks) and b9f0efea, which adds exactly one line, "audience": "developer", to skills/process-hygiene/manifest.json. The merge brought in #5724 and #5727 only. Neither one touches skill_audience.py, install.sh/install.ps1, health-check.py, sutando_config.py, lint-skill.py, MANIFEST.md, python-binary.* or the new test. Compared with main, the PR is still the same 15 files plus the manifest line.

2. process-hygiene is now excluded.

• skill_audience.excluded(skills/*/, developer=False) returns only [('process-hygiene', 'developer-only')].
• lint-skill.py --all reports 64 manifests, 0 errors.
• The new suite passes 16/16.
• I also ran install.sh end to end in a throwaway repo with the real process-hygiene and answer-format skills, a temp Claude home and a minimal env. It printed - process-hygiene (developer-only; not installed …) and linked only answer-format.

3. The "left in place" window is now real.

• #5724 has been on main since 18:43:58Z today, which was a few minutes before this review.
• No release tag contains it yet (git tag --contains 2360e1f7 is empty). So desktop installs that update from releases are not exposed. The exposed hosts are the ones that track main, such as developer-mode checkouts and source installs that pull.
• On those hosts the normal path does link it: startup.sh runs skills/install.sh on every start, and health-check --fix would link it too.
• I reproduced the leftover case with a link created before this PR. After this PR it stays, with only the ⚠ … already installed, left in place line on stdout. startup.sh runs that install with 2>/dev/null || true, and health-check skill-symlinks now skips the skill silently. So nothing a person reads would flag it.

I'm keeping this non-blocking, for two reasons. The exposed group is minutes of main-tracking hosts, and those are almost all developers, who are the skill's intended audience anyway. And a blanket delete would risk user state. But the window closes only when this PR merges, and it must merge before the next release is cut. Otherwise the release ships process-hygiene unmarked to every desktop install, and this policy then keeps it there for good. I'd suggest a narrow, safe follow-up, either here or right after:

• When the opt-in is off, remove a developer skill's link only if it is a symlink whose target resolves to this checkout's own skills/<name>, i.e. the link install.sh itself made. Real directories and links pointing anywhere else stay untouched.
• Separately, have skill-symlinks report "installed but excluded" as a warn, so a leftover is visible.

4. The two fail-closed paths are still untested. If I drop the exit 1 after the "no runnable python3" check in install.sh, the suite still passes. install.ps1 is still pinned only by the source-text delegation check. The code is correct as written; only the coverage is missing.

Gates:

• CLA: license/cla=success (1 context).
• Unresolved threads: 0.
• Mergeable: yes.
• CI at submit time: 23 pass, 9 pending (the python standalone shards, node + out-of-tree python tests, shell lanes).
• No change-request condition fires: tests are behavioural and fail on revert, nothing claimed is missing, and there are no hardcoded paths.

Missing before merge: two approvals at this head from people outside the author's party, and CI finishing green.

— Sutando (susans-MacBook-Pro)

[github-actions]

@cla-assistant check

[qingyun-wu]

Reviewed at b9f0efe, covering the main merge and the audience: developer line in process-hygiene. The merge resolved no conflicts and touched none of this PR's files. The manifest line matches skills/MANIFEST.md, and process-hygiene is the only skill main added since the merge base. The audience tests pass 16/16, skills-install-codex passes 2/2, and lint-skill on process-hygiene is clean.

One blocker: diff coverage >= 95% (python) fails at 87% on src/skill_audience.py, missing lines 57 and 82-87. Line 57 is the sys.path.insert in developer_opt_in. Lines 82-87 are main(), the usage error plus the excluded print loop. Those paths most likely run only as a subprocess from install.sh, which coverage doesn't trace. An in-process test that calls main([]) (expect 2) and main(["excluded", <dev skill>, <user skill>]) with developer_opt_in patched, plus one developer_opt_in call with src removed from sys.path, should cover all seven lines. I'll approve once that check is green.

[qingyun-wu]

Approve at ea88a12. CI: 33 success, 1 skipped, and CLA passes. After a shard rerun, the only red was the browser flake tracked in #4999.

• Coverage. The coverage gap is closed. In-process tests now cover the helper CLI (usage error → 2, and excluded printing the reason line) and developer_opt_in with src removed from sys.path. Those were the 7 missing lines in src/skill_audience.py. The diff-coverage check passes.
• Earlier verification still holds from my 08d8656 review:
  • Fail-closed audience: a missing field means user; an unknown or invalid value means developer.
  • skills.developer is read through load_config and accepts only true.
  • A single decision point serves install.sh, install.ps1 and health-check.
  • No silent removal.
• Merge timing. This needs to merge before the next release is cut. #5724 is on main, so a release before this lands would ship process-hygiene unmarked, and this policy would then keep it installed.
• Follow-up (Susan's suggestion). When the opt-in is off, unlink a developer skill only when the link points at this checkout's own skills/<name>. Separately, have skill-symlinks report "installed but excluded".