🛟 fix: Isolate Invalid Skills During GitHub Sync (#14735)

* fix: treat unrecognized SKILL.md frontmatter keys as warnings

An unknown key in one SKILL.md failed that skill outright, and because the
GitHub sync runner marks a source failed on any validation error, a single
stray key took down every other skill in the repository. Syncing
github.com/cloudflare/skills failed entirely because 2 of its 13 skills
carry a `references:` key.

UNKNOWN_KEY is now a warning, so the skill is stored (unknown keys and all)
and the issue is surfaced rather than fatal. `references` joins the allowed
set with a shallow JSON-safety check instead of a strict kind match: real
files use a string, a list of strings, a list of objects, and a map, and
pinning one shape would reintroduce the same failure.

Malformed frontmatter stays fatal: INVALID_TYPE, INVALID_SHAPE and the
non-plain-object check are unchanged.

* fix: skip individual skills instead of failing a whole sync source

Any error inside the discovery or commit loop reached the outer catch and
marked the entire source failed, so one unusable SKILL.md, one oversized
blob, or one duplicate name cost every other skill in the repository.

Each skill now runs inside its own boundary and a failure is recorded
against that skill. Errors that mean nothing else in the run can succeed
(lock loss, GitHub auth failures, rate limiting) still abort the source
rather than being charged to whichever skill hit them first. Skills are
marked seen before the attempt, so the reconcile pass cannot mirror-delete
the previously synced copy of a skill a later run can repair, and duplicate
names now drop the whole colliding group instead of letting tree order pick
an arbitrary winner.

Status gains `partial` (published some, skipped others) plus a capped
sample of the skipped skills with the reason for each. A run that publishes
nothing and skips something is still `failed`, carrying the first skip's
error. The skipped entries name repository paths, so they follow the same
visibility rule as owner/repo/paths; the bare count does not.

Sync warnings are logged too: a background run has no user-facing surface,
so the log is the only place a maintainer sees why an upstream SKILL.md
looks off.

* test: cover skill sync warnings reaching the log

An unrecognized frontmatter key no longer fails the skill, so a background
sync has nowhere to report it except the log. Every mock in this spec
returned an empty warning list, which left that path unexercised.

* fix: describe nested frontmatter values in the shared skill type

`SkillFrontmatterValue` allowed only scalars and string arrays, while the
server has always stored `hooks` and `metadata` as JSON-safe objects, and now
`references` too. A skill carrying any of them could not be represented by
`TSkill`, `TCreateSkill` or `TUpdateSkillPayload` without a cast.

The type stays free of `any` and `unknown`: values remain JSON-safe by
construction, and the server keeps bounding depth, string length and array
size when it validates them.

* fix: protect moved mirrors and rolled-back counts when a skill is skipped

Continuing past a failed skill exposed two problems that aborting the whole
source used to hide.

A moved skill's mirror keeps its old upstream id until the update lands, and
only the new path was marked as seen, so the reconcile pass read the mirror as
stale and deleted the very copy the skip path exists to preserve. The old id is
now marked as seen too.

Deletion counters were incremented when a stale name-conflicting mirror was
removed, but never undone when the following commit failed and the mirror was
restored. The run no longer stops there, so the status persisted a deletion
that did not happen and the reconcile pass counted the restored row again.
Counters are now rolled back when the restore succeeds.

* fix: bound unknown frontmatter values and keep moved mirrors through duplicates

Tolerating an unrecognized key meant its value skipped the shared JSON-safety
check, so a deeply nested or oversized payload was accepted and persisted under
a key nobody validates. The key stays non-blocking; the value is now held to
the same depth, array and string bounds as every structured key.

A skill that moves into a name another discovered skill also claims is dropped
with the rest of its duplicate group before the sync path can reuse its mirror,
which left the still-published copy unmarked and reconciled away. Both paths now
mark the moved mirror through one helper.

* fix: end the source when a skipped skill fails to roll back

A skill that fails and rolls back cleanly is just a skipped skill. One whose
restore or delete also fails leaves a mirror with half-rewritten files or a
half-created row, and the run now continues past it, so the source could report
partial success while that mirror stayed inconsistent and its pre-marked
upstream id kept reconciliation away from it.

Failed rollbacks now raise a source-fatal error carrying the original failure,
which stops the source the way a lost lock or a refused GitHub token does.

* test: cover a skipped skill discovered at the repository root

A repository-level SKILL.md is discovered with an empty path, so this pins
that a skip recorded against it still persists with the rest of the partial
status rather than taking the whole status row down with it.

* docs: describe unknown skill frontmatter warnings

* fix: preserve mirrors after partial skill sync

* fix: preserve skill validation details during sync

* fix: fail sync when mirror identity cannot be restored

* fix: harden skill sync failure boundaries

* fix: preserve skipped skills on fatal sync

* fix: surface skill sync diagnostics and rollback failures

* fix: preserve skill frontmatter extension keys

* fix: reject skill frontmatter keys that collide when normalized

Frontmatter keys are matched case-insensitively against the canonical
key list, so "Name" and "name" both resolve to "name". Every call site
normalized independently, and the last key in iteration order silently
won, meaning the effective value depended on YAML ordering rather than
on anything the author could see.

Centralize the normalization in normalizeSkillFrontmatterKeys and have
it fail when two recognized keys resolve to the same canonical key,
rather than picking one. parse.ts, deployment.ts and the agent handler
now surface that as a parse error; createSkill and updateSkill surface
it as a blocking DUPLICATE_KEY validation issue. Unrecognized keys are
still passed through untouched so extension frontmatter survives.

deriveStructuredFrontmatterFields and both write paths now run on the
normalized map, so a "Disable-Model-Invocation" key derives the same
column a lowercase one does.

* fix: harden github skill sync against dropped requests and failed cleanup

Three failure paths in the GitHub sync could leave a source looking
healthier than it was.

githubJson only handled HTTP-level errors. A fetch that rejected before
producing a response (DNS failure, socket reset, abort) escaped as a
raw TypeError, so the sync reported a generic crash instead of a typed
sync error. Wrap it as GITHUB_REQUEST_FAILED and add that code to the
fatal set, since a source whose requests never complete cannot be
partially synced.

When a synced file failed to persist, the orphaned upload was cleaned
up on a best-effort basis and the cleanup error was only logged. If the
cleanup itself failed, the source still ended with the original error
and left a real orphan behind. Promote that to a rollback failure so
the source reports SYNC_ROLLBACK_FAILED with the triggering error.

Skill warnings were logged inside commitRemoteSkill, before the file
sync and viewer setup that can still roll the skill back. A skill that
never survived publication therefore emitted warnings as though it had.
Return the warnings from the commit and log them once the skill is
fully published.

* fix: report skipped github skills before credential errors

serializeErrorMessage checked isCredentialError first, and that check
matches on the error text. A skipped skill whose path happens to
contain a credential-ish word, for example skills/credential-helper,
was therefore redacted to "GitHub skill sync credentials are not
available" for admins without credential-metadata access, hiding a
parse failure behind a wrong diagnosis.

Check the promoted skipped-skill case first, since it is identified by
error code rather than by text and is the more specific match. The
credential redaction still applies to everything else.

api/server/routes/skills.test.js

@@ -300,14 +300,27 @@ describe('Skill routes', () => {
       expect(res.status).toBe(400);
     });
 
-    it('rejects frontmatter with unknown keys', async () => {
+    it('accepts frontmatter with unknown keys and warns about them', async () => {
+      const res = await createSkillAsOwner({
+        name: 'unknown-key-frontmatter-skill',
+        frontmatter: { 'not-a-real-key': 'value' },
+      });
+      expect(res.status).toBe(201);
+      expect(res.body.warnings).toEqual(
+        expect.arrayContaining([
+          expect.objectContaining({ code: 'UNKNOWN_KEY', severity: 'warning' }),
+        ]),
+      );
+    });
+
+    it('rejects malformed frontmatter with 400', async () => {
       const res = await createSkillAsOwner({
         name: 'bad-frontmatter-skill',
-        frontmatter: { 'not-a-real-key': 'value' },
+        frontmatter: { 'user-invocable': 'yes' },
       });
       expect(res.status).toBe(400);
       expect(res.body.issues).toEqual(
-        expect.arrayContaining([expect.objectContaining({ code: 'UNKNOWN_KEY' })]),
+        expect.arrayContaining([expect.objectContaining({ code: 'INVALID_TYPE' })]),
       );
     });