Skip to content

test(mcp_registry_e2e): drop the second workspace from the 401 hint test - #8455

Merged
M3gA-Mind merged 1 commit into
tinyhumansai:mainfrom
M3gA-Mind:test/mcp-registry-e2e-tenant-contract
Oct 27, 2026
Merged

M3gA-Mind merged 1 commit into
tinyhumansai:mainfrom
M3gA-Mind:test/mcp-registry-e2e-tenant-contract

Conversation

@M3gA-Mind

@M3gA-Mind M3gA-Mind commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • mcp_registry_e2e's 401 test now uses one workspace. The second-workspace assertions that failed CI Fast intermittently (tests/mcp_registry_e2e.rs:154, e.g. run 40492517510 on fix(credentials): restore the API key when a sign-out fails after reload #8430) are dropped.
  • The test is renamed auth_failure_surfaces_401_reason_as_a_typed_hint, since it no longer claims cross-workspace isolation.
  • Test-only. No production code changes.

Problem

  • The test built two un-tenanted workspaces in one process and expected workspace B to have no auth hint for A's server. The product doesn't promise that:
    • The host binds module storage per tenant profile (storage_host::current_module_scope → module_scope(profile, "tinymcp")).
    • TinyMCP's BusProvider::ports_for answers the same ports for every data directory.
    • So un-tenanted workspaces in one process share TinyMCP's installs.
  • When B's service opened (on its first call, the assertion itself), its boot pass (connect_installed) connected A's enabled server, got the 401 and recorded credential_required in B's own failures. The status call raced that connect, so the assertion failed intermittently, even with the test alone in its own nextest process.
  • Separately, the second fresh_workspace_config call replaced the process-wide storage backend (storage::install) before A wrote its server, so A's data landed in B's backend.

Solution

  • The test uses one workspace and one fresh_workspace_config call, so the backend the server is written to is the one the test reads.
  • Per-tenant isolation stays covered where the contract lives: modules::storage_host module_scopes_are_distinct_per_module_and_per_tenant and a_module_scope_is_the_explicit_contexts_profile_across_tasks. The test's doc comment points there.
  • Independent of fix(modules): prepare module storage callbacks on the module runtime #8454, which fixes a different one-process failure in this target (the dead storage-callback peer at :65).

Submission Checklist

  • Tests added or updated: one e2e test is narrowed to what it can prove. Its happy path and failure path (the 401 surfaces as a typed hint and hides the response text) are kept.
  • N/A: Diff coverage ≥ 80%. Test-only change, no production lines.
  • N/A: Coverage matrix updated. No feature rows change.
  • N/A: Affected feature IDs. No matrix feature changes.
  • No new external network dependencies introduced
  • N/A: Manual smoke checklist. Not a release-cut surface.
  • N/A: Linked issue. Main-branch CI defect, no issue filed.

Impact

  • CI only: removes an intermittent CI Fast failure. No runtime, API or migration impact.

Related

  • Closes: N/A
  • Follow-up PR(s)/TODOs: none

AI Authored PR Metadata (required for Codex/Linear PRs)

Linear Issue

  • Key: N/A
  • URL: N/A

Commit & Branch

  • Branch: test/mcp-registry-e2e-tenant-contract
  • Commit SHA: bffd844

Validation Run

  • N/A: pnpm --filter openhuman-app format:check. No frontend change.
  • N/A: pnpm typecheck. No frontend change.
  • Focused tests: the CI Fast path with the lane's module env: nextest --locked -p openhuman-cli --features "$CLI_FEATURES" --test mcp_registry_e2e --retries 0, the full target, run 20 times. Every run "2 tests run: 2 passed": auth_failure_surfaces_401_reason_as_a_typed_hint 20/20, disabled_server_accepts_environment_update_without_reconnecting 20/20.
  • Rust fmt/check (if changed): cargo fmt --all --check clean.
  • N/A: Tauri fmt/check. No shell change.

Validation Blocked

  • command: N/A
  • error: N/A
  • impact: N/A

Behavior Changes

  • Intended behavior change: none. Test-only.
  • User-visible effect: none.

Parity Contract

  • Legacy behavior preserved: yes. No production code changed.
  • Guard/fallback/dispatch parity checks: N/A

Duplicate / Superseded PR Handling

  • Duplicate PR(s): none
  • Canonical PR: this
  • Resolution (closed/superseded/updated): N/A

Summary by CodeRabbit

  • Tests
    • Updated authentication checks to verify that a failed connection provides a credential-required hint without displaying the underlying error text.
    • The authentication scenario now uses a single workspace, with tenant-isolation coverage handled separately. These changes update automated verification; they do not indicate a change to the behavior available in the product.

Un-tenanted workspaces in one process share TinyMCP's installs: the host
binds module storage per tenant profile, and TinyMCP's bus provider answers
the same ports for every data directory. The second workspace's service
therefore boot-connected the first workspace's server and could record its
own 401 hint before the status call, which failed the cross-workspace
assertion intermittently in CI. The second fresh_workspace_config call also
replaced the process storage backend before the first workspace wrote to it.

Test one workspace; per-tenant isolation is covered by the storage host's
module-scope tests.
@tinysweeper

tinysweeper Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Reviewed head: bffd844a5cb3876ad7502a16aef8d26578f53c75

Verdict: Ready for maintainer review · Priority: none
Coverage: Reviewed 1 of 1 files.
Pull request purpose (title): test(mcp_registry_e2…

Diagnostics and supporting details

Full summary

Pull request purpose (title): test(mcp_registry_e2e): drop the second workspace from the 401 hint test. Tiny Sweeper completed its review; deterministic results follow.

Review snapshot

Change surface Files Review signal Count
Production 0 Active findings 0
Tests 1 Noted findings 0
Documentation 0 Resolved findings 0
Configuration 0 Pending checks/questions 0

Completeness: Complete
Test assessment: Test coverage is assessed from changed tests and lane evidence; execution is not claimed without trusted check data.

What changed

The test still enables the remote server, asserts connect is an error on 401, asserts auth_hint_for_config returns Some("credential_required"), and asserts last_error_for_config is None so the response text stays hidden.

Features

None identified with supported citations.

Tests

  • unchanged — disabled_server_accepts_environment_update_without_reconnecting is not modified by this diff.: Listed alongside the changed test in validation; outside this pull request's changes. (tests/mcp_registry_e2e.rs async fn disabled_server_accepts_environment_update_without_reconnecting() {)

Findings

No active actionable findings.

Before merge

None.

Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The critique lane found the test self-consistent and exercising the same code paths as before, minus the second-workspace negative assertions, and did not raise a defect.
  • Lane summary: The change collapses a two-workspace isolation test into a single-workspace test and adds a doc comment justifying it. The removed assertions (`auth_hint_for_config(&config_b, ...)` and `last_error_for_config(&config_b, ...)` are `None`) were the only cross-workspace isolation coverage in this test, and the replacement doc comment points at `modules::storage_host` tests that cover per-tenant module scope, not per-workspace MCP auth-hint/status isolation. I could not verify the cited tests exist at the current head, so I am not raising that as a defect. What remains is a self-consistent test that exercises the same code paths it did before, minus the second-workspace negative assertions. _The code index is behind this pull request (indexed at `9c1eafb38cd4`), so retrieved context may be out of date._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The security lane reports a test-only edit with no security-relevant sink, network, subprocess or permission change and no findings.
  • Lane summary: The change narrows an existing MCP registry e2e test to a single workspace and drops the cross-workspace assertions, with a comment explaining why (shared module storage per tenant would make a second workspace boot-connect the server). It is a test-only edit; the remaining assertions still exercise the typed 401 auth hint. No security-relevant sink, network, subprocess or permission change is introduced, and the repo's test-placement rules are satisfied (the file is already an E2E test under tests/). No findings. _The code index is behind this pull request (indexed at `9c1eafb38cd4`), so retrieved context may be out of date._

tests

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Lane summary: No behavioural change: nothing outside documentation, configuration and tests.

commits

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The commits lane reports no sensitive content in committed files.
  • Lane summary: No sensitive content in committed files. _The code index is behind this pull request (indexed at `9c1eafb38cd4`), so retrieved context may be out of date._

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The description lane reports that the pull request body matches the diff, including the rename and rationale, with no production change described.
  • Lane summary: The diff narrows the 401 e2e test to a single workspace, removes the cross-workspace assertions, and renames the test to match; the doc comment explains why and the pull request body describes exactly that with no production change. The description matches the diff and the change looks sound. _The code index is behind this pull request (indexed at `9c1eafb38cd4`), so retrieved context may be out of date._
Evidence and run details
  • Models: ladder/vectors-oai3, deepseek-v4.1-flash
  • Spend: $0.003088
  • Tokens: 88630 input · 2326 output · 8448 cached · 185 embedding
  • Updated: 2026-10-27T15:51:17Z
Head State Pass summary
bffd844a5cb3 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-10-27T15:51:17Z)

tinysweeper 0.1.2

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-27T15:52:47.013643Z bffd844 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: dd6cb186-85ae-4a00-bb4d-cc4fba4b32f8
Commits

Reviewing files that changed from the base of the PR and between cb4404d and bffd844.

Files selected for processing: 1
  • tests/mcp_registry_e2e.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Walkthrough

The 401 authentication test now uses one workspace. It retains checks for connection failure, the credential_required hint, and hidden last-error text. It no longer checks cross-workspace leakage.

Changes: MCP registry 401 test

Layer / File(s) Summary
Single-workspace 401 test
tests/mcp_registry_e2e.rs
The test name and comment describe its single-workspace scope. It retains the connection-failure, typed-hint, and hidden-error assertions, and removes the second workspace and its assertions.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related PRs

Suggested labels: rust-core, priority: p3

Merge Risk: Minimal · up to bffd8

The narrowed test continues to validate the intended 401 behavior, with no merge-blocking risk identified.

Pre-merge checks: Passed 5
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main change: removing the second workspace from the 401 hint test.
Docstring Coverage Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within survi…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit checks the 401 reply,
One workspace hops beneath the sky.
A typed hint lands, the error stays,
No second workspace joins the maze.
The test hops on its single way.

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

@tinysweeper tinysweeper Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tinysweeper found nothing blocking. Approving.

Diagnostics

Total spend: $0.0031

@tinysweeper tinysweeper Bot added the priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. label Oct 27, 2026
@coderabbitai coderabbitai Bot added the rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure. label Oct 27, 2026
@M3gA-Mind
M3gA-Mind merged commit 2787a28 into tinyhumansai:main Oct 27, 2026
26 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. rust-core Core Rust runtime in src/: CLI, core_server, shared infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant