refactor(skills): unify auth/oauth handshake on start({validate}) and drop enabled flag#484
Merged
refactor(skills): unify auth/oauth handshake on start({validate}) and drop enabled flag#484
enabled flag#484Conversation
…ving the enabled toggle - Removed the `enabled` field from `SkillPreference`, simplifying the preference model to focus solely on `setup_complete`. - Updated related methods and RPC handlers to reflect this change, ensuring that skills are automatically started based on the completion of their setup process. - Adjusted tests to validate the new behavior, ensuring consistent functionality without the `enabled` toggle. - Enhanced documentation to clarify the new preference management approach.
…te hook - Updated the `handle_auth_complete` function to eliminate the separate `onAuthComplete` JavaScript hook, streamlining the authentication process. - Revised the flow to directly inject new credentials and validate them using the `start` function, which now handles both validation and activation. - Enhanced rollback logic to ensure temporary credentials are cleared if validation fails, preventing persistence on disk. - Improved documentation to clarify the new authentication steps and their implications for credential management.
…h_complete` - Removed the `build_start_credentials_arg` function and integrated its logic directly into `handle_oauth_complete`, simplifying the flow. - Updated the OAuth handling to validate credentials before persisting them, ensuring that only successful validations are saved. - Enhanced rollback logic to clear temporary credentials if validation fails, preventing incorrect state persistence. - Improved documentation to clarify the new steps in the OAuth process and their implications for credential management.
Contributor
📝 WalkthroughWalkthroughThe PR removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as OAuth Handler
participant JS as JS Runtime
participant Persist as Disk Storage
Client->>Handler: handleOAuthComplete(oauth_params)
Handler->>JS: Inject oauth + __oauthClientKey
Handler->>JS: Call start({oauth, auth, validate: true})
JS-->>Handler: Result {status, ...}
alt Validation Success
Handler->>Persist: Write oauth_credential.json
Handler->>Persist: Write client_key.json
Handler-->>Client: Return success
else Validation Failed (status:"error" or Err)
Handler->>JS: Clear injected credentials
Handler-->>Client: Return error
Note over Persist: No writes
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Comment |
senamakel
changed the title
enabled flag
6 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/openhuman/skills/schemas.rs (1)
902-910: Consider rolling backsetup_completeifstart_skillfails.If
start_skillfails afterset_setup_complete(true), the skill remains marked as "enabled" but isn't actually running. On next application restart, the runtime will attempt to auto-start it (becausesetup_completeis true), which may fail again.This might be intentional (retry on restart), but if not:
💡 Optional: Rollback on failure
fn handle_skills_enable(params: Map<String, Value>) -> ControllerFuture { Box::pin(async move { let p: SkillIdParams = serde_json::from_value(Value::Object(params)).map_err(|e| e.to_string())?; let engine = require_engine()?; engine.preferences().set_setup_complete(&p.skill_id, true); - engine.start_skill(&p.skill_id).await?; + if let Err(e) = engine.start_skill(&p.skill_id).await { + // Rollback setup_complete so auto-start doesn't retry a broken skill + engine.preferences().set_setup_complete(&p.skill_id, false); + return Err(e); + } Ok(serde_json::json!({ "ok": true })) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/schemas.rs` around lines 902 - 910, handle_skills_enable currently calls engine.preferences().set_setup_complete(&p.skill_id, true) before awaiting engine.start_skill(&p.skill_id), so if start_skill fails the preference remains true; change the flow to revert setup_complete to false on start_skill failure by calling set_setup_complete(&p.skill_id, false) in the error path (or only set to true after a successful start), ensuring you handle the async error from start_skill and still propagate the error from handle_skills_enable; reference the functions handle_skills_enable, engine.preferences().set_setup_complete, and engine.start_skill when making the change.src/openhuman/skills/qjs_skill_instance/instance.rs (1)
259-264: Minor: String-contains check for logging is fragile but acceptable.The detection of non-null credentials via
!start_args.contains("\"oauth\":null")works but could be brittle if JSON formatting changes. Since this is only for informational logging and doesn't affect control flow, it's fine as-is.A slightly more robust alternative would be to check the
serde_json::Valuebefore serialization:💡 Optional improvement
let start_args = build_start_credentials_arg(&data_dir); + // For logging: check the parsed values before serialization + let oauth_path = data_dir.join("oauth_credential.json"); + let auth_path = data_dir.join("auth_credential.json"); + let has_oauth = oauth_path.exists() && std::fs::read_to_string(&oauth_path).map(|s| !s.trim().is_empty()).unwrap_or(false); + let has_auth = auth_path.exists() && std::fs::read_to_string(&auth_path).map(|s| !s.trim().is_empty()).unwrap_or(false); log::info!( "[skill:{}] Calling start() with credentials (oauth={}, auth={})", config.skill_id, - !start_args.contains("\"oauth\":null"), - !start_args.contains("\"auth\":null"), + has_oauth, + has_auth, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/qjs_skill_instance/instance.rs` around lines 259 - 264, The current info log in instance.rs uses fragile string contains checks on start_args to infer credential presence; instead, parse or build the credentials as a serde_json::Value (or inspect the existing Value used to produce start_args) and check the actual keys/values (e.g., whether the "oauth" and "auth" fields are null or missing) before logging; update the log in the start() context to use config.skill_id and booleans derived from that serde_json::Value rather than string.contains to make the informational message more robust.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/openhuman/skills/qjs_skill_instance/instance.rs`:
- Around line 259-264: The current info log in instance.rs uses fragile string
contains checks on start_args to infer credential presence; instead, parse or
build the credentials as a serde_json::Value (or inspect the existing Value used
to produce start_args) and check the actual keys/values (e.g., whether the
"oauth" and "auth" fields are null or missing) before logging; update the log in
the start() context to use config.skill_id and booleans derived from that
serde_json::Value rather than string.contains to make the informational message
more robust.
In `@src/openhuman/skills/schemas.rs`:
- Around line 902-910: handle_skills_enable currently calls
engine.preferences().set_setup_complete(&p.skill_id, true) before awaiting
engine.start_skill(&p.skill_id), so if start_skill fails the preference remains
true; change the flow to revert setup_complete to false on start_skill failure
by calling set_setup_complete(&p.skill_id, false) in the error path (or only set
to true after a successful start), ensuring you handle the async error from
start_skill and still propagate the error from handle_skills_enable; reference
the functions handle_skills_enable, engine.preferences().set_setup_complete, and
engine.start_skill when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f6422a8-a87e-41cf-8299-8f83428ce654
📒 Files selected for processing (7)
src/openhuman/skills/preferences.rssrc/openhuman/skills/qjs_engine.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/instance.rssrc/openhuman/skills/qjs_skill_instance/js_handlers.rssrc/openhuman/skills/schemas.rs
💤 Files with no reviewable changes (1)
- src/openhuman/skills/qjs_engine.rs
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
2 participants
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
enabled+setup_completedown to a singlesetup_completeflag —resolve_should_startis nowsetup_complete OR manifest.auto_start.handle_oauth_completeandhandle_auth_completearound the same contract: temp-inject the new credentials, callstart({oauth, auth, validate:true}), persist to disk only when start returns{status:'complete'}, otherwise roll back.onOAuthComplete/onAuthCompleteJS hooks.start()is now the single entry point a skill implements for "I am now connected — go activate."{oauth, auth}credential bag into thestartlifecycle on instance spawn (build_start_credentials_argreadsoauth_credential.json/auth_credential.jsonfrom disk).Problem
The skill lifecycle had two parallel "is this skill on?" states (
enabledvssetup_complete) and three places where credentials could be wired up (onOAuthComplete,onAuthComplete,start). That meant skills could end up half-activated — credential persisted to disk but cron never registered, or cron registered against a credential the upstream API rejects on the next call. The tri-state was also a footgun for the test harness, which had to know which RPC to send in which order.Solution
preferences.rs: dropenabled,is_enabled,set_enabled,get_or_default. Tests rewritten to drop theenabledreferences.schemas.rs:handle_skills_enablebecomes a compat shim (set_setup_complete(true) + start_skill);handle_skills_disableis the inverse;handle_skills_is_enabledreadssetup_complete.qjs_engine.rs: removeenable_skill/disable_skill/is_skill_enabled.event_loop/rpc_handlers.rs:handle_oauth_completeandhandle_auth_completenow both temp-inject credentials → callstart({validate:true})viahandle_js_call→ check{status}→ rollback bridge state if validation failed (no disk write), otherwise persistoauth_credential.json/auth_credential.json(andclient_key.jsonfor encrypted OAuth). The oldonOAuthComplete/onAuthCompleteJS calls are deleted.qjs_skill_instance/instance.rs: newbuild_start_credentials_arg(data_dir)reads the persisted credential files and produces the{oauth, auth}bag passed tocall_lifecycle("start", ...)on spawn. All othercall_lifecyclecallers updated to passNone.js_handlers.rs:call_lifecyclenow takesargs_json: Option<&str>so callers can pass JSON intostart({...}).Submission Checklist
cargo test -p openhuman_corefor the rewritten preferences module (existing assertions adjusted for the droppedenabledfield).src/core/notion/live-test-stress.ts(12/12 passes once the matching skills-side fixes land in refactor(notion+gmail): unify auth lifecycle on start(), fix proxy /v1 prefix regression openhuman-skills#12).///onhandle_oauth_complete,handle_auth_complete, andbuild_start_credentials_argexplain the validate-then-persist contract.Impact
start()as authoritative (no separate OAuth/auth complete hooks). The matching TypeScript skill rewrites land in refactor(notion+gmail): unify auth lifecycle on start(), fix proxy /v1 prefix regression openhuman-skills#12.skills_enable/skills_disable/skills_is_enabledJSON-RPC methods still work as compat shims for existing UI code paths; nothing inapp/needs changes.skill-preferences.jsonfiles keep working —enabledis just ignored if present.Related
skills_enable/skills_is_enabledcompat shims onceapp/switches toskills_set_setup_completedirectly.Summary by CodeRabbit