Skip to content

Fix: Add env object syntax to launch.json schema for cppdbg and cppvsdbg - #14691

Open
Prashant Kumar Rai (8prashant) wants to merge 33 commits into
microsoft:mainfrom
8prashant:fix/12537-env-property-schema
Open

Prashant Kumar Rai (8prashant) wants to merge 33 commits into
microsoft:mainfrom
8prashant:fix/12537-env-property-schema

Conversation

@8prashant

Copy link
Copy Markdown
Contributor

Problem

#12537 : Using the shorthand env object syntax in launch.json produces a false validation warning:

Property env is not allowed.

"env": {
    "PATH": "/usr/local/bin"
}

This comment was marked as resolved.

@bobbrow

Copy link
Copy Markdown
Member

I don't think we should have two properties that do the same thing (also, this PR does not update the optionsSchema.json which means the next time package.json is generated, this will be deleted).

Adding Andrew Wang (@WardenGnaw) for comment.

@WardenGnaw

Copy link
Copy Markdown
Member

Thanks for the contribution. This change will work for cppvsdbg because it can also deserialize env and env has higher precedence than environment.

One issue with this change is that cppdbg does not currently support it. MIEngine only reads the environment array.

Aside from updating the OptionsSchema.json. There needs to be a change to MIEngine to support this or limit the new schema entry to cppvsdbg.

The other option is to add a cpptools configuration-provider conversion from env to environment but that adds an additional layer of transforms / errors that can happen when p;umbling the environment variables to the target process.

@8prashant

Copy link
Copy Markdown
Contributor Author

Thanks Andrew Wang (WardenGnaw) for the review. I’ve pushed updates to address this by fixing the no-debug path so the inline adapter now merges both environment and env with env taking precedence, adding cppdbg normalization in the configuration provider to convert env to environment so MIEngine receives the format it currently supports, updating OptionsSchema.json so the new env schema entries are preserved during regeneration, and adding Run Without Debugging integration coverage to verify env is applied and correctly overrides duplicate environment entries. I intentionally kept this fix in cpptools rather than making MIEngine changes in this PR because it resolves both normal launch and Run Without Debugging immediately, whereas a MIEngine-only change would not cover no-debug, and this approach avoids cross-repo dependency and release-coordination risk while keeping the change scoped and low risk; native MIEngine env support can still be added later as a separate follow-up enhancement.

@8prashant

Copy link
Copy Markdown
Contributor Author

Hi Sean McManus (@sean-mcmanus)
Can u please review the changes again!!

@sean-mcmanus
Sean McManus (sean-mcmanus) requested a balanced review from Copilot August 31, 2026 15:46
@sean-mcmanus

Copy link
Copy Markdown
Contributor

Prashant Kumar Rai (@8prashant) Yeah, sorry, I've been busy with other stuff and forgot about these PRs.

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

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.

Reviewed the current head; see the inline comments.

Comment thread Extension/src/Debugger/configurationProvider.ts Outdated
Comment thread Extension/package.nls.json Outdated
Comment thread Extension/test/scenarios/RunWithoutDebugging/assets/envTest.cpp Outdated

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Copilot AI left a comment

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.

🔵 Needs a closer look

Terminals without shell integration can be reused while their previous command is still running.

Review details

Suppressed comments (1)

Extension/src/Debugger/runWithoutDebuggingAdapter.ts:170

  • In the no-shell-integration fallback, sendText only queues the command, so removing the terminal from activeTerminals immediately makes it appear reusable while the launched process may still be running. A subsequent launch can then interleave another command in that shell, or dispose the terminal at line 132 when its environment differs, terminating the first process. Since completion cannot be observed on this path, stop managing this terminal for reuse so the next launch creates a separate terminal.
            activeTerminals.delete(this.terminal);
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

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.

🔵 Needs a closer look

The new macOS external-terminal environment handling lacks automated coverage.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

Extension/src/Debugger/runWithoutDebuggingAdapter.ts:209

  • The macOS external-terminal environment path has no automated coverage: the new env integration test always selects an integrated/internal console, and .github/workflows/job-compile-and-test.yml:143 runs this scenario with skipExternalConsole. Please add coverage for this /usr/bin/env command construction, especially values requiring shell/AppleScript escaping, so regressions in external launches are caught.
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

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.

🔵 Needs a closer look

Cross-platform terminal lifecycle and environment propagation behavior warrants final human validation.

Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@8prashant

Copy link
Copy Markdown
Contributor Author

Hi Sean McManus (@sean-mcmanus) , Could you please re-review when you have a chance?

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.

Re-reviewed the current head; see the inline comment.

Comment thread Extension/src/Debugger/runWithoutDebuggingAdapter.ts

Copilot AI left a comment

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.

🟡 Changes recommended

The parsed-environment interface now permits null values that normal debug adapters and the public schemas do not support.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread Extension/src/Debugger/ParsedEnvironmentFile.ts Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

🔵 Needs a closer look

Cross-platform terminal lifecycle and environment propagation changes warrant final human validation.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

…ing for environment variables in remote and command-driven cppdbg targets

Copilot AI left a comment

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.

🔵 Needs a closer look

An in-flight terminal can become reusable after manual session disposal, allowing a subsequent command to target a busy terminal.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

Extension/src/Debugger/runWithoutDebuggingAdapter.ts:409

  • If the user stops the no-debug session while its shell command is still running, dispose() removes the terminal from activeTerminals even though terminateProcess() cannot stop a terminal execution. The terminal remains in managedTerminals, so the next launch with the same program/environment reuses that busy terminal and may send the new command to the running process instead of its shell. Keep it marked active until the shell execution ends, or remove it from the managed/reusable map when disposing an in-flight session.
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI left a comment

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.

🔵 Needs a closer look

Cross-platform terminal lifecycle and environment behavior warrants final human validation.

Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

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

Labels

None yet

Projects

Status: Pull Request

Development

Successfully merging this pull request may close these issues.

7 participants