feat(skills): support disable-model-invocation skill frontmatter - #3776
feat(skills): support disable-model-invocation skill frontmatter#3776haacked wants to merge 3 commits into
Conversation
Parse the disable-model-invocation frontmatter field into SkillInfo, preserve it through manifest saves (previously editing a skill silently stripped it), emit it from serializeSkillMarkdown, and surface it in the skills UI as a badge plus an edit toggle. Generated-By: PostHog Code Task-Id: b9824961-d68a-4ac2-b272-7053ecc0e497
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
🦔 ReviewHog reviewed this pull requestFound 1 must fix, 0 should fix, 0 consider. Published 1 finding (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 1 issue
Files (8)
packages/shared/src/skills.tspackages/ui/src/features/skills/SkillCard.tsxpackages/ui/src/features/skills/SkillDetailPanel.tsxpackages/ui/src/features/skills/SkillManifestEditor.tsxpackages/workspace-server/src/services/skills/parse-skill-frontmatter.tspackages/workspace-server/src/services/skills/schemas.tspackages/workspace-server/src/services/skills/skill-discovery.tspackages/workspace-server/src/services/skills/skills.ts
…invocation A value like `disable-model-invocation: true # manual only` misparsed to false because extractYamlValue keeps trailing comments, and a later manifest save would then silently drop the field from disk. Strip an unquoted trailing comment before the boolean comparison and cover the commented forms with tests. Generated-By: PostHog Code Task-Id: b9824961-d68a-4ac2-b272-7053ecc0e497
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/workspace-server/src/services/skills/parse-skill-frontmatter.ts:36-37
**Quoted scalar content becomes boolean**
When a hand-authored manifest contains a quoted non-boolean value such as `disable-model-invocation: "true # manual only"`, comment stripping removes the quoted suffix before validation, causing the UI to enable the setting and a subsequent save to rewrite it as boolean `true`.
Reviews (1): Last reviewed commit: "fix(skills): strip trailing YAML comment..." | Re-trigger Greptile |
| const withoutComment = value.replace(/\s+#.*$/, "").trim(); | ||
| return unquoteYamlScalar(withoutComment).toLowerCase() === "true"; |
There was a problem hiding this comment.
Quoted scalar content becomes boolean
When a hand-authored manifest contains a quoted non-boolean value such as disable-model-invocation: "true # manual only", comment stripping removes the quoted suffix before validation, causing the UI to enable the setting and a subsequent save to rewrite it as boolean true.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/workspace-server/src/services/skills/parse-skill-frontmatter.ts
Line: 36-37
Comment:
**Quoted scalar content becomes boolean**
When a hand-authored manifest contains a quoted non-boolean value such as `disable-model-invocation: "true # manual only"`, comment stripping removes the quoted suffix before validation, causing the UI to enable the setting and a subsequent save to rewrite it as boolean `true`.
How can I resolve this? If you propose a fix, please make it concise.| "---", | ||
| `name: ${serializeSkillScalar(meta.name)}`, | ||
| `description: ${serializeSkillScalar(meta.description)}`, | ||
| ...(meta.disableModelInvocation ? ["disable-model-invocation: true"] : []), |
There was a problem hiding this comment.
Medium: Manual-only is not enforced for Codex sessions
This writes only Claude's disable-model-invocation frontmatter, while Codex sessions link these same skill directories into CODEX_HOME. Codex controls automatic selection through policy.allow_implicit_invocation in agents/openai.yaml, so a skill can still be selected and run without an explicit invocation even though the UI labels it Manual-only. Generate the corresponding Codex policy when preparing skills, or scope the setting and UI guarantee to agents that enforce it.
PR overviewAdds support for the The manual-only behavior is currently enforced for Claude sessions but not for Codex sessions, which use a separate implicit-invocation policy. As a result, Codex may still automatically select and run a skill that the UI labels Manual-only; no issues have yet been addressed. Open issues (1)
Fixed/addressed: 0 · PR risk: 4/10 |
Comment stripping in parseYamlBoolean ran on the already-unquoted value, so a quoted non-boolean like "true # manual only" was mangled into boolean true. Move comment handling into extractYamlValue's plain-scalar path (quoted content is literal in YAML and never contains a comment) and add a regression test. Generated-By: PostHog Code Task-Id: b9824961-d68a-4ac2-b272-7053ecc0e497
|
This PR has had no activity for 7 days and has been marked stale. We are moving to the monorepo and tightening PR staleness in preparation, so it will be closed in 7 days if no further activity occurs. |
|
Remade in the monorepo as PostHog/posthog#76464 (PostHog/code main is frozen after the desktop import). |
Problem
Claude Code skills support
disable-model-invocation: truein SKILL.md frontmatter: the agent never invokes the skill on its own — only an explicit user invocation (slash command or composer skill tag) runs it. The bundled agent CLI already honors the field at runtime (both locally and in cloud sandboxes, since skill bundles ship SKILL.md verbatim), but PostHog Desktop was blind to it: the field wasn't parsed, wasn't shown anywhere in the skills UI, and — worse — editing a skill in the manifest editor silently stripped it from the file.Changes
parseSkillFrontmatternow extractsdisable-model-invocation, exposed as an optionaldisableModelInvocationonSkillInfo.serializeSkillMarkdown(the shared SKILL.md serialization contract) emits the field, so manifest saves round-trip it instead of dropping it.Not covered: team skill publish/install — the team-skills API only stores name/description/body, so the flag doesn't survive that round trip yet.
Related (different feature, no overlap): #3735 adds always-on skills.
How did you test this?
pnpm --filter @posthog/workspace-server exec vitest run src/services/skills: 111 passed).pnpm --filter @posthog/ui exec vitest run src/features/skillspassed.pnpm typecheckfor shared, workspace-server, host-router, ui, and web; Biome check on changed files.Automatic notifications
Created with PostHog Code