test(e2e): backfill coverage for migration guard, prompt placement, redirects and the privacy sheet - #5975
Conversation
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
How this change flows0 changed behaviours across 8 relationships. 4 surrounding behaviours are shown (60 graph nodes walked). 56 further behaviours left out to keep the diagram readable. flowchart LR
n0["format"]:::impacted
n1["join"]:::impacted
n2["boot_stack"]:::impacted
n3["spawn_sse_collector"]:::impacted
n2 -->|calls| n0
n2 -->|tests| n0
n2 -->|calls| n1
n2 -->|tests| n1
n3 -->|calls| n0
n3 -->|tests| n0
n3 -->|calls| n1
n3 -->|tests| n1
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 behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
tinysweeper
Bot
added
the
priority: p3
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds end-to-end coverage for Connections redirects, onboarding privacy-sheet interactions, tool-policy prompt placement, and Hermes migration refusal. ChangesConnections deep-link coverage
Onboarding privacy-sheet coverage
Backend safety coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This PR adds and updates end-to-end tests without changing production behavior, and no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 15.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 3 files. (1 skipped: 1 too large.)
Warning Your free Security trial is over. An organization admin can upgrade to Advanced for continuous pull request security review or dismiss this notice. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 56eb259e43
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
dedc738 to
da883db
Compare
da883db to
5a0e694
Compare
…ects, privacy sheet An audit of six PRs merged in the last week found none with e2e coverage of what they changed. Three shipped tests their authors reasonably believed were sufficient: tinyhumansai#5799 added 62 lines to `tests/json_rpc_e2e.rs` that are a determinism fix for a different test; tinyhumansai#5821's four unit tests exercise an extracted pure helper and would pass if `build_system_prompt` stopped calling it; tinyhumansai#5939's two tests are vitest, not an e2e lane. Added, each driving the changed path and asserting the changed behaviour: - tinyhumansai#5799 `json_rpc_migrate_hermes_refuses_null_driver_without_naming_openclaw`. Drives the Hermes migration RPC into a configured null driver and asserts the refusal, that it no longer hard-codes "OpenClaw" in a message both migrations raise, and that the source workspace really is byte-identical afterwards. The third arm tinyhumansai#5799 added is unreachable with modules on and stays with its gates-off unit test; the doc comment says so rather than faking it. - tinyhumansai#5821 two tests on the real `Agent::build_system_prompt`, asserting the tool-policy boundary does not open the prompt and that it is the prompt's final block — the property a prefix cache keys on, and the one a revert to prepending destroys. - tinyhumansai#5939 two cases on `/webhooks`, asserting the query and the fragment survive BOTH redirect hops. They assert the final destination, so a fix to only the first hop still fails them. - tinyhumansai#5845 a new spec opening the privacy sheet, which no e2e had ever done. Also flips `BUG: /skills?tab=channels drops the tab`, which pinned pre-fix behaviour with the note "Flip the two assertions below when that lands". It landed in d434f1e (tinyhumansai#5924), four hours before that spec was last touched, so the test asserts the opposite of shipped behaviour.
`raw_coverage_all` declares `required-features = ["voice", "inference"]`
(Cargo.toml:117). Cargo does not fail a plain `cargo test` for a target whose
required features are off — it SKIPS it, silently. A test placed there is
invisible to any default-feature run, including a contributor's local one:
error: target `raw_coverage_all` in package `openhuman` requires the
features: `voice`, `inference`
`tests/agent_harness_e2e.rs` declares none, so the assertions execute.
Self-contained rather than built on `streaming_support::agent_with_s`: that
helper resolves a real memory store, which needs an `EmbeddingHost` seam a
`tests/` binary cannot install — `host_impls::install_for_tests` is
`#[cfg(test)]`, visible only to the crate's own unit tests. Both tests failed
in setup on that before, in every phase, which is a test that cannot fail for
the reason it claims. The prompt path under test never touches memory, so a
stub is sufficient and steadier.
The working tree was formatted but the commit was not — `cargo fmt --check` run against the tree passed while CI, which checks the commit, reported `Diff in tests/agent_harness_e2e.rs:3236`.
5a0e694 to
a45feef
Compare
1 participant
Summary
Backfills e2e coverage for five recently-merged PRs that shipped without it. Each test drives the changed code path and asserts the changed behaviour — not a symbol mention, which is all the domain e2e gate's string match would require.
Also fixes a test on
mainthat asserts the opposite of shipped behaviour (§/skillsbelow).Problem
An audit of six PRs merged in the last seven days found zero with e2e coverage of what they changed. Three of them shipped tests their authors reasonably believed were sufficient:
tests/json_rpc_e2e.rs, so it reads as e2e-covered. Those lines are a determinism fix for a different test; nothing asserted the refusal it introduced.build_system_promptstopped calling it. The one existing test that goes through the real builder asserts the boundary is present, not where — it passes unchanged under the old prepend.Solution
tests/json_rpc_e2e.rsjson_rpc_migrate_hermes_refuses_null_driver_without_naming_openclawtests/raw_coverage/agent_prompts_subagent_raw_coverage_e2e.rssystem_prompt_appends_the_tool_policy_boundary_after_the_body,two_agents_differing_only_in_policy_share_a_leading_prefixapp/test/playwright/specs/connections-tab-deeplinks.spec.ts/webhooks?tab= survives BOTH hops of its redirect,/webhooks carries the fragment through as well as the queryapp/test/playwright/specs/privacy-what-leaves-sheet.spec.tsRevert-check
Every Rust test was run with the fix in place, with the specific hunk reverted, and restored. All three failed on my assertion, not a compile error or an unrelated panic.
json_rpc_migrate_hermes_refuses_null_driver_without_naming_openclaw1 passedjson_rpc_e2e.rs:14219—!message.contains("OpenClaw"), quoting the restored text: "refusing to import OpenClaw memory into the null driver — memory is disabled by configuration…"system_prompt_appends_the_tool_policy_boundary_after_the_bodypassedagent_harness_e2e.rs:3210— "the boundary must not open the prompt", withPrompt begins: "## Tool Policy Boundary\n- Agent: main\n- Channel: boundary-channel…"passedthe_tool_policy_boundary_is_the_last_block_in_the_promptpassedagent_harness_e2e.rs:3248— "the boundary block must be the prompt's FINAL block"passedThe two Playwright specs were NOT run locally, and I am not claiming they were.
app/scripts/e2e-web-build.shbuilds a full product-featuredopenhuman-corebefore any spec can run, and the fleet is under a disk and memory throttle that prohibits builds of that size. Their revert analysis was done by reading the reverted components:AppRoutes.tsx:243was<Navigate to="/settings/integrations" replace />andsettingsRouteElements.tsx:129was<Navigate to="/connections" replace />. Both bare, so both assertions (tab=channelspresent,#delivery-3present) fail on revert.aria-describedbyassertion is the one that fails on revert, and it sits in the shared open step every test in the file calls.CI's
ci-full.yml"E2E (Playwright / web lane)" job runs them for real.#5845 — three of my first four assertions were vacuous, and I restructured rather than shipping them. I assumed the Radix rewrite introduced
role="dialog", Escape-to-close and overlay-click-to-close. Reading the pre-#5845 component (git show 663399655^:…/WhatLeavesMyComputerSheet.tsx) shows it already had all three:role="dialog"+aria-modalat lines 41-42, adocumentkeydown listener at 19-24, a full-bleed close<button>at 33-37. The only thing the rewrite added isaria-describedby— the old markup rendered the subhead as a bare<p>with no id. The Escape and overlay cases are kept and labelled in-file as preserved behaviour, which a library swap can lose quietly.#5858 cannot be revert-checked, structurally. It re-points ~5000 lines of imports after
tinyagentswas split upstream into five crates. Reverting that does not produce a failing assertion — it produces a tree that does not compile, because the singletinyagentscrate no longer exists at that path. Any test attached to it would be either upstream-crate coverage or vacuous with respect to this PR. A mass re-point is verified by the compiler, and that is the only verification available. The real gap underneath —interrupt_orphaned_agent_runshas no e2e in any lane — is described inW12-test-findings.md§3, including why the existing harness cannot reach it (bootstrap_core_runtimevsbuild_core_http_router).A lane that silently skips its own tests
My first draft put the #5821 tests in
tests/raw_coverage/. They never ran:Cargo.toml:117declares those required features. Cargo does not fail a plaincargo testfor a target whose required features are off — it skips it, and only the explicit--test <name>form reports why. So every test in that 73-file lane is invisible to any default-feature run, including a contributor's localcargo test. Moved both tests totests/agent_harness_e2e.rs, which declares none.Worth someone checking separately: whether any CI lane passes
voice,inference. If none does, that lane is not a lane./skills— a test onmainasserting the opposite of shipped behaviourconnections-tab-deeplinks.spec.ts:176pinned pre-fix behaviour with the comment "Flip the two assertions below when that lands." It landed:d434f1e0f(#5924) moved/skillsontoForwardSearch, and it is an ancestor ofmain. The fix predates the spec that contradicts it by four hours. Flipped, as instructed, and the stale file header corrected.What #5799's test does and does not reach
target_memory_backenddistinguishes three null-driver causes. The third — "this build has no memory module compiled in" — only fires under#[cfg(not(feature = "modules"))]: bothfeature = "modules"variants ofbinding::module_providerreturnDriverClass::Moduleunconditionally (memory/binding.rs:356-404). It is unreachable from any default-feature lane and stays covered by its gates-off unit test. The e2e drives the arm that is reachable and asserts the half of #5799 that changed for every build: the headline stopped hard-coding "OpenClaw" in a messagemigrate_hermesalso raises.Submission Checklist
## Related— no matrix rows changed.Closes #NNN— this is coverage backfill for already-merged PRs, not an issue fix.Impact
Tests only. No production code changes.
Related
Summary by CodeRabbit