Repository navigation
fix(coding-agent): rediscover skills when a task agent autoloads one the session has not seen - #15723
shyamsivakumar wants to merge 4 commits into
Conversation
…the session has not seen Agents are rediscovered on every task spawn, but the parent session's skill list is fixed at startup. An agent whose autoload-skills names a skill added after startup got that skill silently dropped. Call the session's existing refreshSkills() once before resolving autoload skills when a name is missing.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
P1 — focused fix for late-added task-agent skills; no overlapping fix found. Not anchored to diff
|
|
should-fix — regression coverage: |
|
should-fix — changelog attribution: This new |
…session started Spawns through runStructuredSubagent with a parent whose skill list predates the skill, and asserts the child receives the SKILL.md body. Fails without the refresh in structured-subagent.ts.
![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: 5cb2b581a4
ℹ️ 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".
| if (!names?.length || !session.refreshSkills) return; | ||
| const known = new Set((session.skills ?? []).map(skill => skill.name)); | ||
| if (names.every(name => known.has(name))) return; | ||
| await session.refreshSkills(); |
There was a problem hiding this comment.
When the caller is itself a task agent and adds a skill before spawning a nested agent, this call is a no-op: executor.ts:4420 creates every child with skills: options.skills, and sdk.ts:4611 consequently sets skillsReloadable to false, so AgentSession.refreshSkills() never reruns discovery. The subsequent resolveAutoloadSkills still reads the child's startup snapshot and silently drops the newly added skill, even though nested spawns are enabled by the default recursion depth; preserve reloadability for inherited child snapshots (or provide a forced rediscovery path) before relying on this call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62cabfd10d
ℹ️ 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".
| const known = new Set((session.skills ?? []).map(skill => skill.name)); | ||
| if (names.every(name => known.has(name))) return; | ||
| await session.refreshSkills(); |
There was a problem hiding this comment.
When an RPC/ACP batch or parallel task calls this helper immediately after a skill is added, every spawn can read the same stale session.skills snapshot and enter refreshSkills() before the first call completes. TaskTool.#runSyncSpawns starts rows concurrently (bounded by task.maxConcurrency, which defaults to 32), while SessionTools.refreshSkills() performs a full capability scan and queues a serialized base-system-prompt rebuild, so one batch can issue up to 32 scans and 32 serial rebuilds and leave its final children waiting behind all of them. Coalesce the in-flight refresh per parent session and re-check the requested names after awaiting it.
Useful? React with 👍 / 👎.
2 participants
What
I hit this when an agent's new skill was silently skipped in a long-running session; this reuses the existing refreshSkills to fix it.
Before a task spawn resolves an agent's
autoload-skills, the spawn now checks that every named skill is in the parent session's skill list. If one is missing, it calls the session's existingrefreshSkills()once, then resolves as before. When every named skill is already known, nothing changes.Why
Agents are rediscovered on every task spawn, but the session's skill list is fixed at startup.
resolveAutoloadSkillsintask/structured-subagent.tsfilters the agent'sautoload-skillsagainst that startup list, so a skill added after the session started was dropped with no warning: the agent ran without its skill.Testing
Reproduction: a headless
omp -psession in a fresh git repository creates.omp/skills/zebra-check/SKILL.md(body containsZEBRA-4417) and.omp/agents/zebra-agent.mdwithautoload-skills: zebra-check, then spawnszebra-agentand asks for the password without reading files.NONE.ZEBRA-4417.Checks on this branch:
bun run check:tsexits 0 (oxlint, oxfmt, and type checks for all packages).bun test test/task test/discovery/agent-fields.test.ts test/skill-prompt-message.test.ts test/agent-session-tree-skill-injection.test.tsinpackages/coding-agent: 561 pass, 0 fail.check:rsand the full test suite were not run.bun checkpasses (TypeScript half only:bun run check:ts)