Fix/191 skills reliability upstream#282
Merged
senamakel merged 7 commits intotinyhumansai:mainfrom Apr 2, 2026
Merged
Conversation
…tinyhumansai#191) - Add openhuman.skills_data_stats and SkillDataDirectoryStats (disk usage). - Centralize tool execution timeout via OPENHUMAN_TOOL_TIMEOUT_SECS (tool_timeout). - Apply timeout to skill event loop, agent tool loop, harness default, delegate. - Ping scheduler: merge connection_error into published_state via registry. - JSON-RPC e2e: assert skills_data_stats. Closes tinyhumansai#214 Closes tinyhumansai#218 Part of tinyhumansai#213 (backend) Made-with: Cursor
…errors (tinyhumansai#191) - useSkillDataDirectoryStats + Skills.tsx merge disk stats with skill state. - resyncRunningSkillsAfterReconnect on socket connect; disconnectSkill OAuth cleanup in finally. - withTimeout for callTool/triggerSync; VITE_TOOL_TIMEOUT_SECS in app .env.example. - Structured ChatSendError in Conversations (data-chat-send-error-code). Closes tinyhumansai#213 Closes tinyhumansai#215 Closes tinyhumansai#216 Closes tinyhumansai#217 Closes tinyhumansai#219 Made-with: Cursor
- shared-flows: 5-step onboarding, completeOnboardingIfVisible. - auth-access-control + voice-mode use shared helpers. - skills-registry: named skill assertion; new skill-oauth, multi-round, reconnect, lifecycle specs. Closes tinyhumansai#220 Closes tinyhumansai#221 Closes tinyhumansai#222 Closes tinyhumansai#223 Closes tinyhumansai#224 Part of tinyhumansai#189 (E2E helpers) Made-with: Cursor
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds configurable tool execution timeouts (frontend + backend env vars), enforces timeouts in tool calls and syncs, surfaces typed chat send errors, gathers skill data directory stats (RPC + runtime), ensures skills are re-synced after socket reconnect, hardens OAuth disconnect cleanup, and consolidates/expands E2E onboarding and skill-focused tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Socket as SocketService
participant SkillMgr as SkillManager (frontend)
participant Core as Runtime/Core RPC
participant ToolCfg as ToolTimeoutModule (Rust)
Note over Client,Core: Socket reconnect + tool call timeout flow
Client->>Socket: socket 'connect' event
Socket->>Socket: dispatch connected + set socket id
Socket->>Core: syncToolsToBackend()
Core-->>Socket: ok
Socket->>SkillMgr: resyncRunningSkillsAfterReconnect()
SkillMgr->>Core: activateSkill(id) [parallel for each]
Core-->>SkillMgr: tools refreshed
Client->>SkillMgr: callTool(skillId, name, args)
SkillMgr->>SkillMgr: withTimeout(pending RPC, TOOL_TIMEOUT_MS)
SkillMgr->>Core: runtime.callTool(...)
Core->>ToolCfg: tool_execution_timeout_secs()
ToolCfg-->>Core: timeout value
alt tool completes before timeout
Core-->>SkillMgr: tool result
SkillMgr-->>Client: result
else timeout fires
Core-->>SkillMgr: error "timed out after Xs"
SkillMgr-->>Client: timeout error (typed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Fix Type Check workflow: prettier --check and cargo fmt --check. Made-with: Cursor
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/test/e2e/specs/skills-registry.spec.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting issues flagged by CI.
The pipeline reports Prettier --check failures for this file. Run
prettier --writeor your project's formatting command to resolve the style issues before merging.#!/bin/bash # Check what Prettier wants to change in this file cd app && npx prettier --check test/e2e/specs/skills-registry.spec.ts 2>&1 || true npx prettier --write test/e2e/specs/skills-registry.spec.ts --dry-run 2>&1 | head -50🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/skills-registry.spec.ts` at line 1, Run your project's formatter on the failing test file to fix Prettier style errors: open app/test/e2e/specs/skills-registry.spec.ts (the skills-registry.spec.ts test file) and apply the project's formatting command (e.g., from the repo root run npx prettier --write app/test/e2e/specs/skills-registry.spec.ts or your configured format script), then re-run lint/CI to ensure Prettier --check passes and commit the formatted file.src/openhuman/agent/loop_/tool_loop.rs (1)
288-329:⚠️ Potential issue | 🟡 MinorFix formatting to pass CI (cargo fmt).
The pipeline failure indicates formatting differences at line 285. Run
cargo fmtto fix the line wrapping around thetokio::time::timeoutcall.The timeout configuration changes are correct — the tool loop now uses centralized timeout settings and properly logs/reports the configured value.
🔧 Run cargo fmt to fix
cargo fmt --all🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/agent/loop_/tool_loop.rs` around lines 288 - 329, The code fails CI due to formatting around the tokio::time::timeout invocation; run rustfmt (cargo fmt --all) and reflow the tokio::time::timeout(...) call so it matches project formatting rules (ensure the call that uses tool_deadline and tool.execute(call.arguments.clone()) is wrapped consistently), then re-run cargo fmt to commit the formatted changes for tool_deadline, timeout_secs, and the tokio::time::timeout usage.app/test/e2e/specs/voice-mode.spec.ts (1)
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting issues.
The pipeline reports code style issues detected by
prettier --check. Runprettier --writeon this file before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/voice-mode.spec.ts` at line 1, Prettier reported formatting issues in the test file (the one that begins with the top-line directive "// `@ts-nocheck`"); run prettier --write on that file to reformat it, review the resulting changes to ensure no unintended code edits, then stage and commit the reformatted file so the pipeline passes the prettier --check step.
🧹 Nitpick comments (9)
app/src/pages/Conversations.tsx (1)
1-6: Prettier formatting issue flagged by CI.The pipeline reports Prettier style issues for this file. Please run Prettier before merge:
yarn prettier --write app/src/pages/Conversations.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Conversations.tsx` around lines 1 - 6, Prettier style errors are present in Conversations.tsx; run the formatter and update the file so CI passes by executing the project formatter (e.g., run yarn prettier --write app/src/pages/Conversations.tsx or your repo’s configured prettier script), then stage and commit the formatted changes; ensure imports like convertFileSrc, React hooks (useEffect, useRef, useState), Markdown, useNavigate, and the ChatSendError/chatSendError imports remain unchanged except for formatting diffs.app/src/chat/chatSendError.ts (1)
1-24: Prettier formatting issue flagged by CI.The pipeline reports Prettier style issues for this file. Please run
yarn prettier --write app/src/chat/chatSendError.tsto fix formatting before merge.Additionally, per coding guidelines preferring arrow functions, consider converting the factory:
♻️ Optional: Convert to arrow function
-export function chatSendError( - code: ChatSendErrorCode, - message: string, -): ChatSendError { - return { code, message }; -} +export const chatSendError = ( + code: ChatSendErrorCode, + message: string, +): ChatSendError => ({ code, message });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/chat/chatSendError.ts` around lines 1 - 24, Fix the Prettier formatting for this file by running Prettier (e.g., yarn prettier --write) on the file to satisfy the CI/style rules, then tidy up exports/types if Prettier changes them; optionally refactor the factory function chatSendError into an arrow function (retain the ChatSendError and ChatSendErrorCode types and the same return shape) to match the codebase preference for arrow functions.app/test/e2e/specs/skill-multi-round.spec.ts (1)
33-37: Add failure diagnostics for faster debugging.Per coding guidelines, E2E specs should include failure diagnostics. If none of the expected texts are found, the assertion fails without context.
🩺 Add diagnostic logging on failure
const ok = (await textExists('Message OpenHuman')) || (await textExists('Conversation')) || (await textExists('Type a message')); + if (!ok) { + const { dumpAccessibilityTree } = await import('../helpers/element-helpers'); + const tree = await dumpAccessibilityTree(); + console.log('[MultiRoundE2E] Conversations view not found. Tree:\n', tree.slice(0, 4000)); + } expect(ok).toBe(true);As per coding guidelines: "Assert both UI outcomes and backend/mock effects when relevant in E2E specs; add failure diagnostics (request logs,
dumpAccessibilityTree()) for faster debugging".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/skill-multi-round.spec.ts` around lines 33 - 37, The test currently checks presence via textExists(...) and fails silently; update the spec so that if ok is false you gather failure diagnostics before asserting: call dumpAccessibilityTree() and capture relevant request/network logs (e.g., any existing request log helper or page network log) and output them (console.error) along with the page HTML/content, then run expect(ok).toBe(true). Reference textExists to detect absence and add diagnostic calls to dumpAccessibilityTree() and the project’s network/request-log helper (or page.content()) so failures include those diagnostics.app/test/e2e/specs/skill-lifecycle.spec.ts (2)
35-39: Consider asserting backend/mock effects for stronger coverage.The test only verifies UI outcomes. Per E2E guidelines, consider also checking request logs for expected backend calls (e.g., skill registry fetch) to ensure the full flow works correctly.
Example: Add request log assertion
// After navigating to Skills page, verify the backend was called const registryRequest = getRequestLog().find( r => r.method === 'POST' && r.url.includes('skills') ); // Or simply log for debugging if no specific endpoint is expected: console.log('[LifecycleE2E] Request log:', getRequestLog());Based on learnings: "Assert both UI outcomes and backend/mock effects when relevant in E2E specs; add failure diagnostics (request logs,
dumpAccessibilityTree()) for faster debugging"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/skill-lifecycle.spec.ts` around lines 35 - 39, Update the E2E test to assert backend/mock effects in addition to the UI check: after the existing textExists assertions in the skill-lifecycle.spec.ts test, query the request log with getRequestLog() and assert a call to the skill registry (e.g., find an entry where r.method === 'POST' && r.url.includes('skills') or the exact registry path) to ensure the fetch occurred; additionally, on failure add diagnostic output such as console.log('[LifecycleE2E] Request log:', getRequestLog()) and/or calling dumpAccessibilityTree() to aid debugging.
22-40: Add failure diagnostics for easier debugging.When assertions fail, the current test provides no diagnostic output. Consider adding
dumpAccessibilityTree()on failure for faster debugging.Example: Add diagnostics
const content = (await textExists('Skills')) || (await textExists('Install')) || (await textExists('Available')); + if (!content) { + const tree = await dumpAccessibilityTree(); + console.log('[LifecycleE2E] Skills page content not found. Tree:\n', tree.slice(0, 4000)); + } expect(content).toBe(true);Based on learnings: "Assert both UI outcomes and backend/mock effects when relevant in E2E specs; add failure diagnostics (request logs,
dumpAccessibilityTree()) for faster debugging"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/skill-lifecycle.spec.ts` around lines 22 - 40, Wrap the existing assertions in the test (the hash check and the content/textExists checks executed after navigateToSkills and browser.pause) with a try/catch so that on any assertion failure you call dumpAccessibilityTree() (and optionally other diagnostics like request logs) before rethrowing the error; reference the existing helpers triggerAuthDeepLinkBypass, waitForWindowVisible, waitForWebView, waitForAppReady, completeOnboardingIfVisible, navigateToSkills, textExists and ensure the catch invokes dumpAccessibilityTree() to surface accessibility state for debugging, then rethrow the original error so the test still fails.src/openhuman/skills/qjs_engine.rs (1)
883-894: Filesystem errors are silently swallowed.The
unwrap_or((0, 0))on line 887 means permission errors, broken symlinks, or other I/O failures will return zeros indistinguishable from an empty directory. This is acceptable for a non-critical UI metric, but consider logging a warning when the helper fails so operators can diagnose unexpected zero values.Example: Log errors before swallowing
pub fn skill_data_directory_stats(&self, skill_id: &str) -> SkillDataDirectoryStats { let path = self.skill_data_dir(skill_id); let exists = path.exists(); - let (total_bytes, file_count) = directory_byte_and_file_count(&path).unwrap_or((0, 0)); + let (total_bytes, file_count) = match directory_byte_and_file_count(&path) { + Ok(stats) => stats, + Err(e) => { + log::warn!("[runtime] Failed to compute data stats for '{}': {}", skill_id, e); + (0, 0) + } + }; SkillDataDirectoryStats { exists, path: path.display().to_string(), total_bytes, file_count, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/qjs_engine.rs` around lines 883 - 894, The helper call in skill_data_directory_stats currently swallows IO errors via directory_byte_and_file_count(&path).unwrap_or((0, 0)); change this to handle the Result instead: match or if let Err(err) = directory_byte_and_file_count(&path) and on error log a warning that includes the skill path and err (e.g., via your crate logger) and then set total_bytes and file_count to 0; keep the successful branch returning the computed values and continue constructing SkillDataDirectoryStats with exists and path.display().to_string().src/openhuman/skills/skill_registry.rs (1)
310-331: Consider emittingSKILL_STATE_CHANGEDevent after state updates.The
merge_published_statemethod mutatespublished_statedirectly without emitting theSKILL_STATE_CHANGEDevent. While the frontend compensates with 3-second polling (setInterval(refresh, 3000)in hooks), this is inefficient compared to the event-driven updates used for other state changes. For consistency and to avoid unnecessary polling, emit the event after merging:let mut patch = HashMap::new(); patch.insert("connection_status".to_string(), serde_json::json!("error")); if let Err(e) = registry.merge_published_state(skill_id, patch) { // ... } // Add: registry.broadcast_event(events::SKILL_STATE_CHANGED, json!({ "skill_id": skill_id }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/skill_registry.rs` around lines 310 - 331, merge_published_state mutates published_state but doesn't notify listeners — after successfully merging the patch in merge_published_state, call the registry's broadcast_event with events::SKILL_STATE_CHANGED and a payload containing the skill_id (e.g. serde_json::json!({ "skill_id": skill_id })) so frontends receive the update; locate merge_published_state and add the broadcast_event call immediately before returning Ok(()) (use the existing broadcast_event API on the same struct).app/test/e2e/helpers/shared-flows.ts (1)
188-198: Consider extracting overlay text candidates to a constant.The text variants checked in
onboardingOverlayLikelyVisibleare also partially used in other places (e.g., the comment on line 201). Extracting these to a named constant would improve maintainability if the onboarding step labels change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/helpers/shared-flows.ts` around lines 188 - 198, Extract the list of onboarding label strings used in onboardingOverlayLikelyVisible into a shared constant (e.g., ONBOARDING_OVERLAY_TEXTS) and replace the inline string checks in onboardingOverlayLikelyVisible with iterations over that constant (still calling textExists for each entry); update any other places that reference the same labels (such as the nearby comment/logic that mentions the same candidates) to use the constant so label changes are centralized; keep the function signature and behavior (Promise<boolean>) unchanged and ensure imports/exports are adjusted if the constant is moved to a separate module.app/src/lib/skills/manager.ts (1)
114-121: Consider guarding against socket flapping.
resyncRunningSkillsAfterReconnectis invoked directly on every socketconnectevent (perapp/src/services/socketService.ts:171). If the socket rapidly reconnects (flapping), multiple concurrent resync operations could race, triggering redundantlistToolsandsyncToolsToBackendcalls.Consider adding a debounce or in-progress guard:
Example guard pattern
+ private resyncInProgress = false; + async resyncRunningSkillsAfterReconnect(): Promise<void> { + if (this.resyncInProgress) { + console.debug('[SkillManager] resync already in progress, skipping'); + return; + } + this.resyncInProgress = true; + try { const ids = [...this.runtimes.keys()]; await Promise.all(ids.map((id) => this.activateSkill(id))); + } finally { + this.resyncInProgress = false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/lib/skills/manager.ts` around lines 114 - 121, resyncRunningSkillsAfterReconnect can be invoked repeatedly during socket flapping and cause concurrent activateSkill/listTools/syncToolsToBackend races; add a guard (e.g. a module/class-level boolean like _resyncInProgress or a timestamp-based debounce) inside resyncRunningSkillsAfterReconnect to short-circuit if a resync is already running or recently completed, set the flag before starting, run the existing ids.map activateSkill logic, and clear the flag in a finally block so the flag is always reset on completion or error; update the socket connect caller (socketService connect handler) to rely on this guard instead of attempting its own dedupe.
🤖 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/utils/withTimeout.ts`:
- Around line 27-32: Replace direct access to import.meta.env in
toolExecutionTimeoutMsFromEnv by reading a centralized export from
app/src/utils/config.ts: move the VITE_TOOL_TIMEOUT_SECS parsing/validation into
config.ts (export a numeric TOOL_TIMEOUT_SECS in seconds with same
validation/defaults) and update toolExecutionTimeoutMsFromEnv to use that export
(convert seconds to milliseconds and return Math.round(TOOL_TIMEOUT_SECS *
1000)). Keep the function name toolExecutionTimeoutMsFromEnv and ensure behavior
and bounds (default 120, max 3600, positive) remain unchanged.
In `@app/test/e2e/specs/skill-socket-reconnect.spec.ts`:
- Line 1: The file contains Prettier formatting violations (note the top-line
comment "// `@ts-nocheck`" in the diff); run your project's Prettier formatter
(e.g., run prettier --write on this file or through your npm script) to reformat
the file so it passes prettier --check, then stage the updated file and push the
commit.
In `@app/test/e2e/specs/skills-registry.spec.ts`:
- Around line 134-143: The test should collect failure diagnostics before
failing: after computing hasNamedSkill (using navigateToSkills() and
textExists('Telegram'|'Notion'|'Gmail')), if hasNamedSkill is false call
dumpAccessibilityTree() and dumpRequestLogs() (or the project-equivalent
request-logging helper) and stepLog the request logs, then assert
expect(hasNamedSkill).toBe(true). Add these calls in the same it block around
the existing stepLog/expect so diagnostics are emitted on failure; reference
navigateToSkills(), textExists(), stepLog(), dumpAccessibilityTree(), and
dumpRequestLogs()/request log helper to locate where to add them.
In `@src/openhuman/skills/ping_scheduler.rs`:
- Around line 226-242: The code block updating published state (creating `patch`
and calling `registry.merge_published_state`) is misformatted and failing CI;
run `cargo fmt --all` and reformat the `patch.insert` lines (and surrounding
indentation) so the file compiles with rustfmt, then commit the formatted
changes—specifically check the `patch.insert("connection_status"...)`,
`patch.insert("connection_error"...)`, and the `if let Err(e) =
registry.merge_published_state(skill_id, patch)` block for correct spacing and
line breaks.
In `@src/openhuman/skills/qjs_skill_instance/event_loop.rs`:
- Around line 9-10: Rust import formatting in event_loop.rs is failing rustfmt;
combine and/or reorder the two use statements into a rustfmt-friendly grouped
form (e.g., group crate::openhuman::tool_timeout and crate::openhuman::skills
imports under a single crate::openhuman::{...} or alphabetically sort the use
lines) and then run cargo fmt --all (or cargo fmt) to normalize formatting so
rustfmt checks pass; update the use statements that reference
tool_execution_timeout_duration, tool_execution_timeout_secs and qjs_ops
accordingly.
In `@src/openhuman/tool_timeout.rs`:
- Around line 1-29: Move this standalone module into a folder-style submodule:
create a module directory named tool_timeout and place the current code into its
mod.rs, then update the parent openhuman module to pub mod tool_timeout so the
symbols resolved_secs, tool_execution_timeout_secs, and
tool_execution_timeout_duration remain accessible; keep the OnceLock and
Duration logic unchanged, ensure the functions retain their visibility (pub
where needed), and update any imports/usages to reference the new module path
(e.g. openhuman::tool_timeout::tool_execution_timeout_secs).
In `@src/openhuman/tools/delegate.rs`:
- Around line 3-4: The import block in src/openhuman/tools/delegate.rs fails
rustfmt; run `cargo fmt --all` (or `rustfmt`) to fix wrapping/ordering of the
use statements, then re-run `cargo check` to ensure no formatting CI failures;
specifically ensure the `use
crate::openhuman::tool_timeout::tool_execution_timeout_secs;` and `use
crate::openhuman::providers::{self, Provider};` lines are formatted/ordered
according to rustfmt (adjust wrapping or merge/split imports if rustfmt
suggests) before pushing.
---
Outside diff comments:
In `@app/test/e2e/specs/skills-registry.spec.ts`:
- Line 1: Run your project's formatter on the failing test file to fix Prettier
style errors: open app/test/e2e/specs/skills-registry.spec.ts (the
skills-registry.spec.ts test file) and apply the project's formatting command
(e.g., from the repo root run npx prettier --write
app/test/e2e/specs/skills-registry.spec.ts or your configured format script),
then re-run lint/CI to ensure Prettier --check passes and commit the formatted
file.
In `@app/test/e2e/specs/voice-mode.spec.ts`:
- Line 1: Prettier reported formatting issues in the test file (the one that
begins with the top-line directive "// `@ts-nocheck`"); run prettier --write on
that file to reformat it, review the resulting changes to ensure no unintended
code edits, then stage and commit the reformatted file so the pipeline passes
the prettier --check step.
In `@src/openhuman/agent/loop_/tool_loop.rs`:
- Around line 288-329: The code fails CI due to formatting around the
tokio::time::timeout invocation; run rustfmt (cargo fmt --all) and reflow the
tokio::time::timeout(...) call so it matches project formatting rules (ensure
the call that uses tool_deadline and tool.execute(call.arguments.clone()) is
wrapped consistently), then re-run cargo fmt to commit the formatted changes for
tool_deadline, timeout_secs, and the tokio::time::timeout usage.
---
Nitpick comments:
In `@app/src/chat/chatSendError.ts`:
- Around line 1-24: Fix the Prettier formatting for this file by running
Prettier (e.g., yarn prettier --write) on the file to satisfy the CI/style
rules, then tidy up exports/types if Prettier changes them; optionally refactor
the factory function chatSendError into an arrow function (retain the
ChatSendError and ChatSendErrorCode types and the same return shape) to match
the codebase preference for arrow functions.
In `@app/src/lib/skills/manager.ts`:
- Around line 114-121: resyncRunningSkillsAfterReconnect can be invoked
repeatedly during socket flapping and cause concurrent
activateSkill/listTools/syncToolsToBackend races; add a guard (e.g. a
module/class-level boolean like _resyncInProgress or a timestamp-based debounce)
inside resyncRunningSkillsAfterReconnect to short-circuit if a resync is already
running or recently completed, set the flag before starting, run the existing
ids.map activateSkill logic, and clear the flag in a finally block so the flag
is always reset on completion or error; update the socket connect caller
(socketService connect handler) to rely on this guard instead of attempting its
own dedupe.
In `@app/src/pages/Conversations.tsx`:
- Around line 1-6: Prettier style errors are present in Conversations.tsx; run
the formatter and update the file so CI passes by executing the project
formatter (e.g., run yarn prettier --write app/src/pages/Conversations.tsx or
your repo’s configured prettier script), then stage and commit the formatted
changes; ensure imports like convertFileSrc, React hooks (useEffect, useRef,
useState), Markdown, useNavigate, and the ChatSendError/chatSendError imports
remain unchanged except for formatting diffs.
In `@app/test/e2e/helpers/shared-flows.ts`:
- Around line 188-198: Extract the list of onboarding label strings used in
onboardingOverlayLikelyVisible into a shared constant (e.g.,
ONBOARDING_OVERLAY_TEXTS) and replace the inline string checks in
onboardingOverlayLikelyVisible with iterations over that constant (still calling
textExists for each entry); update any other places that reference the same
labels (such as the nearby comment/logic that mentions the same candidates) to
use the constant so label changes are centralized; keep the function signature
and behavior (Promise<boolean>) unchanged and ensure imports/exports are
adjusted if the constant is moved to a separate module.
In `@app/test/e2e/specs/skill-lifecycle.spec.ts`:
- Around line 35-39: Update the E2E test to assert backend/mock effects in
addition to the UI check: after the existing textExists assertions in the
skill-lifecycle.spec.ts test, query the request log with getRequestLog() and
assert a call to the skill registry (e.g., find an entry where r.method ===
'POST' && r.url.includes('skills') or the exact registry path) to ensure the
fetch occurred; additionally, on failure add diagnostic output such as
console.log('[LifecycleE2E] Request log:', getRequestLog()) and/or calling
dumpAccessibilityTree() to aid debugging.
- Around line 22-40: Wrap the existing assertions in the test (the hash check
and the content/textExists checks executed after navigateToSkills and
browser.pause) with a try/catch so that on any assertion failure you call
dumpAccessibilityTree() (and optionally other diagnostics like request logs)
before rethrowing the error; reference the existing helpers
triggerAuthDeepLinkBypass, waitForWindowVisible, waitForWebView,
waitForAppReady, completeOnboardingIfVisible, navigateToSkills, textExists and
ensure the catch invokes dumpAccessibilityTree() to surface accessibility state
for debugging, then rethrow the original error so the test still fails.
In `@app/test/e2e/specs/skill-multi-round.spec.ts`:
- Around line 33-37: The test currently checks presence via textExists(...) and
fails silently; update the spec so that if ok is false you gather failure
diagnostics before asserting: call dumpAccessibilityTree() and capture relevant
request/network logs (e.g., any existing request log helper or page network log)
and output them (console.error) along with the page HTML/content, then run
expect(ok).toBe(true). Reference textExists to detect absence and add diagnostic
calls to dumpAccessibilityTree() and the project’s network/request-log helper
(or page.content()) so failures include those diagnostics.
In `@src/openhuman/skills/qjs_engine.rs`:
- Around line 883-894: The helper call in skill_data_directory_stats currently
swallows IO errors via directory_byte_and_file_count(&path).unwrap_or((0, 0));
change this to handle the Result instead: match or if let Err(err) =
directory_byte_and_file_count(&path) and on error log a warning that includes
the skill path and err (e.g., via your crate logger) and then set total_bytes
and file_count to 0; keep the successful branch returning the computed values
and continue constructing SkillDataDirectoryStats with exists and
path.display().to_string().
In `@src/openhuman/skills/skill_registry.rs`:
- Around line 310-331: merge_published_state mutates published_state but doesn't
notify listeners — after successfully merging the patch in
merge_published_state, call the registry's broadcast_event with
events::SKILL_STATE_CHANGED and a payload containing the skill_id (e.g.
serde_json::json!({ "skill_id": skill_id })) so frontends receive the update;
locate merge_published_state and add the broadcast_event call immediately before
returning Ok(()) (use the existing broadcast_event API on the same struct).
🪄 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: f724024b-65e9-4d47-931f-16acc136de0a
📒 Files selected for processing (30)
.env.exampleapp/.env.exampleapp/src/chat/chatSendError.tsapp/src/lib/skills/hooks.tsapp/src/lib/skills/manager.tsapp/src/pages/Conversations.tsxapp/src/pages/Skills.tsxapp/src/services/socketService.tsapp/src/utils/tauriCommands.tsapp/src/utils/withTimeout.tsapp/test/e2e/helpers/shared-flows.tsapp/test/e2e/specs/auth-access-control.spec.tsapp/test/e2e/specs/skill-lifecycle.spec.tsapp/test/e2e/specs/skill-multi-round.spec.tsapp/test/e2e/specs/skill-oauth.spec.tsapp/test/e2e/specs/skill-socket-reconnect.spec.tsapp/test/e2e/specs/skills-registry.spec.tsapp/test/e2e/specs/voice-mode.spec.tssrc/openhuman/agent/harness/executor.rssrc/openhuman/agent/harness/types.rssrc/openhuman/agent/loop_/tool_loop.rssrc/openhuman/mod.rssrc/openhuman/skills/ping_scheduler.rssrc/openhuman/skills/qjs_engine.rssrc/openhuman/skills/qjs_skill_instance/event_loop.rssrc/openhuman/skills/schemas.rssrc/openhuman/skills/skill_registry.rssrc/openhuman/tool_timeout.rssrc/openhuman/tools/delegate.rstests/json_rpc_e2e.rs
…odule, CI diagnostics - Centralize VITE_TOOL_TIMEOUT_SECS in config.ts (TOOL_TIMEOUT_SECS) - Move tool_timeout to folder module; merge_published_state broadcasts SKILL_STATE_CHANGED - Resync guard after socket reconnect; qjs_engine logs data dir stat errors - E2E: registry/lifecycle/multi-round failure diagnostics; onboarding label constant - chatSendError arrow export; rustfmt/import grouping in event_loop Made-with: Cursor
fix: PR tinyhumansai#282 review — tool timeout config, tool_timeout module, CI di…
- Introduced a comprehensive end-to-end test suite for the Gmail skill, covering the full lifecycle from discovery to OAuth completion and email management. - Implemented two modes: a lifecycle-only mode that validates the skill's event loop without real credentials, and a live mode that interacts with the Gmail API using actual credentials. - Added detailed environment variable requirements and usage instructions for both testing modes. Closes #XXX (replace with relevant issue number if applicable)
senamakel
approved these changes
Apr 2, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/openhuman/skills/qjs_engine.rs (1)
917-945: Consider usingfile_type().is_symlink()to skip symlinks and prevent potential infinite recursion.The
is_dir()andis_file()methods follow symlinks. While no symlinks currently exist in skill data directories, a symlink loop (e.g., pointing to a parent directory) would cause infinite recursion and stack overflow. This is a defensive improvement to prevent traversal outside the skill's data directory.♻️ Proposed fix to skip symlinks
fn walk( path: &std::path::Path, total_bytes: &mut u64, file_count: &mut u64, ) -> std::io::Result<()> { let read = fs::read_dir(path)?; for entry in read { let entry = entry?; let meta = entry.metadata()?; + if meta.file_type().is_symlink() { + continue; + } let p = entry.path(); if meta.is_dir() { walk(&p, total_bytes, file_count)?; } else if meta.is_file() { *total_bytes += meta.len(); *file_count += 1; } } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/skills/qjs_engine.rs` around lines 917 - 945, The directory_byte_and_file_count function's recursive helper walk currently treats symlinks as files/dirs via metadata().is_dir()/is_file(), risking infinite recursion; change the traversal to check the entry's file_type().is_symlink() (via entry.file_type() or metadata.file_type()) and skip any symlink entries before recursing or counting, so walk(...) only recurses into real directories and only counts real files; update both the directory branch (walk(&p, ...)) and the file counting branch to ignore symlinks.app/test/e2e/helpers/shared-flows.ts (1)
199-204: Prefer arrow functions for these new onboarding helpers.
onboardingOverlayLikelyVisibleandcompleteOnboardingIfVisibleare newly added function declarations. The repo convention for JS/TS helpers here isconst ... = async () => {}.As per coding guidelines,
**/*.{js,jsx,ts,tsx}: Use const by default, let if reassignment is needed, avoid var. Prefer arrow functions over function declarations.Also applies to: 258-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/helpers/shared-flows.ts` around lines 199 - 204, The two new helper functions onboardingOverlayLikelyVisible and completeOnboardingIfVisible are declared with function declarations but the codebase prefers const arrow functions; change each to a const assignment using an async arrow (e.g., const onboardingOverlayLikelyVisible = async () => { ... }) and ensure any references remain unchanged, keeping function names the same and preserving return types/behavior.src/openhuman/tool_timeout/mod.rs (1)
1-29: Keepmod.rslight and export-focused.This new domain module still puts all of the timeout parsing and caching logic in
mod.rs. Please move the implementation into a sibling file and re-exporttool_execution_timeout_secs/tool_execution_timeout_durationfrom here so the domain module stays layout-only.As per coding guidelines,
src/openhuman/*/mod.rs: “Keep domainmod.rsfiles light and export-focused; put operational code in sibling files (e.g.ops.rs,store.rs,schedule.rs,types.rs), then re-export the public API frommod.rs”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/tool_timeout/mod.rs` around lines 1 - 29, Move the implementation details (the static OnceLock, constants DEFAULT_SECS/MAX_SECS, function resolved_secs, and Duration conversion logic) out of mod.rs into a sibling file (e.g., timeout.rs or ops.rs), leaving mod.rs only to re-export the public API; implement resolved_secs, tool_execution_timeout_secs, and tool_execution_timeout_duration in the new file and then in mod.rs simply pub use the two public functions (tool_execution_timeout_secs and tool_execution_timeout_duration) so mod.rs remains export-focused while the operational code lives in the sibling module.
🤖 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/lib/skills/manager.ts`:
- Around line 119-125: resyncRunningSkillsAfterReconnect currently re-activates
every id from this.runtimes (calling activateSkill) even for runtimes added by
startSkill before setup/OAuth completes; change it to only re-activate skills
that are already setup/ready (or track an explicit activatedIds set) so
incomplete setups don't trigger listTools()/syncToolsToBackend prematurely.
Specifically, in resyncRunningSkillsAfterReconnect, replace the blind ids =
[...this.runtimes.keys()] with a filtered list based on a readiness indicator on
the runtime (e.g., runtime.setupComplete or runtime.isReady or runtime.status
=== 'ready') or use a maintained activatedIds collection updated by
startSkill/activateSkill, then call activateSkill only for those ready/activated
ids.
- Around line 264-275: The current catch around the
withTimeout(callCoreRpc(...)) in the SkillManager sync path swallows all errors
(including timeouts and real RPC errors); update the catch to only suppress the
specific "skill not running" error and rethrow everything else. Inside the
try/catch around withTimeout(callCoreRpc({ method: "openhuman.skills_sync",
params: { skill_id: skillId } }, ...)), inspect the caught error (e.g.,
error?.message or error?.code) and if it matches the known "skill not running"
indicator preserve the silent skip, otherwise throw the error again so timeouts
and other RPC failures propagate; keep references to withTimeout and callCoreRpc
as the loci of change.
In `@app/src/utils/config.ts`:
- Around line 8-16: parseToolTimeoutSecs currently accepts floats and rounds
them; change it to a const arrow function that only accepts positive integers
matching Rust's parse::<u64>() behavior: treat
import.meta.env.VITE_TOOL_TIMEOUT_SECS as undefined/empty or any non-integer
string as invalid and return DEFAULT_TOOL_TIMEOUT_SECS, otherwise parse the
string as an integer and only accept values in the range
1..=MAX_TOOL_TIMEOUT_SECS (return DEFAULT_TOOL_TIMEOUT_SECS on out-of-range);
update the function signature to "const parseToolTimeoutSecs = (): number => {
... }" and use an integer-only check (e.g., /^\d+$/ or Number.isInteger after
numeric conversion) instead of Number(raw) + Math.round so fractional inputs
like "0.4" are rejected.
In `@app/test/e2e/helpers/shared-flows.ts`:
- Around line 258-261: completeOnboardingIfVisible currently skips the short
polling window walkOnboarding relied on by checking visibility once; restore
that initial wait by invoking walkOnboarding(logPrefix) (instead of only when
onboardingOverlayLikelyVisible() is true) or by inserting a short polling/wait
before the visibility check so the onboarding overlay has time to render; update
the function referencing completeOnboardingIfVisible,
onboardingOverlayLikelyVisible, and walkOnboarding to preserve the original
short polling behavior.
---
Nitpick comments:
In `@app/test/e2e/helpers/shared-flows.ts`:
- Around line 199-204: The two new helper functions
onboardingOverlayLikelyVisible and completeOnboardingIfVisible are declared with
function declarations but the codebase prefers const arrow functions; change
each to a const assignment using an async arrow (e.g., const
onboardingOverlayLikelyVisible = async () => { ... }) and ensure any references
remain unchanged, keeping function names the same and preserving return
types/behavior.
In `@src/openhuman/skills/qjs_engine.rs`:
- Around line 917-945: The directory_byte_and_file_count function's recursive
helper walk currently treats symlinks as files/dirs via
metadata().is_dir()/is_file(), risking infinite recursion; change the traversal
to check the entry's file_type().is_symlink() (via entry.file_type() or
metadata.file_type()) and skip any symlink entries before recursing or counting,
so walk(...) only recurses into real directories and only counts real files;
update both the directory branch (walk(&p, ...)) and the file counting branch to
ignore symlinks.
In `@src/openhuman/tool_timeout/mod.rs`:
- Around line 1-29: Move the implementation details (the static OnceLock,
constants DEFAULT_SECS/MAX_SECS, function resolved_secs, and Duration conversion
logic) out of mod.rs into a sibling file (e.g., timeout.rs or ops.rs), leaving
mod.rs only to re-export the public API; implement resolved_secs,
tool_execution_timeout_secs, and tool_execution_timeout_duration in the new file
and then in mod.rs simply pub use the two public functions
(tool_execution_timeout_secs and tool_execution_timeout_duration) so mod.rs
remains export-focused while the operational code lives in the sibling module.
🪄 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: 4100726c-0bb4-41ab-935c-31256121559c
📒 Files selected for processing (18)
app/src/chat/chatSendError.tsapp/src/lib/skills/manager.tsapp/src/pages/Conversations.tsxapp/src/utils/config.tsapp/src/utils/withTimeout.tsapp/test/e2e/helpers/shared-flows.tsapp/test/e2e/specs/skill-lifecycle.spec.tsapp/test/e2e/specs/skill-multi-round.spec.tsapp/test/e2e/specs/skill-socket-reconnect.spec.tsapp/test/e2e/specs/skills-registry.spec.tsapp/test/e2e/specs/voice-mode.spec.tssrc/openhuman/agent/loop_/tool_loop.rssrc/openhuman/skills/ping_scheduler.rssrc/openhuman/skills/qjs_engine.rssrc/openhuman/skills/qjs_skill_instance/event_loop.rssrc/openhuman/skills/skill_registry.rssrc/openhuman/tool_timeout/mod.rssrc/openhuman/tools/delegate.rs
✅ Files skipped from review due to trivial changes (4)
- src/openhuman/tools/delegate.rs
- app/test/e2e/specs/skill-multi-round.spec.ts
- app/test/e2e/specs/skill-lifecycle.spec.ts
- app/src/chat/chatSendError.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/openhuman/skills/qjs_skill_instance/event_loop.rs
- app/test/e2e/specs/skill-socket-reconnect.spec.ts
- app/src/utils/withTimeout.ts
- app/test/e2e/specs/skills-registry.spec.ts
- src/openhuman/agent/loop_/tool_loop.rs
- src/openhuman/skills/skill_registry.rs
- app/src/pages/Conversations.tsx
- src/openhuman/skills/ping_scheduler.rs
Comment on lines
+119
to
+125
| 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 { |
Contributor
There was a problem hiding this comment.
Don't re-activate setup-incomplete skills on reconnect.
startSkill() adds runtimes to this.runtimes before setup/OAuth completion, but this path calls activateSkill() for every key. Since app/src/services/socketService.ts:163-173 invokes this on every connect, setup-incomplete skills can get listTools() / syncToolsToBackend() prematurely. Please gate this on already-ready skills, or track activated IDs separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/skills/manager.ts` around lines 119 - 125,
resyncRunningSkillsAfterReconnect currently re-activates every id from
this.runtimes (calling activateSkill) even for runtimes added by startSkill
before setup/OAuth completes; change it to only re-activate skills that are
already setup/ready (or track an explicit activatedIds set) so incomplete setups
don't trigger listTools()/syncToolsToBackend prematurely. Specifically, in
resyncRunningSkillsAfterReconnect, replace the blind ids =
[...this.runtimes.keys()] with a filtered list based on a readiness indicator on
the runtime (e.g., runtime.setupComplete or runtime.isReady or runtime.status
=== 'ready') or use a maintained activatedIds collection updated by
startSkill/activateSkill, then call activateSkill only for those ready/activated
ids.
Comment on lines
264
to
275
| 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 | ||
| } |
Contributor
There was a problem hiding this comment.
Don't swallow withTimeout(...) failures in the RPC fallback.
This blanket catch now hides the new timeout rejection too, so triggerSync() looks successful whenever there is no local runtime—even if openhuman.skills_sync timed out or the core returned a real error. Only ignore the specific “skill not running” case here and rethrow the rest.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/lib/skills/manager.ts` around lines 264 - 275, The current catch
around the withTimeout(callCoreRpc(...)) in the SkillManager sync path swallows
all errors (including timeouts and real RPC errors); update the catch to only
suppress the specific "skill not running" error and rethrow everything else.
Inside the try/catch around withTimeout(callCoreRpc({ method:
"openhuman.skills_sync", params: { skill_id: skillId } }, ...)), inspect the
caught error (e.g., error?.message or error?.code) and if it matches the known
"skill not running" indicator preserve the silent skip, otherwise throw the
error again so timeouts and other RPC failures propagate; keep references to
withTimeout and callCoreRpc as the loci of change.
Comment on lines
+8
to
+16
| function parseToolTimeoutSecs(): number { | ||
| const raw = import.meta.env.VITE_TOOL_TIMEOUT_SECS as string | undefined; | ||
| if (raw === undefined || raw === '') return DEFAULT_TOOL_TIMEOUT_SECS; | ||
| const n = Number(raw); | ||
| if (!Number.isFinite(n) || n <= 0 || n > MAX_TOOL_TIMEOUT_SECS) { | ||
| return DEFAULT_TOOL_TIMEOUT_SECS; | ||
| } | ||
| return Math.round(n); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the Rust parser mentioned
fd -t f "mod.rs" | head -20Repository: tinyhumansai/openhuman
Length of output: 709
🏁 Script executed:
# Look at the current config.ts file
fd "config.ts" app/src/utils/Repository: tinyhumansai/openhuman
Length of output: 89
🏁 Script executed:
# Search for TOOL_TIMEOUT references in the codebase
rg "TOOL_TIMEOUT" --type ts --type tsxRepository: tinyhumansai/openhuman
Length of output: 93
🏁 Script executed:
# Search for Rust code that parses timeout
rg "OPENHUMAN_TOOL_TIMEOUT" -t rustRepository: tinyhumansai/openhuman
Length of output: 282
🏁 Script executed:
# Get the Rust tool_timeout parser
cat -n src/openhuman/tool_timeout/mod.rsRepository: tinyhumansai/openhuman
Length of output: 1103
🏁 Script executed:
# Get the TS config file
cat -n app/src/utils/config.tsRepository: tinyhumansai/openhuman
Length of output: 1927
🏁 Script executed:
# Search TOOL_TIMEOUT in ts/js files properly
rg "TOOL_TIMEOUT" --type ts
rg "TOOL_TIMEOUT" --type jsRepository: tinyhumansai/openhuman
Length of output: 966
Make timeout parsing match the Rust parser: only accept positive integers.
The backend parses OPENHUMAN_TOOL_TIMEOUT_SECS with Rust's parse::<u64>(), which rejects fractional values and only accepts integers in the range 1–3600. The frontend currently accepts floats via Number(raw) and rounds them after validation. This creates a mismatch: 0.4 passes frontend validation and rounds to 0, causing an immediate timeout, while the backend would reject it and fall back to 120 seconds.
Additionally, convert this helper to a const arrow function to match the TypeScript style guideline.
Suggested fix
-function parseToolTimeoutSecs(): number {
+const parseToolTimeoutSecs = (): number => {
const raw = import.meta.env.VITE_TOOL_TIMEOUT_SECS as string | undefined;
if (raw === undefined || raw === '') return DEFAULT_TOOL_TIMEOUT_SECS;
- const n = Number(raw);
- if (!Number.isFinite(n) || n <= 0 || n > MAX_TOOL_TIMEOUT_SECS) {
+ if (!/^\d+$/.test(raw)) {
return DEFAULT_TOOL_TIMEOUT_SECS;
}
- return Math.round(n);
+ const n = Number(raw);
+ if (n < 1 || n > MAX_TOOL_TIMEOUT_SECS) return DEFAULT_TOOL_TIMEOUT_SECS;
+ return n;
+};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/utils/config.ts` around lines 8 - 16, parseToolTimeoutSecs currently
accepts floats and rounds them; change it to a const arrow function that only
accepts positive integers matching Rust's parse::<u64>() behavior: treat
import.meta.env.VITE_TOOL_TIMEOUT_SECS as undefined/empty or any non-integer
string as invalid and return DEFAULT_TOOL_TIMEOUT_SECS, otherwise parse the
string as an integer and only accept values in the range
1..=MAX_TOOL_TIMEOUT_SECS (return DEFAULT_TOOL_TIMEOUT_SECS on out-of-range);
update the function signature to "const parseToolTimeoutSecs = (): number => {
... }" and use an integer-only check (e.g., /^\d+$/ or Number.isInteger after
numeric conversion) instead of Number(raw) + Math.round so fractional inputs
like "0.4" are rejected.
Comment on lines
+258
to
261
| export async function completeOnboardingIfVisible(logPrefix = '[E2E]') { | ||
| if (await onboardingOverlayLikelyVisible()) { | ||
| await walkOnboarding(logPrefix); | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve the initial wait in completeOnboardingIfVisible.
This helper now does a single visibility check before delegating. That drops the short polling window walkOnboarding() already relied on, so a slightly late overlay render gets skipped and the spec keeps waiting on Home behind the still-open onboarding screen. Because multiple smoke specs now use this helper, this becomes shared E2E flakiness.
🛠️ Suggested fix
export async function completeOnboardingIfVisible(logPrefix = '[E2E]') {
- if (await onboardingOverlayLikelyVisible()) {
- await walkOnboarding(logPrefix);
+ for (let attempt = 0; attempt < 8; attempt++) {
+ if (await onboardingOverlayLikelyVisible()) {
+ await walkOnboarding(logPrefix);
+ return;
+ }
+ await browser.pause(400);
}
}📝 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
| export async function completeOnboardingIfVisible(logPrefix = '[E2E]') { | |
| if (await onboardingOverlayLikelyVisible()) { | |
| await walkOnboarding(logPrefix); | |
| } | |
| export async function completeOnboardingIfVisible(logPrefix = '[E2E]') { | |
| for (let attempt = 0; attempt < 8; attempt++) { | |
| if (await onboardingOverlayLikelyVisible()) { | |
| await walkOnboarding(logPrefix); | |
| return; | |
| } | |
| await browser.pause(400); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/test/e2e/helpers/shared-flows.ts` around lines 258 - 261,
completeOnboardingIfVisible currently skips the short polling window
walkOnboarding relied on by checking visibility once; restore that initial wait
by invoking walkOnboarding(logPrefix) (instead of only when
onboardingOverlayLikelyVisible() is true) or by inserting a short polling/wait
before the visibility check so the onboarding overlay has time to render; update
the function referencing completeOnboardingIfVisible,
onboardingOverlayLikelyVisible, and walkOnboarding to preserve the original
short polling behavior.
This was referenced Apr 2, 2026
This was referenced Apr 11, 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
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
Implements #191: skill sync/timeout fixes, expanded E2E coverage, and related sub-issues.
What changed
Core (Rust)
openhuman.skills_data_stats— recursive disk usage for skill data dirs.OPENHUMAN_TOOL_TIMEOUT_SECS— shared tool timeouts (skill loop, agent loop, harness, delegate).published_stateviaSkillRegistry::merge_published_state.skills_data_stats.App
useSkillDataDirectoryStats; merge with skill state for counts / last sync.resyncRunningSkillsAfterReconnectaftertool:sync.finallywhen revoke did not succeed.callTool/triggerSync— bounded waits (VITE_TOOL_TIMEOUT_SECS).ChatSendErrorwithdata-chat-send-error-code(including safety timeout).E2E
completeOnboardingIfVisible.Issues closed
Closes #191
Sub-issues
callTool/triggerSyncRelated
Verification
cargo check,cargo test(includingjson_rpc_skills_runtime_start_tools_call_stop)yarn typecheck,yarn lintSummary by CodeRabbit
New Features
Improvements
Tests