From 3a09ca908f16ac769350c67a6d41419c6a213e3c Mon Sep 17 00:00:00 2001 From: JoyerJin <116236375+JoyerJin@users.noreply.github.com> Date: Wed, 28 Oct 2026 18:09:38 +0800 Subject: [PATCH 1/9] Add Vally argument forwarding and track isolation to common eval templates Support extensible eval pipeline templates --- .../pipelines/templates/jobs/eval-shard.yml | 32 ++++++++-- .../templates/jobs/eval-summarize.yml | 25 +++++++- .../templates/stages/archetype-eval.yml | 8 +++ .../pipelines/templates/steps/eval-invoke.yml | 6 ++ eng/common/scripts/eval/README.md | 41 ++++++++++++ eng/common/scripts/eval/build-eval-summary.ts | 3 +- eng/common/scripts/eval/invoke-eval-shard.ts | 54 +++++++++++----- eng/common/scripts/eval/lib/shard-results.ts | 18 ++++-- .../scripts/eval/test/shard-results.test.ts | 59 ++++++++++++++++- eng/common/scripts/eval/test/verdict.test.ts | 63 +++++++++++++++++++ 10 files changed, 281 insertions(+), 28 deletions(-) diff --git a/eng/common/pipelines/templates/jobs/eval-shard.yml b/eng/common/pipelines/templates/jobs/eval-shard.yml index 569f7d4fdf..032484f3df 100644 --- a/eng/common/pipelines/templates/jobs/eval-shard.yml +++ b/eng/common/pipelines/templates/jobs/eval-shard.yml @@ -6,10 +6,24 @@ parameters: - name: vallyRoot # Required (no default) so each repo passes its own eval root explicitly. type: string + - name: matrixStageName + # Stage containing the generate_eval_matrix job. + type: string + default: Prepare + - name: artifactNamespace + # Optional track namespace (e.g. 'forced'); common adds the separator. + type: string + default: '' - name: evalInvokeTemplate # Swap point: runs `vally eval` for the shard (the common template is auth-conditional). type: string default: /eng/common/pipelines/templates/steps/eval-invoke.yml + - name: vallyArguments + type: object + default: [] + - name: azureSubscription + type: string + default: opensource-api-connection - name: UseAzSdkAuthentication # Forwarded to the invoke template: true runs the shard under AzureCLI@2. type: boolean @@ -44,7 +58,7 @@ jobs: workspace: clean: all strategy: - matrix: $[ stageDependencies.Prepare.generate_eval_matrix.outputs['detect.matrix'] ] + matrix: $[ stageDependencies.${{ parameters.matrixStageName }}.generate_eval_matrix.outputs['detect.matrix'] ] maxParallel: 10 templateContext: outputs: @@ -53,12 +67,18 @@ jobs: # The Summary job keeps only the highest attempt per shard so retries supersede cleanly. - output: pipelineArtifact displayName: 'Publish $(shardName) raw results, JUnit and completeness' - artifactName: 'eval-result-$(shardName)-$(System.JobAttempt)' + ${{ if eq(parameters.artifactNamespace, '') }}: + artifactName: 'eval-result-$(shardName)-$(System.JobAttempt)' + ${{ else }}: + artifactName: 'eval-result-${{ parameters.artifactNamespace }}-$(shardName)-$(System.JobAttempt)' targetPath: $(Build.ArtifactStagingDirectory)/eval-result condition: always() - output: pipelineArtifact displayName: 'Publish $(shardName) transcripts (on failure)' - artifactName: 'eval-debug-$(shardName)-$(System.JobAttempt)' + ${{ if eq(parameters.artifactNamespace, '') }}: + artifactName: 'eval-debug-$(shardName)-$(System.JobAttempt)' + ${{ else }}: + artifactName: 'eval-debug-${{ parameters.artifactNamespace }}-$(shardName)-$(System.JobAttempt)' targetPath: $(Build.ArtifactStagingDirectory)/debug condition: failed() steps: @@ -113,12 +133,16 @@ jobs: - template: /eng/common/pipelines/templates/steps/login-to-copilot.yml - # Runs the eval via the invoke template (auth-conditional in the common default). - template: ${{ parameters.evalInvokeTemplate }} parameters: vallyRoot: ${{ parameters.vallyRoot }} UseAzSdkAuthentication: ${{ parameters.UseAzSdkAuthentication }} threshold: ${{ parameters.threshold }} + # Existing custom templates keep their contract unless opting into the argument list. + ${{ if eq(parameters.evalInvokeTemplate, '/eng/common/pipelines/templates/steps/eval-invoke.yml') }}: + azureSubscription: ${{ parameters.azureSubscription }} + ${{ if ne(length(parameters.vallyArguments), 0) }}: + vallyArguments: ${{ parameters.vallyArguments }} # Optional repo-specific teardown after the eval. - ${{ if parameters.postEvalTemplate }}: diff --git a/eng/common/pipelines/templates/jobs/eval-summarize.yml b/eng/common/pipelines/templates/jobs/eval-summarize.yml index eddc71c6e9..d97717d736 100644 --- a/eng/common/pipelines/templates/jobs/eval-summarize.yml +++ b/eng/common/pipelines/templates/jobs/eval-summarize.yml @@ -2,6 +2,15 @@ # gates on it when failOnFailedTests is true. parameters: + - name: matrixStageName + type: string + default: Prepare + - name: evalStageName + type: string + default: Eval + - name: artifactNamespace + type: string + default: '' - name: failOnFailedTests type: boolean default: false @@ -22,15 +31,25 @@ jobs: workspace: clean: all variables: - EvalExpectedMatrix: $[ stageDependencies.Prepare.generate_eval_matrix.outputs['detect.matrix'] ] + EvalExpectedMatrix: $[ stageDependencies.${{ parameters.matrixStageName }}.generate_eval_matrix.outputs['detect.matrix'] ] templateContext: outputs: - output: pipelineArtifact displayName: 'Retain selected build summary and saved bundle' - artifactName: 'eval-summary-$(System.JobAttempt)' + ${{ if eq(parameters.artifactNamespace, '') }}: + artifactName: 'eval-summary-$(System.JobAttempt)' + ${{ else }}: + artifactName: 'eval-summary-${{ parameters.artifactNamespace }}-$(System.JobAttempt)' targetPath: $(Build.ArtifactStagingDirectory)/eval-summary condition: always() steps: + # Dashboard archives identify a single build-wide summary, not separate tracks. + - ${{ if and(ne(parameters.artifactNamespace, ''), or(parameters.createDashboardBundle, parameters.publishDashboardResults)) }}: + - script: | + echo "##vso[task.logissue type=error]Namespaced eval tracks support artifact-only summaries, not dashboard bundles or publication." + exit 1 + displayName: 'Reject ambiguous multi-track dashboard identity' + - checkout: self clean: true fetchDepth: 1 @@ -60,6 +79,7 @@ jobs: env: SYSTEM_ACCESSTOKEN: $(System.AccessToken) EVAL_EXPECTED_MATRIX: $(EvalExpectedMatrix) + EVAL_STAGE_NAME: ${{ parameters.evalStageName }} - script: | node --experimental-strip-types "$(Build.SourcesDirectory)/eng/common/scripts/eval/build-eval-summary.ts" \ @@ -71,6 +91,7 @@ jobs: condition: always() env: EVAL_EXPECTED_MATRIX: $(EvalExpectedMatrix) + EVAL_ARTIFACT_NAMESPACE: ${{ parameters.artifactNamespace }} - ${{ if or(parameters.createDashboardBundle, parameters.publishDashboardResults) }}: - template: ../steps/eval-publish-results.yml diff --git a/eng/common/pipelines/templates/stages/archetype-eval.yml b/eng/common/pipelines/templates/stages/archetype-eval.yml index 128cc16c70..d3d56bf81c 100644 --- a/eng/common/pipelines/templates/stages/archetype-eval.yml +++ b/eng/common/pipelines/templates/stages/archetype-eval.yml @@ -43,6 +43,12 @@ parameters: # When true, the invoke step runs under AzureCLI@2 (live MCP inherits the service connection). type: boolean default: false + - name: azureSubscription + type: string + default: opensource-api-connection + - name: vallyArguments + type: object + default: [] - name: threshold # Pass-rate gate forwarded to `vally eval --threshold` and the shard verdict. Repos may override. type: number @@ -102,6 +108,8 @@ extends: vallyRoot: ${{ parameters.vallyRoot }} evalInvokeTemplate: ${{ parameters.evalInvokeTemplate }} UseAzSdkAuthentication: ${{ parameters.UseAzSdkAuthentication }} + azureSubscription: ${{ parameters.azureSubscription }} + vallyArguments: ${{ parameters.vallyArguments }} threshold: ${{ parameters.threshold }} preEvalTemplate: ${{ parameters.preEvalTemplate }} postEvalTemplate: ${{ parameters.postEvalTemplate }} diff --git a/eng/common/pipelines/templates/steps/eval-invoke.yml b/eng/common/pipelines/templates/steps/eval-invoke.yml index 191765af3f..044c50c719 100644 --- a/eng/common/pipelines/templates/steps/eval-invoke.yml +++ b/eng/common/pipelines/templates/steps/eval-invoke.yml @@ -16,6 +16,10 @@ parameters: - name: azureSubscription type: string default: opensource-api-connection + - name: vallyArguments + # Supplementary --tag/--runs/--workers/--timeout argument pairs, not shell text. + type: object + default: [] steps: - ${{ if parameters.UseAzSdkAuthentication }}: @@ -34,6 +38,7 @@ steps: --threshold ${{ parameters.threshold }} env: COPILOT_GITHUB_TOKEN: $(COPILOT_GITHUB_TOKEN) + VALLY_ARGUMENTS_JSON: ${{ convertToJson(parameters.vallyArguments) }} - ${{ else }}: - script: | @@ -46,3 +51,4 @@ steps: workingDirectory: ${{ parameters.vallyRoot }} env: COPILOT_GITHUB_TOKEN: $(COPILOT_GITHUB_TOKEN) + VALLY_ARGUMENTS_JSON: ${{ convertToJson(parameters.vallyArguments) }} diff --git a/eng/common/scripts/eval/README.md b/eng/common/scripts/eval/README.md index cccbca4809..6e8abb3151 100644 --- a/eng/common/scripts/eval/README.md +++ b/eng/common/scripts/eval/README.md @@ -13,6 +13,47 @@ templates. its own pnpm package/lock for **fflate only** and CLI/PowerShell tests. Shard evaluator restores do not install it; publication adds no Node Azure SDK dependencies. +## Extending a shard job + +The shared `eng/common/pipelines/templates/jobs/eval-shard.yml` owns checkout, +Node/Vally installation, MCP staging, git fixture initialization and Copilot login. +Its defaults preserve the standard Prepare/Eval/Summary pipeline behavior. +Repo-local adapters can customize it without copying that infrastructure: + +| Parameter | Contract | +| --- | --- | +| `matrixStageName` | Stage containing `generate_eval_matrix`; defaults to `Prepare`. The eval stage must depend on it. | +| `artifactNamespace` | Optional track namespace, e.g. `forced`. Common adds the separator: `eval-result-forced--` / `eval-debug-forced--`. Empty preserves existing names. Configure the matching summary with the same namespace. | +| `preEvalTemplate` / `postEvalTemplate` | Existing parameter-free setup/teardown step templates. | +| `evalInvokeTemplate` | Existing invocation template accepting `vallyRoot`, `UseAzSdkAuthentication` and `threshold`. | +| `azureSubscription` | Existing invoke service-connection setting, also forwarded by the stage/job when using the common invoke template. | +| `vallyArguments` | Optional string array of supplementary Vally option/value pairs. Available through the stage, shard job and common invoke template. Defaults to `[]`. | + +Supported supplementary options are `--tag`, `--runs`, `--workers` and `--timeout`. +For example, `['--tag', 'mode=forced', '--runs', '3']` selects a local benchmark +configuration without replacing the common runner. Trial/worker counts must be +positive integer strings; use `${{ format('{0}', parameters.Runs) }}` for numeric +pipeline parameters. Both authentication branches forward the array as JSON in +`VALLY_ARGUMENTS_JSON`; the runner passes literal arguments to the pinned CLI using +Node directly, without a shell or whitespace splitting of supplementary values. +Unsupported flags (including aliases and `--flag=value`) are rejected. Eval +selection, output paths, JUnit, plugins and verdict settings remain framework-owned. +Legacy custom invoke templates retain their existing parameter contract when the +array is empty. A custom template opting into a non-empty array must declare its +own `vallyArguments` parameter; otherwise template compilation rejects it. + +Use the existing pre/post-eval and MCP setup hooks for repository-specific work. +Do not replace authentication, result collection or retry/completeness checks. +Supplementary reports must not rewrite JUnit thresholds or raw results. + +For multiple tracks in one build, use a distinct artifact namespace and configure +the common summary's `matrixStageName`, `evalStageName` and `artifactNamespace` +to match the shard job. Each Summary stage must depend on both its Prepare and +Eval stages. Timeline attempts are checked against that exact Eval stage, and +only that namespace's latest verified attempts are selected. Namespaced tracks +support artifact-only summaries: dashboard bundles/publication are rejected +because the archive identity currently represents one build-wide Summary. + ## Complete build results and direct Blob publishing Shards now retain the newest invocation's raw JSONL, JUnit and a completion marker diff --git a/eng/common/scripts/eval/build-eval-summary.ts b/eng/common/scripts/eval/build-eval-summary.ts index 43f7ef682b..af6d8cc36a 100644 --- a/eng/common/scripts/eval/build-eval-summary.ts +++ b/eng/common/scripts/eval/build-eval-summary.ts @@ -379,7 +379,8 @@ async function main(argv) { } } selection = selectSummaryResults({ resultsRoot: options.resultsRoot, - selectedRoot: options.selectedRoot, expectedShards, jobAttempts }); + selectedRoot: options.selectedRoot, expectedShards, jobAttempts, + artifactNamespace: process.env.EVAL_ARTIFACT_NAMESPACE ?? "" }); if (!matrixValid) { selection.status.push({ shard: "Prepare", attempt: null, complete: false, reason: "The expected shard matrix is unavailable or invalid; check the Prepare stage." }); diff --git a/eng/common/scripts/eval/invoke-eval-shard.ts b/eng/common/scripts/eval/invoke-eval-shard.ts index 135a71d5a9..064c1c7e46 100644 --- a/eng/common/scripts/eval/invoke-eval-shard.ts +++ b/eng/common/scripts/eval/invoke-eval-shard.ts @@ -6,9 +6,32 @@ import path from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; import { getVallyShardVerdict } from "./lib/verdict.ts"; -// The pinned Vally CLI is installed next to this script (package.json + node_modules live here), -// so the npm --prefix is just this file's own directory. +// Invoke the pinned CLI with Node directly so arguments never pass through a shell. const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); +const VALLY_CLI = path.join(SCRIPT_DIR, "node_modules", "@microsoft", "vally-cli", "dist", "index.js"); + +export function validateVallyArguments(value) { + if (!Array.isArray(value) || value.some((arg) => typeof arg !== "string" || arg.includes("\0"))) { + throw new Error("Vally arguments must be an array of strings without null characters."); + } + // Deliberately exclude eval selection, plugins, output, JUnit and verdict overrides. + const allowed = new Set(["--tag", "--runs", "--workers", "--timeout"]); + for (let index = 0; index < value.length; index += 2) { + const option = value[index]; + if (!allowed.has(option)) { + throw new Error(`Unsupported supplementary Vally option: ${option}`); + } + const argument = value[index + 1]; + if (!argument || argument.startsWith("-")) { + throw new Error(`Missing value for ${option}`); + } + if (["--runs", "--workers"].includes(option) && + (!/^[1-9]\d*$/.test(argument) || !Number.isSafeInteger(Number(argument)))) { + throw new Error(`${option} must be a positive integer.`); + } + } + return [...value]; +} /** * Executes the shard and returns the process exit code (0 pass, 1 fail). Side effects @@ -19,11 +42,14 @@ const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); * @param {string} options.shardName Shard name (log messages + result folder). * @param {string} options.outputDir `--output-dir` Vally writes results into. * @param {number} [options.threshold] Pass-rate gate forwarded to `vally eval --threshold`. + * @param {string[]} [options.vallyArguments] Supplementary, allowlisted Vally arguments. * @returns {number} 0 when the shard verdict passes, 1 otherwise. */ -export function runShard({ evalArgs, shardName, outputDir, threshold = 0.8 }) { +export function runShard({ evalArgs, shardName, outputDir, threshold = 0.8, vallyArguments = [] }, + { spawn = spawnSync } = {}) { const evalArgList = evalArgs.split(/\s+/).filter(Boolean); const thresholdArg = String(threshold); + const supplementaryArgs = validateVallyArguments(vallyArguments); console.log( `Running: vally eval ${evalArgs} --junit --threshold ${thresholdArg} --output-dir "${outputDir}"` @@ -33,25 +59,22 @@ export function runShard({ evalArgs, shardName, outputDir, threshold = 0.8 }) { // Uses spawnSync (not the vendored execFile helper) on purpose: the shard needs vally's output // streamed live to the log (stdio: "inherit") and must keep going on a non-zero exit, whereas // the exec helper captures output and rejects on failure. - const proc = spawnSync( - "npm", + const proc = spawn( + process.execPath, [ - "exec", - "--no", - "--prefix", - SCRIPT_DIR, - "--", - "vally", + VALLY_CLI, "eval", ...evalArgList, + ...supplementaryArgs, "--junit", "--threshold", thresholdArg, "--output-dir", outputDir, ], - { stdio: "inherit", shell: process.platform === "win32" } + { stdio: "inherit", shell: false } ); + if (proc.error) throw proc.error; const vallyExit = proc.status ?? 1; const verdict = getVallyShardVerdict({ resultsDir: outputDir, threshold }); @@ -91,8 +114,11 @@ export function runShard({ evalArgs, shardName, outputDir, threshold = 0.8 }) { // ----- CLI ----- -function parseArgs(argv) { - const options = { threshold: 0.8 }; +export function parseArgs(argv, env = process.env) { + const options = { + threshold: 0.8, + vallyArguments: validateVallyArguments(JSON.parse(env.VALLY_ARGUMENTS_JSON ?? "[]")), + }; for (let i = 0; i < argv.length; i++) { const next = () => argv[++i]; switch (argv[i]) { diff --git a/eng/common/scripts/eval/lib/shard-results.ts b/eng/common/scripts/eval/lib/shard-results.ts index 8160e51f22..a5450ab00d 100644 --- a/eng/common/scripts/eval/lib/shard-results.ts +++ b/eng/common/scripts/eval/lib/shard-results.ts @@ -31,13 +31,14 @@ export function expectedShardsFromMatrix(value) { } // The artifact list cannot reveal a newer retry that lost its agent before publishing. -export function latestShardAttempts(timeline, matrix) { +export function latestShardAttempts(timeline, matrix, evalStageName = "Eval") { expectedShardsFromMatrix(matrix); + if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(evalStageName)) throw new Error("Invalid eval stage identity."); if (!Array.isArray(timeline?.records)) throw new Error("The current build timeline is required."); const attempts = {}; for (const [key, entry] of Object.entries(matrix)) { if (!validShard(key)) throw new Error("Invalid matrix leg identity."); - const jobs = timeline.records.filter(record => record?.type === "Job" && record.identifier === `Eval.RunShard.${key}`); + const jobs = timeline.records.filter(record => record?.type === "Job" && record.identifier === `${evalStageName}.RunShard.${key}`); for (const record of jobs) { if (!Number.isSafeInteger(record.attempt) || record.attempt < 1) throw new Error("Invalid timeline attempt."); const current = { attempt: record.attempt, complete: record.state === "completed" && @@ -66,7 +67,7 @@ export async function readShardAttempts(env, { fetchImpl = fetch, wait = delay } response = await fetchImpl(url, { headers: { authorization: `Bearer ${env.SYSTEM_ACCESSTOKEN}` }, redirect: "error", signal: AbortSignal.timeout(30_000) }); if (!response.ok) throw new Error("Timeline could not be read."); - return latestShardAttempts(await response.json(), matrix); + return latestShardAttempts(await response.json(), matrix, env.EVAL_STAGE_NAME ?? "Eval"); } catch (error) { if ((response && ![408, 429, 500, 502, 503, 504].includes(response.status)) || attempt === 2) throw error; } finally { await response?.body?.cancel().catch(() => {}); } @@ -177,18 +178,23 @@ export function stageShardResults({ resultsRoot, outputDirectory, shardName, att return metadata; } -export function selectSummaryResults({ resultsRoot, selectedRoot, expectedShards, jobAttempts }) { +export function selectSummaryResults({ resultsRoot, selectedRoot, expectedShards, jobAttempts, artifactNamespace = "" }) { if (!Array.isArray(expectedShards) || expectedShards.some((name) => !validShard(name)) || new Set(expectedShards).size !== expectedShards.length) { throw new Error("Expected shards must be unique, valid shard names from Prepare."); } + if (typeof artifactNamespace !== "string" || (artifactNamespace !== "" && !validShard(artifactNamespace))) { + throw new Error("Invalid artifact namespace."); + } + const artifactPrefix = `eval-result-${artifactNamespace ? `${artifactNamespace}-` : ""}`; emptyDirectory(selectedRoot); const expected = new Set(expectedShards); const timeline = jobAttempts?.schemaVersion === 1 && jobAttempts.valid === true ? jobAttempts.attempts : null; const selected = new Map(); const seen = new Set(); for (const entry of fs.readdirSync(resultsRoot, { withFileTypes: true })) { - const match = /^eval-result-(.+)-(\d+)$/.exec(entry.name); + if (!entry.name.startsWith(artifactPrefix)) continue; + const match = /^(.+)-(\d+)$/.exec(entry.name.slice(artifactPrefix.length)); if (!match || !expected.has(match[1])) continue; if (!entry.isDirectory()) throw new Error("Shard artifact paths must be directories, not files or symbolic links."); const attempt = Number(match[2]); @@ -232,7 +238,7 @@ export function selectSummaryResults({ resultsRoot, selectedRoot, expectedShards // The Tests tab and Markdown use the SAME selected attempts as the bundle. // Old-layout JUnit remains diagnostic; old artifacts are never upload-ready. const files = junit.length ? junit : globFiles(directory, "**/*.junit.xml"); - const output = path.join(selectedRoot, selectedAttempt.directory, "junit"); + const output = path.join(selectedRoot, `eval-result-${shard}-${selectedAttempt.attempt}`, "junit"); fs.mkdirSync(output, { recursive: true }); for (const [index, file] of files.entries()) { try { diff --git a/eng/common/scripts/eval/test/shard-results.test.ts b/eng/common/scripts/eval/test/shard-results.test.ts index 68fb908b08..d6308ea225 100644 --- a/eng/common/scripts/eval/test/shard-results.test.ts +++ b/eng/common/scripts/eval/test/shard-results.test.ts @@ -70,7 +70,7 @@ function runSummary(root, shards, options = {}) { const child = runCli("build-eval-summary.ts", [ "--results-root", path.join(root, "download"), "--selected-root", path.join(root, "selected"), "--output-path", output, - ], { TF_BUILD: "true", EVAL_EXPECTED_MATRIX: matrix }); + ], { TF_BUILD: "true", EVAL_EXPECTED_MATRIX: matrix, ...options.env }); return { child, markdown: fs.readFileSync(output, "utf8"), result: readJson(path.join(root, "summary", "eval-summary.json")), @@ -88,6 +88,63 @@ test("expected shards come from the full Prepare matrix, not arrived artifacts o } }); +test("same-build tracks select their own artifacts and authoritative retry evidence", (t) => { + const root = setup(t); + const shard = "area_same"; + const matrix = { leg: { shardName: shard } }; + const timeline = { records: [ + { type: "Job", identifier: "EvalForced.RunShard.leg", attempt: 1, state: "completed", result: "succeeded" }, + { type: "Job", identifier: "EvalForced.RunShard.leg", attempt: 2, state: "completed", result: "failed" }, + { type: "Job", identifier: "EvalTrigger.RunShard.leg", attempt: 3, state: "completed", result: "succeeded" }, + ] }; + for (const [namespace, attempt] of [["forced", 1], ["forced", 2], ["trigger", 3], ["forced-extra", 4]]) { + const source = invocation(root, `${namespace}-${attempt}`); + stageShardResults({ resultsRoot: source, + outputDirectory: path.join(root, "download", `eval-result-${namespace}-${shard}-${attempt}`), + shardName: shard, attempt }); + } + const evidence = latestShardAttempts(timeline, matrix, "EvalForced"); + fs.writeFileSync(path.join(root, "job-attempts.json"), JSON.stringify(evidence)); + const result = runSummary(root, [shard], { env: { EVAL_ARTIFACT_NAMESPACE: "forced" } }); + assert.equal(result.child.status, 0, result.child.stderr); + assert.equal(result.result.complete, true); + assert.deepEqual(result.index.attempts, [{ + shard, attempt: 2, directory: `eval-result-forced-${shard}-2`, + }]); + assert.equal(result.result.totals.scenarios, 1); + assert.deepEqual(latestShardAttempts(timeline, matrix, "EvalTrigger").attempts[shard], { attempt: 3, complete: true }); + assert.throws(() => latestShardAttempts(timeline, matrix, "../Eval"), /Invalid eval stage/); + // A newer failed attempt without an artifact must not reuse the older complete run. + const selectedRoot = path.join(root, "selected-missing"); + const missing = selectSummaryResults({ + resultsRoot: path.join(root, "download"), selectedRoot, expectedShards: [shard], + artifactNamespace: "forced", + jobAttempts: { schemaVersion: 1, valid: true, attempts: { [shard]: { attempt: 4, complete: true } } }, + }); + assert.equal(missing.complete, false); + assert.deepEqual(missing.shardInput.attempts, []); +}); + +test("empty artifact namespace preserves legacy names and invalid namespaces are rejected", (t) => { + const root = setup(t); + const shard = "area_test"; + artifact(root, shard, 1); + const result = selectSummaryResults({ + resultsRoot: path.join(root, "download"), selectedRoot: path.join(root, "selected"), + expectedShards: [shard], jobAttempts: readJson(path.join(root, "job-attempts.json")), + artifactNamespace: "", + }); + assert.equal(result.complete, true); + assert.deepEqual(result.shardInput.attempts, [{ shard, attempt: 1, directory: `eval-result-${shard}-1` }]); + for (const [index, namespace] of ["../escape", "with spaces", "/", null, 123, "x".repeat(201)].entries()) { + assert.throws(() => selectSummaryResults({ + resultsRoot: path.join(root, "download"), selectedRoot: path.join(root, `invalid-${index}`), + expectedShards: [shard], jobAttempts: readJson(path.join(root, "job-attempts.json")), + artifactNamespace: namespace, + }), /Invalid artifact namespace/); + } +}); + test("completed failing evaluations stage exact raw bytes and all JUnit, but no unrelated files", (t) => { const root = setup(t); const source = invocation(root, "source", { failed: true }); diff --git a/eng/common/scripts/eval/test/verdict.test.ts b/eng/common/scripts/eval/test/verdict.test.ts index fe4978ea5b..f9e55bc6cc 100644 --- a/eng/common/scripts/eval/test/verdict.test.ts +++ b/eng/common/scripts/eval/test/verdict.test.ts @@ -8,6 +8,69 @@ import path from "node:path"; import { after, before, describe, it } from "node:test"; import { getVallyShardVerdict } from "../lib/verdict.ts"; +import { parseArgs, runShard, validateVallyArguments } from "../invoke-eval-shard.ts"; +import { spawnSync } from "node:child_process"; + +describe("supplementary Vally arguments", () => { + it("accepts only supported option/value pairs without splitting values", () => { + const args = ["--tag", "label=two words; $(not-a-command) & \"quoted\"", "--runs", "3", "--workers", "1", "--timeout", "5m"]; + assert.deepEqual(validateVallyArguments(args), args); + const options = parseArgs(["--eval-args", "-e example.eval.yaml", "--shard-name", "example", "--output-dir", "out"], + { VALLY_ARGUMENTS_JSON: JSON.stringify(args) }); + assert.deepEqual(options.vallyArguments, args); + }); + + it("rejects output, verdict, discovery and plugin overrides, including aliases and equals syntax", () => { + for (const option of ["--output-dir", "--output-dir=elsewhere", "--junit", "--no-junit", "--threshold", + "--require-pass", "--skip-grade", "--skip-validate", "-e", "--eval-spec", "--suite", "--work-dir", + "--reporter-plugin", "--executor-plugin", "--grader-plugin", "--runs=3", "--", "-r"]) { + assert.throws(() => validateVallyArguments([option, "value"]), /Unsupported/); + } + for (const args of [null, {}, " --runs 3", [3], ["--runs"], ["--runs", "0"], ["--runs", "-1"], + ["--workers", "1.5"], ["--tag", "--threshold"], ["--tag", "value\0"]]) { + assert.throws(() => validateVallyArguments(args)); + } + assert.throws(() => parseArgs([], { VALLY_ARGUMENTS_JSON: "not JSON" })); + assert.deepEqual(validateVallyArguments([]), []); + }); + + it("passes literal arguments through a real Node process, retaining framework-owned settings", (t) => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "vally-arguments-")); + t.after(() => fs.rmSync(root, { recursive: true, force: true })); + const capture = path.join(root, "capture.json"); + const fakeCli = path.join(root, "fake-cli.mjs"); + const outputDir = path.join(root, "results with spaces"); + fs.writeFileSync(fakeCli, ` + import fs from "node:fs"; + import path from "node:path"; + const args = process.argv.slice(2); + fs.writeFileSync(${JSON.stringify(capture)}, JSON.stringify(args)); + const output = args[args.indexOf("--output-dir") + 1]; + fs.mkdirSync(output, { recursive: true }); + fs.writeFileSync(path.join(output, "results.jsonl"), JSON.stringify({ + type: "run-summary", evals: [{ name: "test", passed: true, stimuliRun: 1 }] + })); + `); + const args = ["--tag", "label=a b; $(echo bad) & \"quotes\" %PATH%", "--runs", "3"]; + const exit = runShard({ evalArgs: "-e test.eval.yaml", shardName: "test", outputDir, vallyArguments: args }, { + spawn(command, commandArgs, options) { + assert.equal(command, process.execPath); + assert.equal(options.shell, false); + assert.match(commandArgs[0], /vally-cli[\\/]dist[\\/]index\.js$/); + return spawnSync(command, [fakeCli, ...commandArgs.slice(1)], options); + }, + }); + assert.equal(exit, 0); + const actual = JSON.parse(fs.readFileSync(capture, "utf8")); + assert.deepEqual(actual, ["eval", "-e", "test.eval.yaml", ...args, + "--junit", "--threshold", "0.8", "--output-dir", outputDir]); + }); + + it("surfaces process launch failures instead of reading stale results", () => { + assert.throws(() => runShard({ evalArgs: "-e x", shardName: "x", outputDir: "unused" }, + { spawn: () => ({ error: new Error("failed to launch") }) }), /failed to launch/); + }); +}); describe("getVallyShardVerdict", () => { let root; From 9af5303742096079f16df829dab92ef9fca1c23e Mon Sep 17 00:00:00 2001 From: JoyerJin <116236375+JoyerJin@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:33:26 +0800 Subject: [PATCH 2/9] fix run number format error --- eng/common/scripts/eval/README.md | 8 ++++--- eng/common/scripts/eval/invoke-eval-shard.ts | 24 +++++++++++++++----- eng/common/scripts/eval/test/verdict.test.ts | 24 +++++++++++++++++--- 3 files changed, 44 insertions(+), 12 deletions(-) diff --git a/eng/common/scripts/eval/README.md b/eng/common/scripts/eval/README.md index 6e8abb3151..4552b616d1 100644 --- a/eng/common/scripts/eval/README.md +++ b/eng/common/scripts/eval/README.md @@ -27,13 +27,15 @@ Repo-local adapters can customize it without copying that infrastructure: | `preEvalTemplate` / `postEvalTemplate` | Existing parameter-free setup/teardown step templates. | | `evalInvokeTemplate` | Existing invocation template accepting `vallyRoot`, `UseAzSdkAuthentication` and `threshold`. | | `azureSubscription` | Existing invoke service-connection setting, also forwarded by the stage/job when using the common invoke template. | -| `vallyArguments` | Optional string array of supplementary Vally option/value pairs. Available through the stage, shard job and common invoke template. Defaults to `[]`. | +| `vallyArguments` | Optional array of supplementary Vally option/value pairs. Strings are required except that trial/worker counts also accept numbers. Available through the stage, shard job and common invoke template. Defaults to `[]`. | Supported supplementary options are `--tag`, `--runs`, `--workers` and `--timeout`. For example, `['--tag', 'mode=forced', '--runs', '3']` selects a local benchmark configuration without replacing the common runner. Trial/worker counts must be -positive integer strings; use `${{ format('{0}', parameters.Runs) }}` for numeric -pipeline parameters. Both authentication branches forward the array as JSON in +positive integer strings or safe integer numbers. Azure template expansion can +serialize numeric parameters as JSON numbers even when using `format`; the runner +validates and converts only `--runs` / `--workers` numeric values to strings. +All other options and values must be strings. Both authentication branches forward the array as JSON in `VALLY_ARGUMENTS_JSON`; the runner passes literal arguments to the pinned CLI using Node directly, without a shell or whitespace splitting of supplementary values. Unsupported flags (including aliases and `--flag=value`) are rejected. Eval diff --git a/eng/common/scripts/eval/invoke-eval-shard.ts b/eng/common/scripts/eval/invoke-eval-shard.ts index 064c1c7e46..012bc34079 100644 --- a/eng/common/scripts/eval/invoke-eval-shard.ts +++ b/eng/common/scripts/eval/invoke-eval-shard.ts @@ -11,26 +11,38 @@ const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); const VALLY_CLI = path.join(SCRIPT_DIR, "node_modules", "@microsoft", "vally-cli", "dist", "index.js"); export function validateVallyArguments(value) { - if (!Array.isArray(value) || value.some((arg) => typeof arg !== "string" || arg.includes("\0"))) { - throw new Error("Vally arguments must be an array of strings without null characters."); + if (!Array.isArray(value) || value.some((arg) => + (typeof arg !== "string" && typeof arg !== "number") || + (typeof arg === "string" && arg.includes("\0")))) { + throw new Error("Vally arguments must be an array of strings or numeric count values without null characters."); } // Deliberately exclude eval selection, plugins, output, JUnit and verdict overrides. const allowed = new Set(["--tag", "--runs", "--workers", "--timeout"]); + const args = []; for (let index = 0; index < value.length; index += 2) { const option = value[index]; if (!allowed.has(option)) { throw new Error(`Unsupported supplementary Vally option: ${option}`); } - const argument = value[index + 1]; + let argument = value[index + 1]; + const isCount = option === "--runs" || option === "--workers"; + // Azure template expansion can serialize even format('{0}', number) as a JSON number. + if (typeof argument === "number") { + if (!isCount || !Number.isSafeInteger(argument) || argument < 1) { + throw new Error("Numeric values are supported only for positive integer --runs and --workers counts."); + } + argument = String(argument); + } if (!argument || argument.startsWith("-")) { throw new Error(`Missing value for ${option}`); } - if (["--runs", "--workers"].includes(option) && + if (isCount && (!/^[1-9]\d*$/.test(argument) || !Number.isSafeInteger(Number(argument)))) { throw new Error(`${option} must be a positive integer.`); } + args.push(option, argument); } - return [...value]; + return args; } /** @@ -42,7 +54,7 @@ export function validateVallyArguments(value) { * @param {string} options.shardName Shard name (log messages + result folder). * @param {string} options.outputDir `--output-dir` Vally writes results into. * @param {number} [options.threshold] Pass-rate gate forwarded to `vally eval --threshold`. - * @param {string[]} [options.vallyArguments] Supplementary, allowlisted Vally arguments. + * @param {(string | number)[]} [options.vallyArguments] Allowlisted arguments; numeric trial/worker counts are normalized. * @returns {number} 0 when the shard verdict passes, 1 otherwise. */ export function runShard({ evalArgs, shardName, outputDir, threshold = 0.8, vallyArguments = [] }, diff --git a/eng/common/scripts/eval/test/verdict.test.ts b/eng/common/scripts/eval/test/verdict.test.ts index f9e55bc6cc..a360d8433d 100644 --- a/eng/common/scripts/eval/test/verdict.test.ts +++ b/eng/common/scripts/eval/test/verdict.test.ts @@ -20,6 +20,20 @@ describe("supplementary Vally arguments", () => { assert.deepEqual(options.vallyArguments, args); }); + it("normalizes numeric trial and worker counts produced by Azure template expansion", () => { + for (const mode of ["forced", "trigger", "no-skill"]) { + for (const runs of [1, 3]) { + const args = ["--tag", `mode=${mode}`, "--runs", runs, "--workers", 2]; + const expected = ["--tag", `mode=${mode}`, "--runs", String(runs), "--workers", "2"]; + assert.deepEqual(validateVallyArguments(args), expected); + assert.equal(args[3], runs); + const options = parseArgs(["--eval-args", "-e example.eval.yaml", "--shard-name", "example", "--output-dir", "out"], + { VALLY_ARGUMENTS_JSON: JSON.stringify(args) }); + assert.deepEqual(options.vallyArguments, expected); + } + } + }); + it("rejects output, verdict, discovery and plugin overrides, including aliases and equals syntax", () => { for (const option of ["--output-dir", "--output-dir=elsewhere", "--junit", "--no-junit", "--threshold", "--require-pass", "--skip-grade", "--skip-validate", "-e", "--eval-spec", "--suite", "--work-dir", @@ -27,7 +41,11 @@ describe("supplementary Vally arguments", () => { assert.throws(() => validateVallyArguments([option, "value"]), /Unsupported/); } for (const args of [null, {}, " --runs 3", [3], ["--runs"], ["--runs", "0"], ["--runs", "-1"], - ["--workers", "1.5"], ["--tag", "--threshold"], ["--tag", "value\0"]]) { + ["--workers", "1.5"], ["--tag", "--threshold"], ["--tag", "value\0"], + ["--runs", null], ["--runs", true], ["--runs", {}], ["--runs", []], + ["--runs", 0], ["--runs", -1], ["--runs", 1.5], ["--runs", NaN], + ["--workers", Infinity], ["--workers", Number.MAX_SAFE_INTEGER + 1], + ["--tag", 1], ["--timeout", 5], [1, "value"]]) { assert.throws(() => validateVallyArguments(args)); } assert.throws(() => parseArgs([], { VALLY_ARGUMENTS_JSON: "not JSON" })); @@ -51,7 +69,7 @@ describe("supplementary Vally arguments", () => { type: "run-summary", evals: [{ name: "test", passed: true, stimuliRun: 1 }] })); `); - const args = ["--tag", "label=a b; $(echo bad) & \"quotes\" %PATH%", "--runs", "3"]; + const args = ["--tag", "label=a b; $(echo bad) & \"quotes\" %PATH%", "--runs", 3]; const exit = runShard({ evalArgs: "-e test.eval.yaml", shardName: "test", outputDir, vallyArguments: args }, { spawn(command, commandArgs, options) { assert.equal(command, process.execPath); @@ -62,7 +80,7 @@ describe("supplementary Vally arguments", () => { }); assert.equal(exit, 0); const actual = JSON.parse(fs.readFileSync(capture, "utf8")); - assert.deepEqual(actual, ["eval", "-e", "test.eval.yaml", ...args, + assert.deepEqual(actual, ["eval", "-e", "test.eval.yaml", ...validateVallyArguments(args), "--junit", "--threshold", "0.8", "--output-dir", outputDir]); }); From 35924aebae26cec1aebf344c362e3892d469375b Mon Sep 17 00:00:00 2001 From: JoyerJin <116236375+JoyerJin@users.noreply.github.com> Date: Thu, 29 Oct 2026 19:09:28 +0800 Subject: [PATCH 3/9] strength the parameters and MCP kind logic --- eng/common/pipelines/live-eval.yml | 1 + .../pipelines/templates/jobs/eval-shard.yml | 3 +- .../templates/stages/archetype-eval.yml | 3 +- .../pipelines/templates/steps/eval-invoke.yml | 11 +++- eng/common/scripts/eval/README.md | 5 +- eng/common/scripts/eval/test/verdict.test.ts | 52 +++++++++++++++++++ 6 files changed, 70 insertions(+), 5 deletions(-) diff --git a/eng/common/pipelines/live-eval.yml b/eng/common/pipelines/live-eval.yml index e18bafa65f..cc1ae1c0d6 100644 --- a/eng/common/pipelines/live-eval.yml +++ b/eng/common/pipelines/live-eval.yml @@ -37,6 +37,7 @@ extends: - 'workflows/live/*.eval.yaml' # Run each shard under AzureCLI@2 so the real MCP's DevOps calls are authenticated. UseAzSdkAuthentication: true + azureSubscription: opensource-api-connection failOnFailedTests: true # Live scenarios are end-to-end, so give each shard more headroom than the report-only tiers. shardTimeoutInMinutes: 45 diff --git a/eng/common/pipelines/templates/jobs/eval-shard.yml b/eng/common/pipelines/templates/jobs/eval-shard.yml index 032484f3df..e71a387ea2 100644 --- a/eng/common/pipelines/templates/jobs/eval-shard.yml +++ b/eng/common/pipelines/templates/jobs/eval-shard.yml @@ -22,8 +22,9 @@ parameters: type: object default: [] - name: azureSubscription + # Caller-owned service connection; required by the common authenticated invoke template. type: string - default: opensource-api-connection + default: '' - name: UseAzSdkAuthentication # Forwarded to the invoke template: true runs the shard under AzureCLI@2. type: boolean diff --git a/eng/common/pipelines/templates/stages/archetype-eval.yml b/eng/common/pipelines/templates/stages/archetype-eval.yml index d3d56bf81c..65c45ce855 100644 --- a/eng/common/pipelines/templates/stages/archetype-eval.yml +++ b/eng/common/pipelines/templates/stages/archetype-eval.yml @@ -44,8 +44,9 @@ parameters: type: boolean default: false - name: azureSubscription + # Caller-owned service connection; unused when authentication is disabled. type: string - default: opensource-api-connection + default: '' - name: vallyArguments type: object default: [] diff --git a/eng/common/pipelines/templates/steps/eval-invoke.yml b/eng/common/pipelines/templates/steps/eval-invoke.yml index 044c50c719..3f53a22025 100644 --- a/eng/common/pipelines/templates/steps/eval-invoke.yml +++ b/eng/common/pipelines/templates/steps/eval-invoke.yml @@ -14,15 +14,22 @@ parameters: type: number default: 0.8 - name: azureSubscription + # Caller-owned service connection; required only for authenticated runs. type: string - default: opensource-api-connection + default: '' - name: vallyArguments # Supplementary --tag/--runs/--workers/--timeout argument pairs, not shell text. type: object default: [] steps: - - ${{ if parameters.UseAzSdkAuthentication }}: + - ${{ if and(parameters.UseAzSdkAuthentication, eq(trim(parameters.azureSubscription), '')) }}: + - script: | + echo "##vso[task.logissue type=error]azureSubscription must be provided when UseAzSdkAuthentication is true." + exit 1 + displayName: 'Validate authenticated eval service connection' + + - ${{ elseif parameters.UseAzSdkAuthentication }}: - task: AzureCLI@2 displayName: 'vally eval $(shardName) (authenticated)' inputs: diff --git a/eng/common/scripts/eval/README.md b/eng/common/scripts/eval/README.md index 4552b616d1..3aad05117d 100644 --- a/eng/common/scripts/eval/README.md +++ b/eng/common/scripts/eval/README.md @@ -26,7 +26,7 @@ Repo-local adapters can customize it without copying that infrastructure: | `artifactNamespace` | Optional track namespace, e.g. `forced`. Common adds the separator: `eval-result-forced--` / `eval-debug-forced--`. Empty preserves existing names. Configure the matching summary with the same namespace. | | `preEvalTemplate` / `postEvalTemplate` | Existing parameter-free setup/teardown step templates. | | `evalInvokeTemplate` | Existing invocation template accepting `vallyRoot`, `UseAzSdkAuthentication` and `threshold`. | -| `azureSubscription` | Existing invoke service-connection setting, also forwarded by the stage/job when using the common invoke template. | +| `azureSubscription` | Caller-owned Azure service connection, forwarded by the stage/job to the common invoke template. Defaults to empty. Required when `UseAzSdkAuthentication: true`; unauthenticated runs need no connection. | | `vallyArguments` | Optional array of supplementary Vally option/value pairs. Strings are required except that trial/worker counts also accept numbers. Available through the stage, shard job and common invoke template. Defaults to `[]`. | Supported supplementary options are `--tag`, `--runs`, `--workers` and `--timeout`. @@ -45,6 +45,9 @@ array is empty. A custom template opting into a non-empty array must declare its own `vallyArguments` parameter; otherwise template compilation rejects it. Use the existing pre/post-eval and MCP setup hooks for repository-specific work. +Authenticated callers must explicitly configure `azureSubscription`; common has +no deployment-specific connection fallback. An empty or whitespace-only value +fails with a configuration error without creating an `AzureCLI@2` task. Do not replace authentication, result collection or retry/completeness checks. Supplementary reports must not rewrite JUnit thresholds or raw results. diff --git a/eng/common/scripts/eval/test/verdict.test.ts b/eng/common/scripts/eval/test/verdict.test.ts index a360d8433d..4f10b0ce90 100644 --- a/eng/common/scripts/eval/test/verdict.test.ts +++ b/eng/common/scripts/eval/test/verdict.test.ts @@ -10,6 +10,58 @@ import { after, before, describe, it } from "node:test"; import { getVallyShardVerdict } from "../lib/verdict.ts"; import { parseArgs, runShard, validateVallyArguments } from "../invoke-eval-shard.ts"; import { spawnSync } from "node:child_process"; +import { parse as parseYaml } from "yaml"; + +describe("eval template authentication contract", () => { + const load = (relative: string) => parseYaml(fs.readFileSync(new URL(relative, import.meta.url), "utf8")); + const invoke = load("../../../pipelines/templates/steps/eval-invoke.yml"); + + it("keeps the service connection optional and empty in every common template layer", () => { + for (const template of [invoke, + load("../../../pipelines/templates/jobs/eval-shard.yml"), + load("../../../pipelines/templates/stages/archetype-eval.yml")]) { + assert.equal(template.parameters.find((parameter: { name: string }) => parameter.name === "azureSubscription").default, ""); + assert.equal(template.parameters.find((parameter: { name: string }) => parameter.name === "UseAzSdkAuthentication").default, false); + } + }); + + it("rejects empty authenticated configuration without emitting an AzureCLI task", () => { + const steps = invoke.steps[0]["${{ if and(parameters.UseAzSdkAuthentication, eq(trim(parameters.azureSubscription), '')) }}"]; + assert.equal(steps.length, 1); + assert.equal(steps[0].task, undefined); + assert.match(steps[0].script, /##vso\[task\.logissue type=error\].*azureSubscription.*UseAzSdkAuthentication/); + assert.match(steps[0].script, /exit 1/); + }); + + it("uses only the caller's connection for authentication and none for unauthenticated runs", () => { + const authenticated = invoke.steps[1]["${{ elseif parameters.UseAzSdkAuthentication }}"]; + assert.equal(authenticated.length, 1); + assert.equal(authenticated[0].task, "AzureCLI@2"); + assert.equal(authenticated[0].inputs.azureSubscription, "${{ parameters.azureSubscription }}"); + const unauthenticated = invoke.steps[2]["${{ else }}"]; + assert.equal(unauthenticated.length, 1); + assert.equal(unauthenticated[0].task, undefined); + assert.match(unauthenticated[0].script, /invoke-eval-shard\.ts/); + assert.equal(unauthenticated[0].inputs?.azureSubscription, undefined); + for (const step of [authenticated[0], unauthenticated[0]]) { + assert.equal(step.env.VALLY_ARGUMENTS_JSON, "${{ convertToJson(parameters.vallyArguments) }}"); + assert.equal(step.env.COPILOT_GITHUB_TOKEN, "$(COPILOT_GITHUB_TOKEN)"); + } + }); + + it("keeps the existing live entrypoint's connection explicit and forwards caller configuration", () => { + const live = load("../../../pipelines/live-eval.yml").extends.parameters; + assert.equal(live.UseAzSdkAuthentication, true); + assert.equal(live.azureSubscription, "opensource-api-connection"); + const stage = load("../../../pipelines/templates/stages/archetype-eval.yml"); + const job = stage.extends.parameters.stages.find((entry: { stage: string }) => entry.stage === "Eval").jobs[0]; + assert.equal(job.parameters.azureSubscription, "${{ parameters.azureSubscription }}"); + const shard = load("../../../pipelines/templates/jobs/eval-shard.yml"); + const invokeStep = shard.jobs[0].steps.find((step: { template?: string }) => step.template === "${{ parameters.evalInvokeTemplate }}"); + const commonParameters = invokeStep.parameters["${{ if eq(parameters.evalInvokeTemplate, '/eng/common/pipelines/templates/steps/eval-invoke.yml') }}"]; + assert.equal(commonParameters.azureSubscription, "${{ parameters.azureSubscription }}"); + }); +}); describe("supplementary Vally arguments", () => { it("accepts only supported option/value pairs without splitting values", () => { From f2f4803d29d946a32a215b9f18a6411b0499ed2a Mon Sep 17 00:00:00 2001 From: JoyerJin <116236375+JoyerJin@users.noreply.github.com> Date: Fri, 30 Oct 2026 18:22:32 +0800 Subject: [PATCH 4/9] parameter logic addition --- .../pipelines/templates/jobs/eval-shard.yml | 28 ++-- .../templates/jobs/eval-summarize.yml | 24 +-- .../templates/stages/archetype-eval.yml | 8 + .../pipelines/templates/steps/eval-invoke.yml | 6 +- eng/common/scripts/eval/README.md | 46 +++++- eng/common/scripts/eval/build-eval-summary.ts | 6 +- eng/common/scripts/eval/invoke-eval-shard.ts | 28 +++- eng/common/scripts/eval/lib/shard-results.ts | 12 +- eng/common/scripts/eval/lib/verdict.ts | 13 ++ .../scripts/eval/test/shard-results.test.ts | 153 +++++++++++++++-- eng/common/scripts/eval/test/verdict.test.ts | 155 +++++++++++++----- 11 files changed, 387 insertions(+), 92 deletions(-) diff --git a/eng/common/pipelines/templates/jobs/eval-shard.yml b/eng/common/pipelines/templates/jobs/eval-shard.yml index e71a387ea2..942fca312b 100644 --- a/eng/common/pipelines/templates/jobs/eval-shard.yml +++ b/eng/common/pipelines/templates/jobs/eval-shard.yml @@ -10,8 +10,9 @@ parameters: # Stage containing the generate_eval_matrix job. type: string default: Prepare - - name: artifactNamespace - # Optional track namespace (e.g. 'forced'); common adds the separator. + - name: artifactPrefix + # Literal prefix after 'eval-result-' / 'eval-debug-'; include any separator (e.g. 'forced-'). + # Match Summary's prefix and keep final artifact names unique across this build's tracks. type: string default: '' - name: evalInvokeTemplate @@ -19,8 +20,13 @@ parameters: type: string default: /eng/common/pipelines/templates/steps/eval-invoke.yml - name: vallyArguments + # --tag does not filter the supplied matrix; each planned eval must retain matching stimuli. type: object default: [] + - name: failOnFailedTests + # Match Summary's gate; multi-run gating is not currently supported. + type: boolean + default: false - name: azureSubscription # Caller-owned service connection; required by the common authenticated invoke template. type: string @@ -68,18 +74,12 @@ jobs: # The Summary job keeps only the highest attempt per shard so retries supersede cleanly. - output: pipelineArtifact displayName: 'Publish $(shardName) raw results, JUnit and completeness' - ${{ if eq(parameters.artifactNamespace, '') }}: - artifactName: 'eval-result-$(shardName)-$(System.JobAttempt)' - ${{ else }}: - artifactName: 'eval-result-${{ parameters.artifactNamespace }}-$(shardName)-$(System.JobAttempt)' + artifactName: 'eval-result-${{ parameters.artifactPrefix }}$(shardName)-$(System.JobAttempt)' targetPath: $(Build.ArtifactStagingDirectory)/eval-result condition: always() - output: pipelineArtifact displayName: 'Publish $(shardName) transcripts (on failure)' - ${{ if eq(parameters.artifactNamespace, '') }}: - artifactName: 'eval-debug-$(shardName)-$(System.JobAttempt)' - ${{ else }}: - artifactName: 'eval-debug-${{ parameters.artifactNamespace }}-$(shardName)-$(System.JobAttempt)' + artifactName: 'eval-debug-${{ parameters.artifactPrefix }}$(shardName)-$(System.JobAttempt)' targetPath: $(Build.ArtifactStagingDirectory)/debug condition: failed() steps: @@ -92,6 +92,14 @@ jobs: inputs: version: 24.x + - ${{ if eq(parameters.evalInvokeTemplate, '/eng/common/pipelines/templates/steps/eval-invoke.yml') }}: + - script: | + node --experimental-strip-types "$(Build.SourcesDirectory)/eng/common/scripts/eval/invoke-eval-shard.ts" --validate-only + displayName: 'Validate eval run configuration' + env: + VALLY_ARGUMENTS_JSON: ${{ replace(convertToJson(parameters.vallyArguments), '$', '\u0024') }} + EVAL_FAIL_ON_FAILED_TESTS: ${{ parameters.failOnFailedTests }} + # 1ES Official image blocks registry.npmjs.org; restore via the azure-sdk npm mirror. - template: /eng/common/pipelines/templates/steps/create-authenticated-npmrc.yml parameters: diff --git a/eng/common/pipelines/templates/jobs/eval-summarize.yml b/eng/common/pipelines/templates/jobs/eval-summarize.yml index d97717d736..a9552c046a 100644 --- a/eng/common/pipelines/templates/jobs/eval-summarize.yml +++ b/eng/common/pipelines/templates/jobs/eval-summarize.yml @@ -8,10 +8,13 @@ parameters: - name: evalStageName type: string default: Eval - - name: artifactNamespace + - name: artifactPrefix + # Literal prefix, including any separator (e.g. 'forced-'); must match the shard job. + # Callers are responsible for unique final artifact names across tracks in one build. type: string default: '' - name: failOnFailedTests + # Only single-run evaluations support this gate; multi-run observations must leave it off. type: boolean default: false - name: createDashboardBundle @@ -36,17 +39,14 @@ jobs: outputs: - output: pipelineArtifact displayName: 'Retain selected build summary and saved bundle' - ${{ if eq(parameters.artifactNamespace, '') }}: - artifactName: 'eval-summary-$(System.JobAttempt)' - ${{ else }}: - artifactName: 'eval-summary-${{ parameters.artifactNamespace }}-$(System.JobAttempt)' + artifactName: 'eval-summary-${{ parameters.artifactPrefix }}$(System.JobAttempt)' targetPath: $(Build.ArtifactStagingDirectory)/eval-summary condition: always() steps: # Dashboard archives identify a single build-wide summary, not separate tracks. - - ${{ if and(ne(parameters.artifactNamespace, ''), or(parameters.createDashboardBundle, parameters.publishDashboardResults)) }}: + - ${{ if and(ne(parameters.artifactPrefix, ''), or(parameters.createDashboardBundle, parameters.publishDashboardResults)) }}: - script: | - echo "##vso[task.logissue type=error]Namespaced eval tracks support artifact-only summaries, not dashboard bundles or publication." + echo "##vso[task.logissue type=error]Prefixed eval tracks support artifact-only summaries, not dashboard bundles or publication." exit 1 displayName: 'Reject ambiguous multi-track dashboard identity' @@ -59,15 +59,15 @@ jobs: inputs: version: 24.x - # Allowlist result data only, excluding debug, MCP, SDL and signing artifacts. + # Download only this track's result data; an empty prefix retains all eval-result-* artifacts. - task: DownloadPipelineArtifact@2 displayName: 'Download all shard results' inputs: buildType: 'current' itemPattern: | - eval-result-*/results.jsonl - eval-result-*/shard.json - eval-result-*/**/*.junit.xml + eval-result-${{ parameters.artifactPrefix }}*/results.jsonl + eval-result-${{ parameters.artifactPrefix }}*/shard.json + eval-result-${{ parameters.artifactPrefix }}*/**/*.junit.xml targetPath: $(Pipeline.Workspace)/eval-results # Only attempt verification needs the current build token; rendering uses saved evidence. @@ -91,7 +91,7 @@ jobs: condition: always() env: EVAL_EXPECTED_MATRIX: $(EvalExpectedMatrix) - EVAL_ARTIFACT_NAMESPACE: ${{ parameters.artifactNamespace }} + EVAL_ARTIFACT_PREFIX: ${{ parameters.artifactPrefix }} - ${{ if or(parameters.createDashboardBundle, parameters.publishDashboardResults) }}: - template: ../steps/eval-publish-results.yml diff --git a/eng/common/pipelines/templates/stages/archetype-eval.yml b/eng/common/pipelines/templates/stages/archetype-eval.yml index 65c45ce855..fe88cac0ba 100644 --- a/eng/common/pipelines/templates/stages/archetype-eval.yml +++ b/eng/common/pipelines/templates/stages/archetype-eval.yml @@ -48,8 +48,13 @@ parameters: type: string default: '' - name: vallyArguments + # --tag does not filter Prepare's matrix; align evalGlobs so every eval has matching stimuli. type: object default: [] + - name: artifactPrefix + # Literal artifact prefix, including any separator (e.g. 'forced-'). + type: string + default: '' - name: threshold # Pass-rate gate forwarded to `vally eval --threshold` and the shard verdict. Repos may override. type: number @@ -107,10 +112,12 @@ extends: - template: /eng/common/pipelines/templates/jobs/eval-shard.yml parameters: vallyRoot: ${{ parameters.vallyRoot }} + artifactPrefix: ${{ parameters.artifactPrefix }} evalInvokeTemplate: ${{ parameters.evalInvokeTemplate }} UseAzSdkAuthentication: ${{ parameters.UseAzSdkAuthentication }} azureSubscription: ${{ parameters.azureSubscription }} vallyArguments: ${{ parameters.vallyArguments }} + failOnFailedTests: ${{ parameters.failOnFailedTests }} threshold: ${{ parameters.threshold }} preEvalTemplate: ${{ parameters.preEvalTemplate }} postEvalTemplate: ${{ parameters.postEvalTemplate }} @@ -123,6 +130,7 @@ extends: jobs: - template: /eng/common/pipelines/templates/jobs/eval-summarize.yml parameters: + artifactPrefix: ${{ parameters.artifactPrefix }} failOnFailedTests: ${{ parameters.failOnFailedTests }} createDashboardBundle: ${{ parameters.createDashboardBundle }} publishDashboardResults: ${{ parameters.publishDashboardResults }} diff --git a/eng/common/pipelines/templates/steps/eval-invoke.yml b/eng/common/pipelines/templates/steps/eval-invoke.yml index 3f53a22025..2302fb03c3 100644 --- a/eng/common/pipelines/templates/steps/eval-invoke.yml +++ b/eng/common/pipelines/templates/steps/eval-invoke.yml @@ -19,6 +19,8 @@ parameters: default: '' - name: vallyArguments # Supplementary --tag/--runs/--workers/--timeout argument pairs, not shell text. + # --tag does not filter the matrix; callers must keep every planned eval non-empty. + # JSON-escape dollar signs below so agent macros cannot rewrite argument data. type: object default: [] @@ -45,7 +47,7 @@ steps: --threshold ${{ parameters.threshold }} env: COPILOT_GITHUB_TOKEN: $(COPILOT_GITHUB_TOKEN) - VALLY_ARGUMENTS_JSON: ${{ convertToJson(parameters.vallyArguments) }} + VALLY_ARGUMENTS_JSON: ${{ replace(convertToJson(parameters.vallyArguments), '$', '\u0024') }} - ${{ else }}: - script: | @@ -58,4 +60,4 @@ steps: workingDirectory: ${{ parameters.vallyRoot }} env: COPILOT_GITHUB_TOKEN: $(COPILOT_GITHUB_TOKEN) - VALLY_ARGUMENTS_JSON: ${{ convertToJson(parameters.vallyArguments) }} + VALLY_ARGUMENTS_JSON: ${{ replace(convertToJson(parameters.vallyArguments), '$', '\u0024') }} diff --git a/eng/common/scripts/eval/README.md b/eng/common/scripts/eval/README.md index 3aad05117d..07cb19aac1 100644 --- a/eng/common/scripts/eval/README.md +++ b/eng/common/scripts/eval/README.md @@ -23,7 +23,7 @@ Repo-local adapters can customize it without copying that infrastructure: | Parameter | Contract | | --- | --- | | `matrixStageName` | Stage containing `generate_eval_matrix`; defaults to `Prepare`. The eval stage must depend on it. | -| `artifactNamespace` | Optional track namespace, e.g. `forced`. Common adds the separator: `eval-result-forced--` / `eval-debug-forced--`. Empty preserves existing names. Configure the matching summary with the same namespace. | +| `artifactPrefix` | Optional literal prefix, e.g. `forced-`. Common adds no separator: `eval-result-forced--` / `eval-debug-forced--` / `eval-summary-forced-`. Empty preserves existing names. Available through the stage and jobs; shard and summary prefixes must match. | | `preEvalTemplate` / `postEvalTemplate` | Existing parameter-free setup/teardown step templates. | | `evalInvokeTemplate` | Existing invocation template accepting `vallyRoot`, `UseAzSdkAuthentication` and `threshold`. | | `azureSubscription` | Caller-owned Azure service connection, forwarded by the stage/job to the common invoke template. Defaults to empty. Required when `UseAzSdkAuthentication: true`; unauthenticated runs need no connection. | @@ -38,12 +38,33 @@ validates and converts only `--runs` / `--workers` numeric values to strings. All other options and values must be strings. Both authentication branches forward the array as JSON in `VALLY_ARGUMENTS_JSON`; the runner passes literal arguments to the pinned CLI using Node directly, without a shell or whitespace splitting of supplementary values. +The template JSON-escapes dollar signs before agent macro expansion; the runner's +JSON decoding restores them. Tags such as `build=$(Build.BuildId)` stay literal, +even when the pipeline variable exists. Unsupported flags (including aliases and `--flag=value`) are rejected. Eval selection, output paths, JUnit, plugins and verdict settings remain framework-owned. Legacy custom invoke templates retain their existing parameter contract when the array is empty. A custom template opting into a non-empty array must declare its own `vallyArguments` parameter; otherwise template compilation rejects it. +`--runs` greater than 1 must not be combined with `failOnFailedTests: true`. +Use `failOnFailedTests: false` (the default) for multi-run observations; +explicit `--runs 1` may still be used with the gate enabled. Unsupported +combinations fail the common shard's configuration check before installation or +evaluation. When composing shard and summary jobs directly, pass the same +`failOnFailedTests` value to both. This check covers explicit `--runs` values in +`vallyArguments` only; Summary does not infer run counts from results. Callers must +ensure that repetition configured in `.vally.yaml` or custom invoke templates is +not combined with the enabled gate. + +`--tag` filters stimuli during execution only; it does not filter Prepare's matrix. +The caller must align `evalGlobs` (or its custom matrix) with the tags so every +planned eval retains at least one matching stimulus. For example, `--tag area=github` +alone does not remove other areas from the matrix: also restrict the selected files +to the intended GitHub evals. Recheck this alignment when adding evals or changing tags. +Zero-match evals are not treated as successful skips; a zero-trial shard still fails +the verdict/completeness checks even with `failOnFailedTests: false`. + Use the existing pre/post-eval and MCP setup hooks for repository-specific work. Authenticated callers must explicitly configure `azureSubscription`; common has no deployment-specific connection fallback. An empty or whitespace-only value @@ -51,13 +72,25 @@ fails with a configuration error without creating an `AzureCLI@2` task. Do not replace authentication, result collection or retry/completeness checks. Supplementary reports must not rewrite JUnit thresholds or raw results. -For multiple tracks in one build, use a distinct artifact namespace and configure -the common summary's `matrixStageName`, `evalStageName` and `artifactNamespace` -to match the shard job. Each Summary stage must depend on both its Prepare and +For multiple tracks in one build, use an artifact prefix and configure +the common summary's `matrixStageName`, `evalStageName` and `artifactPrefix` +to match the shard job. Each Summary downloads only result-file patterns matching +its prefix; an empty prefix retains the build-wide `eval-result-*` download. +Each Summary stage must depend on both its Prepare and Eval stages. Timeline attempts are checked against that exact Eval stage, and -only that namespace's latest verified attempts are selected. Namespaced tracks +only that track's latest verified attempts are selected. Prefixed tracks support artifact-only summaries: dashboard bundles/publication are rejected because the archive identity currently represents one build-wide Summary. +Prefixes and shard names must start with an ASCII letter, digit or underscore, +contain only ASCII letters, digits, underscores or hyphens, and be at most 200 +characters; prefixes may also be empty. Include any desired separator +in the prefix: `forced-` and `forced` are used exactly as supplied. Existing shard +names, including names with hyphens, remain supported. + +The caller must ensure final artifact names are unique across all tracks in the +build, including unprefixed tracks. Different prefixes alone do not guarantee this: +`forced-` + `area-foo` and `forced-area-` + `foo` produce the same name. +Common does not perform cross-track collision detection. ## Complete build results and direct Blob publishing @@ -201,6 +234,9 @@ then `node --experimental-strip-types --test` from this directory. These shared Node tests do not need Tools-owned CI files, PowerShell tests or the consuming repository's redirect settings. A consumer-copy regression runs them without Tools files or Git history, with absent and different redirect templates. +Template contract checks use bounded text assertions, not a YAML parser, and need +no evaluator dependencies. They do not evaluate Azure template expressions; +server-side expansion and service-connection authorization require separate validation. In Tools, run `Invoke-Pester -Path eng/common-tests/eval/Publish-EvalResults.Tests.ps1 -CI` from the repository root for the **PowerShell 7.4+ / Pester 5 / Azure CLI** tests. That Tools-only diff --git a/eng/common/scripts/eval/build-eval-summary.ts b/eng/common/scripts/eval/build-eval-summary.ts index af6d8cc36a..54b268ea60 100644 --- a/eng/common/scripts/eval/build-eval-summary.ts +++ b/eng/common/scripts/eval/build-eval-summary.ts @@ -380,7 +380,7 @@ async function main(argv) { } selection = selectSummaryResults({ resultsRoot: options.resultsRoot, selectedRoot: options.selectedRoot, expectedShards, jobAttempts, - artifactNamespace: process.env.EVAL_ARTIFACT_NAMESPACE ?? "" }); + artifactPrefix: process.env.EVAL_ARTIFACT_PREFIX ?? "" }); if (!matrixValid) { selection.status.push({ shard: "Prepare", attempt: null, complete: false, reason: "The expected shard matrix is unavailable or invalid; check the Prepare stage." }); @@ -414,7 +414,9 @@ async function main(argv) { if (process.env.TF_BUILD) { console.log(`##vso[task.uploadsummary]${path.resolve(options.outputPath)}`); - if (selection) console.log(`##vso[task.setvariable variable=EvalSummaryComplete]${selection.complete}`); + } + if (process.env.TF_BUILD && selection) { + console.log(`##vso[task.setvariable variable=EvalSummaryComplete]${selection.complete}`); } // Completed evaluation failures remain governed by failOnFailedTests. Missing result diff --git a/eng/common/scripts/eval/invoke-eval-shard.ts b/eng/common/scripts/eval/invoke-eval-shard.ts index 012bc34079..bf71969c35 100644 --- a/eng/common/scripts/eval/invoke-eval-shard.ts +++ b/eng/common/scripts/eval/invoke-eval-shard.ts @@ -4,13 +4,13 @@ import { spawnSync } from "node:child_process"; import path from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; -import { getVallyShardVerdict } from "./lib/verdict.ts"; +import { assertSupportedEvalGate, getVallyShardVerdict, parseFailedTestsGate } from "./lib/verdict.ts"; // Invoke the pinned CLI with Node directly so arguments never pass through a shell. const SCRIPT_DIR = path.dirname(fileURLToPath(import.meta.url)); const VALLY_CLI = path.join(SCRIPT_DIR, "node_modules", "@microsoft", "vally-cli", "dist", "index.js"); -export function validateVallyArguments(value) { +export function validateVallyArguments(value, { failOnFailedTests = false } = {}) { if (!Array.isArray(value) || value.some((arg) => (typeof arg !== "string" && typeof arg !== "number") || (typeof arg === "string" && arg.includes("\0")))) { @@ -40,6 +40,9 @@ export function validateVallyArguments(value) { (!/^[1-9]\d*$/.test(argument) || !Number.isSafeInteger(Number(argument)))) { throw new Error(`${option} must be a positive integer.`); } + if (option === "--runs") { + assertSupportedEvalGate({ runs: Number(argument), failOnFailedTests }); + } args.push(option, argument); } return args; @@ -129,11 +132,15 @@ export function runShard({ evalArgs, shardName, outputDir, threshold = 0.8, vall export function parseArgs(argv, env = process.env) { const options = { threshold: 0.8, - vallyArguments: validateVallyArguments(JSON.parse(env.VALLY_ARGUMENTS_JSON ?? "[]")), + vallyArguments: validateVallyArguments(JSON.parse(env.VALLY_ARGUMENTS_JSON ?? "[]"), + { failOnFailedTests: parseFailedTestsGate(env.EVAL_FAIL_ON_FAILED_TESTS) }), }; for (let i = 0; i < argv.length; i++) { const next = () => argv[++i]; switch (argv[i]) { + case "--validate-only": + options.validateOnly = true; + break; case "--eval-args": options.evalArgs = next(); break; @@ -150,9 +157,11 @@ export function parseArgs(argv, env = process.env) { throw new Error(`Unknown argument: ${argv[i]}`); } } - for (const required of ["evalArgs", "shardName", "outputDir"]) { - if (!options[required]) { - throw new Error(`Missing required argument for ${required}.`); + if (!options.validateOnly) { + for (const required of ["evalArgs", "shardName", "outputDir"]) { + if (!options[required]) { + throw new Error(`Missing required argument for ${required}.`); + } } } return options; @@ -160,7 +169,12 @@ export function parseArgs(argv, env = process.env) { if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { try { - process.exit(runShard(parseArgs(process.argv.slice(2)))); + const options = parseArgs(process.argv.slice(2)); + if (options.validateOnly) { + console.log("Eval run configuration is supported."); + } else { + process.exit(runShard(options)); + } } catch (error) { console.error(error.message); process.exit(1); diff --git a/eng/common/scripts/eval/lib/shard-results.ts b/eng/common/scripts/eval/lib/shard-results.ts index a5450ab00d..f1251d7805 100644 --- a/eng/common/scripts/eval/lib/shard-results.ts +++ b/eng/common/scripts/eval/lib/shard-results.ts @@ -178,23 +178,23 @@ export function stageShardResults({ resultsRoot, outputDirectory, shardName, att return metadata; } -export function selectSummaryResults({ resultsRoot, selectedRoot, expectedShards, jobAttempts, artifactNamespace = "" }) { +export function selectSummaryResults({ resultsRoot, selectedRoot, expectedShards, jobAttempts, artifactPrefix = "" }) { if (!Array.isArray(expectedShards) || expectedShards.some((name) => !validShard(name)) || new Set(expectedShards).size !== expectedShards.length) { throw new Error("Expected shards must be unique, valid shard names from Prepare."); } - if (typeof artifactNamespace !== "string" || (artifactNamespace !== "" && !validShard(artifactNamespace))) { - throw new Error("Invalid artifact namespace."); + if (typeof artifactPrefix !== "string" || (artifactPrefix !== "" && !validShard(artifactPrefix))) { + throw new Error("Invalid artifact prefix."); } - const artifactPrefix = `eval-result-${artifactNamespace ? `${artifactNamespace}-` : ""}`; + const resultPrefix = `eval-result-${artifactPrefix}`; emptyDirectory(selectedRoot); const expected = new Set(expectedShards); const timeline = jobAttempts?.schemaVersion === 1 && jobAttempts.valid === true ? jobAttempts.attempts : null; const selected = new Map(); const seen = new Set(); for (const entry of fs.readdirSync(resultsRoot, { withFileTypes: true })) { - if (!entry.name.startsWith(artifactPrefix)) continue; - const match = /^(.+)-(\d+)$/.exec(entry.name.slice(artifactPrefix.length)); + if (!entry.name.startsWith(resultPrefix)) continue; + const match = /^(.+)-(\d+)$/.exec(entry.name.slice(resultPrefix.length)); if (!match || !expected.has(match[1])) continue; if (!entry.isDirectory()) throw new Error("Shard artifact paths must be directories, not files or symbolic links."); const attempt = Number(match[2]); diff --git a/eng/common/scripts/eval/lib/verdict.ts b/eng/common/scripts/eval/lib/verdict.ts index 330c59be75..e9e1617c19 100644 --- a/eng/common/scripts/eval/lib/verdict.ts +++ b/eng/common/scripts/eval/lib/verdict.ts @@ -7,6 +7,19 @@ import fs from "node:fs"; import path from "node:path"; import { globFiles } from "./glob.ts"; +export function parseFailedTestsGate(value: unknown = "false") { + if (typeof value !== "string" || !/^(true|false)$/i.test(value)) { + throw new Error("EVAL_FAIL_ON_FAILED_TESTS must be true or false."); + } + return value.toLowerCase() === "true"; +} + +export function assertSupportedEvalGate({ runs, failOnFailedTests }) { + if (failOnFailedTests && runs > 1) { + throw new Error("This eval configuration is not currently supported: multiple runs cannot be combined with failOnFailedTests=true. Disable the gate or set --runs to 1."); + } +} + // Formats a 0..1 ratio as a fixed-1-decimal percentage string (e.g. 97.1). function pct(ratio) { return (ratio * 100).toFixed(1); diff --git a/eng/common/scripts/eval/test/shard-results.test.ts b/eng/common/scripts/eval/test/shard-results.test.ts index d6308ea225..54deedb0bb 100644 --- a/eng/common/scripts/eval/test/shard-results.test.ts +++ b/eng/common/scripts/eval/test/shard-results.test.ts @@ -70,7 +70,8 @@ function runSummary(root, shards, options = {}) { const child = runCli("build-eval-summary.ts", [ "--results-root", path.join(root, "download"), "--selected-root", path.join(root, "selected"), "--output-path", output, - ], { TF_BUILD: "true", EVAL_EXPECTED_MATRIX: matrix, ...options.env }); + ], { TF_BUILD: "true", EVAL_EXPECTED_MATRIX: matrix, EVAL_ARTIFACT_PREFIX: "", + EVAL_FAIL_ON_FAILED_TESTS: "false", ...options.env }); return { child, markdown: fs.readFileSync(output, "utf8"), result: readJson(path.join(root, "summary", "eval-summary.json")), @@ -88,6 +89,69 @@ test("expected shards come from the full Prepare matrix, not arrived artifacts o } }); +test("Summary renders threshold verdicts, including 4/5 passes, without inferring configuration support", (t) => { + for (const [runs, passed] of [[1, 1], [5, 4], [5, 5]]) { + for (const gate of ["true", "false"]) { + const root = setup(t); + const shard = "area_runs"; + const output = artifact(root, shard, 1); + const records = Array.from({ length: runs }, (_, index) => ({ + type: "trial-result", status: index < passed ? "success" : "error", itemId: "example", + })); + fs.writeFileSync(path.join(output, "results.jsonl"), + [...records, { type: "run-summary", evals: [{ name: "example", passed: true }] }] + .map(record => JSON.stringify(record)).join("\n") + "\n"); + const metadata = readJson(path.join(output, "shard.json")); + metadata.trials = runs; + fs.writeFileSync(path.join(output, "shard.json"), JSON.stringify(metadata)); + const xml = '' + + Array.from({ length: runs }, (_, index) => `${ + index < passed ? "" : ''}`).join("") + ""; + const junit = path.join(output, "junit", "0.junit.xml"); + fs.writeFileSync(junit, xml); + const result = runSummary(root, [shard], { env: { EVAL_FAIL_ON_FAILED_TESTS: gate } }); + assert.equal(result.child.status, 0, result.child.stderr); + assert.equal(result.result.complete, true); + assert.equal(result.result.totals.failed, 0); + assert.equal(fs.readFileSync(junit, "utf8"), xml); + assert.equal(fs.readFileSync(path.join(root, "selected", `eval-result-${shard}-1`, "junit", "0.junit.xml"), "utf8"), xml); + assert.match(result.child.stdout, /variable=EvalSummaryComplete]true/); + } + } +}); + +test("Summary does not reject distinct single-run evals with the same stimulus name", (t) => { + const root = setup(t); + const shard = "area_same_names"; + const output = artifact(root, shard, 1); + const evals = ["first", "second"]; + fs.writeFileSync(path.join(output, "results.jsonl"), + [...evals.map(name => ({ type: "trial-result", status: "success", itemId: name })), + { type: "run-summary", evals: evals.map(name => ({ name, passed: true })) }] + .map(record => JSON.stringify(record)).join("\n") + "\n"); + const metadata = readJson(path.join(output, "shard.json")); + metadata.trials = evals.length; + fs.writeFileSync(path.join(output, "shard.json"), JSON.stringify(metadata)); + fs.writeFileSync(path.join(output, "junit", "0.junit.xml"), + `${evals.map(name => ` + + + `).join("")}`); + const result = runSummary(root, [shard], { env: { EVAL_FAIL_ON_FAILED_TESTS: "true" } }); + assert.equal(result.child.status, 0, result.child.stderr); + assert.equal(result.result.complete, true); + assert.equal(result.result.totals.failed, 0); + assert.match(result.child.stdout, /variable=EvalSummaryComplete]true/); +}); + +test("single-run gating does not bypass missing-result completeness failures", (t) => { + const root = setup(t); + artifact(root, "area_present", 1); + const result = runSummary(root, ["area_present", "area_missing"], { env: { EVAL_FAIL_ON_FAILED_TESTS: "true" } }); + assert.equal(result.child.status, 1); + assert.equal(result.result.complete, false); +}); + test("same-build tracks select their own artifacts and authoritative retry evidence", (t) => { const root = setup(t); const shard = "area_same"; @@ -97,15 +161,15 @@ test("same-build tracks select their own artifacts and authoritative retry evide { type: "Job", identifier: "EvalForced.RunShard.leg", attempt: 2, state: "completed", result: "failed" }, { type: "Job", identifier: "EvalTrigger.RunShard.leg", attempt: 3, state: "completed", result: "succeeded" }, ] }; - for (const [namespace, attempt] of [["forced", 1], ["forced", 2], ["trigger", 3], ["forced-extra", 4]]) { - const source = invocation(root, `${namespace}-${attempt}`); + for (const [prefix, attempt] of [["forced-", 1], ["forced-", 2], ["trigger-", 3], ["forced-extra-", 4]]) { + const source = invocation(root, `${prefix}${attempt}`); stageShardResults({ resultsRoot: source, - outputDirectory: path.join(root, "download", `eval-result-${namespace}-${shard}-${attempt}`), + outputDirectory: path.join(root, "download", `eval-result-${prefix}${shard}-${attempt}`), shardName: shard, attempt }); } const evidence = latestShardAttempts(timeline, matrix, "EvalForced"); fs.writeFileSync(path.join(root, "job-attempts.json"), JSON.stringify(evidence)); - const result = runSummary(root, [shard], { env: { EVAL_ARTIFACT_NAMESPACE: "forced" } }); + const result = runSummary(root, [shard], { env: { EVAL_ARTIFACT_PREFIX: "forced-" } }); assert.equal(result.child.status, 0, result.child.stderr); assert.equal(result.result.complete, true); assert.deepEqual(result.index.attempts, [{ @@ -118,30 +182,97 @@ test("same-build tracks select their own artifacts and authoritative retry evide const selectedRoot = path.join(root, "selected-missing"); const missing = selectSummaryResults({ resultsRoot: path.join(root, "download"), selectedRoot, expectedShards: [shard], - artifactNamespace: "forced", + artifactPrefix: "forced-", jobAttempts: { schemaVersion: 1, valid: true, attempts: { [shard]: { attempt: 4, complete: true } } }, }); assert.equal(missing.complete, false); assert.deepEqual(missing.shardInput.attempts, []); }); -test("empty artifact namespace preserves legacy names and invalid namespaces are rejected", (t) => { +test("literal prefixes publish names selected by Summary without adding separators", (t) => { + const load = name => fs.readFileSync(new URL(`../../../pipelines/templates/jobs/${name}.yml`, import.meta.url), "utf8"); + const shardTemplate = load("eval-shard"); + const resultName = shardTemplate.match(/^ artifactName: '(eval-result-[^'\r\n]+)'\r?$/m); + const debugName = shardTemplate.match(/^ artifactName: '(eval-debug-[^'\r\n]+)'\r?$/m); + const summaryName = load("eval-summarize").match(/^ artifactName: '(eval-summary-[^'\r\n]+)'\r?$/m); + assert.ok(resultName); + assert.ok(debugName); + assert.ok(summaryName); + for (const prefix of ["", "forced-", "trigger-", "forced", "forced-extra-"]) { + const root = setup(t); + const shard = "extra-area_same"; + const render = expression => expression.replace("${{ parameters.artifactPrefix }}", prefix) + .replace("$(shardName)", shard).replace("$(System.JobAttempt)", "1"); + const name = render(resultName[1]); + assert.equal(name, `eval-result-${prefix}${shard}-1`); + assert.equal(render(debugName[1]), `eval-debug-${prefix}${shard}-1`); + assert.equal(render(summaryName[1]), `eval-summary-${prefix}1`); + const source = invocation(root, "source"); + stageShardResults({ resultsRoot: source, outputDirectory: path.join(root, "download", name), + shardName: shard, attempt: 1 }); + fs.writeFileSync(path.join(root, "job-attempts.json"), JSON.stringify({ + schemaVersion: 1, valid: true, attempts: { [shard]: { attempt: 1, complete: true } }, + })); + const result = runSummary(root, [shard], { env: { EVAL_ARTIFACT_PREFIX: prefix } }); + assert.equal(result.child.status, 0, result.child.stderr); + assert.equal(result.result.complete, true); + assert.deepEqual(result.index.attempts, [{ shard, attempt: 1, directory: name }]); + } +}); + +test("template source defaults to legacy names and forwards a single literal artifact prefix", () => { + const load = name => fs.readFileSync(new URL(`../../../pipelines/templates/${name}.yml`, import.meta.url), "utf8"); + for (const name of ["jobs/eval-shard", "jobs/eval-summarize", "stages/archetype-eval"]) { + const template = load(name); + assert.match(template, /^ - name: artifactPrefix\r?\n(?: #[^\r\n]*\r?\n)* type: string\r?\n default: ''\r?$/m); + assert.doesNotMatch(template, /^ - name: artifactNamespace\r?$/m); + } + const summary = load("jobs/eval-summarize"); + const render = summary.split(/^ - script: \|\r?$/m).find(section => section.includes("build-eval-summary.ts")); + assert.ok(render); + assert.match(render, /^ EVAL_ARTIFACT_PREFIX: \$\{\{ parameters\.artifactPrefix \}\}\r?$/m); + assert.match(summary, /^ - \$\{\{ if and\(ne\(parameters\.artifactPrefix, ''\), or\(parameters\.createDashboardBundle, parameters\.publishDashboardResults\)\) \}\}:\r?$/m); + const stage = load("stages/archetype-eval"); + for (const name of ["Eval", "Summary"]) { + const section = stage.split(/^ - stage: /m).find(value => value.split(/\r?\n/, 1)[0] === name); + assert.ok(section); + assert.match(section, /^ artifactPrefix: \$\{\{ parameters\.artifactPrefix \}\}\r?$/m); + } +}); + +test("Summary download source applies the literal prefix to all result-file patterns", () => { + const summary = fs.readFileSync(new URL("../../../pipelines/templates/jobs/eval-summarize.yml", import.meta.url), "utf8"); + const download = summary.split(/^ - task: /m).find(section => + section.split(/\r?\n/, 1)[0] === "DownloadPipelineArtifact@2"); + assert.ok(download); + const block = download.match(/^ itemPattern: \|\r?\n((?: {12}[^\r\n]+\r?\n)+)/m); + assert.ok(block); + const patterns = block[1].trim().split(/\r?\n/).map(line => line.trim()); + const files = ["results.jsonl", "shard.json", "**/*.junit.xml"]; + assert.deepEqual(patterns, files.map(file => `eval-result-\${{ parameters.artifactPrefix }}*/${file}`)); + for (const prefix of ["", "forced-", "trigger-", "forced"]) { + assert.deepEqual(patterns.map(pattern => pattern.replace("${{ parameters.artifactPrefix }}", prefix)), + files.map(file => `eval-result-${prefix}*/${file}`)); + } +}); + +test("empty artifact prefix preserves legacy names and invalid prefixes are rejected", (t) => { const root = setup(t); const shard = "area_test"; artifact(root, shard, 1); const result = selectSummaryResults({ resultsRoot: path.join(root, "download"), selectedRoot: path.join(root, "selected"), expectedShards: [shard], jobAttempts: readJson(path.join(root, "job-attempts.json")), - artifactNamespace: "", + artifactPrefix: "", }); assert.equal(result.complete, true); assert.deepEqual(result.shardInput.attempts, [{ shard, attempt: 1, directory: `eval-result-${shard}-1` }]); - for (const [index, namespace] of ["../escape", "with spaces", "/", null, 123, "x".repeat(201)].entries()) { + for (const [index, prefix] of ["../escape", "with spaces", "forced.extra", "/", null, 123, "x".repeat(201)].entries()) { assert.throws(() => selectSummaryResults({ resultsRoot: path.join(root, "download"), selectedRoot: path.join(root, `invalid-${index}`), expectedShards: [shard], jobAttempts: readJson(path.join(root, "job-attempts.json")), - artifactNamespace: namespace, - }), /Invalid artifact namespace/); + artifactPrefix: prefix, + }), /Invalid artifact prefix/); } }); diff --git a/eng/common/scripts/eval/test/verdict.test.ts b/eng/common/scripts/eval/test/verdict.test.ts index 4f10b0ce90..3a850dfa2a 100644 --- a/eng/common/scripts/eval/test/verdict.test.ts +++ b/eng/common/scripts/eval/test/verdict.test.ts @@ -5,65 +5,144 @@ import assert from "node:assert/strict"; import fs from "node:fs"; import os from "node:os"; import path from "node:path"; +import { fileURLToPath } from "node:url"; import { after, before, describe, it } from "node:test"; -import { getVallyShardVerdict } from "../lib/verdict.ts"; +import { getVallyShardVerdict, parseFailedTestsGate } from "../lib/verdict.ts"; import { parseArgs, runShard, validateVallyArguments } from "../invoke-eval-shard.ts"; import { spawnSync } from "node:child_process"; -import { parse as parseYaml } from "yaml"; -describe("eval template authentication contract", () => { - const load = (relative: string) => parseYaml(fs.readFileSync(new URL(relative, import.meta.url), "utf8")); +describe("eval template authentication source contract", () => { + const load = (relative: string) => fs.readFileSync(new URL(relative, import.meta.url), "utf8"); const invoke = load("../../../pipelines/templates/steps/eval-invoke.yml"); + const branches = invoke.split(/(?=^ - \$\{\{ )/m).slice(1); it("keeps the service connection optional and empty in every common template layer", () => { for (const template of [invoke, load("../../../pipelines/templates/jobs/eval-shard.yml"), load("../../../pipelines/templates/stages/archetype-eval.yml")]) { - assert.equal(template.parameters.find((parameter: { name: string }) => parameter.name === "azureSubscription").default, ""); - assert.equal(template.parameters.find((parameter: { name: string }) => parameter.name === "UseAzSdkAuthentication").default, false); + assert.match(template, /^ - name: azureSubscription\r?\n(?: #[^\r\n]*\r?\n)* type: string\r?\n default: ''\r?$/m); + assert.match(template, /^ - name: UseAzSdkAuthentication\r?\n(?: #[^\r\n]*\r?\n)* type: boolean\r?\n default: false\r?$/m); } }); - it("rejects empty authenticated configuration without emitting an AzureCLI task", () => { - const steps = invoke.steps[0]["${{ if and(parameters.UseAzSdkAuthentication, eq(trim(parameters.azureSubscription), '')) }}"]; - assert.equal(steps.length, 1); - assert.equal(steps[0].task, undefined); - assert.match(steps[0].script, /##vso\[task\.logissue type=error\].*azureSubscription.*UseAzSdkAuthentication/); - assert.match(steps[0].script, /exit 1/); - }); - - it("uses only the caller's connection for authentication and none for unauthenticated runs", () => { - const authenticated = invoke.steps[1]["${{ elseif parameters.UseAzSdkAuthentication }}"]; - assert.equal(authenticated.length, 1); - assert.equal(authenticated[0].task, "AzureCLI@2"); - assert.equal(authenticated[0].inputs.azureSubscription, "${{ parameters.azureSubscription }}"); - const unauthenticated = invoke.steps[2]["${{ else }}"]; - assert.equal(unauthenticated.length, 1); - assert.equal(unauthenticated[0].task, undefined); - assert.match(unauthenticated[0].script, /invoke-eval-shard\.ts/); - assert.equal(unauthenticated[0].inputs?.azureSubscription, undefined); - for (const step of [authenticated[0], unauthenticated[0]]) { - assert.equal(step.env.VALLY_ARGUMENTS_JSON, "${{ convertToJson(parameters.vallyArguments) }}"); - assert.equal(step.env.COPILOT_GITHUB_TOKEN, "$(COPILOT_GITHUB_TOKEN)"); + it("keeps the missing-connection error in the first branch without an AzureCLI task", () => { + assert.equal(branches.length, 3); + const configuration = branches[0]; + assert.equal(configuration.split(/\r?\n/, 1)[0], + " - ${{ if and(parameters.UseAzSdkAuthentication, eq(trim(parameters.azureSubscription), '')) }}:"); + assert.equal(configuration.match(/^ - /gm)?.length, 1); + assert.match(configuration, /^ - script: \|\r?$/m); + assert.doesNotMatch(configuration, /^\s*- task:/m); + assert.match(configuration, /##vso\[task\.logissue type=error\].*azureSubscription.*UseAzSdkAuthentication/); + assert.match(configuration, /^ exit 1\r?$/m); + }); + + it("references the caller's connection only in the authenticated branch", () => { + const authenticated = branches[1]; + assert.equal(authenticated.split(/\r?\n/, 1)[0], " - ${{ elseif parameters.UseAzSdkAuthentication }}:"); + assert.equal(authenticated.match(/^ - /gm)?.length, 1); + assert.match(authenticated, /^ - task: AzureCLI@2\r?$/m); + assert.match(authenticated, /^ azureSubscription: \$\{\{ parameters\.azureSubscription \}\}\r?$/m); + const unauthenticated = branches[2]; + assert.equal(unauthenticated.split(/\r?\n/, 1)[0], " - ${{ else }}:"); + assert.equal(unauthenticated.match(/^ - /gm)?.length, 1); + assert.match(unauthenticated, /^ - script: \|\r?$/m); + assert.match(unauthenticated, /invoke-eval-shard\.ts/); + assert.doesNotMatch(unauthenticated, /^\s*(?:- task|azureSubscription):/m); + for (const branch of [authenticated, unauthenticated]) { + assert.equal(branch.match(/^ VALLY_ARGUMENTS_JSON: (.+)\r?$/m)?.[1].trim(), + "${{ replace(convertToJson(parameters.vallyArguments), '$', '\\u0024') }}"); + assert.match(branch, /^ COPILOT_GITHUB_TOKEN: \$\(COPILOT_GITHUB_TOKEN\)\r?$/m); } }); it("keeps the existing live entrypoint's connection explicit and forwards caller configuration", () => { - const live = load("../../../pipelines/live-eval.yml").extends.parameters; - assert.equal(live.UseAzSdkAuthentication, true); - assert.equal(live.azureSubscription, "opensource-api-connection"); + const live = load("../../../pipelines/live-eval.yml"); + assert.match(live, /^ UseAzSdkAuthentication: true\r?$/m); + assert.match(live, /^ azureSubscription: opensource-api-connection\r?$/m); const stage = load("../../../pipelines/templates/stages/archetype-eval.yml"); - const job = stage.extends.parameters.stages.find((entry: { stage: string }) => entry.stage === "Eval").jobs[0]; - assert.equal(job.parameters.azureSubscription, "${{ parameters.azureSubscription }}"); + const evalStage = stage.split(/^ - stage: /m).find(section => section.split(/\r?\n/, 1)[0] === "Eval"); + assert.ok(evalStage); + assert.match(evalStage, /^ - template: \/eng\/common\/pipelines\/templates\/jobs\/eval-shard\.yml\r?$/m); + assert.match(evalStage, /^ azureSubscription: \$\{\{ parameters\.azureSubscription \}\}\r?$/m); const shard = load("../../../pipelines/templates/jobs/eval-shard.yml"); - const invokeStep = shard.jobs[0].steps.find((step: { template?: string }) => step.template === "${{ parameters.evalInvokeTemplate }}"); - const commonParameters = invokeStep.parameters["${{ if eq(parameters.evalInvokeTemplate, '/eng/common/pipelines/templates/steps/eval-invoke.yml') }}"]; - assert.equal(commonParameters.azureSubscription, "${{ parameters.azureSubscription }}"); + const invokeStep = shard.split(/^ - template: /m).find(section => + section.split(/\r?\n/, 1)[0] === "${{ parameters.evalInvokeTemplate }}"); + assert.ok(invokeStep); + assert.match(invokeStep, /^ \$\{\{ if eq\(parameters\.evalInvokeTemplate, '\/eng\/common\/pipelines\/templates\/steps\/eval-invoke\.yml'\) \}\}:\r?\n azureSubscription: \$\{\{ parameters\.azureSubscription \}\}\r?$/m); + }); + + it("preflights common shard arguments before setup and forwards the gate to both jobs", () => { + const shard = load("../../../pipelines/templates/jobs/eval-shard.yml"); + const preflight = shard.indexOf("displayName: 'Validate eval run configuration'"); + assert.ok(preflight >= 0 && preflight < shard.indexOf("displayName: 'Install Vally CLI")); + assert.match(shard, /^ - \$\{\{ if eq\(parameters\.evalInvokeTemplate, '\/eng\/common\/pipelines\/templates\/steps\/eval-invoke\.yml'\) \}\}:\r?\n - script: \|/m); + assert.match(shard, /invoke-eval-shard\.ts" --validate-only/); + assert.match(shard, /^ EVAL_FAIL_ON_FAILED_TESTS: \$\{\{ parameters\.failOnFailedTests \}\}\r?$/m); + assert.equal(shard.match(/^ VALLY_ARGUMENTS_JSON: (.+)\r?$/m)?.[1].trim(), + "${{ replace(convertToJson(parameters.vallyArguments), '$', '\\u0024') }}"); + const stage = load("../../../pipelines/templates/stages/archetype-eval.yml"); + for (const name of ["Eval", "Summary"]) { + const section = stage.split(/^ - stage: /m).find(value => value.split(/\r?\n/, 1)[0] === name); + assert.ok(section); + assert.match(section, /^ failOnFailedTests: \$\{\{ parameters\.failOnFailedTests \}\}\r?$/m); + } + const summary = load("../../../pipelines/templates/jobs/eval-summarize.yml"); + assert.doesNotMatch(summary, /EVAL_FAIL_ON_FAILED_TESTS/); + assert.match(summary, /^ failTaskOnFailedTests: \$\{\{ parameters\.failOnFailedTests \}\}\r?$/m); }); }); describe("supplementary Vally arguments", () => { + it("rejects only multi-run gating, preserving single-run gates and multi-run observations", () => { + for (const runs of [1, "1"]) { + assert.deepEqual(validateVallyArguments(["--runs", runs], { failOnFailedTests: true }), ["--runs", "1"]); + } + for (const runs of [2, "2", 5, "5"]) { + assert.throws(() => validateVallyArguments(["--runs", runs], { failOnFailedTests: true }), + /not currently supported.*failOnFailedTests=true/); + assert.deepEqual(validateVallyArguments(["--runs", runs], { failOnFailedTests: false }), ["--runs", String(runs)]); + } + assert.deepEqual(validateVallyArguments([], { failOnFailedTests: true }), []); + assert.deepEqual(validateVallyArguments(["--workers", 5], { failOnFailedTests: true }), ["--workers", "5"]); + for (const value of ["true", "True", "TRUE"]) assert.equal(parseFailedTestsGate(value), true); + for (const value of [undefined, "false", "False"]) assert.equal(parseFailedTestsGate(value), false); + for (const value of ["", "yes", "1", null, true]) assert.throws(() => parseFailedTestsGate(value), /must be true or false/); + }); + + it("validates the configuration through the CLI without starting an evaluation", () => { + for (const [runs, gate, expected] of [[5, "true", 1], ["5", "True", 1], [1, "true", 0], ["1", "True", 0], [5, "false", 0]] as const) { + const env = { ...process.env, EVAL_FAIL_ON_FAILED_TESTS: gate, VALLY_ARGUMENTS_JSON: JSON.stringify(["--runs", runs]) }; + delete env.NODE_TEST_CONTEXT; + const child = spawnSync(process.execPath, ["--experimental-strip-types", + fileURLToPath(new URL("../invoke-eval-shard.ts", import.meta.url)), "--validate-only"], { + encoding: "utf8", timeout: 30_000, env, + }); + assert.ifError(child.error); + assert.equal(child.status, expected, child.stderr); + assert.doesNotMatch(child.stdout, /Running: vally eval/); + if (expected) assert.match(child.stderr, /not currently supported.*failOnFailedTests=true/); + else assert.match(child.stdout, /configuration is supported/); + } + }); + + it("preserves defined pipeline macros through simulated agent expansion and JSON decoding", () => { + const variables: Record = { "Build.BuildId": "12345", DefinedValue: 'quoted "value"\nand newline' }; + const expand = (value: string) => value.replace(/\$\(([^)]+)\)/g, (macro, name) => variables[name] ?? macro); + const args = ["--tag", "build=$(Build.BuildId)", "--tag", "label=$(DefinedValue)", + "--tag", String.raw`literal=\u0024; $5; $(undefined-variable)`]; + assert.equal(expand(args[1]), "build=12345"); + const json = JSON.stringify(args); + assert.throws(() => JSON.parse(expand(json)), SyntaxError); + const escaped = json.replaceAll("$", "\\u0024"); + assert.doesNotMatch(escaped, /\$\(/); + assert.equal(expand(escaped), escaped); + const options = parseArgs(["--eval-args", "-e example.eval.yaml", "--shard-name", "example", "--output-dir", "out"], + { VALLY_ARGUMENTS_JSON: expand(escaped) }); + assert.deepEqual(options.vallyArguments, args); + }); + it("accepts only supported option/value pairs without splitting values", () => { const args = ["--tag", "label=two words; $(not-a-command) & \"quoted\"", "--runs", "3", "--workers", "1", "--timeout", "5m"]; assert.deepEqual(validateVallyArguments(args), args); @@ -121,8 +200,10 @@ describe("supplementary Vally arguments", () => { type: "run-summary", evals: [{ name: "test", passed: true, stimuliRun: 1 }] })); `); - const args = ["--tag", "label=a b; $(echo bad) & \"quotes\" %PATH%", "--runs", 3]; - const exit = runShard({ evalArgs: "-e test.eval.yaml", shardName: "test", outputDir, vallyArguments: args }, { + const args = ["--tag", "label=a b; $(echo bad) & \"quotes\" %PATH%", "--tag", "build=$(Build.BuildId)", "--runs", 3]; + const options = parseArgs(["--eval-args", "-e test.eval.yaml", "--shard-name", "test", "--output-dir", outputDir], + { VALLY_ARGUMENTS_JSON: JSON.stringify(args).replaceAll("$", "\\u0024") }); + const exit = runShard(options, { spawn(command, commandArgs, options) { assert.equal(command, process.execPath); assert.equal(options.shell, false); From 7b5ed84ab8b4f1264af226f7a1aa6b03548a9c28 Mon Sep 17 00:00:00 2001 From: JoyerJin <116236375+JoyerJin@users.noreply.github.com> Date: Tue, 3 Nov 2026 18:44:49 +0800 Subject: [PATCH 5/9] fix with review comment --- eng/common/pipelines/live-eval.yml | 3 +- .../pipelines/templates/jobs/eval-shard.yml | 6 --- .../templates/stages/archetype-eval.yml | 11 ----- .../pipelines/templates/steps/eval-invoke.yml | 11 ++--- eng/common/scripts/eval/README.md | 17 +++++--- .../scripts/eval/test/shard-results.test.ts | 11 ++--- eng/common/scripts/eval/test/verdict.test.ts | 41 +++++++++++++++---- 7 files changed, 53 insertions(+), 47 deletions(-) diff --git a/eng/common/pipelines/live-eval.yml b/eng/common/pipelines/live-eval.yml index cc1ae1c0d6..c877b7894f 100644 --- a/eng/common/pipelines/live-eval.yml +++ b/eng/common/pipelines/live-eval.yml @@ -22,6 +22,8 @@ parameters: variables: # Managed-pool image selection (LINUXPOOL/LINUXVMIMAGE). Repo-local. - template: /eng/pipelines/templates/variables/image.yml + # EvalAzureSubscription is configured by the repo, not by synced templates. + - template: /eng/pipelines/templates/variables/globals.yml extends: template: /eng/common/pipelines/templates/stages/archetype-eval.yml @@ -37,7 +39,6 @@ extends: - 'workflows/live/*.eval.yaml' # Run each shard under AzureCLI@2 so the real MCP's DevOps calls are authenticated. UseAzSdkAuthentication: true - azureSubscription: opensource-api-connection failOnFailedTests: true # Live scenarios are end-to-end, so give each shard more headroom than the report-only tiers. shardTimeoutInMinutes: 45 diff --git a/eng/common/pipelines/templates/jobs/eval-shard.yml b/eng/common/pipelines/templates/jobs/eval-shard.yml index 942fca312b..747d54e96c 100644 --- a/eng/common/pipelines/templates/jobs/eval-shard.yml +++ b/eng/common/pipelines/templates/jobs/eval-shard.yml @@ -27,10 +27,6 @@ parameters: # Match Summary's gate; multi-run gating is not currently supported. type: boolean default: false - - name: azureSubscription - # Caller-owned service connection; required by the common authenticated invoke template. - type: string - default: '' - name: UseAzSdkAuthentication # Forwarded to the invoke template: true runs the shard under AzureCLI@2. type: boolean @@ -148,8 +144,6 @@ jobs: UseAzSdkAuthentication: ${{ parameters.UseAzSdkAuthentication }} threshold: ${{ parameters.threshold }} # Existing custom templates keep their contract unless opting into the argument list. - ${{ if eq(parameters.evalInvokeTemplate, '/eng/common/pipelines/templates/steps/eval-invoke.yml') }}: - azureSubscription: ${{ parameters.azureSubscription }} ${{ if ne(length(parameters.vallyArguments), 0) }}: vallyArguments: ${{ parameters.vallyArguments }} diff --git a/eng/common/pipelines/templates/stages/archetype-eval.yml b/eng/common/pipelines/templates/stages/archetype-eval.yml index fe88cac0ba..77fbbb9265 100644 --- a/eng/common/pipelines/templates/stages/archetype-eval.yml +++ b/eng/common/pipelines/templates/stages/archetype-eval.yml @@ -43,18 +43,10 @@ parameters: # When true, the invoke step runs under AzureCLI@2 (live MCP inherits the service connection). type: boolean default: false - - name: azureSubscription - # Caller-owned service connection; unused when authentication is disabled. - type: string - default: '' - name: vallyArguments # --tag does not filter Prepare's matrix; align evalGlobs so every eval has matching stimuli. type: object default: [] - - name: artifactPrefix - # Literal artifact prefix, including any separator (e.g. 'forced-'). - type: string - default: '' - name: threshold # Pass-rate gate forwarded to `vally eval --threshold` and the shard verdict. Repos may override. type: number @@ -112,10 +104,8 @@ extends: - template: /eng/common/pipelines/templates/jobs/eval-shard.yml parameters: vallyRoot: ${{ parameters.vallyRoot }} - artifactPrefix: ${{ parameters.artifactPrefix }} evalInvokeTemplate: ${{ parameters.evalInvokeTemplate }} UseAzSdkAuthentication: ${{ parameters.UseAzSdkAuthentication }} - azureSubscription: ${{ parameters.azureSubscription }} vallyArguments: ${{ parameters.vallyArguments }} failOnFailedTests: ${{ parameters.failOnFailedTests }} threshold: ${{ parameters.threshold }} @@ -130,7 +120,6 @@ extends: jobs: - template: /eng/common/pipelines/templates/jobs/eval-summarize.yml parameters: - artifactPrefix: ${{ parameters.artifactPrefix }} failOnFailedTests: ${{ parameters.failOnFailedTests }} createDashboardBundle: ${{ parameters.createDashboardBundle }} publishDashboardResults: ${{ parameters.publishDashboardResults }} diff --git a/eng/common/pipelines/templates/steps/eval-invoke.yml b/eng/common/pipelines/templates/steps/eval-invoke.yml index 2302fb03c3..a5f380924c 100644 --- a/eng/common/pipelines/templates/steps/eval-invoke.yml +++ b/eng/common/pipelines/templates/steps/eval-invoke.yml @@ -2,6 +2,7 @@ # node script directly. UseAzSdkAuthentication=true wraps the same run in AzureCLI@2 so a live # MCP's Azure DevOps calls inherit the service-connection identity. # The shard's login-to-copilot template supplies the executor and prompt-judge token. +# Authenticated callers define EvalAzureSubscription in pipeline YAML or a variable template. parameters: - name: vallyRoot @@ -13,10 +14,6 @@ parameters: # Pass-rate gate passed to `vally eval --threshold` and the shard verdict in invoke-eval-shard.js. type: number default: 0.8 - - name: azureSubscription - # Caller-owned service connection; required only for authenticated runs. - type: string - default: '' - name: vallyArguments # Supplementary --tag/--runs/--workers/--timeout argument pairs, not shell text. # --tag does not filter the matrix; callers must keep every planned eval non-empty. @@ -25,9 +22,9 @@ parameters: default: [] steps: - - ${{ if and(parameters.UseAzSdkAuthentication, eq(trim(parameters.azureSubscription), '')) }}: + - ${{ if and(parameters.UseAzSdkAuthentication, eq(trim(coalesce(variables.EvalAzureSubscription, ' ')), '')) }}: - script: | - echo "##vso[task.logissue type=error]azureSubscription must be provided when UseAzSdkAuthentication is true." + echo "##vso[task.logissue type=error]EvalAzureSubscription must be defined in YAML when UseAzSdkAuthentication is true." exit 1 displayName: 'Validate authenticated eval service connection' @@ -35,7 +32,7 @@ steps: - task: AzureCLI@2 displayName: 'vally eval $(shardName) (authenticated)' inputs: - azureSubscription: ${{ parameters.azureSubscription }} + azureSubscription: ${{ variables.EvalAzureSubscription }} scriptType: bash scriptLocation: inlineScript workingDirectory: ${{ parameters.vallyRoot }} diff --git a/eng/common/scripts/eval/README.md b/eng/common/scripts/eval/README.md index 07cb19aac1..a557103523 100644 --- a/eng/common/scripts/eval/README.md +++ b/eng/common/scripts/eval/README.md @@ -23,10 +23,9 @@ Repo-local adapters can customize it without copying that infrastructure: | Parameter | Contract | | --- | --- | | `matrixStageName` | Stage containing `generate_eval_matrix`; defaults to `Prepare`. The eval stage must depend on it. | -| `artifactPrefix` | Optional literal prefix, e.g. `forced-`. Common adds no separator: `eval-result-forced--` / `eval-debug-forced--` / `eval-summary-forced-`. Empty preserves existing names. Available through the stage and jobs; shard and summary prefixes must match. | +| `artifactPrefix` | Optional literal prefix, e.g. `forced-`. Common adds no separator: `eval-result-forced--` / `eval-debug-forced--` / `eval-summary-forced-`. Available only on the shard and summary job templates; their prefixes must match. The stage template uses unprefixed defaults. | | `preEvalTemplate` / `postEvalTemplate` | Existing parameter-free setup/teardown step templates. | | `evalInvokeTemplate` | Existing invocation template accepting `vallyRoot`, `UseAzSdkAuthentication` and `threshold`. | -| `azureSubscription` | Caller-owned Azure service connection, forwarded by the stage/job to the common invoke template. Defaults to empty. Required when `UseAzSdkAuthentication: true`; unauthenticated runs need no connection. | | `vallyArguments` | Optional array of supplementary Vally option/value pairs. Strings are required except that trial/worker counts also accept numbers. Available through the stage, shard job and common invoke template. Defaults to `[]`. | Supported supplementary options are `--tag`, `--runs`, `--workers` and `--timeout`. @@ -66,9 +65,17 @@ Zero-match evals are not treated as successful skips; a zero-trial shard still f the verdict/completeness checks even with `failOnFailedTests: false`. Use the existing pre/post-eval and MCP setup hooks for repository-specific work. -Authenticated callers must explicitly configure `azureSubscription`; common has -no deployment-specific connection fallback. An empty or whitespace-only value -fails with a configuration error without creating an `AzureCLI@2` task. +Authenticated callers define the pipeline variable `EvalAzureSubscription` in YAML +or an existing variable template. The common invoke step reads it directly; there +is no service-connection parameter to forward through stages and jobs, and no +deployment-specific fallback. An absent, empty or whitespace-only value fails with +a configuration error without creating an `AzureCLI@2` task. Unauthenticated runs +do not need the variable. +The value must be available during template expansion, not set by a task, output +variable or queue-time/runtime setting. The shared `live-eval.yml` entrypoint imports +the repo-owned `eng/pipelines/templates/variables/globals.yml`; this repo defines its +live connection there. Benchmarks define the same variable in their own pipeline +YAML. Other repos configure their own authorized connection in YAML. Do not replace authentication, result collection or retry/completeness checks. Supplementary reports must not rewrite JUnit thresholds or raw results. diff --git a/eng/common/scripts/eval/test/shard-results.test.ts b/eng/common/scripts/eval/test/shard-results.test.ts index 54deedb0bb..bead16236e 100644 --- a/eng/common/scripts/eval/test/shard-results.test.ts +++ b/eng/common/scripts/eval/test/shard-results.test.ts @@ -220,9 +220,9 @@ test("literal prefixes publish names selected by Summary without adding separato } }); -test("template source defaults to legacy names and forwards a single literal artifact prefix", () => { +test("job templates retain literal prefixes while the stage template uses unprefixed defaults", () => { const load = name => fs.readFileSync(new URL(`../../../pipelines/templates/${name}.yml`, import.meta.url), "utf8"); - for (const name of ["jobs/eval-shard", "jobs/eval-summarize", "stages/archetype-eval"]) { + for (const name of ["jobs/eval-shard", "jobs/eval-summarize"]) { const template = load(name); assert.match(template, /^ - name: artifactPrefix\r?\n(?: #[^\r\n]*\r?\n)* type: string\r?\n default: ''\r?$/m); assert.doesNotMatch(template, /^ - name: artifactNamespace\r?$/m); @@ -232,12 +232,7 @@ test("template source defaults to legacy names and forwards a single literal art assert.ok(render); assert.match(render, /^ EVAL_ARTIFACT_PREFIX: \$\{\{ parameters\.artifactPrefix \}\}\r?$/m); assert.match(summary, /^ - \$\{\{ if and\(ne\(parameters\.artifactPrefix, ''\), or\(parameters\.createDashboardBundle, parameters\.publishDashboardResults\)\) \}\}:\r?$/m); - const stage = load("stages/archetype-eval"); - for (const name of ["Eval", "Summary"]) { - const section = stage.split(/^ - stage: /m).find(value => value.split(/\r?\n/, 1)[0] === name); - assert.ok(section); - assert.match(section, /^ artifactPrefix: \$\{\{ parameters\.artifactPrefix \}\}\r?$/m); - } + assert.doesNotMatch(load("stages/archetype-eval"), /\bartifactPrefix\b/); }); test("Summary download source applies the literal prefix to all result-file patterns", () => { diff --git a/eng/common/scripts/eval/test/verdict.test.ts b/eng/common/scripts/eval/test/verdict.test.ts index 3a850dfa2a..80353471cc 100644 --- a/eng/common/scripts/eval/test/verdict.test.ts +++ b/eng/common/scripts/eval/test/verdict.test.ts @@ -6,6 +6,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; +import { runInNewContext } from "node:vm"; import { after, before, describe, it } from "node:test"; import { getVallyShardVerdict, parseFailedTestsGate } from "../lib/verdict.ts"; @@ -17,11 +18,11 @@ describe("eval template authentication source contract", () => { const invoke = load("../../../pipelines/templates/steps/eval-invoke.yml"); const branches = invoke.split(/(?=^ - \$\{\{ )/m).slice(1); - it("keeps the service connection optional and empty in every common template layer", () => { + it("keeps authentication opt-in without forwarding a service-connection parameter", () => { for (const template of [invoke, load("../../../pipelines/templates/jobs/eval-shard.yml"), load("../../../pipelines/templates/stages/archetype-eval.yml")]) { - assert.match(template, /^ - name: azureSubscription\r?\n(?: #[^\r\n]*\r?\n)* type: string\r?\n default: ''\r?$/m); + assert.doesNotMatch(template, /^ - name: azureSubscription\r?$|parameters\.azureSubscription/m); assert.match(template, /^ - name: UseAzSdkAuthentication\r?\n(?: #[^\r\n]*\r?\n)* type: boolean\r?\n default: false\r?$/m); } }); @@ -30,20 +31,40 @@ describe("eval template authentication source contract", () => { assert.equal(branches.length, 3); const configuration = branches[0]; assert.equal(configuration.split(/\r?\n/, 1)[0], - " - ${{ if and(parameters.UseAzSdkAuthentication, eq(trim(parameters.azureSubscription), '')) }}:"); + " - ${{ if and(parameters.UseAzSdkAuthentication, eq(trim(coalesce(variables.EvalAzureSubscription, ' ')), '')) }}:"); assert.equal(configuration.match(/^ - /gm)?.length, 1); assert.match(configuration, /^ - script: \|\r?$/m); assert.doesNotMatch(configuration, /^\s*- task:/m); - assert.match(configuration, /##vso\[task\.logissue type=error\].*azureSubscription.*UseAzSdkAuthentication/); + assert.match(configuration, /##vso\[task\.logissue type=error\].*EvalAzureSubscription.*UseAzSdkAuthentication/); assert.match(configuration, /^ exit 1\r?$/m); }); + it("requires a configured YAML connection only when authentication is enabled", () => { + const condition = branches[0].split(/\r?\n/, 1)[0].match(/if (.+) \}\}:$/)?.[1]; + assert.ok(condition); + for (const enabled of [false, true]) { + for (const connection of [undefined, null, "", " \t ", "repo-owned-connection"]) { + const missingConnection = runInNewContext(condition, { + parameters: { UseAzSdkAuthentication: enabled }, + variables: { EvalAzureSubscription: connection }, + and: (...values: boolean[]) => values.every(Boolean), + eq: (left: string, right: string) => left === right, + trim: (value: string) => value.trim(), + coalesce: (...values: (string | null | undefined)[]) => + values.find(value => value !== undefined && value !== null && value !== ""), + }); + assert.equal(missingConnection, enabled && !connection?.trim(), + `authentication=${enabled}, connection=${JSON.stringify(connection)}`); + } + } + }); + it("references the caller's connection only in the authenticated branch", () => { const authenticated = branches[1]; assert.equal(authenticated.split(/\r?\n/, 1)[0], " - ${{ elseif parameters.UseAzSdkAuthentication }}:"); assert.equal(authenticated.match(/^ - /gm)?.length, 1); assert.match(authenticated, /^ - task: AzureCLI@2\r?$/m); - assert.match(authenticated, /^ azureSubscription: \$\{\{ parameters\.azureSubscription \}\}\r?$/m); + assert.match(authenticated, /^ azureSubscription: \$\{\{ variables\.EvalAzureSubscription \}\}\r?$/m); const unauthenticated = branches[2]; assert.equal(unauthenticated.split(/\r?\n/, 1)[0], " - ${{ else }}:"); assert.equal(unauthenticated.match(/^ - /gm)?.length, 1); @@ -57,20 +78,22 @@ describe("eval template authentication source contract", () => { } }); - it("keeps the existing live entrypoint's connection explicit and forwards caller configuration", () => { + it("lets the live entrypoint inherit repo configuration without passing a connection", () => { const live = load("../../../pipelines/live-eval.yml"); assert.match(live, /^ UseAzSdkAuthentication: true\r?$/m); - assert.match(live, /^ azureSubscription: opensource-api-connection\r?$/m); + assert.match(live, /^ - template: \/eng\/pipelines\/templates\/variables\/globals\.yml\r?$/m); + assert.doesNotMatch(live, /^\s*azureSubscription:|variables\/eval\.yml/m); + assert.doesNotMatch(live, /opensource-api-connection/); const stage = load("../../../pipelines/templates/stages/archetype-eval.yml"); const evalStage = stage.split(/^ - stage: /m).find(section => section.split(/\r?\n/, 1)[0] === "Eval"); assert.ok(evalStage); assert.match(evalStage, /^ - template: \/eng\/common\/pipelines\/templates\/jobs\/eval-shard\.yml\r?$/m); - assert.match(evalStage, /^ azureSubscription: \$\{\{ parameters\.azureSubscription \}\}\r?$/m); + assert.doesNotMatch(evalStage, /^\s*azureSubscription:/m); const shard = load("../../../pipelines/templates/jobs/eval-shard.yml"); const invokeStep = shard.split(/^ - template: /m).find(section => section.split(/\r?\n/, 1)[0] === "${{ parameters.evalInvokeTemplate }}"); assert.ok(invokeStep); - assert.match(invokeStep, /^ \$\{\{ if eq\(parameters\.evalInvokeTemplate, '\/eng\/common\/pipelines\/templates\/steps\/eval-invoke\.yml'\) \}\}:\r?\n azureSubscription: \$\{\{ parameters\.azureSubscription \}\}\r?$/m); + assert.doesNotMatch(invokeStep, /^\s*azureSubscription:/m); }); it("preflights common shard arguments before setup and forwards the gate to both jobs", () => { From e19493a3c38efbca6f2f3abec746386509ec6a71 Mon Sep 17 00:00:00 2001 From: JoyerJin <116236375+JoyerJin@users.noreply.github.com> Date: Thu, 5 Nov 2026 17:11:15 +0800 Subject: [PATCH 6/9] sync with dashboard design and revert common's changes --- .../pipelines/templates/jobs/eval-shard.yml | 15 +-- .../templates/jobs/eval-summarize.yml | 31 +---- eng/common/scripts/eval/README.md | 36 ++--- eng/common/scripts/eval/build-eval-summary.ts | 3 +- eng/common/scripts/eval/lib/shard-results.ts | 14 +- .../scripts/eval/test/shard-results.test.ts | 123 +++--------------- 6 files changed, 46 insertions(+), 176 deletions(-) diff --git a/eng/common/pipelines/templates/jobs/eval-shard.yml b/eng/common/pipelines/templates/jobs/eval-shard.yml index 747d54e96c..e8adb85fce 100644 --- a/eng/common/pipelines/templates/jobs/eval-shard.yml +++ b/eng/common/pipelines/templates/jobs/eval-shard.yml @@ -6,15 +6,6 @@ parameters: - name: vallyRoot # Required (no default) so each repo passes its own eval root explicitly. type: string - - name: matrixStageName - # Stage containing the generate_eval_matrix job. - type: string - default: Prepare - - name: artifactPrefix - # Literal prefix after 'eval-result-' / 'eval-debug-'; include any separator (e.g. 'forced-'). - # Match Summary's prefix and keep final artifact names unique across this build's tracks. - type: string - default: '' - name: evalInvokeTemplate # Swap point: runs `vally eval` for the shard (the common template is auth-conditional). type: string @@ -61,7 +52,7 @@ jobs: workspace: clean: all strategy: - matrix: $[ stageDependencies.${{ parameters.matrixStageName }}.generate_eval_matrix.outputs['detect.matrix'] ] + matrix: $[ stageDependencies.Prepare.generate_eval_matrix.outputs['detect.matrix'] ] maxParallel: 10 templateContext: outputs: @@ -70,12 +61,12 @@ jobs: # The Summary job keeps only the highest attempt per shard so retries supersede cleanly. - output: pipelineArtifact displayName: 'Publish $(shardName) raw results, JUnit and completeness' - artifactName: 'eval-result-${{ parameters.artifactPrefix }}$(shardName)-$(System.JobAttempt)' + artifactName: 'eval-result-$(shardName)-$(System.JobAttempt)' targetPath: $(Build.ArtifactStagingDirectory)/eval-result condition: always() - output: pipelineArtifact displayName: 'Publish $(shardName) transcripts (on failure)' - artifactName: 'eval-debug-${{ parameters.artifactPrefix }}$(shardName)-$(System.JobAttempt)' + artifactName: 'eval-debug-$(shardName)-$(System.JobAttempt)' targetPath: $(Build.ArtifactStagingDirectory)/debug condition: failed() steps: diff --git a/eng/common/pipelines/templates/jobs/eval-summarize.yml b/eng/common/pipelines/templates/jobs/eval-summarize.yml index a9552c046a..46241aed8b 100644 --- a/eng/common/pipelines/templates/jobs/eval-summarize.yml +++ b/eng/common/pipelines/templates/jobs/eval-summarize.yml @@ -2,17 +2,6 @@ # gates on it when failOnFailedTests is true. parameters: - - name: matrixStageName - type: string - default: Prepare - - name: evalStageName - type: string - default: Eval - - name: artifactPrefix - # Literal prefix, including any separator (e.g. 'forced-'); must match the shard job. - # Callers are responsible for unique final artifact names across tracks in one build. - type: string - default: '' - name: failOnFailedTests # Only single-run evaluations support this gate; multi-run observations must leave it off. type: boolean @@ -34,22 +23,15 @@ jobs: workspace: clean: all variables: - EvalExpectedMatrix: $[ stageDependencies.${{ parameters.matrixStageName }}.generate_eval_matrix.outputs['detect.matrix'] ] + EvalExpectedMatrix: $[ stageDependencies.Prepare.generate_eval_matrix.outputs['detect.matrix'] ] templateContext: outputs: - output: pipelineArtifact displayName: 'Retain selected build summary and saved bundle' - artifactName: 'eval-summary-${{ parameters.artifactPrefix }}$(System.JobAttempt)' + artifactName: 'eval-summary-$(System.JobAttempt)' targetPath: $(Build.ArtifactStagingDirectory)/eval-summary condition: always() steps: - # Dashboard archives identify a single build-wide summary, not separate tracks. - - ${{ if and(ne(parameters.artifactPrefix, ''), or(parameters.createDashboardBundle, parameters.publishDashboardResults)) }}: - - script: | - echo "##vso[task.logissue type=error]Prefixed eval tracks support artifact-only summaries, not dashboard bundles or publication." - exit 1 - displayName: 'Reject ambiguous multi-track dashboard identity' - - checkout: self clean: true fetchDepth: 1 @@ -59,15 +41,14 @@ jobs: inputs: version: 24.x - # Download only this track's result data; an empty prefix retains all eval-result-* artifacts. - task: DownloadPipelineArtifact@2 displayName: 'Download all shard results' inputs: buildType: 'current' itemPattern: | - eval-result-${{ parameters.artifactPrefix }}*/results.jsonl - eval-result-${{ parameters.artifactPrefix }}*/shard.json - eval-result-${{ parameters.artifactPrefix }}*/**/*.junit.xml + eval-result-*/results.jsonl + eval-result-*/shard.json + eval-result-*/**/*.junit.xml targetPath: $(Pipeline.Workspace)/eval-results # Only attempt verification needs the current build token; rendering uses saved evidence. @@ -79,7 +60,6 @@ jobs: env: SYSTEM_ACCESSTOKEN: $(System.AccessToken) EVAL_EXPECTED_MATRIX: $(EvalExpectedMatrix) - EVAL_STAGE_NAME: ${{ parameters.evalStageName }} - script: | node --experimental-strip-types "$(Build.SourcesDirectory)/eng/common/scripts/eval/build-eval-summary.ts" \ @@ -91,7 +71,6 @@ jobs: condition: always() env: EVAL_EXPECTED_MATRIX: $(EvalExpectedMatrix) - EVAL_ARTIFACT_PREFIX: ${{ parameters.artifactPrefix }} - ${{ if or(parameters.createDashboardBundle, parameters.publishDashboardResults) }}: - template: ../steps/eval-publish-results.yml diff --git a/eng/common/scripts/eval/README.md b/eng/common/scripts/eval/README.md index a557103523..dd65fe9aca 100644 --- a/eng/common/scripts/eval/README.md +++ b/eng/common/scripts/eval/README.md @@ -22,8 +22,6 @@ Repo-local adapters can customize it without copying that infrastructure: | Parameter | Contract | | --- | --- | -| `matrixStageName` | Stage containing `generate_eval_matrix`; defaults to `Prepare`. The eval stage must depend on it. | -| `artifactPrefix` | Optional literal prefix, e.g. `forced-`. Common adds no separator: `eval-result-forced--` / `eval-debug-forced--` / `eval-summary-forced-`. Available only on the shard and summary job templates; their prefixes must match. The stage template uses unprefixed defaults. | | `preEvalTemplate` / `postEvalTemplate` | Existing parameter-free setup/teardown step templates. | | `evalInvokeTemplate` | Existing invocation template accepting `vallyRoot`, `UseAzSdkAuthentication` and `threshold`. | | `vallyArguments` | Optional array of supplementary Vally option/value pairs. Strings are required except that trial/worker counts also accept numbers. Available through the stage, shard job and common invoke template. Defaults to `[]`. | @@ -79,25 +77,17 @@ YAML. Other repos configure their own authorized connection in YAML. Do not replace authentication, result collection or retry/completeness checks. Supplementary reports must not rewrite JUnit thresholds or raw results. -For multiple tracks in one build, use an artifact prefix and configure -the common summary's `matrixStageName`, `evalStageName` and `artifactPrefix` -to match the shard job. Each Summary downloads only result-file patterns matching -its prefix; an empty prefix retains the build-wide `eval-result-*` download. -Each Summary stage must depend on both its Prepare and -Eval stages. Timeline attempts are checked against that exact Eval stage, and -only that track's latest verified attempts are selected. Prefixed tracks -support artifact-only summaries: dashboard bundles/publication are rejected -because the archive identity currently represents one build-wide Summary. -Prefixes and shard names must start with an ASCII letter, digit or underscore, -contain only ASCII letters, digits, underscores or hyphens, and be at most 200 -characters; prefixes may also be empty. Include any desired separator -in the prefix: `forced-` and `forced` are used exactly as supplied. Existing shard -names, including names with hyphens, remain supported. - -The caller must ensure final artifact names are unique across all tracks in the -build, including unprefixed tracks. Different prefixes alone do not guarantee this: -`forced-` + `area-foo` and `forced-area-` + `foo` produce the same name. -Common does not perform cross-track collision detection. +The shared job templates use the `Prepare` matrix and `Eval` timeline stage, with +one build-wide Summary depending on both stages. Result/debug artifacts are named +`eval-result--` / `eval-debug--`; the summary artifact +is `eval-summary-`. Summary downloads `eval-result-*` result data and selects +only the latest verified attempt for each expected shard. +Shard names must start with an ASCII letter, digit or underscore, contain only +ASCII letters, digits, underscores or hyphens, and be at most 200 characters. + +Benchmarks select one mode per build through repo-owned pipeline parameters, +aligned `evalGlobs` and `vallyArguments`. Run separate builds to compare modes; +multiple independently published tracks within one build are not supported. ## Complete build results and direct Blob publishing @@ -136,11 +126,11 @@ explicit bootstrap, without changing dependency checks or the frozen lock. | Control | Current use | | --- | --- | -| `publishDashboardResults` | The only checkbox; defaults to `true` in the three tools entrypoints. Set `false` to disable both upload and Azure Storage egress | +| `publishDashboardResults` | The only checkbox; defaults to `true` in the three standard tools entrypoints and the author benchmark. Set `false` to disable both upload and Azure Storage egress | | `EvalStorageAccountName` | Pipeline-owner configuration. Upload is skipped when absent; set it to `evaltestsummary` for this dashboard | | `EvalDashboardRefreshUrl` | Pipeline-owner configuration: `https://azsdk-eval-bue6a7dwanatgpb3.westus3-01.azurewebsites.net/api/refresh` | | `EvalDashboardResource` | Pipeline-owner configuration: `api://258998df-81ec-460c-bdd7-56a9bdde1e48` (resource URI, not a `/.default` scope) | -| `createDashboardBundle` | Shared-template artifact-only mode; enabled for all three tools entrypoints | +| `createDashboardBundle` | Shared-template artifact-only mode; enabled for the three standard tools entrypoints and the author benchmark | The pipeline owner must set **`EvalStorageAccountName=evaltestsummary`** and the two dashboard variables above in each onboarded DevOps pipeline, with **queue-time diff --git a/eng/common/scripts/eval/build-eval-summary.ts b/eng/common/scripts/eval/build-eval-summary.ts index 54b268ea60..27758d34bf 100644 --- a/eng/common/scripts/eval/build-eval-summary.ts +++ b/eng/common/scripts/eval/build-eval-summary.ts @@ -379,8 +379,7 @@ async function main(argv) { } } selection = selectSummaryResults({ resultsRoot: options.resultsRoot, - selectedRoot: options.selectedRoot, expectedShards, jobAttempts, - artifactPrefix: process.env.EVAL_ARTIFACT_PREFIX ?? "" }); + selectedRoot: options.selectedRoot, expectedShards, jobAttempts }); if (!matrixValid) { selection.status.push({ shard: "Prepare", attempt: null, complete: false, reason: "The expected shard matrix is unavailable or invalid; check the Prepare stage." }); diff --git a/eng/common/scripts/eval/lib/shard-results.ts b/eng/common/scripts/eval/lib/shard-results.ts index f1251d7805..7159d5c35e 100644 --- a/eng/common/scripts/eval/lib/shard-results.ts +++ b/eng/common/scripts/eval/lib/shard-results.ts @@ -31,14 +31,13 @@ export function expectedShardsFromMatrix(value) { } // The artifact list cannot reveal a newer retry that lost its agent before publishing. -export function latestShardAttempts(timeline, matrix, evalStageName = "Eval") { +export function latestShardAttempts(timeline, matrix) { expectedShardsFromMatrix(matrix); - if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(evalStageName)) throw new Error("Invalid eval stage identity."); if (!Array.isArray(timeline?.records)) throw new Error("The current build timeline is required."); const attempts = {}; for (const [key, entry] of Object.entries(matrix)) { if (!validShard(key)) throw new Error("Invalid matrix leg identity."); - const jobs = timeline.records.filter(record => record?.type === "Job" && record.identifier === `${evalStageName}.RunShard.${key}`); + const jobs = timeline.records.filter(record => record?.type === "Job" && record.identifier === `Eval.RunShard.${key}`); for (const record of jobs) { if (!Number.isSafeInteger(record.attempt) || record.attempt < 1) throw new Error("Invalid timeline attempt."); const current = { attempt: record.attempt, complete: record.state === "completed" && @@ -67,7 +66,7 @@ export async function readShardAttempts(env, { fetchImpl = fetch, wait = delay } response = await fetchImpl(url, { headers: { authorization: `Bearer ${env.SYSTEM_ACCESSTOKEN}` }, redirect: "error", signal: AbortSignal.timeout(30_000) }); if (!response.ok) throw new Error("Timeline could not be read."); - return latestShardAttempts(await response.json(), matrix, env.EVAL_STAGE_NAME ?? "Eval"); + return latestShardAttempts(await response.json(), matrix); } catch (error) { if ((response && ![408, 429, 500, 502, 503, 504].includes(response.status)) || attempt === 2) throw error; } finally { await response?.body?.cancel().catch(() => {}); } @@ -178,15 +177,12 @@ export function stageShardResults({ resultsRoot, outputDirectory, shardName, att return metadata; } -export function selectSummaryResults({ resultsRoot, selectedRoot, expectedShards, jobAttempts, artifactPrefix = "" }) { +export function selectSummaryResults({ resultsRoot, selectedRoot, expectedShards, jobAttempts }) { if (!Array.isArray(expectedShards) || expectedShards.some((name) => !validShard(name)) || new Set(expectedShards).size !== expectedShards.length) { throw new Error("Expected shards must be unique, valid shard names from Prepare."); } - if (typeof artifactPrefix !== "string" || (artifactPrefix !== "" && !validShard(artifactPrefix))) { - throw new Error("Invalid artifact prefix."); - } - const resultPrefix = `eval-result-${artifactPrefix}`; + const resultPrefix = "eval-result-"; emptyDirectory(selectedRoot); const expected = new Set(expectedShards); const timeline = jobAttempts?.schemaVersion === 1 && jobAttempts.valid === true ? jobAttempts.attempts : null; diff --git a/eng/common/scripts/eval/test/shard-results.test.ts b/eng/common/scripts/eval/test/shard-results.test.ts index bead16236e..74eb6541c3 100644 --- a/eng/common/scripts/eval/test/shard-results.test.ts +++ b/eng/common/scripts/eval/test/shard-results.test.ts @@ -70,7 +70,7 @@ function runSummary(root, shards, options = {}) { const child = runCli("build-eval-summary.ts", [ "--results-root", path.join(root, "download"), "--selected-root", path.join(root, "selected"), "--output-path", output, - ], { TF_BUILD: "true", EVAL_EXPECTED_MATRIX: matrix, EVAL_ARTIFACT_PREFIX: "", + ], { TF_BUILD: "true", EVAL_EXPECTED_MATRIX: matrix, EVAL_FAIL_ON_FAILED_TESTS: "false", ...options.env }); return { child, markdown: fs.readFileSync(output, "utf8"), @@ -152,90 +152,28 @@ test("single-run gating does not bypass missing-result completeness failures", ( assert.equal(result.result.complete, false); }); -test("same-build tracks select their own artifacts and authoritative retry evidence", (t) => { - const root = setup(t); - const shard = "area_same"; - const matrix = { leg: { shardName: shard } }; - const timeline = { records: [ - { type: "Job", identifier: "EvalForced.RunShard.leg", attempt: 1, state: "completed", result: "succeeded" }, - { type: "Job", identifier: "EvalForced.RunShard.leg", attempt: 2, state: "completed", result: "failed" }, - { type: "Job", identifier: "EvalTrigger.RunShard.leg", attempt: 3, state: "completed", result: "succeeded" }, - ] }; - for (const [prefix, attempt] of [["forced-", 1], ["forced-", 2], ["trigger-", 3], ["forced-extra-", 4]]) { - const source = invocation(root, `${prefix}${attempt}`); - stageShardResults({ resultsRoot: source, - outputDirectory: path.join(root, "download", `eval-result-${prefix}${shard}-${attempt}`), - shardName: shard, attempt }); - } - const evidence = latestShardAttempts(timeline, matrix, "EvalForced"); - fs.writeFileSync(path.join(root, "job-attempts.json"), JSON.stringify(evidence)); - const result = runSummary(root, [shard], { env: { EVAL_ARTIFACT_PREFIX: "forced-" } }); - assert.equal(result.child.status, 0, result.child.stderr); - assert.equal(result.result.complete, true); - assert.deepEqual(result.index.attempts, [{ - shard, attempt: 2, directory: `eval-result-forced-${shard}-2`, - }]); - assert.equal(result.result.totals.scenarios, 1); - assert.deepEqual(latestShardAttempts(timeline, matrix, "EvalTrigger").attempts[shard], { attempt: 3, complete: true }); - assert.throws(() => latestShardAttempts(timeline, matrix, "../Eval"), /Invalid eval stage/); - // A newer failed attempt without an artifact must not reuse the older complete run. - const selectedRoot = path.join(root, "selected-missing"); - const missing = selectSummaryResults({ - resultsRoot: path.join(root, "download"), selectedRoot, expectedShards: [shard], - artifactPrefix: "forced-", - jobAttempts: { schemaVersion: 1, valid: true, attempts: { [shard]: { attempt: 4, complete: true } } }, - }); - assert.equal(missing.complete, false); - assert.deepEqual(missing.shardInput.attempts, []); -}); - -test("literal prefixes publish names selected by Summary without adding separators", (t) => { +test("shard job artifact names preserve hyphenated shard identities through Summary selection", (t) => { const load = name => fs.readFileSync(new URL(`../../../pipelines/templates/jobs/${name}.yml`, import.meta.url), "utf8"); const shardTemplate = load("eval-shard"); const resultName = shardTemplate.match(/^ artifactName: '(eval-result-[^'\r\n]+)'\r?$/m); - const debugName = shardTemplate.match(/^ artifactName: '(eval-debug-[^'\r\n]+)'\r?$/m); - const summaryName = load("eval-summarize").match(/^ artifactName: '(eval-summary-[^'\r\n]+)'\r?$/m); assert.ok(resultName); - assert.ok(debugName); - assert.ok(summaryName); - for (const prefix of ["", "forced-", "trigger-", "forced", "forced-extra-"]) { - const root = setup(t); - const shard = "extra-area_same"; - const render = expression => expression.replace("${{ parameters.artifactPrefix }}", prefix) - .replace("$(shardName)", shard).replace("$(System.JobAttempt)", "1"); - const name = render(resultName[1]); - assert.equal(name, `eval-result-${prefix}${shard}-1`); - assert.equal(render(debugName[1]), `eval-debug-${prefix}${shard}-1`); - assert.equal(render(summaryName[1]), `eval-summary-${prefix}1`); - const source = invocation(root, "source"); - stageShardResults({ resultsRoot: source, outputDirectory: path.join(root, "download", name), - shardName: shard, attempt: 1 }); - fs.writeFileSync(path.join(root, "job-attempts.json"), JSON.stringify({ - schemaVersion: 1, valid: true, attempts: { [shard]: { attempt: 1, complete: true } }, - })); - const result = runSummary(root, [shard], { env: { EVAL_ARTIFACT_PREFIX: prefix } }); - assert.equal(result.child.status, 0, result.child.stderr); - assert.equal(result.result.complete, true); - assert.deepEqual(result.index.attempts, [{ shard, attempt: 1, directory: name }]); - } -}); - -test("job templates retain literal prefixes while the stage template uses unprefixed defaults", () => { - const load = name => fs.readFileSync(new URL(`../../../pipelines/templates/${name}.yml`, import.meta.url), "utf8"); - for (const name of ["jobs/eval-shard", "jobs/eval-summarize"]) { - const template = load(name); - assert.match(template, /^ - name: artifactPrefix\r?\n(?: #[^\r\n]*\r?\n)* type: string\r?\n default: ''\r?$/m); - assert.doesNotMatch(template, /^ - name: artifactNamespace\r?$/m); + const root = setup(t); + const shard = "extra-area_same"; + for (const attempt of [1, 2]) { + const name = resultName[1].replace("$(shardName)", shard).replace("$(System.JobAttempt)", String(attempt)); + stageShardResults({ resultsRoot: invocation(root, `source-${attempt}`), + outputDirectory: path.join(root, "download", name), shardName: shard, attempt }); } - const summary = load("jobs/eval-summarize"); - const render = summary.split(/^ - script: \|\r?$/m).find(section => section.includes("build-eval-summary.ts")); - assert.ok(render); - assert.match(render, /^ EVAL_ARTIFACT_PREFIX: \$\{\{ parameters\.artifactPrefix \}\}\r?$/m); - assert.match(summary, /^ - \$\{\{ if and\(ne\(parameters\.artifactPrefix, ''\), or\(parameters\.createDashboardBundle, parameters\.publishDashboardResults\)\) \}\}:\r?$/m); - assert.doesNotMatch(load("stages/archetype-eval"), /\bartifactPrefix\b/); + fs.writeFileSync(path.join(root, "job-attempts.json"), JSON.stringify({ + schemaVersion: 1, valid: true, attempts: { [shard]: { attempt: 2, complete: true } }, + })); + const result = runSummary(root, [shard]); + assert.equal(result.child.status, 0, result.child.stderr); + assert.equal(result.result.complete, true); + assert.deepEqual(result.index.attempts, [{ shard, attempt: 2, directory: `eval-result-${shard}-2` }]); }); -test("Summary download source applies the literal prefix to all result-file patterns", () => { +test("Summary downloads only build-wide result data", () => { const summary = fs.readFileSync(new URL("../../../pipelines/templates/jobs/eval-summarize.yml", import.meta.url), "utf8"); const download = summary.split(/^ - task: /m).find(section => section.split(/\r?\n/, 1)[0] === "DownloadPipelineArtifact@2"); @@ -244,31 +182,7 @@ test("Summary download source applies the literal prefix to all result-file patt assert.ok(block); const patterns = block[1].trim().split(/\r?\n/).map(line => line.trim()); const files = ["results.jsonl", "shard.json", "**/*.junit.xml"]; - assert.deepEqual(patterns, files.map(file => `eval-result-\${{ parameters.artifactPrefix }}*/${file}`)); - for (const prefix of ["", "forced-", "trigger-", "forced"]) { - assert.deepEqual(patterns.map(pattern => pattern.replace("${{ parameters.artifactPrefix }}", prefix)), - files.map(file => `eval-result-${prefix}*/${file}`)); - } -}); - -test("empty artifact prefix preserves legacy names and invalid prefixes are rejected", (t) => { - const root = setup(t); - const shard = "area_test"; - artifact(root, shard, 1); - const result = selectSummaryResults({ - resultsRoot: path.join(root, "download"), selectedRoot: path.join(root, "selected"), - expectedShards: [shard], jobAttempts: readJson(path.join(root, "job-attempts.json")), - artifactPrefix: "", - }); - assert.equal(result.complete, true); - assert.deepEqual(result.shardInput.attempts, [{ shard, attempt: 1, directory: `eval-result-${shard}-1` }]); - for (const [index, prefix] of ["../escape", "with spaces", "forced.extra", "/", null, 123, "x".repeat(201)].entries()) { - assert.throws(() => selectSummaryResults({ - resultsRoot: path.join(root, "download"), selectedRoot: path.join(root, `invalid-${index}`), - expectedShards: [shard], jobAttempts: readJson(path.join(root, "job-attempts.json")), - artifactPrefix: prefix, - }), /Invalid artifact prefix/); - } + assert.deepEqual(patterns, files.map(file => `eval-result-*/${file}`)); }); test("completed failing evaluations stage exact raw bytes and all JUnit, but no unrelated files", (t) => { @@ -667,7 +581,8 @@ const timelineEnv = { SYSTEM_COLLECTIONURI: "https://dev.azure.com/azure-sdk/", const timelineRecord = { type: "Job", identifier: "Eval.RunShard.a", attempt: 1, state: "completed", result: "failed" }; test("timeline uses matrix identifiers, highest attempts and completed state, rejecting ambiguous attempts", () => { - const matrix = { a: { shardName: "a" } }, records = [timelineRecord, { ...timelineRecord, attempt: 2 }]; + const matrix = { a: { shardName: "a" } }, records = [timelineRecord, { ...timelineRecord, attempt: 2 }, + { ...timelineRecord, identifier: "OtherEval.RunShard.a", attempt: 3 }]; assert.deepEqual(latestShardAttempts({ records }, matrix).attempts.a, { attempt: 2, complete: true }); records[1].result = "canceled"; assert.equal(latestShardAttempts({ records }, matrix).attempts.a.complete, false); From 07c4b0aaf5dff4f098f9f24db39270b29ce00703 Mon Sep 17 00:00:00 2001 From: JoyerJin <116236375+JoyerJin@users.noreply.github.com> Date: Fri, 6 Nov 2026 16:51:09 +0800 Subject: [PATCH 7/9] change parameter location --- eng/common/pipelines/live-eval.yml | 2 +- .../pipelines/templates/jobs/eval-shard.yml | 7 +++++ .../templates/stages/archetype-eval.yml | 5 +++ .../pipelines/templates/steps/eval-invoke.yml | 11 ++++--- eng/common/scripts/eval/README.md | 27 ++++++++++------ eng/common/scripts/eval/test/verdict.test.ts | 31 +++++++++---------- 6 files changed, 52 insertions(+), 31 deletions(-) diff --git a/eng/common/pipelines/live-eval.yml b/eng/common/pipelines/live-eval.yml index c877b7894f..b8c54882dd 100644 --- a/eng/common/pipelines/live-eval.yml +++ b/eng/common/pipelines/live-eval.yml @@ -22,7 +22,6 @@ parameters: variables: # Managed-pool image selection (LINUXPOOL/LINUXVMIMAGE). Repo-local. - template: /eng/pipelines/templates/variables/image.yml - # EvalAzureSubscription is configured by the repo, not by synced templates. - template: /eng/pipelines/templates/variables/globals.yml extends: @@ -39,6 +38,7 @@ extends: - 'workflows/live/*.eval.yaml' # Run each shard under AzureCLI@2 so the real MCP's DevOps calls are authenticated. UseAzSdkAuthentication: true + azureSubscription: ${{ variables.EvalAzureSubscription }} failOnFailedTests: true # Live scenarios are end-to-end, so give each shard more headroom than the report-only tiers. shardTimeoutInMinutes: 45 diff --git a/eng/common/pipelines/templates/jobs/eval-shard.yml b/eng/common/pipelines/templates/jobs/eval-shard.yml index e8adb85fce..60e5b2eb26 100644 --- a/eng/common/pipelines/templates/jobs/eval-shard.yml +++ b/eng/common/pipelines/templates/jobs/eval-shard.yml @@ -22,6 +22,10 @@ parameters: # Forwarded to the invoke template: true runs the shard under AzureCLI@2. type: boolean default: false + - name: azureSubscription + # Explicit caller-owned connection; no shared deployment fallback. + type: string + default: '' - name: threshold # Pass-rate gate forwarded to the invoke template (`vally eval --threshold` + shard verdict). type: number @@ -133,6 +137,9 @@ jobs: parameters: vallyRoot: ${{ parameters.vallyRoot }} UseAzSdkAuthentication: ${{ parameters.UseAzSdkAuthentication }} + # Custom invoke templates receive this only when explicitly configured. + ${{ if ne(parameters.azureSubscription, '') }}: + azureSubscription: ${{ parameters.azureSubscription }} threshold: ${{ parameters.threshold }} # Existing custom templates keep their contract unless opting into the argument list. ${{ if ne(length(parameters.vallyArguments), 0) }}: diff --git a/eng/common/pipelines/templates/stages/archetype-eval.yml b/eng/common/pipelines/templates/stages/archetype-eval.yml index 77fbbb9265..ccd4492ccd 100644 --- a/eng/common/pipelines/templates/stages/archetype-eval.yml +++ b/eng/common/pipelines/templates/stages/archetype-eval.yml @@ -43,6 +43,10 @@ parameters: # When true, the invoke step runs under AzureCLI@2 (live MCP inherits the service connection). type: boolean default: false + - name: azureSubscription + # Required by authenticated callers; configured outside eng/common. + type: string + default: '' - name: vallyArguments # --tag does not filter Prepare's matrix; align evalGlobs so every eval has matching stimuli. type: object @@ -106,6 +110,7 @@ extends: vallyRoot: ${{ parameters.vallyRoot }} evalInvokeTemplate: ${{ parameters.evalInvokeTemplate }} UseAzSdkAuthentication: ${{ parameters.UseAzSdkAuthentication }} + azureSubscription: ${{ parameters.azureSubscription }} vallyArguments: ${{ parameters.vallyArguments }} failOnFailedTests: ${{ parameters.failOnFailedTests }} threshold: ${{ parameters.threshold }} diff --git a/eng/common/pipelines/templates/steps/eval-invoke.yml b/eng/common/pipelines/templates/steps/eval-invoke.yml index a5f380924c..16a9b88ae1 100644 --- a/eng/common/pipelines/templates/steps/eval-invoke.yml +++ b/eng/common/pipelines/templates/steps/eval-invoke.yml @@ -2,7 +2,7 @@ # node script directly. UseAzSdkAuthentication=true wraps the same run in AzureCLI@2 so a live # MCP's Azure DevOps calls inherit the service-connection identity. # The shard's login-to-copilot template supplies the executor and prompt-judge token. -# Authenticated callers define EvalAzureSubscription in pipeline YAML or a variable template. +# Authenticated callers pass a repo-owned service connection explicitly. parameters: - name: vallyRoot @@ -10,6 +10,9 @@ parameters: - name: UseAzSdkAuthentication type: boolean default: false + - name: azureSubscription + type: string + default: '' - name: threshold # Pass-rate gate passed to `vally eval --threshold` and the shard verdict in invoke-eval-shard.js. type: number @@ -22,9 +25,9 @@ parameters: default: [] steps: - - ${{ if and(parameters.UseAzSdkAuthentication, eq(trim(coalesce(variables.EvalAzureSubscription, ' ')), '')) }}: + - ${{ if and(parameters.UseAzSdkAuthentication, eq(trim(parameters.azureSubscription), '')) }}: - script: | - echo "##vso[task.logissue type=error]EvalAzureSubscription must be defined in YAML when UseAzSdkAuthentication is true." + echo "##vso[task.logissue type=error]azureSubscription must be provided when UseAzSdkAuthentication is true." exit 1 displayName: 'Validate authenticated eval service connection' @@ -32,7 +35,7 @@ steps: - task: AzureCLI@2 displayName: 'vally eval $(shardName) (authenticated)' inputs: - azureSubscription: ${{ variables.EvalAzureSubscription }} + azureSubscription: ${{ parameters.azureSubscription }} scriptType: bash scriptLocation: inlineScript workingDirectory: ${{ parameters.vallyRoot }} diff --git a/eng/common/scripts/eval/README.md b/eng/common/scripts/eval/README.md index dd65fe9aca..b2432c0a37 100644 --- a/eng/common/scripts/eval/README.md +++ b/eng/common/scripts/eval/README.md @@ -24,6 +24,7 @@ Repo-local adapters can customize it without copying that infrastructure: | --- | --- | | `preEvalTemplate` / `postEvalTemplate` | Existing parameter-free setup/teardown step templates. | | `evalInvokeTemplate` | Existing invocation template accepting `vallyRoot`, `UseAzSdkAuthentication` and `threshold`. | +| `azureSubscription` | Optional service-connection name, default `''`. Required by the common invoke template when `UseAzSdkAuthentication: true`; explicitly forwarded through stages and jobs. | | `vallyArguments` | Optional array of supplementary Vally option/value pairs. Strings are required except that trial/worker counts also accept numbers. Available through the stage, shard job and common invoke template. Defaults to `[]`. | Supported supplementary options are `--tag`, `--runs`, `--workers` and `--timeout`. @@ -63,17 +64,23 @@ Zero-match evals are not treated as successful skips; a zero-trial shard still f the verdict/completeness checks even with `failOnFailedTests: false`. Use the existing pre/post-eval and MCP setup hooks for repository-specific work. -Authenticated callers define the pipeline variable `EvalAzureSubscription` in YAML -or an existing variable template. The common invoke step reads it directly; there -is no service-connection parameter to forward through stages and jobs, and no -deployment-specific fallback. An absent, empty or whitespace-only value fails with +Authenticated callers pass `azureSubscription` explicitly to the stage or shard +template. The parameter is forwarded to the common invoke step; shared templates +default to `''` and have no deployment-specific fallback. +An absent, empty or whitespace-only value fails with a configuration error without creating an `AzureCLI@2` task. Unauthenticated runs -do not need the variable. -The value must be available during template expansion, not set by a task, output -variable or queue-time/runtime setting. The shared `live-eval.yml` entrypoint imports -the repo-owned `eng/pipelines/templates/variables/globals.yml`; this repo defines its -live connection there. Benchmarks define the same variable in their own pipeline -YAML. Other repos configure their own authorized connection in YAML. +do not need the parameter. Custom invoke templates receive it only when nonempty +and must declare their own `azureSubscription` parameter to opt in. +Configure an authorized connection in repo-owned YAML and explicitly pass it from +the entrypoint to the stage template. Do not rely on an +ambient variable being visible across nested templates, or on task/output variables +that are unavailable during template expansion. +The `eng/common/pipelines/live-eval.yml` entrypoint imports the repo-owned +`eng/pipelines/templates/variables/globals.yml` and passes its `EvalAzureSubscription` +value as `azureSubscription`. Nested stage, job and invoke templates use only the +parameter. Scheduled runs use the same YAML configuration without a connection run +parameter. Other repos must configure their own authorized connection in that +repo-owned variable template. Do not replace authentication, result collection or retry/completeness checks. Supplementary reports must not rewrite JUnit thresholds or raw results. diff --git a/eng/common/scripts/eval/test/verdict.test.ts b/eng/common/scripts/eval/test/verdict.test.ts index 80353471cc..82cdcbf62a 100644 --- a/eng/common/scripts/eval/test/verdict.test.ts +++ b/eng/common/scripts/eval/test/verdict.test.ts @@ -18,11 +18,12 @@ describe("eval template authentication source contract", () => { const invoke = load("../../../pipelines/templates/steps/eval-invoke.yml"); const branches = invoke.split(/(?=^ - \$\{\{ )/m).slice(1); - it("keeps authentication opt-in without forwarding a service-connection parameter", () => { + it("keeps authentication opt-in with an empty connection default in every shared layer", () => { for (const template of [invoke, load("../../../pipelines/templates/jobs/eval-shard.yml"), load("../../../pipelines/templates/stages/archetype-eval.yml")]) { - assert.doesNotMatch(template, /^ - name: azureSubscription\r?$|parameters\.azureSubscription/m); + assert.match(template, /^ - name: azureSubscription\r?\n(?: #[^\r\n]*\r?\n)* type: string\r?\n default: ''\r?$/m); + assert.doesNotMatch(template, /EvalAzureSubscription|opensource-api-connection|azuresdkqabot-dev/); assert.match(template, /^ - name: UseAzSdkAuthentication\r?\n(?: #[^\r\n]*\r?\n)* type: boolean\r?\n default: false\r?$/m); } }); @@ -31,27 +32,25 @@ describe("eval template authentication source contract", () => { assert.equal(branches.length, 3); const configuration = branches[0]; assert.equal(configuration.split(/\r?\n/, 1)[0], - " - ${{ if and(parameters.UseAzSdkAuthentication, eq(trim(coalesce(variables.EvalAzureSubscription, ' ')), '')) }}:"); + " - ${{ if and(parameters.UseAzSdkAuthentication, eq(trim(parameters.azureSubscription), '')) }}:"); assert.equal(configuration.match(/^ - /gm)?.length, 1); assert.match(configuration, /^ - script: \|\r?$/m); assert.doesNotMatch(configuration, /^\s*- task:/m); - assert.match(configuration, /##vso\[task\.logissue type=error\].*EvalAzureSubscription.*UseAzSdkAuthentication/); + assert.match(configuration, /##vso\[task\.logissue type=error\].*azureSubscription.*UseAzSdkAuthentication/); assert.match(configuration, /^ exit 1\r?$/m); }); - it("requires a configured YAML connection only when authentication is enabled", () => { + it("uses only the explicit connection parameter when authentication is enabled", () => { const condition = branches[0].split(/\r?\n/, 1)[0].match(/if (.+) \}\}:$/)?.[1]; assert.ok(condition); for (const enabled of [false, true]) { - for (const connection of [undefined, null, "", " \t ", "repo-owned-connection"]) { + for (const connection of ["", " \t ", "repo-owned-connection"]) { const missingConnection = runInNewContext(condition, { - parameters: { UseAzSdkAuthentication: enabled }, - variables: { EvalAzureSubscription: connection }, + parameters: { UseAzSdkAuthentication: enabled, azureSubscription: connection }, + variables: { EvalAzureSubscription: "unrelated-outer-variable" }, and: (...values: boolean[]) => values.every(Boolean), eq: (left: string, right: string) => left === right, trim: (value: string) => value.trim(), - coalesce: (...values: (string | null | undefined)[]) => - values.find(value => value !== undefined && value !== null && value !== ""), }); assert.equal(missingConnection, enabled && !connection?.trim(), `authentication=${enabled}, connection=${JSON.stringify(connection)}`); @@ -64,7 +63,7 @@ describe("eval template authentication source contract", () => { assert.equal(authenticated.split(/\r?\n/, 1)[0], " - ${{ elseif parameters.UseAzSdkAuthentication }}:"); assert.equal(authenticated.match(/^ - /gm)?.length, 1); assert.match(authenticated, /^ - task: AzureCLI@2\r?$/m); - assert.match(authenticated, /^ azureSubscription: \$\{\{ variables\.EvalAzureSubscription \}\}\r?$/m); + assert.match(authenticated, /^ azureSubscription: \$\{\{ parameters\.azureSubscription \}\}\r?$/m); const unauthenticated = branches[2]; assert.equal(unauthenticated.split(/\r?\n/, 1)[0], " - ${{ else }}:"); assert.equal(unauthenticated.match(/^ - /gm)?.length, 1); @@ -78,22 +77,22 @@ describe("eval template authentication source contract", () => { } }); - it("lets the live entrypoint inherit repo configuration without passing a connection", () => { + it("resolves the live connection at the entrypoint and forwards it through stage and job parameters", () => { const live = load("../../../pipelines/live-eval.yml"); assert.match(live, /^ UseAzSdkAuthentication: true\r?$/m); assert.match(live, /^ - template: \/eng\/pipelines\/templates\/variables\/globals\.yml\r?$/m); - assert.doesNotMatch(live, /^\s*azureSubscription:|variables\/eval\.yml/m); - assert.doesNotMatch(live, /opensource-api-connection/); + assert.match(live, /^ azureSubscription: \$\{\{ variables\.EvalAzureSubscription \}\}\r?$/m); + assert.doesNotMatch(live, /^ - name: azureSubscription\r?$|parameters\.azureSubscription|opensource-api-connection|azuresdkqabot-dev/m); const stage = load("../../../pipelines/templates/stages/archetype-eval.yml"); const evalStage = stage.split(/^ - stage: /m).find(section => section.split(/\r?\n/, 1)[0] === "Eval"); assert.ok(evalStage); assert.match(evalStage, /^ - template: \/eng\/common\/pipelines\/templates\/jobs\/eval-shard\.yml\r?$/m); - assert.doesNotMatch(evalStage, /^\s*azureSubscription:/m); + assert.match(evalStage, /^ azureSubscription: \$\{\{ parameters\.azureSubscription \}\}\r?$/m); const shard = load("../../../pipelines/templates/jobs/eval-shard.yml"); const invokeStep = shard.split(/^ - template: /m).find(section => section.split(/\r?\n/, 1)[0] === "${{ parameters.evalInvokeTemplate }}"); assert.ok(invokeStep); - assert.doesNotMatch(invokeStep, /^\s*azureSubscription:/m); + assert.match(invokeStep, /^ \$\{\{ if ne\(parameters\.azureSubscription, ''\) \}\}:\r?\n azureSubscription: \$\{\{ parameters\.azureSubscription \}\}\r?$/m); }); it("preflights common shard arguments before setup and forwards the gate to both jobs", () => { From c81fdb723078086785237c9d2f686ae3e320de63 Mon Sep 17 00:00:00 2001 From: JoyerJin <116236375+JoyerJin@users.noreply.github.com> Date: Fri, 6 Nov 2026 17:12:56 +0800 Subject: [PATCH 8/9] revert eval-invoke default value --- .../pipelines/templates/jobs/eval-shard.yml | 2 +- .../templates/stages/archetype-eval.yml | 2 +- .../pipelines/templates/steps/eval-invoke.yml | 8 +++--- eng/common/scripts/eval/README.md | 14 +++++------ eng/common/scripts/eval/test/verdict.test.ts | 25 ++++++++++++++----- 5 files changed, 32 insertions(+), 19 deletions(-) diff --git a/eng/common/pipelines/templates/jobs/eval-shard.yml b/eng/common/pipelines/templates/jobs/eval-shard.yml index 60e5b2eb26..fc202be4b7 100644 --- a/eng/common/pipelines/templates/jobs/eval-shard.yml +++ b/eng/common/pipelines/templates/jobs/eval-shard.yml @@ -23,7 +23,7 @@ parameters: type: boolean default: false - name: azureSubscription - # Explicit caller-owned connection; no shared deployment fallback. + # Leave blank to preserve the invoke template's default connection. type: string default: '' - name: threshold diff --git a/eng/common/pipelines/templates/stages/archetype-eval.yml b/eng/common/pipelines/templates/stages/archetype-eval.yml index ccd4492ccd..7c2d570aa7 100644 --- a/eng/common/pipelines/templates/stages/archetype-eval.yml +++ b/eng/common/pipelines/templates/stages/archetype-eval.yml @@ -44,7 +44,7 @@ parameters: type: boolean default: false - name: azureSubscription - # Required by authenticated callers; configured outside eng/common. + # Optional override; blank preserves the invoke template's default connection. type: string default: '' - name: vallyArguments diff --git a/eng/common/pipelines/templates/steps/eval-invoke.yml b/eng/common/pipelines/templates/steps/eval-invoke.yml index 16a9b88ae1..2adb06f62d 100644 --- a/eng/common/pipelines/templates/steps/eval-invoke.yml +++ b/eng/common/pipelines/templates/steps/eval-invoke.yml @@ -2,7 +2,7 @@ # node script directly. UseAzSdkAuthentication=true wraps the same run in AzureCLI@2 so a live # MCP's Azure DevOps calls inherit the service-connection identity. # The shard's login-to-copilot template supplies the executor and prompt-judge token. -# Authenticated callers pass a repo-owned service connection explicitly. +# Callers may override the existing service connection through azureSubscription. parameters: - name: vallyRoot @@ -10,13 +10,13 @@ parameters: - name: UseAzSdkAuthentication type: boolean default: false - - name: azureSubscription - type: string - default: '' - name: threshold # Pass-rate gate passed to `vally eval --threshold` and the shard verdict in invoke-eval-shard.js. type: number default: 0.8 + - name: azureSubscription + type: string + default: opensource-api-connection - name: vallyArguments # Supplementary --tag/--runs/--workers/--timeout argument pairs, not shell text. # --tag does not filter the matrix; callers must keep every planned eval non-empty. diff --git a/eng/common/scripts/eval/README.md b/eng/common/scripts/eval/README.md index b2432c0a37..0d155327d5 100644 --- a/eng/common/scripts/eval/README.md +++ b/eng/common/scripts/eval/README.md @@ -24,7 +24,7 @@ Repo-local adapters can customize it without copying that infrastructure: | --- | --- | | `preEvalTemplate` / `postEvalTemplate` | Existing parameter-free setup/teardown step templates. | | `evalInvokeTemplate` | Existing invocation template accepting `vallyRoot`, `UseAzSdkAuthentication` and `threshold`. | -| `azureSubscription` | Optional service-connection name, default `''`. Required by the common invoke template when `UseAzSdkAuthentication: true`; explicitly forwarded through stages and jobs. | +| `azureSubscription` | Optional service-connection override. Stage/job templates default to `''` and omit an empty override; the common invoke template retains its `opensource-api-connection` default. | | `vallyArguments` | Optional array of supplementary Vally option/value pairs. Strings are required except that trial/worker counts also accept numbers. Available through the stage, shard job and common invoke template. Defaults to `[]`. | Supported supplementary options are `--tag`, `--runs`, `--workers` and `--timeout`. @@ -64,10 +64,10 @@ Zero-match evals are not treated as successful skips; a zero-trial shard still f the verdict/completeness checks even with `failOnFailedTests: false`. Use the existing pre/post-eval and MCP setup hooks for repository-specific work. -Authenticated callers pass `azureSubscription` explicitly to the stage or shard -template. The parameter is forwarded to the common invoke step; shared templates -default to `''` and have no deployment-specific fallback. -An absent, empty or whitespace-only value fails with +Authenticated callers can pass `azureSubscription` explicitly to the stage or shard +template. Nonempty overrides are forwarded to the common invoke step; otherwise +the invoke template retains its existing `opensource-api-connection` default. +An explicitly empty or whitespace-only value passed directly to the invoke template fails with a configuration error without creating an `AzureCLI@2` task. Unauthenticated runs do not need the parameter. Custom invoke templates receive it only when nonempty and must declare their own `azureSubscription` parameter to opt in. @@ -133,11 +133,11 @@ explicit bootstrap, without changing dependency checks or the frozen lock. | Control | Current use | | --- | --- | -| `publishDashboardResults` | The only checkbox; defaults to `true` in the three standard tools entrypoints and the author benchmark. Set `false` to disable both upload and Azure Storage egress | +| `publishDashboardResults` | The only checkbox; defaults to `true` in the three standard tools entrypoints. Set `false` to disable both upload and Azure Storage egress | | `EvalStorageAccountName` | Pipeline-owner configuration. Upload is skipped when absent; set it to `evaltestsummary` for this dashboard | | `EvalDashboardRefreshUrl` | Pipeline-owner configuration: `https://azsdk-eval-bue6a7dwanatgpb3.westus3-01.azurewebsites.net/api/refresh` | | `EvalDashboardResource` | Pipeline-owner configuration: `api://258998df-81ec-460c-bdd7-56a9bdde1e48` (resource URI, not a `/.default` scope) | -| `createDashboardBundle` | Shared-template artifact-only mode; enabled for the three standard tools entrypoints and the author benchmark | +| `createDashboardBundle` | Shared-template artifact-only mode; enabled for the three standard tools entrypoints | The pipeline owner must set **`EvalStorageAccountName=evaltestsummary`** and the two dashboard variables above in each onboarded DevOps pipeline, with **queue-time diff --git a/eng/common/scripts/eval/test/verdict.test.ts b/eng/common/scripts/eval/test/verdict.test.ts index 82cdcbf62a..b2091d8f54 100644 --- a/eng/common/scripts/eval/test/verdict.test.ts +++ b/eng/common/scripts/eval/test/verdict.test.ts @@ -18,13 +18,18 @@ describe("eval template authentication source contract", () => { const invoke = load("../../../pipelines/templates/steps/eval-invoke.yml"); const branches = invoke.split(/(?=^ - \$\{\{ )/m).slice(1); - it("keeps authentication opt-in with an empty connection default in every shared layer", () => { + it("preserves the invoke connection default and leaves upstream overrides optional", () => { + assert.match(invoke, /^ - name: azureSubscription\r?\n type: string\r?\n default: opensource-api-connection\r?$/m); for (const template of [invoke, + load("../../../pipelines/templates/jobs/eval-shard.yml"), + load("../../../pipelines/templates/stages/archetype-eval.yml")]) { + assert.match(template, /^ - name: UseAzSdkAuthentication\r?\n(?: #[^\r\n]*\r?\n)* type: boolean\r?\n default: false\r?$/m); + } + for (const template of [ load("../../../pipelines/templates/jobs/eval-shard.yml"), load("../../../pipelines/templates/stages/archetype-eval.yml")]) { assert.match(template, /^ - name: azureSubscription\r?\n(?: #[^\r\n]*\r?\n)* type: string\r?\n default: ''\r?$/m); assert.doesNotMatch(template, /EvalAzureSubscription|opensource-api-connection|azuresdkqabot-dev/); - assert.match(template, /^ - name: UseAzSdkAuthentication\r?\n(?: #[^\r\n]*\r?\n)* type: boolean\r?\n default: false\r?$/m); } }); @@ -40,20 +45,28 @@ describe("eval template authentication source contract", () => { assert.match(configuration, /^ exit 1\r?$/m); }); - it("uses only the explicit connection parameter when authentication is enabled", () => { + it("uses the default when omitted and the caller's override when provided", () => { + const defaultConnection = invoke.match(/^ default: (opensource-api-connection)\r?$/m)?.[1]; + assert.ok(defaultConnection); const condition = branches[0].split(/\r?\n/, 1)[0].match(/if (.+) \}\}:$/)?.[1]; + const input = branches[1].match(/azureSubscription: \$\{\{ (.+) \}\}/)?.[1]; assert.ok(condition); + assert.ok(input); for (const enabled of [false, true]) { - for (const connection of ["", " \t ", "repo-owned-connection"]) { + for (const connection of [undefined, "", " \t ", "repo-owned-connection"]) { + const parameters = { UseAzSdkAuthentication: enabled, azureSubscription: connection ?? defaultConnection }; const missingConnection = runInNewContext(condition, { - parameters: { UseAzSdkAuthentication: enabled, azureSubscription: connection }, + parameters, variables: { EvalAzureSubscription: "unrelated-outer-variable" }, and: (...values: boolean[]) => values.every(Boolean), eq: (left: string, right: string) => left === right, trim: (value: string) => value.trim(), }); - assert.equal(missingConnection, enabled && !connection?.trim(), + assert.equal(missingConnection, enabled && !(connection ?? defaultConnection).trim(), `authentication=${enabled}, connection=${JSON.stringify(connection)}`); + if (enabled && !missingConnection) { + assert.equal(runInNewContext(input, { parameters }), connection ?? defaultConnection); + } } } }); From 0d2ccfe575cf62cb0238744007d7834d9dea8abb Mon Sep 17 00:00:00 2001 From: JoyerJin <116236375+JoyerJin@users.noreply.github.com> Date: Mon, 9 Nov 2026 15:11:34 +0800 Subject: [PATCH 9/9] update allowed list to reserved list --- .../pipelines/templates/steps/eval-invoke.yml | 2 +- eng/common/scripts/eval/README.md | 76 ++++++++++--------- eng/common/scripts/eval/invoke-eval-shard.ts | 29 +++++-- eng/common/scripts/eval/test/verdict.test.ts | 48 ++++++++++-- 4 files changed, 102 insertions(+), 53 deletions(-) diff --git a/eng/common/pipelines/templates/steps/eval-invoke.yml b/eng/common/pipelines/templates/steps/eval-invoke.yml index 2adb06f62d..3e0c354e70 100644 --- a/eng/common/pipelines/templates/steps/eval-invoke.yml +++ b/eng/common/pipelines/templates/steps/eval-invoke.yml @@ -18,7 +18,7 @@ parameters: type: string default: opensource-api-connection - name: vallyArguments - # Supplementary --tag/--runs/--workers/--timeout argument pairs, not shell text. + # Supplementary non-reserved --option value pairs and --switch flags, not shell text. # --tag does not filter the matrix; callers must keep every planned eval non-empty. # JSON-escape dollar signs below so agent macros cannot rewrite argument data. type: object diff --git a/eng/common/scripts/eval/README.md b/eng/common/scripts/eval/README.md index 0d155327d5..0e44d2a64e 100644 --- a/eng/common/scripts/eval/README.md +++ b/eng/common/scripts/eval/README.md @@ -25,43 +25,45 @@ Repo-local adapters can customize it without copying that infrastructure: | `preEvalTemplate` / `postEvalTemplate` | Existing parameter-free setup/teardown step templates. | | `evalInvokeTemplate` | Existing invocation template accepting `vallyRoot`, `UseAzSdkAuthentication` and `threshold`. | | `azureSubscription` | Optional service-connection override. Stage/job templates default to `''` and omit an empty override; the common invoke template retains its `opensource-api-connection` default. | -| `vallyArguments` | Optional array of supplementary Vally option/value pairs. Strings are required except that trial/worker counts also accept numbers. Available through the stage, shard job and common invoke template. Defaults to `[]`. | - -Supported supplementary options are `--tag`, `--runs`, `--workers` and `--timeout`. -For example, `['--tag', 'mode=forced', '--runs', '3']` selects a local benchmark -configuration without replacing the common runner. Trial/worker counts must be -positive integer strings or safe integer numbers. Azure template expansion can -serialize numeric parameters as JSON numbers even when using `format`; the runner -validates and converts only `--runs` / `--workers` numeric values to strings. -All other options and values must be strings. Both authentication branches forward the array as JSON in -`VALLY_ARGUMENTS_JSON`; the runner passes literal arguments to the pinned CLI using -Node directly, without a shell or whitespace splitting of supplementary values. -The template JSON-escapes dollar signs before agent macro expansion; the runner's -JSON decoding restores them. Tags such as `build=$(Build.BuildId)` stay literal, -even when the pipeline variable exists. -Unsupported flags (including aliases and `--flag=value`) are rejected. Eval -selection, output paths, JUnit, plugins and verdict settings remain framework-owned. -Legacy custom invoke templates retain their existing parameter contract when the -array is empty. A custom template opting into a non-empty array must declare its -own `vallyArguments` parameter; otherwise template compilation rejects it. - -`--runs` greater than 1 must not be combined with `failOnFailedTests: true`. -Use `failOnFailedTests: false` (the default) for multi-run observations; -explicit `--runs 1` may still be used with the gate enabled. Unsupported -combinations fail the common shard's configuration check before installation or -evaluation. When composing shard and summary jobs directly, pass the same -`failOnFailedTests` value to both. This check covers explicit `--runs` values in -`vallyArguments` only; Summary does not infer run counts from results. Callers must -ensure that repetition configured in `.vally.yaml` or custom invoke templates is -not combined with the enabled gate. - -`--tag` filters stimuli during execution only; it does not filter Prepare's matrix. -The caller must align `evalGlobs` (or its custom matrix) with the tags so every -planned eval retains at least one matching stimulus. For example, `--tag area=github` -alone does not remove other areas from the matrix: also restrict the selected files -to the intended GitHub evals. Recheck this alignment when adding evals or changing tags. -Zero-match evals are not treated as successful skips; a zero-trial shard still fails -the verdict/completeness checks even with `failOnFailedTests: false`. +| `vallyArguments` | Optional array of supplementary Vally options, values and switches. Strings are required except that trial/worker counts also accept numbers. Available through the stage, shard job and common invoke template. Defaults to `[]`. | + +**Not allowed in `vallyArguments`:** + +| Affected part | Reserved options | Reason | +| --- | --- | --- | +| Shard structure and execution context | `--eval-spec` / `-e`, `--no-eval-spec`, `--suite`, `--no-suite`, `--eval-plugin`, `--no-eval-plugin`, `--work-dir`, `--no-work-dir` | Prepare already assigns files to shards. These change eval selection/loading or the working directory; `--suite` also conflicts with the assigned `-e` arguments. Use `evalGlobs` and `vallyRoot` instead. | +| Result collection and Summary | `--output-dir`, `--no-output-dir`, `--output`, `--no-output`, `--junit`, `--no-junit` | The pipeline owns result locations and enables JUnit. In Vally 0.19.0, `--output jsonl` sends results to stdout instead of `results.jsonl`. | +| Validation and scoring | `--threshold`, `--no-threshold`, `--skip-grade`, `--no-skip-grade`, `--skip-validate`, `--no-skip-validate` | The runner does not support bypassing validation/grading, and scoring must match the shard gate. Set the template's `threshold` parameter instead. | +| Exit-code gating | `--require-pass`, `--no-require-pass` | The shard decides pass/fail from `results.jsonl`, not Vally's exit code; this option would not control the pipeline gate. | + +**Allowed:** other Vally options using `--option value` or standalone switches such as +`--verbose`. This includes `--tag`, `--runs`, `--workers`, `--timeout`, +`--model` and supplementary plugin options. New options need no common-template changes; +Vally validates their names, values and whether they require a value. +Aliases, `--flag=value` and positional paths/globs are not supported. +Values must be nonempty strings without null characters and must not start with `-`; +`--runs` and `--workers` also accept positive safe integer numbers. +Arguments are passed literally, without a shell or whitespace splitting. + +For example, replace `` and `` with a tag defined in your eval stimuli: + +```yaml +vallyArguments: + - --tag + - '=' + - --runs + - 3 + - --workers + - 2 + - --verbose +failOnFailedTests: false +``` + +Multiple runs require `failOnFailedTests: false`, including repetition configured +outside `vallyArguments`. `--tag` uses keys and values defined in your eval stimuli, +not predefined mode names. Tags filter stimuli, not the shard matrix: keep +`evalGlobs` aligned so every selected eval has matching stimuli. +Custom invoke templates must declare `vallyArguments` to receive a non-empty array. Use the existing pre/post-eval and MCP setup hooks for repository-specific work. Authenticated callers can pass `azureSubscription` explicitly to the stage or shard diff --git a/eng/common/scripts/eval/invoke-eval-shard.ts b/eng/common/scripts/eval/invoke-eval-shard.ts index bf71969c35..4d86403016 100644 --- a/eng/common/scripts/eval/invoke-eval-shard.ts +++ b/eng/common/scripts/eval/invoke-eval-shard.ts @@ -16,16 +16,30 @@ export function validateVallyArguments(value, { failOnFailedTests = false } = {} (typeof arg === "string" && arg.includes("\0")))) { throw new Error("Vally arguments must be an array of strings or numeric count values without null characters."); } - // Deliberately exclude eval selection, plugins, output, JUnit and verdict overrides. - const allowed = new Set(["--tag", "--runs", "--workers", "--timeout"]); + // These options would change matrix selection, result collection or the verdict contract. + const reserved = new Set([ + "--eval-spec", "--suite", "--work-dir", "--eval-plugin", + "--output-dir", "--output", "--junit", "--threshold", + "--require-pass", "--skip-grade", "--skip-validate", + ]); const args = []; - for (let index = 0; index < value.length; index += 2) { + for (let index = 0; index < value.length; index++) { const option = value[index]; - if (!allowed.has(option)) { - throw new Error(`Unsupported supplementary Vally option: ${option}`); + if (typeof option !== "string" || !/^--[a-z][a-z0-9]*(?:-[a-z0-9]+)*$/.test(option)) { + throw new Error(`Unsupported supplementary Vally option format: ${option}. Use --option value or --switch, not aliases or --option=value.`); + } + if (reserved.has(option) || reserved.has(option.replace(/^--no-/, "--"))) { + throw new Error(`Reserved Vally option: ${option}. Eval selection, output and verdict settings are framework-owned.`); } let argument = value[index + 1]; const isCount = option === "--runs" || option === "--workers"; + if (argument === undefined || (typeof argument === "string" && argument.startsWith("-"))) { + if (isCount) { + throw new Error(`Missing value for ${option}`); + } + args.push(option); + continue; + } // Azure template expansion can serialize even format('{0}', number) as a JSON number. if (typeof argument === "number") { if (!isCount || !Number.isSafeInteger(argument) || argument < 1) { @@ -33,7 +47,7 @@ export function validateVallyArguments(value, { failOnFailedTests = false } = {} } argument = String(argument); } - if (!argument || argument.startsWith("-")) { + if (!argument) { throw new Error(`Missing value for ${option}`); } if (isCount && @@ -44,6 +58,7 @@ export function validateVallyArguments(value, { failOnFailedTests = false } = {} assertSupportedEvalGate({ runs: Number(argument), failOnFailedTests }); } args.push(option, argument); + index++; } return args; } @@ -57,7 +72,7 @@ export function validateVallyArguments(value, { failOnFailedTests = false } = {} * @param {string} options.shardName Shard name (log messages + result folder). * @param {string} options.outputDir `--output-dir` Vally writes results into. * @param {number} [options.threshold] Pass-rate gate forwarded to `vally eval --threshold`. - * @param {(string | number)[]} [options.vallyArguments] Allowlisted arguments; numeric trial/worker counts are normalized. + * @param {(string | number)[]} [options.vallyArguments] Non-reserved options and switches; numeric trial/worker counts are normalized. * @returns {number} 0 when the shard verdict passes, 1 otherwise. */ export function runShard({ evalArgs, shardName, outputDir, threshold = 0.8, vallyArguments = [] }, diff --git a/eng/common/scripts/eval/test/verdict.test.ts b/eng/common/scripts/eval/test/verdict.test.ts index b2091d8f54..8fc89c01e6 100644 --- a/eng/common/scripts/eval/test/verdict.test.ts +++ b/eng/common/scripts/eval/test/verdict.test.ts @@ -178,14 +178,31 @@ describe("supplementary Vally arguments", () => { assert.deepEqual(options.vallyArguments, args); }); - it("accepts only supported option/value pairs without splitting values", () => { + it("passes non-reserved options through without a catalog or splitting values", () => { const args = ["--tag", "label=two words; $(not-a-command) & \"quoted\"", "--runs", "3", "--workers", "1", "--timeout", "5m"]; + args.push("--model", "example-model", "--max-retries", "0", "--reporter-plugin", "./reporter.js", + "--executor-plugin", "./executor.js", "--grader-plugin", "./grader.js", "--future-option", "literal value"); assert.deepEqual(validateVallyArguments(args), args); const options = parseArgs(["--eval-args", "-e example.eval.yaml", "--shard-name", "example", "--output-dir", "out"], { VALLY_ARGUMENTS_JSON: JSON.stringify(args) }); assert.deepEqual(options.vallyArguments, args); }); + it("preserves switches before, between and after option values without a switch catalog", () => { + for (const args of [ + ["--verbose"], + ["--verbose", "--in-place", "--future-switch"], + ["--verbose", "--model", "example-model", "--future-switch", "--runs", 2, "--in-place"], + ]) { + const expected = args.map(String); + assert.deepEqual(validateVallyArguments(args), expected); + assert.deepEqual(parseArgs(["--validate-only"], + { VALLY_ARGUMENTS_JSON: JSON.stringify(args) }).vallyArguments, expected); + } + // Vally, not the wrapper, validates whether ordinary options require values. + assert.deepEqual(validateVallyArguments(["--model"]), ["--model"]); + }); + it("normalizes numeric trial and worker counts produced by Azure template expansion", () => { for (const mode of ["forced", "trigger", "no-skill"]) { for (const runs of [1, 3]) { @@ -200,14 +217,27 @@ describe("supplementary Vally arguments", () => { } }); - it("rejects output, verdict, discovery and plugin overrides, including aliases and equals syntax", () => { - for (const option of ["--output-dir", "--output-dir=elsewhere", "--junit", "--no-junit", "--threshold", - "--require-pass", "--skip-grade", "--skip-validate", "-e", "--eval-spec", "--suite", "--work-dir", - "--reporter-plugin", "--executor-plugin", "--grader-plugin", "--runs=3", "--", "-r"]) { - assert.throws(() => validateVallyArguments([option, "value"]), /Unsupported/); + it("rejects framework-owned output, verdict and eval selection overrides", () => { + for (const option of ["--output-dir", "--output", "--junit", "--threshold", + "--require-pass", "--skip-grade", "--skip-validate", "--eval-spec", "--suite", "--work-dir", "--eval-plugin"]) { + for (const spelling of [option, option.replace(/^--/, "--no-")]) { + assert.throws(() => validateVallyArguments([spelling, "value"]), /Reserved Vally option/); + assert.throws(() => validateVallyArguments(["--verbose", spelling]), /Reserved Vally option/); + } + } + }); + + it("rejects aliases, equals syntax, positional eval paths and malformed arguments", () => { + for (const option of ["-e", "-eelsewhere.eval.yaml", "-r", "--", "--output-dir=elsewhere", + "--no-junit=false", "--runs=3", "--model=example", "elsewhere.eval.yaml", "**/*.eval.yaml", + "--model example", "--model\n", "--model;echo"]) { + assert.throws(() => validateVallyArguments([option, "value"]), /Unsupported.*format/); } - for (const args of [null, {}, " --runs 3", [3], ["--runs"], ["--runs", "0"], ["--runs", "-1"], + for (const args of [null, {}, " --runs 3", [3], ["--runs"], ["--workers"], + ["--runs", "--verbose"], ["--workers", "--in-place"], ["--runs", "0"], ["--runs", "-1"], ["--workers", "1.5"], ["--tag", "--threshold"], ["--tag", "value\0"], + ["--model", ""], ["--model", 1], ["--model", null], ["--model", "value\0"], + ["--model", "example", "elsewhere.eval.yaml"], ["--runs", null], ["--runs", true], ["--runs", {}], ["--runs", []], ["--runs", 0], ["--runs", -1], ["--runs", 1.5], ["--runs", NaN], ["--workers", Infinity], ["--workers", Number.MAX_SAFE_INTEGER + 1], @@ -235,7 +265,9 @@ describe("supplementary Vally arguments", () => { type: "run-summary", evals: [{ name: "test", passed: true, stimuliRun: 1 }] })); `); - const args = ["--tag", "label=a b; $(echo bad) & \"quotes\" %PATH%", "--tag", "build=$(Build.BuildId)", "--runs", 3]; + const args = ["--verbose", "--tag", "label=a b; $(echo bad) & \"quotes\" %PATH%", "--tag", "build=$(Build.BuildId)", "--runs", 3, + "--in-place", "--model", "example-model", "--reporter-plugin", "./reporter with spaces.js", + "--future-option", "literal value", "--future-switch"]; const options = parseArgs(["--eval-args", "-e test.eval.yaml", "--shard-name", "test", "--output-dir", outputDir], { VALLY_ARGUMENTS_JSON: JSON.stringify(args).replaceAll("$", "\\u0024") }); const exit = runShard(options, {