Added composite command support and test coverage#639
Added composite command support and test coverage#639msivasubramaniaan wants to merge 43 commits intoche-incubator:mainfrom
Conversation
msivasubramaniaan
requested review from
RomanNikitenko,
azatsarynnyy and
vitaliy-guliy
as code owners
|
Click here to review and test in web IDE: |
&message=Dev%20cluster%20(for%20maintainers)&logo=eclipseche&color=525C86&labelColor=FDB940)
| "peerDependencies": { | ||
| "jsep": "^0.4.0||^1.0.0" | ||
| "node_modules/@emnapi/core": { | ||
| "version": "1.8.1", |
There was a problem hiding this comment.
FYI
The license check is failing
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
|
|
||
| // Single-component → join commands | ||
| const joiner = parallel ? " & " : " && "; | ||
| let compositeCmd = execs.map((e) => e.commandLine).join(joiner); |
There was a problem hiding this comment.
@RomanNikitenko , is string manipulation the only way to do this ? Using compound tasks on the vscode side still isn't possible ?
There was a problem hiding this comment.
I investigated that area more than 2 years ago, so I asked @msivasubramaniaan to double check that option when he started to work on the issue.
@msivasubramaniaan could you provide result of your investigation?
There was a problem hiding this comment.
I double-checked this again.
At the moment, yes — string manipulation is still the only practical way to achieve this.
VS Code does not support dynamically creating or chaining compound tasks programmatically from an extension. Compound tasks must be statically defined in tasks.json, and there’s no API to generate or invoke a compound task on the fly based on resolved components or runtime data.
Because of that limitation, joining commands using && / & (depending on parallel vs sequential execution) remains the only viable solution when the task needs to be assembled dynamically.
So the approach of building a composite command string is the only way
RomanNikitenko
left a comment
RomanNikitenko
left a comment
There was a problem hiding this comment.
@msivasubramaniaan
I tried to test the functionality using your samples
I run component-parallel-demo command:
My understanding - it should run signal-frontend in the frontend container and signal-backend in the backend container.
Actual behaviour:
- there is no output for both commands, for both containers
- there is just message "Composite multi-component execution completed" which comes from the
taskProviderhttps://github.com/che-incubator/che-code/pull/639/changes#diff-e505171c2bbd2db1494bbb49e7c7ccaad44644b923a12f6c5ca2ebdedd0fdbcdR389
Moreover, the dependent commands just hanging without any output when I run them separately:
Could you clarify:
- what is expected behaviour there?
- is it possible to detect which component is used for your test commands?
- If commands are in the running state - why composite command reports that
Composite multi-component execution completed?
thanks in advance!
|
@msivasubramaniaan 
|
|
@msivasubramaniaan 
|
|
@msivasubramaniaan |
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
@RomanNikitenko Also the inconsistency behavior issue also fixed and removed the unnecessary logs as well |
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
|
|
||
| vscode.tasks.executeTask(childTask); | ||
| }); | ||
| closeEmitter.fire(0); |
There was a problem hiding this comment.
The most critical use case is support for Parallel Composite tasks.
In my view, the current implementation would be the best one:
- it is aligned with the VS Code behavior for compound tasks
- dependent tasks run in their own terminals
- there is no mixed output in a single terminal (as in the previous implementation)
- there are no extra terminals: number of terminals = number of dependent tasks (vs the previous approach, where it was number of dependent tasks + 1 parent terminal)
Unfortunately, the current approach has one significant drawback: if I understand correctly - actually a parent (extra) terminal is still created and then immediately closed. It looks like a bug on UI.
Screen.Recording.2026-02-27.at.11.29.26.mov
|
@msivasubramaniaan
Please see on the video 2 behaviors:
build_and_run_bug.mov |
|
@msivasubramaniaan
Update: it does not matter first command fails or the second one - command item has incorrect status... 
VS Code behavior, for example: 
|
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
1 similar comment
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
|
Hello @RomanNikitenko |
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
| } | ||
|
|
||
| private sanitize(cmd: string) { | ||
| return cmd.replace(/(?:\s*(?:&&|\|\||[|;&]))+\s*$/, "").trim(); |
There was a problem hiding this comment.
@msivasubramaniaan
I guess this logic changes behavior from background/non-blocking to foreground/blocking, which is a semantic regression.
Like: cmd & becomes cmd
Could you double check it using logs or another way?
I prepared simple commands that show a background command blocks next command
BG_command.mov
There was a problem hiding this comment.
@msivasubramaniaan
For example, we use background command to run machine-exec in the che-code: https://github.com/che-incubator/che-code/blob/main/build/scripts/entrypoint-volume.sh#L74
There was a problem hiding this comment.
I used the following commands for testing:
- id: bg-command
exec:
label: "Background Command"
component: ubi9-tools
commandLine: "sh -c 'echo Start Background Task; sleep 5; echo Complete Background Task' &"
- id: after-bg-command
exec:
label: "NEXT Task after BG Command"
component: ubi9-tools
commandLine: "sh -c 'sleep 1; echo NEXT Task after BG TASK'"
- id: bg-in-composite
composite:
label: "Background operator stripped"
commands:
- bg-command
- after-bg-command
parallel: false
|
@msivasubramaniaan On the video you can see that:
Working_Dir_bug.mp4 |
📝 WalkthroughWalkthroughAdds Jest testing infra and tests for the che-commands extension, implements a CompositeTaskBuilder for devfile composite commands, updates task provider to use it, adjusts TypeScript configs and package.json for testing, and runs extension tests during Docker image builds. Changes
Sequence DiagramsequenceDiagram
participant Client as Client / Extension
participant TaskProvider as DevfileTaskProvider
participant CompBuilder as CompositeTaskBuilder
participant Terminal as Pseudo-Terminal
participant Component as Workspace Component
Client->>TaskProvider: provideTasks(devfile)
activate TaskProvider
TaskProvider->>CompBuilder: build(composite, allCommands)
activate CompBuilder
CompBuilder->>CompBuilder: validate & flatten composite graph
alt single-component composite
CompBuilder->>Terminal: create PTY that runs concatenated commands
activate Terminal
Terminal->>Component: execute command(s)
Component-->>Terminal: output / exit code
deactivate Terminal
else multi-component composite
CompBuilder->>Terminal: create PTY that starts per-step exec tasks
activate Terminal
Terminal->>Component: for each step: execute via machine exec or tasks.executeTask
Component-->>Terminal: outputs
deactivate Terminal
end
CompBuilder-->>TaskProvider: return vscode.Task
deactivate CompBuilder
TaskProvider-->>Client: Task[]
deactivate TaskProvider
Client->>Terminal: execute task
activate Terminal
Terminal->>Component: run underlying commands
Component-->>Terminal: output / exit
Terminal-->>Client: task complete
deactivate Terminal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable poems in the walkthrough.Disable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
code/extensions/che-commands/src/compositeTaskBuilder.ts (1)
328-330:⚠️ Potential issue | 🟠 MajorBackground operator
&at end of command is stripped, changing semantics.The
sanitize()regex removes trailing&which converts background commands to foreground. For example,cmd &becomescmd. This was flagged in past reviews - a command intended to run in background will now block.🐛 Proposed fix: only strip joiners when combining commands
- private sanitize(cmd: string) { - return cmd.replace(/(?:\s*(?:&&|\|\||[|;&]))+\s*$/, "").trim(); + private sanitize(cmd: string) { + // Only strip trailing sequential joiners (&&, ||, ;) - preserve background & + return cmd.replace(/(?:\s*(?:&&|\|\||;))+\s*$/, "").trim(); }Note: This preserves standalone
&for backgrounding while still removing trailing&&,||, and;that are meant as joiners.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/extensions/che-commands/src/compositeTaskBuilder.ts` around lines 328 - 330, sanitize currently strips a trailing single ampersand (background operator) because the regex includes '&' in the character class; update the sanitize function so it only strips command joiners (&&, ||, |, ;) but preserves a standalone '&' at the end; specifically, replace the current regex in sanitize with one that does not include '&' in the single-character class (e.g., remove '&' from [|;&] and keep matching multi-char operators like && and ||) so sanitize(cmd: string) returns the trimmed string but leaves a trailing single '&' intact.
🧹 Nitpick comments (6)
code/extensions/che-commands/package.json (1)
82-82: Missing trailing newline at end of file.Most editors and linters expect a trailing newline. Consider adding one for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/extensions/che-commands/package.json` at line 82, The file code/extensions/che-commands/package.json is missing a trailing newline after the final closing brace; open that package.json and add a single newline character at the end of file so the file ends with "\n" (ensure the final "}" remains intact), save the file and include the change in your commit so linters/editors recognize the EOF newline.code/extensions/che-commands/src/compositeTaskBuilder.ts (1)
187-213: Consider extracting duplicate cancellation logic.The
close()andhandleInput('\x03')handlers share nearly identical cleanup logic. Extract to a helper for maintainability.♻️ Suggested refactor
+ const cancelExecution = () => { + isCancelled = true; + for (const p of activePtys) { + try { + p.handleInput?.("\x03"); + p.close?.(); + } catch {} + } + activePtys.clear(); + closeEmitter.fire(130); + }; + const aggregator: vscode.Pseudoterminal = { // ... close: () => { - isCancelled = true; - for (const p of activePtys) { - try { - p.handleInput?.("\x03"); - p.close?.(); - } catch {} - } - activePtys.clear(); - closeEmitter.fire(130); + cancelExecution(); }, handleInput: (data: string) => { if (data === "\x03") { - isCancelled = true; - for (const p of activePtys) { - p.handleInput?.("\x03"); - p.close?.(); - } - activePtys.clear(); - closeEmitter.fire(130); + cancelExecution(); } }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/extensions/che-commands/src/compositeTaskBuilder.ts` around lines 187 - 213, The cancellation/cleanup logic duplicated in the compositeTaskBuilder object's close() and handleInput(data) when data === "\x03" should be extracted into a single helper function (e.g., cancelAndCleanup or performCancellation) to avoid repetition: move the body that sets isCancelled = true, iterates activePtys calling p.handleInput?.("\x03") and p.close?.(), clears activePtys, and fires closeEmitter.fire(130) into that helper, then call that helper from both close and handleInput branches; update references to activePtys, isCancelled, and closeEmitter inside the new helper so behavior remains identical.code/extensions/che-commands/tests/__mocks__/vscode.ts (1)
30-55: Consider addingTaskRevealKind,TaskPanelKind, andtasksto the mock.The
compositeTaskBuilder.tsusesvscode.TaskRevealKind,vscode.TaskPanelKind, andvscode.tasks.executeTask, which are currently mocked inline in tests. Adding them here would improve consistency and reduce test-level mocking.♻️ Optional additions to the mock
export const TaskScope = { Workspace: 1, }; + +export const TaskRevealKind = { + Always: 1, + Silent: 2, + Never: 3, +}; + +export const TaskPanelKind = { + Shared: 1, + Dedicated: 2, + New: 3, +}; + +export const tasks = { + executeTask: async () => ({ terminate: () => {} }), +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/extensions/che-commands/tests/__mocks__/vscode.ts` around lines 30 - 55, The mock is missing TaskRevealKind, TaskPanelKind, and vscode.tasks.executeTask used by compositeTaskBuilder.ts; add TaskRevealKind and TaskPanelKind enums/objects (e.g., values like Always/Hidden/never and Shared/Dedicated/Replace) alongside the existing Task and TaskScope, and add a tasks object exposing an executeTask method that returns a Promise (can resolve to undefined) and a registerTaskProvider stub; ensure the symbols TaskRevealKind, TaskPanelKind, and tasks.executeTask are exported from the mock so tests can use them consistently.code/extensions/che-commands/tests/mocks.ts (1)
45-47:close()always fires exit code 0, ignoring configuredexitCode.When
close()is called directly (e.g., during cancellation), it fires0instead of the configured exit code. This may not matter for current tests but could cause confusion if tests expectclose()to respect configured exit codes.♻️ Consider using configured exitCode or a parameter
- close() { - for (const l of this.closeListeners) l(0); + close(code?: number) { + for (const l of this.closeListeners) l(code ?? this.exitCode); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/extensions/che-commands/tests/mocks.ts` around lines 45 - 47, The close() method currently always invokes closeListeners with 0; update it to use the configured exit code (e.g., this.exitCode) or accept an optional parameter so it calls each listener with the real exit code instead of hardcoding 0; modify the close() signature and callers (or default to this.exitCode) and ensure closeListeners is invoked with that exit code value so cancellation paths and tests receive the correct code (refer to close(), closeListeners and any exitCode property/field).code/extensions/che-commands/tests/taskProvider.test.ts (1)
143-166: Cleanupprocess.env.TEST_DIRafter test to avoid pollution.The test sets
process.env.TEST_DIRwithout cleanup, which could affect other tests.♻️ Suggested fix
describe("Exec working directory expansion", () => { + const originalTestDir = process.env.TEST_DIR; + + afterEach(() => { + if (originalTestDir === undefined) { + delete process.env.TEST_DIR; + } else { + process.env.TEST_DIR = originalTestDir; + } + }); + test("expands ${VAR} placeholders from process.env", async () => { process.env.TEST_DIR = "/tmp/demo";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/extensions/che-commands/tests/taskProvider.test.ts` around lines 143 - 166, The test "expands ${VAR} placeholders from process.env" sets process.env.TEST_DIR but never restores it; either restore the original value or delete process.env.TEST_DIR after the test to avoid pollution. Update the test (or add an afterEach) around the test that calls provide/runFirst/MockTerminalAPI so you capture the previous value of process.env.TEST_DIR, run the assertions, and then set process.env.TEST_DIR back to the previous value (or delete it) in a finally/cleanup step to ensure other tests aren’t affected.code/extensions/che-commands/tests/compositeTaskBuilder.test.ts (1)
117-153: Test isolation: restore mockedvscodeproperties after test.The test mutates the global
vscodemock (TaskRevealKind,TaskPanelKind,tasks) without cleanup. This can pollute subsequent tests in the same suite.♻️ Suggested fix using beforeEach/afterEach
+describe("Composite — cross component execution", () => { + let originalTaskRevealKind: any; + let originalTaskPanelKind: any; + let originalTasks: any; + + beforeEach(() => { + originalTaskRevealKind = (vscode as any).TaskRevealKind; + originalTaskPanelKind = (vscode as any).TaskPanelKind; + originalTasks = (vscode as any).tasks; + }); + + afterEach(() => { + (vscode as any).TaskRevealKind = originalTaskRevealKind; + (vscode as any).TaskPanelKind = originalTaskPanelKind; + (vscode as any).tasks = originalTasks; + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/extensions/che-commands/tests/compositeTaskBuilder.test.ts` around lines 117 - 153, The test "parallel composite executes all components and opens only child terminals" mutates global vscode properties (TaskRevealKind, TaskPanelKind, tasks) and should restore them to avoid polluting other tests; modify the test file to save the original values of (vscode as any).TaskRevealKind, (vscode as any).TaskPanelKind and (vscode as any).tasks before mutating and restore them in an afterEach (or restore at the end of the test), or move the setup into a beforeEach and the teardown into afterEach so provide, MockTerminalAPI and runByName behavior remains isolated; locate the test by its name and update the surrounding describe block to include beforeEach/afterEach that capture/restore those symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/extensions/che-commands/jest.config.js`:
- Around line 19-23: The current jest config uses the deprecated
globals['ts-jest'] and likely a preset; replace this by removing the preset and
moving the ts-jest settings into the transform option: remove the globals object
(and any preset entry), add a transform mapping for the TypeScript files that
references ts-jest and passes the tsconfig.jest.json path (preserving the same
tsconfig). Update any references to globals or preset so Jest uses transform
with ts-jest instead of globals['ts-jest'].
In `@code/extensions/che-commands/tsconfig.json`:
- Line 10: Remove test-specific types and include patterns from the main
tsconfig: delete "jest" from the "types" array and remove the "tests/**/*.ts"
(or any test globs) from "include" in the existing tsconfig.json; instead ensure
the dedicated tsconfig.jest.json contains those entries and is used for test
runs. Locate the "types": ["jest"] reference and any "tests/**/*.ts" include
entries in the tsconfig.json and remove them, and verify tsconfig.jest.json
contains the test-specific "types" and "include" so test files and Jest types
are only declared there.
---
Duplicate comments:
In `@code/extensions/che-commands/src/compositeTaskBuilder.ts`:
- Around line 328-330: sanitize currently strips a trailing single ampersand
(background operator) because the regex includes '&' in the character class;
update the sanitize function so it only strips command joiners (&&, ||, |, ;)
but preserves a standalone '&' at the end; specifically, replace the current
regex in sanitize with one that does not include '&' in the single-character
class (e.g., remove '&' from [|;&] and keep matching multi-char operators like
&& and ||) so sanitize(cmd: string) returns the trimmed string but leaves a
trailing single '&' intact.
---
Nitpick comments:
In `@code/extensions/che-commands/package.json`:
- Line 82: The file code/extensions/che-commands/package.json is missing a
trailing newline after the final closing brace; open that package.json and add a
single newline character at the end of file so the file ends with "\n" (ensure
the final "}" remains intact), save the file and include the change in your
commit so linters/editors recognize the EOF newline.
In `@code/extensions/che-commands/src/compositeTaskBuilder.ts`:
- Around line 187-213: The cancellation/cleanup logic duplicated in the
compositeTaskBuilder object's close() and handleInput(data) when data === "\x03"
should be extracted into a single helper function (e.g., cancelAndCleanup or
performCancellation) to avoid repetition: move the body that sets isCancelled =
true, iterates activePtys calling p.handleInput?.("\x03") and p.close?.(),
clears activePtys, and fires closeEmitter.fire(130) into that helper, then call
that helper from both close and handleInput branches; update references to
activePtys, isCancelled, and closeEmitter inside the new helper so behavior
remains identical.
In `@code/extensions/che-commands/tests/__mocks__/vscode.ts`:
- Around line 30-55: The mock is missing TaskRevealKind, TaskPanelKind, and
vscode.tasks.executeTask used by compositeTaskBuilder.ts; add TaskRevealKind and
TaskPanelKind enums/objects (e.g., values like Always/Hidden/never and
Shared/Dedicated/Replace) alongside the existing Task and TaskScope, and add a
tasks object exposing an executeTask method that returns a Promise (can resolve
to undefined) and a registerTaskProvider stub; ensure the symbols
TaskRevealKind, TaskPanelKind, and tasks.executeTask are exported from the mock
so tests can use them consistently.
In `@code/extensions/che-commands/tests/compositeTaskBuilder.test.ts`:
- Around line 117-153: The test "parallel composite executes all components and
opens only child terminals" mutates global vscode properties (TaskRevealKind,
TaskPanelKind, tasks) and should restore them to avoid polluting other tests;
modify the test file to save the original values of (vscode as
any).TaskRevealKind, (vscode as any).TaskPanelKind and (vscode as any).tasks
before mutating and restore them in an afterEach (or restore at the end of the
test), or move the setup into a beforeEach and the teardown into afterEach so
provide, MockTerminalAPI and runByName behavior remains isolated; locate the
test by its name and update the surrounding describe block to include
beforeEach/afterEach that capture/restore those symbols.
In `@code/extensions/che-commands/tests/mocks.ts`:
- Around line 45-47: The close() method currently always invokes closeListeners
with 0; update it to use the configured exit code (e.g., this.exitCode) or
accept an optional parameter so it calls each listener with the real exit code
instead of hardcoding 0; modify the close() signature and callers (or default to
this.exitCode) and ensure closeListeners is invoked with that exit code value so
cancellation paths and tests receive the correct code (refer to close(),
closeListeners and any exitCode property/field).
In `@code/extensions/che-commands/tests/taskProvider.test.ts`:
- Around line 143-166: The test "expands ${VAR} placeholders from process.env"
sets process.env.TEST_DIR but never restores it; either restore the original
value or delete process.env.TEST_DIR after the test to avoid pollution. Update
the test (or add an afterEach) around the test that calls
provide/runFirst/MockTerminalAPI so you capture the previous value of
process.env.TEST_DIR, run the assertions, and then set process.env.TEST_DIR back
to the previous value (or delete it) in a finally/cleanup step to ensure other
tests aren’t affected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c09384de-6585-4a52-b536-95dd5db8d961
⛔ Files ignored due to path filters (1)
code/extensions/che-commands/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
build/dockerfiles/linux-libc-ubi8.Dockerfilebuild/dockerfiles/linux-libc-ubi9.Dockerfilebuild/dockerfiles/linux-musl.Dockerfilecode/extensions/che-commands/jest.config.jscode/extensions/che-commands/package.jsoncode/extensions/che-commands/src/compositeTaskBuilder.tscode/extensions/che-commands/src/taskProvider.tscode/extensions/che-commands/tests/__mocks__/vscode.tscode/extensions/che-commands/tests/compositeTaskBuilder.test.tscode/extensions/che-commands/tests/mocks.tscode/extensions/che-commands/tests/taskProvider.test.tscode/extensions/che-commands/tsconfig.jest.jsoncode/extensions/che-commands/tsconfig.json
| "declaration": true, | ||
| "types": [ | ||
| "node", | ||
| "jest", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if test files are compiled to the out directory during build
# and if the webpack config processes tsconfig.json
echo "=== Checking webpack config usage of tsconfig.json ==="
rg -n "tsconfig.json" code/extensions/shared.webpack.config.mjs code/extensions/mangle-loader.js
echo ""
echo "=== Checking if tests directory exists and what it contains ==="
fd -t f . code/extensions/che-commands/tests 2>/dev/null | head -20
echo ""
echo "=== Checking gulp compile task for che-commands ==="
rg -n "che-commands" code/build/gulpfile.extensions.js 2>/dev/null || echo "No specific gulp task found"Repository: che-incubator/che-code
Length of output: 758
🏁 Script executed:
#!/bin/bash
echo "=== Checking for tsconfig.jest.json ==="
find code/extensions/che-commands -name "tsconfig.jest.json" -o -name "tsconfig.*.json" | head -20
echo ""
echo "=== Examining shared.webpack.config.mjs to understand tsconfig usage ==="
sed -n '40,160p' code/extensions/shared.webpack.config.mjs
echo ""
echo "=== Checking current tsconfig.json include array ==="
cat code/extensions/che-commands/tsconfig.json | grep -A 10 '"include"'Repository: che-incubator/che-code
Length of output: 4833
🏁 Script executed:
#!/bin/bash
echo "=== Checking tsLoaderOptions definition ==="
rg -B 5 "tsLoaderOptions" code/extensions/shared.webpack.config.mjs | head -30
echo ""
echo "=== Examining mangle-loader.js usage of configFile ==="
cat code/extensions/mangle-loader.js | head -50
echo ""
echo "=== Checking if tsconfig.jest.json extends main tsconfig ==="
cat code/extensions/che-commands/tsconfig.jest.jsonRepository: che-incubator/che-code
Length of output: 2606
Test files and Jest types should not be in the main tsconfig.json.
While the CopyWebpackPlugin prevents test files from being included in the production output, including "jest" in types and "tests/**/*.ts" in the main tsconfig still pollutes the production type-checking environment. The dedicated tsconfig.jest.json already exists and should be the single source of truth for all test-related configuration.
Proposed fix
"compilerOptions": {
"outDir": "./out",
"experimentalDecorators": true,
"emitDecoratorMetadata": true,
- "declaration": true,
"types": [
"node",
- "jest",
"reflect-metadata"
]
},
"include": [
"src/**/*",
- "tests/**/*.ts",
"../../src/vscode-dts/vscode.d.ts"
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "jest", | |
| "compilerOptions": { | |
| "outDir": "./out", | |
| "experimentalDecorators": true, | |
| "emitDecoratorMetadata": true, | |
| "types": [ | |
| "node", | |
| "reflect-metadata" | |
| ] | |
| }, | |
| "include": [ | |
| "src/**/*", | |
| "../../src/vscode-dts/vscode.d.ts" | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/extensions/che-commands/tsconfig.json` at line 10, Remove test-specific
types and include patterns from the main tsconfig: delete "jest" from the
"types" array and remove the "tests/**/*.ts" (or any test globs) from "include"
in the existing tsconfig.json; instead ensure the dedicated tsconfig.jest.json
contains those entries and is used for test runs. Locate the "types": ["jest"]
reference and any "tests/**/*.ts" include entries in the tsconfig.json and
remove them, and verify tsconfig.jest.json contains the test-specific "types"
and "include" so test files and Jest types are only declared there.
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/extensions/che-commands/jest.config.js`:
- Around line 14-28: Replace the multiple named exports (testEnvironment, roots,
collectCoverage, transform, moduleNameMapper, collectCoverageFrom) with a single
exported Jest configuration object (e.g., module.exports = { ... } or export
default { ... }) that contains those keys and values; keep the existing values
(including the '^.+\\.tsx?$' transform using ts-jest and its tsconfig setting,
the moduleNameMapper for '^vscode$', and the collectCoverageFrom array) but
remove the individual export const declarations and return them as properties of
one config object.
In `@code/extensions/che-commands/src/compositeTaskBuilder.ts`:
- Around line 43-45: The code currently turns an empty/unresolved composite into
a no-op echo task by returning this.echoTask(`Composite ${command.id} resolved
empty`); instead of skipping it, change the logic to return undefined (or null)
when execs.length === 0 so DevfileTaskProvider.computeTasks() can filter it out;
update the same pattern found around lines where the alternate branch returns an
echo task (referencing this.echoTask and the composite task building function in
compositeTaskBuilder.ts) and ensure callers expect a possibly undefined task
rather than a placeholder echo.
- Around line 88-90: The composite PTY emits duplicate close events because both
the child PTY exit handler and the final open() success path call
closeEmitter.fire; modify the CustomExecution created in compositeTaskBuilder.ts
so closeEmitter.fire is invoked exactly once by adding a single shared "closed"
flag (or using an atomic/once guard) checked/updated before firing; update the
open() finalization path and any child PTY exit/CTRL+C handlers (the places that
currently call closeEmitter.fire with exit code 130 and the final success close)
to consult this guard and only fire when it transitions from false to true, and
ensure any related disposables/listeners clear or ignore further close attempts.
- Around line 61-67: Composite exec commands are bypassing the normal standalone
path (createCheTask) and call getMachineExecPTY directly, causing env to be
dropped, sanitize() to be skipped and workingDir to differ; update the composite
branches that build exec tasks (places that use cmd.exec.commandLine and call
getMachineExecPTY) to instead construct tasks via createCheTask(...) passing
through cmd.exec.env and resolved workingDir (use cmd.exec.workingDir ||
"${PROJECT_SOURCE}"), and ensure the commandLine is run through sanitize() (or
the same sanitizer used by createCheTask) so composite, sequential and parallel
branches behave identically; apply the same change to the other
composite-building occurrences that mirror this pattern.
- Around line 190-207: The composite PTY currently fires closeEmitter.fire(0)
immediately after launching children; change it to capture the TaskExecution
returned by vscode.tasks.executeTask(childTask) inside the open() of the
CustomExecution (where execs.forEach and buildExecTask(...) are invoked), track
those executions and wait for their termination via
vscode.tasks.onDidEndTaskProcess or vscode.tasks.onDidEndTask, and only call
closeEmitter.fire(0) when all tracked child TaskExecution instances have
completed; also wire parent close() to cancel/terminate any still-running
TaskExecution handles to support proper cancellation and cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8456d5f7-14d6-434c-a3c9-ddc4e2bce433
📒 Files selected for processing (3)
code/extensions/che-commands/jest.config.jscode/extensions/che-commands/src/compositeTaskBuilder.tscode/extensions/che-commands/tests/compositeTaskBuilder.test.ts
| export const testEnvironment = 'node'; | ||
| export const roots = ['<rootDir>/tests']; | ||
| export const collectCoverage = true; | ||
| export const transform = { | ||
| '^.+\\.tsx?$': ['ts-jest', { | ||
| tsconfig: 'tsconfig.json' }], | ||
| }; | ||
| export const moduleNameMapper = { | ||
| '^vscode$': '<rootDir>/tests/__mocks__/vscode.ts' | ||
| }; | ||
| export const collectCoverageFrom = [ | ||
| "src/**/*.ts", | ||
| "!src/**/*.d.ts", | ||
| "!tests/**", | ||
| ] No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Jest's official configuration docs, should jest.config.js export the configuration object itself (module.exportsorexport default) instead of named exports?
💡 Result:
Yes, in Jest's official configuration docs, jest.config.js should export the configuration object itself using module.exports = config (for CommonJS JavaScript) or export default config (for ES modules or TypeScript), rather than named exports. The docs explicitly state: "The configuration file should simply export an object," and provide examples using these default export patterns.
Citations:
- 1: https://jestjs.io/docs/configuration
- 2: https://jestjs.io/docs/29.5/configuration
- 3: https://jestjs.io/docs/en/configuration
Export the Jest configuration as a single object, not named exports.
The jest.config.js file currently exports individual named bindings (export const testEnvironment, export const roots, etc.), but Jest requires the configuration to be exported as a single object via module.exports or export default. The runner will fail to load these settings before tests run.
Proposed fix
//@ts-check
-export const testEnvironment = 'node';
-export const roots = ['<rootDir>/tests'];
-export const collectCoverage = true;
-export const transform = {
- '^.+\\.tsx?$': ['ts-jest', {
- tsconfig: 'tsconfig.json' }],
-};
-export const moduleNameMapper = {
- '^vscode$': '<rootDir>/tests/__mocks__/vscode.ts'
-};
-export const collectCoverageFrom = [
- "src/**/*.ts",
- "!src/**/*.d.ts",
- "!tests/**",
-]
+/** `@type` {import('jest').Config} */
+module.exports = {
+ testEnvironment: 'node',
+ roots: ['<rootDir>/tests'],
+ collectCoverage: true,
+ transform: {
+ '^.+\\.tsx?$': ['ts-jest', { tsconfig: 'tsconfig.json' }],
+ },
+ moduleNameMapper: {
+ '^vscode$': '<rootDir>/tests/__mocks__/vscode.ts',
+ },
+ collectCoverageFrom: [
+ 'src/**/*.ts',
+ '!src/**/*.d.ts',
+ '!tests/**',
+ ],
+};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/extensions/che-commands/jest.config.js` around lines 14 - 28, Replace
the multiple named exports (testEnvironment, roots, collectCoverage, transform,
moduleNameMapper, collectCoverageFrom) with a single exported Jest configuration
object (e.g., module.exports = { ... } or export default { ... }) that contains
those keys and values; keep the existing values (including the '^.+\\.tsx?$'
transform using ts-jest and its tsconfig setting, the moduleNameMapper for
'^vscode$', and the collectCoverageFrom array) but remove the individual export
const declarations and return them as properties of one config object.
| if (!execs.length) { | ||
| return this.echoTask(`Composite ${command.id} resolved empty`); | ||
| } |
There was a problem hiding this comment.
Skip empty composites instead of surfacing a fake echo task.
Line 44 turns an unresolved/empty composite into a runnable task. Because DevfileTaskProvider.computeTasks() only filters undefined, that no-op still shows up in the task list and the “invalid composites are not exposed” behavior can pass just because the task was renamed.
💡 Proposed fix
- if (!execs.length) {
- return this.echoTask(`Composite ${command.id} resolved empty`);
- }
+ if (!execs.length) {
+ this.channel.appendLine(`Skipping composite ${command.id}: resolved empty`);
+ return undefined;
+ }Also applies to: 291-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/extensions/che-commands/src/compositeTaskBuilder.ts` around lines 43 -
45, The code currently turns an empty/unresolved composite into a no-op echo
task by returning this.echoTask(`Composite ${command.id} resolved empty`);
instead of skipping it, change the logic to return undefined (or null) when
execs.length === 0 so DevfileTaskProvider.computeTasks() can filter it out;
update the same pattern found around lines where the alternate branch returns an
echo task (referencing this.echoTask and the composite task building function in
compositeTaskBuilder.ts) and ensure callers expect a possibly undefined task
rather than a placeholder echo.
| if (cmd.exec?.commandLine) { | ||
| result.push({ | ||
| command: cmd.exec.commandLine, | ||
| workdir: cmd.exec.workingDir || "${PROJECT_SOURCE}", | ||
| component: cmd.exec.component, | ||
| env: cmd.exec.env, | ||
| label: cmd.exec.label, |
There was a problem hiding this comment.
Route composite leaves through the same exec-task path as standalone commands.
flatten() keeps env, the sequential branch sanitizes commands, and standalone exec tasks still go through createCheTask(...); both composite branches bypass that by calling getMachineExecPTY(...) directly. That makes the same command behave differently inside a composite: env is dropped, workingDir handling can drift from standalone tasks, and the parallel branch skips sanitize(). The workingDir regression called out in the PR discussion is a likely symptom of this split.
Also applies to: 99-103, 241-257, 273-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/extensions/che-commands/src/compositeTaskBuilder.ts` around lines 61 -
67, Composite exec commands are bypassing the normal standalone path
(createCheTask) and call getMachineExecPTY directly, causing env to be dropped,
sanitize() to be skipped and workingDir to differ; update the composite branches
that build exec tasks (places that use cmd.exec.commandLine and call
getMachineExecPTY) to instead construct tasks via createCheTask(...) passing
through cmd.exec.env and resolved workingDir (use cmd.exec.workingDir ||
"${PROJECT_SOURCE}"), and ensure the commandLine is run through sanitize() (or
the same sanitizer used by createCheTask) so composite, sequential and parallel
branches behave identically; apply the same change to the other
composite-building occurrences that mirror this pattern.
| const execution = new vscode.CustomExecution(async () => { | ||
| const writeEmitter = new vscode.EventEmitter<string>(); | ||
| const closeEmitter = new vscode.EventEmitter<number>(); |
There was a problem hiding this comment.
Close the sequential PTY exactly once.
When Ctrl+C or close() emits 130, open() can still fall through to the final success close after the active child PTY exits. That produces duplicate close events and can make a cancelled composite look successful.
💡 Proposed fix
const execution = new vscode.CustomExecution(async () => {
const writeEmitter = new vscode.EventEmitter<string>();
const closeEmitter = new vscode.EventEmitter<number>();
+ let closed = false;
+ const finalize = (exitCode: number) => {
+ if (closed) return;
+ closed = true;
+ closeEmitter.fire(exitCode);
+ };
const aggregator: vscode.Pseudoterminal = {
onDidWrite: writeEmitter.event,
onDidClose: closeEmitter.event,
@@
for (const e of execs) {
if (isCancelled) break;
const exitCode: number = await run(e);
if(exitCode !== 0) {
isCancelled = true;
- closeEmitter.fire(exitCode);
+ finalize(exitCode);
return;
}
}
- closeEmitter.fire(0);
+ finalize(isCancelled ? 130 : 0);
},
@@
activePtys.clear();
- closeEmitter.fire(130);
+ finalize(130);
},
@@
activePtys.clear();
- closeEmitter.fire(130);
+ finalize(130);
}
},Also applies to: 125-148, 151-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/extensions/che-commands/src/compositeTaskBuilder.ts` around lines 88 -
90, The composite PTY emits duplicate close events because both the child PTY
exit handler and the final open() success path call closeEmitter.fire; modify
the CustomExecution created in compositeTaskBuilder.ts so closeEmitter.fire is
invoked exactly once by adding a single shared "closed" flag (or using an
atomic/once guard) checked/updated before firing; update the open() finalization
path and any child PTY exit/CTRL+C handlers (the places that currently call
closeEmitter.fire with exit code 130 and the final success close) to consult
this guard and only fire when it transitions from false to true, and ensure any
related disposables/listeners clear or ignore further close attempts.
| const execution = new vscode.CustomExecution(async () => { | ||
| const closeEmitter = new vscode.EventEmitter<number>(); | ||
|
|
||
| const pty: vscode.Pseudoterminal = { | ||
| onDidWrite: new vscode.EventEmitter<string>().event, | ||
| onDidClose: closeEmitter.event, | ||
|
|
||
| open: () => { | ||
| execs.forEach((e, index) => { | ||
| const childTask = this.buildExecTask(e, index === 0); | ||
| vscode.tasks.executeTask(childTask); | ||
| }); | ||
|
|
||
| closeEmitter.fire(0); | ||
| }, | ||
|
|
||
| close: () => closeEmitter.fire(0), | ||
| handleInput: () => {}, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the VS Code Tasks API, what is the recommended way for a CustomExecutiontask to wait for and terminate child tasks started viavscode.tasks.executeTask?
💡 Result:
The recommended way for a CustomExecution task to wait for and terminate child tasks started via vscode.tasks.executeTask is to use the global task events and taskExecutions list within the Pseudoterminal implementation. To wait for child tasks: - Call vscode.tasks.executeTask(task) to get the TaskExecution. - Use vscode.tasks.onDidEndTask or onDidEndTaskProcess with a Promise and disposable listener to wait until the specific child task ends, matching by task properties like source, name, definition, or group (noting historical bugs with direct === comparison). Example: const childExecution = await vscode.tasks.executeTask(childTask); return new Promise((resolve, reject) => { const disposable = vscode.tasks.onDidEndTask(e => { if (matchesTask(e.execution.task, childTask)) { // implement matcher disposable.dispose; if (e.execution.exitCode === 0) resolve; else reject; } }); }); To terminate child tasks: - Access vscode.tasks.taskExecutions (array of active TaskExecution). - Iterate and call terminate on matching TaskExecution instances. - Handle in Pseudoterminal.close method for task cancellation. Example: close { for (const exec of vscode.tasks.taskExecutions) { if (matchesTask(exec.task, expectedChildTask)) { exec.terminate; } } this.closeEmitter.fire(0); // or handle appropriately } This leverages the Tasks API events and TaskExecution.terminate method, as shown in StackOverflow examples and official API docs. CustomExecution manages its own logic via Pseudoterminal, so track child executions explicitly if needed. No official docs show executeTask directly in samples, but API supports it fully.
Citations:
- 1: https://stackoverflow.com/questions/61428928/how-to-await-a-build-task-in-a-vs-code-extension
- 2: https://www.vscodeapi.com/modules/vscode.tasks.html
- 3: https://code.visualstudio.com/api/references/vscode-api
- 4: https://code.visualstudio.com/api/extension-guides/task-provider
- 5: https://vscode-api.js.org/interfaces/vscode.TaskExecution.html
- 6: https://stackoverflow.com/questions/50431036/vscode-terminate-task-by-name
Wait for child task completion before closing the composite.
vscode.tasks.executeTask() returns a TaskExecution handle required to track and terminate the child task. The current code discards this handle and fires close(0) immediately after launching children, so the composite reports success before any child finishes. This also breaks parent-side cancellation—the launched tasks cannot be terminated.
Capture the returned TaskExecution from each executeTask() call, listen for task completion using vscode.tasks.onDidEndTask or onDidEndTaskProcess, and close the parent PTY only after all children finish. (VS Code Task Provider API, TaskExecution reference)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/extensions/che-commands/src/compositeTaskBuilder.ts` around lines 190 -
207, The composite PTY currently fires closeEmitter.fire(0) immediately after
launching children; change it to capture the TaskExecution returned by
vscode.tasks.executeTask(childTask) inside the open() of the CustomExecution
(where execs.forEach and buildExecTask(...) are invoked), track those executions
and wait for their termination via vscode.tasks.onDidEndTaskProcess or
vscode.tasks.onDidEndTask, and only call closeEmitter.fire(0) when all tracked
child TaskExecution instances have completed; also wire parent close() to
cancel/terminate any still-running TaskExecution handles to support proper
cancellation and cleanup.
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-639-amd64 |
|
@RomanNikitenko 
|
RomanNikitenko
left a comment
RomanNikitenko
left a comment
There was a problem hiding this comment.
@msivasubramaniaan
I tested commands with env variables, for example go stack uses this approach: https://github.com/devfile/registry/blob/7c358cd8ef31c59111fac73808eaab7b37afd19c/stacks/go/2.6.0/devfile.yaml#L66-L73
It works for regular commands,
but
env variables are not resolved for composite commands
resolving_env_var_bug.mov
3 participants
What does this PR do?
This PR supports to run the composite commands in terminal
What issues does this PR fix?
eclipse-che/che#21859
eclipse-che/che#23726
eclipse-che/che#23709
eclipse-che/che#23725
How to test this PR?
Added workspace which has all commands of devfile. Run those commands from
devfile.yaml.Does this PR contain changes that override default upstream Code-OSS behavior?
git rebasewere added to the .rebase folderSummary by CodeRabbit
New Features
Tests
Chores