feat(skills)!: deliver skills through an AionUi-owned view instead of the workspace - #938
Merged
Conversation
added 15 commits
August 24, 2026 23:29
Declares per-vendor skill delivery as DATA rather than code, so a new vendor capability ships without a migration or a release. The column carries no DB CHECK on purpose: it is an open extension point, not a closed state machine, and a CHECK would hard-fail a registry insert carrying a newer mode on an older DB. Validation lives in the application layer and is deliberately tolerant -- an unknown mode warns WITH the actual value and falls back to injected, on a log line distinct from malformed JSON, because 'a newer registry wrote this' and 'this data is damaged' need opposite responses. Seeded values, verified against the builds we actually spawn: - claude 2.1.231: argv (--plugin-dir) plus --add-dir allow-listing. A matched pair under the real default permission mode showed both files read with two --add-dir flags and both refused with none, so repeated allow-listing works and --plugin-dir does not exempt skill dirs from the path check. - codex: protocol (skills/extraRoots/set), per its self-generated schema. - codebuddy: stays injected. The pinned 2.137.1 documents --plugin-dir and accepts it at argv level, but whether it actually surfaces skills is unprobed (needs an authenticated account); declaring argv would switch injection to light, so an inert flag would leave it with no skills at all. - antigravity, opencode: injected, no session-scoped skill flag exists. - every other vendor: NULL, read as injected.
Moves the symlink landing site out of the user's workspace into
{data_dir}/session-skills/{user_id}/{conversation_id}/. One tree serves both
vendor shapes: the plugin root carries .claude-plugin/plugin.json for
--plugin-dir, and skills/ underneath is the skills root codex extraRoots wants.
Both were probed against this exact layout, including with the skill directory
as a symlink.
Owning the tree changes two things versus the workspace path it replaces:
rebuild is an exact match against the snapshot, so a skill dropped from
extra.skills actually disappears (first-write-wins could not do that), and
deletion is safe. Removing the tree unlinks our symlinks and never touches the
real skill sources, which a test pins.
No copy fallback: a failed link is skipped with a warn rather than degrading to
a recursive copy of real files, as the old workspace path did.
plugin.json's name is 'aionui' and is deliberately user-visible -- a plugin name
becomes the prefix of every skill name the agent sees, so extra.skills' bare
'cron' is presented as 'aionui:cron'. Callers must not assume the two sides
match when correlating by name.
Orphan cleanup is keyed by the (user, conversation) PAIR, not the conversation
alone, so one user's missing conversation cannot reap another user's view.
… links Additive on purpose: the view directory starts being built and torn down, while workspace linking keeps working. Nothing loses skills in this state, which is what makes removing the workspace path a separate, revertible step. The view is synced for EVERY agent, not only vendors that consume it, so flipping a delivery mode in the registry -- a data-only change -- needs no per-conversation backfill. An empty snapshot syncs nothing: registering an empty plugin root would still cost the agent an always-on token line. Lifecycle: create builds it, build-task re-syncs it idempotently (so a view deleted out-of-band, or never built on an older conversation, comes back), and delete drops it BEFORE the row disappears -- afterwards nothing knows the (user, conversation) pair and the view would wait for a startup sweep. skill_delivery_plan is a pure function so the substitution rules are testable in isolation: they are the part most likely to go subtly wrong (wrong root handed to the wrong vendor, an allow-list collapsing to one entry). Backends only ever see already-substituted strings. A leftover placeholder from a newer registry is kept verbatim and reported rather than dropped, because dropping a flag silently changes the spawn. sync_skill_view/remove_skill_view default to no-ops on the trait so the nine existing test stubs need no bodies; the default cannot hide a production regression because the real implementation has its own test driving it against a temp data dir.
Adds resolve_skill_delivery as the single place every factory branch resolves delivery: which mode applies, which paths get substituted, what gets logged. Splitting that per branch is how the old native_skills_dirs logic drifted between the create path and the build path. claude's layer-1 flags travel via SessionConfig.extra_args rather than being hard-coded in build_claude_init_args, which makes its delivery data-driven like every ACP vendor's: a flag change becomes a registry row instead of a code change. Verified with a live paired probe against claude 2.1.231 that repeating --add-dir works despite the flag being declared variadic (<directories...>) -- two flags read both out-of-cwd files, none refused both -- and pinned on our side by a test that prepend_args does not de-duplicate. The ACP lane gets the same args appended after the launch policy, so a newly probed vendor needs one config row and no code. resolve_skill_dirs_for_user routes through the same materialize_skills_for_agent resolution the conversation service uses for the view, so the directories a CLI gets allow-listed and the directories the view links to agree by construction rather than by coincidence. The delivery decision is logged at info, not debug: it is the anchor for 'why did this vendor not pick up native skills' in a default-level production log. Paths are counted, never listed -- a full allow-list would put user directory names in the log.
Verified against codex-cli 0.146.0 in two ways this change relies on.
Wire shape, from the binary's own generated schema (codex app-server
generate-json-schema, v2/SkillsExtraRootsSetParams.json): exactly one required
extraRoots array of absolute paths, nothing else.
Behaviour, from a live codex app-server --stdio probe: the request answers {},
codex pushes skills/changed, and skills/list reports the skill with errors: [].
Two properties that probe pinned are load-bearing -- a SYMLINKED skill directory
is discovered (which the whole view design depends on), and path comes back as
the REAL source path, so a canonical-path check would not match a view entry.
That is why allow-listing targets real source dirs instead.
R2' is written into the code as a hard constraint. extraRoots is process-scoped,
confirmed twice over: the params carry no threadId (ThreadForkParams and friends
do), and the probe reported the skill with scope: user. Isolation holds only
because this file opens one process per logical session, so codex layer-1
delivery and thread multiplexing are mutually exclusive -- enabling reuse first
requires switching to per-skill skills/config/write or dropping codex to
injected, with a cross-conversation isolation test in the same change.
Sending is fire-and-forget: the response is an empty object so there is nothing
to claim, and a rejection must not fail the session because the dual channel
still covers skills.
… to the skill root Two independent defects on the fallback channel, both of which fail silently. P1: detection ran on the think-stripped text, so a request written inside <think> -- where reasoning models routinely put this kind of meta-decision -- disappeared entirely, with no log line anywhere. It presented as 'the model said it would use a skill, then nothing happened'. Detection now reads the RAW message while display keeps reading the stripped text; the two have different purposes and must not share one value. A request is now logged at info with the same structured fields as the command channel, including whether it came from a thinking block, so the split between the channels is measurable. P2: the injected block was '[Skill: name]\n<body>' with no path, so the body's own relative references (references/*.md, scripts/*.py -- skill-creator alone has three) resolved against the agent's CWD, the workspace. The harmless outcome is a missing file; the dangerous one is a workspace that happens to hold a same-named file, which the agent then reads believing it read the skill's. The skill root now leads the block. An integration test builds exactly that collision -- a decoy references/workflows.md in the workspace -- and asserts the injected text points at the skill and never at the workspace. P2 stands on its own, ahead of any directory allow-listing: even where the agent cannot yet read the stated path, naming it downgrades 'silently reads the wrong user file' to 'visibly cannot read it'.
Channel A: an agent that can run commands reads its skills through a normal tool call instead of the [LOAD_SKILL] text-protocol round trip. Channel B stays as the fallback for agents that cannot run commands at all, and the AGENT picks -- we do not try to predict which, because permission mode is agent-side runtime state that no CLI capability query reveals. Lives in its own domain crate rather than in aionui-extension: runtime-token validation needs aionui-ai-agent, which sits above the extension crate, so putting it there would invert the layering. Deliberately NOT merged with 'config skills *' either -- that surface lists every importable skill and can write, while this one is read-only and scoped to one conversation's snapshot. Merging them would let a conversation-scoped token reach installation-wide authority. The snapshot filter is the security boundary, so the tests lead with it rather than with the happy path: a skill enabled in a SIBLING conversation is refused 403 (not 404 -- a 404 invites retrying with variations), another user's conversation is invisible, and a token minted for a different conversation is rejected even for the same user. Traversal containment canonicalizes both sides. Rejecting '..' lexically is not enough: a symlink inside the skill directory pointing outward looks entirely innocent, and a test builds exactly that. The root is canonicalized too, because the view directory reaches skills THROUGH symlinks -- comparing a canonical target against a raw root would reject every legitimate read. A refusal names only the requested relative shape, never the resolved target, which a test pins. 'list' reads descriptions from the same on-disk SKILL.md that 'show' reads rather than from the DB catalog, so the two commands cannot disagree about what exists -- a catalog read would make 'list' depend on whether a startup sync had run. Frontmatter stripping moved into aionui-extension so this domain and the [LOAD_SKILL] channel return byte-identical bodies. Registering 'skills' in the top-level domain index closes the discovery half of the same defect that left 'session' unlisted despite having a full contract: an unlisted domain is invisible to 'aioncore capabilities', which would silently push every agent onto the fallback channel.
… aionrs an injection path at all Two parts. The second is a gap the design missed. Injection content. Descriptions are truncated to 200 CHARS (not bytes -- a byte slice splits a multi-byte codepoint), which is safe only because channel A exists: an agent seeing a cut description can fetch the full text with 'skills show'. The block now advertises both channels, command first, since that is a normal tool call rather than text-matching plus an extra turn. The index is also sorted, because discovery returns from a HashMap and an unordered block would vary between opens of the same conversation, churning context for nothing. The light/heavy switch now reads skill_delivery instead of native_skills_dirs.is_some(). The old signal conflated 'declares a workspace skills directory' with 'can discover skills natively', so a vendor with a declared directory but no working discovery got light injection and therefore no skills. Both layer-1 modes are light; previously Protocol would have fallen to the heavy branch and given codex a duplicate index. agy and aionrs had NO injection path. Index injection has only ever lived in the ACP prompt pipeline, and neither backend runs it -- verified by grepping every caller. Their skills arrived solely through the workspace links this refactor removes, so without this they would lose skills outright. Worse for agy: its backend consumed only init.mcp_servers, so the assistant's preset context was being dropped too. agy now prepends the composed block to its first -p invocation only (it resumes with --conversation afterwards and would otherwise repeat the block every turn) and finally honours extra_args, which it ignored entirely. aionrs folds the index into system_prompt, its one always-present channel. Both compose through the same function as the ACP hook, so the three paths cannot drift into different wording. Two existing tests pinned the literal '[LOAD_SKILL: skill-name]', which the second channel legitimately rewrote to '<name>'. Rewritten to assert the protocol MARKER plus the new command channel -- the meaningful part of the original check -- rather than deleted.
The destructive half of the refactor, landing only now that every replacement
channel is in place -- so no vendor passes through a state with no skills.
AionUi no longer symlinks skills into {workspace}/.claude/skills and friends.
That path applied to user-selected workspaces too (frequently git repositories),
created directories for the 5 auto-inject defaults even with zero user
configuration, stacked one set per member per vendor in a shared team workspace,
was re-created on the next build if deleted by hand, and degraded into COPYING
real files in when a symlink failed. All of it is gone, including the fallback.
Four G1 regression tests snapshot the workspace file tree before and after and
assert it is unchanged: a user-selected workspace holding the user's OWN
.claude/skills, an auto-provisioned one (the removed code took it as licence to
write, so testing only the selected case would miss half the defect), five
vendors sharing one directory, and a second build. The snapshot walks with
symlink_metadata so it never descends THROUGH a link.
One acknowledged exception, whitelisted by name: .agents/hooks.json, agy's
PreToolUse permission gate. Removing it would silently downgrade agy's security.
Any other new workspace write fails these tests.
agy's slash-command list now comes from the session's resolved skill dirs
instead of scanning {workspace}/.agents/skills, which no longer exists. Strictly
better: it lists exactly this conversation's enabled skills, so a stale residue
cannot reappear in the picker.
write_mcp_config is deleted. Its own header documented a measurement proving agy
does NOT read {workspace}/.agents/mcp_config.json -- the same MCP server is
invoked from the global path and not from the workspace file, where agy logs
'empty component: prompt section mcp_servers' -- and descriptor.rs already
declares agy with no MCP transport. It was writing a file with no consumer, into
the user's workspace.
Three existing tests encoded the removed behaviour. Rewritten, not deleted, per
the test-failure policy: the same scenarios now assert the workspace is untouched
and the view is correct. The link_workspace_skills unit tests went with the
function; among them a Windows case for the copy fallback, a degradation this
change deliberately cancels -- a failed link is now skipped with a warn rather
than materialising real files into a directory we treat as disposable.
A startup sweep reaps view directories whose conversation is gone, keyed by the
(user, conversation) PAIR so one user's deletion cannot reap another's. Listing
failure reaps nothing, which is the safe direction: a leaked view costs disk, a
wrongly-deleted one costs a session its skills.
Also measured, and it changed the config: agy gets NO directory allow-listing.
Our argv always passes --dangerously-skip-permissions, and a live probe under
exactly that argv read a file outside the cwd with its directory not
allow-listed. Declaring the flag would add one argument per skill for no effect
and would read as a guarantee the measurement contradicts.
AgentType::native_skills_dirs() is retired from delivery but kept as reference
data, matching the column's own fate -- with a comment against reintroducing it
as a signal, since conflating 'declares a directory' with 'can discover skills'
is how a vendor could end up with neither.
…migration A full app-server turn probe (initialize -> extraRoots/set -> thread/start -> turn/start) had the agent read a symlinked skill's references/ file from outside the thread cwd under sandbox: workspace-write, and it succeeded, reaching the real source path. So codex needs no directory allow-listing for reads, unlike claude -- which is why its config carries none. Recorded with its caveat: the probe ran with approvalPolicy: never, so under the on-request default an out-of-cwd read may surface an approval request rather than being refused. That is the same behaviour claude has and AionUi already renders a permission card for it.
Conflicts, both in aionui-app: * cmd_capabilities.rs `domains` -- each side added one entry (main added `session`, this branch added `skills`). Kept BOTH. * cmd_capabilities.rs test module -- took MAIN's side. Its structural invariants (walk the CLI subcommands against the index in both directions, plus per-domain contract-string agreement) subsume the name-by-name assertions this branch had, and cover `skills` automatically. Added `skills` to its contract loop, and kept the one property it does not check: a read-only domain must not advertise write authority. * router/state.rs imports -- one new import per side, kept both. Migration renumbered 042 -> 043: main shipped 042_add_encryption_secret.sql (#917) and has been released, so this branch's migration moves. Reconciled against what main changed underneath: * #930 slimmed the auto-inject descriptions; all five are now <= 200 chars (max 194, aionui-config down from 687), so they land inside this branch's truncation budget and the truncation is pure backstop as designed. * #931 bumped the codebuddy pin to 2.138.0. Re-probed `--help` at the new pin: `--plugin-dir <dirs...>` and `--add-dir <directories...>` unchanged, so the vendor comment now cites the version we actually spawn. * #929 registered `session` in the capability index, which is the sibling change this branch's test comment pointed at. That pointer is gone with the adopted test module.
`rebuild_view` had no synchronization, and a first turn calls it three
times -- conversation create, runtime ensure, and send -- with the last
two landing under a millisecond apart. Because the rebuild is "snapshot
the link names, wipe the tree, relink", concurrent calls interleave
destructively.
Observed against a real backend, four distinct symptoms of the one race:
* a spurious "skills added to a conversation that already had a view"
warning, computed against a snapshot taken mid-wipe;
* four `File exists (os error 17)` link failures plus two
`skills 2 / requested 4` lines on a view that in fact held all four,
i.e. the reported counts were wrong;
* `sync_skill_view failed ... Directory not empty (os error 66)` at
ERROR level, when one call's `remove_dir_all` hit a directory the
other was repopulating -- failing the whole rebuild;
* that failure left a conversation's view holding 3 of its 4 enabled
skills, so its agent could not see the fourth for over two hours,
until an unrelated later rebuild happened to restore the link.
The benign outcomes were timing luck, not a property of the code.
Two changes:
* serialize per (user_id, conversation_id), so unrelated conversations
still rebuild concurrently. Unheld entries are dropped on each
acquisition so the registry cannot grow with every conversation the
process has opened, and ids are validated before a lock entry can be
registered.
* return early when the view already matches the snapshot. This is the
common case -- two of a turn's three rebuilds ask for the set the
first just linked -- and skipping them also stops briefly emptying a
directory a CLI may be reading.
The count now reports what the view holds rather than what one call
personally created, and the log separates "requested" from "linkable" so
a missing source directory can no longer read as a short link count.
Sequential tests cannot reach this: the destructive interleaving needs
one call's wipe to land inside another's relink loop. The new test drives
eight concurrent rebuilds of the same conversation and asserts on the
result rather than on timing -- every call reports the full set, and the
final view holds exactly the requested skills. Removing the lock makes it
fail (1 of 6 linked).
The injected skills index told agents to run `skills show <name>` and
`skills cat <name>/<relative-path>`, but both subcommands are declared as
argument-less variants that read a JSON object on stdin, so every taught
form was rejected outright:
$ aioncore skills show mermaid
error: unexpected argument 'mermaid' found
Reaching for `--help` did not help either: it printed `Usage: aioncore
skills show` and said nothing about stdin, so the obvious self-service
path was a dead end too. Measured on live agents, three of them across
two backends: aionrs burned three failed tool calls and tripped its
tool-recovery guard; opencode burned five, recovering only after it
happened to try `skills capabilities`. `skills list` masked the problem
because it takes no arguments.
This is not model capability. The same agents got `session send-message`
right on the first attempt every time, because that command's contract is
documented correctly in its skill body. Wrong docs cost three to five
rounds; correct docs cost none.
Three changes, one per gap found:
* the index now teaches the stdin form and points at
`skills capabilities` as the authoritative contract;
* `Show` and `Cat` document the stdin payload and a runnable example, so
`--help` is a working path rather than a dead end. The rationale stays
in a plain comment -- help text an agent reads should be purely
instructional;
* a test extracts every `skills …` command line the index teaches and
asserts clap accepts it, and asserts `skills show <name>` stays a
parse error. Nothing structural stopped the two crates from
disagreeing before: the text side only asserted the string mentioned
`$AIONUI_HELPER_BIN`, which the broken wording satisfied.
Restoring the old wording makes the new test fail.
The ACP and Antigravity lanes resolve skill delivery through `resolve_skill_delivery` and emit a "resolved delivery plan for session" anchor log; aionrs does neither, and a maintainer grepping for that log while debugging an aionrs session would find the gap and reasonably suspect a bug. It is deliberate. aionrs is an in-process agent compiled into aioncore, with no launch-argument channel (argv) and no wire protocol (protocol), so `system_prompt` injection is the only delivery it can physically perform. Its `skill_delivery` column has no applicable value other than its `injected` default, so routing it through the shared decision would fetch a metadata row and compute a plan this lane cannot use, to honor modes it cannot execute. Comment only -- no behavior change. Records the decision where the next reader will look, and marks the spot aionrs would re-join the shared path if it ever gains a real native skill channel.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reworks how a conversation's skills are delivered to an agent CLI. Previously AionUi materialized skill symlinks (
.claude/skills/and friends) inside the conversation's working directory, which may be a user's git repository. This branch moves the symlink landing site into an AionUi-owned directory and delivers it to each vendor through the most native mechanism that vendor supports, falling back to prompt injection.What changed
Skill view directory (AionUi-owned). Skill symlinks now land under an AionUi-managed per-conversation directory instead of the working directory. Because AionUi owns this tree, rebuilds are an exact match against the enabled-skill snapshot (a dropped skill actually disappears) and cleanup is safe. There is deliberately no copy fallback: a failed link is skipped with a warning rather than materializing user files into a disposable directory.
Two-layer, per-vendor delivery. A new
skill_deliverycolumn onagent_metadata(JSON, migration043) declares how each vendor receives skills:argvhands the view directory to the CLI via launch arguments (claude--plugin-dir+--add-dir);protocolregisters it over the wire (codexskills/extraRoots/set).NULLcolumn reads as, so an unprobed vendor is zero-intrusion by default and needs no migration to work.The column intentionally has no CHECK constraint: shipping a new vendor capability should be a data change, not a migration + release. Validation lives in the application layer and is tolerant — an unknown mode warns and degrades to
injected.Two on-demand load channels for injected-mode vendors:
aioncore skillsCLI (list/show/cat/capabilities) scoped to the current conversation, authenticated with a session runtime token. A normal tool call instead of a text round-trip.[LOAD_SKILL: <name>]text protocol as a fallback for agents that cannot execute commands; the requested body is provided on the next turn.Injection path for the non-ACP backends. The two backends without a prompt pipeline (Antigravity, aionrs) previously received skills only through workspace links; with those removed they gained a dedicated injection path so they keep their skills (and, for Antigravity, their assistant preset context, which was being dropped entirely before).
Breaking / behavior changes (call out in release notes):
native_skills_dirsno longer affects delivery (kept as historical metadata; such agents fall toinjected, skills still available).aionui:prefix.Fixes made during dev-environment verification
Three defects were found while exercising the branch against real agent CLIs and fixed here — none were caught by the pre-existing unit suite because they only surface under real concurrency / real agent behavior:
fix(skills): serialize the session skill view rebuild per conversation— the view rebuild had no synchronization, and a first turn rebuilds it three times within milliseconds. Concurrent rebuilds interleaved destructively (observed: spurious "skills added" warnings, wrong link counts, anENOTEMPTYfailure, and one session left missing a skill for hours). Now serialized per conversation with an idempotent early-return when the view already matches. Regression test drives concurrent rebuilds and asserts on the converged result.fix(skills): teach channel A the invocation the CLI actually accepts— the injected index taughtskills show <name>, but the subcommands read a JSON object from stdin and reject positional arguments;--helpalso omitted the stdin contract. Live agents burned several failed tool calls recovering. Corrected the injected text and--help, and added a cross-crate test that parses every command line the index teaches.docs(skills): record why aionrs stays off the shared delivery decision— comment-only: documents that the in-process agent isinjected-only by architecture, so it deliberately does not route through the shared delivery decision.Verification (dev environment, real agent CLIs)
Per-vendor behavior was first probed against specific real CLI versions (recorded in the migration comments — e.g. the repeatable
--plugin-dir/--add-dirpaired probe for claude, the process-scopedextraRootsschema for codex, the permission-mode measurement for Antigravity). This branch then added live end-to-end verification, in two phases, with evidence checked in logs / DB / prompt dumps rather than by assertion.Phase 1 — live run of the original branch (defects surfaced). Exercising the five delivery modes against real CLIs surfaced two defects that the unit suite could not catch because they only appear under real concurrency / real agent behavior:
ENOTEMPTYfailure, one session left missing a skill);Both were fixed in this branch (see the fixes section above).
Phase 2 — end-to-end verification on the post-fix build (the state this PR ships). After rebuilding with all three fixes and confirming the running binary contained them, the full set was re-run:
argv, codex →protocol, Antigravity / aionrs / opencode →injected; migration applied cleanly and the per-vendor rows are correct.list/show) works end-to-end.[LOAD_SKILL]request is detected and the skill body is injected on the next turn; the injected body is anchored with the skill's absolute directory, so its relative references resolve to the real skill files — verified against a deliberately planted same-named file in the working directory, which was not read.Not covered: codebuddy (its
--plugin-dirvisibility could not be confirmed without an authenticated account, so it is deliberately kept oninjected); the thinking-block variant of Channel B detection on a backend that inlines think tags (this backend surfaces reasoning on a separate channel, so that path is covered by an integration test rather than live).Test plan
cargo testfor the touched crates (extension / ai-agent / app / conversation) — greencargo clippy -- -D warningson the touched crates — cleancargo fmt --all -- --check— cleancargo test --workspacevia the pre-push gate