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

Schema inconsistency, terminal-reuse regression, Windows casing behavior, and a race-prone test remain unresolved.

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

Review details

Suppressed comments (2)

Extension/tools/OptionsSchema.json:1034

• The cppvsdbg source schema permits only strings here, while package.json and the implementation accept null as the deletion value. Because package.json is regenerated from this file, regeneration will remove that support from the published schema. Please include null here as well.

          "additionalProperties": {
            "type": "string"
          },

Extension/test/scenarios/RunWithoutDebugging/tests/runWithoutDebugging.integration.test.ts:331

• This assertion can read the file after ofstream creates it but before envTest.cpp writes the inherited value, allowing the test to pass with an empty read even when null removal is broken. Wait for the matching debug session to terminate before reading the completed file, or have the helper program write an explicit sentinel when the variable is absent.

            assert.strictEqual(started, true, 'The noDebug launch with a null env value did not start successfully.');
            assert.strictEqual(await waitForResultFile(envResultFilePath, 10000), '', 'A null env value should remove the inherited variable.');

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

[Copilot]

🟡 Changes recommended

Null removal and case-insensitive overrides are incorrect in some launch paths, while the null test can falsely pass.

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

Review details

Suppressed comments (1)

Extension/src/Debugger/runWithoutDebuggingAdapter.ts:102

• A null value is removed from this object, but integrated terminals merge TerminalOptions.env onto VS Code's inherited/configured environment. Omitting the key therefore does not communicate a deletion, so an inherited variable can reappear when launchIntegratedTerminal receives this map. Preserve a null tombstone for the integrated-terminal options (while omitting it for child_process launches), or otherwise provide an exact terminal environment.

            if (value === null) {
                const keysToDelete = isWindows
                    ? Object.keys(env).filter(name => name.toLowerCase() === key.toLowerCase())
                    : [key];
                keysToDelete.forEach(name => delete env[name]);

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

[Sean McManus (sean-mcmanus)]

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

[Sean McManus (sean-mcmanus)]

Prashant Kumar Rai (@8prashant) I see some older unresolved threads -- can you resolve those if you already resolved them (I assume you did).

[Copilot]

🟢 Approval recommended

The schema and runtime changes are consistent and adequately covered by integration tests.

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),

Thanks again for the detailed review. I’ve pushed updates to address the remaining Copilot review feedback from #14691 (review).

Summary of the latest fixes:

• Updated Run Without Debugging terminal reuse so only terminals tracked by the adapter module are reused or disposed, addressing discussion_r3937293282.
• Preserved null tombstones for TerminalOptions.env in the integrated-terminal path while keeping deleted keys omitted for spawned process environments.
• Fixed Windows case-insensitive env replacement/deletion for both environment and env.
• Aligned OptionsSchema.json and generated package.json so env and environment values support null consistently.
• Updated localization text to document the null removal behavior.
• Tightened the Run Without Debugging tests so each scenario waits for session termination and cleans up on failure.

Could you please re-review when you have a chance?

[Sean McManus (sean-mcmanus)]

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

[Copilot]

🔵 Needs a closer look

Normal cppdbg launches still cannot correctly implement the newly documented null-as-deletion behavior.

Review details

Suppressed comments (1)

Extension/src/Debugger/configurationProvider.ts:729

• null is still forwarded to the normal cppdbg adapter as an environment entry, but that adapter's environment command path expects a string and does not interpret null as deletion. As a result, the newly documented “remove inherited variables” behavior only works for the inline no-debug adapter; a regular debug launch can set an empty value or fail instead. Please either implement deletion in the normal adapter and add a normal-debug test, or stop accepting/documenting nullable values for this path.

        for (const [name, value] of Object.entries(envObject)) {
            if (util.isString(value) || value === null) {
                mergedEnvironment.set(name, value);
            }

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

[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