feat(skills): multi-source skills library — bundled community repo + per-instance custom repos (abilityai/trinity-enterprise#237) by obasilakis · Pull Request #1901 · Abilityai/trinity

obasilakis · 2026-07-30T20:39:07Z

Summary

Turns the skills library from one admin-configured repo into many: a bundled public community catalog plus any number of admin-added custom repos. skill_sources replaces the single skills_library_url setting; skill_service orchestrates N SkillSourceClones under /data/skills-library/<source_id>/ instead of being one clone itself.

Implements abilityai/trinity-enterprise#237. All four AC-flagged design decisions were taken explicitly and are recorded below with the alternative that was rejected.

The decisions

AC#4 — name collisions resolve custom-wins, names stay bare. Resolution is priority ASC then created_at ASC (custom 100, bundled 1000). Prefixing (community/pdf-export) was rejected 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 — a prefix changes every agent's /skill-name invocation string and needs a fleet-wide re-inject. agent_skills.source_id records which source a name resolved to (it is not part of the UNIQUE — two sources' copies cannot coexist on disk anyway).

Never a silent overwrite: the winner carries shadowed_by, surfaced in the library listing, the Settings panel, and as a shadowed_source:<name> warning at inject time.

AC#5 — the bundled source pins to a tag, and a moved tag is refused. Skills carry executable scripts/ (ent#183) that ent#139 runs, and ent#236 makes syncing automatic — a branch-tracking community source puts every merged upstream commit on every install with no human in the loop. Git does not enforce tag immutability, so a tag resolving to a different commit than the last sync fails as moved_tag. Two independent mechanisms: the fetch omits --force (git refuses to clobber a moved tag), plus an explicit SHA comparison for the fresh-clone case where no local ref exists to conflict. Custom sources keep tracking a branch.

AC#7 — all OSS-core (vybe's call, here). One seam per area: distribution free, execution paid via the existing skill_runner entitlement. No requires_entitlement, no private module, no gated Vue.

AC#6 — existing installs migrate losslessly. A configured skills_library_url is adopted as a custom source (custom, not default — precedence must keep preferring the repo the operator actually chose). Row written before the clone moves, so a crash between them leaves a source that simply re-clones; the reverse order strands a checkout no row points at.

AC#1 (create the public repo) is split to abilityai/trinity-enterprise#296 — repo administration plus a content-curation call, not Trinity code.

Auth boundary

Every mutating /api/skills/sources route carries reject_agent_principal in addition to require_admin, and so does the LIST route. require_admin answers what role, never is this a human: an agent-scoped MCP key resolves to its owner carrying the owner's role (abilityai/trinity-enterprise#293). Registering a source decides which repo the fleet executes code from — the grant action. The list route needs it too because its gate exists to protect private repo URLs, which a role check does not deliver; that one was caught by /review on this branch and is now a recorded lesson (grant-vs-use is the wrong axis for reads).

This PR removes ent#293's step-1 target (skills_library_url), but does not close that issue — the generic settings gate is the actual defect.

Ordering note

The default source points at abilityai/trinity-skills, which does not exist until abilityai/trinity-enterprise#296 lands. Sync is fail-soft (never raises), so a fresh install shows one failed source rather than a populated catalog until then. Deliberately not worked around in code — seeding it disabled would satisfy AC#3 on paper while leaving the library empty. Prefer landing #296 first, or accept the window knowingly.

Test plan

tests/unit/test_ent237_skill_sources.py — 47 tests: precedence, shadowing, handover on disable, cache invalidation, broken-source isolation, adoption, seeding, auth gates
The moved-tag test builds the actual attack (force-moves an upstream tag onto a commit adding a payload) and asserts the payload never reaches disk — not merely that the call returned an error
Mutation-verified: neutering the pin, the agent-principal gate, or the setting-consume each fails exactly its own test
Fixed 27 regressions this branch introduced in the pre-existing skills tests (they exercise the single-clone API that moved)
Full unit suite: 5681 passed, 2 failed — both failures pre-existing and reproduced on clean origin/dev (filed as bug(tests): test_ent183_skill_packages sys.modules stubs poison later tests (order-dependent CI flake) #1898)
tsc --noEmit clean · vite build clean · 70 CI parity guards pass

Not included

The per-skill source badge in SkillsPanel.vue — #1877 is rewriting that file, so editing it here would hand @dolho a conflict. The backend already exposes source_name/shadowed_by; noted on that PR.

Refs abilityai/trinity-enterprise#237

🤖 Generated with Claude Code

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. Also moves the embedded-credential guard from `routers/skills.py` into `utils/url_validation.py` as `reject_embedded_credentials` + `EmbeddedCredentialError`. Two reasons: it is URL policy and belongs with URL policy, and testing it через 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 router now maps the domain error to its 400. The remaining CodeQL alerts are pre-existing on dev (a test file I did not touch, 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 its results do not depend on which file pytest runs first. 159 tests across the five skills/SSRF files pass in the polluted ordering. Refs Abilityai/trinity-enterprise#237

…ules 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

… library (ent#263) - stores/skillsLibrary.js (new): fleet-scoped store, deliberately separate from stores/skills.js (KeepAlive-cached AgentDetail means SkillsPanel's clear() never fires on nav-away — shared refs would poison the cached tab); imports nothing from stores/skills.js. 4-state emptyReason discriminator (unconfigured/not_cloned/empty + error carried separately); sync() with a 180s timeout and ECONNABORTED -> status-refetch (a first clone can outlive the 30s api.js default; client timeout != server failure) - components/LibrarySkillsSection.vue (new): sync-state header leads with commit_sha + skill_count (disk-derived; last_sync is per-worker in-memory and renders only when truthy); repo URL admin-only, userinfo-stripped, labeled 'Primary source', hidden when status.sources reports >1 (#1901 forward-compat); admin Sync now; per-kind empty states teaching the next action; dormant source_name/shadowed_by slots; interpolation only - components/skills/{SkillContractChips.vue,contract.js} (new): the #183 contract-chips seam extracted from SkillsPanel so both the per-agent tab and the Library browse render package facts from one seam - SkillsPanel.vue: consumes the shared seam (local SkillMeta/formatBytes/deps removed); stores/skills.js untouched - Library.vue: skills section wired in + header jump anchors (no ?kind=) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…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>

github-actions · 2026-07-31T09:27:00Z

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

obasilakis · 2026-07-31T09:39:55Z

reopening to re-trigger CI — the original push landed during a repo-wide Actions dispatch gap (nothing ran anywhere in the repo between 2026-07-30T21:19Z and 2026-07-31T09:22Z)

…ules 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

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

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

…ed (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

…t#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

…on (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

…nt#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

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

…ce 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

`_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

…(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

…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

…ules 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

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.

…ecycle automation ent#236 (PR #1883) landed on dev BEFORE this branch rather than after, so it rewrote the same sync entry points ent#237 was replacing. The reconciliation, not the textual conflicts, is the substance of this merge. Kept from ent#236, adapted to N sources: - The cross-worker sync lock now wraps the multi-source loop (`_sync_library_locked(url)` -> `_sync_sources_locked(sources)`). ONE lock for the whole sweep: per-source locking would let two workers interleave and each publish a merged listing built from a half-updated set of checkouts, and the listing, the cache invalidation and the durable status are all library-wide. - Durable sync status (`skills_library_last_*`) stays library-wide and is still written on BOTH branches; per-source truth is on each `skill_sources` row. - `commit_changed` — the gate the fleet re-inject fires on — is computed per source against that source's own DURABLE `last_commit_sha` and OR'd, so a fresh process cannot read "changed" and sweep the fleet on every restart. - The non-repo-directory quarantine is ported into `SkillSourceClone`, where clone-vs-update now lives. Without it ent#236's forever-fail fix would have been silently dropped per source. Superseded and removed: `_git_clone` / `_git_pull` / `_get_current_commit` / `_quarantine_non_repo_dir` on SkillService — `SkillSourceClone` owns one checkout's git lifecycle and `skill_service` orchestrates N of them. Settings.vue keeps ent#236's automation card (auto-sync, interval, fleet re-inject) alongside <SkillSourcesPanel />; taking this branch's side wholesale would have deleted a shipped feature. Only the single-library URL/branch/sync controls the source list supersedes are dropped. Its loader is wired into the admin-only loaders — the old mount call was auto-merged away with the deletion. Also caught in the auto-merged (non-conflicting) regions: - `skills_sync_service` read top-level `commit_sha`/`action`, which the multi-source return no longer had. `commit_sha` is back as an explicitly documented library-wide summary marker; the audit row now records the per-source breakdown instead of one arbitrary source's action passed off as the library's. - The default source ref was pinned to a `v1.0.0` tag that will never exist — ent#296 cuts v0.1.0. A fresh install would have seeded a source that can never sync, failing quietly. Documented in .env.example. Tests: two files stub `utils.url_validation` with only one symbol while this branch imports ALLOWED_SKILLS_LIBRARY_HOSTS from it; the stub installs only when the module is absent from sys.modules, so this passed CI on file order alone and failed in isolation on the pre-merge tip too. Both stubs completed. Nine ent#236 tests drove the deleted single-repo API and are retargeted to the new seams — the properties (durable status, PAT scrubbing, commit-changed-vs-durable-row, quarantine) are unchanged and now exercise real code paths. Full unit suite at seed 12345: 7248 passed / 0 failed (clean dev: 7176 / 0). Refs Abilityai/trinity-enterprise#237, Abilityai/trinity-enterprise#236

…nch (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

github-advanced-security AI found potential problems Jul 30, 2026

View reviewed changes

Comment thread src/backend/services/skill_service.py Fixed

Comment thread src/backend/services/skill_service.py Fixed

Comment thread src/backend/routers/skills.py Fixed

github-advanced-security AI found potential problems Jul 30, 2026

View reviewed changes

Comment thread src/backend/services/skill_service.py Fixed

obasilakis closed this Jul 30, 2026

obasilakis reopened this Jul 30, 2026

obasilakis force-pushed the feature/ent-237-multi-source-skills branch from 25d74a5 to c60b393 Compare July 30, 2026 22:05

vybe mentioned this pull request Jul 31, 2026

feat(library): rename Templates page to Library — one surface for agent templates + skills (ent#263) #1904

Merged

obasilakis closed this Jul 31, 2026

obasilakis reopened this Jul 31, 2026

obasilakis force-pushed the feature/ent-237-multi-source-skills branch from c60b393 to 3a58b05 Compare July 31, 2026 09:41

obasilakis force-pushed the feature/ent-237-multi-source-skills branch from 3a58b05 to 4ed3c40 Compare July 31, 2026 09:43

obasilakis added 12 commits July 31, 2026 12:51

obasilakis force-pushed the feature/ent-237-multi-source-skills branch from 4ed3c40 to 2d64614 Compare July 31, 2026 09:52

obasilakis requested a review from vybe July 31, 2026 11:00

obasilakis added 2 commits August 4, 2026 14:31

Navigation Menu

Uh oh!

feat(skills): multi-source skills library — bundled community repo + per-instance custom repos (abilityai/trinity-enterprise#237) - #1901

feat(skills): multi-source skills library — bundled community repo + per-instance custom repos (abilityai/trinity-enterprise#237)#1901
obasilakis wants to merge 15 commits into
devfrom
feature/ent-237-multi-source-skills

obasilakis commented Jul 30, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

obasilakis commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

obasilakis commented Jul 30, 2026

Summary

The decisions

Auth boundary

Ordering note

Test plan

Not included

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

obasilakis commented Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants