chore: migrate from yarn to pnpm#886
Merged
senamakel merged 4 commits intotinyhumansai:mainfrom Apr 24, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRepository-wide migration from Yarn/npx to pnpm (pnpm pinned) across CI, scripts, docs, and package manifests; Tauri build hook and Debian packaging adjustments; multiple E2E and Rust test additions/refactors; small app code tweaks (UUIDs, logging severity, route removal, memoization). Changes
Sequence Diagram(s)sequenceDiagram
participant E2E as Browser/E2E test
participant UI as Electron/Tauri UI
participant Tauri as Tauri core
participant Sidecar as openhuman-core (sidecar)
participant Mock as Mock backend
E2E->>UI: launch app / query core_rpc_url
UI->>Tauri: invoke core_rpc_url command
Tauri->>Sidecar: request /rpc ping/version/health
Sidecar->>Tauri: respond (HTTP 200 / JSON-RPC)
Tauri->>UI: return RPC url / method results
E2E->>UI: trigger UI action -> Tauri invoke
Tauri->>Sidecar: forward JSON-RPC call
Sidecar->>Mock: (sidecar may talk to Mock) / internal handling
Sidecar-->>Tauri: JSON-RPC response
Tauri-->>E2E: result asserted by test
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/scripts/e2e-run-spec.sh (1)
108-118:⚠️ Potential issue | 🟡 MinorUpdate error messages to reference
pnpminstead ofyarn.Lines 111 and 116 instruct users to run
yarn test:e2e:build, but the PR migrates topnpm(line 184 confirms withpnpm exec wdio). Sincetest:e2e:buildis defined in theappworkspace and invoked via pnpm, these error messages should guide users consistently.✏️ Proposed fix
echo "ERROR: No frontend bundle found at dist/assets/index-*.js." >&2 - echo " Run 'yarn test:e2e:build' to build the app before running E2E tests." >&2 + echo " Run 'pnpm test:e2e:build' to build the app before running E2E tests." >&2 exit 1 fi if ! grep -q "127.0.0.1:${E2E_MOCK_PORT}" "$DIST_JS"; then echo "ERROR: frontend bundle does NOT contain mock server URL (127.0.0.1:${E2E_MOCK_PORT})." >&2 - echo " Run 'yarn test:e2e:build' to rebuild with the mock URL." >&2 + echo " Run 'pnpm test:e2e:build' to rebuild with the mock URL." >&2 exit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/scripts/e2e-run-spec.sh` around lines 108 - 118, Update the user-facing error messages in e2e-run-spec.sh to reference pnpm instead of yarn: change the two echo lines that currently tell users to run 'yarn test:e2e:build' (found near the DIST_JS existence check and the mock URL grep check using the DIST_JS and E2E_MOCK_PORT variables) so they instruct running 'pnpm --filter app test:e2e:build' or simply 'pnpm test:e2e:build' consistent with the repo's pnpm usage; ensure both messages are updated so they consistently mention pnpm and preserve the surrounding wording and exit behavior..husky/pre-push (1)
40-43:⚠️ Potential issue | 🟠 MajorLeftover
yarninvocation breaks the pnpm-only environment.After this PR, contributors likely won't have
yarninstalled, yet thelint:commands-tokenscheck on line 41 still shells out toyarn --cwd app. This step will fail with "command not found" on fresh clones and silently bypass the cmd-tokens guard (theCMD_TOKENS_EXITcapture will record the shell-level error, but the actual lint check won't run). Switch to pnpm to match the rest of the hook.🔧 Proposed fix
# Enforce scoped cmd-* tokens in components/commands/ set +e -yarn --cwd app lint:commands-tokens +pnpm --filter openhuman-app lint:commands-tokens CMD_TOKENS_EXIT=$? set -e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.husky/pre-push around lines 40 - 43, Replace the leftover Yarn invocation with pnpm to avoid breaking pnpm-only environments: change the command that runs the cmd-tokens lint (currently invoked as "yarn --cwd app lint:commands-tokens") to use pnpm (for example "pnpm --dir app run lint:commands-tokens"), keeping the surrounding set +e / set -e and the CMD_TOKENS_EXIT capture so the hook still records the linter's exit code..github/workflows/release.yml (1)
542-552:⚠️ Potential issue | 🟡 MinorStale comments reference a "pnpm build" step that no longer exists.
Both comment blocks here talk about "The earlier explicit
pnpm buildstep" and "must match the firstpnpm build", but the explanatory note at lines 453–455 (and the actual step list) confirms there is no separate frontend build step —vite buildruns only viatauri.conf.json'sbeforeBuildCommandinsidecargo tauri build. The comments appear to be carried over from an older version where a dedicated build step existed and will mislead future readers debugging Sentry / source-map issues.🔧 Suggested rewording
# Re-declare Vite env so tauri.conf.json's `beforeBuildCommand` # (`vite build`) bakes Sentry + debug flags into the final bundle. - # The earlier explicit `pnpm build` step ran with these, but the - # rebuild triggered here would strip them and ship a DSN-less UI. + # Without these, the vite build triggered by cargo tauri build + # would ship a DSN-less UI with no source-map upload. VITE_SENTRY_DSN: ${{ vars.VITE_SENTRY_DSN }} VITE_DEBUG: ${{ vars.VITE_DEBUG }} - # Sentry release tracking (`#405`) — must match the first `pnpm build` - # so source maps uploaded there still resolve against the final bundle - # produced by the tauri-driven rebuild. + # Sentry release tracking (`#405`) — ensures source maps uploaded by + # `@sentry/vite-plugin` resolve against the final bundled artifacts. VITE_BUILD_SHA: ${{ needs.prepare-build.outputs.sha }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 542 - 552, The comments incorrectly refer to an earlier "pnpm build" step that no longer exists; update the explanatory text around VITE_SENTRY_DSN, VITE_DEBUG, VITE_BUILD_SHA and SENTRY_RELEASE to state that Vite is invoked via tauri.conf.json's beforeBuildCommand (i.e., `vite build` runs as part of `cargo tauri build`), remove all references to "first/earlier pnpm build", and clearly state that VITE_BUILD_SHA and SENTRY_RELEASE must match the build invocation that uploads source maps (the tauri-triggered Vite build) so Sentry source maps resolve correctly.
🧹 Nitpick comments (9)
app/src/pages/Conversations.tsx (1)
401-401: AlignaddInferenceResponsewith the new UUID ID scheme.Good move from
Date.now()+Math.random()tocrypto.randomUUID()— collision-safer and more consistent with a message-ID format. However,app/src/store/threadSlice.ts(per the relevant snippet, ~lines 123–150) still mints fallback IDs as`inference-${Date.now()}-${Math.random()}`inaddInferenceResponse. Consider updating that site too so user/agent messages use the samemsg_<uuid>shape.♻️ Proposed follow-up in
app/src/store/threadSlice.ts- id: payload.messageId ?? `inference-${Date.now()}-${Math.random()}`, + id: payload.messageId ?? `msg_${globalThis.crypto.randomUUID()}`,Note:
globalThis.crypto.randomUUID()requires a secure context (Tauri webview andhttps/localhostqualify); if this code ever runs in a plain-httpnon-localhost preview, guard with a fallback. Not blocking for this PR's scope (pnpm migration).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/pages/Conversations.tsx` at line 401, Update addInferenceResponse in threadSlice.ts to produce IDs matching the new msg_<uuid> scheme used elsewhere: replace the current fallback template string (`inference-${Date.now()}-${Math.random()}`) with `msg_${globalThis.crypto?.randomUUID ? globalThis.crypto.randomUUID() : /* fallback uuid-like */ }` so both user/agent messages share the same prefix and UUID shape; ensure the change targets the addInferenceResponse function and include a safe fallback when globalThis.crypto.randomUUID is unavailable so code still runs in non-secure contexts.tests/webview_apis_bridge.rs (1)
63-72: Consider logging the swallowed errors in the accept loop.Both
listener.accept()(line 64-66) andaccept_async(line 69-72) discard their error and silentlycontinue/return. If CI ever has a flaky mock-server hang, there will be zero signal about why the handshake or accept failed. A singleeprintln!/tracing::debug!on each error branch would make these paths debuggable without changing behavior.As per coding guidelines: "Add substantial development-oriented logging at entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error paths".
🪵 Suggested minimal logging
loop { let (stream, _peer) = match listener.accept().await { Ok(v) => v, - Err(_) => continue, + Err(e) => { + eprintln!("[webview_apis-mock] accept failed: {e}"); + continue; + } }; tokio::spawn(async move { let ws = match tokio_tungstenite::accept_async(stream).await { Ok(w) => w, - Err(_) => return, + Err(e) => { + eprintln!("[webview_apis-mock] ws handshake failed: {e}"); + return; + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/webview_apis_bridge.rs` around lines 63 - 72, The accept loop currently swallows errors from listener.accept() and tokio_tungstenite::accept_async() (the match arms around listener.accept().await and tokio_tungstenite::accept_async(stream).await); update those error branches to log the error (e.g., eprintln! or tracing::debug!) including context like "listener.accept failed" and "accept_async handshake failed" while preserving the existing continue/return behavior so functionality doesn't change.docs/BUILDING.md (1)
103-117: Split Option 1 and Option 2 into separate code blocks for clarity.The two options are currently inside a single
```bashblock, and the#!/bin/bashshebang on line 112 is part of the wrapper-script content to create, not a line to run in the same shell as Option 1. Readers copy-pasting the whole block will get a confusing hybrid. Suggest splitting into two labeled blocks, and also useexec/ append toLD_LIBRARY_PATHto avoid clobbering any pre-existing value.📝 Suggested doc structure
**Option 1 — Direct invocation** ```bash REL_DIR=app/src-tauri/target/aarch64-unknown-linux-gnu/release CEF_DIR=$(ls -d "$REL_DIR"/build/cef-dll-sys-*/out/cef_linux_aarch64 2>/dev/null | head -n1) export LD_LIBRARY_PATH="$CEF_DIR:$REL_DIR/deps:$REL_DIR${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" "$REL_DIR/OpenHuman" --no-sandbox ``` **Option 2 — Wrapper script (recommended)** Save the following to `~/bin/openhuman` and `chmod +x` it: ```bash #!/bin/bash REL_DIR=/path/to/app/src-tauri/target/aarch64-unknown-linux-gnu/release CEF_DIR=$(ls -d "$REL_DIR"/build/cef-dll-sys-*/out/cef_linux_aarch64 2>/dev/null | head -n1) export LD_LIBRARY_PATH="$CEF_DIR:$REL_DIR/deps:$REL_DIR${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" exec "$REL_DIR/OpenHuman" --no-sandbox "$@" ```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/BUILDING.md` around lines 103 - 117, Split the single bash block into two separate labeled code blocks ("Option 1 — Direct invocation" and "Option 2 — Wrapper script (recommended)"); leave the shebang (#!/bin/bash) and the wrapper content only in the wrapper block and show instructions to save to ~/bin/openhuman and chmod +x it; in both blocks set REL_DIR and CEF_DIR as before but export LD_LIBRARY_PATH by appending the existing value using export LD_LIBRARY_PATH="$CEF_DIR:$REL_DIR/deps:$REL_DIR${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}" to avoid clobbering, and in the wrapper use exec "$REL_DIR/OpenHuman" --no-sandbox "$@" (quote variables) so the script replaces the shell..github/workflows/test.yml (1)
251-277: Commented-oute2e-macosjob still referencesyarn.Lines 255, 261, 274, 277 still use
cache: "yarn",yarn install --frozen-lockfile, andyarn workspace openhuman-app …. Even though the job is commented out, it serves as the template for when macOS E2E is re-enabled — once this PR lands, reviving the job with yarn commands will be broken. Mirror the pnpm migration applied to thee2e-linuxblock above for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 251 - 277, The commented-out e2e-macos job still uses yarn-specific settings; update that template to match the pnpm migration used in e2e-linux: change cache: "yarn" to the pnpm cache key, replace `yarn install --frozen-lockfile` with the repository-wide pnpm install (e.g. `pnpm -w install --frozen-lockfile`), and replace workspace invocations like `yarn workspace openhuman-app test:e2e:build` / `yarn workspace openhuman-app test:e2e:all:flows` with the equivalent pnpm workspace commands used in e2e-linux (e.g. `pnpm -w --filter openhuman-app...` or the exact pnpm commands your repo uses) so the e2e-macos template will work when re-enabled.package.json (2)
29-30:typecheckandcompileare now exact aliases — consider collapsing.Both scripts resolve to
pnpm --filter openhuman-app compile(which inapp/package.jsonistsc --noEmit). Keeping two names is harmless, but one of them can delegate to the other to avoid future drift:🔧 Suggested tweak
- "typecheck": "pnpm --filter openhuman-app compile" + "typecheck": "pnpm compile"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 29 - 30, The two npm scripts "typecheck" and "compile" currently point to the same command and may drift; collapse them by making one delegate to the other (e.g., keep "compile" as the primary and change "typecheck" to run "pnpm --filter openhuman-app compile"), updating the package.json "scripts" section so only one script contains the actual work and the other simply invokes it.
4-4: Optional: pinpackageManagerwith a Corepack integrity hash.Corepack supports an SRI-style suffix on
packageManagerfor integrity verification:"packageManager": "pnpm@10.10.0+sha512-<hash>". This hardens reproducibility and detects tampering. To generate the hash, runcorepack use pnpm@10.10.0locally—Corepack will compute and write the full specifier to package.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 4, Update the packageManager entry to include Corepack's integrity suffix: run "corepack use pnpm@10.10.0" locally to let Corepack compute the SRI hash and then replace the current "packageManager": "pnpm@10.10.0" value with the full specifier it writes (e.g., "pnpm@10.10.0+sha512-<hash>") so the packageManager field in package.json contains the integrity hash for reproducible installs..github/workflows/release.yml (1)
563-569: Consider using the same heap size at step-level and inline.Step-level
NODE_OPTIONS=--max-old-space-size=4096(line 564) is overridden by the inline--max-old-space-size=8192on line 569 for thecargo tauri buildprocess (per the comment, to reach the vite child on macOS ARM). Any othernodeinvocation during the step runs at 4 GB, which works today but makes the two values look contradictory. Aligning them (or dropping the step-level one and relying on the inline value as the source of truth) would remove the confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 563 - 569, The step-level NODE_OPTIONS value (--max-old-space-size=4096) conflicts with the inline override (--max-old-space-size=8192) used for the cargo tauri build command; update the workflow so the two are consistent by either setting the step-level NODE_OPTIONS to --max-old-space-size=8192 or removing the step-level NODE_OPTIONS and keeping only the inline NODE_OPTIONS="--max-old-space-size=8192" before the cargo tauri build invocation (ensure references to MATRIX_ARGS, beforeBuildCommand and any other node children are still covered by the chosen value)..github/workflows/typecheck.yml (2)
26-30: Remove hardcoded pnpm version from workflow files and rely onpackageManagerfield inpackage.json.The
package.jsondeclares"packageManager": "pnpm@10.10.0", andpnpm/action-setup@v4automatically detects this when theversioninput is omitted. Currently, six workflow files have hardcodedversion: 10.10.0(typecheck.yml, release.yml, e2e-agent-review.yml, test.yml, build.yml, build-windows.yml), creating multiple sources of truth that must be kept in sync during version upgrades.🔧 Proposed fix
- name: Setup pnpm uses: pnpm/action-setup@v4 with: - version: 10.10.0 cache: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/typecheck.yml around lines 26 - 30, Remove the hardcoded pnpm version from the workflow step that sets up pnpm: locate the step with "name: Setup pnpm" and "uses: pnpm/action-setup@v4" and delete the "version: 10.10.0" input so the action will auto-detect the pnpm version from the package.json "packageManager" field; apply the same change across the other workflows (release.yml, e2e-agent-review.yml, test.yml, build.yml, build-windows.yml) that contain the same "Setup pnpm" step.
37-38: Use--frozen-lockfilein CI instead of--no-frozen-lockfile.In CI,
pnpm install --no-frozen-lockfilewill silently updatepnpm-lock.yamlwhenpackage.jsonand the lockfile disagree, masking lockfile drift and making CI results non-reproducible against what was committed. pnpm's default in CI is--frozen-lockfile; the explicit opt-out here should be flipped to align with best practices.This same pattern is used in
.github/workflows/release.yml(line 401) and.github/workflows/e2e-agent-review.yml(line 96) — fixing it across the three CI workflows is recommended so release builds and E2E also catch lockfile drift.🔧 Proposed fix
- name: Install dependencies - run: pnpm install --no-frozen-lockfile + run: pnpm install --frozen-lockfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/typecheck.yml around lines 37 - 38, Replace the pnpm install invocation that currently uses the opt-out flag by changing the "run" command string from `pnpm install --no-frozen-lockfile` to `pnpm install --frozen-lockfile` in the CI workflow step named "Install dependencies" (the job step where the run command is `pnpm install ...`); apply the same change to the identical steps in the other workflows that use `pnpm install --no-frozen-lockfile` so CI will fail on lockfile drift instead of silently updating the lockfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-windows.yml:
- Around line 83-84: Replace the CI dependency install step that uses "pnpm
install --no-frozen-lockfile" with a strict frozen lockfile invocation so CI
fails on lockfile drift: update the "Install dependencies" step (the run command
under that job) to "pnpm install --frozen-lockfile" and apply the same change to
the other workflow files mentioned (.github/workflows/build.yml,
.github/workflows/test.yml, .github/workflows/release.yml) so all CI pipelines
use the frozen lockfile policy; if tests fail locally due to an out-of-sync
lockfile, regenerate and commit the updated pnpm-lock.yaml before re-running CI.
In @.github/workflows/typecheck.yml:
- Around line 50-53: The "Run ESLint" workflow step currently hides lint
failures by appending "|| true" to the command; remove the "|| true" so that
"pnpm --filter openhuman-app lint" fails the step on lint errors, or if a
specific rule is known to be broken, disable that rule via ESLint config or pass
a targeted override (e.g., using --rule) instead of silencing the entire job;
update the workflow step named "Run ESLint" and ensure the CI exits non‑zero on
lint failures so regressions are caught.
In `@app/src-tauri/tauri.conf.json`:
- Around line 54-64: Update the Debian dependency names to match the runtime and
Debian naming: in tauri.conf.json locate the "linux" -> "deb" -> "depends" array
and replace "libwebkit2gtk-4.0-37" with "libwebkit2gtk-4.1-0" and
"libgdk-pixbuf2.0-0" with the correct "libgdk-pixbuf-2.0-0"; make the same
substitutions in packages/deb/control.in so the package declares
libwebkit2gtk-4.1-0 and libgdk-pixbuf-2.0-0 as dependencies.
In `@app/src/components/settings/panels/AIPanel.tsx`:
- Around line 50-57: loadAIPreview and loadLocalAiStatus are recreated each
render causing the useEffect to re-run endlessly; wrap both functions in
useCallback with appropriate dependency arrays (e.g., include only stable
refs/state they actually use) so their identities remain stable, then keep them
in the effect deps; specifically memoize the loadAIPreview function that calls
aiGetConfig and setAiConfig/setLoading/setError and the loadLocalAiStatus
function that calls openhumanLocalAiStatus and set state, and leave the
setInterval logic in the effect as-is so the timer is created only once on
mount.
In `@packages/deb/control.in`:
- Line 8: The control.in currently contains unresolved substvars
(${shlibs:Depends}, ${misc:Depends}) and a GUI-based dependency list
(libgtk-3-0, libwebkit2gtk-4.0-37, etc.) which don't apply to the CLI package;
either remove those substvars and replace the Depends line with a minimal static
list, or change the packaging flow to run dpkg-shlibdeps and dpkg-gencontrol
instead of calling dpkg-deb directly so substvars are expanded. To fix: inspect
the actual CLI binary (/usr/bin/openhuman) and Cargo.toml, run ldd or objdump -p
on that binary to discover real shared-library requirements, then update the
Depends entry in packages/deb/control.in to only include those libraries (or
implement dpkg-shlibdeps + dpkg-gencontrol in build.sh if you want automatic
substvar expansion).
In `@tests/webview_apis_bridge.rs`:
- Line 111: Wrap the std::env::set_var("OPENHUMAN_WEBVIEW_APIS_PORT",
port.to_string()) call in an unsafe block and add a SAFETY comment explaining
why this is safe (e.g., this is a mock server port, not a secret, and the test
controls concurrency), for example by updating the use in
tests/webview_apis_bridge.rs (the test that sets the port) accordingly; also
note there are other tests that call env::set_var and should be fixed in a
follow-up to standardize this pattern.
---
Outside diff comments:
In @.github/workflows/release.yml:
- Around line 542-552: The comments incorrectly refer to an earlier "pnpm build"
step that no longer exists; update the explanatory text around VITE_SENTRY_DSN,
VITE_DEBUG, VITE_BUILD_SHA and SENTRY_RELEASE to state that Vite is invoked via
tauri.conf.json's beforeBuildCommand (i.e., `vite build` runs as part of `cargo
tauri build`), remove all references to "first/earlier pnpm build", and clearly
state that VITE_BUILD_SHA and SENTRY_RELEASE must match the build invocation
that uploads source maps (the tauri-triggered Vite build) so Sentry source maps
resolve correctly.
In @.husky/pre-push:
- Around line 40-43: Replace the leftover Yarn invocation with pnpm to avoid
breaking pnpm-only environments: change the command that runs the cmd-tokens
lint (currently invoked as "yarn --cwd app lint:commands-tokens") to use pnpm
(for example "pnpm --dir app run lint:commands-tokens"), keeping the surrounding
set +e / set -e and the CMD_TOKENS_EXIT capture so the hook still records the
linter's exit code.
In `@app/scripts/e2e-run-spec.sh`:
- Around line 108-118: Update the user-facing error messages in e2e-run-spec.sh
to reference pnpm instead of yarn: change the two echo lines that currently tell
users to run 'yarn test:e2e:build' (found near the DIST_JS existence check and
the mock URL grep check using the DIST_JS and E2E_MOCK_PORT variables) so they
instruct running 'pnpm --filter app test:e2e:build' or simply 'pnpm
test:e2e:build' consistent with the repo's pnpm usage; ensure both messages are
updated so they consistently mention pnpm and preserve the surrounding wording
and exit behavior.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 563-569: The step-level NODE_OPTIONS value
(--max-old-space-size=4096) conflicts with the inline override
(--max-old-space-size=8192) used for the cargo tauri build command; update the
workflow so the two are consistent by either setting the step-level NODE_OPTIONS
to --max-old-space-size=8192 or removing the step-level NODE_OPTIONS and keeping
only the inline NODE_OPTIONS="--max-old-space-size=8192" before the cargo tauri
build invocation (ensure references to MATRIX_ARGS, beforeBuildCommand and any
other node children are still covered by the chosen value).
In @.github/workflows/test.yml:
- Around line 251-277: The commented-out e2e-macos job still uses yarn-specific
settings; update that template to match the pnpm migration used in e2e-linux:
change cache: "yarn" to the pnpm cache key, replace `yarn install
--frozen-lockfile` with the repository-wide pnpm install (e.g. `pnpm -w install
--frozen-lockfile`), and replace workspace invocations like `yarn workspace
openhuman-app test:e2e:build` / `yarn workspace openhuman-app
test:e2e:all:flows` with the equivalent pnpm workspace commands used in
e2e-linux (e.g. `pnpm -w --filter openhuman-app...` or the exact pnpm commands
your repo uses) so the e2e-macos template will work when re-enabled.
In @.github/workflows/typecheck.yml:
- Around line 26-30: Remove the hardcoded pnpm version from the workflow step
that sets up pnpm: locate the step with "name: Setup pnpm" and "uses:
pnpm/action-setup@v4" and delete the "version: 10.10.0" input so the action will
auto-detect the pnpm version from the package.json "packageManager" field; apply
the same change across the other workflows (release.yml, e2e-agent-review.yml,
test.yml, build.yml, build-windows.yml) that contain the same "Setup pnpm" step.
- Around line 37-38: Replace the pnpm install invocation that currently uses the
opt-out flag by changing the "run" command string from `pnpm install
--no-frozen-lockfile` to `pnpm install --frozen-lockfile` in the CI workflow
step named "Install dependencies" (the job step where the run command is `pnpm
install ...`); apply the same change to the identical steps in the other
workflows that use `pnpm install --no-frozen-lockfile` so CI will fail on
lockfile drift instead of silently updating the lockfile.
In `@app/src/pages/Conversations.tsx`:
- Line 401: Update addInferenceResponse in threadSlice.ts to produce IDs
matching the new msg_<uuid> scheme used elsewhere: replace the current fallback
template string (`inference-${Date.now()}-${Math.random()}`) with
`msg_${globalThis.crypto?.randomUUID ? globalThis.crypto.randomUUID() : /*
fallback uuid-like */ }` so both user/agent messages share the same prefix and
UUID shape; ensure the change targets the addInferenceResponse function and
include a safe fallback when globalThis.crypto.randomUUID is unavailable so code
still runs in non-secure contexts.
In `@docs/BUILDING.md`:
- Around line 103-117: Split the single bash block into two separate labeled
code blocks ("Option 1 — Direct invocation" and "Option 2 — Wrapper script
(recommended)"); leave the shebang (#!/bin/bash) and the wrapper content only in
the wrapper block and show instructions to save to ~/bin/openhuman and chmod +x
it; in both blocks set REL_DIR and CEF_DIR as before but export LD_LIBRARY_PATH
by appending the existing value using export
LD_LIBRARY_PATH="$CEF_DIR:$REL_DIR/deps:$REL_DIR${LD_LIBRARY_PATH:+:$LD_LIBRARY_PATH}"
to avoid clobbering, and in the wrapper use exec "$REL_DIR/OpenHuman"
--no-sandbox "$@" (quote variables) so the script replaces the shell.
In `@package.json`:
- Around line 29-30: The two npm scripts "typecheck" and "compile" currently
point to the same command and may drift; collapse them by making one delegate to
the other (e.g., keep "compile" as the primary and change "typecheck" to run
"pnpm --filter openhuman-app compile"), updating the package.json "scripts"
section so only one script contains the actual work and the other simply invokes
it.
- Line 4: Update the packageManager entry to include Corepack's integrity
suffix: run "corepack use pnpm@10.10.0" locally to let Corepack compute the SRI
hash and then replace the current "packageManager": "pnpm@10.10.0" value with
the full specifier it writes (e.g., "pnpm@10.10.0+sha512-<hash>") so the
packageManager field in package.json contains the integrity hash for
reproducible installs.
In `@tests/webview_apis_bridge.rs`:
- Around line 63-72: The accept loop currently swallows errors from
listener.accept() and tokio_tungstenite::accept_async() (the match arms around
listener.accept().await and tokio_tungstenite::accept_async(stream).await);
update those error branches to log the error (e.g., eprintln! or
tracing::debug!) including context like "listener.accept failed" and
"accept_async handshake failed" while preserving the existing continue/return
behavior so functionality doesn't change.
🪄 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: c3e6bc34-bbd8-482e-8e14-a9d36079f150
⛔ Files ignored due to path filters (2)
app/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (27)
.github/workflows/build-windows.yml.github/workflows/build.yml.github/workflows/e2e-agent-review.yml.github/workflows/release.yml.github/workflows/test.yml.github/workflows/typecheck.yml.husky/pre-pushapp/package.jsonapp/scripts/e2e-build.shapp/scripts/e2e-run-spec.shapp/src-tauri/src/lib.rsapp/src-tauri/tauri.conf.jsonapp/src/AppRoutes.tsxapp/src/components/settings/panels/AIPanel.tsxapp/src/pages/Conversations.tsxapp/test/e2e/helpers/artifacts.tsapp/test/e2e/specs/agent-review.spec.tsapp/test/wdio.conf.tsdocs/BUILDING.mdpackage.jsonpackages/deb/control.inpnpm-workspace.yamlscripts/review/README.mdscripts/review/cli.shsrc/core/all.rssrc/openhuman/webhooks/bus.rstests/webview_apis_bridge.rs
💤 Files with no reviewable changes (3)
- app/src/AppRoutes.tsx
- src/core/all.rs
- src/openhuman/webhooks/bus.rs
Comment on lines
+54
to
+64
| "linux": { | ||
| "deb": { | ||
| "depends": [ | ||
| "libgtk-3-0", | ||
| "libwebkit2gtk-4.0-37", | ||
| "libx11-6", | ||
| "libgdk-pixbuf2.0-0", | ||
| "libglib2.0-0" | ||
| ] | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect what webkit2gtk version the app actually links against, and whether
# dev dependencies elsewhere reference 4.0 or 4.1.
rg -nP --no-heading 'webkit2gtk-4\.[01]' -g '!**/node_modules/**' -g '!**/target/**'
rg -nP --no-heading 'gdk-pixbuf2?\.0' -g '!**/node_modules/**' -g '!**/target/**'
# Also check Cargo manifests for wry/tao/tauri version constraints that imply webkit2gtk version.
fd -t f Cargo.toml --exec rg -nH -P '^(wry|tao|tauri)\b|webkit2gtk' {}Repository: tinyhumansai/openhuman
Length of output: 2479
🌐 Web query:
libgdk-pixbuf2.0-0 vs libgdk-pixbuf-2.0-0 ubuntu 22.04 debian package name
💡 Result:
In Ubuntu 22.04 (Jammy), the correct Debian package name for the GDK Pixbuf runtime library is libgdk-pixbuf-2.0-0. The variant libgdk-pixbuf2.0-0 does not exist; it appears to be a common misspelling or confusion from older naming conventions seen in some Debian versions or documentation snippets. Official Ubuntu package listings confirm libgdk-pixbuf-2.0-0 as the shared library package from the gdk-pixbuf source, with version 2.42.8+dfsg-1ubuntu0.5 as of recent security updates. Related packages include libgdk-pixbuf2.0-common (data files), libgdk-pixbuf2.0-bin (tools), and libgdk-pixbuf-2.0-dev (headers). To install: sudo apt install libgdk-pixbuf-2.0-0.
Citations:
- 1: https://launchpad.net/ubuntu/jammy/+source/gdk-pixbuf
- 2: https://www.ubuntuupdates.org/package/core/jammy/main/updates/libgdk-pixbuf-2.0-0
- 3: https://packages.ubuntu.com/en/jammy-updates/libgdk-pixbuf-2.0-0
- 4: https://packages.ubuntu.com/jammy/libgdk-pixbuf-2.0-0
- 5: https://answers.launchpad.net/ubuntu/jammy/amd64/libgdk-pixbuf-2.0-0
Fix the libwebkit2gtk and libgdk-pixbuf package versions to match the actual runtime and correct Debian naming.
The added deb.depends lists libwebkit2gtk-4.0-37, but the E2E build environment (e2e/Dockerfile line 21) installs libwebkit2gtk-4.1-dev, and Tauri v2 links against the 4.1 runtime. Declaring a 4.0-37 dependency causes dpkg to install the wrong library version; users will see missing-symbol errors at launch.
Also, libgdk-pixbuf2.0-0 does not exist in Ubuntu 22.04. The correct package name is libgdk-pixbuf-2.0-0 (note the hyphen). The current name will cause installation to fail.
Update both app/src-tauri/tauri.conf.json and packages/deb/control.in to use:
libwebkit2gtk-4.1-0(instead of 4.0-37)libgdk-pixbuf-2.0-0(instead of 2.0-0)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src-tauri/tauri.conf.json` around lines 54 - 64, Update the Debian
dependency names to match the runtime and Debian naming: in tauri.conf.json
locate the "linux" -> "deb" -> "depends" array and replace
"libwebkit2gtk-4.0-37" with "libwebkit2gtk-4.1-0" and "libgdk-pixbuf2.0-0" with
the correct "libgdk-pixbuf-2.0-0"; make the same substitutions in
packages/deb/control.in so the package declares libwebkit2gtk-4.1-0 and
libgdk-pixbuf-2.0-0 as dependencies.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/e2e-agent-review.yml (1)
42-95:⚠️ Potential issue | 🔴 CriticalBroken YAML indentation — workflow will fail to parse.
- name: Setup pnpm(42) and- name: Install JS dependencies(93) are at column 0 with their child keys at 6 spaces. YAMLlint flags line 42. Even though this workflow is currently dispatch-only, the whole file must still parse — otherwiseworkflow_dispatchis also unavailable.🔧 Proposed fix
-- name: Setup pnpm - if: steps.gate.outputs.present == 'true' - uses: pnpm/action-setup@v4 - with: - cache: true + - name: Setup pnpm + if: steps.gate.outputs.present == 'true' + uses: pnpm/action-setup@v4 + with: + cache: true ... -- name: Install JS dependencies - if: steps.gate.outputs.present == 'true' - run: pnpm install --frozen-lockfile + - name: Install JS dependencies + if: steps.gate.outputs.present == 'true' + run: pnpm install --frozen-lockfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/e2e-agent-review.yml around lines 42 - 95, The workflow has incorrect YAML indentation: the step entries "Setup pnpm" and "Install JS dependencies" are misaligned (at column 0) relative to their child keys which are indented, causing the parser to fail; fix by re-indenting those step blocks so they are nested under the proper job/steps sequence (ensure "- name: Setup pnpm" and "- name: Install JS dependencies" have the same indentation level as the other step list items and their "if:", "uses:", and "run:" keys are indented one level under each "- name"), and then validate the file (e.g., with yamllint) to confirm the workflow parses..github/workflows/release.yml (2)
541-567:⚠️ Potential issue | 🔴 CriticalEnv block and
run:key indentation are broken — Vite env vars will be dropped and NODE_OPTIONS won't propagate.In the "Build and package Tauri app" step, the env values are split between two indentation levels:
VITE_BACKEND_URL(540) andSENTRY_RELEASEonward (551+) are at 10 spaces (correct underenv:).VITE_SENTRY_DSN,VITE_DEBUG,VITE_BUILD_SHAand the surrounding comments (541–550) sit at 6 spaces, outside theenv:map.run: |at line 563 is at 4 spaces — should be 8 to be a sibling ofenv:.Even if the file somehow parses,
VITE_SENTRY_DSN/VITE_DEBUG/VITE_BUILD_SHAwon't be exported to thecargo tauri buildchild process, so the Vite sub-build called viabeforeBuildCommandwon't include Sentry DSN / debug flags / release SHA — source maps will fail to resolve against the published bundle, silently breaking error tracking on prod releases. AndNODE_OPTIONSdrops out of scope, re-triggering the very OOM on macos-arm64 that lines 564–566 explicitly warn about.🔧 Proposed fix
VITE_BACKEND_URL: ${{ needs.prepare-build.outputs.base_url }} - # Re-declare Vite env so tauri.conf.json's `beforeBuildCommand` - # (`vite build`) bakes Sentry + debug flags into the final bundle. - # Vite is invoked via tauri.conf.json's beforeBuildCommand during - # `cargo tauri build`, so these env vars must be present here. - VITE_SENTRY_DSN: ${{ vars.VITE_SENTRY_DSN }} - VITE_DEBUG: ${{ vars.VITE_DEBUG }} - # Sentry release tracking (`#405`) — must match the Vite build triggered - # by Tauri so source maps uploaded there resolve correctly against the - # final bundle produced by the tauri-driven build. - VITE_BUILD_SHA: ${{ needs.prepare-build.outputs.sha }} + # Re-declare Vite env so tauri.conf.json's `beforeBuildCommand` + # (`vite build`) bakes Sentry + debug flags into the final bundle. + VITE_SENTRY_DSN: ${{ vars.VITE_SENTRY_DSN }} + VITE_DEBUG: ${{ vars.VITE_DEBUG }} + # Sentry release tracking (`#405`) + VITE_BUILD_SHA: ${{ needs.prepare-build.outputs.sha }} SENTRY_RELEASE: openhuman@${{ needs.prepare-build.outputs.version }}+${{ needs.prepare-build.outputs.sha }} ... MATRIX_ARGS: ${{ matrix.settings.args }} - run: | - # Inline NODE_OPTIONS so it reaches the vite child spawned by - # beforeBuildCommand. Step-level env was observed not to propagate - # on macos-arm64 runners, causing OOM at node's ~2GB auto default. - NODE_OPTIONS="--max-old-space-size=8192" cargo tauri build -c "$TAURI_CONFIG_OVERRIDE" $MATRIX_ARGS + run: | + NODE_OPTIONS="--max-old-space-size=8192" cargo tauri build -c "$TAURI_CONFIG_OVERRIDE" $MATRIX_ARGS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 541 - 567, The env block indentation is broken so VITE_SENTRY_DSN, VITE_DEBUG and VITE_BUILD_SHA are not under env: and the run: | key is mis-indented, causing those vars and the inline NODE_OPTIONS to be dropped; fix by aligning the VITE_* entries and their surrounding comment to the same indentation level as the other env keys (so they are children of env:), and indent the run: | line to be a sibling of env: so the inline NODE_OPTIONS="--max-old-space-size=8192" cargo tauri build -c "$TAURI_CONFIG_OVERRIDE" $MATRIX_ARGS invocation runs with those env vars and NODE_OPTIONS propagated to the beforeBuildCommand/vite child process.
292-400:⚠️ Potential issue | 🔴 CriticalBroken YAML indentation — release workflow will fail to parse.
Both
- name: Setup pnpm(292) and- name: Install dependencies(399) are at column 0 with their child keys at 6 spaces, whereas sibling steps use 6 spaces for- name:and 8 for step keys. YAMLlint flags line 292. This blocks the entire release pipeline — no desktop builds, no Docker image, nothing.🔧 Proposed fix
-- name: Setup pnpm - uses: pnpm/action-setup@v4 - with: - cache: true + - name: Setup pnpm + uses: pnpm/action-setup@v4 + with: + cache: true ... -- name: Install dependencies - run: pnpm install --frozen-lockfile + - name: Install dependencies + run: pnpm install --frozen-lockfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 292 - 400, The workflow has inconsistent YAML indentation for the step entries "Setup pnpm" and "Install dependencies" which breaks parsing; fix by aligning these step blocks to match the other steps in the job (ensure each "- name: <step>" uses the same base indentation as sibling steps and their child keys like "uses", "with", "run", "if", and "shell" are one additional indent level), adjust the "Setup pnpm" block (contains uses: pnpm/action-setup@v4 and with: cache: true) and the "Install dependencies" run: pnpm install --frozen-lockfile block to the same indentation pattern as surrounding steps so the YAML linter no longer reports errors.
🧹 Nitpick comments (5)
app/src/components/settings/panels/AIPanel.tsx (1)
41-48: Optional: log the swallowed error inloadLocalAiStatus.The
catch {}discards the error and silently setslocalAiStatustonull, which makes failures ofopenhumanLocalAiStatus(including when the polling hits it every 5s) invisible during development. Consider a namespaceddebuglog so repeated failures aren't lost.💡 Proposed tweak
- const loadLocalAiStatus = useCallback(async () => { - try { - const result = await openhumanLocalAiStatus(); - setLocalAiStatus(result.result); - } catch { - setLocalAiStatus(null); - } - }, []); + const loadLocalAiStatus = useCallback(async () => { + try { + const result = await openhumanLocalAiStatus(); + setLocalAiStatus(result.result); + } catch (err) { + debug('ai-panel:local-status-failed', err); + setLocalAiStatus(null); + } + }, []);As per coding guidelines: "Add substantial development-oriented logging at entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error paths" and "Frontend debug logging should use namespaced
debugcalls with dev-only detail."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/settings/panels/AIPanel.tsx` around lines 41 - 48, The catch block in loadLocalAiStatus silently swallows errors from openhumanLocalAiStatus; update loadLocalAiStatus (the useCallback that calls openhumanLocalAiStatus and sets setLocalAiStatus) to log the caught error using a namespaced debug logger (e.g., debug('components:settings:AIPanel') or similar) in the catch so failures are visible in development; keep the behavior of setting setLocalAiStatus(null) on error but include a concise, dev-only debug message with the error details to avoid silent failures during polling.app/test/e2e/specs/linux-cef-deb-runtime.spec.ts (1)
22-22: Dead code:isTauriDriverimport andgetProcessEnvhelper.
isTauriDriveris imported but never referenced in this file.getProcessEnvalways returns{}(the innerbrowser.executecallback is a no-op), is never called, and the leading comment describes behavior it doesn't implement. Please remove or actually implement them before this lands, otherwise it will create confusion during future debugging.♻️ Proposed cleanup
-import { isTauriDriver, supportsExecuteScript } from '../helpers/platform'; +import { supportsExecuteScript } from '../helpers/platform'; @@ -/** - * Get the current process environment variables from WebView context. - * Used to verify env vars are properly passed to the core sidecar. - */ -async function getProcessEnv(): Promise<Record<string, string> | null> { - if (!supportsExecuteScript()) { - return null; - } - - try { - return await browser.execute(() => { - // Access process env via Tauri API if available, otherwise return empty - return {}; - }); - } catch { - return null; - } -}Also applies to: 63-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/test/e2e/specs/linux-cef-deb-runtime.spec.ts` at line 22, Remove the dead code: delete the unused import isTauriDriver from the import statement (keep supportsExecuteScript) and remove the unused getProcessEnv helper function and its misleading comment (or implement it properly if needed); ensure no references to isTauriDriver or getProcessEnv remain in this spec (look for the import line and the getProcessEnv helper definition) and run tests to verify nothing else depends on them..github/workflows/test.yml (1)
183-183: Commented e2e-linux block still uses--no-frozen-lockfile.Every live CI install step in this PR was flipped to
--frozen-lockfile; this commented-out path will re-introduce the drift-masking behavior the moment the e2e-linux job is re-enabled. Align it now so future uncomment is safe by default.♻️ Proposed change
- # run: pnpm install --no-frozen-lockfile + # run: pnpm install --frozen-lockfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml at line 183, Commented e2e-linux CI step still uses --no-frozen-lockfile which will reintroduce drift when re-enabled; update the commented run line in the e2e-linux block to use --frozen-lockfile instead of --no-frozen-lockfile (replace the token in the commented "run: pnpm install --no-frozen-lockfile" line) so the commented path matches the active CI behavior and is safe to uncomment later..github/workflows/build-windows.yml (1)
28-33: Drop the hardcodedversion: 10.10.0to match the rest of the migration.The PR explicitly removes the hardcoded pnpm version elsewhere and relies on
packageManagerinpackage.json(plus the corepack integrity hash) for version resolution;pnpm/action-setup@v4auto-detects frompackageManagerwhenversionis omitted. Keeping it pinned here is a second source of truth that will silently drift.♻️ Proposed change
- name: Setup pnpm uses: pnpm/action-setup@v4 with: - version: 10.10.0 cache: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-windows.yml around lines 28 - 33, Remove the hardcoded "version: 10.10.0" entry from the pnpm setup step so pnpm/action-setup@v4 will auto-detect the version from packageManager in package.json (and the corepack integrity hash); keep the "name: Setup pnpm", "uses: pnpm/action-setup@v4" and "cache: true" lines intact and ensure no other pinned version remains in this workflow to avoid a second source of truth.docs/BUILDING.md (1)
99-122: Heading hierarchy: nestOption 1/Option 2underRunning the ARM binary.Lines 103 and 112 use
###, matching their parent section on line 99. This flattens the TOC — Option 1 and Option 2 are conceptually children of "Running the ARM binary", so they should be####.♻️ Proposed change
### Running the ARM binary The binary requires the CEF library path to be set: -### Option 1 — Direct invocation +#### Option 1 — Direct invocation ... -### Option 2 — Wrapper script (recommended) +#### Option 2 — Wrapper script (recommended)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/BUILDING.md` around lines 99 - 122, The "Option 1" and "Option 2" headings are using the same level (###) as their parent "Running the ARM binary", flattening the hierarchy; change those two headings to a deeper level (use #### for the "Option 1 — Direct invocation" and "Option 2 — Wrapper script (recommended)" headings) so they are nested under the "Running the ARM binary" section and the TOC/heading structure is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-windows.yml:
- Around line 83-84: The YAML step "- name: Install dependencies" has
inconsistent indentation (the `- name:` is at column 0 while `run:` is indented
6 spaces) which breaks the workflow; fix by aligning the step under the job's
steps list so `- name: Install dependencies` is indented the same as other steps
(use 6 spaces before `- name:`) and indent the step keys like `run:` two spaces
further (8 spaces total), ensuring `run: pnpm install --frozen-lockfile` is
correctly nested.
In @.github/workflows/build.yml:
- Around line 56-67: The three workflow steps "Setup pnpm", "Setup Node.js
24.x", and "Install dependencies" are under-indented and will break YAML
parsing; update their indentation so each "- name:" key and its nested keys
(uses/with/run) align with the other step entries (use the same number of
leading spaces as sibling steps), ensuring the "- name:" lines and their child
keys have consistent indentation throughout the job.
In @.github/workflows/test.yml:
- Around line 36-50: The YAML workflow has mis-indented step entries for the
steps named "Setup pnpm", "Setup Node.js 24.x", "Install dependencies" and "Run
tests with coverage"; fix by aligning their indentation with the other steps
under the job's steps: ensure each "- name:" line uses the same leading spaces
as sibling steps (e.g., 6 spaces before "- name:") and the step keys under them
(uses:, with:, run:) are indented one level deeper (e.g., 8 spaces) so the
"unit-tests" and "rust-tests" jobs parse correctly.
In @.github/workflows/typecheck.yml:
- Around line 26-52: The YAML has broken indentation for the steps named "Setup
pnpm", "Install dependencies", and "Run ESLint": align each "- name: Setup
pnpm", "- name: Install dependencies", and "- name: Run ESLint" to the same
indentation level as other step entries and ensure their child keys ("uses",
"with", "run", "env") are indented exactly two spaces further than the "- name"
line; also fix the "Run ESLint" block so "env:" is aligned with "run:" (same
indentation) to resolve the parsing error.
In `@app/test/e2e/specs/linux-cef-deb-runtime.spec.ts`:
- Around line 220-229: The negative substring assertions for 'error' and 'panic'
in the "accessibility tree is accessible for diagnostics" test are brittle;
remove the two expect(...) .not.toContain(...) checks (leave the positive
expect(source.length).toBeGreaterThan(0) which verifies reachability), or if you
need to assert absence of crashes, replace them with checks for a structured
crash marker emitted by the app (e.g., assert !source.includes('<panic>') or
!source.includes('data-error="true"')) instead of plain substrings; update
references in this test around dumpAccessibilityTree() and
stepLog('Accessibility tree length') accordingly.
- Around line 1-18: Update the module-level docblock and any other inline
comments that reference "PR `#3`" (including the block near the end of the file
that repeats the same text) to use a short human description instead of the PR
number; replace occurrences of "PR `#3`" with "Linux CEF .deb runtime: core binary
resolution + tray gating" so the comment remains meaningful after rebases/merges
(search for the top-of-file docblock and the trailing comment block that echoes
the PR reference to locate the exact spots to change).
- Around line 312-324: The test "sidecar is running with non-default port when
OPENHUMAN_CORE_PORT is set" currently only calls
invokeTauriCommand('core_rpc_url') and checks URL format, so update it to
actually verify OPENHUMAN_CORE_PORT: either (A) set the env var before the
app/sidecar starts (e.g., export or pass OPENHUMAN_CORE_PORT into the test
harness/startup so the sidecar boots with that port) and then call
invokeTauriCommand('core_rpc_url') and assert the returned URL contains the
specific port you set (match /http:\/\/.+:EXPECTED_PORT\/rpc/), or (B) if you
don't want to control startup env here, rename the test to accurately reflect
that it only validates a well-formed RPC URL (change the test title and keep the
invokeTauriCommand('core_rpc_url') assertion). Ensure you reference the test
name and the invokeTauriCommand('core_rpc_url') call when making the change.
- Around line 165-186: The test currently includes a non-registered JSON-RPC
method "core.health" in the methods array, which will always produce a
method-not-found error and can mask real failures; update the test by removing
"core.health" from the methods list (leave only "core.ping" and "core.version"),
keep the loop that calls callOpenhumanRpc and populates results, and adjust any
surrounding comments or logs to reflect only the registered methods so the
health check truly validates the sidecar via core.ping and core.version.
- Around line 182-185: The assertion uses expect(anySuccess).toBe(true, ...)
which ignores the message; update the test so that when anySuccess is false you
explicitly throw with the diagnostic payload (e.g. if (!anySuccess) throw new
Error(`Expected at least one health check to pass. Results:
${JSON.stringify(results)}`)), then keep a plain expect(anySuccess).toBe(true)
or expect(anySuccess).toBeTruthy(); locate the assertion referencing anySuccess
and results and replace the two-arg expect with the manual throw pattern to
ensure failure output includes results.
In `@tests/linux_cef_deb_runtime_e2e.rs`:
- Around line 66-105: Tests that mutate environment vars
(core_bin_env_override_takes_precedence_when_exists,
core_bin_env_override_graceful_when_nonexistent, core_port_env_configuration,
core_rpc_url_env_override) race under parallel test execution and also collide
on the fixed temp dir used by create_fake_core_binary; fix by serializing
env-mutating tests with a global Mutex (e.g., define static TEST_ENV_LOCK:
once_cell::sync::Lazy<std::sync::Mutex<()>> and lock it at the start of each
test before calling EnvGuard::set) and replace the fixed std::env::temp_dir()
usage in create_fake_core_binary/test setup with tempfile::tempdir() (unique per
test) so fake binaries do not collide; apply the same locking and tempfile
changes to the other tests mentioned (lines ~136-156 and ~184-193).
- Around line 233-254: The test tray_setup_linux_cef_gate currently does nothing
useful (assert!(true)); replace it with an actual invocation of setup_tray()
under the appropriate cfg branches: call setup_tray() inside a
#[cfg(all(target_os = "linux", feature = "cef"))] block and assert it returns
Ok(()) and (optionally) that a warning log was emitted, and call setup_tray()
inside a #[cfg(not(all(target_os = "linux", feature = "cef")))] block and assert
it returns Ok(()) and that a tray was created (or the expected non-warning
behavior). Locate the test function tray_setup_linux_cef_gate and the
setup_tray() function to implement these conditional calls and proper assertions
so the test actually verifies the gated behavior instead of being a no-op.
- Around line 64-132: The tests are tautological because they assert values they
set instead of invoking the real resolution/parsing helpers; make the actual
functions used by production code callable from tests (e.g. mark
resolve_daemon_executable in src/openhuman/service/common.rs, core_port in
src/core/jsonrpc.rs, and whatever builds the RPC URL—e.g. core_rpc_url—as
pub(crate) or expose them via a small pub re-export module) and update each test
to call those functions directly (replace environment-only assertions with calls
to resolve_daemon_executable, core_port, and core_rpc_url and assert their
outputs), ensuring the packaged probe list used in tests is read from the same
constant/array used by resolve_daemon_executable instead of hard-coding paths.
- Around line 325-351: The test logging_patterns_are_grep_friendly documents log
strings that don't match actual emitters (e.g., "[core] default_core_bin:" and
"[tray] deferring tray setup to RunEvent::Ready" are absent), so either make the
test assert against the real source or remove it and document patterns at the
emission sites: update the test to read the emitter source (use
include_str!("../app/src-tauri/src/core_process.rs") or the appropriate Rust
file) and assert the expected patterns exist in that string (referencing the
log! / trace! / info! macros in core_process.rs and the RunEvent::Ready
handling), or delete logging_patterns_are_grep_friendly and add inline comments
next to the actual log::... macros that emit the grep-friendly patterns (and
sync the companion TS spec strings such as "[tray] skipping tray setup on
linux+cef" to match the chosen source of truth).
---
Outside diff comments:
In @.github/workflows/e2e-agent-review.yml:
- Around line 42-95: The workflow has incorrect YAML indentation: the step
entries "Setup pnpm" and "Install JS dependencies" are misaligned (at column 0)
relative to their child keys which are indented, causing the parser to fail; fix
by re-indenting those step blocks so they are nested under the proper job/steps
sequence (ensure "- name: Setup pnpm" and "- name: Install JS dependencies" have
the same indentation level as the other step list items and their "if:",
"uses:", and "run:" keys are indented one level under each "- name"), and then
validate the file (e.g., with yamllint) to confirm the workflow parses.
In @.github/workflows/release.yml:
- Around line 541-567: The env block indentation is broken so VITE_SENTRY_DSN,
VITE_DEBUG and VITE_BUILD_SHA are not under env: and the run: | key is
mis-indented, causing those vars and the inline NODE_OPTIONS to be dropped; fix
by aligning the VITE_* entries and their surrounding comment to the same
indentation level as the other env keys (so they are children of env:), and
indent the run: | line to be a sibling of env: so the inline
NODE_OPTIONS="--max-old-space-size=8192" cargo tauri build -c
"$TAURI_CONFIG_OVERRIDE" $MATRIX_ARGS invocation runs with those env vars and
NODE_OPTIONS propagated to the beforeBuildCommand/vite child process.
- Around line 292-400: The workflow has inconsistent YAML indentation for the
step entries "Setup pnpm" and "Install dependencies" which breaks parsing; fix
by aligning these step blocks to match the other steps in the job (ensure each
"- name: <step>" uses the same base indentation as sibling steps and their child
keys like "uses", "with", "run", "if", and "shell" are one additional indent
level), adjust the "Setup pnpm" block (contains uses: pnpm/action-setup@v4 and
with: cache: true) and the "Install dependencies" run: pnpm install
--frozen-lockfile block to the same indentation pattern as surrounding steps so
the YAML linter no longer reports errors.
---
Nitpick comments:
In @.github/workflows/build-windows.yml:
- Around line 28-33: Remove the hardcoded "version: 10.10.0" entry from the pnpm
setup step so pnpm/action-setup@v4 will auto-detect the version from
packageManager in package.json (and the corepack integrity hash); keep the
"name: Setup pnpm", "uses: pnpm/action-setup@v4" and "cache: true" lines intact
and ensure no other pinned version remains in this workflow to avoid a second
source of truth.
In @.github/workflows/test.yml:
- Line 183: Commented e2e-linux CI step still uses --no-frozen-lockfile which
will reintroduce drift when re-enabled; update the commented run line in the
e2e-linux block to use --frozen-lockfile instead of --no-frozen-lockfile
(replace the token in the commented "run: pnpm install --no-frozen-lockfile"
line) so the commented path matches the active CI behavior and is safe to
uncomment later.
In `@app/src/components/settings/panels/AIPanel.tsx`:
- Around line 41-48: The catch block in loadLocalAiStatus silently swallows
errors from openhumanLocalAiStatus; update loadLocalAiStatus (the useCallback
that calls openhumanLocalAiStatus and sets setLocalAiStatus) to log the caught
error using a namespaced debug logger (e.g.,
debug('components:settings:AIPanel') or similar) in the catch so failures are
visible in development; keep the behavior of setting setLocalAiStatus(null) on
error but include a concise, dev-only debug message with the error details to
avoid silent failures during polling.
In `@app/test/e2e/specs/linux-cef-deb-runtime.spec.ts`:
- Line 22: Remove the dead code: delete the unused import isTauriDriver from the
import statement (keep supportsExecuteScript) and remove the unused
getProcessEnv helper function and its misleading comment (or implement it
properly if needed); ensure no references to isTauriDriver or getProcessEnv
remain in this spec (look for the import line and the getProcessEnv helper
definition) and run tests to verify nothing else depends on them.
In `@docs/BUILDING.md`:
- Around line 99-122: The "Option 1" and "Option 2" headings are using the same
level (###) as their parent "Running the ARM binary", flattening the hierarchy;
change those two headings to a deeper level (use #### for the "Option 1 — Direct
invocation" and "Option 2 — Wrapper script (recommended)" headings) so they are
nested under the "Running the ARM binary" section and the TOC/heading structure
is correct.
🪄 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: 59cfe95b-1d2b-4a80-a061-362d0dcda458
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.github/workflows/build-windows.yml.github/workflows/build.yml.github/workflows/e2e-agent-review.yml.github/workflows/release.yml.github/workflows/test.yml.github/workflows/typecheck.yml.husky/pre-pushapp/scripts/e2e-run-spec.shapp/src-tauri/tauri.conf.jsonapp/src/components/settings/panels/AIPanel.tsxapp/src/store/threadSlice.tsapp/test/e2e/specs/linux-cef-deb-runtime.spec.tsdocs/BUILDING.mdpackage.jsonpackages/deb/control.intests/linux_cef_deb_runtime_e2e.rstests/webview_apis_bridge.rs
✅ Files skipped from review due to trivial changes (2)
- packages/deb/control.in
- tests/webview_apis_bridge.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src-tauri/tauri.conf.json
- .husky/pre-push
- app/scripts/e2e-run-spec.sh
Comment on lines
+36
to
+50
| - name: Setup pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| cache: true | ||
|
|
||
| - name: Cache node modules | ||
| id: yarn-cache | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: node_modules | ||
| key: ${{ runner.os }}-test-${{ hashFiles('**/yarn.lock') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-test- | ||
| ${{ runner.os }}-build- | ||
| - name: Setup Node.js 24.x | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 24.x | ||
|
|
||
| - name: Install dependencies | ||
| if: steps.yarn-cache.outputs.cache-hit != 'true' | ||
| run: yarn install --frozen-lockfile | ||
| - name: Install dependencies | ||
| run: pnpm install --frozen-lockfile | ||
|
|
||
| - name: Run tests with coverage | ||
| run: yarn test:coverage | ||
| run: pnpm test:coverage |
Contributor
There was a problem hiding this comment.
Broken YAML indentation — workflow will fail to parse.
- name: Setup pnpm (36), - name: Setup Node.js 24.x (41), and - name: Install dependencies (46) are under-indented (0 or 4 spaces) relative to the job's steps: list where sibling steps use 6 spaces for - name: and 8 for step keys. YAMLlint flags line 36; GitHub Actions will reject the entire file — the unit-tests job and the rust-tests job both fail to run.
🔧 Proposed fix
-- name: Setup pnpm
- uses: pnpm/action-setup@v4
- with:
- cache: true
-
- - name: Setup Node.js 24.x
- uses: actions/setup-node@v4
- with:
- node-version: 24.x
-
- - name: Install dependencies
- run: pnpm install --frozen-lockfile
+ - name: Setup pnpm
+ uses: pnpm/action-setup@v4
+ with:
+ cache: true
+
+ - name: Setup Node.js 24.x
+ uses: actions/setup-node@v4
+ with:
+ node-version: 24.x
+
+ - name: Install dependencies
+ run: pnpm install --frozen-lockfile🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 36-36: syntax error: expected , but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yml around lines 36 - 50, The YAML workflow has
mis-indented step entries for the steps named "Setup pnpm", "Setup Node.js
24.x", "Install dependencies" and "Run tests with coverage"; fix by aligning
their indentation with the other steps under the job's steps: ensure each "-
name:" line uses the same leading spaces as sibling steps (e.g., 6 spaces before
"- name:") and the step keys under them (uses:, with:, run:) are indented one
level deeper (e.g., 8 spaces) so the "unit-tests" and "rust-tests" jobs parse
correctly.
Comment on lines
26
to
52
| - name: Setup pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| cache: true | ||
|
|
||
| - name: Setup Node.js 24.x | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 24.x | ||
| cache: "yarn" | ||
|
|
||
| - name: Cache node modules | ||
| id: yarn-cache | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: node_modules | ||
| key: ${{ runner.os }}-typecheck-${{ hashFiles('**/yarn.lock') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-typecheck- | ||
| ${{ runner.os }}-test- | ||
| ${{ runner.os }}-build- | ||
|
|
||
| - name: Install dependencies | ||
| if: steps.yarn-cache.outputs.cache-hit != 'true' | ||
| run: yarn install --frozen-lockfile | ||
| - name: Install dependencies | ||
| run: pnpm install --frozen-lockfile | ||
|
|
||
| - name: Type check TypeScript files | ||
| run: yarn workspace openhuman-app compile | ||
| run: pnpm --filter openhuman-app compile | ||
| env: | ||
| NODE_ENV: test | ||
|
|
||
| - name: Check Prettier formatting | ||
| run: yarn workspace openhuman-app format:check | ||
| run: pnpm --filter openhuman-app format:check | ||
| env: | ||
| NODE_ENV: test | ||
|
|
||
| - name: Run ESLint | ||
| run: yarn workspace openhuman-app lint | ||
| - name: Run ESLint | ||
| run: pnpm --filter openhuman-app lint | ||
| env: | ||
| NODE_ENV: test |
Contributor
There was a problem hiding this comment.
Broken YAML indentation across three steps — workflow will fail to parse.
- name: Setup pnpm (26), - name: Install dependencies (36), and - name: Run ESLint (49) are all at column 0 instead of 6 spaces. Their child keys are at 6 spaces instead of 8, and under Run ESLint the mismatch between run: (6 spaces) and env: (8 spaces) compounds it. YAMLlint flags line 26 and GitHub Actions will reject the file — neither typecheck nor rust-quality jobs can run.
🔧 Proposed fix
-- name: Setup pnpm
- uses: pnpm/action-setup@v4
- with:
- cache: true
+ - name: Setup pnpm
+ uses: pnpm/action-setup@v4
+ with:
+ cache: true
- name: Setup Node.js 24.x
uses: actions/setup-node@v4
with:
node-version: 24.x
-- name: Install dependencies
- run: pnpm install --frozen-lockfile
+ - name: Install dependencies
+ run: pnpm install --frozen-lockfile
- name: Type check TypeScript files
run: pnpm --filter openhuman-app compile
env:
NODE_ENV: test
- name: Check Prettier formatting
run: pnpm --filter openhuman-app format:check
env:
NODE_ENV: test
-- name: Run ESLint
- run: pnpm --filter openhuman-app lint
+ - name: Run ESLint
+ run: pnpm --filter openhuman-app lint
env:
NODE_ENV: test🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 26-26: syntax error: expected , but found '-'
(syntax)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/typecheck.yml around lines 26 - 52, The YAML has broken
indentation for the steps named "Setup pnpm", "Install dependencies", and "Run
ESLint": align each "- name: Setup pnpm", "- name: Install dependencies", and "-
name: Run ESLint" to the same indentation level as other step entries and ensure
their child keys ("uses", "with", "run", "env") are indented exactly two spaces
further than the "- name" line; also fix the "Run ESLint" block so "env:" is
aligned with "run:" (same indentation) to resolve the parsing error.
Comment on lines
+1
to
+18
| /** | ||
| * E2E: Linux CEF deb package runtime changes (PR #3) | ||
| * | ||
| * Tests the cross-process behavior: | ||
| * - UI → Tauri `core_rpc_url` command → sidecar binary resolution | ||
| * - Core binary path probing: env override → packaged Linux locations → fallback | ||
| * - Tray setup on linux+cef (skipped without panicking) | ||
| * - Grep-friendly logging patterns for diagnostics | ||
| * | ||
| * This spec validates that the Linux .deb package can find openhuman-core | ||
| * in system paths like /usr/bin/openhuman-core when installed via .deb. | ||
| * | ||
| * Coverage: | ||
| * - core_process::default_core_bin() resolution paths | ||
| * - setup_tray() conditional compilation for linux+cef | ||
| * - Tauri command: core_rpc_url | ||
| * - Sidecar JSON-RPC connectivity | ||
| */ |
Contributor
There was a problem hiding this comment.
Update stale "PR #3" references.
The module-level docblock (and comments at Lines 291–294) references "PR #3", but this work is landing as PR #886. Once rebased the bare PR number will read as a dangling reference; prefer a short human description ("Linux CEF .deb runtime: core binary resolution + tray gating") rather than a PR number that won't be meaningful after merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/test/e2e/specs/linux-cef-deb-runtime.spec.ts` around lines 1 - 18, Update
the module-level docblock and any other inline comments that reference "PR `#3`"
(including the block near the end of the file that repeats the same text) to use
a short human description instead of the PR number; replace occurrences of "PR
`#3`" with "Linux CEF .deb runtime: core binary resolution + tray gating" so the
comment remains meaningful after rebases/merges (search for the top-of-file
docblock and the trailing comment block that echoes the PR reference to locate
the exact spots to change).
Comment on lines
+312
to
+324
| it('sidecar is running with non-default port when OPENHUMAN_CORE_PORT is set', async () => { | ||
| // When OPENHUMAN_CORE_PORT is set, the sidecar should use that port | ||
| // This verifies env var propagation to the sidecar | ||
|
|
||
| const result = await invokeTauriCommand<string>('core_rpc_url'); | ||
| expect(result.ok).toBe(true); | ||
|
|
||
| const url = result.result!; | ||
| stepLog('Sidecar RPC URL', { url }); | ||
|
|
||
| // URL should be well-formed | ||
| expect(url).toMatch(/^http:\/\/.+:\d+\/rpc$/); | ||
| }); |
Contributor
There was a problem hiding this comment.
Test description promises env var verification but doesn't perform any.
"sidecar is running with non-default port when OPENHUMAN_CORE_PORT is set" implies this test sets or verifies OPENHUMAN_CORE_PORT, but the body only format-checks the RPC URL — identical to what core_rpc_url Tauri command returns valid RPC URL already covers (Lines 111–126). As written, this is effectively a duplicate of the earlier test with a misleading name.
Either set/propagate OPENHUMAN_CORE_PORT before the app boots and assert the URL reflects that port, or rename the test to describe what it actually verifies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/test/e2e/specs/linux-cef-deb-runtime.spec.ts` around lines 312 - 324, The
test "sidecar is running with non-default port when OPENHUMAN_CORE_PORT is set"
currently only calls invokeTauriCommand('core_rpc_url') and checks URL format,
so update it to actually verify OPENHUMAN_CORE_PORT: either (A) set the env var
before the app/sidecar starts (e.g., export or pass OPENHUMAN_CORE_PORT into the
test harness/startup so the sidecar boots with that port) and then call
invokeTauriCommand('core_rpc_url') and assert the returned URL contains the
specific port you set (match /http:\/\/.+:EXPECTED_PORT\/rpc/), or (B) if you
don't want to control startup env here, rename the test to accurately reflect
that it only validates a well-formed RPC URL (change the test title and keep the
invokeTauriCommand('core_rpc_url') assertion). Ensure you reference the test
name and the invokeTauriCommand('core_rpc_url') call when making the change.
Comment on lines
+64
to
+132
| /// Test that OPENHUMAN_CORE_BIN override takes precedence when file exists. | ||
| #[test] | ||
| fn core_bin_env_override_takes_precedence_when_exists() { | ||
| let temp_dir = std::env::temp_dir().join("openhuman-core-test-override"); | ||
| let _ = fs::remove_dir_all(&temp_dir); | ||
| fs::create_dir_all(&temp_dir).expect("create temp dir"); | ||
|
|
||
| // Create a fake core binary | ||
| let fake_core = create_fake_core_binary(&temp_dir, "openhuman-core"); | ||
| let fake_core_str = fake_core.to_str().unwrap(); | ||
|
|
||
| // Set the env override | ||
| let _guard = EnvGuard::set("OPENHUMAN_CORE_BIN", fake_core_str); | ||
|
|
||
| // Import and call the function from the tauri crate | ||
| // We can't directly import from src-tauri, but we verify the behavior | ||
| // by checking that the env var is set and file exists | ||
| assert!(fake_core.exists(), "Fake core binary should exist"); | ||
| assert_eq!( | ||
| std::env::var("OPENHUMAN_CORE_BIN").ok().as_deref(), | ||
| Some(fake_core_str) | ||
| ); | ||
|
|
||
| // Cleanup | ||
| let _ = fs::remove_dir_all(&temp_dir); | ||
| } | ||
|
|
||
| /// Test that OPENHUMAN_CORE_BIN override gracefully handles non-existent files. | ||
| #[test] | ||
| fn core_bin_env_override_graceful_when_nonexistent() { | ||
| // Set env override to a non-existent path | ||
| let _guard = EnvGuard::set("OPENHUMAN_CORE_BIN", "/nonexistent/path/openhuman-core"); | ||
|
|
||
| // Verify the env var is set | ||
| assert_eq!( | ||
| std::env::var("OPENHUMAN_CORE_BIN").ok().as_deref(), | ||
| Some("/nonexistent/path/openhuman-core") | ||
| ); | ||
|
|
||
| // Verify the file doesn't exist | ||
| assert!(!std::path::Path::new("/nonexistent/path/openhuman-core").exists()); | ||
| } | ||
|
|
||
| /// Test packaged Linux paths are probed in correct order. | ||
| #[test] | ||
| fn core_bin_packaged_linux_paths_order() { | ||
| // Document the expected search order for packaged Linux binaries | ||
| let expected_paths = [ | ||
| "/usr/bin/openhuman-core", | ||
| "/usr/lib/OpenHuman/openhuman-core", | ||
| ]; | ||
|
|
||
| // Verify these are valid absolute paths | ||
| for path in &expected_paths { | ||
| let p = std::path::Path::new(path); | ||
| assert!(p.is_absolute(), "Path should be absolute: {}", path); | ||
| assert!( | ||
| path.contains("openhuman-core"), | ||
| "Path should contain 'openhuman-core': {}", | ||
| path | ||
| ); | ||
| } | ||
|
|
||
| // Log the expected search order for documentation | ||
| println!("Packaged Linux core binary search order:"); | ||
| for (i, path) in expected_paths.iter().enumerate() { | ||
| println!(" {}. {}", i + 1, path); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Several tests are tautological — they don't exercise the production code paths they claim to cover.
A number of tests here read as self-checks on literals the test itself just wrote:
core_bin_env_override_takes_precedence_when_exists(Lines 66–89) setsOPENHUMAN_CORE_BINand then assertsstd::env::var("OPENHUMAN_CORE_BIN")returns what was just set — it never callsresolve_daemon_executablefromsrc/openhuman/service/common.rs, which is the actual behavior under test.core_bin_packaged_linux_paths_order(Lines 108–132) hard-codes/usr/bin/openhuman-core//usr/lib/OpenHuman/openhuman-coreand asserts they look like absolute paths. It is not connected to the real probe list in the binary-resolution code; if someone changes the real probe order, this test will still pass.core_port_env_configuration(Lines 135–156) re-implements the port-parsing logic locally instead of callingcore_port()fromsrc/core/jsonrpc.rs, so it only testsu16::parse.core_rpc_url_format(Lines 160–180) formats strings and compares them to the same strings.core_rpc_url_env_override(Lines 183–193) just round-trips an env var.
For this suite to offer real regression protection, expose the resolver / port / URL helpers through a public (or pub(crate)) re-export that integration tests can call, and invoke them directly. Otherwise the coverage numbers are misleading and a refactor that breaks the real behavior will ship green.
Also applies to: 158-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/linux_cef_deb_runtime_e2e.rs` around lines 64 - 132, The tests are
tautological because they assert values they set instead of invoking the real
resolution/parsing helpers; make the actual functions used by production code
callable from tests (e.g. mark resolve_daemon_executable in
src/openhuman/service/common.rs, core_port in src/core/jsonrpc.rs, and whatever
builds the RPC URL—e.g. core_rpc_url—as pub(crate) or expose them via a small
pub re-export module) and update each test to call those functions directly
(replace environment-only assertions with calls to resolve_daemon_executable,
core_port, and core_rpc_url and assert their outputs), ensuring the packaged
probe list used in tests is read from the same constant/array used by
resolve_daemon_executable instead of hard-coding paths.
Comment on lines
+66
to
+105
| fn core_bin_env_override_takes_precedence_when_exists() { | ||
| let temp_dir = std::env::temp_dir().join("openhuman-core-test-override"); | ||
| let _ = fs::remove_dir_all(&temp_dir); | ||
| fs::create_dir_all(&temp_dir).expect("create temp dir"); | ||
|
|
||
| // Create a fake core binary | ||
| let fake_core = create_fake_core_binary(&temp_dir, "openhuman-core"); | ||
| let fake_core_str = fake_core.to_str().unwrap(); | ||
|
|
||
| // Set the env override | ||
| let _guard = EnvGuard::set("OPENHUMAN_CORE_BIN", fake_core_str); | ||
|
|
||
| // Import and call the function from the tauri crate | ||
| // We can't directly import from src-tauri, but we verify the behavior | ||
| // by checking that the env var is set and file exists | ||
| assert!(fake_core.exists(), "Fake core binary should exist"); | ||
| assert_eq!( | ||
| std::env::var("OPENHUMAN_CORE_BIN").ok().as_deref(), | ||
| Some(fake_core_str) | ||
| ); | ||
|
|
||
| // Cleanup | ||
| let _ = fs::remove_dir_all(&temp_dir); | ||
| } | ||
|
|
||
| /// Test that OPENHUMAN_CORE_BIN override gracefully handles non-existent files. | ||
| #[test] | ||
| fn core_bin_env_override_graceful_when_nonexistent() { | ||
| // Set env override to a non-existent path | ||
| let _guard = EnvGuard::set("OPENHUMAN_CORE_BIN", "/nonexistent/path/openhuman-core"); | ||
|
|
||
| // Verify the env var is set | ||
| assert_eq!( | ||
| std::env::var("OPENHUMAN_CORE_BIN").ok().as_deref(), | ||
| Some("/nonexistent/path/openhuman-core") | ||
| ); | ||
|
|
||
| // Verify the file doesn't exist | ||
| assert!(!std::path::Path::new("/nonexistent/path/openhuman-core").exists()); | ||
| } |
Contributor
There was a problem hiding this comment.
Env-var tests race under Cargo's default parallel test runner.
cargo test runs tests from the same binary on a shared thread pool by default, and the environment is a single process-global resource. Several tests in this file mutate overlapping vars:
core_bin_env_override_takes_precedence_when_existssetsOPENHUMAN_CORE_BINcore_bin_env_override_graceful_when_nonexistentsetsOPENHUMAN_CORE_BINcore_port_env_configurationsets and unsetsOPENHUMAN_CORE_PORTcore_rpc_url_env_overridesetsOPENHUMAN_CORE_RPC_URL
These will interleave: one test's EnvGuard::drop can fire between another test's set and its read, producing flaky failures. The fake-binary paths also collide because create_fake_core_binary uses a fixed name under std::env::temp_dir() — two runs/tests hitting the same directory will trample each other.
Recommended fixes:
- Serialize all env-mutating tests behind a single
Mutex(e.g.,once_cell::sync::Lazy<Mutex<()>>) and acquire it before creating theEnvGuard, or use theserial_testcrate. - Use
tempfile::tempdir()(unique per test) instead of a hard-coded path undertemp_dir().
Also applies to: 136-156, 184-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/linux_cef_deb_runtime_e2e.rs` around lines 66 - 105, Tests that mutate
environment vars (core_bin_env_override_takes_precedence_when_exists,
core_bin_env_override_graceful_when_nonexistent, core_port_env_configuration,
core_rpc_url_env_override) race under parallel test execution and also collide
on the fixed temp dir used by create_fake_core_binary; fix by serializing
env-mutating tests with a global Mutex (e.g., define static TEST_ENV_LOCK:
once_cell::sync::Lazy<std::sync::Mutex<()>> and lock it at the start of each
test before calling EnvGuard::set) and replace the fixed std::env::temp_dir()
usage in create_fake_core_binary/test setup with tempfile::tempdir() (unique per
test) so fake binaries do not collide; apply the same locking and tempfile
changes to the other tests mentioned (lines ~136-156 and ~184-193).
Comment on lines
+233
to
+254
| /// Test that tray setup on linux+cef is properly gated. | ||
| #[test] | ||
| fn tray_setup_linux_cef_gate() { | ||
| // Document the conditional compilation behavior: | ||
| // - On linux + cef: setup_tray() logs a warning and returns Ok(()) | ||
| // - On other platforms: setup_tray() creates the actual tray | ||
|
|
||
| // This is compile-time gated via #[cfg] attributes | ||
| // We document the expected behavior here | ||
|
|
||
| let is_linux = cfg!(target_os = "linux"); | ||
| let has_cef_feature = false; // Would be cfg!(feature = "cef") in actual code | ||
|
|
||
| if is_linux && has_cef_feature { | ||
| println!("On linux+cef: setup_tray() should log warning and skip tray creation"); | ||
| } else { | ||
| println!("On other platforms: setup_tray() should create tray normally"); | ||
| } | ||
|
|
||
| // The actual test is that this compiles and doesn't panic | ||
| assert!(true); | ||
| } |
Contributor
There was a problem hiding this comment.
assert!(true) is a no-op; drop this test or make it assert something real.
The whole body only prints a string based on a hard-coded has_cef_feature = false. The comment says "the actual test is that this compiles and doesn't panic" — but every test in Rust satisfies that, so the assertion adds nothing. If the intent is to cover the setup_tray() cfg-gated behavior, it needs to actually invoke setup_tray() under both cfg(feature = "cef") paths (or be deleted).
♻️ Minimal fix
- // The actual test is that this compiles and doesn't panic
- assert!(true);
+ // TODO: exercise setup_tray() under cfg(all(target_os = "linux", feature = "cef"))
+ // once it is callable from this integration test.📝 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
| /// Test that tray setup on linux+cef is properly gated. | |
| #[test] | |
| fn tray_setup_linux_cef_gate() { | |
| // Document the conditional compilation behavior: | |
| // - On linux + cef: setup_tray() logs a warning and returns Ok(()) | |
| // - On other platforms: setup_tray() creates the actual tray | |
| // This is compile-time gated via #[cfg] attributes | |
| // We document the expected behavior here | |
| let is_linux = cfg!(target_os = "linux"); | |
| let has_cef_feature = false; // Would be cfg!(feature = "cef") in actual code | |
| if is_linux && has_cef_feature { | |
| println!("On linux+cef: setup_tray() should log warning and skip tray creation"); | |
| } else { | |
| println!("On other platforms: setup_tray() should create tray normally"); | |
| } | |
| // The actual test is that this compiles and doesn't panic | |
| assert!(true); | |
| } | |
| /// Test that tray setup on linux+cef is properly gated. | |
| #[test] | |
| fn tray_setup_linux_cef_gate() { | |
| // Document the conditional compilation behavior: | |
| // - On linux + cef: setup_tray() logs a warning and returns Ok(()) | |
| // - On other platforms: setup_tray() creates the actual tray | |
| // This is compile-time gated via #[cfg] attributes | |
| // We document the expected behavior here | |
| let is_linux = cfg!(target_os = "linux"); | |
| let has_cef_feature = false; // Would be cfg!(feature = "cef") in actual code | |
| if is_linux && has_cef_feature { | |
| println!("On linux+cef: setup_tray() should log warning and skip tray creation"); | |
| } else { | |
| println!("On other platforms: setup_tray() should create tray normally"); | |
| } | |
| // TODO: exercise setup_tray() under cfg(all(target_os = "linux", feature = "cef")) | |
| // once it is callable from this integration test. | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/linux_cef_deb_runtime_e2e.rs` around lines 233 - 254, The test
tray_setup_linux_cef_gate currently does nothing useful (assert!(true)); replace
it with an actual invocation of setup_tray() under the appropriate cfg branches:
call setup_tray() inside a #[cfg(all(target_os = "linux", feature = "cef"))]
block and assert it returns Ok(()) and (optionally) that a warning log was
emitted, and call setup_tray() inside a #[cfg(not(all(target_os = "linux",
feature = "cef")))] block and assert it returns Ok(()) and that a tray was
created (or the expected non-warning behavior). Locate the test function
tray_setup_linux_cef_gate and the setup_tray() function to implement these
conditional calls and proper assertions so the test actually verifies the gated
behavior instead of being a no-op.
Comment on lines
+325
to
+351
| /// Test that the logging patterns are grep-friendly. | ||
| #[test] | ||
| fn logging_patterns_are_grep_friendly() { | ||
| // Document the expected log patterns that should appear in the logs | ||
| let expected_patterns = [ | ||
| "[core] default_core_bin:", | ||
| "[core] spawning dedicated core binary:", | ||
| "[core] core process ready", | ||
| "[tray] deferring tray setup to RunEvent::Ready", | ||
| ]; | ||
|
|
||
| for pattern in &expected_patterns { | ||
| // Verify patterns are stable and contain expected prefixes | ||
| assert!( | ||
| pattern.starts_with('['), | ||
| "Log pattern should start with bracketed category: {}", | ||
| pattern | ||
| ); | ||
| assert!( | ||
| pattern.contains('[') && pattern.contains(']'), | ||
| "Log pattern should have [category] format: {}", | ||
| pattern | ||
| ); | ||
|
|
||
| println!("Grep-friendly pattern: {}", pattern); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which log prefixes are actually emitted for the core/tray paths these tests document.
rg -nP '\[(core|tray)\][^"]*' --type=rust -g '!tests/**'Repository: tinyhumansai/openhuman
Length of output: 6026
🏁 Script executed:
cat -n app/test/e2e/specs/linux-cef-deb-runtime.spec.ts | sed -n '290,300p'Repository: tinyhumansai/openhuman
Length of output: 728
🏁 Script executed:
# Check what [tray] patterns exist in the actual TypeScript/Rust source
rg -n '\[tray\]' --type=rust app/src-tauri/src/ | head -20Repository: tinyhumansai/openhuman
Length of output: 984
🏁 Script executed:
# Search for any mention of "default_core_bin" in the source
rg -i 'default_core_bin' --type=rust src/ app/Repository: tinyhumansai/openhuman
Length of output: 492
🏁 Script executed:
# Search for "deferring tray setup" pattern
rg 'deferring.*tray' --type=rustRepository: tinyhumansai/openhuman
Length of output: 159
Pattern mismatch between tests and actual log emitters needs resolution.
The test documents "[core] default_core_bin:", "[core] spawning dedicated core binary:", "[core] core process ready", and "[tray] deferring tray setup to RunEvent::Ready", but verification found:
- ✓
"[core] spawning dedicated core binary:"and"[core] core process ready"exist in source - ✗
"[core] default_core_bin:"has no log emitter (the function exists but logs nothing with this pattern) - ✗
"[tray] deferring tray setup to RunEvent::Ready"appears only in the test, not in actual source
Additionally, the companion TS spec (app/test/e2e/specs/linux-cef-deb-runtime.spec.ts, lines 292–295) documents yet different patterns ("[tray] skipping tray setup on linux+cef"), creating a three-way mismatch.
Since neither test calls the real logger and the documented patterns drift from actual source emitters, on-call engineers grepping production logs will hit empty results.
Either:
- Make assertions meaningful by reading actual source (e.g.,
include_str!("../app/src-tauri/src/core_process.rs")and asserting the patterns exist there), or - Remove the test and document these patterns as inline comments on the actual
log::*!macros that emit them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/linux_cef_deb_runtime_e2e.rs` around lines 325 - 351, The test
logging_patterns_are_grep_friendly documents log strings that don't match actual
emitters (e.g., "[core] default_core_bin:" and "[tray] deferring tray setup to
RunEvent::Ready" are absent), so either make the test assert against the real
source or remove it and document patterns at the emission sites: update the test
to read the emitter source (use
include_str!("../app/src-tauri/src/core_process.rs") or the appropriate Rust
file) and assert the expected patterns exist in that string (referencing the
log! / trace! / info! macros in core_process.rs and the RunEvent::Ready
handling), or delete logging_patterns_are_grep_friendly and add inline comments
next to the actual log::... macros that emit the grep-friendly patterns (and
sync the companion TS spec strings such as "[tray] skipping tray setup on
linux+cef" to match the chosen source of truth).
VectorJet
force-pushed
the
pnpm-migration-squash
branch
from
11da091 to
34229fa
Compare
This commit migrates the entire monorepo from yarn to pnpm: - Replace yarn with pnpm in all GitHub Actions workflows - Update husky pre-push hooks to use pnpm - Update all app scripts from yarn to pnpm - Add pnpm-workspace.yaml for workspace configuration - Generate pnpm-lock.yaml lockfiles - Update tauri.conf.json to use pnpm exec - Fix typecheck and lint issues - Guard tray icon setup in headless CI environments - Update documentation (BUILDING.md, review scripts README) - Add ARM Linux build instructions and GTK init fix
- Update frozen lockfile policy in CI workflows (remove --no-frozen-lockfile) - Fix ESLint to fail on errors (remove || true) - Update Debian dependencies (libwebkit2gtk-4.1-0, libgdk-pixbuf-2.0-0) - Update CLI package dependencies to actual binary requirements - Memoize loadAIPreview and loadLocalAiStatus with useCallback - Add SAFETY comment for env::set_var in test - Fix NODE_OPTIONS inconsistency in release workflow - Update commented e2e-macos job to use pnpm - Remove hardcoded pnpm version from workflows (auto-detect from packageManager) - Use msg_ UUID prefix in threadSlice addInferenceResponse - Split BUILDING.md bash blocks and fix LD_LIBRARY_PATH - Add corepack integrity hash to packageManager field - Add error logging to webview_apis_bridge test - Fix release.yml comments about Vite build process - Update e2e-run-spec.sh error messages to reference pnpm - Fix .husky/pre-push to use pnpm instead of yarn
VectorJet
force-pushed
the
pnpm-migration-squash
branch
from
34229fa to
7c2ff51
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/src/pages/Conversations.tsx (1)
410-417:⚠️ Potential issue | 🟡 MinorMirror the defensive fallback used in
threadSlice.tsforcrypto.randomUUID().The sibling producer in
app/src/store/threadSlice.ts(addInferenceResponse, lines 124-160) intentionally guards this exact call:`msg_${globalThis.crypto?.randomUUID ? globalThis.crypto.randomUUID() : `${Date.now()}-${Math.random().toString(36).slice(2)}`}`This direct
globalThis.crypto.randomUUID()here drops that guard.Web Crypto.randomUUIDis only defined in secure contexts — it's fine in the Tauri webview and onlocalhost, but will throwTypeErroron plain-http://hosts (e.g. LAN preview builds) and in older test runners / jsdom versions that don't expose it. If that throws,handleSendMessageblows up beforedispatch(addMessageLocal(...))ever runs, so the entire send flow is dead rather than degrading.Either match the fallback in
threadSlice.tsfor consistency, or extract a shared helper so both callers stay in lockstep.🛡️ Proposed fix
const sendingThreadId = selectedThreadId; const userMessage: ThreadMessage = { - id: `msg_${globalThis.crypto.randomUUID()}`, + id: `msg_${ + globalThis.crypto?.randomUUID + ? globalThis.crypto.randomUUID() + : `${Date.now()}-${Math.random().toString(36).slice(2)}` + }`, content: trimmed,Better still, pull this into a small helper (e.g.
app/src/utils/ids.tsexportingnewMessageId()) and use it from bothConversations.tsxandthreadSlice.tsto remove the duplicated fallback logic.🤖 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 410 - 417, The current creation of userMessage.id in Conversations.tsx uses globalThis.crypto.randomUUID() without the fallback used in threadSlice.ts (addInferenceResponse), which can throw in non-secure or older environments; update Conversations.tsx to either use the same guarded expression used in addInferenceResponse or, better, extract a shared helper (e.g., newMessageId()) in a new utils/ids module and call that from both Conversations.tsx (where userMessage is built) and threadSlice.ts (addInferenceResponse) so both locations use the identical fallback implementation.app/scripts/e2e-run-spec.sh (1)
114-123:⚠️ Potential issue | 🟠 MajorDuplicated grep block — leftover yarn reference was not deleted during the migration.
Lines 114–118 and 119–123 check the exact same condition (
grep -q "127.0.0.1:${E2E_MOCK_PORT}" "$DIST_JS"). The first (new) block printspnpm test:e2e:build; the second (old) block still printsyarn test:e2e:build. The intent per the PR summary was to consolidate into a single pnpm check, but the yarn block was left behind.Impact: if the mock URL is missing, the script exits from the first block and the user sees the correct pnpm hint. But any reader of this script still sees a yarn-based hint, and the second
grepis dead work on every normal run.🔧 Proposed fix — remove the stale yarn block
if ! grep -q "127.0.0.1:${E2E_MOCK_PORT}" "$DIST_JS"; then echo "ERROR: frontend bundle does NOT contain mock server URL (127.0.0.1:${E2E_MOCK_PORT})." >&2 echo " Run 'pnpm test:e2e:build' to rebuild with the mock URL." >&2 exit 1 fi -if ! grep -q "127.0.0.1:${E2E_MOCK_PORT}" "$DIST_JS"; then - echo "ERROR: frontend bundle does NOT contain mock server URL (127.0.0.1:${E2E_MOCK_PORT})." >&2 - echo " Run 'yarn test:e2e:build' to rebuild with the mock URL." >&2 - exit 1 -fi echo "Verified: frontend bundle contains mock server URL."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/scripts/e2e-run-spec.sh` around lines 114 - 123, Remove the duplicated grep block that checks for "127.0.0.1:${E2E_MOCK_PORT}" in "$DIST_JS" – keep the first block that prints the pnpm suggestion and delete the second (stale yarn) block so the script only performs the check once; look for the duplicated grep condition using the E2E_MOCK_PORT and DIST_JS variables and remove the block that references "yarn test:e2e:build".docs/BUILDING.md (1)
153-179:⚠️ Potential issue | 🟡 MinorStale
yarnreferences in the troubleshooting section.The rest of the doc was migrated to pnpm, but the "CEF cache is held" troubleshooting block at lines 153, 157, and 178 still instructs users to run
yarn dev:app. After this PR lands, that command won't resolve (root-levelpackage.jsonno longer runs yarn, and thedev:appscript only lives inapp/package.json).🔧 Proposed fix
-### macOS: `yarn dev:app` exits with "CEF cache is held by another OpenHuman instance" +### macOS: `pnpm dev:app` exits with "CEF cache is held by another OpenHuman instance" … -`yarn dev:app` (or any debug build of the Tauri shell) exits before the window appears with a message like: +`pnpm dev:app` (or any debug build of the Tauri shell) exits before the window appears with a message like: … pkill -f "OpenHuman.app/Contents" pkill -f "openhuman-core" -yarn dev:app +pnpm dev:app🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/BUILDING.md` around lines 153 - 179, Update the troubleshooting text that currently tells users to run "yarn dev:app" (references to the dev:app script) to the pnpm equivalent used by the repo; replace each "yarn dev:app" occurrence with "pnpm --filter app run dev:app" (or the repo's pnpm script invocation) so developers run the app package's dev:app script correctly; keep the existing pkill workaround lines unchanged and ensure any other "yarn" mentions in this CEF cache section are similarly converted to pnpm..github/workflows/build.yml (1)
79-124:⚠️ Potential issue | 🔴 CriticalUnresolved git merge conflict markers — workflow is invalid YAML.
Lines 79 (
<<<<<<< HEAD) and 119 (=======) are raw conflict markers left over from squashing the 28-commit fork. The two env blocks on either side are identical (CARGO_PROFILE_RELEASE_OPT_LEVEL,_CODEGEN_UNITS,_LTO,_STRIP,_DEBUG), so both sides of the conflict were accepted. actionlint and YAMLlint both confirm this fails to parse, which means the entire Build job (and anything else in this file) will not run — CI effectively goes dark on this workflow.🔧 Proposed fix — drop markers and dedupe the env block
env: NODE_ENV: production -<<<<<<< HEAD CARGO_PROFILE_RELEASE_OPT_LEVEL: '1' CARGO_PROFILE_RELEASE_CODEGEN_UNITS: '16' CARGO_PROFILE_RELEASE_LTO: 'false' CARGO_PROFILE_RELEASE_STRIP: 'true' CARGO_PROFILE_RELEASE_DEBUG: 'false' - name: Upload Rust debug symbols to Sentry … sentry-cli releases finalize "${RELEASE}" echo "Sentry symbol upload complete for ${RELEASE}" -======= - CARGO_PROFILE_RELEASE_OPT_LEVEL: '1' - CARGO_PROFILE_RELEASE_CODEGEN_UNITS: '16' - CARGO_PROFILE_RELEASE_LTO: 'false' - CARGO_PROFILE_RELEASE_STRIP: 'true' - CARGO_PROFILE_RELEASE_DEBUG: 'false' +# (also remove the closing `>>>>>>> …` marker further down if present)#!/bin/bash # Confirm there are no leftover conflict markers anywhere in this PR's workflows/config. rg -nP '^(<{7}|={7}|>{7})( |$)' .github/ app/ packages/ 2>/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 79 - 124, Remove the leftover git merge conflict markers (e.g. '<<<<<<< HEAD', '=======', '>>>>>>>') from the .github/workflows/build.yml and dedupe the duplicated environment block (CARGO_PROFILE_RELEASE_OPT_LEVEL, CARGO_PROFILE_RELEASE_CODEGEN_UNITS, CARGO_PROFILE_RELEASE_LTO, CARGO_PROFILE_RELEASE_STRIP, CARGO_PROFILE_RELEASE_DEBUG) so the YAML parses; ensure only a single clean env block remains and no conflict markers are present anywhere in the file..github/workflows/typecheck.yml (1)
27-50:⚠️ Potential issue | 🔴 CriticalMigrate
typecheck.ymlto pnpm — this job will break CI after the package-manager switch.The repository now has both
pnpm-lock.yamlandpnpm-workspace.yamlalongsideyarn.lock. Thebuild.ymlworkflow correctly usespnpm/action-setup@v4, buttypecheck.ymlstill pins every step to yarn:
- Line 27:
cache: yarn- Lines 29–37:
yarn-cachestep hashed againstyarn.lock- Line 40:
yarn install --frozen-lockfile- Lines 42, 46, 50:
yarn workspace openhuman-app …— this is yarn-workspace syntax; pnpm workspaces usepnpm --filter openhuman-app …insteadThe same issues affect
test.yml(lines 35–49) andbuild-windows.yml(lines 28, 70). All three workflows will fail to run Node-based checks becauseyarn workspacecommands do not work in a pnpm workspace context.🔧 Proposed fix — align with
build.yml's pnpm pattern- name: Setup Node.js 24.x uses: actions/setup-node@v4 with: node-version: 24.x - cache: yarn - - name: Cache node modules - id: yarn-cache - uses: actions/cache@v4 - with: - path: node_modules - key: ${{ runner.os }}-typecheck-${{ hashFiles('**/yarn.lock') }} - restore-keys: | - ${{ runner.os }}-typecheck- - ${{ runner.os }}-test- - ${{ runner.os }}-build- + - name: Setup pnpm + uses: pnpm/action-setup@v4 + with: + cache: true - name: Install dependencies - if: steps.yarn-cache.outputs.cache-hit != 'true' - run: yarn install --frozen-lockfile + run: pnpm install --frozen-lockfile - name: Type check TypeScript files - run: yarn workspace openhuman-app compile + run: pnpm --filter openhuman-app compile env: NODE_ENV: test - name: Check Prettier formatting - run: yarn workspace openhuman-app format:check + run: pnpm --filter openhuman-app format:check env: NODE_ENV: test - name: Run ESLint - run: yarn workspace openhuman-app lint + run: pnpm --filter openhuman-app lint env: NODE_ENV: testApply the same pattern to
test.ymlandbuild-windows.yml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/typecheck.yml around lines 27 - 50, The workflow still uses yarn-specific steps (cache: yarn, yarn-cache step keyed to yarn.lock, `yarn install --frozen-lockfile`, and `yarn workspace openhuman-app ...` commands) which must be migrated to pnpm; replace the cache key to use pnpm (and pnpm-lock.yaml), change the cache step id (e.g., yarn-cache → pnpm-cache) and set cache: pnpm, use pnpm/action-setup@v4 like build.yml, replace `yarn install --frozen-lockfile` with `pnpm install --frozen-lockfile`, and convert `yarn workspace openhuman-app compile|format:check|lint` to `pnpm --filter openhuman-app run compile|format:check|lint` (or `pnpm --filter openhuman-app exec` if appropriate); apply the exact same pattern to the other affected workflows (test.yml and build-windows.yml) so CI uses pnpm consistently.
🧹 Nitpick comments (1)
tests/webview_apis_bridge.rs (1)
43-126: Prefertokio::sync::oneshotover blockingstd::sync::mpsc::recv()insideasync fn.
ensure_mock_serverisasync, butport_rx.recv()on line 118 is a blocking call that parks the current Tokio worker thread until the dedicated OS thread sends the port. With#[tokio::test(flavor = "multi_thread", worker_threads = 2)]this happens to work (another worker can make progress), but it's an easy footgun — any future reuse of this helper from a single-threaded runtime, or a failure in the spawned thread beforesend, will deadlock the runtime rather than yield/propagate.A
tokio::sync::oneshotchannel.awaits correctly and surfaces sender-drop as an error.♻️ Suggested change
- let (port_tx, port_rx) = mpsc::channel(); + let (port_tx, port_rx) = tokio::sync::oneshot::channel::<u16>(); thread::spawn(move || { let runtime = tokio::runtime::Builder::new_multi_thread() ... runtime.block_on(async move { let listener = TcpListener::bind::<SocketAddr>("127.0.0.1:0".parse().unwrap()) .await .expect("bind"); let port = listener.local_addr().unwrap().port(); - port_tx.send(port).expect("send mock server port"); + let _ = port_tx.send(port); ... }); }); - let port = port_rx.recv().expect("receive mock server port"); + let port = port_rx.await.expect("receive mock server port");You can then drop the
use std::sync::mpsc;import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/webview_apis_bridge.rs` around lines 43 - 126, The helper ensure_mock_server currently uses std::sync::mpsc::channel and blocks on port_rx.recv(), which can deadlock a Tokio runtime; change the channel to tokio::sync::oneshot (create a oneshot::channel, send the port with the sender in the spawned thread, and await the receiver with .await instead of recv()), update the thread::spawn closure to call port_tx.send(port).expect(...) using the oneshot sender, and remove the blocking recv usage (and the std::sync::mpsc import) so the async function awaits the port properly and handles sender-drop errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/webview_apis_bridge.rs`:
- Around line 65-115: The block starting at the WebSocket server loop has
inconsistent indentation (e.g., the loop handling listener.accept(),
tokio::spawn(...) async move, and the ws.split()/sink.send loop) which will be
rewritten by cargo fmt; fix this by running cargo fmt on
tests/webview_apis_bridge.rs (or reformat the async block so indentation matches
the surrounding runtime.block_on(async move { … }) 4-space style) and ensure the
nested expressions (listener.accept, tokio_tungstenite::accept_async, ws.split,
and sink.send) are properly indented before committing.
- Around line 82-85: The detached connection task is panicking on malformed
frames due to unwraps in the spawned async handler; replace the fallible calls
(serde_json::from_str(&text), req["id"].as_str(), req["method"].as_str()) with
proper error handling inside the Message::Text branch of the stream loop so the
task does not panic. On parse or field-missing errors, construct and send a JSON
error response (e.g., { "ok": false, "error": "<description>" }) back over the
same connection instead of unwrapping, and continue the loop; use the existing
variables req, id, method and the async handler context to locate where to catch
errors and send the response. Ensure errors include a brief message so tests
fail with a clear reason rather than stalling.
---
Outside diff comments:
In @.github/workflows/build.yml:
- Around line 79-124: Remove the leftover git merge conflict markers (e.g.
'<<<<<<< HEAD', '=======', '>>>>>>>') from the .github/workflows/build.yml and
dedupe the duplicated environment block (CARGO_PROFILE_RELEASE_OPT_LEVEL,
CARGO_PROFILE_RELEASE_CODEGEN_UNITS, CARGO_PROFILE_RELEASE_LTO,
CARGO_PROFILE_RELEASE_STRIP, CARGO_PROFILE_RELEASE_DEBUG) so the YAML parses;
ensure only a single clean env block remains and no conflict markers are present
anywhere in the file.
In @.github/workflows/typecheck.yml:
- Around line 27-50: The workflow still uses yarn-specific steps (cache: yarn,
yarn-cache step keyed to yarn.lock, `yarn install --frozen-lockfile`, and `yarn
workspace openhuman-app ...` commands) which must be migrated to pnpm; replace
the cache key to use pnpm (and pnpm-lock.yaml), change the cache step id (e.g.,
yarn-cache → pnpm-cache) and set cache: pnpm, use pnpm/action-setup@v4 like
build.yml, replace `yarn install --frozen-lockfile` with `pnpm install
--frozen-lockfile`, and convert `yarn workspace openhuman-app
compile|format:check|lint` to `pnpm --filter openhuman-app run
compile|format:check|lint` (or `pnpm --filter openhuman-app exec` if
appropriate); apply the exact same pattern to the other affected workflows
(test.yml and build-windows.yml) so CI uses pnpm consistently.
In `@app/scripts/e2e-run-spec.sh`:
- Around line 114-123: Remove the duplicated grep block that checks for
"127.0.0.1:${E2E_MOCK_PORT}" in "$DIST_JS" – keep the first block that prints
the pnpm suggestion and delete the second (stale yarn) block so the script only
performs the check once; look for the duplicated grep condition using the
E2E_MOCK_PORT and DIST_JS variables and remove the block that references "yarn
test:e2e:build".
In `@app/src/pages/Conversations.tsx`:
- Around line 410-417: The current creation of userMessage.id in
Conversations.tsx uses globalThis.crypto.randomUUID() without the fallback used
in threadSlice.ts (addInferenceResponse), which can throw in non-secure or older
environments; update Conversations.tsx to either use the same guarded expression
used in addInferenceResponse or, better, extract a shared helper (e.g.,
newMessageId()) in a new utils/ids module and call that from both
Conversations.tsx (where userMessage is built) and threadSlice.ts
(addInferenceResponse) so both locations use the identical fallback
implementation.
In `@docs/BUILDING.md`:
- Around line 153-179: Update the troubleshooting text that currently tells
users to run "yarn dev:app" (references to the dev:app script) to the pnpm
equivalent used by the repo; replace each "yarn dev:app" occurrence with "pnpm
--filter app run dev:app" (or the repo's pnpm script invocation) so developers
run the app package's dev:app script correctly; keep the existing pkill
workaround lines unchanged and ensure any other "yarn" mentions in this CEF
cache section are similarly converted to pnpm.
---
Nitpick comments:
In `@tests/webview_apis_bridge.rs`:
- Around line 43-126: The helper ensure_mock_server currently uses
std::sync::mpsc::channel and blocks on port_rx.recv(), which can deadlock a
Tokio runtime; change the channel to tokio::sync::oneshot (create a
oneshot::channel, send the port with the sender in the spawned thread, and await
the receiver with .await instead of recv()), update the thread::spawn closure to
call port_tx.send(port).expect(...) using the oneshot sender, and remove the
blocking recv usage (and the std::sync::mpsc import) so the async function
awaits the port properly and handles sender-drop errors.
🪄 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: f29a3f23-16a9-4f50-879b-946c8b92b421
⛔ Files ignored due to path filters (2)
app/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
.github/workflows/build-windows.yml.github/workflows/build.yml.github/workflows/docker-ci-image.yml.github/workflows/e2e-agent-review.yml.github/workflows/installer-smoke.yml.github/workflows/release-packages.yml.github/workflows/release.yml.github/workflows/test.yml.github/workflows/typecheck.yml.husky/pre-pushapp/package.jsonapp/scripts/e2e-build.shapp/scripts/e2e-run-spec.shapp/src-tauri/src/lib.rsapp/src-tauri/tauri.conf.jsonapp/src/AppRoutes.tsxapp/src/components/settings/panels/AIPanel.tsxapp/src/pages/Conversations.tsxapp/src/store/threadSlice.tsapp/test/e2e/helpers/artifacts.tsapp/test/e2e/specs/agent-review.spec.tsapp/test/e2e/specs/linux-cef-deb-runtime.spec.tsapp/test/wdio.conf.tsdocs/BUILDING.mdpackage.jsonpackages/deb/control.inpnpm-workspace.yamlscripts/review/README.mdscripts/review/cli.shsrc/core/all.rssrc/openhuman/webhooks/bus.rstests/linux_cef_deb_runtime_e2e.rstests/webview_apis_bridge.rs
💤 Files with no reviewable changes (3)
- app/src/AppRoutes.tsx
- src/openhuman/webhooks/bus.rs
- src/core/all.rs
✅ Files skipped from review due to trivial changes (13)
- pnpm-workspace.yaml
- scripts/review/cli.sh
- packages/deb/control.in
- .github/workflows/installer-smoke.yml
- app/src-tauri/src/lib.rs
- app/scripts/e2e-build.sh
- scripts/review/README.md
- app/test/e2e/specs/agent-review.spec.ts
- app/test/e2e/helpers/artifacts.ts
- .github/workflows/docker-ci-image.yml
- package.json
- .github/workflows/build-windows.yml
- .github/workflows/test.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- app/src/store/threadSlice.ts
- .husky/pre-push
- app/src/components/settings/panels/AIPanel.tsx
- app/test/e2e/specs/linux-cef-deb-runtime.spec.ts
- .github/workflows/e2e-agent-review.yml
- .github/workflows/release.yml
senamakel
approved these changes
Apr 24, 2026
Member
|
lmk once ready to merge from your side @VectorJet |
Member
|
Merging it anyways.. i see gh actions causing a ton of back and forth. will resolve this myself in a seperate PR.. welcome to the fam :D |
senamakel
added a commit
to senamakel/openhuman
that referenced
this pull request
Apr 24, 2026
Upstream merged the pnpm migration (tinyhumansai#886) and the CEF deb runtime spec (tinyhumansai#893). After resolving conflicts: - Drop unused `waitForText` import and escape-`/` in the runtime spec so eslint passes on the hook's default rules. - Re-run `pnpm install` to refresh the root lockfile with the command-palette deps already declared in `app/package.json`.
5 tasks
oxoxDev
added a commit
to oxoxDev/openhuman
that referenced
this pull request
Apr 24, 2026
…untime (tinyhumansai#883) pnpm migration (tinyhumansai#886) landed this file with a 4-space indent block (prettier expects 6), an unused `waitForText` import, and a no-useless-escape `\/` in a regex. Pre-push husky blocks our push because `pnpm format:check` and `pnpm lint` fail on this upstream file. Fix it here so PR tinyhumansai#892 can land. Unrelated to welcome-lockdown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
5 tasks
This was referenced Apr 26, 2026
Closed
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
Migrate the entire monorepo from yarn to pnpm for improved performance and consistency.
Changes
Testing
Related
Closes migration from yarn to pnpm (originally 28 commits from fork).
Summary by CodeRabbit
Chores
Bug Fixes
Documentation
Tests