fix(skills): match the legacy library URL by repo, not by raw string (#2763)

[dolho]

Fixes #2763

The defect

_adopt_legacy_clone decided whether a legacy skills_library_url named an already-configured source with raw string equality, while the rest of the platform stores that URL normalized. The same repository written two ways never matched, so the install took the ent#346 "already has sources" refusal branch on every sync — forever — and filed a fresh un-deduped high-priority alert each time. That flood is #2744; this is its cause.

validate_skills_library_url is a validator and a normalizer, and the write route uses it as one:

# routers/skills.py:649  — ASSIGNS the return
url = validate_skills_library_url(body.url)

# services/skill_service.py:734 (before) — DISCARDED it
validate_skills_library_url(url)
...
existing = [s for s in db.list_skill_sources() if s.url == url]

Both representations occur on real installs by construction:

where the source came from	stored as
bundled default — config.TRINITY_DEFAULT_SKILL_SOURCE (config.py:522), a bare literal that never passes the validator	github.com/abilityai/trinity-skills
POST /api/skills/sources	https://github.com/abilityai/trinity-skills

So either direction of the mismatch is reachable, and both are covered.

The change

Two lines of behaviour, plus a helper:

• assign the validator's return (normalized_url);
• compare through _same_skills_repo, which normalizes the stored side too — normalizing only the setting would still miss every install whose source came from the seed;
• reject_embedded_credentials keeps seeing the original string. It must judge what was actually written, not a form we produced.

Why this cannot weaken ent#346

A match returns an existing source id and creates no row — it is the no-op branch. The grant branch (count_skill_sources() == 0 → create_skill_source) is untouched, and a key naming a genuinely different repo still reaches the refusal exactly as before.

Normalizing removes false positives from the detector; it does not widen what may be granted. That is asserted by two of the eight tests, not just claimed here (test_a_genuinely_different_repo_still_reaches_the_ent346_refusal, test_the_grant_branch_is_untouched_on_a_pre_migration_install).

Verification

Every test drives the real _adopt_legacy_clone; the only stubs are db and library_root.

Reverting only the call site — keeping the helper defined, so the module still imports and the behavioural tests actually run — fails exactly the two regression cases and leaves the other six green:

FAILED test_the_same_repo_in_either_form_matches_and_files_no_alert[seeded-default-vs-https-key]
FAILED test_the_same_repo_in_either_form_matches_and_files_no_alert[api-created-source-vs-bare-key]
2 failed, 6 passed

With the fix: 8 passed. The fix flips what it targets and nothing else.

496 passed, 2 skipped across every skills-related suite (-k "skill or 346 or 237").

Scope

Deliberately the scheme/no-scheme split the platform itself creates. These stay distinct and are pinned by a test, so collapsing them later is an argued ent#346 decision rather than a quiet widening:

https://github.com/abilityai/trinity-skills.git
https://github.com/abilityai/trinity-skills/
abilityai/trinity-skills

The grant branch still registers the raw url, unchanged — normalizing what gets created is a behaviour change to the branch this PR claims not to touch.

Relationship to #2744

Complementary, not overlapping — #2744 is assigned and in progress, and this does not take any of its three suggestions:

1. stable dedupe key — still wanted; bounds any future genuine refusal.
2. severity downgrade — strengthened by this: the common cause turns out to be a representation mismatch, not a security event.
3. clear the key — unaffected here. That decision is still waiting on reading the refused URL on the affected install.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ

[dolho]

Validated against a live instance, not just unit tests

Ran the whole thing end-to-end on a local dev instance (PostgreSQL, v0.9.5-rc2) whose backend was running the unfixed dev code. The instance was restored to its exact prior state afterwards.

Precondition — the real-world shape, not a contrived one

skills_library_url  = https://github.com/abilityai/trinity-skills   (the setting)
skill_sources.url   = github.com/abilityai/trinity-skills           (the configured source)

Same repository. Two representations. Set exactly as it occurs in the wild: the source row is the bundled default seeded from config.TRINITY_DEFAULT_SKILL_SOURCE (no scheme), the setting holds the form a human or the UI would write.

Before the fix — reproduced exactly as reported

One sync produced one permanent high alert naming the repo it had just synced successfully:

POST /api/skills/library/sync
  -> {"success": true, "synced": 1, "skill_count": 41, "failed": 0}
  -> queue: 1 x  high | Legacy skills-library adoption refused
            url = https://github.com/abilityai/trinity-skills

And it grows without bound — one alert per sync, each with a distinct un-deduped id and expires_at: None:

after sync #1 -> 1 pending alert
after sync #2 -> 2 pending alerts
after sync #3 -> 3 pending alerts
after sync #4 -> 4 pending alerts

skills-legacy-adoption-2026-09-14T09:00:03.303473Z | high | expires_at=None
skills-legacy-adoption-2026-09-14T09:00:02.055444Z | high | expires_at=None
skills-legacy-adoption-2026-09-14T09:00:00.879717Z | high | expires_at=None
skills-legacy-adoption-2026-09-14T08:59:48.958685Z | high | expires_at=None

In the UI this read as "5 pending responses" with the Operations badge at 21, four of the five being this alert — and the one item that genuinely needed a person ("A Workspace client rated a response as not useful") pushed to the bottom of the list. That is #2744's "it degrades the gate it lives in", visible.

After the fix — same precondition, unchanged

The setting and the source row were left exactly as they were; only the code changed (backend restarted on the patched skill_service.py):

key    = https://github.com/abilityai/trinity-skills   <- unchanged
source = github.com/abilityai/trinity-skills           <- unchanged

after sync #1 -> 0 pending alerts
after sync #2 -> 0 pending alerts
after sync #3 -> 0 pending alerts
after sync #4 -> 0 pending alerts
after 5 more  -> 0 pending alerts

Nine syncs, zero alerts. Badge 21 -> 17, "5 pending responses" -> "1 pending response" — and the Workspace rating item is now at the top, unburied.

Negative control — the detector still detects

The part that matters for ent#346. With the fix live, the setting was repointed at a genuinely different repository:

key = https://github.com/attacker/evil-skills
POST /api/skills/library/sync
  -> queue: 1 x  high | Legacy skills-library adoption refused
            url = https://github.com/attacker/evil-skills

The alert fires, names the offending repo, and reaches context.url exactly as before. So the fix removes false positives without weakening the guard — demonstrated on a running instance, not only asserted in a unit test.

Restored

Legacy key deleted (absent, as originally found), test alerts cleared, the patched file reverted byte-for-byte from backup, backend restarted on the original code, 0 pending _skills-sync alerts. /health green.

Screenshots of all three states (before / after / negative control) were captured and are with the reviewer.

[dolho]

[dolho]

Review: /review + /validate-pr

Self-review, so the goal was to find what I got wrong rather than confirm it. One informational finding I did not know when I opened this, no critical. Recommendation: APPROVE, with [I1] worth a sentence in the code.

/validate-pr — Lane B

Category	Status	Notes
Lane	B	services/skill_service.py + a test; no auth/credential/parser/image/CI path
Base branch	✅	dev
Closing keyword	✅	Fixes #2763 — same-repo, so the promotion to status-in-dev fires automatically
Size	✅	2 files, +190/−2
Security greps	✅	clean
Docs	➖	bug fix → descriptive commit only, per tiered docs
Test adequacy	✅	8 tests; reverting only the call site fails exactly the 2 regressions and leaves 6 green
Merge gate	🟡	MERGEABLE / BLOCKED — needs a review, no failing checks

/review — structural

Critical: none.

Verified rather than assumed:

• The fix cannot widen what matches. Two different repos cannot normalize to the same string — validate_skills_library_url only adds a scheme; it never rewrites owner or repo.
• The grant branch is untouched. count_skill_sources() == 0 → create_skill_source is byte-identical, and a test asserts it registers the raw url exactly as before.
• reject_embedded_credentials still sees the ORIGINAL string, not the normalized form — it must judge what was actually written. Pinned by test_a_credential_bearing_key_is_still_refused_without_echoing_it.
• Fail-safe direction. An unparseable stored url returns False → falls through to the refusal, rather than being treated as equal and silently adopted.

[I1] The comparison now makes a DNS call, and the code does not say so (Confidence: 9/10)

_same_skills_repo calls validate_skills_library_url on the stored side. That function is not pure — it performs SSRF defence-in-depth:

# utils/url_validation.py
# Defense-in-depth: resolve hostname and reject private/internal IPs
resolved_ips = socket.getaddrinfo(hostname, None)

So a comparison that used to be == can now issue a blocking DNS resolution, on a path (_adopt_legacy_clone) that runs on every sync_library(). I did not know this when I wrote the helper, and nothing in the diff warns the next reader.

Three things keep the impact small, and they are worth stating because they are why this is informational rather than a defect:

1. The exact-match short-circuit runs first (if stored_url == normalized_url: return True), so a correctly-stored install never reaches the validator. Only the broken case — the one this PR fixes — takes the slow path, and only until the key is cleared.
2. A DNS failure is tolerated upstream — except socket.gaierror: pass — so an outage does not turn a legitimate match into a refusal.
3. The source list is 1–2 rows on a real install, and the caller is already about to do git I/O.

But there is one narrow regression path worth knowing about: if a stored URL's hostname resolves to a private/internal address, the validator raises ValueError, _same_skills_repo returns False, and the install starts emitting the very alert this PR exists to stop. That needs github.com to resolve privately — a split-horizon DNS or an internal mirror — so it is unlikely, not impossible.

Cheapest fix is a comment naming the DNS call and the short-circuit that avoids it. A pure string normalizer would remove the network call entirely, but it would be a second implementation of a policy that deliberately lives in one place, which is the trade #2763 was fixing in the first place — so I would not.

Note on scope

.git suffixes, trailing slashes and the bare owner/repo shorthand stay distinct after normalization, pinned by test_the_documented_tail_is_still_distinct. That is deliberate — collapsing them changes which strings count as "already configured", which is an ent#346 decision rather than a tidy-up.

_{🤖 Generated with Claude Code · https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ}

[vybe]

merge-train note — pushed one mechanical commit (871c54ba3) to this branch: the _record_adoption_failure stub in tests/unit/test_2763_skills_url_normalized_match.py:51 now accepts **kw. #2777 (riding the same train) passes steady_state=True to that call; on the merged tree the old stub raised TypeError, which the outer except Exception swallowed and re-recorded, and test_a_genuinely_different_repo_still_reaches_the_ent346_refusal failed on 'already has'. Both PRs are individually green — this only shows up once they share dev. Verified: 8/8 pass on the branch, 15/15 on the 2764+2777 merged tree.

[vybe]

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