feat(skills): library lifecycle automation — auto-sync, fleet re-inject, removal-on-unassign (abilityai/trinity-enterprise#236) - #1883
Merged
Conversation
…ct, removal-on-unassign Closes the three "Not Built" gaps in the skills lifecycle (requirements §21.1/§21.4). All three default OFF/no-op, so a zero-config install is unchanged. Removal-on-unassign - `compute_removal` is `compute_prune` against an empty new manifest, so path confinement, `..` rejection, and the cap live in one place and cannot drift. - `remove_skills` takes the SAME per-agent lock as injection (both mutate ~/.claude/skills and read-modify-write CLAUDE.md). Only manifest paths are deleted, so agent-authored files and runtime artifacts survive; a directory the removal empties is reaped via os.rmdir, which refuses a non-empty dir. - Wired into BOTH the single DELETE and the bulk PUT — the bulk PUT is the primary UI/MCP path and drops skills far more often. - The DB unassign is authoritative and always succeeds; a stopped agent, busy lock, or dead transport degrades to a named `removal_deferred:*`. - A partially-failed removal keeps the meta AND the .gitignore line: dropping the meta strands survivors as unmanaged orphans, and dropping the ignore line lets the 15-min auto-sync commit leftover injected files (#1595/#1596 class). Start-path reconciliation - The assignment row is gone by the time a stopped agent starts, so removal is reconciled, not replayed: the agent's platform-managed skill dirs are diffed against the assignment set. No tombstone table, no migration, and every removal route converges. Runs after injection and also at zero assigned skills — that is exactly the "unassigned the last skill" case. - Blast-radius guard: >10 removals for one agent refuses wholesale and alarms. A wiped agent_skills table is indistinguishable from a mass-unassign, and keeping files is the recoverable direction (#1638/#1644). Scheduled auto-sync + fleet re-inject - New leader-locked backend service (skills:sync:leader). Backend-hosted because the sweep must reach agent containers and the scheduler is platform-network-only. - Sweeps only when the library commit actually changed; running non-ghost agents; force=False so the ent#183 tree-SHA skip makes unchanged skills free; bounded concurrency; inject-lock contention is skip-and-report. - Honest aggregate report in Settings + operator alarm only when an agent failed. - Config re-read each cycle, so an interval change needs no restart. Durable sync status - `_last_sync`/`_last_commit_sha` were per-process; under `--workers 2` the worker answering /status was usually not the one that synced, so the panel showed a stale timestamp and could never show an error at all. Now mirrored to system_settings on both the success and failure branches, and the commit-changed comparison reads the durable row (the in-memory field would make every restart look like a change). Hardening found by /review and /cso on this branch - The persisted sync error is PAT-scrubbed: sync_library's outer handler passed a raw str(e), and the authenticated remote URL is an argument in the subprocess command list, so an OSError could carry a token into system_settings and the admin panel. - Cross-worker lock around the shared clone: scheduled + manual sync both run `git fetch` + `git reset --hard` on /data/skills-library. Contention returns 409 and writes no status row, so a contended click cannot paint "Last sync failed". - remove_skills re-reads assignments inside the lock and refuses a still-assigned skill, closing the bulk-PUT check-then-act race (#1445 pattern). - PUT /api/settings/skills-library is human-only (reject_agent_principal): it is the on-switch for an unattended fleet-wide write of SKILL.md files, which Claude executes as instructions. assert_admin answers "what role", never "is this a human" — third occurrence of the trinity-ops-agent#232 class. The remaining half of that chain (skills_library_url writable by an agent key via the generic settings PUT) is pre-existing and filed as Abilityai/trinity-enterprise#293. Dedicated range-validated GET/PUT /api/settings/skills-library; the three keys are blocked on the unvalidated generic PUT. 58 new tests. Fixes Abilityai/trinity-enterprise#236
obasilakis
force-pushed
the
feature/ent-236-skills-lifecycle-automation
branch
from
July 30, 2026 07:47
bd5f664 to
f55a6cd
Compare
|
Resolve by running |
…-lifecycle-automation # Conflicts: # docs/memory/learnings.md
…example `SKILLS_RECONCILE_MAX_REMOVALS` and `SKILLS_FLEET_INJECT_CONCURRENCY` were read via os.getenv() but never reached the container — the #1056 / trinity-enterprise#31 packaging class. The reconcile-refusal alarm names the first var in its own remediation text ("raise SKILLS_RECONCILE_MAX_REMOVALS"), so on a deployed stack the operator was told to turn a lever that does not exist, leaving a legitimate mass-unassign permanently blocked at the default cap of 10. Found by /validate-pr on PR #1883.
4 tasks
…an honest Sync-now affordance Three cosmetic fixes plus one UX gap on the agent detail tabs. - SkillsPanel had no card wrapper, so its content sat flush against the tab bar unlike every sibling tab. Wrapped in the same card the other panels use. - FoldersPanel and InfoPanel each had a `v-else` content wrapper with no spacing class, so the root `space-y-6` never reached the cards inside and they rendered touching. Same bug in both files. - SkillsPanel: saving assignments writes the DB rows, but the files only reach the container on a sync or the next agent start. That was stated in muted text above the button, which is easy to miss — an operator can save, message the agent, and be told the skill doesn't exist, because it isn't there yet. "Sync now" now goes prominent while that gap is open, and only while the agent is running (shouting at a disabled control helps nobody). A failed or 409-busy sync keeps it lit, since the gap is still open.
8 of the agent-detail tab wrappers use p-6 (overview, info, brain, dashboard, schedules, playbooks, git, folders). Settings and Skills had none, so their cards ran flush into the enclosing panel's left and right edges instead of sitting inset like the rest.
…ke the panel's failures actionable Three fixes found while testing this PR on a live instance. All pre-existing on dev, but each is sharpened by putting sync on an unattended timer. 1. sync_library() chose pull-vs-clone on `library_path.exists()` — directory existence, not repo-ness. A path holding no `.git` (clone interrupted by a full disk, a stray mkdir, a restored backup) failed `git pull` with "not a git repository" on every attempt, with nothing able to re-clone: permanent, and recoverable only by shell access. Now tests for `.git` and re-clones. Detecting it is only half a fix, since `git clone` refuses a non-empty destination — so a non-repo directory is moved aside first. Renamed, never deleted: the platform owns the path exclusively so removal would probably be safe, but "probably safe" is not the standard for an unattended timer deleting a directory derived from an operator-supplied setting (#1638/#1644). One quarantine is kept, so a recurring fault cannot grow without bound. 2. syncSkillsLibrary() saves settings first (setting showSuccess), then syncs; the catch set `error` without clearing it, so a failed sync showed "Settings saved successfully!" and "Clone failed" together. Both true, which is precisely what makes the pair untrustworthy. 3. The panel never said a private library needs a GitHub PAT — it is in two internal docs only, and what an operator hits is raw git ("could not read Username"), which names no remedy and does not hint that the PAT field is one section above. Added that line. The error-string mapping is deliberately NOT done: matching git stderr is brittle across versions and locales, and the raw error is at least surfaced rather than swallowed. Tests: 5 new (4 fail without the fix). Also fixes 7 existing tests that created the library path without `.git` — under the corrected predicate they took the clone path and reached for the network; the suite drops 5.01s -> 0.62s.
…-lifecycle-automation # Conflicts: # docs/memory/learnings.md
vybe
approved these changes
Jul 31, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
Validated via /validate-pr: requirements §21 + architecture + feature flows updated, ent#236 tuning vars wired into both compose files and .env.example, no secrets, blast-radius guard on reconcile, full CI green (6 pytest runs). Cross-tracker close — will set status-in-dev on ent#236 manually.
obasilakis
added a commit
that referenced
this pull request
Aug 4, 2026
…ecycle automation ent#236 (PR #1883) landed on dev BEFORE this branch rather than after, so it rewrote the same sync entry points ent#237 was replacing. The reconciliation, not the textual conflicts, is the substance of this merge. Kept from ent#236, adapted to N sources: - The cross-worker sync lock now wraps the multi-source loop (`_sync_library_locked(url)` -> `_sync_sources_locked(sources)`). ONE lock for the whole sweep: per-source locking would let two workers interleave and each publish a merged listing built from a half-updated set of checkouts, and the listing, the cache invalidation and the durable status are all library-wide. - Durable sync status (`skills_library_last_*`) stays library-wide and is still written on BOTH branches; per-source truth is on each `skill_sources` row. - `commit_changed` — the gate the fleet re-inject fires on — is computed per source against that source's own DURABLE `last_commit_sha` and OR'd, so a fresh process cannot read "changed" and sweep the fleet on every restart. - The non-repo-directory quarantine is ported into `SkillSourceClone`, where clone-vs-update now lives. Without it ent#236's forever-fail fix would have been silently dropped per source. Superseded and removed: `_git_clone` / `_git_pull` / `_get_current_commit` / `_quarantine_non_repo_dir` on SkillService — `SkillSourceClone` owns one checkout's git lifecycle and `skill_service` orchestrates N of them. Settings.vue keeps ent#236's automation card (auto-sync, interval, fleet re-inject) alongside <SkillSourcesPanel />; taking this branch's side wholesale would have deleted a shipped feature. Only the single-library URL/branch/sync controls the source list supersedes are dropped. Its loader is wired into the admin-only loaders — the old mount call was auto-merged away with the deletion. Also caught in the auto-merged (non-conflicting) regions: - `skills_sync_service` read top-level `commit_sha`/`action`, which the multi-source return no longer had. `commit_sha` is back as an explicitly documented library-wide summary marker; the audit row now records the per-source breakdown instead of one arbitrary source's action passed off as the library's. - The default source ref was pinned to a `v1.0.0` tag that will never exist — ent#296 cuts v0.1.0. A fresh install would have seeded a source that can never sync, failing quietly. Documented in .env.example. Tests: two files stub `utils.url_validation` with only one symbol while this branch imports ALLOWED_SKILLS_LIBRARY_HOSTS from it; the stub installs only when the module is absent from sys.modules, so this passed CI on file order alone and failed in isolation on the pre-merge tip too. Both stubs completed. Nine ent#236 tests drove the deleted single-repo API and are retargeted to the new seams — the properties (durable status, PAT scrubbing, commit-changed-vs-durable-row, quarantine) are unchanged and now exercise real code paths. Full unit suite at seed 12345: 7248 passed / 0 failed (clean dev: 7176 / 0). Refs Abilityai/trinity-enterprise#237, Abilityai/trinity-enterprise#236
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the three skills-lifecycle gaps documented as "Not Built" in requirements §21.1/§21.4. All three default OFF/no-op — a zero-config install behaves exactly as before.
DELETEand the bulkPUT(the primary UI/MCP path, which drops skills far more often)._last_syncwas per-process, so under--workers 2the panel showed a stale timestamp and could never show an error at all.Design decisions worth knowing
Reconciliation, not tombstones. Every removal route converges (single DELETE, bulk-PUT shrink, direct DB edit) with no schema change. It runs after injection and also at zero assigned skills — that's exactly the "unassigned the last skill" case an early return would strand forever.
Blast-radius guard. A reconcile proposing >10 removals for one agent refuses wholesale and alarms. A wiped
agent_skillstable is indistinguishable from a legitimate mass-unassign, and keeping files is the recoverable direction (#1638/#1644 discipline; the alarm carries counts, never skill names).Backend-hosted, not the standalone scheduler. The sweep must reach agent containers; the scheduler sits on
trinity-platformonly.Only platform-written files die. The delete set is the previous injection's own manifest, so agent-authored files and runtime artifacts survive;
os.rmdirrefuses a non-empty directory, so a skill dir holding agent files stays.Hardening found by
/reviewand/cso --diffon this branchFour issues were found and fixed here, not deferred:
sync_library's outer handler passed a rawstr(e)into a newsystem_settingsrow rendered in Settings, and the authenticated remote URL is an argument in thesubprocess.runcommand list. Now scrubbed at the write (canary G-04 class).git fetch+git reset --hardon/data/skills-library. Two admins colliding was never realistic; a scheduled loop landing on a manual click is routine. Cross-worker lock added; contention returns 409 and writes no status row, so a contended click can't paint "Last sync failed".previous - newis computed before the lock is taken, so concurrent PUTs could each hand the other's newly-assigned name to the remover. Assignments are now re-read inside the lock and a still-assigned skill is refused (bug: valid webhook token intermittently returns 404 under concurrent load #1445 pattern).PUT /api/settings/skills-libraryenables an unattended fleet-wide write ofSKILL.mdfiles — which Claude executes as instructions, not data.assert_adminanswers "what role", never "is this a human": an agent-scoped MCP key resolves to its owner carrying the owner's role. Nowreject_agent_principal. Third occurrence of the trinity-ops-agent#232 class (bug: retention prunes have no blast-radius guard — a mistyped window deletes most of a table within 5 minutes #1644, bug: trinity-system never adopts a rebuilt base image — ensure_deployed short-circuits on 'already running' #1816).skills_library_urlwritable by an agent-scoped key through the generic settings PUT, with the SSRF guard checking only the host — is pre-existing and still open, filed asabilityai/trinity-enterprise#293(P1). It's filed privately rather than here because publishing an unfixed exploit chain on a public repo is the wrong trade. That issue asks for a structural fix rather than a fourth per-endpoint patch.Changes
services/skills_sync_service.pyservices/skill_service.py,services/skill_packaging.pyrouters/skills.py,services/agent_service/lifecycle.py,main.pyrouters/settings.py,services/settings_service.py,models.py,views/Settings.vuearchitecture.md,skill-injection.md,skills-library-sync.md,learnings.mdtests/unit/test_ent236_skills_lifecycle.py(58),test_telegram_webhook_backfill.py(stub kept current)Test Plan
pytest tests/unit/test_ent236_skills_lifecycle.py -q→ 58 passedorigin/devbaseline, three orderings:origin/devwith that seed (pre-existing order-dependence intest_1089_*)node scripts/check-design-tokens.mjs→ all references resolveskill_packaging.pyis backend-only, Invariant Setup improvements #5 unaffected)Fixes abilityai/trinity-enterprise#236
🤖 Generated with Claude Code