Skip to content

test(e2e): default file keyring + RPC diagnostics to unblock Linux Appium (re-delivers #2609)#2645

Merged
sanil-23 merged 4 commits into
tinyhumansai:mainfrom
sanil-23:fix/e2e-keyring-rpc-diagnostics
May 25, 2026
Merged

test(e2e): default file keyring + RPC diagnostics to unblock Linux Appium (re-delivers #2609)#2645
sanil-23 merged 4 commits into
tinyhumansai:mainfrom
sanil-23:fix/e2e-keyring-rpc-diagnostics

Conversation

@sanil-23
Copy link
Copy Markdown
Contributor

@sanil-23 sanil-23 commented May 25, 2026

Summary

  • Re-delivery of test(e2e): stabilize Linux Appium auth diagnostics #2609 (by @YOMXXX, 李冠辰 <liguanchen@xiaomi.com>) rebased onto current main with the sole merge conflict resolved. All three original commits are preserved with their original authorship.
  • Default the E2E session runner to OPENHUMAN_KEYRING_BACKEND=file so headless Linux Appium runners stop losing the backend session token after login (still respects an explicit caller override).
  • Add expectRpcOk / formatRpcCallFailure E2E RPC helpers so a failed positive RPC assertion prints method + HTTP status + error text instead of an opaque Expected: true / Received: false.
  • Convert the mega-flow Composio trigger assertions to expectRpcOk, plus Vitest coverage for the failure formatter and runner-temp app-log globs on the E2E failure artifact upload.

Problem

The e2e / E2E (Linux / Appium Chromium) job is red on main itself and on every open PR. Both Composio scenarios in mega-flow.spec.ts fail at the first RPC call:

composio_list_triggers FAILED: {"ok":false,"error":"composio unavailable: no backend session token. Sign in first (auth_store_session)."}

The deep-link login does call auth_store_session, but the headless CI runner selects keyring backend os (Secret Service), which has no permission there — so the stored backend session token can't be read back and every Composio RPC reports it missing.

#2609 already diagnosed and fixed this and is fully green (incl. the Appium job), but it sits CONFLICTING/DIRTY against current main and lives on a fork branch we can't push to. This PR carries the identical fix in a mergeable state so the e2e gate is unblocked repo-wide.

Solution

  • app/scripts/e2e-run-session.sh: : "${OPENHUMAN_KEYRING_BACKEND:=file}" + export, so auth secrets live under the disposable OPENHUMAN_WORKSPACE instead of the host OS keychain.
  • app/test/e2e/helpers/core-rpc*.ts: formatRpcCallFailure + expectRpcOk.
  • app/test/e2e/specs/mega-flow.spec.ts: positive Composio assertions use expectRpcOk.
  • app/test/core-rpc-node.test.ts: Vitest coverage for the formatter.
  • .github/workflows/e2e-reusable.yml: also upload ${RUNNER_TEMP}/openhuman-e2e-app-*.log.

Conflict resolution: the only conflict was in mega-flow.spec.ts, where main had added an interim console.log(...FAILED...) + bare expect(ok).toBe(true) stop-gap to surface the opaque error — exactly what #2609 replaces with expectRpcOk. Took #2609's side for both hunks (the only consistent choice, since listTriggersMethod/enableTriggerMethod are referenced downstream); all of main's other changes to the file are preserved.

Submission Checklist

If a section does not apply to this change, mark the item as N/A with a one-line reason. Do not delete items.

  • Tests added or updated — core-rpc-node.test.ts Vitest coverage for the RPC failure formatter (happy + failure path); mega-flow positive paths now assert via expectRpcOk.
  • Diff coverage ≥ 80% — N/A as a local run: deferred to CI (changed lines are E2E harness/test + a one-line shell default). Identical diff to test(e2e): stabilize Linux Appium auth diagnostics #2609 which passed the merged coverage gate; this PR's CI re-reports it.
  • Coverage matrix updated — N/A: E2E-harness + CI-env change, no feature rows added/removed/renamed.
  • All affected feature IDs listed under ## Related — N/A: no TEST-COVERAGE-MATRIX feature IDs touched.
  • No new external network dependencies introduced — none; the E2E mock backend is used.
  • Manual smoke checklist updated — N/A: does not touch release-cut surfaces (test harness + CI env only).
  • Linked issue closed via Closes #NNN — N/A: no tracking issue; this re-delivers PR test(e2e): stabilize Linux Appium auth diagnostics #2609 (referenced below).

Impact

  • CI / E2E only. No runtime, app, or shipped-binary behavior changes — OPENHUMAN_KEYRING_BACKEND only defaults to file inside e2e-run-session.sh, and only when unset.
  • Unblocks the Linux Appium e2e gate for main and all open PRs.

Related


AI Authored PR Metadata (required for Codex/Linear PRs)

AI-assisted re-delivery (conflict resolution + rebase). Original implementation by @YOMXXX in #2609.

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: sanil-23:fix/e2e-keyring-rpc-diagnostics
  • Commit SHA: 9c8931a6fd5bbefd0182cdc8d7d6293815f9e45e

Validation Run

  • pnpm --filter openhuman-app format:check — via CI; diff is identical to the already-green test(e2e): stabilize Linux Appium auth diagnostics #2609 (deferred locally: deps not installed in the cherry-pick worktree).
  • pnpm typecheck — via CI; same as above.
  • Focused tests: app/test/core-rpc-node.test.ts (Vitest) + the mega-flow Composio scenarios on the Linux Appium job — green on test(e2e): stabilize Linux Appium auth diagnostics #2609; re-run here by CI.
  • Rust fmt/check (if changed): N/A — no Rust files changed.
  • Tauri fmt/check (if changed): N/A — no Tauri files changed.

Validation Blocked

  • command: pnpm typecheck / format:check not run in the cherry-pick worktree
  • error: node_modules not installed in the throwaway worktree
  • impact: none — the 6-file diff is identical to test(e2e): stabilize Linux Appium auth diagnostics #2609, which passed the full CI matrix (typecheck, format, coverage, Linux Appium e2e) green; this PR's CI re-validates.

Behavior Changes

  • Intended behavior change: E2E session runner defaults to a file-backed keyring; richer RPC failure diagnostics in E2E.
  • User-visible effect: none in the shipped app — CI/test-harness only.

Parity Contract

  • Legacy behavior preserved: an explicit OPENHUMAN_KEYRING_BACKEND from the caller still wins; non-E2E code paths unchanged.
  • Guard/fallback/dispatch parity checks: default applied only when the var is unset.

Summary by CodeRabbit

  • Tests

    • Added stronger validation and compact diagnostics for Core JSON-RPC responses; updated end-to-end specs to use the new checks for clearer failures.
  • Chores

    • Improved CI artifact collection for better debugging of test failures.
    • Adjusted CI environment handling to support headless Linux runners more reliably.

Review Change Stack

@sanil-23 sanil-23 requested a review from a team May 25, 2026 15:18
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 25, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f95465a7-37f8-45a7-979e-40c7328a8ca4

📥 Commits

Reviewing files that changed from the base of the PR and between 9c8931a and 7e1bc6a.

📒 Files selected for processing (1)
  • app/test/e2e/helpers/core-rpc-node.ts

📝 Walkthrough

Walkthrough

This PR standardizes JSON-RPC failure reporting and assertions for E2E tests (adds formatting and expect helpers), updates the reusable E2E workflow to upload runner-temp app logs on failures across Linux/macOS (and clarifies Windows temp comment), sets a default keyring backend for headless CI, and refactors the mega-flow spec to use the new RPC helpers.

Changes

E2E Testing Infrastructure

Layer / File(s) Summary
CI artifact collection for all platforms
.github/workflows/e2e-reusable.yml
Linux smoke, Linux full-suite, and macOS full-suite failure-artifact uploads now include ${{ runner.temp }}/openhuman-e2e-app-*.log; Windows failure-artifact comment clarified for RUNNER_TEMP semantics.
E2E runner keyring backend configuration
app/scripts/e2e-run-session.sh
E2E session script exports OPENHUMAN_KEYRING_BACKEND defaulting to file and logs the selected backend.
RPC failure formatting and assertion helpers
app/test/e2e/helpers/core-rpc-node.ts, app/test/e2e/helpers/core-rpc.ts, app/test/core-rpc-node.test.ts
Adds truncation and safe-JSON utilities, formatRpcCallFailure and expectRpcOk exports, a unit test for formatting, and re-exports to the public core-rpc helper surface.
Mega-flow spec refactored to use RPC helpers
app/test/e2e/specs/mega-flow.spec.ts
Refactors Composio RPC calls to use local method-name constants and expectRpcOk for list/enable operations, replacing manual .ok checks and inline failure logging.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰
Logs gathered from the runner's nest,
RPC failures trimmed and dressed,
Keyring set for headless nights,
Tests now clearer in their sights,
I hop, I cheer—CI sleeps tight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: defaulting file keyring backend and adding RPC diagnostics for E2E testing to unblock Linux Appium, matching the core objectives.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot added the working A PR that is being worked on by the team. label May 25, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/test/e2e/helpers/core-rpc-node.ts`:
- Around line 56-63: The type predicate on expectRpcOk is too strong: it asserts
result.result is present even though RpcCallResult<T> declares result?: T and
the function only checks result.ok. Update the assertion signature in
expectRpcOk to reflect that result.result may be undefined (e.g. change the
asserted type to RpcCallResult<T> & { ok: true; result?: T } or RpcCallResult<T>
& { ok: true; result: T | undefined }) so the TS narrowing matches the actual
runtime check; alternatively, if you want to guarantee a non-undefined result,
add an explicit runtime check for result.result !== undefined and throw if
absent.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 571619a6-356d-472f-b514-22cc7726c93a

📥 Commits

Reviewing files that changed from the base of the PR and between cdee8f7 and 9c8931a.

📒 Files selected for processing (6)
  • .github/workflows/e2e-reusable.yml
  • app/scripts/e2e-run-session.sh
  • app/test/core-rpc-node.test.ts
  • app/test/e2e/helpers/core-rpc-node.ts
  • app/test/e2e/helpers/core-rpc.ts
  • app/test/e2e/specs/mega-flow.spec.ts

Comment thread app/test/e2e/helpers/core-rpc-node.ts
CodeRabbit (tinyhumansai#2645): the assertion narrowed result.result to non-undefined
(`& { ok: true; result: T }`), but RpcCallResult<T> declares `result?: T`
and expectRpcOk only checks `result.ok`. Drop the unsound `result: T` so the
TS narrowing matches what the runtime actually guarantees; call sites already
read `result?.result?` via optional chaining.

Co-Authored-By: Claude <noreply@anthropic.com>
@sanil-23 sanil-23 self-assigned this May 25, 2026
@sanil-23 sanil-23 merged commit f756893 into tinyhumansai:main May 25, 2026
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

working A PR that is being worked on by the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants