sbroenne/project-conventions

Context

ExcelMcp is a Windows-only toolset for programmatic Excel automation via COM interop, with TWO equal entry points: MCP Server (for AI assistants) and CLI (for scripting/agents). .NET/C# codebase.

Patterns

Exception Propagation (CRITICAL)

Core commands NEVER wrap batch.Execute() in try-catch that returns error results. Let exceptions propagate naturally — batch.Execute() handles them via TaskCompletionSource.

// ✅ CORRECT
return await batch.Execute((ctx, ct) => {
    // operation
    return ValueTask.FromResult(new OperationResult { Success = true });
});

// ❌ WRONG — double-wraps, loses stack context
try { return await batch.Execute(...); }
catch (Exception ex) { return new OperationResult { Success = false, ErrorMessage = ex.Message }; }

COM Object Cleanup

ALL dynamic COM objects must be released in finally blocks using ComUtilities.Release(ref obj!). NEVER use catch blocks to swallow exceptions.

Success Flag Invariant

Success == true ⟹ ErrorMessage == null || ErrorMessage == "". Set Success in try block, always false in catch.

Error Handling

• Core Commands: Let exceptions propagate through batch.Execute()
• MCP Server: Return JSON with isError: true for business errors; throw McpException for validation
• CLI: Wrap Core calls in try-catch, display with AnsiConsole.MarkupLine
• When evolving failure envelopes, preserve the legacy error field for compatibility and add richer diagnostics additively (errorMessage, isError, exceptionType, hresult, innerError) instead of renaming contracts in place.
• If CLI/MCP parity needs richer diagnostics, add them to the shared ServiceResponse transport first so both entry points receive the same failure detail.

MCP Schema Discoverability

• For hand-written MCP tools, add [Description("...")] attributes to each exposed parameter that needs to appear in the published schema.
• XML /// <param> comments are not enough by themselves for MCP client discoverability; verify the live schema with an integration test that calls ListToolsAsync().
• When fixing LLM discoverability bugs, preserve runtime aliases if needed, but make the canonical parameter pair explicit in both the parameter descriptions and the shared skill doc.

Testing

• Framework: xUnit with integration tests ONLY (no unit tests)
• Test location: tests/ExcelMcp.Core.Tests/, tests/ExcelMcp.McpServer.Tests/, tests/ExcelMcp.CLI.Tests/
• Run command: dotnet test tests/ExcelMcp.Core.Tests --filter "Feature=<name>&RunType!=OnDemand"
• TDD: Write test FIRST → RED → implement → GREEN
• NEVER share test files between tests — each test creates unique files
• ALWAYS verify actual Excel state, not just success flags (round-trip validation)
• For range write bugs, test payload shape explicitly: rectangular wide writes, jagged rows, and create-sheet-then-write-non-A1 flows. Don’t infer COM limits from jagged input failures.
• When hardening diagnostics regressions, centralize failure-envelope assertions in the shared test harness so every regression checks the same contract: success=false, isError=true, error == errorMessage, expected exceptionType, and scenario-specific presence/absence of errorCategory, hresult, and innerError.
• For CLI parity tests, assert exit code 1 on business-error paths (for example, missing sheet or invalid input). If the setup command itself returns non-JSON stdout, treat that as startup/harness noise and surface raw stdout/stderr in the helper exception instead of misclassifying it as a contract failure.

Bug Report Triage For Tests

• Check the live tool surface before treating a report as a missing-feature bug; verify ServiceAction coverage and current MCP tool docs first.
• Classify each report item before writing tests: regression in promised behavior, discoverability/documentation gap, or new feature request.
• Regressions get exact failing workflow tests first at Core and MCP layers.
• Existing capabilities with weak discoverability get positive end-to-end coverage before any API expansion.
• New features get acceptance tests only after the public API shape is agreed.

Code Style

• Analyzer: TreatWarningsAsErrors=true with .NET analyzers
• Naming: PascalCase for public, camelCase for params → auto-converts to snake_case in MCP
• One public class per file, file name = class name
• Partial classes for 15+ methods (split by feature domain)
• No emojis in LLM-consumed content (XML docs, skill .md files)

File Structure

src/ExcelMcp.ComInterop/  — COM patterns, STA threading, sessions
src/ExcelMcp.Core/         — Excel business logic, commands
src/ExcelMcp.Service/      — Session management, command routing
src/ExcelMcp.McpServer/    — MCP protocol tools
src/ExcelMcp.CLI/          — Command-line interface
src/ExcelMcp.Generators*/  — Source generators
tests/                     — Integration tests (no unit tests)
skills/shared/             — Single source of truth for docs/prompts

Formatting Surface Split

Before classifying a formatting bug as missing functionality, check both range and range_format.

• range owns value/formula-adjacent display formats such as set-number-format
• range_format owns visual styling and layout actions such as format-range, auto-fit-columns, and auto-fit-rows

If the capability already exists under one of those tools, treat it as a discoverability/API-shape issue first, not a backend feature gap.

For real batching work, prefer existing list-of-objects patterns over ad hoc JSON blobs. IRangeEditCommands.Sort(... List<SortColumn> ...) is a precedent that the shared surface can carry structured collections cleanly.

Anti-Patterns

• Unit tests — NEVER write unit tests. Integration tests only for COM interop.
• RefreshAll() — NEVER use. Use individual queryTable.Refresh(false) (synchronous).
• Catch-and-swallow — NEVER catch exceptions in Core commands to return error results.
• Dual test fixtures — NEVER use both IClassFixture<T> AND [Collection("...")].
• Manual ScreenUpdating — ExcelWriteGuard handles this automatically.
• Suppressing EnableEvents — Data Model operations depend on them.
• Assuming Excel has a hidden range-width cap — if set-values fails with ArgumentOutOfRangeException, inspect for jagged List<List<object?>> input before blaming COM.

Triage Pattern

Before assigning a bug to Core, check three things in order:

1. Existing integration coverage for the exact shape or a close analogue.
2. Whether the capability already exists under a different tool or action name.
3. Whether the failure is more likely in MCP/service argument binding, docs/skills discoverability, or true COM/Core behavior.

Use this especially for reports that claim a hard product limit or a missing feature. Wide-range failures and formatting gaps are often mis-triaged when tests or tool surfaces already cover the scenario elsewhere.