Repository navigation
Use GitHub MCP for CI failure searches - #132619
vitek-karas wants to merge 6 commits into
Conversation
Make scanner retrieval failures visible and fail closed, and validate the production GitHub MCP search path in Vally evals. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 51814c28-9cf0-4574-83d5-0d3e31506cc9
|
Azure Pipelines: Successfully started running 1 pipeline(s). 15 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 15 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR migrates CI failure searches to read-only GitHub MCP tools, improves fail-closed retrieval handling, and supplies MCP credentials to Vally evaluations.
Changes:
- Adds scoped MCP search requirements and stronger shell-read protections.
- Adds temporary MCP environment-file setup and cleanup.
- Updates evaluation specifications, workflow metadata, and shared KBE guidance.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Summary |
|---|---|
.github/workflows/shared/create-kbe.instructions.md |
Tightens open-KBE search qualifiers. |
.github/workflows/evals/README.md |
Documents MCP-backed evaluation setup. |
.github/workflows/evals/ci-failure-scan.eval.yaml |
Adds MCP authentication and search graders. |
.github/workflows/evals/ci-failure-scan-feedback.eval.yaml |
Adds MCP authentication plumbing. |
.github/workflows/evals/ci-failure-fix.eval.yaml |
Adds MCP authentication plumbing. |
.github/workflows/ci-failure-scan.md |
Requires MCP lookups and fail-closed retrieval. |
.github/workflows/ci-failure-scan.lock.yml |
Regenerates workflow metadata and shell permissions. |
.github/workflows/ci-eval.yml |
Creates and cleans up the MCP environment file. |
Suppressed comments (3)
.github/workflows/ci-failure-scan.md:93
- Rule 13 defines
data retrieval failedonly as a per-signature skip, but a failed build-list or timeline fetch can occur before any signature exists. In that case there is no instructed tally entry, so Step 6 can omit the pipeline and make the failure invisible—the exact failure mode this change is intended to prevent. Add an explicit pipeline-level skip and final-summary requirement for pre-signature retrieval failures.
[!NOTE] This review comment was created by GitHub Copilot.
13. **A failed command or tool call is not an empty result.** For shell pipelines, enable `pipefail`, use failure-reporting flags such as `curl -fsS`, and never suppress stderr. Distinguish an explicit error from a successful zero-result response; an expected `grep` no-match used as a predicate is not a retrieval failure. If a required fetch, search, or parse fails, correct the command and retry; if it still cannot produce a verified result, stop processing that signature and record `skipped: data retrieval failed: <source>`. Never conclude that no matching data exists from empty, missing, malformed, or truncated output.
.github/workflows/ci-failure-scan.md:401
- The new rule says query-string URLs must be pre-bound, but the trusted scanner eval still gives the agent an inline
curl -fsSLURL at.github/workflows/evals/ci-failure-scan.eval.yaml:49. Because the eval supplies both prompts, this is contradictory and can trigger the tool-approver failure that the new rule forbids. Update the eval stimulus to use the variable form and the required%24topencoding.
[!NOTE] This review comment was created by GitHub Copilot.
- **Pre-bind every non-GitHub URL to a shell variable on its own line, then use `curl -fsS "$url"`.** Inline URLs with `?` or `&` are rejected as "Permission denied" even single-quoted (the tool-approver treats query strings as interactive prompts). Working pattern:
```bash
url='https://dev.azure.com/dnceng-public/public/_apis/build/builds?definitions=154&branchName=refs/heads/main&statusFilter=completed&resultFilter=succeeded,failed,partiallySucceeded&%24top=25&api-version=7.1'
set -o pipefail
.github/workflows/evals/ci-failure-scan.eval.yaml:118
- This grader only requires two
github-search_issuescalls. The shared KBE lookup flow also requires the scanner to search existing PRs and verify candidate issue/PR records, so a run that omitssearch_pull_requestsand the corresponding reads can still pass. Add required scoped MCP PR-search/read evidence, or narrow the eval's claimed coverage.
[!NOTE] This review comment was created by GitHub Copilot.
args:
owner: '^dotnet$'
repo: '^runtime$'
query: '(?i)is:open'
min_count: 2
|
Can we update the skills in dotnet/arcade-skills as well if that hasn't already been done? |
Make pre-signature retrieval failures visible and require scoped issue and pull request MCP searches in the live scanner eval. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 51814c28-9cf0-4574-83d5-0d3e31506cc9
|
Let me try to update the KBE skill here first - and see if it works... I'm still a bit hesitant completely disabling "curl" or similar to the agent, it limits what it can do quite a bit. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/ci-failure-scan.md:409
- This blanket rule is not applied to the Step 4.0 cache check immediately above:
test ... && cut ... | grep -Fxqstill runs withoutpipefail. Iffiled.tsvexists butcutcannot read it,grepsees no input and returns 1, so the agent treats the signature as new and can file a duplicate KBE. Addset -o pipefailto that cache-check invocation (and the analogous verification snippets) or validate the cache separately before using a no-match predicate.
[!NOTE] This review comment was created by GitHub Copilot.
- **Every piped shell command starts with `set -o pipefail`.** Do not redirect or suppress stderr. Verify the expected output shape before treating an empty result as valid.
.github/workflows/ci-failure-scan.md:410
- This blanket prohibition conflicts with the required same-run dedup cache writes at lines 257 and 275, which append to
/tmp/gh-aw/agent/filed.tsvwith>>. If an agent follows the new rule literally, it may omit those appends, so identical signatures are no longer cached and duplicate KBE issues can be emitted in one run. Restrict the prohibition to redirecting fetched responses, or explicitly allow append redirection for local workflow state.
[!NOTE] This review comment was created by GitHub Copilot.
- **No `>` or `-o` redirection.** Use `| tee /path/to/file`.
.github/workflows/evals/ci-failure-scan.eval.yaml:135
- The PR-search grader is too narrow for the shared instructions this eval copies.
create-kbe.instructions.mdrequires valid queries such asis:pr is:open in:title "<test-name>" "[ci-scan]"andis:pr is:open "<test-name>"; because this matcher insists on^is:open\s+", a compliant MCP call is rejected and the eval can fail despite doing the required search. Allow the optionalis:prandin:titlequalifiers (the tool is already PR-scoped).
[!NOTE] This review comment was created by GitHub Copilot.
query: '(?i)^is:open\s+".+'
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
.github/workflows/ci-failure-scan.md:409
- This new rule requires
set -o pipefailfor every pipeline, but the prompt still contains piped snippets that omit it, notably the Step 4.0 dedup check (cut ... | grep). Iffiled.tsvcannot be read, that pipeline can look exactly like the expected grep no-match and the agent can emit a duplicate, contrary to the fail-closed goal. Update the existing snippets and explicitly distinguish grep's expected exit 1 from an upstream/read failure before relying on this rule.\n\n> [!NOTE] This review comment was created by GitHub Copilot.
- **Every piped shell command starts with `set -o pipefail`.** Do not redirect or suppress stderr. Verify the expected output shape before treating an empty result as valid.
|
I tried to use the GitHub MCP in the skill as well, but that seems to be a bit more tricky (the copilot CLI environment doesn't always provide all the tools). So I think it would be better to do that in a separate change. I'd like to keep this PR as-is focused only on the ci-scan for now - merge it, and see how it works in real world. If it does well, then I would expand this to the other ci- workflows. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 51814c28-9cf0-4574-83d5-0d3e31506cc9
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 51814c28-9cf0-4574-83d5-0d3e31506cc9
There was a problem hiding this comment.
🟢 Approval recommended
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
.github/workflows/shared/create-kbe.instructions.md:53
- Variation 5’s example query omits
is:open, which conflicts with the surrounding guidance to search open issues and can pull in closed area trackers/noise. Includingis:openkeeps results scoped the same way as the other variations while still omitting the KBE label filter.
`SocketBlockingModeTransitionTests label:area-System.Net.Sockets`.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
| args: | ||
| owner: '^dotnet$' | ||
| repo: '^runtime$' | ||
| query: '(?i)^is:open\s+label:"Known Build Error"\s+".+' |
There was a problem hiding this comment.
The eval still pins MCP v1.5.0 while production pins v1.9.0, so these graders validate lexical search even though production uses semantic search_issues behavior. That can let this test pass while production selects different candidates or misses an existing KBE. Please align the eval image/digest with production and make the candidate-selection contract explicit. Also allow the type-qualified form prescribed by the mounted shared instructions, such as is:issue is:open ..., rather than requiring is:open to be the first token.
Note
This review comment was generated by GitHub Copilot.
| 11. **AzDO API: anonymous only.** Stay on `_apis/build/...`. Never call `_apis/test/...` or `vstmr.dev.azure.com` (both redirect to sign-in). | ||
| 12. **Don't add `area-*` references to issue titles.** Multi-area titles produce multi-label assignments from the labeler bot. | ||
| 13. **A failed command or tool call is not an empty result.** For shell pipelines, enable `pipefail`, use failure-reporting flags such as `curl -fsS`, and never suppress stderr. Distinguish an explicit error from a successful zero-result response; an expected `grep` no-match used as a predicate is not a retrieval failure. If a required fetch, search, or parse fails, correct the command and retry; if it still cannot produce a verified result, stop processing that signature and record `skipped: data retrieval failed: <source>`. If the failure happens before a signature exists (for example, while fetching the build list or timeline), stop processing that pipeline and record the same reason as a pipeline-level skip. Never conclude that no matching data exists from empty, missing, malformed, or truncated output. | ||
| 14. **All GitHub issue and pull-request searches and reads use the GitHub MCP.** Use `search_issues`, `search_pull_requests`, `issue_read`, and `pull_request_read`. Pass `owner: dotnet` and `repo: runtime` as tool arguments rather than adding a `repo:` qualifier to the query; put state, label, date, and text filters in the query. Do not reimplement these operations with `curl`, `gh`, or another shell command. |
There was a problem hiding this comment.
Production now requires semantic MCP issue search, which has a 10-request-per-minute bucket, while the shared lookup catalogue can issue roughly 17-19 searches for one unmatched signature. The immediate retry required above remains inside the same reset window, so the 11th search can fail twice and suppress a real KBE as data retrieval failed. Please short-circuit/reduce the catalogue or add explicit rate-limit-reset handling with a bounded wait path. The eval should also use the same v1.9.0 image so this behavior is exercised.
Note
This review comment was generated by GitHub Copilot.
| config: | ||
| disallowed: | ||
| - name: '^(bash|powershell)$' | ||
| command: '(?s)^(?!.*\b(?:curl|wget|Invoke-WebRequest|Invoke-RestMethod|iwr|irm)\b[^|;&\r\n]*https?://)(?!.*\b([a-z_][a-z0-9_]*)\s*=\s*["'']?https?://[^\s"'']+["'']?[\s\S]*\b(?:curl|wget|Invoke-WebRequest|Invoke-RestMethod|iwr|irm)\b[^|;&\r\n]*(?:\$\{\1\}|\$\1\b))[\s\S]*\b(?:curl|wget|Invoke-WebRequest|Invoke-RestMethod|iwr|irm)\b' |
There was a problem hiding this comment.
This allow-list only recognizes lowercase shell variable names. A compliant same-call fetch such as URL='https://example.test/data'; curl -fsS "$URL" is rejected even though it satisfies the intended literal-binding contract. Please accept normal shell identifiers with [A-Za-z_][A-Za-z0-9_]* and add an uppercase-variable fixture.
Note
This review comment was generated by GitHub Copilot.
| command: '(?s)(\b(?:curl|wget|Invoke-WebRequest|Invoke-RestMethod|iwr|irm)\b[^|;&\r\n]*(?:\$\(|`)|\bcurl\b[^|;&\r\n]*(?:--config(?:[=\s]|$)|-K(?:\S+|\s|$))|\bwget\b[^|;&\r\n]*(?:--input-file(?:[=\s]|$)|-i(?:\s|$))|\bxargs\b[^|;&\r\n]*\b(?:curl|wget)\b)' | ||
| - name: '^(bash|powershell)$' | ||
| command: 'gh (search|issue)' | ||
| command: '(?im)(\b(?:bash|sh)\b(?:\s+(?:--|-[a-zA-Z]+))*\s+(?:/tmp/|\./|\.\./)\S+|(?:\bsource|(?:^|[\s"'';&])\.)\s+(?:/tmp/|\./|\.\./)\S+|\b(?:bash|sh)\b[^;\r\n]*\s-c\s+["'']\s*(?:/tmp/|\./|\.\./)\S+)' |
There was a problem hiding this comment.
The bare-dot alternative is not anchored to shell command position, so benign commands such as find . /tmp/gh-aw/agent ... or ls . /tmp/... are misclassified as . /tmp/script sourcing and fail the eval. Please anchor . as an actual shell command token and add negative controls for ordinary commands that take both . and /tmp/... as arguments.
Note
This review comment was generated by GitHub Copilot.
## Summary - Route CI failure scanner issue searches through a repository-scoped MCP script that returns only issue numbers and author logins. - Require every returned candidate to be inspected with `issue_read` before making semantic duplicate decisions. - Remove direct built-in issue-search access and strengthen the scanner eval to enforce the wrapper path and fail-closed behavior. ## Dependency This PR depends on [#133958](#133958), which must merge first. The `/ci-eval` workflow restores evaluator inputs from `main` before checking out the PR head; without the prerequisite files already present on `main`, the wrapper, grader, and focused tests are removed during restoration and this PR's evaluator changes are not exercised. ## Motivation The built-in issue search result projection can omit author metadata. When that happens, existing bot-authored KBEs can be hidden from duplicate detection even though prompt guidance requests the author field. This makes the transport deterministic while leaving non-exact query formulation and semantic comparison to the model. This is narrower than #132619 for issue lookup: pull request searches continue to use the GitHub MCP tools, while issue searches use the dedicated wrapper. ## Scope limitation The eval's direct-search protection is pattern-based and cannot recognize every possible shell spelling or equivalent command. This is an existing limitation of the eval setup and is out of scope for this PR; the workflow's network policy and mandatory wrapper checks remain the enforcement mechanisms for this change. ## Validation - Compiled `ci-failure-scan` with gh-aw v0.86.2 and actionlint enabled. - Passed Vally 0.14 strict lint for `ci-failure-scan.eval.yaml`. - Passed actionlint v1.7.12 for `ci-eval.yml`. - Exercised the wrapper against live KBE searches and confirmed bot-authored candidates include their author logins. - Passed the focused Node test suite with 10 tests. > [!NOTE] > This pull request description was generated by GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Motivation
A failed shell-based issue search could produce no output and be interpreted as finding no matching KBE, allowing the scanner to create a duplicate. Using the configured GitHub MCP tools gives the agent structured results and explicit tool failures it can retry or handle as a failed retrieval.
For the Vally evals, the Docker-based stdio MCP process does not receive the workflow-provided authentication environment variable from the parent eval process. The workflow now creates a temporary Docker env file, passes its path to Vally, and removes it when the evaluation finishes so the MCP container receives the required environment.
Validation
github-search_issuescalls.Note
This pull request description was created with GitHub Copilot.