Skip to content

Public-readiness fixes (Tasks 1–5 combined)#47

Merged
baanish merged 11 commits into
masterfrom
cursor/public-readiness-93e4
Jul 1, 2026
Merged

Public-readiness fixes (Tasks 1–5 combined)#47
baanish merged 11 commits into
masterfrom
cursor/public-readiness-93e4

Conversation

@baanish

@baanish baanish commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Summary

Combined integration of all five public-readiness workstreams:

Task Branch Focus
1 cursor/init-secret-hardening-93e4 Init secret handling + config recovery
2 cursor/cron-install-safety-93e4 Cron install safety
3 cursor/shell-wrapper-exit-status-93e4 Shell wrapper exit status
4 cursor/public-docs-cleanup-93e4 README / release docs
5 cursor/release-metadata-hygiene-93e4 Cargo metadata + MSRV CI

Tasks 1 and 2 both touched install_cron in main.rs; merged cleanly with the cron-safety helpers retained.

Test plan

  • cargo test (full suite)
  • cargo fmt --all -- --check
  • cargo clippy --all-targets --locked -- -D warnings
  • cargo package --locked --offline
  • Docs drift rg check
Open in Web Open in Cursor 

cursoragent and others added 9 commits July 1, 2026 09:27
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@cursor[bot], you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Free

Run ID: 1f2f2f01-0112-4560-918c-c6702fd44880

📥 Commits

Reviewing files that changed from the base of the PR and between fcf57af and c8b42a4.

📒 Files selected for processing (11)
  • .github/workflows/ci.yml
  • Cargo.toml
  • README.md
  • docs/PRE_RELEASE_REVIEW.md
  • src/ai/client.rs
  • src/atomic.rs
  • src/config.rs
  • src/main.rs
  • src/secret.rs
  • src/shell.rs
  • tests/cli.rs

Note

🎁 Summarized by CodeRabbit Free

Your organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login.

Comment @coderabbitai help to get the list of available commands.

@cursor cursor Bot marked this pull request as ready for review July 1, 2026 09:40
Comment thread src/main.rs Outdated
let output = Command::new("crontab")
.arg("-l")
.output()
.context("Failed to run `crontab -l`")?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WARNING: Installing cron now aborts qr init when the crontab binary is absent

Command::new("crontab").arg("-l").output() returns an Err (not a non-zero exit) when the crontab binary is not installed (minimal containers, some WSL/CI images). The previous implementation had a dedicated Err(_) arm that printed the cron_line and let init continue; this rewrite propagates the error via ?, so an explicit "yes" to the cron prompt fails the whole qr init after the config and wrapper have already been written (scan and price fetch never run). Consider restoring a graceful fallback that prints the manual cron line and logs the failure to stderr when crontab cannot be spawned.


Reply with @kilocode-bot fix it to have Kilo Code address this issue.

@kilo-code-bot

kilo-code-bot Bot commented Jul 1, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental review (commit c8b42a4d) found no new issues. The previously raised env-override recreation concern (P1) and shared keychain-account concern (P2) are both resolved by the new changes:

  • P1 resolvedexecute_init now detects a corrupt config via AppConfig::load_file_without_env(&config_path), which parses config.toml without applying QR_* overrides. An invalid env override (e.g. QR_SCAN_DEPTH=not-a-number) no longer marks a valid file as corrupt, so qr init fails fast without clobbering the existing config. Covered by the new init_keeps_valid_config_when_env_override_is_invalid test.
  • P2 resolved — keychain accounts are now role-scoped: account_for(role, api_key_env, default) produces primary:<env> / fallback:<env>, so two providers sharing the same env-var name no longer overwrite each other. Lookup uses get_for_role, which retains a legacy bare-account fallback for primary keys stored before role-prefixing shipped. Covered by updated init_stores_fallback_key_in_keychain_when_requested and new secret.rs unit tests.
Files Re-verified (5 files)
  • src/ai/client.rs
  • src/config.rs
  • src/main.rs
  • src/secret.rs
  • tests/cli.rs
Previous Review Summaries (2 snapshots, latest commit b99e5f7)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit b99e5f7)

Status: No Issues Found | Recommendation: Merge

The previous warning is resolved: install_cron now catches the crontab spawn error, prints the manual cron line plus a stderr warning, and lets qr init continue (returns Ok(())).

Files Reviewed (1 file)
  • src/main.rs

Previous review (commit eab16e4)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
src/main.rs 569 install_cron now propagates a spawn error via ?, aborting the whole qr init when crontab is absent; the previous code had a graceful fallback that printed the manual cron line.
Files Reviewed (10 files)
  • src/main.rs - 1 issue
  • src/shell.rs - 0 issues
  • src/atomic.rs - 0 issues
  • src/config.rs - 0 issues
  • src/secret.rs - 0 issues
  • tests/cli.rs - 0 issues
  • Cargo.toml - 0 issues
  • .github/workflows/ci.yml - 0 issues
  • README.md - 0 issues
  • docs/PRE_RELEASE_REVIEW.md - 0 issues

Fix these issues in Kilo Cloud


Reviewed by glm-5.2-short · Input: 22K · Output: 4.7K · Cached: 294.7K

Restore the pre-refactor behavior for spawn failures: print the manual
cron line and continue init instead of aborting. List failures after a
successful spawn still abort before any crontab write.

Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
@baanish

baanish commented Jul 1, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b99e5f7a04

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/main.rs Outdated
Comment on lines +283 to +287
Some(AppConfig::load_from_env_with_path(config_path.clone()))
} else {
None
};
let needs_recreate = !config_path.exists() || existing.as_ref().is_some_and(Result::is_err);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Avoid recreating config on env override errors

Because load_from_env_with_path also applies QR_* overrides, any invalid override such as QR_SCAN_DEPTH=abc makes existing an Err even when config.toml itself is valid. needs_recreate then treats that as a corrupt config and prompts through a rewrite, so running qr init in that environment can overwrite a good config and still fail on the same env error when reloading; the corruption check should parse the file without env overrides or classify env errors separately.

Useful? React with 👍 / 👎.

Comment thread src/main.rs Outdated

fn prompt_fallback_ai_config() -> Result<FallbackAiConfig> {
let (protocol, base_url, model, api_key, api_key_env) = prompt_provider_fields("fallback")?;
let api_key = maybe_store_key_in_keychain(protocol, &api_key_env, api_key)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Keep fallback keychain accounts distinct

This stores the fallback key with the same account derivation as the primary (api_key_env or the protocol default). When both providers use the same default, e.g. two openai-compatible endpoints with different keys, accepting the keychain prompt for the fallback overwrites the primary keychain entry; because both config api_key fields are then empty, the primary later resolves the fallback key and fails against the primary endpoint. The keychain account needs to include provider role or otherwise reject duplicate accounts when the entered secrets differ.

Useful? React with 👍 / 👎.

- Parse config.toml without QR_* overrides when deciding if init should recreate
- Fail clearly on invalid env overrides instead of treating them as corrupt config
- Prefix keychain accounts with primary/fallback role; legacy lookup for primary
- Add tests for invalid-env init behavior and shared-default-env keychain storage

Co-authored-by: Aanish Bhirud <baanish@users.noreply.github.com>
@cursor cursor Bot force-pushed the cursor/public-readiness-93e4 branch from ca3787a to c8b42a4 Compare July 1, 2026 10:37
@baanish baanish merged commit 452a2d0 into master Jul 1, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants