skills support

Codex Custom Skills Support

Date: 2026-08-04 Tracking: Issue #119, sub-issues #120, #121, #122, #123, #124, #125, #126, and #127 Status: Implementation review completed; remediation planned on 2026-08-06

Invocation UX superseded: The invocation-surface portions of this historical plan are superseded by Unify Built-in Commands and Skill Invocation in the Slash Menu, tracked by Issue #140.

Summary

Add Codex custom skills support per doc/task.md Phase 3.2: skills/list retrieval with caching, $<skill-name> mention resolution and a skill turn input item, skills/config/write enable/disable, and skills/changed invalidation.

This plan started from a draft produced by GitHub Copilot and was reviewed against this repository's Worker/Contracts/Remote-UI patterns and the vendored app-server schemas before implementation began. The review corrected several blockers that would otherwise have shipped broken or unsafe behavior; see below.

Protocol baseline

Verified by regenerating the app-server protocol schema from the currently installed codex-cli (0.145.0) and diffing against the vendored schemas/ (generated 2026-06-10): every skills/* schema is unchanged. Key facts:

• skills/list accepts { cwds, forceReload }. Empty cwds resolves to the session working directory. The response is an array of per-cwd entries, each carrying its own skills[] and errors[] — a SKILL.md parse failure must not be silently dropped when the entries are flattened for the client.
• SkillMetadata has no id field. Identity is the (name, scope, path) tuple; scope is one of user, repo, system, admin.
• skills/list has no pagination cursor (unlike model/list and permissionProfile/list); the response array is still unbounded and must be capped client-side.
• skills/changed carries an empty params: {} object — not no params. A handler that does not intercept it before the generic notification tail turns every skill file save into a stray Unknown conversation event in the chat transcript.
• The skill turn input item is { type: "skill", name, path }, structurally parallel to mention. Nothing in the schema marks any skills/* method as experimental.
• No AppUserInput/app-mention turn item exists yet, even though app/read and app/installed were added in the same schema refresh — the $<app-slug> mention planned elsewhere in this wiki has no protocol backing today.

Corrected blockers

The original draft would have compiled but broken at runtime in several ways:

1. skills/changed leaks into the transcript. Its notification has to be intercepted before CodexSessionService's generic notification tail, the same way account/login/completed and similar methods already are.
2. Skill turn items must not reuse the file-attachment pipeline. That pipeline requires its attachment-specific workspace policy. A skill path is instead the absolute path of its SKILL.md file and can live outside the workspace by design (user/system/admin scope). Skills need their own request field and their own validator that checks structural validity without applying attachment containment.
3. A plain DTO renders as empty rows in Remote UI. Only [DataContract]/ [DataMember] types reach the VS-side proxy; any skill list surfaced in the tool window needs a dedicated presentation type, not the wire DTO directly.
4. Adding an event to IWorkerBridge is a source break, unlike adding a method (which can carry a default implementation). The hand-written test double in ViewModelTests.cs needs an explicit update in the same PR that adds SkillsChanged.
5. The contract version must bump (13 → 14) alongside the StartTurnRequest and ICodexWorkerClient changes, or a stale Worker binary fails to connect with no chat-visible reason.

Confirmed decisions

Question	Decision
$ mention trigger	Reserved for skills only in v1. The $<app-slug> mention noted elsewhere is deferred until an app turn-input type exists.
skills/config/write confirmation	No confirmation dialog — it is a local, instantly reversible per-item toggle. system/admin scope skills are client-side non-toggleable, and the UI always reconciles to the server's effectiveEnabled rather than the requested value.
Experimental API gate	Not gated behind ExperimentalApiEnabled. Rely purely on the existing -32601 capability probe; nothing in the protocol marks skills as experimental, and the hard-gate pattern used elsewhere is sticky for the whole session.

Sub-issue breakdown (#120–#127)

Eight stacked PRs, each branching from the previous and independently buildable/testable:

#	Scope
#120	skills/list retrieval in the Worker session (contract bump, capability probe, caps, Fake app-server seed data).
#121	/skills slash command listing the catalog in the chat transcript — first user-visible feature, no XAML.
#122	skills/changed invalidation and the Worker's first positive result cache.
#123	$skill mention resolution against the cached catalog and the skill turn input item, without the completion overlay.
#124	Inline $ skill-suggestion overlay in the composer.
#125	Read-only skills panel (scope, status, load errors).
#126	skills/config/write enable/disable toggle — isolated because it is the only mutation in the stack.
#127	Documentation (doc/skills.md + _ja, doc/task.md, doc/slash-commands.md).

Review remediation execution plan (2026-08-06)

The implementation review accepted eight actionable findings across PRs #128–#135. The current app-server documentation was rechecked on 2026-08-06 before planning: the recommended skill turn item and path-based skills/config/write both use an absolute SKILL.md path; skills/changed remains an invalidation signal; and skill names can be namespaced, as in github:yeet.

Because these are stacked PRs, update them from the bottom of the stack upward. For each child, rebase onto the newly pushed parent, resolve only stack-related conflicts, run its focused checks, and push with --force-with-lease. Record the remote head SHA before every rebase so an unexpected remote update stops the push instead of being overwritten.

Order	Issue / PR	Remediation	Focused verification	Intended commit
1	#120 / PR #128	Change Fake app-server seed paths from skill directories to absolute SKILL.md paths and update its protocol assertions.	Fake app-server request/response tests, Worker skill-list tests, git diff --check.	fix: align fake skill paths with app-server protocol
2	#121 / PR #129	Reject unsupported /skills arguments; accept only an empty argument or reload, and return local usage guidance without sending a turn.	Slash-command parsing and transcript tests for empty, reload, unknown, and excess arguments.	fix: validate skills command arguments
3	#122 / PR #130	Add a skill-cache generation. Capture it before skills/list and publish the result only if no skills/changed, reconnect, or other invalidation advanced the generation.	A controllable in-flight request test proving an old response cannot repopulate an invalidated cache, plus cache-hit and reload tests.	fix: prevent stale skill cache repopulation
4	#123 / PR #131	Stop a $skill token at sentence punctuation while preserving supported name characters, including hyphens and namespace colons. Correct ADR wording so paths are SKILL.md files, not directories.	Parser tests for comma/period/parenthesis boundaries, github:yeet, hyphenated names, escapes, duplicates, and emitted turn items.	fix: parse punctuated skill mentions
5	#124 / PR #132	In the suggestion refresh error path, close the overlay only when the failing request still owns the current refresh token.	A delayed request A that fails after request B succeeds must not close or replace B's suggestions.	fix: preserve newer skill suggestions
6	#125 / PR #133	Guard panel refreshes with the connection generation and availability state so a pre-disconnect request cannot repopulate the panel. Replace translucent scope/description foregrounds with opaque Visual Studio theme resources for High Contrast.	Disconnect/reconnect race tests, panel mutual-exclusion tests, XAML resource/opacity assertions, and manual Light/Dark/Blue/High Contrast inspection.	fix: prevent stale skills panel updates
7	#126 / PR #134	Rebase the configuration writer onto the generation-aware cache and route successful writes through the same invalidation helper. This PR has no separate review finding, but must not restore direct cache assignment while integrating #130.	Toggle success/failure/reconciliation tests and a write-during-list race test.	fix: invalidate skill cache after config writes
8	#127 / PR #135	Correct English and Japanese documentation to describe absolute SKILL.md file paths and external-workspace scope accurately. Recheck every protocol example against current app-server documentation.	Documentation link/path review, English/Japanese parity, git diff --check, then final stack validation.	fix: correct skill path documentation

After each push, wait for that PR's required GitHub checks and confirm that its diff still contains only its own issue scope relative to the updated base. After PR #135, run one clean warnings-as-errors Release build, the Core and UI skill-focused tests, and the complete test projects with --no-build. Any pre-existing failure must be reproduced on the unchanged baseline before it can be classified as unrelated. Finish with a Fake app-server smoke test for skills/list, skills/changed, skills/config/write, and the emitted skill turn item.

Verification

1. Regenerate the app-server schema from the pinned Codex CLI version before starting the first PR and diff the skills/* files against the vendored copy.
2. One deterministic warnings-as-errors solution build, then both test projects with --no-build.
3. CodexSessionServiceTests/WorkerRpcServiceTests cover capability-probe fallback, cache hit/invalidation, malformed/truncated payloads, and redaction.
4. ViewModelTests/XAML regression tests cover transcript output, panel mutual exclusion, composer suggestion keyboard behavior, and Remote UI sanitization.
5. Manual: pipe skills/list/skills/changed/turn/start requests into Codex.AppServer.Fake and confirm the seeded catalog, the absence of a stray transcript event on skills/changed, and the emitted skill turn item.