Revert VS Code to canonical LSP file-watching, closing #31 (Q9)

CHANGELOG.md

@@ -7,6 +7,7 @@
 * Run CodeLens now shows the scenario's last-run pass/fail glyph, matching VS's own test CodeLens (VS) - see #504
 * Run, hook-match-count, and step-hooks CodeLenses on a `Scenario:` line now appear in a deterministic order (Run, then hook count, then step-hooks) instead of an unspecified tie (VS) - see #504
 * Go to Step Definition's ambiguous-match picker now shows the target method's source line instead of a method name/step-type label, built from the standard `textDocument/definition` response instead of a Reqnroll-specific message (VS Code) - see #126
+* Post-build binding rediscovery now relies solely on the server's standard LSP dynamic file-watch registration instead of a redundant client-side watcher, after confirming the canonical path reliably detects real `dotnet build`s on its own (VS Code) - see #31
 
 ## Bug fixes:
 

docs/LSP-IDE-Support-Feature-Designs.md

@@ -265,7 +265,7 @@ Both discovery paths are managed by the LSP server, so no IDE-specific code is r
 | Server (internal) | IPC to Connector | Launch reflection discovery, receive `BindingDiscoveryResult` |
 | Server → Client | `textDocument/publishDiagnostics` | Push updated diagnostics after registry change |
 
-> **Open question (Q9)**: How does the LSP server reliably detect that the solution has been rebuilt? Watching the output assembly path via `workspace/didChangeWatchedFiles` is the current assumption, but this needs verification per-IDE. See [Open Questions & Risk Register](LSP-IDE-Support-Open-Questions.md).
+> **Resolved (Q9)**: watching the output assembly path via `workspace/didChangeWatchedFiles` is confirmed reliable per-IDE using each client's standard dynamic-registration handling — no IDE-specific client code is needed. See [Open Questions & Risk Register](LSP-IDE-Support-Open-Questions.md) (Q9) for the verification details and the one confirmed caveat (`files.watcherExclude` covering `bin/`).
 
 #### Sequence diagram
 
@@ -308,7 +308,7 @@ The Roslyn (source-level) path is **implemented**. `TextDocumentSyncHandler` is
 
 **Behavioural nuance**: a step renders as *unbound* (a `reqnroll.undefined_step` token / "step definition not found" diagnostic) only once the owning project has a **valid** (non-`Invalid`) registry — i.e. after any discovery has completed, whether the startup reflection run **or** the first Roslyn `.cs` open. Against an `Invalid` registry (no discovery yet) the tag parser skips step matching, leaving steps unclassified rather than unbound.
 
