feat(llm): add per-user model preferences and commands - #7439
Conversation
…wlist # Conflicts: # crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rs # crates/product/ironclaw_webui/src/webui_v2/router.rs
italic-jinxin
added
size: XL
|
🚅 Deployed to the ironclaw-pr-7439 environment in ironclaw-ci-preview
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds caller-scoped model preferences with filesystem persistence, assistant commands, WebUI routes, runtime wiring, model resolution, and replay preservation. It also excludes server diagnostic events from product-visible operator logs. ChangesUser model preferences
Operator log filtering
Estimated code review effort: 5 (Critical) | ~90+ minutes Mergeability Score: 🟡 Moderate · up to This PR adds per-user model preference persistence and changes recovery behavior; interrupted writes can leave sequence gaps that break recovered first-message handling, and lookup failures can be hidden, leaving recovery incomplete without a clear error. These are bounded but concrete runtime correctness risks requiring fixes or explicit owner acceptance before merge. 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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: docs
|
@claude review |
|
@ironloopai review |
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. ⬛ Final result · Stopped
Manual command by italic-jinxin · attempt 1 of 3 · stopped after 9m 44s IronLoop stopped because the pull request target branch or head changed while this Run was active. |
86292d0 to
786ea3a
Compare
…wlist # Conflicts: # crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rs
786ea3a to
06d79c7
Compare
|
@claude review |
|
@ironloopai review |
🧭 IronLoop Run · ReviewThis comment updates in place as the Run moves through its stages. 🟩 Final result · Completed
Manual command by italic-jinxin · attempt 1 of 3 · completed in 4m 11s IronLoop completed the review and posted it to GitHub. 🔗 Result |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…-preference # Conflicts: # crates/app/ironclaw_architecture_tests/tests/reborn_dependency_boundaries.rs # crates/product/ironclaw_assistant/src/inbound_turn.rs # crates/product/ironclaw_assistant/src/reborn_services.rs # crates/product/ironclaw_assistant/src/reborn_services/llm_config.rs # crates/product/ironclaw_assistant/tests/reborn_services_contract.rs # crates/product/ironclaw_webui/src/webui_v2/mod.rs # crates/product/ironclaw_webui/src/webui_v2/router.rs
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs (1)
6661-6665: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTest tenant isolation with the same user ID.
This test changes only the user ID. It cannot detect a regression that keys preferences by user alone. Create another router for
user-alphain a different tenant, then assert it remains unset before and after the first tenant selects or resets a model.The PR objective requires tenant isolation. As per path instructions, “Test through the caller” requires route-level coverage at the real caller seam.
🤖 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/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs` around lines 6661 - 6665, Extend the test around router_with_caller and caller_for_user to create a second user-alpha caller in a different tenant, then assert its model preference is unset before and after user-alpha selects and resets a model in the first tenant. Keep the assertions at the route-level caller seam and verify tenant isolation rather than only differing user IDs.Source: 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/domains/ironclaw_threads/src/filesystem_service.rs`:
- Around line 1766-1770: In the concurrent pending-claim branch of the message
acceptance flow, ensure the returned identifier uses the updated message
identity from message.message_id rather than the stale local message_id.
Preserve the persisted record.message_id through transcript writing and
submission, and add a regression test covering a losing concurrent accept that
resumes a pending intent and verifies the returned ID matches the persisted
transcript row.
In `@crates/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs`:
- Around line 980-992: Update the fallback in the USER_MODEL_PREFERENCE_VIEW
handler to return UserModelPreference with model unset (None) when no record
exists, rather than defaulting to "model-b". Adjust the initial and other-caller
assertions in this contract test to expect an empty preference object until that
caller selects a model.
---
Duplicate comments:
In `@crates/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs`:
- Around line 6661-6665: Extend the test around router_with_caller and
caller_for_user to create a second user-alpha caller in a different tenant, then
assert its model preference is unset before and after user-alpha selects and
resets a model in the first tenant. Keep the assertions at the route-level
caller seam and verify tenant isolation rather than only differing user IDs.
🪄 Autofix
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: ddf43cfe-c611-4fe3-b157-e4d41d4ea32d
📒 Files selected for processing (5)
crates/domains/ironclaw_threads/src/filesystem_service.rscrates/domains/ironclaw_threads/tests/filesystem_session_thread_contract.rscrates/product/ironclaw_operator/src/llm_admin/llm_config_service.rscrates/product/ironclaw_operator/src/llm_admin/user_model_preference_store.rscrates/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs
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 (3)
crates/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs (1)
882-892: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not mutate the test store before a failed invocation is returned.
If
next_invoke_responsecontainsErr, this branch inserts the preference before returning the error. A later GET can then observe state for a failed PUT. Move the insertion after successful response handling, or add an explicit commit-then-error contract and test it.This finding follows from the
StubServices::invokeordering in this file.🤖 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/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs` around lines 882 - 892, Update StubServices::invoke so the user_model_preferences insertion in the LLM_USER_MODEL_PREFERENCE_SET_CAPABILITY_ID branch occurs only after next_invoke_response has completed successfully; preserve the failed invocation response without mutating the test store, and ensure subsequent GETs cannot observe preferences from failed PUTs.crates/domains/ironclaw_threads/src/filesystem_service.rs (2)
1778-1792: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPropagate recovery lookup errors.
The
if let Ok(Some(accepted))pattern discards every error fromaccepted_message_from_idempotency_path. This hides filesystem and deserialization failures, then returns only the earlierwrite_new_messageerror. MatchOk(Some(...)),Ok(None), andErr(recovery_error)explicitly. Preserve the recovery error or attach it to the original error.As per path instructions: “Fail loud: flag silent-failure patterns.”
🤖 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/domains/ironclaw_threads/src/filesystem_service.rs` around lines 1778 - 1792, Update the recovery branch in the write_new_message error path to explicitly handle Ok(Some(accepted)), Ok(None), and Err(recovery_error) from accepted_message_from_idempotency_path. Preserve successful recovery, while propagating the recovery error or attaching it to the original write error instead of silently discarding it.Source: Path instructions
1681-1684: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist or reuse the reserved sequence during pending recovery.
The recovery record stores
message_idandreplay_metadata, but it does not storesequence. The fallback reserves a sequence at Line 1776 and writes the message later. If the reservation succeeds and the write fails, the next retry reserves a different sequence. Concurrent pending retries can consume additional sequence values. The first persisted message can then havesequence > 1, while Line 1797 treatssequence == 1as the first message.Make sequence reservation and message persistence atomic, or persist the reserved sequence in the recovery intent and reuse it. Add a filesystem caller-level regression test for failure after reservation and concurrent pending resumes.
As per coding guidelines: “Persisted state must remain reconstructible after interruption; test conflicts, retry exhaustion, restart, and partial-failure behavior at the public domain-operation or typed-wrapper seam.”
Also applies to: 1776-1777
🤖 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/domains/ironclaw_threads/src/filesystem_service.rs` around lines 1681 - 1684, Update the pending-recovery flow around the recovery record construction and the fallback reservation near the message write to persist the reserved sequence and reuse it on retries, or make reservation and persistence atomic. Ensure concurrent or interrupted resumes cannot consume new sequence values and that the first persisted message retains sequence 1. Add a filesystem caller-level regression test through the public domain-operation or typed-wrapper seam covering failure after reservation and concurrent pending resumes.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.
Inline comments:
In `@crates/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs`:
- Line 6724: Add a same-user/different-tenant caller in the test around the
existing model-a preference setup, issue its GET after model-a is stored, and
assert an empty JSON object to verify tenant isolation. Keep the existing
different-user assertion unchanged, using the test’s established
caller-construction and request helpers.
---
Outside diff comments:
In `@crates/domains/ironclaw_threads/src/filesystem_service.rs`:
- Around line 1778-1792: Update the recovery branch in the write_new_message
error path to explicitly handle Ok(Some(accepted)), Ok(None), and
Err(recovery_error) from accepted_message_from_idempotency_path. Preserve
successful recovery, while propagating the recovery error or attaching it to the
original write error instead of silently discarding it.
- Around line 1681-1684: Update the pending-recovery flow around the recovery
record construction and the fallback reservation near the message write to
persist the reserved sequence and reuse it on retries, or make reservation and
persistence atomic. Ensure concurrent or interrupted resumes cannot consume new
sequence values and that the first persisted message retains sequence 1. Add a
filesystem caller-level regression test through the public domain-operation or
typed-wrapper seam covering failure after reservation and concurrent pending
resumes.
In `@crates/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs`:
- Around line 882-892: Update StubServices::invoke so the user_model_preferences
insertion in the LLM_USER_MODEL_PREFERENCE_SET_CAPABILITY_ID branch occurs only
after next_invoke_response has completed successfully; preserve the failed
invocation response without mutating the test store, and ensure subsequent GETs
cannot observe preferences from failed PUTs.
🪄 Autofix
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: 64ecf64b-6c74-476b-bdff-4b9f512a67ce
📒 Files selected for processing (2)
crates/domains/ironclaw_threads/src/filesystem_service.rscrates/product/ironclaw_webui/tests/webui_v2_handlers_contract.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/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs (1)
6660-6802: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd composed authentication tests for the model-preference routes.
webui_v2_handlers_contract.rsinjectsProductSurfaceCallerdirectly. Existingwebui_v2_apptests cover authentication only on other routes. Add valid and invalid bearer tests for both preference routes, and assert the authenticated tenant/user reaches the service. Keep the existing isolation test.This follows the “Trusted-ingress seal” and “Test through the caller” invariants in
.claude/rulesandcrates/product/ironclaw_webui/AGENTS.md.🤖 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/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs` around lines 6660 - 6802, Extend user_model_preference_routes_are_caller_scoped_and_do_not_require_admin with composed-authentication coverage for both GET and PUT model-preference routes, including valid and invalid bearer-token cases. Exercise the routes through the webui_v2_app authentication path rather than directly injected ProductSurfaceCaller instances, and assert valid requests propagate the authenticated tenant and user to the service while invalid requests are rejected. Preserve the existing caller-isolation assertions.Sources: Coding guidelines, Path instructions, Learnings
🤖 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/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs`:
- Around line 6660-6802: Extend
user_model_preference_routes_are_caller_scoped_and_do_not_require_admin with
composed-authentication coverage for both GET and PUT model-preference routes,
including valid and invalid bearer-token cases. Exercise the routes through the
webui_v2_app authentication path rather than directly injected
ProductSurfaceCaller instances, and assert valid requests propagate the
authenticated tenant and user to the service while invalid requests are
rejected. Preserve the existing caller-isolation assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3133ce8f-c28e-4fa2-ad14-5803e4051728
📒 Files selected for processing (1)
crates/product/ironclaw_webui/tests/webui_v2_handlers_contract.rs
* feat(llm): add tenant model selection policy * fix(composition): move model policy store to operator * test(llm): specify per-user model preference behavior * feat(llm): persist per-user model preferences * feat(llm): add per-user model commands * fix(llm): address model preference review findings * fix(logging): preserve model preference error causes * fix(model): apply user preferences to channel turns * test(channel): cover model preference handoff * fix(composition): keep model config wiring concrete * fix(model): preserve resolved model across inbound replay * fix: preserve accepted model across replay failures * test(model): restore caller-scoped preference coverage * fix(model): close preference review gaps * fix(model): preserve concurrent replay identity * test(model): cover cross-tenant preference isolation * fix(cli): honor saved user model preference
This branch was successfully deployed
2 participants
Summary
/model,/model use <model>, and/model default.Linked Issue
Closes #7420
Validation
cargo test -p ironclaw_assistant --test product_commands_contractcargo test -p ironclaw_assistant --test reborn_services_contract member_model_preference_commands_update_only_the_callers_preferencecargo test -p ironclaw_webui --test webui_v2_descriptors_contractSecurity Impact
Preference reads and writes are scoped to the authenticated tenant and user. Ordinary members cannot update another user's preference or modify the workspace allowlist.
Database Impact
No database schema or migration changes. Preferences use caller-scoped filesystem persistence and are retained when reset.
Blast Radius
Product contracts, caller-scoped persistence, composition wiring, model resolution, WebUI APIs, and product commands.
Rollback Plan
Revert the three #7420 commits. Persisted preference files become unused and do not affect the workspace default.
Review track
Track C — caller-scoped persistence, authorization, and runtime model-selection change.