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

[obasilakis]

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 route under /api/skills/ that mutates or reads source configuration carries reject_agent_principal in addition to require_admin — the four source routes, the LIST route, and both sync routes. 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).

The rule is gate on what the route does, not on which verb it is — arrived at the hard way, because the grant-vs-use framing mis-classified two routes and both had to be corrected:

• LIST, caught by /review on this branch: "read" says nothing about the private repo URLs it returns.
• Both sync routes, caught by @dolho in review: originally role-gated as "use, not grant", but a sync clones executable material and, when the commit moves with auto-reinject on, spawns run_fleet_reinject — pushing skill scripts/ to every running agent. Fleet-wide executable delivery is not "use".

The static guard that pins this was itself checkable-by-comment (it grepped ast.dump(fn), which renders docstrings) — found by mutation-testing it, now matches an ast.Call node.

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 — 92 tests: precedence, shadowing, handover on disable, cache invalidation, broken-source isolation, adoption, seeding, auth gates, and (review round) source-edit-reaches-disk + checkout reclamation
• 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: 7322 passed, 18 skipped, 0 failed (the two bug(tests): test_ent183_skill_packages sys.modules stubs poison later tests (order-dependent CI flake) #1898 failures are gone since the dev merge)
• tsc --noEmit clean · vite build clean · 70 CI parity guards pass

Review round (@dolho, 2026-08-05)

Three source-lifecycle defects and one auth inconsistency, all fixed in b92c3859. Three shared one shape — the row moved and the checkout did not:

1. Repointing url was a no-op on disk. _update_* fetches origin, written at clone time, so every later sync silently pulled the OLD repo with last_sync_status: success. Now discards + re-clones on a genuine mismatch (not remote set-url, which leaves the old repo's refs and turns a shared tag name into a permanent moved_tag refusal). Compare is credential-stripped, and an unreadable origin counts as a match — the action gated on it is an rmtree.
2. The documented tag-bump path was unreachable. update_source now clears the sync bookkeeping on a url/ref/ref_type change. Explicitly not on name/enabled/priority: 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. Reclaimed on delete (row first, disk second), with a fail-closed orphan sweep on full syncs as the backstop and the only reclamation path for installs that already accumulated clones.
4. Both sync routes gated human-only — see Auth boundary above.

20 new tests. Each fix mutation-verified to fail exactly its own test.

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

[github-actions]

⚠️ 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]

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)

[dolho]

Reviewed the full diff (30 files). The architecture is right — per-source SkillSourceClone, precedence as a plain ORDER BY, dual-track migrations present (SQLite skill_sources_table + Alembic 0034, re-parented onto the current dev head), the moved-tag test that builds the actual attack rather than asserting an error string. The tag pin and the fresh-clone bypass closure are genuinely well reasoned.

Requesting changes on three source-editing/lifecycle defects, one auth inconsistency against the PR's own stated invariant, plus red CI.

---

CI is red — must be green before merge

regression diff fails: https://github.com/Abilityai/trinity/actions/runs/30906617238/job/91987135445

##[error]Unable to download artifact(s): Failed to GetSignedArtifactURL: Unable to make request: ECONNRESET

To be fair: this is infrastructure, not this branch. All six pytest shards (head + base × 3 seeds) passed and the job died downloading the junit artifacts. But the check is red and blocks the merge gate, so please re-run it and confirm green. If the flake repeats, that's worth its own issue against the workflow.

---

1. Editing a source's url is a no-op on disk — it keeps syncing the old repo

PUT /skills/sources/{id} accepts url (SkillSourceUpdate.url, _MUTABLE_FIELDS), but SkillSourceClone.sync branches on (self.path / ".git").exists() and takes _update, which never touches the remote:

def _update_branch(self):
    fetch = self._git([..., "fetch", "origin", self.ref], ...)
    reset = self._git(["reset", "--hard", f"origin/{self.ref}"])

origin still points at the URL captured at first clone. auth_url is only consumed by _clone. So after repointing a source at a different repo, every subsequent sync silently pulls the old one — indefinitely, with last_sync_status: success and a changed commit that feeds ent#236's fleet re-inject. The admin sees a repointed source in Settings and the fleet keeps receiving the previous repo's executables. Silent-wrong is the bad failure mode here, worse than the tag case below which at least errors.

Fix: on a url change, either git remote set-url origin before fetch, or drop the checkout so the next sync re-clones. Whichever, it needs a test that changes url on a synced source and asserts the content on disk comes from the new repo.

2. The documented tag-bump path is unreachable

skill_source_clone says, twice, "point this source at a new tag" as the way to adopt new content. But update_source never clears last_commit_sha, and the caller passes it unconditionally for tag sources:

expected_sha=src.last_commit_sha if src.ref_type == "tag" else None,

So bumping ref from v1 to v2 hits _update_tag's SHA comparison against v1's recorded SHA, mismatches, and is refused as moved_tag — the error text telling the operator to do the thing they just did. On the fresh-clone path it's worse: _refuse_moved_pin_after_clone rmtrees the checkout first, so a source that was merely being bumped ends up with no content at all.

Same class for a branch → tag transition.

Fix: clear last_commit_sha in update_source when ref or ref_type changes (a recorded SHA is only meaningful for the ref it was recorded against). Test: create a tag source, sync, bump ref to a second tag, sync, assert the new tag's content lands and no moved_tag.

3. Deleting a source leaks its checkout

delete_source deletes the row only; nothing removes /data/skills-library/<source_id>/. Same for <source_id>.broken quarantines, which are bounded per source but survive the source's deletion. Add/remove cycles accumulate full clones on the data volume with no reclamation path short of shell access.

The comment on _adopt_legacy_library already names this exact hazard ("an orphan nothing ever cleans up") for the crash-ordering case, so the reasoning is present — it just isn't applied to delete. Either remove the directory on delete, or reconcile dirs-without-rows on sync. Given #1638/#1644, a reconcile sweep should log what it reclaims.

4. The sync routes don't match the PR description's auth claim

The description says "Every mutating /api/skills/sources route carries reject_agent_principal in addition to require_admin". sync_skill_source and sync_library carry only require_admin, and test_every_mutating_source_route_is_covered explicitly subtracts sync_skill_source on a grant-vs-use rationale.

Two things:

