NO-JIRA: Improve AI skill quality based on review comment analysis

[bryan-cox]

Summary

Automated analysis of review comments from the JIRA Agent Dashboard
identified recurring patterns in PR feedback. This PR updates AGENTS.md
(CLAUDE.md) to address the most common issues.

Analysis Period

2025-01-01 to 2026-06-05 — 78 comments analyzed, 62 actionable
(after filtering nitpick/approval/unclassified)

Changes

Change 1: Fleet-wide rollout impact awareness

File: AGENTS.md (Common Gotchas section)

Pattern: architecture_design — 5 comments (3 required_change, 2 suggestion)

Evidence:

"this change would trigger a fleet wide nodepool rollout. This is reflected in the hash changes of the unit tests and it should fail e2e-aws-upgrade-hypershift-operator"
— @enxebre, PR #7943

"This is dangerous, it will affect to all HostedCluster currently deployed, causing a rollout."
— @jparrill, PR #7943

Reasoning: AI-generated PRs added data to ignition configs without
understanding that changes to config hash inputs cause fleet-wide NodePool
rollouts. The new rule instructs agents to check for config hash impact
and run the upgrade e2e test.

---

Change 2: No unrelated changes in PRs

File: AGENTS.md (Common Gotchas section)

Pattern: process — 5 required_change comments about cosmetic changes

Evidence:

"This cosmetic formatting change is completely unrelated to Azure role assignment validation. It increases the review surface and makes the PR harder to revert cleanly."
— @bryan-cox, PR #8319

Reasoning: AI-generated PRs frequently included import reordering or
whitespace changes in unrelated files, making reviews harder and PRs
difficult to revert.

---

Change 3: Follow existing codebase patterns

File: AGENTS.md (Common Gotchas section)

Pattern: architecture_design — 3 required_change comments

Evidence:

"We have been moving away from using the service-ca operator at all. How does that affect this PR?"
— @bryan-cox, PR #7943

"You should be able to look at reconcileSelfSignedCA in the codebase"
— @bryan-cox, PR #7943

Reasoning: AI implemented TLS using service-ca operator when the
codebase has been migrating to self-signed certs. The rule instructs
agents to search for existing patterns before implementing novel approaches.

---

Change 4: Validation regex correctness

File: AGENTS.md (Code conventions section)

Pattern: logic_bug — 23 comments (20 required_change), many about regex

Evidence:

"It looks like the regular expression here allows the subscription ID to contain { and } at the beginning and end. Is this actually valid?"
— @everettraven, PR #8106

"It looks like the valid character set TODO here wasn't resolved."
— @everettraven, PR #8211

Reasoning: Multiple PRs shipped regex patterns with over-broad
character classes or unresolved TODOs. The rules require matching upstream
specs exactly and resolving all TODOs before submitting.

---

Change 5: Dead code and export hygiene

File: AGENTS.md (Code conventions section)

Pattern: style — 6 comments (2 required_change, 4 suggestion)

Evidence:

"CompareCRDs is exported but only used in tests."
— @bryan-cox, PR #8535

"mustParseCRD is dead code — defined but never called anywhere in the test file. Remove it."
— @bryan-cox, PR #8535

Reasoning: AI-generated code left exported wrappers only used by tests
and functions that were never called. Rules now prohibit both patterns.

---

Change 6: Test coverage and real-world fixtures

File: AGENTS.md (Code conventions section)

Pattern: test_gap — 11 comments (4 required_change, 7 suggestion)

Evidence:

"Add unit tests for this method, at least with the 5 basic flow paths"
— @jparrill, PR #7943

"A real example would be helpful, maybe use quay.io/openshift-release-dev/ocp-release:4.21.10-x86_64"
— @bryan-cox, PR #8211

Reasoning: AI-generated tests used synthetic fixtures and missed key
paths. Rules now require real-world values and minimum coverage paths.

Test Plan

