fix(skills): resolve skills info name mismatches#38701
Conversation
|
Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
Greptile SummaryThis PR fixes the Key points:
Confidence Score: 4/5
Last reviewed commit: 1355cae |
| it("resolves skill info case-insensitively", () => { | ||
| const report = createMockReport([ | ||
| createMockSkill({ | ||
| name: "Excel-XLSX", | ||
| skillKey: "excel-xlsx", | ||
| description: "Spreadsheet helpers", | ||
| }), | ||
| ]); | ||
|
|
||
| const output = formatSkillInfo(report, "excel-xlsx", {}); | ||
| expect(output).toContain("Spreadsheet helpers"); | ||
| }); |
There was a problem hiding this comment.
Test bypasses the case-insensitive code path
This test is labeled "resolves skill info case-insensitively" but the skill fixture's skillKey is already an exact lowercase match for the requested name. resolveSkillByName therefore finds the skill on the very first direct equality check and returns before the case-insensitive branch is ever evaluated.
To actually cover the toLowerCase comparison branch, both name and skillKey in the fixture should use a casing that does not exactly match the query, so the lookup is forced past the direct-match step. As currently written, a regression in the case-insensitive logic would leave this test green.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/skills-cli.test.ts
Line: 152-163
Comment:
**Test bypasses the case-insensitive code path**
This test is labeled "resolves skill info case-insensitively" but the skill fixture's `skillKey` is already an exact lowercase match for the requested name. `resolveSkillByName` therefore finds the skill on the very first direct equality check and returns before the case-insensitive branch is ever evaluated.
To actually cover the `toLowerCase` comparison branch, both `name` and `skillKey` in the fixture should use a casing that does not exactly match the query, so the lookup is forced past the direct-match step. As currently written, a regression in the case-insensitive logic would leave this test green.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1355caefff
ℹ️ 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".
| report.skills.find( | ||
| (s) => | ||
| normalizeSkillLookupToken(s.name) === normalized || | ||
| normalizeSkillLookupToken(s.skillKey) === normalized, |
There was a problem hiding this comment.
Disambiguate normalized skill-name collisions
resolveSkillByName picks the first entry whose normalized token matches, so skills info can return the wrong skill when multiple entries normalize to the same value (for example foo-bar and foo bar, or a name colliding with another skill’s skillKey). This is reachable because skill loading keeps distinct names as separate entries (src/agents/skills/workspace.ts:369-388), and the result then depends on source/load order rather than the user’s intended skill. Please detect multiple normalized candidates and surface an explicit ambiguous-name error instead of silently choosing the first match.
Useful? React with 👍 / 👎.
|
Queue is now under limit and I opened replacement PR #38713 with the same fix plus updated case-insensitive test coverage from bot feedback. |
Summary
openclaw skills info <name>resolve skills using exact, case-insensitive, and separator-normalized matchingWhy
Fixes #38546 where skills can show as ready in
skills listbut fail lookup inskills info(and by extension agent-facing selection paths) due strict identifier matching.Testing