feat(skills): advanced authentication modes for skills#315
Merged
senamakel merged 12 commits intotinyhumansai:mainfrom Apr 4, 2026
Merged
feat(skills): advanced authentication modes for skills#315senamakel merged 12 commits intotinyhumansai:mainfrom
senamakel merged 12 commits intotinyhumansai:mainfrom
Conversation
- Introduced a new `AuthModeSelector` component for selecting authentication methods (managed, self-hosted, text). - Enhanced the `SkillManager` to handle revocation of both OAuth and auth credentials. - Updated the API to support advanced auth configurations with multiple modes. - Added functionality to persist and restore auth credentials in the skill's data directory. - Updated relevant types and interfaces to accommodate the new auth structure.
…flow - Updated `SkillSetupWizard` to support a two-phase authentication process: an optional auth mode selection followed by the setup phase. - Integrated `AuthModeSelector` for users to choose between managed, self-hosted, and text authentication methods. - Improved state management to handle various auth phases, including error handling and transitions to the setup phase upon successful authentication. - Refactored relevant types and interfaces to accommodate the new authentication structure.
Contributor
|
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 10 minutes and 14 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 selected for processing (10)
📝 WalkthroughWalkthroughThis PR introduces comprehensive advanced authentication mode support for skills alongside the existing OAuth flow. It adds TypeScript UI components for auth mode selection and form rendering, extends skill configuration types to support multiple auth modes, implements credential persistence and restoration mechanisms, creates new RPC handlers for auth flows, and removes dead code from unused bridge modules. The changes span frontend UI, TypeScript APIs, Rust type definitions, JavaScript runtime globals, and event loop refactoring. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SkillSetupWizard as Setup Wizard
participant AuthModeSelector as Auth UI
participant SkillAPI as Frontend API
participant CoreRPC as Core RPC
participant Runtime as Skill Runtime
participant Memory as Memory Client
User->>SkillSetupWizard: Start setup
SkillSetupWizard->>SkillSetupWizard: Check if auth phase needed
alt Auth modes available
SkillSetupWizard->>AuthModeSelector: Show mode selection
User->>AuthModeSelector: Select auth mode
AuthModeSelector->>SkillSetupWizard: onSelect(mode)
alt Mode is "managed"
SkillSetupWizard->>SkillSetupWizard: Show waiting state
SkillSetupWizard->>CoreRPC: Start skill, await managed auth
Runtime->>Runtime: Complete managed auth
CoreRPC->>SkillSetupWizard: Auth complete notification
else Mode is "self_hosted" or "text"
SkillSetupWizard->>SkillSetupWizard: Render auth form
User->>SkillSetupWizard: Enter credentials
SkillSetupWizard->>CoreRPC: POST auth/complete with credentials
CoreRPC->>Runtime: Inject credentials via globalThis.auth
end
SkillSetupWizard->>SkillAPI: callCoreRpc(auth/complete, mode, payload)
SkillAPI->>CoreRPC: RPC call
CoreRPC->>Runtime: Restore auth credentials
Runtime->>SkillSetupWizard: auth/complete response
end
SkillSetupWizard->>SkillAPI: startSetup()
SkillAPI->>CoreRPC: Start setup phase
CoreRPC->>Runtime: Run setup steps
Runtime->>Memory: Persist state (via skill/sync)
SkillSetupWizard->>SkillSetupWizard: Mark setup complete
SkillSetupWizard->>User: Setup finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/components/skills/SkillSetupWizard.tsx`:
- Around line 92-115: handleManagedAuth currently opens the OAuth URL then sets
phase "auth_managed_waiting" and immediately later the flow transitions to setup
without finalizing the managed credential; update handleManagedAuth (and the
similar path at lines ~187-202) to wait for snap.state.__oauth_credential to
appear (poll or subscribe) and then call openhuman.skills_rpc with method
"auth/complete" to finalize the credential before calling transitionToSetup().
Ensure you pass the same mode/provider context when invoking auth/complete and
only transition once auth.fetch()/auth.getMode() reflect the new credential.
In `@app/src/lib/skills/manager.ts`:
- Around line 423-433: The auth revocation and persisted-auth cleanup are
incorrectly skipped when authMode is falsy; remove the conditional gating so
rpcRevokeAuth(skillId, authMode) is attempted inside its try/catch regardless of
authMode and ensure the persisted-auth cleanup logic always runs as well (do not
skip the block that clears stored credentials even if mode parsing failed); keep
the existing try/catch around rpcRevokeAuth, preserve setting
authRevokeSucceeded, and apply the same change for the second occurrence
currently at the other block (the one around lines 447-454) so both revoke and
cleanup always execute.
In `@src/openhuman/skills/manifest.rs`:
- Around line 27-37: Change SkillAuthMode.mode_type from free-form String to a
strongly-typed enum (e.g., SkillAuthType with variants Managed, SelfHosted,
Text) and derive/annotate it for serde (use serde rename or rename_all to match
existing manifest strings) so invalid types fail at deserialize; also validate
that SkillAuthConfig.modes is non-empty during deserialization by adding a
custom Deserialize impl or using a deserialize_with helper (e.g., non_empty_vec)
that returns an error when modes.is_empty(); update references to SkillAuthMode
and mode_type accordingly so callers use the new SkillAuthType enum.
In `@src/openhuman/skills/qjs_skill_instance/event_loop.rs`:
- Around line 703-785: Currently the code injects credentials into
globalThis/state and writes auth_credential.json (and oauth_credential.json for
managed mode) before calling the validation hook handle_js_call(...,
"onAuthComplete", ...); move the injection and file writes to after
handle_js_call returns success (and do not persist if it returns an error or
throws). Concretely: compute cred_json/managed credential strings as you do now,
then call handle_js_call(rt, ctx, "onAuthComplete", ¶ms_str).await and
inspect the result (treat thrown exceptions or a returned {status:"error"} as
failure); only if validation succeeds run ctx.with(|js_ctx| js_ctx.eval(...)) to
call globalThis.auth.__setCredential / globalThis.state.set and then perform
std::fs::write to cred_path and oauth_path; if validation fails, skip injection
and writes (or explicitly clear any temporary in-memory state). Use the existing
symbols cred_json, managed_bridge (or move managed_bridge generation until after
validation), params_str, handle_js_call, cred_path and oauth_path for locating
edits.
- Around line 715-718: Reformat the serde_json::to_string(...) multiline
expressions to satisfy rustfmt: rewrite the block that sets creds_json (the
statement using
serde_json::to_string(params.get("credentials").unwrap_or(&serde_json::Value::Null)).unwrap_or_else(|_|
"null".to_string())) so it adheres to rustfmt style (break into properly
indented lines or assign intermediate variables), run cargo fmt to verify, and
apply the same reformatting to the similar occurrence around the params block at
the later serde_json::to_string call (the one at lines 768–771) so CI passes.
In `@src/openhuman/skills/qjs_skill_instance/js_helpers.rs`:
- Around line 153-161: The OAuth bridge code only sets credentials when
cred.mode === 'managed' but never clears them when restored mode is not managed;
update the block handling cred (in the same conditional around cred.mode and
cred.credentials) to explicitly clear OAuth state when mode is not 'managed' or
credentials are absent: if globalThis.oauth.__setCredential exists call it with
null (or a dedicated clear method like __clearCredential if present), and if
globalThis.state.set exists call state.set('__oauth_credential', null) or remove
the key, ensuring both globalThis.oauth and globalThis.state are updated in the
else branch so stale managed credentials are removed when switching to
self-hosted/text.
In `@src/openhuman/skills/quickjs_libs/bootstrap.js`:
- Around line 966-969: The managed branch currently returns via
globalThis.oauth.fetch(...) without clearing persisted credentials, so expired
credentials are resurrected; update the managed-path to call the same
revoke/cleanup routine used for the self-hosted branch (the code that clears
__authCredential and blanks JS state) before returning, and extend that revoke
routine to also remove oauth_credential.json when running in managed mode;
ensure both branches call the shared revoke/cleanup helper (reuse the existing
code that clears __authCredential and JS state) so auth_credential.json is
deleted in all cases and oauth_credential.json is deleted for managed mode.
- Around line 971-989: The code is wrongly attaching the app session JWT to
arbitrary fetch targets and can overwrite existing auth headers; change the
injection logic in the headers assembly so that Authorization is only
auto-injected for trusted/internal endpoints (e.g., check the request URL origin
against the known self-hosted backend base or require an explicit
options.injectAuth flag) and never for arbitrary external URLs or when in text
mode; also perform a case-insensitive check for an existing authorization header
(check for both 'authorization' and 'Authorization' keys or lower-case all
header names) before adding either the Basic auth (using
creds.client_id/creds.client_secret) or the Bearer token from
__ops.get_session_token()/jwtToken, and ensure this logic is applied
consistently for net.fetch()/auth.fetch() calls.
🪄 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: 6b6ad598-2f38-46a9-83f1-29c3f7b1a09f
📒 Files selected for processing (11)
app/src/components/skills/AuthModeSelector.tsxapp/src/components/skills/SetupFormRenderer.tsxapp/src/components/skills/SkillSetupWizard.tsxapp/src/lib/skills/manager.tsapp/src/lib/skills/skillsApi.tsapp/src/lib/skills/types.tssrc/openhuman/skills/manifest.rssrc/openhuman/skills/qjs_skill_instance/event_loop.rssrc/openhuman/skills/qjs_skill_instance/instance.rssrc/openhuman/skills/qjs_skill_instance/js_helpers.rssrc/openhuman/skills/quickjs_libs/bootstrap.js
Comment on lines
+92
to
+115
| const handleManagedAuth = useCallback( | ||
| async (mode: AuthMode) => { | ||
| if (!mode.provider) { | ||
| setState({ phase: "error", message: "Managed mode requires a provider." }); | ||
| return; | ||
| } | ||
| try { | ||
| const shouldShowJson = IS_DEV ? 'responseType=json&' : ''; | ||
| const data = await apiClient.get<{ oauthUrl?: string }>( | ||
| `/auth/${mode.provider}/connect?${shouldShowJson}skillId=${skillId}`, | ||
| ); | ||
| if (!data.oauthUrl) { | ||
| setState({ phase: "error", message: "Failed to get OAuth URL from backend." }); | ||
| return; | ||
| } | ||
| await openUrl(data.oauthUrl); | ||
| setState({ phase: "auth_managed_waiting", mode }); | ||
| } catch (err) { | ||
| const msg = err instanceof Error ? err.message : String(err); | ||
| setState({ phase: "error", message: `OAuth connection failed: ${msg}` }); | ||
| } | ||
| }, | ||
| [skillId], | ||
| ); |
Contributor
There was a problem hiding this comment.
Managed auth still needs the auth/complete commit step.
After OAuth finishes, Lines 189-191 jump straight to transitionToSetup(). In this component, only the self-hosted/text submit path calls openhuman.skills_rpc with method: "auth/complete", but src/openhuman/skills/qjs_skill_instance/event_loop.rs:703-785 is the code that actually writes __auth_credential / auth_credential.json and bridges managed credentials into globalThis.auth. As written, a managed skill can reach setup with connection_status === "connected" while auth.getMode() and auth.fetch() still see no auth credential. Prefer waiting for snap.state.__oauth_credential and finalizing it through auth/complete before transitioning.
Also applies to: 187-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/components/skills/SkillSetupWizard.tsx` around lines 92 - 115,
handleManagedAuth currently opens the OAuth URL then sets phase
"auth_managed_waiting" and immediately later the flow transitions to setup
without finalizing the managed credential; update handleManagedAuth (and the
similar path at lines ~187-202) to wait for snap.state.__oauth_credential to
appear (poll or subscribe) and then call openhuman.skills_rpc with method
"auth/complete" to finalize the credential before calling transitionToSetup().
Ensure you pass the same mode/provider context when invoking auth/complete and
only transition once auth.fetch()/auth.getMode() reflect the new credential.
Comment on lines
+703
to
+785
| "auth/complete" => { | ||
| // Inject auth credential into the auth bridge + in-memory state | ||
| let cred_json = | ||
| serde_json::to_string(¶ms).unwrap_or_else(|_| "null".to_string()); | ||
| let is_managed = params | ||
| .get("mode") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|m| m == "managed") | ||
| .unwrap_or(false); | ||
|
|
||
| // Inject into globalThis.auth + state; for managed mode also bridge to oauth | ||
| let managed_bridge = if is_managed { | ||
| let creds_json = serde_json::to_string( | ||
| params.get("credentials").unwrap_or(&serde_json::Value::Null), | ||
| ) | ||
| .unwrap_or_else(|_| "null".to_string()); | ||
| format!( | ||
| r#" | ||
| if (typeof globalThis.oauth !== 'undefined' && globalThis.oauth.__setCredential) {{ | ||
| globalThis.oauth.__setCredential({creds}); | ||
| }} | ||
| if (typeof globalThis.state !== 'undefined' && globalThis.state.set) {{ | ||
| globalThis.state.set('__oauth_credential', {creds}); | ||
| }}"#, | ||
| creds = creds_json | ||
| ) | ||
| } else { | ||
| String::new() | ||
| }; | ||
|
|
||
| let code = format!( | ||
| r#"(function() {{ | ||
| if (typeof globalThis.auth !== 'undefined' && globalThis.auth.__setCredential) {{ | ||
| globalThis.auth.__setCredential({cred}); | ||
| }} | ||
| if (typeof globalThis.state !== 'undefined' && globalThis.state.set) {{ | ||
| globalThis.state.set('__auth_credential', {cred}); | ||
| }} | ||
| {managed_bridge} | ||
| }})()"#, | ||
| cred = cred_json, | ||
| managed_bridge = managed_bridge | ||
| ); | ||
| ctx.with(|js_ctx| { | ||
| let _ = js_ctx.eval::<rquickjs::Value, _>(code.as_bytes()); | ||
| }) | ||
| .await; | ||
|
|
||
| // Persist auth credential to disk | ||
| let cred_path = data_dir.join("auth_credential.json"); | ||
| if let Err(e) = std::fs::write(&cred_path, &cred_json) { | ||
| log::error!( | ||
| "[skill:{}] Failed to persist auth credential: {e}", | ||
| skill_id | ||
| ); | ||
| } else { | ||
| log::info!( | ||
| "[skill:{}] Auth credential persisted to {}", | ||
| skill_id, | ||
| cred_path.display() | ||
| ); | ||
| } | ||
|
|
||
| // For managed mode, also persist as oauth_credential.json for backward compat | ||
| if is_managed { | ||
| let oauth_cred_json = serde_json::to_string( | ||
| params.get("credentials").unwrap_or(&serde_json::Value::Null), | ||
| ) | ||
| .unwrap_or_else(|_| "null".to_string()); | ||
| let oauth_path = data_dir.join("oauth_credential.json"); | ||
| if let Err(e) = std::fs::write(&oauth_path, &oauth_cred_json) { | ||
| log::error!( | ||
| "[skill:{}] Failed to persist managed OAuth credential: {e}", | ||
| skill_id | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Call skill's onAuthComplete lifecycle hook for validation | ||
| let params_str = | ||
| serde_json::to_string(¶ms).unwrap_or_else(|_| "{}".to_string()); | ||
| handle_js_call(rt, ctx, "onAuthComplete", ¶ms_str).await | ||
| } |
Contributor
There was a problem hiding this comment.
Persist the auth credential only after onAuthComplete accepts it.
Lines 733-779 commit __auth_credential and write auth_credential.json and, for managed mode, oauth_credential.json before the validation hook on Lines 781-784 runs. If onAuthComplete returns { status: "error" } or throws, the rejected secret is already live in state and survives restarts/tool execution. Keep the in-memory credential only for validation if needed, but defer the state and file write, or roll it back, until validation succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/skills/qjs_skill_instance/event_loop.rs` around lines 703 -
785, Currently the code injects credentials into globalThis/state and writes
auth_credential.json (and oauth_credential.json for managed mode) before calling
the validation hook handle_js_call(..., "onAuthComplete", ...); move the
injection and file writes to after handle_js_call returns success (and do not
persist if it returns an error or throws). Concretely: compute cred_json/managed
credential strings as you do now, then call handle_js_call(rt, ctx,
"onAuthComplete", ¶ms_str).await and inspect the result (treat thrown
exceptions or a returned {status:"error"} as failure); only if validation
succeeds run ctx.with(|js_ctx| js_ctx.eval(...)) to call
globalThis.auth.__setCredential / globalThis.state.set and then perform
std::fs::write to cred_path and oauth_path; if validation fails, skip injection
and writes (or explicitly clear any temporary in-memory state). Use the existing
symbols cred_json, managed_bridge (or move managed_bridge generation until after
validation), params_str, handle_js_call, cred_path and oauth_path for locating
edits.
Comment on lines
+715
to
+718
| let creds_json = serde_json::to_string( | ||
| params.get("credentials").unwrap_or(&serde_json::Value::Null), | ||
| ) | ||
| .unwrap_or_else(|_| "null".to_string()); |
Contributor
There was a problem hiding this comment.
Rustfmt this hunk before merge.
cargo fmt --check is already failing on the multiline serde_json::to_string(...) calls here, so CI will stay red until this block is reformatted.
Also applies to: 768-771
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/skills/qjs_skill_instance/event_loop.rs` around lines 715 -
718, Reformat the serde_json::to_string(...) multiline expressions to satisfy
rustfmt: rewrite the block that sets creds_json (the statement using
serde_json::to_string(params.get("credentials").unwrap_or(&serde_json::Value::Null)).unwrap_or_else(|_|
"null".to_string())) so it adheres to rustfmt style (break into properly
indented lines or assign intermediate variables), run cargo fmt to verify, and
apply the same reformatting to the similar occurrence around the params block at
the later serde_json::to_string call (the one at lines 768–771) so CI passes.
Comment on lines
+966
to
+969
| // Managed mode: delegate to oauth.fetch for proxy behavior | ||
| if (mode === 'managed' && typeof globalThis.oauth !== 'undefined') { | ||
| return globalThis.oauth.fetch(url, options); | ||
| } |
Contributor
There was a problem hiding this comment.
401/403 cleanup needs to revoke the persisted auth state too.
The managed branch on Lines 966-969 returns through oauth.fetch() before touching __authCredential, and Lines 1002-1014 only blank JS state for self-hosted/text. src/openhuman/skills/qjs_skill_instance/js_helpers.rs:127-156 and src/openhuman/skills/qjs_skill_instance/event_loop.rs:341-343,525-527 reload auth_credential.json on startup and before tool/webhook execution, so the same expired credential is resurrected on the next invocation unless this path also removes the persisted files. Please route both cases through the same revoke path that deletes auth_credential.json and, for managed mode, oauth_credential.json.
Also applies to: 1002-1014
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/skills/quickjs_libs/bootstrap.js` around lines 966 - 969, The
managed branch currently returns via globalThis.oauth.fetch(...) without
clearing persisted credentials, so expired credentials are resurrected; update
the managed-path to call the same revoke/cleanup routine used for the
self-hosted branch (the code that clears __authCredential and blanks JS state)
before returning, and extend that revoke routine to also remove
oauth_credential.json when running in managed mode; ensure both branches call
the shared revoke/cleanup helper (reuse the existing code that clears
__authCredential and JS state) so auth_credential.json is deleted in all cases
and oauth_credential.json is deleted for managed mode.
…orts - Deleted the unused `loader.rs` module, which was reserved for future ES module import support. - Removed references to the `loader` module in `mod.rs`. - Cleaned up the `bridge` module by removing several unused bridge files, including `cron_bridge.rs`, `db.rs`, `log_bridge.rs`, `skills_bridge.rs`, `store.rs`, and `tauri_bridge.rs`. - Added comprehensive tests for the `manifest.rs`, `ops.rs`, and `preferences.rs` modules to ensure functionality remains intact after the refactor.
…registry - Added a new `registry_cache.rs` module to handle disk-based caching for the remote skill registry. - Implemented functions for managing cache, including reading, writing, and checking cache freshness. - Updated `registry_ops.rs` to utilize the new caching functions, improving performance and reducing redundant network calls. - Introduced a new `event_loop` module to manage the QuickJS runtime, including handling incoming messages and persisting state to memory. - Added webhook request handling and RPC message handlers to facilitate communication with external services. - Removed the obsolete `event_loop.rs` file, consolidating functionality into the new structure.
…agement functions - Modified test module imports in `registry_ops.rs` to include new cache management functions from `registry_cache`. - This change ensures that tests can utilize the updated caching functionality introduced in the recent registry cache management feature.
…matting - Updated the OpenHuman dependency version in `Cargo.lock` to 0.51.6. - Refactored code in several files for improved readability by adjusting formatting and removing unnecessary line breaks. - Ensured consistent logging format in `event_loop` and `rpc_handlers` modules for better clarity in log messages.
- Refactored the Conversations component to enhance the display of team usage information, including clearer conditional rendering for budget exhaustion messages. - Improved formatting of budget display for both 5-hour and weekly limits, ensuring consistent presentation. - Streamlined the import of the core HTTP base URL in the WebhooksDebugPanel for better code organization.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/openhuman/skills/ops.rs (1)
220-228: Add malformed-JSON coverage forparse_skill_manifestfailure branch.Current tests validate missing fields (
{}), but not invalid JSON parse failure. Add one test so theNonebranch is explicitly guarded.Proposed test addition
#[cfg(test)] mod tests { use super::*; @@ fn parse_skill_manifest_missing_fields_uses_defaults() { let dir = tempfile::tempdir().unwrap(); let path = dir.path().join("skill.json"); std::fs::write(&path, "{}").unwrap(); let skill = parse_skill_manifest(&path, "fallback"); assert_eq!(skill.name, "fallback"); assert!(skill.description.is_empty()); assert!(skill.tags.is_empty()); } + + #[test] + fn parse_skill_manifest_invalid_json_falls_back_to_defaults() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("skill.json"); + std::fs::write(&path, "{invalid json").unwrap(); + let skill = parse_skill_manifest(&path, "fallback"); + assert_eq!(skill.name, "fallback"); + assert!(skill.description.is_empty()); + assert!(skill.tags.is_empty()); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/ops.rs` around lines 220 - 228, Add a new unit test that verifies parse_skill_manifest returns None on invalid JSON: create a tempfile, write malformed JSON (e.g. "{") to a skill.json path, call parse_skill_manifest(&path, "fallback") and assert the result is None; name the test something like parse_skill_manifest_malformed_json_returns_none and mark it with #[test] to explicitly cover the failure branch of parse_skill_manifest.src/openhuman/skills/manifest.rs (1)
170-260: Consider adding tests for auth config parsing.The new
SkillAuthConfigandSkillAuthModetypes lack dedicated test coverage. Adding tests for parsing manifests withsetup.authwould help ensure the serde attributes (rename,default) work as expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/manifest.rs` around lines 170 - 260, Add unit tests in the existing tests module in manifest.rs to cover SkillAuthConfig and SkillAuthMode parsing via SkillManifest::from_json: include manifests with a setup.auth object specifying different modes (e.g., "none", "prompt", "token") and with/without optional fields to verify serde rename/default behavior, assert the parsed SkillManifest.setup.auth maps to the expected SkillAuthMode and SkillAuthConfig values, and include a case for missing setup.auth to assert the default is used.src/openhuman/skills/qjs_skill_instance/event_loop/rpc_handlers.rs (1)
43-55: Consider using async file I/O to avoid blocking the runtime.The
std::fs::writecall blocks the async runtime. While credential files are small and this is unlikely to cause issues in practice, usingtokio::fs::writewould be more consistent with the async context.Example change
- if let Err(e) = std::fs::write(&cred_path, &cred_json) { + if let Err(e) = tokio::fs::write(&cred_path, &cred_json).await {Apply similarly to other
std::fs::writeandstd::fs::remove_filecalls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/qjs_skill_instance/event_loop/rpc_handlers.rs` around lines 43 - 55, The blocking std::fs I/O should be converted to async tokio I/O: replace std::fs::write(&cred_path, &cred_json) with tokio::fs::write(&cred_path, &cred_json).await (and similarly replace any std::fs::remove_file calls with tokio::fs::remove_file(...).await), ensuring the surrounding handler/function is async and result handling uses .await and propagates or logs errors as before (retain log::error! and log::info! using skill_id, cred_path, data_dir, cred_json identifiers).src/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rs (1)
53-56: Status code cast is safe for HTTP but could silently truncate invalid values.The
as u16cast fromu64will truncate values > 65535. While HTTP status codes are always 3 digits (100-599), a malformed JS response could produce unexpected results. Consider clamping or validating:.map(|v| v.min(999) as u16)This is minor since skill JS code is trusted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rs` around lines 53 - 56, The current extraction of status_code from response_val (response_val.get("statusCode").and_then(|v| v.as_u64()).unwrap_or(200) as u16) can silently truncate large u64s; replace it with a validated/clamped conversion so only valid HTTP ranges are allowed: parse the u64 via response_val.get("statusCode")/and_then(|v| v.as_u64()), then map or clamp the value into the HTTP range (e.g., clamp to 100..=599) before casting to u16, and keep the fallback default of 200 if parsing fails; update the binding named status_code accordingly.src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs (1)
557-558: Moveusestatement to the imports section.The
use super::js_handlers::handle_server_event;at the end of the file is unconventional. Move it to the imports section (around line 23-28) for consistency.Proposed fix
Move to imports section:
use super::js_handlers::{ - call_lifecycle, handle_cron_trigger, handle_js_call, handle_js_void_call, + call_lifecycle, handle_cron_trigger, handle_js_call, handle_js_void_call, handle_server_event, read_pending_tool_result, start_async_tool_call, };And remove line 558.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs` around lines 557 - 558, Move the stray `use super::js_handlers::handle_server_event;` into the top import section with the other `use` statements (around the existing imports near lines 23–28) and remove the duplicate/trailing `use` at the end of the file; ensure `handle_server_event` remains imported exactly once so references in this module continue to compile.
🤖 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/openhuman/skills/registry_cache.rs`:
- Around line 41-47: The is_cache_fresh function currently treats future
fetched_at timestamps as fresh because (now - fetched) yields a negative age;
change the logic in is_cache_fresh (operating on CachedRegistry.fetched_at and
comparing against CACHE_TTL_SECS) to detect and reject future timestamps by
returning false when the computed age is negative (i.e., fetched is after now),
otherwise compare the non-negative age in seconds against CACHE_TTL_SECS to
decide freshness.
- Around line 23-25: is_local_path currently only checks for Unix-style "/" and
"file://" and therefore misclassifies Windows absolute paths; update the
is_local_path function to also return true for Windows drive-letter absolute
paths (match r"^[A-Za-z]:\\" or r"^[A-Za-z]:/") and for UNC paths starting with
"\\\\" so strings like "C:\\path", "C:/path" and "\\\\server\\share" are treated
as local, while keeping the existing checks for leading '/' and "file://".
---
Nitpick comments:
In `@src/openhuman/skills/manifest.rs`:
- Around line 170-260: Add unit tests in the existing tests module in
manifest.rs to cover SkillAuthConfig and SkillAuthMode parsing via
SkillManifest::from_json: include manifests with a setup.auth object specifying
different modes (e.g., "none", "prompt", "token") and with/without optional
fields to verify serde rename/default behavior, assert the parsed
SkillManifest.setup.auth maps to the expected SkillAuthMode and SkillAuthConfig
values, and include a case for missing setup.auth to assert the default is used.
In `@src/openhuman/skills/ops.rs`:
- Around line 220-228: Add a new unit test that verifies parse_skill_manifest
returns None on invalid JSON: create a tempfile, write malformed JSON (e.g. "{")
to a skill.json path, call parse_skill_manifest(&path, "fallback") and assert
the result is None; name the test something like
parse_skill_manifest_malformed_json_returns_none and mark it with #[test] to
explicitly cover the failure branch of parse_skill_manifest.
In `@src/openhuman/skills/qjs_skill_instance/event_loop/mod.rs`:
- Around line 557-558: Move the stray `use
super::js_handlers::handle_server_event;` into the top import section with the
other `use` statements (around the existing imports near lines 23–28) and remove
the duplicate/trailing `use` at the end of the file; ensure
`handle_server_event` remains imported exactly once so references in this module
continue to compile.
In `@src/openhuman/skills/qjs_skill_instance/event_loop/rpc_handlers.rs`:
- Around line 43-55: The blocking std::fs I/O should be converted to async tokio
I/O: replace std::fs::write(&cred_path, &cred_json) with
tokio::fs::write(&cred_path, &cred_json).await (and similarly replace any
std::fs::remove_file calls with tokio::fs::remove_file(...).await), ensuring the
surrounding handler/function is async and result handling uses .await and
propagates or logs errors as before (retain log::error! and log::info! using
skill_id, cred_path, data_dir, cred_json identifiers).
In `@src/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rs`:
- Around line 53-56: The current extraction of status_code from response_val
(response_val.get("statusCode").and_then(|v| v.as_u64()).unwrap_or(200) as u16)
can silently truncate large u64s; replace it with a validated/clamped conversion
so only valid HTTP ranges are allowed: parse the u64 via
response_val.get("statusCode")/and_then(|v| v.as_u64()), then map or clamp the
value into the HTTP range (e.g., clamp to 100..=599) before casting to u16, and
keep the fallback default of 200 if parsing fails; update the binding named
status_code accordingly.
🪄 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: 4dc45c55-536e-48c3-af57-37b3fba73131
⛔ Files ignored due to path filters (1)
app/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
src/openhuman/skills/bridge/cron_bridge.rssrc/openhuman/skills/bridge/db.rssrc/openhuman/skills/bridge/log_bridge.rssrc/openhuman/skills/bridge/mod.rssrc/openhuman/skills/bridge/skills_bridge.rssrc/openhuman/skills/bridge/store.rssrc/openhuman/skills/bridge/tauri_bridge.rssrc/openhuman/skills/loader.rssrc/openhuman/skills/manifest.rssrc/openhuman/skills/mod.rssrc/openhuman/skills/ops.rssrc/openhuman/skills/preferences.rssrc/openhuman/skills/qjs_skill_instance/event_loop/mod.rssrc/openhuman/skills/qjs_skill_instance/event_loop/rpc_handlers.rssrc/openhuman/skills/qjs_skill_instance/event_loop/webhook_handler.rssrc/openhuman/skills/qjs_skill_instance/instance.rssrc/openhuman/skills/qjs_skill_instance/types.rssrc/openhuman/skills/registry_cache.rssrc/openhuman/skills/registry_ops.rssrc/openhuman/skills/types.rs
💤 Files with no reviewable changes (9)
- src/openhuman/skills/qjs_skill_instance/types.rs
- src/openhuman/skills/loader.rs
- src/openhuman/skills/bridge/mod.rs
- src/openhuman/skills/bridge/skills_bridge.rs
- src/openhuman/skills/bridge/tauri_bridge.rs
- src/openhuman/skills/bridge/cron_bridge.rs
- src/openhuman/skills/bridge/log_bridge.rs
- src/openhuman/skills/bridge/db.rs
- src/openhuman/skills/bridge/store.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/skills/qjs_skill_instance/instance.rs
Comment on lines
+23
to
+25
| pub(crate) fn is_local_path(url: &str) -> bool { | ||
| url.starts_with('/') || url.starts_with("file://") | ||
| } |
Contributor
There was a problem hiding this comment.
is_local_path misses Windows absolute paths.
Line 24 only recognizes /... and file://...; C:\... is treated as non-local and falls into the HTTP path.
Proposed fix
pub(crate) fn is_local_path(url: &str) -> bool {
- url.starts_with('/') || url.starts_with("file://")
+ if url.starts_with("file://") {
+ return true;
+ }
+ Path::new(url).is_absolute()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/skills/registry_cache.rs` around lines 23 - 25, is_local_path
currently only checks for Unix-style "/" and "file://" and therefore
misclassifies Windows absolute paths; update the is_local_path function to also
return true for Windows drive-letter absolute paths (match r"^[A-Za-z]:\\" or
r"^[A-Za-z]:/") and for UNC paths starting with "\\\\" so strings like
"C:\\path", "C:/path" and "\\\\server\\share" are treated as local, while
keeping the existing checks for leading '/' and "file://".
Comment on lines
+41
to
+47
| pub(crate) fn is_cache_fresh(cached: &CachedRegistry) -> bool { | ||
| let Ok(fetched) = chrono::DateTime::parse_from_rfc3339(&cached.fetched_at) else { | ||
| return false; | ||
| }; | ||
| let now = chrono::Utc::now(); | ||
| (now - fetched.to_utc()).num_seconds() < CACHE_TTL_SECS | ||
| } |
Contributor
There was a problem hiding this comment.
Future timestamps currently bypass TTL validation.
At Line 46, a future fetched_at yields negative age and is considered fresh, which can keep cache valid much longer than intended.
Proposed fix
pub(crate) fn is_cache_fresh(cached: &CachedRegistry) -> bool {
let Ok(fetched) = chrono::DateTime::parse_from_rfc3339(&cached.fetched_at) else {
return false;
};
let now = chrono::Utc::now();
- (now - fetched.to_utc()).num_seconds() < CACHE_TTL_SECS
+ let age_secs = (now - fetched.to_utc()).num_seconds();
+ (0..CACHE_TTL_SECS).contains(&age_secs)
}📝 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.
Suggested change
| pub(crate) fn is_cache_fresh(cached: &CachedRegistry) -> bool { | |
| let Ok(fetched) = chrono::DateTime::parse_from_rfc3339(&cached.fetched_at) else { | |
| return false; | |
| }; | |
| let now = chrono::Utc::now(); | |
| (now - fetched.to_utc()).num_seconds() < CACHE_TTL_SECS | |
| } | |
| pub(crate) fn is_cache_fresh(cached: &CachedRegistry) -> bool { | |
| let Ok(fetched) = chrono::DateTime::parse_from_rfc3339(&cached.fetched_at) else { | |
| return false; | |
| }; | |
| let now = chrono::Utc::now(); | |
| let age_secs = (now - fetched.to_utc()).num_seconds(); | |
| (0..CACHE_TTL_SECS).contains(&age_secs) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/openhuman/skills/registry_cache.rs` around lines 41 - 47, The
is_cache_fresh function currently treats future fetched_at timestamps as fresh
because (now - fetched) yields a negative age; change the logic in
is_cache_fresh (operating on CachedRegistry.fetched_at and comparing against
CACHE_TTL_SECS) to detect and reject future timestamps by returning false when
the computed age is negative (i.e., fetched is after now), otherwise compare the
non-negative age in seconds against CACHE_TTL_SECS to decide freshness.
- Updated the SkillManager to attempt revoking auth credentials even if the auth mode is unknown, improving robustness. - Introduced a new deserialization function to ensure that at least one authentication mode is provided in the SkillAuthConfig. - Enhanced the handling of temporary credential injection during the auth completion process, ensuring credentials are only persisted upon successful validation. - Improved the handling of OAuth credentials in JavaScript, ensuring stale credentials are cleared for non-managed modes. - Added checks for existing Authorization headers to prevent redundant injections in self-hosted modes.
- Introduced tests to validate the deserialization of known authentication modes in the SkillManifest. - Added checks to ensure invalid authentication types and empty mode lists are correctly rejected. - These tests enhance the robustness of the authentication configuration handling in the manifest module.
…ants and improving input handling - Removed unused constants related to channel statuses, authentication modes, and fallback definitions to streamline the MessagingPanel component. - Updated input handling to use a more concise onChange function, enhancing code readability. - Commented out placeholder and className properties for input fields to improve clarity and focus on essential functionality.
- Combined the import statements for Tunnel type and tunnelsApi from the same module into a single line, enhancing code readability and organization.
This was referenced Apr 4, 2026
5 tasks
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
setup.oauthskills continue to work via auto-synthesis as managed modeChanges
Rust Core
manifest.rs: AddedSkillAuthConfig,SkillAuthModestructs withmanaged,self_hosted,textmode typesbootstrap.js: AddedglobalThis.authbridge withgetCredential(),getMode(),getCredentials(),fetch()APIsjs_helpers.rs: Addedrestore_auth_credential()for persisting/restoring auth credentials across restartsevent_loop.rs: Addedauth/completeandauth/revokedRPC handlers with managed-mode bridging to OAuth systeminstance.rs: Added auth credential restore at skill startupFrontend (app/)
types.ts: AddedAuthMode,SkillAuthConfiginterfaces; extendedSetupFieldwithtextareatypeSetupFormRenderer.tsx: Addedtextareafield renderer with monospace fontAuthModeSelector.tsx: New card-based mode selector componentSkillSetupWizard.tsx: Added auth phases (auth_mode_select,auth_form,auth_submitting,auth_managed_waiting); two-phase flow (auth → setup)skillsApi.ts: AddedrevokeAuth()andremovePersistedAuthCredential()manager.ts: Updated disconnect flow to handle both OAuth and auth credentialsTest plan
setup.oauth) still work unchangedsetup.authshow mode selector UIonAuthCompleteonAuthCompleteauth_credential.json)cargo checkpassesyarn typecheckpasses🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Refactoring