perf(skills): batch remote skill discovery reads#31507
Draft
nornagon-openai wants to merge 1 commit into
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a generic batched remote-filesystem read primitive and use it in the legacy skill loader so remote thread startup no longer fans out into one RPC per discovered skill file.
The change adds
ExecutorFileSystem::read_filesand the genericfs/readFilesexec-server RPC with per-path success/error results, a 128-path request cap, and a 64 MiB aggregate decoded-response cap. Oversized files fail individually without consuming the remaining batch budget, so later small files can still succeed. Mixed-version clients fall back to individualfs/readFilecalls when an older exec server does not know the new method.Skill discovery now uses the existing bounded server-side
fs/walkinventory. Walk can report canonical paths while following directory symlinks, preserving canonical skill identity without a client-side canonicalize RPC per skill on current servers; older walk responses retain the compatibility fallback. Hidden descendants are pruned the same way as the legacy scanner, while the root.agents/skillsdirectory itself remains discoverable.After inventory, the loader reads discovered
SKILL.mdfiles and candidateagents/openai.yamlmetadata through the batched primitive, then keeps parsing, warnings, precedence, ordering, and namespace policy in Codex core. Plugin namespace manifest batching is deliberately left to the per-root namespace work in #31348 rather than duplicating that change.Why
Browser-created remote-environment
thread/starthas a 30 second client timeout. The legacy loader performed remote directory discovery and file reads as many serialized filesystem RPCs, so cold skill discovery could spend most of that budget on network latency instead of parsing.Validation
just test -p codex-core-skills(112 passed)just test -p codex-exec-server-protocol(5 passed)just test -p codex-exec-server batched_reads_return_per_path_errors_and_reject_too_many_paths remote_file_system_batches_paths_and_keeps_per_path_errorsjust test -p codex-exec-server file_system_walk_handles_directory_symlinks file_system_walk_returns_a_bounded_treejust fix -p codex-file-system -p codex-exec-server-protocol -p codex-exec-server -p codex-core-skillsjust fmtgit diff --checkjust test -p codex-exec-serverwas also run: 271 passed, 34 failed, and 2 skipped. The failures are the existing macOS sandbox integration cases in this runner, all failing withsandbox-exec: sandbox_apply: Operation not permitted; the new batched-read tests passed.🧵 Codex Thread