feat(ui): Skills management surface — unhide and rebuild the Skills tab (ent#235) - #1877
Conversation
…ab (ent#235) The skills machinery shipped across three planes (#182 distribute/place/expose, #183 package injection with a per-skill result contract) and nothing rendered any of it. The Agent Detail Skills tab was excluded from `visibleTabs` per requirements §22.2 ("component preserved for potential admin-only access"), assignment was REST/MCP-only (§21.3), and #183's statuses and named warnings had no consumer at all. A user could not browse the library, see what an agent had, or assign anything. What lands: * **Tab unhidden** for owners/admins on non-system agents, matching the other management tabs. `OverflowTabs` absorbs it. * **`stores/skills.js`** — a domain store (Invariant #6). The old panel called `axios` directly with a hand-built auth header, silently bypassing the shared client every other call relies on; everything now goes through `api` (Invariant #7). * **Library browse** with the §21.6 contract surfaced: description, automation, `user_invocable`, declared `requires` (binaries/packages/env), multi-file file count, size, and the git tree SHA as version. Dependencies are shown BEFORE assignment, because they are what later becomes a `missing_binary:*` warning. * **Assignment** with bulk save through the existing `PUT .../skills`, plus a dirty/reset affordance so a half-made selection is recoverable. * **Honest injection status.** This is the load-bearing part. #183 reports `injected | unchanged | fallback | failed` with named warnings; the panel renders the verdict per skill and translates the tokens into what they mean for THIS agent ("`jq` is not installed in this agent — the skill may not run"). `fallback` renders as "partial" in amber, never a green tick — an explicit AC. Injection results are kept separate from assignment in the store precisely so a durable assignment cannot be painted with a stale success. * **Manual sync** (`force=True` repair action) with in-flight state, and a 409 from `SkillInjectionBusy` reported as "already running" rather than a generic failure. * **No dead empty states** — the store computes one discriminator (`library_unconfigured` / `library_empty` / `none_assigned`) so the panel cannot invent a fourth. Unconfigured routes an admin to Settings and tells a non-admin to ask one. * **Stopped agent** renders persisted assignment state with Sync disabled and the reason in the tooltip, rather than offering an action that would fail. Verified against the live instance: tab appears, 3-skill library renders with contract fields, bulk assign persists, agent started, "Sync now" returns `{haiku: injected/2 files, word-count: injected/2 files}` and the badges + last-sync line render from that response. Gating confirmed OSS-core with the issue author before building: every file here is already public, the endpoints are ungated, and the paid piece (skill_runner, ent#139) plus exposure curation (#178) are both explicitly out of scope. Related to trinity-enterprise#235
…t#235 review) Self-review of #1877 found two defects, both in the "no dead empty states" AC this panel exists to satisfy. 1) The "Configure the library" CTA linked to `/settings?tab=skills`. There is no such tab — the Skills Library config lives under Settings → **agents** (`Settings.vue`, `v-if="activeTab === 'agents'"`). So the one call-to-action offered to an admin staring at an unconfigured library went nowhere. I also asserted in the PR body that the Settings panel already reported sync status / last-synced / skill count without checking; it does report all three — but I had the tab wrong, which is what checking would have caught. 2) `api.get('/api/skills/library').catch(() => ({ data: [] }))` swallowed every error, not just the unconfigured case: a 500, a timeout or an auth failure all rendered as "the library is configured but has no skills yet" — a confident, wrong empty state that points the operator at the wrong problem. The list is now fetched only when `status.configured` is true, so the known empty state comes from the status read and any other failure surfaces as one. Related to trinity-enterprise#235
/review — self-reviewBranch: Scope: CLEAN. One commit, one issue. All 9 ACs traced; the two that were not actually met are below. (A Critical — both fixed in
|
vybe
left a comment
There was a problem hiding this comment.
Skills management surface (ent#235): frontend-only (SkillsPanel + skills store + AgentDetail tab wiring), no backend change. Live-probe verification in PR comments covers browse/assign/sync end-to-end. Security greps clean.
|
Heads-up from ent#237 (multi-source skills library) so we don't collide, @dolho — I have deliberately not touched Two things that are now available for the rebuild whenever it suits you (backend already shipped on
{
"name": "pdf-export",
"source_name": "Acme internal skills", // which repo it came from
"shadowed_by": [ // lower-precedence repos ALSO shipping this name
{ "source_id": "src_…", "source_name": "Trinity Community Skills" }
]
}
Also FYI: I've split the admin side into No action needed to merge this PR; I'll rebase ent#237 onto it. |
…endpoint rot sweep (ent#263) - requirements/core-agent.md: new §4.5 Library Page — unified /library surface (agent templates + fleet skills browse), query+hash-preserving /templates redirect, stacked sections, per-kind empty states, the AC#4 page-identity naming rule; fleet assignment visibility named as Not Built - requirements/skills.md (surgical — §21.3/§22.2/new §22.3 only, avoiding PR #1901's §21.1/§21.5 hunks): §21.3 stale 'Skills tab is hidden' note corrected (visible since ent#235/PR #1877); §22.2 rewritten as visible/rebuilt; new §22.3 Library Page fleet skills browse — browse-only over the existing /api/skills/library reads, own skillsLibrary store + the KeepAlive rationale, admin-only URL/Sync, #1901 forward-compat, assignment read = Not Built - architecture.md: 'Top-nav IA — Library (ent#263)' paragraph beside the #1109 Operations one; stale 'Templates (4 endpoints)' table corrected to the 2 real routes (POST /refresh AND GET /env-template both verified absent) - feature-flows: templates-page.md git-mv'd to library-page.md + full rewrite (the old file was deeply stale — AgentSubNav, dead endpoints); index row + platform-settings.md Related-Flows link repointed - template-processing.md + CREDENTIAL_MANAGEMENT.md: dead env-template endpoint references removed/replaced (same rot class as the architecture table); Templates.vue references repointed at Library.vue - user docs (creating-agents.md, faq/agents.md): Templates page → Library (+ redirect note) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the single URL+branch form with `components/SkillSourcesPanel.vue` backed by `stores/skillSources.js`, and strips the now-dead single-repo state and methods out of Settings.vue (~130 lines). Extracted to a component rather than grown inline: Settings.vue was already 3800+ lines, and per-source rows, an add form, and error surfacing would have added meaningfully to that. The store is a SEPARATE domain from `stores/skills.js` on purpose. That store owns per-agent skill assignment (an owner surface); this one owns which repositories the platform syncs from (an admin surface, and the grant action of requirements §21.1.2). They share no state — and keeping them apart also avoids colliding with the Skills-tab rebuild in ent#235 / PR #1877, which is rewriting stores/skills.js right now. UI decisions that carry the design rather than decorate it: * the list renders in backend RESOLUTION order and the first row is badged "wins conflicts". The store never re-sorts — ordering is a backend contract, and a client-side sort would silently misreport which source actually wins * `ref_type` shows as "pinned" vs "branch" with a tooltip explaining that a pinned tag which moves is REFUSED. That is the supply-chain posture (§21.1.2), so it belongs on the row, not buried in an edit form * the add form warns when to pin: "when you don't fully control who can merge" * backend errors surface VERBATIM. A refused moved tag names the tag and says to point at a new one; a generic "sync failed" would throw exactly that away * the shadowed-skills count gets its own banner — non-zero means someone is running a different source's version of a skill than intended (AC#4) * Remove says explicitly that agents keep installed skills and assignments are not removed, so it can't read as "strip these skills from my agents" * a failed fetch leaves the list untouched rather than clearing it — blanking would read as "no sources configured", a different and alarming claim * the add form stays filled on failure so a bad URL is corrected, not retyped Verified with a real `vite build`. NOT included: the per-skill source badge + shadow warning in SkillsPanel.vue. That file is being rewritten in PR #1877 (dolho), so editing it here would hand him a conflict. The backend already exposes `source_name`/`shadowed_by` on GET /api/skills/library, so it is a small additive change once #1877 lands. Refs Abilityai/trinity-enterprise#237
Implements
trinity-enterprise#235. OSS-core, ungated — confirmed with the issue author before building: every file here is already public, the endpoints are already ungated, and the paid piece (skill_runner, ent#139) plus exposure curation (#178) are both explicitly out of scope.The gap
The skills machinery shipped across three planes (#182 distribute/place/expose, #183 package injection with a per-skill result contract) and nothing rendered any of it. The tab was excluded from
visibleTabsper requirements §22.2 ("component preserved for potential admin-only access"), assignment was REST/MCP-only (§21.3), and #183's statuses and named warnings had no consumer. A user could not browse the library, see what an agent had, or assign anything.What lands
Tab unhidden for owners/admins on non-system agents, matching the other management tabs;
OverflowTabsabsorbs it.stores/skills.js— a domain store (Invariant #6). Worth flagging: the old panel calledaxiosdirectly with a hand-built auth header, silently bypassing the shared client every other call relies on. Everything now goes throughapi(Invariant #7).Library browse with the §21.6 contract: description, automation,
user_invocable, declaredrequires, multi-file count, size, and the git tree SHA as version. Dependencies are shown before assignment, because they're exactly what later becomes amissing_binary:*warning.Assignment via the existing
PUT .../skillswith bulk save, plus dirty/reset so a half-made selection is recoverable.Honest injection status — the load-bearing part. #183 reports
injected | unchanged | fallback | failedwith named warnings. The panel renders the verdict per skill and translates tokens into what they mean for this agent (missing_binary:jq→ "jqis not installed in this agent — the skill may not run").fallbackrenders as "partial" in amber, never a green tick — an explicit AC. Injection results are held separately from assignment in the store precisely so durable assignment state can't be painted with a stale success.Manual sync (
force=Truerepair action) with in-flight state; a 409 fromSkillInjectionBusyis reported as "already running", not a generic failure.No dead empty states — the store computes one discriminator (
library_unconfigured/library_empty/none_assigned) so the panel can't invent a fourth. Unconfigured routes an admin to Settings and tells a non-admin to ask one.Stopped agent renders persisted assignment with Sync disabled and the reason in the tooltip, rather than offering an action that would fail.
Verified against the live instance
Not just rendered — exercised:
The stopped-agent path was confirmed first (Sync disabled with tooltip), then the running path after starting the agent.
Not in scope
Exposure curation (#178) — what an agent advertises outward — is deliberately untouched, per the issue. The Settings library panel already reports sync status/last-synced/skill count, so it needed no change; say the word if you'd like it extended further.