Skip to content

Conversation

RomneyDa
Copy link
Collaborator

@RomneyDa RomneyDa commented Sep 20, 2025

Description

Jaro Winkler was removed here #7240
Other fallbacks were removed here #7267
This PR reintroduces whitespace and trimmed fallbacks

Other key changes

  • unifies search and replace logic between extensions and CLI
  • Removes ability to create a file with multi edit and empty old_string because it's confusing and pretty much fully separate functionality

Summary by cubic

Restores trimmed and whitespace-ignored find/replace fallbacks and fixes match indexing to make edits more reliable. Adds structured ContinueError reasons surfaced in CLI/GUI and telemetry, and consolidates find/replace logic in core with extensive tests.

  • New Features

    • Restored trimmed and whitespace-ignored matching as fallbacks; corrected endIndex mapping for whitespace-ignored matches.
    • Added findSearchMatches and performReplace helpers for single and multi-edit flows.
    • Introduced ContinueError with reason codes; propagated through CLI/GUI and captured in PostHog (errorReason).
    • Added comprehensive vitest coverage for matching strategies and edge cases.
  • Refactors

    • Moved find/replace utils from GUI to core; updated imports across app.
    • Removed experimental CLI search-and-replace tool and its parsers.
    • Updated tools and file security checks to throw ContinueError (with specific reasons).
    • Disabled Jaro-Winkler fuzzy strategy pending fix (left TODO).

@RomneyDa RomneyDa requested a review from a team as a code owner September 20, 2025 00:51
@RomneyDa RomneyDa removed the request for review from a team September 20, 2025 00:51
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Sep 20, 2025
Copy link

AI Code Review

AI review failed due to service initialization issues. Please check the Continue API key and configuration.

No specific line comments generated.


💡 To request a new detailed review, comment @continue-detailed-review

@RomneyDa RomneyDa marked this pull request as draft September 20, 2025 00:54
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9 issues found across 26 files

Prompt for AI agents (all 9 issues)

Understand the root cause of the following 9 issues and fix them.


<file name="gui/src/pages/gui/ToolCallDiv/FindAndReplace.test.tsx">

<violation number="1" location="gui/src/pages/gui/ToolCallDiv/FindAndReplace.test.tsx:46">
Import targets core module while tests still mock a different (now missing) path, so performFindAndReplace won’t be mocked and vi.importActual will fail.</violation>
</file>

<file name="core/edit/searchAndReplace/findSearchMatch.vitest.ts">

<violation number="1" location="core/edit/searchAndReplace/findSearchMatch.vitest.ts:204">
Test title contradicts assertion and implementation; rename to reflect emptySearch result instead of null.</violation>
</file>

<file name="gui/src/util/clientTools/callClientTool.ts">

<violation number="1" location="gui/src/util/clientTools/callClientTool.ts:64">
Unknown error case drops the original message, reducing debuggability; include String(e) when creating the ContinueError.</violation>
</file>

<file name="extensions/cli/src/telemetry/telemetryService.ts">

<violation number="1" location="extensions/cli/src/telemetry/telemetryService.ts:525">
PostHog events may be dropped on process exit; telemetryService.shutdown doesn’t call posthogService.shutdown to flush pending events.</violation>

<violation number="2" location="extensions/cli/src/telemetry/telemetryService.ts:525">
PostHog capture is gated by isEnabled() (OTLP metrics). Without OTLP config, this call never executes; if PostHog should send regardless, move it outside this gate.</violation>

<violation number="3" location="extensions/cli/src/telemetry/telemetryService.ts:525">
Async telemetry call is not awaited or explicitly marked fire-and-forget; prefix with void to make intent clear and avoid accidental unhandled promise usage.</violation>
</file>

<file name="gui/src/redux/thunks/callToolById.ts">

<violation number="1" location="gui/src/redux/thunks/callToolById.ts:142">
Telemetry schema change: replacing errorMessage with errorReason may break existing analytics consumers. Consider maintaining backward compatibility (e.g., include both fields or deprecate gradually).</violation>
</file>

<file name="core/edit/searchAndReplace/performReplace.ts">

<violation number="1" location="core/edit/searchAndReplace/performReplace.ts:31">
Comment says edits are applied in reverse order, but code applies them sequentially; update the comment for accuracy or implement reverse ordering.</violation>
</file>

<file name="core/util/errors.ts">

<violation number="1" location="core/util/errors.ts:19">
Set the error name in the constructor so logs and handlers see &#39;ContinueError&#39; instead of the generic &#39;Error&#39;.</violation>
</file>

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

});
});

it("should return null for empty search content after whitespace removal", () => {
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test title contradicts assertion and implementation; rename to reflect emptySearch result instead of null.

Prompt for AI agents
Address the following comment on core/edit/searchAndReplace/findSearchMatch.vitest.ts at line 204:

<comment>Test title contradicts assertion and implementation; rename to reflect emptySearch result instead of null.</comment>

<file context>
@@ -0,0 +1,571 @@
+      });
+    });
+
+    it(&quot;should return null for empty search content after whitespace removal&quot;, () =&gt; {
+      const fileContent = &quot;some content&quot;;
+      const searchContent = &quot;   \t\n   &quot;;
</file context>
Fix with Cubic

? e
: e instanceof Error
? new ContinueError(ContinueErrorReason.Unspecified, e.message)
: new ContinueError(ContinueErrorReason.Unknown),
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unknown error case drops the original message, reducing debuggability; include String(e) when creating the ContinueError.

Prompt for AI agents
Address the following comment on gui/src/util/clientTools/callClientTool.ts at line 64:

<comment>Unknown error case drops the original message, reducing debuggability; include String(e) when creating the ContinueError.</comment>

<file context>
@@ -51,18 +52,16 @@ export async function callClientTool(
+          ? e
+          : e instanceof Error
+            ? new ContinueError(ContinueErrorReason.Unspecified, e.message)
+            : new ContinueError(ContinueErrorReason.Unknown),
       output: undefined,
     };
</file context>
Suggested change
: new ContinueError(ContinueErrorReason.Unknown),
: new ContinueError(ContinueErrorReason.Unknown, String(e)),
Fix with Cubic

// TODO: Implement OTLP logs export
logger.debug("Tool result event", attributes);

posthogService.capture("cli_tool_call_outcome", {
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog events may be dropped on process exit; telemetryService.shutdown doesn’t call posthogService.shutdown to flush pending events.

Prompt for AI agents
Address the following comment on extensions/cli/src/telemetry/telemetryService.ts at line 525:

<comment>PostHog events may be dropped on process exit; telemetryService.shutdown doesn’t call posthogService.shutdown to flush pending events.</comment>

<file context>
@@ -515,11 +518,17 @@ class TelemetryService {
     // TODO: Implement OTLP logs export
     logger.debug(&quot;Tool result event&quot;, attributes);
+
+    posthogService.capture(&quot;cli_tool_call_outcome&quot;, {
+      succeeded: options.success,
+      toolName: options.toolName,
</file context>
Fix with Cubic

// TODO: Implement OTLP logs export
logger.debug("Tool result event", attributes);

posthogService.capture("cli_tool_call_outcome", {
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostHog capture is gated by isEnabled() (OTLP metrics). Without OTLP config, this call never executes; if PostHog should send regardless, move it outside this gate.

Prompt for AI agents
Address the following comment on extensions/cli/src/telemetry/telemetryService.ts at line 525:

<comment>PostHog capture is gated by isEnabled() (OTLP metrics). Without OTLP config, this call never executes; if PostHog should send regardless, move it outside this gate.</comment>

<file context>
@@ -515,11 +518,17 @@ class TelemetryService {
     // TODO: Implement OTLP logs export
     logger.debug(&quot;Tool result event&quot;, attributes);
+
+    posthogService.capture(&quot;cli_tool_call_outcome&quot;, {
+      succeeded: options.success,
+      toolName: options.toolName,
</file context>

✅ Addressed in c6858ad

succeeded: !error,
toolName: toolCallState.toolCall.function.name,
errorMessage: errorMessage,
errorReason: error?.reason,
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Sep 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Telemetry schema change: replacing errorMessage with errorReason may break existing analytics consumers. Consider maintaining backward compatibility (e.g., include both fields or deprecate gradually).

Prompt for AI agents
Address the following comment on gui/src/redux/thunks/callToolById.ts at line 142:

<comment>Telemetry schema change: replacing errorMessage with errorReason may break existing analytics consumers. Consider maintaining backward compatibility (e.g., include both fields or deprecate gradually).</comment>

<file context>
@@ -131,14 +137,14 @@ export const callToolById = createAsyncThunk&lt;
+    succeeded: !error,
     toolName: toolCallState.toolCall.function.name,
-    errorMessage: errorMessage,
+    errorReason: error?.reason,
     duration_ms: duration_ms,
   });
</file context>
Fix with Cubic

@RomneyDa RomneyDa changed the title feat: restore some search and replace fallbacks feat: restore trimmed/whitespace search and replace fallbacks Sep 20, 2025
@RomneyDa RomneyDa closed this Sep 20, 2025
@github-project-automation github-project-automation bot moved this from Todo to Done in Issues and PRs Sep 20, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Sep 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant