Skip to content

feat(ui): Skills management surface — unhide and rebuild the Skills tab (ent#235) - #1877

Merged
vybe merged 2 commits into
devfrom
feat/235-skills-ui
Jul 29, 2026
Merged

feat(ui): Skills management surface — unhide and rebuild the Skills tab (ent#235)#1877
vybe merged 2 commits into
devfrom
feat/235-skills-ui

Conversation

@dolho

@dolho dolho commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

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 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. 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). Worth flagging: 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: description, automation, user_invocable, declared requires, multi-file count, size, and the git tree SHA as version. Dependencies are shown before assignment, because they're exactly what later becomes a missing_binary:* warning.

Assignment via the existing PUT .../skills with bulk save, plus dirty/reset so a half-made selection is recoverable.

Honest injection status — the load-bearing part. #183 reports injected | unchanged | fallback | failed with named warnings. The panel renders the verdict per skill and translates tokens into what they mean for this agent (missing_binary:jq → "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 held separately from assignment in the store precisely so durable assignment state can't be painted with a stale success.

Manual sync (force=True repair action) with in-flight state; a 409 from SkillInjectionBusy is 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:

library:      3 skills, contract fields rendered
bulk assign:  PUT .../skills → ['haiku', 'word-count'] persisted
agent start:  200
Sync now:     {"haiku": {status: injected, files_written: 2},
               "word-count": {status: injected, files_written: 2}}
UI:           version SHAs + `synced` badges + "Last sync 29/07/2026, 18:06:40"

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.

dolho added 2 commits July 29, 2026 18:07
…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
@dolho

dolho commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

/review — self-review

Branch: feat/235-skills-uidev · 3 files, +386/−282 (diffed against the merge-base, not the base tip)

Scope: CLEAN. One commit, one issue. All 9 ACs traced; the two that were not actually met are below. (A src/backend/enterprise line appears in a naive git diff dev — that is working-tree noise from a sibling worktree, not in the commit. Verified with git diff <merge-base> HEAD.)


Critical — both fixed in 7ac03171

[C1] The "Configure the library" CTA was a dead link (Confidence 10/10)

<router-link v-if="isAdmin" to="/settings?tab=skills">Configure the library</router-link>

There is no skills tab. Settings has general · access · integrations · mcp-keys · agent-permissions · security · sso · agents · retention · activation, and the Skills Library config lives under agents (Settings.vue, v-if="activeTab === 'agents'").

So the single call-to-action offered to an admin looking at an unconfigured library went nowhere — inside the "no dead empty states" AC this panel was built to satisfy. Now points at agents.

Related: I asserted in the PR body that the Settings panel already reported sync status / last-synced / skill count, without checking. It does report all three (skill_count, commit_sha, last_sync) — but I had the tab wrong, and checking is what would have caught that.

[C2] Every library error rendered as "empty library" (Confidence 9/10)

api.get('/api/skills/library').catch(() => ({ data: [] })),

The intent was to tolerate an unconfigured library. The effect was to swallow all errors — a 500, a timeout, an auth failure — and render "The library is configured but has no skills yet": a confident, wrong empty state pointing the operator at the wrong problem, which is the failure mode this panel is supposed to remove.

Fixed by fetching the list only when status.configured is true. The known empty state now comes from the status read; anything else surfaces as a real error.


Clean

XSS — no v-html; skill names/descriptions are {{ }}-interpolated (they originate from an admin-controlled git library, but are not trusted into markup either way). Invariant #6/#7 — new domain store, all HTTP via the shared api client; this PR removes the old panel's direct axios + hand-built auth header. Auth — tab gated on can_share && !isSystem, matching the sibling management tabs; no new endpoint. Stale-state — injection results are held separately from assignment, so a durable assignment can't inherit a stale success (verified in the store shape, and the "not synced from this screen yet" line covers the no-result case). Empty-state completeness — one discriminator in the store, so the panel cannot invent a fourth branch.

Informational — not fixed

[I1] SkillMeta drops a passed class (6/10) — the inline functional component renders its own class and doesn't merge fall-through attrs, so class="mt-1" at one call site is ignored. Cosmetic spacing only; verified the component itself renders correctly on the live instance (213 B / 277 B).

[I2] watch(() => store.assigned, resetDraft, {deep:true}) (5/10) — if assigned ever changed while a user had unsaved ticks, the draft would be silently reset. No polling or WS feed touches this store today, so it can't fire; worth remembering if one is added.


Verification

Re-ran the live probe after the fixes: tab present, 3-skill library renders, save button present, and the earlier end-to-end still holds — bulk assign → agent start → Sync now{haiku: injected/2 files, word-count: injected/2 files} with badges, version SHAs and last-sync line rendering from that response.

Critical: 2 (fixed) · Informational: 2 · Scope: clean

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vybe
vybe merged commit 8a768b6 into dev Jul 29, 2026
20 checks passed
@obasilakis

Copy link
Copy Markdown
Contributor

Heads-up from ent#237 (multi-source skills library) so we don't collide, @dolhoI have deliberately not touched SkillsPanel.vue, since you're rewriting it here.

Two things that are now available for the rebuild whenever it suits you (backend already shipped on feature/ent-237-multi-source-skills):

GET /api/skills/library entries carry two new fields:

{
  "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" }
  ]
}
  • source_name is a name only, never a URL — deliberate, because that endpoint is reachable by agent-scoped keys and a private repo's URL is itself sensitive. Source URLs live on the admin-only GET /api/skills/sources.
  • shadowed_by non-empty means another repo also ships that skill name and its copy is unreachable. ent#237 AC#4 requires the conflict be visible — "conflict surfaced in the UI, never a silent overwrite" — so a badge/tooltip on the row would be the natural home for it. Zero urgency; it's additive and the field is simply ignored until something renders it.

Also FYI: I've split the admin side into components/SkillSourcesPanel.vue + stores/skillSources.js rather than adding to stores/skills.js, specifically so this PR's file isn't in my diff. Different domain too — yours is per-agent assignment, mine is which repos the platform syncs from.

No action needed to merge this PR; I'll rebase ent#237 onto it.

vybe pushed a commit that referenced this pull request Jul 31, 2026
…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>
obasilakis added a commit that referenced this pull request Jul 31, 2026
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
obasilakis added a commit that referenced this pull request Aug 5, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants