3 Commits

• Overlap executor skill reads with namespace discovery (#30225)

  ## Why
  
  Environment skill discovery needs two independent pieces of information:
  
  - plugin namespaces from `plugin.json` files; and
  - skill metadata from each `SKILL.md` file.
  
  Today these happen in sequence. Codex waits for every plugin namespace
  lookup to finish before it starts reading any skill files. On a remote
  executor, that creates an avoidable network-latency barrier.
  
  ```text
  before: walk -> namespace lookups -> skill reads -> build catalog
  after:  walk -> namespace lookups ─┐
               -> skill reads ───────┴-> build catalog
  ```
  
  ## What changes
  
  - Read and parse skill files without waiting for plugin namespace
  discovery.
  - Resolve root and nested plugin namespaces concurrently.
  - Join both results only when constructing the final qualified skill
  names.
  - Keep the existing 64-skill concurrency bound, output ordering,
  warnings, metadata behavior, and namespace rules.
  
  ## Testing
  
  The regression test makes plugin manifest lookup wait until a `SKILL.md`
  read has started. The old serialized pipeline would time out; the new
  pipeline completes and still returns the correctly namespaced skill.
  
  `just test -p codex-core-skills` passes all 111 tests.
  
  ## Out of scope
  
  This does not add an exec-server endpoint, batch filesystem calls, or
  reduce the number of files transferred. A frontmatter-only read or
  server-side skill catalog can remain a separate follow-up if benchmarks
  show that transferred bytes are the next bottleneck.

  jif · 2026-06-26 18:37:59 +00:00
  a938d5f607
• Reuse walk inventory for environment skill metadata (#30145)

  ## Why
  
  Environment skill discovery already asks the executor to run one
  `fs/walk`. That response contains every regular file path found under
  the selected root, including any `agents/openai.yaml` files.
  
  Today Core keeps the discovered `SKILL.md` paths but discards the rest
  of that file inventory. It then sends one `fs/getMetadata` request per
  skill just to ask whether `agents/openai.yaml` exists. A root with 66
  skills and no metadata therefore pays for 66 unnecessary network round
  trips.
  
  ## What changes
  
  - Keep the `fs/walk` file and directory inventory for the duration of
  the scan.
  - Associate each discovered `SKILL.md` with metadata that is known
  present, known absent, or still requires a fallback probe.
  - Read a known `agents/openai.yaml` directly instead of statting it
  first.
  - Skip the metadata request entirely when a complete walk shows that the
  skill has no `agents` directory.
  - Read a known `SKILL.md` and `agents/openai.yaml` concurrently.
  - Keep parsing and validation in `core-skills`.
  
  The inventory is scan-local. This does not add another cache or change
  cache lifetime.
  
  ## Network impact
  
  For a complete scan of 66 valid skills with no `agents/openai.yaml`, and
  one root `.codex-plugin/plugin.json`:
  
  | Operation | Current | After this PR |
  | --- | ---: | ---: |
  | `fs/walk` | 1 | 1 |
  | Read `SKILL.md` | 66 | 66 |
  | Stat `agents/openai.yaml` | 66 | 0 |
  | Read `agents/openai.yaml` | 0 | 0 |
  | Stat plugin manifest | 1 | 1 |
  | Read plugin manifest | 1 | 1 |
  | **Total executor RPCs** | **135** | **69** |
  
  This removes exactly 66 request/response exchanges from the common cold
  scan. Warm scans remain at zero discovery RPCs because the thread-level
  executor catalog cache is unchanged.
  
  When metadata exists, each file still requires one read. This PR removes
  only the preceding existence check; it does not batch file contents into
  a new RPC.
  
  ## Correctness fallbacks
  
  Absence is trusted only when the walk is complete and the metadata
  directory was not present. Core keeps the existing `getMetadata`
  fallback when:
  
  - the walk was truncated;
  - the walk reported an error; or
  - an `agents` directory was observed but `openai.yaml` was not, which
  preserves support for file symlinks and traversal boundaries.
  
  ## Deliberate scope
  
  This PR changes only the environment skill loader and its existing
  filesystem-call regression coverage. It does not:
  
  - change `fs/walk` or any exec-server protocol;
  - add `readFiles` or a skills-list endpoint;
  - change thread caching;
  - change local skill discovery;
  - change exec-server request concurrency; or
  - optimize plugin-manifest lookup.
  
  The plugin-manifest stat is intentionally left in place, which is why
  this PR reaches 69 calls rather than the broader 68-call estimate. That
  lookup has separate alternate-path, ancestor, and symlink semantics and
  should not be mixed into this change.

  jif · 2026-06-26 01:47:00 +01:00
  8ebf71ec25
• Cache plugin namespace during executor skill discovery (#29831)

  ## Why
  
  Executor skill discovery runs before the remote skills catalog is
  available. For a remote environment, each `ExecutorFileSystem` operation
  becomes an exec-server RPC.
  
  Previously, every discovered `SKILL.md` independently resolved its
  plugin namespace by walking its ancestors and probing both supported
  manifest locations. In the common `plugin/skills/<skill>/SKILL.md`
  layout, that repeats 8 RPCs per skill even though every skill under the
  plugin root uses the same namespace. These lookups happen while skills
  are parsed, so their cost grows linearly with the skill count and adds
  directly to first-turn latency.
  
  A selected capability root can also contain standalone skills, multiple
  sibling plugins, nested plugins, or symlinked directories. The
  optimization therefore needs to retain the nearest-ancestor namespace
  for each skill rather than assuming the selected root represents exactly
  one plugin.
  
  ## What changed
  
  - record plugin-root candidates from directory entries already returned
  during skill discovery
  - prune candidates that are not ancestors of any discovered `SKILL.md`
  before reading manifests
  - resolve each relevant plugin root once, with one fallback lookup per
  canonical traversal root for symlinked directories
  - select the nearest cached plugin namespace for each discovered skill
  - avoid namespace lookup entirely when the root contains no skills
  
  No additional directory traversal is required. Namespace work now scales
  with the number of plugin roots that contain discovered skills, rather
  than the total number of skills or unrelated sibling plugins. Standalone
  and nested-plugin names keep their previous behavior.
  
  ## Benchmarks
  
  I used a temporary counting `ExecutorFileSystem` around the real local
  filesystem. Each filesystem operation was counted as one remote RPC and
  given 1 ms of injected latency. Each variant ran three times; times
  below are medians.
  
  ### One plugin with 100 skills
  
  | Operation | Before | After | Delta |
  | --- | ---: | ---: | ---: |
  | `get_metadata` | 1,002 | 303 | -699 |
  | `read_file` | 200 | 101 | -99 |
  | `read_directory` | 102 | 102 | 0 |
  | **Total filesystem RPCs** | **1,304** | **506** | **-798 (-61.2%)** |
  | **Median load time** | **2.890 s** | **0.997 s** | **2.90× faster** |
  
  The namespace-specific work drops from 800 RPCs to 2 in this layout.
  
  ### Multiple plugins under one selected root
  
  These runs compare the correct pre-optimization implementation with the
  final nearest-plugin-root cache. The total plugin skill count stays at
  100 while the number of plugin roots changes.
  
  | Layout | Before RPCs | After RPCs | Reduction | Before | After |
  Speedup |
  | --- | ---: | ---: | ---: | ---: | ---: | ---: |
  | 2 plugins × 50 skills | 1,312 | 530 | 59.6% | 1,819 ms | 711 ms |
  2.56× |
  | 10 plugins × 10 skills | 1,344 | 578 | 57.0% | 1,850 ms | 778 ms |
  2.38× |
  | 50 plugins × 2 skills | 1,504 | 818 | 45.6% | 2,094 ms | 1,086 ms |
  1.93× |
  | 10 plugins × 10 skills + 10 standalone skills | 1,596 | 630 | 60.5% |
  2,209 ms | 860 ms | 2.57× |
  
  The remaining cost grows with the number of relevant plugin manifests.
  Each relevant manifest is read once instead of once per skill, while
  sibling plugins with no discovered skills are not read. Absolute latency
  savings depend on the executor's real RPC latency.
  
  ## Tests
  
  - `just test -p codex-core-skills` (109 passed across the library and
  integration-test binaries)
  - one integration test covers standalone, outer-plugin, nested-plugin,
  and unused sibling-plugin layouts, and asserts the exact set of
  manifests read

  jif · 2026-06-24 17:14:34 +01:00
  390b73133b