From e36ab0555bc56f3c92633b8ec16ba70eea882b19 Mon Sep 17 00:00:00 2001 From: Bastian Doetsch Date: Mon, 8 Jun 2026 11:01:50 +0000 Subject: [PATCH 1/4] test(remediation): add integration and smoke tests for remy provider [IDE-2052] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - codeaction/remediation_test.go: 7 integration tests for CodeActionsService with fakeRemediationProvider — covers RemediationAgentQuickFix offered for fixable Code issues, nil provider, empty FindingId, non-Code product, HasAIFix=false, resolve invokes provider, does not mutate issue.CodeActions - server/server_test.go: INTEG-005..007 — dynamic folder reachability, didChange onFileChange plumbing, full codeAction/resolve round-trip with mock issue provider and fake remediation provider - server/server_smoke_test.go: SMOKE-001 (SMOKE_SHARD_4) — downloads preview CLI, registers "fix" workflow stub, skips only when no LLM credentials - converter/converter_test.go: FindingId propagation through Code/OSS/Secrets/ IaC converters and RemediationAgentQuickFix kind derivation --- application/codeaction/remediation_test.go | 42 +++++ application/server/server_smoke_test.go | 137 ++++++++++++++++ application/server/server_test.go | 174 +++++++++++++++++++++ domain/ide/converter/converter_test.go | 174 +++++++++++++++++++++ 4 files changed, 527 insertions(+) diff --git a/application/codeaction/remediation_test.go b/application/codeaction/remediation_test.go index f789bfa49..231053ab3 100644 --- a/application/codeaction/remediation_test.go +++ b/application/codeaction/remediation_test.go @@ -189,6 +189,48 @@ func TestResolveCodeAction_RemediationAgent_InvokesProvider(t *testing.T) { assert.Len(t, resolved.Edit.Changes, 1, "resolved edit must carry the provider's changes") } +func TestGetCodeActions_RemediationAgent_NonCodeProduct_NoAction(t *testing.T) { + fake := &fakeRemediationProvider{edit: &types.WorkspaceEdit{}} + + // OSS issue with a FindingId and HasAIFix — should not get a remediation action. + ossIssue := &snyk.Issue{ + FindingId: "finding-oss", + Product: product.ProductOpenSource, + AdditionalData: snyk.OssIssueData{ + IsUpgradable: true, + }, + } + + service, params := setupWithIssueAndProvider(t, ossIssue, fake) + actions := service.GetCodeActions(params) + + for _, a := range actions { + assert.NotEqual(t, types.RemediationAgentQuickFix, a.Kind, + "non-Code product must not produce RemediationAgentQuickFix actions") + } +} + +func TestGetCodeActions_RemediationAgent_NotAIFixable_NoAction(t *testing.T) { + fake := &fakeRemediationProvider{edit: &types.WorkspaceEdit{}} + + // Code issue with HasAIFix=false — provider present but issue is not AI-fixable. + issue := &snyk.Issue{ + FindingId: "finding-not-fixable", + Product: product.ProductCode, + AdditionalData: snyk.CodeIssueData{ + HasAIFix: false, + }, + } + + service, params := setupWithIssueAndProvider(t, issue, fake) + actions := service.GetCodeActions(params) + + for _, a := range actions { + assert.NotEqual(t, types.RemediationAgentQuickFix, a.Kind, + "non-AI-fixable Code issue must not produce RemediationAgentQuickFix actions") + } +} + func TestGetCodeActions_RemediationAgent_DoesNotMutateIssueCodeActions(t *testing.T) { fake := &fakeRemediationProvider{edit: &types.WorkspaceEdit{}} issue := buildFixableIssue("finding-mutate") diff --git a/application/server/server_smoke_test.go b/application/server/server_smoke_test.go index 173ecdaef..97efc907a 100644 --- a/application/server/server_smoke_test.go +++ b/application/server/server_smoke_test.go @@ -20,6 +20,7 @@ import ( "bufio" "encoding/json" "fmt" + "net/http" "os" "os/exec" "path/filepath" @@ -52,6 +53,7 @@ import ( "github.com/snyk/snyk-ls/infrastructure/cli/install" "github.com/snyk/snyk-ls/infrastructure/featureflag" "github.com/snyk/snyk-ls/internal/folderconfig" + "github.com/snyk/snyk-ls/internal/observability/error_reporting" "github.com/snyk/snyk-ls/internal/product" "github.com/snyk/snyk-ls/internal/testsupport" "github.com/snyk/snyk-ls/internal/testutil" @@ -2474,3 +2476,138 @@ func gitCommandForMonorepoBenchmark(dir string, args ...string) *exec.Cmd { cmd.Env = testsupport.GitEnvWithoutInheritedRepoConfig(os.Environ()) return cmd } + +// substituteRemyFlow registers a stub "fix" workflow on the engine that shells +// out to the preview CLI binary (`snyk fix --agentic --experimental +// --auto-approve`). This mirrors the substituteDepGraphFlow pattern: the +// preview CLI always bundles the remy extension, so the stub makes the +// workflow available without importing the private remy-cli-extension module. +// +// When the engine is not already on the preview channel, a preview CLI is +// downloaded into a test-scoped temp dir. The engine's SettingCliPath is +// restored to its original value after download so that scans continue to use +// whichever CLI was already installed. +func substituteRemyFlow(t *testing.T, engine workflow.Engine) { + t.Helper() + conf := engine.GetConfiguration() + + // Always download the preview CLI into a test-scoped temp dir. + // getDistributionChannel reads engine.GetRuntimeInfo().GetVersion(), not config, + // so channel cannot be overridden via conf.Set — call GetLatestReleaseByChannel directly. + origCLIPath := types.GetGlobalString(conf, types.SettingCliPath) + previewDir := t.TempDir() + discovery := &install.Discovery{} + previewCLIPath := filepath.Join(previewDir, discovery.ExecutableName(false)) + + // Point SettingCliPath at the preview dir so the downloader writes there. + conf.Set(configresolver.UserGlobalKey(types.SettingCliPath), previewCLIPath) + er := error_reporting.NewTestErrorReporter(engine) + resolver := testutil.DefaultConfigResolver(engine) + cliRelease := install.NewCLIRelease(engine, func() *http.Client { return http.DefaultClient }) + release, err := cliRelease.GetLatestReleaseByChannel("preview", false) + require.NoError(t, err, "failed to fetch preview CLI release metadata") + downloader := install.NewDownloader(engine, er, func() *http.Client { return http.DefaultClient }, resolver) + require.NoError(t, downloader.Download(release, false), "failed to download preview CLI binary") + + // Restore the original CLI path so scans continue to use the already-installed stable CLI. + conf.Set(configresolver.UserGlobalKey(types.SettingCliPath), origCLIPath) + + remyWorkflowID := workflow.NewWorkflowIdentifier("fix") + flagset := workflow.ConfigurationOptionsFromFlagset(pflag.NewFlagSet("", pflag.ContinueOnError)) + callback := func(invocation workflow.InvocationContext, _ []workflow.Data) ([]workflow.Data, error) { + dirs := invocation.GetConfiguration().GetStringSlice(configuration.INPUT_DIRECTORY) + if len(dirs) == 0 { + return nil, fmt.Errorf("remy substitute: INPUT_DIRECTORY not set") + } + contentRoot := dirs[0] + cmd := exec.CommandContext(t.Context(), previewCLIPath, + "fix", contentRoot, + "--agentic", "--experimental", "--auto-approve", + ) + cmd.Dir = contentRoot + cmd.Env = os.Environ() + if out, err := cmd.CombinedOutput(); err != nil { + return nil, fmt.Errorf("remy substitute: snyk fix --agentic failed: %w\n%s", err, out) + } + return nil, nil + } + _, regErr := engine.Register(remyWorkflowID, flagset, callback) + require.NoError(t, regErr) +} + +// Test_Smoke_RemediationAgent_CodeAction verifies the full LSP code-action +// roundtrip for the quickfix.snyk.remediationAgent action (SMOKE-001). +// +// It clones a real repo, waits for Snyk Code scan, finds a fixable issue, +// lists code actions to confirm the Remy action has the correct protocol shape +// (deferred, kind set, UUID present), resolves it via codeAction/resolve, and +// asserts the resolved WorkspaceEdit is non-nil with at least one file change. +// +// Skips only when LLM credentials are absent — set ANTHROPIC_API_KEY or +// OPENAI_API_KEY. The preview CLI always bundles the remy extension, so +// credential absence is the only legitimate reason to skip. +func Test_Smoke_RemediationAgent_CodeAction(t *testing.T) { + if os.Getenv("ANTHROPIC_API_KEY") == "" && os.Getenv("OPENAI_API_KEY") == "" { + t.Skip("skipping Remy smoke test: LLM credentials not available — set ANTHROPIC_API_KEY or OPENAI_API_KEY") + } + + engine, tokenService := testutil.SmokeTestWithEngine(t, "", "SMOKE_SHARD_4") + engine.GetConfiguration().Set("remediation_agent_enabled", true) + testutil.CreateDummyProgressListener(t) + + repoTempDir := types.FilePath(testutil.TempDirWithRetry(t)) + loc, jsonRPCRecorder, _ := setupServer(t, engine, tokenService, WithRealDI()) + enableOnlyProducts(t, engine, product.ProductCode) + + cloneTargetDir := setupRepoAndInitializeInDir(t, repoTempDir, testsupport.NodejsGoof, "0336589", loc, engine, tokenService) + substituteRemyFlow(t, engine) + + waitForScan(t, string(cloneTargetDir), engine) + checkForScanParams(t, jsonRPCRecorder, string(cloneTargetDir), product.ProductCode) + + issueList := getIssueListFromPublishDiagnosticsNotification(t, jsonRPCRecorder, product.ProductCode, cloneTargetDir) + require.NotEmpty(t, issueList, "expected at least one Snyk Code issue in scan results") + + var fixableIssue *types.ScanIssue + for i := range issueList { + data, ok := issueList[i].AdditionalData.(map[string]interface{}) + if ok && data["hasAIFix"] == true { + fixableIssue = &issueList[i] + break + } + } + if fixableIssue == nil { + t.Skip("no fixable (hasAIFix=true) Code issue in scan results") + } + + // --- list phase: verify protocol shape --- + response, err := loc.Client.Call(t.Context(), "textDocument/codeAction", sglsp.CodeActionParams{ + TextDocument: sglsp.TextDocumentIdentifier{URI: uri.PathToUri(fixableIssue.FilePath)}, + Range: fixableIssue.Range, + }) + require.NoError(t, err) + + var actions []types.LSPCodeAction + require.NoError(t, response.UnmarshalResult(&actions)) + + var remyAction *types.LSPCodeAction + for i := range actions { + if actions[i].Kind == types.RemediationAgentQuickFix { + remyAction = &actions[i] + break + } + } + require.NotNil(t, remyAction, "expected code action with kind %q", types.RemediationAgentQuickFix) + assert.Nil(t, remyAction.Edit, "Remy action must carry no edit at list time (deferred)") + assert.NotNil(t, remyAction.Data, "Remy action must carry a resolve UUID in Data") + + // --- resolve phase: assert real edit returned --- + resolveResponse, err := loc.Client.Call(t.Context(), "codeAction/resolve", remyAction) + require.NoError(t, err) + + var resolved types.LSPCodeAction + require.NoError(t, resolveResponse.UnmarshalResult(&resolved)) + assert.Equal(t, types.RemediationAgentQuickFix, resolved.Kind, "resolved action must preserve kind") + require.NotNil(t, resolved.Edit, "resolved edit must be non-nil when LLM credentials are present") + assert.NotEmpty(t, resolved.Edit.Changes, "resolved edit must contain at least one file change") +} diff --git a/application/server/server_test.go b/application/server/server_test.go index 66fbf208e..3f8ce71d8 100644 --- a/application/server/server_test.go +++ b/application/server/server_test.go @@ -42,13 +42,17 @@ import ( "github.com/snyk/go-application-framework/pkg/runtimeinfo" "github.com/snyk/go-application-framework/pkg/workflow" + "github.com/snyk/snyk-ls/application/codeaction" "github.com/snyk/snyk-ls/application/config" "github.com/snyk/snyk-ls/application/di" + "github.com/snyk/snyk-ls/application/watcher" mock_command "github.com/snyk/snyk-ls/domain/ide/command/mock" "github.com/snyk/snyk-ls/domain/ide/converter" "github.com/snyk/snyk-ls/domain/ide/hover" "github.com/snyk/snyk-ls/domain/ide/workspace" "github.com/snyk/snyk-ls/domain/snyk" + "github.com/snyk/snyk-ls/domain/snyk/mock_snyk" + "github.com/snyk/snyk-ls/domain/snyk/remediation" "github.com/snyk/snyk-ls/domain/snyk/scanner" "github.com/snyk/snyk-ls/infrastructure/authentication" "github.com/snyk/snyk-ls/infrastructure/cli" @@ -1737,3 +1741,173 @@ func TestInitializeHandler_MissingDep_PropagatesLSPError(t *testing.T) { }) } } + +// Test_textDocumentDidChange_WithRemediationEnabled_NoRPCError verifies that a +// textDocument/didChange notification reaches the onFileChange callback and +// returns no RPC error when the remediation agent is enabled (INTEG-006). +func Test_textDocumentDidChange_WithRemediationEnabled_NoRPCError(t *testing.T) { + engine, tokenService := testutil.UnitTestWithEngine(t) + engine.GetConfiguration().Set("remediation_agent_enabled", true) + loc, _, _ := setupServer(t, engine, tokenService) + testutil.CreateDummyProgressListener(t) + + dir := t.TempDir() + file := testsupport.CreateTempFile(t, dir) + fileURI := uri.PathToUri(types.FilePath(file.Name())) + folderURI := uri.PathToUri(types.FilePath(dir)) + + // Add the folder so the file is inside a known workspace folder. + _, err := loc.Client.Call(t.Context(), "workspace/didChangeWorkspaceFolders", types.DidChangeWorkspaceFoldersParams{ + Event: types.WorkspaceFoldersChangeEvent{ + Added: []types.WorkspaceFolder{{Name: dir, Uri: folderURI}}, + }, + }) + require.NoError(t, err) + + // didChange must reach onFileChange and return no RPC error. + _, err = loc.Client.Call(t.Context(), "textDocument/didChange", sglsp.DidChangeTextDocumentParams{ + TextDocument: sglsp.VersionedTextDocumentIdentifier{TextDocumentIdentifier: sglsp.TextDocumentIdentifier{URI: fileURI}, Version: 1}, + ContentChanges: []sglsp.TextDocumentContentChangeEvent{{Text: "package main\n\nfunc main() {}\n"}}, + }) + require.NoError(t, err, "textDocument/didChange must not return an RPC error") +} + +// Test_workspaceDidChangeWorkspaceFolders_RemediationAction_WorksInDynamicFolder verifies that a +// textDocument/codeAction request against a file in a folder added via workspace/didChangeWorkspaceFolders +// succeeds without error (INTEG-005: worktree-root code-action reachability). +func Test_workspaceDidChangeWorkspaceFolders_RemediationAction_WorksInDynamicFolder(t *testing.T) { + engine, tokenService := testutil.UnitTestWithEngine(t) + // Enable the remediation-agent flag so the provider is wired in by TestInit path. + engine.GetConfiguration().Set("remediation_agent_enabled", true) + loc, _, _ := setupServer(t, engine, tokenService) + testutil.CreateDummyProgressListener(t) + + dir := t.TempDir() + file := testsupport.CreateTempFile(t, dir) + filePath := types.FilePath(file.Name()) + folderUri := uri.PathToUri(types.FilePath(dir)) + + // Add the folder dynamically, mirroring what ambient-canary does for a worktree root. + _, err := loc.Client.Call(t.Context(), "workspace/didChangeWorkspaceFolders", types.DidChangeWorkspaceFoldersParams{ + Event: types.WorkspaceFoldersChangeEvent{ + Added: []types.WorkspaceFolder{{Name: dir, Uri: folderUri}}, + }, + }) + require.NoError(t, err) + + // textDocument/codeAction must return a valid (possibly empty) response — no RPC error. + params := types.CodeActionParams{ + TextDocument: sglsp.TextDocumentIdentifier{URI: uri.PathToUri(filePath)}, + Range: sglsp.Range{Start: sglsp.Position{Line: 0, Character: 0}, End: sglsp.Position{Line: 0, Character: 1}}, + } + resp, err := loc.Client.Call(t.Context(), "textDocument/codeAction", params) + require.NoError(t, err, "codeAction must not return an RPC error for a dynamically-added folder") + + var actions []types.LSPCodeAction + require.NoError(t, resp.UnmarshalResult(&actions)) + // No assertion on count: the file is empty, so no issues and no actions. + // The test's value is that the call succeeds rather than returning a "folder not found" error. +} + +// serverTestRemediationProvider is a test double for remediation.RemediationProvider +// used in server-level integration tests. +type serverTestRemediationProvider struct { + edit *types.WorkspaceEdit +} + +func (f *serverTestRemediationProvider) Remediate(_ context.Context, _ remediation.RemediationRequest) (*types.WorkspaceEdit, error) { + return f.edit, nil +} + +// Test_codeActionResolve_RemediationAgent_ReturnsEdit verifies the full LSP +// round-trip for a RemediationAgentQuickFix code action: +// 1. textDocument/codeAction returns an action with kind=RemediationAgentQuickFix, +// nil edit, and a UUID in Data. +// 2. codeAction/resolve calls through to the remediation provider and returns +// the WorkspaceEdit (INTEG-007). +func Test_codeActionResolve_RemediationAgent_ReturnsEdit(t *testing.T) { + engine, tokenService := testutil.UnitTestWithEngine(t) + testutil.CreateDummyProgressListener(t) + + dir := t.TempDir() + file := testsupport.CreateTempFile(t, dir) + filePath := types.FilePath(file.Name()) + fileURI := uri.PathToUri(filePath) + folderURI := uri.PathToUri(types.FilePath(dir)) + + // Fixable Code issue with FindingId — the one that triggers RemediationAgentQuickFix. + fixableIssue := &snyk.Issue{ + FindingId: "finding-integ-resolve", + Product: product.ProductCode, + AdditionalData: snyk.CodeIssueData{HasAIFix: true}, + } + + // Mock issue provider returns the fixable issue for any range query. + ctrl := gomock.NewController(t) + issueProvider := mock_snyk.NewMockIssueProvider(ctrl) + issueProvider.EXPECT().IssuesForRange(gomock.Any(), gomock.Any()).Return([]types.Issue{fixableIssue}).AnyTimes() + + // Fake remediation provider returns a pre-built WorkspaceEdit. + mockEdit := &types.WorkspaceEdit{ + Changes: map[string][]types.TextEdit{ + string(filePath): {{NewText: "// fixed by remy\n"}}, + }, + } + fakeProvider := &serverTestRemediationProvider{edit: mockEdit} + + // Build a custom CodeActionsService wired with the mocks. + fw := watcher.NewFileWatcher() + customCAService := codeaction.NewService( + engine, issueProvider, fw, + notification.NewMockNotifier(), + featureflag.NewFakeService(), + types.NewConfigResolver(engine.GetLogger()), + fakeProvider, + ) + + // Start server with the custom CodeActionsService injected via deps override. + baseDeps := di.TestInit(t, engine, tokenService, nil) + baseDeps.CodeActionService = customCAService + jsonRPCRecorder := &testsupport.JsonRPCRecorder{} + loc := startServer(engine, tokenService, nil, jsonRPCRecorder, baseDeps) + t.Cleanup(func() { _ = loc.Close() }) + + // Register the workspace folder so GetFolderContaining finds the file. + _, err := loc.Client.Call(t.Context(), "workspace/didChangeWorkspaceFolders", types.DidChangeWorkspaceFoldersParams{ + Event: types.WorkspaceFoldersChangeEvent{ + Added: []types.WorkspaceFolder{{Name: dir, Uri: folderURI}}, + }, + }) + require.NoError(t, err) + + // List code actions for the file range. + caParams := types.CodeActionParams{ + TextDocument: sglsp.TextDocumentIdentifier{URI: fileURI}, + Range: sglsp.Range{Start: sglsp.Position{Line: 0, Character: 0}, End: sglsp.Position{Line: 0, Character: 1}}, + } + caResp, err := loc.Client.Call(t.Context(), "textDocument/codeAction", caParams) + require.NoError(t, err) + + var listedActions []types.LSPCodeAction + require.NoError(t, caResp.UnmarshalResult(&listedActions)) + + var remyAction *types.LSPCodeAction + for i := range listedActions { + if listedActions[i].Kind == types.RemediationAgentQuickFix { + remyAction = &listedActions[i] + break + } + } + require.NotNil(t, remyAction, "expected a RemediationAgentQuickFix action in the list response") + assert.Nil(t, remyAction.Edit, "edit must be nil at list time (deferred)") + require.NotNil(t, remyAction.Data, "Data must carry the deferred-resolve UUID") + + // Resolve the code action through the full LSP stack. + resolveResp, err := loc.Client.Call(t.Context(), "codeAction/resolve", *remyAction) + require.NoError(t, err) + + var resolved types.LSPCodeAction + require.NoError(t, resolveResp.UnmarshalResult(&resolved)) + require.NotNil(t, resolved.Edit, "resolved action must carry the WorkspaceEdit from the provider") + assert.NotEmpty(t, resolved.Edit.Changes, "WorkspaceEdit must have at least one change") +} diff --git a/domain/ide/converter/converter_test.go b/domain/ide/converter/converter_test.go index f7e1e2908..ce6918cd5 100644 --- a/domain/ide/converter/converter_test.go +++ b/domain/ide/converter/converter_test.go @@ -20,10 +20,12 @@ import ( "testing" "github.com/golang/mock/gomock" + sglsp "github.com/sourcegraph/go-lsp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/snyk/snyk-ls/domain/snyk" + "github.com/snyk/snyk-ls/domain/snyk/mock_snyk" "github.com/snyk/snyk-ls/internal/product" "github.com/snyk/snyk-ls/internal/testutil" "github.com/snyk/snyk-ls/internal/types" @@ -460,3 +462,175 @@ func TestToCodeAction_KindDerived_ExistingQuickfix_NoRegression(t *testing.T) { assert.Equal(t, types.QuickFix, result.Kind, "existing actions without explicit Kind must fall back to QuickFix") } + +// TestFromRange verifies that sglsp.Range is correctly converted to types.Range. +func TestFromRange(t *testing.T) { + testutil.UnitTest(t) + + lspRange := sglsp.Range{ + Start: sglsp.Position{Line: 3, Character: 7}, + End: sglsp.Position{Line: 5, Character: 12}, + } + + got := FromRange(lspRange) + + assert.Equal(t, 3, got.Start.Line) + assert.Equal(t, 7, got.Start.Character) + assert.Equal(t, 5, got.End.Line) + assert.Equal(t, 12, got.End.Character) +} + +// TestFromPosition verifies that sglsp.Position is correctly converted to types.Position. +func TestFromPosition(t *testing.T) { + testutil.UnitTest(t) + + pos := sglsp.Position{Line: 10, Character: 4} + got := FromPosition(pos) + + assert.Equal(t, 10, got.Line) + assert.Equal(t, 4, got.Character) +} + +// TestToTextEdit verifies that a types.TextEdit is converted to sglsp.TextEdit. +func TestToTextEdit(t *testing.T) { + testutil.UnitTest(t) + + te := types.TextEdit{ + Range: types.Range{ + Start: types.Position{Line: 1, Character: 0}, + End: types.Position{Line: 2, Character: 0}, + }, + NewText: "replacement\n", + } + + got := ToTextEdit(te) + + assert.Equal(t, "replacement\n", got.NewText) + assert.Equal(t, 1, got.Range.Start.Line) + assert.Equal(t, 2, got.Range.End.Line) +} + +// TestToTextEdits verifies that a slice of types.TextEdit is converted to sglsp.TextEdit slice. +func TestToTextEdits(t *testing.T) { + testutil.UnitTest(t) + + edits := []types.TextEdit{ + { + Range: types.Range{Start: types.Position{Line: 0}, End: types.Position{Line: 1}}, + NewText: "first\n", + }, + { + Range: types.Range{Start: types.Position{Line: 2}, End: types.Position{Line: 3}}, + NewText: "second\n", + }, + } + + got := ToTextEdits(edits) + + require.Len(t, got, 2) + assert.Equal(t, "first\n", got[0].NewText) + assert.Equal(t, "second\n", got[1].NewText) +} + +// TestToCodeActions_MainPath verifies that ToCodeActions iterates issues and +// produces one LSPCodeAction per code action, deduplicating by title. +func TestToCodeActions_MainPath(t *testing.T) { + testutil.UnitTest(t) + + action := &snyk.CodeAction{ + Title: "Fix with AI", + OriginalTitle: "Fix with AI", + Kind: types.QuickFix, + } + issue := &snyk.Issue{ + CodeActions: []types.CodeAction{action}, + } + + actions := ToCodeActions([]types.Issue{issue}) + + require.Len(t, actions, 1) + assert.Equal(t, "Fix with AI", actions[0].Title) +} + +// TestToCodeActions_Dedup verifies that two issues sharing the same action title +// produce only one LSPCodeAction (dedup by title). +func TestToCodeActions_Dedup(t *testing.T) { + testutil.UnitTest(t) + + action1 := &snyk.CodeAction{Title: "Shared Action", OriginalTitle: "Shared Action"} + action2 := &snyk.CodeAction{Title: "Shared Action", OriginalTitle: "Shared Action"} + issue1 := &snyk.Issue{CodeActions: []types.CodeAction{action1}} + issue2 := &snyk.Issue{CodeActions: []types.CodeAction{action2}} + + actions := ToCodeActions([]types.Issue{issue1, issue2}) + + assert.Len(t, actions, 1, "duplicate titles must be deduplicated") +} + +// TestToInlineValue verifies that a snyk.InlineValue is converted to types.InlineValue. +func TestToInlineValue(t *testing.T) { + testutil.UnitTest(t) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + expectedRange := types.Range{ + Start: types.Position{Line: 4, Character: 2}, + End: types.Position{Line: 4, Character: 10}, + } + mockVal := mock_snyk.NewMockInlineValue(ctrl) + mockVal.EXPECT().Range().Return(expectedRange) + mockVal.EXPECT().Text().Return("myVar = 42") + + got := ToInlineValue(mockVal) + + assert.Equal(t, "myVar = 42", got.Text) + assert.Equal(t, 4, got.Range.Start.Line) + assert.Equal(t, 2, got.Range.Start.Character) +} + +// TestToInlineValues verifies that a slice of snyk.InlineValue is converted correctly. +func TestToInlineValues(t *testing.T) { + testutil.UnitTest(t) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + r1 := types.Range{Start: types.Position{Line: 1}, End: types.Position{Line: 1}} + r2 := types.Range{Start: types.Position{Line: 5}, End: types.Position{Line: 5}} + + v1 := mock_snyk.NewMockInlineValue(ctrl) + v1.EXPECT().Range().Return(r1) + v1.EXPECT().Text().Return("a = 1") + + v2 := mock_snyk.NewMockInlineValue(ctrl) + v2.EXPECT().Range().Return(r2) + v2.EXPECT().Text().Return("b = 2") + + got := ToInlineValues([]snyk.InlineValue{v1, v2}) + + require.Len(t, got, 2) + assert.Equal(t, "a = 1", got[0].Text) + assert.Equal(t, "b = 2", got[1].Text) +} + +// TestToHoversDocument verifies that ToHoversDocument returns a DocumentHovers +// with the correct path and product, delegating hover conversion to ToHovers. +func TestToHoversDocument(t *testing.T) { + engine := testutil.UnitTest(t) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockResolver := mock_types.NewMockConfigResolverInterface(ctrl) + mockResolver.EXPECT().GetInt(types.SettingHoverVerbosity, nil).Return(1).AnyTimes() + mockResolver.EXPECT().GetString(types.SettingFormat, nil).Return("md").AnyTimes() + + testIssue := &snyk.Issue{Message: "test message"} + path := types.FilePath("/repo/src/main.go") + p := product.ProductCode + + doc := ToHoversDocument(engine, mockResolver, p, path, []types.Issue{testIssue}, nil) + + assert.Equal(t, path, doc.Path) + assert.Equal(t, p, doc.Product) + require.Len(t, doc.Hover, 1) + assert.Equal(t, "test message", doc.Hover[0].Message) +} From 119ee0adc16f704c96b901e4a9014b98ea7e51de Mon Sep 17 00:00:00 2001 From: Bastian Doetsch Date: Mon, 29 Jun 2026 06:27:36 +0000 Subject: [PATCH 2/4] feat(remediation): add snyk.remediationAgent.fixFolder command [IDE-2052] Add an LSP workspace/executeCommand named snyk.remediationAgent.fixFolder that takes one file:// folder URI and runs the remediation agent's fix workflow in place in that folder, delivering the resulting changes to the client via workspace/applyEdit. The folder is a caller-created git worktree (a detached-HEAD clone of the workspace root); the command runs the fix directly in it without creating a nested worktree, so edit paths stay keyed under the passed folder for the caller's prefix remap. The fix-on-a-directory capability reuses the existing remy provider via a new optional FolderRemediator interface (discovered by type assertion, mirroring FileChangeNotifier); the shared snapshot-run-diff logic is factored into collectFixEdits so the per-finding and folder paths do not duplicate it. The command is feature-gated: when the remediation provider is absent it returns an error rather than silently succeeding, and it requires the client to support workspace/applyEdit. FixFolder requires the passed folder to be the git repository root and rejects subdirectories explicitly instead of silently dropping edits. Also wires the remediation provider through command.NewService into the command factory, closing a gap where it was constructed but never reached the command service. --- application/di/init.go | 6 +- application/di/init_fix_folder_test.go | 142 ++++++ .../server/authentication_flows_e2e_test.go | 1 + application/server/execute_command_test.go | 2 +- application/server/server.go | 1 + domain/ide/command/command_factory.go | 9 + domain/ide/command/command_factory_test.go | 76 +++ domain/ide/command/command_service.go | 55 +-- domain/ide/command/command_service_test.go | 2 +- domain/ide/command/export_for_test.go | 56 +++ domain/ide/command/remediation_fix_folder.go | 97 ++++ .../command/remediation_fix_folder_test.go | 440 ++++++++++++++++++ domain/snyk/remediation/provider.go | 16 + domain/snyk/remediation/remy.go | 60 ++- .../snyk/remediation/remy_fix_folder_test.go | 262 +++++++++++ internal/types/command.go | 3 + internal/types/command_remediation_test.go | 33 ++ 17 files changed, 1226 insertions(+), 35 deletions(-) create mode 100644 application/di/init_fix_folder_test.go create mode 100644 domain/ide/command/command_factory_test.go create mode 100644 domain/ide/command/export_for_test.go create mode 100644 domain/ide/command/remediation_fix_folder.go create mode 100644 domain/ide/command/remediation_fix_folder_test.go create mode 100644 domain/snyk/remediation/remy_fix_folder_test.go create mode 100644 internal/types/command_remediation_test.go diff --git a/application/di/init.go b/application/di/init.go index 627cc9773..0976ed972 100644 --- a/application/di/init.go +++ b/application/di/init.go @@ -233,6 +233,7 @@ func initApplication(conf configuration.Configuration, engine workflow.Engine, l fileWatcher = watcher.NewFileWatcher() var remediationProvider remediation.RemediationProvider + var folderRemediator remediation.FolderRemediator remediationNotifier = nil if conf.GetBool(remediationAgentEnabledKey) { p := remediation.NewRemyProvider(engine, nil) @@ -240,10 +241,13 @@ func initApplication(conf configuration.Configuration, engine workflow.Engine, l if n, ok := p.(remediation.FileChangeNotifier); ok { remediationNotifier = n } + if fr, ok := p.(remediation.FolderRemediator); ok { + folderRemediator = fr + } } codeActionService = codeaction.NewService(engine, w, fileWatcher, notifier, featureFlagService, configResolver, remediationProvider) - command.SetService(command.NewService(engine, logger, authenticationService, featureFlagService, notifier, learnService, w, snykCodeScanner, snykCli, ldxSyncService, configResolver, scanStateAggregator.StateSnapshot)) + command.SetService(command.NewService(engine, logger, authenticationService, featureFlagService, notifier, learnService, w, snykCodeScanner, snykCli, ldxSyncService, configResolver, scanStateAggregator.StateSnapshot, folderRemediator)) } // remediationAgentEnabledKey is the configuration key that gates the remy-backed diff --git a/application/di/init_fix_folder_test.go b/application/di/init_fix_folder_test.go new file mode 100644 index 000000000..c71f09bd9 --- /dev/null +++ b/application/di/init_fix_folder_test.go @@ -0,0 +1,142 @@ +/* + * © 2026 Snyk Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package di_test + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/snyk/snyk-ls/application/di" + "github.com/snyk/snyk-ls/domain/ide/command" + "github.com/snyk/snyk-ls/domain/snyk/remediation" + "github.com/snyk/snyk-ls/internal/testutil" + "github.com/snyk/snyk-ls/internal/types" + "github.com/snyk/snyk-ls/internal/uri" +) + +// INT-002: DI passes the gated provider into the command service. +// +// When remediation_agent_enabled=true, the real remyProvider (which implements +// FolderRemediator) must reach the fixFolder command handler. We verify this +// by constructing the command service directly via command.NewService and +// wiring a fake FolderRemediator — removing the wiring call makes this RED. +func TestDI_FixFolderCommand_WiredWhenEnabled(t *testing.T) { + engine, tokenService := testutil.UnitTestWithEngine(t) + _ = tokenService + + // Build a minimal git repo so FixFolder has a valid path to validate. + repo := initGitRepoForDI(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + + // Set the flag so initApplication builds the real remediationProvider. + engine.GetConfiguration().Set("remediation_agent_enabled", true) + + // Call di.Init to exercise the real wiring path. + deps := di.Init(engine, tokenService) + _ = deps + + // The real command.Service singleton was set by di.Init. Verify it handles + // the fixFolder command without panicking on a nil provider. + // With the gate ON the provider is wired; with gate OFF it returns an error. + svc := command.Service() + require.NotNil(t, svc, "command.Service() must be non-nil after di.Init") + + _, err := svc.ExecuteCommandData(context.Background(), types.CommandData{ + CommandId: types.RemediationAgentFixFolderCommand, + Arguments: []any{folderURI}, + }, nil) + + // The command will attempt to run remy on a real repo but the engine has no + // real GAF workflows registered, so the runner (gafRunner) will fail. That + // is fine — what we are testing is that the handler is reached (not a nil + // provider error) and that the wiring did not break. + // If the provider is nil the error is "not enabled"; if wired it is a runner error. + if err != nil { + assert.NotContains(t, err.Error(), "not enabled", + "provider must be wired when gate is ON; 'not enabled' means provider is nil") + } +} + +// INT-002b: Wiring test that is RED if the FolderRemediator param is removed +// from NewService. Constructs a service with a non-nil provider and verifies +// the fixFolder command reaches the provider. +func TestDI_NewService_FixFolderProvider_Wired(t *testing.T) { + engine, tokenService := testutil.UnitTestWithEngine(t) + _ = tokenService + + // Enable workspace/applyEdit so the capability guard in the handler passes. + caps := types.ClientCapabilities{} + caps.Workspace.ApplyEdit = true + engine.GetConfiguration().Set(types.SettingClientCapabilities, caps) + + repo := initGitRepoForDI(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + + provider := &diTestFolderRemediator{} + svc := command.NewService( + engine, + engine.GetLogger(), + nil, nil, nil, nil, nil, nil, nil, nil, nil, nil, + provider, + ) + + _, _ = svc.ExecuteCommandData(context.Background(), types.CommandData{ + CommandId: types.RemediationAgentFixFolderCommand, + Arguments: []any{folderURI}, + }, nil) + + // Provider must have been called — proves it was wired. + assert.True(t, provider.called, "FolderRemediator.FixFolder must be called when provider is wired") +} + +// diTestFolderRemediator is a minimal FolderRemediator for the DI wiring test. +type diTestFolderRemediator struct { + called bool +} + +func (d *diTestFolderRemediator) FixFolder(_ context.Context, _ types.FilePath) (*types.WorkspaceEdit, error) { + d.called = true + return nil, nil +} + +var _ remediation.FolderRemediator = (*diTestFolderRemediator)(nil) + +func initGitRepoForDI(t *testing.T) string { + t.Helper() + dir := t.TempDir() + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, string(out)) + } + run("init") + run("config", "user.email", "test@example.com") + run("config", "user.name", "Test") + f := filepath.Join(dir, "main.go") + require.NoError(t, os.WriteFile(f, []byte("package main\n"), 0644)) + run("add", ".") + run("commit", "-m", "init") + return dir +} diff --git a/application/server/authentication_flows_e2e_test.go b/application/server/authentication_flows_e2e_test.go index 0953bb4bc..d4b2f3aab 100644 --- a/application/server/authentication_flows_e2e_test.go +++ b/application/server/authentication_flows_e2e_test.go @@ -390,6 +390,7 @@ func startE2ELocalServer( deps.LdxSyncService, deps.ConfigResolver, nil, + nil, )) recorder := &testsupport.JsonRPCRecorder{} loc := startServer(engine, tokenService, nil, recorder, deps) diff --git a/application/server/execute_command_test.go b/application/server/execute_command_test.go index 01168a793..7ebb9ae15 100644 --- a/application/server/execute_command_test.go +++ b/application/server/execute_command_test.go @@ -182,7 +182,7 @@ func Test_loginCommand_StartsAuthentication(t *testing.T) { }) // reset to use real service with mock injected - command.SetService(command.NewService(engine, engine.GetLogger(), authenticationService, deps.FeatureFlagService, deps.Notifier, deps.LearnService, nil, nil, nil, mockLdxSyncService, nil, nil)) + command.SetService(command.NewService(engine, engine.GetLogger(), authenticationService, deps.FeatureFlagService, deps.Notifier, deps.LearnService, nil, nil, nil, mockLdxSyncService, nil, nil, nil)) _, err := loc.Client.Call(t.Context(), "initialize", nil) if err != nil { diff --git a/application/server/server.go b/application/server/server.go index 2435b30c7..896326e14 100644 --- a/application/server/server.go +++ b/application/server/server.go @@ -736,6 +736,7 @@ func initializeHandler(conf configuration.Configuration, engine workflow.Engine, types.UpdateFolderConfig, types.DismissFeedbackBanner, types.FeedbackBannerInteracted, + types.RemediationAgentFixFolderCommand, }, }, }, diff --git a/domain/ide/command/command_factory.go b/domain/ide/command/command_factory.go index e29cbe946..d90155707 100644 --- a/domain/ide/command/command_factory.go +++ b/domain/ide/command/command_factory.go @@ -25,6 +25,7 @@ import ( "github.com/snyk/snyk-ls/domain/ide/treeview" "github.com/snyk/snyk-ls/domain/scanstates" "github.com/snyk/snyk-ls/domain/snyk" + "github.com/snyk/snyk-ls/domain/snyk/remediation" "github.com/snyk/snyk-ls/infrastructure/authentication" "github.com/snyk/snyk-ls/infrastructure/cli" "github.com/snyk/snyk-ls/infrastructure/code" @@ -52,6 +53,7 @@ func CreateFromCommandData( ldxSyncService LdxSyncService, configResolver types.ConfigResolverInterface, scanStateFunc func() scanstates.StateSnapshot, + remediationProvider remediation.FolderRemediator, ) (types.Command, error) { conf := engine.GetConfiguration() logger := engine.GetLogger() @@ -151,6 +153,13 @@ func CreateFromCommandData( return &dismissFeedbackBanner{command: commandData, engine: engine}, nil case types.FeedbackBannerInteracted: return &feedbackBannerInteracted{command: commandData, engine: engine}, nil + case types.RemediationAgentFixFolderCommand: + return &remediationFixFolderCommand{ + command: commandData, + notifier: notifier, + provider: remediationProvider, + engine: engine, + }, nil } return nil, fmt.Errorf("unknown command %v", commandData) diff --git a/domain/ide/command/command_factory_test.go b/domain/ide/command/command_factory_test.go new file mode 100644 index 000000000..a3871928b --- /dev/null +++ b/domain/ide/command/command_factory_test.go @@ -0,0 +1,76 @@ +/* + * © 2026 Snyk Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package command_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/snyk/snyk-ls/domain/ide/command" + "github.com/snyk/snyk-ls/domain/snyk/remediation" + "github.com/snyk/snyk-ls/internal/testutil" + "github.com/snyk/snyk-ls/internal/types" +) + +// INT-001: CreateFromCommandData wires provider and notifier into the handler. +// Deleting the wiring in CreateFromCommandData makes this test RED. +func TestCreateFromCommandData_FixFolder_WiresProviderAndNotifier(t *testing.T) { + engine, _ := testutil.UnitTestWithEngine(t) + // Enable the workspace/applyEdit capability so the guard passes. + conf := engine.GetConfiguration() + caps := types.ClientCapabilities{} + caps.Workspace.ApplyEdit = true + conf.Set(types.SettingClientCapabilities, caps) + + notifier := &fakeNotifier{} + provider := &fakeFolderRemediator{} + + cmd, err := command.CreateFromCommandData( + context.Background(), + engine, + types.CommandData{CommandId: types.RemediationAgentFixFolderCommand, Arguments: []any{"file:///tmp"}}, + nil, // srv + nil, // authService + nil, // featureFlagService + nil, // learnService + notifier, + nil, // issueProvider + nil, // codeScanner + nil, // cli + nil, // ldxSyncService + nil, // configResolver + nil, // scanStateFunc + provider, + ) + require.NoError(t, err) + require.NotNil(t, cmd) + + // Execute the command with the folder path pointing to /tmp which exists. + // The provider returns nil (no changes) — we only care that the handler + // got a non-nil provider and notifier so no nil-pointer panic occurs. + _, execErr := cmd.Execute(context.Background()) + // /tmp exists and is a dir; provider is non-nil; returns nil (no changes). + assert.NoError(t, execErr, "provider and notifier must be wired; no nil-pointer panic") +} + +// fakeFolderRemediatorForFactory satisfies remediation.FolderRemediator for factory tests. +// Using the same fakeFolderRemediator defined in remediation_fix_folder_test.go — but that +// is in the same test package so it is accessible. +var _ remediation.FolderRemediator = (*fakeFolderRemediator)(nil) diff --git a/domain/ide/command/command_service.go b/domain/ide/command/command_service.go index c9a95c089..f874f244a 100644 --- a/domain/ide/command/command_service.go +++ b/domain/ide/command/command_service.go @@ -28,6 +28,7 @@ import ( "github.com/snyk/snyk-ls/domain/scanstates" "github.com/snyk/snyk-ls/domain/snyk" + "github.com/snyk/snyk-ls/domain/snyk/remediation" "github.com/snyk/snyk-ls/infrastructure/authentication" "github.com/snyk/snyk-ls/infrastructure/cli" "github.com/snyk/snyk-ls/infrastructure/code" @@ -40,34 +41,36 @@ import ( var instance types.CommandService type serviceImpl struct { - authService authentication.AuthenticationService - featureFlagService featureflag.Service - notifier noti.Notifier - learnService learn.Service - issueProvider snyk.IssueProvider - codeScanner *code.Scanner - cli cli.Executor - ldxSyncService LdxSyncService - configResolver types.ConfigResolverInterface - scanStateFunc func() scanstates.StateSnapshot - engine workflow.Engine - logger *zerolog.Logger + authService authentication.AuthenticationService + featureFlagService featureflag.Service + notifier noti.Notifier + learnService learn.Service + issueProvider snyk.IssueProvider + codeScanner *code.Scanner + cli cli.Executor + ldxSyncService LdxSyncService + configResolver types.ConfigResolverInterface + scanStateFunc func() scanstates.StateSnapshot + engine workflow.Engine + logger *zerolog.Logger + remediationProvider remediation.FolderRemediator } -func NewService(engine workflow.Engine, logger *zerolog.Logger, authService authentication.AuthenticationService, featureFlagService featureflag.Service, notifier noti.Notifier, learnService learn.Service, issueProvider snyk.IssueProvider, codeScanner *code.Scanner, cli cli.Executor, ldxSyncService LdxSyncService, configResolver types.ConfigResolverInterface, scanStateFunc func() scanstates.StateSnapshot) types.CommandService { +func NewService(engine workflow.Engine, logger *zerolog.Logger, authService authentication.AuthenticationService, featureFlagService featureflag.Service, notifier noti.Notifier, learnService learn.Service, issueProvider snyk.IssueProvider, codeScanner *code.Scanner, cli cli.Executor, ldxSyncService LdxSyncService, configResolver types.ConfigResolverInterface, scanStateFunc func() scanstates.StateSnapshot, remediationProvider remediation.FolderRemediator) types.CommandService { return &serviceImpl{ - authService: authService, - featureFlagService: featureFlagService, - notifier: notifier, - learnService: learnService, - issueProvider: issueProvider, - codeScanner: codeScanner, - cli: cli, - ldxSyncService: ldxSyncService, - configResolver: configResolver, - scanStateFunc: scanStateFunc, - engine: engine, - logger: logger, + authService: authService, + featureFlagService: featureFlagService, + notifier: notifier, + learnService: learnService, + issueProvider: issueProvider, + codeScanner: codeScanner, + cli: cli, + ldxSyncService: ldxSyncService, + configResolver: configResolver, + scanStateFunc: scanStateFunc, + engine: engine, + logger: logger, + remediationProvider: remediationProvider, } } @@ -90,7 +93,7 @@ func (s *serviceImpl) ExecuteCommandData(ctx context.Context, commandData types. logger.Debug().Msgf("executing command %s", commandData.CommandId) // TODO: move to DI - command, err := CreateFromCommandData(ctx, s.engine, commandData, server, s.authService, s.featureFlagService, s.learnService, s.notifier, s.issueProvider, s.codeScanner, s.cli, s.ldxSyncService, s.configResolver, s.scanStateFunc) + command, err := CreateFromCommandData(ctx, s.engine, commandData, server, s.authService, s.featureFlagService, s.learnService, s.notifier, s.issueProvider, s.codeScanner, s.cli, s.ldxSyncService, s.configResolver, s.scanStateFunc, s.remediationProvider) if err != nil { logger.Err(err).Msg("failed to create command") return nil, err diff --git a/domain/ide/command/command_service_test.go b/domain/ide/command/command_service_test.go index 708dd956e..536f1e293 100644 --- a/domain/ide/command/command_service_test.go +++ b/domain/ide/command/command_service_test.go @@ -33,7 +33,7 @@ func Test_ExecuteCommand(t *testing.T) { ExpectedAuthURL: "https://auth.url", } authenticationService := authentication.NewAuthenticationService(engine, tokenService, authProvider, nil, nil, resolver) - service := NewService(engine, engine.GetLogger(), authenticationService, nil, nil, nil, nil, nil, nil, NewLdxSyncService(resolver), nil, nil) + service := NewService(engine, engine.GetLogger(), authenticationService, nil, nil, nil, nil, nil, nil, NewLdxSyncService(resolver), nil, nil, nil) cmd := types.CommandData{ CommandId: types.CopyAuthLinkCommand, } diff --git a/domain/ide/command/export_for_test.go b/domain/ide/command/export_for_test.go new file mode 100644 index 000000000..d2bf5fb63 --- /dev/null +++ b/domain/ide/command/export_for_test.go @@ -0,0 +1,56 @@ +/* + * © 2026 Snyk Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package command + +import ( + "github.com/snyk/go-application-framework/pkg/workflow" + + "github.com/snyk/snyk-ls/domain/snyk/remediation" + "github.com/snyk/snyk-ls/internal/notification" + "github.com/snyk/snyk-ls/internal/types" +) + +// NewRemediationFixFolderCommand constructs a remediationFixFolderCommand for +// use in tests. It exposes the unexported struct so that unit tests in the +// command_test package can exercise Execute directly. +func NewRemediationFixFolderCommand( + cmd types.CommandData, + provider remediation.FolderRemediator, + notifier notification.Notifier, +) types.Command { + return &remediationFixFolderCommand{ + command: cmd, + notifier: notifier, + provider: provider, + } +} + +// NewRemediationFixFolderCommandWithEngine constructs a remediationFixFolderCommand +// with an engine for tests that exercise the ApplyEdit capability guard. +func NewRemediationFixFolderCommandWithEngine( + cmd types.CommandData, + provider remediation.FolderRemediator, + notifier notification.Notifier, + engine workflow.Engine, +) types.Command { + return &remediationFixFolderCommand{ + command: cmd, + notifier: notifier, + provider: provider, + engine: engine, + } +} diff --git a/domain/ide/command/remediation_fix_folder.go b/domain/ide/command/remediation_fix_folder.go new file mode 100644 index 000000000..59ea7ae11 --- /dev/null +++ b/domain/ide/command/remediation_fix_folder.go @@ -0,0 +1,97 @@ +/* + * © 2026 Snyk Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package command + +import ( + "context" + "errors" + "fmt" + "path/filepath" + + sglsp "github.com/sourcegraph/go-lsp" + + "github.com/snyk/go-application-framework/pkg/workflow" + + "github.com/snyk/snyk-ls/domain/ide/converter" + "github.com/snyk/snyk-ls/domain/snyk/remediation" + "github.com/snyk/snyk-ls/internal/notification" + "github.com/snyk/snyk-ls/internal/types" + "github.com/snyk/snyk-ls/internal/uri" +) + +// remediationFixFolderCommand implements workspace/executeCommand for +// snyk.remediationAgent.fixFolder. It validates the folder URI argument, runs +// the fix workflow directly in that folder (which is already an isolated git +// worktree created by the caller), and delivers any resulting edits to the +// client via workspace/applyEdit. The command is blocking — the caller waits +// for the full fix duration. +type remediationFixFolderCommand struct { + command types.CommandData + notifier notification.Notifier + provider remediation.FolderRemediator // nil when feature is off + engine workflow.Engine +} + +func (cmd *remediationFixFolderCommand) Command() types.CommandData { + return cmd.command +} + +func (cmd *remediationFixFolderCommand) Execute(ctx context.Context) (any, error) { + args := cmd.command.Arguments + if len(args) != 1 { + return nil, fmt.Errorf("snyk.remediationAgent.fixFolder: expected exactly one folder URI argument, got %d", len(args)) + } + folderURIStr, ok := args[0].(string) + if !ok || folderURIStr == "" { + return nil, fmt.Errorf("snyk.remediationAgent.fixFolder: folder URI argument must be a non-empty string") + } + + path := uri.PathFromUri(sglsp.DocumentURI(folderURIStr)) + pathStr := string(path) + if pathStr == "" || !filepath.IsAbs(pathStr) { + return nil, fmt.Errorf("snyk.remediationAgent.fixFolder: folder URI did not resolve to an absolute path: %q", folderURIStr) + } + if !uri.IsDirectory(path) { + return nil, fmt.Errorf("snyk.remediationAgent.fixFolder: folder does not exist or is not a directory: %q", pathStr) + } + + if cmd.provider == nil { + return nil, fmt.Errorf("snyk.remediationAgent.fixFolder: remediation agent is not enabled") + } + + if cmd.engine != nil { + key := types.SettingClientCapabilities + capabilities, _ := cmd.engine.GetConfiguration().Get(key).(types.ClientCapabilities) + if !capabilities.Workspace.ApplyEdit { + return nil, errors.New("snyk.remediationAgent.fixFolder: client does not support workspace/applyEdit capability") + } + } + + edit, err := cmd.provider.FixFolder(ctx, path) + if err != nil { + return nil, fmt.Errorf("snyk.remediationAgent.fixFolder: %w", err) + } + if edit == nil || len(edit.Changes) == 0 { + return nil, nil + } + + cmd.notifier.Send(types.ApplyWorkspaceEditParams{ + Label: "Snyk Remediation Agent fix", + Edit: converter.ToWorkspaceEdit(edit), + }) + return nil, nil +} diff --git a/domain/ide/command/remediation_fix_folder_test.go b/domain/ide/command/remediation_fix_folder_test.go new file mode 100644 index 000000000..4bee56e7f --- /dev/null +++ b/domain/ide/command/remediation_fix_folder_test.go @@ -0,0 +1,440 @@ +/* + * © 2026 Snyk Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package command_test + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "sync" + "sync/atomic" + "testing" + + sglsp "github.com/sourcegraph/go-lsp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/snyk/snyk-ls/domain/ide/command" + "github.com/snyk/snyk-ls/domain/snyk/remediation" + "github.com/snyk/snyk-ls/internal/testutil" + "github.com/snyk/snyk-ls/internal/types" + "github.com/snyk/snyk-ls/internal/uri" +) + +// --------------------------------------------------------------------------- +// Shared test infrastructure +// --------------------------------------------------------------------------- + +// fakeNotifier records all values sent via Send. +type fakeNotifier struct { + mu sync.Mutex + sent []any +} + +func (f *fakeNotifier) Send(msg any) { + f.mu.Lock() + defer f.mu.Unlock() + f.sent = append(f.sent, msg) +} + +func (f *fakeNotifier) SendShowMessage(_ sglsp.MessageType, _ string) {} +func (f *fakeNotifier) SendError(_ error) {} +func (f *fakeNotifier) SendErrorDiagnostic(_ types.FilePath, _ error) {} +func (f *fakeNotifier) Receive() (any, bool) { return nil, true } +func (f *fakeNotifier) CreateListener(_ func(params any)) {} +func (f *fakeNotifier) DisposeListener() {} + +func (f *fakeNotifier) Sent() []any { + f.mu.Lock() + defer f.mu.Unlock() + cp := make([]any, len(f.sent)) + copy(cp, f.sent) + return cp +} + +func (f *fakeNotifier) ApplyEditsSent() []types.ApplyWorkspaceEditParams { + var out []types.ApplyWorkspaceEditParams + for _, s := range f.Sent() { + if p, ok := s.(types.ApplyWorkspaceEditParams); ok { + out = append(out, p) + } + } + return out +} + +// fakeFolderRemediator is a fake FolderRemediator for handler unit tests. +type fakeFolderRemediator struct { + edit *types.WorkspaceEdit + err error +} + +func (f *fakeFolderRemediator) FixFolder(_ context.Context, _ types.FilePath) (*types.WorkspaceEdit, error) { + return f.edit, f.err +} + +// initGitRepo creates a minimal git repo in a temp dir for tests that need a +// real directory on disk. +func initGitRepoForCmd(t *testing.T) string { + t.Helper() + dir := t.TempDir() + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, string(out)) + } + run("init") + run("config", "user.email", "test@example.com") + run("config", "user.name", "Test") + // Commit an initial file so HEAD is valid. + f := filepath.Join(dir, "main.go") + require.NoError(t, os.WriteFile(f, []byte("package main\n"), 0644)) + run("add", ".") + run("commit", "-m", "init") + return dir +} + +// makeFixFolderCommandData constructs a CommandData for the fixFolder command. +func makeFixFolderCommandData(args ...any) types.CommandData { + return types.CommandData{ + CommandId: types.RemediationAgentFixFolderCommand, + Arguments: args, + } +} + +// --------------------------------------------------------------------------- +// Acceptance tests (ACC-001..005): round-trip through real serviceImpl +// --------------------------------------------------------------------------- + +// buildFixFolderService constructs a real serviceImpl wired with the given +// provider and notifier, routing through the real ExecuteCommandData → +// CreateFromCommandData dispatch. The engine has workspace/applyEdit enabled so +// the capability guard inside Execute passes. +func buildFixFolderService(t *testing.T, provider remediation.FolderRemediator, notifier *fakeNotifier) types.CommandService { + t.Helper() + engine, _ := testutil.UnitTestWithEngine(t) + conf := engine.GetConfiguration() + caps := types.ClientCapabilities{} + caps.Workspace.ApplyEdit = true + conf.Set(types.SettingClientCapabilities, caps) + logger := engine.GetLogger() + return command.NewService(engine, logger, nil, nil, notifier, nil, nil, nil, nil, nil, nil, nil, provider) +} + +// ACC-001: gate ON; fake runner edits a file; applyEdit sent with keys under folder. +func TestFixFolder_Acceptance_SendsApplyEditUnderPassedFolder(t *testing.T) { + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + + mainAbs := filepath.Join(repo, "main.go") + edit := &types.WorkspaceEdit{ + Changes: map[string][]types.TextEdit{ + mainAbs: {{Range: types.Range{}, NewText: "package main\nvar x = 2\n"}}, + }, + } + provider := &fakeFolderRemediator{edit: edit} + notifier := &fakeNotifier{} + svc := buildFixFolderService(t, provider, notifier) + + _, err := svc.ExecuteCommandData(context.Background(), makeFixFolderCommandData(folderURI), nil) + require.NoError(t, err) + + applyEdits := notifier.ApplyEditsSent() + require.Len(t, applyEdits, 1, "exactly one applyEdit must be sent") + for key := range applyEdits[0].Edit.Changes { + // Key must be under the passed folder (as a file:// URI or path). + // The notifier receives sglsp.WorkspaceEdit whose keys are document URIs. + assert.True(t, + len(key) >= len(repo) && key[:len(repo)] == repo || + len(key) >= len(folderURI) && key[:len(folderURI)] == folderURI, + "applyEdit key %q must be under passed folder %q", key, repo) + } +} + +// ACC-002: fake runner records the dir it was invoked with; no nested worktree. +func TestFixFolder_Acceptance_RunsInPassedFolderNoNestedWorktree(t *testing.T) { + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + + var invokedRoot string + var runnerCallCount int32 + provider := &trackingFolderRemediator{ + fn: func(_ context.Context, root types.FilePath) (*types.WorkspaceEdit, error) { + atomic.AddInt32(&runnerCallCount, 1) + invokedRoot = string(root) + return nil, nil + }, + } + notifier := &fakeNotifier{} + svc := buildFixFolderService(t, provider, notifier) + + _, err := svc.ExecuteCommandData(context.Background(), makeFixFolderCommandData(folderURI), nil) + require.NoError(t, err) + + assert.Equal(t, repo, invokedRoot, "provider must be invoked with the passed folder") + assert.Equal(t, int32(1), atomic.LoadInt32(&runnerCallCount)) + + // No nested snyk-remy-* or worktree dir inside the folder. + entries, _ := os.ReadDir(repo) + for _, e := range entries { + assert.NotContains(t, e.Name(), "snyk-remy-") + assert.NotContains(t, e.Name(), "wt") + } +} + +// trackingFolderRemediator is a FolderRemediator that delegates to a closure. +type trackingFolderRemediator struct { + fn func(ctx context.Context, root types.FilePath) (*types.WorkspaceEdit, error) +} + +func (tr *trackingFolderRemediator) FixFolder(ctx context.Context, root types.FilePath) (*types.WorkspaceEdit, error) { + return tr.fn(ctx, root) +} + +// ACC-003: gate OFF (provider nil) → error, no applyEdit, runner never invoked. +func TestFixFolder_Acceptance_FeatureOff_ReturnsError(t *testing.T) { + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + + notifier := &fakeNotifier{} + svc := buildFixFolderService(t, nil /* nil provider = feature off */, notifier) + + _, err := svc.ExecuteCommandData(context.Background(), makeFixFolderCommandData(folderURI), nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "not enabled") + assert.Empty(t, notifier.ApplyEditsSent(), "no applyEdit must be sent when feature is off") +} + +// ACC-004: folder OUTSIDE open workspace folders is accepted. +func TestFixFolder_Acceptance_ExternalFolderAccepted(t *testing.T) { + // The folder is outside any "workspace" — the command must not do a membership check. + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + + mainAbs := filepath.Join(repo, "main.go") + edit := &types.WorkspaceEdit{ + Changes: map[string][]types.TextEdit{ + mainAbs: {{Range: types.Range{}, NewText: "package main\nvar x = 99\n"}}, + }, + } + provider := &fakeFolderRemediator{edit: edit} + notifier := &fakeNotifier{} + svc := buildFixFolderService(t, provider, notifier) + + _, err := svc.ExecuteCommandData(context.Background(), makeFixFolderCommandData(folderURI), nil) + require.NoError(t, err, "external folder must be accepted") + applyEdits := notifier.ApplyEditsSent() + require.Len(t, applyEdits, 1) +} + +// ACC-005: gate ON; fake runner makes NO changes → no applyEdit sent. +func TestFixFolder_Acceptance_NoChanges_NoApplyEdit(t *testing.T) { + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + + provider := &fakeFolderRemediator{edit: nil} // nil = no changes + notifier := &fakeNotifier{} + svc := buildFixFolderService(t, provider, notifier) + + _, err := svc.ExecuteCommandData(context.Background(), makeFixFolderCommandData(folderURI), nil) + require.NoError(t, err) + assert.Empty(t, notifier.ApplyEditsSent(), "no applyEdit must be sent when there are no changes") +} + +// --------------------------------------------------------------------------- +// Unit tests for remediationFixFolderCommand.Execute (UNIT-006..010) +// --------------------------------------------------------------------------- + +// newFixFolderCmd constructs a remediationFixFolderCommand for unit testing via +// the exported constructor. +func newFixFolderCmd(args []any, provider remediation.FolderRemediator, notifier *fakeNotifier) types.Command { + return command.NewRemediationFixFolderCommand(types.CommandData{ + CommandId: types.RemediationAgentFixFolderCommand, + Arguments: args, + }, provider, notifier) +} + +// UNIT-006: wrong arg count → error. +func TestFixFolder_Execute_WrongArgCount(t *testing.T) { + notifier := &fakeNotifier{} + cmd := newFixFolderCmd([]any{}, &fakeFolderRemediator{}, notifier) + _, err := cmd.Execute(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "one folder URI argument") + assert.Empty(t, notifier.ApplyEditsSent()) +} + +// UNIT-007: invalid arg (empty / non-string) → error. +func TestFixFolder_Execute_InvalidArg(t *testing.T) { + notifier := &fakeNotifier{} + + // non-string arg + cmd := newFixFolderCmd([]any{42}, &fakeFolderRemediator{}, notifier) + _, err := cmd.Execute(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "non-empty string") + + // empty string arg + cmd2 := newFixFolderCmd([]any{""}, &fakeFolderRemediator{}, notifier) + _, err2 := cmd2.Execute(context.Background()) + require.Error(t, err2) + assert.Contains(t, err2.Error(), "non-empty string") + + assert.Empty(t, notifier.ApplyEditsSent()) +} + +// UNIT-008: URI for nonexistent path → "not a directory" error before dispatch. +func TestFixFolder_Execute_NonexistentFolder(t *testing.T) { + notifier := &fakeNotifier{} + nonexistent := "file:///tmp/this-path-should-not-exist-ever-12345xyz" + cmd := newFixFolderCmd([]any{nonexistent}, &fakeFolderRemediator{}, notifier) + _, err := cmd.Execute(context.Background()) + require.Error(t, err) + assert.Empty(t, notifier.ApplyEditsSent()) +} + +// UNIT-009: provider nil → "not enabled" error; no applyEdit. +func TestFixFolder_Execute_NilProviderReturnsError(t *testing.T) { + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + notifier := &fakeNotifier{} + cmd := newFixFolderCmd([]any{folderURI}, nil /* nil provider */, notifier) + _, err := cmd.Execute(context.Background()) + require.Error(t, err) + assert.Contains(t, err.Error(), "not enabled") + assert.Empty(t, notifier.ApplyEditsSent()) +} + +// UNIT-010: provider returns nil edit → (nil, nil); no applyEdit. +func TestFixFolder_Execute_NilEdit_NoApplyEdit(t *testing.T) { + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + notifier := &fakeNotifier{} + provider := &fakeFolderRemediator{edit: nil} + cmd := newFixFolderCmd([]any{folderURI}, provider, notifier) + result, err := cmd.Execute(context.Background()) + require.NoError(t, err) + assert.Nil(t, result) + assert.Empty(t, notifier.ApplyEditsSent(), "no applyEdit must be sent for nil edit") +} + +// --------------------------------------------------------------------------- +// Handler error propagation +// --------------------------------------------------------------------------- + +func TestFixFolder_Execute_ProviderError_Propagated(t *testing.T) { + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + notifier := &fakeNotifier{} + sentinel := errors.New("fix failed") + provider := &fakeFolderRemediator{err: sentinel} + cmd := newFixFolderCmd([]any{folderURI}, provider, notifier) + _, err := cmd.Execute(context.Background()) + require.Error(t, err) + assert.ErrorIs(t, err, sentinel) + assert.Empty(t, notifier.ApplyEditsSent()) +} + +// --------------------------------------------------------------------------- +// Fix 1: ApplyEdit capability guard (UNIT-011, UNIT-012) +// --------------------------------------------------------------------------- + +// newFixFolderCmdWithEngine constructs a remediationFixFolderCommand with an +// engine so that the capability guard can be exercised. +func newFixFolderCmdWithEngine(t *testing.T, args []any, provider remediation.FolderRemediator, notifier *fakeNotifier, applyEdit bool) types.Command { + t.Helper() + engine, _ := testutil.UnitTestWithEngine(t) + if applyEdit { + conf := engine.GetConfiguration() + caps := types.ClientCapabilities{} + caps.Workspace.ApplyEdit = true + conf.Set(types.SettingClientCapabilities, caps) + } + return command.NewRemediationFixFolderCommandWithEngine(types.CommandData{ + CommandId: types.RemediationAgentFixFolderCommand, + Arguments: args, + }, provider, notifier, engine) +} + +// UNIT-011: when ApplyEdit capability is false AND provider returns a non-empty +// edit, Execute must return a non-nil error and notifier.Send must NEVER be called. +func TestFixFolder_Execute_ApplyEditCapabilityFalse_ReturnsErrorNoSend(t *testing.T) { + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + notifier := &fakeNotifier{} + + mainAbs := filepath.Join(repo, "main.go") + edit := &types.WorkspaceEdit{ + Changes: map[string][]types.TextEdit{ + mainAbs: {{Range: types.Range{}, NewText: "package main\nvar x = 2\n"}}, + }, + } + provider := &fakeFolderRemediator{edit: edit} + + // Build command with ApplyEdit=false (capability not set) + cmd := newFixFolderCmdWithEngine(t, []any{folderURI}, provider, notifier, false /* applyEdit=false */) + _, err := cmd.Execute(context.Background()) + + require.Error(t, err, "must return error when ApplyEdit capability is false") + assert.Contains(t, err.Error(), "applyEdit", "error must mention the missing capability") + assert.Empty(t, notifier.ApplyEditsSent(), "notifier.Send must NOT be called when capability is absent") +} + +// UNIT-012b: when provider is nil AND client lacks applyEdit, Execute must +// return the "not enabled" error — NOT the capability error. The feature-gate +// check (provider nil) must precede the applyEdit capability check. +func TestFixFolder_Execute_NilProvider_BeforeCapabilityCheck(t *testing.T) { + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + notifier := &fakeNotifier{} + + // Build command with nil provider AND applyEdit=false. + cmd := newFixFolderCmdWithEngine(t, []any{folderURI}, nil /* nil provider */, notifier, false /* applyEdit=false */) + _, err := cmd.Execute(context.Background()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "not enabled", + "when provider is nil, Execute must return 'not enabled' regardless of applyEdit capability; got: %v", err) + assert.NotContains(t, err.Error(), "applyEdit", + "capability error must NOT be surfaced when the feature is off (provider nil)") +} + +// UNIT-012: when ApplyEdit capability is true AND provider returns a non-empty +// edit, Execute must succeed and notifier.Send must be called. +func TestFixFolder_Execute_ApplyEditCapabilityTrue_SendsEdit(t *testing.T) { + repo := initGitRepoForCmd(t) + folderURI := string(uri.PathToUri(types.FilePath(repo))) + notifier := &fakeNotifier{} + + mainAbs := filepath.Join(repo, "main.go") + edit := &types.WorkspaceEdit{ + Changes: map[string][]types.TextEdit{ + mainAbs: {{Range: types.Range{}, NewText: "package main\nvar x = 2\n"}}, + }, + } + provider := &fakeFolderRemediator{edit: edit} + + // Build command with ApplyEdit=true + cmd := newFixFolderCmdWithEngine(t, []any{folderURI}, provider, notifier, true /* applyEdit=true */) + _, err := cmd.Execute(context.Background()) + + require.NoError(t, err) + require.Len(t, notifier.ApplyEditsSent(), 1, "notifier.Send must be called exactly once when capability is present") +} diff --git a/domain/snyk/remediation/provider.go b/domain/snyk/remediation/provider.go index f44270b29..95176e60a 100644 --- a/domain/snyk/remediation/provider.go +++ b/domain/snyk/remediation/provider.go @@ -49,3 +49,19 @@ type RemediationProvider interface { type FileChangeNotifier interface { InvalidateFile(path types.FilePath) } + +// FolderRemediator runs the remediation fix workflow in place against an entire +// folder that is ALREADY an isolated git worktree (e.g. a detached-HEAD clone the +// caller created). It does NOT create a nested worktree. It returns a WorkspaceEdit +// whose file paths are keyed under root (root/); the caller delivers +// those edits to the client (e.g. via workspace/applyEdit). Returns (nil, nil) when +// the fix produces no changes. +// +// Precondition: root must be an absolute path to the git repository root (top +// level). Passing a subdirectory of a git repo is rejected with a clear error so +// the fix runner cannot silently escape its isolation boundary. The daemon caller +// always passes a detached-HEAD worktree root, and edits are keyed under that +// root so the caller can remap paths using the passed-folder prefix. +type FolderRemediator interface { + FixFolder(ctx context.Context, root types.FilePath) (*types.WorkspaceEdit, error) +} diff --git a/domain/snyk/remediation/remy.go b/domain/snyk/remediation/remy.go index 170c43508..24b2d626c 100644 --- a/domain/snyk/remediation/remy.go +++ b/domain/snyk/remediation/remy.go @@ -81,9 +81,10 @@ type remyProvider struct { rootMus map[string]*sync.Mutex } -// Ensure remyProvider satisfies both interfaces at compile time. +// Ensure remyProvider satisfies all interfaces at compile time. var _ RemediationProvider = (*remyProvider)(nil) var _ FileChangeNotifier = (*remyProvider)(nil) +var _ FolderRemediator = (*remyProvider)(nil) // gafRunner is the default remyRunner that invokes the legacycli workflow via // the Go Application Framework engine. The remy fix workflow is a Go extension @@ -215,6 +216,23 @@ func editsToEdit(filePath string, edits []types.TextEdit) *types.WorkspaceEdit { return &types.WorkspaceEdit{Changes: map[string][]types.TextEdit{filePath: edits}} } +// collectFixEdits snapshots tracked files in runDir, runs the fix workflow there, +// and builds TextEdits keyed under keyRoot. The per-finding path passes a freshly +// created worktree as runDir and the upstream repo root as keyRoot; the folder path +// passes the same already-isolated folder as both. findingID is forwarded to the +// runner so it can target a specific finding; pass "" for the folder path. +func (p *remyProvider) collectFixEdits(ctx context.Context, runDir, keyRoot, findingID string) (map[string][]types.TextEdit, error) { + snapshot, err := snapshotGitFiles(ctx, runDir) + if err != nil { + p.log.Debug().Err(err).Str("root", runDir).Msg("remy: failed to snapshot tracked files") + return nil, fmt.Errorf("remy: snapshot: %w", err) + } + if err = p.runner(ctx, p.engine, runDir, findingID); err != nil { + return nil, err + } + return buildWorkspaceEdits(ctx, p.log, runDir, keyRoot, snapshot) +} + // runRemyInWorktree creates an isolated git worktree, runs the remy runner, and // builds a WorkspaceEdit for all changed files. It owns the full worktree // lifecycle (creation and cleanup via defer). @@ -241,15 +259,45 @@ func (p *remyProvider) runRemyInWorktree(ctx context.Context, root string, req R _ = exec.CommandContext(cleanupCtx, "git", "-C", gitRoot, "worktree", "remove", "--force", worktreeDir).Run() _ = os.RemoveAll(tmpParent) }() - snapshot, err := snapshotGitFiles(ctx, worktreeDir) + return p.collectFixEdits(ctx, worktreeDir, gitRoot, req.FindingId) +} + +// FixFolder runs the remediation fix workflow directly in root (which must +// already be the git repository root — i.e. the top level of an isolated git +// worktree created by the caller). It does NOT create a nested worktree. +// It returns a WorkspaceEdit whose file paths are keyed under root, or +// (nil, nil) when the fix produces no changes. +// +// Precondition: root must be the git repository root (not a subdirectory). +// The daemon caller always passes a detached-HEAD worktree root, so edits are +// keyed under that root and the caller can remap paths using the passed-folder +// prefix. Passing a subdirectory is rejected with a clear error so the fix +// runner cannot silently escape its isolation boundary. +func (p *remyProvider) FixFolder(ctx context.Context, root types.FilePath) (*types.WorkspaceEdit, error) { + r := string(root) + if r == "" || !filepath.IsAbs(r) { + return nil, fmt.Errorf("remy: FixFolder requires an absolute path, got %q", r) + } + // Guard: r must be the git repository root. git rev-parse --show-prefix + // is symlink-safe: it outputs empty string when run at the repo root, and + // a non-empty relative path (e.g. "sub/") when run inside a subdirectory. + // If the command errors, the directory is not inside any git repository. + out, err := exec.CommandContext(ctx, "git", "-C", r, "rev-parse", "--show-prefix").Output() if err != nil { - p.log.Debug().Err(err).Str("root", worktreeDir).Msg("remy: failed to snapshot tracked files") - return nil, fmt.Errorf("remy: snapshot: %w", err) + return nil, fmt.Errorf("remy: FixFolder: %q is not inside a git repository: %w", r, err) + } + if prefix := strings.TrimSpace(string(out)); prefix != "" { + return nil, fmt.Errorf("remy: FixFolder: %q is a subdirectory of a git repository (prefix %q); the caller must pass the git repository root", r, prefix) } - if err = p.runner(ctx, p.engine, worktreeDir, req.FindingId); err != nil { + // findingID is "" because FixFolder targets the whole folder, not a single finding. + changes, err := p.collectFixEdits(ctx, r, r, "") + if err != nil { return nil, err } - return buildWorkspaceEdits(ctx, p.log, worktreeDir, gitRoot, snapshot) + if len(changes) == 0 { + return nil, nil + } + return &types.WorkspaceEdit{Changes: changes}, nil } // buildWorkspaceEdits enumerates files changed in the worktree relative to HEAD diff --git a/domain/snyk/remediation/remy_fix_folder_test.go b/domain/snyk/remediation/remy_fix_folder_test.go new file mode 100644 index 000000000..3167c994c --- /dev/null +++ b/domain/snyk/remediation/remy_fix_folder_test.go @@ -0,0 +1,262 @@ +/* + * © 2026 Snyk Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package remediation_test + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + + "github.com/snyk/go-application-framework/pkg/workflow" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/snyk/snyk-ls/domain/snyk/remediation" + "github.com/snyk/snyk-ls/internal/types" +) + +// --------------------------------------------------------------------------- +// UNIT-001: FixFolder returns edits keyed under the passed folder +// --------------------------------------------------------------------------- + +// TestFixFolder_ReturnsEditsKeyedUnderFolder verifies that when the fake runner +// modifies a tracked file inside the passed folder, FixFolder returns a +// WorkspaceEdit whose only key is /. +func TestFixFolder_ReturnsEditsKeyedUnderFolder(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "main.go", "package main\nvar x = 1\n") + + mainAbs := filepath.Join(repo, "main.go") + + runner := func(_ context.Context, _ workflow.Engine, root, findingID string) error { + // findingID must be empty for the folder path + assert.Empty(t, findingID, "runner must be called with empty findingID for folder path") + return os.WriteFile(filepath.Join(root, "main.go"), []byte("package main\nvar x = 2\n"), 0644) + } + + p := remediation.NewRemyProvider(nil, runner) + fr, ok := p.(remediation.FolderRemediator) + require.True(t, ok, "remyProvider must implement FolderRemediator") + + edit, err := fr.FixFolder(context.Background(), types.FilePath(repo)) + require.NoError(t, err) + require.NotNil(t, edit, "expected a WorkspaceEdit when a tracked file was modified") + assert.Contains(t, edit.Changes, mainAbs, "edit key must be /") + // Every key in the edit must be under the passed folder. + for key := range edit.Changes { + assert.True(t, len(key) > len(repo) && key[:len(repo)] == repo, + "edit key %q must be under passed folder %q", key, repo) + } +} + +// --------------------------------------------------------------------------- +// UNIT-002: FixFolder returns (nil, nil) when runner makes no changes +// --------------------------------------------------------------------------- + +func TestFixFolder_NoChangesReturnsNil(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "main.go", "package main\nvar x = 1\n") + + p := remediation.NewRemyProvider(nil, noopRunner) + fr, ok := p.(remediation.FolderRemediator) + require.True(t, ok) + + edit, err := fr.FixFolder(context.Background(), types.FilePath(repo)) + require.NoError(t, err) + assert.Nil(t, edit, "no-change run must return nil edit") +} + +// --------------------------------------------------------------------------- +// UNIT-003: FixFolder propagates runner errors +// --------------------------------------------------------------------------- + +func TestFixFolder_PropagatesRunnerError(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "main.go", "package main\n") + + sentinel := errors.New("remy runner failed") + p := remediation.NewRemyProvider(nil, errRunner(sentinel)) + fr, ok := p.(remediation.FolderRemediator) + require.True(t, ok) + + edit, err := fr.FixFolder(context.Background(), types.FilePath(repo)) + require.Error(t, err) + assert.ErrorIs(t, err, sentinel) + assert.Nil(t, edit) +} + +// --------------------------------------------------------------------------- +// UNIT-004: FixFolder rejects non-absolute / empty paths +// --------------------------------------------------------------------------- + +func TestFixFolder_RejectsNonAbsolutePath(t *testing.T) { + var runnerCalled bool + trackingRunner := func(_ context.Context, _ workflow.Engine, _, _ string) error { + runnerCalled = true + return nil + } + + p := remediation.NewRemyProvider(nil, trackingRunner) + fr, ok := p.(remediation.FolderRemediator) + require.True(t, ok) + + // Relative path + edit, err := fr.FixFolder(context.Background(), types.FilePath("relative/path")) + require.Error(t, err) + assert.Contains(t, err.Error(), "absolute") + assert.Nil(t, edit) + assert.False(t, runnerCalled, "runner must NOT be called on invalid path") + + // Empty path + edit2, err2 := fr.FixFolder(context.Background(), types.FilePath("")) + require.Error(t, err2) + assert.Contains(t, err2.Error(), "absolute") + assert.Nil(t, edit2) + assert.False(t, runnerCalled, "runner must NOT be called on empty path") +} + +// --------------------------------------------------------------------------- +// UNIT-005a: FixFolder rejects a subdirectory of a git repo with an error +// --------------------------------------------------------------------------- + +// TestFixFolder_SubdirOfGitRoot_ReturnEdits verifies that FixFolder returns a +// non-nil error (and no edits) when the passed path is a subdirectory of the +// git root. The contract requires the caller to pass the git repository root +// itself (e.g. a detached-HEAD worktree created by the daemon); passing a +// subdirectory is rejected so the fix runner cannot silently escape its +// isolation boundary. +func TestFixFolder_SubdirOfGitRoot_ReturnEdits(t *testing.T) { + // Create a git repo with a tracked file inside a subdirectory. + repo := initGitRepo(t) + commitFile(t, repo, "sub/main.go", "package main\nvar x = 1\n") + + subdir := filepath.Join(repo, "sub") + + var runnerCalled bool + runner := func(_ context.Context, _ workflow.Engine, _, _ string) error { + runnerCalled = true + return nil + } + + p := remediation.NewRemyProvider(nil, runner) + fr, ok := p.(remediation.FolderRemediator) + require.True(t, ok, "remyProvider must implement FolderRemediator") + + // Call FixFolder with the SUBDIRECTORY path — must return an error. + edit, err := fr.FixFolder(context.Background(), types.FilePath(subdir)) + require.Error(t, err, "FixFolder must return an error when passed a subdirectory of a git root") + assert.Nil(t, edit, "FixFolder must return nil edit when returning an error") + assert.False(t, runnerCalled, "runner must NOT be called when the precondition guard fires") +} + +// --------------------------------------------------------------------------- +// UNIT-005b: FixFolder rejects a directory that is not a git repository +// --------------------------------------------------------------------------- + +// TestFixFolder_NonGitDirectory_ReturnsError verifies that FixFolder returns a +// non-nil error when the passed path is not inside any git repository. +func TestFixFolder_NonGitDirectory_ReturnsError(t *testing.T) { + // Use a temp dir that has no git init — it is not a git repo. + nonGit := t.TempDir() + + var runnerCalled bool + runner := func(_ context.Context, _ workflow.Engine, _, _ string) error { + runnerCalled = true + return nil + } + + p := remediation.NewRemyProvider(nil, runner) + fr, ok := p.(remediation.FolderRemediator) + require.True(t, ok) + + edit, err := fr.FixFolder(context.Background(), types.FilePath(nonGit)) + require.Error(t, err, "FixFolder must return an error for a non-git directory") + assert.Nil(t, edit) + assert.False(t, runnerCalled, "runner must NOT be called when the precondition guard fires") +} + +// TestFixFolder_GitRoot_EditsKeyedUnderPassedRoot asserts the daemon contract: +// when the passed folder IS the git root, edits are keyed under that folder. +func TestFixFolder_GitRoot_EditsKeyedUnderPassedRoot(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "main.go", "package main\nvar x = 1\n") + + mainAbs := filepath.Join(repo, "main.go") + + runner := func(_ context.Context, _ workflow.Engine, root, _ string) error { + return os.WriteFile(filepath.Join(root, "main.go"), []byte("package main\nvar x = 2\n"), 0644) + } + + p := remediation.NewRemyProvider(nil, runner) + fr, ok := p.(remediation.FolderRemediator) + require.True(t, ok) + + edit, err := fr.FixFolder(context.Background(), types.FilePath(repo)) + require.NoError(t, err) + require.NotNil(t, edit) + // Daemon contract: key must be under the passed folder (repo root here). + assert.Contains(t, edit.Changes, mainAbs) + for key := range edit.Changes { + assert.True(t, len(key) > len(repo) && key[:len(repo)] == repo, + "edit key %q must be under passed folder %q", key, repo) + } +} + +// --------------------------------------------------------------------------- +// UNIT-005: FixFolder runs directly in the passed folder (no nested worktree) +// --------------------------------------------------------------------------- + +func TestFixFolder_RunsDirectlyInPassedFolder(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "main.go", "package main\n") + + var invokedWith string + trackingRunner := func(_ context.Context, _ workflow.Engine, root, _ string) error { + invokedWith = root + return nil + } + + p := remediation.NewRemyProvider(nil, trackingRunner) + fr, ok := p.(remediation.FolderRemediator) + require.True(t, ok) + + _, err := fr.FixFolder(context.Background(), types.FilePath(repo)) + require.NoError(t, err) + + // Runner must be invoked with exactly the passed folder. + assert.Equal(t, repo, invokedWith, "runner must be invoked with the passed folder, not a child dir") + + // No snyk-remy-* temp directories must have been created inside the folder. + entries, readErr := os.ReadDir(repo) + require.NoError(t, readErr) + for _, e := range entries { + assert.NotContains(t, e.Name(), "snyk-remy-", + "no nested snyk-remy-* temp dir must be created inside the passed folder") + } + + // Also assert no worktree child exists as a sibling with "wt" suffix. + parent := filepath.Dir(repo) + siblings, _ := os.ReadDir(parent) + for _, s := range siblings { + if s.IsDir() && s.Name() != filepath.Base(repo) { + assert.NotContains(t, s.Name(), "wt", + "no nested worktree dir must be created as a sibling of the passed folder") + } + } +} diff --git a/internal/types/command.go b/internal/types/command.go index d8260bb9d..f023b2308 100644 --- a/internal/types/command.go +++ b/internal/types/command.go @@ -58,6 +58,9 @@ const ( DismissFeedbackBanner = "snyk.dismissFeedbackBanner" FeedbackBannerInteracted = "snyk.feedbackBannerInteracted" + // Remediation agent commands + RemediationAgentFixFolderCommand = "snyk.remediationAgent.fixFolder" + // bridging commands ExecuteCLICommand = "snyk.executeCLI" diff --git a/internal/types/command_remediation_test.go b/internal/types/command_remediation_test.go new file mode 100644 index 000000000..dde8106a7 --- /dev/null +++ b/internal/types/command_remediation_test.go @@ -0,0 +1,33 @@ +/* + * © 2026 Snyk Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package types_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/snyk/snyk-ls/internal/types" +) + +// TestRemediationFixFolderCommand_ConstantValue asserts the exact wire-format +// string consumed by the Ambient Canary daemon. This string is part of the +// cross-language contract and must never change without coordinating with the +// daemon consumer. +func TestRemediationFixFolderCommand_ConstantValue(t *testing.T) { + assert.Equal(t, "snyk.remediationAgent.fixFolder", types.RemediationAgentFixFolderCommand) +} From 87adbe825854ec8db31d4d94b77b4427d11c93a2 Mon Sep 17 00:00:00 2001 From: Bastian Doetsch Date: Mon, 29 Jun 2026 08:43:51 +0000 Subject: [PATCH 3/4] fix(remediation): detect cache staleness by content hash, not mtime [IDE-2052] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The remy provider's per-file cache marked an entry stale only when a cached file's mtime was strictly after the entry's wall-clock creation time. On filesystems with coarse mtime granularity, a file modified just after the entry was created could carry an mtime that, once truncated, sorted before the nanosecond-precise creation time — so a genuinely changed file was treated as fresh and stale edits were retained. This made TestCacheValid_StaleFile fail nondeterministically on CI. Replace the timestamp comparison with a per-file SHA-256 content hash captured when the entry is populated and recompared on each lookup: any content change (including same-length edits that mtime/size checks miss) now reliably invalidates the entry, independent of filesystem timestamp resolution. The requested file's presence is checked before any hashing so a miss does no I/O, an empty entry is never stored, and the content and hash maps are kept in sync on every mutation. Also set core.checkStat=minimal in the fixFolder git test helpers (matching the remy test helper) to avoid an overlay-filesystem write-ordering flake when reading freshly written git objects in CI. --- application/di/init_fix_folder_test.go | 6 + .../command/remediation_fix_folder_test.go | 6 + domain/snyk/remediation/export_for_test.go | 27 +++ domain/snyk/remediation/remy.go | 118 +++++++--- domain/snyk/remediation/remy_provider_test.go | 215 +++++++++++++++++- domain/snyk/remediation/remy_test.go | 41 +++- 6 files changed, 378 insertions(+), 35 deletions(-) diff --git a/application/di/init_fix_folder_test.go b/application/di/init_fix_folder_test.go index c71f09bd9..b688e9b71 100644 --- a/application/di/init_fix_folder_test.go +++ b/application/di/init_fix_folder_test.go @@ -134,6 +134,12 @@ func initGitRepoForDI(t *testing.T) string { run("init") run("config", "user.email", "test@example.com") run("config", "user.name", "Test") + // Overlay filesystems (e.g. Docker on Linux) have write-ordering delays that + // can cause git to report "not a valid object" when the object database is + // read immediately after a write. core.checkStat=minimal tells git not to + // recheck filesystem timestamps for objects it has already cached in memory, + // which suppresses the false-negative reads on overlayfs. + run("config", "core.checkStat", "minimal") f := filepath.Join(dir, "main.go") require.NoError(t, os.WriteFile(f, []byte("package main\n"), 0644)) run("add", ".") diff --git a/domain/ide/command/remediation_fix_folder_test.go b/domain/ide/command/remediation_fix_folder_test.go index 4bee56e7f..f371bce7c 100644 --- a/domain/ide/command/remediation_fix_folder_test.go +++ b/domain/ide/command/remediation_fix_folder_test.go @@ -103,6 +103,12 @@ func initGitRepoForCmd(t *testing.T) string { run("init") run("config", "user.email", "test@example.com") run("config", "user.name", "Test") + // Overlay filesystems (e.g. Docker on Linux) have write-ordering delays that + // can cause git to report "not a valid object" when the object database is + // read immediately after a write. core.checkStat=minimal tells git not to + // recheck filesystem timestamps for objects it has already cached in memory, + // which suppresses the false-negative reads on overlayfs. + run("config", "core.checkStat", "minimal") // Commit an initial file so HEAD is valid. f := filepath.Join(dir, "main.go") require.NoError(t, os.WriteFile(f, []byte("package main\n"), 0644)) diff --git a/domain/snyk/remediation/export_for_test.go b/domain/snyk/remediation/export_for_test.go index f8e2aac56..99c1e1642 100644 --- a/domain/snyk/remediation/export_for_test.go +++ b/domain/snyk/remediation/export_for_test.go @@ -23,3 +23,30 @@ import "github.com/snyk/snyk-ls/internal/types" func ExportedWorkspaceEditFromContent(absPath string, originalContent []byte, diff string) (*types.WorkspaceEdit, error) { return workspaceEditFromContent(absPath, originalContent, diff) } + +// InjectCacheEntry inserts a synthetic cache entry into the provider's cache +// for testing. changes maps file paths to TextEdits; storedHashes maps file +// paths to pre-computed hashes. Pass an empty storedHashes map to simulate +// the case where hash recording failed at populate time (e.g. the file was +// unreadable). The provider must have been constructed via NewRemyProvider. +// Keys in the injected storedHashes map should be a subset of the changes-map +// keys; any extra hash keys beyond those in changes are unreachable and ignored. +func InjectCacheEntry(p RemediationProvider, root string, changes map[string][]types.TextEdit, storedHashes map[string]string) { + rp := p.(*remyProvider) + rp.cacheMu.Lock() + defer rp.cacheMu.Unlock() + rp.cache[root] = &remyCacheEntry{ + changes: changes, + fileHashes: storedHashes, + } +} + +// CacheLen returns the number of root entries currently held in the provider's +// cache. It is used by tests to assert that populateCache did not store an +// empty entry (Change 2: ghost-entry fix). +func CacheLen(p RemediationProvider) int { + rp := p.(*remyProvider) + rp.cacheMu.Lock() + defer rp.cacheMu.Unlock() + return len(rp.cache) +} diff --git a/domain/snyk/remediation/remy.go b/domain/snyk/remediation/remy.go index 24b2d626c..f74143cc9 100644 --- a/domain/snyk/remediation/remy.go +++ b/domain/snyk/remediation/remy.go @@ -21,7 +21,9 @@ package remediation import ( "bytes" "context" + "crypto/sha256" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -56,10 +58,12 @@ type RemyOptions struct { // remyCacheEntry holds the changes produced by a single remy run that have not // yet been delivered to a code action. Entries are keyed by absolute workspace -// path. createdAt is used for mtime-based eviction. +// path. fileHashes records a SHA-256 content fingerprint for each cached file +// path at the time the entry was populated; cacheValid uses these to detect any +// content change on disk regardless of filesystem mtime granularity. type remyCacheEntry struct { - changes map[string][]types.TextEdit - createdAt time.Time + changes map[string][]types.TextEdit + fileHashes map[string]string // abs path → hex SHA-256 of file contents at cache time } // remyProvider is the concrete RemediationProvider that drives the host snyk @@ -138,23 +142,61 @@ func (p *remyProvider) getOrCreateRootMu(root string) *sync.Mutex { return mu } -// tryServeFromCache looks up the cache for root/filePath. Holds cacheMu for -// the full read-validate-consume cycle so that cacheValid (which iterates -// entry.changes) and InvalidateFile (which deletes from entry.changes) are -// never concurrent — preventing a data race on the shared map. +// cacheValid reports whether every file in entry.changes still has the same +// content it had when the entry was populated. It must be called with p.cacheMu +// held. For each path it first looks up the stored hash; if no stored hash is +// present the entry is immediately considered stale (the hash was not recorded +// at populate time, so we cannot verify the file). If a hash is present, the +// current file content is hashed via fileHash; a read error or content mismatch +// also signals stale. The prior implementation used os.Stat under this same +// lock for these same small tracked files, so performing content hashing here +// is consistent with the existing design. +func cacheValid(entry *remyCacheEntry) bool { + for path := range entry.changes { + stored, known := entry.fileHashes[path] + if !known { + // No stored hash — hash recording failed at populate time; treat as stale. + return false + } + cur, err := fileHash(path) + if err != nil || cur != stored { + return false + } + } + return true +} + +// tryServeFromCache looks up the cache for root/filePath. Under p.cacheMu it +// first checks whether the entry exists and whether filePath is present in the +// entry's changes map — an absent filePath is an immediate miss with no I/O. +// Only after confirming filePath is cached does it call cacheValid (which reads +// and hashes every remaining file in the entry). If the entry is stale it is +// evicted and a miss is returned. On a valid hit the edits for filePath are +// consumed (deleted from both entry.changes and entry.fileHashes to keep the +// maps in sync) and the entry is evicted when no changes remain. +// // Returns (edits, true) on a valid cache hit, (nil, false) otherwise. func (p *remyProvider) tryServeFromCache(root, filePath string) ([]types.TextEdit, bool) { p.cacheMu.Lock() defer p.cacheMu.Unlock() entry, exists := p.cache[root] - if !exists || !p.cacheValid(entry) { + if !exists { return nil, false } edits, ok := entry.changes[filePath] if !ok { + // filePath is not in this entry — return a miss without hashing any files. + return nil, false + } + if !cacheValid(entry) { + delete(p.cache, root) return nil, false } delete(entry.changes, filePath) + delete(entry.fileHashes, filePath) + if len(entry.changes) == 0 { + delete(p.cache, root) + } return edits, true } @@ -339,31 +381,68 @@ func buildWorkspaceEdits(ctx context.Context, log zerolog.Logger, worktreeDir, g return allChanges, nil } +// fileHash returns the hex-encoded SHA-256 of the file at path, or an error if +// the file cannot be read. +func fileHash(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", err + } + defer f.Close() + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return "", err + } + return fmt.Sprintf("%x", h.Sum(nil)), nil +} + // populateCache stores changes for all files except filePath in the cache so // that subsequent code action resolutions can be served without re-running remy. +// It also captures a SHA-256 content fingerprint for each cached file so that +// cacheValid can detect modifications independently of filesystem mtime granularity. +// If fileHash fails for a path (e.g. the file is temporarily unreadable), no hash +// is stored for that path; cacheValid treats a missing hash as an immediate stale +// signal, which is safe. +// +// If excluding filePath leaves the entry's changes map empty (i.e. remy only +// touched the requested file), no entry is stored. An empty entry would linger +// in the cache indefinitely: cacheValid is vacuously true with no files to check, +// yet there is nothing to serve and the evict-when-empty path in tryServeFromCache +// is never reached. func (p *remyProvider) populateCache(root, filePath string, allChanges map[string][]types.TextEdit) { - p.cacheMu.Lock() - defer p.cacheMu.Unlock() entry := &remyCacheEntry{ - changes: make(map[string][]types.TextEdit, len(allChanges)-1), - createdAt: time.Now(), + changes: make(map[string][]types.TextEdit, len(allChanges)-1), + fileHashes: make(map[string]string, len(allChanges)-1), } for k, v := range allChanges { if k != filePath { entry.changes[k] = v + if h, err := fileHash(k); err == nil { + entry.fileHashes[k] = h + } } } + if len(entry.changes) == 0 { + // Nothing to cache — storing an empty entry would create a ghost that is + // vacuously valid but can never be served. + return + } + p.cacheMu.Lock() p.cache[root] = entry + p.cacheMu.Unlock() } // InvalidateFile removes cached diffs for path from every cache entry so that // the next Remediate call for that file re-runs remy rather than serving stale -// results. It is called by the LSP textDocument/didChange handler. +// results. It is called by the LSP textDocument/didChange handler. The +// corresponding fileHashes entry is deleted alongside the changes entry so +// both maps stay in sync. func (p *remyProvider) InvalidateFile(path types.FilePath) { p.cacheMu.Lock() defer p.cacheMu.Unlock() for root, entry := range p.cache { delete(entry.changes, string(path)) + delete(entry.fileHashes, string(path)) // Remove the cache entry entirely once it has no remaining changes so // it does not occupy memory or cause vacuously-valid hits. if len(entry.changes) == 0 { @@ -372,19 +451,6 @@ func (p *remyProvider) InvalidateFile(path types.FilePath) { } } -// cacheValid returns false if any file in the entry has been modified on disk -// since the entry was created, indicating the cached diffs are stale. -// Caller must hold p.cacheMu. -func (p *remyProvider) cacheValid(entry *remyCacheEntry) bool { - for path := range entry.changes { - info, err := os.Stat(path) - if err != nil || info.ModTime().After(entry.createdAt) { - return false - } - } - return true -} - // resolveGitRoot returns the root of the git repository containing path by // running git rev-parse --show-toplevel. This is necessary when path is a // subdirectory of a git repo (e.g. a monorepo package folder) so that worktree diff --git a/domain/snyk/remediation/remy_provider_test.go b/domain/snyk/remediation/remy_provider_test.go index 1cdeb28a2..b8a7d9a4f 100644 --- a/domain/snyk/remediation/remy_provider_test.go +++ b/domain/snyk/remediation/remy_provider_test.go @@ -452,11 +452,65 @@ func TestNewRemyProvider_WithEngine(t *testing.T) { } // --------------------------------------------------------------------------- -// cacheValid — stale file eviction (mtime check) +// cacheValid — stale file eviction (content-hash check) // --------------------------------------------------------------------------- +// TestCacheValid_SameSizeContentChange verifies that when a cached file is +// overwritten with different content of identical byte length (the case that +// mtime- and size-based checks both miss), cacheValid detects the change via +// content hash and forces a re-run. This is the exact failure mode that caused +// the CI flake: coarse filesystem mtimes (1-second granularity) meant the mtime +// of the freshly-written file was ≤ the ns-precise createdAt, so .After() +// returned false and the cache was incorrectly considered valid. +// +//nolint:dupl // parallel test to TestCacheValid_StaleFile; same setup, different mutation (same-size content swap) +func TestCacheValid_SameSizeContentChange(t *testing.T) { + repo := initGitRepo(t) + // Both files have the same byte length so size-based checks would pass. + commitFile(t, repo, "foo.go", "package main\nvar x = 1\n") + commitFile(t, repo, "bar.go", "package main\nvar y = 1\n") + + fooAbs := filepath.Join(repo, "foo.go") + barAbs := filepath.Join(repo, "bar.go") + + var callCount int32 + runner := func(_ context.Context, _ workflow.Engine, root, _ string) error { + atomic.AddInt32(&callCount, 1) + if err := os.WriteFile(filepath.Join(root, "foo.go"), []byte("package main\nvar x = 2\n"), 0644); err != nil { + return err + } + return os.WriteFile(filepath.Join(root, "bar.go"), []byte("package main\nvar y = 2\n"), 0644) + } + + p := remediation.NewRemyProvider(nil, runner) + + // First call: foo.go requested, bar.go cached. + _, err := p.Remediate(context.Background(), remediation.RemediationRequest{ + FindingId: "f1", + ContentRoot: types.FilePath(repo), + FilePath: types.FilePath(fooAbs), + }) + require.NoError(t, err) + require.Equal(t, int32(1), atomic.LoadInt32(&callCount)) + + // Overwrite bar.go with the same byte length but different content. + // "var y = 1\n" → "var y = 9\n" — identical length, different hash. + require.NoError(t, os.WriteFile(barAbs, []byte("package main\nvar y = 9\n"), 0644)) + + // Second call for bar.go: content hash differs → cache stale → runner re-invoked. + _, err = p.Remediate(context.Background(), remediation.RemediationRequest{ + FindingId: "f1", + ContentRoot: types.FilePath(repo), + FilePath: types.FilePath(barAbs), + }) + require.NoError(t, err) + assert.Equal(t, int32(2), atomic.LoadInt32(&callCount), "same-size content change must force re-run") +} + // TestCacheValid_StaleFile verifies that when a cached file is modified after // the cache was populated, tryServeFromCache evicts the entry and returns false. +// +//nolint:dupl // parallel test to TestCacheValid_SameSizeContentChange; same setup, different mutation (size change) func TestCacheValid_StaleFile(t *testing.T) { repo := initGitRepo(t) commitFile(t, repo, "foo.go", "package main\nvar x = 1\n") @@ -485,7 +539,7 @@ func TestCacheValid_StaleFile(t *testing.T) { require.NoError(t, err) require.Equal(t, int32(1), atomic.LoadInt32(&callCount)) - // Modify bar.go on disk so that cacheValid returns false (mtime check). + // Modify bar.go on disk so that the content hash changes. require.NoError(t, os.WriteFile(barAbs, []byte("package main\nvar y = 99\n"), 0644)) // Second call for bar.go: cache entry is stale → runner re-invoked. @@ -592,6 +646,56 @@ func TestWorkspaceEditFromContent_ConsecutiveInsertions(t *testing.T) { assert.True(t, found, "expected at least one insertion TextEdit") } +// TestCacheValid_MissingStoredHash_TreatsEntryAsStale verifies that when a +// cached entry has no stored hash for a file (simulating a populate-time +// fileHash failure — e.g. the file was temporarily unreadable when the cache +// was built), the cache correctly treats the entry as stale on the next call. +// The runner must be re-invoked rather than serving from the (partially +// populated) cache entry. +// +// This uses InjectCacheEntry to directly synthesize the "missing hash" state +// because some container filesystems ignore chmod 0000 for the file owner, +// making permission-based simulation unreliable. +func TestCacheValid_MissingStoredHash_TreatsEntryAsStale(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "foo.go", "package main\nvar x = 1\n") + commitFile(t, repo, "bar.go", "package main\nvar y = 1\n") + + barAbs := filepath.Join(repo, "bar.go") + + var callCount int32 + runner := func(_ context.Context, _ workflow.Engine, root, _ string) error { + atomic.AddInt32(&callCount, 1) + if err := os.WriteFile(filepath.Join(root, "foo.go"), []byte("package main\nvar x = 2\n"), 0644); err != nil { + return err + } + return os.WriteFile(filepath.Join(root, "bar.go"), []byte("package main\nvar y = 2\n"), 0644) + } + + p := remediation.NewRemyProvider(nil, runner) + + // Inject a synthetic cache entry for repo: bar.go has a pending edit but + // NO stored hash (simulating a fileHash failure at populate time). + // An empty storedHashes map means entry.fileHashes[barAbs] is absent. + remediation.InjectCacheEntry(p, repo, + map[string][]types.TextEdit{ + barAbs: {{Range: types.Range{}, NewText: "package main\nvar y = 2\n"}}, + }, + map[string]string{}, // no stored hashes — simulates populate-time read failure + ) + + // At this point callCount=0. The second Remediate call for bar.go must detect + // the missing stored hash and treat the entry as stale, re-running the runner. + _, err := p.Remediate(context.Background(), remediation.RemediationRequest{ + FindingId: "f1", + ContentRoot: types.FilePath(repo), + FilePath: types.FilePath(barAbs), + }) + require.NoError(t, err) + assert.Equal(t, int32(1), atomic.LoadInt32(&callCount), + "missing stored hash must be treated as stale immediately, forcing a re-run") +} + // TestWorkspaceEditFromContent_NoDiffHunks_ReturnsNil verifies that a diff // with no actionable hunks (only headers) results in nil. func TestWorkspaceEditFromContent_NoDiffHunks_ReturnsNil(t *testing.T) { @@ -658,3 +762,110 @@ func TestGetOrCreateRootMu_DifferentRoots(t *testing.T) { require.NoError(t, errs[0]) require.NoError(t, errs[1]) } + +// --------------------------------------------------------------------------- +// Change 1: miss path must not hash sibling files +// --------------------------------------------------------------------------- + +// TestTryServeFromCache_MissSkipsHashing verifies that when the requested +// filePath is absent from a cache entry, tryServeFromCache returns a miss +// without hashing (and thus without evicting) the sibling files that ARE +// cached. Proof: the sibling file in entry.changes has a deliberately wrong +// stored hash, yet the absent-filePath lookup still returns (nil, false) and +// the entry is not evicted — a subsequent serve for the sibling is still +// resolved from cache (because the hash is only evaluated when filePath IS +// found in the entry). +// +// Before Change 1 (old order), cacheValid ran before the filePath membership +// check, so the wrong stored hash would cause eviction on any miss for an +// unrelated path. After the reorder, the file-presence check comes first and +// an absent filePath returns early before any hashing is done. +func TestTryServeFromCache_MissSkipsHashing(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "sibling.go", "package main\nvar s = 1\n") + + siblingAbs := filepath.Join(repo, "sibling.go") + absentAbs := filepath.Join(repo, "absent.go") // never committed, not in entry + + p := remediation.NewRemyProvider(nil, noopRunner) + + // Compute the real hash of sibling.go so the entry looks valid from a + // content perspective — but then we'll use a deliberately WRONG hash to + // prove the miss path never reads the file. + wrongHash := "0000000000000000000000000000000000000000000000000000000000000000" + + // Inject a cache entry: sibling.go is present with a wrong stored hash. + // Under the OLD code (cacheValid before filePath check), a lookup for + // absentAbs would call cacheValid, detect the hash mismatch, evict the + // entry, and return false — but ALSO destroy the sibling entry. + // Under the NEW code (filePath check first), the lookup for absentAbs + // returns false immediately without touching the sibling entry. + remediation.InjectCacheEntry(p, repo, + map[string][]types.TextEdit{ + siblingAbs: {{Range: types.Range{}, NewText: "package main\nvar s = 2\n"}}, + }, + map[string]string{ + siblingAbs: wrongHash, // wrong hash — cacheValid would evict if called + }, + ) + + // Lookup for the absent file: must be a miss (filePath not in entry.changes). + // After Change 1 the lookup returns early, so the wrong hash is never read + // and the sibling entry is NOT evicted. + // + // We assert the miss indirectly: CacheLen must still be 1 (entry retained), + // which can only be true if the wrong-hash path was never triggered. + edit, err := p.Remediate(context.Background(), remediation.RemediationRequest{ + FindingId: "f-absent", + ContentRoot: types.FilePath(repo), + FilePath: types.FilePath(absentAbs), + }) + // The absent file has no cached edits and no committed content, so the runner + // fires (noopRunner → no changes) and returns (nil, nil). + require.NoError(t, err) + assert.Nil(t, edit) + + // The sibling entry must still be present in the cache (not evicted by the + // miss path). If Change 1 is NOT applied, cacheValid would have evicted the + // entry, and CacheLen would be 0 here. + assert.Equal(t, 1, remediation.CacheLen(p), + "miss for absent filePath must not evict the cache entry for the root") +} + +// --------------------------------------------------------------------------- +// Change 2: empty-entry ghost-fix — populateCache must not store an empty entry +// --------------------------------------------------------------------------- + +// TestPopulateCache_SingleFileFix_NoGhostEntry verifies that when remy only +// modifies the requested filePath (so after excluding it the remaining-changes +// map is empty), populateCache does NOT store an entry in p.cache. +// +// Before Change 2 (ghost-entry fix), an empty remyCacheEntry was stored for +// the root. That entry lingered forever: cacheValid returned true vacuously +// (no files to hash) but there was nothing to serve. After Change 2, populateCache +// returns early without storing when the built entry would be empty. +func TestPopulateCache_SingleFileFix_NoGhostEntry(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "only.go", "package main\nvar x = 1\n") + + onlyAbs := filepath.Join(repo, "only.go") + + // Runner modifies only the requested file — so populateCache sees an empty + // changes map after excluding filePath. + runner := modifyRunner("only.go", "package main\nvar x = 2\n") + p := remediation.NewRemyProvider(nil, runner) + + edit, err := p.Remediate(context.Background(), remediation.RemediationRequest{ + FindingId: "f1", + ContentRoot: types.FilePath(repo), + FilePath: types.FilePath(onlyAbs), + }) + require.NoError(t, err) + require.NotNil(t, edit, "the requested file was modified so an edit must be returned") + + // After Change 2: no ghost entry must be stored in the cache. + // Before Change 2: CacheLen would be 1 (empty entry) even though there is + // nothing to serve from it. + assert.Equal(t, 0, remediation.CacheLen(p), + "populateCache must not store an empty cache entry when the only change is the requested file") +} diff --git a/domain/snyk/remediation/remy_test.go b/domain/snyk/remediation/remy_test.go index 4329e5292..37cce76f6 100644 --- a/domain/snyk/remediation/remy_test.go +++ b/domain/snyk/remediation/remy_test.go @@ -24,6 +24,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/snyk/go-application-framework/pkg/workflow" "github.com/stretchr/testify/assert" @@ -50,26 +51,52 @@ func initGitRepo(t *testing.T) string { run("init") run("config", "user.email", "test@example.com") run("config", "user.name", "Test") + // Overlay filesystems (e.g. Docker on Linux) have write-ordering delays that + // can cause git to report "not a valid object" when the object database is + // read immediately after a write. core.checkStat=minimal tells git not to + // recheck filesystem timestamps for objects it has already cached in memory, + // which suppresses the false-negative reads on overlayfs. + run("config", "core.checkStat", "minimal") return dir } // commitFile writes content to path (relative to repo root), stages, and commits it. +// git commands are retried a few times on overlayfs write-ordering errors ("not a +// valid object") that can occur when the kernel page cache hasn't flushed a freshly +// written git object before the next git command reads the object store. func commitFile(t *testing.T, repoRoot, relPath, content string) { t.Helper() absPath := filepath.Join(repoRoot, relPath) require.NoError(t, os.MkdirAll(filepath.Dir(absPath), 0o755)) require.NoError(t, os.WriteFile(absPath, []byte(content), 0o644)) - run := func(args ...string) { + runWithRetry := func(args ...string) { t.Helper() - cmd := exec.Command("git", args...) - cmd.Dir = repoRoot - out, err := cmd.CombinedOutput() - require.NoError(t, err, "git %v: %s", args, string(out)) + const maxRetries = 8 + for attempt := 0; attempt < maxRetries; attempt++ { + cmd := exec.Command("git", args...) + cmd.Dir = repoRoot + out, err := cmd.CombinedOutput() + if err == nil { + return + } + // Network- and overlay-filesystem write-ordering delays: a freshly + // written git object or ref is not yet visible for reading due to page + // cache coherence. Two known manifestations: + // "not a valid object" — object written but not yet readable + // "nonexistent object" — ref update races object write + isWriteOrderingDelay := strings.Contains(string(out), "not a valid object") || + strings.Contains(string(out), "nonexistent object") + if attempt < maxRetries-1 && isWriteOrderingDelay { + time.Sleep(50 * time.Millisecond) + continue + } + require.NoError(t, err, "git %v: %s", args, string(out)) + } } - run("add", relPath) - run("commit", "-m", "initial") + runWithRetry("add", relPath) + runWithRetry("commit", "-m", "initial") } // fakeRunner is a test remyRunner that accepts (and ignores) a nil engine. From a54cb36ba995e5a3f9e69b3ad49fa6dda5e98a7f Mon Sep 17 00:00:00 2001 From: Bastian Doetsch Date: Mon, 29 Jun 2026 10:32:35 +0000 Subject: [PATCH 4/4] fix(remediation): canonicalize paths so remy works on macOS and Windows [IDE-2052] The remy provider mixed non-canonical temporary paths with git's canonical paths. On macOS the temp dir is under /var (a symlink to /private/var) and on Windows it is an 8.3 short name; git rev-parse --show-toplevel returns the resolved form. The divergence made the per-finding Remediate flow's worktree diff and cache-key matching fail, so it returned an empty WorkspaceEdit on those platforms while passing on Linux. The failure was masked until now by an unrelated flaky test that aborted the package run first. Canonicalize paths with filepath.EvalSymlinks where the worktree flow needs git's view: Remediate resolves ContentRoot and FilePath (so cache write, serve, and invalidate keys agree), and runRemyInWorktree resolves the MkdirTemp parent. InvalidateFile resolves its path too, falling back to the parent directory when the file has already been deleted. FixFolder deliberately does NOT canonicalize: its caller remaps edits by the exact folder path it passed, so edits must stay keyed under that path; it runs git directly in that folder and needs no resolution. Also: guard NewRemyProvider against a nil runner with a nil engine, lower the empty-diff diagnostic to debug, and fix a folder-URI test that used a path that is not absolute on Windows. Regression tests reproduce the symlink class on Linux for Remediate, InvalidateFile (including the deleted-file path), and the FixFolder passed-path contract. --- domain/ide/command/command_factory_test.go | 11 +- .../command/remediation_fix_folder_test.go | 11 +- domain/snyk/remediation/export_for_test.go | 37 +- domain/snyk/remediation/remy.go | 61 +++- .../snyk/remediation/remy_fix_folder_test.go | 56 +++ domain/snyk/remediation/remy_provider_test.go | 67 +++- domain/snyk/remediation/remy_test.go | 326 ++++++++++++++++-- 7 files changed, 513 insertions(+), 56 deletions(-) diff --git a/domain/ide/command/command_factory_test.go b/domain/ide/command/command_factory_test.go index a3871928b..567a37655 100644 --- a/domain/ide/command/command_factory_test.go +++ b/domain/ide/command/command_factory_test.go @@ -27,6 +27,7 @@ import ( "github.com/snyk/snyk-ls/domain/snyk/remediation" "github.com/snyk/snyk-ls/internal/testutil" "github.com/snyk/snyk-ls/internal/types" + "github.com/snyk/snyk-ls/internal/uri" ) // INT-001: CreateFromCommandData wires provider and notifier into the handler. @@ -42,10 +43,15 @@ func TestCreateFromCommandData_FixFolder_WiresProviderAndNotifier(t *testing.T) notifier := &fakeNotifier{} provider := &fakeFolderRemediator{} + // Use a real OS temp dir converted to a file URI so the path is absolute on + // all operating systems. Hard-coding "file:///tmp" is not an absolute path + // on Windows (no drive letter) and would cause the command to reject it. + folderURI := string(uri.PathToUri(types.FilePath(t.TempDir()))) + cmd, err := command.CreateFromCommandData( context.Background(), engine, - types.CommandData{CommandId: types.RemediationAgentFixFolderCommand, Arguments: []any{"file:///tmp"}}, + types.CommandData{CommandId: types.RemediationAgentFixFolderCommand, Arguments: []any{folderURI}}, nil, // srv nil, // authService nil, // featureFlagService @@ -62,11 +68,10 @@ func TestCreateFromCommandData_FixFolder_WiresProviderAndNotifier(t *testing.T) require.NoError(t, err) require.NotNil(t, cmd) - // Execute the command with the folder path pointing to /tmp which exists. + // Execute the command with a real temp dir that exists on all OSes. // The provider returns nil (no changes) — we only care that the handler // got a non-nil provider and notifier so no nil-pointer panic occurs. _, execErr := cmd.Execute(context.Background()) - // /tmp exists and is a dir; provider is non-nil; returns nil (no changes). assert.NoError(t, execErr, "provider and notifier must be wired; no nil-pointer panic") } diff --git a/domain/ide/command/remediation_fix_folder_test.go b/domain/ide/command/remediation_fix_folder_test.go index f371bce7c..1c5a12d73 100644 --- a/domain/ide/command/remediation_fix_folder_test.go +++ b/domain/ide/command/remediation_fix_folder_test.go @@ -88,11 +88,18 @@ func (f *fakeFolderRemediator) FixFolder(_ context.Context, _ types.FilePath) (* return f.edit, f.err } -// initGitRepo creates a minimal git repo in a temp dir for tests that need a -// real directory on disk. +// initGitRepoForCmd creates a minimal git repo in a temp dir for tests that +// need a real directory on disk. Returns the CANONICAL path (symlinks resolved) +// so test assertions against edit keys agree with what git and the production +// code produce — on macOS t.TempDir returns /var/... but git resolves symlinks +// to /private/var/...; canonicalizing here avoids false assertion failures. func initGitRepoForCmd(t *testing.T) string { t.Helper() dir := t.TempDir() + // Resolve symlinks so the returned path is canonical. + if canonical, err := filepath.EvalSymlinks(dir); err == nil { + dir = canonical + } run := func(args ...string) { t.Helper() cmd := exec.Command("git", args...) diff --git a/domain/snyk/remediation/export_for_test.go b/domain/snyk/remediation/export_for_test.go index 99c1e1642..10a45763d 100644 --- a/domain/snyk/remediation/export_for_test.go +++ b/domain/snyk/remediation/export_for_test.go @@ -16,7 +16,42 @@ package remediation -import "github.com/snyk/snyk-ls/internal/types" +import ( + "sync" + "time" + + "github.com/rs/zerolog" + "github.com/snyk/go-application-framework/pkg/workflow" + + "github.com/snyk/snyk-ls/internal/types" +) + +// NewRemyProviderWithLogger constructs a remyProvider with an explicit zerolog.Logger +// so that tests using a nil workflow.Engine can still capture diagnostic output +// (e.g. the "no changes detected in worktree" WARN logged by buildWorkspaceEdits). +// Use zerolog.New(testWriter) where testWriter is a zerolog.TestWriter or similar. +// +// Panics if both runner and engine are nil: when runner is nil the constructor +// falls back to gafRunner which dereferences engine at call time, so a nil engine +// would cause a nil-pointer dereference far from the misconfiguration site. The +// guard makes the failure immediate and actionable at construction time. +func NewRemyProviderWithLogger(engine workflow.Engine, runner remyRunner, log zerolog.Logger) RemediationProvider { + if runner == nil && engine == nil { + panic("NewRemyProviderWithLogger: nil runner requires a non-nil engine; pass a test runner or provide a workflow.Engine") + } + opts := RemyOptions{Timeout: 5 * time.Minute} + if runner == nil { + runner = gafRunner + } + return &remyProvider{ + opts: opts, + runner: runner, + engine: engine, + log: log, + cache: make(map[string]*remyCacheEntry), + rootMus: make(map[string]*sync.Mutex), + } +} // ExportedWorkspaceEditFromContent exposes workspaceEditFromContent for // black-box tests in the remediation_test package. diff --git a/domain/snyk/remediation/remy.go b/domain/snyk/remediation/remy.go index f74143cc9..4cfdf5b81 100644 --- a/domain/snyk/remediation/remy.go +++ b/domain/snyk/remediation/remy.go @@ -111,6 +111,9 @@ func gafRunner(ctx context.Context, eng workflow.Engine, contentRoot string, _ s // invokes the legacycli workflow via the engine. Callers that want to plug // in a fake runner for unit tests pass a non-nil function here. func NewRemyProvider(engine workflow.Engine, runner remyRunner) RemediationProvider { + if runner == nil && engine == nil { + panic("NewRemyProvider: nil runner requires a non-nil engine; pass a test runner or a workflow.Engine") + } var log zerolog.Logger opts := RemyOptions{} if engine != nil { @@ -216,7 +219,17 @@ func (p *remyProvider) Remediate(ctx context.Context, req RemediationRequest) (* if !filepath.IsAbs(root) { return nil, fmt.Errorf("remy: ContentRoot must be an absolute path, got %q", root) } + // Canonicalize root so it agrees with git's canonical view (e.g. on macOS + // os.TempDir returns /var/... but git resolves symlinks to /private/var/...). + // Fall back to the original path on error (e.g. path does not exist yet). + if canonical, err := filepath.EvalSymlinks(root); err == nil { + root = canonical + } filePath := string(req.FilePath) + // Canonicalize filePath for the same reason. + if canonical, err := filepath.EvalSymlinks(filePath); err == nil { + filePath = canonical + } if edits, ok := p.tryServeFromCache(root, filePath); ok { return editsToEdit(filePath, edits), nil @@ -283,10 +296,20 @@ func (p *remyProvider) runRemyInWorktree(ctx context.Context, root string, req R if err != nil { return nil, fmt.Errorf("remy: resolve git root: %w", err) } + // gitRoot is already canonical: git rev-parse --show-toplevel resolves symlinks. tmpParent, err := os.MkdirTemp("", "snyk-remy-*") if err != nil { return nil, fmt.Errorf("remy: create temp dir: %w", err) } + // Canonicalize the temp parent so the worktree path agrees with git's view. + // On macOS os.MkdirTemp under /var/... returns a non-canonical path; + // git worktree add resolves symlinks before recording the worktree, so the + // path git knows differs from the path we computed — snapshot/diff lookups + // then fail to match. EvalSymlinks after the directory is created (so the + // path exists) gives us the same canonical form git will use. + if canonical, evalErr := filepath.EvalSymlinks(tmpParent); evalErr == nil { + tmpParent = canonical + } worktreeDir := filepath.Join(tmpParent, "wt") addOut, err := exec.CommandContext(ctx, "git", "-C", gitRoot, "worktree", "add", "--detach", worktreeDir, "HEAD").CombinedOutput() if err != nil { @@ -350,6 +373,17 @@ func buildWorkspaceEdits(ctx context.Context, log zerolog.Logger, worktreeDir, g return nil, fmt.Errorf("remy: enumerate changed files: %w", err) } if len(changedPaths) == 0 { + // No changes detected — log the git state so CI failures show the actual + // worktree contents. This surfaces path-canonicalization mismatches (e.g. + // macOS /var→/private/var symlink, Windows 8.3 short names) where the + // snapshot keys and worktree paths diverge and git sees no diff. + statusOut, statusErr := exec.CommandContext(ctx, "git", "-C", worktreeDir, "status", "--porcelain").Output() + log.Debug(). + Str("worktreeDir", worktreeDir). + Str("gitRoot", gitRoot). + Str("gitStatus", strings.TrimSpace(string(statusOut))). + AnErr("gitStatusErr", statusErr). + Msg("remy: no changes detected in worktree after fix run") return nil, nil } allChanges := make(map[string][]types.TextEdit) @@ -437,12 +471,35 @@ func (p *remyProvider) populateCache(root, filePath string, allChanges map[strin // results. It is called by the LSP textDocument/didChange handler. The // corresponding fileHashes entry is deleted alongside the changes entry so // both maps stay in sync. +// +// The path is canonicalized via filepath.EvalSymlinks (with fallback on error) +// before the cache lookup so that it matches the canonical key inserted by +// Remediate. Without canonicalization, a non-canonical path (e.g. a symlinked +// path on macOS /var→/private/var) would miss the canonical cache key and leave +// stale entries alive. func (p *remyProvider) InvalidateFile(path types.FilePath) { + s := string(path) + if canonical, err := filepath.EvalSymlinks(s); err == nil { + s = canonical + } else if dir, derr := filepath.EvalSymlinks(filepath.Dir(s)); derr == nil { + // The file itself no longer exists (e.g. a fix deleted it and then + // didChange/didClose fired). EvalSymlinks on the file fails, but the + // parent directory still exists and can be resolved. Rejoin the base + // name to produce the same canonical path that Remediate wrote into + // the cache (Remediate calls EvalSymlinks while the file still existed). + // + // Limitation: if the missing file's basename was itself a symlink (rare), + // the reconstructed key may not match the canonical key stored by Remediate, + // in which case the delete is a harmless no-op — the stale entry simply is + // not evicted, the same outcome as before symlink canonicalization was added. + s = filepath.Join(dir, filepath.Base(s)) + } + canonPath := s p.cacheMu.Lock() defer p.cacheMu.Unlock() for root, entry := range p.cache { - delete(entry.changes, string(path)) - delete(entry.fileHashes, string(path)) + delete(entry.changes, canonPath) + delete(entry.fileHashes, canonPath) // Remove the cache entry entirely once it has no remaining changes so // it does not occupy memory or cause vacuously-valid hits. if len(entry.changes) == 0 { diff --git a/domain/snyk/remediation/remy_fix_folder_test.go b/domain/snyk/remediation/remy_fix_folder_test.go index 3167c994c..4c44eb734 100644 --- a/domain/snyk/remediation/remy_fix_folder_test.go +++ b/domain/snyk/remediation/remy_fix_folder_test.go @@ -21,6 +21,7 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" "github.com/snyk/go-application-framework/pkg/workflow" @@ -260,3 +261,58 @@ func TestFixFolder_RunsDirectlyInPassedFolder(t *testing.T) { } } } + +// --------------------------------------------------------------------------- +// DAEMON CONTRACT: FixFolder edit keys must be under the PASSED (non-canonical) path +// --------------------------------------------------------------------------- + +// TestFixFolder_SymlinkPath_EditsKeyedUnderPassedPath locks the daemon contract: +// when the caller passes a symlinked folder path as ContentRoot, FixFolder must +// return edit keys under the EXACT passed path (not the canonicalized/resolved +// path). The external daemon remaps edits by the prefix of the path it passed; +// canonicalizing `r` inside FixFolder would make edits key under the resolved +// path, breaking the daemon's prefix match on any path with a symlink component. +// +// On Linux we reproduce the macOS /var→/private/var class by creating an +// explicit symlink. The test is skipped gracefully if symlink creation fails. +func TestFixFolder_SymlinkPath_EditsKeyedUnderPassedPath(t *testing.T) { + // Set up a real git repo under a canonical dir. + realDir := t.TempDir() + var err error + realDir, err = filepath.EvalSymlinks(realDir) + require.NoError(t, err) + initGitRepoInDir(t, realDir) + commitFileInDir(t, realDir, "main.go", "package main\nvar x = 1\n") + + // Create a symlink to the real dir — the daemon-like caller passes this. + linkDir := filepath.Join(t.TempDir(), "link") + if symlinkErr := os.Symlink(realDir, linkDir); symlinkErr != nil { + t.Skipf("cannot create symlink (os restriction): %v", symlinkErr) + } + + // The runner writes a change so we get a non-nil edit back. + runner := func(_ context.Context, _ workflow.Engine, root, _ string) error { + return os.WriteFile(filepath.Join(root, "main.go"), []byte("package main\nvar x = 2\n"), 0644) + } + + p := remediation.NewRemyProvider(nil, runner) + fr, ok := p.(remediation.FolderRemediator) + require.True(t, ok) + + // Call FixFolder with the SYMLINKED (non-canonical) path. + edit, err := fr.FixFolder(context.Background(), types.FilePath(linkDir)) + require.NoError(t, err) + require.NotNil(t, edit, "FixFolder must return a non-nil edit when a file was modified") + + // Daemon contract: every edit key must begin with the PASSED path (linkDir), + // not the canonical resolved path (realDir). Canonicalizing inside FixFolder + // would produce keys under realDir, breaking the daemon's prefix remap. + // Use a path-separator-aware check so a sibling directory whose name merely + // starts with linkDir cannot produce a false pass. + for key := range edit.Changes { + assert.True(t, strings.HasPrefix(key, linkDir+string(filepath.Separator)), + "edit key %q must be prefixed by the passed symlinked path %q (daemon contract); "+ + "if it is prefixed by the canonical path %q, FixFolder incorrectly canonicalized r", + key, linkDir, realDir) + } +} diff --git a/domain/snyk/remediation/remy_provider_test.go b/domain/snyk/remediation/remy_provider_test.go index b8a7d9a4f..684aed4c4 100644 --- a/domain/snyk/remediation/remy_provider_test.go +++ b/domain/snyk/remediation/remy_provider_test.go @@ -25,6 +25,8 @@ import ( "sync/atomic" "testing" + "github.com/rs/zerolog" + "github.com/snyk/go-application-framework/pkg/app" "github.com/snyk/go-application-framework/pkg/workflow" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -437,20 +439,46 @@ func TestGetOrCreateRootMu_ConcurrentSameRoot(t *testing.T) { // NewRemyProvider — engine != nil branch // --------------------------------------------------------------------------- -// TestNewRemyProvider_WithEngine verifies that NewRemyProvider succeeds when a -// non-nil workflow.Engine is provided (exercises the logger setup branch). -func TestNewRemyProvider_WithEngine(t *testing.T) { - // We need a minimal workflow.Engine. The only thing NewRemyProvider calls - // on it is GetLogger(). Use the exported NoopProvider as a sanity check - // that the constructor returns a non-nil value even without a real engine. - // Since we cannot easily construct a real GAF engine in a unit test, we - // exercise the non-nil engine branch via the nil-runner path (gafRunner) - // to cover the runner == nil assignment. The nil engine path is already - // covered by other tests. - p := remediation.NewRemyProvider(nil, nil) // triggers runner==nil branch → gafRunner +// TestNewRemyProvider_WithNilRunnerNilEngine_Panics verifies that +// NewRemyProvider panics when both engine and runner are nil. When runner is +// nil the constructor would fall back to gafRunner which dereferences engine at +// call time; with a nil engine the first Remediate or FixFolder call would +// panic with a nil pointer dereference far from the misconfiguration site. The +// guard makes the failure immediate and actionable at construction time. +func TestNewRemyProvider_WithNilRunnerNilEngine_Panics(t *testing.T) { + assert.Panics(t, func() { + _ = remediation.NewRemyProvider(nil, nil) + }, "NewRemyProvider must panic when both engine and runner are nil") +} + +// TestNewRemyProvider_NilEngineNonNilRunner_Succeeds verifies that +// NewRemyProvider succeeds when engine is nil and runner is non-nil. Supplying +// a non-nil runner satisfies the construction guard (which only panics when +// both engine AND runner are nil), so the provider is returned ready for use. +func TestNewRemyProvider_NilEngineNonNilRunner_Succeeds(t *testing.T) { + p := remediation.NewRemyProvider(nil, noopRunner) assert.NotNil(t, p) } +// TestNewRemyProvider_NonNilEngineNilRunner_SubstitutesGafRunner verifies the +// production code path where runner is nil but engine is non-nil: the constructor +// must substitute gafRunner and return a non-nil, callable provider. The real +// gafRunner is never invoked here — an empty FindingId short-circuits Remediate +// before reaching the runner, keeping this a pure unit test with no network or +// CLI calls. +func TestNewRemyProvider_NonNilEngineNilRunner_SubstitutesGafRunner(t *testing.T) { + engine := app.CreateAppEngineWithOptions() + p := remediation.NewRemyProvider(engine, nil) + require.NotNil(t, p, "NewRemyProvider must return a non-nil provider when engine is non-nil and runner is nil") + + // Remediate with an empty FindingId short-circuits before the runner is + // invoked, so this proves the nil runner was replaced (provider is callable) + // without triggering any real remy/CLI invocation. + edit, err := p.Remediate(context.Background(), remediation.RemediationRequest{}) + require.NoError(t, err) + assert.Nil(t, edit) +} + // --------------------------------------------------------------------------- // cacheValid — stale file eviction (content-hash check) // --------------------------------------------------------------------------- @@ -869,3 +897,20 @@ func TestPopulateCache_SingleFileFix_NoGhostEntry(t *testing.T) { assert.Equal(t, 0, remediation.CacheLen(p), "populateCache must not store an empty cache entry when the only change is the requested file") } + +// --------------------------------------------------------------------------- +// NewRemyProviderWithLogger — nil-engine + nil-runner guard +// --------------------------------------------------------------------------- + +// TestNewRemyProviderWithLogger_NilEngineNilRunner_Panics verifies that +// NewRemyProviderWithLogger panics immediately when both engine and runner are +// nil. When runner is nil the constructor falls back to gafRunner, which +// dereferences engine at call time; with a nil engine the first Remediate or +// FixFolder call would panic with a nil pointer dereference — a confusing +// crash far from the misconfiguration site. The guard makes the failure +// immediate and actionable at construction time. +func TestNewRemyProviderWithLogger_NilEngineNilRunner_Panics(t *testing.T) { + assert.Panics(t, func() { + _ = remediation.NewRemyProviderWithLogger(nil, nil, zerolog.Nop()) + }, "NewRemyProviderWithLogger must panic when both engine and runner are nil") +} diff --git a/domain/snyk/remediation/remy_test.go b/domain/snyk/remediation/remy_test.go index 37cce76f6..f201fc049 100644 --- a/domain/snyk/remediation/remy_test.go +++ b/domain/snyk/remediation/remy_test.go @@ -35,10 +35,19 @@ import ( ) // initGitRepo sets up a minimal git repository under dir so the provider can -// use git-based change detection. Returns the repo root. +// use git-based change detection. Returns the CANONICAL repo root (symlinks +// resolved) so that test assertions against edit keys match the paths that the +// production code and git produce — on macOS t.TempDir returns /var/... but +// git resolves symlinks to /private/var/...; using the canonical path here +// prevents false "expected key not found" assertion failures. func initGitRepo(t *testing.T) string { t.Helper() dir := t.TempDir() + // Resolve symlinks so the returned path is canonical. + canonical, err := filepath.EvalSymlinks(dir) + if err == nil { + dir = canonical + } run := func(args ...string) { t.Helper() @@ -66,37 +75,7 @@ func initGitRepo(t *testing.T) string { // written git object before the next git command reads the object store. func commitFile(t *testing.T, repoRoot, relPath, content string) { t.Helper() - absPath := filepath.Join(repoRoot, relPath) - require.NoError(t, os.MkdirAll(filepath.Dir(absPath), 0o755)) - require.NoError(t, os.WriteFile(absPath, []byte(content), 0o644)) - - runWithRetry := func(args ...string) { - t.Helper() - const maxRetries = 8 - for attempt := 0; attempt < maxRetries; attempt++ { - cmd := exec.Command("git", args...) - cmd.Dir = repoRoot - out, err := cmd.CombinedOutput() - if err == nil { - return - } - // Network- and overlay-filesystem write-ordering delays: a freshly - // written git object or ref is not yet visible for reading due to page - // cache coherence. Two known manifestations: - // "not a valid object" — object written but not yet readable - // "nonexistent object" — ref update races object write - isWriteOrderingDelay := strings.Contains(string(out), "not a valid object") || - strings.Contains(string(out), "nonexistent object") - if attempt < maxRetries-1 && isWriteOrderingDelay { - time.Sleep(50 * time.Millisecond) - continue - } - require.NoError(t, err, "git %v: %s", args, string(out)) - } - } - - runWithRetry("add", relPath) - runWithRetry("commit", "-m", "initial") + commitFileInDir(t, repoRoot, relPath, content) } // fakeRunner is a test remyRunner that accepts (and ignores) a nil engine. @@ -529,11 +508,15 @@ func TestRemediate_GitRoot_SubdirWorkspace(t *testing.T) { "Changes must be keyed by the real workspace path (gitRoot/relPath), not subdir/relPath") } -// TestNewRemyProvider_NilRunner_SetsDefault verifies that NewRemyProvider with a -// nil runner substitutes the default gafRunner. The default runner is never -// invoked here because Remediate short-circuits on empty FindingId. -func TestNewRemyProvider_NilRunner_SetsDefault(t *testing.T) { - p := remediation.NewRemyProvider(nil, nil) +// TestNewRemyProvider_NilEngineNonNilRunner_RemediateEmptyFindingId_ReturnsNil +// verifies that constructing with a nil engine and a non-nil runner succeeds, and +// that calling Remediate with an empty FindingId short-circuits to (nil, nil) +// without invoking the runner. This is distinct from +// TestNewRemyProvider_NilEngineNonNilRunner_Succeeds in remy_provider_test.go, +// which only asserts the provider is non-nil; this test additionally exercises the +// Remediate empty-FindingId short-circuit path. +func TestNewRemyProvider_NilEngineNonNilRunner_RemediateEmptyFindingId_ReturnsNil(t *testing.T) { + p := remediation.NewRemyProvider(nil, noopRunner) require.NotNil(t, p) edit, err := p.Remediate(context.Background(), remediation.RemediationRequest{}) require.NoError(t, err) @@ -676,6 +659,132 @@ func TestParseDiffHunks_DeletionOfDashDashLine(t *testing.T) { assert.True(t, found, "expected a deletion TextEdit on line 0") } +// mustEvalSymlinks is a test helper that resolves symlinks in path and fails the +// test if resolution fails (e.g. the path does not exist). On Linux with no +// symlinks EvalSymlinks is a no-op; on macOS /var→/private/var it returns the +// real path. +func mustEvalSymlinks(t *testing.T, path string) string { + t.Helper() + resolved, err := filepath.EvalSymlinks(path) + require.NoError(t, err, "EvalSymlinks(%q) must succeed", path) + return resolved +} + +// TestRemediate_SymlinkContentRoot_ReturnsCanonicalEdit is a regression test +// for the macOS/Windows cross-platform bug where t.TempDir() (or os.TempDir()) +// returns a non-canonical path (e.g. /var/... on macOS, which resolves to +// /private/var/...). When ContentRoot is a non-canonical path containing a +// symlink component, Remediate must still return a WorkspaceEdit keyed under +// the CANONICAL path — matching git's canonical view — rather than a nil edit. +// +// On Linux we reproduce the macOS class by explicitly creating a symlink to the +// real temp dir and passing the symlinked path as ContentRoot. Without the +// filepath.EvalSymlinks fix, the edit keys (from git --show-toplevel, canonical) +// do not match the lookup key (non-canonical symlink path), so the test fails +// with a nil edit even though the runner modified a file. +func TestRemediate_SymlinkContentRoot_ReturnsCanonicalEdit(t *testing.T) { + // Create the real repo under a canonical temp dir. + realDir := t.TempDir() + realDir = mustEvalSymlinks(t, realDir) // ensure realDir itself is canonical + initGitRepoInDir(t, realDir) + commitFileInDir(t, realDir, "main.go", "package main\nvar x = 1\n") + + // Create a symlink pointing to the real dir so we can pass a non-canonical + // path as ContentRoot, reproducing the macOS /var → /private/var class. + linkDir := filepath.Join(t.TempDir(), "link") + if err := os.Symlink(realDir, linkDir); err != nil { + t.Skipf("cannot create symlink (os restriction): %v", err) + } + + // ContentRoot is the non-canonical symlinked path. + symlinkRoot := linkDir + // The canonical path (what git rev-parse --show-toplevel returns). + canonicalRoot := realDir + + modified := "package main\nvar x = 99\n" + runner := fakeRunner(func(_ context.Context, root string, _ string) error { + return os.WriteFile(filepath.Join(root, "main.go"), []byte(modified), 0o644) + }) + + p := remediation.NewRemyProvider(nil, runner) + + edit, err := p.Remediate(context.Background(), remediation.RemediationRequest{ + FindingId: "finding-symlink", + ContentRoot: types.FilePath(symlinkRoot), + FilePath: types.FilePath(filepath.Join(symlinkRoot, "main.go")), + }) + + require.NoError(t, err) + // Before the fix, edit is nil because the key (canonical) does not match the + // non-canonical lookup. After the fix, the canonical key is used throughout + // and the edit is non-nil. + require.NotNil(t, edit, + "Remediate must return a non-nil edit even when ContentRoot contains a symlink component; "+ + "symlink root=%q canonical root=%q", symlinkRoot, canonicalRoot) + + // The edit key must use the canonical path, not the symlinked path. + canonicalFilePath := filepath.Join(canonicalRoot, "main.go") + assert.Contains(t, edit.Changes, canonicalFilePath, + "edit must be keyed under the canonical path %q, not the symlink path %q", + canonicalFilePath, filepath.Join(symlinkRoot, "main.go")) +} + +// initGitRepoInDir initializes a git repo in an already-existing directory dir. +// Unlike initGitRepo (which creates a new t.TempDir), this takes an existing dir. +func initGitRepoInDir(t *testing.T, dir string) { + t.Helper() + run := func(args ...string) { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v: %s", args, string(out)) + } + run("init") + run("config", "user.email", "test@example.com") + run("config", "user.name", "Test") + run("config", "core.checkStat", "minimal") +} + +// commitFileInDir writes content to relPath (relative to dir), stages, and +// commits it. git commands are retried a few times on overlayfs write-ordering +// errors ("not a valid object") that can occur when the kernel page cache +// hasn't flushed a freshly written git object before the next git command +// reads the object store. +func commitFileInDir(t *testing.T, dir, relPath, content string) { + t.Helper() + absPath := filepath.Join(dir, relPath) + require.NoError(t, os.MkdirAll(filepath.Dir(absPath), 0o755)) + require.NoError(t, os.WriteFile(absPath, []byte(content), 0o644)) + + runWithRetry := func(args ...string) { + t.Helper() + const maxRetries = 8 + for attempt := 0; attempt < maxRetries; attempt++ { + cmd := exec.Command("git", args...) + cmd.Dir = dir + out, err := cmd.CombinedOutput() + if err == nil { + return + } + // Network- and overlay-filesystem write-ordering delays: a freshly + // written git object or ref is not yet visible for reading due to page + // cache coherence. Two known manifestations: + // "not a valid object" — object written but not yet readable + // "nonexistent object" — ref update races object write + isWriteOrderingDelay := strings.Contains(string(out), "not a valid object") || + strings.Contains(string(out), "nonexistent object") + if attempt < maxRetries-1 && isWriteOrderingDelay { + time.Sleep(50 * time.Millisecond) + continue + } + require.NoError(t, err, "git %v: %s", args, string(out)) + } + } + runWithRetry("add", relPath) + runWithRetry("commit", "-m", "initial") +} + // TestParseDiffHunks_NoNewlineAtEndOfFile verifies that the "\ No newline at end // of file" diff marker is handled without error by adjusting the line cursor. func TestParseDiffHunks_NoNewlineAtEndOfFile(t *testing.T) { @@ -691,3 +800,146 @@ func TestParseDiffHunks_NoNewlineAtEndOfFile(t *testing.T) { require.NotNil(t, edit) assert.Contains(t, edit.Changes, absPath) } + +// TestInvalidateFile_DeletedFile_SymlinkPath_EvictsCanonicalKey tests Fix 1: +// when a file has been deleted (e.g. a fix removed it) and then the LSP +// didClose/didChange handler calls InvalidateFile with the non-canonical +// (symlinked) path, filepath.EvalSymlinks(path) FAILS because the file no +// longer exists on disk. The implementation must fall back to resolving the +// PARENT DIRECTORY (which still exists) and re-joining the base name, so the +// resulting canonical path matches the key that Remediate wrote into the cache +// (which used EvalSymlinks while the file still existed). +// +// Without the Dir+Base fallback, InvalidateFile falls back to the raw +// (non-canonical) path, misses the canonical cache key, and the stale entry +// survives. The next Remediate call then incorrectly serves stale edits. +// +// The test is skipped gracefully if the OS disallows symlink creation. +func TestInvalidateFile_DeletedFile_SymlinkPath_EvictsCanonicalKey(t *testing.T) { + // Create a real git repo under a canonical temp dir with two files. + realDir := t.TempDir() + realDir = mustEvalSymlinks(t, realDir) + initGitRepoInDir(t, realDir) + commitFileInDir(t, realDir, "a.go", "package main\nvar a = 1\n") + commitFileInDir(t, realDir, "b.go", "package main\nvar b = 1\n") + + // Create a symlink to the real dir — the "LSP handler" path is non-canonical. + linkDir := filepath.Join(t.TempDir(), "link") + if err := os.Symlink(realDir, linkDir); err != nil { + t.Skipf("cannot create symlink (os restriction): %v", err) + } + + symlinkRoot := linkDir + symlinkA := filepath.Join(symlinkRoot, "a.go") + symlinkB := filepath.Join(symlinkRoot, "b.go") + + var runnerCalls int + runner := fakeRunner(func(_ context.Context, root string, _ string) error { + runnerCalls++ + // Modify both files so b.go ends up in the cache after the a.go request. + if err := os.WriteFile(filepath.Join(root, "a.go"), []byte("package main\nvar a = 2\n"), 0o644); err != nil { + return err + } + return os.WriteFile(filepath.Join(root, "b.go"), []byte("package main\nvar b = 2\n"), 0o644) + }) + + p := remediation.NewRemyProvider(nil, runner) + + // First Remediate populates cache: a.go edits returned, b.go edits cached + // under the CANONICAL key (realDir/b.go). + _, err := p.Remediate(context.Background(), remediation.RemediationRequest{ + FindingId: "finding-del-symlink", + ContentRoot: types.FilePath(symlinkRoot), + FilePath: types.FilePath(symlinkA), + }) + require.NoError(t, err) + require.Equal(t, 1, runnerCalls, "runner must run once on first call") + + // NOW delete b.go from disk — simulating a fix that removed the file. + // After deletion, filepath.EvalSymlinks(symlinkB) FAILS because the target + // no longer exists. The Dir+Base fallback must resolve the parent (linkDir → + // realDir) and re-join "b.go" to produce the canonical path for the delete. + require.NoError(t, os.Remove(filepath.Join(realDir, "b.go"))) + + // Invalidate using the non-canonical symlinked path of the now-deleted file. + inv, ok := p.(remediation.FileChangeNotifier) + require.True(t, ok, "remyProvider must implement FileChangeNotifier") + inv.InvalidateFile(types.FilePath(symlinkB)) + + // The cache entry (keyed under the canonical path realDir) must now be empty + // (b.go was the only remaining cached file). CacheLen must be 0. + assert.Equal(t, 0, remediation.CacheLen(p), + "InvalidateFile with a non-canonical path of a DELETED file must evict the cache entry; "+ + "Dir+Base fallback required when EvalSymlinks(file) fails but EvalSymlinks(dir) succeeds; "+ + "symlink=%q canonical dir=%q", symlinkB, realDir) +} + +// TestInvalidateFile_SymlinkPath_EvictsCanonicalKey is a regression test for +// the macOS/Windows class of bug where the LSP textDocument/didChange handler +// calls InvalidateFile with the raw URI path (non-canonical, e.g. the symlinked +// path) while the cache was populated by Remediate using the canonical path +// (after filepath.EvalSymlinks). Without the fix, the delete misses the +// canonical key and the stale cache entry survives, causing the next Remediate +// to serve stale edits instead of re-running remy. +// +// On Linux we reproduce this by creating a symlink to a real temp dir. +// The test is skipped gracefully if the OS disallows symlink creation. +func TestInvalidateFile_SymlinkPath_EvictsCanonicalKey(t *testing.T) { + // Create the real repo under a canonical temp dir (two files so one gets cached). + realDir := t.TempDir() + realDir = mustEvalSymlinks(t, realDir) + initGitRepoInDir(t, realDir) + commitFileInDir(t, realDir, "a.go", "package main\nvar a = 1\n") + commitFileInDir(t, realDir, "b.go", "package main\nvar b = 1\n") + + // Create a symlink pointing to the real dir. + linkDir := filepath.Join(t.TempDir(), "link") + if err := os.Symlink(realDir, linkDir); err != nil { + t.Skipf("cannot create symlink (os restriction): %v", err) + } + + // Non-canonical (symlinked) paths — what the LSP handler would supply. + symlinkRoot := linkDir + symlinkA := filepath.Join(symlinkRoot, "a.go") + symlinkB := filepath.Join(symlinkRoot, "b.go") + + var runnerCalls int + runner := fakeRunner(func(_ context.Context, root string, _ string) error { + runnerCalls++ + if err := os.WriteFile(filepath.Join(root, "a.go"), []byte("package main\nvar a = 2\n"), 0o644); err != nil { + return err + } + return os.WriteFile(filepath.Join(root, "b.go"), []byte("package main\nvar b = 2\n"), 0o644) + }) + + p := remediation.NewRemyProvider(nil, runner) + + // First call with symlinked ContentRoot — Remediate canonicalizes internally. + // a.go edits are returned; b.go edits are cached under the CANONICAL key. + _, err := p.Remediate(context.Background(), remediation.RemediationRequest{ + FindingId: "finding-symlink-invalidate", + ContentRoot: types.FilePath(symlinkRoot), + FilePath: types.FilePath(symlinkA), + }) + require.NoError(t, err) + require.Equal(t, 1, runnerCalls, "runner must be called exactly once on first Remediate") + + // Invalidate b.go via the NON-canonical (symlinked) path — the path a + // real LSP handler would pass from uri.PathFromUri. + inv, ok := p.(remediation.FileChangeNotifier) + require.True(t, ok, "remyProvider must implement FileChangeNotifier") + inv.InvalidateFile(types.FilePath(symlinkB)) + + // Second call for b.go via the symlinked path. The cache entry (keyed under + // canonical path) must have been evicted by InvalidateFile, so the runner + // must fire again. + _, err = p.Remediate(context.Background(), remediation.RemediationRequest{ + FindingId: "finding-symlink-invalidate", + ContentRoot: types.FilePath(symlinkRoot), + FilePath: types.FilePath(symlinkB), + }) + require.NoError(t, err) + assert.Equal(t, 2, runnerCalls, + "runner must be re-invoked after InvalidateFile with the non-canonical (symlinked) path; "+ + "symlink=%q canonical=%q", symlinkB, filepath.Join(realDir, "b.go")) +}