Skip to content

fix(ci): fail the MCP write audit at the module seam in the diagnostics test - #7761

Merged
senamakel merged 2 commits into
tinyhumansai:mainfrom
M3gA-Mind:fix/main-health-mcp-audit-diag
Oct 13, 2026
Merged

senamakel merged 2 commits into
tinyhumansai:mainfrom
M3gA-Mind:fix/main-health-mcp-audit-diag

Conversation

@M3gA-Mind

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

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes openhuman-cli::raw_coverage_all tool_registry_approval_raw_coverage_e2e::tool_registry_diagnostics_reports_config_and_audit_store_failures in CI Fast on main (run 38441930612; same failure on main 5a5c68ab00 in run 38447185171).
  • The test now makes the MCP write audit unavailable at the module seam the diagnostics read, rather than through a broken workspace that no longer reaches the audit.

Problem

FAIL raw_coverage_all tool_registry_approval_raw_coverage_e2e::tool_registry_diagnostics_reports_config_and_audit_store_failures
  assertion `left == right` failed
    left: Some(0)
   right: None

Since 605ca6e the write audit lives in the TinyMCP module (mcp::audit::list_writes → McpConfiguredClient). In tinymcp v0.18.0 the module picks its storage provider once per process, at load: host storage if it can connect, otherwise an in-memory default (tinybus_module/mod.rs setup). A file used as workspace_dir therefore no longer breaks the audit. list_writes succeeds with zero rows, and the diagnostics report Some(0).

Solution

  • The broken config also sets modules.enabled = false. McpConfiguredClient::for_config returns Unavailable, list_writes errors, and the existing assertions hold as written: recent_rows == None and a non-empty last_error.
  • That is the behaviour this test owns: how diagnostics surface an audit they cannot read.
  • Storage failure inside the audit is tinymcp's to test, and tinymcp already does (crates/tinymcp/src/storage/failing.rs).
  • A failing installed backend was considered and rejected. Because the provider is fixed at the module's first load, it would only take effect if this test loaded TinyMCP first. Under libtest's shared process it could also fault the module for every other MCP test in raw_coverage_all.
  • No assertion changed. The config-load failure half of the test is untouched.

Submission Checklist

  • Tests added or updated: the diagnostics e2e test injects the audit failure at the module seam.
  • (N/A) Diff coverage ≥ 80%: test-only change, no Rust or app source changed.
  • (N/A) Coverage matrix updated: test fixture only, no feature rows change.
  • (N/A) Affected feature IDs listed: no matrix row changes.
  • No new external network dependencies introduced.
  • (N/A) Manual smoke checklist: no release-cut surface touched.
  • (N/A) Linked issue: fixes red CI on main; no tracking issue.

Impact

  • Tests only.

Related


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

N/A

Validation Run

  • rustfmt --check on the changed file.
  • (N/A) Rust tests: not run locally; CI runs them.

Summary by CodeRabbit

  • Tests
    • Updated an audit-failure test scenario to disable modules before running diagnostics.

…cs test

Since 605ca6e the MCP write audit lives in the TinyMCP module, which
picks its storage provider once per process at load. A file as the
workspace no longer reaches it, so diagnostics reported recent_rows
Some(0) instead of the expected None.

The test now disables modules on the broken config, so the module client
is unavailable and the diagnostics report the audit as failed
(recent_rows None, non-empty last_error). That is the surface this test
owns. Storage failures inside the audit are tinymcp's to cover
(storage/failing.rs).
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 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-13T19:51:14.207406Z b9fefd6 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 13, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Warning

Review limit reached

  • Run on-demand review

This review includes 1 billable file and costs up to $0.25.

Or wait 15 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 60cbfcf8-1185-47f0-8d62-07a2495b3a82
📥 Commits

Reviewing files that changed from the base of the PR and between b9fefd6 and 66898a9.

📒 Files selected for processing: 1
  • tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4e4ceb50-2aab-46d6-8e05-eb8029e728f0
📥 Commits

Reviewing files that changed from the base of the PR and between aa44c25 and b9fefd6.

📒 Files selected for processing: 1
  • tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs

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

📝 Walkthrough

The audit-failure test now disables modules on its configuration before calling diagnostics. The fixture also explains why the workspace path no longer exercises the TinyMCP audit provider.

Changes: Audit failure fixture

Layer / File(s) Summary
Configure the audit-failure fixture
tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs
The fixture creates a mutable configuration and disables modules before calling diagnostics. Its comment explains that workspace loading no longer reaches the TinyMCP audit provider.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: senamakel

Merge Risk: ⚪ Minimal · up to b9fef

The diagnostics test exercises the intended unavailable-audit state. No merge-blocking issue is evident from this change.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Linked Issues check Warning Directly linked open issue #7759 requires four other raw_coverage_all fixes: host storage for the two Composio tests, approval failure injection at the opened database, and the curated round20 tool-… Implement the four coding objectives from #7759 in this PR, with their required test changes and unchanged failure assertions where specified. Alternatively, remove the direct link if this PR is intentionally limited to the excluded diagnos…
Out of Scope Changes check Warning The PR changes tool_registry_diagnostics_reports_config_and_audit_store_failures. Issue #7759 explicitly lists this test as not included and assigns it to a separate failure-injection change. The ch… Move the diagnostics test fix to a separate PR with the appropriate issue link, or update the linked issue scope and acceptance criteria before including this change.
Passed Passed checks: 3 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies the test-only change: making the MCP write audit fail at the module seam in the diagnostics test. It is specific and matches the pull request objective.
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. Analyzed 1 functions across 1 files.
Full details: Linked Issues check

Explanation

Directly linked open issue #7759 requires four other raw_coverage_all fixes: host storage for the two Composio tests, approval failure injection at the opened database, and the curated round20 tool-list expectation. The reviewed diff changes only tool_registry_diagnostics_reports_config_and_audit_store_failures, which #7759 explicitly excludes. The required four fixes are therefore not implemented by this PR.

Resolution

Implement the four coding objectives from #7759 in this PR, with their required test changes and unchanged failure assertions where specified. Alternatively, remove the direct link if this PR is intentionally limited to the excluded diagnostics test.

Full details: Out of Scope Changes check

Explanation

The PR changes tool_registry_diagnostics_reports_config_and_audit_store_failures. Issue #7759 explicitly lists this test as not included and assigns it to a separate failure-injection change. The change is therefore outside the scope of the directly linked issue, even though it is a test-only fix.

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

A rabbit checks the test setup with care,
Modules rest, while diagnostics run there.
The workspace path waits by the door,
The audit provider is reached no more.
One small fixture change completes the chore.

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

coderabbitai[bot]
coderabbitai Bot previously approved these changes Oct 13, 2026
The test disables modules to fail the write audit at the module seam;
check last_error names that seam ("MCP module unavailable", from
mcp/audit/mod.rs) instead of only being non-empty.
@senamakel
senamakel merged commit d6d5e4b into tinyhumansai:main Oct 13, 2026
10 of 13 checks passed
@tinysweeper

tinysweeper Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Tiny Sweeper review

Tiny Sweeper completed its review; deterministic results follow.

State: Ready for maintainer review
Priority: none
Reviewed head: 66898a9d5266
Updated: 2026-10-14T01:17:31Z

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: No supported feature-to-test mapping was available; this does not mean tests are absent or passed.

