From a0f8a7799033edf57e3522becb4947ca4d295deb Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Wed, 10 Jun 2026 13:07:18 +0700 Subject: [PATCH] docs: add lessons learned to AGENT.md + Star History to README MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENT.md: - Updated code structure with new modules (mcp/, engine/security_scanner, etc.) - Updated test count (205 → 453) - Added 'Lessons Learned' section covering workflow discipline, Rust-specific patterns, CI/code scanning, MCP design, and security scanner tuning .agent.md: - Updated pre-release checklist (test count, CI checks, Code Scanning) - Added CI required checks list (10) - Added module dependency diagram - Updated branch naming convention README.md: - Added Star History chart section before Contributing --- .agent.md | 34 ++++++++++++++++++--- AGENT.md | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++----- README.md | 10 +++++++ 3 files changed, 122 insertions(+), 12 deletions(-) diff --git a/.agent.md b/.agent.md index 1d75c7e..dbc7576 100644 --- a/.agent.md +++ b/.agent.md @@ -17,17 +17,20 @@ Before creating any release PR, verify ALL of the following are updated: - [ ] `docs/roadmap.md` — mark completed items, add new planned items - [ ] `docs/examples.md` — CI examples, new features - [ ] Pre-commit hook passes (`cora review --staged`) -- [ ] All tests pass (`cargo test`) -- [ ] CI checks green on PR +- [ ] All tests pass (`cargo test`) — currently 453 tests +- [ ] CI checks green on PR (10 required checks) +- [ ] Code Scanning alerts: 0 open (evaluate + fix or dismiss each) ## Branch Conventions - All PRs target `develop` (not `main`) -- Feature branches: `feat/` +- Feature branches: `feat/-` +- Fix branches: `fix/` - Release branches: `release/vX.Y.Z` - Pre-commit hook must pass before each commit +- **One issue = one branch = one PR = wait CI green = merge = next** -## Config Architecture (v0.4.5+) +## Config Architecture (v0.4.6+) | File | Contents | |------|----------| @@ -41,3 +44,26 @@ Before creating any release PR, verify ALL of the following are updated: - CORA_API_KEY env var: CI use only, not for local dev - Provider info auto-migrated from auth.toml → config.yaml - Config priority: CLI flags → env vars → .cora.yaml → config.yaml → auto-detect → defaults +- Marketplace action: separate repo `codecoradev/cora-review-action` (not in-tree) +- Quality gate: exit code 2 on failure (1 = error, 0 = success) +- `merge_into()` returns `Result<(), CoraError>` — fail-fast on invalid profile config +- MCP server: JSON-RPC 2.0 over stdio, 5 tools + +## CI Required Checks (10) + +Check, Format, Clippy, Test, Build, Security Audit (CVE), Website Build, NPM Audit, Cargo Outdated, Cora Review + +## Module Dependencies + +``` +config/ → engine/ → formatters/ + ↗ ↘ +main.rs → commands/ → output + ↘ ↗ + git/ → engine/review + mcp/ → engine/* (rules, security_scanner, secrets_scanner, profiles, diff_parser) + hook/ +``` + +Key: engine modules should NOT depend on commands or config/loader directly. +mcp/tools uses engine internals via `crate::engine::*`. diff --git a/AGENT.md b/AGENT.md index 5c46d0e..1301b51 100644 --- a/AGENT.md +++ b/AGENT.md @@ -9,17 +9,21 @@ no managed API, no cloud service. Runs locally against diffs, scans, or branches - **Edition:** Rust 2024 (MSRV 1.85) - **Repo:** `codecoradev/cora-cli` - **Default branch:** `develop` +- **Marketplace:** https://github.com/marketplace/actions/cora-ai-code-review +- **Website:** https://codecora.dev ## Build & Development Commands ```bash cargo build # Build (debug) cargo build --release # Build (release) -cargo test # Run all 224 tests -cargo clippy # Lint -cargo fmt # Format check (use -- --check for CI) +cargo test # Run all 453 tests +cargo clippy --all-targets -- -D warnings # Lint (strict) +cargo fmt --all -- --check # Format check ``` +Always run `cargo fmt --all` before committing. CI runs all three checks. + ## Code Structure (`src/`) ``` @@ -46,7 +50,24 @@ src/ │ ├── review.rs # Review orchestration logic │ ├── scanner.rs # File scanning engine │ ├── llm.rs # LLM API interaction -│ └── types.rs # Severity, finding, and result types +│ ├── types.rs # Severity, finding, and result types +│ ├── diff_parser.rs # Diff → FileChunk parsing +│ ├── chunker.rs # Auto-chunking large diffs +│ ├── profiles.rs # Quality profiles (strict/balanced/lax) +│ ├── quality_gate.rs # Quality gate thresholds + pass/fail +│ ├── security_scanner.rs # Static security pattern matching +│ ├── language_analyzer.rs # Language-specific review guidance +│ ├── secrets_scanner.rs # Secret/credential detection +│ └── rules/ # Custom rule engine +│ ├── mod.rs +│ ├── builtin.rs # Built-in rules +│ ├── matching.rs # Rule matching logic +│ └── types.rs # Rule & finding types +├── mcp/ # MCP server (Model Context Protocol) +│ ├── mod.rs +│ ├── protocol.rs # JSON-RPC 2.0 types +│ ├── server.rs # Stdio transport + request dispatch +│ └── tools.rs # 5 tool handlers ├── formatters/ # Output format implementations │ ├── mod.rs │ ├── pretty.rs # Human-readable terminal output @@ -75,10 +96,10 @@ src/ ## Testing ```bash -cargo test # 205 tests total - # 183 unit tests - # 16 CLI integration tests - # 6 config tests +cargo test # 453 tests total + # 431 unit tests + # 16 CLI integration tests + # 6 config tests cargo test --no-verify # Skip pre-commit hooks (avoids timeout in hooks) ``` @@ -145,3 +166,56 @@ API keys live in a separate `auth.toml` file (`~/.cora/auth.toml`), not in - **Documentation update before release**: README config/features/flags, website configuration docs page, and homepage feature bullets MUST be updated to reflect new features BEFORE version bump and tagging. + +## Lessons Learned (Agent Operating Principles) + +These are hard-won lessons from actual development sessions. Follow them. + +### Workflow Discipline + +- **One issue = one branch = one PR = wait CI green = merge = next.** Never stack + multiple features on one branch. Serial workflow prevents merge conflicts and + makes bisecting bugs trivial. +- **Local green ≠ CI green.** CI may use a different Rust version that is stricter + (e.g. treats dead_code as error, not warning). Always wait for CI to pass. +- **Delete stale branches immediately after merge.** `git push origin --delete `. + A clean `git branch -a` improves developer experience. + +### Rust-Specific + +- **`pub` visibility = API contract.** Every `pub` added must have justification. + When exposing internals (e.g. `SecurityPattern` fields for MCP), consider adding + getter methods instead of making fields public. +- **`Result<(), CoraError>` is infectious.** Changing a return type from `()` to + `Result` will propagate to every callsite. Plan the blast radius before changing + signatures. +- **Use `edit` (exact text replacement), not `sed` for refactoring.** `sed` applies + globally and can mangle struct literal instances when you only meant to change the + struct definition. +- **`#[allow(dead_code)]` for structs used only in `#[cfg(test)]`.** Clippy in CI + with `-D warnings` treats dead_code as error. Functions called only from tests + need this annotation. + +### CI & Code Scanning + +- **Code Scanning alerts: evaluate before dismissing.** Some are false positives + (test fixtures like `AKIAIOSFODNN7EXAMPLE`), but others catch real bugs (e.g. + redundant `parse_diff()` calls, broken line-based stdio parsing). +- **Dismiss with reason.** Always provide `dismissed_reason` ("won't fix" or + "false positive") so future reviewers understand the decision. + +### MCP Server Design + +- **MCP is simpler than you think.** JSON-RPC 2.0 over stdio. No HTTP server needed. + The client (Claude Code, Cursor, etc.) manages the lifecycle. +- **Don't use line-based stdin parsing for JSON-RPC.** Pretty-printed JSON spans + multiple lines. Use brace-depth tracking instead. +- **Reuse parsed data.** If `parse_diff()` already ran, pass the `Vec` to + downstream functions. Never parse the same diff twice. + +### Security Scanner + +- **Every regex pattern needs exemption logic.** Skip test files, example keys, + fixtures. Without exemptions, noise drowns out real findings. +- **Balance sensitivity.** Too strict = wall of false positives. Too loose = miss + real vulnerabilities. Start conservative, tune based on real findings. diff --git a/README.md b/README.md index cee6fab..681c517 100644 --- a/README.md +++ b/README.md @@ -158,6 +158,16 @@ Provider-specific keys are auto-detected: `OPENAI_API_KEY`, `ANTHROPIC_API_KEY`, | [Changelog](https://codecora.dev/changelog.html) | Release history | | [Roadmap](https://codecora.dev/roadmap.html) | Planned features | +## Star History + + + + + + Star History Chart + + + ## Contributing See **[CONTRIBUTING.md](CONTRIBUTING.md)** for guidelines. PRs welcome!