diff --git a/docs/lsp.md b/docs/lsp.md index 90cf0b0a..34630d9b 100644 --- a/docs/lsp.md +++ b/docs/lsp.md @@ -208,6 +208,49 @@ Call `unsubscribe_diagnostics` to opt back out (idempotent). Pair with `get_code_actions` + `apply_code_action` + `fix_all_in_file` for the full edit-time diagnostic loop without polling. +## Language-specific behaviour + +A few servers need per-language handling beyond the generic router. These +knobs are **environment variables** read by the daemon process — set them +where you launch `gortex daemon` / `gortex server`. There is no `.gortex.yaml` +key for them today. + +### C# — NuGet audit advisories (`omnisharp` / `csharp-ls`) + +The Roslyn `MSBuildWorkspace` these servers build escalates a NuGet audit +*warning* (the `NU19xx` family — e.g. a transitively vulnerable package) to a +**fatal** project-load failure and drops every project that references the +flagged package. Those projects then have no compilation, so the server emits +false `CS####` "unresolved type / namespace" diagnostics — even though +`dotnet build` / `dotnet test` keep `NU19xx` a non-fatal warning and succeed. + +Gortex applies two complementary, C#-scoped fixes, **both on by default**: + +| Env var | Default | Effect | +| --- | --- | --- | +| `GORTEX_LSP_CSHARP_RESTORE` | on | Before spawning the C# server, run `dotnet restore -p:NuGetAudit=false` in the workspace so the MSBuild workspace loads every project (root-cause fix). Best-effort: a failure logs and falls through to a normal spawn; skipped on passive IDE attach and when `dotnet` is not on `PATH`. | +| `GORTEX_LSP_CSHARP_DIAG_FILTER` | on | Strip diagnostics whose code is the `NU####` NuGet family from `publishDiagnostics` before storing / fanning out (symptom fix). Deliberately narrow — real `CS####` compiler diagnostics always pass through. | + +Set either to a falsey value (`0` / `off` / `false` / `none`) to disable it — +e.g. `GORTEX_LSP_CSHARP_RESTORE=0` for offline / air-gapped indexing or to +keep indexing off the network. Restore is on by default because gortex only +indexes repositories you explicitly add (it never auto-discovers), and +spawning the C# server already evaluates the project's MSBuild graph — so the +restore adds no execution surface beyond the workspace load it precedes. A +successful restore logs `lsp: csharp pre-restore complete (NuGetAudit +suppressed)`; a failure logs `lsp: csharp pre-restore failed; spawning server +anyway` with the restore output tail. + +### Java — build-backed resolution (`jdtls`) + +By default `jdtls` runs in a **no-build** mode (JRE-only classpath, Maven / +Gradle import and autobuild disabled) so indexing an untrusted Java repo never +runs its build. Resolution is more limited in this mode (jdtls falls back to +an "invisible project"). Set `GORTEX_LSP_JDTLS_TRUST_BUILD=1` (or `true`) to +allow full Maven / Gradle import + autobuild for higher-fidelity resolution — +**opt-in**, because it executes the repository's build tooling. Enable it only +for repositories you trust. + ## Troubleshooting - **`no_lsp_for` error:** the file extension didn't match any diff --git a/internal/semantic/lsp/csharp_diag_test.go b/internal/semantic/lsp/csharp_diag_test.go new file mode 100644 index 00000000..2f9d014f --- /dev/null +++ b/internal/semantic/lsp/csharp_diag_test.go @@ -0,0 +1,127 @@ +package lsp + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestIsNuGetAdvisoryCode pins the advisory-code predicate: it must match the +// NU#### NuGet family (the audit/restore advisories we filter) and must NOT +// match real CS#### compiler codes — filtering those would hide genuine errors. +func TestIsNuGetAdvisoryCode(t *testing.T) { + cases := []struct { + code string + want bool + }{ + {"NU1902", true}, // the transitive-vuln advisory that bites csharp-ls + {"NU1903", true}, + {"NU1605", true}, + {"NU1", true}, + {"nu1902", true}, // prefix is case-insensitive + {"CS0246", false}, // real compiler error — must survive the filter + {"CS1591", false}, + {"NU", false}, // no digits + {"NUGET", false}, // letters after NU + {"NU19A2", false}, // non-digit inside the number + {"1902", false}, + {"N", false}, + {"", false}, + } + for _, c := range cases { + assert.Equalf(t, c.want, isNuGetAdvisoryCode(c.code), "isNuGetAdvisoryCode(%q)", c.code) + } +} + +// TestDiagCodeString covers the wire-type normalisation: string codes pass +// through, json.Number renders, and anything else (incl. JSON-unmarshalled +// numeric float64) yields "" — sufficient because NU codes are always strings. +func TestDiagCodeString(t *testing.T) { + assert.Equal(t, "NU1902", diagCodeString("NU1902")) + assert.Equal(t, "1902", diagCodeString(json.Number("1902"))) + assert.Equal(t, "", diagCodeString(float64(1902))) + assert.Equal(t, "", diagCodeString(1902)) + assert.Equal(t, "", diagCodeString(nil)) +} + +// TestFilterCSharpAdvisoryDiags verifies the filter drops only NU#### NuGet +// advisories, preserves real CS#### diagnostics and their order, and returns +// the input untouched when there is nothing to drop. +func TestFilterCSharpAdvisoryDiags(t *testing.T) { + // No advisories → same slice handed back, unchanged. + clean := []Diagnostic{ + {Code: "CS0246", Severity: DiagSeverityError, Message: "type or namespace not found"}, + {Code: "CS1591", Severity: DiagSeverityWarning, Message: "missing XML comment"}, + } + assert.Equal(t, clean, filterCSharpAdvisoryDiags(clean)) + + // Mixed: NU advisories dropped, CS diagnostics kept in original order. + mixed := []Diagnostic{ + {Code: "NU1902", Severity: DiagSeverityError, Message: "package has a known vulnerability"}, + {Code: "CS0246", Severity: DiagSeverityError, Message: "type or namespace not found"}, + {Code: "NU1903", Severity: DiagSeverityWarning, Message: "high severity vulnerability"}, + {Code: "CS0103", Severity: DiagSeverityError, Message: "name does not exist"}, + } + got := filterCSharpAdvisoryDiags(mixed) + assert.Len(t, got, 2) + assert.Equal(t, "CS0246", got[0].Code) + assert.Equal(t, "CS0103", got[1].Code) + + // All advisories → empty result. + assert.Empty(t, filterCSharpAdvisoryDiags([]Diagnostic{{Code: "NU1902"}, {Code: "NU1605"}})) + + // Empty / nil input is safe. + assert.Empty(t, filterCSharpAdvisoryDiags(nil)) +} + +// TestServesCSharp checks the language-scoping guard used to confine the +// C#-specific behaviour to C# providers. +func TestServesCSharp(t *testing.T) { + assert.True(t, (&Provider{languages: []string{"csharp"}}).servesCSharp()) + assert.True(t, (&Provider{languages: []string{"go", "csharp"}}).servesCSharp()) + assert.False(t, (&Provider{languages: []string{"go"}}).servesCSharp()) + assert.False(t, (&Provider{}).servesCSharp()) +} + +// TestCSharpDiagFilterEnabled confirms the filter is ON by default and only +// the explicit falsey values disable it. +func TestCSharpDiagFilterEnabled(t *testing.T) { + t.Setenv(CSharpDiagFilterEnv, "") // unset-equivalent → default ON + assert.True(t, csharpDiagFilterEnabled()) + + for _, off := range []string{"0", "off", "false", "none", "OFF", "False"} { + t.Setenv(CSharpDiagFilterEnv, off) + assert.Falsef(t, csharpDiagFilterEnabled(), "value %q should disable the filter", off) + } + + t.Setenv(CSharpDiagFilterEnv, "1") + assert.True(t, csharpDiagFilterEnabled()) +} + +// TestCSharpPreRestoreEligible verifies the pre-restore gate: ON by default for +// a C# provider that is spawning, disabled only by an explicit falsey +// CSharpRestoreEnv, and never active for a non-C# provider or a passive attach +// (where the IDE owns restore). +func TestCSharpPreRestoreEligible(t *testing.T) { + csharp := func() *Provider { return &Provider{languages: []string{"csharp"}} } + + t.Setenv(CSharpRestoreEnv, "") // unset-equivalent → default ON + assert.True(t, csharp().csharpPreRestoreEligible(), "on by default for a spawning C# provider") + + for _, off := range []string{"0", "off", "false", "none", "OFF", "False"} { + t.Setenv(CSharpRestoreEnv, off) + assert.Falsef(t, csharp().csharpPreRestoreEligible(), "value %q should disable pre-restore", off) + } + + t.Setenv(CSharpRestoreEnv, "1") + assert.True(t, csharp().csharpPreRestoreEligible(), "explicitly enabled + serves C# + spawning") + + // Not a C# provider → never restore, even with restore enabled. + assert.False(t, (&Provider{languages: []string{"go"}}).csharpPreRestoreEligible()) + + // C# but passively attached (the IDE owns restore) → skip. + p := csharp() + p.connect = &ConnectSpec{} + assert.False(t, p.csharpPreRestoreEligible(), "passive-attach must not trigger restore") +} diff --git a/internal/semantic/lsp/provider.go b/internal/semantic/lsp/provider.go index d3910237..8bed3b3f 100644 --- a/internal/semantic/lsp/provider.go +++ b/internal/semantic/lsp/provider.go @@ -1,6 +1,7 @@ package lsp import ( + "context" "encoding/json" "fmt" "os" @@ -946,6 +947,191 @@ func lspCallTimeout() time.Duration { } } +// servesCSharp reports whether this provider routes C# (.cs) files. Used to +// scope the C#-specific pre-restore and diagnostic-filter behaviour so it can +// never touch another language's provider. +func (p *Provider) servesCSharp() bool { + for _, l := range p.languages { + if l == "csharp" { + return true + } + } + return false +} + +// CSharpDiagFilterEnv toggles the C# advisory-diagnostic filter (see +// filterCSharpAdvisoryDiags). ON by default; set to a falsey value +// ("0" / "off" / "false" / "none") to pass every diagnostic through unchanged. +const CSharpDiagFilterEnv = "GORTEX_LSP_CSHARP_DIAG_FILTER" + +// csharpDiagFilterEnabled reports whether the C# advisory-diagnostic filter is +// active. Default ON — the filter only drops NuGet advisory codes, which are +// never code-level problems an indexer acts on. +func csharpDiagFilterEnabled() bool { + switch strings.ToLower(strings.TrimSpace(os.Getenv(CSharpDiagFilterEnv))) { + case "0", "off", "false", "none": + return false + default: + return true + } +} + +// CSharpRestoreEnv toggles the C# pre-spawn `dotnet restore` (see +// Provider.maybeCSharpPreRestore). ON by default; set to a falsey value +// ("0" / "off" / "false" / "none") to skip it — e.g. offline / air-gapped +// indexing, or to keep indexing off the network. +const CSharpRestoreEnv = "GORTEX_LSP_CSHARP_RESTORE" + +// csharpRestoreEnabled reports whether the C# pre-spawn restore is active. +// Default ON: gortex only restores repositories the user has explicitly added +// (it never auto-discovers), and spawning the C# server already evaluates the +// project's MSBuild graph — so restore adds no execution surface beyond the +// workspace load it precedes, while letting the Roslyn workspace load every +// project instead of dropping audit-flagged ones and reporting false errors. +func csharpRestoreEnabled() bool { + switch strings.ToLower(strings.TrimSpace(os.Getenv(CSharpRestoreEnv))) { + case "0", "off", "false", "none": + return false + default: + return true + } +} + +// diagCodeString renders a Diagnostic.Code as a string when it is a string +// code (NuGet / Roslyn codes such as "NU1902" / "CS0246"); numeric codes +// return "". Sufficient for the NuGet-advisory check, which only matches NU####. +func diagCodeString(code any) string { + switch c := code.(type) { + case string: + return c + case json.Number: + return c.String() + default: + return "" + } +} + +// isNuGetAdvisoryCode reports whether code is a NuGet code — "NU" (any case) +// followed by one or more digits — the audit / restore advisory family. +func isNuGetAdvisoryCode(code string) bool { + if len(code) < 3 { + return false + } + if (code[0] != 'N' && code[0] != 'n') || (code[1] != 'U' && code[1] != 'u') { + return false + } + for _, r := range code[2:] { + if r < '0' || r > '9' { + return false + } + } + return true +} + +// filterCSharpAdvisoryDiags drops NuGet *advisory* diagnostics (code NU####) +// from a C# publishDiagnostics batch and returns the survivors. +// +// Why: csharp-ls / OmniSharp build a Roslyn MSBuildWorkspace that escalates a +// NuGet audit *warning* — e.g. NU1902 "package has a known vulnerability" — to +// a fatal project-load failure and then surfaces it as a diagnostic. The +// `dotnet build` / `dotnet test` CLIs keep the same NU19xx a non-fatal warning +// and succeed, so the diagnostic is noise from the indexer's point of view +// (gortex does not act on dependency-vulnerability advisories). +// +// The filter is deliberately narrow: it matches ONLY the NU#### NuGet code +// family. Real compiler diagnostics (CS####) and analyzer warnings always pass +// through — a dropped project's genuine "unresolved type" errors are fixed by +// loading the project (the pre-restore guard), never by hiding CS codes +// here. Returns the input slice unchanged (no allocation) when nothing matches. +func filterCSharpAdvisoryDiags(diags []Diagnostic) []Diagnostic { + drop := false + for _, d := range diags { + if isNuGetAdvisoryCode(diagCodeString(d.Code)) { + drop = true + break + } + } + if !drop { + return diags + } + out := make([]Diagnostic, 0, len(diags)) + for _, d := range diags { + if isNuGetAdvisoryCode(diagCodeString(d.Code)) { + continue + } + out = append(out, d) + } + return out +} + +// csharpRestoreTimeout bounds the pre-spawn `dotnet restore`. +const csharpRestoreTimeout = 5 * time.Minute + +// csharpPreRestoreEligible reports whether the C# pre-restore should run for +// this provider: it serves C#, restore is enabled (csharpRestoreEnabled, ON by +// default), and we are spawning the server (not passively attached to an +// IDE-owned LSP, which manages its own restore). +func (p *Provider) csharpPreRestoreEligible() bool { + return p.connect == nil && p.servesCSharp() && csharpRestoreEnabled() +} + +// maybeCSharpPreRestore runs `dotnet restore` with NuGet audit suppressed in +// workspaceRoot before the C# LSP starts, when csharpPreRestoreEligible. +// +// Why: csharp-ls / OmniSharp treat a NuGet audit warning (NU19xx) as a fatal +// project-load failure and drop the project; its files then have no compilation +// and the server reports false "unresolved type" errors, while `dotnet build` +// keeps NU19xx a non-fatal warning. Restoring with `-p:NuGetAudit=false` writes +// a clean project.assets.json so the workspace loads every project. +// +// On by default (CSharpRestoreEnv): gortex only indexes repositories the user +// explicitly added (never auto-discovered), and the C# server spawn already +// evaluates the project's MSBuild graph — so restore adds no execution surface +// beyond the workspace load it precedes. Best-effort: a restore failure logs +// and falls through to the normal spawn (status quo), never aborting +// enrichment; skipped on passive attach (the IDE owns restore) and when dotnet +// is not on PATH. +func (p *Provider) maybeCSharpPreRestore(workspaceRoot string) { + if !p.csharpPreRestoreEligible() { + return + } + if _, err := exec.LookPath("dotnet"); err != nil { + if p.logger != nil { + p.logger.Debug("lsp: csharp pre-restore skipped — dotnet not on PATH", + zap.Error(err)) + } + return + } + ctx, cancel := context.WithTimeout(context.Background(), csharpRestoreTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, "dotnet", "restore", "-p:NuGetAudit=false") + cmd.Dir = workspaceRoot + cmd.Env = append(os.Environ(), p.env...) + if out, err := cmd.CombinedOutput(); err != nil { + if p.logger != nil { + p.logger.Warn("lsp: csharp pre-restore failed; spawning server anyway", + zap.String("workspace", workspaceRoot), + zap.Error(err), + zap.ByteString("output", lastBytes(out, 600)), + ) + } + return + } + if p.logger != nil { + p.logger.Info("lsp: csharp pre-restore complete (NuGetAudit suppressed)", + zap.String("workspace", workspaceRoot)) + } +} + +// lastBytes returns up to the last n bytes of b — keeps a failed restore's +// tail (where the error sits) out of an unbounded log line. +func lastBytes(b []byte, n int) []byte { + if len(b) <= n { + return b + } + return b[len(b)-n:] +} + // ensureClient starts the LSP server if not already running, OR // reconnects if the previous client's transport went away (e.g. the // IDE that owned a passive-attach LSP restarted, closing the socket). @@ -978,6 +1164,12 @@ func (p *Provider) ensureClient(workspaceRoot string) error { p.dynamicCaps = map[string]Registration{} p.capsMu.Unlock() + // C#: optionally `dotnet restore` (NuGet audit suppressed) before the + // server starts so its MSBuild workspace loads every project instead of + // dropping audit-warning projects and reporting false errors. On by + // default and best-effort — see maybeCSharpPreRestore / CSharpRestoreEnv. + p.maybeCSharpPreRestore(workspaceRoot) + client, err := p.dialOrSpawn(workspaceRoot) if err != nil { return err @@ -996,10 +1188,18 @@ func (p *Provider) ensureClient(workspaceRoot string) error { if abs == "" { return } + diags := pd.Diagnostics + // C#: strip NuGet audit advisories (NU19xx) that csharp-ls / + // OmniSharp surface as diagnostics (and escalate to fatal + // project drops) — they are not code errors an indexer acts + // on. Real CS#### compiler diagnostics pass through untouched. + if p.servesCSharp() && csharpDiagFilterEnabled() { + diags = filterCSharpAdvisoryDiags(diags) + } p.docMu.Lock() - p.lastDiag[abs] = pd.Diagnostics + p.lastDiag[abs] = diags p.docMu.Unlock() - p.fanoutDiagnostics(abs, pd.Diagnostics) + p.fanoutDiagnostics(abs, diags) }) // Some servers (rust-analyzer, jdtls) emit progress / log messages // — silently swallow them; they're not actionable for the indexer.