• The description overclaims and should be corrected either way.
• More substantively: this PR already records that grant-vs-use was the wrong axis once (that's why the LIST route got the gate). Sync isn't a read — on this branch it clones code and, when commit_changed and auto-reinject is on, spawns run_fleet_reinject, pushing executable scripts/ to every running agent. An agent-scoped MCP key on an admin-owned install resolves to the owner's role and passes require_admin, so an agent can trigger fleet-wide executable delivery. Combined with #1 above (origin never repointed), an agent-triggered sync is also the mechanism that propagates a stale repo's content.

I'd gate both sync routes human-only and add them to MUTATING. If you disagree, say so explicitly in the test comment and the issue — but the current split isn't defensible as "use, not grant" when the effect is fleet-wide code injection.

---

Non-blocking

Ordering. The seeded default points at abilityai/trinity-skills, which doesn't exist until abilityai/trinity-enterprise#296. Your note is honest about it, and I agree not to fake it in code — but landing this first means every fresh install boots with a permanently failing source until #296 exists. Prefer sequencing #296 first; if that slips, the release note has to say so.

Docs. architecture.md / requirements/skills.md are updated, good. Once #1 and #2 are fixed, the "point at a new tag" instruction in skill_source_clone's module docstring becomes true rather than aspirational.

Happy to re-review quickly once the three defects and the auth gate are addressed.

[obasilakis]

All four addressed, plus one you didn't ask for that fell out of verifying #4.

1. Repointed url was a no-op on disk — fixed

You had it exactly right: _update_* fetches origin, which git wrote at clone time.

Fixed by discarding and re-cloning on a genuine mismatch rather than git remote set-url. set-url is the smaller edit but leaves the old repo's refs in place, and a tag name present in both repos at different commits then reads to _update_tag as a moved pin and is refused forever — trading a silent-wrong for a permanently-stuck.

Two details worth flagging because they cut the other way:

• The compare is credential-stripped (canonical_remote). A private source's on-disk origin carries a PAT the stored URL never does, so a naive string compare would report a repoint on every sync and re-clone the whole library each time. Test test_an_unchanged_url_does_not_re_clone pins that by writing a marker inside .git and asserting it survives.
• An unreadable or absent origin counts as a match, not a mismatch. My first cut had it the other way ("fetch origin would fail anyway, so re-clone is the recovery either way") and it broke test_real_repo_still_pulls from ent#236 — whose whole point is "the fix must not turn every sync into a re-clone". It was right and I was wrong: the action gated on this answer is an rmtree, so ambiguity has to resolve to don't. The repoint case always has a readable origin, which is the entire problem, so narrowing costs nothing.

Test: test_repointing_the_url_changes_what_lands_on_disk asserts content on disk comes from the new repo and the old skill is gone; test_repointing_moves_the_git_remote_too asserts origin itself moved.

2. Tag bump unreachable — fixed

update_source now clears the sync bookkeeping when url/ref/ref_type change. last_commit_sha is the load-bearing one (a baseline is only meaningful for the ref it was recorded against); status/timestamp/error go with it because "Synced <date>" against a repo the source has never fetched is a claim the row can't support — and never already renders as "Never synced" in the panel, so no frontend change.

The negative is the part I'd ask you to look at hardest, because getting it wrong is a security regression rather than a UX one: non-identity edits (name/enabled/priority) must not clear it. If disabling a source cleared its baseline, re-enabling would re-clone with nothing to compare against, and a tag moved during the disabled window would be adopted silently — the AC#5 bypass, reachable by toggling a checkbox. That's parametrized (test_a_non_identity_edit_keeps_the_pin_baseline), and it's not hypothetical: the enabled toggle is currently the only edit the UI wires to PUT. Plus test_rewriting_a_field_with_its_current_value_is_not_a_change, since a form-driven UI PUTs the whole object back.

Both halves of your scenario are covered — the ordinary bump, and the fresh-clone variant where the refusal rmtreed first and left the source empty.

test_a_moved_tag_is_still_refused_after_all_this is the guard that the pin survived the fix that made bumping work.

3. Deleted source leaked its checkout — fixed

Both halves, as you suggested:

• discard_source_checkout removes <source_id>/ and <source_id>.broken on delete. Row first, disk second — the row is authoritative and a filesystem failure must not fail a committed delete. Result surfaces as checkout_reclaimed on the response and in the audit row.
• _reclaim_orphan_checkouts is the backstop, on full sweeps only (a single-source sync must not reach outside the source it was asked about), and it's the only reclamation path for installs that already accumulated clones. Logs what it reclaims, per bug: retention prunes have no blast-radius guard — a mistyped window deletes most of a table within 5 minutes #1644.

Fail-closed on the row read, which is the whole ballgame: a sweep that reads "DB error" as "no sources configured" deletes the entire library. test_reclamation_fails_closed_when_sources_cannot_be_read pins it. Scope is the server-minted id shape only, so the legacy pre-ent#237 .git checkout, the migration staging dir and anything an operator parked there are structurally out of reach — and a disabled source keeps its checkout (reclaiming there would discard the very checkout the tag pin is measured against).

4. Sync routes — you're right, gated

Agreed without reservation, and your framing is the correct one: grant-vs-use is a claim about effect, and the effect here is cloning executable material and spawning run_fleet_reinject. Both routes now carry reject_agent_principal, both are in MUTATING, and I widened the coverage scan from /skills/sources to /skills/ — POST /skills/library/sync sits outside the sources prefix, so the old scan would have reported full coverage while missing exactly the route under discussion.

The router comment and requirements/skills.md now record the rule as gate on what the route does, not on which verb it is, naming both misreads (LIST, then sync) rather than restating the axis that produced them.

No MCP tool touches either route, so nothing downstream breaks.

The one you didn't ask for

Mutation-testing #4 — deleting the reject_agent_principal call and expecting red — came back green. test_mutating_routes_reject_agent_principals checked "reject_agent_principal" in ast.dump(fn), and ast.dump renders docstrings, so the docstring I'd just written explaining the gate satisfied the guard on its own. The guard has been checkable-by-comment since it was written; it only surfaced because I happened to document the thing it checks for.

Now matched as an ast.Call node. Re-ran the mutation: red.

All four mutations (neutered repoint detection / removed identity-clear / neutered reclamation / removed auth gate) fail exactly their own tests and nothing else.

Non-blocking, as raised

• Ordering — agreed, and unchanged: I'd rather feat: Telegram bot connection UI in Settings (TGRAM-UI) #296 land first than fake it here. If it slips, the release note carries it.
• Docs — skill_source_clone's "point this source at a new tag" is now true rather than aspirational, and says so. architecture.md and requirements/skills.md carry all three fixes and the corrected auth rule.
• CI — the description's auth claim was an overclaim when you read it and is now simply accurate; corrected in place either way. The regression diff flake gets a fresh run off this push.

Verification

• tests/unit/test_ent237_skill_sources.py: 72 → 92 tests, all passing
• All skills suites together (ent183 compat / ent236 / ent237 / ent332 / SSRF / UA): 245 passing
• Full unit suite: 7322 passed, 18 skipped, 0 failed (the two bug(tests): test_ent183_skill_packages sys.modules stubs poison later tests (order-dependent CI flake) #1898 failures are gone since the dev merge)
• Mutation-verified per fix, as above

[obasilakis]

Follow-up on CI, since my numbers above are now superseded by a dev merge.

Why no checks were running. Not the Actions dispatch gap I assumed — the PR was mergeable_state: dirty against dev, so GitHub could not compute refs/pull/1901/merge and every pull_request-triggered workflow silently declined to dispatch. Same condition the 07-31 nightly bot reported. Merged dev in (a37db853); mergeable: true, checks now running.

The only conflict was tests/registry.json, additive on both sides — this branch appended the ent#332 entry, dev appended ten. Resolved as a union keyed on file, verified against the merge base: nothing removed on either side, and the one entry differing between sides (test_181_agent_display_label, hyphen → em dash) was dev's edit against an untouched copy here, so dev's kept. Also registered unit/test_ent237_skill_sources.py, which was never in the catalog — ent#332's entry landed but its parent's did not.

Corrected suite numbers (post-merge, fixed order):

	this branch	clean origin/dev
fixed order (-p no:randomly)	7619 passed, 18 skipped, 0 failed	7473 passed, 18 skipped, 0 failed
random order	2 failed	the same 2 failed

The random-order failures are test_1081_physical_meter.py::{TestDisjointBulkMeter::test_max_respects_506_clamp,TestPerAgentMeter::test_available_floors_at_zero_over_ceiling}[sqlite] — identical on clean dev, and named in #1898's own reproduction. An earlier random run here hit test_subscription_auto_switch_pingpong.py::TestKeyRolloverFanOut instead (fixture ERROR, _install_database_stub() + importlib.reload()); different seed, same class. This branch touches neither file — both byte-identical to dev — and none of its files pollute either pairwise.

So: pre-existing, tracked as #1898, not introduced here. Worth noting the shard matrix (base × head × 3 seeds) is precisely the right instrument for this and nets it out.

Everything else has gone green so far; the pytest shards are still running.

[dolho]

/review Report

Branch: feature/ent-237-multi-source-skills → dev (merge-base e6df5bf8)
Files Changed: 36 (+5789/-766)
Scope: CLEAN
Plan Completion: 4 of my 4 earlier findings DONE, verified in code — plus one they found themselves

All four things I asked for in the 3 Aug review are genuinely fixed in b92c3859, not papered over. Re-verified each against the code rather than the commit message, and mutation-tested two of them independently.

My finding	Status	Evidence
Editing url never repointed origin — silently synced the old repo	DONE	_origin_matches + _discard_repointed_checkout; mutation-verified below
Tag bump refused as moved_tag (stale last_commit_sha)	DONE	db/skill_sources.py:207 clears the pin bookkeeping on a real url/ref/ref_type change
Delete leaked the checkout	DONE	discard_source_checkout (row first, disk second) + _reclaim_orphan_checkouts backstop
Both sync routes role-gated only	DONE	reject_agent_principal on sync_library and sync_skill_source

Three of those are better than what I suggested, and the reasoning is the reason:

• Discard rather than git remote set-url — set-url 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. I hadn't thought that through.
• Pin cleared on url/ref/ref_type but deliberately NOT on enabled — clearing on disable would let a tag moved during the disabled window be adopted silently on re-enable. That is an AC#5 bypass via a checkbox, and the enabled toggle is the only edit the UI currently wires to PUT. Nice catch.
• The clear also compares against the current value (skill_sources.py:208), so a no-op PUT that resends the same URL doesn't disarm the pin.

And the self-caught one is the best item in the diff: test_mutating_routes_reject_agent_principals grepped ast.dump(fn), which renders docstrings — so documenting the new gate in prose made the guard pass with the real call deleted. Now matched as an ast.Call node. That is the third instance of this class on the repo (#1871's containers_run guard, ent#314's loader sweep) and I hit it myself today on #2019.

---

Informational Findings

[I1] Test gap: the fail-safe branch of an unattended rmtree is uncovered (Confidence: 9/10)
File: src/backend/services/skill_source_clone.py — _origin_matches

Evidence:

proc = self._git(["config", "--get", "remote.origin.url"], timeout=10)
if proc.returncode != 0:
    return True

The docstring argues this at length and cites #1638/#1644 — an unreadable origin counts as a MATCH because the action gated on the answer is an rmtree, so ambiguity must resolve to "don't". That reasoning is right, and it is exactly the property most worth pinning.

It isn't pinned. I flipped it to return False — making the discard fail-open, so a transient git config failure deletes the checkout — and ran the suite:

92 passed in 81.63s

Zero failures. The safety argument lives only in the docstring; someone can invert it in a refactor and CI stays green. Note the contrast with the other half of the same fix — neutering the origin check itself does fail (test_repointing_moves_the_git_remote_too, 2 failures), so the feature is covered and only the safe direction is not.

Suggestion: one test with _git stubbed to a non-zero return, asserting _origin_matches is True and that no rmtree happens. Cheap, and it is the assertion that makes the docstring load-bearing.

[I2] A repoint leaves the old repo's quarantine behind (Confidence: 7/10)
File: src/backend/services/skill_source_clone.py — _discard_repointed_checkout

shutil.rmtree(self.path, ...) removes <id>/ but not <id>.broken. discard_source_checkout handles both on delete, and _reclaim_orphan_checkouts only considers ids with no row — so for a live, repointed source the quarantine holding the previous repo's content persists indefinitely.

Bounded (one quarantine per source, pre-existing invariant) and not served to anyone, so it is disk hygiene rather than a correctness bug. Reusing discard_source_checkout's two-path loop here would close it in a line.

[I3] update_source's pin-clear decision is a read-then-write (Confidence: 6/10)
File: src/backend/db/skill_sources.py:205-216

current = self.get_source(source_id) and the UPDATE are separate statements, so two concurrent admin PUTs can decide "changed?" against a stale row. The security-relevant direction is a clear that shouldn't have happened (pin baseline dropped). Admin-only, requires concurrent edits of the same source, and the threat model for AC#5 is a moved upstream tag rather than a racing operator — so I'd note it, not block on it. Folding the comparison into the UPDATE's WHERE would remove the window if you want it gone.

Clean Categories

• SQL — all SQLAlchemy Core with bound params; no interpolation in the diff
• Auth — every mutating source route and the LIST route carry require_admin + reject_agent_principal; both sync routes now too, verified by reading the handlers
• Credential exposure — canonical_remote strips userinfo before comparing, and redact() still covers git output; the compare never logs the auth URL
• Race conditions — _reclaim_orphan_checkouts runs inside _sync_sources_locked under the cross-worker lock, and only on a full sweep (single_source is the caller's intent, not len(sources) == 1 — which is the trap an install with exactly one source would have fallen into)
• Destructive-op safety — the sweep is fail-closed on the row read, scoped to the server-minted src_<hex> id shape (legacy checkout and operator directories structurally out of reach), and logs what it reclaims
• Enum completeness — no new enum/status value in this diff

Summary

• Critical: 0 — my four findings are resolved and I could not construct a fifth
• Informational: 3 — I1 is the one I'd act on before merge; it is a one-test fix
• Scope: clean

Verification: 144 passed across test_ent237_skill_sources.py + test_ent332_skills_root.py; two fixes independently mutation-tested (results quoted above).

My 3 Aug CHANGES_REQUESTED is satisfied — happy for it to be dismissed. I'd take I1 first; I2/I3 are fine as follow-ups.