feat: finish skills migration

[jif-oai]

No description provided.

[chatgpt-codex-connector]

💡 Codex Review

codex/codex-rs/ext/skills/src/render.rs

Lines 69 to 70 in 1dbce87

	skill_lines.push(format!(
	"- {omitted} additional {skill_word} omitted from this bounded skills list."

Preserve warnings when skills are omitted

When the catalog exceeds MAX_AVAILABLE_SKILLS_CHARS, this path only adds a model-visible bullet about omitted skills and never emits the user-facing WarningEvent that the old build_available_skills path produced. Existing app-server coverage still expects a thread-scoped warning notification when the skills budget omits entries (codex-rs/app-server/tests/suite/v2/turn_start.rs:620-632), so users can now lose the warning that many skills were left out of the model-visible list.

ℹ️ 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".

[jif-oai]

1. Renderer equivalence is not preserved. True.
   Old: an empty visible skill set returned no skills section at all.
   New: the extension renders:

   <skills_instructions>

   • No prompt-visible skills are currently available.
     </skills_instructions>

   Old under budget pressure: build_available_skills could alias roots, compress paths, truncate descriptions, emit the old warning string, and record render metrics.
   New: it always calls the renderer with no roots/aliases, drops whole entries when over budget, and only adds a plain “N additional skills omitted” line.

2. Budget warning behavior is still missing/broken. True.
   Old: if the catalog overflowed, the user saw a thread-scoped warning like:

   Exceeded skills context budget of 2%...
   thread_id = Some(actual_thread_id)

   New: catalog-budget omission has no warning event from the renderer. Provider/load warnings routed through app-server would currently use:

   thread_id = None

   and the standalone MCP server installs the extension with the default noop event sink, so equivalent extension warnings would be dropped there.

3. Model-input ordering can still break. True, but specifically on refresh/change.
   Local HEAD fixes the initial-context path enough that the first unchanged catalog is usually emitted before user input. But when the catalog changes later, TurnInputContributor emits the catalog as an injection item, and run_turn records user input before injection items.

   Old expected second request tail:

   ...,
   user: "second input"

   New changed-catalog path:

   ...,
   user: "second input",
   developer: "<skills_instructions>..."

   That matches the SDK failure where the last input became <skills_instructions> instead of "second input".

4. Explicit skill selection rules are not preserved. True.
   Old plain $name selection required exactly one matching skill and no app connector with the same slug.

   Old ambiguous example:

   skills: repo/demo, user/demo
   input: use $demo
   result: no selected skill

   New extension selector:

   result: first enabled skill named demo is selected

   Old connector collision example:

   skill: alpha-skill
   app connector slug: alpha-skill
   input: $alpha-skill
   result: no skill injection

   New: no connector-slug count is checked, so the skill can be injected.

5. Dependency prompting / analytics can be bypassed. True.
   This follows from fix: update package-lock.json name to codex #4. Core dependency prompting and invocation analytics are still driven by the old core selector. If the extension selects a host skill that core would reject, the extension can inject it while core thinks no skill was selected.

   Example:

   skills: two skills both named demo
   input: $demo

   Old/core selector:

   selected = []
   no dependency prompt
   no invocation tracking
   no prompt injection

   New extension path:

   selected = first demo
   prompt injected
   core still records no selected skill, so dependency prompt / analytics are skipped

6. Executor/environment routing is incomplete. True.
   TurnInputContext has enough information:

   environment_id, cwd, is_primary

   but SkillListQuery only gets executor authority IDs.

   Old/needed example:

   env A: environment_id=exec-1, cwd=/repo-a, primary=true
   env B: environment_id=exec-1, cwd=/repo-b, primary=false

   A cwd-scoped provider needs to discover different repo-local skills for /repo-a vs /repo-b. New query collapses both down to the same authority id, so the provider cannot distinguish them or know which env is primary.

[github-actions]

Closing this pull request because it has had no updates for more than 14 days. If you plan to continue working on it, feel free to reopen or open a new PR.