Features

  • Modified — Audit-store failure injected at the TinyMCP module seam: The diagnostics case now builds `broken_audit_config` as a mutable `Config` and sets `modules.enabled = false`, so the audit half fails at the module client boundary rather than via a broken workspace; this follows the write audit moving into the TinyMCP module, which resolves its storage provider once per process at load, meaning a broken workspace no longer reaches it. (tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs, tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs#async fn tool_registry_diagnostics_reports_config_and_audit_store_failures() {)
  • Modified — `last_error` assertion tightened to the module-unavailable prefix: The assertion on `diagnostics.value.mcp_write_audit.last_error` no longer accepts any non-empty string; it requires the text to contain `"MCP module unavailable"`, which is the prefix `mcp::audit::list_writes` wraps around `McpConfiguredClient::for_config` failures, and which a `Config` with `modules.enabled = false` produces via `ModuleCallError::Unavailable`. The companion assertions that `enabled` is true and `recent_rows` is `None` remain consistent with the health error branch. (tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs, tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs#async fn tool_registry_diagnostics_reports_config_and_audit_store_failures() {)
  • Modified — Explanation comment added for the retargeted injection: An inline comment records why a broken workspace no longer reaches the audit and why the test fails at the module seam instead. Its claims about per-process storage-provider selection and about tinymcp's `storage/failing.rs` covering storage failures could not be confirmed because the vendored submodule is not checked out, but they do not affect the assertions. (tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs, tests/raw_coverage/tool_registry_approval_raw_coverage_e2e.rs#async fn tool_registry_diagnostics_reports_config_and_audit_store_failures() {)

Tests

No supported feature-to-test mapping was produced. Test execution is not inferred.

Findings

No active actionable findings.

Before merge

None.

Agent review details

critique

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: No finding the critique lane would block on; the retargeted injection and the three assertions are reported as consistent with `mcp::audit::list_writes` wrapping `McpConfiguredClient::for_config` failures with the `MCP module unavailable` prefix and with `mcp_write_audit_health` returning `enabled: true` and `recent_rows: None` on the error arm.
  • Lane summary: The test change replaces a workspace-based audit failure with a module-gate failure, which matches the code under test: `mcp::audit::list_writes` wraps `McpConfiguredClient::for_config`'s error as "MCP module unavailable: …", and `for_config` returns `ModuleCallError::Unavailable` when `config.modules.enabled` is false, so `recent_rows: None` and the new `last_error` assertion hold. No defect found. _The code index is behind this pull request (indexed at `ff734fb3ff26`), so retrieved context may be out of date._

security

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: No security-relevant change reported: the diff only adapts an existing E2E assertion to the audit log's relocation into the TinyMCP module, and no untrusted input, auth, network, subprocess or CI-permission surface is touched.
  • Lane summary: The change only adapts an existing E2E assertion to the audit log's relocation into the TinyMCP module: it disables the module in the test config to force the new failure seam and narrows the expected error string accordingly. The quoted code matches what `mcp_write_audit_health` reads through `mcp::audit`, and no untrusted input, auth, network, subprocess or CI-permission surface is touched. _The code index is behind this pull request (indexed at `ff734fb3ff26`), so retrieved context may be out of date._

tests

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Positive: The tests lane reports no behavioural change, as nothing outside documentation, configuration and tests is touched.
  • Lane summary: No behavioural change: nothing outside documentation, configuration and tests.

commits

  • Conclusion: Neutral
  • Scope reviewed: all assigned evidence
  • Positive: The commits lane found nothing sensitive in what this pull request commits, and the title aligns with the change of failing the MCP write audit at the module seam.
  • Lane summary: Nothing sensitive found in what this pull request commits.

description

  • Conclusion: Success
  • Scope reviewed: all assigned evidence
  • Positive: The code change itself looks consistent with the PR's stated intent of switching the audit-failure injection from a broken workspace to `modules.enabled = false`; only the description is inaccurate, because its claim that no assertion changed does not match the diff that tightens the `last_error` assertion.
  • Lane summary: The change is a test-only fix that reproduces the audit-unavailable condition at the module seam (modules.enabled = false) instead of through a broken workspace directory, and tightens the last_error assertion to the "MCP module unavailable" message. The quoted production code in crates/openhuman-core/src/mcp/audit/mod.rs does produce exactly that message when McpConfiguredClient::for_config fails, so the new assertion matches the path the config now triggers; no assertion was removed and no production behaviour changes. _The code index is behind this pull request (indexed at `ff734fb3ff26`), so retrieved context may be out of date._
Evidence and run details
  • Models: ladder/vectors-oai3, deepseek-v4.1-flash
  • Spend: $0.000414
  • Tokens: 118600 input · 5073 output · 27136 cached · 93 embedding
Head State Pass summary
66898a9d5266 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-10-14T00:15:23Z)
66898a9d5266 ready for maintainer review 0 active finding(s), 0 resolved finding(s) (at 2026-10-14T01:17:31Z)

tinysweeper 0.1.2

@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.

             $0.0019 · 134,915 in / 26,777 out · 36,992 cached (27%) · ladder/vectors-oai3, deepseek-v4.1-flash · 93 embedded
critique:    $0.0010 · 55,712 in  / 14,155 out · 17,536 cached (31%) · deepseek-v4.1-flash
security:    $0.0003 · 51,629 in  / 4,100 out  · 14,336 cached (28%) · deepseek-v4.1-flash
description: $0.0004 · 25,615 in  / 5,542 out  · 5,120 cached (20%)  · deepseek-v4.1-flash

@tinysweeper tinysweeper Bot added the priority: p3 Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect. label Oct 14, 2026
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants