Skip to content

Harden MCP execution and make import/export more reliable - #207

Merged
Mike Krüger (mkrueger) merged 12 commits into
mainfrom
dev/mkrueger/mcp-export-hardening
Sep 25, 2026
Merged

Mike Krüger (mkrueger) merged 12 commits into
mainfrom
dev/mkrueger/mcp-export-hardening

Conversation

@mkrueger

@mkrueger Mike Krüger (mkrueger) commented Sep 7, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Improve state safety, keep MCP activity visible, and make bulk file operations more reliable without including the RPM packaging changes from #201. This branch is based directly on main.

  • Serialize shell and MCP command execution, including nested shell calls, and preserve the existing cancellation behavior.
  • Include the connected account and navigation context in destructive MCP confirmations. Refuse execution when connection or navigation changes while awaiting approval, including navigating away and back.
  • Keep echoing MCP command lines and recording them in history so their activity stays visible in the shell. Echoing and the line editor no longer fail the command they announce when the host has no ANSI terminal. History remains complete and replayable, including connection strings; document its sensitivity.
  • Write exports through a temporary file in the destination directory and replace the destination only after successful completion. Preserve existing files on failures and cancellation, respect overwrite protection, dispose query iterators, and stop requesting pages once --max is reached.
  • Read CSV imports incrementally instead of loading the whole file into memory, and parse them with CsvHelper 33.1.0 so a malformed record aborts with its physical line number instead of silently absorbing the rest of the file. Files that previously imported without an error can now be rejected. Spool CSV export documents to private temporary storage rather than retaining all documents in memory, preserving the complete dynamic column set. Flush JSON array output incrementally.
  • Add regression coverage and update README, command reference, MCP security documentation, history documentation, and localized CSV errors.

Operational Notes

  • MCP clients still share one connection and navigation context; explicit database/container arguments remain recommended. Confirmation waits do not hold the execution lock.
  • CSV export requires temporary disk space for its JSON spool in addition to the destination export.
  • Imports are not transactional: earlier writes remain if a later record fails. Use --dry-run to validate the entire input first.
  • Abrupt process termination can leave an unfinished destination-directory export temporary file.
  • Adds a dependency on CsvHelper 33.1.0.

Serialize shell and MCP execution and invalidate destructive confirmations after context changes. Preserve replayable interactive history while omitting MCP command echo and history entries.

Write exports through temporary files, stop pagination at the requested limit, dispose iterators, and spool CSV exports to disk. Stream CSV imports with CsvHelper and report malformed record line numbers.

Add regression tests and document history policy, context sharing, and export guarantees. Offline validation: 2315 passed, 2 interactive-console tests skipped.
Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Commands/ExportCommand.cs Fixed
Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Commands/ExportCommand.cs Fixed
Comment thread CosmosDBShell.Tests/CommandTests/ExportCommandTests.cs Fixed
Comment thread CosmosDBShell.Tests/CommandTests/ExportCommandTests.cs Fixed
Comment thread CosmosDBShell.Tests/CommandTests/ExportCommandTests.cs Fixed
Comment thread CosmosDBShell.Tests/CommandTests/ExportCommandTests.cs Fixed
Comment thread CosmosDBShell.Tests/CommandTests/ExportCommandTests.cs Fixed
Comment thread CosmosDBShell.Tests/CommandTests/ExportCommandTests.cs Fixed
@github-code-quality

github-code-quality Bot commented Sep 7, 2026 •

Copy link
Copy Markdown

Code Coverage Overview

Languages: C#

C# / code-coverage/dotnet

The overall line coverage in commit 749a5c9 in the dev/mkrueger/mcp-exp... branch remains at 63%, unchanged from commit 2d3ee88 in the main branch.

Show a line coverage summary of the most impacted files.
File main 2d3ee88 dev/mkrueger/mcp-exp... 749a5c9 +/-
D:\a\CosmosDBSh...mportCommand.cs 46% 46% 0%
D:\a\CosmosDBSh...tementParser.cs 75% 75% 0%
D:\a\CosmosDBSh...lInterpreter.cs 66% 67% +1%
D:\a\CosmosDBSh...\RmDbCommand.cs 26% 28% +2%
D:\a\CosmosDBSh...olOperations.cs 90% 93% +3%
D:\a\CosmosDBSh...xportCommand.cs 36% 62% +26%

Updated September 24, 2026 13:07 UTC

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

It introduces a serialized execution semaphore that is not disposed and an export temp-file cleanup path that can mask the original failure, both of which should be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR hardens MCP tool execution against shared-shell state changes and improves import/export reliability and scalability, while updating user-facing documentation and adding regression tests across MCP, serialization, and bulk file operations.

Changes:

  • Serialize shell + MCP command execution and invalidate destructive MCP confirmations when connection/navigation context changes.
  • Make export atomic via destination-directory temp files, improve iterator/page-limit handling, and spool CSV exports to disk; stream CSV imports with CsvHelper and better error localization.
  • Update README/docs and add regression tests covering the new behavior.
