Skip to content

Fix over-blunt async verifier checks + run skill-efficacy eval on PRs via OIDC#240

Open
yumnahussain wants to merge 3 commits into
AzureCosmosDB:mainfrom
yumnahussain:remove-async-client-verifier-check
Open

Fix over-blunt async verifier checks + run skill-efficacy eval on PRs via OIDC#240
yumnahussain wants to merge 3 commits into
AzureCosmosDB:mainfrom
yumnahussain:remove-async-client-verifier-check

Conversation

@yumnahussain

@yumnahussain yumnahussain commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

This PR bundles two related verifier fixes with the CI-auth change that lets the skill-efficacy eval run on PRs.

1. Verifier: stop over-flagging sync Cosmos clients in async apps

benchmarks/cosmos-sdk-skills/shared/verifier/check_skills.py

Removed TestPythonAsyncClient.test_async_client_when_async_framework_present. It statically failed any Python app that paired an async web framework (FastAPI/Quart/Sanic/Starlette/async Azure Functions/LangGraph) with the sync azure.cosmos.CosmosClient. That over-flags:

  • The app works. FastAPI runs sync def handlers in a threadpool, so a sync client there does not block the event loop. In 5 mosaic-python runs the apps ran fine — 37/38 tests passed, with only this static check failing.
  • It only greps for azure.cosmos.aio. It cannot distinguish a genuinely bad pattern (sync client inside an async def handler) from a fine one (sync client inside a sync def handler).
  • It misattributes the failure. The assertion cites rule sdk-python-async-deps, but that rule only governs pinning aiohttp when the aio client is used — not client selection. No documented rule mandates the aio client purely from framework presence.

Fixed the sibling test_at_least_one_async_handler, which had the same flaw — it fired on async-framework detection alone, so a FastAPI app that intentionally used the sync client with def handlers still failed. It now skips unless azure.cosmos.aio is actually imported, and the message no longer implies the async client is in use when it isn't. Dropped the now-unused _PY_ASYNC_FRAMEWORKS marker list.

Retained test_aiohttp_dependency_pinned (maps to the actual documented rule) and updated the class docstring to match what remains.

Client-selection under concurrency is a real scale/quality concern, but it's better measured with an at-scale experiment than gated as a reward=0 correctness failure on a single-request functional run.

2. CI: run the eval on PRs via OIDC managed identity

.github/workflows/msbench-eval.yaml

  • Unattended auth. Replaces the interactive device-code az login with GitHub OIDC federated login to a user-assigned managed identity (azure/login@v2 + id-token: write), using the AZURE_CLIENT_ID / AZURE_TENANT_ID / AZURE_SUBSCRIPTION_ID repo secrets. No client secret is stored.
  • Label-gated trigger. Adds pull_request_target: types: [labeled] so a maintainer can run the eval on any PR (including forks) by adding the run-msbench label. The maintainer's review of the diff is the security gate before secrets/OIDC are exposed; the job checks out the PR head with persist-credentials: false.
  • Adds a Resolve-parameters step (PR runs have no workflow_dispatch inputs, so it supplies defaults — baseline on), a per-PR concurrency group, and drops the now-unused tenant_id input.

Operational notes for reviewers/maintainers

  • Because pull_request_target always uses the workflow from the default branch, this takes effect only after merge — it won't run on this PR itself.
  • The run-msbench label must exist in the repo before it can be applied.
  • The managed identity's federated credential subject must match a pull_request_target run (base branch), i.e. repo:AzureCosmosDB/cosmosdb-agent-kit:ref:refs/heads/mainnot :pull_request.

python -m py_compile passes; the workflow YAML parses.

Remove TestPythonAsyncClient.test_async_client_when_async_framework_present
from the cosmos-sdk-skills verifier.

The check statically failed any Python app that paired an async web
framework (FastAPI/Quart/etc.) with the sync azure.cosmos.CosmosClient.
This over-flags: FastAPI runs sync `def` handlers in a threadpool, so a
sync client there does not block the event loop and the app works
correctly (runs passed 37/38 with only this check failing). It also
misattributes the failure to rule sdk-python-async-deps, which only
governs pinning aiohttp when the aio client is used -- not client
selection. No documented rule actually mandates the aio client purely
from framework presence, so the check tested for guidance that is not in
the skill. Client-selection is better evaluated as a scale/quality
signal than a reward-gating correctness failure.

The sibling checks (async handler present, aiohttp pinned when aio client
used) are retained.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings July 22, 2026 13: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 adjusts the cosmos-sdk-skills verifier for Python apps by removing a static check that required azure.cosmos.aio whenever an async web framework was detected, reducing false failures for apps that correctly use the sync CosmosClient behind sync handlers.

Changes:

  • Removed TestPythonAsyncClient.test_async_client_when_async_framework_present from the verifier.
  • Updated the TestPythonAsyncClient class docstring to reflect the remaining checks.

Comment thread benchmarks/cosmos-sdk-skills/shared/verifier/check_skills.py Outdated
yumnahussain and others added 2 commits July 23, 2026 16:19
Replace the interactive device-code az login with unattended GitHub OIDC
federated login to a user-assigned managed identity (azure/login@v2 +
id-token: write), using the AZURE_CLIENT_ID / AZURE_TENANT_ID /
AZURE_SUBSCRIPTION_ID repo secrets. No client secret is stored.

Add a label-gated pull_request_target trigger so maintainers can run the
eval on any PR (including forks) by adding the 'run-msbench' label — the
maintainer's review of the diff is the security gate before secrets/OIDC
are exposed. The job checks out the PR head (with persist-credentials:false)
so the eval tests the PR's actual skill/benchmark changes, and a new
Resolve-parameters step supplies defaults (baseline on) since PR runs have
no workflow_dispatch inputs. Adds a per-PR concurrency group and drops the
now-unused tenant_id input.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
test_at_least_one_async_handler fired on async-framework detection alone,
so a FastAPI/Starlette app that intentionally uses the sync CosmosClient
with plain def handlers still failed -- the same over-blunt false positive
the removed check was addressing. Gate it on azure.cosmos.aio actually
being imported, and reword the message so it no longer implies the async
client is in use when it may not be. Drop the now-unused
_PY_ASYNC_FRAMEWORKS marker list.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@yumnahussain yumnahussain changed the title Remove over-blunt async-client-selection verifier check Fix over-blunt async verifier checks + run skill-efficacy eval on PRs via OIDC Jul 24, 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.

2 participants