Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions .claude/commands/api-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
---
name: api-review
description: Run strict OpenShift API review workflow for PR changes
parameters:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this parameters block coming from? I can only see $N args convention in claude docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude generated most of this document itself based on guidance we were giving it through the CLI, I'm pretty certain it wrote this out

Copy link
Contributor

@theobarberbany theobarberbany Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See https://docs.claude.com/en/docs/claude-code/slash-commands#parameters

We're just naming, and explicitly requiring them here, I think..the docs aren't too clear

- name: pr_url
description: GitHub PR URL to review
required: true
---

# Output Format Requirements
You MUST use this EXACT format for ALL review feedback:


+LineNumber: Brief description
**Current (problematic) code:**
```go
[exact code from the PR diff]
```

**Suggested change:**
```diff
- [old code line]
+ [new code line]
```

**Explanation:** [Why this change is needed]


I'll run a comprehensive API review for the OpenShift API changes in the specified GitHub PR.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like something an LLM would spit out when you ask for a review - is it necessary to include this in the instructions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this whole section looks like it is something an LLM spit out as an execution plan. Should something like this be hand-rolled with explicit instructions on how to conduct the review and important considerations?

I guess my curiosity here is if we made an explicit guidelines type document that humans could follow, an LLM should be able to follow along relatively easily and we can potentially enforce more nuanced guardrails.

Not worth blocking this on - more so asking questions for myself as I've not worked with LLMs in this capacity before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an artefact of how these documents are built. Using Claude locally, I've given it text based prompts, and it has converted that into instructions that it can read. So 95% of this document is it translating my instructions and feedback into rules that it can later apply.

Copy link
Contributor

@everettraven everettraven Sep 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. That is interesting. So this file is automatically updated with more detailed instructions by Claude as the AGENTS.md file is updated?

If you update this file by hand to "improve it", what happens?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can improve it by hand, that's also fine and I have made a few edits here and there. But the bulk was generated, (then pruned - it was more verbose), and tested again to see if it was producing good results.

Copy link
Contributor

@theobarberbany theobarberbany Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to include this in the instructions?

I've found giving explicit examples, when you want a particular format of output to work quite well. Unfortunately, it seems best with monkey see monkey do style learning in this department :(

I guess my curiosity here is if we made an explicit guidelines type document that humans could follow, an LLM should be able to follow along relatively easily and we can potentially enforce more nuanced guardrails.

I think this should work, and is sort of what the section ### API review in AGENTS.md is, but you would still need something similar to this document for the command to give it a guide on how you want the output, and communication style alongside extra prompts for when it fails to follow the rules...

If you update this file by hand to "improve it", what happens?

I'be found that often, what makes sense to me (as a human, reading the document), the AI will ignore / may degrade performance. Conversing with the model and asking it what seems to be the issue (why didn't you listen to my instructions?) does seem to help - but yeilds different structures to what we'd normally expect e.g for documentation to be consumed by humans.

This does lead into one of the bigger problems with automating problems like this, which is how do you ensure consistent output, and avoid regressions when experimenting with prompts? You've got a probabilistic system you need to build confidence in, and don't want to rely on manual checks or 'vibes'. evals are the way the industry seems to be moving, but the OSS tooling all seems pretty heavyweight for automating internal tasks / reviews.

One approach for API review that may work could be directing the command (or a version of it) to output json:

{
  "summary": "",
  "issues": [
    {"rule": "fieldDocumentation",  "msg": "...", "lines": ""},
    {"rule": "optionalFieldBehaviour", "msg": "...", "lines": ""}
  ],
  "meta": {"model": "claude-xxx",  "rules_version": "<commit?>"}
}

Having sets of real API PRs, where we can write units that Expect the correct issues to be caught. Something like:

golden/: real API doc chunks + your expected issue set (ground truth).
synthetic/: synthetic snippets each targeting exactly one rule. These are your 'unit tests' .

Then you can compare existing rules/prompt to changed, and hopefully catch any changes? There are lots of ways to extend this too e.g mutating synthetic snippets, and still expecting issues to be caught, or using existing reviews (e.g merged PRs with comment chains + changes requested) and an llm as a judge style system.

Either way, its not simple... unfortunately :(


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to create a separate H1 heading to explain to the LLM that this is the steps for how to actually conduct the review?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weirdly, I'll run a comprehensive API review for the OpenShift API changes in the specified GitHub PR. seems to work just fine. I've had instances (e.g on ccapio) where adding headings lead to the agent ignoring them 🤦

It's the sort of thing you want to build something to allow you to test :/

## Step 1: Pre-flight checks and branch management

First, I'll check for uncommitted changes and save the current branch:

```bash
# Save current branch
CURRENT_BRANCH=$(git branch --show-current)
echo "📍 Current branch: $CURRENT_BRANCH"

# Check for uncommitted changes
if ! git diff --quiet || ! git diff --cached --quiet; then
echo "❌ ERROR: Uncommitted changes detected. Cannot proceed with API review."
echo "Please commit or stash your changes before running the API review."
git status --porcelain
exit 1
fi

echo "✅ No uncommitted changes detected. Safe to proceed."
```

## Step 2: Extract PR number and checkout PR

```bash
# Extract PR number from URL
PR_NUMBER=$(echo "$ARGUMENTS" | grep -oE '[0-9]+$')
echo "🔍 Reviewing PR #$PR_NUMBER"

# Checkout the PR
echo "🔄 Checking out PR #$PR_NUMBER..."
gh pr checkout "$PR_NUMBER"

# Get changed Go files in API directories
echo "📁 Analyzing changed files..."
gh pr view "$PR_NUMBER" --json files --jq '.files[].path' | grep '\.go$' | grep -E '/(v1|v1alpha1|v1beta1)/'
```

## Step 3: Run linting checks on PR changes

```bash
echo "⏳ Running linting checks on PR changes..."
make lint
```

## Step 4: Documentation validation

For each changed API file, I'll validate:

1. **Field Documentation**: All struct fields must have documentation comments
2. **Optional Field Behavior**: Optional fields must explain what happens when they are omitted
3. **Validation Documentation**: Validation rules must be documented and match markers

Let me check each changed file for these requirements:

```thinking
I need to analyze the changed files to:
1. Find struct fields without documentation
2. Find optional fields without behavior documentation
3. Find validation annotations without corresponding documentation
For each Go file, I'll:
- Look for struct field definitions
- Check if they have preceding comment documentation
- For optional fields (those with `+kubebuilder:validation:Optional` or `+optional`), verify behavior is explained
- For fields with validation annotations, ensure the validation is documented
```

## Step 5: Generate comprehensive review report

I'll provide a comprehensive report showing:
- ✅ Files that pass all checks
- ❌ Files with documentation issues
- 📋 Specific lines that need attention
- 📚 Guidance on fixing any issues

The review will fail if any documentation requirements are not met for the changed files.

## Step 6: Switch back to original branch

After completing the review, I'll switch back to the original branch:

```bash
echo "🔄 Switching back to original branch: $CURRENT_BRANCH"
git checkout "$CURRENT_BRANCH"
echo "✅ API review complete. Back on branch: $(git branch --show-current)"
```

**CRITICAL WORKFLOW REQUIREMENTS:**
1. MUST check for uncommitted changes before starting
2. MUST abort if uncommitted changes are detected
3. MUST save current branch name before switching
4. MUST checkout the PR before running `make lint`
5. MUST switch back to original branch when complete
6. If any step fails, MUST attempt to switch back to original branch before exiting
148 changes: 148 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
This file provides guidance to AI agents when working with code in this repository.

This is the OpenShift API repository - the canonical location of OpenShift API type definitions and serialization code. It contains:

- API type definitions for OpenShift-specific resources (Custom Resource Definitions)
- FeatureGate management system for controlling API availability across cluster profiles
- Generated CRD manifests and validation schemas
- Integration test suite for API validation

## Key Architecture Components

### FeatureGate System
The FeatureGate system (`features/features.go`) controls API availability across different cluster profiles (Hypershift, SelfManaged) and feature sets (Default, TechPreview, DevPreview). Each API feature is gated behind a FeatureGate that can be enabled/disabled per cluster profile and feature set.

### API Structure
APIs are organized by group and version (e.g., `route/v1`, `config/v1`). Each API group contains:
- `types.go` - Go type definitions
- `zz_generated.*` files - Generated code (deepcopy, CRDs, etc.)
- `tests/` directories - Integration test definitions
- CRD manifest files

## Common Development Commands

### Building
```bash
make build # Build render and write-available-featuresets binaries
make clean # Clean build artifacts
```

### Code Generation
```bash
make update # Alias for update-codegen-crds
```

### Testing
```bash
make test-unit # Run unit tests
make integration # Run integration tests (in tests/ directory)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we teach it how to run the integration tests with more focused arguments?

That way running the integration tests don't take longer than necessary when reviewing a change that only impacts a subset of the APIs/tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a good idea, I'll have a go at that

go test -v ./... # Run tests for specific packages

# Run integration tests for specific API groups
make -C config/v1 test # Run tests for config/v1 API group
make -C route/v1 test # Run tests for route/v1 API group
make -C operator/v1 test # Run tests for operator/v1 API group
```

### Validation and Verification
```bash
make verify # Run all verification checks
make verify-scripts # Verify generated code is up to date
make verify-codegen-crds # Verify CRD generation is current
make lint # Run golangci-lint (only on changes from master)
make lint-fix # Auto-fix linting issues where possible
```

## Adding New APIs

All APIs should start as tech preview.
New fields on stable APIs should be introduced behind a feature gate `+openshift:enable:FeatureGate=MyFeatureGate`.


### For New Stable APIs (v1)
1. Create the API type with proper kubebuilder annotations
2. Include required markers like `+openshift:compatibility-gen:level=1`
3. Add validation tests in `<group>/<version>/tests/<crd-name>/`
4. Run `make update-codegen-crds` to generate CRDs

### For New TechPreview APIs (v1alpha1)
1. First add a FeatureGate in `features/features.go`
2. Create the API type with `+openshift:enable:FeatureGate=MyFeatureGate`
3. Add corresponding test files
4. Run generation commands

### Adding FeatureGates
Add to `features/features.go` using the builder pattern:
```go
FeatureGateMyFeatureName = newFeatureGate("MyFeatureName").
reportProblemsToJiraComponent("my-jira-component").
contactPerson("my-team-lead").
productScope(ocpSpecific).
enableIn(configv1.TechPreviewNoUpgrade).
mustRegister()
```

## Testing Framework

The repository includes a comprehensive integration test suite in `tests/`. Test suites are defined in `*.testsuite.yaml` files alongside API definitions and support:
- `onCreate` tests for validation during resource creation
- `onUpdate` tests for update-specific validations and immutability
- Status subresource testing
- Validation ratcheting tests using `initialCRDPatches`

Use `tests/hack/gen-minimal-test.sh $FOLDER $VERSION` to generate test suite templates.

## Container-based Development
```bash
make verify-with-container # Run verification in container
make generate-with-container # Run code generation in container
```

Uses `podman` by default, set `RUNTIME=docker` or `USE_DOCKER=1` to use Docker instead.

## Custom Claude Code Commands

### API Review
```
/api-review <pr-url>
```
Runs comprehensive API review for OpenShift API changes in a GitHub PR:
- Executes `make lint` to check for kube-api-linter issues
- Validates that all API fields are properly documented
- Ensures optional fields explain behavior when not present
- Confirms validation rules and kubebuilder markers are documented in field comments

#### Documentation Requirements
All kubebuilder validation markers must be documented in the field's comment. For example:

**Good:**
```go
// internalDNSRecords is an optional field that determines whether we deploy
// with internal records enabled for api, api-int, and ingress.
// Valid values are "Enabled" and "Disabled".
// When set to Enabled, in cluster DNS resolution will be enabled for the api, api-int, and ingress endpoints.
// When set to Disabled, in cluster DNS resolution will be disabled and an external DNS solution must be provided for these endpoints.
// +optional
// +kubebuilder:validation:Enum=Enabled;Disabled
InternalDNSRecords InternalDNSRecordsType `json:"internalDNSRecords"`
```

**Bad:**
```go
// internalDNSRecords determines whether we deploy with internal records enabled for
// api, api-int, and ingress.
// +optional // ❌ Optional nature not documented in comment
// +kubebuilder:validation:Enum=Enabled;Disabled // ❌ Valid values not documented
InternalDNSRecords InternalDNSRecordsType `json:"internalDNSRecords"`
```

The comment must explicitly state:
- When a field is optional (for `+kubebuilder:validation:Optional` or `+optional`)
- Valid enum values (for `+kubebuilder:validation:Enum`)
- Validation constraints (for min/max, patterns, etc.)
- Default behavior when field is omitted
- Any interactions with other fields, commonly implemented with `+kubebuilder:validation:XValidation`

**CRITICAL**: When API documentation states field relationships or constraints (e.g., "cannot be used together with field X", "mutually exclusive with field Y"), these relationships MUST be enforced with appropriate validation rules. Use `+kubebuilder:validation:XValidation` with CEL expressions for cross-field constraints. Documentation without enforcement is insufficient and will fail review.

Example: `/api-review https://github.com/openshift/api/pull/1234`
1 change: 1 addition & 0 deletions CLAUDE.md