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

[Prashant Kumar Rai (8prashant)]

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"
}

[Bob Brown (bobbrow)]

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.

[Andrew Wang (WardenGnaw)]

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.

[Prashant Kumar Rai (8prashant)]

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.

[Prashant Kumar Rai (8prashant)]

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

[Sean McManus (sean-mcmanus)]

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

[Copilot]

Pull request overview

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

[Sean McManus (sean-mcmanus)]

Reviewed the current head; see the inline comments.

[Copilot]

Pull request overview

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

[Copilot]

🟡 Changes recommended

Environment-driven terminal replacement can terminate an overlapping no-debug session.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

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

[Copilot]

🟡 Changes recommended

Nullable adapter-facing values and unsafe reuse of unmonitored active terminals remain unresolved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

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

Extension/src/Debugger/ParsedEnvironmentFile.ts:14

• Environment is the adapter-facing type used by ParsedEnvironmentFile, and nullable values are not supported by either launch schema; resolveEnvObject also deliberately retains only strings. Widening this exported type lets an initialEnv null pass unchanged through CreateFromContent and resolveEnvFile into a normal debug adapter, reintroducing the unsupported-null path. Keep this type string-only; the inline no-debug adapter already has its separate nullable terminal-environment type.

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

[Copilot]

🔵 Needs a closer look

Windows case-insensitive environment names are not merged reliably, potentially defeating env precedence.

Review details

Suppressed comments (1)

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

Extension/src/Debugger/configurationProvider.ts:732

• On Windows, environment-variable names are case-insensitive, but this merge is case-sensitive. For example, environment: [{ "name": "PATH", ... }] plus env: { "Path": "..." } produces two entries instead of applying the documented env precedence, leaving the adapter/OS to resolve conflicting values. Normalize lookup keys on Windows while retaining the winning entry's spelling.

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

[Copilot]

🟡 Changes recommended

Terminal lifecycle, macOS external-terminal propagation, and remote-target case sensitivity have unresolved correctness issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

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

Extension/src/Debugger/runWithoutDebuggingAdapter.ts:106

• The computed environment does not reach a no-debug target launched in a macOS external terminal. The downstream macOS branch passes env only to osascript, while Terminal do script executes the command in Terminal.app's shell, which does not inherit the osascript process environment. Consequently both the newly supported env values and legacy environment values are ignored for externalTerminal on macOS. Please transport the overrides into the executed command safely, or fall back to the regular adapter for this configuration.
  Extension/src/Debugger/runWithoutDebuggingAdapter.ts:143
• activeTerminals is populated for every integrated terminal, but it is cleared on termination only when releaseTerminalOnTerminate is true. That flag is set only when shell integration exists; the fallback at lines 165-173 emits terminated without releasing the marker. On shells without integration, every later run of the same program therefore treats the previous idle terminal as permanently active and creates another terminal, so repeated runs accumulate terminals instead of reusing one. Please define a fallback lifecycle that releases or removes this managed terminal once the session ends.

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

[Copilot]

🔵 Needs a closer look

Terminal reuse regresses without shell integration, and macOS external terminals do not receive the configured environment.

Review details

Suppressed comments (2)

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

Extension/src/Debugger/runWithoutDebuggingAdapter.ts:106

• For cppdbg Run Without Debugging with externalConsole on macOS, these merged values still do not reach the target. launchExternalTerminal passes env only to osascript; the shell that runs the command is created by Terminal.app via AppleScript and does not inherit the osascript process environment. Include the environment changes in the command sent to Terminal.app (with shell-safe escaping), or launch the external terminal through an API that propagates the environment.
  Extension/src/Debugger/runWithoutDebuggingAdapter.ts:151
• activeTerminals is populated for every integrated launch, but releaseTerminalOnTerminate is enabled only when shell integration exists. In the fallback sendText branch, sendEvent('terminated') therefore never removes this terminal from the active set, so every later run of the same program creates another terminal instead of reusing the existing one. Mark the terminal releasable for the fallback path as well (or explicitly remove it before sending terminated).

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

[Copilot]

🔵 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]

🔵 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]

🔵 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

[Prashant Kumar Rai (8prashant)]

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