fix(ci): repair upstream main build and checkout - #5469
Conversation
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (130)
📝 WalkthroughWalkthroughThe pull request updates CI container usage, desktop E2E execution, Tauri integration, Rust capability routing, configuration persistence, event-bus usage, and related tests. ChangesCI, Tauri, and E2E execution
Rust behavior and maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant E2ERunner
participant TauriDriver
participant WDIO
CI->>E2ERunner: restore application and frontend artifacts
E2ERunner->>TauriDriver: start driver and poll readiness
E2ERunner->>WDIO: run isolated Linux specifications
WDIO-->>E2ERunner: return specification status
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Assign the Core capability gate to all core and recall memory functions so that a null driver can remove the entire driver-backed memory surface. Previously these functions were ungated, making it impossible to disable them through capability configuration. Also adds support for the "reasoning-v1" model override hint and updates the always-present memory tools list to reflect the new gating strategy. Auto-committed-on: dragonfly
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
tinysweeper
Bot
added
the
priority: p3
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
coderabbitai
Bot
added
bug
rust-core
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 4 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0174 · 268,082 in / 48,726 out · 212,198 cached (79%) · z-ai/glm-5.2
critique: $0.0083 · 74,099 in / 30,335 out · 61,784 cached (83%) · z-ai/glm-5.2
security: $0.0030 · 51,572 in / 8,538 out · 43,873 cached (85%) · z-ai/glm-5.2
tests: $0.0033 · 71,801 in / 6,878 out · 57,028 cached (79%) · z-ai/glm-5.2
description: $0.0028 · 70,610 in / 2,975 out · 49,513 cached (70%) · z-ai/glm-5.2
| } | ||
| if received_ready && self.is_rpc_port_open().await { | ||
| log::info!("[core] core rpc became ready at {}", self.rpc_url()); | ||
| if self.is_rpc_port_open().await { |
There was a problem hiding this comment.
Second wait loop skips the occupied-port guard added to the first
The first wait loop guards the new socket-only readiness path with !preferred_port_was_occupied, precisely so a pre-existing foreign listener on the preferred port is not mistaken for the embedded core becoming ready. The second wait loop, changed in the same diff, drops both received_ready and that guard:
if self.is_rpc_port_open().await {
if !received_ready {
log::warn!(
"[core] core RPC listener became reachable before the embedded ready signal at {}; continuing",
self.rpc_url()
);
} else {
log::info!("[core] core rpc became ready at {}", self.rpc_url());
}
return Ok(());
}When preferred_port_was_occupied is true (a foreign process held the preferred port at spawn time) and the embedded task has not yet sent its ready signal, self.is_rpc_port_open() can report the foreign listener as open. This loop then returns Ok(()), treating a process we do not own as our core's readiness — the exact failure the !preferred_port_was_occupied guard was added to prevent in the loop above. The two loops should apply the same readiness predicate; the second needs the same received_ready || (!preferred_port_was_occupied && ...) shape.
[RULE] Correctness — readiness detection ·
| for spec in "${_spec_paths[@]}"; do | ||
| bash "$APP_DIR/scripts/e2e-run-session.sh" "$spec" | ||
| status=$? | ||
| if [[ $status -ne 0 ]]; then |
There was a problem hiding this comment.
Set _WDIO_EXIT_CODE to 0 before the per-spec loop
In the per-spec loop branch, _WDIO_EXIT_CODE is only assigned when a spec fails. If every spec passes, the variable is never set in this branch, so it retains whatever value it had before this block. The original code unconditionally set it with _WDIO_EXIT_CODE=$? after the single run, and the else branch still does. If _WDIO_EXIT_CODE was not initialized to 0 earlier in the script (or carries a stale non-zero value from earlier logic), the finish() trap at the bottom — which exits with _WDIO_EXIT_CODE — would report a spurious failure on an all-passing Linux run.
[RULE] Missing success-path initialization of _WDIO_EXIT_CODE in the per-spec loop ·
| > "$TAURI_DRIVER_LOG" 2>&1 & | ||
| APP_PID=$! | ||
| export TAURI_DRIVER_PORT | ||
| for i in $(seq 1 30); do |
There was a problem hiding this comment.
Verify tauri-driver readiness after the poll loop before running wdio
The readiness loop only exits early when the driver process dies. If curl never succeeds within 30 iterations but the process stays alive, the loop falls through and the script proceeds to invoke wdio against a port that is not yet listening. There is no post-loop check (e.g. a final curl or a flag set on break) to distinguish "ready" from "timed out but alive."
**[RULE] ** ·
| @@ -176,8 +171,6 @@ describe('Settings - Feature Preferences', function () { | |||
| await waitForText('Color', 15_000); | |||
| expect(await clickSelector('[data-testid="mascot-color-burgundy"]')).toBeDefined(); | |||
| await browser.pause(1000); | |||
There was a problem hiding this comment.
Mascot color test no longer verifies persistence
The test is named persists mascot color selection, but this PR removes the only line that verified persistence across a reload:
existing_code:
await browser.pause(1000);
- await reloadAndReturnTo('/settings/mascot', 'Color');
-
expect(await mascotColorChecked('burgundy')).toBe('true');
After this change, mascotColorChecked('burgundy') is asserted only in the same session, immediately after the click + 1000 ms pause — so the assertion now confirms the click took effect, not that the selection persisted. Unlike the mascot-voice test, no comment is added here explaining the removal, and reloadAndReturnTo is still used in the notifications test in this same diff, so a blanket "reloads terminate the WebDriver session" rationale does not hold across the file. As written, the test no longer tests what its title claims.
[RULE] , ·
| timeout-minutes: 10 | ||
| container: | ||
| image: ghcr.io/tinyhumansai/openhuman_ci:rust-1.96.1 | ||
| image: ghcr.io/tinyhumansai/openhuman_ci:latest |
There was a problem hiding this comment.
Pin CI container images instead of the mutable :latest tag
All four Linux jobs switch their container image from a pinned Rust version tag (rust-1.96.1) to the mutable latest tag. A mutable tag makes CI non-reproducible: the toolchain, system libraries, and any preinstalled build tooling can change at any time the image is republished, and a single bad image push can break every job using it with no corresponding code change to bisect. Pinning to a digest or an explicit version tag preserves the previous reproducibility. Note the repo's own workflow rules call for pinning third-party actions for the same reason; the container image is first-party so this isn't strictly that rule, but the same reproducibility concern applies.
[RULE] Pin CI container images to a version tag or digest ·
| timeout-minutes: 10 | ||
| container: | ||
| image: ghcr.io/tinyhumansai/openhuman_ci:rust-1.96.1 | ||
| image: ghcr.io/tinyhumansai/openhuman_ci:latest |
There was a problem hiding this comment.
Pin CI container images by digest, not the mutable :latest tag
The pull request moves every CI container from a pinned tag (rust-1.96.1) to a floating tag (:latest). A floating tag gives no reproducibility and no supply-chain integrity: the image the CI job runs in can change at any time, and if the registry or the image build pipeline is compromised, a malicious image is picked up automatically on the next run. Even for a first-party image, tags are mutable — a pinned digest (@sha256:…) is the only form that guarantees the exact bytes. Pinning by version tag is weaker than by digest but strictly better than :latest.
image: ghcr.io/tinyhumansai/openhuman_ci:latest
This appears five times in the diff, replacing the previous rust-1.96.1 tag in each job.
[RULE] Supply chain integrity: pin CI container images by digest or immutable tag, not a floating :latest tag. ·
|
|
||
| if received_ready && self.is_rpc_port_open().await { | ||
| log::info!("[core] core rpc became ready at {}", self.rpc_url()); | ||
| if received_ready || (!preferred_port_was_occupied && self.is_rpc_port_open().await) |
There was a problem hiding this comment.
Add tests for the relaxed core RPC readiness logic
The readiness detection logic was significantly relaxed. In the initial check, received_ready now short-circuits without verifying is_rpc_port_open() (previously both were required via &&). In the timeout loop, received_ready is no longer required at all — the loop returns Ok(()) whenever the port is open, regardless of whether the embedded ready signal ever fired. This means a foreign listener on a free port can satisfy readiness, or the ready signal can fire while the RPC port is closed. No tests were added or modified to cover these new paths.
[RULE] untested-behaviour ·
| const CEF_CDP_HOST = process.env.CEF_CDP_HOST || '127.0.0.1'; | ||
| const CEF_CDP_PORT = parseInt(process.env.CEF_CDP_PORT || '19222', 10); | ||
|
|
||
| function linuxAppPath(): string { |
There was a problem hiding this comment.
Fix linuxAppPath dead branch — both paths return the same value
The linuxAppPath helper checks whether the built binary exists, then returns the same path regardless of the result. Both branches of the if return candidate, making the fs.existsSync guard dead code — likely a copy-paste error where the fallback was meant to return a different path or throw.
[RULE] dead-code ·
|
|
||
| #[tokio::test] | ||
| async fn round21_rss_reader_covers_http_body_guards_and_invalid_utf8() { | ||
| async fn round21_rss_reader_rejects_private_hosts_before_fetching() { |
There was a problem hiding this comment.
Restore RSS reader body-size, UTF-8, and HTTP status test coverage
The test was rewritten from comprehensive RSS reader coverage — HTTP 503 status rejection, large body (5 MB+) rejection, and invalid UTF-8 rejection — to a single SSRF rejection assertion. The RSS reader source code (not modified in this diff) presumably still contains body-size guards, UTF-8 validation, and HTTP status handling, but those behavioral paths are no longer exercised by any test in this file.
[RULE] coverage-regression ·
|
|
||
| #[tokio::test] | ||
| async fn rss_reader_lists_reads_and_reports_feed_errors_from_loopback() { | ||
| async fn rss_reader_rejects_private_hosts_before_fetching() { |
There was a problem hiding this comment.
Restore RSS/Atom parsing and HTTP error test coverage
The test was rewritten from comprehensive RSS/Atom parsing coverage — item titles, content types, metadata links, missing-item errors, HTTP 502 status handling, and max_items limiting — to a single SSRF rejection assertion. Content parsing, item reading, and HTTP error handling paths are no longer tested here.
[RULE] coverage-regression ·
What this change touches131 files, +1263 -1693 across 6 components. The code graph knows nothing about these files yet — normal for newly added files, and a cold index otherwise. flowchart LR
n0["src<br/>77 files +612 -556"]:::changed
n1["app<br/>25 files +338 -587<br/>8 findings"]:::blocking
n2["tests<br/>18 files +144 -356<br/>2 findings"]:::flagged
n3[".github<br/>6 files +37 -113<br/>2 findings"]:::flagged
n4["root<br/>2 files +65 -65<br/>1 finding"]:::blocking
n5["scripts<br/>3 files +67 -16"]:::changed
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed. Grey: untouched, reached through an import or a call. Orange: has findings. Red: has a finding that blocks the merge.
Changed files
|
tinysweeper
Bot
added
priority: p1
…epair\n\nfix(ci): repair upstream main build and checkout\n
1 participant
Summary
Validation
pnpm typecheckpnpm lint(98 existing warnings, 0 errors)pnpm rust:clippycargo test --manifest-path app/src-tauri/Cargo.toml --lib --no-runDraft while GitHub Actions, including desktop/E2E coverage, runs.
Summary by CodeRabbit
Bug Fixes
reasoning-v1.Maintenance