feat(skills)!: remove Codex agent support - #254
Merged
Merged
Conversation
Codex now reads skills from the universal `~/.agents` location, so a dedicated Codex host, agent identifier, and per-skill `openai.yaml` manifests are no longer needed. Dropping them removes `--agent codex` and the `codex` host from list, locate, install, and repair output. The universal `~/.agents` host is now always provisioned, guaranteeing at least one install target even when no other supported agent home exists. Add a one-time cleanup that deletes oo-managed skills left under the legacy `~/.codex` home and bundled storage, identified strictly by their `.oo-metadata.json`. Codex does not deduplicate `~/.codex/skills` against `~/.agents`, so the stale copies would otherwise surface as duplicates; user-authored and unmanaged directories are preserved. Signed-off-by: Kevin Cui <bh@bugs.cc>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
Summary by CodeRabbit
WalkthroughThis PR migrates the managed skill agent infrastructure from Codex (legacy) to Universal as the default, always-provisioned agent. The core changes include: removing Codex from the supported agents registry and making Universal always available; renaming bundled skill path constants from codex-specific to generic "managed" naming; updating all path resolution to use Possibly related PRs
✨ Finishing Touches✨ Simplify code
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/application/commands/skills/check.ts (1)
89-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
alwaysProvisionbefore rejecting a missing home.On a clean machine,
--agent universalstill fails here because~/.agentsis checked for existence beforeverifyWritableDirectory()gets a chance to create it. That breaks the new always-provisioned host contract and makes first-run preflight fail unnecessarily.💡 Minimal fix
import { createManagedSkillAgentNotInstalledError, createMissingRequiredSkillAgentError, parseManagedSkillAgentOption, + readManagedSkillAgent, resolveManagedSkillAgentHomeDirectory, } from "./managed-skill-agents.ts"; @@ export async function resolveRequestedManagedSkillHost( env: Record<string, string | undefined>, translator: Pick<CliExecutionContext["translator"], "t">, agentName: BundledSkillAgentName, ): Promise<Array<{ agentName: BundledSkillAgentName; homeDirectory: string }>> { + const agent = readManagedSkillAgent(agentName); const homeDirectory = resolveManagedSkillAgentHomeDirectory(env, agentName); - if (!(await directoryExists(homeDirectory))) { + if (!agent.alwaysProvision && !(await directoryExists(homeDirectory))) { throw createManagedSkillAgentNotInstalledError( agentName, homeDirectory, translator, );As per coding guidelines, "For file operations, attempt the operation and catch the error (EAFP) rather than pre-checking existence then reading (LBYL)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/check.ts` around lines 89 - 97, The code currently checks directoryExists(homeDirectory) and throws createManagedSkillAgentNotInstalledError before honoring the alwaysProvision behavior; instead, call verifyWritableDirectory(homeDirectory) (or the existing helper that creates directories) for the resolved path returned by resolveManagedSkillAgentHomeDirectory(agentName) and let it create the directory or fail, catching errors and only throwing createManagedSkillAgentNotInstalledError if verifyWritableDirectory fails for reasons other than a needed provision; remove the pre-check with directoryExists and rely on EAFP (attempt verifyWritableDirectory and handle errors) so alwaysProvision paths can create the home on first run.src/application/commands/skills/embedded-assets.test.ts (1)
1013-1025:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCover all non-
allowed-toolshosts in this frontmatter assertion.This loop skips
codebuddyandopenclaw, so a renderer regression that starts emittingallowed-toolsfor either host would pass. Derive the list fromavailableBundledSkillAgentNamesand exclude only the hosts that are expected to contain that field.As per coding guidelines, "Any modification must include sufficient tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/embedded-assets.test.ts` around lines 1013 - 1025, The test currently iterates a hardcoded agent list and misses some hosts; modify the loop to iterate over availableBundledSkillAgentNames (instead of the hardcoded ["universal", ...]) and filter out only the known agents that are expected to include "allowed-tools" (e.g., keep an explicit exclude set), then for each skillName from availableBundledSkillNames use getBundledSkillFiles(skillName, agentName) and readBundledSkillFileContent(skillFile) as before and assert that the content does not contain "allowed-tools" for all remaining agents; ensure the test covers all agents by deriving the agent list from availableBundledSkillAgentNames and only excluding the specific hosts that should allow that field.
🧹 Nitpick comments (9)
src/application/commands/skills/list.cli.test.ts (1)
73-73: ⚡ Quick winPrefer a line-based assertion here instead of a regex.
This check only needs to prove the
Universal … controlledrow is rendered, and the regex makes the test more brittle than necessary. A simpleincludes/line-scan assertion would be easier to maintain. As per coding guidelines, "Avoid using regular expressions when possible".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/list.cli.test.ts` at line 73, Replace the brittle regex assertion on result.stdout with a line-based check: scan result.stdout lines and assert that at least one line includes the distinct tokens "Universal", "installed", and "controlled" (e.g. using split('\n') and .some). Update the assertion that currently uses toMatch(/Universal\s+installed\s+\S*controlled/) to this token-in-line style using result.stdout so the test only verifies the rendered row without relying on a regex.src/application/commands/skills/init.test.ts (1)
18-41: ⚡ Quick winAdd one happy-path that does not pre-create the universal home.
These updated tests still
mkdir()the universal agent home before invokingskills init, so they won't catch a regression in the new "universal is always provisioned" contract that this PR introduces. At least one--agent universalsuccess case should rely on the command/bootstrap to create~/.agentsitself. As per coding guidelines, "Any modification must include sufficient tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/init.test.ts` around lines 18 - 41, Add a new happy-path test that verifies "skills init --agent universal" succeeds when the universal agent home does NOT exist beforehand: copy the existing test structure in init.test.ts (using createCliSandbox(), resolveManagedSkillAgentHomeDirectory(..., "universal"), resolveManagedSkillDirectoryPath(..., "campaign-writer"), and sandbox.run([... "skills","init", ...])) but do NOT call mkdir(universalHomeDirectory) before invoking sandbox.run; after the command, assert the process succeeded and that universalHomeDirectory and skillDirectoryPath now exist (and contain the expected files) to ensure the command/bootstrap provisions the universal agent home.src/application/commands/skills/install-output.test.ts (1)
47-75: ⚡ Quick winBuild the fixture paths with
join().These hardcoded POSIX paths make the test data platform-specific. Use
node:pathhelpers here too.As per coding guidelines, "Never assume POSIX path separators in code, tests, snapshots, or assertions. Use `node:path` helpers such as `join()`, `resolve()`, and `relative()` for path construction."♻️ Proposed fix
+import { join } from "node:path"; import { describe, expect, test } from "bun:test"; @@ { agentName: "universal", - path: "/tmp/universal/skills/chatgpt", + path: join("tmp", "universal", "skills", "chatgpt"), }, { agentName: "claude", - path: "/tmp/claude/skills/chatgpt", + path: join("tmp", "claude", "skills", "chatgpt"), }, @@ { agentName: "universal", - path: "/tmp/universal/skills/vision", + path: join("tmp", "universal", "skills", "vision"), }, { agentName: "claude", - path: "/tmp/claude/skills/vision", + path: join("tmp", "claude", "skills", "vision"), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/install-output.test.ts` around lines 47 - 75, The test fixture createMultiSkillMultiAgentSummaries uses hardcoded POSIX paths for the publication.path fields; replace those literal strings with platform-safe constructions using node:path.join (and node:os.tmpdir() if desired) so paths are built via join(...) instead of "/tmp/…". Import the helpers from 'node:path' (and 'node:os' if using the system temp dir) and update the path values in the createMultiSkillMultiAgentSummaries return objects (publication.path entries for agents "universal" and "claude") to use path.join with the appropriate path segments.src/application/commands/uninstall.cli.test.ts (1)
59-60: ⚡ Quick winUse the managed-skill path helpers here instead of re-encoding
~/.agents/skills.This setup duplicates the host layout inline, which is exactly what this PR had to update. Building the path with
resolveManagedSkillAgentHomeDirectory()andresolveManagedSkillDirectoryPath()will keep the test aligned with production behavior if the managed host layout changes again.Suggested fix
- const universalHome = join(options.sandbox.env.HOME!, ".agents"); - const skillDirectory = join(universalHome, "skills", options.skillName); + const universalHome = resolveManagedSkillAgentHomeDirectory( + options.sandbox.env, + "universal", + ); + const skillDirectory = resolveManagedSkillDirectoryPath( + universalHome, + options.skillName, + );You'll also need to import the two path helpers in this file. As per coding guidelines, "When extracting a shared utility from production code, also replace any test helpers or inline expressions that duplicate the same logic."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/uninstall.cli.test.ts` around lines 59 - 60, Replace the inline construction of the managed-skill path in the test by calling the production helpers: use resolveManagedSkillAgentHomeDirectory(options.sandbox.env.HOME) to get the universalHome and resolveManagedSkillDirectoryPath(universalHome, options.skillName) to get skillDirectory so the test mirrors production layout; import both resolveManagedSkillAgentHomeDirectory and resolveManagedSkillDirectoryPath at the top of the test file and remove the join(...) expressions that hard-code "~/.agents/skills".src/application/commands/skills/update.test.ts (1)
36-37: ⚡ Quick winExtract a local helper for installed skill paths instead of repeating the host-layout join.
This file repeats the same
resolveManagedSkillAgentHomeDirectory(...)+join(..., "skills", skillName)setup in many places, and this migration had to edit all of them. A single helper built onresolveManagedSkillDirectoryPath()would make the next host-layout change a one-line update.Suggested helper
function resolveInstalledSkillDirectoryPath( sandbox: Awaited<ReturnType<typeof createCliSandbox>>, agentId: "universal" | "claude" | "hermes", skillName: string, ): string { return resolveManagedSkillDirectoryPath( resolveManagedSkillAgentHomeDirectory(sandbox.env, agentId), skillName, ); }Then replace the repeated
join(homeDirectory, "skills", "...")calls with this helper.As per coding guidelines, "In test files, extract repeated setup (mock, stub, or setup objects) into a local factory function at the bottom of the file."
Also applies to: 117-118, 163-164, 191-192, 309-310, 398-399, 488-489, 619-620, 705-706, 810-811
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/update.test.ts` around lines 36 - 37, Extract a local helper named resolveInstalledSkillDirectoryPath that takes the sandbox (Awaited<ReturnType<typeof createCliSandbox>>), an agentId ("universal" | "claude" | "hermes"), and skillName and returns resolveManagedSkillDirectoryPath(resolveManagedSkillAgentHomeDirectory(sandbox.env, agentId), skillName); then replace the repeated patterns that compute universalHomeDirectory + join(..., "skills", skillName) (and similar for other agents) with calls to resolveInstalledSkillDirectoryPath across this test file (references include the existing resolveManagedSkillAgentHomeDirectory and resolveManagedSkillDirectoryPath uses).src/application/commands/skills/index.cli.test.ts (1)
63-70: ⚡ Quick winAdd a clean-machine startup bootstrap case.
Line 70 pre-creates
universalHomeDirectory, so this test no longer exercises the new~/.agentsprovisioning path. The regression-prone case here is starting the CLI with no managed home on disk and asserting startup bootstraps bundled skills into Universal.As per coding guidelines, "Any modification must include sufficient tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/index.cli.test.ts` around lines 63 - 70, The test "auto-installs bundled skills during cli startup" currently pre-creates universalHomeDirectory (created via resolveManagedSkillAgentHomeDirectory in the test using mkdir), which prevents exercising the clean-machine bootstrap path; remove the pre-creation (the mkdir/universalHomeDirectory setup) or add a separate test that ensures the managed home does not exist before invoking createCliSandbox/startup, then run the CLI startup and assert that bundled skills (skillDirectoryPath and findSkillsDirectoryPath) are created; update the test around createCliSandbox, resolveManagedSkillAgentHomeDirectory, and the assertions so the case where no managed home exists on disk is covered.src/application/commands/skills/repair.cli.test.ts (1)
41-58: ⚡ Quick winCover
repair --agent universalwithout an existing home directory.All successful Universal repair cases still create the home first, so the suite does not prove the always-provisioned Universal host works on a clean machine. Please add one case where the canonical source exists,
~/.agentsis absent, andskills repair --skill oo --agent universalrecreates the host target.As per coding guidelines, "Any modification must include sufficient tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/repair.cli.test.ts` around lines 41 - 58, Add a new test (e.g., "repair --skill oo --agent universal creates home when absent") that does not pre-create the universal home directory: use createCliSandbox() to get sandbox, compute universalHomeDirectory with resolveManagedSkillAgentHomeDirectory(sandbox.env, "universal") and ensure it does not exist (do NOT call mkdir on it). Prepare the canonical/bundled source for the "oo" skill (same approach as other tests: create or ensure the canonical skill bundle that repair uses is present), then call sandbox.run(["skills","repair","--skill","oo","--agent","universal"], { version: TEST_CLI_VERSION }) and assert that universalHomeDirectory and the managed skill directory (resolveManagedSkillDirectoryPath(universalHomeDirectory, "oo")) are created and contain the expected files (e.g., SKILL.md matches the canonical source). Ensure the test uses the same sandbox.run and resolveManagedSkillDirectoryPath identifiers as the existing tests.src/application/commands/skills/managed-skill-hosts.test.ts (1)
10-23: ⚡ Quick winExtract the repeated temp-home setup into a local fixture helper.
Both tests rebuild the same
rootDirectory/env/cleanup flow inline. Move that into a small helper at the bottom of the file so future host-case additions stay consistent.As per coding guidelines, "In test files, extract repeated setup (mock, stub, or setup objects) into a local factory function at the bottom of the file."
Also applies to: 25-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/managed-skill-hosts.test.ts` around lines 10 - 23, Extract the repeated temporary-home setup used by tests (the createTemporaryDirectory call, env = { HOME, USERPROFILE }, and the rm cleanup) into a small local fixture factory function at the bottom of the file (e.g., makeTempHomeFixture) that returns { rootDirectory, env, cleanup } and call that fixture from both tests that use resolveAvailableManagedSkillHosts; replace the inline try/finally with awaiting the fixture and calling its cleanup, and keep references to createTemporaryDirectory, resolveAvailableManagedSkillHosts, and rm intact so the tests use the new helper consistently.src/application/commands/skills/check.test.ts (1)
278-279: ⚡ Quick winReuse the managed skills path helper here.
join(openClawHomeDirectory, "skills")duplicates the production path logic that this suite already imports elsewhere. UsingresolveManagedSkillsDirectoryPath(openClawHomeDirectory)keeps the test aligned with future path-layout changes.As per coding guidelines, "When extracting a shared utility from production code, also replace any test helpers or inline expressions that duplicate the same logic. An extraction is incomplete if test files still contain local functions or raw inline code doing the same thing."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/application/commands/skills/check.test.ts` around lines 278 - 279, The test duplicates production path logic by building publishRootPath with join(openClawHomeDirectory, "skills"); replace that inline join with the shared helper resolveManagedSkillsDirectoryPath(openClawHomeDirectory) so publishRootPath is derived from resolveManagedSkillsDirectoryPath instead, keeping references to resolveManagedSkillAgentHomeDirectory and openClawHomeDirectory intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contrib/skills/shared/oo-create-skill/SKILL.md`:
- Line 3: The shared SKILL.md description hardcodes "Claude/agent skill", which
is misleading for non-Claude hosts; update the description in SKILL.md to use
neutral wording (e.g., "agent skill") or replace the hardcoded name with an
agentic-markdown variable (e.g., an agent-specific placeholder) so the rendered
text is host-agnostic — locate the string "Claude/agent skill" in SKILL.md and
replace it with the neutral term or variable.
In `@docs/path-first-skill-publish-plan.md`:
- Around line 10-12: The sample publish flow hardcodes the agent flag ("--agent
universal") which breaks for agent-native local skills; update the example so
the primary command omits the --agent flag (i.e., show "oo skills locate
<skill-id>" and "oo skills publish <path>") or explicitly instruct users to
supply the skill's owning agent when needed, and add a short note clarifying
that local skills remain agent-native and therefore may require an explicit
--agent if not using the default.
In `@src/application/commands/info.cli.test.ts`:
- Around line 79-85: The test is incorrectly creating the "universal"
managed-skill home which masks a regression where the info command might require
an existing directory; remove the mkdir call that uses
resolveManagedSkillAgentHomeDirectory(...) and
resolveManagedSkillsDirectoryPath(...), and instead assert that the "universal"
agent is reported as "available" when no directory exists (or add a separate
test case named e.g. "info: reports universal available when no home exists"
that verifies info returns available without creating the directory). Ensure the
test references the same helper functions/values
(resolveManagedSkillAgentHomeDirectory, resolveManagedSkillsDirectoryPath,
"universal") so it targets the no-home behavior.
In `@src/application/commands/skills/install.cli.test.ts`:
- Around line 54-58: The assertion depends on ordering by checking targets[0];
instead locate the entry with agentId "universal" and assert against that object
instead. In the test (variable targets) use Array.prototype.find to get the
element where agentId === "universal", assert that it exists, and then assert
its properties (e.g., status: "installed") with toMatchObject or equivalent so
the test is order-independent.
In `@src/application/commands/skills/legacy-codex-cleanup.test.ts`:
- Around line 55-60: Replace the weak assertions that only check for an
isDirectory function on the Stats object with assertions that call isDirectory()
and expect true: for each occurrence where you do await
expect(stat(localSkillPath)).resolves.toMatchObject({ isDirectory:
expect.any(Function) }) (and the similar checks for unmanagedSkillPath and the
other two occurrences around lines 103-105 and 141-143), instead await the stat
call (e.g., const s = await stat(localSkillPath)) and assert
expect(s.isDirectory()).toBe(true); do the same for unmanagedSkillPath and the
other two stat checks so the tests verify the path is actually a directory.
In `@src/application/commands/skills/legacy-codex-cleanup.ts`:
- Around line 44-48: The current Promise.all in removeLegacyCodexManagedSkills
causes a fail-fast return when removeLegacyCodexHomeManagedSkills rejects,
leaving removeLegacyCodexCanonicalBundledStorage running; change the
implementation to await both branches to settle (use Promise.allSettled or await
each branch with try/catch and capture errors) and then rethrow or return an
aggregated error after both have completed; update references in
removeLegacyCodexManagedSkills to ensure it only resolves after both
removeLegacyCodexHomeManagedSkills and removeLegacyCodexCanonicalBundledStorage
have settled, and add a test that mocks removeLegacyCodexHomeManagedSkills to
reject while verifying removeLegacyCodexCanonicalBundledStorage has completed
before removeLegacyCodexManagedSkills resolves/rejects (so run-cli.ts’s
synchronization race is prevented).
---
Outside diff comments:
In `@src/application/commands/skills/check.ts`:
- Around line 89-97: The code currently checks directoryExists(homeDirectory)
and throws createManagedSkillAgentNotInstalledError before honoring the
alwaysProvision behavior; instead, call verifyWritableDirectory(homeDirectory)
(or the existing helper that creates directories) for the resolved path returned
by resolveManagedSkillAgentHomeDirectory(agentName) and let it create the
directory or fail, catching errors and only throwing
createManagedSkillAgentNotInstalledError if verifyWritableDirectory fails for
reasons other than a needed provision; remove the pre-check with directoryExists
and rely on EAFP (attempt verifyWritableDirectory and handle errors) so
alwaysProvision paths can create the home on first run.
In `@src/application/commands/skills/embedded-assets.test.ts`:
- Around line 1013-1025: The test currently iterates a hardcoded agent list and
misses some hosts; modify the loop to iterate over
availableBundledSkillAgentNames (instead of the hardcoded ["universal", ...])
and filter out only the known agents that are expected to include
"allowed-tools" (e.g., keep an explicit exclude set), then for each skillName
from availableBundledSkillNames use getBundledSkillFiles(skillName, agentName)
and readBundledSkillFileContent(skillFile) as before and assert that the content
does not contain "allowed-tools" for all remaining agents; ensure the test
covers all agents by deriving the agent list from
availableBundledSkillAgentNames and only excluding the specific hosts that
should allow that field.
---
Nitpick comments:
In `@src/application/commands/skills/check.test.ts`:
- Around line 278-279: The test duplicates production path logic by building
publishRootPath with join(openClawHomeDirectory, "skills"); replace that inline
join with the shared helper
resolveManagedSkillsDirectoryPath(openClawHomeDirectory) so publishRootPath is
derived from resolveManagedSkillsDirectoryPath instead, keeping references to
resolveManagedSkillAgentHomeDirectory and openClawHomeDirectory intact.
In `@src/application/commands/skills/index.cli.test.ts`:
- Around line 63-70: The test "auto-installs bundled skills during cli startup"
currently pre-creates universalHomeDirectory (created via
resolveManagedSkillAgentHomeDirectory in the test using mkdir), which prevents
exercising the clean-machine bootstrap path; remove the pre-creation (the
mkdir/universalHomeDirectory setup) or add a separate test that ensures the
managed home does not exist before invoking createCliSandbox/startup, then run
the CLI startup and assert that bundled skills (skillDirectoryPath and
findSkillsDirectoryPath) are created; update the test around createCliSandbox,
resolveManagedSkillAgentHomeDirectory, and the assertions so the case where no
managed home exists on disk is covered.
In `@src/application/commands/skills/init.test.ts`:
- Around line 18-41: Add a new happy-path test that verifies "skills init
--agent universal" succeeds when the universal agent home does NOT exist
beforehand: copy the existing test structure in init.test.ts (using
createCliSandbox(), resolveManagedSkillAgentHomeDirectory(..., "universal"),
resolveManagedSkillDirectoryPath(..., "campaign-writer"), and sandbox.run([...
"skills","init", ...])) but do NOT call mkdir(universalHomeDirectory) before
invoking sandbox.run; after the command, assert the process succeeded and that
universalHomeDirectory and skillDirectoryPath now exist (and contain the
expected files) to ensure the command/bootstrap provisions the universal agent
home.
In `@src/application/commands/skills/install-output.test.ts`:
- Around line 47-75: The test fixture createMultiSkillMultiAgentSummaries uses
hardcoded POSIX paths for the publication.path fields; replace those literal
strings with platform-safe constructions using node:path.join (and
node:os.tmpdir() if desired) so paths are built via join(...) instead of
"/tmp/…". Import the helpers from 'node:path' (and 'node:os' if using the system
temp dir) and update the path values in the createMultiSkillMultiAgentSummaries
return objects (publication.path entries for agents "universal" and "claude") to
use path.join with the appropriate path segments.
In `@src/application/commands/skills/list.cli.test.ts`:
- Line 73: Replace the brittle regex assertion on result.stdout with a
line-based check: scan result.stdout lines and assert that at least one line
includes the distinct tokens "Universal", "installed", and "controlled" (e.g.
using split('\n') and .some). Update the assertion that currently uses
toMatch(/Universal\s+installed\s+\S*controlled/) to this token-in-line style
using result.stdout so the test only verifies the rendered row without relying
on a regex.
In `@src/application/commands/skills/managed-skill-hosts.test.ts`:
- Around line 10-23: Extract the repeated temporary-home setup used by tests
(the createTemporaryDirectory call, env = { HOME, USERPROFILE }, and the rm
cleanup) into a small local fixture factory function at the bottom of the file
(e.g., makeTempHomeFixture) that returns { rootDirectory, env, cleanup } and
call that fixture from both tests that use resolveAvailableManagedSkillHosts;
replace the inline try/finally with awaiting the fixture and calling its
cleanup, and keep references to createTemporaryDirectory,
resolveAvailableManagedSkillHosts, and rm intact so the tests use the new helper
consistently.
In `@src/application/commands/skills/repair.cli.test.ts`:
- Around line 41-58: Add a new test (e.g., "repair --skill oo --agent universal
creates home when absent") that does not pre-create the universal home
directory: use createCliSandbox() to get sandbox, compute universalHomeDirectory
with resolveManagedSkillAgentHomeDirectory(sandbox.env, "universal") and ensure
it does not exist (do NOT call mkdir on it). Prepare the canonical/bundled
source for the "oo" skill (same approach as other tests: create or ensure the
canonical skill bundle that repair uses is present), then call
sandbox.run(["skills","repair","--skill","oo","--agent","universal"], { version:
TEST_CLI_VERSION }) and assert that universalHomeDirectory and the managed skill
directory (resolveManagedSkillDirectoryPath(universalHomeDirectory, "oo")) are
created and contain the expected files (e.g., SKILL.md matches the canonical
source). Ensure the test uses the same sandbox.run and
resolveManagedSkillDirectoryPath identifiers as the existing tests.
In `@src/application/commands/skills/update.test.ts`:
- Around line 36-37: Extract a local helper named
resolveInstalledSkillDirectoryPath that takes the sandbox
(Awaited<ReturnType<typeof createCliSandbox>>), an agentId ("universal" |
"claude" | "hermes"), and skillName and returns
resolveManagedSkillDirectoryPath(resolveManagedSkillAgentHomeDirectory(sandbox.env,
agentId), skillName); then replace the repeated patterns that compute
universalHomeDirectory + join(..., "skills", skillName) (and similar for other
agents) with calls to resolveInstalledSkillDirectoryPath across this test file
(references include the existing resolveManagedSkillAgentHomeDirectory and
resolveManagedSkillDirectoryPath uses).
In `@src/application/commands/uninstall.cli.test.ts`:
- Around line 59-60: Replace the inline construction of the managed-skill path
in the test by calling the production helpers: use
resolveManagedSkillAgentHomeDirectory(options.sandbox.env.HOME) to get the
universalHome and resolveManagedSkillDirectoryPath(universalHome,
options.skillName) to get skillDirectory so the test mirrors production layout;
import both resolveManagedSkillAgentHomeDirectory and
resolveManagedSkillDirectoryPath at the top of the test file and remove the
join(...) expressions that hard-code "~/.agents/skills".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8ec0a77-93b1-4a0d-890c-89b8f8e55f0a
⛔ Files ignored due to path filters (2)
src/application/commands/__snapshots__/info.cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/skills/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (53)
contrib/ci/npm-packages.test.tscontrib/skills/shared/oo-create-skill/SKILL.mdcontrib/skills/shared/oo-create-skill/agents/openai.yamlcontrib/skills/shared/oo-find-skills/SKILL.mdcontrib/skills/shared/oo-find-skills/agents/openai.yamlcontrib/skills/shared/oo-publish-skill/agents/openai.yamlcontrib/skills/shared/oo/SKILL.mdcontrib/skills/shared/oo/agents/openai.yamldocs/commands.mddocs/commands.zh-CN.mddocs/path-first-skill-publish-plan.mdsrc/application/bootstrap/run-cli.test.tssrc/application/bootstrap/run-cli.tssrc/application/commands/info.cli.test.tssrc/application/commands/skills/__tests__/helpers.tssrc/application/commands/skills/bundled-skill-observation.test.tssrc/application/commands/skills/bundled-skill-observation.tssrc/application/commands/skills/bundled-skill-paths.tssrc/application/commands/skills/check-update.cli.test.tssrc/application/commands/skills/check.test.tssrc/application/commands/skills/check.tssrc/application/commands/skills/embedded-assets.test.tssrc/application/commands/skills/embedded-assets.tssrc/application/commands/skills/index.cli.test.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/info-inventory.test.tssrc/application/commands/skills/init.test.tssrc/application/commands/skills/install-output.test.tssrc/application/commands/skills/install.cli.test.tssrc/application/commands/skills/legacy-canonical-migration.test.tssrc/application/commands/skills/legacy-canonical-migration.tssrc/application/commands/skills/legacy-codex-cleanup.test.tssrc/application/commands/skills/legacy-codex-cleanup.tssrc/application/commands/skills/list.cli.test.tssrc/application/commands/skills/locate.test.tssrc/application/commands/skills/managed-skill-agents.test.tssrc/application/commands/skills/managed-skill-agents.tssrc/application/commands/skills/managed-skill-hosts.test.tssrc/application/commands/skills/managed-skill-hosts.tssrc/application/commands/skills/managed-skill-paths.test.tssrc/application/commands/skills/managed-skill-paths.tssrc/application/commands/skills/publish.test.tssrc/application/commands/skills/repair.cli.test.tssrc/application/commands/skills/share.test.tssrc/application/commands/skills/shared.test.tssrc/application/commands/skills/sync.cli.test.tssrc/application/commands/skills/uninstall.cli.test.tssrc/application/commands/skills/update.cli.test.tssrc/application/commands/skills/update.test.tssrc/application/commands/uninstall.cli.test.tssrc/application/self-update/uninstall.test.tssrc/application/self-update/uninstall.tssrc/i18n/catalog.ts
💤 Files with no reviewable changes (8)
- src/application/commands/skills/bundled-skill-observation.test.ts
- contrib/skills/shared/oo-create-skill/agents/openai.yaml
- contrib/skills/shared/oo/agents/openai.yaml
- contrib/skills/shared/oo-publish-skill/agents/openai.yaml
- src/application/commands/skills/bundled-skill-observation.ts
- contrib/skills/shared/oo/SKILL.md
- contrib/skills/shared/oo-find-skills/agents/openai.yaml
- src/i18n/catalog.ts
The cleanup used Promise.all, which short-circuits on the first rejection and could leave the second branch unfinished. Switch to Promise.allSettled so both the home and canonical bundled cleanups always run, logging each failure independently as best-effort. Also modernize the related stat assertions to call isDirectory() directly and align the create-skill SKILL.md and publish docs with the agent-neutral wording. Signed-off-by: Kevin Cui <bh@bugs.cc>
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
1 participant
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.
Codex now reads skills from the universal
~/.agentslocation, so a dedicated Codex host, agent identifier, and per-skillopenai.yamlmanifests are no longer needed. Dropping them removes--agent codexand thecodexhost from list, locate, install, and repair output.The universal
~/.agentshost is now always provisioned, guaranteeing at least one install target even when no other supported agent home exists.Add a one-time cleanup that deletes oo-managed skills left under the legacy
~/.codexhome and bundled storage, identified strictly by their.oo-metadata.json. Codex does not deduplicate~/.codex/skillsagainst~/.agents, so the stale copies would otherwise surface as duplicates; user-authored and unmanaged directories are preserved.