15 feature improve security#45
Merged
Merged
Conversation
- Added a new `api` module for user settings, including functions to fetch and update user settings related to skills hint maximum bytes. - Integrated user settings fetching and saving functionality into the `SkillsPanel` component, allowing users to adjust context size dynamically. - Enhanced the SkillsPanel UI to display and manage context size settings with appropriate error handling and loading states. - Updated the Tauri backend to handle new API routes for user settings, ensuring seamless interaction between the frontend and backend.
- Simplified the formatting of the `routing_gameinformer_news_includes_fetch_even_if_other_tools_match_news_token` test function for improved readability. - Adjusted the match statement for `ToolRoutePlan::Subset` to enhance clarity in the code structure.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
📝 WalkthroughWalkthroughThis pull request introduces OS-backed secure storage for bot tokens and MCP secrets, migrating from plaintext configuration files to native keychain/credential stores. It refactors connection persistence to separate metadata from secrets, implements user-configurable skill context byte limits with a UI control, adds keyword-based gating for explicit web search intent, and prevents redundant fetch operations on duplicate URLs within a single message. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPServer as HTTP Server
participant SecureStore as Secure Store<br/>(Keychain)
participant Repository as Bot<br/>Repository
participant AppState
Client->>HTTPServer: PUT /v1/connect<br/>(bot_token)
HTTPServer->>SecureStore: save_token(bot_id,<br/>token)
SecureStore-->>HTTPServer: Ok(())
HTTPServer->>Repository: persist(metadata)
alt Metadata Save Success
Repository-->>HTTPServer: Ok(())
HTTPServer-->>Client: 200 OK
else Metadata Save Failure
Repository-->>HTTPServer: Err(...)
HTTPServer->>SecureStore: delete_token(bot_id)<br/>(rollback)
SecureStore-->>HTTPServer: Ok(())
HTTPServer-->>Client: 500 Error
end
sequenceDiagram
participant App as App<br/>Startup
participant Repository as Bot<br/>Repository
participant SecureStore as Secure Store
participant MCP as MCP<br/>Service
participant AppState
App->>Repository: load(connection.json,<br/>migration_log)
alt Has Plaintext Token
Repository->>SecureStore: save_token(bot_id,<br/>legacy_token)
SecureStore-->>Repository: Ok(())
Repository->>Repository: persist metadata<br/>(remove token)
Repository-->>App: Ok(metadata)
else No Token
Repository-->>App: Ok(metadata)
end
App->>MCP: read_config(mcp.json)
MCP->>MCP: Migrate legacy<br/>passthrough secrets
MCP->>SecureStore: save_mcp_secret(...)<br/>for each migrated key
SecureStore-->>MCP: Ok(())
App->>SecureStore: warm_app_secrets(bot_ids,<br/>mcp_pairs)
SecureStore->>SecureStore: Load/merge unified<br/>secrets blob
SecureStore-->>App: Ok(())
App->>AppState: new(store_path)
AppState->>AppState: Initialize with<br/>preloaded secrets
AppState-->>App: Ready
sequenceDiagram
participant Client
participant SkillsPanel as Skills Panel<br/>(React)
participant SettingsAPI as Settings API
participant HTTPServer as HTTP Server
participant AppState
SkillsPanel->>SkillsPanel: Mount component
SkillsPanel->>SettingsAPI: fetchUserSettings(4000)
SettingsAPI->>HTTPServer: GET /v1/settings
HTTPServer->>AppState: Read state.skills_hint_max_bytes
HTTPServer-->>SettingsAPI: 200 + UserSettings<br/>(current, min, max, default)
SettingsAPI-->>SkillsPanel: UserSettings
SkillsPanel->>SkillsPanel: Render slider with<br/>current value & bounds
Client->>SkillsPanel: Adjust slider &<br/>click Save
SkillsPanel->>SettingsAPI: putUserSettings(new_bytes)
SettingsAPI->>HTTPServer: PUT /v1/settings<br/>(new_bytes)
HTTPServer->>HTTPServer: Validate & clamp
HTTPServer->>AppState: Update skills_hint_max_bytes
HTTPServer-->>SettingsAPI: 200 + updated Settings
SettingsAPI-->>SkillsPanel: { ok: true, settings }
SkillsPanel->>SkillsPanel: Update local state
SkillsPanel-->>Client: Show success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src-tauri/src/infrastructure/http_server.rs (1)
1232-1302:⚠️ Potential issue | 🟡 MinorPassthrough env PUT isn't atomic — keychain and
mcp.jsoncan drift.Two hazards in this block:
- If the loop fails mid-way (
save_mcp_secret/delete_mcp_secretreturnsErr), you return 500 having partially mutated the OS keychain and the in-memorycatalog_passthrough_keys, with no retry or rollback.mcp.jsonstays intact only becausesave_configis called later; on the next valid request the keychain's half-state is still there.- If the keychain loop succeeds but
save_configfails at Line 1296, the keychain now holds secrets thatmcp.jsondoesn't reference incatalog_passthrough_keys(or vice versa for deletes). At spawn time those values won't be spliced, so the user sees a "saved" secret that's effectively dead.Consider one of:
- Stage the desired
catalog_passthrough_keyslist, callsave_configfirst with the new list, and only then mutate the keychain (mirroring the ordering used for/v1/connect's token rollback).- Or accumulate keychain changes, and on any downstream error attempt a best-effort rollback before returning.
Either way, HashMap iteration order is non-deterministic, so the current "first key wins / partial state on failure" behavior is also hard to reason about from the dashboard side.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/infrastructure/http_server.rs` around lines 1232 - 1302, The handler mutates the OS keychain via secure_store::save_mcp_secret/delete_mcp_secret and updates catalog_passthrough_keys directly, risking partial state if those ops or mcp_service::save_config/te_service::sync_workspace_mounted_tools_for_catalog fail; instead, compute a staged new_catalog_passthrough_keys from body.env and the existing ServerEntry::Stdio catalog_passthrough_keys (refer to ServerEntry::Stdio, catalog_passthrough_keys, body.env), update cfg with that staged list and call mcp_service::save_config (and run te_service::sync_workspace_mounted_tools_for_catalog) first to persist mcp.json; only after config save succeeds apply secure_store::save_mcp_secret/delete_mcp_secret to the OS keychain, and on any keychain error perform a best-effort rollback by restoring previous keychain entries and re-saving the prior cfg via mcp_service::save_config to keep mcp.json and the OS keychain consistent.
🧹 Nitpick comments (8)
src-tauri/tests/common/mod.rs (1)
1-6: Consider guarding against already-set values and document ordering caveat.Two minor points:
#[ctor::ctor]runs beforemain, but if any#[ctor]in a dependency (or test harness thread) reads env concurrently,set_varis not thread-safe. In practice ctors run single-threaded at load, so this is typically fine — worth a short comment though.- Unconditionally overwriting
PENGINE_MOCK_KEYCHAINprevents a developer from opting into the real keychain for a local integration run. Using a "set only if unset" pattern preserves that escape hatch.Proposed tweak
#[ctor::ctor] fn enable_mock_keychain() { - std::env::set_var("PENGINE_MOCK_KEYCHAIN", "1"); + if std::env::var_os("PENGINE_MOCK_KEYCHAIN").is_none() { + std::env::set_var("PENGINE_MOCK_KEYCHAIN", "1"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/tests/common/mod.rs` around lines 1 - 6, The ctor function enable_mock_keychain currently unconditionally calls std::env::set_var("PENGINE_MOCK_KEYCHAIN", "1"); change it to only set the variable if it is not already present (use std::env::var_os or similar to detect an existing value) so developers can opt into the real keychain, and add a brief comment above the #[ctor::ctor] noting that ctors run before main and may race with other initialization (set_var is not thread-safe) so this relies on single-threaded ctor execution; keep the function name enable_mock_keychain and the env var PENGINE_MOCK_KEYCHAIN to locate the change.src-tauri/src/modules/skills/types.rs (1)
33-35: Doc update reads well.The clarified semantics (case-insensitive, ä/ö/ü/ß folded, tags length ≥ 6) match the updated gating in
skills/service.rsand the new coverage intests/skills_brave_gate.rs. One nit: "not in a generic denylist" is a bit vague — consider naming the constant or module that defines the denylist so future readers can locate it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/skills/types.rs` around lines 33 - 35, Update the doc comment for the `requires` field to name the actual denylist constant/module instead of the vague phrase "a generic denylist" so readers can find it; e.g., change the sentence to reference the denylist defined alongside the gating logic in `skills/service.rs` (for example, `GENERIC_DENYLIST` or the actual constant name used there). Keep the rest of the clarification (case-insensitive, ä/ö/ü/ß folded, `tags` length ≥ 6, and `brave_web_search`) intact and ensure the doc mentions the exact symbol that tests like `tests/skills_brave_gate.rs` and the gating code rely on.src-tauri/src/modules/mcp/registry.rs (1)
691-705: Substring matching on new short hints broadens false-positive surface.
message_suggests_url_fetchuseslower.contains(h), so short entries like"rss","news","presse"will match inside unrelated tokens (e.g."pressed","newsletter-unrelated","carss"won't, but"across"→ no,"press"in German words like"ausdruecklich"→ no; realistic hits:"pressed"→ matches"presse","newsletter"→ matches"news"). That's enough to force-keepfetchon many unrelated turns.This is consistent with the pre-existing style (
"wttr","api"had the same property), so not a new bug, but it does nudge the routing toward always includingfetch. Consider using the same token-aware approach asskills::service::user_text_covers_tokenfor the short additions if this becomes noisy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/mcp/registry.rs` around lines 691 - 705, The current substring matching in message_suggests_url_fetch using lower.contains(h) causes short hints like "rss", "news", "presse" to trigger false positives; change the check for these short hint entries to use the token-aware matcher skills::service::user_text_covers_token (or equivalent) instead of plain contains — specifically, in message_suggests_url_fetch, for the hint list entries (e.g., the array containing "news","headlines","rss","presse", etc.) apply user_text_covers_token when the hint length is short (or for all hints in that list) so matches only count when they align with token boundaries rather than substring occurrences.src-tauri/src/modules/secure_store/macos.rs (2)
26-33: Non-atomic save (delete-then-set) — acknowledge trade-off.If
set_generic_passwordfails afterdelete_generic_passwordsucceeds, the prior value is lost with no rollback. For user-initiated token re-save this is fine (user simply re-enters), but ifsaveis ever called from a non-interactive path (e.g., auto-refresh), consider tryingsetfirst and only falling back to delete-then-set on a duplicate-item OSStatus. Low priority; documenting the current behavior in the doc-comment would be sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/secure_store/macos.rs` around lines 26 - 33, The save function currently does a delete_generic_password followed by set_generic_password which is non-atomic and can lose the prior value if set fails; either add a doc-comment on save describing this trade-off, or change the implementation to attempt set_generic_password first and only fall back to delete_generic_password + set on a duplicate-item OSStatus from set; reference the save, set_generic_password, and delete_generic_password symbols when making the change and ensure any OSStatus check uses the platform duplicate-item constant before performing the delete-and-retry sequence.
12-18: Consider distinguishing auth failure from user cancellation.
errSecAuthFailed(-25293) on macOS indicates a locked keychain or authentication failure, which is semantically distinct from user-initiated cancellation. Currently both map toUserCancelled, which may cause confusion if the error handling logic changes. However, since no caller in the codebase currently branches on error type (all errors are logged uniformly), this is safe in practice. If error handling becomes more sophisticated (e.g., retry logic varies by error), split these into separate variants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/secure_store/macos.rs` around lines 12 - 18, The mapping in map_error currently collapses ERR_SEC_AUTH_FAILED and ERR_SEC_USER_CANCELED into SecureStoreError::UserCancelled; change this by adding a distinct enum variant (e.g., SecureStoreError::AuthFailed or SecureStoreError::KeychainLocked) to the SecureStoreError enum and update map_error to return SecureStoreError::UserCancelled for ERR_SEC_USER_CANCELED and SecureStoreError::AuthFailed for ERR_SEC_AUTH_FAILED, leaving the fallback arm unchanged; adjust any places that construct or match SecureStoreError to handle the new variant as needed.src-tauri/src/app.rs (1)
27-51: Migration writes already occur during warm_app_secrets on the setup thread — noted, but intentional trade-off.The warm step does execute synchronous keychain I/O, including writes via the migration path (
migrate_all_legacy_into→save_unified_to_keychain) on first startup. The code comment and secure_store documentation already acknowledge this: "If legacy per-bot / per-MCP items still exist, migration may touch the keychain again (a second prompt on first launch after upgrade is normal)." The developers have made this trade-off deliberately. If startup responsiveness becomes a concern, moving migration totauri::async_runtime::spawn_blockingis a valid optimization, but it's not urgent unless the prompt frequency increases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/app.rs` around lines 27 - 51, The warm_app_secrets call on the setup thread performs synchronous keychain I/O and can trigger migrations (via migrate_all_legacy_into → save_unified_to_keychain), which blocks startup; to address this, move the blocking work off the main setup thread by invoking secure_store::warm_app_secrets inside tauri::async_runtime::spawn_blocking (or another blocking executor) so the migration/writes run asynchronously while preserving current behavior and logging; update the comment to note the intentional trade-off and that spawn_blocking is an optimization if prompt latency becomes problematic.src-tauri/src/modules/secure_store/mod.rs (1)
325-415: Global mutex spans keychain I/O — worth documenting.All of
save_token,delete_token,save_mcp_secret, anddelete_mcp_secrethold the globalAPP_SECRETSmutex across asave_unified_to_keychaincall, which in turn can block on an OS prompt / Secret Service round-trip (first-time ACL grant on macOS can be >1s). In practice that serializes/v1/connect,/v1/toolengine/passthrough-env, and disconnect paths behind each other — fine under normal load, but a surprise if one of those ever gets called from a request thread alongside a slow prompt.Reads (
load_token/load_mcp_secret) release the guard before re-enteringwarm_app_secrets, which is the right pattern; consider the same shape for writes (snapshot + serialize under the lock, then release the guard and push bytes to the OS store) if write latency ever becomes a problem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/secure_store/mod.rs` around lines 325 - 415, The write functions (save_token, delete_token, save_mcp_secret, delete_mcp_secret) currently hold the global APP_SECRETS mutex across save_unified_to_keychain (which does OS keychain I/O); change them to avoid holding the mutex during keychain calls by taking/creating an owned UnifiedSecrets snapshot, mutating that owned value while holding the lock (or taking it out with Option::take), then dropping the guard and calling save_unified_to_keychain with the owned snapshot (or pre-serialized bytes) outside the lock; use lock_secrets, load_unified_from_keychain and save_unified_to_keychain to locate code paths to modify.src-tauri/src/modules/skills/service.rs (1)
109-115: Clarify thatMAX_TOTAL_SKILL_HINT_BYTESis a compile-time default, not the runtime cap.The runtime cap comes from
state.skills_hint_max_bytes(populated from user settings at startup and used inbot/agent.rs:574). This constant is never referenced anywhere in the codebase, so anyone scanning this module may assume it's authoritative and inadvertently reintroduce the hard cap later. Either remove the dead code or rename it toDEFAULT_SKILL_HINT_BYTESfor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/modules/skills/service.rs` around lines 109 - 115, MAX_TOTAL_SKILL_HINT_BYTES is a misleading compile-time constant that is not used at runtime (the actual runtime cap is state.skills_hint_max_bytes, populated from user settings and referenced in bot/agent.rs:574); either delete the unused constant or rename it to DEFAULT_SKILL_HINT_BYTES to make it clear it’s only a compile-time default and update its doc comment to mention state.skills_hint_max_bytes as the real runtime source so readers won’t assume this value is authoritative (change the symbol MAX_TOTAL_SKILL_HINT_BYTES accordingly or remove its declaration and comment).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/modules/bot/repository.rs`:
- Around line 25-80: The load function currently silently returns None when
required fields are missing or malformed; update load (around where it parses
serde_json::Value and reads bot_id, bot_username, connected_at and before any
early ? returns) to detect and report malformed/missing fields by validating the
presence and types of "bot_id", "bot_username", and "connected_at" on the parsed
obj, and if any are missing/invalid push a clear diagnostic into migration_log
(e.g., "Malformed connection.json: missing or invalid field X; file parsed as
JSON but does not contain expected connection metadata") before returning None;
keep the existing migration and persist/secure_store::save_token logic
unchanged.
In `@src-tauri/src/modules/mcp/service.rs`:
- Around line 117-187: migrate_legacy_catalog_passthrough uses the ? on
secure_store::save_mcp_secret which aborts the whole migration on a single key
failure; change it to handle each save result inline: call
secure_store::save_mcp_secret(&tool_id, env_key, val_str) and on Err log the
error and continue (do not return Err), and only push env_key into migrated_keys
when the save returned Ok; ensure catalog_passthrough is only removed/updated
for keys present in migrated_keys (so failing keys remain in legacy_map) and
that args.retain still consults migrated_keys to strip only those env args that
were successfully migrated; keep the function signature
(migrate_legacy_catalog_passthrough) and other logic unchanged.
In `@src-tauri/src/shared/state.rs`:
- Around line 12-21: The current #[derive(Debug, Clone)] on ConnectionData
exposes plaintext bot_token; remove Debug from the derive and implement a custom
fmt::Debug for ConnectionData that redacts bot_token (e.g., show "<redacted>" or
"***") while still printing bot_id, bot_username, and connected_at; so change to
#[derive(Clone)] on the struct and add an impl fmt::Debug for ConnectionData
that formats all fields but replaces bot_token with a static redaction string.
In `@src/modules/settings/api/index.ts`:
- Around line 22-36: putUserSettings currently lets fetch() and resp.json()
exceptions propagate; wrap the network and JSON parsing in a try/catch inside
putUserSettings so any thrown error (including AbortSignal.timeout) returns {
ok: false, error: string } instead of throwing. Specifically, surround the
fetch(SETTINGS_URL, ...) and const data = await resp.json() with try/catch, on
catch return a descriptive error string (e.g. String(err)), and keep the
existing resp.ok handling (returning data.error ?? `HTTP ${resp.status}`) when
parsing succeeds; reference putUserSettings, fetch, resp.json, and
AbortSignal.timeout to locate where to add the try/catch.
In `@src/modules/skills/components/SkillsPanel.tsx`:
- Around line 116-127: The effect currently bails silently when
fetchUserSettings(...) returns null leaving the control stuck in loading; update
the async block in the useEffect that calls fetchUserSettings so that when the
returned value is null you set a load-error flag and mark loading finished
instead of returning early. Concretely: add a state (e.g. ctxLoadError /
setCtxLoadError) or reuse an existing error handler, and in the branch where us
is null call setCtxLoadError(true) (and/or record an error message), call
setCtxLoaded(true) to stop the spinner, and log the failure; only skip setting
the other ctx values when us is null. Refer to the existing useEffect,
fetchUserSettings, setCtxLoaded, setCtxBytes, setCtxSaved, setCtxMin, setCtxMax
and setCtxDefault to locate where to insert this behavior.
---
Outside diff comments:
In `@src-tauri/src/infrastructure/http_server.rs`:
- Around line 1232-1302: The handler mutates the OS keychain via
secure_store::save_mcp_secret/delete_mcp_secret and updates
catalog_passthrough_keys directly, risking partial state if those ops or
mcp_service::save_config/te_service::sync_workspace_mounted_tools_for_catalog
fail; instead, compute a staged new_catalog_passthrough_keys from body.env and
the existing ServerEntry::Stdio catalog_passthrough_keys (refer to
ServerEntry::Stdio, catalog_passthrough_keys, body.env), update cfg with that
staged list and call mcp_service::save_config (and run
te_service::sync_workspace_mounted_tools_for_catalog) first to persist mcp.json;
only after config save succeeds apply
secure_store::save_mcp_secret/delete_mcp_secret to the OS keychain, and on any
keychain error perform a best-effort rollback by restoring previous keychain
entries and re-saving the prior cfg via mcp_service::save_config to keep
mcp.json and the OS keychain consistent.
---
Nitpick comments:
In `@src-tauri/src/app.rs`:
- Around line 27-51: The warm_app_secrets call on the setup thread performs
synchronous keychain I/O and can trigger migrations (via migrate_all_legacy_into
→ save_unified_to_keychain), which blocks startup; to address this, move the
blocking work off the main setup thread by invoking
secure_store::warm_app_secrets inside tauri::async_runtime::spawn_blocking (or
another blocking executor) so the migration/writes run asynchronously while
preserving current behavior and logging; update the comment to note the
intentional trade-off and that spawn_blocking is an optimization if prompt
latency becomes problematic.
In `@src-tauri/src/modules/mcp/registry.rs`:
- Around line 691-705: The current substring matching in
message_suggests_url_fetch using lower.contains(h) causes short hints like
"rss", "news", "presse" to trigger false positives; change the check for these
short hint entries to use the token-aware matcher
skills::service::user_text_covers_token (or equivalent) instead of plain
contains — specifically, in message_suggests_url_fetch, for the hint list
entries (e.g., the array containing "news","headlines","rss","presse", etc.)
apply user_text_covers_token when the hint length is short (or for all hints in
that list) so matches only count when they align with token boundaries rather
than substring occurrences.
In `@src-tauri/src/modules/secure_store/macos.rs`:
- Around line 26-33: The save function currently does a delete_generic_password
followed by set_generic_password which is non-atomic and can lose the prior
value if set fails; either add a doc-comment on save describing this trade-off,
or change the implementation to attempt set_generic_password first and only fall
back to delete_generic_password + set on a duplicate-item OSStatus from set;
reference the save, set_generic_password, and delete_generic_password symbols
when making the change and ensure any OSStatus check uses the platform
duplicate-item constant before performing the delete-and-retry sequence.
- Around line 12-18: The mapping in map_error currently collapses
ERR_SEC_AUTH_FAILED and ERR_SEC_USER_CANCELED into
SecureStoreError::UserCancelled; change this by adding a distinct enum variant
(e.g., SecureStoreError::AuthFailed or SecureStoreError::KeychainLocked) to the
SecureStoreError enum and update map_error to return
SecureStoreError::UserCancelled for ERR_SEC_USER_CANCELED and
SecureStoreError::AuthFailed for ERR_SEC_AUTH_FAILED, leaving the fallback arm
unchanged; adjust any places that construct or match SecureStoreError to handle
the new variant as needed.
In `@src-tauri/src/modules/secure_store/mod.rs`:
- Around line 325-415: The write functions (save_token, delete_token,
save_mcp_secret, delete_mcp_secret) currently hold the global APP_SECRETS mutex
across save_unified_to_keychain (which does OS keychain I/O); change them to
avoid holding the mutex during keychain calls by taking/creating an owned
UnifiedSecrets snapshot, mutating that owned value while holding the lock (or
taking it out with Option::take), then dropping the guard and calling
save_unified_to_keychain with the owned snapshot (or pre-serialized bytes)
outside the lock; use lock_secrets, load_unified_from_keychain and
save_unified_to_keychain to locate code paths to modify.
In `@src-tauri/src/modules/skills/service.rs`:
- Around line 109-115: MAX_TOTAL_SKILL_HINT_BYTES is a misleading compile-time
constant that is not used at runtime (the actual runtime cap is
state.skills_hint_max_bytes, populated from user settings and referenced in
bot/agent.rs:574); either delete the unused constant or rename it to
DEFAULT_SKILL_HINT_BYTES to make it clear it’s only a compile-time default and
update its doc comment to mention state.skills_hint_max_bytes as the real
runtime source so readers won’t assume this value is authoritative (change the
symbol MAX_TOTAL_SKILL_HINT_BYTES accordingly or remove its declaration and
comment).
In `@src-tauri/src/modules/skills/types.rs`:
- Around line 33-35: Update the doc comment for the `requires` field to name the
actual denylist constant/module instead of the vague phrase "a generic denylist"
so readers can find it; e.g., change the sentence to reference the denylist
defined alongside the gating logic in `skills/service.rs` (for example,
`GENERIC_DENYLIST` or the actual constant name used there). Keep the rest of the
clarification (case-insensitive, ä/ö/ü/ß folded, `tags` length ≥ 6, and
`brave_web_search`) intact and ensure the doc mentions the exact symbol that
tests like `tests/skills_brave_gate.rs` and the gating code rely on.
In `@src-tauri/tests/common/mod.rs`:
- Around line 1-6: The ctor function enable_mock_keychain currently
unconditionally calls std::env::set_var("PENGINE_MOCK_KEYCHAIN", "1"); change it
to only set the variable if it is not already present (use std::env::var_os or
similar to detect an existing value) so developers can opt into the real
keychain, and add a brief comment above the #[ctor::ctor] noting that ctors run
before main and may race with other initialization (set_var is not thread-safe)
so this relies on single-threaded ctor execution; keep the function name
enable_mock_keychain and the env var PENGINE_MOCK_KEYCHAIN to locate the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12b6f438-499c-42cc-8967-c3354e2bad9d
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
src-tauri/Cargo.tomlsrc-tauri/src/app.rssrc-tauri/src/infrastructure/http_server.rssrc-tauri/src/modules/bot/agent.rssrc-tauri/src/modules/bot/commands.rssrc-tauri/src/modules/bot/repository.rssrc-tauri/src/modules/keywords.rssrc-tauri/src/modules/mcp/registry.rssrc-tauri/src/modules/mcp/service.rssrc-tauri/src/modules/mcp/types.rssrc-tauri/src/modules/mod.rssrc-tauri/src/modules/secure_store/keyring_impl.rssrc-tauri/src/modules/secure_store/macos.rssrc-tauri/src/modules/secure_store/mock_store.rssrc-tauri/src/modules/secure_store/mod.rssrc-tauri/src/modules/skills/keywords.rssrc-tauri/src/modules/skills/mod.rssrc-tauri/src/modules/skills/service.rssrc-tauri/src/modules/skills/types.rssrc-tauri/src/modules/tool_engine/service.rssrc-tauri/src/shared/keywords.rssrc-tauri/src/shared/mod.rssrc-tauri/src/shared/state.rssrc-tauri/src/shared/text.rssrc-tauri/src/shared/user_settings.rssrc-tauri/tests/common/mod.rssrc-tauri/tests/mcp_tools.rssrc-tauri/tests/skills_brave_gate.rssrc/modules/settings/api/index.tssrc/modules/settings/index.tssrc/modules/skills/components/SkillsPanel.tsxsrc/pages/DashboardPage.tsx
💤 Files with no reviewable changes (1)
- src/pages/DashboardPage.tsx
… Pengine - Added instructions for starting Ollama, Podman, and Pengine on login across macOS, Linux, and Windows. - Included detailed steps for configuring autostart settings for each tool, improving user experience after system reboots. - Enhanced the setup wizard's UI with additional information and code snippets for better clarity on setup processes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements