Fix/skill start issue

app/src/components/skills/SkillSetupWizard.tsx

@@ -12,7 +12,7 @@
  * as a single "managed" auth mode.
  */
 
-import { useState, useEffect, useCallback } from "react";
+import { useState, useEffect, useCallback, useRef } from "react";
 import { useSkillSnapshot } from "../../lib/skills/hooks.ts";
 import { skillManager } from "../../lib/skills/manager.ts";
 import { getSkillSnapshot, installSkill, listAvailable, setSetupComplete, startSkill } from "../../lib/skills/skillsApi.ts";
@@ -24,6 +24,25 @@ import SetupFormRenderer from "./SetupFormRenderer.tsx";
 import AuthModeSelector from "./AuthModeSelector.tsx";
 import { IS_DEV } from "../../utils/config.ts";
 
+const SKILL_RUNNING_WAIT_MS = 10_000;
+const SKILL_RUNNING_POLL_MS = 250;
+
+/** Poll `skills_status` until the lifecycle reports `running` (or throw on error / timeout). */
+const waitForSkillRunning = async (skillId: string): Promise<void> => {
+  const deadline = Date.now() + SKILL_RUNNING_WAIT_MS;
+  while (Date.now() < deadline) {
+    const snapshot = await getSkillSnapshot(skillId);
+    if (snapshot.status === "running") return;
+    if (snapshot.status === "error") {
+      throw new Error(snapshot.error ? String(snapshot.error) : "Skill failed to start");
+    }
+    await new Promise<void>(resolve => {
+      setTimeout(resolve, SKILL_RUNNING_POLL_MS);
+    });
+  }
+  throw new Error("Timed out waiting for skill to start");
+};
+
 interface SkillSetupWizardProps {
   skillId: string;
   onComplete: () => void;
@@ -57,6 +76,15 @@ export default function SkillSetupWizard({
   onCancel,
 }: SkillSetupWizardProps) {
   const [state, setState] = useState<WizardState>({ phase: "loading" });
+  /** Ensures managed OAuth auto-advance runs once per browser-login attempt. */
+  const managedOAuthAdvancedRef = useRef(false);
+  /** Ensures legacy OAuth persistence + complete runs once per connected transition. */
+  const legacyOAuthAdvancedRef = useRef(false);
+
+  useEffect(() => {
+    managedOAuthAdvancedRef.current = false;
+    legacyOAuthAdvancedRef.current = false;
+  }, [skillId]);
 
   // Watch skill snapshot for OAuth/managed completion
   const snap = useSkillSnapshot(skillId);
@@ -66,12 +94,11 @@ export default function SkillSetupWizard({
 
   const transitionToSetup = useCallback(async () => {
     try {
-      // Ensure skill runtime is running
-      try {
-        await startSkill(skillId);
-      } catch {
-        // May already be running
-      }
+      // Ensure skill runtime is running.
+      // Note: if the skill is already running, startSkill returns Ok (not an
+      // error), so any exception here is a real failure that must surface.
+      await startSkill(skillId);
+      await waitForSkillRunning(skillId);
 
       const firstStep = await skillManager.startSetup(skillId);
       if (!firstStep) {
@@ -105,6 +132,7 @@ export default function SkillSetupWizard({
           return;
         }
         await openUrl(data.oauthUrl);
+        managedOAuthAdvancedRef.current = false;
         setState({ phase: "auth_managed_waiting", mode });
       } catch (err) {
         const msg = err instanceof Error ? err.message : String(err);
@@ -138,12 +166,11 @@ export default function SkillSetupWizard({
       setState({ phase: "auth_submitting", mode });
 
       try {
-        // Ensure skill runtime is running
-        try {
-          await startSkill(skillId);
-        } catch {
-          // May already be running
-        }
+        // Ensure skill runtime is running.
+        // Note: if the skill is already running, startSkill returns Ok (not an
+        // error), so any exception here is a real failure that must surface.
+        await startSkill(skillId);
+        await waitForSkillRunning(skillId);
 
         // Build credential payload
         const credentials =
@@ -187,19 +214,61 @@ export default function SkillSetupWizard({
   // Detect managed OAuth completion
   useEffect(() => {
     if (state.phase === "auth_managed_waiting" && isConnected) {
-      setTimeout(() => { transitionToSetup(); }, 0);
+      if (managedOAuthAdvancedRef.current) return;
+      managedOAuthAdvancedRef.current = true;
+      void (async () => {
+        try {
+          await startSkill(skillId);
+          await waitForSkillRunning(skillId);
+        } catch (e) {
+          console.warn("[SkillSetupWizard] post-OAuth startSkill:", e);
+          setState({ phase: "error", message: "Failed to start skill after OAuth." });
+          return;
+        }
+        try {
+          const firstStep = await skillManager.startSetup(skillId);
+          if (firstStep) {
+            setState({ phase: "step", step: firstStep });
+          } else {
+            await setSetupComplete(skillId, true);
+            setState({
+              phase: "complete",
+              message: "Successfully connected! You can close this window.",
+            });
+          }
+        } catch (e) {
+          console.warn("[SkillSetupWizard] post-OAuth startSetup:", e);
+          setState({ phase: "error", message: "Setup failed" });
+        }
+      })();
+      return;
     }
     // Legacy OAuth completion
     if (
       (state.phase === "oauth" || state.phase === "oauth_waiting") &&
       isConnected
     ) {
-      setSetupComplete(skillId, true).catch(() => {});
-      setTimeout(() => {
-        setState({ phase: "complete", message: "Successfully connected!" });
-      }, 0);
+      if (legacyOAuthAdvancedRef.current) return;
+      legacyOAuthAdvancedRef.current = true;
+      void (async () => {
+        try {
+          await setSetupComplete(skillId, true);
+          setState({
+            phase: "complete",
+            message:
+              "Successfully connected! You can close this window.",
+          });
+        } catch (e) {
+          console.warn("[SkillSetupWizard] legacy OAuth setSetupComplete:", e);
+          legacyOAuthAdvancedRef.current = false;
+          setState({
+            phase: "error",
+            message: "Failed to save setup state.",
+          });
+        }
+      })();
     }
-  }, [isConnected, state.phase, skillId, transitionToSetup]);
+  }, [isConnected, state.phase, skillId]);
 
   // Start the setup flow on mount
   useEffect(() => {
@@ -259,6 +328,7 @@ export default function SkillSetupWizard({
         try {
           await startSkill(skillId);
           console.log("[SkillSetupWizard] skill started via RPC", skillId);
+          await waitForSkillRunning(skillId);
         } catch (startErr) {
           console.warn("[SkillSetupWizard] runtime start failed:", startErr);
           if (!cancelled) {

app/src/lib/skills/hooks.ts

@@ -19,6 +19,21 @@ import {
   type SkillSnapshotRpc,
 } from './skillsApi';
 
+/** Snapshot used when `skills_status` fails (skill not loaded in runtime yet, RPC error, etc.). */
+const offlineSkillSnapshot = (
+  skillId: string,
+  previous?: SkillSnapshotRpc | null,
+): SkillSnapshotRpc => ({
+  skill_id: skillId,
+  name: previous?.name ?? '',
+  status: previous?.status ?? 'installed',
+  tools: previous?.tools ?? [],
+  state: previous?.state ?? {},
+  setup_complete: previous?.setup_complete ?? false,
+  connection_status: previous?.connection_status ?? 'offline',
+  error: previous?.error,
+});
+
 // ---------------------------------------------------------------------------
 // Legacy pure function kept for compatibility (used by sync.ts, skillsSyncUi)
 // ---------------------------------------------------------------------------
@@ -80,7 +95,13 @@ export function useSkillSnapshot(skillId: string | undefined): SkillSnapshotRpc
       const s = await getSkillSnapshot(skillId);
       if (mountedRef.current) setSnap(s);
     } catch {
-      // Skill may not be running yet — that's OK
+      // Skill may not be in the runtime yet (e.g. never started) — `skills_status`
+      // returns an error and would otherwise leave `snap` null forever.  UI such as
+      // SkillSetupModal waits for a non-null snapshot before leaving its loading
+      // state, so we synthesize an offline snapshot instead of staying stuck.
+      if (mountedRef.current) {
+        setSnap(previous => offlineSkillSnapshot(skillId, previous));
+      }
     }
   }, [skillId]);
 

app/src/lib/skills/manager.ts

@@ -385,22 +385,6 @@ class SkillManager {
       }
     }
 
-    // Kick off an initial sync so the user sees fresh data immediately
-    // after connecting, rather than waiting for the next cron tick.
-    // The Rust core no longer auto-triggers sync on oauth/complete
-    // (removed in commit 840b1d3c), so the frontend drives it here.
-    // Fire-and-forget: any failure is logged but must not block the
-    // OAuth completion flow.
-    try {
-      console.log(`[SkillManager] kicking initial sync after OAuth for '${skillId}'`);
-      await this.triggerSync(skillId);
-    } catch (syncErr) {
-      console.warn(
-        `[SkillManager] initial post-OAuth sync failed for '${skillId}':`,
-        syncErr,
-      );
-    }
-
     emitSkillStateChange(skillId);
   }
 

app/src/pages/__tests__/Skills.third-party-gmail-sync.test.tsx

@@ -5,6 +5,7 @@
 import { fireEvent, screen, waitFor } from '@testing-library/react';
 import { beforeEach, describe, expect, it, vi } from 'vitest';
 
+import '../../test/mockDefaultSkillStatusHooks';
 import { renderWithProviders } from '../../test/test-utils';
 import Skills from '../Skills';
 

app/src/pages/__tests__/Skills.third-party-notion-debug-tools.test.tsx

@@ -6,6 +6,7 @@
 import { fireEvent, screen, waitFor } from '@testing-library/react';
 import { beforeEach, describe, expect, it, vi } from 'vitest';
 
+import '../../test/mockDefaultSkillStatusHooks';
 import { renderWithProviders } from '../../test/test-utils';
 import Skills from '../Skills';
 

app/src/test/mockDefaultSkillStatusHooks.ts

@@ -0,0 +1,36 @@
+/**
+ * Shared Vitest mocks for screen-intelligence / autocomplete / voice status hooks.
+ * Import this module first in Skills page tests so `Skills` does not require `CoreStateProvider`.
+ */
+import { vi } from 'vitest';
+
+/** Shared offline-shaped fields for skill status hook mocks (avoid drift across hooks). */
+const offlineStatusBase = {
+  connectionStatus: 'offline' as const,
+  statusDot: 'bg-stone-400',
+  statusLabel: 'Offline',
+  statusColor: 'text-stone-500',
+  ctaLabel: 'Enable',
+  ctaVariant: 'sage' as const,
+};
+
+vi.mock('../features/screen-intelligence/useScreenIntelligenceSkillStatus', () => ({
+  useScreenIntelligenceSkillStatus: () => ({
+    ...offlineStatusBase,
+    allPermissionsGranted: false,
+    platformUnsupported: false,
+  }),
+}));
+
+vi.mock('../features/autocomplete/useAutocompleteSkillStatus', () => ({
+  useAutocompleteSkillStatus: () => ({ ...offlineStatusBase, platformUnsupported: false }),
+}));
+
+vi.mock('../features/voice/useVoiceSkillStatus', () => ({
+  useVoiceSkillStatus: () => ({
+    ...offlineStatusBase,
+    sttModelMissing: false,
+    voiceStatus: null,
+    serverStatus: null,
+  }),
+}));

app/src/utils/desktopDeepLinkListener.ts

@@ -188,10 +188,8 @@ const handleOAuthDeepLink = async (parsed: URL) => {
       console.warn(`[DeepLink] Could not start skill '${skillId}' in runtime:`, startErr);
     }
 
-    // 3. Notify the running skill of the OAuth credential, mark setup_complete,
-    //    activate (list tools, sync tools to backend), and kick an initial sync.
-    //    The initial sync is driven by SkillManager.notifyOAuthComplete — the
-    //    Rust core no longer auto-syncs on oauth/complete (removed in 840b1d3c).
+    // 3. Notify the running skill of the OAuth credential and mark setup_complete.
+    //    Initial data sync is left to the user / cron / skill UI — not auto-run here.
     try {
       await skillManager.notifyOAuthComplete(skillId, integrationId, undefined, { clientKeyShare });
       console.log(`[DeepLink] OAuth complete sent to skill '${skillId}'`);

app/test/e2e/specs/skill-execution-flow.spec.ts

@@ -113,6 +113,27 @@ describe('Skill execution (UI + core RPC)', () => {
     expect(stop.result?.success === true).toBe(true);
   });
 
+  it('skills_set_setup_complete + skills_status without start (OAuth persistence path)', async () => {
+    try {
+      const set = await callOpenhumanRpc('openhuman.skills_set_setup_complete', {
+        skill_id: E2E_RUNTIME_SKILL_ID,
+        complete: true,
+      });
+      expect(set.ok).toBe(true);
+
+      const st = await callOpenhumanRpc('openhuman.skills_status', {
+        skill_id: E2E_RUNTIME_SKILL_ID,
+      });
+      expect(st.ok).toBe(true);
+      expect(st.result?.setup_complete === true).toBe(true);
+    } finally {
+      await callOpenhumanRpc('openhuman.skills_set_setup_complete', {
+        skill_id: E2E_RUNTIME_SKILL_ID,
+        complete: false,
+      });
+    }
+  });
+
   it('Skills page loads (UI surface for installed tools)', async () => {
     await navigateToSkills();
     await browser.pause(2_000);

app/test/e2e/specs/skill-oauth.spec.ts

@@ -2,6 +2,12 @@
 /**
  * OAuth-oriented skills UI smoke test (issue #221).
  * Verifies Skills page shows connection/setup affordances after auth.
+ *
+ * JSON-RPC coverage for OAuth + setup persistence lives in Rust integration tests
+ * (`tests/json_rpc_e2e.rs`: `json_rpc_skills_status_reflects_setup_complete_without_runtime`,
+ * `json_rpc_skills_oauth_complete_after_start`). The Playwright `skill-execution-flow.spec.ts`
+ * suite exercises `skills_start` → tools against the seeded `e2e-runtime` skill over the same
+ * HTTP JSON-RPC path the desktop UI uses.
  */
 import { waitForApp, waitForAppReady } from '../helpers/app-helpers';
 import { triggerAuthDeepLinkBypass } from '../helpers/deep-link-helpers';