Phase 2: TypeScript tasks, runtime tunnel download, credential hygiene#18
Merged
Conversation
Rebuilds both agent tasks as TypeScript on azure-pipelines-task-lib 5, replaces the bundled 2019 tunnel jar with runtime download, hardens credential handling, and adds a mocha test suite. Stacked on the Phase 1 build restore. Tasks (2.1) - Rewrite tb-main and tb-stop-tunnel as index.ts (tsc -> index.js in place), strict TypeScript, typed task-lib 5 API, tl.setResult for pass/fail. - Build pipeline gains a `compile` step (tsc per task) before webpack; webpack copy now ignores *.ts/tests/tsconfig and ships the compiled index.js. Tunnel (2.2, 2.5) - Replace tb-main/tunnel/2.30.jar (Jan 2019, vulnerable Jetty) + the raw `java -jar` spawn with testingbot-tunnel-launcher ^1.1.18: it downloads and caches the current tunnel, checks Java >= 11, and resolves only when the tunnel is ready. A pre-ready exit / bad creds / no minutes now fails the task. - New inputs mirroring testingbot-tunnel-action: tunnelIdentifier (defaults to azure-<BuildId>-<attempt>), sePort, proxy, readyTimeout, sharedTunnel, noBump, noCache. tbTunnelOptions kept but deprecated (warns). VSIX shrinks ~3.2 MB. - Cross-task stop still works via the stored pid (the launcher's in-process killAsync can't reach another task's tunnel); the stop task keeps the Phase 1 guard + ESRCH handling. Credentials (2.3) - tl.setSecret(secret) + export TB_SECRET/TESTINGBOT_SECRET as secret variables (masked in logs). Key stays a normal variable. Task metadata (2.6) - runsOn Agent/DeploymentGroup, outputVariables, restrictions (commands: restricted + settableVariables allowlist), tunnel input group, real instanceNameFormat, index.js execution target. Tests (2.7) - mocha + azure-pipelines-task-lib mock-run/mock-test. tb-main: no-tunnel path registers the secret, sets it as a secret variable, writes the attachment. tb-stop-tunnel: no-pid and garbage-pid guards. `npm test` -> 3 passing. /mini viewer (per maintainer): the results tab keeps the embedded TestingBot /mini video/logs viewer, which needs the key/secret to sign the share URL. Those credentials therefore still travel in the build attachment. Removing them from the browser without losing /mini requires signing the URL server-side via a TestingBot API data source -- deferred to the Phase 3 tab redesign. The tab is untouched in this PR. Verified: `npm test` (3 passing), `npm run package` produces testingbot.testingbot-tasks-0.2.7.vsix containing compiled index.js + the launcher/task-lib node_modules and no jar; a real (non-mock) run of the compiled tb-main no-tunnel path succeeds with the secret masked and the attachment intact. Residual (noted for later): the launcher passes key/secret as java argv internally (finding #9, Low-Med, shared self-hosted agents); the attachment credential exposure (finding #1) is resolved in Phase 3.
📝 WalkthroughWalkthroughThe pull request migrates TestingBot Azure Pipelines tasks to TypeScript entrypoints, adds compile/test packaging workflows, expands tunnel configuration, introduces runtime tunnel launching, and adds automated tests for configuration and tunnel shutdown behavior. ChangesTestingBot task modernization
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant AzurePipelines
participant TBMain
participant TunnelLauncher
participant BuildAttachment
AzurePipelines->>TBMain: Provide inputs and endpoint authorization
TBMain->>TBMain: Resolve credentials and export variables
TBMain->>TunnelLauncher: Start tunnel with tunnel options
TunnelLauncher-->>TBMain: Return tunnel PID and identifier
TBMain->>BuildAttachment: Write and attach testingbot.json
TBMain-->>AzurePipelines: Report task completion
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
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.
Restore tb-build-info/scripts/info.js to match master; a trailing-whitespace change had slipped in during the earlier revert. The tab is Phase 3 scope.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 49-66: Update the modernization roadmap sections around the Phase
1 status to reflect that the TypeScript task entrypoints, runtime tunnel
downloads, and secret handling described by TBMain and TBStopTunnel are already
implemented. Remove or revise obsolete “pending” guidance while preserving the
roadmap’s remaining accurate phases and findings.
In `@scripts/ensure-task-src-deps.js`:
- Line 20: Update the dependency-check logic around the node_modules guard in
the task dependency setup flow so it validates installed dependencies against
the current package.json and lockfile, rather than only checking directory
existence. Reinstall or refresh dependencies when the installed metadata is
missing or stale, while preserving the skip behavior when the installed state
matches the current manifests.
In `@tb-main/index.ts`:
- Around line 89-100: Validate the parsed sePort and readyTimeout values before
assigning them to options or launching the tunnel: reject non-numeric or
partially parsed inputs, require sePort to be an integer from 1 through 65535,
and require readyTimeout to be a positive number. Preserve the existing
optional-input behavior and proxy handling in the surrounding tunnel setup flow.
- Around line 175-184: Reorder the success flow so writeAttachment(credentials)
completes before startTunnel(credentials) is called, ensuring attachment
failures occur before the tunnel is detached. Preserve the existing tunnel
condition, success result, explicit exit, and failure handling.
In `@tb-stop-tunnel/tests/_suite.ts`:
- Around line 45-61: Update the “actually kills a running process” test around
runStop and its delayed liveness check to always clean up the spawned child. Add
error handling for spawn/test failures and ensure the child is terminated in a
finally-style cleanup path, while preserving the existing assertions and done
callback behavior.
🪄 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: 1408393d-b2f5-4e1c-8232-387bb07f522b
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsontb-main/package-lock.jsonis excluded by!**/package-lock.jsontb-main/tunnel/.DS_Storeis excluded by!**/.DS_Storetb-main/tunnel/2.30.jaris excluded by!**/*.jar
📒 Files selected for processing (19)
.gitignoreCLAUDE.mdpackage.jsonscripts/ensure-task-src-deps.jsscripts/install-task-deps.jstb-main/index.tstb-main/package.jsontb-main/task.jsontb-main/testingbot.jstb-main/tests/_suite.tstb-main/tests/no-tunnel.tstb-main/tsconfig.jsontb-stop-tunnel/index.tstb-stop-tunnel/package.jsontb-stop-tunnel/sample.jstb-stop-tunnel/task.jsontb-stop-tunnel/tests/_suite.tstb-stop-tunnel/tsconfig.jsonwebpack.config.js
💤 Files with no reviewable changes (2)
- tb-stop-tunnel/sample.js
- tb-main/testingbot.js
Comment on lines
+49
to
+66
| - **`tb-main/`** — Pipeline task "TestingBot Configuration" (`TBMain`). TypeScript | ||
| task (`index.ts`) that reads a TestingBot service-endpoint credential, exports | ||
| `TB_KEY`/`TB_SECRET`/`SELENIUM_HOST`/etc. as pipeline variables (the secret ones | ||
| via `setSecret` + a secret variable) so downstream test steps can reach the | ||
| grid, and optionally launches **TestingBot Tunnel** via the | ||
| `testingbot-tunnel-launcher` npm package (which downloads/caches the current | ||
| tunnel jar at runtime — nothing is bundled). It writes `testingbot.json` and | ||
| attaches it to the build via `task.addattachment` with type | ||
| `TestingBotBuildResult` — this attachment is how the results tab (below) later | ||
| finds the build's credentials and build name. The credentials still travel in | ||
| that attachment because the tab needs them to sign TestingBot `/mini` share | ||
| URLs; removing them from the browser (finding #1) is the Phase 3 tab redesign. | ||
|
|
||
| - **`tb-stop-tunnel/`** — Pipeline task "Stop TestingBot Tunnel" (`TBStopTunnel`). | ||
| Kills the tunnel process using the PID that `tb-main` stored in the | ||
| `TB_TUNNEL_PID` variable. Add this task after your test steps to tear the tunnel | ||
| down. | ||
| TypeScript task (`index.ts`) that kills the tunnel process using the PID that | ||
| `tb-main` stored in the `TB_TUNNEL_PID` variable (the launcher's in-process | ||
| `killAsync` can't reach a tunnel started by a different task). Add this task | ||
| after your test steps to tear the tunnel down. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale modernization roadmap.
These sections describe TypeScript task entrypoints, runtime tunnel downloads, and secret handling as implemented, but lines 121-123 still say this is Phase 1 and those capabilities are pending. Update the roadmap so contributors do not follow obsolete project guidance.
Also applies to: 85-98
🤖 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 `@CLAUDE.md` around lines 49 - 66, Update the modernization roadmap sections
around the Phase 1 status to reflect that the TypeScript task entrypoints,
runtime tunnel downloads, and secret handling described by TBMain and
TBStopTunnel are already implemented. Remove or revise obsolete “pending”
guidance while preserving the roadmap’s remaining accurate phases and findings.
| const taskDir = path.join(root, entry); | ||
| const pkgPath = path.join(taskDir, 'package.json'); | ||
| if (!fs.existsSync(pkgPath)) continue; | ||
| if (fs.existsSync(path.join(taskDir, 'node_modules'))) continue; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the installed dependency state, not only the directory’s existence.
Line 20 skips installation whenever node_modules exists, including after an interrupted install or when package.json/the lockfile has changed. That can leave tsc resolving stale or missing declarations and make npm run compile fail based on workspace history. Track the installed manifest/lockfile or refresh dependencies when metadata is out of date.
🤖 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/ensure-task-src-deps.js` at line 20, Update the dependency-check
logic around the node_modules guard in the task dependency setup flow so it
validates installed dependencies against the current package.json and lockfile,
rather than only checking directory existence. Reinstall or refresh dependencies
when the installed metadata is missing or stale, while preserving the skip
behavior when the installed state matches the current manifests.
Comment on lines
+89
to
+100
| const sePort = tl.getInput('sePort'); | ||
| if (sePort) { | ||
| options['se-port'] = parseInt(sePort, 10); | ||
| } | ||
| const proxy = tl.getInput('proxy'); | ||
| if (proxy) { | ||
| options.proxy = proxy; | ||
| } | ||
| const readyTimeout = tl.getInput('readyTimeout'); | ||
| if (readyTimeout) { | ||
| options.timeout = parseInt(readyTimeout, 10); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate numeric tunnel inputs before launching.
parseInt accepts values such as 12abc and produces NaN for non-numeric input. Reject invalid ports/timeouts explicitly, including ports outside 1–65535 and non-positive timeouts.
Proposed fix
const sePort = tl.getInput('sePort');
if (sePort) {
- options['se-port'] = parseInt(sePort, 10);
+ const port = Number(sePort);
+ if (!Number.isInteger(port) || port < 1 || port > 65535) {
+ throw new Error('Selenium relay port must be an integer from 1 to 65535.');
+ }
+ options['se-port'] = port;
}
@@
const readyTimeout = tl.getInput('readyTimeout');
if (readyTimeout) {
- options.timeout = parseInt(readyTimeout, 10);
+ const timeout = Number(readyTimeout);
+ if (!Number.isInteger(timeout) || timeout < 1) {
+ throw new Error('Ready timeout must be a positive integer.');
+ }
+ options.timeout = timeout;
}📝 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.
Suggested change
| const sePort = tl.getInput('sePort'); | |
| if (sePort) { | |
| options['se-port'] = parseInt(sePort, 10); | |
| } | |
| const proxy = tl.getInput('proxy'); | |
| if (proxy) { | |
| options.proxy = proxy; | |
| } | |
| const readyTimeout = tl.getInput('readyTimeout'); | |
| if (readyTimeout) { | |
| options.timeout = parseInt(readyTimeout, 10); | |
| } | |
| const sePort = tl.getInput('sePort'); | |
| if (sePort) { | |
| const port = Number(sePort); | |
| if (!Number.isInteger(port) || port < 1 || port > 65535) { | |
| throw new Error('Selenium relay port must be an integer from 1 to 65535.'); | |
| } | |
| options['se-port'] = port; | |
| } | |
| const proxy = tl.getInput('proxy'); | |
| if (proxy) { | |
| options.proxy = proxy; | |
| } | |
| const readyTimeout = tl.getInput('readyTimeout'); | |
| if (readyTimeout) { | |
| const timeout = Number(readyTimeout); | |
| if (!Number.isInteger(timeout) || timeout < 1) { | |
| throw new Error('Ready timeout must be a positive integer.'); | |
| } | |
| options.timeout = timeout; | |
| } |
🤖 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/index.ts` around lines 89 - 100, Validate the parsed sePort and
readyTimeout values before assigning them to options or launching the tunnel:
reject non-numeric or partially parsed inputs, require sePort to be an integer
from 1 through 65535, and require readyTimeout to be a positive number. Preserve
the existing optional-input behavior and proxy handling in the surrounding
tunnel setup flow.
Comment on lines
+175
to
+184
| if (tl.getBoolInput('tbTunnel', false)) { | ||
| await startTunnel(credentials); | ||
| } | ||
|
|
||
| writeAttachment(credentials); | ||
| tl.setResult(tl.TaskResult.Succeeded, 'TestingBot configured.'); | ||
| // Exit explicitly: a running tunnel child keeps the event loop alive. | ||
| process.exit(0); | ||
| } catch (err) { | ||
| tl.setResult(tl.TaskResult.Failed, err instanceof Error ? err.message : String(err)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent a detached tunnel from surviving task failure.
startTunnel detaches the child before writeAttachment can throw. That failure marks the task failed but leaves the tunnel running, and a normally conditioned stop task may then be skipped. Write the attachment before starting the tunnel or retain and terminate the child in the catch path.
Minimal fix
const credentials = getEndpointDetails('connectedServiceName');
exportVariables(credentials);
+ writeAttachment(credentials);
if (tl.getBoolInput('tbTunnel', false)) {
await startTunnel(credentials);
}
- writeAttachment(credentials);
tl.setResult(tl.TaskResult.Succeeded, 'TestingBot configured.');📝 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.
Suggested change
| if (tl.getBoolInput('tbTunnel', false)) { | |
| await startTunnel(credentials); | |
| } | |
| writeAttachment(credentials); | |
| tl.setResult(tl.TaskResult.Succeeded, 'TestingBot configured.'); | |
| // Exit explicitly: a running tunnel child keeps the event loop alive. | |
| process.exit(0); | |
| } catch (err) { | |
| tl.setResult(tl.TaskResult.Failed, err instanceof Error ? err.message : String(err)); | |
| const credentials = getEndpointDetails('connectedServiceName'); | |
| exportVariables(credentials); | |
| writeAttachment(credentials); | |
| if (tl.getBoolInput('tbTunnel', false)) { | |
| await startTunnel(credentials); | |
| } | |
| tl.setResult(tl.TaskResult.Succeeded, 'TestingBot configured.'); | |
| // Exit explicitly: a running tunnel child keeps the event loop alive. | |
| process.exit(0); | |
| } catch (err) { | |
| tl.setResult(tl.TaskResult.Failed, err instanceof Error ? err.message : String(err)); |
🤖 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/index.ts` around lines 175 - 184, Reorder the success flow so
writeAttachment(credentials) completes before startTunnel(credentials) is
called, ensuring attachment failures occur before the tunnel is detached.
Preserve the existing tunnel condition, success result, explicit exit, and
failure handling.
Comment on lines
+45
to
+61
| it('actually kills a running process', function (done) { | ||
| const child = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000)'], { stdio: 'ignore' }); | ||
| child.on('spawn', () => { | ||
| const { status } = runStop(`pid_${child.pid}`); | ||
| assert.strictEqual(status, 0); | ||
| // Give the signal a moment, then confirm the process is gone. | ||
| setTimeout(() => { | ||
| let alive = true; | ||
| try { | ||
| process.kill(child.pid as number, 0); | ||
| } catch (err) { | ||
| alive = (err as NodeJS.ErrnoException).code !== 'ESRCH'; | ||
| } | ||
| assert.strictEqual(alive, false, 'child process should have been killed'); | ||
| done(); | ||
| }, 200); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Always clean up the spawned test process.
If runStop, an assertion, or the delayed liveness check fails, the interval child can remain alive and keep CI running. Add an error handler and terminate the child from a finally/cleanup path.
🤖 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-stop-tunnel/tests/_suite.ts` around lines 45 - 61, Update the “actually
kills a running process” test around runStop and its delayed liveness check to
always clean up the spawned child. Add error handling for spawn/test failures
and ensure the child is terminated in a finally-style cleanup path, while
preserving the existing assertions and done callback behavior.
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.
Phase 2 of
MODERNIZATION.md: modernize the two agent tasks.What changed
TypeScript rewrite (2.1)
tb-mainandtb-stop-tunnelare nowindex.ts(strict TS, typedazure-pipelines-task-lib5 API,tl.setResult), compiled toindex.jsbytsc. The build gains acompilestep before webpack; the copy step ignores*.ts/tests/tsconfigand ships the compiled JS.Runtime tunnel (2.2, 2.5)
tb-main/tunnel/2.30.jar(Jan 2019, vulnerable Jetty) and the rawjava -jarspawn.tb-mainnow usestestingbot-tunnel-launcher@^1.1.18, which downloads/caches the current tunnel, checks Java ≥ 11, and resolves only when the tunnel is ready — so a pre-ready exit, bad credentials or an exhausted account now fails the task. VSIX shrinks ~3.2 MB.testingbot-tunnel-action:tunnelIdentifier(defaultazure-<BuildId>-<attempt>),sePort,proxy,readyTimeout,sharedTunnel,noBump,noCache.tbTunnelOptionskept but deprecated (warns).killAsynccan't reach another task's tunnel); the stop task keeps the Phase 1 guard +ESRCHhandling.Credential hygiene (2.3)
tl.setSecret(secret)+TB_SECRET/TESTINGBOT_SECRETexported as secret variables (masked in logs). The key stays a normal variable.Task metadata (2.6)
runsOn,outputVariables,restrictions(commands: restricted+settableVariablesallowlist), a tunnel input group, realinstanceNameFormat,index.jstarget.Tests (2.7)
mock-run/mock-test.tb-mainno-tunnel path (registers secret, sets secret variable, writes attachment) andtb-stop-tunnelno-pid/garbage-pid guards.npm test→ 3 passing./miniviewer — keptPer maintainer feedback, the results tab keeps the embedded TestingBot
/minivideo/logs viewer, which the developer sees inside the build UI. Signing that share URL needsmd5(key:secret:session_id), so the key/secret still travel in the build attachment and the tab is untouched here. Removing them from the browser without losing/minimeans signing the URL server-side via a TestingBot API data source — that's the Phase 3 tab redesign. This PR does not regress the DX.Verification
npm test→ 3 passing.npm run package→testingbot.testingbot-tasks-0.2.7.vsixcontaining the compiledindex.jsfor both tasks +testingbot-tunnel-launcher/azure-pipelines-task-libindist/tb-*/node_modules, no jar, no.ts/tests.tb-mainno-tunnel path succeeds: secret masked, attachment carries the creds/minineeds.Residuals (later)
/minisigning).Summary by CodeRabbit
New Features
Bug Fixes
Documentation