Skip to content

Add CI pipeline for SDK skill-efficacy evaluation#222

Merged
TheovanKraay merged 32 commits into
AzureCosmosDB:mainfrom
yumnahussain:yumnahussain/ideal-succotash
Jul 21, 2026
Merged

Add CI pipeline for SDK skill-efficacy evaluation#222
TheovanKraay merged 32 commits into
AzureCosmosDB:mainfrom
yumnahussain:yumnahussain/ideal-succotash

Conversation

@yumnahussain

@yumnahussain yumnahussain commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a manually-triggered GitHub Actions workflow and supporting scripts to evaluate the cosmosdb-best-practices skill set using an internal skill-efficacy evaluation harness. The pipeline measures whether the skills help an agent produce Cosmos DB SDK best-practice-compliant code, grading the generated app both behaviorally (drive its HTTP API + independently read the data store) and via static source checks.

Current scope

Scoped to a single Harbor task instance (mosaic-python) while the end-to-end pipeline is validated. The harness (shared base image, remote-backend runner, verifier, contracts) already supports additional scenarios × SDKs, which will be added in follow-up PRs.

What's included

Area Files Purpose
CI trigger .github/workflows/msbench-eval.yaml workflow_dispatch job that signs in as the triggering user via device code, installs msbench-cli, runs the eval, and opens an issue on failure
Benchmark publish .github/workflows/publish-benchmark.yaml Builds/publishes task images via harbor-format-curation
Orchestration scripts/msbench-eval.py Submits msbench-cli run --repeat N, runs a remote-backend submit preflight, polls, reads harbor eval.json, computes pass@k, checks threshold
Issue creation scripts/create-skills-issue.py Maps failing test classes to sdk-* rules, opens a tracking issue
Benchmark task + harness benchmarks/cosmos-sdk-skills/** The mosaic-python task, shared remote-backend runner, and pytest verifier that independently reads the store to confirm real persistence

Authentication (changed)

The pipeline no longer uses an Azure OIDC service principal. Instead it authenticates to Azure / the remote execution backend as the person who dispatches the run, via an interactive device-code sign-in — the same identity used for a local eval. The job prints a URL + one-time code to its logs; the dispatcher completes the browser sign-in, and all subsequent az / msbench-cli calls run as that account (which is what authorizes the backend submission and the Azure Artifacts feed read).

  • New optional tenant_id workflow input (blank = az default tenant).
  • A fully-unattended service-principal path is tracked separately in the service-principal setup issue; it needs extra permissions granted to the SP first.

Live Cosmos account (optional)

An optional COSMOS connection-string secret. When set, msbench-eval.py forwards it (encrypted) so the verifier grades against a real Cosmos DB account instead of the bundled emulator. Leave it unset to grade against the emulator. This enables checks that a single-node emulator can't prove (e.g. multi-region routing).

Verifier: Python client-config check

check_source.py adds TestClientConfigViaKwargs — a Python-gated static check that flags the legacy documents.ConnectionPolicy(...) construction / attribute-mutation pattern (e.g. policy.RequestTimeout = ...) and steers toward passing config as keyword arguments to CosmosClient(...). The skill rule this test cites (sdk-python-client-kwargs) lives in companion PR #225 (one-rule-per-PR); the test references the rule id as a string, so the two PRs are independent and can merge in either order.

How it works

  1. Trigger via Actions → SDK Skill-Efficacy Evaluation and complete the device-code sign-in in the job logs.
  2. A remote-backend submit preflight confirms the backend accepts a submission and returns a run_id before the real run.
  3. Runs the task instance(s) with --repeat independent attempts (default 3).
  4. Computes per-instance pass rate from the harbor eval.json.
  5. If any instance falls below the threshold (default 90%), opens a GitHub issue mapping failures to specific sdk-* rules with suggested skill improvements.

Inputs

  • benchmark — benchmark name (default cosmos-sdk-skills)
  • repeat — independent attempts per instance (default 3)
  • threshold — pass-rate threshold 0–1 (default 0.9, tentative)
  • model — overrides the in-container @github/copilot CLI (blank = runner default claude-sonnet-4.6); forwarded as COPILOT_MODEL, while the eval CLI's own --model stays on a valid plugin model-map key
  • tenant_id — Azure tenant to sign into (blank = az default)

Prerequisites

  • The dispatcher has an Azure account entitled to submit to the remote execution backend and read access to the internal Azure Artifacts feed, and completes the device-code prompt within the timeout.
  • Repo variables for the internal identifiers kept out of the public repo: MSBENCH_FEED, MSBENCH_TOKEN_RESOURCE, CES_ACR, MSBENCH_BACKEND (Settings → Secrets and variables → Actions → Variables).
  • COPILOT_GITHUB_TOKEN secret — a github.com token for an account with a Copilot subscription (the default Actions token has no Copilot access); forwarded into the remote-backend container via --encrypted-env GITHUB_TOKEN.
  • (Optional) COSMOS secret — a Cosmos DB connection string to grade against a live account instead of the emulator.
  • If the tenant's Conditional Access blocks device-code sign-in, use a self-hosted runner where you are already az logined (or the SP path).

Companion benchmark repo

The Harbor tasks and verifier were originally authored in a separate registration repo (cosmos-sdk-skills-bench). The in-scope task + verifier are vendored into benchmarks/cosmos-sdk-skills/ here so CI can build and grade directly.

Tentative decisions (feedback welcome)

  • 90% pass threshold
  • Device-code (per-dispatcher) auth as the default vs. a service principal
  • Vendoring the benchmark task/verifier into this repo vs. keeping it external

Adds a GitHub Actions workflow and supporting scripts to evaluate
the cosmosdb-sdk skill set using MSBench. The pipeline:

- Runs 15 Harbor task instances (3 scenarios × 5 SDKs) with 3 repeats
- Uses pass@k metrics to measure skill reliability
- Creates GitHub issues mapping failures to specific sdk-* rules

New files:
- .github/workflows/msbench-eval.yaml (workflow_dispatch trigger)
- scripts/msbench-eval.py (orchestration: submit, poll, report)
- scripts/create-skills-issue.py (failure → rule mapping → issue)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 25, 2026 21:40
@yumnahussain
yumnahussain marked this pull request as draft June 25, 2026 21:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a manually-triggered GitHub Actions pipeline plus helper scripts to run MSBench-based evaluations for Cosmos DB SDK skill guidance, generate a merged pass@k report, and (optionally) open a tracking issue when results fall below a threshold.

Changes:

  • Adds a workflow_dispatch GitHub Actions workflow to authenticate via Azure OIDC, install msbench-cli, run the evaluation, and upload results.json.
  • Adds scripts/msbench-eval.py to orchestrate MSBench runs, poll for completion, generate a merged report, and evaluate per-instance pass rates.
  • Adds scripts/create-skills-issue.py to parse results, map failing tests to sdk-* rules, and create a GitHub issue via gh issue create.

Reviewed changes

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

File Description
.github/workflows/msbench-eval.yaml New manual CI workflow to run MSBench evaluation, optionally create an issue, and upload artifacts.
scripts/msbench-eval.py New orchestrator script for running MSBench, merging reports, parsing pass rates, and flagging failing instances.
scripts/create-skills-issue.py New issue-creation script to summarize failures and map them to rule files.

Comment thread .github/workflows/msbench-eval.yaml Outdated
Comment thread scripts/msbench-eval.py
Comment thread scripts/msbench-eval.py
Comment thread scripts/create-skills-issue.py
Comment thread .github/workflows/msbench-eval.yaml Outdated
yumnahussain and others added 19 commits June 25, 2026 14:45
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Use masked env var for pip token instead of persisting in pip config
- Guard issue creation step on results.json existence
- Fix normalize_rate to correctly handle '0.9%' as 0.009 not 0.9
- Remove duplicate issue creation from msbench-eval.py (workflow handles it)
- Make labels optional in create-skills-issue.py with fallback

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Moves the cosmos-sdk-skills benchmark into benchmarks/cosmos-sdk-skills/:
- 15 Harbor tasks (3 scenarios × 5 SDKs): mosaic, ticketing, IoT
- Shared verifier with 5 check files (pytest-based SDK best practice tests)
- runner.sh entrypoint (emulator startup, app build/run, pytest execution)
- runner-with-skills.sh (custom runner that loads cosmosdb-sdk skill)
- dataset.jsonl (15 MSBench instances)
- publish-benchmark.yaml workflow (auto-rebuild on benchmarks/ changes)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The actual tool for building/publishing benchmark images is
harbor-format-curation, not msbench-cli. Updated the workflow to use
the correct commands: import, build, update-database, publish.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The verifier graded best-practice rules mostly by regex-scanning agent
source (check_source.py / check_advanced_source.py). Per the MSBench
lessons doc, those static checks are gameable and prove nothing about
runtime behavior. This adds a behavioral grader driven by the live app
plus independent emulator reads, for the mosaic scenario (all 5 SDKs).

- check_behavior.py (NEW, wired into runner.sh): builds/runs the app,
  drives the HTTP API, then re-reads the Cosmos emulator with the
  verifier's own client and asserts they agree: persistence is real (no
  in-memory/SQLite fakes), API read matches the stored doc, duplicate
  POST -> 409 with exactly one persisted doc, partition-key value == user
  id with a working point read, and city filter == emulator query set.
- conftest.py: independent-emulator-read helpers/fixtures
  (emulator_docs_for_id, persisted_docs, users_partition_key_field).
- check_source.py: honest docstring — these remain STATIC client-config
  signals the single-node emulator cannot prove (retry, Direct mode,
  preferred regions, diagnostics, singleton), kept for A/B coverage.
- 5x mosaic instruction.md: grading section now states live behavior is
  primary and symmetric across arms.
- README.md + docs/benchmark-coverage.md: document the behavioral layer.

Note: emulator not runnable on Windows host; re-validate gold in WSL and
regenerate the dataset prompt (instruction.md changed).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Validated the behavioral verifier end-to-end by running the mosaic-python
gold reference against the vNext Cosmos emulator. The run surfaced two real
reference bugs that had never been exercised, plus a gold/agent asymmetry
that prevented fix_validation from ever reaching reward=1:

- reference/requirements.txt: add email-validator (app.py uses pydantic
  EmailStr, so the service failed to boot without it).
- reference/app.py: drop enable_cross_partition_query from the async
  query_items call. The azure-cosmos v4 async SDK removed that kwarg
  (cross-partition is automatic); passing it raised TypeError -> 500 on
  GET /users?city=. Caught by both check_api and the new check_behavior.
- check_skills.py TestAgentReadSkills: skip (instead of fail) when no
  agent transcript exists. Gold/fix_validation runs have no Copilot agent,
  so the transcript is never written; a present-but-empty transcript still
  fails (real infra signal).
- .gitignore: ignore the locally staged skills build-context artifact.

Gold now passes: 41 passed, 3 skipped, reward=1.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The vnext emulator's gateway serves static endpoints (/, emulator.pem) before
the pgcosmos data-plane extension finishes initializing. The old readiness probe
only checked that :8081 answered, so runner.sh proceeded while real Cosmos calls
still returned HTTP 503 'pgcosmos extension is still starting'. SDK clients that
connect eagerly at process start (dotnet/java/nodejs/go) crashed at boot; the
python reference only survived because the emulator was already warm.

Add a dataplane_ready() check that issues a signed GET /dbs and a wait_dataplane()
loop that blocks until the data plane answers with a non-5xx status. Both the
already-running fast path and the post-launch path now wait on it. Validated:
mosaic-nodejs gold goes from reward=0 to reward=1, and the other SDKs now reach
their apps instead of dying on the startup 503.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tives

All five mosaic SDK gold references now pass local emulator gold runs
(reward=1): python, nodejs, dotnet, java, go.

Reference fixes (emulator integration; production literals preserved so
check_source static rules still pass):
- dotnet: use ConnectionMode.Gateway when the endpoint is the local
  emulator (emulator has no Direct/rntbd backend), keep Direct for real
  accounts.
- java: break a Spring bean cycle by moving the CosmosClient @bean into a
  nested @configuration class; use gatewayMode + endpointDiscoveryEnabled(
  false) and drop preferredRegions for the emulator (single-region
  emulator advertises no regional endpoints, so discovery tried to resolve
  localhost-WestUS2). The Java v4 SDK always uses TLS for the gateway, so
  this instance runs the emulator in https mode and trusts its self-signed
  root CA in run.sh.
- go: azcosmos v1.2.0 has no NewCrossPartitionQueryItemsPager; use
  NewQueryItemsPager with an empty PartitionKey for the cross-partition
  query. Add the missing go.sum (go mod tidy).

Harness fix:
- start-emulator.sh: the data-plane readiness probe now skips TLS
  verification when COSMOS_PROTOCOL=https (self-signed emulator cert).

Verifier false-positive fixes (baked into base image):
- conftest.py: skip the dotnet build output dir (out/) when scanning
  source, and strip XML comments (<!-- -->) so a commented-out package id
  in a .csproj no longer trips package checks.
- check_source.py: the "no Azure.Cosmos preview package" check no longer
  matches inside the legit Microsoft.Azure.Cosmos id (negative lookbehind).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rewrite the 'How you will be graded' section of all 15 task
instruction.md files so the agent gets no mention or hint of skills,
an agent skill set, or Cosmos DB best-practice criteria. Remove the
enumerated best-practice categories (retry, preferred regions, Direct
mode, diagnostics, client reuse, indexing policy, throughput) and the
mosaic-go transparency rubric. Grading is now described purely
functionally: a verifier drives the HTTP API and independently reads
the emulator to confirm endpoints behave per the API contract and
returned data matches stored data.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The instruction scrub removed all agent-facing hints about a skill set,
so the Go/Node.js 'missing-guidance transparency' rubric is now
undiscoverable (an agent can't know to write 'borrowed from ...'). The
skills-read gate (did the agent open SKILL.md / a rule file) is also out
of scope for this benchmark. Remove both:

- check_skills.py: drop TestGoMissingGuidanceTransparency,
  TestNoOverstatedCertainty, TestNodejsNoOverstatedCertainty, and
  TestAgentReadSkills plus their helpers/constants; unused imports
  removed; module retitled to 'Source anti-pattern checks'.
- conftest.py: drop the now-unused source_text_with_comments and
  docs_text fixtures.
- README.md / docs/benchmark-coverage.md: remove the transparency and
  skill-engagement descriptions, matrix rows, and legend so the docs
  match the graders that actually run.

The remaining source anti-pattern checks (hardcoded key, endpoint from
env, forbidden packages, sync-over-async blocking, async client) are
unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…eting + iot

Refactor the shared verifier (check_api/behavior/cosmos/source, conftest) to
drive all generic checks off declarative per-scenario contracts instead of the
hardcoded mosaic contract. Each root-entity sub-check (create/get/list,
persistence, duplicate rejection, partition key, indexing, throughput, document
shape) is toggled by the contract so mosaic, ticketing, and iot each get exactly
the checks they declare.

- shared/contracts/{mosaic,ticketing,iot}.json: declarative contracts baked
  into the base image (SCENARIO env selects the active one).
- conftest.py: loads the contract, exposes ROOTS/CHILDREN + shared helpers and
  fixtures (container resolution, emulator doc lookup, root seeding/persistence).
- check_source.py: provision-count bound is now contract-aware (1 database +
  one create-container per declared container) so multi-container scenarios pass.
- Per-scenario /tests/checks.py grade the child entities the generic engine does
  not model: ticket purchase/duplicate/ETag-cancel, reading ingest/time-range/
  device-summary. Copied to all 5 SDK dirs each.
- Python reference apps for ticketing (events+tickets, ETag cancel) and iot
  (devices+readings, time-range + summary), mirroring mosaic-python.
- Task Dockerfiles set per-scenario env; python tasks wire in the reference.

Gold-validated locally: all 5 mosaic SDKs, ticketing-python, and iot-python
report reward=1.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ends

The CES backend writes instance_id into /drop/metadata.json, but the local
Docker backend writes run-level metadata (instance_ids, plural) and passes the
id via the instanceId env var. entry.sh hard-read ['instance_id'] under set -e,
so on the local backend the python KeyError aborted the whole script before the
agent ran (gold runs never exercised this path — they call solve.sh/test.sh
directly). Prefer the instanceId env var, fall back to metadata.json, default
to 'unknown', mirroring parse.py's existing logic.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Convert the 6 structurally-complete tasks to the proven harbor-format-curation
layout so CES runs the baked verifier and produces real 0/1 scores (not "error").

Per task: git mv reference -> environment/reference; rewrite environment/Dockerfile
to pre-install which/zip/unzip via tdnf + dnf->tdnf shim (CES agent harness skips
installs for tools already present on Azure Linux), use relative COPY reference,
keep per-language toolchain installs, and drop harbor-baked tests/solution/
instruction/metadata COPY lines.

Pipeline reads authoritative scores from eval.json inside output.zip instead of
msbench-cli report (which mis-parses harbor eval.json). Regenerate all 15
metadata.json with the full harbor contract; add harbor-generated dataset.jsonl
and benchmark_loaders.toml.

Validated on CES: ticketing-python (run 28973765143) and mosaic-python
(run 28975682758) both graded end-to-end with 0 errors/0 eval-errors.

Deferred: iot-{dotnet,go,java,nodejs} and ticketing-{dotnet,go,java,nodejs}
need reference apps + language toolchains authored before they can grade.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Narrow the PR and weekly MSBench run to the single, CES-validated
mosaic-python task (run 28975682758 graded 0/1 with 0 errors).

- Remove the other 14 task dirs (mosaic-{dotnet,go,java,nodejs},
  ticketing-*, iot-*); their full work is preserved on branch
  yumnahussain/all-15-tasks-backup for later expansion.
- Reduce dataset.jsonl and msbench-registration/dataset.jsonl to the
  single mosaic-python row (pipeline reads the top-level dataset).
- Update README.md + docs/benchmark-coverage.md with a scope banner and
  corrected counts (1 task, Python-only); the shared verifier keeps its
  per-language checks (gated by SDK), so no shared/ changes are needed.

The workflow and scripts/msbench-eval.py are already task-count agnostic
(they iterate the dataset), so no logic changes are required.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Normalize pass-rate fractions before rendering, preserve aggregated repeat counts in generated issue analysis, forward selected Copilot models into CES runs, and remove unused issue creation helper.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…arsed status

msbench-cli run (without --no-wait) blocks to completion, so its output
already contains report/analysis prose that mis-parses harbor eval.json and
emits a bogus 'error' status token. The early terminal-failure guard scraped
that token and aborted with exit 2 before reaching the authoritative
build_results_from_eval grader -- so every completed real run crashed the
pipeline instead of writing results.json, checking the threshold, and filing
the skills issue.

- msbench-eval.py: don't hard-abort on a failure-looking status from the
  already-completed run output; only use status to decide whether to keep
  polling. Make wait_for_completion defer a scraped TERMINAL_FAILURE to
  eval.json grading (genuine failures still surface via the report fallback).
- create-skills-issue.py: map TestPythonAsyncClient -> sdk-python-async-deps
  (the real async verifier class that failed in a live run was unmapped).

Verified end-to-end against a real run (29036621569): 0/1 resolved now grades
as a clean threshold FAIL (exit 1 -> issue creation) instead of exit 2 crash.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The runner previously forced a custom-agent persona (--agent
cosmosdb-best-practices) whose routing table explicitly told the
agent-under-test to read the skill rules. That contaminated the
benchmark: it measured a hinted agent, not the skill's organic effect.

Replace the custom-agent staging with a plain skill install into
~/.copilot/skills/ (the personal skills directory @github/copilot
auto-discovers). The default agent now runs the task with no hint --
it only sees the skill's name+description and decides on its own
whether to consult it, exactly as a real user's setup would.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…runner

Parse a single COSMOS connection string (AccountEndpoint + AccountKey) into
COSMOS_ENDPOINT + COSMOS_KEY before live-mode detection, so passing
--encrypted-env COSMOS alone switches the grader off the bundled emulator and
onto a live account. Order-independent; base64 keys preserved.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 50 out of 51 changed files in this pull request and generated 5 comments.

Comment thread benchmarks/cosmos-sdk-skills/shared/ces/entry.sh Outdated
Comment thread scripts/msbench-eval.py
Comment thread scripts/msbench-eval.py
Comment thread scripts/msbench-eval.py
Comment thread scripts/create-skills-issue.py
yumnahussain and others added 4 commits July 9, 2026 15:50
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: yumnahussain <161499581+yumnahussain@users.noreply.github.com>
Forward live-account credentials (COSMOS connection string, or split
COSMOS_ENDPOINT/COSMOS_KEY) via --encrypted-env when present so the
verifier grades against a real Cosmos account instead of the bundled
emulator. When unset, the pipeline runs against the emulator as before.

Also fix an --encrypted-env argparse bug: msbench-cli's --encrypted-env
is nargs='+', so repeating the flag kept only the last value and silently
dropped the rest. Emit all names under one flag.

The workflow now surfaces an optional COSMOS secret to the run step.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Wire --no-wait through to msbench-cli so the driver submits and returns
immediately with the server-assigned run_id instead of blocking on the full
graded run, and assert that run_id in main() as the CES-acceptance proof.

Add .github/workflows/msbench-ces-preflight.yaml: a workflow_dispatch job that
logs in as the Azure service principal via OIDC, installs msbench-cli, and runs
the --no-wait submit. A returned run_id proves the SP authenticated to CES and
cleared MSBench entitlement; no run_id fails the job with the CLI error.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
New skill rule sdk-python-client-kwargs: in azure-cosmos v4 all CosmosClient
config is passed as keyword arguments; constructing documents.ConnectionPolicy
and mutating its attributes (e.g. policy.RequestTimeout = 10000) is a legacy
v3/pydocumentdb pattern the SDK replaces with _build_connection_policy().

New verifier test TestClientConfigViaKwargs in check_source.py (python-gated,
static) flags ConnectionPolicy() construction and legacy attribute assignments
(RequestTimeout/PreferredLocations/RetryOptions/...). Scoped to ConnectionPolicy
only so SSLConfiguration/ProxyConfiguration objects are not false-flagged.

Regenerated AGENTS.md (npm run build).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The new skill rule and its AGENTS.md regeneration now live in a separate PR
(AzureCosmosDB#225). This PR keeps only the verifier test
TestClientConfigViaKwargs in check_source.py, which cites the rule id.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@yumnahussain
yumnahussain force-pushed the yumnahussain/ideal-succotash branch from cb851df to 31522cb Compare July 14, 2026 19:46
Add a preflight job to msbench-eval.yaml that submits one --no-wait run as the
Azure service principal and asserts a CES run_id. The evaluate job now needs:
preflight and only runs when the preflight succeeded (or was skipped), so the
full ~16-min graded run never starts if the SP cannot submit to CES.

Add a skip_preflight workflow_dispatch input to bypass the gate once the SP
path is proven and avoid the extra throwaway submission.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@NaluTripician

Copy link
Copy Markdown

Review: cosmos-sdk-skills benchmark (Python slice)

Really nice work here — the architecture is sound and the behavioral core is the strongest part. Below is a consolidated review focused on the Python (mosaic-python) slice that ships and runs today. Grouped into strengths, things to fix, and suggested next steps. Non-Python SDKs and fault-injection are noted only as future directions.

What's strong (keep it)

  • The behavioral core is the right design and is hard to game. Driving the live API and then independently re-reading the emulator with the verifier's own client is exactly right — it catches apps that answer HTTP but never persist (in-memory/SQLite), proves duplicate-rejection by state rather than by scanning source for IfNoneMatch, and cross-checks the city filter against the emulator's own query result.
  • Contract-driven engine cleanly separates the generic checks from scenario specifics.
  • Honest labelingcheck_source.py openly documents that its regex rules are the weaker half and explains why they can't be behavioral on a single node. That's the right framing; the goal should be to keep shrinking that static set.
  • Isolated verifier venv + comment-stripping before regex are good anti-interference touches.

Things to fix (internal consistency — a benchmark that contradicts itself is hard to trust)

  1. check_advanced_source.py never runs. It isn't in runner.sh's pytest target list and isn't imported anywhere, so ~10 rule classes (availability strategy, circuit breaker, conditional-create ETag, ETag concurrency, excluded regions, patch-counter, async-api, enum serialization, per-call request options) are dead. Either wire it in or delete it and trim the coverage docs to what actually executes.
  2. Skill-name drift. The repo only has skills/cosmosdb-best-practices, but the README, runner-with-skills.sh, and create-skills-issue.py (PREFERRED_RULES_DIR = skills/cosmosdb-sdk/rules) reference a non-existent cosmosdb-sdk. Pick one name and make everything match.
  3. The documented runner isn't the one the pipeline uses. README describes runner-with-skills.sh (custom --agent, AGENTS.md injected — hinted); the actual pipeline (msbench-eval.pyshared/ces/runner.sh) installs the skill for the default agent with no hint (organic). These are different experiments — pick one as canonical and document it.
  4. Rule-ID drift. sdk-retry-429 (in reference/app.py and create-skills-issue.py's TEST_TO_RULE_MAP) vs sdk-retry-throttled (what check_source.py cites). Generated issues will name rules that don't exist; several map keys also point at the dead check_advanced_source.py tests.
  5. Port contract is muddled (8079 / 8080 / 9080). Base image bakes APP_PORT=9080 and instruction.md says 9080, but runner.sh/conftest.py/reference/run.sh default to 8080, LIVE_MODE forces python→8080, and start-emulator.sh assumes the app is on 8080. APP_PORT_PYTHON silently doesn't apply in emulator mode. Unify to one value with one source of truth.

Test-design notes

  • Binary all-or-nothing reward saturates. reward=1 requires every mandatory check to pass, so one missed static regex → 0 with no gradient — and a 0 can't distinguish "close" from "hopeless," or "skill helped" from "skill didn't." Graded/bucketed reward (already flagged as future work) is effectively required for usable signal.
  • Some mandatory checks are advanced/niche. Gating pass/fail on rules the task never exercises depresses pass rates for reasons unrelated to what's being measured. Separate "must behave correctly" from "nice-to-have config."
  • Only comments are stripped (not string literals), so tighten the loosest patterns to avoid a keyword-in-a-string satisfying a rule.

Suggested next steps (by impact)

  • A. Make the framework internally consistent (items 1–5). Prerequisite credibility work.
  • B. Add a skill-on / skill-off A/B — the core upgrade. A SKILL_ENABLED flag in ces/runner.sh (when 0, skip installing the skill), same dataset run both ways with --repeat N. This is what turns a test suite into a benchmark and measures the skill's effect rather than just pass rate.
  • C. Emit per-check results and report per-rule deltas. Add --junitxml to the pytest run and aggregate per-test pass/fail across repeats and across the A/B arms. Keep the binary reward for MSBench's contract; layer analytics on top. The story becomes "the skill moved singleton-client 40%→90%, diagnostics 10%→70%," etc.
  • D. Move toward a graded/bucketed reward (builds / behavioral / best-practices) so near-misses are visible and static-regex brittleness stops dominating the headline number.
  • E. Add an oracle smoke test in CI — run reference/ through the verifier and assert reward=1, so a future 0 is attributable to the agent, not a harness regression.
  • F. Separate infra failures from agent failures — detect "emulator never became ready" (pgcosmos warmup) and retry the instance instead of scoring it as an agent miss; otherwise flaky infra corrupts the A/B.
  • G. Harden static regexes against false negatives; H. stamp provenance (model id / skill SHA / image tag / dataset) into results.json and document one-command repro; I. keep promoting static rules to behavioral wherever they're observable on the emulator.
Future directions (out of the current Python scope — roadmap)
  • Fault injection to make today's static rules behavioral: a Cosmos-aware HTTP fault proxy between the app and the emulator can turn retry-on-429, transient recovery, end-to-end timeouts, and even sync-vs-async / provision-once resource behavior (CPU/RAM/threads under load) into observed behavior. Only the app should traverse the proxy; the verifier's independent read must stay unfaulted. This also produces a graded score for free.
  • Multi-region behaviors (hedging, preferred/excluded regions, availability strategy, cross-region circuit breaker) can't be proven on a single-node emulator — the existing runner.sh LIVE_MODE (COSMOS connection string) path is the natural home for a real multi-region arm.
  • Additional SDKs: the verifier is already language-gated. Rust (azure_data_cosmos) is the strategically interesting next target — gateway/HTTP (emulator-friendly), the skill already carries partial Rust guidance (incl. an sdk-emulator-ssl rule), and a Rust iteration already exists under testing-v2/. .NET/Java are emulator traps (Direct-mode + TLS) and should be deferred with that rationale.

Bottom line: the behavioral core is strong and worth building on. The two highest-value moves are (1) making the framework internally consistent and (2) adding a skill-on/off A/B with per-check reporting so it measures the skill's effect, not just whether agents pass.

yumnahussain and others added 2 commits July 14, 2026 15:04
Replace the service-principal OIDC login (and the SP preflight gate) with an
interactive 'az login --use-device-code' step so the manually-triggered eval runs
as whoever dispatches it, using their own MSBench entitlement. This matches how
MSBench authenticates a developer locally and needs no SP to be provisioned first.

The workflow stays workflow_dispatch-only (no schedule). Adds a tenant_id input and
a 60-minute timeout to bound the interactive sign-in. Removes the standalone SP
submit preflight workflow; SP setup for unattended runs is tracked in a separate issue.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Addresses PR review (things-to-fix 1-5 + test-design notes):
- Delete dead check_advanced_source.py (never in pytest target list)
- Delete hinted runner-with-skills.sh; document shared/ces/runner.sh as
  the canonical organic (un-hinted) agent runner in README
- Fix skill-name drift cosmosdb-sdk -> cosmosdb-best-practices
  (README, create-skills-issue.py PREFERRED_RULES_DIR)
- Fix rule-ID drift sdk-retry-throttled -> sdk-retry-429 (check_source.py,
  reference README) and prune TEST_TO_RULE_MAP keys for deleted tests
- Unify app port to 9080 across runner.sh, conftest.py, reference/run.sh;
  correct stale :8080 comments in start-emulator.sh
- Tighten loose static regexes to require code-adjacency (retry,
  preferred_locations) and document that string literals are not stripped

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@yumnahussain

Copy link
Copy Markdown
Contributor Author

Addressed the things to fix and test-design notes from the review in 0fc1c23.

Things to fix

  1. check_advanced_source.py never runs — deleted (it wasn't in the pytest target list nor imported anywhere; only the README referenced it). Removed from the README tree.
  2. Skill-name drift cosmosdb-sdkcosmosdb-best-practices — fixed in README.md and scripts/create-skills-issue.py (PREFERRED_RULES_DIR). The hinted runner-with-skills.sh (which used the wrong name and set COPILOT_CUSTOM_AGENT) is deleted.
  3. Documented runner ≠ pipeline runnershared/ces/runner.sh is now documented as the canonical runner. It installs the skill as a normally discoverable skill with no hint and drives the default agent, so the benchmark measures the skill's organic effect. README "CI Pipeline" section rewritten (runner + device-code auth + dropped the stale "29 rules" claim).
  4. Rule-ID drift sdk-retry-throttledsdk-retry-429 — fixed in check_source.py (all 4 language checks) and reference/README.md. Pruned the 10 TEST_TO_RULE_MAP keys that pointed at the now-deleted check_advanced_source tests.
  5. Port contract — unified the app port to 9080 (the value baked into the Dockerfile and used in instruction.md/dataset.jsonl) across shared/verifier/runner.sh, conftest.py, and reference/run.sh. Corrected the stale :8080-is-the-app-port comments in start-emulator.sh (behavior unchanged — the health-server cleanup stays as hygiene).

Test-design notes

  • Binary reward saturates → graded reward: agreed, but this changes the MSBench reward contract (/logs/verifier/reward.txt is binary), so I'm leaving it as deferred future work rather than folding it into this PR.
  • Separate must-behave from nice-to-have: deleting check_advanced_source.py removes the niche/unexercised static gates. The remaining split stands — check_behavior.py is the core behavioral suite; check_source.py/check_skills.py are the documented weaker static half.
  • Only comments are stripped, not string literals: tightened the two loosest Python regexes to require code-adjacency (retry_total\s*=|…, preferred_locations\s*=) and documented the constraint in the check_source.py docstring. I intentionally did not strip string literals wholesale — source_text also concatenates dependency manifests, and the SDK-presence checks (azure-cosmos, @azure/cosmos, Microsoft.Azure.Cosmos) legitimately match package IDs that live inside those manifest strings.

Validated: py_compile on the edited Python, bash -n on the edited shell scripts, and a repo-wide grep confirms no lingering cosmosdb-sdk / sdk-retry-throttled / check_advanced_source / runner-with-skills references.

@yumnahussain
yumnahussain marked this pull request as ready for review July 15, 2026 00:18
Move the internal Azure Artifacts feed path, AAD token resource GUID, CES
ACR name, and CES execution backend out of the committed workflows/scripts
and into GitHub repo variables so only placeholders appear in the public repo.
Mechanism is unchanged.

- msbench-eval.yaml / publish-benchmark.yaml: feed + token resource now read
  from vars.MSBENCH_FEED / vars.MSBENCH_TOKEN_RESOURCE (auth URL + trusted-host
  derived at runtime); ACR login uses vars.CES_ACR; backend passed via
  vars.MSBENCH_BACKEND env.
- publish-benchmark.yaml: [docker.prod] profile is materialized at publish time
  from vars.CES_ACR instead of hardcoding the registry in mosaic.toml.
- mosaic.toml: drop the committed [docker.prod] registry name; document CI
  injection + local override.
- msbench-eval.py: DEFAULT_BACKEND now reads MSBENCH_BACKEND env (empty default).
- README / benchmark_loaders.toml: genericize devdiv-microsoft/MicrosoftSweBench
  and msbench-benchmarks references.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@yumnahussain

Copy link
Copy Markdown
Contributor Author

Scrubbed the internal MSBench identifiers out of the public repo (feed path, AAD token resource GUID, CES ACR name, CES backend). Mechanism is unchanged — the values now come from GitHub repo variables (Settings → Secrets and variables → Actions → Variables), so only placeholders appear in the YAML/scripts.

Repo variables to create before running the pipelines:

Variable Value Used by
MSBENCH_FEED feed host+path, no scheme (<host>/_packaging/<feed>/pypi/simple/) both workflows (pip index URL + trusted-host derived at runtime)
MSBENCH_TOKEN_RESOURCE AAD resource id for the feed token both workflows
CES_ACR ACR name (the <name> in az acr login --name) publish-benchmark.yaml (ACR login + injected [docker.prod])
MSBENCH_BACKEND CES execution backend msbench-eval.yamlmsbench-eval.py

Notes:

  • These are variables, not secrets — none are credentials (the feed token is minted at runtime and masked). Repo variables aren't part of the source tree, so nothing internal is exposed publicly.
  • mosaic.toml no longer commits the [docker.prod] registry; publish-benchmark.yaml appends that profile at publish time from CES_ACR. For a local prod build, add a matching [docker.prod] section yourself (documented in mosaic.toml).
  • Verified: both workflows YAML-parse, msbench-eval.py compiles, both TOMLs parse, the CI-injected prod profile parses to the correct registry, and a repo-wide grep finds zero remaining internal identifiers.

Replace the internal tool name (MSBench) and remote-service name (CES)
in human-readable prose (comments, docstrings, help text, README,
workflow display name) with generic 'skill-efficacy evaluation' /
'remote execution backend' wording. Functional tokens (msbench-cli,
filenames, cache paths, image tags, labels) are intentionally retained.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@yumnahussain yumnahussain changed the title Add MSBench CI pipeline for SDK skills evaluation Add CI pipeline for SDK skill-efficacy evaluation Jul 16, 2026
Print the run-analysis URL (https://msbenchapp.azurewebsites.net/run-analysis/<run_id>)
as soon as the run_id is known, again after the summary, and in the --no-wait
preflight path so the run is viewable regardless of pass/fail. Also record
run_id and analysis_url in the results.json payload.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@TheovanKraay TheovanKraay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, just a couple of things...

publish-benchmark.yaml runs on push to main under benchmarks/**, so merging this will trigger it immediately (and on every benchmarks change after). It needs CES_ACR and the Azure OIDC secrets to publish, and those aren't set up in this repo yet, so it'll fail every time, harmlessly but leaving a red run on main. Can we switch it to workflow_dispatch only for now so it just runs when we manually kick it off? Once the registry and OIDC are configured we can add the auto-trigger back. (Or leave the trigger and make the missing-CES_ACR check exit 0 and skip instead of failing, same result.).

Also msbench-eval.py still hardcodes the internal msbenchapp.azurewebsites.net URL, move that to a repo var like the others.

And a question on the eval, was leaving out the skill-on/off comparison intentional? Right now the pipeline only runs the skill-on path, so we're measuring compliance rather than efficacy and there's no delta to point at. I can see runner-no-agent.sh is already there as a baseline but not wired in, so was that planned for a follow-up, or worth adding here?

@sajeetharan sajeetharan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. I agree with Theo’s comments and don’t have any additional concerns beyond those, the rest would just be nits (just added them in lines)

Comment thread .github/workflows/msbench-eval.yaml Outdated
Comment thread .github/workflows/publish-benchmark.yaml Outdated
…ning

- Wire in the skill-off baseline (runner-no-agent.sh) via --baseline so the
  pipeline reports the skill-on vs skill-off pass-rate delta (efficacy), not just
  compliance. Adds a comparison table + results-comparison.json artifact. Gated
  behind the compare_baseline workflow input (default true); timeout doubled.
- Move the run-analysis URL out of the script into the MSBENCH_RUN_ANALYSIS_URL
  repo variable, matching MSBENCH_BACKEND/MSBENCH_FEED. Link is omitted when unset.
- publish-benchmark.yaml: manual (workflow_dispatch) trigger only until CES_ACR +
  Azure OIDC are configured, so it no longer fails on every push to main.
- Drop unnecessary pip --trusted-host from both workflows (Azure Artifacts is HTTPS
  with valid certs; the flag only weakened TLS verification).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@yumnahussain

Copy link
Copy Markdown
Contributor Author

Thanks Theo, good calls — all three are in the latest push.

Flipped publish-benchmark.yaml to manual-only (workflow_dispatch) so it stops red-failing on main. Left the push trigger commented out with a note so we can switch it back on once CES_ACR and OIDC are sorted.

Pulled the hardcoded URL out into a MSBENCH_RUN_ANALYSIS_URL repo var, same as MSBENCH_BACKEND — already set it on the repo. If it's ever unset the link just doesn't show up, nothing breaks.

And yeah, you're right about the skill-on/off thing — I'd assumed someone else had that experiment, but it really does belong here. So I wired runner-no-agent.sh in as a --baseline pass. It now runs both and prints a skill-off vs skill-on vs delta table plus a results-comparison.json. Put it behind a compare_baseline toggle (on by default) since it doubles the runtime, and kept the threshold gate on the skill-on run.

@TheovanKraay
TheovanKraay self-requested a review July 21, 2026 12:42
@TheovanKraay
TheovanKraay merged commit 9a157ed into AzureCosmosDB:main Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants