From 2e6bf6a2247c811ad97f36ad5ca6806efa8a6a02 Mon Sep 17 00:00:00 2001 From: Arthur van de Vondervoort Date: Sun, 4 Oct 2026 10:31:49 +0200 Subject: [PATCH 1/4] feat!: one error envelope and fixed error vocabulary for all native tools All five native tools return CallToolResult with IsError = true on failure and a single ToolError envelope { error, message, reason?, candidates?, filePath?, diagnosticId?, detail? } with the fixed codes Invalid, NotFound, Ambiguous, Stale, Unavailable and Faulted. get_fixes returns an object with the fixes array; its no-match cases become NotFound with a reason (incl. ruleset vs pragma suppression). Ambiguous carries candidates with key, title and provider; equivalence keys round trip (null keys were advertised as "" but never matched). list_rules catches exceptions and validates projectPath like analyze (closes #33). The empty project list message is fixed. BREAKING CHANGE: error codes renamed, get_fixes success shape changed, isError set. Closes #41 Closes #33 Co-Authored-By: Claude Fable 5.1 --- AGENTS.md | 15 +- README.md | 36 ++- src/ALCops.Mcp/Models/CodeFixInfo.cs | 9 +- src/ALCops.Mcp/Models/FileWriteConflict.cs | 4 +- src/ALCops.Mcp/Models/FixAllResult.cs | 16 +- src/ALCops.Mcp/Models/FixLookupResult.cs | 49 ++++ src/ALCops.Mcp/Models/GetFixesResult.cs | 9 + src/ALCops.Mcp/Models/ToolError.cs | 47 ++++ src/ALCops.Mcp/Services/CodeFixRunner.cs | 253 +++++++++++------- src/ALCops.Mcp/Services/ProjectScope.cs | 77 ++++++ src/ALCops.Mcp/Services/ToolErrors.cs | 100 +++++++ src/ALCops.Mcp/Services/ToolResults.cs | 19 ++ src/ALCops.Mcp/Tools/AnalyzeTool.cs | 80 +++--- src/ALCops.Mcp/Tools/ApplyFixAllTool.cs | 65 +++-- src/ALCops.Mcp/Tools/ApplyFixTool.cs | 64 +++-- src/ALCops.Mcp/Tools/GetFixesTool.cs | 30 ++- src/ALCops.Mcp/Tools/ListRulesTool.cs | 89 +++--- tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs | 164 +++++++++--- .../ALCops.Mcp.Tests/ApplyFixAllToolTests.cs | 155 +++++++---- .../ApplyFixThenCompileTests.cs | 8 +- tests/ALCops.Mcp.Tests/ApplyFixToolTests.cs | 190 +++++++++++-- tests/ALCops.Mcp.Tests/FixRoundTripTests.cs | 94 +++++++ .../Fixtures/PragmaProject/PageWithPragma.al | 20 ++ .../PragmaProject/PageWithoutPragma.al | 18 ++ tests/ALCops.Mcp.Tests/GetFixesToolTests.cs | 221 +++++++++++++++ tests/ALCops.Mcp.Tests/ListRulesToolTests.cs | 111 ++++++++ tests/ALCops.Mcp.Tests/ToolErrorsTests.cs | 78 ++++++ tests/ALCops.Mcp.Tests/ToolResultAssert.cs | 51 ++++ 28 files changed, 1683 insertions(+), 389 deletions(-) create mode 100644 src/ALCops.Mcp/Models/FixLookupResult.cs create mode 100644 src/ALCops.Mcp/Models/GetFixesResult.cs create mode 100644 src/ALCops.Mcp/Models/ToolError.cs create mode 100644 src/ALCops.Mcp/Services/ProjectScope.cs create mode 100644 src/ALCops.Mcp/Services/ToolErrors.cs create mode 100644 src/ALCops.Mcp/Services/ToolResults.cs create mode 100644 tests/ALCops.Mcp.Tests/FixRoundTripTests.cs create mode 100644 tests/ALCops.Mcp.Tests/Fixtures/PragmaProject/PageWithPragma.al create mode 100644 tests/ALCops.Mcp.Tests/Fixtures/PragmaProject/PageWithoutPragma.al create mode 100644 tests/ALCops.Mcp.Tests/GetFixesToolTests.cs create mode 100644 tests/ALCops.Mcp.Tests/ListRulesToolTests.cs create mode 100644 tests/ALCops.Mcp.Tests/ToolErrorsTests.cs create mode 100644 tests/ALCops.Mcp.Tests/ToolResultAssert.cs diff --git a/AGENTS.md b/AGENTS.md index e34547d..a21bced 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -43,7 +43,7 @@ It is deliberately **thin**: Microsoft's `almcp` already compiles, runs diagnost ### Key layers - **Tools/** — MCP tool endpoints, annotated `[McpServerToolType]` / `[McpServerTool]`, auto-discovered via `WithToolsFromAssembly()`. Five native tools: `list_rules`, `get_fixes`, `apply_fix`, `apply_fix_all`, `analyze`. The proxied `al_*` tools are served by the dynamic list/call handlers in `McpHost`, not by classes here. -- **Services/**, all singletons registered in `McpHost`: +- **Services/**, singletons registered in `McpHost` unless noted: - `BcToolsLocator` — the single runtime lookup. Finds the one directory holding both `Microsoft.Dynamics.Nav.*.dll` and `almcp` (they ship side by side). Probe order: `--devtools-path` → `BCDEVELOPMENTTOOLSPATH` → dotnet tool store → hard error naming every probe and the install command. The AL VS Code extension is no longer probed; the dotnet tool store is the primary channel on every OS. No auto-install by design. `AlMcpLaunch` describes how to start the child: native `almcp.exe` on Windows, `dotnet almcp.dll` on Linux/macOS (the nupkg ships no extension-less launcher). - `WorkspaceStartupResolver` — discovers AL projects (mirrors `almcp`'s own `DiscoverProjectPaths`: downward scan for `app.json`, depth 4, standard exclusions) and composes the child `almcp`'s `--projects` / `--codeanalyzers` / `--rulesetpath` / `--packagecachepath` args. `almcp` in MCP mode never reads `.vscode/settings.json` and has no per-call analyzer, ruleset or package-cache parameter, so this bridge at launch is the only thing keeping `al_compile` and our fix tools in agreement (`ProjectLoader` reads the same `al.packageCachePath` for the in-process compilation). - `AlMcpProxy` — child process lifecycle plus generic tool forwarding over a single long-lived MCP client that reconnects on session expiry. `ForwardAsync` is a passthrough with **no per-tool argument rewriting**; configuration is conveyed at launch instead. @@ -51,6 +51,8 @@ It is deliberately **thin**: Microsoft's `almcp` already compiles, runs diagnost - `AlcopsAnalyzerProvisioner` — cache-first: when a valid cached version exists, `Task Ready` completes immediately with it and a background task checks NuGet for a newer version (for the next start). `internal Task BackgroundRefresh` is that background task, drained by `ProvisionAsync` and cancelled by `AlcopsAnalyzerProvisionerStartup.StopAsync`. On a cold cache or with a pinned version, the provisioner fetches from NuGet before completing. Internal HTTP timeouts surface as `TimeoutException` and fall back to cache; only the caller's cancellation propagates. Version ordering is SemVer 2 via `SemanticVersion`. The `.in-use` lock (`FileShare.None`) is a cross-process lock via `flock` on Unix, reliable on local file systems and advisory on network mounts such as NFS home directories. Configured via `--alcops-analyzers` / `ALCOPS_ANALYZERS` / `ALCOPS_ANALYZERS_CACHE`. - `ExternalAnalyzerLoader` — loads analyzer DLLs through `AnalyzerAssemblyLoadContext`, which resolves shared types by simple name from the default context. That type sharing is what makes `typeof(DiagnosticAnalyzer).IsAssignableFrom` work, and therefore what makes in-process code fixes possible at all. - `ProjectSessionManager` / `ProjectLoader` — caches AL project workspaces keyed by path; `GetOrLoadProjectAsync` is the entry point tools use. On a cache hit it calls `ProjectSession.RefreshFromDiskAsync`: length+mtime gate, then ordinal content compare; changed files get `OnDocumentTextChanged`, new/deleted files are added/removed; untouched documents keep `DocumentId` and compilation state. Mutations go through `Workspace.OnDocument*` (like almcp's `ProjectWatcher`), never `TryApplyChanges` on a forked solution. + - `ToolResults` / `ToolErrors` — static helpers, not registered. `ToolResults.Ok` serializes a success payload; `ToolErrors` is the only place a tool error is built (see Tool patterns). + - `ProjectScope` — static helper, not registered. `Resolve` validates `projectPath` against the startup projects for `analyze` and `list_rules`; `RequireProjectFolder` checks that the fix tools got a folder that exists and holds `app.json`. A failure is an `Invalid` error. - `GuardedFileWriter` — atomic, encoding-preserving `.al` writes; the only code that writes `.al` files. Used by `apply_fix` (`WriteIfUnchangedAsync`) and `apply_fix_all` (`WriteAllIfUnchangedAsync`). Its internal `Action` constructor is a test seam for injecting move failures. - **Models/** — record types for tool return values, serialized with `JsonDefaults.Options` (camelCase, not indented). @@ -64,13 +66,14 @@ When passing analyzers to the child `almcp`, their sibling dependencies must tra ### Tool patterns -- All tool methods are `static async Task`, receiving DI services as parameters. -- Tools return JSON-serialized results. Errors are caught and returned as `{ error, message }` JSON, not thrown. -- `list_rules` is read-only. `get_fixes`, `apply_fix` and `apply_fix_all` re-read changed `.al` files from disk before computing a fix (via `RefreshFromDiskAsync`); the write only proceeds if the file still equals the text the fix was computed from. `apply_fix` returns `{ error: "StaleFile" }` on a conflict; `apply_fix_all` writes the non-conflicting files and lists the rest in `conflicts`. There is no explicit reload after writing — the next `GetOrLoadProjectAsync` call picks the written file up via the same refresh. -- Writes go through `GuardedFileWriter`: read the file as bytes → decode with BOM detection → ordinal compare with the expected text → write preamble + new text to a sibling `..alcops.tmp` → `File.Move(temp, path, overwrite: true)`; the temp is deleted in `finally`. The encoding is detected at write time from that read, not stored on `TrackedDocument`. The BOM is sniffed explicitly (UTF-32 LE before UTF-16 LE) and every branch, BOM-less UTF-8 included, uses a strict (throwing) decoder: bytes invalid in the detected encoding (e.g. a Windows-1252 file, a stray Windows-1252 byte behind a UTF-8 BOM, a lone surrogate in UTF-16; the loader reads all of these with U+FFFD replacement, so the stale check alone would pass) are refused as `UnsupportedEncoding` instead of being re-encoded. Never decode through `StreamReader` with BOM detection: it swaps in its own lossy encoding when a BOM is present. `File.Move` replaces a symlink at the target with a regular file and drops Unix mode bits; both accepted, out of scope. **Never `File.Replace` and never a backup file**: almcp's `ProjectWatcher` (`*.al` filter, Renamed handler checks `EndsWith(".al")`) treats a move-over as an in-place update that keeps the `DocumentId`, while replace-with-backup makes it remove and re-add the document. Every `FileWriteConflict` has a `Kind` (serialized as `kind`): `StaleFile`, `UnsupportedEncoding`, `ReadFailed`, `WriteFailed`, `RolledBack`, `NotWritten`; `apply_fix` returns it as `error`. `apply_fix_all` is two-phase: stale/deleted/unreadable/invalidly-encoded files, and files whose fixed text the strict encoder cannot encode (also `UnsupportedEncoding`), drop out as conflicts first, then the rest are committed one by one; a commit failure (any exception; a cancellation is rethrown after the rollback) restores every file already committed in that call from the original bytes held in memory (same temp + move path) and reports the whole staged set in `conflicts` with `applied: false`. A rollback that itself fails, or a file modified after this call wrote it (rollback compares current bytes with what was written and skips the restore), is named in `message` and that file stays in `filesChanged`. `LoadProjectAsync` and `RefreshFromDiskAsync` sweep stray `*.alcops.tmp` files outside `.alpackages` (`ProjectLoader.SweepTempFiles`); the refresh only removes files older than 30 s because tool calls run concurrently and a write may be between temp-create and move. +- All tool methods are `static async Task`, receiving DI services as parameters. Success goes through `ToolResults.Ok(payload)` (`IsError` left unset); errors only through `ToolErrors`. No tool calls `JsonSerializer.Serialize` itself or builds an anonymous `error` object. +- Every error sets `IsError = true` and carries one `ToolError` envelope `{ error, message, reason?, candidates?, filePath?, diagnosticId?, detail? }` (optional fields omitted when null via per-property `JsonIgnore`; `JsonDefaults.Options` is not changed). `error` is one of `Invalid`, `NotFound`, `Ambiguous`, `Stale`, `Unavailable`, `Faulted`; `NotFound` reasons are the `FixNotFoundReason` names, `Unavailable` reasons the `UnavailableReason` constants. The README `## Errors` section is the public table. Every tool body ends in `catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } catch (Exception ex) { return ToolErrors.Faulted(ex); }`: a cancellation propagates, anything else becomes `Faulted` with `detail` = the exception's full type name. +- `get_fixes` returns `{ diagnosticId, filePath, line, column, fixes }` with a non-empty `fixes`; every no-match case is `NotFound` with a reason (`CodeFixRunner` returns `FixLookupResult` / `FixApplyResult` / `FixAllResult` with a `FixNotFoundReason`). Ruleset suppression is checked before analysis; pragma suppression is told apart from no diagnostic because `GetEffectiveDiagnostics` keeps pragma-suppressed diagnostics with `IsSuppressed = true`. Equivalence keys are compared through `KeyOf(action) => action.EquivalenceKey ?? ""` on both the advertising and the matching side, so every advertised key round-trips. `candidates` are de-duplicated by key, first occurrence wins. +- `list_rules` is read-only. `get_fixes`, `apply_fix` and `apply_fix_all` re-read changed `.al` files from disk before computing a fix (via `RefreshFromDiskAsync`); the write only proceeds if the file still equals the text the fix was computed from. `apply_fix` returns the error `Stale` on a conflict; `apply_fix_all` writes the non-conflicting files and lists the rest in `conflicts`. There is no explicit reload after writing — the next `GetOrLoadProjectAsync` call picks the written file up via the same refresh. +- Writes go through `GuardedFileWriter`: read the file as bytes → decode with BOM detection → ordinal compare with the expected text → write preamble + new text to a sibling `..alcops.tmp` → `File.Move(temp, path, overwrite: true)`; the temp is deleted in `finally`. The encoding is detected at write time from that read, not stored on `TrackedDocument`. The BOM is sniffed explicitly (UTF-32 LE before UTF-16 LE) and every branch, BOM-less UTF-8 included, uses a strict (throwing) decoder: bytes invalid in the detected encoding (e.g. a Windows-1252 file, a stray Windows-1252 byte behind a UTF-8 BOM, a lone surrogate in UTF-16; the loader reads all of these with U+FFFD replacement, so the stale check alone would pass) are refused as `UnsupportedEncoding` instead of being re-encoded. Never decode through `StreamReader` with BOM detection: it swaps in its own lossy encoding when a BOM is present. `File.Move` replaces a symlink at the target with a regular file and drops Unix mode bits; both accepted, out of scope. **Never `File.Replace` and never a backup file**: almcp's `ProjectWatcher` (`*.al` filter, Renamed handler checks `EndsWith(".al")`) treats a move-over as an in-place update that keeps the `DocumentId`, while replace-with-backup makes it remove and re-add the document. Every `FileWriteConflict` has a `Kind` (serialized as `kind`): `StaleFile`, `UnsupportedEncoding`, `ReadFailed`, `WriteFailed`, `RolledBack`, `NotWritten`; `apply_fix_all` reports it verbatim in `conflicts[].kind`, `apply_fix` maps `StaleFile` to the error `Stale` and every other kind to `Faulted` with `reason` = the kind (an exception thrown by the write itself is `Faulted` / `WriteFailed`). `apply_fix_all` is two-phase: stale/deleted/unreadable/invalidly-encoded files, and files whose fixed text the strict encoder cannot encode (also `UnsupportedEncoding`), drop out as conflicts first, then the rest are committed one by one; a commit failure (any exception; a cancellation is rethrown after the rollback) restores every file already committed in that call from the original bytes held in memory (same temp + move path) and reports the whole staged set in `conflicts` with `applied: false`. A rollback that itself fails, or a file modified after this call wrote it (rollback compares current bytes with what was written and skips the restore), is named in `message` and that file stays in `filesChanged`. `LoadProjectAsync` and `RefreshFromDiskAsync` sweep stray `*.alcops.tmp` files outside `.alpackages` (`ProjectLoader.SweepTempFiles`); the refresh only removes files older than 30 s because tool calls run concurrently and a write may be between temp-create and move. - `al_compile` defaults to `onlyErrors: true` while nearly every ALCops rule is a warning — callers must pass `options.onlyErrors: false` (the flag lives inside almcp's `options` object; a top-level `onlyErrors` is ignored, and omitting `options` entirely also yields `onlyErrors: false` with no diagnostics cap). This is documented rather than patched, because `ForwardAsync` stays a generic passthrough. - After `apply_fix` / `apply_fix_all`, verify with `analyze` (preferred) or `al_compile` (`options.onlyErrors: false`), not `al_getdiagnostics`. almcp's `ProjectWatcher` (`FileSystemWatcher`) re-reads changed `.al` files, and `al_compile` awaits `WaitForProcessingAsync` before compiling, so it normally picks up on-disk changes before the compile starts. The gate starts signalled and has no debounce, so on slow file systems or right after a large `apply_fix_all` a second `al_compile` may be needed if the watcher has not yet delivered the change notification. `al_getdiagnostics` returns cached compilation results without re-analyzing and will report stale diagnostics. `al_build` does not await the watcher at all. -- `analyze` is the one native tool that goes through `AlMcpProxy.ForwardAsync` (`al_compile` with nested `options.onlyErrors=false`, `enableCodeAnalysis=true`, `maxDiagnosticsPerCompilation=int.MaxValue`, never `codeAnalyzers`); it resolves the proxy via `IServiceProvider` because under `--no-proxy` the type is unregistered and the SDK would otherwise expose it as a tool argument; it returns `{error:"ProxyUnavailable"}` in that case. +- `analyze` is the one native tool that goes through `AlMcpProxy.ForwardAsync` (`al_compile` with nested `options.onlyErrors=false`, `enableCodeAnalysis=true`, `maxDiagnosticsPerCompilation=int.MaxValue`, never `codeAnalyzers`); it resolves the proxy via `IServiceProvider` because under `--no-proxy` the type is unregistered and the SDK would otherwise expose it as a tool argument; it returns `Unavailable` / `NoProxy` in that case, `AlmcpNotFound` when almcp is not installed, `AlmcpNotReady` when it failed to start, and `AlmcpCallFailed` (almcp text in `detail`) when the proxied call itself reports an error. - `analyze` inherits the FileSystemWatcher staleness caveat after `apply_fix` / `apply_fix_all`. ## Conventions diff --git a/README.md b/README.md index eba7e4a..5760149 100644 --- a/README.md +++ b/README.md @@ -52,9 +52,9 @@ The native tools (`list_rules`, `get_fixes`, `apply_fix`, `apply_fix_all`) work | Tool | Description | |------|-------------| | `list_rules` | List analyzer rules with metadata (ID, title, severity, category, cop). | -| `get_fixes` | Get available code fixes for a specific diagnostic at a location. | -| `apply_fix` | Apply a code fix to resolve a diagnostic. Writes the fixed content to disk unless the file changed after the fix was computed (`StaleFile`). The write is atomic (sibling temp file renamed over the target) and keeps the file's encoding, BOM and line endings. | -| `apply_fix_all` | Apply a code fix to every occurrence of a diagnostic rule across a project or a single file (like VS Code's "Fix all in workspace"). Writes to disk unless `dryRun` is set. Files that changed on disk mid-operation are skipped and listed in `conflicts`; the rest are written as one batch — if any write fails, every file already written in that call is restored and nothing from the batch is kept. | +| `get_fixes` | Get available code fixes for a specific diagnostic at a location. Returns `{ diagnosticId, filePath, line, column, fixes: [{ equivalenceKey, title, providerName }] }`; when nothing matches, the error `NotFound` with a `reason` (see [Errors](#errors)). | +| `apply_fix` | Apply a code fix to resolve a diagnostic. Writes the fixed content to disk unless the file changed after the fix was computed (error `Stale`); a file that cannot be read, decoded or written is `Faulted` with `reason` `ReadFailed`, `UnsupportedEncoding` or `WriteFailed`. The write is atomic (sibling temp file renamed over the target) and keeps the file's encoding, BOM and line endings. | +| `apply_fix_all` | Apply a code fix to every occurrence of a diagnostic rule across a project or a single file (like VS Code's "Fix all in workspace"). Writes to disk unless `dryRun` is set. A rule with several distinct fixes and no `equivalenceKey` is the error `Ambiguous` with `candidates`. Zero occurrences is a success (`applied: false`, `diagnosticsFound: 0`), except a rule the project ruleset suppresses, which is `NotFound` with `reason: SuppressedByRuleset`. Files that changed on disk mid-operation are skipped and listed in `conflicts`; the rest are written as one batch — if any write fails, every file already written in that call is restored and nothing from the batch is kept. | | `analyze` | Compile with all configured analyzers and return structured cop + compiler diagnostics (analyzer, hasFix, filters, summary). Wraps `al_compile` with `onlyErrors: false`; needs `almcp`. | ### Proxied from Microsoft's `almcp` @@ -73,7 +73,35 @@ After `apply_fix` or `apply_fix_all`, call `analyze` (preferred) or `al_compile` **Editing between calls:** `get_fixes`, `apply_fix` and `apply_fix_all` re-read `.al` files that changed on disk before every call (only changed files are re-parsed), so you can edit files between `get_fixes` and `apply_fix`, and neither tool will overwrite a file that no longer matches the text its fix was computed from. -**How files are written:** each fixed file is written to a sibling `.al..alcops.tmp` and then renamed over the original, so a crash or a full disk never leaves a truncated `.al` file. The file's encoding is detected from its byte-order mark and kept: UTF-8 with or without BOM, UTF-16 and UTF-32 come back exactly as they were, and line endings are untouched. Each file is decoded strictly in its detected encoding (UTF-8 when there is no BOM): a file holding bytes that are invalid in that encoding, such as a legacy Windows-1252 file or a stray Windows-1252 byte in a UTF-8 file with BOM, is never re-encoded; the fix is refused with `UnsupportedEncoding` instead. Every entry in `apply_fix_all`'s `conflicts` carries a `kind` (`StaleFile`, `UnsupportedEncoding`, `ReadFailed`, `WriteFailed`, `RolledBack`, `NotWritten`); `apply_fix` returns the same value as its `error`. No backup files are created. A stray `*.alcops.tmp` left by an interrupted write is removed the next time the project is loaded or refreshed. +**How files are written:** each fixed file is written to a sibling `.al..alcops.tmp` and then renamed over the original, so a crash or a full disk never leaves a truncated `.al` file. The file's encoding is detected from its byte-order mark and kept: UTF-8 with or without BOM, UTF-16 and UTF-32 come back exactly as they were, and line endings are untouched. Each file is decoded strictly in its detected encoding (UTF-8 when there is no BOM): a file holding bytes that are invalid in that encoding, such as a legacy Windows-1252 file or a stray Windows-1252 byte in a UTF-8 file with BOM, is never re-encoded; the fix is refused with `UnsupportedEncoding` instead. Every entry in `apply_fix_all`'s `conflicts` carries a `kind` (`StaleFile`, `UnsupportedEncoding`, `ReadFailed`, `WriteFailed`, `RolledBack`, `NotWritten`); `apply_fix` reports `StaleFile` as the error `Stale` and the other kinds as `Faulted` with that kind as `reason`. No backup files are created. A stray `*.alcops.tmp` left by an interrupted write is removed the next time the project is loaded or refreshed. + +## Errors + +Every native tool reports a failure the same way: the MCP result has `isError: true`, and its single text block holds one JSON envelope. A successful result never sets `isError`. The full JSON stays in the text block, so a client that hides error results still has the message. + +```json +{ "error": "NotFound", "message": "No LC0020 at C:\\src\\MyPage.al:12:17; re-run analyze and use its line/column.", "reason": "NoDiagnosticAtPosition", "filePath": "C:\\src\\MyPage.al", "diagnosticId": "LC0020" } +``` + +| Field | Present | +|-------|---------| +| `error` | Always; one of the codes below. | +| `message` | Always; human-readable. | +| `reason` | For `NotFound`, `Unavailable`, and a `Faulted` write. | +| `candidates` | For `Ambiguous` and `NotFound` / `NoFixForEquivalenceKey`: `[{ equivalenceKey, title, providerName }]`. | +| `filePath`, `diagnosticId` | When the error concerns one file or rule. | +| `detail` | For `Faulted` from an exception (its full type name) and `Unavailable` / `AlmcpCallFailed` (the text almcp returned). | + +| `error` | Meaning | What to do | `reason` | +|---------|---------|------------|----------| +| `Invalid` | The arguments are unusable as given: unknown `scope`, missing `filePath`, `limit` not positive, a `projectPath` that is not an AL project folder, or (for `analyze` and `list_rules`) not one of the projects the server was started with. | Fix the call. | – | +| `NotFound` | Nothing matched. | `NoDiagnosticAtPosition`: re-run `analyze` and use its line/column. `NoFixForEquivalenceKey`: pick a key from `candidates`. Otherwise stop; there is nothing to fix. | `NoFixProvider`, `FileNotInProject`, `NoAnalyzerForRule`, `SuppressedByRuleset`, `SuppressedByPragma`, `NoDiagnosticAtPosition`, `NoFixForDiagnostic`, `NoFixForEquivalenceKey` | +| `Ambiguous` | The rule offers several distinct fixes; `candidates` lists them. | Pass one `equivalenceKey` verbatim, or ask the user. | – | +| `Stale` | The file changed or was deleted after the fix was computed; nothing was written. | Re-run the call. | – | +| `Unavailable` | A dependency is down. | Report it; do not retry in a loop. | `NoProxy` (`--no-proxy`), `AlmcpNotFound`, `AlmcpNotReady`, `AlmcpCallFailed` (also covers errors `al_compile` itself reports) | +| `Faulted` | An unexpected exception, or a write that was refused or failed. | Report it, including `message`. | `UnsupportedEncoding`, `ReadFailed`, `WriteFailed` for writes; none for exceptions | + +`candidates` holds one entry per distinct `equivalenceKey`: when two providers offer the same key, the first one wins, and that is also the one a key match applies. A key is `""` when the provider sets none; pass it back as is. The proxied `al_*` tools are not covered by this envelope: their errors pass through from `almcp` unchanged. ## Analyzers diff --git a/src/ALCops.Mcp/Models/CodeFixInfo.cs b/src/ALCops.Mcp/Models/CodeFixInfo.cs index 7541c56..99189be 100644 --- a/src/ALCops.Mcp/Models/CodeFixInfo.cs +++ b/src/ALCops.Mcp/Models/CodeFixInfo.cs @@ -1,7 +1,12 @@ namespace ALCops.Mcp.Models; +/// +/// One code fix offered for a diagnostic: the shape of both the fixes of get_fixes and +/// the candidates of an error, so the two are identical by construction. +/// is the action's key, or "" when the provider set none; pass it +/// back verbatim. +/// public record CodeFixInfo( - string Title, string EquivalenceKey, - string DiagnosticId, + string Title, string ProviderName); diff --git a/src/ALCops.Mcp/Models/FileWriteConflict.cs b/src/ALCops.Mcp/Models/FileWriteConflict.cs index af39a9a..ca6d9b0 100644 --- a/src/ALCops.Mcp/Models/FileWriteConflict.cs +++ b/src/ALCops.Mcp/Models/FileWriteConflict.cs @@ -5,7 +5,9 @@ namespace ALCops.Mcp.Models; /// StaleFile (changed on disk or deleted), UnsupportedEncoding (the file is not valid in its detected encoding, or the fixed text cannot be encoded in it), /// ReadFailed (could not be read for the stale check), WriteFailed (the file whose commit threw), /// RolledBack (committed, then restored) or NotWritten (staged but never attempted). -/// Serialized as kind; this vocabulary is additive and may be renamed by issue #41. +/// Serialized as kind. apply_fix_all reports these kinds verbatim in conflicts[].kind; +/// apply_fix maps StaleFile to the Stale error and every other kind to Faulted +/// with reason = the kind. /// public record FileWriteConflict(string FilePath, string Message, string Kind = FileWriteConflictKind.StaleFile); diff --git a/src/ALCops.Mcp/Models/FixAllResult.cs b/src/ALCops.Mcp/Models/FixAllResult.cs index fb692ae..01d5cce 100644 --- a/src/ALCops.Mcp/Models/FixAllResult.cs +++ b/src/ALCops.Mcp/Models/FixAllResult.cs @@ -9,8 +9,10 @@ public enum FixAllStatus /// The fix-all pass ran; see Changes/Unfixed for what happened (possibly zero changes). Completed, NoDiagnosticsFound, - NoFixAvailable, - AmbiguousFix + /// No fix could be chosen; says why. + NotFound, + /// Several distinct fixes apply and no equivalence key was given; see . + Ambiguous } /// One file whose content changed (or would change, if dryRun) as a result of the fix-all pass. @@ -19,6 +21,11 @@ public record FixAllFileChange(string FilePath, string OriginalContent, string M /// A diagnostic matching the requested rule that no available fix could resolve. public record FixAllUnfixedDiagnostic(string FilePath, int Line, int Column); +/// +/// The distinct fixes offered for the probe diagnostic, filled for +/// and for ; empty otherwise. +/// +/// Set when is . public record FixAllResult( FixAllStatus Status, string DiagnosticId, @@ -26,5 +33,6 @@ public record FixAllResult( string? FixTitle, string? EquivalenceKey, IReadOnlyList Changes, - IReadOnlyList AvailableEquivalenceKeys, - IReadOnlyList Unfixed); + IReadOnlyList Candidates, + IReadOnlyList Unfixed, + FixNotFoundReason? NotFoundReason = null); diff --git a/src/ALCops.Mcp/Models/FixLookupResult.cs b/src/ALCops.Mcp/Models/FixLookupResult.cs new file mode 100644 index 0000000..b58a84e --- /dev/null +++ b/src/ALCops.Mcp/Models/FixLookupResult.cs @@ -0,0 +1,49 @@ +using Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics; + +namespace ALCops.Mcp.Models; + +/// +/// Why a fix lookup found nothing. Serialized by name as the reason of a NotFound error, +/// so the member names are part of the tool contract. +/// +public enum FixNotFoundReason +{ + /// No loaded analyzer assembly registers a code fix provider for the rule. + NoFixProvider, + /// The file is not a document of the project. + FileNotInProject, + /// A fix provider exists, but no loaded analyzer reports the rule. + NoAnalyzerForRule, + /// The project's ruleset sets the rule to None. + SuppressedByRuleset, + /// The diagnostic exists at the position but a #pragma warning disable suppresses it. + SuppressedByPragma, + /// The rule is not reported at that line/column (nor anywhere on that line). + NoDiagnosticAtPosition, + /// The diagnostic was found, but no provider registered an applicable fix for it. + NoFixForDiagnostic, + /// Fixes exist, but none has the requested equivalence key. + NoFixForEquivalenceKey, +} + +/// Outcome of CodeFixRunner.GetFixesAsync: either fixes, or the reason there are none. +public record FixLookupResult(IReadOnlyList Fixes, FixNotFoundReason? NotFoundReason) +{ + public static FixLookupResult Found(IReadOnlyList fixes) => new(fixes, null); + public static FixLookupResult NotFound(FixNotFoundReason reason) => new([], reason); +} + +/// +/// Outcome of CodeFixRunner.ApplyFixAsync: the computed fix, or the reason there is none. +/// is filled for . +/// +public record FixApplyResult(CodeFixResult? Fix, FixNotFoundReason? NotFoundReason, IReadOnlyList Candidates) +{ + public static FixApplyResult Applied(CodeFixResult fix) => new(fix, null, []); + + public static FixApplyResult NotFound(FixNotFoundReason reason, IReadOnlyList? candidates = null) => + new(null, reason, candidates ?? []); +} + +/// A diagnostic found at a position, or the reason none was. +internal readonly record struct DiagnosticLookup(Diagnostic? Diagnostic, FixNotFoundReason? Reason); diff --git a/src/ALCops.Mcp/Models/GetFixesResult.cs b/src/ALCops.Mcp/Models/GetFixesResult.cs new file mode 100644 index 0000000..0d8c978 --- /dev/null +++ b/src/ALCops.Mcp/Models/GetFixesResult.cs @@ -0,0 +1,9 @@ +namespace ALCops.Mcp.Models; + +/// Success result of get_fixes. is never empty. +public record GetFixesResult( + string DiagnosticId, + string FilePath, + int Line, + int Column, + IReadOnlyList Fixes); diff --git a/src/ALCops.Mcp/Models/ToolError.cs b/src/ALCops.Mcp/Models/ToolError.cs new file mode 100644 index 0000000..bd2ce9c --- /dev/null +++ b/src/ALCops.Mcp/Models/ToolError.cs @@ -0,0 +1,47 @@ +using System.Text.Json.Serialization; + +namespace ALCops.Mcp.Models; + +/// +/// The single error envelope every native tool returns (with isError: true). Only +/// and are always present; the optional fields are omitted +/// when null. The parameter order is the JSON key order. +/// +public sealed record ToolError( + string Error, + string Message, + [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] string? Reason = null, + [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] IReadOnlyList? Candidates = null, + [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] string? FilePath = null, + [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] string? DiagnosticId = null, + [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] string? Detail = null); + +/// The fixed vocabulary of . +public static class ToolErrorCode +{ + /// The arguments are unusable as given; fix the call. + public const string Invalid = "Invalid"; + /// Nothing matched; reason is a name. + public const string NotFound = "NotFound"; + /// Several distinct fixes apply; candidates lists them. + public const string Ambiguous = "Ambiguous"; + /// The file changed or was deleted after the fix was computed; nothing was written. + public const string Stale = "Stale"; + /// A dependency is down; reason is an value. + public const string Unavailable = "Unavailable"; + /// An unexpected exception, or a refused or failed write (reason = the write kind). + public const string Faulted = "Faulted"; +} + +/// The reason values of an error. +public static class UnavailableReason +{ + /// The server runs with --no-proxy. + public const string NoProxy = "NoProxy"; + /// almcp is not in the DevTools directory. + public const string AlmcpNotFound = "AlmcpNotFound"; + /// almcp failed to start, or has been stopped. + public const string AlmcpNotReady = "AlmcpNotReady"; + /// The proxied call failed or almcp reported an error; detail carries the almcp text. + public const string AlmcpCallFailed = "AlmcpCallFailed"; +} diff --git a/src/ALCops.Mcp/Services/CodeFixRunner.cs b/src/ALCops.Mcp/Services/CodeFixRunner.cs index 20286cd..fd1554f 100644 --- a/src/ALCops.Mcp/Services/CodeFixRunner.cs +++ b/src/ALCops.Mcp/Services/CodeFixRunner.cs @@ -12,9 +12,9 @@ namespace ALCops.Mcp.Services; public sealed class CodeFixRunner { /// - /// Gets available code fixes for a specific diagnostic at a location. + /// Gets available code fixes for a specific diagnostic at a location, or the reason there are none. /// - public async Task> GetFixesAsync( + public async Task GetFixesAsync( ProjectSession session, string filePath, string diagnosticId, @@ -23,75 +23,139 @@ public async Task> GetFixesAsync( IAnalyzerProvider analyzerProvider, CancellationToken ct = default) { - var providers = analyzerProvider.GetCodeFixProvidersForDiagnostic(diagnosticId); - if (providers.IsEmpty) - return []; + var (located, notFound) = await LocateAsync(session, filePath, diagnosticId, line, column, analyzerProvider, ct); + if (notFound is { } reason) + return FixLookupResult.NotFound(reason); - var document = session.GetDocument(filePath); - if (document is null) - return []; + var (document, diagnostic, providers) = located!.Value; + var pairs = await CollectActionsAsync(providers, document, diagnostic, ct); + if (pairs.Count == 0) + return FixLookupResult.NotFound(FixNotFoundReason.NoFixForDiagnostic); - // Find the diagnostic at the specified location - var diagnostic = await FindDiagnosticAsync(session, document, diagnosticId, line, column, ct, analyzerProvider); - if (diagnostic is null) - return []; + return FixLookupResult.Found([.. pairs.Select(p => ToInfo(p.Provider, p.Action))]); + } - // Collect code actions from all providers - var fixes = new List(); + /// + /// Applies a specific code fix and returns the modified content without writing to disk, or the + /// reason no fix applies. + /// + public async Task ApplyFixAsync( + ProjectSession session, + string filePath, + string diagnosticId, + int line, + int column, + string equivalenceKey, + IAnalyzerProvider analyzerProvider, + CancellationToken ct = default) + { + var (located, notFound) = await LocateAsync(session, filePath, diagnosticId, line, column, analyzerProvider, ct); + if (notFound is { } reason) + return FixApplyResult.NotFound(reason); - foreach (var fixProvider in providers) - { - var actions = new List(); + var (document, diagnostic, providers) = located!.Value; + var pairs = await CollectActionsAsync(providers, document, diagnostic, ct); + if (pairs.Count == 0) + return FixApplyResult.NotFound(FixNotFoundReason.NoFixForDiagnostic); - var context = new CodeFixContext( - document, - diagnostic.Location.SourceSpan, - ImmutableArray.Create(diagnostic), - (action, _) => actions.Add(action), - ct); + var matching = pairs.Where(p => KeyOf(p.Action) == equivalenceKey).ToList(); + if (matching.Count == 0) + return FixApplyResult.NotFound(FixNotFoundReason.NoFixForEquivalenceKey, ToCandidates(pairs)); - await fixProvider.RegisterCodeFixesAsync(context); + var originalText = (await document.GetTextAsync(ct)).ToString(); - foreach (var action in actions) + foreach (var (_, matchingAction) in matching) + { + // Apply the code action to get the modified document + var operations = await matchingAction.GetOperationsAsync(ct); + + foreach (var operation in operations) { - fixes.Add(new CodeFixInfo( - Title: action.Title, - EquivalenceKey: action.EquivalenceKey ?? "", - DiagnosticId: diagnosticId, - ProviderName: fixProvider.GetType().Name)); + if (operation is ApplyChangesOperation applyChanges) + { + var changedSolution = applyChanges.ChangedSolution; + var changedDocument = changedSolution.GetDocument(document.Id); + if (changedDocument is null) + continue; + + var newText = await changedDocument.GetTextAsync(ct); + var modifiedContent = newText?.ToString() ?? ""; + + return FixApplyResult.Applied(new CodeFixResult( + FilePath: filePath, + OriginalContent: originalText, + ModifiedContent: modifiedContent, + FixTitle: matchingAction.Title)); + } } } - return fixes; + // The key matched, but no matching action changes this document. + return FixApplyResult.NotFound(FixNotFoundReason.NoFixForDiagnostic); } /// - /// Applies a specific code fix and returns the modified content without writing to disk. + /// The equivalence key a caller sees and passes back. A null key is advertised as "", so + /// every comparison goes through this too; otherwise a null-key action could never be selected. + /// + private static string KeyOf(CodeAction action) => action.EquivalenceKey ?? ""; + + private static CodeFixInfo ToInfo(CodeFixProvider provider, CodeAction action) => + new(KeyOf(action), action.Title, provider.GetType().Name); + + /// + /// One candidate per distinct equivalence key, in registration order. When two providers (or two + /// actions) share a key, the first occurrence wins: that is also the one a key match selects. /// - public async Task ApplyFixAsync( + private static IReadOnlyList ToCandidates(IEnumerable<(CodeFixProvider Provider, CodeAction Action)> pairs) => + [.. pairs + .GroupBy(p => KeyOf(p.Action), StringComparer.Ordinal) + .Select(g => ToInfo(g.First().Provider, g.First().Action))]; + + /// True when the project ruleset sets to None. + private static bool IsRulesetSuppressed(IAnalyzerProvider provider, string diagnosticId) => + provider is AnalyzerSet { RuleActions: var ruleActions } && RulesetFilter.IsSuppressed(ruleActions, diagnosticId, out _); + + /// + /// The shared front half of get_fixes and apply_fix: a fix provider, the document, no ruleset + /// suppression, and the diagnostic at the position, in that order. Returns the first failing reason. + /// + private static async Task<((Document Document, Diagnostic Diagnostic, ImmutableArray Providers)? Located, FixNotFoundReason? NotFound)> LocateAsync( ProjectSession session, string filePath, string diagnosticId, int line, int column, - string equivalenceKey, IAnalyzerProvider analyzerProvider, - CancellationToken ct = default) + CancellationToken ct) { var providers = analyzerProvider.GetCodeFixProvidersForDiagnostic(diagnosticId); if (providers.IsEmpty) - return null; + return (null, FixNotFoundReason.NoFixProvider); var document = session.GetDocument(filePath); if (document is null) - return null; + return (null, FixNotFoundReason.FileNotInProject); - // Find the diagnostic - var diagnostic = await FindDiagnosticAsync(session, document, diagnosticId, line, column, ct, analyzerProvider); - if (diagnostic is null) - return null; + if (IsRulesetSuppressed(analyzerProvider, diagnosticId)) + return (null, FixNotFoundReason.SuppressedByRuleset); + + var lookup = await FindDiagnosticAsync(session, document, diagnosticId, line, column, ct, analyzerProvider); + if (lookup.Reason is { } reason) + return (null, reason); + + return ((document, lookup.Diagnostic!, providers), null); + } + + /// Every code action every provider registers for , in provider order. + private static async Task> CollectActionsAsync( + ImmutableArray providers, + Document document, + Diagnostic diagnostic, + CancellationToken ct) + { + var pairs = new List<(CodeFixProvider Provider, CodeAction Action)>(); - // Find the matching code action foreach (var fixProvider in providers) { var actions = new List(); @@ -105,39 +169,11 @@ public async Task> GetFixesAsync( await fixProvider.RegisterCodeFixesAsync(context); - var matchingAction = actions.FirstOrDefault(a => - string.Equals(a.EquivalenceKey, equivalenceKey, StringComparison.Ordinal)); - - if (matchingAction is null) - continue; - - var originalText = (await document.GetTextAsync(ct)).ToString(); - - // Apply the code action to get the modified document - var operations = await matchingAction.GetOperationsAsync(ct); - - foreach (var operation in operations) - { - if (operation is ApplyChangesOperation applyChanges) - { - var changedSolution = applyChanges.ChangedSolution; - var changedDocument = changedSolution.GetDocument(document.Id); - if (changedDocument is null) - continue; - - var newText = await changedDocument.GetTextAsync(ct); - var modifiedContent = newText?.ToString() ?? ""; - - return new CodeFixResult( - FilePath: filePath, - OriginalContent: originalText, - ModifiedContent: modifiedContent, - FixTitle: matchingAction.Title); - } - } + foreach (var action in actions) + pairs.Add((fixProvider, action)); } - return null; + return pairs; } /// @@ -158,7 +194,11 @@ public async Task ApplyFixAllAsync( { var providers = analyzerProvider.GetCodeFixProvidersForDiagnostic(diagnosticId); if (providers.IsEmpty) - return NoFixAvailable(diagnosticId); + return NotFound(diagnosticId, FixNotFoundReason.NoFixProvider); + + // Checked up front so a suppressed rule is reported as such rather than as zero occurrences. + if (IsRulesetSuppressed(analyzerProvider, diagnosticId)) + return NotFound(diagnosticId, FixNotFoundReason.SuppressedByRuleset); var normalizedFilePath = filePath is null ? null : Path.GetFullPath(filePath); @@ -184,34 +224,30 @@ public async Task ApplyFixAllAsync( } if (candidateActions.Count == 0) - return NoFixAvailable(diagnosticId, diagnostics.Length); + return NotFound(diagnosticId, FixNotFoundReason.NoFixForDiagnostic, diagnostics.Length); (CodeFixProvider Provider, CodeAction Action) chosen; if (equivalenceKey is not null) { - var match = candidateActions.FirstOrDefault(c => - string.Equals(c.Action.EquivalenceKey, equivalenceKey, StringComparison.Ordinal)); + var match = candidateActions.FirstOrDefault(c => KeyOf(c.Action) == equivalenceKey); if (match.Action is null) - return NoFixAvailable(diagnosticId, diagnostics.Length); + return NotFound(diagnosticId, FixNotFoundReason.NoFixForEquivalenceKey, diagnostics.Length, + ToCandidates(candidateActions)); chosen = match; } else { - var distinctKeys = candidateActions - .Select(c => c.Action.EquivalenceKey ?? "") - .Distinct(StringComparer.Ordinal) - .ToList(); - - if (distinctKeys.Count > 1) + var candidates = ToCandidates(candidateActions); + if (candidates.Count > 1) return new FixAllResult( - FixAllStatus.AmbiguousFix, diagnosticId, diagnostics.Length, - null, null, [], distinctKeys, []); + FixAllStatus.Ambiguous, diagnosticId, diagnostics.Length, + null, null, [], candidates, []); chosen = candidateActions[0]; } var fixProviderInstance = chosen.Provider; - var chosenKey = chosen.Action.EquivalenceKey ?? ""; + var chosenKey = KeyOf(chosen.Action); var fixTitle = chosen.Action.Title; var project = session.GetProject(); @@ -304,8 +340,9 @@ public async Task ApplyFixAllAsync( changes, [], unfixed); } - private static FixAllResult NoFixAvailable(string diagnosticId, int diagnosticsFound = 0) => - new(FixAllStatus.NoFixAvailable, diagnosticId, diagnosticsFound, null, null, [], [], []); + private static FixAllResult NotFound( + string diagnosticId, FixNotFoundReason reason, int diagnosticsFound = 0, IReadOnlyList? candidates = null) => + new(FixAllStatus.NotFound, diagnosticId, diagnosticsFound, null, null, [], candidates ?? [], [], reason); /// /// Applies fixes to a single document one diagnostic at a time, in descending source-position @@ -330,8 +367,7 @@ private static async Task ApplyIterativeFixesAsync( await fixProvider.RegisterCodeFixesAsync(context); - var match = actions.FirstOrDefault(a => - string.Equals(a.EquivalenceKey, equivalenceKey, StringComparison.Ordinal)); + var match = actions.FirstOrDefault(a => KeyOf(a) == equivalenceKey); if (match is null) continue; @@ -467,7 +503,13 @@ public override Task> GetAllDiagnosticsAsync( Task.FromResult(_all.Where(d => diagnosticIds.Contains(d.Id))); } - private static async Task FindDiagnosticAsync( + /// + /// Finds the diagnostic at a position, or says why there is none. Ruleset suppression is checked by + /// the caller before analysis; pragma suppression is detected here, which relies on + /// GetEffectiveDiagnostics keeping pragma-suppressed diagnostics with IsSuppressed = true + /// rather than dropping them. + /// + private static async Task FindDiagnosticAsync( ProjectSession session, Document document, string diagnosticId, @@ -484,7 +526,7 @@ public override Task> GetAllDiagnosticsAsync( .ToImmutableArray(); if (analyzers.IsEmpty) - return null; + return new DiagnosticLookup(null, FixNotFoundReason.NoAnalyzerForRule); var compilationWithAnalyzers = new CompilationWithAnalyzers( compilation, analyzers, null!, ct); @@ -495,19 +537,32 @@ public override Task> GetAllDiagnosticsAsync( var effectiveDiagnostics = CompilationWithAnalyzers .GetEffectiveDiagnostics(rawDiagnostics, compilation); - // Apply ruleset suppression (RuleAction.None) - var ruleActions = provider is AnalyzerSet analyzerSet ? analyzerSet.RuleActions : null; - - // Filter to the target file and diagnostic ID + // Filter to the target file and diagnostic ID. Suppressed diagnostics are kept here so a + // pragma-suppressed hit can be told apart from no hit at all. var documentPath = document.FilePath ?? ""; var diagnostics = effectiveDiagnostics - .Where(d => !d.IsSuppressed && !RulesetFilter.IsSuppressed(ruleActions, d.Id, out _)) .Where(d => d.Id == diagnosticId && d.Location.SourceTree?.FilePath is string fp && Path.GetFullPath(fp).Equals(Path.GetFullPath(documentPath), StringComparison.OrdinalIgnoreCase)) .ToImmutableArray(); - // Find the diagnostic at or near the specified line/column (1-based input) + if (AtPosition(diagnostics.Where(d => !d.IsSuppressed), line, column) is { } hit) + return new DiagnosticLookup(hit, null); + + if (AtPosition(diagnostics.Where(d => d.IsSuppressed), line, column) is not null) + return new DiagnosticLookup(null, FixNotFoundReason.SuppressedByPragma); + + return new DiagnosticLookup(null, FixNotFoundReason.NoDiagnosticAtPosition); + } + + /// + /// The diagnostic starting exactly at the 1-based /, + /// or else the first one starting on that line. + /// + private static Diagnostic? AtPosition(IEnumerable candidates, int line, int column) + { + var diagnostics = candidates.ToList(); + return diagnostics.FirstOrDefault(d => { var lineSpan = d.Location.GetLineSpan(); diff --git a/src/ALCops.Mcp/Services/ProjectScope.cs b/src/ALCops.Mcp/Services/ProjectScope.cs new file mode 100644 index 0000000..4e7d1ea --- /dev/null +++ b/src/ALCops.Mcp/Services/ProjectScope.cs @@ -0,0 +1,77 @@ +namespace ALCops.Mcp.Services; + +/// +/// Project argument validation shared by the native tools. A failure is an Invalid error with +/// the returned message. +/// +internal static class ProjectScope +{ + private const string EmptyProjectListMessage = + "No AL project available. Pass projectPath, or start the server from a folder " + + "containing app.json (or use --projects)."; + + /// + /// For tools scoped to the projects this server was started with (analyze, list_rules): + /// resolves , or the primary project when it is null, and rejects a + /// path that is not one of . Returns the + /// normalized project folder, or null with set. + /// + internal static string? Resolve(WorkspaceStartupConfig config, string? projectPath, out string? invalidMessage) + { + var knownProjects = config.ProjectDirectories.Select(Normalize).ToList(); + + if (knownProjects.Count == 0) + { + invalidMessage = EmptyProjectListMessage; + return null; + } + + if (projectPath is null) + { + invalidMessage = null; + return knownProjects[0]; + } + + var normalized = Normalize(projectPath); + if (!knownProjects.Contains(normalized, StringComparer.OrdinalIgnoreCase)) + { + invalidMessage = + $"'{projectPath}' is not one of the AL projects this server was started with: " + + $"{string.Join(", ", knownProjects)}. Pass one of those, or restart the server with --projects."; + return null; + } + + invalidMessage = null; + return normalized; + } + + /// + /// For the fix tools, which accept any AL project: the folder must exist and contain app.json. + /// + internal static bool RequireProjectFolder(string projectPath, out string? invalidMessage) + { + if (string.IsNullOrWhiteSpace(projectPath)) + { + invalidMessage = "projectPath is required: the absolute path of the AL project folder (contains app.json)."; + return false; + } + + var full = Path.GetFullPath(projectPath); + if (!Directory.Exists(full)) + { + invalidMessage = $"projectPath '{projectPath}' does not exist. Pass the absolute path of the AL project folder (contains app.json)."; + return false; + } + + if (!File.Exists(Path.Combine(full, "app.json"))) + { + invalidMessage = $"projectPath '{projectPath}' contains no app.json. Pass the AL project folder itself, not a parent or child folder."; + return false; + } + + invalidMessage = null; + return true; + } + + private static string Normalize(string path) => Path.TrimEndingDirectorySeparator(Path.GetFullPath(path)); +} diff --git a/src/ALCops.Mcp/Services/ToolErrors.cs b/src/ALCops.Mcp/Services/ToolErrors.cs new file mode 100644 index 0000000..7318bed --- /dev/null +++ b/src/ALCops.Mcp/Services/ToolErrors.cs @@ -0,0 +1,100 @@ +using System.Text.Json; +using ALCops.Mcp.Models; +using ModelContextProtocol.Protocol; + +namespace ALCops.Mcp.Services; + +/// +/// The only place a native tool error is built. Every result has +/// set and carries a envelope as its single text block, so a client that hides +/// error results still has the full message. +/// +internal static class ToolErrors +{ + /// The arguments are unusable as given. + internal static CallToolResult Invalid(string message) => + Build(new ToolError(ToolErrorCode.Invalid, message)); + + /// Nothing matched; is written by name. + internal static CallToolResult NotFound( + FixNotFoundReason reason, + string message, + string? filePath = null, + string? diagnosticId = null, + IReadOnlyList? candidates = null) => + Build(new ToolError(ToolErrorCode.NotFound, message, reason.ToString(), candidates, filePath, diagnosticId)); + + /// Several distinct fixes apply; the caller has to pick one of . + internal static CallToolResult Ambiguous( + string message, + IReadOnlyList candidates, + string? filePath = null, + string? diagnosticId = null) => + Build(new ToolError(ToolErrorCode.Ambiguous, message, Candidates: candidates, FilePath: filePath, DiagnosticId: diagnosticId)); + + /// The file changed or was deleted after the fix was computed; nothing was written. + internal static CallToolResult Stale(string filePath, string? diagnosticId, string message) => + Build(new ToolError(ToolErrorCode.Stale, message, FilePath: filePath, DiagnosticId: diagnosticId)); + + /// A dependency is down; is an value. + internal static CallToolResult Unavailable(string reason, string message, string? detail = null) => + Build(new ToolError(ToolErrorCode.Unavailable, message, reason, Detail: detail)); + + /// An unexpected exception: message is its message, detail its full type name. + internal static CallToolResult Faulted(Exception ex) => + Build(new ToolError(ToolErrorCode.Faulted, ex.Message, Detail: ex.GetType().FullName)); + + /// A refused or failed write; is the . + internal static CallToolResult Faulted( + string reason, + string message, + string? filePath = null, + string? diagnosticId = null, + string? detail = null) => + Build(new ToolError(ToolErrorCode.Faulted, message, reason, FilePath: filePath, DiagnosticId: diagnosticId, Detail: detail)); + + /// + /// The shared wording of a message, so the three fix tools + /// say the same thing for the same reason. / are + /// null for apply_fix_all, which has no position. + /// + internal static string NotFoundMessage( + FixNotFoundReason reason, + string diagnosticId, + string? filePath, + int? line = null, + int? column = null, + string? equivalenceKey = null) + { + var at = line is null + ? (filePath is null ? "in the project" : $"in {filePath}") + : $"at {filePath}:{line}:{column}"; + + return reason switch + { + FixNotFoundReason.NoFixProvider => + $"No code fix provider is registered for {diagnosticId} by the configured analyzers.", + FixNotFoundReason.FileNotInProject => + $"{filePath} is not an .al file of this project.", + FixNotFoundReason.NoAnalyzerForRule => + $"No loaded analyzer reports {diagnosticId}, so the diagnostic cannot be located. Check al.codeAnalyzers.", + FixNotFoundReason.SuppressedByRuleset => + $"{diagnosticId} is suppressed by the project ruleset (action None); there is nothing to fix.", + FixNotFoundReason.SuppressedByPragma => + $"{diagnosticId} {at} is suppressed by #pragma warning disable; there is nothing to fix.", + FixNotFoundReason.NoDiagnosticAtPosition => + $"No {diagnosticId} {at}; re-run analyze and use its line/column.", + FixNotFoundReason.NoFixForDiagnostic => + $"{diagnosticId} {at} was found, but no code fix applies to it.", + FixNotFoundReason.NoFixForEquivalenceKey => + $"No fix for {diagnosticId} {at} has equivalence key '{equivalenceKey}'. Pass one candidate's equivalenceKey verbatim.", + _ => $"No fix found for {diagnosticId} {at}.", + }; + } + + private static CallToolResult Build(ToolError error) => new() + { + IsError = true, + Content = [new TextContentBlock { Text = JsonSerializer.Serialize(error, JsonDefaults.Options) }] + }; +} diff --git a/src/ALCops.Mcp/Services/ToolResults.cs b/src/ALCops.Mcp/Services/ToolResults.cs new file mode 100644 index 0000000..b0ffb54 --- /dev/null +++ b/src/ALCops.Mcp/Services/ToolResults.cs @@ -0,0 +1,19 @@ +using System.Text.Json; +using ModelContextProtocol.Protocol; + +namespace ALCops.Mcp.Services; + +/// +/// Success results of the native tools. Errors go through only. +/// +internal static class ToolResults +{ + /// + /// The payload serialized with as a single text block. + /// is left unset: only errors set it. + /// + internal static CallToolResult Ok(T payload) => new() + { + Content = [new TextContentBlock { Text = JsonSerializer.Serialize(payload, JsonDefaults.Options) }] + }; +} diff --git a/src/ALCops.Mcp/Tools/AnalyzeTool.cs b/src/ALCops.Mcp/Tools/AnalyzeTool.cs index e55a687..5940c8b 100644 --- a/src/ALCops.Mcp/Tools/AnalyzeTool.cs +++ b/src/ALCops.Mcp/Tools/AnalyzeTool.cs @@ -14,7 +14,7 @@ public sealed class AnalyzeTool [McpServerTool(Name = "analyze", ReadOnly = true), Description("Compile the AL workspace with all configured analyzers and return cop + compiler diagnostics as structured JSON. Wraps Microsoft's al_compile (onlyErrors=false, enableCodeAnalysis=true, no diagnostic cap) using the analyzers and ruleset the server passed to almcp at startup, then enriches each diagnostic with the owning analyzer ('CodeCop', 'ALCops.LinterCop', ..., or 'Compiler' for AL#### errors) and whether a native code fix exists (hasFix). Prefer this over al_compile or al_getdiagnostics whenever you want cop diagnostics: al_compile hides warnings unless you remember onlyErrors=false, and al_getdiagnostics never runs analyzers. Scope with filePath, folderPath or projectPath (combined with AND); filter with severities, analyzers, ruleIds; cap with limit (default 500). totalCount, truncated and summary always describe the full filtered set. Results are sorted by filePath, line, column. Scoping: without any scope argument, results are limited to the startup project. With filePath or folderPath and no projectPath, the file/folder is the only scope and analyzer/hasFix come from the analyzer configuration of the project that contains it (falling back to the startup project). The 'project' field names that project. Next steps: for a diagnostic with hasFix=true call get_fixes (then apply_fix) at its filePath/line/column/id, or apply_fix_all for every occurrence of one rule. After apply_fix / apply_fix_all, call analyze again to verify; almcp's file watcher normally sees the write first, but on slow file systems or right after a large apply_fix_all a second call may be needed before the fixed diagnostic disappears.")] - public static async Task Analyze( + public static async Task Analyze( IServiceProvider services, ProjectAnalyzerResolver analyzerResolver, WorkspaceStartupResolver workspaceResolver, @@ -29,16 +29,25 @@ public static async Task Analyze( { try { + if (limit <= 0) + return ToolErrors.Invalid("limit must be a positive integer."); + var proxy = services.GetService(typeof(AlMcpProxy)) as AlMcpProxy; if (proxy is null) - return Error("ProxyUnavailable", + return ToolErrors.Unavailable(UnavailableReason.NoProxy, "analyze wraps the proxied al_compile, but this server runs with --no-proxy. " + "Restart without --no-proxy to use analyze."); - if (!proxy.IsAvailable || !await proxy.Ready.WaitAsync(cancellationToken)) - return Error("ProxyUnavailable", - "MS AL MCP Server (almcp) is not available (not found in the DevTools directory, or it failed to start). " + - "See the server log on stderr."); + // IsAvailable never changes and Ready never regresses from false to true, so these two + // checks are not racy; they are what tells "not installed" apart from "failed to start". + if (!proxy.IsAvailable) + return ToolErrors.Unavailable(UnavailableReason.AlmcpNotFound, + "almcp was not found in the DevTools directory, so analyze (which wraps al_compile) is unavailable. " + + "Install BC DevTools 17.0 or later; see the server log on stderr."); + + if (!await proxy.Ready.WaitAsync(cancellationToken)) + return ToolErrors.Unavailable(UnavailableReason.AlmcpNotReady, + "almcp failed to start; see the server log on stderr."); var callerPassedProjectPath = projectPath is not null; var callerPassedFileOrFolder = filePath is not null || folderPath is not null; @@ -48,40 +57,19 @@ public static async Task Analyze( var normalizedFolderPath = folderPath is not null ? Path.TrimEndingDirectorySeparator(Path.GetFullPath(folderPath)) : null; var scopePath = normalizedFilePath ?? normalizedFolderPath; - string? enrichmentProject; - if (callerPassedProjectPath) - { - enrichmentProject = Path.TrimEndingDirectorySeparator(Path.GetFullPath(projectPath!)); - - var knownProjects = workspaceResolver.Config.ProjectDirectories - .Select(p => Path.TrimEndingDirectorySeparator(Path.GetFullPath(p))).ToList(); - if (!knownProjects.Contains(enrichmentProject, StringComparer.OrdinalIgnoreCase)) - return Error("UnknownProject", - $"'{projectPath}' is not one of the AL projects this server was started with: " + - $"{string.Join(", ", knownProjects)}. Pass one of those, or restart the server with --projects."); - } - else if (scopePath is not null) - { - enrichmentProject = CompileDiagnosticsParser.FindContainingProject( - scopePath, workspaceResolver.Config.ProjectDirectories) - ?? workspaceResolver.Config.PrimaryProject; - } - else - { - enrichmentProject = workspaceResolver.Config.PrimaryProject; - } + var config = workspaceResolver.Config; + string? invalidMessage = null; + var enrichmentProject = !callerPassedProjectPath && scopePath is not null + ? CompileDiagnosticsParser.FindContainingProject(scopePath, config.ProjectDirectories) + ?? ProjectScope.Resolve(config, null, out invalidMessage) + : ProjectScope.Resolve(config, projectPath, out invalidMessage); if (enrichmentProject is null) - return Error("NoProject", - "No AL project available. Pass projectPath, or start the server from a folder " + - "containing app.json (or use --projects)."); + return ToolErrors.Invalid(invalidMessage!); // Project filter applies when projectPath is explicit or when no file/folder scope was given. var projectScopeFilter = callerPassedFileOrFolder && !callerPassedProjectPath ? null : enrichmentProject; - if (limit <= 0) - return Error("InvalidLimit", "limit must be a positive integer."); - HashSet? severitySet = severities is { Length: > 0 } ? new HashSet(severities, StringComparer.OrdinalIgnoreCase) : null; HashSet? analyzerSet = analyzers is { Length: > 0 } @@ -105,11 +93,7 @@ public static async Task Analyze( var result = await proxy.ForwardAsync("al_compile", args, cancellationToken); if (result.IsError == true) - { - var errorMessage = string.Join('\n', result.Content.OfType().Select(b => b.Text)); - return Error("ProxyCallFailed", - "The proxied al_compile call failed (almcp may have exited, or its session was lost): " + errorMessage); - } + return MapProxyFailure(result); var (raw, message, succeeded) = CompileDiagnosticsParser.Parse(result); warnings.AddRange(CompileDiagnosticsParser.ExtractWarnings(message)); @@ -125,14 +109,24 @@ public static async Task Analyze( var analyzeResult = CompileDiagnosticsParser.Build(enrichmentProject, sorted, limit, warnings); - return JsonSerializer.Serialize(analyzeResult, JsonDefaults.Options); + return ToolResults.Ok(analyzeResult); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; } catch (Exception ex) { - return Error(ex.GetType().Name, ex.Message); + return ToolErrors.Faulted(ex); } } - private static string Error(string code, string message) => - JsonSerializer.Serialize(new { error = code, message }, JsonDefaults.Options); + /// + /// Maps a failed proxied al_compile (almcp gone, session lost, or an error al_compile itself + /// reported) to Unavailable/AlmcpCallFailed, with the almcp text in detail. + /// + internal static CallToolResult MapProxyFailure(CallToolResult result) => + ToolErrors.Unavailable(UnavailableReason.AlmcpCallFailed, + "The proxied al_compile call failed (almcp may have exited, its session was lost, or al_compile itself reported an error).", + string.Join('\n', result.Content.OfType().Select(b => b.Text))); } diff --git a/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs b/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs index e5a9589..287bdcf 100644 --- a/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs +++ b/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs @@ -1,8 +1,8 @@ using System.ComponentModel; -using System.Text.Json; using ALCops.Mcp.Models; using ALCops.Mcp.Services; using Microsoft.Dynamics.Nav.CodeAnalysis.CodeFixes; +using ModelContextProtocol.Protocol; using ModelContextProtocol.Server; namespace ALCops.Mcp.Tools; @@ -13,13 +13,16 @@ public sealed class ApplyFixAllTool [McpServerTool(Name = "apply_fix_all", ReadOnly = false, Destructive = false), Description("Apply a code fix to every occurrence of a diagnostic rule across a project (or a single file). " + "Runs analysis once, then fixes all matches for that rule ID in one pass — like VS Code's 'Fix all in workspace'. " + - "Writes changed files directly to disk unless dryRun is true. Use get_fixes first to discover equivalenceKey options. " + + "Writes changed files directly to disk unless dryRun is true. " + + "If the rule offers more than one distinct fix and no equivalenceKey is given, returns the error Ambiguous whose candidates " + + "list each fix's equivalenceKey, title and providerName; pass one equivalenceKey verbatim. " + + "Zero occurrences is a success (applied: false, diagnosticsFound: 0), except a rule the project ruleset suppresses, which is NotFound with reason SuppressedByRuleset. " + "Changed project files are re-read from disk first. Files that change on disk while the fix is being computed " + "(or that cannot be read, or are not valid in their detected encoding) " + "are left untouched and listed in 'conflicts' (each with a 'kind') and their diagnostics remain in 'unfixedDiagnostics' (positions as analysed, so they may have shifted if the file was edited); the other files are still written. " + "If a write fails, every file written in this call is restored (unless it was edited since) and all of them are listed in 'conflicts'. " + "Verify with analyze or al_compile (options.onlyErrors: false).")] - public static async Task ApplyFixAll( + public static async Task ApplyFixAll( ProjectSessionManager sessionManager, CodeFixRunner codeFixRunner, ProjectAnalyzerResolver analyzerResolver, @@ -42,14 +45,13 @@ public static async Task ApplyFixAll( else if (string.Equals(scope, "document", StringComparison.OrdinalIgnoreCase)) fixAllScope = FixAllScope.Document; else - return JsonSerializer.Serialize( - new { error = "InvalidScope", message = $"Unknown scope '{scope}'. Use 'project' or 'document'." }, - JsonDefaults.Options); + return ToolErrors.Invalid($"Unknown scope '{scope}'. Use 'project' or 'document'."); if (fixAllScope == FixAllScope.Document && string.IsNullOrWhiteSpace(filePath)) - return JsonSerializer.Serialize( - new { error = "MissingFilePath", message = "filePath is required when scope='document'." }, - JsonDefaults.Options); + return ToolErrors.Invalid("filePath is required when scope='document'."); + + if (!ProjectScope.RequireProjectFolder(projectPath, out var invalidMessage)) + return ToolErrors.Invalid(invalidMessage!); string? warning = null; if (fixAllScope == FixAllScope.Project && !string.IsNullOrWhiteSpace(filePath)) @@ -69,7 +71,7 @@ public static async Task ApplyFixAll( switch (result.Status) { case FixAllStatus.NoDiagnosticsFound: - return JsonSerializer.Serialize(new + return ToolResults.Ok(new { applied = false, dryRun, @@ -77,26 +79,19 @@ public static async Task ApplyFixAll( diagnosticsFound = 0, message = $"No occurrences of {diagnosticId} were found in the given scope.", warning - }, JsonDefaults.Options); - - case FixAllStatus.NoFixAvailable: - return JsonSerializer.Serialize(new - { - error = "NoFixAvailable", - message = $"No applicable code fix found for {diagnosticId}" + - (equivalenceKey is not null ? $" with equivalence key '{equivalenceKey}'." : "."), - diagnosticsFound = result.DiagnosticsFound - }, JsonDefaults.Options); - - case FixAllStatus.AmbiguousFix: - return JsonSerializer.Serialize(new - { - error = "AmbiguousFix", - message = $"{diagnosticId} has multiple distinct fixes available. " + - "Call get_fixes on one occurrence to see titles, then pass the desired equivalenceKey.", - diagnosticsFound = result.DiagnosticsFound, - availableEquivalenceKeys = result.AvailableEquivalenceKeys - }, JsonDefaults.Options); + }); + + case FixAllStatus.NotFound: + var reason = result.NotFoundReason!.Value; + return ToolErrors.NotFound(reason, + ToolErrors.NotFoundMessage(reason, diagnosticId, filePath, equivalenceKey: equivalenceKey), + filePath, diagnosticId, + reason == FixNotFoundReason.NoFixForEquivalenceKey ? result.Candidates : null); + + case FixAllStatus.Ambiguous: + return ToolErrors.Ambiguous( + $"{diagnosticId} has {result.Candidates.Count} distinct fixes. Pass one candidate's equivalenceKey verbatim.", + result.Candidates, filePath, diagnosticId); } // Completed @@ -128,7 +123,7 @@ public static async Task ApplyFixAll( var unfixed = MergeUnfixed(result, conflicts); - return JsonSerializer.Serialize(new + return ToolResults.Ok(new { applied = !dryRun && written.Count > 0, dryRun, @@ -141,11 +136,15 @@ public static async Task ApplyFixAll( message = conflictMessage, unfixedDiagnostics = unfixed, warning - }, JsonDefaults.Options); + }); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; } catch (Exception ex) { - return JsonSerializer.Serialize(new { error = ex.GetType().Name, message = ex.Message }, JsonDefaults.Options); + return ToolErrors.Faulted(ex); } } diff --git a/src/ALCops.Mcp/Tools/ApplyFixTool.cs b/src/ALCops.Mcp/Tools/ApplyFixTool.cs index 012f895..62aabae 100644 --- a/src/ALCops.Mcp/Tools/ApplyFixTool.cs +++ b/src/ALCops.Mcp/Tools/ApplyFixTool.cs @@ -1,7 +1,7 @@ using System.ComponentModel; -using System.Text.Json; using ALCops.Mcp.Models; using ALCops.Mcp.Services; +using ModelContextProtocol.Protocol; using ModelContextProtocol.Server; namespace ALCops.Mcp.Tools; @@ -12,12 +12,13 @@ public sealed class ApplyFixTool [McpServerTool(Name = "apply_fix", ReadOnly = false, Destructive = false), Description("Apply a code fix to resolve a diagnostic. Changed project files are re-read from disk first. " + "Writes the fixed content to the file on disk unless the file changed after the fix was computed, " + - "in which case nothing is written and { error: 'StaleFile' } is returned. " + + "in which case nothing is written and the error Stale is returned (re-run). " + "A file that is not valid in its detected encoding (e.g. a Windows-1252 file read as UTF-8) is never re-encoded: " + - "nothing is written and { error: 'UnsupportedEncoding' } is returned ({ error: 'ReadFailed' } if the file cannot be read). " + + "nothing is written and the error Faulted with reason UnsupportedEncoding is returned (reason ReadFailed if the file cannot be read, WriteFailed if the write fails). " + + "When no fix matches, returns NotFound with a reason, the same as get_fixes; for NoFixForEquivalenceKey, candidates lists the keys that do apply. " + "The write is atomic and preserves the file's encoding and line endings. " + "Verify with analyze or al_compile (options.onlyErrors: false).")] - public static async Task ApplyFix( + public static async Task ApplyFix( ProjectSessionManager sessionManager, CodeFixRunner codeFixRunner, ProjectAnalyzerResolver analyzerResolver, @@ -27,51 +28,68 @@ public static async Task ApplyFix( [Description("The diagnostic rule ID (e.g., 'AC0018', 'LC0001').")] string diagnosticId, [Description("Line number of the diagnostic (1-based).")] int line, [Description("Column number of the diagnostic (1-based).")] int column, - [Description("Equivalence key of the fix to apply (from get_fixes results).")] string equivalenceKey, + [Description("Equivalence key of the fix to apply (from get_fixes results), verbatim.")] string equivalenceKey, [Description("Optional: JSON array of analyzer specs (e.g., '[\"${CodeCop}\",\"${UICop}\"]'). If omitted, auto-discovers from .vscode/settings.json.")] string? analyzers = null, CancellationToken cancellationToken = default) { try { + if (!ProjectScope.RequireProjectFolder(projectPath, out var invalidMessage)) + return ToolErrors.Invalid(invalidMessage!); + var session = await sessionManager.GetOrLoadProjectAsync(projectPath, cancellationToken); var analyzerSpecs = AnalyzerSpec.ParseJsonArray(analyzers); var analyzerSet = await analyzerResolver.ResolveAsync(projectPath, analyzerSpecs, cancellationToken); - var result = await codeFixRunner.ApplyFixAsync( + var outcome = await codeFixRunner.ApplyFixAsync( session, filePath, diagnosticId, line, column, equivalenceKey, analyzerSet, cancellationToken); - if (result is null) - return JsonSerializer.Serialize( - new { error = "NoFixFound", message = $"No code fix with equivalence key '{equivalenceKey}' found for {diagnosticId} at {filePath}:{line}:{column}." }, - JsonDefaults.Options); + if (outcome.NotFoundReason is { } reason) + return ToolErrors.NotFound(reason, + ToolErrors.NotFoundMessage(reason, diagnosticId, filePath, line, column, equivalenceKey), + filePath, diagnosticId, + reason == FixNotFoundReason.NoFixForEquivalenceKey ? outcome.Candidates : null); + + var fix = outcome.Fix!; - var conflict = await fileWriter.WriteIfUnchangedAsync( - filePath, result.OriginalContent, result.ModifiedContent, cancellationToken); + FileWriteConflict? conflict; + try + { + conflict = await fileWriter.WriteIfUnchangedAsync( + filePath, fix.OriginalContent, fix.ModifiedContent, cancellationToken); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + Console.Error.WriteLine($"Warning: {filePath} could not be written: {ex.Message}"); + return ToolErrors.Faulted(FileWriteConflictKind.WriteFailed, + $"{filePath} could not be written ({ex.GetType().Name}: {ex.Message}); the file is unchanged.", + filePath, diagnosticId, detail: ex.GetType().FullName); + } if (conflict is not null) { Console.Error.WriteLine($"Warning: {conflict.Message}"); - return JsonSerializer.Serialize(new - { - error = conflict.Kind, - message = conflict.Message, - filePath = conflict.FilePath, - diagnosticId - }, JsonDefaults.Options); + return conflict.Kind == FileWriteConflictKind.StaleFile + ? ToolErrors.Stale(conflict.FilePath, diagnosticId, conflict.Message) + : ToolErrors.Faulted(conflict.Kind, conflict.Message, conflict.FilePath, diagnosticId); } - return JsonSerializer.Serialize(new + return ToolResults.Ok(new { applied = true, filePath, - fixTitle = result.FixTitle, + fixTitle = fix.FixTitle, diagnosticId - }, JsonDefaults.Options); + }); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; } catch (Exception ex) { - return JsonSerializer.Serialize(new { error = ex.GetType().Name, message = ex.Message }, JsonDefaults.Options); + return ToolErrors.Faulted(ex); } } } diff --git a/src/ALCops.Mcp/Tools/GetFixesTool.cs b/src/ALCops.Mcp/Tools/GetFixesTool.cs index faa3f9f..1e157ab 100644 --- a/src/ALCops.Mcp/Tools/GetFixesTool.cs +++ b/src/ALCops.Mcp/Tools/GetFixesTool.cs @@ -1,6 +1,7 @@ using System.ComponentModel; -using System.Text.Json; +using ALCops.Mcp.Models; using ALCops.Mcp.Services; +using ModelContextProtocol.Protocol; using ModelContextProtocol.Server; namespace ALCops.Mcp.Tools; @@ -9,8 +10,12 @@ namespace ALCops.Mcp.Tools; public sealed class GetFixesTool { [McpServerTool(Name = "get_fixes", ReadOnly = true), - Description("Get available code fixes for a specific diagnostic at a location. Returns fix titles and equivalence keys needed by apply_fix.")] - public static async Task GetFixes( + Description("Get available code fixes for a specific diagnostic at a location. " + + "Returns { diagnosticId, filePath, line, column, fixes: [{ equivalenceKey, title, providerName }] }; " + + "pass a fix's equivalenceKey verbatim to apply_fix (or apply_fix_all). fixes is never empty. " + + "When nothing matches, returns the error NotFound with a reason: NoDiagnosticAtPosition (re-run analyze and use its line/column), " + + "SuppressedByRuleset or SuppressedByPragma (nothing to fix), NoFixProvider, NoAnalyzerForRule, FileNotInProject or NoFixForDiagnostic.")] + public static async Task GetFixes( ProjectSessionManager sessionManager, CodeFixRunner codeFixRunner, ProjectAnalyzerResolver analyzerResolver, @@ -24,19 +29,32 @@ public static async Task GetFixes( { try { + if (!ProjectScope.RequireProjectFolder(projectPath, out var invalidMessage)) + return ToolErrors.Invalid(invalidMessage!); + var session = await sessionManager.GetOrLoadProjectAsync(projectPath, cancellationToken); var analyzerSpecs = AnalyzerSpec.ParseJsonArray(analyzers); var analyzerSet = await analyzerResolver.ResolveAsync(projectPath, analyzerSpecs, cancellationToken); - var fixes = await codeFixRunner.GetFixesAsync( + var lookup = await codeFixRunner.GetFixesAsync( session, filePath, diagnosticId, line, column, analyzerSet, cancellationToken); - return JsonSerializer.Serialize(fixes, JsonDefaults.Options); + if (lookup.NotFoundReason is { } reason) + return ToolErrors.NotFound(reason, + ToolErrors.NotFoundMessage(reason, diagnosticId, filePath, line, column), + filePath, diagnosticId); + + return ToolResults.Ok(new GetFixesResult( + diagnosticId, Path.GetFullPath(filePath), line, column, lookup.Fixes)); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + throw; } catch (Exception ex) { - return JsonSerializer.Serialize(new { error = ex.GetType().Name, message = ex.Message }, JsonDefaults.Options); + return ToolErrors.Faulted(ex); } } } diff --git a/src/ALCops.Mcp/Tools/ListRulesTool.cs b/src/ALCops.Mcp/Tools/ListRulesTool.cs index e520dac..22953e7 100644 --- a/src/ALCops.Mcp/Tools/ListRulesTool.cs +++ b/src/ALCops.Mcp/Tools/ListRulesTool.cs @@ -1,7 +1,7 @@ using System.ComponentModel; -using System.Text.Json; using ALCops.Mcp.Models; using ALCops.Mcp.Services; +using ModelContextProtocol.Protocol; using ModelContextProtocol.Server; namespace ALCops.Mcp.Tools; @@ -11,60 +11,67 @@ public sealed class ListRulesTool { [McpServerTool(Name = "list_rules", ReadOnly = true), Description("List available analyzer rules. Rules come from the analyzers the project configures via al.codeAnalyzers — nothing is bundled. By default returns a compact list (ID, title, cop name). Use verbose=true for full metadata including description, severity, category, help URI, and code fix availability.")] - public static async Task ListRules( + public static async Task ListRules( ProjectAnalyzerResolver analyzerResolver, WorkspaceStartupResolver workspaceResolver, - [Description("Optional: absolute path to the AL project folder. Defaults to the project discovered at startup.")] string? projectPath = null, + [Description("Optional: absolute path to one of the AL project folders this server was started with. Defaults to the project discovered at startup.")] string? projectPath = null, [Description("Filter rules by cop name (e.g., 'LinterCop', 'ApplicationCop', 'CodeCop'). Leave empty for all cops.")] string? copFilter = null, [Description("Optional: JSON array of analyzer specs (e.g., '[\"${CodeCop}\",\"${UICop}\"]'). If omitted, auto-discovers from .vscode/settings.json.")] string? analyzers = null, [Description("Return full rule metadata (description, severity, category, helpUri, hasCodeFix). Default: false.")] bool verbose = false, CancellationToken cancellationToken = default) { - // No projectPath: fall back to the project discovered at startup, the same one the proxied - // MS tools operate on. There is no project-independent rule list any more — analyzers are - // whatever the project configures. - projectPath ??= workspaceResolver.Config.PrimaryProject; - if (projectPath is null) - return JsonSerializer.Serialize(new - { - error = "NoProject", - message = "No AL project available. Pass projectPath, or start the server from a folder " + - "containing app.json (or use --projects)." - }, JsonDefaults.Options); + try + { + // No projectPath: fall back to the project discovered at startup, the same one the proxied + // MS tools operate on. There is no project-independent rule list any more — analyzers are + // whatever the project configures. An explicit projectPath must be one of the startup + // projects, exactly as for analyze. + var project = ProjectScope.Resolve(workspaceResolver.Config, projectPath, out var invalidMessage); + if (project is null) + return ToolErrors.Invalid(invalidMessage!); - var analyzerSpecs = AnalyzerSpec.ParseJsonArray(analyzers); - var provider = await analyzerResolver.ResolveAsync(projectPath, analyzerSpecs, cancellationToken); - var warnings = provider.Warnings.Count > 0 ? provider.Warnings : null; + var analyzerSpecs = AnalyzerSpec.ParseJsonArray(analyzers); + var provider = await analyzerResolver.ResolveAsync(project, analyzerSpecs, cancellationToken); + var warnings = provider.Warnings.Count > 0 ? provider.Warnings : null; - var descriptors = provider.GetAllDescriptors(); + var descriptors = provider.GetAllDescriptors(); - var filtered = descriptors.Values - .Select(d => (Descriptor: d, CopName: provider.GetCopName(d.Id))) - .Where(r => copFilter is null || r.CopName.Equals(copFilter, StringComparison.OrdinalIgnoreCase)) - .OrderBy(r => r.Descriptor.Id); + var filtered = descriptors.Values + .Select(d => (Descriptor: d, CopName: provider.GetCopName(d.Id))) + .Where(r => copFilter is null || r.CopName.Equals(copFilter, StringComparison.OrdinalIgnoreCase)) + .OrderBy(r => r.Descriptor.Id); - if (verbose) + if (verbose) + { + var rules = filtered.Select(r => new RuleInfo( + Id: r.Descriptor.Id, + Title: r.Descriptor.Title.ToString(), + Description: r.Descriptor.Description.ToString(), + Severity: r.Descriptor.DefaultSeverity.ToString(), + Category: r.Descriptor.Category, + CopName: r.CopName, + HasCodeFix: provider.HasCodeFix(r.Descriptor.Id), + HelpUri: r.Descriptor.HelpLinkUri)).ToList(); + return ToolResults.Ok(new { rules, warnings }); + } + else + { + var rules = filtered.Select(r => new + { + id = r.Descriptor.Id, + title = r.Descriptor.Title.ToString(), + cop = r.CopName + }).ToList(); + return ToolResults.Ok(new { rules, warnings }); + } + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - var rules = filtered.Select(r => new RuleInfo( - Id: r.Descriptor.Id, - Title: r.Descriptor.Title.ToString(), - Description: r.Descriptor.Description.ToString(), - Severity: r.Descriptor.DefaultSeverity.ToString(), - Category: r.Descriptor.Category, - CopName: r.CopName, - HasCodeFix: provider.HasCodeFix(r.Descriptor.Id), - HelpUri: r.Descriptor.HelpLinkUri)).ToList(); - return JsonSerializer.Serialize(new { rules, warnings }, JsonDefaults.Options); + throw; } - else + catch (Exception ex) { - var rules = filtered.Select(r => new - { - id = r.Descriptor.Id, - title = r.Descriptor.Title.ToString(), - cop = r.CopName - }).ToList(); - return JsonSerializer.Serialize(new { rules, warnings }, JsonDefaults.Options); + return ToolErrors.Faulted(ex); } } } diff --git a/tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs b/tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs index 556d83b..bb724e9 100644 --- a/tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs +++ b/tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs @@ -4,6 +4,7 @@ using ALCops.Mcp.Tools; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging.Abstractions; +using ModelContextProtocol.Protocol; using ModelContextProtocol.Server; using Xunit; using Xunit.Abstractions; @@ -12,26 +13,26 @@ namespace ALCops.Mcp.Tests; public sealed class AnalyzeToolTests { + private static WorkspaceStartupResolver DummyResolver(ProjectAnalyzerResolver analyzerResolver) => + new(analyzerResolver, + new ExternalAnalyzerLoader(TestAnalyzers.ToolsLocator), + NullLogger.Instance, + ["C:\\dummy"]); + [Fact] - public async Task ProxyUnavailable_WhenNoProxy() + public async Task Unavailable_NoProxy_WhenServerRunsWithNoProxy() { var services = new ServiceCollection().BuildServiceProvider(); var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); - var resolver = new WorkspaceStartupResolver( - analyzerResolver, - new ExternalAnalyzerLoader(TestAnalyzers.ToolsLocator), - NullLogger.Instance, - ["C:\\dummy"]); - var json = await AnalyzeTool.Analyze(services, analyzerResolver, resolver); - var doc = JsonDocument.Parse(json); + var result = await AnalyzeTool.Analyze(services, analyzerResolver, DummyResolver(analyzerResolver)); - Assert.Equal("ProxyUnavailable", doc.RootElement.GetProperty("error").GetString()); - Assert.Contains("--no-proxy", doc.RootElement.GetProperty("message").GetString()); + var root = ToolResultAssert.Error(result, "Unavailable", "NoProxy"); + Assert.Contains("--no-proxy", root.GetProperty("message").GetString()); } [Fact] - public async Task ProxyUnavailable_WhenAlmcpNotFound() + public async Task Unavailable_AlmcpNotFound_WhenToolsDirHasNoAlmcp() { var toolsDir = Path.Combine(Path.GetTempPath(), $"alcops-noalmcp-analyze-{Guid.NewGuid():N}"); Directory.CreateDirectory(toolsDir); @@ -55,11 +56,70 @@ public async Task ProxyUnavailable_WhenAlmcpNotFound() sc.AddSingleton(proxy); var sp = sc.BuildServiceProvider(); - var json = await AnalyzeTool.Analyze(sp, analyzerResolver, resolver); - var doc = JsonDocument.Parse(json); + var result = await AnalyzeTool.Analyze(sp, analyzerResolver, resolver); + + var root = ToolResultAssert.Error(result, "Unavailable", "AlmcpNotFound"); + Assert.Contains("not found", root.GetProperty("message").GetString()); + } + finally + { + TestAnalyzers.TryDeleteDirectory(toolsDir); + } + } + + [Fact] + public async Task Unavailable_AlmcpNotReady_WhenAlmcpStopped() + { + var toolsDir = CreateStubToolsDirWithAlmcp(); + + try + { + var locator = new BcToolsLocator(toolsDir); + Assert.True(locator.HasAlMcp); + + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + var resolver = DummyResolver(analyzerResolver); + + // Never started; disposing completes Ready with false, as a failed start does. + var proxy = new AlMcpProxy(locator, resolver, new CapturingLogger()); + await proxy.DisposeAsync(); + + var sc = new ServiceCollection(); + sc.AddSingleton(proxy); + + var result = await AnalyzeTool.Analyze(sc.BuildServiceProvider(), analyzerResolver, resolver); + + ToolResultAssert.Error(result, "Unavailable", "AlmcpNotReady"); + } + finally + { + TestAnalyzers.TryDeleteDirectory(toolsDir); + } + } + + [Fact] + public async Task Cancellation_Propagates_InsteadOfFaulted() + { + var toolsDir = CreateStubToolsDirWithAlmcp(); + + try + { + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + var resolver = DummyResolver(analyzerResolver); + + // Never started and not disposed: Ready stays pending, so the cancelled wait throws. + var proxy = new AlMcpProxy(new BcToolsLocator(toolsDir), resolver, new CapturingLogger()); + + var sc = new ServiceCollection(); + sc.AddSingleton(proxy); + + using var cts = new CancellationTokenSource(); + await cts.CancelAsync(); - Assert.Equal("ProxyUnavailable", doc.RootElement.GetProperty("error").GetString()); - Assert.Contains("not available", doc.RootElement.GetProperty("message").GetString()); + await Assert.ThrowsAnyAsync(() => + AnalyzeTool.Analyze(sc.BuildServiceProvider(), analyzerResolver, resolver, cancellationToken: cts.Token)); + + await proxy.DisposeAsync(); } finally { @@ -67,6 +127,45 @@ public async Task ProxyUnavailable_WhenAlmcpNotFound() } } + [Fact] + public void MapProxyFailure_IsUnavailableAlmcpCallFailed_WithAlmcpTextAsDetail() + { + var failed = new CallToolResult + { + IsError = true, + Content = [new TextContentBlock { Text = "Session not found" }] + }; + + var result = AnalyzeTool.MapProxyFailure(failed); + + var root = ToolResultAssert.Error(result, "Unavailable", "AlmcpCallFailed"); + Assert.Equal("Session not found", root.GetProperty("detail").GetString()); + } + + [Fact] + public async Task Invalid_WhenLimitIsNotPositive() + { + // The limit check runs before the proxy checks, so this needs no almcp. + var services = new ServiceCollection().BuildServiceProvider(); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + + var result = await AnalyzeTool.Analyze(services, analyzerResolver, DummyResolver(analyzerResolver), limit: 0); + + var root = ToolResultAssert.Error(result, "Invalid"); + Assert.Contains("limit", root.GetProperty("message").GetString()); + } + + /// A tools directory that BcToolsLocator accepts as having almcp; nothing in it can run. + private static string CreateStubToolsDirWithAlmcp() + { + var toolsDir = Path.Combine(Path.GetTempPath(), $"alcops-stubalmcp-analyze-{Guid.NewGuid():N}"); + Directory.CreateDirectory(toolsDir); + File.WriteAllText(Path.Combine(toolsDir, "Microsoft.Dynamics.Nav.CodeAnalysis.dll"), "stub"); + File.WriteAllText(Path.Combine(toolsDir, "almcp.exe"), "stub"); + File.WriteAllText(Path.Combine(toolsDir, "almcp.dll"), "stub"); + return toolsDir; + } + [Fact] public void Schema_ExposesUserParameters_HidesServicesAndCancellationToken() { @@ -104,20 +203,16 @@ private async Task RunAnalyze( string[]? ruleIds = null, int limit = AnalyzeTool.DefaultLimit) { - var json = await AnalyzeTool.Analyze( + var result = await AnalyzeTool.Analyze( fixture.ServiceProvider, fixture.AnalyzerResolver, fixture.WorkspaceResolver, filePath, folderPath, projectPath, severities, analyzers, ruleIds, limit, Cts.Token); - output.WriteLine(json); - - var doc = JsonDocument.Parse(json); - Assert.False(doc.RootElement.TryGetProperty("error", out var err), - $"Unexpected error: {err}"); + output.WriteLine(ToolResultAssert.Text(result)); - return JsonSerializer.Deserialize(json, JsonDefaults.Options)!; + return ToolResultAssert.OkAs(result); } [AlMcpFact] @@ -265,35 +360,20 @@ public async Task FolderPathProjB_NoProjectPath_ReturnsProjBDiagnostics() } [AlMcpFact] - public async Task InvalidLimit_ReturnsError() - { - var json = await AnalyzeTool.Analyze( - fixture.ServiceProvider, - fixture.AnalyzerResolver, - fixture.WorkspaceResolver, - limit: 0, - cancellationToken: Cts.Token); - - var doc = JsonDocument.Parse(json); - Assert.Equal("InvalidLimit", doc.RootElement.GetProperty("error").GetString()); - } - - [AlMcpFact] - public async Task UnknownProject_ReturnsError() + public async Task UnknownProject_ReturnsInvalid() { var bogus = Path.Combine(Path.GetDirectoryName(fixture.ProjA)!, "NonExistent"); - var json = await AnalyzeTool.Analyze( + var result = await AnalyzeTool.Analyze( fixture.ServiceProvider, fixture.AnalyzerResolver, fixture.WorkspaceResolver, projectPath: bogus, cancellationToken: Cts.Token); - output.WriteLine(json); - var doc = JsonDocument.Parse(json); - Assert.Equal("UnknownProject", doc.RootElement.GetProperty("error").GetString()); + output.WriteLine(ToolResultAssert.Text(result)); + var root = ToolResultAssert.Error(result, "Invalid"); - var message = doc.RootElement.GetProperty("message").GetString()!; + var message = root.GetProperty("message").GetString()!; Assert.Contains(fixture.ProjA, message, StringComparison.OrdinalIgnoreCase); Assert.Contains(fixture.ProjB, message, StringComparison.OrdinalIgnoreCase); } diff --git a/tests/ALCops.Mcp.Tests/ApplyFixAllToolTests.cs b/tests/ALCops.Mcp.Tests/ApplyFixAllToolTests.cs index c2b92e7..0946433 100644 --- a/tests/ALCops.Mcp.Tests/ApplyFixAllToolTests.cs +++ b/tests/ALCops.Mcp.Tests/ApplyFixAllToolTests.cs @@ -47,14 +47,13 @@ public async Task ApplyFixAll_ProjectScope_FixesAllOccurrencesAcrossFiles() { using var ctx = new TestContext(); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020"); - using var doc = JsonDocument.Parse(resultJson); - var root = doc.RootElement; + var root = ToolResultAssert.Ok(result); - Assert.True(root.GetProperty("applied").GetBoolean(), resultJson); + Assert.True(root.GetProperty("applied").GetBoolean(), ToolResultAssert.Text(result)); Assert.Equal(2, root.GetProperty("diagnosticsFound").GetInt32()); Assert.Empty(root.GetProperty("conflicts").EnumerateArray()); @@ -90,14 +89,13 @@ public async Task ApplyFixAll_DocumentScope_OnlyFixesTargetFile() using var ctx = new TestContext(); var pageAPath = Path.Combine(ctx.ProjectPath, "PageA.al"); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020", scope: "document", filePath: pageAPath); - using var doc = JsonDocument.Parse(resultJson); - var root = doc.RootElement; + var root = ToolResultAssert.Ok(result); - Assert.True(root.GetProperty("applied").GetBoolean(), resultJson); + Assert.True(root.GetProperty("applied").GetBoolean(), ToolResultAssert.Text(result)); Assert.Equal(1, root.GetProperty("diagnosticsFound").GetInt32()); var filesChanged = root.GetProperty("filesChanged").EnumerateArray() @@ -116,14 +114,13 @@ public async Task ApplyFixAll_DryRun_ReportsChangesWithoutWriting() var originalPageA = ctx.ReadFile("PageA.al"); var originalPageB = ctx.ReadFile("PageB.al"); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020", dryRun: true); - using var doc = JsonDocument.Parse(resultJson); - var root = doc.RootElement; + var root = ToolResultAssert.Ok(result); - Assert.False(root.GetProperty("applied").GetBoolean(), resultJson); + Assert.False(root.GetProperty("applied").GetBoolean(), ToolResultAssert.Text(result)); Assert.True(root.GetProperty("dryRun").GetBoolean()); Assert.Equal(2, root.GetProperty("diagnosticsFound").GetInt32()); Assert.Equal(2, root.GetProperty("filesChanged").GetArrayLength()); @@ -138,35 +135,96 @@ public async Task ApplyFixAll_MultipleDistinctFixesWithoutEquivalenceKey_Returns { using var ctx = new TestContext(); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "AC0012"); - using var doc = JsonDocument.Parse(resultJson); - var root = doc.RootElement; + var root = ToolResultAssert.Error(result, "Ambiguous"); + Assert.False(root.TryGetProperty("reason", out _)); + Assert.Equal("AC0012", root.GetProperty("diagnosticId").GetString()); - Assert.Equal("AmbiguousFix", root.GetProperty("error").GetString()); - var keys = root.GetProperty("availableEquivalenceKeys").EnumerateArray().Select(e => e.GetString()).ToArray(); - Assert.True(keys.Length > 1, resultJson); + var candidates = root.GetProperty("candidates").EnumerateArray().ToArray(); + Assert.True(candidates.Length > 1, ToolResultAssert.Text(result)); + Assert.All(candidates, c => + { + Assert.Equal(JsonValueKind.String, c.GetProperty("equivalenceKey").ValueKind); + Assert.False(string.IsNullOrEmpty(c.GetProperty("title").GetString())); + Assert.False(string.IsNullOrEmpty(c.GetProperty("providerName").GetString())); + }); + + var keys = candidates.Select(c => c.GetProperty("equivalenceKey").GetString()).ToArray(); + Assert.Equal(keys.Length, keys.Distinct(StringComparer.Ordinal).Count()); // The codeunit must be untouched since no fix was chosen or applied. Assert.Contains("Access = Internal;", ctx.ReadFile("Codeunit.al")); } + [Fact] + public async Task ApplyFixAll_UnknownEquivalenceKey_ReturnsNotFoundWithCandidates() + { + using var ctx = new TestContext(); + + var result = await ApplyFixAllTool.ApplyFixAll( + ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, + ctx.ProjectPath, "AC0012", equivalenceKey: "no-such-key"); + + var root = ToolResultAssert.Error(result, "NotFound", "NoFixForEquivalenceKey"); + Assert.True(root.GetProperty("candidates").GetArrayLength() > 1, ToolResultAssert.Text(result)); + Assert.False(root.TryGetProperty("diagnosticsFound", out _)); + Assert.Contains("Access = Internal;", ctx.ReadFile("Codeunit.al")); + } + + [Fact] + public async Task ApplyFixAll_RuleWithoutFixProvider_ReturnsNotFoundNoFixProvider() + { + using var ctx = new TestContext(); + + var result = await ApplyFixAllTool.ApplyFixAll( + ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, + ctx.ProjectPath, "ZZ9999"); + + ToolResultAssert.Error(result, "NotFound", "NoFixProvider"); + } + + [Fact] + public async Task ApplyFixAll_UnknownScope_ReturnsInvalid() + { + using var ctx = new TestContext(); + + var result = await ApplyFixAllTool.ApplyFixAll( + ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, + ctx.ProjectPath, "LC0020", scope: "workspace"); + + var root = ToolResultAssert.Error(result, "Invalid"); + Assert.Contains("workspace", root.GetProperty("message").GetString()); + } + + [Fact] + public async Task ApplyFixAll_DocumentScopeWithoutFilePath_ReturnsInvalid() + { + using var ctx = new TestContext(); + + var result = await ApplyFixAllTool.ApplyFixAll( + ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, + ctx.ProjectPath, "LC0020", scope: "document"); + + var root = ToolResultAssert.Error(result, "Invalid"); + Assert.Contains("filePath", root.GetProperty("message").GetString()); + } + [Fact] public async Task ApplyFixAll_NoDiagnosticsFound_ReturnsNotAppliedWithZeroCount() { using var ctx = new TestContext(); var pageCPath = Path.Combine(ctx.ProjectPath, "PageC.al"); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020", scope: "document", filePath: pageCPath); - using var doc = JsonDocument.Parse(resultJson); - var root = doc.RootElement; + var root = ToolResultAssert.Ok(result); - Assert.False(root.GetProperty("applied").GetBoolean(), resultJson); + Assert.False(root.GetProperty("applied").GetBoolean(), ToolResultAssert.Text(result)); Assert.Equal(0, root.GetProperty("diagnosticsFound").GetInt32()); } @@ -187,13 +245,12 @@ await ApplyFixAllTool.ApplyFixAll( await File.WriteAllTextAsync(pageBPath, pageBContent.Replace("OtherField", "RenamedField")); // Apply for real — the refresh must pick up the rename. - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020"); - using var doc = JsonDocument.Parse(resultJson); - var root = doc.RootElement; - Assert.True(root.GetProperty("applied").GetBoolean(), resultJson); + var root = ToolResultAssert.Ok(result); + Assert.True(root.GetProperty("applied").GetBoolean(), ToolResultAssert.Text(result)); // PageB must still contain the externally-renamed field AND have exactly one ApplicationArea. var pageBFinal = ctx.ReadFile("PageB.al"); @@ -217,13 +274,12 @@ await ApplyFixAllTool.ApplyFixAll( .Replace("PageB", "PageD"); await File.WriteAllTextAsync(Path.Combine(ctx.ProjectPath, "PageD.al"), pageDContent); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020"); - using var doc = JsonDocument.Parse(resultJson); - var root = doc.RootElement; - Assert.True(root.GetProperty("applied").GetBoolean(), resultJson); + var root = ToolResultAssert.Ok(result); + Assert.True(root.GetProperty("applied").GetBoolean(), ToolResultAssert.Text(result)); Assert.Equal(3, root.GetProperty("diagnosticsFound").GetInt32()); var filesChanged = root.GetProperty("filesChanged").EnumerateArray() @@ -244,13 +300,12 @@ await ApplyFixAllTool.ApplyFixAll( // Delete PageB.al — only PageA still has LC0020. File.Delete(Path.Combine(ctx.ProjectPath, "PageB.al")); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020"); - using var doc = JsonDocument.Parse(resultJson); - var root = doc.RootElement; - Assert.True(root.GetProperty("applied").GetBoolean(), resultJson); + var root = ToolResultAssert.Ok(result); + Assert.True(root.GetProperty("applied").GetBoolean(), ToolResultAssert.Text(result)); Assert.Equal(1, root.GetProperty("diagnosticsFound").GetInt32()); var filesChanged = root.GetProperty("filesChanged").EnumerateArray() @@ -260,7 +315,7 @@ await ApplyFixAllTool.ApplyFixAll( } [Fact] - public async Task ApplyFixAll_RulesetSuppressesRule_TreatsItAsZeroDiagnostics() + public async Task ApplyFixAll_RulesetSuppressesRule_ReturnsNotFoundSuppressedByRuleset() { // FixAllRulesetProject ships a custom.ruleset.json setting LC0020 to "None", // even though PageA.al contains a redundant ApplicationArea occurrence. @@ -268,15 +323,12 @@ public async Task ApplyFixAll_RulesetSuppressesRule_TreatsItAsZeroDiagnostics() var pageAPath = Path.Combine(ctx.ProjectPath, "PageA.al"); var originalPageA = ctx.ReadFile("PageA.al"); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020", scope: "document", filePath: pageAPath); - using var doc = JsonDocument.Parse(resultJson); - var root = doc.RootElement; - - Assert.False(root.GetProperty("applied").GetBoolean(), resultJson); - Assert.Equal(0, root.GetProperty("diagnosticsFound").GetInt32()); + var root = ToolResultAssert.Error(result, "NotFound", "SuppressedByRuleset"); + Assert.Equal("LC0020", root.GetProperty("diagnosticId").GetString()); Assert.Equal(originalPageA, ctx.ReadFile("PageA.al")); } @@ -311,13 +363,13 @@ public async Task ApplyFixAll_MixedEncodings_EachFileKeepsItsOwn() var pageBPath = Path.Combine(ctx.ProjectPath, "PageB.al"); await File.WriteAllBytesAsync(pageBPath, [0xEF, 0xBB, 0xBF, .. await File.ReadAllBytesAsync(pageBPath)]); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020"); - using var doc = JsonDocument.Parse(resultJson); - Assert.True(doc.RootElement.GetProperty("applied").GetBoolean(), resultJson); - Assert.Equal(2, doc.RootElement.GetProperty("filesChanged").GetArrayLength()); + var root = ToolResultAssert.Ok(result); + Assert.True(root.GetProperty("applied").GetBoolean(), ToolResultAssert.Text(result)); + Assert.Equal(2, root.GetProperty("filesChanged").GetArrayLength()); var pageB = await File.ReadAllBytesAsync(pageBPath); Assert.Equal([0xEF, 0xBB, 0xBF], pageB.Take(3)); @@ -344,14 +396,13 @@ public async Task ApplyFixAll_SecondCommitFails_RollsBackAndReportsAllThree() ctx.Writer = GuardedFileWriterTests.FailingOnCalls(2); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020"); - using var doc = JsonDocument.Parse(resultJson); - var root = doc.RootElement; + var root = ToolResultAssert.Ok(result); - Assert.False(root.GetProperty("applied").GetBoolean(), resultJson); + Assert.False(root.GetProperty("applied").GetBoolean(), ToolResultAssert.Text(result)); Assert.Equal(3, root.GetProperty("diagnosticsFound").GetInt32()); Assert.Empty(root.GetProperty("filesChanged").EnumerateArray()); @@ -385,12 +436,12 @@ public async Task ApplyFixAll_NoTempFilesLeftBehind() { using var ctx = new TestContext(); - var resultJson = await ApplyFixAllTool.ApplyFixAll( + var result = await ApplyFixAllTool.ApplyFixAll( ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, ctx.ProjectPath, "LC0020"); - using var doc = JsonDocument.Parse(resultJson); - Assert.True(doc.RootElement.GetProperty("applied").GetBoolean(), resultJson); + var root = ToolResultAssert.Ok(result); + Assert.True(root.GetProperty("applied").GetBoolean(), ToolResultAssert.Text(result)); Assert.Empty(TempFiles(ctx.ProjectPath)); } diff --git a/tests/ALCops.Mcp.Tests/ApplyFixThenCompileTests.cs b/tests/ALCops.Mcp.Tests/ApplyFixThenCompileTests.cs index d56af1a..6f7e644 100644 --- a/tests/ALCops.Mcp.Tests/ApplyFixThenCompileTests.cs +++ b/tests/ALCops.Mcp.Tests/ApplyFixThenCompileTests.cs @@ -57,15 +57,15 @@ public async Task ApplyFix_ThenAlCompile_NoLongerReportsFixedDiagnostic() var analyzerSet = await analyzerResolver.ResolveAsync(projectDir, null, Cts.Token); const int line = 11, column = 17; - var fixes = await codeFixRunner.GetFixesAsync(session, filePath, "LC0020", line, column, analyzerSet, Cts.Token); - Assert.True(fixes.Count > 0, "Expected a fixable LC0020 at line 11, column 17."); + var lookup = await codeFixRunner.GetFixesAsync(session, filePath, "LC0020", line, column, analyzerSet, Cts.Token); + Assert.True(lookup.Fixes.Count > 0, "Expected a fixable LC0020 at line 11, column 17."); var applyResult = await ApplyFixTool.ApplyFix( sessionManager, codeFixRunner, analyzerResolver, new GuardedFileWriter(), projectDir, filePath, "LC0020", line, column, - fixes[0].EquivalenceKey, analyzers: null, Cts.Token); + lookup.Fixes[0].EquivalenceKey, analyzers: null, Cts.Token); - Assert.Contains("\"applied\":true", applyResult); + Assert.True(ToolResultAssert.Ok(applyResult).GetProperty("applied").GetBoolean()); // 3. Compile again — LC0020 must be gone fixture.Logger.Lines.Clear(); diff --git a/tests/ALCops.Mcp.Tests/ApplyFixToolTests.cs b/tests/ALCops.Mcp.Tests/ApplyFixToolTests.cs index 30934b6..296de97 100644 --- a/tests/ALCops.Mcp.Tests/ApplyFixToolTests.cs +++ b/tests/ALCops.Mcp.Tests/ApplyFixToolTests.cs @@ -1,3 +1,4 @@ +using ALCops.Mcp.Models; using ALCops.Mcp.Services; using ALCops.Mcp.Tools; using Xunit; @@ -31,22 +32,22 @@ public async Task ApplyFix_WritesModifiedContentToDisk() var analyzerSet = await analyzerResolver.ResolveAsync(tempProjectPath, null); // LC0020 (ApplicationAreaRedundancy) on the field-level ApplicationArea in MyPage.al. - // Hardcoded rather than discovered, matching GetFixes_RulesetSuppressesRule_ReturnsNoFixes. + // Hardcoded rather than discovered, matching GetFixes_RulesetSuppressesRule_ReturnsSuppressedByRuleset. const int line = 11, column = 17; - var fixes = await codeFixRunner.GetFixesAsync( + var lookup = await codeFixRunner.GetFixesAsync( session, filePath, "LC0020", line, column, analyzerSet); - Assert.True(fixes.Count > 0, - $"Expected a fixable LC0020 at line {line}, column {column}. Loaded {analyzerSet.GetAllAnalyzers().Length} " + + Assert.True(lookup.NotFoundReason is null && lookup.Fixes.Count > 0, + $"Expected a fixable LC0020 at line {line}, column {column}, got {lookup.NotFoundReason}. Loaded {analyzerSet.GetAllAnalyzers().Length} " + $"analyzer(s); warnings: {string.Join("; ", analyzerSet.Warnings)}. " + "The fixture, the location, or the analyzer configuration may have changed."); var result = await ApplyFixTool.ApplyFix( sessionManager, codeFixRunner, analyzerResolver, new GuardedFileWriter(), tempProjectPath, filePath, "LC0020", line, column, - fixes[0].EquivalenceKey); + lookup.Fixes[0].EquivalenceKey); - Assert.Contains("\"applied\":true", result); + Assert.True(ToolResultAssert.Ok(result).GetProperty("applied").GetBoolean()); // The actual regression assertion: the file on disk must have changed. var updatedContent = await File.ReadAllTextAsync(filePath); @@ -77,9 +78,9 @@ public async Task ApplyFix_ExternalEditBetweenCalls_PreservesEditAndAppliesFix() var analyzerSet = await analyzerResolver.ResolveAsync(tempProjectPath, null); const int line = 11, column = 17; - var fixes = await codeFixRunner.GetFixesAsync( + var lookup = await codeFixRunner.GetFixesAsync( session, filePath, "LC0020", line, column, analyzerSet); - Assert.True(fixes.Count > 0, "Expected a fixable LC0020."); + Assert.True(lookup.Fixes.Count > 0, "Expected a fixable LC0020."); // External edit: append a comment as the final line. Line 11 stays valid. var content = await File.ReadAllTextAsync(filePath); @@ -90,9 +91,9 @@ public async Task ApplyFix_ExternalEditBetweenCalls_PreservesEditAndAppliesFix() var result = await ApplyFixTool.ApplyFix( sessionManager, codeFixRunner, analyzerResolver, new GuardedFileWriter(), tempProjectPath, filePath, "LC0020", line, column, - fixes[0].EquivalenceKey); + lookup.Fixes[0].EquivalenceKey); - Assert.Contains("\"applied\":true", result); + Assert.True(ToolResultAssert.Ok(result).GetProperty("applied").GetBoolean()); var finalContent = await File.ReadAllTextAsync(filePath); Assert.Contains("// edited", finalContent); @@ -130,7 +131,7 @@ public async Task ApplyFix_FileWithUtf8Bom_KeepsBom() var (result, _) = await ApplyLc0020Async(tempProjectPath, filePath, new GuardedFileWriter()); - Assert.Contains("\"applied\":true", result); + Assert.True(ToolResultAssert.Ok(result).GetProperty("applied").GetBoolean()); var written = await File.ReadAllBytesAsync(filePath); Assert.Equal([0xEF, 0xBB, 0xBF], written.Take(3)); @@ -145,7 +146,7 @@ public async Task ApplyFix_FileWithUtf8Bom_KeepsBom() } [Fact] - public async Task ApplyFix_WriteFails_ReturnsIOExceptionAndLeavesFileIntact() + public async Task ApplyFix_WriteFails_ReturnsFaultedWriteFailedAndLeavesFileIntact() { var tempProjectPath = TestAnalyzers.CopyFixtureWithAnalyzers("ApplyFixProject", "alcops-applyfix-ioerr-test"); @@ -157,8 +158,11 @@ public async Task ApplyFix_WriteFails_ReturnsIOExceptionAndLeavesFileIntact() var writer = new GuardedFileWriter((_, _) => throw new IOException("injected move failure")); var (result, _) = await ApplyLc0020Async(tempProjectPath, filePath, writer); - Assert.Contains("\"error\":\"IOException\"", result); - Assert.Contains("injected move failure", result); + var root = ToolResultAssert.Error(result, "Faulted", "WriteFailed"); + Assert.Equal("System.IO.IOException", root.GetProperty("detail").GetString()); + Assert.Contains("injected move failure", root.GetProperty("message").GetString()); + Assert.Equal(filePath, root.GetProperty("filePath").GetString()); + Assert.Equal("LC0020", root.GetProperty("diagnosticId").GetString()); Assert.Equal(originalBytes, await File.ReadAllBytesAsync(filePath)); Assert.Empty(Directory.GetFiles(tempProjectPath, "*" + GuardedFileWriter.TempSuffix, SearchOption.AllDirectories)); } @@ -169,7 +173,7 @@ public async Task ApplyFix_WriteFails_ReturnsIOExceptionAndLeavesFileIntact() } [Fact] - public async Task ApplyFix_FileNotValidUtf8_ReturnsUnsupportedEncodingAndLeavesFileIntact() + public async Task ApplyFix_FileNotValidUtf8_ReturnsFaultedUnsupportedEncodingAndLeavesFileIntact() { var tempProjectPath = TestAnalyzers.CopyFixtureWithAnalyzers("ApplyFixProject", "alcops-applyfix-cp1252-test"); @@ -184,8 +188,8 @@ public async Task ApplyFix_FileNotValidUtf8_ReturnsUnsupportedEncodingAndLeavesF var (result, _) = await ApplyLc0020Async(tempProjectPath, filePath, new GuardedFileWriter()); - Assert.Contains("\"error\":\"UnsupportedEncoding\"", result); - Assert.Contains("not valid in its detected encoding (UTF-8;", result); + var root = ToolResultAssert.Error(result, "Faulted", "UnsupportedEncoding"); + Assert.Contains("not valid in its detected encoding (UTF-8;", root.GetProperty("message").GetString()); Assert.Equal(originalBytes, await File.ReadAllBytesAsync(filePath)); Assert.Empty(Directory.GetFiles(tempProjectPath, "*" + GuardedFileWriter.TempSuffix, SearchOption.AllDirectories)); } @@ -195,8 +199,135 @@ public async Task ApplyFix_FileNotValidUtf8_ReturnsUnsupportedEncodingAndLeavesF } } + [Fact] + public async Task ApplyFix_FileChangedButRefreshSkipsIt_ReturnsStaleAndWritesNothing() + { + var tempProjectPath = TestAnalyzers.CopyFixtureWithAnalyzers("ApplyFixProject", "alcops-applyfix-stale-test"); + + try + { + var filePath = Path.Combine(tempProjectPath, "MyPage.al"); + + using var sessionManager = new ProjectSessionManager(new ProjectLoader()); + var codeFixRunner = new CodeFixRunner(); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + + // Prime the session so the cached text is the original. + var session = await sessionManager.GetOrLoadProjectAsync(tempProjectPath); + var analyzerSet = await analyzerResolver.ResolveAsync(tempProjectPath, null); + var lookup = await codeFixRunner.GetFixesAsync(session, filePath, "LC0020", 11, 17, analyzerSet); + Assert.True(lookup.Fixes.Count > 0, "Expected a fixable LC0020 at line 11, column 17."); + + // A same-length edit with the old timestamp: the refresh's length+mtime gate skips the + // file, so the fix is computed from the cached text and the guarded write must refuse it. + var stamp = File.GetLastWriteTimeUtc(filePath); + var content = await File.ReadAllTextAsync(filePath); + Assert.Contains("50100", content); + await File.WriteAllTextAsync(filePath, content.Replace("50100", "50109")); + File.SetLastWriteTimeUtc(filePath, stamp); + var editedBytes = await File.ReadAllBytesAsync(filePath); + + var result = await ApplyFixTool.ApplyFix( + sessionManager, codeFixRunner, analyzerResolver, new GuardedFileWriter(), + tempProjectPath, filePath, "LC0020", 11, 17, lookup.Fixes[0].EquivalenceKey); + + var root = ToolResultAssert.Error(result, "Stale"); + Assert.Equal(filePath, root.GetProperty("filePath").GetString()); + Assert.Equal("LC0020", root.GetProperty("diagnosticId").GetString()); + Assert.False(root.TryGetProperty("reason", out _)); + Assert.Equal(editedBytes, await File.ReadAllBytesAsync(filePath)); + Assert.Empty(Directory.GetFiles(tempProjectPath, "*" + GuardedFileWriter.TempSuffix, SearchOption.AllDirectories)); + } + finally + { + TestAnalyzers.TryDeleteDirectory(tempProjectPath); + } + } + + [Fact] + public async Task ApplyFix_UnknownEquivalenceKey_ReturnsNotFoundWithCandidates() + { + var tempProjectPath = TestAnalyzers.CopyFixtureWithAnalyzers("ApplyFixProject", "alcops-applyfix-badkey-test"); + + try + { + var filePath = Path.Combine(tempProjectPath, "MyPage.al"); + var originalBytes = await File.ReadAllBytesAsync(filePath); + + using var sessionManager = new ProjectSessionManager(new ProjectLoader()); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + + var result = await ApplyFixTool.ApplyFix( + sessionManager, new CodeFixRunner(), analyzerResolver, new GuardedFileWriter(), + tempProjectPath, filePath, "LC0020", 11, 17, "no-such-key"); + + var root = ToolResultAssert.Error(result, "NotFound", "NoFixForEquivalenceKey"); + var candidates = root.GetProperty("candidates").EnumerateArray().ToList(); + Assert.NotEmpty(candidates); + Assert.All(candidates, c => + { + Assert.True(c.TryGetProperty("equivalenceKey", out _)); + Assert.False(string.IsNullOrEmpty(c.GetProperty("title").GetString())); + Assert.False(string.IsNullOrEmpty(c.GetProperty("providerName").GetString())); + }); + Assert.Contains("no-such-key", root.GetProperty("message").GetString()); + Assert.Equal(originalBytes, await File.ReadAllBytesAsync(filePath)); + } + finally + { + TestAnalyzers.TryDeleteDirectory(tempProjectPath); + } + } + + [Fact] + public async Task ApplyFix_RuleWithoutFixProvider_ReturnsNotFoundNoFixProvider() + { + var tempProjectPath = TestAnalyzers.CopyFixtureWithAnalyzers("ApplyFixProject", "alcops-applyfix-noprovider-test"); + + try + { + using var sessionManager = new ProjectSessionManager(new ProjectLoader()); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + + var result = await ApplyFixTool.ApplyFix( + sessionManager, new CodeFixRunner(), analyzerResolver, new GuardedFileWriter(), + tempProjectPath, Path.Combine(tempProjectPath, "MyPage.al"), "ZZ9999", 11, 17, "any"); + + var root = ToolResultAssert.Error(result, "NotFound", "NoFixProvider"); + Assert.False(root.TryGetProperty("candidates", out _)); + } + finally + { + TestAnalyzers.TryDeleteDirectory(tempProjectPath); + } + } + + [Fact] + public async Task ApplyFix_FolderWithoutAppJson_ReturnsInvalid() + { + var folder = Path.Combine(Path.GetTempPath(), $"alcops-applyfix-noappjson-{Guid.NewGuid():N}"); + Directory.CreateDirectory(folder); + + try + { + using var sessionManager = new ProjectSessionManager(new ProjectLoader()); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + + var result = await ApplyFixTool.ApplyFix( + sessionManager, new CodeFixRunner(), analyzerResolver, new GuardedFileWriter(), + folder, Path.Combine(folder, "MyPage.al"), "LC0020", 11, 17, "any"); + + var root = ToolResultAssert.Error(result, "Invalid"); + Assert.Contains("app.json", root.GetProperty("message").GetString()); + } + finally + { + TestAnalyzers.TryDeleteDirectory(folder); + } + } + /// Loads the project, finds the LC0020 fix at MyPage.al:11:17 and applies it through the tool. - private static async Task<(string Result, string EquivalenceKey)> ApplyLc0020Async( + private static async Task<(ModelContextProtocol.Protocol.CallToolResult Result, string EquivalenceKey)> ApplyLc0020Async( string projectPath, string filePath, GuardedFileWriter writer) { using var sessionManager = new ProjectSessionManager(new ProjectLoader()); @@ -207,23 +338,23 @@ public async Task ApplyFix_FileNotValidUtf8_ReturnsUnsupportedEncodingAndLeavesF var analyzerSet = await analyzerResolver.ResolveAsync(projectPath, null); const int line = 11, column = 17; - var fixes = await codeFixRunner.GetFixesAsync(session, filePath, "LC0020", line, column, analyzerSet); - Assert.True(fixes.Count > 0, "Expected a fixable LC0020 at line 11, column 17."); + var lookup = await codeFixRunner.GetFixesAsync(session, filePath, "LC0020", line, column, analyzerSet); + Assert.True(lookup.Fixes.Count > 0, "Expected a fixable LC0020 at line 11, column 17."); var result = await ApplyFixTool.ApplyFix( sessionManager, codeFixRunner, analyzerResolver, writer, projectPath, filePath, "LC0020", line, column, - fixes[0].EquivalenceKey); + lookup.Fixes[0].EquivalenceKey); - return (result, fixes[0].EquivalenceKey); + return (result, lookup.Fixes[0].EquivalenceKey); } [Fact] - public async Task GetFixes_RulesetSuppressesRule_ReturnsNoFixes() + public async Task GetFixes_RulesetSuppressesRule_ReturnsSuppressedByRuleset() { // FixAllRulesetProject ships a custom.ruleset.json setting LC0020 to "None". Even though - // PageA.al still has a redundant ApplicationArea, get_fixes must not offer a fix for it - // (CodeFixRunner.FindDiagnosticAsync must honor ruleset suppression). + // PageA.al still has a redundant ApplicationArea, get_fixes must not offer a fix for it, + // and must say the ruleset is why. var tempProjectPath = TestAnalyzers.CopyFixtureWithAnalyzers("FixAllRulesetProject", "alcops-ruleset-getfixes-test"); try @@ -241,18 +372,19 @@ public async Task GetFixes_RulesetSuppressesRule_ReturnsNoFixes() // (not a location mismatch) is what suppresses the result below. const int line = 11, column = 17; var withoutRuleset = TestAnalyzers.LoadAnalyzersWithoutRuleset(loader, tempProjectPath); - var controlFixes = await codeFixRunner.GetFixesAsync( + var control = await codeFixRunner.GetFixesAsync( session, filePath, "LC0020", line, column, withoutRuleset); - Assert.True(controlFixes.Count > 0, + Assert.True(control.NotFoundReason is null && control.Fixes.Count > 0, "Expected a fixable LC0020 at line 11, column 17 with no ruleset applied — fixture or location may have changed."); // With the resolved AnalyzerSet (loads FixAllRulesetProject's custom.ruleset.json, // which sets LC0020 to "None"), the same location must yield no fixes. var analyzerSet = await analyzerResolver.ResolveAsync(tempProjectPath, null); - var fixes = await codeFixRunner.GetFixesAsync( + var lookup = await codeFixRunner.GetFixesAsync( session, filePath, "LC0020", line, column, analyzerSet); - Assert.Empty(fixes); + Assert.Equal(FixNotFoundReason.SuppressedByRuleset, lookup.NotFoundReason); + Assert.Empty(lookup.Fixes); } finally { diff --git a/tests/ALCops.Mcp.Tests/FixRoundTripTests.cs b/tests/ALCops.Mcp.Tests/FixRoundTripTests.cs new file mode 100644 index 0000000..997bd98 --- /dev/null +++ b/tests/ALCops.Mcp.Tests/FixRoundTripTests.cs @@ -0,0 +1,94 @@ +using ALCops.Mcp.Models; +using ALCops.Mcp.Services; +using ALCops.Mcp.Tools; +using Microsoft.Dynamics.Nav.CodeAnalysis.CodeFixes; +using Xunit; + +namespace ALCops.Mcp.Tests; + +/// +/// Every equivalence key the tools advertise (Ambiguous candidates, get_fixes fixes) must be accepted +/// back verbatim by apply_fix and apply_fix_all. AC0012 in FixAllProject has two fix providers. +/// +public sealed class FixRoundTripTests +{ + private const string Rule = "AC0012"; + + [Fact] + public async Task AdvertisedKeys_RoundTrip_ThroughApplyFixAllGetFixesAndApplyFix() + { + var projectPath = TestAnalyzers.CopyFixtureWithAnalyzers("FixAllProject", "alcops-roundtrip-test"); + try + { + using var sessionManager = new ProjectSessionManager(new ProjectLoader()); + var runner = new CodeFixRunner(); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + var writer = new GuardedFileWriter(); + + // 1. No key: Ambiguous, with the candidates. + var ambiguous = await ApplyFixAllTool.ApplyFixAll( + sessionManager, runner, analyzerResolver, writer, projectPath, Rule); + var candidateKeys = ToolResultAssert.Error(ambiguous, "Ambiguous") + .GetProperty("candidates").EnumerateArray() + .Select(c => c.GetProperty("equivalenceKey").GetString()!) + .ToHashSet(StringComparer.Ordinal); + Assert.True(candidateKeys.Count > 1); + + // 2. Every candidate key is accepted by apply_fix_all. + foreach (var key in candidateKeys) + { + var dryRun = await ApplyFixAllTool.ApplyFixAll( + sessionManager, runner, analyzerResolver, writer, projectPath, Rule, + equivalenceKey: key, dryRun: true); + var root = ToolResultAssert.Ok(dryRun); + Assert.Equal(key, root.GetProperty("equivalenceKey").GetString()); + } + + // 3. Locate one occurrence, then get_fixes there advertises the same key set. + var session = await sessionManager.GetOrLoadProjectAsync(projectPath); + var analyzerSet = await analyzerResolver.ResolveAsync(projectPath, null); + var probe = await runner.ApplyFixAllAsync( + session, Rule, FixAllScope.Project, null, candidateKeys.First(), analyzerSet); + Assert.Equal(FixAllStatus.Completed, probe.Status); + var occurrence = probe.Changes[0].Diagnostics[0]; + + var getFixes = await GetFixesTool.GetFixes( + sessionManager, runner, analyzerResolver, + projectPath, occurrence.FilePath, Rule, occurrence.Line, occurrence.Column); + var fixKeys = ToolResultAssert.OkAs(getFixes).Fixes + .Select(f => f.EquivalenceKey) + .ToHashSet(StringComparer.Ordinal); + Assert.Equal(candidateKeys.Order(StringComparer.Ordinal), fixKeys.Order(StringComparer.Ordinal)); + + // 4. Every key is accepted by apply_fix, each on a fresh copy. + var relativeFile = Path.GetRelativePath(projectPath, occurrence.FilePath); + foreach (var key in fixKeys) + await ApplyFixOnFreshCopyAsync(relativeFile, occurrence.Line, occurrence.Column, key); + } + finally + { + TestAnalyzers.TryDeleteDirectory(projectPath); + } + } + + private static async Task ApplyFixOnFreshCopyAsync(string relativeFile, int line, int column, string key) + { + var projectPath = TestAnalyzers.CopyFixtureWithAnalyzers("FixAllProject", "alcops-roundtrip-apply-test"); + try + { + using var sessionManager = new ProjectSessionManager(new ProjectLoader()); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + + var result = await ApplyFixTool.ApplyFix( + sessionManager, new CodeFixRunner(), analyzerResolver, new GuardedFileWriter(), + projectPath, Path.Combine(projectPath, relativeFile), Rule, line, column, key); + + Assert.True(ToolResultAssert.Ok(result).GetProperty("applied").GetBoolean(), + $"apply_fix rejected advertised key '{key}': {ToolResultAssert.Text(result)}"); + } + finally + { + TestAnalyzers.TryDeleteDirectory(projectPath); + } + } +} diff --git a/tests/ALCops.Mcp.Tests/Fixtures/PragmaProject/PageWithPragma.al b/tests/ALCops.Mcp.Tests/Fixtures/PragmaProject/PageWithPragma.al new file mode 100644 index 0000000..b6fddad --- /dev/null +++ b/tests/ALCops.Mcp.Tests/Fixtures/PragmaProject/PageWithPragma.al @@ -0,0 +1,20 @@ +#pragma warning disable LC0020 +page 50102 PageWithPragma +{ + ApplicationArea = All; + + layout + { + area(content) + { + field(MyField; MyField) + { + ApplicationArea = All; + } + } + } + + var + MyField: Text; +} +#pragma warning restore LC0020 diff --git a/tests/ALCops.Mcp.Tests/Fixtures/PragmaProject/PageWithoutPragma.al b/tests/ALCops.Mcp.Tests/Fixtures/PragmaProject/PageWithoutPragma.al new file mode 100644 index 0000000..d331445 --- /dev/null +++ b/tests/ALCops.Mcp.Tests/Fixtures/PragmaProject/PageWithoutPragma.al @@ -0,0 +1,18 @@ +page 50103 PageWithoutPragma +{ + ApplicationArea = All; + + layout + { + area(content) + { + field(MyField; MyField) + { + ApplicationArea = All; + } + } + } + + var + MyField: Text; +} diff --git a/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs b/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs new file mode 100644 index 0000000..9b39296 --- /dev/null +++ b/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs @@ -0,0 +1,221 @@ +using System.Collections.Immutable; +using ALCops.Mcp.Models; +using ALCops.Mcp.Services; +using ALCops.Mcp.Tools; +using Microsoft.Dynamics.Nav.CodeAnalysis.CodeFixes; +using Microsoft.Dynamics.Nav.CodeAnalysis.Diagnostics; +using ModelContextProtocol.Protocol; +using Xunit; + +namespace ALCops.Mcp.Tests; + +/// +/// get_fixes: the success object and every NotFound reason. The reasons that the real +/// analyzer configuration cannot produce (NoAnalyzerForRule, NoFixForDiagnostic) are driven at the +/// level through an decorator. +/// +public sealed class GetFixesToolTests +{ + private static async Task GetFixesAsync( + string projectPath, string fileName, string diagnosticId, int line, int column) + { + using var sessionManager = new ProjectSessionManager(new ProjectLoader()); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + + return await GetFixesTool.GetFixes( + sessionManager, new CodeFixRunner(), analyzerResolver, + projectPath, Path.Combine(projectPath, fileName), diagnosticId, line, column); + } + + private static async Task WithFixtureAsync(string fixtureName, Func> body) + { + var projectPath = TestAnalyzers.CopyFixtureWithAnalyzers(fixtureName, "alcops-getfixes-test"); + try + { + return await body(projectPath); + } + finally + { + TestAnalyzers.TryDeleteDirectory(projectPath); + } + } + + [Fact] + public async Task Success_ReturnsObjectWithPositionAndFixes() + { + await WithFixtureAsync("ApplyFixProject", async projectPath => + { + var result = await GetFixesAsync(projectPath, "MyPage.al", "LC0020", 11, 17); + + var root = ToolResultAssert.Ok(result); + Assert.Equal(System.Text.Json.JsonValueKind.Object, root.ValueKind); + + var fixes = ToolResultAssert.OkAs(result); + Assert.Equal("LC0020", fixes.DiagnosticId); + Assert.Equal(Path.GetFullPath(Path.Combine(projectPath, "MyPage.al")), fixes.FilePath); + Assert.Equal(11, fixes.Line); + Assert.Equal(17, fixes.Column); + Assert.NotEmpty(fixes.Fixes); + Assert.All(fixes.Fixes, f => + { + Assert.NotNull(f.EquivalenceKey); + Assert.False(string.IsNullOrEmpty(f.Title)); + Assert.False(string.IsNullOrEmpty(f.ProviderName)); + }); + return 0; + }); + } + + [Fact] + public async Task NoFixProvider_ForUnknownRule() + { + await WithFixtureAsync("ApplyFixProject", async projectPath => + { + var result = await GetFixesAsync(projectPath, "MyPage.al", "ZZ9999", 11, 17); + + var root = ToolResultAssert.Error(result, "NotFound", "NoFixProvider"); + Assert.Equal("ZZ9999", root.GetProperty("diagnosticId").GetString()); + return 0; + }); + } + + [Fact] + public async Task FileNotInProject_ForUnknownFile() + { + await WithFixtureAsync("ApplyFixProject", async projectPath => + { + var result = await GetFixesAsync(projectPath, "Nope.al", "LC0020", 11, 17); + + ToolResultAssert.Error(result, "NotFound", "FileNotInProject"); + return 0; + }); + } + + [Fact] + public async Task NoDiagnosticAtPosition_WhenRuleIsNotReportedThere() + { + await WithFixtureAsync("ApplyFixProject", async projectPath => + { + var result = await GetFixesAsync(projectPath, "MyPage.al", "LC0020", 1, 1); + + var root = ToolResultAssert.Error(result, "NotFound", "NoDiagnosticAtPosition"); + Assert.Contains("re-run analyze", root.GetProperty("message").GetString()); + Assert.False(root.TryGetProperty("candidates", out _)); + return 0; + }); + } + + [Fact] + public async Task SuppressedByRuleset_WhenRulesetSetsRuleToNone() + { + await WithFixtureAsync("FixAllRulesetProject", async projectPath => + { + var result = await GetFixesAsync(projectPath, "PageA.al", "LC0020", 11, 17); + + ToolResultAssert.Error(result, "NotFound", "SuppressedByRuleset"); + return 0; + }); + } + + [Fact] + public async Task SuppressedByPragma_WhenPragmaDisablesRuleAtPosition() + { + await WithFixtureAsync("PragmaProject", async projectPath => + { + // Control: the same page without the pragma has a fixable LC0020. + var control = await GetFixesAsync(projectPath, "PageWithoutPragma.al", "LC0020", 11, 17); + Assert.NotEmpty(ToolResultAssert.OkAs(control).Fixes); + + // The pragma line shifts the field-level ApplicationArea down by one line. + var result = await GetFixesAsync(projectPath, "PageWithPragma.al", "LC0020", 12, 17); + + ToolResultAssert.Error(result, "NotFound", "SuppressedByPragma"); + return 0; + }); + } + + [Fact] + public async Task Invalid_WhenProjectFolderHasNoAppJson() + { + var folder = Path.Combine(Path.GetTempPath(), $"alcops-getfixes-noappjson-{Guid.NewGuid():N}"); + Directory.CreateDirectory(folder); + try + { + var result = await GetFixesAsync(folder, "MyPage.al", "LC0020", 11, 17); + ToolResultAssert.Error(result, "Invalid"); + + var missing = await GetFixesAsync(Path.Combine(folder, "missing"), "MyPage.al", "LC0020", 11, 17); + ToolResultAssert.Error(missing, "Invalid"); + } + finally + { + TestAnalyzers.TryDeleteDirectory(folder); + } + } + + [Fact] + public async Task NoAnalyzerForRule_WhenFixProviderExistsButNoAnalyzerReportsTheRule() + { + await WithFixtureAsync("ApplyFixProject", async projectPath => + { + using var sessionManager = new ProjectSessionManager(new ProjectLoader()); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + var session = await sessionManager.GetOrLoadProjectAsync(projectPath); + var real = await analyzerResolver.ResolveAsync(projectPath, null); + + var provider = new DecoratedProvider(real, hideAnalyzers: true); + var lookup = await new CodeFixRunner().GetFixesAsync( + session, Path.Combine(projectPath, "MyPage.al"), "LC0020", 11, 17, provider); + + Assert.Equal(FixNotFoundReason.NoAnalyzerForRule, lookup.NotFoundReason); + return 0; + }); + } + + [Fact] + public async Task NoFixForDiagnostic_WhenProviderRegistersNothing() + { + await WithFixtureAsync("ApplyFixProject", async projectPath => + { + using var sessionManager = new ProjectSessionManager(new ProjectLoader()); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + var session = await sessionManager.GetOrLoadProjectAsync(projectPath); + var real = await analyzerResolver.ResolveAsync(projectPath, null); + + var provider = new DecoratedProvider(real, fixProviders: [new SilentFixProvider()]); + var runner = new CodeFixRunner(); + var filePath = Path.Combine(projectPath, "MyPage.al"); + + var lookup = await runner.GetFixesAsync(session, filePath, "LC0020", 11, 17, provider); + Assert.Equal(FixNotFoundReason.NoFixForDiagnostic, lookup.NotFoundReason); + + var apply = await runner.ApplyFixAsync(session, filePath, "LC0020", 11, 17, "", provider); + Assert.Equal(FixNotFoundReason.NoFixForDiagnostic, apply.NotFoundReason); + Assert.Null(apply.Fix); + return 0; + }); + } + + /// Wraps a real provider, optionally hiding its analyzers or replacing its fix providers. + private sealed class DecoratedProvider( + IAnalyzerProvider inner, + bool hideAnalyzers = false, + ImmutableArray? fixProviders = null) : IAnalyzerProvider + { + public ImmutableArray GetAllAnalyzers() => hideAnalyzers ? [] : inner.GetAllAnalyzers(); + public ImmutableArray GetAllCodeFixProviders() => fixProviders ?? inner.GetAllCodeFixProviders(); + public ImmutableArray GetCodeFixProvidersForDiagnostic(string diagnosticId) => + fixProviders ?? inner.GetCodeFixProvidersForDiagnostic(diagnosticId); + public ImmutableDictionary GetAllDescriptors() => inner.GetAllDescriptors(); + public string GetCopName(string diagnosticId) => inner.GetCopName(diagnosticId); + public bool HasCodeFix(string diagnosticId) => inner.HasCodeFix(diagnosticId); + } + + /// Claims LC0020 but registers no code action. + private sealed class SilentFixProvider : CodeFixProvider + { + public override ImmutableArray FixableDiagnosticIds => ["LC0020"]; + + public override Task RegisterCodeFixesAsync(CodeFixContext context) => Task.CompletedTask; + } +} diff --git a/tests/ALCops.Mcp.Tests/ListRulesToolTests.cs b/tests/ALCops.Mcp.Tests/ListRulesToolTests.cs new file mode 100644 index 0000000..63a8697 --- /dev/null +++ b/tests/ALCops.Mcp.Tests/ListRulesToolTests.cs @@ -0,0 +1,111 @@ +using ALCops.Mcp.Services; +using ALCops.Mcp.Tools; +using Microsoft.Extensions.Logging.Abstractions; +using Xunit; + +namespace ALCops.Mcp.Tests; + +/// list_rules: success shapes, project validation shared with analyze (issue #33), and the catch-all. +public sealed class ListRulesToolTests : IDisposable +{ + private readonly string _projectPath = TestAnalyzers.CopyFixtureWithAnalyzers("ApplyFixProject", "alcops-listrules-test"); + + public void Dispose() => TestAnalyzers.TryDeleteDirectory(_projectPath); + + private static (ProjectAnalyzerResolver Analyzers, WorkspaceStartupResolver Workspace) Resolvers(params string[] projects) + { + var (analyzerResolver, loader) = TestAnalyzers.CreateAnalyzerResolver(); + var workspace = new WorkspaceStartupResolver( + analyzerResolver, loader, NullLogger.Instance, projects); + return (analyzerResolver, workspace); + } + + /// A resolver whose only --projects entry does not exist, so it has no projects at all. + private static (ProjectAnalyzerResolver Analyzers, WorkspaceStartupResolver Workspace) NoProjects() => + Resolvers(Path.Combine(Path.GetTempPath(), $"alcops-listrules-missing-{Guid.NewGuid():N}")); + + [Fact] + public async Task Ok_Compact_ListsRulesOfTheStartupProject() + { + var (analyzers, workspace) = Resolvers(_projectPath); + + var result = await ListRulesTool.ListRules(analyzers, workspace); + + var root = ToolResultAssert.Ok(result); + var rules = root.GetProperty("rules").EnumerateArray().ToList(); + Assert.NotEmpty(rules); + Assert.Contains(rules, r => r.GetProperty("id").GetString() == "LC0020"); + Assert.All(rules, r => + { + Assert.True(r.TryGetProperty("title", out _)); + Assert.True(r.TryGetProperty("cop", out _)); + Assert.False(r.TryGetProperty("hasCodeFix", out _)); + }); + } + + [Fact] + public async Task Ok_Verbose_IncludesFullMetadata() + { + var (analyzers, workspace) = Resolvers(_projectPath); + + var result = await ListRulesTool.ListRules(analyzers, workspace, projectPath: _projectPath, verbose: true); + + var root = ToolResultAssert.Ok(result); + var lc0020 = root.GetProperty("rules").EnumerateArray().Single(r => r.GetProperty("id").GetString() == "LC0020"); + Assert.True(lc0020.GetProperty("hasCodeFix").GetBoolean()); + Assert.True(lc0020.TryGetProperty("severity", out _)); + Assert.True(lc0020.TryGetProperty("copName", out _)); + } + + [Fact] + public async Task Invalid_WhenExplicitProjectIsNotAStartupProject() + { + var (analyzers, workspace) = Resolvers(_projectPath); + var other = Path.Combine(Path.GetTempPath(), "SomeOtherProject"); + + var result = await ListRulesTool.ListRules(analyzers, workspace, projectPath: other); + + var root = ToolResultAssert.Error(result, "Invalid"); + var message = root.GetProperty("message").GetString()!; + Assert.Contains(_projectPath, message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("SomeOtherProject", message); + } + + [Fact] + public async Task Invalid_WithNoStartupProjects_AndExplicitPath_DoesNotRenderEmptyList() + { + var (analyzers, workspace) = NoProjects(); + Assert.Empty(workspace.Config.ProjectDirectories); + + var result = await ListRulesTool.ListRules(analyzers, workspace, projectPath: _projectPath); + + var root = ToolResultAssert.Error(result, "Invalid"); + var message = root.GetProperty("message").GetString()!; + Assert.DoesNotContain("started with: .", message); + Assert.Contains("No AL project available", message); + } + + [Fact] + public async Task Invalid_WithNoStartupProjects_AndNoPath() + { + var (analyzers, workspace) = NoProjects(); + + var result = await ListRulesTool.ListRules(analyzers, workspace); + + var root = ToolResultAssert.Error(result, "Invalid"); + Assert.Contains("No AL project available", root.GetProperty("message").GetString()); + } + + [Fact] + public async Task Faulted_WhenProjectPathCannotBeNormalized() + { + var (analyzers, workspace) = Resolvers(_projectPath); + + // An embedded NUL makes Path.GetFullPath throw ArgumentException; list_rules must catch it. + var result = await ListRulesTool.ListRules(analyzers, workspace, projectPath: "C:\\bad\0path"); + + var root = ToolResultAssert.Error(result, "Faulted"); + Assert.Equal("System.ArgumentException", root.GetProperty("detail").GetString()); + Assert.False(root.TryGetProperty("reason", out _)); + } +} diff --git a/tests/ALCops.Mcp.Tests/ToolErrorsTests.cs b/tests/ALCops.Mcp.Tests/ToolErrorsTests.cs new file mode 100644 index 0000000..686c0f8 --- /dev/null +++ b/tests/ALCops.Mcp.Tests/ToolErrorsTests.cs @@ -0,0 +1,78 @@ +using System.Text.Json; +using ALCops.Mcp.Models; +using ALCops.Mcp.Services; +using ModelContextProtocol.Protocol; +using Xunit; + +namespace ALCops.Mcp.Tests; + +/// The error envelope: isError, key order and omitted optional fields. +public sealed class ToolErrorsTests +{ + private static string[] Keys(CallToolResult result) => + [.. JsonDocument.Parse(ToolResultAssert.Text(result)).RootElement.EnumerateObject().Select(p => p.Name)]; + + [Fact] + public void Faulted_FromException_HasErrorMessageAndDetailOnly() + { + var result = ToolErrors.Faulted(new InvalidOperationException("boom")); + + Assert.Equal(["error", "message", "detail"], Keys(result)); + var root = ToolResultAssert.Error(result, "Faulted"); + Assert.Equal("boom", root.GetProperty("message").GetString()); + Assert.Equal("System.InvalidOperationException", root.GetProperty("detail").GetString()); + } + + [Fact] + public void NotFound_WithCandidates_KeepsKeyOrder() + { + var result = ToolErrors.NotFound( + FixNotFoundReason.NoFixForEquivalenceKey, "no such key", "C:\\p\\a.al", "LC0020", + [new CodeFixInfo("k1", "Title 1", "Provider")]); + + Assert.Equal(["error", "message", "reason", "candidates", "filePath", "diagnosticId"], Keys(result)); + var root = ToolResultAssert.Error(result, "NotFound", "NoFixForEquivalenceKey"); + var candidate = Assert.Single(root.GetProperty("candidates").EnumerateArray().ToList()); + Assert.Equal(["equivalenceKey", "title", "providerName"], candidate.EnumerateObject().Select(p => p.Name).ToArray()); + } + + [Fact] + public void Invalid_OmitsEveryOptionalField() + { + Assert.Equal(["error", "message"], Keys(ToolErrors.Invalid("bad"))); + } + + [Fact] + public void Ok_LeavesIsErrorUnset() + { + var result = ToolResults.Ok(new { applied = true }); + + Assert.Null(result.IsError); + Assert.Equal("{\"applied\":true}", ToolResultAssert.Text(result)); + } + + [Fact] + public void EveryError_SetsIsError() + { + CallToolResult[] errors = + [ + ToolErrors.Invalid("m"), + ToolErrors.NotFound(FixNotFoundReason.NoFixProvider, "m"), + ToolErrors.Ambiguous("m", [new CodeFixInfo("k", "t", "p")]), + ToolErrors.Stale("f.al", "LC0020", "m"), + ToolErrors.Unavailable(UnavailableReason.NoProxy, "m"), + ToolErrors.Faulted(new IOException("m")), + ToolErrors.Faulted(FileWriteConflictKind.WriteFailed, "m"), + ]; + + Assert.All(errors, e => Assert.True(e.IsError)); + } + + [Fact] + public void NotFoundMessage_NoDiagnosticAtPosition_PointsToAnalyze() + { + var message = ToolErrors.NotFoundMessage(FixNotFoundReason.NoDiagnosticAtPosition, "LC0020", "C:\\p\\a.al", 3, 5); + + Assert.Equal("No LC0020 at C:\\p\\a.al:3:5; re-run analyze and use its line/column.", message); + } +} diff --git a/tests/ALCops.Mcp.Tests/ToolResultAssert.cs b/tests/ALCops.Mcp.Tests/ToolResultAssert.cs new file mode 100644 index 0000000..4ae3156 --- /dev/null +++ b/tests/ALCops.Mcp.Tests/ToolResultAssert.cs @@ -0,0 +1,51 @@ +using System.Text.Json; +using ModelContextProtocol.Protocol; +using Xunit; + +namespace ALCops.Mcp.Tests; + +/// Assertions over the the native tools return. +internal static class ToolResultAssert +{ + /// The single text block of a native tool result. + public static string Text(CallToolResult result) => + Assert.IsType(Assert.Single(result.Content)).Text; + + /// Asserts a success result (no isError, no error property) and returns its JSON root. + public static JsonElement Ok(CallToolResult result) + { + var text = Text(result); + Assert.True(result.IsError != true, $"Expected success, got an error result: {text}"); + + var root = JsonDocument.Parse(text).RootElement; + if (root.ValueKind == JsonValueKind.Object) + Assert.False(root.TryGetProperty("error", out _), $"Success result carries an error property: {text}"); + return root; + } + + /// Asserts a success result and deserializes it with the server's JSON options. + public static T OkAs(CallToolResult result) + { + Ok(result); + return JsonSerializer.Deserialize(Text(result), JsonDefaults.Options)!; + } + + /// + /// Asserts an error result with isError set, as error and, when given, + /// as reason. Returns the JSON root. + /// + public static JsonElement Error(CallToolResult result, string code, string? reason = null) + { + var text = Text(result); + Assert.True(result.IsError == true, $"Expected isError = true: {text}"); + + var root = JsonDocument.Parse(text).RootElement; + Assert.Equal(code, root.GetProperty("error").GetString()); + Assert.False(string.IsNullOrEmpty(root.GetProperty("message").GetString()), $"Empty message: {text}"); + + if (reason is not null) + Assert.Equal(reason, root.GetProperty("reason").GetString()); + + return root; + } +} From dced699d35c5bc71a80a12f3cd9c87a0b48c636c Mon Sep 17 00:00:00 2001 From: Arthur van de Vondervoort Date: Sun, 4 Oct 2026 10:38:29 +0200 Subject: [PATCH 2/4] fix: address review round 1 (Invalid for malformed paths, exact-column match first, precise no-change message) Malformed projectPath/filePath/folderPath strings are reported as Invalid instead of Faulted. FindDiagnosticAsync tries an exact line/column match in both the live and the pragma-suppressed set before any same-line fallback, so a suppressed position is not answered with a neighbouring live diagnostic. apply_fix says when a matching key produced no change in the document. README and AGENTS.md note that a rolled-back apply_fix_all batch is a non-error result with applied false. Co-Authored-By: Claude Fable 5.1 --- AGENTS.md | 2 +- README.md | 2 +- src/ALCops.Mcp/Models/FixLookupResult.cs | 3 + src/ALCops.Mcp/Services/CodeFixRunner.cs | 80 +++++++++++++------ src/ALCops.Mcp/Services/ProjectScope.cs | 42 +++++++++- src/ALCops.Mcp/Tools/AnalyzeTool.cs | 20 ++++- src/ALCops.Mcp/Tools/ApplyFixTool.cs | 3 +- tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs | Bin 16766 -> 18194 bytes tests/ALCops.Mcp.Tests/GetFixesToolTests.cs | Bin 9168 -> 9633 bytes tests/ALCops.Mcp.Tests/ListRulesToolTests.cs | Bin 4693 -> 5529 bytes tests/ALCops.Mcp.Tests/MatchPositionTests.cs | 70 ++++++++++++++++ 11 files changed, 188 insertions(+), 34 deletions(-) create mode 100644 tests/ALCops.Mcp.Tests/MatchPositionTests.cs diff --git a/AGENTS.md b/AGENTS.md index a21bced..fcb2c07 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -70,7 +70,7 @@ When passing analyzers to the child `almcp`, their sibling dependencies must tra - Every error sets `IsError = true` and carries one `ToolError` envelope `{ error, message, reason?, candidates?, filePath?, diagnosticId?, detail? }` (optional fields omitted when null via per-property `JsonIgnore`; `JsonDefaults.Options` is not changed). `error` is one of `Invalid`, `NotFound`, `Ambiguous`, `Stale`, `Unavailable`, `Faulted`; `NotFound` reasons are the `FixNotFoundReason` names, `Unavailable` reasons the `UnavailableReason` constants. The README `## Errors` section is the public table. Every tool body ends in `catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { throw; } catch (Exception ex) { return ToolErrors.Faulted(ex); }`: a cancellation propagates, anything else becomes `Faulted` with `detail` = the exception's full type name. - `get_fixes` returns `{ diagnosticId, filePath, line, column, fixes }` with a non-empty `fixes`; every no-match case is `NotFound` with a reason (`CodeFixRunner` returns `FixLookupResult` / `FixApplyResult` / `FixAllResult` with a `FixNotFoundReason`). Ruleset suppression is checked before analysis; pragma suppression is told apart from no diagnostic because `GetEffectiveDiagnostics` keeps pragma-suppressed diagnostics with `IsSuppressed = true`. Equivalence keys are compared through `KeyOf(action) => action.EquivalenceKey ?? ""` on both the advertising and the matching side, so every advertised key round-trips. `candidates` are de-duplicated by key, first occurrence wins. - `list_rules` is read-only. `get_fixes`, `apply_fix` and `apply_fix_all` re-read changed `.al` files from disk before computing a fix (via `RefreshFromDiskAsync`); the write only proceeds if the file still equals the text the fix was computed from. `apply_fix` returns the error `Stale` on a conflict; `apply_fix_all` writes the non-conflicting files and lists the rest in `conflicts`. There is no explicit reload after writing — the next `GetOrLoadProjectAsync` call picks the written file up via the same refresh. -- Writes go through `GuardedFileWriter`: read the file as bytes → decode with BOM detection → ordinal compare with the expected text → write preamble + new text to a sibling `..alcops.tmp` → `File.Move(temp, path, overwrite: true)`; the temp is deleted in `finally`. The encoding is detected at write time from that read, not stored on `TrackedDocument`. The BOM is sniffed explicitly (UTF-32 LE before UTF-16 LE) and every branch, BOM-less UTF-8 included, uses a strict (throwing) decoder: bytes invalid in the detected encoding (e.g. a Windows-1252 file, a stray Windows-1252 byte behind a UTF-8 BOM, a lone surrogate in UTF-16; the loader reads all of these with U+FFFD replacement, so the stale check alone would pass) are refused as `UnsupportedEncoding` instead of being re-encoded. Never decode through `StreamReader` with BOM detection: it swaps in its own lossy encoding when a BOM is present. `File.Move` replaces a symlink at the target with a regular file and drops Unix mode bits; both accepted, out of scope. **Never `File.Replace` and never a backup file**: almcp's `ProjectWatcher` (`*.al` filter, Renamed handler checks `EndsWith(".al")`) treats a move-over as an in-place update that keeps the `DocumentId`, while replace-with-backup makes it remove and re-add the document. Every `FileWriteConflict` has a `Kind` (serialized as `kind`): `StaleFile`, `UnsupportedEncoding`, `ReadFailed`, `WriteFailed`, `RolledBack`, `NotWritten`; `apply_fix_all` reports it verbatim in `conflicts[].kind`, `apply_fix` maps `StaleFile` to the error `Stale` and every other kind to `Faulted` with `reason` = the kind (an exception thrown by the write itself is `Faulted` / `WriteFailed`). `apply_fix_all` is two-phase: stale/deleted/unreadable/invalidly-encoded files, and files whose fixed text the strict encoder cannot encode (also `UnsupportedEncoding`), drop out as conflicts first, then the rest are committed one by one; a commit failure (any exception; a cancellation is rethrown after the rollback) restores every file already committed in that call from the original bytes held in memory (same temp + move path) and reports the whole staged set in `conflicts` with `applied: false`. A rollback that itself fails, or a file modified after this call wrote it (rollback compares current bytes with what was written and skips the restore), is named in `message` and that file stays in `filesChanged`. `LoadProjectAsync` and `RefreshFromDiskAsync` sweep stray `*.alcops.tmp` files outside `.alpackages` (`ProjectLoader.SweepTempFiles`); the refresh only removes files older than 30 s because tool calls run concurrently and a write may be between temp-create and move. +- Writes go through `GuardedFileWriter`: read the file as bytes → decode with BOM detection → ordinal compare with the expected text → write preamble + new text to a sibling `..alcops.tmp` → `File.Move(temp, path, overwrite: true)`; the temp is deleted in `finally`. The encoding is detected at write time from that read, not stored on `TrackedDocument`. The BOM is sniffed explicitly (UTF-32 LE before UTF-16 LE) and every branch, BOM-less UTF-8 included, uses a strict (throwing) decoder: bytes invalid in the detected encoding (e.g. a Windows-1252 file, a stray Windows-1252 byte behind a UTF-8 BOM, a lone surrogate in UTF-16; the loader reads all of these with U+FFFD replacement, so the stale check alone would pass) are refused as `UnsupportedEncoding` instead of being re-encoded. Never decode through `StreamReader` with BOM detection: it swaps in its own lossy encoding when a BOM is present. `File.Move` replaces a symlink at the target with a regular file and drops Unix mode bits; both accepted, out of scope. **Never `File.Replace` and never a backup file**: almcp's `ProjectWatcher` (`*.al` filter, Renamed handler checks `EndsWith(".al")`) treats a move-over as an in-place update that keeps the `DocumentId`, while replace-with-backup makes it remove and re-add the document. Every `FileWriteConflict` has a `Kind` (serialized as `kind`): `StaleFile`, `UnsupportedEncoding`, `ReadFailed`, `WriteFailed`, `RolledBack`, `NotWritten`; `apply_fix_all` reports it verbatim in `conflicts[].kind`, `apply_fix` maps `StaleFile` to the error `Stale` and every other kind to `Faulted` with `reason` = the kind (an exception thrown by the write itself is `Faulted` / `WriteFailed`). `apply_fix_all` is two-phase: stale/deleted/unreadable/invalidly-encoded files, and files whose fixed text the strict encoder cannot encode (also `UnsupportedEncoding`), drop out as conflicts first, then the rest are committed one by one; a commit failure (any exception; a cancellation is rethrown after the rollback) restores every file already committed in that call from the original bytes held in memory (same temp + move path) and reports the whole staged set in `conflicts` with `applied: false`. That rolled-back batch is a normal (non-`isError`) result with the failure in `message`, because the per-file outcome is in the body; only argument, lookup and single-file write failures use the error envelope. A rollback that itself fails, or a file modified after this call wrote it (rollback compares current bytes with what was written and skips the restore), is named in `message` and that file stays in `filesChanged`. `LoadProjectAsync` and `RefreshFromDiskAsync` sweep stray `*.alcops.tmp` files outside `.alpackages` (`ProjectLoader.SweepTempFiles`); the refresh only removes files older than 30 s because tool calls run concurrently and a write may be between temp-create and move. - `al_compile` defaults to `onlyErrors: true` while nearly every ALCops rule is a warning — callers must pass `options.onlyErrors: false` (the flag lives inside almcp's `options` object; a top-level `onlyErrors` is ignored, and omitting `options` entirely also yields `onlyErrors: false` with no diagnostics cap). This is documented rather than patched, because `ForwardAsync` stays a generic passthrough. - After `apply_fix` / `apply_fix_all`, verify with `analyze` (preferred) or `al_compile` (`options.onlyErrors: false`), not `al_getdiagnostics`. almcp's `ProjectWatcher` (`FileSystemWatcher`) re-reads changed `.al` files, and `al_compile` awaits `WaitForProcessingAsync` before compiling, so it normally picks up on-disk changes before the compile starts. The gate starts signalled and has no debounce, so on slow file systems or right after a large `apply_fix_all` a second `al_compile` may be needed if the watcher has not yet delivered the change notification. `al_getdiagnostics` returns cached compilation results without re-analyzing and will report stale diagnostics. `al_build` does not await the watcher at all. - `analyze` is the one native tool that goes through `AlMcpProxy.ForwardAsync` (`al_compile` with nested `options.onlyErrors=false`, `enableCodeAnalysis=true`, `maxDiagnosticsPerCompilation=int.MaxValue`, never `codeAnalyzers`); it resolves the proxy via `IServiceProvider` because under `--no-proxy` the type is unregistered and the SDK would otherwise expose it as a tool argument; it returns `Unavailable` / `NoProxy` in that case, `AlmcpNotFound` when almcp is not installed, `AlmcpNotReady` when it failed to start, and `AlmcpCallFailed` (almcp text in `detail`) when the proxied call itself reports an error. diff --git a/README.md b/README.md index 5760149..38424ef 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ The native tools (`list_rules`, `get_fixes`, `apply_fix`, `apply_fix_all`) work | `list_rules` | List analyzer rules with metadata (ID, title, severity, category, cop). | | `get_fixes` | Get available code fixes for a specific diagnostic at a location. Returns `{ diagnosticId, filePath, line, column, fixes: [{ equivalenceKey, title, providerName }] }`; when nothing matches, the error `NotFound` with a `reason` (see [Errors](#errors)). | | `apply_fix` | Apply a code fix to resolve a diagnostic. Writes the fixed content to disk unless the file changed after the fix was computed (error `Stale`); a file that cannot be read, decoded or written is `Faulted` with `reason` `ReadFailed`, `UnsupportedEncoding` or `WriteFailed`. The write is atomic (sibling temp file renamed over the target) and keeps the file's encoding, BOM and line endings. | -| `apply_fix_all` | Apply a code fix to every occurrence of a diagnostic rule across a project or a single file (like VS Code's "Fix all in workspace"). Writes to disk unless `dryRun` is set. A rule with several distinct fixes and no `equivalenceKey` is the error `Ambiguous` with `candidates`. Zero occurrences is a success (`applied: false`, `diagnosticsFound: 0`), except a rule the project ruleset suppresses, which is `NotFound` with `reason: SuppressedByRuleset`. Files that changed on disk mid-operation are skipped and listed in `conflicts`; the rest are written as one batch — if any write fails, every file already written in that call is restored and nothing from the batch is kept. | +| `apply_fix_all` | Apply a code fix to every occurrence of a diagnostic rule across a project or a single file (like VS Code's "Fix all in workspace"). Writes to disk unless `dryRun` is set. A rule with several distinct fixes and no `equivalenceKey` is the error `Ambiguous` with `candidates`. Zero occurrences is a success (`applied: false`, `diagnosticsFound: 0`), except a rule the project ruleset suppresses, which is `NotFound` with `reason: SuppressedByRuleset`. Files that changed on disk mid-operation are skipped and listed in `conflicts`; the rest are written as one batch — if any write fails, every file already written in that call is restored and nothing from the batch is kept. A batch that fails and is rolled back is a normal (non-`isError`) result with `applied: false`, the failure in `message` and every staged file in `conflicts`, because the per-file outcome is in the body; only argument, lookup and single-file write failures use the error envelope. | | `analyze` | Compile with all configured analyzers and return structured cop + compiler diagnostics (analyzer, hasFix, filters, summary). Wraps `al_compile` with `onlyErrors: false`; needs `almcp`. | ### Proxied from Microsoft's `almcp` diff --git a/src/ALCops.Mcp/Models/FixLookupResult.cs b/src/ALCops.Mcp/Models/FixLookupResult.cs index b58a84e..e27d267 100644 --- a/src/ALCops.Mcp/Models/FixLookupResult.cs +++ b/src/ALCops.Mcp/Models/FixLookupResult.cs @@ -36,9 +36,12 @@ public record FixLookupResult(IReadOnlyList Fixes, FixNotFoundReaso /// /// Outcome of CodeFixRunner.ApplyFixAsync: the computed fix, or the reason there is none. /// is filled for . +/// , when set, replaces the shared per-reason message of the NotFound error. /// public record FixApplyResult(CodeFixResult? Fix, FixNotFoundReason? NotFoundReason, IReadOnlyList Candidates) { + public string? MessageOverride { get; init; } + public static FixApplyResult Applied(CodeFixResult fix) => new(fix, null, []); public static FixApplyResult NotFound(FixNotFoundReason reason, IReadOnlyList? candidates = null) => diff --git a/src/ALCops.Mcp/Services/CodeFixRunner.cs b/src/ALCops.Mcp/Services/CodeFixRunner.cs index fd1554f..9f90b41 100644 --- a/src/ALCops.Mcp/Services/CodeFixRunner.cs +++ b/src/ALCops.Mcp/Services/CodeFixRunner.cs @@ -90,8 +90,14 @@ public async Task ApplyFixAsync( } } - // The key matched, but no matching action changes this document. - return FixApplyResult.NotFound(FixNotFoundReason.NoFixForDiagnostic); + // The key matched, but no matching action changes this document. Same reason as "no fix at + // all", but get_fixes did advertise this key, so the message says what actually happened. + return FixApplyResult.NotFound(FixNotFoundReason.NoFixForDiagnostic) with + { + MessageOverride = + $"A fix with equivalence key '{equivalenceKey}' exists for {diagnosticId} at {filePath}:{line}:{column} " + + "but produced no change in this document (it may edit another file, which apply_fix does not support yet)." + }; } /// @@ -546,36 +552,58 @@ private static async Task FindDiagnosticAsync( && Path.GetFullPath(fp).Equals(Path.GetFullPath(documentPath), StringComparison.OrdinalIgnoreCase)) .ToImmutableArray(); - if (AtPosition(diagnostics.Where(d => !d.IsSuppressed), line, column) is { } hit) - return new DiagnosticLookup(hit, null); + var (hit, suppressed) = MatchPosition(diagnostics, StartOf, d => d.IsSuppressed, line, column); + if (hit is null) + return new DiagnosticLookup(null, FixNotFoundReason.NoDiagnosticAtPosition); - if (AtPosition(diagnostics.Where(d => d.IsSuppressed), line, column) is not null) - return new DiagnosticLookup(null, FixNotFoundReason.SuppressedByPragma); + return suppressed + ? new DiagnosticLookup(null, FixNotFoundReason.SuppressedByPragma) + : new DiagnosticLookup(hit, null); + } - return new DiagnosticLookup(null, FixNotFoundReason.NoDiagnosticAtPosition); + /// 1-based start line/column of a diagnostic. + private static (int Line, int Column) StartOf(Diagnostic d) + { + var start = d.Location.GetLineSpan().StartLinePosition; + return (start.Line + 1, start.Character + 1); } /// - /// The diagnostic starting exactly at the 1-based /, - /// or else the first one starting on that line. + /// Picks the candidate at the 1-based /. An exact + /// match wins over a same-line fallback in either set, so a pragma-suppressed diagnostic at the exact + /// position is not answered with a live neighbour on the same line. Order: exact live, exact + /// suppressed, same-line live, same-line suppressed. Suppressed says which set the hit came from. /// - private static Diagnostic? AtPosition(IEnumerable candidates, int line, int column) + internal static (T? Hit, bool Suppressed) MatchPosition( + IEnumerable candidates, + Func startOf, + Func isSuppressed, + int line, + int column) + where T : class { - var diagnostics = candidates.ToList(); - - return diagnostics.FirstOrDefault(d => - { - var lineSpan = d.Location.GetLineSpan(); - var startLine = lineSpan.StartLinePosition.Line + 1; - var startCol = lineSpan.StartLinePosition.Character + 1; - - return startLine == line && startCol == column; - }) - // Fallback: find any diagnostic with matching ID on the same line - ?? diagnostics.FirstOrDefault(d => - { - var lineSpan = d.Location.GetLineSpan(); - return lineSpan.StartLinePosition.Line + 1 == line; - }); + var all = candidates.ToList(); + var live = all.Where(d => !isSuppressed(d)).ToList(); + var suppressed = all.Where(isSuppressed).ToList(); + + if (ExactlyAt(live, startOf, line, column) is { } exactLive) + return (exactLive, false); + if (ExactlyAt(suppressed, startOf, line, column) is { } exactSuppressed) + return (exactSuppressed, true); + if (OnLine(live, startOf, line) is { } lineLive) + return (lineLive, false); + if (OnLine(suppressed, startOf, line) is { } lineSuppressed) + return (lineSuppressed, true); + return (null, false); } + + /// The first candidate starting exactly at /. + private static T? ExactlyAt(IEnumerable candidates, Func startOf, int line, int column) + where T : class => + candidates.FirstOrDefault(d => startOf(d) == (line, column)); + + /// The first candidate starting anywhere on . + private static T? OnLine(IEnumerable candidates, Func startOf, int line) + where T : class => + candidates.FirstOrDefault(d => startOf(d).Line == line); } diff --git a/src/ALCops.Mcp/Services/ProjectScope.cs b/src/ALCops.Mcp/Services/ProjectScope.cs index 4e7d1ea..59a4921 100644 --- a/src/ALCops.Mcp/Services/ProjectScope.cs +++ b/src/ALCops.Mcp/Services/ProjectScope.cs @@ -32,7 +32,9 @@ internal static class ProjectScope return knownProjects[0]; } - var normalized = Normalize(projectPath); + if (!TryNormalizePath(projectPath, trimTrailingSeparator: true, out var normalized, out invalidMessage, "projectPath")) + return null; + if (!knownProjects.Contains(normalized, StringComparer.OrdinalIgnoreCase)) { invalidMessage = @@ -56,7 +58,9 @@ internal static bool RequireProjectFolder(string projectPath, out string? invali return false; } - var full = Path.GetFullPath(projectPath); + if (!TryNormalizePath(projectPath, trimTrailingSeparator: false, out var full, out invalidMessage, "projectPath")) + return false; + if (!Directory.Exists(full)) { invalidMessage = $"projectPath '{projectPath}' does not exist. Pass the absolute path of the AL project folder (contains app.json)."; @@ -73,5 +77,39 @@ internal static bool RequireProjectFolder(string projectPath, out string? invali return true; } + /// + /// (optionally without a trailing separator) for a + /// caller-supplied path. An empty or whitespace-only string, or one + /// rejects (embedded NUL, unsupported format, too long), is an argument error: returns false with + /// set, so the tool reports Invalid rather than Faulted. + /// + internal static bool TryNormalizePath( + string path, + bool trimTrailingSeparator, + out string normalized, + out string? invalidMessage, + string parameterName = "path") + { + normalized = ""; + if (string.IsNullOrWhiteSpace(path)) + { + invalidMessage = $"{parameterName} '{path}' is not a valid path."; + return false; + } + + try + { + var full = Path.GetFullPath(path); + normalized = trimTrailingSeparator ? Path.TrimEndingDirectorySeparator(full) : full; + invalidMessage = null; + return true; + } + catch (Exception ex) when (ex is ArgumentException or NotSupportedException or PathTooLongException) + { + invalidMessage = $"{parameterName} '{path}' is not a valid path."; + return false; + } + } + private static string Normalize(string path) => Path.TrimEndingDirectorySeparator(Path.GetFullPath(path)); } diff --git a/src/ALCops.Mcp/Tools/AnalyzeTool.cs b/src/ALCops.Mcp/Tools/AnalyzeTool.cs index 5940c8b..31ad7a6 100644 --- a/src/ALCops.Mcp/Tools/AnalyzeTool.cs +++ b/src/ALCops.Mcp/Tools/AnalyzeTool.cs @@ -32,6 +32,23 @@ public static async Task Analyze( if (limit <= 0) return ToolErrors.Invalid("limit must be a positive integer."); + // Normalized before the proxy checks so a malformed path is Invalid whether or not almcp runs. + string? normalizedFilePath = null; + if (filePath is not null) + { + if (!ProjectScope.TryNormalizePath(filePath, trimTrailingSeparator: false, out var full, out var pathMessage, "filePath")) + return ToolErrors.Invalid(pathMessage!); + normalizedFilePath = full; + } + + string? normalizedFolderPath = null; + if (folderPath is not null) + { + if (!ProjectScope.TryNormalizePath(folderPath, trimTrailingSeparator: true, out var full, out var pathMessage, "folderPath")) + return ToolErrors.Invalid(pathMessage!); + normalizedFolderPath = full; + } + var proxy = services.GetService(typeof(AlMcpProxy)) as AlMcpProxy; if (proxy is null) return ToolErrors.Unavailable(UnavailableReason.NoProxy, @@ -53,9 +70,6 @@ public static async Task Analyze( var callerPassedFileOrFolder = filePath is not null || folderPath is not null; var callerPassedAnyScope = callerPassedProjectPath || callerPassedFileOrFolder; - var normalizedFilePath = filePath is not null ? Path.GetFullPath(filePath) : null; - var normalizedFolderPath = folderPath is not null ? Path.TrimEndingDirectorySeparator(Path.GetFullPath(folderPath)) : null; - var scopePath = normalizedFilePath ?? normalizedFolderPath; var config = workspaceResolver.Config; string? invalidMessage = null; diff --git a/src/ALCops.Mcp/Tools/ApplyFixTool.cs b/src/ALCops.Mcp/Tools/ApplyFixTool.cs index 62aabae..fcc0267 100644 --- a/src/ALCops.Mcp/Tools/ApplyFixTool.cs +++ b/src/ALCops.Mcp/Tools/ApplyFixTool.cs @@ -47,7 +47,8 @@ public static async Task ApplyFix( if (outcome.NotFoundReason is { } reason) return ToolErrors.NotFound(reason, - ToolErrors.NotFoundMessage(reason, diagnosticId, filePath, line, column, equivalenceKey), + outcome.MessageOverride + ?? ToolErrors.NotFoundMessage(reason, diagnosticId, filePath, line, column, equivalenceKey), filePath, diagnosticId, reason == FixNotFoundReason.NoFixForEquivalenceKey ? outcome.Candidates : null); diff --git a/tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs b/tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs index bb724e981c81b438a48923f91a1ea0ca49de3659..8d55813d8b6eaa715541ee696c2e420512abd80e 100644 GIT binary patch delta 4036 zcmcIneQaA-6`%8M$8MYte}Cl0NftYGY$tKj#7<(n$y!2_wCS2<&^6nO^U`>8{9OCF zN*L8p{~Tgkb3mYJ6PtiPK%%G!X&}@I2_~feF;+BzCN#P~5NJ|CtAzSb#m71KJ^R^h z))b*x)_w1ud+s^E^Ks9)@86eod#~tTwYf}&QZbV|&r%DsQoa~kOy@(3(uSBV#!Xiu zF6TbWXC#TQrXxDj6&S|h6Ze&m_4MpLQRaWTl*`ERnpr5dUX%+$TFyv0lx{E?a>BM) z%nNC;TKgHXDD!1l*7Y}-3@@5sM|WOlf?GN-j{nll;h3t6$*on2o5Q9--j z0bgnue@PK=sUrW(mM9f=&#BtxbJ zI3Kc1+k#9LH}Ke|l-&>uzWoHLoV11p+u%m0@ug#^{$$}8-*iflx8{qBLUvOsY>OMu zK5>n!kjV<` z=(x`a)XBP_39fl91FAkQ?O{6p>VhB?cuU-|YlNxFG0;XZAkY+Y>}!ts6ED7@V`>MCauc~LIxDq?7Y z394(FK+b$$7c4f}d0yT$^ug7supuj@1z9R!T0&2&8~)tnLB@McJ{&#Gb{xl=6An!l z$)n%dbCk#?h`0ISr_F<04Z<{oWJEAXuHM%I5)~Q_} z@e2{ntb!<)3OOcIhn&k4&`2`)fy|$^FKY^4v!6!cx9!6mtJe|5*=Yv_^cxN;4DULq z;pv^Ld|hx-UcU2i9hJd2>fGY&O`eij{A7;XbtX0r6g;p|JeH!>KW=Ie_mi+hj^_Q zu`zD2=@|{V3l;|*8u|eU^pD|=wf^J(AHLTf57*#BzdD+Z-VgE-T)Ts4p6iLitwEdr zKF>bfLqGE$sX>5pu8ljPdBjA4N(LyG#DI^Rxi5~j&Sv;9(1vUjaGKQ*TKP-~I=OBQ z=PJ4f;tpRQU2Q!hE#NiAPlofGpE+t(?*NdDTDkJy9FEuKmp(`bB_5=Xs$|&ZAhrBY zg0qiTx!|A_*%)fCaeyhC+0Mu@HmpQ}lJVC=c#if$9Kcyv}m z&KmjfddN}sCP&@{cSD{7#iYV5rUMX27b_4{Ba+J7LTZx7dI4%{1d{V!OQ^r%YQ{ZiwFavIcTga(sqe4sw;9Dv>d z-yHL3{(O7vY3@(@COM#ljx*%EIewVy#S)4$yzBB==}1yRYlruaOlhx12v0;*hw~=H zTfwr<4K}x_E5_zi#ob(bA+ssUnQf7OYJGnq#zo$mpolmpqc|>2&hXI&- zmMKcK(^GNmRd_U)4|U^>m|EDaDMgWKI);YsHG9;%0!gG_Rub^52`dPGB34~OY3VF} z zP1@kciG=!ApQKAqchXY53!P$}%M|lcQRJTYS3a>|Gf54Iu5RB+DmT`v$+7!-`0XUc Lgu*wQBIExH;n2^& delta 2833 zcmb_eTWnKh98WhcuIt9ucD!2x+>2 zKKLL4+y4O+AA}bYlSO#&!H^*MVoXR3FT^MsUo-+yK@%^DLI3Bpr|kwbA>kzFJKs6q z`TzdE|MmM;Pw^hT$vY!;@eBEMcAkukjZlku>(1Fl>rQGemC5@aUzkkgj-+Q(dEd&o zY~>uW@|JkzoPFg1(UQnfG&M_Qd@B$1ri$EjA)BT-5cb6ynaR_+L?o zL7uURUxXV9F`Va3!f!k^v^T|YA>8VP(@lhc&nNNPH;RG>;9;i}K7K&}-!@S~M1Hv0jKIhJE_fu^#viAU*wJG;vkx8)%HhZ+w~&=7Jpt0e!SR$)=xZ^; zL`(lF;e?w?r?mV#!-CEoV-c8nah30ZVZLQcc^M{}HEkW)qjqwb!onOZ=DE6_; z-xUk-J7dEqVmJIEE>_;qqUXe0ESchXFG)<@)le0yGbVYq=i|8?l_LX~hlP{{K9`IM zsy~!4bs};JIHd-p$CSV+sTM9vC!dz{HW|t3oR&?jE2B49gdkfrB86istE9w-U5X_v zR1YV{Q>N?6WR-Ynl^FKUWimRmh1BKg!VFekm0vDJ7jjvWrl?Flmd=$I)+gZ&VuFKK zsCL7G%8ooPs9bo9;I7I9LbV5WsAF(Oy%$&Rsoi*!5Y>?QmeL%7JDMJ>36(ONTu52S z#bhqIFpkrFg+_^wHiSt#_8byc$tkAq+$VRVa9C@>k$ zOzU(|)LA)h5xETp^j+|mu7iE5^qu(ZfxY?xcv~MrpfB~qIBqrsq!rJxkA0C0w%1x8 zqYZ-` z+Ti8)``tBFJ;1Y?px0`IgjI)pH+Xx^+6!w|CH!HP)ERrQUhf9)U-#^Vgsp|Gl=!f; zyw40v5&D|VS0BjDb_aOuRu=YzeI6l2@UPvbt(n2jo?=AW(OG9iYbL#rroE({aT^nW zn~qM*0}I|NZLW~5#H|tFoKsN;2y0Gx*&Fbu)7~&?UUB(2!4F&kxMJuL9!h6Yi6s5H z7b|W)6=@L5HLC)y4I1m37P~d?sbo=O&7Tde3#8GlM9R;WG>--E@W#M#Oc0tYqYiK5 zGP>o}a31;FJB>Z~$=!ixRcx7|<_mcX$ozeXk4*yGbP#7Z{9%@dGT^T~1yBmu%ISj{ zfnDn$kASZODyC!o6Y!$UP|ydtpbIVqgYXmH#?T}j3r)dSp&huY3isoV;3E>h(~S(@pv8W*$-Df7?f*IG!3|&NGBSwv|1RVbs4F8N@Hb*e;lf^;I{Rv$0Xm zmp{f_uoRbqJs!fRvvz!`Y$S83JRUF0h0ogfZM>b6eHho^cFpuc2H|#>NL+7ISq7cD NJnCZ|dwUc;`ag8l42=K) diff --git a/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs b/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs index 9b392969f7b871e456eaf768748f9dadab083915..daec51054e08fc54c96fcf26dbb964507f402e4c 100644 GIT binary patch delta 1993 zcmb7F&2Jl35XT>}*BigYcGgaI{q=So(wGFhm1vX_C$tL0N=sMVcvS3jVl!5)oOO$yx*JO z%)Iwze=2@)<)ciT@Xbc0zC|y+Jg;vza!Z@rxh1`%RU0#8zYv%HWBOHHw`TS8F0!vr z>^}LxrKN>@CL<9p8}8-~Rp2zc`dp7yRIB?IrU z`8{_UzVR+N>-+k)#P&SHn8qDvfZo6D7vNss$hc!fFAT_@a;2_TtKAFovE0W#B0C@( z&&aKX>kI0p`4)lqW5aOKH{k^Gj;|PWmLc~6zX<*Qu$5NQ{~YGB;$N{wU;2yINCGdH zvb6$t+15ViLbmn;7qK(}4!M}ENr7m{pUBHKEbI;>2Nx>ucIXwxwBncG!~SWE z?DU+&5DdyEau(nqn6P+vLl+=MhI;ZZLW`U`(LdWho|~lSSY-$#{VcimJ__*Nz>7$? z;unApp6!Wv7(N@!A19XK&%uc&&rSi(M!ZlCiFSfld*%qjSUB6w(+ZDv^L!O%e*X%W z?OYDI0JI`g7U%uQ8d?u{76}K7Ln%9wm*Dqbp0(OIhA zRx2hgYVeN`B+C5r8#mR`Q;b%D$3D3o-`vrHNva&rp%d%>HlwLib)t2rw%CQR0eAf0 zZL9E%=Nd-zhqR1xCccVtFMie9&fntG@byR(Ee{eDejia#3yL?aId*IFNdZ0;$I*KF zn=8VA)OEmn4M|117ZN#?wZyy?%7X;^yR-wEwJf#d){$ilgjAPQTL+pIT^bA|SRP1_7n=36Ow*mBO%?nLKl`p zq8p7SBnG1kc4|!A8WV#WBO795tuY}cZuRvy6a>;qX6DSf_uX^vd-u*0|HtloHill{ zgW*~B{N;{Fly~(`MP0p-=|G70CfoGd|Bb_uNXoYEOg^HM>&?lX>Xxk2=t4Lc_tJEj zn-B0&ZYq!;b2z}qd17?C-K>van4jlji*4vEn#5YsG$_$TWya(dY&S(UB6IS8ACgt{BdVCm_R2XDxuMxl9fjH?Ls?W@Z3U zW{4E6gx}~RSu7jRpiid;XJT<%XT?+9Bw=#{`Q4;KlfJI7tzfXB-*==3=KZJyIQ%ra zpf`08Va_z2C*DdJ);ycX)Zm?2llqqRErddKTr?V5OpSYHUiXH5Z(iYHV_0MB@rkwI zH*1I1GML*X*2dtDWdf;p!m5MbdNGUp@xeMsFppK+75uV|2tU_;dpGXeTXP|{?L#EV PZ94;hbq|$2lmz_`ER3QU diff --git a/tests/ALCops.Mcp.Tests/ListRulesToolTests.cs b/tests/ALCops.Mcp.Tests/ListRulesToolTests.cs index 63a86970091d4476e4deda23f82209590527375e..9185bca741119c8a90fb1ae9ce46d47b340c6f3b 100644 GIT binary patch delta 1399 zcmaJ>TWb?R6t)4n;J#* z)fZ9bNq>Qe3PSNwpG6P^eG`1}!5<-lXJ!*yZFJc^Gt9Ys=X~ePe%}4~z{Bm^Q6f}6 zU%(?1qasj+NiQf&3&B(oJvw?M-OyeRxvz^Tfgau1(Poziw_1?tY&q7B61R~#)pFOK znzt>JZtZKIF@EbnCrV&=z+fcSYo4|i!RK4+agf{Eu7a$!6%vuj9qawFEr7$m_C=8Q z+i42Uod1=x#M86L^e48MbZr0Xs3#rVz1VrTzKpZWE85N z&eyn2*#Nm)HyS}K4qSu zM2OwC#NJJdm zi&@y-Iv;z?2eb{e-_bD0&+Df_=90(%757GRc6HpnW@ie4M+Yj_d2W={-8BBbZcFQO zYH(MDDn%Atp_b7#1-$AWUISEzXtc7C`f_Tv{5K8LWh!KCw%&}Z zq_i#|x}3>1Gk|QGE9@zZ*uc>#DXvpbPeF)rHA&dKkQG|WDlP3j30AbZ2;Yr&G>Ff7 zCZ?9kzNRW59>u=GTwt3ECK?mQ3XzAU9|RwLI3$Z>RdCFqD!RO-U0;F|bcWiu`VfW}qA{E|y0lQ%z6cI$r!dZ;IPY=mFp z^kx7|(9+mVj|SQod`x*Hpep;jbaxkQJyjxccFtUm?YrB!BK0zvuk@S@8C7^*u#&=r z6IQ!LC1!$@piZBK5|lpI0xuECfo9-939QQ$3(jiTD^n7Hu7<&+LKcBq4O%IdR%!=A zp<03~6TWhPG@zpDd$j1nctm^UgU|w836;i5`>@ra9RwhKd%TT!$F3z6Lj zf-jLJzF&^+N=W5v2UgRU;A2#W-_hGph`oR}v7#(eJp`X(9xrXAm)(_)yG0B}^P2jg z3%g}4iWK`?47eD85iygDoPhVD>-0@h*b+F+LU4@WTi<1LFm4pUipGNcW-KUc%P#Yl6r-X>1y=n7`-Ds~)~JtoU; z$Htb~CC$7hW_0{J`c68-i{q9_*=EPL2vu#LGS$G$XsaeGa-Zv&sp`E(<0Wo3W<3s_ h3T!W-P?VqH=;B1Y(rNiNaTw%AO0YU|UkTmlp}*5b*5CjD diff --git a/tests/ALCops.Mcp.Tests/MatchPositionTests.cs b/tests/ALCops.Mcp.Tests/MatchPositionTests.cs new file mode 100644 index 0000000..c4fcef1 --- /dev/null +++ b/tests/ALCops.Mcp.Tests/MatchPositionTests.cs @@ -0,0 +1,70 @@ +using ALCops.Mcp.Services; +using Xunit; + +namespace ALCops.Mcp.Tests; + +/// +/// Position matching behind get_fixes / apply_fix. An AL pragma covers whole lines, so a live and a +/// suppressed diagnostic of one rule on one line cannot be built from a fixture cheaply; the ordering +/// is tested on the helper directly. +/// +public sealed class MatchPositionTests +{ + private sealed record Hit(string Name, int Line, int Column, bool Suppressed); + + private static (Hit? Hit, bool Suppressed) Match(int line, int column, params Hit[] hits) => + CodeFixRunner.MatchPosition(hits, h => (h.Line, h.Column), h => h.Suppressed, line, column); + + [Fact] + public void ExactSuppressed_WinsOverSameLineLive() + { + var (hit, suppressed) = Match(5, 20, + new Hit("live", 5, 3, false), + new Hit("pragma", 5, 20, true)); + + Assert.Equal("pragma", hit?.Name); + Assert.True(suppressed); + } + + [Fact] + public void ExactLive_WinsOverExactSuppressed() + { + var (hit, suppressed) = Match(5, 3, + new Hit("pragma", 5, 3, true), + new Hit("live", 5, 3, false)); + + Assert.Equal("live", hit?.Name); + Assert.False(suppressed); + } + + [Fact] + public void SameLineLive_WinsOverSameLineSuppressed_WhenNoExactMatch() + { + var (hit, suppressed) = Match(5, 99, + new Hit("pragma", 5, 3, true), + new Hit("live", 5, 20, false)); + + Assert.Equal("live", hit?.Name); + Assert.False(suppressed); + } + + [Fact] + public void SameLineSuppressed_WhenOnlySuppressedOnLine() + { + var (hit, suppressed) = Match(5, 99, + new Hit("other line", 6, 99, false), + new Hit("pragma", 5, 3, true)); + + Assert.Equal("pragma", hit?.Name); + Assert.True(suppressed); + } + + [Fact] + public void NoHit_WhenNothingOnLine() + { + var (hit, suppressed) = Match(5, 3, new Hit("other line", 6, 3, false)); + + Assert.Null(hit); + Assert.False(suppressed); + } +} From 555a6bc5c6db55619113b589b03999cad7a260ea Mon Sep 17 00:00:00 2001 From: Arthur van de Vondervoort Date: Sun, 4 Oct 2026 10:45:20 +0200 Subject: [PATCH 3/4] fix: address review round 2 (validate filePath, consistent path echo, FileNotInProject for document scope) The fix tools validate filePath like projectPath, so a malformed path is Invalid, and echo the normalized absolute path in every result. apply_fix_all in document scope reports a file outside the project as NotFound/FileNotInProject instead of zero occurrences. analyze puts the almcp text in the message as well as in detail. Test sources no longer contain literal NUL bytes. Doc comment on CodeFixInfo corrected. Co-Authored-By: Claude Fable 5.1 --- src/ALCops.Mcp/Models/CodeFixInfo.cs | 5 +-- src/ALCops.Mcp/Services/CodeFixRunner.cs | 3 ++ src/ALCops.Mcp/Tools/AnalyzeTool.cs | 13 +++++--- src/ALCops.Mcp/Tools/ApplyFixAllTool.cs | 7 +++++ src/ALCops.Mcp/Tools/ApplyFixTool.cs | 4 +++ src/ALCops.Mcp/Tools/GetFixesTool.cs | 6 +++- tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs | Bin 18194 -> 17821 bytes .../ALCops.Mcp.Tests/ApplyFixAllToolTests.cs | 29 ++++++++++++++++++ tests/ALCops.Mcp.Tests/ApplyFixToolTests.cs | 25 +++++++++++++++ tests/ALCops.Mcp.Tests/GetFixesToolTests.cs | Bin 9633 -> 10180 bytes tests/ALCops.Mcp.Tests/ListRulesToolTests.cs | Bin 5529 -> 5403 bytes 11 files changed, 84 insertions(+), 8 deletions(-) diff --git a/src/ALCops.Mcp/Models/CodeFixInfo.cs b/src/ALCops.Mcp/Models/CodeFixInfo.cs index 99189be..a310fac 100644 --- a/src/ALCops.Mcp/Models/CodeFixInfo.cs +++ b/src/ALCops.Mcp/Models/CodeFixInfo.cs @@ -1,8 +1,9 @@ namespace ALCops.Mcp.Models; /// -/// One code fix offered for a diagnostic: the shape of both the fixes of get_fixes and -/// the candidates of an error, so the two are identical by construction. +/// One code fix offered for a diagnostic: the element type of both the fixes of get_fixes and +/// the candidates of an error. candidates holds one entry per distinct key (first wins); +/// fixes lists every registered action. /// is the action's key, or "" when the provider set none; pass it /// back verbatim. /// diff --git a/src/ALCops.Mcp/Services/CodeFixRunner.cs b/src/ALCops.Mcp/Services/CodeFixRunner.cs index 9f90b41..2beb963 100644 --- a/src/ALCops.Mcp/Services/CodeFixRunner.cs +++ b/src/ALCops.Mcp/Services/CodeFixRunner.cs @@ -208,6 +208,9 @@ public async Task ApplyFixAllAsync( var normalizedFilePath = filePath is null ? null : Path.GetFullPath(filePath); + if (scope == FixAllScope.Document && session.GetDocument(normalizedFilePath!) is null) + return NotFound(diagnosticId, FixNotFoundReason.FileNotInProject); + var diagnostics = await CollectDiagnosticsForRuleAsync(session, diagnosticId, normalizedFilePath, ct, analyzerProvider); if (diagnostics.IsEmpty) return new FixAllResult(FixAllStatus.NoDiagnosticsFound, diagnosticId, 0, null, null, [], [], []); diff --git a/src/ALCops.Mcp/Tools/AnalyzeTool.cs b/src/ALCops.Mcp/Tools/AnalyzeTool.cs index 31ad7a6..fd5f54c 100644 --- a/src/ALCops.Mcp/Tools/AnalyzeTool.cs +++ b/src/ALCops.Mcp/Tools/AnalyzeTool.cs @@ -137,10 +137,13 @@ public static async Task Analyze( /// /// Maps a failed proxied al_compile (almcp gone, session lost, or an error al_compile itself - /// reported) to Unavailable/AlmcpCallFailed, with the almcp text in detail. + /// reported) to Unavailable/AlmcpCallFailed, with the almcp text in message and detail. /// - internal static CallToolResult MapProxyFailure(CallToolResult result) => - ToolErrors.Unavailable(UnavailableReason.AlmcpCallFailed, - "The proxied al_compile call failed (almcp may have exited, its session was lost, or al_compile itself reported an error).", - string.Join('\n', result.Content.OfType().Select(b => b.Text))); + internal static CallToolResult MapProxyFailure(CallToolResult result) + { + var almcpText = string.Join('\n', result.Content.OfType().Select(b => b.Text)); + return ToolErrors.Unavailable(UnavailableReason.AlmcpCallFailed, + $"The proxied al_compile call failed (almcp may have exited, its session was lost, or al_compile itself reported an error): {almcpText}", + almcpText); + } } diff --git a/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs b/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs index 287bdcf..367b825 100644 --- a/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs +++ b/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs @@ -50,6 +50,13 @@ public static async Task ApplyFixAll( if (fixAllScope == FixAllScope.Document && string.IsNullOrWhiteSpace(filePath)) return ToolErrors.Invalid("filePath is required when scope='document'."); + if (fixAllScope == FixAllScope.Document) + { + if (!ProjectScope.TryNormalizePath(filePath!, trimTrailingSeparator: false, out var fullFilePath, out var invalidFilePath, "filePath")) + return ToolErrors.Invalid(invalidFilePath!); + filePath = fullFilePath; + } + if (!ProjectScope.RequireProjectFolder(projectPath, out var invalidMessage)) return ToolErrors.Invalid(invalidMessage!); diff --git a/src/ALCops.Mcp/Tools/ApplyFixTool.cs b/src/ALCops.Mcp/Tools/ApplyFixTool.cs index fcc0267..88a5d3e 100644 --- a/src/ALCops.Mcp/Tools/ApplyFixTool.cs +++ b/src/ALCops.Mcp/Tools/ApplyFixTool.cs @@ -37,6 +37,10 @@ public static async Task ApplyFix( if (!ProjectScope.RequireProjectFolder(projectPath, out var invalidMessage)) return ToolErrors.Invalid(invalidMessage!); + if (!ProjectScope.TryNormalizePath(filePath, trimTrailingSeparator: false, out var fullFilePath, out var invalidFilePath, "filePath")) + return ToolErrors.Invalid(invalidFilePath!); + filePath = fullFilePath; + var session = await sessionManager.GetOrLoadProjectAsync(projectPath, cancellationToken); var analyzerSpecs = AnalyzerSpec.ParseJsonArray(analyzers); diff --git a/src/ALCops.Mcp/Tools/GetFixesTool.cs b/src/ALCops.Mcp/Tools/GetFixesTool.cs index 1e157ab..1f8def4 100644 --- a/src/ALCops.Mcp/Tools/GetFixesTool.cs +++ b/src/ALCops.Mcp/Tools/GetFixesTool.cs @@ -32,6 +32,10 @@ public static async Task GetFixes( if (!ProjectScope.RequireProjectFolder(projectPath, out var invalidMessage)) return ToolErrors.Invalid(invalidMessage!); + if (!ProjectScope.TryNormalizePath(filePath, trimTrailingSeparator: false, out var fullFilePath, out var invalidFilePath, "filePath")) + return ToolErrors.Invalid(invalidFilePath!); + filePath = fullFilePath; + var session = await sessionManager.GetOrLoadProjectAsync(projectPath, cancellationToken); var analyzerSpecs = AnalyzerSpec.ParseJsonArray(analyzers); @@ -46,7 +50,7 @@ public static async Task GetFixes( filePath, diagnosticId); return ToolResults.Ok(new GetFixesResult( - diagnosticId, Path.GetFullPath(filePath), line, column, lookup.Fixes)); + diagnosticId, filePath, line, column, lookup.Fixes)); } catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { diff --git a/tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs b/tests/ALCops.Mcp.Tests/AnalyzeToolTests.cs index 8d55813d8b6eaa715541ee696c2e420512abd80e..09d0e8b0c4b0fd50798faded70ca1149ed2f8779 100644 GIT binary patch delta 3029 zcmb_eTTD}D98W=1NR5y16CO7n3Dq`ml$^#OYql5|_;vvutkJ%a*Cx)GgU1S+@PpX-_XWo0xc#bH4AK z@BDxN-~al3o2LZtofMprbP3a`cw(Li4EEEDDRTeZBDtTA#;BD0$%V04aydR1OSw0O zq#Ngy8?TEu&e=8|DJzGQG(*qPlzZcm-jtP_Oef+D55m2Wf!P$3jLb1{I)U)u6DS2j zI3zHZ30L8UTny(0WAIR*hMKYvF8HflaJoz>5(){trUp=O1|HW*;KSV___j>Hll#63 zA4p$-;qpP02*{3>n-Tc9rwjfO?GX;qNbJ~(&g_QA-7;9-;qY-XYgx#HM`CnxWhR5a zH$lY{u~3AGm~{QF>FL=>blQ42PQ}OwrLz!;G)0+b;NcFWSE1OGU8bpMEUClqH5Jlw zc4YZkr6O3a80GnWSrIMbNI~zeUSxaR)5p>JeAg5=26y}Gi|7?v+{vQ}6hUaKG{Q(_ z`=-(kw-k0s#v0a1Omc-oVCSowLMQYJEju&IFjB68H}TWmdL^6>s^DLtPMZ6f6<{xl zj-Wvna98=7o>-ikVV%3)V1lbvt!TijwIrObHGoHC#Wz}XkXLoWV@LmMKwUj%$f2y| zW{2a62t^ezg{zHDWo|k1BOjweCx}5+Bh5QP3gb2EQr3ITGWfoxo~JcZz7MVUAXi0$ z3NG&P@wPzSKm)8dHs)>O?CFLFwLLg9)D1p2GXmS_E1wcP&KnrBJmXnpJ72`GPweKH zza_56@2m|Ui5>8>cs2Wm7Ck54;-+z)_fnIoITxzc+>A}0y{T|ANhb*}=3zQ!flr$T zMY$i=Fm(d5Ca_BkNRO@1QxYv)mW<^CA4sKQNrnt3qN#~EbHt=`5pYw|45VE0w8F4T z37*)bbfl=vUgOkcN~}TNxr<}6CWHg>C2TfMUQD2f7u3?G^2JSMuPaJXIJ8-x4=q=X!h*_%JT9oZ@NR&6DihSJov=?Gf-~v~ zT)D4y;7veKL*QFXvkZ4NEm$}zMIy2gvk;4sWMp9or>QKBH6hyICv4a^3D_jY+1~kp z7)4>f)`Cx3+X)x72Dqu!!ac1Xs&z)N>kKfd)4{5a(aBP7I zeFwau_aV?{dbUYb7`&3K=eS`iK;agVFG|MH%|l)>^rOHpZSAn+u;kNv6=sYkU`z3R z<1x5xZUKL@p*rW%HMXaG*Qtav%^JAe?0}z|O$c*et%0+xQn=iDx}<)Q$)4BW!gYU^ z!3EPx$V662(Q^?d7R3!<$svXgb9I)80(>1ZIB5vn1 zcLZ5ON?LCRjgwW#dY~HKc1rX`USLaz&!uf%ey81$SJeUn(gdxf5r#<}^4;d`EwUB1 zNCo^xHWe6qs8H`V?_YOLz_7KF+l0cslFVM&D?#XMR(D|_D{P(Mw2>U_3EMnEHo)ID zt2S>2H+zZ^@y@yeBdVzQLY#3CHr8!y0B&{GVIElUR%w&zL^f`v0Hbz!0U&JIWf^b4 z?{-_sq~4Y~1nUU^a_NYyPCC zD3C^n0x3UR(mWBoquT?=GeKy+jA~t_%jmXC!+YdU*CaOT|L+b0QoeJRo=>MNAoa8% zJ~j#V9D}nP9zVxJ;q_!k5v+Nwne@Soz^-*jpoFiyDz;<&<#nM_l8&vLOk>7O`qe*;?OZQ{u554zyvpt=|;dj!gyKh6Cr{BLi65TB>|*+Y*~ zB;CcGDlzI`_jlu$`p^NG4YlPqn6se)-j_dwx?m|R0bAIIqdmXXWjB&&EQLqpOyP4j meiN?YWgmw%xTmwdkVUxD*dQ*nDI>-XL z+)=lPo<|-aceL|AR^YdJYkvK%Y%-n+6n(kEBk4>kHkyfZNsvy_R5CWnrl(?~Y@zlc zHk~1CG$?8i)6@YP&4?yMG`cC$1N~DJ1sX010DZYcTaD!~@C^5YY(W%lwPymcMNu;u$8%S|8vnG3hMj`P)gx`E{ zLH^M5ESH+ylhd{(nTRLZXe<-6nR+^QkHp5F7>_4dS1e(2Fv)BpVFxTg zu^4%gXS{;GbLt3sSl$3TbrnvaVa+(C1HT))G?SVYKER7tE81X{s6~!sl#)2nLG6Z_ zqf|T4E0hVc1@9|-5?l<7cF^b%6;dr_5AOkYLo`G(8=9$XB0?WkyFlERl@_26D&1tR zU+PE05(60@kn98whVhh`LiZ$on;;j$mDfHVPsS37Vhft<+Kj@A++rcbMUIlAYqU~u z)=zZn8k}>bRfUdJ)#jXI!Py2_k5XE6sFoqKCu?;mTphv|@HWHfmfS{o3zn0we(8Sl z{y^IIe^~ftP1D+~b5P6MGD7T_SO)!vGF+j*$r=bxCw~&YCgcam7ufq>TXeW?hhWVQ zbvB6*X2@KrLJR60iaux;`TRIY5>M(>ptlqj0_!8i4p=Estpy8r+JIJckd5G=PIvE7%ePVZ7ITE#%mGK|7v{(H1yf9KaX4FRS9l1mSM*n z%or2wf=-^Zz$c)`Z9eQbp_6vLp8o}^i%rc%sKBeLb&SLvp$-mWasM3L zuN)hTV1QxQJJi+V@fj!~)&+Nf7a|a_eNLQDN1P@S_+?SucPd~B4!p*tgHhgXvoPl4 zZILd5Mx%E&?xcpezIZxg1FUd(t5I_smV-z8+7v|Ht>(a@?2^e)z>O2+SvNi$c~z&~ z_*!JN%aF$7ZY)r>nhy?pYKnY7C*qUwOb6rPqX9`+>v1qCHl0mmTs>SinZZ~MULBHp zt3jo~Yh5)7GhTI(4s^+@%KH*Th*~_Wig>OiAc#2K5`=A1rxlg?YSvzjI6N188>$Mt z!~0!@*7(3gm*nIA@T$RkL;o`Mkglp)hZkOOE}3Xh}}+D2c))b?1c|?OoM? zciRnuC%)1eaLhHocXHG(HoMh^aKsN!ZLO%yO7e*4)-|5bs zu;lC^-G5M|MQ1v7eBb}1vx5kD+=+eP9`pg740aJ(k3J6K#(gLFG|GGnZ6)i! z?{boNWvH9HJ!LAyhN=alUJ9v|LX2?>;LpSWK3~s*vYBw9ETGc7`|?t zRMXu7{*d-i{iQfN9(VKF9N`XNkB9ZS%K#ROpM^^;{Du_VCoyXnH;}M{A$=rwxxF8b z5cwyhb`ZBWf}MLP;^%c_;3uGYZU0So=|>hifS3b z=-ee>4E~?MI1GmPAAX1w(Rg}_OS9qhY;x3Q=b3OVdNqn$%9*H*htgbQR~9)aCxeKo P+=L^BgE_LP0|5LD`>98z diff --git a/tests/ALCops.Mcp.Tests/ApplyFixAllToolTests.cs b/tests/ALCops.Mcp.Tests/ApplyFixAllToolTests.cs index 0946433..e856427 100644 --- a/tests/ALCops.Mcp.Tests/ApplyFixAllToolTests.cs +++ b/tests/ALCops.Mcp.Tests/ApplyFixAllToolTests.cs @@ -212,6 +212,35 @@ public async Task ApplyFixAll_DocumentScopeWithoutFilePath_ReturnsInvalid() Assert.Contains("filePath", root.GetProperty("message").GetString()); } + [Theory] + [InlineData("C:\\bad\0file.al")] + [InlineData("")] + public async Task ApplyFixAll_DocumentScopeMalformedFilePath_ReturnsInvalid(string filePath) + { + using var ctx = new TestContext(); + + var result = await ApplyFixAllTool.ApplyFixAll( + ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, + ctx.ProjectPath, "LC0020", scope: "document", filePath: filePath); + + var root = ToolResultAssert.Error(result, "Invalid"); + Assert.Contains("filePath", root.GetProperty("message").GetString()); + } + + [Fact] + public async Task ApplyFixAll_DocumentScopeFileOutsideProject_ReturnsNotFoundFileNotInProject() + { + using var ctx = new TestContext(); + var nope = Path.Combine(ctx.ProjectPath, "Nope.al"); + + var result = await ApplyFixAllTool.ApplyFixAll( + ctx.SessionManager, ctx.CodeFixRunner, ctx.AnalyzerResolver, ctx.Writer, + ctx.ProjectPath, "LC0020", scope: "document", filePath: nope); + + var root = ToolResultAssert.Error(result, "NotFound", "FileNotInProject"); + Assert.Equal(Path.GetFullPath(nope), root.GetProperty("filePath").GetString()); + } + [Fact] public async Task ApplyFixAll_NoDiagnosticsFound_ReturnsNotAppliedWithZeroCount() { diff --git a/tests/ALCops.Mcp.Tests/ApplyFixToolTests.cs b/tests/ALCops.Mcp.Tests/ApplyFixToolTests.cs index 296de97..09be44b 100644 --- a/tests/ALCops.Mcp.Tests/ApplyFixToolTests.cs +++ b/tests/ALCops.Mcp.Tests/ApplyFixToolTests.cs @@ -326,6 +326,31 @@ public async Task ApplyFix_FolderWithoutAppJson_ReturnsInvalid() } } + [Theory] + [InlineData("C:\\bad\0file.al")] + [InlineData("")] + public async Task ApplyFix_MalformedFilePath_ReturnsInvalid(string filePath) + { + var tempProjectPath = TestAnalyzers.CopyFixtureWithAnalyzers("ApplyFixProject", "alcops-applyfix-badfile-test"); + + try + { + using var sessionManager = new ProjectSessionManager(new ProjectLoader()); + var (analyzerResolver, _) = TestAnalyzers.CreateAnalyzerResolver(); + + var result = await ApplyFixTool.ApplyFix( + sessionManager, new CodeFixRunner(), analyzerResolver, new GuardedFileWriter(), + tempProjectPath, filePath, "LC0020", 11, 17, "any"); + + var root = ToolResultAssert.Error(result, "Invalid"); + Assert.Contains("filePath", root.GetProperty("message").GetString()); + } + finally + { + TestAnalyzers.TryDeleteDirectory(tempProjectPath); + } + } + /// Loads the project, finds the LC0020 fix at MyPage.al:11:17 and applies it through the tool. private static async Task<(ModelContextProtocol.Protocol.CallToolResult Result, string EquivalenceKey)> ApplyLc0020Async( string projectPath, string filePath, GuardedFileWriter writer) diff --git a/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs b/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs index daec51054e08fc54c96fcf26dbb964507f402e4c..bbed6f6d0d466bc64308c6b0182079f38661f23d 100644 GIT binary patch delta 1924 zcma)7O>7%g5LTST_HL8-Z~e3WU2ojfZW6neNJDEUq@+nnLy{_qXh_^t+jyNAHn!z; zDx%;Ur@#UCs|1%KDpf*7aOe#X5*H2#2~|`Ty;O*vK}Zo2;#S_Sy=$jGk)O2d*>C2} zH}l@i`1b6N3m^Bh^p2b_Y;xz`oGg~)_}oS*K3B|&lANlI`#jGpmy1P3cXzP%8C|8daielcsBFbjx@~<| zl~#eZX=q-uF8Mfsoq@vJyDKNEALo}pfQIYq)MuO;T+{ajq>6vYK%N1 z2+q5$(EcNHoUyO2o*|1L;*v@7j1o^I=@_gwAHF=Mr7_+f zpBw8v;UoFdaP|7hDSq!9M#$EKLa(De*DN0PCXaNzWF+yohX1sWRZo7{a-J~WqgZ@o zIoTeY8zyU_mF=o^@NjXY@xU6;6XDx+d&4%;bUW4RlCv+MWY0BfSP|AVyJcr_n8wK;kV2dc<12Dy6wOi$4%!6ay3es|LZIgDS~|$L%wdi-ok&b z5o6|>kjy(ZO+px&1yYVtIJzrvAlal#nxsDQobOb7b-%OpFEqX)8+D# z7#Acy_5vdUs=d6=^qia~c1r8d$a8{}E0(v#Y*en4mD#DdP^W@QvxgV-d&++qA@&kU z_P`j$)6Twb)fSen*Zr!bbf~sk1`Tcn{hiISv(Phz>w=We@@j`O7`D)6jUH2$q4q9r zO}n(0M~V`=L}GPdHjpM!?+0GRyw?dK$l(Jok1xFI1l4BOA3+AEebM&FS={p-s{Ut2 zW8->%+MTJsH+EpfFeY7>_Vso@1 z$(3=38^W(#82@k)c*2X?DRKQQJ`87|{8o7{eEb^?c>%xhi$s=jv;S@U-G5pCA6kf9 cz$cM0&Bf!$5()BggvIRfizAL(e|JRMuq7r}EwGqXJx zGBd}VsSb^qPRwZ?B-_a0U?sDlR0HuVG?2E@*n||1@0}p#hnBO5s0FFuTpARRq%CtnLvRX{qG~^cDcKFJOBq7hnq9ch8X?!FY z#7(5J+ie~JTqK8AgDJGL-oHy;_GCiD=sb{W7qB^2MeC}w7VeqNxy`Zb+u0OT;;w8S zZs*iuqG_Z6CX;v|elxr@xfT8eOC30fJx!izQgt96KT|GL(nTYX66!~yu51q8nwwNl z$gN0|{9$f*H@OJka?Ad9yv$HxGr*|Dsfz7Po67_NYj7{$tkt)d@2Qo3_q%me)wId5 zFl(zxZLqBmQSHFfj2LLOd(@21z^a{1D=RM$&ZT7(;;!QK6|U?bcRfW&yS@u-^Cx0y% zg&>ku2_dwS(5NJ_?iS#UkUHQr9S)9(L1jb|$1pCHBt>n&6suLs+A$N4mr9e9@wpHd z16l)6LcX@fdx{TSD^5_60UoxBX$54xBHF`VAKDIYx27lJcft$r0K(K;^YPH(JG;}P v=k}+M{$V2iB-&U0%V^z!-rYZz8-W%CHv$dXR}CdSilDxdVf8BDUjUz9ncu3? diff --git a/tests/ALCops.Mcp.Tests/ListRulesToolTests.cs b/tests/ALCops.Mcp.Tests/ListRulesToolTests.cs index 9185bca741119c8a90fb1ae9ce46d47b340c6f3b..71fa0425056f069bf2bbde4cc4a38e80a2a8b51f 100644 GIT binary patch delta 835 zcmbQKJzHynlmu64ab{k+f}@XfenGLGZ*qZNNPd1!vGqg~Lv5&BL}^}Ti8U8jUSe)) zaY15oDvIjV;*w%(u8C8tCJQi{GI3Q;_GGMQ= zCx2lwo2<>8Fu8|Wj~y&k%Qg83a{*9k48LD~Kv8~HYH~@jhNgn8-Q((^AgNlQAz&KKS_^DemH;h=S;)y*HTfi` z%;W`}8bI29GAEav5LT1KCoAxCPOjzBKr?aj8!j)fN5Ush;ARJ#Y&ZEPw*%M<#yr+Q zT5579j|^^86(@h@QAL<)JUN8d8f;=RG-x2^Rq;X0y1=K5+bm5abHHZ6g4Yi0jthKJ zlidY0lTmyal98HUR2d5vi}uXR$;?Z2Ni0d!P;$14iAhRKi7_ZhEXh#P#G(Qig~rLf{xM;c`nX%_&JuQPQ-Y{81!+a-gUUC)CrmTmUmJn#=x`LyRbACawo^Nu2UPyj^PO&xbL?c5%m~2F8US^3kFBk8` zLK$Wd)mO#HJDG#gl!ce8ns>52V?9W8@_t5NFfGRtGdYtUqp$1|%h!6M$qTvmO^jNi9hCI_3hPpd3p!knChh0%F*t!N*FED+Y)c~mVH>)R*bYW8fk|}IKKyn+KJP*VusB`YKft+%mO`AVCzd%Vx zA-}XlAzo9{ns;(AyQ4BfX)RC)>;#beFx+yIT@lkQ3k1U_&*J3~(NQQ%ElSESPPI}f zDJo3`8N@po;=n5$puoGxDKj~XQv=M>nY@ov7|gO0!)c)hie*66Yq=oa23hIP%?=F$ z)ye00C4sWfc^!b{RqhZVsmWu8%iKbqSY&h6Cd=~k0(G$ST7%7lc!v+G+3I|tP~5_A zBA=UDT%4E=_9Q5TF+yu2p96*^U?UMR2Xpoo{@5V&APC7w%`d8qg~otqUQT9Ss!L)? zqK1;QRZLQ13PV9+NrsXpHU&V}fFvfz3y1>Us>r20dAb0W6vQT&36#zkGz5|h1XIww z>ns$BEBHWZ@~9A!iC=}xz`AvWK_*TWjs=nptR|DyM1Dh Hr~wfGidoee From 9ded32328699488df57ecb94d57c4b83dd5c3f0e Mon Sep 17 00:00:00 2001 From: Arthur van de Vondervoort Date: Sun, 4 Oct 2026 10:53:17 +0200 Subject: [PATCH 4/4] fix: address review round 3 (apply_fix_all reports NoAnalyzerForRule instead of zero occurrences) apply_fix_all checked for a fix provider and for ruleset suppression up front but not for a loaded analyzer that reports the rule, so a rule whose analyzer is not configured came back as a success with zero occurrences while get_fixes and apply_fix report NotFound/NoAnalyzerForRule. The three tools now agree. README and the tool description name the second exception. Co-Authored-By: Claude Fable 5.1 --- README.md | 2 +- src/ALCops.Mcp/Services/CodeFixRunner.cs | 5 +++++ src/ALCops.Mcp/Tools/ApplyFixAllTool.cs | 2 +- tests/ALCops.Mcp.Tests/GetFixesToolTests.cs | 10 +++++++++- 4 files changed, 16 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 38424ef..e55adf5 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ The native tools (`list_rules`, `get_fixes`, `apply_fix`, `apply_fix_all`) work | `list_rules` | List analyzer rules with metadata (ID, title, severity, category, cop). | | `get_fixes` | Get available code fixes for a specific diagnostic at a location. Returns `{ diagnosticId, filePath, line, column, fixes: [{ equivalenceKey, title, providerName }] }`; when nothing matches, the error `NotFound` with a `reason` (see [Errors](#errors)). | | `apply_fix` | Apply a code fix to resolve a diagnostic. Writes the fixed content to disk unless the file changed after the fix was computed (error `Stale`); a file that cannot be read, decoded or written is `Faulted` with `reason` `ReadFailed`, `UnsupportedEncoding` or `WriteFailed`. The write is atomic (sibling temp file renamed over the target) and keeps the file's encoding, BOM and line endings. | -| `apply_fix_all` | Apply a code fix to every occurrence of a diagnostic rule across a project or a single file (like VS Code's "Fix all in workspace"). Writes to disk unless `dryRun` is set. A rule with several distinct fixes and no `equivalenceKey` is the error `Ambiguous` with `candidates`. Zero occurrences is a success (`applied: false`, `diagnosticsFound: 0`), except a rule the project ruleset suppresses, which is `NotFound` with `reason: SuppressedByRuleset`. Files that changed on disk mid-operation are skipped and listed in `conflicts`; the rest are written as one batch — if any write fails, every file already written in that call is restored and nothing from the batch is kept. A batch that fails and is rolled back is a normal (non-`isError`) result with `applied: false`, the failure in `message` and every staged file in `conflicts`, because the per-file outcome is in the body; only argument, lookup and single-file write failures use the error envelope. | +| `apply_fix_all` | Apply a code fix to every occurrence of a diagnostic rule across a project or a single file (like VS Code's "Fix all in workspace"). Writes to disk unless `dryRun` is set. A rule with several distinct fixes and no `equivalenceKey` is the error `Ambiguous` with `candidates`. Zero occurrences is a success (`applied: false`, `diagnosticsFound: 0`), except a rule the project ruleset suppresses (`NotFound` with `reason: SuppressedByRuleset`) or one that no loaded analyzer reports (`NotFound` with `reason: NoAnalyzerForRule`). Files that changed on disk mid-operation are skipped and listed in `conflicts`; the rest are written as one batch — if any write fails, every file already written in that call is restored and nothing from the batch is kept. A batch that fails and is rolled back is a normal (non-`isError`) result with `applied: false`, the failure in `message` and every staged file in `conflicts`, because the per-file outcome is in the body; only argument, lookup and single-file write failures use the error envelope. | | `analyze` | Compile with all configured analyzers and return structured cop + compiler diagnostics (analyzer, hasFix, filters, summary). Wraps `al_compile` with `onlyErrors: false`; needs `almcp`. | ### Proxied from Microsoft's `almcp` diff --git a/src/ALCops.Mcp/Services/CodeFixRunner.cs b/src/ALCops.Mcp/Services/CodeFixRunner.cs index 2beb963..e62ff71 100644 --- a/src/ALCops.Mcp/Services/CodeFixRunner.cs +++ b/src/ALCops.Mcp/Services/CodeFixRunner.cs @@ -206,6 +206,11 @@ public async Task ApplyFixAllAsync( if (IsRulesetSuppressed(analyzerProvider, diagnosticId)) return NotFound(diagnosticId, FixNotFoundReason.SuppressedByRuleset); + // Same for a rule no loaded analyzer reports: get_fixes and apply_fix say NoAnalyzerForRule, and + // "zero occurrences" would wrongly tell the caller the code is clean. + if (!analyzerProvider.GetAllAnalyzers().Any(a => a.SupportedDiagnostics.Any(d => d.Id == diagnosticId))) + return NotFound(diagnosticId, FixNotFoundReason.NoAnalyzerForRule); + var normalizedFilePath = filePath is null ? null : Path.GetFullPath(filePath); if (scope == FixAllScope.Document && session.GetDocument(normalizedFilePath!) is null) diff --git a/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs b/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs index 367b825..4daa39b 100644 --- a/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs +++ b/src/ALCops.Mcp/Tools/ApplyFixAllTool.cs @@ -16,7 +16,7 @@ public sealed class ApplyFixAllTool "Writes changed files directly to disk unless dryRun is true. " + "If the rule offers more than one distinct fix and no equivalenceKey is given, returns the error Ambiguous whose candidates " + "list each fix's equivalenceKey, title and providerName; pass one equivalenceKey verbatim. " + - "Zero occurrences is a success (applied: false, diagnosticsFound: 0), except a rule the project ruleset suppresses, which is NotFound with reason SuppressedByRuleset. " + + "Zero occurrences is a success (applied: false, diagnosticsFound: 0), except a rule the project ruleset suppresses (NotFound with reason SuppressedByRuleset) or one no loaded analyzer reports (NotFound with reason NoAnalyzerForRule). " + "Changed project files are re-read from disk first. Files that change on disk while the fix is being computed " + "(or that cannot be read, or are not valid in their detected encoding) " + "are left untouched and listed in 'conflicts' (each with a 'kind') and their diagnostics remain in 'unfixedDiagnostics' (positions as analysed, so they may have shifted if the file was edited); the other files are still written. " + diff --git a/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs b/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs index bbed6f6..cef1139 100644 --- a/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs +++ b/tests/ALCops.Mcp.Tests/GetFixesToolTests.cs @@ -188,10 +188,18 @@ await WithFixtureAsync("ApplyFixProject", async projectPath => var real = await analyzerResolver.ResolveAsync(projectPath, null); var provider = new DecoratedProvider(real, hideAnalyzers: true); - var lookup = await new CodeFixRunner().GetFixesAsync( + var runner = new CodeFixRunner(); + var lookup = await runner.GetFixesAsync( session, Path.Combine(projectPath, "MyPage.al"), "LC0020", 11, 17, provider); Assert.Equal(FixNotFoundReason.NoAnalyzerForRule, lookup.NotFoundReason); + + // apply_fix_all agrees instead of reporting zero occurrences (which would read as "clean"). + var fixAll = await runner.ApplyFixAllAsync( + session, "LC0020", FixAllScope.Project, null, null, provider); + + Assert.Equal(FixAllStatus.NotFound, fixAll.Status); + Assert.Equal(FixNotFoundReason.NoAnalyzerForRule, fixAll.NotFoundReason); return 0; }); }