-The reflection (post-build) trigger shown in the lower half of the diagram is also implemented: `WatchedFilesHandler` registers `workspace/didChangeWatchedFiles` watchers for `**/bin/**/*.dll` (and `**/reqnroll.json`) and calls `ConnectorBindingRegistryProvider.TriggerRefresh()` for the project whose output path matches. An initial run is likewise triggered on `reqnroll/projectLoaded`. Whether each IDE reliably *delivers* those watched-file events on build remains [Q9](LSP-IDE-Support-Open-Questions.md).
+The reflection (post-build) trigger shown in the lower half of the diagram is also implemented: `WatchedFilesHandler` registers `workspace/didChangeWatchedFiles` watchers for `**/bin/**/*.dll` (and `**/reqnroll.json`) and calls `ConnectorBindingRegistryProvider.TriggerRefresh()` for the project whose output path matches. An initial run is likewise triggered on `reqnroll/projectLoaded`. VS Code was confirmed to reliably deliver those watched-file events on build via its standard LSP client with no IDE-specific glue ([Q9](LSP-IDE-Support-Open-Questions.md), resolved for VS Code); VS doesn't need this signal at all, since `VsProjectEventMonitor` hooks `DTE.Events.BuildEvents.OnBuildDone` directly; Rider was not part of that verification.
 
 > **Planned change — index-driven, multi-project routing.** As built, `CSharpBindingDiscoveryService` routes a `.cs` edit to a **single** owning project via `ILspWorkspaceScopeManager.GetProjectForUri` (longest folder-prefix match). Under the [membership-index design](LSP-IDE-Support-Architecture.md#project-membership-the-path--projects-index) this becomes a lookup returning the **set** of owning projects, and the per-file Roslyn patch fans out to *each* of their registries (a linked `.cs` legitimately belongs to several projects, so one edit invalidates several registries). The same lookup **gates** the patch: a `.cs` that no project's index claims — e.g. one excluded from its `.csproj` but opened in the editor — contributes bindings to **no** registry, preventing phantom bindings that would otherwise be wiped on the next build. The folder-prefix match is retained only as the fallback for projects that have not (yet) sent a `reqnroll/projectFiles` baseline.
 

docs/LSP-IDE-Support-Open-Questions.md

@@ -32,7 +32,7 @@
 | Q6 | Is VS.Extensibility Code Lens support planned for a future VS version, which would remove the VSSDK dependency for F18? | TBD | Monitor VS roadmap |
 | Q7 | Is it worth standardizing on a single LSP transport (e.g., stdio) across all three IDE clients, rather than using named pipe for Visual Studio? | — | **Resolved**: all three clients use stdio. |
 | Q8 | Should the `Reqnroll.IdeSupport.Common` assemblies be referenced directly by IDE clients (enabling client-side telemetry and logging for installation/upgrade events) rather than having all telemetry flow through the LSP server? | TBD | Open |
-| Q9 | How does the LSP server reliably detect that the solution has been rebuilt across all three IDEs? Watching the output assembly path via `workspace/didChangeWatchedFiles` is the current assumption. | TBD | Needs testing |
+| Q9 | How does the LSP server reliably detect that the solution has been rebuilt across all three IDEs? Watching the output assembly path via `workspace/didChangeWatchedFiles` is the current assumption. | — | **Resolved for VS Code and VS**; Rider not yet verified. VS Code was the outstanding case — [PR #26](https://github.com/reqnroll/Reqnroll.IdeSupport/pull/26) had added a client-side per-project watcher on the (untested) assumption that `vscode-languageclient`'s generic `FileSystemWatcherFeature` wasn't reliably delivering these events. Issue #31 tested that directly: an Extension Host recreation of #26's original symptom recovered correctly with the client-side glue *disabled*, and two independent real `dotnet build`s in a live manual VS Code session (one incremental, one full/clean with 150+ dependency-DLL noise) were both correctly detected and filtered using only the canonical `WatchedFilesHandler` dynamic registration + `vscode-languageclient`'s own watcher, no client-specific glue. The one confirmed caveat: a `files.watcherExclude` setting covering `bin/` breaks *any* `createFileSystemWatcher`-based approach equally (canonical or client-side) — not specific to this fix, and not addressed by it. VS Code's client-side glue was removed accordingly. VS never needed this signal at all (`VsProjectEventMonitor` hooks `DTE.Events.BuildEvents.OnBuildDone` directly). Rider's behavior was out of scope for this pass and remains open — tracked in [#511](https://github.com/reqnroll/Reqnroll.IdeSupport/issues/511). |
 | Q10 | Should the VisualStudio.* projects be nested under the `clients/` folder alongside the VS Code and Rider clients, or remain in `src/`? | TBD | Open |
 | Q11 | Which telemetry architecture should be used? Three options: (a) **Direct HTTP from LSP server** (via `Reqnroll.IdeSupport.Common`) — centralized, but misses pre-server events; (b) **Direct HTTP from each IDE client** — captures installation events, but requires telemetry code in three clients; (c) **LSP `telemetry/event` notification** (server → client) — server fires events, client relays to HTTP endpoint — best of both but requires all three clients to handle the notification. See [Architecture §9 Telemetry](LSP-IDE-Support-Architecture.md#telemetry). | — | **Resolved (as-built)**: option (c). The server emits `telemetry/event`; each IDE host owns the concrete transmitter (`TelemetryTransmitter` in VS's `VSSDKIntegration`, `telemetry.ts`/`TelemetryReporter` in VS Code — both point at the same Application Insights resource). See archived `build-plan-telemetry-capture.md` and `plan-refactor-analytics-appinsights.md` in `docs/Archive/`. |
 | Q12 | Should we plan for debug support for feature files (breakpoints, step-into, etc.) in a future phase? | TBD | Open |

src/VSCode/CONTRIBUTING.md

@@ -151,6 +151,17 @@ Unlike the Visual Studio extension, VS Code doesn't spawn the server with `--tra
 Changing `reqnroll.trace.server` requires a window reload to take effect on the already-running
 server (the `--log-level` it maps to is fixed at process launch).
 
+**The Output panel can appear empty even with tracing on.** `reqnroll.trace.server: verbose`
+correctly drives `vscode-languageclient` to trace (`InitializeParams.Trace`/`$/setTrace` as
+above), but the **Reqnroll LSP Trace** channel is a `vscode.LogOutputChannel`, which has its own
+independent display-level filter — set only by the user, via that channel's own dropdown in the
+Output panel (or Command Palette → "Developer: Set Log Level…" → pick the channel). Nothing in
+`reqnroll.trace.server`, or anywhere else in the extension, can raise that filter programmatically,
+so a channel left at its default level will silently show nothing even while tracing is fully
+active. If the panel looks empty, check the timestamped file log under `%LOCALAPPDATA%\Reqnroll\`
+(or the platform equivalent above) instead — it's written directly to disk and isn't subject to
+this filter, so it's the more reliable place to look.
+
 ## CI
 
 The GitHub Actions workflow [`.github/workflows/ci.yml`](../../.github/workflows/ci.yml) runs its VS Code jobs (`build-vscode-extension`, `tsc-only`) whenever a push or PR touches VS Code, Core, or LSP paths. It:

src/VSCode/src/lsp/projectManager.ts

@@ -1,10 +1,6 @@
 import * as path from 'path';
 import * as vscode from 'vscode';
-import {
-  DidChangeWatchedFilesNotification,
-  FileChangeType,
-  LanguageClient,
-} from 'vscode-languageclient/node';
+import { LanguageClient } from 'vscode-languageclient/node';
 import { evaluateProject, ProjectFileItem, ProjectProperties } from './msbuildEvaluator';
 import { ReqnrollMethods } from './lspMethods';
 
@@ -89,30 +85,35 @@ export function findOwningProjectFile(
  *     own `workspace/didChangeWorkspaceFolders` LSP notification is already sent automatically
  *     by vscode-languageclient's WorkspaceFoldersFeature; this only covers *our* project
  *     discovery, which the library has no knowledge of).
- * v5: forwards output-assembly (re)build events to the server. Connector-based binding
- *     discovery (`ConnectorBindingRegistryProvider`, server-side) reflects over the project's
- *     `OutputAssemblyPath` DLL; if that DLL doesn't exist yet when the initial
- *     `reqnroll/projectLoaded` baseline is sent (e.g. a freshly cloned repo opened before its
- *     first `dotnet build`), discovery fails once with "Output assembly not found". The server
- *     already declares a standard `workspace/didChangeWatchedFiles` registration for
- *     `**\/bin/**\/*.dll` (`WatchedFilesHandler.cs`) specifically to retry discovery once the
- *     assembly appears — but whether each IDE's LSP client actually *delivers* those dynamically
- *     registered watched-file events reliably is an open question (Q9 in
- *     docs/LSP-IDE-Support-Open-Questions.md); VS Code's `files.watcherExclude` commonly excludes
- *     `bin/`/`obj/` from the file watching a dynamically-registered `FileSystemWatcherFeature`
- *     relies on. Rather than resending the full `reqnroll/projectLoaded` + baseline (which re-runs
- *     `dotnet msbuild` and duplicates work the server can already do with the `OutputAssemblyPath`
- *     it was given at initial registration — that path is computed from MSBuild properties and is
- *     correct even before the file exists), this watcher sends the *same standard*
- *     `workspace/didChangeWatchedFiles` notification directly, landing on the server's existing
- *     handler with no extra round trip. VS's `VsProjectEventMonitor` doesn't need this fallback —
- *     it hooks `DTE.Events.BuildEvents.OnBuildDone` directly.
+ * v5 (added, then reverted — see below): forwarded output-assembly (re)build events to the server
+ *     as a client-side `RelativePattern(outputDir, '*.dll')` watcher per project, sending a
+ *     synthetic `workspace/didChangeWatchedFiles` notification directly. This was added by PR #26
+ *     to fix issue #2 (a project registered before its first `dotnet build` — DLL missing —
+ *     permanently failed Connector-based binding discovery with no retry), on the assumption that
+ *     the server's own standard dynamic registration for `**\/bin/**\/*.dll`
+ *     (`WatchedFilesHandler.cs`) wasn't reliably delivered by `vscode-languageclient`'s generic
+ *     `FileSystemWatcherFeature` — specifically that VS Code's `files.watcherExclude` (which many
+ *     users add for `bin/`/`obj/`) suppresses it.
+ *
+ *     Issue #31 (Q9) investigated that assumption directly. Two findings: (1) `files.watcherExclude`
+ *     covering `bin/` does suppress *any* `createFileSystemWatcher`-based approach, including this
+ *     client-side glue — narrowing the glob doesn't route around it, so the glue provided no
+ *     resilience against the one confirmed failure mode. (2) Issue #2's original symptom does not
+ *     reproduce today: an Extension-Host recreation (a real, unbuilt project registered before its
+ *     first build, then built) recovered correctly with this client-side glue *disabled* — the
+ *     canonical path (server dynamic registration + `vscode-languageclient`'s own watcher) is
+ *     sufficient on its own under default settings. Confirmed again in live manual testing (real
+ *     `dotnet build`s against a real multi-project solution, one incremental and one full/clean
+ *     rebuild producing 150+ dependency-DLL noise) — both correctly detected and filtered by the
+ *     server using canonical registration alone, per the server's own file log
+ *     (`HandleOutputAssemblyChange: ... triggering discovery for '<project>'`). This class was
+ *     reverted to that canonical path; VS's `VsProjectEventMonitor` doesn't need any of this — it
+ *     hooks `DTE.Events.BuildEvents.OnBuildDone` directly.
  */
 export class ProjectManager {
   private readonly _client: LanguageClient;
   private readonly _watcher: vscode.FileSystemWatcher;
   private readonly _fileWatcher: vscode.FileSystemWatcher;
-  private readonly _outputWatchers = new Map<string, vscode.FileSystemWatcher>();
   private readonly _knownProjects = new Set<string>();
   private readonly _resendTimers = new Map<string, ReturnType<typeof setTimeout>>();
   private _disposables: vscode.Disposable[] = [];
@@ -154,8 +155,6 @@ export class ProjectManager {
   dispose(): void {
     this._watcher.dispose();
     this._fileWatcher.dispose();
-    for (const watcher of this._outputWatchers.values()) watcher.dispose();
-    this._outputWatchers.clear();
     for (const timer of this._resendTimers.values()) clearTimeout(timer);
     this._resendTimers.clear();
     for (const d of this._disposables) d.dispose();
@@ -239,46 +238,12 @@ export class ProjectManager {
   /**
    * Re-runs MSBuild evaluation for an already-registered project and resends both
    * `reqnroll/projectLoaded` and its `reqnroll/projectFiles` baseline. Used for `.cs`/`.feature`
-   * additions/removals, where the file *membership* itself may have changed — not for output
-   * assembly rebuilds (see {@link notifyOutputAssemblyChanged}), which don't need a fresh MSBuild
-   * evaluation since `OutputAssemblyPath` doesn't change just because the DLL was rebuilt.
+   * additions/removals, where the file *membership* itself may have changed.
    */
   private async resendProjectFiles(projectFile: string): Promise<void> {
     const { props } = await this.sendProjectLoaded(projectFile);
     if (!props) return; // msbuild unavailable — index stays Pending, same as v1 fallback
     await this.sendProjectFilesBaseline(projectFile, props.targetFrameworkMoniker, props.files);
-
-    // v5: arm the output-assembly watcher if this resend is what first discovered
-    // outputAssemblyPath (e.g. initial registration ran before `dotnet restore`). Guarded so a
-    // project resent more than once (this path isn't one-shot like registerProject) doesn't leak
-    // a duplicate watcher.
-    if (props.outputAssemblyPath && !this._outputWatchers.has(projectFile)) {
-      this.watchProjectOutputPath(projectFile, props.outputAssemblyPath);
-    }
-  }
-
-  /**
-   * Forwards a `bin/**` DLL create/change event to the server as a standard
-   * `workspace/didChangeWatchedFiles` notification (v5, see class doc). No-ops for assemblies
-   * that don't belong to a known project (dependency DLLs, other tools' output). Deliberately
-   * does *not* re-run MSBuild or resend `reqnroll/projectLoaded`/`reqnroll/projectFiles` — the
-   * server's `WatchedFilesHandler` already has the project's `OutputAssemblyPath` from its
-   * original registration (computed from MSBuild properties, valid whether or not the file
-   * exists yet) and can retry discovery from just the URI + change type.
-   */
-  private notifyOutputAssemblyChanged(uri: vscode.Uri, changeType: FileChangeType): void {
-    if (!findOwningProjectFile(uri.fsPath, this._knownProjects)) return;
-
-    void this._client
-      .sendNotification(DidChangeWatchedFilesNotification.type, {
-        changes: [{ uri: uri.toString(), type: changeType }],
-      })
-      .catch((err: unknown) => {
-        console.error(
-          `ProjectManager: failed to notify output assembly change for ${uri.fsPath}:`,
-          err,
-        );
-      });
   }
 
   // ── Notification sending ──────────────────────────────────────────────
@@ -311,31 +276,9 @@ export class ProjectManager {
         result.props.targetFrameworkMoniker,
         result.props.files,
       );
-
-      // v5: Forward output-assembly build events to the server as a standard
-      // workspace/didChangeWatchedFiles notification. Instead of a workspace-wide
-      // **/bin/**/*.dll glob that fires on every project's bin/ output (including
-      // unrelated dependency DLLs), watch only this project's output directory.
-      if (result.props.outputAssemblyPath) {
-        this.watchProjectOutputPath(projectFile, result.props.outputAssemblyPath);
-      }
     }
   }
 
-  private watchProjectOutputPath(projectFile: string, outputAssemblyPath: string): void {
-    const outputDir = path.dirname(outputAssemblyPath);
-
-    // Use a RelativePattern scoped to the project's output directory instead of a
-    // workspace-wide **/bin/**/*.dll glob that fires on every project's DLL output.
-    const watcher = vscode.workspace.createFileSystemWatcher(
-      new vscode.RelativePattern(outputDir, '*.dll'),
-    );
-    watcher.onDidCreate((uri) => this.notifyOutputAssemblyChanged(uri, FileChangeType.Created));
-    watcher.onDidChange((uri) => this.notifyOutputAssemblyChanged(uri, FileChangeType.Changed));
-
-    this._outputWatchers.set(projectFile, watcher);
-  }
-
   /**
    * Evaluates `projectFile` via MSBuild and sends `reqnroll/projectLoaded` with the result
    * (empty fields when msbuild is unavailable — v1 compat, file stays folder-prefix `Pending`).
@@ -413,13 +356,6 @@ export class ProjectManager {
     const projectFile = uri.fsPath;
     if (!this._knownProjects.has(projectFile)) return;
 
-    // Dispose the scoped output-assembly watcher for this project
-    const watcher = this._outputWatchers.get(projectFile);
-    if (watcher) {
-      watcher.dispose();
-      this._outputWatchers.delete(projectFile);
-    }
-
     const params = { projectFile };
 
     try {

src/VSCode/src/test/extension.test.ts

@@ -5,6 +5,8 @@ import { ReqnrollExtensionApi } from '../extension';
 
 // Pull in all additional test suites so the single entry-point loads them all
 import './lsp/projectManager.test';
+import './lsp/watcherExclude.test';
+import './lsp/defineStepRecovery.test';
 import './lsp/lspInspectorLogger.test';
 import './lsp/msbuildEvaluator.test';
 import './resolveServerPath.test';