no-jira: improve AI agent readiness with guidelines and AGENTS.md#10474
Conversation
|
Skipping CI for Draft Pull Request. |
|
@rochacbruno: This pull request explicitly references no jira issue. 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. |
|
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:
Note
|
| Layer / File(s) | Summary |
|---|---|
CodeRabbit configuration .coderabbit.yaml |
New repository configuration for CodeRabbit: language/tone, review workflow, branch auto-review rules, path filters, per-path review guidance, integrated scanners/linters, and chat auto-replies. |
AI agent guidance AGENTS.md |
New agents guide consolidating repository conventions: Go style, import ordering, generated-code rules, asset DAG architecture, provider/destroy/CAPI wiring, build-tag/vendoring practices, and contribution norms. |
Top-level docs consolidation CLAUDE.md |
Replaces expanded top-level guidance with a single include/reference to AGENTS.md, removing duplicated architecture and project overview content. |
Guidelines — integration docs/integration-guidelines.md |
Adds platform integration conventions: file placement, defaults/validation patterns, destroyer registration (init()), CAPI provider hooks/registration, metadata wiring, resource tagging, and AWS/GCP-specific patterns. |
Guidelines — error handling docs/error-handling-guidelines.md |
Adds error creation/wrapping/propagation rules, validation error patterns (field.ErrorList), asset error propagation conventions, sentinel messages, and platform-specific error-handling guidance. |
Guidelines — performance docs/performance-guidelines.md |
Adds concurrency, retry, ordering, and synchronization patterns: platform-specific teardown models, semaphores/worker pools, retry/backoff configs, logging/thread-safety, and context/timeout conventions. |
Guidelines — security docs/security-guidelines.md |
Adds security conventions: TLS/certificate rules, crypto randomness, credential generation/storage, FIPS behaviors, SSH/pull-secret handling, file permission guidance, and gosec annotation practices. |
Guidelines — testing docs/testing-guidelines.md |
Adds testing conventions: unit/integration test patterns, mocking/regeneration with gomock, testscript usage, test layout, assertion preferences, and shared test helpers/templates. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 15
✅ Passed checks (15 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main change: adding AI agent readiness documentation and configuration (AGENTS.md, guidelines, .coderabbit.yaml). |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| 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. |
| Stable And Deterministic Test Names | ✅ Passed | Repository uses standard Go table-driven tests with t.Run(), not Ginkgo. No It()/Describe()/Context()/When() patterns found. Ginkgo-specific check is not applicable. |
| Test Structure And Quality | ✅ Passed | PR contains only documentation and configuration files (.md and .yaml files); no Ginkgo test code is present, making the check inapplicable. |
| Microshift Test Compatibility | ✅ Passed | This PR contains only documentation and configuration files (8 files: .coderabbit.yaml and 7 Markdown guideline docs). No Ginkgo e2e tests are added, making this custom check not applicable. |
| Single Node Openshift (Sno) Test Compatibility | ✅ Passed | No Ginkgo e2e tests are added in this PR; only documentation and configuration files are modified. The check is not applicable. |
| Topology-Aware Scheduling Compatibility | ✅ Passed | The PR adds only documentation and configuration files. The check applies exclusively to deployment manifests, operator code, and controllers—none of which are present. |
| Ote Binary Stdout Contract | ✅ Passed | The PR correctly handles OTE stdout contract: main.go sets klog.SetOutput(io.Discard) at start of main(), preventing klog output corruption. No fmt.Print calls or problematic init() code found. |
| Ipv6 And Disconnected Network Test Compatibility | ✅ Passed | PR adds only documentation and configuration files. No Ginkgo e2e tests are added, so the IPv6/disconnected network check is not applicable. |
| No-Weak-Crypto | ✅ Passed | No weak crypto (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB) found. PR adds documentation/config only. Recommends RSA-2048, ECDSA P-256, bcrypt, crypto/rand. |
| Container-Privileges | ✅ Passed | PR contains only documentation and configuration files; no Kubernetes/container manifests with privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation settings found. |
| No-Sensitive-Data-In-Logs | ✅ Passed | PR contains documentation and configuration files only. No logging code exposes passwords, tokens, API keys, or PII. Security guidelines document proper credential handling. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Comment @coderabbitai help to get the list of available commands and usage tips.
b345bd2 to
eac5fb9
Compare
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 42: Update the phrase "Platform type packages" to the hyphenated form
"platform-type packages" in the AGENTS.md sentence that reads "Every package
should have a `doc.go` file with a package comment. Platform type packages also
define their platform `Name` constant in `doc.go`" so it becomes "platform-type
packages also define their platform `Name` constant in `doc.go`" to improve
correctness and readability.
- Around line 143-149: The fenced code block showing "<subsystem>: <what
changed> ... Fixes #<issue-number>" needs a language hint to satisfy MD040 and
improve rendering; update that fenced block (the one containing "<subsystem>:
<what changed>" at the noted snippet) to use a language tag such as "text"
(i.e., change the opening ``` to ```text) so the markdown linter and renderers
recognize the block type.
🪄 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: Pro Plus
Run ID: 9f77b501-cfdb-4892-9569-d862135c6d77
📒 Files selected for processing (3)
.coderabbit.yamlAGENTS.mdCLAUDE.md
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- .coderabbit.yaml
barbacbd
left a comment
There was a problem hiding this comment.
/approve
Concept is good. Coderabbit only had very minor updates that I don't think are 100% necessary. But you need to address the failing tests.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: barbacbd 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 |
Add layered documentation system for AI-assisted development: - docs/security-guidelines.md — TLS, credentials, FIPS, secrets - docs/error-handling-guidelines.md — Error wrapping, validation, cloud errors - docs/testing-guidelines.md — Mocks, table-driven tests, integration tests - docs/integration-guidelines.md — Cloud platform structure, CAPI, tagging, SDKs - docs/performance-guidelines.md — Concurrency, rate limiting, destroy flows - AGENTS.md — Cross-cutting conventions, architecture, docs index - CLAUDE.md — Updated to import AGENTS.md, removed duplicated content - .coderabbit.yaml — Added code_guidelines.filePatterns for guideline enforcement Assessment: | Requirement | Before | After | |----------------------------------------------------|--------|-------| | Domain-specific guideline files (docs/) | ❌ | ✅ | | AGENTS.md (agent onboarding + docs index) | ❌ | ✅ | | CLAUDE.md (Claude-specific config + imports) | ✅ (no import) | ✅ | | CodeRabbit configured to enforce guidelines | ❌ | ✅ | | README.md (project overview + getting started) | ✅ | ✅ | | CONTRIBUTING.md (contribution workflow) | ✅ | ✅ | | docs/ARCHITECTURE.md (design decisions + context) | ❌ | ❌ | Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4a61c3e to
955d169
Compare
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 `@docs/error-handling-guidelines.md`:
- Around line 55-57: The fenced code block showing the error example is missing
a language tag; update that fenced block so it uses a language identifier of
"text" for the example containing failed to generate asset "Cluster" -> failed
to fetch dependency of "Cluster" -> ... (the fenced block in
docs/error-handling-guidelines.md), i.e., change the triple-backtick fence to
```text and leave the inner content unchanged so it satisfies the MD040 linter
rule.
In `@docs/testing-guidelines.md`:
- Around line 156-170: The fenced code block in docs/testing-guidelines.md
currently uses an unannotated triple-backtick which triggers markdownlint MD040;
update the example fence around the testscript (the block starting with the
comment "# Comment describing the test" and the lines showing exec
openshift-install ... and the install-config.yaml/agent-config.yaml snippets) to
include a language identifier (e.g., change ``` to ```txt) so the block is
annotated and MD040 is satisfied.
🪄 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: e26e1e5e-d5d1-475f-be18-100f6e833b0c
📒 Files selected for processing (8)
.coderabbit.yamlAGENTS.mdCLAUDE.mddocs/error-handling-guidelines.mddocs/integration-guidelines.mddocs/performance-guidelines.mddocs/security-guidelines.mddocs/testing-guidelines.md
✅ Files skipped from review due to trivial changes (4)
- docs/security-guidelines.md
- docs/integration-guidelines.md
- AGENTS.md
- docs/performance-guidelines.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .coderabbit.yaml
- CLAUDE.md
Hyphenate compound modifier "Platform-type packages" and add `text` language hint to commit format fenced code block (MD040). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
955d169 to
fda325a
Compare
|
/retest-required |
|
/test okd-scos-images |
tthvo
left a comment
There was a problem hiding this comment.
Nice, looks really good! I just have a few comments, mostly for coderrabit config file.
| ## TLS Certificates | ||
|
|
||
| ### Key size and algorithm | ||
| All RSA keys use 2048-bit via `rsa.GenerateKey(rand.Reader, 2048)` in `pkg/asset/tls/tls.go`. The constant `keySize = 2048` is the single source of truth. Do not introduce alternative key sizes without updating this constant. |
- .coderabbit.yaml: add inheritance from org config, release-5.* branch, zz_generated exclusion, expanded knowledge base patterns, remove incomplete suggested_labels - error-handling: prefer fmt.Errorf over deprecated pkg/errors, clarify graceful degradation is exception-only - integration: add wait period annotations to hook order, improve AWS tagging description, remove altinfra references per openshift#10585 - performance: clarify resource ordering is platform-specific - security: clarify credential file examples - testing: remove Go version from table, add \Q/\E literal matching - AGENTS.md: narrow doc.go to pkg/types/, remove altinfra references Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/verified bypass These are mostly AI agent guidelines. Since there is a new repo-wide coderrabit config (affecting all PR reviews), I hold for Patrick to green light it. |
|
@tthvo: 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. |
|
/hold cancel Thanks @rochacbruno I do hope this is something we continue to evolve. I think it is debated how effective these readmes are, I think the CLAUDE.md used to write claude code gets passed around and it is super simple (but i can't find the reference right now so maybe my memory is faulty). On the other hand, Boris Cherny recommends how effective it is to tell claude to stop making mistakes. So let's keep thinking about what's effective for us. |
5e91bda
into
openshift:main
Add layered documentation system for AI-assisted development:
Assessment:
Summary by CodeRabbit
Documentation
Chores