feat(codex): support workflow skills#1534
Open
borshyo wants to merge 2 commits intocoleam00:devfrom
Open
Conversation
Fixes applied: - Align workflow skill validation with provider-specific resolution - Fail skill resolution on unreadable high-precedence candidates - Update workflow docs for Codex/Pi skills support Tests added: - Provider resolver unreadable/provider-root coverage - Workflow validator provider-specific skill coverage - CLI validate Codex skillRoots integration coverage Skipped (see review artifacts): - GitHub PR comment: no PR exists for this branch Review artifacts: /Users/kirill/.archon/workspaces/borshyo/Archon/artifacts/runs/9e81ffba134e9eb4d500dbb810a4f22d/review/
📝 WalkthroughWalkthroughThis PR extends skill support from Claude-only to cross-provider functionality. It introduces a new skill resolution module, enables Codex skills capability, adds ChangesCross-Provider Skills Support
Sequence DiagramsequenceDiagram
participant User as User/<br/>CLI
participant Config as Config<br/>Loader
participant Validator as Workflow<br/>Validator
participant Provider as Codex<br/>Provider
participant Skills as Skills<br/>Resolver
User->>Config: Load merged config<br/>(repo + global)
Config-->>User: Codex skillRoots,<br/>assistants config
User->>Validator: validateWorkflowsCommand<br/>(skillRootsByProvider)
Validator->>Validator: For each workflow node
alt Node has skills
Validator->>Skills: resolveProviderSkillReferences<br/>(provider, skillRefs,<br/>skillRootsByProvider)
Skills-->>Validator: resolved skills ∪<br/>missing skills
alt Missing skills exist
Validator->>Validator: Report errors
else All skills resolved
Validator->>Validator: Continue validation ✓
end
end
Note over User,Validator: Validation complete
User->>Provider: sendQuery(node.skills)
Provider->>Skills: resolveSkillReferences<br/>(skillRefs, skillRoots)
Skills-->>Provider: resolved + loaded skills
Provider->>Provider: Build skill context<br/>from frontmatter
Provider->>Provider: Prepend skills to prompt
Provider-->>User: Stream response<br/>(with skill context)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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: 466702dfc7
ℹ️ 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".
| @@ -86,6 +104,7 @@ export async function validateWorkflowsCommand( | |||
| ): Promise<number> { | |||
| const config = await buildValidationConfig(cwd); | |||
| const mergedConfig = await loadConfig(cwd); | |||
There was a problem hiding this comment.
validateWorkflowsCommand now awaits loadConfig(cwd) without a fallback, so a malformed or unreadable global/repo config causes the command to throw before discovery/validation runs. This regresses prior behavior where config-load failures were tolerated (via discoverWorkflowsWithConfig’s internal try/catch) and users could still validate workflows with defaults.
Useful? React with 👍 / 👎.
| function expandRoot(root: string, cwd: string): string { | ||
| const home = getHomeDir(); | ||
| if (root === '~') return home; | ||
| if (root.startsWith(`~${sep}`)) return join(home, root.slice(2)); |
There was a problem hiding this comment.
The tilde expansion only matches ~${sep}. On Windows, sep is \, so config values written as ~/.codex/skills (common in docs and cross-platform configs) are treated as relative paths instead of home-relative roots. That makes skill resolution/validation miss configured roots on Windows hosts unless users rewrite paths with backslashes.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/workflows/src/validator.ts (1)
414-422:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not skip the rest of node validation when skills are unsupported.
Line 421 uses
continue, which skips subsequent checks (hooks/agents/tool restrictions/script validation) for the same node. Only skills resolution should be skipped.Suggested fix
if (provider && isRegisteredProvider(provider)) { const caps = getProviderCapabilities(provider); if (!caps.skills) { issues.push({ level: 'warning', nodeId: node.id, field: 'skills', message: `Skills are not supported by provider '${provider}' — this will be ignored`, hint: 'Remove the skills field or switch to a provider that supports skills', }); - continue; + } else { + const skillRoots = provider ? config?.skillRootsByProvider?.[provider] : undefined; + const skillResolution = await resolveProviderSkillReferences(provider, cwd, node.skills, { + skillRoots, + }); + for (const missingSkill of skillResolution.missing) { + const reason = missingSkill.reason ? `${missingSkill.reason}\n` : ''; + issues.push({ + level: 'error', + nodeId: node.id, + field: 'skills', + message: `Skill '${missingSkill.ref}' not found or not readable`, + hint: `${reason}Searched:\n${missingSkill.searchedPaths.map(path => ` - ${path}`).join('\n')}`, + }); + } } - } - - const skillRoots = provider ? config?.skillRootsByProvider?.[provider] : undefined; - const skillResolution = await resolveProviderSkillReferences(provider, cwd, node.skills, { - skillRoots, - }); - for (const missingSkill of skillResolution.missing) { - const reason = missingSkill.reason ? `${missingSkill.reason}\n` : ''; - issues.push({ - level: 'error', - nodeId: node.id, - field: 'skills', - message: `Skill '${missingSkill.ref}' not found or not readable`, - hint: `${reason}Searched:\n${missingSkill.searchedPaths.map(path => ` - ${path}`).join('\n')}`, - }); - } + } else { + const skillRoots = provider ? config?.skillRootsByProvider?.[provider] : undefined; + const skillResolution = await resolveProviderSkillReferences(provider, cwd, node.skills, { + skillRoots, + }); + for (const missingSkill of skillResolution.missing) { + const reason = missingSkill.reason ? `${missingSkill.reason}\n` : ''; + issues.push({ + level: 'error', + nodeId: node.id, + field: 'skills', + message: `Skill '${missingSkill.ref}' not found or not readable`, + hint: `${reason}Searched:\n${missingSkill.searchedPaths.map(path => ` - ${path}`).join('\n')}`, + }); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/validator.ts` around lines 414 - 422, The code currently pushes a warning about unsupported skills and then does a bare `continue`, which skips all further validation for that node (hooks/agents/tool/script checks); remove the `continue` and instead only bypass the skills-resolution step: keep the warning push for provider/skills (referencing provider and node.id) but ensure subsequent validation logic (hooks, agents, tool restrictions, script validation) still runs for the same node; if there is a separate skills resolution call, gate only that call on provider support rather than skipping the entire node validation.
🧹 Nitpick comments (1)
packages/cli/package.json (1)
11-11: Isolatevalidate.test.tsfrom the other CLI suites.This test calls
clearConfigCache(), which mutates module-level state, alongside mutations toprocess.env.ARCHON_HOMEandDEFAULT_AI_ASSISTANT. Running it together with other test files in the samebun testinvocation risks cross-file interference if the cache state is not fully isolated between files.🔎 Suggested fix
- "test": "bun test src/commands/version.test.ts src/commands/setup.test.ts src/commands/skill.test.ts src/commands/validate.test.ts && bun test src/commands/workflow.test.ts && bun test src/commands/isolation.test.ts && bun test src/commands/chat.test.ts && bun test src/commands/serve.test.ts", + "test": "bun test src/commands/version.test.ts src/commands/setup.test.ts src/commands/skill.test.ts && bun test src/commands/validate.test.ts && bun test src/commands/workflow.test.ts && bun test src/commands/isolation.test.ts && bun test src/commands/chat.test.ts && bun test src/commands/serve.test.ts",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/package.json` at line 11, The test runner groups validate.test.ts with other CLI tests and that file calls clearConfigCache() and mutates process.env (e.g., DEFAULT_AI_ASSISTANT, ARCHON_HOME), so change the "test" script in package.json to run validate.test.ts in a separate bun test invocation: remove src/commands/validate.test.ts from the grouped bun test call and add an independent && bun test src/commands/validate.test.ts (so clearConfigCache and env mutations run in their own process). This targets the test script and the specific file name validate.test.ts and the implicated function clearConfigCache() and environment variables to avoid cross-file state leakage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/docs-web/src/content/docs/guides/skills.md`:
- Around line 128-131: Replace the anonymous fenced code block containing the
directory tree snippet (the triple-backticks block showing
".agents/skills/my-skill/ └── SKILL.md") with a language-labeled fence by adding
"text" (or "plaintext") after the opening backticks so the fence becomes
```text, which will satisfy MD040 in CI.
In `@packages/providers/src/codex/config.ts`:
- Around line 41-43: When building result.skillRoots from raw.skillRoots (the
Array.isArray branch for raw.skillRoots), trim each string and filter out
entries that are not strings or that are empty after trimming so whitespace-only
values are dropped; update the predicate used when populating result.skillRoots
(referencing raw.skillRoots and result.skillRoots) to map/trim values and only
keep non-empty trimmed strings before assigning to result.skillRoots.
In `@packages/providers/src/codex/provider.ts`:
- Around line 96-100: The prompt builder buildCodexSkillContext currently
injects skill.skillPath into model input and elsewhere full paths are logged;
remove any use of skill.skillPath in prompts and replace with non-sensitive
identifiers (e.g., skill.name and an index or truncated/sanitized ID) so only
name/count are sent to the model; update corresponding info logs that currently
print skill.skillPath to instead log either path.basename(skill.skillPath), a
relative/sanitized identifier, or redact the path entirely; also scan and update
other locations that log or send skill.skillPath (notably the other usages
referenced around the same module) to use the same sanitized identifier
approach.
In `@packages/workflows/src/schemas/dag-node.ts`:
- Around line 147-150: The schema now treats the field skills as a validated
first-class property but the constant LOOP_NODE_AI_FIELDS still lists "skills"
as an unsupported/ignored field, causing loop nodes to drop or warn about
skills; remove "skills" from the LOOP_NODE_AI_FIELDS set/array (or update the
logic that builds that list) so loop-node handling no longer treats skills as
unsupported, and run/update any tests that assert supported fields for the loop
node (references: skills and LOOP_NODE_AI_FIELDS).
---
Outside diff comments:
In `@packages/workflows/src/validator.ts`:
- Around line 414-422: The code currently pushes a warning about unsupported
skills and then does a bare `continue`, which skips all further validation for
that node (hooks/agents/tool/script checks); remove the `continue` and instead
only bypass the skills-resolution step: keep the warning push for
provider/skills (referencing provider and node.id) but ensure subsequent
validation logic (hooks, agents, tool restrictions, script validation) still
runs for the same node; if there is a separate skills resolution call, gate only
that call on provider support rather than skipping the entire node validation.
---
Nitpick comments:
In `@packages/cli/package.json`:
- Line 11: The test runner groups validate.test.ts with other CLI tests and that
file calls clearConfigCache() and mutates process.env (e.g.,
DEFAULT_AI_ASSISTANT, ARCHON_HOME), so change the "test" script in package.json
to run validate.test.ts in a separate bun test invocation: remove
src/commands/validate.test.ts from the grouped bun test call and add an
independent && bun test src/commands/validate.test.ts (so clearConfigCache and
env mutations run in their own process). This targets the test script and the
specific file name validate.test.ts and the implicated function
clearConfigCache() and environment variables to avoid cross-file state leakage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e20e1991-068b-4637-a84f-bd883e986d63
📒 Files selected for processing (26)
packages/cli/package.jsonpackages/cli/src/commands/validate.test.tspackages/cli/src/commands/validate.tspackages/core/src/config/config-loader.test.tspackages/core/src/config/config-loader.tspackages/docs-web/src/content/docs/book/quick-reference.mdpackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/guides/index.mdpackages/docs-web/src/content/docs/guides/skills.mdpackages/providers/package.jsonpackages/providers/src/codex/capabilities.tspackages/providers/src/codex/config.tspackages/providers/src/codex/provider.test.tspackages/providers/src/codex/provider.tspackages/providers/src/community/pi/options-translator.tspackages/providers/src/index.tspackages/providers/src/registry.test.tspackages/providers/src/skills.test.tspackages/providers/src/skills.tspackages/providers/src/types.tspackages/workflows/src/dag-executor.test.tspackages/workflows/src/deps.tspackages/workflows/src/loader.test.tspackages/workflows/src/schemas/dag-node.tspackages/workflows/src/validator.test.tspackages/workflows/src/validator.ts
Comment on lines
128
to
131
| ``` | ||
| .claude/skills/my-skill/ | ||
| .agents/skills/my-skill/ | ||
| └── SKILL.md | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the directory-tree fence.
The anonymous fence at Line 128 will keep tripping MD040 in docs CI. Mark it as text/plaintext so the lint warning goes away.
♻️ Proposed fix
-```
+```text
.agents/skills/my-skill/
└── SKILL.md🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/docs-web/src/content/docs/guides/skills.md` around lines 128 - 131,
Replace the anonymous fenced code block containing the directory tree snippet
(the triple-backticks block showing ".agents/skills/my-skill/ └── SKILL.md")
with a language-labeled fence by adding "text" (or "plaintext") after the
opening backticks so the fence becomes ```text, which will satisfy MD040 in CI.
Comment on lines
+41
to
+43
| if (Array.isArray(raw.skillRoots)) { | ||
| result.skillRoots = raw.skillRoots.filter((d): d is string => typeof d === 'string'); | ||
| } |
There was a problem hiding this comment.
Trim and drop empty skillRoots entries before storing them.
Right now any whitespace-only string survives parsing and can reach skill resolution as a real root. That makes config typos behave like valid search roots instead of being ignored.
🛠️ Suggested fix
if (Array.isArray(raw.skillRoots)) {
- result.skillRoots = raw.skillRoots.filter((d): d is string => typeof d === 'string');
+ result.skillRoots = raw.skillRoots
+ .filter((d): d is string => typeof d === 'string')
+ .map(root => root.trim())
+ .filter(root => root.length > 0);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (Array.isArray(raw.skillRoots)) { | |
| result.skillRoots = raw.skillRoots.filter((d): d is string => typeof d === 'string'); | |
| } | |
| if (Array.isArray(raw.skillRoots)) { | |
| result.skillRoots = raw.skillRoots | |
| .filter((d): d is string => typeof d === 'string') | |
| .map(root => root.trim()) | |
| .filter(root => root.length > 0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/codex/config.ts` around lines 41 - 43, When building
result.skillRoots from raw.skillRoots (the Array.isArray branch for
raw.skillRoots), trim each string and filter out entries that are not strings or
that are empty after trimming so whitespace-only values are dropped; update the
predicate used when populating result.skillRoots (referencing raw.skillRoots and
result.skillRoots) to map/trim values and only keep non-empty trimmed strings
before assigning to result.skillRoots.
Comment on lines
+96
to
+100
| function buildCodexSkillContext(skills: readonly LoadedSkill[]): string { | ||
| const blocks = skills.map( | ||
| skill => | ||
| `--- BEGIN SKILL ${skill.name} (${skill.skillPath}) ---\n${skill.content.trim()}\n--- END SKILL ${skill.name} ---` | ||
| ); |
There was a problem hiding this comment.
Avoid exposing absolute skill paths in prompts and info logs.
This currently sends skill.skillPath to the model and logs full paths at info; both can leak usernames/repo layout. Keep names (and maybe count) only, or log sanitized relative identifiers.
Suggested hardening diff
- `--- BEGIN SKILL ${skill.name} (${skill.skillPath}) ---\n${skill.content.trim()}\n--- END SKILL ${skill.name} ---`
+ `--- BEGIN SKILL ${skill.name} ---\n${skill.content.trim()}\n--- END SKILL ${skill.name} ---` getLog().info(
{
skills: loadedSkills.map(skill => skill.name),
- skillPaths: loadedSkills.map(skill => skill.skillPath),
+ skillCount: loadedSkills.length,
},
'codex.skills_loaded'
);Also applies to: 585-589
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/codex/provider.ts` around lines 96 - 100, The prompt
builder buildCodexSkillContext currently injects skill.skillPath into model
input and elsewhere full paths are logged; remove any use of skill.skillPath in
prompts and replace with non-sensitive identifiers (e.g., skill.name and an
index or truncated/sanitized ID) so only name/count are sent to the model;
update corresponding info logs that currently print skill.skillPath to instead
log either path.basename(skill.skillPath), a relative/sanitized identifier, or
redact the path entirely; also scan and update other locations that log or send
skill.skillPath (notably the other usages referenced around the same module) to
use the same sanitized identifier approach.
Comment on lines
147
to
150
| skills: z | ||
| .array(z.string().min(1, 'each skill must be a non-empty string')) | ||
| .array(z.string().trim().min(1, 'each skill must be a non-empty string')) | ||
| .nonempty("'skills' must be a non-empty array") | ||
| .optional(), |
There was a problem hiding this comment.
Don't keep skills in the loop-node ignore list.
This change makes skills a first-class validated field, but LOOP_NODE_AI_FIELDS later in this file still treats it as unsupported. Loop nodes will keep warning or dropping skills, which breaks the new provider-agnostic behavior.
🔧 Suggested fix
export const LOOP_NODE_AI_FIELDS: readonly string[] = BASH_NODE_AI_FIELDS.filter(
- f => f !== 'model' && f !== 'provider'
+ f => f !== 'model' && f !== 'provider' && f !== 'skills'
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/schemas/dag-node.ts` around lines 147 - 150, The
schema now treats the field skills as a validated first-class property but the
constant LOOP_NODE_AI_FIELDS still lists "skills" as an unsupported/ignored
field, causing loop nodes to drop or warn about skills; remove "skills" from the
LOOP_NODE_AI_FIELDS set/array (or update the logic that builds that list) so
loop-node handling no longer treats skills as unsupported, and run/update any
tests that assert supported fields for the loop node (references: skills and
LOOP_NODE_AI_FIELDS).
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.
Summary
Describe this PR in 2-5 bullets:
skills:.assistants.codex.skillRoots, enabled Codexskillscapability, wired workflow validation/CLI validation to provider-specific roots, and updated docs/tests.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
skills:schema@archon/providers/skills@archon/providers/skillsSKILL.mdfiles at runtimeskillRootsinto provider defaultsLabel Snapshot
risk: mediumsize: Mworkflows|providers|cli|config|docs|tests|skillsproviders:codex,providers:skills,workflows:validator,cli:validateChange Metadata
featureworkflows|providersLinked Issue
Validation Evidence (required)
Commands and result summary:
ARCHON_HOME=$(mktemp -d) bun run validateResult: passed. This ran bundled-default checks, type-check, lint with
--max-warnings 0, format check, and the per-package isolated test suite.Additional targeted checks run during development:
Real Archon workflow E2E skill smokes:
Security Impact (required)
YesNoNoYesCodex nodes can now explicitly load local
SKILL.mdfiles selected by workflowskills:. The file read scope is limited to explicit skill path refs and documented skill roots, with validation errors for missing or unreadable skills. The resolver now fails fast when a higher-precedence candidate exists but cannot be read, avoiding silent fallback to unintended lower-precedence instructions.Compatibility / Migration
YesYesNoExisting workflows without
skills:are unchanged.assistants.codex.skillRootsis optional; default roots still work. No upgrade steps are required unless a user wants custom skill roots:Human Verification (required)
What was personally validated beyond CI:
skills:using an explicit absolute skill directory;skills:using a skill name resolved from~/.codex/skills;$skilldiscovery inside an Archon workflow;.codex/skillsroots;minimax/MiniMax-M2.7was unavailable; validation did reachskillCount: 1/missingSkillCount: 0before provider auth failure.Side Effects / Blast Radius (required)
codex.skills_loadedwith loaded skill names and paths.Rollback Plan (required)
skills:from affected workflows restores prior Codex behavior for those nodes.Risks and Mitigations
Summary by CodeRabbit
New Features
Documentation
Tests