Repository navigation
fix(ci): the domain e2e coverage gate was measuring a seventh of the surface - #5936
Merged
senamakel merged 1 commit intoSep 1, 2026
Merged
Conversation
The gate reported 100% for five namespaces it had measured nothing in, and
never looked at ~50 more at all. Three defects, all provable:
(a) Discovery read only files whose PATH matched /(^|\/)schemas?(\.rs|\/)/.
The 2026-08-30 include! split (tinyhumansai#5856/tinyhumansai#5857) moved ControllerSchema literals
into *_part_NN.rs siblings that the pattern does not match -- flows into
flows_schema_part_01/02.rs, and the same for threads, tools, composio,
inference, memory/sources, mcp/registry, agent/learning, agent/orchestration.
13 files and 180 controllers went invisible in one commit with no signal.
Restoring the old filter on top of this change drops discovery from 625
controllers / 83 namespaces to 445 / 72.
The path filter bought nothing a content match does not: a file with no
ControllerSchema literal contributes nothing either way. Dropped it.
(b) percent = expected.size === 0 ? 100 : ... turned "measured nothing" into a
pass. On this tree the old script prints, verbatim:
| channels | channels | 0/0 | 100.0% | - |
| composio | composio | 0/0 | 100.0% | - |
| threads | threads | 0/0 | 100.0% | - |
| tools | tools | 0/0 | 100.0% | - |
| memory_sources | memory_sources | 0/0 | 100.0% | - |
indistinguishable from genuine full coverage. A namespace named in MODULES
with no discovered controllers is now a hard failure with its own message,
which says explicitly that nothing was measured.
channels is the case that made this load-bearing rather than theoretical:
its 20 controllers are declared in the vendored tinychannels-bus crate as
ChannelControllerSchema literals, and openhuman's adapter only maps them
across with a dynamic namespace field no static scan can read. Added that
crate as a second schema root, as
app/src/services/__tests__/rpcMethods.test.ts already does for the same
reason. channels now measures 18/20.
(c) MODULES was the SCOPE of the check, so a namespace nobody added a line for
was never measured at any threshold: webhooks, skill_runtime, subagent,
mcp_setup, flows, skills, cron, voice, workflow_run, team, billing, medulla
and ~40 more. MODULES is now presentational grouping only; every discovered
namespace is measured whether or not it is listed. A list you must remember
to extend is a list that silently stops covering things.
Honest numbers, not tuned -- threshold left at 90:
before: 17 namespaces, 4 failing
after : 83 namespaces, 625 controllers, 359 named by an e2e target (57.4%),
62 namespaces below 90%, 39 of them at 0%
The worst are whole namespaces with no Rust e2e at all: webhooks 0/13,
learning 0/11, medulla 0/9, session_db 0/6, skill_runtime 0/6, mcp_setup 0/6,
socket 0/5, memory_goals 0/5, test_support 0/5.
Also documented, not fixed: coverage is a string match, so a method NAMED by an
e2e target counts as covered without being provably invoked. Measured both
cheap tightenings before leaving it -- comment-only credit is exactly zero
today (390 methods with comments, 390 without), and bare-list-entry credit is
not separable by line shape, because rustfmt puts a long call's method argument
on its own line and a list element looks identical. Separating them needs an AST.
scripts/__tests__/coverage-script-help.test.mjs still passes.
Contributor
|
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; 7 remain after this review. 📝 WalkthroughWalkthroughThe coverage script discovers controller schemas from OpenHuman and vendored TinyChannels Rust sources. It supports two schema types, groups all discovered namespaces, reports aggregate totals, and fails for missing configured namespaces or rows below the coverage threshold. ChangesController coverage validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change corrects CI coverage reporting without changing product behavior or runtime code; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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: f1ba9f989f
ℹ️ 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".
| for (const root of SCHEMA_ROOTS) { | ||
| for (const file of walk(root, (f) => f.endsWith('.rs'))) { | ||
| const text = read(file); | ||
| const constNamespace = text.match(/const\s+NAMESPACE:\s*&str\s*=\s*"([a-z_]+)"/)?.[1]; |
There was a problem hiding this comment.
When a split schema file imports NAMESPACE from its parent rather than declaring it locally, this per-file lookup returns undefined, causing every controller in that file to be skipped. Both memory/schema/schema_schema_part_01.rs and _part_02.rs have this shape, so 25 registered memory_tree controllers disappear and the script incorrectly reports that namespace as 5/5 (100%); resolve the namespace from the owning module or otherwise include inherited constants.
Useful? React with 👍 / 👎.
| // `ChannelControllerSchema` is the vendored bus crate's equivalent shape. | ||
| for (const match of text.matchAll(/(?:Channel)?ControllerSchema\s*\{([\s\S]*?)\n\s*\}/g)) { | ||
| const block = match[1]; | ||
| const namespaceToken = block.match(/namespace:\s*(?:NAMESPACE|"([a-z_]+)")/); |
There was a problem hiding this comment.
When a controller namespace contains a digit, this pattern fails to capture it, so the new all-namespace gate silently omits the registered web3_swap, web3_bridge, web3_dapp, and x402 surfaces. Those files define ten controllers in total, including methods already referenced by Rust E2E tests, but none appears in the generated table or totals; accept the same alphanumeric namespace vocabulary that the method matcher already supports.
Useful? React with 👍 / 👎.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0188 · 51,906 in / 5,667 out · 10,270 cached (20%) · openrouter/openai/text-embedding-3-small, z-ai/glm-5.2, deepseek/deepseek-v4-flash · 464 embedded
critique: $0.0158 · 14,432 in / 5,303 out · 10,270 cached (71%) · z-ai/glm-5.2
security: $0.0012 · 15,046 in / 95 out · 0 cached (0%) · deepseek/deepseek-v4-flash
tests: $0.0012 · 14,959 in / 145 out · 0 cached (0%) · deepseek/deepseek-v4-flash
description: $0.0006 · 7,469 in / 124 out · 0 cached (0%) · deepseek/deepseek-v4-flash
How this change flows1 changed behaviour across 4 relationships. 5 surrounding behaviours are shown (60 graph nodes walked). 53 further behaviours left out to keep the diagram readable. flowchart LR
n0["collectSchemaMethods<br/>changed"]:::changed
n1["row"]:::impacted
n2["covered"]:::impacted
n3["namespace"]:::impacted
n4["declaredButMissing"]:::impacted
n5["totalCovered"]:::impacted
n0 -->|uses| n3
n2 -->|uses| n1
n4 -->|uses| n3
n5 -->|uses| 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
M3gA-Mind
added a commit
to M3gA-Mind/openhuman
that referenced
this pull request
Sep 2, 2026
…and the e2e gate Three behaviours merged in the last week had no test that would fail if the fix were reverted. Each test below drives the changed code path and was revert-checked against the specific hunk it covers. - tinyhumansai#5795 — the auth profile store is written owner-only. Driven through `auth.store_provider_credentials` rather than `AuthProfilesStore`, so the RPC path acquiring a different writer is caught too. The assertion is umask-sensitive by nature, so the test probes the ambient umask first and says so rather than reporting a pass it did not earn. - tinyhumansai#5947 — `validate_only` is a dry run for the stt workload, not only tts. A reserved slug is used because it is the one dry-run answer fully determined without a network call. - tinyhumansai#5936 — the domain e2e coverage gate itself. Its only prior test asserted `--help` output, so all three defects it fixed were unpinned. Fixture-driven cover for part-file discovery, the empty-namespace failure, and measuring a namespace MODULES does not name. Refs tinyhumansai#5795, tinyhumansai#5947, tinyhumansai#5936
M3gA-Mind
added a commit
to M3gA-Mind/openhuman
that referenced
this pull request
Sep 2, 2026
…and the e2e gate Three behaviours merged in the last week had no test that would fail if the fix were reverted. Each test below drives the changed code path and was revert-checked against the specific hunk it covers. - tinyhumansai#5795 — the auth profile store is written owner-only. Driven through `auth.store_provider_credentials` rather than `AuthProfilesStore`, so the RPC path acquiring a different writer is caught too. The assertion is umask-sensitive by nature, so the test probes the ambient umask first and says so rather than reporting a pass it did not earn. - tinyhumansai#5947 — `validate_only` is a dry run for the stt workload, not only tts. A reserved slug is used because it is the one dry-run answer fully determined without a network call. - tinyhumansai#5936 — the domain e2e coverage gate itself. Its only prior test asserted `--help` output, so all three defects it fixed were unpinned. Fixture-driven cover for part-file discovery, the empty-namespace failure, and measuring a namespace MODULES does not name. Refs tinyhumansai#5795, tinyhumansai#5947, tinyhumansai#5936
M3gA-Mind
added a commit
to M3gA-Mind/openhuman
that referenced
this pull request
Sep 2, 2026
…and the e2e gate Three behaviours merged in the last week had no test that would fail if the fix were reverted. Each test below drives the changed code path and was revert-checked against the specific hunk it covers. - tinyhumansai#5795 — the auth profile store is written owner-only. Driven through `auth.store_provider_credentials` rather than `AuthProfilesStore`, so the RPC path acquiring a different writer is caught too. The assertion is umask-sensitive by nature, so the test probes the ambient umask first and says so rather than reporting a pass it did not earn. - tinyhumansai#5947 — `validate_only` is a dry run for the stt workload, not only tts. A reserved slug is used because it is the one dry-run answer fully determined without a network call. - tinyhumansai#5936 — the domain e2e coverage gate itself. Its only prior test asserted `--help` output, so all three defects it fixed were unpinned. Fixture-driven cover for part-file discovery, the empty-namespace failure, and measuring a namespace MODULES does not name. Refs tinyhumansai#5795, tinyhumansai#5947, tinyhumansai#5936
senamakel
added a commit
to HDZTony/openhuman
that referenced
this pull request
Sep 11, 2026
…verage-gate\n\nfix(ci): the domain e2e coverage gate was measuring a seventh of the surface\n
senamakel
pushed a commit
to HDZTony/openhuman
that referenced
this pull request
Sep 11, 2026
…and the e2e gate\n\nThree behaviours merged in the last week had no test that would fail if the\nfix were reverted. Each test below drives the changed code path and was\nrevert-checked against the specific hunk it covers.\n\n- tinyhumansai#5795 — the auth profile store is written owner-only. Driven through\n `auth.store_provider_credentials` rather than `AuthProfilesStore`, so the\n RPC path acquiring a different writer is caught too. The assertion is\n umask-sensitive by nature, so the test probes the ambient umask first and\n says so rather than reporting a pass it did not earn.\n- tinyhumansai#5947 — `validate_only` is a dry run for the stt workload, not only tts.\n A reserved slug is used because it is the one dry-run answer fully\n determined without a network call.\n- tinyhumansai#5936 — the domain e2e coverage gate itself. Its only prior test asserted\n `--help` output, so all three defects it fixed were unpinned. Fixture-driven\n cover for part-file discovery, the empty-namespace failure, and measuring a\n namespace MODULES does not name.\n\nRefs tinyhumansai#5795, tinyhumansai#5947, tinyhumansai#5936\n
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
scripts/check-domain-e2e-coverage.mjs, the gate that guards Rust e2e controller coverage.Problem
Three separate holes, each of which alone makes the gate unreliable:
(a) It cannot see most controllers.
collectSchemaMethods()only reads files matching/(^|\/)schemas?(\.rs|\/)/. The #5856/#5857include!split movedControllerSchemaliterals into*_part_NN.rssiblings that do not match, hiding 180 controllers across 12 namespaces.(b) Measuring nothing scores 100%.
percent = expected.size === 0 ? 100 : …means a namespace whose controllers all became invisible reports 100% — indistinguishable in the gate's own output from genuinely full coverage. Four namespaces were doing exactly that:composio0/23,threads0/20,memory_sources0/17,tools0/6.(c)
MODULESomits most of the product. ~50 namespaces are not listed at all, so they are never measured at any threshold —webhooks0/13,skill_runtime0/6,subagent0/2,mcp_setup0/6,voice2/17,flows19/36.Solution
Discovery over the same roots the gate already means to cover, an explicit failure when a namespace discovers zero controllers, and a
MODULESextension.The honest post-fix numbers are much worse than the pre-fix ones, and that is the point: 359/625 controllers = 57.4% across 83 namespaces (up from 17 namespaces visible). 62 namespaces fall below the unchanged 90% threshold, 39 of them at exactly 0%.
The threshold is untouched and no ratchet was added. Making the gate green today is the failure mode being removed. It should stay red and honest until the coverage is real — how to stage that is a maintainer decision, and the repo's own precedent (
scripts/kernel-floor.limits, "only goes DOWN") is the obvious tool if one is wanted.Also measured and deliberately not changed: comment-only credit is a provable no-op today (390 methods found with comments, 390 without), and separating bare-list-entry credit from real invocations needs an AST — rustfmt puts a long call's method argument on its own line, so
"openhuman.flows_create",is indistinguishable from a list element by regex.Submission Checklist
N/A: this changes a CI script, not product code; its behaviour is verified by the measured before/after controller counts in the Problem section.N/A: CI script, not covered by the product coverage lanes.N/A: behaviour-only change, no feature rows affected.N/A: no feature IDs affected.N/A: no product surface changes.Closes #NNN—N/A: intentionally not auto-closing; see Related.Impact
Related
fix/rpc-methods-drift-guardfixes the same blindness inapp/src/services/__tests__/rpcMethods.test.ts.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/domain-e2e-coverage-gateValidation Run
pnpm --filter openhuman-app format:check—N/A: no app/ files changed.pnpm typecheck—N/A: no TypeScript changed(the script is.mjs).N/A: no .rs files changed.N/A: app/src-tauri not touched.Validation Blocked
command:re-running the gate after the most recent rebaseerror:not attempted — local CI runs are disabled by standing instructionimpact:the numbers quoted were measured before the rebase onto current main; the script's logic is unchanged since, so they should hold, but CI is the authority.Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit