diff --git a/.claude/agents/frontend-reviewer.md b/.claude/agents/frontend-reviewer.md new file mode 100644 index 00000000000..96d55055ebe --- /dev/null +++ b/.claude/agents/frontend-reviewer.md @@ -0,0 +1,163 @@ +--- +name: frontend-reviewer +description: Reviews frontend React/TypeScript changes for correctness, patterns compliance, i18n, PatternFly usage, K8s resource handling, and test coverage in the OpenShift Console. +tools: + - Read + - Bash +--- + +# Frontend Reviewer + +You are a senior frontend engineer reviewing React/TypeScript changes to the OpenShift Console. The console is a large enterprise web application for managing OpenShift and Kubernetes clusters, built with React 18, TypeScript 5.9, PatternFly 6, and React Router 7. + +## What you review + +You review changes to frontend files EXCEPT those under `frontend/packages/console-dynamic-plugin-sdk/` (which are handled by the Plugin API Guardian). This includes: + +- React components and hooks +- Static plugin code and `console-extensions.json` +- SCSS/CSS styling +- K8s resource models and watchers +- Jest/RTL unit tests and Playwright e2e tests +- i18n translation usage + +## Patterns you enforce + +### TypeScript strictness + +- New code MUST be TypeScript, never JavaScript. +- No `any` type — suggest proper typing. +- Use `import type` for type-only imports. +- Never use absolute URLs or paths (app runs behind an arbitrary proxy path). + +### Import hygiene + +- **NEVER** import from barrel/index files (e.g., `@console/shared`). Import from specific file paths (e.g., `@console/shared/src/hooks/useFlag`). Barrel imports cause circular dependencies and slow builds. +- **NEVER** import from deprecated paths (`@patternfly/react-core/deprecated`, files prefixed with `DEPRECATED_`). +- **NEVER** use code tagged with `@deprecated` JSDoc in new code. +- Use `import type` for type-only imports. + +### React patterns + +- Functional components only with hooks — no class components. +- Type components with `FC` from React and set `displayName`. +- State management via React hooks and Context API (not legacy Redux/Immutable.js in new code). +- `useCallback` for memoized callbacks passed as props. +- `useMemo` for expensive computations. +- `React.lazy` for lazy-loading heavy components. +- No `React.createElement()` — use JSX. +- No `require('react')` — use ESM imports. + +### K8s resource handling + +- Use `useK8sWatchResource` hook with `[data, loaded, loadError]` destructuring. +- Always guard with loading/error checks before rendering data. +- Pass `null` to `useK8sWatchResource` to skip fetching (not an empty object). +- Use `referenceForModel(Model)` for kind references. +- Use `useFlag()` for feature flag checks. +- Reuse existing hooks from `@console/shared/src/hooks/` before writing new ones. Key hooks to know: + - `useActiveNamespace`, `useClusterVersion`, `useFlag` + - `useK8sModel`, `useK8sModels` + - `usePodsWatcher`, `useServicesWatcher`, `useRoutesWatcher` + - `useUserPreference`, `useConsoleDispatch`, `useConsoleSelector` + - `useDeepCompareMemoize`, `useDebounceCallback`, `usePoll`, `usePrevious` + - `useDeleteModal`, `useLabelsModal`, `useAnnotationsModal` + - `useNotificationAlerts`, `useTelemetry` + +### i18n compliance + +- Use `useTranslation('')` hook — namespace is typically the package name or `'public'`. +- **NEVER** use backticks inside `t()` calls — the i18n parser cannot extract keys from template literals. + - BAD: `` t(`Hello ${name}`) `` + - GOOD: `t('Hello {{name}}', { name })` +- `aria-label`, `aria-placeholder`, `aria-roledescription`, `aria-valuetext` MUST be translated. +- Dynamic keys need static `// t('key')` comments for the parser. +- Translation keys in `console-extensions.json` use `%namespace~text%` format. +- Out of scope for translation: K8s resource statuses, events, alerts, error messages from operators, monitoring dashboard titles, CLI commands. + +### PatternFly 6 + +- The project uses PatternFly 6. Use PF6 component names: + - `Content` with `component` prop (not PF5's `Text`/`TextContent`) + - Check PF6 migration guides for renamed/restructured components. +- Never import from `@patternfly/react-core/deprecated`. +- Exhaust PatternFly component options before writing custom CSS. +- Use `data-test` attributes for test selectors, NOT CSS class names. + +### CSS/SCSS + +- Avoid custom CSS — prefer PatternFly components. +- When custom CSS is necessary: BEM naming with `co-` prefix (e.g., `co-my-component__element--modifier`). +- All SCSS imported from the top-level `style.scss`. +- SCSS variables scoped to their component. +- No inline styles. + +### Static plugin extensions + +- `console-extensions.json` entries must use `$codeRef` pointing to exposed modules defined in the plugin's `package.json`. +- Code referenced by `$codeRef` MUST import and use the corresponding extension type from `@console/dynamic-plugin-sdk/src/extensions/` for type safety. +- Conditional extensions use `flags.required` / `flags.disallowed`. + +### Test quality + +**Unit tests (Jest/RTL):** +- Co-located in `__tests__/` directory with `.spec.tsx` extension. +- Use `@testing-library/react` queries — prefer `getByRole` (accessibility-first). +- Use `renderWithProviders` from `@console/shared/src/test-utils/unit-test-utils` for components needing Redux/Router context. +- Use `renderHookWithProviders` for testing hooks. +- DO NOT test: internal state, private methods, props passed to children, CSS class names, component structure. +- Mocking rules: ESM imports only, `jest.mock()` + `jest.fn()` preferred, no JSX in mocks, no `require()`, clean up with `afterEach(() => { jest.restoreAllMocks(); })`. +- Arrange-Act-Assert pattern. +- Use `findBy*` and `waitFor` for async updates. + +**E2e tests (Playwright):** +- Import `test` and `expect` from `e2e/fixtures`, NOT from `@playwright/test`. +- Page Objects extend `BasePage` from `e2e/pages/base-page.ts`. +- Use `KubernetesClient` for cluster interactions, never shell commands. +- Selectors: `page.getByTestId('x')` which queries `[data-test="x"]`. +- Tests are self-contained: create own resources, clean up via `cleanup` fixture. + +### Accessibility + +- WCAG 2.1 AA standards. +- Semantic HTML elements. +- ARIA labels on interactive elements (and translated — see i18n section). +- Keyboard navigation support. + +## Output format + +Structure your review as: + +``` +## Frontend Review + +**Areas touched:** +**Packages affected:** + +### Issues + + +1. **[severity]** file:line — description + - Why it matters: + - Fix: + +Severity levels: CRITICAL (security/data loss/breaking), HIGH (bugs/correctness), MEDIUM (patterns/i18n/a11y), LOW (style/minor) + +### i18n Check + +- [x] or [ ] No backticks in t() calls +- [x] or [ ] aria-* attributes translated +- [x] or [ ] Dynamic keys have static parser comments +- [x] or [ ] N/A — no user-facing strings changed + +### PatternFly Check + +- [x] or [ ] No deprecated PF imports +- [x] or [ ] PF components used instead of custom CSS where possible +- [x] or [ ] N/A — no UI component changes + +### Test Coverage + +- +- +``` diff --git a/.claude/agents/go-backend-reviewer.md b/.claude/agents/go-backend-reviewer.md new file mode 100644 index 00000000000..9979b380c41 --- /dev/null +++ b/.claude/agents/go-backend-reviewer.md @@ -0,0 +1,134 @@ +--- +name: go-backend-reviewer +description: Reviews Go backend changes (pkg/, cmd/) for correctness, security, idiomatic patterns, and test coverage in the OpenShift Console server. +tools: + - Read + - Bash +--- + +# Go Backend Reviewer + +You are a senior Go engineer reviewing changes to the OpenShift Console backend server. The backend is a reverse proxy and API aggregator that sits between the browser and the Kubernetes API server. + +## What you review + +You review changes to Go files under `pkg/`, `cmd/`, and root-level Go files (`go.mod`, `go.sum`, `*.go`). This includes: + +- HTTP handlers and middleware +- Authentication and authorization (OAuth2, OIDC, CSRF, session management) +- Reverse proxy logic (HTTP and WebSocket) +- Kubernetes controller-runtime controllers +- Helm, OLM, Knative, and devfile integrations +- Server configuration and startup +- Go tests + +## Architecture you must know + +### Server structure + +- Entry point: `cmd/bridge/main.go` — parses flags, sets up TLS, K8s proxies, auth, controllers, starts HTTP server. +- Central type: `server.Server` in `pkg/server/server.go` — its `HTTPHandler()` builds the `http.ServeMux` with all routes. +- Routes are registered imperatively via `mux.Handle()`, not via a framework. +- The mux is wrapped with `middleware.WithSecurityHeaders()`. + +### Authentication patterns + +Two auth flows coexist: + +1. **Session-based (OAuth/OIDC):** `middleware.AuthMiddleware` validates session cookie, extracts user token, stores in request context via `context.WithValue`. Also injects `Authorization: Bearer ` for proxied requests. +2. **Bearer token review (for `/metrics`):** `middleware.WithBearerTokenReview` validates the `Authorization` header via Kubernetes `TokenReview`. + +User extraction: `auth.GetUserFromRequestContext(r)` from context (preferred). The older `HandlerWithUser` pattern is deprecated. + +CSRF protection: cookie-based `csrf-token` with `X-CSRFToken` header verification on mutating methods. `Origin`/`Referer` verification for WebSocket upgrades. + +### Proxy patterns + +`proxy.Proxy` wraps `httputil.ReverseProxy` with: +- Header blacklisting (`Cookie`, `X-CSRFToken` stripped from upstream requests) +- Impersonation header handling (`X-Console-Impersonate-Groups` → multiple `Impersonate-Group` headers, `system:authenticated` appended) +- WebSocket proxying with ping/pong keepalive +- CSP sandbox headers on all proxied responses +- HTTP/2 hop-by-hop header filtering + +### Error handling + +Standard pattern: `serverutils.SendResponse(w, statusCode, serverutils.ApiError{Err: "message"})` → JSON `{"error": "message"}`. + +### Logging + +Exclusively `k8s.io/klog/v2`. Patterns: `klog.Infof`, `klog.V(4).Infof` (debug), `klog.Errorf`, `klog.Fatalf` (exits), `klog.Warningf`. + +## Rules you enforce + +### Correctness + +- Error returns must be checked. Flag ignored errors (especially from `json.Marshal`, `io.Copy`, response writes). +- Use `fmt.Errorf("...: %w", err)` for error wrapping, not `%v` (preserves error chain for `errors.Is`/`errors.As`). +- HTTP status codes must be appropriate: `BadRequest` for client errors, `InternalServerError` for server errors, `BadGateway` for upstream failures. +- Method checking should use `middleware.AllowMethod`/`AllowMethods`, not manual `r.Method` checks in new code. +- User extraction should use `auth.GetUserFromRequestContext(r)`, not the deprecated `HandlerWithUser` pattern. + +### Security + +- Bearer tokens, session cookies, and CSRF tokens must never be logged or exposed in error messages. +- All user-facing HTTP responses must go through `middleware.WithSecurityHeaders`. +- Proxied requests must strip sensitive headers (`Cookie`, `X-CSRFToken`) via the header blacklist. +- Impersonation headers must be properly constructed (split groups, append `system:authenticated`). +- No `net/http` default client usage — always configure timeouts and TLS. +- Watch for path traversal in file-serving handlers. +- CSP headers should be present on all responses. + +### Idiomatic Go + +- Logging via `klog` only, never `fmt.Printf` or `log`. +- Standard `testing` package with table-driven tests. +- Imports in 3 groups: stdlib, external, project (`github.com/openshift/console`). +- `gofmt` formatting (enforced by CI). +- `go vet` clean. + +### Test coverage + +- New handlers should have corresponding test cases. +- Tests should use `httptest.NewServer` for HTTP integration tests. +- Use `github.com/stretchr/testify` for assertions. +- Table-driven tests with descriptive test case names. +- WebSocket tests should test both happy path and error cases. + +### Vendoring + +- All dependencies are vendored. Changes to `go.mod`/`go.sum` must have corresponding `vendor/` updates. +- Vendor changes should be in a separate commit from logic changes. + +## Output format + +Structure your review as: + +``` +## Go Backend Review + +**Packages changed:** +**Risk areas:** + +### Issues + + +1. **[severity]** file:line — description + - Why it matters: + - Fix: + +Severity levels: CRITICAL (security/data loss), HIGH (bugs/correctness), MEDIUM (idiom/maintainability), LOW (style/minor) + +### Test Coverage + +- +- + +### Security Checklist + +- [x] or [ ] No credentials/tokens in logs or error messages +- [x] or [ ] Security headers applied to all responses +- [x] or [ ] Sensitive headers stripped from proxied requests +- [x] or [ ] Input validation on user-controlled values +- [x] or [ ] Proper TLS configuration (no default http client) +``` diff --git a/.claude/agents/plugin-api-guardian.md b/.claude/agents/plugin-api-guardian.md new file mode 100644 index 00000000000..519905ee5e9 --- /dev/null +++ b/.claude/agents/plugin-api-guardian.md @@ -0,0 +1,115 @@ +--- +name: plugin-api-guardian +description: Reviews changes to the OpenShift Console dynamic plugin SDK for breaking changes, backwards compatibility, changelog compliance, and proper documentation. +tools: + - Read + - Bash + - Edit + - Write + - WebFetch + - WebSearch +--- + +# Plugin API Guardian + +You are a senior engineer specializing in the OpenShift Console dynamic plugin SDK. Your job is to review changes for breaking API impact, backwards compatibility, changelog compliance, and documentation quality. + +## What you review + +You review changes to files under `frontend/packages/console-dynamic-plugin-sdk/` and any other files that affect the public plugin API surface. This includes: + +- **Public API exports** in `src/api/internal-api.ts`, `internal-console-api.ts`, `internal-topology-api.ts`, `internal-types.ts` +- **Extension type definitions** in `src/extensions/` +- **Shared modules** in `src/shared-modules/` +- **Webpack plugin** (`src/webpack/`) +- **Generated schemas** in `generated/` +- **Changelogs**: `CHANGELOG-core.md`, `CHANGELOG-webpack.md` +- **Release notes** in `release-notes/` +- **CSS classes** that external plugins may use + +Non-SDK JavaScript/TypeScript changes should be ignored unless they affect the public API surface. + +## Rules you enforce + +### Breaking changes + +- Removal of exports, extension types, or shared modules MUST have been deprecated in a previous release first. +- Major version bumps in shared modules do not require prior deprecation notice. +- Any breaking change MUST be documented in the appropriate changelog. +- Check `internal-*.ts` files for removed or renamed exports — these are the public API surface that external plugins import from. + +### Changelog compliance + +- Changes to the `@openshift-console/dynamic-plugin-sdk` package MUST be documented in `CHANGELOG-core.md` under the next version section. +- Changes to the `@openshift-console/dynamic-plugin-sdk-webpack` package MUST be documented in `CHANGELOG-webpack.md`. +- Changelog entries require a JIRA ticket and PR number. +- If the next version section doesn't exist, note that it needs to be created. +- Determine the JIRA issue from the branch name prefix or commit messages (e.g., `CONSOLE-1234` or `OCPBUGS-5678`). + +### Release notes + +- Major API changes should have an entry in the corresponding `release-notes/` file. +- CSS class removals/renames go in the "CSS styling" section of release notes. +- Changes to shared modules go in the "Changes to shared modules and API" section. + +### Documentation quality + +- New public APIs MUST have TSDoc with: description, all parameters documented, return types documented, usage examples for complex APIs. +- Extension type definitions must be clear enough for external plugin authors who don't know Console internals. +- Proper IntelliSense support is required for all public APIs. + +### Extension types and schemas + +- Changes to extension types in `src/extensions/` MUST be backwards-compatible with the current OCP version. +- Exception: fixing an extension that doesn't work in the current version (but this must still be documented). +- Extension type names follow the `console.*` naming convention. + +### CSS impact + +- CSS class changes may break external plugins. When you find CSS changes, note that a GitHub search should be performed for the class name in repos that consume `@openshift-console/dynamic-plugin-sdk`. + +### Code quality (SDK-specific) + +- Re-exports in `internal-*.ts` files must use consistent patterns. +- Type-only exports should use `export type`. +- No barrel imports from package index files. +- Follow the project styleguide for TypeScript conventions. + +## How to determine the current OCP version + +The current in-development version is determined by finding the lowest-number release branch that still tracks `main` in the upstream remote. If `git diff upstream/main...upstream/release-X.Y` shows no differences, then `X.Y` is the current version. + +The next SDK version is the current OCP version appended with `.0-prerelease.N` (incrementing N). + +## Output format + +Structure your review as: + +``` +## Plugin API Review + +**API change:** +**JIRA issue:** +**Breaking:** Yes/No +**Changelog updated:** Yes/No/Not needed + +### Findings + +- [x] or [ ] Backwards compatibility verified +- [x] or [ ] Changelog entries present and correct (CHANGELOG-core.md) +- [x] or [ ] Changelog entries present and correct (CHANGELOG-webpack.md) +- [x] or [ ] Release notes updated if needed +- [x] or [ ] TSDoc documentation adequate for new/changed APIs +- [x] or [ ] Extension type changes are backwards-compatible +- [x] or [ ] CSS changes assessed for external plugin impact +- [x] or [ ] JIRA issue associated with the change +- [x] or [ ] Styleguide compliance + +### Issues + + + +### Recommendations + + +``` diff --git a/.claude/skills/pre-push-review/SKILL.md b/.claude/skills/pre-push-review/SKILL.md index a3f61764509..68590e4242e 100644 --- a/.claude/skills/pre-push-review/SKILL.md +++ b/.claude/skills/pre-push-review/SKILL.md @@ -1,6 +1,6 @@ --- name: pre-push-review -description: Comprehensive local code review using both Claude AI and CodeRabbit AI before pushing changes to GitHub. This command analyzes your local changes and provides actionable feedback without posting anything to GitHub. +description: Fast local code review using domain-specialist agents before pushing changes to GitHub. Spawns parallel reviewers for SDK, frontend, and backend changes. argument-hint: "[--staged] [--base ]" --- @@ -8,530 +8,144 @@ argument-hint: "[--staged] [--base ]" ## Usage ```bash -# Review all commits compared to main branch (default) -/pre-push-review - -# Review only staged changes -/pre-push-review --staged - -# Review commits compared to a specific base branch -/pre-push-review --base upstream/main - -# Review commits compared to develop branch -/pre-push-review --base develop - -# Review staged changes (base branch used for context only) -/pre-push-review --staged --base upstream/main +/pre-push-review # Review commits vs main +/pre-push-review --staged # Review only staged changes +/pre-push-review --base upstream/main # Review commits vs specific base +/pre-push-review --staged --base dev # Staged changes, custom base ``` ## Description -Comprehensive local code review using both Claude AI and CodeRabbit AI before pushing changes to GitHub. This command analyzes your local changes and provides actionable feedback without posting anything to GitHub. - -**Two Review Modes**: -1. **Commits Mode** (default): Review all commits that are new compared to the base branch -2. **Staged Mode** (`--staged`): Review only staged changes (what you've `git add`-ed) - -## Prerequisites Check - -Before starting the review, verify that required tools are installed and authenticated: - -1. **Git**: - - Must be in a git repository - - Check current branch with `git branch --show-current` - - Verify there are changes to review - -2. **CodeRabbit CLI**: - - Check: Run `coderabbit auth status` - - If not installed: - - Run: `curl -fsSL https://cli.coderabbit.ai/install.sh | sh` - - Then: `source ~/.zshrc` (or appropriate shell config) - - Authentication: - - Ask user to request: "Run: coderabbit auth login" - - Claude will execute the command and provide authentication URL - - User opens URL in browser and copies token - - User pastes token back to Claude - - Verify with: `coderabbit auth status` +Local code review that spawns domain-specialist agents in parallel to review your changes before pushing. Each agent is an expert in its domain — SDK compliance, frontend patterns, or Go backend — and reviews only the files relevant to it. -**IMPORTANT**: If CodeRabbit is missing or not authenticated, you will stop and will not proceed. +Nothing is posted to GitHub. No external tools required. ## Process -### Phase 1: Determine Review Scope +### Phase 1: Gather changes -1. **Parse Parameters**: - - Check for `--staged` flag (review staged changes) - - Check for `--base ` parameter (default: `main`) - - If no flags: default to commits mode +1. **Parse parameters**: + - `--staged` → review staged changes only + - `--base ` → compare against this branch (default: `main`) -2. **Gather Git Information**: +2. **Validate scope**: ```bash - # Get current branch git branch --show-current - - # For commits mode: count new commits vs base - git rev-list --count ..HEAD - - # For staged mode: check if there are staged changes + # Commits mode: + git rev-list --count ..HEAD + # Staged mode: git diff --cached --stat + ``` + - No changes found → inform user and exit - # Get repository root - git rev-parse --show-toplevel +3. **Collect diff and file list**: + ```bash + # Commits mode: + git diff ..HEAD --name-only + git diff ..HEAD + git diff ..HEAD --stat + git log ..HEAD --oneline + + # Staged mode: + git diff --cached --name-only + git diff --cached + git diff --cached --stat ``` -3. **Validate Scope**: - - **Commits mode**: Ensure there are new commits compared to base branch - - If no new commits: inform user and exit - - **Staged mode**: Ensure there are staged changes - - If no staged changes: inform user to run `git add` first and exit - - If base branch doesn't exist: inform user and suggest alternatives +### Phase 2: Classify and dispatch -### Phase 2: Gather Changes for Review +Classify changed files into three domains: -**For Commits Mode**: -```bash -# Get commit log -git log ..HEAD --oneline +| Domain | File pattern | Agent | +|--------|-------------|-------| +| **SDK** | `frontend/packages/console-dynamic-plugin-sdk/**` | `plugin-api-guardian` | +| **Frontend** | `frontend/**` (excluding SDK) | `frontend-reviewer` | +| **Backend** | `pkg/**`, `cmd/**`, `*.go`, `go.mod`, `go.sum` | `go-backend-reviewer` | -# Get full diff of all commits -git diff ..HEAD +Files that don't match any domain (docs, CI config, scripts) are noted but not sent to an agent. -# Get stats (files changed, additions, deletions) -git diff ..HEAD --stat +**Spawn only agents for domains that have changes.** Use the `Agent` tool with `subagent_type` set to the agent name. Send all relevant agents in a single message so they run in parallel. -# Get list of changed files -git diff ..HEAD --name-only -``` +Each agent's prompt MUST include: +- The list of changed files in its domain +- The full diff for those files only (filter the diff to their domain) +- The commit messages (commits mode only) +- Instruction to return structured findings per its output format -**For Staged Mode**: -```bash -# Get staged changes diff -git diff --cached +### Phase 3: Synthesize -# Get stats of staged changes -git diff --cached --stat +Once all agents return: -# Get list of staged files -git diff --cached --name-only -``` +1. **Collect findings** from each agent's response +2. **Add cross-domain observations** — issues at the boundary between domains: + - Backend added an endpoint but frontend doesn't call it + - SDK type changed but static plugin extensions weren't updated + - Model changes in frontend don't match Go struct changes +3. **Rank all issues by severity**: CRITICAL > HIGH > MEDIUM > LOW +4. **Check commit quality** (commits mode): messages, atomicity, organization -### Phase 3: Dual AI Review (Parallel Execution) - -**IMPORTANT**: This is a LOCAL review only - nothing is posted to GitHub. - -Run both reviews concurrently for efficiency: - -1. **CodeRabbit Review** (Background - START THIS FIRST): - - CodeRabbit analyzes git changes and can take 7-30+ minutes - - **For Commits Mode**: `coderabbit --prompt-only --type committed --base ` - - **For Staged Mode**: `coderabbit --prompt-only --type uncommitted` - - ⚠️ **Background execution note**: Run in background to allow parallel processing, BUT the background output may only show status messages ("Analyzing", "Reviewing"). If no findings appear, run the command again directly to capture actual results. - - Flags explained: - - `--prompt-only`: Token-efficient AI-optimized output (does NOT post to GitHub) - - `--type committed`: Review committed changes (for commits mode) - - `--type uncommitted`: Review working directory and staged changes (for staged mode) - - `--base `: Specify base branch for comparison - - Monitor progress: CodeRabbit will output findings as it discovers them - - Note: CodeRabbit automatically reads `AGENTS.md` for project context - - **Expected output format**: CodeRabbit provides findings in format: - ``` - File: path/to/file.ts - Line: X to Y - Type: potential_issue - - Prompt for AI Agent: - [Detailed description of the issue and suggested fix approach] - ``` - -2. **Claude AI Review** (While CodeRabbit runs): - - Analyze the diff output from Phase 2 - - Review commit messages (commits mode only) - - Focus on: - - Code quality and best practices - - Potential bugs and edge cases - - Security vulnerabilities (SQL injection, XSS, command injection, etc.) - - Performance implications - - Breaking changes and backwards compatibility - - API design and consistency - - Type safety and error handling - - Test coverage gaps - - Documentation completeness - - Project-specific requirements: - - i18n usage (per AGENTS.md) - - PatternFly v6 usage, avoid inline styles - - No React namespace imports - -### Phase 4: Wait for CodeRabbit Completion - -1. **Monitor Background Process**: Check if CodeRabbit is still running -2. **Read Output**: Once complete, read the full CodeRabbit analysis - - ⚠️ **Common Issue**: If background execution only shows status messages without findings, run the command again directly: `coderabbit --prompt-only --type committed --base ` - - This ensures you capture the actual AI-generated findings, not just status updates -3. **Handle Timeout**: If CodeRabbit takes longer than expected, you can: - - Continue waiting (recommended - CodeRabbit is thorough) - - Proceed with Claude-only review if time-constrained - - Note in the final report if CodeRabbit was incomplete - -### Phase 5: Cross-Check and Synthesize - -1. **Compare Findings**: - - Identify issues found by both Claude and CodeRabbit (high confidence) - - Identify issues found by only one reviewer (needs validation) - - Flag conflicting recommendations - - Note CodeRabbit's unique insights (race conditions, memory leaks, subtle logic errors) - -2. **Categorize Issues by Severity**: - - **Critical**: Security vulnerabilities, data loss risks, breaking changes, memory leaks - - **Actionable**: Bugs, race conditions, performance issues, incorrect implementations - - **Minor**: Code style, naming conventions, small optimizations - - **Informational**: Suggestions, best practices, future improvements - -3. **Prioritize by Confidence**: - - Issues found by both reviewers should be prioritized - - Single-reviewer findings should be validated before flagging as critical - -### Phase 6: Generate Review Summary - -Output a comprehensive review following this template: +### Phase 4: Report + +Output a unified review: ```markdown -# Pre-Push Review Summary +# Pre-Push Review ## Overview -- **Review Mode**: Commits (vs. ) | Staged Changes -- **Current Branch**: -- **Base Branch**: -- **Commits**: new commits | N/A (staged mode) -- **Files Changed**: -- **Lines Changed**: + - -- **Review Method**: Claude + CodeRabbit | Claude only (if CodeRabbit unavailable) - -## Critical Issues (Action Required Immediately) - critical issues identified - -### 1. -- **Location**: : -- **Severity**: Critical -- **Found by**: Claude + CodeRabbit | Claude only | CodeRabbit only -- **Issue**: -- **Why it matters**: -- **Consequence if not fixed**: -- **Suggested fix**: - -## Actionable Issues (Should Address Before Push) - actionable issues identified - -### 1. -- **Location**: : -- **Severity**: High/Medium -- **Found by**: Claude + CodeRabbit | Claude only | CodeRabbit only -- **Issue**: -- **Why it matters**: -- **Consequence if not fixed**: -- **Suggested fix**: - -## Minor Issues (Optional Improvements) - minor issues identified - -- : - -- : - - -## Positive Highlights -- -- -- -- - -## Cross-Check Summary -- **Issues confirmed by both reviewers**: -- **Claude-only findings**: -- **CodeRabbit-only findings**: -- **Conflicting recommendations**: (if any, explain the conflict) - -## Commit Quality Analysis (Commits Mode Only) -- **Commit Messages**: Clear and descriptive | Needs improvement -- **Commit Organization**: Well-structured | Consider squashing/splitting -- **Atomic Commits**: Each commit is self-contained | Some commits too large - -## Recommendation -- [ ] Ready to push (no critical issues) -- [ ] Fix critical issues before pushing -- [ ] Consider addressing actionable issues before push -- [ ] Safe to push with minor issues (can address in follow-up) - -## Next Steps - -1. Fix critical issue X in file Y -2. Consider refactoring Z for better performance -3. Add tests for edge case A -4. Update documentation for feature B - -## Additional Context - - -## CodeRabbit Insights - -- Race conditions detected -- Memory leak potential -- Subtle logic errors that static analysis wouldn't catch -- Performance hotspots -- Concurrency issues -``` - -### Phase 7: Optional Fix Implementation +- **Branch**: → +- **Mode**: Commits () | Staged +- **Files changed**: (+ -) +- **Agents used**: -After completing the review, Claude can implement fixes for identified issues: +## Critical Issues + -1. **Offer to implement fixes**: Ask user if they want Claude to implement the actionable issues -2. **Implement fixes systematically**: - - Start with critical issues first - - Apply one fix at a time for safety - - Use proper coding patterns (avoid React hooks violations, maintain type safety) - - Follow project conventions (i18n, PatternFly usage, etc.) -3. **Handle pre-commit hooks**: Be prepared for pre-commit hooks that may catch issues in the fixes themselves - - ESLint violations, React hooks issues, formatting problems - - Fix these immediately and re-stage changes -4. **Commit fixes with descriptive message**: Include details about what was fixed and why +### 1. +- **File**: <path>:<line> +- **Agent**: <which agent found it> +- **Issue**: <description> +- **Fix**: <suggestion> -### Phase 8: Verification (Re-run Review) +## High Priority +<bugs, correctness issues> -After implementing fixes, optionally re-run the review to verify: +## Medium Priority +<patterns, i18n, a11y, style> -1. **Re-run pre-push review**: Check that fixes don't introduce new issues -2. **Validate fix quality**: Ensure the implemented solutions are correct -3. **Confirm readiness**: Final confirmation that code is ready to push +## Low Priority +<minor improvements> -## Guidelines - -**Review Standards**: -- Focus on objective, actionable feedback -- Provide specific file and line references -- Explain WHY changes are needed, not just WHAT to change -- Prioritize security, correctness, and maintainability over style -- Consider the change context (bug fix, feature, refactor) when reviewing -- Cross-reference with project style guides and contribution guidelines -- Be concise but thorough - avoid over-explaining obvious issues - -**Code Review Principles**: -- Assume good intent from the developer -- Focus on teaching moments for complex issues -- Highlight what's done well, not just problems -- Consider if the issue blocks pushing or can be addressed later -- Validate that tests cover the changes -- Check for proper i18n usage (per AGENTS.md) -- Verify PatternFly v6 usage and avoid inline styles (per AGENTS.md) -- Check for React namespace import issues (per AGENTS.md) - -**Tool Usage**: -- Use `git diff` and `git log` for gathering local changes -- Run CodeRabbit in background: - - Commits mode: `coderabbit --prompt-only --type committed --base <branch>` - - Staged mode: `coderabbit --prompt-only --type uncommitted` -- CodeRabbit takes 7-30+ minutes - monitor progress and wait for completion -- Parse git output to understand scope and context -- Handle errors gracefully if git commands fail - -**Output Format**: -- Use clear markdown formatting for terminal display -- Use color/emphasis sparingly for readability -- Group related issues together -- Provide actionable next steps -- Include links to relevant documentation when applicable - -## Example Usage - -```bash -# Review all commits before pushing to PR -/pre-push-review +## What looks good +- <positive observations> -# Review commits compared to upstream main -/pre-push-review --base upstream/main +## Commit Quality (commits mode only) +- **Messages**: <assessment> +- **Organization**: <assessment> -# Review only what you've staged -git add src/component.tsx -/pre-push-review --staged - -# Review commits on feature branch vs develop -git checkout feature/new-auth -/pre-push-review --base develop +## Recommendation +Ready to push | Fix critical issues first | Address high-priority items ``` -## Success Criteria -- CodeRabbit is installed and authenticated (or Claude-only review) -- Git repository and branch information retrieved successfully -- Changes to review are identified (commits or staged) -- Both AI reviewers complete their analysis (or Claude-only if CodeRabbit unavailable) -- Findings are cross-checked and categorized by severity and confidence -- Comprehensive review summary generated in terminal -- No reviews posted to GitHub (local-only analysis) -- User has clear understanding of: - - Which issues are critical vs. actionable vs. minor - - Why each issue matters and consequences of not fixing - - Specific actionable steps to resolve each issue - - Whether changes are ready to push or need fixes - -## Workflow Tracking -- Use TaskCreate to track the 6-8 phases of the review process (including optional implementation and verification phases) -- Mark each phase as in_progress when starting and completed when done via TaskUpdate -- This gives the user visibility into review progress, especially during CodeRabbit's long analysis -- If implementing fixes, track individual fix tasks to show progress - -## Error Handling - -**If CodeRabbit is missing**: -1. Inform user that CodeRabbit is not installed -2. Provide installation command: `curl -fsSL https://cli.coderabbit.ai/install.sh | sh` -3. Offer to proceed with Claude-only review -4. Note in summary that only Claude review was performed - -**If CodeRabbit authentication fails**: -1. Run `coderabbit auth status` to check status -2. Guide user through authentication: `coderabbit auth login` -3. Note: Authentication improves quality significantly -4. Proceed with Claude-only review if user prefers not to authenticate - -**If no changes to review**: -1. **Commits mode**: No new commits compared to base branch - - Inform user: "No new commits found compared to <base-branch>" - - Suggest checking current branch or specifying different base - - Exit gracefully -2. **Staged mode**: No staged changes - - Inform user: "No staged changes found" - - Suggest running `git add <files>` first - - Show `git status` to help user see what can be staged - -**If base branch doesn't exist**: -1. Inform user that base branch was not found -2. Show available branches with `git branch -a` -3. Suggest common alternatives (main, master, develop, upstream/main) -4. Exit and wait for user to specify correct base branch - -**If git repository not found**: -1. Check if current directory is a git repository -2. Inform user to run from within a git repository -3. Exit gracefully - -**If CodeRabbit fails or times out**: -1. Check `coderabbit auth status` (authentication improves quality) -2. Verify git repository is clean with `git status` -3. Ensure you're analyzing code files (not just docs/configs) -4. If timeout: Note in summary and proceed with Claude-only findings -5. Consider using `--type uncommitted` for faster analysis (staged mode) - -**If CodeRabbit background execution shows no findings**: -1. This is common when using background execution - only status messages are captured -2. Run the same command again directly (not in background) to capture actual findings -3. Example: `coderabbit --prompt-only --type committed --base main` -4. The direct execution will show the detailed AI findings with file locations and suggestions - -**If fix implementation introduces new issues**: -1. **Pre-commit hooks may catch new violations**: ESLint errors, React hooks violations, etc. -2. **Fix immediately**: Address the violation while preserving the original fix intent -3. **Common issues during fixes**: - - React hooks violations (early returns before hooks, conditional hook calls) - - ESLint formatting or rule violations - - TypeScript type errors -4. **Re-stage changes**: After fixing pre-commit issues, re-stage the files and commit again - -**If one reviewer fails**: -1. Proceed with the working reviewer -2. Clearly note in the summary that only one review was completed -3. Explain which reviewer failed and why -4. Provide troubleshooting steps for the failed reviewer -5. Suggest retrying with the specific reviewer once issue is resolved - -## Notes - -**Review Behavior**: -- This command performs LOCAL review only - nothing is posted to GitHub -- No branches are changed - you stay on your current branch -- No files are modified - this is read-only analysis -- Run this before `git push` to catch issues early -- Re-run after fixing issues to verify changes - -**CodeRabbit Integration**: -- CodeRabbit excels at finding race conditions, memory leaks, and subtle logic errors -- The `--prompt-only` flag provides AI-optimized output without posting to GitHub -- CodeRabbit automatically reads `AGENTS.md` for project-specific context -- Reviews can take 7-30+ minutes depending on change size - this is normal -- Authenticated CodeRabbit provides significantly better review quality -- Use `--type uncommitted` (staged mode) for faster reviews - -**Cross-Check Value**: -- Issues found by both reviewers have highest confidence and should be prioritized -- CodeRabbit may catch concurrency/memory issues that static analysis misses -- Claude provides strong context on architecture, API design, and project conventions -- Conflicting recommendations should be analyzed carefully - both may have valid points - -## Performance Tips - -**Optimize CodeRabbit Analysis Time**: -- **Staged mode** (`--type uncommitted`) is fastest - only reviews what you've staged -- **Commits mode** (`--type committed`) reviews all commits vs base branch -- Keep commits focused and smaller for quicker reviews -- CodeRabbit is fastest on pure code files vs. configuration/documentation - -**Managing Long Review Times**: -- CodeRabbit reviews take 7-30+ minutes - this is expected for thorough analysis -- Always run CodeRabbit in background first, then start Claude review -- Use TaskCreate/TaskUpdate to show progress - helps user understand what's happening -- Don't interrupt CodeRabbit - let it complete for best results -- If truly time-constrained, proceed with Claude-only review - -## Best Practices - -**Before Running Review**: -- Ensure CodeRabbit is authenticated for best results (or accept Claude-only) -- Know which base branch you're comparing against (main, upstream/main, develop) -- For staged mode: ensure you've `git add`-ed the files you want reviewed -- For commits mode: ensure you have commits to review - -**During Review**: -- Monitor CodeRabbit progress in background -- Claude can analyze while CodeRabbit runs (parallel efficiency) -- Don't skip waiting for CodeRabbit unless absolutely necessary -- Use TaskCreate/TaskUpdate phases to communicate progress to user - -**After Review**: -- Address critical issues before pushing (Claude can implement these fixes) -- Consider fixing actionable issues for code quality -- Minor issues can often be addressed in follow-up commits -- **If implementing fixes**: Let Claude implement them systematically, watching for pre-commit hook issues -- **Re-run review after fixes** to verify no new issues were introduced -- When ready: `git push` with confidence - -**Workflow Integration**: -```bash -# Typical workflow -git add src/feature.tsx -git commit -m "Add new feature" -/pre-push-review # Review before push - -# If issues found, let Claude implement fixes: -# Claude will: -# 1. Apply fixes to identified issues -# 2. Handle any pre-commit hook violations -# 3. Stage changes and commit with descriptive message - -/pre-push-review # Verify fixes (optional but recommended) -git push origin feature-branch # Push with confidence -``` +### Phase 5: Fix (optional) -**Extended Workflow with Fix Implementation**: -```bash -# 1. Initial development -git add . -git commit -m "feat: implement user authentication" +If the user wants fixes implemented: -# 2. Run comprehensive review -/pre-push-review +1. Start with critical issues, then high priority +2. Apply one fix at a time +3. Follow project conventions (the agents already flagged what's wrong) +4. Handle pre-commit hook failures +5. Offer to re-run the review to verify -# 3. Claude finds issues and offers to fix them -# User: "Yes, please fix the actionable issues" -# Claude implements fixes, handles pre-commit hooks, commits changes +## Error handling -# 4. Optional verification -/pre-push-review +- **No changes**: Inform user, suggest checking branch or using `--staged` +- **Base branch doesn't exist**: Show available branches, suggest alternatives +- **Not a git repo**: Inform user and exit +- **Agent returns no findings**: Note "no issues found" for that domain — this is fine -# 5. Push with confidence -git push origin feature-branch -``` +## Guidelines + +- Only spawn agents for domains with actual changes — don't waste tokens +- Filter the diff so each agent only sees its own files +- Keep the report concise — file:line references, not paragraphs +- Focus on actionable findings, not style nitpicks +- Cross-domain issues are the main agent's responsibility, not the agents'