mirror of
https://github.com/openai/codex.git
synced 2026-10-25 18:28:38 +00:00
jif/windows-executable-resolution-pr6
13
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 jif
|
220f5b76b2 |
Load executor skills without host path conversion (#29626)
## Why After #28918, selected skill roots are `PathUri`, but the executor skill provider still converts them to the app-server host's `AbsolutePathBuf`. A foreign Windows root therefore cannot be discovered by a Unix host, and the inverse has the same problem. This PR keeps executor skill discovery and reads on the filesystem that owns the selected root while reusing the existing skill rules. ## What changed - Generalize the existing skill traversal to operate on `PathUri` through `ExecutorFileSystem`, preserving its depth, directory, symlink, and sibling-metadata concurrency behavior. - Add a small environment skill loader that reuses the shared discovery, frontmatter validation, dependency parsing, product policy, and prompt-visibility rules. - Keep the environment id and entrypoint `PathUri` in the skill catalog, then route `skills.read` back through the same environment filesystem. - Preserve the executor's path convention when deriving catalog handles, including literal backslashes in POSIX filenames. - Resolve plugin namespaces from nearby manifests through URI-native filesystem reads. - Cover foreign Windows roots, executor-owned reads, namespaces, metadata, policy, and path identity. ```text selected root (PathUri) | v shared discovery over ExecutorFileSystem | v environment-bound catalog entry --skills.read--> same ExecutorFileSystem ``` No second filesystem abstraction or duplicate traversal implementation is introduced. ## Stack 1. #29614 — add lexical `PathUri` containment. 2. #29620 — share URI-native manifest path resolution. 3. #28918 — keep selected plugin roots and resources URI-native. 4. **This PR** — load executor skills without host path conversion. 5. #29628 — resolve executor MCP working directories without host path conversion. |
||
|
jif
|
2e69966cd8 |
Make selected plugin roots URI-native (#28918)
## Why Selected capability roots belong to the executor filesystem, not the app-server host. Converting their path strings into the host's native `Path` breaks whenever the two machines use different path conventions, such as a Windows executor behind a Unix app-server. This PR establishes `PathUri` as the selected-plugin boundary so the executor remains authoritative for its paths. ## What changed - Require `selectedCapabilityRoots[].location.path` to be a canonical `file:` URI and deserialize it directly as `PathUri`; native path strings are rejected. - Update the app-server schema, generated TypeScript, examples, and request coverage for the URI contract. - Keep selected roots, resolved plugin locations, manifest paths, and manifest resources as `PathUri`. - Inspect and read plugin roots and manifests only through the selected environment's `ExecutorFileSystem`. - Parse executor manifests with the shared URI-native parser from #29620 instead of projecting them onto the host filesystem. - Enforce resource containment lexically and preserve the root URI's POSIX or Windows path convention. - Cover foreign Windows plugin roots and URI-native manifest resources. ```text thread/start selectedCapabilityRoots[].location.path = "file:///C:/plugins/demo" | PathUri v ExecutorFileSystem | +--> plugin.json +--> manifest resources ``` This PR stops at the shared selected-plugin representation. The next two PRs remove the remaining host-path projections in the skill and MCP consumers. ## Stack 1. #29614 — add lexical `PathUri` containment. 2. #29620 — share URI-native manifest path resolution. 3. **This PR** — keep selected plugin roots and resources URI-native. 4. #29626 — load executor skills without host path conversion. 5. #29628 — resolve executor MCP working directories without host path conversion. |
||
|
charlesgong-openai
|
64bdeed9f7 |
[codex] Preserve skill descriptions outside model context (#29006)
## Why Skill descriptions are used in model-visible lists: the default available-skills catalog that supports implicit selection, and the on-demand `skills.list` tool response used to discover orchestrator skills. A single overlong description should not consume a disproportionate share of either list. Enforcing the 1024-character limit while loading or migrating skills is the wrong boundary: it rejects otherwise-valid skills and discards metadata that non-model consumers and full skill reads may need. Skill metadata and `SKILL.md` content should remain intact; the cap belongs at model-visible list rendering boundaries. ## What changed - Preserve full `description` and `metadata.short-description` values when loading skills. - Preserve full external-agent command descriptions during `source-command-*` migration instead of skipping commands solely because their descriptions exceed 1024 characters. - Preserve full normalized orchestrator descriptions in the underlying skills catalog. - Cap each description at 1024 Unicode characters when rendering the default available-skills context in `codex-core-skills` and `codex-skills-extension`. - Apply the same cap when serializing descriptions in the model-visible `skills.list` response. - Render truncated descriptions as 1021 original characters plus `...`. - Leave explicit `$skill` injection, `skills.read`, underlying metadata, and on-disk `SKILL.md` files unchanged and full-fidelity. ## Implicit skill selection Codex injects a bounded catalog containing each implicitly allowed skill's name, description, and source locator, together with instructions to use a skill when the task clearly matches its description. The model makes that semantic choice; after selecting a skill, it reads the full `SKILL.md` from its filesystem or provider resource. Explicit `$skill` mentions remain a separate path that injects the full skill instructions. For orchestrator skills, `skills.list` provides bounded discovery metadata before `skills.read` returns the full selected resource. ## Test plan - `just test -p codex-core-skills` - `just test -p codex-skills-extension` - `just test -p codex-external-agent-migration` The focused regressions verify that overlong metadata is preserved at load and migration boundaries while default available-skills rendering and `skills.list` output produce the 1021-character prefix plus `...`. |
||
|
xl-openai
|
e83b7841b0 |
[codex] Reuse parsed plugin skills during session startup (#28844)
## Summary - Preserve raw plugin skill-root snapshots in the matching loaded-plugin cache entry, keyed by the effective plugin root identity including namespace. - Pass those snapshots through `SkillsLoadInput` as an optional preload, so session startup reuses plugin parsing while ordinary skill loads pass `None`. - Keep plugin skill loading cohesive: the existing loaders accept the optional snapshots directly, and uncached or marketplace-detail paths do not create a cache. ## Why Plugin discovery already parses plugin skills to determine available capabilities. Cold session startup then scanned and parsed the same roots again while building the skills snapshot. This solves the same duplicate-work problem as #28623 while keeping ownership narrow: `PluginsManager` creates and owns `PluginSkillSnapshots` only for its loaded-plugin cache entry; `SkillsService` consumes an optional clone. Entry replacement or clearing naturally drops the snapshots, with no separate generation, capacity policy, or watcher coupling. ## Validation - `cargo clippy -p codex-core-skills --all-targets -- -D warnings` - `just test -p codex-core-plugins skills_service_reuses_skills_parsed_during_plugin_load` - `just test -p codex-core-skills namespaces_plugin_skills_using_provided_namespace` - `just fmt` |
||
|
Matthew Zeng
|
c73296a0f0 |
[codex] Pass plugin namespace into skill loading (#28608)
## What changed - retain the parsed plugin manifest namespace on loaded plugins - carry that namespace through `PluginSkillRoot` and `SkillRoot` - use the provided namespace when qualifying plugin skill names - include the namespace in the skills cache key ## Why Plugin loading has already parsed `plugin.json`, but skill parsing currently walks every `SKILL.md` ancestor and probes/reads the manifest again to reconstruct the same namespace. Passing the parsed namespace removes those repeated filesystem calls, which are particularly costly on remote filesystems. Context: https://openai.slack.com/archives/C0ARA9GF5D4/p1781639496496439?thread_ts=1781202444.891669&cid=C0ARA9GF5D4 ## Impact Plugin skill names remain unchanged. A regression test uses a deliberately different on-disk manifest name to verify that plugin roots use the provided parsed namespace. ## Validation - `just test -p codex-core-skills -p codex-core-plugins -p codex-plugin -p codex-utils-plugins` (352 passed) - `just fix -p codex-core-skills -p codex-core-plugins -p codex-plugin -p codex-utils-plugins` - `just fmt` |
||
|
jif
|
0318381762 |
Replace SkillsManager with SkillsService (#28705)
## Why Host skill discovery was still exposed as a manager even though it is a process-owned service shared by sessions, the app-server catalog, and file-watcher invalidation. The skills extension also consumed an ad hoc loaded-skills wrapper instead of a named immutable snapshot. ## What changed - replace `SkillsManager` with concrete `SkillsService` - make the service cache and return immutable `HostSkillsSnapshot` values - migrate the skills extension host provider to the snapshot boundary - migrate app-server catalog, watcher, and invalidation paths to the service This keeps the service limited to host discovery, caching, roots, and invalidation. Catalog rendering and invocation remain extension responsibilities for the next stacked change. |
||
|
Adam Perry @ OpenAI
|
968a3ac9c1 |
[codex] make PathUri::from_abs_path infallible (#27976)
## Why `PathUri::from_abs_path` can fail for absolute paths that do not have a normal `file:` URI representation, forcing filesystem call sites to handle a conversion error even though the original path can be preserved losslessly. ## What Make `from_abs_path` infallible and migrate its callers. Unrepresentable paths use `file:///%00/bad/path/<base64>`, encoding Unix bytes or Windows UTF-16LE; `to_abs_path` validates and decodes that fallback. The leading encoded null reserves a namespace that cannot collide with a real Unix or Windows path, and fallback URIs remain opaque to lexical path operations. ## Validation Added path-URI coverage for Unix null and non-UTF-8 paths, Windows device/verbatim and non-Unicode paths, serialization, malformed fallbacks, opaque lexical operations, invalid native payloads, and literal `/bad/path` collision resistance. |
||
|
Adam Perry @ OpenAI
|
b2a4e3be27 |
[codex] migrate ExecutorFileSystem paths to PathUri (#27424)
## Why We're moving exec-server to use PathUri for its internal path representations. ## What Move `ExecutorFileSystem` APIs to use `PathUri` instead of `AbsolutePathBuf`. Future changes will convert higher-level parts of exec-server. |
||
|
Adam Perry @ OpenAI
|
4a05d3b282 |
[codex] remove EnvironmentPathRef (#27433)
We're switching to using a static encoding of the host path in `PathUri`. We may need a type like this again but we can add it when it's more compelling. Stacked on #27454. |
||
|
jif
|
7b3eb8e4bc |
skills: expose remote skill resource tools (#27388)
## Why PR #27387 makes backend plugin skills discoverable and invocable without an executor, but resources referenced by those skills still sit behind the generic MCP resource surface. The model needs a skills-owned API that preserves the provider authority and package boundary instead of treating remote resources like local files. This is stacked on #27387. ## What - Adds one `skills` namespace with bounded `list` and `read` tools for remote skill providers. - Revalidates `authority + package` against the live remote catalog on every read, then routes the opaque resource ID back through that provider. - Allows the backend provider to read canonical child `skill://` resources while rejecting cross-package, non-canonical, query, fragment, and traversal-shaped URIs. - Caps each serialized tool result at 8 KB. Lists are paginated; reads return an opaque continuation cursor. - Marks the JSON output as external context so memory generation can apply its normal suppression policy. - Deliberately does not add `skills.search`; that waits for a bounded plugin-service search contract. ## Tool contract Pseudo-Python matching the wire shape: ```python from typing import Literal, NotRequired, TypedDict class RemoteSkillAuthority(TypedDict): kind: Literal["remote"] id: str # e.g. "codex_apps" class RemoteSkill(TypedDict): authority: RemoteSkillAuthority package: str # opaque provider-owned package ID name: str description: str main_resource: str # opaque provider-owned SKILL.md ID class SkillsListParams(TypedDict): cursor: NotRequired[str] class SkillsListResult(TypedDict): skills: list[RemoteSkill] next_cursor: str | None warnings: list[str] truncated: bool class SkillsReadParams(TypedDict): authority: RemoteSkillAuthority # copied from skills.list package: str # copied from skills.list resource: str # provider-owned child resource ID cursor: NotRequired[str] # copy next_cursor to continue class SkillsReadResult(TypedDict): resource: str contents: str next_cursor: str | None truncated: bool class Skills: def list(self, params: SkillsListParams) -> SkillsListResult: ... def read(self, params: SkillsReadParams) -> SkillsReadResult: ... ``` There is one namespace for all remote skills, not one tool or MCP server per skill. No resource ID is converted into a filesystem path. ## Backend dependency `/ps/mcp` must support direct reads of child resources such as `skill://plugin_demo/deploy/references/deploy.md`. This PR implements and tests the Codex side of that contract; production child reads remain dependent on the corresponding plugin-service support. Search remains out of scope until that service exposes a bounded search/resource API. ## Validation - Added an app-server integration test covering `skills.list` followed by `skills.read` with no executor. - Ran `just fmt`. - Ran `just bazel-lock-update` and `just bazel-lock-check`. - Did not run Rust tests or Clippy locally, per request; CI will run them. |
||
|
jif
|
a287c5dffd |
skills: make backend plugin skills invocable without an executor (#27387)
## Why #27198 made the extension-owned `codex_apps` MCP connection the hosted plugin runtime, but its `mcp/skill` resources still bypassed the skills extension. App-server could list and read those resources through generic MCP APIs, but a thread with no selected environment did not expose them in the model's skills catalog or load their `SKILL.md` through `$skill`. Hosted skills should stay remote while using the same typed catalog, source authority, deduplication, bounded contextual catalog, and selected-skill prompt injection as host and executor skills. They should not be downloaded or exposed as ambient filesystem paths. ## What changed - Add a session-scoped `McpResourceClient` over the replaceable MCP connection manager so resource list/read calls follow startup and refresh replacements. - Add a `BackendSkillProvider` that pages `codex_apps` resources, accepts bounded and validated `mcp/skill` entries, and reads a selected skill's `SKILL.md` through the same MCP connection. - Register the remote provider in app-server and include it in the skills catalog even when a thread has no selected capability roots or executor. - Contribute hosted skill metadata through the bounded `AvailableSkillsInstructions` developer-context path, exclude remote entries from per-turn catalog injection, and classify `<skills>` messages as contextual developer content so rollback can trim and rebuild them correctly. ## Testing - Extend the app-server MCP resource integration test with `environments: []` to exercise two-page discovery, filter a non-`mcp/skill` resource, verify the escaped developer catalog entry and user-role `<skill>` fragment containing the fetched `SKILL.md`, and preserve generic MCP resource reads. - Add core event-mapping coverage that classifies `<skills>` developer messages as contextual history. |
||
|
jif
|
89ac3ec27c |
Load selected executor skills through extensions (#27184)
## Why
CCA is moving toward a split runtime where the orchestrator may not have
a filesystem, while executors can expose preinstalled plugins and
skills. A thread therefore needs to select capabilities without asking
app-server or core to interpret executor-owned paths through the
orchestrator's filesystem.
The longer-term model is broader than executor skills:
- A plugin is a bundle of skills, MCP servers, connectors/apps, and
hooks.
- A plugin root can be local, executor-owned, or hosted by a backend.
- Components inside one plugin can use different access and execution
mechanisms. A skill may be read from a filesystem or through backend
tools; an HTTP MCP server can run without an executor; a stdio MCP
server or hook needs an execution environment.
- Core should carry generic extension initialization data. The extension
that owns a component should discover it, expose it to the model, and
invoke it through the appropriate runtime.
This PR establishes that architecture through one complete vertical:
selecting a root on an executor, discovering the skills beneath it,
exposing those skills to the model, and reading an explicitly invoked
`SKILL.md` through the same executor.
## Contract
`thread/start` gains an experimental `selectedCapabilityRoots` field:
```json
{
"selectedCapabilityRoots": [
{
"id": "deploy-plugin@1",
"location": {
"type": "environment",
"environmentId": "workspace",
"path": "/opt/codex/plugins/deploy"
}
}
]
}
```
The root is intentionally not classified as a "plugin" or "skill" in the
API. It can point at a standalone skill, a directory containing several
skills, or a plugin containing skills and other components. This PR only
teaches the skills extension how to consume it; later extensions can
resolve MCP, connector, and hook components from the same selection.
The platform-supplied `id` is stable selection identity. The location
says which runtime owns the root and gives that runtime an opaque path.
App-server does not inspect or canonicalize the path.
## What changed
### Generic thread extension initialization
App-server converts selected roots into `ExtensionDataInit`. Core
carries that generic initialization value until the final thread ID is
known, then creates thread-scoped `ExtensionData` before lifecycle
contributors run.
This keeps `Session` and core independent of the capability-selection
contract. The initialization value is consumed during construction; it
is not retained as another long-lived `Session` field.
### Executor-backed skills
The skills extension now owns an `ExecutorSkillProvider` that:
- resolves the selected environment through `EnvironmentManager`
- discovers, canonicalizes, and reads skills through that environment's
`ExecutorFileSystem`
- contributes the bounded selected-skill catalog as stable developer
context
- reads an explicitly invoked skill body through the authority that
listed it
- warns when an environment or root is unavailable
- never falls back to the orchestrator filesystem for an executor-owned
root
Skill catalog and instruction fragments have hard byte bounds, which
also bound them below the 10K-token per-item context limit. If a
selected executor skill has the same name as a legacy local skill, the
executor selection owns that invocation and the local body is not
injected a second time.
Existing local and bundled skill loading remains in place. Omitting
`selectedCapabilityRoots` therefore preserves current local-only
behavior.
## Current semantics
- Only environment-owned locations are represented in this first
contract.
- Roots are resolved by the destination extension, not by app-server or
core.
- An unavailable executor or invalid root produces a warning and no
capabilities from that root; it does not trigger a local-filesystem
fallback.
- Selection applies to a newly started active thread.
- MCP servers, connectors, and hooks beneath a selected plugin root are
not activated yet.
- Selection is not yet persisted or inherited across resume, fork, or
subagent creation. Existing local capabilities continue to behave as
they do today in those flows.
## Planned vertical follow-ups
1. **Hosted HTTP MCP:** add an extension-backed HTTP MCP source that
works without an executor, then replace the special-purpose MCP plugins
loader with that implementation.
2. **Executor MCP:** register and execute stdio MCP servers through the
environment that owns the selected plugin root.
3. **Backend skills:** add a hosted skill source whose catalog and
bodies are accessed through extension tools rather than a filesystem.
4. **Connectors and hooks:** activate those components through their
owning extensions, using the same selected-root boundary and
component-specific runtime.
5. **Durable selection:** define the desired-selection lifecycle,
persist it, and make resume, fork, and subagent inheritance explicit
rather than accidental.
6. **Local convergence:** incrementally route existing local plugin,
skill, and MCP loading through the same extension model while preserving
current local behavior.
Each follow-up remains reviewable as an end-to-end capability. The
platform selects roots, generic thread extension data carries the
selection, and the owning extension resolves and operates its component.
## Verification
Coverage added for:
- app-server end-to-end discovery and explicit invocation of a skill
inside an executor-selected plugin root
- exclusive invocation when a selected executor skill collides with a
local skill name
- executor filesystem authority for discovery, canonicalization, and
reads
- thread extension initialization before lifecycle contributors run
- stable executor catalog context, explicit invocation, context
rebuilding, hidden skills, and preserved host/remote catalog behavior
Targeted protocol, core-skills, skills-extension, core lifecycle, and
app-server executor-skill tests were run during development.
|
||
|
jif
|
d46a98d31a |
Bridge host-loaded skills into the skills extension (#26172)
## Why The skills extension needs to become the path that exposes local host skills without losing the behavior already owned by core skill loading. Host skill discovery is not just `$CODEX_HOME/skills`: it also includes config layers, bundled-skill settings, plugin roots, runtime extra roots, and the filesystem for the selected primary environment. Rather than making the extension reload host skills and risk drifting from that authoritative load, this PR bridges the already-loaded per-turn skills outcome into the extension. That lets the extension advertise host skills and inject explicit `$skill` prompts while preserving the same roots, disabled/hidden state, rendered paths, and environment-backed file reads that the legacy path uses. ## What Changed - Adds `HostLoadedSkills` in `core-skills` to wrap the turn's `SkillLoadOutcome` and read `SKILL.md` through the filesystem that loaded that skill. - Stores `HostLoadedSkills` in turn extension data for normal turns and review turns, so the skills extension can consume the loaded host catalog without reloading it. - Adds `HostSkillProvider` under `ext/skills/src/provider/host.rs`, mapping host-loaded skill metadata into the skills-extension catalog/read contract. - Registers the host provider by default from `codex_skills_extension::install()`. - Preserves host skill metadata such as dependencies, disabled state, hidden-from-prompt policy, and slash-normalized display paths. - Passes host-loaded skills through `SkillListQuery` and `SkillReadRequest` so explicit skill invocation reads only resources from the loaded host catalog. - Adds integration coverage for a real legacy `$CODEX_HOME/skills/.../SKILL.md` skill being listed and injected through the installed extension. ## Testing - Added `installed_extension_loads_host_skills_from_legacy_roots` in `ext/skills/tests/skills_extension.rs`. - `just test -p codex-skills-extension` |