Add Vally skill-effectiveness experiments and Python test scenarios

[LarryOsterman]

Split from #375 (5 of 5).

Adds the Vally skill-effectiveness experiment framework and Python test scenarios:

• New scripts: run-skill-experiments.ps1, run-all-evals.ps1, generate-skill-experiments.ps1, compare-experiment.mjs
• Standardized vally/ eval + experiment yaml and scenarios.yaml for ~40 Python skills
• Shared Python grader tooling under tests/scenarios/_shared/
• Updates to run-harness-by-language.ps1 and test dependency manifests

178 files under tests/ (excludes tests/harness/ and *-rust scenarios, covered by #380 and #381).

[Copilot]

Pull request overview

Adds a standardized Vally “skill effectiveness” experiment setup for Python skills under tests/scenarios/, including shared Python grader tooling and updated test-runner dependencies/ignores. This PR is part of the split from #375 and focuses on the Python scenario/eval content (not harness/CI).

Changes:

• Adds shared Node-based graders for Python syntax + basic idiomatic checks under tests/scenarios/_shared/vally/tools/.
• Introduces per-skill Vally eval.yaml + skill_effectiveness_experiment.yaml (and, for some, skill_effectiveness_eval.yaml) plus updates many scenarios.yaml model defaults.
• Updates tests/package.json (Vally version + package manager metadata) and expands tests/.gitignore to ignore evaluation output folders.

Reviewed changes

Copilot reviewed 176 out of 178 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/package.json	Bumps @microsoft/vally and adds packageManager metadata for tests tooling.
tests/.gitignore	Ignores additional eval output directories under tests/.
tests/scenarios/_shared/vally/tools/check-python-syntax.mjs	New shared Python syntax validation grader (supports optional exclude patterns config).
tests/scenarios/_shared/vally/tools/check-python-idiomatic.mjs	New shared heuristic Python “idiomatic” checks grader.
tests/scenarios/_shared/vally/azure-sdk-python-skill-evaluations.yaml	Aggregates many Python skill evals into a single suite file.
tests/scenarios/agent-framework-azure-ai-py/vally/skill_effectiveness_experiment.yaml	Adds skill-vs-baseline experiment definition for this skill.
tests/scenarios/agent-framework-azure-ai-py/vally/eval.yaml	Adds Vally eval suite for this skill’s scenarios.
tests/scenarios/agent-framework-azure-ai-py/vally/skill_effectiveness_eval.yaml	Adds the skill-effectiveness eval suite definition for this skill.
tests/scenarios/agent-framework-azure-ai-py/scenarios.yaml	Sets scenario model config for this skill.
tests/scenarios/agent-framework-azure-ai-py/test-output.txt	Adds a captured output artifact for this scenario directory.
tests/scenarios/azure-search-documents-py/vally/skill_effectiveness_experiment.yaml	Adds skill-vs-baseline experiment definition for this skill.
tests/scenarios/azure-search-documents-py/vally/eval.yaml	Adds Vally eval suite for this skill’s scenarios.
tests/scenarios/azure-search-documents-py/scenarios.yaml	Updates scenario model config for this skill.
tests/scenarios/azure-eventgrid-py/vally/skill_effectiveness_experiment.yaml	Adds skill-vs-baseline experiment definition for this skill.
tests/scenarios/azure-eventgrid-py/vally/eval.yaml	Adds Vally eval suite for this skill’s scenarios.
tests/scenarios/azure-eventgrid-py/scenarios.yaml	Updates scenario model config for this skill.
tests/scenarios/azure-messaging-webpubsubservice-py/vally/skill_effectiveness_experiment.yaml	Adds skill-vs-baseline experiment definition for this skill.
tests/scenarios/azure-messaging-webpubsubservice-py/vally/eval.yaml	Adds Vally eval suite for this skill’s scenarios.
tests/scenarios/azure-messaging-webpubsubservice-py/scenarios.yaml	Updates scenario model config for this skill.
tests/scenarios/azure-monitor-opentelemetry-py/vally/skill_effectiveness_experiment.yaml	Adds skill-vs-baseline experiment definition for this skill.
tests/scenarios/azure-monitor-opentelemetry-py/vally/eval.yaml	Adds Vally eval suite for this skill’s scenarios.
tests/scenarios/azure-monitor-opentelemetry-py/scenarios.yaml	Updates scenario model config for this skill.
tests/scenarios/pydantic-models-py/vally/syntax-check-config.json	Adds per-scenario syntax-check exclusion config.
tests/scenarios/pydantic-models-py/vally/skill_effectiveness_experiment.yaml	Adds skill-vs-baseline experiment definition for this skill.
tests/scenarios/pydantic-models-py/scenarios.yaml	Updates scenario model config for this skill.
tests/scenarios/fastapi-router-py/vally/syntax-check-config.json	Adds per-scenario syntax-check exclusion config.
tests/scenarios/fastapi-router-py/vally/skill_effectiveness_experiment.yaml	Adds skill-vs-baseline experiment definition for this skill.
tests/scenarios/azure-storage-queue-py/vally/skill_effectiveness_experiment.yaml	Adds skill-vs-baseline experiment definition for this skill.
tests/scenarios/azure-storage-queue-py/vally/eval.yaml	Adds Vally eval suite for this skill’s scenarios.
tests/scenarios/azure-storage-queue-py/vally/skill_effectiveness_eval.yaml	Adds the skill-effectiveness eval suite definition for this skill.
tests/scenarios/azure-storage-queue-py/scenarios.yaml	Updates scenario model config for this skill.
tests/scenarios/m365-agents-py/vally/skill_effectiveness_experiment.yaml	Adds skill-vs-baseline experiment definition for this skill.
tests/scenarios/m365-agents-py/vally/eval.yaml	Adds Vally eval suite for this skill’s scenarios.
tests/scenarios/m365-agents-py/vally/skill_effectiveness_eval.yaml	Adds the skill-effectiveness eval suite definition for this skill.
tests/scenarios/m365-agents-py/scenarios.yaml	Updates scenario model config for this skill.
tests/scenarios/azure-ai-language-conversations-py/scenarios.yaml	Adds scenario definitions and mock responses for this skill.
tests/scenarios/azure-ai-language-conversations-py/acceptance-criteria.md	Expands/clarifies acceptance criteria with correct/incorrect examples.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

[LarryOsterman]

Split from #375 (see #375 for the full split plan). Do not merge #375 until this and the sibling split PRs (#379, #380, #381, #382, #383) are merged.

[Copilot]

Pull request overview

Copilot reviewed 176 out of 178 changed files in this pull request and generated 7 comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

[Copilot]

Pull request overview

Copilot reviewed 176 out of 178 changed files in this pull request and generated 16 comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

[richardpark-msft]

Some stuff I think is worth fixing:

• Let's use a real python linter (pylint?). Our skills should produce clean code anyways.
• Can you add in a README.md too? There's some handy little workflows in here, hard to discover without a small roadmap.

[Copilot]

Pull request overview

Copilot reviewed 148 out of 150 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (7)

tests/run-skill-experiments.ps1:1085

• The script validates the package-local Vally binary above, but the actual runner invokes a global vally. A standard setup following the new README only installs dependencies under tests, so both sequential runs and Start-Job workers can fail with “vally not recognized” despite the preflight succeeding. Invoke pnpm --dir <tests> exec vally inside the runner and pass the tests directory into background jobs.

            $output = & vally experiment run skill_effectiveness_experiment.yaml --output-dir $skillOutputRoot 2>&1

tests/package.json:36

• This package-manager pin conflicts with .github/workflows/skill-evaluation.yml:42-45, which explicitly installs pnpm 9 before running pnpm install. pnpm's package-manager version enforcement will make the nightly workflow fail before tests run. Update that workflow to use the pinned pnpm 11.10.0 (or let Corepack honor this field) in the same change.

  "packageManager": "pnpm@11.10.0+sha512.0b7f8b98060031904c017e3a41eb187a16d40eeb829b95c4f8cb03681761fc4ab53dd219115b9b447f4dce1a05a214764461e7d3703392a9f32f9511ce8c86c8"

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:58

• This template now renders /projects/{project}_id after replacing {{resource_name}}, while the endpoint parameter is project_id. FastAPI rejects routes whose path placeholder has no matching function parameter. Keep _id inside the literal braces so the rendered route is /projects/{project_id}.

@router.get("/{{resource_plural}}/{{{resource_name}}}_id", response_model={{ResourceName}})

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:96

• The PATCH template renders a {resource} path parameter but the handler accepts resource_id, causing FastAPI route registration to fail after placeholder substitution. Keep _id inside the path braces.

@router.patch("/{{resource_plural}}/{{{resource_name}}}_id", response_model={{ResourceName}})

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:127

• The DELETE route has the same placeholder regression: it renders {resource}_id, which does not match the handler's resource_id argument, so generated routers cannot start. Keep _id inside the braces.

    "/{{resource_plural}}/{{{resource_name}}}_id",

.github/workflows/test-harness.yml:1

• Removing the PR smoke-test workflow leaves no replacement check for harness unit tests, type checking, or skill-link synchronization: run-vally-evaluations.yml only triggers for tests/scenarios/**/vally/**. It also contradicts this split's description, which assigns workflow/harness updates to #380. Keep this workflow change in #380 or include its replacement here so these changes remain CI-covered.
  tests/scenarios/azure-eventhub-rust/scenarios.yaml:27
• This Rust scenario change contradicts the PR's stated Python-only scope and its explicit note that *-rust scenarios are covered by #381. Move these Rust corrections (including the acceptance-criteria update) to #381 so the split PRs do not overlap or create merge conflicts.

[Copilot]

Pull request overview

Copilot reviewed 148 out of 150 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (2)

tests/package.json:36

• Pinning pnpm 11.10 here conflicts with the remaining nightly harness workflow, which explicitly installs pnpm 9 (.github/workflows/skill-evaluation.yml:42-45) before running pnpm install. Align that workflow with this pin; otherwise the only remaining TypeScript/harness CI path can fail the package-manager version check or run with an unsupported pnpm version.

  "packageManager": "pnpm@11.10.0+sha512.0b7f8b98060031904c017e3a41eb187a16d40eeb829b95c4f8cb03681761fc4ab53dd219115b9b447f4dce1a05a214764461e7d3703392a9f32f9511ce8c86c8"

.github/workflows/test-harness.yml:1

• Deleting this workflow removes all pull-request and push execution of TypeScript typechecking, unit tests, skill-link checks, and mock harness evaluations. The remaining skill-evaluation.yml is nightly/manual and may be disabled, while run-vally-evaluations.yml only covers Vally paths. Keep or replace these PR smoke checks before removing the workflow.

[Copilot]

Pull request overview

Copilot reviewed 147 out of 149 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (1)

tests/scenarios/azure-eventhub-rust/acceptance-criteria.md:15

• The examples below now import azure_core::credentials::TokenCredential, but this setup immediately removes azure_core after adding it. Following these commands still leaves the examples uncompilable. Keep azure_core installed.

[Copilot]

Pull request overview

Copilot reviewed 147 out of 149 changed files in this pull request and generated 1 comment.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (7)

tests/generate-skill-experiments.ps1:99

• The generated baseline removes all skills, but every referenced eval.yaml retains constraints.expect_skills for that skill. Vally treats this as a required skill activation, so the baseline is guaranteed to violate a constraint and the A/B score measures a forced baseline failure rather than skill effectiveness. Use a baseline-compatible eval/constraint override (and include that path in vary) or omit the activation constraint for experiments, then regenerate the checked-in experiment files.

  sonnet_baseline:
    overrides:
      model: claude-sonnet-4.6
    environment:
      skills: []

tests/run-skill-experiments.ps1:1085

• This bypasses the package-local command that was computed above. pnpm exec only adds node_modules/.bin for its child process, so a normal local install does not leave a bare vally command available here; parallel Start-Job workers are especially isolated. Pass the tests directory into the worker and invoke pnpm --dir <tests> exec vally ... there.

            $output = & vally experiment run skill_effectiveness_experiment.yaml --output-dir $skillOutputRoot 2>&1

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:58

• After placeholder replacement this route becomes /items/{item}_id, while the handler parameter is item_id. FastAPI therefore sees an unmatched item path parameter and the generated router cannot initialize. Keep _id inside the placeholder braces.

@router.get("/{{resource_plural}}/{{{resource_name}}}_id", response_model={{ResourceName}})

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:96

• After placeholder replacement this route becomes /items/{item}_id, but the update handler accepts item_id; FastAPI cannot bind that path parameter. Keep _id inside the braces.

@router.patch("/{{resource_plural}}/{{{resource_name}}}_id", response_model={{ResourceName}})

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:127

• After placeholder replacement this route exposes {item} followed by a literal _id, while the delete handler expects item_id. This prevents FastAPI from binding the generated route. Keep _id inside the braces.

    "/{{resource_plural}}/{{{resource_name}}}_id",

tests/scenarios/azure-eventhub-rust/scenarios.yaml:27

• These Rust scenario edits contradict this PR's stated scope (*-rust scenarios are explicitly excluded and assigned to #381). Keeping them here risks conflicting fixes and makes the split non-independent; remove both azure-eventhub-rust file changes from this PR.
  tests/scenarios/azure-eventhub-rust/acceptance-criteria.md:15
• This command adds azure_core, but the very next command removes it even though the updated examples import azure_core::credentials::TokenCredential. Rust cannot use that transitive crate after removal, so following the “correct” setup produces an uncompilable project. Keep azure_core as a direct dependency.

[Copilot]

Pull request overview

Copilot reviewed 148 out of 150 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

[Copilot]

Pull request overview

Copilot reviewed 148 out of 150 changed files in this pull request and generated 40 comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (8)

tests/pnpm-workspace.yaml:5

• allowBuilds values must be booleans. This placeholder string makes the pnpm 11 workspace policy invalid, so the frozen install performed by the new eval runner/CI cannot complete. better-sqlite3 is a native dependency of @microsoft/vally-server, so explicitly allow its build.

  better-sqlite3: set this to true or false

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:58

• This template now renders /items/{item}_id, while the handler argument is item_id. FastAPI therefore cannot bind the route parameter and rejects the route at startup. The literal closing brace must remain after _id.

@router.get("/{{resource_plural}}/{{{resource_name}}}_id", response_model={{ResourceName}})

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:96

• This template renders the path parameter as {item} followed by literal _id, but the function expects item_id. FastAPI will report that the path parameter is missing from the function signature. Keep _id inside the route braces.

@router.patch("/{{resource_plural}}/{{{resource_name}}}_id", response_model={{ResourceName}})

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:127

• The generated DELETE route becomes /items/{item}_id, which does not match the item_id handler parameter and prevents FastAPI from registering the endpoint. Move the closing route brace after _id.

    "/{{resource_plural}}/{{{resource_name}}}_id",

tests/scenarios/azure-eventhub-rust/acceptance-criteria.md:14

• The next command removes azure_core, but the updated producer and consumer examples directly import azure_core::credentials::TokenCredential. Following this “correct” dependency sequence therefore leaves the examples uncompilable; keep azure_core installed.
  tests/run-skill-experiments.ps1:1042
• The script requires PowerShell 7 and documents Windows usage, but pnpm exec which vally depends on the Unix-only which command. On Windows this preflight reports Vally missing even though the newly added local CLI is installed. Probe pnpm exec vally directly and check $LASTEXITCODE instead.

$vallyExe = & pnpm --dir $PSScriptRoot exec which vally 2>$null
if (-not $vallyExe) {
    Write-Error "vally command not found. Please install @microsoft/vally via pnpm."

tests/run-skill-experiments.ps1:1085

• This invokes a global vally executable, even though setup installs Vally only as a local tests dependency and the preflight explicitly resolves it through pnpm. On a normal checkout (and inside Start-Job, which gets a fresh PATH), every experiment fails with command-not-found. Invoke pnpm --dir <tests> exec vally and pass the tests path into the job script.

            $output = & vally experiment run skill_effectiveness_experiment.yaml --output-dir $skillOutputRoot 2>&1

tests/scenarios/README.md:39

• The documented one-time setup installs only pnpm dependencies, but every Python eval runs python -m pylint and pylint is only declared in tests/requirements.txt. A fresh local setup following these steps therefore fails all idiomatic graders with “No module named pylint.” Add the pip requirements install used by CI to this setup sequence.

3. Install test dependencies:
   pnpm --dir tests install --frozen-lockfile

4. Approve required build scripts (if prompted):
   pnpm --dir tests approve-builds

[Copilot]

Pull request overview

Copilot reviewed 148 out of 150 changed files in this pull request and generated 5 comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (2)

.github/plugins/azure-sdk-rust/skills/azure-eventhub-rust/SKILL.md:162

• This rule incorrectly applies the consumer signature to producers. ProducerClientBuilder::open expects &str, while only the consumer builder requires an owned String; following this guidance breaks producer examples.

3. **Pass the hub name as an owned `String`.** `.open()` takes `hub_name: String`, not `&str` — call `.to_string()` on `&str` values.

tests/scenarios/azure-eventhub-rust/acceptance-criteria.md:199

• This is a producer example, whose .open() method takes the Event Hub name as &str. Converting eventhub to String contradicts the actual producer API and can make the documented example fail to compile.

[Copilot]

Fixed in c4527a9. The skill now shows DeveloperToolsCredential::new(None)? being passed directly into .open() and no longer requires an explicit Arc<dyn TokenCredential> binding.

[Copilot]

Fixed in c4527a9. I removed the incorrect producer anti-pattern that treated a direct DeveloperToolsCredential::new(None)? binding as invalid.

[Copilot]

Pull request overview

Copilot reviewed 148 out of 150 changed files in this pull request and generated 4 comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (13)

tests/pnpm-workspace.yaml:5

• allowBuilds entries must be booleans under pnpm 11; this generated placeholder makes the frozen install preflight fail before any eval runs. Since Vally 0.10 brings in native better-sqlite3, explicitly approve its build.

  better-sqlite3: set this to true or false

tests/run-skill-experiments.ps1:1040

• This preflight relies on the Unix which command, so it fails on Windows even though the script documents Windows usage. Resolve and validate the platform-specific package-local node_modules/.bin/vally executable instead; that executable should then be passed into the worker script block.

$vallyExe = & pnpm --dir $PSScriptRoot exec which vally 2>$null

tests/run-skill-experiments.ps1:1085

• The setup only proves that Vally is available through package-local pnpm exec, but the actual worker invokes bare vally. A normal local install does not put that binary on the global PATH, so every experiment fails with command-not-found. Invoke the resolved package-local executable while preserving $vallyDir as the working directory, and pass it into Start-Job workers.

            $output = & vally experiment run skill_effectiveness_experiment.yaml --output-dir $skillOutputRoot 2>&1

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:58

• After placeholder substitution this route becomes /projects/{project}_id, while the endpoint parameter is project_id. FastAPI then treats project_id as a query parameter instead of the path parameter. Keep _id inside the outer route braces so the generated path is /projects/{project_id}.

@router.get("/{{resource_plural}}/{{{resource_name}}_id}", response_model={{ResourceName}})

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:96

• This generates /projects/{project}_id, which does not bind the function's project_id argument as a path parameter. Keep _id inside the outer route braces.

@router.patch("/{{resource_plural}}/{{{resource_name}}_id}", response_model={{ResourceName}})

.github/plugins/azure-sdk-python/skills/fastapi-router-py/assets/template.py:127

• This generates /projects/{project}_id, so the delete handler's project_id is not supplied from the path. Keep _id inside the outer route braces.

    "/{{resource_plural}}/{{{resource_name}}_id}",

tests/scenarios/azure-eventhub-rust/acceptance-criteria.md:184

• The producer builder takes the Event Hub name as &str (also documented in the changed skill at line 155). Converting it to an owned String produces a type mismatch, so this “correct” example does not compile.
  tests/scenarios/azure-eventhub-rust/acceptance-criteria.md:116
• This “correct” example unnecessarily requires azure_core, contradicting the changed skill's direct-credential guidance and the installation example that removes azure_core. Use the concrete credential returned by DeveloperToolsCredential::new so the documented setup compiles.
  tests/scenarios/fastapi-router-py/vally/eval.yaml:89
• This second idiomatic grader also omits the exclusion config, so it can fail on the unreplaced asset template rather than the generated task router. Pass the same config used by the syntax grader.
  tests/scenarios/pydantic-models-py/vally/eval.yaml:86
• This second idiomatic grader also scans the placeholder template without the exclusion config and can fail independently of the generated answer. Pass the same config used by the syntax grader.
  tests/scenarios/fastapi-router-py/vally/eval.yaml:43
• The same placeholder template excluded from syntax checking is still collected by this idiomatic checker, which supports the same config argument. Pylint can parse assets/template.py and fail on its unreplaced placeholders, making the grader fail independently of the generated answer. Pass the exclusion config here too.
  tests/scenarios/pydantic-models-py/vally/eval.yaml:42
• The idiomatic checker scans the same Python files as the syntax checker and accepts the same optional exclusion config, but it is not given that config here. It can therefore run Pylint over the placeholder template.py that this eval explicitly excludes from syntax validation and fail regardless of the generated models.
  .github/scripts/check_skill_rust_syntax.py:28
• This allowlist omits common parser diagnostics such as expected \;`, found .... For those malformed snippets Cargo exits nonzero, _extract_syntax_errorsreturns empty, and_validate_rust_source` treats the failure as a non-fatal dependency error, so invalid Rust passes. Classify parser diagnostics comprehensively rather than relying on an incomplete phrase list.

SYNTAX_PATTERNS = [
    "expected item",
    "expected one of",
    "expected expression",
    "expected identifier",

[Copilot]

Pull request overview

Copilot reviewed 148 out of 150 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (8)

tests/pnpm-workspace.yaml:5

• allowBuilds values must be booleans. This placeholder string makes the pnpm 11 workspace policy invalid (and leaves better-sqlite3's native build undecided), so the documented frozen install/preflight cannot succeed reliably. Approve the dependency explicitly.

  better-sqlite3: set this to true or false

tests/scenarios/azure-eventhub-rust/acceptance-criteria.md:184

• The producer builder takes the Event Hub name as &str; only ConsumerClient::builder().open(...) requires an owned String (also documented in the updated SKILL.md at lines 154-155). Calling to_string() here makes this “correct” producer example use the wrong argument type.
  tests/generate-skill-experiments.ps1:83
• Generated experiments reuse eval.yaml, but every new Python stimulus in those files has constraints.expect_skills while sonnet_baseline removes all skills. The baseline is therefore guaranteed to violate the activation constraint, biasing the central skill-effectiveness comparison. Generate/use an experiment-specific eval without expect_skills (while retaining it for normal capability evals), or vary the constraint per variant, then regenerate the committed experiments.

evals:
    - eval.yaml

tests/run-skill-experiments.ps1:1085

• The preflight resolves Vally through pnpm's package-local binary, but each worker invokes bare vally. After the documented local pnpm install (without a global CLI), this throws command-not-found, including inside Start-Job. Invoke pnpm --dir <tests> exec vally in the worker or pass the resolved executable path into every job.

            $output = & vally experiment run skill_effectiveness_experiment.yaml --output-dir $skillOutputRoot 2>&1

tests/compare-experiment.mjs:84

• Both inputs are JSONL streams, but all results.jsonl record types are currently counted and the entire summary stream is parsed as one JSON document. Non-trial-result records dilute averages with zeros, and a multi-line summary throws. Parse records line-by-line, retain only trials, and select the latest summary record as run-all-evals.ps1 already does.

  // results.jsonl may be multi-line (one record per stimulus); aggregate them
  const rows = fs.readFileSync(path.join(dir, 'results.jsonl'), 'utf8')
    .split('\n').filter(l => l.trim())
    .map(l => JSON.parse(l));
  const summary = JSON.parse(fs.readFileSync(path.join(dir, 'run-summary.jsonl'), 'utf8').trim());

tests/compare-experiment.mjs:298

• This verdict can report “SKILL IS BETTER” even when the skill variant has a materially lower quality score, provided both variants barely pass and the skill uses fewer tokens/time. Include non-regression of avgScore in the condition (or report quality and efficiency separately) so the effectiveness conclusion is not misleading.

if (bothPass) {
  const skillBetter = sS.totalTokens < bS.totalTokens && sS.wallTimeSec < bS.wallTimeSec;
  const msg = skillBetter
    ? `✅  Skill variant passes with ${tokenDelta} fewer tokens and ${timeDelta} less wall time → SKILL IS BETTER`
    : `⚠️  Skill variant does not uniformly reduce both tokens and time — review per-metric deltas above.`;

tests/scenarios/README.md:39

• The one-time setup installs Node dependencies but never installs tests/requirements.txt. Every new idiomatic Python grader executes python -m pylint, so a clean setup following this guide fails with “No module named pylint.” Add the Python requirements installation before running evals.

3. Install test dependencies:
   pnpm --dir tests install --frozen-lockfile

4. Approve required build scripts (if prompted):
   pnpm --dir tests approve-builds

tests/scenarios/azure-eventhub-rust/scenarios.yaml:85

• The PR description explicitly says *-rust scenarios are excluded here and covered by #381, but this PR also modifies the Event Hubs Rust scenario (and its acceptance criteria). That overlaps the stated split and risks conflicting or duplicating #381; remove the Rust changes from this PR or update the split/description if ownership changed.

[Copilot]

Pull request overview

Copilot reviewed 150 out of 153 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

[Copilot]

Pull request overview

Copilot reviewed 190 out of 193 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (6)

tests/pnpm-workspace.yaml:5

• This is the placeholder emitted by pnpm's approval flow, not a valid boolean policy value. The preflight runs pnpm install --frozen-lockfile, and Vally 0.10 pulls in native better-sqlite3, so leaving this string prevents a reproducible install/build. Commit the approval as a boolean.

  better-sqlite3: set this to true or false

tests/run-skill-experiments.ps1:1079

• The script verifies the package-local Vally binary above, but then invokes bare vally. node_modules/.bin is not normally on PATH, and detached Start-Job workers do not gain it, so the documented local pnpm setup still fails with “vally not recognized.” Pass the resolved executable (or a pnpm --dir ... exec vally invocation) into the runner job and invoke that value here.

            $output = & vally experiment run skill_effectiveness_experiment.yaml --output-dir $skillOutputRoot 2>&1

tests/compare-experiment.mjs:85

• run-summary.jsonl is JSON Lines, but this parses the entire file as one JSON document. As soon as it contains more than one summary record, comparison generation throws even though the run is valid; run-all-evals.ps1 already handles this format by selecting the last non-empty line.

  const rows = fs.readFileSync(path.join(dir, 'results.jsonl'), 'utf8')
    .split('\n').filter(l => l.trim())
    .map(l => JSON.parse(l));
  const summary = JSON.parse(fs.readFileSync(path.join(dir, 'run-summary.jsonl'), 'utf8').trim());
  return { rows, summary, variantName };

tests/scenarios/azure-eventhub-rust/acceptance-criteria.md:116

• This “correct” example now imports azure_core solely to force a trait-object annotation, while the same acceptance criteria remove azure_core at line 15 and the canonical skill says the returned Arc<DeveloperToolsCredential> should be passed directly (SKILL.md:31,97-103). Following both instructions therefore yields an undeclared-crate build failure. Keep the direct credential binding and remove these extra imports.
  tests/scenarios/azure-eventhub-rust/scenarios.yaml:85
• The mock response introduces a direct azure_core dependency only for an unnecessary trait-object annotation. This contradicts the updated skill, which explicitly passes DeveloperToolsCredential::new(None)? directly, and makes the mock incomplete under the documented dependency set. Use the direct credential binding here as well.
  .github/plugins/azure-sdk-rust/skills/azure-eventhub-rust/SKILL.md:31
• The PR description explicitly assigns *-rust scenarios to #381 and says this split excludes them, but this change modifies Rust skill guidance while the PR also changes Rust acceptance criteria and scenarios. Move these Rust edits to #381 (or update the split scope) to avoid overlapping ownership and conflicting changes between the two PRs.

> `DeveloperToolsCredential::new(None)?` already returns an `Arc<DeveloperToolsCredential>`, so you can pass or clone it directly into `.open()`. Add `azure_core` only when you need direct `azure_core` imports such as `ErrorKind`.

[Copilot]

Pull request overview

Copilot reviewed 150 out of 153 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (4)

tests/pnpm-workspace.yaml:5

• allowBuilds entries must be booleans. This placeholder string makes the pnpm 11 workspace policy invalid and prevents the new pnpm install --frozen-lockfile preflight from completing; better-sqlite3 also needs its native install script enabled. Replace it with true.

  better-sqlite3: set this to true or false

tests/run-skill-experiments.ps1:1079

• The script validates the package-local Vally executable above, but then invokes the bare global vally command here; $vallyCmd is never used. A normal pnpm install does not put that binary on the parent shell's PATH (and Start-Job does not fix that), so the documented experiment runner fails unless users separately install Vally globally. Invoke the validated package-local binary instead.

            $output = & vally experiment run skill_effectiveness_experiment.yaml --output-dir $skillOutputRoot 2>&1

tests/scenarios/azure-eventhub-rust/acceptance-criteria.md:184

• This producer example contradicts the signature documented by the updated skill: ProducerClient::builder().open(...) takes the hub name as &str, while only the consumer takes an owned String (.github/plugins/azure-sdk-rust/skills/azure-eventhub-rust/SKILL.md:153-155). Calling to_string() here therefore makes the “correct” example fail type checking.
  tests/scenarios/azure-eventhub-rust/scenarios.yaml:81
• The PR description explicitly says *-rust scenarios are excluded and handled by #381, but this PR still changes Rust Event Hubs scenarios (and Rust Blob evals). Keeping these edits in both split PRs creates overlapping ownership and likely merge conflicts; remove the Rust changes from this PR or correct the declared split scope.

[Copilot]

Pull request overview

Copilot reviewed 177 out of 180 changed files in this pull request and generated 5 comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file

Comments suppressed due to low confidence (5)

tests/pnpm-workspace.yaml:5

• allowBuilds values must be booleans. This placeholder string makes the pnpm workspace configuration invalid, so the new mandatory pnpm install --frozen-lockfile preflight cannot succeed. better-sqlite3 is a native dependency of Vally and should be allowed to build.

  better-sqlite3: set this to true or false

tests/run-skill-experiments.ps1:1079

• The script validates that the package-local Vally executable exists and even constructs $vallyCmd, but this invocation ignores both and resolves only a global vally command. A normal local install therefore fails here, and background jobs are especially unlikely to have tests/node_modules/.bin on PATH. Pass the local executable or a pnpm --dir ... exec vally invocation into the worker script block.

            $output = & vally experiment run skill_effectiveness_experiment.yaml --output-dir $skillOutputRoot 2>&1

tests/compare-experiment.mjs:298

• The verdict can declare SKILL IS BETTER even when the skill variant scores worse, as long as both variants clear the threshold and the skill uses fewer tokens/time. That is misleading for a skill-effectiveness report. Require a non-regressing score (or explicitly incorporate the score delta) before emitting the positive verdict.

  const skillBetter = sS.totalTokens < bS.totalTokens && sS.wallTimeSec < bS.wallTimeSec;
  const msg = skillBetter
    ? `✅  Skill variant passes with ${tokenDelta} fewer tokens and ${timeDelta} less wall time → SKILL IS BETTER`
    : `⚠️  Skill variant does not uniformly reduce both tokens and time — review per-metric deltas above.`;

tests/scenarios/azure-eventhub-rust/vally/eval.yaml:168

• This command no longer enforces the rubric's “without warnings” requirement: plain cargo clippy exits successfully when it emits warnings. Restore -- -D warnings so the post-execution grader rejects lint warnings as the prompt and updated skill guidance require.
  tests/scenarios/README.md:39
• The new idiomatic grader executes python -m pylint, but the documented one-time setup installs only pnpm dependencies. Following these instructions locally leaves pylint unavailable and makes every Python idiomatic grader fail. Add installation of tests/requirements.txt to the setup steps.

3. Install test dependencies:
   pnpm --dir tests install --frozen-lockfile

4. Approve required build scripts (if prompted):
   pnpm --dir tests approve-builds

[Copilot]

Pull request overview

Copilot reviewed 177 out of 180 changed files in this pull request and generated no new comments.

Files not reviewed (1)

• tests/pnpm-lock.yaml: Generated file