From 9eaaf2dd32ab51dbb9d7f0e124d3313802e2b469 Mon Sep 17 00:00:00 2001 From: Jesse Houwing Date: Sun, 22 Feb 2026 23:12:39 +0100 Subject: [PATCH 1/4] Introduces Validate command and adds line/col/file data to validation warnings/errors. --- .../_lib/extension-composer-factory.ts | 17 +- app/exec/extension/_lib/interfaces.ts | 18 +- app/exec/extension/_lib/merger.ts | 53 +++- app/exec/extension/create.ts | 8 +- app/exec/extension/default.ts | 13 +- app/exec/extension/publish.ts | 1 + app/exec/extension/validate.ts | 113 ++++++++ app/lib/jsonvalidate.ts | 87 ++++-- docs/extensions.md | 36 ++- package.json | 1 + tests/extension-local-tests.ts | 266 +++++++++++++++--- 11 files changed, 535 insertions(+), 78 deletions(-) create mode 100644 app/exec/extension/validate.ts diff --git a/app/exec/extension/_lib/extension-composer-factory.ts b/app/exec/extension/_lib/extension-composer-factory.ts index 6fb7ea0e..35d7a837 100644 --- a/app/exec/extension/_lib/extension-composer-factory.ts +++ b/app/exec/extension/_lib/extension-composer-factory.ts @@ -34,11 +34,20 @@ export class ComposerFactory { break; default: if (!settings.bypassValidation) { - throw new Error( + const message = "'" + - target.id + - "' is not a recognized target. Valid targets are 'Microsoft.VisualStudio.Services', 'Microsoft.VisualStudio.Services.Integration', 'Microsoft.VisualStudio.Offer'", - ); + target.id + + "' is not a recognized target. Valid targets are 'Microsoft.VisualStudio.Services', 'Microsoft.VisualStudio.Services.Integration', 'Microsoft.VisualStudio.Offer'"; + const err: any = new Error(message); + err.validationIssues = [ + { + file: null, + line: null, + col: null, + message: message, + }, + ]; + throw err; } break; } diff --git a/app/exec/extension/_lib/interfaces.ts b/app/exec/extension/_lib/interfaces.ts index eff30f3c..c5a1f0c1 100644 --- a/app/exec/extension/_lib/interfaces.ts +++ b/app/exec/extension/_lib/interfaces.ts @@ -136,8 +136,8 @@ export interface MergeSettings { root: string; /* - * Manifest in the form of a standard Node.js CommonJS module with an exported function. - * The function takes an environment as a parameter and must return the manifest JSON object. + * Manifest in the form of a standard Node.js CommonJS module with an exported function. + * The function takes an environment as a parameter and must return the manifest JSON object. * Environment variables are specified with the env command line parameter. * If this is present then manifests and manifestGlobs are ignored. */ @@ -183,6 +183,11 @@ export interface MergeSettings { * which supports comments, unquoted keys, etc. */ json5: boolean; + + /** + * If true, task.json validation warnings are treated as errors. + */ + warningsAsErrors?: boolean; } export interface PackageSettings { @@ -239,6 +244,15 @@ export interface PublishSettings { bypassScopeCheck?: boolean; } +/** + * Common extension identity/version metadata. + */ +export interface ExtensionVersionInfo { + extensionId: string; + version: string; + publisher: string; +} + /*** Types related to localized resources ***/ export interface ResourcesFile { diff --git a/app/exec/extension/_lib/merger.ts b/app/exec/extension/_lib/merger.ts index 23d0bda0..c34f8b1a 100644 --- a/app/exec/extension/_lib/merger.ts +++ b/app/exec/extension/_lib/merger.ts @@ -281,10 +281,17 @@ export class Merger { if (validationResult.length === 0 || this.settings.bypassValidation) { return components; } else { - throw new Error( + const validationErr: any = new Error( "There were errors with your extension. Address the following and re-run the tool.\n" + validationResult, ); + validationErr.validationIssues = validationResult.map(message => ({ + file: null, + line: null, + col: null, + message: message, + })); + throw validationErr; } }); }); @@ -397,6 +404,12 @@ export class Merger { } private async validateBuildTaskContributions(contributions: any[]): Promise { + const warnings: string[] = []; + const addWarning = (message: string) => { + warnings.push(message); + trace.warn(message); + }; + try { // Filter contributions to only build tasks const buildTaskContributions = contributions.filter(contrib => @@ -440,22 +453,24 @@ export class Merger { } } } catch (err) { - trace.warn(`Error reading task directory ${absoluteTaskPath}: ${err}`); + addWarning(`Error reading task directory ${absoluteTaskPath}: ${err}`); } } // Validate task.json files for this contribution with backwards compatibility checking if (contributionTaskJsonPaths.length > 0) { trace.debug(`Validating ${contributionTaskJsonPaths.length} task.json files for contribution ${contrib.id || taskPath}`); - + for (const taskJsonPath of contributionTaskJsonPaths) { validate(taskJsonPath, "no task.json in specified directory", contributionTaskJsonPaths); } - + // Also collect for global tracking if needed allTaskJsonPaths.push(...contributionTaskJsonPaths); } else { - trace.warn(`Build task contribution '${contrib.id || taskPath}' does not have a task.json file. Expected task.json in ${absoluteTaskPath} or its subdirectories.`); + addWarning( + `Build task contribution '${contrib.id || taskPath}' does not have a task.json file. Expected task.json in ${absoluteTaskPath} or its subdirectories.`, + ); } } @@ -466,11 +481,33 @@ export class Merger { trace.debug(`Successfully validated ${allTaskJsonPaths.length} task.json files across ${buildTaskContributions.length} build task contributions`); + if (this.settings.warningsAsErrors && warnings.length > 0) { + const warningsAsErrorsErr: any = new Error( + "Task.json validation produced warnings. Re-run without --warnings-as-errors to treat them as warnings only.\n" + + warnings.join("\n"), + ); + warningsAsErrorsErr.validationIssues = warnings.map(message => ({ + file: null, + line: null, + col: null, + message: message, + })); + throw warningsAsErrorsErr; + } + } catch (err) { const warningMessage = "Please, make sure the task.json file is correct. In the future, this warning will be treated as an error.\n"; - trace.warn(err && err instanceof Error - ? warningMessage + err.message - : `Error occurred while validating build task contributions. ${warningMessage}`); + if (this.settings.warningsAsErrors) { + if (err && err instanceof Error) { + throw err; + } + throw new Error("Error occurred while validating build task contributions."); + } + trace.warn( + err && err instanceof Error + ? warningMessage + err.message + : `Error occurred while validating build task contributions. ${warningMessage}`, + ); } } } diff --git a/app/exec/extension/create.ts b/app/exec/extension/create.ts index 84b86334..4f6dae30 100644 --- a/app/exec/extension/create.ts +++ b/app/exec/extension/create.ts @@ -1,6 +1,6 @@ import { Merger } from "./_lib/merger"; import { VsixManifestBuilder } from "./_lib/vsix-manifest-builder"; -import { MergeSettings, PackageSettings } from "./_lib/interfaces"; +import { ExtensionVersionInfo, MergeSettings, PackageSettings } from "./_lib/interfaces"; import { VsixWriter } from "./_lib/vsix-writer"; import { TfCommand } from "../../lib/tfcommand"; import colors = require("colors"); @@ -11,11 +11,8 @@ export function getCommand(args: string[]): TfCommand { @@ -56,6 +53,7 @@ export class ExtensionCreate extends extBase.ExtensionBase { "overridesFile", "revVersion", "bypassValidation", + "warningsAsErrors", "publisher", "extensionId", "outputPath", diff --git a/app/exec/extension/default.ts b/app/exec/extension/default.ts index 3d1ff135..d303b6a0 100644 --- a/app/exec/extension/default.ts +++ b/app/exec/extension/default.ts @@ -50,6 +50,7 @@ export interface ExtensionArguments extends CoreArguments { unshareWith: args.ArrayArgument; version: args.StringArgument; vsix: args.ReadableFilePathsArgument; + warningsAsErrors: args.BooleanArgument; zipUri: args.StringArgument; } @@ -133,7 +134,7 @@ export class ExtensionBase extends TfCommand { "shareWith", "Share with", "List of Azure DevOps organization(s) with which to share the extension (space separated).", - args.ArrayArgument, + args.ArrayArgument, null, ); this.registerCommandArgument( @@ -151,6 +152,13 @@ export class ExtensionBase extends TfCommand { ); this.registerCommandArgument("bypassScopeCheck", "Bypass scope check", null, args.BooleanArgument, "false"); this.registerCommandArgument("bypassValidation", "Bypass local validation", null, args.BooleanArgument, "false"); + this.registerCommandArgument( + "warningsAsErrors", + "Warnings as errors", + "Treat task.json validation warnings as errors.", + args.BooleanArgument, + "false", + ); this.registerCommandArgument( "locRoot", "Localization root", @@ -196,6 +204,7 @@ export class ExtensionBase extends TfCommand { this.commandArgs.overridesFile.val(), this.commandArgs.revVersion.val(), this.commandArgs.bypassValidation.val(), + this.commandArgs.warningsAsErrors.val(), this.commandArgs.publisher.val(true), this.commandArgs.extensionId.val(true), this.commandArgs.json5.val(true), @@ -211,6 +220,7 @@ export class ExtensionBase extends TfCommand { overridesFile, revVersion, bypassValidation, + warningsAsErrors, publisher, extensionId, json5, @@ -251,6 +261,7 @@ export class ExtensionBase extends TfCommand { manifestGlobs: manifestGlob, overrides: mergedOverrides, bypassValidation: bypassValidation, + warningsAsErrors: warningsAsErrors, revVersion: revVersion, json5: json5, }; diff --git a/app/exec/extension/publish.ts b/app/exec/extension/publish.ts index f499dca9..dea132af 100644 --- a/app/exec/extension/publish.ts +++ b/app/exec/extension/publish.ts @@ -48,6 +48,7 @@ export class ExtensionPublish extends extBase.ExtensionBase { + return new ExtensionValidate(args); +} + +export interface ValidationIssue { + file: string | null; + line: number | null; + col: number | null; + message: string; +} + +export interface ValidationResult { + source: "manifest"; + status: "success" | "error"; + issues: ValidationIssue[]; +} + +export class ExtensionValidate extends extBase.ExtensionBase { + protected description = + "Validate an extension from manifests without packaging or publishing."; + protected serverCommand = false; + + constructor(passedArgs: string[]) { + super(passedArgs); + } + + protected getHelpArgs(): string[] { + return [ + "root", + "manifestJs", + "env", + "manifests", + "manifestGlobs", + "json5", + "override", + "overridesFile", + "warningsAsErrors", + "publisher", + "extensionId", + "locRoot", + ]; + } + + public async exec(): Promise { + const vsix = await this.commandArgs.vsix.val(true); + if (vsix && vsix.length > 0) { + throw new Error("The --vsix argument is not supported by 'tfx extension validate'. Validate command only supports manifest-based local validation."); + } + + return this.validateManifestInputs(); + } + + private async validateManifestInputs(): Promise { + const mergeSettings = await this.getMergeSettings(); + + // Validation command should not mutate version nor bypass validation checks. + mergeSettings.revVersion = false; + mergeSettings.bypassValidation = false; + + try { + await new Merger(mergeSettings).merge(); + + return { + source: "manifest", + status: "success", + issues: [], + }; + } catch (err) { + return { + source: "manifest", + status: "error", + issues: this.errorToIssues(err), + }; + } + } + + private errorToIssues(err: any): ValidationIssue[] { + const sourceIssues = err && Array.isArray(err.validationIssues) ? err.validationIssues : null; + if (sourceIssues && sourceIssues.length > 0) { + return sourceIssues.map(issue => ({ + file: issue && issue.file !== undefined ? issue.file : null, + line: issue && issue.line !== undefined ? issue.line : null, + col: issue && issue.col !== undefined ? issue.col : null, + message: issue && issue.message ? issue.message : String(issue), + })); + } + + const message = err && err.message ? err.message : String(err); + return [{ file: null, line: null, col: null, message: message }]; + } + + protected friendlyOutput(data: ValidationResult): void { + if (data.status === "success") { + trace.info(colors.green("\n=== Completed operation: validate extension ===")); + trace.info(" - Source: %s", data.source); + trace.info(" - Validation: %s", colors.green("success")); + } else { + trace.info(colors.red("\n=== Completed operation: validate extension ===")); + trace.info(" - Source: %s", data.source); + trace.info(" - Validation: %s", colors.red("failed")); + trace.info(" - Issues (%s):", data.issues.length.toString()); + data.issues.forEach(issue => { + trace.info(" - %s", issue.message); + }); + } + } +} diff --git a/app/lib/jsonvalidate.ts b/app/lib/jsonvalidate.ts index b74aa10d..d5357ced 100644 --- a/app/lib/jsonvalidate.ts +++ b/app/lib/jsonvalidate.ts @@ -2,9 +2,17 @@ var fs = require("fs"); import * as path from 'path'; var check = require("validator"); var trace = require("./trace"); +const jsonSourceMap = require("json-source-map"); const deprecatedRunners = ["Node", "Node6", "Node10", "Node16"]; +export interface StructuredValidationIssue { + file: string | null; + line: number | null; + col: number | null; + message: string; +} + export interface TaskJson { id: string; } @@ -22,22 +30,31 @@ export function validate(jsonFilePath: string, jsonMissingErrorMessage?: string, var jsonMissingErrorMsg: string = jsonMissingErrorMessage || "specified json file does not exist."; exists(jsonFilePath, jsonMissingErrorMsg); + const sourceText = fs.readFileSync(jsonFilePath, "utf8"); + var taskJson; + let pointers: any; try { - taskJson = require(jsonFilePath); + const parsed = jsonSourceMap.parse(sourceText); + taskJson = parsed.data; + pointers = parsed.pointers; } catch (jsonError) { trace.debug("Invalid task json: %s", jsonError); - throw new Error("Invalid task json: " + jsonError); + const err: any = new Error("Invalid task json: " + jsonError); + err.validationIssues = [{ file: jsonFilePath, line: null, col: null, message: "Invalid task json." }]; + throw err; } - var issues: string[] = validateTask(jsonFilePath, taskJson); + const issues = validateTask(jsonFilePath, taskJson, pointers); if (issues.length > 0) { var output: string = "Invalid task json:"; for (var i = 0; i < issues.length; i++) { - output += "\n\t" + issues[i]; + output += "\n\t" + issues[i].message; } trace.debug(output); - throw new Error(output); + const err: any = new Error(output); + err.validationIssues = issues; + throw err; } trace.debug("Json is valid."); @@ -45,6 +62,27 @@ export function validate(jsonFilePath: string, jsonMissingErrorMessage?: string, return taskJson; } +function createIssue(file: string, message: string, line: number | null = null, col: number | null = null): StructuredValidationIssue { + return { file, line, col, message }; +} + +function escapeJsonPointerToken(token: string): string { + return token.replace(/~/g, "~0").replace(/\//g, "~1"); +} + +function pointerLocation(pointers: any, pointerPath: string): { line: number | null; col: number | null } { + if (!pointers || !pointers[pointerPath]) { + return { line: null, col: null }; + } + + const loc = pointers[pointerPath].key || pointers[pointerPath].value; + if (!loc || typeof loc.line !== "number" || typeof loc.column !== "number") { + return { line: null, col: null }; + } + + return { line: loc.line + 1, col: loc.column + 1 }; +} + /* * Wrapper for fs.existsSync that includes a user-specified errorMessage in an InvalidDirectoryException */ @@ -112,32 +150,38 @@ export function validateRunner(taskData: any, allMatchedPaths?: string[]) { * @param taskData the parsed json file * @return list of issues with the json file */ -export function validateTask(taskPath: string, taskData: any): string[] { +export function validateTask(taskPath: string, taskData: any, pointers: any): StructuredValidationIssue[] { var vn = taskData.name || taskPath; - var issues: string[] = []; + var issues: StructuredValidationIssue[] = []; + + const rootLoc = pointerLocation(pointers, ""); + const idLoc = pointerLocation(pointers, "/id"); + const nameLoc = pointerLocation(pointers, "/name"); + const friendlyNameLoc = pointerLocation(pointers, "/friendlyName"); + const instanceNameFormatLoc = pointerLocation(pointers, "/instanceNameFormat"); if (!taskData.id || !check.isUUID(taskData.id)) { - issues.push(vn + ": id is a required guid"); + issues.push(createIssue(taskPath, "id is a required guid", idLoc.line ?? rootLoc.line, idLoc.col ?? rootLoc.col)); } if (!taskData.name || !check.matches(taskData.name, /^[A-Za-z0-9\-]+$/)) { - issues.push(vn + ": name is a required alphanumeric string"); + issues.push(createIssue(taskPath, "name is a required alphanumeric string", nameLoc.line ?? rootLoc.line, nameLoc.col ?? rootLoc.col)); } if (!taskData.friendlyName || !check.isLength(taskData.friendlyName, 1, 40)) { - issues.push(vn + ": friendlyName is a required string <= 40 chars"); + issues.push(createIssue(taskPath, "friendlyName is a required string <= 40 chars", friendlyNameLoc.line ?? rootLoc.line, friendlyNameLoc.col ?? rootLoc.col)); } if (!taskData.instanceNameFormat) { - issues.push(vn + ": instanceNameFormat is required"); + issues.push(createIssue(taskPath, "instanceNameFormat is required", instanceNameFormatLoc.line ?? rootLoc.line, instanceNameFormatLoc.col ?? rootLoc.col)); } - issues.push(...validateAllExecutionHandlers(taskPath, taskData, vn)); + issues.push(...validateAllExecutionHandlers(taskPath, taskData, vn, pointers)); // Fix: Return issues array regardless of whether execution block exists or not // Previously this return was inside the if(taskData.execution) block, causing // tasks without execution configuration to return undefined instead of validation issues - return (issues.length > 0) ? [taskPath, ...issues] : []; + return issues; } /** @@ -147,8 +191,8 @@ export function validateTask(taskPath: string, taskData: any): string[] { * @param vn Name of the task or path * @returns Array of issues found for all handlers */ -function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: string): string[] { - const issues: string[] = []; +function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: string, pointers: any): StructuredValidationIssue[] { + const issues: StructuredValidationIssue[] = []; const executionProperties = ['execution', 'prejobexecution', 'postjobexecution']; const supportedRunners = ["Node", "Node10", "Node16", "Node20_1", "Node24", "PowerShell", "PowerShell3", "Process"]; executionProperties.forEach(executionType => { @@ -156,7 +200,7 @@ function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: strin Object.keys(taskData[executionType]).forEach(runner => { if (supportedRunners.indexOf(runner) === -1) return; const target = taskData[executionType][runner]?.target; - issues.push(...validateExecutionTarget(taskPath, vn, executionType, runner, target)); + issues.push(...validateExecutionTarget(taskPath, vn, executionType, runner, target, pointers)); }); } }); @@ -172,10 +216,13 @@ function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: strin * @param target Execution handler's target * @returns Array of issues found for this runner */ -function validateExecutionTarget(taskPath: string, vn: string, executionType: string, runner: string, target: string | undefined): string[] { - const issues: string[] = []; +function validateExecutionTarget(taskPath: string, vn: string, executionType: string, runner: string, target: string | undefined, pointers: any): StructuredValidationIssue[] { + const issues: StructuredValidationIssue[] = []; + const targetPointer = `/${escapeJsonPointerToken(executionType)}/${escapeJsonPointerToken(runner)}/target`; + const targetLoc = pointerLocation(pointers, targetPointer); + if (!target) { - issues.push(`${vn}: ${executionType}.${runner}.target is required`); + issues.push(createIssue(taskPath, `${executionType}.${runner}.target is required`, targetLoc.line, targetLoc.col)); } else { const normalizedTarget = target.replace(/\$\(\s*currentdirectory\s*\)/i, "."); @@ -186,7 +233,7 @@ function validateExecutionTarget(taskPath: string, vn: string, executionType: st // check if the target file exists if (!fs.existsSync(path.join(path.dirname(taskPath), normalizedTarget))) { - issues.push(`${vn}: ${executionType}.${runner}.target references file that does not exist: ${normalizedTarget}`); + issues.push(createIssue(taskPath, `${executionType}.${runner}.target references file that does not exist: ${normalizedTarget}`, targetLoc.line, targetLoc.col)); } } return issues; diff --git a/docs/extensions.md b/docs/extensions.md index c56698bb..574f5958 100644 --- a/docs/extensions.md +++ b/docs/extensions.md @@ -23,6 +23,7 @@ To learn more about TFX, its pre-reqs and how to install, see the [readme](../RE * `--overrides-file`: Path to a JSON file with overrides. This partial manifest will always take precedence over any values in the manifests. * `--rev-version`: Rev the latest version number of the extension and save the result. * `--bypass-validation`: Bypass local validation. +* `--warnings-as-errors`: Treat task.json validation warnings as errors. * `--publisher`: Use this as the publisher ID instead of what is specified in the manifest. * `--extension-id`: Use this as the extension ID instead of what is specified in the manifest. * `--output-path`: Path to write the VSIX. @@ -30,7 +31,7 @@ To learn more about TFX, its pre-reqs and how to install, see the [readme](../RE ### Examples -#### Package for a different publisher +#### Package for a different publisher ``` tfx extension create --publisher mypublisher --manifest-js myextension.config.js --env mode=production @@ -113,6 +114,38 @@ tfx extension publish --publisher mypublisher --manifest-js myextension.config.j 3. When you run the `publish` command, you will be prompted for a Personal Access Token to authenticate to the Marketplace. For more information about obtaining a Personal Access Token, see [Publish from the command line](https://docs.microsoft.com/azure/devops/extend/publish/command-line?view=vsts). +## Validate an extension + +### Usage + +``` +tfx extension validate +``` + +### Arguments + +You can validate an extension from manifests (similar inputs to `extension create`): + +* `--root`: Root directory. +* `--manifest-js`: Manifest in the form of a standard Node.js CommonJS module with an exported function. If present then the manifests and manifest-globs arguments are ignored. +* `--env`: Environment variables passed to the manifestJs module. +* `--manifests`: List of individual manifest files (space separated). +* `--manifest-globs`: List of globs to find manifests (space separated). +* `--override`: JSON string which is merged into the manifests, overriding any values. +* `--overrides-file`: Path to a JSON file with overrides. This partial manifest will always take precedence over any values in the manifests. +* `--warnings-as-errors`: Treat task.json validation warnings as errors. +* `--publisher`: Use this as the publisher ID instead of what is specified in the manifest. +* `--extension-id`: Use this as the extension ID instead of what is specified in the manifest. +* `--loc-root`: Root of localization hierarchy (see README for more info). + +### Examples + +Validate from manifests: + +``` +tfx extension validate --root . +``` + ## Other commands @@ -120,6 +153,7 @@ tfx extension publish --publisher mypublisher --manifest-js myextension.config.j * `tfx extension show`: Show information about a published extension. * `tfx extension share`: Share an extension with an account. * `tfx extension unshare`: Unshare an extension with an account. +* `tfx extension validate`: Validate an extension from manifests or VSIX without creating/publishing. * `tfx extension isvalid`: Checks if an extension is valid. For more details on a specific command, run: diff --git a/package.json b/package.json index f4d554c3..61373592 100644 --- a/package.json +++ b/package.json @@ -48,6 +48,7 @@ "glob": "^11.1.0", "jju": "^1.4.0", "json-in-place": "^1.0.1", + "json-source-map": "^0.6.1", "jszip": "^3.10.1", "lodash": "^4.17.21", "minimist": "^1.2.6", diff --git a/tests/extension-local-tests.ts b/tests/extension-local-tests.ts index 00a76c30..db9c4bca 100644 --- a/tests/extension-local-tests.ts +++ b/tests/extension-local-tests.ts @@ -76,13 +76,14 @@ describe('Extension Commands - Local Tests', function() { }); describe('Command Help and Hierarchy', function() { - + it('should display extension command group help', function(done) { execAsyncWithLogging(`node "${tfxPath}" extension --help`, 'extension --help') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); assert(cleanOutput.includes('Available commands and command groups in tfx / extension'), 'Should show extension command hierarchy'); assert(cleanOutput.includes('create:'), 'Should list create command'); + assert(cleanOutput.includes('validate:'), 'Should list validate command'); assert(cleanOutput.includes('publish:'), 'Should list publish command'); assert(cleanOutput.includes('show:'), 'Should list show command'); assert(cleanOutput.includes('install:'), 'Should list install command'); @@ -127,6 +128,19 @@ describe('Extension Commands - Local Tests', function() { .catch(done); }); + it('should display validate command help', function(done) { + execAsyncWithLogging(`node "${tfxPath}" extension validate --help`, 'extension validate --help') + .then(({ stdout }) => { + const cleanOutput = stripColors(stdout); + assert(cleanOutput.includes('Validate an extension from manifests without packaging or publishing'), 'Should show validate command description'); + assert(!cleanOutput.includes('--vsix'), 'Should not show vsix argument'); + assert(cleanOutput.includes('--warnings-as-errors'), 'Should show warnings-as-errors argument'); + assert(cleanOutput.includes('--manifest-js') || cleanOutput.includes('--manifests'), 'Should show manifest arguments'); + done(); + }) + .catch(done); + }); + it('should display install command help', function(done) { execAsyncWithLogging(`node "${tfxPath}" extension install --help`, 'extension install --help') .then(({ stdout }) => { @@ -176,13 +190,13 @@ describe('Extension Commands - Local Tests', function() { it('should create extension from basic sample', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'test-extension.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create basic sample') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); assert(cleanOutput.includes('Completed operation: create extension'), 'Should indicate successful creation'); assert(fs.existsSync(outputPath), 'Should create .vsix file'); - + const stats = fs.statSync(outputPath); assert(stats.size > 0, 'Created .vsix file should not be empty'); done(); @@ -195,7 +209,7 @@ describe('Extension Commands - Local Tests', function() { if (!fs.existsSync(tempDir)) { fs.mkdirSync(tempDir); } - + const outputPath = path.join(__dirname, 'temp-extension-create.vsix'); execAsyncWithLogging(`node "${tfxPath}" extension create --root "${tempDir}" --output-path "${outputPath}" --no-prompt`, 'extension create missing manifest') .then(() => { @@ -204,7 +218,7 @@ describe('Extension Commands - Local Tests', function() { .catch((error) => { const cleanOutput = stripColors(error.stderr || error.stdout || error.message); assert(cleanOutput.includes('ENOENT') || cleanOutput.includes('vss-extension.json') || cleanOutput.includes('manifest') || cleanOutput.includes('no manifests found'), 'Should mention missing manifest file'); - + // Cleanup try { if (fs.existsSync(outputPath)) { @@ -218,16 +232,194 @@ describe('Extension Commands - Local Tests', function() { }); }); + describe('Extension Validation - Basic Operations', function() { + it('should validate extension from manifest inputs without creating a vsix', function(done) { + const basicExtensionPath = path.join(samplesPath, 'basic-extension'); + const outputPath = path.join(basicExtensionPath, 'validate-manifest-should-not-exist.vsix'); + + execAsyncWithLogging(`node "${tfxPath}" extension validate --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension validate from manifest') + .then(({ stdout }) => { + const cleanOutput = stripColors(stdout); + assert(cleanOutput.includes('Completed operation: validate extension'), 'Should indicate validate operation completed'); + assert(cleanOutput.includes('Validation: success'), 'Should indicate validation success'); + assert(!fs.existsSync(outputPath), 'Validate should not create a .vsix file'); + done(); + }) + .catch(done) + .finally(() => { + try { + if (fs.existsSync(outputPath)) { + fs.unlinkSync(outputPath); + } + } catch (e) { + // Ignore cleanup errors + } + }); + }); + + it('should reject --vsix for validate command', function(done) { + const basicExtensionPath = path.join(samplesPath, 'basic-extension'); + const vsixPath = path.join(basicExtensionPath, 'validate-input.vsix'); + + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${vsixPath}"`, 'extension create for validate-vsix') + .then(() => { + return execAsyncWithLogging(`node "${tfxPath}" extension validate --vsix "${vsixPath}"`, 'extension validate --vsix'); + }) + .then(() => { + done(new Error('Validate should fail when --vsix is provided')); + }) + .catch((error) => { + const cleanOutput = stripColors(error.stderr || error.stdout || error.message); + assert(cleanOutput.includes('--vsix') && cleanOutput.includes('not supported'), 'Should indicate that --vsix is not supported'); + done(); + }) + .finally(() => { + try { + if (fs.existsSync(vsixPath)) { + fs.unlinkSync(vsixPath); + } + } catch (e) { + // Ignore cleanup errors + } + }); + }); + + it('should treat task.json warnings as errors when --warnings-as-errors is set', function(done) { + const invalidTaskExtensionPath = path.join(samplesPath, 'invalid-task-extension'); + + execAsyncWithLogging( + `node "${tfxPath}" extension validate --root "${invalidTaskExtensionPath}" --warnings-as-errors --no-prompt`, + 'extension validate --warnings-as-errors', + ) + .then(({ stdout }) => { + const cleanOutput = stripColors(stdout); + assert(cleanOutput.includes('Validation: failed'), 'Should fail when warnings are treated as errors'); + assert(cleanOutput.includes('id is a required guid'), 'Should surface task.json validation details as errors'); + done(); + }) + .catch(done); + }); + + it('should output structured diagnostics in json mode', function(done) { + const invalidTaskExtensionPath = path.join(samplesPath, 'invalid-task-extension'); + + execAsyncWithLogging( + `node "${tfxPath}" extension validate --root "${invalidTaskExtensionPath}" --json --warnings-as-errors --no-prompt`, + 'extension validate --json --warnings-as-errors', + ) + .then(({ stdout }) => { + const result = JSON.parse(stdout); + assert(result && result.status === 'error', 'Should return error status in json mode'); + assert(Array.isArray(result.issues) && result.issues.length > 0, 'Should include issues'); + + const firstIssue = result.issues[0]; + assert(Object.prototype.hasOwnProperty.call(firstIssue, 'file'), 'Issue should include file'); + assert(Object.prototype.hasOwnProperty.call(firstIssue, 'line'), 'Issue should include line'); + assert(Object.prototype.hasOwnProperty.call(firstIssue, 'col'), 'Issue should include col'); + assert(Object.prototype.hasOwnProperty.call(firstIssue, 'message'), 'Issue should include message'); + + const markerIssue = result.issues.find(issue => issue.message === 'Invalid task json:' || issue.message === 'task.json'); + assert(!markerIssue, 'Should not include marker-only issues in structured output'); + + const idIssue = result.issues.find(issue => issue.message.indexOf('id is a required guid') >= 0); + assert(idIssue, 'Should include id validation issue'); + assert(idIssue.line && idIssue.line > 0, 'Issue should include line when available'); + assert(idIssue.col && idIssue.col > 0, 'Issue should include col when available'); + + done(); + }) + .catch(done); + }); + + it('should report missing task name when name is omitted', function(done) { + const tempRoot = path.join(__dirname, '../temp-extensions/missing-name-task-extension'); + const taskDir = path.join(tempRoot, 'MissingNameTask'); + const manifestPath = path.join(tempRoot, 'vss-extension.json'); + const taskJsonPath = path.join(taskDir, 'task.json'); + const taskTargetPath = path.join(taskDir, 'index.js'); + + try { + if (!fs.existsSync(path.dirname(tempRoot))) { + fs.mkdirSync(path.dirname(tempRoot)); + } + if (!fs.existsSync(tempRoot)) { + fs.mkdirSync(tempRoot); + } + if (!fs.existsSync(taskDir)) { + fs.mkdirSync(taskDir); + } + + const manifest = { + manifestVersion: 1, + id: 'missing-name-task-extension', + publisher: 'fabrikam', + version: '1.0.0', + name: 'Missing Name Task Extension', + targets: [{ id: 'Microsoft.VisualStudio.Services' }], + contributions: [ + { + id: 'missing-name-task-contribution', + type: 'ms.vss-distributed-task.task', + targets: ['ms.vss-distributed-task.tasks'], + properties: { + name: 'MissingNameTask', + }, + }, + ], + }; + + const taskJson = { + id: '11111111-1111-1111-1111-111111111111', + friendlyName: 'Sample Friendly Name', + instanceNameFormat: 'Do work', + helpMarkDown: 'Help', + category: 'Utility', + author: 'Microsoft', + version: { Major: 1, Minor: 0, Patch: 0 }, + inputs: [], + execution: { + Node20_1: { + target: 'index.js', + }, + }, + }; + + fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2)); + fs.writeFileSync(taskJsonPath, JSON.stringify(taskJson, null, 2)); + fs.writeFileSync(taskTargetPath, 'console.log("ok");'); + } catch (setupError) { + done(setupError); + return; + } + + execAsyncWithLogging( + `node "${tfxPath}" extension validate --root "${tempRoot}" --json --warnings-as-errors --no-prompt`, + 'extension validate missing task name', + ) + .then(({ stdout }) => { + const result = JSON.parse(stdout); + assert(result && result.status === 'error', 'Should return error status in json mode'); + assert(Array.isArray(result.issues) && result.issues.length > 0, 'Should include issues'); + + const nameIssue = result.issues.find(issue => issue.message.indexOf('name is a required alphanumeric string') >= 0); + assert(nameIssue, 'Should include missing name validation issue'); + assert(nameIssue.file && nameIssue.file.endsWith('task.json'), 'Issue should point to task.json file'); + done(); + }) + .catch(done); + }); + }); + describe('Extension Creation - Advanced Features', function() { it('should handle --override parameter', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'override-test.vsix'); const overrideFilePath = path.join(basicExtensionPath, 'test-overrides.json'); - + // Create temporary overrides file fs.writeFileSync(overrideFilePath, JSON.stringify({ version: "2.0.0" }, null, 2)); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}" --overrides-file "${overrideFilePath}"`, 'extension create with overrides') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); @@ -256,7 +448,7 @@ describe('Extension Commands - Local Tests', function() { it('should handle --rev-version parameter', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'rev-version-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}" --rev-version`, 'extension create --rev-version') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); @@ -270,7 +462,7 @@ describe('Extension Commands - Local Tests', function() { it('should handle --bypass-validation parameter', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'bypass-validation-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}" --bypass-validation`, 'extension create --bypass-validation') .then(({ stdout }) => { // With bypass validation, it might still fail due to other issues, but validation should be skipped @@ -289,7 +481,7 @@ describe('Extension Commands - Local Tests', function() { }); describe('Extension Global Arguments', function() { - + it('should handle --no-color argument', function(done) { execAsyncWithLogging(`node "${tfxPath}" extension --help --no-color`, 'extension --help --no-color') .then(({ stdout }) => { @@ -305,7 +497,7 @@ describe('Extension Commands - Local Tests', function() { it('should handle --trace-level argument', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'trace-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}" --trace-level debug`, 'extension create --trace-level debug') .then(({ stdout, stderr }) => { const cleanOutput = stripColors(stdout + stderr); @@ -330,15 +522,15 @@ describe('Extension Commands - Local Tests', function() { }); describe('Extension File Path Handling', function() { - + it('should handle relative paths', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'relative-test.vsix'); - + // Change to the extension directory and use relative paths const oldCwd = process.cwd(); process.chdir(basicExtensionPath); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root . --output-path relative-test.vsix`, 'extension create relative path') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); @@ -356,7 +548,7 @@ describe('Extension Commands - Local Tests', function() { it('should handle absolute paths', function(done) { const basicExtensionPath = path.resolve(samplesPath, 'basic-extension'); const outputPath = path.resolve(basicExtensionPath, 'absolute-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create absolute path') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); @@ -369,11 +561,11 @@ describe('Extension Commands - Local Tests', function() { }); describe('Extension Manifest Variations', function() { - + it('should handle manifest-globs parameter', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'manifest-globs-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}" --manifest-globs "vss-extension.json"`, 'extension create --manifest-globs') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); @@ -657,13 +849,13 @@ describe('Extension Commands - Local Tests', function() { } const outputPath = path.join(complexExtensionPath, 'complex-extension.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${complexExtensionPath}" --output-path "${outputPath}"`, 'extension create complex') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); assert(cleanOutput.includes('Completed operation: create extension'), 'Should create complex extension'); assert(fs.existsSync(outputPath), 'Should create .vsix file for complex extension'); - + const stats = fs.statSync(outputPath); assert(stats.size > 1000, 'Complex extension should be reasonably sized'); done(); @@ -674,7 +866,7 @@ describe('Extension Commands - Local Tests', function() { it('should override publisher in manifest', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'publisher-override.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}" --publisher "test-publisher"`, 'extension create --publisher') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); @@ -688,7 +880,7 @@ describe('Extension Commands - Local Tests', function() { it('should override extension-id in manifest', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'extension-id-override.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}" --extension-id "test-extension-id"`, 'extension create --extension-id') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); @@ -702,7 +894,7 @@ describe('Extension Commands - Local Tests', function() { it('should support JSON output format for create command', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'json-output-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}" --json`, 'extension create --json') .then(({ stdout }) => { // With --json flag, output might be JSON formatted @@ -768,7 +960,7 @@ describe('Extension Commands - Local Tests', function() { it('should handle extension with missing files referenced in manifest', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'missing-files-test.vsix'); - + // This should still create the extension but might show warnings execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create missing files in manifest') .then(({ stdout }) => { @@ -797,7 +989,7 @@ describe('Extension Commands - Local Tests', function() { } const outputPath = path.join(taskExtensionPath, 'valid-task-extension.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${taskExtensionPath}" --output-path "${outputPath}"`, 'extension create task extension') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); @@ -817,7 +1009,7 @@ describe('Extension Commands - Local Tests', function() { } const outputPath = path.join(taskExtensionPath, 'validated-task.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${taskExtensionPath}" --output-path "${outputPath}"`, 'extension create validated task') .then(({ stdout }) => { // Should validate task.json files without errors @@ -837,7 +1029,7 @@ describe('Extension Commands - Local Tests', function() { } const outputPath = path.join(taskExtensionPath, 'deprecated-runner.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${taskExtensionPath}" --output-path "${outputPath}"`, 'extension create deprecated runner') .then(({ stdout }) => { // Should still create extension despite warnings @@ -856,7 +1048,7 @@ describe('Extension Commands - Local Tests', function() { } const outputPath = path.join(taskExtensionPath, 'versioned-tasks.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${taskExtensionPath}" --output-path "${outputPath}"`, 'extension create versioned tasks') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); @@ -870,7 +1062,7 @@ describe('Extension Commands - Local Tests', function() { it('should warn about invalid task.json but still create extension', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'invalid-task-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create invalid task') .then(({ stdout }) => { // Should create extension despite task validation warnings @@ -883,7 +1075,7 @@ describe('Extension Commands - Local Tests', function() { it('should warn about missing task.json file', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'missing-task-json.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create missing task json') .then(({ stdout }) => { // Should create extension despite missing task files @@ -896,7 +1088,7 @@ describe('Extension Commands - Local Tests', function() { it('should warn about missing execution target file', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'missing-execution-file.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create missing execution file') .then(({ stdout }) => { // Should create extension despite warnings about missing execution files @@ -909,7 +1101,7 @@ describe('Extension Commands - Local Tests', function() { it('should warn about invalid task name format', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'invalid-task-name.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create invalid task name') .then(({ stdout }) => { // Should create extension despite task name validation warnings @@ -922,7 +1114,7 @@ describe('Extension Commands - Local Tests', function() { it('should warn about friendly name length', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'long-name-extension.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}" --no-prompt`, 'extension create long name') .then(({ stdout, stderr }) => { // Should create extension despite friendly name warnings @@ -941,7 +1133,7 @@ describe('Extension Commands - Local Tests', function() { } const outputPath = path.join(taskExtensionPath, 'contributions-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${taskExtensionPath}" --output-path "${outputPath}"`, 'extension create contributions test') .then(({ stdout }) => { // Should validate task directory structure @@ -954,7 +1146,7 @@ describe('Extension Commands - Local Tests', function() { it('should handle extensions without task contributions', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'no-tasks-extension.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create no tasks') .then(({ stdout }) => { const cleanOutput = stripColors(stdout); @@ -968,7 +1160,7 @@ describe('Extension Commands - Local Tests', function() { it('should validate required task fields', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'task-fields-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create task fields') .then(({ stdout }) => { // Should validate task field requirements @@ -981,7 +1173,7 @@ describe('Extension Commands - Local Tests', function() { it('should validate task inputs structure', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'task-inputs-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create task inputs') .then(({ stdout }) => { // Should validate task inputs structure @@ -994,7 +1186,7 @@ describe('Extension Commands - Local Tests', function() { it('should validate execution targets exist', function(done) { const basicExtensionPath = path.join(samplesPath, 'basic-extension'); const outputPath = path.join(basicExtensionPath, 'execution-targets-test.vsix'); - + execAsyncWithLogging(`node "${tfxPath}" extension create --root "${basicExtensionPath}" --output-path "${outputPath}"`, 'extension create execution targets') .then(({ stdout }) => { // Should validate execution target file existence From 1ba4ec2eae9bebb404e27fa5143c0a435cef5dab Mon Sep 17 00:00:00 2001 From: Jesse Houwing Date: Sun, 22 Feb 2026 23:58:51 +0100 Subject: [PATCH 2/4] Extends location data to other manifests as well --- .../_lib/extension-composer-factory.ts | 7 +- app/exec/extension/_lib/merger.ts | 227 ++++++++++++++++-- app/exec/extension/default.ts | 67 +++++- app/exec/extension/validate.ts | 16 +- app/lib/errorhandler.ts | 82 ++++++- app/lib/jsonvalidate.ts | 152 +++++++++--- package-lock.json | 16 +- package.json | 2 +- tests/extension-local-tests.ts | 95 ++++++++ 9 files changed, 600 insertions(+), 64 deletions(-) diff --git a/app/exec/extension/_lib/extension-composer-factory.ts b/app/exec/extension/_lib/extension-composer-factory.ts index 35d7a837..b554ec1a 100644 --- a/app/exec/extension/_lib/extension-composer-factory.ts +++ b/app/exec/extension/_lib/extension-composer-factory.ts @@ -38,12 +38,13 @@ export class ComposerFactory { "'" + target.id + "' is not a recognized target. Valid targets are 'Microsoft.VisualStudio.Services', 'Microsoft.VisualStudio.Services.Integration', 'Microsoft.VisualStudio.Offer'"; + const targetWithSource = target; const err: any = new Error(message); err.validationIssues = [ { - file: null, - line: null, - col: null, + file: targetWithSource.__origin || null, + line: targetWithSource.__line || null, + col: targetWithSource.__col || null, message: message, }, ]; diff --git a/app/exec/extension/_lib/merger.ts b/app/exec/extension/_lib/merger.ts index c34f8b1a..e0e02049 100644 --- a/app/exec/extension/_lib/merger.ts +++ b/app/exec/extension/_lib/merger.ts @@ -16,6 +16,7 @@ import fs = require("fs"); import { glob } from "glob"; import jju = require("jju"); import jsonInPlace = require("json-in-place"); +import * as jsonc from "jsonc-parser"; import loc = require("./loc"); import path = require("path"); import trace = require("../../../lib/trace"); @@ -49,6 +50,84 @@ export class Merger { this.manifestBuilders = []; } + private getPointerLocation(pointerMap: any, pointerPath: string): { line: number | null; col: number | null } { + if (!pointerMap || !pointerMap.root || typeof pointerMap.sourceText !== "string") { + return { line: null, col: null }; + } + + const segments = pointerPath === "" + ? [] + : pointerPath + .split("/") + .slice(1) + .map(token => token.replace(/~1/g, "/").replace(/~0/g, "~")) + .map(token => /^\d+$/.test(token) ? parseInt(token, 10) : token); + + const node = jsonc.findNodeAtLocation(pointerMap.root, segments); + if (!node) { + return { line: null, col: null }; + } + + const locationNode = node.parent && node.parent.type === "property" ? node.parent : node; + const location = this.offsetToLineCol(pointerMap.sourceText, locationNode.offset); + + return { + line: location.line, + col: location.col, + }; + } + + private offsetToLineCol(text: string, offset: number): { line: number; col: number } { + let line = 1; + let col = 1; + + for (let i = 0; i < offset && i < text.length; i++) { + const ch = text.charCodeAt(i); + if (ch === 13) { + if (i + 1 < text.length && text.charCodeAt(i + 1) === 10) { + i++; + } + line++; + col = 1; + } else if (ch === 10) { + line++; + col = 1; + } else { + col++; + } + } + + return { line, col }; + } + + private parseManifestText(jsonData: string): { data: any; pointers: any } { + try { + const parseErrors: jsonc.ParseError[] = []; + const rootNode = jsonc.parseTree(jsonData, parseErrors, { + allowTrailingComma: !!this.settings.json5, + disallowComments: !this.settings.json5, + }); + if (parseErrors.length > 0 || !rootNode) { + const parseErr: any = new Error("Invalid JSON/JSONC content."); + parseErr.parseErrors = parseErrors; + throw parseErr; + } + return { + data: jsonc.getNodeValue(rootNode), + pointers: { + sourceText: jsonData, + root: rootNode, + }, + }; + } catch (strictErr) { + if (this.settings.json5) { + const data = jju.parse(jsonData); + return { data, pointers: null }; + } + throw strictErr; + } + } + private async gatherManifests(): Promise { trace.debug("merger.gatherManifests"); @@ -128,12 +207,32 @@ export class Merger { promisify(readFile)(file, "utf8").then(data => { const jsonData = data.replace(/^\uFEFF/, ""); try { - const result = this.settings.json5 ? jju.parse(jsonData) : JSON.parse(jsonData); + const parsed = this.parseManifestText(jsonData); + let result: any = parsed.data; + result.__pointerMap = parsed.pointers; result.__origin = file; // save the origin in order to resolve relative paths later. return result; } catch (err) { trace.error("Error parsing the JSON in %s: ", file); trace.debug(jsonData, null); + const parseErrors = err && (err).parseErrors; + if (!err || !Array.isArray((err).validationIssues)) { + let line: number | null = null; + let col: number | null = null; + if (Array.isArray(parseErrors) && parseErrors.length > 0 && typeof parseErrors[0].offset === "number") { + const loc = this.offsetToLineCol(jsonData, parseErrors[0].offset); + line = loc.line; + col = loc.col; + } + (err).validationIssues = [ + { + file: file, + line: line, + col: col, + message: "Could not parse JSON.", + }, + ]; + } throw err; } }), @@ -153,10 +252,34 @@ export class Merger { let allContributions: any[] = []; partials.forEach(partial => { if (_.isArray(partial["targets"])) { - targets = targets.concat(partial["targets"]); + partial["targets"].forEach((target: any, targetIndex: number) => { + const idLoc = this.getPointerLocation(partial.__pointerMap, `/targets/${targetIndex}/id`); + const targetWithSource: any = _.assign({}, target, { + __origin: partial.__origin || null, + __line: idLoc.line, + __col: idLoc.col, + }); + targets.push(targetWithSource); + }); } if (_.isArray(partial["contributions"])) { - allContributions = allContributions.concat(partial["contributions"]); + partial["contributions"].forEach((contribution: any, contributionIndex: number) => { + const nameLoc = this.getPointerLocation( + partial.__pointerMap, + `/contributions/${contributionIndex}/properties/name`, + ); + const contributionLoc = this.getPointerLocation( + partial.__pointerMap, + `/contributions/${contributionIndex}`, + ); + allContributions.push( + _.assign({}, contribution, { + __origin: partial.__origin || null, + __line: nameLoc.line !== null ? nameLoc.line : contributionLoc.line, + __col: nameLoc.col !== null ? nameLoc.col : contributionLoc.col, + }), + ); + }); } }); this.extensionComposer = ComposerFactory.GetComposer(this.settings, targets); @@ -404,10 +527,27 @@ export class Merger { } private async validateBuildTaskContributions(contributions: any[]): Promise { - const warnings: string[] = []; - const addWarning = (message: string) => { - warnings.push(message); - trace.warn(message); + const warnings: Array<{ file: string | null; line: number | null; col: number | null; message: string }> = []; + const formatIssueForEditor = ( + issue: { file: string | null; line: number | null; col: number | null; message: string }, + severity: "warning" | "error" = "warning", + ): string => { + if (issue.file && issue.line !== null && issue.col !== null) { + return `${issue.file}(${issue.line},${issue.col}): ${severity}: ${issue.message}`; + } + if (issue.file && issue.line !== null) { + return `${issue.file}(${issue.line}): ${severity}: ${issue.message}`; + } + if (issue.file) { + return `${issue.file}: ${severity}: ${issue.message}`; + } + return `${severity}: ${issue.message}`; + }; + + const addWarning = (message: string, file: string | null = null, line: number | null = null, col: number | null = null) => { + const warning = { file, line, col, message }; + warnings.push(warning); + console.warn(formatIssueForEditor(warning, "warning")); }; try { @@ -453,7 +593,7 @@ export class Merger { } } } catch (err) { - addWarning(`Error reading task directory ${absoluteTaskPath}: ${err}`); + addWarning(`Error reading task directory ${absoluteTaskPath}: ${err}`, absoluteTaskPath, 1, 1); } } @@ -462,7 +602,7 @@ export class Merger { trace.debug(`Validating ${contributionTaskJsonPaths.length} task.json files for contribution ${contrib.id || taskPath}`); for (const taskJsonPath of contributionTaskJsonPaths) { - validate(taskJsonPath, "no task.json in specified directory", contributionTaskJsonPaths); + validate(taskJsonPath, "no task.json in specified directory", contributionTaskJsonPaths, this.settings.json5); } // Also collect for global tracking if needed @@ -470,6 +610,9 @@ export class Merger { } else { addWarning( `Build task contribution '${contrib.id || taskPath}' does not have a task.json file. Expected task.json in ${absoluteTaskPath} or its subdirectories.`, + contrib && contrib.__origin !== undefined ? contrib.__origin : null, + contrib && contrib.__line !== undefined ? contrib.__line : null, + contrib && contrib.__col !== undefined ? contrib.__col : null, ); } } @@ -484,13 +627,13 @@ export class Merger { if (this.settings.warningsAsErrors && warnings.length > 0) { const warningsAsErrorsErr: any = new Error( "Task.json validation produced warnings. Re-run without --warnings-as-errors to treat them as warnings only.\n" + - warnings.join("\n"), + warnings.map(w => w.message).join("\n"), ); - warningsAsErrorsErr.validationIssues = warnings.map(message => ({ - file: null, - line: null, - col: null, - message: message, + warningsAsErrorsErr.validationIssues = warnings.map(warning => ({ + file: warning.file, + line: warning.line, + col: warning.col, + message: warning.message, })); throw warningsAsErrorsErr; } @@ -503,11 +646,55 @@ export class Merger { } throw new Error("Error occurred while validating build task contributions."); } - trace.warn( - err && err instanceof Error - ? warningMessage + err.message - : `Error occurred while validating build task contributions. ${warningMessage}`, - ); + const structuredIssues = err && Array.isArray((err).validationIssues) ? (err).validationIssues : null; + if (structuredIssues && structuredIssues.length > 0) { + const warningByFile: { [file: string]: boolean } = {}; + structuredIssues.forEach(issue => { + const fileKey = issue && issue.file ? String(issue.file) : ""; + if (!warningByFile[fileKey]) { + warningByFile[fileKey] = true; + console.warn( + formatIssueForEditor( + { + file: issue && issue.file !== undefined ? issue.file : null, + line: 1, + col: 1, + message: warningMessage.trim(), + }, + "warning", + ), + ); + } + }); + structuredIssues.forEach(issue => { + console.warn( + formatIssueForEditor( + { + file: issue && issue.file !== undefined ? issue.file : null, + line: issue && issue.line !== undefined ? issue.line : null, + col: issue && issue.col !== undefined ? issue.col : null, + message: issue && issue.message ? issue.message : String(issue), + }, + "warning", + ), + ); + }); + } else { + console.warn( + formatIssueForEditor( + { + file: null, + line: null, + col: null, + message: + err && err instanceof Error + ? `${warningMessage}${err.message}` + : `Error occurred while validating build task contributions. ${warningMessage}`, + }, + "warning", + ), + ); + } } } } diff --git a/app/exec/extension/default.ts b/app/exec/extension/default.ts index d303b6a0..ff54f567 100644 --- a/app/exec/extension/default.ts +++ b/app/exec/extension/default.ts @@ -6,6 +6,7 @@ import { GalleryBase, CoreExtInfo, PublisherManager, PackagePublisher } from "./ import * as path from "path"; import _ = require("lodash"); import jju = require("jju"); +import * as jsonc from "jsonc-parser"; import args = require("../../lib/arguments"); import https = require("https"); import trace = require("../../lib/trace"); @@ -14,6 +15,29 @@ import { readFile } from "fs"; import { promisify } from "util"; import { GalleryApi } from "azure-devops-node-api/GalleryApi"; +function offsetToLineCol(text: string, offset: number): { line: number; col: number } { + let line = 1; + let col = 1; + + for (let i = 0; i < offset && i < text.length; i++) { + const ch = text.charCodeAt(i); + if (ch === 13) { + if (i + 1 < text.length && text.charCodeAt(i + 1) === 10) { + i++; + } + line++; + col = 1; + } else if (ch === 10) { + line++; + col = 1; + } else { + col++; + } + } + + return { line, col }; +} + export function getCommand(args: string[]): TfCommand { return new ExtensionBase(args); } @@ -246,9 +270,48 @@ export class ExtensionBase extends TfCommand { let mergedOverrides = {}; let contentJSON = {}; try { - contentJSON = json5 ? jju.parse(content) : JSON.parse(content); + try { + const parseErrors: jsonc.ParseError[] = []; + const rootNode = jsonc.parseTree(content, parseErrors, { + allowTrailingComma: !!json5, + disallowComments: !json5, + }); + if (parseErrors.length > 0 || !rootNode) { + const parseErr: any = new Error("Invalid JSON/JSONC content."); + parseErr.parseErrors = parseErrors; + throw parseErr; + } + contentJSON = jsonc.getNodeValue(rootNode); + contentJSON["__pointerMap"] = { + sourceText: content, + root: rootNode, + }; + } catch (strictErr) { + if (json5) { + contentJSON = jju.parse(content); + } else { + throw strictErr; + } + } } catch (e) { - throw new Error("Could not parse contents of " + overridesFile[0] + " as JSON. \n"); + let line: number | null = null; + let col: number | null = null; + const parseErrors = e && (e).parseErrors; + if (Array.isArray(parseErrors) && parseErrors.length > 0 && typeof parseErrors[0].offset === "number") { + const loc = offsetToLineCol(content, parseErrors[0].offset); + line = loc.line; + col = loc.col; + } + const parseErr: any = new Error("Could not parse contents of " + overridesFile[0] + " as JSON. \n"); + parseErr.validationIssues = [ + { + file: overridesFile && overridesFile.length > 0 ? overridesFile[0] : null, + line: line, + col: col, + message: "Could not parse JSON.", + }, + ]; + throw parseErr; } contentJSON["__origin"] = overridesFile ? overridesFile[0] : path.join(root[0], "_override.json"); _.merge(mergedOverrides, contentJSON, override); diff --git a/app/exec/extension/validate.ts b/app/exec/extension/validate.ts index dc37ab95..162bd3f1 100644 --- a/app/exec/extension/validate.ts +++ b/app/exec/extension/validate.ts @@ -21,6 +21,19 @@ export interface ValidationResult { issues: ValidationIssue[]; } +function formatIssueForEditor(issue: ValidationIssue): string { + if (issue.file && issue.line !== null && issue.col !== null) { + return `${issue.file}(${issue.line},${issue.col}): error: ${issue.message}`; + } + if (issue.file && issue.line !== null) { + return `${issue.file}(${issue.line}): error: ${issue.message}`; + } + if (issue.file) { + return `${issue.file}: error: ${issue.message}`; + } + return `error: ${issue.message}`; +} + export class ExtensionValidate extends extBase.ExtensionBase { protected description = "Validate an extension from manifests without packaging or publishing."; @@ -104,9 +117,8 @@ export class ExtensionValidate extends extBase.ExtensionBase { trace.info(colors.red("\n=== Completed operation: validate extension ===")); trace.info(" - Source: %s", data.source); trace.info(" - Validation: %s", colors.red("failed")); - trace.info(" - Issues (%s):", data.issues.length.toString()); data.issues.forEach(issue => { - trace.info(" - %s", issue.message); + trace.info(formatIssueForEditor(issue)); }); } } diff --git a/app/lib/errorhandler.ts b/app/lib/errorhandler.ts index d0c37812..371125c1 100644 --- a/app/lib/errorhandler.ts +++ b/app/lib/errorhandler.ts @@ -1,5 +1,52 @@ import trace = require("./trace"); +function shouldEmitJsonError(): boolean { + const argv = process.argv || []; + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + if (arg === "--json") { + return true; + } + if (arg === "--output" && i + 1 < argv.length && argv[i + 1].toLowerCase() === "json") { + return true; + } + if (arg.indexOf("--output=") === 0 && arg.substring("--output=".length).toLowerCase() === "json") { + return true; + } + } + return false; +} + +function toStructuredIssues(err: any): any[] { + if (!err || !Array.isArray(err.validationIssues)) { + return null; + } + return err.validationIssues.map(issue => ({ + file: issue && issue.file !== undefined ? issue.file : null, + line: issue && issue.line !== undefined ? issue.line : null, + col: issue && issue.col !== undefined ? issue.col : null, + message: issue && issue.message ? issue.message : String(issue), + })); +} + +function formatIssueForEditor(issue: any): string { + const file = issue && issue.file ? String(issue.file) : null; + const line = issue && typeof issue.line === "number" ? issue.line : null; + const col = issue && typeof issue.col === "number" ? issue.col : null; + const message = issue && issue.message ? String(issue.message) : String(issue); + + if (file && line !== null && col !== null) { + return `${file}(${line},${col}): error: ${message}`; + } + if (file && line !== null) { + return `${file}(${line}): error: ${message}`; + } + if (file) { + return `${file}: error: ${message}`; + } + return `error: ${message}`; +} + /** * Formats any error type into a readable string message. * Handles AggregateError, Error, strings, objects, and other types. @@ -7,22 +54,22 @@ import trace = require("./trace"); function formatError(err: any): string { // Handle AggregateError (from Promise.all/Promise.any failures) if (err && err.name === "AggregateError" && Array.isArray(err.errors)) { - const messages = err.errors.map((e: any, index: number) => + const messages = err.errors.map((e: any, index: number) => ` [${index + 1}] ${formatError(e)}` ); return `Multiple errors occurred:\n${messages.join("\n")}`; } - + // Handle plain strings if (typeof err === "string") { return err; } - + // Handle Error instances - use toString() to preserve "Error: message" format if (err instanceof Error) { return err.toString(); } - + // Handle objects with a custom toString method (not the default Object.prototype.toString) if (err !== null && typeof err === "object" && typeof err.toString === "function" && err.toString !== Object.prototype.toString) { const result = err.toString(); @@ -31,12 +78,12 @@ function formatError(err: any): string { return result; } } - + // Handle objects with a message property (error-like objects) if (typeof err?.message === "string") { return err.message; } - + // Handle plain objects - try JSON serialization if (typeof err === "object" && err !== null) { try { @@ -45,7 +92,7 @@ function formatError(err: any): string { return String(err); } } - + // Fallback for any other type return String(err); } @@ -91,6 +138,27 @@ export function httpErr(obj): any { export function errLog(arg: any): void { trace.debug(arg?.stack); + if (shouldEmitJsonError()) { + const payload: any = { + status: "error", + message: formatError(arg), + }; + const issues = toStructuredIssues(arg); + if (issues) { + payload.issues = issues; + } + console.log(JSON.stringify(payload, null, 4)); + process.exit(-1); + return; + } + const issues = toStructuredIssues(arg); + if (issues && issues.length > 0) { + issues.forEach(issue => { + console.error(formatIssueForEditor(issue)); + }); + process.exit(-1); + return; + } trace.error(formatError(arg)); process.exit(-1); } diff --git a/app/lib/jsonvalidate.ts b/app/lib/jsonvalidate.ts index d5357ced..d73d4d5f 100644 --- a/app/lib/jsonvalidate.ts +++ b/app/lib/jsonvalidate.ts @@ -2,7 +2,7 @@ var fs = require("fs"); import * as path from 'path'; var check = require("validator"); var trace = require("./trace"); -const jsonSourceMap = require("json-source-map"); +import * as jsonc from "jsonc-parser"; const deprecatedRunners = ["Node", "Node6", "Node10", "Node16"]; @@ -25,7 +25,7 @@ export interface TaskJson { * @return the parsed json file * @throws InvalidDirectoryException if json file doesn't exist, InvalidJsonException on failed parse or *first* invalid field in json */ -export function validate(jsonFilePath: string, jsonMissingErrorMessage?: string, allMatchedPaths?: string[]): TaskJson { +export function validate(jsonFilePath: string, jsonMissingErrorMessage?: string, allMatchedPaths?: string[], json5?: boolean): TaskJson { trace.debug("Validating task json..."); var jsonMissingErrorMsg: string = jsonMissingErrorMessage || "specified json file does not exist."; exists(jsonFilePath, jsonMissingErrorMsg); @@ -33,19 +33,41 @@ export function validate(jsonFilePath: string, jsonMissingErrorMessage?: string, const sourceText = fs.readFileSync(jsonFilePath, "utf8"); var taskJson; - let pointers: any; + let pointerContext: any; try { - const parsed = jsonSourceMap.parse(sourceText); - taskJson = parsed.data; - pointers = parsed.pointers; + const parseErrors: jsonc.ParseError[] = []; + const root = jsonc.parseTree(sourceText, parseErrors, { + allowTrailingComma: !!json5, + disallowComments: !json5, + }); + + if (parseErrors.length > 0 || !root) { + const parseErr: any = new Error("Invalid JSON/JSONC content."); + parseErr.parseErrors = parseErrors; + throw parseErr; + } + + taskJson = jsonc.getNodeValue(root); + pointerContext = { + sourceText, + root, + }; } catch (jsonError) { trace.debug("Invalid task json: %s", jsonError); const err: any = new Error("Invalid task json: " + jsonError); - err.validationIssues = [{ file: jsonFilePath, line: null, col: null, message: "Invalid task json." }]; + let line: number | null = null; + let col: number | null = null; + const parseErrors = jsonError && (jsonError).parseErrors; + if (Array.isArray(parseErrors) && parseErrors.length > 0 && typeof parseErrors[0].offset === "number") { + const loc = offsetToLineCol(sourceText, parseErrors[0].offset); + line = loc.line; + col = loc.col; + } + err.validationIssues = [{ file: jsonFilePath, line, col, message: "Invalid task json." }]; throw err; } - const issues = validateTask(jsonFilePath, taskJson, pointers); + const issues = validateTask(jsonFilePath, taskJson, pointerContext); if (issues.length > 0) { var output: string = "Invalid task json:"; for (var i = 0; i < issues.length; i++) { @@ -58,7 +80,7 @@ export function validate(jsonFilePath: string, jsonMissingErrorMessage?: string, } trace.debug("Json is valid."); - validateRunner(taskJson, allMatchedPaths); + validateRunner(taskJson, allMatchedPaths, jsonFilePath, pointerContext); return taskJson; } @@ -70,17 +92,49 @@ function escapeJsonPointerToken(token: string): string { return token.replace(/~/g, "~0").replace(/\//g, "~1"); } -function pointerLocation(pointers: any, pointerPath: string): { line: number | null; col: number | null } { - if (!pointers || !pointers[pointerPath]) { +function pointerLocation(pointerContext: any, pointerPath: string): { line: number | null; col: number | null } { + if (!pointerContext || !pointerContext.root || typeof pointerContext.sourceText !== "string") { return { line: null, col: null }; } - const loc = pointers[pointerPath].key || pointers[pointerPath].value; - if (!loc || typeof loc.line !== "number" || typeof loc.column !== "number") { + const segments = pointerPath === "" + ? [] + : pointerPath + .split("/") + .slice(1) + .map(token => token.replace(/~1/g, "/").replace(/~0/g, "~")) + .map(token => /^\d+$/.test(token) ? parseInt(token, 10) : token); + + const node = jsonc.findNodeAtLocation(pointerContext.root, segments); + if (!node) { return { line: null, col: null }; } - return { line: loc.line + 1, col: loc.column + 1 }; + const locationNode = node.parent && node.parent.type === "property" ? node.parent : node; + return offsetToLineCol(pointerContext.sourceText, locationNode.offset); +} + +function offsetToLineCol(text: string, offset: number): { line: number; col: number } { + let line = 1; + let col = 1; + + for (let i = 0; i < offset && i < text.length; i++) { + const ch = text.charCodeAt(i); + if (ch === 13) { + if (i + 1 < text.length && text.charCodeAt(i + 1) === 10) { + i++; + } + line++; + col = 1; + } else if (ch === 10) { + line++; + col = 1; + } else { + col++; + } + } + + return { line, col }; } /* @@ -120,7 +174,7 @@ function countValidRunners(taskData: any): number { * @param taskData the parsed json file * @param allMatchedPaths optional array of all matched task.json paths for backwards compat detection */ -export function validateRunner(taskData: any, allMatchedPaths?: string[]) { +export function validateRunner(taskData: any, allMatchedPaths?: string[], taskPath?: string, pointerContext?: any) { if (countValidRunners(taskData) == 0) { if (allMatchedPaths) { for (const matchedPath of allMatchedPaths) { @@ -140,7 +194,51 @@ export function validateRunner(taskData: any, allMatchedPaths?: string[]) { } } - trace.warn("Task %s@%s is dependent on a task runner that is end-of-life and will be removed in the future. Please visit https://aka.ms/node-runner-guidance to learn how to upgrade the task.", taskData.name, taskData.version?.Major || "?") + const messagePrefix = + "Task " + + (taskData.name || "?") + + "@" + + (taskData.version?.Major || "?") + + " is dependent on a task runner that is end-of-life and will be removed in the future. Please visit https://aka.ms/node-runner-guidance to learn how to upgrade the task."; + + const executionProperties = ['execution', 'prejobexecution', 'postjobexecution']; + const locations: Array<{ executionType: string; runner: string; line: number | null; col: number | null }> = []; + + for (const executionType of executionProperties) { + if (taskData[executionType]) { + Object.keys(taskData[executionType]).forEach(runner => { + if (deprecatedRunners.indexOf(runner) !== -1) { + const runnerLoc = pointerLocation(pointerContext, `/${escapeJsonPointerToken(executionType)}/${escapeJsonPointerToken(runner)}`); + locations.push({ + executionType, + runner, + line: runnerLoc.line, + col: runnerLoc.col, + }); + } + }); + } + } + + if (locations.length === 0) { + if (taskPath) { + console.warn(`${taskPath}(1,1): warning: ${messagePrefix}`); + } else { + console.warn(`warning: ${messagePrefix}`); + } + return; + } + + locations.forEach(location => { + const detail = `${messagePrefix} Deprecated runner '${location.runner}' found in '${location.executionType}'.`; + if (taskPath && location.line !== null && location.col !== null) { + console.warn(`${taskPath}(${location.line},${location.col}): warning: ${detail}`); + } else if (taskPath) { + console.warn(`${taskPath}(1,1): warning: ${detail}`); + } else { + console.warn(`warning: ${detail}`); + } + }); } } @@ -150,15 +248,15 @@ export function validateRunner(taskData: any, allMatchedPaths?: string[]) { * @param taskData the parsed json file * @return list of issues with the json file */ -export function validateTask(taskPath: string, taskData: any, pointers: any): StructuredValidationIssue[] { +export function validateTask(taskPath: string, taskData: any, pointerContext: any): StructuredValidationIssue[] { var vn = taskData.name || taskPath; var issues: StructuredValidationIssue[] = []; - const rootLoc = pointerLocation(pointers, ""); - const idLoc = pointerLocation(pointers, "/id"); - const nameLoc = pointerLocation(pointers, "/name"); - const friendlyNameLoc = pointerLocation(pointers, "/friendlyName"); - const instanceNameFormatLoc = pointerLocation(pointers, "/instanceNameFormat"); + const rootLoc = pointerLocation(pointerContext, ""); + const idLoc = pointerLocation(pointerContext, "/id"); + const nameLoc = pointerLocation(pointerContext, "/name"); + const friendlyNameLoc = pointerLocation(pointerContext, "/friendlyName"); + const instanceNameFormatLoc = pointerLocation(pointerContext, "/instanceNameFormat"); if (!taskData.id || !check.isUUID(taskData.id)) { issues.push(createIssue(taskPath, "id is a required guid", idLoc.line ?? rootLoc.line, idLoc.col ?? rootLoc.col)); @@ -176,7 +274,7 @@ export function validateTask(taskPath: string, taskData: any, pointers: any): St issues.push(createIssue(taskPath, "instanceNameFormat is required", instanceNameFormatLoc.line ?? rootLoc.line, instanceNameFormatLoc.col ?? rootLoc.col)); } - issues.push(...validateAllExecutionHandlers(taskPath, taskData, vn, pointers)); + issues.push(...validateAllExecutionHandlers(taskPath, taskData, vn, pointerContext)); // Fix: Return issues array regardless of whether execution block exists or not // Previously this return was inside the if(taskData.execution) block, causing @@ -191,7 +289,7 @@ export function validateTask(taskPath: string, taskData: any, pointers: any): St * @param vn Name of the task or path * @returns Array of issues found for all handlers */ -function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: string, pointers: any): StructuredValidationIssue[] { +function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: string, pointerContext: any): StructuredValidationIssue[] { const issues: StructuredValidationIssue[] = []; const executionProperties = ['execution', 'prejobexecution', 'postjobexecution']; const supportedRunners = ["Node", "Node10", "Node16", "Node20_1", "Node24", "PowerShell", "PowerShell3", "Process"]; @@ -200,7 +298,7 @@ function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: strin Object.keys(taskData[executionType]).forEach(runner => { if (supportedRunners.indexOf(runner) === -1) return; const target = taskData[executionType][runner]?.target; - issues.push(...validateExecutionTarget(taskPath, vn, executionType, runner, target, pointers)); + issues.push(...validateExecutionTarget(taskPath, vn, executionType, runner, target, pointerContext)); }); } }); @@ -216,10 +314,10 @@ function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: strin * @param target Execution handler's target * @returns Array of issues found for this runner */ -function validateExecutionTarget(taskPath: string, vn: string, executionType: string, runner: string, target: string | undefined, pointers: any): StructuredValidationIssue[] { +function validateExecutionTarget(taskPath: string, vn: string, executionType: string, runner: string, target: string | undefined, pointerContext: any): StructuredValidationIssue[] { const issues: StructuredValidationIssue[] = []; const targetPointer = `/${escapeJsonPointerToken(executionType)}/${escapeJsonPointerToken(runner)}/target`; - const targetLoc = pointerLocation(pointers, targetPointer); + const targetLoc = pointerLocation(pointerContext, targetPointer); if (!target) { issues.push(createIssue(taskPath, `${executionType}.${runner}.target is required`, targetLoc.line, targetLoc.col)); diff --git a/package-lock.json b/package-lock.json index a79b30ab..56982c8b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "tfx-cli", - "version": "0.23.0", + "version": "0.23.1", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "tfx-cli", - "version": "0.23.0", + "version": "0.23.1", "license": "MIT", "dependencies": { "app-root-path": "1.0.0", @@ -17,6 +17,7 @@ "glob": "^11.1.0", "jju": "^1.4.0", "json-in-place": "^1.0.1", + "jsonc-parser": "^3.3.1", "jszip": "^3.10.1", "lodash": "^4.17.21", "minimist": "^1.2.6", @@ -2611,6 +2612,12 @@ "integrity": "sha1-vT7V1+Vgudma0iNPKMpwb7N3t9Q=", "license": "ISC" }, + "node_modules/jsonc-parser": { + "version": "3.3.1", + "resolved": "https://registry.npmjs.org/jsonc-parser/-/jsonc-parser-3.3.1.tgz", + "integrity": "sha512-HUgH65KyejrUFPvHFPbqOY0rsFip3Bo5wb4ngvdi1EpCYWUQDC5V+Y7mZws+DLkr4M//zQJoanu1SP+87Dv1oQ==", + "license": "MIT" + }, "node_modules/jszip": { "version": "3.10.1", "resolved": "https://pkgs.dev.azure.com/mseng/PipelineTools/_packaging/PipelineTools_PublicPackages/npm/registry/jszip/-/jszip-3.10.1.tgz", @@ -6252,6 +6259,11 @@ "resolved": "https://pkgs.dev.azure.com/mseng/PipelineTools/_packaging/PipelineTools_PublicPackages/npm/registry/json-lexer/-/json-lexer-1.1.1.tgz", "integrity": "sha1-vT7V1+Vgudma0iNPKMpwb7N3t9Q=" }, + "jsonc-parser": { + "version": "3.3.1", + "resolved": "https://registry.npmjs.org/jsonc-parser/-/jsonc-parser-3.3.1.tgz", + "integrity": "sha512-HUgH65KyejrUFPvHFPbqOY0rsFip3Bo5wb4ngvdi1EpCYWUQDC5V+Y7mZws+DLkr4M//zQJoanu1SP+87Dv1oQ==" + }, "jszip": { "version": "3.10.1", "resolved": "https://pkgs.dev.azure.com/mseng/PipelineTools/_packaging/PipelineTools_PublicPackages/npm/registry/jszip/-/jszip-3.10.1.tgz", diff --git a/package.json b/package.json index 61373592..f3bb31e9 100644 --- a/package.json +++ b/package.json @@ -48,7 +48,7 @@ "glob": "^11.1.0", "jju": "^1.4.0", "json-in-place": "^1.0.1", - "json-source-map": "^0.6.1", + "jsonc-parser": "^3.3.1", "jszip": "^3.10.1", "lodash": "^4.17.21", "minimist": "^1.2.6", diff --git a/tests/extension-local-tests.ts b/tests/extension-local-tests.ts index db9c4bca..bb094086 100644 --- a/tests/extension-local-tests.ts +++ b/tests/extension-local-tests.ts @@ -331,6 +331,26 @@ describe('Extension Commands - Local Tests', function() { .catch(done); }); + it('should include manifest file location for invalid target diagnostics', function(done) { + const invalidExtensionPath = path.join(samplesPath, 'invalid-extension'); + + execAsyncWithLogging( + `node "${tfxPath}" extension validate --root "${invalidExtensionPath}" --json --no-prompt`, + 'extension validate invalid target json', + ) + .then(({ stdout }) => { + const result = JSON.parse(stdout); + assert(result && result.status === 'error', 'Should return error status in json mode'); + const targetIssue = result.issues.find(issue => issue.message.indexOf('not a recognized target') >= 0); + assert(targetIssue, 'Should include invalid target issue'); + assert(targetIssue.file && targetIssue.file.endsWith('vss-extension.json'), 'Issue should include manifest file'); + assert(targetIssue.line && targetIssue.line > 0, 'Issue should include line when available'); + assert(targetIssue.col && targetIssue.col > 0, 'Issue should include col when available'); + done(); + }) + .catch(done); + }); + it('should report missing task name when name is omitted', function(done) { const tempRoot = path.join(__dirname, '../temp-extensions/missing-name-task-extension'); const taskDir = path.join(tempRoot, 'MissingNameTask'); @@ -905,6 +925,81 @@ describe('Extension Commands - Local Tests', function() { }) .catch(done); }); + + it('should output structured JSON errors for create command failures', function(done) { + const tempRoot = path.join(__dirname, '../temp-extensions/create-json-error'); + const partialsDir = path.join(tempRoot, 'partials'); + const manifestMainPath = path.join(tempRoot, 'vss-extension.json'); + const manifestBadPath = path.join(partialsDir, 'bad-target.json'); + + try { + if (!fs.existsSync(path.dirname(tempRoot))) { + fs.mkdirSync(path.dirname(tempRoot)); + } + if (!fs.existsSync(tempRoot)) { + fs.mkdirSync(tempRoot); + } + if (!fs.existsSync(partialsDir)) { + fs.mkdirSync(partialsDir); + } + + fs.writeFileSync( + manifestMainPath, + JSON.stringify( + { + manifestVersion: 1, + id: 'create-json-error-extension', + publisher: 'fabrikam', + version: '1.0.0', + name: 'Create JSON Error Extension', + targets: [{ id: 'Microsoft.VisualStudio.Services' }], + }, + null, + 2, + ), + ); + fs.writeFileSync( + manifestBadPath, + JSON.stringify( + { + targets: [{ id: 'Invalid.Target' }], + }, + null, + 2, + ), + ); + } catch (setupError) { + done(setupError); + return; + } + + execAsyncWithLogging( + `node "${tfxPath}" extension create --root "${tempRoot}" --manifest-globs vss-extension.json partials/*.json --output-path "${path.join(tempRoot, 'out.vsix')}" --json --no-prompt`, + 'extension create --json failure', + ) + .then(() => { + done(new Error('Create should fail for invalid target')); + }) + .catch((error) => { + const jsonText = error.stdout || error.stderr || error.message; + let payload: any; + try { + payload = JSON.parse(jsonText); + } catch (parseError) { + done(new Error('Expected JSON error output from create --json')); + return; + } + + assert(payload.status === 'error', 'Should emit error status'); + assert(Array.isArray(payload.issues) && payload.issues.length > 0, 'Should include structured issues'); + const targetIssue = payload.issues.find(issue => issue.message.indexOf('not a recognized target') >= 0); + assert(targetIssue, 'Should include invalid target issue'); + assert(targetIssue.file && targetIssue.file.endsWith('bad-target.json'), 'Issue should point to offending manifest'); + assert(targetIssue.line && targetIssue.line > 0, 'Issue should include line'); + assert(targetIssue.col && targetIssue.col > 0, 'Issue should include col'); + done(); + }); + }); }); describe('Extension Creation - File System Edge Cases', function() { From 79c48f39fb8d42429848a67842aa8af0b0ea17d8 Mon Sep 17 00:00:00 2001 From: Jesse Houwing Date: Mon, 23 Feb 2026 00:22:30 +0100 Subject: [PATCH 3/4] Refactor diagnostics handling: introduce formatDiagnostic and normalizeIssue functions, replace inline formatting with these utilities across multiple files. --- app/exec/extension/_lib/merger.ts | 140 ++++++------------------------ app/exec/extension/default.ts | 24 +---- app/exec/extension/validate.ts | 16 +--- app/lib/diagnostics.ts | 32 +++++++ app/lib/errorhandler.ts | 28 +----- app/lib/jsoncLocation.ts | 63 ++++++++++++++ app/lib/jsonvalidate.ts | 99 +++++---------------- 7 files changed, 150 insertions(+), 252 deletions(-) create mode 100644 app/lib/diagnostics.ts create mode 100644 app/lib/jsoncLocation.ts diff --git a/app/exec/extension/_lib/merger.ts b/app/exec/extension/_lib/merger.ts index e0e02049..9d22b1e5 100644 --- a/app/exec/extension/_lib/merger.ts +++ b/app/exec/extension/_lib/merger.ts @@ -21,6 +21,8 @@ import loc = require("./loc"); import path = require("path"); import trace = require("../../../lib/trace"); import version = require("../../../lib/dynamicVersion"); +import { formatDiagnostic, normalizeIssue } from "../../../lib/diagnostics"; +import { offsetToLineCol, pointerLocation } from "../../../lib/jsoncLocation"; import { promisify } from "util"; import { readdir, readFile, writeFile, lstat } from "fs"; @@ -50,56 +52,6 @@ export class Merger { this.manifestBuilders = []; } - private getPointerLocation(pointerMap: any, pointerPath: string): { line: number | null; col: number | null } { - if (!pointerMap || !pointerMap.root || typeof pointerMap.sourceText !== "string") { - return { line: null, col: null }; - } - - const segments = pointerPath === "" - ? [] - : pointerPath - .split("/") - .slice(1) - .map(token => token.replace(/~1/g, "/").replace(/~0/g, "~")) - .map(token => /^\d+$/.test(token) ? parseInt(token, 10) : token); - - const node = jsonc.findNodeAtLocation(pointerMap.root, segments); - if (!node) { - return { line: null, col: null }; - } - - const locationNode = node.parent && node.parent.type === "property" ? node.parent : node; - const location = this.offsetToLineCol(pointerMap.sourceText, locationNode.offset); - - return { - line: location.line, - col: location.col, - }; - } - - private offsetToLineCol(text: string, offset: number): { line: number; col: number } { - let line = 1; - let col = 1; - - for (let i = 0; i < offset && i < text.length; i++) { - const ch = text.charCodeAt(i); - if (ch === 13) { - if (i + 1 < text.length && text.charCodeAt(i + 1) === 10) { - i++; - } - line++; - col = 1; - } else if (ch === 10) { - line++; - col = 1; - } else { - col++; - } - } - - return { line, col }; - } - private parseManifestText(jsonData: string): { data: any; pointers: any } { try { const parseErrors: jsonc.ParseError[] = []; @@ -220,7 +172,7 @@ export class Merger { let line: number | null = null; let col: number | null = null; if (Array.isArray(parseErrors) && parseErrors.length > 0 && typeof parseErrors[0].offset === "number") { - const loc = this.offsetToLineCol(jsonData, parseErrors[0].offset); + const loc = offsetToLineCol(jsonData, parseErrors[0].offset); line = loc.line; col = loc.col; } @@ -253,7 +205,7 @@ export class Merger { partials.forEach(partial => { if (_.isArray(partial["targets"])) { partial["targets"].forEach((target: any, targetIndex: number) => { - const idLoc = this.getPointerLocation(partial.__pointerMap, `/targets/${targetIndex}/id`); + const idLoc = pointerLocation(partial.__pointerMap, `/targets/${targetIndex}/id`); const targetWithSource: any = _.assign({}, target, { __origin: partial.__origin || null, __line: idLoc.line, @@ -264,11 +216,11 @@ export class Merger { } if (_.isArray(partial["contributions"])) { partial["contributions"].forEach((contribution: any, contributionIndex: number) => { - const nameLoc = this.getPointerLocation( + const nameLoc = pointerLocation( partial.__pointerMap, `/contributions/${contributionIndex}/properties/name`, ); - const contributionLoc = this.getPointerLocation( + const contributionLoc = pointerLocation( partial.__pointerMap, `/contributions/${contributionIndex}`, ); @@ -528,26 +480,11 @@ export class Merger { private async validateBuildTaskContributions(contributions: any[]): Promise { const warnings: Array<{ file: string | null; line: number | null; col: number | null; message: string }> = []; - const formatIssueForEditor = ( - issue: { file: string | null; line: number | null; col: number | null; message: string }, - severity: "warning" | "error" = "warning", - ): string => { - if (issue.file && issue.line !== null && issue.col !== null) { - return `${issue.file}(${issue.line},${issue.col}): ${severity}: ${issue.message}`; - } - if (issue.file && issue.line !== null) { - return `${issue.file}(${issue.line}): ${severity}: ${issue.message}`; - } - if (issue.file) { - return `${issue.file}: ${severity}: ${issue.message}`; - } - return `${severity}: ${issue.message}`; - }; const addWarning = (message: string, file: string | null = null, line: number | null = null, col: number | null = null) => { const warning = { file, line, col, message }; warnings.push(warning); - console.warn(formatIssueForEditor(warning, "warning")); + console.warn(formatDiagnostic(warning, "warning")); }; try { @@ -640,6 +577,9 @@ export class Merger { } catch (err) { const warningMessage = "Please, make sure the task.json file is correct. In the future, this warning will be treated as an error.\n"; + const emitWarning = (issue: { file: string | null; line: number | null; col: number | null; message: string }) => { + console.warn(formatDiagnostic(issue, "warning")); + }; if (this.settings.warningsAsErrors) { if (err && err instanceof Error) { throw err; @@ -648,52 +588,26 @@ export class Merger { } const structuredIssues = err && Array.isArray((err).validationIssues) ? (err).validationIssues : null; if (structuredIssues && structuredIssues.length > 0) { - const warningByFile: { [file: string]: boolean } = {}; - structuredIssues.forEach(issue => { - const fileKey = issue && issue.file ? String(issue.file) : ""; - if (!warningByFile[fileKey]) { - warningByFile[fileKey] = true; - console.warn( - formatIssueForEditor( - { - file: issue && issue.file !== undefined ? issue.file : null, - line: 1, - col: 1, - message: warningMessage.trim(), - }, - "warning", - ), - ); + const normalizedIssues = structuredIssues.map(issue => normalizeIssue(issue)); + const warnedFiles: { [file: string]: boolean } = {}; + normalizedIssues.forEach(issue => { + const fileKey = issue.file ? String(issue.file) : ""; + if (!warnedFiles[fileKey]) { + warnedFiles[fileKey] = true; + emitWarning({ file: issue.file, line: 1, col: 1, message: warningMessage.trim() }); } - }); - structuredIssues.forEach(issue => { - console.warn( - formatIssueForEditor( - { - file: issue && issue.file !== undefined ? issue.file : null, - line: issue && issue.line !== undefined ? issue.line : null, - col: issue && issue.col !== undefined ? issue.col : null, - message: issue && issue.message ? issue.message : String(issue), - }, - "warning", - ), - ); + emitWarning(issue); }); } else { - console.warn( - formatIssueForEditor( - { - file: null, - line: null, - col: null, - message: - err && err instanceof Error - ? `${warningMessage}${err.message}` - : `Error occurred while validating build task contributions. ${warningMessage}`, - }, - "warning", - ), - ); + emitWarning({ + file: null, + line: null, + col: null, + message: + err && err instanceof Error + ? `${warningMessage}${err.message}` + : `Error occurred while validating build task contributions. ${warningMessage}`, + }); } } } diff --git a/app/exec/extension/default.ts b/app/exec/extension/default.ts index ff54f567..59105b9b 100644 --- a/app/exec/extension/default.ts +++ b/app/exec/extension/default.ts @@ -10,34 +10,12 @@ import * as jsonc from "jsonc-parser"; import args = require("../../lib/arguments"); import https = require("https"); import trace = require("../../lib/trace"); +import { offsetToLineCol } from "../../lib/jsoncLocation"; import { readFile } from "fs"; import { promisify } from "util"; import { GalleryApi } from "azure-devops-node-api/GalleryApi"; -function offsetToLineCol(text: string, offset: number): { line: number; col: number } { - let line = 1; - let col = 1; - - for (let i = 0; i < offset && i < text.length; i++) { - const ch = text.charCodeAt(i); - if (ch === 13) { - if (i + 1 < text.length && text.charCodeAt(i + 1) === 10) { - i++; - } - line++; - col = 1; - } else if (ch === 10) { - line++; - col = 1; - } else { - col++; - } - } - - return { line, col }; -} - export function getCommand(args: string[]): TfCommand { return new ExtensionBase(args); } diff --git a/app/exec/extension/validate.ts b/app/exec/extension/validate.ts index 162bd3f1..01d39b16 100644 --- a/app/exec/extension/validate.ts +++ b/app/exec/extension/validate.ts @@ -3,6 +3,7 @@ import { TfCommand } from "../../lib/tfcommand"; import colors = require("colors"); import extBase = require("./default"); import trace = require("../../lib/trace"); +import { formatDiagnostic } from "../../lib/diagnostics"; export function getCommand(args: string[]): TfCommand { return new ExtensionValidate(args); @@ -21,19 +22,6 @@ export interface ValidationResult { issues: ValidationIssue[]; } -function formatIssueForEditor(issue: ValidationIssue): string { - if (issue.file && issue.line !== null && issue.col !== null) { - return `${issue.file}(${issue.line},${issue.col}): error: ${issue.message}`; - } - if (issue.file && issue.line !== null) { - return `${issue.file}(${issue.line}): error: ${issue.message}`; - } - if (issue.file) { - return `${issue.file}: error: ${issue.message}`; - } - return `error: ${issue.message}`; -} - export class ExtensionValidate extends extBase.ExtensionBase { protected description = "Validate an extension from manifests without packaging or publishing."; @@ -118,7 +106,7 @@ export class ExtensionValidate extends extBase.ExtensionBase { trace.info(" - Source: %s", data.source); trace.info(" - Validation: %s", colors.red("failed")); data.issues.forEach(issue => { - trace.info(formatIssueForEditor(issue)); + trace.info(formatDiagnostic(issue, "error")); }); } } diff --git a/app/lib/diagnostics.ts b/app/lib/diagnostics.ts new file mode 100644 index 00000000..e890a475 --- /dev/null +++ b/app/lib/diagnostics.ts @@ -0,0 +1,32 @@ +export interface DiagnosticIssue { + file: string | null; + line: number | null; + col: number | null; + message: string; +} + +export type DiagnosticSeverity = "warning" | "error"; + +export function normalizeIssue(issue: any): DiagnosticIssue { + return { + file: issue && issue.file !== undefined ? String(issue.file) : null, + line: issue && typeof issue.line === "number" ? issue.line : null, + col: issue && typeof issue.col === "number" ? issue.col : null, + message: issue && issue.message ? String(issue.message) : String(issue), + }; +} + +export function formatDiagnostic(issue: any, severity: DiagnosticSeverity = "error"): string { + const normalized = normalizeIssue(issue); + + if (normalized.file && normalized.line !== null && normalized.col !== null) { + return `${normalized.file}(${normalized.line},${normalized.col}): ${severity}: ${normalized.message}`; + } + if (normalized.file && normalized.line !== null) { + return `${normalized.file}(${normalized.line}): ${severity}: ${normalized.message}`; + } + if (normalized.file) { + return `${normalized.file}: ${severity}: ${normalized.message}`; + } + return `${severity}: ${normalized.message}`; +} diff --git a/app/lib/errorhandler.ts b/app/lib/errorhandler.ts index 371125c1..e044d19e 100644 --- a/app/lib/errorhandler.ts +++ b/app/lib/errorhandler.ts @@ -1,4 +1,5 @@ import trace = require("./trace"); +import { formatDiagnostic, normalizeIssue } from "./diagnostics"; function shouldEmitJsonError(): boolean { const argv = process.argv || []; @@ -21,30 +22,7 @@ function toStructuredIssues(err: any): any[] { if (!err || !Array.isArray(err.validationIssues)) { return null; } - return err.validationIssues.map(issue => ({ - file: issue && issue.file !== undefined ? issue.file : null, - line: issue && issue.line !== undefined ? issue.line : null, - col: issue && issue.col !== undefined ? issue.col : null, - message: issue && issue.message ? issue.message : String(issue), - })); -} - -function formatIssueForEditor(issue: any): string { - const file = issue && issue.file ? String(issue.file) : null; - const line = issue && typeof issue.line === "number" ? issue.line : null; - const col = issue && typeof issue.col === "number" ? issue.col : null; - const message = issue && issue.message ? String(issue.message) : String(issue); - - if (file && line !== null && col !== null) { - return `${file}(${line},${col}): error: ${message}`; - } - if (file && line !== null) { - return `${file}(${line}): error: ${message}`; - } - if (file) { - return `${file}: error: ${message}`; - } - return `error: ${message}`; + return err.validationIssues.map(issue => normalizeIssue(issue)); } /** @@ -154,7 +132,7 @@ export function errLog(arg: any): void { const issues = toStructuredIssues(arg); if (issues && issues.length > 0) { issues.forEach(issue => { - console.error(formatIssueForEditor(issue)); + console.error(formatDiagnostic(issue, "error")); }); process.exit(-1); return; diff --git a/app/lib/jsoncLocation.ts b/app/lib/jsoncLocation.ts new file mode 100644 index 00000000..0588c1f1 --- /dev/null +++ b/app/lib/jsoncLocation.ts @@ -0,0 +1,63 @@ +import * as jsonc from "jsonc-parser"; + +export interface JsoncPointerContext { + sourceText: string; + root: any; +} + +export function escapeJsonPointerToken(token: string): string { + return token.replace(/~/g, "~0").replace(/\//g, "~1"); +} + +function decodeJsonPointerToken(token: string): string { + return token.replace(/~1/g, "/").replace(/~0/g, "~"); +} + +export function pointerLocation( + pointerContext: JsoncPointerContext | null | undefined, + pointerPath: string, +): { line: number | null; col: number | null } { + if (!pointerContext || !pointerContext.root || typeof pointerContext.sourceText !== "string") { + return { line: null, col: null }; + } + + const segments = + pointerPath === "" + ? [] + : pointerPath + .split("/") + .slice(1) + .map(token => decodeJsonPointerToken(token)) + .map(token => (/^\d+$/.test(token) ? parseInt(token, 10) : token)); + + const node = jsonc.findNodeAtLocation(pointerContext.root, segments); + if (!node) { + return { line: null, col: null }; + } + + const locationNode = node.parent && node.parent.type === "property" ? node.parent : node; + return offsetToLineCol(pointerContext.sourceText, locationNode.offset); +} + +export function offsetToLineCol(text: string, offset: number): { line: number; col: number } { + let line = 1; + let col = 1; + + for (let i = 0; i < offset && i < text.length; i++) { + const ch = text.charCodeAt(i); + if (ch === 13) { + if (i + 1 < text.length && text.charCodeAt(i + 1) === 10) { + i++; + } + line++; + col = 1; + } else if (ch === 10) { + line++; + col = 1; + } else { + col++; + } + } + + return { line, col }; +} diff --git a/app/lib/jsonvalidate.ts b/app/lib/jsonvalidate.ts index d73d4d5f..ec3d9616 100644 --- a/app/lib/jsonvalidate.ts +++ b/app/lib/jsonvalidate.ts @@ -3,16 +3,11 @@ import * as path from 'path'; var check = require("validator"); var trace = require("./trace"); import * as jsonc from "jsonc-parser"; +import { DiagnosticIssue, formatDiagnostic } from "./diagnostics"; +import { escapeJsonPointerToken, offsetToLineCol, pointerLocation } from "./jsoncLocation"; const deprecatedRunners = ["Node", "Node6", "Node10", "Node16"]; -export interface StructuredValidationIssue { - file: string | null; - line: number | null; - col: number | null; - message: string; -} - export interface TaskJson { id: string; } @@ -84,59 +79,6 @@ export function validate(jsonFilePath: string, jsonMissingErrorMessage?: string, return taskJson; } -function createIssue(file: string, message: string, line: number | null = null, col: number | null = null): StructuredValidationIssue { - return { file, line, col, message }; -} - -function escapeJsonPointerToken(token: string): string { - return token.replace(/~/g, "~0").replace(/\//g, "~1"); -} - -function pointerLocation(pointerContext: any, pointerPath: string): { line: number | null; col: number | null } { - if (!pointerContext || !pointerContext.root || typeof pointerContext.sourceText !== "string") { - return { line: null, col: null }; - } - - const segments = pointerPath === "" - ? [] - : pointerPath - .split("/") - .slice(1) - .map(token => token.replace(/~1/g, "/").replace(/~0/g, "~")) - .map(token => /^\d+$/.test(token) ? parseInt(token, 10) : token); - - const node = jsonc.findNodeAtLocation(pointerContext.root, segments); - if (!node) { - return { line: null, col: null }; - } - - const locationNode = node.parent && node.parent.type === "property" ? node.parent : node; - return offsetToLineCol(pointerContext.sourceText, locationNode.offset); -} - -function offsetToLineCol(text: string, offset: number): { line: number; col: number } { - let line = 1; - let col = 1; - - for (let i = 0; i < offset && i < text.length; i++) { - const ch = text.charCodeAt(i); - if (ch === 13) { - if (i + 1 < text.length && text.charCodeAt(i + 1) === 10) { - i++; - } - line++; - col = 1; - } else if (ch === 10) { - line++; - col = 1; - } else { - col++; - } - } - - return { line, col }; -} - /* * Wrapper for fs.existsSync that includes a user-specified errorMessage in an InvalidDirectoryException */ @@ -231,12 +173,15 @@ export function validateRunner(taskData: any, allMatchedPaths?: string[], taskPa locations.forEach(location => { const detail = `${messagePrefix} Deprecated runner '${location.runner}' found in '${location.executionType}'.`; - if (taskPath && location.line !== null && location.col !== null) { - console.warn(`${taskPath}(${location.line},${location.col}): warning: ${detail}`); - } else if (taskPath) { - console.warn(`${taskPath}(1,1): warning: ${detail}`); + if (taskPath) { + console.warn( + formatDiagnostic( + { file: taskPath, line: location.line ?? 1, col: location.col ?? 1, message: detail }, + "warning", + ), + ); } else { - console.warn(`warning: ${detail}`); + console.warn(formatDiagnostic({ file: null, line: null, col: null, message: detail }, "warning")); } }); } @@ -248,9 +193,9 @@ export function validateRunner(taskData: any, allMatchedPaths?: string[], taskPa * @param taskData the parsed json file * @return list of issues with the json file */ -export function validateTask(taskPath: string, taskData: any, pointerContext: any): StructuredValidationIssue[] { +export function validateTask(taskPath: string, taskData: any, pointerContext: any): DiagnosticIssue[] { var vn = taskData.name || taskPath; - var issues: StructuredValidationIssue[] = []; + var issues: DiagnosticIssue[] = []; const rootLoc = pointerLocation(pointerContext, ""); const idLoc = pointerLocation(pointerContext, "/id"); @@ -259,19 +204,19 @@ export function validateTask(taskPath: string, taskData: any, pointerContext: an const instanceNameFormatLoc = pointerLocation(pointerContext, "/instanceNameFormat"); if (!taskData.id || !check.isUUID(taskData.id)) { - issues.push(createIssue(taskPath, "id is a required guid", idLoc.line ?? rootLoc.line, idLoc.col ?? rootLoc.col)); + issues.push({ file: taskPath, line: idLoc.line ?? rootLoc.line, col: idLoc.col ?? rootLoc.col, message: "id is a required guid" }); } if (!taskData.name || !check.matches(taskData.name, /^[A-Za-z0-9\-]+$/)) { - issues.push(createIssue(taskPath, "name is a required alphanumeric string", nameLoc.line ?? rootLoc.line, nameLoc.col ?? rootLoc.col)); + issues.push({ file: taskPath, line: nameLoc.line ?? rootLoc.line, col: nameLoc.col ?? rootLoc.col, message: "name is a required alphanumeric string" }); } if (!taskData.friendlyName || !check.isLength(taskData.friendlyName, 1, 40)) { - issues.push(createIssue(taskPath, "friendlyName is a required string <= 40 chars", friendlyNameLoc.line ?? rootLoc.line, friendlyNameLoc.col ?? rootLoc.col)); + issues.push({ file: taskPath, line: friendlyNameLoc.line ?? rootLoc.line, col: friendlyNameLoc.col ?? rootLoc.col, message: "friendlyName is a required string <= 40 chars" }); } if (!taskData.instanceNameFormat) { - issues.push(createIssue(taskPath, "instanceNameFormat is required", instanceNameFormatLoc.line ?? rootLoc.line, instanceNameFormatLoc.col ?? rootLoc.col)); + issues.push({ file: taskPath, line: instanceNameFormatLoc.line ?? rootLoc.line, col: instanceNameFormatLoc.col ?? rootLoc.col, message: "instanceNameFormat is required" }); } issues.push(...validateAllExecutionHandlers(taskPath, taskData, vn, pointerContext)); @@ -289,8 +234,8 @@ export function validateTask(taskPath: string, taskData: any, pointerContext: an * @param vn Name of the task or path * @returns Array of issues found for all handlers */ -function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: string, pointerContext: any): StructuredValidationIssue[] { - const issues: StructuredValidationIssue[] = []; +function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: string, pointerContext: any): DiagnosticIssue[] { + const issues: DiagnosticIssue[] = []; const executionProperties = ['execution', 'prejobexecution', 'postjobexecution']; const supportedRunners = ["Node", "Node10", "Node16", "Node20_1", "Node24", "PowerShell", "PowerShell3", "Process"]; executionProperties.forEach(executionType => { @@ -314,13 +259,13 @@ function validateAllExecutionHandlers(taskPath: string, taskData: any, vn: strin * @param target Execution handler's target * @returns Array of issues found for this runner */ -function validateExecutionTarget(taskPath: string, vn: string, executionType: string, runner: string, target: string | undefined, pointerContext: any): StructuredValidationIssue[] { - const issues: StructuredValidationIssue[] = []; +function validateExecutionTarget(taskPath: string, vn: string, executionType: string, runner: string, target: string | undefined, pointerContext: any): DiagnosticIssue[] { + const issues: DiagnosticIssue[] = []; const targetPointer = `/${escapeJsonPointerToken(executionType)}/${escapeJsonPointerToken(runner)}/target`; const targetLoc = pointerLocation(pointerContext, targetPointer); if (!target) { - issues.push(createIssue(taskPath, `${executionType}.${runner}.target is required`, targetLoc.line, targetLoc.col)); + issues.push({ file: taskPath, line: targetLoc.line, col: targetLoc.col, message: `${executionType}.${runner}.target is required` }); } else { const normalizedTarget = target.replace(/\$\(\s*currentdirectory\s*\)/i, "."); @@ -331,7 +276,7 @@ function validateExecutionTarget(taskPath: string, vn: string, executionType: st // check if the target file exists if (!fs.existsSync(path.join(path.dirname(taskPath), normalizedTarget))) { - issues.push(createIssue(taskPath, `${executionType}.${runner}.target references file that does not exist: ${normalizedTarget}`, targetLoc.line, targetLoc.col)); + issues.push({ file: taskPath, line: targetLoc.line, col: targetLoc.col, message: `${executionType}.${runner}.target references file that does not exist: ${normalizedTarget}` }); } } return issues; From bac445cb7294d9a75b57e2053b5f0b8c77cab0f9 Mon Sep 17 00:00:00 2001 From: Jesse Houwing Date: Mon, 23 Feb 2026 00:27:46 +0100 Subject: [PATCH 4/4] test(build): fix flaky task.json format integration test timeouts Align the invalid task.json server-integration assertion with current CLI diagnostic output and harden async error handling in done-style tests. remove dependency on legacy "Invalid task json" prefix assert on field-level validation errors (id, name, friendlyName) wrap assertion logic in try/catch inside rejection handlers call done(assertionError) on assertion failures to fail fast (not timeout) apply the same catch-block hardening pattern across related build server tests --- tests/build-server-integration-tests.ts | 115 ++++++++++++++++-------- 1 file changed, 77 insertions(+), 38 deletions(-) diff --git a/tests/build-server-integration-tests.ts b/tests/build-server-integration-tests.ts index a2521176..95debfb8 100644 --- a/tests/build-server-integration-tests.ts +++ b/tests/build-server-integration-tests.ts @@ -190,11 +190,15 @@ describe('Build Commands - Server Integration Tests', function() { assert.fail('Should have failed without build ID'); }) .catch((error) => { - const errorOutput = stripColors(error.stderr || error.stdout || ''); + try { + const errorOutput = stripColors(error.stderr || error.stdout || ''); - // Verify specific error message format - assert(errorOutput.includes('error: Error: Missing required value for argument \'buildId\''), 'Should show specific buildId requirement error'); - done(); + // Verify specific error message format + assert(errorOutput.includes('error: Error: Missing required value for argument \'buildId\''), 'Should show specific buildId requirement error'); + done(); + } catch (assertionError) { + done(assertionError); + } }); }); }); @@ -256,11 +260,15 @@ describe('Build Commands - Server Integration Tests', function() { assert.fail('Should have failed without definition'); }) .catch((error) => { - const errorOutput = stripColors(error.stderr || error.stdout || ''); + try { + const errorOutput = stripColors(error.stderr || error.stdout || ''); - // Verify specific error message format - assert(errorOutput.includes('error: Error: No definition found with name null'), 'Should show specific definition requirement error'); - done(); + // Verify specific error message format + assert(errorOutput.includes('error: Error: No definition found with name null'), 'Should show specific definition requirement error'); + done(); + } catch (assertionError) { + done(assertionError); + } }); }); }); @@ -296,10 +304,14 @@ describe('Build Commands - Server Integration Tests', function() { assert.fail('Should have failed without authentication'); }) .catch((error) => { - const errorOutput = stripColors(error.stderr || error.stdout || ''); - // Check for specific error message about missing token - assert(errorOutput.includes("error: Error: Missing required value for argument 'token'."), 'Should show missing token error'); - done(); + try { + const errorOutput = stripColors(error.stderr || error.stdout || ''); + // Check for specific error message about missing token + assert(errorOutput.includes("error: Error: Missing required value for argument 'token'."), 'Should show missing token error'); + done(); + } catch (assertionError) { + done(assertionError); + } }); }); }); @@ -339,10 +351,14 @@ describe('Build Commands - Server Integration Tests', function() { } catch (e) { // Ignore cleanup errors } - const errorOutput = stripColors(error.stderr || error.stdout || ''); - // Should fail with specific error message - assert(errorOutput.includes('error: Error: no task.json in specified directory'), 'Should indicate task.json is missing'); - done(); + try { + const errorOutput = stripColors(error.stderr || error.stdout || ''); + // Should fail with specific error message + assert(errorOutput.includes('error: Error: no task.json in specified directory'), 'Should indicate task.json is missing'); + done(); + } catch (assertionError) { + done(assertionError); + } }); }); @@ -374,13 +390,16 @@ describe('Build Commands - Server Integration Tests', function() { done(); }) .catch((error) => { - const errorOutput = stripColors(error.stderr || error.stdout || ''); - // Should fail with specific error messages for invalid fields - assert(errorOutput.includes('error: Error: Invalid task json:'), 'Should indicate invalid task json'); - assert(errorOutput.includes('id is a required guid'), 'Should indicate missing id'); - assert(errorOutput.includes('name is a required alphanumeric string'), 'Should indicate missing name'); - assert(errorOutput.includes('friendlyName is a required string <= 40 chars'), 'Should indicate missing friendlyName'); - done(); + try { + const errorOutput = stripColors(error.stderr || error.stdout || ''); + // Should fail with specific error messages for invalid fields + assert(errorOutput.includes('id is a required guid'), 'Should indicate missing id'); + assert(errorOutput.includes('name is a required alphanumeric string'), 'Should indicate missing name'); + assert(errorOutput.includes('friendlyName is a required string <= 40 chars'), 'Should indicate missing friendlyName'); + done(); + } catch (assertionError) { + done(assertionError); + } }); }); @@ -392,9 +411,13 @@ describe('Build Commands - Server Integration Tests', function() { assert.fail('Should have failed without task path'); }) .catch((error) => { - const errorOutput = stripColors(error.stderr || error.stdout || ''); - assert(errorOutput.includes('error: Error: You must specify either --task-path or --task-zip-path.'), 'Should indicate task path is required'); - done(); + try { + const errorOutput = stripColors(error.stderr || error.stdout || ''); + assert(errorOutput.includes('error: Error: You must specify either --task-path or --task-zip-path.'), 'Should indicate task path is required'); + done(); + } catch (assertionError) { + done(assertionError); + } }); }); }); @@ -424,9 +447,13 @@ describe('Build Commands - Server Integration Tests', function() { assert.fail('Should have failed without task ID'); }) .catch((error) => { - const errorOutput = stripColors(error.stderr || error.stdout || ''); - assert(errorOutput.includes("error: Error: Missing required value for argument 'taskId'."), 'Should indicate task ID is required'); - done(); + try { + const errorOutput = stripColors(error.stderr || error.stdout || ''); + assert(errorOutput.includes("error: Error: Missing required value for argument 'taskId'."), 'Should indicate task ID is required'); + done(); + } catch (assertionError) { + done(assertionError); + } }); }); @@ -438,9 +465,13 @@ describe('Build Commands - Server Integration Tests', function() { done(); }) .catch((error) => { - const errorOutput = stripColors(error.stderr || error.stdout || ''); - assert(errorOutput.includes('error: Error: No task found with provided ID: invalid-task-id'), 'Should indicate no task found for invalid ID'); - done(); + try { + const errorOutput = stripColors(error.stderr || error.stdout || ''); + assert(errorOutput.includes('error: Error: No task found with provided ID: invalid-task-id'), 'Should indicate no task found for invalid ID'); + done(); + } catch (assertionError) { + done(assertionError); + } }); }); }); @@ -563,9 +594,13 @@ describe('Build Commands - Server Integration Tests', function() { assert.fail('Should have failed without project'); }) .catch((error) => { - const errorOutput = stripColors(error.stderr || error.stdout || ''); - assert(errorOutput.includes("error: Error: Missing required value for argument 'project'."), 'Should indicate project is required'); - done(); + try { + const errorOutput = stripColors(error.stderr || error.stdout || ''); + assert(errorOutput.includes("error: Error: Missing required value for argument 'project'."), 'Should indicate project is required'); + done(); + } catch (assertionError) { + done(assertionError); + } }); }); @@ -577,9 +612,13 @@ describe('Build Commands - Server Integration Tests', function() { done(); }) .catch((error) => { - const errorOutput = stripColors(error.stderr || error.stdout || ''); - assert(errorOutput.includes("error: Error: Unsupported auth type. Currently, 'pat' and 'basic' auth are supported."), 'Should indicate unsupported auth type'); - done(); + try { + const errorOutput = stripColors(error.stderr || error.stdout || ''); + assert(errorOutput.includes("error: Error: Unsupported auth type. Currently, 'pat' and 'basic' auth are supported."), 'Should indicate unsupported auth type'); + done(); + } catch (assertionError) { + done(assertionError); + } }); }); });