-
Notifications
You must be signed in to change notification settings - Fork 52
Fix/191 skills reliability upstream #282
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
9bb5a91
9a29b56
0f8957d
b0b110e
2947194
a1688c7
b2cf63e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| /** Structured chat send / delivery errors (issue #219) — stable `code` for analytics and tests. */ | ||
|
|
||
| export type ChatSendErrorCode = | ||
| | 'socket_disconnected' | ||
| | 'local_model_failed' | ||
| | 'cloud_send_failed' | ||
| | 'voice_transcription' | ||
| | 'microphone_unavailable' | ||
| | 'microphone_recording' | ||
| | 'microphone_access' | ||
| | 'voice_playback' | ||
| | 'safety_timeout'; | ||
|
|
||
| export interface ChatSendError { | ||
| code: ChatSendErrorCode; | ||
| message: string; | ||
| } | ||
|
|
||
| export const chatSendError = (code: ChatSendErrorCode, message: string): ChatSendError => ({ | ||
| code, | ||
| message, | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,11 +30,13 @@ import { | |
| runtimeSkillDataRead, | ||
| runtimeSkillDataWrite, | ||
| } from "../../utils/tauriCommands"; | ||
| import { toolExecutionTimeoutMsFromEnv, withTimeout } from "../../utils/withTimeout"; | ||
| // Env vars kept for reverse RPC compatibility (may be used by skills via state) | ||
|
|
||
|
|
||
| class SkillManager { | ||
| private runtimes = new Map<string, SkillRuntime>(); | ||
| private resyncAfterReconnectInProgress = false; | ||
|
|
||
| /** | ||
| * Get skill-specific load parameters (e.g., wallet address for wallet skill) | ||
|
|
@@ -110,6 +112,21 @@ class SkillManager { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * After realtime socket reconnect: refresh tool lists for every running skill so | ||
| * `tool:sync` matches the Rust engine (issue #215). | ||
| */ | ||
| async resyncRunningSkillsAfterReconnect(): Promise<void> { | ||
| if (this.resyncAfterReconnectInProgress) return; | ||
| this.resyncAfterReconnectInProgress = true; | ||
| try { | ||
| const ids = [...this.runtimes.keys()]; | ||
| await Promise.all(ids.map((id) => this.activateSkill(id))); | ||
| } finally { | ||
| this.resyncAfterReconnectInProgress = false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Activate a skill that has completed setup — list its tools and mark as ready. | ||
| */ | ||
|
|
@@ -197,7 +214,12 @@ class SkillManager { | |
| console.error(`[SkillManager] callTool failed — skill "${skillId}" has no running runtime`); | ||
| throw new Error(`Skill ${skillId} is not running`); | ||
| } | ||
| const result = await runtime.callTool(name, args); | ||
| const timeoutMs = toolExecutionTimeoutMsFromEnv(); | ||
| const result = await withTimeout( | ||
| runtime.callTool(name, args), | ||
| timeoutMs, | ||
| `[SkillManager] callTool skill="${skillId}" tool="${name}"`, | ||
| ); | ||
| console.log(`[SkillManager] callTool result skill="${skillId}" tool="${name}" isError=${result.isError}`); | ||
| return result; | ||
| } | ||
|
|
@@ -229,16 +251,25 @@ class SkillManager { | |
| * Progress updates are published to Redux via the skill's state fields. | ||
| */ | ||
| async triggerSync(skillId: string): Promise<void> { | ||
| const timeoutMs = toolExecutionTimeoutMsFromEnv(); | ||
| const runtime = this.runtimes.get(skillId); | ||
| if (runtime) { | ||
| await runtime.triggerSync(); | ||
| await withTimeout( | ||
| runtime.triggerSync(), | ||
| timeoutMs, | ||
| `[SkillManager] triggerSync skill="${skillId}"`, | ||
| ); | ||
| } else { | ||
| // Try via core RPC pass-through | ||
| try { | ||
| await callCoreRpc({ | ||
| method: "openhuman.skills_sync", | ||
| params: { skill_id: skillId }, | ||
| }); | ||
| await withTimeout( | ||
| callCoreRpc({ | ||
| method: "openhuman.skills_sync", | ||
| params: { skill_id: skillId }, | ||
| }), | ||
| timeoutMs, | ||
| `[SkillManager] skills_sync skill="${skillId}"`, | ||
| ); | ||
| } catch { | ||
| // Skill not running — skip sync silently | ||
| } | ||
|
Comment on lines
264
to
275
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't swallow This blanket 🤖 Prompt for AI Agents |
||
|
|
@@ -366,30 +397,27 @@ class SkillManager { | |
| // Revoke OAuth credential before stopping so the running skill can clean up | ||
| // its in-memory state and the event loop deletes oauth_credential.json. | ||
| let revokeSucceeded = false; | ||
| if (credentialId) { | ||
| try { | ||
| await rpcRevokeOAuth(skillId, credentialId); | ||
| revokeSucceeded = true; | ||
| } catch (err) { | ||
| console.debug( | ||
| "[SkillManager] oauth/revoked failed (runtime may be stopped):", | ||
| err, | ||
| ); | ||
| } | ||
| try { | ||
| await rpcRevokeOAuth(skillId, credentialId ?? "default"); | ||
| revokeSucceeded = true; | ||
| } catch (err) { | ||
| console.debug( | ||
| "[SkillManager] oauth/revoked failed (runtime may be stopped):", | ||
| err, | ||
| ); | ||
| } | ||
|
|
||
| await this.stopSkill(skillId); | ||
|
|
||
| // Host-side fallback: if the RPC couldn't reach the runtime (already stopped, | ||
| // or non-OAuth skill), delete the persisted credential file so it isn't | ||
| // restored on next start. | ||
| if (!revokeSucceeded) { | ||
| await removePersistedOAuthCredential(skillId).catch((err) => { | ||
| console.debug( | ||
| "[SkillManager] host-side credential cleanup failed:", | ||
| err, | ||
| ); | ||
| }); | ||
| try { | ||
| await this.stopSkill(skillId); | ||
| } finally { | ||
| if (!revokeSucceeded) { | ||
| await removePersistedOAuthCredential(skillId).catch((err) => { | ||
| console.debug( | ||
| "[SkillManager] host-side credential cleanup failed:", | ||
| err, | ||
| ); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| await rpcSetSetupComplete(skillId, false).catch(() => {}); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't re-activate setup-incomplete skills on reconnect.
startSkill()adds runtimes tothis.runtimesbefore setup/OAuth completion, but this path callsactivateSkill()for every key. Sinceapp/src/services/socketService.ts:163-173invokes this on everyconnect, setup-incomplete skills can getlistTools()/syncToolsToBackend()prematurely. Please gate this on already-ready skills, or track activated IDs separately.🤖 Prompt for AI Agents