Conversation
When users chose "Environment variable" option in Step 2 (Security) of the setup wizard, the secrets_crypto context was not initialized. This caused API key saves in Step 3 (LLM Provider) to fail silently, leaving users with no stored API key and a non-working LLM configuration. Changes: - Add secrets_master_key_hex field to SetupWizard to store the generated key - Initialize secrets_crypto when env var mode is chosen (like keychain mode) - Write SECRETS_MASTER_KEY to ~/.ironclaw/.env automatically via write_bootstrap_env - Update wizard message to inform users the key will be written automatically - Add regression tests for issue #666 Fixes #666 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Tianer Zhou <ezhoureal@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug in the setup wizard where API keys failed to save silently when the "Environment variable" security option was chosen. The changes ensure that the necessary cryptographic context is initialized, the generated master key is persisted correctly, and the user is informed of the automatic key saving process, leading to a robust and functional LLM configuration. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a critical bug in the setup wizard where the secrets crypto context was not being initialized for the environment variable mode. The changes are logical, and the inclusion of regression tests is a great addition to prevent this issue from recurring. I've identified one high-severity issue in the new tests related to test isolation, which could lead to flakiness due to shared global state, and have provided a detailed suggestion for resolution, referencing relevant testing guidelines.
Note: Security Review did not run due to the size of the PR.
Use a temporary directory with IRONCLAW_BASE_DIR env var instead of writing to the user's actual ~/.ironclaw/.env file. This prevents side effects and makes the test isolated and reliable. Co-Authored-By: Claude <noreply@anthropic.com>
a6f22cc to
7fa7e5e
Compare
zmanian
left a comment
There was a problem hiding this comment.
The fix addresses a real bug (issue #666) -- env var mode left secrets_crypto uninitialized, causing silent failures in subsequent wizard steps. The approach is correct.
Three issues:
-
Use
SecretStringforsecrets_master_key_hex-- The master encryption key is stored as a plainStringthat won't be zeroed on drop. The project usessecrecy::SecretStringextensively (e.g.,llm_api_key: Option<SecretString>on the same struct). The master key should beOption<SecretString>. -
Missing
inject_single_varcall -- After generating the key,SECRETS_MASTER_KEYwon't be visible tooptional_env()during the rest of the wizard session until the process restarts and reads the.envfile. Needcrate::config::inject_single_var("SECRETS_MASTER_KEY", &key_hex). -
First test is a tautology --
test_secrets_master_key_hex_field_initializedmanually sets fields then asserts they'reSome. It doesn't exercise any real code path. Consider testing that the crypto context can actually encrypt/decrypt with the generated key. The second test (bootstrap env) is good.
When users chose "Environment variable" option in Step 2 (Security) of the setup wizard, the secrets_crypto context was not initialized. This caused API key saves in Step 3 (LLM Provider) to fail silently, leaving users with no stored API key and a non-working LLM configuration.
Changes:
Fixes #666