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
…per-instance custom repos (Abilityai/trinity-enterprise#237) (#1901) * feat(skills): skill_sources data model + per-assignment source (ent#237) Replaces the single `skills_library_url` setting with a `skill_sources` table: one row per git repo the library syncs from, so an install can carry the bundled community catalog AND its own repo(s) at once. Two AC-flagged design decisions are encoded here: * Custom-wins precedence, bare names (AC#4). Resolution is `priority` ASC then `created_at` ASC; custom sources default to 100 and the bundled source to 1000. Names stay bare because the agent-side identity IS the directory `.claude/skills/<name>/`, and both the ent#139 runner and the ent#178 A2A card resolve by bare name — prefixing would change every agent's invocation string and require a fleet-wide re-inject. `agent_skills.source_id` RECORDS which source a name resolved to (so a cross-source swap is detectable) but is deliberately not part of the UNIQUE, which would permit two rows that cannot coexist on disk. * Branch-vs-tag refs (AC#5). `ref`/`ref_type` let the bundled source pin to a tag while custom sources track a branch. At most one default source, enforced by a partial-unique index rather than a read-then-write check, so a concurrent second worker loses at the DB. `is_default` is immutable: promoting a custom source would change its trust posture without changing where it points. Deleting a source does not cascade to assignments — the skill keeps resolving by bare name through whatever source still provides it, and cascading would silently unassign skills that are still available. Adopting an existing `skills_library_url` is NOT done in the migration: the legacy clone at /data/skills-library/ must move into a per-source subdir in the same operation, so it belongs in skill_service where both halves succeed or fail together. Dual-track per invariant #9: SQLite `skill_sources_table` + Alembic 0031_skill_sources, with schema.py DDL and tables.py MetaData kept consistent. Refs Abilityai/trinity-enterprise#237 * feat(skills): per-source clone with tag pinning (ent#237) Extracts one source's git lifecycle into `SkillSourceClone` so the library can hold N checkouts, one per `skill_sources` row, under /data/skills-library/. Behaviour for a branch source is the old single-clone path unchanged. The addition is tag pinning (AC#5). Skills carry executable `scripts/` (ent#183) that the ent#139 runner executes, and ent#236 makes syncing automatic — so a branch-tracking source puts every merged upstream commit on every install with no human in the loop. The bundled community source pins to a tag; custom sources, whose write access the operator controls, keep tracking a branch. A pin is only worth as much as the tag's immutability, which git does not enforce, so a tag resolving to a different commit than the last sync is refused as `moved_tag` rather than adopted. Two independent mechanisms: the fetch deliberately omits `--force` (git then refuses to clobber a moved tag ref), and an explicit SHA comparison catches the same condition on a fresh clone where no local ref exists to conflict. Moving to new content is done by pointing the source at a new tag NAME — an explicit admin action. Source ids and refs become argv and directory names, so both are regex-gated at construction rather than trusted from the DB: this blocks traversal via either, and `-`-leading refs that would smuggle a git option. The realpath containment check is now per-clone, closing an escape route that did not exist with a single checkout (a symlink resolving into a *different* source's tree). tests/unit/test_ent237_skill_sources.py builds the actual attack — upstream force-moves a tag onto a commit adding a payload — and asserts the payload never reaches disk, not merely that the call returned an error. Verified by mutation: neutering the pin fails that test and only that test. Refs Abilityai/trinity-enterprise#237 * feat(skills): merge skills across sources, custom-wins, shadow-reported (ent#237) Makes the library actually multi-source. `skill_service` now orchestrates N `SkillSourceClone`s instead of being one clone: sync, list, get, and inject all resolve through the precedence order from `skill_sources`. Resolution (AC#4): the first source offering a name owns it, and every lower-precedence source shipping that name is recorded in `shadowed_by`. The record is the point — a bare "first wins" with no trace is exactly the silent overwrite AC#4 forbids. The shadowed copy is deliberately NOT a second list entry: it is unreachable, and offering it would imply a choice the flat `.claude/skills/<name>/` namespace cannot honour. Injection reports it too (`shadowed_source:<name>` warning), so "I assigned Community's copy and got Acme's" can't happen quietly. One source failing never blinds the others: per-source sync outcomes are reported individually and the aggregate succeeds if any source synced. With several repos configured some will be broken at any moment, so this is the normal case, not the edge. `_git` in the clone now tolerates a missing directory. A source whose first clone failed has none, and `subprocess.run(cwd=<missing>)` raises FileNotFoundError — which escaped through `current_commit` into the list-cache fingerprint and took down the whole merged listing over ONE unreachable repo. Caught by test_one_broken_source_does_not_blind_the_others. Other correctness points: * the list cache is keyed on a fingerprint over EVERY enabled source's commit, not one SHA — otherwise adding/disabling/re-syncing a second source serves a stale merged list indefinitely * injection resolves ONCE up front; re-resolving per skill would let a concurrent admin sync inject a half-and-half set * the agent-side version marker records the OWNING source's commit plus its source_id — with N sources there is no single library commit, and stamping the wrong one makes the record unauditable * `_skill_files`/`_parse_skill_info` take the clone explicitly rather than re-resolving, so describing a non-winning copy can't silently describe a different source's files * realpath containment moved into the clone, applied per-source — a shared root would let a symlink in one source resolve into another's checkout and pass Legacy adoption (AC#6, kept per vybe): an existing `skills_library_url` becomes a regular CUSTOM source — custom, not default, so precedence keeps preferring the repo the operator actually chose. DB row is written before the clone moves, so a crash between them leaves a source that simply re-clones; the reverse order would strand a checkout no row points at. Idempotent and fail-soft. `get_library_status` reports per source (with each one's WON skill count, the number that matters when sources overlap) and retains the legacy url/branch/commit_sha fields so the pre-ent#237 MCP tool and Settings panel keep rendering until they migrate. 11 new tests covering the merge, precedence handover on disable, cache invalidation, broken-source isolation, and all three adoption properties. Refs Abilityai/trinity-enterprise#237 * feat(skills): seed the bundled community source on fresh installs (ent#237 AC#3) A fresh install now starts with the community catalog configured, so the library is never empty out of the box. Seeded as a ROW, not resolved as a code default at read time. That distinction is the design, and it mirrors the #1638 retention-floor lesson: a read-time default is re-applied on every boot, so it would resurrect a source the admin deliberately deleted and would silently hand an EXISTING install a source it never configured. A row can be deleted or disabled and stay that way. Fresh-install detection reuses #1638's signal — `users` is empty for exactly one moment in a database's life — so the seed runs in the same window and, like the retention seed, must run BEFORE _ensure_admin_user (which is what makes `users` non-empty). * ref_type is `tag`, per AC#5: the community catalog takes PRs from strangers and its skills carry executables the ent#139 runner runs, so instances follow a tag we bump, never the branch head. * priority is DEFAULT_SOURCE_PRIORITY (highest number = lowest precedence), so any custom source added later wins a name collision with no reordering (AC#4). * INSERT OR IGNORE against the partial-unique default index: both migration locks fail open, so two workers can race this and must not produce two defaults. * TRINITY_DEFAULT_SKILL_SOURCE="" disables it entirely, for an operator who wants no community catalog (mirrors TRINITY_DEFAULT_SYSTEM_MANIFEST). * Never raises. `init_database` runs at import, so a raising seed would crash-loop boot permanently; a skipped seed just leaves an empty library, which is the pre-ent#237 status quo. Both dialects, as invariant #9 requires: the sqlite cursor path and the engine-based path for PostgreSQL, sharing one `_default_skill_source_values` so the seeded row cannot drift between them. The repo it points at is ent#296 and does not exist yet, so until that lands a fresh install reports one failed source rather than a populated catalog — fail-soft by design, `sync_library` never raises. Refs Abilityai/trinity-enterprise#237 * feat(skills): source-management endpoints, human-only on every mutation (ent#237) Adds the REST surface for multi-source: list, create, update, delete, and per-source sync, plus the SkillSourceCreate/SkillSourceUpdate models in models.py (invariant #14). Every MUTATING route carries `reject_agent_principal` in ADDITION to `require_admin`, and that is load-bearing, not padding. `require_admin` answers "what role", never "is this a human": an agent-scoped MCP key resolves to its owner CARRYING the owner's role, so on a default admin-owned install every agent's injected TRINITY_MCP_API_KEY satisfies it (ent#293 — the third occurrence after trinity-ops-agent#232 → #1644 → #1816). Registering a source is the GRANT action from the learnings.md grant-vs-use distinction: it decides which repo the fleet executes code from, and skills are instructions Claude follows. A prompt-injected agent able to add its own source would get unattended, fleet-wide, persistent prompt injection — the exact chain ent#293 documents, reopened through a new door. ent#237 removes that issue's step-1 target (`skills_library_url`), so shipping these routes with only `assert_admin` would have re-created the hole it closes. Reading and syncing an already-configured source is USE, so those stay role-gated only. Other boundary decisions: * `is_default` is absent from BOTH models — a caller must not be able to claim the bundled source's trust posture (tag-pinned, ours to bump) for an arbitrary repo. The db layer refuses it too, so adding the field back here cannot silently start working. * the SSRF allowlist (#179) runs on write, so a bad URL is rejected at the boundary instead of surfacing later as a recurring sync failure * distinct 409s for duplicate-(url,ref) vs default-already-exists, so the UI can say which * audit is best-effort — a logging failure must not undo a completed write * list is admin-only because the rows carry repo URLs, which for a private source are themselves sensitive; the per-agent Skills tab gets `source_name` from GET /skills/library instead, which exposes no URLs Tests pin the gate statically (an integration test needs a live app + DB, and this must fail the moment a new mutating route lands without it) plus a guards-the-guard test that fails on any unreviewed POST/PUT/DELETE under /skills/sources. Mutation-verified: dropping the call from one handler fails exactly one test. Also fixes a leaky test: the adoption tests assigned `ss.get_skills_library_url` directly instead of via monkeypatch. Since skill_service imports those getters by value, the assignment persisted for the whole session and made every later sync adopt the fixture repo as an extra source — surfacing as unrelated failures once random ordering moved them. 46 tests now pass on three consecutive randomized orderings. Refs Abilityai/trinity-enterprise#237 * feat(skills): surface source provenance on the REST + MCP surfaces (ent#237) Threads `source_id`, `source_name` and `shadowed_by` out to the two consumer surfaces, and documents the multi-source model in requirements. `SkillInfo` is constructed field-by-field in the router, so a new service-layer field is invisible over REST until it is named there — the three provenance fields are spelled out explicitly for that reason. Same in the MCP tool: the list output now carries `source` and `shadowed_by`. `source_name` only, never the URL, on both surfaces. `list_skills` is reachable by agent-scoped keys, and a private source's repo URL is itself sensitive — the URLs stay on the admin-only `GET /skills/sources`. Source *management* is deliberately REST-only and not an MCP tool at all: it is the grant action, and the whole point of the ent#293 gate is that an agent principal must not reach it. `SkillsLibraryStatus` in the MCP types gains the `sources` array; the flat url/branch/commit_sha fields are retained (reflecting the first source in resolution order) so the existing tool and Settings panel keep rendering until they migrate. Typechecked with tsc --noEmit. requirements/skills.md §21.1 rewritten for multi-source with three new subsections — 21.1.1 custom-wins resolution, 21.1.2 the tag-pinning supply-chain posture and the grant/use auth boundary, 21.1.3 migration + fresh-install seeding — each recording WHY, since every one of those was a decision with a rejected alternative. §21.1 also now records the AC#7 outcome (all OSS-core) and the honest Not-Built list, including the fixed `.claude/skills/` layout convention: a repo shaped differently syncs to zero skills. Refs Abilityai/trinity-enterprise#237 * feat(skills): Settings panel becomes a source list (ent#237) 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 * test(skills): retarget the pre-ent#183 skills tests to the multi-source API Fixes 27 regressions this branch introduced. My new ent#237 tests passing said nothing about the existing ones: they exercise the single-clone API that moved onto SkillSourceClone (`_git_clone`, `_git_pull`, `_get_current_commit`, `_git_tree_shas`, `_git_archive_skill`) or whose signature gained the owning clone (`_parse_skill_info`, `_skill_files`). No production behaviour changed here — the properties these tests pin are all still true, just relocated: * test_ent183_skill_packages: the shared fixture now wires exactly ONE source (still a valid configuration), so all 22 injection/listing assertions stand unchanged. The git seams are mocked on the clone instead of the service. * test_skill_service_user_agent: the #184 User-Agent property is pinned on SkillSourceClone, where the subprocess calls now live. `_clone(exists=True)` creates the directory because `_git` short-circuits to a synthetic failure when the clone is absent. * the containment guard moved with it. Note `service._skill_dir` now also requires the skill to EXIST in a source, so the pure path-safety property is pinned on `clone.skill_dir` to keep it independent of existence — otherwise "does the regex-less containment check hold" would silently become "does this skill exist". Two ordering bugs surfaced while fixing this, both mine, both order-dependent and therefore worth removing rather than working around: 1. test_ent183 installs sys.modules stubs at IMPORT time, which permanently binds `skill_service.db` to a MagicMock for the session. My tests depended on that singleton, so they passed or failed based on which files ran first. They now inject an explicit `_SourcesFacade`, removing the ordering question. 2. the same stubs make `get_skills_library_url()` return a real-looking URL, so every sync in my fixture also adopted a phantom legacy source and inflated the expected counts. The fixture now pins that getter to None; the adoption tests opt in via `_fake_legacy_setting`. Also switched the two static auth guards from `inspect.getsource(module)` to reading the router file off disk: importing the module can be affected by another file's stubs, and a static guard that silently stops running is worse than no guard. 115 tests across the four skills files pass, stable under both file orderings. Refs Abilityai/trinity-enterprise#237 * fix(skills): make legacy adoption a one-way migration (ent#237) `_adopt_legacy_clone` left `skills_library_url` in place after adopting it as a source, so the setting acted as a read-time default: an admin who deliberately deleted the migrated source got it silently re-created on the next sync. That is the same resurrection trap the fresh-install seed is a ROW specifically to avoid (#1638's lesson — a read-time default is re-applied on every boot, so it cannot honour a deletion). I wrote that reasoning into the seed and then reintroduced the bug one function away. Adoption now consumes both legacy keys once it succeeds. Nothing else reads them after ent#237 (verified: the only remaining references are the SSRF validator's name and the settings-router special case), so consuming them strands no consumer. Best-effort — a failed delete leaves a duplicate-suppressed re-adoption on the next sync via the existing `existing` check, never a broken migration. Regression test asserts the full sequence: adopt, admin deletes, sync again, source stays gone. Mutation-verified — neutering the delete fails it. Refs Abilityai/trinity-enterprise#237 * fix(skills): reject agent principals on the source LIST endpoint too (ent#237) Found by /review on this branch. `GET /api/skills/sources` was `require_admin` only, and its own docstring justified that gate as "the rows carry repo URLs, which for a private source are themselves sensitive." `require_admin` does not deliver that. An agent-scoped MCP key resolves to its owner carrying the owner's role (ent#293), so on a default admin-owned install every agent could read exactly the private repo URLs the gate exists to protect — and a prompt-injected agent reading them is the disclosure, not a hypothetical. I had applied `reject_agent_principal` to all three mutating routes on the grant-vs-use reasoning, and that framing is what hid this: grant-vs-use quietly implies reads are fine on a role gate. It is the wrong axis for a read. The right question is whether the RESPONSE is sensitive, and here it is. Sync stays `require_admin` only: it pulls an already-admin-configured repo and returns no URLs, so it is genuinely use. The static guard gains a GATED_READS list so a future read endpoint carrying source URLs fails the same way rather than being argued about again. Lesson recorded in learnings.md — the ledger already tracks this class (trinity-ops-agent#232 → #1644 → #1816 → ent#293) and the read-vs-write blind spot is the new part. Refs Abilityai/trinity-enterprise#237 * fix(skills): use the status-danger design token, reject credentialed source URLs (ent#237) Two fixes. **CI, frontend-build**: SkillSourcesPanel used `status-error-*`, which does not exist. The palette is `status-success` / `status-warning` / `status-danger` / `status-info` / `status-urgent` — I invented `error` by analogy from the two I happened to see in Settings.vue. `vite build` passes on an unknown Tailwind class (it just emits nothing), so only `npm run check:tokens` catches it, and I had run the build but not that check. **Security, from /cso --diff**: a source URL embedding a token (`https://<token>@github.com/owner/repo`) passed validation and was persisted verbatim. `validate_skills_library_url` checks `parsed.hostname`, which IGNORES userinfo, and returns the URL unchanged — so the credential would land in `skill_sources.url` in plain text, be returned by GET /skills/sources, and be rendered in the Settings panel. Pasting one is an easy mistake: it is the form GitHub hands you for scripted clones. Rejected with a named 400 pointing at the GitHub PAT setting, rather than stripped silently — a silently-stripped token would leave the admin believing private-repo auth was configured when it was not. Enforced at the ent#237 routes, deliberately NOT in the shared validator: that helper also serves the pre-ent#237 `skills_library_url` setting, and an install relying on an embedded token for private-repo access would break on upgrade. The class is pre-existing; this branch widens the exposure (N sources, a new endpoint returning them, a panel rendering them), so the guard sits on the new surface. A test pins that the shared validator does NOT strip userinfo — if that ever changes, it fails and the extra guard can be reconsidered rather than lingering as unexplained defense. Refs Abilityai/trinity-enterprise#237 * fix(skills): parse the host before splicing the PAT; drop the sys.modules footgun (ent#237) CodeQL flagged 4 new alerts on PR #1901; this addresses the actionable one and relocates the credential guard so it is testable. **py/incomplete-url-substring-sanitization (high)** — `_authenticated_url` decided whether to splice the platform GitHub PAT with `"github.com" in url`, then spliced via `url.replace("https://", f"https://{pat}@")`. A substring test is satisfied by `https://evil.example/?x=github.com`, so that pair would have sent a live GitHub credential to an attacker host. Not reachable today — `sync_library` validates every source URL against the github.com allowlist first — but "safe only because a caller three frames up validates" is exactly the property that breaks when a caller is added, and the blast radius is a live PAT. Now: shorthand is normalised to an absolute https URL first, the host is PARSED, and the splice happens only on an exact match against the same `ALLOWED_SKILLS_LIBRARY_HOSTS` the SSRF guard uses. Rebuilt via urlunparse rather than str.replace, which would also rewrite a second "https://" occurrence inside a path or query. Moves the embedded-credential guard from `routers/skills.py` into `utils/url_validation.py` as `reject_embedded_credentials` + `EmbeddedCredentialError`. It is URL policy and belongs with URL policy, and testing it through the router required importing the whole `routers` package, which drags in the agent-service chain and collapsed under another module's import-time stubs. A leaf module is importable from anywhere. The remaining CodeQL alerts are pre-existing on dev (a test file untouched here, and a path-injection alert on the `_skill_dir` chokepoint whose realpath containment is the documented guard, now applied per-source). Test-ordering fixes forced by the same stub fragility (#1898): the ent#183 stub of `utils.url_validation` now mirrors every name `skill_service` imports — a missing constant is an ImportError at collection, not graceful degradation — and this file gets an autouse fixture that evicts detectable stubs (a stub has no `__file__`) so results do not depend on which file pytest runs first. That fixture uses `monkeypatch.delitem`, NOT a bare `del sys.modules[...]`: `tests/lint_sys_modules.py` exists to stop exactly that pattern, and working around sys.modules pollution by polluting sys.modules is how this file would have become the next #1898. The eviction is undone at teardown. Squashed with its follow-up because a test-only commit matches no workflow path filter, so CI never ran on it. Refs Abilityai/trinity-enterprise#237 * fix(skills): clear the three new CodeQL alerts on this PR (ent#237) CI's CodeQL check flagged 3 new alerts in code this PR changed. 1. py/incomplete-url-substring-sanitization (high) — `_authenticated_url` decided whether a scheme-less source URL already carried a host with `url.startswith("github.com/")`. That is the same bypassable class of check the surrounding docstring says this function exists to avoid, one line below the comment saying so. Decided by parsing now, against the same allowlist the splice itself uses, so there is exactly ONE way this module answers "which host is this". Side effect: `www.github.com/owner/repo` shorthand now resolves to that host instead of becoming a repo path under github.com. 2+3. py/stack-trace-exposure (medium ×2) — both sync routes hand `sync_library()["error"]` to FastAPI verbatim as an HTTP `detail`, so that string is an API surface, not a log line; three `except ... as e` branches interpolated the caught exception into it. The messages are rebuilt from the stored source row (ref / ref_type / a fixed URL-shape hint) and the full exception goes to the log, which is where an operator debugs from anyway. The second alert is the pre-existing one on `/skills/library/sync`, re-flagged because this PR rewrote its sources — same root cause, cleared by the same change. Tests pin all three: without the fix 3 of the new cases fail on the exception text and one on the www host. * fix(skills): close the two defects /review and /cso found on this branch (ent#237) Both are pre-merge finds on this branch, not regressions from dev. ## Alembic revision graph forked into two heads (/review, CRITICAL) This branch cut `0031_skill_sources` off `0030`; dev independently cut `0031_channel_report_back` off the same parent. Separate files, so the merge never conflicted — but `alembic upgrade head` refuses a multi-head argument, so `init_database()`'s non-SQLite branch would fail and PostgreSQL boot with it. CI could not have caught this before the merge: it ran when `0031_skill_sources` was the only 0031. Renumbered to `0034_skill_sources` onto the current head rather than adding an Alembic merge revision — this table has no relationship to the channel/telegram/evaluations chain, and a linear history is what the `schema-parity` and `pg-migrations` jobs assert. ## Tag pin bypassed on the clone path (/cso, HIGH, exploit executed) `_update_tag` enforced the AC#5 pin two ways — a `fetch` without `--force` (git refuses to clobber an existing tag ref) and an explicit recorded-SHA comparison — and BOTH are properties of an existing checkout. `sync()` routes to `_clone()` whenever `.git` is absent, and that path took no `expected_sha` and compared nothing. The `_update_tag` docstring claimed to cover the fresh-clone case; it does not, because a fresh clone never reaches it. Lose the checkout — this class's OWN quarantine rename, a restored /data backup, a recreated volume — while upstream moves the tag, and the moved tag was adopted silently: success=True, moved_tag=None, commit changed. `commit_changed` then goes true, so ent#236's fleet re-inject pushes the moved tag's executables to every running agent with no human in the loop. That is the exact scenario tag pinning exists to prevent, defeated behind a successful-looking sync. Verified by executing it against a local upstream: before the fix the payload landed on disk; after, sync refuses with moved_tag and the checkout is gone. `_refuse_moved_pin_after_clone` re-checks the resolved HEAD against the recorded SHA after a fresh clone and DELETES the checkout on refusal — `list_skills` and injection read the working tree, so a failed sync that left it behind would still serve the moved tag's content. A first-ever sync has no recorded SHA and is untouched. The existing `test_moved_tag_is_refused_and_payload_never_lands` passed throughout because it clones BEFORE moving the tag, so it can only ever exercise the update path. The new regression test varies the starting state instead, and fails without the fix. ## Also - `POST /skills/sources/{id}/sync` returned 400 on lock contention where its sibling returns 409, and ran a git clone inline in an async handler. Both are consequences of ent#237 moving the ent#236 sync lock into the shared `sync_library`; now `asyncio.to_thread` + 409, mirroring the full sweep. - architecture.md: `skill_sources` DDL, the three source routes, the sync-lock Redis key, and a `skill_service` description that still described single-repo sync (CLAUDE.md rule 4 — API/schema change). - requirements/skills.md §21.1.2 asserted the fresh-clone coverage the code did not have; corrected to describe both paths. - learnings.md: the durable class — a "must not have changed" control enforced on the update path is bypassed on the create path, and the test that proves it clones first. Full unit suite at seed 12345: 7250 passed / 0 failed. Refs Abilityai/trinity-enterprise#237 * feat(skills): vendor-neutral library layout — per-source skills root (Abilityai/trinity-enterprise#332) The skills-library source layout was hardcoded to `.claude/skills/`, so the public community catalog (abilityai/trinity-skills, ent#296) — which already ships a vendor-neutral top-level `skills/` layout plus a root `catalog.yaml` declaring `skills_root: skills/` — synced to zero skills. The SKILL.md format is harness-portable; only the *discovery path* is Claude-specific. Deliberately reopens ent#237's "root not configurable per source" call: the repo is untagged with zero syncing instances today, so this is the cheap moment — after the first `v*` tag it becomes a breaking migration. Resolution is per source, in `SkillSourceClone.skills_rel_root()` (lazily cached; instances are per-operation, so no staleness across syncs): catalog.yaml `skills_root:` → evidence-gated `skills/` probe → `.claude/skills/` Every invalid tier falls THROUGH to the next rather than blanking the source: an unusable catalog (unparseable, alias-bearing, non-mapping, unknown `schema_version`, oversized, symlinked, invalid value) degrades to the probe, and a symlinked or clone-escaping declared root does too. Only the final tier escaping containment yields an empty listing — None/empty-propagated through `skill_dir`/`skill_names`, never a raise through `list_skills`. A source serving skills today cannot be blanked by a tier it does not use. The agent-side destination stays `~/.claude/skills/<name>/`, and `filter_skill_archive(source_root=…)` is the ONE point where source layout becomes destination: it rewrites tar arcnames to `.claude/skills/<name>/…` (identity for the legacy layout). That placement is required, not merely tidy — `executable_paths` → the agent-side chmod list, the restore sent-vs-restored accounting, the persisted manifest, and `_legacy_fallback`'s SKILL.md lookup all key on post-filter arcnames. Consequence: manifests, prune/removal confinement, per-skill `.gitignore` lines, the restore `paths` allowlist and the whole ent#236 removal machinery stay destination-canonical with ZERO migration, and installed packages keep pruning correctly across a repo restructure. Three guards on the author-controlled value, each catching what the others structurally cannot (see docs/memory/learnings.md): - Segment-wise validation — split on `/`, reject any segment that is empty, `.` or `..`. A whole-string charset regex admits `.`, `./skills` and `skills//x`, each of which breaks archive-prefix math into per-skill "empty package" failures. A leading `-` is refused so the value can never read as a git option (the `--` separators at every pathspec are the belt to this suspenders). - lstat-order symlink refusal on catalog.yaml and on both candidate roots, with a bounded `read(cap+1)` — a post-read length check is defeated by `catalog.yaml -> /dev/zero`, and git materializes author symlinks. A symlinked `skills/` would list fine but yield empty `git archive` output from HEAD, failing every injection. - realpath containment against the REALPATH'd base — `/data/skills-library` may itself be a symlink, so comparing a resolved target to an unresolved base would refuse every source spuriously. Parsing goes through the shared ent#314 hardened loader (`AliasPolicy.REJECT`, 64 KB cap), catching `HardenedYamlError` explicitly alongside `yaml.YAMLError` — it is a ValueError, not a YAMLError, and missing it would escape through `list_skills` and 500 the merged listing. `schema_version` gates the whole catalog (absent or `1` — int and string both tolerated); anything else degrades to the probe rather than being silently misread, which keeps a future schema bump safe. Platform parses only these two keys; `categories`/`providers` stay catalog metadata for UIs. Dual-layout guard: SKILL.md evidence under BOTH roots with no catalog to decide keeps `.claude/skills/` and flags `layout_conflict` in status. A pre-existing dual-layout source must never silently flip *which executable content* the ent#236 unattended auto-sync injects fleet-wide; switching requires the explicit declaration. Also: per-source `skills_root` + `layout_conflict` in library status (null until cloned, honest rather than guessed), honest per-source `path` provenance, and the now-stale operator copy in SkillSourcesPanel. 52 new tests. Independently reviewed by strategy + engineering agents and a second-opinion model; `/cso --diff` found zero findings introduced by this change (report under docs/security-reports/; one pre-existing inherited disclosure filed separately as Abilityai/trinity-enterprise#334). Refs Abilityai/trinity-enterprise#237, Abilityai/trinity-enterprise#296 Fixes Abilityai/trinity-enterprise#332 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(skills): make source edits reach disk, and gate sync on effect (ent#237) Four defects from review, three of one shape: the row moved and the checkout did not. 1. Repointing a source's `url` was a no-op on disk. `_update_*` fetches `origin`, written at clone time, so every later sync silently pulled the OLD repo — reporting success, with a moving commit that feeds ent#236's fleet re-inject. `sync` now compares `origin` against the configured remote and discards + re-clones on a genuine mismatch. Discard rather than `remote set-url`: the latter leaves the old repo's refs behind, and a tag name present in both repos at different commits then reads as a moved pin and is refused forever. The compare strips credentials (a private source's on-disk origin carries a PAT the stored URL never does, so a naive compare re-clones on every sync), and an unreadable origin counts as a MATCH — the action gated on that answer is an rmtree, so ambiguity resolves to "don't". 2. The documented tag-bump path was unreachable. `update_source` never cleared `last_commit_sha`, so bumping `v1` → `v2` compared v2 against v1's recorded SHA and was refused as `moved_tag`, with the error telling the operator to do what they had just done; on the fresh-clone path the refusal rmtree's first, so the source ended up empty. The bookkeeping is now cleared when `url`/`ref`/`ref_type` change — and deliberately NOT on name/enabled/priority, since a cleared baseline on disable would let a tag moved during the disabled window be adopted silently on re-enable (the AC#5 bypass via a checkbox — and the enabled toggle is the only edit the UI currently wires to PUT). 3. Deleting a source leaked its checkout. `discard_source_checkout` reclaims `<id>/` + `<id>.broken` on delete (row first, disk second), with `_reclaim_orphan_checkouts` on full sweeps as the backstop for the crash window and for installs that already accumulated clones. Fail- closed on the row read — a sweep reading "DB error" as "no sources" deletes the whole library — and scoped to the server-minted id shape, so the legacy checkout and operator directories are out of reach. 4. Both sync routes are now human-only. They were role-gated as "use, not grant", but grant-vs-use is a claim about EFFECT: a sync clones executable material and spawns `run_fleet_reinject`, pushing skill scripts/ to every running agent, and an agent-scoped key resolves to its owner carrying the owner's role (ent#293). The coverage scan widened from /skills/sources to /skills/ — `POST /skills/library/sync` sits outside the sources prefix, so the old scan reported full coverage while missing the route in question. Also: `test_mutating_routes_reject_agent_principals` was satisfiable by a comment. It grepped `ast.dump(fn)`, which renders docstrings, so documenting the gate in (4) made the guard pass with the real call deleted. Now matches an ast.Call node. 20 new tests. Each of the four fixes mutation-verified to fail exactly its own test. Full unit suite: 7322 passed, 18 skipped. * test(skills): pin the fail-safe direction of the repoint discard (ent#237) @dolho's I1 on #1901: `_origin_matches` returns MATCH when `git config` cannot read `origin`, because the action gated on the answer is an `rmtree` and an unknown answer must not widen an unattended delete (#1638/#1644). The argument lived only in the docstring — flipping the branch to `return False`, so a transient git failure discards the checkout, left all 92 tests green. The feature half was already covered (neutering the origin check fails `test_repointing_moves_the_git_remote_too`); only the safe direction was not. Staged as the case where a discard would otherwise be CORRECT — the url IS genuinely repointed, and only the origin read is blinded — so the assertion is about the ambiguity resolving to "leave it alone", not about a mismatch simply not existing. Asserts both halves: `_origin_matches` is True, and a marker written inside `.git` survives the sync. Mutation-verified: `return True` → `return False` fails exactly this test (1 failed, 92 passed) and nothing else. Refs Abilityai/trinity-enterprise#237 * fix(skills): reclaim the old repo's quarantine when a source is repointed (ent#237) @dolho's I2 on #1901: `_discard_repointed_checkout` removed `<id>/` but not `<id>.broken`, and neither reclamation path can reach a quarantine while the source still exists — `discard_source_checkout` runs on delete, and `_reclaim_orphan_checkouts` considers only ids with NO row. So the previous repo's content sat in the library root for the life of the source. Bounded at one per source, so disk hygiene rather than a correctness bug, but a repoint makes that directory permanently irrelevant by construction. `.broken` was already spelled in four places across two modules — the writer here and both reclaimers in `skill_service` — so this adds a shared `QUARANTINE_SUFFIX` and a `quarantine_path` property rather than a fifth literal. A suffix that drifts between the writer and the reclaimer leaves quarantines nothing can collect, which is this same bug one level up. The test produces the quarantine the way production does (a checkout that has lost its `.git`, taking the clone branch) rather than writing the directory by hand, so it breaks if the naming ever moves, and asserts the old-repo content was really there before the repoint. Mutation-verified: restoring the single-path `rmtree` fails exactly this test (1 failed, 93 passed). Full unit suite 7557 passed, 18 skipped (+64 in test_ent236_skills_lifecycle.py, which still cannot be co-collected with test_ent183_skill_packages.py — the pre-existing #1898 sys.modules stub, present on this branch before these two commits). Refs Abilityai/trinity-enterprise#237 --------- Co-authored-by: Eugene Vyborov <1073874+vybe@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: dolho <66411456+dolho@users.noreply.github.com>
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.