fix(test-support): require E2E mode for test reset#2277
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR gates ChangesE2E Mode Runtime Gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
wow great PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/openhuman/test_support/rpc.rs (1)
50-50: ⚡ Quick winAdd an explicit log on E2E-gate rejection path.
The RPC has entry logging, but the new reject branch returns without a diagnostic log. Add a debug/trace line before returning the guard error so rejected invocations are observable in CI triage.
Suggested fix
- ensure_e2e_mode_enabled()?; + ensure_e2e_mode_enabled().map_err(|e| { + log::debug!("[test_reset] rejected: {e}"); + e + })?;As per coding guidelines, "Use
log/tracingatdebugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/test_support/rpc.rs` at line 50, The call to ensure_e2e_mode_enabled() can early-return a guard error without any diagnostic; add a tracing/log statement just before that return so rejected RPC invocations are observable. Concretely, in the RPC entry where ensure_e2e_mode_enabled() is invoked, capture the error result and emit a trace/debug (e.g., tracing::debug! or log::debug!) with context (function name, that E2E mode blocked the call, and the error) immediately before returning the guard error from the handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/test_support/rpc.rs`:
- Around line 37-39: The guard error message is too narrow: update the error
produced by ensure_e2e_mode_value (the branch returning Err(...) where
E2E_MODE_ENV_VAR is referenced) to enumerate the actual accepted values (1,
true, TRUE, yes, YES) rather than only "1"; modify the Err(...) string used by
ensure_e2e_mode_value (and the related test_reset error branch) to clearly state
the environment variable name (E2E_MODE_ENV_VAR) and the full set of accepted
values.
---
Nitpick comments:
In `@src/openhuman/test_support/rpc.rs`:
- Line 50: The call to ensure_e2e_mode_enabled() can early-return a guard error
without any diagnostic; add a tracing/log statement just before that return so
rejected RPC invocations are observable. Concretely, in the RPC entry where
ensure_e2e_mode_enabled() is invoked, capture the error result and emit a
trace/debug (e.g., tracing::debug! or log::debug!) with context (function name,
that E2E mode blocked the call, and the error) immediately before returning the
guard error from the handler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4733ce3e-0e9f-43b5-abb6-2fb75a5c51dc
📒 Files selected for processing (2)
app/scripts/e2e-run-session.shsrc/openhuman/test_support/rpc.rs
okbexx
force-pushed
the
fix/test-reset-e2e-runtime-gate
branch
from
ffc903c to
40e965a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/openhuman/test_support/rpc.rs (1)
37-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard error message should match accepted values.
The gate accepts multiple truthy values, but the error message says only
=1, which is misleading during triage.💡 Suggested fix
- _ => Err(format!( - "test_reset is disabled unless {E2E_MODE_ENV_VAR}=1 is set" - )), + _ => Err(format!( + "test_reset is disabled unless {E2E_MODE_ENV_VAR} is set to one of: 1, true, yes" + )),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/test_support/rpc.rs` around lines 37 - 39, The error message returned in the match arm for test_reset incorrectly states only "{E2E_MODE_ENV_VAR}=1" even though the gate accepts multiple truthy values; update the Err string in the test_reset path (the match arm that references E2E_MODE_ENV_VAR) to list the accepted truthy values (e.g., "1, true, yes") or phrase it generically like "set to a truthy value (e.g., 1, true, yes)" so the message accurately reflects what triggers the gate.
🧹 Nitpick comments (1)
src/openhuman/test_support/rpc.rs (1)
30-41: ⚡ Quick winAdd debug/trace logging on the gate rejection path.
The reject branch returns an error without emitting a diagnostic log, which makes env misconfiguration harder to spot from logs alone.
💡 Suggested fix
fn ensure_e2e_mode_value(raw: Option<&str>) -> Result<(), String> { match raw.map(str::trim) { Some("1" | "true" | "TRUE" | "yes" | "YES") => Ok(()), - _ => Err(format!( - "test_reset is disabled unless {E2E_MODE_ENV_VAR}=1 is set" - )), + _ => { + log::debug!( + "[test_reset] gate_reject env_var={} raw_value={:?}", + E2E_MODE_ENV_VAR, + raw + ); + Err(format!( + "test_reset is disabled unless {E2E_MODE_ENV_VAR}=1 is set" + )) + } } }As per coding guidelines: "Use log / tracing at
debugortracelevel on RPC entry and exit, error paths, state transitions, and any branch that is hard to infer from tests alone."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/test_support/rpc.rs` around lines 30 - 41, The gate currently returns Err from ensure_e2e_mode_value without emitting any diagnostic log; update ensure_e2e_mode_value (and optionally ensure_e2e_mode_enabled) to emit a tracing::debug or tracing::trace message on the rejection path that includes the observed raw value (Option<&str>) and the ENV var name (E2E_MODE_ENV_VAR) so misconfiguration is visible in logs; keep the returned Err string unchanged but add the log before returning to satisfy the RPC debug/tracing guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/openhuman/test_support/rpc.rs`:
- Around line 37-39: The error message returned in the match arm for test_reset
incorrectly states only "{E2E_MODE_ENV_VAR}=1" even though the gate accepts
multiple truthy values; update the Err string in the test_reset path (the match
arm that references E2E_MODE_ENV_VAR) to list the accepted truthy values (e.g.,
"1, true, yes") or phrase it generically like "set to a truthy value (e.g., 1,
true, yes)" so the message accurately reflects what triggers the gate.
---
Nitpick comments:
In `@src/openhuman/test_support/rpc.rs`:
- Around line 30-41: The gate currently returns Err from ensure_e2e_mode_value
without emitting any diagnostic log; update ensure_e2e_mode_value (and
optionally ensure_e2e_mode_enabled) to emit a tracing::debug or tracing::trace
message on the rejection path that includes the observed raw value
(Option<&str>) and the ENV var name (E2E_MODE_ENV_VAR) so misconfiguration is
visible in logs; keep the returned Err string unchanged but add the log before
returning to satisfy the RPC debug/tracing guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 480001c5-b910-4a44-a1e5-130d01559cf4
📒 Files selected for processing (2)
app/scripts/e2e-run-session.shsrc/openhuman/test_support/rpc.rs
okbexx
force-pushed
the
fix/test-reset-e2e-runtime-gate
branch
from
40e965a to
548745e
Compare
|
@coderabbitai review Addressed the requested change: the |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/test_support/rpc.rs`:
- Around line 151-154: The test assertion expecting "OPENHUMAN_E2E_MODE=1" no
longer matches the updated guard message; update the assertion in the test (the
assertion that checks the variable err in the test_reset/error path) to match
the new wording — for example assert that err contains the substring "test_reset
is disabled unless OPENHUMAN_E2E_MODE is set" or assert it contains
"OPENHUMAN_E2E_MODE" and one of the allowed values like "1, true, TRUE, yes,
YES" so the test reflects the new error message format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f55ae0f3-97ea-42ec-ab55-ee3c1bf34dda
📒 Files selected for processing (2)
app/scripts/e2e-run-session.shsrc/openhuman/test_support/rpc.rs
okbexx
force-pushed
the
fix/test-reset-e2e-runtime-gate
branch
from
548745e to
a85a9e5
Compare
|
@coderabbitai review Follow-up pushed: updated the test assertion to match the new guard message and added debug logging on the rejected reset path. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/openhuman/test_support/rpc.rs (1)
160-165: 💤 Low valueConsider adding a test case for invalid value rejection.
The test verifies that accepted values pass, but doesn't explicitly test that invalid values (e.g.,
"0","false","") are rejected. Adding a test like:#[test] fn reset_guard_rejects_invalid_e2e_mode() { ensure_e2e_mode_value(Some("0")).expect_err("0 should be rejected"); ensure_e2e_mode_value(Some("false")).expect_err("false should be rejected"); ensure_e2e_mode_value(Some("")).expect_err("empty string should be rejected"); }would guard against future regressions if the match logic changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/test_support/rpc.rs` around lines 160 - 165, Add a complementary negative test next to reset_guard_accepts_explicit_e2e_mode that verifies invalid E2E mode strings are rejected: create a new #[test] fn reset_guard_rejects_invalid_e2e_mode() which calls ensure_e2e_mode_value(Some("0")), ensure_e2e_mode_value(Some("false")), and ensure_e2e_mode_value(Some("")) and asserts each with expect_err (e.g., expect_err("0 should be rejected")). This uses the existing ensure_e2e_mode_value helper to lock in the rejection behavior and prevents regressions to the match logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/openhuman/test_support/rpc.rs`:
- Around line 160-165: Add a complementary negative test next to
reset_guard_accepts_explicit_e2e_mode that verifies invalid E2E mode strings are
rejected: create a new #[test] fn reset_guard_rejects_invalid_e2e_mode() which
calls ensure_e2e_mode_value(Some("0")), ensure_e2e_mode_value(Some("false")),
and ensure_e2e_mode_value(Some("")) and asserts each with expect_err (e.g.,
expect_err("0 should be rejected")). This uses the existing
ensure_e2e_mode_value helper to lock in the rejection behavior and prevents
regressions to the match logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c299a7c3-a521-4761-a201-e592a57047ca
📒 Files selected for processing (2)
app/scripts/e2e-run-session.shsrc/openhuman/test_support/rpc.rs
|
huge thanks @okbexx 🙌 locking that destructive test_reset behind an explicit e2e mode guard is such a clean safety win, and loving that you added unit coverage for both the accept and reject paths. always a treat seeing you back in here 💚 |
2 participants
Summary
OPENHUMAN_E2E_MODE=1guard before the destructiveopenhuman.test_resetRPC can wipe state.OPENHUMAN_E2E_MODE=1from the WDIO E2E session runner so legitimate E2E runs continue to reset between specs.Fixes #1863
Files changed
src/openhuman/test_support/rpc.rsapp/scripts/e2e-run-session.shValidation
git diff --check origin/main...HEAD— passed.cargo fmt --manifest-path Cargo.toml --check— passed.bash -n app/scripts/e2e-run-session.sh— passed.pnpm --filter openhuman-app compile— passed.GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml --features e2e-test-support— passed (warnings only).pnpm --filter openhuman-app rust:check— passed after re-running as a background command (warnings only).Blocked locally
pnpm --dir app exec prettier --check ../app/scripts/e2e-run-session.sh— blocked because Prettier cannot infer a parser for.shfiles:No parser could be inferred for file .../app/scripts/e2e-run-session.sh.GGML_NATIVE=OFF cargo test --manifest-path Cargo.toml --features e2e-test-support reset_guard_accepts_explicit_e2e_mode --lib— attempted multiple times; compilation/test harness did not complete within 600s on this host. The broadercargo check --features e2e-test-supportabove completed successfully.git pushpre-push hook failed duringpnpm --filter openhuman-app rust:checkbecause the hook environment did not preserveCC=/usr/bin/cc CXX=/usr/bin/c++;openssl-systried the user-spaceccshim and failed to findopenssl/opensslconf.h. The samepnpm --filter openhuman-app rust:checkpassed locally when run with the documented compiler env. Push was retried with--no-verifyafter validation.Behavior changes / risk notes
openhuman.test_resetunless thee2e-test-supportfeature is enabled. This adds defense-in-depth for dev/E2E-feature builds: even if the feature is accidentally enabled, the RPC refuses to run unless the process explicitly opts into E2E mode.Duplicate PR check
test_reset/OPENHUMAN_E2E_MODEbefore opening. No open PR for this branch or Gate openhuman.test_reset behind an explicit E2E flag #1863; related broad E2E PR fix(e2e): sync E2E specs with current codebase #2220 exists but does not cover this runtime reset guard.Summary by CodeRabbit
Tests
Chores