fix: add Codex skill self-test mode - #680
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a deterministic --self-test mode to the Codex skills validator: helpers parse SKILL.md frontmatter and build/normalize deterministic Skill payloads; validateCodexSkills runs per-skill self-tests and returns a selfTests array; npm script, README, story doc, manifest, and unit tests were added/updated. ChangesCodex Skills Self-Test Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 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. Comment |
github-actions
Bot
added
area: agents
📊 Coverage ReportCoverage report not available
Generated by PR Automation (Story 6.1) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js (1)
174-181: 💤 Low valueSelf-test payload round-trip checks are tautological and can never fail.
createSkillToolSelfTestPayload(item.skillId)setsinput.skill = item.skillId;normalizeSkillToolTargetresolvesinput.skillfirst — sotargetis alwaysitem.skillIdandexpectedIds.has(target)is alwaystrue. The error branches on lines 176–181 are unreachable dead code that adds noise to future readers.🧹 Suggested cleanup
- const payload = createSkillToolSelfTestPayload(item.skillId); - const target = normalizeSkillToolTarget(payload); - if (target !== item.skillId) { - errors.push(`self-test Skill payload target mismatch: expected "${item.skillId}", got "${target}"`); - } - if (!expectedIds.has(target)) { - errors.push(`self-test Skill payload target is not a generated skill: ${target || '<empty>'}`); - }If the intent is to guard against future refactoring of
normalizeSkillToolTargetbreaking this contract, a unit test on the exported function (as already exists in the test file) is a better place for that assertion than runtime validation 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 @.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js around lines 174 - 181, The two runtime checks using createSkillToolSelfTestPayload(...) and normalizeSkillToolTarget(...) are tautological (normalizeSkillToolTarget reads input.skill which createSkillToolSelfTestPayload sets), so remove the unreachable error branches that push into errors (the comparisons against item.skillId and expectedIds.has(target)). Instead either delete those lines or replace them with a brief comment referencing the existing unit test for normalizeSkillToolTarget; if you want to keep a guard, move the assertions into a unit test rather than runtime validation. Ensure references to createSkillToolSelfTestPayload, normalizeSkillToolTarget, expectedIds, item.skillId and the errors array are handled accordingly.
🤖 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 `@tests/unit/codex-skills-validate.test.js`:
- Around line 8-11: The test uses relative requires for validateCodexSkills and
normalizeSkillToolTarget (and the pre-existing syncSkills import); update these
to use the project's absolute import paths instead of ../../ relative paths so
they conform to the absolute-imports guideline—replace the require calls that
load validateCodexSkills, normalizeSkillToolTarget (and syncSkills) with their
corresponding absolute module specifiers according to the project module map so
the test imports these symbols using absolute paths.
- Around line 79-81: The test currently assumes result.selfTests.find(...)
returns an object and accesses .ok, which can throw if undefined; update the
test to first capture the result of result.selfTests.find(test => test.skillId
=== 'aiox-dev') into a variable (e.g., selfTest), assert that this variable is
defined/not null, then assert selfTest.ok is false so failures are readable and
won't raise a TypeError.
---
Nitpick comments:
In @.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js:
- Around line 174-181: The two runtime checks using
createSkillToolSelfTestPayload(...) and normalizeSkillToolTarget(...) are
tautological (normalizeSkillToolTarget reads input.skill which
createSkillToolSelfTestPayload sets), so remove the unreachable error branches
that push into errors (the comparisons against item.skillId and
expectedIds.has(target)). Instead either delete those lines or replace them with
a brief comment referencing the existing unit test for normalizeSkillToolTarget;
if you want to keep a guard, move the assertions into a unit test rather than
runtime validation. Ensure references to createSkillToolSelfTestPayload,
normalizeSkillToolTarget, expectedIds, item.skillId and the errors array are
handled accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83463c3e-8190-4656-97df-7da0b04aae5e
📒 Files selected for processing (7)
.aiox-core/data/entity-registry.yaml.aiox-core/infrastructure/scripts/codex-skills-sync/README.md.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js.aiox-core/install-manifest.yamldocs/stories/epic-123/STORY-123.21-codex-skill-tool-self-test.mdpackage.jsontests/unit/codex-skills-validate.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js:
- Around line 179-187: The code currently only checks that
extractCanonicalAgentPath(content) exists on disk; instead construct the
expected canonical relative path for the current item using item.filename (the
agents source path under the development agents directory), assert
canonicalAgentPath === expectedRelativePath and push an error if they differ,
and then (optionally) keep the fs.existsSync(resolved.projectRoot +
canonicalAgentPath) check as a secondary guard; update the block that references
extractCanonicalAgentPath, canonicalAgentPath, resolved.projectRoot, errors, and
item.filename to perform this equality check before the existence check.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 292e2ea6-fa83-4717-a880-bd9ba3e3820c
📒 Files selected for processing (3)
.aiox-core/data/entity-registry.yaml.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js.aiox-core/install-manifest.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .aiox-core/data/entity-registry.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js (1)
254-257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEarly return omits
selfTestsfrom the return shape.The fast-exit path at line 256 (when
skillsDiris missing) does not includeselfTests. The new version replaces the safe functions withloadanddump, both safe by default now. WhileformatHumanReportguards against it withresult.selfTests &&, external callers relying on the documented return shape (e.g.,result.selfTests.length) would receive aTypeErroron this path.🛠️ Proposed fix
- return { ok: false, checked: 0, expected: 0, errors, warnings, missing: [], orphaned: [], ignored: [] }; + return { ok: false, checked: 0, expected: 0, errors, warnings, missing: [], orphaned: [], legacy: [], ignored: [], selfTests: [] };🤖 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 @.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js around lines 254 - 257, The early return when resolved.skillsDir is missing omits the selfTests field causing callers expecting result.selfTests to crash; update the fast-exit return in validate.js (the block checking fs.existsSync(resolved.skillsDir)) to include selfTests (an empty array) in the returned object so the return shape matches the normal path (keep existing errors, warnings, missing, orphaned, ignored and add selfTests: []). Ensure you update the return value used by formatHumanReport and any external callers that access result.selfTests.
🧹 Nitpick comments (1)
.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js (1)
167-204: ⚡ Quick winPayload roundtrip check produces cascading errors and false confidence.
Two related problems:
Cascading errors on name mismatch. When
frontmatter.data.name !== item.skillId, the frontmatter error at line 174 is the root cause. BecausedeclaredSkillIdis set to the wrong name, lines 199-200 and 202-203 each fire again for the same mismatched name — resulting in up to 3 errors about a single field.False confidence on parse failure. When
frontmatter.erroris set,declaredSkillIdfalls back toitem.skillId(line 168 initializer). The payload checks then trivially pass (target === item.skillId), silently reporting no issue even though the frontmatter is broken.♻️ Proposed fix
- const payload = createSkillToolSelfTestPayload(declaredSkillId); - const target = normalizeSkillToolTarget(payload); - if (target !== item.skillId) { - errors.push(`self-test Skill payload target mismatch: expected "${item.skillId}", got "${target}"`); - } - if (!expectedIds.has(target)) { - errors.push(`self-test Skill payload target is not a generated skill: ${target || '<empty>'}`); - } + if (!frontmatter.error) { + const payload = createSkillToolSelfTestPayload(declaredSkillId); + const target = normalizeSkillToolTarget(payload); + if (target !== item.skillId) { + errors.push(`self-test Skill payload target mismatch: expected "${item.skillId}", got "${target}"`); + } else if (!expectedIds.has(target)) { + errors.push(`self-test Skill payload target is not a generated skill: ${target || '<empty>'}`); + } + }🤖 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 @.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js around lines 167 - 204, The current logic causes duplicate errors and a false-pass when frontmatter parsing fails: fix by only running the payload roundtrip (createSkillToolSelfTestPayload + normalizeSkillToolTarget + expectedIds checks) when frontmatter parsed successfully (frontmatter.error is falsy) AND frontmatter.data.name equals item.skillId; set declaredSkillId from frontmatter.data.name only in that success path (leave it undefined/null otherwise) and skip the payload/target comparisons (the checks that reference declaredSkillId, createSkillToolSelfTestPayload, normalizeSkillToolTarget, and expectedIds) when parsing failed or the frontmatter name mismatches to avoid cascading/duplicate errors.
🤖 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 @.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js:
- Around line 140-147: The default skillsDir is computed from process.cwd()
before options are merged, so a caller-provided projectRoot is ignored; modify
runSkillSelfTests to compute skillsDir after merging options (i.e., build
resolved first with projectRoot from options/default, then set
resolved.skillsDir = resolved.skillsDir || path.join(resolved.projectRoot,
'.codex', 'skills')) so skillsDir respects a supplied projectRoot; update
references to resolved.skillsDir and keep expected mapping logic (expectedIds)
unchanged.
- Around line 206-208: The self-test greeting-command check duplicates the same
content.includes('generate-greeting.js ${item.agentId}') validation already
performed by validateSkillContent, causing duplicate errors when selfTest is
true; remove (or skip) the duplicate check from runSkillSelfTests by deleting
the if (!content.includes(...)) errors.push(...) branch in runSkillSelfTests (or
guard it with a condition that skips it when selfTest mode is active) so only
validateSkillContent performs this check.
---
Outside diff comments:
In @.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js:
- Around line 254-257: The early return when resolved.skillsDir is missing omits
the selfTests field causing callers expecting result.selfTests to crash; update
the fast-exit return in validate.js (the block checking
fs.existsSync(resolved.skillsDir)) to include selfTests (an empty array) in the
returned object so the return shape matches the normal path (keep existing
errors, warnings, missing, orphaned, ignored and add selfTests: []). Ensure you
update the return value used by formatHumanReport and any external callers that
access result.selfTests.
---
Nitpick comments:
In @.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js:
- Around line 167-204: The current logic causes duplicate errors and a
false-pass when frontmatter parsing fails: fix by only running the payload
roundtrip (createSkillToolSelfTestPayload + normalizeSkillToolTarget +
expectedIds checks) when frontmatter parsed successfully (frontmatter.error is
falsy) AND frontmatter.data.name equals item.skillId; set declaredSkillId from
frontmatter.data.name only in that success path (leave it undefined/null
otherwise) and skip the payload/target comparisons (the checks that reference
declaredSkillId, createSkillToolSelfTestPayload, normalizeSkillToolTarget, and
expectedIds) when parsing failed or the frontmatter name mismatches to avoid
cascading/duplicate errors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c7831b3-5d02-4f04-9609-e8eba510901d
📒 Files selected for processing (4)
.aiox-core/data/entity-registry.yaml.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js.aiox-core/install-manifest.yamltests/unit/codex-skills-validate.test.js
✅ Files skipped from review due to trivial changes (1)
- .aiox-core/install-manifest.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/codex-skills-validate.test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js:
- Around line 200-208: The else-if checking expectedIds.has(target) is dead
because expectedIds is built from resolved.expected and the loop compares
against item.skillId, so when target === item.skillId it will always be in
expectedIds; update the logic in the canRoundtripSkillPayload block (around
createSkillToolSelfTestPayload and normalizeSkillToolTarget) to first validate
that target exists in expectedIds (if (!expectedIds.has(target)) push the "not a
generated skill" error), and only then check for mismatch with item.skillId
(else if (target !== item.skillId) push the "payload target mismatch" error), or
simply remove the unreachable else-if if you prefer; adjust the order in that
block so existence is checked before equality.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6619a247-3cf9-4086-b055-48f88fc1696b
📒 Files selected for processing (4)
.aiox-core/data/entity-registry.yaml.aiox-core/infrastructure/scripts/codex-skills-sync/validate.js.aiox-core/install-manifest.yamltests/unit/codex-skills-validate.test.js
✅ Files skipped from review due to trivial changes (1)
- .aiox-core/install-manifest.yaml
|
DevOps merge attempt blocked by CODEOWNERS.\n\nCurrent head: e56353f\nValidation: all required CI checks pass, including ESLint, TypeScript, Jest Node 18/20/22/24/25, Validation Summary, Coverage Report, and CodeRabbit approval.\n\nAttempted merge paths:\n- gh pr merge 680 --squash --admin --delete-branch\n- REST PUT /repos/SynkraAI/aiox-core/pulls/680/merge with squash and fixed head SHA\n\nBoth were blocked server-side by: Waiting on code owner review from Pedrovaleriolopez and/or oalanicolas.\n\n@Pedrovaleriolopez @oalanicolas please review/approve so this can be merged. Closes #622. |
1 participant
Summary
--self-testmode to Codex skill validation for Skill tool driven skills.validate:codex-skills:self-test, unit coverage, manifest update, and Story 123.21.Closes #622
Validation
node -c .aiox-core/infrastructure/scripts/codex-skills-sync/validate.jsnpm test -- tests/unit/codex-skills-validate.test.js --runInBand --forceExitnpm run validate:codex-skills:self-testnpm run generate:manifestnpm run validate:manifestnpm run lint -- --quietnpm run typechecknpm run validate:paritynpm run test:ciSummary by CodeRabbit
New Features
Documentation
Tests
Chores