feat(reborn): expose user-scoped tool settings#5256
Conversation
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5256
📝 WalkthroughSummary by CodeRabbit
WalkthroughWebUI v2 adds authenticated ChangesWebUI v2 settings/tools surface
Chat send blocking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 introduces new user-scoped tool approval settings endpoints (/api/webchat/v2/settings/tools) to allow regular multi-user sessions to read and update global auto-approve settings and per-tool overrides without requiring operator-level access. It updates the backend routes, descriptors, handlers, and tests, as well as the frontend API calls to leverage these new endpoints. The review feedback suggests adding validation to the state field in the set_settings_tool_permission handler to ensure only valid permission states are accepted.
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.
| /// `POST /api/webchat/v2/settings/tools/{capability_id}` | ||
| pub async fn set_settings_tool_permission( | ||
| State(state): State<WebUiV2State>, | ||
| Extension(caller): Extension<WebUiAuthenticatedCaller>, | ||
| Path(SettingsToolPermissionPath { capability_id }): Path<SettingsToolPermissionPath>, | ||
| Json(body): Json<SettingsToolPermissionRequest>, | ||
| ) -> Result<Json<RebornOperatorConfigGetResponse>, WebUiV2HttpError> { | ||
| let key = validate_operator_config_key(format!("tool.{capability_id}"))?; | ||
| let response = state | ||
| .services() | ||
| .set_operator_config_key( | ||
| caller, | ||
| key, | ||
| RebornOperatorConfigSetRequest { | ||
| value: serde_json::json!({ "state": body.state }), | ||
| }, | ||
| ) | ||
| .await?; | ||
| Ok(Json(response)) | ||
| } |
There was a problem hiding this comment.
The state field in SettingsToolPermissionRequest is passed directly to the configuration service without validation. To prevent malformed or invalid state values from being persisted, validate that body.state is one of the allowed permission states ("default", "always_allow", "ask_each_time", or "disabled") before processing the request.
/// `POST /api/webchat/v2/settings/tools/{capability_id}`
pub async fn set_settings_tool_permission(
State(state): State<WebUiV2State>,
Extension(caller): Extension<WebUiAuthenticatedCaller>,
Path(SettingsToolPermissionPath { capability_id }): Path<SettingsToolPermissionPath>,
Json(body): Json<SettingsToolPermissionRequest>,
) -> Result<Json<RebornOperatorConfigGetResponse>, WebUiV2HttpError> {
match body.state.as_str() {
"default" | "always_allow" | "ask_each_time" | "disabled" => {}
_ => {
return Err(RebornServicesError::from(WebUiInboundValidationError::new(
"state",
WebUiInboundValidationCode::InvalidValue,
))
.into());
}
}
let key = validate_operator_config_key(format!("tool.{capability_id}"))?;
let response = state
.services()
.set_operator_config_key(
caller,
key,
RebornOperatorConfigSetRequest {
value: serde_json::json!({ "state": body.state }),
},
)
.await?;
Ok(Json(response))
}
github-actions
Bot
added
scope: docs
think-in-universe
changed the title
|
🚅 Deployed to the ironclaw-pr-5256 environment in ironclaw-ci-preview
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5256
![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: d6185b855a
ℹ️ 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".
|
|
||
| export async function fetchSettingsExport() { | ||
| const data = await apiFetch(OPERATOR_CONFIG_BASE); | ||
| const data = await apiFetch(SETTINGS_TOOLS_BASE); |
There was a problem hiding this comment.
This endpoint change is only in the source modules; the committed SPA bundle that index.html serves (/v2/dist/app.js) was not regenerated. I checked static/dist/app.js and it still contains /api/webchat/v2/operator/config with no /api/webchat/v2/settings/tools, so in multi-user deployments where operator routes are absent or gated the Settings Tools tab will keep calling the operator config API and fail instead of using the new caller-scoped route. Please rebuild and commit static/dist after editing static/js/**.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_webui_v2/tests/webui_v2_descriptors_contract.rs`:
- Around line 716-766: The /settings/tools contract tests currently cover only
route metadata and do not verify that the new public surface is limited to
tool-specific keys. Add a caller-driven regression test around the existing
handlers in handlers.rs (especially list_operator_config and
set_operator_config_key) that exercises the real GET/POST call sites and asserts
GET returns only tool approval settings while POST can only modify
agent.auto_approve_tools and tool.* entries, preventing unrelated caller-scoped
config from leaking through RebornOperatorConfig* DTOs.
In `@crates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rs`:
- Around line 2975-3041: The settings tools route test is using StubServices in
a way that bypasses the production caller-scoped behavior, so it cannot detect
the real 503/service-unavailable path. Update
settings_tool_routes_do_not_require_operator_capability to exercise the same
service behavior as the production facade for set_operator_config_key and
list_operator_config, or add a separate test that uses the real reborn_services
implementation, so the assertions verify the route through the actual caller
path rather than a stubbed success path.
🪄 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: e5617d9b-4c3d-4903-a871-b9a53ba4ab5d
📒 Files selected for processing (11)
FEATURE_PARITY.mdcrates/ironclaw_webui_v2/CLAUDE.mdcrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/src/lib.rscrates/ironclaw_webui_v2/src/router.rscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_operator_route_predicate_contract.rscrates/ironclaw_webui_v2_static/static/js/pages/settings/hooks/useSettings.jscrates/ironclaw_webui_v2_static/static/js/pages/settings/lib/settings-api.js
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5256
github-actions
Bot
added
size: L
think-in-universe
force-pushed
the
codex/multi-tenant-tool-approvals-main
branch
from
9191719 to
f6f8a88
Compare
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5256
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_webui_v2_static/static/js/pages/chat/lib/chat-input.test.mjs`:
- Around line 230-256: The null-path test for ChatInput only verifies the draft
is preserved, but it should also cover the retry behavior in
ChatInput.handleSend. Update the test that uses onSend: async () => null to
press Enter a second time after the first failed send and assert that onSend is
invoked again, while still confirming the draft is not cleared. Use the existing
renderChatInput, templateProps, and textarea keydown flow so the regression
where sending gets stuck blocked is covered.
🪄 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: cb891d9b-7852-4432-bb93-916e0dfaf26d
📒 Files selected for processing (4)
crates/ironclaw_webui_v2_static/static/js/pages/chat/chat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/chat-input.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat-input.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat.test.mjs
|
[BLOCK] Not ready for human final review yet. Blocking status:
Pass list of checks:
Action before human-ready:
|
think-in-universe
force-pushed
the
codex/multi-tenant-tool-approvals-main
branch
from
f6f8a88 to
8ae360e
Compare
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5256
github-actions
Bot
added
size: XL
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ironclaw_webui_v2_static/static/js/pages/settings/lib/settings-api.js (1)
29-40: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the path key as
capability_id, notname.
POST /api/webchat/v2/settings/tools/{capability_id}is keyed by capability id, but this client poststool.name.toolFromConfigEntry()currently prefersvalue.nameover the key suffix, so any display label returned by the server will turn permission writes into the wrong route.Suggested diff
export function toolFromConfigEntry(entry) { if (!entry?.key?.startsWith(TOOL_PREFIX)) return null; const value = entry.value || {}; - const name = value.name || entry.key.slice(TOOL_PREFIX.length); + const capability_id = entry.key.slice(TOOL_PREFIX.length); + const name = value.name || capability_id; return { + capability_id, name, description: value.description || "", state: normalizeToolState(value.state), @@ -export async function updateToolPermission(name, state) { +export async function updateToolPermission(capabilityId, state) { const normalized = normalizeToolUpdateState(state); - const data = await apiFetch(`${SETTINGS_TOOLS_BASE}/${encodeURIComponent(name)}`, { + const data = await apiFetch( + `${SETTINGS_TOOLS_BASE}/${encodeURIComponent(capabilityId)}`, + { method: "POST", body: JSON.stringify({ state: normalized }), - }); + } + ); return { success: true, tool: toolFromConfigEntry(data.entry), entry: data.entry }; }Also applies to: 161-166
🤖 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_webui_v2_static/static/js/pages/settings/lib/settings-api.js` around lines 29 - 40, The settings tool mapping is using the display name instead of the capability key, which causes writes to target the wrong route. Update toolFromConfigEntry() so it preserves the path-derived capability identifier from entry.key as capability_id and does not prefer value.name for that field; then adjust the settings API caller that posts tool updates to use capability_id rather than tool.name. Keep name as the UI label if needed, but make the identifier used for permission writes come from the TOOL_PREFIX key suffix.
🤖 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_webui_v2_static/static/js/pages/settings/hooks/useSettings.js`:
- Around line 48-50: The settings invalidation for agent.auto_approve_tools is
only handled in the direct save path, so mirror the same
queryClient.invalidateQueries call in the import flow as well. Update
importSettings()/importMutation.onSuccess in useSettings to also invalidate the
settings-tools query whenever the imported payload changes
agent.auto_approve_tools, alongside the existing settings-export invalidation,
so the Tools tab refreshes after import.
In `@crates/ironclaw_webui_v2/src/handlers.rs`:
- Around line 1329-1407: The settings handlers list_settings_tools,
set_settings_tools_auto_approve, and set_settings_tool_permission currently only
require WebUiAuthenticatedCaller, so they can execute even when
WebUiV2Capabilities was not injected by the host. Update these handler
signatures to also extract WebUiV2Capabilities as an axum::Extension, matching
the existing authenticated handler pattern, so the request fails closed with a
500 at the boundary when host composition is incomplete.
- Around line 1370-1403: `SettingsToolPermissionRequest` is still using a raw
`String` for the fixed `state` contract, which forces manual validation in
`validate_settings_tool_permission_state`. Replace that field with a serde enum
representing the four allowed values, using canonical wire names via
`#[serde(rename_all = "snake_case")]` or explicit renames. Then update
`set_settings_tool_permission` to rely on typed deserialization and serialize
the enum back into the config value instead of string matching.
In
`@crates/ironclaw_webui_v2/tests/webui_v2_operator_route_predicate_contract.rs`:
- Line 7: The operator predicate contract test is only excluding the two POST
settings tool routes, so it would miss a regression that incorrectly treats the
GET tools route as operator-only. Update the exclusion set in
webui_v2_operator_route_predicate_contract.rs for the predicate coverage around
WEBUI_V2_ROUTE_SET_SETTINGS_TOOL_PERMISSION and
WEBUI_V2_ROUTE_SET_SETTINGS_TOOLS_AUTO_APPROVE to also include
WEBUI_V2_ROUTE_LIST_SETTINGS_TOOLS, so the full /api/webchat/v2/settings/tools
surface stays caller-authenticated in the test.
---
Outside diff comments:
In
`@crates/ironclaw_webui_v2_static/static/js/pages/settings/lib/settings-api.js`:
- Around line 29-40: The settings tool mapping is using the display name instead
of the capability key, which causes writes to target the wrong route. Update
toolFromConfigEntry() so it preserves the path-derived capability identifier
from entry.key as capability_id and does not prefer value.name for that field;
then adjust the settings API caller that posts tool updates to use capability_id
rather than tool.name. Keep name as the UI label if needed, but make the
identifier used for permission writes come from the TOOL_PREFIX key suffix.
🪄 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: 9a8fa76c-b5bc-4a9d-aadc-9dbfee2bebb1
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (15)
FEATURE_PARITY.mdcrates/ironclaw_webui_v2/CLAUDE.mdcrates/ironclaw_webui_v2/src/descriptors.rscrates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/src/lib.rscrates/ironclaw_webui_v2/src/router.rscrates/ironclaw_webui_v2/tests/webui_v2_descriptors_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_operator_route_predicate_contract.rscrates/ironclaw_webui_v2_static/static/js/pages/chat/chat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/components/chat-input.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat-input.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/chat.test.mjscrates/ironclaw_webui_v2_static/static/js/pages/settings/hooks/useSettings.jscrates/ironclaw_webui_v2_static/static/js/pages/settings/lib/settings-api.js
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5256
|
@claude review |
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_webui_v2_static/static/js/pages/chat/hooks/useChat.js (1)
249-265: 🎯 Functional Correctness | 🟡 MinorSync the send-blocking refs before paint.
setIsProcessingState(false)/setPendingGateState(null)clear the thread-switch state during render, butisProcessingRef.currentandpendingGateRef.currentonly update inReact.useEffect. That leavessend()able to read the previous thread’s busy/gate state and reject the first submit on the new thread. Move those ref writes toReact.useLayoutEffect, or keep the refs in lockstep with the reset path.🤖 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_webui_v2_static/static/js/pages/chat/hooks/useChat.js` around lines 249 - 265, The send-blocking refs are updated too late, so `send()` can still see stale busy/gate state after a thread switch. In `useChat`, keep `isProcessingRef.current` and `pendingGateRef.current` in sync with the reset path by moving their updates from `React.useEffect` to `React.useLayoutEffect`, or otherwise update the refs immediately when `setIsProcessingState(false)` and `setPendingGateState(null)` run. Ensure the ref values match the new thread before paint so the first submit on the new thread is not rejected.
🤖 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_webui_v2_static/static/js/pages/chat/hooks/useChat.js`:
- Around line 249-265: The send-blocking refs are updated too late, so `send()`
can still see stale busy/gate state after a thread switch. In `useChat`, keep
`isProcessingRef.current` and `pendingGateRef.current` in sync with the reset
path by moving their updates from `React.useEffect` to `React.useLayoutEffect`,
or otherwise update the refs immediately when `setIsProcessingState(false)` and
`setPendingGateState(null)` run. Ensure the ref values match the new thread
before paint so the first submit on the new thread is not rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 84a36cc9-484d-4a26-a939-6147b3fef2e9
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
crates/ironclaw_webui_v2_static/static/js/pages/chat/components/chat-input.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs
Code Review Results — PR #5256Found 3 issues: 1. [HIGH:75] Information Disclosure — Response data leakage from mutation endpointsLocation: The The backend response Fix: Apply the same filtering pattern to both mutation handlers: response.entries.retain(|e| {
e.key == "agent.auto_approve_tools" || e.key.starts_with("tool.")
});2. [MEDIUM:75] Type-driven design violation — stringly-typed permission stateLocation: The The PR also validates a Fix: Replace 3. [MEDIUM:HIGH] Unbounded path parameter allocationLocation: The Fix: Validate const MAX_CAPABILITY_ID_LEN: usize = 128 - SETTINGS_TOOL_CONFIG_PREFIX.len();
if capability_id.len() > MAX_CAPABILITY_ID_LEN {
return Err(/* validation error */);
}
let key = format!("{SETTINGS_TOOL_CONFIG_PREFIX}{capability_id}");Positive Notes✅ Route patterns, descriptors, and operator-capability boundary are correct |
|
@claude review |
|
Found 8 issues: 2 HIGH (inefficient filtering, JS response mismatch), 6 MEDIUM (type safety, validation, error handling, performance), 3 LOW (docs, tests, rate limits). See detailed analysis in https://github.com/anthropics/ironclaw/pull/5256 |
|
HIGH SEVERITY:
|
|
MEDIUM SEVERITY:
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5256
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_webui_v2_static/static/js/pages/settings/hooks/useSettings.js`:
- Around line 63-66: The settings import handler in useSettings should
invalidate the tools query for per-tool permission imports as well, not just
agent.auto_approve_tools. Update the importedKeys check in the settings import
logic so that it also detects any tool.* entries before calling
queryClient.invalidateQueries for ["settings-tools"], ensuring the Tools tab
refreshes when imported payloads change individual tool permissions.
🪄 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: 41106e22-97db-4b08-bfe1-83b1f713df5e
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (4)
crates/ironclaw_webui_v2/src/handlers.rscrates/ironclaw_webui_v2/tests/webui_v2_handlers_contract.rscrates/ironclaw_webui_v2/tests/webui_v2_operator_route_predicate_contract.rscrates/ironclaw_webui_v2_static/static/js/pages/settings/hooks/useSettings.js
| const importedKeys = Object.keys(payload?.settings || {}); | ||
| if (importedKeys.includes("agent.auto_approve_tools")) { | ||
| queryClient.invalidateQueries({ queryKey: ["settings-tools"] }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Invalidate ["settings-tools"] for imported tool.* keys too.
The new tools query covers both agent.auto_approve_tools and tool.* entries, so an import that only changes per-tool permissions leaves the Tools tab stale until refresh.
Suggested diff
onSuccess: (_data, payload) => {
queryClient.invalidateQueries({ queryKey: ["settings-export"] });
const importedKeys = Object.keys(payload?.settings || {});
- if (importedKeys.includes("agent.auto_approve_tools")) {
+ if (
+ importedKeys.some(
+ (key) =>
+ key === "agent.auto_approve_tools" || key.startsWith("tool.")
+ )
+ ) {
queryClient.invalidateQueries({ queryKey: ["settings-tools"] });
}
if (importedKeys.some((key) => RESTART_REQUIRED_KEYS.has(key))) {
setNeedsRestart(true);
}📝 Committable suggestion
‼️ 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.
| const importedKeys = Object.keys(payload?.settings || {}); | |
| if (importedKeys.includes("agent.auto_approve_tools")) { | |
| queryClient.invalidateQueries({ queryKey: ["settings-tools"] }); | |
| } | |
| const importedKeys = Object.keys(payload?.settings || {}); | |
| if ( | |
| importedKeys.some( | |
| (key) => | |
| key === "agent.auto_approve_tools" || key.startsWith("tool.") | |
| ) | |
| ) { | |
| queryClient.invalidateQueries({ queryKey: ["settings-tools"] }); | |
| } |
🤖 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_webui_v2_static/static/js/pages/settings/hooks/useSettings.js`
around lines 63 - 66, The settings import handler in useSettings should
invalidate the tools query for per-tool permission imports as well, not just
agent.auto_approve_tools. Update the importedKeys check in the settings import
logic so that it also detects any tool.* entries before calling
queryClient.invalidateQueries for ["settings-tools"], ensuring the Tools tab
refreshes when imported payloads change individual tool permissions.
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 (2)
crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.js (2)
248-255: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear the submit latch on thread changes.
Line 248 resets per-thread transient state, but
submitBusyRefsurvives the switch. If the prior thread already returned arun_id, the only normal clear isonRunSettled; afteruseSSEmoves to the newthreadId, that old settlement may never be observed, leaving sends in the new thread returningnull. Reset or thread-scope this ref without writing it in the render-time reset block.Proposed direction
React.useEffect(() => { resetToolActivityState(toolActivityStateRef); locallyResolvedGatesRef.current.clear(); + submitBusyRef.current = false; }, [threadId]);🤖 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_webui_v2_static/static/js/pages/chat/hooks/useChat.js` around lines 248 - 255, Clear the submit latch when switching threads, since submitBusyRef can stay latched from the previous thread and block sends in the new one. Update useChat’s thread-change handling around the stateThreadId reset so the latch is reset or scoped per thread, but do not write to the ref inside the render-time reset block; instead tie the cleanup to the thread transition and/or the existing onRunSettled flow so useSSE and submit handling stay in sync.
417-471: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSet
submitBusyRefbefore the first await.Line 421 awaits
resolveConnectAction()before Line 471 flips the local latch, so two same-tick sends can both pass Lines 409-413 and create duplicate threads/messages. Flip the latch immediately after the guard, and clear it for channel-connect early returns and preflight failures.Proposed direction
if ( submitBusyRef.current || isProcessingRef.current || activeRunBlocksSend ) { return null; } + submitBusyRef.current = true; // Channel-connect slash commands... + try { if (stagedAttachments.length === 0) { const connectable = await resolveConnectAction(content); if (connectable) { setChannelConnectAction(connectable); + submitBusyRef.current = false; return { channel_connect_action: connectable }; } } setChannelConnectAction(null); let sendThreadId = targetThreadId || threadId; if (!sendThreadId) { const created = await createThreadRequest(); queryClient.invalidateQueries({ queryKey: ["threads"] }); sendThreadId = created?.thread?.thread_id; if (!sendThreadId) { throw new Error("createThread returned no thread_id"); } } + } catch (err) { + submitBusyRef.current = false; + throw err; + } - submitBusyRef.current = true; updateCurrentThread((prev) => [...prev, pendingRenderMessage]);🤖 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_webui_v2_static/static/js/pages/chat/hooks/useChat.js` around lines 417 - 471, `useChat.js` has a race in the send flow: `submitBusyRef` is set too late, after `resolveConnectAction()` and thread creation work have already started. Move the latch assignment to immediately after the initial guard in the send path so `submitBusyRef.current` blocks same-tick reentry before any await in the send handler, and make sure it is reset on the channel-connect early return and any preflight/error exit paths. Use the existing `submitBusyRef`, `resolveConnectAction`, `createThreadRequest`, and `setChannelConnectAction` flow to locate and update the send handler.
🤖 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_webui_v2_static/static/js/pages/chat/hooks/useChat.js`:
- Around line 248-255: Clear the submit latch when switching threads, since
submitBusyRef can stay latched from the previous thread and block sends in the
new one. Update useChat’s thread-change handling around the stateThreadId reset
so the latch is reset or scoped per thread, but do not write to the ref inside
the render-time reset block; instead tie the cleanup to the thread transition
and/or the existing onRunSettled flow so useSSE and submit handling stay in
sync.
- Around line 417-471: `useChat.js` has a race in the send flow: `submitBusyRef`
is set too late, after `resolveConnectAction()` and thread creation work have
already started. Move the latch assignment to immediately after the initial
guard in the send path so `submitBusyRef.current` blocks same-tick reentry
before any await in the send handler, and make sure it is reset on the
channel-connect early return and any preflight/error exit paths. Use the
existing `submitBusyRef`, `resolveConnectAction`, `createThreadRequest`, and
`setChannelConnectAction` flow to locate and update the send handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 361e4f41-4f76-485c-bb05-5cd27458c3a9
⛔ Files ignored due to path filters (1)
crates/ironclaw_webui_v2_static/static/dist/app.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
crates/ironclaw_webui_v2_static/static/js/pages/chat/hooks/useChat.jscrates/ironclaw_webui_v2_static/static/js/pages/chat/lib/useChat-send.test.mjs
Rebase #4544 onto current main (106 commits of drift). Resolved 2 conflicts: product_workflow/src/lib.rs export list (kept both LifecycleSearchExtensionSummary from main and lifecycle_package_kind_label from #4544); FEATURE_PARITY.md (kept #4544's scoped-lifecycle clause on Hosted MCP, main's newer #5256 rows for NEAR AI MCP and Tool policies). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Fixes #5242
Replaces closed draft #5251 with a clean branch based directly on current
main(6af6b9318).Summary
GET /api/webchat/v2/settings/toolsPOST /api/webchat/v2/settings/toolsPOST /api/webchat/v2/settings/tools/{capability_id}operator_webui_config./operator/config.Security / tenancy
Tests
CARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/home/robert/ironclaw-dev-1/target cargo +1.92.0 test -p ironclaw_webui_v2 --features webui-v2-beta every_descriptor_matches_the_locked_policy_surfaceCARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/home/robert/ironclaw-dev-1/target cargo +1.92.0 test -p ironclaw_webui_v2 --features webui-v2-beta settings_tool_routes_do_not_require_operator_capabilityCARGO_INCREMENTAL=0 CARGO_TARGET_DIR=/home/robert/ironclaw-dev-1/target cargo +1.92.0 test -p ironclaw_webui_v2 --features webui-v2-beta operator_route_predicate_matches_operator_config_routes_onlygit diff --checkNote: an initial parallel test attempt failed because the local filesystem was full while compiling. I removed generated
target/debug/incrementalcache artifacts and reran the focused checks above successfully with incremental compilation disabled.