Skip to content

fix(health): resolve PR #330 review findings#333

Merged
Akarsh-Hegde merged 1 commit into
pre-mainfrom
fix/pr-330-review
Jun 24, 2026
Merged

fix(health): resolve PR #330 review findings#333
Akarsh-Hegde merged 1 commit into
pre-mainfrom
fix/pr-330-review

Conversation

@Akarsh-Hegde

Copy link
Copy Markdown
Member

Summary

Addresses all review findings raised by @adityaharishch on PR #330. Targets pre-main so it is included in the pre-main → main promotion.

Fixed

[HIGH CORRECTNESS] meridian doctor exits non-zero on every healthy post-fold install

  • src/health/ui.rs: Replaced the entire Node-era UI probe (HTTP GET http://localhost:3939, /_next/static asset check, serve_mode_check) with a single Info check: "dashboard embedded in the Tauri binary (no separate service)". The com.meridiona.ui launchd agent no longer exists post-fold; probing for it always produced a false CRITICAL.
  • src/health/platform.rs: Removed LABEL_UI constant and replaced ui_service() body with the same Info check. The Node-era plist/PID check is gone.

[HIGH CORRECTNESS] MLX restart-budget exhaustion permanently wedges

  • tray/src-tauri/src/poll/mod.rs: Added MLX_COOLING_TICKS = 10. After MLX_MAX_RESTARTS consecutive failures, attempts now increments through the cooling window. At MAX_RESTARTS + COOLING_TICKS (~10 min), it resets to 0supervise() re-runs, kills any zombie holding port 7823 (via the existing KilledWedged path), and allows a fresh restart cycle. Previously the tray would stay wedged forever.

[HIGH SECURITY] MERIDIAN_RUNTIME_MANIFEST_URL redirectable in release builds

  • tray/src-tauri/src/mlx_server.rs: Gated the runtime env-var override behind #[cfg(debug_assertions)]. A release binary can no longer be redirected to an attacker-controlled manifest via a compromised environment variable.
  • Deferred: The self-referential SHA-256 issue (manifest and tarball both served from the same origin — a compromised server can swap both atomically) requires an out-of-band trust root (minisign or embedded public key, similar to the updater's pub_key). This is a follow-up design task; the env-var gate closes the more immediate attack surface.

[MEDIUM CORRECTNESS] capture_coverage datetime comparison bug

  • src/health/capture.rs: datetime('now', ?1) produces '2026-06-23 17:00:00' (space separator, no Z), but insert_capture_frame writes RFC 3339 '2026-06-24T16:00:00.000000Z' (T+Z). At index 10, 'T' (0x54) > ' ' (0x20) — every row passed the filter, so the 1-day window never excluded anything and coverage ghosting went undetected. Fixed to julianday(timestamp) > julianday('now', ?1) following the frame_freshness pattern already in the same file.

[MEDIUM CORRECTNESS] subscribe() snapshot prime silently swallows errors

  • ui/lib/bridge.ts: Replaced .catch(() => {}) with retry-once-after-2s (targeted at the cold-DB startup race — tray can beat the daemon by ~1 s), then console.warn(...) on second failure. The view can still stay blank if the DB never opens, but the error is now visible in DevTools rather than silently dropped.

[MEDIUM SECURITY] sudo npm install runs lifecycle scripts as root

  • scripts/install-from-bundle.sh: Added --ignore-scripts to both npm install -g lines (non-sudo and sudo paths). A compromised screenpipe@0.4.6 tarball can no longer execute lifecycle scripts as root.

Deferred (noted, not blocked)

  • install.sh OO sha256: OpenObserve v0.90.3 tarball is downloaded without integrity verification. Fix requires sha256 of the official arm64 + amd64 tarballs. These need to be fetched from the OO release page and pinned — straightforward but requires the real values.
  • build-mlx-runtime.sh PBS version pin: releases/latest is resolved at build time without a sha256 check. Fix requires pinning PBS_RELEASE_TAG to a specific astral-sh/python-build-standalone release and verifying the downloaded archive.
  • MLX manifest self-referential SHA-256: Manifest and tarball served from the same origin — requires a minisign/ed25519 out-of-band trust root. Design tracked separately.

Test plan

  • cargo clippy -- -D warnings — clean ✅ (pre-push verified)
  • cargo test — all passing ✅ (pre-push verified)
  • UI build — clean ✅ (pre-push verified)
  • meridian doctor on a post-fold install should show Info for the ui group, not Critical/Warn
  • MLX wedge: manually exhaust MLX_MAX_RESTARTS; verify attempts increments through cooling and resets at tick 15

🤖 Generated with Claude Code

https://claude.ai/code/session_01SKdh3tKWLZhtQ9FCfA5RYH

- health/ui.rs + platform.rs: remove legacy Node-era HTTP probes
  (serve_health, serve_mode_check, asset checks) — the dashboard is
  now a static export embedded in the Tauri binary; no com.meridiona.ui
  launchd agent or HTTP port exists post-fold. meridian doctor was
  reporting CRITICAL on every healthy post-fold install. Both checks now
  return an Info line.

- health/capture.rs: fix capture_coverage datetime comparison. The
  query used datetime('now', ?1) which produces '2026-06-23 17:00:00'
  (space separator, no Z), while insert_capture_frame writes RFC 3339
  '2026-06-24T16:00:00.000000Z' (T+Z). String comparison at index 10 had
  'T' > ' ' so every row passed the filter — the 1-day coverage window
  never excluded anything. Rewritten as julianday(timestamp) >
  julianday('now', ?1) following the existing frame_freshness pattern.

- poll/mod.rs (supervise_mlx): add MLX_COOLING_TICKS=10 cooling period.
  Budget-exhausted path now lets attempts grow past MLX_MAX_RESTARTS; at
  MAX+COOLING ticks it resets to 0 so supervise() can kill any zombie on
  port 7823 and attempt a new restart cycle. Prevents permanent wedge
  after OOM/crash.

- bridge.ts (subscribe): replace silent .catch(() => {}) with retry-once-
  after-2s for the cold-DB startup race (tray beats daemon on launch),
  then console.warn on second failure so it is visible in DevTools.

- install-from-bundle.sh: add --ignore-scripts to both npm install lines
  for screenpipe. The sudo path previously ran lifecycle scripts as root.

- mlx_server.rs (manifest_url): gate MERIDIAN_RUNTIME_MANIFEST_URL env-
  var override behind #[cfg(debug_assertions)]. Kills the "env redirects
  the manifest in production" half of the HIGH security finding. The
  self-referential SHA-256 issue (manifest and tarball served from the
  same origin) requires an out-of-band trust root and is deferred.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SKdh3tKWLZhtQ9FCfA5RYH
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c07ab096-60d8-453c-a5b1-b43a4ddc402d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/pr-330-review

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

@Akarsh-Hegde Akarsh-Hegde merged commit 768f47b into pre-main Jun 24, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant