Skip to content

[vscode-shared] Add outputMarker option to runWorkspaceCommandAsync#5590

Closed
Copilot wants to merge 4 commits intomainfrom
copilot/refactor-string-marking-logic
Closed

[vscode-shared] Add outputMarker option to runWorkspaceCommandAsync#5590
Copilot wants to merge 4 commits intomainfrom
copilot/refactor-string-marking-logic

Conversation

Copy link
Contributor

Copilot AI commented Feb 3, 2026

  • Explore and understand the codebase structure
  • Add outputMarker option to runWorkspaceCommandAsync function
  • Update debug-certificate-manager-vscode-extension to use the new option
  • Update playwright-local-browser-server-vscode-extension to use the new option
  • Build and verify changes compile successfully
  • Address feedback: Remove node -p extraction from function - callers provide full command
  • Run CodeQL security checker

Summary

This PR refactors the duplicate string marker logic from two VS Code extensions into runWorkspaceCommandAsync.

Changes:

  • Added IOutputMarkerOptions interface with prefix and suffix properties
  • When outputMarker is provided, the function extracts only the content between the markers from the output
  • The function is generic and does not construct any commands - callers provide the full command line
  • Updated both extensions to use the new option

Security Summary

No security vulnerabilities detected by CodeQL.

Original prompt

This section details on the original issue you should resolve

<issue_title>[vscode-shared] Refactor string marking logic into runWorkspaceCommandAsync</issue_title>
<issue_description>runWorkspaceCommandAsync used in two places:

const markerPrefix: string = '<<<HOMEDIR_START>>>';
const markerSuffix: string = '<<<HOMEDIR_END>>>';
const output: string = await runWorkspaceCommandAsync({
terminalOptions: { name: 'debug-certificate-manager', hideFromUser: true },
// Wrapping the desired node output in markers to trim uninteresting shell output.
commandLine: `node -p "'${markerPrefix}' + require('os').homedir() + '${markerSuffix}'"`,
terminal
});
terminal.writeLine(`Running command to resolve home directory: ${output}`);
const startIndex: number = output.lastIndexOf(markerPrefix);
const endIndex: number = output.lastIndexOf(markerSuffix);
if (startIndex !== -1 && endIndex !== -1 && endIndex > startIndex) {
homeDir = output.substring(startIndex + markerPrefix.length, endIndex).trim();
} else {
throw new Error('Failed to parse home directory from command output');
}

and

const markerPrefix: string = '<<<TEMPDIR_START>>>';
const markerSuffix: string = '<<<TEMPDIR_END>>>';
const output: string = await runWorkspaceCommandAsync({
terminalOptions: { name: 'playwright-local-browser-server', hideFromUser: true },
commandLine: `node -p "'${markerPrefix}' + require('node:os').tmpdir() + '${markerSuffix}'"`,
terminal
});
const startIndex: number = output.indexOf(markerPrefix);
const endIndex: number = output.indexOf(markerSuffix);
if (startIndex !== -1 && endIndex !== -1 && endIndex > startIndex) {
tempDir = output.substring(startIndex + markerPrefix.length, endIndex).trim();
} else {
throw new Error('Failed to parse temp directory from command output');
}

with duplicate string marker logic wrapped around the usage of these functions.

We should abstract this as a new option in runWorkspaceCommandAsync so we don't have this extra logic. </issue_description>

Comments on the Issue (you are @copilot in this section)


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits February 3, 2026 05:17
Co-authored-by: TheLarkInn <3408176+TheLarkInn@users.noreply.github.com>
…arch

Co-authored-by: TheLarkInn <3408176+TheLarkInn@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor string marking logic into runWorkspaceCommandAsync [vscode-shared] Add outputMarker option to runWorkspaceCommandAsync Feb 3, 2026
Copilot AI requested a review from TheLarkInn February 3, 2026 05:22
Co-authored-by: TheLarkInn <3408176+TheLarkInn@users.noreply.github.com>
@TheLarkInn TheLarkInn marked this pull request as ready for review February 3, 2026 15:14
@TheLarkInn TheLarkInn requested a review from bmiddha February 3, 2026 15:15
Comment on lines +94 to +95
const startIndex: number = outputStream.indexOf(outputMarker.prefix);
const endIndex: number = outputStream.indexOf(outputMarker.suffix);
Copy link
Contributor

Choose a reason for hiding this comment

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

Per prior conversation regarding the debug certificate manager, to deal with some shells that echo the command to be run, we should be using lastIndexOf instead of indexOf, and also should anchor one from the other.

Comment on lines +9 to +12
* Options for extracting a specific value from the command output using markers.
* When provided, the function will search for the markers in the command output and
* return only the content between them. This is useful for extracting specific
* values from shell output that may contain additional noise.
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to explicitly define the behavior when the markers appear multiple times.

@TheLarkInn
Copy link
Member

After talking with Bharat we're going to just leave this refactor as is. It doesn't really help much, however we can still refactor to use lastIndexOf() in a separate PR.

@TheLarkInn TheLarkInn closed this Feb 4, 2026
@github-project-automation github-project-automation bot moved this from Needs triage to Closed in Bug Triage Feb 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

[vscode-shared] Refactor string marking logic into runWorkspaceCommandAsync

3 participants