feat(skills): vendor-neutral library layout — per-source skills root (trinity-enterprise#332)

[vybe]

Summary

• Makes the skills-library source layout resolvable per source: catalog.yaml skills_root: → evidence-gated skills/ probe → .claude/skills/ fallback. Existing sources keep working with zero config.
• Unblocks the public community catalog (abilityai/trinity-skills, ent#296), which already ships skills/<name>/ + a root catalog.yaml and currently syncs to zero skills.
• The agent-side destination is unchanged — filter_skill_archive rewrites tar arcnames to the canonical .claude/skills/<name>/…, so manifests, prune/removal confinement, gitignore lines and the whole ent#236 machinery stay destination-canonical with zero migration.

Base branch is feature/ent-237-multi-source-skills, not dev — AC#4 requires this to land before ent#237 merges. The repo is untagged with zero syncing instances today; after the first v* tag this becomes a breaking migration instead of a rename.

Design

SkillSourceClone.skills_rel_root() owns resolution (per source by construction; lazily cached — instances are per-operation, so no staleness across syncs). Every invalid tier falls through to the next rather than blanking the source; only the final tier escaping containment yields an empty listing, None/empty-propagated so list_skills never raises.

filter_skill_archive(source_root=…) is the ONE point where source layout becomes agent destination. That placement is required, not tidy: executable_paths → the chmod list, the restore sent-vs-restored accounting, the persisted manifest, and _legacy_fallback's SKILL.md lookup all key on post-filter arcnames.

Three independent guards on the author-controlled value, each catching what the others structurally cannot:

Guard	Catches what the others can't
Segment-wise validation (reject empty/./.. segments)	A whole-string charset regex admits ., ./skills, skills//x — each breaks prefix math into per-skill "empty package" failures
lstat symlink refusal + bounded read(cap+1)	A post-read length check is defeated by catalog.yaml -> /dev/zero; a symlinked skills/ lists fine but yields empty git archive output from HEAD
realpath containment vs the realpath'd base	/data/skills-library may itself be a symlink — comparing against an unresolved base refuses every source spuriously

Parsing uses the shared ent#314 hardened loader (AliasPolicy.REJECT, 64 KB cap), catching HardenedYamlError explicitly alongside yaml.YAMLError (it is a ValueError — missing it would 500 the merged listing). schema_version gates the whole catalog so a future bump degrades instead of being misread.

Dual-layout guard: evidence under BOTH roots with no catalog to decide keeps .claude/skills/ and flags layout_conflict. A pre-existing dual-layout source must never silently flip which executable content the ent#236 unattended auto-sync pushes fleet-wide.

Changes

File	Change
services/skill_source_clone.py	skills_rel_root() + resolution tiers, validate_declared_root, skills_root() -> Optional[str], all five path/git reads derive from the resolved root (-- on every pathspec)
services/skill_packaging.py	filter_skill_archive(source_root=…) + arcname rewrite; defensive re-validation (ValueError on unsafe roots)
services/skill_service.py	Inject-loop wiring, honest per-source path provenance, skills_root/layout_conflict in library status
components/SkillSourcesPanel.vue	Stale operator copy (.claude/skills/… was stated as the convention)
docs/	requirements §21.1.4, architecture, feature-flow + changelog, learnings entry

Test Plan

• pytest tests/unit/test_ent332_skills_root.py -v — 52 new tests: resolution order, zero-config back-compat, dual-layout, every bad-catalog shape, symlink/containment tiers, cross-layout version parity, arcname rewrite + prefix-escape resistance, _legacy_fallback with rewritten members, end-to-end mixed-layout merge
• Full unit suite: 7179 passed, 19 skipped (hypothesis-dependent files excluded — missing dep locally, pre-existing)
• ent#237 / ent#183 / ent#236 / ent#314 suites green unmodified (fallback path proves back-compat)
• Prior ent#237 HIGH (moved-tag on the clone path) verified still resolved

Review

• Independent strategy + engineering reviews (/autoplan) — both independently found the same hole in the plan's stated validation (skills_root: "." was claimed rejected and wasn't); fixed with segment-wise validation before implementation.
• Second-opinion model review — approved; its four implementation notes (int/string schema_version, realpath base, None-propagation, symlink fail-safe) are all folded in.
• /cso --diff — zero findings introduced by this change. Actively verified: no member name under any source_root can escape the destination prefix; every unsafe root raises. Report: docs/security-reports/cso-diff-2026-08-04-ent332-skills-root.{json,md}.
• One pre-existing disclosure (inherited from dev, unrelated to this diff) was surfaced during the audit and filed separately as trinity-enterprise#334 rather than scope-creeping this PR.

Fixes abilityai/trinity-enterprise#332
Refs abilityai/trinity-enterprise#237, abilityai/trinity-enterprise#296

🤖 Generated with Claude Code

[dolho]

/review Report

Branch: feature/ent-332-per-source-skills-root → feature/ent-237-multi-source-skills (merge-base 0bd6a8c5)
Files Changed: 12 (+1162/-30)
Scope: CLEAN
Plan Completion: 5 done / 0 partial / 0 not done / 0 changed / 0 unverifiable

Verified locally rather than taken on trust (see I1 — CI did not run the suite):

tests/unit/test_ent332_skills_root.py .................... 52 passed in 3.92s
tests/unit/test_ent236_skills_lifecycle.py ............... 64 passed
tests/unit/test_ent237_skill_sources.py + test_ent183_skill_packages.py + backward_compat + user_agent — green

I also empirically checked the one silent-semantics change in the diff — git ls-tree -z HEAD -- "skills/" vs the previous no--- form — on a scratch repo. Identical output, tree entries listed as before. tree_shas() is unaffected.

Plan completion (against trinity-enterprise#332 AC)

AC	Verdict	Evidence
Root resolution catalog.yaml → skills/ probe → .claude/skills/, zero-config back-compat	DONE	skill_source_clone.py:_resolve_rel_root + test_legacy_layout_needs_zero_config
Every path honors the resolved root (walk, tree-SHA, packaging, _skill_dir containment)	DONE	skill_names/tree_shas/archive_skill/skill_dir all derive from skills_rel_root(); service-side access funnels through clone.skill_dir (skill_service.py:735,748,840,1388) and clone.skill_names (214,693) — no residual hardcoded source root, verified by grep (remaining .claude/skills hits are all agent-side destination, which is the point)
Root resolved independently per source	DONE	instance-scoped _rel_root; instances are per-operation
Lands on the ent#237 branch before it merges	DONE	base branch is correct
Tests cover probe order, override, fallback	DONE	52 tests

Critical Findings (block merge)

None. The three-guard structure (segment-wise validation / lstat-before-open with bounded read(cap+1) / realpath containment against the realpath'd base) is the right decomposition, and I could not construct a member name under any accepted source_root that escapes dest_prefix: filter_skill_archive requires name.startswith(src_prefix), rejects "/../" on the full name before slicing, and the rewrite is dest_prefix + rel where rel is the vetted remainder (skill_packaging.py:261-282). HardenedYamlError being caught explicitly alongside yaml.YAMLError is the right call — it is a ValueError and would otherwise 500 the merged listing.

Informational Findings

[I1] Test Gaps: no Python test or CodeQL job ran on this PR at all (Confidence: 10/10)
File: .github/workflows/backend-unit-test.yml:19-22, .github/workflows/codeql.yml:10-13
Evidence:

on:
  pull_request:
    types: [opened, synchronize, reopened]
    branches: [dev, main]

Issue: both gates are branch-filtered to dev/main. This PR targets feature/ent-237-multi-source-skills, so gh pr checks 1998 shows build / e2e / gitleaks / guard / prod-image-smoke / schema-parity / verify-* and no pytest, no CodeQL — 1162 lines including a new YAML-parsing and path-resolution surface merged with zero automated Python coverage in CI. The test plan's "7179 passed" is a local claim the merge gate never reproduced.

This is structural, not your mistake — but the consequence lands on someone: the first CI run of this code happens on #1901's PR, where a failure gets attributed to ent#237. Suggestion: either note in the description that the ent#237 PR's run is the real gate (and re-check it after this merges up), or extend the two workflows' branch filters to feature/**. I'd favor the latter as a small separate PR — stacked PRs are going to keep happening.

[I2] Product Quality Bar: layout_conflict is API-only — nothing renders it (Confidence: 9/10)
File: src/backend/services/skill_service.py:900-901, src/frontend/src/components/SkillSourcesPanel.vue
Evidence:

"skills_root": clone.skills_rel_root() if cloned else None,
"layout_conflict": bool(clone.dual_layout) if cloned else False,

grep -rn "layout_conflict\|skills_root" src/frontend/src/ → no matches. The Vue change in this diff is three lines of descriptive copy only.

Issue: the dual-layout case is precisely the one you argue must not resolve silently — "a pre-existing dual-layout source must never silently flip which executable content the ent#236 auto-sync injects fleet-wide". Today it resolves silently to the operator: the decision is a logger.warning plus a boolean in a status payload no UI consumes. An operator who restructures their repo and expects skills/ to win gets .claude/skills/ with no visible signal, and the fix (declare skills_root) is undiscoverable from the screen.

Suggestion: render both fields in SkillSourcesPanel — resolved root as a chip on each source row, and a warning badge when layout_conflict with the "declare skills_root in catalog.yaml to switch" remedy. Small, and it's the difference between a designed fallback and a silent one. If you'd rather not touch that file while #1877 is in flight (same reasoning you applied on #1901), a follow-up issue is fine — but please file it rather than leaving the field unread.

[I3] Consistency: layout resolution reads the working tree, packaging reads HEAD (Confidence: 7/10)
File: src/backend/services/skill_source_clone.py:_declared_root, _probe_skills_layout vs archive_skill/tree_shas
Issue: catalog.yaml and the skills/ evidence probe are filesystem reads of the checkout; git archive HEAD and ls-tree HEAD read the committed tree. For platform-managed clones these agree, and reset --hard / checkout --force keep them agreeing for tracked content. The gap is untracked content: an untracked catalog.yaml or skills/ surviving in the clone dir would redirect resolution to a root that yields an empty archive, i.e. every skill in that source injects as an empty package rather than failing loudly.

Nothing writes untracked files into these directories today, so this is a latent inconsistency, not a live bug — which is why it's informational. Worth one sentence in the skills_rel_root docstring recording that resolution is working-tree-sourced deliberately, so a future change that starts writing into clone dirs knows what it's stepping on.

Clean Categories

• SQL & data safety — no SQL, no schema, no migration in the diff
• Auth boundaries — no new endpoints; get_library_status is unchanged in its gating
• Credential exposure — new log lines carry source_id, an OSError, or the declared root string; no values. redact() untouched
• Race conditions — _rel_root/dual_layout are instance state on per-operation clones (skill_service._clones constructs fresh ones per call); no cross-worker or cross-call sharing
• Enum completeness — no new enum/status value
• Error handling — every resolution tier falls through rather than raising; skills_root() -> Optional[str] is None-propagated at both consumers (skill_dir, skill_names), so list_skills cannot TypeError
• Docs staleness — architecture, requirements §21.1.4, feature-flow and the learnings ledger are all updated in-diff; the learnings entry on path-shaped author config is a genuinely reusable one

Merge-order note (not a finding against this PR)

The base branch #1901 currently carries four unresolved change requests from my review, two of which live in skill_source_clone.py — the file this PR extends: editing a source's url never repoints origin (silently keeps syncing the old repo), and bumping ref to a new tag is refused as moved_tag because last_commit_sha is never cleared. Neither is touched or worsened here, but both need fixing in the same file, so the stack should resolve #1901 first rather than rebasing this on top of a moving fix.

Summary

• Critical: 0 — none found
• Informational: 3 — I1 (CI blind spot) is the one I'd act on before this merges upward
• Scope: clean

Nice work on the _refuse_moved_pin_after_clone interaction — the fresh-clone tier of the pin is the one the ledger entry above says gets missed, and this diff keeps it intact through the root change.

(Skill Step 7 note: no new durable bug class to add — the author's own learnings entry already covers the path-validation lesson. The stacked-PR CI gap is a process finding; I'm leaving it in this review rather than committing to learnings.md from an unrelated working branch.)