CONSOLE-5194: Audit and fix Claude Code skills for accuracy#16316
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sg00dwin: This pull request references CONSOLE-5194 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughUpdates multiple Claude Code skill docs and review tooling: testing guidance in gen-rtl-test (prefer userEvent, import path update), project-context and workflow tracking references switched to AGENTS.md and TaskCreate/TaskUpdate, a new skill-audit skill added to validate skills and references, and CodeRabbit config extended with a pre-merge test check. Claude skills and tooling updates
Sequence Diagram(s) sequenceDiagram
participant SkillAuditor
participant SkillsDir as .claude/skills/
participant RepoFS as RepositoryFS
participant FrontendPkg as frontend/package.json
participant Reporter
SkillAuditor->>SkillsDir: enumerate skill markdown files
SkillsDir-->>SkillAuditor: file list
SkillAuditor->>RepoFS: resolve referenced paths (repo root then frontend/)
RepoFS-->>SkillAuditor: existence results
SkillAuditor->>FrontendPkg: check yarn/npm script entries
FrontendPkg-->>SkillAuditor: script presence
SkillAuditor->>Reporter: emit per-skill PASS/WARN/ERROR/INFO report
Estimated code review effort: Suggested labels: Suggested reviewers:
🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/skill-audit/SKILL.md:
- Line 81: The fenced code block that opens before the "## Skill Audit Report"
example lacks a language tag (triggering MD040); update the opening fence for
that example to include a language identifier (e.g., change the opening "```" to
"```markdown") so the block is correctly recognized by markdown-lint and docs
pipelines.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 43404fdc-d47e-4a9b-a7db-218a67b993d3
📒 Files selected for processing (4)
.claude/skills/gen-rtl-test/SKILL.md.claude/skills/pre-push-review/SKILL.md.claude/skills/skill-audit/SKILL.md.claude/skills/update-package/SKILL.md
📜 Review details
🧰 Additional context used
🪛 markdownlint-cli2 (0.22.0)
.claude/skills/skill-audit/SKILL.md
[warning] 81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔀 Multi-repo context openshift/console-operator
[::openshift/console-operator::] .claude/commands/ci-prep.md: line 96 — contains guidance "Use the TodoWrite tool to track progress..." (TodoWrite referenced).
[::openshift/console-operator::] CLAUDE.md: line 7 — references AGENTS.md.
[::openshift/console-operator::] vendor/github.com/openshift/api/.coderabbit.yaml: lines 27,49-50 — references AGENTS.md and CLAUDE.md.
Notes: this repository has no .claude/skills directory and no matches for @console/test-utils or @console/shared test-utils imports.
🔇 Additional comments (4)
.claude/skills/pre-push-review/SKILL.md (1)
134-134: Good consistency update on context + workflow tooling.Switching project guidance to
AGENTS.mdand standardizing progress tracking onTaskCreate/TaskUpdateis coherent across the file and reduces stale-tool drift.Also applies to: 159-159, 320-323, 373-374, 454-454, 476-476, 492-492
.claude/skills/update-package/SKILL.md (1)
73-73: Nice alignment with current Claude task APIs.Using
TaskCreate/TaskUpdatehere keeps package-update workflow tracking consistent with other skills and avoids deprecated tool names..claude/skills/gen-rtl-test/SKILL.md (2)
512-512: Rule 14 update is a solid improvement.Preferring
userEventwhile still allowingfireEventfor simple synchronous cases is the right balance for realism and maintainability in generated tests.Also applies to: 556-575, 578-587
196-196: No action needed — the import alias is properly configured.The alias
@console/shared/*is explicitly mapped infrontend/tsconfig.json(line 22) to./packages/console-shared/*, which correctly resolves@console/shared/src/test-utils/unit-test-utilsto the existing file atfrontend/packages/console-shared/src/test-utils/unit-test-utils.tsx. Generated tests placed in thefrontend/directory will inherit this configuration and resolve the import without issues.
There was a problem hiding this comment.
LGTM (but I did not write the skill :)
sg00dwin
force-pushed
the
CONSOLE-5194-AI-Tooling-Review
branch
from
4f19328 to
58c6de6
Compare
| // Simulate clicking | ||
| fireEvent.click(button); | ||
| expect(screen.getByText(/success/i)).toBeVisible(); | ||
| }, 30000); // userEvent.type is slow — extend timeout for tests with significant typing |
There was a problem hiding this comment.
Would it be better to remove , 30000) and show proper async handling instead? Rather than mask timing issues with extended timeouts.
sg00dwin
force-pushed
the
CONSOLE-5194-AI-Tooling-Review
branch
from
58c6de6 to
94d7ff9
Compare
|
Tech debt, so assigning labels |
openshift-ci
Bot
added
px-approved
|
/verified bypass |
openshift-ci-robot
added
the
verified
|
@logonoff: The DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
sg00dwin
force-pushed
the
CONSOLE-5194-AI-Tooling-Review
branch
from
94d7ff9 to
426af92
Compare
openshift-ci-robot
removed
the
verified
openshift-ci
Bot
added
lgtm
|
/verified by @jhadvig |
jhadvig
added
the
plugin-api-approved
openshift-ci-robot
added
the
verified
|
@jhadvig: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Assisted by Claude code
…from Claude Code and replaced by TaskCreate/TaskUpdate
Assisted by Claude code
sg00dwin
force-pushed
the
CONSOLE-5194-AI-Tooling-Review
branch
from
426af92 to
8ec2aea
Compare
openshift-ci-robot
removed
the
verified
|
@sg00dwin: This pull request references CONSOLE-5194 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.claude/skills/skill-audit/SKILL.md (1)
154-154:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced report example.
The code block at line 154 lacks a language specifier, triggering markdownlint MD040.
Suggested fix
-``` +```markdown ## Skill Audit Report ...</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In @.claude/skills/skill-audit/SKILL.md at line 154, The fenced code block that
begins with "## Skill Audit Report" is missing a language tag which triggers
markdownlint MD040; update the opening fence fromtomarkdown so the
block is explicitly marked as Markdown (locate the triple-backtick fence
surrounding the "## Skill Audit Report" example in SKILL.md and change it to
🤖 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 @.claude/skills/skill-audit/SKILL.md:
- Around line 27-31: The numbered steps in SKILL.md currently read "1., 3., 4.,
5." because the second item was mislabeled as 3; update the list so the sequence
is consecutive (1, 2, 3, 4, 5) by changing the line that currently starts with
"3" to "2" and ensuring the following lines remain 3, 4, 5; check the block that
lists "For each path found:" and correct the numbering so each step (Resolve
relative to the repo root; If not found, try resolving relative to `frontend/`;
Check if the file exists on disk; Record any missing paths with the skill name,
file, and line number) is numbered 1–4 (or 1–5 if there are five items
intended).
In @.coderabbit.yaml:
- Line 49: The config uses the wrong key name "instruction" on the custom check
entry; rename that field to "instructions" (plural) so it matches the schema;
locate the entry using the "instruction" key and replace it with "instructions"
ensuring the value structure remains unchanged.
---
Duplicate comments:
In @.claude/skills/skill-audit/SKILL.md:
- Line 154: The fenced code block that begins with "## Skill Audit Report" is
missing a language tag which triggers markdownlint MD040; update the opening
fence from ``` to ```markdown so the block is explicitly marked as Markdown
(locate the triple-backtick fence surrounding the "## Skill Audit Report"
example in SKILL.md and change it to ```markdown).
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0855b7ad-7dde-41b9-8649-c002fe08d458
📒 Files selected for processing (5)
.claude/skills/gen-rtl-test/SKILL.md.claude/skills/pre-push-review/SKILL.md.claude/skills/skill-audit/SKILL.md.claude/skills/update-package/SKILL.md.coderabbit.yaml
✅ Files skipped from review due to trivial changes (2)
- .claude/skills/gen-rtl-test/SKILL.md
- .claude/skills/pre-push-review/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/skills/update-package/SKILL.md
cajieh
left a comment
cajieh
left a comment
There was a problem hiding this comment.
It looks good!👍
Added comments. Thank you.
| Inconclusive: Unable to determine test file presence (e.g., file patterns don't | ||
| match conventions). | ||
|
|
||
| Skip: Files matching these patterns are exempt from this check: |
There was a problem hiding this comment.
Does the sequence of execution matter? Should the check for 'Skip' precede 'Pass' and 'Fail'?
There was a problem hiding this comment.
Filter out exempt files first, then evaluate what's left, reads more naturally and reduces ambiguity. I'd reorder it. 👍
| review_details: true | ||
| pre_merge_checks: | ||
| custom_checks: | ||
| - name: "Test coverage for new source files" |
There was a problem hiding this comment.
Without the mode: error would the pre-merge fail?
| related_prs: false | ||
| suggested_labels: false | ||
| review_details: true | ||
| pre_merge_checks: |
There was a problem hiding this comment.
The pre_merge_checks nudges authors to add test files for new components; Jest coverage thresholds enforce measured execution coverage. They complement each other rather than replace one another.
| custom_checks: | ||
| - name: "Test coverage for new source files" | ||
| instruction: | | ||
| For every new TypeScript/TSX source file added under frontend/ (excluding test files, |
There was a problem hiding this comment.
Maybe we should add the ".go" source files as well.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.claude/skills/gen-rtl-test/SKILL.md (1)
1085-1099: ⚡ Quick winAlign Strategy 3 title with the example implementation.
The heading says to use
waitFor, but the snippet demonstratesfindByText. Either rename the strategy to emphasizefindBy*for dropdown content, or update the snippet to include an actualwaitForpattern.🤖 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 @.claude/skills/gen-rtl-test/SKILL.md around lines 1085 - 1099, The Strategy 3 heading currently mentions waitFor but the example uses findByText; update the docs so the title matches the example: change the heading text from "Use waitFor for complex interactions (e.g., dropdowns)" to "Use findBy* for async dropdown content (e.g., dropdowns)" (or similar), and keep the example that uses userEvent.setup(), screen.getByText('Select Option'), await screen.findByText('Option 1'), and await user.click(option); alternatively, if you prefer to keep the original waitFor intent, replace the example's findByText usage with a waitFor pattern that waits for screen.getByText('Option 1') before clicking. Ensure you update the Strategy 3 title and the associated snippet (references: Strategy 3 heading, userEvent.setup(), screen.getByText('Select Option'), findByText/ waitFor) so they consistently reflect the chosen approach.
🤖 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.
Nitpick comments:
In @.claude/skills/gen-rtl-test/SKILL.md:
- Around line 1085-1099: The Strategy 3 heading currently mentions waitFor but
the example uses findByText; update the docs so the title matches the example:
change the heading text from "Use waitFor for complex interactions (e.g.,
dropdowns)" to "Use findBy* for async dropdown content (e.g., dropdowns)" (or
similar), and keep the example that uses userEvent.setup(),
screen.getByText('Select Option'), await screen.findByText('Option 1'), and
await user.click(option); alternatively, if you prefer to keep the original
waitFor intent, replace the example's findByText usage with a waitFor pattern
that waits for screen.getByText('Option 1') before clicking. Ensure you update
the Strategy 3 title and the associated snippet (references: Strategy 3 heading,
userEvent.setup(), screen.getByText('Select Option'), findByText/ waitFor) so
they consistently reflect the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: abbb2e54-dcb9-4232-99c0-be1016a22de5
📒 Files selected for processing (1)
.claude/skills/gen-rtl-test/SKILL.md
Update coderabbit config and fix skill-audit numbering Assisted by Claude code
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.coderabbit.yaml:
- Around line 52-58: The skip-pattern list under "Skip: Files matching these
patterns" is missing the declaration file pattern for TypeScript (.d.ts),
causing type-definition files to be incorrectly validated; update the skip
patterns to include the declaration-file glob (e.g., add **/*.d.ts) alongside
the existing **/types.ts, **/types.tsx, **/index.ts, **/index.tsx entries so
.d.ts files are exempt from the test-file-exists check.
- Around line 5-11: The `tone_instructions` value exceeds the schema maxLength
and may fail validation; shorten or relocate it: trim the text in the
`tone_instructions` key to under 250 characters, or move the full guidance to an
external file (e.g., docs or TESTING.md) and replace `tone_instructions` with a
short reference or alias. Update the `.coderabbit.yaml` entry named
`tone_instructions` accordingly and ensure any code that reads this key
(configuration loader) still accepts the shorter value or resolves the external
reference.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 77438b08-a53e-4e76-83eb-ad6df0f26a6c
📒 Files selected for processing (2)
.claude/skills/skill-audit/SKILL.md.coderabbit.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude/skills/skill-audit/SKILL.md
| tone_instructions: >- | ||
| Principal engineer reviewing OpenShift Console (React/TS/JS/Go). | ||
| Focus: architecture, security (RBAC/SCC), performance, maintainability, backward-compatible APIs, testing/edge cases. | ||
| Enforce K8s best practices, PatternFly, a11y/i18n. | ||
| For every changed or new component, hook, or utility, check whether the PR includes corresponding test coverage. | ||
| Flag specific untested code paths, edge cases, and error conditions that should have tests. | ||
| Reference TESTING.md conventions when suggesting test approaches. |
There was a problem hiding this comment.
tone_instructions likely violates schema max length.
Line 5 defines a tone_instructions block that appears to exceed the schema limit (maxLength: 250). This can cause config validation problems or the instruction being ignored.
Suggested minimal fix
-tone_instructions: >-
- Principal engineer reviewing OpenShift Console (React/TS/JS/Go).
- Focus: architecture, security (RBAC/SCC), performance, maintainability, backward-compatible APIs, testing/edge cases.
- Enforce K8s best practices, PatternFly, a11y/i18n.
- For every changed or new component, hook, or utility, check whether the PR includes corresponding test coverage.
- Flag specific untested code paths, edge cases, and error conditions that should have tests.
- Reference TESTING.md conventions when suggesting test approaches.
+tone_instructions: >-
+ Review as a principal engineer for OpenShift Console. Prioritize architecture, security, performance, maintainability, compatibility, and test gaps. Follow TESTING.md, AGENTS.md, and STYLEGUIDE.md.📝 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.
| tone_instructions: >- | |
| Principal engineer reviewing OpenShift Console (React/TS/JS/Go). | |
| Focus: architecture, security (RBAC/SCC), performance, maintainability, backward-compatible APIs, testing/edge cases. | |
| Enforce K8s best practices, PatternFly, a11y/i18n. | |
| For every changed or new component, hook, or utility, check whether the PR includes corresponding test coverage. | |
| Flag specific untested code paths, edge cases, and error conditions that should have tests. | |
| Reference TESTING.md conventions when suggesting test approaches. | |
| tone_instructions: >- | |
| Review as a principal engineer for OpenShift Console. Prioritize architecture, security, performance, maintainability, compatibility, and test gaps. Follow TESTING.md, AGENTS.md, and STYLEGUIDE.md. |
🤖 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 @.coderabbit.yaml around lines 5 - 11, The `tone_instructions` value exceeds
the schema maxLength and may fail validation; shorten or relocate it: trim the
text in the `tone_instructions` key to under 250 characters, or move the full
guidance to an external file (e.g., docs or TESTING.md) and replace
`tone_instructions` with a short reference or alias. Update the
`.coderabbit.yaml` entry named `tone_instructions` accordingly and ensure any
code that reads this key (configuration loader) still accepts the shorter value
or resolves the external reference.
| added under pkg/ or cmd/ (excluding test files, type definition files, index files, | ||
| and SCSS/CSS files), verify that a corresponding test file exists or is added in | ||
| this PR. | ||
|
|
||
| Skip: Files matching these patterns are exempt from this check: | ||
| - **/types.ts, **/types.tsx (type-only files) | ||
| - **/index.ts, **/index.tsx (barrel exports) |
There was a problem hiding this comment.
Type-definition exclusion is incomplete vs stated rule.
Lines 52 and 57-58 say type definition files are excluded, but the patterns do not exempt *.d.ts. Add that pattern to align behavior with the stated rule and avoid false failures.
Suggested patch
Skip: Files matching these patterns are exempt from this check:
+ - **/*.d.ts (TypeScript declaration files)
- **/types.ts, **/types.tsx (type-only files)📝 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.
| added under pkg/ or cmd/ (excluding test files, type definition files, index files, | |
| and SCSS/CSS files), verify that a corresponding test file exists or is added in | |
| this PR. | |
| Skip: Files matching these patterns are exempt from this check: | |
| - **/types.ts, **/types.tsx (type-only files) | |
| - **/index.ts, **/index.tsx (barrel exports) | |
| added under pkg/ or cmd/ (excluding test files, type definition files, index files, | |
| and SCSS/CSS files), verify that a corresponding test file exists or is added in | |
| this PR. | |
| Skip: Files matching these patterns are exempt from this check: | |
| - **/*.d.ts (TypeScript declaration files) | |
| - **/types.ts, **/types.tsx (type-only files) | |
| - **/index.ts, **/index.tsx (barrel exports) |
🤖 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 @.coderabbit.yaml around lines 52 - 58, The skip-pattern list under "Skip:
Files matching these patterns" is missing the declaration file pattern for
TypeScript (.d.ts), causing type-definition files to be incorrectly validated;
update the skip patterns to include the declaration-file glob (e.g., add
**/*.d.ts) alongside the existing **/types.ts, **/types.tsx, **/index.ts,
**/index.tsx entries so .d.ts files are exempt from the test-file-exists check.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh, jhadvig, sg00dwin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by @cajieh |
|
/test all |
openshift-ci-robot
added
the
verified
|
@cajieh: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@sg00dwin: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
Analysis:
AI coding skills in
.claude/skills/accumulate stale references over time — deprecated tool names, incorrect file paths, outdated config file references — with no automated way to detect the drift.Note: Assigning review of skill-specific changes to the associated creator of each skill.
/assign @cajieh to review
.claude/skills/gen-rtl-test/SKILL.md/assign @Leo6Leo to review
.claude/skills/pre-push-review/SKILL.md/assign @logonoff to review
.claude/skills/update-package/SKILL.md/assign @jhadvig to review
claude/skills/skill-audit/SKILL.mdDescription:
gen-rtl-test: Corrected@console/test-utilsimport path to@console/shared/src/test-utils/unit-test-utils; updated Rule 14 to recommenduserEventoverfireEvent(no longer blocked by Jest version)pre-push-review: Replaced 6 references to deprecatedTodoWritewithTaskCreate/TaskUpdate; corrected.claude/CLAUDE.mdreferences toAGENTS.mdupdate-package: ReplacedTodoWritereference withTaskCreate/TaskUpdateskill-auditskill (/skill-audit) that detects:@console/*import paths.claude/skills/directories (bidirectional: config→skills and skills→config)Test cases:
/skill-auditand verify it produces a report with no false positives/skill-audit gen-rtl-testand verify single-skill mode works/gen-rtl-teston a component and verify the corrected import path appears in generated tests/pre-push-reviewand verify it usesTaskCreate/TaskUpdate(notTodoWrite)Summary by CodeRabbit
New Features
Documentation
Chores