fix(skills): refuse to overwrite an agent-authored skill of the same name (#2914)

[dolho]

Fixes #2914

What

Assigning a library skill whose name matched a .claude/skills/<name>/ the agent had written itself overwrote the agent's copy, gitignored + untracked the directory, and left only an unmanaged_dir_overwritten warning in an API response the Skills-tab operator never sees. The library and the agent's repo share one flat namespace on the agent side, so a name match proves nothing — the library silently won.

Inject path (skill_service._inject_skills_locked) — a directory that exists on the agent without the platform's .trinity-skill.json marker is now refused before a byte is staged: no git archive, no restore call, no .gitignore line, no untracking, not listed under CLAUDE.md Platform Skills. Applies on every trigger (assign, manual Sync with force=True, start path, fleet re-inject) — force is a repair of platform-written packages, never permission to replace agent work. Per-skill status is conflict — neither injected nor failed; overall success stays true so the fleet sweep does not raise an operator-queue alarm on every auto-sync for a standing conflict. skills_conflict + conflicts[] ride the result; deliver_assigned reports conflict (every requested name collides) or partial + conflicts[].

Recorded on the row — agent_skills.delivery_status ('conflict' / NULL). Stamped by the inject path, cleared by the next injection where the name lands, carried across the bulk-replace PUT for retained names (the PUT only re-injects added names, so it would otherwise go quiet until the next start), gone on unassign. Dual-track: SQLite agent_skills_delivery_status + Alembic 0065_agent_skills_delivery_status (IF NOT EXISTS, matching 0046–0062); schema.py/tables.py updated; check_alembic_heads + check_alembic_parity pass.

Skills tab — reads the verdict off the assignment rows (stores/skills.js::conflictNames), so on a fresh load the skill shows a name conflict badge, which directory the agent owns, that the agent's copy is what runs, and the two ways out — an inline Unassign library skill (BaseButton) or rename/remove the agent's directory and sync. inject() re-reads the rows so a resolved conflict clears without a reload. deliveryText gains a conflict arm and never arms the Sync nudge for it (Sync refuses again by design). Primitives (BaseBadge, BaseButton), tokens only; raw-colour ratchet unchanged.

MCP — delivery passes through; sync_agent_skills names conflicts on its success branch; get_agent_skills carries delivery_status per row + a conflicts[] summary; descriptions name the vocabulary.

Kept as-is: a platform-managed dir (marker present — including one a pre-#2914 assignment overwrote) keeps upgrading in place (AC#4/#6); an unreadable probe (_read_agent_skill_metas → {}) keeps the pre-existing fail-open direction rather than refusing the whole start path on a transient exec fault — documented in code and in the test.

Acceptance criteria

• Agent-authored dir survives byte-for-byte, stays tracked — test_agent_authored_dir_survives_byte_for_byte (both force arms: no archive / restore / finalize / delete)
• Honest conflict status, not injected, not a post-hoc warning — test_conflict_is_neither_injected_nor_failed, test_delivery_names_the_conflict_explicitly
• Skills tab shows which skill, that the agent's copy runs, with a next action — SkillsPanel.vue + store specs
• Platform-managed dirs upgrade in place — test_platform_managed_dir_still_upgrades_in_place
• Resolving clears on next sync/inject — test_conflict_is_recorded_on_the_row_and_landed_names_are_cleared, test_set_skill_delivery_status_stamps_and_clears_only_named_rows, inject re-reads the rows… spec
• Pre-existing overwrites not made worse — same test as AC#4 (marker present ⇒ managed)
• Tests: tests/unit/test_2914_skill_name_conflict.py (15), test_ent183 updated, frontend skillAssignDelivery.spec.js (+5), MCP skills.test.ts (+3)

Verification

pytest unit/test_2914_skill_name_conflict.py unit/test_ent183_skill_packages.py unit/test_2703_skill_assign_delivery.py unit/test_ent236_skills_lifecycle.py unit/test_ent237_skill_sources.py
264 passed
pytest unit/test_schema_parity.py unit/test_alembic_parity_guard.py unit/test_alembic_revision_id_length.py
96 passed
scripts/ci/check_alembic_heads.py → 66 revision(s), 1 head — PASS
scripts/ci/check_alembic_parity.py HEAD~1 HEAD → PASS — both tracks updated
SQLite migration applied to an old-shape DB twice (idempotent), existing row reads NULL
vitest skillAssignDelivery.spec.js (21) + rawColorRatchet + loadingGateRatchet → 36 passed; vite build OK; check:tokens OK
mcp-server npm test → 442 pass; tsc --noEmit clean

Full unit suite (pytest unit/ -n 6 --dist loadfile): 16711 passed, 31 skipped, 16 failed — the 16 are pre-existing on the base commit 0c470c76c (origin/dev): 15 in test_736_a2a_outbound_edges / test_ent14_registry_url_ssrf / test_ent399_ipv6_origin / test_mcp_validator (IPv6-mapped-address parametrizations, environment-specific — reproduced on a clean detached checkout of the base: 15 failed, 414 passed) plus test_2814_workflow_trigger_parity::test_every_accepted_entry_names_a_real_divergence (stale ACCEPTED_UNTIL_RELEASE entries now that main declares them). None touch skills.

/verify-local --skip-unit --skip-agent: pass — backend image build + import smoke OK, isolated sibling stack boot+health OK (the SQLite migration ran at boot), integration suite 70 passed, 13 skipped (all skips are the pre-existing agent-gated ones: TRINITY_*_TEST_AGENT unset / testfix agent absent).

Live check (dev stack, PostgreSQL, agent sidekick, this branch mounted): Alembic 0065 applied at boot (alembic_version = 0065_agent_skills_delivery_status, delivery_status column present). Planted an agent-authored .claude/skills/add-backlog/SKILL.md (committed in a temp repo, no marker), then:

step	result
POST /agents/sidekick/skills/add-backlog	delivery.status: conflict, conflicts: ["add-backlog"], per-skill error: name_conflict: …
row	delivery_status = 'conflict'
agent side	SKILL.md sha256 unchanged, still git ls-files-tracked, no .trinity-skill.json, no .gitignore line, not in CLAUDE.md
POST …/skills/inject (Sync, force=True)	skills_conflict: 1, status conflict, sha still unchanged
bulk PUT adding add-memory beside it	add-memory injected (marker + gitignore line + CLAUDE.md), add-backlog row keeps conflict
Skills tab (Playwright, light + dark)	name conflict badge, explanation naming .claude/skills/add-backlog/, "Unassign library skill" visible
click Unassign library skill	note "Unassigned add-backlog — the agent's own skill stays.", card gone, row deleted, agent's SKILL.md sha unchanged
re-assign → conflict again; agent removes its dir → Sync	injected, row cleared to NULL (AC#5)

Found while live-testing, not introduced here: BaseButton variant="secondary" has a transparent border in the light theme (the #2662 class inside the primitive) — filed as #2921.

Docs

docs/memory/requirements/skills.md §21.4 (new bullet), docs/memory/feature-flows/skill-injection.md (delivery vocabulary, key behaviours, result contract, what-the-user-is-told), docs/memory/architecture/database.md (column note).

🤖 Generated with Claude Code

[dolho]

[vybe]

merge-train: ejected from the 2026-09-21 train — rides the next one once fixed. Nothing was pushed to this branch.

Saying the good part first, because it is most of the PR: the tests genuinely execute the changed path. I reverted only services/skill_service.py to dev in a throwaway worktree, kept your tests, and 9 of 15 went red; restored, 15/15. The six survivors are the real-DB and behaviour-preserving cases, which is correct. No readFileSync, no inspect.getsource, no source-text stand-ins — the only toContain hits assert the return value of deliveryText(...). The dual-track migration is complete and correct on all four artifacts, and check_alembic_heads.py passes on the merged tree (66 revisions, 1 head). delivery_status is threaded to a live consumer end to end — row → AgentSkill response model → store → badge — and I checked every status consumer for an else ⇒ failed fallthrough the new enum value would break. There are none. Durable-state-on-the-row over a field-on-the-response is the right design, and inverting the ent#183 test rather than deleting it was the right call.

What blocks it is that marker-absence is treated as one case, and this PR's own code says it is two.

skill_service.py:2222-2224, edited by this PR:

No meta ⇒ the platform never wrote this directory (an agent-authored Playbook of the same name, or the pre-#183 single-file era).

The new refusal at :1778 — if agent_entry.get("exists") and not isinstance(agent_meta, dict) — collapses that disjunction into "the agent authored it". The platform writes marker-less directories itself, on two live paths:

1. _legacy_fallback (_restore_skill on a 404, pre-Reset-preserve-state operation (S3) #384 agent image) writes only SKILL.md — one write_file, files_written: 1, no .trinity-skill.json. It returns status: "fallback", and :1856 (if outcome["status"] in ("injected", "fallback")) appends it to injected_names, so _finalize_injected_dirs gitignores and untracks the dir. On disk that is exactly your own UNMANAGED = {"exists": True, "meta": None} fixture.
2. The skipped-meta path (_restore_skill:2604-2610): META_FILENAME goes into sent, and when it doesn't come back in restored the code appends restore_skipped:<meta> — while still returning status: "injected". A successful-looking injection can leave the directory marker-less.

Either way the next injection takes the new branch and returns conflict permanently, and there is no repair path: force=True deliberately does not override (:1769-1771, pinned by test_agent_authored_dir_survives_byte_for_byte[manual-sync]), and remove_skills:2222 returns not_managed + unmanaged_dir_kept, so unassigning orphans the dir and re-assigning conflicts again.

Two consequences worth naming. The Skills-tab copy (SkillsPanel.vue:102-108) — "a skill it authored, not this library package… Its copy was left intact and is what runs" — is factually wrong for this population: the content is the library's own truncated SKILL.md. And its recommended action, "Unassign the library skill to keep the agent's", is actively harmful. AC#6 ("pre-existing overwrites not made worse") is violated here specifically: such a dir used to upgrade on every sync; now it never upgrades again.

Reachability, honestly: this needs a pre-#384 agent image or a restore that drops the meta. Not the common path on current images — but both are live code paths, not hypotheticals, and the failure is permanent and silent when it fires.

What unblocks it is a design call, which is why I parked it rather than patching in-train: either have _legacy_fallback (and the skipped-meta path) emit a minimal marker so platform-written dirs stay identifiable, or give the refusal an explicit operator-driven take-over. Plus the test that would have caught it — the platform wrote this dir marker-less, then re-inject — which test_2914_skill_name_conflict.py does not currently have.

Nine warnings also came out of the pass. Three are worth fixing while you are in there; the rest are in the thread if you want them:

• skillDelivery.js:21-25 renders "the agent already has its own a skill with that name" — noun = names.length === 1 ? 'a skill' : 'skills'. skillAssignDelivery.spec.js:190 asserts the broken string, so the suite locks it in. The plural arm disagrees too ("skills with that name… its copy is kept").
• skillDelivery.js:90-97 — the not_delivered arm ignores report.conflicts and returns needsSync: true + "Sync now", prescribing exactly the retry Sync refuses by design. Your own test_delivery_conflict_plus_failure_is_not_delivered_but_still_names_the_conflict proves the state is reachable.
• stores/skills.js:182 — inject() now re-reads into assigned.value, SkillsPanel.vue:347 deep-watches it to resetDraft, and Sync is disabled only on injecting || !agentRunning, not on dirty. Tick boxes → Sync → ticks vanish. That is a new behavioural regression from this PR.

Second blocker, which landed after the seeds finished: regression diff is RED.

## ❌ New failures introduced by HEAD (4)
- [E]/[F] test_subprocess_pgroup.TestDrainReaderThreads::test_buffered_data_preserved_after_grandchild_kill
- [E]/[F] test_subprocess_pgroup.TestDrainReaderThreads::test_unwinds_reader_stuck_on_grandchild_pipe

Base clean on all three seeds, head failing on two — a within-run head-vs-base differential, which is what that job exists to isolate. The underlying error is the signal guard firing:

signal_guard.ForeignProcessSignal: os.killpg(13797, 9) refused: group contains a process
outside the session cgroup … A test reached the REAL process killer — usually a monkeypatch
that landed on the wrong module copy (trinity-enterprise#620).

That is a test-isolation defect, order-dependent by construction, which is why it is seed-dependent. I do not think this PR introduced it — the diff adds zero subprocess/signal/threading/killpg lines and test_subprocess_pgroup.py is not among its changed files. What it most likely does is expose it: a 351-line new test file changes collection order, the one input this class is sensitive to. It did not reproduce standalone (that file passes alone, with test_drain_bounded.py, and with test_2914 collected in front), so it needs full-suite ordering under a specific seed.

I checked whether dev covers it, and it does not. backend-unit-test is red on dev right now (09-17 onward), but on a different test — test_2814_workflow_trigger_parity::test_every_accepted_entry_names_a_real_divergence, a stale ACCEPTED_UNTIL_RELEASE allowlist. So the pre-existing rot does not account for this, and the differential stands against your head.

Either way it has to be resolved before merge — fixed, or established as pre-existing with evidence. Worth naming why this matters beyond the one PR: regression diff is not one of dev's four required contexts, so "all required checks green" would have merged this.

One process note, not a finding: pytest (head, seed 67890) was still running when the validation finished — its two siblings and all three base seeds passed. Per the train's own rule a seed running well past its siblings with no --timeout firing is usually a stalled xdist worker rather than a regression, so I would cancel and gh run rerun --failed rather than wait it out.

[dolho]

Re: merge-train ejection — fixed in d660483db

Blocker — marker-less platform dirs. Took the first option: the platform never leaves a marker-less directory behind, closed at the write rather than with an operator take-over.

• _legacy_fallback now writes .trinity-skill.json beside SKILL.md, with manifest = [SKILL.md] (what was actually written, so a later prune on a newer image diffs against the truth). files_written: 2.
• _restore_skill: when the agent's restored list lacks the marker member, it is written back with one write_file (marker_written_directly warning).
• Either marker write failing → the injection is reported failed (marker_not_written); the legacy write is rolled back (SKILL.md + dir deleted), so a retry Sync lands cleanly instead of hitting the refusal. A half-managed directory is never left.
• Tests (test_2914_skill_name_conflict.py): legacy fallback writes the marker with the truthful manifest · legacy marker failure rolls back + fails + nothing finalized · restore-without-meta writes it back · that failing → failed · the round trip that closes AC#6 — a legacy-fallback marker reads as managed on the next sync and upgrades.
• Residual population: dirs a pre-fix legacy fallback already left marker-less on a pre-Reset-preserve-state operation (S3) #384 image are indistinguishable from agent-authored ones; they resolve through the conflict's second way out (rename/remove, then Sync). Named in the requirement (§21.4) and the flow doc. The tab copy no longer asserts authorship: "…that the platform did not create — usually a skill it authored."

The three warnings

• skillDelivery.js — "its own a skill" fixed; real plural arm ("its own skills with those names (a, b) — its copies are kept and run"); spec updated (it asserted the broken string) + a plural case.
• not_delivered arm now appends the conflict clause after the Sync prescription ("Also, the agent already has its own skill with that name (c)…"), so Sync is asked for the failure, not the conflict. Spec added.
• Draft wipe on Sync: SkillsPanel now resets the draft only when the assigned set changes ([...assignedNames].sort().join('|')), not on every row refetch. Spec pins that inject() re-reads rows without changing the set.

Second blocker — regression diff. Established as pre-existing, with evidence: the same test_subprocess_pgroup.TestDrainReaderThreads::test_unwinds_reader_stuck_on_grandchild_pipe was reported as a "new failure introduced by HEAD" on the 2026-09-18 train run (run 35328963969, branch train/20260918-0918-dolho, head 36c563f) — a tree that does not contain this PR. test_subprocess_pgroup.py was last touched 2026-06-01. The test spawns a real harness and killpgs its group; the ent#620 signal guard refuses when the group check sees a process outside the session cgroup — order/runner-dependent, not this diff (zero subprocess/signal lines changed). It is the class #2891 (in progress) is about. Not fixed here; scope stays the issue's.

Rerun requested on this head.

[vybe]

merge-train — one test pushed to this branch (03f7f807)

On the 2026-09-21 train. Validated as lane B+schema. The backend is in good shape and I'm not asking you to change any of it: both migration tracks are present and agree on identical DDL, check_alembic_heads.py on the merged tree returns 66 revisions, 1 head (0065), and four mutations against the second-round marker code all go red (_restore_skill writeback, _legacy_fallback, set_skill_delivery_status, the kept_status carry). Every value this PR introduces was traced to a live production consumer — delivery_status reaches SkillsPanel and mcp-server/src/tools/skills.ts both.

What was missing was a test, so I wrote it rather than sending the PR back.

SkillsPanel.vue had no spec at all. Proven by mutation: disabling the entire conflict UI (isConflict() → return false) and reverting the draft-wipe fix at the same time left the suite byte-identically green — 139 files / 3209 tests either way. That includes the fix for the regression this PR itself introduced, which the last train caught: inject() re-reads the rows, and the old deep watch wiped unsaved ticks on every Sync. The test added for it (skillAssignDelivery.spec.js:127) asserts a store property that was already true before the fix, so reverting SkillsPanel.vue's watcher left it green.

tests/unit/skillsPanelDraftSurvivesSync.spec.js mounts the panel (the ent#625 harness) and drives the real path — a real checkbox tick, a real store.inject() over a mocked axios returning a new rows array with the same set. Negative-controlled in both directions, deliberately, so neither test can be satisfied by the other's cheat:

revert to `watch(() => store.assigned, resetDraft, { deep: true })`
    -> "keeps an unsaved tick when Sync re-reads the SAME set"  FAILS
delete the watcher outright  ("never reset" would pass test 1)
    -> "still follows a real change to the assignment set"      FAILS

Full frontend suite on the branch afterwards: 142 files, 3246 passed.

Nothing else was changed. Left as yours to judge, none of it blocking: the refusal is conditional on a successful probe (skill_service.py:1778 — a transient exec fault makes exists falsy and re-opens the overwrite; deliberate and tested, but AC#1 holds only when the probe succeeds); the fleet sweep and start path still report success for a conflict, so the fleet report is silently incomplete; data-testid="skill-conflict" / -unassign have no consumers; and marker_written_directly renders to the operator untranslated.

Worth noting for context: your #2922 is on the same train, and its ratchet cannot catch this class — it fails a spec that reads source text, not a component that has no spec. That gap is the one thing #2918's title names and the ratchet does not close.

[vybe]

merge-train: batch validated on train/20260921-1644 (#2939) — full suite green, including pg-migrations and schema-parity. Both migration tracks present and agreeing; check_alembic_heads.py on the merged tree returns 66 revisions, 1 head (0065). Backend mutation-verified (4 mutations, all red). A mounted spec was pushed to this branch pinning the draft-wipe regression this PR introduced and fixed — negative-controlled in both directions; see the comment above.