Skip to content

feat(vscode) - Adding additional csharp lsp server functionality#8828

Open
lambrianmsft wants to merge 47 commits intoAzure:ccastrotrejo/csharpLSPServerfrom
lambrianmsft:ccastrotrejo/csharpLSPServer
Open

feat(vscode) - Adding additional csharp lsp server functionality#8828
lambrianmsft wants to merge 47 commits intoAzure:ccastrotrejo/csharpLSPServerfrom
lambrianmsft:ccastrotrejo/csharpLSPServer

Conversation

@lambrianmsft
Copy link
Copy Markdown
Contributor

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

Adding ability to open overview page for a workflow, adding a new workflow, reducing the connection pane load time, and updating the package.

Impact of Change

  • Users:
    Workflows runs can now be opened and new workflows can be added
  • Developers:
  • System:

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in:

Contributors

Screenshots/Videos

… call times for checking child process, added options to add new codeful workflows to the project, optimzed the get-child-process.ps1 script, and finally added the overview page for codeful workflows and ability to view runs
…d to be passed as a path and added additional telemetry to capture this whenever it happens
…ger name isn't defined, we pull the default from the lsp server sdk. Disabled the create unit test from run for codeful workflows
…perience, prevent callback url from getting continuously pinged, updated sdk version
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Feb 19, 2026

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

⚠️ PR Title

  • Current: feat(vscode) - Adding additional csharp lsp server functionality
  • Issue: The title is OK but can be improved for clarity, conventional-commit style, and scope. It uses lower-case "csharp" and a dash instead of the conventional colon style. It is somewhat vague about what "additional" functionality is — the diff shows many concrete changes (C# LSP SDK updates, codeful workflow support, Program.cs / workflow creation, many tests, packaging changes, runtime/process handling, auth caching, PowerShell script updates and more).
  • Recommendation: Make the title concise, imperative, and specific. Example suggestions:
    • feat(vscode): add C# LSP server support and codeful workflow tooling
    • feat(vscode): C# LSP + codeful workflow support (Program.cs generation, LSP SDK, debug/runtime plumbing)
    • Use C# capitalization and conventional commit format feat(scope): description.

Commit Type

  • Properly selected (feature).
  • Note: Only one commit type is selected in the body which is correct. No change required.

Risk Level

  • Assessment: The PR body marks "Medium", but the repository labels do not include any risk label (e.g. risk:low, risk:medium, risk:high). The code diff is large (47 commits in this PR snapshot, ~4662 additions, ~626 deletions across ~84 files) and touches many runtime and security-sensitive areas (LSP server, packaging of SDK nupkg, process management, design-time/runtime startup, auth token caching, PowerShell scripts, and many user-visible behaviors). For those reasons I advise a risk level of High.
  • Recommendation:
    • Either update the PR body to mark High (if you agree), or add a GitHub label that matches the risk declared in the body.
    • Add a repository label to the PR such as risk:high (preferred given the scope) or risk:medium if you will provide additional justification and mitigation steps. The PR must include a risk label that matches the body.
    • If you keep Medium, explain in the body why this large, multi-area change is Medium (mitigations, opt-in flags, feature gates, automated test coverage & CI passing, staged rollout plan).

⚠️ What & Why

  • Current: "Adding ability to open overview page for a workflow, adding a new workflow, reducing the connection pane load time, and updating the package."
  • Issue: This is a short summary and doesn't call out the full surface area. The diff includes many concrete changes that reviewers and maintainers need to know about (C# LSP server binary and SDK packaging changes, codeful workflow creation & Program.cs manipulation, many new tests, changes to how design-time/runtime processes are discovered and validated, caching for Azure connector details, changes to extension build script, and new/updated PowerShell scripts). The body should explicitly list the main changes and any required upgrade steps.
  • Recommendation: Expand to a brief list of the main functional changes and the rationale. Example copy you can paste and adapt:
    • "What: Adds C# LSP server and codeful workflow support: Program.cs and workflow file generation, Program.cs insertion for multiple workflows, new LSP SDK package in assets, improved completion filtering, debug configuration updates for codeful projects, create/publish codeful project support in deploy flow, and many unit/E2E tests. Also includes performance and reliability improvements: improved get-child-processes PowerShell script, caching of Azure connector details, telemetry/debugging info, and build script changes (npm install in dist).
    • Why: Enable first-class codeful (C#) workflow authoring, improve developer experience when debugging codeful projects, reduce connection pane load latency, and ship LSP-driven completions and trigger extraction for codeful workflows."

Impact of Change

  • Issue: The Impact section in the PR body is incomplete (Users / Developers / System bullets are empty or minimal). Given the breadth of changes, this section must be filled.
  • Recommendation (fill these into the PR body):
    • Users: Describe user-facing changes. Example: "Users can now create and open codeful (C#) workflows from the extension, open a Workflow Overview from C# files, run-and-debug codeful workflows locally, and run unit/E2E tests for codeful flows. There may be new local.settings keys (WORKFLOW_CODEFUL_ENABLED) and different extension bundle URIs for codeful projects."
    • Developers: "API/behavior changes for extension code: Program.cs is created/modified for workflows, new utility functions for codeful workflows, new LSP request custom/getWorkflowMetadata used by extension, and public helper functions added (resolveConnectionEdit, resolveCurrentConnectionId, etc.). Build scripts extract/copy LSP server and SDK; package now includes a .nupkg. Reviewers should run the unit and integration tests and ensure CI passes."
    • System: "Changes impact extension packaging (LSP zip and nupkg added), runtime/process validation logic (child func PID resolution), and PowerShell scripts for process enumeration. Might affect supported build/release steps and CI times. There are changes to extension bundle sources and versions — validate that the target feed and bundle versions are correct for release."

Test Plan

  • Assessment: The PR body marks E2E tests and Manual testing, but the code diff contains many new/updated unit tests and test files (multiple src/__test__ additions and many new test suites). The PR body did NOT mark "Unit tests added/updated" — that's a mismatch and must be corrected.
  • Recommendation:
    • Update the Test Plan checkboxes to include: Unit tests added/updated (checked) because the diff includes many unit tests, and E2E tests added/updated if that is indeed present — list the E2E test files if so. If there are no E2E tests, uncheck the E2E box and explain why.
    • Provide a short runnable test checklist in the PR (commands to run unit tests, integration/E2E, and any manual steps). Example: "Run pnpm -w test or pnpm --filter apps/vs-code-designer test and ensure all tests pass. Manually verify: create codeful workspace, add a workflow, open overview from .cs, run debugger attach flow for codeful workflow, open connection view and insert connection into a .cs parameter, and validate runtime start/stop flows."
    • If manual testing was done, list the exact scenarios, environments (Windows/macOS/Linux), and telemetry checks.

⚠️ Contributors

  • Assessment: Contributors section is blank.
  • Recommendation: Add the names/handles of PMs, designers, and reviewers who helped shape the change. At minimum, add a short note: "Contributors: @lambrianmsft, @ccastrotrejo, ...". This is optional but recommended so people get credit.

⚠️ Screenshots/Videos

  • Assessment: Blank (OK).
  • Recommendation: If there are visible UI changes (connection pane, overview UI changes, loading states), consider adding screenshots or a short video to help reviewers. If not applicable, leave blank.

Summary Table

Section Status Recommendation
Title ⚠️ Make it concise and specific; use conventional commit style (e.g. feat(vscode): add C# LSP & codeful workflow support)
Commit Type commit type feature selected correctly
Risk Level Add matching risk label to the PR. I advise High due to large, cross-cutting changes — update the PR body or labels accordingly.
What & Why ⚠️ Expand the short summary into a few bullets listing major areas changed and why.
Impact of Change Complete Users/Developers/System bullets and mention upgrade/migration steps.
Test Plan Check Unit tests added/updated (many unit tests in diff). Provide runnable test checklist and ensure CI passes.
Contributors ⚠️ Add contributor list (optional but recommended).
Screenshots/Videos ⚠️ Add if UI/UX changed; otherwise optional.

Final notes and actions to take

  • Advised risk: High (higher than the PR body Medium). Reason: This PR modifies many core paths (LSP server and packaged SDK, process detection and PowerShell script, build/packaging behavior, runtime start/validation, connection management and auth caching), adds binaries/nupkg assets, and makes behavior changes that will affect users and CI. Please either:

    • Update the PR risk to High in the body and add a risk:high label to the PR, OR
    • If you believe the changes are not high risk, provide clear mitigation details in the PR body (feature gates, fallback-safe behavior, thorough E2E coverage, rollout plan) and still add a matching risk label (e.g. risk:medium) with justification.
  • Tests: The diff contains many new test files. Update the Test Plan to mark Unit tests added/updated checked and list the test commands to run locally and in CI. Ensure all tests pass in CI and mention this in the PR.

  • Body details to add or expand (copy-paste friendly suggestions):

    • What & Why (short):
      • "What: Add C# codeful workflow support and LSP improvements. Key changes: Program.cs generation and patching to add workflows, new Codeful workflow templates, LSP server and SDK packaging (added nupkg + LSP zip), improved completion filtering, new helper APIs (resolveConnectionEdit, resolveCurrentConnectionId), improved child process detection (PowerShell script + findChildProcess), caching of Azure connector details, telemetry/diagnostics, and many unit/integration tests."
      • "Why: Provide first-class C# (codeful) workflow authoring and debugging in VS Code and reduce UX friction (faster connection pane, more reliable process detection)."
    • Impact of Change: exact bullets for Users/Developers/System as shown earlier.
    • Test Plan: list commands and which suites changed (files). Example:
      • Unit tests: pnpm -w test or pnpm --filter apps/vs-code-designer test (list changed test files under apps/vs-code-designer/src/**/test)
      • Manual: steps to create codeful workspace, add workflow, verify Program.cs updates, run debug attach flow, run overview/run history flows, test connection insertion on .cs file, verify auth caching behavior.
  • Labels: Add a repository risk label to the PR (e.g. risk:high) and ensure it matches the PR body.

  • Title: Update to conventional commit style and make it specific. Example: feat(vscode): C# LSP & codeful workflow support (Program.cs generation, LSP SDK, debug/runtime improvements)

Please update the PR title, add the missing risk label (or change the risk in the body), update the Test Plan to reflect unit tests added, and expand the What/Why and Impact sections with the concrete items above. Once the PR body is updated to include the recommended details and labels (and CI/tests are green), re-request review and we can re-evaluate. Thank you for the thorough work — this is a big feature and the more context you provide in the PR body the faster the reviewers can approve and the safer the roll-out will be.


Last updated: Thu, 23 Apr 2026 21:25:07 GMT

} else {
// For codeless projects, build custom code functions if they exist
const customFolderExists = await fse.pathExists(path.join(logicAppNode.fsPath, libDirectory, customDirectory));
if (customFolderExists) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be if !customFolderExists? Since we would build in the case where lib/custom/* doesn't already exist for codeless, i.e. not already built

// For codeless projects, build custom code functions if they exist
const customFolderExists = await fse.pathExists(path.join(logicAppNode.fsPath, libDirectory, customDirectory));
if (customFolderExists) {
await buildCustomCodeFunctionsProject(actionContext, logicAppNode);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

buildCustomCodeFunctionsProject is supposed to check whether the .net project is specifically a custom code functions project before building, part of that is to check for Microsoft.Azure.Workflows.Webjobs.Sdk in the .csproj, which I don't think will exist for codeful projects. Should there be a separate function for handling building codeful?


// Find the location to insert the new workflow builder
// Look for the "Build all workflows" comment or insert before host.Run()
const buildCallRegex = /(\s*)(\/\/ Build all workflows\s*\n(?:\s*\/\/.*\n)*\s*)(.*?)(\s*host\.Run\(\);)/s;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible to avoid using a regex here to update Program.cs with a new workflow, and instead just overwrite Program.cs with one containing all workflows? We would need to track the workflows in a codeful project elsewhere, but we should probably avoid using regex as much as possible since it's bound to break in some cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants