feat: config redesign + severity comparison fix#60
Closed
ajianaz wants to merge 23 commits into
Closed
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (31)
📝 WalkthroughWalkthroughThis PR introduces a multi-level configuration system with global ( ChangesConfig Redesign & Severity Fixes
Sequence DiagramsequenceDiagram
participant User
participant CLI as cora config
participant Execute as execute_config_set
participant Loader as config loader
participant Write as YAML write
User->>CLI: cora config set base_url https://example.com --global
CLI->>Execute: Call with global flag true
Execute->>Loader: Check for legacy config.toml
Loader->>Loader: Migrate TOML to YAML format
Loader->>Loader: Extract api_key to auth.toml
Execute->>Write: Load ~/.cora/config.yaml
Write->>Write: Update provider.base_url field
Write->>Write: Serialize with skip None
Write-->>Execute: Write complete
Execute-->>CLI: Success confirmation
CLI-->>User: Global config updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Cora AI Code Review❌ Blocked — critical issues found. 🔴 Error (6)
Review powered by cora-cli · BYOK · MIT |
ajianaz
pushed a commit
that referenced
this pull request
May 31, 2026
- Fix API key TOML injection: use toml::Table instead of format!() - Add migration marker (.migrated) to prevent repeated filesystem I/O - Restrict global config file permissions to 0600 - Guard project config set: require git repo (no orphan files)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/CONFIG-REDESIGN.md`:
- Around line 13-15: The fenced code block containing "CLI flags → CORA_* env
vars → .cora.yaml (project) → ~/.cora/config.yaml (global) → Defaults" in
docs/CONFIG-REDESIGN.md needs a language hint to satisfy markdownlint MD040;
update that fenced block to start with ```text (i.e., add the language specifier
"text" after the opening backticks) so the block becomes a text-marked fenced
code block.
In `@docs/FEATURE-DESIGN.md`:
- Around line 26-63: The markdown file contains fenced code blocks without
language tags that trigger MD040; update the two shown code fences (the cora
init and cora doctor examples) to include a language tag (e.g., change ``` to
```bash) and apply the same fix to the other occurrences flagged (lines 91–120)
so every fenced block declares a language (bash or text) to satisfy the linter.
- Line 126: Update the doctor check table entry that currently reads "| 1 |
Config file found | error | Looks for `.cora.yaml` or
`~/.config/cora/config.yaml` |" to use the new global config path; replace
`~/.config/cora/config.yaml` with `~/.cora/config.yaml` so the row reads
something like "| 1 | Config file found | error | Looks for `.cora.yaml` or
`~/.cora/config.yaml` |", ensuring the doc matches the redesigned config spec.
In `@README.md`:
- Around line 221-223: The fenced code block containing "CLI flags → CORA_* env
vars → .cora.yaml (project) → ~/.cora/config.yaml (global) → defaults" is
missing a language tag; update that markdown fence from ``` to ```text (or
another appropriate language like ```console) so the block reads ```text
followed by the same line and closing ``` to satisfy markdownlint MD040 and
preserve formatting.
- Around line 328-329: The curl command uses an unversioned asset name
("cora-x86_64-unknown-linux-gnu.tar.gz") under the "/releases/latest/download/"
path which may not exist; update the README to reference a specific release
asset or dynamically resolve the latest asset. Replace the hardcoded filename
with either a versioned file (e.g., use the actual tag like
"cora-vX.Y.Z-x86_64-unknown-linux-gnu.tar.gz") or show a safe approach to fetch
the latest asset URL (e.g., using the GitHub API or a small curl+grep pipeline
to discover the actual filename) so the mv/install steps target an existing
artifact.
In `@src/commands/config_cmd.rs`:
- Around line 114-131: The code currently uses
CoraFile::from_str(&content).unwrap_or_default() which silently discards the
existing config on parse errors; change this to explicitly handle parse failures
for the CoraFile load (the variable cora and the parsed path content): attempt
CoraFile::from_str(&content) and on Err either return an error with context
(e.g., propagate with a meaningful message including path.display()) or log/warn
the user and abort, rather than falling back to CoraFile::default(); ensure any
error uses with_context/map_err so the failure mentions the failing file and
parse error.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 949e8d06-b664-48f7-8870-94cb73fa6e0e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
CHANGELOG.mdCONTRIBUTING.mdREADME.mddocs/CONFIG-REDESIGN.mddocs/FEATURE-DESIGN.mdsrc/commands/auth.rssrc/commands/config_cmd.rssrc/commands/review.rssrc/commands/scan.rssrc/config/loader.rssrc/config/schema.rssrc/engine/review.rssrc/main.rs
Comment on lines
+13
to
+15
| ``` | ||
| CLI flags → CORA_* env vars → .cora.yaml (project) → ~/.cora/config.yaml (global) → Defaults | ||
| ``` |
There was a problem hiding this comment.
Add a language hint to the fenced code block.
This fence is missing a language specifier (text is sufficient) and will fail markdownlint MD040.
Suggested patch
-```
+```text
CLI flags → CORA_* env vars → .cora.yaml (project) → ~/.cora/config.yaml (global) → Defaults</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for 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.
In `@docs/CONFIG-REDESIGN.md` around lines 13 - 15, The fenced code block
containing "CLI flags → CORA_* env vars → .cora.yaml (project) →
~/.cora/config.yaml (global) → Defaults" in docs/CONFIG-REDESIGN.md needs a
language hint to satisfy markdownlint MD040; update that fenced block to start
with ```text (i.e., add the language specifier "text" after the opening
backticks) so the block becomes a text-marked fenced code block.
Comment on lines
+26
to
+63
| ``` | ||
| $ cora init -i | ||
|
|
||
| 🚀 Welcome to Cora! Let's set up your project. | ||
|
|
||
| ? Select a provider: | ||
| > OpenAI | ||
| Anthropic | ||
| Groq | ||
| Ollama (local) | ||
| Custom (OpenAI-compatible) | ||
|
|
||
| ? Model name (default: gpt-4o-mini): glm-5.1 | ||
|
|
||
| ? Base URL (default: https://api.openai.com/v1): https://litellm:4000 | ||
|
|
||
| ? API key: **** (or leave blank to use CORA_API_KEY env var) | ||
|
|
||
| ? Review focus areas (space to toggle, enter to confirm): | ||
| [✓] security | ||
| [✓] performance | ||
| [ ] bugs | ||
| [ ] best_practice | ||
| [ ] style | ||
| [ ] maintainability | ||
|
|
||
| ? Pre-commit hook mode: | ||
| > warn (print findings but allow commit) | ||
| block (fail commit on findings above threshold) | ||
|
|
||
| ✅ Created .cora.yaml with your settings. | ||
| ✅ Pre-commit hook installed. | ||
|
|
||
| Next steps: | ||
| cora review # Review staged changes | ||
| cora scan # Scan the full project | ||
| ``` |
There was a problem hiding this comment.
Fenced examples need language tags for markdownlint.
Both fenced blocks should declare a language (bash or text) to satisfy MD040.
Suggested patch
-```
+```bash
$ cora init -i
...@@
- +bash
$ cora doctor
...
Also applies to: 91-120
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for 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.
In `@docs/FEATURE-DESIGN.md` around lines 26 - 63, The markdown file contains
fenced code blocks without language tags that trigger MD040; update the two
shown code fences (the cora init and cora doctor examples) to include a language
tag (e.g., change ``` to ```bash) and apply the same fix to the other
occurrences flagged (lines 91–120) so every fenced block declares a language
(bash or text) to satisfy the linter.
|
|
||
| | # | Check | Severity | What it does | | ||
| |---|-------|----------|-------------| | ||
| | 1 | Config file found | error | Looks for `.cora.yaml` or `~/.config/cora/config.yaml` | |
There was a problem hiding this comment.
Use the new global config path in doctor checks.
The check table still points to ~/.config/cora/config.yaml, but this redesign uses ~/.cora/config.yaml. Please align this to avoid contradicting the config spec.
🤖 Prompt for 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.
In `@docs/FEATURE-DESIGN.md` at line 126, Update the doctor check table entry that
currently reads "| 1 | Config file found | error | Looks for `.cora.yaml` or
`~/.config/cora/config.yaml` |" to use the new global config path; replace
`~/.config/cora/config.yaml` with `~/.cora/config.yaml` so the row reads
something like "| 1 | Config file found | error | Looks for `.cora.yaml` or
`~/.cora/config.yaml` |", ensuring the doc matches the redesigned config spec.
Comment on lines
+221
to
+223
| ``` | ||
| CLI flags → CORA_* env vars → .cora.yaml (project) → ~/.cora/config.yaml (global) → defaults | ||
| ``` |
There was a problem hiding this comment.
Add a language specifier to this fenced block.
This block is missing a language tag and triggers markdownlint MD040.
Suggested patch
-```
+```text
CLI flags → CORA_* env vars → .cora.yaml (project) → ~/.cora/config.yaml (global) → defaults</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 221-221: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @README.md around lines 221 - 223, The fenced code block containing "CLI
flags → CORA_* env vars → .cora.yaml (project) → ~/.cora/config.yaml (global) →
defaults" is missing a language tag; update that markdown fence from totext (or another appropriate language like console) so the block reads text followed by the same line and closing ``` to satisfy markdownlint MD040
and preserve formatting.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+114
to
131
| // Determine target path | ||
| let path = if global { | ||
| let dir = loader::cora_dir()?; | ||
| std::fs::create_dir_all(&dir) | ||
| .with_context(|| format!("failed to create {}", dir.display()))?; | ||
| dir.join("config.yaml") | ||
| } else { | ||
| PathBuf::from(".cora.yaml") | ||
| }; | ||
|
|
||
| // Load existing config.toml content if it exists | ||
| let mut table = if path.is_file() { | ||
| // Load existing file or start fresh | ||
| let mut cora = if path.is_file() { | ||
| let content = std::fs::read_to_string(&path) | ||
| .with_context(|| format!("failed to read {}", path.display()))?; | ||
| content.parse::<toml::Table>().unwrap_or_default() | ||
| CoraFile::from_str(&content).unwrap_or_default() | ||
| } else { | ||
| toml::Table::new() | ||
| CoraFile::default() | ||
| }; |
There was a problem hiding this comment.
Silent data loss on config parse failure.
If the existing config file has a YAML syntax error, CoraFile::from_str fails and unwrap_or_default() silently replaces the entire config with an empty default. Other valid config entries in the file would be lost without warning.
Consider warning the user or returning an error when the existing config cannot be parsed.
Proposed fix to warn on parse failure
// Load existing file or start fresh
let mut cora = if path.is_file() {
let content = std::fs::read_to_string(&path)
.with_context(|| format!("failed to read {}", path.display()))?;
- CoraFile::from_str(&content).unwrap_or_default()
+ match CoraFile::from_str(&content) {
+ Ok(c) => c,
+ Err(e) => {
+ eprintln!(
+ "{} Config file {} has syntax errors: {}",
+ "⚠️".yellow(),
+ path.display(),
+ e
+ );
+ eprintln!(" Creating new config (existing values will be lost).");
+ CoraFile::default()
+ }
+ }
} else {
CoraFile::default()
};🤖 Prompt for 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.
In `@src/commands/config_cmd.rs` around lines 114 - 131, The code currently uses
CoraFile::from_str(&content).unwrap_or_default() which silently discards the
existing config on parse errors; change this to explicitly handle parse failures
for the CoraFile load (the variable cora and the parsed path content): attempt
CoraFile::from_str(&content) and on Err either return an error with context
(e.g., propagate with a meaningful message including path.display()) or log/warn
the user and abort, rather than falling back to CoraFile::default(); ensure any
error uses with_context/map_err so the failure mentions the failing file and
parse error.
Flags added: - review --commit <ref>: review git commit/range - review --diff-file <path>: review from diff file - review --quiet: suppress non-essential output - review --severity <level>: filter by min severity - scan --focus <areas>: override focus areas - config show: display resolved config - config set <key> <value>: update config values Documentation fixes: - README: cargo install cora-cli, fix config path, remove non-existent flags (--branch, --full, --commit HEAD), remove Homebrew (tap doesn't exist), fix env vars table - Website docs: fix all 10 inconsistencies across 7 pages - Fix test version assertion (use env var, not hardcoded) Fixes version test to use CARGO_PKG_VERSION env var
- Runs after check/fmt/clippy/test pass - Uses Infisical OIDC for LLM API key (CORA_API_KEY, CORA_PROVIDER, CORA_BASE_URL, CORA_MODEL) - Reviews diff against origin/develop with --severity major --quiet - Uploads SARIF to GitHub Code Scanning (block-capable) - Posts PR comment with grouped issues (update existing comment on re-run) - Checks for blocking issues (error-level) to fail the check - Only runs on pull_request events (not push to main) - Added permissions: security-events, pull-requests, id-token
- Remove 'invocation' from runs root (not allowed by GitHub validator) - Remove 'helpUri: null' from rules (must be string or omitted) - Add 'artifactChanges' to fixes (SARIF 2.1.0 required property) - Deduplicate rules by id - Remove unused chrono::Utc import - Upgrade CodeQL upload action v3 → v4
- Move Infisical identity-id to secret (INFISICAL_IDENTITY_ID) - Remove 2>&1 redirect (was mixing stderr into SARIF JSON) - Remove || true (was masking failures) - Guard SARIF upload with hashFiles check
- Create .github/actions/cora-review/action.yml - Configurable: base-branch, severity, infisical settings - Handles: SARIF upload, PR comment (update existing), blocking check - Suppresses cargo build noise (2>/dev/null) - No GITHUB_STEP_SUMMARY branding pollution - Slim ci.yml: cora-review job now 5 lines (uses composite action) - Fix duplicate 'error' in blocking check python script
- Remove || true (was masking failures silently) - Remove 2>/dev/null from cargo build (show build errors) - Keep 2>/dev/null on cora output only (prevent stderr corrupting SARIF) - Add echo for review completion status
Removed (0 uses each): - owo-colors (duplicate of colored) - thiserror (using anyhow instead) - chrono (no longer needed after SARIF fix) Replaced: - serde_yaml (deprecated) → serde_yml (maintained fork) Optimized: - tokio: 'full' → 'rt-multi-thread,macros' (unused features removed) Results: - Build time: 2m52s → 1m03s (-56%) - Transitive deps: 393 → 377 (-16) - 151 tests pass
serde_yml itself is deprecated. serde_yaml_ng v0.10 is the community-maintained fork with stable API. 151 tests pass.
Release binary now packaged as 'cora' (not target triple) inside tar. Updated release notes template to match new naming. Synced cora-review action with consumer repos: - Download from releases (not build from source) - cora-version default: 'latest' - SARIF upload: optional (default false) Co-authored-by: CTO Hermes <cto-hermes@users.noreply.github.com>
Co-authored-by: CTO Hermes <cto-hermes@users.noreply.github.com>
- Add website link (codecora.dev) to README header - Rewrite CI/CD Integration section with full setup guide - Add cora-review-simple action (GitHub Secrets, no Infisical) - Fix cora-review action: add latest version resolve step - Add troubleshooting section with real-world pitfalls - Add comparison table: cora-review vs cora-review-simple
Co-authored-by: CTO Hermes <cto-hermes@users.noreply.github.com>
…tructure (#58) Co-authored-by: CTO Hermes <cto-hermes@users.noreply.github.com>
- config set now writes YAML (not unread TOML) - support project (.cora.yaml) and global (~/.cora/config.yaml) config - config set --global flag for global config - add base_url to config set keys - auth writes to separate ~/.cora/auth.toml (0o600) - config show displays resolved config with priority chain - skip_serializing_if on all Option fields (no more null in YAML) - fix severity comparison: Ord is Critical(0)<Major(1)<Minor(2)<Info(3), so 'at or above min_severity' uses <= not >= - fix should_block in engine/review.rs and commands/scan.rs - fix display filter in commands/review.rs (consistent direction) - add config set --help, --global flag - design docs: CONFIG-REDESIGN.md, FEATURE-DESIGN.md
…verity fix - Fix global config path (~/.config/cora → ~/.cora/config.yaml) - Document --global flag, base_url key, config priority chain - Add Authentication section with auth.toml details - Update CI/CD section with composite action example - Fix CONTRIBUTING: Rust version, project structure, test example - Add Unreleased changelog entry for all changes
- Fix API key TOML injection: use toml::Table instead of format!() - Add migration marker (.migrated) to prevent repeated filesystem I/O - Restrict global config file permissions to 0600 - Guard project config set: require git repo (no orphan files)
ajianaz
force-pushed
the
feat/config-redesign-and-severity-fix
branch
from
May 31, 2026 15:52
e827bdf to
b64d401
Compare
ajianaz
pushed a commit
that referenced
this pull request
May 31, 2026
- Fix API key TOML injection: use toml::Table instead of format!() - Add migration marker (.migrated) to prevent repeated filesystem I/O - Restrict global config file permissions to 0600 - Guard project config set: require git repo (no orphan files)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
config setkeys~/.cora/auth.toml(0o600)Bug Fixes
Critical: Severity comparison inverted
should_blockused>=on Ord values, but Critical(0) < Major(1) < Minor(2) < Info(3). This caused:Fixed to use
<=in all 3 locations.config set was non-functional
Wrote to
~/.cora/config.tomlbut loader only reads YAML. Now writes YAML.Changes
Summary by CodeRabbit
Release Notes
New Features
~/.cora/config.yamlwith configurable priority chain (CLI flags → environment variables → project config → global config → defaults).cora config set base_urlcommand support.~/.cora/auth.tomlwith secure permissions.Bug Fixes
Improvements
Documentation