• Verify AGENTS.md parses correctly (no broken markdown)
• Next jira-solve runs should show fewer comments in the addressed categories
• Compare comment counts week-over-week in the dashboard

🤖 Generated with Claude Code via improve-ai-quality skill

Summary by CodeRabbit

• Documentation
  • Updated internal development guidance with new best practices for fleet-wide deployments, code conventions for validation patterns, certificate/TLS handling, and testing standards.

[openshift-merge-bot]

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@bryan-cox: This pull request explicitly references no jira issue.

Details

In response to this:

Summary

Automated analysis of review comments from the JIRA Agent Dashboard
identified recurring patterns in PR feedback. This PR updates AGENTS.md
(CLAUDE.md) to address the most common issues.

Analysis Period

2025-01-01 to 2026-06-05 — 78 comments analyzed, 62 actionable
(after filtering nitpick/approval/unclassified)

Changes

Change 1: Fleet-wide rollout impact awareness

File: AGENTS.md (Common Gotchas section)

Pattern: architecture_design — 5 comments (3 required_change, 2 suggestion)

Evidence:

"this change would trigger a fleet wide nodepool rollout. This is reflected in the hash changes of the unit tests and it should fail e2e-aws-upgrade-hypershift-operator"
— @enxebre, PR #7943

"This is dangerous, it will affect to all HostedCluster currently deployed, causing a rollout."
— @jparrill, PR #7943

Reasoning: AI-generated PRs added data to ignition configs without
understanding that changes to config hash inputs cause fleet-wide NodePool
rollouts. The new rule instructs agents to check for config hash impact
and run the upgrade e2e test.

---

Change 2: No unrelated changes in PRs

File: AGENTS.md (Common Gotchas section)

Pattern: process — 5 required_change comments about cosmetic changes

Evidence:

"This cosmetic formatting change is completely unrelated to Azure role assignment validation. It increases the review surface and makes the PR harder to revert cleanly."
— @bryan-cox, PR #8319

Reasoning: AI-generated PRs frequently included import reordering or
whitespace changes in unrelated files, making reviews harder and PRs
difficult to revert.

---

Change 3: Follow existing codebase patterns

File: AGENTS.md (Common Gotchas section)

Pattern: architecture_design — 3 required_change comments

Evidence:

"We have been moving away from using the service-ca operator at all. How does that affect this PR?"
— @bryan-cox, PR #7943

"You should be able to look at reconcileSelfSignedCA in the codebase"
— @bryan-cox, PR #7943

Reasoning: AI implemented TLS using service-ca operator when the
codebase has been migrating to self-signed certs. The rule instructs
agents to search for existing patterns before implementing novel approaches.

---

Change 4: Validation regex correctness

File: AGENTS.md (Code conventions section)

Pattern: logic_bug — 23 comments (20 required_change), many about regex

Evidence:

"It looks like the regular expression here allows the subscription ID to contain { and } at the beginning and end. Is this actually valid?"
— @everettraven, PR #8106

"It looks like the valid character set TODO here wasn't resolved."
— @everettraven, PR #8211

Reasoning: Multiple PRs shipped regex patterns with over-broad
character classes or unresolved TODOs. The rules require matching upstream
specs exactly and resolving all TODOs before submitting.

---

Change 5: Dead code and export hygiene

File: AGENTS.md (Code conventions section)

Pattern: style — 6 comments (2 required_change, 4 suggestion)

Evidence:

"CompareCRDs is exported but only used in tests."
— @bryan-cox, PR #8535

"mustParseCRD is dead code — defined but never called anywhere in the test file. Remove it."
— @bryan-cox, PR #8535

Reasoning: AI-generated code left exported wrappers only used by tests
and functions that were never called. Rules now prohibit both patterns.

---

Change 6: Test coverage and real-world fixtures

File: AGENTS.md (Code conventions section)

Pattern: test_gap — 11 comments (4 required_change, 7 suggestion)

