Skip to content

Feat/capability abstraction - #43536

Draft
neriousy wants to merge 2 commits into
v2from
feat/capability-abstraction
Draft

Feat/capability abstraction#43536
neriousy wants to merge 2 commits into
v2from
feat/capability-abstraction

Conversation

@neriousy

@neriousy neriousy commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Issue for this PR

Closes #

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

Adds a global capability-preference abstraction, initially used only for skills. This keeps mutable user preferences out of Skill.Info and leaves permissions, runtime health, and visibility as separate concerns.

A capability is identified by a typed reference. The only supported kind in this PR is:

{ kind: "skill", key: ["effect"] }

Explicit enabled and disabled preferences are stored through the shared KV service under capability:preferences. inherit removes the explicit preference and restores the skill default, which is derived from autoinvoke.

The capability API returns the default state, explicit preference, and effective state separately. The Skills dialog uses that API, while SkillInstructions applies the effective state when building automatic skill guidance. Explicit @skill references remain available even when automatic discovery is disabled.

To add another capability kind later:

  1. Extend Capability.Kind and add a canonical reference constructor such as Capability.tool(name).
  2. Add that domain's inventory to the capability API projection.
  3. Call capability.resolve(ref, defaultState) at the domain's real availability boundary.
  4. Build a domain-specific dialog over the shared preference API.

The preference service only resolves desired activation. Each domain still owns what disabling means.

How did you verify your code works?

  • Ran Core, Server, Client, and TUI typechecks.
  • Ran capability persistence and skill-instruction tests.
  • Ran the existing skill prompt and session tests.
  • Regenerated the Effect and Promise clients.

Screenshots / recordings

Not included.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

If you do not follow this template your PR will be automatically rejected.

@neriousy
neriousy changed the base branch from dev to v2 August 19, 2026 22:47
@neriousy
neriousy force-pushed the feat/capability-abstraction branch from b5c42b3 to 0003689 Compare August 19, 2026 23:08
@neriousy
neriousy changed the base branch from v2 to feat/proper-skill-ux August 19, 2026 23:09
Base automatically changed from feat/proper-skill-ux to v2 August 19, 2026 23:28
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

  1. packages/core/src/capability.ts:41 — load() deletes the whole preferences key whenever decoding fails (kv.remove(Key)) — a schema evolution or partially-written value then silently wipes every user preference on first read, irreversibly — don't destroy data you can't parse: log + ignore, or store a schema version alongside so migrations are possible.

  2. packages/core/src/capability.ts:60 — set() does load → filter → write with no locking or serialization; two concurrent updates (two TUI clients, or rapid toggles from different surfaces than this dialog) lose one preference — wrap the read-modify-write in an Effect semaphore/single-flight like other config writers, since KV isn't transactional here.

  3. packages/core/src/session/runner/to-llm-message.ts:227 + compaction.ts:141 — dropping skills[].text from message lowering and compaction means pre-existing sessions recorded with inline skill guidance lose that context when resumed or compacted after upgrade — fine as a forward-looking design, but worth a release note ("old sessions degrade to mention-only") and ideally a compaction fallback that re-resolves attachments by id if still installed.

  4. packages/server/src/handlers/capability.ts:14 — capability.list calls capability.get(ref) per skill, and each get re-reads and re-decodes the entire preferences array from KV (O(N) reads per request) — load once and match locally, mirroring what the service already exposes via list().

  5. packages/core/test/capability.test.ts:8 — coverage of the happy path and inherit semantics is good, but the risky branches are untested: the corrupted-payload removal from item 1, concurrent set calls (item 2), and the new HTTP handler itself (list/update round-trip) — please pin down at least the first two before merge.

  6. Nit — packages/tui/src/component/dialog-skill.tsx:47 — toggle derives the wire state as preference ?? "inherit" while optimistically rendering state; if another client flips defaultState between render and update, "inherit" lands on a different effective value than the checkbox showed — acceptable for a single-user dialog, just be aware the optimistic row can disagree until refresh.

Overall: nice abstraction — the Ref/Preference/Info split, event emission, and the move to on-demand skill loading via the tool are clean, and the TUI dialog rewrite (optimistic toggle + revert + search) is a clear UX win. Items 1–2 are the ones I'd fix pre-merge. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants