refactor(agent): align skill slugs with published packages#3025
refactor(agent): align skill slugs with published packages#3025daryllimyt wants to merge 7 commits into
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
daryllimyt
changed the title
daryllimyt
added
agents
|
✅ No security or compliance issues detected. Reviewed everything up to 1c12315. Security Overview
Detected Code Changes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 477b68f22a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Confidence score: 5/5
- Safe to merge after the addressed issues were fixed.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
477b68f to
72bf05e
Compare
26518da to
6fe6d45
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72bf05e74e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72bf05e74e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
72bf05e to
104b5a0
Compare
6fe6d45 to
796b9ec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 104b5a0d24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
104b5a0 to
c8e9748
Compare
796b9ec to
83ca8d8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8e9748087
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
83ca8d8 to
6203a77
Compare
c8e9748 to
2a1cc8c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a1cc8ca9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
6203a77 to
56d977c
Compare
2a1cc8c to
b84dda6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b84dda6ad3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b84dda6 to
8ec3a5e
Compare
56d977c to
be3e4a0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ec3a5ec15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
be3e4a0 to
ae0bba9
Compare
8ec3a5e to
38c44ef
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38c44efe41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Legacy rows written by old pods still carry slug IS NULL and project their name as the effective slug, which the partial unique index cannot see. Close the paths that could mint a duplicate effective slug or bind against the wrong owner: - restore_version validates the restored package slug against live rows (same pre-check as the publish paths) before minting the new version. - Workspace-sync skill renames check availability on coalesce(slug, name) so a mapped rename cannot land on a legacy row's effective slug (NameSwapPlan gains an optional availability_column); unmapped imports resolve through get_skill_by_slug so exact owners win and legacy-only ambiguity fails loud. - Dependency-closure exports dedupe per requested slug with exact-owner preference and fail loud (ambiguous_skill_slug) when only legacy rows contend, instead of emitting duplicate specs for one slug. - Preset-sync skill bindings resolve through the runtime resolver (new SkillService.get_skill_by_slug, extracted from get_skill_by_identifier): exact slug owners win over legacy rows and ambiguous legacy-only matches fail loud. - Both restore routes map slug conflicts to 409 skill_slug_conflict instead of escaping as unhandled validation errors. - Seeded drafts carry the allocated live slug: duplicate display-name creates seed SKILL.md with the suffixed slug, and duplicate-name uploads get their manifest name rewritten, so publish no longer hits skill_slug_conflict on untouched drafts.
Full workspace exports reached _projection_from_skills without the exact-owner/legacy dedupe that project_dependency_refs applies, so an expand-window canonical+legacy pair emitted two specs claiming one slug. Factor the dedupe into a shared helper used by both projection paths: exact slug owners win, legacy-only contention fails loud with ambiguous_skill_slug.
A preset version bound to a legacy skill (slug IS NULL) whose name is owned as a slug by a different live skill exported the binding as that non-unique effective slug; the dependency closure then resolved it to the canonical owner, silently rebinding the preset on import. Detect the shadowed state with a batched EXISTS probe and fail loud with shadowed_skill_binding during the expand window.
Sync schemas accepted any non-empty string as a skill slug, so a manifest slug with uppercase or spaces imported verbatim into Skill.slug while the strict runtime resolver returned None — the preset's skill binding silently dropped although the pull succeeded. Apply the canonical SkillName shape to the skill resource spec and preset binding ref slugs so invalid manifests fail loud as parse diagnostics before any database write.
38c44ef to
1c12315
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c123156e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| description="Current package locator used for cross-references.", | ||
| ) | ||
| name: str = Field(min_length=1, description="Human-readable skill name.") | ||
| name: str = Field(min_length=1, description="User-facing display name.") |
There was a problem hiding this comment.
Because the importer now persists this field to Skill.name, a skill.yml with a valid slug but a display name longer than 64 characters passes Pydantic parsing and dependency validation, then hits the String(64) database column during insert/update (name=spec.name) and fails as an unhandled DB error instead of a sync validation diagnostic. Add the same 64-character limit here or validate before assigning it.
Useful? React with 👍 / 👎.
1 participant
Mental model
Skill.idis the stable ResourceHead identity used by internal edges.Skill.slugis the current published package locator. Publishing or restoring a version atomically advances it to that version's rootSKILL.mdfrontmattername.Skill.nameis user-facing display metadata. The column keeps its current name for compatibility; a later migration can rename it todisplay_name.SkillVersion.nameremains the immutable, version-local Agent Skills package identifier and runtime directory name.SkillVersion, and the existing per-preset-resolution and executor duplicate-package guards remain in place.This intentionally does not add
SkillVersion.slug, change the Temporal payload, or move runtime staging away from the resolvedSkillVersion.name.Changes
Skill.slugfrom published/restored frontmatter and fail atomically withskill_slug_conflictwhen another live head owns the locator.Skill.namewhen draft frontmatter or published package names change.slugas the head locator, top-levelnameas display metadata, and versionnameas the immutable package identifier.Rollout note
Changing frontmatter
nameon publish changes slug-authored API, URL, and Workspace Sync references. Internal preset edges are unaffected because they use the stable Skill head ID. Existing skills whose historical stable slug differs from their current version remain readable and syncable; republishing opts them into the new behavior.Related: ENG-1532
Verification
just gen-client-ciproduced no diff.Summary by cubic
Use skill slugs as the package identity and runtime directory; keep
namedisplay-only. Adds expand-window safeguards, strict slug validation, and 409-conflict handling across publish/restore and sync to prevent duplicate effective slugs and shadowed bindings. (ENG-1532)Refactors
Skill.slugis the canonical package locator;Skill.nameis user-facing only. API/DB/docs and generated clients clarify both fields.slugfrom the rootSKILL.mdnameand pre-validate availability (excluding the owner, including against legacy rows withslug IS NULL); conflicts surface as HTTP 409skill_slug_conflicton both public and internal restore routes.SKILL.md: duplicate display-name creates seed a suffixed slug, and duplicate-name uploads rewrite the manifestnameto the allocated slug so untouched drafts publish cleanly.SkillService.get_skill_by_slug; Workspace Sync rename checks validate againstcoalesce(slug, name), and imports/preset bindings resolve via this resolver so exact owners win and legacy-only ambiguity fails loud.ambiguous_skill_slugwhen only legacy rows contend; preset exports rejectshadowed_skill_bindingwhen a legacy binding is masked by a live slug owner; sync schemas enforce the canonicalSkillNamefor both skill specs and preset binding slugs.Migration
SKILL.mdfrontmatternamechanges the slug; update any slug-based API, URL, or Workspace Sync references after publish/restore.slugcontinue to work (fallback toname) until re-publish backfillsslug; sync exports may surfaceambiguous_skill_slugorshadowed_skill_bindingduring the expand window.Written for commit 1c12315. Summary will update on new commits.