fix(skills): the legacy-adoption refusal files one row per refused URL, not one per sync (#2744)

[trinity-ability]

Fixes #2744

Problem

SkillService._adopt_legacy_clone() is the first statement of every sync_library(). On an install that is past migration but still carries a skills_library_url system setting matching no configured source, it takes the terminal count_skill_sources() > 0 branch and calls _record_adoption_failure(), which minted a timestamped request_id at priority: "high" with expires_at: None.

A timestamped id defeats create_item's (agent_name, request_id) ON CONFLICT DO NOTHING by construction, and expires_at: None means the 5-second mark_operator_queue_expired sweep never reaps the row. So: one permanent, high-priority, operator-unclearable alert per sync, forever — 17 of them (~17% of everything pending) on the reporting install, and 288/day at the ent#236 auto-sync interval floor.

The guard itself is correct. ent#346's refusal, the validate_skills_library_url + reject_embedded_credentials pair, and the grant branch are all untouched here. Only the alert cadence and severity of one already-correct refusal change.

The change

Two behaviour changes, on one branch only — the terminal "this install already has skills sources, so it is past migration" branch, which is the designed resting state of every migrated install whose legacy key lingers, not a failure.

Fix 1 — a stable, URL-keyed request_id.

f"skills-legacy-adoption-refused-{hashlib.sha256(url.strip().encode()).hexdigest()[:12]}"

create_item's ON CONFLICT on (agent_name, request_id) then collapses N syncs to exactly one row per refused URL. A different refused URL still mints its own id and raises its own item (AC 3). .strip() because the setting is operator-authored free text and a trailing newline would otherwise mint a second permanent row for the same URL. The hash is taken over the raw URL, never a normalised one: normalising means re-calling validate_skills_library_url, which performs a live socket.getaddrinfo and can raise — a network call and a raise path inside a decorative, fail-soft alarm whose whole contract is that it never blocks a sync. The id computation stays inside the try, so a non-str setting value degrades to a logger.warning and no alarm rather than turning the alarm into a raiser.

This is also strictly better under concurrency: _adopt_legacy_clone() runs outside the SingleFlightLock acquired later in sync_library(), so two uvicorn workers and the scheduler can enter it at once. Today that mints N different timestamped ids ⇒ N rows; with the stable id, concurrent inserts collide and one row survives however many raced.

Fix 2 — priority: "low" + logger.info on that branch only.

Shaped as a keyword-only steady_state: bool = False on the existing _record_adoption_failure rather than a second emitter method — one #1677 _ALLOWED_CALLERS key (the qualname is unchanged), and it mirrors the in-file precedent _announce_reconcile_refusal, which already uses a stable content-derived id on the same _skills-sync host.

AC 4, provable by diff. The default False reproduces today's behaviour exactly, so the two actionable call sites (validation reject, adoption exception) are literally unchanged lines. Grepping _record_adoption_failure across the src/backend/ diff returns only the def pair:

-    def _record_adoption_failure(self, url: str, message: str) -> None:
+    def _record_adoption_failure(

An unchanged call site cannot regress. Both keep priority: "high" and their timestamped, repeat-visible ids.

skills-legacy-adoption- joins _RESERVED_ID_PREFIXES. The family prefix, deliberately — it covers the new -refused- stable id, the two actionable branches' timestamped ids, and the 17 historical rows. The reasoning, in order:

• A stable id derived from an admin-visible URL is guessable, which is the bug: operator-queue create path has no rate limit or size caps — flooding/operator-fatigue surface (blocking pull default-ON) #1632 C2 self-suppression class: pre-create the id, and the platform's alarm is swallowed by the same ON CONFLICT this PR relies on.
• The host _skills-sync is uncreatable at agent CREATE (sanitize_agent_name strips a leading _) but not on the rename path: routers/agent_rename.py uses a bespoke sanitizer, commented "same as agent creation" when it is not — _ is in its keep class and .strip('-') strips hyphens only, so _skills-sync maps to itself. An owner (no admin needed) can therefore land a real agent on that name and have _poll_cycle ingest its file under the matching agent_name.
• It is the house pattern for a stable-id platform alarm on a pseudo-host: sub-headroom-/_sub-headroom, db-backup-/_db-backup and log-archive-/_log-archive are all reserved despite their hosts being equally uncreatable. The two unreserved skills siblings are the outliers, not the rule.
• The same tuple drives is_platform_minted, which has three sinks, not two: the ent#499 responded write-back, and the ent#329 respond→resume gate in routers/operator_queue.py. Unreserved, this item answers "not platform-minted", which is simply wrong for an item the platform mints — and this PR makes "an operator clicks through on it" the expected action.

Verified safe: the prefix check runs only in _sync_agent ingestion, never in db.create_item, so the platform's own create is unaffected.

Credential scrub on the URL echo (strip_url_credentials, already imported in this file and already used for this purpose). This is a deliberate, disclosed scope addition — it is 2 lines inside the hunk this PR already rewrites, and the alternative was to carry forward a docstring claim known to be false. The emitter's docstring asserted that a credential-bearing URL is refused by a message that does not echo it. That is true of message and of nothing else: EmbeddedCredentialError is a ValueError subclass, so the validation-reject branch is exactly the branch a PAT-bearing URL reaches, and it passed the raw url to both logger.error(...) and context["url"]. A skills_library_url of the form https://ghp_…@github.com/o/r therefore landed the PAT (i) in the Vector-captured backend log at ERROR and (ii) durably in operator_queue.context JSON in SQLite → every backup → rendered in the Operating Room. CWE-312, Rule #5, Invariant #12, the same class as ent#334/ent#347. The hash still runs over the raw value — scrubbing first would collide two different tokens on one repo.

Decisions recorded

• Fix (3) — clearing the lingering skills_library_url key — is deferred, as the issue author asked. The issue marks it a deliberate behaviour change with a security-review caveat: on the reporting install the key's value provably differs from the one existing source's URL and was written after migration completed, and why is under separate private investigation — that answer decides whether clearing is hygiene or destroying the only record of a refused write. Fixes (1) and (2) are independent of it. The diff contains no read, write or clear of that key; skills_library_url appears in the source diff only inside added docstring prose.
• context["reason"] = "already_migrated" was dropped by the owner at review. An earlier commit added it as a machine-readable discriminator between the two classes now sharing one alert_type. QueueItemDetail.vue renders every context key, so it would have surfaced to operators as a reason │ already_migrated row; put to the owner as keep / drop / rename, the answer was drop. Nothing reads the key, so removal is non-breaking. The steady-state discriminator is priority + the logger.info level alone.
• The #1677 justification string was false and is corrected. _ALLOWED_CALLERS read "platform-only: admin-driven sync cadence (ent#346)"; ent#236's auto-sync loop calls the same sync_library() unattended on a 300s–86400s timer, so nothing admin-driven bounds it. The emitter stays platform-only — correctly — but on the real ground: its only input is skills_library_url, which routers/settings.py blocks on the generic settings PUT (LEGACY_SKILLS_LIBRARY_KEYS, 422) and no other writer reaches, so no agent can drive its volume. Corrected in tests/unit/test_1677_operator_alert_emitters.py::_ALLOWED_CALLERS, co-located as a comment at the emitter (the guard compares (path, qualname) set membership and never asserts the justification text, which is precisely how this string drifted into falsehood), and in feature-flows/operating-room.md.

Behaviour to know

• Invariant Fix git pushing bug #9 is N/A — there is no schema change. request_id and its UNIQUE(agent_name, request_id) index shipped in bug: operator_queue.id is a global primary key — cross-agent id collision silently swallows requests #1631/fix(operator-queue): scope request-id uniqueness per agent (#1631) #1684. No db/migrations.py entry and no Alembic revision; nothing to go looking for.
• Dismissal now sticks, so operators should CANCEL, not acknowledge. create_item's conflict target is (agent_name, request_id) and ignores status, so once the single stable-id row exists a later sync's insert is a no-op whatever that row's status is. That satisfies AC 1 ("at most one pending item") more than strictly, and the issue names "unclearable by the operator" as part of the defect. But the UI's Acknowledge button calls respondToItem(id, 'acknowledged', '') → db.respond_to_item, which writes status responded — and responded is deliberately excluded from Clear All (db/operator_queue.py:492 admits only acknowledged/cancelled/expired) because "their response still has to be delivered to the agent". Nothing will ever deliver this one: mark_acknowledged fires only from a real agent's file sync, and _skills-sync has no container. The item would leave the open pane and sit un-hideable in the resolved list. POST /api/operator-queue/{id}/cancel (or bulk-cancel) yields cancelled, which is terminal, Clear-All-able, and uses the normal retention window rather than max(OPERATOR_QUEUE_RESPONDED_MIN_RETENTION_DAYS=30, operator_queue_retention_days).
• Retention-0 caveat. _sweep_operator_queue_retention returns early when days <= 0, and operator_queue_retention_days = 0 is a documented "disabled" value (_guard_allows can also refuse the prune). On such an install, because the conflict target ignores status, dismissing this alert mutes the refusal permanently — including a later genuine refusal at the same URL. The residual signal is the per-sync logger.info, which is not suppressible and is Vector-captured. Both halves stated rather than a bound asserted that does not always exist.
• The surviving row is frozen. create_item never updates on conflict, so created_at, question and context.url stay pinned to the first refusal; there is no "last seen" anywhere in the queue. With fix (3) deferred, this replaces a recurring timestamped operator-visible signal with one frozen row plus a log line. That is the tradeoff, named.
• Pre-existing rows are not retro-cleaned. The 17 historical high items keep their timestamped ids and stay pending until dealt with — a data-cleanup migration over operator_queue is out of scope and is the destructive direction (bug: retention floor (#1065) silently deletes pre-existing execution history on upgrade — currently dev-only, ships to everyone at next release #1638's lesson). The zero-code remedy is POST /api/operator-queue/bulk-cancel → cancelled → Clear All hides them → the Add operator_queue retention sweep to cleanup_service #1142 sweep deletes them. After this fix, at most one new item is filed per refused URL.
• low does not weaken the ent#346 security posture. The control is the refusal — the row is not created, the repo is not adopted, no skills are injected. Unchanged. The alert is observability about a refusal that already succeeded; it still exists, still names the refused URL, and still logs under the greppable [ent#346] prefix (at INFO, which the root logger captures). The id is URL-keyed, so a changed skills_library_url mints a new item — a new URL still alerts.

Coordination with #2763 / PR #2764

Complementary, and both wanted — dolho's comment on #2744 says so: "#2764 deliberately takes none of the three — it only stops the branch being entered spuriously. All yours." #2744 is the blast-radius fix (any future legitimate refusal is bounded); #2763 removes the false positive firing today (a raw-vs-normalised URL compare that enters this branch for a repo the install is already syncing).

PR #2764 is still open and not on dev, so nothing is reconciled here — this note stands in for it.

In skill_service.py the hunks are adjacent but non-overlapping: theirs are a new module-level _same_skills_repo(), the validate_skills_library_url return assignment, and the existing comparison; ours are import hashlib, the steady_state=True keyword at the terminal call site, and the emitter body ~1500 lines further down. A rebase either way is a pure line-offset rebase.

The one genuine collision is in #2764's test file, not in the source. Its new test stubs our emitter with a positional-only lambda:

patch.object(ss.SkillService, "_record_adoption_failure",
             lambda self, url, msg: alerts.append((url, msg)))

After this change the terminal branch calls self._record_adoption_failure(url, msg, steady_state=True) ⇒ TypeError: <lambda>() got an unexpected keyword argument 'steady_state'. That TypeError is raised inside _adopt_legacy_clone's try:, so the outer except Exception swallows it and re-calls the emitter with two positional args — which the lambda does accept. The failure is therefore not clean: len(alerts) == 1 still holds, but the message becomes "legacy skills-library adoption failed: <lambda>() got an unexpected keyword argument…", so assert "already has" in alerts[0][1] fails pointing at the wrong cause. Whoever lands second widens the stub to lambda self, url, msg, **kw.

Operational note. Once #2764 lands, the reporting install may stop entering this branch at all. Do not wait for the single low item to appear as confirmation — absence is the expected outcome there.

Tests

tests/unit/test_2744_skills_adoption_alert_idempotency.py — a new file rather than an append to test_ent346_skills_source_injection.py, so the two parallel branches do not collide in one test module. Every test drives the real _adopt_legacy_clone; the only stubs are db and library_root. Two harness rules are load-bearing: utc_now_iso is imported inside the emitter body, so the patch target is utils.helpers.utc_now_iso (patching services.skill_service.utc_now_iso yields a vacuous test), and every terminal-branch test also asserts "already has" in item["question"] — validate_skills_library_url does a live socket.getaddrinfo, so on a sandboxed resolver a test intending the terminal branch would silently drive the validation-reject branch and fail reading exactly like the fix regressing.

Red-on-base (service change stashed), reproduced independently by a second reviewer — 5 failed / 2 passed of the 7 node ids:

Node id	On base	Pins
test_n_syncs_in_the_terminal_state_file_exactly_one_item	❌ fail	AC 1, AC 5 — 5 syncs, one id (incrementing clock stub guarantees 5 distinct ids on base)
test_a_different_refused_url_gets_its_own_item	❌ fail	AC 3 — the exact sha256(url.strip())[:12] id per URL, frozen clock
test_the_terminal_refusal_is_not_high_priority	❌ fail	AC 2 — low, and logger.info not logger.error
test_the_stable_id_is_reserved_and_id_shaped	❌ fail	prefix ∈ _RESERVED_ID_PREFIXES, 12 lowercase hex, ^[A-Za-z0-9._:-]+$, ≤512
test_a_pat_bearing_url_is_never_echoed_into_the_log_or_the_queue	❌ fail	the credential scrub — sweeps the whole item, not just context["url"]
test_the_actionable_branches_keep_high_and_repeat_visible_ids[validation reject…]	✅ pass	AC 4 — green on base by design
test_the_actionable_branches_keep_high_and_repeat_visible_ids[adoption exception…]	✅ pass	AC 4 — green on base by design

After the fix: 7 passed (6 test functions; the AC-4 test is parametrized over both actionable branches). The owner's context["reason"] drop removed one assertion, not a test.

Deliberately not asserted here: that the DB actually dedupes. That is create_item's contract, shipped and covered by #1631/#1684. These tests assert the property the service controls — the id is stable — and cite the mechanism.

# neighbourhood (post-rebase)
cd tests && python3 -m pytest unit/test_2744_skills_adoption_alert_idempotency.py \
  unit/test_ent346_skills_source_injection.py unit/test_1677_operator_alert_emitters.py \
  unit/test_1677_operator_alert_budget.py unit/test_ent237_skill_sources.py
→ 164 passed

# reserved-prefix consumers (post-rebase)
cd tests && python3 -m pytest unit/test_1816_system_agent_adoption.py unit/test_2205_archive_preflight.py \
  unit/test_2216_backup_observability.py unit/test_2529_gitignore_precedence.py \
  unit/test_ent434_headroom_alerts.py unit/test_ent499_problem_report.py unit/test_ent500_role_drift_prefix.py
→ 238 passed

/verify-local --skip-agent on the pre-rebase tip (project trinity-verify-83674660): unit 15,554 passed / 31 skipped / 0 failed (710 s), integration 70 passed / 13 skipped / 2 deselected / 0 failed, build + import-smoke and boot + health both pass. /review and /cso --diff both CLEAN on the diff, with the security pass recording the change as net-positive because it closes the pre-existing CWE-312 exposure above.

Docs touched

File	Delta
docs/memory/requirements/skills.md §21.1.3	the terminal refusal is bounded by its own class — low, stable URL-derived id, reserved prefix, dismissal sticks, scrubbed echo; the two failure branches stay high by product decision, not because they are bounded
docs/memory/requirements/security.md §26.7	skills-legacy-adoption- added to the reserved-prefix enumeration
docs/memory/feature-flows/operating-room.md	×3 sites — both reserved-prefix enumerations, plus the "Platform exemption & emitter budget (#1677)" paragraph, which bundled this alarm under a disjunction including "operator-driven"; it now carries its own per-branch bound and the correction of the false justification — and a Revision History row
docs/memory/feature-flows.md	one dated change-log row
tests/registry.json	the new test file's entry

No architecture delta: neither architecture.md nor architecture/agent-lifecycle.md documents the legacy-adoption path. docs/memory/feature-flows/skills-library-sync.md is already stale w.r.t. ent#237/ent#346; rewriting it is a separate docs issue, not this one.

Follow-ups (named, not filed)

1. The rename path bypasses the name sanitizer. routers/agent_rename.py's bespoke sanitizer keeps a leading _ and strips hyphens only, so all five _-prefixed alarm hosts — _skills-sync, _retention-guard, _db-backup, _log-archive, _sub-headroom — are landable by an owner. Route rename through the same sanitize_agent_name used at creation.
2. _skills-sync is missing from canary/snapshot.py::_PLATFORM_ALARM_SENTINELS, so every pending _skills-sync alarm reports as an L-03 ghost-agent orphan. This fix improves the volume (17 → 1) but converts a decaying stream into one permanent finding.
3. The two sibling emitters are the same unbounded class. _announce_fleet_failures (skills-fleet-reinject-{ts}, medium, expires_at: None, one permanent row per partially-failed sweep, and its "one per sync run" justification is equally false) and _announce_reconcile_refusal (re-floods when the orphan count flaps). Neither is reserved.
4. Branches A and C are unbounded too — a permanently invalid skills_library_url re-enters the validation branch every sync, forever, at high. AC 4 forbids changing that here.
5. Make the #1677 justifications machine-checkable — structured (edge_triggered | idempotent_id | cooldown | leader_locked | operator_driven) plus an AST check that an emitter claiming idempotent_id has no timestamp interpolation in its id expression. Two entries currently assert bounds that do not hold; this turns "found after 17 rows in production" into "caught at commit".
6. expires_at in addition to the stable id — self-clears the item from the open pane and dissolves the "responded row nothing will ever acknowledge" wart above. expires_at instead of the stable id would be strictly worse (a row per sync, each flipped to expired).
7. Fix (3) — give the refusal an ending by clearing the lingering key, pending the private investigation into why it holds a post-migration non-matching value.
8. The three reserved-prefix doc enumerations are each pre-existingly incomplete — every one of them already omits prefixes the live tuple carries. Ours is added; their drift is untouched (Rule Feature/gemini runtime support #2).
9. strip_url_credentials is userinfo-only. It scrubs user:pass@host; a token in the path or query (…/repo?token=…) still reaches the log and context. The reported shape is covered; the general one is not.
10. question is credential-free only by an undeclared coupling — it happens to be built from the exception text on branch A, and EmbeddedCredentialError happens not to echo the URL. Nothing pins that; a future message that interpolates the URL would reopen the leak on a field this PR does not scrub.
11. One owed .claude/agents/test-runner.md catalog row for the new test file. Edited but uncommitted — the .claude submodule is detached in this worktree.

🤖 Generated with Claude Code

https://claude.ai/code/session_0176XEqK8PTCAK5K6yZURQLv

[vybe]

merge-train: batch validated on train/20260914-1332