feat(skills): skill sets — assign a named slice of the library as a unit (ent#530)

[dolho]

Fixes abilityai/trinity-enterprise#530 (absorbs ent#342)

What

A library source's catalog.yaml may now declare skill sets: named families of its own skills. Assigning set:<name> assigns every member in one act, and the platform keeps the family current. A member added upstream is injected on the next re-inject; one removed upstream is pruned.

sets:
  project-management: [project-init, project-task]        # short form
  dev-backlog:                                              # long form
    skills: [backlog, roadmap, groom]
    requires: {env: [GITHUB_TOKEN]}
    schedules: [{name: Weekly groom, cron: "0 9 * * 1", message: /groom}]   # shown, never created

• Rows: members are materialised as agent_skills rows with individual = 0. A member also assigned on its own keeps 1.
• Unassign: unassigning a set removes only the members that no individual assignment and no other held set names. Each assignment stands on its own.
• Fails closed: while any held set is unresolved, no set-derived row is removed. The reasons are not_found, invalid, partial_upstream (which also covers an empty skills root) and source_changed (a disabled source must never swap in another source's family). A catalog hiccup never strips a fleet's skills.
• One transaction: every mutation ends in one transactional recompute under a per-agent lock: a PostgreSQL row lock, or a SQLite RESERVED lock taken before the reads. The bulk PUT resolves held sets inside its own transaction, so a set member you untick is demoted, never deleted. set: entries only add sets; the sets field replaces them.
• Honest status (epic: Fix test suite failures (13 failing tests) #342): each set reports ok, partial or unresolved, with per-member state and drift. Prerequisites cover the set's own env keys and its members', checked only while the agent runs (?probe=true), otherwise unknown.
• Why a skill is present: GET /skills carries individual and via_sets. The injected meta and the agent's CLAUDE.md say "via ".
• Governance: every set write is behind the ent#596 skill-manager fence. The route-graph guard was widened to /skill so the new routes are covered.

Surfaces

Layer	Change
API	GET /api/skills/library/sets, GET /api/agents/{a}/skill-sets, POST/DELETE /api/agents/{a}/skill-sets/{set}; PUT /skills accepts sets / set:
MCP	assign_skill_to_agent routes set:<name>; set_agent_skills forwards set: entries (add-only); get_agent_skills reports individual / via_sets / sets; new unassign_skill_set (fenced)
UI	Library → Skills lists sets, expandable to members with versions, with partial / invalid / shadowed badges, prerequisites and suggested schedules, plus assign. The agent's Skills tab shows set rows with status, "via " badges and set-only members locked; its bulk save sends individual skills only
DB	agent_skill_sets + agent_skills.individual on both tracks (SQLite migration + Alembic 0080_agent_skill_sets ← 0079_telegram_group_context); AgentRef CASCADE
Inject	start, manual Sync and fleet re-inject all reconcile set rows before reading names, then prune

Review and audit

• Independent pre-landing review: found 5 data-loss paths, all fixed with a regression test each and verified by reverting each fix:
  • a malformed set parsed as an ok set with no members;
  • an empty skills root;
  • a source swap;
  • unticking in the PUT deleted rows;
  • the PUT raced concurrent set writes.
• /cso --diff: 0 critical, 0 high, 0 medium, and 3 low, all fixed. The report is in docs/security-reports/cso-diff-2026-09-24-ent530-skill-sets.md.

Tests

• tests/unit/test_ent530_skill_sets.py: 51 tests. They cover the parser, the fail-closed rule, a real git repo, real SQLite, both migration tracks plus the single head, routes through a real FastAPI app, fleet re-inject, start and manual-inject ordering, and CLAUDE.md.
• Mutations: I reverted each of these and confirmed its tests go red:
  • the fail-closed rule;
  • the PUT keeping set-derived rows;
  • the individual-only save;
  • the invalid-set status;
  • the source-change check;
  • the SQLite lock.
• src/frontend/tests/unit/skillSets.spec.js: 16 mounted jsdom specs.
• MCP: skills.test.ts and access.test.ts pass, and tsc is clean.
• Suites: the frontend suite passed (3,576 tests), with the raw-colour, loading-gate and source-text ratchets green.
• Full backend unit run: 62 of 17,962 failed.
  • 1 was mine (a stub in test_ent236 that accepted one argument), now fixed.
  • 15 also fail on a clean origin/dev in this environment: mapped-IPv6 SSRF tests on a Python 3.12 venv.
  • 46 fail only under the full randomized order (the bug: an installed pytest-randomly silently randomizes local runs, surfacing latent order dependencies as phantom regressions #2891 class) and pass in isolation. They also pass when run after this PR's test files in a fixed order, so these files are not the polluter.

Notes

• Alembic head collision: feat(workspace): the autonomy dial — what a companion may do unprompted (ent#641) #2984 (0080_seat_ask_class_state) and feat(git-sync): the container pulls origin on its own (trinity-enterprise#703) #3021 (0080_pull_sync) also sit on 0079_telegram_group_context, so whichever of the three merges later must re-chain onto the live head. The single-head check passes against dev.
• Out of scope (trinity-pm owns them): the library validator and the CONTRIBUTING sets: rule in trinity-skills. The catalog schema is posted on the issue.

🤖 Generated with Claude Code

[dolho]

📸 Walkthrough: https://claude.ai/artifact/2QtMs5kR4NMZWLb4JQD3ei — Library → Skills sets (expanded members + versions, a partial set), the agent Skills tab (set status with a missing credential flagged, assigning a second set, via <set> badges, set-only members locked), light + dark, and the agent's own CLAUDE.md "(via dev-backlog)" lines. Taken on a local demo stack of this branch.

The screenshot pass found two UI issues, fixed in 5c12f71: a green "complete" badge beside the missing-credentials warning (now needs credentials), and the set picker rendering blank after an assign.

[github-actions]

✅ Alembic head check clear — merging this PR into dev leaves one head (0079_telegram_group_context).

Previously flagged; resolved.

Advisory — this check does not block merge. · head_sha: 0b3ab51ef69cb6996e3a17816e7f2fd95b02b17b · run

[github-actions]

✅ Nightly unit-suite clean when this PR is merged into dev, all 3 seeds (head_sha: 0b3ab51ef69cb6996e3a17816e7f2fd95b02b17b).

[github-actions]

⚠️ Live-instance suite skipped — merge conflict against dev.

Resolve by merging dev locally and pushing the result; the next nightly re-tests.

[vybe]

merge-train (2026-09-27): not on this run. Rides the next one once the four fixes below are in. Nothing was pushed to this branch. This was the PR's first validation pass: lane B + schema, head 5bf56993. No criticals, and the design holds up: sets are a grouping over ordinary agent_skills rows (individual = 0), delivery still goes through the unchanged inject_skills / remove_skills / reconcile_agent_skills, both migration tracks agree, and neither frontend baseline moved.

To fix before it rides

1. stores/skillsLibrary.js:343-358 (caller AssignedAgents.vue:192): the library-page unassign ignores the new removed: false / retained_via_sets answer and drops the holder chip while the agent still holds the skill through a set.
2. stores/skillsLibrary.js:360: the same function returns detail raw, and the new 409 skill_set_unresolved detail (routers/skills.py:656) is an object, so InlineError receives a non-string.
3. db/skills.py:253-290: set_agent_skills reads existing / kept_by / kept_status before it takes the agent lock. A set assigned in that window loses its member rows in the delete-all and only heals at the next reconcile. Read inside the lock.
4. tests/unit/test_ent530_skill_sets.py:421: assert MIGRATIONS[-1] == ("agent_skill_sets", …) fails as soon as any later migration is appended. Assert membership, not position.

For your call

• services/skill_set_service.py:336 with db/skill_sets.py:144-163: a PUT sets replace resolves an already-held set against its current source and never updates the stored source_id, so a set that moved sources swaps families and then reads unresolved/source_changed for good. No first-party client sends sets today.
• No MCP tool lists library sets or full set status, and the # mcp: header on routers/skills.py omits unassign_skill_set.
• db/skill_sets.py:79 imports plan_member_rows from the services layer (Invariant Fix: Add missing Docker labels to system agent container #1 runs the other way).
• AgentSkillSets.vue:11 and LibrarySkillSets.vue:15,27 hand-roll buttons where a Base primitive exists.
• No test reaches the container: inject_skills, deliver_assigned, remove_skills and reconcile_agent_skills are mocked everywhere, and the via_sets wiring at skill_service.py:1704, :1863, :1925, :2328 is never run.
• No reader yet for retained_via_sets, the agent-set member version and shadowed_source, the assigned_by / assigned_at columns, or the injected meta.via_sets.

When you rebase

dev is at 0076_operator_queue_ask_object and more revisions are landing today, so re-parent onto whatever is head when you push. The rename touches: the revision file (name, Revision ID, Revises, revision, down_revision), the docstring at db/migrations.py:4649, tests/unit/test_ent530_skill_sets.py:427, :430, :443, docs/memory/architecture/database.md:1114 and docs/memory/requirements/skills.md:117. The PR body and docs/security-reports/cso-diff-2026-09-24-ent530-skill-sets.md:22 still say 0073. The migrations.py tail and tests/registry.json are keep-both, dev's entries first; rebuild the registry from the stages, since this branch's entry has off-indent braces.

The red schema-parity and pg-migrations runs are the old SQLAlchemy 2.1 psycopg import error, fixed on dev by #3015; merging dev clears them.

[AndriiPasternak31]

Requesting changes. There are two things to fix, and both can go in the rebase you already need.

1. The PR no longer merges. alembic-head-watch is correct: dev now has 0075_auto_sync_enabled_backfill … 0079_telegram_group_context, and 0075_agent_skill_sets still sits on 0074, so merging would give two heads. migrations.py and tests/registry.json also conflict. To fix:

• renumber to 0080 on 0079_telegram_group_context;
• re-append the SQLite entry;
• update the pinned pair in test_the_alembic_revision_extends_the_single_head;
• update the stale 0073 in the cso report and PR body.

The pg-migrations and schema-parity reds are not from this PR. Both are No module named 'psycopg' (SQLAlchemy 2.1), which #3015 fixed on dev after your last run, so they should clear on the rebase.

2. set_agent_skills reads before it locks (db/skills.py L253-286 read, then lock_agent_rows at L290). A POST /skill-sets/X that commits between those reads and the lock has its member rows deleted. kept_by_set is built from the stale existing, and the delete-all removes the rest. I reproduced it on real SQLite: I stubbed lock_agent_rows so a concurrent assign_set committed first, and the agent ended up holding dev with rows {solo}. The next reconcile heals it, but this is the "a set assigned concurrently is never missed" guarantee from finding 5/9. Taking the lock as the first statement of the transaction (when set_resolver is given) fixes it. Please add a regression test for that interleaving.

Should fix or file as a follow-up:

• MCP has no way to list library sets. An orchestrator can assign_skill_to_agent("set:…") but cannot discover names or members. Add a list_skill_sets tool, and update the # mcp: header of routers/skills.py, which also misses unassign_skill_set.

Small items, not blocking:

• ?probe=true runs an exec for read-only principals; consider owner-gating it.
• A re-assign that re-points source_id keeps the old assigned_by.
• The new components use raw gray-* classes, not semantic tokens.
• Upstream sets: edits now add skill names fleet-wide as assigned_by="system". That is what the issue asks for, but it widens ent#662, and I'll note it there.

Everything else I checked holds up: the ent#596 fence on every set write, including the widened route-graph guard; the #2914 conflict protection for set members; the fail-closed reconcile on every inject path; unassign semantics; both migration tracks; and route order. Nice work on the review and mutation trail.

Also, can we get a one-line OSS-core ruling on ent#530? It's in the enterprise tracker with no gating decision, and I agree it should follow the ungated skills-library precedent.

[dolho]

Both blocking items, vybe's four fixes and the "should fix" item are in.

Blocking (Andrii)

1. 1ced057ed merges dev. migrations.py and tests/registry.json keep both sides, dev's entries first, and the registry is rebuilt from the merge stages. The revision is now 0080_agent_skill_sets ← 0079_telegram_group_context, and every reference to it is updated: the revision file, the migrations.py docstring, the pinned pair in the test, database.md, requirements/skills.md, the cso report and the PR body. check_alembic_heads.py: 81 revisions, 1 head. The psycopg reds cleared with the merge.
2. 70c0c9711: lock_agent_rows is now the first statement of the transaction whenever a set_resolver is given. The regression test commits a concurrent assign_set at the moment the lock is requested. It fails on the old order (the member rows are deleted) and passes now.

vybe's four

1. and 2. 87c1e42c5: library unassign keeps the holder chip and returns the server's sentence when the answer is removed: false with retained_via_sets. A {code, message} detail (the 409 skill_set_unresolved) now comes back as its message string from both unassignSkill and assignSkill. Two store specs, both failing before.
2. The same lock fix as blocking item 2.
3. The SQLite registration test now checks that the migration is registered, not that it is last.

Should fix

• 530b485ab: new list_skill_sets MCP tool (GET /api/skills/library/sets, access none like list_skills). The # mcp: header on routers/skills.py now names it and the previously missing unassign_skill_set. tsc is clean and 557/557 MCP tests pass.
• 0b3ab51ef: the two hand-rolled "retry" links on failed set reads are replaced with LoadFailed.

Not changed

• db/skill_sets.py still imports plan_member_rows from services. The planner is pure (no I/O), and it has to run inside the db transaction under the agent lock. Moving it out would mean threading a planner through four db methods. Happy to do it if you'd rather have the strict layering.
• Not touched: re-pointing source_id on a PUT sets replace, owner-gating ?probe=true, a re-assign keeping the old assigned_by, a test that reaches the container, and readers for the new fields.

Ruling on ent#530: OSS-core, ungated, following the skills-library precedent.

Backend: 742 across the ent530/596/2914/parity/migration files, and 657 across every skill test. Full vitest passes, ratchets included.

[dolho]

Ownership moves to @AndriiPasternak31 (ent#530 handover; after ent#500 lands). Everything from both reviews is addressed except the items listed as 'Not changed' in my last comment.

[AndriiPasternak31]

Approving. Both blockers from my last review are fixed. I checked the lock-order fix by running its regression test against the old db/skills.py: it fails there and passes on this head. CI is green, and the branch merges cleanly with one Alembic head. What's left is one should-fix and a few small items, and all of them can be follow-ups.

Previous findings

• ✅ Alembic head / rebase. The revision is 0080_agent_skill_sets ← 0079_telegram_group_context. check_alembic_heads.py reports 81 revisions and 1 head, and no 0080* has landed on origin/dev. The SQLite entry is re-appended last in MIGRATIONS (db/migrations.py:5013). The pinned pair is updated (test_the_alembic_revision_extends_the_single_head), the registration test now checks membership rather than position (test_ent530_skill_sets.py:423), and the 0073 references are gone from the cso report and the PR body.
• ✅ set_agent_skills reads before it locks. lock_agent_rows is now the first statement whenever a set_resolver is given (db/skills.py:252-254), and the router always passes one. The regression test test_a_set_assigned_while_the_replace_takes_its_lock_keeps_its_members fails on the pre-fix file with {'solo': True} != {'solo': True, 'backlog': False} and passes now.
• ✅ MCP list_skill_sets. The tool is added (GET /api/skills/library/sets, registered before /{skill_name}). Its access row is none (access.ts:284), and unassign_skill_set sits behind the ent#596 fence row, with the totality test extended to cover it. The # mcp: header now names both tools.
• ⚠️ ?probe=true for read-only principals. Not changed, and it's wider than I thought. See finding 1.
• ❌ Re-assign keeps the old assigned_by. Not changed. db/skill_sets.py:120-123 still only updates source_id. Fine as a follow-up.
• ❌ Raw gray-* classes. Not changed: for example AgentSkillSets.vue:3,5,21 and LibrarySkillSets.vue. The ratchet passes because gray is allowed in new files, but the design contract asks for semantic tokens. Fine as a follow-up.
• ✅ OSS-core ruling. It's posted on this PR (dolho, 2026-09-28): OSS-core and ungated, following the skills-library precedent.
• ✅ vybe's four merge-train items. All fixed. Library unassign keeps the holder chip and shows the server's sentence, and the object detail is rendered as its message in stores/skillsLibrary.js. The lock is the same fix as above, and the migration assertion now checks membership. Each has a store or unit spec.

New findings

1. [should-fix] src/frontend/src/stores/skills.js:180 with src/backend/routers/skills.py:698-712: the route docstring says probe is "off by default, so a plain read is never an exec amplifier (cso L1)". But the first-party Skills tab always sends probe: true, on every load() and after every set assign or unassign (_refreshRows → loadSets). The route is gated only by get_authorized_agent_by_name. So in practice any principal with read access to the agent, including a shared user, runs one in-container exec each time they open or refresh the tab. The exec only runs when the agent is running and a held set declares env prerequisites. The probe returns only booleans, so this is a bounded amplifier, not a leak. Still, the cso L1 mitigation doesn't hold for the path that actually gets used. Fix: honour probe=true only for principals that pass the skill-manager/owner fence and return unknown for everyone else, or have the UI probe only when the viewer can manage the agent's skills.

What I checked

• I read the whole diff at 0b3ab51ef against origin/dev, focusing on the four commits since my review.
• ent#596 (96795e23c) is in the branch and not bypassed. Every set write (POST/DELETE /skill-sets/{set}, the PUT sets field and the set: prefix) goes through get_skill_managed_agent_by_name. The only callers of assign, replace and unassign are those fenced routes. The widened route-graph guard and the refused-agent-key test cover the new routes.
• The fail-closed paths hold. plan_member_rows treats a set that is held but missing from resolved as unresolved, and library_sets() returning {} on failure also fails closed.
• No new admin endpoints, so the grant-vs-use and require_admin rules aren't touched.
• Backend tests: 683 passed and 2 skipped on seeds 12345 and 99999. This covers ent530, ent596, ent236, 2914, 2703, ent386, 384, ent237, ent332, ent183, 2991, schema parity, migrations, the Alembic parity/length/heads guards, cleanup and rename-cascade parity, models-centralized, 1310 auth wiring and the 293 admin gate. These ran on Python 3.14, not the image's 3.13. The PG FOR UPDATE lock path isn't exercised by any pytest here or in CI.
• MCP: npm test gives 557/557 and tsc --noEmit is clean.
• Frontend: npm run test:unit gives 3700/3700, ratchets included.
• CI and merge state: gh pr checks is all green or skipped. The PR is MERGEABLE, and it's blocked only by my review.
• Merge-order heads-up: #2984, #3021 and #3035 also add an 0080_* revision on 0079_telegram_group_context. Whichever lands later has to re-chain onto the live head.