Repository navigation
feat(reborn-cli): onboarding journey — keychain master key, two-prompt setup, login link - #6174
Conversation
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds Reborn onboarding with interactive LLM credential provisioning, local secret-store and keychain handling, WebUI token login, service lifecycle integration, richer status output, stored-key runtime fallback, and exact Docker configuration migration. ChangesReborn onboarding and runtime integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
github-actions
Bot
added
scope: dependencies
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the CLI onboarding experience by introducing interactive prompts for LLM credentials, OS-keychain master-key provisioning, and a CLI-token login endpoint (GET /login?token=) for WebUI authentication. Feedback on these changes highlights a compilation failure when the libsql feature is disabled, a potential terminal redirection issue during interactivity checks, and an opportunity to log suppressed keychain failures at the debug! level to aid troubleshooting.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6ba49f961
ℹ️ 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".
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ IronLoop Review: reviewer
Review at a glance
| Verdict | Blocking | Notes | Inline | Head |
|---|---|---|---|---|
| ❌ Changes requested | 6 | 0 | 6 | d6ba49f96131 |
Head: d6ba49f961319e681099b9bf3d07b9b2247de90e
Next: Fix the blocking findings, push the PR branch, then re-run this reviewer.
Run details
Status: Current
Needs human: no
Needs validation: no
Summary
The stack-layer diff is reviewable, but it introduces multiple blocking correctness and security issues in credential provisioning, master-key recovery, login-link generation, and dependency boundaries.
Findings
Blocking: 6 / Notes: 0
Blocking findings
1. ❌ [HIGH] Stored onboarding keys do not make selected providers bootable
Location: crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs:384-386
Writing directly through LlmKeyStore and then calling RebornProviderAdmin::set_provider bypasses the provider-overlay update performed by LlmConfigService. For required-key providers such as OpenAI or Anthropic, api_key_required remains true, so the next run/serve fails during provider resolution for a missing environment variable before apply_startup_stored_llm_key can read the saved key. For NearAI, endpoint selection occurs while the key is still absent, so a stored cloud API key is applied after the private/session endpoint has already been selected. Route onboarding through a composition-owned credential/config operation equivalent to the canonical settings write path, and test OpenAI and NearAI end-to-end with their key environment variables cleared.
2. ❌ [HIGH] Keychain backend failures can strand existing encrypted secrets
Location: crates/ironclaw_reborn_composition/src/factory.rs:3640-3643
Every keychain error is treated as a missing key. After a successful keychain-backed onboarding has encrypted secrets with key K1, a later temporary lock, permission denial, or backend outage causes this branch to generate K2 and persist it in the dotfile. The dotfile then permanently wins over the recovered keychain, making the K1 rows unreadable and allowing new K2 rows to create split-key state. Generate only for SecretError::NotFound (including the suppression path) and propagate other keychain errors; add a regression covering a keychain error after an encrypted row already exists.
3. ❌ [MEDIUM] An empty API-key answer permanently poisons the default NearAI setup
Location: crates/ironclaw_reborn_cli/src/commands/onboard/prompts.rs:94-97
Pressing Enter at the masked prompt returns an empty string, which onboarding stores as a real credential. already_configured_outcome subsequently sees its metadata and skips future prompts, while apply_stored_api_key turns NearAI's key into Some(""); NearAiChatProvider therefore enters API-key mode and sends an empty bearer instead of using session authentication. Reject empty/whitespace keys for required-key providers, or treat an empty answer as no stored credential for optional/keyless providers, with caller-level coverage.
4. ❌ [HIGH] Status JSON exposes the operator bearer and session-signing secret
Location: crates/ironclaw_reborn_cli/src/dto.rs:17-22
StatusDto derives Serialize, so ironclaw-reborn status --json now emits the complete `/login?token=[redacted] URL. That token is both the operator bearer and the HMAC session-signing secret normally protected by a 0600 file. Generic status JSON is commonly captured in diagnostics and automation logs, turning an observability command into a credential-exfiltration path. Keep the raw link out of serialized status output and require an explicit, interactive login-link action if it must be displayed.
5. ❌ [MEDIUM] Printed login links ignore the effective serve configuration
Location: crates/ironclaw_reborn_cli/src/webui_token.rs:204-214
The link always uses the token file and 127.0.0.1:3000, but serve gives an environment token precedence, honors configured listen host/port, and does not mount /login when SSO is enabled. Consequently supported deployments can receive a link that returns 401, connects to the wrong port/interface, or reaches an unmounted route. Generate or suppress the link using the same effective token, listener, and SSO resolution as serve, and cover each precedence case.
6. ❌ [MEDIUM] The new secrets dependency violates the enforced CLI boundary
Location: crates/ironclaw_reborn_cli/Cargo.toml:108
reborn_cli_binary_crate_stays_separate_from_v1_root asserts that the CLI's normal workspace dependencies are exactly composition, config, traces, and WebUI ingress. Adding ironclaw_secrets makes that existing architecture test deterministically fail and exposes a lower-level secret-store API directly to the command layer. Move keychain provisioning and LLM-key persistence behind a facade-shaped composition API rather than widening this boundary.
Developer follow-up
After fixing this feedback:
- Push the fix to this PR branch.
- Re-run this reviewer with
@ironloopai review --agent reviewerif you only changed this reviewer's findings. - Re-run all reviewers with
@ironloopai reviewwhen the fix may affect multiple areas.
|
🚅 Deployed to the ironclaw-pr-6174 environment in ironclaw-ci-preview
|
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Complete the standalone Reborn journey with two-prompt onboarding, encrypted key storage, background service startup, and CLI-token browser login.
Stats: 9 findings after dedup (9 selected; 5 body-only) across 5 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Parallel dispatch hit the active-agent cap; parent-context fallback completed the remaining lenses. Existing unresolved threads were checked and duplicate concerns were suppressed. Validation: exact-head diff line mapping passed.
bugs
High Gate onboarding's LLM store/admin references by their features (crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs:403-421, confidence 99) — anchor: crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs:403
The onboarding module is compiled for feature combinations that omit root-llm-provider and/or libsql, but already_configured_outcome and LocalDevLlmKeyStoreOpener still reference RebornProviderAdmin, LlmKeyStore, and open_local_dev_secret_store unconditionally. cargo check -p ironclaw_reborn_cli --no-default-features fails with E0425/E0433, and the libsql-only matrix fails on the root-LLM-provider symbols.
bugs
Medium Build login links from the effective listen address (crates/ironclaw_reborn_cli/src/webui_token.rs:204-214, confidence 92) — (no diff position — body only) — anchor: crates/ironclaw_reborn_cli/src/webui_token.rs:204
login_link always formats http://127.0.0.1:3000, while serve honors [webui].listen_host, [webui].listen_port, and CLI overrides. On an existing or customized install, onboarding/status can print a URL that does not reach the running service.
conventions
Medium Do not discard secret-store open errors (crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs:413-417, confidence 98) — anchor: crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs:416
The idempotent probe treats every secret-store open/migration/encryption error as if the key were simply absent and silently proceeds to prompt. This hides actionable storage failures and can make onboarding appear healthy while the configured store is unusable.
maintainability
Medium Split credential provisioning out of the onboarding command module (crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs:324-351, confidence 90) — anchor: crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs:324
The onboarding module now combines command orchestration, service startup, marker serialization, LLM secret persistence, terminal policy, and extensive tests in an 858-line file. The mixed responsibilities increase coupling and make the command flow harder to review and extend.
performance
High Serialize master-key check and creation (crates/ironclaw_reborn_cli/src/commands/onboard/master_key.rs:57-64, confidence 96) — anchor: crates/ironclaw_reborn_cli/src/commands/onboard/master_key.rs:58
Concurrent onboard and serve processes can both observe no key, generate different keys, and overwrite the OS keychain entry. If either process opens the encrypted store between those writes, the store can be encrypted with a key that is later replaced and become unreadable on the next startup.
security
Medium Do not expose the long-lived bearer token in URLs (crates/ironclaw_reborn_cli/src/webui_token.rs:204-214, confidence 90) — (no diff position — body only) — anchor: crates/ironclaw_reborn_cli/src/webui_token.rs:209
The login link embeds the long-lived WebUI bearer directly in the query string. Browser history, terminal captures, HTTP access logs, proxy logs, and referrer leakage can disclose it; anyone obtaining the value can authenticate as the operator.
tests
High Add a production-wired CLI login integration test (crates/ironclaw_reborn_cli/src/commands/serve.rs:570-670, confidence 95) — (no diff position — body only) — anchor: crates/ironclaw_reborn_cli/src/commands/serve.rs:594
The new route has isolated handler tests, but no whole-path test starts the production serve wiring and verifies GET /login followed by POST /auth/session/exchange. The smoke coverage only checks that a link is printed, so a mount/composition regression could leave the advertised browser login unusable.
tests
Medium Cover session-store failure without minting a ticket (crates/ironclaw_reborn_webui_ingress/src/cli_token_login.rs:262-279, confidence 90) — (no diff position — body only) — anchor: crates/ironclaw_reborn_webui_ingress/src/cli_token_login.rs:262
The login handler returns 500 when create_session fails, but the route tests do not exercise that branch. Without caller-level coverage, a future change could redirect or retain a ticket after session creation failed.
tests
Medium Cover malformed and oversized exchange requests (crates/ironclaw_reborn_webui_ingress/src/cli_token_login.rs:299-334, confidence 85) — (no diff position — body only) — anchor: crates/ironclaw_reborn_webui_ingress/src/cli_token_login.rs:316
The public exchange endpoint declares a body limit but has no route tests for malformed JSON, a missing or blank ticket, or an oversized body. These are externally reachable parsing and boundary cases.
d6ba49f to
bfa0123
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@crates/ironclaw_reborn_cli/src/commands/serve.rs`:
- Around line 1023-1031: Update resolve_webui_user_id_raw to use
present_unicode_env_var for the environment lookup instead of
env::var(...).ok(), and propagate VarError::NotUnicode rather than falling back
to default_owner_id. Adjust the function and its callers as needed to return the
lookup error, and add a regression case covering a non-Unicode user-ID
environment value that verifies authentication fails closed.
In `@crates/ironclaw_reborn_cli/src/webui_token.rs`:
- Around line 394-404: The login_link function must stop embedding the
persistent bearer/session-signing secret in the URL; mint a short-lived,
single-use bootstrap ticket through an authenticated local channel or
authorization header and include only that ticket in the returned URL. Update
crates/ironclaw_reborn_cli/src/webui_token.rs lines 394-404 accordingly, and
update crates/ironclaw_reborn_webui_ingress/src/cli_token_login.rs lines 239-260
to consume and invalidate the bootstrap ticket rather than authenticating the
persistent bearer from ?token=, failing closed at the authentication boundary.
In `@crates/ironclaw_reborn_cli/tests/smoke.rs`:
- Around line 3547-3590: Remove the master-key dotfile creation and LLM key
seeding block before the serve invocation so the smoke test exercises the
missing-key fallback through the real caller. Update the test to assert that
serve creates LOCAL_DEV_SECRETS_MASTER_KEY_PATH after binding, while preserving
stored-key overlay coverage separately if required.
- Around line 1512-1554: Replace the duplicated stderr polling, 15-second
timeout, listener-banner detection, and child cleanup in both new serve tests
with calls to the existing wait_for_serve_banner helper. Update each test’s
setup or invocation as needed so the helper performs the shared waiting
behavior, preserving the current failure expectations and cleanup.
- Around line 2167-2168: Update the response-body reading in the smoke test
around reader and body to propagate read_to_string failures instead of
discarding them. Use the test’s existing error-return mechanism and add context
identifying the auth response body read, so timeout or truncation errors surface
directly.
In `@crates/ironclaw_reborn_webui_ingress/src/cli_token_login.rs`:
- Around line 92-139: Restrict CliTokenLoginConfig::redirect_after to a
validated same-origin relative path: make the configuration fields private,
validate both constructor/setter inputs before build_cli_token_login creates the
mount, and reject schemes, authorities, and paths beginning with //. Preserve
authentication’s fail-closed behavior and add a regression test proving external
and scheme-relative redirect targets are rejected.
In `@crates/ironclaw_reborn_webui_ingress/tests/cli_token_login_route.rs`:
- Around line 96-177: Replace the direct Router::oneshot coverage in the
token-login tests with a random-port real HTTP setup that starts serve_webui_v2
and uses a reqwest::Client. Preserve coverage for valid-token redirect, ticket
exchange and authenticated bearer lookup, single-use replay rejection,
wrong-token rejection without a redirect, and missing-token rejection through
the production listener and security middleware.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a43fc26e-3f59-4d32-85a8-cf3cd182dc51
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/Cargo.lock
📒 Files selected for processing (25)
crates/ironclaw_reborn_cli/Cargo.tomlcrates/ironclaw_reborn_cli/src/commands/config/init.rscrates/ironclaw_reborn_cli/src/commands/onboard.rscrates/ironclaw_reborn_cli/src/commands/onboard/master_key.rscrates/ironclaw_reborn_cli/src/commands/onboard/mod.rscrates/ironclaw_reborn_cli/src/commands/onboard/prompts.rscrates/ironclaw_reborn_cli/src/commands/serve.rscrates/ironclaw_reborn_cli/src/commands/service/mod.rscrates/ironclaw_reborn_cli/src/commands/status.rscrates/ironclaw_reborn_cli/src/dto.rscrates/ironclaw_reborn_cli/src/render/tests.rscrates/ironclaw_reborn_cli/src/webui_token.rscrates/ironclaw_reborn_cli/tests/extension.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/llm_admin/llm_key_store.rscrates/ironclaw_reborn_composition/src/llm_admin/provider_admin.rscrates/ironclaw_reborn_composition/src/test_support/local_dev_boot.rscrates/ironclaw_reborn_composition/tests/facade_factory.rscrates/ironclaw_reborn_webui_ingress/src/cli_token_login.rscrates/ironclaw_reborn_webui_ingress/src/lib.rscrates/ironclaw_reborn_webui_ingress/tests/cli_token_login_route.rscrates/ironclaw_secrets/src/keychain.rstests/integration/secrets.rs
💤 Files with no reviewable changes (1)
- crates/ironclaw_reborn_cli/src/commands/onboard.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs (1)
382-389: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck existing credentials before soft-skipping headless sessions.
A configured non-interactive rerun exits at Line 382 before
already_configured_outcome, so it is reported asSkippedNonInteractiveand the marker incorrectly listsllm_credentialsas pending.Proposed fix
- if !prompts.is_interactive() { - return Err(LlmCredentialPromptError::NonInteractive); - } - let admin = ironclaw_reborn_composition::RebornProviderAdmin::new(boot.clone()); if !force && let Some(outcome) = already_configured_outcome(&admin, home, store_opener)? { return Ok(outcome); } + + if !prompts.is_interactive() { + return Err(LlmCredentialPromptError::NonInteractive); + }Add an
OnboardCommand::executeregression covering an already-configured headless rerun. As per coding guidelines, side-effect gates must be tested through their caller.🤖 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 `@crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs` around lines 382 - 389, Update OnboardCommand::execute to call already_configured_outcome before rejecting non-interactive sessions, while preserving the NonInteractive error for unconfigured headless runs. Add a regression test through OnboardCommand::execute covering an already-configured headless rerun and verifying it is not reported as SkippedNonInteractive or pending llm_credentials.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs`:
- Around line 382-389: Update OnboardCommand::execute to call
already_configured_outcome before rejecting non-interactive sessions, while
preserving the NonInteractive error for unconfigured headless runs. Add a
regression test through OnboardCommand::execute covering an already-configured
headless rerun and verifying it is not reported as SkippedNonInteractive or
pending llm_credentials.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da6db1e2-cc4d-4ad2-9076-cd58887ac267
📒 Files selected for processing (8)
crates/ironclaw_reborn_cli/src/commands/onboard/master_key.rscrates/ironclaw_reborn_cli/src/commands/onboard/mod.rscrates/ironclaw_reborn_cli/src/commands/onboard/prompts.rscrates/ironclaw_reborn_cli/src/commands/status.rscrates/ironclaw_reborn_cli/src/dto.rscrates/ironclaw_reborn_cli/src/render/tests.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ironclaw_reborn_cli/tests/smoke.rs (1)
2358-2366: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse a file token so this test actually exercises the SSO guard.
The env token already makes
webui_token_source != File, so this test passes even if thesso_enabledcondition is removed. Seedreborn_home/webui-tokenand removeIRONCLAW_REBORN_WEBUI_TOKEN; then SSO is the sole reason/loginremains unmounted.As per coding guidelines, every bug fix needs a regression test through the production caller.
🤖 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 `@crates/ironclaw_reborn_cli/tests/smoke.rs` around lines 2358 - 2366, Update the smoke test setup around reborn_command so it seeds the webui-token file under reborn_home with the test token, removes IRONCLAW_REBORN_WEBUI_TOKEN from the child environment, and preserves the existing /login assertion. Ensure the test reaches the production caller with webui_token_source set to File, making sso_enabled the sole condition that keeps /login unmounted.Sources: Coding guidelines, Path instructions
crates/ironclaw_reborn_cli/src/webui_token.rs (1)
500-511: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRead the token once through the checked file path.
webui_token_file_is_valid()validates one open, thenfs::read_to_string()reopens the path without the regular-file, size, or no-follow guarantees. A path swap bypasses those checks, whileunwrap_or(false)and.ok()?hide the resulting I/O error. ReturnResult<Option<String>>and build the link from oneread_token_file_checked()result.As per path instructions, security-relevant I/O must fail loudly; checked token-file guarantees must not be weakened by a second unchecked read.
🤖 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 `@crates/ironclaw_reborn_cli/src/webui_token.rs` around lines 500 - 511, The login_link function must return Result<Option<String>> and replace the separate webui_token_file_is_valid and fs::read_to_string calls with a single read_token_file_checked result. Propagate security-relevant I/O errors instead of using unwrap_or(false) or .ok()?, while preserving None for an invalid or absent token file and constructing the URL from the checked read’s token.Sources: Coding guidelines, Path instructions
🤖 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 `@crates/ironclaw_reborn_cli/src/commands/onboard/mod.rs`:
- Around line 195-207: Add the required inline `// silent-ok: <reason>`
exemption adjacent to the `.ok().flatten()` call in the `config_file` resolution
within the onboarding flow, preserving the existing fallback behavior and
rationale. Do not alter the surrounding configuration loading logic.
In `@crates/ironclaw_reborn_cli/src/runtime/mod.rs`:
- Around line 568-616: The Serve-only fallback in
resolve_reborn_runtime_llm_with_stored_key_fallback performs an unbounded
open_local_dev_secret_store probe. Replace it with the existing bounded,
noninteractive secret retrieval mechanism or add an explicit timeout around the
block_on_cli operation, and convert timeout/keychain failures into an actionable
startup error rather than hanging. Preserve fail-fast behavior for non-Serve
callers and add coverage exercising the timeout through the Serve runtime-input
caller.
In `@crates/ironclaw_reborn_cli/src/webui_token.rs`:
- Around line 407-409: The env_token_is_active function currently hides invalid
non-Unicode environment values by treating them as inactive. Change it to return
Result<bool>, map NotPresent to false, and propagate NotUnicode as an error so
status/onboard match serve; update all callers accordingly and add the Unix
regression test for a non-Unicode token value.
- Around line 327-330: Replace the derived Debug implementation on
ResolvedWebuiToken with a manual implementation that preserves Debug support for
expect_err while redacting value. Keep source visible in diagnostics, but ensure
the bearer/session-signing key is never formatted verbatim.
In `@crates/ironclaw_reborn_cli/tests/smoke.rs`:
- Around line 3862-3868: Replace the blocking Command::output call in the serve
regression test with child spawning and bounded polling via try_wait(). Use a
deadline, terminate the child on timeout, and assert the process exits with the
expected failure so a successful bind cannot hang CI.
In `@crates/ironclaw_reborn_composition/src/llm_admin/llm_catalog.rs`:
- Around line 171-182: Update resolve_llm_selection_allow_missing_key and
related resolution flow so the keyless retry occurs only when normal resolution
returns the exact ApiKeyEnvUnset error. Preserve ApiKeyEnvUnconfigured for
required-key providers lacking api_key_env, and add a regression test covering
that failure rather than allowing the provider to resolve.
---
Outside diff comments:
In `@crates/ironclaw_reborn_cli/src/webui_token.rs`:
- Around line 500-511: The login_link function must return
Result<Option<String>> and replace the separate webui_token_file_is_valid and
fs::read_to_string calls with a single read_token_file_checked result. Propagate
security-relevant I/O errors instead of using unwrap_or(false) or .ok()?, while
preserving None for an invalid or absent token file and constructing the URL
from the checked read’s token.
In `@crates/ironclaw_reborn_cli/tests/smoke.rs`:
- Around line 2358-2366: Update the smoke test setup around reborn_command so it
seeds the webui-token file under reborn_home with the test token, removes
IRONCLAW_REBORN_WEBUI_TOKEN from the child environment, and preserves the
existing /login assertion. Ensure the test reaches the production caller with
webui_token_source set to File, making sso_enabled the sole condition that keeps
/login unmounted.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5dd9db62-746c-4be8-bcff-76498beb4603
📒 Files selected for processing (10)
crates/ironclaw_reborn_cli/src/commands/onboard/mod.rscrates/ironclaw_reborn_cli/src/commands/serve.rscrates/ironclaw_reborn_cli/src/commands/status.rscrates/ironclaw_reborn_cli/src/dto.rscrates/ironclaw_reborn_cli/src/render/tests.rscrates/ironclaw_reborn_cli/src/runtime/mod.rscrates/ironclaw_reborn_cli/src/webui_token.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/llm_admin/llm_catalog.rs
…age root resolve_reborn_runtime_llm_with_stored_key_fallback checked the bare reborn home for a stored LLM key, but onboarding writes it under <home>/<profile>/ (local_runtime_storage_root) — the same two-database class of bug fixed for onboarding in d7f84ea, missed at this call site. serve now reads the same root onboarding writes to, and treats a not-yet-created storage root as "no stored key" (fail through to the original ApiKeyEnvUnset) instead of surfacing a raw filesystem error. The existing regression test masked this because it seeded the stored key at the bare root, matching the bug instead of the fix; it now seeds at the runtime root to match what onboarding actually writes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…in race NearAiLoginStateStore::issue only pruned expired entries, growing unbounded within the 15-min TTL if callers mint redirects but never complete them. Cap it and evict-oldest, mirroring LoginTicketStore's MAX_TICKETS pattern. start_codex_login's "already in flight" check and attempt-map write were in separate lock sections with an unlocked initiate_device_code() await between them, so two concurrent calls for the same tenant+user could both start a device-code flow, with the second's insert silently orphaning the first's tracked attempt id. Reserve a placeholder entry under the lock before requesting a device code; a concurrent caller sees the placeholder and fails fast instead of racing a second request. Also adds the two ironclaw_webui LoginTicketStore tests from the same ledger: MAX_TICKETS eviction and single-redemption under concurrent take(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Fixed the 4 accepted triage items in 2571a42 and a3f8a17:
|
…d rename LocalDevKeychainMasterKeyOutcome -> KeychainMasterKeyOutcome changed the composition public facade intentionally (LocalDev* typename ratchet); the snapshot pins that facade and must follow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_reborn_composition/src/llm_admin/llm_config_service.rs (1)
804-815: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftFinalize the reservation only if
attempt_idis still current.If device-code initiation exceeds the reservation TTL, another caller can prune it and reserve a new attempt. Lines 804-815 then unconditionally overwrite that newer reservation, recreating the orphaned-flow race this fix targets.
Use an ID-checked in-place update; if the reservation was superseded, return without replacing it. Add a caller-level concurrent test through
start_codex_loginusing a controllable session-manager seam—the helper-only tests never exercise this finalization race.Proposed finalization guard
- { + let finalized = { let mut attempts = self.codex_login_attempts.lock().await; - attempts.insert( - attempt_key.clone(), - CodexLoginAttempt { - id: attempt_id, - user_code: Some(login.user_code.clone()), - verification_uri: Some(login.verification_uri.clone()), - expires_at: Instant::now() + CODEX_LOGIN_ATTEMPT_TTL, - }, - ); + if let Some(attempt) = attempts + .get_mut(&attempt_key) + .filter(|attempt| attempt.id == attempt_id) + { + attempt.user_code = Some(login.user_code.clone()); + attempt.verification_uri = Some(login.verification_uri.clone()); + attempt.expires_at = Instant::now() + CODEX_LOGIN_ATTEMPT_TTL; + true + } else { + false + } + }; + if !finalized { + return Err(LlmConfigServiceError::Internal); }As per coding guidelines, side-effect-gating concurrency fixes require regression coverage through the production caller, not only the helper.
As per path instructions, test through the real call site when a helper gates a side effect.Also applies to: 1700-1768
🤖 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 `@crates/ironclaw_reborn_composition/src/llm_admin/llm_config_service.rs` around lines 804 - 815, Update the reservation finalization in start_codex_login and the corresponding flow around lines 1700-1768 to mutate the existing CodexLoginAttempt only when its stored ID still matches attempt_id; if superseded or absent, return without overwriting the newer reservation. Add a concurrent caller-level regression test through start_codex_login using a controllable session-manager seam to reproduce initiation exceeding the reservation TTL and verify the newer reservation remains intact.Sources: Coding guidelines, Path instructions
🤖 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 `@crates/ironclaw_reborn_composition/src/llm_admin/llm_config_service.rs`:
- Around line 775-797: Preserve the source errors in both
`OpenAiCodexSessionManager::new` and `manager.initiate_device_code()` failure
branches by binding each error, logging it with relevant context, then returning
the existing sanitized `LlmConfigServiceError::Internal` after cleanup. Do not
alter the successful paths or attempt-removal behavior.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/llm_admin/llm_config_service.rs`:
- Around line 804-815: Update the reservation finalization in start_codex_login
and the corresponding flow around lines 1700-1768 to mutate the existing
CodexLoginAttempt only when its stored ID still matches attempt_id; if
superseded or absent, return without overwriting the newer reservation. Add a
concurrent caller-level regression test through start_codex_login using a
controllable session-manager seam to reproduce initiation exceeding the
reservation TTL and verify the newer reservation remains intact.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 30d967a3-96d7-41fa-9f83-377b6f6b2649
📒 Files selected for processing (5)
crates/ironclaw_reborn_cli/src/runtime/mod.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/src/llm_admin/llm_config_service.rscrates/ironclaw_webui/src/cli_token_login.rsdocs/plans/composition-pubuse.snapshot
henrypark133
left a comment
henrypark133
left a comment
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Add standalone Reborn CLI onboarding that configures an LLM, provisions secrets, starts a service, and provides secure browser login.
Stats: 10 findings from 10 raw reviewer findings; 8 after live-thread duplicate suppression; 7 after same-line dedup across 6 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0.
bugs
- Medium Master-key check looks in the wrong directory (
crates/ironclaw_reborn_cli/src/commands/onboard/master_key.rs:56-58, confidence 93) — anchor: crates/ironclaw_reborn_cli/src/commands/onboard/master_key.rs:56
The resolver stores and reads.reborn-local-dev-secrets-master-keyunder the profile-specific runtime storage root, but onboarding checks only<reborn_home>/.reborn-local-dev-secrets-master-key. Rerunning onboarding therefore fails to detect an existing cached key and may overwrite the OS-keychain copy with a different key, leaving keychain recovery inconsistent with the encrypted database. - High Stored-key boot reload still requires an env API key (
crates/ironclaw_reborn_composition/src/llm_admin/llm_reload.rs:41-42, confidence 98) — anchor: crates/ironclaw_reborn_composition/src/llm_admin/llm_reload.rs:41
The boot-time reload uses strictresolve_reborn_runtime_llm, so configurations created by onboarding with an encrypted stored key but no API-key environment variable fail withApiKeyEnvUnset. The runtime remains on the placeholder gateway and chat cannot use the onboarded provider.
security
- High Do not place the long-lived bearer token in the login URL (
crates/ironclaw_reborn_cli/src/webui_token.rs:517-522, confidence 90) — anchor: crates/ironclaw_reborn_cli/src/webui_token.rs:518
The generated link embeds the reusable WebUI bearer in a query parameter. Browser history, local access logs, proxy logs, and Referer propagation can disclose it; anyone obtaining it gains operator access directly and through the session-minting route.
Also flagged by: security/Medium
performance
- Medium Keychain provisioning races can replace the active master key (
crates/ironclaw_reborn_composition/src/factory.rs:3923-3927, confidence 91) — anchor: n/a
Provisioning checkshas_master_key()and then generates/stores a key in separate operations. Two concurrent onboarding processes can both observe absence and overwrite the keychain with different keys; subsequent secret-store writes can then be encrypted under different keys, making previously written secrets undecryptable. - Medium Codex login attempts have no global capacity bound (
crates/ironclaw_reborn_composition/src/llm_admin/llm_config_service.rs:174-174, confidence 86) — anchor: n/a
The per-caller Codex attempt map is pruned only when a new request arrives and has no maximum size. Distinct authenticated users can each reserve a 15-minute attempt, spawning a polling task and retaining map state, so memory and background-task usage grow with the number of active callers rather than a fixed bound.
maintainability
- Medium Onboarding APIs push provider_admin.rs past 1,100 lines (
crates/ironclaw_reborn_composition/src/llm_admin/provider_admin.rs:275-318, confidence 95) — anchor: crates/ironclaw_reborn_composition/src/llm_admin/provider_admin.rs:275; crates/ironclaw_reborn_composition/CLAUDE.md
The new menu, environment-detection, and probe orchestration is onboarding-specific but is appended to the existing provider-admin facade, growing the file from 605 to 1,154 lines and combining unrelated responsibilities.
tests
- Medium Candidate probe wrapper lacks caller-level coverage (
crates/ironclaw_reborn_composition/src/llm_admin/provider_admin.rs:408-431, confidence 95) — anchor: crates/ironclaw_reborn_composition/src/llm_admin/provider_admin.rs:408
The new public probe_candidate method is never called by a test. Existing tests cover only the lower helper's unknown-adapter branch and inject fake LlmProbe implementations, so the registry lookup, protocol/key/model forwarding, successful probe, and persisted-before-probe onboarding flow are unverified.
RebornLlmReloadAdapter::reload used the strict resolver first, so an api_key_required provider (e.g. openai) configured only through the onboarding stored-key path (no env var) failed closed on ApiKeyEnvUnset before ever reaching the stored-key lookup, leaving the placeholder gateway wired forever on both boot-time reload and Settings -> Inference save. Fall back to the stored-key-tolerant resolution when a stored key exists for the provider, mirroring the CLI serve boot path's existing fallback. Adds a regression pin driving boot with openai configured via the stored-key path and no env var. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dex finalize guard - master_key.rs: provision_master_key now checks the dotfile at local_runtime_storage_root, not the bare RebornHome root, mirroring 2571a42's fix for the same two-root-class mismatch. The bare-root check was always false, so onboarding re-attempted keychain provisioning on every rerun. Adds a regression test seeding the dotfile at the runtime root and asserting the no-op path. - llm_credentials.rs / mod.rs: corrects the env-accept disclosure text (doc comment and the onboarding println) — the detected API key is persisted to the encrypted secret store so a background service can resolve it, not left solely in the env var as previously stated. - llm_config_service.rs: finalize_codex_login_slot guards a stale device-code finalize from clobbering a newer reservation that was made after the stale attempt's TTL lapsed. Adds a regression test driving reserve -> expire -> reserve -> stale finalize and asserting the newer reservation survives untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review-comment batch fixed and pushed (a946277):
Note: the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/ironclaw_reborn_composition/src/llm_admin/llm_config_service.rs`:
- Around line 149-173: Update finalize_codex_login_slot to return whether the
existing map entry is still present and owned by attempt_id, returning false for
absent or mismatched entries and true after successful finalization. In the
production caller that handles codex device-code initiation, check this result
and abort before spawning the polling task when finalization fails. Add hermetic
device-code-double coverage through the public caller, including the superseded
reservation being removed before the stale request returns.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c55f79c-ae02-496d-8a9a-47a14a07ae23
📒 Files selected for processing (6)
crates/ironclaw_reborn_cli/src/commands/onboard/llm_credentials.rscrates/ironclaw_reborn_cli/src/commands/onboard/master_key.rscrates/ironclaw_reborn_cli/src/commands/onboard/mod.rscrates/ironclaw_reborn_cli/tests/smoke.rscrates/ironclaw_reborn_composition/src/llm_admin/llm_config_service.rscrates/ironclaw_reborn_composition/src/llm_admin/llm_reload.rs
RebornProviderAdmin::probe_candidate had zero coverage: onboard tests always inject fake probes. This seam already produced a real bug (empty base URL -> always "could not reach"). Add a local loopback HTTP stub asserting the probe hits the CONFIGURED base URL with the ENTERED key (200 -> ok, 401 -> not ok). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/ironclaw_reborn_composition/src/llm_admin/provider_admin.rs`:
- Around line 1303-1329: Update probe_candidate_reports_401_as_not_ok to retain
the request receiver from spawn_models_stub, await it with a timeout, and assert
the captured request targets the stub endpoint and includes the expected
authorization details before asserting outcome.ok is false. Ensure the test
specifically distinguishes the stub’s 401 response from URL or connection
failures and fails before the corresponding fix.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ffaf818d-5895-4d01-adf5-2ab6a6672fa5
📒 Files selected for processing (1)
crates/ironclaw_reborn_composition/src/llm_admin/provider_admin.rs
finalize_codex_login_slot only blocked overwriting a DIFFERENT attempt id; an absent entry (superseding reservation failed and removed itself before the stale device-code request returned) passed through and let the stale finalize reinsert itself. Require the entry to still be present and match, returning false otherwise so the caller aborts without inserting or spawning a poller. Also moves probe_candidate's live-stub tests out of provider_admin.rs into tests/provider_admin_probe.rs: the architecture boundary test reborn_product_api_crates_do_not_bind_http_ingress text-scans every src/ file for a loopback bind with no #[cfg(test)] awareness, and the in-module stub tripped it (matches webui_v2_serve.rs's existing pattern). While moving, fix probe_candidate_reports_401_as_not_ok to await the captured request and assert method/path + Authorization — previously it discarded the receiver, so a transport failure would have passed identically to a real 401. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/ironclaw_reborn_composition/tests/provider_admin_probe.rs`:
- Around line 145-147: Prevent indefinite waits in both request-capture sites in
provider_admin_probe.rs: wrap each request_rx await with tokio::time::timeout
using a 2-second Duration, including the probe path at lines 145-147 and the 401
path at lines 187-189, while preserving the existing failure expectations.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 55f93e58-9daa-43ab-b80c-65176157d0dc
📒 Files selected for processing (3)
crates/ironclaw_reborn_composition/src/llm_admin/llm_config_service.rscrates/ironclaw_reborn_composition/src/llm_admin/provider_admin.rscrates/ironclaw_reborn_composition/tests/provider_admin_probe.rs
| let captured = request_rx | ||
| .await | ||
| .expect("stub must have received exactly one request"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Prevent test suite hangs on probe failures.
Awaiting the oneshot receiver without a timeout will cause the test to hang indefinitely if the probe fails to connect or hits the wrong endpoint (since the spawned stub task blocks on listener.accept() and never drops the sender). Wrap the await in tokio::time::timeout so a network regression fails the test quickly rather than stalling CI runs.
crates/ironclaw_reborn_composition/tests/provider_admin_probe.rs#L145-L147: Wrap the await intokio::time::timeout(std::time::Duration::from_secs(2), request_rx).crates/ironclaw_reborn_composition/tests/provider_admin_probe.rs#L187-L189: Wrap the await with the same timeout to prevent hangs on the 401 path.
📍 Affects 1 file
crates/ironclaw_reborn_composition/tests/provider_admin_probe.rs#L145-L147(this comment)crates/ironclaw_reborn_composition/tests/provider_admin_probe.rs#L187-L189
🤖 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 `@crates/ironclaw_reborn_composition/tests/provider_admin_probe.rs` around
lines 145 - 147, Prevent indefinite waits in both request-capture sites in
provider_admin_probe.rs: wrap each request_rx await with tokio::time::timeout
using a 2-second Duration, including the probe path at lines 145-147 and the 401
path at lines 187-189, while preserving the existing failure expectations.
Reborn onboarding: menu → key → model → background service → browser
Makes
ironclaw-rebornusable standalone from a source build. One command sets up everything; the browser is the only chat surface.The journey
Gmail/Slack are configured lazily later (
config set, PR C) — onboarding never asks.Design decisions
config.toml [llm.default]is the single LLM source of truth, written only by explicit acts (onboard,config set, webui settings). Nothing pre-seeds it: fresh interactive installs always see the menu. Env vars are detected and offered as a one-keypress seed; headless+env seeds silently; headless+no-env leaves it unset with a teaching message. Runtime resolution (slot → env → degraded boot with warning) is unchanged./login?token=link) carry operator capability; SSO/admin-created sessions provably cannot escalate (provenance recorded at mint, tripwire tests both directions).private.near.aisession branch was deleted (reborn never wired session renewal; the branch was a dead end that broke probes and runtime endpoint selection).status --jsonredacts the login link;Debugon the token type redacts.Railway / production safety
Serve boot with env vars is byte-identical (pinned by regression tests). The shipped docker config no longer bakes an LLM slot (env drives;
NEARAI_MODELnow actually takes effect); a narrowly-gated one-time entrypoint migration strips only byte-identical legacy stubs from existing volumes (backup kept, operator-modified configs untouched).Service correctness found by live-testing
WorkingDirectory=<reborn_home>/workspace(cwd=/ crash-loop fixed; boot-from-cwd smoke test + crash-repro tripwire)statusqueries real service state, suppresses the login link when the service is downIRONCLAW_REBORN_HOMEonly)Testing
Full-chain capstones drive the real binary: onboard → serve boots → GET login link → ticket exchange → bearer authorizes a protected AND an operator-gated route; stored-key boot (openai + nearai); headless env-seed boot; model choice reaching runtime; token precedence; idempotent reruns (key and no-key providers); escalation pins. Port-race in the serve smoke tests fixed (serialized spawning), retiring the recurring CI flakes.
Review ledger
~70 review threads triaged, fixed-or-adjudicated, and resolved across 5 rounds (3 bot reviewers + self-review). Declines carry simplicity rationale per the repo's day-1 design laws. Follow-ups filed: #6183 (config-aware login link), #6184 (per-IP rate limiting behind proxies).
Known limitations (deliberate)
Model budget accountant regression (disclosed, deferred to Reborn: model cost table / budget accountant not rebuilt by LLM reload chokepoint (regression from #6174 boot convergence) #6215): the boot convergence onto the reload chokepoint means the static model cost table is no longer derived at boot, so
model_budget_accountantis never constructed and model spending limits are unenforced on Reborn until Reborn: model cost table / budget accountant not rebuilt by LLM reload chokepoint (regression from #6174 boot convergence) #6215 lands (swappable cost table refreshed by the same reload path).No tunnel integration (v1's tunnel module not ported — external tunnel +
IRONCLAW_REBORN_WEBUI_BASE_URLfor public exposure)Non-loopback bind refused on the local-dev profile (by design)
OAuth/AWS/file-flow providers (bedrock, ollama, gemini_oauth, …) configured via
config set, not the menuWindows: foreground
serveonly (no service backend, no keychain)🤖 Generated with Claude Code