File summaries
File Description
README.md Documents atomic export behavior, serialized MCP execution, and history sensitivity.
docs/navigation.md Documents sensitive history contents and MCP history/echo behavior.
docs/mcp.md Documents destructive confirmation context + invalidation and serialized execution model.
docs/commands.md Updates import/export behavior details (temp files, streaming, spooling, limits).
Directory.Packages.props Adds CsvHelper package version for streaming CSV import support.
CosmosDBShell/lang/en.ftl Adds localized message for invalid CSV record errors with line number.
CosmosDBShell/CosmosDBShell.csproj Adds CsvHelper reference to the main application project.
CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs Serializes MCP execution, adds context-aware confirmations, and refuses execution on context changes.
CosmosDBShell/Azure.Data.Cosmos.Shell.Core/ShellInterpreter.cs Introduces serialized execution gate and state version tracking.
CosmosDBShell/Azure.Data.Cosmos.Shell.Commands/ImportCommand.cs Switches CSV import parsing to streaming CsvHelper parser with physical line reporting.
CosmosDBShell/Azure.Data.Cosmos.Shell.Commands/ExportCommand.cs Implements atomic exports with temp files, incremental JSON array flushing, CSV spooling, iterator disposal, and page-limit enforcement.
CosmosDBShell.Tests/ToolOperationsCallToolTests.cs Adds tests for context invalidation and MCP echo/history behavior.
CosmosDBShell.Tests/Runtime/SerializedExecutionTests.cs Adds coverage for serialized execution behavior, nesting, failure, and cancellation.
CosmosDBShell.Tests/CommandTests/ImportCommandTests.cs Adds CSV streaming, cancellation, and malformed-record line number tests.
CosmosDBShell.Tests/CommandTests/ExportCommandTests.cs Adds tests for atomic export preservation, overwrite behavior, cancellation, page-boundary limits, and document lifetime.
Review details
  • Files reviewed: 15/15 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Commands/ExportCommand.cs
Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Commands/ExportCommand.cs
Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Core/ShellInterpreter.cs
Copilot AI review requested due to automatic review settings September 7, 2026 10:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A couple of small but concrete issues remain (notably best-effort temp-file cleanup not covering all failure modes, and a potentially misleading trace log message) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs:591

  • This trace log says "Invoking" even though the command may be refused later if the confirmation context changed; that can make diagnostics misleading. Consider logging this as a request rather than an invocation (or move the log to just before ExecuteCosmosCommandAsync after the version check).
  • Files reviewed: 15/15 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Commands/ExportCommand.cs
Copilot AI review requested due to automatic review settings September 7, 2026 10:53
@mkrueger

Copy link
Copy Markdown
Collaborator Author

Second review follow-up in 2e2a58d: the suppressed trace finding is fixed by logging request handling instead of claiming execution before context validation. The DirectoryNotFoundException thread was already covered by IOException handling; an explicit regression now verifies it. Validation: 59 focused export/MCP tests passed, both projects built without warnings. All current review threads have explanatory replies and are resolved.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The changes are cohesive, align with the stated behavior, and include strong regression coverage for the new execution/IO semantics.

Review details
  • Files reviewed: 15/15 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI review requested due to automatic review settings September 21, 2026 11:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The localization catalog is out of sync, causing clean verification to fail.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)

Comment thread CosmosDBShell/lang/en.ftl
Copilot AI review requested due to automatic review settings September 21, 2026 11:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified in the supplied review.

Review effort: Lite
Findings: 1 High severity

Open (1)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

All reviewed changes have approval readiness and no unresolved review comments.

Review effort: Lite
Findings: None

Resolved since last review (1)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

CSV cancellation remains unresponsive while a large record is being synchronously read and parsed.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Commands/ImportCommand.cs
Copilot AI review requested due to automatic review settings September 23, 2026 11:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

MCP commands are still echoed and recorded, directly contradicting the stated PR requirement.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)

Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs
Copilot AI review requested due to automatic review settings September 23, 2026 12:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

History persistence and synchronization remain incomplete for concurrent MCP and interactive activity.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity

Open (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Persist MCP history entries before session exit

docs/​navigation.md:239

MCP invocations are currently appended only to the in-memory history by PrintCommand; SaveHistory is called only after a later interactive command in RunAsync. An MCP-only session can therefore exit without writing any of its commands to cmd_history, contrary to this statement that they are recorded in the same replayable history. Persist the history when recording an MCP invocation and add a restart/persistence regression test.

Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Core/ShellInterpreter.cs
Copilot AI review requested due to automatic review settings September 23, 2026 12:42
Comment thread CosmosDBShell.Tests/CommandTests/ImportCommandTests.cs Fixed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

MCP confirmation rendering, CSV cancellation, and persistent MCP history have unresolved correctness gaps.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Persist MCP command history and enforce its size limit

CosmosDBShell/​Azure.Data.Cosmos.Shell.Core/​ShellInterpreter.cs:1827

This records MCP commands only in memory. SaveHistory() is called solely from the interactive input path, so an MCP invocation is absent from cmd_history unless a later interactive command happens to trigger a save; an MCP-only session also never applies MAXHISTORYITEMS. Persist MCP entries through a synchronized history update so the documented history remains available after restart and stays bounded.

Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs
Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Commands/ImportCommand.cs
…cked CSV reads

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 24, 2026 11:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Sparse MCP positionals produce non-equivalent replay commands, and test history isolation can still target a developer’s real configuration.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
Resolved since last review (2)

Comment thread CosmosDBShell.Tests/TestEnvironmentInitializer.cs Outdated
Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Mcp/ToolOperations.cs Outdated
Copilot AI review requested due to automatic review settings September 24, 2026 12:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Sensitive MCP history files are created with potentially permissive default Unix permissions.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread CosmosDBShell/Azure.Data.Cosmos.Shell.Core/ShellInterpreter.cs
Copilot AI review requested due to automatic review settings September 24, 2026 13:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The broad concurrency, destructive-operation, and filesystem changes warrant final human validation despite comprehensive regression coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@mkrueger
Mike Krüger (mkrueger) merged commit c614f52 into main Sep 25, 2026
12 checks passed
@mkrueger
Mike Krüger (mkrueger) deleted the dev/mkrueger/mcp-export-hardening branch September 25, 2026 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants