test(e2e): default file keyring + RPC diagnostics to unblock Linux Appium (re-delivers #2609)#2645
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis 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. ChangesE2E Testing Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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. 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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (6)
.github/workflows/e2e-reusable.ymlapp/scripts/e2e-run-session.shapp/test/core-rpc-node.test.tsapp/test/e2e/helpers/core-rpc-node.tsapp/test/e2e/helpers/core-rpc.tsapp/test/e2e/specs/mega-flow.spec.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>
Summary
<liguanchen@xiaomi.com>) rebased onto currentmainwith the sole merge conflict resolved. All three original commits are preserved with their original authorship.OPENHUMAN_KEYRING_BACKEND=fileso headless Linux Appium runners stop losing the backend session token after login (still respects an explicit caller override).expectRpcOk/formatRpcCallFailureE2E RPC helpers so a failed positive RPC assertion prints method + HTTP status + error text instead of an opaqueExpected: true / Received: false.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 onmainitself and on every open PR. Both Composio scenarios inmega-flow.spec.tsfail at the first RPC call:The deep-link login does call
auth_store_session, but the headless CI runner selects keyring backendos(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/DIRTYagainst currentmainand 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 disposableOPENHUMAN_WORKSPACEinstead 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 useexpectRpcOk.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, wheremainhad added an interimconsole.log(...FAILED...)+ bareexpect(ok).toBe(true)stop-gap to surface the opaque error — exactly what #2609 replaces withexpectRpcOk. Took #2609's side for both hunks (the only consistent choice, sincelistTriggersMethod/enableTriggerMethodare referenced downstream); all ofmain's other changes to the file are preserved.Submission Checklist
core-rpc-node.test.tsVitest coverage for the RPC failure formatter (happy + failure path); mega-flow positive paths now assert viaexpectRpcOk.## Related— N/A: noTEST-COVERAGE-MATRIXfeature IDs touched.Closes #NNN— N/A: no tracking issue; this re-delivers PR test(e2e): stabilize Linux Appium auth diagnostics #2609 (referenced below).Impact
OPENHUMAN_KEYRING_BACKENDonly defaults tofileinsidee2e-run-session.sh, and only when unset.mainand all open PRs.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
sanil-23:fix/e2e-keyring-rpc-diagnostics9c8931a6fd5bbefd0182cdc8d7d6293815f9e45eValidation 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.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.Validation Blocked
command:pnpm typecheck/format:checknot run in the cherry-pick worktreeerror:node_modules not installed in the throwaway worktreeimpact: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
Parity Contract
OPENHUMAN_KEYRING_BACKENDfrom the caller still wins; non-E2E code paths unchanged.Summary by CodeRabbit
Tests
Chores