Evidence:

"Add unit tests for this method, at least with the 5 basic flow paths"
— @jparrill, PR #7943

"A real example would be helpful, maybe use quay.io/openshift-release-dev/ocp-release:4.21.10-x86_64"
— @bryan-cox, PR #8211

Reasoning: AI-generated tests used synthetic fixtures and missed key
paths. Rules now require real-world values and minimum coverage paths.

Test Plan

• Verify AGENTS.md parses correctly (no broken markdown)
• Next jira-solve runs should show fewer comments in the addressed categories
• Compare comment counts week-over-week in the dashboard

🤖 Generated with Claude Code via improve-ai-quality skill

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.

[openshift-ci]

Please specify an area label

Details

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 kubernetes-sigs/prow repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bryan-cox

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [bryan-cox]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 3e81165c-5cf6-44e2-9a61-0910dd0f81ad

📥 Commits

Reviewing files that changed from the base of the PR and between f13c62d and c53a118.

📒 Files selected for processing (1)

• AGENTS.md

---

📝 Walkthrough

Walkthrough

This PR adds developer guidance to AGENTS.md covering operational and code quality practices. Three new "Common Gotchas" bullets document NodePool config-hash-driven fleet rollout impact, the importance of keeping PRs focused, and adherence to established patterns for certificate and TLS handling. Six new "Code conventions" bullets address validation regex maintenance, stricter character set matching aligned with upstream formats, avoiding exported test-only helpers, removing dead code, using real-world fixture values, ensuring unit test coverage for newly exported functions/methods, and verifying OwnerReference assertions during reconciliation.

Suggested reviewers

• devguyio
• clebs

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
Stable And Deterministic Test Names	❌ Error	test/envtest/generator.go contains a Ginkgo test with a dynamic name: It(fmt.Sprintf("should install all CRDs for feature set %q", featureSet), ...). Test names must be static and deterministic.	Replace fmt.Sprintf with a static test name, e.g., It("should install all CRDs for the feature set", ...) and include the featureSet value only in the test body for assertions.

✅ Passed checks (10 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: updating AGENTS.md with guidance based on code review analysis to improve AI agent quality.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Test Structure And Quality	✅ Passed	PR only modifies AGENTS.md documentation (+10/-0); no Ginkgo test files are changed, making the test structure check not applicable to this pull request.
Topology-Aware Scheduling Compatibility	✅ Passed	PR only updates AGENTS.md documentation guidance; no deployment manifests, operator code, or controllers are added or modified. Check not applicable.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	PR adds documentation to AGENTS.md file only; no new Ginkgo e2e tests (It(), Describe(), etc.) are added. The custom check applies only to new test additions and is not applicable here.
No-Weak-Crypto	✅ Passed	PR adds documentation and config files only. No weak cryptographic algorithms, custom crypto implementations, or insecure secret comparisons detected in AGENTS.md or other files added.
Container-Privileges	✅ Passed	PR updates AGENTS.md documentation. Privileged containers in K8s manifests have legitimate justifications: DaemonSets for kernel config, CSI controllers for storage, E2E test fixtures.
No-Sensitive-Data-In-Logs	✅ Passed	PR adds documentation and API type definitions. No new logging code exposes passwords, tokens, API keys, PII, session IDs, or customer data.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 41.43%. Comparing base (f13c62d) to head (c53a118).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8690   +/-   ##
=======================================
  Coverage   41.43%   41.43%           
=======================================
  Files         756      756           
  Lines       93647    93647           
=======================================
  Hits        38802    38802           
  Misses      52124    52124           
  Partials     2721     2721           

Flag	Coverage Δ
cmd-support	34.87% <ø> (ø)
cpo-hostedcontrolplane	43.50% <ø> (ø)
cpo-other	42.74% <ø> (ø)
hypershift-operator	51.57% <ø> (ø)
other	31.64% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.