From 7a5aa4017df982eae3101abb6947dbead856dd50 Mon Sep 17 00:00:00 2001 From: Bastian Doetsch Date: Mon, 29 Jun 2026 13:03:53 +0000 Subject: [PATCH] fix(remediation): address review findings in the code-action flow [IDE-2052] Follow-up fixes to the remediation code-action path, surfaced by review of the fixFolder work: - Restrict remediation quick-fixes to products the agent can handle. The filter previously let fixable Open Source issues through and offered a RemediationAgentQuickFix the LLM agent cannot apply; replace the negated IaC bypass with an explicit product switch (Code when it has an AI fix, IaC) and drop everything else. - Propagate the codeAction/resolve request context to the provider. The deferred edit ran with context.Background(), so cancelling a resolve left the remediation running; the deferred-edit type now carries a context that is threaded through to Remediate, and a nil deferred edit is guarded in both the resolve and the AI-fix command paths. - Make the code-action cache concurrency-safe. actionsCache was an unsynchronised map read by codeAction/resolve and written by textDocument/codeAction; add a mutex covering every access, take it only to read and (via defer) delete the entry, and run the slow edit without the lock held so the entry survives a concurrent retry and is always cleaned up, even on panic. - FixFolder now rejects a worktree with uncommitted tracked changes (git status --porcelain --untracked-files=no) so it cannot silently fold pre-existing edits into its result, while ignoring untracked artifacts. - Advertise snyk.remediationAgent.fixFolder only when the remediation agent is enabled, via an exported RemediationAgentEnabledKey shared with the DI gate. Correct the filtered-issue debug count and remove the unused NoopProvider. --- .gitignore | 1 + application/codeaction/codeaction.go | 71 ++++++- application/codeaction/codeaction_test.go | 191 +++++++++++++++++- application/codeaction/export_test.go | 32 +++ application/codeaction/race_test.go | 145 +++++++++++++ application/codeaction/remediation_test.go | 81 +++++++- application/di/init.go | 9 +- application/di/init_fix_folder_test.go | 2 +- application/server/codeaction_handlers.go | 2 +- application/server/server.go | 81 ++++---- application/server/server_smoke_test.go | 2 +- application/server/server_test.go | 40 +++- domain/ide/command/code_fix.go | 9 +- domain/ide/command/code_fix_test.go | 46 ++++- domain/snyk/codeaction.go | 10 +- domain/snyk/codeaction_test.go | 5 +- domain/snyk/remediation/noop.go | 32 --- domain/snyk/remediation/noop_test.go | 34 ---- domain/snyk/remediation/remy.go | 13 ++ .../snyk/remediation/remy_fix_folder_test.go | 127 +++++++++++- infrastructure/oss/code_actions.go | 3 +- infrastructure/oss/code_actions_test.go | 15 +- internal/types/issues.go | 3 +- 23 files changed, 802 insertions(+), 152 deletions(-) create mode 100644 application/codeaction/export_test.go create mode 100644 application/codeaction/race_test.go delete mode 100644 domain/snyk/remediation/noop.go delete mode 100644 domain/snyk/remediation/noop_test.go diff --git a/.gitignore b/.gitignore index 5514f8f92..79e359192 100644 --- a/.gitignore +++ b/.gitignore @@ -127,3 +127,4 @@ brain/ .snyk-code-output.json .snyk-code-output.err .verification-result.* +IDE-2052_20260629_110429_ctx.log diff --git a/application/codeaction/codeaction.go b/application/codeaction/codeaction.go index da1d0c151..f1ef6ae93 100644 --- a/application/codeaction/codeaction.go +++ b/application/codeaction/codeaction.go @@ -21,6 +21,7 @@ import ( "context" "errors" "fmt" + "sync" "time" "github.com/google/uuid" @@ -48,6 +49,12 @@ type CodeActionsService struct { IssuesProvider snyk.IssueProvider featureFlagService featureflag.Service + // actionsCacheMu protects actionsCache. Every read and write of actionsCache + // must hold this lock. In ResolveCodeAction, the lock is released BEFORE + // invoking the slow deferred edit so that concurrent codeAction/resolve + // requests (which run on separate LSP handler goroutines) are not serialized + // through the potentially multi-minute remediation call. + actionsCacheMu sync.Mutex // actionsCache holds all the issues that were returns by the GetCodeActions method. // This is used to resolve the code actions later on in ResolveCodeAction. actionsCache map[uuid.UUID]cachedAction @@ -115,7 +122,7 @@ func (c *CodeActionsService) GetCodeActions(params types.CodeActionParams) []typ } filteredIssues = append(filteredIssues, issue) } - c.logger.Debug().Any("path", path).Any("range", r).Msgf("Filtered to %d issues", len(issues)) + c.logger.Debug().Any("path", path).Any("range", r).Msgf("Filtered to %d issues", len(filteredIssues)) } quickFixGroupables := c.getQuickFixGroupablesAndCache(filteredIssues) @@ -146,19 +153,27 @@ func (c *CodeActionsService) remediationCodeActions(issues []types.Issue, path t continue } issueProduct := issue.GetProduct() - if issueProduct == product.ProductSecrets || issueProduct == product.ProductUnknown { - continue - } additionalData := issue.GetAdditionalData() - if issueProduct != product.ProductInfrastructureAsCode && (additionalData == nil || !additionalData.IsFixable()) { + switch issueProduct { + case product.ProductCode: + // Remy handles Code issues that carry an AI-generated fix (hasAIFix). + if additionalData == nil || !additionalData.IsFixable() { + continue + } + case product.ProductInfrastructureAsCode: + // Remy handles IaC issues via FindingId; IaCIssueData.IsFixable() is + // always false, so eligibility is determined by FindingId alone (already + // guarded above). + default: + // OSS, Secrets, Unknown, and any future product are not handled by remy. continue } // Capture loop variables for the closure. issueFindingId := findingId issueRange := r provider := c.remediationProvider - deferredEdit := func() *types.WorkspaceEdit { - edit, err := provider.Remediate(context.Background(), remediation.RemediationRequest{ + deferredEdit := func(ctx context.Context) *types.WorkspaceEdit { + edit, err := provider.Remediate(ctx, remediation.RemediationRequest{ FindingId: issueFindingId, FilePath: path, ContentRoot: folderPath, @@ -257,11 +272,24 @@ func (c *CodeActionsService) cacheCodeAction(action types.CodeAction, issue type issue: issue, action: action, } + c.actionsCacheMu.Lock() c.actionsCache[*action.GetUuid()] = cached + c.actionsCacheMu.Unlock() } } -func (c *CodeActionsService) ResolveCodeAction(action types.LSPCodeAction) (types.LSPCodeAction, error) { +// ResolveCodeAction resolves a cached code action by invoking its deferred edit +// and returning the resulting LSPCodeAction. The cache entry is kept alive for +// the duration of the edit so that a concurrent retry for the same UUID can +// still find it (e.g. a client that timed out and re-sent the request). The +// entry is always removed after the edit — even if the edit panics (the jrpc2 +// handler recovers panics, keeping the process alive) — because the delete is +// registered as a deferred call before the edit runs. +// +// Concurrent resolves of the same UUID are not deduplicated at this layer: both +// callers may invoke the deferred edit independently. Providers must be safe for +// concurrent calls; the remy provider serializes per content-root. +func (c *CodeActionsService) ResolveCodeAction(ctx context.Context, action types.LSPCodeAction) (types.LSPCodeAction, error) { c.logger.Debug().Msg("Received code action resolve request") t := time.Now() @@ -277,14 +305,35 @@ func (c *CodeActionsService) ResolveCodeAction(action types.LSPCodeAction) (type } key := uuid.UUID(*action.Data) + + // Read under lock, then release before invoking the slow deferred edit. + // The entry stays in the cache while the edit runs so that a concurrent + // retry for the same UUID (e.g. a client that timed out and re-sent the + // request) can still find it instead of receiving a hard "not found" error. + c.actionsCacheMu.Lock() cached, found := c.actionsCache[key] + c.actionsCacheMu.Unlock() + if !found { return types.LSPCodeAction{}, errors.New(fmt.Sprint("could not find cached action for uuid ", key)) } - // only delete cache entry after it's been resolved - defer delete(c.actionsCache, key) - edit := (*cached.action.GetDeferredEdit())() + // Remove the cache entry once the edit is done, even if it panics. + // Using defer guarantees cleanup on any exit path — normal return or panic. + defer func() { + c.actionsCacheMu.Lock() + delete(c.actionsCache, key) + c.actionsCacheMu.Unlock() + }() + + // Invoke the deferred edit outside the lock. When DeferredEdit is nil the + // action carries no edit (e.g. a command-only CodeAction); skip the call and + // leave edit as nil so the resolved action is returned without an Edit field. + var edit *types.WorkspaceEdit + if de := cached.action.GetDeferredEdit(); de != nil { + edit = (*de)(ctx) + } + resolvedAction := cached.action resolvedAction.SetEdit(edit) elapsed := time.Since(t) diff --git a/application/codeaction/codeaction_test.go b/application/codeaction/codeaction_test.go index 34d373b10..f04c2ffe6 100644 --- a/application/codeaction/codeaction_test.go +++ b/application/codeaction/codeaction_test.go @@ -17,6 +17,7 @@ package codeaction_test import ( + "context" "testing" "github.com/golang/mock/gomock" @@ -124,6 +125,122 @@ func Test_GetCodeActions_NoIssues_ReturnsNil(t *testing.T) { assert.Nil(t, actions) } +// Test_ResolveCodeAction_CacheEntryPresentDuringDeferredEdit verifies that the +// cache entry for a code action is still present while the deferred edit is +// executing, and is removed only after ResolveCodeAction returns. This ensures +// that a client retry arriving while the first resolve is still computing can +// still find the entry instead of receiving a hard "not found" error. +func Test_ResolveCodeAction_CacheEntryPresentDuringDeferredEdit(t *testing.T) { + engine := testutil.UnitTest(t) + + id := uuid.New() + + // svcRef holds the service pointer so the deferred-edit closure can call + // CacheLenForTest without a forward-reference compile error. + var svcRef *codeaction.CodeActionsService + + // cacheLenDuringEdit captures the cache length as observed from inside the deferred edit. + var cacheLenDuringEdit int + deferredEdit := func(_ context.Context) *types.WorkspaceEdit { + // While the edit is running, the entry must still be in the cache so + // that a concurrent retry can find it. + cacheLenDuringEdit = codeaction.CacheLenForTest(svcRef) + return nil + } + + issue := &snyk.Issue{ + CodeActions: []types.CodeAction{ + &snyk.CodeAction{ + Title: "Fix with remy", + OriginalTitle: "Fix with remy", + DeferredEdit: &deferredEdit, + Uuid: &id, + }, + }, + } + + service, _, _ := setupWithSingleIssue(t, engine, issue) + svcRef = service + + // Populate the cache. + params := types.CodeActionParams{ + TextDocument: sglsp.TextDocumentIdentifier{URI: documentUriExample}, + Range: exampleRange, + Context: types.CodeActionContext{}, + } + actions := service.GetCodeActions(params) + assert.Equal(t, 1, codeaction.CacheLenForTest(service), "cache must hold exactly one entry after GetCodeActions") + + // Resolve the action — this invokes deferredEdit, which reads cacheLenDuringEdit. + lspAction := actions[0] + _, err := service.ResolveCodeAction(t.Context(), lspAction) + assert.NoError(t, err) + + // The entry must have been present during the edit (i.e. not yet deleted). + assert.Equal(t, 1, cacheLenDuringEdit, + "cache entry must still be present while the deferred edit is executing") + // After ResolveCodeAction returns the entry must be gone. + assert.Equal(t, 0, codeaction.CacheLenForTest(service), + "cache entry must be removed after ResolveCodeAction returns") +} + +// Test_ResolveCodeAction_PanicInDeferredEdit_CacheEntryRemoved verifies that +// the cache entry is deleted even when the deferred edit function panics. If +// the delete is done with an explicit (non-deferred) block after the edit call, +// a panic aborts that path and the entry leaks forever. With a defer the entry +// is always cleaned up. +// +// The test: +// 1. Caches an action whose deferred edit panics. +// 2. Calls ResolveCodeAction — the caller recovers the panic (simulating the +// jrpc2 handler recovery), so the goroutine stays alive. +// 3. Asserts that the cache length is 0 after recovery (entry must be gone). +func Test_ResolveCodeAction_PanicInDeferredEdit_CacheEntryRemoved(t *testing.T) { + engine := testutil.UnitTest(t) + + id := uuid.New() + deferredEdit := func(_ context.Context) *types.WorkspaceEdit { + panic("simulated provider panic") + } + + issue := &snyk.Issue{ + CodeActions: []types.CodeAction{ + &snyk.CodeAction{ + Title: "Fix with remy", + OriginalTitle: "Fix with remy", + DeferredEdit: &deferredEdit, + Uuid: &id, + }, + }, + } + + service, _, _ := setupWithSingleIssue(t, engine, issue) + + // Populate the cache. + params := types.CodeActionParams{ + TextDocument: sglsp.TextDocumentIdentifier{URI: documentUriExample}, + Range: exampleRange, + Context: types.CodeActionContext{}, + } + actions := service.GetCodeActions(params) + assert.Equal(t, 1, codeaction.CacheLenForTest(service), + "cache must hold exactly one entry after GetCodeActions") + + lspAction := actions[0] + + // Invoke ResolveCodeAction and recover the panic (mimicking jrpc2 recovery). + func() { + defer func() { _ = recover() }() + _, _ = service.ResolveCodeAction(t.Context(), lspAction) + }() + + // Whether the delete used defer or an explicit block determines whether the + // entry survives the panic. With defer it must be gone; with an explicit + // block it leaks (CacheLen would be 1). + assert.Equal(t, 0, codeaction.CacheLenForTest(service), + "cache entry must be removed even when the deferred edit panics") +} + func Test_ResolveCodeAction_ReturnsCorrectEdit(t *testing.T) { engine := testutil.UnitTest(t) // Arrange @@ -140,7 +257,7 @@ func Test_ResolveCodeAction_ReturnsCorrectEdit(t *testing.T) { "someUri": {mockTextEdit}, }, } - deferredEdit := func() *types.WorkspaceEdit { + deferredEdit := func(_ context.Context) *types.WorkspaceEdit { return mockEdit } id := uuid.New() @@ -159,7 +276,7 @@ func Test_ResolveCodeAction_ReturnsCorrectEdit(t *testing.T) { // Act actions := service.GetCodeActions(codeActionsParam) actionFromRequest := actions[0] - resolvedAction, _ := service.ResolveCodeAction(actionFromRequest) + resolvedAction, _ := service.ResolveCodeAction(t.Context(), actionFromRequest) // Assert assert.NotNil(t, resolvedAction) @@ -169,6 +286,70 @@ func Test_ResolveCodeAction_ReturnsCorrectEdit(t *testing.T) { assert.NotNil(t, resolvedAction.Edit) } +// Test_ResolveCodeAction_NilDeferredEdit_NoPanic verifies that resolving a cached +// action whose DeferredEdit is nil (e.g. a command-only CodeAction) does not panic. +// Previously, the code unconditionally dereferenced GetDeferredEdit(), causing a +// nil-pointer panic for command-only actions. +func Test_ResolveCodeAction_NilDeferredEdit_NoPanic(t *testing.T) { + engine := testutil.UnitTest(t) + + // Build a deferred action with a command but no edit (DeferredEdit == nil). + deferredCmd := func() *types.CommandData { + return &types.CommandData{CommandId: "some.command"} + } + id := uuid.New() + action := &snyk.CodeAction{ + Title: "Command only", + OriginalTitle: "Command only", + DeferredCommand: &deferredCmd, + Uuid: &id, + } + // DeferredEdit is nil — this is the scenario under test. + if action.GetDeferredEdit() != nil { + t.Fatal("test setup error: DeferredEdit must be nil for this test") + } + + issue := &snyk.Issue{} + + // Wire a service and manually populate the cache by embedding the action in an issue. + _, _ = workspaceutil.SetupWorkspace(t, engine, types.FilePath("/path/to")) + ctrl := gomock.NewController(t) + providerMock := mock_snyk.NewMockIssueProvider(ctrl) + providerMock.EXPECT().IssuesForRange(gomock.Any(), gomock.Any()). + Return([]types.Issue{issue}).AnyTimes() + // The issue carries the action so GetCodeActions caches it. + issue.CodeActions = []types.CodeAction{action} + + service := codeaction.NewService(engine, providerMock, watcher.NewFileWatcher(), notification.NewMockNotifier(), featureflag.NewFakeService(), types.NewConfigResolver(engine.GetLogger()), nil) + codeActionsParam := types.CodeActionParams{ + TextDocument: sglsp.TextDocumentIdentifier{URI: documentUriExample}, + Range: exampleRange, + Context: types.CodeActionContext{}, + } + + // Populate the cache via GetCodeActions. + actions := service.GetCodeActions(codeActionsParam) + + // Find the action with a Data field (deferred/cached actions have Data set). + var cachedAction *types.LSPCodeAction + for i := range actions { + if actions[i].Data != nil { + cachedAction = &actions[i] + break + } + } + if cachedAction == nil { + t.Skip("no cached action found; test setup may have changed") + return + } + + // Must not panic even though DeferredEdit is nil. + resolved, err := service.ResolveCodeAction(t.Context(), *cachedAction) + assert.NoError(t, err, "resolving a command-only action must not error") + // Without a deferred edit the resolved action must have no edit. + assert.Nil(t, resolved.Edit, "command-only action must produce no edit") +} + func Test_ResolveCodeAction_KeyDoesNotExist_ReturnError(t *testing.T) { engine := testutil.UnitTest(t) // Arrange @@ -184,7 +365,7 @@ func Test_ResolveCodeAction_KeyDoesNotExist_ReturnError(t *testing.T) { // Act var err error - _, err = service.ResolveCodeAction(ca) + _, err = service.ResolveCodeAction(t.Context(), ca) // Assert assert.Error(t, err, "Expected error when resolving a code action with a key that doesn't exist") @@ -201,7 +382,7 @@ func Test_ResolveCodeAction_KeyAndCommandIsNull_ReturnsError(t *testing.T) { Data: nil, } - _, err := service.ResolveCodeAction(ca) + _, err := service.ResolveCodeAction(t.Context(), ca) assert.Error(t, err, "Expected error when resolving a code action with a null key") assert.True(t, codeaction.IsMissingKeyError(err)) } @@ -217,7 +398,7 @@ func Test_ResolveCodeAction_KeyIsNull_ReturnsCodeAction(t *testing.T) { Data: nil, } - actual, err := service.ResolveCodeAction(expected) + actual, err := service.ResolveCodeAction(t.Context(), expected) assert.NoError(t, err, "Expected error when resolving a code action with a null key") assert.Equal(t, expected.Command.Command, actual.Command.Command) } diff --git a/application/codeaction/export_test.go b/application/codeaction/export_test.go new file mode 100644 index 000000000..0ff5f082c --- /dev/null +++ b/application/codeaction/export_test.go @@ -0,0 +1,32 @@ +/* + * © 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 codeaction — test-only exports. +// This file is compiled only during testing (it is a _test.go file in package +// codeaction, not package codeaction_test). It gives external test packages +// access to unexported fields without polluting the production binary's API +// surface. +package codeaction + +// CacheLenForTest returns the number of entries currently held in the actions +// cache. It is intentionally NOT a method on CodeActionsService so that it does +// not appear in the production type's public API; the _test.go suffix guarantees +// this file is excluded from non-test builds. +func CacheLenForTest(c *CodeActionsService) int { + c.actionsCacheMu.Lock() + defer c.actionsCacheMu.Unlock() + return len(c.actionsCache) +} diff --git a/application/codeaction/race_test.go b/application/codeaction/race_test.go new file mode 100644 index 000000000..cfcd5eb6c --- /dev/null +++ b/application/codeaction/race_test.go @@ -0,0 +1,145 @@ +/* + * © 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. + */ + +// This file contains race-detector tests for CodeActionsService. Run with: +// +// go test -race ./application/codeaction/... +package codeaction_test + +import ( + "context" + "sync" + "sync/atomic" + "testing" + + "github.com/golang/mock/gomock" + "github.com/google/uuid" + sglsp "github.com/sourcegraph/go-lsp" + "github.com/stretchr/testify/require" + + "github.com/snyk/snyk-ls/application/codeaction" + "github.com/snyk/snyk-ls/application/watcher" + "github.com/snyk/snyk-ls/domain/ide/converter" + "github.com/snyk/snyk-ls/domain/snyk" + "github.com/snyk/snyk-ls/domain/snyk/mock_snyk" + "github.com/snyk/snyk-ls/infrastructure/featureflag" + "github.com/snyk/snyk-ls/internal/notification" + "github.com/snyk/snyk-ls/internal/testutil" + "github.com/snyk/snyk-ls/internal/testutil/workspaceutil" + "github.com/snyk/snyk-ls/internal/types" + "github.com/snyk/snyk-ls/internal/uri" +) + +// TestActionsCache_NoDataRace verifies that concurrent writes to actionsCache +// (via GetCodeActions → cacheCodeAction) and concurrent reads/deletes (via +// ResolveCodeAction) do not produce a data race. +// +// Pattern: +// - One writer goroutine repeatedly calls GetCodeActions with fresh issue sets +// (each cycle produces a brand-new issue list so no CodeAction is shared +// between writer iterations — only actionsCache itself is shared). +// - Many reader goroutines call ResolveCodeAction on actions that were +// pre-cached by the initial serial call. +// +// Without actionsCacheMu the Go race detector reports "concurrent map +// read/write"; with the mutex the test completes cleanly. +func TestActionsCache_NoDataRace(t *testing.T) { + engine := testutil.UnitTest(t) + r := exampleRange + uriPath := documentUriExample + path := uri.PathFromUri(uriPath) + + _, _ = workspaceutil.SetupWorkspace(t, engine, types.FilePath("/path/to")) + + // issueGen generates a brand-new issue list on each call so that no + // CodeAction object is shared between goroutines (only actionsCache is shared). + const batchSize = 10 + var callCount atomic.Int32 + issueGen := func() []types.Issue { + n := int(callCount.Add(1)) + issues := make([]types.Issue, batchSize) + for i := range batchSize { + id := uuid.New() + idCopy := id + de := func(_ context.Context) *types.WorkspaceEdit { return nil } + action, _ := snyk.NewDeferredCodeAction("Fix"+string(rune('A'+n%26)), &de, nil, "", nil) + action.Uuid = &idCopy + issues[i] = &snyk.Issue{CodeActions: []types.CodeAction{&action}} + } + return issues + } + + ctrl := gomock.NewController(t) + providerMock := mock_snyk.NewMockIssueProvider(ctrl) + providerMock.EXPECT(). + IssuesForRange(gomock.Eq(path), gomock.Eq(converter.FromRange(r))). + DoAndReturn(func(_ types.FilePath, _ types.Range) []types.Issue { + return issueGen() + }). + AnyTimes() + + service := codeaction.NewService( + engine, + providerMock, + watcher.NewFileWatcher(), + notification.NewMockNotifier(), + featureflag.NewFakeService(), + types.NewConfigResolver(engine.GetLogger()), + nil, + ) + + params := types.CodeActionParams{ + TextDocument: sglsp.TextDocumentIdentifier{URI: uriPath}, + Range: r, + Context: types.CodeActionContext{}, + } + + // Serial pre-population: cache one batch so readers have entries to find. + initialActions := service.GetCodeActions(params) + require.NotEmpty(t, initialActions, "cache must be populated before the race test") + + ctx := context.Background() + var wg sync.WaitGroup + + // Single writer goroutine: repeatedly calls GetCodeActions to drive cache writes. + // Each call produces its own fresh issue list (via issueGen) so no CodeAction + // pointer is shared between calls, keeping the only shared state as actionsCache. + wg.Add(1) + go func() { + defer wg.Done() + for range 50 { + service.GetCodeActions(params) + } + }() + + // Reader goroutines: each calls ResolveCodeAction on one pre-cached LSP action. + // This exercises the concurrent map lookup+delete path. + for _, a := range initialActions { + wg.Add(1) + go func() { + defer wg.Done() + for range 5 { + // Entry may already be deleted on the first call; errors are expected. + // We only care about the absence of a concurrent-map-access panic. + _, _ = service.ResolveCodeAction(ctx, a) + } + }() + } + + wg.Wait() + // Reaching here under -race without a "concurrent map read/write" panic confirms + // that all actionsCache accesses are properly serialized by actionsCacheMu. +} diff --git a/application/codeaction/remediation_test.go b/application/codeaction/remediation_test.go index 231053ab3..646c2dc6d 100644 --- a/application/codeaction/remediation_test.go +++ b/application/codeaction/remediation_test.go @@ -182,7 +182,7 @@ func TestResolveCodeAction_RemediationAgent_InvokesProvider(t *testing.T) { } require.NotNil(t, remyAction, "expected RemediationAgentQuickFix action") - resolved, err := service.ResolveCodeAction(*remyAction) + resolved, err := service.ResolveCodeAction(t.Context(), *remyAction) require.NoError(t, err) require.NotNil(t, resolved.Edit, "resolved action must have an edit") // The converter transforms the key via uri.PathToUri; just verify the edit is populated. @@ -210,6 +210,35 @@ func TestGetCodeActions_RemediationAgent_NonCodeProduct_NoAction(t *testing.T) { } } +// TestGetCodeActions_RemediationAgent_FullyFixableOSS_NoAction verifies that an +// OSS issue where IsFixable() returns true (upgradable with a real upgrade path) +// does NOT receive a RemediationAgentQuickFix action. The remy agent is designed +// for Code (hasAIFix) and IaC issues; OSS upgrades are handled by a separate +// mechanism (package manager upgrade, not LLM code edits). +func TestGetCodeActions_RemediationAgent_FullyFixableOSS_NoAction(t *testing.T) { + fake := &fakeRemediationProvider{edit: &types.WorkspaceEdit{}} + + // Construct an OSS issue where IsFixable() returns true: + // IsUpgradable=true, UpgradePath[1] != From[1], len(UpgradePath)>1, len(From)>1. + ossIssue := &snyk.Issue{ + FindingId: "finding-oss-fixable", + Product: product.ProductOpenSource, + AdditionalData: snyk.OssIssueData{ + IsUpgradable: true, + UpgradePath: []any{"lodash@4.17.21", "lodash@4.17.21"}, + From: []string{"app@1.0.0", "lodash@4.17.4"}, + }, + } + + service, params := setupWithIssueAndProvider(t, ossIssue, fake) + actions := service.GetCodeActions(params) + + for _, a := range actions { + assert.NotEqual(t, types.RemediationAgentQuickFix, a.Kind, + "fully-fixable OSS issue (IsFixable()==true) must not produce RemediationAgentQuickFix actions; remy handles Code/IaC only") + } +} + func TestGetCodeActions_RemediationAgent_NotAIFixable_NoAction(t *testing.T) { fake := &fakeRemediationProvider{edit: &types.WorkspaceEdit{}} @@ -312,3 +341,53 @@ func TestGetCodeActions_RemediationAgent_IaCIssue_WithFindingId_OfferedAction(t } assert.True(t, found, "IaC issue with FindingId must produce RemediationAgentQuickFix action") } + +// recordingRemediationProvider captures the context passed to Remediate. +type recordingRemediationProvider struct { + receivedCtx context.Context +} + +func (r *recordingRemediationProvider) Remediate(ctx context.Context, _ remediation.RemediationRequest) (*types.WorkspaceEdit, error) { + r.receivedCtx = ctx + return &types.WorkspaceEdit{}, nil +} + +// TestResolveCodeAction_RemediationAgent_PropagatesContext proves that the +// context supplied to ResolveCodeAction is threaded through to the provider. +// Before the fix, provider.Remediate receives context.Background() even when +// ResolveCodeAction is called with a cancellable context; after the fix it +// receives the passed context, so canceling that context also cancels the +// provider call. +func TestResolveCodeAction_RemediationAgent_PropagatesContext(t *testing.T) { + recorder := &recordingRemediationProvider{} + issue := buildFixableIssue("finding-ctx") + + service, params := setupWithIssueAndProvider(t, issue, recorder) + + actions := service.GetCodeActions(params) + + var remyAction *types.LSPCodeAction + for i := range actions { + if actions[i].Kind == types.RemediationAgentQuickFix { + remyAction = &actions[i] + break + } + } + require.NotNil(t, remyAction, "expected RemediationAgentQuickFix action") + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // pre-cancel so we can assert the provider received a Done ctx + + _, err := service.ResolveCodeAction(ctx, *remyAction) + require.NoError(t, err) + + // The provider must have received the same (canceled) context — not + // context.Background(), which would never be Done. + require.NotNil(t, recorder.receivedCtx, "provider must have been called") + select { + case <-recorder.receivedCtx.Done(): + // correct: the provider received a canceled context + default: + t.Fatal("provider received a non-canceled context; cancellation was not propagated") + } +} diff --git a/application/di/init.go b/application/di/init.go index 0976ed972..90cc0ad75 100644 --- a/application/di/init.go +++ b/application/di/init.go @@ -235,7 +235,7 @@ func initApplication(conf configuration.Configuration, engine workflow.Engine, l var remediationProvider remediation.RemediationProvider var folderRemediator remediation.FolderRemediator remediationNotifier = nil - if conf.GetBool(remediationAgentEnabledKey) { + if conf.GetBool(RemediationAgentEnabledKey) { p := remediation.NewRemyProvider(engine, nil) remediationProvider = p if n, ok := p.(remediation.FileChangeNotifier); ok { @@ -250,11 +250,14 @@ func initApplication(conf configuration.Configuration, engine workflow.Engine, l 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 +// RemediationAgentEnabledKey is the configuration key that gates the remy-backed // remediation provider. The feature ships off by default; set this key to true // only when the host CLI bundles the remy subcommand and an LLM API key is // available. -const remediationAgentEnabledKey = "remediation_agent_enabled" +// +// The key is exported so that server.go and other callers can reference the +// same constant rather than duplicating the raw string literal. +const RemediationAgentEnabledKey = "remediation_agent_enabled" /* TODO Accessors: This should go away, since all dependencies should be satisfied at startup-time, if needed for testing diff --git a/application/di/init_fix_folder_test.go b/application/di/init_fix_folder_test.go index b688e9b71..4a2240c5a 100644 --- a/application/di/init_fix_folder_test.go +++ b/application/di/init_fix_folder_test.go @@ -49,7 +49,7 @@ func TestDI_FixFolderCommand_WiredWhenEnabled(t *testing.T) { folderURI := string(uri.PathToUri(types.FilePath(repo))) // Set the flag so initApplication builds the real remediationProvider. - engine.GetConfiguration().Set("remediation_agent_enabled", true) + engine.GetConfiguration().Set(di.RemediationAgentEnabledKey, true) // Call di.Init to exercise the real wiring path. deps := di.Init(engine, tokenService) diff --git a/application/server/codeaction_handlers.go b/application/server/codeaction_handlers.go index bb966e8c9..6b3b3c8c8 100644 --- a/application/server/codeaction_handlers.go +++ b/application/server/codeaction_handlers.go @@ -37,7 +37,7 @@ func ResolveCodeActionHandler(logger *zerolog.Logger, service *codeaction.CodeAc l = l.With().Interface("request", params).Logger() l.Debug().Msg("RECEIVING") - action, err := service.ResolveCodeAction(params) + action, err := service.ResolveCodeAction(ctx, params) if err != nil { if codeaction.IsMissingKeyError(err) { l.Debug().Msg("Skipping code action - missing key") diff --git a/application/server/server.go b/application/server/server.go index 896326e14..9a6c61700 100644 --- a/application/server/server.go +++ b/application/server/server.go @@ -703,41 +703,7 @@ func initializeHandler(conf configuration.Configuration, engine workflow.Engine, CodeLensProvider: &sglsp.CodeLensOptions{ResolveProvider: false}, InlineValueProvider: true, ExecuteCommandProvider: &sglsp.ExecuteCommandOptions{ - Commands: []string{ - types.NavigateToRangeCommand, - types.WorkspaceScanCommand, - types.WorkspaceFolderScanCommand, - types.OpenBrowserCommand, - types.LoginCommand, - types.CopyAuthLinkCommand, - types.LogoutCommand, - types.TrustWorkspaceFoldersCommand, - types.OpenLearnLesson, - types.GetLearnLesson, - types.SubmitIgnoreRequest, - types.GetSettingsSastEnabled, - types.GetFeatureFlagStatus, - types.GetActiveUserCommand, - types.CodeFixCommand, - types.CodeSubmitFixFeedback, - types.CodeFixDiffsCommand, - types.CodeFixApplyEditCommand, - types.ExecuteCLICommand, - types.ConnectivityCheckCommand, - types.DirectoryDiagnosticsCommand, - types.ClearCacheCommand, - types.GenerateIssueDescriptionCommand, - types.ReportAnalyticsCommand, - types.WorkspaceConfigurationCommand, - types.GetTreeView, - types.ToggleTreeFilter, - types.SetNodeExpanded, - types.ShowScanErrorDetails, - types.UpdateFolderConfig, - types.DismissFeedbackBanner, - types.FeedbackBannerInteracted, - types.RemediationAgentFixFolderCommand, - }, + Commands: remediationGatedCommands(conf), }, }, } @@ -748,6 +714,51 @@ func initializeHandler(conf configuration.Configuration, engine workflow.Engine, }) } +// remediationGatedCommands returns the full command list for ExecuteCommandProvider, +// appending RemediationAgentFixFolderCommand only when di.RemediationAgentEnabledKey +// is true. Advertising a command the feature flag is off teaches the client that the +// command exists, which leads to "command not found" errors when the client calls it. +func remediationGatedCommands(conf configuration.Configuration) []string { + cmds := []string{ + types.NavigateToRangeCommand, + types.WorkspaceScanCommand, + types.WorkspaceFolderScanCommand, + types.OpenBrowserCommand, + types.LoginCommand, + types.CopyAuthLinkCommand, + types.LogoutCommand, + types.TrustWorkspaceFoldersCommand, + types.OpenLearnLesson, + types.GetLearnLesson, + types.SubmitIgnoreRequest, + types.GetSettingsSastEnabled, + types.GetFeatureFlagStatus, + types.GetActiveUserCommand, + types.CodeFixCommand, + types.CodeSubmitFixFeedback, + types.CodeFixDiffsCommand, + types.CodeFixApplyEditCommand, + types.ExecuteCLICommand, + types.ConnectivityCheckCommand, + types.DirectoryDiagnosticsCommand, + types.ClearCacheCommand, + types.GenerateIssueDescriptionCommand, + types.ReportAnalyticsCommand, + types.WorkspaceConfigurationCommand, + types.GetTreeView, + types.ToggleTreeFilter, + types.SetNodeExpanded, + types.ShowScanErrorDetails, + types.UpdateFolderConfig, + types.DismissFeedbackBanner, + types.FeedbackBannerInteracted, + } + if conf.GetBool(di.RemediationAgentEnabledKey) { + cmds = append(cmds, types.RemediationAgentFixFolderCommand) + } + return cmds +} + func startClientMonitor(params types.InitializeParams, logger zerolog.Logger) { go func() { if params.ProcessID == 0 { diff --git a/application/server/server_smoke_test.go b/application/server/server_smoke_test.go index 97efc907a..478b3b072 100644 --- a/application/server/server_smoke_test.go +++ b/application/server/server_smoke_test.go @@ -2552,7 +2552,7 @@ func Test_Smoke_RemediationAgent_CodeAction(t *testing.T) { } engine, tokenService := testutil.SmokeTestWithEngine(t, "", "SMOKE_SHARD_4") - engine.GetConfiguration().Set("remediation_agent_enabled", true) + engine.GetConfiguration().Set(di.RemediationAgentEnabledKey, true) testutil.CreateDummyProgressListener(t) repoTempDir := types.FilePath(testutil.TempDirWithRetry(t)) diff --git a/application/server/server_test.go b/application/server/server_test.go index 3f8ce71d8..a21963d5b 100644 --- a/application/server/server_test.go +++ b/application/server/server_test.go @@ -1747,7 +1747,7 @@ func TestInitializeHandler_MissingDep_PropagatesLSPError(t *testing.T) { // 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) + engine.GetConfiguration().Set(di.RemediationAgentEnabledKey, true) loc, _, _ := setupServer(t, engine, tokenService) testutil.CreateDummyProgressListener(t) @@ -1778,7 +1778,7 @@ func Test_textDocumentDidChange_WithRemediationEnabled_NoRPCError(t *testing.T) 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) + engine.GetConfiguration().Set(di.RemediationAgentEnabledKey, true) loc, _, _ := setupServer(t, engine, tokenService) testutil.CreateDummyProgressListener(t) @@ -1911,3 +1911,39 @@ func Test_codeActionResolve_RemediationAgent_ReturnsEdit(t *testing.T) { 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") } + +// Test_initialize_RemediationFixFolderCommand_NotAdvertised_WhenFlagOff asserts that +// RemediationAgentFixFolderCommand is absent from the advertised commands when the +// remediation_agent_enabled flag is false (the default). Advertising a command the +// feature flag is off teaches the client that the command exists and that it should +// call it, which leads to a "command not found" error at runtime. +func Test_initialize_RemediationFixFolderCommand_NotAdvertised_WhenFlagOff(t *testing.T) { + engine, tokenService := testutil.UnitTestWithEngine(t) + // remediation_agent_enabled is false by default; confirm the command is absent. + engine.GetConfiguration().Set(di.RemediationAgentEnabledKey, false) + loc, _, _ := setupServer(t, engine, tokenService) + + rsp, err := loc.Client.Call(t.Context(), "initialize", nil) + require.NoError(t, err) + var result types.InitializeResult + require.NoError(t, rsp.UnmarshalResult(&result)) + + assert.NotContains(t, result.Capabilities.ExecuteCommandProvider.Commands, types.RemediationAgentFixFolderCommand, + "RemediationAgentFixFolderCommand must not be advertised when remediation_agent_enabled=false") +} + +// Test_initialize_RemediationFixFolderCommand_Advertised_WhenFlagOn asserts that +// RemediationAgentFixFolderCommand IS advertised when remediation_agent_enabled=true. +func Test_initialize_RemediationFixFolderCommand_Advertised_WhenFlagOn(t *testing.T) { + engine, tokenService := testutil.UnitTestWithEngine(t) + engine.GetConfiguration().Set(di.RemediationAgentEnabledKey, true) + loc, _, _ := setupServer(t, engine, tokenService) + + rsp, err := loc.Client.Call(t.Context(), "initialize", nil) + require.NoError(t, err) + var result types.InitializeResult + require.NoError(t, rsp.UnmarshalResult(&result)) + + assert.Contains(t, result.Capabilities.ExecuteCommandProvider.Commands, types.RemediationAgentFixFolderCommand, + "RemediationAgentFixFolderCommand must be advertised when remediation_agent_enabled=true") +} diff --git a/domain/ide/command/code_fix.go b/domain/ide/command/code_fix.go index fad06a9ad..12c7cfc5a 100644 --- a/domain/ide/command/code_fix.go +++ b/domain/ide/command/code_fix.go @@ -44,7 +44,7 @@ func (cmd *fixCodeIssue) Command() types.CommandData { return cmd.command } -func (cmd *fixCodeIssue) Execute(_ context.Context) (any, error) { +func (cmd *fixCodeIssue) Execute(ctx context.Context) (any, error) { key := types.SettingClientCapabilities capabilities, _ := cmd.engine.GetConfiguration().Get(key).(types.ClientCapabilities) if !capabilities.Workspace.ApplyEdit { @@ -71,7 +71,12 @@ func (cmd *fixCodeIssue) Execute(_ context.Context) (any, error) { } // execute autofix codeaction - edit := (*action.GetDeferredEdit())() + de := action.GetDeferredEdit() + if de == nil { + cmd.logger.Debug().Msg("No deferred edit available for code action.") + return nil, nil + } + edit := (*de)(ctx) if edit == nil { cmd.logger.Debug().Msg("No fix could be computed.") return nil, nil diff --git a/domain/ide/command/code_fix_test.go b/domain/ide/command/code_fix_test.go index 45b6a3aa8..f741b07d7 100644 --- a/domain/ide/command/code_fix_test.go +++ b/domain/ide/command/code_fix_test.go @@ -17,6 +17,7 @@ package command import ( + "context" "testing" "github.com/golang/mock/gomock" @@ -76,7 +77,7 @@ func setupCommand(t *testing.T, engine workflow.Engine, mockNotifier *notificati return cmd } -func setupMockEdit() (edit *types.WorkspaceEdit, deferredEdit func() *types.WorkspaceEdit) { +func setupMockEdit() (edit *types.WorkspaceEdit, deferredEdit func(context.Context) *types.WorkspaceEdit) { var mockTextEdit = types.TextEdit{ Range: types.Range{ Start: types.Position{Line: 1, Character: 2}, @@ -88,7 +89,7 @@ func setupMockEdit() (edit *types.WorkspaceEdit, deferredEdit func() *types.Work "someUri": {mockTextEdit}, }, } - var deferredMockEdit = func() *types.WorkspaceEdit { + var deferredMockEdit = func(_ context.Context) *types.WorkspaceEdit { return mockEdit } return mockEdit, deferredMockEdit @@ -181,7 +182,7 @@ func Test_fixCodeIssue_noEdit(t *testing.T) { require.True(t, ok) issueRange, err := cmd.toRange(rangeDto) require.NoError(t, err) - deferredMockEdit := func() *types.WorkspaceEdit { + deferredMockEdit := func(_ context.Context) *types.WorkspaceEdit { return nil } codeAction := snyk.CodeAction{ @@ -210,6 +211,45 @@ func Test_fixCodeIssue_noEdit(t *testing.T) { assert.Equal(t, sentMessages, mockNotifier.SentMessages()) } +func Test_fixCodeIssue_nilDeferredEdit_noOp(t *testing.T) { + // A CodeAction that has no DeferredEdit (nil) should not panic and should + // return a no-op result — matching the guard already present in ResolveCodeAction. + engine := testutil.UnitTest(t) + ctrl := gomock.NewController(t) + setupClientCapability(engine) + + mockNotifier := notification.NewMockNotifier() + cmd := setupCommand(t, engine, mockNotifier) + + filePath := sampleArgs[1].(string) + path := types.FilePath(filePath) + issueRange, err := cmd.toRange(sampleArgs[2]) + require.NoError(t, err) + + // CodeAction with nil DeferredEdit — simulates a command-only CodeAction + codeAction := snyk.CodeAction{ + Uuid: &codeActionId, + DeferredEdit: nil, + } + issues := setupSampleIssues(issueRange, codeAction, cmd.command) + issueMap := snyk.IssuesByFile{ + path: issues, + } + + issueProviderMock := mock_snyk.NewMockIssueProvider(ctrl) + issueProviderMock.EXPECT().Issues().Return(issueMap) + cmd.issueProvider = issueProviderMock + + // Must not panic; must return no-op (nil, nil) + res, err := cmd.Execute(t.Context()) + + assert.NoError(t, err) + assert.Nil(t, res) + // No edit notification should be sent + var expectedMessages []any + assert.Equal(t, expectedMessages, mockNotifier.SentMessages()) +} + func Test_fixCodeIssue_NoIssueFound(t *testing.T) { engine := testutil.UnitTest(t) // arrange diff --git a/domain/snyk/codeaction.go b/domain/snyk/codeaction.go index d151f4ec0..2ca3ac70e 100644 --- a/domain/snyk/codeaction.go +++ b/domain/snyk/codeaction.go @@ -18,6 +18,7 @@ package snyk import ( + "context" "errors" "github.com/google/uuid" @@ -51,8 +52,11 @@ type CodeAction struct { // DeferredEdit is a function that returns a WorkspaceEdit. // Used for heavy calculations that shouldn't be done ahead of time. + // The context passed to the function propagates cancellation from the + // codeAction/resolve request, allowing long-running operations to be + // interrupted when the client disconnects or abandons the request. // A CodeAction cannot have both Edit and DeferredEdit. - DeferredEdit *func() *types.WorkspaceEdit `json:"-"` + DeferredEdit *func(context.Context) *types.WorkspaceEdit `json:"-"` // Command that will be executed after the Edit (if present). Command *types.CommandData @@ -102,7 +106,7 @@ func (c *CodeAction) GetEdit() *types.WorkspaceEdit { return c.Edit } -func (c *CodeAction) GetDeferredEdit() *func() *types.WorkspaceEdit { +func (c *CodeAction) GetDeferredEdit() *func(context.Context) *types.WorkspaceEdit { return c.DeferredEdit } @@ -152,7 +156,7 @@ func NewCodeAction(title string, edit *types.WorkspaceEdit, command *types.Comma func NewDeferredCodeAction( title string, - deferredEdit *func() *types.WorkspaceEdit, + deferredEdit *func(context.Context) *types.WorkspaceEdit, deferredCommand *func() *types.CommandData, groupingKey types.Key, groupingValue any, diff --git a/domain/snyk/codeaction_test.go b/domain/snyk/codeaction_test.go index e6bd7e9ff..f15aad8e4 100644 --- a/domain/snyk/codeaction_test.go +++ b/domain/snyk/codeaction_test.go @@ -17,6 +17,7 @@ package snyk import ( + "context" "testing" "github.com/stretchr/testify/assert" @@ -41,7 +42,7 @@ var mockCommand = &types.CommandData{ Title: "command", } -var mockDeferredEdit = func() *types.WorkspaceEdit { +var mockDeferredEdit = func(_ context.Context) *types.WorkspaceEdit { return mockEdit } @@ -82,7 +83,7 @@ func assertActionsInitializedCorrectly(t *testing.T, action types.CodeAction, expectedEdit *types.WorkspaceEdit, expectedCommand *types.CommandData, - mockDeferredEdit *func() *types.WorkspaceEdit, + mockDeferredEdit *func(context.Context) *types.WorkspaceEdit, mockDeferredCommand *func() *types.CommandData, ) { t.Helper() diff --git a/domain/snyk/remediation/noop.go b/domain/snyk/remediation/noop.go deleted file mode 100644 index f7fcc4ee5..000000000 --- a/domain/snyk/remediation/noop.go +++ /dev/null @@ -1,32 +0,0 @@ -/* - * © 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 - -import ( - "context" - - "github.com/snyk/snyk-ls/internal/types" -) - -// NoopProvider always returns no fix. Replace with a real implementation when available. -type NoopProvider struct{} - -var _ RemediationProvider = (*NoopProvider)(nil) - -func (n *NoopProvider) Remediate(_ context.Context, _ RemediationRequest) (*types.WorkspaceEdit, error) { - return nil, nil -} diff --git a/domain/snyk/remediation/noop_test.go b/domain/snyk/remediation/noop_test.go deleted file mode 100644 index 3349fa737..000000000 --- a/domain/snyk/remediation/noop_test.go +++ /dev/null @@ -1,34 +0,0 @@ -/* - * © 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" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/snyk/snyk-ls/domain/snyk/remediation" -) - -func TestNoopProvider_ReturnsNil(t *testing.T) { - provider := &remediation.NoopProvider{} - edit, err := provider.Remediate(context.Background(), remediation.RemediationRequest{}) - require.NoError(t, err) - assert.Nil(t, edit) -} diff --git a/domain/snyk/remediation/remy.go b/domain/snyk/remediation/remy.go index 4cfdf5b81..599a13977 100644 --- a/domain/snyk/remediation/remy.go +++ b/domain/snyk/remediation/remy.go @@ -354,6 +354,19 @@ func (p *remyProvider) FixFolder(ctx context.Context, root types.FilePath) (*typ 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) } + // Guard: the passed worktree must be clean of tracked-file changes. + // --untracked-files=no excludes "??" lines for untracked files (e.g. build + // artifacts) so they do not falsely trip the guard. Only uncommitted + // modifications to tracked files matter: they would be silently included in + // the returned edit, making the fix unpredictable and violating the isolation + // guarantee that the caller must pass a fresh detached-HEAD worktree. + statusOut, err := exec.CommandContext(ctx, "git", "-C", r, "status", "--porcelain", "--untracked-files=no").Output() + if err != nil { + return nil, fmt.Errorf("remy: FixFolder: failed to check worktree status of %q: %w", r, err) + } + if status := strings.TrimSpace(string(statusOut)); status != "" { + return nil, fmt.Errorf("remy: FixFolder: %q has uncommitted changes to tracked files; the caller must pass a clean detached-HEAD worktree:\n%s", r, status) + } // findingID is "" because FixFolder targets the whole folder, not a single finding. changes, err := p.collectFixEdits(ctx, r, r, "") if err != nil { diff --git a/domain/snyk/remediation/remy_fix_folder_test.go b/domain/snyk/remediation/remy_fix_folder_test.go index 4c44eb734..b1ced196f 100644 --- a/domain/snyk/remediation/remy_fix_folder_test.go +++ b/domain/snyk/remediation/remy_fix_folder_test.go @@ -33,7 +33,7 @@ import ( ) // --------------------------------------------------------------------------- -// UNIT-001: FixFolder returns edits keyed under the passed folder +// FixFolder returns edits keyed under the passed folder // --------------------------------------------------------------------------- // TestFixFolder_ReturnsEditsKeyedUnderFolder verifies that when the fake runner @@ -67,7 +67,7 @@ func TestFixFolder_ReturnsEditsKeyedUnderFolder(t *testing.T) { } // --------------------------------------------------------------------------- -// UNIT-002: FixFolder returns (nil, nil) when runner makes no changes +// FixFolder returns (nil, nil) when runner makes no changes // --------------------------------------------------------------------------- func TestFixFolder_NoChangesReturnsNil(t *testing.T) { @@ -84,7 +84,7 @@ func TestFixFolder_NoChangesReturnsNil(t *testing.T) { } // --------------------------------------------------------------------------- -// UNIT-003: FixFolder propagates runner errors +// FixFolder propagates runner errors // --------------------------------------------------------------------------- func TestFixFolder_PropagatesRunnerError(t *testing.T) { @@ -103,7 +103,7 @@ func TestFixFolder_PropagatesRunnerError(t *testing.T) { } // --------------------------------------------------------------------------- -// UNIT-004: FixFolder rejects non-absolute / empty paths +// FixFolder rejects non-absolute / empty paths // --------------------------------------------------------------------------- func TestFixFolder_RejectsNonAbsolutePath(t *testing.T) { @@ -133,7 +133,7 @@ func TestFixFolder_RejectsNonAbsolutePath(t *testing.T) { } // --------------------------------------------------------------------------- -// UNIT-005a: FixFolder rejects a subdirectory of a git repo with an error +// FixFolder rejects a subdirectory of a git repo with an error // --------------------------------------------------------------------------- // TestFixFolder_SubdirOfGitRoot_ReturnEdits verifies that FixFolder returns a @@ -167,7 +167,7 @@ func TestFixFolder_SubdirOfGitRoot_ReturnEdits(t *testing.T) { } // --------------------------------------------------------------------------- -// UNIT-005b: FixFolder rejects a directory that is not a git repository +// FixFolder rejects a directory that is not a git repository // --------------------------------------------------------------------------- // TestFixFolder_NonGitDirectory_ReturnsError verifies that FixFolder returns a @@ -220,7 +220,7 @@ func TestFixFolder_GitRoot_EditsKeyedUnderPassedRoot(t *testing.T) { } // --------------------------------------------------------------------------- -// UNIT-005: FixFolder runs directly in the passed folder (no nested worktree) +// FixFolder runs directly in the passed folder (no nested worktree) // --------------------------------------------------------------------------- func TestFixFolder_RunsDirectlyInPassedFolder(t *testing.T) { @@ -266,6 +266,119 @@ func TestFixFolder_RunsDirectlyInPassedFolder(t *testing.T) { // DAEMON CONTRACT: FixFolder edit keys must be under the PASSED (non-canonical) path // --------------------------------------------------------------------------- +// TestFixFolder_UncommittedChanges_ReturnsError verifies that FixFolder returns a +// clear error when the passed worktree already has uncommitted working-tree changes. +// The caller contract requires a fresh detached-HEAD worktree; a dirty worktree +// would silently include pre-existing changes in the returned edit, making the fix +// unpredictable and violating the isolation guarantee. +func TestFixFolder_UncommittedChanges_ReturnsError(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "main.go", "package main\nvar x = 1\n") + + // Introduce an uncommitted working-tree change (tracked file, modified but not staged). + err := os.WriteFile(filepath.Join(repo, "main.go"), []byte("package main\nvar x = 99\n"), 0644) + require.NoError(t, err, "test setup: writing dirty file") + + 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") + + edit, err := fr.FixFolder(context.Background(), types.FilePath(repo)) + require.Error(t, err, "FixFolder must return an error when the worktree has uncommitted changes") + 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") +} + +// TestFixFolder_CleanWorktree_Succeeds verifies that FixFolder proceeds normally +// when the passed worktree has no uncommitted changes. +func TestFixFolder_CleanWorktree_Succeeds(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "main.go", "package main\nvar x = 1\n") + // No uncommitted changes — worktree is clean. + + 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, "FixFolder must succeed on a clean worktree") + assert.NotNil(t, edit, "FixFolder must return an edit when the runner modifies files") +} + +// TestFixFolder_UntrackedFileOnly_DoesNotError verifies that a worktree containing +// only untracked files (e.g. build artifacts) is NOT rejected by the dirty-worktree +// guard. The guard's purpose is to prevent pre-existing modifications to TRACKED +// files from being silently included in the fix; untracked files are irrelevant to +// the fix output and must be ignored. +// +// Previously, the guard ran "git status --porcelain" which includes "?? " +// lines for untracked files, causing FixFolder to return an error for any worktree +// that contains untracked build artifacts. +func TestFixFolder_UntrackedFileOnly_DoesNotError(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "main.go", "package main\nvar x = 1\n") + + // Add an untracked file (not committed, not staged) — simulates a build artifact. + err := os.WriteFile(filepath.Join(repo, "artifact.bin"), []byte("binary"), 0644) + require.NoError(t, err, "test setup: writing untracked artifact file") + + runner := func(_ context.Context, _ workflow.Engine, root, _ string) error { + // Verify the runner can see the untracked file (it is in the worktree). + 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") + + // Must NOT return an error — an untracked-only worktree is considered clean. + edit, err := fr.FixFolder(context.Background(), types.FilePath(repo)) + require.NoError(t, err, "FixFolder must not error when the only dirty state is untracked files") + // Runner modified main.go, so a non-nil edit is expected. + assert.NotNil(t, edit, "FixFolder must return an edit when the runner modifies a tracked file") +} + +// TestFixFolder_TrackedFileModified_StillErrors verifies that the dirty-worktree +// guard still fires when a tracked file has uncommitted working-tree changes, +// even after switching to --untracked-files=no. +func TestFixFolder_TrackedFileModified_StillErrors(t *testing.T) { + repo := initGitRepo(t) + commitFile(t, repo, "main.go", "package main\nvar x = 1\n") + + // Modify the tracked file without staging or committing. + err := os.WriteFile(filepath.Join(repo, "main.go"), []byte("package main\nvar x = 99\n"), 0644) + require.NoError(t, err, "test setup: writing dirty tracked file") + + // Also add an untracked file to confirm it is ignored. + err = os.WriteFile(filepath.Join(repo, "artifact.bin"), []byte("binary"), 0644) + require.NoError(t, err, "test setup: writing untracked artifact file") + + 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(repo)) + require.Error(t, err, "FixFolder must return an error when a tracked file has uncommitted changes") + assert.Nil(t, edit, "FixFolder must return nil edit when returning an error") + assert.False(t, runnerCalled, "runner must NOT be called when the dirty-worktree guard fires") +} + // 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 diff --git a/infrastructure/oss/code_actions.go b/infrastructure/oss/code_actions.go index 71ac8b376..d9e3494aa 100644 --- a/infrastructure/oss/code_actions.go +++ b/infrastructure/oss/code_actions.go @@ -17,6 +17,7 @@ package oss import ( + "context" "fmt" "os" "regexp" @@ -248,7 +249,7 @@ func addQuickFixAction(engine workflow.Engine, configResolver types.ConfigResolv // corrupt the value. The latch makes the deferred action single-shot, which the // disk guard cannot guarantee. var applied bool - autofixEditCallback := func() *types.WorkspaceEdit { + autofixEditCallback := func(_ context.Context) *types.WorkspaceEdit { edit := &types.WorkspaceEdit{} if applied { logger.Warn().Str("file", filePathString).Msg("quickfix already applied; refusing to re-apply") diff --git a/infrastructure/oss/code_actions_test.go b/infrastructure/oss/code_actions_test.go index df5569393..d66c98bf1 100644 --- a/infrastructure/oss/code_actions_test.go +++ b/infrastructure/oss/code_actions_test.go @@ -17,6 +17,7 @@ package oss import ( + "context" "encoding/xml" "errors" "fmt" @@ -137,7 +138,7 @@ func Test_GetCodeActions_MavenPropertyInParentPom_RedirectsToParent(t *testing.T &scanResult{}, nil, depNode, getLearnMock(t), error_reporting.NewTestErrorReporter(engine), "", nil) quickFix := findUpgradeAction(t, snykIssue.CodeActions) - edit := (*quickFix.GetDeferredEdit())() + edit := (*quickFix.GetDeferredEdit())(context.Background()) // The edit must target the PARENT pom, not the child. require.Empty(t, edit.Changes[childPath], "child pom must not be edited") @@ -220,7 +221,7 @@ func Test_GetCodeActions_MavenParentPomPropertyVersion_RedirectsToParentProperty &scanResult{}, nil, depNode, getLearnMock(t), error_reporting.NewTestErrorReporter(engine), "", nil) quickFix := findUpgradeAction(t, snykIssue.CodeActions) - edit := (*quickFix.GetDeferredEdit())() + edit := (*quickFix.GetDeferredEdit())(context.Background()) // The edit must target the parent pom's entry, not its , // and the child pom must be untouched. @@ -323,7 +324,7 @@ func Test_addQuickFixAction_RefusesStaleEdit(t *testing.T) { changed := "\n 11.0.1\n\n" require.NoError(t, os.WriteFile(pomPath, []byte(changed), 0600)) - edit := (*action.GetDeferredEdit())() + edit := (*action.GetDeferredEdit())(context.Background()) assert.Empty(t, edit.Changes, "stale edit must not be applied") } @@ -371,7 +372,7 @@ func Test_addQuickFixAction_AppliesWhenContentMatches(t *testing.T) { []string{"root@1.0.0", "org:art@9.0.5"}, []any{"false", "org:art@11.0.1"}, nil) require.NotNil(t, action) - edit := (*action.GetDeferredEdit())() + edit := (*action.GetDeferredEdit())(context.Background()) edits := edit.Changes[pomPath] require.Len(t, edits, 1) assert.Equal(t, "11.0.1", edits[0].NewText) @@ -408,13 +409,13 @@ func Test_MavenPropertyQuickfix_AppliedTwice_DoesNotCorruptPom(t *testing.T) { // double-applied into 11.0.11. result := propertyManagedPom - edit := (*quickFix.GetDeferredEdit())() + edit := (*quickFix.GetDeferredEdit())(context.Background()) edits := edit.Changes[pomPath] require.Len(t, edits, 1, "first apply should produce the upgrade edit") result = applySingleLineEdit(t, result, edits[0]) require.NoError(t, os.WriteFile(pomPath, []byte(result), 0600)) - edit = (*quickFix.GetDeferredEdit())() + edit = (*quickFix.GetDeferredEdit())(context.Background()) assert.Empty(t, edit.Changes, "second apply must be refused, not re-applied") // The property value is upgraded exactly once. @@ -634,7 +635,7 @@ func applyMavenUpgradeQuickfix(t *testing.T, pom string) []types.TextEdit { snykIssue := toIssue(engine, defaultResolver(t, engine), types.FilePath(dir), types.FilePath(pomPath), issue, &scanResult{}, nil, depNode, getLearnMock(t), error_reporting.NewTestErrorReporter(engine), "", nil) quickFix := findUpgradeAction(t, snykIssue.CodeActions) - edit := (*quickFix.GetDeferredEdit())() + edit := (*quickFix.GetDeferredEdit())(context.Background()) return edit.Changes[pomPath] } diff --git a/internal/types/issues.go b/internal/types/issues.go index 073c60e50..c9692516f 100644 --- a/internal/types/issues.go +++ b/internal/types/issues.go @@ -17,6 +17,7 @@ package types import ( + "context" "encoding/json" "fmt" "net/url" @@ -75,7 +76,7 @@ type CodeAction interface { GetOriginalTitle() string GetIsPreferred() *bool GetEdit() *WorkspaceEdit - GetDeferredEdit() *func() *WorkspaceEdit + GetDeferredEdit() *func(context.Context) *WorkspaceEdit GetCommand() *CommandData GetDeferredCommand() *func() *CommandData GetUuid() *uuid.UUID