Phase 1: restore the build and run tasks on 2026 agents#17
Merged
Conversation
The repo could not produce a .vsix from a clean checkout: Dependabot had bumped gulp to ^4 and webpack-stream to ^7 without migrating the gulp 3 / webpack 1 / babel 6 code, so `gulp` and every CI path failed. Both tasks also declared the removed Node 6 execution handler. This restores a working, versioned build and gets the tasks onto Node20_1. Build system (1.1-1.2, 1.6) - Replace the Gulpfile with npm scripts + webpack 5 (webpack.config.js): AMD bundle of the results-tab scripts + copy-webpack-plugin for static files. - Add scripts/install-task-deps.js (installs each task's prod node_modules into dist) and scripts/stamp-version.js (package.json version -> dist manifest + task.json, resolving the 0.2.7/0.1.0/0.2.0 drift). - Upgrade tfx-cli ^0.12 -> ^0.23.4; drop the dead gulp/babel-6/webpack-stream toolchain and .travis.yml (it only ran `gulp`). Tasks (1.3-1.4) - Switch both task.json execution handlers Node -> Node20_1; bump minimumAgentVersion to 3.232.1. - Add tb-main/package.json (azure-pipelines-task-lib ^5.277.0); delete the 210 committed vendored node_modules files; require azure-pipelines-task-lib/task. Correctness fixes (1.5) - tb-stop-tunnel: guard TB_TUNNEL_PID before use and validate the pid, so an unstarted tunnel no longer crashes with a TypeError. - tb-main: null-check endpoint auth and fix the copy-pasted "SonarQube" error message; use tl.getBoolInput for tbTunnel; fix the tunnel_bin variable shadowing (jar path vs ChildProcess); fall back to RELEASE_* vars for TB_BUILD_NAME so release runs get a real name. - task.json: connectedServiceName required:true; drop the $(message) ghost input from instanceNameFormat. Docs - Add MODERNIZATION.md (full roadmap; this is Phase 1). Update README build steps and CLAUDE.md to the new build. Ignore nested node_modules and .serena. Verified: `npm run package` produces testingbot.testingbot-tasks-0.2.7.vsix; both tasks run under Node 22 (no-tunnel path, stop-tunnel guards, RELEASE_* fallback all exercised).
📝 WalkthroughWalkthroughThe pull request replaces the legacy Gulp packaging setup with npm scripts and webpack, adds task dependency installation and version stamping, migrates tasks to Node20_1 and the Azure Pipelines task library, improves tunnel handling, and adds repository guidance and modernization documentation. ChangesBuild and task modernization
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant npm as npm run package
participant webpack as webpack
participant tasks as install-task-deps.js
participant manifests as stamp-version.js
participant tfx as tfx extension create
Developer->>npm: start packaging
npm->>webpack: build web assets into dist
webpack->>tasks: provide tb-* task directories
tasks->>manifests: install production dependencies
manifests->>tfx: stamp extension and task versions
tfx-->>Developer: create packaged VSIX
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 73-78: Update the task-code documentation paragraph to state that
azure-pipelines-task-lib/task is used only by tb-main/testingbot.js, while
tb-stop-tunnel/sample.js reads TB_TUNNEL_PID directly and remains
dependency-free. Keep the existing tb-main runtime-dependency and
build-installation details, and explicitly preserve the stop-tunnel task’s
dependency-free requirement.
In `@MODERNIZATION.md`:
- Around line 7-15: Update MODERNIZATION.md to mark the completed Phase 1
findings as resolved or historical, including clean-checkout packaging, Node 6
handler migration, dependency/version drift, and the Gulp/npm/webpack migration.
Remove these items from the pending action list while preserving only genuinely
outstanding work as future actions.
In `@README.md`:
- Around line 12-23: Update the README build instructions to use consistent 1.,
2., and 3. ordered-list markers, add blank lines before and after each fenced
code block, and specify bash as the fence language for the npm commands to
satisfy MD029, MD031, and MD040.
In `@scripts/stamp-version.js`:
- Around line 27-30: Update the task-manifest loop in the version-stamping logic
to fail immediately when either required manifest under tb-main or
tb-stop-tunnel is missing, instead of continuing. Preserve stamping from
package.json for every existing task.json and report the missing manifest as a
build error.
- Around line 9-10: Update the version parsing in scripts/stamp-version.js
around the version constant and major/minor/patch destructuring to require an
exact major.minor.patch format, reject prerelease or malformed values, and
convert each component to a number before stamping task.json. Ensure the
resulting Major, Minor, and Patch fields are numeric and fail clearly when the
package version is invalid.
In `@tb-main/package.json`:
- Around line 1-11: Add an engines.node constraint to the tb-main package
manifest requiring Node.js 20 or later, while preserving the existing package
metadata and dependencies.
In `@tb-main/task.json`:
- Line 19: Update the stale minimum-agent version message in testingbot.js to
require version 3.232.1, matching the minimumAgentVersion value in task
metadata, or derive the message directly from that metadata.
In `@tb-stop-tunnel/sample.js`:
- Around line 10-15: Validate the PID parsed in the sample’s shutdown flow
before any process.kill call: accept only a complete, decimal, strictly positive
PID matching the expected pid_ value format, rejecting pid_0, negative values,
suffixes, and other malformed input. Preserve the existing log and clean exit
for invalid PIDs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e64f41bc-cec1-4b27-bb60-d6bbb9306e14
⛔ Files ignored due to path filters (212)
package-lock.jsonis excluded by!**/package-lock.jsontb-main/node_modules/.bin/semveris excluded by!**/node_modules/**tb-main/node_modules/.bin/shjsis excluded by!**/node_modules/**tb-main/node_modules/.bin/uuidis excluded by!**/node_modules/**tb-main/node_modules/balanced-match/.npmignoreis excluded by!**/node_modules/**tb-main/node_modules/balanced-match/LICENSE.mdis excluded by!**/node_modules/**tb-main/node_modules/balanced-match/README.mdis excluded by!**/node_modules/**tb-main/node_modules/balanced-match/index.jsis excluded by!**/node_modules/**tb-main/node_modules/balanced-match/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/brace-expansion/README.mdis excluded by!**/node_modules/**tb-main/node_modules/brace-expansion/index.jsis excluded by!**/node_modules/**tb-main/node_modules/brace-expansion/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/concat-map/.travis.ymlis excluded by!**/node_modules/**tb-main/node_modules/concat-map/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/concat-map/README.markdownis excluded by!**/node_modules/**tb-main/node_modules/concat-map/example/map.jsis excluded by!**/node_modules/**tb-main/node_modules/concat-map/index.jsis excluded by!**/node_modules/**tb-main/node_modules/concat-map/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/concat-map/test/map.jsis excluded by!**/node_modules/**tb-main/node_modules/glob/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/glob/README.mdis excluded by!**/node_modules/**tb-main/node_modules/glob/common.jsis excluded by!**/node_modules/**tb-main/node_modules/glob/glob.jsis excluded by!**/node_modules/**tb-main/node_modules/glob/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/glob/sync.jsis excluded by!**/node_modules/**tb-main/node_modules/inflight/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/inflight/README.mdis excluded by!**/node_modules/**tb-main/node_modules/inflight/inflight.jsis excluded by!**/node_modules/**tb-main/node_modules/inflight/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/inherits/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/inherits/README.mdis excluded by!**/node_modules/**tb-main/node_modules/inherits/inherits.jsis excluded by!**/node_modules/**tb-main/node_modules/inherits/inherits_browser.jsis excluded by!**/node_modules/**tb-main/node_modules/inherits/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/minimatch/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/minimatch/README.mdis excluded by!**/node_modules/**tb-main/node_modules/minimatch/minimatch.jsis excluded by!**/node_modules/**tb-main/node_modules/minimatch/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/mockery/.npmignoreis excluded by!**/node_modules/**tb-main/node_modules/mockery/.travis.ymlis excluded by!**/node_modules/**tb-main/node_modules/mockery/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/mockery/README.mdis excluded by!**/node_modules/**tb-main/node_modules/mockery/examples/example1.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/mockery.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/1-general.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/allowable-unhook.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/fixtures/fake_module.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/fixtures/fake_module_2.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/fixtures/intermediary.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/fixtures/substitute_fake_module.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/intermediary.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/logging-allow-array.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/logging-allowable.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/logging.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/registered.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/self-register.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/substitute.jsis excluded by!**/node_modules/**tb-main/node_modules/mockery/test/warn-replace.jsis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/.npmignoreis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/LICENSE.mdis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/README.mdis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/benchmark/README.mdis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/benchmark/bench.gnuis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/benchmark/bench.shis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/benchmark/benchmark-native.cis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/benchmark/benchmark.jsis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/bin/uuidis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/bower.jsonis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/component.jsonis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/test/compare_v1.jsis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/test/test.htmlis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/test/test.jsis excluded by!**/node_modules/**tb-main/node_modules/node-uuid/uuid.jsis excluded by!**/node_modules/**tb-main/node_modules/once/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/once/README.mdis excluded by!**/node_modules/**tb-main/node_modules/once/once.jsis excluded by!**/node_modules/**tb-main/node_modules/once/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/path-is-absolute/index.jsis excluded by!**/node_modules/**tb-main/node_modules/path-is-absolute/licenseis excluded by!**/node_modules/**tb-main/node_modules/path-is-absolute/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/path-is-absolute/readme.mdis excluded by!**/node_modules/**tb-main/node_modules/q/CHANGES.mdis excluded by!**/node_modules/**tb-main/node_modules/q/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/q/README.mdis excluded by!**/node_modules/**tb-main/node_modules/q/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/q/q.jsis excluded by!**/node_modules/**tb-main/node_modules/q/queue.jsis excluded by!**/node_modules/**tb-main/node_modules/semver/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/semver/README.mdis excluded by!**/node_modules/**tb-main/node_modules/semver/bin/semveris excluded by!**/node_modules/**tb-main/node_modules/semver/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/semver/range.bnfis excluded by!**/node_modules/**tb-main/node_modules/semver/semver.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/.documentup.jsonis excluded by!**/node_modules/**tb-main/node_modules/shelljs/.jshintrcis excluded by!**/node_modules/**tb-main/node_modules/shelljs/.npmignoreis excluded by!**/node_modules/**tb-main/node_modules/shelljs/.travis.ymlis excluded by!**/node_modules/**tb-main/node_modules/shelljs/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/shelljs/README.mdis excluded by!**/node_modules/**tb-main/node_modules/shelljs/bin/shjsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/global.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/make.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/shelljs/scripts/generate-docs.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/scripts/run-tests.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/shell.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/cat.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/cd.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/chmod.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/common.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/cp.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/dirs.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/echo.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/error.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/exec.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/find.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/grep.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/ln.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/ls.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/mkdir.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/mv.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/popd.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/pushd.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/pwd.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/rm.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/sed.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/tempdir.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/test.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/to.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/toEnd.jsis excluded by!**/node_modules/**tb-main/node_modules/shelljs/src/which.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/BuildApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/ClientApiBases.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/CoreApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/FileContainerApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/GalleryApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/GitApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/HttpClient.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/README.mdis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/ReleaseManagementApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/RestClient.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/Serialization.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/TaskAgentApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/TaskAgentApiBase.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/TaskApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/TestApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/TfvcApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/VsoClient.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/WebApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/WorkItemTrackingApi.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/handlers/apiversion.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/handlers/basiccreds.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/handlers/bearertoken.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/BuildInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/CoreInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/FileContainerInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/GalleryInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/GitInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/ReleaseManagementInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/TaskAgentInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/TestInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/TfvcInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/WorkItemTrackingInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/common/FormInputInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/common/OperationsInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/common/SystemDataInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/common/TfsInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/common/VSSInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/interfaces/common/VsoBaseInterfaces.jsis excluded by!**/node_modules/**tb-main/node_modules/vso-node-api/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/README.mdis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/Strings/resources.resjson/de-de/resources.resjsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/Strings/resources.resjson/en-US/resources.resjsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/Strings/resources.resjson/es-es/resources.resjsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/Strings/resources.resjson/fr-fr/resources.resjsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/Strings/resources.resjson/it-IT/resources.resjsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/Strings/resources.resjson/ja-jp/resources.resjsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/Strings/resources.resjson/ko-KR/resources.resjsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/Strings/resources.resjson/ru-RU/resources.resjsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/Strings/resources.resjson/zh-CN/resources.resjsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/Strings/resources.resjson/zh-TW/resources.resjsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/extensions.d.tsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/extensions.jsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/lib.jsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/mock-answer.d.tsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/mock-answer.jsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/mock-run.d.tsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/mock-run.jsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/mock-task.d.tsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/mock-task.jsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/mock-test.d.tsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/mock-test.jsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/mock-toolrunner.d.tsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/mock-toolrunner.jsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/task.d.tsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/task.jsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/taskcommand.d.tsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/taskcommand.jsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/toolrunner.d.tsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/toolrunner.jsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/typings.jsonis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/vault.d.tsis excluded by!**/node_modules/**tb-main/node_modules/vsts-task-lib/vault.jsis excluded by!**/node_modules/**tb-main/node_modules/wrappy/LICENSEis excluded by!**/node_modules/**tb-main/node_modules/wrappy/README.mdis excluded by!**/node_modules/**tb-main/node_modules/wrappy/package.jsonis excluded by!**/node_modules/**tb-main/node_modules/wrappy/wrappy.jsis excluded by!**/node_modules/**tb-main/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.gitignore.travis.ymlCLAUDE.mdGulpfile.jsMODERNIZATION.mdREADME.mdpackage.jsonscripts/install-task-deps.jsscripts/stamp-version.jstb-build-info/scripts/info.jstb-main/package.jsontb-main/task.jsontb-main/testingbot.jstb-stop-tunnel/sample.jstb-stop-tunnel/task.jsonwebpack.config.js
💤 Files with no reviewable changes (3)
- .travis.yml
- tb-build-info/scripts/info.js
- Gulpfile.js
- stamp-version.js: validate package.json version is exact numeric major.minor.patch (reject prerelease/malformed) and stamp numeric task versions; fail the build if a task manifest is missing instead of skipping it. - tb-stop-tunnel/sample.js: accept only "pid_<positive-int>" before process.kill (rejects pid_0, negatives that would signal a whole process group, and trailing garbage); handle ESRCH so an already-gone tunnel is a graceful no-op. - tb-main/testingbot.js: update the stale "min version 0.3.0" endpoint error to match minimumAgentVersion 3.232.1. - tb-main/package.json: declare engines.node >=20. - README.md: fix markdown lint (ordered-list markers, fenced-block spacing, bash language) MD029/MD031/MD040. - CLAUDE.md: document that only tb-main uses azure-pipelines-task-lib; the stop task reads TB_TUNNEL_PID directly and stays dependency-free. - MODERNIZATION.md: add a phase status banner and annotate the top-5 actions so the roadmap reflects what Phase 1/2 already shipped. Verified: npm run package produces the vsix; stop-tunnel guards exercised for empty/pid_0/pid_-1/garbage/dead-pid; version regex rejects bad values.
git add -A on the phase1 branch picked up index.js/*.map and the tests output left in the working tree after switching from the phase2 branch (phase1's .gitignore did not cover them). Phase 1 ships testingbot.js/sample.js, not the compiled TypeScript. Untrack them and add ignore rules to prevent recurrence.
jochen-testingbot
added a commit
that referenced
this pull request
Jul 17, 2026
Brings in the CodeRabbit fixes from PR #17 (stamp-version validation, README/CLAUDE/MODERNIZATION docs, engines.node) and applies the review points that also affect the Phase 2 TypeScript code: - tb-stop-tunnel is now genuinely dependency-free (honors the "stop task must not pull azure-pipelines-task-lib" guideline consistently): index.ts reads TB_TUNNEL_PID from process.env, validates the exact "pid_<positive-int>" shape (rejects pid_0 / negatives that would signal a process group / trailing garbage), handles ESRCH, and emits the raw ##vso[task.complete] command itself. Its package.json declares no dependencies, so nothing is installed into dist/tb-stop-tunnel (VSIX no longer ships task-lib twice). - install-task-deps.js / ensure-task-src-deps.js skip tasks with no deps. - Tests rewritten to spawn the compiled task directly (no task-lib): no-pid, malformed pids, ESRCH, and a real process-kill case. - engines.node >=20 on both task package.json files. Verified: npm test -> 8 passing; npm run package -> vsix with compiled index.js for both tasks, tunnel-launcher under tb-main only, tb-stop-tunnel with no node_modules, and no source .ts shipped.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tb-main/testingbot.js (1)
92-104: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftHandle tunnel spawn failures and early close as fatal.
spawn()can emiterrorwhen Java is missing, and the currentclosehandler exits with the child’s code even if the tunnel never reachedYou may start your tests, so an early0can look like success. Add anerrorlistener and only allow a clean exit after readiness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tb-main/testingbot.js` around lines 92 - 104, Update the tunnel_process lifecycle around spawn to listen for its error event and terminate with failure when Java or the tunnel cannot be started. Track whether the tunnel has reached the existing “You may start your tests” readiness signal, and make the close handler exit successfully only after readiness; otherwise treat early closure as fatal regardless of the child exit code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@MODERNIZATION.md`:
- Around line 5-15: Update the Phase 2 status in MODERNIZATION.md to PENDING,
since testingbot.js remains JavaScript and still launches 2.30.jar. Revise the
Phase 2 summary and related roadmap references to show the TypeScript,
testingbot-tunnel-launcher, and test migrations are unfinished, while preserving
the stated scope boundaries for the PR.
In `@scripts/stamp-version.js`:
- Around line 11-17: Update the version validation in the stamp-version flow
around the match and destructuring logic to require canonical numeric components
without leading zeros, and reject any component that is not a safe integer.
Validate the string segments before converting them with Number(), preserving
acceptance of ordinary major.minor.patch versions and rejection of oversized or
non-canonical values.
---
Outside diff comments:
In `@tb-main/testingbot.js`:
- Around line 92-104: Update the tunnel_process lifecycle around spawn to listen
for its error event and terminate with failure when Java or the tunnel cannot be
started. Track whether the tunnel has reached the existing “You may start your
tests” readiness signal, and make the close handler exit successfully only after
readiness; otherwise treat early closure as fatal regardless of the child exit
code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6882ca6b-fbef-470c-9b93-d2e22bece7be
📒 Files selected for processing (8)
.gitignoreCLAUDE.mdMODERNIZATION.mdREADME.mdscripts/stamp-version.jstb-main/package.jsontb-main/testingbot.jstb-stop-tunnel/sample.js
Comment on lines
+5
to
+15
| > **Status (living document).** The findings below describe the pre-modernization | ||
| > state as originally analyzed; sections are being resolved phase by phase. | ||
| > - **Phase 1 — DONE (PR #17):** clean-checkout packaging restored (npm scripts + | ||
| > webpack 5, gulp/Travis removed), both tasks on the `Node20_1` handler, | ||
| > `azure-pipelines-task-lib` 5 with committed `node_modules` deleted, version | ||
| > drift resolved via `scripts/stamp-version.js`, immediate correctness bugs fixed. | ||
| > - **Phase 2 — DONE (PR #18):** tasks rewritten in TypeScript, runtime tunnel via | ||
| > `testingbot-tunnel-launcher` (jar deleted), secret pipeline variables, tests. | ||
| > - **Phase 3 — PENDING:** results-tab / SDK rewrite, including moving the `/mini` | ||
| > URL signing server-side so the secret leaves the browser (finding #1). | ||
| > - **Phase 4 — PENDING:** CI/CD, lint, publishing. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep Phase 2 marked pending.
The document claims TypeScript tasks, testingbot-tunnel-launcher, and tests are complete, but tb-main/testingbot.js is still JavaScript and still spawns 2.30.jar at Lines 84-98. The PR objectives also explicitly leave these migrations out of scope. Align the status and roadmap so unfinished work is not treated as shipped.
Also applies to: 25-29, 73-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@MODERNIZATION.md` around lines 5 - 15, Update the Phase 2 status in
MODERNIZATION.md to PENDING, since testingbot.js remains JavaScript and still
launches 2.30.jar. Revise the Phase 2 summary and related roadmap references to
show the TypeScript, testingbot-tunnel-launcher, and test migrations are
unfinished, while preserving the stated scope boundaries for the PR.
Comment on lines
+11
to
+17
| // Require an exact numeric major.minor.patch — task.json versions must be | ||
| // integers and a malformed/prerelease value would silently produce a broken vsix. | ||
| const match = /^(\d+)\.(\d+)\.(\d+)$/.exec(version); | ||
| if (!match) { | ||
| throw new Error(`package.json version "${version}" is not a plain major.minor.patch version.`); | ||
| } | ||
| const [, major, minor, patch] = match.map(Number); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files | rg '^(scripts/stamp-version\.js|package\.json|README\.md|docs/|\.github/)' || true
printf '\n== script outline ==\n'
ast-grep outline scripts/stamp-version.js --view expanded || true
printf '\n== script lines ==\n'
wc -l scripts/stamp-version.js
cat -n scripts/stamp-version.js | sed -n '1,220p'
printf '\n== version guidance search ==\n'
rg -n "version.*source of truth|single source of truth|stamp-version|task\.json|major\.minor\.patch|safe integer|canonical" . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' || trueRepository: testingbot/vso-testingbot-plugin
Length of output: 7673
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n scripts/stamp-version.js | sed -n '1,120p'Repository: testingbot/vso-testingbot-plugin
Length of output: 2301
🌐 Web query:
semver numeric identifier leading zeros invalid major.minor.patch safe integer javascript Number precision package.json version
💡 Result:
According to the official Semantic Versioning (SemVer) 2.0.0 specification, numeric identifiers for major, minor, patch, and pre-release versions must not include leading zeros [1][2]. An exception is the single digit "0", which is permitted [1][3]. For example, "1.0.0" is valid, whereas "1.0.01" is invalid [4][5]. In practice, the npm ecosystem enforces this strictly. When managing package versions in package.json, including leading zeros in these identifiers will cause errors, such as failures during npm version commands or invalidation of the version string [5][6]. While some coercion utilities (like those in the semver npm package) can automatically strip leading zeros to make a version string valid [7][8], these should not be relied upon for maintaining your package.json. Regarding JavaScript precision, the semver package for Node.js handles numeric components by attempting to parse them into JavaScript Numbers [9]. Because JavaScript uses IEEE-754 double-precision floats, it can only represent integers exactly up to Number.MAX_SAFE_INTEGER (2^53 - 1) [10][11]. To avoid precision loss when dealing with extremely large numeric identifiers (those exceeding this limit), the semver package has evolved to handle these values as strings or via BigInt comparisons, ensuring that version comparison logic remains accurate even when identifiers exceed safe integer limits [9]. However, as a best practice, version identifiers should remain within reasonable numerical bounds to ensure broad compatibility and adherence to the SemVer spec [1][12].
Citations:
- 1: https://semver.org/
- 2: https://github.com/semver/semver/blob/d58db1686379c8c6d52e32d42d3a530a964264e5/semver.md
- 3: May prerelease-version contain leading zeros? semver/semver#583
- 4: [Clarification request] spec-item-9 "Numeric identifiers MUST NOT include leading zeroes." and example
1.0.0-0.3.7semver/semver#1084 - 5: https://stackoverflow.com/questions/45698640/npm-cant-handle-leading-0s-in-version-npm-version-patch-not-being-recogn
- 6: Npm version does not allow leading 0 after any dots in version preid npm/npm#19981
- 7: Add leading zeros remove npm/node-semver#233
- 8: https://www.npmjs.com/package/semver
- 9: fix: compare large numeric prerelease ids without precision loss npm/node-semver#880
- 10: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER
- 11: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isSafeInteger
- 12: https://github.com/npm/node-semver
Reject non-canonical or oversized version components. ^(\d+)\.(\d+)\.(\d+)$ still accepts invalid SemVer like 01.2.3, and Number() can round values above Number.MAX_SAFE_INTEGER before they’re written into task.json. Enforce canonical numeric segments and safe integers here.
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 13-13: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/stamp-version.js` around lines 11 - 17, Update the version validation
in the stamp-version flow around the match and destructuring logic to require
canonical numeric components without leading zeros, and reject any component
that is not a safe integer. Validate the string segments before converting them
with Number(), preserving acceptance of ordinary major.minor.patch versions and
rejection of oversized or non-canonical values.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
From a clean checkout the repo cannot build a
.vsix: Dependabot bumpedgulpto^4andwebpack-streamto^7(webpack 5) without migrating the gulp 3 / webpack 1 / babel 6 code, sogulp,npm run package, and every CI path fail. On top of that, both pipeline tasks declare the removed Node 6"Node"execution handler (removed from new agents Nov 2026) and run against a vendored 2016-eravsts-task-libcommitted into the repo.This is Phase 1 of the roadmap in
MODERNIZATION.md: make it build again and get the tasks onto a supported runtime. Credential hygiene, the tunnel-launcher swap, TypeScript, and the web-tab/SDK rewrite are Phases 2–4 and intentionally out of scope here.What changed
Build system (1.1–1.2, 1.6)
Gulpfile.js+.travis.yml; replace with npm scripts + webpack 5 (webpack.config.js) — AMD bundle of the results-tab scripts +copy-webpack-pluginfor static files.scripts/install-task-deps.jsinstalls each task's productionnode_modulesintodist/;scripts/stamp-version.jsstampspackage.json's version into the built manifest +task.jsons (resolves the 0.2.7 / 0.1.0 / 0.2.0 drift —package.jsonis now the single source of truth).tfx-cli^0.12→^0.23.4; drop the babel-6 / gulp / webpack-stream toolchain.Tasks (1.3–1.4)
task.jsonhandlersNode→Node20_1,minimumAgentVersion→3.232.1.tb-main/package.json(azure-pipelines-task-lib@^5.277.0); delete the 210 committed vendorednode_modulesfiles;require('azure-pipelines-task-lib/task').Correctness fixes (1.5)
tb-stop-tunnel: guardTB_TUNNEL_PIDbefore use + validate the pid — an unstarted tunnel no longer crashes with aTypeError.tb-main: null-check endpoint auth; fix the copy-pasted "SonarQube" error message;tl.getBoolInputfortbTunnel; fix thetunnel_binvariable shadowing (jar path vs ChildProcess);RELEASE_*fallback forTB_BUILD_NAME.task.json:connectedServiceNamerequired: true; remove the$(message)ghost input frominstanceNameFormat.Verification
npm run package→Packages/testingbot.testingbot-tasks-0.2.7.vsix(VSIX contains AMD-bundledinfo.js/dialog.js,azure-pipelines-task-libunderdist/tb-main/node_modules, and versions stamped to 0.2.7 across the manifest + both tasks).tb-mainno-tunnel happy path (writes the attachment, exits 0),tb-stop-tunnelno-pid and garbage-pid guards (graceful exit 0), and theRELEASE_*fallback (TB_BUILD_NAME=My_Release_99).Not in scope (later phases)
Plaintext-credential attachment + browser-side secret use, the bundled 2019 tunnel jar, the unpkg.com CDN script, the
vss-web-extension-sdk→azure-devops-extension-sdkport, CI/publishing automation, lint/tests. SeeMODERNIZATION.md.Summary by CodeRabbit
npm run package, including consistent version stamping.